Skip to content

Add ML-KEM support to HPKE (draft-ietf-hpke-pq-05) - #3277

Open
jakemas wants to merge 1 commit into
aws:mainfrom
jakemas:pq-hpke-mlkem
Open

Add ML-KEM support to HPKE (draft-ietf-hpke-pq-05)#3277
jakemas wants to merge 1 commit into
aws:mainfrom
jakemas:pq-hpke-mlkem

Conversation

@jakemas

@jakemas jakemas commented May 29, 2026

Copy link
Copy Markdown
Contributor

Implements ML-KEM-512, ML-KEM-768 and ML-KEM-1024 as HPKE KEMs per draft-ietf-hpke-pq-05, enabling post-quantum HPKE. Also adds HKDF-SHA384, so the priority suite HPKE(ML-KEM-1024, HKDF-SHA384, AES-256-GCM) is available.

Built on AWS-LC's existing ML-KEM implementation in crypto/fipsmodule/ml_kem/; no new cryptographic primitives are introduced.

Spec conformance

Value Source
KEM IDs 0x0040 / 0x0041 / 0x0042 draft §8.1 Table 2, IANA HPKE registry
Nsecret 32 (all three) draft §3
Nenc 768 / 1088 / 1568 draft §8.1
Npk 800 / 1184 / 1568 draft §8.1
Nsk 64 (all three) draft §3

Three points worth calling out explicitly:

The ML-KEM shared secret is used directly, with no ExtractAndExpand. DHKEM runs its shared secret through ExtractAndExpand; ML-KEM does not. The draft defines Encap/Decap as ML-KEM.Encaps and ML-KEM.Decaps directly, and Nsecret = 32 matches ML-KEM's native shared secret length. The generic RFC 9180 key schedule is unchanged and still processes the shared secret.

A private key is the 64-byte d || z seed, not the expanded decapsulation key. FIPS 203 returns the expanded form, dk = dk_PKE || ek_PKE || H(ek_PKE) || z:

FIPS 203 page 16
Screenshot 2026-05-29 at 1 46 17 PM

The draft does not use that form. It is explicit: "the decapsulation key is returned in seed format rather than the expanded form returned by ML-KEM.KeyGen", and Nsk is 64 for every parameter set. So EVP_HPKE_KEM_private_key_len() returns 64, not 1632/2400/3168, and EVP_HPKE_MAX_PRIVATE_KEY_LENGTH stays at 64. That is the serialized form: the seed is what EVP_HPKE_KEY_init accepts and what EVP_HPKE_KEY_private_key emits. Internally the seed is expanded once, at import or generation, with ml_kem_*_keypair_deterministic, which takes exactly the 64-byte seed, and the expanded key is cached in the struct — see the FIPS section for why. This matters for interoperability — every published test vector's skRm is 64 bytes, and a peer implementing the draft cannot import an expanded key. BoringSSL reaches the same conclusion independently: EVP_HPKE_MAX_PRIVATE_KEY_LENGTH is 64 there too.

Auth mode is refused for ML-KEM. ML-KEM cannot do AuthEncap/AuthDecap (draft §7.2), so the auth_encap_with_seed and auth_decap hooks are NULL and EVP_HPKE_CTX_setup_auth_sender/_auth_recipient fail with EVP_R_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE.

The encapsulation key is validated on encap via ml_kem_*_check_pk, since the draft requires an encapsulation key check failure to surface as an HPKE EncapError. mlk_kem_enc_derand in the backend already performs the same modulus check, so this call is belt-and-braces — it makes the intent explicit and gives the failure a distinguishable reason code.

Error codes differ slightly from upstream: a bad ML-KEM peer key raises EVP_R_INVALID_PEER_KEY where upstream raises EVP_R_DECODE_ERROR. Ours is consistent with the X25519 path in the same file and with RFC 9180's EncapError, so it is intentional.

Behaviour change to EVP_HPKE_KEY_cleanup

EVP_HPKE_KEY_cleanup was a documented no-op. It now cleanses both secrets — the seed and the cached expanded decapsulation key — and clears kem, returning the key to the zero state, and tolerates NULL.

Clearing kem matters more than it looks. Cleansing alone would leave kem set and the public key intact with an all-zero private key — and every 64-byte string is a valid ML-KEM seed, while a zero X25519 scalar is clamped to a valid one. A use-after-cleanup would therefore succeed, decapsulating under a key anyone can compute, rather than failing. Clearing kem makes that path fail instead.

