node:tls: implement setDefaultCACertificates() - #30570
Conversation
|
Updated 5:19 AM PT - Jun 17th, 2026
❌ @robobun, your commit cb396e9 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30570That installs a local version of the PR into your bun-30570 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
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 process-global, mutex-protected user override for the default TLS CA store with native setter/getter, JSC host functions for reset/read (PEM parsing and dedupe), TypeScript binding tls.setDefaultCACertificates(), and comprehensive tests. ChangesUser-Overridable Default CA Certificates
🚥 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: 3
🤖 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/root_certs.cpp`:
- Around line 193-205: The override state is currently process-global
(shared_store_mutex, shared_store, user_root_certs, has_user_root_certs) which
allows one Worker to affect all Workers; change these globals to be thread-local
so the override is scoped per Worker/thread: replace the static globals with
thread_local equivalents (e.g., thread_local X509_STORE *shared_store,
thread_local STACK_OF(X509)* user_root_certs, thread_local bool
has_user_root_certs) and remove or narrow the use of shared_store_mutex
accordingly so that us_get_default_ca_store() and
us_get_shared_default_ca_store() operate on per-thread stores and no longer
silently change TLS verification for other Workers. Ensure any
initialization/cleanup paths that reference those symbols use the thread-local
variants.
In `@src/jsc/bindings/NodeTLS.cpp`:
- Around line 277-300: The loop currently walking STACK_OF(X509)* certs returned
by us_get_user_root_certs() can race with us_set_user_root_certs(); take a
stable snapshot under the same lock used by us_set_user_root_certs() (or perform
PEM serialization while holding that lock) before iterating: acquire the lock,
copy or up-ref each X509 (or serialize to PEM while locked) so
sk_X509_num/sk_X509_value are safe, then release the lock and continue the
BIO/PEM conversion and the existing error handling (references:
us_get_user_root_certs, us_set_user_root_certs, certs, sk_X509_num,
sk_X509_value, BIO_new/BIO_free, PEM_write_bio_X509, throwOutOfMemoryError,
throwError).
In `@test/js/node/tls/node-tls-set-default-ca-certificates.test.ts`:
- Around line 174-179: The test currently only checks lengths after rollback
which can false-pass; change the assertions to verify the actual certificates
were restored by comparing the certificate arrays directly (use deep equality
between the variables before and after returned from
tls.getCACertificates("default")) or, alternatively, compare extracted cert
metadata (e.g., fingerprint/subject fields) from before and after to ensure the
rollback restored the exact same cert set; update the assertions around
tls.getCACertificates("default") and tls.setDefaultCACertificates(["not a
certificate"]) accordingly (replace assert.strictEqual(after.length,
before.length) with a deep comparison of before and after or their metadata).
🪄 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: 528db0aa-00c9-43cc-b433-8367014e6e01
📒 Files selected for processing (7)
packages/bun-usockets/src/crypto/root_certs.cpppackages/bun-usockets/src/crypto/root_certs_header.hsrc/js/node/tls.tssrc/jsc/bindings/NodeTLS.cppsrc/jsc/bindings/NodeTLS.htest/js/node/test/common/tls.jstest/js/node/tls/node-tls-set-default-ca-certificates.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/jsc/bindings/NodeTLS.cpp (1)
277-300:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTake a stable snapshot before walking
user_root_certs.This still iterates the raw
STACK_OF(X509)*returned byus_get_user_root_certs()with no synchronization. A concurrenttls.setDefaultCACertificates()can free or replace that stack mid-loop and turnsk_X509_value/PEM_write_bio_X509into a use-after-free.🤖 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 `@src/jsc/bindings/NodeTLS.cpp` around lines 277 - 300, The code currently iterates the raw STACK_OF(X509)* from us_get_user_root_certs() (using sk_X509_num/sk_X509_value and calling PEM_write_bio_X509) without synchronization, risking use-after-free if tls.setDefaultCACertificates() replaces or frees the stack; to fix, take a stable snapshot immediately after calling us_get_user_root_certs(): copy the X509* entries into a local container (or call X509_up_ref on each and push into std::vector<X509*>) and then iterate that snapshot when calling PEM_write_bio_X509/sk_X509_value; after the loop release the references (X509_free) or clear the vector; handle nullptr/zero-size the same as before and preserve existing error/BIO handling (functions referenced: us_get_user_root_certs, sk_X509_num, sk_X509_value, PEM_write_bio_X509).
🤖 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/jsc/bindings/NodeTLS.cpp`:
- Around line 195-200: The CA override is currently process-global via the
static symbols user_root_certs and shared_store_mutex, causing one Worker to
affect all Workers; change the implementation so the override is scoped
per-Worker (as Node.js does) by moving user_root_certs and its mutex out of
static globals into per-Worker storage (e.g., thread_local or a member on the
Worker/Isolate object) and update us_set_user_root_certs, any getters/setters,
and code paths that reference shared_store_mutex to operate on the per-Worker
variable/lock instead; ensure initialization, cleanup, and access patterns
mirror Node.js semantics (explicit empty override vs no override) and that
concurrent access is protected by the per-Worker mutex.
- Around line 151-156: The function appendX509sFromPEM performs an unchecked
cast of data.size() to int for BIO_new_mem_buf; add a guard at the top of
appendX509sFromPEM that rejects inputs where data.size() > INT_MAX before
calling BIO_new_mem_buf (and before the size_t→int cast), set/put an OpenSSL
error (e.g., via ERR_put_error) and return a non-zero error code (like
ERR_peek_last_error()) so oversized buffers are rejected safely; reference
appendX509sFromPEM, data.size(), BIO_new_mem_buf, and INT_MAX when locating the
change.
---
Duplicate comments:
In `@src/jsc/bindings/NodeTLS.cpp`:
- Around line 277-300: The code currently iterates the raw STACK_OF(X509)* from
us_get_user_root_certs() (using sk_X509_num/sk_X509_value and calling
PEM_write_bio_X509) without synchronization, risking use-after-free if
tls.setDefaultCACertificates() replaces or frees the stack; to fix, take a
stable snapshot immediately after calling us_get_user_root_certs(): copy the
X509* entries into a local container (or call X509_up_ref on each and push into
std::vector<X509*>) and then iterate that snapshot when calling
PEM_write_bio_X509/sk_X509_value; after the loop release the references
(X509_free) or clear the vector; handle nullptr/zero-size the same as before and
preserve existing error/BIO handling (functions referenced:
us_get_user_root_certs, sk_X509_num, sk_X509_value, PEM_write_bio_X509).
🪄 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: 69012620-6f50-4000-9ffa-31759fb3854d
📒 Files selected for processing (1)
src/jsc/bindings/NodeTLS.cpp
![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
♻️ Duplicate comments (1)
src/jsc/bindings/NodeTLS.cpp (1)
151-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the
size_t→intcast beforeBIO_new_mem_buf().
BIO_new_mem_buf()takes a signed length. Ifdata.size() > INT_MAX, the cast wraps negative and BoringSSL/OpenSSL switches to NUL-terminated semantics, so an oversizedArrayBufferViewcan read past the provided buffer. Reject oversized inputs before the cast and surface the existing crypto failure path.🤖 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 `@src/jsc/bindings/NodeTLS.cpp` around lines 151 - 156, In appendX509sFromPEM, guard the size_t→int cast before calling BIO_new_mem_buf by checking if data.size() > INT_MAX and rejecting oversized inputs via the same crypto failure path (i.e., return the existing error code used on failure such as ERR_peek_last_error()) so the cast is never performed and BIO_new_mem_buf is not given a wrapped negative length; adjust the logic around BIO_new_mem_buf/BIO_new_mem_buf(data.data(), static_cast<int>(data.size())) 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.
Inline comments:
In `@src/jsc/bindings/NodeTLS.cpp`:
- Around line 159-163: The loop that reads certs with PEM_read_bio_X509 and
calls sk_X509_push currently ignores sk_X509_push's return value, so on
allocation failure the code still increments pushed (misleading count) and in
the other location calls X509_up_ref before pushing (causing leaks when push
fails); update both sites to check sk_X509_push() and treat a return of 0 as an
error: if push fails, free the newly created X509 (and if X509_up_ref was
already called, decrement/refclean appropriately), pop and free any certs
already pushed and free the stack, then return/report an error instead of
continuing; reference the PEM_read_bio_X509 loop, the sk_X509_push calls,
X509_up_ref usage, and the us_set_user_root_certs call when implementing the
unwind and error handling.
In `@test/js/node/tls/node-tls-set-default-ca-certificates.test.ts`:
- Around line 219-228: The failing assertion uses a different hostname for req2
which can mask hostname-mismatch or connection errors; change the https.request
options for req2 to reuse "localhost" (or include servername: "localhost") so
the same host is used, and then only treat the test as passing if the emitted
error is a TLS verification failure (e.g., check err.code/err.message for a
TLS/certificate verification indicator) rather than any connection error; update
the req2 creation and its error assertion around
tls.setDefaultCACertificates([]), referencing tls.setDefaultCACertificates,
https.request/req2, and the error handler to enforce verification-specific
failure.
---
Duplicate comments:
In `@src/jsc/bindings/NodeTLS.cpp`:
- Around line 151-156: In appendX509sFromPEM, guard the size_t→int cast before
calling BIO_new_mem_buf by checking if data.size() > INT_MAX and rejecting
oversized inputs via the same crypto failure path (i.e., return the existing
error code used on failure such as ERR_peek_last_error()) so the cast is never
performed and BIO_new_mem_buf is not given a wrapped negative length; adjust the
logic around BIO_new_mem_buf/BIO_new_mem_buf(data.data(),
static_cast<int>(data.size())) accordingly.
🪄 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: 7ce79f9f-5824-4305-98c5-b16629dfa1e9
📒 Files selected for processing (4)
packages/bun-usockets/src/crypto/root_certs.cpppackages/bun-usockets/src/crypto/root_certs_header.hsrc/jsc/bindings/NodeTLS.cpptest/js/node/tls/node-tls-set-default-ca-certificates.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/jsc/bindings/NodeTLS.cpp (1)
162-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
sk_X509_push()and unwind on failure.Line 163 and Line 265 ignore stack insertion failure. If the stack grow fails, the parse path leaks
x, and the dedupe path leaks the extraX509_up_ref()while still committing a truncated CA set. Treat a falseysk_X509_push()as fatal and clean up before returning.Suggested fix
while (X509* x = PEM_read_bio_X509(bio, nullptr, [](char*, int, int, void*) -> int { return 0; }, nullptr)) { - sk_X509_push(out, x); + if (!sk_X509_push(out, x)) { + X509_free(x); + BIO_free(bio); + while (pushed-- > 0) { + X509_free(sk_X509_pop(out)); + } + return ERR_peek_last_error(); + } pushed++; }for (int i = 0; i < (int)sk_X509_num(parsed); i++) { X509* cert = sk_X509_value(parsed, i); if (seen.insert(cert).second) { X509_up_ref(cert); - sk_X509_push(deduped, cert); + if (!sk_X509_push(deduped, cert)) { + X509_free(cert); // balance X509_up_ref + sk_X509_pop_free(deduped, X509_free); + freeParsed(); + throwOutOfMemoryError(globalObject, scope); + return {}; + } } }Also applies to: 261-266
🤖 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 `@src/jsc/bindings/NodeTLS.cpp` around lines 162 - 165, The loop that calls PEM_read_bio_X509 and then sk_X509_push does not handle a falsey sk_X509_push result, which can leak the newly read X509* (from PEM_read_bio_X509) and, in the dedupe path, leak the extra X509_up_ref() and commit a truncated CA set; modify the code in NodeTLS.cpp around the PEM_read_bio_X509 loop and the dedupe/commit path to treat sk_X509_push failure as fatal: on any sk_X509_push() == 0, immediately free the pushed X509* (and any already-pushed stack members), undo any X509_up_ref() increments, clear/ free the target STACK_OF(X509)* (out) and return an error/abort the parse rather than proceeding to dedupe/commit so no leaked X509 or inconsistent CA set remains.
🤖 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/jsc/bindings/NodeTLS.cpp`:
- Around line 168-179: The code currently treats PEM_R_NO_START_LINE as EOF
whenever a certificate was parsed (pushed>0) but does not validate trailing
bytes; update the branch that checks ERR_GET_LIB(last)==ERR_LIB_PEM &&
ERR_GET_REASON(last)==PEM_R_NO_START_LINE so that before calling
ERR_clear_error() and returning success it inspects the remaining input buffer
used by this parsing routine (the buffer pointer/remaining length variables in
this function) and verifies all remaining bytes are empty or whitespace only; if
any non-whitespace data remains, do not clear the error or return success—let
the error path run (which rolls back via the pushed/sk_X509_pop/X509_free loop)
so malformed trailing data causes failure.
---
Duplicate comments:
In `@src/jsc/bindings/NodeTLS.cpp`:
- Around line 162-165: The loop that calls PEM_read_bio_X509 and then
sk_X509_push does not handle a falsey sk_X509_push result, which can leak the
newly read X509* (from PEM_read_bio_X509) and, in the dedupe path, leak the
extra X509_up_ref() and commit a truncated CA set; modify the code in
NodeTLS.cpp around the PEM_read_bio_X509 loop and the dedupe/commit path to
treat sk_X509_push failure as fatal: on any sk_X509_push() == 0, immediately
free the pushed X509* (and any already-pushed stack members), undo any
X509_up_ref() increments, clear/ free the target STACK_OF(X509)* (out) and
return an error/abort the parse rather than proceeding to dedupe/commit so no
leaked X509 or inconsistent CA set remains.
🪄 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: 82a9b761-353f-4264-8e80-f985b0e97368
📒 Files selected for processing (1)
src/jsc/bindings/NodeTLS.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/jsc/bindings/NodeTLS.cpp (1)
168-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
sk_X509_pushfailures in both parse and dedupe paths.Line 170 and Line 272 ignore push failures. That can leak cert refs and leave partial state installed on allocation failure.
Proposed fix
- while (X509* x = PEM_read_bio_X509(bio, nullptr, [](char*, int, int, void*) -> int { return 0; }, nullptr)) { - sk_X509_push(out, x); - pushed++; + while (X509* x = PEM_read_bio_X509(bio, nullptr, [](char*, int, int, void*) -> int { return 0; }, nullptr)) { + if (!sk_X509_push(out, x)) { + X509_free(x); + OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); + break; + } + pushed++; } ... if (seen.insert(cert).second) { X509_up_ref(cert); - sk_X509_push(deduped, cert); + if (!sk_X509_push(deduped, cert)) { + X509_free(cert); // undo up_ref + sk_X509_pop_free(deduped, X509_free); + freeParsed(); + throwOutOfMemoryError(globalObject, scope); + return {}; + } }#!/bin/bash rg -nC3 'sk_X509_push\(out, x\)|X509_up_ref\(cert\)|sk_X509_push\(deduped, cert\)' src/jsc/bindings/NodeTLS.cppAlso applies to: 268-274
🤖 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 `@src/jsc/bindings/NodeTLS.cpp` around lines 168 - 173, The loops that call PEM_read_bio_X509(...) then sk_X509_push(out, x) (and the dedupe path that calls sk_X509_push(deduped, cert) and X509_up_ref(cert)) must check sk_X509_push's return value and handle failures: if sk_X509_push returns 0 (failure) immediately free the newly-read X509 (X509_free(x)) or drop the up-ref (X509_free(cert) when you failed after X509_up_ref), free the BIO (BIO_free) and clean up the entire stack being built (iterate and X509_free any already-pushed certs and sk_X509_free the stack) before signalling/returning the error to avoid leaking cert refs or leaving partial state; update the PEM_read_bio_X509 loop and the dedupe block (where X509_up_ref and sk_X509_push are used) to perform these checks and cleanup paths.test/js/node/tls/node-tls-set-default-ca-certificates.test.ts (1)
262-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert verification-specific failure, not “any error code”.
Line 262 and Line 284 currently pass for any coded error (including non-TLS-path failures), which can mask regressions in the CA override behavior being tested.
Proposed tightening
- } catch (err) { - assert(err.code && err.code !== "ERR_ASSERTION", "step1: " + err.message); + } catch (err) { + const code = String(err?.code || ""); + assert(/UNABLE_TO_|SELF_SIGNED|CERT/i.test(code), "step1: expected TLS verify failure, got " + (err?.message || err)); } ... - } catch (err) { - assert(err.code && err.code !== "ERR_ASSERTION", "step3: " + err.message); + } catch (err) { + const code = String(err?.code || ""); + assert(/UNABLE_TO_|SELF_SIGNED|CERT/i.test(code), "step3: expected TLS verify failure, got " + (err?.message || err)); }Also applies to: 283-285
🤖 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-set-default-ca-certificates.test.ts` around lines 262 - 263, The current assertions (the expression using assert(err.code && err.code !== "ERR_ASSERTION", "step1: " + err.message); and the similar checks later) accept any error with a code and thereby mask non-TLS failures; replace these with assertions that err.code is one of the expected TLS/CA-related error codes for this test (e.g., check membership in an explicit allowed set like ['ERR_SSL_HANDSHAKE_FAILURE', 'ERR_OSSL_*', 'ERR_TLS_CERT_ALTNAME_INVALID'] or the concrete codes your runtime emits) and include the original err.message in the failure text; update both occurrences (the assert expression and the later similar block) to perform this exact-code check rather than just rejecting ERR_ASSERTION.
🤖 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 565-570: The code uses the implementation-defined sentinel (void
*)1 when storing ex_data on SSL_CTX; replace this with the address of a static
marker variable to be portable: add a static variable (e.g., a file-static char
or int) and pass its address into SSL_CTX_set_ex_data instead of (void *)1,
updating any checks that compare the stored pointer (related to
us_ctx_default_ca_ex_idx and ssl_context) to compare against that marker's
address.
In `@packages/bun-usockets/src/crypto/root_certs.cpp`:
- Around line 205-211: Replace the racy global bool has_user_root_certs with a
std::atomic<bool> and use relaxed atomics for the cheap racy read/write
semantics: update the declaration of has_user_root_certs to std::atomic<bool>,
include <atomic>, change the read in us_has_user_root_certs to
has_user_root_certs.load(std::memory_order_relaxed) and change any writes (the
assignment currently guarded by the mutex) to has_user_root_certs.store(...,
std::memory_order_relaxed) so the behavior is unchanged but well-defined.
---
Duplicate comments:
In `@src/jsc/bindings/NodeTLS.cpp`:
- Around line 168-173: The loops that call PEM_read_bio_X509(...) then
sk_X509_push(out, x) (and the dedupe path that calls sk_X509_push(deduped, cert)
and X509_up_ref(cert)) must check sk_X509_push's return value and handle
failures: if sk_X509_push returns 0 (failure) immediately free the newly-read
X509 (X509_free(x)) or drop the up-ref (X509_free(cert) when you failed after
X509_up_ref), free the BIO (BIO_free) and clean up the entire stack being built
(iterate and X509_free any already-pushed certs and sk_X509_free the stack)
before signalling/returning the error to avoid leaking cert refs or leaving
partial state; update the PEM_read_bio_X509 loop and the dedupe block (where
X509_up_ref and sk_X509_push are used) to perform these checks and cleanup
paths.
In `@test/js/node/tls/node-tls-set-default-ca-certificates.test.ts`:
- Around line 262-263: The current assertions (the expression using
assert(err.code && err.code !== "ERR_ASSERTION", "step1: " + err.message); and
the similar checks later) accept any error with a code and thereby mask non-TLS
failures; replace these with assertions that err.code is one of the expected
TLS/CA-related error codes for this test (e.g., check membership in an explicit
allowed set like ['ERR_SSL_HANDSHAKE_FAILURE', 'ERR_OSSL_*',
'ERR_TLS_CERT_ALTNAME_INVALID'] or the concrete codes your runtime emits) and
include the original err.message in the failure text; update both occurrences
(the assert expression and the later similar block) to perform this exact-code
check rather than just rejecting ERR_ASSERTION.
🪄 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: 6c01aa59-d7dd-4f76-a20e-b60ffb5b5d5a
📒 Files selected for processing (8)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/crypto/root_certs.cpppackages/bun-usockets/src/crypto/root_certs_header.hsrc/js/node/tls.tssrc/jsc/bindings/NodeTLS.cpptest/js/node/test/parallel/test-tls-set-default-ca-certificates-array-buffer.jstest/js/node/test/parallel/test-tls-set-default-ca-certificates-basic.jstest/js/node/tls/node-tls-set-default-ca-certificates.test.ts
There was a problem hiding this comment.
I didn't find any remaining issues, but this replaces the process-wide TLS trust root set and reworks the shared X509_STORE lifecycle/locking in native code — definitely one for a human reviewer.
Extended reasoning...
Overview
Implements tls.setDefaultCACertificates() across four layers: a mutex-guarded process-global STACK_OF(X509)* override and invalidatable cached X509_STORE in root_certs.cpp; an SSL_CTX ex_data marker plus per-SSL verify-store refresh in openssl.c so cached HTTPS contexts pick up runtime changes; JSC host functions in NodeTLS.cpp for PEM parsing/dedup and snapshot read-back; and the JS wrapper in tls.ts. ~600 lines plus tests.
Security risks
This is the TLS trust-root surface. The override is intentionally process-global (diverging from Node's per-thread semantics), so one Worker can change verification for all. The PR went through several rounds fixing a UAF in the snapshot path, a data race on has_user_root_certs, an X509 refcount leak, and sk_X509_push allocation-failure handling — all now addressed. The remaining design (mutex + atomic flag + SSL_set0_verify_cert_store per-SSL) looks sound to me, but the blast radius of a mistake here is silent mis-verification of TLS peers.
Level of scrutiny
High. Native crypto code with manual BoringSSL refcounting, cross-thread shared mutable state, and a behavioural change to how every default-CA TLS connection builds its trust store (the shared store moved from std::call_once to a mutex-guarded rebuildable cache). The us_internal_ssl_attach change also runs on the hot per-connection path.
Other factors
All prior bot feedback (mine and CodeRabbit's) is resolved. Test coverage is solid — input validation, dedup, rollback-on-error, Worker visibility, and an end-to-end fail→install→succeed→clear→fail HTTPS sequence, plus two upstream Node parallel tests. No outstanding human review comments. Still, the combination of security-critical + concurrency + manual memory management + intentional Node.js divergence is exactly the profile that warrants human sign-off.
StatusImplementation complete at e2136fa. All review threads resolved; claude[bot]'s final review found no remaining issues. CI282/286 jobs pass. The 4 failures are all on tests unrelated to this diff (which only touches
The HTTP-abort test timed out 4/4 attempts on 3 Windows lanes in two consecutive builds (53807 and 53813), so re-rolling is unlikely to help. The new tests added by this PR ( Ready for maintainer review / merge. |
e2136fa to
32d9894
Compare
There was a problem hiding this comment.
No issues found in this pass, but this touches the process-wide TLS trust-root path (mutex-guarded shared X509_STORE, per-SSL verify-store override in us_internal_ssl_attach, intentional process-global vs Node's thread-local semantics) — worth a human sign-off before merge.
Extended reasoning...
Overview
This PR implements tls.setDefaultCACertificates() (Node.js v24.5.0). It spans:
packages/bun-usockets/src/crypto/root_certs.cpp— replaces thestd::call_once-cached shared defaultX509_STOREwith a mutex-guarded, invalidatable cache; adds process-globaluser_root_certsoverride withstd::atomic<bool>fast-path flag and a snapshot-under-lock duplicator.packages/bun-usockets/src/crypto/openssl.c— newSSL_CTXex_data marker sous_internal_ssl_attach()can swap the verify store per-SSLwhen the default set has changed under a long-lived cached CTX (fetch/https).src/jsc/bindings/NodeTLS.cpp— ~210 new lines: PEM parsing with rollback,X509_cmp-based dedup viastd::set, manualsk_X509_*refcounting, two new host functions.src/js/node/tls.ts— JS wrapper, input validation, per-VM cache invalidation that defers to native for cross-Worker visibility.- New Bun test, two upstream Node parallel tests, and
common/tls.jshelpers.
Security risks
This is squarely security-sensitive: it rewires how the default TLS trust root set is selected for every client connection that doesn't pass an explicit ca. Specific concerns a human should weigh:
- The shared default store moves from immutable-once to mutex-guarded mutable. Every TLS client attach now takes the fast-path branch in
us_internal_ssl_attach; correctness depends on the ex_data marker only being set when no usercawas supplied. - Manual BoringSSL refcounting (
X509_up_ref/sk_X509_pop_free/X509_STORE_add_cert) across two ownership domains; the review cycle already caught one UAF and one leak here. - Intentional divergence from Node.js: override is process-global, not per-Worker (
thread_local). The rationale (SSL attach runs off the JS thread) is documented and was discussed in-thread, but it's a design decision that affects multi-Worker isolation and should be ratified by a maintainer.
Level of scrutiny
High. This is production-critical crypto/TLS plumbing in C/C++ with concurrency, not a config tweak. The change also affects the hot path for all default-CA TLS connections (the shared-store cache mechanism), not just callers of the new API.
Other factors
- The PR went through ~7 fix commits during review (UAF snapshot, per-VM cache desync,
size_t→intcast,sk_X509_pushreturn checks,std::atomic<bool>, static-address marker, redundantX509_up_refleak, test hostname). All raised threads are resolved and the current diff reflects the fixes. - Test coverage is good: input validation, round-trip, dedup, ArrayBufferView variants, invalid-PEM rollback, Worker visibility, and an end-to-end fail→install→succeed→clear→fail HTTPS sequence; plus two unmodified upstream Node tests.
- CI: 282/286 passing; the 4 failures are documented as unrelated flakes.
- No CODEOWNERS check performed here, but TLS/crypto code in this repo typically has designated owners.
CI statusThe diff is green on the TLS lanes: Build 63126 (cb396e9) failed on lanes unrelated to this diff:
None touch Review statusAll CodeRabbit and claude[bot] review threads resolved. The final automated review (on cb396e9) found no new issues and flagged the change as security-sensitive native TLS code that merits a human pass — which is expected for a trust-store change. |
tls.setDefaultCACertificates(certs) replaces the default CA trust store
used for TLS client verification when no explicit 'ca' option is given.
After calling it, tls.getCACertificates('default') returns exactly the
supplied (deduplicated) certificates, and new TLS/HTTPS connections
verify against that set instead of the bundled Mozilla roots.
- root_certs.cpp: mutex-guarded process-global user cert override that
us_get_default_ca_store() honours exclusively when present; the cached
shared store is invalidated on each setDefaultCACertificates() call.
has_user_root_certs is atomic for the lock-free fast-path check.
- openssl.c: SSL_CTXs built against the default CA set (request_cert,
no explicit ca) are tagged via ex_data so us_internal_ssl_attach()
refreshes the per-SSL verify store after the defaults change — the
cached fetch/https CTX picks up the override without a rebuild.
- NodeTLS.cpp: resetRootCertStore parses PEM strings/ArrayBufferViews
(multi-cert bundles supported), deduplicates by X509 identity, and
installs the set; getUserRootCertificates returns an up-ref'd PEM
snapshot, or undefined when no override is installed.
- tls.ts: validates input (ERR_INVALID_ARG_TYPE), queries the native
override state so Workers always report the store their connections
actually verify against.
Fixes #24340
Fixes #13868
b54356c to
14bac29
Compare
…identity setDefaultCACertificates() invalidates and rebuilds the cached shared X509_STORE, so the old pointer-identity check in us_ssl_ctx_add_ca_cert (store == us_get_shared_default_ca_store()) no longer recognises a CTX holding the stale shared store, and would mutate it in place, leaking the added CA into every sibling CTX still referencing it. Gate the clone on !us_ctx_user_ca_ex_idx instead: that flag is set exactly when the CTX has a private store, so its absence means the store is shared-or-empty and must be cloned before appending. Also mirror the client-side per-SSL verify-store refresh on the server accept path so an mTLS server built before setDefaultCACertificates() verifies client certificates against the current defaults.
There was a problem hiding this comment.
I didn't find any new issues, but this rewrites the TLS default trust-store as a process-global, mutex-invalidated cache with per-SSL verify-store refresh on both client and server attach paths — security-critical native code that's worth a human pass, especially the intentional process-global (vs Node's per-thread) override semantics and the cb396e9 addCACert clone-on-write change.
Extended reasoning...
Overview
This PR replaces the JS-only tls.setDefaultCACertificates() stub with a native trust-store override. It touches:
packages/bun-usockets/src/crypto/root_certs.cpp— moves the cached sharedX509_STOREfromstd::call_onceto a mutex-invalidated cache; adds process-globaluser_root_certsguarded byshared_store_mutexand a relaxed-atomichas_user_root_certsflag.packages/bun-usockets/src/crypto/openssl.c— adds per-SSL verify-store refresh on both the client connect and server accept paths inus_internal_ssl_attach, and rewritesus_ssl_ctx_add_ca_cert's clone-on-write to gate on theus_ctx_user_ca_ex_idxflag instead of pointer identity against the (now mutable) shared store.src/jsc/bindings/NodeTLS.cpp— ~210 lines of new JSC host functions: PEM bundle parsing, X509 dedup viastd::set<X509*, X509DerLess>, snapshot-under-lock forgetUserRootCertificates.src/js/node/tls.ts— removes the_defaultCACertificatesOverrideJS hook from #31155 and routes through native;getCACertificates('default')now queries native on every call.- ~480 lines of new tests across two files plus an updated
ssl-ctx-cache.test.tsthat flips theca: []semantics.
Security risks
This is squarely security-sensitive: it governs which CAs every default TLS connection in the process trusts. Specific areas:
- Trust-store isolation: the cb396e9 fix to
us_ssl_ctx_add_ca_certchanges how clone-on-write detects a shared store. The new gate (!us_ctx_user_ca_ex_idx) is correct for stores attached by Bun's own paths, but assumes every code path that hands a shared/aliased store to an SSL_CTX also leaves that flag clear. - Server accept path now overrides the verify store per-SSL when
us_has_user_root_certs()is true — newly-added behavior that affects mTLS client-cert verification. - Process-global override: one Worker calling
setDefaultCACertificates([])empties the trust store for the entire process. This is an intentional, documented divergence from Node's per-thread semantics, but it's a design call a maintainer should sign off on. - The review history surfaced and fixed a UAF (unlocked snapshot iteration), a per-VM/process-global desync, an
X509_up_refleak, abooldata race, and a clone-on-write regression — all addressed, but the density of memory-safety/concurrency fixes underscores the complexity.
Level of scrutiny
High. This is native C/C++ in the TLS verification path, with OpenSSL refcounting, cross-thread mutex protocols, and a behavioral change to an existing function (us_ssl_ctx_add_ca_cert). It also flips a test assertion in ssl-ctx-cache.test.ts (ca: [] now authorizes where it previously didn't), which is a user-visible behavior change explained in the PR body as a known follow-up.
Other factors
- All prior automated review threads (CodeRabbit + my own) are resolved with code changes and explanations.
- CI is green on the new TLS tests across all lanes including ASAN; the 2 remaining failures are unrelated flakes (
terminal.test.ts,next-pages/dev-server.test.ts). - All seven upstream Node
test-tls-set-default-ca-certificates-*.jstests reportedly pass unmodified. - No CODEOWNERS check performed, but
packages/bun-usockets/src/crypto/is core networking infrastructure.
This is well-tested and the iteration history is thorough, but it's not a change I can approve without a human looking at the trust-store semantics and the openssl.c attach-path changes.
1 participant
What does this PR do?
Implements
tls.setDefaultCACertificates(certs)(added in Node.js v24.5.0) as a native trust-store override, replacing the JS-only stub from #31155.Fixes #24340
Fixes #13868
Repro
Before:
TypeError: tls.setDefaultCACertificates is not a functionAfter: replaces the default trust store with the system CA set.
How
packages/bun-usockets/src/crypto/root_certs.cppadds a mutex-guarded user-overrideSTACK_OF(X509)*. When present,us_get_default_ca_store()builds the store exclusively from it (Node.js does not merge bundled/system/extra back in). The cached shared store moves fromstd::call_onceto a mutex sosetDefaultCACertificatescan invalidate it for subsequent connections; in-flight SSL*s keep their ownX509_STOREreference viaSSL_set0_verify_cert_store.packages/bun-usockets/src/crypto/openssl.c:https.request()/fetch()build their HTTPSSSL_CTXonce on the HTTP thread and reuse it for the process lifetime, so a JS-side injection atcreateSecureContexttime can't reach them.us_internal_ssl_attach()now checksus_has_user_root_certs()and, for any CTX not marked withus_ctx_user_ca_ex_idx(i.e. no explicitca), overrides the verify store per-SSL with the current shared default. CTXs built with an explicitcaare left untouched.src/jsc/bindings/NodeTLS.cpp:resetRootCertStoreaccepts an array of PEM strings /ArrayBufferViews (each entry may contain multiple certs), deduplicates byX509_cmpidentity, and installs the result.getUserRootCertificatesreturnsundefinedwhen no override is installed (so the JS side falls back to bundled/system/extra) or a frozen PEM array otherwise; the snapshot is taken under the root-cert mutex so a concurrent Worker swapping certs can't free them mid-serialise.src/js/node/tls.ts:setDefaultCACertificates(certs)validatescertsis anArrayofstring | ArrayBufferView(ERR_INVALID_ARG_TYPEwith Node-compatible wording), calls into native, and invalidates the JS-side cache.getCACertificates('default')queries native for the override on every call so a Worker sees an override installed on another thread. Removes the JS-side_defaultCACertificatesOverridehook from net,tls: port Node.js net/tls compatibility tests and fix the gaps they surface — half-open/reset/write semantics, server TLSSocket wrap, session/keylog, SNICallback/ALPNCallback, pfx, OpenSSL error shapes, addCACert, local binding (+305 tests) #31155 (the native store now handles every path, includingfetch()and long-lived SSL_CTXs that the JS hook could not reach).Node.js parity notes
ERR_CRYPTO_OPERATION_FAILEDand leaves the current defaults untouched.getCACertificates('bundled')/'system'/'extra'remain unchanged.thread_local); this implementation is process-global, guarded by a mutex so Workers don't race each other.getCACertificates('default')reflects the process-global override from any Worker.Tests
test/js/node/tls/node-tls-set-default-ca-certificates.test.tscovers: existence, input validation (non-array / bad element types), round-tripping throughgetCACertificates('default'), deduplication,Buffer/Uint8Array/DataViewinputs, invalid-PEM rollback, Worker visibility, and an end-to-end HTTPS sequence of fail (no CA) → install CA → succeed → clear CA → fail that proves the cached HTTP-thread SSL_CTX picks up runtime changes.All seven upstream
test/js/node/test/parallel/test-tls-set-default-ca-certificates-*.jsand all tentest-tls-get-ca-certificates-*.jspass unmodified.test/js/node/tls/ssl-ctx-cache.test.ts's "ca: [] skips the override" case is updated:ca: []collapses to no-CA at the nativeSSLConfigboundary ([] → None), so it now behaves like omittingcaand the process default applies. The test now asserts that, plus a new case forca: [nonMatchingCA]to prove an explicitcastill wins over the default. Node's full "ca: [] = empty trust store" semantics remain a follow-up (same as before this PR; the previous behaviour was also not Node-correct, per the test's own comment).This also provides the "runtime hook to add a CA cert" that #13868 asks for: in a standalone executable you can embed a PEM string and call
tls.setDefaultCACertificates([...tls.getCACertificates('bundled'), myCA])at startup.Rebase note (squashed onto main at 0c537fe): #31155 landed a JS-only
setDefaultCACertificatesthat injected_defaultCACertificatesOverrideas thecaoption insideInternalSecureContext, plus its ownus_ctx_user_ca_ex_idxSSL_CTX marker. The conflict resolution:openssl.c: dropped this PR's separateus_ctx_default_ca_ex_idxmarker in favour of the inverse check!us_ctx_user_ca_ex_idxthat main already defines; the per-SSL refresh inus_internal_ssl_attachnow gates on that.src/js/node/tls.ts: removed the_defaultCACertificatesOverrideJS hook and its injection sites inInternalSecureContext/setSecureContextsince the native store override covers every construction path (including the cached HTTPSSSL_CTXandfetch(), which the JS hook could not reach).test/js/node/test/common/tls.js: keptassertEqualCertscomparing viaJSON.stringifyd metadata (aSetof raw objects compares by reference).