WIP: ECH Draft-10

[sayrer]

[codecov-commenter]

Codecov Report

Merging #663 (ab6c143) into main (beef0be) will decrease coverage by 0.38%. The diff coverage is 91.49%.

@@            Coverage Diff             @@
##             main     #663      +/-   ##
==========================================
- Coverage   96.41%   96.03%   -0.39%     
==========================================
  Files          57       59       +2     
  Lines        9125     9807     +682     
==========================================
+ Hits         8798     9418     +620     
- Misses        327      389      +62     

Impacted Files	Coverage Δ
rustls/src/lib.rs	100.00% <ø> (ø)
rustls/src/msgs/mod.rs	100.00% <ø> (ø)
rustls/src/key_schedule.rs	96.25% <20.00%> (-2.70%)	:arrow_down:
rustls/src/client/mod.rs	94.73% <75.00%> (-2.22%)	:arrow_down:
rustls/src/client/tls13.rs	96.42% <86.66%> (-0.31%)	:arrow_down:
rustls/src/msgs/ech.rs	90.26% <90.26%> (ø)
rustls/src/msgs/handshake.rs	96.70% <91.39%> (-0.51%)	:arrow_down:
rustls/src/client/hs.rs	97.12% <91.89%> (-0.73%)	:arrow_down:
rustls/src/ech_key.rs	99.22% <99.22%> (ø)
rustls/src/error.rs	88.29% <100.00%> (+0.25%)	:arrow_up:
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update beef0be...ab6c143. Read the comment docs.

[sayrer]

I removed the minimal-versions check temporarily, since that error is being caused by the "hpke" crate. I don't plan to leave that dependency in the end, but I want to be able to spot other errors easily.

[briansmith]

@sayrer Are you wanting feedback on this? Not sure what your intent is RE the "WIP:" prefix.

[sayrer]

@sayrer Are you wanting feedback on this? Not sure what your intent is RE the "WIP:" prefix.

No, not yet. In particular, the ech_key.rs file is just for bootstrapping from other open-source implementations. My goal is to get the signing / authentication parts right for one ECH ciphersuite. I believe that can be tested only with messages, before integrating the code into the handshake.

[sayrer]

I switched this to hpke-rs for now. It's a bit easier to use since it uses the enum-heavy style that Rustls does. The hpke crate worked fine, but I would've had to implement some macros we won't end up using. Also, using hpke-rs with the "hazmat" feature lets you do things like get private key bytes. This will be good for testing during development, but I don't expect ring will support anything like this.

[sayrer]

I'm a little mystified by this Catalina test failure. It passes here on Big Sur, and also seems to pass on Linux (looking through the logs).

[sayrer]

This latest patch is only failing on code coverage metrics. I am not sure what's going on with the macOS tests--I think I'll wait it out since they pass on my local Macs.

The next step is to work through the ClientHello transformations required by the spec, and then sign that plaintext and aad. This patch shows that HPKE itself works when instantiated from the TLS wire format.

[sayrer]

Looks like the GitHub CI machines are pretty old--they don't have AES-NI and/or AVX instructions, which Evercrypt requires.

[briansmith]

Looks like the GitHub CI machines are pretty old--they don't have AES-NI and/or AVX instructions, which Evercrypt requires.

I believe M1 Macs don't have AVX either in their x86 emulation.

[sayrer]

Looks like the GitHub CI machines are pretty old--they don't have AES-NI and/or AVX instructions, which Evercrypt requires.

I believe M1 Macs don't have AVX either in their x86 emulation.

Could be that, too. Turns out there are also some AVX bugs that I'm not sure are fixed when building on 10.15.7.

Edit: I wanted to know the answer to this question, so I wrote a little repo that runs sysctl and runs a binary that self-checks if it's running under Rosetta emulation. The Mac machines report as "Intel(R) Xeon(R) CPU E5-1650 v2 @ 3.50GHz" with AVX1.0, but not AVX2 and up--I think this means they couldn't be M1s, because those don't emulate AVX at all. The binaries run as x86_64. https://github.com/grafica/MacPlatformTest/runs/2530250619.

They are probably old trashcan Mac Pros running VMWare Fusion, since the model reports as "VMware7,1".

[briansmith]

@sayrer How is this going? Is there any other prerequisite refactoring work that would be useful for this?

[sayrer]

@sayrer How is this going? Is there any other prerequisite refactoring work that would be useful for this?

It's going fine--I think the handshake is ready for it. I'm just going to work in ech.rs until I understand and test all of the mechanisms. Next step is to do ClientHelloOuterAAD, then actually seal the ClientHelloInner, which I'm currently working on.

Note that this work won't be possible to finish until the WG finalizes its approach to HRR. https://mailarchive.ietf.org/arch/msg/tls/mRGvQ26RzRIvbw4Od2VENQBrorc/

[briansmith]

OK, great to hear. I am not trying to be pushy about getting this done, BTW. I'm just interested to know if you've identified any other refactorings we should be doing.

[sayrer]

Added a demo that connects to Cloudflare. It doesn't work yet, because I haven't used the ClienHelloInner/ClientHelloOuter code in the handshake yet. It does pull the ECH configuration using Trust-DNS, and instantiates an HPKE context from that.

Negotiated ciphersuite: TLS13_AES_256_GCM_SHA384
read bytes: 111
HTTP/1.1 421 Misdirected Request
Date: Tue, 18 May 2021 19:39:14 GMT
Content-Length: 0
Connection: close

[sayrer]

% cargo run --example tls_ech_client
Error: Custom { kind: InvalidData, error: PeerMisbehavedError("server sent unsolicited encrypted extension") }

I can add EncryptedClientHello to be expected, and get past this.

The ClientHelloInner must be wrong, because I'm getting a ServerExtension::EncryptedClientHello with the same configuration as the initial one. [edit: The info argument to HPKE::SetupBaseS is wrong. Will fix.]

[sayrer]

It looks like the server is accepting the ECH now. There's some work to do in handle_server_hello to update the transcript upon acceptance. Getting this after that point:

Error: Custom { kind: InvalidData, error: DecryptError }

[sayrer]

It looks like there are two issues now:

1. The spec isn't very precise about what goes in the transcript, so I am writing unit tests for this. I've tried to write it a few times, but the EncodedClientHelloInner and what's in the transcript are not the same. It seems to just vary by message envelopes etc.

2. In order to confirm ECH was accepted, the first 8 bytes of a ring::Prk need to be examined. see: https://tlswg.org/draft-ietf-tls-esni/draft-ietf-tls-esni.html#section-7.2.

[sayrer]

OK, I got this to work, at least for non-HRR requests. The issue was that the ECH transcript needs to be calculated from an unsent inner ClientHello message, which differs in a few ways from the EncodedClientHelloInner used in HPKE. Upon ECH acceptance, the transcript from the ClientHelloOuter is discarded, then substituted with one calculated from the unsent inner ClientHello message, and the client random is changed to the inner random value. After that, the ServerHello message is added to the transcript as normal.

The code is still messy, so I'm not going to update the PR yet. You can see it here.

[sayrer]

The draft-10 patch will have some refactors concerning ExpectServerHello::handle and tls13::handle_server_hello. I don't think they will be necessary in later drafts due to this change: https://github.com/tlswg/draft-ietf-tls-esni/pull/420/files

[sayrer]

• The spec isn't very precise about what goes in the transcript, so I am writing unit tests for this. I've tried to write it a few times, but the EncodedClientHelloInner and what's in the transcript are not the same. It seems to just vary by message envelopes etc.

• In order to confirm ECH was accepted, the first 8 bytes of a ring::Prk need to be examined. see: https://tlswg.org/draft-ietf-tls-esni/draft-ietf-tls-esni.html#section-7.2.

I got these two things working, so I'm going to add HRR support, more tests, and clean this patch up enough to be ready for feedback. I don't think it will make sense to merge, because the draft-10 design is invasive to the client handshake in ways that draft-11 won't be. I'll add some notes on that via comments. After that, I'll work on the server.

[sayrer]

I've been exchanging some emails with Cloudflare regarding HRR. They have a bug fix for their server deploying later this week: https://github.com/cloudflare/go/pull/85/files

They're also going to disable P-384 and P-521 on their test server, so it will be easy to trigger HRR.

[cjpatton]

I'm curious if anyone plans to pick this up in the next few months. By the way, the current version is draft-13, which (we hope!) will be simpler to implement on the client side.

[cpu]

I think we should close this PR to reflect that it is:

i. Not under active development by anyone at this time ii. Has substantial conflicts with main iii. Is out-of-date with the current ECH drafts

I think it would be great to revive the ECH effort, but leaving this draft PR open doesn't feel like it will help advance that goal. It also comes at the cost of distracting from other work closer to being mergeable in the PR queue.

[djc]

I think it would be great to revive the ECH effort, but leaving this draft PR open doesn't feel like it will help advance that goal.

Agreed.

It also comes at the cost of distracting from other work closer to being mergeable in the PR queue.

This doesn't feel like a big win by itself...

[cpu]

This doesn't feel like a big win by itself...

For one PR in isolation it's not a huge win, but as more of them enter into this state the clutter gives the appearance of an unmaintained project. This branch in particular hasn't had a commit since 2021, it's very stale.

In either case, reopening closed PRs if someone feels differently is practically free ;-)

[cjpatton]

I support closing. The protocol (not to mention rustls itself) has changed so much since draft-10 that I think it would be worth starting fresh.