From 0509984b47fbc208b30b51f02a1835c9fba49245 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Sat, 21 Dec 2024 10:53:50 -0700 Subject: [PATCH] Remove the remaining bits of unneeded CSRF code Additionally expand upon the godocs for our various middlewares so that it's clear which ones provide CSRF protection. --- integration/appaccess/pack.go | 11 ++--------- integration/helpers/instance.go | 12 ------------ integration/helpers/web.go | 8 -------- lib/client/weblogin.go | 17 ----------------- lib/httplib/csrf/csrf.go | 18 +++++++----------- lib/web/apiserver.go | 12 ++++++++++++ 6 files changed, 21 insertions(+), 57 deletions(-) diff --git a/integration/appaccess/pack.go b/integration/appaccess/pack.go index 609a60adca036..cf7afae964fe6 100644 --- a/integration/appaccess/pack.go +++ b/integration/appaccess/pack.go @@ -49,7 +49,6 @@ import ( "github.com/gravitational/teleport/lib/client" "github.com/gravitational/teleport/lib/cryptosuites" "github.com/gravitational/teleport/lib/events" - "github.com/gravitational/teleport/lib/httplib/csrf" "github.com/gravitational/teleport/lib/httplib/reverseproxy" "github.com/gravitational/teleport/lib/reversetunnelclient" "github.com/gravitational/teleport/lib/service" @@ -251,15 +250,9 @@ func (p *Pack) initWebSession(t *testing.T) { req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewBuffer(csReq)) require.NoError(t, err) - // Attach CSRF token in cookie and header. - csrfToken, err := utils.CryptoRandomHex(32) - require.NoError(t, err) - req.AddCookie(&http.Cookie{ - Name: csrf.CookieName, - Value: csrfToken, - }) + // Set Content-Type header, otherwise Teleport's CSRF protection will + // reject the request. req.Header.Set("Content-Type", "application/json; charset=utf-8") - req.Header.Set(csrf.HeaderName, csrfToken) // Issue request. client := &http.Client{ diff --git a/integration/helpers/instance.go b/integration/helpers/instance.go index b132b027b3bef..f6b25459ea275 100644 --- a/integration/helpers/instance.go +++ b/integration/helpers/instance.go @@ -60,7 +60,6 @@ import ( "github.com/gravitational/teleport/lib/cryptosuites" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/events" - "github.com/gravitational/teleport/lib/httplib/csrf" "github.com/gravitational/teleport/lib/observability/tracing" "github.com/gravitational/teleport/lib/reversetunnel" "github.com/gravitational/teleport/lib/reversetunnelclient" @@ -1531,18 +1530,7 @@ func CreateWebSession(proxyHost, user, password string) (*web.CreateSessionRespo return nil, nil, trace.Wrap(err) } - // Attach CSRF token in cookie and header. - csrfToken, err := utils.CryptoRandomHex(32) - if err != nil { - return nil, nil, trace.Wrap(err) - } - - req.AddCookie(&http.Cookie{ - Name: csrf.CookieName, - Value: csrfToken, - }) req.Header.Set("Content-Type", "application/json; charset=utf-8") - req.Header.Set(csrf.HeaderName, csrfToken) // Issue request. httpClient := &http.Client{ diff --git a/integration/helpers/web.go b/integration/helpers/web.go index 5cfc4b87f64b6..b5a96c5d72619 100644 --- a/integration/helpers/web.go +++ b/integration/helpers/web.go @@ -68,15 +68,7 @@ func LoginWebClient(t *testing.T, host, username, password string) *WebClientPac req, err := http.NewRequest(http.MethodPost, u.String(), bytes.NewBuffer(csReq)) require.NoError(t, err) - // Attach CSRF token in cookie and header. - csrfToken, err := utils.CryptoRandomHex(32) - require.NoError(t, err) - req.AddCookie(&http.Cookie{ - Name: csrf.CookieName, - Value: csrfToken, - }) req.Header.Set("Content-Type", "application/json; charset=utf-8") - req.Header.Set(csrf.HeaderName, csrfToken) // Issue request. client := &http.Client{ diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index c3415e340417d..1319507ff17ee 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -21,9 +21,7 @@ package client import ( "bytes" "context" - "crypto/rand" "crypto/x509" - "encoding/hex" "encoding/json" "fmt" "io" @@ -50,7 +48,6 @@ import ( wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/httplib" - "github.com/gravitational/teleport/lib/httplib/csrf" websession "github.com/gravitational/teleport/lib/web/session" ) @@ -911,12 +908,6 @@ func SSHAgentLoginWeb(ctx context.Context, login SSHLoginDirect) (*WebClient, ty return nil, nil, trace.Wrap(err) } - token := make([]byte, 32) - if _, err := rand.Read(token); err != nil { - return nil, nil, trace.Wrap(err) - } - - csrfToken := hex.EncodeToString(token) resp, err := httplib.ConvertResponse(clt.RoundTrip(func() (*http.Response, error) { var buf bytes.Buffer if err := json.NewEncoder(&buf).Encode(&CreateWebSessionReq{ @@ -932,15 +923,7 @@ func SSHAgentLoginWeb(ctx context.Context, login SSHLoginDirect) (*WebClient, ty return nil, err } - cookie := &http.Cookie{ - Name: csrf.CookieName, - Value: csrfToken, - } - - req.AddCookie(cookie) - req.Header.Set("Content-Type", "application/json") - req.Header.Set(csrf.HeaderName, csrfToken) return clt.HTTPClient().Do(req) })) if err != nil { diff --git a/lib/httplib/csrf/csrf.go b/lib/httplib/csrf/csrf.go index 376db98db73ab..87bca31f089c7 100644 --- a/lib/httplib/csrf/csrf.go +++ b/lib/httplib/csrf/csrf.go @@ -29,17 +29,13 @@ import ( "github.com/gravitational/teleport/lib/utils" ) -const ( - // CookieName is the name of the CSRF cookie used to protect against login - // CSRF in Teleport's SSO flows. - // - // It's prefixed with "__Host-" as an additional defense in depth measure. - // This makes sure the cookie is sent from a secure page (HTTPS), - // won't be sent to subdomains, and the path attribute is set to /. - CookieName = "__Host-grv_csrf" - // HeaderName is the default HTTP request header to inspect. - HeaderName = "X-CSRF-Token" -) +// CookieName is the name of the CSRF cookie used to protect against login +// CSRF in Teleport's SSO flows. +// +// It's prefixed with "__Host-" as an additional defense in depth measure. +// This makes sure the cookie is sent from a secure page (HTTPS), +// won't be sent to subdomains, and the path attribute is set to /. +const CookieName = "__Host-grv_csrf" // tokenLenBytes is the length of a raw CSRF token prior to encoding const tokenLenBytes = 32 diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 294e5ecee5117..fb24932a6ecf7 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -4399,6 +4399,8 @@ type ClusterWebsocketHandler func(w http.ResponseWriter, r *http.Request, p http // WithClusterAuth wraps a ClusterHandler to ensure that a request is authenticated to this proxy // (the same as WithAuth), as well as to grab the remoteSite (which can represent this local cluster // or a remote trusted cluster) as specified by the ":site" url parameter. +// +// WithClusterAuth also provides CSRF protection by requiring the bearer token to be present. func (h *Handler) WithClusterAuth(fn ClusterHandler) httprouter.Handle { return httplib.MakeHandler(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) { sctx, site, err := h.authenticateRequestWithCluster(w, r, p) @@ -4694,6 +4696,7 @@ func (h *Handler) WithMetaRedirect(fn redirectHandlerFunc) httprouter.Handle { // WithAuth ensures that a request is authenticated. // Authenticated requests require both a session cookie as well as a bearer token. +// WithAuth also provides CSRF protection by requiring the bearer token to be present. func (h *Handler) WithAuth(fn ContextHandler) httprouter.Handle { return httplib.MakeHandler(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) { sctx, err := h.AuthenticateRequest(w, r, true /* check bearer token */) @@ -4706,6 +4709,10 @@ func (h *Handler) WithAuth(fn ContextHandler) httprouter.Handle { // WithSession ensures that the request provides a session cookie. // It does not check for a bearer token. +// +// WithSession does not provide CSRF protection, so it should only +// be used for non-state-changing requests or when other CSRF mitigations +// are applied. func (h *Handler) WithSession(fn ContextHandler) httprouter.Handle { return httplib.MakeHandler(func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) { sctx, err := h.AuthenticateRequest(w, r, false /* check bearer token */) @@ -5118,6 +5125,11 @@ type SSOCallbackResponse struct { // error is returned. func SSOSetWebSessionAndRedirectURL(w http.ResponseWriter, r *http.Request, response *SSOCallbackResponse, verifyCSRF bool) error { if verifyCSRF { + // Make sure that the CSRF token provided in this request matches + // the token that was used to initiate this SSO attempt. + // + // This ensures that an attacker cannot perform a login CSRF attack + // in order to get the victim to log in to the incorrect account. if err := csrf.VerifyToken(response.CSRFToken, r); err != nil { return trace.Wrap(err) }