From 3fc1588e83796ead8fcf95a1a6062789e399a660 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 4 Aug 2026 00:25:29 +0200 Subject: [PATCH] fix: declare the agent socket dir's owner in tmpfiles, not by chown after /run/hive-agent/ was 0777 root root in steady state, not just during first spawn. A directory without the sticky bit lets any user unlink files in it, and the gateway container has all of /run/hive-agent bind-mounted in, so anything that could reach the path could delete an agent's agent.sock, bind its own, and receive that agent's todos from hive-c0re. Two mechanisms were writing the dir and undoing each other: the tmpfiles.d entry wrote 0777 root root, then hive-c0re round-tripped through hive-priv's ChownSocketDir to narrow it. `d` re-asserts mode and owner on every apply and the file is regenerated on any agent's spawn or destroy, so every such event reset every agent's dir back to world-writable. SyncAgentTmpfiles now carries each agent's container uid/gid and the entry declares the answer: 0751 . Three principals need the dir and no two share a group -- the harness binds its sockets (owner rwx), hive-c0re dials agent.sock and the gateway's nginx dials web.sock (both only need traverse, and both sockets are already 0666). Deletes ChownSocketDir and ChmodSocketDir, both priv_client wrappers, the either/or in host_config with its two swallowed warn!s, and the now-dead socket_dir_path -- two verbs off the privileged helper's surface and one round-trip off every agent spawn. Also makes the two tmpfiles rules for /run/hive-agent itself agree: the gateway module said hive-core, the generated file said root, and which won depended on the order systemd read them in. --- docs/gateway.md | 15 ++++ docs/security.md | 1 - hive-c0re/src/lifecycle/host_config.rs | 19 ++--- hive-c0re/src/lifecycle/mod.rs | 10 +++ hive-c0re/src/priv_client.rs | 30 ++------ hive-priv-sock/src/lib.rs | 41 ++++++---- hive-priv/src/main.rs | 94 +++++++++++------------ nix/host-modules/hive-gateway/default.nix | 3 + 8 files changed, 110 insertions(+), 103 deletions(-) diff --git a/docs/gateway.md b/docs/gateway.md index 69540319..f486c51d 100644 --- a/docs/gateway.md +++ b/docs/gateway.md @@ -95,6 +95,21 @@ now set unconditionally for every agent. The mechanism: bind, not file bind — file bind-mounts don't survive the harness's `unlink + bind(2)` cycle on socket replace. Per-agent subdir keeps each agent's container blind to siblings' sockets. + + **Ownership of that dir is declared, not repaired.** The tmpfiles.d + entry written by `SyncAgentTmpfiles` names the agent's container + uid/gid directly — `d /run/hive-agent/ 0751 -`. + Three principals need the dir and no two share a group: the harness + (owner, `rwx`, binds + unlinks its sockets), `hive-c0re` (dials + `agent.sock`) and the gateway's nginx (dials `web.sock`, and has all + of `/run/hive-agent` bind-mounted in). The latter two only need + traverse, which is what `o=--x` grants; both sockets are `0666`. + Do **not** reintroduce a chown here: tmpfiles re-applies this entry + on every boot *and* every agent spawn/destroy, so any ownership set + afterwards is reverted the next time any agent changes. The mode is + also load-bearing — a directory without the sticky bit lets any user + unlink files in it, so a world-writable socket dir would let anything + that can reach the path replace an agent's socket with its own. 3. **Marker gate**. After successful `bind_unix`, the harness drops `/hyperhive-socket-bound` next to the socket. c0re's `agent_sockets::write` filters its JSON map by marker presence — diff --git a/docs/security.md b/docs/security.md index 6a2b5e1b..d0ed66d3 100644 --- a/docs/security.md +++ b/docs/security.md @@ -212,7 +212,6 @@ known operations; there is no arbitrary command pass-through: | `WriteResourceLimits` | write `CPUQuota=`/`MemoryMax=`/`CPUWeight=`/`IOWeight=` systemd drop-in for agent container | | `RemoveServiceDropin` | remove `container@.service.d/` drop-in on destroy | | `DaemonReload` | `systemctl daemon-reload` | -| `ChownSocketDir` / `ChmodSocketDir` | chown/chmod `/run/hive-agent//` socket directory | | `RunForgeAdmin` | `nixos-container run hive-forge -- runuser -u forgejo -- forgejo admin ` | | `WriteAgentForgeToken` / `WriteAgentMatrixToken` | write `0600` credential file into agent state dir | | `RestartMatrixDaemon` | `systemctl --machine=h- restart hive-matrix-daemon.service` | diff --git a/hive-c0re/src/lifecycle/host_config.rs b/hive-c0re/src/lifecycle/host_config.rs index 1574f1a5..7a7c76a5 100644 --- a/hive-c0re/src/lifecycle/host_config.rs +++ b/hive-c0re/src/lifecycle/host_config.rs @@ -10,8 +10,8 @@ use hive_priv_sock::{BindMount, CredentialMount}; use crate::coordinator::{AgentPaths, HiveEnv}; use super::{ - AGENT_PREFIX, CONTAINER_RUNTIME_MOUNT, CONTAINER_SHARED_MOUNT, agent_uid_gid, - bridge_gateway_ip, container_claude_mount, container_name, validate, + AGENT_PREFIX, CONTAINER_RUNTIME_MOUNT, CONTAINER_SHARED_MOUNT, bridge_gateway_ip, + container_claude_mount, container_name, validate, }; /// Re-apply the per-container host-side config: nspawn flags (bind @@ -289,16 +289,11 @@ async fn set_nspawn_flags( let socket_dir = crate::agent_sockets::agent_dir_for(agent_name); std::fs::create_dir_all(&socket_dir) .with_context(|| format!("create {}", socket_dir.display()))?; - // Chown to the agent user so the non-root harness can bind(2) here. - // Falls back to 0777 on first spawn when uid lookup returns None - // (container /etc/passwd not yet rendered). - if let Some((uid, gid)) = agent_uid_gid(agent_name) { - if let Err(e) = crate::priv_client::chown_socket_dir(agent_name, uid, gid).await { - tracing::warn!(%agent_name, error = ?e, "chown socket dir failed"); - } - } else if let Err(e) = crate::priv_client::chmod_socket_dir(agent_name, 0o777).await { - tracing::warn!(%agent_name, error = ?e, "chmod socket dir failed"); - } + // Ownership is NOT repaired here. The dir's owner + mode are declared by + // the tmpfiles.d entry (`SyncAgentTmpfiles`), which is the mechanism that + // re-applies on every boot and every spawn — so a chown made here was + // silently reverted the next time any agent was spawned or destroyed. + // This `create_dir_all` only covers the window before that sync lands. binds.push(BindMount { host_path: socket_dir.to_string_lossy().into_owned(), container_path: socket_dir.to_string_lossy().into_owned(), diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs index a69b71a9..4bd00109 100644 --- a/hive-c0re/src/lifecycle/mod.rs +++ b/hive-c0re/src/lifecycle/mod.rs @@ -712,6 +712,16 @@ pub async fn sync_tmpfiles() { Ok(containers) => containers .into_iter() .filter_map(|c| c.strip_prefix(AGENT_PREFIX).map(str::to_owned)) + .map(|name| { + // Resolved here, not in hive-priv: the mapping lives in the + // container's /etc/passwd, which is c0re's to read. `None` + // until the container's first boot renders it. + let (uid, gid) = match agent_uid_gid(&name) { + Some((uid, gid)) => (Some(uid), Some(gid)), + None => (None, None), + }; + hive_priv_sock::AgentTmpfilesEntry { name, uid, gid } + }) .collect::>(), Err(e) => { tracing::warn!(error = ?e, "sync_tmpfiles: list failed; skipping"); diff --git a/hive-c0re/src/priv_client.rs b/hive-c0re/src/priv_client.rs index 4663db86..f2f45ed2 100644 --- a/hive-c0re/src/priv_client.rs +++ b/hive-c0re/src/priv_client.rs @@ -8,8 +8,8 @@ use anyhow::{Context as _, Result, bail}; use hive_priv_sock::{ - BindMount, CredentialMount, InfraAction, InfraContainer, JournalQuery, NetworkIsolation, - PRIV_SOCK, PrivEvent, PrivRequest, PrivResponse, PrivStream, + AgentTmpfilesEntry, BindMount, CredentialMount, InfraAction, InfraContainer, JournalQuery, + NetworkIsolation, PRIV_SOCK, PrivEvent, PrivRequest, PrivResponse, PrivStream, }; use std::os::fd::{AsRawFd as _, OwnedFd, RawFd}; @@ -338,23 +338,6 @@ pub async fn reload_gateway_nginx() -> Result<()> { ok(call(&PrivRequest::ReloadGatewayNginx).await?) } -pub async fn chown_socket_dir(agent_name: &str, uid: u32, gid: u32) -> Result<()> { - ok(call(&PrivRequest::ChownSocketDir { - agent_name: agent_name.to_owned(), - uid, - gid, - }) - .await?) -} - -pub async fn chmod_socket_dir(agent_name: &str, mode: u32) -> Result<()> { - ok(call(&PrivRequest::ChmodSocketDir { - agent_name: agent_name.to_owned(), - mode, - }) - .await?) -} - /// Run `forgejo admin ` inside the `hive-forge` container via /// hive-priv (which runs as root and can nsenter into the container). /// Returns `(stdout, stderr)` on success. @@ -652,15 +635,16 @@ pub async fn send_agent_snapshot_to_file( Ok(stdout) } -/// Write `/etc/tmpfiles.d/hyperhive-agents.conf` for `agents` (logical names, -/// e.g. `"atlas"`) and immediately apply it with `systemd-tmpfiles --create`. -/// See [`PrivRequest::SyncAgentTmpfiles`] for the full semantics. +/// Write `/etc/tmpfiles.d/hyperhive-agents.conf` for `agents` and immediately +/// apply it with `systemd-tmpfiles --create`. Each entry carries the agent's +/// container uid/gid so the socket dir's ownership is *declared* here rather +/// than corrected afterwards. See [`PrivRequest::SyncAgentTmpfiles`]. /// /// # Errors /// /// Returns an error if the priv socket call fails, if any agent name is /// invalid, or if `systemd-tmpfiles --create` exits non-zero. -pub async fn sync_agent_tmpfiles(agents: &[String]) -> Result<()> { +pub async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<()> { ok(call(&PrivRequest::SyncAgentTmpfiles { agents: agents.to_vec(), }) diff --git a/hive-priv-sock/src/lib.rs b/hive-priv-sock/src/lib.rs index 0c006a4b..1ec43637 100644 --- a/hive-priv-sock/src/lib.rs +++ b/hive-priv-sock/src/lib.rs @@ -375,19 +375,6 @@ pub enum PrivRequest { /// namespace via the machine bus (forbidden for unprivileged users). ReloadGatewayNginx, - // --- Socket dir ownership --- - /// Set ownership of `/run/hive-agent//` to `uid:gid`. - /// Called by `lifecycle::set_nspawn_flags` after `create_dir_all`. - ChownSocketDir { - agent_name: String, - uid: u32, - gid: u32, - }, - - /// Set mode of `/run/hive-agent//`. - /// Fallback when uid lookup returns `None` on first spawn. - ChmodSocketDir { agent_name: String, mode: u32 }, - // --- Forge admin CLI --- /// Run `forgejo admin ` inside the `hive-forge` container as the /// `forgejo` unix user. hive-priv executes: @@ -754,12 +741,34 @@ pub enum PrivRequest { /// Called at hive-c0re startup and after every agent spawn / destroy. /// Agents are logical names (validated by `validate_agent_name`). SyncAgentTmpfiles { - /// Logical agent names (e.g. `"atlas"`, `"ruth"`). hive-priv validates - /// each name before writing any path component derived from it. - agents: Vec, + /// One entry per live agent. hive-priv validates each name before + /// writing any path component derived from it. + agents: Vec, }, } +/// One agent's runtime-dir declaration for `SyncAgentTmpfiles`. +/// +/// Carries the container uid/gid so the tmpfiles entry can *declare* who owns +/// `/run/hive-agent/` instead of having it corrected afterwards by a +/// privileged chown. The two mechanisms used to fight: the tmpfiles line wrote +/// `0777 root root` and a follow-up `ChownSocketDir` narrowed it, but any +/// later spawn or destroy re-applied the file and reset *every* agent's dir +/// back to world-writable. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct AgentTmpfilesEntry { + /// Logical agent name (e.g. `"atlas"`, `"ruth"`). + pub name: String, + /// Container uid/gid of the agent user, when known. + /// + /// `None` only before the container's `/etc/passwd` has been rendered + /// (first boot). The dir must stay writable by the not-yet-identifiable + /// harness in that window, so hive-priv falls back to the historical + /// permissive mode for that one agent; the next sync tightens it. + pub uid: Option, + pub gid: Option, +} + /// Response from the privileged helper. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PrivResponse { diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index b2091554..27312cb4 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -22,10 +22,10 @@ use std::path::{Path, PathBuf}; use anyhow::{Context as _, Result, bail}; use hive_priv_sock::{ - AGENT_PREFIX, AGENT_RUNTIME_ROOT, AGENT_STATE_ROOT, BindMount, CredentialMount, InfraAction, - InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT, NetworkIsolation, - PAUSED_MARKER_FILE, PRIV_SOCK, PrivEvent, PrivRequest, PrivResponse, PrivStream, - PrivStreamLine, SIBLING_CONTAINERS, + AGENT_PREFIX, AGENT_RUNTIME_ROOT, AGENT_STATE_ROOT, AgentTmpfilesEntry, BindMount, + CredentialMount, InfraAction, InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT, + NetworkIsolation, PAUSED_MARKER_FILE, PRIV_SOCK, PrivEvent, PrivRequest, PrivResponse, + PrivStream, PrivStreamLine, SIBLING_CONTAINERS, }; use tokio::io::{AsyncWriteExt, BufReader}; use tokio::net::unix::OwnedWriteHalf; @@ -418,17 +418,6 @@ async fn exec( PrivRequest::ReloadGatewayNginx => sync_gateway_nginx().await, - PrivRequest::ChownSocketDir { - ref agent_name, - uid, - gid, - } => chown_socket_dir(agent_name, uid, gid), - - PrivRequest::ChmodSocketDir { - ref agent_name, - mode, - } => chmod_socket_dir(agent_name, mode), - PrivRequest::RunForgeAdmin { ref args } => { for arg in args { validate_forge_admin_arg(arg)?; @@ -714,26 +703,6 @@ fn remove_service_dropin(container: &str) -> Result<(String, String)> { Ok((String::new(), String::new())) } -/// `ChownSocketDir` — chown the agent's host socket dir to its -/// container uid/gid. -fn chown_socket_dir(agent_name: &str, uid: u32, gid: u32) -> Result<(String, String)> { - validate_agent_name(agent_name)?; - let path = socket_dir_path(agent_name); - std::os::unix::fs::chown(&path, Some(uid), Some(gid)) - .with_context(|| format!("chown {} to {uid}:{gid}", path.display()))?; - Ok((String::new(), String::new())) -} - -/// `ChmodSocketDir` — set the mode on the agent's host socket dir. -fn chmod_socket_dir(agent_name: &str, mode: u32) -> Result<(String, String)> { - use std::os::unix::fs::PermissionsExt as _; - validate_agent_name(agent_name)?; - let path = socket_dir_path(agent_name); - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)) - .with_context(|| format!("chmod {:o} {}", mode, path.display()))?; - Ok((String::new(), String::new())) -} - /// `WriteResourceLimits` — drop the systemd resource settings into the /// container service's drop-in dir, together with a /// `ConditionPathIsDirectory=` guard on the agent's MCP runtime dir. @@ -2205,11 +2174,6 @@ fn container_system_name(name: &str) -> String { format!("{AGENT_PREFIX}{name}") } -/// Path of the per-agent unix-socket dir on the host. -fn socket_dir_path(agent_name: &str) -> PathBuf { - PathBuf::from(format!("{SOCKET_DIR_ROOT}/{agent_name}")) -} - /// Validate a logical agent name (the name hive-c0re uses internally, /// before the `h-` container prefix is applied). fn validate_agent_name(name: &str) -> Result<()> { @@ -2427,10 +2391,10 @@ fn write_bridge_dns_marker(container: &str, isolation: Option<&NetworkIsolation> /// - `/run/hive-agent/` (web socket dir, bind-mounted into container) const TMPFILES_PATH: &str = "/etc/tmpfiles.d/hyperhive-agents.conf"; -async fn sync_agent_tmpfiles(agents: &[String]) -> Result<(String, String)> { +async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, String)> { use std::fmt::Write as _; - for name in agents { - validate_agent_name(name)?; + for entry in agents { + validate_agent_name(&entry.name)?; } // Build tmpfiles.d content. Root dirs first, then per-agent. @@ -2441,20 +2405,48 @@ async fn sync_agent_tmpfiles(agents: &[String]) -> Result<(String, String)> { // tmpfiles.d entry here ensures it exists before hive-c0re starts (boot race). content.push_str("d /run/hyperhive 0750 hive-core hive-core -\n"); writeln!(content, "d {AGENT_RUNTIME_ROOT} 0755 hive-core hive-core -").ok(); - writeln!(content, "d {SOCKET_DIR_ROOT} 0755 root root -").ok(); + // `hive-core`, not root: c0re does the `create_dir_all` for a new agent's + // subdir itself, so a root-owned parent EACCESes on the first spawn of a + // fresh host. This must stay in step with the identical rule in + // `nix/host-modules/hive-gateway/default.nix` — the two files declared + // different owners for this one path, and which won depended on the order + // systemd happened to read them in. + writeln!(content, "d {SOCKET_DIR_ROOT} 0755 hive-core hive-core -").ok(); // Per-agent dirs. - for name in agents { + for entry in agents { + let name = &entry.name; writeln!( content, "d {AGENT_RUNTIME_ROOT}/{name} 0755 hive-core hive-core -" ) .ok(); - // 0777: agent harness (non-root uid) must bind sockets here. - // `d` adjusts mode/owner on existing dirs; world-writable matches - // the chmod_socket_dir(0o777) fallback so a runtime re-sync doesn't - // break a live agent's socket dir. host_config's chown_socket_dir - // tightens ownership afterwards when the agent uid is available. - writeln!(content, "d {SOCKET_DIR_ROOT}/{name} 0777 root root -").ok(); + // The agent's socket dir. Three principals need it and no two share a + // group, so the mode has to say so explicitly: + // + // owner = the agent user rwx binds + unlinks agent.sock/web.sock + // other = --x traverse only, no listing + // + // "other" covers hive-c0re (dials agent.sock) and the gateway's nginx + // (dials web.sock, and has all of /run/hive-agent bind-mounted in). + // Both sockets are 0666, so traversal is all they need. + // + // 0751 rather than the historical 0777 is a fix, not a tidy-up: a + // directory without the sticky bit lets *any* user unlink files in it, + // so world-writable here means anything that can reach the path could + // delete an agent's socket, bind its own, and receive that agent's + // todos. Declaring the owner here also ends the tug-of-war with the + // old ChownSocketDir: `d` re-applies on every sync, so a chown made + // afterwards was reset by the next agent's spawn. + if let (Some(uid), Some(gid)) = (entry.uid, entry.gid) { + writeln!(content, "d {SOCKET_DIR_ROOT}/{name} 0751 {uid} {gid} -").ok(); + } else { + // Before the container's /etc/passwd exists there is no uid to + // name, and the harness must still be able to bind. Keep the old + // permissive mode for that agent alone; the next sync (any spawn + // or destroy, or c0re restart) resolves the uid and tightens it. + tracing::info!(%name, "tmpfiles.d: agent uid unknown, deferring 0751 on socket dir"); + writeln!(content, "d {SOCKET_DIR_ROOT}/{name} 0777 root root -").ok(); + } } // Atomic write: write to a tmp file then rename so a concurrent reader diff --git a/nix/host-modules/hive-gateway/default.nix b/nix/host-modules/hive-gateway/default.nix index a9df82c5..f813692e 100644 --- a/nix/host-modules/hive-gateway/default.nix +++ b/nix/host-modules/hive-gateway/default.nix @@ -77,6 +77,9 @@ in # header so nginx can start + include the file before c0re writes # its first real content (f = create-if-absent, no overwrite). systemd.tmpfiles.rules = [ + # Must stay in step with the identical rule hive-priv generates into + # /etc/tmpfiles.d/hyperhive-agents.conf — the two used to declare + # different owners for this path. "d /run/hive-agent 0755 hive-core hive-core - -" "d /var/lib/hyperhive 0755 root root - -" "d /var/lib/hyperhive/gateway 0755 root root - -"