HPCC-35300 LDAP should handle brute force password hack attempts by kenrowland · Pull Request #21172 · hpcc-systems/HPCC-Platform

kenrowland · 2026-04-02T19:49:05Z

Added cache of failed user authentication attempts with configurable number of allowed failed attempts and lockout timeout

Signed-Off-By: Kenneth Rowland kenneth.rowland@lexisnexisrisk.com

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:

Smoketest:

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

Testing:

github-actions · 2026-04-02T19:50:20Z

🔄 Upmerge Test Results

Status: ✅ All branches merged successfully
PR: #21172 - HPCC-35300 LDAP should handle brute force password hack attempts
Base Branch: candidate-10.2.x
Test Time: 2026-04-02 19:49:42 UTC

✅ Successful Branches (1)

master
This comment was automatically generated by the upmerge test workflow.

Added cache of failed user authentication attempts with configurable number of allowed failed attempts and lockout timeout Signed-Off-By: Kenneth Rowland kenneth.rowland@lexisnexisrisk.com

github-actions · 2026-04-02T20:35:54Z

🔄 Upmerge Test Results

Status: ✅ All branches merged successfully
PR: #21172 - HPCC-35300 LDAP should handle brute force password hack attempts
Base Branch: candidate-10.2.x
Test Time: 2026-04-02 20:35:35 UTC

✅ Successful Branches (1)

master
This comment was automatically generated by the upmerge test workflow.

asselitx · 2026-04-03T15:52:21Z

        user->setAuthenticateStatus(AS_AUTHENTICATED);
-
-    if (AS_AUTHENTICATED == user->getAuthenticateStatus())
+    if (isCaching)


question: Shouldn't this entire if (isCaching) still be inside a check to confirm if (AS_AUTHENTICATED == user->getAuthenticateStatus())? Otherwise, as it is now you could be adding a user to the cache if the call m_ldap_client->authenticate(*user) does not authenticate.

Looks like an artifact of some Copilot changes. Good find. Updated.

asselitx · 2026-04-03T19:02:51Z

