Limit concurrent bcrypt authentications to bound CPU under auth floods by groeneai · Pull Request #108527 · ClickHouse/ClickHouse · GitHub
Skip to content

Limit concurrent bcrypt authentications to bound CPU under auth floods#108527

Open
groeneai wants to merge 4 commits into
ClickHouse:masterfrom
groeneai:fix/harden-bcrypt-concurrent-auth-dos
Open

Limit concurrent bcrypt authentications to bound CPU under auth floods#108527
groeneai wants to merge 4 commits into
ClickHouse:masterfrom
groeneai:fix/harden-bcrypt-concurrent-auth-dos

Conversation

@groeneai

@groeneai groeneai commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Related: #87115

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Added a server setting max_concurrent_bcrypt_authentications that caps the number of bcrypt_password verifications running concurrently across the server. bcrypt is intentionally CPU-expensive, so a flood of authentication attempts with distinct passwords (which the bcrypt result cache cannot absorb) can saturate all CPU cores and cause a denial of service. Attempts exceeding the limit fail fast with the generic authentication error and are counted by the BcryptAuthenticationThrottled event in system.events. Cache hits and repeated identical credentials are never throttled. The default is 0 (unlimited), preserving the previous behavior.

Description

bcrypt is deliberately CPU-expensive (the default work factor of 12 is roughly 100ms of CPU per verification). An unauthenticated client who knows a user with a bcrypt password exists can send repeated authentication attempts; each forces a full bcrypt_checkpw, so a modest request rate saturates CPU cores and denies service.

PR #87115 added an LRU cache keyed on SHA256(password) plus the bcrypt hash (negative results cached too). That solves the repeated-identical-credential vector: after the first miss every subsequent attempt with the same pair is an O(1) cache hit. The residual gap is that the cache is keyed on the password, so an attacker sending a different password each request misses the cache every time and runs a full bcrypt_checkpw per attempt. CacheBase::getOrSet serializes only same-key threads, so distinct passwords all run bcrypt concurrently. The per-user FAILED_SEQUENTIAL_AUTHENTICATIONS quota does not cover this either: it is opt-in and does not apply to unknown usernames.

Approach. A process-wide cap on the number of bcrypt verifications running concurrently, applied only on the cache-miss path inside getOrSet's load function:

  • Cache hits never touch the limiter, so a healthy client retrying the same credentials is unaffected.
  • Admission is fail-fast, not blocking: a thread that cannot get a slot throws AUTHENTICATION_FAILED immediately rather than queuing, so a flood cannot pile up connection threads and amplify the DoS.
  • The rejection surfaces as the existing generic authentication error (no information disclosure) and is not cached, so a legitimate client retrying once load drops succeeds.
  • Rejections are observable via the BcryptAuthenticationThrottled profile event.

This bounds worst-case bcrypt CPU to concurrency * cost(workfactor) regardless of how many distinct passwords are tried, composes with #87115 (the cache absorbs repeats, the cap absorbs the distinct-password flood), and, being identity-agnostic, cannot be abused to lock out a specific user (which is why per-source / fail2ban-style blocking is risky here).

Compatibility. The new key is a server config-file setting (config.xml), plumbed through AccessControl::setupFromMainConfig next to bcrypt_workfactor, and defaults to 0 = unlimited. With the key absent the behavior is byte-for-byte identical to today. It is not a query-level Setting, so SettingsChangesHistory.cpp does not apply (same as bcrypt_workfactor). Like bcrypt_workfactor, it is read once at startup from the main config and is not re-applied by SYSTEM RELOAD CONFIG, so a restart is required to change the limit.

Tests.

  • Unit test gtest_bcrypt_concurrency_limiter for the limiter primitive: zero means unlimited, admission/rejection at the limit, slot release on guard destruction, move semantics, and that the bound holds under 16-thread contention.
  • Integration test test_bcrypt_concurrency_limit for end-to-end behavior: (a) repeated identical correct credentials are never throttled (cache hits), (b) a concurrent distinct-password flood is capped (observable via the profile event) while every attempt still returns the generic auth error, and (c) the default 0 preserves unlimited behavior.

Verified locally as well: with the limit set to 1 a 64-way distinct-password flood produced 63 throttled rejections (all returning the generic message, with no internal "concurrent bcrypt" wording leaked to the client); identical correct credentials produced 0 throttles; and with the limit at 0 the same flood produced 0 throttles.

Open question for maintainers. This PR ships a conservative default of 0 (unlimited) so it is strictly backward-compatible. A non-zero default would harden the out-of-the-box posture but risks rejecting legitimate concurrent logins (e.g. a fleet of clients reconnecting at once). Happy to switch to a small non-zero default (such as a fraction of CPU cores) if preferred.

bcrypt is intentionally CPU-expensive (workfactor 12 is ~100ms of CPU per
verification). An unauthenticated client can send repeated auth attempts and
saturate every core, causing a denial of service.

PR ClickHouse#87115 added an LRU cache keyed on SHA256(password) + bcrypt hash, which
absorbs floods that reuse the same credential: after the first miss, repeats
are O(1) cache hits. But the cache is keyed on the password, so an attacker
sending a different password each request misses every time and runs a full
bcrypt_checkpw per attempt. CacheBase::getOrSet serializes only same-key
threads, so distinct passwords all run bcrypt concurrently. The per-user
FAILED_SEQUENTIAL_AUTHENTICATIONS quota does not help: it is opt-in and does
not cover unknown usernames.

This adds a process-wide cap on the number of bcrypt verifications running
concurrently, applied only on the cache-miss path inside getOrSet's load
function so cache hits stay completely unthrottled. Admission is fail-fast:
a thread that cannot get a slot throws AUTHENTICATION_FAILED rather than
blocking, so a flood cannot pile up connection threads and amplify the DoS.
The throw surfaces as the generic authentication error (no information
disclosure) and is not cached, so a legitimate client retrying under lower
load succeeds. Rejections are counted by the BcryptAuthenticationThrottled
profile event.

The cap is a new server config key max_concurrent_bcrypt_authentications,
plumbed through AccessControl::setupFromMainConfig next to bcrypt_workfactor.
It defaults to 0 (unlimited), so behavior is unchanged unless configured.

Together with ClickHouse#87115 this bounds worst-case bcrypt CPU to
concurrency * cost(workfactor) regardless of how many distinct passwords are
tried: the cache absorbs repeats, the cap absorbs the distinct-password flood.
Being identity-agnostic, it cannot be abused to lock out a specific user.

Adds a unit test for the limiter (admission, release, move semantics, the
bound under contention) and an integration test covering the three behaviors:
cache hits unthrottled, concurrent distinct-password floods capped, and 0
preserving unlimited behavior.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

@groeneai

Copy link
Copy Markdown
Contributor Author

cc @kcmannem @pufit — could you review this? It closes the residual bcrypt auth DoS left after #87115: that cache only absorbs repeated identical credentials, so a flood of distinct passwords still runs an unbounded number of concurrent bcrypt_checkpw calls and saturates CPU. This adds an optional process-wide cap (max_concurrent_bcrypt_authentications, default 0 = unlimited) applied only on the cache-miss path, so cache hits stay unthrottled and worst-case bcrypt CPU is bounded regardless of how many distinct passwords are tried.

@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jun 25, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [067b259]

Summary:

job_name test_name status info comment
Stateless tests (amd_tsan, s3 storage, parallel, 2/2) FAIL
Server died FAIL cidb, issue
ThreadSanitizer: data race (STID: 4071-3348) FAIL cidb
Stress test (amd_debug) FAIL
Cannot start clickhouse-server FAIL cidb
Logical error: 'Unexpected exception in refresh scheduling' (STID: 2508-34af) FAIL cidb
Check failed FAIL cidb
Stress test (arm_msan) FAIL
Logical error: Stream A for column B with type C is not found (STID: 3190-5d7d) FAIL cidb

AI Review

Summary

This PR adds an optional process-wide bcrypt_password verification concurrency limit, exposes it through server config, counts limiter rejections in BcryptAuthenticationThrottled, and covers the limiter and end-to-end authentication behavior with new tests. The current code fixes the earlier restart-only documentation and multi-auth-method fall-through issues, and I do not see an unresolved correctness or safety problem in the changed authentication path.

Findings

💡 Nits

  • [PR description] The visible relationship line uses Related: #87115; the repository instructions ask for full GitHub URLs for related pull requests. Use Related: https://github.com/ClickHouse/ClickHouse/pull/87115.
Final Verdict

Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jun 25, 2026
Comment thread src/Access/AccessControl.cpp
Like bcrypt_workfactor, the setting is read once at startup via
AccessControl::setupFromMainConfig and is not re-applied by SYSTEM RELOAD
CONFIG, so a restart is required for a change to take effect. Add a doc note
matching bcrypt_workfactor's reload behavior.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Good catch. Documented as restart-only rather than wiring a reload path.

I checked the neighbor setting: bcrypt_workfactor is set in the same AccessControl::setupFromMainConfig, whose only caller is the startup path (Server.cpp). SYSTEM RELOAD CONFIG runs the main-config reload callback (which touches access_control only via setAllowTierSettings) and access_control.reload(USERS_CONFIG_ONLY) (users.xml entities only). Neither re-reads bcrypt config, so bcrypt_workfactor is itself restart-only.

The PR positions this setting next to bcrypt_workfactor, so matching that neighbor's behavior is the consistent choice. Applying setupFromMainConfig from the reload path would also change reload semantics for the rest of that config block (allow_no_password, the access_control_improvements.* flags, etc.), which is out of scope here.

Fix in b527160: dropped the "changeable on reload" claim from the PR description and added a doc note that the setting is read once at startup and requires a restart to change, mirroring bcrypt_workfactor.

Comment thread src/Access/AuthenticationData.cpp Outdated
The concurrency limiter rejected an over-the-limit bcrypt verification by
throwing out of checkPasswordBcrypt. That exception propagated through
IAccessStorage::authenticateImpl's per-method loop, aborting it. A user may
list several authentication methods as alternatives (e.g. bcrypt_password and
plaintext_password); the loop tries the next method only when the current one
returns Fail. So a throttled bcrypt method wrongly rejected a login that a
later method (here plaintext) would have accepted.

Make limiter denial behave like a failed bcrypt method: return false so the
loop continues, matching how every other method in checkBasicAuthentication
reports a failed check. Denial is signalled out of the cache load function via
a sentinel that escapes getOrSet uncached, so a correct password throttled once
is not cached as wrong and still authenticates under lower load.

The denial still increments BcryptAuthenticationThrottled and the worst-case
bcrypt CPU is still bounded; only the loop-aborting behavior changes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

