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

WIP: ECH Draft-10 serialization code #539

Closed
wants to merge 23 commits into from
Closed

Conversation

sayrer
Copy link
Contributor

@sayrer sayrer commented Feb 24, 2021

In the interest of sharing early and often, here's code that serializes and deserializes ECH draft-09 ECHConfig records. The test record is from a live server at crypto.cloudflare.com. Part of addressing #508.

The next step is to add a little demo Trust-DNS client that pulls this record and parses it. I'll add the SVCB/HTTPS-RR code to Trust-DNS if need be. The Trust-DNS issue is hickory-dns/hickory-dns#1323. I used a Python library to pull the ECHConfig for this test, as described in #508.

After that, I think I'll start prototyping with the rust-hpke crate, since it has the older HPKE code needed for draft-09. I don't really think ring should bother with it, and instead just focus on the newest HPKE draft.

cc @djc @briansmith @ctz

@codecov-io
Copy link

codecov-io commented Feb 25, 2021

Codecov Report

Merging #539 (ef2b780) into main (1721e51) will decrease coverage by 0.39%.
The diff coverage is 85.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #539      +/-   ##
==========================================
- Coverage   96.78%   96.39%   -0.40%     
==========================================
  Files          51       53       +2     
  Lines        8962     9121     +159     
==========================================
+ Hits         8674     8792     +118     
- Misses        288      329      +41     
Impacted Files Coverage Δ
rustls/src/msgs/mod.rs 100.00% <ø> (ø)
rustls/src/msgs/handshake.rs 96.94% <79.10%> (-1.54%) ⬇️
rustls/src/msgs/ech.rs 80.85% <80.85%> (ø)
rustls/src/msgs/ech_test.rs 93.87% <93.87%> (ø)
rustls/src/msgs/base.rs 100.00% <100.00%> (ø)
rustls/src/msgs/handshake_test.rs 99.42% <100.00%> (+<0.01%) ⬆️
rustls/src/error.rs 93.75% <0.00%> (-2.50%) ⬇️
rustls/src/msgs/message.rs 97.63% <0.00%> (-1.58%) ⬇️
rustls/src/ticketer.rs 90.32% <0.00%> (-1.08%) ⬇️
... and 7 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 1721e51...b9651ba. Read the comment docs.

rustls/src/msgs/ech_test.rs Outdated Show resolved Hide resolved
@djc djc mentioned this pull request Feb 25, 2021
@sayrer
Copy link
Contributor Author

sayrer commented Feb 26, 2021

I'm happy to add some tests to keep the handshake.rs coverage up, but the CI coverage job seems to be failing in bogo/

@sayrer
Copy link
Contributor Author

sayrer commented Feb 26, 2021

Looks like the min-version checks are failing due to code that rust-hpke is pulling in, so I'll ignore that for now.

Going to refactor HPKEKeyPair a bit to carry its algorithm with it.

@sayrer
Copy link
Contributor Author

sayrer commented Feb 26, 2021

Looks like there will be a renamed TLS struct and DNS field, based on discussion and patches in

MikeBishop/dns-alt-svc#298

and

tlswg/draft-ietf-tls-esni#391

@sayrer
Copy link
Contributor Author

sayrer commented Mar 2, 2021

I don't really like Cloudflare's ECHKey serialization, but I'll keep the code around for now to facilitate testing.

@sayrer
Copy link
Contributor Author

sayrer commented Mar 10, 2021

When it came time to write the HPKE tests, I found myself wondering what data would be available. So I had to hack in a demonstration of the API @ctz suggested here: #318 (comment)

I think it will be a little nicer once HelloData (or whatever we call it) is threaded through the handshake code. extra_exts isn't easy to use with ClientSession::new right now, and ECH adds a few more variations of that problem.

@sayrer
Copy link
Contributor Author

sayrer commented Mar 11, 2021

Oops, that broke quic. I will use cargo test --all-features from now on.

@djc
Copy link
Member

djc commented Mar 11, 2021

Probably useful to extract that part into a separate PR?

@sayrer
Copy link
Contributor Author

sayrer commented Mar 11, 2021

Probably useful to extract that part into a separate PR?

I agree. I just needed to figure it out before I did the rest of the HPKE stuff. I think your other PRs on the handshake code will change it too.

@sayrer
Copy link
Contributor Author

sayrer commented Mar 11, 2021

I've gotten this to the point that I'm digging through the various Expect* structs after any HRR. At that point, only the dns_name is needed, so I'm looking for the boundary where I can stop using "HelloData" and just pass the dns_name on. I think that same boundary will be where ECH ends too.

I'll do a PR for just the handshake changes, which can simply combine dns_name + extra_exts into a struct initially, after #547 lands. Then, a new EncryptedHost struct can be added later. It will be useful through HRR as well.

@sayrer sayrer changed the title WIP: ECH Draft-09 serialization code WIP: ECH Draft-10 serialization code Apr 15, 2021
@sayrer
Copy link
Contributor Author

sayrer commented Apr 19, 2021

Closing in favor of #663.

@sayrer sayrer closed this Apr 19, 2021
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.

None yet

3 participants