Drop the hand-rolled TLS client hello parser

[pimterry]

Our TLS code currently contains a custom parser for TLS client hellos hand-rolled in C++. This is complicated and a bit scary (which is why we have a separate fuzzer for it) and does extra work anytime it's used: OpenSSL already parses the hello, and this duplicates it, and adds checks throughout various other methods to support that. It's only required for a niche legacy feature (TLS session ids) that isn't even supported in TLS 1.3, but it makes up just under 10% of our TLS implementation (and ~15% of the TLS C++).

This parser exists because 'resumeSession' needs to support async lookups for session ids, on top of OpenSSL's SSL_CTX_sess_set_get_cb which is sync-only. We handled that by parsing out the session id ourselves, in advance, and passing through to OpenSSL when we were ready to answer synchronously.

Nowadays though (since OpenSSL 1.1.1) both OpenSSL & BoringSSL have an early ClientHello callback with suspend/resume support to handle this properly. This custom parser is redundant, and can be replaced by standard APIs in both backends.

We've previously talked about dropping this, over literally more than a decade, e.g. #1464 stopping using the parser to power SNI & OCSP, #1462 discussed dropping session ids completely to help us kill it, and #5774 trying to deprecate it away.

Nowadays it's much easier and we don't need any breaking changes at all. This PR deletes lots of code and switches to the modern mechanisms for this (separate impls for BoringSSL & OpenSSL). It drops the client hello parser & all related infrastructure, while preserving all the functionality. We could aim to drop session ids later anyway as a breaking change, but this makes them very cheap to keep in the meantime.

In addition, one of the new tests here covers a bug that this fixes en route: the hello parser silently dropped session events on fragmented hellos, which we now handle correctly instead for free. That fix should be the only externally visible change.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75299/

[codecov]

Codecov Report

❌ Patch coverage is 88.04348% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (41525ab) to head (a2cfcc4).
⚠️ Report is 116 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_tls.cc	87.95%	2 Missing and 8 partials ⚠️
src/crypto/crypto_tls.h	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64827      +/-   ##
==========================================
+ Coverage   90.16%   90.30%   +0.14%     
==========================================
  Files         746      759      +13     
  Lines      242660   247376    +4716     
  Branches    45720    46651     +931     
==========================================
+ Hits       218793   223392    +4599     
- Misses      15360    15458      +98     
- Partials     8507     8526      +19     

Files with missing lines	Coverage Δ
lib/internal/tls/wrap.js	95.15% <100.00%> (+0.04%)	⬆️
src/crypto/crypto_context.cc	72.34% <100.00%> (+0.06%)	⬆️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.h	82.35% <50.00%> (-4.32%)	⬇️
src/crypto/crypto_tls.cc	78.72% <87.95%> (+0.42%)	⬆️

... and 125 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pimterry]

Had to force push to fix docs linting, but this is otherwise green and good to go. Can I get a quick re-review @nodejs/crypto or @mcollina to land this?

[panva]

reviewing...

[panva]

@pimterry worth looking into?

Diff

diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc
index 9981704fb34..fe11a049b9c 100644
--- a/src/crypto/crypto_tls.cc
+++ b/src/crypto/crypto_tls.cc
@@ -218,32 +218,18 @@ int SSLCertCallback(SSL* s, void* arg) {
     // handshake will continue after certcb is done.
     return -1;
 
-  Environment* env = w->env();
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
   w->set_cert_cb_running();
 
-  Local<Object> info = Object::New(env->isolate());
+  // The view points into SSL-owned memory, so copy it before deferring.
+  std::string servername;
+  if (auto name = SSLPointer::GetServerName(s)) servername = *name;
 
-  auto servername = SSLPointer::GetServerName(s);
-  Local<String> servername_str =
-      !servername.has_value()
-          ? String::Empty(env->isolate())
-          : OneByteString(env->isolate(), servername.value());
-
-  Local<Value> ocsp = Boolean::New(
-      env->isolate(), SSL_get_tlsext_status_type(s) == TLSEXT_STATUSTYPE_ocsp);
+  w->ScheduleCertCb(std::move(servername),
+                    SSL_get_tlsext_status_type(s) == TLSEXT_STATUSTYPE_ocsp);
 
-  if (info->Set(env->context(), env->servername_string(), servername_str)
-          .IsNothing() ||
-      info->Set(env->context(), env->ocsp_request_string(), ocsp).IsNothing()) {
-    return 1;
-  }
-
-  Local<Value> argv[] = { info };
-  w->MakeCallback(env->oncertcb_string(), arraysize(argv), argv);
-
-  return w->is_cert_cb_running() ? -1 : 1;
+  // Suspend handshake with SSL_ERROR_WANT_X509_LOOKUP, and handshake will
+  // continue after certcb is done.
+  return -1;
 }
 
 int SelectALPNCallback(
@@ -519,9 +505,7 @@ void TLSWrap::EmitClientHello(const std::vector<unsigned char>& session_id,
                 env->tls_ticket_string(),
                 Boolean::New(env->isolate(), has_ticket))
           .IsNothing()) {
-    // Continue the handshake unresumed rather than leaving it suspended.
-    hello_answered_ = true;
-    Cycle();
+    // An exception is pending, so don't re-enter SSL or JS to resume.
     return;
   }
 
@@ -529,6 +513,41 @@ void TLSWrap::EmitClientHello(const std::vector<unsigned char>& session_id,
   MakeCallback(env->onclienthello_string(), arraysize(argv), argv);
 }
 
+// As with the ClientHello, JS must not run on the library's stack: 'oncertcb'
+// handlers synchronously call back into the handle to resume the handshake.
+void TLSWrap::ScheduleCertCb(std::string servername, bool ocsp) {
+  Debug(this, "Scheduling oncertcb");
+  BaseObjectPtr<TLSWrap> strong_ref{this};
+  env()->SetImmediate([this,
+                       strong_ref,
+                       servername = std::move(servername),
+                       ocsp](Environment* env) {
+    if (ssl_) EmitCertCb(servername, ocsp);
+  });
+}
+
+void TLSWrap::EmitCertCb(const std::string& servername, bool ocsp) {
+  Debug(this, "Emitting oncertcb");
+  Environment* env = this->env();
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
+
+  Local<Object> info = Object::New(env->isolate());
+  if (info->Set(env->context(),
+                env->servername_string(),
+                OneByteString(env->isolate(), servername))
+          .IsNothing() ||
+      info->Set(env->context(),
+                env->ocsp_request_string(),
+                Boolean::New(env->isolate(), ocsp))
+          .IsNothing()) {
+    return;
+  }
+
+  Local<Value> argv[] = {info};
+  MakeCallback(env->oncertcb_string(), arraysize(argv), argv);
+}
+
 void TLSWrap::InitSSL() {
   // Initialize SSL – OpenSSL takes ownership of these.
   enc_in_ = NodeBIO::New(env()).release();
diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h
index 61f773b9c60..a5ded339291 100644
--- a/src/crypto/crypto_tls.h
+++ b/src/crypto/crypto_tls.h
@@ -115,6 +115,9 @@ class TLSWrap : public AsyncWrap,
                           size_t session_id_len,
                           bool has_ticket);
 
+  // Schedules 'oncertcb'. The handshake stays suspended until certCbDone().
+  void ScheduleCertCb(std::string servername, bool ocsp);
+
   // Implement MemoryRetainer:
   void MemoryInfo(MemoryTracker* tracker) const override;
   SET_MEMORY_INFO_NAME(TLSWrap)
@@ -149,6 +152,7 @@ class TLSWrap : public AsyncWrap,
   void WaitForCertCb(CertCb cb, void* arg);
   void EmitClientHello(const std::vector<unsigned char>& session_id,
                        bool has_ticket);
+  void EmitCertCb(const std::string& servername, bool ocsp);
 
   TLSWrap(Environment* env,
           v8::Local<v8::Object> obj,
diff --git a/staged.diff b/staged.diff
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/test/parallel/test-tls-certcb-sync-write.js b/test/parallel/test-tls-certcb-sync-write.js
new file mode 100644
index 00000000000..caf591d669f
--- /dev/null
+++ b/test/parallel/test-tls-certcb-sync-write.js
@@ -0,0 +1,49 @@
+'use strict';
+
+// Writing to a server TLSSocket synchronously from inside an SNICallback,
+// while the handshake is still waiting on the certificate callback, must not
+// break the connection; the data must be delivered once the handshake ends.
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const net = require('net');
+const tls = require('tls');
+
+const secureContext = tls.createSecureContext({
+  key: fixtures.readKey('rsa_private.pem'),
+  cert: fixtures.readKey('rsa_cert.crt'),
+});
+
+let serverSocket;
+const server = net.createServer(common.mustCall((raw) => {
+  serverSocket = new tls.TLSSocket(raw, {
+    isServer: true,
+    secureContext,
+    SNICallback: common.mustCall((servername, callback) => {
+      assert.strictEqual(servername, 'localhost');
+      serverSocket.write('from-mid-handshake');
+      callback(null, null);
+    }),
+  });
+  serverSocket.on('error', common.mustNotCall());
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = tls.connect({
+    port: server.address().port,
+    servername: 'localhost',
+    rejectUnauthorized: false,
+  }, common.mustCall(() => {
+    client.on('data', common.mustCall((data) => {
+      assert.strictEqual(data.toString(), 'from-mid-handshake');
+      client.end();
+      server.close();
+    }));
+  }));
+  client.on('error', common.mustNotCall());
+}));

[pimterry]

Oh good find @panva! Yes, the existing SNI/OCSP callbacks have the same reentrancy issues today as the resumeSession callback is now handling here, well spotted. I've pulled in your changes to fix them as well 👍.

[pimterry]

Gah, missed the CPP autoformat, now fixed.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75451/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75455/

[nodejs-github-bot]

Landed in 72768c7...d18457b