WIP: Encrypted Client Hello support (client only) #1718
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1718 +/- ##
==========================================
- Coverage 95.48% 93.52% -1.96%
==========================================
Files 86 95 +9
Lines 18664 19950 +1286
==========================================
+ Hits 17821 18658 +837
- Misses 843 1292 +449 ☔ 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. |
I traced this down to a limit that aws-lc-rs was placing on the maximum allowable |
cpu
force-pushed
the
cpu-client-ech
branch
2 times, most recently
from
7903425
to
97b8911
Compare
I think I've gotten to the bottom of 4 of the 5 of these ignored tests (still TODO: |
cpu
force-pushed
the
cpu-client-ech
branch
2 times, most recently
from
f7aa7c0
to
4fec0a1
Compare
Done.
For the curious, for 3 of the 5 tests ( For 1 of the 5, That means there's just one bogo failure left to chase down, |
cpu
force-pushed
the
cpu-client-ech
branch
2 times, most recently
from
944df73
to
1fa3a32
Compare
Fixed; I was using the wrong transcript/random in one more place related to the early data key schedule w/ ECH. All the bogo tests I think are relevant are now passing so I'm going to switch my focus to implementing an HPKE provider backed by aws-lc-rs. |
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.
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 `provider-example` crate. When we've implemented an ECH provider using the `aws-lc-rs` or `*ring*` we can move the example - presently we're relying on the RustCrypto backend of `hpke-rs` and so it's a better fit for the provider example crate. An updated pki-types dependency is required for the new shared `EchConfigListBytes` type, representing raw TLS-encoded ECH configuration data in list form. 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.
Getting bogo test coverage working for ECH requires taking a (temporary) dev-dep. on the provider-example crate so we can use the Rust Crypto HPKE provider. This in turn means that we now have two Rustls versions in tree, the src crate and the older version being used by the provider example by way of hickory-dns. This will fall away when proper HPKE support is implemented with one/both built-in crypto providers but in the meantime requires some small adjustments in CI and the runme script. In general the scope of a proper HPKE impl will be much less invasive than implementing ECH so we can use this hack for now and revisit shortly. The bogo shim also 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. 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
|
First pass for this up for review: #1963 I will rebase this branch on top and remove the provider-example workaround hacks shortly. |
Description
This is a work in progress branch that brings encrypted client hello (ECH) compatibility to Rustls clients based on draft-ietf-tls-esni-18.
Updates #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.
The only HPKE provider we have available at the moment is based on HPKE-RS with a Rust Crypto backend. This means the demo application is confined to the
provider-examplecrate. For production usage we need an HPKE provider based on ring and AWS-LC-RS. This will be handled in a separate PR, and considered a prereq for merging this one.Extension compression isn't implemented yet (see Section 5.1) - this is a "MAY" I've left out for while getting the core protocol implemented. We should try to implement this pre-merge.
TODOs
Bogo test coverageFigure out the aws-lc-rs provider panicFigure out the 5 remaining bogo tests I think require adjustment on our siderustls-pemfilesupport.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.