denoland/deno · #36592

fix(ext/node): do not resume client TLS sessions unless requested

innovatedev-john-pope · merged Aug 26, 20264 files · 350 + / 31
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 and
ext/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 for
ext/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);+  }+});