Repository navigation
fix: honor the configured proxy in HTTP/2 and HTTP/3 code paths - #541
cloud-1104 wants to merge 2 commits into
Conversation
When an HTTPS request through a CONNECT proxy negotiates HTTP/2 via ALPN, the tunnel is pooled with persistConn.alt = t.t2. On a pool miss (dead tunnel, max concurrent streams) the h2 transport dialed the origin directly with its own dialer, ignoring Transport.Proxy and the custom dial hooks, so any proxy failure silently turned into direct traffic. The stdlib guards this spot with noDialH2RoundTripper; the type existed in internal/http2 but was never used, and its RoundTrip called the dialing RoundTrip anyway. - wrap persistConn.alt in an exported NoDialH2RoundTripper that only uses cached conns; a miss returns ErrNoCachedConn and the HTTP/1 retry loop redials through the proxy, same as net/http - forced HTTP/2 over https no longer short-circuits roundTrip: it goes through the standard flow which honors the proxy and enforces h2 during the TLS handshake (enforcement added to customTlsHandshake too). h2c with a proxy now errors instead of bypassing it. Fixes imroc#206. - HTTP/3 cannot traverse a proxy, so forced h3 with a proxy is now an explicit error, and Alt-Svc h3 upgrades are skipped when a proxy applies. Related: imroc#347, imroc#455. Regression tests included: each one fails before the fix and passes after.
|
Thanks for the detailed write-up and the regression tests — I reviewed this on a Linux machine (go1.27.0) and independently verified all three bugs. Blocking one issue found below; everything else looks correct. Verification
Blocking issue:
|
RoundTripOpt hardcoded reused=true in the OnlyCachedConn branch. Once the HTTP/2 alt RoundTripper is wrapped in NoDialH2RoundTripper, the first request over a freshly dialed connection also takes that branch, so GotConnInfo.Reused (and TraceInfo IsConnReused / IsConnWasIdle / ConnIdleTime) were wrong for every first HTTP/2 request. Compute reused with the same compare-and-swap the dialing branch uses, so a brand new connection reports false and genuinely cached ones report true.
|
Thanks for the thorough review — you're right on the root cause, and the fix is pushed as b217679. |
|
Confirmed — b217679 matches the suggested fix exactly (CAS-based Verified locally on top of b217679 (go1.27.0, Linux):
That resolves the blocking issue from my earlier review; everything else there still stands. From my side this looks good — as before, since it touches |
We route outbound HTTPS traffic through HTTP proxies, and noticed requests were occasionally leaving from the machine's real IP straight to origin servers, even though the client had a proxy configured. The tell in the logs was the error shape: plain
dial tcp <origin-ip>:443failures with noproxyconnectprefix. Digging into it, the leak is in the HTTP/2 integration, and there are three variants of it.1. A dead pooled h2 tunnel makes the next request dial the origin directly
When a proxied HTTPS connection negotiates h2 via ALPN,
dialConnregisters the tunnel in the h2 conn pool and returnspersistConn{alt: t.t2}. Later requests go throughalt.RoundTrip, i.e.RoundTripOpt{OnlyCachedConn: false}. On a pool miss — the tunnel died, or the conn hit max concurrent streams — that callsGetClientConn(req, addr, true), which dials withzeroDialer/tls.Dialerstraight to the origin authority. That dial never looks atTransport.Proxy, and it doesn't go through the custom dial hooks either.So the moment a proxy tunnel hiccups, traffic silently leaves the proxy. net/http guards this exact spot with
noDialH2RoundTripper: the alt round tripper may only use cached conns, a miss returnsErrNoCachedConn, and the HTTP/1 retry loop then dials again through the proxy. The type exists ininternal/http2but is never referenced — and itsRoundTripcalls the dialingRoundTrip, so it wouldn't have worked even if it had been wired up.2. EnableForceHTTP2 ignores the proxy (#206)
roundTripshort-circuits tot.t2.RoundTrip(req), and the h2 transport always dials the origin directly, proxy or not.3. HTTP/3 ignores the proxy (#347, #455)
QUIC is UDP and can't traverse a CONNECT or SOCKS proxy, but
EnableForceHTTP3and Alt-Svc h3 upgrades still dial the origin directly when a proxy is set.Reproduction
Each case has a regression test built around a local CONNECT proxy that counts CONNECT requests. All of them fail before the fix and pass after:
TestHTTP2ProxyNoDirectDialOnTunnelDeath: establish h2 through the proxy, kill the tunnel, then the next request must produce a second CONNECT. Before the fix it succeeds with no new CONNECT — direct dial.TestForceHTTP2ViaProxy: forced h2 must go through the proxy. Before: zero CONNECTs.TestForceHTTP2ViaProxyRejectsHTTP1Server: forced h2 against an HTTP/1.1-only server must fail after dialing through the proxy, not by bypassing it. Before: the request never reaches the proxy and fails withhttp2: unexpected ALPN protocolfrom a direct dial.TestForceHTTP3WithProxyFails: forced h3 with a proxy now fails fast withreq: HTTP3 cannot be used with a proxy. Before: it burned a full QUIC handshake timeout dialing the origin.TestHTTP3AltSvcSkippedWithProxy: an Alt-Svc h3 advertisement with a proxy configured must not trigger any QUIC traffic, verified with a plain UDP listener. Before: probe packets show up.Fix
persistConn.altis now wrapped in a realNoDialH2RoundTripper(exported frominternal/http2,RoundTripusesRoundTripOnlyCachedConn). Pool miss →ErrNoCachedConn→ the HTTP/1 retry loop redials through the proxy. Same mechanism as net/http.roundTrip; it goes through the standard flow, which honors the proxy and already enforces h2 during the TLS handshake. Also added the missing enforcement tocustomTlsHandshake(the TLS fingerprinting path), so forced h2 can't silently downgrade to h1 there. h2c stays on the h2 transport, but h2c + proxy now returns an error instead of bypassing. Fixes EnableForceHTTP2 会导致 SetProxyURL 不起作用 #206.handleAltSvc,checkAltSvc, and the cached-conn fast path) are skipped when a proxy applies — the same fallback browsers do. Related: Proxy over HTTP3 doesn't work #347, making a HTTP3 request using proxy doesnt actually use the proxy. #455.No behavior change when no proxy is configured.
go test ./...passes; the only failures here areTestSetFileandTestTraceInfo, which also fail on a clean checkout of master on this machine.