callback_handler.go: write 2 invalid cookie tests

Also common-ize some more constants shared between the auth and callback
endpoints.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

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,

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
+}

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)
 

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{