diff --git a/internal/authenticators/authenticators.go b/internal/authenticators/authenticators.go index 80bc3c5b7..dcddb9f87 100644 --- a/internal/authenticators/authenticators.go +++ b/internal/authenticators/authenticators.go @@ -31,7 +31,7 @@ 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, skipGroups bool) (*Response, bool, error) + AuthenticateUser(ctx context.Context, username, password string) (*Response, bool, error) } type Response struct { diff --git a/internal/federationdomain/endpoints/auth/auth_handler.go b/internal/federationdomain/endpoints/auth/auth_handler.go index 49223936d..5afcab382 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler.go +++ b/internal/federationdomain/endpoints/auth/auth_handler.go @@ -13,7 +13,6 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/openid" "github.com/ory/fosite/token/jwt" - "k8s.io/utils/strings/slices" oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc" "go.pinniped.dev/internal/federationdomain/csrftoken" @@ -202,9 +201,7 @@ func (h *authorizeHandler) authorizeWithoutBrowser( return err } - groupsWillBeIgnored := !slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups) - - identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword, groupsWillBeIgnored) + identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword) if err != nil { return err } diff --git a/internal/federationdomain/endpoints/login/post_login_handler.go b/internal/federationdomain/endpoints/login/post_login_handler.go index f510e74b8..07feb9f5e 100644 --- a/internal/federationdomain/endpoints/login/post_login_handler.go +++ b/internal/federationdomain/endpoints/login/post_login_handler.go @@ -9,9 +9,7 @@ import ( "net/url" "github.com/ory/fosite" - "k8s.io/utils/strings/slices" - oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc" "go.pinniped.dev/internal/federationdomain/downstreamsession" "go.pinniped.dev/internal/federationdomain/endpoints/loginurl" "go.pinniped.dev/internal/federationdomain/federationdomainproviders" @@ -67,10 +65,8 @@ func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.Fed return redirectToLoginPage(r, w, issuerURL, encodedState, loginurl.ShowBadUserPassErr) } - skipGroups := !slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups) - // Attempt to authenticate the user with the upstream IDP. - identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword, skipGroups) + identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword) if err != nil { switch { case errors.Is(err, resolvedldap.ErrUnexpectedUpstreamLDAPError): diff --git a/internal/federationdomain/endpoints/login/post_login_handler_test.go b/internal/federationdomain/endpoints/login/post_login_handler_test.go index cb894ca5c..789772f3b 100644 --- a/internal/federationdomain/endpoints/login/post_login_handler_test.go +++ b/internal/federationdomain/endpoints/login/post_login_handler_test.go @@ -20,6 +20,7 @@ import ( configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" "go.pinniped.dev/internal/authenticators" + "go.pinniped.dev/internal/celtransformer" "go.pinniped.dev/internal/federationdomain/endpoints/jwks" "go.pinniped.dev/internal/federationdomain/oidc" "go.pinniped.dev/internal/federationdomain/oidcclientvalidator" @@ -674,6 +675,44 @@ func TestPostLoginEndpoint(t *testing.T) { wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantDownstreamCustomSessionData: expectedHappyLDAPUpstreamCustomSession, }, + { + name: "login with dynamic client which is not allowed to request groups still runs identity transformations with upstream groups " + + "in case transforms want to reject auth based on groups, even though groups would not be included in final ID token", + idps: testidplister.NewUpstreamIDPListerBuilder(). + WithLDAP(upstreamLDAPIdentityProviderBuilder. + WithTransformsForFederationDomain(transformtestutil.NewPipeline(t, + []celtransformer.CELTransformation{ + &celtransformer.AllowAuthenticationPolicy{ + Expression: `!groups.exists(g, g in ["` + happyLDAPGroups[0] + `"])`, // reject auth for users who belongs to an upstream group + RejectedAuthenticationMessage: `users who belong to certain upstream group are not allowed`, + }, + }), + ).Build()), + kubeResources: func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) { + oidcClient, secret := testutil.OIDCClientAndStorageSecret(t, + "some-namespace", downstreamDynamicClientID, downstreamDynamicClientUID, + []configv1alpha1.GrantType{"authorization_code", "refresh_token"}, // token exchange not allowed (required to exclude groups scope) + []configv1alpha1.Scope{"openid", "offline_access", "username"}, // groups not allowed + downstreamRedirectURI, []string{testutil.HashedPassword1AtGoMinCost}, oidcclientvalidator.Validate) + require.NoError(t, supervisorClient.Tracker().Add(oidcClient)) + require.NoError(t, kubeClient.Tracker().Add(secret)) + }, + decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) { + data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient, + map[string]string{"scope": "openid username offline_access"}, + ).Encode() + }), + formParams: happyUsernamePasswordFormParams, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyString: "", + wantRedirectLocationString: urlWithQuery(downstreamRedirectURI, map[string]string{ + "error": "access_denied", + // auth was rejected because of the upstream group to which the user belonged, as shown by the configured RejectedAuthenticationMessage appearing here + "error_description": "The resource owner or authorization server denied the request. Reason: configured identity policy rejected this authentication: users who belong to certain upstream group are not allowed.", + "state": happyDownstreamState, + }), + }, { name: "happy LDAP when downstream OIDC validations are skipped because the openid scope was not requested", idps: testidplister.NewUpstreamIDPListerBuilder().WithLDAP(upstreamLDAPIdentityProvider), diff --git a/internal/federationdomain/endpoints/token/token_handler.go b/internal/federationdomain/endpoints/token/token_handler.go index ef893c027..bcfd31fc2 100644 --- a/internal/federationdomain/endpoints/token/token_handler.go +++ b/internal/federationdomain/endpoints/token/token_handler.go @@ -147,7 +147,7 @@ func upstreamRefresh( oldUntransformedGroups := session.Custom.UpstreamGroups oldTransformedUsername := session.Custom.Username - previousIdentity := resolvedprovider.Identity{ + previousIdentity := &resolvedprovider.Identity{ UpstreamUsername: oldUntransformedUsername, UpstreamGroups: oldUntransformedGroups, DownstreamSubject: session.Fosite.Claims.Subject, @@ -155,7 +155,7 @@ func upstreamRefresh( } // Perform the upstream refresh. - refreshedIdentity, err := idp.UpstreamRefresh(ctx, &previousIdentity, skipGroups) + refreshedIdentity, err := idp.UpstreamRefresh(ctx, previousIdentity) if err != nil { return err } diff --git a/internal/federationdomain/endpoints/token/token_handler_test.go b/internal/federationdomain/endpoints/token/token_handler_test.go index cb2848ffb..00f845534 100644 --- a/internal/federationdomain/endpoints/token/token_handler_test.go +++ b/internal/federationdomain/endpoints/token/token_handler_test.go @@ -42,6 +42,7 @@ import ( configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1" supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" + "go.pinniped.dev/internal/celtransformer" "go.pinniped.dev/internal/crud" "go.pinniped.dev/internal/federationdomain/clientregistry" "go.pinniped.dev/internal/federationdomain/endpoints/jwks" @@ -2907,6 +2908,64 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "refresh grant when the upstream refresh when groups scope not requested on original request, when using dynamic client, " + + "still runs identity transformations with upstream groups in case transforms want to reject auth based on groups, even though groups would not be included in final ID token", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + "my-groups-claim": []string{"new-group1", "new-group2", "new-group3"}, // refreshed claims includes updated groups + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). + WithTransformsForFederationDomain(transformtestutil.NewPipeline(t, + []celtransformer.CELTransformation{ + &celtransformer.AllowAuthenticationPolicy{ + Expression: `!groups.exists(g, g in ["` + "new-group1" + `"])`, // reject auth for users who belongs to an upstream group + RejectedAuthenticationMessage: `users who belong to certain upstream group are not allowed`, + }, + }), + ).Build()), + kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources, + authcodeExchange: authcodeExchangeInputs{ + modifyAuthRequest: func(r *http.Request) { + addDynamicClientIDToFormPostBody(r) + r.Form.Set("scope", "openid offline_access username") + }, + modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth, + customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(), + want: tokenEndpointResponseExpectedValues{ + wantStatus: http.StatusOK, + wantClientID: dynamicClientID, + wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, + wantRequestedScopes: []string{"openid", "offline_access", "username"}, + wantGrantedScopes: []string{"openid", "offline_access", "username"}, + wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(), + wantUsername: goodUsername, + wantGroups: nil, + }, + }, + refreshRequest: refreshRequestInputs{ + modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { + r.Body = happyRefreshRequestBody(refreshToken).WithClientID("").WithScope("openid offline_access username").ReadCloser() + r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) // Use basic auth header instead. + }, + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true), + wantStatus: http.StatusUnauthorized, + // auth was rejected because of the upstream group to which the user belonged, as shown by the configured RejectedAuthenticationMessage appearing here + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh rejected by configured identity policy: users who belong to certain upstream group are not allowed." + } + `), + }, + }, + }, { // fosite does not look at the scopes provided in refresh requests, although it is a valid parameter. // even if 'groups' is not sent in the refresh request, we will send groups all the same. diff --git a/internal/federationdomain/resolvedprovider/resolved_provider.go b/internal/federationdomain/resolvedprovider/resolved_provider.go index e1f84d54e..df3bca8f9 100644 --- a/internal/federationdomain/resolvedprovider/resolved_provider.go +++ b/internal/federationdomain/resolvedprovider/resolved_provider.go @@ -141,20 +141,16 @@ type FederationDomainResolvedIdentityProvider interface { // Login performs auth using a username and password that was submitted by the client, without a web browser. // This function should authenticate the user with the upstream identity provider, extract their upstream // identity, and transform it into their downstream identity. - // The groupsWillBeIgnored parameter will be true when the returned groups are going to be ignored by the caller, - // in which case this function may be able to save some effort by avoiding getting the user's upstream groups. // Returned errors should be of type fosite.RFC6749Error. - Login(ctx context.Context, submittedUsername string, submittedPassword string, groupsWillBeIgnored bool) (*Identity, *IdentityLoginExtras, error) + Login(ctx context.Context, submittedUsername string, submittedPassword string) (*Identity, *IdentityLoginExtras, error) // UpstreamRefresh performs a refresh with the upstream provider. // The user's previous identity information is provided as a parameter. // Implementations may use this information to assist in refreshes, but mutations to this argument will be ignored. // If possible, implementations should update the user's upstream group memberships by fetching them from the // upstream provider during the refresh, and returning them. - // The groupsWillBeIgnored parameter will be true when the returned groups are going to be ignored by the caller, - // in which case this function may be able to save some effort by avoiding getting the user's upstream groups. // Returned errors should be of type fosite.RFC6749Error. - UpstreamRefresh(ctx context.Context, identity *Identity, groupsWillBeIgnored bool) (refreshedIdentity *RefreshedIdentity, err error) + UpstreamRefresh(ctx context.Context, identity *Identity) (refreshedIdentity *RefreshedIdentity, err error) } // ErrMissingUpstreamSessionInternalError returns a common type of error that can happen during a login or refresh. diff --git a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go index 59255a099..c8fea641c 100644 --- a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go @@ -125,9 +125,8 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login( ctx context.Context, submittedUsername string, submittedPassword string, - groupsWillBeIgnored bool, ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { - authenticateResponse, authenticated, err := p.Provider.AuthenticateUser(ctx, submittedUsername, submittedPassword, groupsWillBeIgnored) + authenticateResponse, authenticated, err := p.Provider.AuthenticateUser(ctx, submittedUsername, submittedPassword) if err != nil { plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", p.Provider.GetName()) return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(err) @@ -185,7 +184,6 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) LoginFromCallback( func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh( ctx context.Context, identity *resolvedprovider.Identity, - groupsWillBeIgnored bool, ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { var dn string var additionalAttributes map[string]string @@ -229,7 +227,6 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh( DN: dn, Groups: identity.UpstreamGroups, AdditionalAttributes: additionalAttributes, - SkipGroups: groupsWillBeIgnored, }, p.GetDisplayName()) if err != nil { return nil, resolvedprovider.ErrUpstreamRefreshError().WithHint( diff --git a/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go b/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go index 9fa096aa5..1258e292c 100644 --- a/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go @@ -129,7 +129,6 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) Login( ctx context.Context, submittedUsername string, submittedPassword string, - _groupsWillBeIgnored bool, // ignored because we always compute the user's group memberships for OIDC, if possible ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { if !p.Provider.AllowsPasswordGrant() { // Return a user-friendly error for this case which is entirely within our control. @@ -226,7 +225,6 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) LoginFromCallback( func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh( ctx context.Context, identity *resolvedprovider.Identity, - groupsWillBeIgnored bool, ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { sessionData, ok := identity.IDPSpecificSessionData.(*psession.OIDCSessionData) if !ok { @@ -283,21 +281,18 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh( return nil, err } - var refreshedUntransformedGroups []string - if !groupsWillBeIgnored { - // If possible, update the user's group memberships. The configured groups claim name (if there is one) may or - // may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the - // claim name. It could also be missing because the claim was originally found in the ID token during login, but - // now we might not have a refreshed ID token. - // If the claim is found, then use it to update the user's group membership in the session. - // If the claim is not found, then we have no new information about groups, so skip updating the group membership - // and let any old groups memberships in the session remain. - refreshedUntransformedGroups, err = getGroupsFromUpstreamIDToken(p.Provider, mergedClaims) - if err != nil { - return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf( - "Upstream refresh error while extracting groups claim.").WithTrace(err). - WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) - } + // If possible, update the user's group memberships. The configured groups claim name (if there is one) may or + // may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the + // claim name. It could also be missing because the claim was originally found in the ID token during login, but + // now we might not have a refreshed ID token. + // If the claim is found, then use it to update the user's group membership in the session. + // If the claim is not found, then we have no new information about groups, so skip updating the group membership + // and let any old groups memberships in the session remain. + refreshedUntransformedGroups, err := getGroupsFromUpstreamIDToken(p.Provider, mergedClaims) + if err != nil { + return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf( + "Upstream refresh error while extracting groups claim.").WithTrace(err). + WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) } // It's possible that a username wasn't returned by the upstream provider during refresh, diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upsteam_provider.go index 37bb27a5d..40162c52a 100644 --- a/internal/federationdomain/upstreamprovider/upsteam_provider.go +++ b/internal/federationdomain/upstreamprovider/upsteam_provider.go @@ -32,10 +32,6 @@ type RefreshAttributes struct { DN string Groups []string AdditionalAttributes map[string]string - // Skip group search for this particular session refresh. - // E.g. This could be set to true when the user was not granted the downstream groups scope. - // There is no reason to spend the cost of an LDAP group search unless we are going to use the results. - SkipGroups bool } // UpstreamIdentityProviderI includes the interface functions that are common to all upstream identity provider types. diff --git a/internal/testutil/oidctestutil/testldapprovider.go b/internal/testutil/oidctestutil/testldapprovider.go index 55a4eae52..b4ef6363f 100644 --- a/internal/testutil/oidctestutil/testldapprovider.go +++ b/internal/testutil/oidctestutil/testldapprovider.go @@ -115,7 +115,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetName() string { return u.Name } -func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string, _skipGroups bool) (*authenticators.Response, bool, error) { +func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) { return u.AuthenticateFunc(ctx, username, password) } diff --git a/internal/testutil/transformtestutil/transformtestutil.go b/internal/testutil/transformtestutil/transformtestutil.go index 63576a184..01aa1cb39 100644 --- a/internal/testutil/transformtestutil/transformtestutil.go +++ b/internal/testutil/transformtestutil/transformtestutil.go @@ -1,4 +1,4 @@ -// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2023-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package transformtestutil @@ -56,3 +56,20 @@ func NewRejectAllAuthPipeline(t *testing.T) *idtransform.TransformationPipeline return p } + +func NewPipeline(t *testing.T, transforms []celtransformer.CELTransformation) *idtransform.TransformationPipeline { + t.Helper() + + transformer, err := celtransformer.NewCELTransformer(5 * time.Second) + require.NoError(t, err) + + p := idtransform.NewTransformationPipeline() + + for _, transform := range transforms { + compiledTransform, err := transformer.CompileTransformation(transform, nil) + require.NoError(t, err) + p.AppendTransformation(compiledTransform) + } + + return p +} diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index 39c524460..b49333608 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -250,10 +250,6 @@ func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes u if p.c.GroupSearch.SkipGroupRefresh { return storedRefreshAttributes.Groups, nil } - // If we were asked to skip group search for this particular session, then do not search for groups or return any. - if storedRefreshAttributes.SkipGroups { - return nil, nil - } var groupSearchUserAttributeForFilterValue string if p.useGroupSearchUserAttributeForFilter() { @@ -422,23 +418,23 @@ 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, skipGroups bool) (*authenticators.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 } - return p.authenticateUserImpl(ctx, username, skipGroups, endUserBindFunc) + return p.authenticateUserImpl(ctx, username, endUserBindFunc) } // AuthenticateUser authenticates an end user and returns their mapped username, groups, and UID. Implements authenticators.UserAuthenticator. -func (p *Provider) AuthenticateUser(ctx context.Context, username, password string, skipGroups bool) (*authenticators.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, skipGroups, endUserBindFunc) + return p.authenticateUserImpl(ctx, username, endUserBindFunc) } -func (p *Provider) authenticateUserImpl(ctx context.Context, username string, skipGroups bool, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.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 @@ -467,7 +463,7 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, sk return nil, false, fmt.Errorf(`error binding as %q before user search: %w`, p.c.BindUsername, err) } - response, err := p.searchAndBindUser(conn, username, skipGroups, bindFunc) + response, err := p.searchAndBindUser(conn, username, bindFunc) if err != nil { p.traceAuthFailure(t, err) return nil, false, err @@ -564,7 +560,7 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e return searchBase, nil } -func (p *Provider) searchAndBindUser(conn Conn, username string, skipGroups bool, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, 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`, @@ -610,22 +606,19 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, skipGroups bool return nil, err } - var mappedGroupNames []string - if !skipGroups { - var groupSearchUserAttributeForFilterValue string - if p.useGroupSearchUserAttributeForFilter() { - groupSearchUserAttributeForFilterValue, err = p.getSearchResultAttributeValue(p.c.GroupSearch.UserAttributeForFilter, userEntry, username) - if err != nil { - return nil, err - } - } - - mappedGroupNames, err = p.searchGroupsForUserMembership(conn, userEntry.DN, groupSearchUserAttributeForFilterValue) + var groupSearchUserAttributeForFilterValue string + if p.useGroupSearchUserAttributeForFilter() { + groupSearchUserAttributeForFilterValue, err = p.getSearchResultAttributeValue(p.c.GroupSearch.UserAttributeForFilter, userEntry, username) if err != nil { return nil, err } } + mappedGroupNames, err := p.searchGroupsForUserMembership(conn, userEntry.DN, groupSearchUserAttributeForFilterValue) + if err != nil { + return nil, err + } + mappedRefreshAttributes := make(map[string]string) for k := range p.c.RefreshAttributeChecks { mappedVal, err := p.getSearchResultAttributeRawValueEncoded(k, userEntry, username) diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 9bd7100da..6bbbf47ba 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -178,7 +178,6 @@ func TestEndUserAuthentication(t *testing.T) { name string username string password string - skipGroups bool providerConfig *ProviderConfig searchMocks func(conn *mockldapconn.MockConn) bindEndUserMocks func(conn *mockldapconn.MockConn) @@ -291,25 +290,6 @@ func TestEndUserAuthentication(t *testing.T) { info.Groups = []string{} }), }, - { - name: "when groups are requested to be skipped, don't do group search", - username: testUpstreamUsername, - password: testUpstreamPassword, - skipGroups: true, - providerConfig: providerConfig(nil), - searchMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) - conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1) - conn.EXPECT().Close().Times(1) - }, - bindEndUserMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) - }, - wantAuthResponse: expectedAuthResponse(func(r *authenticators.Response) { - info := r.User.(*user.DefaultInfo) - info.Groups = nil - }), - }, { name: "when the UsernameAttribute is dn and there is a user search filter provided", username: testUpstreamUsername, @@ -754,30 +734,6 @@ func TestEndUserAuthentication(t *testing.T) { }, wantAuthResponse: expectedAuthResponse(nil), }, - { - name: "when the UserAttributeForFilter is set to something other than dn but groups are to be skipped, so skips validating UserAttributeForFilter attribute value", - username: testUpstreamUsername, - password: testUpstreamPassword, - skipGroups: true, - providerConfig: providerConfig(func(p *ProviderConfig) { - p.GroupSearch.UserAttributeForFilter = "someUserAttrName" - }), - searchMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) - conn.EXPECT().Search(expectedUserSearch(func(r *ldap.SearchRequest) { - // need to additionally ask for the attribute when performing user search - r.Attributes = []string{testUserSearchUsernameAttribute, testUserSearchUIDAttribute, "someUserAttrName"} - })).Return(exampleUserSearchResult, nil).Times(1) // result does not contain someUserAttrName, but does not matter since group search is skipped - conn.EXPECT().Close().Times(1) - }, - bindEndUserMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1) - }, - wantAuthResponse: expectedAuthResponse(func(r *authenticators.Response) { - info := r.User.(*user.DefaultInfo) - info.Groups = nil - }), - }, { name: "when the UserAttributeForFilter is set to something other than dn but that attribute is not returned by the user search", username: testUpstreamUsername, @@ -1421,7 +1377,7 @@ func TestEndUserAuthentication(t *testing.T) { ldapProvider := New(*tt.providerConfig) - authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password, tt.skipGroups) + authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != nil: @@ -1453,7 +1409,7 @@ func TestEndUserAuthentication(t *testing.T) { } // Skip tt.bindEndUserMocks since DryRunAuthenticateUser() never binds as the end user. - authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username, tt.skipGroups) + authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username) require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) switch { case tt.wantError != nil: @@ -1585,7 +1541,6 @@ func TestUpstreamRefresh(t *testing.T) { tests := []struct { name string providerConfig *ProviderConfig - skipGroups bool setupMocks func(conn *mockldapconn.MockConn) refreshUserDN string dialError error @@ -1721,18 +1676,6 @@ func TestUpstreamRefresh(t *testing.T) { }, wantGroups: nil, // do not update groups }, - { - name: "happy path where group search is configured but groups search is requested to be skipped", - providerConfig: providerConfig(nil), - setupMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) - conn.EXPECT().Search(expectedUserSearch(nil)).Return(happyPathUserSearchResult, nil).Times(1) - // note that group search is not expected - conn.EXPECT().Close().Times(1) - }, - skipGroups: true, - wantGroups: nil, - }, { name: "happy path where group search is configured but skipGroupRefresh is set, when the UserAttributeForFilter is set to something other than dn, still skips group refresh, and skips validating UserAttributeForFilter attribute value", providerConfig: providerConfig(func(p *ProviderConfig) { @@ -1750,23 +1693,6 @@ func TestUpstreamRefresh(t *testing.T) { }, wantGroups: nil, // do not update groups }, - { - name: "happy path where group search is configured but groups search is requested to be skipped, when the UserAttributeForFilter is set to something other than dn, still skips group refresh, and skips validating UserAttributeForFilter attribute value", - providerConfig: providerConfig(func(p *ProviderConfig) { - p.GroupSearch.UserAttributeForFilter = "someUserAttrName" - }), - setupMocks: func(conn *mockldapconn.MockConn) { - conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) - conn.EXPECT().Search(expectedUserSearch(func(r *ldap.SearchRequest) { - // need to additionally ask for the attribute when performing user search - r.Attributes = []string{testUserSearchUsernameAttribute, testUserSearchUIDAttribute, "someUserAttrName", pwdLastSetAttribute} - })).Return(happyPathUserSearchResult, nil).Times(1) - // note that group search is not expected - conn.EXPECT().Close().Times(1) - }, - skipGroups: true, - wantGroups: nil, - }, { name: "happy path when the UserAttributeForFilter is set to something other than dn", providerConfig: providerConfig(func(p *ProviderConfig) { @@ -2282,7 +2208,6 @@ func TestUpstreamRefresh(t *testing.T) { Subject: subject, DN: tt.refreshUserDN, AdditionalAttributes: map[string]string{pwdLastSetAttribute: initialPwdLastSetEncoded}, - SkipGroups: tt.skipGroups, }, testUpstreamName) if tt.wantErr != "" { require.Error(t, err) diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index ae81a84f1..b7eeb10ce 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -73,7 +73,6 @@ func TestLDAPSearch_Parallel(t *testing.T) { name string username string password string - skipGroups bool provider *upstreamldap.Provider wantError testutil.RequireErrorStringFunc wantAuthResponse *authenticators.Response @@ -115,18 +114,6 @@ func TestLDAPSearch_Parallel(t *testing.T) { ExtraRefreshAttributes: map[string]string{}, }, }, - { - name: "skip groups", - username: "pinny", - password: pinnyPassword, - skipGroups: true, - provider: upstreamldap.New(*providerConfig(nil)), - wantAuthResponse: &authenticators.Response{ - User: &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: nil}, - DN: "cn=pinny,ou=users,dc=pinniped,dc=dev", - ExtraRefreshAttributes: map[string]string{}, - }, - }, { name: "when the user search filter is already wrapped by parenthesis", username: "pinny", @@ -741,7 +728,7 @@ func TestLDAPSearch_Parallel(t *testing.T) { for _, test := range tests { tt := test t.Run(tt.name, func(t *testing.T) { - authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password, tt.skipGroups) + authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password) switch { case tt.wantError != nil: @@ -799,7 +786,7 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) { authUserCtx, authUserCtxCancelFunc := context.WithTimeout(context.Background(), 2*time.Minute) defer authUserCtxCancelFunc() - authResponse, authenticated, err := provider.AuthenticateUser(authUserCtx, env.SupervisorUpstreamLDAP.TestUserCN, env.SupervisorUpstreamLDAP.TestUserPassword, false) + authResponse, authenticated, err := provider.AuthenticateUser(authUserCtx, env.SupervisorUpstreamLDAP.TestUserCN, env.SupervisorUpstreamLDAP.TestUserPassword) resultCh <- authUserResult{ response: authResponse, authenticated: authenticated,