WIP: ECH Draft-10#663
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
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. |
|
@sayrer Are you wanting feedback on this? Not sure what your intent is RE the "WIP:" prefix. |
No, not yet. In particular, the |
|
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. |
|
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). |
|
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. |
|
Looks like the GitHub CI machines are pretty old--they don't have AES-NI and/or AVX instructions, which Evercrypt requires. |
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.] |
|
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: |
|
It looks like there are two issues now:
|
|
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. |
|
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 |
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. |
| secret | ||
| } | ||
|
|
||
| pub fn server_ech_confirmation_secret(&mut self, hs_hash: &Digest) -> PayloadU8 { |
There was a problem hiding this comment.
The idea behind the the ring HKDF API is that you would do something like this:
pub struct Iv([u8; aead::NONCE_LEN]);
struct IvLen;
impl hkdf::KeyType for IvLen {
fn len(&self) -> usize {
aead::NONCE_LEN
}
}
s/Iv/EchConfirmationSecret/.
There was a problem hiding this comment.
I don't think this code will live on in the final implementation, given https://github.com/tlswg/draft-ietf-tls-esni/pull/420/files, so I just did the easiest thing for interop testing in draft-10.
There was a problem hiding this comment.
Also, I would say all the existing uses of PayloadU8Len are unfortunate, so I wouldn't copy them. derive_ticket_psk in particular should not be using Vec<u8> as its return type, but instead it should use a stronger key type.
Ideally export_keying_material would have a more sophisticated API as well, more like ring's HKDF API; I think the desire to avoid ring types in Rust's public API is what makes that difficult.
OTOH, probably it makes more sense for server_ech_confirmation_secret to compute the return the entire Random including the 24 random bytes and the 8 derived bytes.
There was a problem hiding this comment.
To add to my above comment, I think the draft-11 ECH confirmation will be able to happen entirely in client/hs.rs. The draft-10 design requires changes to client/tls13.rs and key_schedule.rs, but I don't think those will be needed in the future.
|
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. |
|
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. |
|
I think we should close this PR to reflect that it is: i. Not under active development by anyone at this time 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. |
Agreed.
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 ;-) |
|
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. |
7 participants
No description provided.