From 7412a0f1bdcabc9e29681cbebb7f59120c141686 Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 1 Nov 2026 14:12:26 +0100 Subject: [PATCH] walk an agent's dirs by fd when moving them, and refuse a gid that differs from the uid --- docs/agent-lifecycle/persistence.md | 6 +- hive-priv/src/userns.rs | 310 +++++++++++++++++++++------- 2 files changed, 245 insertions(+), 71 deletions(-) diff --git a/docs/agent-lifecycle/persistence.md b/docs/agent-lifecycle/persistence.md index c60897df..07426830 100644 --- a/docs/agent-lifecycle/persistence.md +++ b/docs/agent-lifecycle/persistence.md @@ -478,11 +478,13 @@ can't switch it on or off. template owned by the agent user. A later request with the marker present chowns nothing. A running container waits for its next stop. The socket dir is on a tmpfs, so `EnsureAgentSocketDir` hands it to `dataOwner` at every - start. + 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. - **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. Pin `services.hyperhive.agent.user.uid` and `.gid` to one number in the - agent's config before listing it. + agent's config, and start it once, before listing it. The move refuses an + agent whose `state` dir has another gid than uid, naming the agent. - **Leaving the list is manual.** hive-priv refuses to start an agent whose marker exists but who is no longer on the list, and one whose `dataOwner` changed. Chown the four dirs back, then delete the marker. diff --git a/hive-priv/src/userns.rs b/hive-priv/src/userns.rs index 66c8a03b..48e0f8ed 100644 --- a/hive-priv/src/userns.rs +++ b/hive-priv/src/userns.rs @@ -16,11 +16,15 @@ //! in the template root too. The agent's socket dir lives on a tmpfs and goes //! to the data owner at every start instead ([`owner_for_agent`]). -use std::os::unix::fs::{MetadataExt as _, lchown}; +use std::ffi::CStr; +use std::os::fd::{AsFd, BorrowedFd, OwnedFd}; +use std::os::unix::fs::lchown; use std::path::{Path, PathBuf}; use anyhow::{Context as _, Result, bail}; use hive_priv_sock::{AGENT_STATE_ROOT, BindMount}; +use rustix::fs::{AtFlags, Mode, OFlags}; +use rustix::io::Errno; use crate::files::SOCKET_DIR_ROOT; @@ -51,7 +55,27 @@ pub(crate) struct Env<'a> { /// Lowest uid taken for an agent's own: below it a dir belongs to root or /// to a system user such as hive-core. pub(crate) min_uid: u32, + /// Whether an agent's gid has to equal its uid: the id map copies the uid + /// map for gids, so a group of another number has no mapping. + pub(crate) same_gid: bool, + /// Chown a mount point in the root template. pub(crate) chown: &'a dyn Fn(&Path, u32, u32) -> std::io::Result<()>, + /// Chown the entry `name` of an open directory, following no link; `"."` + /// is the directory itself. The only way the data walk changes ownership. + pub(crate) chown_at: &'a ChownAt<'a>, +} + +/// The signature of [`Env::chown_at`]. +pub(crate) type ChownAt<'a> = dyn Fn(BorrowedFd<'_>, &CStr, u32, u32) -> std::io::Result<()> + 'a; + +fn chown_at(dir: BorrowedFd<'_>, name: &CStr, uid: u32, gid: u32) -> std::io::Result<()> { + Ok(rustix::fs::chownat( + dir, + name, + Some(rustix::fs::Uid::from_raw(uid)), + Some(rustix::fs::Gid::from_raw(gid)), + AtFlags::SYMLINK_NOFOLLOW, + )?) } /// The data owner for `agent` under the unit's environment: `None` when the @@ -112,7 +136,9 @@ pub(crate) fn activate(container: &str, agent: &str, binds: &[BindMount]) -> Res owner, stopped, min_uid: MIN_AGENT_UID, + same_gid: true, chown: &|path, uid, gid| lchown(path, Some(uid), Some(gid)), + chown_at: &chown_at, }, agent, binds, @@ -182,55 +208,133 @@ fn write_marker(path: &Path, owner: u32, (uid, gid): (u32, u32)) -> Result<()> { /// and gid, read from its `state` dir before the chown. /// /// Only the [`DATA_DIRS`] under `/` are touched, each only -/// if it is a real directory. The walk does not follow links or cross into -/// another filesystem, and the container is stopped while it runs. +/// 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)> { - let base = env.state_root.join(agent); - let state = base.join("state"); - let meta = - std::fs::symlink_metadata(&state).with_context(|| format!("stat {}", state.display()))?; - let (uid, gid) = (meta.uid(), meta.gid()); - if !meta.is_dir() || uid < env.min_uid || uid == owner { - bail!( - "{agent}: {} is not a directory owned by a container user (uid {uid}); start the \ - agent once without the namespace so its user exists", - state.display() - ); - } + 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 { - let dir = base.join(sub); - match std::fs::symlink_metadata(&dir) { - Ok(m) if m.is_dir() => chown_tree(env, &dir, m.dev(), owner)?, - Ok(_) => bail!("{} is not a directory", dir.display()), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => return Err(e).with_context(|| format!("stat {}", dir.display())), + 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((uid, gid)) + Ok((st.st_uid, st.st_gid)) } -/// Chown everything under `top`, `top` included, to `owner:owner`. -fn chown_tree(env: &Env<'_>, top: &Path, dev: u64, owner: u32) -> Result<()> { - let mut dirs: Vec = vec![top.to_path_buf()]; - while let Some(dir) = dirs.pop() { - (env.chown)(&dir, owner, owner).with_context(|| format!("chown {}", dir.display()))?; - for entry in std::fs::read_dir(&dir).with_context(|| format!("read {}", dir.display()))? { - let entry = entry?; - let meta = entry.metadata()?; - if meta.dev() != dev { - continue; - } - if meta.is_dir() { - dirs.push(entry.path()); - } else { - (env.chown)(&entry.path(), owner, owner) - .with_context(|| format!("chown {}", entry.path().display()))?; +/// Refuse an agent whose `state` dir is not owned by a container user, or whose +/// gid differs from its uid when `same_gid` asks for them to match. +/// +/// The ids come from the dir's owner, which the agent's own root can set, so +/// this catches a misconfigured pin, not a hostile agent. +fn check_ids( + agent: &str, + (uid, gid): (u32, u32), + owner: u32, + (min_uid, same_gid): (u32, bool), +) -> Result<()> { + if uid < min_uid || uid == owner { + bail!( + "{agent}: its state dir is owned by uid {uid}, not a container user; start the \ + agent once without the namespace so its user exists" + ); + } + if same_gid && gid != uid { + bail!( + "{agent}: its state dir is owned by {uid}:{gid}, and a namespaced agent's gid has \ + to equal its uid; pin services.hyperhive.agent.user.uid and .gid to {uid} in its \ + config and start it once before listing it" + ); + } + Ok(()) +} + +/// The deepest directory [`chown_tree`] enters. It holds one fd per level, and +/// an agent can make a tree as deep as it likes. +const MAX_DEPTH: u32 = 256; + +/// Open `path` as a directory, following no link in its last component. +fn open_base(path: &Path) -> Result { + rustix::fs::open( + path, + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC, + Mode::empty(), + ) + .with_context(|| format!("open {}", path.display())) +} + +/// Open `name` under `dir` as a directory without following a link. `None` +/// when the entry is something else, a link included; an error when it is +/// missing or unreadable. +fn open_dir_at(dir: impl AsFd, name: impl rustix::path::Arg) -> Result> { + match rustix::fs::openat( + dir, + name, + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC, + Mode::empty(), + ) { + Ok(fd) => Ok(Some(fd)), + Err(Errno::NOTDIR | Errno::LOOP) => Ok(None), + Err(e) => Err(std::io::Error::from(e).into()), + } +} + +/// Chown the directory `top` is open on, and everything under it, to +/// `owner:owner`. +/// +/// The walk never names a path. Each directory is entered with +/// `openat(parent_fd, name, O_NOFOLLOW|O_DIRECTORY)` and each other entry is +/// chowned through `chown_at` on its parent's fd, which does not follow a link. +/// A directory swapped for a link while this runs is therefore chowned as the +/// link, or skipped, and never followed. A directory on another filesystem than +/// `top` is left alone. +fn chown_tree(env: &Env<'_>, top: &OwnedFd, owner: u32, depth: u32) -> Result<()> { + if depth > MAX_DEPTH { + bail!("a data dir is nested more than {MAX_DEPTH} levels deep"); + } + let dev = rustix::fs::fstat(top)?.st_dev; + (env.chown_at)(top.as_fd(), c".", owner, owner).context("chown a data dir")?; + for entry in rustix::fs::Dir::read_from(top)? { + let entry = entry?; + let name = entry.file_name(); + if matches!(name.to_bytes(), b"." | b"..") { + continue; + } + match open_dir_at(top, name) { + Ok(Some(dir)) => { + if rustix::fs::fstat(&dir)?.st_dev == dev { + chown_tree(env, &dir, owner, depth + 1)?; + } } + Ok(None) => (env.chown_at)(top.as_fd(), name, owner, owner) + .with_context(|| format!("chown {}", name.to_string_lossy()))?, + // Removed since it was listed. + Err(e) if is_not_found(&e) => {} + Err(e) => return Err(e.context(format!("open {}", name.to_string_lossy()))), } } Ok(()) } +fn is_not_found(e: &anyhow::Error) -> bool { + e.downcast_ref::() + .is_some_and(|e| e.kind() == std::io::ErrorKind::NotFound) +} + /// Create, in the template root, the mount point of each bind whose source is /// one of the agent's data dirs or its socket dir, owned by the agent's ids /// inside the container. @@ -269,10 +373,11 @@ fn mount_points( #[cfg(test)] mod tests { - use super::{Env, activate_in, owner_for}; + use super::{Env, activate_in, check_ids, owner_for}; use crate::shared::test_support::scratch; use hive_priv_sock::BindMount; use std::cell::RefCell; + use std::os::fd::AsRawFd as _; use std::os::unix::fs::MetadataExt as _; use std::path::{Path, PathBuf}; @@ -287,7 +392,7 @@ mod tests { /// `/agents/a` with data in each dir, `config` beside them, a link /// out of `work`, and `outside/o` next to the tree. Returns the root. fn tree() -> PathBuf { - let root = scratch(); + let root = scratch().canonicalize().unwrap(); let a = root.join("agents/a"); for f in ["state/f", "harness/d/g", "work/x", "claude/c", "config/cfg"] { write(&a.join(f)); @@ -298,18 +403,35 @@ mod tests { } /// Run [`activate_in`] on `root` and return its answer with every path it - /// asked to chown, relative to `root`. - fn run( + /// asked to chown, relative to `root`. The walk's chowns arrive as an open + /// 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], + (min_uid, same_gid): (u32, bool), ) -> (anyhow::Result, Vec) { let seen = RefCell::new(Vec::new()); + let rel = |p: &Path| p.strip_prefix(root).unwrap().display().to_string(); let chown = |p: &Path, _: u32, _: u32| { - seen.borrow_mut() - .push(p.strip_prefix(root).unwrap().display().to_string()); + seen.borrow_mut().push(rel(p)); + Ok(()) + }; + let chown_at = |fd: std::os::fd::BorrowedFd<'_>, name: &std::ffi::CStr, _: u32, _: u32| { + assert!( + !name.to_bytes().contains(&b'/'), + "{name:?} is not a bare name" + ); + let dir = std::fs::read_link(format!("/proc/self/fd/{}", fd.as_raw_fd())).unwrap(); + let path = if name == c"." { + dir + } else { + dir.join(name.to_str().unwrap()) + }; + seen.borrow_mut().push(rel(&path)); Ok(()) }; let (state_root, rootfs) = (root.join("agents"), root.join("rootfs")); @@ -321,13 +443,25 @@ mod tests { rootfs: &rootfs, owner, stopped, - min_uid: 0, + min_uid, + same_gid, chown: &chown, + chown_at: &chown_at, }; let answer = activate_in(&env, agent, binds); (answer, seen.into_inner()) } + fn run( + root: &Path, + owner: Option, + stopped: bool, + agent: &str, + binds: &[BindMount], + ) -> (anyhow::Result, Vec) { + run_with(root, owner, stopped, agent, binds, (0, false)) + } + fn bind(root: &Path, sub: &str, target: &str) -> BindMount { BindMount { host_path: root.join("agents/a").join(sub).display().to_string(), @@ -347,7 +481,9 @@ mod tests { } /// The move chowns the four data dirs and what is in them and nothing - /// else: not `config`, not what a link in `work` points at. + /// else: not `config`, and not what a link in `work` points at, which is + /// chowned as the link. Every chown of the walk is fd-relative (checked in + /// `run_with`). #[test] fn the_move_stays_inside_the_agents_data_dirs() { let root = tree(); @@ -370,6 +506,29 @@ mod tests { assert!(root.join("marker/a").exists()); } + /// A link to a directory is never entered, whatever the listing said it + /// was when it was read: nothing under the target is chowned, and the + /// control is that the same walk chowns the real directories beside it. + #[test] + fn a_link_to_a_directory_is_not_entered() { + let root = tree(); + let (answer, seen) = run(&root, Some(OWNER), true, "a", &[]); + assert!(answer.unwrap()); + assert!(seen.contains(&"agents/a/harness/d/g".to_owned()), "control"); + assert!(seen.iter().all(|p| !p.starts_with("outside")), "{seen:?}"); + } + + /// A tree deeper than the walk's fd budget is refused, and nothing is + /// recorded as moved. + #[test] + fn a_tree_nested_too_deep_is_refused() { + let root = tree(); + std::fs::create_dir_all(root.join("agents/a/work").join("d/".repeat(300))).unwrap(); + let (answer, _) = run(&root, Some(OWNER), true, "a", &[]); + assert!(format!("{:#}", answer.unwrap_err()).contains("levels deep")); + assert!(!root.join("marker/a").exists()); + } + /// Once the marker is there a second call chowns nothing. The first call /// in `the_move_stays_inside_the_agents_data_dirs` is the control that /// the same setup does chown. @@ -436,29 +595,42 @@ mod tests { let euid = std::fs::metadata(root.join("agents/a/state")) .unwrap() .uid(); - let seen = RefCell::new(Vec::new()); - let chown = |p: &Path, _: u32, _: u32| { - seen.borrow_mut().push(p.to_path_buf()); - Ok(()) - }; - let (state_root, rootfs) = (root.join("agents"), root.join("rootfs")); - let mut env = Env { - state_root: &state_root, - socket_dir: &root.join("run/a"), - marker_root: &root.join("marker"), - rootfs: &rootfs, - owner: Some(OWNER), - stopped: true, - min_uid: euid + 1, - chown: &chown, - }; - assert!(activate_in(&env, "a", &[]).is_err(), "below the minimum"); - assert!(seen.borrow().is_empty()); - env.min_uid = euid; - env.owner = Some(euid); - assert!(activate_in(&env, "a", &[]).is_err(), "already the owner"); - env.owner = Some(OWNER); - assert!(activate_in(&env, "a", &[]).unwrap(), "control"); + let below = run_with(&root, Some(OWNER), true, "a", &[], (euid + 1, false)); + assert!(below.0.is_err() && below.1.is_empty(), "below the minimum"); + let owner = run_with(&root, Some(euid), true, "a", &[], (euid, false)); + assert!(owner.0.is_err(), "already the owner"); + let ok = run_with(&root, Some(OWNER), true, "a", &[], (euid, false)); + assert!(ok.0.unwrap(), "control"); + } + + /// The production `chown_at` acts on the link itself: a link to nothing is + /// chowned, where following it would fail with ENOENT. + #[test] + fn chown_at_does_not_follow_a_link() { + let root = scratch(); + std::os::unix::fs::symlink(root.join("missing"), root.join("l")).unwrap(); + let dir = super::open_base(&root).unwrap(); + let me = std::fs::metadata(&root).unwrap(); + super::chown_at(std::os::fd::AsFd::as_fd(&dir), c"l", me.uid(), me.gid()).unwrap(); + } + + /// A gid that differs from the uid is refused with the agent named, when + /// asked for; the same ids pass without it. Through `activate_in` the + /// answer follows whether the scratch dir's own ids match. + #[test] + fn a_gid_other_than_the_uid_is_refused() { + let err = check_ids("juno", (1000, 999), OWNER, (1000, true)).unwrap_err(); + let msg = format!("{err:#}"); + assert!(msg.contains("juno") && msg.contains("1000:999"), "{msg}"); + check_ids("juno", (1000, 999), OWNER, (1000, false)).unwrap(); + check_ids("juno", (1000, 1000), OWNER, (1000, true)).unwrap(); + assert!(check_ids("juno", (0, 0), OWNER, (1000, true)).is_err()); + assert!(check_ids("juno", (OWNER, OWNER), OWNER, (1000, true)).is_err()); + + let root = tree(); + let meta = std::fs::metadata(root.join("agents/a/state")).unwrap(); + let (answer, _) = run_with(&root, Some(OWNER), true, "a", &[], (0, true)); + assert_eq!(answer.is_ok(), meta.uid() == meta.gid()); } /// A mount point is made for a bind of the agent's own data dir, and for