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

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

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)
 	}
 

internal/upstreamldap/upstreamldap_test.go

@@ -89,6 +89,7 @@ func TestAuthenticateUser(t *testing.T) {
 		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)

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)

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() {
@@ -63,6 +65,7 @@ func TestLDAPSearch(t *testing.T) {
 		provider            *upstreamldap.Provider
 		wantError           string
 		wantAuthResponse    *authenticator.Response
+		wantUnauthenticated bool
 	}{
 		{
 			name:     "happy path",
@@ -197,14 +200,14 @@ func TestLDAPSearch(t *testing.T) {
 			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": `,
+			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`,
+			wantUnauthenticated: true,
 		},
 		{
 			name:      "when the user search filter does not compile",
@@ -333,14 +336,14 @@ func TestLDAPSearch(t *testing.T) {
 			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`,
+			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`,
+			wantUnauthenticated: true,
 		},
 		{
 			name:      "when there is no password specified",
@@ -354,7 +357,7 @@ func TestLDAPSearch(t *testing.T) {
 			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": `,
+			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)