From fb392e34b5413e586681d80edf395de562057ebc Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Sun, 13 Nov 2022 09:58:47 -0800
Subject: [PATCH] net/tshttpproxy: don't ignore env-based HTTP proxies after
 system lookups fail

There was a mechanism in tshttpproxy to note that a Windows proxy
lookup failed and to stop hitting it so often. But that turns out to
fire a lot (no PAC file configured at all results in a proxy lookup),
so after the first proxy lookup, we were enabling the "omg something's
wrong, stop looking up proxies" bit for awhile, which was then also
preventing the normal Go environment-based proxy lookups from working.

This at least fixes environment-based proxies.

Plenty of other Windows-specific proxy work remains (using
WinHttpGetIEProxyConfigForCurrentUser instead of just PAC files,
ignoring certain types of errors, etc), but this should fix
the regression reported in #4811.

Updates #4811

Change-Id: I665e1891897d58e290163bda5ca51a22a017c5f9
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
---
 net/tshttpproxy/tshttpproxy.go      | 14 ++++++++-----
 net/tshttpproxy/tshttpproxy_test.go | 31 +++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/net/tshttpproxy/tshttpproxy.go b/net/tshttpproxy/tshttpproxy.go
index 1f2298e2b..990789d3b 100644
--- a/net/tshttpproxy/tshttpproxy.go
+++ b/net/tshttpproxy/tshttpproxy.go
@@ -28,6 +28,7 @@ var (
 	noProxyUntil time.Time // if non-zero, time at which ProxyFromEnvironment should check again
 )
 
+// setNoProxyUntil stops calls to sysProxyEnv (if any) for the provided duration.
 func setNoProxyUntil(d time.Duration) {
 	mu.Lock()
 	defer mu.Unlock()
@@ -41,7 +42,15 @@ var _ = setNoProxyUntil // quiet staticcheck; Windows uses the above, more might
 // For example, WPAD PAC files on Windows.
 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.
 func ProxyFromEnvironment(req *http.Request) (*url.URL, error) {
+	u, err := http.ProxyFromEnvironment(req)
+	if u != nil && err == nil {
+		return u, nil
+	}
+
 	mu.Lock()
 	noProxyTime := noProxyUntil
 	mu.Unlock()
@@ -49,11 +58,6 @@ func ProxyFromEnvironment(req *http.Request) (*url.URL, error) {
 		return nil, nil
 	}
 
-	u, err := http.ProxyFromEnvironment(req)
-	if u != nil && err == nil {
-		return u, nil
-	}
-
 	if sysProxyFromEnv != nil {
 		u, err := sysProxyFromEnv(req)
 		if u != nil && err == nil {
diff --git a/net/tshttpproxy/tshttpproxy_test.go b/net/tshttpproxy/tshttpproxy_test.go
index 350068bd9..88e390847 100644
--- a/net/tshttpproxy/tshttpproxy_test.go
+++ b/net/tshttpproxy/tshttpproxy_test.go
@@ -5,10 +5,15 @@
 package tshttpproxy
 
 import (
+	"net/http"
 	"net/url"
+	"os"
 	"runtime"
 	"strings"
 	"testing"
+	"time"
+
+	"tailscale.com/util/must"
 )
 
 func TestGetAuthHeaderNoResult(t *testing.T) {
@@ -51,3 +56,29 @@ func TestGetAuthHeaderBasicAuth(t *testing.T) {
 		t.Fatalf("GetAuthHeader(%q) = %q; want %q", proxyURL, got, want)
 	}
 }
+
+func TestProxyFromEnvironment_setNoProxyUntil(t *testing.T) {
+	const fakeProxyEnv = "10.1.2.3:456"
+	const fakeProxyFull = "http://" + fakeProxyEnv
+
+	defer os.Setenv("HTTPS_PROXY", os.Getenv("HTTPS_PROXY"))
+	os.Setenv("HTTPS_PROXY", fakeProxyEnv)
+
+	req := &http.Request{URL: must.Get(url.Parse("https://example.com/"))}
+	for i := 0; i < 3; i++ {
+		switch i {
+		case 1:
+			setNoProxyUntil(time.Minute)
+		case 2:
+			setNoProxyUntil(0)
+		}
+		got, err := ProxyFromEnvironment(req)
+		if err != nil {
+			t.Fatalf("[%d] ProxyFromEnvironment: %v", i, err)
+		}
+		if got == nil || got.String() != fakeProxyFull {
+			t.Errorf("[%d] Got proxy %v; want %v", i, got, fakeProxyFull)
+		}
+	}
+
+}