swarm: revoke an agent's queue credential when it is declared destroyed
A per-agent queue credential is minted at agent creation and nothing has ever removed it. An agent declared destroyed loses its container and keeps its credential: a bearer secret recovered from a snapshot or a stale capture still authenticates as that agent, so the set of usable credentials only grows. Delete the path the mint published, on the one transition that ends an agent's life. It mirrors step 3 of `mint_and_verify` and no other step: the leaf, the ACL document and the cert role are what a hive uses to collect an agent's secrets and are re-minted on every run of the mint. Every version, not the newest. The mint rewrites the path when the principal it names needs correcting, so KV v2's plain delete would leave the identical secret readable at ?version=N. That is a separately-ACL'd path, hence the second stanza in the controller's grant -- `delete` on metadata discloses nothing, and `update` on the data path already lets this principal destroy any agent credential's usability. The destroy is not blocked by a failed revocation: the declaration is already published and refusing the call would leave an operator with an agent they cannot tear down. The failure is logged at error instead, naming the agent, since a silent orphan is the fault being removed.
This commit is contained in:
parent
88c386c96c
commit
8caf688ee4
5 changed files with 229 additions and 12 deletions
|
|
@ -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/<path>`, which the store ACLs separately
|
||||
/// from the `secret/data/<path>` 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/<name>`, the deprecated alias,
|
||||
/// and the store ACLs that path separately from `sys/policies/acl/<name>`. 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());
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue