-
Notifications
You must be signed in to change notification settings - Fork 746
crypto: rework CryptoProvider as struct #1628
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1628 +/- ##
==========================================
- Coverage 95.88% 95.88% -0.01%
==========================================
Files 78 78
Lines 16058 16044 -14
==========================================
- Hits 15398 15384 -14
Misses 660 660 ☔ View full report in Codecov by Sentry. |
d56bb37
to
a0810e0
Compare
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 think this is looking pretty good.
572f22b
to
babcd08
Compare
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.
Thanks for working on this! Looks great.
The unwrap
for checking cipher suite / TLS version compatibility still feels awkward, even with introducing the builder_with_protocol_versions
.
There was some prior discussion when the ConfigBuilder was first introduced, starting here: #564 (comment)
The suggestion was to have different types for TLS 1.2 cipher suites vs TLS 1.3 cipher suites. In fact we have that already, we just wrap them in an enum so they can share a type:
pub enum SupportedCipherSuite {
Tls12(&'static Tls12CipherSuite),
Tls13(&'static Tls13CipherSuite),
}
What if CryptoProvider
had two fields:
tls12_cipher_suites: Vec<&'static Tls12CipherSuite>,
tls13_cipher_suites: Vec<&'static Tls13CipherSuite>,
And the way to disable TLS 1.2 would be to provide an empty Vec
for tls12_cipher_suites
. That way we could do away with both the unwrap()
and builder_with_protocol_versions
.
Under the hood, the builder could then bundle those up into the enum SupportedCipherSuite
to minimize unnecessary diffs.
This seems like a good idea to me. I think I advocated at the time that there was no need to specify protocol versions anyway because this was implicitly expressed by the supply of cipher suites. I think that was deemed not explicit enough but just wanted to mention it here for completeness. But most of this discussion seems out of scope for this specific PR and seems mostly-orthogonal #1372 stuff? (I guess it becomes easier/more ergonomic with the changes in this PR but I still wouldn't want to see it become part of this PR.) |
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.
IMO this is a big improvement
We're working towards making `CryptoProvider` a struct holding distinct elements to be used for cryptography. To support this the `fill_random` fn needs to be lifted to a new trait, `SecureRandom`. We can hold a `&dyn SecureRandom` in the to-be-added struct to invoke as required for `fill_random`. Since the trait now provides additional context, the fn is renamed from `fill_random` to `fill`. This commit adds the new trait, includes `SecureRandom` in the existing `CryptoProvider` trait bounds, and updates the *ring*, aws-lc-rs, and provider example crypto providers to implement `SecureRandom`.
In preparation for moving to a struct based model where a `CryptoProvider` has a `&'static dyn SecureRandom` field, this commit splits the `SecureRandom` trait from the `CryptoProvider` trait. In its place `CryptoProvider` gets a `secure_random(&self)` fn that acts as a stand-in for what will be a field in the struct based approach.
We're working towards making `CryptoProvider` a struct holding distinct elements to be used for cryptography. To support this the `load_private_key` fn needs to be lifted to a new trait, `KeyProvider`. We can hold a `&dyn KeyProvider` in the to-be-added struct to invoke as required for `load_private_key`. This commit adds the new trait, includes `KeyProvider` in the existing `CryptoProvider` trait bounds, and updates the *ring*, aws-lc-rs, and provider example crypto providers to implement `KeyProvider`.
In preparation for moving to a struct based model where a `CryptoProvider` has a `&'static dyn KeyProvider` field, this commit splits the `KeyProvider` trait from the `CryptoProvider` trait. In its place `CryptoProvider` gets a `key_provider(&self)` fn that acts as a stand-in for what will be a field in the struct based approach.
This commit replaces the existing `CryptoProvider` trait with a `CryptoProvider` struct. This has several advantages: * it consolidates all of the cryptography related settings into one API surface, the `CryptoProvider` struct members. Previously the provider had methods to suggest default ciphersuites, key exchanges etc, but the builder API methods could override them in confusing ways. * it allows removing the `WantsCipherSuites` and `WantsKxGroups` builder states - the "safe defaults" are automatically supplied by the choice of a crypto provider. Customization is achieved by overriding the provider's struct fields. Having fewer builder states makes the API easier to understand and document. * it makes customization easier: the end user can rely on "struct update syntax"[0] to only specify fields values for the required customization, and defer the rest to an existing `CryptoProvider`. Achieving this requires a couple of additional changes: * The cipher suite and key exchange groups are now expressed as `Vec` elements. This avoids imposing a `&'static` lifetime that would preclude runtime customization (e.g. the tls*-mio examples that build the list of ciphersuites at runtime based on command line flags). * As a result of the `Vec` members we can no longer offer the concrete `CryptoProvider`s as `static` members of their respective modules. Instead we add `pub fn default_provider() -> CryptoProvider` methods to the `ring` and `aws-lc-rs` module that construct the `CryptoProvider` with the safe defaults, ready for further customization. [0]: https://doc.rust-lang.org/book/ch05-01-defining-structs.html#creating-instances-from-other-instances-with-struct-update-syntax
babcd08
to
cade9d3
Compare
When building a client config or a server config using the default provider we know that the ciphersuites will be compatible with any choice of protocol version. By having the default `builder` method configure itself with safe default versions, and offering a `builder_with_protocol_versions` for customization we can transition directly to `WantsVerifier` for these default provider builders, removing a `Result` that will never be an error and making the API more ergonomic in the common case.
cade9d3
to
9f73119
Compare
Looks like it was bounced from the cross compile task hitting some bad internet weather:
|
I guess we would put
We'd also need to put a |
This is a good point, though as you say:
So I think it's an okay policy decision to make.
Also a good point, and very achievable I think. But this does raise my estimate of how much work is involved. |
I think we'd also need to wrap this together with removing the |
Filed as a separate issue: #1659 |
This branch replaces the existing
CryptoProvider
trait with aCryptoProvider
struct. This has several advantages:CryptoProvider
struct members. Previously the provider had methods to suggest default ciphersuites, key exchanges etc, but the builder API methods could override them in confusing ways.WantsCipherSuites
andWantsKxGroups
builder states - the "safe defaults" are automatically supplied by the choice of a crypto provider. Customization is achieved by overriding the provider's struct fields. Having fewer builder states makes the API easier to understand and document.customization, and defer the rest to an existing
CryptoProvider
.Achieving this requires a couple of additional changes:
fill_random
andload_private_key
fns of theCryptoProvider
trait are broken out into separate traits,SecureRandom
andKeyProvider
, so that we can hold separate&dyn
references to them in the newCryptoProvider
struct.Vec
elements. This avoids imposing a&'static
lifetime that would preclude runtime customization (e.g. the tls*-mio examples that build the list of ciphersuites at runtime based on command line flags).Vec
members we can no longer offer the concreteCryptoProvider
s asstatic
members of their respective modules. Instead we addpub fn provider() -> CryptoProvider
methods to thering
andaws-lc-rs
module that construct theCryptoProvider
with the safe defaults, ready for further customization.Finally, to avoid an unnecessary
Result
unwrap when using the default crypto provider and default protocol versions, or customized protocol versions, theClientConfig
andServerConfig
defaultbuilder
fns transition directly toWantsVerifier
using default safe protocol versions. Customization of only protocol versions can be done with newbuilder_with_protocol_versions
fns that transition toWantsVerifier
without needing an unwrap. Using a custom provider requires handling theResult
that may occur if either the default safe protocol versions or provided custom versions can't be supported by the provider's ciphersuites.Updates #1372, #1603