http2: reject unsupported ALPN protocols#64493
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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
🚀 New features to boost your workflow:
|
9fbd282 to
0eee3d3
Compare
Signed-off-by: Matteo Collina <hello@matteocollina.com>
0eee3d3 to
b5408ed
Compare
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 tospdy/3.1for 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-fbfor HTTP/2, istio usesistio-h2on 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.1instead ofhttp/1.1for 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.
Fixes: #26835