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

[kenrowland]

Added cache for user attempts that are not authorized. Automatically fail repeated attempts.

Signed-Off-By: Kenneth Rowland [email protected]

Type of change:

• This change is a bug fix (non-breaking change which fixes an issue).
• This change is a new feature (non-breaking change which adds functionality).
• This change improves the code (refactor or other change that does not change the functionality)
• This change fixes warnings (the fix does not alter the functionality or the generated code)
• This change is a breaking change (fix or feature that will cause existing behavior to change).
• This change alters the query API (existing queries will have to be recompiled)

Checklist:

• My code follows the code style of this project.
  • My code does not create any new warnings from compiler, build system, or lint.
• The commit message is properly formatted and free of typos.
  • The commit message title makes sense in a changelog, by itself.
  • The commit is signed.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly, or...
  • I have created a JIRA ticket to update the documentation.
  • Any new interfaces or exported functions are appropriately commented.
• I have read the CONTRIBUTORS document.
• The change has been fully tested:
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have checked that this change does not introduce memory leaks.
  • I have used Valgrind or similar tools to check for potential issues.
• I have given due consideration to all of the following potential concerns:
  • Scalability
  • Performance
  • Security
  • Thread-safety
  • Cloud-compatibility
  • Premature optimization
  • Existing deployed queries will not be broken
  • This change fixes the problem, not just the symptom
  • The target branch of this pull request is appropriate for such a change.
• There are no similar instances of the same problem that should be addressed
  • I have addressed them here
  • I have raised JIRA issues to address them separately
• This is a user interface / front-end modification
  • I have tested my changes in multiple modern browsers
  • The component(s) render as expected

Smoketest:

• Send notifications about my Pull Request position in Smoketest queue.
• Test my draft Pull Request.

Testing:

[Copilot]

Pull Request Overview

This PR implements a cache to prevent repeated LDAP authentication attempts for users with invalid credentials. The cache stores username-password pairs for failed authentication attempts and returns early if the same credentials are retried, reducing unnecessary LDAP server load from brute force or repeated failed login attempts.

Key changes:

• Added an unauthorized user cache that stores failed username/password combinations
• Implemented automatic cache expiration every 5 minutes to prevent unbounded growth
• Modified authentication flow to check the cache before attempting LDAP authentication

[Copilot]

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

[Copilot]

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>

[kenrowland]

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

[Copilot]

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

[kenrowland]

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.

[Copilot]

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)

[kenrowland]

Probably overkill for the purposes of this change.

[Copilot]

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);

[kenrowland]

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.

[Copilot]

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.

[jakesmith]

I think the other routes to denial should also be considered, e.g. AS_PASSWORD_VALID_BUT_EXPIRED, AS_ACCOUNT_LOCKED, etc..

I think a better approach would be to add a temporary account lockout mechanism for any user that fails authentication (for most reasons) multiple times within a short space of time, and then they will be blocked for a defined period of time.
e.g. N requests per minute = blocked for 20 mins.

That would also avoid the caching of stale passwords - which although invalid are still sensitive.

[kenrowland]

@jakesmith

I opened https://hpccsystems.atlassian.net/browse/HPCC-35300 to generally handle successive attempts to brute force logging in with different passwords. In some ways it could handle the subject of this PR as well. However, I think handling separately is better since the rejection purpose would differ.

On the subject of this PR, I left AS_PASSWORD_VALID_BUT_EXPIRED and AS_ACCOUNT_LOCKED out for the following reasons:

• AS_PASSWORD_VALID_BUT_EXPIRED - an expired password needs to be changed. Presumably ESP or ECL Watch will detect and let the user change it. A subsequent login attempt with the new changed password should succeed, otherwise the user would have to wait the cache timeout period before logging in (this is also the reason to save the password)
• AS_ACCOUNT_LOCKED - If the AD admin removes the lock, the user should be able to login immediately w/o having to wait for the cache timeout.

We could hash the saved passwords, but since they are not valid and are stored in memory, I felt it a bit of overkill. Plus, each authenticate attempt would have to go through the hashing process.

[jakesmith]

Not convinced. A successive failed login attempt if expired, or locked in a short space of time, would still be a DoS type attack, so fall under the remit of HPCC-35300.
A wrong password is no different - and I agree with copilot that you really don't want to be caching these credentials even if stale - hashing is a way to avoid it, but better not to at all.

If the logic is fast failed login attempts lock account for a period, then it covers them all. It's a common approach in general, for wrong passwords to, it either requires an admin to reset, or the the timeout period to lapse. The user should see "temporary locked" or similar.
When we have that, it would be good to provide a reset feature for admins, but if we initially keep the timeout period relatively short it will push the problem to the users who are submitting stale credentials.

[jakesmith]

Btw, re. general DoS prevention, I put this prompt into github.com agent's panel yesterday for kicks:

in system/security/LdapSecurity/ldapconnection.cpp in the CLdapClient::authenticate implement a strategy which defends against repeated failed authentication attempts (that can act like a DoS attack).

It autnetication fails (!= LDAP_SUCCESS) it should consider it as a potential DoS attack. When N have occured within a relatively short space (time T seconds) amount of time, the account should be blocked for B seconds.

This is the result: jakesmith#156
Haven't looked at it carefully, but looks like a reasonable 1st stab.