Skip to content

Verify Cloudflare Access assertions at the origin - #372

Merged
Babissimo merged 3 commits into
mainfrom
feat/admin-access-gate
Sep 14, 2026
Merged

Verify Cloudflare Access assertions at the origin#372
Babissimo merged 3 commits into
mainfrom
feat/admin-access-gate

Conversation

@Babissimo

@Babissimo Babissimo commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

The origin half of ClickUp 123zgec1gtf.
Lands inert: nothing refuses anyone yet, because no deployed environment has an
audience except the test droplet, and the anonymous-admin bypass is untouched.
Closing it is a separate change that needs the staging and production Access
applications to exist first.

Independent of #371 and touches no file it does. Either order.

Why the fix is here and not at the edge

Every vhost in deploy/nginx/nginx.conf.template proxies /api/ to the same
FastAPI app, and FastAPI does not read the Host header, so
https://api.retina.fm/api/admin/node-owners reaches the same handler as the
admin hostname does. An Access application on admin.retina.fm protects that
hostname's HTML and nothing else. Enforcement therefore has to be backend-side,
and Access becomes the human doorway rather than the control. That is also why
api.retina.fm needs no application: it is the fleet's ingest hostname, and
nodes cannot complete an interactive login.

What is checked

Signature against the team's published keys, aud pinned to this environment's
application, iss the team domain, exp with 30s leeway, and every one of them
required. The audience is the load-bearing one and the easiest to omit, because a
token failing it is still perfectly signed: the team runs Access on seventeen node
hostnames, and without it any engineer's node session would open the console. It
was driven out by a test that genuinely failed first, as were the issuer, expiry,
signature and alg=none refusals.

CF-Access-Authenticated-User-Email is deliberately not read. There is a test
that fails if it ever is.

The seam

get_current_user and require_admin try a verified assertion before anything
else and otherwise behave exactly as they did. A verified assertion is sufficient
on its own, not additionally checked against AUTH_ADMIN_EMAILS: the Access
policy is already the membership list, and a second list means adding a colleague
takes two edits in two systems that drift. The id is derived from the email with
uuid5 rather than allocated, so one person keeps one id across restarts without
a database row and admin events stay attributable.

An unconfigured verifier is never consulted, so every environment without
CF_ACCESS_AUD, this test suite included, behaves precisely as before.

/api/auth/me stops short-circuiting on its own copy of AUTH_BYPASS. It was the
one place that would have disagreed with the dependencies, reporting
"Admin (no auth)" on the same request require_admin attributed to a real person,
and the dashboard asks it who it is talking to.

Outage behaviour, which is deliberate

Access at the edge and the endpoint publishing the signing 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 console
down on our own account using keys we are still holding. So a held key set keeps
verifying for six hours past its TTL when refresh is failing, then stops: long
past any plausible outage, short beside Cloudflare's rotation cadence of weeks.

Three properties hold on every failing path, and each has a test: at most one
fetch per interval, at most one log line per interval, and a refusal caused by an
outage never reported as a bad key id. The last matters because kid comes from
the unverified header, so without it an unauthenticated caller could drive one
Cloudflare fetch per request while holding the lock every admin request waits on.

Verification

  • 3564 backend tests pass, pre-commit run --all-files clean,
    deploy/check-env-parity.py clean.
  • Five review passes; every finding fixed and each fix mutation-checked, meaning
    the guard was removed and the expected tests confirmed to fail.
  • pyjwt[crypto]==2.8.0 becomes a direct dependency. It was already imported by
    test_auth.py while arriving only as a transitive of fastapi-users, which
    pins this exact version, so the pin is forced rather than chosen.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

Babissimo and others added 3 commits September 14, 2026 12:15
Nothing yet calls this; it is the piece the admin gate needs before
require_admin can refuse anyone, landed on its own so the security-critical
part can be read without the wiring around it.

