transport/http: restore HTTP/2 support on TLS terminated inbounds - #2529
Open
bananacocodrilo wants to merge 2 commits into
Open
transport/http: restore HTTP/2 support on TLS terminated inbounds#2529bananacocodrilo wants to merge 2 commits into
bananacocodrilo wants to merge 2 commits into
Conversation
The Go 1.26 upgrade (yarpc#2494) replaced the h2c handler wrapper and http2.ConfigureServer call with http.Server.Protocols. Those are not equivalent when the inbound terminates TLS. http.Server.Protocols decides the protocol at the connection layer: HTTP/2 over TLS is only selected when ALPN negotiated "h2", and SetUnencryptedHTTP2 only applies to connections that are not *tls.Conn. The h2c handler it replaced ran at the request layer, after TLS termination, so it detected the HTTP/2 client preface on plaintext and TLS connections alike. This also exposed a latent bug. With TLS enabled the inbound wraps its listener with muxlistener, which terminates TLS using the inbound TLS configuration rather than http.Server.TLSConfig. http2.ConfigureServer only ever added "h2" to the latter, so the server has never advertised "h2" over ALPN. That was harmless while the h2c handler caught the preface; without it, an HTTP/2 client is answered in HTTP/1.1 and fails with "http2: frame too large, note that the frame header looked like an HTTP/1.1 header". Advertise "h2" on the TLS configuration handed to the mux listener, and restore the h2c handler alongside http.Server.Protocols. "h2" is appended with the lowest ALPN preference instead of being promoted. Clients that offer both "http/1.1" and "h2" cannot be assumed to speak HTTP/2 over TLS: the HTTP/1.1 outbound of this package advertises both but always speaks HTTP/1.1, and preferring "h2" makes the server answer it in HTTP/2. "http/1.1" is added when missing so that plain HTTPS/1.1 clients are not rejected with no_application_protocol.
withHTTP2ALPN returned early whenever "h2" was already configured, so an "h2"-first configuration was left untouched. Go's TLS server returns on the first of its own protocols that the client also offers, so such a server negotiates "h2" with any client that offers both "http/1.1" and "h2". This package's HTTP/1.1 outbound is exactly such a client: it advertises both but only ever speaks HTTP/1.1, so it then sends an HTTP/1.1 request over a connection the server is serving as HTTP/2. Go's http/1.1 fallback does not cover this. It only applies when no protocol matched exactly, and here "h2" matches. Ensure "http/1.1" always outranks "h2" instead, by moving or inserting it immediately before a configured "h2" while preserving the relative order of every other protocol. Configurations where "http/1.1" already ranks higher are returned untouched. Also drop the claim that "http/1.1" is added to avoid no_application_protocol rejections. Go admits a client offering only "http/1.1" against an "h2"-only server by negotiating no protocol at all (golang/go#46310), and a client offering neither is rejected whether or not "http/1.1" is advertised. The reason to rank "http/1.1" first is ordering, not rejection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
v1.89.2regressed HTTP/2 on inbounds that have TLS enabled. An HTTP/2 client now fails against them with:Background: how the server decides between HTTP/1.1 and HTTP/2
A server only ever learns that a client wants HTTP/2 in one of two ways:
h2.PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n.Both paths have to work, and neither is under the server's control.
v1.89.2broke both of them on TLS terminated inbounds.What changed
The Go 1.26 upgrade (#2494) replaced the
h2chandler wrapper and thehttp2.ConfigureServercall withhttp.Server.Protocols:Those are not equivalent once TLS is involved, for two reasons.
1.
Protocolsdecides at the connection layer,h2cdecided at the request layer.http.Server.Protocolsbranches on the connection type: for a*tls.Conn, HTTP/2 is selected only when ALPN negotiatedh2, andSetUnencryptedHTTP2(as the name says) applies only to connections that are not a*tls.Conn. So on a TLS connection, preface detection never runs at all.The
h2chandler it replaced wrapped the handler, not the connection. It ran after TLS termination, where the preface is ordinary plaintext that happens to parse as a valid HTTP/1.1 request line, so it caught prior-knowledge clients on plaintext and TLS connections alike.2. The server has never advertised
h2over ALPN.That leaves ALPN as the only surviving path, and it was never wired up. When
tlsMode != Disabled, the inbound wraps its listener withmuxlistener, which terminates TLS itself usingi.tlsConfig, noti.server.TLSConfig.http2.ConfigureServeronly ever appendedh2to the latter, so the ALPN list actually used by the handshake was untouched. This was latent and harmless while theh2chandler caught the preface; with it gone, nothing does.With both paths dead, a TLS terminated inbound answers every client in HTTP/1.1, whatever the client asked for.
Fix
h2on the TLS configuration handed tomuxlistener(cloned, so the caller's config is not mutated). This restores the ALPN path, for clients that negotiate.h2chandler alongsidehttp.Server.Protocols, for parity with v1.89.1. This restores the preface path, for clients that dial TLS with prior-knowledge HTTP/2 and offer no ALPN at all (negotiated protocol is""). WithSetUnencryptedHTTP2(true)the stdlib already handles cleartext h2 at the connection layer, so the handler path only fires on the TLS case.http.Server.Protocolsis kept. It is correct, just insufficient for TLS.Deployments behind a proxy that does not implement ALPN are served entirely by the second bullet. Go's
negotiateALPNreturns early when either side offers no protocols, so a client sending no ALPN never reaches the preference comparison and the server's list is irrelevant to it.Note on ALPN ordering
The inbound guarantees one invariant on the list it hands to
muxlistener:http/1.1always outranksh2.Go's TLS server iterates its own protocols in order and returns on the first one the client also offers, so whichever of the two is listed first decides the protocol for every client offering both. Offering
h2does not mean a client can speak it over TLS. This package's own HTTP/1.1 outbound is exactly such a client: it dials throughhttp.Transport.DialContextreturning a*tls.Conn, so it always speaks HTTP/1.1 regardless of what ALPN negotiated, while the TLS config it is given commonly advertises["http/1.1", "h2"]. If the server answers it in HTTP/2 the connection fails (bogus greeting "POST / HTTP/1.1...", ormalformed HTTP response "\x00\x00\x1e\x04..."on the client side).Concretely:
h2, it is appended last, so it never outranks a protocol the caller asked for.h2withouthttp/1.1above it,http/1.1is moved, or added, immediately before it. Anh2-first configuration is deliberately rewritten rather than honored, because the failure it causes is silent and hits yarpc's own outbound.http/1.1already outranksh2are returned untouched, adjacent or not.The relative order of every other protocol is preserved in all cases.
This is not about handshake rejection, and an earlier revision of this PR justified adding
http/1.1on those grounds incorrectly. Go admits a client offering onlyhttp/1.1against anh2-only server by negotiating no protocol at all (golang/go#46310), and a client offering neither is rejected withno_application_protocolwhether or nothttp/1.1is advertised. The reason to rankhttp/1.1first is ordering.Test Plan
New
TestInboundHTTP2OverTLSintransport/http, covering a TLS-enabled inbound inPermissivemode. The server ALPN list is["http/1.1"]unless stated, soh2cases only pass if the inbound adds it back:NextProtosh2over ALPN[http/1.1][http/1.1][http/1.1]h2over ALPN but speaks HTTP/1.1[http/1.1]h2over ALPN but speaks HTTP/1.1[h2][h2]The fourth and fifth rows guard the downgrade that promoting
h2would introduce, the fifth specifically against anh2-first server configuration. The last row is the non-ALPN proxy case, and it is deliberately insensitive to the ordering work.TestWithHTTP2ALPNunit-tests the rewrite, including that the caller'stls.Configis not mutated (tls.Config.Clonecopies theNextProtosslice header, so the helper builds a fresh slice):nil[http/1.1, h2][http/1.1][http/1.1, h2][http/1.1, h2][myproto][myproto, http/1.1, h2][h2][http/1.1, h2][h2, http/1.1][http/1.1, h2][h2, myproto][http/1.1, h2, myproto][myproto, h2][myproto, http/1.1, h2][http/1.1, myproto, h2]Every part of the fix was verified to be load-bearing by ablation:
tls_client_negotiating_http2_over_alpnfails withremote error: tls: no application protocol.h2chandler:tls_client_using_http2_with_prior_knowledge_and_no_alpnfails withhttp2: frame too large, note that the frame header looked like an HTTP/1.1 header, the reported symptom.h2is present" shortcut: fourTestWithHTTP2ALPNcases fail, andtls_client_speaking_http1_while_advertising_h2_against_h2_only_serverfails end to end withmalformed HTTP response "\x00\x00\x1e\x04\x00\x00\x00\x00\x00...", the client choking on an HTTP/2 SETTINGS frame.tls_client_with_no_alpn_against_h2_only_serverstill passes under this ablation, confirming non-ALPN clients are unaffected by the ordering.gofmt -lis clean.Checklist
RELEASE NOTES:
Fixed: HTTP/2 is served again on TLS terminated HTTP inbounds. Since v1.89.2 such inbounds answered HTTP/2 clients in HTTP/1.1, failing them with "http2: frame too large, note that the frame header looked like an HTTP/1.1 header". The inbound now advertises
h2over ALPN on the TLS configuration used by the mux listener, ranked belowhttp/1.1so that clients advertising both are not downgraded, and again detects the HTTP/2 client preface for clients that offer no ALPN.