From 6009fab7068c2732ce7d3223ca113728b9a90093 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Wed, 31 May 2023 13:51:20 -0400 Subject: [PATCH] backport of commit b54645514400b7c3db6e4a60b5491cdb7d55ceb6 (#20869) Co-authored-by: akshya96 <87045294+akshya96@users.noreply.github.com> --- changelog/20783.txt | 3 + vault/core.go | 9 +- vault/core_test.go | 2 +- .../identity/userlockouts_test.go | 505 ++++-------------- vault/logical_system_user_lockout.go | 4 +- vault/request_handling.go | 116 ++-- vault/request_handling_util.go | 4 +- 7 files changed, 193 insertions(+), 450 deletions(-) create mode 100644 changelog/20783.txt diff --git a/changelog/20783.txt b/changelog/20783.txt new file mode 100644 index 000000000..372d36cb7 --- /dev/null +++ b/changelog/20783.txt @@ -0,0 +1,3 @@ +```release-note:bug +core: Fix writes to readonly storage on performance standbys when user lockout feature is enabled. +``` \ No newline at end of file diff --git a/vault/core.go b/vault/core.go index 1231069c9..511e5e3f9 100644 --- a/vault/core.go +++ b/vault/core.go @@ -3546,18 +3546,19 @@ func (c *Core) runLockedUserEntryUpdatesForMountAccessor(ctx context.Context, mo lockoutDurationFromConfiguration := userLockoutConfiguration.LockoutDuration // get the entry for the locked user from userFailedLoginInfo map - failedLoginInfoFromMap := c.GetUserFailedLoginInfo(ctx, loginUserInfoKey) + failedLoginInfoFromMap := c.LocalGetUserFailedLoginInfo(ctx, loginUserInfoKey) // check if the storage entry for locked user is stale if time.Now().After(lastFailedLoginTimeFromStorageEntry.Add(lockoutDurationFromConfiguration)) { // stale entry, remove from storage + // leaving this as it is as this happens on the active node + // also handles case where namespace is deleted if err := c.barrier.Delete(ctx, path+alias); err != nil { return 0, err } - // remove entry for this user from userFailedLoginInfo map if present as the user is not locked if failedLoginInfoFromMap != nil { - if err = c.UpdateUserFailedLoginInfo(ctx, loginUserInfoKey, nil, true); err != nil { + if err = updateUserFailedLoginInfo(ctx, c, loginUserInfoKey, nil, true); err != nil { return 0, err } } @@ -3574,7 +3575,7 @@ func (c *Core) runLockedUserEntryUpdatesForMountAccessor(ctx context.Context, mo if failedLoginInfoFromMap != &actualFailedLoginInfo { // entry is invalid, updating the entry in userFailedLoginMap with correct information - if err = c.UpdateUserFailedLoginInfo(ctx, loginUserInfoKey, &actualFailedLoginInfo, false); err != nil { + if err = updateUserFailedLoginInfo(ctx, c, loginUserInfoKey, &actualFailedLoginInfo, false); err != nil { return 0, err } } diff --git a/vault/core_test.go b/vault/core_test.go index 6fb5f6d17..59e0706f1 100644 --- a/vault/core_test.go +++ b/vault/core_test.go @@ -482,7 +482,7 @@ func TestCore_RunLockedUserUpdatesForValidEntry(t *testing.T) { mountAccessor: "mountAccessor1", } - failedLoginInfoFromMap := core.GetUserFailedLoginInfo(context.Background(), loginUserInfoKey) + failedLoginInfoFromMap := core.LocalGetUserFailedLoginInfo(context.Background(), loginUserInfoKey) if failedLoginInfoFromMap == nil { t.Fatalf("err: entry must exist for locked user in userFailedLoginInfo map") } diff --git a/vault/external_tests/identity/userlockouts_test.go b/vault/external_tests/identity/userlockouts_test.go index 2ff49f180..21c31712b 100644 --- a/vault/external_tests/identity/userlockouts_test.go +++ b/vault/external_tests/identity/userlockouts_test.go @@ -4,9 +4,9 @@ package identity import ( + "os" "strings" "testing" - "time" "github.com/hashicorp/vault/api" "github.com/hashicorp/vault/builtin/credential/userpass" @@ -15,94 +15,21 @@ import ( "github.com/hashicorp/vault/vault" ) -// TestIdentityStore_UserLockoutTest tests that the user gets locked after -// more than 1 failed login request than the number specified for -// lockout threshold field in user lockout configuration. It also -// tests that the user gets unlocked after the duration specified -// for lockout duration field has passed -func TestIdentityStore_UserLockoutTest(t *testing.T) { - coreConfig := &vault.CoreConfig{ - CredentialBackends: map[string]logical.Factory{ - "userpass": userpass.Factory, - }, - } - cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ - HandlerFunc: vaulthttp.Handler, - }) - cluster.Start() - defer cluster.Cleanup() - active := cluster.Cores[0].Client - standby := cluster.Cores[1].Client +const ( + UserLockoutThresholdDefault = 5 +) - err := active.Sys().EnableAuthWithOptions("userpass", &api.EnableAuthOptions{ - Type: "userpass", - }) - if err != nil { - t.Fatal(err) - } +// TestIdentityStore_DisableUserLockoutTest tests that user login will +// fail when supplied with wrong credentials. If the user is locked, +// it returns permission denied. Otherwise, it returns invalid user +// credentials error if the user lockout feature is disabled. +// It tests disabling the feature using env variable VAULT_DISABLE_USER_LOCKOUT +// and also using auth tune. Also, tests that env var has more precedence over +// settings in auth tune. +func TestIdentityStore_DisableUserLockoutTest(t *testing.T) { + // reset to false before exiting + defer os.Unsetenv("VAULT_DISABLE_USER_LOCKOUT") - // tune auth mount - userlockoutConfig := &api.UserLockoutConfigInput{ - LockoutThreshold: "3", - LockoutDuration: "5s", - LockoutCounterResetDuration: "5s", - } - err = active.Sys().TuneMount("auth/userpass", api.MountConfigInput{ - UserLockoutConfig: userlockoutConfig, - }) - if err != nil { - t.Fatal(err) - } - - // create a user for userpass - _, err = standby.Logical().Write("auth/userpass/users/bsmith", map[string]interface{}{ - "password": "training", - }) - if err != nil { - t.Fatal(err) - } - - // login failure count 1 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login failure count 2 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login failure count 3 - active.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login : permission denied as user locked out - _, err = standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "training", - }) - if err == nil { - t.Fatal("expected login to fail as user locked out") - } - if !strings.Contains(err.Error(), logical.ErrPermissionDenied.Error()) { - t.Fatalf("expected to see permission denied error as user locked out, got %v", err) - } - - time.Sleep(5 * time.Second) - - // login with right password and wait for user to get unlocked - _, err = standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "training", - }) - if err != nil { - t.Fatal("expected login to succeed as user is unlocked") - } -} - -// TestIdentityStore_UserFailedLoginMapResetOnSuccess tests that -// the user lockout feature is reset for a user after one successfull attempt -// after multiple failed login attempts (within lockout threshold) -func TestIdentityStore_UserFailedLoginMapResetOnSuccess(t *testing.T) { coreConfig := &vault.CoreConfig{ CredentialBackends: map[string]logical.Factory{ "userpass": userpass.Factory, @@ -114,8 +41,10 @@ func TestIdentityStore_UserFailedLoginMapResetOnSuccess(t *testing.T) { cluster.Start() defer cluster.Cleanup() - client := cluster.Cores[0].Client + // standby client + client := cluster.Cores[1].Client + // enable userpass err := client.Sys().EnableAuthWithOptions("userpass", &api.EnableAuthOptions{ Type: "userpass", }) @@ -123,20 +52,7 @@ func TestIdentityStore_UserFailedLoginMapResetOnSuccess(t *testing.T) { t.Fatal(err) } - // tune auth mount - userlockoutConfig := &api.UserLockoutConfigInput{ - LockoutThreshold: "3", - LockoutDuration: "5s", - LockoutCounterResetDuration: "5s", - } - err = client.Sys().TuneMount("auth/userpass", api.MountConfigInput{ - UserLockoutConfig: userlockoutConfig, - }) - if err != nil { - t.Fatal(err) - } - - // create a user for userpass + // create a userpass user _, err = client.Logical().Write("auth/userpass/users/bsmith", map[string]interface{}{ "password": "training", }) @@ -144,308 +60,119 @@ func TestIdentityStore_UserFailedLoginMapResetOnSuccess(t *testing.T) { t.Fatal(err) } - // login failure count 1 - client.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login failure count 2 - client.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login with right credentials - successful login - // entry for this user is removed from userFailedLoginInfo map - _, err = client.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "training", - }) - if err != nil { - t.Fatal(err) - } - - // login failure count 3, is now count 1 after successful login - client.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login failure count 4, is now count 2 after successful login - // error should not be permission denied as user not locked out - _, err = client.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - if err == nil { - t.Fatal("expected login to fail due to wrong credentials") - } - if !strings.Contains(err.Error(), "invalid username or password") { - t.Fatalf("expected to see invalid username or password error as user is not locked out, got %v", err) - } -} - -// TestIdentityStore_DisableUserLockoutTest tests that user login will -// fail when supplied with wrong credentials. If the user is locked, -// it returns permission denied. In this case, it returns invalid user -// credentials error as the user lockout feature is disabled and the -// user did not get locked after multiple failed login attempts -func TestIdentityStore_DisableUserLockoutTest(t *testing.T) { - coreConfig := &vault.CoreConfig{ - CredentialBackends: map[string]logical.Factory{ - "userpass": userpass.Factory, - }, - } - cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ - HandlerFunc: vaulthttp.Handler, - }) - cluster.Start() - defer cluster.Cleanup() - - active := cluster.Cores[0].Client - standby := cluster.Cores[1].Client - - err := active.Sys().EnableAuthWithOptions("userpass", &api.EnableAuthOptions{ - Type: "userpass", - }) - if err != nil { - t.Fatal(err) - } - - // tune auth mount - disableLockout := true - userlockoutConfig := &api.UserLockoutConfigInput{ - LockoutThreshold: "3", - DisableLockout: &disableLockout, - } - err = active.Sys().TuneMount("auth/userpass", api.MountConfigInput{ - UserLockoutConfig: userlockoutConfig, - }) - if err != nil { - t.Fatal(err) - } - - // create a userpass user - _, err = standby.Logical().Write("auth/userpass/users/bsmith", map[string]interface{}{ - "password": "training", - }) - if err != nil { - t.Fatal(err) - } - - // login failure count 1 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login failure count 2 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login failure count 3 - active.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // login failure count 4 - _, err = standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - if err == nil { - t.Fatal("expected login to fail due to wrong credentials") - } - if !strings.Contains(err.Error(), "invalid username or password") { - t.Fatalf("expected to see invalid username or password error as user is not locked out, got %v", err) - } -} - -// TestIdentityStore_LockoutCounterResetTest tests that the user lockout counter -// for a user is reset after no failed login attempts for a duration -// as specified for lockout counter reset field in user lockout configuration -func TestIdentityStore_LockoutCounterResetTest(t *testing.T) { - coreConfig := &vault.CoreConfig{ - CredentialBackends: map[string]logical.Factory{ - "userpass": userpass.Factory, - }, - } - cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ - HandlerFunc: vaulthttp.Handler, - }) - cluster.Start() - defer cluster.Cleanup() - active := cluster.Cores[0].Client - standby := cluster.Cores[1].Client - - err := active.Sys().EnableAuthWithOptions("userpass", &api.EnableAuthOptions{ - Type: "userpass", - }) - if err != nil { - t.Fatal(err) - } - - // tune auth mount - userlockoutConfig := &api.UserLockoutConfigInput{ - LockoutThreshold: "3", - LockoutCounterResetDuration: "5s", - } - err = active.Sys().TuneMount("auth/userpass", api.MountConfigInput{ - UserLockoutConfig: userlockoutConfig, - }) - if err != nil { - t.Fatal(err) - } - - // create a user for userpass - _, err = standby.Logical().Write("auth/userpass/users/bsmith", map[string]interface{}{ - "password": "training", - }) - if err != nil { - t.Fatal(err) - } - - // login failure count 1 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - // login failure count 2 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - - // set sleep timer to reset login counter - time.Sleep(5 * time.Second) - - // login failure 3, count should be reset, this will be treated as failed count 1 - active.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - // login failure 4, this will be treated as failed count 2 - _, err = standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - if err == nil { - t.Fatal("expected login to fail due to wrong credentials") - } - if !strings.Contains(err.Error(), "invalid username or password") { - t.Fatalf("expected to see invalid username or password error as user is not locked out, got %v", err) - } -} - -// TestIdentityStore_UnlockUserTest tests the user is -// unlocked if locked using -// sys/locked-users/[mount_accessor]/unlock/[alias-identifier] -func TestIdentityStore_UnlockUserTest(t *testing.T) { - coreConfig := &vault.CoreConfig{ - CredentialBackends: map[string]logical.Factory{ - "userpass": userpass.Factory, - }, - } - cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{ - HandlerFunc: vaulthttp.Handler, - }) - cluster.Start() - defer cluster.Cleanup() - active := cluster.Cores[0].Client - standby := cluster.Cores[1].Client - - // enable userpass auth method on path userpass - if err := active.Sys().EnableAuthWithOptions("userpass", &api.EnableAuthOptions{ - Type: "userpass", - }); err != nil { - t.Fatal(err) - } - // get mount accessor for userpass mount - secret, err := standby.Logical().Read("sys/auth/userpass") + secret, err := client.Logical().Read("sys/auth/userpass") if err != nil || secret == nil { t.Fatal(err) } mountAccessor := secret.Data["accessor"].(string) - // tune auth mount - userlockoutConfig := &api.UserLockoutConfigInput{ - LockoutThreshold: "2", - LockoutDuration: "5m", - } - if err = active.Sys().TuneMount("auth/userpass", api.MountConfigInput{ - UserLockoutConfig: userlockoutConfig, - }); err != nil { - t.Fatal(err) - } + // variables for auth tune + disableLockout := true + enableLockout := false - // create a user for userpass - if _, err = standby.Logical().Write("auth/userpass/users/bsmith", map[string]interface{}{ - "password": "training", - }); err != nil { - t.Fatal(err) + tests := []struct { + name string + setDisableUserLockoutEnvVar string + // default is false + setDisableLockoutAuthTune bool + expectedUserLocked bool + }{ + { + name: "Both unset, uses default behaviour i.e; user lockout feature enabled", + setDisableUserLockoutEnvVar: "", + setDisableLockoutAuthTune: false, + expectedUserLocked: true, + }, + { + name: "User lockout feature is disabled using auth tune", + setDisableUserLockoutEnvVar: "", + setDisableLockoutAuthTune: true, + expectedUserLocked: false, + }, + { + name: "User Lockout feature is disabled using env var VAULT_DISABLE_USER_LOCKOUT", + setDisableUserLockoutEnvVar: "true", + setDisableLockoutAuthTune: false, + expectedUserLocked: false, + }, + { + name: "User lockout feature is enabled using env variable, disabled using auth tune", + setDisableUserLockoutEnvVar: "false", + setDisableLockoutAuthTune: true, + expectedUserLocked: true, + }, + { + name: "User lockout feature is disabled using auth tune and env variable", + setDisableUserLockoutEnvVar: "true", + setDisableLockoutAuthTune: true, + expectedUserLocked: false, + }, } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.setDisableUserLockoutEnvVar != "" { + os.Setenv("VAULT_DISABLE_USER_LOCKOUT", tt.setDisableUserLockoutEnvVar) + } else { + os.Unsetenv("VAULT_DISABLE_USER_LOCKOUT") + } - // create another user for userpass with a different case - if _, err = standby.Logical().Write("auth/userpass/users/bSmith", map[string]interface{}{ - "password": "training", - }); err != nil { - t.Fatal(err) - } + var disableLockoutAuthTune *bool - // login failure count 1 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - // login failure count 2 - standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "wrongPassword", - }) - // login : permission denied as user locked out - if _, err = standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "training", - }); err == nil { - t.Fatal("expected login to fail as user locked out") - } - if !strings.Contains(err.Error(), logical.ErrPermissionDenied.Error()) { - t.Fatalf("expected to see permission denied error as user locked out, got %v", err) - } + // default for disable lockout is false + disableLockoutAuthTune = &enableLockout - // unlock user - if _, err = standby.Logical().Write("sys/locked-users/"+mountAccessor+"/unlock/bsmith", nil); err != nil { - t.Fatal(err) - } + if tt.setDisableLockoutAuthTune == true { + disableLockoutAuthTune = &disableLockout + } - // login: should be successful as user unlocked - if _, err = standby.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ - "password": "training", - }); err != nil { - t.Fatal("expected login to succeed as user is unlocked") - } + // tune auth mount + userlockoutConfig := &api.UserLockoutConfigInput{ + DisableLockout: disableLockoutAuthTune, + } + err := client.Sys().TuneMount("auth/userpass", api.MountConfigInput{ + UserLockoutConfig: userlockoutConfig, + }) + if err != nil { + t.Fatal(err) + } - // login failure count 1 for user bSmith - standby.Logical().Write("auth/userpass/login/bSmith", map[string]interface{}{ - "password": "wrongPassword", - }) - // login failure count 2 for user bSmith - standby.Logical().Write("auth/userpass/login/bSmith", map[string]interface{}{ - "password": "wrongPassword", - }) - // login : permission denied as user locked out for user bSmith - if _, err = standby.Logical().Write("auth/userpass/login/bSmith", map[string]interface{}{ - "password": "training", - }); err == nil { - t.Fatal("expected login to fail as user locked out") - } - if !strings.Contains(err.Error(), logical.ErrPermissionDenied.Error()) { - t.Fatalf("expected to see permission denied error as user locked out, got %v", err) - } + // login for default lockout threshold times with wrong credentials + for i := 0; i < UserLockoutThresholdDefault; i++ { + _, err = client.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ + "password": "wrongPassword", + }) + if err == nil { + t.Fatal("expected login to fail due to wrong credentials") + } + if !strings.Contains(err.Error(), "invalid username or password") { + t.Fatal(err) + } + } - // unlock user bSmith - if _, err = standby.Logical().Write("sys/locked-users/"+mountAccessor+"/unlock/bSmith", nil); err != nil { - t.Fatal(err) - } + // login to check if user locked + _, err = client.Logical().Write("auth/userpass/login/bsmith", map[string]interface{}{ + "password": "wrongPassword", + }) + if err == nil { + t.Fatal("expected login to fail due to wrong credentials") + } - // login: should be successful as user bSmith unlocked - if _, err = standby.Logical().Write("auth/userpass/login/bSmith", map[string]interface{}{ - "password": "training", - }); err != nil { - t.Fatal("expected login to succeed as user is unlocked") - } + switch tt.expectedUserLocked { + case true: + if !strings.Contains(err.Error(), logical.ErrPermissionDenied.Error()) { + t.Fatalf("expected user to get locked but got %v", err) + } + // user locked, unlock user to perform next test iteration + if _, err = client.Logical().Write("sys/locked-users/"+mountAccessor+"/unlock/bsmith", nil); err != nil { + t.Fatal(err) + } - // unlock unlocked user - if _, err = active.Logical().Write("sys/locked-users/mountAccessor/unlock/bsmith", nil); err != nil { - t.Fatal(err) + default: + if !strings.Contains(err.Error(), "invalid username or password") { + t.Fatalf("expected user to be unlocked but locked, got %v", err) + } + } + }) } } diff --git a/vault/logical_system_user_lockout.go b/vault/logical_system_user_lockout.go index c255c9de5..edd0a61e3 100644 --- a/vault/logical_system_user_lockout.go +++ b/vault/logical_system_user_lockout.go @@ -35,6 +35,8 @@ func unlockUser(ctx context.Context, core *Core, mountAccessor string, aliasName lockedUserStoragePath := coreLockedUsersPath + ns.ID + "/" + mountAccessor + "/" + aliasName // remove entry for locked user from storage + // if read only error, the error is handled by handleError in logical_system.go + // this will be forwarded to the active node if err := core.barrier.Delete(ctx, lockedUserStoragePath); err != nil { return err } @@ -44,7 +46,7 @@ func unlockUser(ctx context.Context, core *Core, mountAccessor string, aliasName mountAccessor: mountAccessor, } - // remove entry for locked user from userFailedLoginInfo map + // remove entry for locked user from userFailedLoginInfo map and storage if err := updateUserFailedLoginInfo(ctx, core, loginUserInfoKey, nil, true); err != nil { return err } diff --git a/vault/request_handling.go b/vault/request_handling.go index 29b227774..d23232854 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1402,9 +1402,11 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // if routeErr has invalid credentials error, update the userFailedLoginMap if routeErr != nil && routeErr == logical.ErrInvalidCredentials { - err := c.failedUserLoginProcess(ctx, entry, req) - if err != nil { - return nil, nil, err + if !isUserLockoutDisabled { + err := c.failedUserLoginProcess(ctx, entry, req) + if err != nil { + return nil, nil, err + } } return nil, nil, resp.Error() } @@ -1689,6 +1691,10 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re aliasName: auth.Alias.Name, mountAccessor: auth.Alias.MountAccessor, } + + // We don't need to try to delete the lockedUsers storage entry, since we're + // processing a login request. If a login attempt is allowed, it means the user is + // unlocked and we only add storage entry when the user gets locked. err = updateUserFailedLoginInfo(ctx, c, loginUserInfoKey, nil, true) if err != nil { return nil, nil, err @@ -1855,39 +1861,12 @@ func (c *Core) failedUserLoginProcess(ctx context.Context, mountEntry *MountEntr } } - // update the userFailedLoginInfo map with the updated/new entry + // update the userFailedLoginInfo map (and/or storage) with the updated/new entry err = updateUserFailedLoginInfo(ctx, c, loginUserInfoKey, &failedLoginInfo, false) if err != nil { return err } - // if failed login count has reached threshold, create a storage entry as the user got locked - if failedLoginInfo.count >= uint(userLockoutConfiguration.LockoutThreshold) { - // user locked - ns, err := namespace.FromContext(ctx) - if err != nil { - return fmt.Errorf("could not parse namespace from http context: %w", err) - } - storageUserLockoutPath := fmt.Sprintf(coreLockedUsersPath+"%s/%s/%s", ns.ID, loginUserInfoKey.mountAccessor, loginUserInfoKey.aliasName) - compressedBytes, err := jsonutil.EncodeJSONAndCompress(failedLoginInfo.lastFailedLoginTime, nil) - if err != nil { - c.logger.Error("failed to encode or compress failed login user entry", "error", err) - return err - } - - // Create an entry - entry := &logical.StorageEntry{ - Key: storageUserLockoutPath, - Value: compressedBytes, - } - - // Write to the physical backend - if err := c.barrier.Put(ctx, entry); err != nil { - c.logger.Error("failed to persist failed login user entry", "error", err) - return err - } - - } return nil } @@ -1916,16 +1895,16 @@ func (c *Core) isUserLockoutDisabled(mountEntry *MountEntry) (bool, error) { } // check environment variable - var disableUserLockout bool if disableUserLockoutEnv := os.Getenv(consts.VaultDisableUserLockout); disableUserLockoutEnv != "" { var err error - disableUserLockout, err = strconv.ParseBool(disableUserLockoutEnv) + disableUserLockout, err := strconv.ParseBool(disableUserLockoutEnv) if err != nil { return false, errors.New("Error parsing the environment variable VAULT_DISABLE_USER_LOCKOUT") } - } - if disableUserLockout { - return true, nil + if disableUserLockout { + return true, nil + } + return false, nil } // read auth tune for mount entry @@ -1986,13 +1965,11 @@ func (c *Core) isUserLocked(ctx context.Context, mountEntry *MountEntry, req *lo if time.Now().Unix()-int64(lastLoginTime) < int64(userLockoutConfiguration.LockoutDuration.Seconds()) { // user locked return true, nil - } else { - // user is not locked. Entry is stale, remove this from storage - if err := c.barrier.Delete(ctx, storageUserLockoutPath); err != nil { - c.logger.Error("failed to cleanup storage entry for user", "path", storageUserLockoutPath, "error", err) - } } + // else user is not locked. Entry is stale, this will be removed from storage during cleanup + // by the background thread + default: // entry found in userFailedLoginInfo map, check if the user is locked isCountOverLockoutThreshold := userFailedLoginInfo.count >= uint(userLockoutConfiguration.LockoutThreshold) @@ -2185,7 +2162,11 @@ func (c *Core) RegisterAuth(ctx context.Context, tokenTTL time.Duration, path st aliasName: auth.Alias.Name, mountAccessor: auth.Alias.MountAccessor, } - err = c.UpdateUserFailedLoginInfo(ctx, loginUserInfoKey, nil, true) + + // We don't need to try to delete the lockedUsers storage entry, since we're + // processing a login request. If a login attempt is allowed, it means the user is + // unlocked and we only add storage entry when the user gets locked. + err = updateUserFailedLoginInfo(ctx, c, loginUserInfoKey, nil, true) if err != nil { return err } @@ -2194,8 +2175,8 @@ func (c *Core) RegisterAuth(ctx context.Context, tokenTTL time.Duration, path st return nil } -// GetUserFailedLoginInfo gets the failed login information for a user based on alias name and mountAccessor -func (c *Core) GetUserFailedLoginInfo(ctx context.Context, userKey FailedLoginUser) *FailedLoginInfo { +// LocalGetUserFailedLoginInfo gets the failed login information for a user based on alias name and mountAccessor +func (c *Core) LocalGetUserFailedLoginInfo(ctx context.Context, userKey FailedLoginUser) *FailedLoginInfo { c.userFailedLoginInfoLock.Lock() value, exists := c.userFailedLoginInfo[userKey] c.userFailedLoginInfoLock.Unlock() @@ -2205,23 +2186,52 @@ func (c *Core) GetUserFailedLoginInfo(ctx context.Context, userKey FailedLoginUs return nil } -// UpdateUserFailedLoginInfo updates the failed login information for a user based on alias name and mountAccessor -func (c *Core) UpdateUserFailedLoginInfo(ctx context.Context, userKey FailedLoginUser, failedLoginInfo *FailedLoginInfo, deleteEntry bool) error { +// LocalUpdateUserFailedLoginInfo updates the failed login information for a user based on alias name and mountAccessor +func (c *Core) LocalUpdateUserFailedLoginInfo(ctx context.Context, userKey FailedLoginUser, failedLoginInfo *FailedLoginInfo, deleteEntry bool) error { c.userFailedLoginInfoLock.Lock() switch deleteEntry { case false: - // create or update entry in the map + // update entry in the map c.userFailedLoginInfo[userKey] = failedLoginInfo + + // get the user lockout configuration for the user + mountEntry := c.router.MatchingMountByAccessor(userKey.mountAccessor) + if mountEntry == nil { + mountEntry = &MountEntry{} + mountEntry.NamespaceID = namespace.RootNamespaceID + } + userLockoutConfiguration := c.getUserLockoutConfiguration(mountEntry) + + // if failed login count has reached threshold, create a storage entry as the user got locked + if failedLoginInfo.count >= uint(userLockoutConfiguration.LockoutThreshold) { + // user locked + storageUserLockoutPath := fmt.Sprintf(coreLockedUsersPath+"%s/%s/%s", mountEntry.NamespaceID, userKey.mountAccessor, userKey.aliasName) + + compressedBytes, err := jsonutil.EncodeJSONAndCompress(failedLoginInfo.lastFailedLoginTime, nil) + if err != nil { + c.logger.Error("failed to encode or compress failed login user entry", "error", err) + return err + } + + // Create an entry + entry := &logical.StorageEntry{ + Key: storageUserLockoutPath, + Value: compressedBytes, + } + + // Write to the physical backend + if err := c.barrier.Put(ctx, entry); err != nil { + c.logger.Error("failed to persist failed login user entry", "error", err) + return err + } + + } + default: - // delete the entry from the map + // delete the entry from the map, if no key exists it is no-op delete(c.userFailedLoginInfo, userKey) } c.userFailedLoginInfoLock.Unlock() - // check if the update worked - failedLoginResp := c.GetUserFailedLoginInfo(ctx, userKey) - if (failedLoginResp == nil && !deleteEntry) || (failedLoginResp != nil && deleteEntry) { - return fmt.Errorf("failed to update entry in userFailedLoginInfo map") - } return nil } diff --git a/vault/request_handling_util.go b/vault/request_handling_util.go index c0a52e613..c7dfe7180 100644 --- a/vault/request_handling_util.go +++ b/vault/request_handling_util.go @@ -55,11 +55,11 @@ func getAuthRegisterFunc(c *Core) (RegisterAuthFunc, error) { } func getUserFailedLoginInfo(ctx context.Context, c *Core, userInfo FailedLoginUser) (*FailedLoginInfo, error) { - return c.GetUserFailedLoginInfo(ctx, userInfo), nil + return c.LocalGetUserFailedLoginInfo(ctx, userInfo), nil } func updateUserFailedLoginInfo(ctx context.Context, c *Core, userInfo FailedLoginUser, failedLoginInfo *FailedLoginInfo, deleteEntry bool) error { - return c.UpdateUserFailedLoginInfo(ctx, userInfo, failedLoginInfo, deleteEntry) + return c.LocalUpdateUserFailedLoginInfo(ctx, userInfo, failedLoginInfo, deleteEntry) } func possiblyForwardAliasCreation(ctx context.Context, c *Core, inErr error, auth *logical.Auth, entity *identity.Entity) (*identity.Entity, bool, error) {