Verifying rather than trusting the header matters twice here. Once for the
reason the node code gives: an Access application deleted or misconfigured
leaves the hostname open, and this is the only thing that would notice. And
once for a reason particular to this backend, that every vhost proxies to the
same app, so an assertion is the only thing distinguishing an administrator
from any caller who found api.retina.fm. Gating a hostname cannot do that job.

The audience check is the load-bearing one and the easiest to omit, because a
token failing it is still perfectly signed: the team runs Access on seventeen
node hostnames, and without it any engineer's node session would open the
console. It was driven out by a test that genuinely failed first, as were the
issuer, expiry, signature and alg=none refusals.

Async on httpx rather than the node's blocking requests, since this is awaited
from a dependency on the request path and a cold cache would otherwise stall
the event loop for the whole 10s timeout. PyJWKSetError is caught explicitly:
it descends from PyJWTError but not InvalidTokenError, so the obvious handler
lets a malformed key set escape as a 500 instead of failing closed.

pyjwt becomes a direct dependency. It was already imported by test_auth.py
while arriving only as a transitive of fastapi-users, which pins this exact
version, so the pin is forced rather than chosen.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The verifier landed with nothing calling it. This is the seam: get_current_user
and require_admin try a verified assertion before anything else, and otherwise
behave exactly as they did.

A verified assertion is sufficient on its own, deliberately not also checked
against AUTH_ADMIN_EMAILS. The Access policy is already the membership list, and
a second list means adding a colleague takes two edits in two systems that will
drift apart; the audience pin is what makes that safe, since a token minted for
any of the seventeen node applications fails it.

Access is tried ahead of the bypass rather than after. Where both are available
the real person is the better answer: these are the endpoints that retire
hardware, and "Admin (no auth)" is not an attribution. The id is derived from
the email with uuid5 rather than allocated, so the same person is the same id
across requests and restarts without a row having to exist and admin events stay
attributable.

An unconfigured verifier is never consulted, so the header is not even read where
CF_ACCESS_AUD is unset. Every environment that has not been given an audience,
this test suite included, behaves precisely as before.

/api/auth/me stops short-circuiting on its own copy of AUTH_BYPASS and delegates
instead. It was the one place that would have disagreed with the dependencies,
reporting "Admin (no auth)" on the same request that require_admin attributed to
a real person, and the dashboard asks it who it is talking to. auth_enabled now
derives from whether the identity actually is the anonymous one, so every
existing case reports what it always did.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The seam reads two values and nothing was supplying them. This wires the team
domain and the audience through compose, and teaches the parity checker that one
of them is meant to differ.

The team domain goes in the base file rather than being repeated per overlay:
there is one Zero Trust team for the org, and a per-environment copy could drift
to a different one without the audience check noticing, since that check only
proves a token was minted for this application, not by whom.

The audience is genuinely per environment, one tag per Access application, which
is exactly the shape HOST_ADMIN already has. So it gets the same treatment: an
ALLOWED_DIVERGENCE entry, because without one the second environment to be given
a tag fails the parity check and no deploy passes. Confirmed load-bearing by
removing it, whereupon the check fails naming the key.

Only the test environment carries a tag, because only test-admin.retina.fm has
an application so far. Staging and production are deliberately still unset, which
leaves the verifier unconfigured there and therefore never consulted: those
applications gate their hostnames the moment they exist, and staging's is probed
by CI, so they wait for the smoke-test changes.

The AUTH_ALLOW_ANONYMOUS_ADMIN exclusion gains a test rather than an edit. It
keeps working after the flag is dropped from all three overlays, since it is what
makes CI refuse a change that reintroduces the anonymous admin to one environment
on its own, and it should not quietly stop covering that.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the feat/admin-access-gate branch from 2306b44 to 840dc6a Compare September 14, 2026 11:15
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 2m 22s —— View job


