diff --git a/derp/derphttp/derphttp_client.go b/derp/derphttp/derphttp_client.go index cc97c6e71a09d..4acf906a98fed 100644 --- a/derp/derphttp/derphttp_client.go +++ b/derp/derphttp/derphttp_client.go @@ -260,23 +260,26 @@ func (c *Client) urlString(node *tailcfg.DERPNode) string { if c.url != nil { return c.url.String() } - if node.HostName == "" { - url := &url.URL{ - Scheme: "https", - Host: fmt.Sprintf("%s:%d", node.IPv4, node.DERPPort), - Path: "/derp", - } - if node.ForceHTTP { - url.Scheme = "http" - } - return url.String() - } - proto := "https" + url := &url.URL{ + Scheme: "https", + Host: node.IPv4, + Path: "/derp", + } if debugUseDERPHTTP() || node.ForceHTTP { - proto = "http" + url.Scheme = "http" } - return fmt.Sprintf("%s://%s/derp", proto, node.HostName) + if url.Host == "" { + url.Host = node.HostName + } + + // Add the DERPPort to the URL host, unless the host already contains a port + // or the DERPPort is the default port for the protocol. + if node.DERPPort != 0 && !strings.Contains(url.Host, ":") && ((url.Scheme == "https" && node.DERPPort != 443) || (url.Scheme == "http" && node.DERPPort != 80)) { + url.Host = fmt.Sprintf("%s:%d", url.Host, node.DERPPort) + } + + return url.String() } // AddressFamilySelector decides whether IPv6 is preferred for diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index b6bfef1078cef..070da6d1d495a 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -4,6 +4,7 @@ package magicsock import ( + "bufio" "bytes" "context" crand "crypto/rand" @@ -12,6 +13,7 @@ import ( "errors" "fmt" "io" + "log" "math/rand" "net" "net/http" @@ -37,6 +39,7 @@ import ( "golang.org/x/net/icmp" "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" + "nhooyr.io/websocket" "tailscale.com/cmd/testwrapper/flakytest" "tailscale.com/derp" "tailscale.com/derp/derphttp" @@ -48,6 +51,7 @@ import ( "tailscale.com/net/ping" "tailscale.com/net/stun/stuntest" "tailscale.com/net/tstun" + "tailscale.com/net/wsconn" "tailscale.com/tailcfg" "tailscale.com/tstest" "tailscale.com/tstest/natlab" @@ -161,7 +165,16 @@ func newMagicStack(t testing.TB, logf logger.Logf, l nettype.PacketListener, der return newMagicStackWithKey(t, logf, l, derpMap, privateKey) } +func newMagicStackFunc(t testing.TB, logf logger.Logf, l nettype.PacketListener, derpMap *tailcfg.DERPMap, updateConnFunc func(ms *Conn)) *magicStack { + privateKey := key.NewNode() + return newMagicStackWithKeyFunc(t, logf, l, derpMap, privateKey, updateConnFunc) +} + func newMagicStackWithKey(t testing.TB, logf logger.Logf, l nettype.PacketListener, derpMap *tailcfg.DERPMap, privateKey key.NodePrivate) *magicStack { + return newMagicStackWithKeyFunc(t, logf, l, derpMap, privateKey, nil) +} + +func newMagicStackWithKeyFunc(t testing.TB, logf logger.Logf, l nettype.PacketListener, derpMap *tailcfg.DERPMap, privateKey key.NodePrivate, updateConnFunc func(ms *Conn)) *magicStack { t.Helper() epCh := make(chan []tailcfg.Endpoint, 100) // arbitrary @@ -186,6 +199,11 @@ func newMagicStackWithKey(t testing.TB, logf logger.Logf, l nettype.PacketListen wgLogger := wglog.NewLogger(logf) dev := wgcfg.NewDevice(tsTun, conn.Bind(), wgLogger.DeviceLogger) + + if updateConnFunc != nil { + updateConnFunc(conn) + } + dev.Up() // Wait for magicsock to connect up to DERP. @@ -2876,3 +2894,109 @@ func TestAddrForSendLockedForWireGuardOnly(t *testing.T) { } } } + +// Copied from cmd/derper +func addWebSocketSupport(s *derp.Server, base http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + up := strings.ToLower(r.Header.Get("Upgrade")) + + // Very early versions of Tailscale set "Upgrade: WebSocket" but didn't actually + // speak WebSockets (they still assumed DERP's binary framing). So to distinguish + // clients that actually want WebSockets, look for an explicit "derp" subprotocol. + if up != "websocket" || !strings.Contains(r.Header.Get("Sec-Websocket-Protocol"), "derp") { + base.ServeHTTP(w, r) + return + } + + c, err := websocket.Accept(w, r, &websocket.AcceptOptions{ + Subprotocols: []string{"derp"}, + OriginPatterns: []string{"*"}, + // Disable compression because we transmit WireGuard messages that + // are not compressible. + // Additionally, Safari has a broken implementation of compression + // (see https://github.com/nhooyr/websocket/issues/218) that makes + // enabling it actively harmful. + CompressionMode: websocket.CompressionDisabled, + }) + if err != nil { + log.Printf("websocket.Accept: %v", err) + return + } + defer c.Close(websocket.StatusInternalError, "closing") + if c.Subprotocol() != "derp" { + c.Close(websocket.StatusPolicyViolation, "client must speak the derp subprotocol") + return + } + wc := wsconn.NetConn(r.Context(), c, websocket.MessageBinary) + brw := bufio.NewReadWriter(bufio.NewReader(wc), bufio.NewWriter(wc)) + s.Accept(r.Context(), wc, brw, r.RemoteAddr) + }) +} + +func TestDERPForceWebsockets(t *testing.T) { + logf, closeLogf := logger.LogfCloser(t.Logf) + defer closeLogf() + + // Create a DERP server manually, without a STUN server and with a custom + // handler. + derpServer := derp.NewServer(key.NewNode(), logf) + derpHandler := derphttp.Handler(derpServer) + derpHandler = addWebSocketSupport(derpServer, derpHandler) + + var upgradeCount int64 + httpsrv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + up := r.Header.Get("Upgrade") + if up != "" { + if up != "websocket" { + t.Errorf("unexpected upgrade header: %q", up) + } else { + atomic.AddInt64(&upgradeCount, 1) + } + } + + derpHandler.ServeHTTP(w, r) + })) + httpsrv.Config.ErrorLog = logger.StdLogger(logf) + httpsrv.Config.TLSNextProto = make(map[string]func(*http.Server, *tls.Conn, http.Handler)) + httpsrv.StartTLS() + t.Cleanup(func() { + httpsrv.CloseClientConnections() + httpsrv.Close() + derpServer.Close() + }) + + derpMap := &tailcfg.DERPMap{ + Regions: map[int]*tailcfg.DERPRegion{ + 1: { + RegionID: 1, + RegionCode: "test", + Nodes: []*tailcfg.DERPNode{ + { + Name: "t1", + RegionID: 1, + HostName: "test-node.unused", + IPv4: "127.0.0.1", + IPv6: "", + STUNPort: -1, + DERPPort: httpsrv.Listener.Addr().(*net.TCPAddr).Port, + InsecureForTests: true, + }, + }, + }, + }, + } + + m := &natlab.Machine{Name: "m1"} + ms := newMagicStackFunc(t, logger.WithPrefix(logf, "conn1: "), m, derpMap, func(ms *Conn) { + ms.SetDERPForceWebsockets(true) + }) + defer ms.Close() + + if len(ms.conn.activeDerp) == 0 { + t.Errorf("unexpected DERP empty got: %v want: >0", len(ms.conn.activeDerp)) + } + + if atomic.LoadInt64(&upgradeCount) == 0 { + t.Errorf("no websocket upgrade requests seen") + } +}