http2: reject unsupported ALPN protocols - #64493
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #64493 +/- ##
==========================================
- Coverage 90.24% 90.24% -0.01%
==========================================
Files 741 741
Lines 241424 241694 +270
Branches 45494 45556 +62
==========================================
+ Hits 217883 218121 +238
- Misses 15085 15111 +26
- Partials 8456 8462 +6
... and 48 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
Signed-off-by: Matteo Collina <hello@matteocollina.com>
pimterry
left a comment
There was a problem hiding this comment.
The original issue was filed before Node v19, when Node would accept unknown ALPNs as false, and so this case was reachable unexpectedly with a normal HTTP/2 server. That was indeed bad.
Nowadays (as of #44031) we reject those at the TLS level, so this is only reachable for users who explicitly enable their own weird ALPNs (ALPNProtocols or ALPNCallback). Today the hax/13.37 example just fails to connect over TLS at all.
I think that invalidates the issue - it can just be closed. Any code which hits this today is clearly intending to do HTTP/2 on this connection and has opted into accepting this specific ALPN, so HTTP/2 is what we should do.
This is relevant because there's a surprising amount of weird ALPN in the wild:
- Usage of the draft versions in old clients (spdy/*, h2-*). Until 2024 kube-ctl was still upgrading to spdy/3.1 for all streaming APIs, these are surprisingly widely used.
- grpc/grpc-exp - old ALPN tokens for GRPC, now deprecated but still implemented in many places.
- Private & internal APIs: e.g. all of Facebook's current app traffic uses h2-fb for HTTP/2, istio uses istio-h2 on internal mesh traffic. There will be other private HTTP/2 APIs doing similar things in Node today.
- Workarounds for typos etc: e.g. the v19 ALPN changes broke https-proxy-agent (TooTallNate/proxy-agents#142) with Node servers, because it used http1.1 instead of http/1.1 for ALPN in millions of installs. There'll be others.
I see traffic with these frequently, especially grpc on mobile (I implemented ALPNCallback specifically to handle these). Any handling of these would be broken by this, with only quite awkward workarounds available (performServerHandshake + fully reimplement Http2Server's connection handling on top, compat API becomes completely unavailable).
Agree we should only support h2 & http/1.1 by default. I think that is the current state since v19.
Don't agree we should block users who explicitly configure alternate ALPNs on an H2 server - AFAICT there's no downside to letting people do this and it's perfectly valid, it's just unusual.
Sorry, something went wrong.
Fixes: #26835