fix(#5029): authentication hardening - brute-force lockout, strong generated password, unified policy, constant-time compare - #5088

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review: authentication hardening (#5029)

Overall this is a solid, well-scoped hardening PR. The changes closely mirror the existing API-token brute-force path, the entropy math for the generated password is documented and correct, and the constant-time compare is applied at the right spot. Test coverage for the new lockout behavior (trip, cross-user isolation, counter reset, malformed hash) is good. A few points worth considering before merge.

1. Username-keyed lockout is a targeted account-lockout DoS vector (design tradeoff)

Because the lockout is keyed purely by user name with no IP/source scoping, an attacker who knows a valid username - and root is always a valid, well-known target - can lock that account out of every protocol (http, bolt, postgresw, mongodbw, grpcw, MCP, and the internal ArcadeDBServer.authenticate) by sending 5 bad passwords, and can keep it locked indefinitely by repeating 5 attempts every 30s. This converts an online-guessing mitigation into an availability regression against known accounts.

The PR body acknowledges IP keying as future work, which is fair, but this is the more impactful side of the tradeoff and deserves an explicit note in the issue/changelog. One mitigation that neutralizes the DoS while keeping brute-force protection: during lockout, still evaluate the supplied password and let a correct password through (an attacker does not have it, so guessing stays blocked, but the legitimate user is not locked out). The cost is that you keep paying the PBKDF2 CPU during lockout - which is itself a consideration - so at minimum I would make the threshold/window operator-configurable rather than hard-coded 5/30_000.

2. passwordFailures map can grow unbounded from attacker-controlled keys

The unknown-user branch calls recordPasswordFailure(userName) with the raw, arbitrary-length, attacker-supplied username. An attacker can flood distinct random usernames and grow the map until the 30s cleanup timer fires, giving a memory-pressure vector bounded only by request rate x 30s. The token path caps its key at an 8-char hash prefix, which limits distinct keys; the username path has no such bound. Consider capping the map size, hashing/truncating the key, or only recording failures for existing users (weighed against user-enumeration timing).

3. Minor: data race on the long[] counter

The lockout check reads existing[0]/existing[1] outside compute() while recordPasswordFailure mutates the same array in place. Worst case is a slightly off count - benign, and it matches the existing token path - but worth a comment acknowledging the intentional relaxed consistency.

4. Minor / nits

• recordPasswordFailure duplicates the inline tokenFailures.compute(...) lambda almost verbatim. Extracting a shared recordFailure(map, key, windowMs) helper would keep the two paths from drifting again (the whole PR exists because these paths drifted).
• No test covers lockout expiry after the 30s window / the cleanup-timer purge. Understandable since it needs a clock seam, but the reset-after-window branch in recordPasswordFailure and the timer removal are currently untested.
• Routing PostServerCommandHandler.createUser through validateCredentials also newly enforces username length (min 4 / max 256). That is an improvement and consistent, just flagging it as a behavior change for existing clients that created short-named users via this endpoint.
• docs/5029-auth-hardening.md reads as internal audit/working notes - confirm you intend to ship it under docs/.

Verified correct

• passwordMatch: hash from encodePassword(password, salt, iterations) is the full ALG$ITER$SALT$HASH string, same format/length as the stored hashedPassword, so MessageDigest.isEqual compares equal-length arrays as intended, and the parts.length != 4 guard short-circuits before isEqual for malformed hashes (no throw).
• Lockout window does not self-extend: throttled attempts throw before recordPasswordFailure, so the timestamp is not refreshed and lockout expires exactly 30s after the 5th failure. Good.
• passwordFailures cleanup is wired into the same guarded timer as tokenFailures.

Nice work - the core hardening is correct; my only real ask is to surface the account-lockout DoS tradeoff (item 1) and consider bounding the map (item 2).

Code Review: PR #5088 - Authentication hardening

Overall this is a well-scoped, high-quality change. The four fixes are on-target for #5029, the code mirrors the existing API-token lockout path closely, the tradeoffs are documented, and the review-cycle findings (safe-publish of the long[], bounded map key, null-password guard) were folded in cleanly. Nice work. A few notes below, mostly minor.

Strengths

• Brute-force lockout correctly checks the lockout before user lookup (fast reject, no user enumeration via this path) and records failures uniformly on unknown-user and wrong-password, so it doesn't leak whether a principal exists.
• recordPasswordFailure publishes a fresh long[] on every update instead of mutating in place, so concurrent readers in authenticate always see a consistent snapshot. This is actually safer than the older authenticateByApiToken path, which still mutates entry in place (out of scope here, but a good future cleanup to unify).
• generateRandomPassword is clean: single shared SecureRandom, preallocated StringBuilder, ~143 bits from a 62-char alphabet. SecureRandom.nextInt(62) handles modulo bias via rejection sampling, so no entropy skew.
• Constant-time compare is sound: for a wrong password hash is recomputed with the same salt/iterations extracted from the stored value, so both operands are always the same length (fixed 256-bit PBKDF2 output -> fixed Base64 length), which is exactly the case where MessageDigest.isEqual gives real constant-time behavior.

Suggestions

1. docs/review-deferred-f327302e.md probably shouldn't ship. It's a review-cycle scratch artifact named by commit hash, referencing "claude" and "gemini-code-assist" review passes - it doesn't match the repo's docs/NNNN-<name>.md convention (which 5029-auth-hardening.md correctly follows). The rationale it captures is already in 5029-auth-hardening.md. Consider dropping it from the PR to keep docs/ as durable design notes rather than per-cycle review logs.

2. Account-lockout DoS on root. Already documented as an accepted tradeoff, and it does mirror the token path, so no objection to merging as-is. Worth emphasizing that because the key is per-principal and root is well-known, an attacker can hold root locked across all protocols (http, bolt, postgres, mongo, grpc) with 5 bad passwords every 30s - a broader blast radius than the token path in practice. Making MAX_PASSWORD_FAILURES / PASSWORD_LOCKOUT_MS operator-configurable (as the doc's follow-up notes) would let operators tune or disable this; a small, low-risk follow-up.

3. No automated test for the PostServerCommandHandler.createUser policy change. The min-length-8 routing through validateCredentials is only covered by the manual PostServerCommandHandlerIT in the test plan. A cheap unit/handler test asserting a 4-7 char password is now rejected with the correct message would lock in the fix and guard the regression. Note this is a stricter contract - previously-accepted 4-7 char passwords and <4 char usernames will now be rejected - so it's worth an explicit test.

4. passwordMatch malformed-hash robustness (pre-existing, minor). The new test asserts a malformed stored hash "must not throw", but only exercises the parts.length != 4 branch. A stored value like PBKDF2$notanumber$salt$hash (4 parts, non-numeric iterations) still throws NumberFormatException at Integer.parseInt(parts[1]). This predates the PR, but since you added the malformed-hash test and touched this method, wrapping the parse (or a try/catch -> return false) would make the "must not throw" guarantee hold for all malformed inputs.

Minor / FYI

• The lockout window is anchored at the first failure in a window (entry[1] is preserved on increment), so the 30s clock starts at failure build support #1, not Bump jedis from 3.6.1 to 3.6.3 #5. This is identical to the token path, so it's consistent - just noting the window is "30s from first failure," not "30s from last failure."
• Stale passwordFailures entries are only purged by the cleanup timer scheduled in loadUsers() (server != null path). Entries self-expire logically on read, so this is only a transient-memory concern bounded by the 30s cleanup interval - same as tokenFailures. Fine as-is.

Tests, style (final params, AssertJ assertThat(...).isTrue() form, no fully-qualified names), and comments all read consistently with the surrounding code. LGTM once the review-deferred doc question is resolved.

Review: authentication hardening (#5029)

Overall this is a solid, well-scoped security PR. The four hardening items are each implemented cleanly, mirror the existing API-token path, and come with focused regression tests. The tracking doc, the DoS tradeoff write-up, and the "deferred with rationale" notes make the intent easy to follow. Nice work.

Strengths

• Brute-force lockout correctly checks the counter before user lookup and increments on both unknown-user and wrong-password, so it does not leak account existence via the lockout path.
• recordPasswordFailure safe-publish: returning a fresh long[] per update (rather than the in-place mutation the token path still does) is the more correct concurrency pattern. Good catch during review.
• Null stored-password guard folded into the credential-check branch prevents an NPE in passwordMatch and still returns the generic "User/Password not valid".
• generateRandomPassword is now SecureRandom over a 62-char alphabet with a clearly-documented entropy budget - a real improvement over the ~32-bit truncated UUID.
• passwordMatch correctly compares the full algorithm$iterations$salt$hash string (both same length/format), so MessageDigest.isEqual is a valid constant-time comparison, and the NumberFormatException guard on the iteration count is a good robustness add.
• Good test coverage: lockout trip, cross-principal isolation, counter reset on success, and both malformed-hash failure modes.

Minor points (non-blocking)

1. Lockout window is anchored to the first failure, not the last. recordPasswordFailure preserves entry[1] (first-failure timestamp) across increments, and the lockout check is now - entry[1] < PASSWORD_LOCKOUT_MS. So if the 5 failures are spread over, say, 24s, the effective lockout after tripping is only ~6s rather than a full 30s from the last attempt. This is consistent with the existing token path, so it is fine to keep for parity - but it is a "lockout since first failure" rather than the more common "lockout since last failure" semantics, and worth a one-line comment so a future reader does not mistake it for the latter.

2. Duplicated constants. MAX_PASSWORD_FAILURES/PASSWORD_LOCKOUT_MS are byte-for-byte identical to MAX_TOKEN_FAILURES/TOKEN_LOCKOUT_MS, and the two compute lambdas / cleanup-timer removals are near-identical. You deliberately deferred the shared-helper refactor to avoid touching the tested token path, which is reasonable - but since the password path already improved on the token lambda (safe-publish), the two are now subtly different implementations of the same thing. A follow-up that unifies them (and back-ports the safe-publish fix to the token path, which still mutates long[] in place around lines 747-752) would be worthwhile.

3. Behavior change: username length now validated on the server-command create-user path. Routing through validateCredentials means CREATE USER {...} now also enforces userName.length() >= 4, where before it validated only the password. This is the intended unification, but it is a stricter contract than before - fine given it is documented, just flagging for release notes.

4. Distinguishable lockout message. During lockout the caller gets "Too many failed authentication attempts..." vs "User/Password not valid" otherwise. Since lockout applies to unknown users too, this does not enable enumeration, but it does confirm to an attacker that they have successfully triggered a lockout (useful for the documented targeted-account DoS). Acceptable given the accepted tradeoff.

Question

• Should docs/5029-auth-hardening.md ship in the repo long-term? It follows the existing docs/NNNN-*.md convention, so this is just a confirmation - the content reads more like an internal audit/PR narrative than user-facing documentation.

Nothing here is blocking. The keying-by-principal DoS is inherent and well-documented, and making the threshold/window operator-configurable plus optional IP-scoped keying are the right follow-ups to capture in an issue.

Review: PR 5088 - Authentication hardening

Overall a high-quality, well-scoped security PR. The four fixes are correct, the code mirrors the existing API-token path closely, and the concurrency reasoning (safe-publish of fresh long[] snapshots, ThreadLocal SHA-256 digest, atomic compute) is sound. Tests and the docs/5029 write-up are thorough. A few observations, none blocking.

Strengths

• Lockout check placed before user lookup, and failures recorded for both unknown-user and wrong-password with the same "User/Password not valid" message. This avoids leaking user existence, and since unknown users also get locked out, the distinct "Too many failed attempts" message does not reveal a valid account either.
• Safe publication: recordPasswordFailure returns a fresh long[] per update instead of mutating in place, so the lock-free read at authenticate:206 always sees an internally-consistent snapshot. This is better than the pre-existing token path (authenticateByApiToken:751-754), which still mutates entry[0]++/entry[1] in place under a benign data race.
• Null-password guard (su.getPassword() == null) before passwordMatch, and passwordMatch now returns false (rather than throwing) on both malformed part-count and non-numeric iterations - both well tested.
• Constant-time compare and the 62-char/24-length SecureRandom password are correct; the entropy math checks out (~143 bits).

Points worth considering

1. Behavior change in the server-command create-user path (compat). Routing through validateCredentials now also enforces userName min-length 4 (and empty/too-long checks), where previously the server-command path only checked json.has("name"). Existing automation creating users with 1-3 char names via POST /server will now get a 403. Arguably the intended "unified policy," but it is a wire-visible change beyond just the password rule - worth a release note.

2. Internal/service-account self-lockout. Because lockout is per-principal across all protocols, a service or HA/replication account that authenticates frequently and occasionally supplies a stale credential could lock itself out for 30s cluster-wide. The DoS tradeoff doc covers the attacker case; the accidental self-lockout of automated callers is worth a line in the doc.

3. Divergence between the two failure-tracking paths. The password path uses the safer fresh-array pattern while the token path still mutates in place. Keeping them subtly different invites future drift. Consider back-porting the fresh-array pattern to authenticateByApiToken (low risk, same shape) or extracting a shared recordFailure(map, key, max, window) helper. The PR intentionally deferred this to avoid touching tested token code - reasonable, but the inconsistency is now baked in.

4. Unbounded entry count between cleanup cycles. Hashing the key bounds per-entry size, but an attacker spraying distinct usernames still creates one map entry each until the 30s cleanup timer runs. Same as the token path, so acceptable, but the doc phrase "cannot inflate per-entry memory" could be misread as bounding total memory, which it does not.

Test coverage
Good coverage of the core paths. Gaps that would strengthen it:

• Lockout window expiry - no test that auth succeeds again after PASSWORD_LOCKOUT_MS elapses; the counter-reset-on-stale-window branch in recordPasswordFailure is currently unexercised.
• Unknown-user lockout - tests only lock out an existing user; asserting repeated unknown usernames also trip the lockout (with the same generic message) would lock in the non-enumeration property.
• Minor: PostServerCommandHandlerIT.createUserRejectsShortPassword leads with DROP USER on a user that may not exist - confirm that is idempotent so the test is not order-dependent.

Nit: the PR summary says lockout is "keyed by user name," while the code/doc key by a SHA-256 prefix - a description mismatch only; the code is correct.

Nice work - the security reasoning and concurrency care here are above the bar.

Review: authentication hardening (#5029)

Overall this is a well-scoped, well-tested, and unusually well-documented change. The four hardening items are implemented cleanly, the API-token path is mirrored faithfully, and the review-cycle fixes (safe-publish of the counter, bounded key, null-password guard, malformed-hash guard) resolved the substantive correctness issues. Nice work.

A few observations, mostly non-blocking.

Correctness / security

1. Per-principal lockout is a real cross-protocol DoS on well-known accounts. Already documented in docs/5029-auth-hardening.md, but worth restating as the most important tradeoff: an unauthenticated attacker who knows a valid name (notably root) can hold that account in lockout indefinitely by sending 5 bad passwords every 30s, and because authenticate() is the shared entry point for HTTP, Bolt, Postgres, Mongo, gRPC and MCP, the lockout denies that principal across all protocols at once. Mirroring the API-token behavior is defensible, but I'd prioritize the deferred IP-scoped keying, since root lockout has a materially higher blast radius than an opaque API token. At minimum, consider not locking out the built-in root account on principal alone.

2. User-enumeration timing oracle is untouched. shouldLockOutUnknownUserWithGenericMessage correctly locks in the identical message, but the timing still differs: an unknown user hits recordPasswordFailure and throws immediately, while a known user with a wrong password runs a full PBKDF2 (passwordMatch) first. That's an observable enumeration side channel. It's pre-existing (the original if (su == null) throw had it) and arguably out of scope, but since the PR is titled "auth hardening" and already touches this method, it deserves at least a follow-up note.

3. Failure-map memory is bounded per-entry but not in aggregate. The doc acknowledges this honestly. Since hashing already gives a fixed-size key, a hard cap on passwordFailures.size() (skip new keys past a ceiling, or evict oldest) would cheaply close the spray-distinct-usernames vector that the 30s cleanup timer alone doesn't fully bound. Optional.

Maintainability

4. The token and password paths are now near-duplicate but diverge on the one thing that matters. recordPasswordFailure publishes a fresh long[] (safe), while the inline token lambda in authenticateByApiToken still mutates entry[0]++ in place - the exact race fixed here. Deferring the shared-helper extraction is reasonable, but the safe-publish fix arguably should be back-ported to the token path too, otherwise it retains the latent torn-read the reviewers flagged. A tracked follow-up (already noted in the doc) is fine.

Docs / housekeeping

5. Consider whether the review-process meta-commentary belongs in a durable docs/ file. The design rationale in docs/5029-auth-hardening.md is genuinely useful, but the "Review cycles" section (e.g. "gemini-code-assist ... re-reviews inconsistently", "max-cycles-reached (4/4)") is process chatter that will age poorly in the repo. I'd trim to the technical rationale and drop the reviewer-behavior notes.

6. Minor: comment vs. test mismatch. createUserRejectsShortPassword uses a 5-char password ("short"), but the Javadoc and PR description say "7-char". Cosmetic, but worth aligning.

Nits (non-blocking)

• The PR test plan says generated passwords are "mixed-case", but that's probabilistic and (correctly) not asserted by the test; the wording overstates the guarantee.
• MessageDigest.isEqual short-circuits on length mismatch, but since the computed hash reuses the salt parsed from the stored value, lengths always match for a well-formed hash, so the content comparison stays constant-time. Good.

Test coverage is solid for the in-scope behavior (lockout trip, isolation between principals, counter reset, unknown-user parity, malformed-hash handling, create-user policy). The only meaningful gap is the window-expiry path, which the doc reasonably explains away as needing a clock seam or a 30s wait.

Codecov Report

❌ Patch coverage is 61.29032% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.97%. Comparing base (639bfd1) to head (f00ffc7).
⚠️ Report is 5 commits behind head on main.

@@             Coverage Diff              @@
##               main    #5088      +/-   ##
============================================
+ Coverage     65.61%   65.97%   +0.35%     
- Complexity      841      847       +6     
============================================
  Files          1690     1690              
  Lines        135513   135587      +74     
  Branches      28987    29000      +13     
============================================
+ Hits          88920    89450     +530     
+ Misses        34466    33897     -569     
- Partials      12127    12240     +113     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.