From c222030e3f2eff643d90d30f4b634482fa896ba2 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Wed, 18 Dec 2024 15:08:17 -0700 Subject: [PATCH] Remove CSRF checking middleware (#50358) The remaining two endpoints that were checking the CSRF token were both unauthenticated requests. We don't need a CSRF token here because we require Content-Type: application/json for these requests. --- lib/httplib/httplib.go | 19 +---------- lib/web/apiserver.go | 29 ++-------------- lib/web/apiserver_test.go | 64 +++++++----------------------------- lib/web/login_helper_test.go | 22 +++---------- 4 files changed, 18 insertions(+), 116 deletions(-) diff --git a/lib/httplib/httplib.go b/lib/httplib/httplib.go index 98775ec6904..f241f6d36dd 100644 --- a/lib/httplib/httplib.go +++ b/lib/httplib/httplib.go @@ -41,7 +41,6 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/observability/tracing" tracehttp "github.com/gravitational/teleport/api/observability/tracing/http" - "github.com/gravitational/teleport/lib/httplib/csrf" "github.com/gravitational/teleport/lib/utils" ) @@ -155,23 +154,6 @@ func MakeStdHandlerWithErrorWriter(fn StdHandlerFunc, errWriter ErrorWriter) htt } } -// WithCSRFProtection ensures that request to unauthenticated API is checked against CSRF attacks -func WithCSRFProtection(fn HandlerFunc) httprouter.Handle { - handlerFn := MakeHandler(fn) - return func(w http.ResponseWriter, r *http.Request, p httprouter.Params) { - if r.Method != http.MethodGet && r.Method != http.MethodHead { - errHeader := csrf.VerifyHTTPHeader(r) - errForm := csrf.VerifyFormField(r) - if errForm != nil && errHeader != nil { - slog.WarnContext(r.Context(), "unable to validate CSRF token", "header_error", errHeader, "form_error", errForm) - trace.WriteError(w, trace.AccessDenied("access denied")) - return - } - } - handlerFn(w, r, p) - } -} - // ReadJSON reads HTTP json request and unmarshals it // into passed any obj. A reasonable maximum size is enforced // to mitigate resource exhaustion attacks. @@ -188,6 +170,7 @@ func ReadResourceJSON(r *http.Request, val any) error { func readJSON(r *http.Request, val any, maxSize int64) error { // Check content type to mitigate CSRF attack. + // (Form POST requests don't support application/json payloads.) contentType, _, err := mime.ParseMediaType(r.Header.Get("Content-Type")) if err != nil { slog.WarnContext(r.Context(), "Error parsing media type for reading JSON", "error", err) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index a21d165e382..b88f4c0102e 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -780,7 +780,7 @@ func (h *Handler) bindDefaultEndpoints() { h.POST("/webapi/sessions/app", h.WithAuth(h.createAppSession)) // Web sessions - h.POST("/webapi/sessions/web", httplib.WithCSRFProtection(h.WithLimiterHandlerFunc(h.createWebSession))) + h.POST("/webapi/sessions/web", h.WithLimiter(h.createWebSession)) h.DELETE("/webapi/sessions/web", h.WithAuth(h.deleteWebSession)) h.POST("/webapi/sessions/web/renew", h.WithAuth(h.renewWebSession)) h.POST("/webapi/users", h.WithAuth(h.createUserHandle)) @@ -793,7 +793,7 @@ func (h *Handler) bindDefaultEndpoints() { // h.GET("/webapi/users/password/token/:token", h.WithLimiter(h.getResetPasswordTokenHandle)) h.GET("/webapi/users/*wildcard", h.handleGetUserOrResetToken) - h.PUT("/webapi/users/password/token", httplib.WithCSRFProtection(h.changeUserAuthentication)) + h.PUT("/webapi/users/password/token", h.WithLimiter(h.changeUserAuthentication)) h.PUT("/webapi/users/password", h.WithAuth(h.changePassword)) h.POST("/webapi/users/password/token", h.WithAuth(h.createResetPasswordToken)) h.POST("/webapi/users/privilege/token", h.WithAuth(h.createPrivilegeTokenHandle)) @@ -1994,7 +1994,6 @@ func (h *Handler) githubLoginWeb(w http.ResponseWriter, r *http.Request, p httpr } response, err := h.cfg.ProxyClient.CreateGithubAuthRequest(r.Context(), types.GithubAuthRequest{ - CSRFToken: req.CSRFToken, ConnectorID: req.ConnectorID, CreateWebSession: true, ClientRedirectURL: req.ClientRedirectURL, @@ -2004,7 +2003,6 @@ func (h *Handler) githubLoginWeb(w http.ResponseWriter, r *http.Request, p httpr if err != nil { logger.WithError(err).Error("Error creating auth request.") return client.LoginFailedRedirectURL - } return response.RedirectURL @@ -4705,21 +4703,6 @@ func (h *Handler) WithSession(fn ContextHandler) httprouter.Handle { }) } -// WithAuthCookieAndCSRF ensures that a request is authenticated -// for plain old non-AJAX requests (does not check the Bearer header). -// It enforces CSRF checks (except for "safe" methods). -func (h *Handler) WithAuthCookieAndCSRF(fn ContextHandler) httprouter.Handle { - f := func(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) { - sctx, err := h.AuthenticateRequest(w, r, false) - if err != nil { - return nil, trace.Wrap(err) - } - return fn(w, r, p, sctx) - } - - return httplib.WithCSRFProtection(f) -} - // WithUnauthenticatedLimiter adds a conditional IP-based rate limiting that will limit only unauthenticated requests. // This is a good default to use as both Cluster and User auth are checked here, but `WithLimiter` can be used if // you're certain that no authenticated requests will be made. @@ -5054,8 +5037,6 @@ type SSORequestParams struct { // ConnectorID identifies the SSO connector to use to log in, from // the connector_id query parameter. ConnectorID string - // CSRFToken is the token in the CSRF cookie header. - CSRFToken string } // ParseSSORequestParams extracts the SSO request parameters from an http.Request, @@ -5088,15 +5069,9 @@ func ParseSSORequestParams(r *http.Request) (*SSORequestParams, error) { return nil, trace.BadParameter("missing connector_id query parameter") } - csrfToken, err := csrf.ExtractTokenFromCookie(r) - if err != nil { - return nil, trace.Wrap(err) - } - return &SSORequestParams{ ClientRedirectURL: clientRedirectURL, ConnectorID: connectorID, - CSRFToken: csrfToken, }, nil } diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 962d52514ca..48a9ff61179 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -117,7 +117,6 @@ import ( "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/events/eventstest" "github.com/gravitational/teleport/lib/httplib" - "github.com/gravitational/teleport/lib/httplib/csrf" "github.com/gravitational/teleport/lib/inventory" kubeproxy "github.com/gravitational/teleport/lib/kube/proxy" "github.com/gravitational/teleport/lib/limiter" @@ -947,10 +946,6 @@ func TestWebSessionsCRUD(t *testing.T) { func TestCSRF(t *testing.T) { t.Parallel() s := newWebSuite(t) - type input struct { - reqToken string - cookieToken string - } // create a valid user user := "csrfuser" @@ -958,39 +953,25 @@ func TestCSRF(t *testing.T) { otpSecret := newOTPSharedSecret() s.createUser(t, user, user, pass, otpSecret) - encodedToken1 := "2ebcb768d0090ea4368e42880c970b61865c326172a4a2343b645cf5d7f20992" - encodedToken2 := "bf355921bbf3ef3672a03e410d4194077dfa5fe863c652521763b3e7f81e7b11" - invalid := []input{ - {reqToken: encodedToken2, cookieToken: encodedToken1}, - {reqToken: "", cookieToken: encodedToken1}, - {reqToken: "", cookieToken: ""}, - {reqToken: encodedToken1, cookieToken: ""}, - } - clt := s.client(t) ctx := context.Background() // valid validReq := loginWebOTPParams{ - webClient: clt, - clock: s.clock, - user: user, - password: pass, - otpSecret: otpSecret, - cookieCSRF: &encodedToken1, - headerCSRF: &encodedToken1, + webClient: clt, + clock: s.clock, + user: user, + password: pass, + otpSecret: otpSecret, } loginWebOTP(t, ctx, validReq) - // invalid - for i := range invalid { - req := validReq - req.cookieCSRF = &invalid[i].cookieToken - req.headerCSRF = &invalid[i].reqToken - httpResp, _, err := rawLoginWebOTP(ctx, req) - require.NoError(t, err, "Login via /webapi/sessions/new failed unexpectedly") - assert.Equal(t, http.StatusForbidden, httpResp.StatusCode, "HTTP status code mismatch") - } + // invalid - wrong content-type header + invalidReq := validReq + invalidReq.overrideContentType = "multipart/form-data" + httpResp, _, err := rawLoginWebOTP(ctx, invalidReq) + require.NoError(t, err, "Login via /webapi/sessions/new failed unexpectedly") + require.Equal(t, http.StatusBadRequest, httpResp.StatusCode, "HTTP status code mismatch") } func TestPasswordChange(t *testing.T) { @@ -5953,13 +5934,9 @@ func TestChangeUserAuthentication_WithPrivacyPolicyEnabledError(t *testing.T) { httpReqData, err := json.Marshal(req) require.NoError(t, err) - // CSRF protected endpoint. - csrfToken := "2ebcb768d0090ea4368e42880c970b61865c326172a4a2343b645cf5d7f20992" httpReq, err := http.NewRequest("PUT", clt.Endpoint("webapi", "users", "password", "token"), bytes.NewBuffer(httpReqData)) require.NoError(t, err) - addCSRFCookieToReq(httpReq, csrfToken) httpReq.Header.Set("Content-Type", "application/json") - httpReq.Header.Set(csrf.HeaderName, csrfToken) httpRes, err := httplib.ConvertResponse(clt.RoundTrip(func() (*http.Response, error) { return clt.HTTPClient().Do(httpReq) })) @@ -6104,10 +6081,6 @@ func TestChangeUserAuthentication_settingDefaultClusterAuthPreference(t *testing req, err := http.NewRequest("PUT", clt.Endpoint("webapi", "users", "password", "token"), bytes.NewBuffer(body)) require.NoError(t, err) - csrfToken, err := csrf.GenerateToken() - require.NoError(t, err) - addCSRFCookieToReq(req, csrfToken) - req.Header.Set(csrf.HeaderName, csrfToken) req.Header.Set("Content-Type", "application/json") re, err := clt.Client.RoundTrip(func() (*http.Response, error) { @@ -6129,8 +6102,6 @@ func TestChangeUserAuthentication_settingDefaultClusterAuthPreference(t *testing func TestParseSSORequestParams(t *testing.T) { t.Parallel() - token := "someMeaninglessTokenString" - tests := []struct { name, url string wantErr bool @@ -6142,7 +6113,6 @@ func TestParseSSORequestParams(t *testing.T) { expected: &SSORequestParams{ ClientRedirectURL: "https://localhost:8080/web/cluster/im-a-cluster-name/nodes?search=tunnel&sort=hostname:asc", ConnectorID: "oidc", - CSRFToken: token, }, }, { @@ -6151,7 +6121,6 @@ func TestParseSSORequestParams(t *testing.T) { expected: &SSORequestParams{ ClientRedirectURL: "https://localhost:8080/web/cluster/im-a-cluster-name/nodes?search=tunnel&sort=hostname:asc", ConnectorID: "github", - CSRFToken: token, }, }, { @@ -6160,7 +6129,6 @@ func TestParseSSORequestParams(t *testing.T) { expected: &SSORequestParams{ ClientRedirectURL: "https://localhost:8080/web/cluster/im-a-cluster-name/apps?query=search(%22watermelon%22%2C%20%22this%22)%20%26%26%20labels%5B%22unique-id%22%5D%20%3D%3D%20%22hi%22&sort=name:asc", ConnectorID: "saml", - CSRFToken: token, }, }, { @@ -6179,7 +6147,6 @@ func TestParseSSORequestParams(t *testing.T) { t.Run(tc.name, func(t *testing.T) { req, err := http.NewRequest("", tc.url, nil) require.NoError(t, err) - addCSRFCookieToReq(req, token) params, err := ParseSSORequestParams(req) @@ -7932,15 +7899,6 @@ func (s *WebSuite) url() *url.URL { return u } -func addCSRFCookieToReq(req *http.Request, token string) { - cookie := &http.Cookie{ - Name: csrf.CookieName, - Value: token, - } - - req.AddCookie(cookie) -} - func removeSpace(in string) string { for _, c := range []string{"\n", "\r", "\t"} { in = strings.Replace(in, c, " ", -1) diff --git a/lib/web/login_helper_test.go b/lib/web/login_helper_test.go index ae7f2ddbd93..2829a2d1400 100644 --- a/lib/web/login_helper_test.go +++ b/lib/web/login_helper_test.go @@ -18,6 +18,7 @@ package web import ( "bytes" + "cmp" "context" "encoding/base32" "encoding/json" @@ -34,7 +35,6 @@ import ( "github.com/gravitational/teleport/lib/auth/mocku2f" "github.com/gravitational/teleport/lib/client" - "github.com/gravitational/teleport/lib/httplib/csrf" ) // newOTPSharedSecret returns an OTP shared secret, encoded as a base32 string. @@ -54,9 +54,8 @@ type loginWebOTPParams struct { // If empty then no OTP is sent in the request. otpSecret string - userAgent string // Optional. - - cookieCSRF, headerCSRF *string // Explicit CSRF tokens. Optional. + userAgent string // Optional. + overrideContentType string // Optional. } // DrainedHTTPResponse mimics an http.Response, but without a body. @@ -124,24 +123,11 @@ func rawLoginWebOTP(ctx context.Context, params loginWebOTPParams) (resp *Draine } // Set assorted headers. - req.Header.Set("Content-Type", "application/json") + req.Header.Set("Content-Type", cmp.Or(params.overrideContentType, "application/json")) if params.userAgent != "" { req.Header.Set("User-Agent", params.userAgent) } - // Set CSRF cookie and header. - const defaultCSRFToken = "2ebcb768d0090ea4368e42880c970b61865c326172a4a2343b645cf5d7f20992" - cookieCSRF := defaultCSRFToken - if params.cookieCSRF != nil { - cookieCSRF = *params.cookieCSRF - } - addCSRFCookieToReq(req, cookieCSRF) - headerCSRF := defaultCSRFToken - if params.headerCSRF != nil { - headerCSRF = *params.headerCSRF - } - req.Header.Set(csrf.HeaderName, headerCSRF) - httpResp, err := webClient.HTTPClient().Do(req) if err != nil { return nil, nil, trace.Wrap(err, "do HTTP request")