diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index a22e50289..96cb44645 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -34,16 +34,9 @@ const ( // The `name` passed to the encoder for encoding the upstream state param value. This name is short // because it will be encoded into the upstream state param value and we're trying to keep that small. upstreamStateParamEncodingName = "s" - - // The name of the browser cookie which shall hold our CSRF value. - // `__Host` prefix has a special meaning. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Cookie_prefixes - csrfCookieName = "__Host-pinniped-csrf" - - // The `name` passed to the encoder for encoding and decoding the CSRF cookie contents. - csrfCookieEncodingName = "csrf" ) -// This is the encoding side of the securecookie.Codec interface. +// Encoder is the encoding side of the securecookie.Codec interface. type Encoder interface { Encode(name string, value interface{}) (string, error) } @@ -152,14 +145,14 @@ func NewHandler( } func readCSRFCookie(r *http.Request, codec securecookie.Codec) (csrftoken.CSRFToken, error) { - receivedCSRFCookie, err := r.Cookie(csrfCookieName) + receivedCSRFCookie, err := r.Cookie(oidc.CSRFCookieName) if err != nil { // Error means that the cookie was not found return "", nil } var csrfFromCookie csrftoken.CSRFToken - err = codec.Decode(csrfCookieEncodingName, receivedCSRFCookie.Value, &csrfFromCookie) + err = codec.Decode(oidc.CSRFCookieEncodingName, receivedCSRFCookie.Value, &csrfFromCookie) if err != nil { return "", httperr.Wrap(http.StatusUnprocessableEntity, "error reading CSRF cookie", err) } @@ -242,13 +235,13 @@ func upstreamStateParam( } func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken, codec securecookie.Codec) error { - encodedCSRFValue, err := codec.Encode(csrfCookieEncodingName, csrfValue) + encodedCSRFValue, err := codec.Encode(oidc.CSRFCookieEncodingName, csrfValue) if err != nil { return httperr.Wrap(http.StatusInternalServerError, "error encoding CSRF cookie", err) } http.SetCookie(w, &http.Cookie{ - Name: csrfCookieName, + Name: oidc.CSRFCookieName, Value: encodedCSRFValue, HttpOnly: true, SameSite: http.SameSiteStrictMode, diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 475008e5e..9c5ad6fd1 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -10,17 +10,29 @@ import ( "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/provider" ) +// Decoder is the decoding side of the securecookie.Codec interface. +type Decoder interface { + Decode(name, value string, into interface{}) error +} + func NewHandler( idpListGetter oidc.IDPListGetter, + cookieDecoder Decoder, ) http.Handler { return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.Method != http.MethodGet { return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET)", r.Method) } + _, err := readCSRFCookie(r, cookieDecoder) + if err != nil { + return err + } + if r.FormValue("code") == "" { return httperr.New(http.StatusBadRequest, "code param not found") } @@ -46,3 +58,19 @@ func findUpstreamIDPConfig(r *http.Request, idpListGetter oidc.IDPListGetter) *p } return nil } + +func readCSRFCookie(r *http.Request, cookieDecoder Decoder) (csrftoken.CSRFToken, error) { + receivedCSRFCookie, err := r.Cookie(oidc.CSRFCookieName) + if err != nil { + // Error means that the cookie was not found + return "", httperr.Wrap(http.StatusForbidden, "unauthorized request", err) + } + + var csrfFromCookie csrftoken.CSRFToken + err = cookieDecoder.Decode(oidc.CSRFCookieEncodingName, receivedCSRFCookie.Value, &csrfFromCookie) + if err != nil { + return "", httperr.Wrap(http.StatusForbidden, "unauthorized request", err) + } + + return csrfFromCookie, nil +} diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 05635016f..8d966db41 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -53,27 +53,34 @@ func TestCallbackEndpoint(t *testing.T) { var happyCookieEncoder = securecookie.New(cookieEncoderHashKey, cookieEncoderBlockKey) happyCookieEncoder.SetSerializer(securecookie.JSONEncoder{}) - //happyCSRF := "test-csrf" - //happyPKCE := "test-pkce" - //happyNonce := "test-nonce" + // happyCSRF := "test-csrf" + // happyPKCE := "test-pkce" + // happyNonce := "test-nonce" // - //happyEncodedState, err := happyStateEncoder.Encode("s", - // testutil.ExpectedUpstreamStateParamFormat{ - // P: "todo query goes here", - // N: happyNonce, - // C: happyCSRF, - // K: happyPKCE, - // V: "1", - // }, - //) - //require.NoError(t, err) + // happyEncodedState, err := happyStateEncoder.Encode("s", + // testutil.ExpectedUpstreamStateParamFormat{ + // P: "todo query goes here", + // N: happyNonce, + // C: happyCSRF, + // K: happyPKCE, + // V: "1", + // }, + // ) + // require.NoError(t, err) + + incomingCookieCSRFValue := "csrf-value-from-cookie" + encodedIncomingCookieCSRFValue, err := happyCookieEncoder.Encode("csrf", incomingCookieCSRFValue) + require.NoError(t, err) + happyCSRFCookie := "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue tests := []struct { name string + idpListGetter provider.DynamicUpstreamIDPProvider + cookieDecoder Decoder method string path string - idpListGetter provider.DynamicUpstreamIDPProvider + csrfCookie string wantStatus int wantBody string @@ -111,37 +118,62 @@ func TestCallbackEndpoint(t *testing.T) { wantBody: "Method Not Allowed: DELETE (try GET)\n", }, { - name: "code param was not included on request", - method: http.MethodGet, - path: newRequestPath().WithoutCode().String(), - wantStatus: http.StatusBadRequest, - wantBody: "Bad Request: code param not found\n", + name: "code param was not included on request", + cookieDecoder: happyCookieEncoder, + method: http.MethodGet, + path: newRequestPath().WithoutCode().String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantBody: "Bad Request: code param not found\n", }, { - name: "state param was not included on request", - method: http.MethodGet, - path: newRequestPath().WithoutState().String(), - wantStatus: http.StatusBadRequest, - wantBody: "Bad Request: state param not found\n", + name: "state param was not included on request", + cookieDecoder: happyCookieEncoder, + method: http.MethodGet, + path: newRequestPath().WithoutState().String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantBody: "Bad Request: state param not found\n", }, { name: "state param was not signed correctly, has expired, or otherwise cannot be decoded for any reason", + idpListGetter: testutil.NewIDPListGetter(upstreamOIDCIdentityProvider), + cookieDecoder: happyCookieEncoder, method: http.MethodGet, path: newRequestPath().WithState("this-will-not-decode").String(), - idpListGetter: testutil.NewIDPListGetter(upstreamOIDCIdentityProvider), + csrfCookie: happyCSRFCookie, wantStatus: http.StatusBadRequest, wantBody: "Bad Request: state param not valid\n", }, { name: "the UpstreamOIDCProvider CRD has been deleted", + idpListGetter: testutil.NewIDPListGetter(otherUpstreamOIDCIdentityProvider), + cookieDecoder: happyCookieEncoder, method: http.MethodGet, path: newRequestPath().String(), - idpListGetter: testutil.NewIDPListGetter(otherUpstreamOIDCIdentityProvider), + csrfCookie: happyCSRFCookie, wantStatus: http.StatusUnprocessableEntity, wantBody: "Unprocessable Entity: upstream provider not found\n", }, - // TODO: csrf cookie does not exist on request - // TODO: csrf cookie value cannot be decoded (e.g. invalid signture or any other decoding problem) + { + name: "the CSRF cookie does not exist on request", + idpListGetter: testutil.NewIDPListGetter(otherUpstreamOIDCIdentityProvider), + cookieDecoder: happyCookieEncoder, + method: http.MethodGet, + path: newRequestPath().String(), + wantStatus: http.StatusForbidden, + wantBody: "Forbidden: unauthorized request\n", + }, + { + name: "the CSRF cookie cannot be decoded", + idpListGetter: testutil.NewIDPListGetter(otherUpstreamOIDCIdentityProvider), + cookieDecoder: happyCookieEncoder, + method: http.MethodGet, + path: newRequestPath().String(), + csrfCookie: "__Host-pinniped-csrf=this-value-was-not-signed-by-pinniped", + wantStatus: http.StatusForbidden, + wantBody: "Forbidden: unauthorized request\n", + }, // TODO: csrf value from inside state param does not match csrf cookie value // TODO: state's internal version does not match what we want @@ -169,8 +201,11 @@ func TestCallbackEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.idpListGetter) + subject := NewHandler(test.idpListGetter, test.cookieDecoder) req := httptest.NewRequest(test.method, test.path, nil) + if test.csrfCookie != "" { + req.Header.Set("Cookie", test.csrfCookie) + } rsp := httptest.NewRecorder() subject.ServeHTTP(rsp, req) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index f0f74970f..3b5dcb60c 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -18,6 +18,17 @@ const ( JWKSEndpointPath = "/jwks.json" ) +const ( + // CSRFCookieName is the name of the browser cookie which shall hold our CSRF value. + // The `__Host` prefix has a special meaning. See + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Cookie_prefixes. + CSRFCookieName = "__Host-pinniped-csrf" + + // CSRFCookieEncodingName is the `name` passed to the encoder for encoding and decoding the CSRF + // cookie contents. + CSRFCookieEncodingName = "csrf" +) + func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { return &fosite.DefaultOpenIDConnectClient{ DefaultClient: &fosite.DefaultClient{