Improve TLS extension representation #1475
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1475 +/- ##
==========================================
- Coverage 95.40% 95.29% -0.11%
==========================================
Files 97 97
Lines 21881 21486 -395
==========================================
- Hits 20875 20476 -399
- Misses 1006 1010 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This was referenced Nov 6, 2023
1 task
5bc822b to
dc957ec
Compare
Benchmark resultsInstruction countsSignificant differencesClick to expand
Other differencesClick to expand
Wall-timeSignificant differencesClick to expand
Other differencesClick to expand
Additional informationCheckout details:
|
1 task
dc957ec to
54d1d2c
Compare
c86bdef to
f6fa71d
Compare
|
I skimmed this a few days ago, and feel obliged to ask: what's the value of the macro here? |
At the minute it is mostly saving on writing quite a large number of lines of repeated code, mapping between fields on the struct and |
a722dc5 to
9b54669
Compare
1d06a1a to
43ce9af
Compare
e0e1ff6 to
271401a
Compare
OK, this is ready for another round. Not 100% happy with how many copies of ALPN data are going on, but that is probably an issue for another PR. |
djc
reviewed
Jun 6, 2025
There was a problem hiding this comment.
I'm guessing it would be helpful to land this in several PRs. I think there's a few prefix commits that we could just land straight away.
Maybe it would make sense to re-order this a bit to land 1.3 certs/cert requests/tickets first, and leave ClientHello/HRR/ServerHello to a separate (or three separate) PR(s)?
b08492e to
6918253
Compare
8e7112c to
314864a
Compare
Instead of `Vec<ClientExtension>`, store the extension data as a struct. This is possible because past commits have removed the need for this us to losslessly round-trip extension data. This involves fewer allocations to construct the extensions for clients. It eliminates repeated iteration of the vector to find specific extensions when processing a `ClientHello` for servers. It also reduces the cost of detecting duplicate extensions, which is moved into the decoding process -- a new `InvalidMessage::DuplicateExtension` error is introduced for this purpose. The struct is produced by a macro, which externalises the overall encoding and decoding operations. This allows specialisation for order of encoding (for ClientHello extension order randomisation) and whether an empty input is allowed (for ClientHello extensions in TLS1.2). During decoding a ClientHello, validate that any PS offer appears last. This replaces `ClientHelloPayload::check_psk_ext_is_last`. Now we have a specific error for this we can produce the error bogo expects for this case.
Annoyingly, ECH server confirmation requires that this this can be round tripped. Record the decoded order and prefer to encode in that order.
314864a to
0fb3391
Compare
|
Yes. If you haven't found it already here's where I was expecting this change to (mostly) go: rustls/rustls/src/server/hs.rs Lines 725 to 735 in c84675e |
|
Great! Thanks @ctz. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
not worth reviewing yet:I've broken ECH(now fixed)I've commented out a slab of testsFor other tests, I'm not super happy that they mean making the extension representation "public"this is re #908