Skip to content
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

client-side encrypted client hello (ECH) support #485

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Conversation

cpu
Copy link
Member

@cpu cpu commented Nov 6, 2024

Client ECH support

This branch adds initial support for customizing a rustls_client_config_builder instance to use encrypted client hello
(ECH). Presently this is only supported with the aws-lc-rs crypto provider.

In sum it adds:

  • New rustls_hpke type for representing a collection of HPKE suites as an opaque struct.
  • New rustls_supported_hpke() function for returning a const pointer to a rustls_hpke if available. With the aws-lc-rs backend this is a wrapper around rustls::crypto::aws_lc_rs::hpke::ALL_SUPPORTED_SUITES. For other providers it returns NULL.
  • New rustls_client_config_builder_enable_ech_grease client config builder function for configuring ECH GREASE using a rustls_hpke. This chooses the HPKE suite at random, and so brings in a dep on the rand crate. We could come up with a simpler selection mechanism if avoiding the dependency is preferred.
  • New rustls_client_config_builder_enable_ech client config builder function for configuring ECH using a rustls_hpke and a TLS encoded ECH config.

Additionally the client.c example is updated to add RUSTLS_ECH_GREASE and RUSTLS_ECH_CONFIG env vars. The former when non-zero configures ECH GREASE if possible, the latter configures ECH with the TLS encoded config list read from the provided file path.

To facilitate a reliable cross-platform way to fetch ECH configs a small ech-fetch test program is added that accepts a domain and (optionally) a file name and writes the TLS encoded ECH config for that domain to the file after performing the appropriate DNS-over-HTTPS lookup. This requires a new dev-only dep on HickoryDNS and Tokio.

Testing

Test locally with:

make
cargo test --test ech_fetch -- research.cloudflare.com /tmp/research.cloudflare.com.ech.configs.bin
RUSTLS_ECH_CONFIG_LIST=/tmp/research.cloudflare.com.ech.configs.bin \
  RUSTLS_PLATFORM_VERIFIER=1 \
  ./target/client research.cloudflare.com 443 /cdn-cgi/trace 2>/dev/null \
    | grep "sni="

You should see output that includes:

sni=encrypted

You may have to run this a few times to avoid an error of the form: "content length header not found". The reason for this is that the existing client.c HTTP handling is somewhat primitive and can't handle chunked responses like this server generates.

The existing client code only successfully handles an HTTP response if either:

  1. We get the full plaintext and read a connection close from the server in the first iteration of send_request_and_read_response. In this case we dump the plaintext to stdout without processing any HTTP headers and so it "works" with chunked encoding responses (mostly by accident).
  2. We buffer plaintext until we've found an HTTP body, and then find the content header in the header data, using that to determine how to proceed. This fails for the two ECH servers I typically test with (defo.ie and research.cloudflare.com) because both use chunked encoding and so never send a content length header.

This buggy handling can be replicated without ECH under the right conditions, but with ECH, it's much more frequent that we miss the happy accident case (number 1 above.).

Because of this I've not added CI coverage of running the test client. I think it makes more sense to land this as-is and break out a separate refactoring of the client.c HTTP handling. The code is a little bit messy and could use a more thoughtful rework (maybe as a state machine?).

TODO

@cpu cpu self-assigned this Nov 6, 2024
@ctz ctz self-requested a review November 18, 2024 18:22
@ctz
Copy link
Member

ctz commented Nov 25, 2024

  • We could use a small Rust helper program w/ HickoryDNS to fetch the configs and use those for testing. That's basically what I've done in this branch and it has its own downsides: it's awkward, the dev-dep is large, and it's breaking MSRV tests.

The MSRV tests may have shaken out now?

The other option is to hit cloudflare's DNS API? like

$ curl -vH "accept: application/dns-json" "https://cloudflare-dns.com/dns-query?name=research.cloudflare.com&type=HTTPS" | jq .Answer[0].data

@ctz
Copy link
Member

