feat: add pure mlkem_1024 #5443
Conversation
johubertj
force-pushed
the
feat/add_pure_mlkem
branch
2 times, most recently
from
d737fde to
bfa016d
Compare
|
Manual Integration test: End-to-end testing performed with OpenSSL 3.5 and s2nd. Verified handshake negotiation with pure_mlkem_1024 by temporarily placing it at the top of the default_pq policy preference list. s2nd
OpenSSL 3.5
|
johubertj
force-pushed
the
feat/add_pure_mlkem
branch
from
d3444d0 to
b7135a1
Compare
added mlkem placeholder key exchange + key schedule unit test keep hybrid info in kat file pq handshake minor bugs and comment formatting removal debugging printstatements update client key share fixed recv stuff more refactor updated server clang formatting and skip pure mlkem if pq is disabled added skip added real curve for real runs
johubertj
force-pushed
the
feat/add_pure_mlkem
branch
from
b7135a1 to
8c2550b
Compare
|
Manual Integration test: Verified handshake failure with pure_mlkem_1024 by temporarily removing it from the default_pq policy preference list while requiring it on the client side. s2nd
OpenSSL 3.5
|
tls/s2n_ecc_preferences.c
Outdated
| @@ -64,6 +64,13 @@ const struct s2n_ecc_named_curve *const s2n_ecc_pref_list_default_fips[] = { | |||
| &s2n_ecc_curve_secp384r1, | |||
| }; | |||
|
|
|||
| const struct s2n_ecc_named_curve *const s2n_ecc_pref_list_pure_mlkem[] = { | |||
| #if EVP_APIS_SUPPORTED | |||
| &s2n_ecc_curve_x25519, | |||
There was a problem hiding this comment.
List includes at least one real ECC curve (x25519) so the handshake fallback logic never picks the mlkem_placeholder as the default curve.
| POSIX_ENSURE((expected_kem_group == NULL) != (expected_curve == NULL), S2N_ERR_SAFETY); | ||
| /* Skip XOR check for pure MLKEM since we have a KEM group and a curve (placeholder), otherwise enforce XOR check |
There was a problem hiding this comment.
Why do you need to skip this check? If your placeholder curve is a problem, why does hybrid mlkem work here?
| .expected_kem_group = null_if_no_pure_mlkem_1024, | ||
| .expected_curve = ec_if_no_pure_mlkem, |
There was a problem hiding this comment.
Isn't "expected curve" the curve negotiated if PQ / MLKEM isn't supported? Is this saying that the placeholder curve is negotiated in that case? Shouldn't the placeholder curve NEVER be negotiated?
Have you tested falling back to classic curves with pure MLKEM when handshaking with a peer that doesn't support PQ?
| /* Try to parse the share as ECC, then as PQ/hybrid; will ignore | ||
| * shares for unrecognized groups. */ | ||
| if (named_group == s2n_pure_mlkem_1024.iana_id) { | ||
| POSIX_GUARD(s2n_client_key_share_recv_pure_kem(conn, &key_share, named_group)); | ||
| continue; | ||
| } | ||
|
|
||
| POSIX_GUARD(s2n_client_key_share_recv_ecc(conn, &key_share, named_group)); | ||
| POSIX_GUARD(s2n_client_key_share_recv_pq_hybrid(conn, &key_share, named_group)); |
There was a problem hiding this comment.
This organization doesn't seem right. This function is delegating responsibility for recognizing ecc vs hybrid-- why would it not also delegate responsibility for recognizing pure?
tls/s2n_ecc_preferences.c
Outdated
| &s2n_ecc_curve_mlkem_placeholder, | ||
| }; |
There was a problem hiding this comment.
We should NOT need to include the placeholder on any of these lists. That implies we're allowed to negotiate the placeholder! In fact, there should be a unit test that enforces that no preference list includes the placeholder.
| POSIX_GUARD(s2n_generate_pure_pq_key_share(out, client_params)); | ||
| } else { | ||
| POSIX_GUARD(s2n_generate_pq_hybrid_key_share(out, client_params)); |
There was a problem hiding this comment.
Nit: "pure_pq" vs "pq_hybrid" seems like confusing naming :) Let's keep related functions named in a way that makes their similarities / differences clear. So probably "pq_pure" and "pq_hybrid".
| static int s2n_generate_pure_pq_key_share(struct s2n_stuffer *out, | ||
| struct s2n_kem_group_params *kem_group_params) |
There was a problem hiding this comment.
You're missing the s2n_pq_is_enabled() check that s2n_generate_pq_hybrid_key_share makes.
You're also duplicating all the logic in s2n_generate_pq_hybrid_key_share. Is there a cleaner way to handle this branching?
| static int s2n_client_key_share_recv_pure_kem( | ||
| struct s2n_connection *conn, | ||
| struct s2n_stuffer *key_share, | ||
| uint16_t kem_group_iana_id) | ||
| { |
There was a problem hiding this comment.
I would expect this to mostly duplicate s2n_client_key_share_recv_pq_hybrid, which doesn't seem like the best organization. Duplicating logic means that logic can be inconsistence. Like, do we enforce that the keyshare received is in our kem preferences? s2n_client_key_share_recv_pq_hybrid does that, but s2n_client_key_share_recv_pure_kem doesn't. If that isn't being enforce anywhere, that's a huge security problem.
testing testing testing testing
johubertj
force-pushed
the
feat/add_pure_mlkem
branch
from
fc283cd to
af21fee
Compare
johubertj
requested review from
lrstewart
and removed request for
goatgoose and
lrstewart
Release Summary:
Added pure ML-KEM 1024 (non-hybrid) key exchange support for TLS 1.3.
Resolved issues:
Partially addresses #5152
Description of changes:
Pure ML-KEM removes the ECC component, reducing handshake complexity and ciphertext size, and aligns with CNSA 2.0 requirements for PQ-only TLS
New KEM Group: Defined s2n_pure_mlkem_1024 and ECC placeholder (s2n_ecc_curve_mlkem_placeholder) for shared code paths.
Handshake Updates:
Testing:
Unit Tests: KEM group parsing, feature probing, key exchange & key schedule validation, policy negotiation.
Manual Integration Test: Handshake w/ s2nd and openssl3.5 using pure mlkem 1024
Negative manual integration test: Removed Pure ML-KEM 1024 support and required OpenSSL 3.5 to negotiate Pure ML-KEM 1024. As expected, the handshake failed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.