fix: add websocket fallback when http2 is used

kylecarbs · kylecarbs · commit 5734fc524141 · 2023-03-23T20:30:41.000Z
HTTP/2 doesn't support the 'Upgrade' header, which breaks
the DERP exchange.
diff --git a/derp/derphttp/derphttp_client.go b/derp/derphttp/derphttp_client.go
@@ -347,26 +347,26 @@ func (c *Client) connect(ctx context.Context, caller string) (client *derp.Clien
 	case c.useWebsockets():
 		var urlStr string
 		var tlsConfig *tls.Config
-		// WebSocket connections require a DERP to proxy.
-		// DERP mappings have no explicit requirements on the ordering
-		// of DERP vs STUNOnly nodes. So we pick the first non-STUNOnly one.
-		var node *tailcfg.DERPNode
-		for _, n := range reg.Nodes {
-			if n.STUNOnly {
-				continue
+		if reg != nil {
+			// WebSocket connections require a DERP to proxy.
+			// DERP mappings have no explicit requirements on the ordering
+			// of DERP vs STUNOnly nodes. So we pick the first non-STUNOnly one.
+			var node *tailcfg.DERPNode
+			for _, n := range reg.Nodes {
+				if n.STUNOnly {
+					continue
+				}
+				node = n
+				break
+			}
+			if node == nil {
+				return nil, 0, errors.New("no non-STUN-only nodes in region")
 			}
-			node = n
-			break
-		}
-		if node == nil {
-			return nil, 0, errors.New("no non-STUN-only nodes in region")
-		}
-		if c.url != nil {
-			urlStr = c.url.String()
-			tlsConfig = c.tlsConfig(nil)
-		} else {
 			urlStr = c.urlString(node)
 			tlsConfig = c.tlsConfig(node)
+		} else if c.url != nil {
+			urlStr = c.url.String()
+			tlsConfig = c.tlsConfig(nil)
 		}
 		c.logf("%s: connecting websocket to %v", caller, urlStr)
 		conn, err := dialWebsocketFunc(ctx, urlStr, tlsConfig, c.Header)
@@ -492,6 +492,17 @@ func (c *Client) connect(ctx context.Context, caller string) (client *derp.Clien
 		// No need to flush the HTTP request. the derp.Client's initial
 		// client auth frame will flush it.
 	} else {
+		regionID := 0
+		if reg != nil {
+			regionID = reg.RegionID
+		}
+
+		if tlsState != nil && tlsState.NegotiatedProtocol == "h2" {
+			reason := fmt.Sprintf("The server wanted us to use HTTP/2, but DERP requires Upgrade which needs HTTP/1.1")
+			c.forceWebsockets(regionID, reason)
+			return nil, 0, fmt.Errorf("DERP server did not switch protocols: %s", reason)
+		}
+
 		if err := req.Write(brw); err != nil {
 			return nil, 0, err
 		}
@@ -512,17 +523,12 @@ func (c *Client) connect(ctx context.Context, caller string) (client *derp.Clien
 			// For safety, we'll assume that status codes >= 400 are
 			// proxies that don't support WebSockets.
 			if resp.StatusCode >= 400 && resp.StatusCode < 500 {
-				reason := fmt.Sprintf("GET failed with status code %d: %s", resp.StatusCode, b)
-				c.logf("We'll use WebSockets on the next connection attempt. A proxy could be disallowing the use of 'Upgrade: derp': %s", reason)
-				c.forcedWebsocket.Store(true)
-				forcedWebsocketCallback := c.forcedWebsocketCallback.Load()
-				if forcedWebsocketCallback != nil {
-					go (*forcedWebsocketCallback)(reg.RegionID, reason)
-				}
+				c.forceWebsockets(regionID, fmt.Sprintf("GET failed with status code %d (a proxy could be disallowing the use of 'Upgrade: derp'): %s", resp.StatusCode, b))
 			}
 
 			return nil, 0, fmt.Errorf("GET failed: %v: %s", err, b)
 		}
+
 	}
 	derpClient, err = derp.NewClient(c.privateKey, httpConn, brw, c.logf,
 		derp.MeshKey(c.MeshKey),
@@ -571,6 +577,15 @@ func (c *Client) SetForcedWebsocketCallback(callback func(region int, reason str
 	c.forcedWebsocketCallback.Store(&callback)
 }
 
+func (c *Client) forceWebsockets(regionID int, reason string) {
+	c.logf("We'll use WebSockets on the next connection attempt: %s", reason)
+	c.forcedWebsocket.Store(true)
+	forcedWebsocketCallback := c.forcedWebsocketCallback.Load()
+	if forcedWebsocketCallback != nil {
+		go (*forcedWebsocketCallback)(regionID, reason)
+	}
+}
+
 func (c *Client) dialURL(ctx context.Context) (net.Conn, error) {
 	host := c.url.Hostname()
 	if c.dialer != nil {
@@ -635,6 +650,7 @@ func (c *Client) tlsConfig(node *tailcfg.DERPNode) *tls.Config {
 			tlsdial.SetConfigExpectedCert(tlsConf, node.CertName)
 		}
 	}
+	tlsConf.NextProtos = []string{"h2", "http/1.1"}
 	return tlsConf
 }
 
diff --git a/derp/derphttp/derphttp_test.go b/derp/derphttp/derphttp_test.go
@@ -4,16 +4,21 @@
 package derphttp
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"crypto/tls"
 	"net"
 	"net/http"
+	"net/http/httptest"
+	"strings"
 	"sync"
 	"testing"
 	"time"
 
+	"nhooyr.io/websocket"
 	"tailscale.com/derp"
+	"tailscale.com/net/wsconn"
 	"tailscale.com/types/key"
 )
 
@@ -206,3 +211,63 @@ func TestPing(t *testing.T) {
 		t.Fatalf("Ping: %v", err)
 	}
 }
+
+func TestHTTP2WebSocketFallback(t *testing.T) {
+	serverPrivateKey := key.NewNode()
+	s := derp.NewServer(serverPrivateKey, t.Logf)
+	defer s.Close()
+
+	httpsrv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		up := r.Header.Get("Upgrade")
+		if up != "websocket" {
+			Handler(s).ServeHTTP(w, r)
+			return
+		}
+
+		c, err := websocket.Accept(w, r, &websocket.AcceptOptions{})
+		if err != nil {
+			t.Errorf("websocket.Accept: %v", err)
+			return
+		}
+		defer c.Close(websocket.StatusInternalError, "closing")
+		wc := wsconn.NetConn(context.Background(), c, websocket.MessageBinary)
+		brw := bufio.NewReadWriter(bufio.NewReader(wc), bufio.NewWriter(wc))
+		s.Accept(context.Background(), wc, brw, r.RemoteAddr)
+	}))
+	httpsrv.TLS = &tls.Config{
+		NextProtos: []string{"h2", "http/1.1"},
+	}
+	httpsrv.StartTLS()
+
+	serverURL := httpsrv.URL
+	t.Logf("server URL: %s", serverURL)
+
+	c, err := NewClient(key.NewNode(), serverURL, t.Logf)
+	if err != nil {
+		t.Fatalf("NewClient: %v", err)
+	}
+	c.TLSConfig = &tls.Config{
+		ServerName: "example.com",
+		RootCAs:    httpsrv.Client().Transport.(*http.Transport).TLSClientConfig.RootCAs,
+	}
+	defer c.Close()
+	reasonCh := make(chan string, 1)
+	c.SetForcedWebsocketCallback(func(region int, reason string) {
+		select {
+		case reasonCh <- reason:
+		default:
+		}
+	})
+	err = c.Connect(context.Background())
+	if err == nil {
+		// Expect an error!
+		t.Fatal("client didn't error on initial connect")
+	}
+	reason := <-reasonCh
+	if !strings.Contains(reason, "wanted us to use HTTP/2") {
+		t.Fatalf("reason doesn't contain message: %s", reason)
+	}
+	if err := c.Connect(context.Background()); err != nil {
+		t.Fatalf("client Connect: %v", err)
+	}
+}
