node:crypto: support the chacha20-poly1305 cipher - #33305
Conversation
BoringSSL only exposes ChaCha20-Poly1305 through the one-shot EVP_AEAD
interface, never as an EVP_CIPHER, so Cipher::FromName() could not find it
and createCipheriv("chacha20-poly1305", ...) threw ERR_CRYPTO_UNKNOWN_CIPHER.
getCiphers() left it out for the same reason.
Pin the BoringSSL commit that adds EVP_chacha20_poly1305(). Everything on
this side was already in place: isSupportedAuthenticatedMode(),
isChaCha20Poly1305() and the 16-byte default authTagLength in
initAuthenticated() all handle the cipher once the lookup succeeds.
Fixes #8072
WalkthroughThis PR updates the pinned BoringSSL commit hash used by the build configuration, adjusts the matching ChangesBoringSSL Pin Update and Crypto Test Coverage
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Correct that #25837 targets the same issue, and the PR description here says it supersedes it rather than duplicating it. The two take the same idea in different places, and #25837 does not build as written:
This PR puts @jtdowney, credit where it's due for the groundwork. |
There was a problem hiding this comment.
The Bun-side diff is small and the tests look thorough, but the actual cipher implementation lives in oven-sh/boringssl#9 (which this pins while still unmerged), and it deliberately diverges from Node's chacha20-poly1305 semantics in two places — worth a human sign-off on both the fork crypto code and the compat trade-off.
Extended reasoning...
Overview
This PR enables chacha20-poly1305 in node:crypto by bumping BORINGSSL_COMMIT in scripts/build/deps/boringssl.ts to a commit on the oven-sh/boringssl fork that adds EVP_chacha20_poly1305() as a streaming EVP_CIPHER. The remaining changes are test-only: a ~200-line RFC 8439 known-answer / streaming / tamper suite in test/js/node/crypto/crypto.test.ts, and the matching hash update in process.test.js. No Bun runtime source changes — the existing Cipher::isSupportedAuthenticatedMode() / isChaCha20Poly1305() paths light up once the lookup succeeds.
Security risks
The substantive change is a new AEAD primitive implemented in vendored C++ (the fork PR), not visible in this diff. A bug in the streaming Poly1305 state machine, block-boundary buffering, or tag comparison could yield silently wrong tags or accept tampered ciphertext. The test suite here is good (KAT vector, chunk-boundary matrix, tamper-tag/ciphertext/AAD, truncated tags, missing-tag rejection), but it can't substitute for a review of the actual EVP_CIPHER callbacks in the fork.
Level of scrutiny
High. This is security-sensitive crypto code plus a vendored-dependency bump — both explicitly flagged in the repo's review guidelines as needing human review. Additionally, the description notes BORINGSSL_COMMIT currently points at the head of an unmerged fork PR and will need another bump once oven-sh/boringssl#9 lands, so a maintainer needs to sequence the merges.
Other factors
The PR intentionally diverges from Node.js in two places (throwing on setAAD() after update(), and on final() without setAuthTag(), where Node returns unauthenticated plaintext). The rationale — matching Bun's and Node's own aes-256-gcm behavior and refusing to release unauthenticated data — is defensible, but it's an API-compat decision that should be a maintainer's call, not an auto-approval.
|
Agreed on all three points, and the sequencing is the right thing to flag. A few notes to make the review cheaper. Review map for oven-sh/boringssl#9The whole implementation is
The vectors in On the two divergencesOne of them is not a new opinion. The missing- if (cipher->m_kind == CipherKind::Decipher && cipher->m_ctx.isChaCha20Poly1305() && cipher->m_authTagState != AuthTagState::AuthTagPassedToOpenSSL) {
throwCryptoErrorWithAuth(lexicalGlobalObject, scope);
return {};
}That guard is a port of the equivalent check in That leaves exactly one decision: Sequencingoven-sh/boringssl#9 merges first. Then |
|
The red X on build 68202 is infrastructure, not the diff. 285 of 286 jobs passed; the one failure is a shard of The other shard of that same lane passed, as did Not re-rolling, since the failed shard produced no signal to re-roll for. Happy to push a retrigger if you would rather see a fully green board before merging. |
Fixes #8072. Depends on oven-sh/boringssl#9, which this pins.
Repro
chacha20-poly1305is also absent fromcrypto.getCiphers(), so libraries that pick an AEAD by probing that list never select it on Bun.Cause
BoringSSL only exposes ChaCha20-Poly1305 through the one-shot
EVP_AEADinterface, never as anEVP_CIPHER.Cipher::FromName()goes throughEVP_get_cipherbyname(), which has no entry for it, so the lookup returns null andconstructCipherthrows.getCiphers()walksEVP_CIPHER_do_all_sorted()and has the same gap.The same is true of
aes-*-ccm,aes-*-ocband thearia-*family, which BoringSSL does not implement at all. ChaCha20-Poly1305 is the one where the primitives are already there; this PR is only about that cipher.Fix
oven-sh/boringssl#9 adds
EVP_chacha20_poly1305(), anEVP_CIPHERimplementing RFC 8439 overCRYPTO_chacha_20andCRYPTO_poly1305_*, registered withEVP_get_cipherbyname,EVP_get_cipherbynidandEVP_CIPHER_do_all_sorted. This PR pins that commit.Nothing else was needed:
Cipher::isSupportedAuthenticatedMode(),CipherCtxPointer::isChaCha20Poly1305()and the 16-byte defaultauthTagLengthininitAuthenticated()were all already written for this cipher and start working once the lookup succeeds.Two places where Bun now behaves differently from Node, both deliberate, both matching what Node and Bun already do for
aes-256-gcm:aes-256-gcm(node & bun)chacha20-poly1305(node)chacha20-poly1305(this PR)setAAD()afterupdate()final()with nosetAuthTag()Verification
The tests are the RFC 8439 section 2.8.2 known-answer vector, round trips, streaming in 1/7/16/31/63/64/65/127/128-byte chunks (ChaCha20's block is 64 bytes, so chunks straddling the boundary are the interesting case),
update()/final()output lengths, tampered tag/ciphertext/AAD, truncatedauthTagLength1 through 16, and the nonce, key and tag length checks.BoringSSL's own
crypto_testis 1819 passed / 0 failed on the pinned commit;cipher_tests.txtthere runs the same vectors across every chunk size, in-place, and with a mid-operationEVP_CIPHER_CTX_copy.Note on the automated fail-before check
The gate re-derives the proof with
git stash push -- src/ packages/. This diff touches neither, because the fix lives in the pinned BoringSSL commit, so that check cannot reproduce the failure. TheUSE_SYSTEM_BUN=1run above is the fail-before evidence.Supersedes #25837, which took the same approach through
patches/rather than the fork, and was written against an olderevp_cipher_stlayout (BoringSSL has since splitcipherintocipher_update/cipher_final/update_aad, so its positional initializer would have bound the wrong callbacks).BORINGSSL_COMMITcurrently points at the head of oven-sh/boringssl#9; it needs one more bump to the merge commit once that lands, along with the matching hash intest/js/node/process/process.test.js.