controlclient: do not set HTTPS port for any private coordination server IP (#14564)

Fixes tailscale/tailscale#14563

When creating a NoiseClient, ensure that if any private IP address is provided, with both an `http` scheme and an explicit port number, we do not ever attempt to use HTTPS. We were only handling the case of `127.0.0.1` and `localhost`, but `192.168.x.y` is a private IP as well. This uses the `netip` package to check and adds some logging in case we ever need to troubleshoot this.

Signed-off-by: Andrea Gottardo <andrea@gottardo.me>

control/controlclient/noise.go

@@ -11,6 +11,7 @@ import (
 	"errors"
 	"math"
 	"net/http"
+	"net/netip"
 	"net/url"
 	"sync"
 	"time"
@@ -111,24 +112,39 @@ type NoiseOpts struct {
 // netMon may be nil, if non-nil it's used to do faster interface lookups.
 // dialPlan may be nil
 func NewNoiseClient(opts NoiseOpts) (*NoiseClient, error) {
+	logf := opts.Logf
 	u, err := url.Parse(opts.ServerURL)
 	if err != nil {
 		return nil, err
 	}
+
+	if u.Scheme != "http" && u.Scheme != "https" {
+		return nil, errors.New("invalid ServerURL scheme, must be http or https")
+	}
+
 	var httpPort string
 	var httpsPort string
+	addr, _ := netip.ParseAddr(u.Hostname())
+	isPrivateHost := addr.IsPrivate() || addr.IsLoopback() || u.Hostname() == "localhost"
 	if port := u.Port(); port != "" {
-		// If there is an explicit port specified, trust the scheme and hope for the best
+		// If there is an explicit port specified, entirely rely on the scheme,
-		if u.Scheme == "http" {
+		// unless it's http with a private host in which case we never try using HTTPS.
+		if u.Scheme == "https" {
+			httpPort = ""
+			httpsPort = port
+		} else if u.Scheme == "http" {
 			httpPort = port
 			httpsPort = "443"
-			if u.Hostname() == "127.0.0.1" || u.Hostname() == "localhost" {
+			if isPrivateHost {
+				logf("setting empty HTTPS port with http scheme and private host %s", u.Hostname())
 				httpsPort = ""
 			}
-		} else {
-			httpPort = "80"
-			httpsPort = port
 		}
+	} else if u.Scheme == "http" && isPrivateHost {
+		// Whenever the scheme is http and the hostname is an IP address, do not set the HTTPS port,
+		// as there cannot be a TLS certificate issued for an IP, unless it's a public IP.
+		httpPort = "80"
+		httpsPort = ""
 	} else {
 		// Otherwise, use the standard ports
 		httpPort = "80"

control/controlclient/noise_test.go

@@ -54,6 +54,123 @@ func TestNoiseClientHTTP2Upgrade_earlyPayload(t *testing.T) {
 	}.run(t)
 }
 
+func makeClientWithURL(t *testing.T, url string) *NoiseClient {
+	nc, err := NewNoiseClient(NoiseOpts{
+		Logf:      t.Logf,
+		ServerURL: url,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	return nc
+}
+
+func TestNoiseClientPortsAreSet(t *testing.T) {
+	tests := []struct {
+		name      string
+		url       string
+		wantHTTPS string
+		wantHTTP  string
+	}{
+		{
+			name:      "https-url",
+			url:       "https://example.com",
+			wantHTTPS: "443",
+			wantHTTP:  "80",
+		},
+		{
+			name:      "http-url",
+			url:       "http://example.com",
+			wantHTTPS: "443", // TODO(bradfitz): questionable; change?
+			wantHTTP:  "80",
+		},
+		{
+			name:      "https-url-custom-port",
+			url:       "https://example.com:123",
+			wantHTTPS: "123",
+			wantHTTP:  "",
+		},
+		{
+			name:      "http-url-custom-port",
+			url:       "http://example.com:123",
+			wantHTTPS: "443", // TODO(bradfitz): questionable; change?
+			wantHTTP:  "123",
+		},
+		{
+			name:      "http-loopback-no-port",
+			url:       "http://127.0.0.1",
+			wantHTTPS: "",
+			wantHTTP:  "80",
+		},
+		{
+			name:      "http-loopback-custom-port",
+			url:       "http://127.0.0.1:8080",
+			wantHTTPS: "",
+			wantHTTP:  "8080",
+		},
+		{
+			name:      "http-localhost-no-port",
+			url:       "http://localhost",
+			wantHTTPS: "",
+			wantHTTP:  "80",
+		},
+		{
+			name:      "http-localhost-custom-port",
+			url:       "http://localhost:8080",
+			wantHTTPS: "",
+			wantHTTP:  "8080",
+		},
+		{
+			name:      "http-private-ip-no-port",
+			url:       "http://192.168.2.3",
+			wantHTTPS: "",
+			wantHTTP:  "80",
+		},
+		{
+			name:      "http-private-ip-custom-port",
+			url:       "http://192.168.2.3:8080",
+			wantHTTPS: "",
+			wantHTTP:  "8080",
+		},
+		{
+			name:      "http-public-ip",
+			url:       "http://1.2.3.4",
+			wantHTTPS: "443", // TODO(bradfitz): questionable; change?
+			wantHTTP:  "80",
+		},
+		{
+			name:      "http-public-ip-custom-port",
+			url:       "http://1.2.3.4:8080",
+			wantHTTPS: "443", // TODO(bradfitz): questionable; change?
+			wantHTTP:  "8080",
+		},
+		{
+			name:      "https-public-ip",
+			url:       "https://1.2.3.4",
+			wantHTTPS: "443",
+			wantHTTP:  "80",
+		},
+		{
+			name:      "https-public-ip-custom-port",
+			url:       "https://1.2.3.4:8080",
+			wantHTTPS: "8080",
+			wantHTTP:  "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			nc := makeClientWithURL(t, tt.url)
+			if nc.httpsPort != tt.wantHTTPS {
+				t.Errorf("nc.httpsPort = %q; want %q", nc.httpsPort, tt.wantHTTPS)
+			}
+			if nc.httpPort != tt.wantHTTP {
+				t.Errorf("nc.httpPort = %q; want %q", nc.httpPort, tt.wantHTTP)
+			}
+		})
+	}
+}
+
 func (tt noiseClientTest) run(t *testing.T) {
 	serverPrivate := key.NewMachine()
 	clientPrivate := key.NewMachine()
@@ -81,6 +198,7 @@ func (tt noiseClientTest) run(t *testing.T) {
 		ServerPubKey: serverPrivate.Public(),
 		ServerURL:    hs.URL,
 		Dialer:       dialer,
+		Logf:         t.Logf,
 	})
 	if err != nil {
 		t.Fatal(err)