Phase 3 — federated exchange and scope enforcement¶
Parent plan: PLAN-auth-federation.md.
Scope¶
Phase 3 is the point of the whole plan: a GitHub Actions workflow holding nothing but its own OIDC identity token can exchange it for a scoped, expiring namespace key on a Shaken Fist cluster, and that key can do exactly what it was granted and nothing else.
Getting there needs two halves that are independent until the last step. The enforcement half gives every token a set of scopes and checks them on every request. The issuance half validates an external identity token against a standing, claim-gated rule and mints a key. Enforcement is built first, because a rule that grants scopes is meaningless until scopes are enforced, and because building it first means the federated path is never the only thing exercising it.
The phase covers:
- Inverting authentication from opt-in to opt-out, so a forgotten decorator fails closed rather than silently opening an endpoint.
- A scope vocabulary, mechanically derived, enforced on the universal authentication path.
TrustedIssuerandMappingRuleobjects, with CRUD APIs.- Identity token validation against cached JWKS.
- A recognisable format for cluster-minted key secrets, absorbed from what was phase 7, so that the first federated key ever minted already carries it.
- The exchange endpoint, with the abuse resistance an unauthenticated endpoint needs.
- Proof that scopes survive the namespace trust boundary.
Deliberately not in this phase: sf-client federation ...
commands, which live in the client-python repository and follow as
their own change; the CI conductor integration; the cache
save/restore actions; and the detection half of the old phase 7 —
the gitleaks CI job, the custom scanner rule and the Loki query.
Detection is independent of the exchange and stays its own phase; the
credential format it detects is not, because keys minted before the
format exists would need reissuing.
Design¶
Scope vocabulary and derivation (resolves open question 1)¶
A scope is a <family>.<verb> string. Operators see three verbs:
| Verb | Meaning | Derived from |
|---|---|---|
read |
Observe without changing | get, head |
write |
Create or modify | post, put, patch |
delete |
Destroy | delete |
The family comes from the resource class, and the verb from the
flask-restful method name — which is the HTTP verb, so nothing needs
to consult routing. BlobEndpoint.get derives blob.read with no
decoration at all. Coverage is therefore automatic across all 124
methods, which is what dissolves open question 2 into "audit the
override list".
Two escape hatches, both explicit at the decoration site so they are greppable:
class InstanceRebootEndpoint(api_base.Resource):
@api_base.scope(verb='power') # -> instance.power
def post(self, ...): ...
class SomeOddEndpoint(api_base.Resource):
scope_family = 'artifact' # class name is unhelpful
The wildcard scope * means "everything", and is what legacy
(unscoped) keys carry, so nothing an operator has today changes
behaviour. <family>.* means every verb in one family, matched on the
family rather than on characters so that node.* cannot reach
nodegroup.read.
Growing the vocabulary. A new verb is added only when an operator
genuinely needs to grant that action separately from write. The test
is whether anyone would sensibly write a rule granting it alone.
instance.power passes; instance.reboot-hard does not.
Enforcement inversion (resolves open question 10)¶
Measured on the current tree: 124 HTTP methods across 89 resource
classes, of which 120 already carry @verify_token. The four
which do not are Root.get, Livez.get, Readyz.get and
AuthEndpoint.post — exactly the endpoints that should be public.
There is no existing hole; the risk is the next endpoint someone
writes.
api_base.Resource already has a method_decorators list, and
class-level decorators run outermost, so authentication correctly
precedes the per-method ownership checks. Authentication and scope
enforcement move there, the 120 per-method @verify_token decorators
are removed, and the four public endpoints gain @api_base.public.
The audit inverts from "did every endpoint remember auth?" to "is
every @public justified?", which is four lines to review. A
pre-commit check backstops it, following the from_db_by_ref
namespace-scoping hook precedent already in
.pre-commit-config.yaml.
The structural test worth more than any of this: enumerate every route
Flask has registered and assert each one either enforces
authentication or appears in the @public allowlist. That test makes
the guarantee true by construction rather than by review.
Admin endpoints (resolves open question 9)¶
caller_is_admin today checks only that the request namespace is
system. A key scoped to blob.read but minted into system would
pass every admin endpoint — a scoped credential escalating to full
cluster administration.
Admin endpoints will require both the system namespace and a
cluster-admin scope on the token. Legacy unscoped keys carry the
wildcard, so existing admin automation is unaffected; only a
deliberately scoped system-namespace key is constrained, which is the
entire point.
The scope is hyphenated rather than dotted because it is not a
<family>.<verb> and names no family: of the twenty methods
caller_is_admin guards, only two derive an admin.* scope and the
rest derive node.*, issuer.*, auth.* and blob.read. Spelling
it admin invited reading it as the admin family's wildcard, which
was wrong in both directions — too narrow for what it gates, too
broad for what it grants. Carrying no dot also means no family
wildcard can synthesise it.
Both axes are required, and that is the feature rather than the
friction. ["cluster-admin", "node.read"] is a monitoring credential
with cluster-wide visibility that provably cannot delete a node, and
that is not expressible if administration is one all-or-nothing flag.
Object model (resolves open question 3's leftovers)¶
Both objects follow the NamespaceKey recipe phase 2 established:
Pydantic static/attribute schemas, three-layer MariaDB accessors,
protos, a DatabaseBackedObject with state machine and events,
registration in OBJECT_NAMES_TO_CLASSES.
TrustedIssuer — cluster-level, system-owned. Who may vouch for
identities here is an admin decision.
| Field | Notes |
|---|---|
name |
Unique; how rules reference it |
issuer_url |
Must match the token's iss exactly |
jwks_uri |
Where signing keys are fetched |
audience |
Expected aud |
MappingRule — owned by the namespace it targets, created under
the same gate as add-key (requires_namespace_ownership, which
auth.py already defines and uses for key creation).
| Field | Notes |
|---|---|
namespace |
Owner; rule dies with it |
name |
Unique within the namespace |
issuer |
Reference to a TrustedIssuer |
bound_claims |
Claim name → matcher (see below) |
scopes |
Granted to keys minted through this rule |
key_ttl |
Seconds; becomes the minted key's expiry |
key_name_prefix |
Human-readable prefix for minted key names |
Two things the master plan left for this phase:
- Multiple rules per namespace may bind the same issuer. Two repositories legitimately feed one cache namespace, and forbidding it would push operators toward one over-broad rule — the opposite of what we want.
- Mutating a rule does not touch keys already minted from it. Keys stand alone once minted; provenance records the rule reference and the claims that were satisfied, so the audit trail describes the grant as it was, not as the rule reads today. Shortening a rule's scopes does not retroactively narrow a live key — delete the key if that is what you mean.
Claim matching¶
This is where OIDC federations get compromised, so the semantics are
deliberately narrow. bound_claims maps a claim name to one of:
| Matcher | Form | Example |
|---|---|---|
| Exact | "value" |
"repository": "shakenfist/ryll" |
| Enumerated | ["a", "b"] |
"ref": ["refs/heads/develop", "refs/heads/main"] |
Every bound claim must be present and must match, and matching is exact string comparison — no globbing, no regular expressions, no prefix matching. A rule with no bound claims is rejected at creation: it would grant any holder of any token from that issuer.
Patterns are deliberately absent from v1. repository: shakenfist/*
looks reasonable until someone registers shakenfist-evil, and the
anchored-pattern rules needed to make it safe are exactly the thing
reviewers get wrong. If enumerated alternatives prove insufficient in
practice, patterns can be added later with anchoring enforced at rule
creation — but they must not be the default.
The exchange¶
POST /auth/federated, unauthenticated by nature, body
{token, namespace, rule}. The order of operations is chosen so that
the cheapest rejections happen first:
- Reject if the body exceeds
FEDERATION_MAX_TOKEN_BYTES. No parsing. - Rate limit per source address.
- Parse the JWT header and claims without verifying to read
iss. Reject if noTrustedIssuermatches. No network yet. - Verify the signature against cached JWKS.
- Check
aud,exp,nbf. - Load the named rule in the named namespace; check bound claims.
- Refuse if this
(token, rule)pair has been seen. - Mint the key.
Steps 1 to 3 preceding step 4 matter more than they look.
PyJWKClient fetches synchronously inside the request, so an
unfiltered path would let anyone with a made-up iss tie up a
gunicorn worker on an outbound HTTP call.
Steps 2 and 3 were also the other way around when this section was
first written, on the reasoning that refusing an unknown issuer first
kept the rate limit table from growing a row per made-up iss. That
reasoning does not survive being stated: resolving the issuer is a scan
of every configured issuer with two reads per row, so the ordering
avoided one cheap insert by permitting an unbounded number of reads
above the meter. The review that caught it is recorded below; the meter
is now second, and only the argument checks sit above it.
Steps 6 and 7 were the other way around when this section was first
written, which is not implementable: the pair being claimed is
(token, rule_uuid), and there is no rule uuid until the rule has
been read. Ordering the replay claim last, after claim matching and
after the namespace check, is also the behaviour worth having — a
refusal for any other reason must not consume the token's single use,
or an operator who fixes a rule cannot retry with a token that is
still perfectly valid. Being the last gate before minting is what
makes it effective against concurrency: two simultaneous
presentations of one token cannot both get past it, because the
second one's insert collides with the first one's.
JWKS caching uses PyJWKClient (PyJWT 2.13.0 is already a
dependency — no new package), with cache_jwk_set and an explicit
lifespan. An unknown kid triggers at most one refetch, guarded so
concurrent requests for the same issuer collapse into a single fetch
rather than a thundering herd against the IdP.
Replay is refused per (token, rule), not per token: exchanging one
token against two rules to reach two namespaces is a legitimate
pattern the CI conductor design depends on, while re-exchanging the
same token against the same rule is not. Seen pairs are stored in a
small table with the inbound token's exp as their own expiry, and a
composite primary key on (token_id, rule_uuid) does the arbitration
— the insert failing is the replay detection, with no
read-then-write race. They are reaped like any other expiring row.
token_id is the token's jti where the issuer provides a usable
one, and a SHA-256 of its signature otherwise. Not every identity
provider stamps a jti — the claim is optional in the spec — and
refusing those outright would rule out conforming issuers, while
letting them through unprotected would leave open exactly the hole
this table exists to close. The signature is unique per token by
construction, so it identifies the token just as well; it is hashed
rather than stored because it is the secret half of the credential.
Rate limiting is per source address, backed by MariaDB so the limit is cluster-wide rather than per gunicorn worker. Request volume on this endpoint is low by nature (once per CI job), so a row per source per window is affordable. Windows are fixed rather than sliding: the boundary lets a determined caller send two allowances back to back, which at this volume is not worth what a sliding window costs in rows. Both this counter and the replay claim fail closed — a database error refuses the exchange with a 503 rather than being read as "under the limit" or "not seen before", since both of those readings authorise something.
Auditing. A successful exchange writes an audit event against the minted key and its namespace, carrying the satisfied claims and the rule, never the secret. A failed exchange writes an event against the rule's owning namespace: a stream of near-miss claim failures is what probing looks like, and the namespace owner is the person who needs to see it. Failures against an unknown namespace or rule cannot be audited to an owner and are logged only, to avoid giving an unauthenticated caller a way to write unbounded events.
Minted key naming (resolves open question 5)¶
The rule's key_name_prefix is a prefix, not a template, and the
exchange appends a random discriminator — the same shape as
_service_key_<random>. Names are therefore unique by construction
and there is no collision case to resolve: a workflow re-run gets a
new key rather than silently rotating the secret out from under a
still-running job.
Federated keys appear in the legacy keys listing like any other key.
Hiding them would make audits lie.
Cluster-minted credential format (absorbed from phase 7)¶
Phase 3 is where Shaken Fist starts generating key secrets rather than only accepting operator-chosen ones, so this is where the generated form gets a shape. Doing it later would mean every key minted in between needs reissuing before a scanner could recognise it.
Following the pattern GitHub (ghp_), GitLab (glpat-), Stripe
(sk_live_) and Slack (xoxb-) use:
42 characters total, comfortably inside the existing 72-character
limit that auth.py enforces. The random body carries ~190 bits. The
prefix makes a leaked key greppable; the checksum lets a scanner
reject lookalikes without calling us, which is what makes scanning at
volume tolerable rather than alert spam.
This costs nothing cryptographically. A bearer token is a random identifier, not ciphertext, so a fixed prefix is a label beside the random part rather than a revealed piece of it — the entropy of the random body is unchanged.
The prefix must be reserved. /auth cannot tell which stored key
a presented secret is meant to match until it bcrypt-compares against
each one, so "reject early on a bad checksum" is only sound if no
legitimate secret can carry the prefix and fail the checksum. Key
creation therefore rejects operator-supplied secrets beginning with
sfk_, mirroring how _service_key is already reserved for key
names in auth.py.
That reservation has one upgrade consequence which must reach the
operator guide rather than being discovered: an existing key whose
secret happens to begin with sfk_ will stop authenticating once
early rejection is live, and must be rotated. The probability is
negligible — it is a four-character prefix on a user-chosen secret —
but it is not zero, and it is a silent authentication failure rather
than a loud one.
Applies only to secrets the cluster generates: exchange-minted keys,
the _service_key_* keys get_api_token() mints, and a new
"generate one for me" option on operator key creation. An
operator-supplied secret is whatever they chose and cannot carry our
prefix.
Token lifetime (resolves open question 6)¶
Mint-time expires_delta is capped at the key's remaining lifetime.
The nonce check already invalidates derived tokens the moment the key
expires, so this is cosmetic — but a token whose exp outlives the
credential it came from is confusing to anyone reading it, and the cap
costs one min().
Scopes and trust (resolves open question 11)¶
Namespace trust grants cross-namespace visibility. It must not grant
capability. A token scoped blob.read may read blobs it can see,
including those visible through trust, and must never gain wildcard
behaviour because the object it touched lives in a trusting namespace.
This gets an explicit test: a scoped key in namespace A, a trust from B to A, and an assertion that the scoped token can read across the trust but cannot write across it. Without that test, trust becomes a scope-escape hatch.
Authentik as the second issuer¶
No code differs. An Authentik client_credentials service account
presents a JWT with iss of the Authentik realm, aud of the
configured client, and claims such as sub and groups. A rule binds
{"groups": ["sf-ci"]} instead of
{"repository": "shakenfist/ryll"}. The phase plan's proof obligation
is a test constructing such a token against a mock issuer and
exchanging it successfully — demonstrating the machinery is
issuer-generic without shipping an Authentik dependency.
Decisions¶
- Scopes are derived, not tagged (operator, 2026-07-29). Automatic coverage beats readability-by-hand; overrides are explicit and greppable.
- Authentication inverts to opt-out in this phase (operator, 2026-07-29). Measured as cheap: 120 of 124 methods already authenticate, and the four exceptions are the correct ones.
- Admin endpoints require a
cluster-adminscope (operator, 2026-07-29). Closes scoped-key escalation insidesystem. Namedadminwhen first implemented; renamed 2026-08-01 (operator) because it named no family and read as the admin family's wildcard. The same review confirmed both axes stay required, so a least-privilege administrative credential remains expressible. - Full abuse resistance in v1 (operator, 2026-07-29). The endpoint is unauthenticated by nature and is the most exposed surface in the API.
- Exact and enumerated claim matching only. No patterns in v1.
- Rules may share an issuer; mutating a rule does not affect already-minted keys.
- Minted key names carry a random discriminator, so re-runs never collide and never rotate a live key.
- The credential format moves here from phase 7 (operator, 2026-07-29). Phase 3 mints the first cluster-generated keys, so the format has to exist before them or they need reissuing. The detection half — gitleaks in CI, the custom rule, the Loki query — is independent of the exchange and stays in phase 7.
- The
sfk_prefix is reserved on operator-supplied secrets, so that failing a bad checksum early at/authis sound rather than a guess. Carries a documented upgrade caveat for the negligible but non-zero case of an existing secret already starting withsfk_. - Family wildcards
<family>.*(operator, 2026-08-01). Granting a whole family is the common case in a mapping rule and enumerating three verbs invites getting one wrong. Matched on the family rather than on string prefixes, sonode.*cannot reachnodegroup.delete.
Step plan¶
| Step | Effort | Model | Isolation | Brief for sub-agent |
|---|---|---|---|---|
| 3a | high | opus | none | Enforcement inversion, no scopes yet. Move verify_token onto api_base.Resource.method_decorators — but not log_token_use: measurement showed 3 of the 120 authenticated methods deliberately omit it, and AuthNamespacesEndpoint.post writes its own richer namespace-creation events instead, so moving it universally would double-log there and would not be the pure refactor this step is supposed to be. Leave it per-method; remove the 120 per-method @api_base.verify_token decorators; add an @api_base.public marker and apply it to exactly Root.get, Livez.get, Readyz.get, AuthEndpoint.post. Class-level decorators run outermost so auth precedes ownership checks — verify that ordering with a test, it is the load-bearing assumption. Add the structural test: enumerate app.url_map and assert every rule's methods either authenticate or are @public. Add a pre-commit check modelled on tools/check-from-db-by-ref-namespace.sh that fails if a resource method is added without either. No behaviour change intended: the full existing suite must pass unmodified. Commit subject: "api: authenticate every endpoint by default." |
| 3b | high | opus | none | Scope vocabulary and enforcement. Derivation (<family>.<verb> from resource class and method name, families defaulting from the class name with a scope_family class attribute override, verbs read/write/delete); the @api_base.scope(verb=..., family=...) annotation for overrides; enforcement on the same universal path added in 3a; wildcard * for tokens minted from unscoped keys; default-deny where derivation is impossible. Add the cluster-admin scope requirement to caller_is_admin per Decision 3 (delivered as admin and renamed on 2026-08-01; see that decision). Publish the vocabulary and derivation rule in the developer guide. Tests: derivation for each verb, override honoured, wildcard passes everything, scoped token denied outside its scopes, and an admin endpoint refused to a scoped system key. Commit subject: "auth: derive and enforce token scopes." |
| 3c | medium | opus | none | Cluster-minted credential format, absorbed from the old phase 7. A shakenfist/util/credentials.py (or similar) with generate() producing sfk_ + 32 base62 random + 6 base62 CRC32 checksum, and looks_valid(secret) verifying prefix and checksum. Reserve the prefix: key create and update reject an operator-supplied secret starting with sfk_, mirroring the existing _service_key name reservation in external_api/auth.py. Wire generation into get_api_token()'s _service_key_* secrets and add a "generate one for me" option to operator key creation (a request with no key returns a generated one — the response already returns the key name, so returning the secret is an additive change). Early rejection at /auth: a presented secret carrying the prefix but failing the checksum is refused before any bcrypt comparison. Tests: round trip, checksum catches single-character corruption, prefix reserved at create and update, early rejection fires, an operator secret without the prefix is unaffected. Document the upgrade caveat (an existing secret beginning with sfk_ must be rotated) in the operator guide. Commit subject: "auth: give cluster-minted key secrets a recognisable format." |
| 3d | medium | opus | none | TrustedIssuer object, following the NamespaceKey recipe exactly (shakenfist/namespace_key.py, schema/namespace_key_data.py, the mariadb.py three-layer accessors, protos/database.proto, daemons/database/main.py handlers, OBJECT_NAMES_TO_CLASSES). System-namespace-only CRUD endpoints under /auth/issuers. Unique on name. Run tox -e genprotos, never grpc_tools directly. Commit subject: "objects: add the TrustedIssuer object." |
| 3e | medium | opus | none | MappingRule object, same recipe, owned by its namespace, unique on (namespace, name). CRUD under /auth/namespaces/{namespace}/rules, gated by requires_namespace_ownership (already defined in external_api/auth.py and used by key creation). Claim matcher validation at creation: exact strings or lists of strings only, at least one bound claim, referenced issuer must exist. Rules are deleted with their namespace. Commit subject: "objects: add the MappingRule object." |
| 3f | medium | opus | none | Identity token validation, no endpoint yet. A shakenfist/federation.py module: unverified header/claim peek to read iss; issuer lookup; PyJWKClient with cache_jwk_set and a configured lifespan; single-flight refetch on unknown kid (a lock per issuer, so concurrent requests collapse to one fetch); aud/exp/nbf verification; claim matching against a rule. Pure functions plus one cache object, no Flask. Tests use locally generated RSA keys and a mock JWKS endpoint — no network. Cover: good token, bad signature, wrong aud, expired, unknown kid refetch, refetch happens once under concurrency. Commit subject: "federation: validate identity tokens against trusted issuers." |
| 3g | high | opus | none | The exchange endpoint. POST /auth/federated, @public, body {token, namespace, rule}, implementing the eight-step order in the Design section. Mint via NamespaceKey.new(), with the secret produced by 3c's generator so every federated key is scanner-recognisable from the first one, and with the rule's scopes, key_ttl as expiry, provenance recording the rule reference and satisfied claims, and a random discriminator on the name. Audit events per the Design section, including the failed-exchange event against the rule's owning namespace and the deliberate silence when no owner can be identified. Response {namespace, key_name, key}. Config: FEDERATION_MAX_TOKEN_BYTES, FEDERATION_JWKS_CACHE_SECONDS. Commit subject: "auth: exchange identity tokens for scoped namespace keys." |
| 3h | medium | opus | none | Abuse resistance: the (jti, rule_uuid) replay table with a unique index (the failing insert is the detection — no read-then-write race), expiring at the inbound token's exp and reaped by the cluster daemon alongside expired keys; per-source rate limiting backed by MariaDB so the limit is cluster-wide rather than per worker, with FEDERATION_RATE_LIMIT_PER_MINUTE and 0 to disable. Tests: replay refused, same token against a second rule allowed, rate limit trips and recovers, reaper removes expired jti rows. Commit subject: "federation: replay and rate limit protection for the exchange." |
| 3i | medium | opus | none | Closeout. The trust-composition test from open question 11 (scoped key in A, trust B→A, read allowed across the boundary, write refused). The Authentik proof test: a mock issuer with groups claims, a rule binding them, a successful exchange with no code differing from the GitHub path. End-to-end functional coverage in shakenfist/deploy/shakenfist_ci/cluster_ci_tests. Docs: the scope vocabulary and derivation rule, the exchange flow, issuer and rule configuration, and a worked GitHub Actions example written against public GitHub concepts only — nothing about the private CI conductor. Glossary entries for trusted issuer, mapping rule and scope stop being future tense. Master plan open questions 1, 2, 3, 4, 5, 6, 9, 10, 11 marked resolved; phase 3 → Complete in the Execution table and docs/plans/index.md. Note the sf-client federation ... client-python follow-up in Future work, and confirm the master plan's phase 7 now reads as detection-only since its format half landed here. Commit subject: "docs: federated identity exchange." |
After each step the management session runs
pre-commit run --all-files, reads the diff against the brief, and
confirms no unrelated edits. After 3a additionally: the full existing
suite passed unmodified, since 3a is a pure refactor. After 3b: a
manual read confirming no endpoint lost enforcement. After 3g and 3h:
present the diff for operator review before commit — the exchange is
the security boundary of this whole plan. After 3i:
tox -e genprotos is a no-op against the committed tree.
Risks and mitigations¶
- Risk: 3a touches 120 methods and silently drops enforcement somewhere. Mitigation: it is a pure refactor, so the existing suite must pass unmodified; plus the structural route-enumeration test, which makes the property true by construction rather than by inspection.
- Risk: the decorator ordering assumption is wrong and authentication ends up running after an ownership check that itself assumes an authenticated caller. Mitigation: 3a's brief calls it out as load-bearing and requires a test asserting the order.
- Risk: scope derivation silently mislabels an endpoint, granting more than an operator expects — the failure is quiet because it looks like it works. Mitigation: the override list is one grep; 3i's documentation publishes the derived scope for every endpoint family so it can be reviewed as data rather than inferred from code.
- Risk: claim matching is too narrow for real GitHub usage and operators route around it with an over-broad rule. Mitigation: enumerated alternatives cover the common "several branches, several repos" cases. If practice proves otherwise, anchored patterns are addable later — the danger is shipping them as the default, not having them at all.
- Risk: the exchange endpoint becomes a denial-of-service vector against either us or the IdP. Mitigation: Decision 4's full ordering, with size and allowlist checks before any network call, and single-flight JWKS refetch.
- Risk: a scoped key minted into
systemescalates. Mitigation: Decision 3. Additionally, 3e should consider warning at rule creation when the target namespace issystem. - Risk: reserving the
sfk_prefix breaks an operator whose existing key secret already starts with it, and it breaks as a silent authentication failure rather than a loud one. Mitigation: the probability is negligible (a four-character prefix on a user-chosen secret) but not zero, so it is called out in the operator guide's upgrade notes rather than left to be discovered. 3c's brief makes documenting it part of the step. - Risk: trust becomes a scope-escape hatch. Mitigation: open question 11's test, which is a named deliverable of 3i rather than an afterthought.
Definition of done¶
- Every registered route either authenticates or is one of a
small, individually justified
@publicset, asserted by a test overapp.url_mapand backstopped by a pre-commit check. - Scopes derive automatically for all endpoints; overrides are explicit, greppable and documented; legacy unscoped keys carry the wildcard and behave exactly as before.
- Admin endpoints require both the
systemnamespace and thecluster-adminscope, and a["cluster-admin", "node.read"]credential can read a node but not delete one. -
TrustedIssuerandMappingRuleare database-backed objects with events, CRUD APIs, and the correct ownership gates; rules die with their namespace. - A GitHub Actions OIDC token can be exchanged for a scoped,
expiring key that works with an unmodified
sf-client. - The same machinery exchanges an Authentik-style token with configuration only, proven by test.
- Replay of one token against one rule is refused; the same token against a different rule still works.
- Scopes survive the trust boundary: readable across, not writable across, asserted by test.
- No secret appears in any event; the failed-exchange event reaches the rule owner.
- Every cluster-minted secret carries the
sfk_prefix and a verifiable checksum; the prefix is reserved against operator-supplied secrets; a bad checksum is rejected before any bcrypt comparison; and the rotation caveat for a pre-existingsfk_-prefixed secret is in the operator guide. -
pre-commit run --all-filesclean;tox -e genprotosno-op; unit tests green. Re-confirmed by the pre-push audit after the rebase onto develop. - Functional CI green on the branch before the PR merges.
Could not be established from a local tree and was the operator's
gate at pull request time; #3625 merged as
c64269e63on 2026-08-06 with the merge queue green. - Master plan open questions 1, 2, 3, 4, 5, 6, 9, 10 and 11
marked resolved; phase status updated in the Execution table
and
docs/plans/index.md; glossary and guides updated. - Pre-push security review findings triaged: every high fixed, every medium and low either fixed or recorded below with the reason it was accepted.
Pre-push security review¶
The pre-push audit's security agent read the whole phase 3 diff. What it found and what was done, kept here because the reasoning for an accepted finding is worth more later than the finding itself.
Fixed, each with a regression test that was verified to fail when the fix is reverted:
- A rule could grant scopes its author did not hold. A token scoped
rule.writecould write a rule granting*, satisfy that rule's own bound claims, and exchange it for a wildcard key — which in the system namespace reaches cluster-admin._rule_argumentsnow applies the same ceiling_namespace_keys_putpostapplies when minting a key directly. - Replay refusal was keyed on the token's signature text. base64url leaves four don't-care bits in the final character of a 256 byte signature and the padding is optional, so one signature has many spellings that all verify — measured at 48 for an RS256 token. Each spelling was a separate replay slot. The identity is now derived from the signed material (header and payload), which the signature commits to and an attacker cannot vary.
- The identity token was written to the API log.
log_requestmerges the request body into the decorated method's kwargs and logs them, redacting by field name — and the federated endpoint's credential field istoken, which was not on the list. The body is now dropped entirely for any route under/auth/, matching whatapp.pyalready did for the audit event stream, with the predicate shared so the two cannot disagree. - The JWKS fetch used PyJWT's 30 second default timeout while
holding the issuer's refetch lock, so an unreachable provider could
pin an API worker for 30 seconds per queued request. Now
FEDERATION_JWKS_FETCH_TIMEOUT_SECONDS, default 5. - The exchange's body size check could not do its job. It ran in
the endpoint method, by which point
log_requesthad already calledget_json(force=True)and parsed the whole body; and it readcontent_length, which isNonefor chunked encoding, so a header choice opted out of the limit entirely. The refusal moved to a request hook ahead of every reader of the body, and a body with no declared length is now refused with 411 rather than measured. - Two trusted issuers could claim the same issuer URL, making which provider's keys are trusted depend on listing order — an operator repointing an issuer would believe they had while some requests kept verifying against the old JWKS. Refused on create and update, using the same lookup token validation uses.
- A mapping rule's
key_name_prefixbypassed the reserved key name check. A rule with the prefix_service_keyminted keys colliding with the cluster's own service credentials, which is precisely what the key endpoints refuse. The reserved-name patterns moved toutil.credentialsso both paths ask the same question. - Rule fields had no upper bounds, so an oversized value reached
the database and returned a 500 instead of a message the operator
could act on.
key_ttladditionally had no ceiling, so a rule could mint a key outliving by a year the identity token that justified it. - A damaged rule row leaked its UUID to an anonymous caller. The
generic 500 handler answers with
repr(e)andCorruptMappingRulenames the rule; on the one endpoint anybody may call, that hands a stranger an identifier. Caught and turned into a generic refusal. The first attempt guarded the wrong call and protected nothing; see the pull request review below.
Assessed and accepted:
- The rate limiter keys on
remote_addrwith noProxyFix. Behind a reverse proxy that does not rewrite the source address this is one global limit rather than a per-caller one. TrustingX-Forwarded-Forunconditionally would be worse — it would let any caller choose their own rate limit bucket — and whether the header can be trusted is a property of a deployment we cannot see from here. Documented in both the config description and the operator guide instead. - The refusal reasons are coarse categories. A caller learns "untrusted issuer" versus "token rejected" versus "no such rule". This is deliberate: the rule lookup happens after token validation, so an anonymous caller holding no valid token cannot use it to enumerate rules, and an operator debugging their own workflow needs to know which stage refused them. Which claim missed is still withheld.
PyJWKClientfollows redirects when fetching a JWKS. Reaching this requires the ability to register a trusted issuer, which is cluster-admin — an actor who already has total control. Thehttps://requirement onjwks_uristands.- The
federation_replayandfederation_rate_limitstables grow with traffic. Both are already swept by the cluster daemon's reapers, so this was addressed before the review ran.
Pull request review (#3625)¶
The automated reviewer on the pull request raised eight items. Three were marked for fixing, five as things to consider. All eight were addressed; the two that turned out to matter most are the first two, because between them they show a mitigation and its own regression test agreeing with each other and both being wrong.
Fixed:
- The
CorruptMappingRuleguard was wrapped around a call that cannot raise it. The exception comes from decodingbound_claimsorscopes, which happens on the attributes read;MappingRule.from_db_by_namereads the static row and the object state and touches neither. The first read that could actually fail was theissuercomparison, outside thetry, so the UUID leak the guard was written to prevent was still open. The endpoint now reads the whole policy once, inside atrythat covers it, via the newMappingRule.policy(). - The regression test mocked the one function that cannot raise.
It patched
from_db_by_name, so it passed against the broken guard — which is why the guard shipped in the wrong place. It now patches the attribute read, asserts the lookup really did succeed first, and was verified to fail (401 != 500) against the unguarded endpoint. arg_is_artifact_ref's docstring described the trust behaviour this branch removes, telling the reader that ownership still lets a trusted namespace delete by UUID. It does not, as every other document in the branch says. Rewritten, and the trust case added to therequires_artifact_accesscomment, which had omitted it.
Considered, and changed:
- Issuer resolution ran above the rate limit, and the comment
justified that ordering by calling the preceding steps free.
issuer_claiming_urlscans every configured issuer and reads state and attributes per row, so it was the one unauthenticated, unmetered database amplification path in the new code — and the ordering was backwards on its own logic, since the counter row it was avoiding costs less than the scan it was permitting. The meter moved above the lookup. Counting is one row per source per window, the same row a caller naming a real issuer already earned, so the table grows no faster. The test that asserted the old contract now asserts the new one. issuer_urluniqueness was a check-then-write with nothing serialising it, while three documents described it as an invariant. It cannot have a unique index, because a soft-deleted issuer keeps its row so its URL stays reusable, so both endpoints now hold a cluster lock across the check and the write — thevsock_cidspattern frominstance.py. Tested by asserting the create happens between the acquire and the release.- Every attribute access re-read the row. One exchange made five
round trips for one rule.
policy()reads once; the exchange andexternal_view()both use it. This fell out of the first fix.
Considered, and documented rather than changed:
- Anonymous requests write an audit row before the rate limit
applies.
log_request_infoevents every request toAPI_REQUESTSahead of routing, so_federated_refusal's care not to let an anonymous caller write unbounded rows into a namespace's log does not extend to the API's own. This is pre-existing and shared withPOST /auth, the other public route, so it is out of scope for this branch — but the docstring no longer reads as though the exposure is closed. - Widened artifact name resolution does a trust lookup per candidate. A popular name across many namespaces fans out to one trust read each. It only runs when the caller's own namespace has no match, so the common case is untouched. Recorded in the docstring as a deliberate choice, with the SQL pushdown that would fix it if it ever shows up in a profile.
Second pull request review (#3625)¶
A re-review after the first round raised seven further items: two to fix, one documentation correction, four to consider. All seven were addressed. None of them contradicted a fix from the first round, and the severity fell — the first round found a security hole with a regression test that agreed with it, this one found one live authorisation gap and otherwise inconsistencies between what the code does and what this branch's own documents claim it does.
Fixed:
- A trust still authorised destructive mutation of another
namespace's artifact.
ArtifactUploadEndpoint.postand the cache route resolved a caller-supplied url withArtifact.from_url, whose predicate is visibility — so a trusted namespace could name the owner'ssource_url, land on the owner's artifact, and have its own blob added as the newest version.add_indexends indelete_old_versions, so the owner's older versions went with it. The root cause was one function serving both a read and a write intent, which is the same defect this phase already fixed for name resolution when it splitarg_is_artifact_reffromarg_is_visible_artifact_ref.from_urlnow documents that it resolves by visibility, and the newArtifact.owned_from_url()is what a write path uses. Both routes then authorise the existing and the brand new cases apart: a trust is enough to gift a namespace an artifact it did not have, and not enough to replace what one it already owns resolves to. The audit event also moved below the check, so a refused caller can no longer append to the event log of a namespace it is about to be told does not exist. - Two documents still described the pre-reorder exchange sequence.
The developer guide and this plan both listed issuer resolution above
the rate limit, and this plan contradicted itself within one file.
Both lists now match the code, and the inline step comments in
AuthFederatedEndpoint.post— which read 2, 3, 4-5, 7, 6, 8 — are renumbered, since an ordering argument that cannot be audited against the code is not much of an argument. - The operator guide overstated what a trust withholds. It claimed
a trusted namespace cannot change your objects' metadata, but
requires_namespace_ownershipisnamespace_is_trusted, so a trusted namespace can add keys and write mapping rules in yours — the most privilege-bearing objects this phase adds. The behaviour matches the documentedadd-keyprecedent and is deliberate; the blanket claim was what needed narrowing. The section now says plainly that a trust is administrative access to a namespace's credentials, not only a window onto its resources.
Considered, and changed:
- The rate limiter could fail open. Both federation replies
signalled failure by carrying a non-empty
error, which left the fail-closed property resting on string formatting: an exception raised with no args has an emptystr(), so the reply arrived asattempts=0, error=''and read as "nobody has tried this minute". Not reachable through any current path, because_direct_count_federated_attemptconverts the plausible failures intoDatabaseUnavailablewith text — but an invariant carried by a string being non-empty is not one a future contributor will know to preserve. Both replies now carry an explicitbool ok, set only on the success path, and the client decides on that. - A damaged rule took the whole CRUD listing down.
external_view()calledpolicy(), so one undecodable column turned a namespace's entire rule listing into a 500 and hid every healthy rule. Worse,delete()does the work and then builds the response, so a damaged rule was soft-deleted successfully and the caller still got a 500 — on the one operation that would have cleaned it up.external_view()now describes such a rule with an explicitunusablemarker instead of raising. Note this is the opposite choice from the exchange, deliberately: the exchange must refuse, because bound claims it cannot read are bound claims it cannot check, while the CRUD routes exist to tell an owner which rule is broken. TrustedIssuerwas left with the read-per-property shapeMappingRule.policy()was written to fix.validate_tokenwent back to the same row three times —jwks_uri, thenaudienceandissuer_url. The newTrustedIssuer.configuration()reads once, andJWKS_CACHE.signing_key()takes thejwks_uriits caller is already holding rather than reading it again. Pinned by a test asserting the read count, because nothing else would notice a third read coming back. Theissuer_claiming_urlSQL pushdown the reviewer also suggested was not done: the scan is now below the meter, which was the security half, and pushing the match into SQL is a larger change than this branch should carry.- Reclaiming a rule name destroyed the superseded rule's events.
hard_delete()deletes object events, and on a rule those events are the refusal trail — a stream of near-miss claim failures is what probing looks like. The natural response to spotting it is to delete the rule and write a tighter one under the same name, which is this exact path, so acting on the evidence erased it. BothMappingRuleandTrustedIssuernow record the supersession, with the superseded uuid, on the replacement object. Recorded there rather than on the namespace becausenamespace.pyimportsmapping_rule, and because the replacement is where somebody asking what happened to a rule will look. This preserves the fact and the identifier, not the trail itself; a fuller fix would copy the events, and the unique index means the old row still has to go.
Not done, and why:
- A CI assertion for the artifact upload gap. The reviewer
suggested one in
test_trustsalongside the delete assertions. Theshakenfist_clientpackage is not installed in the development environment, socache_artifact's signature could not be verified, and a functional test written against a guessed client API is worse than none. The unit coverage pins both halves of the fix instead, and each half was confirmed by reverting it and watching the specific tests fail. - A test that
reap_federation_recordsis registered on the cluster daemon schedule. True gap, but a uniform one: no test in the tree asserts schedule registration for any of the ten cluster tasks, and the block sits inside the elected loop where reaching it means refactoringdaemons/cluster/main.py. Covering one task and not the nine beside it would read as assurance that is not there. - A test that a
key_name_prefixcollision does not rotate an existing key. Investigated rather than accepted: the minted name carries an eight character random discriminator, so a collision is not reachable, and a test of that would be a test of probability. What was missing is the property the discriminator exists to protect — that the bare prefix is never used as a key name — so there is now a test that an operator's key named for the prefix still authenticates after a mint.
Merge queue failure (#3625)¶
The first attempt to merge failed for two unrelated reasons. Three matrix jobs died because the home lab pip mirror and proxy were unreachable for about two minutes, which is infrastructure and not this branch. The fourth found a real defect.
Every one of the sixteen tests in
cluster_ci_tests/test_federation.py failed in setUp with
400 jwks_uri must be https. _start_jwks_server() handed the issuer
an http:// address, and _validate_issuer_arguments has refused
non-HTTPS jwks_uri since the TrustedIssuer object was added in
6d9092944. The test, written later in 0a6f5d651, contradicted a
constraint that already existed. It went unnoticed because the
(collection) matrix is skipped on pull_request and only runs on
merge_group, so these tests had never executed.
The API is right and the test was wrong, so the test changed: the
throwaway JWKS server now speaks TLS behind a certificate it signs
itself, and the issuer is registered with an https:// address. No
loopback or private-address exemption was added to the validator. A
JWKS fetched over plaintext can be substituted by anyone on the path,
and a rule with a hole in it for the convenience of tests is a rule
that will eventually be exercised in production.
The cost is stated rather than hidden. The cluster verifies against
the system trust store, so it refuses a self-signed certificate and
never reaches the handler; _require_reachable_jwks sees the JWKS was
never served and skips. Eleven tests — issuer and rule CRUD, ownership
gates, validation, malformed and oversized bodies, the unauthenticated
route — now run for real. The five that need a live callback skip with
a message naming the reason. Issue #3639 tracks giving CI a
certificate the cluster trusts, and records that reachability is
almost certainly not the obstacle, since the tests run on the primary
node alongside the API they call.
This is also the item the second review round raised and this plan
declined, on the grounds that shakenfist_client was not installed
locally so the call could not be verified. That was wrong: the client
installs from PyPI into a scratch venv without difficulty, and doing
so reproduces the failure in seconds. The lesson is recorded in
AGENTS.md under "Cluster CI tests only run in the merge queue".
Third pull request review (#3625)¶
Eight items. Two fixed as defects, two as documentation, two as smaller corrections, one deferred to an issue, one needing nothing.
The damaged-rule handling did not work on a real cluster. This
phase put considerable effort into a rule whose bound_claims or
scopes will not decode: the exchange refuses it with a generic 401,
and external_view() marks it unusable so one bad row does not take
a namespace's listing down. Neither happened on a deployed cluster.
The decode runs inside sf-database, the servicer's catch-all turned
CorruptMappingRule into INTERNAL, and
_grpc_get_mapping_rule_attributes converts any RpcError into
DatabaseUnavailable — so the API answered 503 and both behaviours
were unreachable. Every test for them patched MappingRule._attributes
in-process, which is the only path where they worked.
This is the mistake this plan already named under "A guard has to sit
where the exception is raised", committed one layer down: the guard
was in the right place, and the fault could not reach it. The reply
now carries bool corrupt, the servicer sets it instead of falling
into the catch-all, and the client re-raises CorruptMappingRule.
str(e) is logged rather than passed to set_details(), because the
message names the rule uuid and the exchange is unauthenticated. The
new tests drive the real servicer method and the real client wrapper
rather than a patched attribute; reverting either half fails exactly
the three that should.
The replay key compared tokens as text. token_id was
sa.String(128), so it inherited the server's default collation —
utf8mb4_general_ci on 10.6, utf8mb4_uca1400_ai_ci from 11.4 — under
which two jti values differing only in case are one primary key, and
the second, legitimate, exchange is refused as a replay. It fails safe
rather than open, and the sha256 fallback is lowercase hex so it never
bites, but an issuer minting mixed-case base64 jti values is ordinary.
The obvious fix was wrong and testing caught it. utf8mb4_bin compares
case sensitively but is still PAD SPACE, so 'x' and 'x ' remained
one key; the column is now utf8mb4_nopad_bin, which has existed since
MariaDB 10.2, comfortably below MIN_MARIADB_VERSION of 10.6. Because
no unit test can answer "does this server think these are the same
key", the tests are live ones behind SF_MARIADB_TEST_DSN, verified
against dockerised MariaDB 10.6 and 11.4. tools/ci-enum-widening-test.sh
now runs every test_mariadb_*_live module rather than one by name —
standing up MariaDB is the expensive part, and the script and its job
keep their old names so the required status check does not move.
Two documentation gaps. The release notes recorded that a trust no
longer authorises delete, share, unshare, retag or metadata changes,
but not that it no longer authorises adding a version to an artifact it
previously gifted — so tooling that re-uploads the same source_url
across a trust breaks with a 404 and nothing said so. The operator
guide's "Giving is a separate question from taking" read as though
re-gifting worked. Both now state where the line falls, and both note
that labels are unaffected because a label URL names its own namespace.
Also: the comment on external_view()'s unusable field opened with
"Only ever True", which is wrong — the field is False for every healthy
rule, as its own test asserts. Two documentation files were missing
trailing newlines; there is no end-of-file-fixer in
.pre-commit-config.yaml, which is why nothing caught them.
Deferred. Three call sites ending in add_index still resolve
URLs with the visibility-based from_url — instance create, the label
endpoint, and the artifact fetch operation. None is exploitable the way
the upload route was: the instance path fetches from the owner's own
URL rather than caller-supplied bytes, and label URLs are
namespace-scoped. But this phase writes the ownership rule down as
universal, so leaving three unremarked counter-examples would mislead.
AGENTS.md now names them and issue #3640 tracks narrowing them,
rather than growing this pull request further.
No action. The rate limiter keying on remote_addr without a
ProxyFix was reviewed and agreed with as already analysed and
documented. The absence of functional coverage for a successful
exchange is the subject of #3639, above.
Follow-up: the deferred write paths (#3640)¶
3625 merged as c64269e63. This section records what closing #3640¶
found, because two of the three sites turned out to be misdescribed in the issue and the reasoning is worth more later than the diff.
The label endpoint was the real hole, and the issue called it the
safe one. #3640 recorded LabelEndpoint.post as "close to safe by
construction", because it resolves an sf://label/<namespace>/<name>
URL built from the request rather than taken from the caller. That
reading followed the construction and not the value: _label_url
accepts <namespace>/<label> and hands back the namespace it was
given. The route carries no ownership decorator, and the
requires_admin=True in its swag_from is prose appended to the
generated description. So any authenticated caller holding
label.write — which every legacy unscoped key does, through the
wildcard — could POST /label/<somebody else>/<label> and make its
own blob the newest version of their label, with
delete_old_versions taking the versions underneath. That is a wider
hole than the upload one #3625 closed, since it needed no trust and no
share.
It survived partly because the rest of the endpoint was broken.
_label_url returns a pair, and get and delete both handed the
whole pair to url_filter, which compares it against a string. Nothing
ever matched, and the resulting 404 was constructed but not returned,
so get fell through to an IndexError and delete to a NameError.
Both have answered 500 to every request since the pair was introduced
in 2024. An endpoint nobody could successfully read from is not one
anybody probed hard enough to find out what it would accept.
The instance path is where the obvious narrowing was the wrong fix.
Resolving disk.base with owned_from_url alone would have given every
namespace its own artifact for a shared image's URL — and
transfer_image treats an artifact with no versions as "cluster does
not have a copy", while _http_get_inner mints a fresh blob per fetch.
Every tenant would have downloaded and stored its own copy of every
shared image. The operator guide describes reuse as the entire point of
sharing one ("an official CentOS image that many users will want"), and
in the same paragraph says non-system namespaces "should not be able to
update such an artifact". Both halves of that sentence are the
requirement.
So the split is per verb rather than per artifact: a visible foreign
artifact is resolved to a blob and booted from, which is what the
label, snapshot and upload branches of the same loop already do, and
never fetched into. owned_from_url() picks the write target and
from_url() still picks what may be read. Two tests discriminate —
the enqueued fetch names a blob URL rather than the source URL, and its
artifact_uuid is not the foreign artifact's — and both fail when the
resolution is put back.
Artifact.owned_from_url_or_new() was added for the write paths whose
target namespace is fixed as the caller's own or already authorised.
They have no two cases to tell apart, so they get the create for free;
owned_from_url() itself still refuses to create, because the routes
which accept a caller-nominated namespace must authorise creating and
modifying separately.
Behaviour changes worth flagging at review. Updating a label across
a trust is now refused, which affects the ci-images pattern in the
operator guide: the first gift of a label works, a nightly republish
under the same name needs a key in the receiving namespace. This is the
same break #3625 already took for artifact uploads, and the guide
already said so for uploads — the label carve-out beside it was the
inconsistency, and it was wrong on the facts as well. Separately, a
tenant booting from a shared image's URL now pins to the blob that
image currently has rather than causing a refetch, which is the
intended reading of "should not be able to update".
Follow-up: the CI JWKS certificate (#3639)¶
The issue offered three options and said the choice needed making before anyone wrote code. None of the three was taken as written.
What the issue got wrong about the shape of the problem. It
assumed a CA installed into the node trust stores by the deploy
playbooks. Two facts undercut that. The tests run over SSH on the
primary node (smoke-cluster.yml scps the repo there and runs stestr
under /etc/sf/sfrc), while internal_ca bootstraps on the ansible
control node — so the CA key and the test process are on different
machines, and closing the gap means shipping a signing key to a
hypervisor. And internal_ca is a production role: widening it so
every deployment's SPICE CA also anchors outbound TLS is a real change
to production posture in service of a test.
What was done instead. FEDERATION_JWKS_CA_BUNDLE, an extra set
of trust anchors used only for JWKS fetches. This is a feature in its
own right rather than a test affordance — a self hosted Authentik or
Keycloak is normally behind a private CA, and an operator needs some
way to say so — and CI is simply its first user. It relaxes nothing:
jwks_uri must still be https, hostname and expiry are still checked,
and there is no skip-verification escape.
The anchors are added to the system set rather than replacing it,
and that is the part which is easy to get wrong.
ssl.create_default_context(cafile=...) loads the named file
instead of the system store, so the short spelling would mean that
configuring a private Authentik silently stopped GitHub tokens from
verifying — weeks later, when somebody added the second issuer. The
unit test asserts the union by count, and fails on the short spelling.
CI gets a throwaway CA from tools/ci-jwks-ca.sh, minted per run,
trusted for nothing but JWKS, with its key left on the primary for the
test to issue a leaf with; smoke-cluster.yml calls it before the
cluster suite. Putting a CA key on a node is only acceptable because
that cluster is destroyed within the hour, and the script says so
loudly. The deployment CA is untouched.
Verified locally rather than assumed, and it mattered. A harness
borrowed the real _tls_context, _ci_ca and _start_jwks_server
off the shipped test class, minted a CA with the script's exact
openssl invocation, and fetched through a real PyJWKClient built by
the real jwks_ssl_context. The first run failed: Missing Authority
Key Identifier. Python 3.13 turns on ssl.VERIFY_X509_STRICT by
default, and a leaf without an AKI is refused by it. The symptom is
indistinguishable from a cluster that does not trust the CA, so this
would have shipped as five tests still skipping, for a reason the skip
message does not mention. The leaf now carries AKI, SKI, basic
constraints and an extended key usage; with them the harness fetches,
and without the CA it is still refused with zero fetches — which is
the skip path working as intended.
Not closed by this. The five tests will only actually run once the
shakenfist/actions change merges; until then the fallback keeps them
skipping honestly. That change is a branch in the actions repo for the
operator to raise.
Pull request review (#3641)¶
Eleven items from the automated reviewer. Ten were acted on; one was declined with reasons.
The findings worth remembering.
- A fourth write path.
Instance.snapshot()resolved by visibility and fed the result toadd_index. The original #3640 sweep listed three sites because it was run overexternal_api/andoperations/, and this one is in the core object. Not exploitable — the URL carries the instance UUID and the type filter pins it — but that is an argument for the guard being cheap. The lesson, now in AGENTS.md: grep for the sink (add_index), not for callers of the resolver you happen to be changing. - A discarded error response is not free. The disk.base
fall-through called
_artifact_safety_checksfor its truthiness. That helper builds a Flask 404 andsf_api.errorlogsReturning API error: 404with a traceback as it does, so the ordinary case — a foreign artifact which exists and is not usable — wrote a refusal into the log of a request which returned 200. Split into_artifact_unusable_reason, a predicate returning a (log message, API message) pair, with the response building left where refusals actually happen. - The one setting that names a file. A wrong
FEDERATION_JWKS_CA_BUNDLEpath raisedFileNotFoundErrorfromPyJWKClientconstruction on the request path of the unauthenticated exchange endpoint, escaping asserver error: FileNotFoundError(2, ...)with no mention of the setting, forever (the client is never cached on that path). NowJWKSTrustAnchorUnusable, deliberately not aTokenValidationFailed: the token was never examined, so 401 would send the caller to their identity provider for a fault in our config file. The exchange answers 503, each gunicorn worker complains at startup, and the log names the setting and the path while the response does not. - The pushdown has a landmine. Pushing the ownership namespace to
SQL is correct and worth doing on the instance create path, but the
object iterator drops a namespace criterion of
systemso that listing as system sees the whole cluster. A pushdown replacing the Python predicate would silently turn an ownership test into no test. Both are kept, and there is a test for exactly that sabotage.
Declined: item 10, replacing the artifact fetch route's
resolve-then-fall-back block with owned_from_url_or_new. The review
read the request_namespace() not in [a.namespace, 'system'] guard as
a redundant restatement of the trust check above it. It is not — it is
strictly stronger, and it is the entire authorisation for the existing
artifact case. The trust is enough to gift a namespace an artifact it
lacked; replacing what one it already owns resolves to takes the owner
or system. That is the two-case split owned_from_url_or_new documents
itself as not being for. Comment added at the site so the resemblance
is not mistaken for duplication again.
Also done. set -u and explicit ${nodes:?} / ${primary:?}
guards in tools/ci-jwks-ca.sh (an unsourced environment previously
exited 0 having installed nothing, producing exactly the silent skip
the script exists to remove); the CA key scp'd straight into the 0700
directory rather than via world-readable /tmp; a post-restart check
that the setting actually landed, since readiness only proves sf-api
came back; LabelHierarchyTooDeep caught and answered 400 rather than
500 on all three verbs; release note for the disk.base pinning change;
function-level imports hoisted.
Coverage added: 24 tests across five files — the three resolvers
directly, Instance.snapshot, artifact_fetch_op._image_fetch, the
disk.base fall-throughs including the state-refused case and the
system-creates-elsewhere asymmetry, the malformed label name on all
three verbs, label creation by system into a namespace it does not
own, the two bad-bundle cases, and a functional
(cluster_ci_tests/test_artifacts.py) test of the whole label
authorisation split against a real cluster. Every fix was
sabotage-verified: revert, confirm the specific expected tests fail and
the controls hold, restore.
Back brief¶
Before executing any step of this phase, the implementing sub-agent must back-brief the management session on its understanding of the brief and surrounding context. The management session must present the 3g and 3h diffs for operator review before commit — the exchange endpoint is the security boundary this entire plan exists to create.
The 3g and 3h diffs were both presented. The operator elected to review the phase as a whole at the pull request rather than step by step, which is why both were committed before review.