swarm-secret-client: one module per kind of secret, not one struct

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

@@ -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(&notice.agent)
         .map_err(|e| anyhow::anyhow!("agent name {:?} off the queue: {e}", notice.agent))?;
-    let secret_path = path::matrix_account(&notice.agent, &notice.account)
+    let secret_path = matrix::account_path(&notice.agent, &notice.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!(matrix::account_path("../argus", "ccc").is_err());
-        assert!(path::matrix_account("dmatrix", "../../etc/x").is_err());
+        assert!(matrix::account_path("dmatrix", "../../etc/x").is_err());
-        assert!(path::matrix_account("dmatrix", "ccc").is_ok());
+        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!(matrix::account_path("dmatrix", "ops-relay").is_ok());
-        assert!(path::matrix_account("dmatrix", "Ops_Relay9").is_ok());
+        assert!(matrix::account_path("dmatrix", "Ops_Relay9").is_ok());
     }
 }

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

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<String>,
-}
-
 /// 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.
+    /// not cover it, or the stored object does not decode as `T`.
-    pub async fn read(&self, path: &str) -> Result<Credential, Error> {
+    pub async fn read<T: DeserializeOwned>(&self, path: &str) -> Result<T, Error> {
         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> {
+    pub async fn write<T: Serialize + Sync>(&self, path: &str, value: &T) -> Result<(), Error> {
-        vaultrs::kv2::set(&self.inner, MOUNT, path, credential).await?;
+        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"}"#);
-    }
 }

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
+//! the rules every path obeys ([`path`]), the translation from this
-//! bytes live in, and the translation from this deployment's environment into
+//! deployment's environment into a logged-in client ([`client`]), and, per kind
-//! a logged-in client ([`client`]). Each of those is a thing the controller and
+//! of secret, the path it lives at together with the fields it holds
-//! a hive must say identically, so it is said once here.
+//! ([`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;

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<String, Error> {
+    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<String>,
+}
+
+#[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"}"#);
+    }
+}

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
+//! so what they must agree on is stated once here rather than at each call
-//! call site.
+//! 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<String, Error> {
-    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() {
+    fn a_segment_that_could_change_a_paths_shape_is_refused() {
-        let p = matrix_account("atlas", "ops-relay").expect("both segments are legal");
+        // Each of these is a *different* way to reach outside the segment the
-        assert_eq!(p, "swarm/agents/atlas/matrix/ops-relay");
+        // caller meant, and the last two are the ones a charset check catches
-        assert!(p.starts_with(AGENT_PREFIX));
+        // but a `contains("..")` check does not.
-    }
-
-    #[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",
@@ -89,12 +72,8 @@ mod tests {
             "",
         ] {
             assert!(
-                matrix_account(bad, "ops-relay").is_err(),
+                checked_segment("agent", bad).is_err(),
-                "agent segment {bad:?} must be refused"
+                "segment {bad:?} must be refused"
-            );
-            assert!(
-                matrix_account("atlas", bad).is_err(),
-                "account 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.
+        // everything, the escape cases would pass for the wrong reason. The
-        assert!(matrix_account("a-b_C9", "d-e_F0").is_ok());
+        // 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:?}"
+        );
     }
 }