ctz commented Nov 25, 2024

  • I'm not sure I love the hpke_suite representation. It feels awkward for the consumer and I think I can come up with something that will be less painful to use. I think the main pain points right now are the way rustls_client_config_builder_enable_ech() takes ownership of the inputs, and how it expects a contiguous array of suite pointers as input, but the provider-side API gives them out one index at a time with rustls_aws_lc_rs_hpke_get().

I would be minded to minimise this API as much as possible. I haven't fully thought this through, but how about:

  • one call to get an opaque object for aws-lc-rs-backed HPKE (ie, it's rustls::crypto::aws_lc_rs::hpke::ALL_SUPPORTED_SUITES under the covers, and could be static lifetime)
  • rustls_client_config_builder_enable_ech_grease accepts one of those, chooses a random suite, generates a new key, etc.
  • rustls_client_config_builder_enable_ech accepts one of those, plus the echconfiglist encoding.

(I had a quick look at curl's ECH docs, there is no way of specifying any HPKE suite, or seeing what suite is used AFAIK)

@cpu
Copy link
Member Author

cpu commented Nov 25, 2024

cpu force-pushed the cpu-client-ech branch from f5b09bd to 5ee21ff

just rebasing, no substantive iteration yet.

@cpu
Copy link
Member Author

cpu commented Nov 27, 2024

The MSRV tests may have shaken out now?

Yup!

The other option is to hit cloudflare's DNS API?

I gave this more thought and realized that it's beneficial to do as much lifting as possible in Rust code because it's cross-platform and avoids me having to fight through Powershell on Windows ( 🤢 ). I think the Hickory DNS helper util is probably the best option with that in mind.

I would be minded to minimise this API as much as possible. I haven't fully thought this through, but how about:
...

This was helpful input. I implemented the API you suggested and it was much simpler. I collapsed everything into one commit as a result. PTAL.

Something's a bit buggy with client.c and I'm seeing some requests fail with missing content length in the response. I'm wondering if we're reading a partial response and need to do more buffering? Unclear: haven't done any investigation yet.

I figured this out and wrote an update in the PR description.

TLDR: the existing client.c send_request_and_read_response() function isn't quite right. It needs a rework to support chunked transfer encoding. I started working on that in this branch and got something crude enough that it was functional but realized this area of the code needs a pretty substantial amount of love. If folks are open to it I'd like to defer that to its own follow-up PR.

src/rustls.h Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Nov 28, 2024

Figure out a solution for MSRV 1.71 - presently using 1.71 or 1.72 on this branch produces an internal compiler error. I haven't investigated upstream yet. For now I switched CI to use 1.73 so I can chase this down separately.

I was able to reproduce the problem upstream in Rustls building the ech client example with 1.71 and 1.72. I reported the ICE to the Rust team here: rust-lang/rust#133578

We can either block on a resolution for this upstream, land a #[used] workaround in Rustls and block on that, or declare MSRV 1.73 for this repo as I've done in this commit. I think which choice will be best likely depends on what kind of response 133578 generates.

@cpu cpu marked this pull request as ready for review November 28, 2024 17:12
@cpu cpu force-pushed the cpu-client-ech branch 3 times, most recently from 4da2330 to 2f47234 Compare December 2, 2024 15:11
@cpu
Copy link
Member Author

cpu commented Dec 2, 2024

Figure out a solution for Rustls HPKE suites with MSRV 1.71 - presently using 1.71 or 1.72 on this branch produces an internal compiler error. See rust-lang/rust#133578

The upstream Rust project isn't interested in fixing bugs in old compilers (sensible!) so I think we're left with ~two options:

  • Adding workarounds upstream in Rustls and then cutting a point release. This doesn't seem like a good trade-off. We're not 100% confident in the cause, or the workarounds, and there isn't any indication other projects have bumped into this ICE so far. Either due to the conditions required, or ECH related features being fairly niche.
  • Increasing rustls-ffi's MSRV to 1.73. This is easy to do and requires no upstream changes. The userbase for rustls-ffi is considerably smaller than Rustls and I expect much more able to take the MSRV increase.

I vote for the latter option and have pushed a corresponding update.

@cpu cpu changed the title wip: client-side encrypted client hello (ECH) support client-side encrypted client hello (ECH) support Dec 4, 2024
@cpu
Copy link
Member Author

cpu commented Dec 9, 2024

@ctz Would you be willing to +1 this branch?

I pulled the CI improvements out into a follow-up issue #498 There's work in-flight to unblock this test coverage but I'd like to merge this before waiting on that stuff if possible.

cpu added 4 commits December 12, 2024 11:11
We can't derive `Default` on `ClientConfigBuilder` because we want to
enable SNI by default. Instead, implement it by hand. This allows
simplifying the construction sites to use the default except for the
~1/2 fields needing to be customized.

Along the way, change where we initialize the default protocol versions.
Previously we did this in `rustls_client_config_builder_new()`, but when
we introduce ECH config it will be beneficial to know if the user has
customized protocol versions using
`rustls_client_config_builder_new_custom()` or just wants sensible
defaults. This is easier to deduce if we defer populating the default
protocol values to the time of `rustls_client_config_builder_build()`
when we find the builder's protocol versions vec to be empty, indicating
defaults are desired (explicitly customizing configuration for no
protocols makes no sense).
Previously there were three. Let's reduce to one.
This commit adds initial support for customizing
a `rustls_client_config_builder` instance to use encrypted client hello
(ECH). Presently this is only supported with the `aws-lc-rs` crypto
provider.

In sum it adds:

* New `rustls_hpke` type for representing a collection of HPKE suites as
  an opaque struct.
* New `rustls_supported_hpke()` function for returning a const pointer
  to a `rustls_hpke` if available. With the aws-lc-rs backend this is
  a wrapper around
  `rustls::crypto::aws_lc_rs::hpke::ALL_SUPPORTED_SUITES`. For other
  providers it returns NULL.
* New `rustls_client_config_builder_enable_ech_grease` client config
  builder function for configuring ECH GREASE using a `rustls_hpke`.
  This chooses the HPKE suite at random, and so brings in a dep on the
  `rand` crate.
* New `rustls_client_config_builder_enable_ech` client config builder
  function for configuring ECH using a `rustls_hpke` and a TLS encoded
  ECH config.

Additionally the `client.c` example is updated to add
`RUSTLS_ECH_GREASE` and `RUSTLS_ECH_CONFIG` env vars. The former when
non-zero configures ECH GREASE if possible, the latter configures ECH
with the TLS encoded config list read from the provided file path.

To facilitate a reliable cross-platform way to fetch ECH configs a small
`ech-fetch` test program is added that accepts a domain and (optionally)
a file name and writes the TLS encoded ECH config for that domain to the
file after performing the appropriate DNS-over-HTTPS lookup. This
requires a new dev-only dep on HickoryDNS and Tokio.
1.71 and 1.72 both hit an ICE building with a static reference to
Rustls' aws-lc-rs HPKE suites. Upstream (sensibly) isn't interested in
fixing bugs in old compilers. We also don't want to bump the upstream
Rustls MSRV for niche errors, or add excessive workarounds. So: let's do
the easy thing and increase rustls-ffi's MSRV to 1.73. The rustls-ffi
project has few downstream consumers at this point and so is more agile
for MSRV bumps than the core crate.

Along the way, match the formatter to the MSRV. It had fallen behind.
@cpu
Copy link
Member Author

cpu commented Dec 12, 2024

I think this is self-contained enough and has been up for review long enough that I'm comfortable merging with just Ctz's review.

Happy to iterate further if there's any new feedback. I don't plan to cut a release tag for a bit so there's still time to adjust.

Build+Test (ubuntu-latest, clang, 1.71, aws-lc-rs) Expected — Waiting for status to be reported

Going to admin merge this and then fix the MSRV in the branch rules (it's now 1.73)

@cpu cpu merged commit ce84ad7 into rustls:main Dec 12, 2024
35 checks passed
@cpu cpu deleted the cpu-client-ech branch December 12, 2024 16:26
@cpu
Copy link
Member Author

cpu commented Dec 12, 2024

and then fix the MSRV in the branch rules

Done.

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.

2 participants