Conversation
|
Updated 8:31 PM PT - Aug 17th, 2026
✅ @autofix-ci[bot], your commit 8c38a62beabf77fab5bd5bc6322e9606673a51b9 passed in 🧪 To try this PR locally: bunx bun-pr 28526That installs a local version of the PR into your bun-28526 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request adds AES-CFB8 cipher support (128/192/256-bit variants) to Bun's crypto module by introducing a BoringSSL patch, configuring the build system to apply it, and adding comprehensive test coverage. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28521.test.ts`:
- Around line 11-16: The test currently only ensures that
createCipheriv("aes-192-cfb8", key, iv) constructs; add a roundtrip by
encrypting a short plaintext with createCipheriv and then decrypting with
createDecipheriv using the same key/iv and verify the decrypted output equals
the original plaintext; use Buffer.from for plaintext, call cipher.update and
cipher.final (and decipher.update / decipher.final) to produce output and assert
deep equality, referencing the existing test name (e.g., "aes-192-cfb8 cipher
creates successfully") or add a new test case that uses createCipheriv and
createDecipheriv with the same key/iv and plaintext to validate end-to-end
encryption/decryption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b647adf9-67d5-4f25-9c3f-cb170df1e74f
📥 Commits
Reviewing files that changed from the base of the PR and between 639bc43 and 6e7fed738644ce419c34f447d46b5dc4fdf3ebde.
📒 Files selected for processing (3)
patches/boringssl/expose_aes-cfb8.patchscripts/build/deps/boringssl.tstest/regression/issue/28521.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation looks mechanically correct and follows established patterns, but this is a crypto code addition to a BoringSSL patch with test coverage gaps (missing aes-192-cfb8 roundtrip, no aes-192-cfb tests) — worth a human look before merge.
Extended reasoning...
Overview
This PR adds AES-CFB8 cipher support by patching BoringSSL in 3 files: a new patches/boringssl/expose_aes-cfb8.patch implementing the EVP wrappers, a boringssl.ts change to apply that patch during builds, and a regression test file. The patch touches decrepit/cfb/cfb.cc, crypto/cipher/get_cipher.cc, decrepit/evp/evp_do_all.cc, and include/openssl/cipher.h inside BoringSSL.
Security Risks
This is a cryptographic implementation change — it adds new EVP cipher wrappers around CRYPTO_cfb128_8_encrypt. The primitive itself comes from BoringSSL's FIPS module and is independently validated. The thin wrapper layer here is the risk surface: incorrect use of the num accumulator, wrong key schedule init, or mismatched IV handling could silently produce ciphertext incompatible with OpenSSL/Node.js. The implementation reuses aes_cfb_init_key and EVP_CFB_CTX from the existing CFB128 path, which mitigates most risk, but crypto additions warrant careful review.
Level of Scrutiny
High. Any BoringSSL patch modifying cipher implementations is security-sensitive and requires careful validation. The patch is compact and mechanical, closely following the CFB128 pattern, but the consequences of a subtle bug (e.g., wrong feedback byte, bad IV update) would be silent incorrect encryption rather than a crash.
Other Factors
The bug hunter flagged nit-level test coverage gaps: no encrypt/decrypt roundtrip for aes-192-cfb8, no tests at all for the newly-registered aes-192-cfb variant, and no known-answer vector tests to verify cross-implementation compatibility. There is also a pre-existing commented-out known-answer test in test/js/bun/crypto/cipheriv-decipheriv.test.ts that was blocked by missing CFB8 support and could now be uncommented. These are nits rather than functional bugs, but they reduce confidence in the 192-bit key path specifically.
There was a problem hiding this comment.
Test coverage gaps from my previous review have all been addressed — thanks! The crypto implementation looks sound, but since this patches BoringSSL cipher internals, it warrants a human sign-off.
Extended reasoning...
Overview
This PR adds AES-CFB8 (128/192/256-bit) support to Bun via a BoringSSL build patch. It touches four files: the BoringSSL patch itself (C++ changes to decrepit/cfb/cfb.cc, crypto/cipher/get_cipher.cc, decrepit/evp/evp_do_all.cc, and include/openssl/cipher.h), the build dependency config (boringssl.ts), and two test files.
Security risks
This directly modifies BoringSSL's cipher dispatch layer — specifically the decrepit module which houses legacy/extended ciphers. The CFB8 aes_cfb8_cipher_update function resets num=0 on each call, which is correct for CFB8 (no partial-block streaming state needed), and delegates to CRYPTO_cfb128_8_encrypt, a primitive already present and tested in BoringSSL. A known-answer vector test has been added (by uncommenting the pre-written test in cipheriv-decipheriv.test.ts) that validates cross-implementation compatibility with OpenSSL/Node.js.
Level of scrutiny
High. Any change to cryptographic primitive wrappers in BoringSSL should be reviewed by someone with expertise in the EVP cipher API and CFB8 mode semantics. This is not sandbox or test-only code — it becomes part of the production crypto layer.
Other factors
All three test coverage gaps from my prior review were resolved in commit 975cd2e: aes-192-cfb8 roundtrip, aes-192-cfb registration tests, and the known-answer vector. The CI failures visible in the timeline (24364.test.ts, bun-types.test.ts) are pre-existing and unrelated to this PR. No bugs were found by the automated bug hunting system.
6c1761c to
dd41d89
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28521.test.ts`:
- Around line 25-65: The existing CFB8 tests (tests named "aes-128-cfb8
encrypt/decrypt roundtrip", "aes-192-cfb8...", "aes-256-cfb8...") only call
cipher.update() once; add an additional test that exercises chunked update()
calls by splitting the plaintext into at least two parts, calling
createCipheriv(...).update(part1), .update(part2), then .final(), concatenating
the encrypted chunks, then doing the same chunked sequence with
createDecipheriv(...).update(...) for decryption, concatenating decrypted chunks
and asserting equality with the original plaintext; reference createCipheriv,
createDecipheriv, update(), and final() when implementing the new split-update
roundtrip test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24af1ab2-1ada-4114-811b-4c16d454cf88
📥 Commits
Reviewing files that changed from the base of the PR and between 6c1761c7d17ec153081473602594a2d59b92d32c and 13cee15.
📒 Files selected for processing (4)
patches/boringssl/expose_aes-cfb8.patchscripts/build/deps/boringssl.tstest/js/bun/crypto/cipheriv-decipheriv.test.tstest/regression/issue/28521.test.ts
There was a problem hiding this comment.
All feedback from my prior review has been addressed -- LGTM.
Extended reasoning...
Overview
This PR patches BoringSSL to expose AES-CFB8 mode (128/192/256-bit keys) via the EVP cipher API, fixing createCipheriv("aes-128-cfb8", ...) which previously threw ERR_CRYPTO_UNKNOWN_CIPHER. Four files are changed: a new BoringSSL patch, the build integration that applies it, a new regression test, and an update to the existing cipher test.
Security risks
The CFB8 implementation delegates entirely to BoringSSL existing CRYPTO_cfb128_8_encrypt primitive. The patch follows the same structure as the existing CFB128 wrappers in decrepit/cfb/cfb.cc. No new cryptographic code is introduced -- only plumbing to expose existing, tested primitives through the EVP API. No security risks identified.
Level of scrutiny
Low-to-medium. This is a BoringSSL incremental patch adding EVP wrappers for a standard NIST cipher mode. The pattern is identical to the existing CFB128 additions. The main risk would be a mistake in the EVP_CIPHER struct initialization (wrong key/IV lengths or NID), but the values match the expected 128/192/256-bit key sizes and 16-byte IV.
Other factors
My prior review flagged three test coverage gaps; all three were addressed in 975cd2e: the aes-192-cfb8 roundtrip test was added, aes-192-cfb got creation and getCiphers() coverage, and the pre-existing commented-out known-answer test for aes-128-cfb8 was uncommented. The test suite now validates cipher availability, encrypt/decrypt roundtrips for all three key sizes, and a cross-implementation known-answer vector. No bugs were found by the automated system.
There was a problem hiding this comment.
LGTM — all prior feedback addressed.
Extended reasoning...
Overview
This PR patches BoringSSL to expose AES-CFB8 (128/192/256-bit) via the EVP cipher API, registers the new ciphers in the lookup table and getCiphers() enumeration, and also adds the previously missing aes-192-cfb (CFB128) registration. Four files are modified: the BoringSSL patch itself, the build integration in boringssl.ts, a new regression test, and the existing cipher known-answer test file.
Security risks
The implementation delegates directly to CRYPTO_cfb128_8_encrypt, the existing BoringSSL primitive already present and tested in the library. The CFB8 wrapper reuses aes_cfb_init_key (already tested by the CFB128 path) and correctly maintains streaming state through ctx->iv (modified in-place across update() calls). The num = 0 reset per call is correct for CFB8 since each byte fully consumes one AES-block feedback round, leaving num at 0. No new attack surface is introduced beyond what the existing CFB128 wrappers already expose.
Level of scrutiny
Moderate: this is a BoringSSL patch touching crypto primitives. However, the pattern is identical to the existing expose_aes-cfb.patch already in the codebase, the implementation is minimal and mechanical, and the known-answer test (uncommented from cipheriv-decipheriv.test.ts) provides cross-implementation validation against pre-computed OpenSSL vectors.
Other factors
All three issues from my previous review were resolved in commit 975cd2e: the aes-192-cfb8 roundtrip test was added, aes-192-cfb creation and getCiphers() coverage was added, and the known-answer test was uncommented. The remaining CodeRabbit nit (split-update() test) is a coverage enhancement, not a correctness issue. No bugs were found by the automated system.
13cee15 to
11c0d5f
Compare
There was a problem hiding this comment.
All feedback addressed — LGTM.
Extended reasoning...
Overview
This PR adds AES-CFB8 (128/192/256-bit) support to BoringSSL via a new patch, wires it into the build system, adds a full regression test suite, and re-enables the previously-commented-out aes-128-cfb8 known-answer test vector.
Implementation correctness
The aes_cfb8_cipher_update function correctly delegates to bssl::CRYPTO_cfb128_8_encrypt. The num parameter is reset to 0 on each call, which is safe for CFB8 because each byte consumes a complete 1-byte segment of a freshly-encrypted block, so num is always 0 after processing any number of complete bytes and no cross-call state is lost. The IV shift register state is preserved across calls through ctx->iv. The EVP_CIPHER descriptors for all three key sizes reuse the existing aes_cfb_init_key, which correctly dispatches AES_set_encrypt_key with the right key length.
Security risks
No new security risks. CFB8 is a standard NIST mode; the implementation delegates entirely to BoringSSL own CRYPTO_cfb128_8_encrypt primitive. The patch lives in decrepit/ (BoringSSL compatibility layer), consistent with the existing CFB128 wrappers.
Test coverage
Prior feedback raised three gaps: missing aes-192-cfb8 roundtrip, no aes-192-cfb coverage, and no known-answer vector tests. All three were addressed in 975cd2e: roundtrip tests now cover all three key sizes, aes-192-cfb creation and getCiphers() inclusion are tested, and the aes-128-cfb8 known-answer vector in cipheriv-decipheriv.test.ts has been re-enabled. The remaining CodeRabbit nitpick about split-update() tests is trivial.
Level of scrutiny
This touches BoringSSL internals but is well-scoped: it follows the exact same pattern as the existing CFB128 patch, uses only BoringSSL-internal primitives, and has been verified with bun bd test against the new test suite.
11c0d5f to
93e56ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patches/boringssl/expose_aes-cfb8.patch`:
- Line 13: Remove the unintended registration of the decrepit AES-192-CFB cipher
name by deleting the entry that maps NID_aes_192_cfb128 / "aes-192-cfb" to
EVP_aes_192_cfb128 (the added lookup line that makes "aes-192-cfb"
discoverable); this change must be undone in both places where that tuple was
added so the codebase keeps AES-192-CFB as non-discoverable (decrepit-only) and
avoids widening the public cipher lookup surface.
🪄 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: bc64f2d0-51fa-4220-87ef-5f33fdb4e793
📒 Files selected for processing (2)
patches/boringssl/expose_aes-cfb8.patchscripts/build/deps/boringssl.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/crypto/cipheriv-decipheriv.test.ts`:
- Around line 235-297: Import describe from "bun:test" and replace the nine
separate it(...) blocks with a parameterized table using describe.each([...])
keyed by algorithm, key bytes/Buffer, iv and a sample plaintext; inside the
describe.each callback run three assertions that use createCipheriv (creation),
createCipheriv/createDecipheriv roundtrip (encrypt/decrypt using cipher.update +
cipher.final and decipher.update + decipher.final) and a single test that
asserts getCiphers() contains the algorithm. Keep references to createCipheriv,
createDecipheriv, getCiphers, and use describe.each(...) to drive the tests so
adding new CFB8 variants only requires appending rows to the data table.
🪄 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: aa03d915-5dba-495c-a3ed-9bc87876eb36
📒 Files selected for processing (2)
patches/boringssl/expose_aes-cfb8.patchtest/js/bun/crypto/cipheriv-decipheriv.test.ts
5ddaffb to
27d8741
Compare
0709c27 to
6fdf493
Compare
There was a problem hiding this comment.
I reviewed this PR and the only new finding is the inline nit about the patch comment. The change itself looks correct — it wraps BoringSSL's existing CRYPTO_cfb128_8_encrypt primitive following the same shape as the neighboring CFB128 EVP structs, the kCiphers[] insertions preserve alphabetical name order, IV state persists across update() calls via ctx->iv, and the uncommented aes-128-cfb8 known-answer vector guards interop. Since it's a vendor patch to BoringSSL's cipher surface, a human sign-off would still be worthwhile.
Extended reasoning...
Overview
This PR adds AES-CFB8 (128/192/256) support to Bun's node:crypto by patching the vendored BoringSSL build. It touches three files: a new patches/boringssl/expose_aes-cfb8.patch (implements EVP_CIPHER wrappers in decrepit/cfb/cfb.cc around the pre-existing CRYPTO_cfb128_8_encrypt primitive, registers the names in kCiphers[] and evp_do_all.cc, and declares the accessors in cipher.h), a one-line addition to scripts/build/deps/boringssl.ts to apply the patch, and test additions in test/js/bun/crypto/cipheriv-decipheriv.test.ts (creation × 3, roundtrip × 3, getCiphers() inclusion, plus uncommenting the existing aes-128-cfb8 known-answer vector).
All feedback from my earlier passes has been addressed: the 192-bit roundtrip and known-answer vector were added, the out-of-scope aes-192-cfb (CFB128) registration was dropped, and the tests were relocated from test/regression/ into the module's own test file. All CodeRabbit threads are resolved and CI (Build #57502) is green.
Security risks
The change exposes an additional legacy cipher mode via createCipheriv. It does not implement new cryptography — it wires BoringSSL's own CRYPTO_cfb128_8_encrypt into the EVP dispatch table using the same struct layout, init function (aes_cfb_init_key), and context type (EVP_CFB_CTX) as the adjacent CFB128 wrappers. CFB mode correctly uses the AES encrypt key schedule for both directions. The feedback register lives in ctx->iv and is updated in place by CRYPTO_cfb128_8_encrypt, so state survives across multiple update() calls even though the local num is not persisted (CFB8 has no sub-byte partial state). The kCiphers[] insertions land in the correct alphabetical positions relative to their neighbors. No auth/permission surface is affected.
Level of scrutiny
Higher than average: this is a build-time patch to a vendored crypto library, and REVIEW.md is explicit that vendor patches and crypto codecs get careful review (known-answer vectors over self-roundtrips, small patches with an explanatory comment). The patch is mechanical and the known-answer test satisfies the interop bar, but a maintainer should still confirm the patch applies cleanly against the pinned oven-sh/boringssl commit and that carrying it as a build patch (vs. committing directly to the fork) is the preferred maintenance path.
Other factors
The only finding this run is a nit: the new entry in the patches: array lacks its own explanatory comment/issue link, so the existing three-line comment now visually covers two unrelated patches. That's cosmetic and doesn't affect the merge decision on its own. Deferring solely because vendor crypto patches sit outside the auto-approval bar.
There was a problem hiding this comment.
Thanks for adding the per-entry patch comments in scripts/build/deps/boringssl.ts — that addresses my last note. I reviewed this pass and found no bugs; since this is a hand-written EVP cipher wrapper patched into BoringSSL, a human sign-off on the crypto change would still be worthwhile.
What was reviewed:
kCiphers[]insertions preserve alphabetical order (cfb<cfb8<ctr), soEVP_get_cipherbyname's sorted lookup is intact.- CFB8 IV chaining across
update()calls: state lives inctx->iv(mutated in place byCRYPTO_cfb128_8_encrypt); the localnum = 0is fine because CFB8 processes whole bytes and never leaves a partial-block offset. - Reuse of
aes_cfb_init_key/EVP_CFB_CTXfrom the neighboring CFB128 impl is sound — both directions use the encrypt key schedule. - The uncommented
aes-128-cfb8entry in thereferencestable gives a known-answer check beyond self-roundtrip.
Extended reasoning...
Overview
This PR adds AES-CFB8 (128/192/256) support to Bun's node:crypto by patching the vendored BoringSSL to expose EVP wrappers around its existing CRYPTO_cfb128_8_encrypt primitive. Three files: a new 149-line patch under patches/boringssl/, its registration (with a per-entry comment) in scripts/build/deps/boringssl.ts, and 7 new tests plus one uncommented known-answer vector in test/js/bun/crypto/cipheriv-decipheriv.test.ts.
Security risks
This is crypto surface. The wrapper itself is thin — it delegates to BoringSSL's own CRYPTO_cfb128_8_encrypt and reuses the neighboring CFB128 aes_cfb_init_key / EVP_CFB_CTX, so there is no novel cipher math. The main risk class would be incorrect state chaining producing output that roundtrips against itself but diverges from the standard; the known-answer vector in references["aes-128-cfb8"] (external ciphertext) guards against that for the 128-bit variant. No key-length, IV-length, or block-size fields look wrong. I did not spot a way for user input to reach an unchecked path.
Level of scrutiny
High. Per the approval guidelines, crypto and vendor patches warrant a human maintainer's sign-off even when the change is mechanical. The BoringSSL patch is hand-authored C++ that will be carried across future upstream bumps.
Other factors
All prior review threads are resolved: the out-of-scope aes-192-cfb (CFB128) registration was dropped, tests were relocated from test/regression/ into the module test file, and the patches: array now documents each entry with an issue link. The one unaddressed suggestion (coderabbit's split-update() roundtrip) was a trivial nitpick; IV state persists in ctx->iv, so multi-update chaining is covered by the primitive, though only exercised via the single-update known-answer test. The 21-test file passes on the debug build per the PR evidence.
Implement EVP_CIPHER wrappers for aes-128-cfb8, aes-192-cfb8, and aes-256-cfb8 in BoringSSL's decrepit layer, using the existing CRYPTO_cfb128_8_encrypt primitive. Register them in the cipher lookup table and EVP_CIPHER_do_all_sorted so createCipheriv/createDecipheriv and getCiphers() work with CFB8 mode. Also registers the missing aes-192-cfb (CFB128) variant. Closes #28521
05e3867 to
8c38a62
Compare
|
Rebased on main. Conflict: main removed its boringssl patches array (the memory-hooks patch moved into the oven-sh/boringssl fork), so the array now carries only the CFB8 patch. Verified the patch still applies cleanly against the new pinned commit (2288897e) and all 21 cipher tests pass on the debug build. |
|
@robobun @Jarred-Sumner @cirospaciari Instead of patching here, why not update your fork of BoringSSL directly?! This should avoid maintaining things at multiple places. Same thing I started with and ultimately ended up doing for AES CCM cipher: |
|
That is a fair point. The build patch keeps this PR self-contained: it lands in one review without coordination across two repositories, and I cannot push to oven-sh/boringssl. If the maintainers prefer the fork route, the patch applies cleanly to the pinned commit (2288897e), so folding it into the fork and dropping the patch file here is a small follow-up. Same approach your CCM work took after starting as a patch. |
This branch has not been deployed
2 participants
Problem
createCipheriv('aes-128-cfb8', ...)throwsERR_CRYPTO_UNKNOWN_CIPHER(works in Node.js).CRYPTO_cfb128_8_encryptprimitive but exposes no EVP_CIPHER wrappers for CFB8, soEVP_get_cipherbyname("aes-128-cfb8")returns NULL.Fixes #28521
Fix
patches/boringssl/expose_aes-cfb8.patch, applied at dep fetch time:decrepit/cfb/cfb.cc: EVP_CIPHER wrappers for CFB8 (128/192/256-bit keys) delegating toCRYPTO_cfb128_8_encrypt, reusing the existingaes_cfb_init_key/EVP_CFB_CTXfrom the CFB128 implementationcrypto/cipher/get_cipher.cc: registers the three cfb8 names inkCiphers[](alphabetical order preserved)decrepit/evp/evp_do_all.cc: adds cfb8 entries sogetCiphers()lists theminclude/openssl/cipher.h: declaresEVP_aes_{128,192,256}_cfb8()scripts/build/deps/boringssl.ts.Verification
test/js/bun/crypto/cipheriv-decipheriv.test.ts: creation + encrypt/decrypt roundtrip for all three key sizes,getCiphers()enumeration, and the previously commented-outaes-128-cfb8known-answer vector (external ciphertext) is re-enabled.bun bd test; the cfb8 tests fail withUSE_SYSTEM_BUN=1as expected.Background
ctx->ivand is updated in place, so state chains correctly acrossupdate()calls.decrepit/layer; this patch follows the same shape as the CFB128 wrappers that already live there.