From b5b8cab717ed54d5a24df6c8d6f551aad38c1a0a Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 3 Nov 2021 15:17:50 -0700 Subject: [PATCH] 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(), ) }