quic: apply multiple security posture improvements

[jasnell]

Improve the security footing of the quic implementation with multiple improvements

• Document that TLS certs used with QUIC connections are best when small (established practice)
• Flip the default preferred address policy to 'ignore' (client's should opt in to switching paths)
• Improve the rate limiting for connectionless responses (e.g. stateless resets, retry, immediately close, etc; limits potential DOS vector)
• Add session creation rate limiting (limit the number of new sessions an endpoint can create per second; configurable)
• Document the rate limiting mechanisms
• Improve h3 header size limits
• Add configurable peer cert verification
• Enable net.BlockList support
• Add stream idle timeout (slow loris DOS protection)

There are still a number of ergonomic issues with the API, particularly around streams, but the security details are shaping up nicely.

@nodejs/quic

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/net
• @nodejs/quic
• @nodejs/security-wg

[mcollina]

lgtm

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.34%. Comparing base (c9562dd) to head (026785c).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63483      +/-   ##
==========================================
+ Coverage   90.30%   90.34%   +0.03%     
==========================================
  Files         730      730              
  Lines      234188   234403     +215     
  Branches    43919    43923       +4     
==========================================
+ Hits       211478   211761     +283     
+ Misses      14443    14372      -71     
- Partials     8267     8270       +3     

Files with missing lines	Coverage Δ
lib/internal/blocklist.js	91.33% <100.00%> (+0.02%)	⬆️
lib/internal/quic/quic.js	100.00% <100.00%> (ø)
lib/internal/quic/stats.js	100.00% <100.00%> (ø)
lib/internal/quic/symbols.js	100.00% <100.00%> (ø)
src/node_sockaddr-inl.h	90.09% <100.00%> (ø)
src/node_sockaddr.cc	70.00% <100.00%> (ø)
src/node_sockaddr.h	38.23% <ø> (ø)

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

[nodejs-github-bot]

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

[jasnell]

@pimterry ... I've addressed your blocking concerns. Planning to keep iterating. Going to go ahead and merge then start on the next round of improvements.

[jasnell]

@pimterry
One clarification I wanted to make, you said:

... Idle request timeouts are an HTTP concern.

I disagree here. idle requests might apply to any number of protocols. It should be a generic mechanism that can be configured and switched off entirely.

[jasnell]

Landed in c55b126...2e3daf6

[martenrichter]

May I ask, if a mechanism to support serverCertificateHashes for Webtransport can be considered? (Of course also for http2, this is a thing #49841). I post it here, as this PR also related to certificates.

[pimterry]

Going to go ahead and merge then start on the next round of improvements.

It's a bit frustrating, with a blocking review on a Friday, to see the PR merged before I can check again Monday morning. We can iterate elsewhere, but there's open questions here and it's harder to review & discuss details once they've merged.

Anyway, some notably remaining issues in the code here from a quick skim:

• If you don't specify, it defaults to localhost for hostname validation even if you're sending to another address (so connect({ address: 'example.com' }) requires a localhost certificate in response).
• This doesn't handle IP validation for certificates, only DNS

I disagree here. idle requests might apply to any number of protocols. It should be a generic mechanism that can be configured and switched off entirely.

This isn't true - every protocol I listed in that comment will require this setting to be turned off to implement fully & correctly. It's relevant for some specific kinds of individual streams (like HTTP/3 requests, absolutely) but not as a setting for all streams on a QUIC session. In fact, if you were implementing HTTP/3 manually on the QUIC API here, you'd have to turn the session-wide setting off there too (because QPACK & control streams are incompatible - every session would be killed after 30s, when the client's idle control stream timed out).

A far as I can tell, there are zero QUIC protocols that can be implemented correctly with a session-level request timeout. If 100% of use cases have to turn it off, we shouldn't provide the option, and definitely not set it by default.

A global option for HTTP/3 request streams does work great, and a per-stream opt-in option for QUIC could plausibly be useful as well for the specific streams where it can be enabled, but session-wide for generic QUIC isn't useful.