make a userns data move resumable and refuse before the first chown

docs/agent-lifecycle/persistence.md

@@ -498,6 +498,13 @@ leaves the agent as it is.
   is on a tmpfs, so `EnsureAgentSocketDir` hands it to `dataOwner` at every
   start. The walk works by open directory fd and follows no link, so an agent
   or manager that can write the dirs can't redirect it to a host path.
+- **A move that fails starts over.** hive-priv opens all four dirs and
+  checks the agent's ids before the first chown, so a link or file where a dir
+  goes refuses with nothing changed. It then writes the ids to
+  `/var/lib/hive-priv/userns/<name>.pending`. If the walk fails after that (a
+  chown error, a tree nested too deep), the next start repeats it from those
+  ids, then writes the marker and removes the pending file. hive-priv refuses a
+  pending file that doesn't hold `<owner> <uid> <gid>`, and names it.
 - **The agent's gid must equal its uid.** `owneridmap` maps the gid of a bind
   with the uid map, so a group other than the uid's has no mapping and writes
   fail. The agent module asserts that `user.uid` and `user.gid` hold the
@@ -506,7 +513,8 @@ leaves the agent as it is.
   gid than uid, naming the agent.
 - **Turning it off is manual.** hive-priv refuses to start an agent whose
   marker exists while its record says false, and one whose `dataOwner`
-  changed. Chown the four dirs back, then delete the marker.
+  changed. Chown the four dirs back, then delete the marker and any
+  `<name>.pending` beside it.
 - **Keep a `ManageRootAgent` holder off it.** The other agents' dirs it mounts
   aren't remapped.
 

hive-priv/src/userns.rs

@@ -11,7 +11,11 @@
 //! Getting there moves the agent's data once. [`activate`] chowns its `state`,
 //! `harness`, `work` and `claude` dirs to the data owner, never touching
 //! anything else, and records that in a marker only root can write; the next
-//! call sees the marker and does nothing. `owneridmap` needs the target
+//! call sees the marker and does nothing. Every refusal that can be known up
+//! front happens before the first chown, and the agent's own ids are written to
+//! `<agent>.pending` before it, so a walk that fails midway is repeated from
+//! those ids rather than read off a `state` dir that already changed hands.
+//! `owneridmap` needs the target
 //! directory to be owned by the agent when nspawn mounts, and nspawn creates a
 //! missing one as root in a throwaway snapshot, so the mount points are made
 //! in the template root too. The agent's socket dir lives on a tmpfs and goes
@@ -32,7 +36,8 @@ use crate::files::SOCKET_DIR_ROOT;
 const DATA_OWNER_ENV: &str = "HIVE_PRIV_USERNS_DATA_OWNER";
 
 /// One `<owner> <uid> <gid>` file per agent: the data owner its data was moved
-/// to, and the agent's own ids inside the container.
+/// to, and the agent's own ids inside the container. A move that did not finish
+/// leaves the same content in `<agent>.pending` beside it.
 const MARKER_ROOT: &str = "/var/lib/hive-priv/userns";
 
 /// NixOS gives a normal user, which the agent is, an id from 1000 up.
@@ -160,6 +165,7 @@ pub(crate) fn activate(
 pub(crate) fn activate_in(env: &Env<'_>, agent: &str, binds: &[BindMount]) -> Result<bool> {
     crate::shared::validate_agent_name(agent)?;
     let marker = env.marker_root.join(agent);
+    let pending = env.marker_root.join(format!("{agent}.pending"));
     let moved = read_marker(&marker)?;
     let Some(owner) = env.owner else {
         if moved.is_some() {
@@ -178,8 +184,11 @@ pub(crate) fn activate_in(env: &Env<'_>, agent: &str, binds: &[BindMount]) -> Re
         }
         None if !env.stopped => return Ok(false),
         None => {
-            let ids = move_data(env, agent, owner)?;
+            let resume = read_marker(&pending)?.map(|(_, uid, gid)| (uid, gid));
+            let ids = move_data(env, agent, owner, &pending, resume)?;
             write_marker(&marker, owner, ids)?;
+            std::fs::remove_file(&pending)
+                .with_context(|| format!("remove {}", pending.display()))?;
             ids
         }
     };
@@ -189,7 +198,8 @@ pub(crate) fn activate_in(env: &Env<'_>, agent: &str, binds: &[BindMount]) -> Re
     Ok(true)
 }
 
-/// `<owner> <uid> <gid>` from the marker, `None` when there is none.
+/// `<owner> <uid> <gid>` from the marker or pending file, `None` when there is
+/// none.
 fn read_marker(path: &Path) -> Result<Option<(u32, u32, u32)>> {
     let text = match std::fs::read_to_string(path) {
         Ok(text) => text,
@@ -216,35 +226,53 @@ fn write_marker(path: &Path, owner: u32, (uid, gid): (u32, u32)) -> Result<()> {
 }
 
 /// Chown the agent's data dirs to `owner:owner`; returns the agent's own uid
-/// and gid, read from its `state` dir before the chown.
+/// and gid.
+///
+/// Every data dir is opened and the ids checked before the first chown, so a
+/// dir that is a link or a file refuses while nothing has changed. The ids are
+/// read from `state` and written to `pending` first; `resume` is the ids a
+/// failed earlier walk left there, used as they are because `state` may already
+/// belong to `owner`. Walking a dir again chowns it again to the same owner.
 ///
 /// Only the [`DATA_DIRS`] under `<state_root>/<agent>` are touched, each only
 /// if it is a real directory. Every step is relative to an open directory fd
 /// and follows no link (see [`chown_tree`]), because an agent that holds the
 /// dirs, a manager for one, can write them while this runs as root. The
 /// container itself is stopped.
-fn move_data(env: &Env<'_>, agent: &str, owner: u32) -> Result<(u32, u32)> {
+fn move_data(
+    env: &Env<'_>,
+    agent: &str,
+    owner: u32,
+    pending: &Path,
+    resume: Option<(u32, u32)>,
+) -> Result<(u32, u32)> {
     let base_path = env.state_root.join(agent);
     let base = open_base(&base_path)?;
     let state = open_dir_at(&base, "state")
         .with_context(|| format!("open {}", base_path.join("state").display()))?
         .with_context(|| format!("{} is not a directory", base_path.join("state").display()))?;
-    let st = rustix::fs::fstat(&state).context("stat state")?;
+    let mut dirs = vec![state];
-    check_ids(
+    for sub in &DATA_DIRS[1..] {
-        agent,
+        match open_dir_at(&base, *sub) {
-        (st.st_uid, st.st_gid),
+            Ok(Some(dir)) => dirs.push(dir),
-        owner,
-        (env.min_uid, env.same_gid),
-    )?;
-    for sub in DATA_DIRS {
-        match open_dir_at(&base, sub) {
-            Ok(Some(dir)) => chown_tree(env, &dir, owner, 0)?,
             Ok(None) => bail!("{} is not a directory", base_path.join(sub).display()),
             Err(e) if is_not_found(&e) => {}
             Err(e) => return Err(e.context(format!("open {sub}"))),
         }
     }
-    Ok((st.st_uid, st.st_gid))
+    let ids = if let Some(ids) = resume {
+        ids
+    } else {
+        let st = rustix::fs::fstat(&dirs[0]).context("stat state")?;
+        let ids = (st.st_uid, st.st_gid);
+        check_ids(agent, ids, owner, (env.min_uid, env.same_gid))?;
+        write_marker(pending, owner, ids)?;
+        ids
+    };
+    for dir in &dirs {
+        chown_tree(env, dir, owner, 0)?;
+    }
+    Ok(ids)
 }
 
 /// Refuse an agent whose `state` dir is not owned by a container user, or whose
@@ -418,12 +446,26 @@ mod tests {
     /// directory and a bare name; the name is checked to hold no `/`, and the
     /// path is rebuilt from what the fd points at.
     fn run_with(
+        root: &Path,
+        owner: Option<u32>,
+        stopped: bool,
+        agent: &str,
+        binds: &[BindMount],
+        ids: (u32, bool),
+    ) -> (anyhow::Result<bool>, Vec<String>) {
+        run_failing(root, owner, stopped, agent, binds, ids, None)
+    }
+
+    /// [`run_with`], where the chown of the path `fail` (relative to `root`)
+    /// fails after it is recorded.
+    fn run_failing(
         root: &Path,
         owner: Option<u32>,
         stopped: bool,
         agent: &str,
         binds: &[BindMount],
         (min_uid, same_gid): (u32, bool),
+        fail: Option<&str>,
     ) -> (anyhow::Result<bool>, Vec<String>) {
         let seen = RefCell::new(Vec::new());
         let rel = |p: &Path| p.strip_prefix(root).unwrap().display().to_string();
@@ -443,6 +485,9 @@ mod tests {
                 dir.join(name.to_str().unwrap())
             };
             seen.borrow_mut().push(rel(&path));
+            if fail == Some(rel(&path).as_str()) {
+                return Err(std::io::Error::other("injected"));
+            }
             Ok(())
         };
         let (state_root, rootfs) = (root.join("agents"), root.join("rootfs"));
@@ -602,6 +647,88 @@ mod tests {
         assert!(!root.join("marker/a").exists());
     }
 
+    /// A refusal that is known before the walk leaves no trace: not one chown,
+    /// no marker, no pending file. The `state` chown in the control run is what
+    /// the refusal used to let through.
+    #[test]
+    fn a_refusal_before_the_walk_changes_nothing() {
+        let root = tree();
+        let work = root.join("agents/a/work");
+        std::fs::remove_dir_all(&work).unwrap();
+        std::os::unix::fs::symlink(root.join("outside"), &work).unwrap();
+        let (answer, seen) = run(&root, Some(OWNER), true, "a", &[]);
+        assert!(answer.is_err());
+        assert!(seen.is_empty(), "{seen:?}");
+        assert!(!root.join("marker/a").exists());
+        assert!(!root.join("marker/a.pending").exists());
+
+        std::fs::remove_file(&work).unwrap();
+        std::fs::create_dir(&work).unwrap();
+        let (answer, seen) = run(&root, Some(OWNER), true, "a", &[]);
+        assert!(answer.unwrap());
+        assert!(seen.contains(&"agents/a/state".to_owned()), "control");
+    }
+
+    /// A walk that fails midway leaves the ids it read in `<agent>.pending`,
+    /// and the next call walks again from them, without reading `state`'s
+    /// owner: the minimum uid of the second call is above any real uid, which
+    /// refuses a fresh move.
+    #[test]
+    fn a_walk_that_failed_is_resumed_from_the_pending_ids() {
+        let root = tree();
+        let uid = std::fs::metadata(root.join("agents/a/state"))
+            .unwrap()
+            .uid();
+        let gid = std::fs::metadata(root.join("agents/a/state"))
+            .unwrap()
+            .gid();
+        let (answer, seen) = run_failing(
+            &root,
+            Some(OWNER),
+            true,
+            "a",
+            &[],
+            (0, false),
+            Some("agents/a/harness"),
+        );
+        assert!(format!("{:#}", answer.unwrap_err()).contains("injected"));
+        assert!(seen.contains(&"agents/a/state".to_owned()), "state first");
+        assert!(!root.join("marker/a").exists());
+        let pending = std::fs::read_to_string(root.join("marker/a.pending")).unwrap();
+        assert_eq!(pending, format!("{OWNER} {uid} {gid}\n"));
+
+        let saved = std::fs::read_to_string(root.join("marker/a.pending")).unwrap();
+        std::fs::remove_file(root.join("marker/a.pending")).unwrap();
+        let (answer, _) = run_with(&root, Some(OWNER), true, "a", &[], (u32::MAX, false));
+        assert!(answer.is_err(), "control: without pending, ids are checked");
+        std::fs::write(root.join("marker/a.pending"), saved).unwrap();
+
+        let (answer, seen) = run_with(&root, Some(OWNER), true, "a", &[], (u32::MAX, false));
+        assert!(answer.unwrap());
+        assert!(seen.contains(&"agents/a/work/x".to_owned()), "{seen:?}");
+        assert_eq!(
+            std::fs::read_to_string(root.join("marker/a")).unwrap(),
+            pending
+        );
+        assert!(!root.join("marker/a.pending").exists());
+    }
+
+    /// A pending file that does not hold three ids is refused, named, before
+    /// anything is chowned.
+    #[test]
+    fn a_pending_file_that_does_not_parse_is_refused() {
+        let root = tree();
+        for garbage in ["not ids\n", "60000 1000\n", ""] {
+            std::fs::create_dir_all(root.join("marker")).unwrap();
+            std::fs::write(root.join("marker/a.pending"), garbage).unwrap();
+            let (answer, seen) = run(&root, Some(OWNER), true, "a", &[]);
+            let msg = format!("{:#}", answer.unwrap_err());
+            assert!(msg.contains("a.pending"), "{msg}");
+            assert!(seen.is_empty(), "{seen:?}");
+            assert!(!root.join("marker/a").exists());
+        }
+    }
+
     #[test]
     fn a_path_is_not_an_agent_name() {
         let root = tree();