From 5bcac4eaac2b7e90430b9d20106599b3243e7cbd Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Mon, 21 Sep 2020 14:02:58 -0700
Subject: [PATCH] net/tshttpproxy: add GetProxyForURL negative cache

Otherwise when PAC server is down, we log, and each log entry is a new
HTTP request (from logtail) and a new GetProxyForURL call, which again
logs, non-stop. This is also nicer to the WinHTTP service.

Then also hook up link change notifications to the cache to reset it
if there's a chance the network might work sooner.
---
 net/tshttpproxy/tshttpproxy.go         | 29 ++++++++++++++++++++++++++
 net/tshttpproxy/tshttpproxy_windows.go | 10 ++++++++-
 wgengine/router/ifconfig_windows.go    |  2 +-
 wgengine/userspace.go                  |  8 ++++++-
 4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/net/tshttpproxy/tshttpproxy.go b/net/tshttpproxy/tshttpproxy.go
index b00ecded0..6655fe2f6 100644
--- a/net/tshttpproxy/tshttpproxy.go
+++ b/net/tshttpproxy/tshttpproxy.go
@@ -10,14 +10,43 @@
 	"net/http"
 	"net/url"
 	"os"
+	"sync"
+	"time"
 )
 
+// InvalidateCache invalidates the package-level cache for ProxyFromEnvironment.
+//
+// It's intended to be called on network link/routing table changes.
+func InvalidateCache() {
+	mu.Lock()
+	defer mu.Unlock()
+	noProxyUntil = time.Time{}
+}
+
+var (
+	mu           sync.Mutex
+	noProxyUntil time.Time // if non-zero, time at which ProxyFromEnvironment should check again
+)
+
+func setNoProxyUntil(d time.Duration) {
+	mu.Lock()
+	defer mu.Unlock()
+	noProxyUntil = time.Now().Add(d)
+}
+
 // sysProxyFromEnv, if non-nil, specifies a platform-specific ProxyFromEnvironment
 // func to use if http.ProxyFromEnvironment doesn't return a proxy.
 // For example, WPAD PAC files on Windows.
 var sysProxyFromEnv func(*http.Request) (*url.URL, error)
 
 func ProxyFromEnvironment(req *http.Request) (*url.URL, error) {
+	mu.Lock()
+	noProxyTime := noProxyUntil
+	mu.Unlock()
+	if time.Now().Before(noProxyTime) {
+		return nil, nil
+	}
+
 	u, err := http.ProxyFromEnvironment(req)
 	if u != nil && err == nil {
 		return u, nil
diff --git a/net/tshttpproxy/tshttpproxy_windows.go b/net/tshttpproxy/tshttpproxy_windows.go
index 3c7701c0b..90f2b3f95 100644
--- a/net/tshttpproxy/tshttpproxy_windows.go
+++ b/net/tshttpproxy/tshttpproxy_windows.go
@@ -71,11 +71,19 @@ type result struct {
 		}
 
 		// See https://docs.microsoft.com/en-us/windows/win32/winhttp/error-messages
-		const ERROR_WINHTTP_AUTODETECTION_FAILED = 12180
+		const (
+			ERROR_WINHTTP_AUTODETECTION_FAILED      = 12180
+			ERROR_WINHTTP_UNABLE_TO_DOWNLOAD_SCRIPT = 12167
+		)
 		if err == syscall.Errno(ERROR_WINHTTP_AUTODETECTION_FAILED) {
+			setNoProxyUntil(10 * time.Second)
 			return nil, nil
 		}
 		log.Printf("tshttpproxy: winhttp: GetProxyForURL(%q): %v/%#v", urlStr, err, err)
+		if err == syscall.Errno(ERROR_WINHTTP_UNABLE_TO_DOWNLOAD_SCRIPT) {
+			setNoProxyUntil(10 * time.Second)
+			return nil, nil
+		}
 		return nil, err
 	case <-ctx.Done():
 		cachedProxy.Lock()
diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go
index 4c8e2e0b6..4dab02bbc 100644
--- a/wgengine/router/ifconfig_windows.go
+++ b/wgengine/router/ifconfig_windows.go
@@ -242,7 +242,6 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) error {
 		log.Printf("setPrivateNetwork: adapter %v not found after %d tries, giving up", guid, tries)
 	}()
 
-	routes := []winipcfg.RouteData{}
 	var firstGateway4 *net.IP
 	var firstGateway6 *net.IP
 	addresses := make([]*net.IPNet, len(cfg.LocalAddrs))
@@ -257,6 +256,7 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) error {
 		}
 	}
 
+	var routes []winipcfg.RouteData
 	foundDefault4 := false
 	foundDefault6 := false
 	for _, route := range cfg.Routes {
diff --git a/wgengine/userspace.go b/wgengine/userspace.go
index 276b2957b..b645b5724 100644
--- a/wgengine/userspace.go
+++ b/wgengine/userspace.go
@@ -33,6 +33,7 @@
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/net/interfaces"
 	"tailscale.com/net/tsaddr"
+	"tailscale.com/net/tshttpproxy"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/key"
 	"tailscale.com/types/logger"
@@ -225,7 +226,10 @@ func newUserspaceEngineAdvanced(conf EngineConfig) (_ Engine, reterr error) {
 	}
 	e.tundev.PreFilterOut = e.handleLocalPackets
 
-	mon, err := monitor.New(logf, func() { e.LinkChange(false) })
+	mon, err := monitor.New(logf, func() {
+		e.LinkChange(false)
+		tshttpproxy.InvalidateCache()
+	})
 	if err != nil {
 		e.tundev.Close()
 		return nil, err
@@ -349,6 +353,8 @@ func newUserspaceEngineAdvanced(conf EngineConfig) (_ Engine, reterr error) {
 	}
 	// TODO(danderson): we should delete this. It's pointless to apply
 	// a no-op settings here.
+	// TODO(bradfitz): counter-point: it tests the router implementation early
+	// to see if any part of it might fail.
 	if err := e.router.Set(nil); err != nil {
 		e.magicConn.Close()
 		e.wgdev.Close()