Pre-PR validation gate for commit 1664fcc (multi-auth-method fall-through fix).

Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? Yes. 30+ multi-method users (bcrypt first, plaintext second), each with a distinct correct password, while a separate bcrypt-only user's flood of distinct passwords saturates the single slot (max_concurrent_bcrypt_authentications=1). Each correct plaintext login probed once = guaranteed bcrypt cache miss. Pre-fix: 39/40 rejected; post-fix: 0/40.
b Root cause explained? Throttle denial threw out of checkPasswordBcrypt's getOrSet load function. That exception propagated through IAccessStorage::authenticateImpl's per-method loop and aborted it. Since the loop only advances to the next listed method when the current one returns Fail, a throttled bcrypt method aborted the login before a later (plaintext) method could authenticate.
c Fix matches root cause? Yes. Denial now returns false (a failed bcrypt method) instead of throwing, so the loop continues. Matches every other branch of checkBasicAuthentication, all of which return Fail on a failed check.
d Test intent preserved / new test added? Existing 3 tests unchanged. Added test_throttled_bcrypt_method_falls_through_to_next_method, which asserts (under saturation) that correct logins of multi-method users succeed, the throttle path is actually exercised (counter rises), and a genuinely wrong password is still rejected.
e Both directions demonstrated? Yes. Pre-fix binary (Build ID 68fd2899): 39/40 fail. This-commit binary (Build ID fc912e99): 0/40 fail, throttle counter still +~1000. Same repro, only the source differs.
f Fix general across code paths? Yes. The throttle was the only path in checkBasicAuthentication that threw out of a per-method check; all sibling methods already return Fail. The remaining ret == -1 throw (corrupt stored hash, a genuine server-side data error, not a credential mismatch) is intentionally left as-is. Not a symptom guard: the throw was the defect.
g Generalizes across inputs? Yes. Verified 0 failures for plaintext-first/bcrypt-second and for bcrypt, bcrypt, plaintext users (correct login reachable only after both throttled bcrypt methods fall through). Order-independent and multi-bcrypt-method safe.
h Backward compatible? Yes. No setting default or format change. Default limit 0 = unlimited preserves historical behavior; with a nonzero limit the only change vs the previous commit is that throttling no longer aborts a multi-method login.
i Invariants and contracts preserved? Yes. Cache-not-poisoned invariant verified: a correct password throttled once (returns false uncached) authenticates 5/5 after load subsides (the sentinel escapes getOrSet before the cache set, so false is never stored). Per-key token dedup preserved (limiter stays inside the load function). Limiter in_flight accounting unchanged (RAII guard releases on scope exit, including the sentinel throw).

Session id: cron:clickhouse-worker-slot-1:20260625-181000

Two warnings-as-errors flagged by arm_tidy on the new code:

1. hicpp-exception-baseclass on the throttle sentinel in
   AuthenticationData.cpp. The sentinel must be thrown (not returned)
   so it escapes CacheBase::getOrSet uncached, otherwise a correct but
   throttled password would be cached as wrong. Derive BcryptThrottled
   from std::exception to satisfy the check. It is caught locally in
   checkPasswordBcrypt and never reaches a client, so behavior is
   unchanged: throttling still reports a failed bcrypt method.

2. clang-analyzer-cplusplus.Move / hicpp-invalid-access-moved in
   gtest_bcrypt_concurrency_limiter.cpp, which called acquired() on a
   moved-from guard. Restructure MoveTransfersOwnership to verify move
   semantics through observable in_flight accounting (move out of an
   inner scope, then confirm the single slot is still held and is freed
   only when the owning guard is destroyed) without touching the
   moved-from object.

No functional change; the limiter, multi-method fall-through, and
both-directions behavior are unchanged. unit_tests_dbms passes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.40% 85.40% +0.00%
Functions 92.60% 92.60% +0.00%
Branches 77.60% 77.60% +0.00%

Changed lines: Changed C/C++ lines covered by tests: 128/135 (94.81%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

@groeneai

Copy link
Copy Markdown
Contributor Author

CI finish ledger — 067b259

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test Reason Owner / fixing PR
Stateless tests (amd_tsan, s3 storage, parallel) / TSan data race (STID 4071-3348) + Server died data race in QueryStatus::releaseWorkloadResources (chronic; 49 PRs / 7 master / 30d; no link to bcrypt) #108391 (external, merged 23:49 UTC) — this build (committed 20:26 UTC) predates the fix; cleared on next rebase
Stress test (amd_debug) / "Unexpected exception in refresh scheduling" (STID 2508-34af) + Cannot start clickhouse-server RefreshTask scheduling race (chronic; 111 PRs / 10 master / 30d) #105588 (ours, open)
Stress test (arm_msan) / "Stream for column is not found" (STID 3190-5d7d) Variant/Dynamic/JSON subcolumn serialization (chronic; 8 PRs / 1 master / 30d) #107562 (external, open)
Build (arm_tidy) PR-caused clang-tidy errors in new bcrypt limiter code fixed in this PR @ 067b259 (now SUCCESS)
CH Inc sync CH Inc sync (private, not actionable)

Session id: cron:our-pr-ci-monitor:20260626-010000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants