Repository navigation
tls: apply CA name constraints to the subject CN of certificates without a DNS subjectAltName - #37758
Jarred-Sumner wants to merge 2 commits into
Conversation
…out a DNS subjectAltName Bun (like Node.js) matches the host name against the subject common name whenever a certificate has no DNS subjectAltName. Chain verification now holds host-name-like common names of such certificates to the DNS name constraints of every issuing CA, as OpenSSL's NAME_CONSTRAINTS_check_CN does, so the certificates whose common name is constrained are the same ones whose common name is used for matching. This lives in a BoringSSL patch (patches/boringssl/name-constraints-common-name.patch) so every X509_verify_cert caller reports it through the normal verify result. Wildcard common names are constrained too; SAN-less certificates with an in-subtree common name now verify as they do in Node.js.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes 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 ignored due to path filters (8)
📒 Files selected for processing (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it patches BoringSSL's X.509 chain-verification path (check_name_constraints in x509_vfy.cc) and changes which certificates are accepted under a name-constrained CA, a human review of the security semantics is warranted.
Checked: the range-for over x_impl->altname when the leaf has no SAN extension (nullptr stack) — BoringSSL's STACK_OF iterator treats that as empty, and the nosan fixture exercises it; widen_partial_wildcard_label writes in place into the ASN1_STRING_to_UTF8 buffer, which is heap-owned by free_idval and only narrows the view; the i == 0 guard restricts the CN check to the leaf, matching OpenSSL's NAME_CONSTRAINTS_check_CN placement.
Extended reasoning...
Overview
This PR adds a source patch to the vendored BoringSSL (patches/boringssl/name-constraints-common-name.patch, wired into scripts/build/deps/boringssl.ts) that replaces upstream's reject_dns_name_in_common_name handling in check_name_constraints() with an OpenSSL-style NAME_CONSTRAINTS_check_CN equivalent (x509_check_common_name_constraints). It also adds eight PEM fixtures and new test cases in node-tls-connect-hostname-verification.test.ts and fetch.tls.test.ts covering inside/outside/wildcard/partial-wildcard/no-SAN leaves under a DNS-name-constrained intermediate.
Security risks
This is squarely security-sensitive: it modifies certificate chain verification inside a crypto library, and it loosens one behavior relative to upstream BoringSSL (SAN-less leaves with a DNS-like CN inside the permitted subtree now pass verification where upstream rejected them outright with X509_V_ERR_NAME_CONSTRAINTS_WITHOUT_SANS) while tightening another (leaves with a non-DNS SAN and an out-of-subtree CN, including wildcard/partial-wildcard CNs, now fail). The reasoning is well-argued and aligns with Node.js/OpenSSL, but any change to which certificates X509_verify_cert accepts affects every TLS client and server path in Bun and deserves a maintainer's eyes on the fail-open/fail-closed boundaries — particularly the single-label-CN carve-out next to a SAN extension and the deliberate divergence from OpenSSL on wildcard CNs.
Level of scrutiny
High. The repo review guidelines call out TLS/crypto paths and vendored-dependency patches as areas where flags and checks should not be altered without explicit understanding. Patching x509_vfy.cc is exactly the kind of change a human should sign off on regardless of how careful the implementation looks.
Other factors
Test coverage is solid (both accept and reject cases across tls.connect and fetch, including rejectUnauthorized: false reporting), and the PR description documents a 42-case comparison against Node v26 and openssl verify. I found no memory-safety issues in the new C++: the UTF-8 buffer is RAII-owned by UniquePtr, embedded-NUL handling matches OpenSSL's cn2dnsid, and the stack-allocated ASN1_IA5STRING/GENERAL_NAME passed to nc_match mirrors how NAME_CONSTRAINTS_check builds its temporary in the same file. Still deferring given the subject matter.
|
Updated 6:05 AM PT - Aug 12th, 2026
❌ @Jarred-Sumner, your commit 867c9e5 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37758That installs a local version of the PR into your bun-37758 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it patches BoringSSL's X.509 chain-verification path (check_name_constraints) and changes when certificates are accepted or rejected under a name-constrained CA, a human look is still warranted.
Checked: the range-for over x_impl->altname is safe when the leaf has no SAN extension (BoringSSL's stack iterators handle nullptr, and the nosan fixture exercises it); widen_partial_wildcard_label writes only into the ASN1_STRING_to_UTF8 buffer owned by free_idval; the i == 0 guard restricts the new CN check to the leaf, matching OpenSSL's NAME_CONSTRAINTS_check_CN placement.
Extended reasoning...
Overview
This PR adds a BoringSSL patch (patches/boringssl/name-constraints-common-name.patch) that rewrites part of check_name_constraints() in crypto/x509/x509_vfy.cc and adds x509_check_common_name_constraints() to v3_ncons.cc. It replaces upstream BoringSSL's blanket rejection of DNS-like common names on SAN-less leaves under a constrained CA with OpenSSL-style behaviour: when the leaf has no dNSName SAN, each host-name-like CN is matched against the issuer's DNS name constraints via nc_match. The patch is wired into the build via scripts/build/deps/boringssl.ts, and eight new PEM fixtures plus tests in node-tls-connect-hostname-verification.test.ts and fetch.tls.test.ts cover accept/reject cases for in-subtree, out-of-subtree, wildcard, partial-wildcard and no-SAN CNs.
Security risks
This is directly security-sensitive: it modifies the certificate chain verification path that every TLS client and server in Bun (fetch, node:tls, Bun.connect, WebSocket, SQL/Valkey clients) relies on. The change both loosens verification in one direction (SAN-less leaves with an in-subtree CN are now accepted where upstream BoringSSL rejected them) and tightens it in another (leaves with a non-DNS SAN whose CN falls outside the permitted subtree are now rejected). The heuristics for which CNs count as DNS names (x509v3_looks_like_dns_name + partial-wildcard widening + single-label exemption next to a SAN) determine whether a name-constraint bypass is possible, so any gap between what host-name matching honours and what this function constrains would be a security hole. The PR description documents a 42-case comparison against Node.js/OpenSSL and the residual differences are all strict-side (Bun rejects, Node accepts), which is the safe direction — but that analysis deserves human confirmation.
Level of scrutiny
High. Per the repo's own review rules, TLS/crypto verification paths are called out explicitly ("Never remove a flag you don't understand in a TLS/crypto path", "Security checks fail closed and cover every path to the protected effect"). Patching a vendored crypto library's X509_verify_cert internals is not a mechanical change; the interaction between X509_check_host's CN fallback, Node-style checkServerIdentity, and this new constraint check is subtle and the PR itself enumerates seven cases where behaviour intentionally diverges from Node.js.
Other factors
The PR is very well-documented and thoroughly tested (both tls.connect and fetch paths, both accept and reject cases, both rejectUnauthorized values). The bug-hunting pass found nothing. I spot-checked memory handling (UniquePtr frees the UTF-8 buffer on every path; the in-place '*' write stays inside that buffer), null-SAN iteration, and that the new check is scoped to the leaf (i == 0). Nothing looks wrong to me — but given this is chain verification for every TLS connection in the runtime, this is exactly the kind of change that should not land on automated review alone.
This branch has not been deployed
2 participants
What does this PR do?
Aligns certificate chain verification with how Bun (and Node.js) match host names, for CAs that carry an X.509
nameConstraintsextension.Bun, like Node.js, matches the host name against the subject common name whenever a certificate has no DNS
subjectAltNameentry. Node.js pairs that rule with OpenSSL'sNAME_CONSTRAINTS_check_CN: during chain verification, a host-name-like common name of such a leaf must satisfy the DNS name constraints of every issuing CA. Upstream BoringSSL keys its common-name handling to its ownX509_check_hostinstead (which only consults the common name when there is nosubjectAltNameextension at all, and then rejects any DNS-like common name under a constrained CA outright), so the certificates whose common name was held to the constraints were not the same certificates whose common name Bun uses for matching, and legacy SAN-less certificates with an in-subtree common name were rejected where Node.js accepts them.This adds
patches/boringssl/name-constraints-common-name.patch(applied through the existingpatches:list inscripts/build/deps/boringssl.ts), which replaces that block ofcheck_name_constraints()with the OpenSSL behaviour: when the leaf has no dNSName SAN, each common name that reads as a host name is matched against the issuer's constraints with BoringSSL's ownnc_match, and a mismatch is reported through the normal verify result (permitted subtree violation/excluded subtree violation). Because it is part ofX509_verify_cert, every client and server path (fetch,WebSocket,node:tls/node:https,Bun.connect/Bun.listen, the SQL and Valkey clients) observes it the same way, throughrejectUnauthorized/authorizationError, exactly like Node.js.Which common names count as host names is decided by BoringSSL's existing
x509v3_looks_like_dns_name(the same heuristicX509_check_hostuses for its common-name fallback, so on SAN-less certificates the two stay in step), with two adjustments: a leftmost label containing a single partial wildcard (w*.example.com, which Node-style matching honours) is widened to*first, so it is constrained like the whole-label wildcard that covers it; and next to asubjectAltNameextension a single-label common name (localhost,jsmith) is not taken to be a DNS identifier, as in OpenSSL'scn2dnsid, so client and device certificates of that shape keep verifying as they do in Node.js and in earlier Bun releases. OpenSSL also skips wildcard and absolute (trailing-dot) common names; Bun does not, so for that shape Bun is deliberately stricter than Node.js: under a name-constrained CA, a certificate without a DNSsubjectAltNamewhose common name is a wildcard pattern outside the permitted subtree fails chain verification (whatever name or address it is dialed by). Every other case in the comparison below matches Node.js. Decorative common names (Some Server,jdoe@example.com) are not treated as host names.How did you verify your code works?
test/js/node/tls/fixtures/name-constraints-*: root CA ->intermediatewithnameConstraints=critical,permitted;DNS:.allowed.test-> five leaves sharing one key:inside(CN=host.allowed.test, email SAN),outside(CN=outside.example.test, email SAN),wildcard(CN=.example.test, email SAN),partial-wildcard(CN=w.example.test, email SAN),nosan(CN=legacy.allowed.test, no SAN extension).test/js/node/tls/node-tls-connect-hostname-verification.test.ts:tls.connectrejectsoutside,wildcardandpartial-wildcardwithpermitted subtree violation(and reportsauthorized === false/authorizationError === "UNSPECIFIED"withrejectUnauthorized: false, matching Node.js), and acceptsinsideandnosan.test/js/web/fetch/fetch.tls.test.ts:fetchrejectsoutside,wildcardandpartial-wildcardwithPERMITTED_VIOLATIONand succeeds forinsideandnosan.nodev26.3.0,openssl verify3.2 and this build: they agree in 35; the 7 differences are the wildcard-pattern and label-boundary-hyphen common names described above plus a SAN-less single-label common name (rejected before and after this change), all of which Bun rejects and Node.js accepts. Relative to Bun 1.4, SAN-less certificates whose common name lies inside the permitted subtree (or whose issuers only constrain other name types) are now accepted, as in Node.js.test/js/node/tls/*,test/js/node/crypto/x509.test.ts,test/js/bun/http/bun-serve-ssl.test.tsand the vendoredtest-tls-*/test-crypto-x509Node.js tests pass.