Repository navigation
Conversation
Add EVP_CIPHER wrapper for ChaCha20-Poly1305 AEAD cipher to enable
`createCipheriv('chacha20-poly1305', ...)` in node:crypto module.
This is needed for HomeKit/hap-nodejs compatibility and other applications
that require this cipher.
Implementation:
- Patch BoringSSL to expose EVP_chacha20_poly1305() function
- Add EVP_CIPHER implementation wrapping BoringSSL primitives
- Support streaming encryption with proper keystream buffering
- Handle AEAD operations (AAD, auth tags)
Fixes oven-sh#8072
WalkthroughThis pull request introduces ChaCha20-Poly1305 cipher support to the crypto module. It adds a new EVP cipher implementation with complete AEAD functionality, integrates it into the cipher enumeration and lookup system, updates build configuration to include the new source file, and provides comprehensive regression tests covering encryption, decryption, authentication, and edge cases. Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @patches/boringssl/expose_chacha20_poly1305.patch:
- Around line 11-20: EVP_CIPHER_do_all_sorted must call ciphers in alphabetical
order; the added callback for EVP_chacha20_poly1305 ("chacha20-poly1305") is
placed after EVP_bf_cfb and breaks sorting. Move the
callback(EVP_chacha20_poly1305(), "chacha20-poly1305", NULL, arg) so it appears
before the entries for the "bf-*" ciphers (i.e., before callback(EVP_bf_cbc(),
...)) to restore correct alphabetical order.
In @test/regression/issue/8072.test.ts:
- Around line 93-99: The test title and content mismatch: the test named
"supports different IV lengths up to 12 bytes" only checks a 12-byte IV. Either
rename the test to "supports standard 12-byte IV" (update the test(...) string)
or extend the test to actually exercise other IV lengths by creating additional
ciphers (e.g., const cipher8 = createCipheriv("chacha20-poly1305", key,
Buffer.alloc(8, 0x11)); cipher8.update(plaintext); cipher8.final();
expect(cipher8.getAuthTag().length).toBe(16);) so the name matches behavior;
adjust or add assertions accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
patches/boringssl/expose_chacha20_poly1305.patchtest/regression/issue/8072.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/regression/issue/8072.test.ts
test/regression/issue/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
Regression tests for specific issues go in
/test/regression/issue/${issueNumber}.test.ts. Do not put tests without issue numbers in the regression directory
Files:
test/regression/issue/8072.test.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue/8072.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/8072.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue/8072.test.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/8072.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `Buffer.alloc(count, fill).toString()` instead of `'A'.repeat(count)` to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Applied to files:
test/regression/issue/8072.test.ts
🧬 Code graph analysis (1)
test/regression/issue/8072.test.ts (1)
test/js/node/test/parallel/test-crypto-getcipherinfo.js (1)
ciphers(14-14)
🔇 Additional comments (16)
test/regression/issue/8072.test.ts (6)
1-12: LGTM!Good use of
bun:testimports and proper test setup withBuffer.alloc. The sequential initialization of key/IV values (lines 11-12) is correctly done with a loop since you need incrementing values.
14-53: Well-structured basic tests.Good coverage of cipher listing, info retrieval, and round-trip encryption with and without AAD. The auth tag length assertion (line 32) is a nice touch.
55-83: Good error scenario coverage.Properly tests that decryption fails with corrupted auth tag and incorrect AAD. The XOR corruption technique (line 62) is a good approach.
101-137: Excellent use of RFC 7539 test vectors.Using the official test vector from Section 2.8.2 provides strong validation that the implementation is correct. The bidirectional verification (encrypt then decrypt) is thorough.
139-168: Good edge case coverage.Empty plaintext and large data (1MB) tests properly exercise boundary conditions. Using
Buffer.alloc(1024 * 1024, 0x42)follows coding guidelines.
170-193: Streaming consistency test is well-designed.This test validates an important property - that incremental encryption produces the same result as single-call encryption. Critical for streaming use cases.
patches/boringssl/expose_chacha20_poly1305.patch (10)
65-82: Well-structured context for streaming AEAD.The context properly tracks all state needed for streaming encryption: keystream buffer, usage counter, block counter, and Poly1305 state.
84-91: LGTM - correct little-endian length encoding.The implicit truncation to
uint8_tcorrectly extracts each byte. This matches RFC 7539's Poly1305 length field format.
93-102: Correct Poly1305 key derivation.Per RFC 7539, the Poly1305 key is derived from the first ChaCha20 block (counter=0). The implementation correctly encrypts zeros to get the keystream as the Poly1305 key.
104-132: LGTM - proper key/IV initialization.State reset is correct:
keystream_used = 64forces fresh keystream generation, andcounter = 1reserves counter=0 for Poly1305 key derivation per RFC 7539.
134-138: Good security practice.Using
OPENSSL_cleanseensures sensitive key material is securely erased and won't be optimized away by the compiler.
177-184: Good constant-time tag comparison.Using
CRYPTO_memcmpfor authentication tag verification prevents timing attacks. This is a critical security requirement for AEAD ciphers.
200-266: Streaming implementation is correct.The three-step approach (use remaining keystream → process full blocks → buffer partial block) correctly handles streaming encryption while maintaining keystream state across calls. The Poly1305 update order (ciphertext) is correct for both encrypt and decrypt paths.
287-291: Verify IV length handling consistency.
EVP_CTRL_AEAD_SET_IVLENaccepts IVs from 1-12 bytes, butchacha20_poly1305_init_key(line 119) always copies exactly 12 bytes from the IV. If a caller sets a shorter IV length, they must still provide a 12-byte buffer. This is consistent with BoringSSL's behavior but could be documented.
325-333: LGTM - context copy implementation.Properly handles
EVP_CTRL_COPYfor cipher context duplication. The null checks ensure safety.
340-358: LGTM - correct cipher object definition.The static
EVP_CIPHERis properly configured:
block_size=1for stream cipher- Correct key/IV lengths (32/12)
- Appropriate flags for AEAD operation with custom IV and streaming support
| --- a/decrepit/evp/evp_do_all.cc | ||
| +++ b/decrepit/evp/evp_do_all.cc | ||
| @@ -36,6 +36,7 @@ void EVP_CIPHER_do_all_sorted(void (*callback)(const EVP_CIPHER *cipher, | ||
| callback(EVP_aes_256_gcm(), "aes-256-gcm", NULL, arg); | ||
| callback(EVP_bf_cbc(), "bf-cbc", NULL, arg); | ||
| callback(EVP_bf_cfb(), "bf-cfb", NULL, arg); | ||
| + callback(EVP_chacha20_poly1305(), "chacha20-poly1305", NULL, arg); | ||
| callback(EVP_bf_ecb(), "bf-ecb", NULL, arg); | ||
| callback(EVP_des_cbc(), "des-cbc", NULL, arg); | ||
| callback(EVP_des_ecb(), "des-ecb", NULL, arg); |
There was a problem hiding this comment.
Cipher not inserted in alphabetical order.
EVP_CIPHER_do_all_sorted implies alphabetical ordering. "chacha20-poly1305" should come before "bf-cbc" (c < bf), not after "bf-cfb".
🔎 Proposed fix
void EVP_CIPHER_do_all_sorted(void (*callback)(const EVP_CIPHER *cipher,
callback(EVP_aes_256_gcm(), "aes-256-gcm", NULL, arg);
+ callback(EVP_chacha20_poly1305(), "chacha20-poly1305", NULL, arg);
callback(EVP_bf_cbc(), "bf-cbc", NULL, arg);
callback(EVP_bf_cfb(), "bf-cfb", NULL, arg);
- callback(EVP_chacha20_poly1305(), "chacha20-poly1305", NULL, arg);
callback(EVP_bf_ecb(), "bf-ecb", NULL, arg);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @patches/boringssl/expose_chacha20_poly1305.patch around lines 11-20,
EVP_CIPHER_do_all_sorted must call ciphers in alphabetical order; the added
callback for EVP_chacha20_poly1305 ("chacha20-poly1305") is placed after
EVP_bf_cfb and breaks sorting. Move the callback(EVP_chacha20_poly1305(),
"chacha20-poly1305", NULL, arg) so it appears before the entries for the "bf-*"
ciphers (i.e., before callback(EVP_bf_cbc(), ...)) to restore correct
alphabetical order.
| test("supports different IV lengths up to 12 bytes", () => { | ||
| // 12-byte IV is standard | ||
| const cipher12 = createCipheriv("chacha20-poly1305", key, Buffer.alloc(12, 0x11)); | ||
| cipher12.update(plaintext); | ||
| cipher12.final(); | ||
| expect(cipher12.getAuthTag().length).toBe(16); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name doesn't match test content.
The test is titled "supports different IV lengths up to 12 bytes" but only verifies the standard 12-byte IV. Consider either:
- Renaming to "supports standard 12-byte IV", or
- Adding tests for shorter IV lengths (e.g., 8-byte) if they're actually supported
🔎 Option 1: Rename to match actual behavior
- test("supports different IV lengths up to 12 bytes", () => {
- // 12-byte IV is standard
+ test("supports standard 12-byte IV", () => {
const cipher12 = createCipheriv("chacha20-poly1305", key, Buffer.alloc(12, 0x11));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("supports different IV lengths up to 12 bytes", () => { | |
| // 12-byte IV is standard | |
| const cipher12 = createCipheriv("chacha20-poly1305", key, Buffer.alloc(12, 0x11)); | |
| cipher12.update(plaintext); | |
| cipher12.final(); | |
| expect(cipher12.getAuthTag().length).toBe(16); | |
| }); | |
| test("supports standard 12-byte IV", () => { | |
| const cipher12 = createCipheriv("chacha20-poly1305", key, Buffer.alloc(12, 0x11)); | |
| cipher12.update(plaintext); | |
| cipher12.final(); | |
| expect(cipher12.getAuthTag().length).toBe(16); | |
| }); |
🤖 Prompt for AI Agents
In @test/regression/issue/8072.test.ts around lines 93-99, The test title and
content mismatch: the test named "supports different IV lengths up to 12 bytes"
only checks a 12-byte IV. Either rename the test to "supports standard 12-byte
IV" (update the test(...) string) or extend the test to actually exercise other
IV lengths by creating additional ciphers (e.g., const cipher8 =
createCipheriv("chacha20-poly1305", key, Buffer.alloc(8, 0x11));
cipher8.update(plaintext); cipher8.final();
expect(cipher8.getAuthTag().length).toBe(16);) so the name matches behavior;
adjust or add assertions accordingly.
|
Picked this up in #33305, which takes the same approach but lands Worth flagging one thing from this PR that no longer holds: BoringSSL has since split Thanks for the groundwork on this either way. |
|
Superseded by #33305 |
Summary
Add EVP_CIPHER wrapper for ChaCha20-Poly1305 AEAD cipher to enable
createCipheriv('chacha20-poly1305', ...)in node:crypto module.EVP_chacha20_poly1305()functionThis is needed for HomeKit/hap-nodejs compatibility and other applications that require this cipher.
Fixes #8072
Test plan
bun bd test test/regression/issue/8072.test.ts- 12 tests passUnknown cipher: chacha20-poly1305)