src: fixup dtls resumption and first-flight error

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opecode

Author: jasnell

doc/api/dtls.md

@@ -580,6 +580,37 @@ skip verification while appearing to succeed. For the same reason a `session`
 that did not come from [`session.session`][] is rejected outright: nothing
 records which identity it belongs to, so it cannot be checked.
 
+### Resuming under `rejectUnauthorized`
+
+A session carries the verification result it was established with, so a session
+established with `rejectUnauthorized: false` cannot be resumed by a connection
+that asked for a verified peer. The handshake fails:
+
+```mjs
+import { connect } from 'node:dtls';
+
+// Connected without verifying anything.
+const first = connect('192.0.2.1', 5684, { rejectUnauthorized: false });
+await first.opened;
+console.log(first.authorized);         // False.
+const ticket = first.session;
+await first.close();
+
+const second = connect('192.0.2.1', 5684, {
+  rejectUnauthorized: true,
+  session: ticket,
+});
+await second.opened;                   // Rejects: verification failed.
+```
+
+The host is the same in both, so binding the session to its authenticated
+identity does not cover this on its own; what differs is whether the caller
+asked for the peer to be verified. Because a resumed handshake runs no
+verification of its own, the recorded result is re-checked once it completes,
+and a session whose peer never verified is refused wherever verification is
+required. [`session.authorized`][] and [`session.authorizationError`][] report
+the recorded result on a resumed session either way.
+
 ### Ticket keys
 
 The key that encrypts session tickets is generated at random for each context,

lib/internal/dtls/dtls.js

@@ -628,7 +628,7 @@ class DTLSEndpoint {
 
   // --- Client mode ---
 
-  [kDoConnect](context, host, port, servername, session) {
+  [kDoConnect](context, host, port, servername, session, ownsEndpoint = false) {
     // Resolve SNI and the expected peer identity here so that every caller of
     // the endpoint API -- not only the top-level dtls.connect() -- gets safe
     // defaults. The identity is always bound to the requested servername (or,
@@ -660,11 +660,21 @@ class DTLSEndpoint {
     const resume = session !== undefined ?
       unwrapSession(session, verifyHost) : undefined;
 
+    // The binding's connect() builds the session but does not start its
+    // handshake, because the first flight can fail synchronously and until
+    // the wrapper below exists there is nowhere for that failure to go: the
+    // callback dispatch reaches the session through its handle. Whether the
+    // session owns the endpoint has to be settled before the handshake too,
+    // since the error path closes the endpoint only for a session that owns
+    // it -- set afterwards, a first-flight failure left the endpoint open and
+    // the event loop with nothing to drain it.
     const sessionHandle = this.#handle.connect(
       context, host, port, sni, verifyHost, verifyIsIp, resume);
     const newSession = new DTLSSession(
       kPrivateConstructor, sessionHandle, this, verifyHost);
     this.#sessions.add(newSession);
+    newSession[kOwnsEndpoint] = ownsEndpoint;
+    sessionHandle.start();
     return newSession;
   }
 
@@ -1425,12 +1435,12 @@ function connect(host, port, options = kEmptyObject) {
   // DTLSEndpoint.connect(), which defaults both to the host argument.
   // The identity is enforced whenever the context verifies, i.e.
   // unless rejectUnauthorized is false.
-  const session = endpoint[kDoConnect](
-    context, host, port, options.servername, options.session);
-  // Mark that this session owns the endpoint so it gets closed
-  // automatically when the session closes, allowing process exit.
-  session[kOwnsEndpoint] = true;
-  return session;
+  // The session owns the endpoint, so the endpoint gets closed automatically
+  // when the session closes, allowing process exit. Passed in rather than set
+  // on the way back out because the handshake starts inside, and a handshake
+  // that fails immediately has to find the flag already set.
+  return endpoint[kDoConnect](
+    context, host, port, options.servername, options.session, true);
 }
 
 module.exports = {

src/dtls/dtls_endpoint.cc

@@ -254,9 +254,14 @@ BaseObjectPtr<DTLSSession> DTLSEndpoint::Connect(
     }
   }
 
-  // Initiate the DTLS handshake by running Cycle.
-  session->Cycle();
-
+  // The handshake is not started here. Cycle() emits the ClientHello and can
+  // report a failure doing it -- a send error, or an exception from a
+  // callback -- and there is nothing to report it to yet: the callback
+  // dispatch finds the JavaScript session through the handle, and the wrapper
+  // that attaches itself to the handle is built from the value this returns.
+  // An error from the first flight was raised against a wrapper that did not
+  // exist, dropped, and then left to look like a handshake that timed out.
+  // The wrapper calls start() once it is in place.
   return session;
 }
 
@@ -724,7 +729,7 @@ void DTLSEndpoint::AcceptConnection(const uint8_t* data,
   }
 
   // Drive the handshake forward — produces ServerHello etc.
-  session->Cycle();
+  session->Start();
 }
 
 // --- JS binding methods ---

