Skip to content

HPCC-35282 Cache failed user authentication attempts to reduce AD overhead #20602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kenrowland wants to merge 1 commit into from
Closed

HPCC-35282 Cache failed user authentication attempts to reduce AD overhead #20602

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions system/security/LdapSecurity/ldapconnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include <map>
#include <string>
#include <set>
#include <openssl/evp.h>

Copilot AI Nov 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OpenSSL EVP header is included but not used anywhere in the code. This import should be removed unless there are plans to hash passwords before caching them.

Suggested change
#include <openssl/evp.h>

Copilot uses AI. Check for mistakes.

@kenrowland kenrowland Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was considering strong hashing of invalid passwords. Currently not doing that. Will remove if we decide plaintext is sufficient.


#ifdef _WIN32
#include <lm.h>
Expand Down Expand Up @@ -1603,6 +1604,11 @@ static __int64 getMaxPwdAge(Owned<ILdapConnectionPool> _conns, const char * _bas
}

static CriticalSection lcCrit;
static CriticalSection uaCrit;
std::map<std::string, std::string> unauthorizedUserCache;
Comment thread
kenrowland marked this conversation as resolved.

Copilot AI Nov 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing plaintext passwords in the cache poses a significant security risk. If the cache is exposed through a memory dump or debugging tool, passwords would be compromised. Consider hashing passwords with a fast cryptographic hash (e.g., SHA-256 using OpenSSL EVP functions) before storing them in the cache. The codebase already has examples of EVP_sha256() usage in system/security/cryptohelper/digisign.cpp.

Suggested change
std::map<std::string, std::string> unauthorizedUserCache;
// Store SHA-256 hashes of passwords instead of plaintext

Copilot uses AI. Check for mistakes.

@kenrowland kenrowland Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently not storing strong invalid passwords. We only store the most recent invalid password, the cache is cleared every 5 minutes, and the purpose is to quickly fail repeated invalid credentials.

time_t lastClearedUnauthorizedUserCache = 0;
constexpr int UNAUTHORIZED_CACHE_TTL_SECONDS = 300;

class CLdapClient : implements ILdapClient, public CInterface
{
private:
Expand Down Expand Up @@ -1778,6 +1784,38 @@ class CLdapClient : implements ILdapClient, public CInterface
return false;
}

//
// Check for duplicate attempts to authorize the same unauthorized user. If the user is found
// in the cache, check the password. If unchanged, exit early with a fail. If the password
// has changed, remove the entry from the cache and continue. Entries only added when there
// is an authentication failure with the password. Other failure not logged so that an AD
// change can make the user valid.
{
CriticalBlock block(uaCrit);
time_t currentTime = time(nullptr);

//
// reset every 5 minutes to ensure cache does not grow unbounded
if (currentTime - lastClearedUnauthorizedUserCache > UNAUTHORIZED_CACHE_TTL_SECONDS)
{
lastClearedUnauthorizedUserCache = currentTime;
unauthorizedUserCache.clear();
}
else
{
auto it = unauthorizedUserCache.find(username);
if (it != unauthorizedUserCache.end())
{
if (it->second == std::string(password))

Copilot AI Nov 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String comparison for passwords is not constant-time and may be vulnerable to timing attacks. While this is a cache check rather than the primary authentication, consider using a constant-time comparison function to avoid potential information leakage.

Suggested change
if (it->second == std::string(password))
const std::string &cachedPassword = it->second;
const std::string currentPassword(password);
if (cachedPassword.length() == currentPassword.length() &&
CRYPTO_memcmp(cachedPassword.data(), currentPassword.data(), cachedPassword.length()) == 0)

Copilot uses AI. Check for mistakes.

@kenrowland kenrowland Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably overkill for the purposes of this change.

{
user.setAuthenticateStatus(AS_INVALID_CREDENTIALS);
return false;
}
unauthorizedUserCache.erase(it); // different password, try authenticating
}
}
}

if (getMaxPwdAge(m_connections,(char*)m_ldapconfig->getBasedn(), m_ldapconfig->getLdapTimeout()) != PWD_NEVER_EXPIRES)
m_domainPwdsNeverExpire = false;
else
Expand Down Expand Up @@ -2015,6 +2053,7 @@ class CLdapClient : implements ILdapClient, public CInterface
}
else
{
addUserToUnauthenticatedCache(username, password);

Copilot AI Nov 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding expired passwords to the unauthenticated cache at line 2068 may not be appropriate. Password expiration is a different failure mode from invalid credentials. A user with an expired password has valid credentials but needs to change their password. Caching this prevents them from authenticating after changing their password until the cache expires (up to 5 minutes). Consider only caching AS_INVALID_CREDENTIALS failures.

Copilot uses AI. Check for mistakes.
DBGLOG("LDAP: Authentication(1) (%c) for user %s failed - %s", isWorkunitDAToken(password) ? 't' :'f', username, ldap_err2string(rc));
user.setAuthenticateStatus(AS_INVALID_CREDENTIALS);
}
Expand All @@ -2026,17 +2065,20 @@ class CLdapClient : implements ILdapClient, public CInterface
if (user.getPasswordDaysRemaining() == scPasswordExpired)
{
DBGLOG("LDAP: Password Expired(2) for user %s", username);
addUserToUnauthenticatedCache(username, password);
Comment thread
kenrowland marked this conversation as resolved.
user.setAuthenticateStatus(AS_PASSWORD_EXPIRED);
}
else
{
DBGLOG("LDAP: Authentication(2) for user %s failed - %s", username, ldap_err2string(rc));
addUserToUnauthenticatedCache(username, password);
user.setAuthenticateStatus(AS_INVALID_CREDENTIALS);
}
}
return false;
}
user.setAuthenticateStatus(AS_AUTHENTICATED);
removeUserFromUnauthenticatedCache(username); // remove user from cache since valid credentials provided
}
//Always retrieve user info(SID, UID, fullname, etc) for Active Directory, when the user first logs in.
if((m_ldapconfig->getServerType() == ACTIVE_DIRECTORY) && (m_pp != NULL))
Expand All @@ -2045,6 +2087,21 @@ class CLdapClient : implements ILdapClient, public CInterface
return true;
};

void addUserToUnauthenticatedCache(const char *username, const char *password)
{
CriticalBlock block(uaCrit);
unauthorizedUserCache.emplace(username, password);
Comment thread
kenrowland marked this conversation as resolved.

Copilot AI Nov 4, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using emplace() here doesn't update existing entries if the username already exists. If a user fails authentication with password A, then immediately tries password B (which also fails), the cache will retain password A. Use operator[] or insert_or_assign() to ensure the cache stores the most recent failed password attempt.

Suggested change
unauthorizedUserCache.emplace(username, password);
unauthorizedUserCache.insert_or_assign(username, password);

Copilot uses AI. Check for mistakes.

@kenrowland kenrowland Nov 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a user that previously failed authorization makes another attempt with a different password, the cached entry is removed and normal processing is done. If the new password is not valid, a new cache entry is created. so, no need to handle changing the password.

}

void removeUserFromUnauthenticatedCache(const char *username)
{
CriticalBlock block(uaCrit);
auto it = unauthorizedUserCache.find(username);
if (it != unauthorizedUserCache.end())
unauthorizedUserCache.erase(it);
}


virtual bool authorize(SecResourceType rtype, ISecUser& user, IArrayOf<ISecResource>& resources, const char * resName = nullptr)
{
bool ok = false;
Expand Down
Loading