From c8f4dfc8c06000d9ce1379e8f66164754a8a7fb6 Mon Sep 17 00:00:00 2001
From: Tom DNetto <tom@tailscale.com>
Date: Tue, 19 Apr 2022 11:46:30 -0700
Subject: [PATCH] derp/derphttp,net/netcheck: improve netcheck behavior under
 MITM proxies

In cases where tailscale is operating behind a MITM proxy, we need to consider
that a lot more of the internals of our HTTP requests are visible and may be
used as part of authorization checks. As such, we need to 'behave' as closely
as possible to ideal.

 - Some proxies do authorization or consistency checks based the on Host header
   or HTTP URI, instead of just the IP/hostname/SNI. As such, we need to
   construct a `*http.Request` with a valid URI everytime HTTP is going to be
   used on the wire, even if its over TLS.
   Aside from the singular instance in net/netcheck, I couldn't find anywhere
   else a http.Request was constructed incorrectly.

 - Some proxies may deny requests, typically by returning a 403 status code. We
   should not consider these requests as a valid latency check, so netcheck
   semantics have been updated to consider >299 status codes as a failed probe.

Signed-off-by: Tom DNetto <tom@tailscale.com>
---
 derp/derphttp/derphttp_client.go | 17 +++++++++++------
 net/netcheck/netcheck.go         | 27 +++++++++++++++++++++------
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/derp/derphttp/derphttp_client.go b/derp/derphttp/derphttp_client.go
index 3360d436f..376f55519 100644
--- a/derp/derphttp/derphttp_client.go
+++ b/derp/derphttp/derphttp_client.go
@@ -538,12 +538,17 @@ func (c *Client) tlsClient(nc net.Conn, node *tailcfg.DERPNode) *tls.Conn {
 	return tls.Client(nc, tlsConf)
 }
 
-func (c *Client) DialRegionTLS(ctx context.Context, reg *tailcfg.DERPRegion) (tlsConn *tls.Conn, connClose io.Closer, err error) {
+// DialRegionTLS returns a TLS connection to a DERP node in the given region.
+//
+// DERP nodes for a region are tried in sequence according to their order
+// in the DERP map. TLS is initiated on the first node where a socket is
+// established.
+func (c *Client) DialRegionTLS(ctx context.Context, reg *tailcfg.DERPRegion) (tlsConn *tls.Conn, connClose io.Closer, node *tailcfg.DERPNode, err error) {
 	tcpConn, node, err := c.dialRegion(ctx, reg)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, nil, err
 	}
-	done := make(chan bool) // unbufferd
+	done := make(chan bool) // unbuffered
 	defer close(done)
 
 	tlsConn = c.tlsClient(tcpConn, node)
@@ -556,13 +561,13 @@ func (c *Client) DialRegionTLS(ctx context.Context, reg *tailcfg.DERPRegion) (tl
 	}()
 	err = tlsConn.Handshake()
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, nil, err
 	}
 	select {
 	case done <- true:
-		return tlsConn, tcpConn, nil
+		return tlsConn, tcpConn, node, nil
 	case <-ctx.Done():
-		return nil, nil, ctx.Err()
+		return nil, nil, nil, ctx.Err()
 	}
 }
 
diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go
index 18a87f4d8..dd2afe55b 100644
--- a/net/netcheck/netcheck.go
+++ b/net/netcheck/netcheck.go
@@ -979,13 +979,21 @@ func (c *Client) runHTTPOnlyChecks(ctx context.Context, last *Report, rs *report
 			// One warm-up one to get HTTP connection set
 			// up and get a connection from the browser's
 			// pool.
-			if _, err := http.DefaultClient.Do(req); err != nil {
-				c.logf("probing %s: %v", node.HostName, err)
+			if r, err := http.DefaultClient.Do(req); err != nil || r.StatusCode > 299 {
+				if err != nil {
+					c.logf("probing %s: %v", node.HostName, err)
+				} else {
+					c.logf("probing %s: unexpected status %s", node.HostName, r.Status)
+				}
 				return
 			}
 			t0 := c.timeNow()
-			if _, err := http.DefaultClient.Do(req); err != nil {
-				c.logf("probing %s: %v", node.HostName, err)
+			if r, err := http.DefaultClient.Do(req); err != nil || r.StatusCode > 299 {
+				if err != nil {
+					c.logf("probing %s: %v", node.HostName, err)
+				} else {
+					c.logf("probing %s: unexpected status %s", node.HostName, r.Status)
+				}
 				return
 			}
 			d := c.timeNow().Sub(t0)
@@ -1005,7 +1013,7 @@ func (c *Client) measureHTTPSLatency(ctx context.Context, reg *tailcfg.DERPRegio
 	var ip netaddr.IP
 
 	dc := derphttp.NewNetcheckClient(c.logf)
-	tlsConn, tcpConn, err := dc.DialRegionTLS(ctx, reg)
+	tlsConn, tcpConn, node, err := dc.DialRegionTLS(ctx, reg)
 	if err != nil {
 		return 0, ip, err
 	}
@@ -1036,7 +1044,7 @@ func (c *Client) measureHTTPSLatency(ctx context.Context, reg *tailcfg.DERPRegio
 	}
 	hc := &http.Client{Transport: tr}
 
-	req, err := http.NewRequestWithContext(ctx, "GET", "https://derp-unused-hostname.tld/derp/latency-check", nil)
+	req, err := http.NewRequestWithContext(ctx, "GET", "https://" + node.HostName + "/derp/latency-check", nil)
 	if err != nil {
 		return 0, ip, err
 	}
@@ -1047,6 +1055,13 @@ func (c *Client) measureHTTPSLatency(ctx context.Context, reg *tailcfg.DERPRegio
 	}
 	defer resp.Body.Close()
 
+	// DERPs should give us a nominal status code, so anything else is probably
+	// an access denied by a MITM proxy (or at the very least a signal not to
+	// trust this latency check).
+	if resp.StatusCode > 299 {
+		return 0, ip, fmt.Errorf("unexpected status code: %d (%s)", resp.StatusCode, resp.Status)
+	}
+
 	_, err = io.Copy(ioutil.Discard, io.LimitReader(resp.Body, 8<<10))
 	if err != nil {
 		return 0, ip, err