-
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
Return correct ConnectionTrafficSecrets
variant when AES-256-GCM is negotiated.
#1834
Conversation
I didn't add a test for this because the existing tests don't test with the granularity of specific protocol versions / ciphersuites. I can add a test if you want, though in that case I would rather add something that exhaustively tests every combination of TLS 1.2 / TLS 1.3 and AES-128-GCM / AES-256-GCM / CHACHA20-POLY1305. In fact I hit the original bug with a kTLS client that tests every combination in exactly that way. I have verified this branch using that client. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1834 +/- ##
==========================================
- Coverage 95.83% 95.82% -0.01%
==========================================
Files 84 84
Lines 18864 18876 +12
==========================================
+ Hits 18078 18088 +10
- Misses 786 788 +2 ☔ View full report in Codecov by Sentry. |
Thanks for the report and the fix! Going to go ahead and push a test for this on top of your PR -- I expect this will point to a similar problem in the aws-lc-rs code. |
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.
🤦♂️ thanks for catching my mistake here!
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
Thank you! |
I guess we should do a release for this? |
Certainly, yes. I'll aim to cut a 0.23.2 later this week. |
I think it probably merits a 0.22 point release as well |
55bb279 inadvertently changed
extract_keys
to always returnConnectionTrafficSecrets::Aes128Gcm
, even when AES-256-GCM was negotiated. This change fixes it by restoring the key length check.Fixes #1833