quic: implement multiple c++ side improvements#62620
quic: implement multiple c++ side improvements#62620jasnell wants to merge 5 commits intonodejs:mainfrom
Conversation
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
Signed-off-by: James M Snell <jasnell@gmail.com> Assisted-by: Opencode:Opus 4.6
|
Review requested:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
pimterry
left a comment
There was a problem hiding this comment.
Not deeply familiar with the QUIC code (or rapidhash) yet but it makes sense overall, details look reasonable to me - just two questions about the public API surface.
| The ALPN (Application-Layer Protocol Negotiation) identifier(s). | ||
|
|
||
| For **client** sessions, this is a single string specifying the protocol | ||
| the client wants to use (e.g. `'h3'`). |
There was a problem hiding this comment.
Why do we only support one ALPN value for clients? AFAICT there's no such restriction in QUIC itself, so we will want to support multiple values for clients fairly rapidly. It seems simpler to set the signature as string[] for everybody right from the start, and it looks like it'd make the implementation simpler and more consistent across the two sides.
There was a problem hiding this comment.
The question is largely about use case... specifically, what use case will there be here the client doesn't know what protocol it wants to speak? Restricting to a single ALPN here simplifies the implementation flow and covers the majority case. But the overall api here is far from done... if there are use cases we need to cover, let's identify them.
| endpoint.setSNIContexts({ | ||
| 'api.example.com': { keys: [newApiKey], certs: [newApiCert] }, | ||
| }, { replace: true }); | ||
| ``` |
There was a problem hiding this comment.
I see a good argument for the sni object form in the options, for performance & simplicity in the common case. I'm not so sure about the dynamic update method though?
We are eventually going to need a callback here (for dynamic cert generation, wildcard certificates, on-demand cert loading, etc). I would expect most setSNIContexts use cases would be subsumed by a SNICallback option, which means this method is largely redundant, and a callback would be more consistent with all our other TLS configuration anyway. Is there a use case I'm missing where this is API is preferable?
Not really opposed if you'd prefer to do this now and punt callbacks to later anyway, just want to make sure we're not aiming to implement this to replace SNI callbacks entirely, because I don't think it'll be sufficient.
There was a problem hiding this comment.
Again, comes down to use cases. The current design keeps the implementation simple and easier to optimize. It's not immediately apparent if callbacks will definitely be needed later
Commit Queue failed- Loading data for nodejs/node/pull/62620 ✔ Done loading data for nodejs/node/pull/62620 ----------------------------------- PR info ------------------------------------ Title quic: implement multiple c++ side improvements (#62620) Author James M Snell <jasnell@gmail.com> (@jasnell) Branch jasnell:jasnell/quic-tlscontext-improvements -> nodejs:main Labels lib / src, author ready, needs-ci Commits 5 - quic: implement rapidhash for hashing improvements - quic: apply multiple TLS context improvements and SNI support - quic: support multiple ALPN negotiation - quic: fixup token verification to handle zero expiration - quic: fixup cpp linting after updates Committers 1 - James M Snell <jasnell@gmail.com> PR-URL: https://github.com/nodejs/node/pull/62620 Reviewed-By: Robert Nagy <ronagy@icloud.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/62620 Reviewed-By: Robert Nagy <ronagy@icloud.com> -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 06 Apr 2026 21:20:29 GMT ✔ Approvals: 1 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/62620#pullrequestreview-4070035275 ✘ This PR needs to wait 117 more hours to land (or 0 minutes if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-04-06T23:54:10Z: https://ci.nodejs.org/job/node-test-pull-request/72526/ - Querying data for job/node-test-pull-request/72526/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/24166549552 |
Further QUIC c++ side improvements supporting proper SNI handling, multi-ALPN negotiation on the server-side, hash performance/strength improvements, and some bug fixes.
Signed-off-by: James M Snell jasnell@gmail.com
Assisted-by: Opencode:Opus 4.6