From f49eef299b44e946c0026acfa65062cf7bceed4f Mon Sep 17 00:00:00 2001
From: atlas <atlas@pr1ma.darkest.space>
Date: Mon, 7 Sep 2026 22:53:27 +0200
Subject: [PATCH] hive-priv: publish an agent's credential by rename, not in
 place
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`write_agent_dir_file` opened the final path with O_TRUNC and filled it, so
the file existed empty before it held anything. Several of these paths are
watched, and the kinds differ: `nix/agent-modules/matrix.nix` starts the
agent's matrix daemon on `PathExistsGlob = ".../matrix-token*"`, which fires
on the file *existing* — the O_CREAT moment, ahead of the content.
`nix/agent-modules/forge.nix` uses `PathChanged` and has no such window.

Write to a temp in the same directory, chown that, then rename it into
place. The chown stays ahead of the publish for the same reason the write
now does: the file must never be visible under its final name while still
root-owned.

The temp is dot-prefixed rather than suffixed, because `matrix-token-x.partial`
matches the daemon's own glob — a suffix would wake it on exactly the empty
file the rename exists to hide.

`write_state_file_nofollow` is deliberately unchanged. Its O_NOFOLLOW and
fchmod/fchown-on-the-fd properties are what make a root write into an
agent-owned directory safe, and its existing tests are the control on them;
the caller is the part that needed to change.

That does move the leaf validation, though: the helper now only ever sees the
temp name, which is a plain component whatever the caller passed. So
`ensure_plain_filename` is extracted — it was already duplicated in
`delete_agent_state_file` — and the caller's own name is checked with it.

Whether the empty-file window is reachable in practice is not measured;
`matrix.nix`'s documented skip condition is "missing" rather than "empty", so
a first provision could plausibly lose one. This makes the question moot
rather than answering it.

Refs #3726
---
 hive-priv/src/main.rs | 134 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 125 insertions(+), 9 deletions(-)

diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs
index 571a7976..2cb567f3 100644
--- a/hive-priv/src/main.rs
+++ b/hive-priv/src/main.rs
@@ -1355,6 +1355,24 @@ async fn control_infra_container(
     ))
 }
 
+/// A leaf safe to join onto an agent-owned directory: one plain component, so
+/// it can neither climb out of the directory nor name the directory itself.
+fn ensure_plain_filename(who: &str, filename: &str) -> Result<()> {
+    if filename.is_empty() || filename == "." || filename == ".." || filename.contains('/') {
+        bail!("{who}: refusing non-plain filename {filename:?}");
+    }
+    Ok(())
+}
+
+/// 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*`,
+/// which a `<name>.partial` suffix would match — waking it on precisely the
+/// empty file the rename exists to hide.
+fn partial_name(filename: &str) -> String {
+    format!(".{filename}.partial")
+}
+
 /// Create/overwrite `dir/filename` at 0600 without following a symlink at the
 /// leaf, returning the open fd for the caller to `fchown`. `filename` must be a
 /// single plain component (no `/`, `.`, `..`) — the leaf sits in an
@@ -1369,9 +1387,7 @@ fn write_state_file_nofollow(dir: &Path, filename: &str, content: &str) -> Resul
     use std::io::Write as _;
     use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _};
 
-    if filename.is_empty() || filename == "." || filename == ".." || filename.contains('/') {
-        bail!("write_state_file_nofollow: refusing non-plain filename {filename:?}");
-    }
+    ensure_plain_filename("write_state_file_nofollow", filename)?;
     let path = dir.join(filename);
     let mut file = std::fs::OpenOptions::new()
         .write(true)
