From c4ff1ca30448553a4c8c716fb2595048629ca4bc Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 20 Nov 2020 13:56:35 -0800 Subject: [PATCH] auth_handler.go: Ignore invalid CSRF cookies rather than return error Generate a new cookie for the user and move on as if they had not sent a bad cookie. Hopefully this will make the user experience better if, for example, the server rotated cookie signing keys and then a user submitted a very old cookie. Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler.go | 17 ++++---- internal/oidc/auth/auth_handler_test.go | 54 ++++++++++++++++--------- 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 8a566620..5634fc01 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -41,11 +41,7 @@ func NewHandler( return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) } - csrfFromCookie, err := readCSRFCookie(r, cookieCodec) - if err != nil { - plog.InfoErr("error reading CSRF cookie", err) - return err - } + csrfFromCookie := readCSRFCookie(r, cookieCodec) authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r) if err != nil { @@ -133,20 +129,23 @@ func NewHandler( }) } -func readCSRFCookie(r *http.Request, codec oidc.Codec) (csrftoken.CSRFToken, error) { +func readCSRFCookie(r *http.Request, codec oidc.Codec) csrftoken.CSRFToken { receivedCSRFCookie, err := r.Cookie(oidc.CSRFCookieName) if err != nil { // Error means that the cookie was not found - return "", nil + return "" } var csrfFromCookie csrftoken.CSRFToken err = codec.Decode(oidc.CSRFCookieEncodingName, receivedCSRFCookie.Value, &csrfFromCookie) if err != nil { - return "", httperr.Wrap(http.StatusUnprocessableEntity, "error reading CSRF cookie", err) + // We can ignore any errors and just make a new cookie. Hopefully this will + // make the user experience better if, for example, the server rotated + // cookie signing keys and then a user submitted a very old cookie. + return "" } - return csrfFromCookie, nil + return csrfFromCookie } func grantOpenIDScopeIfRequested(authorizeRequester fosite.AuthorizeRequester) { diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 381ff052..519f5b7c 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -328,6 +328,26 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")), wantUpstreamStateParamInLocationHeader: true, }, + { + name: "error while decoding CSRF cookie just generates a new cookie and succeeds as usual", + issuer: downstreamIssuer, + idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: happyGetRequestPath, + csrfCookie: "__Host-pinniped-csrf=this-value-was-not-signed-by-pinniped", + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + // Generated a new CSRF cookie and set it in the response. + wantCSRFValueInCookieHeader: happyCSRF, + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")), + wantUpstreamStateParamInLocationHeader: true, + wantBodyStringWithLocationInHref: true, + }, { name: "happy path when downstream redirect uri matches what is configured for client except for the port number", issuer: downstreamIssuer, @@ -639,22 +659,6 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "text/plain; charset=utf-8", wantBodyString: "Internal Server Error: error generating PKCE param\n", }, - { - name: "error while decoding CSRF cookie", - issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), - generateCSRF: happyCSRFGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - stateEncoder: happyStateEncoder, - cookieEncoder: happyCookieEncoder, - method: http.MethodGet, - path: happyGetRequestPath, - csrfCookie: "__Host-pinniped-csrf=this-value-was-not-signed-by-pinniped", - wantStatus: http.StatusUnprocessableEntity, - wantContentType: "text/plain; charset=utf-8", - wantBodyString: "Unprocessable Entity: error reading CSRF cookie\n", - }, { name: "no upstream providers are configured", issuer: downstreamIssuer, @@ -864,10 +868,20 @@ func requireEqualURLs(t *testing.T, actualURL string, expectedURL string, ignore require.NoError(t, err) expectedLocationURL, err := url.Parse(expectedURL) require.NoError(t, err) - require.Equal(t, expectedLocationURL.Scheme, actualLocationURL.Scheme) - require.Equal(t, expectedLocationURL.User, actualLocationURL.User) - require.Equal(t, expectedLocationURL.Host, actualLocationURL.Host) - require.Equal(t, expectedLocationURL.Path, actualLocationURL.Path) + require.Equal(t, expectedLocationURL.Scheme, actualLocationURL.Scheme, + "schemes were not equal: expected %s but got %s", expectedURL, actualURL, + ) + require.Equal(t, expectedLocationURL.User, actualLocationURL.User, + "users were not equal: expected %s but got %s", expectedURL, actualURL, + ) + + require.Equal(t, expectedLocationURL.Host, actualLocationURL.Host, + "hosts were not equal: expected %s but got %s", expectedURL, actualURL, + ) + + require.Equal(t, expectedLocationURL.Path, actualLocationURL.Path, + "paths were not equal: expected %s but got %s", expectedURL, actualURL, + ) expectedLocationQuery := expectedLocationURL.Query() actualLocationQuery := actualLocationURL.Query()