Repository navigation
security(secrets): Secret.is_accessible_by has no production caller -- the secrets rules it models decide nothing #16982
Description
Activity
Chunking triage — a proposal, not an assignment
- Proposed priority:
priority: low— not applied — and the priority is moot if the recommended move happens first. - triage(v0.9.0): release criticality of the 123 open issues — 26 blocking, 36 small, 18 umbrellas, 41 misfiled, 2 undetermined #17639 criterion met: none of the five. Placed as misfiled: not blocking and not "landing now". triage(v0.9.0): release criticality of the 123 open issues — 26 blocking, 36 small, 18 umbrellas, 41 misfiled, 2 undetermined #17639 recommends moving it out of v0.9.0.
- triage(v0.9.0): release criticality of the 123 open issues — 26 blocking, 36 small, 18 umbrellas, 41 misfiled, 2 undetermined #17639 bucket: 4 — misfiled (not blocking and not "landing now")
- Umbrella / container: no.
- Pre-filter: clean — no merged commit on
origin/mainsince 2026-08-14 references this issue. - Basis: triage(v0.9.0): release criticality of the 123 open issues — 26 blocking, 36 small, 18 umbrellas, 41 misfiled, 2 undetermined #17639's per-issue row, reused rather than re-derived — "Dead security helper with no production caller (Secret.is_accessible_by); no exposure because real access goes through vault grants. AC1 is an owner ruling (wire vs retire). Target v0.10.0"
Nothing was relabelled, moved or closed by this pass.
- Proposed priority:
Investigation: the claim holds, but the policy is not dead — and that changes what (a) and (b) mean
"No production caller" is true, checked as a claim to disprove
is_accessible_byappears atmodels/secret.py:211(definition) and inmodels/secret_test.py. Nothing else. I also followed its delegates rather than stopping at the symbol, since a check reached through a helper looks uncalled to a grep of its own name.But the rule it models ships — just not for secrets
is_accessible_bydelegates toautobot_shared.scoping.is_visible. That primitive is production-enforced:knowledge/ownership.py:214calls it for KB ownership, with the same owner / scope / shared-with shape and its own grant lookup.So the situation is not "a security rule exists nowhere in production". It is: one shared policy primitive, enforced for knowledge and unwired for secrets. That distinction matters for (b) — retiring
is_accessible_bydoes not retire the policy, and the tests that look like they test secret policy are largely testingis_visiblethrough a secrets-shaped fixture.Why (a) needs the statement the issue asks for — the two checks are different kinds
rule kind EnvelopeSecretsService.read(:120-132)decrypt via any grant whose granteeis inaccessible_vaults, elseSecretAccessErrorcryptographic capability is_accessible_byowner / org / team / scope + session binding + shared_withmetadata policy The envelope rule is not a policy evaluation. It is possession of a wrapped DEK the caller's KEK can unwrap — access is denied by being unable to decrypt, not by a predicate returning
False.Consequence for (a): a metadata gate placed beside a cryptographic one can only subtract. It cannot grant access the grant denies, so "second gate" means "additional denial path", and it introduces a second place the access policy lives. That is worth saying plainly because it is the failure mode this repo keeps finding — two expressions of one rule, and drift between them is invisible until they disagree.
One argument I was about to make, and it is wrong — recording it because it nearly shipped
I was going to argue that
is_accessible_byevaluates fields that are never persisted, citing my #16450 finding thatorg_id/team_ids/shared_withare dropped before storage.That finding is about a different class. #16450 concerns
api/schemas_system.py'sSecretModelandSecretCreateRequest.to_secret_model(). This method belongs tomodels/secret.py'sSecret, whereowner_id,org_id,team_ids,session_idandscopeare all realmapped_columns. The fields are persisted here.Two classes named for the same concept, with different storage, and a finding that is true of one and false of the other. Checking the columns is what stopped it.
What I am not deciding
I am not choosing (a) or (b). Two reasons:
- (b) collides with the standing never-delete rule. Unwired code is unfinished work, and the disposition of a security-modelling method is an owner call rather than an implementer's.
- The strongest technical case for (b) — that its inputs are never populated — is the argument I just retracted. Without it, (b) rests on "the enforced rule is cryptographic and a metadata copy adds only denial paths", which is a real argument but a judgement about defence-in-depth, not a fact.
What would settle it: whether a deny that the vault grant would allow is ever wanted. If yes, (a) with the rule stated once and called from the read path. If no, (b).
What I am building now, which does not depend on the ruling
AC2 — "A guard pins that
is_accessible_byis either called from a read path or absent" — is implementable either way. The invariant is: the symbol is referenced from at least one non-test production module, or it does not exist. That holds under (a) and under (b), and fails exactly in today's state.That is the piece that stops this recurring, and it is the half I can deliver without the decision.
Owner ruling 2026-09-28: wire
is_accessible_byinto the secret read pathAsked and answered directly by the repository owner, relayed by the coordinating session, bundled with #12684's store ruling because the two questions collapse into one.
The vault grant is necessary, not sufficient. The canonical secret store enforces the metadata policy — visibility / organization / team / shared-with — in addition to the vault grant. There are reads the grant allows that the policy must still deny.
So
Secret.is_accessible_byis wired, not retired. It already delegates toautobot_shared.scoping.is_visible, which is enforced for KB ownership atknowledge/ownership.py:214; after this, one shared primitive has one enforcement state across both stores. The AC2 guard already written onissue-16982-unwired-access-check— failing on today's state with an actionable message — ships with the wiring.This also settles #16450's scope: the four controls are enforced, not persisted-as-metadata. #17099's UI path lands against the ratified canonical store per #12684.
Guard artefact location (relayed from the session that wrote it; unpushed by design).
- Branch
issue-16982-wire-access-check, commit9d4c0b59df— the repo-test guard assertingis_accessible_byis referenced from production or absent (disjunction on purpose: it fails in exactly today's state, and never has to be edited to permit the fix). - Must not be pushed alone — it fails on
mainby construction; it lands with the wiring. - Non-vacuity supports: >500 module-count floor on the sweep, and
test_the_sweep_would_notice_a_referencepointed atis_visible(production-wired atknowledge/ownership.py).repo_tests/excluded from swept roots so the guard cannot satisfy itself. - Deliberately weak in the permissive direction: any production reference passes; correct enforcement is the wiring's read-path test, not this guard's job.
No PR is open for this issue yet (#17732 is #16985's PR — its branch name is misleading). Whoever builds the wiring starts from
issue-16982-wire-access-check, not fromsecrets-v093— the duplicate there was removed.- Branch
Wiring analysis for this issue's half of the bundled ruling — posted in full on #12684, with the blocker summarised here
Read-only at
origin/mainab05ba3aef. Analysis only; no branch, no code.The vocabulary question has no open answer:
is_accessible_byalready speaks the canonical scope.models/secret.py:24-29—SecretScope = ScopeLevel, a deprecated alias since #11290 collapsed the three enums — so theself.scopecomparisons at:204,:206,:265areScopeLevelcomparisons, and:222-226documents the method as a thin adapter overautobot_shared.scoping.is_visible. No new type, no adapter layer, no vocabulary migration.One blocker to settle before the first commit, from
services/secrets_coordinator.py:103-111andservices/secrets_authz.py:52-61:readandlistholduser_idandfacts.team_ids, and neither they norPrincipalFactsholds asession_id._grant_lookup:204-205answers a SESSION-scoped secret withself.session_id == session_id, so wiring the check withsession_id=Nonemakes every session-scoped secret unreadable through the coordinator — as a clean refusal, with no error and the cause two frames away. Thread it from the request, or exclude session-scoped secrets from this path deliberately and say so at the call site. Either is defensible; the default is what breaks.Second, smaller:
is_accessible_bytakes a singleuser_org_idwhilefacts.company_rolesis aMapping[str, MembershipRole]. Given the 16:02Z ruling ratifies a company-scoped store, the company most likely comes from the secret's vault rather than from the principal — one value derived from the object, not a cardinality to resolve in the coordinator.The good news for the shape of the PR:
listalready returnslist[Secret](:109), i.e. objects that carry the method, so the wiring is a post-authorizefilter plus the same check onread. The owner's "necessary, not sufficient" composes exactly:facts.accessible_vaults()is the vault grant,is_accessible_byis the per-secret scope.The guard at
9d4c0b59dfpins referenced-or-absent, which is the right pin — it fails onmainby construction and passes once the call site exists.Not taken by this session. My scope here is analysis and issues; the PR is re-routed. Recording this so whoever takes it starts from a decided question rather than re-deriving it.
Decision needed before the first commit: session-scoped secrets on the coordinator path
Surfaced by the read-only analysis above (a4).
services/secrets_coordinator.py:103-111—read/listtakeuser_id+permissions, buildPrincipalFacts(services/secrets_authz.py:52-61:user_id,is_admin,team_ids,role_names,company_roles,granted_permissions,active), and passfacts.accessible_vaults(). Nothing on that path carries asession_id.is_accessible_by's_grant_lookup(models/secret.py:204-205) answers a SESSION-scoped secret withself.session_id == session_id, so wiring the check withsession_id=Nonemakes every session-scoped secret unreadable through the coordinator — a clean refusal, no error, cause two frames away.Options
option consequence A Thread session_idfrom the request intoread/list(signature change on the coordinator, the callers pass what the request already knows)session-scoped secrets keep working through the coordinator; the check is complete; one more argument on a two-call surface B Exclude session-scoped secrets from this path deliberately, recorded at the call site smaller diff; a whole scope class silently bypasses the new enforcement, which is the failure shape this issue exists to remove Recommendation: A. B re-creates the "check exists but the path does not run it" state for one scope. If the request path genuinely has no session concept at the coordinator's callers, that is a finding to record here, not a reason to pick B by default.
Second, smaller: the check takes one
user_org_idwhilefacts.company_rolesis a mapping — derive the company from the secret's vault (one value from the object), not a cardinality resolved inside the coordinator.Owner: answer here or the implementing session (63) proceeds with A when this PR starts (it is behind #15907 and the caretaker fixes in the queue). Provenance: posted by the coordinating session; the analysis is a4's, on #12684.
The wiring guard is weaker than its name — recorded before anyone reads it as satisfied.
9d4c0b59df's guard assertsis_accessible_byis referenced from a production module. A reference is not a reachable call: mutatingSecretsCoordinatortoreturn Trueabove the call site leaves the guard green. It still does its job for the case it was written for (the symbol absent from production entirely), and it was deliberately weak in the permissive direction — but this issue's criterion is that no access rule lives only in uncalled code, andreferenceddoes not establishcalled. #17772 adds three tests that driveread/listwith fakes and assert the value is never decrypted when the scope refuses, which is the reachability half. Same family as #17769/#17761: a guard whose mechanism is narrower than the claim its name makes.And one correction to the org analysis above, verified at the primitive.
autobot_shared/scoping/visibility.py:80isresource.company_id is not None and resource.company_id == principal.company_id— so supplying the secret's own company as the principal's company makes the ORGANIZATION comparison tautological and every org-scoped secret readable by anyone who reaches the vault. PassingNoneis the opposite failure: every org-scoped secret unreadable. The implementation therefore takes the candidate company from the secret and supplies it only when the principal actually holds it — one value, cardinality resolved, rule intact. The framing of the problem was right and is what made this findable; the recommendation needed the extra step.Remaining scope after #17772, so the criterion is not read as met: the coordinator's read path now asks the secret's scope, but two production callers reach
SecretsService.readdirectly and still get the vault grant alone —llm_shared/provider_auth.py:177andservices/credential_reconcile.py:101(verified onorigin/main). Both may be legitimate service principals; the defect is that nobody has said so, and the wiring guard cannot tell — any single production reference satisfies it, so a third direct caller would appear silently. Filed as #17773 with a per-caller verdict, a stated sweep, and the guard strengthened from referenced to the coordinator is the only user-facing entry. This issue's criterion — no access rule living only in uncalled code — is met for the coordinator and open for those two.
Problem
Secret.is_accessible_by()(models/secret.py) is a security check with no production caller. Its only callers aremodels/secret_test.py. It models secret access asis_visible()over owner, org, team and scope, plus_grant_lookup(session binding,shared_with).Production secret access does not go through it:
SecretGrantfor one of the caller's vaults (EnvelopeSecretsService.read/list_for_vaults).credential_read,json_secrets_read,llc_secrets_read).A reader of
is_accessible_byconcludes the secrets rules live there. They do not, and a change to it changes nothing that ships.Found on #16927: the owner ruling's first draft named it "the real access check" for secrets. #16927 fixed its
str(self.owner_id)stringification (a null would have read as the truthy"None"), but the method still decides nothing.Decide
Acceptance criteria
is_accessible_byis either called from a read path or absent