From e42f5488fa4a39fc5a7756f84f003cd71ed88cf8 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 21 Jul 2022 09:26:00 -0700 Subject: [PATCH] More unit tests for dynamic clients - Add dynamic client unit tests for the upstream OIDC callback and POST login endpoints. - Enhance a few log statements to print the full fosite error messages into the logs where they were previously only printing the name of the error type. --- internal/oidc/auth/auth_handler_test.go | 20 +- internal/oidc/callback/callback_handler.go | 6 +- .../oidc/callback/callback_handler_test.go | 109 +++++- internal/oidc/login/post_login_handler.go | 3 +- .../oidc/login/post_login_handler_test.go | 345 ++++++++++++++---- internal/oidc/oidc.go | 2 +- .../testutil/oidctestutil/oidctestutil.go | 20 +- test/testlib/client.go | 4 +- 8 files changed, 400 insertions(+), 109 deletions(-) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 768ab10f..6f8e7598 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -25,7 +25,6 @@ import ( "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" - kubetesting "k8s.io/client-go/testing" "k8s.io/utils/pointer" supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" @@ -3088,7 +3087,7 @@ func TestAuthorizationEndpoint(t *testing.T) { // OIDC validations are checked in fosite after the OAuth authcode (and sometimes the OIDC session) // is stored, so it is possible with an LDAP upstream to store objects and then return an error to // the client anyway (which makes the stored objects useless, but oh well). - require.Len(t, filterActions(kubeClient.Actions()), test.wantUnnecessaryStoredRecords) + require.Len(t, oidctestutil.FilterClientSecretCreateActions(kubeClient.Actions()), test.wantUnnecessaryStoredRecords) case test.wantRedirectLocationRegexp != "": if test.wantDownstreamClientID == "" { test.wantDownstreamClientID = pinnipedCLIClientID // default assertion value when not provided by test case @@ -3301,20 +3300,3 @@ func requireEqualURLs(t *testing.T, actualURL string, expectedURL string, ignore } require.Equal(t, expectedLocationQuery, actualLocationQuery) } - -// filterActions ignores any reads made to get a storage secret corresponding to an OIDCClient, since these -// are normal actions when the request is using a dynamic client's client_id, and we don't need to make assertions -// about these Secrets since they are not related to session storage. -func filterActions(actions []kubetesting.Action) []kubetesting.Action { - filtered := make([]kubetesting.Action, 0, len(actions)) - for _, action := range actions { - if action.Matches("get", "secrets") { - getAction := action.(kubetesting.GetAction) - if strings.HasPrefix(getAction.GetName(), "pinniped-storage-oidc-client-secret-") { - continue // filter out OIDCClient's storage secret reads - } - } - filtered = append(filtered, action) // otherwise include the action - } - return filtered -} diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index db6ada1a..88b94392 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -48,7 +48,8 @@ func NewHandler( reconstitutedAuthRequest := &http.Request{Form: downstreamAuthParams} authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), reconstitutedAuthRequest) if err != nil { - plog.Error("error using state downstream auth params", err) + plog.Error("error using state downstream auth params", err, + "fositeErr", oidc.FositeErrorForLog(err)) return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } @@ -83,7 +84,8 @@ func NewHandler( authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { - plog.WarningErr("error while generating and saving authcode", err, "upstreamName", upstreamIDPConfig.GetName()) + plog.WarningErr("error while generating and saving authcode", err, + "upstreamName", upstreamIDPConfig.GetName(), "fositeErr", oidc.FositeErrorForLog(err)) return httperr.Wrap(http.StatusInternalServerError, "error while generating and saving authcode", err) } diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index dea3f2df..57dcfcd5 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -54,7 +54,9 @@ const ( downstreamIssuer = "https://my-downstream-issuer.com/path" downstreamRedirectURI = "http://127.0.0.1/callback" - downstreamClientID = "pinniped-cli" + downstreamPinnipedClientID = "pinniped-cli" + downstreamDynamicClientID = "client.oauth.pinniped.dev-test-name" + downstreamDynamicClientUID = "fake-client-uid" downstreamNonce = "some-nonce-value" downstreamPKCEChallenge = "some-challenge" downstreamPKCEChallengeMethod = "S256" @@ -70,14 +72,19 @@ var ( happyDownstreamRequestParamsQuery = url.Values{ "response_type": []string{"code"}, "scope": []string{strings.Join(happyDownstreamScopesRequested, " ")}, - "client_id": []string{downstreamClientID}, + "client_id": []string{downstreamPinnipedClientID}, "state": []string{happyDownstreamState}, "nonce": []string{downstreamNonce}, "code_challenge": []string{downstreamPKCEChallenge}, "code_challenge_method": []string{downstreamPKCEChallengeMethod}, "redirect_uri": []string{downstreamRedirectURI}, } - happyDownstreamRequestParams = happyDownstreamRequestParamsQuery.Encode() + happyDownstreamRequestParams = happyDownstreamRequestParamsQuery.Encode() + + happyDownstreamRequestParamsForDynamicClient = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"client_id": downstreamDynamicClientID}, + ).Encode() + happyDownstreamCustomSessionData = &psession.CustomSessionData{ ProviderUID: happyUpstreamIDPResourceUID, ProviderName: happyUpstreamIDPName, @@ -122,6 +129,7 @@ func TestCallbackEndpoint(t *testing.T) { happyCookieCodec.SetSerializer(securecookie.JSONEncoder{}) happyState := happyUpstreamStateParam().Build(t, happyStateCodec) + happyStateForDynamicClient := happyUpstreamStateParamForDynamicClient().Build(t, happyStateCodec) encodedIncomingCookieCSRFValue, err := happyCookieCodec.Encode("csrf", happyDownstreamCSRF) require.NoError(t, err) @@ -137,6 +145,13 @@ func TestCallbackEndpoint(t *testing.T) { // Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it happyDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+groups&state=` + happyDownstreamState + addFullyCapableDynamicClientAndSecretToKubeResources := func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) { + oidcClient, secret := testutil.FullyCapableOIDCClientAndStorageSecret(t, + "some-namespace", downstreamDynamicClientID, downstreamDynamicClientUID, downstreamRedirectURI, []string{testutil.HashedPassword1AtGoMinCost}) + require.NoError(t, supervisorClient.Tracker().Add(oidcClient)) + require.NoError(t, kubeClient.Tracker().Add(secret)) + } + tests := []struct { name string @@ -157,6 +172,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenGroups []string wantDownstreamRequestedScopes []string wantDownstreamNonce string + wantDownstreamClientID string wantDownstreamPKCEChallenge string wantDownstreamPKCEChallengeMethod string wantDownstreamCustomSessionData *psession.CustomSessionData @@ -185,6 +201,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -208,6 +225,32 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, + wantAuthcodeExchangeCall: &expectedAuthcodeExchange{ + performedByUpstreamName: happyUpstreamIDPName, + args: happyExchangeAndValidateTokensArgs, + }, + }, + { + name: "GET with good state and cookie and successful upstream token exchange returns 303 to downstream client callback with its state and code when using dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + method: http.MethodGet, + path: newRequestPath().WithState(happyStateForDynamicClient).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusSeeOther, + wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, + wantBody: "", + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamDynamicClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -231,6 +274,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamAccessTokenCustomSessionData, @@ -263,6 +307,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: []string{"openid"}, wantDownstreamGrantedScopes: []string{"openid"}, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -286,6 +331,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: &psession.CustomSessionData{ @@ -321,6 +367,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -346,6 +393,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -373,6 +421,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -401,6 +450,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -531,6 +581,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -556,6 +607,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -581,6 +633,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -714,6 +767,42 @@ func TestCallbackEndpoint(t *testing.T) { wantContentType: htmlContentType, wantBody: "Bad Request: error using state downstream auth params\n", }, + { + name: "state's downstream auth params have invalid client_id", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), + method: http.MethodGet, + path: newRequestPath().WithState( + happyUpstreamStateParam(). + WithAuthorizeRequestParams(shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, map[string]string{"client_id": "bogus"}).Encode()). + Build(t, happyStateCodec), + ).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantContentType: htmlContentType, + wantBody: "Bad Request: error using state downstream auth params\n", + }, + { + name: "dynamic clients do not allow response_mode=form_post", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + method: http.MethodGet, + path: newRequestPath().WithState( + happyUpstreamStateParam().WithAuthorizeRequestParams( + shallowCopyAndModifyQuery( + happyDownstreamRequestParamsQuery, + map[string]string{ + "client_id": downstreamDynamicClientID, + "response_mode": "form_post", + "scope": "openid", + }, + ).Encode(), + ).Build(t, happyStateCodec), + ).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantContentType: htmlContentType, + wantBody: "Bad Request: error using state downstream auth params\n", + }, { name: "state's downstream auth params does not contain openid scope", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().Build()), @@ -733,6 +822,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamGrantedScopes: []string{"groups"}, wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -759,6 +849,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamRequestedScopes: []string{"profile", "email"}, wantDownstreamGrantedScopes: []string{}, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -786,6 +877,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamGrantedScopes: []string{"openid", "offline_access", "groups"}, wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -884,6 +976,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamIDTokenGroups: []string{}, wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: happyDownstreamCustomSessionData, @@ -1139,7 +1232,7 @@ func TestCallbackEndpoint(t *testing.T) { test.wantDownstreamPKCEChallenge, test.wantDownstreamPKCEChallengeMethod, test.wantDownstreamNonce, - downstreamClientID, + test.wantDownstreamClientID, downstreamRedirectURI, test.wantDownstreamCustomSessionData, ) @@ -1166,7 +1259,7 @@ func TestCallbackEndpoint(t *testing.T) { test.wantDownstreamPKCEChallenge, test.wantDownstreamPKCEChallengeMethod, test.wantDownstreamNonce, - downstreamClientID, + test.wantDownstreamClientID, downstreamRedirectURI, test.wantDownstreamCustomSessionData, ) @@ -1237,6 +1330,12 @@ func happyUpstreamStateParam() *oidctestutil.UpstreamStateParamBuilder { } } +func happyUpstreamStateParamForDynamicClient() *oidctestutil.UpstreamStateParamBuilder { + p := happyUpstreamStateParam() + p.P = happyDownstreamRequestParamsForDynamicClient + return p +} + func happyUpstream() *oidctestutil.TestUpstreamOIDCIdentityProviderBuilder { return oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). WithName(happyUpstreamIDPName). diff --git a/internal/oidc/login/post_login_handler.go b/internal/oidc/login/post_login_handler.go index 85ccbc25..4c214452 100644 --- a/internal/oidc/login/post_login_handler.go +++ b/internal/oidc/login/post_login_handler.go @@ -41,7 +41,8 @@ func NewPostHandler(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvider if err != nil { // This shouldn't really happen because the authorization endpoint has already validated these params // by calling NewAuthorizeRequest() itself. - plog.Error("error using state downstream auth params", err) + plog.Error("error using state downstream auth params", err, + "fositeErr", oidc.FositeErrorForLog(err)) return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } diff --git a/internal/oidc/login/post_login_handler_test.go b/internal/oidc/login/post_login_handler_test.go index 7e8cffef..80931ee9 100644 --- a/internal/oidc/login/post_login_handler_test.go +++ b/internal/oidc/login/post_login_handler_test.go @@ -38,7 +38,9 @@ func TestPostLoginEndpoint(t *testing.T) { downstreamIssuer = "https://my-downstream-issuer.com/path" downstreamRedirectURI = "http://127.0.0.1/callback" - downstreamClientID = "pinniped-cli" + downstreamPinnipedCLIClientID = "pinniped-cli" + downstreamDynamicClientID = "client.oauth.pinniped.dev-test-name" + downstreamDynamicClientUID = "fake-client-uid" happyDownstreamState = "8b-state" downstreamNonce = "some-nonce-value" downstreamPKCEChallenge = "some-challenge" @@ -90,7 +92,7 @@ func TestPostLoginEndpoint(t *testing.T) { happyDownstreamRequestParamsQuery := url.Values{ "response_type": []string{"code"}, "scope": []string{strings.Join(happyDownstreamScopesRequested, " ")}, - "client_id": []string{downstreamClientID}, + "client_id": []string{downstreamPinnipedCLIClientID}, "state": []string{happyDownstreamState}, "nonce": []string{downstreamNonce}, "code_challenge": []string{downstreamPKCEChallenge}, @@ -99,14 +101,10 @@ func TestPostLoginEndpoint(t *testing.T) { } happyDownstreamRequestParams := happyDownstreamRequestParamsQuery.Encode() - copyOfHappyDownstreamRequestParamsQuery := func() url.Values { - params := url.Values{} - for k, v := range happyDownstreamRequestParamsQuery { - params[k] = make([]string, len(v)) - copy(params[k], v) - } - return params - } + happyDownstreamRequestParamsQueryForDynamicClient := shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"client_id": downstreamDynamicClientID}, + ) + happyDownstreamRequestParamsForDynamicClient := happyDownstreamRequestParamsQueryForDynamicClient.Encode() happyLDAPDecodedState := &oidc.UpstreamStateParamData{ AuthParams: happyDownstreamRequestParams, @@ -124,15 +122,20 @@ func TestPostLoginEndpoint(t *testing.T) { return ©OfHappyLDAPDecodedState } - happyActiveDirectoryDecodedState := &oidc.UpstreamStateParamData{ - AuthParams: happyDownstreamRequestParams, - UpstreamName: activeDirectoryUpstreamName, - UpstreamType: activeDirectoryUpstreamType, - Nonce: happyDownstreamNonce, - CSRFToken: happyDownstreamCSRF, - PKCECode: happyDownstreamPKCE, - FormatVersion: happyDownstreamStateVersion, - } + happyLDAPDecodedStateForDynamicClient := modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = happyDownstreamRequestParamsForDynamicClient + }) + + happyActiveDirectoryDecodedState := modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.UpstreamName = activeDirectoryUpstreamName + data.UpstreamType = activeDirectoryUpstreamType + }) + + happyActiveDirectoryDecodedStateForDynamicClient := modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = happyDownstreamRequestParamsForDynamicClient + data.UpstreamName = activeDirectoryUpstreamName + data.UpstreamType = activeDirectoryUpstreamType + }) happyLDAPUsername := "some-ldap-user" happyLDAPUsernameFromAuthenticator := "some-mapped-ldap-username" @@ -232,6 +235,13 @@ func TestPostLoginEndpoint(t *testing.T) { return urlToReturn } + addFullyCapableDynamicClientAndSecretToKubeResources := func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) { + oidcClient, secret := testutil.FullyCapableOIDCClientAndStorageSecret(t, + "some-namespace", downstreamDynamicClientID, downstreamDynamicClientUID, downstreamRedirectURI, []string{testutil.HashedPassword1AtGoMinCost}) + require.NoError(t, supervisorClient.Tracker().Add(oidcClient)) + require.NoError(t, kubeClient.Tracker().Add(secret)) + } + tests := []struct { name string idps *oidctestutil.UpstreamIDPListerBuilder @@ -262,6 +272,7 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamPKCEChallenge string wantDownstreamPKCEChallengeMethod string wantDownstreamNonce string + wantDownstreamClient string wantDownstreamCustomSessionData *psession.CustomSessionData // Authorization requests for either a successful OIDC upstream or for an error with any upstream @@ -289,6 +300,31 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamRedirectURI: downstreamRedirectURI, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamPinnipedCLIClientID, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, + }, + { + name: "happy LDAP login with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder(). + WithLDAP(&upstreamLDAPIdentityProvider). // should pick this one + WithActiveDirectory(&erroringUpstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: happyLDAPDecodedStateForDynamicClient, + formParams: happyUsernamePasswordFormParams, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyString: "", + wantRedirectLocationRegexp: happyAuthcodeDownstreamRedirectLocationRegexp, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, + wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, + wantDownstreamIDTokenGroups: happyLDAPGroups, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamRedirectURI: downstreamRedirectURI, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamDynamicClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, @@ -311,6 +347,31 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamRedirectURI: downstreamRedirectURI, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamPinnipedCLIClientID, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyActiveDirectoryUpstreamCustomSession, + }, + { + name: "happy AD login with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder(). + WithLDAP(&erroringUpstreamLDAPIdentityProvider). + WithActiveDirectory(&upstreamActiveDirectoryIdentityProvider), // should pick this one + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: happyActiveDirectoryDecodedStateForDynamicClient, + formParams: happyUsernamePasswordFormParams, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyString: "", + wantRedirectLocationRegexp: happyAuthcodeDownstreamRedirectLocationRegexp, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, + wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, + wantDownstreamIDTokenGroups: happyLDAPGroups, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamRedirectURI: downstreamRedirectURI, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamDynamicClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyActiveDirectoryUpstreamCustomSession, @@ -319,9 +380,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "happy LDAP login when downstream response_mode=form_post returns 200 with HTML+JS form", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["response_mode"] = []string{"form_post"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"response_mode": "form_post"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusOK, @@ -335,6 +396,7 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamRedirectURI: downstreamRedirectURI, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamPinnipedCLIClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, @@ -343,9 +405,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "happy LDAP login when downstream redirect uri matches what is configured for client except for the port number", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["redirect_uri"] = []string{"http://127.0.0.1:4242/callback"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"redirect_uri": "http://127.0.0.1:4242/callback"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -359,6 +421,33 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamRedirectURI: "http://127.0.0.1:4242/callback", wantDownstreamGrantedScopes: happyDownstreamScopesGranted, wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamPinnipedCLIClientID, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, + }, + { + name: "happy LDAP login when downstream redirect uri matches what is configured for client except for the port number with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient, + map[string]string{"redirect_uri": "http://127.0.0.1:4242/callback"}, + ).Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyString: "", + wantRedirectLocationRegexp: "http://127.0.0.1:4242/callback" + `\?code=([^&]+)&scope=openid\+groups&state=` + happyDownstreamState, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, + wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, + wantDownstreamIDTokenGroups: happyLDAPGroups, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamRedirectURI: "http://127.0.0.1:4242/callback", + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamDynamicClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, @@ -367,9 +456,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "happy LDAP login when there are additional allowed downstream requested scopes", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["scope"] = []string{"openid offline_access pinniped:request-audience"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"scope": "openid offline_access pinniped:request-audience"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -383,6 +472,33 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamRedirectURI: downstreamRedirectURI, wantDownstreamGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamPinnipedCLIClientID, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, + }, + { + name: "happy LDAP login when there are additional allowed downstream requested scopes with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient, + map[string]string{"scope": "openid offline_access pinniped:request-audience"}, + ).Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyString: "", + wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+offline_access\+pinniped%3Arequest-audience&state=` + happyDownstreamState, + wantDownstreamIDTokenSubject: upstreamLDAPURL + "&sub=" + happyLDAPUID, + wantDownstreamIDTokenUsername: happyLDAPUsernameFromAuthenticator, + wantDownstreamIDTokenGroups: happyLDAPGroups, + wantDownstreamRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, + wantDownstreamRedirectURI: downstreamRedirectURI, + wantDownstreamGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, + wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamDynamicClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, @@ -391,11 +507,13 @@ func TestPostLoginEndpoint(t *testing.T) { name: "happy LDAP when downstream OIDC validations are skipped because the openid scope was not requested", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["scope"] = []string{"email"} - // The following prompt value is illegal when openid is requested, but note that openid is not requested. - query["prompt"] = []string{"none login"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{ + "scope": "email", + // The following prompt value is illegal when openid is requested, but note that openid is not requested. + "prompt": "none login", + }, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -409,6 +527,7 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamRedirectURI: downstreamRedirectURI, wantDownstreamGrantedScopes: []string{}, // no scopes granted wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamPinnipedCLIClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, @@ -419,9 +538,9 @@ func TestPostLoginEndpoint(t *testing.T) { WithLDAP(&upstreamLDAPIdentityProvider). // should pick this one WithActiveDirectory(&erroringUpstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["scope"] = []string{"openid"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"scope": "openid"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -434,6 +553,7 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamRedirectURI: downstreamRedirectURI, wantDownstreamGrantedScopes: []string{"openid"}, wantDownstreamNonce: downstreamNonce, + wantDownstreamClient: downstreamPinnipedCLIClientID, wantDownstreamPKCEChallenge: downstreamPKCEChallenge, wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, @@ -502,9 +622,21 @@ func TestPostLoginEndpoint(t *testing.T) { name: "downstream redirect uri does not match what is configured for client", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["redirect_uri"] = []string{"http://127.0.0.1/wrong_callback"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"redirect_uri": "http://127.0.0.1/wrong_callback"}, + ).Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantErr: "error using state downstream auth params", + }, + { + name: "downstream redirect uri does not match what is configured for client with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient, + map[string]string{"redirect_uri": "http://127.0.0.1/wrong_callback"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantErr: "error using state downstream auth params", @@ -513,9 +645,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "downstream client does not exist", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["client_id"] = []string{"wrong_client_id"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"client_id": "wrong_client_id"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantErr: "error using state downstream auth params", @@ -524,9 +656,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "downstream client is missing", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - delete(query, "client_id") - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"client_id": ""}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantErr: "error using state downstream auth params", @@ -535,9 +667,21 @@ func TestPostLoginEndpoint(t *testing.T) { name: "response type is unsupported", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["response_type"] = []string{"unsupported"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"response_type": "unsupported"}, + ).Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantErr: "error using state downstream auth params", + }, + { + name: "response type form_post is unsupported for dynamic clients", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient, + map[string]string{"response_type": "form_post"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantErr: "error using state downstream auth params", @@ -546,9 +690,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "response type is missing", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - delete(query, "response_type") - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"response_type": ""}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantErr: "error using state downstream auth params", @@ -557,9 +701,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "PKCE code_challenge is missing", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - delete(query, "code_challenge") - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"code_challenge": ""}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -572,9 +716,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "PKCE code_challenge_method is invalid", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["code_challenge_method"] = []string{"this-is-not-a-valid-pkce-alg"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"code_challenge_method": "this-is-not-a-valid-pkce-alg"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -587,9 +731,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "PKCE code_challenge_method is `plain`", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["code_challenge_method"] = []string{"plain"} // plain is not allowed - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"code_challenge_method": "plain"}, // plain is not allowed + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -602,9 +746,25 @@ func TestPostLoginEndpoint(t *testing.T) { name: "PKCE code_challenge_method is missing", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - delete(query, "code_challenge_method") - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"code_challenge_method": ""}, + ).Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyString: "", + wantRedirectLocationString: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeMethodErrorQuery), + wantUnnecessaryStoredRecords: 2, // fosite already stored the authcode and oidc session before it noticed the error + }, + { + name: "PKCE code_challenge_method is missing with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient, + map[string]string{"code_challenge_method": ""}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -617,9 +777,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "prompt param is not allowed to have none and another legal value at the same time", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["prompt"] = []string{"none login"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"prompt": "none login"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantStatus: http.StatusSeeOther, @@ -632,9 +792,9 @@ func TestPostLoginEndpoint(t *testing.T) { name: "downstream state does not have enough entropy", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["state"] = []string{"short"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"state": "short"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantErr: "error using state downstream auth params", @@ -643,9 +803,21 @@ func TestPostLoginEndpoint(t *testing.T) { name: "downstream scopes do not match what is configured for client", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { - query := copyOfHappyDownstreamRequestParamsQuery() - query["scope"] = []string{"openid offline_access pinniped:request-audience scope_not_allowed"} - data.AuthParams = query.Encode() + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQuery, + map[string]string{"scope": "openid offline_access pinniped:request-audience scope_not_allowed"}, + ).Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantErr: "error using state downstream auth params", + }, + { + name: "downstream scopes do not match what is configured for client with dynamic client", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&upstreamLDAPIdentityProvider), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient, + map[string]string{"scope": "openid offline_access pinniped:request-audience scope_not_allowed"}, + ).Encode() }), formParams: happyUsernamePasswordFormParams, wantErr: "error using state downstream auth params", @@ -677,8 +849,8 @@ func TestPostLoginEndpoint(t *testing.T) { secretsClient := kubeClient.CoreV1().Secrets("some-namespace") oidcClientsClient := supervisorClient.ConfigV1alpha1().OIDCClients("some-namespace") - if test.kubeResources != nil { - test.kubeResources(t, supervisorClient, kubeClient) + if tt.kubeResources != nil { + tt.kubeResources(t, supervisorClient, kubeClient) } // Configure fosite the same way that the production code would. @@ -704,7 +876,7 @@ func TestPostLoginEndpoint(t *testing.T) { err := subject(rsp, req, happyEncodedUpstreamState, tt.decodedState) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) - require.Empty(t, kubeClient.Actions()) + require.Empty(t, oidctestutil.FilterClientSecretCreateActions(kubeClient.Actions())) return // the http response doesn't matter when the function returns an error, because the caller should handle the error } // Otherwise, expect no error. @@ -735,7 +907,7 @@ func TestPostLoginEndpoint(t *testing.T) { tt.wantDownstreamPKCEChallenge, tt.wantDownstreamPKCEChallengeMethod, tt.wantDownstreamNonce, - downstreamClientID, + tt.wantDownstreamClient, tt.wantDownstreamRedirectURI, tt.wantDownstreamCustomSessionData, ) @@ -745,12 +917,12 @@ func TestPostLoginEndpoint(t *testing.T) { expectedLocation := downstreamIssuer + oidc.PinnipedLoginPath + "?err=" + tt.wantRedirectToLoginPageError + "&state=" + happyEncodedUpstreamState require.Equal(t, expectedLocation, actualLocation) - require.Len(t, kubeClient.Actions(), tt.wantUnnecessaryStoredRecords) + require.Len(t, oidctestutil.FilterClientSecretCreateActions(kubeClient.Actions()), tt.wantUnnecessaryStoredRecords) case tt.wantRedirectLocationString != "": // Expecting an error redirect to the client. require.Equal(t, tt.wantBodyString, rsp.Body.String()) require.Equal(t, tt.wantRedirectLocationString, actualLocation) - require.Len(t, kubeClient.Actions(), tt.wantUnnecessaryStoredRecords) + require.Len(t, oidctestutil.FilterClientSecretCreateActions(kubeClient.Actions()), tt.wantUnnecessaryStoredRecords) case tt.wantBodyFormResponseRegexp != "": // Expecting the body of the response to be a html page with a form (for "response_mode=form_post"). _, hasLocationHeader := rsp.Header()["Location"] @@ -770,7 +942,7 @@ func TestPostLoginEndpoint(t *testing.T) { tt.wantDownstreamPKCEChallenge, tt.wantDownstreamPKCEChallengeMethod, tt.wantDownstreamNonce, - downstreamClientID, + tt.wantDownstreamClient, tt.wantDownstreamRedirectURI, tt.wantDownstreamCustomSessionData, ) @@ -781,3 +953,18 @@ func TestPostLoginEndpoint(t *testing.T) { }) } } + +func shallowCopyAndModifyQuery(query url.Values, modifications map[string]string) url.Values { + copied := url.Values{} + for key, value := range query { + copied[key] = value + } + for key, value := range modifications { + if value == "" { + copied.Del(key) + } else { + copied[key] = []string{value} + } + } + return copied +} diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index f78eaef5..11218e58 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -457,7 +457,7 @@ func PerformAuthcodeRedirect( ) { authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { - plog.WarningErr("error while generating and saving authcode", err) + plog.WarningErr("error while generating and saving authcode", err, "fositeErr", FositeErrorForLog(err)) WriteAuthorizeError(w, oauthHelper, authorizeRequester, err, isBrowserless) return } diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index fb1e8a7a..23fcc821 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + kubetesting "k8s.io/client-go/testing" "k8s.io/utils/strings/slices" "go.pinniped.dev/internal/authenticators" @@ -954,7 +955,7 @@ func RequireAuthCodeRegexpMatch( if includesOpenIDScope(wantDownstreamGrantedScopes) { expectedNumberOfCreatedSecrets++ } - require.Len(t, kubeClient.Actions(), expectedNumberOfCreatedSecrets) + require.Len(t, FilterClientSecretCreateActions(kubeClient.Actions()), expectedNumberOfCreatedSecrets) // One authcode should have been stored. testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secretsClient, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1) @@ -1164,3 +1165,20 @@ func castStoredAuthorizeRequest(t *testing.T, storedAuthorizeRequest fosite.Requ return storedRequest, storedSession } + +// FilterClientSecretCreateActions ignores any reads made to get a storage secret corresponding to an OIDCClient, since these +// are normal actions when the request is using a dynamic client's client_id, and we don't need to make assertions +// about these Secrets since they are not related to session storage. +func FilterClientSecretCreateActions(actions []kubetesting.Action) []kubetesting.Action { + filtered := make([]kubetesting.Action, 0, len(actions)) + for _, action := range actions { + if action.Matches("get", "secrets") { + getAction := action.(kubetesting.GetAction) + if strings.HasPrefix(getAction.GetName(), "pinniped-storage-oidc-client-secret-") { + continue // filter out OIDCClient's storage secret reads + } + } + filtered = append(filtered, action) // otherwise include the action + } + return filtered +} diff --git a/test/testlib/client.go b/test/testlib/client.go index 2c514f7d..efad330b 100644 --- a/test/testlib/client.go +++ b/test/testlib/client.go @@ -15,6 +15,8 @@ import ( "testing" "time" + "go.pinniped.dev/internal/oidc/oidcclientvalidator" + "github.com/stretchr/testify/require" "golang.org/x/crypto/bcrypt" authorizationv1 "k8s.io/api/authorization/v1" @@ -435,7 +437,7 @@ func createOIDCClientSecret(t *testing.T, forOIDCClient *configv1alpha1.OIDCClie _, err := io.ReadFull(rand.Reader, buf[:]) require.NoError(t, err) randomSecret := hex.EncodeToString(buf[:]) - hashedRandomSecret, err := bcrypt.GenerateFromPassword([]byte(randomSecret), 15) + hashedRandomSecret, err := bcrypt.GenerateFromPassword([]byte(randomSecret), oidcclientvalidator.DefaultMinBcryptCost) require.NoError(t, err) created, err := kubeClient.CoreV1().Secrets(env.SupervisorNamespace).Create(ctx, &corev1.Secret{