From 51263a0f07c8cc222ec6493f76f9827cfdc688eb Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 13 Apr 2021 16:22:13 -0700 Subject: [PATCH] Return unauthenticated instead of error for bad username or password - Bad usernames and passwords aren't really errors, since they are based on end-user input. - Other kinds of authentication failures are caused by bad configuration so still treat those as errors. - Empty usernames and passwords are already prevented by our endpoint handler, but just to be safe make sure they cause errors inside the authenticator too. --- internal/oidc/auth/auth_handler.go | 3 +- internal/upstreamldap/upstreamldap.go | 26 ++++++- internal/upstreamldap/upstreamldap_test.go | 65 +++++++++++++--- test/deploy/tools/ldap.yaml | 2 +- ...ldapsearch_test.go => ldap_client_test.go} | 78 ++++++++++--------- 5 files changed, 123 insertions(+), 51 deletions(-) rename test/integration/{ldapsearch_test.go => ldap_client_test.go} (91%) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 701d64da..434d6ce4 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -95,10 +95,11 @@ func handleAuthRequestForLDAPUpstream( authenticateResponse, authenticated, err := ldapUpstream.AuthenticateUser(r.Context(), username, password) if err != nil { - plog.WarningErr("unexpected error during upstream authentication", err, "upstreamName", ldapUpstream.GetName()) + plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", ldapUpstream.GetName()) return httperr.New(http.StatusBadGateway, "unexpected error during upstream authentication") } if !authenticated { + plog.Debug("failed upstream LDAP authentication", "upstreamName", ldapUpstream.GetName()) // Return an error according to OIDC spec 3.1.2.6 (second paragraph). err = errors.WithStack(fosite.ErrAccessDenied.WithHintf("Username/password not accepted by LDAP provider.")) oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) diff --git a/internal/upstreamldap/upstreamldap.go b/internal/upstreamldap/upstreamldap.go index a5af4fe6..cf3b5ffa 100644 --- a/internal/upstreamldap/upstreamldap.go +++ b/internal/upstreamldap/upstreamldap.go @@ -15,12 +15,15 @@ import ( "github.com/go-ldap/ldap/v3" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" + + "go.pinniped.dev/internal/plog" ) const ( ldapsScheme = "ldaps" distinguishedNameAttributeName = "dn" userSearchFilterInterpolationLocationMarker = "{}" + invalidCredentialsErrorPrefix = `LDAP Result Code 49 "Invalid Credentials":` ) // Conn abstracts the upstream LDAP communication protocol (mostly for testing). @@ -185,6 +188,11 @@ func (p *Provider) AuthenticateUser(ctx context.Context, username, password stri return nil, false, fmt.Errorf(`must specify UserSearch Filter when UserSearch UsernameAttribute is "dn"`) } + if len(username) == 0 { + // Empty passwords are already handled by go-ldap. + return nil, false, nil + } + conn, err := p.dial(ctx) if err != nil { return nil, false, fmt.Errorf(`error dialing host "%s": %w`, p.Host, err) @@ -200,6 +208,10 @@ func (p *Provider) AuthenticateUser(ctx context.Context, username, password stri if err != nil { return nil, false, err } + if len(mappedUsername) == 0 || len(mappedUID) == 0 { + // Couldn't find the username or couldn't bind using the password. + return nil, false, nil + } response := &authenticator.Response{ User: &user.DefaultInfo{ @@ -216,7 +228,12 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, password string if err != nil { return "", "", fmt.Errorf(`error searching for user "%s": %w`, username, err) } - if len(searchResult.Entries) != 1 { + if len(searchResult.Entries) == 0 { + plog.Debug("error finding user: user not found (if this username is valid, please check the user search configuration)", + "upstreamName", p.GetName(), "username", username) + return "", "", nil + } + if len(searchResult.Entries) > 1 { return "", "", fmt.Errorf(`searching for user "%s" resulted in %d search results, but expected 1 result`, username, len(searchResult.Entries), ) @@ -236,9 +253,14 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, password string return "", "", err } - // Take care that any other LDAP commands after this bind will be run as this user instead of as the configured BindUsername! + // Caution: Note that any other LDAP commands after this bind will be run as this user instead of as the configured BindUsername! err = conn.Bind(userEntry.DN, password) if err != nil { + plog.DebugErr("error binding for user (if this is not the expected dn for this username, please check the user search configuration)", + err, "upstreamName", p.GetName(), "username", username, "dn", userEntry.DN) + if strings.HasPrefix(err.Error(), invalidCredentialsErrorPrefix) { + return "", "", nil + } return "", "", fmt.Errorf(`error binding for user "%s" using provided password against DN "%s": %w`, username, userEntry.DN, err) } diff --git a/internal/upstreamldap/upstreamldap_test.go b/internal/upstreamldap/upstreamldap_test.go index 39b19093..c58505dd 100644 --- a/internal/upstreamldap/upstreamldap_test.go +++ b/internal/upstreamldap/upstreamldap_test.go @@ -80,15 +80,16 @@ func TestAuthenticateUser(t *testing.T) { } tests := []struct { - name string - username string - password string - provider *Provider - setupMocks func(conn *mockldapconn.MockConn) - dialError error - wantError string - wantToSkipDial bool - wantAuthResponse *authenticator.Response + name string + username string + password string + provider *Provider + setupMocks func(conn *mockldapconn.MockConn) + dialError error + wantError string + wantToSkipDial bool + wantAuthResponse *authenticator.Response + wantUnauthenticated bool }{ { name: "happy path", @@ -282,6 +283,8 @@ func TestAuthenticateUser(t *testing.T) { }, { name: "when dial fails", + username: testUpstreamUsername, + password: testUpstreamPassword, provider: provider(nil), dialError: errors.New("some dial error"), wantError: fmt.Sprintf(`error dialing host "%s": some dial error`, testHost), @@ -299,6 +302,8 @@ func TestAuthenticateUser(t *testing.T) { }, { name: "when binding as the bind user returns an error", + username: testUpstreamUsername, + password: testUpstreamPassword, provider: provider(nil), setupMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Return(errors.New("some bind error")).Times(1) @@ -330,7 +335,7 @@ func TestAuthenticateUser(t *testing.T) { }, nil).Times(1) conn.EXPECT().Close().Times(1) }, - wantError: fmt.Sprintf(`searching for user "%s" resulted in 0 search results, but expected 1 result`, testUpstreamUsername), + wantUnauthenticated: true, }, { name: "when searching for the user returns multiple results", @@ -524,6 +529,37 @@ func TestAuthenticateUser(t *testing.T) { }, wantError: fmt.Sprintf(`error binding for user "%s" using provided password against DN "%s": some bind error`, testUpstreamUsername, testSearchResultDNValue), }, + { + name: "when binding as the found user returns a specific invalid credentials error", + username: testUpstreamUsername, + password: testUpstreamPassword, + provider: provider(nil), + setupMocks: func(conn *mockldapconn.MockConn) { + conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) + conn.EXPECT().Search(expectedSearch(nil)).Return(&ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: testSearchResultDNValue, + Attributes: []*ldap.EntryAttribute{ + ldap.NewEntryAttribute(testUserSearchUsernameAttribute, []string{testSearchResultUsernameAttributeValue}), + ldap.NewEntryAttribute(testUserSearchUIDAttribute, []string{testSearchResultUIDAttributeValue}), + }, + }, + }, + }, nil).Times(1) + conn.EXPECT().Bind(testSearchResultDNValue, testUpstreamPassword).Return(errors.New(`LDAP Result Code 49 "Invalid Credentials": some bind error`)).Times(1) + conn.EXPECT().Close().Times(1) + }, + wantUnauthenticated: true, + }, + { + name: "when no username is specified", + username: "", + password: testUpstreamPassword, + provider: provider(nil), + wantToSkipDial: true, + wantUnauthenticated: true, + }, } for _, test := range tests { @@ -551,11 +587,16 @@ func TestAuthenticateUser(t *testing.T) { require.Equal(t, !tt.wantToSkipDial, dialWasAttempted) - if tt.wantError != "" { + switch { + case tt.wantError != "": require.EqualError(t, err, tt.wantError) require.False(t, authenticated) require.Nil(t, authResponse) - } else { + case tt.wantUnauthenticated: + require.NoError(t, err) + require.False(t, authenticated) + require.Nil(t, authResponse) + default: require.NoError(t, err) require.True(t, authenticated) require.Equal(t, tt.wantAuthResponse, authResponse) diff --git a/test/deploy/tools/ldap.yaml b/test/deploy/tools/ldap.yaml index 20040599..b97151a3 100644 --- a/test/deploy/tools/ldap.yaml +++ b/test/deploy/tools/ldap.yaml @@ -14,7 +14,7 @@ stringData: #@yaml/text-templated-strings ldap.ldif: | # ** CAUTION: Blank lines separate entries in the LDIF format! Do not remove them! *** - # Here's a good explaination of LDIF: + # Here's a good explanation of LDIF: # https://www.digitalocean.com/community/tutorials/how-to-use-ldif-files-to-make-changes-to-an-openldap-system # pinniped.dev (organization, root) diff --git a/test/integration/ldapsearch_test.go b/test/integration/ldap_client_test.go similarity index 91% rename from test/integration/ldapsearch_test.go rename to test/integration/ldap_client_test.go index 6bd36241..14524ea3 100644 --- a/test/integration/ldapsearch_test.go +++ b/test/integration/ldap_client_test.go @@ -24,6 +24,8 @@ import ( "go.pinniped.dev/internal/upstreamldap" ) +// Unlike most other integration tests, you can run this test with no special setup, as long as you have Docker. +// It does not depend on Kubernetes. func TestLDAPSearch(t *testing.T) { ctx, cancelFunc := context.WithCancel(context.Background()) t.Cleanup(func() { @@ -57,12 +59,13 @@ func TestLDAPSearch(t *testing.T) { wallyPassword := "password456" // from the LDIF file below tests := []struct { - name string - username string - password string - provider *upstreamldap.Provider - wantError string - wantAuthResponse *authenticator.Response + name string + username string + password string + provider *upstreamldap.Provider + wantError string + wantAuthResponse *authenticator.Response + wantUnauthenticated bool }{ { name: "happy path", @@ -193,18 +196,18 @@ func TestLDAPSearch(t *testing.T) { wantError: `error binding as "cn=admin,dc=pinniped,dc=dev" before user search: LDAP Result Code 49 "Invalid Credentials": `, }, { - name: "when the end user password is wrong", - username: "pinny", - password: "wrong-pinny-password", - provider: provider(nil), - wantError: `error binding for user "pinny" using provided password against DN "cn=pinny,ou=users,dc=pinniped,dc=dev": LDAP Result Code 49 "Invalid Credentials": `, + name: "when the end user password is wrong", + username: "pinny", + password: "wrong-pinny-password", + provider: provider(nil), + wantUnauthenticated: true, }, { - name: "when the end user username is wrong", - username: "wrong-username", - password: pinnyPassword, - provider: provider(nil), - wantError: `searching for user "wrong-username" resulted in 0 search results, but expected 1 result`, + name: "when the end user username is wrong", + username: "wrong-username", + password: pinnyPassword, + provider: provider(nil), + wantUnauthenticated: true, }, { name: "when the user search filter does not compile", @@ -329,18 +332,18 @@ func TestLDAPSearch(t *testing.T) { wantError: `error searching for user "pinny": LDAP Result Code 32 "No Such Object": `, }, { - name: "when the search base causes no search results", - username: "pinny", - password: pinnyPassword, - provider: provider(func(p *upstreamldap.Provider) { p.UserSearch.Base = "ou=groups,dc=pinniped,dc=dev" }), - wantError: `searching for user "pinny" resulted in 0 search results, but expected 1 result`, + name: "when the search base causes no search results", + username: "pinny", + password: pinnyPassword, + provider: provider(func(p *upstreamldap.Provider) { p.UserSearch.Base = "ou=groups,dc=pinniped,dc=dev" }), + wantUnauthenticated: true, }, { - name: "when there is no username specified", - username: "", - password: pinnyPassword, - provider: provider(nil), - wantError: `searching for user "" resulted in 0 search results, but expected 1 result`, + name: "when there is no username specified", + username: "", + password: pinnyPassword, + provider: provider(nil), + wantUnauthenticated: true, }, { name: "when there is no password specified", @@ -350,11 +353,11 @@ func TestLDAPSearch(t *testing.T) { wantError: `error binding for user "pinny" using provided password against DN "cn=pinny,ou=users,dc=pinniped,dc=dev": LDAP Result Code 206 "Empty password not allowed by the client": ldap: empty password not allowed by the client`, }, { - name: "when the user has no password in their entry", - username: "olive", - password: "anything", - provider: provider(nil), - wantError: `error binding for user "olive" using provided password against DN "cn=olive,ou=users,dc=pinniped,dc=dev": LDAP Result Code 49 "Invalid Credentials": `, + name: "when the user has no password in their entry", + username: "olive", + password: "anything", + provider: provider(nil), + wantUnauthenticated: true, }, } @@ -363,11 +366,16 @@ func TestLDAPSearch(t *testing.T) { t.Run(tt.name, func(t *testing.T) { authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password) - if tt.wantError != "" { + switch { + case tt.wantError != "": require.EqualError(t, err, tt.wantError) require.False(t, authenticated) require.Nil(t, authResponse) - } else { + case tt.wantUnauthenticated: + require.NoError(t, err) + require.False(t, authenticated) + require.Nil(t, authResponse) + default: require.NoError(t, err) require.True(t, authenticated) require.Equal(t, tt.wantAuthResponse, authResponse) @@ -486,7 +494,7 @@ func writeToNewTempFile(t *testing.T, dir string, filename string, contents []by filePath := path.Join(dir, filename) - err := ioutil.WriteFile(filePath, contents, 0644) + err := ioutil.WriteFile(filePath, contents, 0600) require.NoError(t, err) t.Cleanup(func() { @@ -497,7 +505,7 @@ func writeToNewTempFile(t *testing.T, dir string, filename string, contents []by var testLDIF = ` # ** CAUTION: Blank lines separate entries in the LDIF format! Do not remove them! *** -# Here's a good explaination of LDIF: +# Here's a good explanation of LDIF: # https://www.digitalocean.com/community/tutorials/how-to-use-ldif-files-to-make-changes-to-an-openldap-system # pinniped.dev (organization, root)