From e263681f1dfc94ded52e49e2daa3e4bc5ebde4ba Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 8 Sep 2026 12:58:45 +0200 Subject: [PATCH] swarm-secret-client: one module per kind of secret, not one struct MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mara, reviewing the previous commit: "the field is specific to matrix, why add it to the general struct". She is right, and the answer is that there was no general struct — `Credential` had one consumer, the crate's only path builder was `matrix_account`, and `value` is pinned by `glue-matrix-bao-token.nix`, a matrix unit. It was matrix's throughout, wearing a general name; adding `homeserver` is what made that visible. `client` now moves whatever type a caller names and decodes nothing itself. That is forwarding rather than machinery: `vaultrs::kv2::read`/`set` are already generic over the payload. The matrix agreement moves to its own module holding both halves — where a credential lives (`account_path`, was `path::matrix_account`) and what the object at that path holds. `path` keeps only what every path obeys, so a second kind of swarm secret becomes a module beside `matrix` rather than another optional field on a struct it shares. argus raised the same collision from the other direction on #4092: two mutually-exclusive `Option`s modelling one concept is the failure mode this forecloses. `checked_segment` stays public in `path`: hive-priv builds an on-disk path from the same names and must accept the same charset. Behaviour is unchanged. The compatibility properties move with the struct — `Option` is what lets a pre-`homeserver` stored object decode, and `skip_serializing_if` is what keeps a token-only object free of `"homeserver":null` for that nix reader. Refs #3726 --- hive-c0re/src/workers/credential.rs | 22 ++--- swarm-secret-client/README.md | 13 ++- swarm-secret-client/src/client.rs | 84 +++-------------- swarm-secret-client/src/lib.rs | 14 ++- swarm-secret-client/src/matrix.rs | 134 ++++++++++++++++++++++++++++ swarm-secret-client/src/path.rs | 61 ++++++------- 6 files changed, 205 insertions(+), 123 deletions(-) create mode 100644 swarm-secret-client/src/matrix.rs diff --git a/hive-c0re/src/workers/credential.rs b/hive-c0re/src/workers/credential.rs index 2d62482f..72ee4745 100644 --- a/hive-c0re/src/workers/credential.rs +++ b/hive-c0re/src/workers/credential.rs @@ -21,7 +21,7 @@ use anyhow::{Context, Result}; use hive_types::Ident; use swarm_queue_client::CredentialNotice; -use swarm_secret_client::{SecretStore, path}; +use swarm_secret_client::{SecretStore, matrix}; /// Read the credential `notice` names and write it into the agent's state dir. /// @@ -36,13 +36,13 @@ pub async fn deliver(notice: &CredentialNotice, cert_role: &str) -> Result<()> { // not a round trip to the store. let agent = Ident::parse(¬ice.agent) .map_err(|e| anyhow::anyhow!("agent name {:?} off the queue: {e}", notice.agent))?; - let secret_path = path::matrix_account(¬ice.agent, ¬ice.account) + let secret_path = matrix::account_path(¬ice.agent, ¬ice.account) .context("building the credential's path in the store")?; let store = SecretStore::from_env(cert_role) .await .context("connecting to the swarm secret store")?; - let credential = store + let credential: matrix::Credential = store .read(&secret_path) .await .with_context(|| format!("reading {secret_path} from the store"))?; @@ -74,16 +74,16 @@ mod tests { #[test] fn a_name_that_could_address_another_agent_is_refused() { - // `path::matrix_account` owns this rule; asserted here because this is + // `matrix::account_path` owns this rule; asserted here because this is // the module that feeds it names off the wire. - assert!(path::matrix_account("../argus", "ccc").is_err()); - assert!(path::matrix_account("dmatrix", "../../etc/x").is_err()); - assert!(path::matrix_account("dmatrix", "ccc").is_ok()); + assert!(matrix::account_path("../argus", "ccc").is_err()); + assert!(matrix::account_path("dmatrix", "../../etc/x").is_err()); + assert!(matrix::account_path("dmatrix", "ccc").is_ok()); } #[test] fn an_agent_name_off_the_queue_must_pass_the_ident_parser_too() { - // Two independent refusals, not one restated: `path::matrix_account` + // Two independent refusals, not one restated: `matrix::account_path` // guards the address in the *store*, and `Ident` guards the name this // module hands to hive-priv, which builds the on-disk path from it. for bad in ["../argus", "dmatrix/../argus", "Dmatrix", "d matrix", ""] { @@ -102,7 +102,7 @@ mod tests { fn an_account_name_off_the_queue_is_refused_for_the_store_path() { for bad in ["../argus", "a/b", "a b", ""] { assert!( - path::matrix_account("dmatrix", bad).is_err(), + matrix::account_path("dmatrix", bad).is_err(), "account {bad:?} must be refused" ); } @@ -110,7 +110,7 @@ mod tests { // passing because everything is refused. Uppercase and underscore are // deliberate — `matrixAccounts` is an attrset, so both are names an // operator can already write, and hive-priv must accept them too. - assert!(path::matrix_account("dmatrix", "ops-relay").is_ok()); - assert!(path::matrix_account("dmatrix", "Ops_Relay9").is_ok()); + assert!(matrix::account_path("dmatrix", "ops-relay").is_ok()); + assert!(matrix::account_path("dmatrix", "Ops_Relay9").is_ok()); } } diff --git a/swarm-secret-client/README.md b/swarm-secret-client/README.md index 273865aa..6c4126e6 100644 --- a/swarm-secret-client/README.md +++ b/swarm-secret-client/README.md @@ -61,12 +61,23 @@ read an error about, and none of them needs a reachable store to happen. ## Names that arrive from elsewhere -`path::matrix_account` is fallible, which for a string formatter needs saying: +`matrix::account_path` is fallible, which for a string formatter needs saying: its segments are an agent name from the topology and an account name from that agent's own config. A `/` turns one agent's segment into another agent's directory and `..` walks out of the prefix entirely, so the charset it accepts is deliberately narrower than what the store would. +## One module per kind of secret + +`client` moves whatever type a caller names; it decodes nothing itself. What a +stored object _holds_ is stated in the module that also builds its path — +`matrix` today, and a second kind of swarm secret gets a module beside it. + +The split is deliberate. A single shared struct that grows one field per +consumer ends up carrying, on every path, a field only one path's reader has +ever heard of; and the two things a kind of secret must pin — where it lives +and what is in it — are one agreement that reads worse split across modules. + ## What this crate does not do It has no opinion on **what** a caller may read. That is the policy attached to diff --git a/swarm-secret-client/src/client.rs b/swarm-secret-client/src/client.rs index 8af06a4d..74e61af8 100644 --- a/swarm-secret-client/src/client.rs +++ b/swarm-secret-client/src/client.rs @@ -1,6 +1,6 @@ //! A logged-in handle on the store, built from this deployment's environment. -use serde::{Deserialize, Serialize}; +use serde::{Serialize, de::DeserializeOwned}; use vaultrs::client::{Client, VaultClient, VaultClientSettingsBuilder}; use crate::{Error, path::MOUNT}; @@ -19,33 +19,6 @@ 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"; -/// 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. - 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. /// /// Separate from the connect so the environment can be checked without one: @@ -157,21 +130,25 @@ impl SecretStore { Ok(Self { inner }) } - /// Read the credential stored at `path`. + /// Read the object stored at `path`, decoded as `T`. + /// + /// The shape belongs to whoever owns the path — see [`crate::matrix`] for + /// the one kind of secret that exists — so this moves bytes and knows no + /// fields. /// /// # 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 { + /// not cover it, or the stored object does not decode as `T`. + pub async fn read(&self, path: &str) -> Result { Ok(vaultrs::kv2::read(&self.inner, MOUNT, path).await?) } - /// Write `credential` at `path`, creating a new version. + /// Write `value` 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, credential: &Credential) -> Result<(), Error> { - vaultrs::kv2::set(&self.inner, MOUNT, path, credential).await?; + pub async fn write(&self, path: &str, value: &T) -> Result<(), Error> { + vaultrs::kv2::set(&self.inner, MOUNT, path, value).await?; Ok(()) } } @@ -248,43 +225,4 @@ mod tests { other => panic!("wanted an Identity error, got {other:?}"), } } - - /// 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"}"#); - } } diff --git a/swarm-secret-client/src/lib.rs b/swarm-secret-client/src/lib.rs index 02fa1efd..a146e0cd 100644 --- a/swarm-secret-client/src/lib.rs +++ b/swarm-secret-client/src/lib.rs @@ -2,12 +2,18 @@ //! reach it. //! //! The HTTP is [`vaultrs`]'s job. What this crate owns is the *agreements* — -//! the path a credential is written to and read from ([`path`]), the field its -//! bytes live in, and the translation from this deployment's environment into -//! a logged-in client ([`client`]). Each of those is a thing the controller and -//! a hive must say identically, so it is said once here. +//! the rules every path obeys ([`path`]), the translation from this +//! deployment's environment into a logged-in client ([`client`]), and, per kind +//! of secret, the path it lives at together with the fields it holds +//! ([`matrix`]). Each of those is a thing the controller and a hive must say +//! identically, so it is said once here. +//! +//! [`client`] is deliberately ignorant of all of it: it moves whatever type a +//! caller names, so a second kind of secret is a new module beside [`matrix`] +//! and not another field on a struct shared with it. pub mod client; +pub mod matrix; pub mod path; pub use client::SecretStore; diff --git a/swarm-secret-client/src/matrix.rs b/swarm-secret-client/src/matrix.rs new file mode 100644 index 00000000..19469b29 --- /dev/null +++ b/swarm-secret-client/src/matrix.rs @@ -0,0 +1,134 @@ +//! The matrix agreement: where an account's credential lives in the store, and +//! what the object at that path holds. +//! +//! Both halves are one agreement and neither end of it is senior, so they are +//! stated together here rather than split between the path module and the +//! client. Nothing in [`crate::client`] knows this shape — it moves whatever +//! type a caller names — so a second kind of swarm secret gets its own module +//! beside this one instead of another field on a shared struct. + +use serde::{Deserialize, Serialize}; + +use crate::{ + Error, + path::{AGENT_PREFIX, checked_segment}, +}; + +/// The path holding `agent`'s token for the external matrix account `account`. +/// +/// # Errors +/// [`Error::PathSegment`] when either name contains anything but +/// `[A-Za-z0-9_-]`, which is what keeps one agent's name from addressing +/// another agent's secret. +pub fn account_path(agent: &str, account: &str) -> Result { + checked_segment("agent", agent)?; + checked_segment("account", account)?; + Ok(format!("{AGENT_PREFIX}/{agent}/matrix/{account}")) +} + +/// What an account's path holds: the token, plus the homeserver it belongs to. +/// +/// The homeserver rides with the token rather than on the queue notice that +/// triggers a delivery, because a notice is not persistence — re-delivering a +/// credential has to reconstruct it, 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. + 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, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_well_formed_pair_lands_under_the_agent_prefix() { + let p = account_path("atlas", "ops-relay").expect("both segments are legal"); + assert_eq!(p, "swarm/agents/atlas/matrix/ops-relay"); + assert!(p.starts_with(AGENT_PREFIX)); + } + + #[test] + fn a_segment_cannot_escape_its_own_directory() { + // Each of these is a *different* way to address another agent's tree, + // and the last two are the ones a charset check catches but a + // `contains("..")` check does not. + for bad in [ + "../argus", + "atlas/../argus", + "atlas/matrix", + "a b", + "a.b", + "", + ] { + assert!( + account_path(bad, "ops-relay").is_err(), + "agent segment {bad:?} must be refused" + ); + assert!( + account_path("atlas", bad).is_err(), + "account segment {bad:?} must be refused" + ); + } + } + + #[test] + fn the_legal_charset_is_actually_reachable() { + // The control for the test above: if `checked_segment` rejected + // everything, the escape cases would pass for the wrong reason. + assert!(account_path("a-b_C9", "d-e_F0").is_ok()); + } + + /// 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. + #[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"}"#); + } +} diff --git a/swarm-secret-client/src/path.rs b/swarm-secret-client/src/path.rs index 8176488e..345ca0ed 100644 --- a/swarm-secret-client/src/path.rs +++ b/swarm-secret-client/src/path.rs @@ -1,8 +1,10 @@ -//! Where a credential lives, for both ends of the store. +//! The rules every path into the store obeys, whatever kind of secret it +//! addresses. //! //! The controller writes and a hive reads, and neither is senior to the other, -//! so the path they must agree on is built here rather than formatted at each -//! call site. +//! so what they must agree on is stated once here rather than at each call +//! site. A *particular* kind of secret builds its path from these pieces in its +//! own module — [`crate::matrix`] is the one that exists. use crate::Error; @@ -52,34 +54,15 @@ pub fn checked_segment(kind: &'static str, value: &str) -> Result<(), Error> { Ok(()) } -/// The path holding `agent`'s token for the external matrix account `account`. -/// -/// # Errors -/// [`Error::PathSegment`] when either name contains anything but -/// `[A-Za-z0-9_-]`, which is what keeps one agent's name from addressing -/// another agent's secret. -pub fn matrix_account(agent: &str, account: &str) -> Result { - checked_segment("agent", agent)?; - checked_segment("account", account)?; - Ok(format!("{AGENT_PREFIX}/{agent}/matrix/{account}")) -} - #[cfg(test)] mod tests { use super::*; #[test] - fn a_well_formed_pair_lands_under_the_agent_prefix() { - let p = matrix_account("atlas", "ops-relay").expect("both segments are legal"); - assert_eq!(p, "swarm/agents/atlas/matrix/ops-relay"); - assert!(p.starts_with(AGENT_PREFIX)); - } - - #[test] - fn a_segment_cannot_escape_its_own_directory() { - // Each of these is a *different* way to address another agent's tree, - // and the last two are the ones a charset check catches but a - // `contains("..")` check does not. + fn a_segment_that_could_change_a_paths_shape_is_refused() { + // Each of these is a *different* way to reach outside the segment the + // caller meant, and the last two are the ones a charset check catches + // but a `contains("..")` check does not. for bad in [ "../argus", "atlas/../argus", @@ -89,12 +72,8 @@ mod tests { "", ] { assert!( - matrix_account(bad, "ops-relay").is_err(), - "agent segment {bad:?} must be refused" - ); - assert!( - matrix_account("atlas", bad).is_err(), - "account segment {bad:?} must be refused" + checked_segment("agent", bad).is_err(), + "segment {bad:?} must be refused" ); } } @@ -102,7 +81,21 @@ mod tests { #[test] fn the_legal_charset_is_actually_reachable() { // The control for the test above: if `checked_segment` rejected - // everything, the escape cases would pass for the wrong reason. - assert!(matrix_account("a-b_C9", "d-e_F0").is_ok()); + // everything, the escape cases would pass for the wrong reason. The + // charset is the one `hive-priv` accepts for the same names on disk, + // so uppercase and underscore have to stay legal here. + assert!(checked_segment("agent", "a-b_C9").is_ok()); + assert!(checked_segment("account", "d-e_F0").is_ok()); + } + + #[test] + fn the_rejected_segment_is_named_in_the_error() { + // The caller usually got the name from config and needs to see which + // of the two it was. + let e = checked_segment("account", "a b").expect_err("a space is not legal"); + assert!( + matches!(e, Error::PathSegment { kind, ref value } if kind == "account" && value == "a b"), + "got {e:?}" + ); } }