mirror of
https://github.com/tailscale/tailscale.git
synced 2025-07-28 23:04:10 +00:00
client/web: deprecate gorilla/csrf
Replace gorilla/csrf with a handler that requires the Sec-Fetch-Site header to be set to same-origin preventing CSRF attacks. Ref: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Sec-Fetch-Site Ref: https://caniuse.com/mdn-http_headers_sec-fetch-site Browser support should be now sufficiently broad to minimize false-positive rejections. Updates corp#25340 Signed-off-by: Patrick O'Doherty <patrick@tailscale.com>
This commit is contained in:
Patrick O'Doherty
parent
dbf13976d3
commit
3d46418020
@ -6,7 +6,6 @@ package web
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/rand"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
@ -14,14 +13,11 @@ import (
|
||||
"log"
|
||||
"net/http"
|
||||
"net/netip"
|
||||
"os"
|
||||
"path"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"github.com/gorilla/csrf"
|
||||
"tailscale.com/client/local"
|
||||
"tailscale.com/client/tailscale/apitype"
|
||||
"tailscale.com/clientupdate"
|
||||
@ -205,7 +201,7 @@ func NewServer(opts ServerOpts) (s *Server, err error) {
|
||||
|
||||
var metric string
|
||||
s.apiHandler, metric = s.modeAPIHandler(s.mode)
|
||||
s.apiHandler = s.withCSRF(s.apiHandler)
|
||||
s.apiHandler = s.withSecFetchSite(s.apiHandler)
|
||||
|
||||
// Don't block startup on reporting metric.
|
||||
// Report in separate go routine with 5 second timeout.
|
||||
@ -218,23 +214,30 @@ func NewServer(opts ServerOpts) (s *Server, err error) {
|
||||
return s, nil
|
||||
}
|
||||
|
||||
func (s *Server) withCSRF(h http.Handler) http.Handler {
|
||||
csrfProtect := csrf.Protect(s.csrfKey(), csrf.Secure(false))
|
||||
// withSecFetchSite wraps a HTTP handler and ensures requests made to it have
|
||||
// the `Sec-Fetch-Site` header set to same-origin to prevent CSRF attacks.
|
||||
func (s *Server) withSecFetchSite(h http.Handler) http.Handler {
|
||||
// Ref https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Sec-Fetch-Site
|
||||
// Sent by all modern browsers to indicate the relationship between the
|
||||
// origin from which the request was made and the requested resource.
|
||||
// Require that the Sec-Fetch-Site header is set to "same-origin"
|
||||
// for all requests to the web client, preventing CSRF attacks.
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
switch r.Header.Get("Sec-Fetch-Site") {
|
||||
case "same-origin":
|
||||
// valid request
|
||||
case "cross-site", "same-site":
|
||||
// consider anything other than the same-origin to be invalid.
|
||||
http.Error(w, "forbidden non `same-origin` request", http.StatusForbidden)
|
||||
return
|
||||
case "":
|
||||
http.Error(w, "missing required Sec-Fetch-Site request header. You may need to update your browser", http.StatusForbidden)
|
||||
return
|
||||
}
|
||||
// serve all same-site requests.
|
||||
h.ServeHTTP(w, r)
|
||||
})
|
||||
|
||||
// ref https://github.com/tailscale/tailscale/pull/14822
|
||||
// signal to the CSRF middleware that the request is being served over
|
||||
// plaintext HTTP to skip TLS-only header checks.
|
||||
withSetPlaintext := func(h http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
r = csrf.PlaintextHTTPRequest(r)
|
||||
h.ServeHTTP(w, r)
|
||||
})
|
||||
}
|
||||
|
||||
// NB: the order of the withSetPlaintext and csrfProtect calls is important
|
||||
// to ensure that we signal to the CSRF middleware that the request is being
|
||||
// served over plaintext HTTP and not over TLS as it presumes by default.
|
||||
return withSetPlaintext(csrfProtect(h))
|
||||
}
|
||||
|
||||
func (s *Server) modeAPIHandler(mode ServerMode) (http.Handler, string) {
|
||||
@ -450,9 +453,8 @@ func (s *Server) authorizeRequest(w http.ResponseWriter, r *http.Request) (ok bo
|
||||
|
||||
// serveLoginAPI serves requests for the web login client.
|
||||
// It should only be called by Server.ServeHTTP, via Server.apiHandler,
|
||||
// which protects the handler using gorilla csrf.
|
||||
// which protects the handler using s.withSecFetchSite.
|
||||
func (s *Server) serveLoginAPI(w http.ResponseWriter, r *http.Request) {
|
||||
w.Header().Set("X-CSRF-Token", csrf.Token(r))
|
||||
switch {
|
||||
case r.URL.Path == "/api/data" && r.Method == httpm.GET:
|
||||
s.serveGetNodeData(w, r)
|
||||
@ -575,7 +577,6 @@ func (s *Server) serveAPI(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
}
|
||||
|
||||
w.Header().Set("X-CSRF-Token", csrf.Token(r))
|
||||
path := strings.TrimPrefix(r.URL.Path, "/api")
|
||||
switch {
|
||||
case path == "/data" && r.Method == httpm.GET:
|
||||
@ -834,12 +835,11 @@ type nodeData struct {
|
||||
KeyExpiry string // time.RFC3339
|
||||
KeyExpired bool
|
||||
|
||||
TUNMode bool
|
||||
IsSynology bool
|
||||
DSMVersion int // 6 or 7, if IsSynology=true
|
||||
IsUnraid bool
|
||||
UnraidToken string
|
||||
URLPrefix string // if set, the URL prefix the client is served behind
|
||||
TUNMode bool
|
||||
IsSynology bool
|
||||
DSMVersion int // 6 or 7, if IsSynology=true
|
||||
IsUnraid bool
|
||||
URLPrefix string // if set, the URL prefix the client is served behind
|
||||
|
||||
UsingExitNode *exitNode
|
||||
AdvertisingExitNode bool
|
||||
@ -899,7 +899,6 @@ func (s *Server) serveGetNodeData(w http.ResponseWriter, r *http.Request) {
|
||||
IsSynology: distro.Get() == distro.Synology || envknob.Bool("TS_FAKE_SYNOLOGY"),
|
||||
DSMVersion: distro.DSMVersion(),
|
||||
IsUnraid: distro.Get() == distro.Unraid,
|
||||
UnraidToken: os.Getenv("UNRAID_CSRF_TOKEN"),
|
||||
RunningSSHServer: prefs.RunSSH,
|
||||
URLPrefix: strings.TrimSuffix(s.pathPrefix, "/"),
|
||||
ControlAdminURL: prefs.AdminPageURL(),
|
||||
@ -1276,37 +1275,6 @@ func (s *Server) proxyRequestToLocalAPI(w http.ResponseWriter, r *http.Request)
|
||||
}
|
||||
}
|
||||
|
||||
// csrfKey returns a key that can be used for CSRF protection.
|
||||
// If an error occurs during key creation, the error is logged and the active process terminated.
|
||||
// If the server is running in CGI mode, the key is cached to disk and reused between requests.
|
||||
// If an error occurs during key storage, the error is logged and the active process terminated.
|
||||
func (s *Server) csrfKey() []byte {
|
||||
csrfFile := filepath.Join(os.TempDir(), "tailscale-web-csrf.key")
|
||||
|
||||
// if running in CGI mode, try to read from disk, but ignore errors
|
||||
if s.cgiMode {
|
||||
key, _ := os.ReadFile(csrfFile)
|
||||
if len(key) == 32 {
|
||||
return key
|
||||
}
|
||||
}
|
||||
|
||||
// create a new key
|
||||
key := make([]byte, 32)
|
||||
if _, err := rand.Read(key); err != nil {
|
||||
log.Fatalf("error generating CSRF key: %v", err)
|
||||
}
|
||||
|
||||
// if running in CGI mode, try to write the newly created key to disk, and exit if it fails.
|
||||
if s.cgiMode {
|
||||
if err := os.WriteFile(csrfFile, key, 0600); err != nil {
|
||||
log.Fatalf("unable to store CSRF key: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
return key
|
||||
}
|
||||
|
||||
// enforcePrefix returns a HandlerFunc that enforces a given path prefix is used in requests,
|
||||
// then strips it before invoking h.
|
||||
// Unlike http.StripPrefix, it does not return a 404 if the prefix is not present.
|
||||
|
||||
@ -11,7 +11,6 @@ import (
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/cookiejar"
|
||||
"net/http/httptest"
|
||||
"net/netip"
|
||||
"net/url"
|
||||
@ -21,7 +20,6 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/google/go-cmp/cmp"
|
||||
"github.com/gorilla/csrf"
|
||||
"tailscale.com/client/local"
|
||||
"tailscale.com/client/tailscale/apitype"
|
||||
"tailscale.com/ipn"
|
||||
@ -1491,82 +1489,77 @@ func mockWaitAuthURL(_ context.Context, id string, src tailcfg.NodeID) (*tailcfg
|
||||
}
|
||||
}
|
||||
|
||||
func TestCSRFProtect(t *testing.T) {
|
||||
s := &Server{}
|
||||
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("GET /test/csrf-token", func(w http.ResponseWriter, r *http.Request) {
|
||||
token := csrf.Token(r)
|
||||
_, err := io.WriteString(w, token)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
})
|
||||
mux.HandleFunc("POST /test/csrf-protected", func(w http.ResponseWriter, r *http.Request) {
|
||||
_, err := io.WriteString(w, "ok")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
})
|
||||
h := s.withCSRF(mux)
|
||||
ser := nettest.NewHTTPServer(nettest.GetNetwork(t), h)
|
||||
defer ser.Close()
|
||||
|
||||
jar, err := cookiejar.New(nil)
|
||||
if err != nil {
|
||||
t.Fatalf("unable to construct cookie jar: %v", err)
|
||||
func TestSecFetchSiteProtect(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
withSameOrigin bool
|
||||
withSameSite bool
|
||||
withCrossSite bool
|
||||
expectError bool
|
||||
}{
|
||||
{
|
||||
name: "requests with same-origin pass",
|
||||
withSameOrigin: true,
|
||||
},
|
||||
{
|
||||
name: "requests with same-site fail",
|
||||
withSameSite: true,
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "requests with cross-site fail",
|
||||
withCrossSite: true,
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "requests with no Sec-Fetch-Site fail",
|
||||
expectError: true,
|
||||
},
|
||||
}
|
||||
|
||||
client := ser.Client()
|
||||
client.Jar = jar
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
s := &Server{}
|
||||
mux := http.NewServeMux()
|
||||
|
||||
// make GET request to populate cookie jar
|
||||
resp, err := client.Get(ser.URL + "/test/csrf-token")
|
||||
if err != nil {
|
||||
t.Fatalf("unable to make request: %v", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("unexpected status: %v", resp.Status)
|
||||
}
|
||||
tokenBytes, err := io.ReadAll(resp.Body)
|
||||
if err != nil {
|
||||
t.Fatalf("unable to read body: %v", err)
|
||||
}
|
||||
mux.HandleFunc("POST /protected", func(w http.ResponseWriter, r *http.Request) {
|
||||
_, err := io.WriteString(w, "ok")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
})
|
||||
|
||||
csrfToken := strings.TrimSpace(string(tokenBytes))
|
||||
if csrfToken == "" {
|
||||
t.Fatal("empty csrf token")
|
||||
}
|
||||
h := s.withSecFetchSite(mux)
|
||||
|
||||
// make a POST request without the CSRF header; ensure it fails
|
||||
resp, err = client.Post(ser.URL+"/test/csrf-protected", "text/plain", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("unable to make request: %v", err)
|
||||
}
|
||||
if resp.StatusCode != http.StatusForbidden {
|
||||
t.Fatalf("unexpected status: %v", resp.Status)
|
||||
}
|
||||
serv := nettest.NewHTTPServer(nettest.GetNetwork(t), h)
|
||||
defer serv.Close()
|
||||
|
||||
// make a POST request with the CSRF header; ensure it succeeds
|
||||
req, err := http.NewRequest("POST", ser.URL+"/test/csrf-protected", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("error building request: %v", err)
|
||||
}
|
||||
req.Header.Set("X-CSRF-Token", csrfToken)
|
||||
resp, err = client.Do(req)
|
||||
if err != nil {
|
||||
t.Fatalf("unable to make request: %v", err)
|
||||
}
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("unexpected status: %v", resp.Status)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
out, err := io.ReadAll(resp.Body)
|
||||
if err != nil {
|
||||
t.Fatalf("unable to read body: %v", err)
|
||||
}
|
||||
if string(out) != "ok" {
|
||||
t.Fatalf("unexpected body: %q", out)
|
||||
client := serv.Client()
|
||||
|
||||
req, err := http.NewRequest("POST", serv.URL+"/protected", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("error building request: %v", err)
|
||||
}
|
||||
|
||||
if tt.withSameOrigin {
|
||||
req.Header.Set("Sec-Fetch-Site", "same-origin")
|
||||
} else if tt.withSameSite {
|
||||
req.Header.Set("Sec-Fetch-Site", "same-site")
|
||||
} else if tt.withCrossSite {
|
||||
req.Header.Set("Sec-Fetch-Site", "cross-site")
|
||||
}
|
||||
|
||||
resp, err := client.Do(req)
|
||||
if err != nil {
|
||||
t.Fatalf("unable to make request: %v", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if tt.expectError && resp.StatusCode == http.StatusOK {
|
||||
t.Fatalf("expected error but got ok")
|
||||
} else if !tt.expectError && resp.StatusCode != http.StatusOK {
|
||||
t.Fatalf("expected ok got %q", resp.Status)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@ -144,8 +144,6 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/
|
||||
L github.com/google/nftables/internal/parseexprfunc from github.com/google/nftables+
|
||||
L github.com/google/nftables/xt from github.com/google/nftables/expr+
|
||||
github.com/google/uuid from github.com/prometheus-community/pro-bing+
|
||||
github.com/gorilla/csrf from tailscale.com/client/web
|
||||
github.com/gorilla/securecookie from github.com/gorilla/csrf
|
||||
github.com/hdevalence/ed25519consensus from tailscale.com/clientupdate/distsign+
|
||||
L 💣 github.com/illarion/gonotify/v3 from tailscale.com/net/dns
|
||||
L github.com/illarion/gonotify/v3/syscallf from github.com/illarion/gonotify/v3
|
||||
@ -1131,13 +1129,12 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/
|
||||
W debug/dwarf from debug/pe
|
||||
W debug/pe from github.com/dblohm7/wingoes/pe
|
||||
embed from github.com/tailscale/web-client-prebuilt+
|
||||
encoding from encoding/gob+
|
||||
encoding from encoding/json+
|
||||
encoding/asn1 from crypto/x509+
|
||||
encoding/base32 from github.com/fxamacker/cbor/v2+
|
||||
encoding/base64 from encoding/json+
|
||||
encoding/binary from compress/gzip+
|
||||
encoding/csv from github.com/spf13/pflag
|
||||
encoding/gob from github.com/gorilla/securecookie
|
||||
encoding/hex from crypto/x509+
|
||||
encoding/json from expvar+
|
||||
encoding/pem from crypto/tls+
|
||||
@ -1159,7 +1156,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/
|
||||
hash/fnv from google.golang.org/protobuf/internal/detrand
|
||||
hash/maphash from go4.org/mem
|
||||
html from html/template+
|
||||
html/template from github.com/gorilla/csrf+
|
||||
html/template from tailscale.com/util/eventbus
|
||||
internal/abi from crypto/x509/internal/macos+
|
||||
internal/asan from internal/runtime/maps+
|
||||
internal/bisect from internal/godebug
|
||||
@ -1191,7 +1188,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/
|
||||
internal/runtime/math from internal/runtime/maps+
|
||||
internal/runtime/sys from crypto/subtle+
|
||||
L internal/runtime/syscall from runtime+
|
||||
internal/saferio from debug/pe+
|
||||
W internal/saferio from debug/pe
|
||||
internal/singleflight from net
|
||||
internal/stringslite from embed+
|
||||
internal/sync from sync+
|
||||
|
||||
@ -27,8 +27,6 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
|
||||
L github.com/google/nftables/internal/parseexprfunc from github.com/google/nftables+
|
||||
L github.com/google/nftables/xt from github.com/google/nftables/expr+
|
||||
DW github.com/google/uuid from tailscale.com/clientupdate+
|
||||
github.com/gorilla/csrf from tailscale.com/client/web
|
||||
github.com/gorilla/securecookie from github.com/gorilla/csrf
|
||||
github.com/hdevalence/ed25519consensus from tailscale.com/clientupdate/distsign+
|
||||
L 💣 github.com/jsimonetti/rtnetlink from tailscale.com/net/netmon
|
||||
L github.com/jsimonetti/rtnetlink/internal/unix from github.com/jsimonetti/rtnetlink
|
||||
@ -318,12 +316,11 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
|
||||
W debug/dwarf from debug/pe
|
||||
W debug/pe from github.com/dblohm7/wingoes/pe
|
||||
embed from github.com/peterbourgon/ff/v3+
|
||||
encoding from encoding/gob+
|
||||
encoding from encoding/json+
|
||||
encoding/asn1 from crypto/x509+
|
||||
encoding/base32 from github.com/fxamacker/cbor/v2+
|
||||
encoding/base64 from encoding/json+
|
||||
encoding/binary from compress/gzip+
|
||||
encoding/gob from github.com/gorilla/securecookie
|
||||
encoding/hex from crypto/x509+
|
||||
encoding/json from expvar+
|
||||
encoding/pem from crypto/tls+
|
||||
@ -337,7 +334,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
|
||||
hash/crc32 from compress/gzip+
|
||||
hash/maphash from go4.org/mem
|
||||
html from html/template+
|
||||
html/template from github.com/gorilla/csrf+
|
||||
html/template from tailscale.com/util/eventbus
|
||||
image from github.com/skip2/go-qrcode+
|
||||
image/color from github.com/skip2/go-qrcode+
|
||||
image/png from github.com/skip2/go-qrcode
|
||||
@ -371,7 +368,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
|
||||
internal/runtime/math from internal/runtime/maps+
|
||||
internal/runtime/sys from crypto/subtle+
|
||||
L internal/runtime/syscall from runtime+
|
||||
internal/saferio from debug/pe+
|
||||
W internal/saferio from debug/pe
|
||||
internal/singleflight from net
|
||||
internal/stringslite from embed+
|
||||
internal/sync from sync+
|
||||
|
||||
@ -116,8 +116,6 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
|
||||
L github.com/google/nftables/internal/parseexprfunc from github.com/google/nftables+
|
||||
L github.com/google/nftables/xt from github.com/google/nftables/expr+
|
||||
DW github.com/google/uuid from tailscale.com/clientupdate+
|
||||
github.com/gorilla/csrf from tailscale.com/client/web
|
||||
github.com/gorilla/securecookie from github.com/gorilla/csrf
|
||||
github.com/hdevalence/ed25519consensus from tailscale.com/clientupdate/distsign+
|
||||
L 💣 github.com/illarion/gonotify/v3 from tailscale.com/net/dns
|
||||
L github.com/illarion/gonotify/v3/syscallf from github.com/illarion/gonotify/v3
|
||||
@ -580,12 +578,11 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
|
||||
W debug/dwarf from debug/pe
|
||||
W debug/pe from github.com/dblohm7/wingoes/pe
|
||||
embed from github.com/tailscale/web-client-prebuilt+
|
||||
encoding from encoding/gob+
|
||||
encoding from encoding/json+
|
||||
encoding/asn1 from crypto/x509+
|
||||
encoding/base32 from github.com/fxamacker/cbor/v2+
|
||||
encoding/base64 from encoding/json+
|
||||
encoding/binary from compress/gzip+
|
||||
encoding/gob from github.com/gorilla/securecookie
|
||||
encoding/hex from crypto/x509+
|
||||
encoding/json from expvar+
|
||||
encoding/pem from crypto/tls+
|
||||
@ -599,7 +596,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
|
||||
hash/crc32 from compress/gzip+
|
||||
hash/maphash from go4.org/mem
|
||||
html from html/template+
|
||||
html/template from github.com/gorilla/csrf+
|
||||
html/template from tailscale.com/util/eventbus
|
||||
internal/abi from crypto/x509/internal/macos+
|
||||
internal/asan from internal/runtime/maps+
|
||||
internal/bisect from internal/godebug
|
||||
@ -630,7 +627,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
|
||||
internal/runtime/math from internal/runtime/maps+
|
||||
internal/runtime/sys from crypto/subtle+
|
||||
L internal/runtime/syscall from runtime+
|
||||
internal/saferio from debug/pe+
|
||||
W internal/saferio from debug/pe
|
||||
internal/singleflight from net
|
||||
internal/stringslite from embed+
|
||||
internal/sync from sync+
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user