node:tls: honor the crl option when building a SecureContext - #33629
node:tls: honor the crl option when building a SecureContext#33629robobun wants to merge 8 commits into
Conversation
The crl option on tls.createServer / tls.connect / tls.createSecureContext was silently dropped before reaching BoringSSL, so a revoked client or server certificate was treated as authorized. Plumb the option through SSLConfig into us_bun_socket_context_options_t and load each CRL into the context's X509_STORE with X509_V_FLAG_CRL_CHECK|CRL_CHECK_ALL, matching Node's SecureContext::AddCRL. An unparseable CRL now throws ERR_CRYPTO_OPERATION_FAILED at context creation instead of being ignored.
|
Updated 7:09 AM PT - Jul 7th, 2026
❌ @robobun, your commit b8ce22c has some failures in 🧪 To try this PR locally: bunx bun-pr 33629That installs a local version of the PR into your bun-33629 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The overlap with #32630 is real: that PR also wires This PR is the focused security subset only: plumb Happy to close this if #32630 is about to land; otherwise it can merge independently so the silent-CRL-bypass is fixed without waiting on the broader sync. Rebase on whichever lands first should be trivial since both touch the same set of struct fields the same way. |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds TLS CRL support across native OpenSSL handling, Rust and JS option propagation, JS error mapping, Node ChangesTLS CRL support end-to-end
Possibly related PRs
🚥 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: 2
🤖 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 `@packages/bun-usockets/src/crypto/openssl.c`:
- Around line 768-793: The CRL loader in add_crl_to_ctx_store currently treats
X509_STORE_add_crl failures and partial parsing as success, so a bad bundle can
still return a nonzero count. Update add_crl_to_ctx_store to fail fast on any
parse/store error: check the return value of X509_STORE_add_crl, stop processing
and return 0 if it fails, and avoid incrementing count unless the CRL was
actually added. Keep the existing success path in place so the caller’s
CREATE_BUN_SOCKET_ERROR_INVALID_CRL mapping still triggers correctly when any
CRL block is invalid.
In `@test/js/node/tls/node-tls-server.test.ts`:
- Around line 1338-1371: The test in node-tls-server.test.ts has a race because
the client "close" wait is attached only after awaiting the server result, so
the event may already have fired by then. Update the rejectUnauthorized:true
revoked-certificate test to register the client close listener before awaiting
result.promise, using the existing client and result promise flow, so the close
event cannot be missed if tlsClientError resolves first.
🪄 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: 6c72e3d1-32ee-4f36-ae74-8b782347ef49
📒 Files selected for processing (13)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/libusockets.hpackages/bun-uws/src/App.hsrc/http/ssl_config.rssrc/js/node/tls.tssrc/jsc/generated.rssrc/runtime/socket/SSLConfig.bindv2.tssrc/runtime/socket/SSLConfig.rssrc/runtime/socket/uws_jsc.rssrc/sql_jsc/jsc.rssrc/uws_sys/SocketContext.rssrc/uws_sys/lib.rstest/js/node/tls/node-tls-server.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ext's store as user-owned
add_crl_to_ctx_store now checks X509_STORE_add_crl's return and fails when
the PEM loop terminates on anything other than PEM_R_NO_START_LINE, so a
bundle with one valid CRL followed by garbage is rejected.
The CRL block in us_ssl_ctx_build_raw now seeds a fresh default-roots store
when the context's store is empty and sets us_ctx_user_ca_ex_idx, so the
per-socket client attach in us_internal_ssl_attach does not override the
verify store with the shared default roots and drop the CRL flags for a
tls.connect({ crl }) without an explicit ca.
Also register the client 'close' listener before awaiting the server result
in the rejectUnauthorized:true test to avoid a listener-registration race.
node-tls-server.test.ts has a pre-existing unrelated failure on main
("SNICallback runs even when the requested servername matches the bind
hostname" fails with ECONNREFUSED). Keep the CRL coverage in
node-tls-crl.test.ts so it is isolated from that.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/tls/node-tls-crl.test.ts`:
- Around line 52-132: This test suite only covers the `tls.createServer` CRL
path, so add sibling coverage for the `tls.connect` path and the BunFile input
variant. Extend the `handshake`/`connect`-based coverage to verify that passing
`crl` into `tls.connect()` correctly accepts or rejects the server certificate
based on revocation status, and add a case that uses a BunFile-backed CRL input
(for example via `Bun.file(...)`) to ensure that input type is honored. Keep the
new tests alongside the existing `tls.createServer crl option` cases and reuse
the existing fixtures/helpers like `handshake`, `serverBase`, and `ca2Crl` where
possible.
🪄 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: b92662d8-4022-4d7f-b25c-59f025f5606d
📒 Files selected for processing (1)
test/js/node/tls/node-tls-crl.test.ts
…he extern buffer Appending keeps the existing field offsets in ExternSSLConfig unchanged so the hand-written #[repr(C)] mirror only grows at the tail. Zero-initialising the MaybeUninit buffer before bindgenConvertJSToSSLConfig writes into it means a stale generated header that under-writes the struct leaves the trailing field as its None variant instead of garbage; every field's all-zeros representation is valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/tls/node-tls-crl.test.ts`:
- Around line 78-85: The TLSOptions.crl type is missing BunFile support, which
is why the test has to cast to never. Update the TLS type definition in bun.d.ts
so crl matches the existing ca and cert patterns by accepting both BunFile and
BunFile[] alongside the current string/Buffer forms, and ensure the
handshake-related types still work without any cast in the crl test.
🪄 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: a50e7151-109e-43b9-b5f8-71cdcc711341
📒 Files selected for processing (3)
src/jsc/generated.rssrc/runtime/socket/SSLConfig.bindv2.tstest/js/node/tls/node-tls-crl.test.ts
|
Status: diff is ready for review.
The internal fail-before gate's ASAN step has been bouncing with a |
|
The Checked by running this PR's The only part of this PR that is not on main is the |
1 participant
What
The
crloption ontls.createServer/tls.connect/tls.createSecureContextwas accepted without error and then dropped before reaching BoringSSL. A client certificate revoked by the supplied CRL was fully authorized (authorized === true,authorizationErrorunset) underrequestCert: true+rejectUnauthorized: true.Cause
SSLConfig(the nativeTLSOptionsmirror) had nocrlfield, so the option never reachedus_bun_socket_context_options_tor theSSL_CTX'sX509_STORE, andX509_V_FLAG_CRL_CHECKwas never set.Fix
Plumb
crl(string | Buffer | BunFile | array) end to end:SSLConfig.bindv2.ts/jsc/generated.rs/runtime/socket/SSLConfig.rs: read the option from JS.http/ssl_config.rs: store it onSSLConfig(owned, freed indeinit, part of the content hash and equality check).uws_sys/SocketContext.rs/libusockets.h/App.h: addcrl/crl_countto the C ABI options struct and its digest.openssl.c: inus_ssl_ctx_build_raw, parse each PEM CRL withPEM_read_bio_X509_CRL,X509_STORE_add_crl, then setX509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALLon the context's store (matching Node'sSecureContext::AddCRL). The store is cloned first if it is the process-wide shared default so the CRL flags do not leak across contexts.ERR_CRYPTO_OPERATION_FAILED("Failed to parse CRL") at context creation instead of being ignored.tls.ts: validateoptions.crlthe same wayca/cert/keyare validated, and forward it from theServeroptions to the native listener.Verification
Using the Node test fixtures (agent4 is signed by ca2 and revoked by ca2-crl; agent3 is not revoked):
All of the CERT_REVOKED cases fail against the released binary (the revoked client is authorized).