Encrypted Client Hello support (client only) #1718
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1718 +/- ##
==========================================
- Coverage 94.18% 94.08% -0.10%
==========================================
Files 96 97 +1
Lines 20648 21516 +868
==========================================
+ Hits 19447 20243 +796
- Misses 1201 1273 +72 ☔ View full report in Codecov by Sentry. |
Definitely worth looking at the boringssl test suite -- there are ECH tests there that should be ready to go (albeit with some neccessary additions to our bogo_shim.rs). Currently they are disabled here: https://github.com/rustls/rustls/blob/main/bogo/config.json#L34 |
Oh great! Thanks for the pointer. I will focus attention there 👍 |
|
Might also be useful to do a bogo update before this work? I assume ECH tests may have evolved since June. |
There was a problem hiding this comment.
Looking good! I have a bunch of nits (hope that's useful at this stage) but not much substantial feedback.
I feel like the commit history overindexes a bit on small commits and might benefit from some squashing of things together so more context is available in commits.
|
Now that a bunch of the smaller work has been pulled out and merged I will try to find time to rebase this and address the remaining feedback from djc's first review pass. As a quick note: ISRG wants me to focus on finishing some of the remaining no-alloc/async work ahead of prioritizing ECH completion, so I'll be picking up work for this branch after making more progress on that front. |
|
|
I needed a break from my current tasks so I rebased this branch and addressed some of the low hanging review feedback. I haven't resolved all of it yet (so probably not ready for a second pass). |
|
It all looks good to me. I think the Server Name Enum choices are good. I just threw stuff on the heap because I didn't really know what was going to be required. You're right that the spec is complicated and out-of-order when you sit down to write the implementation. I'd recommend reading it all the way through once before writing any more code, just to keep it all in your head. That's what I had to do a few times, anyway. The HPKE trait is good, too. I wrote mine when rustls only had ring and didn't let one vary the dependencies, and was assuming ring would add that. I see you already have the OpenSSL server in your tests. You can find some variations that will trigger HRR via your HPKE choice here: Those might be good for connect tests, and as a way to go once the server code is added. As a practical matter, I think you will have to implement the handshake compression stuff. I showed up because I saw that you've added support for Kyber/X25519. In that case, you will really need it. |
|
Hey sayrer,
Awesome. Thanks so much for taking a look. I'm focused on this branch again as of the past couple of days and hope we can get it across the line soon.
Makes sense 👍
Yeah :-( That's what I ended up doing too. It's some comfort to hear I wasn't alone in feeling this. From my perspective it feels like it's a bit too late in the document's process to try and fix the "editorial narrative" to be closer aligned to what an implementer might want. That's unfortunate but c'est la vie.
Useful pointers, thank you 🔖
100% agreed. IIRC your initial branch had an implementation of the compression (or at least the start of one). I think we'll want to revive that before merging this work. I left it out only to try and keep the scope contained while figuring out the bigger picture. Thanks again for the feedback (and the original work!). |
I think it was a little more ready for that part than this branch, but it didn't do it either (I think it did sort the extensions to prepare, I think the draft-07 one was better). It's been a while, but it really wanted some Iter features that were nightly-only at the time. I think you can probably get a good version now. |
I don't think you need to be so cautious. Firefox, Chrome, and Cloudflare are all shipping it. 
|
ECH has gone through working group last call so it's unlikely to change significantly beyond version -18 |
Pushed some progress from my local branch:
I'm still working through bogo coverage and that's now my major focus. I've grokked the overall test approach and done some bogo_shim updates and initial results indicate there's a ways to go w.r.t protocol compatibility in the cornercases. In particular the client ECH bogo tests are flagging that the inner hello contains some extensions it shouldn't, and most importantly, that I believe I've broken resumption in the presence of ECH. Hope to have some more updates on this front shortly. |
cpu
force-pushed
the
cpu-client-ech
branch
2 times, most recently
from
4bba847
to
4feb74f
Compare
Benchmark resultsInstruction countsSignificant differencesClick to expand
Other differencesClick to expand
Wall-timeSignificant differencesClick to expand
Other differencesClick to expand
Additional informationCheckout details:
|
The SVCB/HTTPS record handling in hickory-dns 0.24 was stripping the TLS encoded list prefix from the `ECHConfigList` that is serialized into DNS records. This meant our previous `ech.rs` connect test was subtly wrong: it would only ever deserialize a single `EchConfig` from what it found in DNS. This commit updates Rustls to: 1. Use the new `EchConfigListBytes` type from pki-types to represent what it gets from DNS. Soon we will have more API surface expecting this type. 2. Use a hickory-dns release with some upstream fixes that ensure we get the correct wire-encoding of the `ECHConfigList`. 3. Update the ech connect tests unit tests to assert all of the ECH configs that may be found are the correct version.
For ECH GREASE it's convenient to be able to generate throw-away HPKE key-pairs. Similarly, down the road we may want to support generating server-side ECH configurations, which will require key generation. This commit updates the `Hpke` trait to add a `generate_key_pair()` fn for this purpose. Both the aws-lc-rs and provider-example HPKE trait impls are updated to add this functionality. Along the way I also copied the smoke test and fips unit tests from the aws-lc-rs HPKE provider to the provider-example HPKE provider so that there's coverage of key generation for both. We won't use key generation in the rustls-provider-tests crate since the RFC test vectors come with private key material to use.
This commit introduces the first piece of ECH support to the public API: configuring a client connection with an optional ECH config. Since ECH requires TLS 1.3 we offer a configuration entry-point `with_ech(...)` on the `ConfigBuilder<ClientConfig, WantsVersions>` state, and fix the supported protocol versions to only TLS 1.3. Handling configurations in a way that will be forward compatible and adhering to the draft spec requires some extra care. Notably we need to be able to treat ECH configs in an ECH config list with an unsupported version as opaque blobs. This requires splitting our config representation into an enum with two variants: one for a recognized ECH config and one for an unsupported one. Similarly we need to process optional extensions in ECH config contents to ensure that: 1. There MUST NOT be duplicate extensions. 2. We MUST parse and check for unsupported mandatory extensions. An example TLS client that fetches ECH configurations using DNS-over-HTTPS and configures Rustls to use ECH is added to the examples crate. The HTTPS-over-DNS lookup in that example requires adding a new dep on hickory-dns. We use the recently added aws-lc-rs backed HPKE implementations default selection of HPKE suites for the ECH config. As of this commit the ECH configuration is not used (except as a TODO to avoid a `dead_code` warning). Subsequent commits will introduce actual ECH offering and protocol support.
In order to support ECH we need to be prepared for `emit_client_hello_for_retry` to return an `Error` where it was otherwise infallible - this can occur (for e.g.) if the HPKE provider we use for ECH encryption fails. This commit changes `emit_client_hello_for_retry` to return `NextStateOrError` instead of `NextState` in preparation for that.
Previously we separately iterated the `input.hello.sent_extensions` to track our sent extensions before constructing a `ClientHelloPayload` that contained them. The `ClientHelloPayload` was constructed in-line for the `HandshakeMessagePayload` towards the end of `emit_client_hello_for_retry`. To support ECH we will want to do some additional work with the `ClientHelloPayload`, and so this commit does some minor rearranging to facilitate this.
This commit continues the implementation of client-side ECH support. We now act on the provided ECH configuration when available to produce an appropriate outer/inner client hello. Adding this support requires introduction of a new `EchState` struct for maintaining the required handshake state specific to ECH. We use this in combination with new types for representing the ECH extensions to produce the outer and inner client hello messages. We do not yet properly handle confirmation of ECH acceptance, resulting in decrypt errors when offering ECH. Subsequent commits will update hello retry request and server hello handling to fix this.
This commit continues the implementation of client-side ECH by processing the result of offering ECH in the non-HRR case. This involves processing the received server hello, attempting to derive and match a shared secret indicating ECH acceptance, and forwarding on our understanding of ECH status (not offered, offered and rejected, or offered and accepted) to later steps of the handshake. If we arrive at the end of the handshake and our ECH offer wasn't accepted, then we emit an appropriate alert and return an error potentially containing ECH configs to retry with from the server's response. If our offer _was_ accepted, we change out the handshake transcript, sent extensions and client hello as if the inner client hello was used as the outer client hello. TLS handshaking continues as normal from that point. This level of support is sufficient for the ech-client example to successfully use ECH with the defo.ie test server. Importantly we do **not** yet handle the case where we offer ECH and the server sends a hello retry request. This support will follow in a subsequent commit.
This commit continues the implementation of client-side ECH by supporting the case where we offer ECH and the server replies with a hello retry request (HRR). In this case we detect ECH acceptance in a slightly different manner. If ECH was accepted we update the separate ECH transcript using the received HRR and proceed to offer ECH again in our retried hello. After this point ECH acceptance is handled as normal. This completes the primary functionality of client-side ECH. The remaining work involves GREASE ECH and extension compression.
In the situation where an ECH config is not available some clients may wish to send a "GREASE" ECH extension as an anti-ossification mechanism (see RFC 8701[0] for more information on the general concept of GREASE). To support this we update the configuration to accept an optional ECH mode instead of an optional ECH config. The ECH mode has a variant for enabling ECH that holds an ECH config, and a separate variant that holds only what's required to emit a GREASE ECH extension. We don't automatically fall back to GREASE if ECH configs are provided but none are compatible. If desired this should be handled by the caller. Similarly we don't default to sending GREASE, it is opt-in. [0]: https://www.rfc-editor.org/rfc/rfc8701
* Use docopt to make it feasible to provide args/flags * Add flags for choosing CA cert, DNS-over-HTTPS server, etc * Add config for performing placeholder "GREASE" ECH for hosts without ECH configs, doing so by default for hosts without ECH configs, and forcing it if desired * Add config flag for specifying a ECH config from disk. * Add flag for specifying how many requests to do, to test resumption. * Asserts on the expected ECH status after handshaking.
This matches the rustls crate's current default provider choice.
A `ClientConfig` can only be considered FIPS compatible when using ECH if the configured HPKE suite is FIPS compatible. This commit updates `ClientConfig::fips()` to add that consideration. Similarly, it may be helpful to know if a given client connection was created from a FIPS compatible `ClientConfig`. To do this we update the client config data to stash the `ClientConfig::fips()` result when creating a new client connection. A new `ClientConnection::fips()` accessor returns that stashed value.
The bogo shim requires some updates to support new command line flags. Additionally in order to be able to assert some details in errors (e.g. that an ECH required err contained expected retry configs) we have to pipe the `Options` struct deeper into the client/server processing logic. Since `*ring*` has no HPKE provider, it can't do ECH, which means we need an unfortunate number of cfg guards on imports/logic in the shim related to HPKE/ECH. C'est la vie. We can revisit if `*ring*` gains static ECDH or HPKE. Beyond these changes, it's worth discussing the ignored tests. They're either not applicable, or need upstream bogo fixes: "TLS-ECH-Server*": We ignore all the TLS-ECH-Server tests. We haven't implemented server side ECH yet "TLS-ECH-Client-ExpectECHOuterExtensions" "TLS-ECH-Client-CompressSupportedVersions": These rely on extension compression between inner/outer hellos. NYI. "TLS-ECH-Client-SelectECHConfig" "TLS-ECH-Client-NoSupportedConfigs" These are meant to test unsupported configs are handled correctly: we happen to support the HPKE ciphersuites that make them "unsupported". There's a fix for this upstream we can take later. "TLS-ECH-Client-SkipInvalidPublicName*": Our name validation allows underscores in names. We also don't fallback to GREASE when there are no valid ECH configs. "TLS-ECH-Client-NoSupportedConfigs-GREASE": We don't fallback to GREASE for no ECH configs. "TLS-ECH-Client-Reject-ResumeInnerSession-TLS13": This test expects an unexpected extension error in the resumption connection, but this only happens if the outer hello didn't include GREASE PSK. BoringSSL's impl doesn't. Ours does. As a result we produce `:ECH_REJECTED:` instead of :UNEXPECTED_EXTENSION:` and have to ignore this test. "TLS-ECH-Client-TLS12-RejectRetryConfigs" "TLS-ECH-Client-Reject-NoClientCertificate-TLS12" "TLS-ECH-Client-Reject-TLS12" "TLS-ECH-Client-Reject-ResumeInnerSession-TLS12" "TLS-ECH-GREASE-Client-TLS12-RejectRetryConfigs" "TLS-ECH-Client-Reject-EarlyDataRejected-TLS12" "TLS-ECH-Client-Reject-NoClientCertificate-TLS12-Async" We never offer/support TLS 1.2 when using ECH. There's no fallback to plaintext or GREASE for a server that won't support TLS 1.3
This avoids breaking our MSRV task due to the later versions of hickory-dns (née trust-dns) that use workspace inheritance. A workspace-level patch must be resolved even for crates that don't require the patched dep, and resolving the patch for 0.22+ requires an MSRV of 1.64+ This can be reverted if there's a finalized 0.25 with the SVCB fixes, or a pre-release, made available.
|
@djc Do you think you'll have a chance to review this branch this week? |
|
Yup! Sorry for not getting to it sooner, I'll keep it at the top of my list for today/tomorrow. |
|
Thanks! Appreciated |
I have this work staged in a draft PR on my fork that targets this branch as its base. |
| Ok(Self { config, suite }) | ||
| } | ||
|
|
||
| fn select_config_and_suite( |
There was a problem hiding this comment.
In this commit, it's not clear to me why making this a separate associated function is useful.
rustls/src/client/ech.rs
Outdated
| #[cfg(feature = "logging")] | ||
| use crate::log::{debug, warn}; | ||
| use crate::msgs::codec::{Codec, Reader}; | ||
| use crate::msgs::handshake::EchConfig as EchConfigMsg; |
There was a problem hiding this comment.
Is there a strong reason to keep the two types with the same name? Maybe the msgs type should be called _Payload or something?
There was a problem hiding this comment.
I don't think there's a strong reason. I was trying to match to the names from draft-ietf-tls-esni-18 §4, but I think the enum arrangement already makes this not 1:1 with the simple presentation language in the spec.
Calling it EchConfigPayload sounds reasonable to me 👍 I will add a rename commit up-front and remove the aliased use's.
rustls/src/client/hs.rs
Outdated
| let _config = &ech_config.config; | ||
| Some(()) | ||
| }); | ||
| let ech_state = match config.ech_config.as_ref() { |
There was a problem hiding this comment.
Hmm, I think it might make things more awkward? I can't use an if let / else to match the &EchMode given to the map closure because of MSRV, so I'd still need a match { } in the map body making it about the ~same amount of code, but more annoying to deal with the fallibility of the EchState::new().
Am I overthinking what you had in mind?
| ech_state: Option<EchState>, | ||
| server_hello_msg: &Message, |
There was a problem hiding this comment.
Style nit: feels like server_hello_msg should come earlier in this list? At least before ech_state, which seems like it would be longer lived.
rustls/src/client/tls13.rs
Outdated
| @@ -146,6 +149,10 @@ pub(super) fn handle_server_hello( | |||
|
|
|||
| let shared_secret = our_key_share.complete(&their_key_share.payload.0)?; | |||
|
|
|||
| // TODO(@cpu): Handle confirmation of ECH. | |||
There was a problem hiding this comment.
As discussed a while ago, I question the value of leaving these loose ends in main branch history (relative to squashing more things here).
There was a problem hiding this comment.
I think this is the most awkward join point and it could be resolved by squashing the "ech: construct ECH client hello" commit with the subsequent "ech: confirm acceptance of ECH offer" commit.
I'll do that in the rebase that applies your feedback. 👍
| @@ -1198,16 +1204,7 @@ pub struct ServerHelloPayload { | |||
|
|
|||
| impl Codec<'_> for ServerHelloPayload { | |||
| fn encode(&self, bytes: &mut Vec<u8>) { | |||
| self.legacy_version.encode(bytes); | |||
There was a problem hiding this comment.
On the other hand, this is something I might do in a separate commit.
There was a problem hiding this comment.
I think I'll end up with dead code warnings if I try to do that before implementing confirmation, or I'd have to implement a watered down version that only adds Encoding::Standard. In the latter case having that separated seems like it has less value. Do you feel strongly about this one?
| confirmation_transcript.current_hash(), | ||
| ); | ||
|
|
||
| Ok( |
There was a problem hiding this comment.
(Same thing for the Ok-wrapping here.)
| fn hello_retry_request_conf(retry_req: &HelloRetryRequest) -> Message { | ||
| Self::ech_conf_message(HandshakeMessagePayload { | ||
| typ: HandshakeType::HelloRetryRequest, | ||
| payload: HandshakePayload::HelloRetryRequest(retry_req.clone()), |
There was a problem hiding this comment.
This will shake out the same as the server_hello_conf fn - see my reply there.
| // See draft-ietf-tls-esni-18 7.2.1: | ||
| // <https://datatracker.ietf.org/doc/html/draft-ietf-tls-esni-18#name-sending-helloretryrequest-2> | ||
| Encoding::EchConfirmation => { | ||
| let extensions = LengthPrefixedBuffer::new(ListLength::U16, bytes); |
There was a problem hiding this comment.
Style nit: maybe take this out of the match arm?
| // If we haven't offered ECH, and have no ECH state, then consider whether to use GREASE | ||
| // ECH. | ||
| (EchStatus::NotOffered, None) => { | ||
| if let Some(EchMode::Grease(grease_config)) = &config.ech_mode { |
Description
This branch brings encrypted client hello (ECH) compatibility to Rustls clients based on draft-ietf-tls-esni-18.
Closes #199
Limitations
It does not add any support for writing Rustls' servers that accept ECH (in either shared or split mode). I think client support is the most important at this stage, and this branch is already very large/complex. We will track server-side support in Server-side Encrypted Client Hello (ECH) support #1980
Extension compression is implemented separately in ech: implement inner hello extension compression cpu/rustls#6
Comparison to previous PRs:
Server Name Enum
Initially we expected to update the
ServerNameenum to have a variant for ECH - I tried this initially and found that it didn't fit the design of ECH very well. Mainly the issue is that we need to carry a lot of additional state beyond the inner SNI name. Putting that state into theServerNameis challenging, and an impedance mismatch with the existing usage. Having separate state and the ECH variant inServerNamemeant having two pieces of information and having to contend with what to do if one is present but not the other. This is also made increasingly complex since we lifted theServerNametype into the pki-types crate. As a point of reference, you can compare this branch with the approach in #663 where aServerIdentityenum stands in forServerNameand contains aEncryptedClientHello(Box<EncryptedClientHello>)variant that holds aBoxof heap allocated state.Credit where credit is due
I started this branch by first adopting work from what @sayrer started in #663. Many thanks for kicking this effort off and giving me a solid foundation to start with!
Some major changes compared to that branch:
ServerNamevariant for holding the inner SNI name and associated state.main, adjusting for variousCodecchanges and associated code drift.Purposethat lets us share the message encoding logic as much as possible with the exception of the parts that are unique for ECH confirmation.