Merge pull request #31 from jasnell/jasnell/dtls-tests

net: additional dtls fixups and tests

Author: jasnell

src/dtls/dtls.h

@@ -58,16 +58,18 @@ void RecordTimestampStat(Stats* stats) {
   V(MESSAGES_SENT, messages_sent)                                              \
   V(RETRANSMIT_COUNT, retransmit_count)
 
-// State indices shared between C++ and JS via AliasedStruct/DataView.
+// State "indices" shared between C++ and JS via AliasedStruct/DataView. These
+// are BYTE OFFSETS into the state struct, not sequential indices: session_count
+// is a uint32, so `busy`, which follows it, sits at byte offset 8. The
+// static_asserts in dtls_endpoint.cc pin these to the actual struct layout.
 // Keep in sync with lib/internal/dtls/state.js.
 enum DTLSEndpointStateIndex {
   IDX_ENDPOINT_STATE_BOUND = 0,
-  IDX_ENDPOINT_STATE_LISTENING,
-  IDX_ENDPOINT_STATE_CLOSING,
-  IDX_ENDPOINT_STATE_DESTROYED,
-  IDX_ENDPOINT_STATE_SESSION_COUNT,
-  IDX_ENDPOINT_STATE_BUSY,
-  IDX_ENDPOINT_STATE_COUNT
+  IDX_ENDPOINT_STATE_LISTENING = 1,
+  IDX_ENDPOINT_STATE_CLOSING = 2,
+  IDX_ENDPOINT_STATE_DESTROYED = 3,
+  IDX_ENDPOINT_STATE_SESSION_COUNT = 4,
+  IDX_ENDPOINT_STATE_BUSY = 8,
 };
 
 enum DTLSSessionStateIndex {

src/dtls/dtls_endpoint.cc

@@ -19,6 +19,7 @@
 #include <openssl/err.h>
 #include <openssl/ssl.h>
 
+#include <cstddef>
 #include <cstring>
 
 namespace node {
@@ -45,6 +46,22 @@ struct SendReq {
 };
 }  // namespace
 
+// The endpoint state "indices" are byte offsets into DTLSEndpointStateData,
+// accessed from JS via a DataView. Pin them to the actual struct layout so a
+// mismatch (as once existed for `busy`, which follows a uint32) can't recur.
+static_assert(IDX_ENDPOINT_STATE_BOUND ==
+              offsetof(DTLSEndpointStateData, bound));
+static_assert(IDX_ENDPOINT_STATE_LISTENING ==
+              offsetof(DTLSEndpointStateData, listening));
+static_assert(IDX_ENDPOINT_STATE_CLOSING ==
+              offsetof(DTLSEndpointStateData, closing));
+static_assert(IDX_ENDPOINT_STATE_DESTROYED ==
+              offsetof(DTLSEndpointStateData, destroyed));
+static_assert(IDX_ENDPOINT_STATE_SESSION_COUNT ==
+              offsetof(DTLSEndpointStateData, session_count));
+static_assert(IDX_ENDPOINT_STATE_BUSY ==
+              offsetof(DTLSEndpointStateData, busy));
+
 DTLSEndpoint::DTLSEndpoint(Environment* env, Local<Object> wrap)
     : HandleWrap(env,
                  wrap,
@@ -268,6 +285,11 @@ void DTLSEndpoint::CloseGracefully() {
 
   server_context_.reset();
 
+  // Keep ourselves alive until OnClose() runs, so a garbage collection while
+  // uv_close() is in flight cannot collect the wrapper before the close is
+  // reported. Released in OnClose().
+  self_ref_ = BaseObjectPtr<DTLSEndpoint>(this);
+
   // HandleWrap::Close() calls uv_close and manages the lifecycle.
   HandleWrap::Close();
 }
@@ -293,6 +315,9 @@ void DTLSEndpoint::Destroy() {
     state_->listening = 0;
   }
 
+  // Keep ourselves alive until OnClose() (see CloseGracefully()).
+  self_ref_ = BaseObjectPtr<DTLSEndpoint>(this);
+
   HandleWrap::Close();
 }
 
@@ -402,6 +427,17 @@ void DTLSEndpoint::OnClose() {
   state_->destroyed = 1;
   DTLS_STAT_RECORD_TIMESTAMP(DTLSEndpointStats, destroyed_at);
 
+  // Release the strong self-reference taken when the close was initiated.
+  // HandleWrap::OnClose still holds its own reference for the duration of this
+  // call, so this does not free us here.
+  self_ref_.reset();
+
+  // A close initiated outside CloseGracefully()/Destroy() (e.g. an endpoint
+  // abandoned mid-construction and closed at environment teardown) takes no
+  // self-reference, so its wrapper may already be collected. There is no JS
+  // side to notify in that case; skip it rather than touch a freed wrapper.
+  if (persistent().IsEmpty()) return;
+
   Local<Function> cb = GetCallback(DTLS_CB_ENDPOINT_CLOSE);
   if (!cb.IsEmpty()) {
     Local<Value> argv[] = {};

src/dtls/dtls_endpoint.h

@@ -156,6 +156,11 @@ class DTLSEndpoint final : public HandleWrap {
   AliasedStruct<DTLSEndpointStateData> state_;
   AliasedStruct<DTLSEndpointStats> stats_;
 
+  // Strong self-reference held while a graceful close/destroy is in flight, so
+  // the wrapper is not garbage-collected before OnClose() runs and reports the
+  // close. Cleared in OnClose().
+  BaseObjectPtr<DTLSEndpoint> self_ref_;
+
   bool listening_ = false;
   uint32_t mtu_ = 1200;  // Conservative default MTU for data payload
 };

src/dtls/dtls_session.cc

@@ -391,11 +391,12 @@ void DTLSSession::ClearOut() {
         // Send our close_notify back.
         SSL_shutdown(ssl_.get());
         EncOut();
+        // Detach from the endpoint's session table before notifying JS so an
+        // observer of the close sees a consistent session count. Cycle() holds
+        // a strong reference for the duration of the pump.
+        if (auto ep = endpoint_.get()) ep->RemoveSession(remote_address_);
         Local<Value> argv[] = {};
         EmitCallback(DTLS_CB_SESSION_CLOSE, 0, argv);
-        // Drop ourselves from the endpoint's session table now that the peer
-        // has closed. Safe here: Cycle() holds a strong reference for the
-        // duration of the pump.
         Destroy();
       }
       break;
@@ -490,16 +491,20 @@ void DTLSSession::Close() {
 
   state_->open = 0;
 
+  // Detach from the endpoint's session table before notifying JS, so an
+  // observer of the close (e.g. one awaiting `closed`) sees a consistent
+  // session count. We stay alive via strong_ref, and endpoint_ remains valid
+  // for the callback below; the Destroy() that follows clears it.
+  if (auto ep = endpoint_.get()) ep->RemoveSession(remote_address_);
+
   // Notify JS.
   HandleScope handle_scope(env()->isolate());
   Context::Scope context_scope(env()->context());
   Local<Value> argv[] = {};
   EmitCallback(DTLS_CB_SESSION_CLOSE, 0, argv);
 
-  // Remove ourselves from the endpoint's session table and release resources.
-  // Without this, a gracefully-closed session would linger in the table for
-  // the life of the endpoint, leaking memory and blocking reuse of its
-  // address.
+  // Release the remaining resources. RemoveSession above already detached us,
+  // so the one inside Destroy() is a no-op.
   Destroy();
 }
 

test/parallel/test-dtls-alpn.mjs

@@ -26,7 +26,6 @@ const ca = readKey('ca1-cert.pem');
 const serverAlpnChecked = Promise.withResolvers();
 
 const endpoint = listen(mustCall(async (session) => {
-  session.onmessage = () => {};
   await session.opened;
   // Server should see the negotiated ALPN protocol.
   strictEqual(session.alpnProtocol, 'coap');
@@ -54,3 +53,33 @@ await serverAlpnChecked.promise;
 
 await session.close();
 await endpoint.close();
+
+// ALPN with no protocol in common: the handshake still completes and neither
+// peer reports a negotiated protocol.
+{
+  const gotServerSession = Promise.withResolvers();
+
+  const server = listen(mustCall((s) => gotServerSession.resolve(s)), {
+    cert: serverCert.toString(),
+    key: serverKey.toString(),
+    port: 0,
+    host: '127.0.0.1',
+    alpn: ['bar'],
+  });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    ca: [ca.toString()],
+    rejectUnauthorized: false,
+    alpn: ['foo'],
+  });
+
+  await client.opened;
+  const serverSession = await gotServerSession.promise;
+  await serverSession.opened;
+
+  strictEqual(client.alpnProtocol, undefined);
+  strictEqual(serverSession.alpnProtocol, undefined);
+
+  await client.close();
+  await server.close();
+}

test/parallel/test-dtls-ciphers.mjs

@@ -0,0 +1,79 @@
+// Flags: --experimental-dtls --no-warnings
+
+// Test: cipher and ECDH-curve selection and validation.
+
+import { hasCrypto, skip, mustCall, mustNotCall } from '../common/index.mjs';
+import assert from 'node:assert';
+import * as fixtures from '../common/fixtures.mjs';
+
+const { strictEqual, throws } = assert;
+const { readKey } = fixtures;
+
+if (!hasCrypto) {
+  skip('missing crypto');
+}
+
+if (!process.features.dtls) {
+  skip('DTLS is not enabled');
+}
+
+const { listen, connect } = await import('node:dtls');
+
+const cert = readKey('agent1-cert.pem').toString();
+const key = readKey('agent1-key.pem').toString();
+const ca = readKey('ca1-cert.pem').toString();
+
+const CIPHER = 'ECDHE-RSA-AES128-GCM-SHA256';
+
+// Case 1: a specific cipher is negotiated and reported on both peers.
+{
+  const gotServerSession = Promise.withResolvers();
+
+  const server = listen(mustCall((session) => {
+    gotServerSession.resolve(session);
+  }), { cert, key, port: 0, host: '127.0.0.1', ciphers: CIPHER });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    ca: [ca],
+    rejectUnauthorized: false,
+    ciphers: CIPHER,
+  });
+
+  await client.opened;
+  const serverSession = await gotServerSession.promise;
+  await serverSession.opened;
+
+  strictEqual(client.cipher.name, CIPHER);
+  strictEqual(serverSession.cipher.name, CIPHER);
+
+  await client.close();
+  await server.close();
+}
+
+// Case 2: an invalid cipher list is rejected.
+throws(() => listen(mustNotCall(), {
+  cert, key, port: 0, host: '127.0.0.1', ciphers: 'THIS-IS-NOT-A-CIPHER',
+}), { code: 'ERR_CRYPTO_OPERATION_FAILED' });
+
+// Case 3: a valid ECDH curve completes a handshake.
+{
+  const server = listen(mustCall(), {
+    cert, key, port: 0, host: '127.0.0.1', ecdhCurve: 'P-256',
+  });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    ca: [ca],
+    rejectUnauthorized: false,
+    ecdhCurve: 'P-256',
+  });
+
+  await client.opened;
+
+  await client.close();
+  await server.close();
+}
+
+// Case 4: an invalid ECDH curve is rejected.
+throws(() => listen(mustNotCall(), {
+  cert, key, port: 0, host: '127.0.0.1', ecdhCurve: 'not-a-curve',
+}), { code: 'ERR_CRYPTO_OPERATION_FAILED' });

test/parallel/test-dtls-client-cert.mjs

@@ -0,0 +1,73 @@
+// Flags: --experimental-dtls --no-warnings
+
+// Test: DTLS mutual authentication. A server with requestCert verifies the
+// client's certificate; a client that presents no certificate is rejected.
+
+import { hasCrypto, skip, mustCall } from '../common/index.mjs';
+import assert from 'node:assert';
+import * as fixtures from '../common/fixtures.mjs';
+
+const { ok, rejects } = assert;
+const { readKey } = fixtures;
+
+if (!hasCrypto) {
+  skip('missing crypto');
+}
+
+if (!process.features.dtls) {
+  skip('DTLS is not enabled');
+}
+
+const { listen, connect } = await import('node:dtls');
+
+const cert = readKey('agent1-cert.pem').toString();
+const key = readKey('agent1-key.pem').toString();
+const ca = readKey('ca1-cert.pem').toString();
+
+// Case 1: the client presents a certificate the server can verify.
+{
+  const gotServerSession = Promise.withResolvers();
+
+  const server = listen(mustCall((session) => {
+    gotServerSession.resolve(session);
+  }), {
+    cert, key, ca: [ca], requestCert: true, port: 0, host: '127.0.0.1',
+  });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    cert, key, ca: [ca], rejectUnauthorized: false,
+  });
+
+  await client.opened;
+  const serverSession = await gotServerSession.promise;
+  await serverSession.opened;
+
+  // The server received and verified the client's certificate.
+  const clientCert = serverSession.peerCertificate;
+  ok(clientCert);
+  ok(clientCert.includes('BEGIN CERTIFICATE'));
+
+  await client.close();
+  await server.close();
+}
+
+// Case 2: the client presents no certificate; the server requires one and
+// rejects the handshake.
+{
+  const server = listen(mustCall(), {
+    cert, key, ca: [ca], requestCert: true, port: 0, host: '127.0.0.1',
+  });
+
+  const client = connect('127.0.0.1', server.address.port, {
+    ca: [ca], rejectUnauthorized: false,
+  });
+
+  // The exact alert text varies, so assert only that the handshake is rejected.
+  await rejects(client.opened, {
+    message: /handshake failure/
+  });
+
+  // The failed client tears down its internally-owned endpoint.
+  await client.endpoint.closed;
+  await server.close();
+}

