addressing comments

changelog/17338.txt

@@ -1,3 +1,3 @@
 ```release-note:feature
-core: Add user lockout field to config and configuring this for auth mount using auth tune 
+core: Add user lockout field to config and configuring this for auth mount using auth tune to prevent brute forcing in auth methods 
 ```

command/auth_tune.go

@@ -150,15 +150,15 @@ func (c *AuthTuneCommand) Flags() *FlagSets {
  })
 
  f.IntVar(&IntVar{
- Name: "user-lockout-threshold",
+ Name: flagNameUserLockoutThreshold,
  Target: &c.flagUserLockoutThreshold,
  Usage: "The threshold for user lockout for this auth method. If unspecified, this " +
  "defaults to the Vault server's globally configured user lockout threshold, " +
  "or a previously configured value for the auth method.",
  })
 
  f.DurationVar(&DurationVar{
- Name: "user-lockout-duration",
+ Name: flagNameUserLockoutDuration,
  Target: &c.flagUserLockoutDuration,
  Completion: complete.PredictAnything,
  Usage: "The user lockout duration for this auth method. If unspecified, this " +
@@ -167,7 +167,7 @@ func (c *AuthTuneCommand) Flags() *FlagSets {
  })
 
  f.DurationVar(&DurationVar{
- Name: "user-lockout-counter-reset-duration",
+ Name: flagNameUserLockoutCounterResetDuration,
  Target: &c.flagUserLockoutCounterResetDuration,
  Completion: complete.PredictAnything,
  Usage: "The user lockout counter reset duration for this auth method. If unspecified, this " +
@@ -176,7 +176,7 @@ func (c *AuthTuneCommand) Flags() *FlagSets {
  })
 
  f.BoolVar(&BoolVar{
- Name: "user-lockout-disable",
+ Name: flagNameUserLockoutDisable,
  Target: &c.flagUserLockoutDisable,
  Default: false,
  Usage: "Disable user lockout for this auth method. If unspecified, this " +

command/commands.go

@@ -126,6 +126,14 @@ const (
  flagNameAllowedManagedKeys = "allowed-managed-keys"
  // flagNamePluginVersion selects what version of a plugin should be used.
  flagNamePluginVersion = "plugin-version"
+ // flagNameUserLockoutThreshold is the flag name used for tuning the auth mount lockout threshold parameter
+ flagNameUserLockoutThreshold = "user-lockout-threshold"
+ // flagNameUserLockoutDuration is the flag name used for tuning the auth mount lockout duration parameter
+ flagNameUserLockoutDuration = "user-lockout-duration"
+ // flagNameUserLockoutCounterResetDuration is the flag name used for tuning the auth mount lockout counter reset parameter
+ flagNameUserLockoutCounterResetDuration = "user-lockout-counter-reset-duration"
+ // flagNameUserLockoutDisable is the flag name used for tuning the auth mount disable lockout parameter
+ flagNameUserLockoutDisable = "user-lockout-disable"
 )
 
 var (

internalshared/configutil/userlockout.go

@@ -52,6 +52,8 @@ func ParseUserLockouts(result *SharedConfig, list *ast.ObjectList) error {
  }
 
  userLockoutConfig.Type = strings.ToLower(userLockoutConfig.Type)
+ // Supported auth methods for user lockout configuration: ldap, approle, userpass
+ // "all" is used to apply the configuration to all supported auth methods
  switch userLockoutConfig.Type {
  case "all", "ldap", "approle", "userpass":
  result.found(userLockoutConfig.Type, userLockoutConfig.Type)
@@ -103,7 +105,8 @@ func ParseUserLockouts(result *SharedConfig, list *ast.ObjectList) error {
  return nil
 }
 
-// setDefaultUserLockoutValuesInMap sets default user lockout values for key "all" (all auth methods) for user lockout fields that are not configured using config file
+// setDefaultUserLockoutValuesInMap sets default user lockout values for key "all" (all auth methods)
+// for user lockout fields that are not configured using config file
 func setDefaultUserLockoutValuesInMap(userLockoutsMap map[string]*UserLockout) map[string]*UserLockout {
  if userLockoutAll, ok := userLockoutsMap["all"]; !ok {
  var tmpUserLockoutConfig UserLockout
@@ -132,7 +135,8 @@ func setDefaultUserLockoutValuesInMap(userLockoutsMap map[string]*UserLockout) m
  return userLockoutsMap
 }
 
-// setDefaultUserLockoutValuesInMap sets missing user lockout fields for auth methods with default values (from key "all") that are not configured using config file
+// setDefaultUserLockoutValuesInMap sets missing user lockout fields for auth methods
+// with default values (from key "all") that are not configured using config file
 func setMissingUserLockoutValuesInMap(userLockoutsMap map[string]*UserLockout) map[string]*UserLockout {
  userLockoutsMap = setDefaultUserLockoutValuesInMap(userLockoutsMap)
  for _, userLockoutAuth := range userLockoutsMap {

vault/logical_system.go

@@ -938,7 +938,7 @@ func (b *SystemBackend) mountInfo(ctx context.Context, entry *MountEntry) map[st
  if entry.Table == credentialTableType {
  entryConfig["token_type"] = entry.Config.TokenType.String()
  }
- if (entry.Config.UserLockoutConfig != UserLockoutConfig{}) {
+ if entry.Config.UserLockoutConfig != nil {
  userLockoutConfig := map[string]interface{}{
  "user_lockout_counter_reset_duration": int64(entry.Config.UserLockoutConfig.LockoutCounterReset.Seconds()),
  "user_lockout_threshold": entry.Config.UserLockoutConfig.LockoutThreshold,
@@ -1607,7 +1607,7 @@ func (b *SystemBackend) handleTuneReadCommon(ctx context.Context, path string) (
  resp.Data["allowed_managed_keys"] = rawVal.([]string)
  }
 
- if (mountEntry.Config.UserLockoutConfig != UserLockoutConfig{}) {
+ if mountEntry.Config.UserLockoutConfig != nil {
  resp.Data["user_lockout_counter_reset_duration"] = int64(mountEntry.Config.UserLockoutConfig.LockoutCounterReset.Seconds())
  resp.Data["user_lockout_threshold"] = mountEntry.Config.UserLockoutConfig.LockoutThreshold
  resp.Data["user_lockout_duration"] = int64(mountEntry.Config.UserLockoutConfig.LockoutDuration.Seconds())
@@ -1740,12 +1740,6 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string,
  userLockoutConfigMap := data.Get("user_lockout_config").(map[string]interface{})
  var err error
  // Augmenting userLockoutConfigMap for some config options to treat them as comma separated entries
- err = expandStringValsWithCommas(userLockoutConfigMap)
- if err != nil {
- return logical.ErrorResponse(
- "unable to parse given auth user lockout config information"),
- logical.ErrInvalidRequest
- }
  if userLockoutConfigMap != nil && len(userLockoutConfigMap) != 0 {
  err := mapstructure.Decode(userLockoutConfigMap, &apiuserLockoutConfig)
  if err != nil {
@@ -1754,6 +1748,8 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string,
  logical.ErrInvalidRequest
  }
 
+ // Supported auth methods for user lockout configuration: ldap, approle, userpass
+ // "all" is used to apply the configuration to all supported auth methods
  switch strings.ToLower(mountEntry.Type) {
  case "all", "ldap", "approle", "userpass":
  default:
@@ -1763,21 +1759,25 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string,
  }
  }
 
+ if mountEntry.Config.UserLockoutConfig == nil {
+ mountEntry.Config.UserLockoutConfig = &UserLockoutConfig{}
+ }
+
  if _, ok := userLockoutConfigMap["lockout_threshold"]; ok {
- var newUserLockoutThreshold int64
  userLockoutThreshold, err := parseutil.ParseInt(apiuserLockoutConfig.LockoutThreshold)
  if err != nil {
  return nil, fmt.Errorf("unable to parse user lockout threshold: %w", err)
  }
+
  oldUserLockoutThreshold := mountEntry.Config.UserLockoutConfig.LockoutThreshold
 
  switch {
  case userLockoutThreshold < 0:
- newUserLockoutThreshold = oldUserLockoutThreshold
+ mountEntry.Config.UserLockoutConfig.LockoutThreshold = oldUserLockoutThreshold
  default:
- newUserLockoutThreshold = userLockoutThreshold
+ mountEntry.Config.UserLockoutConfig.LockoutThreshold = userLockoutThreshold
  }
- mountEntry.Config.UserLockoutConfig.LockoutThreshold = newUserLockoutThreshold
+
  // Update the mount table
  switch {
  case strings.HasPrefix(path, "auth/"):
@@ -1795,9 +1795,11 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string,
 
  }
 
- {
- var newUserLockoutDuration time.Duration
- oldUserLockoutDuration := mountEntry.Config.UserLockoutConfig.LockoutDuration
+ if _, ok := userLockoutConfigMap["lockout_duration"]; ok {
+ var newUserLockoutDuration, oldUserLockoutDuration time.Duration
+
+ oldUserLockoutDuration = mountEntry.Config.UserLockoutConfig.LockoutDuration
+
  switch apiuserLockoutConfig.LockoutDuration {
 
  case "":
@@ -1830,9 +1832,12 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string,
  }
 
  }
- {
- var newUserLockoutCounterReset time.Duration
- oldUserLockoutCounterReset := mountEntry.Config.UserLockoutConfig.LockoutCounterReset
+
+ if _, ok := userLockoutConfigMap["lockout_counter_reset_duration"]; ok {
+ var newUserLockoutCounterReset, oldUserLockoutCounterReset time.Duration
+
+ oldUserLockoutCounterReset = mountEntry.Config.UserLockoutConfig.LockoutCounterReset
+
  switch apiuserLockoutConfig.LockoutCounterResetDuration {
  case "":
  newUserLockoutCounterReset = oldUserLockoutCounterReset
@@ -1862,27 +1867,28 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string,
  b.Core.logger.Info("tuning of user-lockout-counter-reset successful", "path", path, "user-lockout-counter-reset", apiuserLockoutConfig.LockoutCounterResetDuration)
  }
  }
- {
- if rawVal, ok := userLockoutConfigMap["lockout_disable"]; ok {
- userLockoutDisable := rawVal.(bool)
- oldUserLockoutDisable := mountEntry.Config.UserLockoutConfig.DisableLockout
- mountEntry.Config.UserLockoutConfig.DisableLockout = userLockoutDisable
- var err error
- switch {
- case strings.HasPrefix(path, "auth/"):
- err = b.Core.persistAuth(ctx, b.Core.auth, &mountEntry.Local)
- default:
- err = b.Core.persistMounts(ctx, b.Core.mounts, &mountEntry.Local)
- }
- if err != nil {
- mountEntry.Config.UserLockoutConfig.DisableLockout = oldUserLockoutDisable
- return handleError(err)
- }
- if b.Core.logger.IsInfo() {
- b.Core.logger.Info("tuning of user-lockout-disable successful", "path", path, "user-lockout-disable", userLockoutDisable)
- }
 
+ if rawVal, ok := userLockoutConfigMap["lockout_disable"]; ok {
+ userLockoutDisable := rawVal.(bool)
+ var oldUserLockoutDisable bool
+
+ oldUserLockoutDisable = mountEntry.Config.UserLockoutConfig.DisableLockout
+ mountEntry.Config.UserLockoutConfig.DisableLockout = userLockoutDisable
+ var err error
+ switch {
+ case strings.HasPrefix(path, "auth/"):
+ err = b.Core.persistAuth(ctx, b.Core.auth, &mountEntry.Local)
+ default:
+ err = b.Core.persistMounts(ctx, b.Core.mounts, &mountEntry.Local)
  }
+ if err != nil {
+ mountEntry.Config.UserLockoutConfig.DisableLockout = oldUserLockoutDisable
+ return handleError(err)
+ }
+ if b.Core.logger.IsInfo() {
+ b.Core.logger.Info("tuning of user-lockout-disable successful", "path", path, "user-lockout-disable", userLockoutDisable)
+ }
+
  }
 
  }
@@ -5216,6 +5222,10 @@ in the plugin catalog.`,
  `The options to pass into the backend. Should be a json object with string keys and values.`,
  },
 
+ "tune_user_lockout_config": {
+ `The user lockout configuration to pass into the backend. Should be a json object with string keys and values.`,
+ },
+
  "remount": {
  "Move the mount point of an already-mounted backend, within or across namespaces",
  `

vault/logical_system_paths.go

@@ -1544,7 +1544,7 @@ func (b *SystemBackend) authPaths() []*framework.Path {
  },
  "user_lockout_config": {
  Type: framework.TypeMap,
- Description: strings.TrimSpace(sysHelp["user_lockout_config"][0]),
+ Description: strings.TrimSpace(sysHelp["tune_user_lockout_config"][0]),
  },
  "plugin_version": {
  Type: framework.TypeString,
@@ -1935,7 +1935,7 @@ func (b *SystemBackend) mountPaths() []*framework.Path {
  },
  "user_lockout_config": {
  Type: framework.TypeMap,
- Description: strings.TrimSpace(sysHelp["user_lockout_config"][0]),
+ Description: strings.TrimSpace(sysHelp["tune_user_lockout_config"][0]),
  },
  },
 

vault/mount.go

@@ -349,7 +349,7 @@ type MountConfig struct {
  AllowedResponseHeaders []string `json:"allowed_response_headers,omitempty" structs:"allowed_response_headers" mapstructure:"allowed_response_headers"`
  TokenType logical.TokenType `json:"token_type,omitempty" structs:"token_type" mapstructure:"token_type"`
  AllowedManagedKeys []string `json:"allowed_managed_keys,omitempty" mapstructure:"allowed_managed_keys"`
- UserLockoutConfig UserLockoutConfig  `json:"user_lockout_config,omitempty" mapstructure:"user_lockout_config"`
+ UserLockoutConfig *UserLockoutConfig `json:"user_lockout_config,omitempty" mapstructure:"user_lockout_config"`
 
  // PluginName is the name of the plugin registered in the catalog.
  //