Bound concurrent password-hash verifications and cache repeated failed credential checks

[pgtgrly]

Description

Adds bounded concurrency and caching around password-hash verification in the internal authentication backend.

• Category: Enhancement

• Why these changes are required?

  The internal authentication backend performs a full password-hash verification (BCrypt) for every credential check, on the request-handling thread. BCrypt is intentionally expensive, so under high authentication volume these verifications can consume a large share of CPU and add latency to concurrent requests. There is currently no bound on how many run at once, and repeated identical failing credentials re-run the full verification every time.

• What is the old behavior before changes and new behavior after changes?

  Old behavior: every credential check runs a full hash verification with unbounded concurrency; repeated identical failures each pay the full cost.

  New behavior: three additions, all opt-out-able via settings and with conservative defaults:

  1. Bounded concurrent hash verifications — a semaphore limits how many verifications run at once (default max(1, availableProcessors / 4)). When the limit is reached, the request receives 503 SERVICE_UNAVAILABLE, signalled internally by a new AuthBackendThrottledException. These responses are deliberately not reported to auth_failure_listeners, so transient load does not affect the IP/username rate limiters. Only the internal backend is limited; LDAP, SAML, Kerberos, JWT and PKI paths are untouched.

  2. Short-lived cache of failed credential checks — repeated identical failing credentials are answered from cache without re-running the verification. Only definitive credential failures populate it, signalled by a new typed InvalidCredentialsException. Transient errors (e.g. "backend not configured" during startup or a config reload) continue to throw the generic OpenSearchSecurityException and are never cached. The cache is cleared alongside the other auth caches on config reload.

  3. Optional AuthenticationBackend#userExists() fast-path — a default method (returns Optional.empty(), so existing custom backends are unaffected) letting a backend report whether a user is known, so requests for unknown users can be short-circuited before the hash verification runs.

  New optional settings, all with safe defaults:

  Setting	Default
  plugins.security.auth.max_concurrent_bcrypt	max(1, availableProcessors / 4) (0 disables the limit)
  plugins.security.cache.incorrect_credential_ttl_minutes	10
  plugins.security.cache.incorrect_credential_max_size	10000

Issues Resolved

None — this is a standalone improvement to the authentication path and is not tied to an existing issue.

Is this a backport? No. Requesting a backport 3.7 label so this also lands on the 3.7 branch.

Does this introduce new permission(s) to be displayed in the static dropdown on the front-end? No.

Testing

