From 19281313ddfafb952a81f9834b70ab2c8f3ada35 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Fri, 22 Oct 2021 13:57:30 -0700 Subject: [PATCH 01/10] Basic upstream LDAP/AD refresh This stores the user DN in the session data upon login and checks that the entry still exists upon refresh. It doesn't check anything else about the entry yet. --- .../authorizationcode/authorizationcode.go | 12 +- internal/oidc/auth/auth_handler.go | 28 ++ internal/oidc/auth/auth_handler_test.go | 10 + .../provider/dynamic_upstream_idp_provider.go | 3 + internal/oidc/token/token_handler.go | 57 ++- internal/oidc/token/token_handler_test.go | 395 +++++++++++++++++- internal/psession/pinniped_session.go | 14 + .../testutil/oidctestutil/oidctestutil.go | 77 +++- internal/upstreamldap/upstreamldap.go | 77 +++- internal/upstreamldap/upstreamldap_test.go | 147 +++++++ test/integration/ldap_client_test.go | 40 +- test/integration/supervisor_login_test.go | 60 ++- 12 files changed, 842 insertions(+), 78 deletions(-) diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index b259e406..3418f672 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -328,14 +328,22 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ "providerType": "闣ʬ橳(ý綃ʃʚƟ覣k眐4Ĉt", "oidc": { "upstreamRefreshToken": "嵽痊w©Ź榨Q|ôɵt毇妬" + }, + "ldap": { + "userDN": "6鉢緋uƴŤȱʀļÂ?墖\u003cƬb獭潜Ʃ饾" + }, + "activedirectory": { + "userDN": "|鬌R蜚蠣麹概÷驣7Ʀ澉1æɽ誮rʨ鷞" } } }, "requestedAudience": [ - "6鉢緋uƴŤȱʀļÂ?墖\u003cƬb獭潜Ʃ饾" + "ŚB碠k9" ], "grantedAudience": [ - "|鬌R蜚蠣麹概÷驣7Ʀ澉1æɽ誮rʨ鷞" + "ʘ赱", + "ď逳鞪?3)藵睋邔\u0026Ű惫蜀Ģ¡圔", + "墀jMʥ" ] }, "version": "2" diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 4c457faf..d22f9ad7 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -112,6 +112,10 @@ func handleAuthRequestForLDAPUpstream( subject := downstreamSubjectFromUpstreamLDAP(ldapUpstream, authenticateResponse) username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() + dn := userDNFromAuthenticatedResponse(authenticateResponse) + if dn == "" { + return httperr.New(http.StatusInternalServerError, "unexpected error during upstream authentication") + } customSessionData := &psession.CustomSessionData{ ProviderUID: ldapUpstream.GetResourceUID(), @@ -119,6 +123,17 @@ func handleAuthRequestForLDAPUpstream( ProviderType: idpType, } + if idpType == psession.ProviderTypeLDAP { + customSessionData.LDAP = &psession.LDAPSessionData{ + UserDN: dn, + } + } + if idpType == psession.ProviderTypeActiveDirectory { + customSessionData.ActiveDirectory = &psession.ActiveDirectorySessionData{ + UserDN: dn, + } + } + return makeDownstreamSessionAndReturnAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, subject, username, groups, customSessionData) } @@ -477,3 +492,16 @@ func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentit ldapURL.RawQuery = q.Encode() return ldapURL.String() } + +func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string { + // These errors shouldn't happen, but do some error checking anyway so it doesn't panic + extra := authenticatedResponse.User.GetExtra() + if len(extra) == 0 { + return "" + } + dnSlice := extra["userDN"] + if len(dnSlice) != 1 { + return "" + } + return dnSlice[0] +} diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 7f61c103..39264037 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -267,6 +267,7 @@ func TestAuthorizationEndpoint(t *testing.T) { happyLDAPUsernameFromAuthenticator := "some-mapped-ldap-username" happyLDAPPassword := "some-ldap-password" //nolint:gosec happyLDAPUID := "some-ldap-uid" + happyLDAPUserDN := "cn=foo,dn=bar" happyLDAPGroups := []string{"group1", "group2", "group3"} parsedUpstreamLDAPURL, err := url.Parse(upstreamLDAPURL) @@ -282,6 +283,7 @@ func TestAuthorizationEndpoint(t *testing.T) { Name: happyLDAPUsernameFromAuthenticator, UID: happyLDAPUID, Groups: happyLDAPGroups, + Extra: map[string][]string{"userDN": {happyLDAPUserDN}}, }, }, true, nil } @@ -438,6 +440,10 @@ func TestAuthorizationEndpoint(t *testing.T) { ProviderName: activeDirectoryUpstreamName, ProviderType: psession.ProviderTypeActiveDirectory, OIDC: nil, + LDAP: nil, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: happyLDAPUserDN, + }, } expectedHappyLDAPUpstreamCustomSession := &psession.CustomSessionData{ @@ -445,6 +451,10 @@ func TestAuthorizationEndpoint(t *testing.T) { ProviderName: ldapUpstreamName, ProviderType: psession.ProviderTypeLDAP, OIDC: nil, + LDAP: &psession.LDAPSessionData{ + UserDN: happyLDAPUserDN, + }, + ActiveDirectory: nil, } expectedHappyOIDCPasswordGrantCustomSession := &psession.CustomSessionData{ diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 88710f00..c5f2b7b1 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -88,6 +88,9 @@ type UpstreamLDAPIdentityProviderI interface { // UserAuthenticator adds an interface method for performing user authentication against the upstream LDAP provider. authenticators.UserAuthenticator + + // PerformRefresh performs a refresh against the upstream LDAP identity provider + PerformRefresh(ctx context.Context, userDN string) error } type DynamicUpstreamIDPProvider interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 30956524..dcec411b 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -89,14 +89,12 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, case psession.ProviderTypeOIDC: return upstreamOIDCRefresh(ctx, customSessionData, providerCache) case psession.ProviderTypeLDAP: - // upstream refresh not yet implemented for LDAP, so do nothing + return upstreamLDAPRefresh(ctx, customSessionData, providerCache) case psession.ProviderTypeActiveDirectory: - // upstream refresh not yet implemented for AD, so do nothing + return upstreamLDAPRefresh(ctx, customSessionData, providerCache) default: return errorsx.WithStack(errMissingUpstreamSessionInternalError) } - - return nil } func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister) error { @@ -165,3 +163,54 @@ func findOIDCProviderByNameAndValidateUID( return nil, errorsx.WithStack(errUpstreamRefreshError. WithHintf("Provider %q of type %q from upstream session data was not found.", s.ProviderName, s.ProviderType)) } + +func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister) error { + plog.Warning("refreshing upstream") + // if you have neither a valid ldap session config nor a valid active directory session config + if (s.LDAP == nil || s.LDAP.UserDN == "") && (s.ActiveDirectory == nil || s.ActiveDirectory.UserDN == "") { + return errorsx.WithStack(errMissingUpstreamSessionInternalError) + } + plog.Warning("going to find provider", "provider", s.ProviderName) + + // get ldap/ad provider out of cache + p, dn, _ := findLDAPProviderByNameAndValidateUID(s, providerCache) + // TODO error checking + // run PerformRefresh + err := p.PerformRefresh(ctx, dn) + if err != nil { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed using provider %q of type %q.", + s.ProviderName, s.ProviderType).WithWrap(err)) + } + + return nil +} + +func findLDAPProviderByNameAndValidateUID( + s *psession.CustomSessionData, + providerCache oidc.UpstreamIdentityProvidersLister, +) (provider.UpstreamLDAPIdentityProviderI, string, error) { + var providers []provider.UpstreamLDAPIdentityProviderI + var dn string + if s.ProviderType == psession.ProviderTypeLDAP { + providers = providerCache.GetLDAPIdentityProviders() + dn = s.LDAP.UserDN + } else if s.ProviderType == psession.ProviderTypeActiveDirectory { + providers = providerCache.GetActiveDirectoryIdentityProviders() + dn = s.ActiveDirectory.UserDN + } + + for _, p := range providers { + if p.GetName() == s.ProviderName { + if p.GetResourceUID() != s.ProviderUID { + return nil, "", errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Provider %q of type %q from upstream session data has changed its resource UID since authentication.", + s.ProviderName, s.ProviderType)) + } + return p, dn, nil + } + } + + return nil, "", errorsx.WithStack(errUpstreamRefreshError. + WithHintf("Provider %q of type %q from upstream session data was not found.", s.ProviderName, s.ProviderType)) +} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 3b6be1ff..c819847e 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -232,7 +232,7 @@ type tokenEndpointResponseExpectedValues struct { wantErrorResponseBody string wantRequestedScopes []string wantGrantedScopes []string - wantUpstreamOIDCRefreshCall *expectedUpstreamRefresh + wantUpstreamRefreshCall *expectedUpstreamRefresh wantUpstreamOIDCValidateTokenCall *expectedUpstreamValidateTokens wantCustomSessionDataStored *psession.CustomSessionData } @@ -879,8 +879,20 @@ func TestRefreshGrant(t *testing.T) { oidcUpstreamInitialRefreshToken = "initial-upstream-refresh-token" oidcUpstreamRefreshedIDToken = "fake-refreshed-id-token" oidcUpstreamRefreshedRefreshToken = "fake-refreshed-refresh-token" + + ldapUpstreamName = "some-ldap-idp" + ldapUpstreamResourceUID = "ldap-resource-uid" + ldapUpstreamType = "ldap" + ldapUpstreamDN = "some-ldap-user-dn" + + activeDirectoryUpstreamName = "some-ad-idp" + activeDirectoryUpstreamResourceUID = "ad-resource-uid" + activeDirectoryUpstreamType = "activedirectory" + activeDirectoryUpstreamDN = "some-ad-user-dn" ) + ldapUpstreamURL, _ := url.Parse("some-url") + // The below values are funcs so every test can have its own copy of the objects, to avoid data races // in these parallel tests. @@ -907,7 +919,7 @@ func TestRefreshGrant(t *testing.T) { return sessionData } - happyUpstreamRefreshCall := func() *expectedUpstreamRefresh { + happyOIDCUpstreamRefreshCall := func() *expectedUpstreamRefresh { return &expectedUpstreamRefresh{ performedByUpstreamName: oidcUpstreamName, args: &oidctestutil.PerformRefreshArgs{ @@ -917,6 +929,26 @@ func TestRefreshGrant(t *testing.T) { } } + happyLDAPUpstreamRefreshCall := func() *expectedUpstreamRefresh { + return &expectedUpstreamRefresh{ + performedByUpstreamName: ldapUpstreamName, + args: &oidctestutil.PerformRefreshArgs{ + Ctx: nil, + DN: ldapUpstreamDN, + }, + } + } + + happyActiveDirectoryUpstreamRefreshCall := func() *expectedUpstreamRefresh { + return &expectedUpstreamRefresh{ + performedByUpstreamName: activeDirectoryUpstreamName, + args: &oidctestutil.PerformRefreshArgs{ + Ctx: nil, + DN: activeDirectoryUpstreamDN, + }, + } + } + happyUpstreamValidateTokenCall := func(expectedTokens *oauth2.Token) *expectedUpstreamValidateTokens { return &expectedUpstreamValidateTokens{ performedByUpstreamName: oidcUpstreamName, @@ -944,7 +976,7 @@ func TestRefreshGrant(t *testing.T) { // same as the same values as the authcode exchange case. want := happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(wantCustomSessionDataStored) // Should always try to perform an upstream refresh. - want.wantUpstreamOIDCRefreshCall = happyUpstreamRefreshCall() + want.wantUpstreamRefreshCall = happyOIDCUpstreamRefreshCall() // Should only try to ValidateToken when there was an id token returned by the upstream refresh. if expectToValidateToken != nil { want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken) @@ -952,6 +984,18 @@ func TestRefreshGrant(t *testing.T) { return want } + happyRefreshTokenResponseForLDAP := func(wantCustomSessionDataStored *psession.CustomSessionData) tokenEndpointResponseExpectedValues { + want := happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(wantCustomSessionDataStored) + want.wantUpstreamRefreshCall = happyLDAPUpstreamRefreshCall() + return want + } + + happyRefreshTokenResponseForActiveDirectory := func(wantCustomSessionDataStored *psession.CustomSessionData) tokenEndpointResponseExpectedValues { + want := happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(wantCustomSessionDataStored) + want.wantUpstreamRefreshCall = happyActiveDirectoryUpstreamRefreshCall() + return want + } + refreshedUpstreamTokensWithRefreshTokenWithoutIDToken := func() *oauth2.Token { return &oauth2.Token{ AccessToken: "fake-refreshed-access-token", @@ -973,10 +1017,11 @@ func TestRefreshGrant(t *testing.T) { } tests := []struct { - name string - idps *oidctestutil.UpstreamIDPListerBuilder - authcodeExchange authcodeExchangeInputs - refreshRequest refreshRequestInputs + name string + idps *oidctestutil.UpstreamIDPListerBuilder + authcodeExchange authcodeExchangeInputs + authEndpointInitialSessionData *psession.CustomSessionData + refreshRequest refreshRequestInputs }{ { name: "happy path refresh grant with openid scope granted (id token returned)", @@ -1015,7 +1060,7 @@ func TestRefreshGrant(t *testing.T) { wantSuccessBodyFields: []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"offline_access"}, wantGrantedScopes: []string{"offline_access"}, - wantUpstreamOIDCRefreshCall: happyUpstreamRefreshCall(), + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), }, @@ -1096,7 +1141,7 @@ func TestRefreshGrant(t *testing.T) { wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, - wantUpstreamOIDCRefreshCall: happyUpstreamRefreshCall(), + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), wantCustomSessionDataStored: upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), }, @@ -1449,8 +1494,8 @@ func TestRefreshGrant(t *testing.T) { }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ - wantUpstreamOIDCRefreshCall: happyUpstreamRefreshCall(), - wantStatus: http.StatusUnauthorized, + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { "error": "error", @@ -1474,7 +1519,7 @@ func TestRefreshGrant(t *testing.T) { }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ - wantUpstreamOIDCRefreshCall: happyUpstreamRefreshCall(), + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` @@ -1486,6 +1531,322 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "upstream ldap refresh happy path", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: happyRefreshTokenResponseForLDAP( + &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + }, + ), + }, + }, + { + name: "upstream active directory refresh happy path", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: activeDirectoryUpstreamName, + ResourceUID: activeDirectoryUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: happyRefreshTokenResponseForActiveDirectory( + &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + }, + ), + }, + }, + { + name: "upstream ldap refresh when the LDAP session data is nil", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: nil, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: nil, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "upstream active directory refresh when the ad session data is nil", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: activeDirectoryUpstreamName, + ResourceUID: activeDirectoryUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: nil, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + LDAP: nil, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "upstream ldap refresh when the LDAP session data does not contain dn", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: "", + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: "", + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "upstream active directory refresh when the active directory session data does not contain dn", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: "", + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: "", + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "upstream ldap refresh returns an error", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + PerformRefreshErr: errors.New("Some error performing upstream refresh"), + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed using provider 'some-ldap-idp' of type 'ldap'." + } + `), + }, + }, + }, + { + name: "upstream active directory refresh returns an error", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: activeDirectoryUpstreamName, + ResourceUID: activeDirectoryUpstreamResourceUID, + URL: ldapUpstreamURL, + PerformRefreshErr: errors.New("Some error performing upstream refresh"), + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed using provider 'some-ad-idp' of type 'activedirectory'." + } + `), + }, + }, + }, } for _, test := range tests { test := test @@ -1493,6 +1854,8 @@ func TestRefreshGrant(t *testing.T) { t.Parallel() // First exchange the authcode for tokens, including a refresh token. + // its actually fine to use this function even when simulating ldap (which uses a different flow) because it's + // just populating a secret in storage. subject, rsp, authCode, jwtSigningKey, secrets, oauthStore := exchangeAuthcodeForTokens(t, test.authcodeExchange, test.idps.Build()) var parsedAuthcodeExchangeResponseBody map[string]interface{} require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedAuthcodeExchangeResponseBody)) @@ -1525,11 +1888,11 @@ func TestRefreshGrant(t *testing.T) { t.Logf("second response body: %q", refreshResponse.Body.String()) // Test that we did or did not make a call to the upstream OIDC provider interface to perform a token refresh. - if test.refreshRequest.want.wantUpstreamOIDCRefreshCall != nil { - test.refreshRequest.want.wantUpstreamOIDCRefreshCall.args.Ctx = reqContext + if test.refreshRequest.want.wantUpstreamRefreshCall != nil { + test.refreshRequest.want.wantUpstreamRefreshCall.args.Ctx = reqContext test.idps.RequireExactlyOneCallToPerformRefresh(t, - test.refreshRequest.want.wantUpstreamOIDCRefreshCall.performedByUpstreamName, - test.refreshRequest.want.wantUpstreamOIDCRefreshCall.args, + test.refreshRequest.want.wantUpstreamRefreshCall.performedByUpstreamName, + test.refreshRequest.want.wantUpstreamRefreshCall.args, ) } else { test.idps.RequireExactlyZeroCallsToPerformRefresh(t) diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index 72ea3bdb..8009f91d 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -45,6 +45,10 @@ type CustomSessionData struct { // Only used when ProviderType == "oidc". OIDC *OIDCSessionData `json:"oidc,omitempty"` + + LDAP *LDAPSessionData `json:"ldap,omitempty"` + + ActiveDirectory *ActiveDirectorySessionData `json:"activedirectory,omitempty"` } type ProviderType string @@ -60,6 +64,16 @@ type OIDCSessionData struct { UpstreamRefreshToken string `json:"upstreamRefreshToken"` } +// LDAPSessionData is the additional data needed by Pinniped when the upstream IDP is an LDAP provider. +type LDAPSessionData struct { + UserDN string `json:"userDN"` +} + +// ActiveDirectorySessionData is the additional data needed by Pinniped when the upstream IDP is an Active Directory provider. +type ActiveDirectorySessionData struct { + UserDN string `json:"userDN"` +} + // NewPinnipedSession returns a new empty session. func NewPinnipedSession() *PinnipedSession { return &PinnipedSession{ diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 90361ddd..84d160f1 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -63,6 +63,7 @@ type PasswordCredentialsGrantAndValidateTokensArgs struct { type PerformRefreshArgs struct { Ctx context.Context RefreshToken string + DN string } // ValidateTokenArgs is used to spy on calls to @@ -74,10 +75,13 @@ type ValidateTokenArgs struct { } type TestUpstreamLDAPIdentityProvider struct { - Name string - ResourceUID types.UID - URL *url.URL - AuthenticateFunc func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) + Name string + ResourceUID types.UID + URL *url.URL + AuthenticateFunc func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) + performRefreshCallCount int + performRefreshArgs []*PerformRefreshArgs + PerformRefreshErr error } var _ provider.UpstreamLDAPIdentityProviderI = &TestUpstreamLDAPIdentityProvider{} @@ -98,6 +102,32 @@ func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL { return u.URL } +func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN string) error { + if u.performRefreshArgs == nil { + u.performRefreshArgs = make([]*PerformRefreshArgs, 0) + } + u.performRefreshCallCount++ + u.performRefreshArgs = append(u.performRefreshArgs, &PerformRefreshArgs{ + Ctx: ctx, + DN: userDN, + }) + if u.PerformRefreshErr != nil { + return u.PerformRefreshErr + } + return nil +} + +func (u *TestUpstreamLDAPIdentityProvider) PerformRefreshCallCount() int { + return u.performRefreshCallCount +} + +func (u *TestUpstreamLDAPIdentityProvider) PerformRefreshArgs(call int) *PerformRefreshArgs { + if u.performRefreshArgs == nil { + u.performRefreshArgs = make([]*PerformRefreshArgs, 0) + } + return u.performRefreshArgs[call] +} + type TestUpstreamOIDCIdentityProvider struct { Name string ClientID string @@ -390,31 +420,54 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToPerformRefresh( t.Helper() var actualArgs *PerformRefreshArgs var actualNameOfUpstreamWhichMadeCall string - actualCallCountAcrossAllOIDCUpstreams := 0 + actualCallCountAcrossAllUpstreams := 0 for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { callCountOnThisUpstream := upstreamOIDC.performRefreshCallCount - actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream + actualCallCountAcrossAllUpstreams += callCountOnThisUpstream if callCountOnThisUpstream == 1 { actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name actualArgs = upstreamOIDC.performRefreshArgs[0] } } - require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, - "should have been exactly one call to PerformRefresh() by all OIDC upstreams", + for _, upstreamLDAP := range b.upstreamLDAPIdentityProviders { + callCountOnThisUpstream := upstreamLDAP.performRefreshCallCount + actualCallCountAcrossAllUpstreams += callCountOnThisUpstream + if callCountOnThisUpstream == 1 { + actualNameOfUpstreamWhichMadeCall = upstreamLDAP.Name + actualArgs = upstreamLDAP.performRefreshArgs[0] + } + } + for _, upstreamAD := range b.upstreamActiveDirectoryIdentityProviders { + callCountOnThisUpstream := upstreamAD.performRefreshCallCount + actualCallCountAcrossAllUpstreams += callCountOnThisUpstream + if callCountOnThisUpstream == 1 { + actualNameOfUpstreamWhichMadeCall = upstreamAD.Name + actualArgs = upstreamAD.performRefreshArgs[0] + } + } + require.Equal(t, 1, actualCallCountAcrossAllUpstreams, + "should have been exactly one call to PerformRefresh() by all upstreams", ) require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall, - "PerformRefresh() was called on the wrong OIDC upstream", + "PerformRefresh() was called on the wrong upstream", ) require.Equal(t, expectedArgs, actualArgs) } func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToPerformRefresh(t *testing.T) { t.Helper() - actualCallCountAcrossAllOIDCUpstreams := 0 + actualCallCountAcrossAllUpstreams := 0 for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { - actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.performRefreshCallCount + actualCallCountAcrossAllUpstreams += upstreamOIDC.performRefreshCallCount } - require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, + for _, upstreamLDAP := range b.upstreamLDAPIdentityProviders { + actualCallCountAcrossAllUpstreams += upstreamLDAP.performRefreshCallCount + } + for _, upstreamActiveDirectory := range b.upstreamActiveDirectoryIdentityProviders { + actualCallCountAcrossAllUpstreams += upstreamActiveDirectory.performRefreshCallCount + } + + require.Equal(t, 0, actualCallCountAcrossAllUpstreams, "expected exactly zero calls to PerformRefresh()", ) } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 1baab58b..b0b05e07 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -169,6 +169,43 @@ func (p *Provider) GetConfig() ProviderConfig { return p.c } +func (p *Provider) PerformRefresh(ctx context.Context, userDN string) error { + t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetName()}) + defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches + search := p.refreshUserSearchRequest(userDN) + + conn, err := p.dial(ctx) + if err != nil { + p.traceAuthFailure(t, err) + return fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) + } + defer conn.Close() + + err = conn.Bind(p.c.BindUsername, p.c.BindPassword) + if err != nil { + p.traceAuthFailure(t, err) + return fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) + } + + searchResult, err := conn.Search(search) + + if err != nil { + return fmt.Errorf(`error searching for user "%s": %w`, userDN, err) + } + + // if any more or less than one entry, error. + // we don't need to worry about logging this because we know it's a dn. + if len(searchResult.Entries) != 1 { + return fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, + userDN, len(searchResult.Entries), + ) + } + + // do nothing. if we got exactly one search result back then that means the user + // still exists. + return nil +} + func (p *Provider) dial(ctx context.Context) (Conn, error) { tlsAddr, err := endpointaddr.Parse(p.c.Host, defaultLDAPSPort) if err != nil { @@ -355,7 +392,7 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi return nil, false, fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) } - mappedUsername, mappedUID, mappedGroupNames, err := p.searchAndBindUser(conn, username, bindFunc) + mappedUsername, mappedUID, mappedGroupNames, userDN, err := p.searchAndBindUser(conn, username, bindFunc) if err != nil { p.traceAuthFailure(t, err) return nil, false, err @@ -371,6 +408,7 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi Name: mappedUsername, UID: mappedUID, Groups: mappedGroupNames, + Extra: map[string][]string{"userDN": {userDN}}, }, } p.traceAuthSuccess(t) @@ -454,7 +492,7 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e return searchBase, nil } -func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(conn Conn, foundUserDN string) error) (string, string, []string, error) { +func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(conn Conn, foundUserDN string) error) (string, string, []string, string, error) { searchResult, err := conn.Search(p.userSearchRequest(username)) if err != nil { plog.All(`error searching for user`, @@ -462,7 +500,7 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c "username", username, "err", err, ) - return "", "", nil, fmt.Errorf(`error searching for user: %w`, err) + return "", "", nil, "", fmt.Errorf(`error searching for user: %w`, err) } if len(searchResult.Entries) == 0 { if plog.Enabled(plog.LevelAll) { @@ -473,38 +511,38 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c } else { plog.Debug("error finding user: user not found (cowardly avoiding printing username because log level is not 'all')", "upstreamName", p.GetName()) } - return "", "", nil, nil + return "", "", nil, "", nil } // At this point, we have matched at least one entry, so we can be confident that the username is not actually // someone's password mistakenly entered into the username field, so we can log it without concern. if len(searchResult.Entries) > 1 { - return "", "", nil, fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, + return "", "", nil, "", fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, username, len(searchResult.Entries), ) } userEntry := searchResult.Entries[0] if len(userEntry.DN) == 0 { - return "", "", nil, fmt.Errorf(`searching for user "%s" resulted in search result without DN`, username) + return "", "", nil, "", fmt.Errorf(`searching for user "%s" resulted in search result without DN`, username) } mappedUsername, err := p.getSearchResultAttributeValue(p.c.UserSearch.UsernameAttribute, userEntry, username) if err != nil { - return "", "", nil, err + return "", "", nil, "", err } // We would like to support binary typed attributes for UIDs, so always read them as binary and encode them, // even when the attribute may not be binary. mappedUID, err := p.getSearchResultAttributeRawValueEncoded(p.c.UserSearch.UIDAttribute, userEntry, username) if err != nil { - return "", "", nil, err + return "", "", nil, "", err } mappedGroupNames := []string{} if len(p.c.GroupSearch.Base) > 0 { mappedGroupNames, err = p.searchGroupsForUserDN(conn, userEntry.DN) if err != nil { - return "", "", nil, err + return "", "", nil, "", err } } sort.Strings(mappedGroupNames) @@ -516,12 +554,12 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c err, "upstreamName", p.GetName(), "username", username, "dn", userEntry.DN) ldapErr := &ldap.Error{} if errors.As(err, &ldapErr) && ldapErr.ResultCode == ldap.LDAPResultInvalidCredentials { - return "", "", nil, nil + return "", "", nil, "", nil } - return "", "", nil, fmt.Errorf(`error binding for user "%s" using provided password against DN "%s": %w`, username, userEntry.DN, err) + return "", "", nil, "", fmt.Errorf(`error binding for user "%s" using provided password against DN "%s": %w`, username, userEntry.DN, err) } - return mappedUsername, mappedUID, mappedGroupNames, nil + return mappedUsername, mappedUID, mappedGroupNames, userEntry.DN, nil } func (p *Provider) defaultNamingContextRequest() *ldap.SearchRequest { @@ -568,6 +606,21 @@ func (p *Provider) groupSearchRequest(userDN string) *ldap.SearchRequest { } } +func (p *Provider) refreshUserSearchRequest(dn string) *ldap.SearchRequest { + // See https://ldap.com/the-ldap-search-operation for general documentation of LDAP search options. + return &ldap.SearchRequest{ + BaseDN: dn, + Scope: ldap.ScopeBaseObject, + DerefAliases: ldap.NeverDerefAliases, + SizeLimit: 2, + TimeLimit: 90, + TypesOnly: false, + Filter: "(objectClass=*)", // we already have the dn, so the filter doesn't matter + Attributes: []string{}, // TODO this will need to include some other AD attributes + Controls: nil, // this could be used to enable paging, but we're already limiting the result max size + } +} + func (p *Provider) userSearchRequestedAttributes() []string { attributes := []string{} if p.c.UserSearch.UsernameAttribute != distinguishedNameAttributeName { diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 0cb1f355..b7f953be 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -156,6 +156,7 @@ func TestEndUserAuthentication(t *testing.T) { Name: testUserSearchResultUsernameAttributeValue, UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{testGroupSearchResultGroupNameAttributeValue1, testGroupSearchResultGroupNameAttributeValue2}, + Extra: map[string][]string{"userDN": {testUserSearchResultDNValue}}, } if editFunc != nil { editFunc(u) @@ -503,6 +504,7 @@ func TestEndUserAuthentication(t *testing.T) { Name: testUserSearchResultUsernameAttributeValue, UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{"a", "b", "c"}, + Extra: map[string][]string{"userDN": {testUserSearchResultDNValue}}, }, }, }, @@ -1212,6 +1214,151 @@ func TestEndUserAuthentication(t *testing.T) { } } +func TestUpstreamRefresh(t *testing.T) { + expectedUserSearch := &ldap.SearchRequest{ + BaseDN: testUserSearchResultDNValue, + Scope: ldap.ScopeBaseObject, + DerefAliases: ldap.NeverDerefAliases, + SizeLimit: 2, + TimeLimit: 90, + TypesOnly: false, + Filter: "(objectClass=*)", + Attributes: []string{}, + Controls: nil, // don't need paging because we set the SizeLimit so small + } + + happyPathUserSearchResult := &ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{}, + }, + }, + } + + providerConfig := &ProviderConfig{ + Name: "some-provider-name", + Host: testHost, + CABundle: nil, // this field is only used by the production dialer, which is replaced by a mock for this test + ConnectionProtocol: TLS, + BindUsername: testBindUsername, + BindPassword: testBindPassword, + UserSearch: UserSearchConfig{ + Base: testUserSearchBase, + }, + } + + tests := []struct { + name string + providerConfig *ProviderConfig + setupMocks func(conn *mockldapconn.MockConn) + dialError error + wantErr string + }{ + { + name: "happy path where searching the dn returns a single entry", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(happyPathUserSearchResult, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + }, + { + name: "error where dial fails", + providerConfig: providerConfig, + dialError: errors.New("some dial error"), + wantErr: "error dialing host \"ldap.example.com:8443\": some dial error", + }, + { + name: "error binding", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Return(errors.New("some bind error")).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "error binding as \"cn=some-bind-username,dc=pinniped,dc=dev\" before user search: some bind error", + }, + { + name: "search result returns no entries", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{}, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "searching for user \"some-upstream-user-dn\" resulted in 0 search results, but expected 1 result", + }, + { + name: "error searching", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(nil, errors.New("some search error")) + conn.EXPECT().Close().Times(1) + }, + wantErr: "error searching for user \"some-upstream-user-dn\": some search error", + }, + { + name: "search result returns more than one entry", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{}, + }, + { + DN: "doesn't-matter", + Attributes: []*ldap.EntryAttribute{}, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "searching for user \"some-upstream-user-dn\" resulted in 2 search results, but expected 1 result", + }, + } + + for _, test := range tests { + tt := test + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + conn := mockldapconn.NewMockConn(ctrl) + if tt.setupMocks != nil { + tt.setupMocks(conn) + } + + dialWasAttempted := false + providerConfig.Dialer = LDAPDialerFunc(func(ctx context.Context, addr endpointaddr.HostPort) (Conn, error) { + dialWasAttempted = true + require.Equal(t, providerConfig.Host, addr.Endpoint()) + if tt.dialError != nil { + return nil, tt.dialError + } + + return conn, nil + }) + + provider := New(*providerConfig) + err := provider.PerformRefresh(context.Background(), testUserSearchResultDNValue) + if tt.wantErr != "" { + require.NotNil(t, err) + require.Equal(t, tt.wantErr, err.Error()) + } else { + require.NoError(t, err) + } + require.Equal(t, true, dialWasAttempted) + }) + } +} + func TestTestConnection(t *testing.T) { providerConfig := func(editFunc func(p *ProviderConfig)) *ProviderConfig { config := &ProviderConfig{ diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 9c21698c..8cafa810 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -83,7 +83,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(nil)), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -95,7 +95,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.ConnectionProtocol = upstreamldap.StartTLS })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -104,7 +104,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Base = "dc=pinniped,dc=dev" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -113,7 +113,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Filter = "(cn={})" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -125,7 +125,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.Filter = "cn={}" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "cn=pinny,ou=users,dc=pinniped,dc=dev", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "cn=pinny,ou=users,dc=pinniped,dc=dev", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -136,7 +136,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.Filter = "(|(cn={})(mail={}))" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -147,7 +147,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.Filter = "(|(cn={})(mail={}))" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -156,7 +156,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UIDAttribute = "dn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("cn=pinny,ou=users,dc=pinniped,dc=dev"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("cn=pinny,ou=users,dc=pinniped,dc=dev"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -165,7 +165,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UIDAttribute = "sn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("Seal"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("Seal"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -174,7 +174,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UsernameAttribute = "sn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "Seal", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, // note that the final answer has case preserved from the entry + User: &user.DefaultInfo{Name: "Seal", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, // note that the final answer has case preserved from the entry }, }, { @@ -187,7 +187,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.UIDAttribute = "givenName" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "Pinny the 🦭", UID: b64("Pinny the 🦭"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "Pinny the 🦭", UID: b64("Pinny the 🦭"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -199,7 +199,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.UsernameAttribute = "cn" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -220,7 +220,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.Base = "" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -231,7 +231,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.Base = "ou=users,dc=pinniped,dc=dev" // there are no groups under this part of the tree })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -245,7 +245,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{ "cn=ball-game-players,ou=beach-groups,ou=groups,dc=pinniped,dc=dev", "cn=seals,ou=groups,dc=pinniped,dc=dev", - }}, + }, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -259,7 +259,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{ "cn=ball-game-players,ou=beach-groups,ou=groups,dc=pinniped,dc=dev", "cn=seals,ou=groups,dc=pinniped,dc=dev", - }}, + }, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -270,7 +270,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.GroupNameAttribute = "objectClass" // silly example, but still a meaningful test })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"groupOfNames", "groupOfNames"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"groupOfNames", "groupOfNames"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -281,7 +281,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.Filter = "(&(&(objectClass=groupOfNames)(member={}))(cn=seals))" })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -292,7 +292,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.GroupSearch.Filter = "foobar={}" // foobar is not a valid attribute name for this LDAP server's schema })), wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, }, { @@ -671,7 +671,7 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) { assert.NoError(t, result.err) assert.True(t, result.authenticated, "expected the user to be authenticated, but they were not") assert.Equal(t, &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, }, result.response) } } diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 770d67b7..33881325 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -214,7 +214,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.LDAP.UserDN) + customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamLDAP.Host+ @@ -281,7 +285,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.LDAP.UserDN) + customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamLDAP.StartTLSOnlyHost+ @@ -348,9 +356,13 @@ func TestSupervisorLogin(t *testing.T) { true, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type - wantErrorDescription: "The resource owner or authorization server denied the request. Username/password not accepted by LDAP provider.", - wantErrorType: "access_denied", + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.LDAP.UserDN) + customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, + wantErrorDescription: "The resource owner or authorization server denied the request. Username/password not accepted by LDAP provider.", + wantErrorType: "access_denied", }, { name: "ldap login still works after updating bind secret", @@ -426,7 +438,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.LDAP.UserDN) + customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamLDAP.Host+ @@ -525,7 +541,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.LDAP.UserDN) + customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamLDAP.Host+ @@ -580,7 +600,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) + customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamActiveDirectory.Host+ @@ -648,7 +672,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) + customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamActiveDirectory.Host+ @@ -721,7 +749,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) + customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamActiveDirectory.Host+ @@ -809,7 +841,11 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) + customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( "ldaps://"+env.SupervisorUpstreamActiveDirectory.Host+ @@ -864,7 +900,7 @@ func TestSupervisorLogin(t *testing.T) { true, ) }, - breakRefreshSessionData: nil, // upstream refresh not yet implemented for this IDP type + breakRefreshSessionData: nil, wantErrorDescription: "The resource owner or authorization server denied the request. Username/password not accepted by LDAP provider.", wantErrorType: "access_denied", }, From 7a58086040e4ec34c75156a72a18cec48c8edc69 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Mon, 25 Oct 2021 14:25:43 -0700 Subject: [PATCH 02/10] Check that username and subject remain the same for ldap refresh --- internal/oidc/auth/auth_handler.go | 5 +- .../downstreamsession/downstream_session.go | 7 ++ .../provider/dynamic_upstream_idp_provider.go | 2 +- internal/oidc/token/token_handler.go | 29 ++++-- internal/oidc/token/token_handler_test.go | 90 +++++++++++++++-- .../testutil/oidctestutil/oidctestutil.go | 16 +-- internal/upstreamldap/upstreamldap.go | 33 ++++++- internal/upstreamldap/upstreamldap_test.go | 99 +++++++++++++++++-- 8 files changed, 246 insertions(+), 35 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index d22f9ad7..cc637b4d 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -487,10 +487,7 @@ func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticator.Response) string { ldapURL := *ldapUpstream.GetURL() - q := ldapURL.Query() - q.Set(oidc.IDTokenSubjectClaim, authenticateResponse.User.GetUID()) - ldapURL.RawQuery = q.Encode() - return ldapURL.String() + return downstreamsession.DownstreamLDAPSubject(authenticateResponse.User.GetUID(), ldapURL) } func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string { diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 0fee5a78..8618ab57 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -169,6 +169,13 @@ func extractStringClaimValue(claimName string, upstreamIDPName string, idTokenCl return valueAsString, nil } +func DownstreamLDAPSubject(uid string, ldapURL url.URL) string { + q := ldapURL.Query() + q.Set(oidc.IDTokenSubjectClaim, uid) + ldapURL.RawQuery = q.Encode() + return ldapURL.String() +} + func downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, url.QueryEscape(upstreamSubject)) } diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index c5f2b7b1..d7aa02c0 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -90,7 +90,7 @@ type UpstreamLDAPIdentityProviderI interface { authenticators.UserAuthenticator // PerformRefresh performs a refresh against the upstream LDAP identity provider - PerformRefresh(ctx context.Context, userDN string) error + PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error } type DynamicUpstreamIDPProvider interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index dcec411b..fee06a63 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -6,6 +6,7 @@ package token import ( "context" + "fmt" "net/http" "github.com/ory/fosite" @@ -75,6 +76,18 @@ func NewHandler( func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, providerCache oidc.UpstreamIdentityProvidersLister) error { session := accessRequest.GetSession().(*psession.PinnipedSession) + fositeSession := session.Fosite + if fositeSession == nil { + return fmt.Errorf("fosite session not found") + } + claims := fositeSession.Claims + if claims == nil { + return fmt.Errorf("fosite session claims not found") + } + extra := claims.Extra + downstreamUsername := extra["username"].(string) + downstreamSubject := claims.Subject + customSessionData := session.Custom if customSessionData == nil { return errorsx.WithStack(errMissingUpstreamSessionInternalError) @@ -89,9 +102,9 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, case psession.ProviderTypeOIDC: return upstreamOIDCRefresh(ctx, customSessionData, providerCache) case psession.ProviderTypeLDAP: - return upstreamLDAPRefresh(ctx, customSessionData, providerCache) + return upstreamLDAPRefresh(ctx, customSessionData, providerCache, downstreamUsername, downstreamSubject) case psession.ProviderTypeActiveDirectory: - return upstreamLDAPRefresh(ctx, customSessionData, providerCache) + return upstreamLDAPRefresh(ctx, customSessionData, providerCache, downstreamUsername, downstreamSubject) default: return errorsx.WithStack(errMissingUpstreamSessionInternalError) } @@ -164,19 +177,19 @@ func findOIDCProviderByNameAndValidateUID( WithHintf("Provider %q of type %q from upstream session data was not found.", s.ProviderName, s.ProviderType)) } -func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister) error { - plog.Warning("refreshing upstream") +func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister, username string, subject string) error { // if you have neither a valid ldap session config nor a valid active directory session config if (s.LDAP == nil || s.LDAP.UserDN == "") && (s.ActiveDirectory == nil || s.ActiveDirectory.UserDN == "") { return errorsx.WithStack(errMissingUpstreamSessionInternalError) } - plog.Warning("going to find provider", "provider", s.ProviderName) // get ldap/ad provider out of cache - p, dn, _ := findLDAPProviderByNameAndValidateUID(s, providerCache) - // TODO error checking + p, dn, err := findLDAPProviderByNameAndValidateUID(s, providerCache) + if err != nil { + return err + } // run PerformRefresh - err := p.PerformRefresh(ctx, dn) + err = p.PerformRefresh(ctx, dn, username, subject) if err != nil { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( "Upstream refresh failed using provider %q of type %q.", diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index c819847e..50825b80 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -933,8 +933,10 @@ func TestRefreshGrant(t *testing.T) { return &expectedUpstreamRefresh{ performedByUpstreamName: ldapUpstreamName, args: &oidctestutil.PerformRefreshArgs{ - Ctx: nil, - DN: ldapUpstreamDN, + Ctx: nil, + DN: ldapUpstreamDN, + ExpectedSubject: goodSubject, + ExpectedUsername: goodUsername, }, } } @@ -943,8 +945,10 @@ func TestRefreshGrant(t *testing.T) { return &expectedUpstreamRefresh{ performedByUpstreamName: activeDirectoryUpstreamName, args: &oidctestutil.PerformRefreshArgs{ - Ctx: nil, - DN: activeDirectoryUpstreamDN, + Ctx: nil, + DN: activeDirectoryUpstreamDN, + ExpectedSubject: goodSubject, + ExpectedUsername: goodUsername, }, } } @@ -1796,7 +1800,8 @@ func TestRefreshGrant(t *testing.T) { }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ - wantStatus: http.StatusUnauthorized, + wantUpstreamRefreshCall: happyLDAPUpstreamRefreshCall(), + wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { "error": "error", @@ -1837,7 +1842,8 @@ func TestRefreshGrant(t *testing.T) { }, refreshRequest: refreshRequestInputs{ want: tokenEndpointResponseExpectedValues{ - wantStatus: http.StatusUnauthorized, + wantUpstreamRefreshCall: happyActiveDirectoryUpstreamRefreshCall(), + wantStatus: http.StatusUnauthorized, wantErrorResponseBody: here.Doc(` { "error": "error", @@ -1847,6 +1853,78 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "upstream ldap idp not found", + idps: oidctestutil.NewUpstreamIDPListerBuilder(), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Provider 'some-ldap-idp' of type 'ldap' from upstream session data was not found." + } + `), + }, + }, + }, + { + name: "upstream active directory idp not found", + idps: oidctestutil.NewUpstreamIDPListerBuilder(), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + }, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Provider 'some-ad-idp' of type 'activedirectory' from upstream session data was not found." + } + `), + }, + }, + }, } for _, test := range tests { test := test diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 84d160f1..23b193f0 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -61,9 +61,11 @@ type PasswordCredentialsGrantAndValidateTokensArgs struct { // PerformRefreshArgs is used to spy on calls to // TestUpstreamOIDCIdentityProvider.PerformRefreshFunc(). type PerformRefreshArgs struct { - Ctx context.Context - RefreshToken string - DN string + Ctx context.Context + RefreshToken string + DN string + ExpectedUsername string + ExpectedSubject string } // ValidateTokenArgs is used to spy on calls to @@ -102,14 +104,16 @@ func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL { return u.URL } -func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN string) error { +func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error { if u.performRefreshArgs == nil { u.performRefreshArgs = make([]*PerformRefreshArgs, 0) } u.performRefreshCallCount++ u.performRefreshArgs = append(u.performRefreshArgs, &PerformRefreshArgs{ - Ctx: ctx, - DN: userDN, + Ctx: ctx, + DN: userDN, + ExpectedUsername: expectedUsername, + ExpectedSubject: expectedSubject, }) if u.PerformRefreshErr != nil { return u.PerformRefreshErr diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index b0b05e07..9f24f762 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -27,6 +27,7 @@ import ( "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/endpointaddr" + "go.pinniped.dev/internal/oidc/downstreamsession" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" ) @@ -169,7 +170,7 @@ func (p *Provider) GetConfig() ProviderConfig { return p.c } -func (p *Provider) PerformRefresh(ctx context.Context, userDN string) error { +func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error { t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches search := p.refreshUserSearchRequest(userDN) @@ -201,6 +202,30 @@ func (p *Provider) PerformRefresh(ctx context.Context, userDN string) error { ) } + userEntry := searchResult.Entries[0] + if len(userEntry.DN) == 0 { + return fmt.Errorf(`searching for user with original DN "%s" resulted in search result without DN`, userDN) + } + + newUsername, err := p.getSearchResultAttributeValue(p.c.UserSearch.UsernameAttribute, userEntry, userDN) + if err != nil { + return err // TODO test having no values or more than one maybe + } + if newUsername != expectedUsername { + return fmt.Errorf(`searching for user "%s" returned a different username than the previous value. expected: "%s", actual: "%s"`, + userDN, expectedUsername, newUsername, + ) + } + + newUID, err := p.getSearchResultAttributeRawValueEncoded(p.c.UserSearch.UIDAttribute, userEntry, userDN) + if err != nil { + return err // TODO test + } + newSubject := downstreamsession.DownstreamLDAPSubject(newUID, *p.GetURL()) + if newSubject != expectedSubject { + return fmt.Errorf(`searching for user "%s" produced a different subject than the previous value. expected: "%s", actual: "%s"`, userDN, expectedSubject, newSubject) + } + // do nothing. if we got exactly one search result back then that means the user // still exists. return nil @@ -615,9 +640,9 @@ func (p *Provider) refreshUserSearchRequest(dn string) *ldap.SearchRequest { SizeLimit: 2, TimeLimit: 90, TypesOnly: false, - Filter: "(objectClass=*)", // we already have the dn, so the filter doesn't matter - Attributes: []string{}, // TODO this will need to include some other AD attributes - Controls: nil, // this could be used to enable paging, but we're already limiting the result max size + Filter: "(objectClass=*)", // we already have the dn, so the filter doesn't matter + Attributes: p.userSearchRequestedAttributes(), // TODO this will need to include some other AD attributes + Controls: nil, // this could be used to enable paging, but we're already limiting the result max size } } diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index b7f953be..259c6e94 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -1223,15 +1223,25 @@ func TestUpstreamRefresh(t *testing.T) { TimeLimit: 90, TypesOnly: false, Filter: "(objectClass=*)", - Attributes: []string{}, + Attributes: []string{testUserSearchUsernameAttribute, testUserSearchUIDAttribute}, Controls: nil, // don't need paging because we set the SizeLimit so small } happyPathUserSearchResult := &ldap.SearchResult{ Entries: []*ldap.Entry{ { - DN: testUserSearchResultDNValue, - Attributes: []*ldap.EntryAttribute{}, + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{testUserSearchResultUsernameAttributeValue}, + }, + { + Name: testUserSearchUIDAttribute, + Values: []string{testUserSearchResultUIDAttributeValue}, + ByteValues: [][]byte{[]byte(testUserSearchResultUIDAttributeValue)}, + }, + }, }, }, } @@ -1244,7 +1254,9 @@ func TestUpstreamRefresh(t *testing.T) { BindUsername: testBindUsername, BindPassword: testBindPassword, UserSearch: UserSearchConfig{ - Base: testUserSearchBase, + Base: testUserSearchBase, + UIDAttribute: testUserSearchUIDAttribute, + UsernameAttribute: testUserSearchUsernameAttribute, }, } @@ -1322,6 +1334,80 @@ func TestUpstreamRefresh(t *testing.T) { }, wantErr: "searching for user \"some-upstream-user-dn\" resulted in 2 search results, but expected 1 result", }, + { + name: "search result has wrong uid", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{testUserSearchResultUsernameAttributeValue}, + }, + { + Name: testUserSearchUIDAttribute, + Values: []string{"wrong-uid"}, + ByteValues: [][]byte{[]byte("wrong-uid")}, + }, + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "searching for user \"some-upstream-user-dn\" produced a different subject than the previous value. expected: \"ldaps://ldap.example.com:8443?base=some-upstream-user-base-dn&sub=c29tZS11cHN0cmVhbS11aWQtdmFsdWU\", actual: \"ldaps://ldap.example.com:8443?base=some-upstream-user-base-dn&sub=d3JvbmctdWlk\"", + }, + { + name: "search result has wrong username", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{"wrong-username"}, + }, + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "searching for user \"some-upstream-user-dn\" returned a different username than the previous value. expected: \"some-upstream-username-value\", actual: \"wrong-username\"", + }, + { + name: "search result has no dn", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{testUserSearchResultUsernameAttributeValue}, + }, + { + Name: testUserSearchUIDAttribute, + Values: []string{testUserSearchResultUIDAttributeValue}, + }, + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "searching for user with original DN \"some-upstream-user-dn\" resulted in search result without DN", + }, } for _, test := range tests { @@ -1347,9 +1433,10 @@ func TestUpstreamRefresh(t *testing.T) { }) provider := New(*providerConfig) - err := provider.PerformRefresh(context.Background(), testUserSearchResultDNValue) + subject := "ldaps://ldap.example.com:8443?base=some-upstream-user-base-dn&sub=c29tZS11cHN0cmVhbS11aWQtdmFsdWU" + err := provider.PerformRefresh(context.Background(), testUserSearchResultDNValue, testUserSearchResultUsernameAttributeValue, subject) if tt.wantErr != "" { - require.NotNil(t, err) + require.Error(t, err) require.Equal(t, tt.wantErr, err.Error()) } else { require.NoError(t, err) From 2c4dc2951dee68bf79141337853a33701614da7b Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Mon, 25 Oct 2021 16:45:30 -0700 Subject: [PATCH 03/10] resolved a couple of testing related todos --- internal/oidc/auth/auth_handler.go | 8 +- internal/upstreamldap/upstreamldap.go | 11 ++- internal/upstreamldap/upstreamldap_test.go | 110 ++++++++++++++++++++- 3 files changed, 117 insertions(+), 12 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index cc637b4d..caff98f3 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -491,12 +491,8 @@ func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentit } func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string { - // These errors shouldn't happen, but do some error checking anyway so it doesn't panic - extra := authenticatedResponse.User.GetExtra() - if len(extra) == 0 { - return "" - } - dnSlice := extra["userDN"] + // This error shouldn't happen, but do some error checking anyway so it doesn't panic + dnSlice := authenticatedResponse.User.GetExtra()["userDN"] if len(dnSlice) != 1 { return "" } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 9f24f762..fad03711 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -177,20 +177,21 @@ func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUs conn, err := p.dial(ctx) if err != nil { - p.traceAuthFailure(t, err) + p.traceRefreshFailure(t, err) return fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) } defer conn.Close() err = conn.Bind(p.c.BindUsername, p.c.BindPassword) if err != nil { - p.traceAuthFailure(t, err) + p.traceRefreshFailure(t, err) return fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) } searchResult, err := conn.Search(search) if err != nil { + p.traceRefreshFailure(t, err) return fmt.Errorf(`error searching for user "%s": %w`, userDN, err) } @@ -765,6 +766,12 @@ func (p *Provider) traceSearchBaseDiscoveryFailure(t *trace.Trace, err error) { trace.Field{Key: "reason", Value: err.Error()}) } +func (p *Provider) traceRefreshFailure(t *trace.Trace, err error) { + t.Step("refresh failed", + trace.Field{Key: "reason", Value: err.Error()}, + ) +} + func MicrosoftUUIDFromBinary(attributeName string) func(entry *ldap.Entry) (string, error) { // validation has already been done so we can just get the attribute... return func(entry *ldap.Entry) (string, error) { diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 259c6e94..c9295b8f 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -1238,7 +1238,6 @@ func TestUpstreamRefresh(t *testing.T) { }, { Name: testUserSearchUIDAttribute, - Values: []string{testUserSearchResultUIDAttributeValue}, ByteValues: [][]byte{[]byte(testUserSearchResultUIDAttributeValue)}, }, }, @@ -1350,7 +1349,6 @@ func TestUpstreamRefresh(t *testing.T) { }, { Name: testUserSearchUIDAttribute, - Values: []string{"wrong-uid"}, ByteValues: [][]byte{[]byte("wrong-uid")}, }, }, @@ -1397,8 +1395,8 @@ func TestUpstreamRefresh(t *testing.T) { Values: []string{testUserSearchResultUsernameAttributeValue}, }, { - Name: testUserSearchUIDAttribute, - Values: []string{testUserSearchResultUIDAttributeValue}, + Name: testUserSearchUIDAttribute, + ByteValues: [][]byte{[]byte(testUserSearchResultUIDAttributeValue)}, }, }, }, @@ -1408,6 +1406,110 @@ func TestUpstreamRefresh(t *testing.T) { }, wantErr: "searching for user with original DN \"some-upstream-user-dn\" resulted in search result without DN", }, + { + name: "search result has 0 values for username attribute", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{}, + }, + { + Name: testUserSearchUIDAttribute, + ByteValues: [][]byte{[]byte(testUserSearchResultUIDAttributeValue)}, + }, + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "found 0 values for attribute \"some-upstream-username-attribute\" while searching for user \"some-upstream-user-dn\", but expected 1 result", + }, + { + name: "search result has more than one value for username attribute", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{testUserSearchResultUsernameAttributeValue, "something-else"}, + }, + { + Name: testUserSearchUIDAttribute, + ByteValues: [][]byte{[]byte(testUserSearchResultUIDAttributeValue)}, + }, + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "found 2 values for attribute \"some-upstream-username-attribute\" while searching for user \"some-upstream-user-dn\", but expected 1 result", + }, + { + name: "search result has 0 values for uid attribute", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{testUserSearchResultUsernameAttributeValue}, + }, + { + Name: testUserSearchUIDAttribute, + ByteValues: [][]byte{}, + }, + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "found 0 values for attribute \"some-upstream-uid-attribute\" while searching for user \"some-upstream-user-dn\", but expected 1 result", + }, + { + name: "search result has 2 values for uid attribute", + providerConfig: providerConfig, + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedUserSearch).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testUserSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + { + Name: testUserSearchUsernameAttribute, + Values: []string{testUserSearchResultUsernameAttributeValue}, + }, + { + Name: testUserSearchUIDAttribute, + ByteValues: [][]byte{[]byte(testUserSearchResultUIDAttributeValue), []byte("other-uid-value")}, + }, + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantErr: "found 2 values for attribute \"some-upstream-uid-attribute\" while searching for user \"some-upstream-user-dn\", but expected 1 result", + }, } for _, test := range tests { From 8396937503dacdab3848779b2d32234ca60d374a Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 26 Oct 2021 15:01:09 -0700 Subject: [PATCH 04/10] Updates to tests and some error assertions --- internal/oidc/auth/auth_handler.go | 5 +- internal/oidc/token/token_handler.go | 18 +- internal/oidc/token/token_handler_test.go | 235 +++++++++++----------- internal/upstreamldap/upstreamldap.go | 6 +- 4 files changed, 130 insertions(+), 134 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index caff98f3..aaac4926 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -113,9 +113,6 @@ func handleAuthRequestForLDAPUpstream( username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() dn := userDNFromAuthenticatedResponse(authenticateResponse) - if dn == "" { - return httperr.New(http.StatusInternalServerError, "unexpected error during upstream authentication") - } customSessionData := &psession.CustomSessionData{ ProviderUID: ldapUpstream.GetResourceUID(), @@ -491,7 +488,7 @@ func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentit } func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string { - // This error shouldn't happen, but do some error checking anyway so it doesn't panic + // We should always have something here, but do some error checking anyway so it doesn't panic dnSlice := authenticatedResponse.User.GetExtra()["userDN"] if len(dnSlice) != 1 { return "" diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index fee06a63..d26b8642 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -6,7 +6,6 @@ package token import ( "context" - "fmt" "net/http" "github.com/ory/fosite" @@ -76,17 +75,16 @@ func NewHandler( func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, providerCache oidc.UpstreamIdentityProvidersLister) error { session := accessRequest.GetSession().(*psession.PinnipedSession) - fositeSession := session.Fosite - if fositeSession == nil { - return fmt.Errorf("fosite session not found") + extra := session.Fosite.Claims.Extra + if extra == nil { + return errorsx.WithStack(errMissingUpstreamSessionInternalError) } - claims := fositeSession.Claims - if claims == nil { - return fmt.Errorf("fosite session claims not found") + downstreamUsernameInterface := extra["username"] + if downstreamUsernameInterface == nil { + return errorsx.WithStack(errMissingUpstreamSessionInternalError) } - extra := claims.Extra - downstreamUsername := extra["username"].(string) - downstreamSubject := claims.Subject + downstreamUsername := downstreamUsernameInterface.(string) + downstreamSubject := session.Fosite.Claims.Subject customSessionData := session.Custom if customSessionData == nil { diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 50825b80..0cec17b6 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1020,12 +1020,28 @@ func TestRefreshGrant(t *testing.T) { return tokens } + happyActiveDirectoryCustomSessionData := &psession.CustomSessionData{ + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: &psession.ActiveDirectorySessionData{ + UserDN: activeDirectoryUpstreamDN, + }, + } + happyLDAPCustomSessionData := &psession.CustomSessionData{ + ProviderUID: ldapUpstreamResourceUID, + ProviderName: ldapUpstreamName, + ProviderType: ldapUpstreamType, + LDAP: &psession.LDAPSessionData{ + UserDN: ldapUpstreamDN, + }, + } tests := []struct { - name string - idps *oidctestutil.UpstreamIDPListerBuilder - authcodeExchange authcodeExchangeInputs - authEndpointInitialSessionData *psession.CustomSessionData - refreshRequest refreshRequestInputs + name string + idps *oidctestutil.UpstreamIDPListerBuilder + authcodeExchange authcodeExchangeInputs + refreshRequest refreshRequestInputs + modifyRefreshTokenStorage func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) }{ { name: "happy path refresh grant with openid scope granted (id token returned)", @@ -1544,35 +1560,14 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForLDAP( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, }, @@ -1585,35 +1580,14 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForActiveDirectory( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, }, @@ -1779,23 +1753,9 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1821,23 +1781,9 @@ func TestRefreshGrant(t *testing.T) { }), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1858,23 +1804,9 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + customSessionData: happyLDAPCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: ldapUpstreamResourceUID, - ProviderName: ldapUpstreamName, - ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ - UserDN: ldapUpstreamDN, - }, - }, + happyLDAPCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1894,23 +1826,9 @@ func TestRefreshGrant(t *testing.T) { idps: oidctestutil.NewUpstreamIDPListerBuilder(), authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, - customSessionData: &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + customSessionData: happyActiveDirectoryCustomSessionData, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( - &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - ActiveDirectory: &psession.ActiveDirectorySessionData{ - UserDN: activeDirectoryUpstreamDN, - }, - }, + happyActiveDirectoryCustomSessionData, ), }, refreshRequest: refreshRequestInputs{ @@ -1925,6 +1843,85 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "fosite session is empty", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{} + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "username not found in extra field", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + //fositeSessionData: &openid.DefaultSession{}, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Extra: map[string]interface{}{}, + }, + } + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, } for _, test := range tests { test := test @@ -1952,6 +1949,10 @@ func TestRefreshGrant(t *testing.T) { // Send the refresh token back and preform a refresh. firstRefreshToken := parsedAuthcodeExchangeResponseBody["refresh_token"].(string) require.NotEmpty(t, firstRefreshToken) + + if test.modifyRefreshTokenStorage != nil { + test.modifyRefreshTokenStorage(t, oauthStore, firstRefreshToken) + } reqContext := context.WithValue(context.Background(), struct{ name string }{name: "test"}, "request-context") req := httptest.NewRequest("POST", "/path/shouldn't/matter", happyRefreshRequestBody(firstRefreshToken).ReadCloser()).WithContext(reqContext) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index fad03711..33ca62f5 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -641,9 +641,9 @@ func (p *Provider) refreshUserSearchRequest(dn string) *ldap.SearchRequest { SizeLimit: 2, TimeLimit: 90, TypesOnly: false, - Filter: "(objectClass=*)", // we already have the dn, so the filter doesn't matter - Attributes: p.userSearchRequestedAttributes(), // TODO this will need to include some other AD attributes - Controls: nil, // this could be used to enable paging, but we're already limiting the result max size + Filter: "(objectClass=*)", // we already have the dn, so the filter doesn't matter + Attributes: p.userSearchRequestedAttributes(), + Controls: nil, // this could be used to enable paging, but we're already limiting the result max size } } From 722b5dcc1b1fba593f2429db7d396ff2ec6043f7 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 26 Oct 2021 16:24:02 -0700 Subject: [PATCH 05/10] Test for change to stored username or subject. All of this is still done staticly. --- test/integration/supervisor_login_test.go | 56 +++++++++++++---------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 33881325..106b7f5f 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -55,7 +55,7 @@ func TestSupervisorLogin(t *testing.T) { // We don't necessarily have any way to revoke the user's session on the upstream provider, // so to cause the upstream refresh to fail we can cheat by manipulating the user's session // data in such a way that it should cause the next upstream refresh attempt to fail. - breakRefreshSessionData func(t *testing.T, customSessionData *psession.CustomSessionData) + breakRefreshSessionData func(t *testing.T, sessionData *psession.PinnipedSession) }{ { name: "oidc with default username and groups claim settings", @@ -75,7 +75,8 @@ func TestSupervisorLogin(t *testing.T) { }, idpv1alpha1.PhaseReady) }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" @@ -110,7 +111,8 @@ func TestSupervisorLogin(t *testing.T) { }, idpv1alpha1.PhaseReady) }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" @@ -148,7 +150,8 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" @@ -214,10 +217,12 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) - customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Subject = "not-right" }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( @@ -285,10 +290,12 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) - customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Extra["username"] = "not-the-same" }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( @@ -356,11 +363,6 @@ func TestSupervisorLogin(t *testing.T) { true, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { - require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) - require.NotEmpty(t, customSessionData.LDAP.UserDN) - customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" - }, wantErrorDescription: "The resource owner or authorization server denied the request. Username/password not accepted by LDAP provider.", wantErrorType: "access_denied", }, @@ -438,7 +440,8 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" @@ -541,7 +544,8 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) customSessionData.LDAP.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" @@ -600,10 +604,12 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) - customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Extra["username"] = "not-the-same" }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( @@ -672,10 +678,12 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) - customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Subject = "not-right" }, // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( @@ -749,7 +757,8 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" @@ -841,7 +850,8 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, customSessionData *psession.CustomSessionData) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) customSessionData.ActiveDirectory.UserDN = "cn=not-a-user,dc=pinniped,dc=dev" @@ -1045,7 +1055,7 @@ func testSupervisorLogin( t *testing.T, createIDP func(t *testing.T), requestAuthorization func(t *testing.T, downstreamAuthorizeURL, downstreamCallbackURL string, httpClient *http.Client), - breakRefreshSessionData func(t *testing.T, customSessionData *psession.CustomSessionData), + breakRefreshSessionData func(t *testing.T, pinnipedSession *psession.PinnipedSession), wantDownstreamIDTokenSubjectToMatch, wantDownstreamIDTokenUsernameToMatch string, wantDownstreamIDTokenGroups []string, wantErrorDescription string, wantErrorType string, ) { @@ -1227,7 +1237,7 @@ func testSupervisorLogin( // Next mutate the part of the session that is used during upstream refresh. pinnipedSession, ok := storedRefreshSession.GetSession().(*psession.PinnipedSession) require.True(t, ok, "should have been able to cast session data to PinnipedSession") - breakRefreshSessionData(t, pinnipedSession.Custom) + breakRefreshSessionData(t, pinnipedSession) // Then save the mutated Secret back to Kubernetes. // There is no update function, so delete and create again at the same name. From 84edfcb541982b703048d4abec20477d4db9801f Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 26 Oct 2021 17:03:16 -0700 Subject: [PATCH 06/10] Refactor out a function, add tests for getting the wrong idp uid --- internal/oidc/token/token_handler.go | 24 +++++--- internal/oidc/token/token_handler_test.go | 70 ++++++++++++++++++++--- internal/upstreamldap/upstreamldap.go | 7 +-- 3 files changed, 80 insertions(+), 21 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index d26b8642..302321b3 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -75,15 +75,10 @@ func NewHandler( func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, providerCache oidc.UpstreamIdentityProvidersLister) error { session := accessRequest.GetSession().(*psession.PinnipedSession) - extra := session.Fosite.Claims.Extra - if extra == nil { - return errorsx.WithStack(errMissingUpstreamSessionInternalError) + downstreamUsername, err := getDownstreamUsernameFromPinnipedSession(session) + if err != nil { + return err } - downstreamUsernameInterface := extra["username"] - if downstreamUsernameInterface == nil { - return errorsx.WithStack(errMissingUpstreamSessionInternalError) - } - downstreamUsername := downstreamUsernameInterface.(string) downstreamSubject := session.Fosite.Claims.Subject customSessionData := session.Custom @@ -225,3 +220,16 @@ func findLDAPProviderByNameAndValidateUID( return nil, "", errorsx.WithStack(errUpstreamRefreshError. WithHintf("Provider %q of type %q from upstream session data was not found.", s.ProviderName, s.ProviderType)) } + +func getDownstreamUsernameFromPinnipedSession(session *psession.PinnipedSession) (string, error) { + extra := session.Fosite.Claims.Extra + if extra == nil { + return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) + } + downstreamUsernameInterface := extra["username"] + if downstreamUsernameInterface == nil { + return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) + } + downstreamUsername := downstreamUsernameInterface.(string) + return downstreamUsername, nil +} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 0cec17b6..f8a7ccd4 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1644,10 +1644,10 @@ func TestRefreshGrant(t *testing.T) { }, want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( &psession.CustomSessionData{ - ProviderUID: activeDirectoryUpstreamResourceUID, - ProviderName: activeDirectoryUpstreamName, - ProviderType: activeDirectoryUpstreamType, - LDAP: nil, + ProviderUID: activeDirectoryUpstreamResourceUID, + ProviderName: activeDirectoryUpstreamName, + ProviderType: activeDirectoryUpstreamType, + ActiveDirectory: nil, }, ), }, @@ -1705,9 +1705,9 @@ func TestRefreshGrant(t *testing.T) { }, { name: "upstream active directory refresh when the active directory session data does not contain dn", - idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ - Name: ldapUpstreamName, - ResourceUID: ldapUpstreamResourceUID, + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: activeDirectoryUpstreamName, + ResourceUID: activeDirectoryUpstreamResourceUID, URL: ldapUpstreamURL, }), authcodeExchange: authcodeExchangeInputs{ @@ -1716,7 +1716,7 @@ func TestRefreshGrant(t *testing.T) { ProviderUID: ldapUpstreamResourceUID, ProviderName: ldapUpstreamName, ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ + ActiveDirectory: &psession.ActiveDirectorySessionData{ UserDN: "", }, }, @@ -1725,7 +1725,7 @@ func TestRefreshGrant(t *testing.T) { ProviderUID: ldapUpstreamResourceUID, ProviderName: ldapUpstreamName, ProviderType: ldapUpstreamType, - LDAP: &psession.LDAPSessionData{ + ActiveDirectory: &psession.ActiveDirectorySessionData{ UserDN: "", }, }, @@ -1922,6 +1922,58 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "when the ldap provider in the session storage is found but has the wrong resource UID during the refresh request", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: "the-wrong-uid", + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Provider 'some-ldap-idp' of type 'ldap' from upstream session data has changed its resource UID since authentication." + } + `), + }, + }, + }, + { + name: "when the active directory provider in the session storage is found but has the wrong resource UID during the refresh request", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithActiveDirectory(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: activeDirectoryUpstreamName, + ResourceUID: "the-wrong-uid", + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyActiveDirectoryCustomSessionData, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyActiveDirectoryCustomSessionData, + ), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Provider 'some-ad-idp' of type 'activedirectory' from upstream session data has changed its resource UID since authentication." + } + `), + }, + }, + }, } for _, test := range tests { test := test diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 33ca62f5..976b96b8 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -210,7 +210,7 @@ func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUs newUsername, err := p.getSearchResultAttributeValue(p.c.UserSearch.UsernameAttribute, userEntry, userDN) if err != nil { - return err // TODO test having no values or more than one maybe + return err } if newUsername != expectedUsername { return fmt.Errorf(`searching for user "%s" returned a different username than the previous value. expected: "%s", actual: "%s"`, @@ -220,15 +220,14 @@ func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUs newUID, err := p.getSearchResultAttributeRawValueEncoded(p.c.UserSearch.UIDAttribute, userEntry, userDN) if err != nil { - return err // TODO test + return err } newSubject := downstreamsession.DownstreamLDAPSubject(newUID, *p.GetURL()) if newSubject != expectedSubject { return fmt.Errorf(`searching for user "%s" produced a different subject than the previous value. expected: "%s", actual: "%s"`, userDN, expectedSubject, newSubject) } - // do nothing. if we got exactly one search result back then that means the user - // still exists. + // we checked that the user still exists and their information is the same, so just return. return nil } From f988879b6eae5a106d7cb800409fa1226d8c7f11 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 3 Nov 2021 10:33:22 -0700 Subject: [PATCH 07/10] Addressing code review changes - changed to use custom authenticators.Response rather than the k8s one that doesn't include space for a DN - Added more checking for correct idp type in token handler - small style changes Signed-off-by: Margo Crawford --- internal/authenticators/authenticators.go | 9 +- internal/oidc/auth/auth_handler.go | 15 +--- internal/oidc/auth/auth_handler_test.go | 10 +-- .../provider/dynamic_upstream_idp_provider.go | 2 +- internal/oidc/token/token_handler.go | 9 +- internal/oidc/token/token_handler_test.go | 84 +++++++++++++++++++ .../testutil/oidctestutil/oidctestutil.go | 8 +- internal/upstreamldap/upstreamldap.go | 13 ++- internal/upstreamldap/upstreamldap_test.go | 13 ++- test/integration/ldap_client_test.go | 3 +- 10 files changed, 125 insertions(+), 41 deletions(-) diff --git a/internal/authenticators/authenticators.go b/internal/authenticators/authenticators.go index bd24ff0b..f7a59e33 100644 --- a/internal/authenticators/authenticators.go +++ b/internal/authenticators/authenticators.go @@ -7,7 +7,7 @@ package authenticators import ( "context" - "k8s.io/apiserver/pkg/authentication/authenticator" + "k8s.io/apiserver/pkg/authentication/user" ) // This interface is similar to the k8s token authenticator, but works with username/passwords instead @@ -31,5 +31,10 @@ import ( // See k8s.io/apiserver/pkg/authentication/authenticator/interfaces.go for the token authenticator // interface, as well as the Response type. type UserAuthenticator interface { - AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) + AuthenticateUser(ctx context.Context, username, password string) (*Response, bool, error) +} + +type Response struct { + User user.Info + DN string } diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index aaac4926..bbfdba75 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -15,9 +15,9 @@ import ( "github.com/ory/fosite/token/jwt" "github.com/pkg/errors" "golang.org/x/oauth2" - "k8s.io/apiserver/pkg/authentication/authenticator" supervisoroidc "go.pinniped.dev/generated/latest/apis/supervisor/oidc" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/oidc" @@ -112,7 +112,7 @@ func handleAuthRequestForLDAPUpstream( subject := downstreamSubjectFromUpstreamLDAP(ldapUpstream, authenticateResponse) username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() - dn := userDNFromAuthenticatedResponse(authenticateResponse) + dn := authenticateResponse.DN customSessionData := &psession.CustomSessionData{ ProviderUID: ldapUpstream.GetResourceUID(), @@ -482,16 +482,7 @@ func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken return nil } -func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticator.Response) string { +func downstreamSubjectFromUpstreamLDAP(ldapUpstream provider.UpstreamLDAPIdentityProviderI, authenticateResponse *authenticators.Response) string { ldapURL := *ldapUpstream.GetURL() return downstreamsession.DownstreamLDAPSubject(authenticateResponse.User.GetUID(), ldapURL) } - -func userDNFromAuthenticatedResponse(authenticatedResponse *authenticator.Response) string { - // We should always have something here, but do some error checking anyway so it doesn't panic - dnSlice := authenticatedResponse.User.GetExtra()["userDN"] - if len(dnSlice) != 1 { - return "" - } - return dnSlice[0] -} diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 39264037..564ef380 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -19,12 +19,12 @@ import ( "github.com/ory/fosite" "github.com/stretchr/testify/require" "golang.org/x/oauth2" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/utils/pointer" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/csrftoken" @@ -273,18 +273,18 @@ func TestAuthorizationEndpoint(t *testing.T) { parsedUpstreamLDAPURL, err := url.Parse(upstreamLDAPURL) require.NoError(t, err) - ldapAuthenticateFunc := func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { + ldapAuthenticateFunc := func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { if username == "" || password == "" { return nil, false, fmt.Errorf("should not have passed empty username or password to the authenticator") } if username == happyLDAPUsername && password == happyLDAPPassword { - return &authenticator.Response{ + return &authenticators.Response{ User: &user.DefaultInfo{ Name: happyLDAPUsernameFromAuthenticator, UID: happyLDAPUID, Groups: happyLDAPGroups, - Extra: map[string][]string{"userDN": {happyLDAPUserDN}}, }, + DN: happyLDAPUserDN, }, true, nil } return nil, false, nil @@ -307,7 +307,7 @@ func TestAuthorizationEndpoint(t *testing.T) { erroringUpstreamLDAPIdentityProvider := oidctestutil.TestUpstreamLDAPIdentityProvider{ Name: ldapUpstreamName, ResourceUID: ldapUpstreamResourceUID, - AuthenticateFunc: func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { + AuthenticateFunc: func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { return nil, false, fmt.Errorf("some ldap upstream auth error") }, } diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index d7aa02c0..b5f2c215 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -90,7 +90,7 @@ type UpstreamLDAPIdentityProviderI interface { authenticators.UserAuthenticator // PerformRefresh performs a refresh against the upstream LDAP identity provider - PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error + PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error } type DynamicUpstreamIDPProvider interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 302321b3..c2eebc16 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -172,7 +172,9 @@ func findOIDCProviderByNameAndValidateUID( func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister, username string, subject string) error { // if you have neither a valid ldap session config nor a valid active directory session config - if (s.LDAP == nil || s.LDAP.UserDN == "") && (s.ActiveDirectory == nil || s.ActiveDirectory.UserDN == "") { + validLDAP := s.ProviderType == psession.ProviderTypeLDAP && s.LDAP != nil && s.LDAP.UserDN != "" + validAD := s.ProviderType == psession.ProviderTypeActiveDirectory && s.ActiveDirectory != nil && s.ActiveDirectory.UserDN != "" + if !(validLDAP || validAD) { return errorsx.WithStack(errMissingUpstreamSessionInternalError) } @@ -230,6 +232,9 @@ func getDownstreamUsernameFromPinnipedSession(session *psession.PinnipedSession) if downstreamUsernameInterface == nil { return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) } - downstreamUsername := downstreamUsernameInterface.(string) + downstreamUsername, ok := downstreamUsernameInterface.(string) + if !ok || len(downstreamUsername) == 0 { + return "", errorsx.WithStack(errMissingUpstreamSessionInternalError) + } return downstreamUsername, nil } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index f8a7ccd4..773ff5a6 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1922,6 +1922,90 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "username in extra is not a string", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + //fositeSessionData: &openid.DefaultSession{}, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Extra: map[string]interface{}{"username": 123}, + }, + } + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, + { + name: "username in extra is an empty string", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ + Name: ldapUpstreamName, + ResourceUID: ldapUpstreamResourceUID, + URL: ldapUpstreamURL, + }), + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + customSessionData: happyLDAPCustomSessionData, + //fositeSessionData: &openid.DefaultSession{}, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess( + happyLDAPCustomSessionData, + ), + }, + modifyRefreshTokenStorage: func(t *testing.T, oauthStore *oidc.KubeStorage, refreshToken string) { + refreshTokenSignature := getFositeDataSignature(t, refreshToken) + firstRequester, err := oauthStore.GetRefreshTokenSession(context.Background(), refreshTokenSignature, nil) + require.NoError(t, err) + session := firstRequester.GetSession().(*psession.PinnipedSession) + session.Fosite = &openid.DefaultSession{ + Claims: &jwt.IDTokenClaims{ + Extra: map[string]interface{}{"username": ""}, + }, + } + err = oauthStore.DeleteRefreshTokenSession(context.Background(), refreshTokenSignature) + require.NoError(t, err) + err = oauthStore.CreateRefreshTokenSession(context.Background(), refreshTokenSignature, firstRequester) + require.NoError(t, err) + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusInternalServerError, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "There was an internal server error. Required upstream data not found in session." + } + `), + }, + }, + }, { name: "when the ldap provider in the session storage is found but has the wrong resource UID during the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 23b193f0..a1513011 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -21,10 +21,10 @@ import ( "gopkg.in/square/go-jose.v2" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/client-go/kubernetes/fake" v1 "k8s.io/client-go/kubernetes/typed/core/v1" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/crud" "go.pinniped.dev/internal/fositestorage/authorizationcode" "go.pinniped.dev/internal/fositestorage/openidconnect" @@ -80,7 +80,7 @@ type TestUpstreamLDAPIdentityProvider struct { Name string ResourceUID types.UID URL *url.URL - AuthenticateFunc func(ctx context.Context, username, password string) (*authenticator.Response, bool, error) + AuthenticateFunc func(ctx context.Context, username, password string) (*authenticators.Response, bool, error) performRefreshCallCount int performRefreshArgs []*PerformRefreshArgs PerformRefreshErr error @@ -96,7 +96,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetName() string { return u.Name } -func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { +func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { return u.AuthenticateFunc(ctx, username, password) } @@ -104,7 +104,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetURL() *url.URL { return u.URL } -func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error { +func (u *TestUpstreamLDAPIdentityProvider) PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error { if u.performRefreshArgs == nil { u.performRefreshArgs = make([]*PerformRefreshArgs, 0) } diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 976b96b8..4df3fde9 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -21,7 +21,6 @@ import ( "github.com/go-ldap/ldap/v3" "github.com/google/uuid" "k8s.io/apimachinery/pkg/types" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/utils/trace" @@ -170,7 +169,7 @@ func (p *Provider) GetConfig() ProviderConfig { return p.c } -func (p *Provider) PerformRefresh(ctx context.Context, userDN string, expectedUsername string, expectedSubject string) error { +func (p *Provider) PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error { t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches search := p.refreshUserSearchRequest(userDN) @@ -372,7 +371,7 @@ func (p *Provider) TestConnection(ctx context.Context) error { // authentication for a given end user's username. It runs the same logic as AuthenticateUser except it does // not bind as that user, so it does not test their password. It returns the same values that a real call to // AuthenticateUser with the correct password would return. -func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) (*authenticator.Response, bool, error) { +func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) (*authenticators.Response, bool, error) { endUserBindFunc := func(conn Conn, foundUserDN string) error { // Act as if the end user bind always succeeds. return nil @@ -381,14 +380,14 @@ func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) } // Authenticate an end user and return their mapped username, groups, and UID. Implements authenticators.UserAuthenticator. -func (p *Provider) AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) { +func (p *Provider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { endUserBindFunc := func(conn Conn, foundUserDN string) error { return conn.Bind(foundUserDN, password) } return p.authenticateUserImpl(ctx, username, endUserBindFunc) } -func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticator.Response, bool, error) { +func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) { t := trace.FromContext(ctx).Nest("slow ldap authenticate user attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches @@ -428,13 +427,13 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi return nil, false, nil } - response := &authenticator.Response{ + response := &authenticators.Response{ User: &user.DefaultInfo{ Name: mappedUsername, UID: mappedUID, Groups: mappedGroupNames, - Extra: map[string][]string{"userDN": {userDN}}, }, + DN: userDN, } p.traceAuthSuccess(t) return response, true, nil diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index c9295b8f..d04cc3dd 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -18,9 +18,9 @@ import ( "github.com/go-ldap/ldap/v3" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/endpointaddr" "go.pinniped.dev/internal/mocks/mockldapconn" @@ -151,17 +151,16 @@ func TestEndUserAuthentication(t *testing.T) { } // The auth response which matches the exampleUserSearchResult and exampleGroupSearchResult. - expectedAuthResponse := func(editFunc func(r *user.DefaultInfo)) *authenticator.Response { + expectedAuthResponse := func(editFunc func(r *user.DefaultInfo)) *authenticators.Response { u := &user.DefaultInfo{ Name: testUserSearchResultUsernameAttributeValue, UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{testGroupSearchResultGroupNameAttributeValue1, testGroupSearchResultGroupNameAttributeValue2}, - Extra: map[string][]string{"userDN": {testUserSearchResultDNValue}}, } if editFunc != nil { editFunc(u) } - return &authenticator.Response{User: u} + return &authenticators.Response{User: u, DN: testUserSearchResultDNValue} } tests := []struct { @@ -174,7 +173,7 @@ func TestEndUserAuthentication(t *testing.T) { dialError error wantError string wantToSkipDial bool - wantAuthResponse *authenticator.Response + wantAuthResponse *authenticators.Response wantUnauthenticated bool skipDryRunAuthenticateUser bool // tests about when the end user bind fails don't make sense for DryRunAuthenticateUser() }{ @@ -499,13 +498,13 @@ func TestEndUserAuthentication(t *testing.T) { bindEndUserMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) }, - wantAuthResponse: &authenticator.Response{ + wantAuthResponse: &authenticators.Response{ User: &user.DefaultInfo{ Name: testUserSearchResultUsernameAttributeValue, UID: base64.RawURLEncoding.EncodeToString([]byte(testUserSearchResultUIDAttributeValue)), Groups: []string{"a", "b", "c"}, - Extra: map[string][]string{"userDN": {testUserSearchResultDNValue}}, }, + DN: testUserSearchResultDNValue, }, }, { diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 8cafa810..5531a978 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -20,6 +20,7 @@ import ( "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/upstreamldap" "go.pinniped.dev/test/testlib" ) @@ -677,7 +678,7 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) { } type authUserResult struct { - response *authenticator.Response + response *authenticators.Response authenticated bool err error } From c84329d7a4ee6cc1927fb74d16b5ed5327ee57ba Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 3 Nov 2021 11:41:29 -0700 Subject: [PATCH 08/10] Fix broken ldap_client_test --- test/integration/ldap_client_test.go | 84 ++++++++++++++-------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index 5531a978..4bc268e2 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -17,7 +17,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "go.pinniped.dev/internal/authenticators" @@ -75,7 +74,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { password string provider *upstreamldap.Provider wantError string - wantAuthResponse *authenticator.Response + wantAuthResponse *authenticators.Response wantUnauthenticated bool }{ { @@ -83,8 +82,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(nil)), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -95,8 +94,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.Host = "127.0.0.1:" + ldapLocalhostPort p.ConnectionProtocol = upstreamldap.StartTLS })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -104,8 +103,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Base = "dc=pinniped,dc=dev" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -113,8 +112,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Filter = "(cn={})" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -125,8 +124,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.UsernameAttribute = "dn" p.UserSearch.Filter = "cn={}" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "cn=pinny,ou=users,dc=pinniped,dc=dev", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "cn=pinny,ou=users,dc=pinniped,dc=dev", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -136,8 +135,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Filter = "(|(cn={})(mail={}))" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -147,8 +146,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.Filter = "(|(cn={})(mail={}))" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -156,8 +155,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UIDAttribute = "dn" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("cn=pinny,ou=users,dc=pinniped,dc=dev"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("cn=pinny,ou=users,dc=pinniped,dc=dev"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -165,8 +164,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { username: "pinny", password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UIDAttribute = "sn" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("Seal"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("Seal"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -174,8 +173,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { username: "seAl", // note that this is not case-sensitive! sn=Seal. The server decides which fields are compared case-sensitive. password: pinnyPassword, provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.UserSearch.UsernameAttribute = "sn" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "Seal", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, // note that the final answer has case preserved from the entry + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "Seal", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", // note that the final answer has case preserved from the entry }, }, { @@ -187,8 +186,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.UsernameAttribute = "givenName" p.UserSearch.UIDAttribute = "givenName" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "Pinny the 🦭", UID: b64("Pinny the 🦭"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "Pinny the 🦭", UID: b64("Pinny the 🦭"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -199,8 +198,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { p.UserSearch.Filter = "givenName={}" p.UserSearch.UsernameAttribute = "cn" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -220,8 +219,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.GroupSearch.Base = "" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -231,8 +230,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.GroupSearch.Base = "ou=users,dc=pinniped,dc=dev" // there are no groups under this part of the tree })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -242,11 +241,11 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.GroupSearch.GroupNameAttribute = "dn" })), - wantAuthResponse: &authenticator.Response{ + wantAuthResponse: &authenticators.Response{ User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{ "cn=ball-game-players,ou=beach-groups,ou=groups,dc=pinniped,dc=dev", "cn=seals,ou=groups,dc=pinniped,dc=dev", - }, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + }}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -256,11 +255,11 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.GroupSearch.GroupNameAttribute = "" })), - wantAuthResponse: &authenticator.Response{ + wantAuthResponse: &authenticators.Response{ User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{ "cn=ball-game-players,ou=beach-groups,ou=groups,dc=pinniped,dc=dev", "cn=seals,ou=groups,dc=pinniped,dc=dev", - }, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + }}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -270,8 +269,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.GroupSearch.GroupNameAttribute = "objectClass" // silly example, but still a meaningful test })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"groupOfNames", "groupOfNames"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"groupOfNames", "groupOfNames"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -281,8 +280,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.GroupSearch.Filter = "(&(&(objectClass=groupOfNames)(member={}))(cn=seals))" })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"seals"}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -292,8 +291,8 @@ func TestLDAPSearch_Parallel(t *testing.T) { provider: upstreamldap.New(*providerConfig(func(p *upstreamldap.ProviderConfig) { p.GroupSearch.Filter = "foobar={}" // foobar is not a valid attribute name for this LDAP server's schema })), - wantAuthResponse: &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + wantAuthResponse: &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{}}, DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, }, { @@ -671,8 +670,9 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) { // Record failures but allow the test to keep running so that all the background goroutines have a chance to try. assert.NoError(t, result.err) assert.True(t, result.authenticated, "expected the user to be authenticated, but they were not") - assert.Equal(t, &authenticator.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}, Extra: map[string][]string{"userDN": {"cn=pinny,ou=users,dc=pinniped,dc=dev"}}}, + assert.Equal(t, &authenticators.Response{ + User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: []string{"ball-game-players", "seals"}}, + DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", }, result.response) } } From b5b8cab717ed54d5a24df6c8d6f551aad38c1a0a Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 3 Nov 2021 15:17:50 -0700 Subject: [PATCH 09/10] Refactors: - pull construction of authenticators.Response into searchAndBindUser - remove information about the identity provider in the error that gets returned to users. Put it in debug instead, where it may show up in logs. Signed-off-by: Margo Crawford --- internal/oidc/token/token_handler.go | 23 +++++------ internal/oidc/token/token_handler_test.go | 18 ++++----- internal/upstreamldap/upstreamldap.go | 49 +++++++++++++---------- test/integration/supervisor_login_test.go | 5 +-- 4 files changed, 48 insertions(+), 47 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index c2eebc16..c9703e6e 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -118,9 +118,9 @@ func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, pro refreshedTokens, err := p.PerformRefresh(ctx, s.OIDC.UpstreamRefreshToken) if err != nil { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh failed using provider %q of type %q.", - s.ProviderName, s.ProviderType).WithWrap(err)) + return errorsx.WithStack(errUpstreamRefreshError.WithHint( + "Upstream refresh failed.", + ).WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } // Upstream refresh may or may not return a new ID token. From the spec: @@ -133,8 +133,7 @@ func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, pro _, err = p.ValidateToken(ctx, refreshedTokens, "") if err != nil { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh returned an invalid ID token using provider %q of type %q.", - s.ProviderName, s.ProviderType).WithWrap(err)) + "Upstream refresh returned an invalid ID token.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } } else { plog.Debug("upstream refresh request did not return a new ID token", @@ -167,7 +166,7 @@ func findOIDCProviderByNameAndValidateUID( } } return nil, errorsx.WithStack(errUpstreamRefreshError. - WithHintf("Provider %q of type %q from upstream session data was not found.", s.ProviderName, s.ProviderType)) + WithHint("Provider from upstream session data was not found.").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister, username string, subject string) error { @@ -186,9 +185,8 @@ func upstreamLDAPRefresh(ctx context.Context, s *psession.CustomSessionData, pro // run PerformRefresh err = p.PerformRefresh(ctx, dn, username, subject) if err != nil { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh failed using provider %q of type %q.", - s.ProviderName, s.ProviderType).WithWrap(err)) + return errorsx.WithStack(errUpstreamRefreshError.WithHint( + "Upstream refresh failed.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } return nil @@ -211,16 +209,15 @@ func findLDAPProviderByNameAndValidateUID( for _, p := range providers { if p.GetName() == s.ProviderName { if p.GetResourceUID() != s.ProviderUID { - return nil, "", errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Provider %q of type %q from upstream session data has changed its resource UID since authentication.", - s.ProviderName, s.ProviderType)) + return nil, "", errorsx.WithStack(errUpstreamRefreshError.WithHint( + "Provider from upstream session data has changed its resource UID since authentication.").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } return p, dn, nil } } return nil, "", errorsx.WithStack(errUpstreamRefreshError. - WithHintf("Provider %q of type %q from upstream session data was not found.", s.ProviderName, s.ProviderType)) + WithHint("Provider from upstream session data was not found.").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } func getDownstreamUsernameFromPinnipedSession(session *psession.PinnipedSession) (string, error) { diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 773ff5a6..f3767bc2 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1465,7 +1465,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Provider 'this-name-will-not-be-found' of type 'oidc' from upstream session data was not found." + "error_description": "Error during upstream refresh. Provider from upstream session data was not found." } `), }, @@ -1519,7 +1519,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Upstream refresh failed using provider 'some-oidc-idp' of type 'oidc'." + "error_description": "Error during upstream refresh. Upstream refresh failed." } `), }, @@ -1545,7 +1545,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token using provider 'some-oidc-idp' of type 'oidc'." + "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token." } `), }, @@ -1765,7 +1765,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Upstream refresh failed using provider 'some-ldap-idp' of type 'ldap'." + "error_description": "Error during upstream refresh. Upstream refresh failed." } `), }, @@ -1793,7 +1793,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Upstream refresh failed using provider 'some-ad-idp' of type 'activedirectory'." + "error_description": "Error during upstream refresh. Upstream refresh failed." } `), }, @@ -1815,7 +1815,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Provider 'some-ldap-idp' of type 'ldap' from upstream session data was not found." + "error_description": "Error during upstream refresh. Provider from upstream session data was not found." } `), }, @@ -1837,7 +1837,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Provider 'some-ad-idp' of type 'activedirectory' from upstream session data was not found." + "error_description": "Error during upstream refresh. Provider from upstream session data was not found." } `), }, @@ -2026,7 +2026,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Provider 'some-ldap-idp' of type 'ldap' from upstream session data has changed its resource UID since authentication." + "error_description": "Error during upstream refresh. Provider from upstream session data has changed its resource UID since authentication." } `), }, @@ -2052,7 +2052,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Provider 'some-ad-idp' of type 'activedirectory' from upstream session data has changed its resource UID since authentication." + "error_description": "Error during upstream refresh. Provider from upstream session data has changed its resource UID since authentication." } `), }, diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 4df3fde9..ec97baca 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -416,25 +416,16 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, bi return nil, false, fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) } - mappedUsername, mappedUID, mappedGroupNames, userDN, err := p.searchAndBindUser(conn, username, bindFunc) + response, err := p.searchAndBindUser(conn, username, bindFunc) if err != nil { p.traceAuthFailure(t, err) return nil, false, err } - if len(mappedUsername) == 0 || len(mappedUID) == 0 { - // Couldn't find the username or couldn't bind using the password. + if response == nil { p.traceAuthFailure(t, fmt.Errorf("bad username or password")) return nil, false, nil } - response := &authenticators.Response{ - User: &user.DefaultInfo{ - Name: mappedUsername, - UID: mappedUID, - Groups: mappedGroupNames, - }, - DN: userDN, - } p.traceAuthSuccess(t) return response, true, nil } @@ -516,7 +507,7 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e return searchBase, nil } -func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(conn Conn, foundUserDN string) error) (string, string, []string, string, error) { +func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, error) { searchResult, err := conn.Search(p.userSearchRequest(username)) if err != nil { plog.All(`error searching for user`, @@ -524,7 +515,7 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c "username", username, "err", err, ) - return "", "", nil, "", fmt.Errorf(`error searching for user: %w`, err) + return nil, fmt.Errorf(`error searching for user: %w`, err) } if len(searchResult.Entries) == 0 { if plog.Enabled(plog.LevelAll) { @@ -535,38 +526,38 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c } else { plog.Debug("error finding user: user not found (cowardly avoiding printing username because log level is not 'all')", "upstreamName", p.GetName()) } - return "", "", nil, "", nil + return nil, nil } // At this point, we have matched at least one entry, so we can be confident that the username is not actually // someone's password mistakenly entered into the username field, so we can log it without concern. if len(searchResult.Entries) > 1 { - return "", "", nil, "", fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, + return nil, fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, username, len(searchResult.Entries), ) } userEntry := searchResult.Entries[0] if len(userEntry.DN) == 0 { - return "", "", nil, "", fmt.Errorf(`searching for user "%s" resulted in search result without DN`, username) + return nil, fmt.Errorf(`searching for user "%s" resulted in search result without DN`, username) } mappedUsername, err := p.getSearchResultAttributeValue(p.c.UserSearch.UsernameAttribute, userEntry, username) if err != nil { - return "", "", nil, "", err + return nil, err } // We would like to support binary typed attributes for UIDs, so always read them as binary and encode them, // even when the attribute may not be binary. mappedUID, err := p.getSearchResultAttributeRawValueEncoded(p.c.UserSearch.UIDAttribute, userEntry, username) if err != nil { - return "", "", nil, "", err + return nil, err } mappedGroupNames := []string{} if len(p.c.GroupSearch.Base) > 0 { mappedGroupNames, err = p.searchGroupsForUserDN(conn, userEntry.DN) if err != nil { - return "", "", nil, "", err + return nil, err } } sort.Strings(mappedGroupNames) @@ -578,12 +569,26 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(c err, "upstreamName", p.GetName(), "username", username, "dn", userEntry.DN) ldapErr := &ldap.Error{} if errors.As(err, &ldapErr) && ldapErr.ResultCode == ldap.LDAPResultInvalidCredentials { - return "", "", nil, "", nil + return nil, nil } - return "", "", nil, "", fmt.Errorf(`error binding for user "%s" using provided password against DN "%s": %w`, username, userEntry.DN, err) + return nil, fmt.Errorf(`error binding for user "%s" using provided password against DN "%s": %w`, username, userEntry.DN, err) } - return mappedUsername, mappedUID, mappedGroupNames, userEntry.DN, nil + if len(mappedUsername) == 0 || len(mappedUID) == 0 { + // Couldn't find the username or couldn't bind using the password. + return nil, nil + } + + response := &authenticators.Response{ + User: &user.DefaultInfo{ + Name: mappedUsername, + UID: mappedUID, + Groups: mappedGroupNames, + }, + DN: userEntry.DN, + } + + return response, nil } func (p *Provider) defaultNamingContextRequest() *ldap.SearchRequest { diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 106b7f5f..46420c14 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -1250,9 +1250,8 @@ func testSupervisorLogin( require.Error(t, err) require.Regexp(t, regexp.QuoteMeta("oauth2: cannot fetch token: 401 Unauthorized\n")+ - regexp.QuoteMeta(`Response: {"error":"error","error_description":"Error during upstream refresh. Upstream refresh failed using provider '`)+ - "[^']+"+ // this would be the name of the identity provider CR - regexp.QuoteMeta(fmt.Sprintf(`' of type '%s'."`, pinnipedSession.Custom.ProviderType)), + regexp.QuoteMeta(`Response: {"error":"error","error_description":"Error during upstream refresh. Upstream refresh failed`)+ + "[^']+", err.Error(), ) } From cb60a44f8af205dd760deb14f67cf67243637c54 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Fri, 5 Nov 2021 14:18:54 -0700 Subject: [PATCH 10/10] extract ldap refresh search into helper function also added an integration test for refresh failing after updating the username attribute --- internal/upstreamldap/upstreamldap.go | 42 +++--- internal/upstreamldap/upstreamldap_test.go | 4 +- test/integration/supervisor_login_test.go | 162 ++++++++++++++++----- 3 files changed, 155 insertions(+), 53 deletions(-) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index ec97baca..1f0587de 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -172,26 +172,10 @@ func (p *Provider) GetConfig() ProviderConfig { func (p *Provider) PerformRefresh(ctx context.Context, userDN, expectedUsername, expectedSubject string) error { t := trace.FromContext(ctx).Nest("slow ldap refresh attempt", trace.Field{Key: "providerName", Value: p.GetName()}) defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches - search := p.refreshUserSearchRequest(userDN) - - conn, err := p.dial(ctx) + searchResult, err := p.performRefresh(ctx, userDN) if err != nil { p.traceRefreshFailure(t, err) - return fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) - } - defer conn.Close() - - err = conn.Bind(p.c.BindUsername, p.c.BindPassword) - if err != nil { - p.traceRefreshFailure(t, err) - return fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) - } - - searchResult, err := conn.Search(search) - - if err != nil { - p.traceRefreshFailure(t, err) - return fmt.Errorf(`error searching for user "%s": %w`, userDN, err) + return err } // if any more or less than one entry, error. @@ -230,6 +214,28 @@ func (p *Provider) PerformRefresh(ctx context.Context, userDN, expectedUsername, return nil } +func (p *Provider) performRefresh(ctx context.Context, userDN string) (*ldap.SearchResult, error) { + search := p.refreshUserSearchRequest(userDN) + + conn, err := p.dial(ctx) + if err != nil { + return nil, fmt.Errorf(`error dialing host "%s": %w`, p.c.Host, err) + } + defer conn.Close() + + err = conn.Bind(p.c.BindUsername, p.c.BindPassword) + if err != nil { + return nil, fmt.Errorf(`error binding as "%s" before user search: %w`, p.c.BindUsername, err) + } + + searchResult, err := conn.Search(search) + + if err != nil { + return nil, fmt.Errorf(`error searching for user "%s": %w`, userDN, err) + } + return searchResult, nil +} + func (p *Provider) dial(ctx context.Context) (Conn, error) { tlsAddr, err := endpointaddr.Parse(p.c.Host, defaultLDAPSPort) if err != nil { diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index d04cc3dd..6a9eca34 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -1511,8 +1511,8 @@ func TestUpstreamRefresh(t *testing.T) { }, } - for _, test := range tests { - tt := test + for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 46420c14..43b45e78 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -44,7 +44,7 @@ func TestSupervisorLogin(t *testing.T) { tests := []struct { name string maybeSkip func(t *testing.T) - createIDP func(t *testing.T) + createIDP func(t *testing.T) string requestAuthorization func(t *testing.T, downstreamAuthorizeURL, downstreamCallbackURL string, httpClient *http.Client) wantDownstreamIDTokenSubjectToMatch string wantDownstreamIDTokenUsernameToMatch string @@ -55,16 +55,16 @@ func TestSupervisorLogin(t *testing.T) { // We don't necessarily have any way to revoke the user's session on the upstream provider, // so to cause the upstream refresh to fail we can cheat by manipulating the user's session // data in such a way that it should cause the next upstream refresh attempt to fail. - breakRefreshSessionData func(t *testing.T, sessionData *psession.PinnipedSession) + breakRefreshSessionData func(t *testing.T, sessionData *psession.PinnipedSession, idpName string) }{ { name: "oidc with default username and groups claim settings", maybeSkip: func(t *testing.T) { // never need to skip this test }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() - testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ + oidcIDP := testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ Issuer: env.SupervisorUpstreamOIDC.Issuer, TLS: &idpv1alpha1.TLSSpec{ CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)), @@ -73,9 +73,10 @@ func TestSupervisorLogin(t *testing.T) { SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name, }, }, idpv1alpha1.PhaseReady) + return oidcIDP.Name }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) @@ -91,9 +92,9 @@ func TestSupervisorLogin(t *testing.T) { maybeSkip: func(t *testing.T) { // never need to skip this test }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() - testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ + oidcIDP := testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ Issuer: env.SupervisorUpstreamOIDC.Issuer, TLS: &idpv1alpha1.TLSSpec{ CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)), @@ -109,9 +110,10 @@ func TestSupervisorLogin(t *testing.T) { AdditionalScopes: env.SupervisorUpstreamOIDC.AdditionalScopes, }, }, idpv1alpha1.PhaseReady) + return oidcIDP.Name }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) @@ -126,9 +128,9 @@ func TestSupervisorLogin(t *testing.T) { maybeSkip: func(t *testing.T) { // never need to skip this test }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() - testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ + oidcIDP := testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{ Issuer: env.SupervisorUpstreamOIDC.Issuer, TLS: &idpv1alpha1.TLSSpec{ CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)), @@ -140,6 +142,7 @@ func TestSupervisorLogin(t *testing.T) { AllowPasswordGrant: true, // allow the CLI password flow for this OIDCIdentityProvider }, }, idpv1alpha1.PhaseReady) + return oidcIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -150,7 +153,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) @@ -169,7 +172,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("LDAP integration test requires connectivity to an LDAP server") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, map[string]string{ @@ -207,6 +210,7 @@ func TestSupervisorLogin(t *testing.T) { secret.Name, secret.ResourceVersion, ) requireSuccessfulLDAPIdentityProviderConditions(t, ldapIDP, expectedMsg) + return ldapIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -217,7 +221,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) @@ -242,7 +246,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("LDAP integration test requires connectivity to an LDAP server") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, map[string]string{ @@ -280,6 +284,7 @@ func TestSupervisorLogin(t *testing.T) { secret.Name, secret.ResourceVersion, ) requireSuccessfulLDAPIdentityProviderConditions(t, ldapIDP, expectedMsg) + return ldapIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -290,7 +295,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) @@ -315,7 +320,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("LDAP integration test requires connectivity to an LDAP server") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, map[string]string{ @@ -353,6 +358,7 @@ func TestSupervisorLogin(t *testing.T) { secret.Name, secret.ResourceVersion, ) requireSuccessfulLDAPIdentityProviderConditions(t, ldapIDP, expectedMsg) + return ldapIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -374,7 +380,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("LDAP integration test requires connectivity to an LDAP server") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, @@ -430,6 +436,7 @@ func TestSupervisorLogin(t *testing.T) { requireEventually.NoError(err) requireEventuallySuccessfulLDAPIdentityProviderConditions(t, requireEventually, ldapIDP, expectedMsg) }, time.Minute, 500*time.Millisecond) + return ldapIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -440,7 +447,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) @@ -464,7 +471,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("LDAP integration test requires connectivity to an LDAP server") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, @@ -534,6 +541,7 @@ func TestSupervisorLogin(t *testing.T) { requireEventually.NoError(err) requireEventuallySuccessfulLDAPIdentityProviderConditions(t, requireEventually, ldapIDP, expectedMsg) }, time.Minute, 500*time.Millisecond) + return ldapIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -544,7 +552,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeLDAP, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.LDAP.UserDN) @@ -571,7 +579,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("Active Directory hostname not specified") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ad-service-account", v1.SecretTypeBasicAuth, map[string]string{ @@ -594,6 +602,7 @@ func TestSupervisorLogin(t *testing.T) { secret.Name, secret.ResourceVersion, ) requireSuccessfulActiveDirectoryIdentityProviderConditions(t, adIDP, expectedMsg) + return adIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -604,7 +613,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) @@ -631,7 +640,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("Active Directory hostname not specified") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ad-service-account", v1.SecretTypeBasicAuth, map[string]string{ @@ -668,6 +677,7 @@ func TestSupervisorLogin(t *testing.T) { secret.Name, secret.ResourceVersion, ) requireSuccessfulActiveDirectoryIdentityProviderConditions(t, adIDP, expectedMsg) + return adIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -678,7 +688,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) @@ -706,7 +716,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("Active Directory hostname not specified") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ad-service-account", v1.SecretTypeBasicAuth, @@ -747,6 +757,7 @@ func TestSupervisorLogin(t *testing.T) { requireEventually.NoError(err) requireEventuallySuccessfulActiveDirectoryIdentityProviderConditions(t, requireEventually, adIDP, expectedMsg) }, time.Minute, 500*time.Millisecond) + return adIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -757,7 +768,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) @@ -784,7 +795,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("Active Directory hostname not specified") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ad-service-account", v1.SecretTypeBasicAuth, @@ -840,6 +851,7 @@ func TestSupervisorLogin(t *testing.T) { requireEventually.NoError(err) requireEventuallySuccessfulActiveDirectoryIdentityProviderConditions(t, requireEventually, adIDP, expectedMsg) }, time.Minute, 500*time.Millisecond) + return adIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -850,7 +862,7 @@ func TestSupervisorLogin(t *testing.T) { false, ) }, - breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession) { + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _ string) { customSessionData := pinnipedSession.Custom require.Equal(t, psession.ProviderTypeActiveDirectory, customSessionData.ProviderType) require.NotEmpty(t, customSessionData.ActiveDirectory.UserDN) @@ -877,7 +889,7 @@ func TestSupervisorLogin(t *testing.T) { t.Skip("Active Directory hostname not specified") } }, - createIDP: func(t *testing.T) { + createIDP: func(t *testing.T) string { t.Helper() secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ad-service-account", v1.SecretTypeBasicAuth, map[string]string{ @@ -900,6 +912,7 @@ func TestSupervisorLogin(t *testing.T) { secret.Name, secret.ResourceVersion, ) requireSuccessfulActiveDirectoryIdentityProviderConditions(t, adIDP, expectedMsg) + return adIDP.Name }, requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { requestAuthorizationUsingCLIPasswordFlow(t, @@ -914,6 +927,89 @@ func TestSupervisorLogin(t *testing.T) { wantErrorDescription: "The resource owner or authorization server denied the request. Username/password not accepted by LDAP provider.", wantErrorType: "access_denied", }, + { + name: "ldap refresh fails when username changes from email as username to dn as username", + maybeSkip: func(t *testing.T) { + t.Helper() + if len(env.ToolsNamespace) == 0 && !env.HasCapability(testlib.CanReachInternetLDAPPorts) { + t.Skip("LDAP integration test requires connectivity to an LDAP server") + } + }, + createIDP: func(t *testing.T) string { + t.Helper() + secret := testlib.CreateTestSecret(t, env.SupervisorNamespace, "ldap-service-account", v1.SecretTypeBasicAuth, + map[string]string{ + v1.BasicAuthUsernameKey: env.SupervisorUpstreamLDAP.BindUsername, + v1.BasicAuthPasswordKey: env.SupervisorUpstreamLDAP.BindPassword, + }, + ) + ldapIDP := testlib.CreateTestLDAPIdentityProvider(t, idpv1alpha1.LDAPIdentityProviderSpec{ + Host: env.SupervisorUpstreamLDAP.Host, + TLS: &idpv1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamLDAP.CABundle)), + }, + Bind: idpv1alpha1.LDAPIdentityProviderBind{ + SecretName: secret.Name, + }, + UserSearch: idpv1alpha1.LDAPIdentityProviderUserSearch{ + Base: env.SupervisorUpstreamLDAP.UserSearchBase, + Filter: "", + Attributes: idpv1alpha1.LDAPIdentityProviderUserSearchAttributes{ + Username: env.SupervisorUpstreamLDAP.TestUserMailAttributeName, + UID: env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeName, + }, + }, + GroupSearch: idpv1alpha1.LDAPIdentityProviderGroupSearch{ + Base: env.SupervisorUpstreamLDAP.GroupSearchBase, + Filter: "", + Attributes: idpv1alpha1.LDAPIdentityProviderGroupSearchAttributes{ + GroupName: "dn", + }, + }, + }, idpv1alpha1.LDAPPhaseReady) + expectedMsg := fmt.Sprintf( + `successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`, + env.SupervisorUpstreamLDAP.Host, env.SupervisorUpstreamLDAP.BindUsername, + secret.Name, secret.ResourceVersion, + ) + requireSuccessfulLDAPIdentityProviderConditions(t, ldapIDP, expectedMsg) + return ldapIDP.Name + }, + requestAuthorization: func(t *testing.T, downstreamAuthorizeURL, _ string, httpClient *http.Client) { + requestAuthorizationUsingCLIPasswordFlow(t, + downstreamAuthorizeURL, + env.SupervisorUpstreamLDAP.TestUserMailAttributeValue, // username to present to server during login + env.SupervisorUpstreamLDAP.TestUserPassword, // password to present to server during login + httpClient, + false, + ) + }, + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, idpName string) { + // get the idp, update the config. + client := testlib.NewSupervisorClientset(t) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + // Create the LDAPIdentityProvider using GenerateName to get a random name. + upstreams := client.IDPV1alpha1().LDAPIdentityProviders(env.SupervisorNamespace) + ldapIDP, err := upstreams.Get(ctx, idpName, metav1.GetOptions{}) + require.NoError(t, err) + ldapIDP.Spec.UserSearch.Attributes.Username = "dn" + + _, err = upstreams.Update(ctx, ldapIDP, metav1.UpdateOptions{}) + require.NoError(t, err) + time.Sleep(10 * time.Second) // wait for controllers to pick up the change + }, + // the ID token Subject should be the Host URL plus the value pulled from the requested UserSearch.Attributes.UID attribute + wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta( + "ldaps://"+env.SupervisorUpstreamLDAP.Host+ + "?base="+url.QueryEscape(env.SupervisorUpstreamLDAP.UserSearchBase)+ + "&sub="+base64.RawURLEncoding.EncodeToString([]byte(env.SupervisorUpstreamLDAP.TestUserUniqueIDAttributeValue)), + ) + "$", + // the ID token Username should have been pulled from the requested UserSearch.Attributes.Username attribute + wantDownstreamIDTokenUsernameToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamLDAP.TestUserMailAttributeValue) + "$", + wantDownstreamIDTokenGroups: env.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs, + }, } for _, test := range tests { tt := test @@ -1053,9 +1149,9 @@ func requireEventuallySuccessfulActiveDirectoryIdentityProviderConditions(t *tes func testSupervisorLogin( t *testing.T, - createIDP func(t *testing.T), + createIDP func(t *testing.T) string, requestAuthorization func(t *testing.T, downstreamAuthorizeURL, downstreamCallbackURL string, httpClient *http.Client), - breakRefreshSessionData func(t *testing.T, pinnipedSession *psession.PinnipedSession), + breakRefreshSessionData func(t *testing.T, pinnipedSession *psession.PinnipedSession, idpName string), wantDownstreamIDTokenSubjectToMatch, wantDownstreamIDTokenUsernameToMatch string, wantDownstreamIDTokenGroups []string, wantErrorDescription string, wantErrorType string, ) { @@ -1143,7 +1239,7 @@ func testSupervisorLogin( }, 30*time.Second, 200*time.Millisecond) // Create upstream IDP and wait for it to become ready. - createIDP(t) + idpName := createIDP(t) // Perform OIDC discovery for our downstream. var discovery *coreosoidc.Provider @@ -1237,7 +1333,7 @@ func testSupervisorLogin( // Next mutate the part of the session that is used during upstream refresh. pinnipedSession, ok := storedRefreshSession.GetSession().(*psession.PinnipedSession) require.True(t, ok, "should have been able to cast session data to PinnipedSession") - breakRefreshSessionData(t, pinnipedSession) + breakRefreshSessionData(t, pinnipedSession, idpName) // Then save the mutated Secret back to Kubernetes. // There is no update function, so delete and create again at the same name.