lifecycle: make the agent-uid lookup say which failure it hit
`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/<name>
0777 root root` instead of `0751 <uid> <gid>` — 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.
This commit is contained in:
parent
aa2b86c48d
commit
7135931722
2 changed files with 90 additions and 9 deletions
|
|
@ -178,9 +178,10 @@ pub fn network_isolation_from_env() -> Result<hive_priv_sock::NetworkIsolation>
|
|||
}
|
||||
|
||||
/// 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<hive_priv_sock::NetworkIsolation>
|
|||
/// `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
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue