derp/derphttp,ipn/localapi,net/captivedetection: add cache resistance to captive portal detection

Observed on some airlines (British Airways, WestJet), Squid is
configured to cache and transform these results, which is disruptive.
The server and client should both actively request that this is not done
by setting Cache-Control headers.

Send a timestamp parameter to further work against caches that do not
respect the cache-control headers.

Updates #14856

Signed-off-by: James Tucker <james@tailscale.com>

derp/derphttp/derphttp_server.go

@@ -98,6 +98,7 @@ func ServeNoContent(w http.ResponseWriter, r *http.Request) {
 			w.Header().Set(NoContentResponseHeader, "response "+challenge)
 		}
 	}
+	w.Header().Set("Cache-Control", "no-cache, no-store, must-revalidate, no-transform, max-age=0")
 	w.WriteHeader(http.StatusNoContent)
 }
 
@@ -105,7 +106,7 @@ func isChallengeChar(c rune) bool {
 	// Semi-randomly chosen as a limited set of valid characters
 	return ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') ||
 		('0' <= c && c <= '9') ||
-		c == '.' || c == '-' || c == '_'
+		c == '.' || c == '-' || c == '_' || c == ':'
 }
 
 const (

ipn/localapi/debugderp.go

@@ -231,8 +231,14 @@ func (h *Handler) serveDebugDERPRegion(w http.ResponseWriter, r *http.Request) {
 		connSuccess := checkConn(derpNode)
 
 		// Verify that the /generate_204 endpoint works
-		captivePortalURL := "http://" + derpNode.HostName + "/generate_204"
+		captivePortalURL := fmt.Sprintf("http://%s/generate_204?t=%d", derpNode.HostName, time.Now().Unix())
-		resp, err := client.Get(captivePortalURL)
+		req, err := http.NewRequest("GET", captivePortalURL, nil)
+		if err != nil {
+			st.Warnings = append(st.Warnings, fmt.Sprintf("Internal error creating request for captive portal check: %v", err))
+			continue
+		}
+		req.Header.Set("Cache-Control", "no-cache, no-store, must-revalidate, no-transform, max-age=0")
+		resp, err := client.Do(req)
 		if err != nil {
 			st.Warnings = append(st.Warnings, fmt.Sprintf("Error making request to the captive portal check %q; is port 80 blocked?", captivePortalURL))
 		} else {

net/captivedetection/captivedetection.go

@@ -11,6 +11,7 @@ import (
 	"net"
 	"net/http"
 	"runtime"
+	"strconv"
 	"strings"
 	"sync"
 	"syscall"
@@ -23,6 +24,7 @@ import (
 
 // Detector checks whether the system is behind a captive portal.
 type Detector struct {
+	clock func() time.Time
 
 	// httpClient is the HTTP client that is used for captive portal detection. It is configured
 	// to not follow redirects, have a short timeout and no keep-alive.
@@ -52,6 +54,13 @@ func NewDetector(logf logger.Logf) *Detector {
 	return d
 }
 
+func (d *Detector) Now() time.Time {
+	if d.clock != nil {
+		return d.clock()
+	}
+	return time.Now()
+}
+
 // Timeout is the timeout for captive portal detection requests. Because the captive portal intercepting our requests
 // is usually located on the LAN, this is a relatively short timeout.
 const Timeout = 3 * time.Second
@@ -187,10 +196,16 @@ func (d *Detector) verifyCaptivePortalEndpoint(ctx context.Context, e Endpoint,
 	ctx, cancel := context.WithTimeout(ctx, Timeout)
 	defer cancel()
 
-	req, err := http.NewRequestWithContext(ctx, "GET", e.URL.String(), nil)
+	u := *e.URL
+	v := u.Query()
+	v.Add("t", strconv.Itoa(int(d.Now().Unix())))
+	u.RawQuery = v.Encode()
+
+	req, err := http.NewRequestWithContext(ctx, "GET", u.String(), nil)
 	if err != nil {
 		return false, err
 	}
+	req.Header.Set("Cache-Control", "no-cache, no-store, must-revalidate, no-transform, max-age=0")
 
 	// Attach the Tailscale challenge header if the endpoint supports it. Not all captive portal detection endpoints
 	// support this, so we only attach it if the endpoint does.

net/captivedetection/captivedetection_test.go

@@ -5,14 +5,21 @@ package captivedetection
 
 import (
 	"context"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"runtime"
+	"strconv"
 	"sync"
 	"sync/atomic"
 	"testing"
+	"time"
 
+	"tailscale.com/derp/derphttp"
 	"tailscale.com/net/netmon"
 	"tailscale.com/syncs"
 	"tailscale.com/tstest/nettest"
+	"tailscale.com/util/must"
 )
 
 func TestAvailableEndpointsAlwaysAtLeastTwo(t *testing.T) {
@@ -81,3 +88,67 @@ func TestEndpointsAreUpAndReturnExpectedResponse(t *testing.T) {
 		t.Errorf("no good endpoints found")
 	}
 }
+
+func TestCaptivePortalRequest(t *testing.T) {
+	d := NewDetector(t.Logf)
+	now := time.Now()
+	d.clock = func() time.Time { return now }
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "GET" {
+			t.Errorf("expected GET, got %q", r.Method)
+		}
+		if r.URL.Path != "/generate_204" {
+			t.Errorf("expected /generate_204, got %q", r.URL.Path)
+		}
+		q := r.URL.Query()
+		if got, want := q.Get("t"), strconv.Itoa(int(now.Unix())); got != want {
+			t.Errorf("timestamp param; got %v, want %v", got, want)
+		}
+		w.Header().Set("X-Tailscale-Response", "response "+r.Header.Get("X-Tailscale-Challenge"))
+
+		w.WriteHeader(http.StatusNoContent)
+	}))
+	defer s.Close()
+
+	e := Endpoint{
+		URL:                        must.Get(url.Parse(s.URL + "/generate_204")),
+		StatusCode:                 204,
+		ExpectedContent:            "",
+		SupportsTailscaleChallenge: true,
+	}
+
+	found, err := d.verifyCaptivePortalEndpoint(ctx, e, 0)
+	if err != nil {
+		t.Fatalf("verifyCaptivePortalEndpoint = %v, %v", found, err)
+	}
+	if found {
+		t.Errorf("verifyCaptivePortalEndpoint = %v, want false", found)
+	}
+}
+
+func TestAgainstDERPHandler(t *testing.T) {
+	d := NewDetector(t.Logf)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	s := httptest.NewServer(http.HandlerFunc(derphttp.ServeNoContent))
+	defer s.Close()
+	e := Endpoint{
+		URL:                        must.Get(url.Parse(s.URL + "/generate_204")),
+		StatusCode:                 204,
+		ExpectedContent:            "",
+		SupportsTailscaleChallenge: true,
+	}
+	found, err := d.verifyCaptivePortalEndpoint(ctx, e, 0)
+	if err != nil {
+		t.Fatalf("verifyCaptivePortalEndpoint = %v, %v", found, err)
+	}
+	if found {
+		t.Errorf("verifyCaptivePortalEndpoint = %v, want false", found)
+	}
+}