From d36f508dffc4d6ed91f873e33252cfd65fa36c30 Mon Sep 17 00:00:00 2001 From: Christian Roessner Date: Thu, 14 Nov 2024 21:20:01 +0100 Subject: [PATCH 1/5] Fix: Refactor user account cache loading logic This commit simplifies the cache loading logic by ensuring the user account is updated in Redis before querying cache names. It introduces a check for Redis errors and ensures known user details are only processed once, improving efficiency and clarity. Signed-off-by: Christian Roessner --- server/core/cache.go | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/server/core/cache.go b/server/core/cache.go index b1e07ca8..9430d3fe 100644 --- a/server/core/cache.go +++ b/server/core/cache.go @@ -38,35 +38,43 @@ func cachePassDB(auth *AuthState) (passDBResult *PassDBResult, err error) { passDBResult = &PassDBResult{} - cacheNames := backend.GetCacheNames(auth.Protocol.Get(), global.CacheAll) + accountName, err = auth.updateUserAccountInRedis() + if err != nil { + return + } - for _, cacheName := range cacheNames.GetStringSlice() { - accountName, err = auth.updateUserAccountInRedis() - if err != nil { - return - } + if accountName != "" { + cacheNames := backend.GetCacheNames(auth.Protocol.Get(), global.CacheAll) - if accountName != "" { + for _, cacheName := range cacheNames.GetStringSlice() { redisPosUserKey := config.LoadableConfig.Server.Redis.Prefix + "ucp:" + cacheName + ":" + accountName ppc = &backend.PositivePasswordCache{} - if _, err = backend.LoadCacheFromRedis(auth.HTTPClientContext, redisPosUserKey, ppc); err != nil { + isRedisErr := false + + if isRedisErr, err = backend.LoadCacheFromRedis(auth.HTTPClientContext, redisPosUserKey, ppc); err != nil { return } - } - if ppc != nil { + // The user was not found for the current cache name + if isRedisErr { + continue + } + + passDBResult.UserFound = true + passDBResult.AccountField = ppc.AccountField + passDBResult.TOTPSecretField = ppc.TOTPSecretField + passDBResult.UniqueUserIDField = ppc.UniqueUserIDField + passDBResult.DisplayNameField = ppc.DisplayNameField + passDBResult.Backend = ppc.Backend + passDBResult.Attributes = ppc.Attributes + if auth.NoAuth || ppc.Password == util.GetHash(util.PreparePassword(auth.Password)) { - passDBResult.UserFound = true - passDBResult.AccountField = ppc.AccountField - passDBResult.TOTPSecretField = ppc.TOTPSecretField - passDBResult.UniqueUserIDField = ppc.UniqueUserIDField - passDBResult.DisplayNameField = ppc.DisplayNameField passDBResult.Authenticated = true - passDBResult.Backend = ppc.Backend - passDBResult.Attributes = ppc.Attributes } + + break } } @@ -75,7 +83,7 @@ func cachePassDB(auth *AuthState) (passDBResult *PassDBResult, err error) { auth.loadPasswordHistoryFromRedis(key) } - // Prevent password lookups for already known wrong passwords. + // Prevent password lookups for already known wrong passwords (And the user is unknown in the entire system) if auth.PasswordHistory != nil { passwordHash := util.GetHash(util.PreparePassword(auth.Password)) if _, foundPassword := (*auth.PasswordHistory)[passwordHash]; foundPassword { From ca5a322de9a7d055dbd1f943b7fc90afb46f3fa7 Mon Sep 17 00:00:00 2001 From: Christian Roessner Date: Thu, 14 Nov 2024 21:28:29 +0100 Subject: [PATCH 2/5] Fix: Remove redundant password history checks in cache loading Eliminated the unnecessary verification steps related to password history within the cache loading process. This streamlines the function and avoids redundant password lookups for already known wrong passwords. Signed-off-by: Christian Roessner --- server/core/cache.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/server/core/cache.go b/server/core/cache.go index 9430d3fe..ca8fdda8 100644 --- a/server/core/cache.go +++ b/server/core/cache.go @@ -52,7 +52,6 @@ func cachePassDB(auth *AuthState) (passDBResult *PassDBResult, err error) { ppc = &backend.PositivePasswordCache{} isRedisErr := false - if isRedisErr, err = backend.LoadCacheFromRedis(auth.HTTPClientContext, redisPosUserKey, ppc); err != nil { return } @@ -78,19 +77,5 @@ func cachePassDB(auth *AuthState) (passDBResult *PassDBResult, err error) { } } - if !passDBResult.Authenticated { - if key := auth.getPasswordHistoryRedisHashKey(true); key != "" { - auth.loadPasswordHistoryFromRedis(key) - } - - // Prevent password lookups for already known wrong passwords (And the user is unknown in the entire system) - if auth.PasswordHistory != nil { - passwordHash := util.GetHash(util.PreparePassword(auth.Password)) - if _, foundPassword := (*auth.PasswordHistory)[passwordHash]; foundPassword { - passDBResult.UserFound = true - } - } - } - return } From 427d1db7525982c2b89ba73cd09177e972cf4d21 Mon Sep 17 00:00:00 2001 From: Christian Roessner Date: Thu, 14 Nov 2024 21:45:14 +0100 Subject: [PATCH 3/5] Fix: Refactor authenticateUser to processPassDBResult Renamed the function and updated its behavior to better reflect its purpose. The new function no longer returns the passDBResult and has an updated inline comment for clarity. Removed redundant return statements in the process. Signed-off-by: Christian Roessner --- server/core/auth.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/server/core/auth.go b/server/core/auth.go index 5bbcf9ec..45f7b8d9 100644 --- a/server/core/auth.go +++ b/server/core/auth.go @@ -956,7 +956,7 @@ func (a *AuthState) verifyPassword(passDBs []*PassDBMap) (*PassDBResult, error) break } } else { - passDBResult, err = authenticateUser(passDBResult, a, passDB) + err = processPassDBResult(passDBResult, a, passDB) if err != nil || a.UserFound { break } @@ -1037,16 +1037,16 @@ func checkAllBackends(configErrors map[global.Backend]error, a *AuthState) (err return err } -// authenticateUser updates the passDBResult based on the provided passDB +// processPassDBResult updates the passDBResult based on the provided passDB // and the AuthState object a. // If passDBResult is nil, it returns an error of type errors.ErrNoPassDBResult. // It then calls the util.DebugModule function to log debug information. // Next, it calls the updateAuthentication function to update the fields of a based on the values in passDBResult. // If the UserFound field of passDBResult is true, it sets the UserFound field of a to true. // Finally, it returns the updated passDBResult and nil error. -func authenticateUser(passDBResult *PassDBResult, a *AuthState, passDB *PassDBMap) (*PassDBResult, error) { +func processPassDBResult(passDBResult *PassDBResult, a *AuthState, passDB *PassDBMap) error { if passDBResult == nil { - return passDBResult, errors.ErrNoPassDBResult + return errors.ErrNoPassDBResult } util.DebugModule( @@ -1057,20 +1057,20 @@ func authenticateUser(passDBResult *PassDBResult, a *AuthState, passDB *PassDBMa "passdb_result", fmt.Sprintf("%+v", *passDBResult), ) - passDBResult = updateAuthentication(a, passDBResult, passDB) + updateAuthentication(a, passDBResult, passDB) - if passDBResult.UserFound { - a.UserFound = true - } - - return passDBResult, nil + return nil } // updateAuthentication updates the fields of the AuthState struct with the values from the PassDBResult struct. // It checks if each field in passDBResult is not nil and if it is not nil, it updates the corresponding field in the AuthState struct. // It also updates the SourcePassDBBackend and UsedPassDBBackend fields of the AuthState struct with the values from passDBResult.Backend and passDB.backend respectively. // It returns the updated PassDBResult struct. -func updateAuthentication(a *AuthState, passDBResult *PassDBResult, passDB *PassDBMap) *PassDBResult { +func updateAuthentication(a *AuthState, passDBResult *PassDBResult, passDB *PassDBMap) { + if passDBResult.UserFound { + a.UserFound = true + } + if passDBResult.AccountField != nil { a.AccountField = passDBResult.AccountField } @@ -1093,8 +1093,6 @@ func updateAuthentication(a *AuthState, passDBResult *PassDBResult, passDB *Pass a.SourcePassDBBackend = passDBResult.Backend a.UsedPassDBBackend = passDB.backend - - return passDBResult } // setStatusCodes sets different status codes for various services. From 52a7b643a1500d229a5182d6b5f75feb11e25cdd Mon Sep 17 00:00:00 2001 From: Christian Roessner Date: Fri, 15 Nov 2024 10:08:12 +0100 Subject: [PATCH 4/5] Fix: Remove redundant AccountField check in auth logic The check for `AccountField` being nil was unnecessary as the code handles the assignment condition elsewhere. This simplifies the `auth.go` file and ensures the logic is more streamlined and maintains correctness. Signed-off-by: Christian Roessner --- server/core/auth.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/core/auth.go b/server/core/auth.go index 45f7b8d9..1f41a974 100644 --- a/server/core/auth.go +++ b/server/core/auth.go @@ -1582,10 +1582,6 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b */ if a.UserFound { - if passDBResult.AccountField != nil { - a.AccountField = passDBResult.AccountField - } - accountName, err = a.updateUserAccountInRedis() if err != nil { level.Error(log.Logger).Log(global.LogKeyGUID, a.GUID, global.LogKeyMsg, err.Error()) From d1ed22b410a6d1474884706358af8fa148212516 Mon Sep 17 00:00:00 2001 From: Christian Roessner Date: Fri, 15 Nov 2024 11:40:10 +0100 Subject: [PATCH 5/5] Fix: Fix user authentication storage issues Ensure authentication requests respect `NoAuth` setting and prevent storage of empty passwords in Redis cache. This update refines the process of handling user database queries and enhances security by avoiding redundant password history retrievals. Signed-off-by: Christian Roessner --- server/core/auth.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/server/core/auth.go b/server/core/auth.go index 1f41a974..8fd50616 100644 --- a/server/core/auth.go +++ b/server/core/auth.go @@ -1594,9 +1594,9 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b } } - if useCache { + // Note: User-DB queries never contain a password! + if !a.NoAuth && useCache { // Make sure the cache backend is in front of the used backend. - // If this is a userdb-request, the authentication state is forced to "true" (see verifyPassword()-moethod) if passDBResult.Authenticated { if accountName != "" { if backendPos[global.BackendCache] < backendPos[a.UsedPassDBBackend] { @@ -1640,7 +1640,10 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b Attributes: a.Attributes, } - go backend.SaveUserDataToRedis(a.HTTPClientContext, *a.GUID, redisUserKey, config.LoadableConfig.Server.Redis.PosCacheTTL, ppc) + // Safety net. Never store empty passwords into ppc. + if ppc.Password != "" { + go backend.SaveUserDataToRedis(a.HTTPClientContext, *a.GUID, redisUserKey, config.LoadableConfig.Server.Redis.PosCacheTTL, ppc) + } } } } else { @@ -1655,10 +1658,7 @@ func (a *AuthState) postVerificationProcesses(ctx *gin.Context, useCache bool, b a.saveFailedPasswordCounterInRedis() } - // Only passdb requests need reloading - if !a.NoAuth { - a.getAllPasswordHistories() - } + a.getAllPasswordHistories() } /*