src/dtls/dtls_session.cc

@@ -193,6 +193,7 @@ Local<FunctionTemplate> DTLSSession::GetConstructorTemplate(Environment* env) {
     SetProtoMethod(isolate, tmpl, "getSession", GetSession);
     SetProtoMethod(isolate, tmpl, "wasReused", WasReused);
     SetProtoMethod(isolate, tmpl, "getVerifyError", GetVerifyError);
+    SetProtoMethod(isolate, tmpl, "start", DoStart);
 
     env->set_dtls_session_constructor_template(tmpl);
   }
@@ -229,6 +230,7 @@ void DTLSSession::RegisterExternalReferences(
   registry->Register(GetVerifyError);
   registry->Register(GetSession);
   registry->Register(WasReused);
+  registry->Register(DoStart);
 }
 
 BaseObjectPtr<DTLSSession> DTLSSession::Create(
@@ -280,13 +282,17 @@ BaseObjectPtr<DTLSSession> DTLSSession::Create(
     SSL_set_connect_state(ssl.get());
 
     // Offer a previous session for resumption. Like SNI this has to happen
-    // before Cycle() emits the ClientHello, since the session id and ticket
+    // before Start() emits the ClientHello, since the session id and ticket
     // ride in it.
     //
     // A session that OpenSSL rejects is not an error: it falls back to a full
     // handshake, which is what an expired or unknown ticket should do. Only a
     // blob that will not parse is worth reporting, and that is the caller
     // handing over something that is not a session at all.
+    //
+    // This also copies the recorded verification result onto the connection,
+    // where a resumed handshake leaves it: PeerVerificationPassed() is what
+    // stops it standing in for verification that never ran.
     if (resume.len > 0) {
       const unsigned char* p = resume.data;
       ncrypto::SSLSessionPointer sess(d2i_SSL_SESSION(nullptr, &p, resume.len));
@@ -300,10 +306,10 @@ BaseObjectPtr<DTLSSession> DTLSSession::Create(
     }
 
     // Configure SNI and peer identity verification BEFORE the handshake
-    // starts. The caller (DTLSEndpoint::Connect) runs Cycle() immediately
-    // after Create() returns, which emits the ClientHello, so anything that
-    // must appear in that flight (SNI) has to be set here rather than via a
-    // post-construction setter.
+    // starts. Start() emits the ClientHello and is called as soon as the
+    // JavaScript wrapper is built, so anything that must appear in that
+    // flight (SNI) has to be set here rather than via a post-construction
+    // setter.
     if (servername != nullptr && servername[0] != '\0') {
       if (!SSL_set_tlsext_host_name(ssl.get(), servername)) {
         THROW_ERR_CRYPTO_OPERATION_FAILED(env,
@@ -412,6 +418,16 @@ void DTLSSession::Receive(const uint8_t* data, size_t len) {
   Cycle();
 }
 
+void DTLSSession::Start() {
+  // A datagram can arrive for a server session between the new-session emit
+  // and this call, and Receive() runs the pump. Starting again after that is
+  // harmless but pointless, and a start() reaching the binding twice should
+  // not re-enter the handshake.
+  if (started_ || destroyed_) return;
+  started_ = true;
+  Cycle();
+}
+
 void DTLSSession::Cycle() {
   if (destroyed_) return;
 
@@ -484,6 +500,19 @@ void DTLSSession::CycleInner() {
 
     // Check if handshake just completed.
     if (SSL_is_init_finished(ssl_.get()) && !handshake_complete_) {
+      // A resumed handshake carries no Certificate message, so OpenSSL runs
+      // no verification and the verify mode has nothing to abort on. What it
+      // does instead is restore the result recorded when the session was
+      // first established -- SSL_set_session() copies verify_result straight
+      // onto the connection, and both it and the peer certificate round-trip
+      // through the DER blob. A session authenticated under
+      // rejectUnauthorized: false therefore arrives here carrying its
+      // original failure alongside a handshake that succeeded, and nothing
+      // downstream re-checks it: a caller that asked for a verified peer
+      // would be handed one that never verified. node:tls checks
+      // verifyError() after a resumption for this reason.
+      if (!PeerVerificationPassed()) return;
+
       handshake_complete_ = true;
       state_->handshaking = 0;
       state_->open = 1;
@@ -625,6 +654,74 @@ bool DTLSSession::HandshakeDeadlineExpired() const {
   return uv_hrtime() / 1000000 >= handshake_deadline_;
 }
 
+long DTLSSession::PeerVerifyResult() const {  // NOLINT(runtime/int)
+  // 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, where the identity was authenticated by the key instead) so it can
+  // be distinguished from a certificate that actually verified.
+  return ssl_.verifyPeerCertificate().value_or(
+      X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT);
+}
+
+bool DTLSSession::PeerVerificationPassed() {
+  // Consulted on every completed handshake rather than only a resumed one. It
+  // costs nothing where OpenSSL already enforced -- there the handshake
+  // failed and never reached here -- and it also covers a resumption the
+  // server refused, where the result restored by SSL_set_session() stays on
+  // the connection unless the full handshake that replaced it verified
+  // something of its own.
+  //
+  // Whether a failure is fatal is decided by the verify mode OpenSSL itself
+  // was given, read back off the SSL rather than tracked beside it, so the
+  // two cannot disagree -- including after an SNI callback swapped the
+  // SSL_CTX out from under this connection.
+  //
+  // A client aborts on a bad chain whenever the mode is not SSL_VERIFY_NONE.
+  // That is the test OpenSSL itself applies when processing the server's
+  // certificate -- it validates "if any flag is set", not only for
+  // SSL_VERIFY_PEER -- and it is the whole of what rejectUnauthorized selects
+  // between.
+  //
+  // A server sets SSL_VERIFY_PEER for requestCert and adds
+  // SSL_VERIFY_FAIL_IF_NO_PEER_CERT only when rejectUnauthorized is set too:
+  // requestCert on its own installs a permissive verify callback precisely so
+  // the application can judge the certificate itself, so it is the added flag
+  // and not SSL_VERIFY_PEER that marks a failure fatal there.
+  int mode = SSL_get_verify_mode(ssl_.get());
+  bool fatal = is_server_ ? (mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) != 0
+                          : mode != SSL_VERIFY_NONE;
+  if (!fatal) return true;
+
+  // Exactly the result session.authorized reports, so what is refused here
+  // and what reads as unauthorized there cannot drift apart. It is also the
+  // predicate node:tls applies -- TLSWrap::VerifyError() is the same
+  // verifyPeerCertificate().value_or() -- so a peer authenticated by a PSK
+  // rather than a certificate passes on the strength of the cipher's auth
+  // method, as it does there.
+  //
+  // One difference from node:tls, which additionally requires a peer
+  // certificate because ncrypto reports X509_V_OK for a TLS 1.3 resumption
+  // without one: a DTLS 1.2 resumption restores the peer certificate along
+  // with the result, so there is a real result to read here. Enabling DTLS
+  // 1.3, once the bundled OpenSSL offers it, means revisiting this.
+  long verify_error = PeerVerifyResult();  // NOLINT(runtime/int)
+  if (verify_error == X509_V_OK) return true;
+
+  // enc_out_ is deliberately not flushed, unlike the alert path in
+  // CycleInner(): there is no alert queued to get out, and what is queued is
+  // this side's own Finished. The peer is left to time out, as it is for
+  // every other handshake failure that OpenSSL did not itself alert on.
+  std::string message = "Peer certificate verification failed: ";
+  message += ncrypto::X509Pointer::ErrorCode(verify_error);
+  Local<Value> str;
+  if (ToV8Value(env()->context(), message).ToLocal(&str)) {
+    Local<Value> argv[] = {str};
+    EmitCallback(DTLS_CB_SESSION_ERROR, 1, argv);
+  }
+  return false;
+}
+
 void DTLSSession::EmitHandshakeTimeout() {
   HandleScope hs(env()->isolate());
   Context::Scope cs(env()->context());
@@ -861,6 +958,12 @@ MaybeLocal<Value> DTLSSession::EmitCallback(int cb_index,
 
 // --- JS binding methods ---
 
+void DTLSSession::DoStart(const FunctionCallbackInfo<Value>& args) {
+  DTLSSession* session;
+  ASSIGN_OR_RETURN_UNWRAP(&session, args.This());
+  session->Start();
+}
+
 void DTLSSession::DoSend(const FunctionCallbackInfo<Value>& args) {
   DTLSSession* session;
   ASSIGN_OR_RETURN_UNWRAP(&session, args.This());
@@ -1114,14 +1217,7 @@ void DTLSSession::GetVerifyError(const FunctionCallbackInfo<Value>& args) {
     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);
+  long verify_error = session->PeerVerifyResult();  // NOLINT(runtime/int)
 
   // undefined means authorized; anything else is the short error code, e.g.
   // "UNABLE_TO_GET_ISSUER_CERT" or "CERT_HAS_EXPIRED".

src/dtls/dtls_session.h

@@ -144,6 +144,7 @@ class DTLSSession final : public AsyncWrap {
   static void GetSession(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void WasReused(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void GetVerifyError(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void DoStart(const v8::FunctionCallbackInfo<v8::Value>& args);
 
  public:
   // The core state machine pump. Processes pending OpenSSL I/O:
@@ -153,6 +154,14 @@ class DTLSSession final : public AsyncWrap {
   //   4. UpdateTimer() - schedule retransmit timer if needed
   void Cycle();
 
+  // Run the first flight. Separate from creation because nothing a session
+  // reports has anywhere to go until its JavaScript wrapper is in place: the
+  // callback dispatch reaches the wrapper through the handle, and a session
+  // still being constructed has no wrapper attached. The server emits its new
+  // session and then starts it; the client's wrapper calls this once it is
+  // built. Repeat calls do nothing.
+  void Start();
+
  private:
   // Read decrypted application data from OpenSSL and emit to JS.
   void ClearOut();
@@ -187,6 +196,17 @@ class DTLSSession final : public AsyncWrap {
   bool HandshakeDeadlineExpired() const;
   void EmitHandshakeTimeout();
 
+  // The peer's chain verification result, with "no certificate, and none was
+  // needed" (PSK) told apart from "a certificate that verified". Shared by
+  // session.authorizationError and the gate below so the two agree.
+  long PeerVerifyResult() const;  // NOLINT(runtime/int)
+
+  // Refuse a completed handshake whose peer was never verified, emitting the
+  // error. True to carry on. Exists for resumption, which skips verification
+  // and restores the result from the session being resumed, so the verify
+  // mode has nothing to act on.
+  bool PeerVerificationPassed();
+
   // Emit a callback to JS via the endpoint's callback dispatch.
   v8::MaybeLocal<v8::Value> EmitCallback(int cb_index,
                                          int argc,
@@ -224,6 +244,7 @@ class DTLSSession final : public AsyncWrap {
 
   SocketAddress remote_address_;
   bool is_server_;
+  bool started_ = false;
   bool handshake_complete_ = false;
   bool closed_ = false;
   bool destroyed_ = false;

test/cctest/test_environment.cc

@@ -361,12 +361,15 @@ TEST_F(EnvironmentTest, WorkerInEnvironmentWithoutSnapshot) {
   const v8::HandleScope handle_scope(isolate_);
   const Argv argv;
   Env env{handle_scope, argv};
-  CHECK_NULL(isolate_data_->snapshot_data());
-  node::LoadEnvironment(*env,
-                        "const { Worker } = require('worker_threads');"
-                        "new Worker('process.exit(0)', { eval: true });")
-      .ToLocalChecked();
-  EXPECT_EQ(node::SpinEventLoop(*env).FromJust(), 0);
+  // When using certain experimental compile options, snapshot data may
+  // not exist. Skip in that case.
+  if (isolate_data_->snapshot_data()) {
+    node::LoadEnvironment(*env,
+                          "const { Worker } = require('worker_threads');"
+                          "new Worker('process.exit(0)', { eval: true });")
+        .ToLocalChecked();
+    EXPECT_EQ(node::SpinEventLoop(*env).FromJust(), 0);
+  }
 }
 
 TEST_F(EnvironmentTest, StopFromExitHandlerDoesNotLeakIntoNextEnvironment) {