fix: stop redirecting DERP and replicasync http requests (#10752)

spikecurtis · web-flow · commit 5173bce5cce5 · 2023-11-20T14:46:59.000+04:00
Fixes an issue where setting CODER_REDIRECT_TO_ACCESS_URL breaks use of multiple Coder server replicas for DERP traffic.
diff --git a/cli/server.go b/cli/server.go
@@ -1922,6 +1922,18 @@ func redirectToAccessURL(handler http.Handler, accessURL *url.URL, tunnel bool,
 			http.Redirect(w, r, accessURL.String(), http.StatusTemporaryRedirect)
 		}
 
+		// Exception: DERP
+		// We use this endpoint when creating a DERP-mesh in the enterprise version to directly
+		// dial other Coderd derpers.  Redirecting to the access URL breaks direct dial since the
+		// access URL will be load-balanced in a multi-replica deployment.
+		//
+		// It's totally fine to access DERP over TLS, but we also don't need to redirect HTTP to
+		// HTTPS as DERP is itself an encrypted protocol.
+		if isDERPPath(r.URL.Path) {
+			handler.ServeHTTP(w, r)
+			return
+		}
+
 		// Only do this if we aren't tunneling.
 		// If we are tunneling, we want to allow the request to go through
 		// because the tunnel doesn't proxy with TLS.
@@ -1949,6 +1961,14 @@ func redirectToAccessURL(handler http.Handler, accessURL *url.URL, tunnel bool,
 	})
 }
 
+func isDERPPath(p string) bool {
+	segments := strings.SplitN(p, "/", 3)
+	if len(segments) < 2 {
+		return false
+	}
+	return segments[1] == "derp"
+}
+
 // IsLocalhost returns true if the host points to the local machine. Intended to
 // be called with `u.Hostname()`.
 func IsLocalhost(host string) bool {
diff --git a/cli/server_internal_test.go b/cli/server_internal_test.go
@@ -243,3 +243,56 @@ func TestRedirectHTTPToHTTPSDeprecation(t *testing.T) {
 		})
 	}
 }
+
+func TestIsDERPPath(t *testing.T) {
+	t.Parallel()
+
+	testcases := []struct {
+		path     string
+		expected bool
+	}{
+		//{
+		//	path:     "/derp",
+		//	expected: true,
+		//},
+		{
+			path:     "/derp/",
+			expected: true,
+		},
+		{
+			path:     "/derp/latency-check",
+			expected: true,
+		},
+		{
+			path:     "/derp/latency-check/",
+			expected: true,
+		},
+		{
+			path:     "",
+			expected: false,
+		},
+		{
+			path:     "/",
+			expected: false,
+		},
+		{
+			path:     "/derptastic",
+			expected: false,
+		},
+		{
+			path:     "/api/v2/derp",
+			expected: false,
+		},
+		{
+			path:     "//",
+			expected: false,
+		},
+	}
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.path, func(t *testing.T) {
+			t.Parallel()
+			require.Equal(t, tc.expected, isDERPPath(tc.path))
+		})
+	}
+}
diff --git a/cli/server_test.go b/cli/server_test.go
@@ -683,6 +683,12 @@ func TestServer(t *testing.T) {
 						require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
 						require.Equal(t, c.expectRedirect, resp.Header.Get("Location"))
 					}
+
+					// We should never redirect DERP
+					respDERP, err := client.Request(ctx, http.MethodGet, "/derp", nil)
+					require.NoError(t, err)
+					defer respDERP.Body.Close()
+					require.Equal(t, http.StatusUpgradeRequired, respDERP.StatusCode)
 				}
 
 				// Verify TLS
diff --git a/enterprise/replicasync/replicasync.go b/enterprise/replicasync/replicasync.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
 	"os"
 	"strings"
 	"sync"
@@ -274,7 +275,19 @@ func (m *Manager) syncReplicas(ctx context.Context) error {
 		wg.Add(1)
 		go func(peer database.Replica) {
 			defer wg.Done()
-			req, err := http.NewRequestWithContext(ctx, http.MethodGet, peer.RelayAddress, nil)
+			ra, err := url.Parse(peer.RelayAddress)
+			if err != nil {
+				m.logger.Warn(ctx, "could not parse relay address",
+					slog.F("relay_address", peer.RelayAddress), slog.Error(err))
+				return
+			}
+			target, err := ra.Parse("/derp/latency-check")
+			if err != nil {
+				m.logger.Warn(ctx, "could not resolve /derp/latency-check endpoint",
+					slog.F("relay_address", peer.RelayAddress), slog.Error(err))
+				return
+			}
+			req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.String(), nil)
 			if err != nil {
 				m.logger.Warn(ctx, "create http request for relay probe",
 					slog.F("relay_address", peer.RelayAddress), slog.Error(err))
diff --git a/enterprise/replicasync/replicasync_test.go b/enterprise/replicasync/replicasync_test.go
@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -55,9 +56,9 @@ func TestReplica(t *testing.T) {
 		// Ensures that the replica reports a successful status for
 		// accessing all of its peers.
 		t.Parallel()
-		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		}))
+		dh := &derpyHandler{}
+		defer dh.requireOnlyDERPPaths(t)
+		srv := httptest.NewServer(dh)
 		defer srv.Close()
 		db, pubsub := dbtestutil.NewDB(t)
 		peer, err := db.InsertReplica(context.Background(), database.InsertReplicaParams{
@@ -98,9 +99,9 @@ func TestReplica(t *testing.T) {
 			ServerName:   "hello.org",
 			RootCAs:      pool,
 		}
-		srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		}))
+		dh := &derpyHandler{}
+		defer dh.requireOnlyDERPPaths(t)
+		srv := httptest.NewUnstartedServer(dh)
 		srv.TLS = tlsConfig
 		srv.StartTLS()
 		defer srv.Close()
@@ -167,9 +168,9 @@ func TestReplica(t *testing.T) {
 		server, err := replicasync.New(ctx, slogtest.Make(t, nil), db, pubsub, nil)
 		require.NoError(t, err)
 		defer server.Close()
-		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		}))
+		dh := &derpyHandler{}
+		defer dh.requireOnlyDERPPaths(t)
+		srv := httptest.NewServer(dh)
 		defer srv.Close()
 		peer, err := db.InsertReplica(ctx, database.InsertReplicaParams{
 			ID:           uuid.New(),
@@ -221,9 +222,9 @@ func TestReplica(t *testing.T) {
 		db := dbmem.New()
 		pubsub := pubsub.NewInMemory()
 		logger := slogtest.Make(t, nil)
-		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(http.StatusOK)
-		}))
+		dh := &derpyHandler{}
+		defer dh.requireOnlyDERPPaths(t)
+		srv := httptest.NewServer(dh)
 		defer srv.Close()
 		var wg sync.WaitGroup
 		count := 20
@@ -255,3 +256,20 @@ func TestReplica(t *testing.T) {
 		wg.Wait()
 	})
 }
+
+type derpyHandler struct {
+	atomic.Uint32
+}
+
+func (d *derpyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	if r.URL.Path != "/derp/latency-check" {
+		w.WriteHeader(http.StatusNotFound)
+		d.Add(1)
+		return
+	}
+	w.WriteHeader(http.StatusUpgradeRequired)
+}
+
+func (d *derpyHandler) requireOnlyDERPPaths(t *testing.T) {
+	require.Equal(t, uint32(0), d.Load())
+}

Original file line number	Diff line number	Diff line change
`@@ -683,6 +683,12 @@ func TestServer(t *testing.T) {`
`683`	`683`	`require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)`
`684`	`684`	`require.Equal(t, c.expectRedirect, resp.Header.Get("Location"))`
`685`	`685`	`}`
	`686`	`+`
	`687`	`+ // We should never redirect DERP`
	`688`	`+ respDERP, err := client.Request(ctx, http.MethodGet, "/derp", nil)`
	`689`	`+ require.NoError(t, err)`
	`690`	`+ defer respDERP.Body.Close()`
	`691`	`+ require.Equal(t, http.StatusUpgradeRequired, respDERP.StatusCode)`
`686`	`692`	`}`
`687`	`693`
`688`	`694`	`// Verify TLS`