diff --git a/proxy/internal/roundtrip/multi.go b/proxy/internal/roundtrip/multi.go index 497e6191b..3604e17d2 100644 --- a/proxy/internal/roundtrip/multi.go +++ b/proxy/internal/roundtrip/multi.go @@ -53,7 +53,6 @@ func NewMultiTransport(embedded http.RoundTripper, logger *log.Logger) *MultiTra } direct := &http.Transport{ DialContext: dialWithTimeout(dialer.DialContext), - ForceAttemptHTTP2: cfg.forceAttemptHTTP2, MaxIdleConns: cfg.maxIdleConns, MaxIdleConnsPerHost: cfg.maxIdleConnsPerHost, MaxConnsPerHost: cfg.maxConnsPerHost, @@ -65,6 +64,8 @@ func NewMultiTransport(embedded http.RoundTripper, logger *log.Logger) *MultiTra ReadBufferSize: cfg.readBufferSize, DisableCompression: cfg.disableCompression, } + applyUpstreamHTTPVersion(direct, cfg.upstreamHTTPVersion) + insecure := direct.Clone() insecure.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // matches the embedded NetBird transport's per-target opt-in diff --git a/proxy/internal/roundtrip/multi_test.go b/proxy/internal/roundtrip/multi_test.go index e32e50d23..6b1229a3a 100644 --- a/proxy/internal/roundtrip/multi_test.go +++ b/proxy/internal/roundtrip/multi_test.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/http/httptest" + "os" "strings" "testing" "time" @@ -85,26 +86,31 @@ func TestMultiTransport_AppliesEnvOverridesToDirect(t *testing.T) { "env tuning must also apply to the insecure-skip-verify direct transport") } -// TestMultiTransport_ForceAttemptHTTP2 pins the protocol actually +// TestMultiTransport_UpstreamHTTPVersion pins the protocol actually // negotiated with an HTTPS upstream that offers both h2 and http/1.1. -// The direct transports dial through a custom DialContext, so net/http -// only reaches for h2 while ForceAttemptHTTP2 is set; clearing it via -// NB_PROXY_FORCE_ATTEMPT_HTTP2 must leave the request on HTTP/1.1. -func TestMultiTransport_ForceAttemptHTTP2(t *testing.T) { +// The request rides the insecure clone, so this also covers the version +// surviving http.Transport.Clone. +func TestMultiTransport_UpstreamHTTPVersion(t *testing.T) { tests := []struct { name string env string wantProto string }{ - {name: "default negotiates h2", env: "", wantProto: "HTTP/2.0"}, - {name: "opt-out stays on http/1.1", env: "false", wantProto: "HTTP/1.1"}, - {name: "explicit opt-in negotiates h2", env: "true", wantProto: "HTTP/2.0"}, + {name: "unset negotiates h2", env: "", wantProto: "HTTP/2.0"}, + {name: "auto negotiates h2", env: "auto", wantProto: "HTTP/2.0"}, + {name: "1.1 pins http/1.1", env: "1.1", wantProto: "HTTP/1.1"}, + {name: "2 negotiates h2", env: "2", wantProto: "HTTP/2.0"}, + {name: "unsupported value keeps the default", env: "http3", wantProto: "HTTP/2.0"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - if tc.env != "" { - t.Setenv(EnvForceAttemptHTTP2, tc.env) + // t.Setenv registers the restore for whatever the process + // inherited; unsetting afterwards lets the default row + // exercise a genuinely absent variable. + t.Setenv(EnvUpstreamHTTPVersion, tc.env) + if tc.env == "" { + require.NoError(t, os.Unsetenv(EnvUpstreamHTTPVersion)) } // The test server's certificate isn't in any root pool, so the @@ -128,7 +134,7 @@ func TestMultiTransport_ForceAttemptHTTP2(t *testing.T) { require.NoError(t, err) assert.Equal(t, tc.wantProto, resp.Proto, - "client-side protocol must follow %s=%q", EnvForceAttemptHTTP2, tc.env) + "client-side protocol must follow %s=%q", EnvUpstreamHTTPVersion, tc.env) assert.Equal(t, tc.wantProto, string(body), "the upstream must see the same protocol the client negotiated") }) diff --git a/proxy/internal/roundtrip/netbird.go b/proxy/internal/roundtrip/netbird.go index bc71a7a64..67e3ce039 100644 --- a/proxy/internal/roundtrip/netbird.go +++ b/proxy/internal/roundtrip/netbird.go @@ -414,7 +414,6 @@ func (n *NetBird) createClientEntry(ctx context.Context, accountID types.Account // not work with reverse proxied requests. transport := &http.Transport{ DialContext: dialWithTimeout(client.DialContext), - ForceAttemptHTTP2: n.transportCfg.forceAttemptHTTP2, MaxIdleConns: n.transportCfg.maxIdleConns, MaxIdleConnsPerHost: n.transportCfg.maxIdleConnsPerHost, MaxConnsPerHost: n.transportCfg.maxConnsPerHost, @@ -426,6 +425,7 @@ func (n *NetBird) createClientEntry(ctx context.Context, accountID types.Account ReadBufferSize: n.transportCfg.readBufferSize, DisableCompression: n.transportCfg.disableCompression, } + applyUpstreamHTTPVersion(transport, n.transportCfg.upstreamHTTPVersion) insecureTransport := transport.Clone() insecureTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec diff --git a/proxy/internal/roundtrip/transport.go b/proxy/internal/roundtrip/transport.go index 7d44b18d0..093d8b3f3 100644 --- a/proxy/internal/roundtrip/transport.go +++ b/proxy/internal/roundtrip/transport.go @@ -1,8 +1,11 @@ package roundtrip import ( + "crypto/tls" + "net/http" "os" "strconv" + "strings" "time" log "github.com/sirupsen/logrus" @@ -21,7 +24,24 @@ const ( EnvReadBufferSize = "NB_PROXY_READ_BUFFER_SIZE" EnvDisableCompression = "NB_PROXY_DISABLE_COMPRESSION" EnvMaxInflight = "NB_PROXY_MAX_INFLIGHT" - EnvForceAttemptHTTP2 = "NB_PROXY_FORCE_ATTEMPT_HTTP2" + EnvUpstreamHTTPVersion = "NB_PROXY_UPSTREAM_HTTP_VERSION" +) + +// upstreamHTTPVersion selects the HTTP version the proxy uses towards an +// upstream. The explicit values are absolute: they mean the same thing +// however the transports are dialled and whatever the default becomes, +// so operator configuration survives a change of default. +type upstreamHTTPVersion string + +const ( + // upstreamHTTPAuto leaves the choice to the proxy's own default. + // This is the only value whose meaning tracks that default. + upstreamHTTPAuto upstreamHTTPVersion = "auto" + // upstreamHTTP11 never offers h2, so the upstream sees HTTP/1.1. + upstreamHTTP11 upstreamHTTPVersion = "1.1" + // upstreamHTTP2 offers h2 in the TLS handshake. Cleartext upstreams + // stay on HTTP/1.1 regardless: the proxy speaks no h2c. + upstreamHTTP2 upstreamHTTPVersion = "2" ) // transportConfig holds tunable parameters for the per-account HTTP transport. @@ -38,13 +58,10 @@ type transportConfig struct { disableCompression bool // maxInflight limits per-backend concurrent requests. 0 means unlimited. maxInflight int - // forceAttemptHTTP2 sets http.Transport.ForceAttemptHTTP2. Both proxy - // transports dial through a custom DialContext, which makes net/http - // disable HTTP/2 unless it is forced, so this defaults to true. - // Setting it to false restores that conservative default, leaving - // HTTPS upstreams on HTTP/1.1 for backends whose h2 support is - // advertised but unusable. - forceAttemptHTTP2 bool + // upstreamHTTPVersion selects the HTTP version used towards HTTPS + // upstreams, for backends whose h2 support is advertised but + // unusable. + upstreamHTTPVersion upstreamHTTPVersion } func defaultTransportConfig() transportConfig { @@ -55,7 +72,7 @@ func defaultTransportConfig() transportConfig { idleConnTimeout: 90 * time.Second, tlsHandshakeTimeout: 10 * time.Second, expectContinueTimeout: 1 * time.Second, - forceAttemptHTTP2: true, + upstreamHTTPVersion: upstreamHTTPAuto, } } @@ -95,8 +112,8 @@ func loadTransportConfig(logger *log.Logger) transportConfig { if v, ok := envInt(EnvMaxInflight, logger); ok { cfg.maxInflight = v } - if v, ok := envBool(EnvForceAttemptHTTP2, logger); ok { - cfg.forceAttemptHTTP2 = v + if v, ok := envUpstreamHTTPVersion(EnvUpstreamHTTPVersion, logger); ok { + cfg.upstreamHTTPVersion = v } logger.WithFields(log.Fields{ @@ -111,12 +128,49 @@ func loadTransportConfig(logger *log.Logger) transportConfig { "read_buffer_size": cfg.readBufferSize, "disable_compression": cfg.disableCompression, "max_inflight": cfg.maxInflight, - "force_attempt_http2": cfg.forceAttemptHTTP2, + "upstream_http_version": cfg.upstreamHTTPVersion, }).Debug("backend transport configuration") return cfg } +// applyUpstreamHTTPVersion configures t for the requested HTTP version. +// It is the single place that decides what "auto" means, so changing the +// proxy's default only touches this function and leaves every explicit +// operator setting intact. +// +// HTTP/1.1 is pinned by clearing ForceAttemptHTTP2 and installing an +// empty TLSNextProto, which disables h2 regardless of how the transport +// is dialled. Relying on net/http's conservative default (h2 off +// whenever a custom dialer is set) would silently start negotiating h2 +// again the day a transport switches to DialTLSContext. +func applyUpstreamHTTPVersion(t *http.Transport, version upstreamHTTPVersion) { + if version == upstreamHTTP11 { + t.ForceAttemptHTTP2 = false + t.TLSNextProto = map[string]func(string, *tls.Conn) http.RoundTripper{} + return + } + t.ForceAttemptHTTP2 = true +} + +// envUpstreamHTTPVersion reads an upstream HTTP version from the +// environment. An unrecognised value warns and leaves the default in +// place rather than guessing at the operator's intent. +func envUpstreamHTTPVersion(key string, logger *log.Logger) (upstreamHTTPVersion, bool) { + s := strings.TrimSpace(os.Getenv(key)) + if s == "" { + return "", false + } + switch v := upstreamHTTPVersion(strings.ToLower(s)); v { + case upstreamHTTPAuto, upstreamHTTP11, upstreamHTTP2: + return v, true + default: + logger.Warnf("ignoring unsupported %s=%q, expected one of %q, %q, %q", + key, s, upstreamHTTPAuto, upstreamHTTP11, upstreamHTTP2) + return "", false + } +} + func envInt(key string, logger *log.Logger) (int, bool) { s := os.Getenv(key) if s == "" {