Skip to content

Conversation

cpu
Copy link
Member

@cpu cpu commented Nov 24, 2023

This branch 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" 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 fill_random and load_private_key fns of the CryptoProvider trait are broken out into separate traits, SecureRandom and KeyProvider, so that we can hold separate &dyn references to them in the new CryptoProvider struct.
  • 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 CryptoProviders as static members of their respective modules. Instead we add pub fn provider() -> CryptoProvider methods to the ring and aws-lc-rs module that construct the CryptoProvider 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, the ClientConfig and ServerConfig default builder fns transition directly to WantsVerifier using default safe protocol versions. Customization of only protocol versions can be done with new builder_with_protocol_versions fns that transition to WantsVerifier without needing an unwrap. Using a custom provider requires handling the Result 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

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d963be3) 95.88% compared to head (9f73119) 95.88%.

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.
📢 Have feedback on the report? Share it here.

@cpu cpu mentioned this pull request Nov 24, 2023
4 tasks
@cpu cpu force-pushed the cpu-1372-crypto-provider-struct branch from d56bb37 to a0810e0 Compare November 27, 2023 16:00
Copy link
Member

@djc djc left a 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.

@cpu cpu force-pushed the cpu-1372-crypto-provider-struct branch from 572f22b to babcd08 Compare November 28, 2023 21:20
Copy link
Contributor

@jsha jsha left a 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.

@djc
Copy link
Member

djc commented Nov 30, 2023

And the way to disable TLS 1.2 would be to provide an empty Vec for tls12_cipher_suites.

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.)

Copy link
Member

@ctz ctz left a 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

cpu added 5 commits November 30, 2023 10:22
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
@cpu cpu force-pushed the cpu-1372-crypto-provider-struct branch from babcd08 to cade9d3 Compare November 30, 2023 15:39
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.
@cpu cpu force-pushed the cpu-1372-crypto-provider-struct branch from cade9d3 to 9f73119 Compare November 30, 2023 15:42
@cpu cpu enabled auto-merge November 30, 2023 15:43
@cpu cpu added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@cpu
Copy link
Member Author

cpu commented Nov 30, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks now

Looks like it was bounced from the cross compile task hitting some bad internet weather:

0.2.5: Pulling from cross-rs/i686-unknown-linux-gnu
docker: Get "https://ghcr.io/v2/cross-rs/i686-unknown-linux-gnu/manifests/sha256:ee34baf5d28e93ff7fcc6cdceb44459c30968bebac4cb3a83bdf672e33ddb1f4": dial tcp 140.82.[11](https://github.com/rustls/rustls/actions/runs/7048898000/job/19186114995#step:5:12)2.33:443: i/o timeout.

@cpu cpu added this pull request to the merge queue Nov 30, 2023
Merged via the queue into rustls:main with commit a719178 Nov 30, 2023
@cpu cpu deleted the cpu-1372-crypto-provider-struct branch November 30, 2023 16:02
@djc djc mentioned this pull request Dec 1, 2023
@ctz
Copy link
Member

ctz commented Dec 1, 2023

What if CryptoProvider had two fields:

tls12_cipher_suites: Vec<&'static Tls12CipherSuite>,
tls13_cipher_suites: Vec<&'static Tls13CipherSuite>,

I guess we would put tls13_cipher_suites before tls12_cipher_suites on the wire? That seems like a bit of a policy decision that is currently not made by the library. Though -- hopefully -- nobody does version negotiation between TLS1.2 and TLS1.3 incorrectly based on the offered cipher suites?

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.

We'd also need to put a &'static SupportedProtocolVersion in each Tls12CipherSuite and Tls13CipherSuite, and then require that everyone fills in the right one? Otherwise #224 would become impossible.

@jsha
Copy link
Contributor

jsha commented Dec 1, 2023

I guess we would put tls13_cipher_suites before tls12_cipher_suites on the wire? That seems like a bit of a policy decision that is currently not made by the library.

This is a good point, though as you say:

hopefully -- nobody does version negotiation between TLS1.2 and TLS1.3 incorrectly based on the offered cipher suites?

So I think it's an okay policy decision to make.

We'd also need to put a &'static SupportedProtocolVersion in each Tls12CipherSuite and Tls13CipherSuite, and then require that everyone fills in the right one? Otherwise #224 would become impossible.

Also a good point, and very achievable I think. But this does raise my estimate of how much work is involved.

@ctz
Copy link
Member

ctz commented Dec 1, 2023

I think we'd also need to wrap this together with removing the tls12 feature flag: Tls12CipherSuite is gated behind that (and right now enum SupportedCipherSuite is insulating most users from that).

@jsha
Copy link
Contributor

jsha commented Dec 1, 2023

Filed as a separate issue: #1659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants