From 15fc6cd96637e8a0e697ff2157c1608ada8e4a39 Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@tailscale.com>
Date: Fri, 3 May 2024 07:01:42 -0700
Subject: [PATCH] derp/derphttp: fix netcheck HTTPS probes

The netcheck client, when no UDP is available, probes distance using
HTTPS.

Several problems:

* It probes using /derp/latency-check.
* But cmd/derper serves the handler at /derp/probe
* Despite the difference, it work by accident until c8f4dfc8c0600
  which made netcheck's probe require a 2xx status code.
* in tests, we only use derphttp.Handler, so the cmd/derper-installed
  mux routes aren't preesnt, so there's no probe. That breaks
  tests in airplane mode. netcheck.Client then reports "unexpected
  HTTP status 426" (Upgrade Required)

This makes derp handle both /derp/probe and /derp/latency-check
equivalently, and in both cmd/derper and derphttp.Handler standalone
modes.

I notice this when wgengine/magicsock TestActiveDiscovery was failing
in airplane mode (no wifi). It still doesn't pass, but it gets
further.

Fixes #11989

Change-Id: I45213d4bd137e0f29aac8bd4a9ac92091065113f
---
 cmd/derper/derper.go             | 18 ++++++------------
 derp/derphttp/derphttp_server.go | 21 +++++++++++++++++++++
 derp/derphttp/derphttp_test.go   | 22 ++++++++++++++++++++++
 3 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/cmd/derper/derper.go b/cmd/derper/derper.go
index cc3ac4467..abf254215 100644
--- a/cmd/derper/derper.go
+++ b/cmd/derper/derper.go
@@ -191,7 +191,12 @@ func main() {
 			http.Error(w, "derp server disabled", http.StatusNotFound)
 		}))
 	}
-	mux.HandleFunc("/derp/probe", probeHandler)
+
+	// These two endpoints are the same. Different versions of the clients
+	// have assumes different paths over time so we support both.
+	mux.HandleFunc("/derp/probe", derphttp.ProbeHandler)
+	mux.HandleFunc("/derp/latency-check", derphttp.ProbeHandler)
+
 	go refreshBootstrapDNSLoop()
 	mux.HandleFunc("/bootstrap-dns", tsweb.BrowserHeaderHandlerFunc(handleBootstrapDNS))
 	mux.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -370,17 +375,6 @@ func isChallengeChar(c rune) bool {
 		c == '.' || c == '-' || c == '_'
 }
 
-// probeHandler is the endpoint that js/wasm clients hit to measure
-// DERP latency, since they can't do UDP STUN queries.
-func probeHandler(w http.ResponseWriter, r *http.Request) {
-	switch r.Method {
-	case "HEAD", "GET":
-		w.Header().Set("Access-Control-Allow-Origin", "*")
-	default:
-		http.Error(w, "bogus probe method", http.StatusMethodNotAllowed)
-	}
-}
-
 var validProdHostname = regexp.MustCompile(`^derp([^.]*)\.tailscale\.com\.?$`)
 
 func prodAutocertHostPolicy(_ context.Context, host string) error {
diff --git a/derp/derphttp/derphttp_server.go b/derp/derphttp/derphttp_server.go
index 81d4c8a4b..d1193f383 100644
--- a/derp/derphttp/derphttp_server.go
+++ b/derp/derphttp/derphttp_server.go
@@ -20,6 +20,16 @@ const fastStartHeader = "Derp-Fast-Start"
 
 func Handler(s *derp.Server) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// These are installed both here and in cmd/derper. The check here
+		// catches both cmd/derper run with DERP disabled (STUN only mode) as
+		// well as DERP being run in tests with derphttp.Handler directly,
+		// as netcheck still assumes this replies.
+		switch r.URL.Path {
+		case "/derp/probe", "/derp/latency-check":
+			ProbeHandler(w, r)
+			return
+		}
+
 		up := strings.ToLower(r.Header.Get("Upgrade"))
 		if up != "websocket" && up != "derp" {
 			if up != "" {
@@ -58,3 +68,14 @@ func Handler(s *derp.Server) http.Handler {
 		s.Accept(r.Context(), netConn, conn, netConn.RemoteAddr().String())
 	})
 }
+
+// ProbeHandler is the endpoint that clients without UDP access (including js/wasm) hit to measure
+// DERP latency, as a replacement for UDP STUN queries.
+func ProbeHandler(w http.ResponseWriter, r *http.Request) {
+	switch r.Method {
+	case "HEAD", "GET":
+		w.Header().Set("Access-Control-Allow-Origin", "*")
+	default:
+		http.Error(w, "bogus probe method", http.StatusMethodNotAllowed)
+	}
+}
diff --git a/derp/derphttp/derphttp_test.go b/derp/derphttp/derphttp_test.go
index 28890f151..5cfd745a8 100644
--- a/derp/derphttp/derphttp_test.go
+++ b/derp/derphttp/derphttp_test.go
@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"net/http/httptest"
 	"net/netip"
 	"sync"
 	"testing"
@@ -464,3 +465,24 @@ func TestLocalAddrNoMutex(t *testing.T) {
 		t.Errorf("got error %q; want %q", got, want)
 	}
 }
+
+func TestProbe(t *testing.T) {
+	h := Handler(nil)
+
+	tests := []struct {
+		path string
+		want int
+	}{
+		{"/derp/probe", 200},
+		{"/derp/latency-check", 200},
+		{"/derp/sdf", http.StatusUpgradeRequired},
+	}
+
+	for _, tt := range tests {
+		rec := httptest.NewRecorder()
+		h.ServeHTTP(rec, httptest.NewRequest("GET", tt.path, nil))
+		if got := rec.Result().StatusCode; got != tt.want {
+			t.Errorf("for path %q got HTTP status %v; want %v", tt.path, got, tt.want)
+		}
+	}
+}