Skip to content

tls: load intermediates and trusted-people from Windows system stores#30408

Merged
cirospaciari merged 1 commit into
mainfrom
claude/win-system-ca-intermediates
May 8, 2026
Merged

tls: load intermediates and trusted-people from Windows system stores#30408
cirospaciari merged 1 commit into
mainfrom
claude/win-system-ca-intermediates

Conversation

@cirospaciari

@cirospaciari cirospaciari commented May 8, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Brings --use-system-ca / NODE_USE_SYSTEM_CA on Windows to parity with Node.js's ReadWindowsCertificates (src/crypto/crypto_context.cc).

Before this change, root_certs_windows.cpp only enumerated the ROOT store for CURRENT_USER and LOCAL_MACHINE (2 CertOpenStore calls). Node opens 13: ROOT, CA (intermediates), and TrustedPeople across LOCAL_MACHINE, CURRENT_USER, and the group-policy / enterprise variants — and filters by EKU.

The most user-visible consequence of the old behavior: when a server is misconfigured and sends only the leaf cert without its intermediates (very common on intranets, the primary use case for --use-system-ca), Node can still build the chain from the intermediates Windows keeps in the CA store; Bun would fail with unable to get local issuer certificate.

Changes, all mirroring Node:

before after
Store names ROOT ROOT, CA, TrustedPeople
Locations LOCAL_MACHINE, CURRENT_USER + GROUP_POLICY, ENTERPRISE variants
CERT_STORE_OPEN_EXISTING_FLAG no yes (don't create a missing store)
EKU server-auth filter (CertGetEnhancedKeyUsage) no yes (skip certs restricted to e.g. code-signing only)

IsCertTrustedForServerAuth and GatherCertsForLocation are direct ports of the equivalents in Node's crypto_context.cc, adapted to Bun's raw-DER-blob layering (this TU is kept OpenSSL-free to avoid wincrypt.h / BoringSSL macro collisions; root_certs.cpp does the d2i_X509 conversion).

Related issues (context, not fixes)

The issue-finder bot flagged #17108, #28612, and #9365. None of them are closed by this PR because it only changes behavior when --use-system-ca / NODE_USE_SYSTEM_CA=1 is set:

Match Node.js's ReadWindowsCertificates when --use-system-ca /
NODE_USE_SYSTEM_CA is enabled on Windows. Previously we only enumerated
the ROOT store for CURRENT_USER and LOCAL_MACHINE, so intermediates
installed in the CA store were ignored and chain building failed for
servers that omit intermediates from their handshake (common in
enterprise/intranet deployments where --use-system-ca is most useful).

Changes, all mirroring src/crypto/crypto_context.cc in Node:
- Also enumerate the CA (Intermediate Certification Authorities) store
  and the LOCAL_MACHINE TrustedPeople store.
- Also enumerate the GROUP_POLICY and ENTERPRISE location variants.
- Pass CERT_STORE_OPEN_EXISTING_FLAG so we never create a missing store.
- Filter each cert with CertGetEnhancedKeyUsage so only certs trusted
  for server auth (or any-EKU) are loaded; previously every cert in the
  store was loaded regardless of its restricted usage.
@robobun

robobun commented May 8, 2026

Copy link
Copy Markdown
Collaborator
Updated 12:43 PM PT - May 8th, 2026

@cirospaciari, your commit 0e966e4 has 3 failures in Build #52862 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30408

That installs a local version of the PR into your bun-30408 executable, so you can run:

bun-30408 --bun

@coderabbitai

coderabbitai Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Windows certificate loading is refactored to filter certificates by extended key usage (EKU) for server authentication. A new trust-check function validates EKU properties, a helper enumerates store locations with filtering, and the main export function collects roots, intermediates, and trusted people from multiple system stores to enable complete chain building.

Changes

Windows Certificate Loading with EKU Filtering

Layer / File(s) Summary
Dependencies
packages/bun-usockets/src/crypto/root_certs_windows.cpp
Added std::string_view include for EKU identifier string comparisons.
Certificate Trust Filter
packages/bun-usockets/src/crypto/root_certs_windows.cpp
New IsCertTrustedForServerAuth() function queries Windows EKU properties via CertGetEnhancedKeyUsage, validates server-auth capability, and fails closed on API errors.
Store Location Enumeration
packages/bun-usockets/src/crypto/root_certs_windows.cpp
New GatherCertsForLocation() helper opens system certificate stores, enumerates certificates, filters by server-auth trust, and exports raw DER bytes.
Exported Function Update
packages/bun-usockets/src/crypto/root_certs_windows.cpp
us_load_system_certificates_windows_raw() expanded to gather ROOT, CA, and TrustedPeople certificates from both CURRENT_USER and LOCAL_MACHINE stores for complete chain building support.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: loading intermediates and trusted-people certificates from Windows system stores, which directly reflects the core functionality added in this PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description is comprehensive and well-structured, covering all required template sections with detailed context.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. FR: Support Loading Certificates from Windows Certificate Store #17108 - Directly requests loading certificates from the Windows Certificate Store; this PR expands which stores are read (CA, TrustedPeople, additional locations)
  2. Windows: interactive TTY startup can make HTTPS fetch fail with unable to get local issuer certificate while WebSocket still opens #28612 - Windows HTTPS fetch fails with "unable to get local issuer certificate"; loading intermediates from the CA store addresses this
  3. UNABLE_TO_GET_ISSUER_CERT_LOCALLY with Bun.connect and socket.upgradeTLS #9365 - UNABLE_TO_GET_ISSUER_CERT_LOCALLY with Bun.connect/upgradeTLS; expanded Windows store reading provides missing intermediate certs

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #17108
Fixes #28612
Fixes #9365

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any issues — the implementation is a faithful port of Node's ReadWindowsCertificates / IsCertTrustedForServerAuth — but since this changes the TLS trust-anchor set on Windows (intermediates and TrustedPeople now flow into X509_STORE_add_cert) and hasn't been exercised end-to-end on a Windows box, it's worth a human sign-off.

Extended reasoning...

Overview

Single-file change to packages/bun-usockets/src/crypto/root_certs_windows.cpp. Expands --use-system-ca on Windows from 2 store enumerations (ROOT × {LM, CU}) to 13 (ROOT/CA/TrustedPeople across LM, CU, group-policy, and enterprise locations), adds CERT_STORE_OPEN_EXISTING_FLAG, and introduces an EKU filter (IsCertTrustedForServerAuth) that skips certs not valid for server auth. The exported symbol and RawCertificate ABI are unchanged; the consumer in root_certs.cpp:292 still d2i_X509's each blob and us_get_default_ca_store (line 248) adds every entry via X509_STORE_add_cert.

Security risks

This is the TLS trust-store loader. The net effect is that everything returned here becomes a trust anchor in the default X509_STORE when --use-system-ca is on. Pulling in the CA (intermediate) and TrustedPeople stores widens the anchor set beyond what Bun previously trusted. That widening is intentional and matches Node.js exactly (Node's ReadWindowsCertificates does the same X509_STORE_add_cert for all 13 stores), it's opt-in, and the new EKU filter actually tightens things relative to the old code which had no filter at all. I don't see an exploitable issue, but "changes what certificates are trusted for TLS" is inherently a human-review category.

Level of scrutiny

High. Trust-store population is security-critical, the change is Windows-only, and the author notes it was only cross-compiled — not run — on Windows, with no test fixture covering the new stores. CI will catch build/link breaks but not behavioral regressions.

Other factors

  • Logic is a near-verbatim port of Node's crypto_context.cc (function names, location whitelist, EKU OID checks, CRYPT_E_NOT_FOUND handling all line up), which substantially de-risks it.
  • No prior reviews on the PR; only the build-bot comment.
  • No bugs flagged by the automated review.

@cirospaciari cirospaciari merged commit 1600acc into main May 8, 2026
76 of 79 checks passed
@cirospaciari cirospaciari deleted the claude/win-system-ca-intermediates branch May 8, 2026 19:19
springmin pushed a commit to springmin/bun that referenced this pull request May 10, 2026
…oven-sh#30408)

### What does this PR do?

Brings `--use-system-ca` / `NODE_USE_SYSTEM_CA` on Windows to parity
with Node.js's `ReadWindowsCertificates`
(`src/crypto/crypto_context.cc`).

Before this change, `root_certs_windows.cpp` only enumerated the `ROOT`
store for `CURRENT_USER` and `LOCAL_MACHINE` (2 `CertOpenStore` calls).
Node opens 13: `ROOT`, `CA` (intermediates), and `TrustedPeople` across
`LOCAL_MACHINE`, `CURRENT_USER`, and the group-policy / enterprise
variants — and filters by EKU.

The most user-visible consequence of the old behavior: when a server is
misconfigured and sends only the leaf cert without its intermediates
(very common on intranets, the primary use case for `--use-system-ca`),
Node can still build the chain from the intermediates Windows keeps in
the `CA` store; Bun would fail with `unable to get local issuer
certificate`.

Changes, all mirroring Node:

| | before | after |
|---|---|---|
| Store names | `ROOT` | `ROOT`, `CA`, `TrustedPeople` |
| Locations | `LOCAL_MACHINE`, `CURRENT_USER` | + `GROUP_POLICY`,
`ENTERPRISE` variants |
| `CERT_STORE_OPEN_EXISTING_FLAG` | no | yes (don't create a missing
store) |
| EKU server-auth filter (`CertGetEnhancedKeyUsage`) | no | yes (skip
certs restricted to e.g. code-signing only) |

`IsCertTrustedForServerAuth` and `GatherCertsForLocation` are direct
ports of the equivalents in Node's `crypto_context.cc`, adapted to Bun's
raw-DER-blob layering (this TU is kept OpenSSL-free to avoid
`wincrypt.h` / BoringSSL macro collisions; `root_certs.cpp` does the
`d2i_X509` conversion).

### Related issues (context, not fixes)

The issue-finder bot flagged oven-sh#17108, oven-sh#28612, and oven-sh#9365. None of them are
closed by this PR because it only changes behavior when
`--use-system-ca` / `NODE_USE_SYSTEM_CA=1` is set:

- oven-sh#17108 asked for the base feature, which oven-sh#22441 already shipped — this
PR refines which Windows stores it reads.
- oven-sh#28612 reports `unable to get local issuer certificate` with **no**
`--use-system-ca` set (default bundled store, public CAs, TTY-startup
race) — different layer.
- oven-sh#9365 reproduces on WSL/Linux too and predates `--use-system-ca` —
likely a server omitting intermediates with no system-store fallback at
all.

Signed-off-by: Sisyphus <sisyphus@ohos-bun.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants