diff --git a/nix/host-modules/swarm-bao.nix b/nix/host-modules/swarm-bao.nix index 20fe13db..e4a3927d 100644 --- a/nix/host-modules/swarm-bao.nix +++ b/nix/host-modules/swarm-bao.nix @@ -362,18 +362,16 @@ let # ../module-eval.nix cannot read them, and a heredoc would make the HCL's # indentation a function of this file's. # - # The last grant is a different kind from the others: they let the controller - # bootstrap hives, this lets it write an agent's credentials. Two things about - # it do not read as they look. - # - # `secret/data/` is KV v2's ACL prefix, not part of the path the code passes: - # `swarm-secret-client` writes `swarm/agents//...` under mount - # `secret`, and the engine inserts `data/`. Matching the code's spelling - # literally would grant nothing. - # - # `read` too: `mint_and_verify` reads a credential back before writing so a - # re-run keeps the value a live agent already holds instead of rotating it — - # the read is required, not incidental. + # The credential-write grant below (unlike the bootstrap ones above it) has + # three things about its paths that do not read as written. `secret/data/` is + # KV v2's ACL prefix, not part of the path the code passes: + # `swarm-secret-client` writes `swarm/agents//...` under mount `secret`, + # and the engine inserts `data/` — matching the code's spelling literally would + # grant nothing. `read` is required too: `mint_and_verify` reads a credential + # back before writing so a re-run keeps the value a live agent already holds + # instead of rotating it. `metadata/` is the revocation half: `delete` on + # `data/` only soft-deletes the newest version, and `+` being one path segment + # keeps this to the queue leaf alone. # # The swarm appservice token and its own OIDC client secret, read-only: it # uses both and writes neither. matrix-ctl publishes the token @@ -402,6 +400,10 @@ let capabilities = ["create", "read", "update"] } + path "${credentialMountPath}/metadata/swarm/agents/*" { + capabilities = ["delete"] + } + path "${credentialMountPath}/data/${swarmAppserviceTokenLeaf}" { capabilities = ["read"] } diff --git a/nix/module-eval/bao-grants.nix b/nix/module-eval/bao-grants.nix index cd66ed66..a02fb9d3 100644 --- a/nix/module-eval/bao-grants.nix +++ b/nix/module-eval/bao-grants.nix @@ -1147,6 +1147,25 @@ let in lib.hasInfix "path \"secret/data/swarm/agents/*\" {\n capabilities = [\"create\", \"read\", \"update\"]" s; } + { + # Revocation, and the reason it is a stanza of its own: `delete` on the + # `data/` path soft-deletes the newest version and leaves earlier ones + # readable, so a credential the mint had ever rewritten would survive it. + # `metadata/` is the path that removes every version, and the store ACLs + # it separately — without this grant the revocation is a 403 and a + # destroyed agent's credential stays valid. + # + # Pinned as the whole capability list for the same reason as the stanza + # above: `read` or `list` here would hand a write-only principal the + # version history of every agent's secrets. + name = "the controller may revoke an agent credential, and only by removing every version of it"; + ok = + let + s = baoGrantHere.systemd.services.swarm-bao-controller-policy.script; + in + lib.hasInfix "path \"secret/metadata/swarm/agents/*\" {\n capabilities = [\"delete\"]" s + && !(lib.hasInfix "secret/metadata/*" s); + } { # The swarm appservice token is a homeserver-admin credential. The # controller mints agents' accounts with it and has no business replacing diff --git a/swarm-controller/src/agent_identity.rs b/swarm-controller/src/agent_identity.rs index 1bab3eac..3928fbe9 100644 --- a/swarm-controller/src/agent_identity.rs +++ b/swarm-controller/src/agent_identity.rs @@ -220,6 +220,54 @@ pub async fn mint_and_verify(agent: &str) -> Result<()> { Ok(()) } +/// Revoke `agent`'s queue credential: delete the path +/// [`mint_and_verify`]'s step 3 published, and everything ever written at it. +/// +/// The undo of that one step and of no other. The leaf, the ACL document and +/// the cert-auth role that make up the rest of an agent's identity stay where +/// they are — they are what a *hive* uses to collect an agent's secrets, they +/// are minted afresh on every run of the mint, and tearing them down is not +/// what the queue credential outliving its holder is about. +/// +/// **Deletes every version, not the newest one.** The mint rewrites this path +/// whenever the principal it names has to be corrected, so a soft delete would +/// leave the identical secret sitting in version history, readable at +/// `?version=N` by anything that can read the path at all — a value still +/// recoverable has not been revoked. See +/// [`SecretStore::delete_all_versions`][swarm_secret_client::SecretStore::delete_all_versions]. +/// +/// **Idempotent**: revoking an agent that never had a credential, or one +/// already revoked, succeeds and says so. A teardown that runs twice is +/// ordinary, and a second run that failed would be a worse fault than the one +/// this exists to fix. +/// +/// # Errors +/// When the store cannot be reached or refuses the delete. The caller decides +/// what that costs — `set_agent_state` logs it and lets the destroy proceed, +/// since a credential that is still live is a smaller harm than an agent that +/// cannot be torn down. +pub async fn revoke_queue_credential(agent: &str) -> Result<()> { + let queue_path = queue::agent_queue_path(agent)?; + let store = crate::store::connect() + .await + .context("logging in to the swarm secret store")?; + store + .delete_all_versions(&queue_path) + .await + .with_context(|| format!("revoking the agent queue credential at {queue_path}"))?; + // At `info` and unconditional: an unlogged revocation is indistinguishable + // from a leak, and this line is the only record an operator has that the + // credential stopped being usable. It cannot say whether one was there — + // the controller's grant on these paths is write-only by design, so it + // deletes blind. + tracing::info!( + agent, + %queue_path, + "agent queue credential revoked: every version of the path deleted" + ); + Ok(()) +} + /// The consumer of everything [`mint_and_verify`] wrote: log in **as the /// agent**, with the leaf just issued, and read back both paths just /// published. diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index cacd1454..32996cc7 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -1050,9 +1050,55 @@ async fn set_agent_state( tracing::warn!(hive = %hive, agent = %agent, error = %format!("{e:#}"), "declaring agent state failed"); error_problem(wanted_error_status(&e), &format!("{e:#}")) })?; + + // After the declaration and never before it: the write above is the + // destroy order, and until it lands the agent is not being torn down, so + // its credential has to keep working. Sequenced rather than spawned so + // the revocation attempt is finished — and its outcome logged — by the + // time the operator's call returns. + if revokes_queue_credential(req.state) { + revoke_or_complain(&hive, &agent).await; + } Ok(Json(render(&declaration))) } +/// Whether declaring an agent into `state` ends its queue credential's life. +/// +/// Exhaustive on purpose, like every other match on [`AgentState`] in this +/// tree: a state added later has to say whether it revokes rather than +/// inheriting "no" from a catch-all. `Offline` and `Paused` deliberately do +/// not — both are states an agent comes back from, and revoking on either +/// would mean an agent that could be stopped but never restarted. +fn revokes_queue_credential(state: swarm_queue_client::wanted::AgentState) -> bool { + use swarm_queue_client::wanted::AgentState; + match state { + AgentState::Destroyed => true, + AgentState::Up | AgentState::Offline | AgentState::Paused => false, + } +} + +/// Revoke the credential, and make a failure impossible to miss without +/// letting it stop the teardown. +/// +/// **The destroy is not blocked by this.** The declaration is already +/// published, the hive will converge on it, and refusing the operator's call +/// at this point would leave them with an agent they cannot destroy — the +/// larger of the two harms. What is left is that the failure must not be +/// quiet, or the outcome is the silent orphan this whole path exists to +/// remove: hence `error`, naming the agent, with the cause chain attached. +async fn revoke_or_complain(hive: &str, agent: &str) { + if let Err(e) = agent_identity::revoke_queue_credential(agent).await { + tracing::error!( + hive, + agent, + error = %format!("{e:#}"), + "revoking this agent's queue credential FAILED; it was destroyed anyway, so \ + the credential outlives it and may still authenticate. Declaring the agent \ + destroyed again re-runs this revocation." + ); + } +} + /// What this swarm currently declares for a hive. /// /// Read back from the bucket rather than from a second copy kept here: the diff --git a/swarm-secret-client/src/client.rs b/swarm-secret-client/src/client.rs index 4b5d8e05..98f2002b 100644 --- a/swarm-secret-client/src/client.rs +++ b/swarm-secret-client/src/client.rs @@ -211,6 +211,41 @@ impl SecretStore { Ok(()) } + /// Delete `path` outright: every version of it, and the metadata that + /// lists them. + /// + /// The undo of [`write`][Self::write], and deliberately **not** KV v2's + /// plain delete. That one soft-deletes the newest version only, leaving + /// every earlier one readable at `?version=N` and the newest one + /// recoverable by `undelete` — so a caller revoking a bearer secret that + /// [`write`][Self::write] has ever replaced would leave the old value + /// sitting in version history, still usable. `metadata` is the one verb + /// that removes the lot. + /// + /// Addresses `secret/metadata/`, which the store ACLs separately + /// from the `secret/data/` the other methods here use: a token + /// granted write on the data path is refused at this one until its policy + /// names the metadata path too. + /// + /// **Idempotent.** Deleting a path that holds nothing succeeds, because + /// the caller is tearing something down and a teardown that runs twice — + /// or on a principal that never got a secret — must not fail the second + /// time. Only a 404 is absence, exactly as in + /// [`read_optional`][Self::read_optional]: a 403 stays an error, or a + /// principal whose grant does not cover the path would report a + /// revocation it never performed. + /// + /// # Errors + /// [`Error::Vault`] for anything that is not a 404 — a denial, or a store + /// that could not be reached. + pub async fn delete_all_versions(&self, path: &str) -> Result<(), Error> { + match vaultrs::kv2::delete_metadata(&self.inner, MOUNT, path).await { + Ok(()) => Ok(()), + Err(e) if is_absent(&e) => Ok(()), + Err(e) => Err(e.into()), + } + } + /// Replace the ACL policy named `name` with `policy`. /// /// A whole-document write, not a merge: the store has no other verb, and @@ -308,6 +343,17 @@ impl SecretStore { } } +/// Whether a failed store call means the path holds nothing — the one status +/// [`SecretStore::delete_all_versions`] is allowed to treat as success. +/// +/// The same rule [`SecretStore::read_optional`] documents, named here because +/// the delete side has to be able to state it in a test: a 403 is a principal +/// whose grant misses the path, and swallowing that would let a caller report +/// a revocation the store refused to perform. +fn is_absent(e: &vaultrs::error::ClientError) -> bool { + matches!(e, vaultrs::error::ClientError::APIError { code: 404, .. }) +} + /// Write an ACL policy, spelled out here because [`vaultrs`] does not have it: /// its `sys::policy` module targets `sys/policy/`, the deprecated alias, /// and the store ACLs that path separately from `sys/policies/acl/`. A @@ -554,4 +600,60 @@ mod tests { assert!(!e.to_string().contains("SECRET-KEY-BYTES"), "{e}"); } } + + /// What makes [`SecretStore::delete_all_versions`] safe to run twice: the + /// second run finds nothing and must still succeed. A teardown that + /// hard-errors on an already-revoked credential is a worse fault than the + /// credential outliving its holder. + #[test] + fn a_path_that_holds_nothing_is_not_a_failed_delete() { + assert!(is_absent(&vaultrs::error::ClientError::APIError { + code: 404, + errors: vec![], + })); + } + + /// The other half, and the one that matters for a revocation: a token + /// whose policy misses the path is refused with a 403, and reading that + /// as "nothing was there" would log a revocation the store never made. + #[test] + fn a_denial_is_not_absence() { + for code in [400, 403, 500, 503] { + assert!( + !is_absent(&vaultrs::error::ClientError::APIError { + code, + errors: vec![], + }), + "{code} must stay a failure" + ); + } + } + + /// Revocation addresses `secret/metadata/…`, not `secret/data/…`. + /// + /// The distinction is the whole point of the verb: the data path's delete + /// leaves earlier versions readable, so revoking a secret that was ever + /// rewritten there would leave the old value recoverable. It is also a + /// separately-ACL'd path, which is why the grant had to gain a stanza. + #[test] + fn a_revocation_targets_every_version_and_not_just_the_newest() { + use rustify::endpoint::Endpoint as _; + + let request = vaultrs::api::kv2::requests::DeleteSecretMetadataRequest::builder() + .mount(MOUNT) + .path("swarm/agents/atlas/queue") + .build() + .expect("the request builds"); + assert_eq!(request.path(), "secret/metadata/swarm/agents/atlas/queue"); + + // The control: the verb deliberately not used, which addresses the + // data path and soft-deletes exactly one version of it. + let soft = vaultrs::api::kv2::requests::DeleteLatestSecretVersionRequest::builder() + .mount(MOUNT) + .path("swarm/agents/atlas/queue") + .build() + .expect("the request builds"); + assert_eq!(soft.path(), "secret/data/swarm/agents/atlas/queue"); + assert_ne!(request.path(), soft.path()); + } }