quic: reuse TLS pause machinery to drop event deferral & improve 0RTT

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae

[pimterry]

With this change, the TLS suspend mechanism I introduced in #64827 becomes generic and we reuse it in QUIC. This adds new client hello structures, which hide the OpenSSL vs BoringSSL differences, and expose an interface for each handshake flow to continue, suspend or fail the handshake. While suspended they can access the details and validate/prepare according to their needs.

There's one immediate advantage here: this allows QUIC to reuse this existing mechanism to largely replace its event deferral system. In QUIC, we need to defer some handshake events (mostly relating to resumption & 0RTT, see #64132) until the handshake is sufficiently complete so that at the session event we can do some setup with the data already available, but before the actual data processing within starts.

To do this currently we have a whole event deferral mechanism and multi-step session ticket extraction & validation flow. Directly suspending the handshake reusing the existing mechanism from node:tls lets us delete that completely, do the simple thing instead, and get the same result.

This then has another medium-term benefit: as part of #63995 we will be adding dynamic attach, where the application protocol is decided dynamically on session creation, which requires deferring session ticket processing even further (because we don't even know which protocol to validate for). This mechanism gives us the structure in which to easily do that, giving us 0RTT + dynamic attach.

As part of this, this changes the internal flow of how ALPN & SNI are resolved for QUIC, and extracts ALPN helpers to share more logic between TLS/QUIC/DTLS. Instead of calculating them synchronously within the OpenSSL callbacks, we now precalculate them at the suspend point, and then the OpenSSL callbacks later just use the precalculated values.

This is intended to only create visible changes to node:quic. Though it touches node:tls & node:dtls, it's just commonizing logic and preserves all existing behaviour (including callback order and other details). In future I'd like to make some small breaking changes to both to bring these further inline, but that's tricky and we can ignore it for now.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/gyp
• @nodejs/quic

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.11%. Comparing base (f509cf1) to head (e641814).
⚠️ Report is 140 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_client_hello.cc	44.57%	43 Missing and 3 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.11% -0.03% 
==========================================
Files 751 753 +2 Lines 253585 253733 +148 Branches 47772 47818 +46 ==========================================
+ Hits 228596 228661 +65 - Misses 16228 16308 +80 - Partials 8761 8764 +3 

Files with missing lines	Coverage Δ
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
src/crypto/crypto_client_hello.h	100.00% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <ø> (-0.07%)	⬇️
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/crypto/crypto_tls.cc	78.78% <100.00%> (+0.01%)	⬆️
src/crypto/crypto_client_hello.cc	44.57% <44.57%> (ø)

... and 40 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.

[jasnell]

Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are?

[jasnell]

Ok, I had an agent summarize out the actual changes here. Let me step through them. The quoted bits here are from the agents summary:

The session event is now delivered before the TLS handshake proceeds past the ClientHello, rather than after the first packet is fully processed. No stream, datagram, keylog, origin, application, goaway or datagram-status event can precede it — this is now a protocol-level guarantee rather than a replay artifact.

I know this is the key goal here with your eventual dynamic attach goal.. essentially pause here and let the JS side set things up. My goal has been to defer calling out to JS until we absolutely need to, handling as much of the handshake as possible in C++ as we can.

I'm still not entirely sold on dynamic attach yet. We know at this point that the ALPN is either h3 or something else, and we know that h3 maps to Http3Application and something else maps to the default behavior. Why call out to JavaScript to have it select and install the application? That seems entirely unnecessary and not a performance win. But I guess that's an argument for the larger PR HTTP3/QUIC split PR.

Per-SNI-identity alpn lists are now honoured. Previously the default endpoint context's alpn was always used. (behaviour change

+1 ... this has been on my list to revisit for a while.

Keylog lines are emitted in real time rather than replayed, so their interleaving with other events changes.

Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.

session.remoteTransportParams is populated at the session event (previously undefined).

+1

Session tickets with absent or malformed application data are now rejected rather than accepted optimistically

This is good. Absent/malformed application data should mean that the ticket is ignored and a renewed ticket is returned. If we just ignore, the client may continue to just keep trying to use the bad ticket.

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

