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
23 changes: 14 additions & 9 deletions src/js/node/tls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,10 @@ function normalizePemKeyOption(key, ctxPassphrase) {
});
}

function newNativeSecureContext(options, cached = true) {
// The digest cache is opt-in: the internal connect/listen paths pass
// `cached = true` explicitly. A forgotten opt-in on a future entry point is a
// perf regression, not a shared trust store.
function newNativeSecureContext(options, cached = false) {
maybeWarnAboutExtraCACerts();
// tls.createSecureContext() with no options still goes through the version
// translation below so the module-level DEFAULT_MIN/MAX_VERSION apply.
Expand Down Expand Up @@ -737,7 +740,7 @@ var InternalSecureContext = class SecureContext {
context;
servername;

constructor(options, cached = true) {
constructor(options, cached = false) {
// When tls.setDefaultCACertificates() has installed an override and no
// explicit `ca` was given, use the override as the default CA set so the
// process-wide default applies on every construction path (the public
Expand Down Expand Up @@ -789,7 +792,9 @@ var InternalSecureContext = class SecureContext {
};

function SecureContext(options): void {
return new InternalSecureContext(options) as never;
// Same contract as createSecureContext(): user-constructed contexts own
// their SSL_CTX exclusively (see the note there), so delegate to it.
return createSecureContext(options) as never;
Comment thread
alii marked this conversation as resolved.
}

function createSecureContext(options) {
Expand All @@ -801,7 +806,7 @@ function createSecureContext(options) {
// is built fresh because it carries the per-call `servername`.
// The user-facing constructor owns its SSL_CTX exclusively so addCACert
// cannot leak across contexts; internal connect/listen paths stay cached.
return new InternalSecureContext(options, false);
return new InternalSecureContext(options);
}

// Translate some fields from the handle's C-friendly format into more idiomatic
Expand Down Expand Up @@ -914,9 +919,9 @@ function TLSSocket(socket?, options?) {
// server-upgrade method below; leaving it unset until then means a synchronous
// teardown during upgradeTLS won't call close() on the bare net.Socket.
}
// Internal path: keep the per-digest cache (only the user-facing
// tls.createSecureContext() owns its SSL_CTX exclusively).
this[ksecureContext] = options.secureContext || new InternalSecureContext(options);
// Internal path: keep the per-digest cache (the user-facing constructors,
// createSecureContext() and new tls.SecureContext(), own theirs exclusively).
this[ksecureContext] = options.secureContext || new InternalSecureContext(options, true);
this.authorized = false;
this.secureConnecting = true;
this._secureEstablished = false;
Expand Down Expand Up @@ -1106,7 +1111,7 @@ TLSSocket.prototype.setKeyCert = function setKeyCert(context) {
// Serve this connection's identity from the given context (Node calls this
// from ALPNCallback/SNICallback before the certificate is sent). Accepts a
// SecureContext or the same options object createSecureContext takes.
const ctx = context?.context ? context : new InternalSecureContext(context);
const ctx = context?.context ? context : new InternalSecureContext(context, true);
this._handle?.setKeyCert?.(ctx.context);
};

Expand Down Expand Up @@ -1294,7 +1299,7 @@ function Server(options, secureConnectionListener): void {
throw new TypeError("hostname must be a string");
}
if (!(context instanceof InternalSecureContext)) {
context = new InternalSecureContext(context);
context = new InternalSecureContext(context, true);
}
const handle = this._handle;
if (handle) {
Expand Down
7 changes: 4 additions & 3 deletions src/runtime/api/SecureContext.classes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ export default [
// digest so identical configs return the same JS cell. Replaces the
// old SHA-256/WeakRef cache that lived in `tls.ts`.
intern: { fn: "intern", length: 1 },
// `tls.createSecureContext()` — exclusive-ownership variant: no digest
// memoisation at either cache level, so addCACert on one context can
// never affect another. The connect/listen paths keep using `intern`.
// The user-facing constructors (`tls.createSecureContext()` and
// `new tls.SecureContext()`) — exclusive ownership: no digest memoisation
// at either cache level, so addCACert on one context can never affect
// another. The internal connect/listen paths keep using `intern`.
createPrivate: { fn: "create_private", length: 1 },
// Parses a PKCS#12 (`pfx`) blob into { key, cert, ca } PEM strings so
// the regular key/cert/ca option plumbing can consume it.
Expand Down
15 changes: 14 additions & 1 deletion src/runtime/api/bun/SecureContext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ pub struct SecureContext {
/// Approximate cert/key/CA byte length plus the BoringSSL `SSL_CTX` floor
/// (~50 KB), so the GC can account for the off-heap allocation.
pub extra_memory: usize,
/// Whether `ctx` is a digest-interned `SSL_CTX*` that other consumers may
/// also hold. Set for every path through `intern`/`create_with_digest`;
/// only `create_private` builds an exclusively-owned context. Prototype
/// mutators (`add_ca_cert`) refuse to touch a shared context so a stray
/// user-reachable interned handle can never poison the cache.
pub shared: bool,
}

/// Exposed via `bun:internal-for-testing` so churn tests can assert
Expand Down Expand Up @@ -186,7 +192,7 @@ impl SecureContext {
Ok(result)
}

/// `tls.createSecureContext()` entry - builds a context that owns its
/// `tls.createSecureContext()` / `new tls.SecureContext()` entry - builds a context that owns its
/// SSL_CTX exclusively: no digest memoisation at either the JS-wrapper
/// cache or the native SSLContextCache level, so prototype mutators like
/// `addCACert` can never affect another context (or the cached
Expand Down Expand Up @@ -227,6 +233,7 @@ impl SecureContext {
ctx,
digest: d,
extra_memory: ctx_opts.approx_cert_bytes() + SSL_CTX_BASE_COST,
shared: false,
});
Ok(Self::to_js_boxed(sc, global))
}
Expand Down Expand Up @@ -325,6 +332,7 @@ impl SecureContext {
ctx,
digest: d,
extra_memory: ctx_opts.approx_cert_bytes() + SSL_CTX_BASE_COST,
shared: true,
}))
}

Expand All @@ -348,6 +356,11 @@ impl SecureContext {
global: &JSGlobalObject,
frame: &CallFrame,
) -> JsResult<JSValue> {
if this.shared {
return Err(global.throw(format_args!(
"cannot mutate a shared SecureContext; use tls.createSecureContext()"
)));
}
let args = frame.arguments();
if args.is_empty() {
return Err(
Expand Down
50 changes: 50 additions & 0 deletions test/js/node/tls/ssl-ctx-cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,56 @@ test("addCACert on one user-facing context does not affect another with identica
expect(a.context).not.toBe(b.context);
});

// The digest-interned cache is deliberately reachable only through internal
// paths, but if one leaks (Symbol.for constructor, TLSSocket internals) the
// mutator itself must refuse rather than silently poison every consumer.
test("addCACert on a digest-interned context throws instead of poisoning the cache", () => {
const NativeSecureContext = tls.Server.prototype[Symbol.for("::buntlsnativesecurecontextctor::")];
const a = NativeSecureContext.intern({ ca: tlsCerts.cert });
const b = NativeSecureContext.intern({ ca: tlsCerts.cert });
expect(a).toBe(b);
expect(() => a.addCACert(tlsCerts.ca)).toThrow("cannot mutate a shared SecureContext");
});

// The exported constructor is user-facing too: it must never hand out the
// digest-interned SSL_CTX, or addCACert on one instance would silently extend
// the trust store of every context sharing that digest.
test("new tls.SecureContext() owns its native handle exclusively, like createSecureContext()", () => {
const a = new (tls as any).SecureContext({ ca: tlsCerts.cert });
const b = new (tls as any).SecureContext({ ca: tlsCerts.cert });
// The interned cache would hand both the same native cell.
expect(a.context).not.toBe(b.context);
});

test("addCACert on one exported SecureContext instance does not change what another verifies", async () => {
// agent6's chain roots at ca1, which is not in the default roots: a client
// using `b` must keep rejecting it after `a` starts trusting ca1 (Node
// isolates the contexts and fails this connection closed).
const fx = (n: string) => readFileSync(join(import.meta.dir, "fixtures", n), "utf8");
const server = tls.createServer({ key: fx("agent6-key.pem"), cert: fx("agent6-cert.pem") }, s => s.end());
server.listen(0);
await once(server, "listening");
const { port } = server.address() as import("net").AddressInfo;
const a = new (tls as any).SecureContext({ ca: fx("ca2-cert.pem") });
const b = new (tls as any).SecureContext({ ca: fx("ca2-cert.pem") });
a.context.addCACert(fx("ca1-cert.pem"));
const outcome = Promise.withResolvers<string>();
const socket = tls.connect({
port,
secureContext: b,
rejectUnauthorized: true,
checkServerIdentity: () => undefined,
});
socket.on("secureConnect", () => outcome.resolve(`secureConnect authorized=${socket.authorized}`));
Comment thread
alii marked this conversation as resolved.
socket.on("error", error => outcome.resolve(`error ${(error as NodeJS.ErrnoException).code}`));
try {
expect(await outcome.promise).toBe("error UNABLE_TO_GET_ISSUER_CERT_LOCALLY");
} finally {
socket.destroy();
server.close();
}
});
Comment thread
claude[bot] marked this conversation as resolved.

test("setDefaultCACertificates() override applies to plain tls.connect (no explicit ca)", async () => {
const keys = (f: string) => readFileSync(join(import.meta.dir, "../test/fixtures/keys", f));
const prev = tls.getCACertificates("default");
Expand Down
Loading