Six entry points read key->kem without checking it, so on a key with no KEM they dereference NULL: EVP_HPKE_KEY_public_key, EVP_HPKE_KEY_private_key, EVP_HPKE_CTX_setup_recipient, both auth-sender setups, and EVP_HPKE_CTX_setup_auth_recipient. That is pre-existing — main has the same unguarded code — but there it was only reachable by passing a key that had never been initialized, and clearing kem in cleanup adds a second route to it. All six now fail with EVP_R_NO_KEY_SET. HPKETest.ZeroedKeyFailsCleanly covers them; with the guards removed it terminates with SIGSEGV rather than failing, so the test detects the guard rather than passing by construction.

Secrets also no longer outlive the calls that use them. The derived shared secret is cleansed once the key schedule has consumed it, and the encapsulation entropy is cleansed after sender setup returns, in both the base and auth paths.

A failed EVP_HPKE_KEY_init or EVP_HPKE_KEY_generate now cleanses as well, rather than only clearing kem. mlkem_init_key derives the expanded decapsulation key straight into the struct, so a failure after that point would otherwise leave key material behind in a key the caller has been told is unusable.

FIPS builds

Service indicator. HPKE is not an approved service, so these APIs must leave the service indicator unchanged. Without that, an HPKE call leaves a counter differential from the approved primitives underneath — HKDF (hkdf.c:35-50), AES-GCM (AEAD_GCM_verify_service_indicator) and RAND_bytes (rand.c:588) — which a caller would read as an approved service having been performed. The 11 public entry points that perform crypto now lock the indicator for the duration of the call, with the bodies moved to static functions; the other ten are lifecycle and accessor functions that only move or clear memory, so there is nothing to suppress. Exported symbols are unchanged. This is not ML-KEM specific — HPKE over X25519 was moving the counter before this change too. Verified on FIPS builds on x86-64 and aarch64: a ServiceIndicatorNotApproved test per ML-KEM suite, plus an X25519 test covering the four entry points the ML-KEM suites cannot reach (auth-mode sender, auth-mode recipient, and both deterministic sender setups). Removing only the lock/unlock calls makes the corresponding tests fail, so they detect the lock rather than passing by construction.

Where the keygen PCT lands. In FIPS builds the ML-KEM key generation entry point runs a pairwise consistency test — a full encapsulation and decapsulation — gated by MLK_CONFIG_KEYGEN_PCT, which is set exactly when AWS-LC is built in FIPS mode. The expanded decapsulation key has to be derived through that entry point, because crypto/fipsmodule/ml_kem/ml_kem.h exposes no PCT-free seed-expansion function today and crypto/fipsmodule/ml_kem/mlkem/ is a pristine import driven by importer.sh, so it should not be patched from crypto/hpke. The struct therefore caches the expanded key at import or generation, so the PCT is paid once per key rather than on every decapsulation. That placement is deliberate: without the cache, a key generation health test with fatal module-failure semantics would sit on a path reached from the network. One consequence worth noting for reviewers is that EVP_HPKE_KEY_init, which imports a caller-supplied seed rather than generating a key, also pays the PCT.

ABI impact — needs a maintainer decision

struct evp_hpke_key_st is public and stack-allocatable. An ML-KEM-1024 encapsulation key is 1568 bytes, so the struct cannot keep holding keys in 32-byte inline arrays; sizeof(EVP_HPKE_KEY) goes from 72 to 4808 bytes. EVP_HPKE_MAX_PUBLIC_KEY_LENGTH and EVP_HPKE_MAX_ENC_LENGTH also change from 32 to 1568, and the struct additionally caches the expanded ML-KEM decapsulation key (EVP_HPKE_MAX_EXPANDED_PRIVATE_KEY_LENGTH, 3168 bytes) so that decapsulation performs no key generation. See the FIPS section above for why that cache is there.

The four abidiff jobs will therefore report an ABI change, and there is no suppression mechanism in .github/docker_images/abidiff/diff.sh — it fails on any abidiff exit >= 4. Per docs/SymbolVersioning.md this implies an ABI_VERSION bump and a new SONAME, which is a release-level decision affecting every consumer, so I have not made it here. Please advise whether you want the bump in this PR or handled as part of a release.

Some notes to inform that decision:

  • No layout preserves ABI. Any struct able to hold a 1568-byte key changes size, so the break is unavoidable rather than a consequence of this particular design.
  • The layout follows upstream BoringSSL's fixed-size-inline-array approach, though it is no longer byte-identical to it: we additionally cache the expanded decapsulation key, which upstream does not, because upstream's seed expansion is PCT-free and ours is not. Adding a future KEM is still a matter of raising these constants rather than changing the shape of the struct.
  • An earlier revision of this PR used heap pointers inside the struct, which kept sizeof small but made EVP_HPKE_KEY a non-trivially-copyable type with owning pointers behind an API documented as stack-allocatable. That caused several problems — re-initialising a key leaked its old key material, EVP_HPKE_KEY_copy(k, k) freed the key and returned success, and EVP_HPKE_KEY_zero silently stopped scrubbing the private key. Fixed-size inline storage removes that whole class of bug, and matches how the library handles the same situation elsewhere (union evp_aead_ctx_st_state's opaque[564] in include/openssl/aead.h, and CRYPTO_MUTEX's sized padding in include/openssl/thread.h).

Two ECH stack buffers in libssl are sized by these macros and grow accordingly (ssl/handshake_client.cc:339, ssl/encrypted_client_hello.cc:522). ECH negotiates only DHKEM(X25519, HKDF-SHA256), so they cannot actually be filled beyond 32 bytes; sizing them by the X25519 lengths instead would avoid the growth, but that is a separate cleanup and is left out to keep this PR focused.

Relatedly, ECHServerConfig::Init now rejects an ECHConfig whose kem_id is not DHKEM(X25519, HKDF-SHA256). It already required the config's kem_id to match the configured key, but an EVP_HPKE_KEY can now hold an ML-KEM key, so a config and key which agreed on ML-KEM were accepted for a protocol that is only defined over X25519. SSL_marshal_ech_config takes the KEM from the key, so such a config was reachable through the public API. SSLTest.UnsupportedECHConfig covers it, and fails if the check is removed.

Relationship to BoringSSL

Upstream implements the same draft. Links below are pinned to e5a214a2:

This change deliberately follows upstream's public API while diverging on implementation:

  • API alignment. Macros are EVP_HPKE_MLKEM512 / _MLKEM768 / _MLKEM1024 (no KEM_ infix), matching upstream's spelling, as EVP_hpke_mlkem768 and EVP_HPKE_HKDF_SHA384 already did. EVP_HPKE_MAX_PRIVATE_KEY_LENGTH 64 and the seed-format private key match upstream too, so consumers built against either library see the same API and the same serialized key format.
  • Kept in C. Upstream's HPKE is now C++ (hpke.cc), part of a library-wide C-to-C++ migration — their crypto/fipsmodule has no .c files left. AWS-LC is not following that migration, so this stays in C. Upstream's ML-KEM parameterisation needs C++ templates because their ML-KEM API is built on opaque types; ours is byte buffers plus lengths, so a small MLKEM_METHOD table of function pointers expresses the same thing and the three parameter sets share one implementation.
  • Uses AWS-LC's ML-KEM. Upstream's HPKE is written against its own <openssl/mlkem.h> and a BCM layer (BCM_mlkem768_encap_external_entropy) that AWS-LC does not have. We use crypto/fipsmodule/ml_kem/, which already exposes deterministic encapsulation directly, so no equivalent plumbing is needed.
  • ML-KEM-512 is exposed. Upstream ships only 768 and 1024. The draft registers 512 and includes it "in the interest of completeness" while preferring 768/1024, so it is available here for callers that need it.

Testing

Known-answer tests come from the WG's machine-readable vectors, the [TestVectors] citation in the draft: crypto/hpke/test-vectors-pq.json, fetched from https://github.com/hpkewg/hpke-pq. These are vendored the same way RFC 9180's test-vectors.json already is, so translate_test_vectors.py stays reproducible.

Following upstream, which keeps hpke_test_vectors_pq.txt separate from the RFC 9180 file, the PQ vectors are generated into their own crypto/hpke/hpke_test_vectors_pq.txt. crypto/hpke/hpke_test_vectors.txt is regenerated and is byte-for-byte unchanged from main.

Three suites are covered — (ML-KEM-512, HKDF-SHA256, AES-128-GCM), (ML-KEM-768, HKDF-SHA256, AES-128-GCM) and (ML-KEM-1024, HKDF-SHA384, AES-256-GCM). The fourth ML-KEM vector in the JSON uses TurboSHAKE256, which this library does not implement, and is filtered out by the script.

These are real KATs, so they pin the wire format rather than just internal self-consistency: enc fixes Nenc and the encapsulation, skRm at 64 bytes fixes Nsk, pkRm fixes Npk, and the ciphertexts and exported values fix the key schedule including the HKDF-SHA384 (Nh = 48) path. I confirmed the vectors are genuinely exercised by corrupting one enc value and checking the suite fails, rather than trusting a green run.

The vector harness gained an optional kem_id attribute, defaulting to DHKEM(X25519) when absent so the RFC 9180 vectors are unaffected. ML-KEM vectors carry ikmE and enc where DHKEM vectors carry skEm and pkEm, because ML-KEM has no ephemeral key pair — encapsulation takes 32 bytes of entropy and emits a ciphertext.

HPKETest.RoundTrip now sweeps every KEM rather than just X25519, as upstream's does, skipping auth mode for the ML-KEM KEMs. That covers combinations the fixed parameter table misses, notably ML-KEM-512/768 with HKDF-SHA384 and with ChaCha20-Poly1305, across three info and three ad values.

New tests beyond the KATs cover round-trip and multi-message sealing for each suite, key serialization round-trip, copy/move including self-copy and self-move, auth-mode rejection, re-initialising an already-initialised key, rejection of an invalid encapsulation key on encap, an ML-KEM-1024 encapsulation key passed where an enc is expected, seed perturbation producing a distinct valid key, implicit rejection of a corrupted encapsulation, use-after-cleanup failing, re-initialization after cleanup succeeding, the zero state after a failed initialization, every entry point rejecting a key with no KEM, cleanup of a NULL key and cleanup leaving the zero state, the service indicator behaviour described above, and the length and buffer-size error paths. Every negative test asserts the specific reason code rather than just a false return, matching the existing X25519 tests.

One note on the vendored JSON: the WG file double-encodes info and pt — the values are the hex encoding of an ASCII hex string, so info decodes to the text 4f6465206f6e2061204772656369616e2055726e rather than to "Ode on a Grecian Urn". The published ciphertexts were computed over those literal bytes, so they are passed through verbatim. There is a comment in translate_test_vectors.py to stop someone "fixing" it later.

Performance

1000 iterations. Encap/Decap are setup_sender/setup_recipient, so they include the key schedule. Keygen is measured separately, and for ML-KEM it carries the seed expansion. Re-measured after the expanded key was cached, so decap no longer expands the seed.

Ciphersuite Keygen Encap Decap Encap+Decap
X25519 + SHA256 + AES-128-GCM 28.7 us 25.9 us 18.5 us 44.4 us
ML-KEM-512 + SHA256 + AES-128-GCM 5.6 us 8.0 us 7.3 us 15.3 us
ML-KEM-768 + SHA256 + AES-256-GCM 8.1 us 10.7 us 10.6 us 21.3 us
ML-KEM-1024 + SHA384 + AES-256-GCM 10.5 us 15.0 us 15.7 us 30.7 us

Non-FIPS RelWithDebInfo build, Apple M-series, macOS. Relative numbers are what matter; these are not comparable to the x86 figures in an earlier revision of this description.

Every ML-KEM suite is faster than X25519 DHKEM here, including ML-KEM-1024. The seed expansion sits in the keygen column rather than in decap, which is the point of caching it: a key is imported or generated once and then decapsulates many times. FIPS builds add the keygen PCT to keygen and to key import, as described above; these numbers do not include it.

Re-verified against draft-ietf-hpke-pq-05

Draft -05 was published on 6 July 2026, after this work started. Section 3 (ML-KEM) is unchanged from -04, and the IANA parameter table rows for 0x0040/0x0041/0x0042 are identical, so nothing normative moved for this implementation and no code change was needed. The section numbers cited here and in the code (§3, §7.2, §8.1) are unchanged in -05.

The published test vectors were regenerated in -05 — none of the -04 ML-KEM values survive. The vendored test-vectors-pq.json is already the -05 set: all four ML-KEM suites' ikmE, skRm and shared_secret values appear in -05 and none appear in -04. Regenerating from it leaves hpke_test_vectors_pq.txt byte-identical, so the KATs are current and were not touched.