A ClientHello that changes server_name across a HelloRetryRequest is now rejected with illegal_parameter. Previously it was OpenSSL's problem.

+1

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Alert selection for SNI failure moves from the servername callback to the ClientHello callback.

+1

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Beyond that, I think this PR could definitely have been better split into multiple commits but ok.

[jasnell]

Overall, +1 but there are a few regressions to investigate.

[pimterry]

Code updated. I've replied on the inline points above, but the other remaining notes from your larger comment:

When no ALPN is configured on the server, the failure mode moves from OnSelectAlpn returning NOACK to the ClientHello callback continuing with no Application.

This one I'm not convinced about. Currently we will fail the handshake if an ALPN that the server does not recognize is presented. With this, we proceed with the handshake and likely fail later. I'd think that's a regression.

I think there's a real issue here, but your description is a misreading of the summary. The problem being highlighted is only reachable when the server is explicitly configured with no ALPN protocols (alpn: []) and then a client connects. With this PR, actual ALPN mismatches did still fail the handshake, it was just that invalid ALPN config in the server made invalid application decisions.

I've now tighten this up, with a validation to server creation, to just reject that empty ALPN list at source instead so this is unreachable. I think ALPN is now correctly strict everywhere for QUIC.

Malformed server_name extensions now read as absent (may select the wildcard identity) instead of relying on OpenSSL's parse.

Hmm.. not sure about this one. Smells bad.

Yes, that was a bug. Server name handling now distinguishes the three states (valid SNI/no SNI/malformed SNI) and handles them correctly (do SNI/use wildcard SNI/fail handshake).

Endpoint bytesReceived / packetsReceived under-count when JS closes a session inside the session callback.

This is a regression. The stats should be accurate at this point.

Agreed, now fixed.

[pimterry]

I've now added some proper benchmarks, one for pure connection setup (separately for QUIC vs H3) and one for single HTTP request time (separately for 1RTT & 0RTT). Results on my machine say no significant changes:

./node benchmark/compare.js --old ./node-old --new ./node-new --analyze -- quic
[00:18:50|% 100| 2/2 files | 60/60 runs | 4/4 configs]: Done
confidence improvement accuracy (*) (**) (***)
quic/h3-request.js n=500 mode='0rtt' +0.35 % ±1.23% ±1.64% ±2.13%
quic/h3-request.js n=500 mode='1rtt' +0.11 % ±1.04% ±1.39% ±1.80%
quic/handshake.js n=1000 concurrency=1 protocol='raw' -0.31 % ±1.35% ±1.80% ±2.36%
quic/handshake.js n=1000 concurrency=10 protocol='raw' +0.60 % ±0.98% ±1.31% ±1.70%
quic/handshake.js n=1000 concurrency=1 protocol='h3' * +0.97 % ±0.82% ±1.09% ±1.42%
quic/handshake.js n=1000 concurrency=10 protocol='h3' -0.47 % ±1.06% ±1.42% ±1.86%
-1.8% 0% +1.8%
quic/h3-request.js n=500 mode='0rtt' ░░░░░░░░░░|▓▓▓░░░░░░░░░░░░░░ +0.35% quic/h3-request.js n=500 mode='1rtt' ░░░░░░░░░░|░░░░░░░░░░░░ +0.11% quic/handshake.js n=1000 concurrency=1 protocol='raw' ░░░░░░░░░░░░░░░▓▓▓|░░░░░░░░░░░ -0.31% quic/handshake.js n=1000 concurrency=10 protocol='raw' ░░░░|▓▓▓▓▓▓░░░░░░░░░░░ +0.60% quic/handshake.js n=1000 concurrency=1 protocol='h3' |██████████░░░░░░░░░ +0.97% *
quic/handshake.js n=1000 concurrency=10 protocol='h3' ░░░░░░░░░░░░▓▓▓▓▓|░░░░░░ -0.47% Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

[nodejs-github-bot]

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

[jasnell]

oh that new --analyze is so nice ;-) ...

[pimterry]

@nodejs/quic @nodejs/crypto I think this is good to go now, just needs a final approve covering the docs & benchmark commits.

[nodejs-github-bot]

Landed in 09f5aae