• Unit tests (added, all passing):

  • AuthBackendThrottledExceptionTest (5 tests) — message preservation, that it is an unchecked RuntimeException, that the stack trace is suppressed (fillInStackTrace returns this, matching Netty's StacklessClosedChannelException pattern), and — importantly — that it does not extend OpenSearchSecurityException, so it is not absorbed by the generic auth-failure handling and can propagate to the 503 handler.
  • InvalidCredentialsExceptionTest (3 tests) — message preservation, that it remains assignable to OpenSearchSecurityException so existing callers are unchanged, and that it retains a stack trace for diagnostics.
  • InternalAuthBackendTests (+3 tests, 7 total) — userExists() returning true/false, and returning Optional.empty() when the InternalUsersModel is transiently null during startup/reload (must not throw).

• Manual testing: built the plugin against 3.7.0 and ran it in a single-node Docker cluster with the limit forced to 1 permit. Verified that: existing users authenticate normally in steady state; concurrent cold-cache logins receive 503 rather than 401 when the limit is reached; and those 503s do not increment the IP rate limiter (a client on the same source IP continued to authenticate successfully throughout).

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 5364bb7.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/auth/BackendRegistry.java	453	medium	The userExists() pre-check short-circuits failure-listener notification before BCrypt runs for non-existent users. Combined with incorrectCredentialCache, subsequent requests for the same non-existent user skip BCrypt (fast rejection), while first attempts hit BCrypt (slow). This creates a timing oracle enabling username enumeration after one probe request.
src/main/java/org/opensearch/security/auth/BackendRegistry.java	884	medium	The incorrectCredentialCache is keyed by AuthCredentials. If AuthCredentials.equals()/hashCode() uses only username (not password), a single failed attempt permanently caches any username for TTL_MINUTES (default 10 min), blocking all subsequent login attempts for that user regardless of password — an account-lockout DoS. Requires verification of AuthCredentials equality semantics.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 5364bb7)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns User enumeration timing side channel: The new userExists() fast-path returns quickly for non-existent users (skipping BCrypt) while existing users incur full BCrypt cost. An attacker measuring response times can enumerate valid usernames — a regression from the previous constant-time-ish behavior where every credential check ran BCrypt regardless. Additionally, invoking authFailureListener.onAuthFailure before authenticate() runs means a probing attacker triggers IP-based rate limiting purely by guessing usernames, which could be abused to lock out legitimate users sharing a NAT.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Semaphore permit leak on cache hit for concurrent callers The BCrypt semaphore is acquired inside the Guava cache.get(ac, Callable) loading function. Guava serializes concurrent loaders for the same key, so only the winning thread runs the Callable and acquires/releases the permit. However, if authBackend.authenticate() succeeds and the User is cached, subsequent identical requests hit the earlier cache.getIfPresent(ac) fast-path and skip the semaphore entirely — this is intended. The concern is the failure path: when authenticate() throws InvalidCredentialsException, the value is not stored in the success cache, but is stored in incorrectCredentialCache. The incorrectCredentialCache check runs before cache.getIfPresent, so repeated failed attempts correctly skip the semaphore. This looks correct — but note that cache.get(...) for a failed load will not populate the success cache, meaning concurrent identical failing calls that arrive before the incorrect-credential cache is populated will all enter the Callable serially (Guava dedups), and each will acquire/release the permit once. This is fine, but worth verifying that Guava's loading behavior does not hold the permit across the waiting queue. try { return cache.get(ac, new Callable<User>() { @Override public User call() throws Exception { if (log.isTraceEnabled()) { log.trace( "Credentials for user {} not cached, return from {} backend directly", ac.getUsername(), authBackend.getType() ); } final Semaphore semaphore = bcryptSemaphore; final boolean shouldAcquire = (semaphore != null && authBackend instanceof InternalAuthenticationBackend); if (shouldAcquire && !semaphore.tryAcquire()) { log.warn("BCrypt concurrency limit reached, rejecting auth for user {}", ac.getUsername()); throw new AuthBackendThrottledException("BCrypt concurrency limit reached for user " + ac.getUsername()); } try { // Narrow catch: only wrap authenticate(). authz() exceptions // (e.g. "role not found") must NOT poison the incorrect-credential cache. final User authenticatedUser; try { authenticatedUser = authBackend.authenticate(context); } catch (InvalidCredentialsException e) { // Definitive credential failure (wrong user / wrong password / // empty password) thrown only by InternalAuthenticationBackend. // Transient errors (e.g. "not configured" during startup/reload) // keep throwing plain OpenSearchSecurityException and are NOT cached. incorrectCredentialCache.put(ac, Boolean.TRUE); throw e; } return authz(context, authenticatedUser, roleCache, authorizers); } finally { if (shouldAcquire) { semaphore.release(); } } } }); Early-notify runs userExists() for every auth domain authDomain.getBackend().userExists(ac.getUsername()) is called for every auth domain in the loop, including non-internal backends (LDAP, SAML, etc.) that return Optional.empty(). For internal backend, if userExists returns false, the IP rate limiter is incremented BEFORE the caller has confirmed via authenticate() that the user is truly absent. If the InternalUsersModel is transiently stale (e.g. right after user creation), a legitimate user could get their IP penalized by rate limiters. Additionally, this exposes a subtle user-enumeration side channel: response timing between "user absent" (fast, no BCrypt) and "user present with wrong password" (slow BCrypt) becomes measurable — the very thing the comment on line 183 in InternalAuthBackendTests claims BCrypt was preventing. Consider whether the timing tradeoff and pre-emptive IP penalty are acceptable. boolean earlyFailureNotified = false; if (ac != null) { Optional<Boolean> userExistsResult = authDomain.getBackend().userExists(ac.getUsername()); if (userExistsResult.isPresent() && !userExistsResult.get()) { if (isDebugEnabled) { log.debug( "User {} does not exist in backend {}, notifying failure listeners early", ac.getUsername(), authDomain.getBackend().getType() ); } for (AuthFailureListener authFailureListener : this.authBackendFailureListeners.get( authDomain.getBackend().getClass().getName() )) { authFailureListener.onAuthFailure( request.getRemoteAddress().map(InetSocketAddress::getAddress).orElse(null), ac, request ); } earlyFailureNotified = true; } } Throttled request skips remaining auth logic incorrectly When authenticationThrottled is set, the code continues to try the next auth domain, but if a later domain also fails (not throttled, just wrong credentials), the loop exits with authenticatedUser == null and the post-loop code sends a 503. This overrides what would otherwise be a legitimate 401 for the actually-attempted domains. A user with correct LDAP credentials that fail elsewhere would receive 503 instead of the correct auth result when the internal backend is saturated. Consider only returning 503 when no other domain successfully authenticated AND no domain returned a definitive credential failure. if (authenticationThrottled) { log.warn( "Authentication throttled due to BCrypt concurrency limit for {} from {}", authCredentials == null ? null : authCredentials.getUsername(), remoteAddress ); request.queueForSending( new SecurityResponse(SC_SERVICE_UNAVAILABLE, "Authentication service temporarily unavailable, please retry later") ); return false; } Incorrect-credential cache key uses AuthCredentials equality The cache is keyed by AuthCredentials (which includes the password). If AuthCredentials.equals/hashCode include mutable state (e.g. attributes populated during authentication) or if the password byte array is cleared/zeroed after use (as done in InternalAuthenticationBackend.authenticate via Arrays.fill(wrap.array(), (byte) 0)), subsequent lookups may not match, defeating the cache — or worse, the zeroed key could collide with other zeroed-password entries. Verify that AuthCredentials equality is stable across the caching lifecycle and that password bytes are not mutated after put. if (authBackend instanceof InternalAuthenticationBackend && incorrectCredentialCache.getIfPresent(ac) != null) { if (log.isDebugEnabled()) { log.debug("Credentials for user {} found in incorrect-credential cache, rejecting", ac.getUsername()); } return null; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5364bb7
Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Avoid IP rate-limiter bypass on cached failures Returning null here skips notifying the AuthFailureListener (IP rate limiter), which means an attacker can hammer the same wrong credential repeatedly without any rate-limit penalty since each hit short-circuits before the failure notification loop. Ensure the caller's failure-listener path still fires for cached negative hits, otherwise the incorrect-credential cache becomes a DoS bypass for the IP blocker. src/main/java/org/opensearch/security/auth/BackendRegistry.java [879-884] if (authBackend instanceof InternalAuthenticationBackend && incorrectCredentialCache.getIfPresent(ac) != null) { if (log.isDebugEnabled()) { log.debug("Credentials for user {} found in incorrect-credential cache, rejecting", ac.getUsername()); } - return null; + // Fall through as a normal failure so AuthFailureListener still increments the IP counter. + throw new InvalidCredentialsException("cached invalid credentials for " + ac.getUsername()); } Suggestion importance[1-10]: 7 __ Why: Valid security concern: returning null on cached invalid credentials skips the AuthFailureListener path, potentially bypassing the IP rate limiter. This could weaken the DoS protection this PR aims to add.	Medium
	Prevent negative-cache flooding attack Caching by AuthCredentials as-is may allow cache pollution: if AuthCredentials.equals/hashCode includes the (wrong) password, an attacker can flood the cache with 10,000 distinct wrong passwords for a valid user, evicting legitimate entries and disabling the negative-cache protection. Verify the key derivation and consider bounding entries per-username or using a username-based key with a small failure counter instead. src/main/java/org/opensearch/security/auth/BackendRegistry.java [922-929] } catch (InvalidCredentialsException e) { - // Definitive credential failure (wrong user / wrong password / - // empty password) thrown only by InternalAuthenticationBackend. - // Transient errors (e.g. "not configured" during startup/reload) - // keep throwing plain OpenSearchSecurityException and are NOT cached. + // Consider using a per-username key to prevent password-space cache flooding. incorrectCredentialCache.put(ac, Boolean.TRUE); throw e; } Suggestion importance[1-10]: 6 __ Why: Legitimate concern about cache-key design: if AuthCredentials includes password in equals/hashCode, attackers could evict legitimate entries. The suggestion asks to verify, but the concern is meaningful for the new negative cache.	Low
General	Verify throttle precedence vs anonymous auth The 503 is returned when authenticatedUser remains null after all domains, but authenticationThrottled is checked before the anonymous-auth branch. If anonymous auth is enabled and this request qualifies, a throttled BCrypt attempt will now return 503 instead of falling through to anonymous access. Move the throttle-503 block to only fire when no other resolution (anonymous or later domain) applies, or ensure this is the intended precedence. src/main/java/org/opensearch/security/auth/BackendRegistry.java [492-497] authenticationThrottled = true; -// continue (not break): subsequent auth domains (LDAP, SAML, etc.) -// do not run BCrypt and may still authenticate this user. The -// post-loop 503 handler only fires when authenticatedUser remains -// null, so a successful later domain harmlessly clears the throttle. +// NOTE: post-loop 503 short-circuits before anonymous-auth fallback; +// verify this precedence is intended, or move the check after anonymous handling. continue; Suggestion importance[1-10]: 5 __ Why: Reasonable observation that the 503 short-circuit runs before anonymous auth fallback, potentially breaking anonymous access under throttling. Worth verifying precedence, though only asks for verification.	Low
	Remove redundant cache pre-check The pre-check cache.getIfPresent(ac) bypasses the authorization step (authz) that is applied when populating the cache via cache.get(...). However, since the cache stores the post-authz User, this is fine — but the concern is that this pre-check duplicates what cache.get(ac, Callable) already does internally (it returns cached value without invoking the Callable). This extra getIfPresent call is redundant and only saves the Callable object allocation. Consider removing it to reduce code complexity, or document why it's needed. src/main/java/org/opensearch/security/auth/BackendRegistry.java [887-890] -// Success cache hit — return without acquiring semaphore -User cachedUser = cache.getIfPresent(ac); -if (cachedUser != null) { - return cachedUser; -} +// cache.get() below returns cached value without invoking Callable on hit; +// no separate getIfPresent() needed. Suggestion importance[1-10]: 3 __ Why: The pre-check does avoid Callable allocation and is a minor optimization. Removing it would slightly simplify code but is not impactful; the suggestion is largely stylistic.	Low

---

Previous suggestions

Suggestions up to commit 8d1f01c

Category	Suggestion	Impact
Security	Avoid username-enumeration timing side channel Calling userExists() before authenticate() and short-circuiting failure notifications creates a timing side channel that lets attackers enumerate valid usernames: nonexistent users trigger an immediate failure listener call and skip BCrypt, whereas existing users incur BCrypt latency. This defeats the constant-time anti-enumeration property that the original code preserved. Consider removing the pre-check or applying it only after credential verification. src/main/java/org/opensearch/security/auth/BackendRegistry.java [458-462] -// Pre-increment IP rate limiter for non-existent users before BCrypt runs +// NOTE: skip pre-check to preserve constant-time behavior and avoid +// username enumeration via timing / rate-limiter side channels. boolean earlyFailureNotified = false; -if (ac != null) { - Optional<Boolean> userExistsResult = authDomain.getBackend().userExists(ac.getUsername()); - if (userExistsResult.isPresent() && !userExistsResult.get()) { Suggestion importance[1-10]: 8 __ Why: This is a valid and important security concern: the pre-check userExists() introduces a timing/behavior side channel that enables username enumeration, undermining constant-time authentication properties.	Medium
	Avoid caching plaintext credentials in memory Populating incorrectCredentialCache with the raw AuthCredentials (which holds the plaintext password bytes) keeps sensitive credentials pinned in heap memory for the entire TTL (default 10 minutes), and also relies on AuthCredentials.equals/hashCode — if these compute over mutable/cleared password state the cache lookup on retry may not hit. Consider caching a derived key (e.g. username + salted hash of the password) instead of the full credentials object, so plaintext isn't retained. src/main/java/org/opensearch/security/auth/BackendRegistry.java [932] try { authenticatedUser = authBackend.authenticate(context); } catch (InvalidCredentialsException e) { - // Definitive credential failure (wrong user / wrong password / - // empty password) thrown only by InternalAuthenticationBackend. - // Transient errors (e.g. "not configured" during startup/reload) - // keep throwing plain OpenSearchSecurityException and are NOT cached. + // Cache a derived key rather than raw credentials to avoid + // pinning plaintext password bytes in memory for the TTL. incorrectCredentialCache.put(ac, Boolean.TRUE); throw e; } Suggestion importance[1-10]: 6 __ Why: Valid concern about pinning plaintext password bytes in the cache for the TTL duration, but the improved_code is identical to the existing_code so it does not actually demonstrate the fix. Still, the security consideration is worth raising.	Low
	Ensure cached failures still penalize IP Returning null here bypasses the AuthFailureListener notification loop in the caller only when earlyFailureNotified is false — but the IP rate limiter should still be incremented on cached failure to prevent attackers from cheaply retrying known-bad passwords without cost. Verify that the caller's post-authcz==null failure listener path still runs (it does, since earlyFailureNotified guards it), otherwise this becomes a rate-limit bypass. src/main/java/org/opensearch/security/auth/BackendRegistry.java [884-889] if (authBackend instanceof InternalAuthenticationBackend && incorrectCredentialCache.getIfPresent(ac) != null) { if (log.isDebugEnabled()) { log.debug("Credentials for user {} found in incorrect-credential cache, rejecting", ac.getUsername()); } + // fall through to caller so AuthFailureListener is invoked and IP is penalized return null; } Suggestion importance[1-10]: 3 __ Why: The suggestion mainly asks to verify caller behavior and acknowledges the current flow likely works correctly; it does not propose a concrete change. Low impact.	Low
General	Use bounded wait instead of immediate reject A non-blocking tryAcquire() causes immediate 503 rejection under any brief spike, even when a permit would free up in milliseconds. Consider using tryAcquire(timeout, unit) with a small bounded wait (e.g. configurable, defaulting to a few hundred ms) so legitimate traffic queues briefly rather than being rejected outright, while still bounding worst-case latency. src/main/java/org/opensearch/security/auth/BackendRegistry.java [915-920] final Semaphore semaphore = bcryptSemaphore; final boolean shouldAcquire = (semaphore != null && authBackend instanceof InternalAuthenticationBackend); -if (shouldAcquire && !semaphore.tryAcquire()) { +if (shouldAcquire && !semaphore.tryAcquire(bcryptAcquireTimeoutMs, TimeUnit.MILLISECONDS)) { log.warn("BCrypt concurrency limit reached, rejecting auth for user {}", ac.getUsername()); throw new AuthBackendThrottledException("BCrypt concurrency limit reached for user " + ac.getUsername()); } Suggestion importance[1-10]: 5 __ Why: Reasonable enhancement to smooth over brief spikes, but the current design is intentional (fast-fail with 503) and this is a debatable trade-off rather than a defect.	Low

[codecov]

Codecov Report

❌ Patch coverage is 62.62626% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.76%. Comparing base (22c36ef) to head (5364bb7).

Files with missing lines	Patch %	Lines
.../org/opensearch/security/auth/BackendRegistry.java	58.13%	25 Missing and 11 partials ⚠️
...y/auth/internal/InternalAuthenticationBackend.java	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6393      +/-   ##
==========================================
- Coverage   75.78%   75.76%   -0.03%     
==========================================
  Files         457      460       +3     
  Lines       30556    30638      +82     
  Branches     4630     4650      +20     
==========================================
+ Hits        23158    23213      +55     
- Misses       5274     5293      +19     
- Partials     2124     2132       +8     

Files with missing lines	Coverage Δ
...h/security/auth/AuthBackendThrottledException.java	100.00% <100.00%> (ø)
...pensearch/security/auth/AuthenticationBackend.java	100.00% <100.00%> (ø)
...rch/security/auth/InvalidCredentialsException.java	100.00% <100.00%> (ø)
...g/opensearch/security/support/ConfigConstants.java	96.55% <ø> (ø)
...y/auth/internal/InternalAuthenticationBackend.java	79.41% <85.71%> (+1.28%)	⬆️
.../org/opensearch/security/auth/BackendRegistry.java	75.86% <58.13%> (-4.03%)	⬇️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[devardee]

method description say's : no auditlog, throw no exception, does also authz for all authorizers . but we are now throwing exception's.

[devardee]

this whole logic is scoped to Basic Auth alone, can we have this logic inside of InternalAuthenticationBackend.authenticate() method ?

[devardee]

Can this lead to timing attacks ?

[devardee]

Instead of 503, we should return 429 to the caller (https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/429)

[devardee]

move this comment to this if block : if (authCredentials == null && anonymousAuthEnabled && isRequestForAnonymousLogin(request.params(), request.getHeaders())) {

[kkhatua]

@pgtgrly
A couple of minor comments:

1. max(1, availableProcessors / 4) --> Why did we settle for this? Could you share some perf numbers around this?
2. If the cache's purpose is to block recomputation of already failed/invalid creds, wouldn't an attacker essentially keep trying different permutations?
3. Why are we holding the password in plaintext? Isnt the hash sufficient to confirm if the retry is a match? It is possible that a typo in the username and a valid password might be stored, and a heapdump would reveal this.

[kkhatua]

One interesting thing is that the speed of response of a system could reveal to an attacker of the existence of a valid username, given that retries with the cache will respond faster.
nit: This might feel like a bit of an overkill, but would it make sense to inject a delay similar to the time for a valid authentication?

[cwperks]

One interesting thing is that the speed of response of a system could reveal to an attacker of the existence of a valid username, given that retries with the cache will respond faster. nit: This might feel like a bit of an overkill, but would it make sense to inject a delay similar to the time for a valid authentication?

We've actually previously created an advisory on that: GHSA-c6wg-cm5x-rqvj

We should have some test cases around this so let's see if they give assurance or need to be updated.

[cwperks]

@pgtgrly Can you please fix the conflicts? Apologies for not being able to review this in the past 2 weeks.

[github-actions]

Persistent review updated to latest commit 5364bb7

[cwperks]

i think this is fine, but I'd really like to see enhancements to auth_failure_listeners which provides similar functionality.

Some of the potential enhancements:

• Block at the cluster-level when any node has reached the threshold for blocking.
• Provide APIs for cluster admins to view if any blocks exist and take admin action to unblock
• UX for configuring auth_failure_listeners to help more people discover the feature

I think this cache will have similar problem as auth_failure_listeners where it only has node-level visibility.

[cwperks]

Why name these bcrypt specifically? Password hashing algorithm is configurable and bcrypt is only the default.