Move rustls-post-quantum into the core crate #2288
Merged
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
Benchmark resultsInstruction countsSignificant differencesClick to expand
Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2288 +/- ##
==========================================
+ Coverage 94.82% 94.87% +0.05%
==========================================
Files 104 103 -1
Lines 24100 24136 +36
==========================================
+ Hits 22853 22900 +47
+ Misses 1247 1236 -11 ☔ View full report in Codecov by Sentry. |
I think this is obi1kenobi/cargo-semver-checks#573 -- I'm pretty sure this is semver compatible, otherwise the "semver trick" could never work. |
😅 |
ctz
commented
Dec 19, 2024
dconnolly
reviewed
Dec 19, 2024
Comment on lines
+237
to
+238
| /// This does not contain MLKEM768; by default MLKEM768 is only offered | ||
| /// in hybrid with X25519. |
There was a problem hiding this comment.
re the comment message:
This maintains the existing
ALL_KX_GROUPSand
introducesDEFAULT_KX_GROUPS. These are different
because (for now)DEFAULT_KX_GROUPSdoes not contain
ML-KEM-768.
That is out of abundance of caution -- in case ML-KEM
does not fare well in its first years of being (partially)
load-bearing for internet traffic. This seems the most
conservative choice.
Fair enough. There is (at time of writing) activity in TLSWG to adopt and possibly make the MLKEM-only key agreements more 'real' and on track to become an RFC than they currently are, including possibly 'Recommended=Y': if that happens I may ask about shuffling the DEFAULT_KX_GROUPS around again 😅
djc
reviewed
Dec 20, 2024
There was a problem hiding this comment.
Very exciting stuff!
So if I understand correctly, the aws-lc-rs default provider will now prioritize X25519MLKEM768 as the key exchange, and that will have a substantial effect on how handshakes will be conducted in the wild. I wonder whether, despite the careful API compatibility, this is the kind of thing that might merit a semver-incompatible release with clear notice in the release notes? Or just start by adding a simple way to enable this without affecting the default providers? (Like crypto::aws_lc_rs::post_quantum_provider().)
Once it's been deployed for a while, we could bring 0.23 up too, but this feels like a big change with substantial risk at breakage.
There is a balance here to be struck between sort of operational risk appetite (PQ key exchange needs many more CPU cycles/round trip-induced latency/bandwidth) and cryptographic risk appetite (quantum computers might be coming soonish so we need to migrate to better crypto ASAP), and in its current state I think this PR is too far on the crypto side.
cpu
reviewed
Dec 21, 2024
There was a problem hiding this comment.
I wonder whether, despite the careful API compatibility, this is the kind of thing that might merit a semver-incompatible release with clear notice in the release notes? Or just start by adding a simple way to enable this without affecting the default providers? (Like crypto::aws_lc_rs::post_quantum_provider().)
I'm optimistic, but share some of Djc's concerns about whether promoting the hybrid kx to default in a point release is going to cause trouble.
It feels like the separate rustls-post-quantum crate probably didn't get a lot of exposure/testing and so I see some appeal in a more gradual process where we make post-qc options more accessible (maybe a crate feature to opt-in to default post-qc without code change?) while concurrently collecting up more work to justify a semver-incompatible release. Hopefully by the time we have a good set of changes for a version bump we'll also have more downstream experience with post-qc and can retire the feature flag and make it a default.
|
Oh, I also like a Cargo feature as a mechanism to enable this, and something that we can easily shift from opt-in to opt-out over time. |
This was referenced Dec 29, 2024
5917ee1 to
aae61da
Compare
I've added a commit to do that. Here's what I'm thinking about a release note for this:
|
|
(I'm also aware that the commit ordering of this PR no longer makes much sense; it's on my TODO list.) |
339504b to
80b9b36
Compare
8bf0bef to
66f2c9e
Compare
|
I think this is ready for a second look. Rough changes since last time:
|
cpu
approved these changes
Jan 23, 2025
rustls/tests/api.rs
Outdated
| version.version, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| fn expected_kx_for_version(_version: &SupportedProtocolVersion) -> NamedGroup { |
| @@ -12,6 +12,7 @@ APIs ([`CryptoProvider`] for example). | |||
| * ECDSA, Ed25519 or RSA server authentication by clients `*` | |||
| DFEA | * ECDSA, Ed25519[^1] or RSA server authentication by servers `*` | ||
| * Forward secrecy using ECDHE; with curve25519, nistp256 or nistp384 curves `*` | |||
| * Post-quantum hybrid key exchange with [X25519MLKEM768](https://datatracker.ietf.org/doc/draft-kwiatkowski-tls-ecdhe-mlkem/) [^2] `*` | |||
The new "prefer-post-quantum" crate feature controls the default preference of `X25519MLKEM768` in the aws-lc-rs provider. If set, `X25519MLKEM768` is the most preferred algorithm in `DEFAULT_KX_GROUPS`. Otherwise, it is least preferred. `ALL_KX_GROUPS` contains both `X25519MLKEM768` and `MLKEM768`. `DEFAULT_KX_GROUPS` does not contain plain `MLKEM768`. That is out of an abundance of caution -- in case ML-KEM does not fare well in its first years of being (partially) load-bearing for internet traffic. This seems the most conservative choice. rustls-post-quantum sets this feature and re-exports the items that are now in the core crate.
827f765 to
0bf04a9
Compare
|
I think I've addressed those comments -- thanks. I also removed the link to |
djc
approved these changes
Jan 24, 2025
|
I'm confused why the semver compatibility lints are failing here. Maybe send some feedback upstream? These seem like false positives: |
This is tracked upstream as obi1kenobi/cargo-semver-checks#638 |
rustls-post-quantum is now a stub, and will have a summit release that reexports the same API from the core crate.
A HRR that does not request a specific KX group should lead to the precise same KeyShares extension in the second ClientHello. However, we omitted the second keyshare which does not meet the "without modification" requirement of RFC8446 §4.1.2. Fixes `ServerAcceptsEarlyDataOnHRR-Client-TLS13` when `prefer-post-quantum` feature is enabled.
This condition only considered the primary kx. Fixes `UnnecessaryHelloRetryRequest-TLS13` when `prefer-post-quantum` feature enabled.
0bf04a9 to
3253af2
Compare
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.
The goal here is: