From 0dd807062cc3becf5208224cd81cf3260d4d2a99 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 7 Sep 2026 23:58:11 +0200 Subject: [PATCH] hive-c0re: deliver a matrix credential through hive-priv, not by writing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 | 142 ++++++++-------------------- hive-priv/src/main.rs | 86 +++++++++++++++-- 2 files changed, 115 insertions(+), 113 deletions(-) diff --git a/hive-c0re/src/workers/credential.rs b/hive-c0re/src/workers/credential.rs index 42e4c643..81f5e998 100644 --- a/hive-c0re/src/workers/credential.rs +++ b/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 -//! agent's matrix daemon already watches for 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. +//! account; this reads the value out of the store and hands it to `hive-priv`, +//! which writes it where the agent's matrix daemon already watches. The write +//! goes through the privileged helper because the file lands in a directory +//! owned by the agent and has to be chowned to it — written from here it +//! arrives owned by `hive-core` and the agent cannot read its own credential. +//! `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 { - 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, ¬ice.account)?, &value) -} - -/// Write `value` to `dest` at `0600`, atomically. -/// -/// Atomic because the daemon's path-watcher fires on the file *appearing*: a -/// token written in place would be visible while still partial, and the daemon -/// would read a truncated credential exactly once, at the moment it is hardest -/// to reproduce. The temp name is dot-prefixed so it cannot match the -/// `matrix-token*` glob on its way past. -fn write_token(dest: &PathBuf, value: &str) -> Result<()> { - let dir = dest - .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())) + // 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 + // and cannot read it. hive-priv also builds the filename, so the name the + // watcher globs for is decided in one place now. + crate::priv_client::write_agent_matrix_token( + agent.as_str(), + &value, + Some(¬ice.account), + None, + ) + .await } #[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 - // *disk*, and `token_path` cannot be called without the second. + // 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", ""] { 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 - /// not cover: an account name is an attr name in `hyperhive.matrixAccounts` - /// and so has no newtype to lean on. Without this, a second caller reaching - /// `token_path` without going through `path::matrix_account` first would - /// build a filename out of an unchecked name. + /// The account half of that same rule now lives where the filename is + /// built — `hive-priv`'s `validate_account_name`, asserted there with the + /// same arms and the same two controls. It is not restated here because + /// this module no longer builds the path. #[test] - fn an_account_name_off_the_queue_is_refused_for_the_disk_path_too() { - let agent = Ident::parse("dmatrix").expect("a legal agent name"); - for bad in ["../argus", "a/b", "a b", "a.b", ""] { + fn an_account_name_off_the_queue_is_refused_for_the_store_path() { + for bad in ["../argus", "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 - // not passing because everything is refused. Uppercase and underscore - // are deliberate — `matrixAccounts` is an attrset, so both are names - // an operator can already write. - assert!(token_path(&agent, "ops-relay").is_ok()); - assert!(token_path(&agent, "Ops_Relay9").is_ok()); + // Controls: the legal charset stays reachable, so the loop above is not + // 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()); } } diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 2cb567f3..6060cffa 100644 --- a/hive-priv/src/main.rs +++ b/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 { - None => "matrix-token".to_owned(), - Some(a) => { - validate_name_chars(a)?; - format!("matrix-token-{a}") - } - }; + if let Some(a) = account { + validate_account_name(a)?; + } + let filename = matrix_token_filename(account); 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-` 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, - toplevel_attr, write_agent_dir_file, write_state_file_nofollow, + limits_dropin_body, matrix_token_filename, partial_name, redact_secret_line, + 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