diff --git a/backend/core/access_identity.py b/backend/core/access_identity.py new file mode 100644 index 00000000..d3994ee1 --- /dev/null +++ b/backend/core/access_identity.py @@ -0,0 +1,368 @@ +"""Verifying that a request really was authenticated by Cloudflare Access. + +Cloudflare puts a signed assertion in `Cf-Access-Jwt-Assertion` on every request +it lets through. This turns that into an email address, or into nothing. + +## Why verify at all, when Access sits in front + +In normal operation nothing unauthenticated reaches the admin hostnames. This is +for the case where that stops being true. An Access application deleted, renamed +or misconfigured leaves the hostname open and the backend happily serving it, and +nothing else in the stack would notice. + +It also does work no edge check can: the same app answers on every vhost, so an +assertion is the only thing that distinguishes an administrator from any caller +who found `api.retina.fm`. + +## What is checked, and why each one matters + + signature against the team's published keys. Without it the header is a + claim anyone can type. + audience the tag of *this environment's* Access application. Without it a + token minted for any other application in the team is accepted, + and the team runs Access on seventeen node hostnames. + issuer the team domain, so a valid token from another Cloudflare team + is not enough. + expiry with a little leeway for clock drift. + +Missing any one of those turns verification into decoration. The audience is the +one most easily left out, because a token that fails it still has a perfectly +good signature. +""" + +import asyncio +import logging + +import httpx +import jwt +from jwt import PyJWKSet + +log = logging.getLogger(__name__) + +#: Cloudflare rotates signing keys, so a cached set goes stale. An unrecognised +#: key id refetches immediately regardless, so this is a ceiling on staleness +#: rather than the mechanism that handles rotation. +JWKS_TTL_SECONDS = 3600 + +#: Rejecting a freshly issued token because a clock is three seconds behind +#: would be a confusing way to fail. +CLOCK_LEEWAY_SECONDS = 30 + +#: Floor on refetching after an unrecognised key id. `kid` is read from the +#: unverified header, so anyone can name one without signing anything, and +#: without this floor each such request costs an outbound fetch to Cloudflare +#: made while holding the lock every admin request waits on. The cost is that a +#: genuine rotation is picked up up to this late, which is short beside both the +#: hour-long TTL and Cloudflare's real rotation cadence. +JWKS_REFETCH_MIN_INTERVAL_SECONDS = 60 + +#: How long a held key set keeps being used once refreshing it starts failing. +#: +#: Cloudflare Access at the edge and the endpoint publishing these keys are +#: different systems, so the outage worth designing for is the one where Access +#: admits the team normally and only the refresh fails. Refusing then would take +#: the admin console down on our own account, during someone else's partial +#: outage, using keys we are still holding and which would almost certainly +#: verify every live assertion. +#: +#: Bounded rather than indefinite: long past any plausible outage, short beside +#: Cloudflare's rotation cadence of weeks, so a genuinely retired key can never +#: be honoured for anything approaching a rotation cycle. +JWKS_GRACE_SECONDS = 6 * 3600 + +#: One refresh attempt per interval once one has failed, so a failing endpoint +#: is not retried by every request in turn while holding the lock. +JWKS_RETRY_INTERVAL_SECONDS = 30 + +#: One outage line per interval. The grace window is hours long and the request +#: rate is not ours to set, so a line each would bury what it reports in the +#: logs somebody is reading during the outage. Once a minute is ~360 lines +#: across the whole window: unmissable, and not a flood. +JWKS_WARN_INTERVAL_SECONDS = 60 + +#: Everything a fetch can fail with. httpx covers the network and the status; +#: PyJWKSetError arrives as PyJWTError, and a body that is not JSON as +#: ValueError. Named once so the fallback path and the caller agree on it. +_FETCH_ERRORS = (httpx.HTTPError, jwt.PyJWTError, ValueError, KeyError) + + +class _KeysUnavailable(Exception): + """There is no key set fit to judge a token against. + + Raised rather than returned so it cannot be confused with "this token's key + id is not among the published keys", which is a different fault, with a + different cause and a different fix. Whoever raises this has already said + why, through `_warn_outage`, so `identity` refuses without adding a line. + """ + + +def _short(value): + """Enough of an identifier to match against, not enough to fill a line.""" + value = str(value or "") + return value if len(value) <= 12 else value[:12] + "..." + + +def _kid(token): + """The key id a token claims, for the log only. + + Read without verifying anything, which is safe precisely because the caller + has already decided to refuse: this only ever describes a rejection. + """ + try: + return _short(jwt.get_unverified_header(token).get("kid")) + except Exception: + return "unreadable" + + +def _claim(token, name): + """One unverified claim, for explaining a rejection. Never decides anything.""" + try: + value = jwt.decode(token, options={"verify_signature": False}).get(name) + except Exception: + return "unreadable" + if isinstance(value, list): + return [_short(v) for v in value] + return _short(value) + + +class AccessIdentity: + """Turns a Cloudflare Access assertion into a verified email address.""" + + def __init__(self, team_domain=None, audience=None, ttl=JWKS_TTL_SECONDS, client=None): + self.team_domain = (team_domain or "").strip() + self.audience = (audience or "").strip() + self.ttl = ttl + self._client = client + self._jwks = None + self._fetched_at = 0.0 + # -inf so the first attempt is never mistaken for one inside a backoff + # window, whatever epoch the loop's monotonic clock happens to use. + self._failed_at = float("-inf") + self._last_error = None + self._warned_at = float("-inf") + # Requests are served concurrently; without this two arriving together + # could each fetch and install a key set the other had moved past. + self._lock = asyncio.Lock() + + def is_configured(self): + return bool(self.team_domain and self.audience) + + # ── signing keys ───────────────────────────────────────────── + + async def _fetch_jwks(self): + if self._client is None: + self._client = httpx.AsyncClient() + url = f"https://{self.team_domain}/cdn-cgi/access/certs" + response = await self._client.get(url, timeout=10) + response.raise_for_status() + return PyJWKSet.from_dict(response.json()) + + async def _refresh(self): + self._jwks = await self._fetch_jwks() + self._fetched_at = asyncio.get_running_loop().time() + self._last_error = None + + async def _try_refresh(self, now): + """Refresh, remembering a failure rather than raising it. + + The error is kept so the caller can re-raise it where there is nothing to + fall back on, which is what lets `identity` tell an outage apart from a + forged key id in the log. + """ + try: + await self._refresh() + return True + except _FETCH_ERRORS as exc: + self._failed_at = now + self._last_error = exc + return False + + def _warn_outage(self, now, message, *args): + """One outage line per interval, however many requests arrive. + + Shared by both outage messages rather than one throttle each: they + describe the same failing endpoint, and the transition between them + happens once, so a line arriving up to an interval late costs nothing + against a condition that is already hours old. + """ + if now - self._warned_at < JWKS_WARN_INTERVAL_SECONDS: + return + self._warned_at = now + log.warning(message, *args) + + async def _usable_keys(self, now): + """The key set to judge this request against. + + Refreshed once it has aged past the TTL. While that refresh is failing + the held set keeps being used, for a bounded grace: Access at the edge + and the endpoint publishing these keys are different systems, so + refusing here would take the console down over an outage in the other + one, using keys we are holding that almost certainly still verify every + live assertion. + """ + if self._jwks is not None and now - self._fetched_at <= self.ttl: + return self._jwks + + if now - self._failed_at >= JWKS_RETRY_INTERVAL_SECONDS and await self._try_refresh(now): + return self._jwks + + if self._jwks is None: + self._warn_outage( + now, + "Refusing every Access assertion: could not reach %s for signing keys and none are held: %s", + self.team_domain, + self._last_error, + ) + raise _KeysUnavailable + + age = now - self._fetched_at + if age > self.ttl + JWKS_GRACE_SECONDS: + self._warn_outage( + now, + "Refusing every Access assertion: %s's signing keys are %.1f hours " + "old, past the grace, and could not reach it to refresh them: %s", + self.team_domain, + age / 3600, + self._last_error, + ) + raise _KeysUnavailable + + self._warn_outage( + now, + "Verifying against signing keys fetched %.0f minutes ago: refreshing " + "them from %s is failing. This holds until they are %.0f hours old, " + "after which every assertion is refused.", + age / 60, + self.team_domain, + (self.ttl + JWKS_GRACE_SECONDS) / 3600, + ) + return self._jwks + + def _lookup(self, kid): + try: + return self._jwks[kid] + except KeyError: + return None + + async def _signing_key(self, token): + """The key this token was signed with, refetching once if it is new. + + An unrecognised key id means either a rotation we have not seen or a + forgery, and asking Cloudflare again tells them apart. Asking is floored + rather than done per request, because the key id comes from the + unverified header and the fetch happens while holding the lock every + other request waits on. + """ + kid = jwt.get_unverified_header(token).get("kid") + if not kid: + return None + + async with self._lock: + now = asyncio.get_running_loop().time() + keys = await self._usable_keys(now) + + try: + return keys[kid] + except KeyError: + pass + + # Only worth asking again if what we hold is old enough that + # Cloudflare could plausibly have published something since. Fresh + # means the answer would be the set we just read. + if now - self._fetched_at < JWKS_REFETCH_MIN_INTERVAL_SECONDS: + return None + if now - self._failed_at < JWKS_RETRY_INTERVAL_SECONDS: + return None + if not await self._try_refresh(now): + return None + return self._lookup(kid) + + # ── the answer ─────────────────────────────────────────────── + + async def identity(self, token): + """The verified email address, or None. + + None covers every way this can fail: no configuration, no token, a bad + signature, the wrong audience, the wrong team, an expired assertion, or + Cloudflare being unreachable. The caller cannot act differently on any + of them, and distinguishing them in a return value would invite somebody + to treat one as good enough. + + The *log* does distinguish them, because operationally they could not be + more different: a wrong audience is a misconfiguration nobody spots from + outside, and an unreachable Cloudflare is an outage. + + The token is never logged. It is a bearer credential for its session, and + a log quoting it hands that session to anyone who can read logs. + """ + if not token: + return None + + if not self.is_configured(): + log.warning( + "Refusing an Access assertion: CF_ACCESS_TEAM_DOMAIN or " + "CF_ACCESS_AUD is unset, so nothing can be verified against. " + "Every admin request will be refused until they are set." + ) + return None + + try: + key = await self._signing_key(token) + if key is None: + log.warning( + "Refusing an Access assertion: signed with key id %s, which is not one of %s's published keys.", + _kid(token), + self.team_domain, + ) + return None + claims = jwt.decode( + token, + key.key, + algorithms=["RS256"], + audience=self.audience, + issuer=f"https://{self.team_domain}", + leeway=CLOCK_LEEWAY_SECONDS, + options={"require": ["exp", "aud", "iss"]}, + ) + except _KeysUnavailable: + # Reported where it was found, at a rate that does not follow the + # rate requests arrive at. Adding a line here would put the flood + # back, and naming the key id would blame the caller for an outage. + return None + except jwt.InvalidAudienceError: + # What a token minted for one of the team's node applications looks + # like. The fix is configuration, not anything the caller did. + log.warning( + "Refusing an Access assertion: it names audience %r, but this " + "environment's application is %s. Either it was issued for a " + "different application, or CF_ACCESS_AUD is stale.", + _claim(token, "aud"), + _short(self.audience), + ) + return None + # PyJWTError rather than InvalidTokenError: PyJWT's error surface is not + # ours to bound, and anything it raises outside that subtree would be a + # 500 on a request that should simply be refused. + except jwt.PyJWTError as exc: + log.warning("Refusing an Access assertion: %s: %s", type(exc).__name__, exc) + return None + except (ValueError, KeyError) as exc: + log.warning("Refusing an Access assertion: malformed: %s: %s", type(exc).__name__, exc) + return None + + # Cloudflare puts the address in `email`. A token that verifies but names + # nobody is not an identity, and something truthy would let a caller + # believe it had authenticated a person. + # + # Lowercased here, at the one place an identity is produced, because the + # claim arrives as the identity provider spelled it and callers derive a + # stable id from it. ADMIN_EMAILS and get_or_create_oauth_user normalise + # the same way, so an Access identity and an OAuth one for one address + # compare equal. + email = (claims.get("email") or "").strip().lower() + if not email: + log.warning( + "Refusing an Access assertion: it verifies, but carries no email claim, so it identifies nobody." + ) + return None + return email diff --git a/backend/core/users.py b/backend/core/users.py index 99ee40a1..e2759c85 100644 --- a/backend/core/users.py +++ b/backend/core/users.py @@ -21,6 +21,7 @@ from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column +from core.access_identity import AccessIdentity from core.env_parsing import parse_comma_list # ── Config ──────────────────────────────────────────────────────────────────── @@ -65,6 +66,19 @@ def _derive_auth_flags(env: Mapping[str, str]) -> tuple[bool, bool]: AUTH_ENABLED, AUTH_BYPASS = _derive_auth_flags(os.environ) +#: The header Cloudflare sets on every request its Access applications admit. +ACCESS_ASSERTION_HEADER = "Cf-Access-Jwt-Assertion" + +#: One instance, so the JWKS cache and its lock are shared across requests. +#: The audience differs per environment and comes from the compose overlays; the +#: team domain is the same everywhere and comes from the base compose file. +#: Either unset means unconfigured, and an unconfigured verifier is never +#: consulted rather than refusing assertions nobody sent. +access_identity = AccessIdentity( + team_domain=os.getenv("CF_ACCESS_TEAM_DOMAIN", ""), + audience=os.getenv("CF_ACCESS_AUD", ""), +) + _DATA_DIR = Path(__file__).resolve().parent.parent / "data" _DATA_DIR.mkdir(parents=True, exist_ok=True) # RETINA_DB_PATH exists so tests and one-off migrations can point at a scratch @@ -312,8 +326,42 @@ async def _read_user_from_request(request: Request) -> User | None: return user +def _access_user_dict(email: str) -> dict: + """A user dict for a verified Access identity, with no database row. + + Membership of the Access group is what grants the console, so anyone whose + assertion verifies for this environment's audience is an administrator; a + second list in AUTH_ADMIN_EMAILS would only be one more thing to drift. + + The id is derived from the email rather than allocated, so the same person + is the same id across requests and restarts and the destructive endpoints + stay attributable in /api/admin/events. + """ + return { + "id": str(uuid.uuid5(uuid.NAMESPACE_URL, f"mailto:{email}")), + "email": email, + "name": email.split("@")[0], + "avatar": "", + "provider": "cloudflare-access", + "role": "admin", + "is_superuser": True, + "created_at": 0, + } + + +async def _access_user_from_request(request: Request) -> dict | None: + """The verified Access identity for this request, or None.""" + if not access_identity.is_configured(): + return None + email = await access_identity.identity(request.headers.get(ACCESS_ASSERTION_HEADER)) + return _access_user_dict(email) if email else None + + async def get_current_user(request: Request) -> dict: """Return user dict or raise 401. Returns anonymous admin where AUTH_BYPASS is opted into.""" + access = await _access_user_from_request(request) + if access is not None: + return access if AUTH_BYPASS: return dict(ANONYMOUS_USER) user = await _read_user_from_request(request) @@ -324,6 +372,11 @@ async def get_current_user(request: Request) -> dict: async def require_admin(request: Request) -> dict: """Like get_current_user but also enforces superuser/admin role.""" + # Ahead of the bypass: where both are available the real person is the better + # answer, since "Admin (no auth)" is not an attribution. + access = await _access_user_from_request(request) + if access is not None: + return access if AUTH_BYPASS: return dict(ANONYMOUS_USER) user = await _read_user_from_request(request) diff --git a/backend/requirements.txt b/backend/requirements.txt index c2ad1969..1cc1c145 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -9,6 +9,10 @@ pyyaml==6.0.3 cryptography==46.0.6 boto3==1.42.89 fastapi-users[sqlalchemy]==13.0.0 +# Direct since core/access_identity.py imports it to verify Cloudflare Access +# assertions; it had arrived only as a transitive of fastapi-users, which pins +# this exact version, so it is not a free choice. [crypto] for RS256. +pyjwt[crypto]==2.8.0 aiosqlite==0.20.0 alembic==1.13.2 pyarrow==16.1.0 diff --git a/backend/routes/auth.py b/backend/routes/auth.py index b69cb078..b5c7e7c7 100644 --- a/backend/routes/auth.py +++ b/backend/routes/auth.py @@ -27,7 +27,6 @@ ) from core.users import ( ANONYMOUS_USER, - AUTH_BYPASS, JWT_LIFETIME_SECONDS, JWT_SECRET, get_current_user, @@ -231,10 +230,13 @@ async def callback_github(request: Request, code: str = "", state: str = ""): @router.get("/me") async def me(request: Request): - if AUTH_BYPASS: - return {**ANONYMOUS_USER, "auth_enabled": False} user_dict = await get_current_user(request) - return {**user_dict, "auth_enabled": True} + # Delegated rather than short-circuiting on AUTH_BYPASS, so this agrees with + # what require_admin decided: a verified Access assertion outranks the + # bypass, and answering "Admin (no auth)" while the admin routes attribute a + # real person would make the console wrong about its own session. + anonymous = user_dict["id"] == ANONYMOUS_USER["id"] + return {**user_dict, "auth_enabled": not anonymous} @router.post("/logout") diff --git a/backend/tests/test_access_identity.py b/backend/tests/test_access_identity.py new file mode 100644 index 00000000..751984cb --- /dev/null +++ b/backend/tests/test_access_identity.py @@ -0,0 +1,507 @@ +"""Tests for verifying Cloudflare Access assertions. + +Uses a real RSA keypair and real signed tokens rather than mocking the +verification, because the failures that matter here are the ones where a token +verifies and should not have. A mocked verifier would pass all of them. +""" + +import json +import logging +import time + +import httpx +import jwt +import pytest +from cryptography.hazmat.primitives.asymmetric import rsa + +from core.access_identity import ( + JWKS_GRACE_SECONDS, + JWKS_REFETCH_MIN_INTERVAL_SECONDS, + JWKS_TTL_SECONDS, + AccessIdentity, +) + +TEAM = "offworldlab.cloudflareaccess.com" +ISSUER = f"https://{TEAM}" +AUD = "e5ff9de8d1ca5fbc62b38d102d92a1fc7d910d5f89ef388caf63c83e828493b3" +OTHER_AUD = "0" * 64 +EMAIL = "someone@offworldlab.com" + + +@pytest.fixture(scope="module") +def keys(): + """One keypair for the suite. Generating RSA is slow enough to matter.""" + private = rsa.generate_private_key(public_exponent=65537, key_size=2048) + return private, private.public_key() + + +def _jwk(public_key, kid): + data = json.loads(jwt.algorithms.RSAAlgorithm.to_jwk(public_key)) + data.update({"kid": kid, "alg": "RS256", "use": "sig"}) + return data + + +class FakeJWKS: + """A real httpx client over a fake transport, counting fetches. + + Real client so the production code's own request path is exercised; the + counter is what lets the caching and single-refetch behaviour be asserted. + """ + + def __init__(self, jwks, fail=False): + self.jwks = jwks + self.fail = fail + self.calls = 0 + + def client(self): + def handler(request): + self.calls += 1 + if self.fail: + raise httpx.ConnectError("unreachable") + return httpx.Response(200, json=self.jwks) + + return httpx.AsyncClient(transport=httpx.MockTransport(handler)) + + +def token(keys, *, aud=AUD, iss=ISSUER, email=EMAIL, kid="kid-1", exp_delta=300, key=None, **extra): + private, _ = keys + claims = { + "aud": aud, + "iss": iss, + "email": email, + "exp": int(time.time()) + exp_delta, + "iat": int(time.time()) - 10, + **extra, + } + return jwt.encode(claims, key or private, algorithm="RS256", headers={"kid": kid}) + + +@pytest.fixture +def verifier(keys): + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "kid-1")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + v.fake = fake + return v + + +async def test_a_valid_assertion_yields_the_email(verifier, keys): + assert await verifier.identity(token(keys)) == EMAIL + + +async def test_the_key_set_is_cached(verifier, keys): + for _ in range(5): + await verifier.identity(token(keys)) + assert verifier.fake.calls == 1 + + +# ── the refusals that matter ───────────────────────────────────── + + +async def test_a_token_for_another_application_is_refused(verifier, keys): + """The one most easily left out, because such a token is perfectly signed. + + The team already runs Access on seventeen node hostnames, so without the + audience check anyone holding a valid session for those is admitted here. + """ + assert await verifier.identity(token(keys, aud=OTHER_AUD)) is None + + +async def test_a_token_from_another_team_is_refused(verifier, keys): + other = "https://someone-else.cloudflareaccess.com" + assert await verifier.identity(token(keys, iss=other)) is None + + +async def test_an_expired_assertion_is_refused(verifier, keys): + assert await verifier.identity(token(keys, exp_delta=-600)) is None + + +async def test_a_token_signed_by_someone_else_is_refused(verifier, keys): + """Right shape, right claims, wrong key. The signature is the whole point.""" + impostor = rsa.generate_private_key(public_exponent=65537, key_size=2048) + assert await verifier.identity(token(keys, key=impostor)) is None + + +async def test_an_unsigned_token_is_refused(verifier): + """alg=none walks past a verifier that trusts the header's own claim.""" + forged = jwt.encode( + {"aud": AUD, "iss": ISSUER, "email": EMAIL, "exp": int(time.time()) + 300}, + key="", + algorithm="none", + ) + assert await verifier.identity(forged) is None + + +async def test_the_email_is_normalised(verifier, keys): + """Cloudflare returns the claim as the identity provider supplied it, and + the id derived from it must be the same person's every time. Everything else + that handles an address here lowercases it first (ADMIN_EMAILS, + get_or_create_oauth_user), so this has to as well or the two never match.""" + assert await verifier.identity(token(keys, email=" Someone@OffworldLab.COM ")) == EMAIL + + +async def test_a_token_naming_nobody_is_refused(verifier, keys): + """Verifies, but authenticates no one. Something truthy would let a caller + believe it had identified a person.""" + assert await verifier.identity(token(keys, email="")) is None + + +async def test_a_token_with_no_audience_claim_is_refused(verifier, keys): + private, _ = keys + claims = {"iss": ISSUER, "email": EMAIL, "exp": int(time.time()) + 300} + naked = jwt.encode(claims, private, algorithm="RS256", headers={"kid": "kid-1"}) + assert await verifier.identity(naked) is None + + +async def test_rubbish_is_refused(verifier): + for value in ("", None, "not-a-token", "a.b.c"): + assert await verifier.identity(value) is None + + +# ── configuration ──────────────────────────────────────────────── + + +async def test_no_config_means_no_identity(keys): + """Fails closed. Before CF_ACCESS_AUD is set there is nothing to check + against, and admitting anyone meanwhile would be the worst default.""" + v = AccessIdentity(team_domain="", audience="") + assert v.is_configured() is False + assert await v.identity(token(keys)) is None + + +async def test_a_partial_config_is_treated_as_absent(keys): + v = AccessIdentity(team_domain=TEAM, audience="") + assert v.is_configured() is False + assert await v.identity(token(keys)) is None + + +# ── key rotation and outages ───────────────────────────────────── + + +async def test_an_unknown_key_id_triggers_one_refetch(keys): + """A key id we have not seen is either a rotation or a forgery, and one + refetch tells them apart. + + The refetch has a floor on it (see test_forged_kids_do_not_fetch_once_per_ + request), so the clock is wound back to put this rotation outside that + window. Winding it back rather than sleeping keeps the test instant, and the + floor is short next to both the hour-long TTL and Cloudflare's real rotation + cadence, so a genuine rotation is never delayed by more than the window. + """ + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "old-kid")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + assert await v.identity(token(keys, kid="old-kid")) == EMAIL + assert fake.calls == 1 + + fake.jwks = {"keys": [_jwk(public, "new-kid")]} + v._fetched_at -= JWKS_REFETCH_MIN_INTERVAL_SECONDS + 1 + assert await v.identity(token(keys, kid="new-kid")) == EMAIL + assert fake.calls == 2 + + +async def test_a_rotation_inside_the_floor_waits_for_it(keys): + """The cost of the floor, stated rather than discovered. + + A new key published seconds ago is not picked up until the window passes. + That is bounded and much shorter than the TTL that would otherwise govern. + """ + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "old-kid")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + assert await v.identity(token(keys, kid="old-kid")) == EMAIL + fake.jwks = {"keys": [_jwk(public, "new-kid")]} + assert await v.identity(token(keys, kid="new-kid")) is None + assert fake.calls == 1 + + v._fetched_at -= JWKS_REFETCH_MIN_INTERVAL_SECONDS + 1 + assert await v.identity(token(keys, kid="new-kid")) == EMAIL + + +async def test_a_kid_that_never_appears_is_refused_and_does_not_loop(keys): + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "real-kid")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + assert await v.identity(token(keys, kid="invented")) is None + assert fake.calls <= 2, "must not refetch endlessly for a forged kid" + + +async def test_one_unknown_kid_costs_one_fetch_not_two(keys): + """A cold cache fetches, misses, and must not immediately fetch the same set + again: the second attempt cannot succeed where the first just failed.""" + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "real-kid")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + assert await v.identity(token(keys, kid="invented")) is None + assert fake.calls == 1, "fetched the key set twice for one lookup" + + +async def test_forged_kids_do_not_fetch_once_per_request(keys): + """The reason there is a floor on refetching at all. + + `kid` is read from the unverified header, so anyone can name one without + signing anything. One outbound fetch per request would let an unauthenticated + caller drive a request to Cloudflare per request of their own, each holding + the lock that every admin request waits on. + """ + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "real-kid")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + for i in range(20): + assert await v.identity(token(keys, kid=f"forged-{i}")) is None + + assert fake.calls <= 2, f"{fake.calls} fetches for 20 forged assertions" + + +async def test_cloudflare_being_unreachable_refuses_rather_than_admits(keys): + """With nothing in hand there is nothing to fall back on.""" + fake = FakeJWKS({}, fail=True) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + assert await v.identity(token(keys)) is None + + +# ── the grace period ───────────────────────────────────────────── +# +# Access at the edge and the endpoint publishing these keys are different +# systems. The outage to design for is the one where Access admits the team +# normally and only the refresh fails; refusing then is an outage of our own +# making, using keys we still hold. + + +async def _aged(v, seconds): + """Wind the held set's age forward without waiting for it.""" + v._fetched_at -= seconds + + +async def test_a_stale_set_still_verifies_while_refreshing_fails(keys): + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "kid-1")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + assert await v.identity(token(keys)) == EMAIL + fake.fail = True + await _aged(v, JWKS_TTL_SECONDS + 60) + + assert await v.identity(token(keys)) == EMAIL, "refused on keys it was holding" + + +async def test_a_failed_refresh_is_not_retried_by_every_request(keys): + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "kid-1")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + assert await v.identity(token(keys)) == EMAIL + fake.fail = True + await _aged(v, JWKS_TTL_SECONDS + 60) + + before = fake.calls + for _ in range(20): + await v.identity(token(keys)) + assert fake.calls - before <= 1, ( + f"{fake.calls - before} attempts for 20 requests; a failing endpoint " + "must not be retried by each one in turn while holding the lock" + ) + + +async def test_past_the_grace_the_stale_set_is_refused(keys): + """The bound. Long past any plausible outage, short beside rotation.""" + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "kid-1")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + assert await v.identity(token(keys)) == EMAIL + fake.fail = True + await _aged(v, JWKS_TTL_SECONDS + JWKS_GRACE_SECONDS + 60) + + assert await v.identity(token(keys)) is None + + +async def test_a_cold_start_outage_is_not_logged_per_request(keys, caplog): + """The other outage path, and the worse one: nothing was ever fetched, so + nothing verifies. It needs the same throttle as the grace path, which it did + not get when that one was added.""" + fake = FakeJWKS({}, fail=True) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + caplog.set_level(logging.WARNING) + for _ in range(50): + assert await v.identity(token(keys)) is None + + assert caplog.records, "went quiet entirely" + assert len(caplog.records) <= 2, f"{len(caplog.records)} warnings for 50 requests" + + +@pytest.mark.parametrize("aged_by", [0, JWKS_TTL_SECONDS + JWKS_GRACE_SECONDS + 60]) +async def test_an_outage_is_never_reported_as_an_unknown_key_id(keys, caplog, aged_by): + """Attribution, on both refusing paths. + + "not one of the published keys" names a forged or rotated key id, which is a + different fault with a different fix. Saying it during an outage sends + whoever is reading the log hunting an attacker while Cloudflare is down. + """ + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "kid-1")]}, fail=(aged_by == 0)) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + if aged_by: + assert await v.identity(token(keys)) == EMAIL + fake.fail = True + await _aged(v, aged_by) + + caplog.set_level(logging.WARNING) + assert await v.identity(token(keys)) is None + + logged = " ".join(r.getMessage() for r in caplog.records) + assert "published keys" not in logged, f"blamed the key id for an outage: {logged}" + + +async def test_the_stale_warning_is_throttled(keys, caplog): + """Loud once, not once per request. + + The window is hours long and the request rate is not ours to set: the header + can be sent by anyone without signing anything, and nginx allows 30 a second + per address. A line each would bury the very thing it reports, during an + outage, in the logs somebody is reading to find out what broke. + """ + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "kid-1")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + await v.identity(token(keys)) + fake.fail = True + await _aged(v, JWKS_TTL_SECONDS + 60) + + caplog.set_level(logging.WARNING) + for _ in range(50): + assert await v.identity(token(keys)) == EMAIL + + assert caplog.records, "went quiet entirely" + assert len(caplog.records) <= 2, f"{len(caplog.records)} warnings for 50 requests" + + +async def test_serving_a_stale_set_is_not_silent(keys, caplog): + """An outage being survived should still be visible, or it is discovered + only when the grace runs out and everybody is locked out at once.""" + _, public = keys + fake = FakeJWKS({"keys": [_jwk(public, "kid-1")]}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + + await v.identity(token(keys)) + fake.fail = True + await _aged(v, JWKS_TTL_SECONDS + 60) + + caplog.set_level(logging.WARNING) + assert await v.identity(token(keys)) == EMAIL + assert caplog.records, "served a stale key set without saying so" + + +async def test_a_malformed_key_set_refuses_rather_than_raising(keys): + """A key set that parses as JSON but carries no usable key. + + PyJWKSet.from_dict raises PyJWKSetError for it, which is a PyJWTError but + not an InvalidTokenError, so it is caught where the fetch happens rather + than by anything reading the token. + """ + fake = FakeJWKS({"keys": []}) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + assert await v.identity(token(keys)) is None + + +# ── clock drift ────────────────────────────────────────────────── + + +async def test_small_clock_drift_is_tolerated(verifier, keys): + assert await verifier.identity(token(keys, exp_delta=-5)) == EMAIL + + +async def test_large_drift_is_not_tolerated(verifier, keys): + assert await verifier.identity(token(keys, exp_delta=-120)) is None + + +# ── explaining refusals ────────────────────────────────────────── +# +# Every refusal returns a bare None, which is right for the caller and useless +# for whoever has to work out why the console stopped admitting anyone. These +# assert the log makes up the difference, without handing out the credential it +# is describing. + + +async def test_every_refusal_says_why(verifier, keys, caplog): + caplog.set_level(logging.WARNING) + cases = { + "wrong audience": token(keys, aud=OTHER_AUD), + "wrong team": token(keys, iss="https://someone-else.cloudflareaccess.com"), + "expired": token(keys, exp_delta=-600), + "no email": token(keys, email=""), + "unknown kid": token(keys, kid="never-published"), + } + for label, bad in cases.items(): + caplog.clear() + assert await verifier.identity(bad) is None, label + assert caplog.records, f"{label} was refused silently" + + +async def test_the_token_is_never_logged(verifier, keys, caplog): + """A refused assertion is still a live bearer credential for its session. + A log quoting it hands that session to anyone who can read logs, and these + lines exist to be read by people debugging.""" + caplog.set_level(logging.DEBUG) + for bad in ( + token(keys, aud=OTHER_AUD), + token(keys, exp_delta=-600), + token(keys, kid="never-published"), + ): + caplog.clear() + await verifier.identity(bad) + logged = " ".join(r.getMessage() for r in caplog.records) + assert bad not in logged + # Not even a substantial slice. Signatures are the long tail. + assert bad.split(".")[2][:24] not in logged + + +async def test_a_wrong_audience_names_both(verifier, keys, caplog): + """The failure a misconfiguration actually produces, and the one invisible + from outside: the hostname is up and refuses everybody.""" + caplog.set_level(logging.WARNING) + await verifier.identity(token(keys, aud=OTHER_AUD)) + logged = " ".join(r.getMessage() for r in caplog.records) + assert OTHER_AUD[:12] in logged, "does not say what the token claimed" + assert AUD[:12] in logged, "does not say what this environment expects" + + +async def test_unreachable_cloudflare_is_distinguishable_from_a_bad_token(keys, caplog): + """An outage and a forgery both return None. Confusing the two sends + somebody hunting an attacker during a network problem.""" + caplog.set_level(logging.WARNING) + fake = FakeJWKS({}, fail=True) + v = AccessIdentity(team_domain=TEAM, audience=AUD, client=fake.client()) + assert await v.identity(token(keys)) is None + logged = " ".join(r.getMessage() for r in caplog.records).lower() + assert "could not reach" in logged + + +async def test_missing_config_names_the_variables(keys, caplog): + caplog.set_level(logging.WARNING) + v = AccessIdentity(team_domain="", audience="") + assert await v.identity(token(keys)) is None + logged = " ".join(r.getMessage() for r in caplog.records) + assert "CF_ACCESS_AUD" in logged + + +async def test_no_token_at_all_is_not_logged(verifier, caplog): + """Unauthenticated requests are ordinary on the ungated vhosts. Logging each + one would bury the refusals that mean something.""" + caplog.set_level(logging.WARNING) + assert await verifier.identity(None) is None + assert await verifier.identity("") is None + assert not caplog.records + + +async def test_a_success_is_not_logged_as_a_refusal(verifier, keys, caplog): + caplog.set_level(logging.WARNING) + assert await verifier.identity(token(keys)) == EMAIL + assert not caplog.records diff --git a/backend/tests/test_access_seam.py b/backend/tests/test_access_seam.py new file mode 100644 index 00000000..421e36f0 --- /dev/null +++ b/backend/tests/test_access_seam.py @@ -0,0 +1,155 @@ +"""The Access assertion as a third source of identity for the auth dependencies. + +The verifier itself is tested in test_access_identity.py against real signed +tokens. These cover the wiring: that a verified email becomes an admin, that an +unverified one becomes a 401, and that an unconfigured verifier changes nothing. +""" + +import asyncio +import uuid +from unittest.mock import MagicMock, patch + +import pytest +from fastapi import HTTPException +from starlette.datastructures import State + +from core.users import get_current_user, require_admin + +EMAIL = "someone@offworldlab.com" + + +class StubVerifier: + """Stands in for AccessIdentity, recording what it was asked to verify.""" + + def __init__(self, email=None, configured=True): + self.email = email + self.configured = configured + self.seen = [] + + def is_configured(self): + return self.configured + + async def identity(self, token): + self.seen.append(token) + # Same contract as the real verifier: no token is no identity, never an + # error. A stub that answered regardless would hide a seam that admitted + # callers who sent no assertion at all. + return self.email if token else None + + +def _request(header=None): + request = MagicMock() + request.headers = {"Cf-Access-Jwt-Assertion": header} if header else {} + request.cookies = {} + request.state = State() + return request + + +# ── a verified assertion is an administrator ───────────────────── + + +def test_a_verified_assertion_makes_require_admin_an_admin(): + verifier = StubVerifier(email=EMAIL) + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + user = asyncio.run(require_admin(_request("a-token"))) + assert user["email"] == EMAIL + assert user["is_superuser"] is True + assert user["role"] == "admin" + + +def test_the_assertion_is_what_gets_verified(): + """Guards against reading the wrong header, which would silently never + authenticate anyone.""" + verifier = StubVerifier(email=EMAIL) + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + asyncio.run(require_admin(_request("the-assertion"))) + assert verifier.seen == ["the-assertion"] + + +def test_a_verified_assertion_also_satisfies_get_current_user(): + verifier = StubVerifier(email=EMAIL) + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + user = asyncio.run(get_current_user(_request("a-token"))) + assert user["email"] == EMAIL + + +def test_the_identity_carries_a_stable_id_derived_from_the_email(): + """Admin actions are attributed in /api/admin/events, so the same person + must be the same id across requests and restarts, without a database row.""" + verifier = StubVerifier(email=EMAIL) + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + first = asyncio.run(require_admin(_request("t1"))) + second = asyncio.run(require_admin(_request("t2"))) + assert first["id"] == second["id"] + assert first["id"] == str(uuid.uuid5(uuid.NAMESPACE_URL, f"mailto:{EMAIL}")) + assert first["id"] != "00000000-0000-0000-0000-000000000000" + + +def test_the_provider_says_where_the_identity_came_from(): + verifier = StubVerifier(email=EMAIL) + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + user = asyncio.run(require_admin(_request("a-token"))) + assert user["provider"] == "cloudflare-access" + + +# ── refusals ───────────────────────────────────────────────────── + + +def test_an_assertion_that_does_not_verify_is_refused(): + verifier = StubVerifier(email=None) + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + with pytest.raises(HTTPException) as exc: + asyncio.run(require_admin(_request("forged"))) + assert exc.value.status_code == 401 + + +def test_the_plain_email_header_is_not_trusted(): + """Cloudflare also sets CF-Access-Authenticated-User-Email, which is an + unsigned header anyone can type. Only the signed assertion is identity.""" + verifier = StubVerifier(email=EMAIL) + request = _request() + request.headers = {"Cf-Access-Authenticated-User-Email": "attacker@example.com"} + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + with pytest.raises(HTTPException) as exc: + asyncio.run(require_admin(request)) + assert exc.value.status_code == 401 + + +def test_no_assertion_and_no_bypass_is_refused(): + verifier = StubVerifier(email=EMAIL) + with patch("core.users.AUTH_BYPASS", False), patch("core.users.access_identity", verifier): + with pytest.raises(HTTPException) as exc: + asyncio.run(require_admin(_request())) + assert exc.value.status_code == 401 + + +# ── the bypass, and not consulting an unconfigured verifier ────── + + +def test_an_unconfigured_verifier_is_never_consulted(): + """Every environment without CF_ACCESS_AUD set, including the test suite. + Reading the header there would refuse tokens nobody sent and log for it.""" + verifier = StubVerifier(email=EMAIL, configured=False) + with patch("core.users.AUTH_BYPASS", True), patch("core.users.access_identity", verifier): + user = asyncio.run(require_admin(_request("a-token"))) + assert verifier.seen == [] + assert user["id"] == "00000000-0000-0000-0000-000000000000" + + +def test_the_bypass_still_works_where_it_is_opted_into(): + """A laptop has no Access assertion, and docker-compose.local.yml keeps the + flag for that reason.""" + verifier = StubVerifier(email=None, configured=False) + with patch("core.users.AUTH_BYPASS", True), patch("core.users.access_identity", verifier): + user = asyncio.run(require_admin(_request())) + assert user["role"] == "admin" + + +def test_a_verified_assertion_beats_the_bypass(): + """Where both are available the real person is the better answer: the + destructive admin endpoints are attributed, and 'Admin (no auth)' is not an + attribution.""" + verifier = StubVerifier(email=EMAIL) + with patch("core.users.AUTH_BYPASS", True), patch("core.users.access_identity", verifier): + user = asyncio.run(require_admin(_request("a-token"))) + assert user["email"] == EMAIL diff --git a/backend/tests/test_auth_routes.py b/backend/tests/test_auth_routes.py index efa89f1a..cfccf73a 100644 --- a/backend/tests/test_auth_routes.py +++ b/backend/tests/test_auth_routes.py @@ -37,6 +37,27 @@ def test_me_returns_anonymous_admin_in_test_mode(self, client): assert body["role"] == "admin" assert body["auth_enabled"] is False + def test_me_reports_the_access_identity_rather_than_the_anonymous_admin(self, client): + """The dashboard asks /me who it is talking to, so this must agree with + what require_admin would decide. Answering "Admin (no auth)" while the + admin routes are attributing a real person is the inconsistency that + makes the console show the wrong thing about its own session.""" + from unittest.mock import patch + + class Stub: + def is_configured(self): + return True + + async def identity(self, token): + return "someone@offworldlab.com" if token else None + + with patch("core.users.access_identity", Stub()): + r = client.get("/api/auth/me", headers={"Cf-Access-Jwt-Assertion": "a-token"}) + assert r.status_code == 200 + body = r.json() + assert body["email"] == "someone@offworldlab.com" + assert body["auth_enabled"] is True + def test_logout_returns_ok(self, client): r = client.post("/api/auth/logout") assert r.status_code == 200 diff --git a/backend/tests/test_env_parity_scoping.py b/backend/tests/test_env_parity_scoping.py index 3e920cc4..733e9240 100644 --- a/backend/tests/test_env_parity_scoping.py +++ b/backend/tests/test_env_parity_scoping.py @@ -55,6 +55,28 @@ def test_edge_network_divergence_is_not_allowed_anywhere(self, parity, path, env assert not parity.allowed(path, env) +class TestCloudflareAccessEntries: + """The two halves of the Access configuration, which want opposite treatment. + + CF_ACCESS_AUD is the audience tag of one environment's Access application, so + it differs by nature exactly as HOST_ADMIN does and must be allowed to. Left + off the allowlist, every environment after the first would fail the parity + check and no deploy would pass. + + AUTH_ALLOW_ANONYMOUS_ADMIN must stay off it, and that does not stop mattering + once the flag is removed everywhere: the entry is what makes CI refuse a + change that reintroduces the anonymous admin to one environment on its own. + """ + + @pytest.mark.parametrize("env", ["test", "staging"]) + def test_the_access_audience_may_differ_per_environment(self, parity, env): + assert parity.allowed("services.server.environment.CF_ACCESS_AUD", env) + + @pytest.mark.parametrize("env", ["test", "staging"]) + def test_the_anonymous_admin_flag_may_never_differ(self, parity, env): + assert not parity.allowed("services.server.environment.AUTH_ALLOW_ANONYMOUS_ADMIN", env) + + class TestScopeValidation: def test_unknown_environment_is_rejected(self, parity): with pytest.raises(SystemExit): diff --git a/deploy/check-env-parity.py b/deploy/check-env-parity.py index 3cd6db90..87955ce0 100755 --- a/deploy/check-env-parity.py +++ b/deploy/check-env-parity.py @@ -80,6 +80,11 @@ r"^services\.server\.environment\.CORS_ORIGINS$", r"^services\.server\.environment\.CSP_CONNECT_SRC$", r"^services\.server\.environment\.HOST_[A-Z_]+$", + # The audience tag of this environment's Cloudflare Access application. One + # tag per application, so it differs per environment exactly as HOST_ADMIN + # does; an environment with none set leaves the verifier unconfigured, which + # means it is never consulted. See backend/core/access_identity.py. + r"^services\.server\.environment\.CF_ACCESS_AUD$", # Staging alone runs an E2E suite that force-retires the nodes it # registers, so staging alone confines what force can reach. Production # leaves the variable unset and so unrestricted: a decommissioned real diff --git a/docker-compose.test.yml b/docker-compose.test.yml index 6ce986a1..c08a61f2 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -65,6 +65,12 @@ services: # Anonymous admin, asked for explicitly since ClickUp 86cb49d29 took it # off RETINA_ENV. No OAuth is configured here either. - AUTH_ALLOW_ANONYMOUS_ADMIN=1 + # Audience of this environment's Access application, on test-admin.retina.fm. + # Pinning it is what stops a token minted for one of the fleet's node + # applications being accepted here; the team domain they share is in the + # base compose file. Staging and production carry their own once their + # applications exist. + - CF_ACCESS_AUD=e5ff9de8d1ca5fbc62b38d102d92a1fc7d910d5f89ef388caf63c83e828493b3 # The simulation subsystem, likewise asked for by name rather than # inherited from what this environment is called. This droplet is a # synthetic fleet and nothing else, so the ingest write path main.py gates diff --git a/docker-compose.yml b/docker-compose.yml index 31c7eec1..8895bb6e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -78,6 +78,13 @@ services: - SIM_FRAC_ANOMALOUS=0.0 - SIM_FRAC_DRONE=0.0 - SIM_FRAC_DARK=0.15 + # The Cloudflare Zero Trust team whose assertions the origin will believe. + # One team for the whole org, so it belongs here rather than in an overlay: + # a per-environment copy could drift to another team and the audience check + # alone would not catch it. The per-application audience does differ, and + # is CF_ACCESS_AUD in each overlay. Not a secret — it appears in the login + # URL of every Access redirect. + - CF_ACCESS_TEAM_DOMAIN=offworldlab.cloudflareaccess.com # bash, not sh: start.sh's supervisor uses `wait -n`, a bash builtin that # dash rejects ("Illegal option -n"), which crash-loops the server. The # script's shebang already says bash; naming it here keeps that true even if