Uh oh!
There was an error while loading. Please reload this page.
Drop the hand-rolled TLS client hello parser - #64827
Closed
pimterry wants to merge 2 commits into
Closed
Conversation
nodejs-github-bot
commented
Jul 29, 2026
Collaborator
Review requested:
|
This existed for 'resumeSession', which needed to do an async lookup though SSL_CTX_sess_set_get_cb is sync-only. Nowadays both OpenSSL & BoringSSL have an early ClientHello callback for suspend/resume to handle this properly, so it was redundant, in addition to being complicated and generally a bit fragile & scary. This PR switches to use the modern OpenSSL/BoringSSL mechanisms for this and drops the client hello parser & related infrastructure completely. In addition, there's a new test here, covering a fixed bug: the hello parser silently dropped fragmented hellos, which we now do handle correctly. Signed-off-by: Tim Perry <pimterry@gmail.com>
pimterryforce-pushed
the
drop-client-parser
branch
from
July 29, 2026 18:38
00c4a38 to
5875e8eComparenodejs-github-bot
commented
Jul 29, 2026
Collaborator
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
pimterry
commented
Aug 3, 2026
MemberAuthor
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
commented
Aug 3, 2026
Member
reviewing... |
Member
@pimterry worth looking into? Diffdiff --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
commented
Aug 3, 2026
MemberAuthor
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 👍. |
Both events (backed by oncertcb) could potentially write to the socket synchronously, re-entering SSL mid-handshake and breaking the connection, so we defer them just like the new 'resumeSession' behaviour. Also fixes a small bug in the error path of EmitClientHello, which now bails out more aggressively instead of resuming handshakes in a V8 teardown scenario. Co-authored-by: Filip Skokan <panva.ip@gmail.com> Signed-off-by: Tim Perry <pimterry@gmail.com>
panva
approved these changes
Aug 3, 2026
pimterryforce-pushed
the
drop-client-parser
branch
from
August 3, 2026 14:10
21e9357 to
a2cfcc4Comparepimterry
commented
Aug 3, 2026
MemberAuthor
Gah, missed the CPP autoformat, now fixed. |
panva
approved these changes
Aug 3, 2026
This comment was marked as outdated.
This comment was marked as outdated.
nodejs-github-bot
commented
Aug 4, 2026
Collaborator
nodejs-github-bot
commented
Aug 4, 2026
Collaborator
nodejs-github-bot
commented
Aug 4, 2026
Collaborator
Landed in 72768c7...d18457b |
nodejs-github-bot pushed a commit
that referenced
this pull request
Aug 4, 2026
This existed for 'resumeSession', which needed to do an async lookup though SSL_CTX_sess_set_get_cb is sync-only. Nowadays both OpenSSL & BoringSSL have an early ClientHello callback for suspend/resume to handle this properly, so it was redundant, in addition to being complicated and generally a bit fragile & scary. This PR switches to use the modern OpenSSL/BoringSSL mechanisms for this and drops the client hello parser & related infrastructure completely. In addition, there's a new test here, covering a fixed bug: the hello parser silently dropped fragmented hellos, which we now do handle correctly. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64827 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
nodejs-github-bot pushed a commit
that referenced
this pull request
Aug 4, 2026
Both events (backed by oncertcb) could potentially write to the socket synchronously, re-entering SSL mid-handshake and breaking the connection, so we defer them just like the new 'resumeSession' behaviour. Also fixes a small bug in the error path of EmitClientHello, which now bails out more aggressively instead of resuming handshakes in a V8 teardown scenario. Co-authored-by: Filip Skokan <panva.ip@gmail.com> Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64827 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
aduh95 pushed a commit
that referenced
this pull request
Aug 13, 2026
This existed for 'resumeSession', which needed to do an async lookup though SSL_CTX_sess_set_get_cb is sync-only. Nowadays both OpenSSL & BoringSSL have an early ClientHello callback for suspend/resume to handle this properly, so it was redundant, in addition to being complicated and generally a bit fragile & scary. This PR switches to use the modern OpenSSL/BoringSSL mechanisms for this and drops the client hello parser & related infrastructure completely. In addition, there's a new test here, covering a fixed bug: the hello parser silently dropped fragmented hellos, which we now do handle correctly. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64827 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
aduh95 pushed a commit
that referenced
this pull request
Aug 13, 2026
Both events (backed by oncertcb) could potentially write to the socket synchronously, re-entering SSL mid-handshake and breaking the connection, so we defer them just like the new 'resumeSession' behaviour. Also fixes a small bug in the error path of EmitClientHello, which now bails out more aggressively instead of resuming handshakes in a V8 teardown scenario. Co-authored-by: Filip Skokan <panva.ip@gmail.com> Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #64827 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_cbwhich 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.