Repository navigation
Conversation
…ains When several certificates match an issuer lookup (same subject and key, different validity windows), BoringSSL's legacy X.509 path builder commits to whichever candidate it finds first and fails the chain with CERT_HAS_EXPIRED if that copy is expired, even though a chain through the time-valid copy was buildable. OpenSSL has preferred time-valid candidates in both lookups since 1.0.2, so Node accepts these chains and Bun did not. Two lookups are affected: - find_issuer() over the peer-supplied chain: servers send both a current and a deprecated intermediate during CA transitions (RFC 8446, section 4.4.2 says to tolerate this), and the expired one can come first. - X509_STORE_CTX_get1_issuer() over the trust store: an expired CA next to its reissued replacement in the ca option, NODE_EXTRA_CA_CERTS, or the system store poisons the chain depending on store sort order, including for servers that present a perfectly valid chain. Patch the vendored BoringSSL (patches/boringssl/prefer-time-valid-issuer.patch) so both lookups prefer a time-valid candidate and fall back to an expired one only when nothing valid matches. Chains whose only path goes through an expired certificate still fail with CERT_HAS_EXPIRED.
|
Updated 3:36 PM PT - Jun 29th, 2026
❌ @robobun, your commit 329b083 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33094That installs a local version of the PR into your bun-33094 --bun |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
The three child processes are independent; sequentially they exceed the 5 second test budget under a debug build.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a BoringSSL patch that prefers time-valid issuer candidates during chain building, wires it into the build, expands TLS/fetch coverage for duplicate intermediates, and updates two documentation pages’ formatting. ChangesPrefer time-valid issuer patch and TLS tests
Documentation formatting updates
Possibly related PRs
Suggested reviewers
🚥 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/boringssl.ts`:
- Around line 39-48: The patch application flow in applyPatch() currently treats
a zero git apply exit as success even when hunks are skipped, which lets
fetchDep() continue and write the .ref stamp against unpatched BoringSSL. Update
scripts/build/fetch-cli.ts so applyPatch() runs git apply with LC_ALL=C -v,
detects any “Skipped patch” output as a failure, and propagates that failure so
the fetch is aborted before writing the stamp.
🪄 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: 83371084-177e-4f9a-bf12-d0b5b74e03ef
⛔ Files ignored due to path filters (5)
test/js/node/tls/fixtures/altchain-int-expired-cert.pemis excluded by!**/*.pemtest/js/node/tls/fixtures/altchain-int-valid-cert.pemis excluded by!**/*.pemtest/js/node/tls/fixtures/altchain-leaf-cert.pemis excluded by!**/*.pemtest/js/node/tls/fixtures/altchain-leaf-key.pemis excluded by!**/*.pemtest/js/node/tls/fixtures/altchain-root-cert.pemis excluded by!**/*.pem
📒 Files selected for processing (5)
patches/boringssl/prefer-time-valid-issuer.patchscripts/build/deps/boringssl.tstest/js/node/tls/fetch-tls-cert.test.tstest/js/node/tls/fixtures/altchain-gen.shtest/js/node/tls/node-tls-cert.test.ts
git apply can exit zero while skipping hunks; catching the LC_ALL=C "Skipped patch" report keeps the .ref stamp from being written against unpatched sources.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this patches BoringSSL's X.509 chain-building logic (TLS certificate verification), which is security-critical enough that a human should review the patch and weigh in on the patch-file vs. fork question raised in the description.
Extended reasoning...
Overview
The core change is a new patches/boringssl/prefer-time-valid-issuer.patch that modifies find_issuer() in crypto/x509/x509_vfy.cc and X509_STORE_CTX_get1_issuer() in crypto/x509/x509_lu.cc to prefer time-valid issuer candidates over expired ones, mirroring OpenSSL behavior. The patch is wired into the BoringSSL dependency build via scripts/build/deps/boringssl.ts, and scripts/build/fetch-cli.ts was hardened (per CodeRabbit feedback, addressed in 9f803c6) to fail the build if git apply skips hunks. New altchain fixtures and 8 new tests in node-tls-cert.test.ts / fetch-tls-cert.test.ts cover both the peer-chain and trust-store cases plus the negative case. The two docs changes are pure autofix reformatting.
Security risks
This directly alters TLS certificate path building in the vendored crypto library. The change makes verification more permissive in the specific case where both an expired and a time-valid copy of the same issuer (same subject/key) are available — it does not accept any chain that lacks a fully valid path, and the negative test confirms CERT_HAS_EXPIRED is still raised when only an expired path exists. The approach matches OpenSSL's long-standing behavior, so the security posture should converge with Node's. That said, the x509_lu.cc hunk restructures a mutex-protected loop and adds a fallback that holds an X509_OBJECT* across the iteration before up-reffing it; correctness here depends on the store lock semantics, and any mistake in this area would affect every TLS connection in Bun.
Level of scrutiny
High. Patching a vendored BoringSSL verifier is exactly the kind of change that warrants a human reviewer with crypto/TLS context, both for the C++ itself (refcounting, locking, the bssl:: namespace definition placement) and for the maintenance question the author explicitly raised: whether this should live as a patch file or be committed to the oven-sh/boringssl fork.
Other factors
The PR is well-motivated, thoroughly documented, and comes with comprehensive regression tests plus a fixture-regeneration script. The bug-hunting system found no issues, and CodeRabbit's one suggestion has been addressed. The robobun CI comment shows widespread build-rust/build-cpp failures on the earlier commit cfa9b93 — worth confirming whether those are infrastructure-related or caused by this change before merging.
There was a problem hiding this comment.
I didn't find any bugs, but this patches BoringSSL's X.509 chain-building/verification logic directly — security-critical code that warrants a human look (and a call on patch-file vs. landing in the oven-sh/boringssl fork).
Extended reasoning...
Overview
This PR adds a vendored patch to BoringSSL that changes how find_issuer() (in x509_vfy.cc) and X509_STORE_CTX_get1_issuer() (in x509_lu.cc) select among multiple issuer candidates with the same subject/key: instead of returning the first match, both now prefer a candidate whose notBefore/notAfter window covers the verification time, falling back to the first (possibly expired) match only when no time-valid candidate exists. The patch is wired into the build via scripts/build/deps/boringssl.ts, scripts/build/fetch-cli.ts is hardened to fail on git apply skipping hunks (per CodeRabbit feedback, addressed in 9f803c6), and new fixtures + tests cover both the peer-chain and trust-store cases for node:tls and fetch. The two docs changes are autofix.ci whitespace/table reflow.
Security risks
This is a direct modification to TLS certificate chain verification — the most security-sensitive code path in the runtime. The change is relaxing (accepts chains previously rejected with CERT_HAS_EXPIRED), which is the right direction only if it never accepts a chain that lacks a genuinely valid path. The design is sound on its face — it mirrors OpenSSL's long-standing behavior, the new helper x509_check_cert_time_valid is side-effect-free and honors X509_V_FLAG_NO_CHECK_TIME/X509_V_FLAG_USE_CHECK_TIME, the fallback preserves the original error reporting when no valid candidate exists, and ref-counting in the x509_lu.cc rewrite looks correct (the early-return path keeps obj's ref, the loop and fallback paths both X509_OBJECT_up_ref_count). But subtle mistakes here can be catastrophic, and I'm not the right reviewer to sign off on a crypto-library patch.
Level of scrutiny
High. Patching vendored BoringSSL X.509 verification is exactly the kind of change that should get eyes from someone comfortable with the OpenSSL/BoringSSL verifier internals. The author also explicitly flags a maintainer decision: whether to carry this as a patches/ file or land it in the oven-sh/boringssl fork instead.
Other factors
The PR is well-motivated and well-tested: the description includes a clear repro matrix vs. Node, the new tests cover both orderings in both the peer chain and the trust store plus a negative case confirming expired-only chains still fail, and the fixture generator script is checked in. CodeRabbit's only finding (silent patch skip) has been addressed. No prior comments from me on this PR.
|
CI status across both runs of this PR (builds 66904 and 66933):
CI was re-run once already; the alpine failure reproduces identically on both runs, on both musl lanes, and predates any interaction with this diff. Ready for review as is. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-29, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What does this PR do?
TLS verification in Bun rejects connections with
CERT_HAS_EXPIREDwhen an expired copy of an issuer certificate is present anywhere in the candidate pool, even though a time-valid copy that completes a valid chain is right next to it. Node (OpenSSL) accepts these chains. The behavior is identical acrossfetch,node:tls, andnode:https, and it is order dependent, so it presents as "works on one machine, fails on another".Two lookups in BoringSSL's legacy X.509 verifier return the first subject/AKID match and never consider validity dates:
find_issuer()incrypto/x509/x509_vfy.cc, over the peer-supplied chain. Servers send both a current and a deprecated intermediate during CA transitions, and RFC 8446 (section 4.4.2) tells clients to tolerate extraneous certificates and arbitrary orderings. If the expired copy comes first in the Certificate message, the chain gets built through it and verification fails.X509_STORE_CTX_get1_issuer()incrypto/x509/x509_lu.cc, over the trust store. An expired CA sitting next to its reissued replacement (same subject, same key) in thecaoption,NODE_EXTRA_CA_CERTS, or the system store wins or loses the lookup depending on the store's internal sort order. Because trusted-first lookup consults the store before the peer chain, this also rejects servers that present a perfectly valid chain.OpenSSL has preferred a time-valid candidate in both of these lookups since 1.0.2, which is exactly the compatibility gap. BoringSSL's own test suite describes its
TRUSTED_FIRST/alt-chains handling as "workarounds for not having an actual path-building verifier" (crypto/x509/x509_test.cc), and Chrome does not rely on this code path at all (it verifies with the path builder in BoringSSL'spki/), so the gap never surfaces in Google's products.Reproduction
Create a root, one intermediate key with two certificates (same subject and key, one valid, one expired), and a
localhostleaf signed by that key. Serve withnode:httpsand connect with each runtime.Results are identical for bun 1.2.18, 1.3.14, and current main, via
fetch(url, { tls: { ca } }),tls.connect({ ca }), andhttps.get({ ca }).The fix
patches/boringssl/prefer-time-valid-issuer.patch, applied to the vendored BoringSSL at fetch time like the otherpatches/*entries: both lookups now prefer a candidate that is valid at the verification time and only fall back to the first (expired) match when no time-valid candidate exists. Chains whose only path runs through an expired certificate still fail withCERT_HAS_EXPIRED. This mirrors OpenSSL'sfind_issuerandX509_STORE_CTX_get1_issuer.If it is preferable to carry this in the
oven-sh/boringsslfork rather than as a patch file, the same two hunks apply there. The remaining gap that this PR does not close (neither does Node): when the only trusted candidate is expired and the peer supplied a valid intermediate, a real path builder (BoringSSLpki/) is needed to recover.Tests
test/js/node/tls/fixtures/altchain-*: root, the two same-key intermediates (valid to 2053, expired in 2024), alocalhostleaf, andaltchain-gen.shto regenerate them. New tests intest/js/node/tls/node-tls-cert.test.ts(node:tls) andtest/js/node/tls/fetch-tls-cert.test.ts(fetch):CERT_HAS_EXPIREDWithout the patch, 5 of the 8 new tests fail with
CERT_HAS_EXPIRED; with it, all pass. The rest oftest/js/node/tls/plustest/js/web/fetch/fetch.tls*.test.tsis unchanged: the only other failures on this machine (fetch timeout works on tls,Bun.connect TLS hostname verification > reports authorized=false ...) reproduce identically on the unmodified release binary, and two more are 5 second budget subprocess tests that only time out under a debug build on a slow container.Note for reviewers: the runtime change lives in
patches/+scripts/build/deps/boringssl.ts, notsrc/, so the unfixed baseline for the new tests isUSE_SYSTEM_BUN=1 bun test test/js/node/tls/node-tls-cert.test.ts(or a build with thepatches:line removed).