-
Notifications
You must be signed in to change notification settings - Fork 599
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
aws-lc-rs: reduce priority of ECDSA_NISTP521_SHA512
#1924
Conversation
In TLS1.2, this actually means ECDSA_SHA512. If the peer selects that, we get caught out depending on the curve of the public key because we don't support (for example) `ECDSA_NISTP256_SHA512`. Reducing the preference of this improves matters, because a peer that respects our priority will only select that if nothing else is possible (which includes the cases that SHA256 and SHA384 are not supported, in which case we are hosed, but also if the version is TLS1.3 and public key is on P521).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1924 +/- ##
=======================================
Coverage 95.49% 95.49%
=======================================
Files 86 86
Lines 18650 18650
=======================================
Hits 17810 17810
Misses 840 840 ☔ View full report in Codecov by Sentry. |
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:
|
What is the right fix and why didn't you submit that? 😉 |
|
Is that hard, or just copying a bunch of constants around? |
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.
This seems like a better state of affairs than main
, and if the other realistic fix requires updating two separate crates (one outside of our org) I think it's fair to land this and pursue further improvements separately.
Thanks!
So do we want to release a 0.23.6 for this? |
I think 👍 but we also need a 0.22.x fix. Support for p521 w/ aws-lc-rs was added in 0.22.2. |
In TLS1.2, this actually means ECDSA_SHA512. If the peer selects that, we get caught out depending on the curve of the public key because we don't support (for example)
ECDSA_NISTP256_SHA512
.Reducing the preference of this improves matters, because a peer that respects our priority will only select that if nothing else is possible (which includes the cases that SHA256 and SHA384 are not supported, in which case we are hosed, but also if the version is TLS1.3 and public key is on P521).
fixes #1912 though unsatisfyingly