diff --git a/README.md b/README.md index c0f7fcb..1b3ed33 100644 --- a/README.md +++ b/README.md @@ -129,6 +129,18 @@ within one access-token lifetime rather than one refresh-token lifetime. Keep `access_token_ttl_seconds` short for that reason: with refresh in place, a short access token costs nothing and is what bounds revocation lag. +**A credential change ends connector sessions too.** Deleting a user, an admin +password set, a self-service password change, a reset-link redemption and the +`set-password` CLI all revoke the account's browser sessions *and* its refresh +families (plus any unredeemed authorization codes); the connector must be +re-authorized. Already-issued access tokens live out their TTL. + +**Rotating a shared password ends its cookies.** A `shared_auth_` cookie +carries a keyed fingerprint of the app's shared-password hash at the time it was +minted, so after you change the hash (and restart), every cookie from the old +password is refused. Upgrading to this version signs everyone out of +shared-password apps once. + Plus environment variables: - `ENLACE_SIGNING_KEY` — signing key (32+ chars). Generate with `python -c diff --git a/enlace_auth/__main__.py b/enlace_auth/__main__.py index e5d8a1e..e1e6cf6 100644 --- a/enlace_auth/__main__.py +++ b/enlace_auth/__main__.py @@ -95,6 +95,34 @@ def _load_session_store(toml_path: Path = Path("platform.toml")): return SessionStore(factory("sessions")) +def _connector_tombstone_ttl(toml_path: Path = Path("platform.toml")) -> int: + """The refresh-family tombstone lifetime the configured OAuth server uses.""" + from enlace_auth.auth.revocation import refresh_tombstone_ttl + + osc = coerce_auth_config(PlatformConfig.from_toml(toml_path).auth).oauth_server + return refresh_tombstone_ttl( + refresh_token_ttl=osc.refresh_token_ttl_seconds, + refresh_reuse_detection=osc.refresh_reuse_detection_seconds, + ) + + +def _revoke_connector_subject(email: str, toml_path: Path) -> int: + """Revoke *email*'s connector refresh families (tombstoned); return count.""" + from enlace_auth.auth.revocation import revoke_refresh_subject + from enlace_auth.stores import make_file_store_factory + + auth = coerce_auth_config(PlatformConfig.from_toml(toml_path).auth) + factory = make_file_store_factory(auth.stores.path) + return revoke_refresh_subject( + factory("oauth_refresh_tokens"), + email, + reason="revoked by the enlace-auth CLI", + tombstone_ttl=_connector_tombstone_ttl(toml_path), + marker_ttl=auth.oauth_server.refresh_family_max_lifetime_seconds, + code_store=factory("oauth_codes"), + ) + + def _load_user_store(toml_path: Path = Path("platform.toml")): """Open the platform's user store (email -> {password_hash, ...}).""" from enlace_auth.stores import make_file_store_factory @@ -228,7 +256,20 @@ def set_password(email: str, *, toml: str = "platform.toml"): store[key] = updated # Same rule as the HTTP reset paths: a new password ends the old sessions. revoked = _load_session_store(Path(toml)).revoke_user(key) - print(f"Password updated for {key}; {revoked} existing session(s) revoked.") + try: + families = _revoke_connector_subject(key, Path(toml)) + except Exception as e: # noqa: BLE001 - the password IS changed; say what isn't + print( + f"Password updated for {key}; {revoked} existing session(s) revoked, " + f"but connector sessions were NOT revoked ({e}). Run " + f"`enlace-auth revoke-connector-session --email {key}`.", + file=sys.stderr, + ) + sys.exit(1) + print( + f"Password updated for {key}; {revoked} existing session(s) and " + f"{families} connector session(s) revoked." + ) def reset_link( @@ -413,6 +454,8 @@ def list_connector_sessions(*, json: bool = False, toml: str = "platform.toml"): continue if not record or record.get("consumed_at") is not None: continue # spent tokens are tombstones, not sessions + if not record.get("family") or not record.get("email"): + continue # revocation markers, not sessions families[record.get("family", key)] = { "family": record.get("family"), "email": record.get("email"), diff --git a/enlace_auth/admin/routes.py b/enlace_auth/admin/routes.py index fb0b09d..5479a58 100644 --- a/enlace_auth/admin/routes.py +++ b/enlace_auth/admin/routes.py @@ -86,6 +86,7 @@ def make_admin_router( reset_link_ttl: int = DEFAULT_HANDOFF_TTL, resource_allowlist: Optional[Mapping[str, list[str]]] = None, public_base_url: Optional[str] = None, + on_credentials_changed=None, # CredentialsChanged; default: sessions only ) -> APIRouter: """Build a FastAPI router exposing ``/_admin/api/*`` endpoints. @@ -115,7 +116,17 @@ def make_admin_router( (it isn't) or, worse, to make some *other* app public by analogy (which would be). Passing the allow-list lets the dashboard show who can actually reach each one. + + ``on_credentials_changed`` (``hook(email, *, keep=None)``) runs after a + user is deleted or has their password set. The default revokes the + account's browser sessions; the plugin injects one that also revokes its + OAuth connector refresh families (``enlace_auth.auth.revocation``). """ + from enlace_auth.auth.revocation import make_on_credentials_changed + + credentials_changed = on_credentials_changed or make_on_credentials_changed( + session_store + ) admin_set = frozenset(e.lower() for e in admin_emails) apps_snapshot = list(apps) app_by_name = {a.name: a for a in apps_snapshot} @@ -214,7 +225,7 @@ async def delete_user(email: str, request: Request) -> dict[str, Any]: # a deleted account keeps working until its cookie expires unless its # sessions go too. (An actor deleting themselves is logged out.) _ = actor - session_store.revoke_user(target) + credentials_changed(target) return {"ok": True, "email": target} @router.post("/users/{email}/password") @@ -234,7 +245,7 @@ async def admin_reset_password( user_store[target] = record # An admin reset is how a compromised account is recovered: whoever # holds a session opened with the old password must lose it. - session_store.revoke_user(target) + credentials_changed(target) return {"ok": True, "email": target} @router.post("/users/{email}/reset-link") diff --git a/enlace_auth/auth/middleware.py b/enlace_auth/auth/middleware.py index 221ec5a..97aa2cd 100644 --- a/enlace_auth/auth/middleware.py +++ b/enlace_auth/auth/middleware.py @@ -36,6 +36,7 @@ from urllib.parse import unquote from enlace_auth.auth.cookies import verify_cookie +from enlace_auth.auth.revocation import shared_cookie_valid from enlace_auth.auth.sessions import SessionStore _logger = logging.getLogger("enlace_auth.middleware") @@ -291,16 +292,21 @@ async def __call__(self, scope, receive, send): app_id = rule.app_id if rule is not None else "" name = f"shared_auth_{app_id}" token = cookies.get(name) - if ( - not token - or verify_cookie( + # The cookie must carry the fingerprint of the app's CURRENT shared + # password hash: rotating the password ends every older cookie, and + # an app with no configured password admits no cookie at all. + value = ( + verify_cookie( token, self._signing_key, max_age=self._max_age, salt=f"shared:{app_id}", ) - is None - ): + if token + else None + ) + expected_hash = rule.shared_password_hash if rule is not None else None + if not shared_cookie_valid(value, expected_hash, self._signing_key): return await self._deny(scope, send, "shared") state["user_id"] = "shared" await self.app(scope, receive, send) diff --git a/enlace_auth/auth/oauth_server.py b/enlace_auth/auth/oauth_server.py index 631bbd2..0c4cd59 100644 --- a/enlace_auth/auth/oauth_server.py +++ b/enlace_auth/auth/oauth_server.py @@ -57,6 +57,12 @@ from enlace_auth.auth import pages from enlace_auth.auth.cookies import sign_cookie, verify_cookie +from enlace_auth.auth.revocation import ( + refresh_tombstone_ttl, + revoke_refresh_family, + revoked_family_key, + subject_revoked_before, +) from enlace_auth.auth.sessions import SessionStore from enlace_auth.stores.validation import sanitize_key @@ -301,6 +307,10 @@ def _norm_resource(resource: str) -> str: # One flag, consulted everywhere: a store with a zero TTL is NOT refresh # support, and metadata that says otherwise mints tokens dead on arrival. _refresh_enabled = refresh_store is not None and refresh_token_ttl > 0 + _tombstone_ttl = refresh_tombstone_ttl( + refresh_token_ttl=refresh_token_ttl, + refresh_reuse_detection=refresh_reuse_detection, + ) _local_claims: set[str] = set() _local_claim_lock = threading.Lock() _client_sweep_cursor: dict = {"pos": 0} @@ -364,40 +374,9 @@ def _revoke_family(family: str, *, reason: str) -> int: """ if refresh_store is None: return 0 - # Write the marker FIRST. Revocation used to be expressed as the ABSENCE - # of records, which cannot work while another worker is concurrently - # creating them: a successor written after the scan snapshot survived, - # and a tombstone write resurrected a parent the scan had just deleted, - # so a chain that happened to be rotating defeated detection outright - # (measured at ~22% of attempts). A single positive fact cannot be raced. - now = _now() - refresh_store[_revoked_key(family)] = { - "revoked_at": now, - "reason": reason, - # Must outlive every token that could still belong to this family. - "exp": now + max(refresh_token_ttl, refresh_reuse_detection), - } - revoked = 0 - for key in list(refresh_store): - try: - record = refresh_store[key] - except KeyError: - continue - if (record or {}).get("family") != family: - continue # (the marker itself carries no "family" key) - try: - del refresh_store[key] - revoked += 1 - except KeyError: - pass - _logger.warning( - "oauth: revoked refresh family %s (%d token(s)) — %s. The connector " - "using it is now dead until a human re-authorizes it.", - family, - revoked, - reason, + return revoke_refresh_family( + refresh_store, family, reason=reason, tombstone_ttl=_tombstone_ttl ) - return revoked def _issue_refresh( *, @@ -424,6 +403,8 @@ def _issue_refresh( "scope": scope, "email": email, "iat": now, + # When the family was authorized; carried through every rotation. + "auth_at": now, "exp": now + refresh_token_ttl, "family_exp": family_exp, "consumed_at": None, @@ -439,11 +420,23 @@ def _touch_client(client_id: str, now: int) -> None: def _revoked_key(family: str) -> str: """Store key for the tombstone that marks a whole family revoked.""" - return f"family:{family}" + return revoked_family_key(family) def _family_revoked(family: Optional[str]) -> bool: return bool(family) and refresh_store.get(_revoked_key(family)) is not None + def _authorized_before_revocation(record: dict) -> bool: + """True if the subject's credentials changed after this family began.""" + revoked_before = subject_revoked_before(refresh_store, record.get("email")) + if revoked_before is None: + return False + auth_at = record.get("auth_at") + if not isinstance(auth_at, (int, float)): + # Records minted before ``auth_at`` existed: derive it from the + # absolute ceiling, which is fixed at authorization. + auth_at = (record.get("family_exp") or 0) - refresh_family_max_lifetime + return auth_at <= revoked_before + def _grace_key(key: str) -> str: """Store key for the short-lived retry copy of a successor plaintext.""" return f"grace:{key}" @@ -662,9 +655,15 @@ def _validate_authorize( ) return auth, None - def _issue_code(auth: _Authorized, email: str) -> str: + def _issue_code(auth: _Authorized, email: str, *, issued_at: int) -> str: + """Mint a code; *issued_at* is when the session was read, not written. + + A credential change between reading the session and writing the code + would otherwise miss this code (see ``subject_revoked_before``). + """ code = secrets.token_urlsafe(32) code_store[code] = { + "iat": issued_at, "client_id": auth.client_id, "email": email, "redirect_uri": auth.redirect_uri, @@ -690,6 +689,7 @@ async def authorize(request: Request): auth.redirect_uri, "invalid_request", auth.state, "PKCE S256 required" ) + session_read_at = _now() email = _current_email(request) if not email: # Reuse the platform login, returning here once authenticated. @@ -702,7 +702,7 @@ async def authorize(request: Request): return HTMLResponse(_denied_page(email), status_code=403) if not require_consent: - code = _issue_code(auth, email) + code = _issue_code(auth, email, issued_at=session_read_at) return RedirectResponse( _with_query(auth.redirect_uri, {"code": code, "state": auth.state}), status_code=302, @@ -725,6 +725,7 @@ async def authorize_consent( csrf: str = Form(...), decision: str = Form(...), ): + session_read_at = _now() email = _current_email(request) if not email: return JSONResponse({"error": "login_required"}, status_code=401) @@ -748,7 +749,7 @@ async def authorize_consent( return _redirect_error(redirect_uri, "access_denied", state) if decision != "approve": return _redirect_error(redirect_uri, "access_denied", state) - code = _issue_code(auth, email) + code = _issue_code(auth, email, issued_at=session_read_at) return RedirectResponse( _with_query(redirect_uri, {"code": code, "state": state}), status_code=302, @@ -869,6 +870,12 @@ def _grant_authorization_code( or not _verify_pkce_s256(code_verifier, data["code_challenge"]) ): return JSONResponse({"error": "invalid_grant"}, status_code=400) + # Issued before the account's credentials changed: dead, even if the + # revocation's scan could not see it (i2mint/enlace_auth#26). + revoked_before = subject_revoked_before(refresh_store, data["email"]) + code_iat = data.get("iat", data["exp"] - code_ttl) + if revoked_before is not None and code_iat <= revoked_before: + return JSONResponse({"error": "invalid_grant"}, status_code=400) body, _ = _token_payload( iss=_issuer(request), @@ -909,6 +916,11 @@ def _grant_refresh_token( # that was meant to delete it. if _family_revoked(record.get("family")): return JSONResponse({"error": "invalid_grant"}, status_code=400) + if _authorized_before_revocation(record): + _revoke_family( + record.get("family"), reason="the account's credentials changed" + ) + return JSONResponse({"error": "invalid_grant"}, status_code=400) if record.get("consumed_at") is not None: return _replayed( @@ -1037,6 +1049,7 @@ def _rotate( "scope": scope, "email": email, "iat": now, + "auth_at": record.get("auth_at"), "exp": now + refresh_token_ttl, "family_exp": family_exp, "consumed_at": None, @@ -1106,6 +1119,9 @@ def _replayed( family = record.get("family") if _family_revoked(family): return JSONResponse({"error": "invalid_grant"}, status_code=400) + if _authorized_before_revocation(record): + _revoke_family(family, reason="the account's credentials changed") + return JSONResponse({"error": "invalid_grant"}, status_code=400) within_grace = now - (record.get("consumed_at") or 0) < refresh_reuse_grace same_client = record.get("client_id") == client_id diff --git a/enlace_auth/auth/revocation.py b/enlace_auth/auth/revocation.py new file mode 100644 index 0000000..60a5f3d --- /dev/null +++ b/enlace_auth/auth/revocation.py @@ -0,0 +1,263 @@ +"""Credential revocation: end everything an account holds when its credentials change. + +An account's access outlives a password change in three places, each with its own +lifetime: browser **sessions** (``session_max_age``), OAuth connector +**refresh-token families** (``refresh_family_max_lifetime``), and the per-app +**shared-password cookies** (which are not per account at all -- see +:func:`shared_password_fingerprint`). This module is the one place that knows how +to end the first two, so every path that changes an account's credentials +(admin delete, admin password set, self-service change, reset-link redemption, +the ``set-password`` CLI) calls a single :func:`make_on_credentials_changed` hook +instead of each remembering its own list. + +Kept free of FastAPI/Authlib imports so the CLI can use it without the +``[oauth]`` extra. +""" + +from __future__ import annotations + +import hashlib +import hmac +import logging +import time +from collections.abc import MutableMapping +from typing import Any, Optional, Protocol + +_logger = logging.getLogger("enlace_auth") + +__all__ = [ + "CredentialsChanged", + "make_on_credentials_changed", + "refresh_tombstone_ttl", + "revoke_refresh_family", + "revoke_refresh_subject", + "revoked_family_key", + "subject_marker_key", + "subject_revoked_before", + "shared_cookie_valid", + "shared_password_fingerprint", +] + + +class CredentialsChanged(Protocol): + """``on_credentials_changed(email, *, keep=None) -> None``. + + *keep* names one browser session id to spare (the browser that just changed + its own password stays signed in). + """ + + def __call__(self, email: str, *, keep: Optional[str] = None) -> None: ... + + +def revoked_family_key(family: str) -> str: + """Store key of the tombstone that marks a whole refresh family revoked.""" + return f"family:{family}" + + +def subject_marker_key(email: str) -> str: + """Store key of the marker that says "nothing *email* authorized before T".""" + digest = hashlib.sha256(email.lower().encode()).hexdigest() + return f"subject:{digest}" + + +def subject_revoked_before( + refresh_store: Optional[MutableMapping[str, Any]], email: Optional[str] +) -> Optional[int]: + """The time before which every authorization by *email* is revoked, if any. + + A scan-and-delete revocation cannot see a family that another worker is + creating at that very moment (its code already consumed, its first refresh + record not yet written), nor a code issued from a session read just before + the change. The marker closes both: the code grant refuses codes issued at + or before it, and the refresh grant refuses families authorized at or + before it. + """ + if refresh_store is None or not email: + return None + try: + record = refresh_store.get(subject_marker_key(email)) + except Exception: # noqa: BLE001 - an unreadable marker is no marker + return None + value = (record or {}).get("revoked_before") if isinstance(record, dict) else None + return value if isinstance(value, (int, float)) else None + + +def refresh_tombstone_ttl( + *, refresh_token_ttl: int, refresh_reuse_detection: int +) -> int: + """How long a family tombstone must live: past every token of the family.""" + return max(refresh_token_ttl, refresh_reuse_detection) + + +def revoke_refresh_family( + refresh_store: MutableMapping[str, Any], + family: str, + *, + reason: str, + tombstone_ttl: int, + now: Optional[int] = None, +) -> int: + """Revoke one refresh-token family; return how many token records went. + + The tombstone is written FIRST: revocation expressed only as the absence of + records loses to a worker concurrently rotating the family (its successor is + written after our scan). A positive marker cannot be raced -- the refresh + grant refuses any record whose family carries one. + """ + now = int(time.time()) if now is None else now + refresh_store[revoked_family_key(family)] = { + "revoked_at": now, + "reason": reason, + "exp": now + tombstone_ttl, + } + revoked = 0 + for key in list(refresh_store): + try: + record = refresh_store[key] + except KeyError: + continue + if (record or {}).get("family") != family: + continue # (the marker itself carries no "family" key) + try: + del refresh_store[key] + revoked += 1 + except KeyError: + pass + _logger.warning( + "oauth: revoked refresh family %s (%d token(s)) — %s. The connector " + "using it is now dead until a human re-authorizes it.", + family, + revoked, + reason, + ) + return revoked + + +def revoke_refresh_subject( + refresh_store: MutableMapping[str, Any], + email: str, + *, + reason: str, + tombstone_ttl: int, + code_store: Optional[MutableMapping[str, Any]] = None, + marker_ttl: int = 0, + now: Optional[int] = None, +) -> int: + """Revoke every refresh family issued to *email*; return how many families. + + Matches the subject case-insensitively. First writes a subject marker (see + :func:`subject_revoked_before`) that lives *marker_ttl* seconds -- give it + the family max lifetime -- so authorizations racing this call are refused + too. Then tombstones each existing family and drops the subject's + unredeemed authorization codes from *code_store* when given. Access JWTs + already issued are self-contained and live out their (short) TTL. + """ + if not email: + raise ValueError("revoke_refresh_subject needs a non-empty email") + target = email.lower() + now = int(time.time()) if now is None else now + refresh_store[subject_marker_key(target)] = { + "revoked_before": now, + "reason": reason, + "exp": now + max(marker_ttl, tombstone_ttl), + } + + def _is_subjects(record: Any) -> bool: + subject = (record or {}).get("email") if isinstance(record, dict) else None + return isinstance(subject, str) and subject.lower() == target + + families: set[str] = set() + for key in list(refresh_store): + try: + record = refresh_store[key] + except KeyError: + continue + if _is_subjects(record) and record.get("family"): + families.add(record["family"]) + for family in sorted(families): + revoke_refresh_family( + refresh_store, family, reason=reason, tombstone_ttl=tombstone_ttl, now=now + ) + if code_store is not None: + for key in list(code_store): + try: + record = code_store[key] + except KeyError: + continue + if _is_subjects(record): + try: + del code_store[key] + except KeyError: + pass + return len(families) + + +def make_on_credentials_changed( + session_store, + *, + refresh_store: Optional[MutableMapping[str, Any]] = None, + code_store: Optional[MutableMapping[str, Any]] = None, + tombstone_ttl: int = 0, + marker_ttl: int = 0, + reason: str = "the account's credentials changed", +) -> CredentialsChanged: + """Return the hook every credential-changing path calls. + + Always revokes the account's browser sessions (sparing *keep*); when a + *refresh_store* is given, also revokes the account's connector refresh + families (and unredeemed codes in *code_store*). A failure to revoke + connector families is logged loudly but does not undo the password change, + which has already been written by the caller. + """ + if refresh_store is not None and tombstone_ttl <= 0: + raise ValueError("tombstone_ttl must be positive when refresh_store is given") + + def on_credentials_changed(email: str, *, keep: Optional[str] = None) -> None: + # Connector revocation runs even if the session revocation fails, and + # vice versa; a session-store failure still surfaces to the caller. + try: + session_store.revoke_user(email, keep=keep) + finally: + if refresh_store is not None: + try: + revoke_refresh_subject( + refresh_store, + email, + reason=reason, + tombstone_ttl=tombstone_ttl, + marker_ttl=marker_ttl, + code_store=code_store, + ) + except Exception: # noqa: BLE001 - say so loudly, don't mask + _logger.exception( + "enlace_auth: could not revoke connector sessions for %r " + "after a credential change; revoke them with " + "`enlace-auth revoke-connector-session --email`", + email, + ) + + return on_credentials_changed + + +def shared_password_fingerprint(password_hash: str, signing_key: str) -> str: + """A short keyed fingerprint of an app's CURRENT shared-password hash. + + Signed into the ``shared_auth_`` cookie and compared by the middleware, + so rotating the shared password invalidates every cookie minted under the + old one. Keyed with *signing_key* so the cookie (whose payload is readable, + only signed) reveals nothing about the hash. + """ + digest = hmac.new( + signing_key.encode(), f"shared-pw:{password_hash}".encode(), hashlib.sha256 + ) + return digest.hexdigest()[:32] + + +def shared_cookie_valid( + value: Optional[str], password_hash: Optional[str], signing_key: str +) -> bool: + """True iff a verified shared-cookie *value* was minted under *password_hash*.""" + if not value or not password_hash: + return False + expected = shared_password_fingerprint(password_hash, signing_key) + return hmac.compare_digest(str(value), expected) diff --git a/enlace_auth/auth/routes.py b/enlace_auth/auth/routes.py index 9a3e065..e4009d4 100644 --- a/enlace_auth/auth/routes.py +++ b/enlace_auth/auth/routes.py @@ -38,6 +38,12 @@ reset_url, verify_reset_token, ) +from enlace_auth.auth.revocation import ( + CredentialsChanged, + make_on_credentials_changed, + shared_cookie_valid, + shared_password_fingerprint, +) from enlace_auth.auth.sessions import SessionStore @@ -87,6 +93,7 @@ def make_auth_router( send_email: Optional[EmailSender] = None, reset_token_max_age: int = DEFAULT_EMAIL_TTL, public_base_url: Optional[str] = None, + on_credentials_changed: Optional[CredentialsChanged] = None, ) -> APIRouter: """Build a FastAPI router exposing ``/auth/*`` endpoints. @@ -109,8 +116,16 @@ def make_auth_router( ``Host`` header, which the requester controls -- a forged ``Host`` would mail the victim a link that hands their reset token to another site, unless a proxy in front only forwards known hosts. + on_credentials_changed: ``hook(email, *, keep=None)`` called after a + password change or reset. Defaults to revoking the account's + browser sessions only; the plugin injects one that also revokes the + account's OAuth connector refresh families (see + ``enlace_auth.auth.revocation``). """ router = APIRouter(prefix="/auth") + credentials_changed: CredentialsChanged = ( + on_credentials_changed or make_on_credentials_changed(session_store) + ) # Distinguish "no delivery channel configured" from "a sender was wired": # the page copy must not promise an email the platform cannot send. email_delivery_configured = send_email is not None @@ -230,7 +245,13 @@ async def shared_login_page(request: Request): status_code=404, ) existing = request.cookies.get(f"shared_auth_{app}") - if existing and verify_cookie(existing, signing_key, salt=f"shared:{app}"): + if existing and shared_cookie_valid( + verify_cookie( + existing, signing_key, max_age=session_max_age, salt=f"shared:{app}" + ), + shared_password_for(app), + signing_key, + ): return RedirectResponse(next_url, status_code=303) return HTMLResponse(pages.render_shared_login_page(app=app, next_url=next_url)) @@ -243,7 +264,13 @@ async def shared_login( raise HTTPException(status_code=404, detail=f"Unknown app '{body.app}'") if not verify_password(stored_hash, body.password): raise HTTPException(status_code=401, detail="Invalid password") - token = sign_cookie("1", signing_key, salt=f"shared:{body.app}") + # The cookie carries a fingerprint of the CURRENT password hash, so + # rotating the shared password ends every cookie minted under the old. + token = sign_cookie( + shared_password_fingerprint(stored_hash, signing_key), + signing_key, + salt=f"shared:{body.app}", + ) cookie_name_shared = f"shared_auth_{body.app}" attrs = [ f"{cookie_name_shared}={token}", @@ -291,7 +318,7 @@ async def change_own_password( user_store[email] = record # Log out every OTHER browser holding this account: a password change # is what a user does when they suspect someone else is signed in. - session_store.revoke_user(email, keep=_current_session_id(request)) + credentials_changed(email, keep=_current_session_id(request)) return {"ok": True, "email": email} # ----- Password recovery --------------------------------------------- @@ -425,7 +452,7 @@ async def password_reset_confirm( user_store[email] = record # A reset is the recovery path for a compromised account, so every # session opened with the old password ends here. - session_store.revoke_user(email) + credentials_changed(email) session_id = session_store.create(user_id=email, email=email) _set_session_cookie( response, diff --git a/enlace_auth/auth/sessions.py b/enlace_auth/auth/sessions.py index 0c0b6f6..17ae540 100644 --- a/enlace_auth/auth/sessions.py +++ b/enlace_auth/auth/sessions.py @@ -2,6 +2,11 @@ A session is ``{"user_id": str, "email": str | None, "created_at": float}``. Session IDs are 32-byte urlsafe tokens. Revocation is a simple delete. + +Records are otherwise only deleted by logout, so given a *max_age* the store +sweeps records older than it, a bounded batch at a time, when a session is +created (at most once per *sweep_interval* per process) -- keeping the +store (and :meth:`SessionStore.revoke_user`'s scan) from growing without bound. """ from __future__ import annotations @@ -9,16 +14,63 @@ import secrets import time from collections.abc import MutableMapping +from itertools import islice from typing import Any, Optional class SessionStore: """Thin adapter around a MutableMapping that speaks session semantics.""" - def __init__(self, store: MutableMapping): + def __init__( + self, + store: MutableMapping, + *, + max_age: Optional[float] = None, + sweep_batch: int = 100, + sweep_interval: float = 3600.0, + ): self._store = store + self._max_age = max_age + self._sweep_batch = sweep_batch + self._sweep_interval = sweep_interval + self._sweep_pos = 0 + self._last_sweep = float("-inf") + + def sweep_expired(self, *, now: Optional[float] = None) -> int: + """Delete up to *sweep_batch* records older than *max_age*; return count. + + No-op without a *max_age*. A cursor carries the position across calls + (wrapping at the end) so successive sweeps walk the whole store. A + record without a numeric ``created_at`` is left alone. + """ + if not self._max_age: + return 0 + now = time.time() if now is None else now + start = self._sweep_pos + keys = list(islice(iter(self._store), start, start + self._sweep_batch)) + self._sweep_pos = 0 if len(keys) < self._sweep_batch else start + len(keys) + removed = 0 + for sid in keys: + try: + record = self._store[sid] + except KeyError: + continue + created = record.get("created_at") if isinstance(record, dict) else None + if isinstance(created, (int, float)) and now - created > self._max_age: + if self.delete(sid): + removed += 1 + return removed def create(self, user_id: str, email: Optional[str] = None) -> str: + # Throttled: on a file store, iterating to the cursor walks the + # directory, which must not be paid on every login. + now = time.time() + if now - self._last_sweep >= self._sweep_interval: + self._last_sweep = now + try: + self.sweep_expired(now=now) + except Exception: # noqa: BLE001 - housekeeping must never block a login + pass session_id = secrets.token_urlsafe(32) self._store[session_id] = { "user_id": user_id, diff --git a/enlace_auth/plugin.py b/enlace_auth/plugin.py index 360905f..f148c1a 100644 --- a/enlace_auth/plugin.py +++ b/enlace_auth/plugin.py @@ -334,7 +334,9 @@ def wire(parent: "FastAPI", config) -> None: platform_factory = make_file_store_factory(auth_cfg.stores.path) session_backend = platform_factory("sessions") user_backend = platform_factory("users") - session_store = SessionStore(session_backend) + session_store = SessionStore( + session_backend, max_age=auth_cfg.session_max_age_seconds + ) # Runtime per-app access grants (additive, optional UTC expiry). Lives beside # sessions/ and users/ under the same persistent store root — outside the @@ -417,6 +419,38 @@ def wire(parent: "FastAPI", config) -> None: # beneath a more specific prefix via longest-prefix match. access_rules.append(AccessRule(prefix="/", level="public", app_id="_root")) + # One hook for every path that changes an account's credentials: browser + # sessions always go; the account's OAuth connector refresh families go too + # when the OAuth server keeps them (i2mint/enlace_auth#26). + from enlace_auth.auth.revocation import ( + make_on_credentials_changed, + refresh_tombstone_ttl, + ) + + osc = auth_cfg.oauth_server + _connector_revocation = ( + dict( + refresh_store=platform_factory("oauth_refresh_tokens"), + code_store=platform_factory("oauth_codes"), + tombstone_ttl=refresh_tombstone_ttl( + refresh_token_ttl=osc.refresh_token_ttl_seconds, + refresh_reuse_detection=osc.refresh_reuse_detection_seconds, + ), + marker_ttl=osc.refresh_family_max_lifetime_seconds, + ) + # Wired even while the OAuth server is off: families minted before it + # was disabled come back to life if it is re-enabled. + if refresh_tombstone_ttl( + refresh_token_ttl=osc.refresh_token_ttl_seconds, + refresh_reuse_detection=osc.refresh_reuse_detection_seconds, + ) + > 0 + else {} + ) + on_credentials_changed = make_on_credentials_changed( + session_store, **_connector_revocation + ) + auth_router = make_auth_router( session_store=session_store, user_store=user_backend, @@ -428,6 +462,7 @@ def wire(parent: "FastAPI", config) -> None: can_register=can_register, send_email=email_sender, public_base_url=_public_base_url(config, auth_cfg), + on_credentials_changed=on_credentials_changed, ) parent.include_router(auth_router) @@ -559,6 +594,7 @@ def wire(parent: "FastAPI", config) -> None: signing_key=signing_key, resource_allowlist=auth_cfg.oauth_server.resource_allowlist, public_base_url=_public_base_url(config, auth_cfg), + on_credentials_changed=on_credentials_changed, ) parent.include_router(admin_router) if admin_emails: diff --git a/tests/test_auth_middleware.py b/tests/test_auth_middleware.py index 054808f..5f731bb 100644 --- a/tests/test_auth_middleware.py +++ b/tests/test_auth_middleware.py @@ -203,12 +203,45 @@ def test_longest_prefix_wins(): def test_protected_shared_with_valid_cookie(): + # A valid shared cookie carries the fingerprint of the app's CURRENT + # shared-password hash (i2mint/enlace_auth#26), not a constant. + from enlace_auth.auth.revocation import shared_password_fingerprint + + rule = AccessRule( + prefix="/s", level="protected:shared", app_id="s", shared_password_hash="h1" + ) + mw = _make_mw([rule]) + token = sign_cookie( + shared_password_fingerprint("h1", SIGNING_KEY), SIGNING_KEY, salt="shared:s" + ) + cap = _Capture() + _run(mw(_http_scope("/s/page", {"shared_auth_s": token}), cap.receive, cap.send)) + assert cap.status() == 200 + + +def test_protected_shared_cookie_minted_under_another_password_denied(): + from enlace_auth.auth.revocation import shared_password_fingerprint + + rule = AccessRule( + prefix="/s", level="protected:shared", app_id="s", shared_password_hash="h2" + ) + mw = _make_mw([rule]) + for value in ("1", shared_password_fingerprint("h1", SIGNING_KEY)): + token = sign_cookie(value, SIGNING_KEY, salt="shared:s") + cap = _Capture() + _run( + mw(_http_scope("/s/page", {"shared_auth_s": token}), cap.receive, cap.send) + ) + assert cap.status() == 401, value + + +def test_protected_shared_without_configured_password_admits_no_cookie(): rule = AccessRule(prefix="/s", level="protected:shared", app_id="s") mw = _make_mw([rule]) token = sign_cookie("1", SIGNING_KEY, salt="shared:s") cap = _Capture() _run(mw(_http_scope("/s/page", {"shared_auth_s": token}), cap.receive, cap.send)) - assert cap.status() == 200 + assert cap.status() == 401 def test_protected_shared_without_cookie_denied(): diff --git a/tests/test_credential_revocation.py b/tests/test_credential_revocation.py new file mode 100644 index 0000000..9716faa --- /dev/null +++ b/tests/test_credential_revocation.py @@ -0,0 +1,392 @@ +"""Credential changes end connector sessions and shared-password cookies too. + +i2mint/enlace_auth#26: #25 made a password change revoke an account's browser +sessions; these tests pin the two credential kinds it left alive -- the +account's OAuth connector refresh-token families, and ``shared_auth_`` +cookies minted under a shared password that has since been rotated -- plus the +bounded session sweep. +""" + +from __future__ import annotations + +import time + +import pytest +from enlace.base import PlatformConfig +from enlace.compose import build_backend +from enlace.discover import discover_apps +from starlette.testclient import TestClient + +from enlace_auth import plugin as auth_plugin +from enlace_auth.auth import hash_password +from enlace_auth.auth.revocation import ( + make_on_credentials_changed, + revoke_refresh_subject, + revoked_family_key, + shared_cookie_valid, + shared_password_fingerprint, +) +from enlace_auth.auth.sessions import SessionStore +from enlace_auth.stores import make_file_store_factory +from tests.test_admin import ( + _SIGNING_KEY, + _csrf, + _login, + _register, + _write_dummy_app, +) +from tests.test_auth_e2e import _make_apps + + +def _refresh_record(family: str, email: str) -> dict: + now = int(time.time()) + return { + "family": family, + "client_id": "c", + "resource": "https://x/api/mcp", + "scope": "mcp:read", + "email": email, + "iat": now, + "exp": now + 3600, + "family_exp": now + 7200, + "consumed_at": None, + "successor": None, + } + + +# ---- unit: revocation helpers ------------------------------------------------ + + +def test_revoke_refresh_subject_tombstones_only_that_subjects_families(): + store = { + "t1": _refresh_record("f1", "Vic@Example.com"), + "t2": {**_refresh_record("f1", "vic@example.com"), "consumed_at": 1}, + "t3": _refresh_record("f2", "vic@example.com"), + "t4": _refresh_record("f3", "bob@example.com"), + } + codes = { + "c1": {"email": "VIC@example.com", "exp": 1}, + "c2": {"email": "bob@example.com", "exp": 1}, + } + n = revoke_refresh_subject( + store, "vic@EXAMPLE.com", reason="t", tombstone_ttl=100, code_store=codes + ) + assert n == 2 + assert revoked_family_key("f1") in store and revoked_family_key("f2") in store + live = {k for k, v in store.items() if "family" in v} + assert live == {"t4"}, "bob's family must survive" + assert set(codes) == {"c2"}, "vic's unredeemed codes must go" + + +def test_revoke_refresh_subject_rejects_empty_email(): + with pytest.raises(ValueError): + revoke_refresh_subject({}, "", reason="t", tombstone_ttl=1) + + +def test_hook_revokes_sessions_and_families_and_spares_keep(): + sessions = SessionStore({}) + keep = sessions.create("vic@example.com", "vic@example.com") + other = sessions.create("vic@example.com", "vic@example.com") + refresh = {"t1": _refresh_record("f1", "vic@example.com")} + hook = make_on_credentials_changed( + sessions, refresh_store=refresh, tombstone_ttl=10 + ) + hook("vic@example.com", keep=keep) + assert sessions.get(keep) is not None + assert sessions.get(other) is None + assert "t1" not in refresh and revoked_family_key("f1") in refresh + + +def test_hook_requires_a_tombstone_ttl_with_a_refresh_store(): + with pytest.raises(ValueError): + make_on_credentials_changed(SessionStore({}), refresh_store={}) + + +def test_shared_cookie_fingerprint_binds_the_current_hash(): + fp = shared_password_fingerprint("h1", "k") + assert shared_cookie_valid(fp, "h1", "k") + assert not shared_cookie_valid(fp, "h2", "k") + assert not shared_cookie_valid("1", "h1", "k"), "legacy constant cookie" + assert not shared_cookie_valid(fp, None, "k") + assert "h1" not in fp + + +def test_session_sweep_drops_only_expired_records(): + backend: dict = {} + sessions = SessionStore(backend, max_age=60, sweep_batch=10) + backend["old"] = {"user_id": "a", "email": "a", "created_at": time.time() - 120} + backend["legacy"] = {"user_id": "a", "email": "a"} # no created_at: kept + fresh = sessions.create("b", "b") # create() sweeps + assert "old" not in backend + assert "legacy" in backend and fresh in backend + + +def test_session_sweep_is_bounded_and_walks_the_whole_store(): + backend = { + f"s{i}": {"user_id": "a", "created_at": time.time() - 999} for i in range(25) + } + sessions = SessionStore(backend, max_age=60, sweep_batch=10) + assert sessions.sweep_expired() == 10 + while backend: + assert sessions.sweep_expired() <= 10 + assert not backend + + +def test_session_sweep_is_a_noop_without_max_age(): + backend = {"old": {"user_id": "a", "created_at": 0}} + SessionStore(backend).create("b") + assert "old" in backend + + +# ---- end to end through the plugin ------------------------------------------- + + +@pytest.fixture +def platform(tmp_path, monkeypatch): + """A gateway with the OAuth server (and so refresh families) enabled.""" + apps_dir = tmp_path / "apps" + apps_dir.mkdir() + _write_dummy_app(apps_dir) + monkeypatch.setenv("ENLACE_SIGNING_KEY", _SIGNING_KEY) + monkeypatch.setenv("ENLACE_ADMIN_EMAILS", "boss@example.com") + store_root = tmp_path / "platform" + config = PlatformConfig( + apps_dir=apps_dir, + auth={ + "enabled": True, + "secure_cookies": False, + "registration_open": True, + "stores": {"backend": "file", "path": str(store_root)}, + "oauth_server": {"enabled": True, "key_dir": str(tmp_path / "keys")}, + }, + ) + app = build_backend(discover_apps(config), plugins=[auth_plugin]) + factory = make_file_store_factory(str(store_root)) + return TestClient(app), factory("oauth_refresh_tokens"), factory("oauth_codes") + + +def _seed(refresh, codes): + refresh["tok-vic"] = _refresh_record("fvic", "vic@example.com") + refresh["tok-bob"] = _refresh_record("fbob", "bob@example.com") + codes["code-vic"] = {"email": "vic@example.com", "exp": int(time.time()) + 60} + + +def _families_left(refresh) -> set: + out = set() + for k in refresh: + rec = refresh[k] + if "family" in rec: + out.add(rec["family"]) + return out + + +def test_admin_password_set_revokes_connector_families(platform): + client, refresh, codes = platform + csrf = _csrf(client) + _register(client, "boss@example.com", "bosspw1!", csrf) + r = client.post( + "/_admin/api/users", + json={"email": "vic@example.com", "password": "victim-pw1"}, + headers=csrf, + ) + assert r.status_code == 200, r.text + _seed(refresh, codes) + r = client.post( + "/_admin/api/users/vic@example.com/password", + json={"password": "brand-new-pw1"}, + headers=csrf, + ) + assert r.status_code == 200, r.text + assert _families_left(refresh) == {"fbob"} + assert refresh.get(revoked_family_key("fvic")) is not None + assert "code-vic" not in codes + + +def test_admin_delete_revokes_connector_families(platform): + client, refresh, codes = platform + csrf = _csrf(client) + _register(client, "boss@example.com", "bosspw1!", csrf) + client.post( + "/_admin/api/users", + json={"email": "vic@example.com", "password": "victim-pw1"}, + headers=csrf, + ) + _seed(refresh, codes) + r = client.delete("/_admin/api/users/vic@example.com", headers=csrf) + assert r.status_code == 200, r.text + assert _families_left(refresh) == {"fbob"} + + +def test_self_service_password_change_revokes_connector_families(platform): + client, refresh, codes = platform + csrf = _csrf(client) + _register(client, "vic@example.com", "victim-pw1", csrf) + assert _login(client, "vic@example.com", "victim-pw1", csrf).status_code == 200 + _seed(refresh, codes) + r = client.post( + "/auth/me/password", + json={"old_password": "victim-pw1", "new_password": "brand-new-pw1"}, + headers=_csrf(client), + ) + assert r.status_code == 200, r.text + assert _families_left(refresh) == {"fbob"} + assert client.get("/auth/whoami").json()["email"] == "vic@example.com", ( + "the browser that changed its own password stays signed in" + ) + + +def _shared_platform(tmp_path, monkeypatch, password: str) -> TestClient: + apps_dir = tmp_path / "apps" + apps_dir.mkdir(exist_ok=True) + if not (apps_dir / "shared_app").exists(): + _make_apps(apps_dir) + monkeypatch.setenv("ENLACE_SIGNING_KEY", _SIGNING_KEY) + monkeypatch.setenv("SHARED_APP_PW", hash_password(password)) + config = PlatformConfig( + apps_dir=apps_dir, + auth={ + "enabled": True, + "secure_cookies": False, + "stores": {"backend": "file", "path": str(tmp_path / "platform")}, + }, + ) + return TestClient(build_backend(discover_apps(config), plugins=[auth_plugin])) + + +def test_rotating_a_shared_password_ends_old_cookies(tmp_path, monkeypatch): + before = _shared_platform(tmp_path, monkeypatch, "open-sesame") + csrf = _csrf_shared(before) + r = before.post( + "/auth/shared-login", + json={"app": "shared_app", "password": "open-sesame"}, + headers=csrf, + ) + assert r.status_code == 200, r.text + cookie = before.cookies.get("shared_auth_shared_app") + assert cookie + assert before.get("/api/shared_app/peek").status_code == 200 + + # Restart with a rotated password: the old cookie must no longer work. + after = _shared_platform(tmp_path, monkeypatch, "new-sesame") + after.cookies.set("shared_auth_shared_app", cookie) + assert after.get("/api/shared_app/peek").status_code == 401 + + +def _csrf_shared(client: TestClient) -> dict: + from enlace_auth.auth.cookies import verify_cookie + + client.get("/api/public_app/ping") + raw = verify_cookie(client.cookies.get("enlace_csrf"), _SIGNING_KEY, salt="csrf") + return {"X-CSRF-Token": raw} + + +# ---- races the scan cannot see (review of #29) ------------------------------- + + +def _pkce_pair(): + import base64 + import hashlib + + verifier = "v" * 64 + challenge = ( + base64.urlsafe_b64encode(hashlib.sha256(verifier.encode()).digest()) + .rstrip(b"=") + .decode() + ) + return verifier, challenge + + +def _revoke_vic_via_admin(client): + csrf = _csrf(client) + _register(client, "boss@example.com", "bosspw1!", csrf) + client.post( + "/_admin/api/users", + json={"email": "vic@example.com", "password": "victim-pw1"}, + headers=csrf, + ) + r = client.post( + "/_admin/api/users/vic@example.com/password", + json={"password": "brand-new-pw1"}, + headers=csrf, + ) + assert r.status_code == 200, r.text + + +def test_family_created_during_revocation_cannot_refresh(platform): + """A family written after the scan (code already consumed) is still dead.""" + from enlace_auth.auth.oauth_server import _hash_refresh + + client, refresh, _codes = platform + _revoke_vic_via_admin(client) + # The racing code grant read the clock before the revocation and wrote its + # family after the scan. + rec = _refresh_record("frace", "vic@example.com") + rec["auth_at"] = int(time.time()) - 5 + refresh[_hash_refresh("racer")] = rec + r = client.post( + "/auth/oauth/token", + data={ + "grant_type": "refresh_token", + "client_id": "c", + "refresh_token": "racer", + }, + ) + assert r.status_code == 400 + assert refresh.get(revoked_family_key("frace")) is not None + + +def test_family_authorized_after_revocation_still_refreshes(platform): + from enlace_auth.auth.oauth_server import _hash_refresh + + client, refresh, _codes = platform + _revoke_vic_via_admin(client) + rec = _refresh_record("flater", "vic@example.com") + rec["auth_at"] = int(time.time()) + 5 + refresh[_hash_refresh("later")] = rec + r = client.post( + "/auth/oauth/token", + data={ + "grant_type": "refresh_token", + "client_id": "c", + "refresh_token": "later", + }, + ) + assert r.status_code == 200, r.text + + +def test_code_issued_before_revocation_but_written_after_is_refused(platform): + client, _refresh, codes = platform + _revoke_vic_via_admin(client) + verifier, challenge = _pkce_pair() + now = int(time.time()) + codes["late-code"] = { + "iat": now - 5, # session read before the change, code written after + "client_id": "c", + "email": "vic@example.com", + "redirect_uri": "http://localhost/cb", + "code_challenge": challenge, + "scope": "mcp:read", + "resource": "https://x/api/mcp", + "exp": now + 60, + } + r = client.post( + "/auth/oauth/token", + data={ + "grant_type": "authorization_code", + "client_id": "c", + "code": "late-code", + "redirect_uri": "http://localhost/cb", + "code_verifier": verifier, + }, + ) + assert r.status_code == 400 + assert r.json()["error"] == "invalid_grant" + + +def test_session_sweep_is_throttled(): + backend: dict = {} + sessions = SessionStore(backend, max_age=60, sweep_interval=3600) + sessions.create("a") + backend["old"] = {"user_id": "a", "created_at": time.time() - 999} + sessions.create("b") + assert "old" in backend, "second create within the interval must not sweep"