tls: implement SecureContext.addCACert#30486
Conversation
|
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:
WalkthroughAdds SecureContext.prototype.addCACert: C API to append PEM CA certs to SSL_CTX, cache invalidation (native + JS), Zig/JSC bindings, JS exposure, internal test helper, and comprehensive tests validating behavior and trust-anchor preservation. ChangesTLS Secure Context addCACert Implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 `@src/runtime/api/bun/SecureContext.zig`:
- Around line 168-175: The JS SecureContext entry must be removed from every
JSGlobal in the VM, not just the current global; instead of calling
cpp.Bun__SecureContextCache__remove only with the current global, iterate all
globals owned by global.bunVM() and call cpp.Bun__SecureContextCache__remove for
each using the same key derived from this.digest (the u64 little-endian from
this.digest[0..8]) after invalidating the native sslCtxCache via
global.bunVM().rareData().sslCtxCache().invalidate(this.ctx, this.digest), so
every global’s WeakGCMap entry for that digest is cleared and future
createSecureContext(sameOptions) calls will rebuild.
In `@test/js/node/tls/node-tls-context.test.ts`:
- Around line 667-690: The test "preserves OS/default trust anchors" currently
depends on external network (tls.connect to bun.sh), make it deterministic by
replacing the external connection with a local TLS server (use tls.createServer
or an HTTPS server) and assert the same behavior: call
tls.createSecureContext(), use ctx.context.addCACert(ca2) as before, connect to
the local server with tls.connect and check client.authorized via the same
Promise.withResolvers pattern; alternatively, if you want minimal change, catch
network errors from tls.connect and skip/mark the test as flaky on network
failure — adjust the test around tls.createSecureContext, ctx.context.addCACert,
tls.connect, and Promise.withResolvers to implement the local-server approach or
conditional skip.
🪄 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: b077ca78-42d3-4432-aefe-73de2e4c33b8
📒 Files selected for processing (9)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/libusockets.hsrc/jsc/bindings/BunSecureContextCache.cppsrc/jsc/bindings/BunSecureContextCache.hsrc/runtime/api/SecureContext.classes.tssrc/runtime/api/bun/SSLContextCache.zigsrc/runtime/api/bun/SecureContext.zigsrc/uws_sys/SocketContext.zigtest/js/node/tls/node-tls-context.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/node/tls/node-tls-context.test.ts (1)
691-714:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hard external-network dependency in this trust-anchor test.
Line 701 reaches
bun.sh:443, which can fail due to DNS/network/transient CI conditions unrelated toaddCACert, making this test flaky. Please make this deterministic (preferred) or explicitly handle network-unavailable errors so CI stability is not coupled to internet reachability.As per coding guidelines: “Do not write flaky tests.”
🤖 Prompt for 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. In `@test/js/node/tls/node-tls-context.test.ts` around lines 691 - 714, The test "preserves OS/default trust anchors" currently relies on connecting to bun.sh:443 which is flaky; change it to avoid external network by either (A) making the assertion against a local TLS server: create a local tls.createServer using a certificate signed by your unrelated CA (ca2), call tls.createSecureContext().context.addCACert(ca2) and connect to the local server with tls.connect to verify client.authorized is true, or (B) if you must keep the external check, catch network/DNS errors on client.on("error") (e.g., ENOTFOUND, EAI_AGAIN, ECONNREFUSED, ENETUNREACH) and treat them as a skipped test (log a warning and resolve/return without failing) so CI flakiness is avoided; update the test around tls.createSecureContext, ctx.context.addCACert, tls.connect and client.on("error") accordingly.
🤖 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.
Duplicate comments:
In `@test/js/node/tls/node-tls-context.test.ts`:
- Around line 691-714: The test "preserves OS/default trust anchors" currently
relies on connecting to bun.sh:443 which is flaky; change it to avoid external
network by either (A) making the assertion against a local TLS server: create a
local tls.createServer using a certificate signed by your unrelated CA (ca2),
call tls.createSecureContext().context.addCACert(ca2) and connect to the local
server with tls.connect to verify client.authorized is true, or (B) if you must
keep the external check, catch network/DNS errors on client.on("error") (e.g.,
ENOTFOUND, EAI_AGAIN, ECONNREFUSED, ENETUNREACH) and treat them as a skipped
test (log a warning and resolve/return without failing) so CI flakiness is
avoided; update the test around tls.createSecureContext, ctx.context.addCACert,
tls.connect and client.on("error") accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e94d8a0-6e11-4386-956d-b82c6e4f4350
📒 Files selected for processing (1)
test/js/node/tls/node-tls-context.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 `@test/js/node/tls/node-tls-context.test.ts`:
- Around line 743-747: The test currently asserts exitCode before checking the
child process payload; change the assertion order so you validate
stdout/stderr/parsed result first and only then assert exitCode. Specifically,
after awaiting proc.stdout.text(), proc.stderr.text(), and proc.exited into
[stdout, stderr, exitCode], parse and assert stdout/ stderr and the JSON
result.authorized (using stdout.trim() → JSON.parse → result.authorized) before
calling expect(exitCode).toBe(0); keep the same variables (proc, stdout, stderr,
exitCode, result) and assertions but move the exitCode check to the end.
- Around line 632-643: The test "silently ignores non-PEM input (Node parity)"
currently asserts that ctx.context.addCACert accepts null, undefined, numbers
and plain objects; narrow it to the intended input surface by removing those
assertions and only keep byte-like/malformed inputs (e.g., string values like ""
and "not pem" and Buffer/TypedArray/ArrayBuffer cases such as Buffer.alloc(0))
when calling ctx.context.addCACert; leave zero-arg and other invalid-type
behavior to their dedicated tests elsewhere.
🪄 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: c5449b41-03a2-4846-b008-0e1330dee39a
📒 Files selected for processing (1)
test/js/node/tls/node-tls-context.test.ts
Two review findings on the addCACert implementation:
1. The previous revision called SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, ...)
so that us_internal_ssl_attach's client branch wouldn't override the
CTX's cert store with the shared default roots. But SecureContext is
mode-neutral — the same object may back both tls.connect and
tls.createServer — and VERIFY_PEER on a server CTX makes BoringSSL send
CertificateRequest in every handshake, even when the user never set
requestCert. This broke the invariant documented in us_ssl_ctx_from_options
("MUST NOT force VERIFY_PEER here") and diverged from Node's
SecureContext::AddCACert, which leaves verify_mode alone.
Replace the verify_mode flip with an ex_data marker (us_ctx_user_ca_idx)
set when addCACert installs a non-default cert store. The client branch
of us_internal_ssl_attach reads the marker and skips its per-socket
SSL_set0_verify_cert_store override when set, so the CTX's own store
(default roots + user CAs) is the one consulted for verification.
2. Also call SSL_CTX_add_client_CA(ctx, x) inside the PEM loop, matching
Node's SecureContext::AddCACert and Bun's own construction-time
add_ca_cert_to_ctx_store. This surfaces the CA's DN in the
CertificateRequest's certificate_authorities list for servers that do
set requestCert — picky clients (browsers, smartcard middleware) use it
to pick among client certs.
Adds bun:internal-for-testing.secureContextVerifyMode so the regression
is gate-able: a test asserting the CTX stays SSL_VERIFY_NONE after
addCACert fails before the fix (received 1 = VERIFY_PEER) and passes
after (0 = VERIFY_NONE).
Resolves the two findings on #30486.
robobun
force-pushed
the
farm/e15c357d/tls-add-ca-cert
branch
from
48e454c to
05bd65b
Compare
Two review findings on the addCACert implementation:
1. The previous revision called SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, ...)
so that us_internal_ssl_attach's client branch wouldn't override the
CTX's cert store with the shared default roots. But SecureContext is
mode-neutral — the same object may back both tls.connect and
tls.createServer — and VERIFY_PEER on a server CTX makes BoringSSL send
CertificateRequest in every handshake, even when the user never set
requestCert. This broke the invariant documented in us_ssl_ctx_from_options
("MUST NOT force VERIFY_PEER here") and diverged from Node's
SecureContext::AddCACert, which leaves verify_mode alone.
Replace the verify_mode flip with an ex_data marker (us_ctx_user_ca_idx)
set when addCACert installs a non-default cert store. The client branch
of us_internal_ssl_attach reads the marker and skips its per-socket
SSL_set0_verify_cert_store override when set, so the CTX's own store
(default roots + user CAs) is the one consulted for verification.
2. Also call SSL_CTX_add_client_CA(ctx, x) inside the PEM loop, matching
Node's SecureContext::AddCACert and Bun's own construction-time
add_ca_cert_to_ctx_store. This surfaces the CA's DN in the
CertificateRequest's certificate_authorities list for servers that do
set requestCert — picky clients (browsers, smartcard middleware) use it
to pick among client certs.
Adds bun:internal-for-testing.secureContextVerifyMode so the regression
is gate-able: a test asserting the CTX stays SSL_VERIFY_NONE after
addCACert fails before the fix (received 1 = VERIFY_PEER) and passes
after (0 = VERIFY_NONE).
Resolves the two findings on #30486.
robobun
force-pushed
the
farm/e15c357d/tls-add-ca-cert
branch
from
05bd65b to
43afd6f
Compare
Two review findings on the addCACert implementation:
1. The previous revision called SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, ...)
so that us_internal_ssl_attach's client branch wouldn't override the
CTX's cert store with the shared default roots. But SecureContext is
mode-neutral — the same object may back both tls.connect and
tls.createServer — and VERIFY_PEER on a server CTX makes BoringSSL send
CertificateRequest in every handshake, even when the user never set
requestCert. This broke the invariant documented in us_ssl_ctx_from_options
("MUST NOT force VERIFY_PEER here") and diverged from Node's
SecureContext::AddCACert, which leaves verify_mode alone.
Replace the verify_mode flip with an ex_data marker (us_ctx_user_ca_idx)
set when addCACert installs a non-default cert store. The client branch
of us_internal_ssl_attach reads the marker and skips its per-socket
SSL_set0_verify_cert_store override when set, so the CTX's own store
(default roots + user CAs) is the one consulted for verification.
2. Also call SSL_CTX_add_client_CA(ctx, x) inside the PEM loop, matching
Node's SecureContext::AddCACert and Bun's own construction-time
add_ca_cert_to_ctx_store. This surfaces the CA's DN in the
CertificateRequest's certificate_authorities list for servers that do
set requestCert — picky clients (browsers, smartcard middleware) use it
to pick among client certs.
Adds bun:internal-for-testing.secureContextVerifyMode so the regression
is gate-able: a test asserting the CTX stays SSL_VERIFY_NONE after
addCACert fails before the fix (received 1 = VERIFY_PEER) and passes
after (0 = VERIFY_NONE).
Resolves the two findings on #30486.
robobun
force-pushed
the
farm/e15c357d/tls-add-ca-cert
branch
from
de74a06 to
fd18185
Compare
There was a problem hiding this comment.
All prior review findings have been addressed and nothing new turned up, but this touches TLS trust-store semantics across C/BoringSSL, C++, and Rust with several subtle invariants — worth a human pass before merging.
Extended reasoning...
Overview
This PR implements Node's SecureContext.prototype.addCACert across 15 files: a new us_ssl_ctx_add_ca_pem helper and us_ctx_user_ca_idx ex_data marker in packages/bun-usockets/src/crypto/openssl.c (with hooks in three construction-time branches of us_ssl_ctx_build_raw and the client branch of us_internal_ssl_attach), C++ SecureContextCache::remove, Rust SecureContext::add_ca_cert / SSLContextCache::invalidate / the ssl_wrapper client-attach gate, parallel (dead-reference) Zig changes, JS class definitions, an internal-for-testing helper, and ~380 lines of tests.
Security risks
This is squarely security-sensitive: it mutates the X.509 trust store used for TLS peer verification and changes when the per-SSL default-roots override is applied. Earlier revisions had real correctness issues (CTX-level verify_mode flip leaking as server CertificateRequest; first addCACert on a {ca: …} context discarding construction-time CAs; the Duplex/SSLWrapper path discarding added CAs) — all fixed and now gated by tests, but the class of risk is exactly the kind that warrants human eyes.
Level of scrutiny
High. The change is neither simple nor mechanical: it threads an invariant ("CTX store is authoritative, do not override") through four producers and two consumers across three languages, interacts with two layers of caching keyed by config digest, and documents explicit Node-parity compromises (pre-mutation cells sharing an SSL_CTX, cross-global cache invalidation limits). Those are design decisions a maintainer should sign off on.
Other factors
No new findings from the bug-hunting system this run; every prior inline comment from me and CodeRabbit is resolved. Test coverage is good (12 new cases including the verify_mode gate, construction-time-CA preservation, and the Duplex/SSLWrapper path). I have not posted a review body on this PR before, so this deferral signals the bot review cycle is complete but auto-approval is not appropriate for TLS trust-store code of this scope.
|
CI on I already spent my one CI re-roll earlier ( |
tls.createSecureContext().context.addCACert was undefined — the native
SecureContext class had an empty prototype. Add it to the proto surface
so it matches Node's API.
Also fix tls.createSecureContext() (no arg) throwing 'TLSOptions must
be an object': Bindgen's generated converter requires an object, but
Node's WebIDL treats missing options as an empty dictionary.
Implementation mirrors Node's lenient behavior:
- Accepts strings and Buffers/TypedArrays/ArrayBuffers, coerces
anything else to a silent no-op
- Empty / malformed / duplicate PEM input is silently ignored
- Multi-cert bundles in a single call are walked to completion
The SSL_CTX path mirrors the existing 'options.ca' flow in
us_ssl_ctx_build_raw: on first call against a VERIFY_NONE context,
install us_get_default_ca_store() (OS roots + baked-in Bun roots) so
the added CA joins (not replaces) the existing trust anchors, and
flip verify_mode to SSL_VERIFY_PEER so the per-socket client override
in us_internal_ssl_attach doesn't discard the mutation.
Before mutating, drop the SecureContext from both the per-VM
SSLContextCache and the per-global Bun__SecureContextCache so a later
createSecureContext(sameOptions) builds a fresh SSL_CTX instead of
handing back the mutated one. Preserves the 'one config, one SSL_CTX'
hot path for the pure createSecureContext({ca, cert, key}) pattern.
Fixes #30485
…TS fixture The bun.sh test was flagged as a CI flake vector (network, cert rotation). Replace with a subprocess test that stashes ca1 in NODE_EXTRA_CA_CERTS so the 'default store' under test is deterministic and local. Still asserts ADD-not-REPLACE semantics: addCACert(ca2) on a fresh context must keep ca1-signed agent1 trusted. Also expand the addCACert doc comment to flag the cross-global caveat coderabbit raised — Bun is one-global-per-VM in production, so the sibling-global cache-staleness edge is sandbox-only and not worth the iteration machinery right now.
Two review findings on the addCACert implementation:
1. The previous revision called SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, ...)
so that us_internal_ssl_attach's client branch wouldn't override the
CTX's cert store with the shared default roots. But SecureContext is
mode-neutral — the same object may back both tls.connect and
tls.createServer — and VERIFY_PEER on a server CTX makes BoringSSL send
CertificateRequest in every handshake, even when the user never set
requestCert. This broke the invariant documented in us_ssl_ctx_from_options
("MUST NOT force VERIFY_PEER here") and diverged from Node's
SecureContext::AddCACert, which leaves verify_mode alone.
Replace the verify_mode flip with an ex_data marker (us_ctx_user_ca_idx)
set when addCACert installs a non-default cert store. The client branch
of us_internal_ssl_attach reads the marker and skips its per-socket
SSL_set0_verify_cert_store override when set, so the CTX's own store
(default roots + user CAs) is the one consulted for verification.
2. Also call SSL_CTX_add_client_CA(ctx, x) inside the PEM loop, matching
Node's SecureContext::AddCACert and Bun's own construction-time
add_ca_cert_to_ctx_store. This surfaces the CA's DN in the
CertificateRequest's certificate_authorities list for servers that do
set requestCert — picky clients (browsers, smartcard middleware) use it
to pick among client certs.
Adds bun:internal-for-testing.secureContextVerifyMode so the regression
is gate-able: a test asserting the CTX stays SSL_VERIFY_NONE after
addCACert fails before the fix (received 1 = VERIFY_PEER) and passes
after (0 = VERIFY_NONE).
Resolves the two findings on #30486.
Review found a real regression in the f1ddc9e ex_data-marker design: the first addCACert() on a SecureContext built with options.ca / options.ca_file_name / options.request_cert would swap in a fresh us_get_default_ca_store(), silently discarding the construction-time CAs (SSL_CTX_set_cert_store frees the previous store). Fix: set us_ctx_user_ca_idx in all three build_raw branches that install a populated trust store. That way the marker means 'this CTX's own store already has default roots + user CAs' regardless of which path supplied them, and addCACert correctly appends to the existing store instead of replacing it. Gated by a new regression test that fails before the fix (agent1 cert signed by construction-time ca1 no longer authorized after addCACert(ca2)) and passes after. Also resolves the remaining coderabbit/claude review nits: - Narrow the lenient-input test to the documented byte-like surface (string/Buffer/TypedArray/ArrayBuffer); drop the null/undefined/number/ plain-object assertions that went past the PR contract. - Static import for secureContextVerifyMode; no await-import in tests. - Assert the child payload before the exit code in the subprocess fixture, per CLAUDE.md. - Update stale doc comments in libusockets.h and SocketContext.zig to describe the ex_data-marker mechanism instead of the old VERIFY_PEER flip.
…mantics Doc-only: the 'Second+ addCACert' branch now also covers contexts whose user CAs were installed at construction by us_ssl_ctx_build_raw. Reword so future readers don't assume the marker means 'prior addCACert only'.
Review caught that src/runtime/socket/ssl_wrapper.zig's client branch mirrors us_internal_ssl_attach's per-SSL trust-store override but wasn't updated in the f1ddc9e redesign. Result: TLS run on top of a Duplex stream (tls.connect({socket: duplex}) / UpgradedDuplex / named pipes) discarded addCACert'd CAs because the wrapper still unconditionally called SSL_set0_verify_cert_store with shared default roots when the CTX's verify_mode was VERIFY_NONE — which is the post-f1ddc9e state. Expose us_ctx_has_user_ca as a public usockets C symbol and have the SSLWrapper honor it the same way. Gated by a new test that runs a tls.connect over a Duplex-proxied socket using a context whose only trust anchor is the addCACert'd CA; the test fails before this fix (client.authorized == false) and passes after. Also repurpose the duplicate 'accepts no arg options' test — byte-for- byte identical to the 'is a function' test above it — to cover the createSecureContext(null) branch, which was previously unexercised.
Per test/CLAUDE.md, dynamic imports are only for tests that exercise dynamic-import semantics. The Duplex-wrapped client test I added in 479c8ed used await import() for convenience; move them to static imports alongside the other node: imports.
Two review nits on cache invalidation edge cases:
1. Bail before touching either cache when bytes.len == 0.
us_ssl_ctx_add_ca_pem short-circuits on empty input without
mutating the SSL_CTX, so eviction just forces an unnecessary
SSL_CTX rebuild on the next createSecureContext(sameOptions) with
no actual state change to drop.
2. Guard the JS-cache remove with Bun__SecureContextCache__get(key)
== callframe.this(), mirroring SSLContextCache.invalidate's
'if (entry.ctx == ctx)' guard. Otherwise the pattern
const a = createSecureContext(X); // cache[X]=A
a.context.addCACert(Y); // both caches invalidated
const b = createSecureContext(X); // cache[X]=B (fresh)
a.context.addCACert(Z); // native no-ops (correct); JS
// evicted B (wrong)
breaks === identity on the next createSecureContext(X) and costs
one extra wrapper allocation. No TLS-correctness impact either
way — the underlying SSL_CTX* is still shared via the native
cache — but the guards bring the two paths into parity and make
the 'idempotent on repeat calls' doc comment fully accurate.
Main merged the Rust rewrite (#30412): SecureContext, SSLContextCache, and the SSLWrapper client-attach path are now Rust. Port the addCACert implementation from the (now-dead) .zig files to their .rs counterparts: - SecureContext.rs: add_ca_cert host_fn (StringOrBuffer input, empty-bytes and JS-cache-identity guards, dual cache invalidation), js_verify_mode test helper, Bun__SecureContextCache__remove extern; handle undefined/null in constructor() and intern() so createSecureContext() with no args doesn't throw 'TLSOptions must be an object'. - SSLContextCache.rs: invalidate() — drop a mutated SSL_CTX from the per-VM cache, guarded on entry.ctx == ctx. - uws/lib.rs (SSLWrapper): gate the per-SSL trust-store override on us_ctx_has_user_ca so addCACert'd CAs survive the Duplex/UpgradedDuplex TLS path, mirroring us_internal_ssl_attach. - uws_sys/SocketContext.rs: us_ssl_ctx_add_ca_pem / us_ctx_has_user_ca externs. The C helpers (us_ssl_ctx_add_ca_pem, us_ctx_has_user_ca) and the construction-time marker-setting in us_ssl_ctx_build_raw are unchanged — they live in openssl.c, which the Rust rewrite didn't touch. Resolved the rebase conflict in openssl.c by keeping main's strict 'explicit ca replaces the default trust store' (Node.js) semantics and layering the addCACert marker on top. All 20 addCACert tests pass; ssl-ctx-cache, node-tls-server, node-tls-cert, and the Duplex/upgrade SSLWrapper tests stay green.
Describe all setters (addCACert + the three build_raw construction-time branches) and both readers (us_internal_ssl_attach and Rust SSLWrapper via us_ctx_has_user_ca), matching the unified marker semantics.
- SSLContextCache::invalidate takes an SSL_CTX* used only as a comparison key / forwarded by value to SSL_CTX_set_ex_data (never dereferenced), so annotate not_unsafe_ptr_arg_deref as a false positive. - Move the SAFETY comment in add_ca_cert to immediately precede its unsafe block (a debug_assert sat between them, tripping undocumented_unsafe_blocks).
The us_ctx_user_ca_idx declaration and the us_ssl_ctx_add_ca_pem if(marked) branch still described the pre-rebase 'default roots + user CAs' for every producer. Post strict-CA-rebase the options.ca / ca_file_name branches install user CAs only, requestCert installs default roots only, and only a prior addCACert yields both. Reword both comments to state the real per-producer contents; the marker's invariant (CTX store is authoritative, don't override) is unchanged. Comment-only, no runtime effect.
- us_ctx_has_user_ca was declared `safe fn` in src/uws/lib.rs, but the C impl dereferences ctx via SSL_CTX_get_ex_data, so a null arg is UB. Drop `safe` to match both its unsafe sibling in SocketContext.rs and the adjacent us_ssl_socket_verify_error_from_ssl. The sole call site is already inside an unsafe block, so no call-site change. - Two comments still pointed at the pre-rebase Zig SSLWrapper: openssl.c's us_ctx_has_user_ca doc and the Duplex-path test comment. Point both at the live Rust ssl_wrapper module in src/uws/lib.rs. Comment/signature only, no runtime effect.
The comment referenced the pre-rebase Zig paths and camelCase `isUndefinedOrNull`; the live short-circuit is Rust `is_undefined_or_null()` in SecureContext.rs. Comment-only.
The "Called from Zig" doc on Bun__SecureContextCache__get/__remove and the "Zig SecureContext" reference predate/ignore the #30412 rewrite; the live callers are Rust (SecureContext::intern / add_ca_cert). Comment-only.
robobun
force-pushed
the
farm/e15c357d/tls-add-ca-cert
branch
from
c8f59c0 to
77d563d
Compare
There was a problem hiding this comment.
All my prior inline findings have been addressed and the latest pass found nothing new, but this touches TLS trust-store/verify-mode handling across the C attach path, the Rust SSLWrapper, and two-level SSL_CTX caching — security-sensitive enough that it should get a human sign-off before merge.
Extended reasoning...
Overview
This PR implements SecureContext.prototype.addCACert for Node.js tls parity. It spans 15 files across five languages: a new us_ssl_ctx_add_ca_pem helper plus an us_ctx_user_ca_idx ex_data marker in packages/bun-usockets/src/crypto/openssl.c; gating of the per-socket trust-store override in both us_internal_ssl_attach (C) and the Rust ssl_wrapper module; cache-invalidation plumbing in SSLContextCache.{rs,zig} and BunSecureContextCache.{cpp,h}; the JS-facing prototype method in SecureContext.{rs,zig} / SecureContext.classes.ts; a test-only secureContextVerifyMode introspection helper; and ~380 lines of new tests in node-tls-context.test.ts. It also relaxes createSecureContext(undefined|null) to match Node's empty-dictionary semantics.
Security risks
The change directly manipulates the BoringSSL X509 trust store and the conditions under which the per-SSL client override replaces it with shared default roots. The review history surfaced three real correctness bugs in earlier revisions (CTX verify_mode flip leaking as server CertificateRequest; first addCACert on a {ca: ...}-built context discarding construction-time CAs; the parallel SSLWrapper/Duplex client-attach path not honoring the marker), all now fixed and gated by tests. The remaining design accepts a documented limitation: two JS cells obtained for the same digest before the first addCACert still share one SSL_CTX, so a mutation through one is visible through the other. That's a deliberate trade-off (no SSL_CTX_dup), but it's exactly the kind of trust-boundary nuance a maintainer should ratify.
Level of scrutiny
High. This is production TLS verification logic, not a config tweak — it changes what certificate chains are trusted and under what conditions, and it modifies construction-time behavior for all {ca} / {caFile} / {requestCert} contexts (they now set the ex_data marker). The PR went through ~18 rounds of inline review with multiple substantive fixes; that history argues for, not against, a final human pass.
Other factors
Test coverage is thorough (12 new tests including a Duplex-path regression, verify_mode gate, construction-time-CA-preservation gate, and a deterministic NODE_EXTRA_CA_CERTS subprocess fixture). All prior inline comments — mine and CodeRabbit's — are resolved. CI on the prior head was 73/74 green with one expired (not failed) macOS lane; the current head 77d563d is building. The bug-hunting pass on the latest revision found nothing. None of that changes the bar: TLS trust-store code should not be auto-approved.
|
CI on 77d563d: the only hard failure is I already used my one retrigger earlier, so I'm not pushing another. The TLS diff itself is green: all build lanes, cargo clippy, and the TLS test suite pass. Needs a maintainer to re-run or merge as-is. |
|
Superseded by #31155, which merged to main and already implements both things this PR set out to fix:
Rebasing this branch onto main now conflicts directly on A few Node-compat refinements from this branch are not in main's version, in case they're wanted as a follow-up:
Closing this out as a duplicate; happy to re-open a focused follow-up for any of the above if a maintainer wants it. |
1 participant
Fixes #30485
Reproduction
Root cause
The native
SecureContextclass inSecureContext.classes.tsdeclared anempty
proto: {}, so none of the methods Node exposes onsecureContext.context(addCACert,addCRL,setCert, …) existed.Secondary:
tls.createSecureContext()with no argument threwTLSOptions must be an objectbecause bindgen's generated converter isstrict; WebIDL treats missing options as an empty dictionary.
Fix
addCACertto theSecureContextprototype, backed by a newus_ssl_ctx_add_ca_pemhelper inpackages/bun-usockets/src/crypto/openssl.cthat mirrors the existing
options.capath: on first call against aVERIFY_NONEcontext, installus_get_default_ca_store()so the added CAjoins (not replaces) OS/baked-in roots, flip verify_mode to
SSL_VERIFY_PEERso the per-socket client override inus_internal_ssl_attachdoesn't discard it, and append each PEM block.Buffer/TypedArray/ArrayBufferaccepted; empty/malformed/duplicate input silently ignored; multi-cert
bundles walked to completion.
SecureContextfrom both the per-VMSSLContextCacheand the per-globalBun__SecureContextCacheso a latercreateSecureContext(sameOptions)builds a freshSSL_CTXinstead ofhanding back the mutated one. Preserves the "one config, one SSL_CTX" hot
path for the pure
createSecureContext({ca, cert, key})pattern.undefined/nullatintern()/constructor()to skip bindgen'snon-object check (matches Node's empty-dictionary semantics).
Verification
test/js/node/tls/node-tls-context.test.tscoveringthe basic surface, trust-chain acceptance, rejection of unrelated CAs,
Buffer input, multi-cert bundles, preservation of default trust anchors,
duplicate-idempotency, and the no-arg throw.
SecureContext-adjacent tests in the same file still pass,plus
ssl-ctx-cache.test.ts(5),node-tls-connect.test.ts(26+),node-tls-create-secure-context-args.test.ts(5).Rebase note (main merged the Rust rewrite #30412)
Rebasing onto main surfaced two large upstream changes:
Rust rewrite (Rewrite Bun in Rust #30412):
SecureContext,SSLContextCache, and theSSLWrapperclient-attach path are now Rust. The implementation was portedfrom the (now-dead)
.zigfiles toSecureContext.rs/SSLContextCache.rs/
uws/lib.rs/uws_sys/SocketContext.rs. The C helpers(
us_ssl_ctx_add_ca_pem,us_ctx_has_user_ca) live inopenssl.c, which therewrite didn't touch, so they carried over unchanged.
Strict CA semantics on main: an explicit
ca/ca_file_name/requestCertoption now replaces the default trust store (Node.jssemantics — chains validate only against supplied CAs) instead of merging with
it. The
openssl.cconflict was resolved by keeping main's strict-replacebehavior and layering the
addCACertmarker (us_ctx_user_ca_idx) on top, soa later
addCACertappends to the construction-time store rather than swappingin the default roots.
All 20
addCACerttests pass after the rebase;ssl-ctx-cache,node-tls-server,node-tls-cert, and the Duplex/upgradeSSLWrappertestsstay green.