walk an agent's dirs by fd when moving them, and refuse a gid that differs from the uid

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.

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 `<state_root>/<agent>` are touched, each only
-/// if it is a real directory. The walk does not follow links or cross into
+/// if it is a real directory. Every step is relative to an open directory fd
-/// another filesystem, and the container is stopped while it runs.
+/// 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 base_path = env.state_root.join(agent);
-    let state = base.join("state");
+    let base = open_base(&base_path)?;
-    let meta =
+    let state = open_dir_at(&base, "state")
-        std::fs::symlink_metadata(&state).with_context(|| format!("stat {}", state.display()))?;
+        .with_context(|| format!("open {}", base_path.join("state").display()))?
-    let (uid, gid) = (meta.uid(), meta.gid());
+        .with_context(|| format!("{} is not a directory", base_path.join("state").display()))?;
-    if !meta.is_dir() || uid < env.min_uid || uid == owner {
+    let st = rustix::fs::fstat(&state).context("stat state")?;
-        bail!(
+    check_ids(
-            "{agent}: {} is not a directory owned by a container user (uid {uid}); start the \
+        agent,
-             agent once without the namespace so its user exists",
+        (st.st_uid, st.st_gid),
-            state.display()
+        owner,
-        );
+        (env.min_uid, env.same_gid),
-    }
+    )?;
     for sub in DATA_DIRS {
-        let dir = base.join(sub);
+        match open_dir_at(&base, sub) {
-        match std::fs::symlink_metadata(&dir) {
+            Ok(Some(dir)) => chown_tree(env, &dir, owner, 0)?,
-            Ok(m) if m.is_dir() => chown_tree(env, &dir, m.dev(), owner)?,
+            Ok(None) => bail!("{} is not a directory", base_path.join(sub).display()),
-            Ok(_) => bail!("{} is not a directory", dir.display()),
+            Err(e) if is_not_found(&e) => {}
-            Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
+            Err(e) => return Err(e.context(format!("open {sub}"))),
-            Err(e) => return Err(e).with_context(|| format!("stat {}", dir.display())),
         }
     }
-    Ok((uid, gid))
+    Ok((st.st_uid, st.st_gid))
 }
 
-/// Chown everything under `top`, `top` included, to `owner:owner`.
+/// Refuse an agent whose `state` dir is not owned by a container user, or whose
-fn chown_tree(env: &Env<'_>, top: &Path, dev: u64, owner: u32) -> Result<()> {
+/// gid differs from its uid when `same_gid` asks for them to match.
-    let mut dirs: Vec<PathBuf> = vec![top.to_path_buf()];
+///
-    while let Some(dir) = dirs.pop() {
+/// The ids come from the dir's owner, which the agent's own root can set, so
-        (env.chown)(&dir, owner, owner).with_context(|| format!("chown {}", dir.display()))?;
+/// this catches a misconfigured pin, not a hostile agent.
-        for entry in std::fs::read_dir(&dir).with_context(|| format!("read {}", dir.display()))? {
+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<OwnedFd> {
+    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<Option<OwnedFd>> {
+    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 meta = entry.metadata()?;
+        let name = entry.file_name();
-            if meta.dev() != dev {
+        if matches!(name.to_bytes(), b"." | b"..") {
             continue;
         }
-            if meta.is_dir() {
+        match open_dir_at(top, name) {
-                dirs.push(entry.path());
+            Ok(Some(dir)) => {
-            } else {
+                if rustix::fs::fstat(&dir)?.st_dev == dev {
-                (env.chown)(&entry.path(), owner, owner)
+                    chown_tree(env, &dir, owner, depth + 1)?;
-                    .with_context(|| format!("chown {}", entry.path().display()))?;
                 }
             }
+            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::<std::io::Error>()
+        .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 {
     /// `<root>/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`.
+    /// asked to chown, relative to `root`. The walk's chowns arrive as an open
-    fn run(
+    /// 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],
+        (min_uid, same_gid): (u32, bool),
     ) -> (anyhow::Result<bool>, Vec<String>) {
         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()
+            seen.borrow_mut().push(rel(p));
-                .push(p.strip_prefix(root).unwrap().display().to_string());
+            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<u32>,
+        stopped: bool,
+        agent: &str,
+        binds: &[BindMount],
+    ) -> (anyhow::Result<bool>, Vec<String>) {
+        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 below = run_with(&root, Some(OWNER), true, "a", &[], (euid + 1, false));
-        let chown = |p: &Path, _: u32, _: u32| {
+        assert!(below.0.is_err() && below.1.is_empty(), "below the minimum");
-            seen.borrow_mut().push(p.to_path_buf());
+        let owner = run_with(&root, Some(euid), true, "a", &[], (euid, false));
-            Ok(())
+        assert!(owner.0.is_err(), "already the owner");
-        };
+        let ok = run_with(&root, Some(OWNER), true, "a", &[], (euid, false));
-        let (state_root, rootfs) = (root.join("agents"), root.join("rootfs"));
+        assert!(ok.0.unwrap(), "control");
-        let mut env = Env {
+    }
-            state_root: &state_root,
+
-            socket_dir: &root.join("run/a"),
+    /// The production `chown_at` acts on the link itself: a link to nothing is
-            marker_root: &root.join("marker"),
+    /// chowned, where following it would fail with ENOENT.
-            rootfs: &rootfs,
+    #[test]
-            owner: Some(OWNER),
+    fn chown_at_does_not_follow_a_link() {
-            stopped: true,
+        let root = scratch();
-            min_uid: euid + 1,
+        std::os::unix::fs::symlink(root.join("missing"), root.join("l")).unwrap();
-            chown: &chown,
+        let dir = super::open_base(&root).unwrap();
-        };
+        let me = std::fs::metadata(&root).unwrap();
-        assert!(activate_in(&env, "a", &[]).is_err(), "below the minimum");
+        super::chown_at(std::os::fd::AsFd::as_fd(&dir), c"l", me.uid(), me.gid()).unwrap();
-        assert!(seen.borrow().is_empty());
+    }
-        env.min_uid = euid;
+
-        env.owner = Some(euid);
+    /// A gid that differs from the uid is refused with the agent named, when
-        assert!(activate_in(&env, "a", &[]).is_err(), "already the owner");
+    /// asked for; the same ids pass without it. Through `activate_in` the
-        env.owner = Some(OWNER);
+    /// answer follows whether the scratch dir's own ids match.
-        assert!(activate_in(&env, "a", &[]).unwrap(), "control");
+    #[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