From 970be588472421acd94b08058e92cdbb9b423360 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Tue, 1 Dec 2020 16:25:12 -0500 Subject: [PATCH 01/20] token_handler.go: first draft of token handler, with a bunch of TODOs Signed-off-by: Andrew Keesler --- internal/oidc/oidc.go | 12 +- internal/oidc/token/token_handler.go | 57 ++ internal/oidc/token/token_handler_test.go | 781 ++++++++++++++++++++++ 3 files changed, 848 insertions(+), 2 deletions(-) create mode 100644 internal/oidc/token/token_handler.go create mode 100644 internal/oidc/token/token_handler_test.go diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 0e193046..508ae334 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -5,6 +5,7 @@ package oidc import ( + "crypto/ecdsa" "time" "github.com/ory/fosite" @@ -28,10 +29,16 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { GrantTypes: []string{"authorization_code"}, Scopes: []string{"openid", "profile", "email"}, }, + TokenEndpointAuthMethod: "none", } } -func FositeOauth2Helper(issuerURL string, oauthStore fosite.Storage, hmacSecretOfLengthAtLeast32 []byte) fosite.OAuth2Provider { +func FositeOauth2Helper( + issuerURL string, + oauthStore fosite.Storage, + hmacSecretOfLengthAtLeast32 []byte, + jwtSigningKey *ecdsa.PrivateKey, +) fosite.OAuth2Provider { oauthConfig := &compose.Config{ AuthorizeCodeLifespan: 3 * time.Minute, // seems more than long enough to exchange a code @@ -57,7 +64,8 @@ func FositeOauth2Helper(issuerURL string, oauthStore fosite.Storage, hmacSecretO oauthStore, &compose.CommonStrategy{ // Note that Fosite requires the HMAC secret to be at least 32 bytes. - CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), + CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), + OpenIDConnectTokenStrategy: compose.NewOpenIDConnectECDSAStrategy(oauthConfig, jwtSigningKey), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. compose.OAuth2AuthorizeExplicitFactory, diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go new file mode 100644 index 00000000..5760c377 --- /dev/null +++ b/internal/oidc/token/token_handler.go @@ -0,0 +1,57 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package token provides a handler for the OIDC token endpoint. +package token + +import ( + "net/http" + + "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" + + "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/plog" +) + +func NewHandler( + oauthHelper fosite.OAuth2Provider, +) http.Handler { + return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { + // check csrf cookie? + + var session openid.DefaultSession + accessRequest, err := oauthHelper.NewAccessRequest(r.Context(), r, &session) + if err != nil { + plog.Info("token request error", fositeErrorForLog(err)...) + oauthHelper.WriteAccessError(w, accessRequest, err) + return nil + } + + // TODO: do we need to grant the openid scope here? Or should it be already granted? + + accessResponse, err := oauthHelper.NewAccessResponse(r.Context(), accessRequest) + if err != nil { + plog.Info("token response error", fositeErrorForLog(err)...) + oauthHelper.WriteAccessError(w, accessRequest, err) + return nil + } + + oauthHelper.WriteAccessResponse(w, accessRequest, accessResponse) + + return nil + }) +} + +// TODO: de-dup me. +func fositeErrorForLog(err error) []interface{} { + rfc6749Error := fosite.ErrorToRFC6749Error(err) + keysAndValues := make([]interface{}, 0) + keysAndValues = append(keysAndValues, "name") + keysAndValues = append(keysAndValues, rfc6749Error.Name) + keysAndValues = append(keysAndValues, "status") + keysAndValues = append(keysAndValues, rfc6749Error.Status()) + keysAndValues = append(keysAndValues, "description") + keysAndValues = append(keysAndValues, rfc6749Error.Description) + return keysAndValues +} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go new file mode 100644 index 00000000..b149106e --- /dev/null +++ b/internal/oidc/token/token_handler_test.go @@ -0,0 +1,781 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package token + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "io" + "io/ioutil" + "mime" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" + "github.com/ory/fosite/storage" + "github.com/ory/fosite/token/jwt" + "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/here" + "go.pinniped.dev/internal/oidc" +) + +const ( + goodIssuer = "https://some-issuer.com" + goodClient = "pinniped-cli" + goodRedirectURI = "http://127.0.0.1/callback" + goodPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" + goodNonce = "some-nonce-that-is-at-least-32-characters-to-meet-entropy-requirements" + goodSubject = "some-subject" + goodUsername = "some-username" + + hmacSecret = "this needs to be at least 32 characters to meet entropy requirements" + + authCodeExpirationSeconds = 3 * 60 // Current, we set our auth code expiration to 3 minutes + accessTokenExpirationSeconds = 5 * 60 // Currently, we set our access token expiration to 5 minutes + idTokenExpirationSeconds = 5 * 60 // Currently, we set our ID token expiration to 5 minutes + + timeComparisonFudgeSeconds = 15 +) + +var ( + fositeInvalidMethodErrorBody = func(actual string) string { + return here.Docf(` + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nHTTP method is \"%s\", expected \"POST\".", + "error_hint": "HTTP method is \"%s\", expected \"POST\".", + "status_code": 400 + } + `, actual, actual) + } + + fositeMissingGrantTypeErrorBody = here.Docf(` + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nRequest parameter \"grant_type\"\" is missing", + "error_hint": "Request parameter \"grant_type\"\" is missing", + "status_code": 400 + } + `) + + fositeEmptyPayloadErrorBody = here.Doc(` + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThe POST body can not be empty.", + "error_hint": "The POST body can not be empty.", + "status_code": 400 + } + `) + + fositeInvalidPayloadErrorBody = here.Doc(` + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nUnable to parse HTTP body, make sure to send a properly formatted form request body.", + "error_hint": "Unable to parse HTTP body, make sure to send a properly formatted form request body.", + "status_code": 400 + } + `) + + fositeInvalidRequestErrorBody = here.Doc(` + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nMake sure that the various parameters are correct, be aware of case sensitivity and trim your parameters. Make sure that the client you are using has exactly whitelisted the redirect_uri you specified.", + "error_hint": "Make sure that the various parameters are correct, be aware of case sensitivity and trim your parameters. Make sure that the client you are using has exactly whitelisted the redirect_uri you specified.", + "status_code": 400 + } + `) + + fositeMissingClientErrorBody = here.Doc(` + { + "error": "invalid_request", + "error_verbose": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nClient credentials missing or malformed in both HTTP Authorization header and HTTP POST body.", + "error_hint": "Client credentials missing or malformed in both HTTP Authorization header and HTTP POST body.", + "status_code": 400 + } + `) + + fositeInvalidClientErrorBody = here.Doc(` + { + "error": "invalid_client", + "error_verbose": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)", + "error_description": "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method)", + "status_code": 401 + } + `) + + fositeInvalidAuthCodeErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_verbose": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client", + "status_code": 400 + } + `) + + fositeReusedAuthCodeErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_verbose": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client\n\nThe authorization code has already been used.", + "error_hint": "The authorization code has already been used.", + "status_code": 400 + } + `) + + fositeInvalidRedirectURIErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_verbose": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client\n\nThe \"redirect_uri\" from this request does not match the one from the authorize request.", + "error_hint": "The \"redirect_uri\" from this request does not match the one from the authorize request.", + "status_code": 400 + } + `) + + fositeMissingPKCEVerifierErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_verbose": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client\n\nThe PKCE code verifier must be at least 43 characters.", + "error_hint": "The PKCE code verifier must be at least 43 characters.", + "status_code": 400 + } + `) + + fositeWrongPKCEVerifierErrorBody = here.Doc(` + { + "error": "invalid_grant", + "error_verbose": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client", + "error_description": "The provided authorization grant (e.g., authorization code, resource owner credentials) or refresh token is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client\n\nThe PKCE code challenge did not match the code verifier.", + "error_hint": "The PKCE code challenge did not match the code verifier.", + "status_code": 400 + } + `) +) + +func TestTokenEndpoint(t *testing.T) { + happyAuthRequest := http.Request{ + Form: url.Values{ + "response_type": {"code"}, + "scope": {"openid profile email"}, + "client_id": {goodClient}, + "state": {"some-state-value-that-is-32-byte"}, + "nonce": {goodNonce}, + "code_challenge": {doSHA256(goodPKCECodeVerifier)}, + "code_challenge_method": {"S256"}, + "redirect_uri": {goodRedirectURI}, + }, + } + + tests := []struct { + name string + + authRequest func(authRequest *http.Request) + storage func(t *testing.T, s *storage.MemoryStore, authCode string) + request func(r *http.Request, authCode string) + + wantStatus int + wantBodyFields []string + wantExactBody string + }{ + // happy path + { + name: "request is valid and tokens are issued", + wantStatus: http.StatusOK, + wantBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, + }, + { + name: "openid scope was not requested from authorize endpoint", + authRequest: func(authRequest *http.Request) { + authRequest.Form.Set("scope", "profile email") + }, + wantStatus: http.StatusOK, + wantBodyFields: []string{"access_token", "token_type", "scope", "expires_in"}, + }, + + // sad path + { + name: "GET method is wrong", + request: func(r *http.Request, authCode string) { r.Method = http.MethodGet }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("GET"), + }, + { + name: "PUT method is wrong", + request: func(r *http.Request, authCode string) { r.Method = http.MethodPut }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("PUT"), + }, + { + name: "PATCH method is wrong", + request: func(r *http.Request, authCode string) { r.Method = http.MethodPatch }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("PATCH"), + }, + { + name: "DELETE method is wrong", + request: func(r *http.Request, authCode string) { r.Method = http.MethodDelete }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidMethodErrorBody("DELETE"), + }, + // TODO: add test for when csrf cookie is invalid...maybe? + { + name: "content type is invalid", + request: func(r *http.Request, authCode string) { r.Header.Set("Content-Type", "text/plain") }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeEmptyPayloadErrorBody, + }, + { + name: "payload is not valid form serialization", + request: func(r *http.Request, authCode string) { + r.Body = ioutil.NopCloser(strings.NewReader("this newline character is not allowed in a form serialization: \n")) + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeMissingGrantTypeErrorBody, + }, + { + name: "payload is empty", + request: func(r *http.Request, authCode string) { r.Body = nil }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidPayloadErrorBody, + }, + { + name: "grant type is missing in request", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithGrantType("").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeMissingGrantTypeErrorBody, + }, + { + name: "grant type is not authorization_code", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithGrantType("bogus").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidRequestErrorBody, + }, + { + name: "client id is missing in request", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithClientID("").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeMissingClientErrorBody, + }, + { + name: "client id is wrong", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithClientID("bogus").ReadCloser() + }, + wantStatus: http.StatusUnauthorized, + wantExactBody: fositeInvalidClientErrorBody, + }, + { + name: "auth code is missing in request", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithAuthCode("").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidAuthCodeErrorBody, + }, + { + name: "auth code has never been valid", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithAuthCode("bogus").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidAuthCodeErrorBody, + }, + { + name: "auth code is invalidated", + storage: func(t *testing.T, s *storage.MemoryStore, authCode string) { + err := s.InvalidateAuthorizeCodeSession(context.Background(), getFositeDataSignature(t, authCode)) + require.NoError(t, err) + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeReusedAuthCodeErrorBody, + }, + { + name: "redirect uri is missing in request", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithRedirectURI("").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidRedirectURIErrorBody, + }, + { + name: "redirect uri is wrong", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithRedirectURI("bogus").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeInvalidRedirectURIErrorBody, + }, + { + name: "pkce is missing in request", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithPKCE("").ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeMissingPKCEVerifierErrorBody, + }, + { + name: "pkce is wrong", + request: func(r *http.Request, authCode string) { + r.Body = happyBody(authCode).WithPKCE( + "bogus-verifier-that-is-at-least-43-characters-for-the-sake-of-entropy", + ).ReadCloser() + }, + wantStatus: http.StatusBadRequest, + wantExactBody: fositeWrongPKCEVerifierErrorBody, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + authRequest := happyAuthRequest + if test.authRequest != nil { + test.authRequest(&authRequest) + } + + oauthStore := storage.NewMemoryStore() + oauthHelper, authCode := makeHappyOauthHelper(t, &authRequest, oauthStore) + if test.storage != nil { + test.storage(t, oauthStore, authCode) + } + subject := NewHandler(oauthHelper) + + req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if test.request != nil { + test.request(req, authCode) + } + // if test.csrfCookie != "" { + // req.Header.Set("Cookie", test.csrfCookie) + // } + rsp := httptest.NewRecorder() + + subject.ServeHTTP(rsp, req) + t.Logf("response: %#v", rsp) + t.Logf("response body: %q", rsp.Body.String()) + + require.Equal(t, test.wantStatus, rsp.Code) + requireEqualContentType(t, rsp.Header().Get("Content-Type"), "application/json") + if test.wantBodyFields != nil { + var m map[string]interface{} + require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &m)) + require.ElementsMatch(t, test.wantBodyFields, getMapKeys(m)) + + code := req.PostForm.Get("code") + wantOpenidScope := contains(test.wantBodyFields, "id_token") + requireValidAuthCodeStorage(t, code, oauthStore) + requireValidAccessTokenStorage(t, m, oauthStore, wantOpenidScope) + requireValidPKCEStorage(t, code, oauthStore) + requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + } else { + require.JSONEq(t, test.wantExactBody, rsp.Body.String()) + } + }) + } + + t.Run("auth code is used twice", func(t *testing.T) { + authRequest := happyAuthRequest + oauthStore := storage.NewMemoryStore() + oauthHelper, authCode := makeHappyOauthHelper(t, &authRequest, oauthStore) + subject := NewHandler(oauthHelper) + + req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + // if test.csrfCookie != "" { + // req.Header.Set("Cookie", test.csrfCookie) + // } + + // First call - should be successful. + rsp0 := httptest.NewRecorder() + subject.ServeHTTP(rsp0, req) + t.Logf("response 0: %#v", rsp0) + t.Logf("response 0 body: %q", rsp0.Body.String()) + require.Equal(t, http.StatusOK, rsp0.Code) + + // Second call - should be unsuccessful since auth code was already used. + rsp1 := httptest.NewRecorder() + subject.ServeHTTP(rsp1, req) + t.Logf("response 1: %#v", rsp1) + t.Logf("response 1 body: %q", rsp1.Body.String()) + require.Equal(t, http.StatusBadRequest, rsp1.Code) + requireEqualContentType(t, rsp1.Header().Get("Content-Type"), "application/json") + require.JSONEq(t, fositeReusedAuthCodeErrorBody, rsp1.Body.String()) + }) +} + +type body url.Values + +func happyBody(happyAuthCode string) body { + return map[string][]string{ + "grant_type": {"authorization_code"}, + "code": {happyAuthCode}, + "redirect_uri": {goodRedirectURI}, + "code_verifier": {goodPKCECodeVerifier}, + "client_id": {goodClient}, + } +} + +func (b body) WithGrantType(grantType string) body { + return b.with("grant_type", grantType) +} + +func (b body) WithClientID(clientID string) body { + return b.with("client_id", clientID) +} + +func (b body) WithAuthCode(code string) body { + return b.with("code", code) +} + +func (b body) WithRedirectURI(redirectURI string) body { + return b.with("redirect_uri", redirectURI) +} + +func (b body) WithPKCE(verifier string) body { + return b.with("code_verifier", verifier) +} + +func (b body) ReadCloser() io.ReadCloser { + return ioutil.NopCloser(strings.NewReader(url.Values(b).Encode())) +} + +func (b body) with(param, value string) body { + if value == "" { + url.Values(b).Del(param) + } else { + url.Values(b).Set(param, value) + } + return b +} + +// getFositeDataSignature returns the signature of the provided data. The provided data could be an auth code, access +// token, etc. It is assumed that the code is of the format "data.signature", which is how Fosite generates auth codes +// and access tokens. +func getFositeDataSignature(t *testing.T, data string) string { + split := strings.Split(data, ".") + require.Len(t, split, 2) + return split[1] +} + +func makeHappyOauthHelper( + t *testing.T, + authRequest *http.Request, + store *storage.MemoryStore, +) (fosite.OAuth2Provider, string) { + t.Helper() + + jwtSigningKey := generateJWTSigningKey(t) + oauthHelper := oidc.FositeOauth2Helper(goodIssuer, store, []byte(hmacSecret), jwtSigningKey) + + // Add the Pinniped CLI client. + store.Clients[goodClient] = oidc.PinnipedCLIOIDCClient() + + // Simulate the auth endpoint running so Fosite code will fill the store with realistic values. + ctx := context.Background() + session := &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Subject: goodSubject, + }, + Subject: goodSubject, + Username: goodUsername, + } + authRequester, err := oauthHelper.NewAuthorizeRequest(ctx, authRequest) + require.NoError(t, err) + if strings.Contains(authRequest.Form.Get("scope"), "openid") { + authRequester.GrantScope("openid") + } + authResponder, err := oauthHelper.NewAuthorizeResponse(ctx, authRequester, session) + require.NoError(t, err) + + return oauthHelper, authResponder.GetCode() +} + +func generateJWTSigningKey(t *testing.T) *ecdsa.PrivateKey { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + return key +} + +func hashAccessToken(accessToken string) string { + // See https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken. + // "Access Token hash value. Its value is the base64url encoding of the left-most half of + // the hash of the octets of the ASCII representation of the access_token value, where the + // hash algorithm used is the hash algorithm used in the alg Header Parameter of the ID + // Token's JOSE Header." + b := sha256.Sum256([]byte(accessToken)) + return base64.RawURLEncoding.EncodeToString(b[:len(b)/2]) +} + +func doSHA256(s string) string { + b := sha256.Sum256([]byte(s)) + return base64.RawURLEncoding.EncodeToString(b[:]) +} + +func requireValidAuthCodeStorage( + t *testing.T, + code string, + storage *storage.MemoryStore, +) { + t.Helper() + + // Make sure we have invalidated this auth code. + _, err := storage.GetAuthorizeCodeSession(context.Background(), getFositeDataSignature(t, code), nil) + require.Equal(t, fosite.ErrInvalidatedAuthorizeCode, err) +} + +func requireValidAccessTokenStorage( + t *testing.T, + body map[string]interface{}, + storage *storage.MemoryStore, + wantGrantedOpenidScope bool, +) { + t.Helper() + + // Get the access token, and make sure we can use it to perform a lookup on the storage. + accessToken, ok := body["access_token"] + require.True(t, ok) + accessTokenString, ok := accessToken.(string) + require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) + authRequest, err := storage.GetAccessTokenSession(context.Background(), getFositeDataSignature(t, accessTokenString), nil) + require.NoError(t, err) + + // Make sure the other body fields are valid. + tokenType, ok := body["token_type"] + require.True(t, ok) + tokenTypeString, ok := tokenType.(string) + require.Truef(t, ok, "wanted token_type to be a string, but got %T", tokenType) + require.Equal(t, "bearer", tokenTypeString) + + expiresIn, ok := body["expires_in"] + require.True(t, ok) + expiresInNumber, ok := expiresIn.(float64) // Go unmarshals JSON numbers to float64, see `go doc encoding/json` + require.Truef(t, ok, "wanted expires_in to be an float64, but got %T", expiresIn) + require.InDelta(t, accessTokenExpirationSeconds, expiresInNumber, timeComparisonFudgeSeconds) + + scopes, ok := body["scope"] + require.True(t, ok) + scopesString, ok := scopes.(string) + require.Truef(t, ok, "wanted scopes to be an string, but got %T", scopes) + wantScopes := "" + if wantGrantedOpenidScope { + wantScopes += "openid" + } + require.Equal(t, wantScopes, scopesString) // TODO: do we need that extra scope that mo put forth? + + // Fosite stores access tokens without any of the original request form pararmeters. + requireValidAuthRequest( + t, + authRequest, + authRequest.Sanitize([]string{}).GetRequestForm(), + hashAccessToken(accessTokenString), + wantGrantedOpenidScope, + ) +} + +func requireValidPKCEStorage( + t *testing.T, + code string, + storage *storage.MemoryStore, +) { + t.Helper() + + // Make sure the PKCE session has been deleted. Note that Fosite stores PKCE codes using the auth code signature + // as a key. + _, err := storage.GetPKCERequestSession(context.Background(), getFositeDataSignature(t, code), nil) + require.Equal(t, fosite.ErrNotFound, err) +} + +func requireValidOIDCStorage( + t *testing.T, + body map[string]interface{}, + code string, + storage *storage.MemoryStore, + wantGrantedOpenidScope bool, +) { + t.Helper() + + if wantGrantedOpenidScope { + // Make sure the OIDC session is still there. Note that Fosite stores OIDC sessions using the full auth code as a key. + authRequest, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) + require.NoError(t, err) + + // Fosite stores OIDC sessions with only the nonce in the original request form. + accessToken, ok := body["access_token"] + require.True(t, ok) + accessTokenString, ok := accessToken.(string) + require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) + requireValidAuthRequest( + t, + authRequest, + authRequest.Sanitize([]string{"nonce"}).GetRequestForm(), + hashAccessToken(accessTokenString), + true, + ) + } else { + _, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) + require.Equal(t, fosite.ErrNotFound, err) + } +} + +func requireValidAuthRequest( + t *testing.T, + authRequest fosite.Requester, + wantRequestForm url.Values, + wantAccessTokenHash string, + wantGrantedOpenidScope bool, +) { + t.Helper() + + // Assert that the getters on the authRequest return what we think they should. + wantRequestedScopes := []string{"profile", "email"} + wantGrantedScopes := []string{} + if wantGrantedOpenidScope { + wantRequestedScopes = append([]string{"openid"}, wantRequestedScopes...) + wantGrantedScopes = append([]string{"openid"}, wantGrantedScopes...) + } + require.NotEmpty(t, authRequest.GetID()) + requireTimeInDelta(t, authRequest.GetRequestedAt(), time.Now().UTC(), timeComparisonFudgeSeconds*time.Second) + require.Equal(t, goodClient, authRequest.GetClient().GetID()) + require.Equal(t, fosite.Arguments(wantRequestedScopes), authRequest.GetRequestedScopes()) + require.Equal(t, fosite.Arguments(wantGrantedScopes), authRequest.GetGrantedScopes()) + require.Empty(t, authRequest.GetRequestedAudience()) + require.Empty(t, authRequest.GetGrantedAudience()) + require.Equal(t, wantRequestForm, authRequest.GetRequestForm()) // Fosite stores access token request without form + + // Cast session to the type we think it should be. + session, ok := authRequest.GetSession().(*openid.DefaultSession) + require.Truef(t, ok, "could not cast %T to %T", authRequest.GetSession(), &openid.DefaultSession{}) + + // Assert that the session claims are what we think they should be, but only if we are doing OIDC. + if wantGrantedOpenidScope { + claims := session.Claims + // require.NotEmpty(t, claims.JTI) // TODO: why is this not passing? + require.Equal(t, goodIssuer, claims.Issuer) + require.Equal(t, goodSubject, claims.Subject) + require.Equal(t, []string{goodClient}, claims.Audience) + require.Equal(t, goodNonce, claims.Nonce) + requireTimeInDelta( + t, + time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), + claims.ExpiresAt, + timeComparisonFudgeSeconds*time.Second, + ) + requireTimeInDelta(t, time.Now().UTC(), claims.IssuedAt, timeComparisonFudgeSeconds*time.Second) + // requireTimeInDelta(t, time.Now().UTC(), claims.RequestedAt, timeComparisonFudgeSeconds*time.Second) // TODO: why is this not passing? + requireTimeInDelta(t, time.Now().UTC(), claims.AuthTime, timeComparisonFudgeSeconds*time.Second) + require.Equal(t, wantAccessTokenHash, claims.AccessTokenHash) + require.Empty(t, claims.AuthenticationContextClassReference) // TODO: huh? + require.Empty(t, claims.AuthenticationMethodsReference) // TODO: huh? + require.Empty(t, claims.CodeHash) // TODO: huh? + require.Empty(t, claims.Extra) // TODO: huh? + } + + // Assert that the session headers are what we think they should be. + headers := session.Headers + require.Empty(t, headers) + + // Assert that the token expirations are what we think they should be. + authCodeExpiresAt, ok := session.ExpiresAt[fosite.AuthorizeCode] + require.True(t, ok, "expected session to hold expiration time for auth code") + requireTimeInDelta( + t, + time.Now().UTC().Add(authCodeExpirationSeconds*time.Second), + authCodeExpiresAt, + timeComparisonFudgeSeconds*time.Second, + ) + accessTokenExpiresAt, ok := session.ExpiresAt[fosite.AccessToken] + require.True(t, ok, "expected session to hold expiration time for access token") + requireTimeInDelta( + t, + time.Now().UTC().Add(accessTokenExpirationSeconds*time.Second), + accessTokenExpiresAt, + timeComparisonFudgeSeconds*time.Second, + ) + // idTokenExpiresAt, ok := session.ExpiresAt[fosite.IDToken] // TODO: why is this failing? + // require.True(t, ok, "expected session to hold expiration time for id token") + // requireTimeInDelta( + // t, + // time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), + // idTokenExpiresAt, + // timeComparisonFudgeSeconds*time.Second, + // ) + + // Assert that the session's username and subject are correct. + require.Equal(t, goodUsername, session.Username) + require.Equal(t, goodSubject, session.Subject) +} + +// TODO: de-dup me. +func requireEqualContentType(t *testing.T, actual string, expected string) { + t.Helper() + + if expected == "" { + require.Empty(t, actual) + return + } + + actualContentType, actualContentTypeParams, err := mime.ParseMediaType(expected) + require.NoError(t, err) + expectedContentType, expectedContentTypeParams, err := mime.ParseMediaType(expected) + require.NoError(t, err) + require.Equal(t, actualContentType, expectedContentType) + require.Equal(t, actualContentTypeParams, expectedContentTypeParams) +} + +// TODO: use actual testutil function. +//nolint:unparam +func requireTimeInDelta(t *testing.T, t1 time.Time, t2 time.Time, delta time.Duration) { + t.Helper() + require.InDeltaf(t, + float64(t1.UnixNano()), + float64(t2.UnixNano()), + float64(delta.Nanoseconds()), + "expected %s and %s to be < %s apart, but they are %s apart", + t1.Format(time.RFC3339Nano), + t2.Format(time.RFC3339Nano), + delta.String(), + t1.Sub(t2).String(), + ) +} + +func getMapKeys(m map[string]interface{}) []string { + keys := make([]string, 0) + for key := range m { + keys = append(keys, key) + } + return keys +} + +func contains(haystack []string, needle string) bool { + for _, hay := range haystack { + if hay == needle { + return true + } + } + return false +} From 8e4c85d8168e774052876bf7aeabd538766826ce Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 2 Dec 2020 11:16:02 -0500 Subject: [PATCH 02/20] WIP: get linting and unit tests passing after token endpoint first draft Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler_test.go | 4 +++- internal/oidc/nullstorage_test.go | 1 + internal/oidc/provider/manager/manager.go | 7 ++++++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 16694676..3a522495 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -4,6 +4,7 @@ package auth import ( + "crypto/ecdsa" "fmt" "html" "mime" @@ -124,8 +125,9 @@ func TestAuthorizationEndpoint(t *testing.T) { // Configure fosite the same way that the production code would, except use in-memory storage. oauthStore := oidc.NullStorage{} hmacSecret := []byte("some secret - must have at least 32 bytes") + var signingKeyIsUnused *ecdsa.PrivateKey require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") - oauthHelper := oidc.FositeOauth2Helper(issuer, oauthStore, hmacSecret) + oauthHelper := oidc.FositeOauth2Helper(issuer, oauthStore, hmacSecret, signingKeyIsUnused) happyCSRF := "test-csrf" happyPKCE := "test-pkce" diff --git a/internal/oidc/nullstorage_test.go b/internal/oidc/nullstorage_test.go index 8555c031..2983a800 100644 --- a/internal/oidc/nullstorage_test.go +++ b/internal/oidc/nullstorage_test.go @@ -30,6 +30,7 @@ func TestNullStorage_GetClient(t *testing.T) { GrantTypes: []string{"authorization_code"}, Scopes: []string{"openid", "profile", "email"}, }, + TokenEndpointAuthMethod: "none", }, client, ) diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 6c2e12db..9414b5bc 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -70,7 +70,12 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { // Use NullStorage for the authorize endpoint because we do not actually want to store anything until // the upstream callback endpoint is called later. - oauthHelper := oidc.FositeOauth2Helper(incomingProvider.Issuer(), oidc.NullStorage{}, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret + oauthHelper := oidc.FositeOauth2Helper( + incomingProvider.Issuer(), + oidc.NullStorage{}, + []byte("some secret - must have at least 32 bytes"), // TODO replace this secret + nil, // TODO: inject me properly + ) // TODO use different codecs for the state and the cookie, because: // 1. we would like to state to have an embedded expiration date while the cookie does not need that From 09e6c86c468e4dc233bbd4f611607bbbb8d4ba7f Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 2 Dec 2020 15:14:01 -0500 Subject: [PATCH 03/20] token_handler.go: complete some TODOs and strengthen double auth code test Signed-off-by: Andrew Keesler --- internal/oidc/token/token_handler.go | 4 - internal/oidc/token/token_handler_test.go | 111 +++++++++++++++------- 2 files changed, 77 insertions(+), 38 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 5760c377..b8747636 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -18,8 +18,6 @@ func NewHandler( oauthHelper fosite.OAuth2Provider, ) http.Handler { return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { - // check csrf cookie? - var session openid.DefaultSession accessRequest, err := oauthHelper.NewAccessRequest(r.Context(), r, &session) if err != nil { @@ -28,8 +26,6 @@ func NewHandler( return nil } - // TODO: do we need to grant the openid scope here? Or should it be already granted? - accessResponse, err := oauthHelper.NewAccessResponse(r.Context(), accessRequest) if err != nil { plog.Info("token response error", fositeErrorForLog(err)...) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index b149106e..9009d7d5 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -50,6 +50,9 @@ const ( ) var ( + goodAuthTime = time.Date(1, 2, 3, 4, 5, 6, 7, time.Local) + goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.Local) + fositeInvalidMethodErrorBody = func(actual string) string { return here.Docf(` { @@ -172,7 +175,7 @@ var ( ) func TestTokenEndpoint(t *testing.T) { - happyAuthRequest := http.Request{ + happyAuthRequest := &http.Request{ Form: url.Values{ "response_type": {"code"}, "scope": {"openid profile email"}, @@ -236,7 +239,6 @@ func TestTokenEndpoint(t *testing.T) { wantStatus: http.StatusBadRequest, wantExactBody: fositeInvalidMethodErrorBody("DELETE"), }, - // TODO: add test for when csrf cookie is invalid...maybe? { name: "content type is invalid", request: func(r *http.Request, authCode string) { r.Header.Set("Content-Type", "text/plain") }, @@ -352,13 +354,13 @@ func TestTokenEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - authRequest := happyAuthRequest + authRequest := deepCopyRequestForm(happyAuthRequest) if test.authRequest != nil { - test.authRequest(&authRequest) + test.authRequest(authRequest) } oauthStore := storage.NewMemoryStore() - oauthHelper, authCode := makeHappyOauthHelper(t, &authRequest, oauthStore) + oauthHelper, authCode := makeHappyOauthHelper(t, authRequest, oauthStore) if test.storage != nil { test.storage(t, oauthStore, authCode) } @@ -369,9 +371,6 @@ func TestTokenEndpoint(t *testing.T) { if test.request != nil { test.request(req, authCode) } - // if test.csrfCookie != "" { - // req.Header.Set("Cookie", test.csrfCookie) - // } rsp := httptest.NewRecorder() subject.ServeHTTP(rsp, req) @@ -387,9 +386,9 @@ func TestTokenEndpoint(t *testing.T) { code := req.PostForm.Get("code") wantOpenidScope := contains(test.wantBodyFields, "id_token") - requireValidAuthCodeStorage(t, code, oauthStore) + requireInvalidAuthCodeStorage(t, code, oauthStore) requireValidAccessTokenStorage(t, m, oauthStore, wantOpenidScope) - requireValidPKCEStorage(t, code, oauthStore) + requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) } else { require.JSONEq(t, test.wantExactBody, rsp.Body.String()) @@ -398,25 +397,39 @@ func TestTokenEndpoint(t *testing.T) { } t.Run("auth code is used twice", func(t *testing.T) { - authRequest := happyAuthRequest + authRequest := deepCopyRequestForm(happyAuthRequest) oauthStore := storage.NewMemoryStore() - oauthHelper, authCode := makeHappyOauthHelper(t, &authRequest, oauthStore) + oauthHelper, authCode := makeHappyOauthHelper(t, authRequest, oauthStore) subject := NewHandler(oauthHelper) req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - // if test.csrfCookie != "" { - // req.Header.Set("Cookie", test.csrfCookie) - // } // First call - should be successful. rsp0 := httptest.NewRecorder() subject.ServeHTTP(rsp0, req) t.Logf("response 0: %#v", rsp0) t.Logf("response 0 body: %q", rsp0.Body.String()) + requireEqualContentType(t, rsp0.Header().Get("Content-Type"), "application/json") require.Equal(t, http.StatusOK, rsp0.Code) + var m map[string]interface{} + require.NoError(t, json.Unmarshal(rsp0.Body.Bytes(), &m)) + + wantBodyFields := []string{"id_token", "access_token", "token_type", "expires_in", "scope"} + require.ElementsMatch(t, wantBodyFields, getMapKeys(m)) + + code := req.PostForm.Get("code") + wantOpenidScope := true + requireInvalidAuthCodeStorage(t, code, oauthStore) + requireValidAccessTokenStorage(t, m, oauthStore, wantOpenidScope) + requireInvalidPKCEStorage(t, code, oauthStore) + requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + // Second call - should be unsuccessful since auth code was already used. + // + // Fosite will also revoke the access token as is recommended by the OIDC spec. Currently, we don't + // delete the OIDC storage...but we probably should. rsp1 := httptest.NewRecorder() subject.ServeHTTP(rsp1, req) t.Logf("response 1: %#v", rsp1) @@ -424,6 +437,11 @@ func TestTokenEndpoint(t *testing.T) { require.Equal(t, http.StatusBadRequest, rsp1.Code) requireEqualContentType(t, rsp1.Header().Get("Content-Type"), "application/json") require.JSONEq(t, fositeReusedAuthCodeErrorBody, rsp1.Body.String()) + + requireInvalidAuthCodeStorage(t, code, oauthStore) + requireInvalidAccessTokenStorage(t, m, oauthStore) + requireInvalidPKCEStorage(t, code, oauthStore) + requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) }) } @@ -495,10 +513,14 @@ func makeHappyOauthHelper( store.Clients[goodClient] = oidc.PinnipedCLIOIDCClient() // Simulate the auth endpoint running so Fosite code will fill the store with realistic values. + // + // We only set the fields in the session that Fosite wants us to set. ctx := context.Background() session := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ - Subject: goodSubject, + Subject: goodSubject, + AuthTime: goodAuthTime, + RequestedAt: goodRequestedAtTime, }, Subject: goodSubject, Username: goodUsername, @@ -536,7 +558,7 @@ func doSHA256(s string) string { return base64.RawURLEncoding.EncodeToString(b[:]) } -func requireValidAuthCodeStorage( +func requireInvalidAuthCodeStorage( t *testing.T, code string, storage *storage.MemoryStore, @@ -585,7 +607,7 @@ func requireValidAccessTokenStorage( if wantGrantedOpenidScope { wantScopes += "openid" } - require.Equal(t, wantScopes, scopesString) // TODO: do we need that extra scope that mo put forth? + require.Equal(t, wantScopes, scopesString) // Fosite stores access tokens without any of the original request form pararmeters. requireValidAuthRequest( @@ -597,7 +619,23 @@ func requireValidAccessTokenStorage( ) } -func requireValidPKCEStorage( +func requireInvalidAccessTokenStorage( + t *testing.T, + body map[string]interface{}, + storage *storage.MemoryStore, +) { + t.Helper() + + // Get the access token, and make sure we can use it to perform a lookup on the storage. + accessToken, ok := body["access_token"] + require.True(t, ok) + accessTokenString, ok := accessToken.(string) + require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) + _, err := storage.GetAccessTokenSession(context.Background(), getFositeDataSignature(t, accessTokenString), nil) + require.Equal(t, fosite.ErrNotFound, err) +} + +func requireInvalidPKCEStorage( t *testing.T, code string, storage *storage.MemoryStore, @@ -674,7 +712,7 @@ func requireValidAuthRequest( // Assert that the session claims are what we think they should be, but only if we are doing OIDC. if wantGrantedOpenidScope { claims := session.Claims - // require.NotEmpty(t, claims.JTI) // TODO: why is this not passing? + require.Empty(t, claims.JTI) // When claims.JTI is empty, Fosite will generate a UUID for this field. require.Equal(t, goodIssuer, claims.Issuer) require.Equal(t, goodSubject, claims.Subject) require.Equal(t, []string{goodClient}, claims.Audience) @@ -686,13 +724,18 @@ func requireValidAuthRequest( timeComparisonFudgeSeconds*time.Second, ) requireTimeInDelta(t, time.Now().UTC(), claims.IssuedAt, timeComparisonFudgeSeconds*time.Second) - // requireTimeInDelta(t, time.Now().UTC(), claims.RequestedAt, timeComparisonFudgeSeconds*time.Second) // TODO: why is this not passing? - requireTimeInDelta(t, time.Now().UTC(), claims.AuthTime, timeComparisonFudgeSeconds*time.Second) require.Equal(t, wantAccessTokenHash, claims.AccessTokenHash) - require.Empty(t, claims.AuthenticationContextClassReference) // TODO: huh? - require.Empty(t, claims.AuthenticationMethodsReference) // TODO: huh? - require.Empty(t, claims.CodeHash) // TODO: huh? - require.Empty(t, claims.Extra) // TODO: huh? + + // We are in charge of setting these fields. For the purpose of testing, we ensure that the + // sentinel test value is set correctly. + require.Equal(t, goodRequestedAtTime, claims.RequestedAt) + require.Equal(t, goodAuthTime, claims.AuthTime) + + // At this time, we don't use any of these optional (per the OIDC spec) fields. + require.Empty(t, claims.AuthenticationContextClassReference) + require.Empty(t, claims.AuthenticationMethodsReference) + require.Empty(t, claims.CodeHash) + require.Empty(t, claims.Extra) } // Assert that the session headers are what we think they should be. @@ -716,14 +759,6 @@ func requireValidAuthRequest( accessTokenExpiresAt, timeComparisonFudgeSeconds*time.Second, ) - // idTokenExpiresAt, ok := session.ExpiresAt[fosite.IDToken] // TODO: why is this failing? - // require.True(t, ok, "expected session to hold expiration time for id token") - // requireTimeInDelta( - // t, - // time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), - // idTokenExpiresAt, - // timeComparisonFudgeSeconds*time.Second, - // ) // Assert that the session's username and subject are correct. require.Equal(t, goodUsername, session.Username) @@ -763,6 +798,14 @@ func requireTimeInDelta(t *testing.T, t1 time.Time, t2 time.Time, delta time.Dur ) } +func deepCopyRequestForm(r *http.Request) *http.Request { + copied := url.Values{} + for k, v := range r.Form { + copied[k] = v + } + return &http.Request{Form: copied} +} + func getMapKeys(m map[string]interface{}) []string { keys := make([]string, 0) for key := range m { From 9419b7392dd7fa20f0ab9f02f111de48ff8a69d7 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 2 Dec 2020 16:26:47 -0500 Subject: [PATCH 04/20] WIP: start to validate ID token returned from token endpoint This won't compile, but we are passing this between two teammates. Signed-off-by: Andrew Keesler --- internal/oidc/token/token_handler_test.go | 31 ++++++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 9009d7d5..e6a594db 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -21,6 +21,7 @@ import ( "testing" "time" + coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/storage" @@ -360,7 +361,7 @@ func TestTokenEndpoint(t *testing.T) { } oauthStore := storage.NewMemoryStore() - oauthHelper, authCode := makeHappyOauthHelper(t, authRequest, oauthStore) + oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) if test.storage != nil { test.storage(t, oauthStore, authCode) } @@ -390,6 +391,10 @@ func TestTokenEndpoint(t *testing.T) { requireValidAccessTokenStorage(t, m, oauthStore, wantOpenidScope) requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + + if wantOpenidScope { + requireValidIDToken(t, m, jwtSigningKey) + } } else { require.JSONEq(t, test.wantExactBody, rsp.Body.String()) } @@ -399,7 +404,7 @@ func TestTokenEndpoint(t *testing.T) { t.Run("auth code is used twice", func(t *testing.T) { authRequest := deepCopyRequestForm(happyAuthRequest) oauthStore := storage.NewMemoryStore() - oauthHelper, authCode := makeHappyOauthHelper(t, authRequest, oauthStore) + oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) subject := NewHandler(oauthHelper) req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) @@ -425,6 +430,7 @@ func TestTokenEndpoint(t *testing.T) { requireValidAccessTokenStorage(t, m, oauthStore, wantOpenidScope) requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + requireValidIDToken(t, m, jwtSigningKey) // Second call - should be unsuccessful since auth code was already used. // @@ -503,7 +509,7 @@ func makeHappyOauthHelper( t *testing.T, authRequest *http.Request, store *storage.MemoryStore, -) (fosite.OAuth2Provider, string) { +) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) { t.Helper() jwtSigningKey := generateJWTSigningKey(t) @@ -533,7 +539,7 @@ func makeHappyOauthHelper( authResponder, err := oauthHelper.NewAuthorizeResponse(ctx, authRequester, session) require.NoError(t, err) - return oauthHelper, authResponder.GetCode() + return oauthHelper, authResponder.GetCode(), jwtSigningKey } func generateJWTSigningKey(t *testing.T) *ecdsa.PrivateKey { @@ -765,6 +771,23 @@ func requireValidAuthRequest( require.Equal(t, goodSubject, session.Subject) } +func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKey *ecdsa.PrivateKey) { + idToken, ok := body["id_token"] + require.Truef(t, ok, "body did not contain 'id_token': %s", body) + idTokenString, ok := idToken.(string) + require.Truef(t, ok, "wanted id_token to be a string, but got %T", idToken) + + // The go-oidc library will validate the signature and the client claim in the ID token. + keySet := newStaticKeySet(jwtSigningKey) // TODO: implement this static key set + verifyConfig := coreosoidc.Config{ClientID: goodClient, SupportedSigningAlgs: []string{coreosoidc.ES256}} + verifier := coreosoidc.NewVerifier(goodIssuer, keySet, &verifyConfig) + token, err := verifier.Verify(context.Background(), idTokenString) + require.NoError(t, err) + + // TODO: we will need to validate all of the other claims! + _ = token +} + // TODO: de-dup me. func requireEqualContentType(t *testing.T, actual string, expected string) { t.Helper() From 1dd7c82af658bf395123a4c4094f251a9db058b7 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 2 Dec 2020 16:55:48 -0800 Subject: [PATCH 05/20] Added id token verification --- internal/oidc/token/token_handler_test.go | 61 +++++++++++++++++++++-- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index e6a594db..86c5acad 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -5,12 +5,14 @@ package token import ( "context" + "crypto" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" "crypto/sha256" "encoding/base64" "encoding/json" + "fmt" "io" "io/ioutil" "mime" @@ -27,6 +29,7 @@ import ( "github.com/ory/fosite/storage" "github.com/ory/fosite/token/jwt" "github.com/stretchr/testify/require" + "gopkg.in/square/go-jose.v2" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" @@ -778,14 +781,66 @@ func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKe require.Truef(t, ok, "wanted id_token to be a string, but got %T", idToken) // The go-oidc library will validate the signature and the client claim in the ID token. - keySet := newStaticKeySet(jwtSigningKey) // TODO: implement this static key set + keySet := newStaticKeySet(jwtSigningKey.Public()) verifyConfig := coreosoidc.Config{ClientID: goodClient, SupportedSigningAlgs: []string{coreosoidc.ES256}} verifier := coreosoidc.NewVerifier(goodIssuer, keySet, &verifyConfig) token, err := verifier.Verify(context.Background(), idTokenString) require.NoError(t, err) - // TODO: we will need to validate all of the other claims! - _ = token + var claims struct { + Subject string `json:"sub"` + Audience []string `json:"aud"` + Issuer string `json:"iss"` + JTI string `json:"jti"` + Nonce string `json:"nonce"` + AccessTokenHash string `json:"at_hash"` + ExpiresAt int64 `json:"exp"` + IssuedAt int64 `json:"iat"` + RequestedAt int64 `json:"rat"` + AuthTime int64 `json:"auth_time"` + } + idTokenFields := []string{"sub", "aud", "iss", "jti", "nonce", "auth_time", "at_hash", "exp", "iat", "rat"} + + // make sure that these are the only fields in the token + var m map[string]interface{} + require.NoError(t, token.Claims(&m)) + require.ElementsMatch(t, idTokenFields, getMapKeys(m)) + + // verify each of the claims + err = token.Claims(&claims) + require.NoError(t, err) + require.Equal(t, goodSubject, claims.Subject) + require.Len(t, claims.Audience, 1) + require.Equal(t, goodClient, claims.Audience[0]) + require.Equal(t, goodIssuer, claims.Issuer) + require.NotEmpty(t, claims.JTI) + require.Equal(t, goodNonce, claims.Nonce) + require.NotEmpty(t, claims.AccessTokenHash) + + expiresAt := time.Unix(claims.ExpiresAt, 0) + issuedAt := time.Unix(claims.IssuedAt, 0) + requestedAt := time.Unix(claims.RequestedAt, 0) + authTime := time.Unix(claims.AuthTime, 0) + requireTimeInDelta(t, time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), expiresAt, timeComparisonFudgeSeconds*time.Second) + requireTimeInDelta(t, time.Now().UTC(), issuedAt, timeComparisonFudgeSeconds*time.Second) + requireTimeInDelta(t, goodRequestedAtTime, requestedAt, timeComparisonFudgeSeconds*time.Second) + requireTimeInDelta(t, goodAuthTime, authTime, timeComparisonFudgeSeconds*time.Second) +} + +func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { + return &staticKeySet{publicKey} +} + +type staticKeySet struct { + publicKey crypto.PublicKey +} + +func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { + jws, err := jose.ParseSigned(jwt) + if err != nil { + return nil, fmt.Errorf("oidc: malformed jwt: %v", err) + } + return jws.Verify(s.publicKey) } // TODO: de-dup me. From fe2e2bdff1da00823f634dac5bad0cb366af6f2a Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 3 Dec 2020 07:46:07 -0500 Subject: [PATCH 06/20] Our ID token signing algorithm is ES256, not RS256 We are currently using EC keys to sign ID tokens, so we should reflect that in our OIDC discovery metadata. Signed-off-by: Andrew Keesler --- internal/oidc/discovery/discovery_handler.go | 2 +- internal/oidc/discovery/discovery_handler_test.go | 2 +- test/integration/supervisor_discovery_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/oidc/discovery/discovery_handler.go b/internal/oidc/discovery/discovery_handler.go index 04039c5b..c6d8f666 100644 --- a/internal/oidc/discovery/discovery_handler.go +++ b/internal/oidc/discovery/discovery_handler.go @@ -56,7 +56,7 @@ func NewHandler(issuerURL string) http.Handler { JWKSURI: issuerURL + oidc.JWKSEndpointPath, ResponseTypesSupported: []string{"code"}, SubjectTypesSupported: []string{"public"}, - IDTokenSigningAlgValuesSupported: []string{"RS256"}, + IDTokenSigningAlgValuesSupported: []string{"ES256"}, TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, TokenEndpointAuthSigningAlgoValuesSupported: []string{"RS256"}, ScopesSupported: []string{"openid", "offline"}, diff --git a/internal/oidc/discovery/discovery_handler_test.go b/internal/oidc/discovery/discovery_handler_test.go index c8fef948..b7d5f84a 100644 --- a/internal/oidc/discovery/discovery_handler_test.go +++ b/internal/oidc/discovery/discovery_handler_test.go @@ -41,7 +41,7 @@ func TestDiscovery(t *testing.T) { JWKSURI: "https://some-issuer.com/some/path/jwks.json", ResponseTypesSupported: []string{"code"}, SubjectTypesSupported: []string{"public"}, - IDTokenSigningAlgValuesSupported: []string{"RS256"}, + IDTokenSigningAlgValuesSupported: []string{"ES256"}, TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, TokenEndpointAuthSigningAlgoValuesSupported: []string{"RS256"}, ScopesSupported: []string{"openid", "offline"}, diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 7df52509..396c0f48 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -479,7 +479,7 @@ func requireWellKnownEndpointIsWorking(t *testing.T, supervisorScheme, superviso "response_types_supported": ["code"], "claims_supported": ["groups"], "subject_types_supported": ["public"], - "id_token_signing_alg_values_supported": ["RS256"] + "id_token_signing_alg_values_supported": ["ES256"] }`) expectedJSON := fmt.Sprintf(expectedResultTemplate, issuerName, issuerName, issuerName, issuerName) From 67bf54a9f9ab5b9f79417908f9dcea57b72f36de Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 3 Dec 2020 11:05:47 -0800 Subject: [PATCH 07/20] Use an interface for storage in token_handler_test.go Signed-off-by: Aram Price --- internal/oidc/token/token_handler_test.go | 35 +++++++++++++++-------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 86c5acad..a5b957a5 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -25,7 +25,9 @@ import ( coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" + "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" + "github.com/ory/fosite/handler/pkce" "github.com/ory/fosite/storage" "github.com/ory/fosite/token/jwt" "github.com/stretchr/testify/require" @@ -53,6 +55,14 @@ const ( timeComparisonFudgeSeconds = 15 ) +type CombinedStorage interface { + oauth2.TokenRevocationStorage + oauth2.CoreStorage + openid.OpenIDConnectRequestStorage + pkce.PKCERequestStorage + fosite.ClientManager +} + var ( goodAuthTime = time.Date(1, 2, 3, 4, 5, 6, 7, time.Local) goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.Local) @@ -196,7 +206,7 @@ func TestTokenEndpoint(t *testing.T) { name string authRequest func(authRequest *http.Request) - storage func(t *testing.T, s *storage.MemoryStore, authCode string) + storage func(t *testing.T, s CombinedStorage, authCode string) request func(r *http.Request, authCode string) wantStatus int @@ -313,7 +323,7 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "auth code is invalidated", - storage: func(t *testing.T, s *storage.MemoryStore, authCode string) { + storage: func(t *testing.T, s CombinedStorage, authCode string) { err := s.InvalidateAuthorizeCodeSession(context.Background(), getFositeDataSignature(t, authCode)) require.NoError(t, err) }, @@ -364,6 +374,8 @@ func TestTokenEndpoint(t *testing.T) { } oauthStore := storage.NewMemoryStore() + // Add the Pinniped CLI client. + oauthStore.Clients[goodClient] = oidc.PinnipedCLIOIDCClient() oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) if test.storage != nil { test.storage(t, oauthStore, authCode) @@ -407,6 +419,8 @@ func TestTokenEndpoint(t *testing.T) { t.Run("auth code is used twice", func(t *testing.T) { authRequest := deepCopyRequestForm(happyAuthRequest) oauthStore := storage.NewMemoryStore() + // Add the Pinniped CLI client. + oauthStore.Clients[goodClient] = oidc.PinnipedCLIOIDCClient() oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) subject := NewHandler(oauthHelper) @@ -511,15 +525,12 @@ func getFositeDataSignature(t *testing.T, data string) string { func makeHappyOauthHelper( t *testing.T, authRequest *http.Request, - store *storage.MemoryStore, + store CombinedStorage, ) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) { t.Helper() jwtSigningKey := generateJWTSigningKey(t) - oauthHelper := oidc.FositeOauth2Helper(goodIssuer, store, []byte(hmacSecret), jwtSigningKey) - - // Add the Pinniped CLI client. - store.Clients[goodClient] = oidc.PinnipedCLIOIDCClient() + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwtSigningKey) // Simulate the auth endpoint running so Fosite code will fill the store with realistic values. // @@ -570,7 +581,7 @@ func doSHA256(s string) string { func requireInvalidAuthCodeStorage( t *testing.T, code string, - storage *storage.MemoryStore, + storage CombinedStorage, ) { t.Helper() @@ -582,7 +593,7 @@ func requireInvalidAuthCodeStorage( func requireValidAccessTokenStorage( t *testing.T, body map[string]interface{}, - storage *storage.MemoryStore, + storage CombinedStorage, wantGrantedOpenidScope bool, ) { t.Helper() @@ -631,7 +642,7 @@ func requireValidAccessTokenStorage( func requireInvalidAccessTokenStorage( t *testing.T, body map[string]interface{}, - storage *storage.MemoryStore, + storage CombinedStorage, ) { t.Helper() @@ -647,7 +658,7 @@ func requireInvalidAccessTokenStorage( func requireInvalidPKCEStorage( t *testing.T, code string, - storage *storage.MemoryStore, + storage CombinedStorage, ) { t.Helper() @@ -661,7 +672,7 @@ func requireValidOIDCStorage( t *testing.T, body map[string]interface{}, code string, - storage *storage.MemoryStore, + storage CombinedStorage, wantGrantedOpenidScope bool, ) { t.Helper() From 05085d8e23a5face115d7c7ea8662434e099358b Mon Sep 17 00:00:00 2001 From: aram price Date: Thu, 3 Dec 2020 11:26:36 -0800 Subject: [PATCH 08/20] Use anonymous interface in test for Storage --- internal/oidc/token/token_handler_test.go | 52 +++++++++++++++-------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index a5b957a5..fd52004e 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -55,14 +55,6 @@ const ( timeComparisonFudgeSeconds = 15 ) -type CombinedStorage interface { - oauth2.TokenRevocationStorage - oauth2.CoreStorage - openid.OpenIDConnectRequestStorage - pkce.PKCERequestStorage - fosite.ClientManager -} - var ( goodAuthTime = time.Date(1, 2, 3, 4, 5, 6, 7, time.Local) goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.Local) @@ -206,8 +198,18 @@ func TestTokenEndpoint(t *testing.T) { name string authRequest func(authRequest *http.Request) - storage func(t *testing.T, s CombinedStorage, authCode string) - request func(r *http.Request, authCode string) + storage func( + t *testing.T, + s interface { + oauth2.TokenRevocationStorage + oauth2.CoreStorage + openid.OpenIDConnectRequestStorage + pkce.PKCERequestStorage + fosite.ClientManager + }, + authCode string, + ) + request func(r *http.Request, authCode string) wantStatus int wantBodyFields []string @@ -323,7 +325,17 @@ func TestTokenEndpoint(t *testing.T) { }, { name: "auth code is invalidated", - storage: func(t *testing.T, s CombinedStorage, authCode string) { + storage: func( + t *testing.T, + s interface { + oauth2.TokenRevocationStorage + oauth2.CoreStorage + openid.OpenIDConnectRequestStorage + pkce.PKCERequestStorage + fosite.ClientManager + }, + authCode string, + ) { err := s.InvalidateAuthorizeCodeSession(context.Background(), getFositeDataSignature(t, authCode)) require.NoError(t, err) }, @@ -525,7 +537,13 @@ func getFositeDataSignature(t *testing.T, data string) string { func makeHappyOauthHelper( t *testing.T, authRequest *http.Request, - store CombinedStorage, + store interface { + oauth2.TokenRevocationStorage + oauth2.CoreStorage + openid.OpenIDConnectRequestStorage + pkce.PKCERequestStorage + fosite.ClientManager + }, ) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) { t.Helper() @@ -581,7 +599,7 @@ func doSHA256(s string) string { func requireInvalidAuthCodeStorage( t *testing.T, code string, - storage CombinedStorage, + storage oauth2.CoreStorage, ) { t.Helper() @@ -593,7 +611,7 @@ func requireInvalidAuthCodeStorage( func requireValidAccessTokenStorage( t *testing.T, body map[string]interface{}, - storage CombinedStorage, + storage oauth2.CoreStorage, wantGrantedOpenidScope bool, ) { t.Helper() @@ -642,7 +660,7 @@ func requireValidAccessTokenStorage( func requireInvalidAccessTokenStorage( t *testing.T, body map[string]interface{}, - storage CombinedStorage, + storage oauth2.CoreStorage, ) { t.Helper() @@ -658,7 +676,7 @@ func requireInvalidAccessTokenStorage( func requireInvalidPKCEStorage( t *testing.T, code string, - storage CombinedStorage, + storage pkce.PKCERequestStorage, ) { t.Helper() @@ -672,7 +690,7 @@ func requireValidOIDCStorage( t *testing.T, body map[string]interface{}, code string, - storage CombinedStorage, + storage openid.OpenIDConnectRequestStorage, wantGrantedOpenidScope bool, ) { t.Helper() From 58237d0e7dece5ded997ee9360099b2dd0d508a3 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 3 Dec 2020 15:34:58 -0500 Subject: [PATCH 09/20] WIP: start to wire signing key into token handler This commit includes a failing test (amongst other compiler failures) for the dynamic signing key fetcher that we will inject into fosite. We are checking it in so that we can pass the WIP off. Signed-off-by: Margo Crawford --- .../supervisorconfig/jwks_observer.go | 32 +++- .../supervisorconfig/jwks_observer_test.go | 74 ++++++++- .../dynamic_open_id_connect_ecdsa_strategy.go | 42 +++++ ...mic_open_id_connect_ecdsa_strategy_test.go | 136 ++++++++++++++++ internal/oidc/jwks/dynamic_jwks_provider.go | 25 ++- internal/oidc/jwks/jwks_handler.go | 2 +- internal/oidc/jwks/jwks_handler_test.go | 8 +- internal/oidc/oidc.go | 7 +- internal/oidc/provider/manager/manager.go | 5 + .../oidc/provider/manager/manager_test.go | 149 +++++++++++++++--- internal/oidc/token/token_handler_test.go | 2 + 11 files changed, 432 insertions(+), 50 deletions(-) create mode 100644 internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go create mode 100644 internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go index 0d6df5b6..5c01c5b4 100644 --- a/internal/controller/supervisorconfig/jwks_observer.go +++ b/internal/controller/supervisorconfig/jwks_observer.go @@ -24,7 +24,10 @@ type jwksObserverController struct { } type IssuerToJWKSMapSetter interface { - SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) + SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, + ) } // Returns a controller which watches all of the OIDCProviders and their corresponding Secrets @@ -70,6 +73,7 @@ func (c *jwksObserverController) Sync(ctx controllerlib.Context) error { // Rebuild the whole map on any change to any Secret or OIDCProvider, because either can have changes that // can cause the map to need to be updated. issuerToJWKSMap := map[string]*jose.JSONWebKeySet{} + issuerToActiveJWKMap := map[string]*jose.JSONWebKey{} for _, provider := range allProviders { secretRef := provider.Status.JWKSSecret @@ -78,17 +82,33 @@ func (c *jwksObserverController) Sync(ctx controllerlib.Context) error { plog.Debug("jwksObserverController Sync could not find JWKS secret", "namespace", ns, "secretName", secretRef.Name) continue } - jwkFromSecret := jose.JSONWebKeySet{} - err = json.Unmarshal(jwksSecret.Data[jwksKey], &jwkFromSecret) + + jwksFromSecret := jose.JSONWebKeySet{} + err = json.Unmarshal(jwksSecret.Data[jwksKey], &jwksFromSecret) if err != nil { plog.Debug("jwksObserverController Sync found a JWKS secret with Data in an unexpected format", "namespace", ns, "secretName", secretRef.Name) continue } - issuerToJWKSMap[provider.Spec.Issuer] = &jwkFromSecret + + activeJWKFromSecret := jose.JSONWebKey{} + err = json.Unmarshal(jwksSecret.Data[activeJWKKey], &activeJWKFromSecret) + if err != nil { + plog.Debug("jwksObserverController Sync found an active JWK secret with Data in an unexpected format", "namespace", ns, "secretName", secretRef.Name) + continue + } + + issuerToJWKSMap[provider.Spec.Issuer] = &jwksFromSecret + issuerToActiveJWKMap[provider.Spec.Issuer] = &activeJWKFromSecret } - plog.Debug("jwksObserverController Sync updated the JWKS cache", "issuerCount", len(issuerToJWKSMap)) - c.issuerToJWKSSetter.SetIssuerToJWKSMap(issuerToJWKSMap) + plog.Debug( + "jwksObserverController Sync updated the JWKS cache", + "issuerJWKSCount", + len(issuerToJWKSMap), + "issuerActiveJWKCount", + len(issuerToActiveJWKMap), + ) + c.issuerToJWKSSetter.SetIssuerToJWKSMap(issuerToJWKSMap, issuerToActiveJWKMap) return nil } diff --git a/internal/controller/supervisorconfig/jwks_observer_test.go b/internal/controller/supervisorconfig/jwks_observer_test.go index 8bf920ef..ad7323b8 100644 --- a/internal/controller/supervisorconfig/jwks_observer_test.go +++ b/internal/controller/supervisorconfig/jwks_observer_test.go @@ -96,13 +96,18 @@ func TestJWKSObserverControllerInformerFilters(t *testing.T) { } type fakeIssuerToJWKSMapSetter struct { - setIssuerToJWKSMapWasCalled bool - issuerToJWKSMapReceived map[string]*jose.JSONWebKeySet + setIssuerToJWKSMapWasCalled bool + issuerToJWKSMapReceived map[string]*jose.JSONWebKeySet + issuerToActiveJWKMapReceived map[string]*jose.JSONWebKey } -func (f *fakeIssuerToJWKSMapSetter) SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) { +func (f *fakeIssuerToJWKSMapSetter) SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, +) { f.setIssuerToJWKSMapWasCalled = true f.issuerToJWKSMapReceived = issuerToJWKSMap + f.issuerToActiveJWKMapReceived = issuerToActiveJWKMap } func TestJWKSObserverControllerSync(t *testing.T) { @@ -181,6 +186,7 @@ func TestJWKSObserverControllerSync(t *testing.T) { r.True(issuerToJWKSSetter.setIssuerToJWKSMapWasCalled) r.Empty(issuerToJWKSSetter.issuerToJWKSMapReceived) + r.Empty(issuerToJWKSSetter.issuerToActiveJWKMapReceived) }) }) @@ -212,10 +218,30 @@ func TestJWKSObserverControllerSync(t *testing.T) { Namespace: installedInNamespace, }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-secret-issuer.com"}, + Status: v1alpha1.OIDCProviderStatus{ + JWKSSecret: corev1.LocalObjectReference{Name: "bad-secret-name"}, + }, + } + oidcProviderWithBadJWKSSecret := &v1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-jwks-secret-oidcprovider", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-jwks-secret-issuer.com"}, Status: v1alpha1.OIDCProviderStatus{ JWKSSecret: corev1.LocalObjectReference{Name: "bad-jwks-secret-name"}, }, } + oidcProviderWithBadActiveJWKSecret := &v1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-active-jwk-secret-oidcprovider", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-active-jwk-secret-issuer.com"}, + Status: v1alpha1.OIDCProviderStatus{ + JWKSSecret: corev1.LocalObjectReference{Name: "bad-active-jwk-secret-name"}, + }, + } oidcProviderWithGoodSecret1 := &v1alpha1.OIDCProvider{ ObjectMeta: metav1.ObjectMeta{ Name: "good-secret-oidcprovider1", @@ -260,24 +286,48 @@ func TestJWKSObserverControllerSync(t *testing.T) { "jwks": []byte(`{"keys": [` + expectedJWK2 + `]}`), }, } + badSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-secret-name", + Namespace: installedInNamespace, + }, + Data: map[string][]byte{"junk": nil}, + } badJWKSSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "bad-jwks-secret-name", Namespace: installedInNamespace, }, - Data: map[string][]byte{"junk": nil}, + Data: map[string][]byte{ + "activeJWK": []byte(expectedJWK2), + "jwks": []byte("bad"), + }, + } + badActiveJWKSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-active-jwk-secret-name", + Namespace: installedInNamespace, + }, + Data: map[string][]byte{ + "activeJWK": []byte("bad"), + "jwks": []byte(`{"keys": [` + expectedJWK2 + `]}`), + }, } r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithoutSecret1)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithoutSecret2)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithBadSecret)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithBadJWKSSecret)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithBadActiveJWKSecret)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithGoodSecret1)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithGoodSecret2)) r.NoError(kubeInformerClient.Tracker().Add(goodJWKSSecret1)) r.NoError(kubeInformerClient.Tracker().Add(goodJWKSSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(badSecret)) r.NoError(kubeInformerClient.Tracker().Add(badJWKSSecret)) + r.NoError(kubeInformerClient.Tracker().Add(badActiveJWKSecret)) }) - requireJWKJSON := func(expectedJWKJSON string, actualJWKS *jose.JSONWebKeySet) { + requireJWKSJSON := func(expectedJWKJSON string, actualJWKS *jose.JSONWebKeySet) { r.NotNil(actualJWKS) r.Len(actualJWKS.Keys, 1) actualJWK := actualJWKS.Keys[0] @@ -286,16 +336,26 @@ func TestJWKSObserverControllerSync(t *testing.T) { r.JSONEq(expectedJWKJSON, string(actualJWKJSON)) } + requireJWKJSON := func(expectedJWKJSON string, actualJWK *jose.JSONWebKey) { + r.NotNil(actualJWK) + actualJWKJSON, err := json.Marshal(actualJWK) + r.NoError(err) + r.JSONEq(expectedJWKJSON, string(actualJWKJSON)) + } + it("updates the issuerToJWKSSetter's map to include only the issuers that had valid JWKS", func() { startInformersAndController() r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.True(issuerToJWKSSetter.setIssuerToJWKSMapWasCalled) r.Len(issuerToJWKSSetter.issuerToJWKSMapReceived, 2) + r.Len(issuerToJWKSSetter.issuerToActiveJWKMapReceived, 2) // the actual JWK should match the one from the test fixture that was put into the secret - requireJWKJSON(expectedJWK1, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret1.com"]) - requireJWKJSON(expectedJWK2, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret2.com"]) + requireJWKSJSON(expectedJWK1, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret1.com"]) + requireJWKJSON(expectedJWK1, issuerToJWKSSetter.issuerToActiveJWKMapReceived["https://issuer-with-good-secret1.com"]) + requireJWKSJSON(expectedJWK2, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret2.com"]) + requireJWKJSON(expectedJWK2, issuerToJWKSSetter.issuerToActiveJWKMapReceived["https://issuer-with-good-secret2.com"]) }) }) }, spec.Parallel(), spec.Report(report.Terminal{})) diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go new file mode 100644 index 00000000..6d7d837f --- /dev/null +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go @@ -0,0 +1,42 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + + "github.com/ory/fosite" + "github.com/ory/fosite/compose" + "github.com/ory/fosite/handler/openid" + + "go.pinniped.dev/internal/oidc/jwks" +) + +// TODO: doc me. +type dynamicOpenIDConnectECDSAStrategy struct { + issuer string + fositeConfig *compose.Config + jwksProvider jwks.DynamicJWKSProvider +} + +var _ openid.OpenIDConnectTokenStrategy = &dynamicOpenIDConnectECDSAStrategy{} + +func newDynamicOpenIDConnectECDSAStrategy( + issuer string, + fositeConfig *compose.Config, + jwksProvider jwks.DynamicJWKSProvider, +) *dynamicOpenIDConnectECDSAStrategy { + return &dynamicOpenIDConnectECDSAStrategy{ + issuer: issuer, + fositeConfig: fositeConfig, + jwksProvider: jwksProvider, + } +} + +func (s *dynamicOpenIDConnectECDSAStrategy) GenerateIDToken( + ctx context.Context, + requester fosite.Requester, +) (string, error) { + return "", nil +} diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go new file mode 100644 index 00000000..93ba46cc --- /dev/null +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go @@ -0,0 +1,136 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "fmt" + "testing" + + coreosoidc "github.com/coreos/go-oidc" + "github.com/ory/fosite" + "github.com/ory/fosite/compose" + "github.com/stretchr/testify/require" + "go.pinniped.dev/internal/oidc/jwks" + "gopkg.in/square/go-jose.v2" +) + +func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { + const ( + goodIssuer = "https://some-good-issuer.com" + clientID = "some-client-id" + ) + + ecPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + rsaPrivateKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + tests := []struct { + name string + issuer string + jwksProvider func(jwks.DynamicJWKSProvider) + wantError string + wantSigningJWK *jose.JSONWebKey + }{ + { + name: "jwks provider does contain signing key for issuer", + issuer: goodIssuer, + jwksProvider: func(provider jwks.DynamicJWKSProvider) { + provider.SetIssuerToJWKSMap( + nil, + map[string]*jose.JSONWebKey{ + goodIssuer: { + Key: ecPrivateKey, + }, + }, + ) + }, + wantSigningJWK: &jose.JSONWebKey{ + Key: ecPrivateKey, + }, + }, + { + name: "jwks provider does not contain signing key for issuer", + issuer: goodIssuer, + wantError: "some unkonwn key error", + }, + { + name: "jwks provider contains signing key of wrong type for issuer", + issuer: goodIssuer, + jwksProvider: func(provider jwks.DynamicJWKSProvider) { + provider.SetIssuerToJWKSMap( + nil, + map[string]*jose.JSONWebKey{ + goodIssuer: { + Key: rsaPrivateKey, + }, + }, + ) + }, + wantError: "some invalid key type error", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + jwksProvider := jwks.NewDynamicJWKSProvider() + if test.jwksProvider != nil { + test.jwksProvider(jwksProvider) + } + s := newDynamicOpenIDConnectECDSAStrategy(test.issuer, &compose.Config{}, jwksProvider) + + requester := &fosite.Request{ + Client: &fosite.DefaultClient{ + ID: clientID, + }, + // Session: fositeopenid.DefaultSession{}, + } + idToken, err := s.GenerateIDToken(context.Background(), requester) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + } else { + require.NoError(t, err) + + // TODO: common-ize this code with token endpoint test. + // TODO: make more assertions about ID token + + privateKey, ok := test.wantSigningJWK.Key.(*ecdsa.PrivateKey) + require.True(t, ok, "wanted private key to be *ecdsa.PrivateKey, but was %T", test.wantSigningJWK) + + keySet := newStaticKeySet(privateKey.Public()) + verifyConfig := coreosoidc.Config{ + ClientID: clientID, + SupportedSigningAlgs: []string{coreosoidc.ES256}, + } + verifier := coreosoidc.NewVerifier(test.issuer, keySet, &verifyConfig) + _, err := verifier.Verify(context.Background(), idToken) + require.NoError(t, err) + } + }) + } +} + +// TODO: de-dep me. +func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { + return &staticKeySet{publicKey} +} + +type staticKeySet struct { + publicKey crypto.PublicKey +} + +func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { + jws, err := jose.ParseSigned(jwt) + if err != nil { + return nil, fmt.Errorf("oidc: malformed jwt: %v", err) + } + return jws.Verify(s.publicKey) +} diff --git a/internal/oidc/jwks/dynamic_jwks_provider.go b/internal/oidc/jwks/dynamic_jwks_provider.go index 8672fd2f..fa156e3c 100644 --- a/internal/oidc/jwks/dynamic_jwks_provider.go +++ b/internal/oidc/jwks/dynamic_jwks_provider.go @@ -10,29 +10,38 @@ import ( ) type DynamicJWKSProvider interface { - SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) - GetJWKS(issuerName string) *jose.JSONWebKeySet + SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, + ) + GetJWKS(issuerName string) (jwks *jose.JSONWebKeySet, activeJWK *jose.JSONWebKey) } type dynamicJWKSProvider struct { - issuerToJWKSMap map[string]*jose.JSONWebKeySet - mutex sync.RWMutex + issuerToJWKSMap map[string]*jose.JSONWebKeySet + issuerToActiveJWKMap map[string]*jose.JSONWebKey + mutex sync.RWMutex } func NewDynamicJWKSProvider() DynamicJWKSProvider { return &dynamicJWKSProvider{ - issuerToJWKSMap: map[string]*jose.JSONWebKeySet{}, + issuerToJWKSMap: map[string]*jose.JSONWebKeySet{}, + issuerToActiveJWKMap: map[string]*jose.JSONWebKey{}, } } -func (p *dynamicJWKSProvider) SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) { +func (p *dynamicJWKSProvider) SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, +) { p.mutex.Lock() // acquire a write lock defer p.mutex.Unlock() p.issuerToJWKSMap = issuerToJWKSMap + p.issuerToActiveJWKMap = issuerToActiveJWKMap } -func (p *dynamicJWKSProvider) GetJWKS(issuerName string) *jose.JSONWebKeySet { +func (p *dynamicJWKSProvider) GetJWKS(issuerName string) (*jose.JSONWebKeySet, *jose.JSONWebKey) { p.mutex.RLock() // acquire a read lock defer p.mutex.RUnlock() - return p.issuerToJWKSMap[issuerName] + return p.issuerToJWKSMap[issuerName], p.issuerToActiveJWKMap[issuerName] } diff --git a/internal/oidc/jwks/jwks_handler.go b/internal/oidc/jwks/jwks_handler.go index bdb036e6..2c975e95 100644 --- a/internal/oidc/jwks/jwks_handler.go +++ b/internal/oidc/jwks/jwks_handler.go @@ -19,7 +19,7 @@ func NewHandler(issuerName string, provider DynamicJWKSProvider) http.Handler { return } - jwks := provider.GetJWKS(issuerName) + jwks, _ := provider.GetJWKS(issuerName) if jwks == nil { http.Error(w, "JWKS not found for requested issuer", http.StatusNotFound) diff --git a/internal/oidc/jwks/jwks_handler_test.go b/internal/oidc/jwks/jwks_handler_test.go index d80e66d5..37f53e8c 100644 --- a/internal/oidc/jwks/jwks_handler_test.go +++ b/internal/oidc/jwks/jwks_handler_test.go @@ -105,8 +105,12 @@ func newDynamicJWKSProvider(t *testing.T, issuer string, jwksJSON string) Dynami var keySet jose.JSONWebKeySet err := json.Unmarshal([]byte(jwksJSON), &keySet) require.NoError(t, err) - jwksProvider.SetIssuerToJWKSMap(map[string]*jose.JSONWebKeySet{ + issuerToJWKSMap := map[string]*jose.JSONWebKeySet{ issuer: &keySet, - }) + } + issuerToActiveJWKMap := map[string]*jose.JSONWebKey{ + issuer: &keySet.Keys[0], + } + jwksProvider.SetIssuerToJWKSMap(issuerToJWKSMap, issuerToActiveJWKMap) return jwksProvider } diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 14f9a725..c5e6abf0 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -5,13 +5,13 @@ package oidc import ( - "crypto/ecdsa" "time" "github.com/ory/fosite" "github.com/ory/fosite/compose" "go.pinniped.dev/internal/oidc/csrftoken" + "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -94,7 +94,7 @@ func FositeOauth2Helper( oauthStore interface{}, issuer string, hmacSecretOfLengthAtLeast32 []byte, - jwtSigningKey *ecdsa.PrivateKey, + jwksProvider jwks.DynamicJWKSProvider, ) fosite.OAuth2Provider { oauthConfig := &compose.Config{ AuthorizeCodeLifespan: 3 * time.Minute, // seems more than long enough to exchange a code @@ -122,7 +122,8 @@ func FositeOauth2Helper( &compose.CommonStrategy{ // Note that Fosite requires the HMAC secret to be at least 32 bytes. CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), - OpenIDConnectTokenStrategy: compose.NewOpenIDConnectECDSAStrategy(oauthConfig, jwtSigningKey), + OpenIDConnectTokenStrategy: newDynamicOpenIDConnectECDSAStrategy(issuer, oauthConfig, jwksProvider), + // OpenIDConnectTokenStrategy: compose.NewOpenIDConnectECDSAStrategy(oauthConfig, jwtSigningKey), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. compose.OAuth2AuthorizeExplicitFactory, diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 1d80a0fc..b1d9d6b5 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -18,6 +18,7 @@ import ( "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/token" "go.pinniped.dev/internal/plog" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -115,6 +116,10 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { issuer+oidc.CallbackEndpointPath, ) + m.providerHandlers[(issuerHostWithPath + oidc.TokenEndpointPath)] = token.NewHandler( + oauthHelperWithKubeStorage, + ) + plog.Debug("oidc provider manager added or updated issuer", "issuer", issuer) } } diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index a3f8090d..d3cf1cc9 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -5,7 +5,12 @@ package manager import ( "context" + "crypto" + "crypto/ecdsa" + "crypto/sha256" + "encoding/base64" "encoding/json" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -13,6 +18,7 @@ import ( "strings" "testing" + coreosoidc "github.com/coreos/go-oidc" "github.com/sclevine/spec" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" @@ -48,13 +54,22 @@ func TestManager(t *testing.T) { issuer2DifferentCaseHostname = "https://exAmPlE.Com/some/path/more/deeply/nested/path" issuer2KeyID = "issuer2-key" upstreamIDPAuthorizationURL = "https://test-upstream.com/auth" + downstreamClientID = "pinniped-cli" downstreamRedirectURL = "http://127.0.0.1:12345/callback" + + downstreamPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" ) newGetRequest := func(url string) *http.Request { return httptest.NewRequest(http.MethodGet, url, nil) } + newPostRequest := func(url, body string) *http.Request { + req := httptest.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + return req + } + requireDiscoveryRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedIssuerInResponse string) { recorder := httptest.NewRecorder() @@ -106,7 +121,7 @@ func TestManager(t *testing.T) { return csrfCookieValue, redirectStateParam } - requireCallbackRequestToBeHandled := func(requestIssuer, requestURLSuffix, csrfCookieValue string) { + requireCallbackRequestToBeHandled := func(requestIssuer, requestURLSuffix, csrfCookieValue string) string { recorder := httptest.NewRecorder() numberOfKubeActionsBeforeThisRequest := len(kubeClient.Actions()) @@ -139,9 +154,56 @@ func TestManager(t *testing.T) { // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+3, "did not perform any kube actions during the callback request, but should have") + + // Return the important parts of the response so we can use them in our next request to the token endpoint. + return actualLocationQueryParams.Get("code") } - requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) { + requireTokenRequestToBeHandled := func(requestIssuer, authCode string, jwks *jose.JSONWebKeySet) { + recorder := httptest.NewRecorder() + + numberOfKubeActionsBeforeThisRequest := len(kubeClient.Actions()) + + tokenRequestBody := url.Values{ + "code": []string{authCode}, + "client_id": []string{downstreamClientID}, + "redirect_uri": []string{downstreamRedirectURL}, + "code_verifier": []string{downstreamPKCECodeVerifier}, + "grant_type": []string{"authorization_code"}, + }.Encode() + subject.ServeHTTP(recorder, newPostRequest(requestIssuer+oidc.TokenEndpointPath, tokenRequestBody)) + + r.False(fallbackHandlerWasCalled) + + // Minimal check to ensure that the right endpoint was called + var body map[string]interface{} + r.Equal(http.StatusOK, recorder.Code) + r.NoError(json.Unmarshal(recorder.Body.Bytes(), &body)) + r.Contains(body, "id_token") + r.Contains(body, "access_token") + + // Validate ID token is signed by the correct JWK to make sure we wired the token endpoint + // signing key correctly. + // TODO: common-ize this code with token endpoint test. + idToken, ok := body["id_token"].(string) + r.True(ok, "wanted id_token type to be string, but was %T", body["id_token"]) + + r.GreaterOrEqual(len(jwks.Keys), 1) + privateKey, ok := jwks.Keys[0].Key.(*ecdsa.PrivateKey) + r.True(ok, "wanted private key to be *ecdsa.PrivateKey, but was %T", jwks.Keys[0].Key) + + keySet := newStaticKeySet(privateKey.Public()) + verifyConfig := coreosoidc.Config{ClientID: downstreamClientID, SupportedSigningAlgs: []string{coreosoidc.ES256}} + verifier := coreosoidc.NewVerifier(requestIssuer, keySet, &verifyConfig) + _, err := verifier.Verify(context.Background(), idToken) + r.NoError(err) + + // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. + r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+7, + "did not perform any kube actions during the callback request, but should have") + } + + requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) *jose.JSONWebKeySet { recorder := httptest.NewRecorder() subject.ServeHTTP(recorder, newGetRequest(requestIssuer+oidc.JWKSEndpointPath+requestURLSuffix)) @@ -156,6 +218,8 @@ func TestManager(t *testing.T) { err = json.Unmarshal(responseBody, &parsedJWKSResult) r.NoError(err) r.Equal(expectedJWKKeyID, parsedJWKSResult.Keys[0].KeyID) + + return &parsedJWKSResult } it.Before(func() { @@ -198,7 +262,7 @@ func TestManager(t *testing.T) { }) }) - newTestJWK := func(keyID string) jose.JSONWebKey { + newTestJWK := func(keyID string) *jose.JSONWebKey { testJWKSJSONString := here.Docf(` { "use": "sig", @@ -206,13 +270,14 @@ func TestManager(t *testing.T) { "kid": "%s", "crv": "P-256", "alg": "ES256", - "x": "awmmj6CIMhSoJyfsqH7sekbTeY72GGPLEy16tPWVz2U", - "y": "FcMh06uXLaq9b2MOixlLVidUkycO1u7IHOkrTi7N0aw" + "x": "9c_oMKjd_ruVIy4pA5y9quT1E-Fampx0w270OtPx5Ng", + "y": "-Y-9nfrtJdFPl-9kzXv55-Fq9Oo2AWDg5zZBs9P-Vds", + "d": "LXdNChAEtGKETBzYXiL_G0wESXceBuajE_EBQbcRuGg" } `, keyID) k := jose.JSONWebKey{} r.NoError(json.Unmarshal([]byte(testJWKSJSONString), &k)) - return k + return &k } requireRoutesMatchingRequestsToAppropriateProvider := func() { @@ -225,8 +290,8 @@ func TestManager(t *testing.T) { requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) - requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) - requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) + issuer1JWKS := requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) + issuer2JWKS := requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) // Hostnames are case-insensitive, so test that we can handle that. @@ -237,10 +302,10 @@ func TestManager(t *testing.T) { authRequestParams := "?" + url.Values{ "response_type": []string{"code"}, "scope": []string{"openid profile email"}, - "client_id": []string{"pinniped-cli"}, + "client_id": []string{downstreamClientID}, "state": []string{"some-state-value-that-is-32-byte"}, - "nonce": []string{"some-nonce-value"}, - "code_challenge": []string{"some-challenge"}, + "nonce": []string{"some-nonce-value-that-is-at-least-32-bytes"}, + "code_challenge": []string{doSHA256(downstreamPKCECodeVerifier)}, "code_challenge_method": []string{"S256"}, "redirect_uri": []string{downstreamRedirectURL}, }.Encode() @@ -258,12 +323,19 @@ func TestManager(t *testing.T) { "state": []string{upstreamStateParam}, }.Encode() - requireCallbackRequestToBeHandled(issuer1, callbackRequestParams, csrfCookieValue) - requireCallbackRequestToBeHandled(issuer2, callbackRequestParams, csrfCookieValue) + downstreamAuthCode1 := requireCallbackRequestToBeHandled(issuer1, callbackRequestParams, csrfCookieValue) + downstreamAuthCode2 := requireCallbackRequestToBeHandled(issuer2, callbackRequestParams, csrfCookieValue) // // Hostnames are case-insensitive, so test that we can handle that. - requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams, csrfCookieValue) - requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams, csrfCookieValue) + downstreamAuthCode3 := requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams, csrfCookieValue) + downstreamAuthCode4 := requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams, csrfCookieValue) + + requireTokenRequestToBeHandled(issuer1, downstreamAuthCode1, issuer1JWKS) + requireTokenRequestToBeHandled(issuer2, downstreamAuthCode2, issuer2JWKS) + + // Hostnames are case-insensitive, so test that we can handle that. + requireTokenRequestToBeHandled(issuer1DifferentCaseHostname, downstreamAuthCode3, issuer1JWKS) + requireTokenRequestToBeHandled(issuer2DifferentCaseHostname, downstreamAuthCode4, issuer2JWKS) } when("given some valid providers via SetProviders()", func() { @@ -274,10 +346,15 @@ func TestManager(t *testing.T) { r.NoError(err) subject.SetProviders(p1, p2) - dynamicJWKSProvider.SetIssuerToJWKSMap(map[string]*jose.JSONWebKeySet{ - issuer1: {Keys: []jose.JSONWebKey{newTestJWK(issuer1KeyID)}}, - issuer2: {Keys: []jose.JSONWebKey{newTestJWK(issuer2KeyID)}}, - }) + jwks := map[string]*jose.JSONWebKeySet{ + issuer1: {Keys: []jose.JSONWebKey{*newTestJWK(issuer1KeyID)}}, + issuer2: {Keys: []jose.JSONWebKey{*newTestJWK(issuer2KeyID)}}, + } + activeJWK := map[string]*jose.JSONWebKey{ + issuer1: newTestJWK(issuer1KeyID), + issuer2: newTestJWK(issuer2KeyID), + } + dynamicJWKSProvider.SetIssuerToJWKSMap(jwks, activeJWK) }) it("sends all non-matching host requests to the nextHandler", func() { @@ -312,10 +389,15 @@ func TestManager(t *testing.T) { r.NoError(err) subject.SetProviders(p2, p1) - dynamicJWKSProvider.SetIssuerToJWKSMap(map[string]*jose.JSONWebKeySet{ - issuer1: {Keys: []jose.JSONWebKey{newTestJWK(issuer1KeyID)}}, - issuer2: {Keys: []jose.JSONWebKey{newTestJWK(issuer2KeyID)}}, - }) + jwks := map[string]*jose.JSONWebKeySet{ + issuer1: {Keys: []jose.JSONWebKey{*newTestJWK(issuer1KeyID)}}, + issuer2: {Keys: []jose.JSONWebKey{*newTestJWK(issuer2KeyID)}}, + } + activeJWK := map[string]*jose.JSONWebKey{ + issuer1: newTestJWK(issuer1KeyID), + issuer2: newTestJWK(issuer2KeyID), + } + dynamicJWKSProvider.SetIssuerToJWKSMap(jwks, activeJWK) }) it("still routes matching requests to the appropriate provider", func() { @@ -324,3 +406,24 @@ func TestManager(t *testing.T) { }) }) } + +func doSHA256(s string) string { + b := sha256.Sum256([]byte(s)) + return base64.RawURLEncoding.EncodeToString(b[:]) +} + +func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { + return &staticKeySet{publicKey} +} + +type staticKeySet struct { + publicKey crypto.PublicKey +} + +func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { + jws, err := jose.ParseSigned(jwt) + if err != nil { + return nil, fmt.Errorf("oidc: malformed jwt: %v", err) + } + return jws.Verify(s.publicKey) +} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index fd52004e..ea64cc1b 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -591,6 +591,7 @@ func hashAccessToken(accessToken string) string { return base64.RawURLEncoding.EncodeToString(b[:len(b)/2]) } +// TODO: de-dup me (manager test). func doSHA256(s string) string { b := sha256.Sum256([]byte(s)) return base64.RawURLEncoding.EncodeToString(b[:]) @@ -856,6 +857,7 @@ func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKe requireTimeInDelta(t, goodAuthTime, authTime, timeComparisonFudgeSeconds*time.Second) } +// TODO: de-dup me (manager test). func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { return &staticKeySet{publicKey} } From 0bb2b10b3b39078aebee0ede12b0dd7be780121d Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Thu, 3 Dec 2020 17:16:08 -0800 Subject: [PATCH 10/20] Passing signing key through to the token endpoint --- .../dynamic_open_id_connect_ecdsa_strategy.go | 18 +++++++++--- ...mic_open_id_connect_ecdsa_strategy_test.go | 28 ++++++++++++++----- internal/oidc/oidc.go | 2 +- internal/oidc/provider/manager/manager.go | 2 +- .../oidc/provider/manager/manager_test.go | 14 +++++----- 5 files changed, 44 insertions(+), 20 deletions(-) diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go index 6d7d837f..00261379 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go @@ -5,6 +5,9 @@ package oidc import ( "context" + "crypto/ecdsa" + + "go.pinniped.dev/internal/constable" "github.com/ory/fosite" "github.com/ory/fosite/compose" @@ -15,7 +18,6 @@ import ( // TODO: doc me. type dynamicOpenIDConnectECDSAStrategy struct { - issuer string fositeConfig *compose.Config jwksProvider jwks.DynamicJWKSProvider } @@ -23,12 +25,10 @@ type dynamicOpenIDConnectECDSAStrategy struct { var _ openid.OpenIDConnectTokenStrategy = &dynamicOpenIDConnectECDSAStrategy{} func newDynamicOpenIDConnectECDSAStrategy( - issuer string, fositeConfig *compose.Config, jwksProvider jwks.DynamicJWKSProvider, ) *dynamicOpenIDConnectECDSAStrategy { return &dynamicOpenIDConnectECDSAStrategy{ - issuer: issuer, fositeConfig: fositeConfig, jwksProvider: jwksProvider, } @@ -38,5 +38,15 @@ func (s *dynamicOpenIDConnectECDSAStrategy) GenerateIDToken( ctx context.Context, requester fosite.Requester, ) (string, error) { - return "", nil + _, activeJwk := s.jwksProvider.GetJWKS(s.fositeConfig.IDTokenIssuer) + if activeJwk == nil { + return "", constable.Error("No JWK found for issuer") + } + key, ok := activeJwk.Key.(*ecdsa.PrivateKey) + if !ok { + return "", constable.Error("JWK must be of type ecdsa") + } + + // todo write story/issue about caching this strategy + return compose.NewOpenIDConnectECDSAStrategy(s.fositeConfig, key).GenerateIDToken(ctx, requester) } diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go index 93ba46cc..a35bfa13 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go @@ -16,15 +16,20 @@ import ( coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/compose" + "github.com/ory/fosite/handler/openid" + "github.com/ory/fosite/token/jwt" "github.com/stretchr/testify/require" - "go.pinniped.dev/internal/oidc/jwks" "gopkg.in/square/go-jose.v2" + + "go.pinniped.dev/internal/oidc/jwks" ) func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { const ( - goodIssuer = "https://some-good-issuer.com" - clientID = "some-client-id" + goodIssuer = "https://some-good-issuer.com" + clientID = "some-client-id" + goodSubject = "some-subject" + goodUsername = "some-username" ) ecPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) @@ -60,7 +65,7 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { { name: "jwks provider does not contain signing key for issuer", issuer: goodIssuer, - wantError: "some unkonwn key error", + wantError: "No JWK found for issuer", }, { name: "jwks provider contains signing key of wrong type for issuer", @@ -75,7 +80,7 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { }, ) }, - wantError: "some invalid key type error", + wantError: "JWK must be of type ecdsa", }, } for _, test := range tests { @@ -85,13 +90,22 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { if test.jwksProvider != nil { test.jwksProvider(jwksProvider) } - s := newDynamicOpenIDConnectECDSAStrategy(test.issuer, &compose.Config{}, jwksProvider) + s := newDynamicOpenIDConnectECDSAStrategy( + &compose.Config{IDTokenIssuer: test.issuer}, + jwksProvider, + ) requester := &fosite.Request{ Client: &fosite.DefaultClient{ ID: clientID, }, - // Session: fositeopenid.DefaultSession{}, + Session: &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Subject: goodSubject, + }, + Subject: goodSubject, + Username: goodUsername, + }, } idToken, err := s.GenerateIDToken(context.Background(), requester) if test.wantError != "" { diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index c5e6abf0..d38a6495 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -122,7 +122,7 @@ func FositeOauth2Helper( &compose.CommonStrategy{ // Note that Fosite requires the HMAC secret to be at least 32 bytes. CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), - OpenIDConnectTokenStrategy: newDynamicOpenIDConnectECDSAStrategy(issuer, oauthConfig, jwksProvider), + OpenIDConnectTokenStrategy: newDynamicOpenIDConnectECDSAStrategy(oauthConfig, jwksProvider), // OpenIDConnectTokenStrategy: compose.NewOpenIDConnectECDSAStrategy(oauthConfig, jwtSigningKey), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index b1d9d6b5..7e8cd6d7 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -82,7 +82,7 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil) // For all the other endpoints, make another oauth helper with exactly the same settings except use real storage. - oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, fositeHMACSecretForThisProvider, nil) + oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider) // TODO use different codecs for the state and the cookie, because: // 1. we would like to state to have an embedded expiration date while the cookie does not need that diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index d3cf1cc9..465b3c10 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -159,7 +159,7 @@ func TestManager(t *testing.T) { return actualLocationQueryParams.Get("code") } - requireTokenRequestToBeHandled := func(requestIssuer, authCode string, jwks *jose.JSONWebKeySet) { + requireTokenRequestToBeHandled := func(requestIssuer, authCode string, jwks *jose.JSONWebKeySet, jwkIssuer string) { recorder := httptest.NewRecorder() numberOfKubeActionsBeforeThisRequest := len(kubeClient.Actions()) @@ -194,7 +194,7 @@ func TestManager(t *testing.T) { keySet := newStaticKeySet(privateKey.Public()) verifyConfig := coreosoidc.Config{ClientID: downstreamClientID, SupportedSigningAlgs: []string{coreosoidc.ES256}} - verifier := coreosoidc.NewVerifier(requestIssuer, keySet, &verifyConfig) + verifier := coreosoidc.NewVerifier(jwkIssuer, keySet, &verifyConfig) _, err := verifier.Verify(context.Background(), idToken) r.NoError(err) @@ -326,16 +326,16 @@ func TestManager(t *testing.T) { downstreamAuthCode1 := requireCallbackRequestToBeHandled(issuer1, callbackRequestParams, csrfCookieValue) downstreamAuthCode2 := requireCallbackRequestToBeHandled(issuer2, callbackRequestParams, csrfCookieValue) - // // Hostnames are case-insensitive, so test that we can handle that. + // Hostnames are case-insensitive, so test that we can handle that. downstreamAuthCode3 := requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams, csrfCookieValue) downstreamAuthCode4 := requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams, csrfCookieValue) - requireTokenRequestToBeHandled(issuer1, downstreamAuthCode1, issuer1JWKS) - requireTokenRequestToBeHandled(issuer2, downstreamAuthCode2, issuer2JWKS) + requireTokenRequestToBeHandled(issuer1, downstreamAuthCode1, issuer1JWKS, issuer1) + requireTokenRequestToBeHandled(issuer2, downstreamAuthCode2, issuer2JWKS, issuer2) // Hostnames are case-insensitive, so test that we can handle that. - requireTokenRequestToBeHandled(issuer1DifferentCaseHostname, downstreamAuthCode3, issuer1JWKS) - requireTokenRequestToBeHandled(issuer2DifferentCaseHostname, downstreamAuthCode4, issuer2JWKS) + requireTokenRequestToBeHandled(issuer1DifferentCaseHostname, downstreamAuthCode3, issuer1JWKS, issuer1) + requireTokenRequestToBeHandled(issuer2DifferentCaseHostname, downstreamAuthCode4, issuer2JWKS, issuer2) } when("given some valid providers via SetProviders()", func() { From 83e0934864cae396dc1ee6ffa25c2049adf9427f Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 4 Dec 2020 09:05:39 -0500 Subject: [PATCH 11/20] Add logging in dynamic OIDC ECDSA strategy I'm worried that these errors are going to be really burried from the user, so add some log statements to try to make them a tiny bit more observable. Also follow some of our error message convetions by using lowercase error messages. Signed-off-by: Andrew Keesler --- .../dynamic_open_id_connect_ecdsa_strategy.go | 16 +++++++++++++++- ...ynamic_open_id_connect_ecdsa_strategy_test.go | 2 +- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go index 00261379..83472a0f 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go @@ -6,8 +6,10 @@ package oidc import ( "context" "crypto/ecdsa" + "reflect" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/plog" "github.com/ory/fosite" "github.com/ory/fosite/compose" @@ -40,10 +42,22 @@ func (s *dynamicOpenIDConnectECDSAStrategy) GenerateIDToken( ) (string, error) { _, activeJwk := s.jwksProvider.GetJWKS(s.fositeConfig.IDTokenIssuer) if activeJwk == nil { - return "", constable.Error("No JWK found for issuer") + plog.Debug("no JWK found for issuer", "issuer", s.fositeConfig.IDTokenIssuer) + return "", constable.Error("no JWK found for issuer") } key, ok := activeJwk.Key.(*ecdsa.PrivateKey) if !ok { + actualType := "nil" + if t := reflect.TypeOf(activeJwk.Key); t != nil { + actualType = t.String() + } + plog.Debug( + "JWK must be of type ecdsa", + "issuer", + s.fositeConfig.IDTokenIssuer, + "actualType", + actualType, + ) return "", constable.Error("JWK must be of type ecdsa") } diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go index a35bfa13..38cb35de 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go @@ -65,7 +65,7 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { { name: "jwks provider does not contain signing key for issuer", issuer: goodIssuer, - wantError: "No JWK found for issuer", + wantError: "no JWK found for issuer", }, { name: "jwks provider contains signing key of wrong type for issuer", From 03806629b80038cb7cbbf82c6404dc1bf347081c Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 4 Dec 2020 10:06:55 -0500 Subject: [PATCH 12/20] Cleanup code via TODOs accumulated during token endpoint work We opened https://github.com/vmware-tanzu/pinniped/issues/254 for the TODO in dynamicOpenIDConnectECDSAStrategy.GenerateToken(). This commit also ensures that linting and unit tests are passing again. Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler.go | 16 +-- internal/oidc/auth/auth_handler_test.go | 26 +--- .../oidc/callback/callback_handler_test.go | 4 +- .../dynamic_open_id_connect_ecdsa_strategy.go | 10 +- ...mic_open_id_connect_ecdsa_strategy_test.go | 43 ++----- internal/oidc/oidc.go | 21 ++++ internal/oidc/oidctestutil/oidc.go | 45 +++++++ .../oidc/provider/manager/manager_test.go | 36 +----- internal/oidc/token/token_handler.go | 18 +-- internal/oidc/token/token_handler_test.go | 117 +++++------------- internal/testutil/assertions.go | 17 +++ internal/testutil/crypto.go | 15 +++ 12 files changed, 168 insertions(+), 200 deletions(-) create mode 100644 internal/testutil/crypto.go diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 231cf8ee..966bfb54 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -45,7 +45,7 @@ func NewHandler( authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r) if err != nil { - plog.Info("authorize request error", fositeErrorForLog(err)...) + plog.Info("authorize request error", oidc.FositeErrorForLog(err)...) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil } @@ -69,7 +69,7 @@ func NewHandler( }, }) if err != nil { - plog.Info("authorize response error", fositeErrorForLog(err)...) + plog.Info("authorize response error", oidc.FositeErrorForLog(err)...) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil } @@ -232,15 +232,3 @@ func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken return nil } - -func fositeErrorForLog(err error) []interface{} { - rfc6749Error := fosite.ErrorToRFC6749Error(err) - keysAndValues := make([]interface{}, 0) - keysAndValues = append(keysAndValues, "name") - keysAndValues = append(keysAndValues, rfc6749Error.Name) - keysAndValues = append(keysAndValues, "status") - keysAndValues = append(keysAndValues, rfc6749Error.Status()) - keysAndValues = append(keysAndValues, "description") - keysAndValues = append(keysAndValues, rfc6749Error.Description) - return keysAndValues -} diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 79f7b7eb..30a8dc5d 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -4,10 +4,8 @@ package auth import ( - "crypto/ecdsa" "fmt" "html" - "mime" "net/http" "net/http/httptest" "net/url" @@ -21,8 +19,10 @@ import ( "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/csrftoken" + "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/oidctestutil" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" ) @@ -125,9 +125,9 @@ func TestAuthorizationEndpoint(t *testing.T) { // Configure fosite the same way that the production code would, using NullStorage to turn off storage. oauthStore := oidc.NullStorage{} hmacSecret := []byte("some secret - must have at least 32 bytes") - var signingKeyIsUnused *ecdsa.PrivateKey require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, signingKeyIsUnused) + jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused) happyCSRF := "test-csrf" happyPKCE := "test-pkce" @@ -725,7 +725,7 @@ func TestAuthorizationEndpoint(t *testing.T) { t.Logf("response body: %q", rsp.Body.String()) require.Equal(t, test.wantStatus, rsp.Code) - requireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) + testutil.RequireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) actualLocation := rsp.Header().Get("Location") if test.wantLocationHeader != "" { @@ -826,22 +826,6 @@ func (*errorReturningEncoder) Encode(_ string, _ interface{}) (string, error) { return "", fmt.Errorf("some encoding error") } -func requireEqualContentType(t *testing.T, actual string, expected string) { - t.Helper() - - if expected == "" { - require.Empty(t, actual) - return - } - - actualContentType, actualContentTypeParams, err := mime.ParseMediaType(expected) - require.NoError(t, err) - expectedContentType, expectedContentTypeParams, err := mime.ParseMediaType(expected) - require.NoError(t, err) - require.Equal(t, actualContentType, expectedContentType) - require.Equal(t, actualContentTypeParams, expectedContentTypeParams) -} - func requireEqualDecodedStateParams(t *testing.T, actualURL string, expectedURL string, stateParamDecoder oidc.Codec) { t.Helper() actualLocationURL, err := url.Parse(actualURL) diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index ead11693..d73fb097 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -24,6 +24,7 @@ import ( kubetesting "k8s.io/client-go/testing" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/oidctestutil" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" @@ -433,7 +434,8 @@ func TestCallbackEndpoint(t *testing.T) { oauthStore := oidc.NewKubeStorage(secrets) hmacSecret := []byte("some secret - must have at least 32 bytes") require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret) + jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused) idpListGetter := oidctestutil.NewIDPListGetter(&test.idp) subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI) diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go index 83472a0f..582b9cc6 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go @@ -18,7 +18,14 @@ import ( "go.pinniped.dev/internal/oidc/jwks" ) -// TODO: doc me. +// dynamicOpenIDConnectECDSAStrategy is an openid.OpenIDConnectTokenStrategy that can dynamically +// load a signing key to issue ID tokens. We want this dynamic capability since our controllers for +// loading OIDCProvider's and signing keys run in parallel, and thus the signing key might not be +// ready when an OIDCProvider is otherwise ready. +// +// If we ever update OIDCProvider's to hold their signing key, we might not need this type, since we +// could have an invariant that routes to an OIDCProvider's endpoints are only wired up if an +// OIDCProvider has a valid signing key. type dynamicOpenIDConnectECDSAStrategy struct { fositeConfig *compose.Config jwksProvider jwks.DynamicJWKSProvider @@ -61,6 +68,5 @@ func (s *dynamicOpenIDConnectECDSAStrategy) GenerateIDToken( return "", constable.Error("JWK must be of type ecdsa") } - // todo write story/issue about caching this strategy return compose.NewOpenIDConnectECDSAStrategy(s.fositeConfig, key).GenerateIDToken(ctx, requester) } diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go index 38cb35de..1b48f471 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go @@ -5,15 +5,13 @@ package oidc import ( "context" - "crypto" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" "crypto/rsa" - "fmt" + "net/url" "testing" - coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/compose" "github.com/ory/fosite/handler/openid" @@ -22,6 +20,7 @@ import ( "gopkg.in/square/go-jose.v2" "go.pinniped.dev/internal/oidc/jwks" + "go.pinniped.dev/internal/oidc/oidctestutil" ) func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { @@ -30,6 +29,7 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { clientID = "some-client-id" goodSubject = "some-subject" goodUsername = "some-username" + goodNonce = "some-nonce-that-is-at-least-32-characters-to-meet-entropy-requirements" ) ecPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) @@ -106,6 +106,9 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { Subject: goodSubject, Username: goodUsername, }, + Form: url.Values{ + "nonce": {goodNonce}, + }, } idToken, err := s.GenerateIDToken(context.Background(), requester) if test.wantError != "" { @@ -113,38 +116,16 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { } else { require.NoError(t, err) - // TODO: common-ize this code with token endpoint test. - // TODO: make more assertions about ID token - privateKey, ok := test.wantSigningJWK.Key.(*ecdsa.PrivateKey) require.True(t, ok, "wanted private key to be *ecdsa.PrivateKey, but was %T", test.wantSigningJWK) - keySet := newStaticKeySet(privateKey.Public()) - verifyConfig := coreosoidc.Config{ - ClientID: clientID, - SupportedSigningAlgs: []string{coreosoidc.ES256}, - } - verifier := coreosoidc.NewVerifier(test.issuer, keySet, &verifyConfig) - _, err := verifier.Verify(context.Background(), idToken) - require.NoError(t, err) + // Perform a light validation on the token to make sure 1) we passed through the correct + // signing key and 2) we forwarded the fosite.Requester correctly. Token generation is + // tested more expansively in the token endpoint. + token := oidctestutil.VerifyECDSAIDToken(t, goodIssuer, clientID, privateKey, idToken) + require.Equal(t, goodSubject, token.Subject) + require.Equal(t, goodNonce, token.Nonce) } }) } } - -// TODO: de-dep me. -func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { - return &staticKeySet{publicKey} -} - -type staticKeySet struct { - publicKey crypto.PublicKey -} - -func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { - jws, err := jose.ParseSigned(jwt) - if err != nil { - return nil, fmt.Errorf("oidc: malformed jwt: %v", err) - } - return jws.Verify(s.publicKey) -} diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index d38a6495..7865def8 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -134,6 +134,27 @@ func FositeOauth2Helper( ) } +// FositeErrorForLog generates a list of information about the provided Fosite error that can be +// passed to a plog function (e.g., plog.Info()). +// +// Sample usage: +// err := someFositeLibraryFunction() +// if err != nil { +// plog.Info("some error", FositeErrorForLog(err)...) +// ... +// } +func FositeErrorForLog(err error) []interface{} { + rfc6749Error := fosite.ErrorToRFC6749Error(err) + keysAndValues := make([]interface{}, 0) + keysAndValues = append(keysAndValues, "name") + keysAndValues = append(keysAndValues, rfc6749Error.Name) + keysAndValues = append(keysAndValues, "status") + keysAndValues = append(keysAndValues, rfc6749Error.Status()) + keysAndValues = append(keysAndValues, "description") + keysAndValues = append(keysAndValues, rfc6749Error.Description) + return keysAndValues +} + type IDPListGetter interface { GetIDPList() []provider.UpstreamOIDCIdentityProviderI } diff --git a/internal/oidc/oidctestutil/oidc.go b/internal/oidc/oidctestutil/oidc.go index 5b214e5c..19aa7c07 100644 --- a/internal/oidc/oidctestutil/oidc.go +++ b/internal/oidc/oidctestutil/oidc.go @@ -5,9 +5,16 @@ package oidctestutil import ( "context" + "crypto" + "crypto/ecdsa" + "fmt" "net/url" + "testing" + coreosoidc "github.com/coreos/go-oidc" + "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "gopkg.in/square/go-jose.v2" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/pkg/oidcclient/nonce" @@ -127,3 +134,41 @@ type ExpectedUpstreamStateParamFormat struct { K string `json:"k"` V string `json:"v"` } + +type staticKeySet struct { + publicKey crypto.PublicKey +} + +func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { + return &staticKeySet{publicKey} +} + +func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { + jws, err := jose.ParseSigned(jwt) + if err != nil { + return nil, fmt.Errorf("oidc: malformed jwt: %w", err) + } + return jws.Verify(s.publicKey) +} + +// VerifyECDSAIDToken verifies that the provided idToken was issued via the provided jwtSigningKey. +// It also performs some light validation on the claims, i.e., it makes sure the provided idToken +// has the provided issuer and clientID. +// +// Further validation can be done via callers via the returned coreosoidc.IDToken. +func VerifyECDSAIDToken( + t *testing.T, + issuer, clientID string, + jwtSigningKey *ecdsa.PrivateKey, + idToken string, +) *coreosoidc.IDToken { + t.Helper() + + keySet := newStaticKeySet(jwtSigningKey.Public()) + verifyConfig := coreosoidc.Config{ClientID: clientID, SupportedSigningAlgs: []string{coreosoidc.ES256}} + verifier := coreosoidc.NewVerifier(issuer, keySet, &verifyConfig) + token, err := verifier.Verify(context.Background(), idToken) + require.NoError(t, err) + + return token +} diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 465b3c10..bf017cfe 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -5,12 +5,8 @@ package manager import ( "context" - "crypto" "crypto/ecdsa" - "crypto/sha256" - "encoding/base64" "encoding/json" - "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -18,7 +14,6 @@ import ( "strings" "testing" - coreosoidc "github.com/coreos/go-oidc" "github.com/sclevine/spec" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" @@ -30,6 +25,7 @@ import ( "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/oidctestutil" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/oidctypes" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -184,7 +180,6 @@ func TestManager(t *testing.T) { // Validate ID token is signed by the correct JWK to make sure we wired the token endpoint // signing key correctly. - // TODO: common-ize this code with token endpoint test. idToken, ok := body["id_token"].(string) r.True(ok, "wanted id_token type to be string, but was %T", body["id_token"]) @@ -192,11 +187,7 @@ func TestManager(t *testing.T) { privateKey, ok := jwks.Keys[0].Key.(*ecdsa.PrivateKey) r.True(ok, "wanted private key to be *ecdsa.PrivateKey, but was %T", jwks.Keys[0].Key) - keySet := newStaticKeySet(privateKey.Public()) - verifyConfig := coreosoidc.Config{ClientID: downstreamClientID, SupportedSigningAlgs: []string{coreosoidc.ES256}} - verifier := coreosoidc.NewVerifier(jwkIssuer, keySet, &verifyConfig) - _, err := verifier.Verify(context.Background(), idToken) - r.NoError(err) + oidctestutil.VerifyECDSAIDToken(t, jwkIssuer, downstreamClientID, privateKey, idToken) // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+7, @@ -305,7 +296,7 @@ func TestManager(t *testing.T) { "client_id": []string{downstreamClientID}, "state": []string{"some-state-value-that-is-32-byte"}, "nonce": []string{"some-nonce-value-that-is-at-least-32-bytes"}, - "code_challenge": []string{doSHA256(downstreamPKCECodeVerifier)}, + "code_challenge": []string{testutil.SHA256(downstreamPKCECodeVerifier)}, "code_challenge_method": []string{"S256"}, "redirect_uri": []string{downstreamRedirectURL}, }.Encode() @@ -406,24 +397,3 @@ func TestManager(t *testing.T) { }) }) } - -func doSHA256(s string) string { - b := sha256.Sum256([]byte(s)) - return base64.RawURLEncoding.EncodeToString(b[:]) -} - -func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { - return &staticKeySet{publicKey} -} - -type staticKeySet struct { - publicKey crypto.PublicKey -} - -func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { - jws, err := jose.ParseSigned(jwt) - if err != nil { - return nil, fmt.Errorf("oidc: malformed jwt: %v", err) - } - return jws.Verify(s.publicKey) -} diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index b8747636..c481168b 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -11,6 +11,7 @@ import ( "github.com/ory/fosite/handler/openid" "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/plog" ) @@ -21,14 +22,14 @@ func NewHandler( var session openid.DefaultSession accessRequest, err := oauthHelper.NewAccessRequest(r.Context(), r, &session) if err != nil { - plog.Info("token request error", fositeErrorForLog(err)...) + plog.Info("token request error", oidc.FositeErrorForLog(err)...) oauthHelper.WriteAccessError(w, accessRequest, err) return nil } accessResponse, err := oauthHelper.NewAccessResponse(r.Context(), accessRequest) if err != nil { - plog.Info("token response error", fositeErrorForLog(err)...) + plog.Info("token response error", oidc.FositeErrorForLog(err)...) oauthHelper.WriteAccessError(w, accessRequest, err) return nil } @@ -38,16 +39,3 @@ func NewHandler( return nil }) } - -// TODO: de-dup me. -func fositeErrorForLog(err error) []interface{} { - rfc6749Error := fosite.ErrorToRFC6749Error(err) - keysAndValues := make([]interface{}, 0) - keysAndValues = append(keysAndValues, "name") - keysAndValues = append(keysAndValues, rfc6749Error.Name) - keysAndValues = append(keysAndValues, "status") - keysAndValues = append(keysAndValues, rfc6749Error.Status()) - keysAndValues = append(keysAndValues, "description") - keysAndValues = append(keysAndValues, rfc6749Error.Description) - return keysAndValues -} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index ea64cc1b..9e8ccd11 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -5,17 +5,14 @@ package token import ( "context" - "crypto" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" "crypto/sha256" "encoding/base64" "encoding/json" - "fmt" "io" "io/ioutil" - "mime" "net/http" "net/http/httptest" "net/url" @@ -23,7 +20,6 @@ import ( "testing" "time" - coreosoidc "github.com/coreos/go-oidc" "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" @@ -35,6 +31,9 @@ import ( "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/jwks" + "go.pinniped.dev/internal/oidc/oidctestutil" + "go.pinniped.dev/internal/testutil" ) const ( @@ -188,7 +187,7 @@ func TestTokenEndpoint(t *testing.T) { "client_id": {goodClient}, "state": {"some-state-value-that-is-32-byte"}, "nonce": {goodNonce}, - "code_challenge": {doSHA256(goodPKCECodeVerifier)}, + "code_challenge": {testutil.SHA256(goodPKCECodeVerifier)}, "code_challenge_method": {"S256"}, "redirect_uri": {goodRedirectURI}, }, @@ -406,7 +405,7 @@ func TestTokenEndpoint(t *testing.T) { t.Logf("response body: %q", rsp.Body.String()) require.Equal(t, test.wantStatus, rsp.Code) - requireEqualContentType(t, rsp.Header().Get("Content-Type"), "application/json") + testutil.RequireEqualContentType(t, rsp.Header().Get("Content-Type"), "application/json") if test.wantBodyFields != nil { var m map[string]interface{} require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &m)) @@ -444,7 +443,7 @@ func TestTokenEndpoint(t *testing.T) { subject.ServeHTTP(rsp0, req) t.Logf("response 0: %#v", rsp0) t.Logf("response 0 body: %q", rsp0.Body.String()) - requireEqualContentType(t, rsp0.Header().Get("Content-Type"), "application/json") + testutil.RequireEqualContentType(t, rsp0.Header().Get("Content-Type"), "application/json") require.Equal(t, http.StatusOK, rsp0.Code) var m map[string]interface{} @@ -470,7 +469,7 @@ func TestTokenEndpoint(t *testing.T) { t.Logf("response 1: %#v", rsp1) t.Logf("response 1 body: %q", rsp1.Body.String()) require.Equal(t, http.StatusBadRequest, rsp1.Code) - requireEqualContentType(t, rsp1.Header().Get("Content-Type"), "application/json") + testutil.RequireEqualContentType(t, rsp1.Header().Get("Content-Type"), "application/json") require.JSONEq(t, fositeReusedAuthCodeErrorBody, rsp1.Body.String()) requireInvalidAuthCodeStorage(t, code, oauthStore) @@ -547,8 +546,8 @@ func makeHappyOauthHelper( ) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) { t.Helper() - jwtSigningKey := generateJWTSigningKey(t) - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwtSigningKey) + jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider) // Simulate the auth endpoint running so Fosite code will fill the store with realistic values. // @@ -574,11 +573,21 @@ func makeHappyOauthHelper( return oauthHelper, authResponder.GetCode(), jwtSigningKey } -func generateJWTSigningKey(t *testing.T) *ecdsa.PrivateKey { +func generateJWTSigningKeyAndJWKSProvider(t *testing.T, issuer string) (*ecdsa.PrivateKey, jwks.DynamicJWKSProvider) { t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) - return key + + jwksProvider := jwks.NewDynamicJWKSProvider() + jwksProvider.SetIssuerToJWKSMap( + nil, // public JWKS unused + map[string]*jose.JSONWebKey{ + issuer: {Key: key}, + }, + ) + + return key, jwksProvider } func hashAccessToken(accessToken string) string { @@ -591,12 +600,6 @@ func hashAccessToken(accessToken string) string { return base64.RawURLEncoding.EncodeToString(b[:len(b)/2]) } -// TODO: de-dup me (manager test). -func doSHA256(s string) string { - b := sha256.Sum256([]byte(s)) - return base64.RawURLEncoding.EncodeToString(b[:]) -} - func requireInvalidAuthCodeStorage( t *testing.T, code string, @@ -736,7 +739,7 @@ func requireValidAuthRequest( wantGrantedScopes = append([]string{"openid"}, wantGrantedScopes...) } require.NotEmpty(t, authRequest.GetID()) - requireTimeInDelta(t, authRequest.GetRequestedAt(), time.Now().UTC(), timeComparisonFudgeSeconds*time.Second) + testutil.RequireTimeInDelta(t, authRequest.GetRequestedAt(), time.Now().UTC(), timeComparisonFudgeSeconds*time.Second) require.Equal(t, goodClient, authRequest.GetClient().GetID()) require.Equal(t, fosite.Arguments(wantRequestedScopes), authRequest.GetRequestedScopes()) require.Equal(t, fosite.Arguments(wantGrantedScopes), authRequest.GetGrantedScopes()) @@ -756,13 +759,13 @@ func requireValidAuthRequest( require.Equal(t, goodSubject, claims.Subject) require.Equal(t, []string{goodClient}, claims.Audience) require.Equal(t, goodNonce, claims.Nonce) - requireTimeInDelta( + testutil.RequireTimeInDelta( t, time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), claims.ExpiresAt, timeComparisonFudgeSeconds*time.Second, ) - requireTimeInDelta(t, time.Now().UTC(), claims.IssuedAt, timeComparisonFudgeSeconds*time.Second) + testutil.RequireTimeInDelta(t, time.Now().UTC(), claims.IssuedAt, timeComparisonFudgeSeconds*time.Second) require.Equal(t, wantAccessTokenHash, claims.AccessTokenHash) // We are in charge of setting these fields. For the purpose of testing, we ensure that the @@ -784,7 +787,7 @@ func requireValidAuthRequest( // Assert that the token expirations are what we think they should be. authCodeExpiresAt, ok := session.ExpiresAt[fosite.AuthorizeCode] require.True(t, ok, "expected session to hold expiration time for auth code") - requireTimeInDelta( + testutil.RequireTimeInDelta( t, time.Now().UTC().Add(authCodeExpirationSeconds*time.Second), authCodeExpiresAt, @@ -792,7 +795,7 @@ func requireValidAuthRequest( ) accessTokenExpiresAt, ok := session.ExpiresAt[fosite.AccessToken] require.True(t, ok, "expected session to hold expiration time for access token") - requireTimeInDelta( + testutil.RequireTimeInDelta( t, time.Now().UTC().Add(accessTokenExpirationSeconds*time.Second), accessTokenExpiresAt, @@ -805,17 +808,15 @@ func requireValidAuthRequest( } func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKey *ecdsa.PrivateKey) { + t.Helper() + idToken, ok := body["id_token"] require.Truef(t, ok, "body did not contain 'id_token': %s", body) idTokenString, ok := idToken.(string) require.Truef(t, ok, "wanted id_token to be a string, but got %T", idToken) // The go-oidc library will validate the signature and the client claim in the ID token. - keySet := newStaticKeySet(jwtSigningKey.Public()) - verifyConfig := coreosoidc.Config{ClientID: goodClient, SupportedSigningAlgs: []string{coreosoidc.ES256}} - verifier := coreosoidc.NewVerifier(goodIssuer, keySet, &verifyConfig) - token, err := verifier.Verify(context.Background(), idTokenString) - require.NoError(t, err) + token := oidctestutil.VerifyECDSAIDToken(t, goodIssuer, goodClient, jwtSigningKey, idTokenString) var claims struct { Subject string `json:"sub"` @@ -837,7 +838,7 @@ func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKe require.ElementsMatch(t, idTokenFields, getMapKeys(m)) // verify each of the claims - err = token.Claims(&claims) + err := token.Claims(&claims) require.NoError(t, err) require.Equal(t, goodSubject, claims.Subject) require.Len(t, claims.Audience, 1) @@ -851,60 +852,10 @@ func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKe issuedAt := time.Unix(claims.IssuedAt, 0) requestedAt := time.Unix(claims.RequestedAt, 0) authTime := time.Unix(claims.AuthTime, 0) - requireTimeInDelta(t, time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), expiresAt, timeComparisonFudgeSeconds*time.Second) - requireTimeInDelta(t, time.Now().UTC(), issuedAt, timeComparisonFudgeSeconds*time.Second) - requireTimeInDelta(t, goodRequestedAtTime, requestedAt, timeComparisonFudgeSeconds*time.Second) - requireTimeInDelta(t, goodAuthTime, authTime, timeComparisonFudgeSeconds*time.Second) -} - -// TODO: de-dup me (manager test). -func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { - return &staticKeySet{publicKey} -} - -type staticKeySet struct { - publicKey crypto.PublicKey -} - -func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { - jws, err := jose.ParseSigned(jwt) - if err != nil { - return nil, fmt.Errorf("oidc: malformed jwt: %v", err) - } - return jws.Verify(s.publicKey) -} - -// TODO: de-dup me. -func requireEqualContentType(t *testing.T, actual string, expected string) { - t.Helper() - - if expected == "" { - require.Empty(t, actual) - return - } - - actualContentType, actualContentTypeParams, err := mime.ParseMediaType(expected) - require.NoError(t, err) - expectedContentType, expectedContentTypeParams, err := mime.ParseMediaType(expected) - require.NoError(t, err) - require.Equal(t, actualContentType, expectedContentType) - require.Equal(t, actualContentTypeParams, expectedContentTypeParams) -} - -// TODO: use actual testutil function. -//nolint:unparam -func requireTimeInDelta(t *testing.T, t1 time.Time, t2 time.Time, delta time.Duration) { - t.Helper() - require.InDeltaf(t, - float64(t1.UnixNano()), - float64(t2.UnixNano()), - float64(delta.Nanoseconds()), - "expected %s and %s to be < %s apart, but they are %s apart", - t1.Format(time.RFC3339Nano), - t2.Format(time.RFC3339Nano), - delta.String(), - t1.Sub(t2).String(), - ) + testutil.RequireTimeInDelta(t, time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), expiresAt, timeComparisonFudgeSeconds*time.Second) + testutil.RequireTimeInDelta(t, time.Now().UTC(), issuedAt, timeComparisonFudgeSeconds*time.Second) + testutil.RequireTimeInDelta(t, goodRequestedAtTime, requestedAt, timeComparisonFudgeSeconds*time.Second) + testutil.RequireTimeInDelta(t, goodAuthTime, authTime, timeComparisonFudgeSeconds*time.Second) } func deepCopyRequestForm(r *http.Request) *http.Request { diff --git a/internal/testutil/assertions.go b/internal/testutil/assertions.go index 77247602..e49233d5 100644 --- a/internal/testutil/assertions.go +++ b/internal/testutil/assertions.go @@ -4,6 +4,7 @@ package testutil import ( + "mime" "testing" "time" @@ -22,3 +23,19 @@ func RequireTimeInDelta(t *testing.T, t1 time.Time, t2 time.Time, delta time.Dur t1.Sub(t2).String(), ) } + +func RequireEqualContentType(t *testing.T, actual string, expected string) { + t.Helper() + + if expected == "" { + require.Empty(t, actual) + return + } + + actualContentType, actualContentTypeParams, err := mime.ParseMediaType(expected) + require.NoError(t, err) + expectedContentType, expectedContentTypeParams, err := mime.ParseMediaType(expected) + require.NoError(t, err) + require.Equal(t, actualContentType, expectedContentType) + require.Equal(t, actualContentTypeParams, expectedContentTypeParams) +} diff --git a/internal/testutil/crypto.go b/internal/testutil/crypto.go new file mode 100644 index 00000000..fdc83e84 --- /dev/null +++ b/internal/testutil/crypto.go @@ -0,0 +1,15 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "crypto/sha256" + "encoding/base64" +) + +// SHA256 returns the base64 URL encoding of the SHA256 sum of the provided string. +func SHA256(s string) string { + b := sha256.Sum256([]byte(s)) + return base64.RawURLEncoding.EncodeToString(b[:]) +} From 37631b41ea605e4a43e855288a2cc6f78e312996 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 4 Dec 2020 10:18:45 -0500 Subject: [PATCH 13/20] Don't set our TokenURL - we don't need it right now TokenURL is used by Fosite to validate clients authenticating with the private_key_jwt method. We don't have any use for this right now, so just leave this blank until we need it. See when Ryan brought this up in https://github.com/vmware-tanzu/pinniped/pull/239#discussion_r528022162. Signed-off-by: Andrew Keesler --- internal/oidc/oidc.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 7865def8..fe50985c 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -105,7 +105,6 @@ func FositeOauth2Helper( RefreshTokenLifespan: 16 * time.Hour, // long enough for a single workday IDTokenIssuer: issuer, - TokenURL: "", // TODO set once we have this endpoint written ScopeStrategy: fosite.ExactScopeStrategy, // be careful and only support exact string matching for scopes AudienceMatchingStrategy: nil, // I believe the default is fine From 8d5f4a93ed35266bf9665c7cc88cffb958162cd8 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 4 Dec 2020 11:16:32 -0500 Subject: [PATCH 14/20] Get rid of an unnecessary comment from 58237d0e7de Signed-off-by: Andrew Keesler --- internal/oidc/oidc.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index fe50985c..7c550751 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -122,7 +122,6 @@ func FositeOauth2Helper( // Note that Fosite requires the HMAC secret to be at least 32 bytes. CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), OpenIDConnectTokenStrategy: newDynamicOpenIDConnectECDSAStrategy(oauthConfig, jwksProvider), - // OpenIDConnectTokenStrategy: compose.NewOpenIDConnectECDSAStrategy(oauthConfig, jwtSigningKey), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. compose.OAuth2AuthorizeExplicitFactory, From ac83633888353b0d988a9f6ce79bfc97f70e6cba Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 4 Dec 2020 14:31:06 -0800 Subject: [PATCH 15/20] Add fosite kube storage for access and refresh tokens Also switched the token_handler_test.go to use kube storage. Signed-off-by: Aram Price --- internal/crud/crud.go | 46 ++- internal/crud/crud_test.go | 274 ++++++++++++++++- .../fositestorage/accesstoken/accesstoken.go | 112 +++++++ .../accesstoken/accesstoken_test.go | 282 ++++++++++++++++++ .../authorizationcode/authorizationcode.go | 2 +- internal/fositestorage/fositestorage.go | 7 +- .../openidconnect/openidconnect.go | 2 +- internal/fositestorage/pkce/pkce.go | 2 +- .../refreshtoken/refreshtoken.go | 112 +++++++ .../refreshtoken/refreshtoken_test.go | 282 ++++++++++++++++++ internal/oidc/kube_storage.go | 38 ++- .../oidc/provider/manager/manager_test.go | 2 +- internal/oidc/token/token_handler_test.go | 113 ++++--- 13 files changed, 1192 insertions(+), 82 deletions(-) create mode 100644 internal/fositestorage/accesstoken/accesstoken.go create mode 100644 internal/fositestorage/accesstoken/accesstoken_test.go create mode 100644 internal/fositestorage/refreshtoken/refreshtoken.go create mode 100644 internal/fositestorage/refreshtoken/refreshtoken_test.go diff --git a/internal/crud/crud.go b/internal/crud/crud.go index 9e15581d..d5386e41 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -14,6 +14,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "go.pinniped.dev/internal/constable" @@ -34,10 +35,11 @@ const ( ) type Storage interface { - Create(ctx context.Context, signature string, data JSON) (resourceVersion string, err error) + Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (resourceVersion string, err error) Get(ctx context.Context, signature string, data JSON) (resourceVersion string, err error) Update(ctx context.Context, signature, resourceVersion string, data JSON) (newResourceVersion string, err error) Delete(ctx context.Context, signature string) error + DeleteByLabel(ctx context.Context, labelName string, labelValue string) error } type JSON interface{} // document that we need valid JSON types @@ -58,8 +60,8 @@ type secretsStorage struct { secrets corev1client.SecretInterface } -func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON) (string, error) { - secret, err := s.toSecret(signature, "", data) +func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (string, error) { + secret, err := s.toSecret(signature, "", data, additionalLabels) if err != nil { return "", err } @@ -98,7 +100,7 @@ func (s *secretsStorage) validateSecret(secret *corev1.Secret) error { } func (s *secretsStorage) Update(ctx context.Context, signature, resourceVersion string, data JSON) (string, error) { - secret, err := s.toSecret(signature, resourceVersion, data) + secret, err := s.toSecret(signature, resourceVersion, data, nil) if err != nil { return "", err } @@ -116,6 +118,28 @@ func (s *secretsStorage) Delete(ctx context.Context, signature string) error { return nil } +func (s *secretsStorage) DeleteByLabel(ctx context.Context, labelName string, labelValue string) error { + list, err := s.secrets.List(ctx, metav1.ListOptions{ + LabelSelector: labels.Set{ + secretLabelKey: s.resource, + labelName: labelValue, + }.String(), + }) + if err != nil { + //nolint:err113 // there's nothing wrong with this error + return fmt.Errorf(`failed to list secrets for resource "%s" matching label "%s=%s": %w`, s.resource, labelName, labelValue, err) + } + // TODO try to delete all of the items and consolidate all of the errors and return them all + for _, secret := range list.Items { + err = s.secrets.Delete(ctx, secret.Name, metav1.DeleteOptions{}) + if err != nil { + //nolint:err113 // there's nothing wrong with this error + return fmt.Errorf(`failed to delete secrets for resource "%s" matching label "%s=%s" with name %s: %w`, s.resource, labelName, labelValue, secret.Name, err) + } + } + return nil +} + //nolint: gochecknoglobals var b32 = base32.StdEncoding.WithPadding(base32.NoPadding) @@ -127,18 +151,24 @@ func (s *secretsStorage) getName(signature string) string { return fmt.Sprintf(secretNameFormat, s.resource, signatureAsValidName) } -func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON) (*corev1.Secret, error) { +func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, additionalLabels map[string]string) (*corev1.Secret, error) { buf, err := json.Marshal(data) if err != nil { return nil, fmt.Errorf("failed to encode secret data for %s: %w", s.getName(signature), err) } + + labels := map[string]string{ + secretLabelKey: s.resource, // make it easier to find this stuff via kubectl + } + for labelName, labelValue := range additionalLabels { + labels[labelName] = labelValue + } + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: s.getName(signature), ResourceVersion: resourceVersion, - Labels: map[string]string{ - secretLabelKey: s.resource, // make it easier to find this stuff via kubectl - }, + Labels: labels, OwnerReferences: nil, }, Data: map[string][]byte{ diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index cb0fc147..5ff4aedc 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -6,6 +6,7 @@ package crud import ( "context" "errors" + "fmt" "testing" "github.com/ory/fosite/compose" @@ -87,6 +88,7 @@ func TestStorage(t *testing.T) { wantSecrets: nil, wantErr: `failed to delete tokens for signature not-a-token: secrets "pinniped-storage-tokens-t2fx427lnci6s" not found`, }, + // TODO make a delete non-existent test for DeleteByLabel { name: "create and get", resource: "access-tokens", @@ -97,7 +99,7 @@ func TestStorage(t *testing.T) { require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is data := &testJSON{Data: "create-and-get"} - rv1, err := storage.Create(ctx, signature, data) + rv1, err := storage.Create(ctx, signature, data, nil) require.Empty(t, rv1) // fake client does not set this require.NoError(t, err) @@ -145,6 +147,68 @@ func TestStorage(t *testing.T) { }, wantErr: "", }, + { + name: "create and get with additional labels", + resource: "access-tokens", + mocks: nil, + run: func(t *testing.T, storage Storage) error { + signature := hmac.AuthorizeCodeSignature(authorizationCode1) + require.NotEmpty(t, signature) + require.NotEmpty(t, validateSecretName(signature, false)) // signature is not valid secret name as-is + + data := &testJSON{Data: "create-and-get"} + rv1, err := storage.Create(ctx, signature, data, map[string]string{"label1": "value1", "label2": "value2"}) + require.Empty(t, rv1) // fake client does not set this + require.NoError(t, err) + + out := &testJSON{} + rv2, err := storage.Get(ctx, signature, out) + require.Empty(t, rv2) // fake client does not set this + require.NoError(t, err) + require.Equal(t, data, out) + + return nil + }, + wantActions: []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "access-tokens", + "label1": "value1", + "label2": "value2", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }), + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq"), + }, + wantSecrets: []corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "access-tokens", + "label1": "value1", + "label2": "value2", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"create-and-get"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-tokens", + }, + }, + wantErr: "", + }, { name: "get existing", resource: "pandas-are-best", @@ -325,6 +389,207 @@ func TestStorage(t *testing.T) { wantSecrets: nil, wantErr: "", }, + { + name: "delete existing by label", + resource: "seals", + mocks: func(t *testing.T, mock mocker) { + require.NoError(t, mock.Tracker().Add(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "seals", + "additionalLabel": "matching-value", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"sad-seal"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/seals", + })) + require.NoError(t, mock.Tracker().Add(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-abcdywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "seals", + "additionalLabel": "matching-value", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"happy-seal"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/seals", + })) + require.NoError(t, mock.Tracker().Add(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-12345wdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "seals", // same type as above + "additionalLabel": "non-matching-value", // different value for the same label + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"sad-seal2"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/seals", + })) + require.NoError(t, mock.Tracker().Add(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-54321wdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "walruses", // different type from above + "additionalLabel": "matching-value", // same value for the same label as above + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"sad-seal3"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/walruses", + })) + }, + run: func(t *testing.T, storage Storage) error { + return storage.DeleteByLabel(ctx, "additionalLabel", "matching-value") + }, + wantActions: []coretesting.Action{ + coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ + LabelSelector: "storage.pinniped.dev=seals,additionalLabel=matching-value", + }), + coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-seals-abcdywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq"), + coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq"), + }, + wantSecrets: []corev1.Secret{ + // the secret of the same type whose label did not match is not deleted + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-12345wdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "seals", // same type as above + "additionalLabel": "non-matching-value", // different value for the same label + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"sad-seal2"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/seals", + }, + // the secrets of other types are not deleted, even if they have a matching label + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-54321wdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "walruses", // different type from above + "additionalLabel": "matching-value", // same value for the same label as above + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"sad-seal3"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/walruses", + }, + }, + wantErr: "", + }, + { + name: "when there is an error performing the delete while deleting by label", + resource: "seals", + mocks: func(t *testing.T, mock mocker) { + require.NoError(t, mock.Tracker().Add(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "seals", + "additionalLabel": "matching-value", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"sad-seal"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/seals", + })) + mock.PrependReactor("delete", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, fmt.Errorf("some delete error") + }) + }, + run: func(t *testing.T, storage Storage) error { + return storage.DeleteByLabel(ctx, "additionalLabel", "matching-value") + }, + wantActions: []coretesting.Action{ + coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ + LabelSelector: "storage.pinniped.dev=seals,additionalLabel=matching-value", + }), + coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq"), + }, + wantSecrets: []corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: namespace, + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "seals", + "additionalLabel": "matching-value", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"sad-seal"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/seals", + }, + }, + wantErr: `failed to delete secrets for resource "seals" matching label "additionalLabel=matching-value" with name pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq: some delete error`, + }, + { + name: "when there is an error listing secrets during a delete by label operation", + resource: "seals", + mocks: func(t *testing.T, mock mocker) { + mock.PrependReactor("list", "secrets", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { + listAction := action.(coretesting.ListActionImpl) + labelRestrictions := listAction.GetListRestrictions().Labels + requiresExactMatch, found := labelRestrictions.RequiresExactMatch("additionalLabel") + if !found || requiresExactMatch != "matching-value" { + // this list action did not use label selector additionalLabel=matching-value, so allow it to proceed without intervention + return false, nil, nil + } + requiresExactMatch, found = labelRestrictions.RequiresExactMatch("storage.pinniped.dev") + if !found || requiresExactMatch != "seals" { + // this list action did not use label selector storage.pinniped.dev=seals, so allow it to proceed without intervention + return false, nil, nil + } + // this list action was the one that did use the expected label selectors so cause it to error + return true, nil, fmt.Errorf("some listing error") + }) + }, + run: func(t *testing.T, storage Storage) error { + return storage.DeleteByLabel(ctx, "additionalLabel", "matching-value") + }, + wantActions: []coretesting.Action{ + coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ + LabelSelector: "storage.pinniped.dev=seals,additionalLabel=matching-value", + }), + }, + wantErr: `failed to list secrets for resource "seals" matching label "additionalLabel=matching-value": some listing error`, + }, { name: "invalid exiting secret type", resource: "candies", @@ -582,8 +847,11 @@ func checkSecretActionNames(t *testing.T, actions []coretesting.Action) { t.Helper() for _, action := range actions { - name := getName(t, action) - assertValidName(t, name) + _, ok := action.(coretesting.ListActionImpl) + if !ok { // list action don't have names, so skip these assertions for list actions + name := getName(t, action) + assertValidName(t, name) + } } } diff --git a/internal/fositestorage/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go new file mode 100644 index 00000000..15623272 --- /dev/null +++ b/internal/fositestorage/accesstoken/accesstoken.go @@ -0,0 +1,112 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package accesstoken + +import ( + "context" + "fmt" + + "github.com/ory/fosite" + "github.com/ory/fosite/handler/oauth2" + "github.com/ory/fosite/handler/openid" + "k8s.io/apimachinery/pkg/api/errors" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + + "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/fositestorage" +) + +const ( + ErrInvalidAccessTokenRequestVersion = constable.Error("access token request data has wrong version") + ErrInvalidAccessTokenRequestData = constable.Error("access token request data must be present") + + accessTokenStorageVersion = "1" +) + +type RevocationStorage interface { + oauth2.AccessTokenStorage + RevokeAccessToken(ctx context.Context, requestID string) error +} + +var _ RevocationStorage = &accessTokenStorage{} + +type accessTokenStorage struct { + storage crud.Storage +} + +type session struct { + Request *fosite.Request `json:"request"` + Version string `json:"version"` +} + +func New(secrets corev1client.SecretInterface) RevocationStorage { + return &accessTokenStorage{storage: crud.New("access-token", secrets)} +} + +func (a *accessTokenStorage) RevokeAccessToken(ctx context.Context, requestID string) error { + return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) +} + +func (a *accessTokenStorage) CreateAccessTokenSession(ctx context.Context, signature string, requester fosite.Requester) error { + request, err := fositestorage.ValidateAndExtractAuthorizeRequest(requester) + if err != nil { + return err + } + + _, err = a.storage.Create( + ctx, + signature, + &session{Request: request, Version: accessTokenStorageVersion}, + map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, + ) + return err +} + +func (a *accessTokenStorage) GetAccessTokenSession(ctx context.Context, signature string, _ fosite.Session) (fosite.Requester, error) { + session, _, err := a.getSession(ctx, signature) + + if err != nil { + return nil, err + } + + return session.Request, err +} + +func (a *accessTokenStorage) DeleteAccessTokenSession(ctx context.Context, signature string) error { + return a.storage.Delete(ctx, signature) +} + +func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) { + session := newValidEmptyAccessTokenSession() + rv, err := a.storage.Get(ctx, signature, session) + + if errors.IsNotFound(err) { + return nil, "", fosite.ErrNotFound.WithCause(err).WithDebug(err.Error()) + } + + if err != nil { + return nil, "", fmt.Errorf("failed to get access token session for %s: %w", signature, err) + } + + if version := session.Version; version != accessTokenStorageVersion { + return nil, "", fmt.Errorf("%w: access token session for %s has version %s instead of %s", + ErrInvalidAccessTokenRequestVersion, signature, version, accessTokenStorageVersion) + } + + if session.Request.ID == "" { + return nil, "", fmt.Errorf("malformed access token session for %s: %w", signature, ErrInvalidAccessTokenRequestData) + } + + return session, rv, nil +} + +func newValidEmptyAccessTokenSession() *session { + return &session{ + Request: &fosite.Request{ + Client: &fosite.DefaultOpenIDConnectClient{}, + Session: &openid.DefaultSession{}, + }, + } +} diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go new file mode 100644 index 00000000..f03ef852 --- /dev/null +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -0,0 +1,282 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package accesstoken + +import ( + "context" + "net/url" + "testing" + "time" + + "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/kubernetes/fake" + coretesting "k8s.io/client-go/testing" +) + +const namespace = "test-ns" + +var secretsGVR = schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "secrets", +} + +func TestAccessTokenStorage(t *testing.T) { + ctx := context.Background() + + wantActions := []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "access-token", + "storage.pinniped.dev/request-id": "abcd-1", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + }), + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-access-token-pwu5zs7lekbhnln2w4"), + coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-access-token-pwu5zs7lekbhnln2w4"), + } + + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + ID: "abcd-1", + RequestedAt: time.Time{}, + Client: &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinny", + Secret: nil, + RedirectURIs: nil, + GrantTypes: nil, + ResponseTypes: nil, + Scopes: nil, + Audience: nil, + Public: true, + }, + JSONWebKeysURI: "where", + JSONWebKeys: nil, + TokenEndpointAuthMethod: "something", + RequestURIs: nil, + RequestObjectSigningAlgorithm: "", + TokenEndpointAuthSigningAlgorithm: "", + }, + RequestedScope: nil, + GrantedScope: nil, + Form: url.Values{"key": []string{"val"}}, + Session: &openid.DefaultSession{ + Claims: nil, + Headers: nil, + ExpiresAt: nil, + Username: "snorlax", + Subject: "panda", + }, + RequestedAudience: nil, + GrantedAudience: nil, + } + err := storage.CreateAccessTokenSession(ctx, "fancy-signature", request) + require.NoError(t, err) + + newRequest, err := storage.GetAccessTokenSession(ctx, "fancy-signature", nil) + require.NoError(t, err) + require.Equal(t, request, newRequest) + + err = storage.DeleteAccessTokenSession(ctx, "fancy-signature") + require.NoError(t, err) + + require.Equal(t, wantActions, client.Actions()) +} + +func TestAccessTokenStorageRevocation(t *testing.T) { + ctx := context.Background() + + wantActions := []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "access-token", + "storage.pinniped.dev/request-id": "abcd-1", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + }), + coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ + LabelSelector: "storage.pinniped.dev=access-token,storage.pinniped.dev/request-id=abcd-1", + }), + coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-access-token-pwu5zs7lekbhnln2w4"), + } + + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + ID: "abcd-1", + RequestedAt: time.Time{}, + Client: &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinny", + Public: true, + }, + JSONWebKeysURI: "where", + TokenEndpointAuthMethod: "something", + }, + Form: url.Values{"key": []string{"val"}}, + Session: &openid.DefaultSession{ + Username: "snorlax", + Subject: "panda", + }, + } + err := storage.CreateAccessTokenSession(ctx, "fancy-signature", request) + require.NoError(t, err) + + // Revoke the request ID of the session that we just created + err = storage.RevokeAccessToken(ctx, "abcd-1") + require.NoError(t, err) + + require.Equal(t, wantActions, client.Actions()) +} + +func TestGetNotFound(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + _, notFoundErr := storage.GetAccessTokenSession(ctx, "non-existent-signature", nil) + require.EqualError(t, notFoundErr, "not_found") + require.True(t, errors.Is(notFoundErr, fosite.ErrNotFound)) +} + +func TestWrongVersion(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "access-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"not-the-right-version"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + } + _, err := secrets.Create(ctx, secret, metav1.CreateOptions{}) + require.NoError(t, err) + + _, err = storage.GetAccessTokenSession(ctx, "fancy-signature", nil) + + require.EqualError(t, err, "access token request data has wrong version: access token session for fancy-signature has version not-the-right-version instead of 1") +} + +func TestNilSessionRequest(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "access-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"nonsense-key": "nonsense-value","version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + } + + _, err := secrets.Create(ctx, secret, metav1.CreateOptions{}) + require.NoError(t, err) + + _, err = storage.GetAccessTokenSession(ctx, "fancy-signature", nil) + require.EqualError(t, err, "malformed access token session for fancy-signature: access token request data must be present") +} + +func TestCreateWithNilRequester(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + err := storage.CreateAccessTokenSession(ctx, "signature-doesnt-matter", nil) + require.EqualError(t, err, "requester must be of type fosite.Request") +} + +func TestCreateWithWrongRequesterDataTypes(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + Session: nil, + Client: &fosite.DefaultOpenIDConnectClient{}, + } + err := storage.CreateAccessTokenSession(ctx, "signature-doesnt-matter", request) + require.EqualError(t, err, "requester's session must be of type openid.DefaultSession") + + request = &fosite.Request{ + Session: &openid.DefaultSession{}, + Client: nil, + } + err = storage.CreateAccessTokenSession(ctx, "signature-doesnt-matter", request) + require.EqualError(t, err, "requester's client must be of type fosite.DefaultOpenIDConnectClient") +} + +func TestCreateWithoutRequesterID(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + ID: "", // empty ID + Session: &openid.DefaultSession{}, + Client: &fosite.DefaultOpenIDConnectClient{}, + } + err := storage.CreateAccessTokenSession(ctx, "signature-doesnt-matter", request) + require.NoError(t, err) + + // the blank ID was filled in with an auto-generated ID + require.NotEmpty(t, request.ID) + + require.Len(t, client.Actions(), 1) + actualAction := client.Actions()[0].(coretesting.CreateActionImpl) + actualSecret := actualAction.GetObject().(*corev1.Secret) + + // The generated secret was labeled with that auto-generated request ID + require.Equal(t, request.ID, actualSecret.Labels["storage.pinniped.dev/request-id"]) +} diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 8aca618a..6425804d 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -64,7 +64,7 @@ func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, s // of the consent authorization request. It is used to identify the session. // signature for lookup in the DB - _, err = a.storage.Create(ctx, signature, &AuthorizeCodeSession{Active: true, Request: request, Version: authorizeCodeStorageVersion}) + _, err = a.storage.Create(ctx, signature, &AuthorizeCodeSession{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil) return err } diff --git a/internal/fositestorage/fositestorage.go b/internal/fositestorage/fositestorage.go index d23c9f6a..d95cda5d 100644 --- a/internal/fositestorage/fositestorage.go +++ b/internal/fositestorage/fositestorage.go @@ -11,9 +11,10 @@ import ( ) const ( - ErrInvalidRequestType = constable.Error("requester must be of type fosite.Request") - ErrInvalidClientType = constable.Error("requester's client must be of type fosite.DefaultOpenIDConnectClient") - ErrInvalidSessionType = constable.Error("requester's session must be of type openid.DefaultSession") + ErrInvalidRequestType = constable.Error("requester must be of type fosite.Request") + ErrInvalidClientType = constable.Error("requester's client must be of type fosite.DefaultOpenIDConnectClient") + ErrInvalidSessionType = constable.Error("requester's session must be of type openid.DefaultSession") + StorageRequestIDLabelName = "storage.pinniped.dev/request-id" //nolint:gosec // this is not a credential ) func ValidateAndExtractAuthorizeRequest(requester fosite.Requester) (*fosite.Request, error) { diff --git a/internal/fositestorage/openidconnect/openidconnect.go b/internal/fositestorage/openidconnect/openidconnect.go index 797d21a8..51fd84a8 100644 --- a/internal/fositestorage/openidconnect/openidconnect.go +++ b/internal/fositestorage/openidconnect/openidconnect.go @@ -52,7 +52,7 @@ func (a *openIDConnectRequestStorage) CreateOpenIDConnectSession(ctx context.Con return err } - _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: oidcStorageVersion}) + _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: oidcStorageVersion}, nil) return err } diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go index 9e8ef3d5..ed82d0df 100644 --- a/internal/fositestorage/pkce/pkce.go +++ b/internal/fositestorage/pkce/pkce.go @@ -46,7 +46,7 @@ func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature st return err } - _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: pkceStorageVersion}) + _, err = a.storage.Create(ctx, signature, &session{Request: request, Version: pkceStorageVersion}, nil) return err } diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go new file mode 100644 index 00000000..15f76539 --- /dev/null +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -0,0 +1,112 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package refreshtoken + +import ( + "context" + "fmt" + + "github.com/ory/fosite" + "github.com/ory/fosite/handler/oauth2" + "github.com/ory/fosite/handler/openid" + "k8s.io/apimachinery/pkg/api/errors" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + + "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/fositestorage" +) + +const ( + ErrInvalidRefreshTokenRequestVersion = constable.Error("refresh token request data has wrong version") + ErrInvalidRefreshTokenRequestData = constable.Error("refresh token request data must be present") + + refreshTokenStorageVersion = "1" +) + +type RevocationStorage interface { + oauth2.RefreshTokenStorage + RevokeRefreshToken(ctx context.Context, requestID string) error +} + +var _ RevocationStorage = &refreshTokenStorage{} + +type refreshTokenStorage struct { + storage crud.Storage +} + +type session struct { + Request *fosite.Request `json:"request"` + Version string `json:"version"` +} + +func New(secrets corev1client.SecretInterface) RevocationStorage { + return &refreshTokenStorage{storage: crud.New("refresh-token", secrets)} +} + +func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { + return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) +} + +func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, signature string, requester fosite.Requester) error { + request, err := fositestorage.ValidateAndExtractAuthorizeRequest(requester) + if err != nil { + return err + } + + _, err = a.storage.Create( + ctx, + signature, + &session{Request: request, Version: refreshTokenStorageVersion}, + map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, + ) + return err +} + +func (a *refreshTokenStorage) GetRefreshTokenSession(ctx context.Context, signature string, _ fosite.Session) (fosite.Requester, error) { + session, _, err := a.getSession(ctx, signature) + + if err != nil { + return nil, err + } + + return session.Request, err +} + +func (a *refreshTokenStorage) DeleteRefreshTokenSession(ctx context.Context, signature string) error { + return a.storage.Delete(ctx, signature) +} + +func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) { + session := newValidEmptyRefreshTokenSession() + rv, err := a.storage.Get(ctx, signature, session) + + if errors.IsNotFound(err) { + return nil, "", fosite.ErrNotFound.WithCause(err).WithDebug(err.Error()) + } + + if err != nil { + return nil, "", fmt.Errorf("failed to get refresh token session for %s: %w", signature, err) + } + + if version := session.Version; version != refreshTokenStorageVersion { + return nil, "", fmt.Errorf("%w: refresh token session for %s has version %s instead of %s", + ErrInvalidRefreshTokenRequestVersion, signature, version, refreshTokenStorageVersion) + } + + if session.Request.ID == "" { + return nil, "", fmt.Errorf("malformed refresh token session for %s: %w", signature, ErrInvalidRefreshTokenRequestData) + } + + return session, rv, nil +} + +func newValidEmptyRefreshTokenSession() *session { + return &session{ + Request: &fosite.Request{ + Client: &fosite.DefaultOpenIDConnectClient{}, + Session: &openid.DefaultSession{}, + }, + } +} diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go new file mode 100644 index 00000000..6e1a6113 --- /dev/null +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -0,0 +1,282 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package refreshtoken + +import ( + "context" + "net/url" + "testing" + "time" + + "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/kubernetes/fake" + coretesting "k8s.io/client-go/testing" +) + +const namespace = "test-ns" + +var secretsGVR = schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "secrets", +} + +func TestRefreshTokenStorage(t *testing.T) { + ctx := context.Background() + + wantActions := []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "refresh-token", + "storage.pinniped.dev/request-id": "abcd-1", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + }), + coretesting.NewGetAction(secretsGVR, namespace, "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4"), + coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4"), + } + + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + ID: "abcd-1", + RequestedAt: time.Time{}, + Client: &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinny", + Secret: nil, + RedirectURIs: nil, + GrantTypes: nil, + ResponseTypes: nil, + Scopes: nil, + Audience: nil, + Public: true, + }, + JSONWebKeysURI: "where", + JSONWebKeys: nil, + TokenEndpointAuthMethod: "something", + RequestURIs: nil, + RequestObjectSigningAlgorithm: "", + TokenEndpointAuthSigningAlgorithm: "", + }, + RequestedScope: nil, + GrantedScope: nil, + Form: url.Values{"key": []string{"val"}}, + Session: &openid.DefaultSession{ + Claims: nil, + Headers: nil, + ExpiresAt: nil, + Username: "snorlax", + Subject: "panda", + }, + RequestedAudience: nil, + GrantedAudience: nil, + } + err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", request) + require.NoError(t, err) + + newRequest, err := storage.GetRefreshTokenSession(ctx, "fancy-signature", nil) + require.NoError(t, err) + require.Equal(t, request, newRequest) + + err = storage.DeleteRefreshTokenSession(ctx, "fancy-signature") + require.NoError(t, err) + + require.Equal(t, wantActions, client.Actions()) +} + +func TestRefreshTokenStorageRevocation(t *testing.T) { + ctx := context.Background() + + wantActions := []coretesting.Action{ + coretesting.NewCreateAction(secretsGVR, namespace, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "refresh-token", + "storage.pinniped.dev/request-id": "abcd-1", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + }), + coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ + LabelSelector: "storage.pinniped.dev=refresh-token,storage.pinniped.dev/request-id=abcd-1", + }), + coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4"), + } + + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + ID: "abcd-1", + RequestedAt: time.Time{}, + Client: &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinny", + Public: true, + }, + JSONWebKeysURI: "where", + TokenEndpointAuthMethod: "something", + }, + Form: url.Values{"key": []string{"val"}}, + Session: &openid.DefaultSession{ + Username: "snorlax", + Subject: "panda", + }, + } + err := storage.CreateRefreshTokenSession(ctx, "fancy-signature", request) + require.NoError(t, err) + + // Revoke the request ID of the session that we just created + err = storage.RevokeRefreshToken(ctx, "abcd-1") + require.NoError(t, err) + + require.Equal(t, wantActions, client.Actions()) +} + +func TestGetNotFound(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + _, notFoundErr := storage.GetRefreshTokenSession(ctx, "non-existent-signature", nil) + require.EqualError(t, notFoundErr, "not_found") + require.True(t, errors.Is(notFoundErr, fosite.ErrNotFound)) +} + +func TestWrongVersion(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "refresh-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"requestedAudience":null,"grantedAudience":null},"version":"not-the-right-version"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + } + _, err := secrets.Create(ctx, secret, metav1.CreateOptions{}) + require.NoError(t, err) + + _, err = storage.GetRefreshTokenSession(ctx, "fancy-signature", nil) + + require.EqualError(t, err, "refresh token request data has wrong version: refresh token session for fancy-signature has version not-the-right-version instead of 1") +} + +func TestNilSessionRequest(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev": "refresh-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"nonsense-key": "nonsense-value","version":"1"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + } + + _, err := secrets.Create(ctx, secret, metav1.CreateOptions{}) + require.NoError(t, err) + + _, err = storage.GetRefreshTokenSession(ctx, "fancy-signature", nil) + require.EqualError(t, err, "malformed refresh token session for fancy-signature: refresh token request data must be present") +} + +func TestCreateWithNilRequester(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", nil) + require.EqualError(t, err, "requester must be of type fosite.Request") +} + +func TestCreateWithWrongRequesterDataTypes(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + Session: nil, + Client: &fosite.DefaultOpenIDConnectClient{}, + } + err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", request) + require.EqualError(t, err, "requester's session must be of type openid.DefaultSession") + + request = &fosite.Request{ + Session: &openid.DefaultSession{}, + Client: nil, + } + err = storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", request) + require.EqualError(t, err, "requester's client must be of type fosite.DefaultOpenIDConnectClient") +} + +func TestCreateWithoutRequesterID(t *testing.T) { + ctx := context.Background() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets(namespace) + storage := New(secrets) + + request := &fosite.Request{ + ID: "", // empty ID + Session: &openid.DefaultSession{}, + Client: &fosite.DefaultOpenIDConnectClient{}, + } + err := storage.CreateRefreshTokenSession(ctx, "signature-doesnt-matter", request) + require.NoError(t, err) + + // the blank ID was filled in with an auto-generated ID + require.NotEmpty(t, request.ID) + + require.Len(t, client.Actions(), 1) + actualAction := client.Actions()[0].(coretesting.CreateActionImpl) + actualSecret := actualAction.GetObject().(*corev1.Secret) + + // The generated secret was labeled with that auto-generated request ID + require.Equal(t, request.ID, actualSecret.Labels["storage.pinniped.dev/request-id"]) +} diff --git a/internal/oidc/kube_storage.go b/internal/oidc/kube_storage.go index 405a6ade..874e3164 100644 --- a/internal/oidc/kube_storage.go +++ b/internal/oidc/kube_storage.go @@ -14,9 +14,11 @@ import ( corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/fositestorage/accesstoken" "go.pinniped.dev/internal/fositestorage/authorizationcode" "go.pinniped.dev/internal/fositestorage/openidconnect" "go.pinniped.dev/internal/fositestorage/pkce" + "go.pinniped.dev/internal/fositestorage/refreshtoken" ) const errKubeStorageNotImplemented = constable.Error("KubeStorage does not implement this method. It should not have been called.") @@ -25,6 +27,8 @@ type KubeStorage struct { authorizationCodeStorage oauth2.AuthorizeCodeStorage pkceStorage fositepkce.PKCERequestStorage oidcStorage openid.OpenIDConnectRequestStorage + accessTokenStorage accesstoken.RevocationStorage + refreshTokenStorage refreshtoken.RevocationStorage } func NewKubeStorage(secrets corev1client.SecretInterface) *KubeStorage { @@ -32,39 +36,41 @@ func NewKubeStorage(secrets corev1client.SecretInterface) *KubeStorage { authorizationCodeStorage: authorizationcode.New(secrets), pkceStorage: pkce.New(secrets), oidcStorage: openidconnect.New(secrets), + accessTokenStorage: accesstoken.New(secrets), + refreshTokenStorage: refreshtoken.New(secrets), } } -func (KubeStorage) RevokeRefreshToken(_ context.Context, _ string) error { - return errKubeStorageNotImplemented +func (k KubeStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { + return k.refreshTokenStorage.RevokeRefreshToken(ctx, requestID) } -func (KubeStorage) RevokeAccessToken(_ context.Context, _ string) error { - return errKubeStorageNotImplemented +func (k KubeStorage) RevokeAccessToken(ctx context.Context, requestID string) error { + return k.accessTokenStorage.RevokeAccessToken(ctx, requestID) } -func (KubeStorage) CreateRefreshTokenSession(_ context.Context, _ string, _ fosite.Requester) (err error) { - return nil +func (k KubeStorage) CreateRefreshTokenSession(ctx context.Context, signature string, request fosite.Requester) (err error) { + return k.refreshTokenStorage.CreateRefreshTokenSession(ctx, signature, request) } -func (KubeStorage) GetRefreshTokenSession(_ context.Context, _ string, _ fosite.Session) (request fosite.Requester, err error) { - return nil, errKubeStorageNotImplemented +func (k KubeStorage) GetRefreshTokenSession(ctx context.Context, signature string, session fosite.Session) (request fosite.Requester, err error) { + return k.refreshTokenStorage.GetRefreshTokenSession(ctx, signature, session) } -func (KubeStorage) DeleteRefreshTokenSession(_ context.Context, _ string) (err error) { - return errKubeStorageNotImplemented +func (k KubeStorage) DeleteRefreshTokenSession(ctx context.Context, signature string) (err error) { + return k.refreshTokenStorage.DeleteRefreshTokenSession(ctx, signature) } -func (KubeStorage) CreateAccessTokenSession(_ context.Context, _ string, _ fosite.Requester) (err error) { - return nil +func (k KubeStorage) CreateAccessTokenSession(ctx context.Context, signature string, requester fosite.Requester) (err error) { + return k.accessTokenStorage.CreateAccessTokenSession(ctx, signature, requester) } -func (KubeStorage) GetAccessTokenSession(_ context.Context, _ string, _ fosite.Session) (request fosite.Requester, err error) { - return nil, errKubeStorageNotImplemented +func (k KubeStorage) GetAccessTokenSession(ctx context.Context, signature string, session fosite.Session) (request fosite.Requester, err error) { + return k.accessTokenStorage.GetAccessTokenSession(ctx, signature, session) } -func (KubeStorage) DeleteAccessTokenSession(_ context.Context, _ string) (err error) { - return errKubeStorageNotImplemented +func (k KubeStorage) DeleteAccessTokenSession(ctx context.Context, signature string) (err error) { + return k.accessTokenStorage.DeleteAccessTokenSession(ctx, signature) } func (k KubeStorage) CreateOpenIDConnectSession(ctx context.Context, authcode string, requester fosite.Requester) error { diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index bf017cfe..2cf9276d 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -190,7 +190,7 @@ func TestManager(t *testing.T) { oidctestutil.VerifyECDSAIDToken(t, jwkIssuer, downstreamClientID, privateKey, idToken) // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. - r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+7, + r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+8, "did not perform any kube actions during the callback request, but should have") } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 9e8ccd11..ba7a0252 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -8,8 +8,6 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" - "crypto/sha256" - "encoding/base64" "encoding/json" "io" "io/ioutil" @@ -24,10 +22,11 @@ import ( "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/handler/pkce" - "github.com/ory/fosite/storage" "github.com/ory/fosite/token/jwt" + "github.com/pkg/errors" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" + "k8s.io/client-go/kubernetes/fake" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" @@ -55,8 +54,8 @@ const ( ) var ( - goodAuthTime = time.Date(1, 2, 3, 4, 5, 6, 7, time.Local) - goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.Local) + goodAuthTime = time.Date(1, 2, 3, 4, 5, 6, 7, time.UTC) + goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.UTC) fositeInvalidMethodErrorBody = func(actual string) string { return here.Docf(` @@ -384,15 +383,19 @@ func TestTokenEndpoint(t *testing.T) { test.authRequest(authRequest) } - oauthStore := storage.NewMemoryStore() - // Add the Pinniped CLI client. - oauthStore.Clients[goodClient] = oidc.PinnipedCLIOIDCClient() + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets("some-namespace") + + oauthStore := oidc.NewKubeStorage(secrets) oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) + if test.storage != nil { test.storage(t, oauthStore, authCode) } subject := NewHandler(oauthHelper) + // TODO add assertions about how many of each storage type exist at this point as a pre-condition + req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") if test.request != nil { @@ -421,6 +424,8 @@ func TestTokenEndpoint(t *testing.T) { if wantOpenidScope { requireValidIDToken(t, m, jwtSigningKey) } + + // TODO add assertions about how many of each storage type are remaining at this point } else { require.JSONEq(t, test.wantExactBody, rsp.Body.String()) } @@ -429,12 +434,16 @@ func TestTokenEndpoint(t *testing.T) { t.Run("auth code is used twice", func(t *testing.T) { authRequest := deepCopyRequestForm(happyAuthRequest) - oauthStore := storage.NewMemoryStore() - // Add the Pinniped CLI client. - oauthStore.Clients[goodClient] = oidc.PinnipedCLIOIDCClient() + + client := fake.NewSimpleClientset() + secrets := client.CoreV1().Secrets("some-namespace") + + oauthStore := oidc.NewKubeStorage(secrets) oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) subject := NewHandler(oauthHelper) + // TODO add assertions about how many of each storage type exist at this point as a pre-condition + req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -460,6 +469,8 @@ func TestTokenEndpoint(t *testing.T) { requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) requireValidIDToken(t, m, jwtSigningKey) + // TODO add assertions about how many of each storage type are remaining at this point + // Second call - should be unsuccessful since auth code was already used. // // Fosite will also revoke the access token as is recommended by the OIDC spec. Currently, we don't @@ -476,6 +487,8 @@ func TestTokenEndpoint(t *testing.T) { requireInvalidAccessTokenStorage(t, m, oauthStore) requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + + // TODO add assertions about how many of each storage type are remaining at this point }) } @@ -590,16 +603,6 @@ func generateJWTSigningKeyAndJWKSProvider(t *testing.T, issuer string) (*ecdsa.P return key, jwksProvider } -func hashAccessToken(accessToken string) string { - // See https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken. - // "Access Token hash value. Its value is the base64url encoding of the left-most half of - // the hash of the octets of the ASCII representation of the access_token value, where the - // hash algorithm used is the hash algorithm used in the alg Header Parameter of the ID - // Token's JOSE Header." - b := sha256.Sum256([]byte(accessToken)) - return base64.RawURLEncoding.EncodeToString(b[:len(b)/2]) -} - func requireInvalidAuthCodeStorage( t *testing.T, code string, @@ -609,7 +612,7 @@ func requireInvalidAuthCodeStorage( // Make sure we have invalidated this auth code. _, err := storage.GetAuthorizeCodeSession(context.Background(), getFositeDataSignature(t, code), nil) - require.Equal(t, fosite.ErrInvalidatedAuthorizeCode, err) + require.True(t, errors.Is(err, fosite.ErrInvalidatedAuthorizeCode)) } func requireValidAccessTokenStorage( @@ -656,8 +659,8 @@ func requireValidAccessTokenStorage( t, authRequest, authRequest.Sanitize([]string{}).GetRequestForm(), - hashAccessToken(accessTokenString), wantGrantedOpenidScope, + true, ) } @@ -674,7 +677,7 @@ func requireInvalidAccessTokenStorage( accessTokenString, ok := accessToken.(string) require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) _, err := storage.GetAccessTokenSession(context.Background(), getFositeDataSignature(t, accessTokenString), nil) - require.Equal(t, fosite.ErrNotFound, err) + require.True(t, errors.Is(err, fosite.ErrNotFound)) } func requireInvalidPKCEStorage( @@ -687,7 +690,7 @@ func requireInvalidPKCEStorage( // Make sure the PKCE session has been deleted. Note that Fosite stores PKCE codes using the auth code signature // as a key. _, err := storage.GetPKCERequestSession(context.Background(), getFositeDataSignature(t, code), nil) - require.Equal(t, fosite.ErrNotFound, err) + require.True(t, errors.Is(err, fosite.ErrNotFound)) } func requireValidOIDCStorage( @@ -709,16 +712,18 @@ func requireValidOIDCStorage( require.True(t, ok) accessTokenString, ok := accessToken.(string) require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken) + require.NotEmpty(t, accessTokenString) + requireValidAuthRequest( t, authRequest, authRequest.Sanitize([]string{"nonce"}).GetRequestForm(), - hashAccessToken(accessTokenString), true, + false, ) } else { _, err := storage.GetOpenIDConnectSession(context.Background(), code, nil) - require.Equal(t, fosite.ErrNotFound, err) + require.True(t, errors.Is(err, fosite.ErrNotFound)) } } @@ -726,8 +731,8 @@ func requireValidAuthRequest( t *testing.T, authRequest fosite.Requester, wantRequestForm url.Values, - wantAccessTokenHash string, wantGrantedOpenidScope bool, + wantAccessTokenExpiresAt bool, ) { t.Helper() @@ -755,24 +760,28 @@ func requireValidAuthRequest( if wantGrantedOpenidScope { claims := session.Claims require.Empty(t, claims.JTI) // When claims.JTI is empty, Fosite will generate a UUID for this field. - require.Equal(t, goodIssuer, claims.Issuer) require.Equal(t, goodSubject, claims.Subject) - require.Equal(t, []string{goodClient}, claims.Audience) - require.Equal(t, goodNonce, claims.Nonce) - testutil.RequireTimeInDelta( - t, - time.Now().UTC().Add(idTokenExpirationSeconds*time.Second), - claims.ExpiresAt, - timeComparisonFudgeSeconds*time.Second, - ) - testutil.RequireTimeInDelta(t, time.Now().UTC(), claims.IssuedAt, timeComparisonFudgeSeconds*time.Second) - require.Equal(t, wantAccessTokenHash, claims.AccessTokenHash) // We are in charge of setting these fields. For the purpose of testing, we ensure that the // sentinel test value is set correctly. require.Equal(t, goodRequestedAtTime, claims.RequestedAt) require.Equal(t, goodAuthTime, claims.AuthTime) + // These fields will all be given good defaults by fosite at runtime and we only need to use them + // if we want to override the default behaviors. We currently don't need to override these defaults, + // so they do not end up being stored. Fosite sets its defaults at runtime in openid.DefaultSession's + // GenerateIDToken() method. + require.Empty(t, claims.Issuer) + require.Empty(t, claims.Audience) + require.Empty(t, claims.Nonce) + require.Zero(t, claims.ExpiresAt) + require.Zero(t, claims.IssuedAt) + + // Fosite unconditionally overwrites claims.AccessTokenHash at runtime in openid.OpenIDConnectExplicitHandler's + // PopulateTokenEndpointResponse() method, just before it calls the same GenerateIDToken() mentioned above, + // so it does not end up saved in storage. + require.Empty(t, claims.AccessTokenHash) + // At this time, we don't use any of these optional (per the OIDC spec) fields. require.Empty(t, claims.AuthenticationContextClassReference) require.Empty(t, claims.AuthenticationMethodsReference) @@ -793,14 +802,20 @@ func requireValidAuthRequest( authCodeExpiresAt, timeComparisonFudgeSeconds*time.Second, ) + + // OpenID Connect sessions do not store access token expiration information. accessTokenExpiresAt, ok := session.ExpiresAt[fosite.AccessToken] - require.True(t, ok, "expected session to hold expiration time for access token") - testutil.RequireTimeInDelta( - t, - time.Now().UTC().Add(accessTokenExpirationSeconds*time.Second), - accessTokenExpiresAt, - timeComparisonFudgeSeconds*time.Second, - ) + if wantAccessTokenExpiresAt { + require.True(t, ok, "expected session to hold expiration time for access token") + testutil.RequireTimeInDelta( + t, + time.Now().UTC().Add(accessTokenExpirationSeconds*time.Second), + accessTokenExpiresAt, + timeComparisonFudgeSeconds*time.Second, + ) + } else { + require.False(t, ok, "expected session to not hold expiration time for access token, but it did") + } // Assert that the session's username and subject are correct. require.Equal(t, goodUsername, session.Username) @@ -830,7 +845,10 @@ func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKe RequestedAt int64 `json:"rat"` AuthTime int64 `json:"auth_time"` } - idTokenFields := []string{"sub", "aud", "iss", "jti", "nonce", "auth_time", "at_hash", "exp", "iat", "rat"} + + // Note that there is a bug in fosite which prevents the `at_hash` claim from appearing in this ID token. + // We can add a workaround for this later. + idTokenFields := []string{"sub", "aud", "iss", "jti", "nonce", "auth_time", "exp", "iat", "rat"} // make sure that these are the only fields in the token var m map[string]interface{} @@ -846,7 +864,6 @@ func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKe require.Equal(t, goodIssuer, claims.Issuer) require.NotEmpty(t, claims.JTI) require.Equal(t, goodNonce, claims.Nonce) - require.NotEmpty(t, claims.AccessTokenHash) expiresAt := time.Unix(claims.ExpiresAt, 0) issuedAt := time.Unix(claims.IssuedAt, 0) From 26a87475095c3d1de02f633a534a66336e350e72 Mon Sep 17 00:00:00 2001 From: Aram Price Date: Fri, 4 Dec 2020 14:39:11 -0800 Subject: [PATCH 16/20] Use the more specific label name of "storage.pinniped.dev/type" Instead of the less specific "storage.pinniped.dev" Signed-off-by: Ryan Richard --- internal/crud/crud.go | 2 +- internal/crud/crud_test.go | 86 +++++++++---------- .../accesstoken/accesstoken_test.go | 10 +-- .../authorizationcode_test.go | 8 +- .../openidconnect/openidconnect_test.go | 6 +- internal/fositestorage/pkce/pkce_test.go | 6 +- .../refreshtoken/refreshtoken_test.go | 10 +-- 7 files changed, 64 insertions(+), 64 deletions(-) diff --git a/internal/crud/crud.go b/internal/crud/crud.go index d5386e41..e04c5403 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -23,7 +23,7 @@ import ( //nolint:gosec // ignore lint warnings that these are credentials const ( secretNameFormat = "pinniped-storage-%s-%s" - secretLabelKey = "storage.pinniped.dev" + secretLabelKey = "storage.pinniped.dev/type" secretTypeFormat = "storage.pinniped.dev/%s" secretVersion = "1" secretDataKey = "pinniped-storage-data" diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 5ff4aedc..58c5f6ed 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -117,7 +117,7 @@ func TestStorage(t *testing.T) { Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-tokens", + "storage.pinniped.dev/type": "access-tokens", }, }, Data: map[string][]byte{ @@ -135,7 +135,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-tokens", + "storage.pinniped.dev/type": "access-tokens", }, }, Data: map[string][]byte{ @@ -175,9 +175,9 @@ func TestStorage(t *testing.T) { Name: "pinniped-storage-access-tokens-i6mhp4azwdxshgsy3s2mvedxpxuh3nudh3ot3m4xamlugj4e6qoq", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-tokens", - "label1": "value1", - "label2": "value2", + "storage.pinniped.dev/type": "access-tokens", + "label1": "value1", + "label2": "value2", }, }, Data: map[string][]byte{ @@ -195,9 +195,9 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-tokens", - "label1": "value1", - "label2": "value2", + "storage.pinniped.dev/type": "access-tokens", + "label1": "value1", + "label2": "value2", }, }, Data: map[string][]byte{ @@ -219,7 +219,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "pandas-are-best", + "storage.pinniped.dev/type": "pandas-are-best", }, }, Data: map[string][]byte{ @@ -254,7 +254,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "pandas-are-best", + "storage.pinniped.dev/type": "pandas-are-best", }, }, Data: map[string][]byte{ @@ -276,7 +276,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "35", Labels: map[string]string{ - "storage.pinniped.dev": "stores", + "storage.pinniped.dev/type": "stores", }, }, Data: map[string][]byte{ @@ -325,7 +325,7 @@ func TestStorage(t *testing.T) { Name: "pinniped-storage-stores-4wssc5gzt5mlln6iux6gl7hzz3klsirisydaxn7indnpvdnrs5ba", ResourceVersion: "35", // update at initial RV Labels: map[string]string{ - "storage.pinniped.dev": "stores", + "storage.pinniped.dev/type": "stores", }, }, Data: map[string][]byte{ @@ -343,7 +343,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "45", // final list at new RV Labels: map[string]string{ - "storage.pinniped.dev": "stores", + "storage.pinniped.dev/type": "stores", }, }, Data: map[string][]byte{ @@ -365,7 +365,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "seals", + "storage.pinniped.dev/type": "seals", }, }, Data: map[string][]byte{ @@ -399,8 +399,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "seals", - "additionalLabel": "matching-value", + "storage.pinniped.dev/type": "seals", + "additionalLabel": "matching-value", }, }, Data: map[string][]byte{ @@ -415,8 +415,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "seals", - "additionalLabel": "matching-value", + "storage.pinniped.dev/type": "seals", + "additionalLabel": "matching-value", }, }, Data: map[string][]byte{ @@ -431,8 +431,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "seals", // same type as above - "additionalLabel": "non-matching-value", // different value for the same label + "storage.pinniped.dev/type": "seals", // same type as above + "additionalLabel": "non-matching-value", // different value for the same label }, }, Data: map[string][]byte{ @@ -447,8 +447,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "walruses", // different type from above - "additionalLabel": "matching-value", // same value for the same label as above + "storage.pinniped.dev/type": "walruses", // different type from above + "additionalLabel": "matching-value", // same value for the same label as above }, }, Data: map[string][]byte{ @@ -463,7 +463,7 @@ func TestStorage(t *testing.T) { }, wantActions: []coretesting.Action{ coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ - LabelSelector: "storage.pinniped.dev=seals,additionalLabel=matching-value", + LabelSelector: "storage.pinniped.dev/type=seals,additionalLabel=matching-value", }), coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-seals-abcdywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq"), coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq"), @@ -476,8 +476,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "seals", // same type as above - "additionalLabel": "non-matching-value", // different value for the same label + "storage.pinniped.dev/type": "seals", // same type as above + "additionalLabel": "non-matching-value", // different value for the same label }, }, Data: map[string][]byte{ @@ -493,8 +493,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "walruses", // different type from above - "additionalLabel": "matching-value", // same value for the same label as above + "storage.pinniped.dev/type": "walruses", // different type from above + "additionalLabel": "matching-value", // same value for the same label as above }, }, Data: map[string][]byte{ @@ -516,8 +516,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "seals", - "additionalLabel": "matching-value", + "storage.pinniped.dev/type": "seals", + "additionalLabel": "matching-value", }, }, Data: map[string][]byte{ @@ -535,7 +535,7 @@ func TestStorage(t *testing.T) { }, wantActions: []coretesting.Action{ coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ - LabelSelector: "storage.pinniped.dev=seals,additionalLabel=matching-value", + LabelSelector: "storage.pinniped.dev/type=seals,additionalLabel=matching-value", }), coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-seals-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq"), }, @@ -546,8 +546,8 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "seals", - "additionalLabel": "matching-value", + "storage.pinniped.dev/type": "seals", + "additionalLabel": "matching-value", }, }, Data: map[string][]byte{ @@ -571,9 +571,9 @@ func TestStorage(t *testing.T) { // this list action did not use label selector additionalLabel=matching-value, so allow it to proceed without intervention return false, nil, nil } - requiresExactMatch, found = labelRestrictions.RequiresExactMatch("storage.pinniped.dev") + requiresExactMatch, found = labelRestrictions.RequiresExactMatch("storage.pinniped.dev/type") if !found || requiresExactMatch != "seals" { - // this list action did not use label selector storage.pinniped.dev=seals, so allow it to proceed without intervention + // this list action did not use label selector storage.pinniped.dev/type=seals, so allow it to proceed without intervention return false, nil, nil } // this list action was the one that did use the expected label selectors so cause it to error @@ -585,7 +585,7 @@ func TestStorage(t *testing.T) { }, wantActions: []coretesting.Action{ coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ - LabelSelector: "storage.pinniped.dev=seals,additionalLabel=matching-value", + LabelSelector: "storage.pinniped.dev/type=seals,additionalLabel=matching-value", }), }, wantErr: `failed to list secrets for resource "seals" matching label "additionalLabel=matching-value": some listing error`, @@ -600,7 +600,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies", + "storage.pinniped.dev/type": "candies", }, }, Data: map[string][]byte{ @@ -635,7 +635,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies", + "storage.pinniped.dev/type": "candies", }, }, Data: map[string][]byte{ @@ -657,7 +657,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies-are-bad", + "storage.pinniped.dev/type": "candies-are-bad", }, }, Data: map[string][]byte{ @@ -692,7 +692,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies-are-bad", + "storage.pinniped.dev/type": "candies-are-bad", }, }, Data: map[string][]byte{ @@ -714,7 +714,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies", + "storage.pinniped.dev/type": "candies", }, }, Data: map[string][]byte{ @@ -749,7 +749,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies", + "storage.pinniped.dev/type": "candies", }, }, Data: map[string][]byte{ @@ -771,7 +771,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies", + "storage.pinniped.dev/type": "candies", }, }, Data: map[string][]byte{ @@ -805,7 +805,7 @@ func TestStorage(t *testing.T) { Namespace: namespace, ResourceVersion: "55", Labels: map[string]string{ - "storage.pinniped.dev": "candies", + "storage.pinniped.dev/type": "candies", }, }, Data: map[string][]byte{ diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index f03ef852..9ac39995 100644 --- a/internal/fositestorage/accesstoken/accesstoken_test.go +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -37,7 +37,7 @@ func TestAccessTokenStorage(t *testing.T) { Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-token", + "storage.pinniped.dev/type": "access-token", "storage.pinniped.dev/request-id": "abcd-1", }, }, @@ -111,7 +111,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) { Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-token", + "storage.pinniped.dev/type": "access-token", "storage.pinniped.dev/request-id": "abcd-1", }, }, @@ -122,7 +122,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) { Type: "storage.pinniped.dev/access-token", }), coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ - LabelSelector: "storage.pinniped.dev=access-token,storage.pinniped.dev/request-id=abcd-1", + LabelSelector: "storage.pinniped.dev/type=access-token,storage.pinniped.dev/request-id=abcd-1", }), coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-access-token-pwu5zs7lekbhnln2w4"), } @@ -180,7 +180,7 @@ func TestWrongVersion(t *testing.T) { Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-token", + "storage.pinniped.dev/type": "access-token", }, }, Data: map[string][]byte{ @@ -208,7 +208,7 @@ func TestNilSessionRequest(t *testing.T) { Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "access-token", + "storage.pinniped.dev/type": "access-token", }, }, Data: map[string][]byte{ diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 616eb2de..904d6074 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -49,7 +49,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "authcode", + "storage.pinniped.dev/type": "authcode", }, }, Data: map[string][]byte{ @@ -65,7 +65,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "authcode", + "storage.pinniped.dev/type": "authcode", }, }, Data: map[string][]byte{ @@ -189,7 +189,7 @@ func TestWrongVersion(t *testing.T) { Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "authcode", + "storage.pinniped.dev/type": "authcode", }, }, Data: map[string][]byte{ @@ -217,7 +217,7 @@ func TestNilSessionRequest(t *testing.T) { Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "authcode", + "storage.pinniped.dev/type": "authcode", }, }, Data: map[string][]byte{ diff --git a/internal/fositestorage/openidconnect/openidconnect_test.go b/internal/fositestorage/openidconnect/openidconnect_test.go index 976828ed..83e86d4b 100644 --- a/internal/fositestorage/openidconnect/openidconnect_test.go +++ b/internal/fositestorage/openidconnect/openidconnect_test.go @@ -36,7 +36,7 @@ func TestOpenIdConnectStorage(t *testing.T) { Name: "pinniped-storage-oidc-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "oidc", + "storage.pinniped.dev/type": "oidc", }, }, Data: map[string][]byte{ @@ -122,7 +122,7 @@ func TestWrongVersion(t *testing.T) { Name: "pinniped-storage-oidc-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "oidc", + "storage.pinniped.dev/type": "oidc", }, }, Data: map[string][]byte{ @@ -150,7 +150,7 @@ func TestNilSessionRequest(t *testing.T) { Name: "pinniped-storage-oidc-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "oidc", + "storage.pinniped.dev/type": "oidc", }, }, Data: map[string][]byte{ diff --git a/internal/fositestorage/pkce/pkce_test.go b/internal/fositestorage/pkce/pkce_test.go index 80b2d9dd..be1b9bf4 100644 --- a/internal/fositestorage/pkce/pkce_test.go +++ b/internal/fositestorage/pkce/pkce_test.go @@ -36,7 +36,7 @@ func TestPKCEStorage(t *testing.T) { Name: "pinniped-storage-pkce-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "pkce", + "storage.pinniped.dev/type": "pkce", }, }, Data: map[string][]byte{ @@ -122,7 +122,7 @@ func TestWrongVersion(t *testing.T) { Name: "pinniped-storage-pkce-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "pkce", + "storage.pinniped.dev/type": "pkce", }, }, Data: map[string][]byte{ @@ -150,7 +150,7 @@ func TestNilSessionRequest(t *testing.T) { Name: "pinniped-storage-pkce-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "pkce", + "storage.pinniped.dev/type": "pkce", }, }, Data: map[string][]byte{ diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index 6e1a6113..bb1e664f 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -37,7 +37,7 @@ func TestRefreshTokenStorage(t *testing.T) { Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "refresh-token", + "storage.pinniped.dev/type": "refresh-token", "storage.pinniped.dev/request-id": "abcd-1", }, }, @@ -111,7 +111,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "refresh-token", + "storage.pinniped.dev/type": "refresh-token", "storage.pinniped.dev/request-id": "abcd-1", }, }, @@ -122,7 +122,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { Type: "storage.pinniped.dev/refresh-token", }), coretesting.NewListAction(secretsGVR, schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, namespace, metav1.ListOptions{ - LabelSelector: "storage.pinniped.dev=refresh-token,storage.pinniped.dev/request-id=abcd-1", + LabelSelector: "storage.pinniped.dev/type=refresh-token,storage.pinniped.dev/request-id=abcd-1", }), coretesting.NewDeleteAction(secretsGVR, namespace, "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4"), } @@ -180,7 +180,7 @@ func TestWrongVersion(t *testing.T) { Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "refresh-token", + "storage.pinniped.dev/type": "refresh-token", }, }, Data: map[string][]byte{ @@ -208,7 +208,7 @@ func TestNilSessionRequest(t *testing.T) { Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", ResourceVersion: "", Labels: map[string]string{ - "storage.pinniped.dev": "refresh-token", + "storage.pinniped.dev/type": "refresh-token", }, }, Data: map[string][]byte{ From 858356610c0884d16a771ae6670a1c9119120198 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 4 Dec 2020 15:40:17 -0800 Subject: [PATCH 17/20] Make assertions about how many secrets were stored by fosite in tests In both callback_handler_test.go and token_handler_test.go Signed-off-by: Aram Price --- internal/crud/crud.go | 9 ++-- .../fositestorage/accesstoken/accesstoken.go | 4 +- .../authorizationcode/authorizationcode.go | 4 +- .../openidconnect/openidconnect.go | 4 +- internal/fositestorage/pkce/pkce.go | 4 +- .../refreshtoken/refreshtoken.go | 4 +- .../oidc/callback/callback_handler_test.go | 42 +++++------------ internal/oidc/token/token_handler_test.go | 46 ++++++++++++++++--- internal/testutil/assertions.go | 13 ++++++ 9 files changed, 84 insertions(+), 46 deletions(-) diff --git a/internal/crud/crud.go b/internal/crud/crud.go index e04c5403..77160e52 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -22,8 +22,9 @@ import ( //nolint:gosec // ignore lint warnings that these are credentials const ( + SecretLabelKey = "storage.pinniped.dev/type" + secretNameFormat = "pinniped-storage-%s-%s" - secretLabelKey = "storage.pinniped.dev/type" secretTypeFormat = "storage.pinniped.dev/%s" secretVersion = "1" secretDataKey = "pinniped-storage-data" @@ -90,7 +91,7 @@ func (s *secretsStorage) validateSecret(secret *corev1.Secret) error { if secret.Type != s.secretType { return fmt.Errorf("%w: %s must equal %s", ErrSecretTypeMismatch, secret.Type, s.secretType) } - if labelResource := secret.Labels[secretLabelKey]; labelResource != s.resource { + if labelResource := secret.Labels[SecretLabelKey]; labelResource != s.resource { return fmt.Errorf("%w: %s must equal %s", ErrSecretLabelMismatch, labelResource, s.resource) } if !bytes.Equal(secret.Data[secretVersionKey], s.secretVersion) { @@ -121,7 +122,7 @@ func (s *secretsStorage) Delete(ctx context.Context, signature string) error { func (s *secretsStorage) DeleteByLabel(ctx context.Context, labelName string, labelValue string) error { list, err := s.secrets.List(ctx, metav1.ListOptions{ LabelSelector: labels.Set{ - secretLabelKey: s.resource, + SecretLabelKey: s.resource, labelName: labelValue, }.String(), }) @@ -158,7 +159,7 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, } labels := map[string]string{ - secretLabelKey: s.resource, // make it easier to find this stuff via kubectl + SecretLabelKey: s.resource, // make it easier to find this stuff via kubectl } for labelName, labelValue := range additionalLabels { labels[labelName] = labelValue diff --git a/internal/fositestorage/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go index 15623272..39e63f2b 100644 --- a/internal/fositestorage/accesstoken/accesstoken.go +++ b/internal/fositestorage/accesstoken/accesstoken.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "access-token" + ErrInvalidAccessTokenRequestVersion = constable.Error("access token request data has wrong version") ErrInvalidAccessTokenRequestData = constable.Error("access token request data must be present") @@ -42,7 +44,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) RevocationStorage { - return &accessTokenStorage{storage: crud.New("access-token", secrets)} + return &accessTokenStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *accessTokenStorage) RevokeAccessToken(ctx context.Context, requestID string) error { diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 6425804d..fc4cb1e7 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -20,6 +20,8 @@ import ( ) const ( + TypeLabelValue = "authcode" + ErrInvalidAuthorizeRequestData = constable.Error("authorization request data must be present") ErrInvalidAuthorizeRequestVersion = constable.Error("authorization request data has wrong version") @@ -39,7 +41,7 @@ type AuthorizeCodeSession struct { } func New(secrets corev1client.SecretInterface) oauth2.AuthorizeCodeStorage { - return &authorizeCodeStorage{storage: crud.New("authcode", secrets)} + return &authorizeCodeStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, signature string, requester fosite.Requester) error { diff --git a/internal/fositestorage/openidconnect/openidconnect.go b/internal/fositestorage/openidconnect/openidconnect.go index 51fd84a8..932e7d35 100644 --- a/internal/fositestorage/openidconnect/openidconnect.go +++ b/internal/fositestorage/openidconnect/openidconnect.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "oidc" + ErrInvalidOIDCRequestVersion = constable.Error("oidc request data has wrong version") ErrInvalidOIDCRequestData = constable.Error("oidc request data must be present") ErrMalformedAuthorizationCode = constable.Error("malformed authorization code") @@ -38,7 +40,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) openid.OpenIDConnectRequestStorage { - return &openIDConnectRequestStorage{storage: crud.New("oidc", secrets)} + return &openIDConnectRequestStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *openIDConnectRequestStorage) CreateOpenIDConnectSession(ctx context.Context, authcode string, requester fosite.Requester) error { diff --git a/internal/fositestorage/pkce/pkce.go b/internal/fositestorage/pkce/pkce.go index ed82d0df..24d554d2 100644 --- a/internal/fositestorage/pkce/pkce.go +++ b/internal/fositestorage/pkce/pkce.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "pkce" + ErrInvalidPKCERequestVersion = constable.Error("pkce request data has wrong version") ErrInvalidPKCERequestData = constable.Error("pkce request data must be present") @@ -37,7 +39,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) pkce.PKCERequestStorage { - return &pkceStorage{storage: crud.New("pkce", secrets)} + return &pkceStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *pkceStorage) CreatePKCERequestSession(ctx context.Context, signature string, requester fosite.Requester) error { diff --git a/internal/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index 15f76539..5cccbf74 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -19,6 +19,8 @@ import ( ) const ( + TypeLabelValue = "refresh-token" + ErrInvalidRefreshTokenRequestVersion = constable.Error("refresh token request data has wrong version") ErrInvalidRefreshTokenRequestData = constable.Error("refresh token request data must be present") @@ -42,7 +44,7 @@ type session struct { } func New(secrets corev1client.SecretInterface) RevocationStorage { - return &refreshTokenStorage{storage: crud.New("refresh-token", secrets)} + return &refreshTokenStorage{storage: crud.New(TypeLabelValue, secrets)} } func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index d73fb097..5a0e8c6c 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -18,18 +18,20 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes/fake" - kubetesting "k8s.io/client-go/testing" + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/fositestorage/openidconnect" + "go.pinniped.dev/internal/fositestorage/pkce" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/oidctestutil" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/oidctypes" - "go.pinniped.dev/pkg/oidcclient/pkce" + oidcpkce "go.pinniped.dev/pkg/oidcclient/pkce" ) const ( @@ -106,7 +108,7 @@ func TestCallbackEndpoint(t *testing.T) { happyExchangeAndValidateTokensArgs := &oidctestutil.ExchangeAuthcodeAndValidateTokenArgs{ Authcode: happyUpstreamAuthcode, - PKCECodeVerifier: pkce.Code(happyDownstreamPKCE), + PKCECodeVerifier: oidcpkce.Code(happyDownstreamPKCE), ExpectedIDTokenNonce: nonce.Nonce(happyDownstreamNonce), RedirectURI: happyUpstreamRedirectURI, } @@ -484,18 +486,8 @@ func TestCallbackEndpoint(t *testing.T) { } require.Len(t, client.Actions(), expectedNumberOfCreatedSecrets) - actualSecretNames := []string{} - for i := range client.Actions() { - actualAction := client.Actions()[i].(kubetesting.CreateActionImpl) - require.Equal(t, "create", actualAction.GetVerb()) - require.Equal(t, schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"}, actualAction.GetResource()) - actualSecret := actualAction.GetObject().(*corev1.Secret) - require.Empty(t, actualSecret.Namespace) // because the secrets client is already scoped to a namespace - actualSecretNames = append(actualSecretNames, actualSecret.Name) - } - // One authcode should have been stored. - requireAnyStringHasPrefix(t, actualSecretNames, "pinniped-storage-authcode-") + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) storedRequestFromAuthcode, storedSessionFromAuthcode := validateAuthcodeStorage( t, @@ -508,7 +500,7 @@ func TestCallbackEndpoint(t *testing.T) { ) // One PKCE should have been stored. - requireAnyStringHasPrefix(t, actualSecretNames, "pinniped-storage-pkce-") + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: pkce.TypeLabelValue}, 1) validatePKCEStorage( t, @@ -522,7 +514,7 @@ func TestCallbackEndpoint(t *testing.T) { // One IDSession should have been stored, if the downstream actually requested the "openid" scope if test.wantGrantedOpenidScope { - requireAnyStringHasPrefix(t, actualSecretNames, "pinniped-storage-oidc") + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) validateIDSessionStorage( t, @@ -684,7 +676,7 @@ func (u *upstreamOIDCIdentityProviderBuilder) Build() oidctestutil.TestUpstreamO UsernameClaim: u.usernameClaim, GroupsClaim: u.groupsClaim, Scopes: []string{"scope1", "scope2"}, - ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce) (oidctypes.Token, map[string]interface{}, error) { + ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier oidcpkce.Code, expectedIDTokenNonce nonce.Nonce) (oidctypes.Token, map[string]interface{}, error) { return oidctypes.Token{}, u.idToken, u.authcodeExchangeErr }, } @@ -847,15 +839,3 @@ func castStoredAuthorizeRequest(t *testing.T, storedAuthorizeRequest fosite.Requ return storedRequest, storedSession } - -func requireAnyStringHasPrefix(t *testing.T, stringList []string, prefix string) { - t.Helper() - - containsPrefix := false - for i := range stringList { - if strings.HasPrefix(stringList[i], prefix) { - containsPrefix = true - } - } - require.Truef(t, containsPrefix, "list %v did not contain any strings with prefix %s", stringList, prefix) -} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index ba7a0252..a56a649c 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -26,8 +26,15 @@ import ( "github.com/pkg/errors" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" + "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes/fake" + "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/fositestorage/accesstoken" + "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/fositestorage/openidconnect" + storagepkce "go.pinniped.dev/internal/fositestorage/pkce" + "go.pinniped.dev/internal/fositestorage/refreshtoken" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/jwks" @@ -394,7 +401,14 @@ func TestTokenEndpoint(t *testing.T) { } subject := NewHandler(oauthHelper) - // TODO add assertions about how many of each storage type exist at this point as a pre-condition + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 1) + if strings.Contains(authRequest.Form.Get("scope"), "openid") { + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) + } else { + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) + } req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -421,11 +435,18 @@ func TestTokenEndpoint(t *testing.T) { requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) + if wantOpenidScope { requireValidIDToken(t, m, jwtSigningKey) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) + } else { + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) } - - // TODO add assertions about how many of each storage type are remaining at this point } else { require.JSONEq(t, test.wantExactBody, rsp.Body.String()) } @@ -442,7 +463,10 @@ func TestTokenEndpoint(t *testing.T) { oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) subject := NewHandler(oauthHelper) - // TODO add assertions about how many of each storage type exist at this point as a pre-condition + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyBody(authCode).ReadCloser()) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -469,7 +493,12 @@ func TestTokenEndpoint(t *testing.T) { requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) requireValidIDToken(t, m, jwtSigningKey) - // TODO add assertions about how many of each storage type are remaining at this point + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 3) // Second call - should be unsuccessful since auth code was already used. // @@ -488,7 +517,12 @@ func TestTokenEndpoint(t *testing.T) { requireInvalidPKCEStorage(t, code, oauthStore) requireValidOIDCStorage(t, m, code, oauthStore, wantOpenidScope) - // TODO add assertions about how many of each storage type are remaining at this point + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: storagepkce.TypeLabelValue}, 0) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1) + testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{}, 2) }) } diff --git a/internal/testutil/assertions.go b/internal/testutil/assertions.go index e49233d5..53f5f73a 100644 --- a/internal/testutil/assertions.go +++ b/internal/testutil/assertions.go @@ -4,11 +4,15 @@ package testutil import ( + "context" "mime" "testing" "time" "github.com/stretchr/testify/require" + v12 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + v1 "k8s.io/client-go/kubernetes/typed/core/v1" ) func RequireTimeInDelta(t *testing.T, t1 time.Time, t2 time.Time, delta time.Duration) { @@ -39,3 +43,12 @@ func RequireEqualContentType(t *testing.T, actual string, expected string) { require.Equal(t, actualContentType, expectedContentType) require.Equal(t, actualContentTypeParams, expectedContentTypeParams) } + +func RequireNumberOfSecretsMatchingLabelSelector(t *testing.T, secrets v1.SecretInterface, labelSet labels.Set, expectedNumberOfSecrets int) { + t.Helper() + storedAuthcodeSecrets, err := secrets.List(context.Background(), v12.ListOptions{ + LabelSelector: labelSet.String(), + }) + require.NoError(t, err) + require.Len(t, storedAuthcodeSecrets.Items, expectedNumberOfSecrets) +} From e0b6133bf1095ab7c5a7e8a17187965f3a76dea2 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 4 Dec 2020 17:07:04 -0800 Subject: [PATCH 18/20] Integration tests call supervisor token endpoint and validate response Signed-off-by: Aram Price --- test/integration/supervisor_login_test.go | 42 +++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 8835752f..da8c10fd 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -24,6 +24,7 @@ import ( configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" idpv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/idp/v1alpha1" "go.pinniped.dev/internal/certauthority" + "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" "go.pinniped.dev/pkg/oidcclient/state" @@ -67,6 +68,7 @@ func TestSupervisorLogin(t *testing.T) { return proxyURL, nil }, }} + oidcHttpClientContext := oidc.ClientContext(ctx, httpClient) // Use the CA to issue a TLS server cert. t.Logf("issuing test certificate") @@ -109,7 +111,7 @@ func TestSupervisorLogin(t *testing.T) { // Perform OIDC discovery for our downstream. var discovery *oidc.Provider assert.Eventually(t, func() bool { - discovery, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), downstream.Spec.Issuer) + discovery, err = oidc.NewProvider(oidcHttpClientContext, downstream.Spec.Issuer) return err == nil }, 30*time.Second, 200*time.Millisecond) require.NoError(t, err) @@ -158,7 +160,43 @@ func TestSupervisorLogin(t *testing.T) { t.Logf("got callback request: %s", library.MaskTokens(callback.URL.String())) require.Equal(t, stateParam.String(), callback.URL.Query().Get("state")) require.Equal(t, "openid", callback.URL.Query().Get("scope")) - require.NotEmpty(t, callback.URL.Query().Get("code")) + authcode := callback.URL.Query().Get("code") + require.NotEmpty(t, authcode) + + // Call the token endpoint to get tokens. + tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHttpClientContext, authcode, pkceParam.Verifier()) + require.NoError(t, err) + + // Verify the ID Token. + rawIDToken, ok := tokenResponse.Extra("id_token").(string) + require.True(t, ok, "expected to get an ID token but did not") + var verifier = discovery.Verifier(&oidc.Config{ClientID: downstreamOAuth2Config.ClientID}) + idToken, err := verifier.Verify(ctx, rawIDToken) + require.NoError(t, err) + + // Check the claims of the ID token. + expectedSubjectPrefix := env.SupervisorTestUpstream.Issuer + "?sub=" + require.True(t, strings.HasPrefix(idToken.Subject, expectedSubjectPrefix)) + require.Greater(t, len(idToken.Subject), len(expectedSubjectPrefix), + "the ID token Subject should include the upstream user ID after the upstream issuer name") + require.NoError(t, nonceParam.Validate(idToken)) + testutil.RequireTimeInDelta(t, time.Now().UTC().Add(time.Minute*5), idToken.Expiry, time.Second*30) + idTokenClaims := map[string]interface{}{} + err = idToken.Claims(&idTokenClaims) + require.NoError(t, err) + idTokenClaimNames := []string{} + for k := range idTokenClaims { + idTokenClaimNames = append(idTokenClaimNames, k) + } + require.ElementsMatch(t, []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat"}, idTokenClaimNames) + + // Some light verification of the other tokens that were returned. + require.NotEmpty(t, tokenResponse.AccessToken) + require.Equal(t, "bearer", tokenResponse.TokenType) + require.NotZero(t, tokenResponse.Expiry) + testutil.RequireTimeInDelta(t, time.Now().UTC().Add(time.Minute*5), tokenResponse.Expiry, time.Second*30) + + require.Empty(t, tokenResponse.RefreshToken) // for now, until the next user story :) } func startLocalCallbackServer(t *testing.T) *localCallbackServer { From 648fa4b9ba2f1c38924472f3c3635b70dc3df3a4 Mon Sep 17 00:00:00 2001 From: Aram Price Date: Mon, 7 Dec 2020 11:53:24 -0800 Subject: [PATCH 19/20] Backfill test for token endpoint error when JWK is not yet available Signed-off-by: Ryan Richard --- .../dynamic_open_id_connect_ecdsa_strategy.go | 4 +- ...mic_open_id_connect_ecdsa_strategy_test.go | 19 ++++-- internal/oidc/token/token_handler_test.go | 64 +++++++++++++++++-- test/integration/supervisor_login_test.go | 6 +- 4 files changed, 77 insertions(+), 16 deletions(-) diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go index 582b9cc6..302044db 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go @@ -50,7 +50,7 @@ func (s *dynamicOpenIDConnectECDSAStrategy) GenerateIDToken( _, activeJwk := s.jwksProvider.GetJWKS(s.fositeConfig.IDTokenIssuer) if activeJwk == nil { plog.Debug("no JWK found for issuer", "issuer", s.fositeConfig.IDTokenIssuer) - return "", constable.Error("no JWK found for issuer") + return "", fosite.ErrTemporarilyUnavailable.WithCause(constable.Error("no JWK found for issuer")) } key, ok := activeJwk.Key.(*ecdsa.PrivateKey) if !ok { @@ -65,7 +65,7 @@ func (s *dynamicOpenIDConnectECDSAStrategy) GenerateIDToken( "actualType", actualType, ) - return "", constable.Error("JWK must be of type ecdsa") + return "", fosite.ErrServerError.WithCause(constable.Error("JWK must be of type ecdsa")) } return compose.NewOpenIDConnectECDSAStrategy(s.fositeConfig, key).GenerateIDToken(ctx, requester) diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go index 1b48f471..85a63501 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go @@ -9,6 +9,7 @@ import ( "crypto/elliptic" "crypto/rand" "crypto/rsa" + "errors" "net/url" "testing" @@ -42,7 +43,8 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { name string issuer string jwksProvider func(jwks.DynamicJWKSProvider) - wantError string + wantErrorType *fosite.RFC6749Error + wantErrorCause string wantSigningJWK *jose.JSONWebKey }{ { @@ -63,9 +65,10 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { }, }, { - name: "jwks provider does not contain signing key for issuer", - issuer: goodIssuer, - wantError: "no JWK found for issuer", + name: "jwks provider does not contain signing key for issuer", + issuer: goodIssuer, + wantErrorType: fosite.ErrTemporarilyUnavailable, + wantErrorCause: "no JWK found for issuer", }, { name: "jwks provider contains signing key of wrong type for issuer", @@ -80,7 +83,8 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { }, ) }, - wantError: "JWK must be of type ecdsa", + wantErrorType: fosite.ErrServerError, + wantErrorCause: "JWK must be of type ecdsa", }, } for _, test := range tests { @@ -111,8 +115,9 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { }, } idToken, err := s.GenerateIDToken(context.Background(), requester) - if test.wantError != "" { - require.EqualError(t, err, test.wantError) + if test.wantErrorType != nil { + require.True(t, errors.Is(err, test.wantErrorType)) + require.EqualError(t, err.(*fosite.RFC6749Error).Cause(), test.wantErrorCause) } else { require.NoError(t, err) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index a56a649c..7a3e2ffe 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -183,6 +183,15 @@ var ( "status_code": 400 } `) + + fositeTemporarilyUnavailableErrorBody = here.Doc(` + { + "error": "temporarily_unavailable", + "error_description": "The authorization server is currently unable to handle the request due to a temporary overloading or maintenance of the server", + "error_verbose": "The authorization server is currently unable to handle the request due to a temporary overloading or maintenance of the server", + "status_code": 503 + } + `) ) func TestTokenEndpoint(t *testing.T) { @@ -214,7 +223,18 @@ func TestTokenEndpoint(t *testing.T) { }, authCode string, ) - request func(r *http.Request, authCode string) + request func(r *http.Request, authCode string) + makeOathHelper func( + t *testing.T, + authRequest *http.Request, + store interface { + oauth2.TokenRevocationStorage + oauth2.CoreStorage + openid.OpenIDConnectRequestStorage + pkce.PKCERequestStorage + fosite.ClientManager + }, + ) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) wantStatus int wantBodyFields []string @@ -381,6 +401,12 @@ func TestTokenEndpoint(t *testing.T) { wantStatus: http.StatusBadRequest, wantExactBody: fositeWrongPKCEVerifierErrorBody, }, + { + name: "private signing key for JWTs has not yet been provided by the controller who is responsible for dynamically providing it", + makeOathHelper: makeOauthHelperWithNilPrivateJWTSigningKey, + wantStatus: http.StatusServiceUnavailable, + wantExactBody: fositeTemporarilyUnavailableErrorBody, + }, } for _, test := range tests { test := test @@ -393,8 +419,16 @@ func TestTokenEndpoint(t *testing.T) { client := fake.NewSimpleClientset() secrets := client.CoreV1().Secrets("some-namespace") + var oauthHelper fosite.OAuth2Provider + var authCode string + var jwtSigningKey *ecdsa.PrivateKey + oauthStore := oidc.NewKubeStorage(secrets) - oauthHelper, authCode, jwtSigningKey := makeHappyOauthHelper(t, authRequest, oauthStore) + if test.makeOathHelper != nil { + oauthHelper, authCode, jwtSigningKey = test.makeOathHelper(t, authRequest, oauthStore) + } else { + oauthHelper, authCode, jwtSigningKey = makeHappyOauthHelper(t, authRequest, oauthStore) + } if test.storage != nil { test.storage(t, oauthStore, authCode) @@ -595,7 +629,30 @@ func makeHappyOauthHelper( jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider) + authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) + return oauthHelper, authResponder.GetCode(), jwtSigningKey +} +func makeOauthHelperWithNilPrivateJWTSigningKey( + t *testing.T, + authRequest *http.Request, + store interface { + oauth2.TokenRevocationStorage + oauth2.CoreStorage + openid.OpenIDConnectRequestStorage + pkce.PKCERequestStorage + fosite.ClientManager + }, +) (fosite.OAuth2Provider, string, *ecdsa.PrivateKey) { + t.Helper() + + jwkProvider := jwks.NewDynamicJWKSProvider() // empty provider which contains no signing key for this issuer + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider) + authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) + return oauthHelper, authResponder.GetCode(), nil +} + +func simulateAuthEndpointHavingAlreadyRun(t *testing.T, authRequest *http.Request, oauthHelper fosite.OAuth2Provider) fosite.AuthorizeResponder { // Simulate the auth endpoint running so Fosite code will fill the store with realistic values. // // We only set the fields in the session that Fosite wants us to set. @@ -616,8 +673,7 @@ func makeHappyOauthHelper( } authResponder, err := oauthHelper.NewAuthorizeResponse(ctx, authRequester, session) require.NoError(t, err) - - return oauthHelper, authResponder.GetCode(), jwtSigningKey + return authResponder } func generateJWTSigningKeyAndJWKSProvider(t *testing.T, issuer string) (*ecdsa.PrivateKey, jwks.DynamicJWKSProvider) { diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index da8c10fd..65e90791 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -68,7 +68,7 @@ func TestSupervisorLogin(t *testing.T) { return proxyURL, nil }, }} - oidcHttpClientContext := oidc.ClientContext(ctx, httpClient) + oidcHTTPClientContext := oidc.ClientContext(ctx, httpClient) // Use the CA to issue a TLS server cert. t.Logf("issuing test certificate") @@ -111,7 +111,7 @@ func TestSupervisorLogin(t *testing.T) { // Perform OIDC discovery for our downstream. var discovery *oidc.Provider assert.Eventually(t, func() bool { - discovery, err = oidc.NewProvider(oidcHttpClientContext, downstream.Spec.Issuer) + discovery, err = oidc.NewProvider(oidcHTTPClientContext, downstream.Spec.Issuer) return err == nil }, 30*time.Second, 200*time.Millisecond) require.NoError(t, err) @@ -164,7 +164,7 @@ func TestSupervisorLogin(t *testing.T) { require.NotEmpty(t, authcode) // Call the token endpoint to get tokens. - tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHttpClientContext, authcode, pkceParam.Verifier()) + tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier()) require.NoError(t, err) // Verify the ID Token. From e1ae48f2e4a4bf4bc52b19fdaca5fb4a5472a089 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 7 Dec 2020 14:15:31 -0800 Subject: [PATCH 20/20] Discovery does not return `token_endpoint_auth_signing_alg_values_supported` `token_endpoint_auth_signing_alg_values_supported` is only related to private_key_jwt and client_secret_jwt client authentication methods at the token endpoint, which we do not support. See https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata for more details. Signed-off-by: Aram Price --- internal/oidc/discovery/discovery_handler.go | 12 +++++------- internal/oidc/discovery/discovery_handler_test.go | 5 ++--- test/integration/supervisor_discovery_test.go | 1 - 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/internal/oidc/discovery/discovery_handler.go b/internal/oidc/discovery/discovery_handler.go index c6d8f666..5eb6c481 100644 --- a/internal/oidc/discovery/discovery_handler.go +++ b/internal/oidc/discovery/discovery_handler.go @@ -31,10 +31,9 @@ type Metadata struct { // vvv Optional vvv - TokenEndpointAuthMethodsSupported []string `json:"token_endpoint_auth_methods_supported"` - TokenEndpointAuthSigningAlgoValuesSupported []string `json:"token_endpoint_auth_signing_alg_values_supported"` - ScopesSupported []string `json:"scopes_supported"` - ClaimsSupported []string `json:"claims_supported"` + TokenEndpointAuthMethodsSupported []string `json:"token_endpoint_auth_methods_supported"` + ScopesSupported []string `json:"scopes_supported"` + ClaimsSupported []string `json:"claims_supported"` // ^^^ Optional ^^^ } @@ -58,9 +57,8 @@ func NewHandler(issuerURL string) http.Handler { SubjectTypesSupported: []string{"public"}, IDTokenSigningAlgValuesSupported: []string{"ES256"}, TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, - TokenEndpointAuthSigningAlgoValuesSupported: []string{"RS256"}, - ScopesSupported: []string{"openid", "offline"}, - ClaimsSupported: []string{"groups"}, + ScopesSupported: []string{"openid", "offline"}, + ClaimsSupported: []string{"groups"}, } if err := json.NewEncoder(w).Encode(&oidcConfig); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/internal/oidc/discovery/discovery_handler_test.go b/internal/oidc/discovery/discovery_handler_test.go index b7d5f84a..f15c9a0c 100644 --- a/internal/oidc/discovery/discovery_handler_test.go +++ b/internal/oidc/discovery/discovery_handler_test.go @@ -43,9 +43,8 @@ func TestDiscovery(t *testing.T) { SubjectTypesSupported: []string{"public"}, IDTokenSigningAlgValuesSupported: []string{"ES256"}, TokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, - TokenEndpointAuthSigningAlgoValuesSupported: []string{"RS256"}, - ScopesSupported: []string{"openid", "offline"}, - ClaimsSupported: []string{"groups"}, + ScopesSupported: []string{"openid", "offline"}, + ClaimsSupported: []string{"groups"}, }, }, { diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 32945490..5e12fc12 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -472,7 +472,6 @@ func requireWellKnownEndpointIsWorking(t *testing.T, supervisorScheme, superviso "authorization_endpoint": "%s/oauth2/authorize", "token_endpoint": "%s/oauth2/token", "token_endpoint_auth_methods_supported": ["client_secret_basic"], - "token_endpoint_auth_signing_alg_values_supported": ["RS256"], "jwks_uri": "%s/jwks.json", "scopes_supported": ["openid", "offline"], "response_types_supported": ["code"],