tshttpproxy: degrade gracefully when WPAD is unavailable

net/tshttpproxy/tshttpproxy.go

@@ -28,11 +28,25 @@ func InvalidateCache() {
 	mu.Lock()
 	defer mu.Unlock()
 	noProxyUntil = time.Time{}
+	epoch++
+}
+
+// CurrentEpoch returns a counter that increments on every InvalidateCache
+// call. Platform-specific sysProxyFromEnv implementations can snapshot the
+// epoch at the start of an in-flight probe and check it again before
+// committing state derived from that probe (e.g. a negative-cache backoff),
+// so that a probe issued on one network does not stomp the next network's
+// state after a link change.
+func CurrentEpoch() uint64 {
+	mu.Lock()
+	defer mu.Unlock()
+	return epoch
 }
 
 var (
 	mu           sync.Mutex
 	noProxyUntil time.Time         // if non-zero, time at which ProxyFromEnvironment should check again
+	epoch        uint64            // incremented on every InvalidateCache
 	config       *httpproxy.Config // used to create proxyFunc
 	proxyFunc    func(*url.URL) (*url.URL, error)
 )
@@ -120,6 +134,11 @@ var sysProxyFromEnv func(*http.Request) (*url.URL, error)
 // ProxyFromEnvironment is like the standard library's http.ProxyFromEnvironment
 // but additionally does OS-specific proxy lookups if the environment variables
 // alone don't specify a proxy.
+//
+// Errors from the platform-specific sysProxyFromEnv hook (e.g. WinHTTP/WPAD
+// on Windows) are intentionally dropped: a failed system-proxy lookup must
+// degrade to "no proxy, dial direct" rather than fail the outbound request.
+// Errors from the env-var-derived proxy are propagated as before.
 func ProxyFromEnvironment(req *http.Request) (*url.URL, error) {
 	localProxyFunc := getProxyFunc()
 	u, err := localProxyFunc(req.URL)
@@ -135,9 +154,9 @@ func ProxyFromEnvironment(req *http.Request) (*url.URL, error) {
 	}
 
 	if sysProxyFromEnv != nil {
-		u, err := sysProxyFromEnv(req)
-		if u != nil && err == nil {
-			return u, nil
+		// Drop the sys error explicitly; see func docs.
+		if su, serr := sysProxyFromEnv(req); su != nil && serr == nil {
+			return su, nil
 		}
 	}
 

net/tshttpproxy/tshttpproxy_test.go

@@ -4,6 +4,7 @@
 package tshttpproxy
 
 import (
+	"errors"
 	"net/http"
 	"net/url"
 	"os"
@@ -205,3 +206,78 @@ func TestSetSelfProxy(t *testing.T) {
 		})
 	}
 }
