From d1c0963fbd3bc0fa7d01175296c66685a9cbc8d0 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 8 Sep 2026 00:20:11 +0200 Subject: [PATCH] swarm-secret-client: carry the homeserver with the credential, not on the notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A delivered matrix account needs two things: the token and the homeserver it belongs to. Only the token was stored, so the homeserver had to ride on the queue notice — and a notice is not persistence. Re-delivering a credential (agent moved, hive re-provisioned, token rotated) has to reconstruct it from somewhere, and there is nowhere; keeping it separately at swarm level would be a second store for one logical object, free to drift from the first. So `Credential` grows a `homeserver` field and `read`/`write` carry the whole object rather than a bare string. `value` keeps its name. `nix/host-modules/glue-matrix-bao-token.nix` reads the store with `bao kv get -field=value` and is the only nix reader of it, checked rather than assumed — so this had to be an addition, never a rename. Two compatibility properties, both of which fail silently if broken: KV2 keeps every prior version, so objects written before this field existed are still decoded by this type. What tolerates their absence is the field being `Option` — a bare `String` would not fail as a migration, every stored credential would become unreadable at once. The new test pins that, with a presence control so the arm is about absence being tolerated rather than the field being ignored. `skip_serializing_if` keeps a token-only credential serialising to exactly the bytes the previous version wrote, with no `homeserver` key rather than a null, which is what that nix reader would otherwise trip over. The existing test pinning `{"value":"t"}` proves it and became the control for free. Mutation testing earned its place here: `#[serde(default)]` was in the first draft and its comment claimed it was what made old objects decode. Dropping it changed nothing — serde already decodes a missing field to `None` for an optional type — so the attribute was redundant and the comment was wrong about its own mechanism. Both removed rather than left to mislead the next reader. The delivery half needed no change: `write_agent_matrix_token` already took a homeserver and already wrote the `matrix-account-.json` sidecar beside the token. `deliver` simply stops passing `None`. A credential stored without one still works exactly as before — no sidecar, and the account needs a configured entry. Refs #3726 --- hive-c0re/src/workers/credential.rs | 13 ++++-- swarm-secret-client/src/client.rs | 66 +++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/hive-c0re/src/workers/credential.rs b/hive-c0re/src/workers/credential.rs index 81f5e998..2d62482f 100644 --- a/hive-c0re/src/workers/credential.rs +++ b/hive-c0re/src/workers/credential.rs @@ -42,7 +42,7 @@ pub async fn deliver(notice: &CredentialNotice, cert_role: &str) -> Result<()> { let store = SecretStore::from_env(cert_role) .await .context("connecting to the swarm secret store")?; - let value = store + let credential = store .read(&secret_path) .await .with_context(|| format!("reading {secret_path} from the store"))?; @@ -52,11 +52,18 @@ pub async fn deliver(notice: &CredentialNotice, cert_role: &str) -> Result<()> { // arrives owned by `hive-core` at 0600 — the daemon wakes on it appearing // and cannot read it. hive-priv also builds the filename, so the name the // watcher globs for is decided in one place now. + // + // A homeserver is passed through when the stored credential carries one; + // hive-priv then writes the `matrix-account-.json` sidecar beside the + // token, which is how the daemon discovers an extra account's homeserver + // without a static `matrixAccounts` entry. Credentials written before that + // field existed carry `None`, and the sidecar is simply not written — the + // account then needs a configured entry, exactly as it did before. crate::priv_client::write_agent_matrix_token( agent.as_str(), - &value, + &credential.value, Some(¬ice.account), - None, + credential.homeserver.as_deref(), ) .await } diff --git a/swarm-secret-client/src/client.rs b/swarm-secret-client/src/client.rs index d2f00a14..8af06a4d 100644 --- a/swarm-secret-client/src/client.rs +++ b/swarm-secret-client/src/client.rs @@ -19,12 +19,31 @@ pub const ENV_CACERT: &str = "BAO_CACERT"; /// The auth mount a cert login goes through, unless a caller says otherwise. pub const DEFAULT_CERT_MOUNT: &str = "cert"; -#[derive(Serialize, Deserialize)] -struct Credential { +/// What a credential path holds: the secret itself, plus the configuration a +/// reader needs to use it. +/// +/// The homeserver rides here rather than on the queue notice because a notice +/// is not persistence — re-delivering a credential (agent moved, hive +/// re-provisioned, token rotated) has to reconstruct it from somewhere, and +/// the store is the only thing that keeps it. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Credential { /// `glue-matrix-bao-token.nix` reads the store with /// `bao kv get -field=value`, so this name is load-bearing for a reader /// this crate does not control. Renaming it silently breaks that unit. - value: String, + pub value: String, + + /// Absent on every object written before this field existed, and KV2 keeps + /// those versions forever. **`Option` is what tolerates that** — serde + /// decodes a missing field to `None` for an optional type, so the type is + /// the compatibility guarantee and changing it to a bare `String` is what + /// would break every stored credential at once. + /// + /// `skip_serializing_if` is doing separate work: without it a token-only + /// credential serialises `"homeserver":null`, and this object is read by + /// `glue-matrix-bao-token.nix` with `bao kv get -field=value`. + #[serde(skip_serializing_if = "Option::is_none")] + pub homeserver: Option, } /// Where the store is and which identity we present to it. @@ -143,20 +162,16 @@ impl SecretStore { /// # Errors /// [`Error::Vault`] when the path does not exist, the token's policy does /// not cover it, or the stored object has no `value` field. - pub async fn read(&self, path: &str) -> Result { - let c: Credential = vaultrs::kv2::read(&self.inner, MOUNT, path).await?; - Ok(c.value) + pub async fn read(&self, path: &str) -> Result { + Ok(vaultrs::kv2::read(&self.inner, MOUNT, path).await?) } - /// Write `value` as the credential at `path`, creating a new version. + /// Write `credential` at `path`, creating a new version. /// /// # Errors /// [`Error::Vault`] when the token's policy does not cover the path. - pub async fn write(&self, path: &str, value: &str) -> Result<(), Error> { - let body = Credential { - value: value.to_owned(), - }; - vaultrs::kv2::set(&self.inner, MOUNT, path, &body).await?; + pub async fn write(&self, path: &str, credential: &Credential) -> Result<(), Error> { + vaultrs::kv2::set(&self.inner, MOUNT, path, credential).await?; Ok(()) } } @@ -234,13 +249,40 @@ mod tests { } } + /// KV2 keeps every prior version, so objects written before `homeserver` + /// existed are still readable and still get decoded by this type. What + /// tolerates the absence is the field being `Option`, not any attribute: + /// making it a bare `String` would not surface as a migration, it would + /// surface as every previously-stored credential becoming unreadable at + /// once. Mutation-tested — dropping `#[serde(default)]` changed nothing, + /// which is how the redundant attribute was found and removed. + #[test] + fn a_credential_stored_before_the_homeserver_field_still_decodes() { + let old: Credential = serde_json::from_str(r#"{"value":"t"}"#) + .expect("an object written by the previous version must still decode"); + assert_eq!(old.value, "t"); + assert_eq!(old.homeserver, None); + + // Control: the field is genuinely read when present, so the arm above + // is about absence being tolerated rather than the field being ignored. + let new: Credential = serde_json::from_str(r#"{"value":"t","homeserver":"https://hs"}"#) + .expect("an object with the field decodes too"); + assert_eq!(new.homeserver.as_deref(), Some("https://hs")); + } + #[test] fn the_value_field_matches_what_the_nix_reader_asks_for() { // The literal is the point: `bao kv get -field=value` is the other end // of this agreement and lives in a file no Rust test can reach, so the // name is pinned here rather than derived from the struct. + // + // It doubles as the compatibility control for `homeserver`: a + // token-only credential must still serialise to exactly these bytes, + // with no `homeserver` key at all, so adding the field cannot change + // what that unit reads. let json = serde_json::to_string(&Credential { value: "t".to_owned(), + homeserver: None, }) .expect("a struct of one String serialises"); assert_eq!(json, r#"{"value":"t"}"#);