From 5487a286fde37560c7d4f9dc752dfcd6982dbf54 Mon Sep 17 00:00:00 2001 From: jorge guerrero Date: Sun, 1 Mar 2026 19:48:58 -0500 Subject: [PATCH] tls: preserve servername on resumed TLS 1.3 sessions OpenSSL's SSL_get_servername() returns NULL on server-side TLS 1.3 resumed sessions because it reads from ssl->ext.hostname (not populated on resumption) rather than session->ext.hostname (which is persisted). The root cause is that Node.js's SNI callback (SelectSNIContextCallback) returns SSL_TLSEXT_ERR_NOACK when no SNI context switch is needed, which prevents OpenSSL from storing the hostname in the session. Without the hostname in the session, SSL_SESSION_get0_hostname() also returns NULL. Fix this with two minimal changes: 1. In SelectSNIContextCallback, explicitly call SSL_SESSION_set1_hostname() to persist the SNI in the session before ticket serialization. 2. In ncrypto::GetServerName(), fall back to SSL_SESSION_get0_hostname() when SSL_get_servername() returns NULL. Fixes: https://github.com/nodejs/node/issues/59202 Co-Authored-By: Claude Opus 4.6 --- deps/ncrypto/ncrypto.cc | 17 +++- src/crypto/crypto_context.h | 1 - src/crypto/crypto_tls.cc | 13 +++ .../test-tls-servername-session-resumption.js | 80 +++++++++++++++++++ 4 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-tls-servername-session-resumption.js diff --git a/deps/ncrypto/ncrypto.cc b/deps/ncrypto/ncrypto.cc index 3a26cfbdcab52e..af84dc31b076f4 100644 --- a/deps/ncrypto/ncrypto.cc +++ b/deps/ncrypto/ncrypto.cc @@ -2941,8 +2941,21 @@ std::optional SSLPointer::GetServerName( const SSL* ssl) { if (ssl == nullptr) return std::nullopt; auto res = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); - if (res == nullptr) return std::nullopt; - return res; + if (res != nullptr) return res; + +#ifndef OPENSSL_IS_BORINGSSL + // SSL_get_servername() returns NULL on server-side TLS 1.3 resumed sessions + // because it reads from ssl->ext.hostname rather than the session. Fall back + // to the session hostname which OpenSSL persists during the original + // handshake and serializes into tickets. + const SSL_SESSION* sess = SSL_get_session(ssl); + if (sess != nullptr) { + res = SSL_SESSION_get0_hostname(sess); + if (res != nullptr) return res; + } +#endif + + return std::nullopt; } std::optional SSLPointer::getServerName() const { diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h index b6801fc0b40708..1f5ed2a9d21a0d 100644 --- a/src/crypto/crypto_context.h +++ b/src/crypto/crypto_context.h @@ -53,7 +53,6 @@ class SecureContext final : public BaseObject { void SetKeylogCallback(KeylogCb cb); void SetNewSessionCallback(NewSessionCb cb); void SetSelectSNIContextCallback(SelectSNIContextCb cb); - inline const ncrypto::X509Pointer& issuer() const { return issuer_; } inline const ncrypto::X509Pointer& cert() const { return cert_; } diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 5c80184ffb5eae..e67d2d608d9edb 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1398,6 +1398,19 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) { !Set(env, p->GetOwner(), env->servername_string(), servername.value())) return SSL_TLSEXT_ERR_NOACK; +#ifndef OPENSSL_IS_BORINGSSL + // OpenSSL only persists the hostname in the session when this callback + // returns SSL_TLSEXT_ERR_OK (i.e. when an SNI context switch occurs). + // Explicitly store it so that SSL_SESSION_get0_hostname() works on resumed + // sessions regardless of the callback return value. + { + SSL_SESSION* sess = SSL_get_session(s); + const char* sni = SSL_get_servername(s, TLSEXT_NAMETYPE_host_name); + if (sess != nullptr && sni != nullptr) + SSL_SESSION_set1_hostname(sess, sni); + } +#endif + Local ctx = p->object()->Get(env->context(), env->sni_context_string()) .FromMaybe(Local()); diff --git a/test/parallel/test-tls-servername-session-resumption.js b/test/parallel/test-tls-servername-session-resumption.js new file mode 100644 index 00000000000000..e490860952d8c4 --- /dev/null +++ b/test/parallel/test-tls-servername-session-resumption.js @@ -0,0 +1,80 @@ +'use strict'; + +// Verify that servername (SNI) is preserved on resumed TLS sessions. +// Regression test for https://github.com/nodejs/node/issues/59202 + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +// The fix relies on SSL_SESSION_get0_hostname which BoringSSL may not support. +if (process.features.openssl_is_boringssl) + common.skip('BoringSSL does not support SSL_SESSION_get0_hostname'); + +const assert = require('assert'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const SERVERNAME = 'agent1.example.com'; + +function test(minVersion, maxVersion) { + const serverOptions = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + minVersion, + maxVersion, + }; + + let serverConns = 0; + const server = tls.createServer(serverOptions, common.mustCall((socket) => { + assert.strictEqual(socket.servername, SERVERNAME); + serverConns++; + // Don't end the socket immediately on the first connection — the client + // needs time to receive the TLS 1.3 NewSessionTicket message. + if (serverConns === 2) + socket.end(); + }, 2)); + + server.listen(0, common.mustCall(function() { + const port = server.address().port; + + // First connection: establish a session with an SNI servername. + const client1 = tls.connect({ + port, + servername: SERVERNAME, + rejectUnauthorized: false, + minVersion, + maxVersion, + }, common.mustCall()); + + client1.once('session', common.mustCall((session) => { + client1.end(); + + // Second connection: resume the session and verify the servername is + // preserved on the server side. + const client2 = tls.connect({ + port, + servername: SERVERNAME, + rejectUnauthorized: false, + session, + minVersion, + maxVersion, + }, common.mustCall(() => { + assert.strictEqual(client2.isSessionReused(), true); + client2.end(); + })); + + client2.on('close', common.mustCall(() => server.close())); + })); + + client1.resume(); + })); +} + +// TLS 1.3 — the primary bug: SSL_get_servername() returns NULL on +// server-side resumed sessions. +test('TLSv1.3', 'TLSv1.3'); + +// TLS 1.2 — OpenSSL handles this natively, but verify the fallback path +// doesn't break it. +test('TLSv1.2', 'TLSv1.2');