-
Notifications
You must be signed in to change notification settings - Fork 612
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
Echo session_id
in HRR
#1374
Echo session_id
in HRR
#1374
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1374 +/- ##
=======================================
Coverage 96.35% 96.35%
=======================================
Files 62 62
Lines 14554 14581 +27
=======================================
+ Hits 14023 14050 +27
Misses 531 531
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is something I regressed in the 0.20 times...
It's not, but was a late addition in TLS1.3 (introduced in draft 22) so got overlooked. |
66ea868
to
5f9dcde
Compare
Maybe that helps explain why BoGo doesn't have coverage for this too? 🤕 |
No, just an oversight / didn't occur to us that someone might mess this up. (If you think about what a TLS-1.2-expecting middlebox might do, any workarounds naturally need to equally apply to ServerHello and HelloRetryRequest. When implementing the compat mode, you need to keep this in mind.) I'll see about adding a test. |
We have a corresponding check on the ServerHello, but not HelloRetryRequest. See also rustls/rustls#1374, where rustls forgot to apply the compatibility logic to HelloRetryRequest. (From the perspective of a TLS-1.2-expecting observer, HelloRetryRequest is the ServerHello, so encoding hacks need to apply to both.) Change-Id: I9b711ea45c54770a76ecfbca8bc992a4eaef6fcd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62906 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com>
We have a corresponding check on the ServerHello, but not HelloRetryRequest. See also rustls/rustls#1374, where rustls forgot to apply the compatibility logic to HelloRetryRequest. (From the perspective of a TLS-1.2-expecting observer, HelloRetryRequest is the ServerHello, so encoding hacks need to apply to both.) Change-Id: I9b711ea45c54770a76ecfbca8bc992a4eaef6fcd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62906 Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com> (cherry picked from commit 9404a0b6c98e049094929db483634210560d31fb)
On the server, we should've been echoing the
session_id
in HelloRetryRequest messages (we already did for ServerHellos).On the client, there's a requirement that we detect this and fail the connection with an
illegal_parameter
alert.fixes #1373