Skip to content

Commit 5487a28

Browse files
jorgitin02claude
andcommitted
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: #59202 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 6964b53 commit 5487a28

File tree

4 files changed

+108
-3
lines changed

4 files changed

+108
-3
lines changed

deps/ncrypto/ncrypto.cc

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2941,8 +2941,21 @@ std::optional<const std::string_view> SSLPointer::GetServerName(
29412941
const SSL* ssl) {
29422942
if (ssl == nullptr) return std::nullopt;
29432943
auto res = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
2944-
if (res == nullptr) return std::nullopt;
2945-
return res;
2944+
if (res != nullptr) return res;
2945+
2946+
#ifndef OPENSSL_IS_BORINGSSL
2947+
// SSL_get_servername() returns NULL on server-side TLS 1.3 resumed sessions
2948+
// because it reads from ssl->ext.hostname rather than the session. Fall back
2949+
// to the session hostname which OpenSSL persists during the original
2950+
// handshake and serializes into tickets.
2951+
const SSL_SESSION* sess = SSL_get_session(ssl);
2952+
if (sess != nullptr) {
2953+
res = SSL_SESSION_get0_hostname(sess);
2954+
if (res != nullptr) return res;
2955+
}
2956+
#endif
2957+
2958+
return std::nullopt;
29462959
}
29472960

29482961
std::optional<const std::string_view> SSLPointer::getServerName() const {

src/crypto/crypto_context.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ class SecureContext final : public BaseObject {
5353
void SetKeylogCallback(KeylogCb cb);
5454
void SetNewSessionCallback(NewSessionCb cb);
5555
void SetSelectSNIContextCallback(SelectSNIContextCb cb);
56-
5756
inline const ncrypto::X509Pointer& issuer() const { return issuer_; }
5857
inline const ncrypto::X509Pointer& cert() const { return cert_; }
5958

src/crypto/crypto_tls.cc

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1398,6 +1398,19 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {
13981398
!Set(env, p->GetOwner(), env->servername_string(), servername.value()))
13991399
return SSL_TLSEXT_ERR_NOACK;
14001400

1401+
#ifndef OPENSSL_IS_BORINGSSL
1402+
// OpenSSL only persists the hostname in the session when this callback
1403+
// returns SSL_TLSEXT_ERR_OK (i.e. when an SNI context switch occurs).
1404+
// Explicitly store it so that SSL_SESSION_get0_hostname() works on resumed
1405+
// sessions regardless of the callback return value.
1406+
{
1407+
SSL_SESSION* sess = SSL_get_session(s);
1408+
const char* sni = SSL_get_servername(s, TLSEXT_NAMETYPE_host_name);
1409+
if (sess != nullptr && sni != nullptr)
1410+
SSL_SESSION_set1_hostname(sess, sni);
1411+
}
1412+
#endif
1413+
14011414
Local<Value> ctx = p->object()->Get(env->context(), env->sni_context_string())
14021415
.FromMaybe(Local<Value>());
14031416

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
'use strict';
2+
3+
// Verify that servername (SNI) is preserved on resumed TLS sessions.
4+
// Regression test for https://github.com/nodejs/node/issues/59202
5+
6+
const common = require('../common');
7+
if (!common.hasCrypto)
8+
common.skip('missing crypto');
9+
10+
// The fix relies on SSL_SESSION_get0_hostname which BoringSSL may not support.
11+
if (process.features.openssl_is_boringssl)
12+
common.skip('BoringSSL does not support SSL_SESSION_get0_hostname');
13+
14+
const assert = require('assert');
15+
const tls = require('tls');
16+
const fixtures = require('../common/fixtures');
17+
18+
const SERVERNAME = 'agent1.example.com';
19+
20+
function test(minVersion, maxVersion) {
21+
const serverOptions = {
22+
key: fixtures.readKey('agent1-key.pem'),
23+
cert: fixtures.readKey('agent1-cert.pem'),
24+
minVersion,
25+
maxVersion,
26+
};
27+
28+
let serverConns = 0;
29+
const server = tls.createServer(serverOptions, common.mustCall((socket) => {
30+
assert.strictEqual(socket.servername, SERVERNAME);
31+
serverConns++;
32+
// Don't end the socket immediately on the first connection — the client
33+
// needs time to receive the TLS 1.3 NewSessionTicket message.
34+
if (serverConns === 2)
35+
socket.end();
36+
}, 2));
37+
38+
server.listen(0, common.mustCall(function() {
39+
const port = server.address().port;
40+
41+
// First connection: establish a session with an SNI servername.
42+
const client1 = tls.connect({
43+
port,
44+
servername: SERVERNAME,
45+
rejectUnauthorized: false,
46+
minVersion,
47+
maxVersion,
48+
}, common.mustCall());
49+
50+
client1.once('session', common.mustCall((session) => {
51+
client1.end();
52+
53+
// Second connection: resume the session and verify the servername is
54+
// preserved on the server side.
55+
const client2 = tls.connect({
56+
port,
57+
servername: SERVERNAME,
58+
rejectUnauthorized: false,
59+
session,
60+
minVersion,
61+
maxVersion,
62+
}, common.mustCall(() => {
63+
assert.strictEqual(client2.isSessionReused(), true);
64+
client2.end();
65+
}));
66+
67+
client2.on('close', common.mustCall(() => server.close()));
68+
}));
69+
70+
client1.resume();
71+
}));
72+
}
73+
74+
// TLS 1.3 — the primary bug: SSL_get_servername() returns NULL on
75+
// server-side resumed sessions.
76+
test('TLSv1.3', 'TLSv1.3');
77+
78+
// TLS 1.2 — OpenSSL handles this natively, but verify the fallback path
79+
// doesn't break it.
80+
test('TLSv1.2', 'TLSv1.2');

0 commit comments

Comments
 (0)