test/parallel/test-dtls-connect-error-cleanup.mjs

@@ -25,11 +25,9 @@ const cert = readKey('agent1-cert.pem').toString();
 const key = readKey('agent1-key.pem').toString();
 const ca = readKey('ca1-cert.pem').toString();
 
-const server = listen(mustCall((session) => {
-  // The client rejects the certificate mid-handshake, so this server session
-  // never opens; ignore its unsettled promise.
-  session.opened.catch(() => {});
-}), { cert, key, port: 0, host: '127.0.0.1' });
+// The client rejects the certificate mid-handshake, so this server session
+// never opens; its opened rejection is handled internally by the library.
+const server = listen(mustCall(), { cert, key, port: 0, host: '127.0.0.1' });
 
 // A servername that does not match the certificate, under rejectUnauthorized,
 // makes the client's handshake fail during verification.

test/parallel/test-dtls-destroy-in-callback.mjs

@@ -72,11 +72,6 @@ const ca = readKey('ca1-cert.pem').toString();
     rejectUnauthorized: false,
   });
 
-  // The server tears its session down the moment its handshake completes. The
-  // client's own handshake normally resolves, but guard against it rejecting
-  // (racing the teardown) so it can't surface as an unhandled rejection.
-  client.opened.catch(() => {});
-
   await destroyed.promise;
 
   await client.close();