One item for future work: TurboSHAKE is now RFC 9861 rather than draft-irtf-cfrg-kangarootwelve, which matters if the single-stage SHAKE/TurboSHAKE KDFs (0x0010-0x0013) are implemented later.

Out of scope

Deliberately not included, to keep this reviewable:

  • The PQ/T hybrids, including X-Wing (0x647a).
  • The single-stage SHAKE and TurboSHAKE KDFs (0x0010-0x0013), and therefore the one ML-KEM test vector that uses TurboSHAKE256.
  • DeriveKeyPair / EVP_HPKE_KEY_derive, which the draft defines over SHAKE256. Upstream has it. Not required by anything here, since the KATs load skRm directly, but it does mean the vectors' ikmR path is not exercised.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@jakemas
jakemas requested a review from a team as a code owner May 29, 2026 19:04
@jakemas
jakemas marked this pull request as draft May 29, 2026 19:04
github-actions[bot]

This comment was marked as spam.

@codecov-commenter

codecov-commenter commented May 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.90997% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.13%. Comparing base (e597639) to head (6973714).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
crypto/hpke/hpke.c 96.20% 8 Missing ⚠️
crypto/hpke/hpke_test.cc 98.76% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3277      +/-   ##
==========================================
+ Coverage   78.02%   78.13%   +0.11%     
==========================================
  Files         699      699              
  Lines      124589   125147     +558     
  Branches    17286    17316      +30     
==========================================
+ Hits        97205    97780     +575     
+ Misses      26516    26498      -18     
- Partials      868      869       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions[bot]

This comment was marked as spam.

github-actions[bot]

This comment was marked as spam.

@github-actions

Copy link
Copy Markdown
Contributor

🔒 Security ReviewView Report

Please review before merging.

Comment thread include/openssl/hpke.h
// Note this grew from 32 to 1568 when the ML-KEM KEMs were added, and
// |EVP_HPKE_KEY| grew with it. Callers which stack-allocate an |EVP_HPKE_KEY| or
// size buffers by this constant must be rebuilt against this header.
#define EVP_HPKE_MAX_PUBLIC_KEY_LENGTH 1568

@jakemas jakemas Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a note on how BoringSSL handled the same problem upstream, since it informed the approach here and it may save a reviewer the archaeology.

Upstream hit this exact situation four separate times as they added KEMs, and each time they grew the constants and kept evp_hpke_key_st a POD with inline arrays. They never moved to heap pointers:

Change MAX_PUBLIC_KEY_LENGTH MAX_ENC_LENGTH MAX_PRIVATE_KEY_LENGTH
P-256 KEM (0a2d3a4de092) 32 → 65 32 → 65 32
X-Wing (0697c8805166) 65 → 1216 65 → 1120 32
ML-KEM-768 (b887f19ede3d) 1216 1120 32 → 64
ML-KEM-1024 (706742e482d8) 1216 → 1568 1120 → 1568 64

Two things worth drawing out of that history:

The P-256 change is where the struct stopped being X25519-shaped. That CL replaced uint8_t private_key[X25519_PRIVATE_KEY_LEN] / public_key[X25519_PUBLIC_VALUE_LEN] with private_key[EVP_HPKE_MAX_PRIVATE_KEY_LENGTH] / public_key[EVP_HPKE_MAX_PUBLIC_KEY_LENGTH], i.e. they deliberately tied the struct to these macros so that adding a KEM is a one-line constant bump. That is the shape this PR adopts, and it is why the struct here is

struct evp_hpke_key_st {
  const EVP_HPKE_KEM *kem;
  uint8_t private_key[EVP_HPKE_MAX_PRIVATE_KEY_LENGTH];
  uint8_t public_key[EVP_HPKE_MAX_PUBLIC_KEY_LENGTH];
};

rather than the three heap pointers an earlier revision of this PR used.

The ML-KEM-768 CL is independent confirmation of the seed format. It sets MAX_PRIVATE_KEY_LENGTH to 64, not to 2400, which is the same conclusion drawn here from draft-ietf-hpke-pq-05 §3: an ML-KEM private key is the 64-byte d || z seed, not the expanded decapsulation key. Current upstream values are 1568 / 64 / 1568, identical to this PR.

Relevant caveat for the ABI discussion: upstream makes no ABI promise and has no abidiff gate, so they absorbed these four size changes silently. We cannot, which is why the ABI question is called out separately in the description. But it does mean that following their approach keeps a future KEM addition to raising these constants rather than reshaping the struct.