+    // This design prevents permanent blocking and allows recovery if the underlying condition is resolved.
+    bool isExpired(const FailedAuthEntry &entry, unsigned currentTick) const
+    {
+        const unsigned elapsedMs = currentTick - entry.firstFailureTick; // unsigned wrap is intentional


This is intentional and OK because worst it would do is lockout someone for a bit longer than otherwise?

Updated comment

asselitx · 2026-04-03T19:39:31Z

+    {
+        if (m_cacheTimeoutSeconds >= (std::numeric_limits<unsigned>::max() / 1000U))
+            return std::numeric_limits<unsigned>::max();
+        return m_cacheTimeoutSeconds * 1000U;


This may be pedantic, but the setter in combination with this function does have a chance for a race condition to result in a value being set that would return something out of bounds. Maybe use the critical section or have some validation in the setter to keep it in bounds? This is the only thing I saw that probably really warrants a change.

Addressed with clamping the value.

asselitx · 2026-04-03T20:00:33Z

+                m_cache.begin(),
+                m_cache.end(),
+                [currentTick](const auto& a, const auto& b) {
+                    const unsigned ageA = currentTick - a.second.firstFailureTick;


This would be a possible simplification, but not a requested change. Since msTick is monotonically increasing (until wraparound) couldn't you just compare the firstFailureTick times to get a relative age ordering (smallest value meaning oldest, ignoring error due to wraparound which I can't think of a way to compensate for anyhow). The math with currentTick gives an absolute age, but you still would have error when the two operands are on either side of a wraparound.

Good question. We should keep subtraction here: unsigned tick subtraction is the correct wrap-safe pattern for monotonic msTick counters. Directly comparing firstFailureTick/currentTick would be less reliable across counter rollover.

github-actions · 2026-04-06T14:09:43Z

🔄 Upmerge Test Results

Status: ✅ All branches merged successfully
PR: #21172 - HPCC-35300 LDAP should handle brute force password hack attempts
Base Branch: candidate-10.2.x
Test Time: 2026-04-06 14:09:27 UTC

✅ Successful Branches (1)

master
This comment was automatically generated by the upmerge test workflow.

kenrowland · 2026-04-06T16:51:52Z

@ghalliday please merge

ghalliday

@kenrowland some questions about structure and efficiency. I'm not a fan of header only classes, but this is only used from 1 place .
Actually it isn't - thee are many #includes of ldapsecurity.ipp - including from some headers. It is worth minimizing the impact.

ghalliday · 2026-04-08T13:11:26Z

+                    unsigned maxAllowedEntries = defaultMaxAllowedEntries)
+        : m_maxFailedAttempts(maxFailedAttempts),
+            // Clamp once at construction so ms conversion cannot overflow.
+            m_cacheTimeoutSeconds(std::min(cacheTimeoutSeconds, std::numeric_limits<unsigned>::max() / 1000U)),


Similar comment to the other PR - use a much lower threshold than 49 days, otherwise it will never expire.

ghalliday · 2026-04-08T13:16:10Z

+            // Entry has expired; remove it and allow retries. No need to trim the
+            // full cache here — trimming happens on every insert/update in updateUserLockoutStatus.
+            m_cache.erase(it);
+            trimFailedAuthCache();


Why is this call necessary?

ghalliday · 2026-04-08T13:19:05Z

+        if (it != m_cache.end())
+        {
+            m_cache.erase(it);
+            trimFailedAuthCache();


why is this call here?

ghalliday · 2026-04-08T13:19:50Z

+    void setCacheTimeoutSeconds(unsigned v) { m_cacheTimeoutSeconds = std::min(v, std::numeric_limits<unsigned>::max() / 1000U); }
+    void setMaxAllowedEntries(unsigned v) { m_maxAllowedEntries = v; }
+
+    unsigned getMaxFailedAttempts() const { return m_maxFailedAttempts; }


Are these needed - they're not called.

ghalliday · 2026-04-08T13:22:06Z

+
+    void trimFailedAuthCache()
+    {
+        // Expect lock is held by caller.


This could be simplified

since it is called whenever an entry is added it can only have 1 extra entry - that can be checked by an assert.

When checking for expired entries, it could also keep a pointer to the oldest non-expired. That could then be removed without a subsequent re-iteration.

ghalliday · 2026-04-08T13:26:53Z

+        return false;
+    }
+
+    bool rc = doUserAuthenticate(user);


What happens if the authentication failed because the server was unavailable?

ghalliday · 2026-04-08T13:32:07Z

+
+    // Initialize/update the shared failed-auth cache with configured values
+    {
+        CriticalBlock block(CLdapSecManager::s_failedAuthCacheInitLock);


This is breaking the encapsulation. There should probably be a single set function that internally locks the critical section.

ghalliday · 2026-04-08T13:35:10Z

+    void setMaxFailedAttempts(unsigned v) { m_maxFailedAttempts = v; }
+    // Keep timeout-ms conversion safe even if set at runtime.
+    void setCacheTimeoutSeconds(unsigned v) { m_cacheTimeoutSeconds = std::min(v, std::numeric_limits<unsigned>::max() / 1000U); }
+    void setMaxAllowedEntries(unsigned v) { m_maxAllowedEntries = v; }


Theoretically this function and timeout should call trimFailedAuthCache(). This could break my assumption that there is at most one extra. I would resolve by clearing all.

HPCC-35300 LDAP should handle brute force password hack attempts

826d076

Added cache of failed user authentication attempts with configurable number of allowed failed attempts and lockout timeout Signed-Off-By: Kenneth Rowland kenneth.rowland@lexisnexisrisk.com

kenrowland force-pushed the HPCC-35300 branch from 62e724b to 826d076 Compare April 2, 2026 20:34

kenrowland requested a review from asselitx April 3, 2026 13:11

asselitx requested changes Apr 3, 2026

View reviewed changes

Addressed review comments and some cleanup

b2aa451

kenrowland requested a review from asselitx April 6, 2026 14:11

asselitx approved these changes Apr 6, 2026

View reviewed changes

ghalliday requested changes Apr 8, 2026

View reviewed changes

Conversation

kenrowland commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

github-actions Bot commented Apr 2, 2026

🔄 Upmerge Test Results

✅ Successful Branches (1)

Uh oh!

github-actions Bot commented Apr 2, 2026

🔄 Upmerge Test Results

✅ Successful Branches (1)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 6, 2026

🔄 Upmerge Test Results

✅ Successful Branches (1)

Uh oh!

kenrowland commented Apr 6, 2026

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kenrowland commented Apr 2, 2026 •

edited

Loading