net/tshttpproxy: don't proxy through ourselves

When running a SOCKS or HTTP proxy, configure the tshttpproxy package to
drop those addresses from any HTTP_PROXY or HTTPS_PROXY environment
variables.

Fixes #7407

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I6cd7cad7a609c639780484bad521c7514841764b

cmd/derper/depaware.txt

@@ -112,7 +112,7 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa
    L    golang.org/x/net/bpf                                         from github.com/mdlayher/netlink+
         golang.org/x/net/dns/dnsmessage                              from net+
         golang.org/x/net/http/httpguts                               from net/http
-        golang.org/x/net/http/httpproxy                              from net/http
+        golang.org/x/net/http/httpproxy                              from net/http+
         golang.org/x/net/http2/hpack                                 from net/http
         golang.org/x/net/idna                                        from golang.org/x/crypto/acme/autocert+
         golang.org/x/net/proxy                                       from tailscale.com/net/netns

cmd/tailscaled/depaware.txt

@@ -355,7 +355,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         golang.org/x/net/bpf                                         from github.com/mdlayher/genetlink+
         golang.org/x/net/dns/dnsmessage                              from net+
         golang.org/x/net/http/httpguts                               from golang.org/x/net/http2+
-        golang.org/x/net/http/httpproxy                              from net/http
+        golang.org/x/net/http/httpproxy                              from net/http+
         golang.org/x/net/http2                                       from golang.org/x/net/http2/h2c+
         golang.org/x/net/http2/h2c                                   from tailscale.com/ipn/ipnlocal
         golang.org/x/net/http2/hpack                                 from golang.org/x/net/http2+

cmd/tailscaled/tailscaled.go

@@ -43,6 +43,7 @@ import (
 	"tailscale.com/net/proxymux"
 	"tailscale.com/net/socks5"
 	"tailscale.com/net/tsdial"
+	"tailscale.com/net/tshttpproxy"
 	"tailscale.com/net/tstun"
 	"tailscale.com/paths"
 	"tailscale.com/safesocket"
@@ -494,11 +495,13 @@ func getLocalBackend(ctx context.Context, logf logger.Logf, logID logid.PublicID
 		}
 	}
 	if socksListener != nil || httpProxyListener != nil {
+		var addrs []string
 		if httpProxyListener != nil {
 			hs := &http.Server{Handler: httpProxyHandler(dialer.UserDial)}
 			go func() {
 				log.Fatalf("HTTP proxy exited: %v", hs.Serve(httpProxyListener))
 			}()
+			addrs = append(addrs, httpProxyListener.Addr().String())
 		}
 		if socksListener != nil {
 			ss := &socks5.Server{
@@ -508,7 +511,9 @@ func getLocalBackend(ctx context.Context, logf logger.Logf, logID logid.PublicID
 			go func() {
 				log.Fatalf("SOCKS5 server exited: %v", ss.Serve(socksListener))
 			}()
+			addrs = append(addrs, socksListener.Addr().String())
 		}
+		tshttpproxy.SetSelfProxy(addrs...)
 	}
 
 	e = wgengine.NewWatchdog(e)

net/tshttpproxy/tshttpproxy.go

@@ -9,11 +9,16 @@ import (
 	"context"
 	"fmt"
 	"log"
+	"net"
 	"net/http"
 	"net/url"
 	"os"
+	"runtime"
+	"strings"
 	"sync"
 	"time"
+
+	"golang.org/x/net/http/httpproxy"
 )
 
 // InvalidateCache invalidates the package-level cache for ProxyFromEnvironment.
@@ -28,8 +33,23 @@ func InvalidateCache() {
 var (
 	mu           sync.Mutex
 	noProxyUntil time.Time         // if non-zero, time at which ProxyFromEnvironment should check again
+	config       *httpproxy.Config // used to create proxyFunc
+	proxyFunc    func(*url.URL) (*url.URL, error)
 )
 
+func getProxyFunc() func(*url.URL) (*url.URL, error) {
+	// Create config/proxyFunc if it's not created
+	mu.Lock()
+	defer mu.Unlock()
+	if config == nil {
+		config = httpproxy.FromEnvironment()
+	}
+	if proxyFunc == nil {
+		proxyFunc = config.ProxyFunc()
+	}
+	return proxyFunc
+}
+
 // setNoProxyUntil stops calls to sysProxyEnv (if any) for the provided duration.
 func setNoProxyUntil(d time.Duration) {
 	mu.Lock()
@@ -39,6 +59,59 @@ func setNoProxyUntil(d time.Duration) {
 
 var _ = setNoProxyUntil // quiet staticcheck; Windows uses the above, more might later
 
+// SetSelfProxy configures this package to avoid proxying through any of the
+// provided addresses–e.g. if they refer to proxies being run by this process.
+func SetSelfProxy(addrs ...string) {
+	mu.Lock()
+	defer mu.Unlock()
+
+	// Ensure we have a valid config
+	if config == nil {
+		config = httpproxy.FromEnvironment()
+	}
+
+	normalizeHostPort := func(s string) string {
+		host, portStr, err := net.SplitHostPort(s)
+		if err != nil {
+			return s
+		}
+
+		// Normalize the localhost IP into "localhost", to avoid IPv4/IPv6 confusion.
+		if host == "127.0.0.1" || host == "::1" {
+			return "localhost:" + portStr
+		}
+
+		// On Linux, all 127.0.0.1/8 IPs are also localhost.
+		if runtime.GOOS == "linux" && strings.HasPrefix(host, "127.0.0.") {
+			return "localhost:" + portStr
+		}
+
+		return s
+	}
+
+	normHTTP := normalizeHostPort(config.HTTPProxy)
+	normHTTPS := normalizeHostPort(config.HTTPSProxy)
+
+	// If any of our proxy variables point to one of the configured
+	// addresses, ignore them.
+	for _, addr := range addrs {
+		normAddr := normalizeHostPort(addr)
+		if normHTTP != "" && normHTTP == normAddr {
+			log.Printf("tshttpproxy: skipping HTTP_PROXY pointing to self: %q", addr)
+			config.HTTPProxy = ""
+			normHTTP = ""
+		}
+		if normHTTPS != "" && normHTTPS == normAddr {
+			log.Printf("tshttpproxy: skipping HTTPS_PROXY pointing to self: %q", addr)
+			config.HTTPSProxy = ""
+			normHTTPS = ""
+		}
+	}
+
+	// Invalidate to cause it to get re-created
+	proxyFunc = nil
+}
+
 // 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.
@@ -48,7 +121,8 @@ var sysProxyFromEnv func(*http.Request) (*url.URL, error)
 // 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)
+	localProxyFunc := getProxyFunc()
+	u, err := localProxyFunc(req.URL)
 	if u != nil && err == nil {
 		return u, nil
 	}

net/tshttpproxy/tshttpproxy_test.go

@@ -81,3 +81,127 @@ func TestProxyFromEnvironment_setNoProxyUntil(t *testing.T) {
 	}
 
 }
+
+func TestSetSelfProxy(t *testing.T) {
+	// Ensure we clean everything up at the end of our test
+	t.Cleanup(func() {
+		config = nil
+		proxyFunc = nil
+	})
+
+	testCases := []struct {
+		name      string
+		env       map[string]string
+		self      []string
+		wantHTTP  string
+		wantHTTPS string
+	}{
+		{
+			name: "no self proxy",
+			env: map[string]string{
+				"HTTP_PROXY":  "127.0.0.1:1234",
+				"HTTPS_PROXY": "127.0.0.1:1234",
+			},
+			self:      nil,
+			wantHTTP:  "127.0.0.1:1234",
+			wantHTTPS: "127.0.0.1:1234",
+		},
+		{
+			name: "skip proxies",
+			env: map[string]string{
+				"HTTP_PROXY":  "127.0.0.1:1234",
+				"HTTPS_PROXY": "127.0.0.1:5678",
+			},
+			self:      []string{"127.0.0.1:1234", "127.0.0.1:5678"},
+			wantHTTP:  "", // skipped
+			wantHTTPS: "", // skipped
+		},
+		{
+			name: "localhost normalization of env var",
+			env: map[string]string{
+				"HTTP_PROXY":  "localhost:1234",
+				"HTTPS_PROXY": "[::1]:5678",
+			},
+			self:      []string{"127.0.0.1:1234", "127.0.0.1:5678"},
+			wantHTTP:  "", // skipped
+			wantHTTPS: "", // skipped
+		},
+		{
+			name: "localhost normalization of addr",
+			env: map[string]string{
+				"HTTP_PROXY":  "127.0.0.1:1234",
+				"HTTPS_PROXY": "127.0.0.1:1234",
+			},
+			self:      []string{"[::1]:1234"},
+			wantHTTP:  "", // skipped
+			wantHTTPS: "", // skipped
+		},
+		{
+			name: "no ports",
+			env: map[string]string{
+				"HTTP_PROXY":  "myproxy",
+				"HTTPS_PROXY": "myproxy",
+			},
+			self:      []string{"127.0.0.1:1234"},
+			wantHTTP:  "myproxy",
+			wantHTTPS: "myproxy",
+		},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			for k, v := range tt.env {
+				oldEnv, found := os.LookupEnv(k)
+				if found {
+					t.Cleanup(func() {
+						os.Setenv(k, oldEnv)
+					})
+				}
+				os.Setenv(k, v)
+			}
+
+			// Reset computed variables
+			config = nil
+			proxyFunc = func(*url.URL) (*url.URL, error) {
+				panic("should not be called")
+			}
+
+			SetSelfProxy(tt.self...)
+
+			if got := config.HTTPProxy; got != tt.wantHTTP {
+				t.Errorf("got HTTPProxy=%q; want %q", got, tt.wantHTTP)
+			}
+			if got := config.HTTPSProxy; got != tt.wantHTTPS {
+				t.Errorf("got HTTPSProxy=%q; want %q", got, tt.wantHTTPS)
+			}
+			if proxyFunc != nil {
+				t.Errorf("wanted nil proxyFunc")
+			}
+
+			// Verify that we do actually proxy through the
+			// expected proxy, if we have one configured.
+			pf := getProxyFunc()
+			if tt.wantHTTP != "" {
+				want := "http://" + tt.wantHTTP
+
+				uu, _ := url.Parse("http://tailscale.com")
+				dest, err := pf(uu)
+				if err != nil {
+					t.Error(err)
+				} else if dest.String() != want {
+					t.Errorf("got dest=%q; want %q", dest, want)
+				}
+			}
+			if tt.wantHTTPS != "" {
+				want := "http://" + tt.wantHTTPS
+
+				uu, _ := url.Parse("https://tailscale.com")
+				dest, err := pf(uu)
+				if err != nil {
+					t.Error(err)
+				} else if dest.String() != want {
+					t.Errorf("got dest=%q; want %q", dest, want)
+				}
+			}
+		})
+	}
+}