refactor: remove length-prefixed logic for PQ key shares - #6041
Open
FreezB11 wants to merge 5 commits into
Open
Conversation
s2n-tls supported multiple draft revisions of the PQ hybrid TLS 1.3 key share wire format. Early drafts required each sub-share (ECC and PQ) to carry its own 2-byte length prefix inside the combined KeyShareEntry blob. Draft revision 5, which is what ML-KEM support uses, dropped this since sizes are fixed per named group and the outer KeyShareEntry length is already sufficient. Since s2n-tls only supports draft revision 5 going forward, the len_prefixed field and all logic branching on it can be removed. - Remove `len_prefixed` from `s2n_kem_params` (tls/s2n_kem.h) - Collapse all `if (len_prefixed)` branches in s2n_kem.c to the unprefixed behavior - Drop the `len_prefixed` parameter from `s2n_ecdhe_send_public_key` and update its call sites - Remove length-prefix send/detect logic from client and server key share extension handling - Remove `s2n_tls13_client_must_use_hybrid_kem_length_prefix` and related kem_preferences plumbing - TLS 1.2 PQ key exchange, which is unconditionally length-prefixed and unrelated to the TLS 1.3 draft-revision cleanup, is unaffected - Strip length-prefixed test paths from the key share and KEM unit tests (s2n_server_key_share_extension_test.c, s2n_client_key_share_extension_pq_test.c, s2n_kem_test.c, s2n_tls13_pq_handshake_test.c, s2n_pq_mlkem_policies_test.c) Resolves aws#5606
alexw91
self-requested a review
August 17, 2026 17:09
Contributor
|
Thanks for this! Can you also remove |
Contributor
Author
|
@alexw91 sure i can |
CarolYeh910
self-requested a review
August 17, 2026 21:32
Contributor
Author
|
thanks for running the actions the issue earlier was the aws-lc i recreated it on my machine and then fixed it, now will go ahead with removing |
Contributor
Author
|
@alexw91 @CarolYeh910 would love some review. |
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
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.
Problem
s2n-tls has supported multiple draft revisions of the PQ hybrid TLS 1.3
key share format while PQ standards were still in flux. Early drafts
required each sub-share (ECC + PQ) inside the combined KeyShareEntry to
carry its own 2-byte length prefix. This was only ever needed for draft
standards; s2n-tls has committed to supporting drafts (tls13_pq_hybrid_draft_revision < 5)
only through the end of 2025.
Draft revision 5 — the format used for ML-KEM support — dropped the
inner length prefixing entirely, since share sizes are fixed per named
group and the outer TLS 1.3 KeyShareEntry length is already sufficient
to parse the blob.
Solution
This PR removes the
len_prefixedfield froms2n_kem_paramsand alllogic that branches on it, since s2n-tls now only needs to support
draft revision 5 going forward.
Source changes:
tls/s2n_kem.h— remove thelen_prefixedfieldtls/s2n_kem.c— collapse the length-prefix branches ins2n_kem_send_public_key,s2n_kem_recv_public_key,s2n_kem_send_ciphertext, ands2n_kem_recv_ciphertextdown to theunprefixed (revision 5) behavior
tls/extensions/s2n_key_share.[ch]— drop thelen_prefixedparameter from
s2n_ecdhe_send_public_keytls/extensions/s2n_client_key_share.c/tls/extensions/s2n_server_key_share.c— remove the logic that setor inferred
len_prefixedfor TLS 1.3 negotiationtls/s2n_kem_preferences.[ch]— removes2n_tls13_client_must_use_hybrid_kem_length_prefixand relatedplumbing
Out of scope / explicitly preserved:
tls/s2n_client_key_exchange.c,tls/s2n_server_key_exchange.c) is unconditionally length-prefixedand is a separate wire format from the TLS 1.3 draft cleanup this
issue targets. That behavior is preserved as-is and not touched by
this PR.
Test changes:
tests/unit/s2n_server_key_share_extension_test.c— remove thelen_prefixedloop, test only the unprefixed pathtests/unit/s2n_client_key_share_extension_pq_test.c— same, plusupdate
s2n_copy_pq_shareands2n_generate_pq_hybrid_key_share_for_testhelpers to drop the prefix parameter
tests/unit/s2n_kem_test.c— remove length-prefixed assertions fromsend/recv public key and ciphertext tests
tests/unit/s2n_tls13_pq_handshake_test.c— removelen_prefix_expectedfrom test vectors and handshake assertionstests/unit/s2n_pq_mlkem_policies_test.c— replace thelength-prefix check with an assertion that ML-KEM policies use draft
revision 5
Testing
s2n_pq_mlkem_policies_test: 725 assertions passs2n_server_key_share_extension_test: 353 assertions passs2n_client_key_share_extension_pq_test: 39 assertions passs2n_kem_test: 167 assertions passs2n_tls13_pq_handshake_test: compiles and passes (pre-existing,unrelated cert-path failure not introduced by this change)
grep -rn "len_prefixed" tls/ tests/returns no resultsResolves #5606