denoland/deno · #36592
fix(ext/node): do not resume client TLS sessions unless requested
ext/node/ops/tls.rs4 + / 2 −
@@ -74,8 +74,10 @@ pub(crate) struct NodeTlsState { pub(crate) server_ticketer: Option<Arc<dyn deno_tls::rustls::server::ProducesTickets>>, /// Cached TLS-1.3 client cert verifiers and shared "no client cert"- /// resolvers, used when a client connection is built without custom CA- /// certs or a client cert. Reusing these `Arc`s across connections keeps+ /// resolvers, used when a client connection is built without per-context+ /// `ca` certs or a client cert (a process-level custom CA from+ /// `setDefaultCACertificates` is fine: the setter drops these caches+ /// whenever it changes). Reusing these `Arc`s across connections keeps /// rustls's session-resumption identity check (`Arc::downgrade(&verifier)`) /// stable, which is what allows `tls.TLSSocket#isSessionReused()` to /// return true on subsequent connections. Keep strict andext/node/ops/tls_wrap.rs122 + / 21 −
@@ -31,6 +31,8 @@ use std::io::Write; use std::ptr::NonNull; use std::rc::Rc; use std::sync::Arc;+use std::sync::atomic::AtomicBool;+use std::sync::atomic::Ordering; use deno_core::CppgcInherits; use deno_core::GarbageCollected;@@ -1154,6 +1156,11 @@ struct TLSWrapInner { client_hello_servername: Option<String>, client_hello_alpn: Vec<Vec<u8>>, + /// Whether this connection was configured with an explicit session to resume.+ /// Node.js client connections only attempt session resumption when+ /// `options.session` is passed (or `setSession()` is called).+ allow_resumption: Arc<AtomicBool>,+ /// User-supplied static read buffer (Node's `onread.buffer`). /// When set, decrypted data is copied into this buffer rather than /// emitted via a fresh ArrayBuffer; the JS callback receives the same@@ -1303,6 +1310,7 @@ impl TLSWrapInner { accepted: None, client_hello_servername: None, client_hello_alpn: Vec::new(),+ allow_resumption: Arc::new(AtomicBool::new(false)), user_buffer: None, task_spawner, }@@ -2262,10 +2270,12 @@ impl TLSWrap { }; let inner = unsafe { &mut *self.inner.as_mut_ptr() };- let client_config = match build_client_config(scope, context, op_state) {- Some((c, _)) => c,- None => return -1,- };+ let allow_resumption = inner.allow_resumption.clone();+ let client_config =+ match build_client_config(scope, context, op_state, allow_resumption) {+ Some((c, _)) => c,+ None => return -1,+ }; // The verifier in `client_config` writes errors via the per-connection // `CURRENT_VERIFY_ERROR` thread-local set by `cycle`, so `inner`'s own // pre-allocated `verify_error` slot stays correctly scoped per@@ -3286,13 +3296,17 @@ impl TLSWrap { // No-op for rustls } - /// Set the serialized TLS session for client resumption.- /// With the shared session store, rustls handles resumption- /// automatically, so this is a no-op; it only exists because the JS- /// layer calls it. isSessionReused() reports resumption based on the- /// negotiated handshake kind instead.+ /// Allow or disallow offering cached sessions for resumption on this+ /// connection. In Node.js, client connections only attempt session+ /// resumption when `options.session` is provided (or `setSession()` is+ /// called); the JS layer validates the session buffer against the+ /// destination and toggles this flag on the underlying+ /// `NodeClientSessionStoreWrapper`. #[fast]- fn set_session(&self, #[buffer] _session: &[u8]) {}+ fn set_session_allowed(&self, allowed: bool) {+ let inner = unsafe { &*self.inner.as_ptr() };+ inner.allow_resumption.store(allowed, Ordering::Relaxed);+ } /// Check if the TLS session was resumed (reused from a previous connection). #[fast]@@ -4053,11 +4067,80 @@ fn normalize_pem_headers(pem: &[u8]) -> std::borrow::Cow<'_, [u8]> { std::borrow::Cow::Owned(s.into_bytes()) } +#[derive(Debug)]+struct NodeClientSessionStoreWrapper {+ inner: Arc<dyn rustls::client::ClientSessionStore>,+ allow_resumption: Arc<AtomicBool>,+}++impl rustls::client::ClientSessionStore for NodeClientSessionStoreWrapper {+ fn set_kx_hint(+ &self,+ server_name: rustls::pki_types::ServerName<'static>,+ group: rustls::NamedGroup,+ ) {+ self.inner.set_kx_hint(server_name, group);+ }++ fn kx_hint(+ &self,+ server_name: &rustls::pki_types::ServerName<'_>,+ ) -> Option<rustls::NamedGroup> {+ self.inner.kx_hint(server_name)+ }++ fn set_tls12_session(+ &self,+ server_name: rustls::pki_types::ServerName<'static>,+ value: rustls::client::Tls12ClientSessionValue,+ ) {+ self.inner.set_tls12_session(server_name, value);+ }++ fn tls12_session(+ &self,+ server_name: &rustls::pki_types::ServerName<'_>,+ ) -> Option<rustls::client::Tls12ClientSessionValue> {+ if self.allow_resumption.load(Ordering::Relaxed) {+ self.inner.tls12_session(server_name)+ } else {+ None+ }+ }++ fn remove_tls12_session(+ &self,+ server_name: &rustls::pki_types::ServerName<'static>,+ ) {+ self.inner.remove_tls12_session(server_name);+ }++ fn insert_tls13_ticket(+ &self,+ server_name: rustls::pki_types::ServerName<'static>,+ value: rustls::client::Tls13ClientSessionValue,+ ) {+ self.inner.insert_tls13_ticket(server_name, value);+ }++ fn take_tls13_ticket(+ &self,+ server_name: &rustls::pki_types::ServerName<'static>,+ ) -> Option<rustls::client::Tls13ClientSessionValue> {+ if self.allow_resumption.load(Ordering::Relaxed) {+ self.inner.take_tls13_ticket(server_name)+ } else {+ None+ }+ }+}+ /// Build a rustls ClientConfig from a SecureContext JS object. fn build_client_config( scope: &mut v8::PinScope, context: v8::Local<v8::Object>, op_state: &mut OpState,+ allow_resumption: Arc<AtomicBool>, ) -> Option<(rustls::ClientConfig, VerifyErrorStore)> { use deno_net::DefaultTlsOptions; use deno_tls::TlsKeys;@@ -4095,6 +4178,12 @@ fn build_client_config( } } + // Whether the SecureContext itself carries `ca` certs, as opposed to+ // CA certs inherited from the process (default store or+ // setDefaultCACertificates). Only explicit per-context certs disqualify+ // the connection from the cached-verifier path below.+ let has_explicit_ca = !ca_certs.is_empty();+ let mut root_cert_store = op_state .borrow::<DefaultTlsOptions>() .root_cert_store()@@ -4174,17 +4263,17 @@ fn build_client_config( // The default-config fast path applies when the caller has not supplied // any of the per-connection knobs that would change cert validation or- // client auth: no extra `ca` certs, no explicit client cert/key, and no- // process-level custom CA set by `setDefaultCACertificates`. In that- // case we cache the verifier and the "no client cert" resolver in- // `NodeTlsState` so successive `tls.connect()` calls hand rustls the- // same `Arc`s and session resumption is allowed to proceed (rustls keys- // its `compatible_config` check on `Arc::downgrade(&verifier)` identity).- let is_default_path = ca_certs.is_empty()+ // client auth: no per-context `ca` certs and no explicit client+ // cert/key. In that case we cache the verifier and the "no client+ // cert" resolver in `NodeTlsState` so successive `tls.connect()` calls+ // hand rustls the same `Arc`s and session resumption is allowed to+ // proceed (rustls keys its `compatible_config` check on+ // `Arc::downgrade(&verifier)` identity). A process-level custom CA set+ // by `setDefaultCACertificates` stays on this path: it is shared by all+ // default-CA connections, and `op_set_default_ca_certificates` drops the+ // cached verifiers whenever it changes.+ let is_default_path = !has_explicit_ca && use_default_ca- && op_state- .try_borrow::<NodeTlsState>()- .is_none_or(|s| s.custom_ca_certs.is_none()) && matches!(maybe_cert_chain_and_key, TlsKeys::Null); // Always build with root certs so NodeServerCertVerifier can check them.@@ -4235,13 +4324,25 @@ fn build_client_config( // first handshake cannot be picked up by a later strict connection // (which would resume without re-running verification and bypass // checkServerIdentity in JS).+ //+ // In Node.js, client session resumption is opt-in per connection:+ // connections only offer cached sessions when `options.session` was+ // explicitly provided (or `setSession()` was called). `NodeClientSessionStoreWrapper`+ // saves newly issued sessions unconditionally (so getSession / 'session'+ // events work), but only offers them for resumption when `allow_resumption`+ // is true. if let Some(node_tls_state) = op_state.try_borrow::<NodeTlsState>() { let store = if reject_unauthorized { node_tls_state.client_session_store.clone() } else { node_tls_state.client_session_store_insecure.clone() };- config.resumption = rustls::client::Resumption::store(store);+ config.resumption = rustls::client::Resumption::store(Arc::new(+ NodeClientSessionStoreWrapper {+ inner: store,+ allow_resumption,+ },+ )); } // Install NodeServerCertVerifier to store verification errors forext/node/polyfills/_tls_wrap.js24 + / 8 −
@@ -517,6 +517,12 @@ TLSSocket.prototype[kReinitializeHandle] = function (handle) { if (options.ALPNProtocols) { this._handle.setAlpnProtocols(options.ALPNProtocols); }+ // Only re-enable resumption when the session already passed+ // setSession()'s host:port validation; an invalid buffer must not+ // enable resumption on the new handle.+ if (this._session && this._sessionReused) {+ this._handle.setSessionAllowed(true);+ } this._undestroy(); this._sockname = undefined;@@ -851,9 +857,15 @@ TLSSocket.prototype._init = function (socket, wrap) { ssl.onhandshakestart = noop; ssl.onhandshakedone = onhandshakedone; - if (options.session) {- ssl.setSession(options.session);- }+ // Unlike Node, options.session is not applied here: it can't be+ // validated against the destination yet, because kConnectOptions+ // isn't set until after construction. tls.connect() applies it via+ // setSession(), which validates against the connect options.+ // Consequence: a directly constructed+ // `new TLSSocket(socket, { session })` never resumes -- with no+ // kConnectOptions, syntheticSessionMatches() can't validate the+ // buffer. Acceptable under the synthetic-session design, since such+ // a buffer can't be matched to a destination in the shared cache. } if (options.ALPNProtocols) {@@ -1107,6 +1119,9 @@ TLSSocket.prototype.setSession = function (_session) { this._session, this[kConnectOptions], );+ if (this._handle) {+ this._handle.setSessionAllowed(this._sessionReused);+ } }; TLSSocket.prototype.getPeerCertificate = function (detailed) {@@ -1684,7 +1699,6 @@ function connect(...args) { isServer: false, requestCert: true, rejectUnauthorized: options.rejectUnauthorized !== false,- session: options.session, ALPNProtocols: options.ALPNProtocols, highWaterMark: options.highWaterMark, servername: options.servername,@@ -1696,6 +1710,12 @@ function connect(...args) { tlssock[kConnectOptions] = options; + // Apply the session before the connection can start so the resumption+ // gate is set before rustls creates the ClientConnection in start().+ if (options.session) {+ tlssock.setSession(options.session);+ }+ if (cb) { tlssock.once("secureConnect", cb); }@@ -1709,10 +1729,6 @@ function connect(...args) { tlssock._releaseControl(); - if (options.session) {- tlssock.setSession(options.session);- }- if (options.servername) { tlssock.setServername(options.servername); }tests/unit_node/tls_test.ts200 + / 0 −
@@ -1555,3 +1555,203 @@ Deno.test("tls write after underlying handle closed does not panic", async () => server.close(() => resolveClosed()); await closed; });++function startTlsEchoServer(): Promise<tls.Server> {+ const { promise, resolve } = Promise.withResolvers<tls.Server>();+ const server = tls.createServer({ cert, key }, (socket: net.Socket) => {+ socket.write("pong");+ socket.end();+ });+ server.listen(0, "127.0.0.1", () => resolve(server));+ return promise;+}++function closeServer(server: tls.Server): Promise<void> {+ const { promise, resolve } = Promise.withResolvers<void>();+ server.close(() => resolve());+ return promise;+}++// deno-lint-ignore no-explicit-any+function connectTlsClient(options: any, session?: Buffer | null) {+ const { promise, resolve, reject } = Promise.withResolvers<{+ isReused: boolean;+ session: Buffer | null;+ attemptedAddresses: string[] | undefined;+ }>();+ let sessionData: Buffer | null = null;+ let isReused = false;+ const client = tls.connect({ rejectUnauthorized: false, ...options });+ if (session !== undefined) {+ // deno-lint-ignore no-explicit-any+ (client as any).setSession(session);+ }+ client.on("session", (s: Buffer) => {+ sessionData = s;+ });+ client.on("secureConnect", () => {+ isReused = client.isSessionReused();+ client.end();+ });+ client.on("data", () => {});+ client.on("close", () => {+ resolve({+ isReused,+ session: sessionData,+ // deno-lint-ignore no-explicit-any+ attemptedAddresses: (client as any).autoSelectFamilyAttemptedAddresses,+ });+ });+ client.on("error", reject);+ client.resume();+ return deadline(promise, 5000);+}++// Regression test for https://github.com/denoland/deno/issues/36228+// Multiple client connections to the same host without options.session+// must not automatically resume TLS sessions (which breaks database connection+// pools like mssql/tedious where the server does not support session resumption).+// Session resumption must be strictly opt-in via options.session or setSession().+Deno.test("tls client session resumption is opt-in per connection", async (t) => {+ const server = await startTlsEchoServer();+ const { port } = server.address() as net.AddressInfo;+ let firstSession: Buffer | null = null;++ await t.step("initial connection is a full handshake", async () => {+ const res = await connectTlsClient({ port, host: "127.0.0.1" });+ assertEquals(res.isReused, false);+ assert(res.session !== null, "expected session data from connection 1");+ firstSession = res.session;+ });++ await t.step(+ "connection without options.session does not resume",+ async () => {+ const res = await connectTlsClient({ port, host: "127.0.0.1" });+ assertEquals(res.isReused, false);+ },+ );++ await t.step("options.session enables resumption", async () => {+ const res = await connectTlsClient({+ port,+ host: "127.0.0.1",+ session: firstSession!,+ });+ assertEquals(res.isReused, true);+ });++ await t.step("setSession() enables resumption", async () => {+ const res = await connectTlsClient(+ { port, host: "127.0.0.1" },+ firstSession,+ );+ assertEquals(res.isReused, true);+ });++ await t.step("malformed session buffer does not resume", async () => {+ const res = await connectTlsClient({+ port,+ host: "127.0.0.1",+ session: Buffer.from("invalid-session-data"),+ });+ assertEquals(res.isReused, false);+ });++ await t.step(+ "session for a different host:port does not resume",+ async () => {+ // A well-formed synthetic session issued for another server must fail+ // syntheticSessionMatches() and keep resumption disabled, even though+ // the shared cache holds tickets for this host.+ const serverB = await startTlsEchoServer();+ try {+ const { port: portB } = serverB.address() as net.AddressInfo;+ const res = await connectTlsClient({+ port: portB,+ host: "127.0.0.1",+ session: firstSession!,+ });+ assertEquals(res.isReused, false);+ } finally {+ await closeServer(serverB);+ }+ },+ );++ await closeServer(server);+});++// Opt-in resumption must keep working when a process-level custom CA is+// installed: rustls only offers a stored session when the config's verifier+// is the same instance the session was stored under, so build_client_config+// must stay on the cached-verifier path after setDefaultCACertificates().+Deno.test("tls client session resumption works with setDefaultCACertificates", async () => {+ // deno-lint-ignore no-explicit-any+ (tls as any).setDefaultCACertificates([rootCaCert]);+ const server = await startTlsEchoServer();+ try {+ const { port } = server.address() as net.AddressInfo;+ const res1 = await connectTlsClient({ port, host: "127.0.0.1" });+ assertEquals(res1.isReused, false);+ assert(res1.session !== null, "expected session data");+ const res2 = await connectTlsClient({+ port,+ host: "127.0.0.1",+ session: res1.session!,+ });+ assertEquals(res2.isReused, true);+ } finally {+ await closeServer(server);+ }+});++// When autoSelectFamily falls back to another address, kReinitializeHandle+// re-creates the TLSWrap; a session that passed setSession() validation must+// stay applied to the new handle.+Deno.test("tls autoSelectFamily fallback preserves a validated session", async () => {+ const server = await startTlsEchoServer();+ try {+ const { port } = server.address() as net.AddressInfo;+ // Resolve to an unreachable ::1 first so the connection falls back to+ // 127.0.0.1, re-creating the TLS handle for the second attempt.+ const lookup = (+ _host: string,+ opts: { all?: boolean },+ cb: (+ err: Error | null,+ addr: string | { address: string; family: number }[],+ family?: number,+ ) => void,+ ) => {+ if (opts.all) {+ cb(null, [+ { address: "::1", family: 6 },+ { address: "127.0.0.1", family: 4 },+ ]);+ } else {+ cb(null, "127.0.0.1", 4);+ }+ };+ const options = {+ port,+ host: "happy-eyeballs.example",+ lookup,+ autoSelectFamily: true,+ };+ const res1 = await connectTlsClient(options);+ assertEquals(res1.isReused, false);+ assert(res1.session !== null, "expected session data");++ const res2 = await connectTlsClient({ ...options, session: res1.session! });+ // Both addresses must have been attempted, proving the resumed+ // handshake ran on the re-created fallback handle.+ assertEquals(res2.attemptedAddresses, [+ `::1:${port}`,+ `127.0.0.1:${port}`,+ ]);+ assertEquals(res2.isReused, true);+ } finally {+ await closeServer(server);+ }+});