From 8747ecaa3566b996b2f9613b356889161884f196 Mon Sep 17 00:00:00 2001 From: Christian Roessner Date: Mon, 25 Nov 2024 08:35:20 +0100 Subject: [PATCH 1/2] Fix: Rename blocked accounts key and related functions Renamed `RedisBlockedAccountsKey` to `RedisAffectedAccountsKey` to better reflect its purpose of storing affected user accounts. Updated all related function names and constants to ensure consistency with the new terminology. This change improves code clarity and alignment with our data taxonomy. Signed-off-by: Christian Roessner --- server/core/bruteforce.go | 8 ++++---- server/core/rest.go | 4 ++-- server/global/const.go | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/core/bruteforce.go b/server/core/bruteforce.go index 41787426..94ca9327 100644 --- a/server/core/bruteforce.go +++ b/server/core/bruteforce.go @@ -698,15 +698,15 @@ func (a *AuthState) processPWHist() (accountName string) { return } -// processBlockedAccount processes a blocked account by checking its existence in Redis and adding it if not present. +// updateAffectedAccount processes a blocked account by checking its existence in Redis and adding it if not present. // It increments Redis read and write counters and logs errors encountered during the operations. -func (a *AuthState) processBlockedAccount() { +func (a *AuthState) updateAffectedAccount() { accountName := a.refreshUserAccount() if accountName == "" { return } - key := config.LoadableConfig.Server.Redis.Prefix + global.RedisBlockedAccountsKey + key := config.LoadableConfig.Server.Redis.Prefix + global.RedisAffectedAccountsKey defer stats.RedisReadCounter.Inc() @@ -897,7 +897,7 @@ func (a *AuthState) processBruteForce(ruleTriggered, alreadyTriggered bool, rule a.BruteForceName = rule.Name - a.processBlockedAccount() + a.updateAffectedAccount() a.saveFailedPasswordCounterInRedis() a.getAllPasswordHistories() diff --git a/server/core/rest.go b/server/core/rest.go index 811f477a..dfac9acd 100644 --- a/server/core/rest.go +++ b/server/core/rest.go @@ -280,7 +280,7 @@ func listBlockedIPAddresses(ctx context.Context, filterCmd *FilterCmd, guid stri func listBlockedAccounts(ctx context.Context, filterCmd *FilterCmd, guid string) (*BlockedAccounts, error) { blockedAccounts := &BlockedAccounts{Accounts: make(map[string][]string)} - key := config.LoadableConfig.Server.Redis.Prefix + global.RedisBlockedAccountsKey + key := config.LoadableConfig.Server.Redis.Prefix + global.RedisAffectedAccountsKey defer stats.RedisReadCounter.Inc() @@ -512,7 +512,7 @@ func processUserCmd(ctx *gin.Context, userCmd *FlushUserCmd, guid string) (remov defer stats.RedisWriteCounter.Inc() // Remove account from BLOCKED_ACCOUNTS - key = config.LoadableConfig.Server.Redis.Prefix + global.RedisBlockedAccountsKey + key = config.LoadableConfig.Server.Redis.Prefix + global.RedisAffectedAccountsKey if err = rediscli.WriteHandle.SRem(ctx, key, accountName).Err(); err != nil { if !stderrors.Is(err, redis.Nil) { level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) diff --git a/server/global/const.go b/server/global/const.go index 15529d00..6d3738e1 100644 --- a/server/global/const.go +++ b/server/global/const.go @@ -470,8 +470,8 @@ const ( // RedisPWHistIPsKey represents the key used for storing password history associated with IPs in Redis. RedisPWHistIPsKey = "PW_HIST_IPS" - // RedisBlockedAccountsKey represents the key used to store blocked accounts in Redis. - RedisBlockedAccountsKey = "BLOCKED_ACCOUNTS" + // RedisAffectedAccountsKey represents the key used for storing affected user accounts in Redis. + RedisAffectedAccountsKey = "AFFECTED_ACCOUNTS" ) // ImageCopyright represents the copyright statement for a logo. From ade288ce64d30a397eb97088a6ffb4dcbe679d88 Mon Sep 17 00:00:00 2001 From: Christian Roessner Date: Mon, 25 Nov 2024 09:46:21 +0100 Subject: [PATCH 2/2] Fix: Refactor Redis error handling and improve key deletion logic Removed redundant error checks and imports. Enhanced the key deletion logic by checking the result of deletion operations and only appending keys that were actually deleted. This refactoring simplifies the code and improves readability. Signed-off-by: Christian Roessner --- server/core/register.go | 7 ------- server/core/rest.go | 46 ++++++++++++++++++----------------------- 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/server/core/register.go b/server/core/register.go index 39897021..3358901a 100644 --- a/server/core/register.go +++ b/server/core/register.go @@ -16,7 +16,6 @@ package core import ( - stderrors "errors" "fmt" "net/http" @@ -34,7 +33,6 @@ import ( "github.com/go-kit/log/level" "github.com/pquerna/otp" "github.com/pquerna/otp/totp" - "github.com/redis/go-redis/v9" "github.com/spf13/viper" "golang.org/x/text/cases" "golang.org/x/text/language" @@ -793,11 +791,6 @@ func registerTotpPOSTHandler(ctx *gin.Context) { if _, err = rediscli.WriteHandle.Del(ctx, userKey).Result(); err != nil { stats.RedisWriteCounter.Inc() - if stderrors.Is(err, redis.Nil) { - - continue - } - level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) break diff --git a/server/core/rest.go b/server/core/rest.go index dfac9acd..f9b2716d 100644 --- a/server/core/rest.go +++ b/server/core/rest.go @@ -467,6 +467,7 @@ func processFlushCache(ctx *gin.Context, userCmd *FlushUserCmd, guid string) (re // 5. Returns false. func processUserCmd(ctx *gin.Context, userCmd *FlushUserCmd, guid string) (removedKeys []string, noUserAccountFound bool) { var ( + result int64 removeHash bool accountName string ipAddresses []string @@ -499,28 +500,24 @@ func processUserCmd(ctx *gin.Context, userCmd *FlushUserCmd, guid string) (remov // Remove PW_HIST_SET from Redis key := getPWHistIPsRedisKey(accountName) - if err = rediscli.WriteHandle.Del(ctx, key).Err(); err != nil { - if !stderrors.Is(err, redis.Nil) { - level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) - } else { - err = nil - } + if result, err = rediscli.WriteHandle.Del(ctx, key).Result(); err != nil { + level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) } else { - removedKeys = append(removedKeys, key) + if result > 0 { + removedKeys = append(removedKeys, key) + } } defer stats.RedisWriteCounter.Inc() - // Remove account from BLOCKED_ACCOUNTS + // Remove an account from AFFECTED_ACCOUNTS key = config.LoadableConfig.Server.Redis.Prefix + global.RedisAffectedAccountsKey - if err = rediscli.WriteHandle.SRem(ctx, key, accountName).Err(); err != nil { - if !stderrors.Is(err, redis.Nil) { - level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) - } else { - err = nil - } + if result, err = rediscli.WriteHandle.SRem(ctx, key, accountName).Result(); err != nil { + level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) } else { - removedKeys = append(removedKeys, key) + if result > 0 { + removedKeys = append(removedKeys, key) + } } removedKeys = append(removedKeys, removeUserFromCache(ctx, userCmd, userKeys, guid, removeHash)...) @@ -591,7 +588,10 @@ func prepareRedisUserKeys(ctx context.Context, guid string, accountName string) // If any error occurs during the removal process, it logs the error and immediately returns. // After successful removal, it logs the keys that have been flushed. func removeUserFromCache(ctx context.Context, userCmd *FlushUserCmd, userKeys config.StringSet, guid string, removeHash bool) []string { - var err error + var ( + result int64 + err error + ) removedKeys := make([]string, 0) @@ -612,21 +612,15 @@ func removeUserFromCache(ctx context.Context, userCmd *FlushUserCmd, userKeys co } for _, userKey := range userKeys.GetStringSlice() { - if err = rediscli.WriteHandle.Del(ctx, userKey).Err(); err != nil { - if !stderrors.Is(err, redis.Nil) { - stats.RedisWriteCounter.Inc() - - level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) + if result, err = rediscli.WriteHandle.Del(ctx, userKey).Result(); err != nil { + level.Error(log.Logger).Log(global.LogKeyGUID, guid, global.LogKeyMsg, err) - return removedKeys - } + return removedKeys } stats.RedisWriteCounter.Inc() - if err != nil { - level.Warn(log.Logger).Log(global.LogKeyGUID, guid, "keys", userKey, "status", "not found") - } else { + if result > 0 { removedKeys = append(removedKeys, userKey) level.Info(log.Logger).Log(global.LogKeyGUID, guid, "keys", userKey, "status", "flushed")