cmd/tailscale/cli: actually perform Noise request in 'debug ts2021'

This actually performs a Noise request in the 'debug ts2021' command,
instead of just exiting once we've dialed a connection. This can help
debug certain forms of captive portals and deep packet inspection that
will allow a connection, but will RST the connection when trying to send
data on the post-upgraded TCP connection.

Updates tailscale#1634

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

cmd/tailscale/cli/debug.go

@@ -28,10 +28,12 @@ import (
 
 	"github.com/peterbourgon/ff/v3/ffcli"
 	"golang.org/x/net/http/httpproxy"
+	"golang.org/x/net/http2"
 	"tailscale.com/client/tailscale"
 	"tailscale.com/client/tailscale/apitype"
 	"tailscale.com/control/controlhttp"
 	"tailscale.com/hostinfo"
+	"tailscale.com/internal/noiseconn"
 	"tailscale.com/ipn"
 	"tailscale.com/net/tsaddr"
 	"tailscale.com/net/tshttpproxy"
@@ -801,7 +803,10 @@ func runTS2021(ctx context.Context, args []string) error {
 		log.Printf("Dial(%q, %q) ...", network, address)
 		c, err := dialer.DialContext(ctx, network, address)
 		if err != nil {
-			log.Printf("Dial(%q, %q) = %v", network, address, err)
+			// skip logging context cancellation errors
+			if !errors.Is(err, context.Canceled) {
+				log.Printf("Dial(%q, %q) = %v", network, address, err)
+			}
 		} else {
 			log.Printf("Dial(%q, %q) = %v / %v", network, address, c.LocalAddr(), c.RemoteAddr())
 		}
@@ -835,32 +840,51 @@ func runTS2021(ctx context.Context, args []string) error {
 
 	log.Printf("final underlying conn: %v / %v", conn.LocalAddr(), conn.RemoteAddr())
 
-	// Make a /whois request to the server to verify that we can actually
-	// communicate over the newly-established connection.
-	whoisURL := "http://" + ts2021Args.host + "/machine/whois"
-	req, err = http.NewRequestWithContext(ctx, "GET", whoisURL, nil)
+	h2Transport, err := http2.ConfigureTransports(&http.Transport{
+		IdleConnTimeout: time.Second,
+	})
 	if err != nil {
-		return err
+		return fmt.Errorf("http2.ConfigureTransports: %w", err)
 	}
 
-	// Use a fake http.Transport that just "dials" by returning the above
-	// conn.
-	tr := http.DefaultTransport.(*http.Transport).Clone()
-	tr.ForceAttemptHTTP2 = true
-	tr.DialContext = func(context.Context, string, string) (net.Conn, error) {
-		return conn, nil
+	// Now, create a Noise conn over the existing conn.
+	nc, err := noiseconn.New(conn.Conn, h2Transport, 0, nil)
+	if err != nil {
+		return fmt.Errorf("noiseconn.New: %w", err)
 	}
-	resp, err := tr.RoundTrip(req)
+	defer nc.Close()
+
+	// Reserve a RoundTrip for the whoami request.
+	ok, _, err := nc.ReserveNewRequest(ctx)
 	if err != nil {
-		return fmt.Errorf("RoundTrip whois request: %w", err)
+		return fmt.Errorf("ReserveNewRequest: %w", err)
 	}
-	defer resp.Body.Close()
-	body, err := io.ReadAll(resp.Body)
+	if !ok {
+		return errors.New("ReserveNewRequest failed")
+	}
+
+	// Make a /whoami request to the server to verify that we can actually
+	// communicate over the newly-established connection.
+	whoamiURL := "http://" + ts2021Args.host + "/machine/whoami"
+	req, err = http.NewRequestWithContext(ctx, "GET", whoamiURL, nil)
+	if err != nil {
+		return err
+	}
+	resp, err := nc.RoundTrip(req)
 	if err != nil {
-		return fmt.Errorf("reading whois response: %w", err)
+		return fmt.Errorf("RoundTrip whoami request: %w", err)
 	}
+	defer resp.Body.Close()
 
-	log.Printf("whois response: %q", body)
+	if resp.StatusCode != 200 {
+		log.Printf("whoami request returned status %v", resp.Status)
+	} else {
+		body, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return fmt.Errorf("reading whoami response: %w", err)
+		}
+		log.Printf("whoami response: %q", body)
+	}
 	return nil
 }
 

cmd/tailscale/depaware.txt

@@ -78,7 +78,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         tailscale.com/cmd/tailscale/cli                              from tailscale.com/cmd/tailscale
         tailscale.com/cmd/tailscale/cli/ffcomplete                   from tailscale.com/cmd/tailscale/cli
         tailscale.com/cmd/tailscale/cli/ffcomplete/internal          from tailscale.com/cmd/tailscale/cli/ffcomplete
-        tailscale.com/control/controlbase                            from tailscale.com/control/controlhttp
+        tailscale.com/control/controlbase                            from tailscale.com/control/controlhttp+
         tailscale.com/control/controlhttp                            from tailscale.com/cmd/tailscale/cli
         tailscale.com/control/controlknobs                           from tailscale.com/net/portmapper
         tailscale.com/derp                                           from tailscale.com/derp/derphttp
@@ -89,6 +89,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         tailscale.com/health                                         from tailscale.com/net/tlsdial+
         tailscale.com/health/healthmsg                               from tailscale.com/cmd/tailscale/cli
         tailscale.com/hostinfo                                       from tailscale.com/client/web+
+        tailscale.com/internal/noiseconn                             from tailscale.com/cmd/tailscale/cli
         tailscale.com/ipn                                            from tailscale.com/client/tailscale+
         tailscale.com/ipn/ipnstate                                   from tailscale.com/client/tailscale+
         tailscale.com/licenses                                       from tailscale.com/client/web+
@@ -188,7 +189,8 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         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/http2/hpack                                 from net/http
+        golang.org/x/net/http2                                       from tailscale.com/cmd/tailscale/cli+
+        golang.org/x/net/http2/hpack                                 from net/http+
         golang.org/x/net/icmp                                        from tailscale.com/net/ping
         golang.org/x/net/idna                                        from golang.org/x/net/http/httpguts+
         golang.org/x/net/ipv4                                        from github.com/miekg/dns+

cmd/tailscaled/depaware.txt

@@ -245,7 +245,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/clientupdate                                   from tailscale.com/client/web+
         tailscale.com/clientupdate/distsign                          from tailscale.com/clientupdate
         tailscale.com/cmd/tailscaled/childproc                       from tailscale.com/cmd/tailscaled+
-        tailscale.com/control/controlbase                            from tailscale.com/control/controlclient+
+        tailscale.com/control/controlbase                            from tailscale.com/control/controlhttp+
         tailscale.com/control/controlclient                          from tailscale.com/cmd/tailscaled+
         tailscale.com/control/controlhttp                            from tailscale.com/control/controlclient
         tailscale.com/control/controlknobs                           from tailscale.com/control/controlclient+
@@ -265,6 +265,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/health                                         from tailscale.com/control/controlclient+
         tailscale.com/health/healthmsg                               from tailscale.com/ipn/ipnlocal
         tailscale.com/hostinfo                                       from tailscale.com/client/web+
+        tailscale.com/internal/noiseconn                             from tailscale.com/control/controlclient
         tailscale.com/ipn                                            from tailscale.com/client/tailscale+
         tailscale.com/ipn/conffile                                   from tailscale.com/cmd/tailscaled+
      💣 tailscale.com/ipn/ipnauth                                    from tailscale.com/ipn/ipnlocal+

control/controlclient/noise.go

@@ -174,12 +174,12 @@ func (nc *NoiseClient) GetSingleUseRoundTripper(ctx context.Context) (http.Round
 		if err != nil {
 			return nil, nil, err
 		}
-		rt, earlyPayloadMaybeNil, err := conn.ReserveNewRequest(ctx)
+		ok, earlyPayloadMaybeNil, err := conn.ReserveNewRequest(ctx)
 		if err != nil {
 			return nil, nil, err
 		}
-		if rt != nil {
-			return rt, earlyPayloadMaybeNil, nil
+		if ok {
+			return conn, earlyPayloadMaybeNil, nil
 		}
 	}
 	return nil, nil, errors.New("[unexpected] failed to reserve a request on a connection")

control/controlclient/noise_test.go

@@ -16,6 +16,7 @@ import (
 
 	"golang.org/x/net/http2"
 	"tailscale.com/control/controlhttp"
+	"tailscale.com/internal/noiseconn"
 	"tailscale.com/net/netmon"
 	"tailscale.com/net/tsdial"
 	"tailscale.com/tailcfg"
@@ -93,22 +94,21 @@ func (tt noiseClientTest) run(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	select {
-	case <-c.earlyPayloadReady:
-		gotNonNil := c.earlyPayload != nil
-		if gotNonNil != tt.sendEarlyPayload {
-			t.Errorf("sendEarlyPayload = %v but got earlyPayload = %T", tt.sendEarlyPayload, c.earlyPayload)
-		}
-		if c.earlyPayload != nil {
-			if c.earlyPayload.NodeKeyChallenge != chalPrivate.Public() {
-				t.Errorf("earlyPayload.NodeKeyChallenge = %v; want %v", c.earlyPayload.NodeKeyChallenge, chalPrivate.Public())
-			}
-		}
-
-	case <-ctx.Done():
+	payload, err := c.GetEarlyPayload(ctx)
+	if err != nil {
 		t.Fatal("timed out waiting for didReadHeaderCh")
 	}
 
+	gotNonNil := payload != nil
+	if gotNonNil != tt.sendEarlyPayload {
+		t.Errorf("sendEarlyPayload = %v but got earlyPayload = %T", tt.sendEarlyPayload, payload)
+	}
+	if payload != nil {
+		if payload.NodeKeyChallenge != chalPrivate.Public() {
+			t.Errorf("earlyPayload.NodeKeyChallenge = %v; want %v", payload.NodeKeyChallenge, chalPrivate.Public())
+		}
+	}
+
 	checkRes := func(t *testing.T, res *http.Response) {
 		t.Helper()
 		defer res.Body.Close()
@@ -184,7 +184,7 @@ func (up *Upgrader) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		// https://httpwg.org/specs/rfc7540.html#rfc.section.4.1 (Especially not
 		// an HTTP/2 settings frame, which isn't of type 'T')
 		var notH2Frame [5]byte
-		copy(notH2Frame[:], earlyPayloadMagic)
+		copy(notH2Frame[:], noiseconn.EarlyPayloadMagic)
 		var lenBuf [4]byte
 		binary.BigEndian.PutUint32(lenBuf[:], uint32(len(earlyJSON)))
 		// These writes are all buffered by caller, so fine to do them

internal/noiseconn/conn.go

@@ -1,3 +1,11 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package noiseconn contains an internal-only wrapper around controlbase.Conn
+// that properly handles the early payload sent by the server before the HTTP/2
+// session begins.
+//
+// See the documentation on the Conn type for more details.
 package noiseconn
 
 import (
@@ -16,9 +24,11 @@ import (
 )
 
 // Conn is a wrapper around controlbase.Conn.
-// It allows attaching an ID to a connection to allow
-// cleaning up references in the pool when the connection
-// is closed.
+//
+// It allows attaching an ID to a connection to allow cleaning up references in
+// the pool when the connection is closed, properly handles an optional "early
+// payload" that's sent prior to beginning the HTTP/2 session, and provides a
+// way to return a connection to a pool when the connection is closed.
 type Conn struct {
 	*controlbase.Conn
 	id      int
@@ -59,9 +69,13 @@ func (c *Conn) RoundTrip(r *http.Request) (*http.Response, error) {
 	return c.h2cc.RoundTrip(r)
 }
 
-// getEarlyPayload waits for the early noise payload to arrive.
+// GetEarlyPayload waits for the early Noise payload to arrive.
 // It may return (nil, nil) if the server begins HTTP/2 without one.
-func (c *Conn) getEarlyPayload(ctx context.Context) (*tailcfg.EarlyNoise, error) {
+//
+// It is safe to call this multiple times; all callers will block until the
+// early Noise payload is ready (if any) and will return the same result for
+// the lifetime of the Conn.
+func (c *Conn) GetEarlyPayload(ctx context.Context) (*tailcfg.EarlyNoise, error) {
 	select {
 	case <-c.earlyPayloadReady:
 		return c.earlyPayload, c.earlyPayloadErr
@@ -71,30 +85,39 @@ func (c *Conn) getEarlyPayload(ctx context.Context) (*tailcfg.EarlyNoise, error)
 }
 
 // ReserveNewRequest will reserve a new concurrent request on the connection.
-// It returns a non-nil http.RoundTripper if the reservation was successful,
-// and any early Noise payload if present. If a reservation was not successful,
-// it will return nil with no error.
-func (c *Conn) ReserveNewRequest(ctx context.Context) (http.RoundTripper, *tailcfg.EarlyNoise, error) {
-	earlyPayloadMaybeNil, err := c.getEarlyPayload(ctx)
+//
+// It returns whether the reservation was successful, and any early Noise
+// payload if present. If a reservation was not successful, it will return
+// false and nil for the early payload.
+func (c *Conn) ReserveNewRequest(ctx context.Context) (bool, *tailcfg.EarlyNoise, error) {
+	earlyPayloadMaybeNil, err := c.GetEarlyPayload(ctx)
 	if err != nil {
-		return nil, nil, err
+		return false, nil, err
 	}
 	if c.h2cc.ReserveNewRequest() {
-		return c, earlyPayloadMaybeNil, nil
+		return true, earlyPayloadMaybeNil, nil
 	}
-	return nil, nil, nil
+	return false, nil, nil
+}
+
+// CanTakeNewRequest reports whether the underlying HTTP/2 connection can take
+// a new request, meaning it has not been closed or received or sent a GOAWAY.
+func (c *Conn) CanTakeNewRequest() bool {
+	return c.h2cc.CanTakeNewRequest()
 }
 
 // The first 9 bytes from the server to client over Noise are either an HTTP/2
 // settings frame (a normal HTTP/2 setup) or, as we added later, an "early payload"
-// header that's also 9 bytes long: 5 bytes (earlyPayloadMagic) followed by 4 bytes
+// header that's also 9 bytes long: 5 bytes (EarlyPayloadMagic) followed by 4 bytes
 // of length. Then that many bytes of JSON-encoded tailcfg.EarlyNoise.
 // The early payload is optional. Some servers may not send it.
 const (
-	hdrLen            = 9 // http2 frame header size; also size of our early payload size header
-	earlyPayloadMagic = "\xff\xff\xffTS"
+	hdrLen = 9 // http2 frame header size; also size of our early payload size header
 )
 
+// EarlyPayloadMagic is the 5-byte magic prefix that indicates an early payload.
+const EarlyPayloadMagic = "\xff\xff\xffTS"
+
 // returnErrReader is an io.Reader that always returns an error.
 type returnErrReader struct {
 	err error // the error to return
@@ -129,13 +152,13 @@ func (c *Conn) readHeader() {
 		setErr(err)
 		return
 	}
-	if string(hdr[:len(earlyPayloadMagic)]) != earlyPayloadMagic {
+	if string(hdr[:len(EarlyPayloadMagic)]) != EarlyPayloadMagic {
 		// No early payload. We have to return the 9 bytes read we already
 		// consumed.
 		c.reader = io.MultiReader(bytes.NewReader(hdr[:]), c.Conn)
 		return
 	}
-	epLen := binary.BigEndian.Uint32(hdr[len(earlyPayloadMagic):])
+	epLen := binary.BigEndian.Uint32(hdr[len(EarlyPayloadMagic):])
 	if epLen > 10<<20 {
 		setErr(errors.New("invalid early payload length"))
 		return
@@ -162,9 +185,3 @@ func (c *Conn) Close() error {
 	}
 	return nil
 }
-
-// CanTakeNewRequest reports whether the connection can take a new request,
-// meaning it has not been closed or received or sent a GOAWAY.
-func (c *Conn) CanTakeNewRequest() bool {
-	return c.h2cc.CanTakeNewRequest()
-}