From 542570c28985bbf3f4a2336c0587ca0649b596e9 Mon Sep 17 00:00:00 2001 From: akshya96 <87045294+akshya96@users.noreply.github.com> Date: Tue, 27 Sep 2022 16:49:14 -0700 Subject: [PATCH] Return errInvalidCredentials when wrong credentials is provided for existent 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 --- builtin/credential/approle/backend_test.go | 10 +++++----- builtin/credential/approle/path_login.go | 2 +- builtin/credential/approle/path_role_test.go | 2 +- builtin/credential/ldap/backend.go | 4 ++-- builtin/credential/userpass/path_login.go | 20 ++++++++++++++++---- changelog/17104.txt | 4 ++++ sdk/logical/error.go | 5 +++++ sdk/logical/response_util.go | 2 ++ sdk/logical/response_util_test.go | 12 ++++++++++++ 9 files changed, 48 insertions(+), 13 deletions(-) create mode 100644 changelog/17104.txt diff --git a/builtin/credential/approle/backend_test.go b/builtin/credential/approle/backend_test.go index 044f02d2a..212fe36f0 100644 --- a/builtin/credential/approle/backend_test.go +++ b/builtin/credential/approle/backend_test.go @@ -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() { @@ -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() { @@ -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() { @@ -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() { @@ -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() { diff --git a/builtin/credential/approle/path_login.go b/builtin/credential/approle/path_login.go index 5822519b1..2ad3924ba 100644 --- a/builtin/credential/approle/path_login.go +++ b/builtin/credential/approle/path_login.go @@ -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 diff --git a/builtin/credential/approle/path_role_test.go b/builtin/credential/approle/path_role_test.go index 8ab3bfc66..06706dda5 100644 --- a/builtin/credential/approle/path_role_test.go +++ b/builtin/credential/approle/path_role_test.go @@ -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() { diff --git a/builtin/credential/ldap/backend.go b/builtin/credential/ldap/backend.go index d318fe4b4..35e0f102c 100644 --- a/builtin/credential/ldap/backend.go +++ b/builtin/credential/ldap/backend.go @@ -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 @@ -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") diff --git a/builtin/credential/userpass/path_login.go b/builtin/credential/userpass/path_login.go index 95d63f28c..8e3f42ea4 100644 --- a/builtin/credential/userpass/path_login.go +++ b/builtin/credential/userpass/path_login.go @@ -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 { diff --git a/changelog/17104.txt b/changelog/17104.txt new file mode 100644 index 000000000..00c5eebf3 --- /dev/null +++ b/changelog/17104.txt @@ -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. +``` \ No newline at end of file diff --git a/sdk/logical/error.go b/sdk/logical/error.go index 02f68dd91..68c8e1373 100644 --- a/sdk/logical/error.go +++ b/sdk/logical/error.go @@ -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") diff --git a/sdk/logical/response_util.go b/sdk/logical/response_util.go index 7454189f1..a269fc639 100644 --- a/sdk/logical/response_util.go +++ b/sdk/logical/response_util.go @@ -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 } } diff --git a/sdk/logical/response_util_test.go b/sdk/logical/response_util_test.go index 823c4afbd..00d70a5c4 100644 --- a/sdk/logical/response_util_test.go +++ b/sdk/logical/response_util_test.go @@ -1,6 +1,7 @@ package logical import ( + "errors" "strings" "testing" ) @@ -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 {