+
+// TestProxyFromEnvironment_sysProxyError verifies that when the
+// platform-specific proxy lookup (e.g. Windows WPAD via WinHTTP) returns
+// an error, ProxyFromEnvironment surfaces (nil, nil) to the caller so
+// http.Transport falls back to a direct connection rather than failing
+// the request. This is the cross-platform glue side of the
+// coder/tailscale fix for tailscale/tailscale#17055 / #10215.
+func TestProxyFromEnvironment_sysProxyError(t *testing.T) {
+	// Make sure no env-var proxy interferes with the fallthrough.
+	for _, k := range []string{"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "http_proxy", "https_proxy", "no_proxy"} {
+		if v, ok := os.LookupEnv(k); ok {
+			os.Unsetenv(k)
+			t.Cleanup(func() { os.Setenv(k, v) })
+		}
+	}
+	// Reset the package-level config/proxyFunc cache so the env-var
+	// changes above take effect.
+	mu.Lock()
+	config = nil
+	proxyFunc = nil
+	noProxyUntil = time.Time{}
+	mu.Unlock()
+	t.Cleanup(func() {
+		mu.Lock()
+		config = nil
+		proxyFunc = nil
+		noProxyUntil = time.Time{}
+		mu.Unlock()
+	})
+
+	prev := sysProxyFromEnv
+	t.Cleanup(func() { sysProxyFromEnv = prev })
+
+	var calls int
+	syntheticErr := errors.New("synthetic WPAD failure")
+	sysProxyFromEnv = func(*http.Request) (*url.URL, error) {
+		calls++
+		return nil, syntheticErr
+	}
+
+	req := &http.Request{URL: must.Get(url.Parse("https://example.com/"))}
+	got, err := ProxyFromEnvironment(req)
+	if err != nil {
+		t.Fatalf("ProxyFromEnvironment returned error %v; want nil so transport dials direct", err)
+	}
+	if got != nil {
+		t.Fatalf("ProxyFromEnvironment = %v; want nil", got)
+	}
+	if calls != 1 {
+		t.Fatalf("sysProxyFromEnv was called %d times; want 1", calls)
+	}
+
+	// Simulate the platform layer applying a backoff after a failure.
+	// While the backoff is active, ProxyFromEnvironment must not
+	// re-invoke the platform hook.
+	setNoProxyUntil(time.Minute)
+	got, err = ProxyFromEnvironment(req)
+	if err != nil || got != nil {
+		t.Fatalf("during backoff: got (%v, %v); want (nil, nil)", got, err)
+	}
+	if calls != 1 {
+		t.Fatalf("sysProxyFromEnv was called %d times during backoff; want 1", calls)
+	}
+
+	// InvalidateCache (e.g. on a netmon link change) must clear the
+	// backoff and allow a fresh platform lookup.
+	InvalidateCache()
+	got, err = ProxyFromEnvironment(req)
+	if err != nil || got != nil {
+		t.Fatalf("after InvalidateCache: got (%v, %v); want (nil, nil)", got, err)
+	}
+	if calls != 2 {
+		t.Fatalf("after InvalidateCache: sysProxyFromEnv calls = %d; want 2", calls)
+	}
+}

net/tshttpproxy/tshttpproxy_windows.go

@@ -31,9 +31,16 @@ func init() {
 	sysAuthHeader = sysAuthHeaderWindows
 }
 
+// cachedProxy holds the most recent successful WPAD result and the
+// InvalidateCache epoch it was discovered under. The timeout branch of
+// proxyFromWinHTTPOrCache reads val as a fallback when an in-flight
+// probe blows past the per-request deadline, but only when epoch
+// matches the current one so we never strand the client on a proxy
+// from a previous network.
 var cachedProxy struct {
 	sync.Mutex
-	val *url.URL
+	val   *url.URL
+	epoch uint64
 }
 
 // proxyErrorf is a rate-limited logger specifically for errors asking
@@ -52,12 +59,45 @@ var (
 	metricErrOther             = clientmetric.NewCounter("winhttp_proxy_err_other")
 )
 
+// WPAD negative-cache backoffs.
+//
+// Coder fork: previously these were 10s, which combined with
+// InvalidateCache() being called on every netmon link change meant a
+// host with no WPAD server (DHCP option 252 unset, no wpad.<domain> A
+// record) would re-issue a 5-second blocking WinHttpGetProxyForUrl call
+// for nearly every outbound HTTP request. Bumping the negative-cache
+// duration lets normal traffic flow while still re-trying WPAD on
+// every link change (via InvalidateCache, which zeros noProxyUntil).
+//
+// See tailscale/tailscale#17055 and tailscale/tailscale#10215.
+const (
+	wpadAutodetectFailedBackoff   = 5 * time.Minute
+	wpadDownloadFailedBackoff     = 5 * time.Minute
+	wpadTimeoutFailedBackoff      = 30 * time.Second
+	wpadUnknownErrorFailedBackoff = 1 * time.Minute
+	// wpadInvalidParameterFailedBackoff is intentionally long: this
+	// error is only seen on Windows 8.1 and is not transient. See
+	// tailscale/tailscale#879.
+	wpadInvalidParameterFailedBackoff = 1 * time.Hour
+)
+
+// winHTTPLookup performs the underlying WPAD lookup. It is a package
+// variable so tests can swap in a fake without exercising winhttp.dll.
+var winHTTPLookup = proxyFromWinHTTP
+
 func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) {
 	if req.URL == nil {
 		return nil, nil
 	}
 	urlStr := req.URL.String()
 
+	// Snapshot the InvalidateCache epoch before launching the probe.
+	// If a netmon link change increments the epoch while the probe is
+	// in flight, we must not commit any state derived from this probe
+	// (negative-cache backoff, cached-proxy fallback) onto the new
+	// network — that probe's answer is for the old network only.
+	startEpoch := CurrentEpoch()
+
 	ctx, cancel := context.WithTimeout(req.Context(), 5*time.Second)
 	defer cancel()
 
@@ -67,10 +107,21 @@ func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) {
 	}
 	resc := make(chan result, 1)
 	go func() {
-		proxy, err := proxyFromWinHTTP(ctx, urlStr)
+		proxy, err := winHTTPLookup(ctx, urlStr)
 		resc <- result{proxy, err}
 	}()
 