@@ -1469,6 +1485,11 @@ fn remove_marker_in(dir: &Path, filename: &str) -> Result<()> {
 /// writes (which target `state/`) and the pause marker (which targets
 /// `harness/`) — both write into a directory owned by the agent, which is
 /// precisely why they need hive-priv at all.
+///
+/// The file is published by `rename`, so a reader woken by its appearance
+/// cannot catch it empty or half-written: several of these paths have a
+/// `systemd.path` unit watching them, and one of those triggers on the file
+/// existing rather than changing.
 fn write_agent_dir_file(
     agent_name: &str,
     dir: &Path,
@@ -1478,6 +1499,10 @@ fn write_agent_dir_file(
     use std::os::fd::AsRawFd as _;
     use std::os::unix::fs::MetadataExt as _;
 
+    // The leaf is validated here as well as in `write_state_file_nofollow`:
+    // that call now sees the temp name, which is plain whatever `filename` is.
+    ensure_plain_filename("write_agent_dir_file", filename)?;
+
     let state_dir = dir.to_path_buf();
     // NOTE: `create_dir_all` is normally a no-op — lifecycle creates and chowns
     // the state dir during spawn. On the rare edge where the dir doesn't exist
@@ -1493,7 +1518,9 @@ fn write_agent_dir_file(
     // planted at the leaf, so this root-privileged create/write/chmod/chown
     // can't be redirected at an arbitrary file. See `write_state_file_nofollow`.
     let path = state_dir.join(filename);
-    let file = write_state_file_nofollow(&state_dir, filename, content)?;
+    let tmp_name = partial_name(filename);
+    let tmp_path = state_dir.join(&tmp_name);
+    let file = write_state_file_nofollow(&state_dir, &tmp_name, content)?;
 
     // Chown to the state dir's owner so the agent process can read the file.
     // fchown on the same fd — TOCTOU-immune (the inode the write hit, never a
@@ -1523,6 +1550,14 @@ fn write_agent_dir_file(
             );
         }
     }
+    // After the chown, never before: the file must never be visible under its
+    // final name while still root-owned. Leaving the temp behind on failure
+    // would also leave a credential readable by nobody but root, so it goes.
+    if let Err(e) = std::fs::rename(&tmp_path, &path) {
+        let _ = std::fs::remove_file(&tmp_path);
+        return Err(e).with_context(|| format!("publishing {}", path.display()));
+    }
+
     tracing::info!(
         agent = %agent_name,
         dir = %state_dir.display(),
@@ -1539,9 +1574,7 @@ fn write_agent_dir_file(
 /// label into a fixed `forge-<label>-token` shape, same as the write
 /// side.
 fn delete_agent_state_file(agent_name: &str, filename: &str) -> Result<(String, String)> {
-    if filename.is_empty() || filename == "." || filename == ".." || filename.contains('/') {
-        bail!("delete_agent_state_file: refusing non-plain filename {filename:?}");
-    }
+    ensure_plain_filename("delete_agent_state_file", filename)?;
     let path = PathBuf::from(AGENT_STATE_ROOT)
         .join(agent_name)
         .join("state")
@@ -3028,8 +3061,8 @@ 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, redact_secret_line, remove_marker_in, single_output_path,
-        toplevel_attr, write_state_file_nofollow,
+        limits_dropin_body, partial_name, redact_secret_line, remove_marker_in, single_output_path,
+        toplevel_attr, write_agent_dir_file, write_state_file_nofollow,
     };
     use std::path::PathBuf;
     use std::sync::atomic::{AtomicU32, Ordering};
@@ -3351,10 +3384,93 @@ mod tests {
                 write_state_file_nofollow(&dir, bad, "x").is_err(),
                 "must reject filename {bad:?}"
             );
+            // The wrapper needs its own check: the only name it hands to
+            // `write_state_file_nofollow` is the temp, which is plain whatever
+            // the caller passed. Pointing it at a directory that does not exist
+            // yet is what makes this arm bite -- a bad leaf fails eventually
+            // either way, at the rename, so the property worth pinning is that
+            // it fails BEFORE any root-privileged filesystem work.
+            let absent = dir.join("never-created");
+            assert!(
+                write_agent_dir_file("a", &absent, bad, "x").is_err(),
+                "wrapper must reject filename {bad:?}"
+            );
+            assert!(
+                !absent.exists(),
+                "rejected write created a directory for {bad:?}"
+            );
         }
         std::fs::remove_dir_all(&dir).ok();
     }
 
+    /// 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
+    /// exists to hide.
+    #[test]
+    fn a_temp_cannot_match_a_glob_on_the_name_it_publishes() {
+        for real in ["matrix-token", "matrix-token-alice", "forge-token"] {
+            let tmp = partial_name(real);
+            assert!(
+                !tmp.starts_with(real),
+                "{tmp:?} matches a `{real}*` path unit while half-written"
+            );
+            assert!(!tmp.contains('/'), "temp must stay in the same directory");
+        }
+    }
+
+    /// A published file arrives complete and alone. The residue arm is the
+    /// load-bearing one: a stranded temp holds the same secret, is owned by
+    /// root, and nothing else in the system would ever remove it.
+    #[test]
+    fn publishing_leaves_only_the_finished_file() {
+        use std::os::unix::fs::PermissionsExt as _;
+        let dir = scratch();
+        let names = |d: &PathBuf| {
+            let mut v: Vec<String> = std::fs::read_dir(d)
+                .unwrap()
+                .map(|e| e.unwrap().file_name().to_string_lossy().into_owned())
+                .collect();
+            v.sort();
+            v
+        };
+        assert!(names(&dir).is_empty(), "control: scratch starts empty");
+
+        let path = dir.join("matrix-token-alice");
+        for content in ["SECRET", "ROTATED"] {
+            write_agent_dir_file("a", &dir, "matrix-token-alice", content).unwrap();
+            assert_eq!(names(&dir), ["matrix-token-alice"], "temp left behind");
+            assert_eq!(std::fs::read_to_string(&path).unwrap(), content);
+        }
+        let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777;
+        assert_eq!(mode, 0o600, "credential must be 0600 once published");
+
+        std::fs::remove_dir_all(&dir).ok();
+    }
+
+    /// A failed publish must not strand the temp.
+    #[test]
+    fn a_failed_publish_removes_the_temp() {
+        let dir = scratch();
+        // A directory at the destination fails the `rename` (EISDIR) without
+        // needing to drop privileges; the write and chown ahead of it still run.
+        std::fs::create_dir(dir.join("forge-token")).unwrap();
+
+        let err = write_agent_dir_file("a", &dir, "forge-token", "SECRET")
+            .expect_err("publishing onto a directory must fail");
+        assert!(
+            format!("{err:#}").contains("publishing"),
+            "must fail AT the publish -- failing earlier leaves no temp to \
+             clean up and makes the next assertion vacuous. got: {err:#}"
+        );
+        assert!(
+            !dir.join(partial_name("forge-token")).exists(),
+            "temp still holds the secret after a failed publish"
+        );
+
+        std::fs::remove_dir_all(&dir).ok();
+    }
+
     #[test]
     fn refuses_symlink_leaf_and_leaves_target_untouched() {
         let dir = scratch();