quic: reuse TLS pause machinery to drop event deferral & improve 0RTT - #65522
quic: reuse TLS pause machinery to drop event deferral & improve 0RTT#65522pimterry wants to merge 1 commit into
Conversation
With this change, the TLS pause mechanisms used in node:tls are now made generic, and used in QUIC to pause the handshake entirely for the session event, thereby stopping almost all events being emitted (everything except qlog) so that all the previously existing deferral machinery is unnecessary. This is useful immediately to clean up & simplify things, but also medium term because this makes the 0RTT flow significantly clearer, which will help with the upcoming dynamic attach for QUIC (which requires deferring ticket processing & 0RTT delivery until after user code). As part of this, this changes the internal flow of how ALPN & SNI are resolved. Instead of calculating them synchronously within the OpenSSL callbacks, we precalculate them at the pause, and then the OpenSSL callbacks just return the precalculated values. This is intended to only create visible changes to node:quic. Though it touches node:tls, it preserves all existing behaviour (including callback order and other details). Signed-off-by: Tim Perry <pimterry@gmail.com>
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65522 +/- ##
==========================================
- Coverage 90.14% 90.14% -0.01%
==========================================
Files 751 752 +1
Lines 253585 253692 +107
Branches 47772 47773 +1
==========================================
+ Hits 228596 228686 +90
- Misses 16228 16255 +27
+ Partials 8761 8751 -10
🚀 New features to boost your workflow:
|
|
Rather than focusing the PR description on the benefits, can you explain what the behavioral / structural differences here are? |
| // entry, of type host_name, of a sane length and free of NULs. Anything | ||
| // else reads as absent here and is rejected outright by the TLS stack a | ||
| // moment later, once it parses extensions for itself. | ||
| inline std::string_view servername() const { |
There was a problem hiding this comment.
These are rather large to be inline definitions. Would much prefer the impls to be moved out to crypto_client_hello.cc
| // moment later, once it parses extensions for itself. | ||
| inline std::string_view servername() const { | ||
| auto ext = extension(TLSEXT_TYPE_server_name); | ||
| if (!ext.has_value()) return {}; |
There was a problem hiding this comment.
hmm... this does not distinguish between not provided and zero length? Should it? e.g. by returning std::optional<std::string_view> instead.
| } | ||
|
|
||
| static inline Result Encode(ClientHelloResult result) { | ||
| #ifdef OPENSSL_IS_BORINGSSL |
There was a problem hiding this comment.
You could simplify this using defines in the same #ifdef that has using Result = to include #define ResultSuccess = ssl_select_cert_success; etc .. then you could eliminate the #ifdef here.
| // Whether protocols, a list in ALPN wire format, contains protocol. | ||
| inline bool AlpnListContains(std::span<const uint8_t> protocols, | ||
| std::string_view protocol) { | ||
| for (size_t n = 0; n < protocols.size();) { |
There was a problem hiding this comment.
Nit: I wonder if we should guard against protocols.size() being unreasonably large... someone could send a rather large payload of [1, 'a', 1, 'a', 1, 'a', ... ]. Some kind of defensive bound might be worthwhile.
| alpn_protos.size(), | ||
| in, | ||
| inlen); | ||
| auto selected = |
There was a problem hiding this comment.
Nit: The { }, { } here could use a code comment to make it a bit more readable
| // CollectSessionTicketAppData writes just the application type byte, so by | ||
| // default all there is to check is that the ticket was issued by a session | ||
| // that negotiated this same application. |
There was a problem hiding this comment.
Nit: negotiated by the same application type? or application instance? Comment is just a bit unclear.
| ? SessionTicket::AppData::Status::TICKET_USE_RENEW | ||
| : SessionTicket::AppData::Status::TICKET_USE; | ||
| if (!has_application()) [[unlikely]] { | ||
| return SessionTicket::AppData::Status::TICKET_IGNORE; |
There was a problem hiding this comment.
Hmm, should this be TICKET_RENEW?
|
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:
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.
+1 ... this has been on my list to revisit for a while.
Seems fine. We'll need to revisit keylog/qlog when all this is done anyway to ensure we're capturing everything correctly.
+1
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.
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.
+1
Hmm.. not sure about this one. Smells bad.
+1
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. |
|
Overall, +1 but there are a few regressions to investigate. |
3 participants
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
sessionevent 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.