Conversation
WalkthroughChangesAdds ML-DSA and ML-KEM both-form PKCS#8 recovery from seed and expanded-key data, supporting PEM, DER, encrypted keys, native imports, and WebCrypto imports. Tests cover valid, invalid, encrypted, and mixed PEM inputs. PQC PKCS#8 recovery
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:28 AM PT - Jul 25th, 2026
❌ @robobun, your commit 2c09d70 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35510That installs a local version of the PR into your bun-35510 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… private keys
BoringSSL's EVP decoder only parses the `seed [0]` arm of the ML-DSA /
ML-KEM private-key CHOICE, rejecting the `both` SEQUENCE (which is
OpenSSL 3.5's default `genpkey` output) with
EVP_R_PRIVATE_KEY_WAS_NOT_SEED. That made both `crypto.createPrivateKey`
and `subtle.importKey("pkcs8")` refuse keys that Node v26 accepts.
When BoringSSL reports that error, re-parse the PKCS#8 with CBS, match
the ML-DSA/ML-KEM OID, extract the seed from the inner
`SEQUENCE { seed, expandedKey }`, and build the key via
`EVP_PKEY_from_private_seed`. The seed-derived public key is checked
against the matching portion of the expanded key (rho for ML-DSA, the
embedded ek for ML-KEM) so inconsistent pairs are still rejected, which
keeps the upstream `testImportPkcs8MismatchedSeed` case passing.
The `expandedKey`-only arm (no seed) remains unsupported: BoringSSL has
no EVP-level representation for a seedless ML-DSA/ML-KEM private key.
The previous commit covered unencrypted PKCS#8 (DER and PEM) and WebCrypto. An EncryptedPrivateKeyInfo wrapping a "both"-form inner key (as produced by `openssl genpkey -algorithm ML-DSA-65 | openssl pkcs8 -topk8`) still failed: `d2i_PKCS8PrivateKey_bio` and `PEM_read_bio_PrivateKey` decrypt and then hit the same `EVP_R_PRIVATE_KEY_WAS_NOT_SEED` on the plaintext. BoringSSL has no public API that stops at the plaintext PrivateKeyInfo bytes; both `PKCS8_decrypt` and `PKCS8_parse_encrypted_private_key` route the result through `EVP_parse_private_key`. The internal `bssl::pkcs8_pbe_decrypt` does exactly that step and has external linkage in the static link, so forward-declare it and feed its output to the same "both"-form seed-extraction used for the unencrypted path. PEM recovery now reads whichever of PRIVATE KEY / ENCRYPTED PRIVATE KEY is present via `PEM_read_bio` and dispatches accordingly. Adds two `*_private_both_encrypted.pem` fixtures (the in-tree "both" fixtures wrapped with `openssl pkcs8 -topk8 -passout pass:password`) and tests for PEM and DER import plus missing/wrong passphrase behaviour.
PEM_read_bio_PrivateKey loops over blocks and skips CERTIFICATE / EC PARAMETERS / etc. until it finds a private-key block. The recovery path now mirrors that so a concatenated PEM whose "both"-form PRIVATE KEY (or ENCRYPTED PRIVATE KEY) block is not first is still recovered.
7eca563 to
27ecef2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/ncrypto.cpp`:
- Around line 2656-2665: Before releasing the decrypted buffer in the PKCS#8
decryption flow, cleanse the `plain` contents for `plainLen` bytes, then call
`OPENSSL_free(plain)`. Keep parsing through
`EVPKeyPointer::TryParsePqcBothFormPkcs8` unchanged and ensure cleanup occurs
after parsing.
- Around line 2627-2635: Update the pkcs8_pbe_decrypt forward declaration to use
the same C linkage as the upstream symbol, either by including
crypto/pkcs8/internal.h or by matching its exact extern "C" declaration; do not
place it only inside namespace bssl.
In `@test/js/node/crypto/crypto-pqc.test.ts`:
- Around line 164-176: Strengthen the test around the tampering setup in the
“both” mismatch case by extracting the known seed from the seed-only fixture and
asserting that the DER range starting at offset 30, with the seed’s length,
matches it before flipping byte 30. Keep the existing importKey and
createPrivateKey rejection assertions unchanged, so the test verifies the
mutation targets the seed rather than merely relying on the resulting mismatch.
- Around line 156-162: Update the parameter-set selection in the test covering
subtle.importKey with the “both” fixture so ML-KEM cases choose the sibling
ML-KEM variant (ML-KEM-768 versus ML-KEM-1024) and use its appropriate key
usages, while preserving the existing ML-DSA sibling behavior and rejection
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35eacbb1-d1ab-4811-b6a4-bf8bbcf0c137
⛔ Files ignored due to path filters (2)
test/js/node/test/fixtures/keys/ml_dsa_44_private_both_encrypted.pemis excluded by!**/*.pemtest/js/node/test/fixtures/keys/ml_kem_768_private_both_encrypted.pemis excluded by!**/*.pem
📒 Files selected for processing (4)
src/jsc/bindings/ncrypto.cppsrc/jsc/bindings/ncrypto.hsrc/jsc/bindings/webcrypto/CryptoKeyAKP.cpptest/js/node/crypto/crypto-pqc.test.ts
- OPENSSL_clear_free the decrypted PrivateKeyInfo buffer (BoringSSL's OPENSSL_free already cleanses, but the explicit form documents intent). - Wrong-key-type test now picks the same-family sibling (ML-KEM-768 vs ML-KEM-1024, ML-DSA-44 vs ML-DSA-65) so the KEM rows exercise a parameter-set mismatch rather than a cross-family one. - Mismatched-seed test asserts byte 30 is the seed before flipping it, anchoring the offset against the seed-only fixture.
The ML-KEM seed is d||z (FIPS 203). ek depends on d alone while z is stored verbatim as the trailing 32 bytes of the expanded dk, so the ek comparison validated d but not z. RFC 9935 section 6 requires rejecting a "both" pair whose expandedKey is not exactly KeyGen_internal(d, z); compare the trailing z bytes too so an inconsistent z is caught, matching what OpenSSL does.
There was a problem hiding this comment.
Both of my earlier inline findings (multi-block PEM recovery, ML-KEM z-half consistency) are addressed and no new issues turned up this pass. Deferring to a human for the architectural call on forward-declaring BoringSSL's internal bssl::pkcs8_pbe_decrypt — it works in the current static link, but it's a private-symbol dependency that a maintainer should sign off on.
What was reviewed:
- CBS bounds in
TryParsePqcBothFormPkcs8: everyCBS_get_asn1return is checked, seed/expanded lengths are validated against the FIPS table before the offset reads, andpubInExpandedOffset + pubCompareLen ≤ expandedLenholds for every row. - PEM recovery loop frees
name/header/deron every iteration including thematchedbreak; decrypted plaintext isOPENSSL_clear_free'd. CryptoKeyAKP::importPkcs8still enforcesEVP_PKEY_id == nidForIdentifieron the recovered key, so a wrong-algorithm import is rejected.
Extended reasoning...
Overview
Adds support for the RFC 9881/9935 "both" PKCS#8 CHOICE arm for ML-DSA/ML-KEM private keys, which BoringSSL's EVP_parse_private_key rejects with EVP_R_PRIVATE_KEY_WAS_NOT_SEED. A new EVPKeyPointer::TryParsePqcBothFormPkcs8 re-parses the PrivateKeyInfo with CBS, extracts the seed, rebuilds the key via EVP_PKEY_from_private_seed, and verifies seed↔expandedKey consistency (rho for ML-DSA; embedded ek plus the trailing z for ML-KEM). The recovery is wired into four entry points in TryParsePrivateKey (unencrypted PEM/DER, encrypted PEM/DER) and into CryptoKeyAKP::importPkcs8. ~180 lines of C++ in ncrypto.cpp, a 9-line header addition, ~16 lines in CryptoKeyAKP.cpp, ~170 lines of new tests, and two encrypted fixtures.
Security risks
This is private-key parsing for post-quantum algorithms. The consistency check between the seed and expanded key is an RFC 9935 §6 MUST — an earlier revision missed the ML-KEM z half, now fixed. The CBS parse is defensive (all lengths checked before offset arithmetic), and the recovery only runs after BoringSSL has already identified the input as a well-formed PKCS#8 for one of these OIDs and pushed the specific not-seed error. The decrypted PrivateKeyInfo is now cleansed before free. I don't see a memory-safety or validation gap, but crypto key-import surfaces warrant maintainer eyes.
Level of scrutiny
High. This is production crypto code, and the encrypted path takes an unusual step: forward-declaring bssl::pkcs8_pbe_decrypt, an internal BoringSSL helper with no public API, to reach the plaintext PrivateKeyInfo bytes. The PR body documents why (both public entry points route through EVP_parse_private_key), and the encrypted-form tests pass in CI, so the linkage is correct for the vendored fork today. But a BoringSSL bump could rename, inline, or change the linkage/signature of this symbol. That's a maintainer-level trade-off, not something I should approve unilaterally.
Other factors
Test coverage is thorough — every parameter set × PEM/DER × unencrypted/encrypted, plus mismatched-seed rejection (both d and z halves for ML-KEM), sibling-algorithm rejection, expandedKey-only rejection, wrong/missing passphrase, and a leading-certificate PEM bundle. My two prior inline findings were addressed in 607f0d0 and 2c09d70. All prior review threads are resolved.
|
CI on build 80185: all
|
This branch has not been deployed
2 participants
ML-DSA / ML-KEM PKCS#8 private keys carry a
CHOICE { seed [0], expandedKey, both SEQUENCE{seed, expandedKey} }(RFC 9881 / 9935). BoringSSL's EVP decoder only parses theseed [0]arm and rejects the other two withEVP_R_PRIVATE_KEY_WAS_NOT_SEED. Thebothform is whatopenssl genpkey -algorithm ML-DSA-65writes by default on OpenSSL 3.5, so Bun could not load those keys on either crypto surface.Reproduction
With the in-tree fixtures (
test/js/node/test/fixtures/keys/ml_dsa_44_private.pemis the OpenSSL "both" form):Node v26.3.0 accepts both calls.
Cause
EVP_parse_private_key/EVP_PKCS82PKEYdispatch top_mldsa.cc/p_mlkem.ccDecodePrivate, which by design only reads[0] seedand errorsEVP_R_PRIVATE_KEY_WAS_NOT_SEEDfor the other arms.Fix
When the BoringSSL parse reports
EVP_R_PRIVATE_KEY_WAS_NOT_SEED, re-parse the PKCS#8 with CBS, match the ML-DSA/ML-KEM OID, read the innerSEQUENCE { OCTET STRING seed, OCTET STRING expandedKey }, and build the key viaEVP_PKEY_from_private_seed. The seed-derived public key is compared against the overlapping portion of the expanded key (rho for ML-DSA, the embeddedekfor ML-KEM) so an inconsistentbothpair is still rejected withDataError/ERR_OSSL_EVP_PRIVATE_KEY_WAS_NOT_SEED, which keeps the upstreamtestImportPkcs8MismatchedSeedcase intest-webcrypto-export-import-ml-{dsa,kem}.jspassing.Applied at every entry point that reaches BoringSSL's ML-DSA/ML-KEM
DecodePrivate:ncrypto::EVPKeyPointer::TryParsePrivateKeyfor unencrypted DER PKCS#8, unencrypted PEM, encrypted DER PKCS#8, and encrypted PEM (coveringcrypto.createPrivateKey/createPublicKey). BoringSSL has no public API that stops at the plaintext PrivateKeyInfo bytes (PKCS8_decryptandPKCS8_parse_encrypted_private_keyboth route throughEVP_parse_private_key), so the encrypted paths forward-declare the internalbssl::pkcs8_pbe_decrypthelper, which has external linkage in the static link, to obtain the decrypted bytes before the EVP dispatch.CryptoKeyAKP::importPkcs8forsubtle.importKey("pkcs8").The
expandedKey-only arm remains unsupported (BoringSSL has no EVP-level seedless ML-DSA/ML-KEM private key); both surfaces keep their existing diagnostic for it.Verification
New tests in
test/js/node/crypto/crypto-pqc.test.tscover, for every ML-DSA/ML-KEM parameter set:createPrivateKey(PEM + DER) andsubtle.importKeyaccepting thebothform and round-tripping to the seed-only form,createPublicKeyderiving the correct public key from it, rejection under a mismatched algorithm, rejection of abothform whose seed was flipped, and continued rejection of theexpandedKey-only arm. Sign/verify with a key imported from thebothform is exercised. Encryptedboth-form fixtures (*_private_both_encrypted.pem, generated withopenssl pkcs8 -topk8on the in-tree*_private.pem) are covered for PEM and DER import plus missing/wrong passphrase. 23 of the new cases fail on an unmodified build and all 60 pass with the fix. Existingcrypto.key-objects.test.ts,web-crypto.test.ts, and the vendored upstreamtest-webcrypto-export-import-ml-{dsa,kem}.jspass.[review] gate passed · iteration 2 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 2 rejected · iteration 2
evidence per changed file