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: