diff --git a/hive-c0re/src/workers/credential.rs b/hive-c0re/src/workers/credential.rs index a6f11933..2b8856dc 100644 --- a/hive-c0re/src/workers/credential.rs +++ b/hive-c0re/src/workers/credential.rs @@ -32,13 +32,20 @@ const TOKEN_PREFIX: &str = "matrix-token"; /// Where `agent`'s credential for `account` is written. /// -/// Takes an [`Ident`] rather than a `&str` because the name arrives off the -/// queue: `agent_state_dir` addresses a directory, and an unvalidated name -/// there is a path-traversal argument. The compiler refusing the `&str` is the -/// check — nothing here has to remember to perform one. -#[must_use] -pub fn token_path(agent: &Ident, account: &str) -> PathBuf { - agent_state_dir(agent).join(format!("{TOKEN_PREFIX}-{account}")) +/// 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. @@ -65,7 +72,7 @@ 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_token(&token_path(&agent, ¬ice.account)?, &value) } /// Write `value` to `dest` at `0600`, atomically. @@ -103,7 +110,8 @@ mod tests { // 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"); + 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"); @@ -113,7 +121,8 @@ mod tests { 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"); + 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}"); @@ -132,8 +141,7 @@ mod tests { 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*. `token_path` cannot even be called without the second, - // which is why it takes an `Ident` rather than validating internally. + // *disk*, and `token_path` cannot be called without the second. for bad in ["../argus", "dmatrix/../argus", "Dmatrix", "d matrix", ""] { assert!(Ident::parse(bad).is_err(), "{bad:?} must be refused"); } @@ -141,4 +149,26 @@ mod tests { // satisfy the loop above. 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. + #[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", ""] { + assert!( + token_path(&agent, 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()); + } } diff --git a/swarm-secret-client/src/path.rs b/swarm-secret-client/src/path.rs index 00116422..8176488e 100644 --- a/swarm-secret-client/src/path.rs +++ b/swarm-secret-client/src/path.rs @@ -23,7 +23,17 @@ pub const AGENT_PREFIX: &str = "swarm/agents"; /// out of the prefix entirely. Both are names this crate receives from /// elsewhere — an agent name from the topology, an account name from an /// agent's own config — so neither is trusted to be well-formed here. -fn checked_segment(kind: &'static str, value: &str) -> Result<(), Error> { +/// +/// Public because the same name is also used to build a path **on disk**, and +/// that guard must accept exactly what this one does. Two copies of a charset +/// are two charsets: they agree until one is edited, and the day they diverge +/// is the day a name is legal in the store and not on the filesystem, or the +/// reverse. +/// +/// # Errors +/// [`Error::PathSegment`] when `value` is empty or holds anything outside +/// `[A-Za-z0-9_-]`. +pub fn checked_segment(kind: &'static str, value: &str) -> Result<(), Error> { if value.is_empty() { return Err(Error::PathSegment { kind,