From 60d8561dd0aa33e8be04e3ffa972a95439a06046 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:02:22 +0000 Subject: [PATCH 01/81] test: fix dtls default CA test identity check The test connects to the IP literal 127.0.0.1 with rejectUnauthorized defaulting to true and no servername, so the peer identity is verified against that IP. agent1-cert.pem is CN = agent1 with no subjectAltName, so verification fails with X509_V_ERR_IP_ADDRESS_MISMATCH before the default CA set is exercised at all. Pass servername so the identity is matched against the certificate CN, keeping verification enabled while testing what the file is named for. Signed-off-by: James M Snell Assisted-by: Opencode --- test/parallel/test-dtls-default-ca.mjs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-dtls-default-ca.mjs b/test/parallel/test-dtls-default-ca.mjs index 7c3475ddbbe9..747d69edb051 100644 --- a/test/parallel/test-dtls-default-ca.mjs +++ b/test/parallel/test-dtls-default-ca.mjs @@ -31,7 +31,14 @@ const endpoint = listen(mustCall(), { port: 0, }); -const client = connect('127.0.0.1', endpoint.address.port); +// `servername` is both the SNI value and the identity checked during +// certificate verification. agent1-cert.pem has no subjectAltName, so the +// identity has to be matched against its CN; connecting to the IP literal +// alone would fail with X509_V_ERR_IP_ADDRESS_MISMATCH before the CA set is +// ever exercised. +const client = connect('127.0.0.1', endpoint.address.port, { + servername: 'agent1', +}); await client.opened; await client.close(); From bf44874255ca41c347acc8db8ff301084564c179 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:26:18 +0000 Subject: [PATCH 02/81] dtls: drain the OpenSSL error queue The error queue is per-thread and shared with every other OpenSSL consumer in the process. DTLS spends most of its time handling unauthenticated input, so failures are routine: rejected handshakes, and DTLSv1_listen() choking on garbage datagrams. None of those entries were discarded. ERR_get_error() in Cycle() and ClearOut() popped only the first entry, and nothing cleared the queue after a failed DTLSv1_listen(), SSL_write() or SSL_shutdown(). The residue was picked up by whatever crypto operation ran next and reported as its error: after 32 junk datagrams, crypto.createPrivateKey() on malformed PEM reported "record too small" with the real DECODER error demoted into opensslErrorStack. Add MarkPopErrorOnReturn to the entry points that drive OpenSSL, so each discards whatever it queued on the way out. Route error rendering through a helper that falls back to a description of the SSL error code when the queue is empty, instead of "error:00000000:lib(0)::reason(0)". Signed-off-by: James M Snell Assisted-by: Opencode --- src/dtls/dtls_endpoint.cc | 6 ++ src/dtls/dtls_session.cc | 58 ++++++++++++++--- test/parallel/test-dtls-error-queue.mjs | 82 +++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-dtls-error-queue.mjs diff --git a/src/dtls/dtls_endpoint.cc b/src/dtls/dtls_endpoint.cc index 44279399e478..e9753447a45c 100644 --- a/src/dtls/dtls_endpoint.cc +++ b/src/dtls/dtls_endpoint.cc @@ -473,6 +473,12 @@ void DTLSEndpoint::AcceptConnection(const uint8_t* data, HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(env()->context()); + // Anything reaching this point is unauthenticated and may well be garbage, + // so DTLSv1_listen() failing is routine rather than exceptional. Discard + // whatever it queues instead of letting it accumulate across packets and + // resurface as a bogus error somewhere else on this thread. + ncrypto::MarkPopErrorOnReturn mark_pop_error_on_return; + // Stateless cookie exchange via DTLSv1_listen() for DoS protection. // // The standard OpenSSL DTLS server flow (see s_server.c) is: diff --git a/src/dtls/dtls_session.cc b/src/dtls/dtls_session.cc index bd4034414414..590cf1506659 100644 --- a/src/dtls/dtls_session.cc +++ b/src/dtls/dtls_session.cc @@ -25,6 +25,7 @@ namespace node { +using ncrypto::MarkPopErrorOnReturn; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; @@ -39,6 +40,32 @@ using v8::Value; namespace dtls { +namespace { +// Format the OpenSSL error queue into a human readable message. +// +// Only the first (oldest, and therefore most specific) entry is rendered; the +// remainder are left for the enclosing MarkPopErrorOnReturn to discard. An +// SSL_ERROR_SSL can be reported with nothing queued -- ERR_error_string_n() +// renders 0 as "error:00000000:lib(0)::reason(0)", which tells nobody +// anything -- so fall back to a description of the SSL error code instead. +std::string FormatSSLError(int ssl_err) { + unsigned long first = ERR_get_error(); // NOLINT(runtime/int) + if (first != 0) { + char buf[256]; + ERR_error_string_n(first, buf, sizeof(buf)); + return buf; + } + switch (ssl_err) { + case SSL_ERROR_SYSCALL: + return "DTLS I/O error"; + case SSL_ERROR_ZERO_RETURN: + return "DTLS connection closed by peer"; + default: + return "DTLS protocol error"; + } +} +} // namespace + DTLSSession::DTLSSession(Environment* env, Local wrap, DTLSEndpoint* endpoint, @@ -59,6 +86,7 @@ DTLSSession::DTLSSession(Environment* env, // an error or running Cycle() below can synchronously // destroy this session, and this timer lives on it. BaseObjectPtr strong_ref{this}; + MarkPopErrorOnReturn mark_pop_error_on_return; DTLS_STAT_INCREMENT(DTLSSessionStats, retransmit_count); int ret = DTLSv1_handle_timeout(ssl_.get()); @@ -285,6 +313,8 @@ void DTLSSession::New(const FunctionCallbackInfo& args) { void DTLSSession::Receive(const uint8_t* data, size_t len) { if (destroyed_ || closed_) return; + MarkPopErrorOnReturn mark_pop_error_on_return; + // Write the encrypted datagram into enc_in_ BIO. int written = BIO_write(enc_in_, data, len); if (written <= 0) return; @@ -296,6 +326,12 @@ void DTLSSession::Receive(const uint8_t* data, size_t len) { void DTLSSession::Cycle() { if (destroyed_) return; + // Everything OpenSSL queues while the pump runs is consumed here (for the + // error callbacks below) or discarded on the way out. Leaving entries behind + // would misattribute them to whatever crypto operation runs next on this + // thread -- including unrelated node:crypto work. + MarkPopErrorOnReturn mark_pop_error_on_return; + // Pin a strong reference to ourselves for the duration of the pump. A JS // callback dispatched below (message/handshake/error) can synchronously // destroy this session, which removes the endpoint's only strong reference @@ -317,14 +353,13 @@ void DTLSSession::Cycle() { if (ret <= 0) { int err = SSL_get_error(ssl_.get(), ret); if (err == SSL_ERROR_SSL) { - unsigned long ossl_err = ERR_get_error(); // NOLINT(runtime/int) - char err_buf[256]; - ERR_error_string_n(ossl_err, err_buf, sizeof(err_buf)); + std::string message = FormatSSLError(err); // Flush any fatal alert OpenSSL queued for the peer before emitting the // error, which tears the session down and detaches the endpoint. EncOut(); Local argv[] = { - String::NewFromUtf8(env()->isolate(), err_buf).ToLocalChecked(), + String::NewFromUtf8(env()->isolate(), message.c_str()) + .ToLocalChecked(), }; EmitCallback(DTLS_CB_SESSION_ERROR, 1, argv); cycle_depth_--; @@ -409,14 +444,13 @@ void DTLSSession::ClearOut() { case SSL_ERROR_SSL: { // SSL error during handshake or data exchange. - unsigned long ossl_err = ERR_get_error(); // NOLINT(runtime/int) - char err_buf[256]; - ERR_error_string_n(ossl_err, err_buf, sizeof(err_buf)); + std::string message = FormatSSLError(err); // Flush any fatal alert OpenSSL queued for the peer before emitting the // error, which tears the session down and detaches the endpoint. EncOut(); Local argv[] = { - String::NewFromUtf8(env()->isolate(), err_buf).ToLocalChecked(), + String::NewFromUtf8(env()->isolate(), message.c_str()) + .ToLocalChecked(), }; EmitCallback(DTLS_CB_SESSION_ERROR, 1, argv); break; @@ -463,6 +497,8 @@ int DTLSSession::Send(const uint8_t* data, size_t len) { return -1; } + MarkPopErrorOnReturn mark_pop_error_on_return; + int written = SSL_write(ssl_.get(), data, len); if (written > 0) { DTLS_STAT_INCREMENT_N(DTLSSessionStats, bytes_sent, written); @@ -481,6 +517,8 @@ void DTLSSession::Close() { // strong reference so `this` survives until we return. BaseObjectPtr strong_ref{this}; + MarkPopErrorOnReturn mark_pop_error_on_return; + closed_ = true; state_->closing = 1; DTLS_STAT_RECORD_TIMESTAMP(DTLSSessionStats, closing_at); @@ -652,6 +690,8 @@ void DTLSSession::GetPeerCertificate(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); Environment* env = session->env(); + MarkPopErrorOnReturn mark_pop_error_on_return; + X509* peer_cert = SSL_get0_peer_certificate(session->ssl_.get()); if (peer_cert == nullptr) return; @@ -711,6 +751,8 @@ void DTLSSession::ExportKeyingMaterial( use_context = true; } + MarkPopErrorOnReturn mark_pop_error_on_return; + std::vector out(length); int ret = SSL_export_keying_material(session->ssl_.get(), out.data(), diff --git a/test/parallel/test-dtls-error-queue.mjs b/test/parallel/test-dtls-error-queue.mjs new file mode 100644 index 000000000000..10a323ff3d1f --- /dev/null +++ b/test/parallel/test-dtls-error-queue.mjs @@ -0,0 +1,82 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: DTLS does not leave entries behind in the OpenSSL error queue. +// +// The queue is per-thread and shared with every other OpenSSL consumer in the +// process. DTLS spends most of its time handling unauthenticated input, so +// failures are routine: rejected handshakes, and DTLSv1_listen() choking on +// garbage datagrams from the accept path. If those entries are not discarded +// they are picked up by whatever crypto operation runs next and reported as +// its error -- e.g. crypto.createPrivateKey() failing with +// "SSL routines::record too small". + +import { hasCrypto, skip } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; +import crypto from 'node:crypto'; +import dgram from 'node:dgram'; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { connect, listen } = await import('node:dtls'); + +const cert = fixtures.readKey('agent1-cert.pem'); +const key = fixtures.readKey('agent1-key.pem'); +const ca1 = fixtures.readKey('ca1-cert.pem'); +const ca2 = fixtures.readKey('ca2-cert.pem'); + +// The server accepts each session (cookie exchange completes) and only the +// client rejects, so sessions do arrive here; nothing needs doing with them. +const endpoint = listen(() => {}, { + cert, key, ca: [ca1], host: '127.0.0.1', port: 0, +}); +const { port } = endpoint.address; + +// Provoke failures from both directions. + +// 1. Rejected handshakes: the server certificate does not chain to ca2. +for (let i = 0; i < 3; i++) { + const client = connect('127.0.0.1', port, { + servername: 'agent1', + ca: [ca2], + }); + await assert.rejects(client.opened, (err) => { + assert.match(err.message, /certificate verify failed/); + return true; + }); +} + +// 2. Garbage datagrams: each one reaches DTLSv1_listen() and fails there. +// The leading 22 makes them look enough like a DTLS handshake record to get +// past a cheap length/type screen. +const socket = dgram.createSocket('udp4'); +for (let i = 0; i < 32; i++) { + socket.send(Buffer.from([22, 254, 253, 0, 0, i & 0xff]), port, '127.0.0.1'); +} +// Let the datagrams reach the endpoint and be processed before closing; +// dgram.close() does not flush queued sends. +await new Promise((resolve) => setTimeout(resolve, 200)); +await new Promise((resolve) => socket.close(resolve)); + +// An unrelated crypto failure must report its own cause. Before the error +// queue was drained this surfaced a stale "SSL routines::..." entry left by +// the DTLS activity above, with the real DECODER error demoted into +// opensslErrorStack. +assert.throws( + () => crypto.createPrivateKey( + '-----BEGIN PRIVATE KEY-----\nZm9v\n-----END PRIVATE KEY-----\n'), + (err) => { + assert.doesNotMatch( + err.message, /SSL routines/, + `crypto error contaminated by a stale DTLS entry: ${err.message}`); + return true; + }, +); + +await endpoint.close(); From 95412dbba2b19c34cd091ab889c6c25e0f041879 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:31:36 +0000 Subject: [PATCH 03/81] dtls: preserve record boundaries on the outbound BIO OpenSSL emits one BIO_write per DTLS record, each fragmented to fit SSL_set_mtu(). enc_out_ was a byte-stream BIO, so those boundaries were lost and EncOut() drained an entire handshake flight into one datagram, defeating the MTU setting. With an agent1 chain and mtu 512, the server flight went out as 60, 2490, 266 bytes -- the 2490 being five correctly sized records concatenated into one datagram that requires IP fragmentation, which NATs and middleboxes routinely drop. SSL_OP_NO_QUERY_MTU also disables OpenSSL's black-hole recovery, so such a handshake retransmits at the same broken size until it gives up. Use BIO_s_dgram_mem() for enc_out_, which returns exactly one datagram per BIO_read. It reports "empty" as a retry and grows on write, so it needs no BIO_set_mem_eof_return(). EncOut() now sends one record per iteration instead of one flight. Loopback has a 64 KiB MTU so no existing test could see this; the new one measures datagram sizes through a relay. Signed-off-by: James M Snell Assisted-by: Opencode --- src/dtls/dtls_endpoint.cc | 5 +- src/dtls/dtls_session.cc | 17 ++-- .../parallel/test-dtls-mtu-record-framing.mjs | 89 +++++++++++++++++++ 3 files changed, 104 insertions(+), 7 deletions(-) create mode 100644 test/parallel/test-dtls-mtu-record-framing.mjs diff --git a/src/dtls/dtls_endpoint.cc b/src/dtls/dtls_endpoint.cc index e9753447a45c..49f7fdbfcdee 100644 --- a/src/dtls/dtls_endpoint.cc +++ b/src/dtls/dtls_endpoint.cc @@ -500,12 +500,13 @@ void DTLSEndpoint::AcceptConnection(const uint8_t* data, ncrypto::SSLPointer ssl(SSL_new(server_context_->ssl_ctx())); if (!ssl) return; + // `out` becomes the session's enc_out_, so it has to preserve the datagram + // boundaries OpenSSL writes records on -- see DTLSSession::Create(). auto in = ncrypto::BIOPointer::NewMem(); - auto out = ncrypto::BIOPointer::NewMem(); + auto out = ncrypto::BIOPointer::New(BIO_s_dgram_mem()); if (!in || !out) return; BIO_set_mem_eof_return(in.get(), -1); - BIO_set_mem_eof_return(out.get(), -1); // SSL_set_bio takes ownership of both BIOs. BIO* in_raw = in.release(); BIO* out_raw = out.release(); diff --git a/src/dtls/dtls_session.cc b/src/dtls/dtls_session.cc index 590cf1506659..aa264f4f7f50 100644 --- a/src/dtls/dtls_session.cc +++ b/src/dtls/dtls_session.cc @@ -199,16 +199,21 @@ BaseObjectPtr DTLSSession::Create(Environment* env, ncrypto::SSLPointer ssl(ssl_raw); // Create memory BIOs for encrypted data I/O. + // enc_out_ must preserve datagram boundaries: OpenSSL emits one BIO_write + // per DTLS record, each sized to fit SSL_set_mtu(). A byte-stream BIO throws + // that away and lets EncOut() coalesce a whole flight into one oversized + // datagram. BIO_s_dgram_mem() returns exactly one datagram per BIO_read and + // already reports "empty" as a retry, so no BIO_set_mem_eof_return() is + // needed for it. auto enc_in = ncrypto::BIOPointer::NewMem(); - auto enc_out = ncrypto::BIOPointer::NewMem(); + auto enc_out = ncrypto::BIOPointer::New(BIO_s_dgram_mem()); if (!enc_in || !enc_out) { THROW_ERR_CRYPTO_OPERATION_FAILED(env, "BIO_new failed"); return {}; } - // Make the BIOs non-blocking. + // Make the BIO non-blocking. BIO_set_mem_eof_return(enc_in.get(), -1); - BIO_set_mem_eof_return(enc_out.get(), -1); // Associate BIOs with the SSL object. SSL_set_bio takes ownership. BIO* enc_in_raw = enc_in.release(); @@ -466,8 +471,10 @@ void DTLSSession::EncOut() { auto ep = endpoint_.get(); if (ep == nullptr) return; - // Read encrypted data from enc_out_ BIO and send via UDP. - // Read in a loop since there may be multiple DTLS records. + // enc_out_ is a datagram BIO, so each BIO_read yields exactly one datagram + // -- one DTLS record as OpenSSL framed it against the MTU. Loop because a + // single flight is several records, and send each one separately rather than + // concatenating them into a datagram that would need IP fragmentation. uint8_t buf[65536]; int read; while ((read = BIO_read(enc_out_, buf, sizeof(buf))) > 0) { diff --git a/test/parallel/test-dtls-mtu-record-framing.mjs b/test/parallel/test-dtls-mtu-record-framing.mjs new file mode 100644 index 000000000000..610d86c4e3a1 --- /dev/null +++ b/test/parallel/test-dtls-mtu-record-framing.mjs @@ -0,0 +1,89 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: DTLS records are sent as individual datagrams sized to the MTU. +// +// OpenSSL emits one BIO_write per DTLS record, each fragmented to fit +// SSL_set_mtu(). If the outbound BIO does not preserve those boundaries the +// whole handshake flight is read back as one blob and sent as a single +// oversized datagram, which defeats the MTU setting entirely and relies on IP +// fragmentation -- routinely dropped by NATs and middleboxes. Loopback has a +// 64 KiB MTU, so nothing else in the suite notices. +// +// A UDP relay sits between client and server so the datagrams the server +// actually puts on the wire can be measured. + +import { hasCrypto, skip } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; +import dgram from 'node:dgram'; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { connect, listen } = await import('node:dtls'); + +const cert = fixtures.readKey('agent1-cert.pem'); +const key = fixtures.readKey('agent1-key.pem'); +const ca = fixtures.readKey('ca1-cert.pem'); + +const MTU = 512; + +// Send the CA alongside the leaf so the server's flight is several times the +// MTU and has to be split across records. +const endpoint = listen(() => {}, { + cert: Buffer.concat([cert, ca]), + key, + host: '127.0.0.1', + port: 0, + mtu: MTU, +}); +const serverPort = endpoint.address.port; + +const serverToClientSizes = []; +const clientSide = dgram.createSocket('udp4'); +const serverSide = dgram.createSocket('udp4'); +let clientAddr = null; + +clientSide.on('message', (msg, rinfo) => { + clientAddr = rinfo; + serverSide.send(msg, serverPort, '127.0.0.1'); +}); +serverSide.on('message', (msg) => { + serverToClientSizes.push(msg.length); + if (clientAddr !== null) { + clientSide.send(msg, clientAddr.port, clientAddr.address); + } +}); + +await new Promise((resolve) => clientSide.bind(0, '127.0.0.1', resolve)); +await new Promise((resolve) => serverSide.bind(0, '127.0.0.1', resolve)); + +const client = connect('127.0.0.1', clientSide.address().port, { + servername: 'agent1', + ca: [ca], + mtu: MTU, +}); + +await client.opened; + +const oversized = serverToClientSizes.filter((size) => size > MTU); +assert.deepStrictEqual( + oversized, [], + `server sent datagram(s) larger than the ${MTU} byte MTU: ` + + `[${oversized}] (all: [${serverToClientSizes}])`); + +// Sanity check that the flight really did exceed one MTU, so the assertion +// above is meaningful rather than vacuous. +const total = serverToClientSizes.reduce((a, b) => a + b, 0); +assert.ok(total > MTU, + `handshake was too small to exercise fragmentation: ${total} bytes`); + +await client.close(); +await endpoint.close(); +clientSide.close(); +serverSide.close(); From 68a4a24f76ee8c364692399deb6f960a580c3239 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:34:16 +0000 Subject: [PATCH 04/81] dtls: drop empty datagrams before the accept path A zero length datagram is legal UDP, costs the sender nothing and can never carry a DTLS record, but OnRecv() forwarded it to ProcessDatagram() like any other. With no matching session it reached AcceptConnection(), which spent an SSL_new(), two BIO_new()s, a DTLSv1_listen() and an SSL_free() establishing there was nothing there -- before any address validation, so the source is spoofable. It also blocks moving enc_in_ to a datagram BIO: a zero length BIO_write enqueues an empty datagram, and the subsequent BIO_read returns 0, which the record layer reads as EOF rather than "try again". Reject len == 0 in ProcessDatagram(), covering both the session and accept paths. Signed-off-by: James M Snell Assisted-by: Opencode --- src/dtls/dtls_endpoint.cc | 8 ++++++ test/parallel/test-dtls-robustness.mjs | 34 ++++++++++++++++++++------ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/dtls/dtls_endpoint.cc b/src/dtls/dtls_endpoint.cc index 49f7fdbfcdee..6c837665dc9c 100644 --- a/src/dtls/dtls_endpoint.cc +++ b/src/dtls/dtls_endpoint.cc @@ -449,6 +449,14 @@ void DTLSEndpoint::ProcessDatagram(const uint8_t* data, const SocketAddress& remote) { if (IsHandleClosing()) return; + // An empty datagram is legal UDP but can never carry a DTLS record, and + // anyone can send one. Dropping it here keeps it out of the accept path, + // where it would otherwise cost a full SSL_new()/DTLSv1_listen()/SSL_free() + // cycle, and out of the session BIOs, where a zero length write to a + // datagram BIO queues an empty datagram whose read reports EOF rather than + // "try again". + if (len == 0) return; + // Look up existing session by remote address. auto it = sessions_.find(remote); if (it != sessions_.end()) { diff --git a/test/parallel/test-dtls-robustness.mjs b/test/parallel/test-dtls-robustness.mjs index a2bc7c541f23..23a9e3a08535 100644 --- a/test/parallel/test-dtls-robustness.mjs +++ b/test/parallel/test-dtls-robustness.mjs @@ -1,9 +1,10 @@ // Flags: --experimental-dtls --no-warnings -// Test: a listening DTLS server drops a non-DTLS (junk) datagram without -// crashing, and still accepts a real client afterwards. +// Test: a listening DTLS server drops malformed datagrams without crashing, +// and still accepts a real client afterwards. import { hasCrypto, skip, mustCall } from '../common/index.mjs'; +import assert from 'node:assert'; import dgram from 'node:dgram'; import * as fixtures from '../common/fixtures.mjs'; @@ -27,14 +28,29 @@ const server = listen(mustCall((session) => { const { port } = server.address; -// Fire a datagram that is not a ClientHello at the server. +// Fire datagrams that are not a ClientHello at the server. The empty one is +// legal UDP but can never carry a DTLS record; it must be dropped before it +// reaches the accept path or a session BIO. +const junk = [ + Buffer.from('this is not a DTLS ClientHello'), + Buffer.alloc(0), + Buffer.from([22]), + Buffer.from([22, 254, 253, 0, 0]), +]; + const raw = dgram.createSocket('udp4'); -await new Promise((resolve, reject) => { - raw.send(Buffer.from('this is not a DTLS ClientHello'), port, '127.0.0.1', - (err) => (err ? reject(err) : resolve())); -}); +for (const datagram of junk) { + await new Promise((resolve, reject) => { + raw.send(datagram, port, '127.0.0.1', + (err) => (err ? reject(err) : resolve())); + }); +} await new Promise((resolve) => raw.close(resolve)); +// None of that should have produced a session. +assert.strictEqual(server.sessions.size, 0); +assert.strictEqual(server.stats.serverSessions, 0n); + // A real client still completes a handshake against the same server. const client = connect('127.0.0.1', port, { ca: [ca], @@ -43,5 +59,9 @@ const client = connect('127.0.0.1', port, { await client.opened; +// ...and is counted, which also confirms the assertions above were not +// vacuously true. +assert.strictEqual(server.stats.serverSessions, 1n); + await client.close(); await server.close(); From ebb27444d5cab383b9029258185d82c228996834 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:36:30 +0000 Subject: [PATCH 05/81] dtls: preserve datagram boundaries on the inbound BIO OpenSSL's DTLS record layer assumes a BIO read returns exactly one datagram, and clamps a read to the bytes remaining in one. enc_in_ was a byte-stream BIO, where that count means "bytes remaining in the queue", so a record header declaring a length longer than its own datagram could consume bytes belonging to the next. Not reachable today: Receive() runs Cycle() after every BIO_write, and Cycle() drains, so enc_in_ never holds more than one datagram and the clamp lands on the boundary by coincidence. The invariant is an emergent property of when Cycle() runs rather than a property of the BIO, so anything that lets two datagrams queue turns it into a silent framing desync. Use BIO_s_dgram_mem(), matching enc_out_. Signed-off-by: James M Snell Assisted-by: Opencode --- src/dtls/dtls_endpoint.cc | 7 +++---- src/dtls/dtls_session.cc | 17 +++++++---------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/dtls/dtls_endpoint.cc b/src/dtls/dtls_endpoint.cc index 6c837665dc9c..66778174c760 100644 --- a/src/dtls/dtls_endpoint.cc +++ b/src/dtls/dtls_endpoint.cc @@ -508,13 +508,12 @@ void DTLSEndpoint::AcceptConnection(const uint8_t* data, ncrypto::SSLPointer ssl(SSL_new(server_context_->ssl_ctx())); if (!ssl) return; - // `out` becomes the session's enc_out_, so it has to preserve the datagram - // boundaries OpenSSL writes records on -- see DTLSSession::Create(). - auto in = ncrypto::BIOPointer::NewMem(); + // These become the session's enc_in_/enc_out_, so both have to preserve + // datagram boundaries -- see DTLSSession::Create(). + auto in = ncrypto::BIOPointer::New(BIO_s_dgram_mem()); auto out = ncrypto::BIOPointer::New(BIO_s_dgram_mem()); if (!in || !out) return; - BIO_set_mem_eof_return(in.get(), -1); // SSL_set_bio takes ownership of both BIOs. BIO* in_raw = in.release(); BIO* out_raw = out.release(); diff --git a/src/dtls/dtls_session.cc b/src/dtls/dtls_session.cc index aa264f4f7f50..1e730c38340e 100644 --- a/src/dtls/dtls_session.cc +++ b/src/dtls/dtls_session.cc @@ -199,22 +199,19 @@ BaseObjectPtr DTLSSession::Create(Environment* env, ncrypto::SSLPointer ssl(ssl_raw); // Create memory BIOs for encrypted data I/O. - // enc_out_ must preserve datagram boundaries: OpenSSL emits one BIO_write - // per DTLS record, each sized to fit SSL_set_mtu(). A byte-stream BIO throws - // that away and lets EncOut() coalesce a whole flight into one oversized - // datagram. BIO_s_dgram_mem() returns exactly one datagram per BIO_read and - // already reports "empty" as a retry, so no BIO_set_mem_eof_return() is - // needed for it. - auto enc_in = ncrypto::BIOPointer::NewMem(); + // Both must preserve datagram boundaries. OpenSSL's DTLS record layer + // assumes a read returns exactly one datagram (see the isdtls branch of + // tls_default_read_n()), and its write path emits one BIO_write per record, + // each sized to fit SSL_set_mtu(). BIO_s_dgram_mem() honours both, already + // reports "empty" as a retry -- so no BIO_set_mem_eof_return() is needed -- + // and grows on write. + auto enc_in = ncrypto::BIOPointer::New(BIO_s_dgram_mem()); auto enc_out = ncrypto::BIOPointer::New(BIO_s_dgram_mem()); if (!enc_in || !enc_out) { THROW_ERR_CRYPTO_OPERATION_FAILED(env, "BIO_new failed"); return {}; } - // Make the BIO non-blocking. - BIO_set_mem_eof_return(enc_in.get(), -1); - // Associate BIOs with the SSL object. SSL_set_bio takes ownership. BIO* enc_in_raw = enc_in.release(); BIO* enc_out_raw = enc_out.release(); From 6645a5a00937f2d009b9976adb9a8615180fbe99 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:43:01 +0000 Subject: [PATCH 06/81] dtls: add session.authorized and session.authorizationError SSL_get_verify_result() was never called or exposed, so there was no way to inspect the verification result or apply an authorization policy: an application could only get an opaque "certificate verify failed". Add session.authorized and session.authorizationError, the latter carrying the short X509 code such as 'CERT_HAS_EXPIRED'. Route the lookup through ncrypto's verifyPeerCertificate() rather than SSL_get_verify_result() directly, because the latter reports X509_V_OK when the peer sent no certificate at all. ncrypto reports that as absent, while still allowing for PSK and resumption, which is mapped to UNABLE_TO_GET_ISSUER_CERT to match node:tls. These are meaningful when rejectUnauthorized is false: OpenSSL verifies the chain under SSL_VERIFY_NONE and simply does not abort. Signed-off-by: James M Snell Assisted-by: Opencode --- doc/api/dtls.md | 44 +++++++++++ lib/internal/dtls/dtls.js | 24 ++++++ src/dtls/dtls_session.cc | 26 +++++++ src/dtls/dtls_session.h | 1 + test/parallel/test-dtls-authorized.mjs | 100 +++++++++++++++++++++++++ 5 files changed, 195 insertions(+) create mode 100644 test/parallel/test-dtls-authorized.mjs diff --git a/doc/api/dtls.md b/doc/api/dtls.md index df0e5b50a428..83f338225527 100644 --- a/doc/api/dtls.md +++ b/doc/api/dtls.md @@ -367,6 +367,50 @@ Immediately destroys the session without sending `close_notify`. * Returns: {string|undefined} The peer's certificate in PEM format. +### `session.authorized` + + + +* Returns: {boolean} `true` if the peer presented a certificate chain that + verified against the configured certificate authorities, and, for a client, + matched the requested identity. `false` before the handshake completes. + +### `session.authorizationError` + + + +* Returns: {string|undefined} The short X509 verification error code, for + example `'CERT_HAS_EXPIRED'` or `'HOSTNAME_MISMATCH'`, or `undefined` if the + peer's chain verified. + +A peer that presented no certificate at all reports +`'UNABLE_TO_GET_ISSUER_CERT'`, so this can be used to distinguish "no +certificate" from "a certificate that failed to verify". + +The chain is verified even when `rejectUnauthorized` is `false`; the result is +simply not enforced. That makes these two properties the way to apply a custom +authorization policy: + +```mjs +import { connect } from 'node:dtls'; + +const session = connect('192.0.2.1', 4433, { + ca: [caCert], + servername: 'example.com', + rejectUnauthorized: false, +}); + +await session.opened; + +if (!session.authorized && session.authorizationError !== 'CERT_HAS_EXPIRED') { + await session.close(); +} +``` + ### `session.alpnProtocol` * Returns: {string|undefined} The negotiated ALPN protocol. diff --git a/lib/internal/dtls/dtls.js b/lib/internal/dtls/dtls.js index e6017e3e8761..1653af307217 100644 --- a/lib/internal/dtls/dtls.js +++ b/lib/internal/dtls/dtls.js @@ -240,6 +240,30 @@ class DTLSSession { return this.#handle.getServername(); } + /** + * The short X509 verification error code for the peer's certificate chain, + * e.g. `'CERT_HAS_EXPIRED'` or `'UNABLE_TO_GET_ISSUER_CERT'`, or `undefined` + * if the chain verified. A peer that presented no certificate at all reports + * `'UNABLE_TO_GET_ISSUER_CERT'` rather than verifying. + * + * Only meaningful once the handshake has completed. + * @type {string|undefined} + */ + get authorizationError() { + if (this.#handle === null) return undefined; + return this.#handle.getVerifyError(); + } + + /** + * Whether the peer presented a certificate chain that verified against the + * configured CAs. Always false before the handshake completes. + * @type {boolean} + */ + get authorized() { + if (this.#handle === null) return false; + return this.#handle.getVerifyError() === undefined; + } + get state() { return this.#state; } get stats() { return this.#stats; } get endpoint() { return this.#endpoint; } diff --git a/src/dtls/dtls_session.cc b/src/dtls/dtls_session.cc index 1e730c38340e..38fb8a53de60 100644 --- a/src/dtls/dtls_session.cc +++ b/src/dtls/dtls_session.cc @@ -150,6 +150,7 @@ Local DTLSSession::GetConstructorTemplate(Environment* env) { SetProtoMethod(isolate, tmpl, "exportKeyingMaterial", ExportKeyingMaterial); SetProtoMethod(isolate, tmpl, "getSRTPProfile", GetSRTPProfile); SetProtoMethod(isolate, tmpl, "getServername", GetServername); + SetProtoMethod(isolate, tmpl, "getVerifyError", GetVerifyError); env->set_dtls_session_constructor_template(tmpl); } @@ -179,6 +180,7 @@ void DTLSSession::RegisterExternalReferences( registry->Register(ExportKeyingMaterial); registry->Register(GetSRTPProfile); registry->Register(GetServername); + registry->Register(GetVerifyError); } BaseObjectPtr DTLSSession::Create(Environment* env, @@ -791,6 +793,30 @@ void DTLSSession::GetSRTPProfile(const FunctionCallbackInfo& args) { } } +void DTLSSession::GetVerifyError(const FunctionCallbackInfo& args) { + DTLSSession* session; + ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); + + MarkPopErrorOnReturn mark_pop_error_on_return; + + // SSL_get_verify_result() reports X509_V_OK when the peer sent no + // certificate at all, because there was nothing to find fault with. Route + // through ncrypto, which reports std::nullopt for that case (allowing for + // PSK and resumption, where the absence is legitimate) so it can be + // distinguished from a certificate that actually verified. + long verify_error = // NOLINT(runtime/int) + session->ssl_.verifyPeerCertificate().value_or( + X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT); + + // undefined means authorized; anything else is the short error code, e.g. + // "UNABLE_TO_GET_ISSUER_CERT" or "CERT_HAS_EXPIRED". + if (verify_error == X509_V_OK) return; + + const char* code = ncrypto::X509Pointer::ErrorCode(verify_error); + args.GetReturnValue().Set( + String::NewFromUtf8(session->env()->isolate(), code).ToLocalChecked()); +} + void DTLSSession::GetServername(const FunctionCallbackInfo& args) { DTLSSession* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.This()); diff --git a/src/dtls/dtls_session.h b/src/dtls/dtls_session.h index 162752b6eb4c..2abe90e4d4e3 100644 --- a/src/dtls/dtls_session.h +++ b/src/dtls/dtls_session.h @@ -128,6 +128,7 @@ class DTLSSession final : public AsyncWrap { const v8::FunctionCallbackInfo& args); static void GetSRTPProfile(const v8::FunctionCallbackInfo& args); static void GetServername(const v8::FunctionCallbackInfo& args); + static void GetVerifyError(const v8::FunctionCallbackInfo& args); public: // The core state machine pump. Processes pending OpenSSL I/O: diff --git a/test/parallel/test-dtls-authorized.mjs b/test/parallel/test-dtls-authorized.mjs new file mode 100644 index 000000000000..f0329a19c2a8 --- /dev/null +++ b/test/parallel/test-dtls-authorized.mjs @@ -0,0 +1,100 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: session.authorized and session.authorizationError report the result of +// peer certificate chain verification. +// +// OpenSSL verifies the chain even under SSL_VERIFY_NONE -- it just does not +// abort on failure -- so these stay accurate when rejectUnauthorized is false, +// which is what makes a custom authorization policy possible. + +import { hasCrypto, mustCall, skip } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { connect, listen } = await import('node:dtls'); + +const cert = fixtures.readKey('agent1-cert.pem'); +const key = fixtures.readKey('agent1-key.pem'); +const ca1 = fixtures.readKey('ca1-cert.pem'); +const ca2 = fixtures.readKey('ca2-cert.pem'); + +// --- Client side --------------------------------------------------------- + +const endpoint = listen(() => {}, { + cert, key, host: '127.0.0.1', port: 0, +}); +const { port } = endpoint.address; + +for (const [description, options, authorized, authorizationError] of [ + ['a trusted chain and matching identity verifies', + { servername: 'agent1', ca: [ca1] }, + true, undefined], + ['an untrusted chain reports the issuer failure', + { servername: 'agent1', ca: [ca2], rejectUnauthorized: false }, + false, 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY'], + ['a trusted chain with the wrong identity reports the name mismatch', + { servername: 'not-agent1', ca: [ca1], rejectUnauthorized: false }, + false, 'HOSTNAME_MISMATCH'], +]) { + const client = connect('127.0.0.1', port, options); + await client.opened; + assert.strictEqual(client.authorized, authorized, description); + assert.strictEqual(client.authorizationError, authorizationError, + description); + await client.close(); +} + +await endpoint.close(); + +// --- Server side --------------------------------------------------------- + +// A peer that sent no certificate is reported as unverified rather than +// authorized, even though OpenSSL has nothing to find fault with. +{ + const serverSession = Promise.withResolvers(); + const server = listen(mustCall((session) => { + session.onhandshake = mustCall(() => serverSession.resolve(session)); + }), { cert, key, host: '127.0.0.1', port: 0 }); + + const client = connect('127.0.0.1', server.address.port, { + servername: 'agent1', ca: [ca1], + }); + await client.opened; + + const session = await serverSession.promise; + assert.strictEqual(session.authorized, false); + assert.strictEqual(session.authorizationError, 'UNABLE_TO_GET_ISSUER_CERT'); + + await client.close(); + await server.close(); +} + +// A peer that sent a chain the server trusts is authorized. +{ + const serverSession = Promise.withResolvers(); + const server = listen(mustCall((session) => { + session.onhandshake = mustCall(() => serverSession.resolve(session)); + }), { + cert, key, ca: [ca1], requestCert: true, host: '127.0.0.1', port: 0, + }); + + const client = connect('127.0.0.1', server.address.port, { + cert, key, servername: 'agent1', ca: [ca1], + }); + await client.opened; + + const session = await serverSession.promise; + assert.strictEqual(session.authorized, true); + assert.strictEqual(session.authorizationError, undefined); + + await client.close(); + await server.close(); +} From 4a02a8f32007b8a7cd78daa99aaaddc8c2f71cb0 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:48:22 +0000 Subject: [PATCH 07/81] dtls: correct the requestCert/rejectUnauthorized matrix createContext() tested rejectUnauthorized first and requestCert only as an else-if, so { requestCert: true, rejectUnauthorized: false } set SSL_VERIFY_NONE. No CertificateRequest was sent and the server saw no peer certificate even when the client offered a valid trusted one. That combination is the node:tls idiom for "ask for a certificate and let the application decide", so code ported from node:tls lost client authentication silently. rejectUnauthorized also wrongly implied requestCert. Follow node:tls and drive the server off requestCert first: requestCert: false -> SSL_VERIFY_NONE requestCert, rejectUnauthorized -> PEER | FAIL_IF_NO_PEER_CERT requestCert, !rejectUnauthorized -> PEER and the client off rejectUnauthorized alone. The permissive verify callback is installed in exactly one case, the server that asked for a certificate but disabled rejection, because it is the only combination where OpenSSL would otherwise abort a handshake the application wants to judge. Also validate requestCert, and CHECK the arguments to setVerifyMode instead of Int32Value(...).FromJust() on an unchecked value. Signed-off-by: James M Snell Assisted-by: Opencode --- doc/api/dtls.md | 10 +- lib/internal/dtls/dtls.js | 46 ++++++-- src/dtls/dtls_context.cc | 27 ++++- test/parallel/test-dtls-request-cert.mjs | 141 +++++++++++++++++++++++ 4 files changed, 213 insertions(+), 11 deletions(-) create mode 100644 test/parallel/test-dtls-request-cert.mjs diff --git a/doc/api/dtls.md b/doc/api/dtls.md index 83f338225527..9898aa17b909 100644 --- a/doc/api/dtls.md +++ b/doc/api/dtls.md @@ -81,7 +81,14 @@ added: REPLACEME * `alpn` {string\[]|Buffer} ALPN protocol names. * `srtp` {string} Colon-separated SRTP protection profile names (e.g., `'SRTP_AES128_CM_SHA1_80:SRTP_AEAD_AES_128_GCM'`). - * `requestCert` {boolean} Request client certificate. **Default:** `false`. + * `requestCert` {boolean} Request a certificate from the client. + **Default:** `false`. + * `rejectUnauthorized` {boolean} Only has an effect together with + `requestCert`. When `true`, a client that presents no certificate, or one + that does not chain to a trusted CA, is rejected during the handshake and + receives a TLS alert. When `false`, the certificate is still requested and + verified but the handshake completes regardless, leaving the decision to + the application via [`session.authorized`][]. **Default:** `true`. * `mtu` {number} Maximum transmission unit for DTLS records. **Default:** `1200`. * Returns: {DTLSEndpoint} @@ -626,3 +633,4 @@ The minimum allowed MTU is 256 bytes. The maximum is 65535. [`DTLSEndpoint`]: #class-dtlsendpoint [`dtls.connect()`]: #dtlsconnecthost-port-options [`dtls.listen()`]: #dtlslistencallback-options +[`session.authorized`]: #sessionauthorized diff --git a/lib/internal/dtls/dtls.js b/lib/internal/dtls/dtls.js index 1653af307217..36b4c7d3a948 100644 --- a/lib/internal/dtls/dtls.js +++ b/lib/internal/dtls/dtls.js @@ -32,6 +32,7 @@ const { } = require('internal/errors'); const { + validateBoolean, validateFunction, validateObject, validateString, @@ -600,15 +601,44 @@ function createContext(options = kEmptyObject) { context.setSRTP(options.srtp); } - // Verification mode - if (options.rejectUnauthorized !== undefined) { - const mode = options.rejectUnauthorized ? - (SSL_VERIFY_PEER_VALUE | SSL_VERIFY_FAIL_IF_NO_PEER_CERT_VALUE) : - SSL_VERIFY_NONE_VALUE; - context.setVerifyMode(mode); - } else if (options.requestCert) { + // Verification mode. + // + // A server only asks for a client certificate when requestCert says to; + // rejectUnauthorized then decides whether a missing or unverifiable one is + // fatal. A client always verifies the server, and rejectUnauthorized decides + // whether a failure is fatal. + // + // The permissive verify callback is installed in exactly one case: a server + // that asked for a certificate but disabled rejection. That is the only + // combination where OpenSSL would otherwise abort a handshake the + // application has said it wants to judge for itself. Elsewhere OpenSSL keeps + // enforcing, so a rejected peer gets a proper alert rather than a silently + // dropped session, and no state is held for a peer that is about to be + // turned away. + // + // A client under SSL_VERIFY_NONE still has its chain verified -- OpenSSL + // simply does not abort -- so session.authorized stays accurate there + // without any callback. + const rejectUnauthorized = options.rejectUnauthorized !== false; + if (options.requestCert !== undefined) { + validateBoolean(options.requestCert, 'options.requestCert'); + } + + if (isServer) { + if (options.requestCert) { + context.setVerifyMode( + rejectUnauthorized ? + (SSL_VERIFY_PEER_VALUE | SSL_VERIFY_FAIL_IF_NO_PEER_CERT_VALUE) : + SSL_VERIFY_PEER_VALUE, + !rejectUnauthorized); + } else { + // Nothing is requested, so there is nothing to reject. + context.setVerifyMode(SSL_VERIFY_NONE_VALUE, false); + } + } else { context.setVerifyMode( - SSL_VERIFY_PEER_VALUE | SSL_VERIFY_FAIL_IF_NO_PEER_CERT_VALUE); + rejectUnauthorized ? SSL_VERIFY_PEER_VALUE : SSL_VERIFY_NONE_VALUE, + false); } return context; diff --git a/src/dtls/dtls_context.cc b/src/dtls/dtls_context.cc index 329ba4aafe65..274f68e1a6a6 100644 --- a/src/dtls/dtls_context.cc +++ b/src/dtls/dtls_context.cc @@ -28,6 +28,7 @@ namespace node { using v8::Context; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Int32; using v8::Isolate; using v8::Local; using v8::Object; @@ -315,12 +316,34 @@ void DTLSContext::SetSRTP(const FunctionCallbackInfo& args) { } } +namespace { +// Installed only where the application has taken responsibility for the +// authorization decision itself: a server that asked for a client certificate +// but disabled rejection. Returning 1 unconditionally keeps the handshake +// going; the verification result is still recorded and remains reachable +// through SSL_get_verify_result(), which is what session.authorized reports. +// +// Everywhere else the callback is left null so OpenSSL enforces, and a peer +// that fails verification receives a proper alert. +int AllowUnauthorizedCallback(int preverify_ok, X509_STORE_CTX* ctx) { + return 1; +} +} // namespace + void DTLSContext::SetVerifyMode(const FunctionCallbackInfo& args) { DTLSContext* ctx; ASSIGN_OR_RETURN_UNWRAP(&ctx, args.This()); - int mode = args[0]->Int32Value(ctx->env()->context()).FromJust(); - SSL_CTX_set_verify(ctx->ctx_.get(), mode, nullptr); + CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsBoolean()); + + int mode = args[0].As()->Value(); + bool defer_to_application = args[1]->IsTrue(); + + SSL_CTX_set_verify( + ctx->ctx_.get(), + mode, + defer_to_application ? AllowUnauthorizedCallback : nullptr); } void DTLSContext::LoadDefaultCAs(const FunctionCallbackInfo& args) { diff --git a/test/parallel/test-dtls-request-cert.mjs b/test/parallel/test-dtls-request-cert.mjs new file mode 100644 index 000000000000..b5e581c44ebe --- /dev/null +++ b/test/parallel/test-dtls-request-cert.mjs @@ -0,0 +1,141 @@ +// Flags: --experimental-dtls --no-warnings + +// Test: the requestCert / rejectUnauthorized matrix on a DTLS server. +// +// requestCert: false -> no certificate is requested +// requestCert, rejectUnauthorized -> OpenSSL enforces, peer gets an alert +// requestCert, !rejectUnauthorized -> certificate is requested, the +// handshake completes either way, and +// the application decides using +// session.authorized +// +// The last row is the node:tls idiom for "ask for a certificate and let the +// application decide". It previously mapped to SSL_VERIFY_NONE, so no +// CertificateRequest was sent at all and the server saw no peer certificate +// even when the client offered a valid one. + +import { hasCrypto, mustNotCall, skip } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; + +if (!hasCrypto) { + skip('missing crypto'); +} + +if (!process.features.dtls) { + skip('DTLS is not enabled'); +} + +const { connect, listen } = await import('node:dtls'); + +const cert = fixtures.readKey('agent1-cert.pem'); +const key = fixtures.readKey('agent1-key.pem'); +const ca1 = fixtures.readKey('ca1-cert.pem'); +const ca2 = fixtures.readKey('ca2-cert.pem'); + +// Runs one handshake and reports what the server made of the client. +async function handshake(serverOptions, clientOptions) { + const gotSession = Promise.withResolvers(); + const server = listen((session) => { + session.onhandshake = () => gotSession.resolve(session); + }, { cert, key, host: '127.0.0.1', port: 0, ...serverOptions }); + + const client = connect('127.0.0.1', server.address.port, { + servername: 'agent1', ca: [ca1], ...clientOptions, + }); + + let result; + try { + await client.opened; + const session = await gotSession.promise; + result = { + rejected: false, + sawPeerCertificate: session.peerCertificate !== undefined, + authorized: session.authorized, + authorizationError: session.authorizationError, + }; + } catch (err) { + result = { rejected: true, message: err.message }; + } + + try { + await client.close(); + } catch { + // The handshake may already have torn the session down. + } + await server.close(); + return result; +} + +// --- requestCert with rejectUnauthorized disabled ------------------------ + +// A trusted client certificate is actually requested and received. This is +// the case that silently produced no certificate at all before. +assert.deepStrictEqual( + await handshake({ requestCert: true, rejectUnauthorized: false, ca: [ca1] }, + { cert, key }), + { + rejected: false, + sawPeerCertificate: true, + authorized: true, + authorizationError: undefined, + }); + +// An untrusted client certificate is received, and the handshake completes so +// the application can decide. +assert.deepStrictEqual( + await handshake({ requestCert: true, rejectUnauthorized: false, ca: [ca2] }, + { cert, key }), + { + rejected: false, + sawPeerCertificate: true, + authorized: false, + authorizationError: 'SELF_SIGNED_CERT_IN_CHAIN', + }); + +// A client that declines to send one is tolerated rather than rejected. +assert.deepStrictEqual( + await handshake({ requestCert: true, rejectUnauthorized: false, ca: [ca1] }, + {}), + { + rejected: false, + sawPeerCertificate: false, + authorized: false, + authorizationError: 'UNABLE_TO_GET_ISSUER_CERT', + }); + +// --- requestCert, enforced ----------------------------------------------- + +// OpenSSL rejects these, so the peer receives a real alert rather than having +// the session quietly dropped after the handshake. +{ + const noCert = await handshake({ requestCert: true, ca: [ca1] }, {}); + assert.strictEqual(noCert.rejected, true); + assert.match(noCert.message, /handshake failure/); + + const untrusted = await handshake({ requestCert: true, ca: [ca2] }, + { cert, key }); + assert.strictEqual(untrusted.rejected, true); + assert.match(untrusted.message, /unknown ca/); +} + +// --- no requestCert ------------------------------------------------------ + +// rejectUnauthorized alone must not turn a server into one that demands +// client certificates. +assert.deepStrictEqual( + await handshake({ rejectUnauthorized: true, ca: [ca1] }, { cert, key }), + { + rejected: false, + sawPeerCertificate: false, + authorized: false, + authorizationError: 'UNABLE_TO_GET_ISSUER_CERT', + }); + +// --- validation ---------------------------------------------------------- + +assert.throws( + () => listen(mustNotCall(), { + cert, key, host: '127.0.0.1', port: 0, requestCert: 'yes', + }), + { code: 'ERR_INVALID_ARG_TYPE' }); From 29d058e9aa2f2c9e92f4d9cc91bd4b45f405124e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 23 Aug 2026 01:54:15 +0000 Subject: [PATCH 08/81] dtls: only extract key material when something is listening SSL_CTX_set_keylog_callback() was called unconditionally, so every handshake's CLIENT_RANDOM and master secret were formatted and copied into V8 strings whether or not the application had set onkeylog -- the JS side only gated delivery. Once a secret is a JS string it is reachable from heap snapshots, core dumps and the inspector for as long as the string lives. node:tls installs its keylog callback only when a listener is attached. Match that: add a has_keylog_listener flag to the shared session state, set it from the onkeylog setter, and return from SSLKeylogCallback before touching V8 when it is clear. Registration also moves to DTLSContext, since keylog is a per-SSL_CTX setting that was being rewritten once per session. While adding a state field, pin the session state offsets with static_asserts the way the endpoint state already does. Signed-off-by: James M Snell Assisted-by: Opencode --- lib/internal/dtls/dtls.js | 4 ++++ lib/internal/dtls/state.js | 14 +++++++++++ src/dtls/dtls.cc | 1 + src/dtls/dtls.h | 1 + src/dtls/dtls_context.cc | 6 +++++ src/dtls/dtls_session.cc | 26 ++++++++++++++++++--- src/dtls/dtls_session.h | 9 +++++++- test/parallel/test-dtls-keylog.mjs | 37 +++++++++++++++++++++++++++++- 8 files changed, 93 insertions(+), 5 deletions(-) diff --git a/lib/internal/dtls/dtls.js b/lib/internal/dtls/dtls.js index 36b4c7d3a948..d8cd727dbdcd 100644 --- a/lib/internal/dtls/dtls.js +++ b/lib/internal/dtls/dtls.js @@ -157,8 +157,12 @@ class DTLSSession { if (fn !== undefined && fn !== null) { validateFunction(fn, 'onkeylog'); this.#onkeylog = FunctionPrototypeBind(fn, this); + // Tells C++ it is worth turning key material into JS strings. Without a + // listener the secrets never leave OpenSSL. + this.#state.hasKeylogListener = true; } else { this.#onkeylog = undefined; + this.#state.hasKeylogListener = false; } } diff --git a/lib/internal/dtls/state.js b/lib/internal/dtls/state.js index 5d86b556f1ad..7314ad768efd 100644 --- a/lib/internal/dtls/state.js +++ b/lib/internal/dtls/state.js @@ -43,6 +43,7 @@ const { IDX_SESSION_STATE_CLOSING, IDX_SESSION_STATE_DESTROYED, IDX_SESSION_STATE_HAS_MESSAGE_LISTENER, + IDX_SESSION_STATE_HAS_KEYLOG_LISTENER, } = internalBinding('dtls'); function isAlive(view) { @@ -119,6 +120,7 @@ class DTLSEndpointState { // uint8_t closing; // offset 2 // uint8_t destroyed; // offset 3 // uint8_t has_message_listener; // offset 4 +// uint8_t has_keylog_listener; // offset 5 class DTLSSessionState { #handle; @@ -164,6 +166,18 @@ class DTLSSessionState { DataViewPrototypeSetUint8( this.#handle, IDX_SESSION_STATE_HAS_MESSAGE_LISTENER, val ? 1 : 0); } + + get hasKeylogListener() { + if (!isAlive(this.#handle)) return false; + return DataViewPrototypeGetUint8( + this.#handle, IDX_SESSION_STATE_HAS_KEYLOG_LISTENER) === 1; + } + + set hasKeylogListener(val) { + if (!isAlive(this.#handle)) return; + DataViewPrototypeSetUint8( + this.#handle, IDX_SESSION_STATE_HAS_KEYLOG_LISTENER, val ? 1 : 0); + } } module.exports = { diff --git a/src/dtls/dtls.cc b/src/dtls/dtls.cc index 288d317dfa14..cc704eefd2ac 100644 --- a/src/dtls/dtls.cc +++ b/src/dtls/dtls.cc @@ -47,6 +47,7 @@ void CreatePerContextProperties(Local target, NODE_DEFINE_CONSTANT(target, IDX_SESSION_STATE_CLOSING); NODE_DEFINE_CONSTANT(target, IDX_SESSION_STATE_DESTROYED); NODE_DEFINE_CONSTANT(target, IDX_SESSION_STATE_HAS_MESSAGE_LISTENER); + NODE_DEFINE_CONSTANT(target, IDX_SESSION_STATE_HAS_KEYLOG_LISTENER); // Endpoint stats indices (for BigUint64Array access from JS) #define V(name, _) IDX_STATS_ENDPOINT_##name, diff --git a/src/dtls/dtls.h b/src/dtls/dtls.h index 1faed3910e21..35aee0d4c7fb 100644 --- a/src/dtls/dtls.h +++ b/src/dtls/dtls.h @@ -78,6 +78,7 @@ enum DTLSSessionStateIndex { IDX_SESSION_STATE_CLOSING, IDX_SESSION_STATE_DESTROYED, IDX_SESSION_STATE_HAS_MESSAGE_LISTENER, + IDX_SESSION_STATE_HAS_KEYLOG_LISTENER, IDX_SESSION_STATE_COUNT }; diff --git a/src/dtls/dtls_context.cc b/src/dtls/dtls_context.cc index 274f68e1a6a6..e6323c3bd174 100644 --- a/src/dtls/dtls_context.cc +++ b/src/dtls/dtls_context.cc @@ -70,6 +70,12 @@ DTLSContext::DTLSContext(Environment* env, SSL_CTX_set_cookie_generate_cb(ctx_.get(), CookieGenerateCallback); SSL_CTX_set_cookie_verify_cb(ctx_.get(), CookieVerifyCallback); + // Keylog is a per-SSL_CTX setting, so register it once here rather than + // re-registering it from every session constructor. The callback is inert + // unless the session it resolves to has a keylog listener, so installing it + // unconditionally costs nothing and no secret reaches the JS heap uninvited. + SSL_CTX_set_keylog_callback(ctx_.get(), DTLSSession::SSLKeylogCallback); + // Store pointer to this context in the SSL_CTX app data for callbacks. SSL_CTX_set_app_data(ctx_.get(), this); } diff --git a/src/dtls/dtls_session.cc b/src/dtls/dtls_session.cc index 38fb8a53de60..bea779677523 100644 --- a/src/dtls/dtls_session.cc +++ b/src/dtls/dtls_session.cc @@ -40,6 +40,23 @@ using v8::Value; namespace dtls { +// The session state "indices" are byte offsets into DTLSSessionStateData, +// accessed from JS via a DataView. Pin them to the actual struct layout, as +// the endpoint state already does, so adding or reordering a field cannot +// silently point JS at the wrong byte. +static_assert(IDX_SESSION_STATE_HANDSHAKING == + offsetof(DTLSSessionStateData, handshaking)); +static_assert(IDX_SESSION_STATE_OPEN == offsetof(DTLSSessionStateData, open)); +static_assert(IDX_SESSION_STATE_CLOSING == + offsetof(DTLSSessionStateData, closing)); +static_assert(IDX_SESSION_STATE_DESTROYED == + offsetof(DTLSSessionStateData, destroyed)); +static_assert(IDX_SESSION_STATE_HAS_MESSAGE_LISTENER == + offsetof(DTLSSessionStateData, has_message_listener)); +static_assert(IDX_SESSION_STATE_HAS_KEYLOG_LISTENER == + offsetof(DTLSSessionStateData, has_keylog_listener)); +static_assert(IDX_SESSION_STATE_COUNT == sizeof(DTLSSessionStateData)); + namespace { // Format the OpenSSL error queue into a human readable message. // @@ -119,9 +136,6 @@ DTLSSession::DTLSSession(Environment* env, // Store this session in SSL app data for callbacks. SSL_set_app_data(ssl_.get(), this); - // Enable keylog for TLS key export (useful for Wireshark debugging). - SSL_CTX_set_keylog_callback(SSL_get_SSL_CTX(ssl_.get()), SSLKeylogCallback); - // Set the MTU on the SSL object. SSL_set_mtu(ssl_.get(), endpoint->mtu()); } @@ -581,6 +595,12 @@ void DTLSSession::SSLKeylogCallback(const SSL* ssl, const char* line) { DTLSSession* session = static_cast(SSL_get_app_data(ssl)); if (session == nullptr || session->destroyed_) return; + // `line` carries the connection's secrets. Do not copy it into the JS heap + // unless something is actually listening -- once it is a JS string it is + // reachable from heap snapshots, core dumps and the inspector for as long as + // the string lives. + if (!session->state_->has_keylog_listener) return; + HandleScope handle_scope(session->env()->isolate()); Context::Scope context_scope(session->env()->context()); diff --git a/src/dtls/dtls_session.h b/src/dtls/dtls_session.h index 2abe90e4d4e3..05ee42897411 100644 --- a/src/dtls/dtls_session.h +++ b/src/dtls/dtls_session.h @@ -30,6 +30,9 @@ struct DTLSSessionStateData { uint8_t closing = 0; uint8_t destroyed = 0; uint8_t has_message_listener = 0; + // Gates SSLKeylogCallback. Secrets are only turned into JS strings when the + // application has actually asked for them. + uint8_t has_keylog_listener = 0; }; // Stats collected for a DTLS session, backed by a BigUint64Array. @@ -148,9 +151,13 @@ class DTLSSession final : public AsyncWrap { // Update the DTLS retransmission timer based on OpenSSL's timeout. void UpdateTimer(); - // OpenSSL keylog callback. + public: + // OpenSSL keylog callback. Registered once per SSL_CTX by DTLSContext; it + // resolves the session from the SSL and does nothing unless that session has + // a keylog listener. static void SSLKeylogCallback(const SSL* ssl, const char* line); + private: // Emit a callback to JS via the endpoint's callback dispatch. v8::MaybeLocal EmitCallback(int cb_index, int argc, diff --git a/test/parallel/test-dtls-keylog.mjs b/test/parallel/test-dtls-keylog.mjs index f32c1142b4c6..224b90a46979 100644 --- a/test/parallel/test-dtls-keylog.mjs +++ b/test/parallel/test-dtls-keylog.mjs @@ -1,7 +1,8 @@ // Flags: --experimental-dtls --no-warnings // Test: the onkeylog callback delivers NSS-format key material during the -// handshake (useful for decrypting captures in Wireshark). +// handshake (useful for decrypting captures in Wireshark), and that key +// material is only extracted when something is listening for it. import { hasCrypto, skip, mustCall, mustCallAtLeast } from '../common/index.mjs'; import assert from 'node:assert'; @@ -32,6 +33,12 @@ const client = connect('127.0.0.1', server.address.port, { rejectUnauthorized: false, }); +// state.hasKeylogListener is the flag the native keylog callback reads to +// decide whether to extract key material at all. Without a listener the +// secrets stay inside OpenSSL and are never turned into JS strings, where +// they would be reachable from heap snapshots and core dumps. +assert.strictEqual(client.state.hasKeylogListener, false); + // A keylog line is "