From 4dc6d4c6f857ffc8248f88b54aa8ffc4c5e85798 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 12 Aug 2026 21:04:25 -0500 Subject: [PATCH] docs(security): the per-account lockout auto-expires but re-locking is unbounded (BACKLOG #1131) Two sentences in docs/SECURITY.md asserted a ceiling the shipped code does not implement, and one test's name asserted the same thing while its body checked something weaker. WHAT WAS FALSE :1544 (anti-automation table, row 1) said the lockout is "auto-expiring, so an attacker cannot maliciously lock an account indefinitely". Each individual lock does auto-expire, but _register_failure (auth/service.py:749-769) restarts the counter on a lapsed window (:757) and re-locks on the next run to the threshold (:761-765). The account row persists a failure count and an expiry, never a count of locks, so nothing accumulates across cycles and the number of cycles has no ceiling. The claim was false in the direction that matters: a reader acting on it would conclude no operator action is ever needed. :59 said "a single-admin deployment can't be locked out". In context that sentence is about AUTO-RETIREMENT, but its words denied the failed-attempt lockout, which is a different mechanism and does reach a claimed sole administrator. It is also the sentence a reader greps. Two sites, one false claim, one axis. WHAT REPLACES THEM A bounded account rather than a bare denial: the auto-expiry, the shipped thresholds, the serial re-lock, the signal the holder gets, the two recovery routes and their cost to the victim, and the sole-administrator residual. Deleting the false clause would have left the section quieter and no more informative. Scope-checked rather than asserted: only local accounts can be locked (the counter has exactly two feeders, auth/service.py:672 and :2092, both local-gated); control 6 narrows who can reach the account but is inert behind an undeclared proxy or NAT, per its own honest-limit note at :906-908, so it is not described as preventing the campaign; and the routes that clear a live lock both issue a new password rather than merely lifting the lock. MEANING CHANGE ON ANCHORED LINES -- READ THIS BEFORE TRUSTING A GREEN GATE docs/SECURITY.md:1544 and :59 carry ASVS evidence anchors whose expect text is matched as a SUBSTRING. A meaning change on such a line leaves the anchor resolving and every gate in this repo silent. This commit changes the MEANING of both lines, not just their position. The covering cells must be re-read by hand; that is the assessor lane's call, and the cell ids are deliberately not named here because this repo is public and the scorecard is not. The retired anchor text "an attacker cannot maliciously lock an account indefinitely" is deleted by this commit. It was certifying a gap, so it should be retired rather than re-pointed at the corrected sentence -- re-pointing would make the record cite the remediation as evidence of the defect. THE TEST test_malicious_lockout_clause_is_discharged_in_the_doc asserted in its NAME and docstring that "an attacker cannot lock an account out indefinitely", while its body only required the section to contain "auto-expiring" OR "lapsed window restarts". The second token is the phrase a truthful correction KEEPS, so the test would have stayed green through this fix while its name went on asserting what the fix disproves. It is renamed to test_lockout_auto_expires_but_re_locking_is_unbounded and now derives what it checks: - Store.record_login_failure's keyword-only parameters, by signature, so the absence of a lock counter is checked structurally. Deliberately not a grep for a SQL literal: that write spells the column as a bind parameter and clears the lock when passed None, so a spelling-based guard would be blind to an unlock built on it -- the same instrument failure this item is about. - _register_failure's reads off the user row, by AST, so a cross-cycle ceiling landing later reds the test and forces the prose to be re-derived. - The two call sites that feed the counter, by AST, which is what makes the local-only scope falsifiable. - Positively, in the row's own "Threshold / window" cell, that both halves are stated; and the thresholds as rendered forms with units rather than bare integers, since "15 min" contains both "5" and "15" and a bare-substring check on those is close to a tautology. Proved red-capable rather than assumed: reverting the retired clause, weakening the unbounded claim, and removing the local scope each turn the test red, with the unmodified tree green. NOT IN THIS COMMIT The ledger reference correction for #1131 (the item cites docs/SECURITY.md:1474 for a sentence that lives at :1544) is a docs/BACKLOG.md edit and the ledger is single-writer. auth/service.py:579-581's docstring carries the same over-strong claim this commit removes from :59 ("this can't lock out a legitimate single-admin deployment"). It is the source the doc sentence was written from, so the defect can regenerate from it. Left alone because the scope of this change was set deliberately at two documentation sites; flagged for follow-up. The sole-administrator dead end is an availability defect in a shipped control, not a documentation gap, and is being filed separately so that fixing the prose does not close it. --- docs/SECURITY.md | 48 ++++++++++++- tests/test_security_doc_rate_limits.py | 98 ++++++++++++++++++++++++-- 2 files changed, 139 insertions(+), 7 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index b80d8f8b..b9e43ee3 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -56,7 +56,12 @@ user exists, no further bootstrap occurs. self-retires while still **unclaimed** (never password-changed): it is **disabled once a second administrator exists**, and — if left unclaimed — **disabled `[auth].bootstrap_expiry_hours` after creation** (default 72 h; `0` disables the timer). Once you change its password it becomes a normal -admin account and is never auto-disabled, so a single-admin deployment can't be locked out. A retired +admin account and auto-retirement stops touching it: both the expiry timer and the supersession check +fire only while the bootstrap is still **unclaimed**, which is carried by the `must_change_password` +flag — so an administrative password reset, which re-sets that flag, **re-arms them**. And +auto-retirement is not the only way to lose an administrator: the failed-attempt lockout is a +**separate mechanism** and it does reach a claimed sole administrator (see +[Brute-force & abuse protection](#brute-force--abuse-protection)). A retired bootstrap login is refused like any other invalid credential and the retirement is audited (`auth.bootstrap_admin_retired`). @@ -1541,7 +1546,7 @@ threshold, the switch that disables it, and — the part that matters for "not d | # | Control | Protects | Threshold / window | Disable switch | What remains when off | |---|---|---|---|---|---| -| 1 | **Per-account lockout** | one account's credential-guessing, on the password **and** TOTP/recovery legs | 5 consecutive failures → 15 min; a lapsed window restarts the counter (auto-expiring, so an attacker cannot maliciously lock an account indefinitely) | **no dedicated off switch.** `lockout_minutes = 0` makes the lock expire instantly, which is the effective opt-out; `lockout_threshold = 0` is **not** an off switch — it locks on the *first* failure | limiters 2 + 3 only | +| 1 | **Per-account lockout** | one account's credential-guessing, on the password **and** TOTP/recovery legs | 5 consecutive failures → 15 min; a lapsed window restarts the counter, so each lock expires on its own — but **repetition is unbounded**: an attacker who keeps failing re-locks the account as each window lapses. Signal, recovery and what to arrange in advance: below the table | **no dedicated off switch.** `lockout_minutes = 0` makes the lock expire instantly, which is the effective opt-out; `lockout_threshold = 0` is **not** an off switch — it locks on the *first* failure | limiters 2 + 3 only | | 2 | **Sign-in sliding window** (`allow_login_attempt`) | password-spraying across many usernames, which never trips a single account's lockout | > 10 attempts per client IP **or** > 60 across all clients, per 60 s (either dimension alone refuses — `global_full or key_full`) | `[auth].login_rate_limit_enabled = false` | lockout only — **and limiter 3 disappears with it** (see below) | | 3 | **Per-actor credential-ceremony budget** (`allow_reauth_attempt`) | a session holder guessing a password at the re-proof surface, where lockout does **not** apply | > 10 ceremonies per acting **user**, per 60 s. **No global dimension** (`glob=0`) | *the same* `[auth].login_rate_limit_enabled` | **nothing** — `POST /me/reauth` and `POST /me/password` then have no anti-automation control at all | | 4 | **argon2 concurrency cap** | executor exhaustion under a login flood | an instance semaphore sized `max(2, min(8, cpu_count))`; every hash/verify runs off the event loop | none | n/a | @@ -1551,6 +1556,45 @@ threshold, the switch that disables it, and — the part that matters for "not d | 8 | **WebAuthn pending-ceremony bound** (`ChallengeCache.put`) | flooding passkey registration/assertion ceremonies | 16 pending ceremonies per **user** (evicts that *same* user's oldest, so one principal can never deny another's), 4096 engine-wide (**refuses** with a cause-naming `ChallengeCacheFullError`), 120 s TTL | none | limiter 3 on the assertion **finish** leg only — `POST /ui/reauth/webauthn` (`routes/core.py:689`) and the error re-render inside `POST /ui/reauth` (`:612`) charge `allow_reauth_attempt`. The routes that *stage* a ceremony — the thing `ChallengeCache.put` actually bounds — charge **no** limiter: `POST /ui/account/webauthn/enroll`, `POST /ui/account/webauthn/verify`, and `GET /ui/reauth`, which re-stages fresh assertion options on **every** render. There this bound plus cookie-holder-only reachability is all there is | | 9 | **JWKS min-refetch floor** (`JwksCache.get_key`) | unauthenticated `kid`-driven refetch amplification against the IdP on the OIDC callback leg — the sibling of control 7 on the *other* federated leg | one upstream fetch per **300 s**, globally (`[auth].oidc_jwks_min_refetch_seconds`), plus a `_MAX_JWKS_BYTES` **512 KiB** response-body cap and a 3600 s key TTL. Within the floor an unknown `kid` raises `JwksError` and that login fails (a still-cached key is served even past the soft TTL rather than fail while throttled) | `oidc_jwks_min_refetch_seconds = 0` — no validator floor, so this **is** a genuine opt-out, and it restores the amplification | limiter 2 and control 7 (the same legs charge `allow_login_attempt` and stage a bounded flow first) | +**Control 1 bounds the lock, not the campaign.** Each lockout releases itself after +`lockout_minutes`, but `_register_failure` restarts the counter on a lapsed window and re-locks on the +next run to the threshold, and the account row persists a failure count and an expiry — never a count +of locks — so nothing accumulates across cycles and the number of cycles has no ceiling. The account +is reachable in the gap between one lock expiring and the next being set, and no longer. Sustaining +the re-lock costs far fewer attempts than control 2's sign-in window admits from a single client +address, so control 2 does not bound it either. The exposure is availability, not credential +disclosure, and its scope is narrow: only **local** accounts can be locked at all (the +lockout-asymmetry note above says why), and control 6 refuses an off-network client before any +failure is counted — but only where client addresses are meaningful. Behind an undeclared proxy or +NAT control 6 is **inert**, by its own honest-limit note above, so it narrows who can reach the +account rather than closing the case. + +**Signal.** The account holder gets an `ACCOUNT_LOCKED` security event — mailed only under the +conditions the security-event notification section above states (an alert sink configured, the +notification setting on, and an address on the account), and recorded on `GET /me/security-events`, +which is a self-scoped feed the holder can read only while they still hold a live, fully-authenticated +session. Each refusal is also audited, so a campaign is visible in the audit log while it runs. What +is **not** available anywhere is **current lock state**: no API or console surface reports whether an +account is locked right now, and a locked account still lists as enabled. Diagnose a suspected lockout +from the audit log, not the user list. + +**Recovery.** Absent a sustained attacker nothing is needed — the lock expires on its own. Against a +sustained one: across all three store backends the writes that clear `locked_until` are +`set_password`, the successful-login write and the failed-attempt write, and only `set_password` can +run while a lock is live — control 1 refuses before any credential is verified, so neither the +successful-login write nor the login-time rehash beside it is ever reached, and the failed-attempt +write only clears an **already lapsed** lock. Two routes reach `set_password` while an account is +locked, both local-account-only, and **both issue a new password rather than merely lifting the +lock**: the holder's own `POST /me/password`, reachable only while they still have a live session +(session validation never consults `locked_until`, and that route is exempt from both the must-change +and the MFA-pending gates), and the +[administrator's reset](#admin-password-reset-wp-l3-12-asvs-646). No shipped command clears a lock +without going through one of those two — the CLI manages no users. + +**Arrange in advance.** Keep a **second administrator who can sign in**: the administrator reset +refuses a self-reset, so a sole administrator holding no live session has no in-band route back for as +long as an attacker sustains the lock. + > **Binding conditionality — controls 2 and 3 are one switch, not two.** > `[auth].login_rate_limit_enabled = false` constructs **neither** limiter: `_login_limiter` and > `_reauth_limiter` are both `None` and both accessors then return `True` unconditionally. They share diff --git a/tests/test_security_doc_rate_limits.py b/tests/test_security_doc_rate_limits.py index a3fddf70..572f428b 100644 --- a/tests/test_security_doc_rate_limits.py +++ b/tests/test_security_doc_rate_limits.py @@ -25,6 +25,7 @@ from __future__ import annotations import ast +import inspect import re from pathlib import Path @@ -32,6 +33,7 @@ from messagefoundry.auth.service import AuthService from messagefoundry.config.settings import AuthSettings, ServiceSettings +from messagefoundry.store.base import Store _ROOT = Path(__file__).resolve().parent.parent _DOC = _ROOT / "docs" / "SECURITY.md" @@ -1054,13 +1056,99 @@ def test_protection_set_guard_detects_a_planted_omission() -> None: ) -def test_malicious_lockout_clause_is_discharged_in_the_doc() -> None: - """The lockout window auto-expires, so an attacker cannot lock an account out indefinitely.""" - block = _section(_H_SET) - lowered = " ".join(block.lower().split()) - assert "auto-expiring" in lowered or "lapsed window restarts" in lowered +def test_lockout_auto_expires_but_re_locking_is_unbounded() -> None: + """One lock releases itself; REPETITION of the lock is not bounded. The doc must carry both. + + The retired assertion (``test_malicious_lockout_clause_is_discharged_in_the_doc``) drew a + conclusion the code does not support and then passed on the token that carried it: the doc said + an attacker "cannot maliciously lock an account indefinitely", while ``_register_failure`` + restarts the counter on a lapsed window, re-locks on the next run to the threshold, and consults + nothing that accumulates across cycles. So the ceiling it claimed does not exist. Here the + unboundedness is DERIVED from the two places lock state can live (the store write's parameters + and the function's reads) and then asserted POSITIVELY against the row's own + "Threshold / window" cell, so a replacement row that says something else cannot pass. + """ + # --- derived: no lock COUNTER is persisted, so nothing can accumulate across cycles ------------ + # Signature-based on purpose. ``record_login_failure`` writes ``locked_until=?`` and CLEARS the + # lock when passed None, so any grep for a literal like "locked_until=NULL" is a SPELLING and + # would miss it (and would miss a future unlock built on the same write). + params = inspect.signature(Store.record_login_failure).parameters + keywords = {name for name, p in params.items() if p.kind is inspect.Parameter.KEYWORD_ONLY} + assert keywords == {"failed_attempts", "locked_until", "now"}, ( + f"Store.record_login_failure now takes {sorted(keywords)}. The persisted lockout state is " + "the whole basis for the 6.1.1 note that re-locking is unbounded; if a lock COUNT landed, " + "re-derive that note in the same change." + ) + + tree = ast.parse(_SERVICE.read_text(encoding="utf-8")) + register = next( + node + for node in ast.walk(tree) + if isinstance(node, ast.AsyncFunctionDef | ast.FunctionDef) + and node.name == "_register_failure" + ) + user_reads = { + node.attr + for node in ast.walk(register) + if isinstance(node, ast.Attribute) + and isinstance(node.value, ast.Name) + and node.value.id == "user" + } + assert user_reads == {"failed_attempts", "locked_until", "id"}, ( + f"_register_failure now consults {sorted(user_reads)} on the user row. If a cross-cycle " + "ceiling landed, the 6.1.1 note that re-locking is unbounded is stale." + ) + + # --- derived: the counter has exactly the two LOCAL feeders the note scopes it to -------------- + feeders = { + node.name + for node in ast.walk(tree) + if isinstance(node, ast.AsyncFunctionDef | ast.FunctionDef) + and node.name != "_register_failure" + and _calls_to(node, {"_register_failure"}) + } + assert feeders == {"_login_local", "verify_mfa"}, ( + f"the per-account lockout is now fed from {sorted(feeders)}; the 6.1.1 note scopes it to " + "LOCAL accounts, and the recovery argument below the table rests on that scope." + ) + + # --- positive: the row's Threshold / window cell states BOTH halves ---------------------------- + table = _protection_rows() + window = table[0].index("Threshold / window") + row = next(r for r in table[1:] if "lockout_minutes" in " ".join(r)) + cell = row[window].lower() + assert "lapsed window restarts" in cell, ( + "the lockout row must still state that the lock EXPIRES on its own (the true half of the " + f"retired clause); its Threshold / window cell reads {row[window]!r}." + ) + assert "unbounded" in cell and any( + verb in cell for verb in ("re-lock", "re-locks", "re-impose", "re-imposes") + ), ( + "the lockout row must state that RE-LOCKING is unbounded, in the Threshold / window cell " + f"where the window property belongs; it reads {row[window]!r}." + ) + + # --- the shipped numbers the cell quotes ------------------------------------------------------ settings = ServiceSettings().auth assert (settings.lockout_threshold, settings.lockout_minutes) == (5, 15) + # RENDERED forms with their units, not bare str(int): "15 min" contains both "5" and "15", so a + # bare-substring check on the integers passes on almost any cell and is close to a tautology. + assert f"{settings.lockout_threshold} consecutive failures" in cell, ( + f"the lockout row must state the shipped threshold with its unit; it reads {row[window]!r}." + ) + assert f"{settings.lockout_minutes} min" in cell, ( + f"the lockout row must state the shipped window with its unit; it reads {row[window]!r}." + ) + + # --- the doc's LOCAL scope, and the exact retired conclusion ---------------------------------- + block = _section(_H_SET) + assert "only **local** accounts can be locked" in block, ( + "the 6.1.1 set must scope the lockout to LOCAL accounts; the recovery argument beneath the " + "table is only true of them." + ) + assert "cannot maliciously lock an account indefinitely" not in _doc_text(), ( + "the retired clause claimed a ceiling the code does not implement." + ) def test_route_to_limiter_map_matches_the_call_sites() -> None: