diff --git a/derp/derphttp/derphttp_server.go b/derp/derphttp/derphttp_server.go index ed7d3d707..50aba774a 100644 --- a/derp/derphttp/derphttp_server.go +++ b/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 ( diff --git a/ipn/localapi/debugderp.go b/ipn/localapi/debugderp.go index dbdf5cf79..6636fd253 100644 --- a/ipn/localapi/debugderp.go +++ b/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" - resp, err := client.Get(captivePortalURL) + captivePortalURL := fmt.Sprintf("http://%s/generate_204?t=%d", derpNode.HostName, time.Now().Unix()) + 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 { diff --git a/net/captivedetection/captivedetection.go b/net/captivedetection/captivedetection.go index 7d598d853..a06362a5b 100644 --- a/net/captivedetection/captivedetection.go +++ b/net/captivedetection/captivedetection.go @@ -11,6 +11,7 @@ "net" "net/http" "runtime" + "strconv" "strings" "sync" "syscall" @@ -23,6 +24,7 @@ // 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. diff --git a/net/captivedetection/captivedetection_test.go b/net/captivedetection/captivedetection_test.go index 29a197d31..064a86c8c 100644 --- a/net/captivedetection/captivedetection_test.go +++ b/net/captivedetection/captivedetection_test.go @@ -5,14 +5,21 @@ 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) + } +}