-
Notifications
You must be signed in to change notification settings - Fork 138
rfq: add price oracle certificate verification #1775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 20063273326Details
💛 - Coveralls |
77cad42 to
9910220
Compare
|
(Changed this from draft; I think the litd tests are failing for an unrelated reason.) |
|
(As pointed out by @ZZiigguurraatt, to be more precise: TLS support already existed for price oracles, but certificate verification was skipped entirely.) |
ffranr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other cases where we need more precise control over TLS behavior. For example:
taproot-assets/proof/courier.go
Lines 309 to 320 in a17a67a
| // serverDialOpts returns the set of server options needed to connect to the | |
| // server using a TLS connection. | |
| func serverDialOpts() ([]grpc.DialOption, error) { | |
| var opts []grpc.DialOption | |
| // Skip TLS certificate verification. | |
| tlsConfig := tls.Config{InsecureSkipVerify: true} | |
| transportCredentials := credentials.NewTLS(&tlsConfig) | |
| opts = append(opts, grpc.WithTransportCredentials(transportCredentials)) | |
| return opts, nil | |
| } |
With that in mind, I wonder if we could define a more general, reusable solution in something like the new rfq/tls.go file, especially given the need for configuration and the importance of which package owns this logic.
| name: "invalid custom certificate", | ||
| expectInsecure: false, | ||
| tlsConfig: &TLSConfig{ | ||
| Enabled: true, | ||
| InsecureSkipVerify: false, | ||
| TrustSystemRootCAs: false, | ||
| CustomCertificates: []byte(invalidCertificate), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I see the purpose of invalidCertificate here. It doesn't look like the test actually exercises its invalidity.
More broadly, do we need certificate examples in our unit tests at all? It seems like we're testing the behavior of the underlying TLS/certificate library rather than the logic we're adding on top of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these tests are super important, but they perform a decent sanity check of the configureTransportCredentials function -- basically, that it uses the supplied config to interact with the underlying libraries in the way that I expect. The tests are cheap, so why not, sort of.
I debated deleting the invalid certificate case, but wound up deciding to return an error in configureTransportCredentials when no valid custom certificates could be found anyway, so now the invalid certificate case exercises the invalidity of the certificate. 😄
2b3ac4f to
035a840
Compare
Introduces rfq/tls.go, which contains a basic TLSConfig type and default value of such. The default value, which for now only indicates that certificate verification should be skipped, is used in place of the 'dialInsecure' bool when setting up the price oracle RPC.
Adds both 'TrustSystemRootCAs' and 'CustomCertificates' to the rfq TLSConfig. The former indicates whether or not to trust the operating system's root CA list; the latter allows additional certificates (CA or self-signed) to be trusted. Also adds a basic unit test skeleton.
We don't skip certificate verification by default, and also default to trusting the operating system's root CA list.
Adds some basic test cases for configuring transport credentials.
Previously we would ignore this, but it's arguably better to return an error here that at least describes what happened.
Ensures that certificate verification is skipped when constructing a communication channel with the itest oracle harness.
Ensures the price oracle TLS toggle fits the existing pattern of flags defaulting to false.
I looked into this and agree that it's probably worth centralizing our TLS handling, but I'd consider it out of scope for this change set, which does resolve a concrete issue as-is. Maybe we can open a broader "refactor TLS handling" issue for that? I get the following hits for "crypto/tls" imports, as a rough metric: |
The mock oracle uses a self-signed certificate for TLS, but we're not concerned with having tapd verify it in the itest environment. This commit adds the 'experimental.rfq.priceoracletlsinsecure' flag added in lightninglabs/taproot-assets#1775 to the litd args list, instructing tapd not to verify certificates.
|
The LiT itest failure should be resolved by lightninglabs/lightning-terminal#1190. |
|
@ffranr: review reminder |
Adds certificate verification for TLS communication with price oracles, mostly following the suggestions proposed in #1278. Adds configuration options for skipping certificate verification, distrusting the operating system's root CA list, and using a custom certificate.
Resolves #1278.