From 24976b5bfd201bb9d8c2757024490d58dbf0ce5e Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 19 Jun 2024 18:30:55 -0400 Subject: [PATCH] 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 #1634 Signed-off-by: Andrew Dunham Change-Id: I1e46ca9c9a0751c55f16373a6a76cdc24fec1f18 --- cmd/tailscale/cli/debug.go | 62 ++++++++++++++++++--------- cmd/tailscale/depaware.txt | 6 ++- cmd/tailscaled/depaware.txt | 3 +- control/controlclient/noise.go | 6 +-- control/controlclient/noise_test.go | 28 ++++++------- internal/noiseconn/conn.go | 65 ++++++++++++++++++----------- 6 files changed, 107 insertions(+), 63 deletions(-) diff --git a/cmd/tailscale/cli/debug.go b/cmd/tailscale/cli/debug.go index 5cfc4aa34..f6d98a830 100644 --- a/cmd/tailscale/cli/debug.go +++ b/cmd/tailscale/cli/debug.go @@ -28,10 +28,12 @@ "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 + h2Transport, err := http2.ConfigureTransports(&http.Transport{ + IdleConnTimeout: time.Second, + }) + if err != nil { + return fmt.Errorf("http2.ConfigureTransports: %w", err) + } + + // 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) + } + defer nc.Close() + + // Reserve a RoundTrip for the whoami request. + ok, _, err := nc.ReserveNewRequest(ctx) + if err != nil { + return fmt.Errorf("ReserveNewRequest: %w", err) + } + 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. - whoisURL := "http://" + ts2021Args.host + "/machine/whois" - req, err = http.NewRequestWithContext(ctx, "GET", whoisURL, nil) + whoamiURL := "http://" + ts2021Args.host + "/machine/whoami" + req, err = http.NewRequestWithContext(ctx, "GET", whoamiURL, nil) if err != nil { return 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 - } - resp, err := tr.RoundTrip(req) + resp, err := nc.RoundTrip(req) if err != nil { - return fmt.Errorf("RoundTrip whois request: %w", err) + return fmt.Errorf("RoundTrip whoami request: %w", err) } defer resp.Body.Close() - body, err := io.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("reading whois response: %w", err) - } - 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 } diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index fa484571e..29a740412 100644 --- a/cmd/tailscale/depaware.txt +++ b/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+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 5a680ff04..bdd1a989d 100644 --- a/cmd/tailscaled/depaware.txt +++ b/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+ diff --git a/control/controlclient/noise.go b/control/controlclient/noise.go index fdda96743..44437e2f3 100644 --- a/control/controlclient/noise.go +++ b/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") diff --git a/control/controlclient/noise_test.go b/control/controlclient/noise_test.go index 78c1effb4..f2627bd0a 100644 --- a/control/controlclient/noise_test.go +++ b/control/controlclient/noise_test.go @@ -16,6 +16,7 @@ "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 diff --git a/internal/noiseconn/conn.go b/internal/noiseconn/conn.go index d826dfb47..7476b7ecc 100644 --- a/internal/noiseconn/conn.go +++ b/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 @@ ) // 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() -}