From 713593172210c5a6f036c95882348b6df62e0540 Mon Sep 17 00:00:00 2001 From: atlas Date: Fri, 11 Sep 2026 09:00:49 +0200 Subject: [PATCH] lifecycle: make the agent-uid lookup say which failure it hit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agent_uid_gid` returns `None` for all 13 agents on every sync, and the tmpfiles caller answers that `None` by writing `d /run/hive-agent/ 0777 root root` instead of `0751 ` — a world-writable socket dir, which docs/trust-boundary/boundary.md spells out as letting anything that can reach the path unlink an agent's socket and bind its own. Which failure fires could not be determined, because the read used `.ok()?` and collapsed every io::Error into the same `None` a missing user produces. None of the three causes the doc comment enumerated (not built yet, unparseable, missing user) fits 13 long-lived containers whose agent user demonstrably exists — from inside one: srw-rw-rw- atlas atlas /run/hive-agent/atlas/agent.sock So the live cause was outside the documented set and unidentifiable. Both `None` arms now log, with the path and the error. Splitting the pure parser out to make it testable surfaced a second, narrower bug. The old scan used `?` on the field reads, and those are only reached once the name matches — so an unusable row *for the wanted user* returned `None` from the whole function instead of skipping, hiding a usable entry below it. (Rows for other users were always skipped fine: `split(':')` always yields at least one item, so the first `?` could not fire.) It now skips unusable rows and keeps looking. Does not pre-empt #3047, which removes the lookup entirely and stays blocked on #3998; this only makes the lookup honest about failing while it exists. Closes #4197. --- hive-c0re/src/lifecycle/mod.rs | 55 ++++++++++++++++++++++++++------ hive-c0re/src/lifecycle/tests.rs | 44 +++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 9 deletions(-) diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs index 761b457a..3b6b69e1 100644 --- a/hive-c0re/src/lifecycle/mod.rs +++ b/hive-c0re/src/lifecycle/mod.rs @@ -178,9 +178,10 @@ pub fn network_isolation_from_env() -> Result } /// Read the agent user's `(uid, gid)` from the container's nixos-managed -/// `/etc/passwd`. Returns `None` when the container hasn't been built -/// yet, the passwd file is unparseable, or the agent user is missing -/// (e.g. legacy container that still runs as root). +/// `/etc/passwd`. Returns `None` when the passwd file cannot be read (not +/// built yet, or this process cannot reach the path), or when it is read +/// but holds no usable entry for the agent (missing user, e.g. a legacy +/// container that still runs as root). /// /// Used by `forge` + `matrix` after writing per-agent state files so /// the bind-mounted host file ends up readable by the agent user @@ -194,20 +195,56 @@ pub fn network_isolation_from_env() -> Result /// `PrivateUsers`), so the uid is directly usable in host-side /// `chown(2)`. /// - Best-effort: caller treats `None` as "skip the chown". +/// - ⚠️ Every `None` is logged with which of those cases produced it: the +/// tmpfiles caller answers `None` by declaring the agent's socket dir +/// `0777`, and a fallback that widens a directory has to say why it fired. #[must_use] pub fn agent_uid_gid(agent_name: &str) -> Option<(u32, u32)> { let container = container_name(agent_name); let passwd_path = format!("/var/lib/nixos-containers/{container}/etc/passwd"); - let content = std::fs::read_to_string(&passwd_path).ok()?; + let content = match std::fs::read_to_string(&passwd_path) { + Ok(content) => content, + Err(e) => { + tracing::warn!( + agent = %agent_name, + path = %passwd_path, + error = ?e, + "agent_uid_gid: cannot read the container's passwd" + ); + return None; + } + }; + let ids = parse_passwd_uid_gid(&content, agent_name); + if ids.is_none() { + tracing::warn!( + agent = %agent_name, + path = %passwd_path, + lines = content.lines().count(), + "agent_uid_gid: passwd read but no usable entry for this agent" + ); + } + ids +} + +/// Find `user`'s `(uid, gid)` in a `passwd(5)` body. +/// +/// Separate from the read so it can be tested at all — the caller's half is a +/// host path no test can stand up. +fn parse_passwd_uid_gid(content: &str, user: &str) -> Option<(u32, u32)> { for line in content.lines() { let mut parts = line.split(':'); - let user = parts.next()?; - if user != agent_name { + // Skip, never abort: an unusable row for this same user must not hide + // a usable one below it. + if parts.next() != Some(user) { continue; } - let _ = parts.next()?; // x (password placeholder) - let uid: u32 = parts.next()?.parse().ok()?; - let gid: u32 = parts.next()?.parse().ok()?; + let _password = parts.next(); + let (Some(uid), Some(gid)) = (parts.next(), parts.next()) else { + continue; + }; + let (Ok(uid), Ok(gid)) = (uid.parse(), gid.parse()) else { + continue; + }; return Some((uid, gid)); } None diff --git a/hive-c0re/src/lifecycle/tests.rs b/hive-c0re/src/lifecycle/tests.rs index c9734efc..c48720b8 100644 --- a/hive-c0re/src/lifecycle/tests.rs +++ b/hive-c0re/src/lifecycle/tests.rs @@ -282,3 +282,47 @@ fn an_unknown_state_is_never_reported_as_failed() { ); } } + +/// A malformed row for the wanted user must not end the scan. +/// +/// The old implementation used `?` on the field reads, which are only reached +/// once the name matches — so a short or unparseable row *for this agent* +/// returned `None` from the whole function and hid a usable entry below it. +/// The caller answers that `None` by declaring the socket dir `0777`. +#[test] +fn passwd_parse_keeps_scanning_past_a_malformed_row_for_the_same_user() { + // The two `atlas` rows above the real one are what matters: a malformed + // row for the SAME user is what used to end the scan, so the usable entry + // below it was never reached. Rows for other users were always skipped + // fine. Both unusable shapes are here because they leave the parser by + // different arms — missing fields, and fields that will not parse — and a + // mutation run showed the second arm untested when only the first was. + let body = "\ +root:x:0:0:System administrator:/root:/bin/sh +# a comment line, not a passwd entry +atlas:x +atlas:x:notanumber:994::/:/bin/sh +atlas:x:1000:994:atlas:/home/atlas:/bin/sh +"; + assert_eq!(parse_passwd_uid_gid(body, "atlas"), Some((1000, 994))); + // Control: the same body must NOT answer for a user it does not carry, + // or the assertion above is satisfied by any parse at all. + assert_eq!(parse_passwd_uid_gid(body, "nobody-here"), None); +} + +/// A matching name with unusable ids is not a match. Without this the parser +/// could return a partly-parsed row and the caller would chown to it. +#[test] +fn passwd_parse_rejects_unusable_ids() { + assert_eq!( + parse_passwd_uid_gid("atlas:x:notanumber:994::/:/bin/sh", "atlas"), + None + ); + assert_eq!(parse_passwd_uid_gid("atlas:x:1000", "atlas"), None); + assert_eq!(parse_passwd_uid_gid("", "atlas"), None); + // Presence control for the three absences above. + assert_eq!( + parse_passwd_uid_gid("atlas:x:1000:994::/:/bin/sh", "atlas"), + Some((1000, 994)) + ); +}