diff --git a/docs/agent-lifecycle/persistence.md b/docs/agent-lifecycle/persistence.md index e55a77a4..337d921d 100644 --- a/docs/agent-lifecycle/persistence.md +++ b/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/.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 ` `, 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 + `.pending` beside it. - **Keep a `ManageRootAgent` holder off it.** The other agents' dirs it mounts aren't remapped. diff --git a/hive-priv/src/userns.rs b/hive-priv/src/userns.rs index 44367205..c812db28 100644 --- a/hive-priv/src/userns.rs +++ b/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 +//! `.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 ` ` 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 `.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 { 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) } -/// ` ` from the marker, `None` when there is none. +/// ` ` from the marker or pending file, `None` when there is +/// none. fn read_marker(path: &Path) -> Result> { 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 `/` 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")?; - check_ids( - agent, - (st.st_uid, st.st_gid), - 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)?, + let mut dirs = vec![state]; + for sub in &DATA_DIRS[1..] { + match open_dir_at(&base, *sub) { + Ok(Some(dir)) => dirs.push(dir), 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, + stopped: bool, + agent: &str, + binds: &[BindMount], + ids: (u32, bool), + ) -> (anyhow::Result, Vec) { + 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, stopped: bool, agent: &str, binds: &[BindMount], (min_uid, same_gid): (u32, bool), + fail: Option<&str>, ) -> (anyhow::Result, Vec) { 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 `.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();