+	// setBackoff applies a negative-cache backoff only if the
+	// InvalidateCache epoch hasn't advanced since the probe started.
+	// Stale probes from a previous network must not gate fresh
+	// lookups on the current one.
+	setBackoff := func(d time.Duration) {
+		if CurrentEpoch() != startEpoch {
+			return
+		}
+		setNoProxyUntil(d)
+	}
+
 	select {
 	case res := <-resc:
 		err := res.err
@@ -82,6 +133,7 @@ func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) {
 				log.Printf("tshttpproxy: winhttp: updating cached proxy setting from %v to %v", was, now)
 			}
 			cachedProxy.val = res.proxy
+			cachedProxy.epoch = startEpoch
 			return res.proxy, nil
 		}
 
@@ -92,31 +144,47 @@ func proxyFromWinHTTPOrCache(req *http.Request) (*url.URL, error) {
 		)
 		if err == syscall.Errno(ERROR_WINHTTP_AUTODETECTION_FAILED) {
 			metricErrDetectionFailed.Add(1)
-			setNoProxyUntil(10 * time.Second)
+			setBackoff(wpadAutodetectFailedBackoff)
 			return nil, nil
 		}
 		if err == windows.ERROR_INVALID_PARAMETER {
 			metricErrInvalidParameters.Add(1)
-			// Seen on Windows 8.1. (https://github.com/tailscale/tailscale/issues/879)
-			// TODO(bradfitz): figure this out.
-			setNoProxyUntil(time.Hour)
+			setBackoff(wpadInvalidParameterFailedBackoff)
 			proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): ERROR_INVALID_PARAMETER [unexpected]", urlStr)
 			return nil, nil
 		}
 		proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): %v/%#v", urlStr, err, err)
 		if err == syscall.Errno(ERROR_WINHTTP_UNABLE_TO_DOWNLOAD_SCRIPT) {
 			metricErrDownloadScript.Add(1)
-			setNoProxyUntil(10 * time.Second)
+			setBackoff(wpadDownloadFailedBackoff)
 			return nil, nil
 		}
 		metricErrOther.Add(1)
-		return nil, err
+		// Coder fork: every error path here returns (nil, nil) so the
+		// caller dials direct rather than failing the request. See the
+		// package-level ProxyFromEnvironment doc and the const block
+		// rationale above.
+		setBackoff(wpadUnknownErrorFailedBackoff)
+		return nil, nil
 	case <-ctx.Done():
 		metricErrTimeout.Add(1)
+		// Coder fork: prefer the most recent successful WPAD result,
+		// but only if it was discovered under the current epoch. A
+		// cached proxy from a previous network would route this
+		// request through a now-unreachable host. When the cache is
+		// stale or empty, fall through to direct and apply a short
+		// backoff so in-flight WinHTTP probes don't pile up.
 		cachedProxy.Lock()
-		defer cachedProxy.Unlock()
-		proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): timeout; using cached proxy %v", urlStr, cachedProxy.val)
-		return cachedProxy.val, nil
+		fallback := cachedProxy.val
+		fallbackEpoch := cachedProxy.epoch
+		cachedProxy.Unlock()
+		if fallback != nil && fallbackEpoch == startEpoch {
+			proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): timeout; using cached proxy %v", urlStr, fallback)
+			return fallback, nil
+		}
+		setBackoff(wpadTimeoutFailedBackoff)
+		proxyErrorf("tshttpproxy: winhttp: GetProxyForURL(%q): timeout; falling back to direct", urlStr)
+		return nil, nil
 	}
 }