Code review

  • Gather context (PR body, changed files, CLAUDE.md)
  • Understand the request (custom instruction: run /code-review on this PR)
  • Run code review over the diff (origin/main...HEAD)
  • Post findings

No issues found.

The diff is unchanged from the prior review on this PR (same 3 commits, same 11 files, same line counts), so this pass re-verified the same code rather than reviewing new changes.

What I checked:

  • backend/core/access_identity.py — traced the JWKS state machine by hand: _usable_keys/_signing_key/_try_refresh under the single asyncio.Lock, the _KeysUnavailable vs. InvalidAudienceError vs. generic PyJWTError branches in identity(), and the grace/backoff constants against their doc comments. Algorithm is pinned to RS256 explicitly (not read from the token header), aud/iss/exp are all required and verified, and the unverified kid/claims are only ever used for logging, never for a decision.
  • Specifically checked whether PyJWKSet.__getitem__(kid) (used in _lookup) actually raises KeyError for an unrecognized key id rather than TypeError — a wrong assumption there would turn every request with an unknown kid into an unhandled 500 instead of a clean refusal. Couldn't install pyjwt in this sandbox to check directly (network installs are blocked here), but test_access_identity.py exercises this exact path through a real PyJWKSet.from_dict() (via a mocked HTTP transport, not a mocked verifier) and asserts identity() returns None for an invented kid (test_a_kid_that_never_appears_is_refused_and_does_not_loop), which confirms the assumption holds.
  • backend/core/users.py / backend/routes/auth.py — seam ordering in get_current_user/require_admin (Access checked first, but _access_user_from_request short-circuits to None when access_identity.is_configured() is false, so an unconfigured verifier is genuinely never consulted), and /me now deriving auth_enabled from whether the resolved user is the anonymous sentinel rather than keeping its own AUTH_BYPASS check.
  • docker-compose.yml / docker-compose.test.yml / deploy/check-env-parity.pyCF_ACCESS_TEAM_DOMAIN in the base compose file (can't drift to another Cloudflare team per environment), CF_ACCESS_AUD scoped as an allowed per-environment divergence with a test enforcing it (test_the_access_audience_may_differ_per_environment) and a companion test making sure AUTH_ALLOW_ANONYMOUS_ADMIN is not similarly scoped. Confirmed the committed CF_ACCESS_AUD value being public is intentional and consistent with how Cloudflare Access audience tags work (not a secret, unlike a client secret) — same treatment as CF_ACCESS_TEAM_DOMAIN, which the diff's own comment notes appears in every Access redirect URL.
  • backend/tests/test_access_seam.py — confirms the plain CF-Access-Authenticated-User-Email header is never trusted, an unconfigured verifier is never consulted (verifier.seen == []), and a verified assertion outranks the anonymous bypass when both are available.

Nothing to flag — this holds up as a careful, narrowly-scoped implementation matching its own design note.

@Babissimo
Babissimo merged commit e230084 into main Sep 14, 2026
45 of 46 checks passed
Babissimo added a commit that referenced this pull request Sep 14, 2026
AUTH_ALLOW_ANONYMOUS_ADMIN made require_admin hand a superuser to any caller on
every vhost, since one FastAPI app answers all of them. With the origin-side
verifier in place (#372) and an Access application now in front of all three
admin hostnames, this flag was the last thing holding the door open.

All three deployed overlays lose it in a single commit because
check-env-parity.py refuses to let them disagree about it, and that guard is
right: closing the bypass in one environment and not the others is drift rather
than a decision. docker-compose.local.yml keeps it deliberately, since a laptop
has no Access assertion and no OAuth, and local sits outside the checker's scope.

Staging and production gain the audience of the application created for each, so
the verifier is configured everywhere the bypass has gone. An environment with
neither would refuse everybody, including the people meant to get in, and failing
closed should be reached on purpose rather than by omission.

test_anonymous_admin_closed.py covers what parity cannot. Parity catches the flag
returning to one environment; it cannot catch it returning to all three at once.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant