Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions .claude/commands/upgrade-boringssl.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
---
description: Upgrade Bun's BoringSSL fork (oven-sh/boringssl) to the latest upstream google/boringssl
---

Bun pins BoringSSL by **commit SHA** in `scripts/build/deps/boringssl.ts` (`BORINGSSL_COMMIT`). The build downloads a tarball from `oven-sh/boringssl` at that SHA — there is no submodule and `vendor/boringssl/` is git-ignored.

The fork carries a small patch set on top of upstream (see "Preserved patches" below). Upgrading means: merge `google/boringssl` into `oven-sh/boringssl@master`, push, then bump the SHA + regenerate source lists in Bun.

## Steps

### 1. Clone the fork and merge upstream

```sh
git clone https://github.com/oven-sh/boringssl.git /tmp/boringssl
cd /tmp/boringssl
git remote add upstream https://github.com/google/boringssl.git
git fetch upstream
git log --oneline $(git merge-base HEAD upstream/main)..HEAD # our patches
git merge upstream/main
```

Resolve conflicts **preserving the fork's additions**. Most conflicts are upstream's periodic `|...|` → `` `...` `` doc-comment restyle landing adjacent to a line we added — keep our line + upstream's comment style. For `include/openssl/nid.h`, keep upstream's new NIDs **and** ours (our NID numbers are from OpenSSL's range and don't collide with BoringSSL's sequential allocation).

### 2. Verify the merged tree builds

```sh
cmake -B build -GNinja -DCMAKE_BUILD_TYPE=Release
ninja -C build crypto ssl decrepit
```

This catches mis-resolved conflicts before they reach Bun's CI.

### 3. Push to the fork

The default branch is **`master`** (not `main`).

```sh
git push origin HEAD:master
NEW_SHA=$(git rev-parse HEAD)
```

### 4. Bump Bun

In the bun repo:

- `scripts/build/deps/boringssl.ts` — set `BORINGSSL_COMMIT` to `$NEW_SHA`.
- `test/js/node/process/process.test.js` — update the `boringssl:` entry in `expectedVersions` to `$NEW_SHA`.
- Regenerate the source lists (the file's header comment has the exact one-liner). Only `gen/sources.json` is authoritative — diff old vs new and apply the delta:

```sh
rm -rf vendor/boringssl # force re-fetch on next build
bun bd --target=clone-boringssl
bun -e 'const j=require("./vendor/boringssl/gen/sources.json");
const f=l=>l.map(JSON.stringify).join(", ");
for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs));
console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm]));
console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))'
Comment on lines +53 to +57

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use bun bd -e in this runbook step.

This snippet currently uses bun -e, which bypasses the repo’s documented debug-build command flow.

Suggested doc fix
-  bun -e 'const j=require("./vendor/boringssl/gen/sources.json");
+  bun bd -e 'const j=require("./vendor/boringssl/gen/sources.json");
           const f=l=>l.map(JSON.stringify).join(", ");
           for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs));
           console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm]));
           console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))'

As per coding guidelines: “Never use bun test or bun <file> directly - always use bun bd ....”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bun -e 'const j=require("./vendor/boringssl/gen/sources.json");
const f=l=>l.map(JSON.stringify).join(", ");
for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs));
console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm]));
console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))'
bun bd -e 'const j=require("./vendor/boringssl/gen/sources.json");
const f=l=>l.map(JSON.stringify).join(", ");
for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs));
console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm]));
console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))'
🤖 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 @.claude/commands/upgrade-boringssl.md around lines 52 - 56, The runbook step
in upgrade-boringssl.md currently uses the bun -e flag directly, which bypasses
the repository's documented debug-build command flow. Replace the bun -e
invocation with bun bd -e to conform to the coding guidelines that require all
bun commands to use the debug-build command wrapper instead of executing bun
directly.

Source: Coding guidelines

```

### 5. Build and test locally

```sh
rm -rf vendor/boringssl
bun bd -p 'require("crypto").createHash("sha3-256").update("hi").digest("hex")'
bun bd test test/js/node/crypto/ test/js/bun/crypto/
bun bd test test/js/node/tls/ test/js/web/fetch/fetch.tls.test.ts
```

### 6. Open the Bun PR

```sh
git checkout -b claude/boringssl-<upstream-short-sha>
git commit -am "deps: upgrade BoringSSL to <upstream-short-sha>"
git push -u origin HEAD
gh pr create
```

Then `bun run ci:watch` and fix anything that turns up.

## Preserved patches (what conflicts to expect)

`git diff $(git merge-base HEAD upstream/main)..HEAD --stat` — currently ~35 files, ~550 insertions:

- **SHA-512/224** — `crypto/fipsmodule/sha/sha512.cc.inc`, `crypto/sha/sha512.cc`, `include/openssl/{sha2,nid,digest}.h`
- **SHA3-224/256/384/512 as `EVP_MD`** — `crypto/digest/digest_extra.cc`, `crypto/fipsmodule/{digest/digests.cc.inc,keccak/*}`, `include/openssl/{digest,nid}.h`
- **HMAC-SHA3** — `crypto/hmac/hmac_test*.{cc,txt}`
- **BLAKE2b-512** — `crypto/blake2/blake2.cc`, `include/openssl/blake2.h`
- **RIPEMD160 in `crypto/` (not `decrepit/`) + `EVP_ripemd160` lookup** — `crypto/ripemd/ripemd.cc` (moved), `crypto/digest/digest_extra.cc`, `include/openssl/digest.h`, `gen/sources.*`, `build.json`
- **`EVP_PBE_validate_scrypt_params`** — `crypto/evp/scrypt.cc`, `include/openssl/evp.h`
- **Electron `SSL_want` / `EVP_CIPHER_do_all_sorted`** — `ssl/ssl_lib.cc` (return `rwstate` directly), `ssl/ssl_test.cc` (drops the corresponding test block), `decrepit/evp/evp_do_all.cc`, `crypto/cipher/get_cipher.cc`, `include/openssl/cipher.h`
- **MLDSA stack-frame pragma** — `crypto/fipsmodule/mldsa/mldsa.cc.inc`

If upstream upstreams any of these (check `git grep` on `upstream/main` before re-applying), drop the fork's copy.

## Things that have broken before

- **`SSL_CTX` / `SSL_ECH_KEYS` / `SSL_CREDENTIAL` made opaque** — Bun's Rust FFI (`src/boringssl_sys/boringssl.rs`) treats them as opaque already, so this is fine, but check `packages/bun-usockets/src/crypto/openssl.c` for any direct field access.
- **`BIO_read`/`BIO_write` error-value narrowing** — can change `SSL_read` error paths over memory BIOs (`SSLWrapper` for TLS-over-duplex). If `node-tls-connect.test.ts` crashes in `flush_pending_events`, see `src/runtime/socket/UpgradedDuplex.rs::teardown` and `WindowsNamedPipe.rs`'s `WRAPPER_BUSY` for the re-entrant-drop guard.
- **Per-handshake allocation churn (PQ key shares)** grows under ASAN quarantine; RSS-delta tests like `tls-connect-socket-churn.test.ts` may need their `isASAN` bound raised. The `sslCtxLiveCount` check is the real regression guard there — if that passes and LSAN is clean, raise the RSS bound.
- **`asn1_string_st` / `GENERAL_NAME_st` layout** — Bun mirrors these in `src/boringssl_sys/boringssl.rs`; diff `include/openssl/{asn1,x509v3}.h` for field changes.
6 changes: 3 additions & 3 deletions scripts/build/deps/boringssl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import type { Dependency, DirectBuild } from "../source.ts";
import { depSourceDir } from "../source.ts";

const BORINGSSL_COMMIT = "0c5fce43b7ed5eb6001487ee48ac65766f5ddcd1";
const BORINGSSL_COMMIT = "1a41b9025c2c0a37edd07ff10f6944f03e028522";

Check warning on line 27 in scripts/build/deps/boringssl.ts

View check run for this annotation

Claude / Claude Code Review

Stale hardcoded SSL_OP_LEGACY_SERVER_CONNECT=0 / SSL_OP_ALL=0 in FFI bindings after upstream restored them as real flags

This bump restores `SSL_OP_LEGACY_SERVER_CONNECT` as a real flag (`0x4`) and defines `SSL_OP_ALL = SSL_OP_LEGACY_SERVER_CONNECT`, but Bun's hand-mirrored constants in `src/boringssl_sys/boringssl.rs:552-554` and `src/boringssl_sys/boringssl.zig:18214,18218` still hardcode `0` (with a now-incorrect "BoringSSL defines this as 0 (no-op flag)" comment). There's no runtime regression today because the new BoringSSL applies this flag by default at `SSL_CTX_new`, so `configure_http_client_with_alpn`'s

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.

🟡 This bump restores SSL_OP_LEGACY_SERVER_CONNECT as a real flag (0x4) and defines SSL_OP_ALL = SSL_OP_LEGACY_SERVER_CONNECT, but Bun's hand-mirrored constants in src/boringssl_sys/boringssl.rs:552-554 and src/boringssl_sys/boringssl.zig:18214,18218 still hardcode 0 (with a now-incorrect "BoringSSL defines this as 0 (no-op flag)" comment). There's no runtime regression today because the new BoringSSL applies this flag by default at SSL_CTX_new, so configure_http_client_with_alpn's SSL_clear_options(ssl, 0)/SSL_set_options(ssl, 0) no-op is harmless — but the comment is now wrong, crypto.constants.SSL_OP_LEGACY_SERVER_CONNECT (C++ #includes the real header) will expose 4 while the Rust/Zig FFI uses 0, and upstream's TODO(crbug.com/41393419) to disable the flag by default would turn this into a real fetch() break on a future bump. Suggest updating both constants to 0x4 and adding an "SSL_OP_* constant mirrors" bullet to the new runbook's §Things that have broken before.

Extended reasoning...

What changed upstream

This PR bumps BORINGSSL_COMMIT from 0c5fce43b to 1a41b9025 (a merge of google/boringssl@606d3a344). One of the upstream changes — called out in this PR's own description as "Restored SSL_OP_LEGACY_SERVER_CONNECT (and fixed it for TLS 1.3), added SSL_OP_ALL defaults" — moved two macros out of the "deprecated, defined as zero" block in include/openssl/ssl.h:

Macro Old (0c5fce43b, ssl.h:5831/5835) New (1a41b9025, ssl.h:605/622)
SSL_OP_LEGACY_SERVER_CONNECT 0 0x00000004L
SSL_OP_ALL 0 SSL_OP_LEGACY_SERVER_CONNECT

What's stale in Bun

Bun hand-mirrors these constants in two places that this PR did not touch:

  • src/boringssl_sys/boringssl.rs:552-554:

    /// `SSL_OP_LEGACY_SERVER_CONNECT` — BoringSSL defines this as 0 (no-op flag);
    /// kept so callers can mirror the OpenSSL clear/set dance verbatim.
    pub const SSL_OP_LEGACY_SERVER_CONNECT: u32 = 0;

    The comment is now factually wrong.

  • src/boringssl_sys/boringssl.zig:18214,18218:

    pub const SSL_OP_ALL = @as(c_int, 0);
    pub const SSL_OP_LEGACY_SERVER_CONNECT = @as(c_int, 0);

These flow into configure_http_client_with_alpn — run on every outgoing fetch()/WebSocket TLS open — at src/http/lib.rs:940-941 and src/boringssl_sys/boringssl.zig:19068-19069:

boringssl::c::SSL_clear_options(ssl, boringssl::c::SSL_OP_LEGACY_SERVER_CONNECT);
boringssl::c::SSL_set_options(ssl, boringssl::c::SSL_OP_LEGACY_SERVER_CONNECT);

The function's doc comment says it "Sets … the legacy-server-connect option", but with the constant still 0 this is now SSL_clear_options(ssl, 0) + SSL_set_options(ssl, 0) — a pure no-op.

Why there's no runtime regression today

The new ssl.h doc for SSL_OP_LEGACY_SERVER_CONNECT explicitly states "This option is enabled by default", and the new SSL_OP_ALL doc says it "is not necessary to pass". Concretely, ssl/internal.h:3985 now initializes SSL_CTX::options = SSL_OP_ALL, and ssl_lib.cc:476 inherits ctx options into each SSL. So:

  1. SSL_CTX_new() sets bit 0x4 on the context.
  2. SSL_new() copies it onto the connection.
  3. Bun's configure_http_client_with_alpn calls SSL_clear_options(ssl, 0) → clears nothing → bit 0x4 stays set.
  4. SSL_set_options(ssl, 0) → ORs in nothing → bit 0x4 stays set.
  5. Handshake against a non-RFC5746 server proceeds exactly as before.

No SSL_CTX_clear_options caller elsewhere in Bun clears bit 4, so the inherited default survives. fetch() to legacy servers continues to work — the Rust/Zig code is correct only by coincidence of upstream's current default.

What is real

  1. Wrong commentboringssl.rs:552 now lies about what BoringSSL defines.
  2. Internal/external divergencecrypto.constants.SSL_OP_LEGACY_SERVER_CONNECT is exposed via NodeConstantsModule.h:794 / ProcessBindingConstants.cpp:822, which #include the real openssl/ssl.h and will now return 4 to JS, while the Rust/Zig FFI passes 0 internally. This is cosmetic today (JS-supplied secureOptions reaches BoringSSL via the C path with the real value), but it's exactly the kind of skew the bindings file is supposed to prevent.
  3. Latent break on the next bump — the new header carries TODO(crbug.com/41393419): Disable SSL_OP_LEGACY_SERVER_CONNECT by default at ssl.h:621. When upstream lands that, step (1) above stops setting bit 0x4, Bun's no-op clear/set still does nothing, and fetch() against non-RFC5746 servers starts failing with unsafe legacy renegotiation disabled — even though Bun's code intends to allow it.
  4. Runbook gap — this PR's new .claude/commands/upgrade-boringssl.md §"Things that have broken before" lists the asn1_string_st/GENERAL_NAME_st struct mirrors but not the hardcoded SSL_OP_* constants in boringssl.rs/boringssl.zig. This is precisely the silent-drift class that section is meant to catch.

Suggested fix

// src/boringssl_sys/boringssl.rs
/// `SSL_OP_LEGACY_SERVER_CONNECT` — allow initial connections to servers that
/// don't support RFC 5746 secure renegotiation. BoringSSL applies this in
/// `SSL_OP_ALL` by default; mirrored so callers can clear/set explicitly.
pub const SSL_OP_LEGACY_SERVER_CONNECT: u32 = 0x00000004;
// src/boringssl_sys/boringssl.zig
pub const SSL_OP_LEGACY_SERVER_CONNECT = @as(c_int, 0x00000004);
pub const SSL_OP_ALL = SSL_OP_LEGACY_SERVER_CONNECT;

And add to upgrade-boringssl.md §"Things that have broken before":

- **`SSL_OP_*` constant values** — Bun mirrors a handful in `src/boringssl_sys/boringssl.{rs,zig}`; diff `include/openssl/ssl.h` for value changes (`SSL_OP_LEGACY_SERVER_CONNECT` went 0→4 in the 606d3a344 bump).

Marking as a nit since there's no behavioral regression in this PR — but worth a one-line cleanup commit so the next BoringSSL bump doesn't silently break fetch().


export const boringssl: Dependency = {
name: "boringssl",
Expand Down Expand Up @@ -131,7 +131,7 @@
"crypto/evp/evp.cc", "crypto/evp/evp_asn1.cc", "crypto/evp/evp_ctx.cc", "crypto/evp/evp_kem.cc",
"crypto/evp/p_dh.cc", "crypto/evp/p_dsa.cc", "crypto/evp/p_ec.cc", "crypto/evp/p_ed25519.cc",
"crypto/evp/p_hkdf.cc", "crypto/evp/p_mldsa.cc", "crypto/evp/p_mlkem.cc", "crypto/evp/p_rsa.cc",
"crypto/evp/p_x25519.cc", "crypto/evp/pbkdf.cc", "crypto/evp/print.cc", "crypto/evp/scrypt.cc",
"crypto/evp/p_x25519.cc", "crypto/evp/p_xwing.cc", "crypto/evp/pbkdf.cc", "crypto/evp/print.cc", "crypto/evp/scrypt.cc",
"crypto/evp/sign.cc", "crypto/ex_data.cc", "crypto/fipsmodule/fips_shared_support.cc",
"crypto/fuzzer_mode.cc", "crypto/hpke/hpke.cc", "crypto/hrss/hrss.cc", "crypto/kyber/kyber.cc",
"crypto/lhash/lhash.cc", "crypto/md4/md4.cc", "crypto/md5/md5.cc", "crypto/mem.cc",
Expand All @@ -143,7 +143,7 @@
"crypto/pkcs8/pkcs8_x509.cc", "crypto/poly1305/poly1305.cc", "crypto/poly1305/poly1305_arm.cc",
"crypto/poly1305/poly1305_vec.cc", "crypto/pool/pool.cc", "crypto/rand/deterministic.cc",
"crypto/rand/fork_detect.cc", "crypto/rand/forkunsafe.cc", "crypto/rand/getentropy.cc",
"crypto/rand/ios.cc", "crypto/rand/passive.cc", "crypto/rand/rand.cc", "crypto/rand/trusty.cc",
"crypto/rand/ios.cc", "crypto/rand/rand.cc", "crypto/rand/trusty.cc",
"crypto/rand/urandom.cc", "crypto/rand/windows.cc", "crypto/rc4/rc4.cc", "crypto/refcount.cc",
"crypto/ripemd/ripemd.cc", "crypto/rsa/rsa_asn1.cc", "crypto/rsa/rsa_crypt.cc",
"crypto/rsa/rsa_extra.cc", "crypto/rsa/rsa_print.cc", "crypto/sha/sha1.cc",
Expand Down
16 changes: 15 additions & 1 deletion src/runtime/socket/UpgradedDuplex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,21 @@ impl UpgradedDuplex {
// clear the timer
self.set_timeout(0);

self.wrapper = None; // Drop runs SSLWrapper teardown
// Neuter in place rather than `self.wrapper = None`: `teardown()` can
// run re-entrantly from `on_close` while a `SSLWrapper::handle_traffic`
// frame is still on the stack with a `*mut Self` into the `Some`
// payload. Assigning `None` to the `Option` runs `Drop` (fine -
// `deinit()` nulls `ssl`/`ctx`) but then memmoves a fresh
// `Option::None` value over the slot, whose payload bytes are stack
// garbage - the in-flight frame's `Self::r(this).ssl` then reads junk
// and `flush_pending_events` UAFs into BoringSSL. `deinit()` alone
// leaves `ssl = None` / `closed_notified = true` readable so those
// guards work; the `Option` is dropped for real when the parent
// `DuplexUpgradeContext` frees on the next tick. See WindowsNamedPipe's
// WRAPPER_BUSY for the sibling pattern.
if let Some(wrapper) = self.wrapper.as_mut() {
wrapper.deinit();
}

self.origin.deinit();
if let Some(callback) = self.on_data_callback.get() {
Expand Down
2 changes: 1 addition & 1 deletion test/js/node/process/process.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ it("process.versions", () => {
// These are the ACTUAL commits built into bun (not derived values, so
// bumping a dep requires updating this test too).
const expectedVersions = {
boringssl: "0c5fce43b7ed5eb6001487ee48ac65766f5ddcd1",
boringssl: "1a41b9025c2c0a37edd07ff10f6944f03e028522",
libarchive: "ded82291ab41d5e355831b96b0e1ff49e24d8939",
mimalloc: "afb41757285694f832e7a2f164d35f5717457f96",
picohttpparser: "066d2b1e9ab820703db0837a7255d92d30f0c9f5",
Expand Down
6 changes: 5 additions & 1 deletion test/js/node/tls/tls-connect-socket-churn.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ test("tls.connect churn does not leak SSL_CTX or us_socket_context_t", async ()
// first verify all bump it). The original regression was ~50 KB/conn of
// SSL_CTX; bound at 16 MB so a return of that leak (50 × 50 KB ≈ 2.5 MB on
// top of the noise floor) would still trip without flaking on the noise.
const rssBound = isASAN || isDebug ? 64 * 1024 * 1024 : 16 * 1024 * 1024;
// ASAN/debug bound is high because BoringSSL's per-handshake allocation
// churn (PQ key shares, transcript buffers) lands in the ASAN quarantine
// and isn't released by Bun.gc — when this file runs after the rest of the
// tls/ suite the delta hits ~150 MB with zero LSAN-reported per-conn leak.
const rssBound = isASAN || isDebug ? 300 * 1024 * 1024 : 16 * 1024 * 1024;
expect(rssAfter - rssBefore).toBeLessThan(rssBound);
} finally {
server.close();
Expand Down
Loading