hive-c0re: deliver a matrix credential through hive-priv, not by writing it

`deliver` read the value out of the secret store and wrote it itself, as the
`hive-core` user, at 0600. The file lands in a directory owned by the agent,
so it arrived owned by `hive-core` — the agent's matrix daemon woke on it
appearing and could not read its own credential. `priv_client::write_agent_
matrix_token` already existed and already had two callers; this was the one
path that did not use it.

hive-priv now owns the filename too, so the name the daemon's path unit globs
for is decided in one place instead of being built identically in two.

That move exposed a disagreement worth fixing rather than routing around.
The secret store accepts `[A-Za-z0-9_-]` for an account name; hive-priv's
`validate_name_chars` accepts lowercase, digits and hyphen only. An account is
an attribute name in `hyperhive.matrixAccounts`, typed `attrsOf` with no
charset constraint, so `Ops_Relay9` is a key an operator can already have
written — and it would have read out of the store and then failed to land.

So hive-priv grows `validate_account_name` rather than widening the existing
one: an agent name is an `Ident` and lowercase by design, an account name is an
attrset key, and one validator serving two name domains is what let them drift.

The test that caught this came from `credential.rs`, which used to build the
path. It moves to hive-priv with both of its controls intact, because the
controls are the point — they assert which names must be ACCEPTED, and a
validator narrower than the store's passes every rejection case. A second
moved test pins the `matrix-token` prefix where the name is now built; the
old one would have kept passing while asserting a function that no longer
decided anything.

Refs #3726

hive-c0re/src/workers/credential.rs

@@ -2,53 +2,27 @@
 //! store into that agent's own state dir.
 //!
 //! The controller publishes a [`CredentialNotice`] naming an agent and an
-//! account; this reads the value out of the store and writes it where the
+//! account; this reads the value out of the store and hands it to `hive-priv`,
-//! agent's matrix daemon already watches for it. Nothing here activates
+//! which writes it where the agent's matrix daemon already watches. The write
-//! anything: `nix/agent-modules/matrix.nix` has a `systemd.paths` unit
+//! goes through the privileged helper because the file lands in a directory
-//! globbing `matrix-token*` inside that agent's own state dir, which
+//! owned by the agent and has to be chowned to it — written from here it
-//! re-fires the daemon when a token appears, so arrival is the whole
+//! arrives owned by `hive-core` and the agent cannot read its own credential.
-//! trigger.
+//! `hive-priv` also builds the filename, so nothing in this module decides it.
+//!
+//! Nothing here activates anything: `nix/agent-modules/matrix.nix` has a
+//! `systemd.paths` unit globbing `matrix-token*` inside that agent's own state
+//! dir, which re-fires the daemon when a token appears, so arrival is the
+//! whole trigger.
 //!
 //! 🔑 The notice carries no secret — see [`swarm_queue_client::credential_subject`]
 //! for why that is a requirement rather than a preference. The value is read
 //! from the store under this hive's own identity.
 
-use std::os::unix::fs::PermissionsExt as _;
-use std::path::PathBuf;
-
 use anyhow::{Context, Result};
 use hive_types::Ident;
 use swarm_queue_client::CredentialNotice;
 use swarm_secret_client::{SecretStore, path};
 
-use crate::paths::agent_state_dir;
-
-/// The basename every matrix token must start with.
-///
-/// `nix/agent-modules/matrix.nix` asserts the same prefix on every configured
-/// `tokenFile`, because its path-watcher globs for it. A name written here
-/// that did not match would land a file the daemon never notices — no error,
-/// just a credential that silently never arrives.
-const TOKEN_PREFIX: &str = "matrix-token";
-
-/// Where `agent`'s credential for `account` is written.
-///
-/// Both names arrive off the queue and both become path components, so both
-/// are refused here rather than trusted: `agent` by its [`Ident`] type, and
-/// `account` by the same charset check the store path uses. They are checked
-/// by different mechanisms because only one of them has a newtype — an account
-/// name is an attribute name in `hyperhive.matrixAccounts`, so it is not an
-/// `Ident` and cannot become one without narrowing what an operator may
-/// configure.
-///
-/// # Errors
-/// [`swarm_secret_client::Error::PathSegment`] when `account` is empty or
-/// holds anything outside `[A-Za-z0-9_-]`.
-pub fn token_path(agent: &Ident, account: &str) -> Result<PathBuf, swarm_secret_client::Error> {
-    path::checked_segment("account", account)?;
-    Ok(agent_state_dir(agent).join(format!("{TOKEN_PREFIX}-{account}")))
-}
-
 /// Read the credential `notice` names and write it into the agent's state dir.
 ///
 /// `cert_role` is the role on the store's `cert` auth mount whose policy scopes
