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 <akeesler@vmware.com>
This commit is contained in:
Ryan Richard 2020-11-20 13:56:35 -08:00 committed by Andrew Keesler
parent b21f0035d7
commit c4ff1ca304
2 changed files with 42 additions and 29 deletions

View File

@ -41,11 +41,7 @@ func NewHandler(
return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method)
} }
csrfFromCookie, err := readCSRFCookie(r, cookieCodec) csrfFromCookie := readCSRFCookie(r, cookieCodec)
if err != nil {
plog.InfoErr("error reading CSRF cookie", err)
return err
}
authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r) authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r)
if err != nil { 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) receivedCSRFCookie, err := r.Cookie(oidc.CSRFCookieName)
if err != nil { if err != nil {
// Error means that the cookie was not found // Error means that the cookie was not found
return "", nil return ""
} }
var csrfFromCookie csrftoken.CSRFToken var csrfFromCookie csrftoken.CSRFToken
err = codec.Decode(oidc.CSRFCookieEncodingName, receivedCSRFCookie.Value, &csrfFromCookie) err = codec.Decode(oidc.CSRFCookieEncodingName, receivedCSRFCookie.Value, &csrfFromCookie)
if err != nil { 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) { func grantOpenIDScopeIfRequested(authorizeRequester fosite.AuthorizeRequester) {

View File

@ -328,6 +328,26 @@ func TestAuthorizationEndpoint(t *testing.T) {
wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")), wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")),
wantUpstreamStateParamInLocationHeader: true, 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", name: "happy path when downstream redirect uri matches what is configured for client except for the port number",
issuer: downstreamIssuer, issuer: downstreamIssuer,
@ -639,22 +659,6 @@ func TestAuthorizationEndpoint(t *testing.T) {
wantContentType: "text/plain; charset=utf-8", wantContentType: "text/plain; charset=utf-8",
wantBodyString: "Internal Server Error: error generating PKCE param\n", 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", name: "no upstream providers are configured",
issuer: downstreamIssuer, issuer: downstreamIssuer,
@ -864,10 +868,20 @@ func requireEqualURLs(t *testing.T, actualURL string, expectedURL string, ignore
require.NoError(t, err) require.NoError(t, err)
expectedLocationURL, err := url.Parse(expectedURL) expectedLocationURL, err := url.Parse(expectedURL)
require.NoError(t, err) require.NoError(t, err)
require.Equal(t, expectedLocationURL.Scheme, actualLocationURL.Scheme) require.Equal(t, expectedLocationURL.Scheme, actualLocationURL.Scheme,
require.Equal(t, expectedLocationURL.User, actualLocationURL.User) "schemes were not equal: expected %s but got %s", expectedURL, actualURL,
require.Equal(t, expectedLocationURL.Host, actualLocationURL.Host) )
require.Equal(t, expectedLocationURL.Path, actualLocationURL.Path) 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() expectedLocationQuery := expectedLocationURL.Query()
actualLocationQuery := actualLocationURL.Query() actualLocationQuery := actualLocationURL.Query()