From 9c14ea81149800428f8eae4fab86aa3cd6923498 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, 25 Oct 2023 11:38:58 -0400 Subject: [PATCH] Revert "Implement user lockout log (#23140)" (#23741) (#23765) This reverts commit 92fcfda8ad30a539be67b7fb7abff539bf93a098. Co-authored-by: davidadeleon <56207066+davidadeleon@users.noreply.github.com> --- changelog/23140.txt | 3 -- command/server.go | 1 - command/server/config_test_helpers.go | 1 - http/sys_config_state_test.go | 1 - internalshared/configutil/config.go | 13 +---- internalshared/configutil/merge.go | 5 -- vault/core.go | 68 +-------------------------- vault/logical_system_user_lockout.go | 12 ----- vault/request_handling.go | 4 +- 9 files changed, 3 insertions(+), 105 deletions(-) delete mode 100644 changelog/23140.txt diff --git a/changelog/23140.txt b/changelog/23140.txt deleted file mode 100644 index c030090bb..000000000 --- a/changelog/23140.txt +++ /dev/null @@ -1,3 +0,0 @@ -```release-note:improvement -core: emit logs when user(s) are locked out and when all lockouts have been cleared -``` \ No newline at end of file diff --git a/command/server.go b/command/server.go index 543706ea9..73975e233 100644 --- a/command/server.go +++ b/command/server.go @@ -2829,7 +2829,6 @@ func createCoreConfig(c *ServerCommand, config *server.Config, backend physical. DisableSSCTokens: config.DisableSSCTokens, Experiments: config.Experiments, AdministrativeNamespacePath: config.AdministrativeNamespacePath, - UserLockoutLogInterval: config.UserLockoutLogInterval, } if c.flagDev { diff --git a/command/server/config_test_helpers.go b/command/server/config_test_helpers.go index d2589915d..ce33c090e 100644 --- a/command/server/config_test_helpers.go +++ b/command/server/config_test_helpers.go @@ -776,7 +776,6 @@ func testConfig_Sanitized(t *testing.T) { "enable_response_header_hostname": false, "enable_response_header_raft_node_id": false, "log_requests_level": "basic", - "user_lockout_log_interval": 0 * time.Second, "ha_storage": map[string]interface{}{ "cluster_addr": "top_level_cluster_addr", "disable_clustering": true, diff --git a/http/sys_config_state_test.go b/http/sys_config_state_test.go index 7bc541fda..5dfdf27f5 100644 --- a/http/sys_config_state_test.go +++ b/http/sys_config_state_test.go @@ -167,7 +167,6 @@ func TestSysConfigState_Sanitized(t *testing.T) { "enable_response_header_hostname": false, "enable_response_header_raft_node_id": false, "log_requests_level": "", - "user_lockout_log_interval": json.Number("0"), "listeners": []interface{}{ map[string]interface{}{ "config": nil, diff --git a/internalshared/configutil/config.go b/internalshared/configutil/config.go index 755889202..04f94a854 100644 --- a/internalshared/configutil/config.go +++ b/internalshared/configutil/config.go @@ -23,9 +23,7 @@ type SharedConfig struct { Listeners []*Listener `hcl:"-"` - UserLockouts []*UserLockout `hcl:"-"` - UserLockoutLogInterval time.Duration `hcl:"-"` - UserLockoutLogIntervalRaw interface{} `hcl:"user_lockout_log_interval"` + UserLockouts []*UserLockout `hcl:"-"` Seals []*KMS `hcl:"-"` Entropy *Entropy `hcl:"-"` @@ -89,14 +87,6 @@ func ParseConfig(d string) (*SharedConfig, error) { result.DisableMlockRaw = nil } - if result.UserLockoutLogIntervalRaw != nil { - if result.UserLockoutLogInterval, err = parseutil.ParseDurationSecond(result.UserLockoutLogIntervalRaw); err != nil { - return nil, err - } - result.FoundKeys = append(result.FoundKeys, "UserLockoutLogInterval") - result.UserLockoutLogIntervalRaw = nil - } - list, ok := obj.Node.(*ast.ObjectList) if !ok { return nil, fmt.Errorf("error parsing: file doesn't contain a root object") @@ -186,7 +176,6 @@ func (c *SharedConfig) Sanitized() map[string]interface{} { "pid_file": c.PidFile, "cluster_name": c.ClusterName, "administrative_namespace_path": c.AdministrativeNamespacePath, - "user_lockout_log_interval": c.UserLockoutLogInterval, } // Optional log related settings diff --git a/internalshared/configutil/merge.go b/internalshared/configutil/merge.go index b6061937a..940e8bfcf 100644 --- a/internalshared/configutil/merge.go +++ b/internalshared/configutil/merge.go @@ -56,11 +56,6 @@ func (c *SharedConfig) Merge(c2 *SharedConfig) *SharedConfig { result.DefaultMaxRequestDuration = c2.DefaultMaxRequestDuration } - result.UserLockoutLogInterval = c.UserLockoutLogInterval - if c2.UserLockoutLogInterval > result.UserLockoutLogInterval { - result.UserLockoutLogInterval = c2.UserLockoutLogInterval - } - result.LogLevel = c.LogLevel if c2.LogLevel != "" { result.LogLevel = c2.LogLevel diff --git a/vault/core.go b/vault/core.go index 2767d6bf4..7392c1c37 100644 --- a/vault/core.go +++ b/vault/core.go @@ -104,11 +104,6 @@ const ( // MfaAuthResponse when the value is not specified in the server config defaultMFAAuthResponseTTL = 300 * time.Second - // defaultUserLockoutLogInterval is the default duration that Vault will - // emit a log informing that a user lockout is in effect when the value - // is not specified in the server config - defaultUserLockoutLogInterval = 1 * time.Minute - // defaultMaxTOTPValidateAttempts is the default value for the number // of failed attempts to validate a request subject to TOTP MFA. If the // number of failed totp passcode validations exceeds this max value, the @@ -663,9 +658,6 @@ type Core struct { updateLockedUserEntriesCancel context.CancelFunc - lockoutLoggerCancel context.CancelFunc - userLockoutLogInterval time.Duration - // number of workers to use for lease revocation in the expiration manager numExpirationWorkers int @@ -879,8 +871,6 @@ type CoreConfig struct { // only accessible in the root namespace, currently sys/audit-hash and sys/monitor. AdministrativeNamespacePath string - UserLockoutLogInterval time.Duration - NumRollbackWorkers int } @@ -929,10 +919,6 @@ func CreateCore(conf *CoreConfig) (*Core, error) { return nil, fmt.Errorf("cannot have DefaultLeaseTTL larger than MaxLeaseTTL") } - if conf.UserLockoutLogInterval == 0 { - conf.UserLockoutLogInterval = defaultUserLockoutLogInterval - } - // Validate the advertise addr if its given to us if conf.RedirectAddr != "" { u, err := url.Parse(conf.RedirectAddr) @@ -1057,7 +1043,6 @@ func CreateCore(conf *CoreConfig) (*Core, error) { disableSSCTokens: conf.DisableSSCTokens, effectiveSDKVersion: effectiveSDKVersion, userFailedLoginInfo: make(map[FailedLoginUser]*FailedLoginInfo), - userLockoutLogInterval: conf.UserLockoutLogInterval, experiments: conf.Experiments, pendingRemovalMountsAllowed: conf.PendingRemovalMountsAllowed, expirationRevokeRetryBase: conf.ExpirationRevokeRetryBase, @@ -3537,51 +3522,6 @@ func (c *Core) setupCachedMFAResponseAuth() { return } -func (c *Core) startLockoutLogger() { - // Are we already running a logger - if c.lockoutLoggerCancel != nil { - return - } - - ctx, cancelFunc := context.WithCancel(c.activeContext) - c.lockoutLoggerCancel = cancelFunc - - // Perform first check for lockout entries - lockedUserCount := c.getUserFailedLoginCount(ctx) - - if lockedUserCount > 0 { - c.Logger().Warn("user lockout(s) in effect") - } else { - // We shouldn't end up here - return - } - - // Start lockout watcher - go func() { - ticker := time.NewTicker(c.userLockoutLogInterval) - for { - select { - case <-ticker.C: - // Check for lockout entries - lockedUserCount := c.getUserFailedLoginCount(ctx) - - if lockedUserCount > 0 { - c.Logger().Warn("user lockout(s) in effect") - break - } - c.Logger().Info("user lockout(s) cleared") - ticker.Stop() - c.lockoutLoggerCancel = nil - return - case <-ctx.Done(): - ticker.Stop() - c.lockoutLoggerCancel = nil - return - } - } - }() -} - // updateLockedUserEntries runs every 15 mins to remove stale user entries from storage // it also updates the userFailedLoginInfo map with correct information for locked users if incorrect func (c *Core) updateLockedUserEntries() { @@ -3610,13 +3550,7 @@ func (c *Core) updateLockedUserEntries() { } } }() -} - -func (c *Core) getUserFailedLoginCount(ctx context.Context) int { - c.userFailedLoginInfoLock.Lock() - defer c.userFailedLoginInfoLock.Unlock() - - return len(c.userFailedLoginInfo) + return } // runLockedUserEntryUpdates runs updates for locked user storage entries and userFailedLoginInfo map diff --git a/vault/logical_system_user_lockout.go b/vault/logical_system_user_lockout.go index bc3aa9460..edd0a61e3 100644 --- a/vault/logical_system_user_lockout.go +++ b/vault/logical_system_user_lockout.go @@ -51,18 +51,6 @@ func unlockUser(ctx context.Context, core *Core, mountAccessor string, aliasName return err } - if core.lockoutLoggerCancel != nil { - // Check if we have no more locked users and cancel any running lockout logger - core.userFailedLoginInfoLock.RLock() - numLockedUsers := len(core.userFailedLoginInfo) - core.userFailedLoginInfoLock.RUnlock() - - if numLockedUsers == 0 { - core.Logger().Info("user lockout(s) cleared") - core.lockoutLoggerCancel() - } - } - return nil } diff --git a/vault/request_handling.go b/vault/request_handling.go index e9fa3aefa..76b3837a5 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1464,7 +1464,6 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re return nil, nil, err } if isloginUserLocked { - c.startLockoutLogger() return nil, nil, logical.ErrPermissionDenied } } @@ -2280,8 +2279,6 @@ func (c *Core) LocalGetUserFailedLoginInfo(ctx context.Context, userKey FailedLo // 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() - defer c.userFailedLoginInfoLock.Unlock() - switch deleteEntry { case false: // update entry in the map @@ -2324,6 +2321,7 @@ func (c *Core) LocalUpdateUserFailedLoginInfo(ctx context.Context, userKey Faile // delete the entry from the map, if no key exists it is no-op delete(c.userFailedLoginInfo, userKey) } + c.userFailedLoginInfoLock.Unlock() return nil }