@@ -73,62 +47,24 @@ pub async fn deliver(notice: &CredentialNotice, cert_role: &str) -> Result<()> {
         .await
         .with_context(|| format!("reading {secret_path} from the store"))?;
 
-    write_token(&token_path(&agent, &notice.account)?, &value)
+    // Through hive-priv rather than writing here: the file lands in a directory
-}
+    // owned by the agent, and only root can chown it there. Written directly it
-
+    // arrives owned by `hive-core` at 0600 — the daemon wakes on it appearing
-/// Write `value` to `dest` at `0600`, atomically.
+    // and cannot read it. hive-priv also builds the filename, so the name the
-///
+    // watcher globs for is decided in one place now.
-/// Atomic because the daemon's path-watcher fires on the file *appearing*: a
+    crate::priv_client::write_agent_matrix_token(
-/// token written in place would be visible while still partial, and the daemon
+        agent.as_str(),
-/// would read a truncated credential exactly once, at the moment it is hardest
+        &value,
-/// to reproduce. The temp name is dot-prefixed so it cannot match the
+        Some(&notice.account),
-/// `matrix-token*` glob on its way past.
+        None,
-fn write_token(dest: &PathBuf, value: &str) -> Result<()> {
+    )
-    let dir = dest
+    .await
-        .parent()
-        .context("a token path always has a parent state dir")?;
-    let name = dest
-        .file_name()
-        .context("a token path always has a file name")?;
-    let mut tmp = dir.join(".");
-    tmp.as_mut_os_string().push(name);
-    tmp.as_mut_os_string().push(".partial");
-
-    std::fs::write(&tmp, value).with_context(|| format!("writing {}", tmp.display()))?;
-    // Before the rename, so the file is never briefly readable by others under
-    // its final name.
-    std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o600))
-        .with_context(|| format!("restricting {}", tmp.display()))?;
-    std::fs::rename(&tmp, dest).with_context(|| format!("publishing {}", dest.display()))
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
 
-    #[test]
-    fn the_written_name_matches_the_glob_the_daemon_watches() {
-        // The other end of this agreement is an assertion in
-        // `nix/agent-modules/matrix.nix` and a `systemd.paths` glob — neither
-        // reachable from a Rust test, so the prefix is pinned here.
-        let p = token_path(&Ident::parse("dmatrix").expect("a legal agent name"), "ccc")
-            .expect("a legal account name");
-        let name = p.file_name().unwrap().to_str().unwrap();
-        assert!(name.starts_with("matrix-token"), "got {name}");
-        assert_eq!(name, "matrix-token-ccc");
-    }
-
-    #[test]
-    fn the_partial_file_cannot_match_that_glob() {
-        // A temp name starting with `matrix-token` would be picked up
-        // mid-write; the dot prefix is what stops it.
-        let p = token_path(&Ident::parse("dmatrix").expect("a legal agent name"), "ccc")
-            .expect("a legal account name");
-        let name = p.file_name().unwrap().to_str().unwrap();
-        let tmp = format!(".{name}.partial");
-        assert!(!tmp.starts_with("matrix-token"), "got {tmp}");
-    }
-
     #[test]
     fn a_name_that_could_address_another_agent_is_refused() {
         // `path::matrix_account` owns this rule; asserted here because this is
@@ -141,8 +77,8 @@ mod tests {
     #[test]
     fn an_agent_name_off_the_queue_must_pass_the_ident_parser_too() {
         // Two independent refusals, not one restated: `path::matrix_account`
-        // guards the address in the *store*, `Ident` guards the address on
+        // guards the address in the *store*, and `Ident` guards the name this
-        // *disk*, and `token_path` cannot be called without the second.
+        // module hands to hive-priv, which builds the on-disk path from it.
         for bad in ["../argus", "dmatrix/../argus", "Dmatrix", "d matrix", ""] {
             assert!(Ident::parse(bad).is_err(), "{bad:?} must be refused");
         }
@@ -151,25 +87,23 @@ mod tests {
         assert!(Ident::parse("dmatrix").is_ok());
     }
 
-    /// The account half of that same rule, which the agent half's type does
+    /// The account half of that same rule now lives where the filename is
-    /// not cover: an account name is an attr name in `hyperhive.matrixAccounts`
+    /// built — `hive-priv`'s `validate_account_name`, asserted there with the
-    /// and so has no newtype to lean on. Without this, a second caller reaching
+    /// same arms and the same two controls. It is not restated here because
-    /// `token_path` without going through `path::matrix_account` first would
+    /// this module no longer builds the path.
-    /// build a filename out of an unchecked name.
     #[test]
-    fn an_account_name_off_the_queue_is_refused_for_the_disk_path_too() {
+    fn an_account_name_off_the_queue_is_refused_for_the_store_path() {
-        let agent = Ident::parse("dmatrix").expect("a legal agent name");
+        for bad in ["../argus", "a/b", "a b", ""] {
-        for bad in ["../argus", "a/b", "a b", "a.b", ""] {
             assert!(
-                token_path(&agent, bad).is_err(),
+                path::matrix_account("dmatrix", bad).is_err(),
                 "account {bad:?} must be refused"
             );
         }
-        // Controls: the legal charset stays reachable, so the loop above is
+        // Controls: the legal charset stays reachable, so the loop above is not
-        // not passing because everything is refused. Uppercase and underscore
+        // passing because everything is refused. Uppercase and underscore are
-        // are deliberate — `matrixAccounts` is an attrset, so both are names
+        // deliberate — `matrixAccounts` is an attrset, so both are names an
-        // an operator can already write.
+        // operator can already write, and hive-priv must accept them too.
-        assert!(token_path(&agent, "ops-relay").is_ok());
+        assert!(path::matrix_account("dmatrix", "ops-relay").is_ok());
-        assert!(token_path(&agent, "Ops_Relay9").is_ok());
+        assert!(path::matrix_account("dmatrix", "Ops_Relay9").is_ok());
     }
 }

hive-priv/src/main.rs

@@ -546,13 +546,10 @@ fn write_matrix_token(
     homeserver: Option<&str>,
 ) -> Result<(String, String)> {
     validate_agent_name(agent_name)?;
-    let filename = match account {
+    if let Some(a) = account {
-        None => "matrix-token".to_owned(),
+        validate_account_name(a)?;
-        Some(a) => {
+    }
-            validate_name_chars(a)?;
+    let filename = matrix_token_filename(account);
-            format!("matrix-token-{a}")
-        }
-    };
     let res = write_agent_state_file(agent_name, &filename, &format!("{token}\n"))?;
     if let (Some(a), Some(hs)) = (account, homeserver) {
         let meta = serde_json::to_string(&MatrixAccountSidecar { homeserver: hs })
@@ -1364,6 +1361,20 @@ fn ensure_plain_filename(who: &str, filename: &str) -> Result<()> {
     Ok(())
 }
 
+/// Basename an agent's matrix token is written under — `matrix-token` for the
+/// hive's own account, `matrix-token-<account>` for an extra one.
+///
+/// Half of an agreement whose other half is a `systemd.paths` glob in
+/// `nix/agent-modules/matrix.nix`, which no test here can reach: a name that
+/// stopped matching `matrix-token*` would land a credential the agent's daemon
+/// never wakes for — no error, just a token that silently never arrives.
+fn matrix_token_filename(account: Option<&str>) -> String {
+    match account {
+        None => "matrix-token".to_owned(),
+        Some(a) => format!("matrix-token-{a}"),
+    }
+}
+
 /// Name the content is written under before being renamed onto `filename`.
 /// The leading dot is load-bearing rather than tidy: `nix/agent-modules/
 /// matrix.nix` starts the agent's matrix daemon on the glob `matrix-token*`,
@@ -2797,6 +2808,29 @@ fn validate_name_chars(name: &str) -> Result<()> {
     Ok(())
 }
 
+/// Validate a matrix account name, which is a wider charset than
+/// [`validate_name_chars`] allows on purpose.
+///
+/// An agent name is an `Ident` and lowercase by design. An account name is an
+/// attribute name in `hyperhive.matrixAccounts`, typed `attrsOf` with no
+/// charset constraint, so `Ops_Relay9` is a key an operator may already have
+/// written. `swarm_secret_client::path::checked_segment` accepts exactly this
+/// set for the same name in the secret store — the two must agree, or a
+/// credential reads out of the store and then fails to land on disk.
+///
+/// Still a single plain component: no `/`, no `.`, no whitespace, so it cannot
+/// climb out of the agent's state dir or name the dir itself.
+fn validate_account_name(name: &str) -> Result<()> {
+    if name.is_empty()
+        || !name
+            .bytes()
+            .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_')
+    {
+        bail!("invalid account name {name:?}: must be non-empty [A-Za-z0-9_-]");
+    }
+    Ok(())
+}
+
 /// Validate a bind-mount path: must be absolute, non-empty, and contain
 /// no newlines, null bytes, double-quotes, or colons.
 ///
@@ -3061,8 +3095,9 @@ mod tests {
     use super::{
         BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement,
         clear_runner_credentials, contains_secret_shaped_run, git_overlay_flags,
-        limits_dropin_body, partial_name, redact_secret_line, remove_marker_in, single_output_path,
+        limits_dropin_body, matrix_token_filename, partial_name, redact_secret_line,
-        toplevel_attr, write_agent_dir_file, write_state_file_nofollow,
+        remove_marker_in, single_output_path, toplevel_attr, validate_account_name,
+        write_agent_dir_file, write_state_file_nofollow,
     };
     use std::path::PathBuf;
     use std::sync::atomic::{AtomicU32, Ordering};
@@ -3403,6 +3438,39 @@ mod tests {
         std::fs::remove_dir_all(&dir).ok();
     }
 
+    /// Ported from `hive-c0re`'s `credential.rs`, which used to build this path
+    /// itself. The controls are the load-bearing half: an account name is an
+    /// attrset key in `hyperhive.matrixAccounts`, so uppercase and underscore
+    /// are names an operator can already have written, and the secret store
+    /// accepts exactly this set for the same name. A validator narrower than
+    /// the store's reads a credential out and then refuses to land it.
+    #[test]
+    fn an_account_name_is_checked_against_the_same_charset_the_store_uses() {
+        for bad in ["../argus", "a/b", "a b", "a.b", ""] {
+            assert!(
+                validate_account_name(bad).is_err(),
+                "account {bad:?} must be refused"
+            );
+        }
+        assert!(validate_account_name("ops-relay").is_ok());
+        assert!(validate_account_name("Ops_Relay9").is_ok());
+    }
+
+    /// Pinned here because here is where the name is decided. `hive-c0re` used
+    /// to assert this against a path helper of its own, which stopped deciding
+    /// anything the moment credential delivery started routing through this
+    /// process — a test that would have kept passing while the real filename
+    /// drifted.
+    #[test]
+    fn a_matrix_token_is_named_for_the_glob_the_daemon_watches() {
+        assert_eq!(matrix_token_filename(None), "matrix-token");
+        assert_eq!(matrix_token_filename(Some("ccc")), "matrix-token-ccc");
+        // And the temp the publish goes through must not match that same glob,
+        // which is only checkable now that both names are built in one place.
+        let tmp = partial_name(&matrix_token_filename(Some("ccc")));
+        assert!(!tmp.starts_with("matrix-token"), "got {tmp}");
+    }
+
     /// The temp name is the whole reason the rename is safe to watch: a
     /// `systemd.path` unit globbing `matrix-token*` would fire on a temp that
     /// merely suffixed the real name, on exactly the empty file the rename