feat: add pure mlkem_1024 definition #5468

johubertj · 2025-08-18T21:27:50Z

Resolved issues:

Partially addresses #5152

Description of changes:

Define the pure ML-KEM group.
Add a placeholder s2n_ecc_curve_none to the ECC curve list.
Update secret generation tooling to support pure ML-KEM KATs.
Updated the unit test for for computing secrets for hybrid mlkem + renamed the file to match pure mlkem

Callouts:

Part 1 of adding pure mlkem. Next PRs will update the handshake(receiving), update the handshake (sending), add policy negotiation and add an integration test w/ OpenSSL 3.5
Breaking up this PR (feat: add pure mlkem_1024 #5443)

Testing:

Unit Tests: feature probing, key exchange & key schedule validation

crypto/s2n_ecc_evp.c

johubertj · 2025-08-18T23:31:35Z

tls/s2n_tls_parameters.h

@@ -71,6 +71,7 @@
 #define TLS_PQ_KEM_GROUP_ID_SECP256R1_MLKEM_768     0x11EB
 #define TLS_PQ_KEM_GROUP_ID_X25519_MLKEM_768        0x11EC
 #define TLS_PQ_KEM_GROUP_ID_SECP384R1_MLKEM_1024    0x11ED
+#define TLS_PQ_KEM_GROUP_ID_MLKEM_1024              0x0202


Value found from https://datatracker.ietf.org/doc/html/draft-ietf-tls-mlkem-04 section 4.1

tests/unit/s2n_tls13_pure_shared_secret_test.c

tls/s2n_tls13_handshake.c

crypto/s2n_ecc_evp.h

tls/s2n_tls13_handshake.c

crypto/s2n_ecc_evp.c

tls/s2n_tls13_handshake.c

tls/s2n_kem.c

tests/unit/s2n_tls13_pure_shared_secret_test.c

crypto/s2n_ecc_evp.c

tls/s2n_tls13_handshake.c

crypto/s2n_ecc_evp.c

lrstewart · 2025-08-22T06:01:40Z

tls/s2n_tls13_handshake.c

+    /* If the negotiated KEM group is pure PQ (no ECDHE), then only the PQ shared secret is used. */
+    if (kem_group->curve == &s2n_ecc_curve_placeholder_for_pure_pq) {
+        POSIX_GUARD(s2n_alloc(shared_secret, pq_shared_secret->size));
+        POSIX_CHECKED_MEMCPY(shared_secret->data, pq_shared_secret->data, pq_shared_secret->size);
+        return S2N_SUCCESS;
+    }


I think this is better, but still not the cleanest way to do this. You're still copying the hybrid logic instead of reusing it. It would be very easy for a developer to later add some safety check or something that accidentally only applies to hybrid, not pure. We should always treat the hybrid and pure mlkem secret calculations the same way. The only difference should be the ecc concatenation.

We already branch on calculating the ecc shared secret on client/server-- could you add a "neither, don't calculate at all" first branch down there? Then we branch on send_kem_first-- could you add an "only kem" branch down there?

lrstewart · 2025-08-22T06:17:42Z

tests/unit/s2n_tls13_pure_pq_shared_secret_test.c

+        /* Re-inject PQ secret for traffic secret derivation.
+         * The first compute_shared_secret wipes it, so we need to restore state for testing. */


This still didn't sound right to me. Why would you need to reset the shared secret after calculating the shared secret? How would that work outside of this test? So I dug into the code.

I was NOT right about what was going on here. s2n_tls13_compute_shared_secret is only called as part of s2n_extract_handshake_secret. s2n_tls13_derive_secret calls s2n_tls13_extract_secret, which calls s2n_extract_handshake_secret, which as mentioned calls s2n_tls13_compute_shared_secret.

So basically, the initialization is strange because this is actually two separate, unrelated tests. This test isn't doing things sequentially to mimic the actual use of these methods in code. Also, you're actually already testing s2n_tls13_compute_shared_secret by testing s2n_tls13_derive_secret.

I think you need to either:

Remove the s2n_tls13_compute_shared_secret part of the test.

Separate the s2n_tls13_compute_shared_secret and s2n_tls13_derive_secret tests. They should not be performed on the same connection in sequence. That's misleading and could lead to one test unexpectedly influencing the other, missing mistakes.

I'd suggest 2. I'm guessing s2n_tls13_compute_shared_secret is tested separately to make errors in that part of the calculation more obvious. The assertion that client == server seems a little weak, but that's better than nothing if we can't assert on an exact value.

However, it's also possible that the test you based this test off of just predates the rework of secret derivation, and didn't really make a deliberate choice to test s2n_tls13_compute_shared_secret separately. It might have been part of a messy later refactor to incorporate the new secret derivation structure. The dangers of copy/paste.

Oh I guess there's also:
3. Remove the s2n_tls13_derive_secret part of the test.

You did only make changes to s2n_tls13_compute_shared_secret, so maybe testing that is sufficient. You'd definitely need a stronger assertion then tho.

crypto/s2n_ecc_evp.c

tests/unit/s2n_tls13_pure_pq_shared_secret_test.c

lrstewart · 2025-08-22T19:08:12Z

tls/s2n_tls13_handshake.c

+int s2n_tls13_compute_pq_shared_secret(struct s2n_connection *conn, struct s2n_blob *shared_secret)
 {


Does this need to be this complicated? Why wouldn't 411d852 work? I checked, and that passes s2n_tls13_pure_pq_shared_secret_test. It's just my suggestions from #5468 (comment), except we don't even need the branch on send_kem_first.

Ah okay switching to your proposal, I was confused on your suggestion.

tests/unit/s2n_ecc_preferences_test.c

tests/unit/s2n_tls13_pure_pq_shared_secret_test.c

tests/unit/s2n_ecc_preferences_test.c

shared secret test addtional compute clang format add back renamed file added file feedback use defer cleanup remove comments s2n_set_up_pure_pq_conns combine hybrid + pure into 1 function removal of comments renamed testing files fixed comment renamed files revert back and added static renamed to &s2n_ecc_curve_placeholder_for_pure_pq; added comment on pure mlkem tracking Refactored pure pq shared secret test clang formatting updated feedback remove deriving secret and changed placeholder name refactor compute pq feedback remove typo clang formatting added label and iterate through all security policies clang format feedback ecc must be not null

lrstewart · 2025-08-25T22:56:04Z

tests/unit/s2n_ecc_preferences_test.c

+    /* Test: check all security policies don't contain the placeholder curve "s2n_ecc_curve_none" */
+    {
+        for (size_t policy_index = 0; security_policy_selection[policy_index].version != NULL; policy_index++) {
+            const struct s2n_security_policy *security_policy = security_policy_selection[policy_index].security_policy;
+            const struct s2n_ecc_preferences *ecc_preferences = security_policy->ecc_preferences;
+            EXPECT_NOT_NULL(ecc_preferences);
+
+            for (size_t curve_index = 0; curve_index < ecc_preferences->count; curve_index++) {
+                EXPECT_NOT_EQUAL(ecc_preferences->ecc_curves[curve_index], &s2n_ecc_curve_none);
+            }
+        }
+    };


Nit: we'll likely reuse this loop for any other ecc preference validation we need to do. I'd suggest naming the test something more generic, like "validate all ecc preferences" and move this more specific test name down to line 56 where you make the assertion.

crypto/s2n_ecc_evp.c

initial

158fe34

github-actions bot added the s2n-core team label Aug 18, 2025

johubertj force-pushed the feat/add_pure_mlkem_definition branch from 433500c to e80ba80 Compare August 18, 2025 23:09

johubertj commented Aug 18, 2025

View reviewed changes

crypto/s2n_ecc_evp.c Show resolved Hide resolved

johubertj requested review from lrstewart and goatgoose August 18, 2025 23:13

johubertj marked this pull request as ready for review August 18, 2025 23:14

johubertj commented Aug 18, 2025

View reviewed changes

lrstewart reviewed Aug 19, 2025

View reviewed changes

johubertj force-pushed the feat/add_pure_mlkem_definition branch from 5ac540c to e80ba80 Compare August 19, 2025 23:49

goatgoose reviewed Aug 20, 2025

View reviewed changes

johubertj force-pushed the feat/add_pure_mlkem_definition branch from 08b4f09 to 5dd9352 Compare August 20, 2025 18:33

johubertj requested review from goatgoose and lrstewart August 20, 2025 19:39

goatgoose reviewed Aug 21, 2025

View reviewed changes

johubertj requested a review from goatgoose August 21, 2025 17:21

goatgoose approved these changes Aug 21, 2025

View reviewed changes

lrstewart reviewed Aug 22, 2025

View reviewed changes

johubertj force-pushed the feat/add_pure_mlkem_definition branch from d3dcf61 to bba3b34 Compare August 22, 2025 18:25

johubertj requested a review from lrstewart August 22, 2025 18:27

lrstewart reviewed Aug 22, 2025

View reviewed changes

johubertj requested a review from lrstewart August 22, 2025 22:29

lrstewart reviewed Aug 25, 2025

View reviewed changes

tests/unit/s2n_ecc_preferences_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_tls13_pure_pq_shared_secret_test.c Outdated Show resolved Hide resolved

johubertj requested a review from lrstewart August 25, 2025 19:04

lrstewart reviewed Aug 25, 2025

View reviewed changes

tests/unit/s2n_ecc_preferences_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_ecc_preferences_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_ecc_preferences_test.c Outdated Show resolved Hide resolved

johubertj requested a review from lrstewart August 25, 2025 22:02

johubertj force-pushed the feat/add_pure_mlkem_definition branch from ed4cbb2 to 01e4ee7 Compare August 25, 2025 22:19

Merge branch 'main' into feat/add_pure_mlkem_definition

dcdee4d

lrstewart approved these changes Aug 25, 2025

View reviewed changes

lrstewart requested a review from goatgoose August 25, 2025 22:57

fixed comment

35e6260

goatgoose approved these changes Aug 26, 2025

View reviewed changes

crypto/s2n_ecc_evp.c Outdated Show resolved Hide resolved

removed comment

a47cb34

johubertj enabled auto-merge August 26, 2025 20:24

johubertj added this pull request to the merge queue Aug 26, 2025

Merged via the queue into aws:main with commit 5111deb Aug 26, 2025
50 checks passed

johubertj deleted the feat/add_pure_mlkem_definition branch August 26, 2025 23:09

		/* Re-inject PQ secret for traffic secret derivation.
		* The first compute_shared_secret wipes it, so we need to restore state for testing. */

		int s2n_tls13_compute_pq_shared_secret(struct s2n_connection conn, struct s2n_blob shared_secret)
		{

feat: add pure mlkem_1024 definition #5468

feat: add pure mlkem_1024 definition #5468

Conversation

johubertj commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resolved issues:

Description of changes:

Callouts:

Testing:

Uh oh!

Uh oh!

johubertj Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrstewart Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

lrstewart Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrstewart Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrstewart Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

johubertj Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lrstewart Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johubertj commented Aug 18, 2025 •

edited

Loading

lrstewart Aug 22, 2025 •

edited

Loading