djpbessems/ContainerImage.Pinniped
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Refactors:

Browse Source 
- 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 <margaretc@vmware.com>
This commit is contained in:
Margo Crawford 2021-11-03 15:17:50 -07:00
parent c84329d7a4
commit b5b8cab717
4 changed files with 48 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
internal/oidc/token/token_handler.go
View File

@ -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) {

18
internal/oidc/token/token_handler_test.go
View File

@ -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."
}
`),
},

49
internal/upstreamldap/upstreamldap.go
View File

@ -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 {

5
test/integration/supervisor_login_test.go
View File

@ -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(),
)
}