Correction (superseded by later commits): this comment originally claimed the layout stays byte-compatible with upstream. That is no longer true. EVP_HPKE_KEY now also caches the expanded ML-KEM decapsulation key, taking sizeof to 4808 rather than 1640, because upstream's seed expansion is PCT-free and ours is not — re-deriving per decapsulation would run a key-generation health test, with fatal module-failure semantics and a DRBG draw, on a path reached from the network. The fixed-size-inline-array approach is shared with upstream; the exact layout is not. The PR description has been updated to match.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it does mean that matching their layout keeps the two libraries byte-compatible, and that a future KEM addition here will be a constant bump rather than another structural change.

I doubt this is still true with the latest changes.(?)

@jakemas jakemas Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected in the comment above, and in the two places the description asserted it: sizeof(EVP_HPKE_KEY) is 4808 rather than 1640, and we share upstream's fixed-size-inline-array approach but are no longer byte-identical to it, since we cache the expanded decapsulation key and upstream does not.

The half that still holds is that adding a future KEM is a matter of raising these constants rather than reshaping the struct.

Comment thread crypto/hpke/hpke.c
Comment on lines +484 to +489
if (meth->encapsulate_deterministic(out_enc, &enc_len, out_shared_secret,
&shared_secret_len, peer_public_key,
seed) != 0) {
OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_PEER_KEY);
return 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intended service-indicator behavior of the public HPKE APIs?

These direct backend calls bypass the explicit ML-KEM updates in EVP_PKEY_encapsulate and EVP_PKEY_decapsulate, but FIPS decapsulation's PCT calls RAND_bytes, which can update the indicator anyway. If HPKE is not an approved service, should the HPKE operation lock the indicator to suppress updates from internal approved primitives? If HPKE is approved, shouldn't it lock internally and update exactly once at the HPKE boundary? Could we add a test for the intended behavior?

@jakemas jakemas Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intended behavior is that HPKE leaves the service indicator unchanged, since it is not an approved service. It now does: each call locks the indicator for its duration, so the approved primitives underneath cannot bump it.

That covers 11 of the 21 public functions — the ones that perform crypto: KEY_init, KEY_generate, the four sender-setup variants, both recipient-setup variants, seal, open and export. Each is now a thin wrapper that locks, makes one call and unlocks, with the body moved to a static. I did it that way rather than bracketing the returns because those functions have 27 exits between them, and one missed unlock would leave the indicator locked for the rest of the thread's life. The other ten are lifecycle and accessor functions that only move or clear memory, so there is nothing to suppress. Exported symbols are unchanged.

Where the updates were actually coming from: HKDF (hkdf.c:35-50), AES-GCM (AEAD_GCM_verify_service_indicator) and RAND_bytes (rand.c:588). Not X25519 — all seven FIPS_service_indicator_update_state sites in curve25519.c are Ed25519, and X25519 is correctly uninstrumented since it is not approved. ML-KEM has no update sites either. So HPKE over X25519 was already moving the counter before this change; this is not ML-KEM specific.

Verified on a FIPS build, since none of it is observable otherwise: 112 HPKE tests pass, including a ServiceIndicatorNotApproved case for each of the five suites, covering keygen, key import, sender setup, seal, recipient setup, open and export. Removing just the lock/unlock calls makes all five fail, so the tests detect the lock rather than passing by construction. Full FIPS crypto_test is green at 4117.

There is no ACVP coverage to add here: ACVP has no HPKE algorithm — it registers primitives plus a few named protocols, with ml-kem present only as the primitive — and util/fipstools/ has no HPKE support either.

One point I would like your read on: crypto/hpke is outside the module, and I did not find precedent for calling FIPS_service_indicator_lock_state from outside crypto/fipsmodule/. It links, and they are unregistered intra-library symbols, but say if you would rather reach them another way.

Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c Outdated

@justsmth justsmth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description is out of date.

Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c Outdated
Comment thread include/openssl/hpke.h
// Note this grew from 32 to 1568 when the ML-KEM KEMs were added, and
// |EVP_HPKE_KEY| grew with it. Callers which stack-allocate an |EVP_HPKE_KEY| or
// size buffers by this constant must be rebuilt against this header.
#define EVP_HPKE_MAX_PUBLIC_KEY_LENGTH 1568

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it does mean that matching their layout keeps the two libraries byte-compatible, and that a future KEM addition here will be a constant bump rather than another structural change.

I doubt this is still true with the latest changes.(?)

@justsmth justsmth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ECHServerConfig::Init, ECH is only defined over DHKEM(X25519, HKDF-SHA256), and the client offers nothing else, but |EVP_HPKE_KEY| can hold other KEMs, so check explicitly.

  if (ech_config_.kem_id != EVP_HPKE_DHKEM_X25519_HKDF_SHA256) {
    OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_ECH_SERVER_CONFIG);
    return false;
  }

Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c
Comment thread crypto/hpke/hpke_test.cc
Comment thread crypto/hpke/hpke_test.cc
Comment thread crypto/hpke/hpke_test.cc
Comment thread include/openssl/hpke.h Outdated
Comment thread crypto/hpke/hpke.c
Comment thread crypto/hpke/hpke.c
Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c
Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke.c
Comment thread crypto/hpke/hpke.c Outdated
Comment thread crypto/hpke/hpke_test.cc
Comment thread crypto/hpke/hpke.c Outdated
Implement ML-KEM-512, ML-KEM-768 and ML-KEM-1024 as HPKE KEMs, enabling
post-quantum HPKE. Also add HKDF-SHA384, so the priority ciphersuite
HPKE(ML-KEM-1024, HKDF-SHA384, AES-256-GCM) is available.

Built on the existing ML-KEM implementation in crypto/fipsmodule/ml_kem;
no new cryptographic primitives are introduced.

Per section 3 of the draft, a private key is serialized as the 64-byte
(d || z) seed rather than the expanded decapsulation key returned by
ML-KEM.KeyGen, so Nsk is 64 for every parameter set and
EVP_HPKE_MAX_PRIVATE_KEY_LENGTH stays at 64. Only the seed is stored; the
expanded key is re-derived per decapsulation with
ml_kem_*_keypair_deterministic, which consumes exactly that seed. The
ML-KEM shared secret is used directly as the HPKE shared secret, with no
DHKEM ExtractAndExpand step, and the encapsulation key is checked on
encap so that a bad key surfaces as an EncapError. ML-KEM cannot do
AuthEncap/AuthDecap, so the auth hooks are NULL and the mode_auth entry
points fail.

Supporting a 1568-byte ML-KEM encapsulation key means evp_hpke_key_st can
no longer hold keys in 32-byte inline arrays, so EVP_HPKE_MAX_ENC_LENGTH
and EVP_HPKE_MAX_PUBLIC_KEY_LENGTH grow to 1568 and sizeof(EVP_HPKE_KEY)
grows from 72 to 1640 bytes. The struct keeps fixed-size inline storage,
matching BoringSSL's layout, rather than taking owning heap pointers
behind an API documented as stack-allocatable. This is an ABI change and
the abidiff jobs will report it.

Known-answer tests come from the working group's test-vectors.json, the
draft's [TestVectors] citation, vendored alongside RFC 9180's. Following
upstream, they are generated into their own hpke_test_vectors_pq.txt;
hpke_test_vectors.txt is unchanged. The vector harness gained an optional
kem_id attribute, defaulting to DHKEM(X25519) when absent, and reads
ikmE/enc for ML-KEM where DHKEM uses skEm/pkEm.

Verified against draft-ietf-hpke-pq-05 (6 July 2026): section 3 is
unchanged from -04, so nothing normative moved for ML-KEM. The published
vectors were regenerated in -05, and the vendored test-vectors.json is
already the -05 set, so no vectors needed regenerating.
@jakemas

jakemas commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

On the ECHServerConfig::Init suggestion from the earlier review, since it wasn't an inline thread:

Added, thanks — this was reachable through the public API, not just in principle. SSL_marshal_ech_config writes EVP_HPKE_KEM_id(EVP_HPKE_KEY_kem(key)) verbatim, so an ML-KEM key produced an ECHConfig whose kem_id matched the key, and the existing check at the public-key comparison only required config and key to agree. Both agreeing on ML-KEM was accepted for a protocol only defined over X25519.

I put the check with the other unsupported-parameter checks, before the cipher suite loop, and added a case to SSLTest.UnsupportedECHConfig. Removing the check makes that test fail, so it is covering the new path rather than the pre-existing mismatch check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants