From a94729663b2798ce90cbb47e6cd0ff9445be93df Mon Sep 17 00:00:00 2001 From: David De Leon <56207066+davidadeleon@users.noreply.github.com> Date: Fri, 20 Oct 2023 11:30:08 -0400 Subject: [PATCH] Revert "Implement user lockout log (#23140)" This reverts commit 8f70fb907458ae0325191deba35b476e1a4bc33f. --- 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 c030090bbbcf..000000000000 --- 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 6f1103edf2ca..a75a6d749d43 100644 --- a/command/server.go +++ b/command/server.go @@ -2751,7 +2751,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 9de386a1ff23..77b0c5ef92fb 100644 --- a/command/server/config_test_helpers.go +++ b/command/server/config_test_helpers.go @@ -773,7 +773,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 d7886c967e25..405f0597d82b 100644 --- a/http/sys_config_state_test.go +++ b/http/sys_config_state_test.go @@ -164,7 +164,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 fe0901b1edaf..7ca21eba48a2 100644 --- a/internalshared/configutil/config.go +++ b/internalshared/configutil/config.go @@ -20,9 +20,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:"-"` @@ -86,14 +84,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") @@ -183,7 +173,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 982ff31729ee..4bc30e62d829 100644 --- a/internalshared/configutil/merge.go +++ b/internalshared/configutil/merge.go @@ -53,11 +53,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 cc71577ca0ae..e0ac1430a1c4 100644 --- a/vault/core.go +++ b/vault/core.go @@ -101,11 +101,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 @@ -660,9 +655,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 @@ -872,8 +864,6 @@ type CoreConfig struct { // only accessible in the root namespace, currently sys/audit-hash and sys/monitor. AdministrativeNamespacePath string - UserLockoutLogInterval time.Duration - NumRollbackWorkers int } @@ -922,10 +912,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) @@ -1050,7 +1036,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, @@ -3525,51 +3510,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() { @@ -3598,13 +3538,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 4d449de10139..de1335a286f8 100644 --- a/vault/logical_system_user_lockout.go +++ b/vault/logical_system_user_lockout.go @@ -48,18 +48,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 437cbe61c295..20bff41e7ad5 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -1447,7 +1447,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 } } @@ -2263,8 +2262,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 @@ -2307,6 +2304,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 }