Skip to content

Commit

Permalink
Return errInvalidCredentials when wrong credentials is provided for e…
Browse files Browse the repository at this point in the history
…xistent users (#17104)

* adding errInvalidCredentials

* fixing tests

* add changelog

* fixing fmt errors

* test if routeErr is seen externally and fixing error comment

* adding fmt changes

* adding comments
  • Loading branch information
akshya96 authored Sep 27, 2022
1 parent c2d427e commit 9d49bfa
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 13 deletions.
10 changes: 5 additions & 5 deletions builtin/credential/approle/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func TestAppRole_RoleNameCaseSensitivity(t *testing.T) {
},
Storage: s,
})
if err != nil {
if err != nil && err != logical.ErrInvalidCredentials {
t.Fatal(err)
}
if resp == nil || !resp.IsError() {
Expand Down Expand Up @@ -233,7 +233,7 @@ func TestAppRole_RoleNameCaseSensitivity(t *testing.T) {
},
Storage: s,
})
if err != nil {
if err != nil && err != logical.ErrInvalidCredentials {
t.Fatal(err)
}
if resp == nil || !resp.IsError() {
Expand Down Expand Up @@ -292,7 +292,7 @@ func TestAppRole_RoleNameCaseSensitivity(t *testing.T) {
},
Storage: s,
})
if err != nil {
if err != nil && err != logical.ErrInvalidCredentials {
t.Fatal(err)
}
if resp == nil || !resp.IsError() {
Expand Down Expand Up @@ -351,7 +351,7 @@ func TestAppRole_RoleNameCaseSensitivity(t *testing.T) {
},
Storage: s,
})
if err != nil {
if err != nil && err != logical.ErrInvalidCredentials {
t.Fatal(err)
}
if resp == nil || !resp.IsError() {
Expand Down Expand Up @@ -410,7 +410,7 @@ func TestAppRole_RoleNameCaseSensitivity(t *testing.T) {
},
Storage: s,
})
if err != nil {
if err != nil && err != logical.ErrInvalidCredentials {
t.Fatal(err)
}
if resp == nil || !resp.IsError() {
Expand Down
2 changes: 1 addition & 1 deletion builtin/credential/approle/path_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat
return nil, err
}
if entry == nil {
return logical.ErrorResponse("invalid secret id"), nil
return logical.ErrorResponse("invalid secret id"), logical.ErrInvalidCredentials
}

// If a secret ID entry does not have a corresponding accessor
Expand Down
2 changes: 1 addition & 1 deletion builtin/credential/approle/path_role_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ func TestAppRole_RoleNameLowerCasing(t *testing.T) {
"secret_id": secretID,
},
})
if err != nil {
if err != nil && err != logical.ErrInvalidCredentials {
t.Fatal(err)
}
if resp == nil || !resp.IsError() {
Expand Down
4 changes: 2 additions & 2 deletions builtin/credential/ldap/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri
if b.Logger().IsDebug() {
b.Logger().Debug("ldap bind failed", "error", err)
}
return "", nil, logical.ErrorResponse(errUserBindFailed), nil, nil
return "", nil, logical.ErrorResponse(errUserBindFailed), nil, logical.ErrInvalidCredentials
}

// We re-bind to the BindDN if it's defined because we assume
Expand All @@ -117,7 +117,7 @@ func (b *backend) Login(ctx context.Context, req *logical.Request, username stri
if b.Logger().IsDebug() {
b.Logger().Debug("error while attempting to re-bind with the BindDN User", "error", err)
}
return "", nil, logical.ErrorResponse("ldap operation failed: failed to re-bind with the BindDN user"), nil, nil
return "", nil, logical.ErrorResponse("ldap operation failed: failed to re-bind with the BindDN user"), nil, logical.ErrInvalidCredentials
}
if b.Logger().IsDebug() {
b.Logger().Debug("re-bound to original binddn")
Expand Down
20 changes: 16 additions & 4 deletions builtin/credential/userpass/path_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,26 @@ func (b *backend) pathLogin(ctx context.Context, req *logical.Request, d *framew
// Check for a password match. Check for a hash collision for Vault 0.2+,
// but handle the older legacy passwords with a constant time comparison.
passwordBytes := []byte(password)
if !legacyPassword {
switch {
case !legacyPassword:
if err := bcrypt.CompareHashAndPassword(userPassword, passwordBytes); err != nil {
return logical.ErrorResponse("invalid username or password"), nil
// The failed login info of existing users alone are tracked as only
// existing user's failed login information is stored in storage for optimization
if user == nil || userError != nil {
return logical.ErrorResponse("invalid username or password"), nil
}
return logical.ErrorResponse("invalid username or password"), logical.ErrInvalidCredentials
}
} else {
default:
if subtle.ConstantTimeCompare(userPassword, passwordBytes) != 1 {
return logical.ErrorResponse("invalid username or password"), nil
// The failed login info of existing users alone are tracked as only
// existing user's failed login information is stored in storage for optimization
if user == nil || userError != nil {
return logical.ErrorResponse("invalid username or password"), nil
}
return logical.ErrorResponse("invalid username or password"), logical.ErrInvalidCredentials
}

}

if userError != nil {
Expand Down
4 changes: 4 additions & 0 deletions changelog/17104.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:change
auth: Returns invalid credentials for ldap, userpass and approle when wrong credentials are provided for existent users.
This will only be used internally for implementing user lockout.
```
5 changes: 5 additions & 0 deletions sdk/logical/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ var (
// ErrPermissionDenied is returned if the client is not authorized
ErrPermissionDenied = errors.New("permission denied")

// ErrInvalidCredentials is returned when the provided credentials are incorrect
// This is used internally for user lockout purposes. This is not seen externally.
// The status code returned does not change because of this error
ErrInvalidCredentials = errors.New("invalid credentials")

// ErrMultiAuthzPending is returned if the the request needs more
// authorizations
ErrMultiAuthzPending = errors.New("request needs further approval")
Expand Down
2 changes: 2 additions & 0 deletions sdk/logical/response_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ func RespondErrorCommon(req *Request, resp *Response, err error) (int, error) {
statusCode = http.StatusNotFound
case errwrap.Contains(err, ErrRelativePath.Error()):
statusCode = http.StatusBadRequest
case errwrap.Contains(err, ErrInvalidCredentials.Error()):
statusCode = http.StatusBadRequest
}
}

Expand Down
12 changes: 12 additions & 0 deletions sdk/logical/response_util_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package logical

import (
"errors"
"strings"
"testing"
)
Expand Down Expand Up @@ -60,6 +61,17 @@ func TestResponseUtil_RespondErrorCommon_basic(t *testing.T) {
respErr: nil,
expectedStatus: 0,
},
{
title: "Invalid Credentials error ",
respErr: ErrInvalidCredentials,
resp: &Response{
Data: map[string]interface{}{
"error": "error due to wrong credentials",
},
},
expectedErr: errors.New("error due to wrong credentials"),
expectedStatus: 400,
},
}

for _, tc := range testCases {
Expand Down

0 comments on commit 9d49bfa

Please sign in to comment.