net/tshttpproxy: synology: pick proxy by scheme

This updates the fix from #4562 to pick the proxy based on the request
scheme.

Updates #4395, #2605, #4562
Signed-off-by: James Tucker <james@tailscale.com>

net/tshttpproxy/tshttpproxy_synology.go

@@ -33,7 +33,8 @@ var (
 
 var cache struct {
 	sync.Mutex
-	proxy   *url.URL
+	httpProxy  *url.URL
+	httpsProxy *url.URL
 	updated    time.Time
 }
 
@@ -45,34 +46,36 @@ func synologyProxyFromConfigCached(req *http.Request) (*url.URL, error) {
 	cache.Lock()
 	defer cache.Unlock()
 
+	var err error
 	modtime := mtime(synologyProxyConfigPath)
 
-	if cache.updated == modtime {
-		return cache.proxy, nil
-	}
-
-	val, err := synologyProxyFromConfig(req)
-	cache.proxy = val
-
+	if modtime != cache.updated {
+		cache.httpProxy, cache.httpsProxy, err = synologyProxiesFromConfig()
 		cache.updated = modtime
+	}
 
-	return val, err
+	if req.URL.Scheme == "https" {
+		return cache.httpsProxy, err
+	}
+	return cache.httpProxy, err
 }
 
-func synologyProxyFromConfig(req *http.Request) (*url.URL, error) {
+func synologyProxiesFromConfig() (*url.URL, *url.URL, error) {
 	r, err := openSynologyProxyConf()
 	if err != nil {
 		if os.IsNotExist(err) {
-			return nil, nil
+			return nil, nil, nil
 		}
-		return nil, err
+		return nil, nil, err
 	}
 	defer r.Close()
 
 	return parseSynologyConfig(r)
 }
 
-func parseSynologyConfig(r io.Reader) (*url.URL, error) {
+// parseSynologyConfig parses the Synology proxy configuration, and returns any
+// http proxy, and any https proxy respectively, or an error if parsing fails.
+func parseSynologyConfig(r io.Reader) (*url.URL, *url.URL, error) {
 	cfg := map[string]string{}
 
 	if err := lineread.Reader(r, func(line []byte) error {
@@ -89,36 +92,43 @@ func parseSynologyConfig(r io.Reader) (*url.URL, error) {
 		cfg[string(key)] = string(value)
 		return nil
 	}); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	if cfg["proxy_enabled"] != "yes" {
-		return nil, nil
+		return nil, nil, nil
 	}
 
-	proxyURL := &url.URL{
-		Scheme: "http", // regardless of proxy type
-	}
+	httpProxyURL := new(url.URL)
+	httpsProxyURL := new(url.URL)
 	if cfg["auth_enabled"] == "yes" {
-		proxyURL.User = url.UserPassword(cfg["proxy_user"], cfg["proxy_pwd"])
+		httpProxyURL.User = url.UserPassword(cfg["proxy_user"], cfg["proxy_pwd"])
+		httpsProxyURL.User = url.UserPassword(cfg["proxy_user"], cfg["proxy_pwd"])
 	}
 
-	host, port := cfg["https_host"], cfg["https_port"]
-	if host == "" {
-		host, port = cfg["http_host"], cfg["http_port"]
+	// As far as we are aware, synology does not support tls proxies.
+	httpProxyURL.Scheme = "http"
+	httpsProxyURL.Scheme = "http"
+
+	httpsProxyURL = addHostPort(httpsProxyURL, cfg["https_host"], cfg["https_port"])
+	httpProxyURL = addHostPort(httpProxyURL, cfg["http_host"], cfg["http_port"])
+
+	return httpProxyURL, httpsProxyURL, nil
 }
 
+// addHostPort adds to u the given host and port and returns the updated url, or
+// if host is empty, it returns nil.
+func addHostPort(u *url.URL, host, port string) *url.URL {
 	if host == "" {
-		return nil, nil
+		return nil
 	}
 
-	if port != "" {
-		proxyURL.Host = net.JoinHostPort(host, port)
+	if port == "" {
+		u.Host = host
 	} else {
-		proxyURL.Host = host
+		u.Host = net.JoinHostPort(host, port)
 	}
-
-	return proxyURL, nil
+	return u
 }
 
 // mtime stat's path and returns its modification time. If path does not exist,

net/tshttpproxy/tshttpproxy_synology_test.go

@@ -22,7 +22,7 @@ import (
 )
 
 func TestSynologyProxyFromConfigCached(t *testing.T) {
-	req, err := http.NewRequest("GET", "https://example.org/", nil)
+	req, err := http.NewRequest("GET", "http://example.org/", nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -37,7 +37,8 @@ func TestSynologyProxyFromConfigCached(t *testing.T) {
 		}
 
 		cache.updated = time.Time{}
-		cache.proxy = nil
+		cache.httpProxy = nil
+		cache.httpsProxy = nil
 
 		if val, err := synologyProxyFromConfigCached(req); val != nil || err != nil {
 			t.Fatalf("got %s, %v; want nil, nil", val, err)
@@ -46,19 +47,25 @@ func TestSynologyProxyFromConfigCached(t *testing.T) {
 		if got, want := cache.updated, time.Unix(0, 0); got != want {
 			t.Fatalf("got %s, want %s", got, want)
 		}
-		if cache.proxy != nil {
-			t.Fatalf("got %s, want nil", cache.proxy)
+		if cache.httpProxy != nil {
+			t.Fatalf("got %s, want nil", cache.httpProxy)
+		}
+		if cache.httpsProxy != nil {
+			t.Fatalf("got %s, want nil", cache.httpsProxy)
 		}
 	})
 
 	t.Run("config file updated", func(t *testing.T) {
 		cache.updated = time.Now()
-		cache.proxy = nil
+		cache.httpProxy = nil
+		cache.httpsProxy = nil
 
 		if err := ioutil.WriteFile(synologyProxyConfigPath, []byte(`
 proxy_enabled=yes
 http_host=10.0.0.55
 http_port=80
+https_host=10.0.0.66
+https_port=443
 		`), 0600); err != nil {
 			t.Fatal(err)
 		}
@@ -67,6 +74,14 @@ http_port=80
 		if err != nil {
 			t.Fatal(err)
 		}
+
+		if cache.httpProxy == nil {
+			t.Fatal("http proxy was not cached")
+		}
+		if cache.httpsProxy == nil {
+			t.Fatal("https proxy was not cached")
+		}
+
 		if want := urlMustParse("http://10.0.0.55:80"); val.String() != want.String() {
 			t.Fatalf("got %s; want %s", val, want)
 		}
@@ -74,7 +89,8 @@ http_port=80
 
 	t.Run("config file removed", func(t *testing.T) {
 		cache.updated = time.Now()
-		cache.proxy = urlMustParse("http://127.0.0.1/")
+		cache.httpProxy = urlMustParse("http://127.0.0.1/")
+		cache.httpsProxy = urlMustParse("http://127.0.0.1/")
 
 		if err := os.Remove(synologyProxyConfigPath); err != nil && !os.IsNotExist(err) {
 			t.Fatal(err)
@@ -87,13 +103,62 @@ http_port=80
 		if val != nil {
 			t.Fatalf("got %s; want nil", val)
 		}
-		if cache.proxy != nil {
-			t.Fatalf("got %s, want nil", cache.proxy)
+		if cache.httpProxy != nil {
+			t.Fatalf("got %s, want nil", cache.httpProxy)
+		}
+		if cache.httpsProxy != nil {
+			t.Fatalf("got %s, want nil", cache.httpsProxy)
 		}
 	})
+
+	t.Run("picks proxy from request scheme", func(t *testing.T) {
+		cache.updated = time.Now()
+		cache.httpProxy = nil
+		cache.httpsProxy = nil
+
+		if err := ioutil.WriteFile(synologyProxyConfigPath, []byte(`
+proxy_enabled=yes
+http_host=10.0.0.55
+http_port=80
+https_host=10.0.0.66
+https_port=443
+		`), 0600); err != nil {
+			t.Fatal(err)
 		}
 
-func TestSynologyProxyFromConfig(t *testing.T) {
+		httpReq, err := http.NewRequest("GET", "http://example.com", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		val, err := synologyProxyFromConfigCached(httpReq)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if val == nil {
+			t.Fatalf("got nil, want an http URL")
+		}
+		if got, want := val.String(), "http://10.0.0.55:80"; got != want {
+			t.Fatalf("got %q, want %q", got, want)
+		}
+
+		httpsReq, err := http.NewRequest("GET", "https://example.com", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		val, err = synologyProxyFromConfigCached(httpsReq)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if val == nil {
+			t.Fatalf("got nil, want an http URL")
+		}
+		if got, want := val.String(), "http://10.0.0.66:443"; got != want {
+			t.Fatalf("got %q, want %q", got, want)
+		}
+	})
+}
+
+func TestSynologyProxiesFromConfig(t *testing.T) {
 	var (
 		openReader io.ReadCloser
 		openErr    error
@@ -104,11 +169,6 @@ func TestSynologyProxyFromConfig(t *testing.T) {
 	}
 	defer func() { openSynologyProxyConf = origOpen }()
 
-	req, err := http.NewRequest("GET", "https://example.com/", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-
 	t.Run("with config", func(t *testing.T) {
 		mc := &mustCloser{Reader: strings.NewReader(`
 proxy_user=foo
@@ -125,13 +185,21 @@ http_port=80
 		defer mc.check(t)
 		openReader = mc
 
-		proxyURL, err := synologyProxyFromConfig(req)
+		httpProxy, httpsProxy, err := synologyProxiesFromConfig()
 
 		if got, want := err, openErr; got != want {
 			t.Fatalf("got %s, want %s", got, want)
 		}
 
-		if got, want := proxyURL, urlMustParse("http://foo:bar@10.0.0.66:8443"); got.String() != want.String() {
+		if got, want := httpsProxy, urlMustParse("http://foo:bar@10.0.0.66:8443"); got.String() != want.String() {
+			t.Fatalf("got %s, want %s", got, want)
+		}
+
+		if got, want := err, openErr; got != want {
+			t.Fatalf("got %s, want %s", got, want)
+		}
+
+		if got, want := httpProxy, urlMustParse("http://foo:bar@10.0.0.55:80"); got.String() != want.String() {
 			t.Fatalf("got %s, want %s", got, want)
 		}
 
@@ -141,12 +209,15 @@ http_port=80
 		openReader = nil
 		openErr = os.ErrNotExist
 
-		proxyURL, err := synologyProxyFromConfig(req)
+		httpProxy, httpsProxy, err := synologyProxiesFromConfig()
 		if err != nil {
 			t.Fatalf("expected no error, got %s", err)
 		}
-		if proxyURL != nil {
-			t.Fatalf("expected no url, got %s", proxyURL)
+		if httpProxy != nil {
+			t.Fatalf("expected no url, got %s", httpProxy)
+		}
+		if httpsProxy != nil {
+			t.Fatalf("expected no url, got %s", httpsProxy)
 		}
 	})
 
@@ -154,12 +225,15 @@ http_port=80
 		openReader = nil
 		openErr = errors.New("example error")
 
-		proxyURL, err := synologyProxyFromConfig(req)
+		httpProxy, httpsProxy, err := synologyProxiesFromConfig()
 		if err != openErr {
 			t.Fatalf("expected %s, got %s", openErr, err)
 		}
-		if proxyURL != nil {
-			t.Fatalf("expected no url, got %s", proxyURL)
+		if httpProxy != nil {
+			t.Fatalf("expected no url, got %s", httpProxy)
+		}
+		if httpsProxy != nil {
+			t.Fatalf("expected no url, got %s", httpsProxy)
 		}
 	})
 
@@ -168,7 +242,8 @@ http_port=80
 func TestParseSynologyConfig(t *testing.T) {
 	cases := map[string]struct {
 		input      string
-		url   *url.URL
+		httpProxy  *url.URL
+		httpsProxy *url.URL
 		err        error
 	}{
 		"populated": {
@@ -184,7 +259,8 @@ https_port=8443
 http_host=10.0.0.55
 http_port=80
 `,
-			url: urlMustParse("http://foo:bar@10.0.0.66:8443"),
+			httpProxy:  urlMustParse("http://foo:bar@10.0.0.55:80"),
+			httpsProxy: urlMustParse("http://foo:bar@10.0.0.66:8443"),
 			err:        nil,
 		},
 		"no-auth": {
@@ -200,10 +276,11 @@ https_port=8443
 http_host=10.0.0.55
 http_port=80
 `,
-			url: urlMustParse("http://10.0.0.66:8443"),
+			httpProxy:  urlMustParse("http://10.0.0.55:80"),
+			httpsProxy: urlMustParse("http://10.0.0.66:8443"),
 			err:        nil,
 		},
-		"http": {
+		"http-only": {
 			input: `
 proxy_user=foo
 proxy_pwd=bar
@@ -216,7 +293,8 @@ https_port=8443
 http_host=10.0.0.55
 http_port=80
 `,
-			url: urlMustParse("http://foo:bar@10.0.0.55:80"),
+			httpProxy:  urlMustParse("http://foo:bar@10.0.0.55:80"),
+			httpsProxy: nil,
 			err:        nil,
 		},
 		"empty": {
@@ -232,14 +310,15 @@ https_port=
 http_host=
 http_port=
 `,
-			url: nil,
+			httpProxy:  nil,
+			httpsProxy: nil,
 			err:        nil,
 		},
 	}
 
 	for name, example := range cases {
 		t.Run(name, func(t *testing.T) {
-			url, err := parseSynologyConfig(strings.NewReader(example.input))
+			httpProxy, httpsProxy, err := parseSynologyConfig(strings.NewReader(example.input))
 			if err != example.err {
 				t.Fatal(err)
 			}
@@ -247,19 +326,33 @@ http_port=
 				return
 			}
 
-			if url == nil && example.url == nil {
-				return
+			if example.httpProxy == nil && httpProxy != nil {
+				t.Fatalf("got %s, want nil", httpProxy)
+			}
+
+			if example.httpProxy != nil {
+				if httpProxy == nil {
+					t.Fatalf("got nil, want %s", example.httpProxy)
 				}
 
-			if example.url == nil {
-				if url != nil {
-					t.Fatalf("got %s, want nil", url)
+				if got, want := example.httpProxy.String(), httpProxy.String(); got != want {
+					t.Fatalf("got %s, want %s", got, want)
 				}
 			}
 
-			if got, want := url.String(), example.url.String(); got != want {
+			if example.httpsProxy == nil && httpsProxy != nil {
+				t.Fatalf("got %s, want nil", httpProxy)
+			}
+
+			if example.httpsProxy != nil {
+				if httpsProxy == nil {
+					t.Fatalf("got nil, want %s", example.httpsProxy)
+				}
+
+				if got, want := example.httpsProxy.String(), httpsProxy.String(); got != want {
 					t.Fatalf("got %s, want %s", got, want)
 				}
+			}
 		})
 	}
 }