Limit concurrent bcrypt authentications to bound CPU under auth floods#108527
Limit concurrent bcrypt authentications to bound CPU under auth floods#108527groeneai wants to merge 4 commits into
Conversation
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>
|
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 |
|
Workflow [PR], commit [067b259] Summary: ❌
AI ReviewSummaryThis PR adds an optional process-wide Findings💡 Nits
Final VerdictStatus: ✅ Approve |
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>
|
Good catch. Documented as restart-only rather than wiring a reload path. I checked the neighbor setting: The PR positions this setting next to 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 |
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>
|
Pre-PR validation gate for commit 1664fcc (multi-auth-method fall-through fix). Pre-PR validation gate (click to expand)
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>
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered by tests: 128/135 (94.81%) | Lost baseline coverage: none · Uncovered code |
CI finish ledger — 067b259Every 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 Session id: cron:our-pr-ci-monitor:20260626-010000 |

Related: #87115
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Added a server setting
max_concurrent_bcrypt_authenticationsthat caps the number ofbcrypt_passwordverifications 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 theBcryptAuthenticationThrottledevent insystem.events. Cache hits and repeated identical credentials are never throttled. The default is0(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 fullbcrypt_checkpwper attempt.CacheBase::getOrSetserializes only same-key threads, so distinct passwords all run bcrypt concurrently. The per-userFAILED_SEQUENTIAL_AUTHENTICATIONSquota 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:AUTHENTICATION_FAILEDimmediately rather than queuing, so a flood cannot pile up connection threads and amplify the DoS.BcryptAuthenticationThrottledprofile 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 throughAccessControl::setupFromMainConfignext tobcrypt_workfactor, and defaults to0= unlimited. With the key absent the behavior is byte-for-byte identical to today. It is not a query-levelSetting, soSettingsChangesHistory.cppdoes not apply (same asbcrypt_workfactor). Likebcrypt_workfactor, it is read once at startup from the main config and is not re-applied bySYSTEM RELOAD CONFIG, so a restart is required to change the limit.Tests.
gtest_bcrypt_concurrency_limiterfor 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.test_bcrypt_concurrency_limitfor 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 default0preserves 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.