feat(cli): agent registry — list, pause, resume, revoke, and per-agent audit replay - #669
Conversation
there was no way to answer "which agents can write to this kb, and what did each one do". bearer tokens are matched in trust.py and hashed into an auth_subject, but nothing enumerated them, nothing could suspend one, and nothing mapped a subject back to a readable name. removing a token from config was the only revocation available, and it took down every agent sharing that token. the registry is keyed on auth_subject, so registering an agent never requires storing, echoing or even seeing its credential. that is what lets it live in committed .vouch/agents.yaml — names, status, scopes and claim dates are reviewable in a pr while the token stays in local config, the same split secrets.py already draws. `vouch agents subject <token>` derives the key without persisting anything. pause and revoke are enforced at one chokepoint. trust.authorized_bearer_token wraps the existing token match and takes the registry gate as an injected callable, so trust.py keeps no storage dependency and both http transports inherit revocation rather than implementing it twice. a denied token returns None — indistinguishable from a wrong token, so a paused agent learns it is not authenticated and nothing more. revocation is terminal on purpose. a revoked credential is one you have decided to stop trusting, and an undo button on that is a footgun; re-admitting an agent means issuing a new token, which is a new subject. existing deployments are unaffected. an unregistered token authenticates exactly as before, as an unnamed active agent, so registration is opt-in rather than a migration. a status that fails to parse resolves to revoked, not active — a corrupted registry must not become an authentication bypass. `vouch agents show` replays the audit log for one agent: what it did, and what was done to it, on one timeline, because "proposed 40 claims, then was paused" is the sentence an operator is trying to read. this is the half ditto's own docs stop short of. last-used is derived from the audit log rather than stored. keeping a column current would mean a disk write on every authenticated request, on the auth path, for a field nothing reads in the hot loop. Closes vouchdev#607
github-actions
Bot
added
docs
# Conflicts: # CHANGELOG.md # src/vouch/cli.py
plind-junior
left a comment
plind-junior
left a comment
There was a problem hiding this comment.
the shape is right and the chokepoint argument holds up — trust.authorized_bearer_token with an injected gate keeps trust.py free of a storage import, both HTTP call sites move together (http_server.py:208, :291), and a denied token returning None is the correct indistinguishability. keying on auth_subject so the committed registry never holds a credential is the right call and is genuinely pinned by test_register_names_a_subject_without_storing_the_token. staying out of kb.* is also right — pausing a credential over the transport being gated would be a mistake.
but three of the properties the description asks a reviewer to check do not hold as written. i ran these against the head commit of this branch:
=== corrupt agents.yaml -> revocation silently lost ===
load_registry : []
is_active : True
subject_is_active : True
authorized_bearer : s3cret-token-example <- revoked agent authenticates
=== replay(limit=0) ===
total events : 2
limit=0 : 2 <- expected 0
=== name-as-actor ===
audit event with actor='ci-bot' (no token) ->
{'event': 'proposal.claim.create', 'by_agent': True}
last_seen moves forward off that event
inline on each. the fail-open one is the serious one: "a corrupted status fails closed" is true for a bad status: value and false for a bad file, and the file-level path is the one a partial write actually produces. the other two are small — an off-by-zero slice and a one-line set literal — and nothing in the 42 tests pins the behaviour that would have to change, so the fixes are cheap.
no objection to the design decisions the description flags as debatable: terminal revocation behind a confirmation prompt reads right, deriving last-used from the audit log instead of writing on the auth path reads right, and deferring per-scope enforcement to #608 rather than half-shipping an authorization model is the correct instinct.
| loaded = yaml.safe_load(path.read_text(encoding="utf-8")) | ||
| except (OSError, yaml.YAMLError): | ||
| return [] | ||
| rows = loaded.get("agents") if isinstance(loaded, dict) else loaded | ||
| if not isinstance(rows, list): | ||
| return [] |
There was a problem hiding this comment.
[blocking] an unparseable file fails open, which is the opposite of the row-level property the description advertises. any OSError / YAMLError — and any non-list agents: — returns [], is_active then treats every subject as unregistered (agents.py:272), and subject_is_active wraps the whole thing in except Exception: return True (agents.py:287, :292). so a corrupt registry silently re-admits every revoked credential:
# agents.yaml = "{{ not yaml", ci-bot previously revoked
load_registry : []
is_active : True
subject_is_active : True
authorized_bearer : s3cret-token-example
tests/test_agents.py:276 currently pins this as intended (test_unreadable_registry_is_not_fatal), so it is a decision rather than an oversight — but it is the wrong one for a denylist, and it undercuts the "a corrupted registry must not become an authentication bypass" comment at agents.py:116-117.
the distinction that matters is absent vs present-but-unreadable. absent means fail open — that is the documented opt-in path and existing deployments depend on it. present and it did not parse should fail closed, or at minimum deny every token and log loudly. suggest a sentinel return (or a RegistryUnreadable raise) that is_active / subject_is_active translate to "deny", leaving only the genuinely-missing case allowing.
| path.write_text( | ||
| yaml.safe_dump({"agents": [a.to_dict() for a in agents]}, sort_keys=False), | ||
| encoding="utf-8", | ||
| ) |
There was a problem hiding this comment.
[non-blocking] this is what makes the comment above reachable in practice. a plain write_text over the live path means a crash or a full disk mid-write leaves a truncated agents.yaml — and because the file is a list of block mappings, truncation usually still parses, so the tail rows just vanish with no error at all:
truncated file parses: ['ci-bot'] # 'other-bot' row silently gone
ci-bot row truncated -> is_active(revoked subject): True
plain write_text is the house pattern in storage.py, and there it is fine — one artifact per file, so a bad write loses one claim. agents.yaml is the whole denylist in one file, so a partial write loses revocations. worth a temp-file + os.replace here even though the rest of the codebase does not bother.
| # was registered are attributed to whatever actor string the transport | ||
| # recorded then, and dropping them would make the replay lie by omission. | ||
| actors = {agent.actor, agent.name} |
There was a problem hiding this comment.
[blocking] matching on the readable name makes the replay spoofable, and the stated reason for it does not hold.
for a token call, _agent() in both transports returns f"token:{subject}" whenever a subject is present (server.py:96-99, jsonl_server.py:111-114) — before and after registration. registering an agent does not change the actor string, so there is no population of pre-registration events recorded under the readable name to rescue.
what the name match does admit is the other direction: the name only ever appears as an actor on an unauthenticated call — the client-supplied X-Vouch-Agent header (jsonl_server.py:114), VOUCH_AGENT, or cli.py:_whoami() falling through to the OS username. all three are caller-controlled, which is exactly why issue #607 rules VOUCH_AGENT out: "an agent can set it to anything. it cannot be the basis for revocation."
audit event, actor='ci-bot', no token involved:
{'event': 'proposal.claim.create', 'actor': 'ci-bot', 'by_agent': True}
so VOUCH_AGENT=ci-bot vouch ... writes into another agent's audited history with by_agent: true, and a human whose OS username happens to match a registered name gets silently folded in too. suggest actors = {agent.actor}. if deliberate aliasing is wanted later, an explicit aliases: column an operator sets is the honest version of it. nothing in the 42 tests covers the name branch, so this is a one-line change.
| would mean a disk write on every authenticated request, on the auth path, | ||
| for a field nobody reads in the hot loop. The audit log already knows. | ||
| """ | ||
| actors = {agent.actor, agent.name} |
There was a problem hiding this comment.
[blocking] same set literal, and here the consequence is sharper than a noisy replay: last-used is the field an operator reads to answer "is this credential dormant?". with the name included, anyone setting VOUCH_AGENT to a registered name moves that agent's last-used forward, so a revoked or paused credential can be made to look freshly active in vouch agents list. actors = {agent.actor} here too.
| if limit is not None and limit >= 0: | ||
| return events[-limit:] |
There was a problem hiding this comment.
[blocking] limit=0 returns everything. events[-0:] is events[0:], so vouch agents show ci-bot --limit 0 prints the full trail instead of nothing — the opposite of what the flag says, and unbounded on a real audit log.
total events : 2
limit=1 : 1
limit=0 : 2 <- expected 0
limit=-1 : 2
return events[-limit:] if limit else [] fixes it. negatives currently fall through to "everything" via the limit >= 0 guard, which is at least harmless, but the cli takes type=int with no floor (cli.py:3835) so it is worth rejecting there rather than leaving it undefined.
| @agents_group.command("subject") | ||
| @click.argument("token") | ||
| def agents_subject(token: str) -> None: | ||
| """Print a token's auth subject without storing the token.""" | ||
| click.echo(trust_mod.auth_subject_for_token(token)) |
There was a problem hiding this comment.
[non-blocking] the credential arrives as an argv positional, so it lands in shell history and is readable from ps / /proc for the life of the call. test_cli_subject_prints_a_hash_not_the_token covers stdout but not argv, and this command is the one place in the feature that touches the raw token — the same design instinct that keeps it out of agents.yaml should keep it off the command line. reading from stdin when the argument is absent or - is a couple of lines:
token = token if token and token != "-" else click.get_text_stream("stdin").read().strip()then vouch agents subject - < token.txt and echo "$CI_TOKEN" | vouch agents subject - both work, and the documented example in the description stops teaching the unsafe form.
| def test_http_transports_use_the_gated_chokepoint() -> None: | ||
| """Both HTTP entry points must inherit revocation, not just one.""" | ||
| from pathlib import Path as _Path | ||
|
|
||
| src = _Path("src/vouch/http_server.py").read_text(encoding="utf-8") |
There was a problem hiding this comment.
[non-blocking] this asserts on source text via a relative path, so it depends on the runner's cwd and on nobody ever mentioning matched_bearer_token( in a comment in this file. the property it is reaching for is behavioural and testable directly: register and revoke a subject, then drive each transport with that bearer token and assert the request is unauthenticated. that survives a refactor that keeps the guarantee, and fails on one that quietly drops the gate= on a single call site — which the substring count would also catch today, but only by accident of the number 2.
|
diff coverage: n/a — this PR changes no python under |
2 participants
Closes #607
there was no way to answer "which agents can write to this kb, and what did each one do". tokens are matched in
trust.pyand hashed into anauth_subject, but nothing enumerated them, nothing could suspend one, and nothing mapped a subject back to a name. removing a token from config was the only revocation available, and it took down every agent sharing that token.the decision that shapes everything else
the registry is keyed on
auth_subject, never on the token.vouch agents subject <token>derives the sha256 prefixtrust.pyalready computes and persists nothing. that is what makes a committed.vouch/agents.yamlsafe: names, status, scopes and claim dates are reviewable in a PR, while the credential stays in local config — the splitsecrets.pyalready draws, and the answer the issue proposes to its own open question.test_register_names_a_subject_without_storing_the_tokenasserts the token string never reaches the file.one chokepoint, and no storage dependency in trust.py
trust.authorized_bearer_tokenwraps the existingmatched_bearer_tokenand takes the registry gate as an injected callable. sotrust.pygains no import of storage, and both HTTP transports inherit revocation rather than implementing it twice —test_http_transports_use_the_gated_chokepointasserts the count is 2 and that no ungated call site survives.a denied token returns
None, i.e. indistinguishable from a wrong token. a paused agent learns it is not authenticated and nothing more.three properties i'd want a reviewer to check, all pinned:
status:resolves torevoked, notactive— otherwise a mangled registry becomes an authentication bypass.test_unreadable_status_fails_closed.revocation is terminal
deliberately. a revoked credential is one you have decided to stop trusting, and an undo button on that is a footgun. re-admitting an agent means issuing a new token, which is a new subject and a new row. the CLI puts it behind a confirmation prompt. if you would rather revoke be reversible, that is a one-line change to
set_status— but i think the sharp edge is the point.vouch agents show— where this beats the thing it's copyingthe issue notes ditto's docs "stop short of" per-action audit. vouch has an append-only log, so the replay shows both what the agent did (it is the actor) and what was done to it (control-plane transitions naming its subject), on one timeline, flagged with
by_agent:"proposed 40 claims, then was paused" is the sentence an operator is trying to read; splitting it across two commands would hide the causal bit.
last-used is derived, not stored
i dropped a stored
last_seen_atafter writing it: keeping it current means a disk write on every authenticated request, on the auth path, for a field nothing reads in the hot loop. the audit log already knows, soagents.last_seenderives it.test_last_seen_is_derived_from_the_audit_logalso asserts it never lands in the committed registry.scope note
this is the registry only. per-scope enforcement (
kb:read/kb:propose/kb:approve) is #608 —scopeshere is recorded metadata that #608 can enforce against, not a permission check pretending to be one. i did not want to half-ship an authorization model.no
kb.*method either: pausing and revoking credentials is an operator act, not something an agent should do to itself over the very transport being gated.tests
42 cases in
tests/test_agents.py: registry round-trip and duplicate/validation refusals, every status transition and its audit event (includingreversible=Falseon revoke and the no-op path), the gate across all three statuses plus the no-gate and wrong-token paths, fail-closed on a corrupt status, malformed/unreadable registry handling, replay separating did-vs-done-to with limits, derived last-used, and the full CLI surface in text and json including the confirmation prompt and clean domain errors.verification