fix(exec): mission work runs as uid 65532, so it stops creating debris it cannot delete
`CreateExecOptions` never set `user`. Not a wrong value — an ABSENT one: the daemon defaults to root, and twelve callers inherited that without any of them choosing it. That single omission is the origin of four separate patches — root-owned `target/` directories inside a checkout owned by 65532, `root_copy` existing at all, and a cleanup that had to re-enter the container as root to undo its own mess. The rule is positional and lives in ONE place: an exec whose workdir is inside `missions_root()` runs as 65532; anything else (preflight probes, image checks) keeps the daemon default so unrelated call sites cannot break. Twelve callers each remembering to pass a uid is twelve chances to forget, and the one that forgets leaves debris the other eleven cannot remove. Non-root needs an environment the image does not provide. Measured in the deployed image: uid 65532's HOME (/zeroclaw-data) and /usr/local/cargo are both root-owned and unwritable, so this would otherwise break every cargo call — the benchmark runner, the judge's sandbox, the delivery test gate — far more quietly than the leak it fixes. The missions root IS bind-mounted and writable by 65532, so HOME/CARGO_HOME move there and the cargo cache is shared across missions rather than re-fetched per mission. Verified on gw-04: a clean `cargo build` as 65532 with those three variables produces output owned entirely by 65532. Root remains reachable only through `exec_as_root`, whose name says so, and which exists solely to clear debris earlier root execs left. `runtime_preflight` now probes the whole policy at boot, so an image that moves or tightens that mount fails loudly instead of failing every cargo call for a reason no error message would connect to a uid. evaluator_tools' inlined fourth copy of the purge is replaced by `root_copy::purge`. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
42108c840d
commit
dcd9514622
@@ -77,6 +77,51 @@ pub fn connect() -> Result<Docker, String> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The uid every mission artefact must belong to.
|
||||||
|
///
|
||||||
|
/// The runtime container's own processes already run as this; only `docker
|
||||||
|
/// exec` defaulted to root, because `CreateExecOptions::user` was never set.
|
||||||
|
/// That one omission is the origin of four separate patches: root-owned
|
||||||
|
/// `target/` directories appearing inside a checkout that uid 65532 then could
|
||||||
|
/// not delete, `root_copy` existing at all, and a cleanup path that had to
|
||||||
|
/// re-enter the container as root to undo what it had just done.
|
||||||
|
const MISSION_UID: &str = "65532:65532";
|
||||||
|
|
||||||
|
/// Environment a non-root exec needs, because the image gives uid 65532 no
|
||||||
|
/// writable `HOME` and no writable `CARGO_HOME`.
|
||||||
|
///
|
||||||
|
/// Measured in the deployed image: `/zeroclaw-data` (its `HOME`) and
|
||||||
|
/// `/usr/local/cargo` are both root-owned and unwritable, so switching execs to
|
||||||
|
/// 65532 without this would break every `cargo` invocation — the benchmark
|
||||||
|
/// runner, the judge's verification sandbox, and the delivery test gate — in a
|
||||||
|
/// new and much quieter way than the problem it fixes.
|
||||||
|
///
|
||||||
|
/// The missions root is bind-mounted into the runtime container at the same
|
||||||
|
/// path and IS writable by 65532, so the cargo cache lives there and is shared
|
||||||
|
/// across missions rather than re-downloaded per mission. Verified end to end:
|
||||||
|
/// a clean `cargo build` as 65532 with these three variables produces output
|
||||||
|
/// owned entirely by 65532.
|
||||||
|
fn mission_env() -> Vec<String> {
|
||||||
|
let root = crate::mission_workspace::missions_root();
|
||||||
|
vec![
|
||||||
|
format!("HOME={}", root.join("_home").display()),
|
||||||
|
format!("CARGO_HOME={}", root.join("_cargo").display()),
|
||||||
|
"TMPDIR=/tmp".to_string(),
|
||||||
|
]
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Whether a workdir is inside the tree missions own.
|
||||||
|
///
|
||||||
|
/// The rule is positional rather than per-caller on purpose. Twelve call sites
|
||||||
|
/// each remembering to pass a uid is twelve chances to forget, and the one that
|
||||||
|
/// forgets leaves debris the others cannot clean up — which is exactly the
|
||||||
|
/// history here.
|
||||||
|
fn is_mission_path(workdir: Option<&str>) -> bool {
|
||||||
|
let Some(dir) = workdir else { return false };
|
||||||
|
let root = crate::mission_workspace::missions_root();
|
||||||
|
std::path::Path::new(dir).starts_with(&root)
|
||||||
|
}
|
||||||
|
|
||||||
/// Run `argv` in `container`, optionally in `workdir`, and capture both
|
/// Run `argv` in `container`, optionally in `workdir`, and capture both
|
||||||
/// streams plus the exit status.
|
/// streams plus the exit status.
|
||||||
///
|
///
|
||||||
@@ -93,6 +138,29 @@ pub async fn exec(
|
|||||||
exec_with_env(docker, container, workdir, argv, &[], timeout).await
|
exec_with_env(docker, container, workdir, argv, &[], timeout).await
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Run `argv` as **root**, deliberately.
|
||||||
|
///
|
||||||
|
/// The one legitimate use is clearing debris that earlier root-run execs left
|
||||||
|
/// behind: uid 65532 cannot delete a root-owned `target/`, so the cleanup has
|
||||||
|
/// to out-rank it. Every other caller goes through [`exec`], which runs mission
|
||||||
|
/// work as 65532 so no new debris is created.
|
||||||
|
pub async fn exec_as_root(
|
||||||
|
docker: &Docker,
|
||||||
|
container: &str,
|
||||||
|
workdir: Option<&str>,
|
||||||
|
argv: &[String],
|
||||||
|
timeout: Duration,
|
||||||
|
) -> Result<ExecOutput, String> {
|
||||||
|
let fut = exec_inner(docker, container, workdir, argv, &[], None);
|
||||||
|
match tokio::time::timeout(timeout, fut).await {
|
||||||
|
Err(_) => Err(format!(
|
||||||
|
"timed out after {}s (the command may still be running in {container})",
|
||||||
|
timeout.as_secs()
|
||||||
|
)),
|
||||||
|
Ok(res) => res,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// As [`exec`], with extra environment for the command.
|
/// As [`exec`], with extra environment for the command.
|
||||||
pub async fn exec_with_env(
|
pub async fn exec_with_env(
|
||||||
docker: &Docker,
|
docker: &Docker,
|
||||||
@@ -102,7 +170,16 @@ pub async fn exec_with_env(
|
|||||||
env: &[String],
|
env: &[String],
|
||||||
timeout: Duration,
|
timeout: Duration,
|
||||||
) -> Result<ExecOutput, String> {
|
) -> Result<ExecOutput, String> {
|
||||||
let fut = exec_inner(docker, container, workdir, argv, env);
|
// Mission work runs as 65532 with a writable HOME/CARGO_HOME; anything
|
||||||
|
// outside the missions tree (runtime preflight probes, image checks) keeps
|
||||||
|
// the daemon's default so this cannot break unrelated call sites.
|
||||||
|
let (user, mut full_env) = if is_mission_path(workdir) {
|
||||||
|
(Some(MISSION_UID), mission_env())
|
||||||
|
} else {
|
||||||
|
(None, Vec::new())
|
||||||
|
};
|
||||||
|
full_env.extend_from_slice(env);
|
||||||
|
let fut = exec_inner(docker, container, workdir, argv, &full_env, user);
|
||||||
match tokio::time::timeout(timeout, fut).await {
|
match tokio::time::timeout(timeout, fut).await {
|
||||||
Err(_) => Err(format!(
|
Err(_) => Err(format!(
|
||||||
"timed out after {}s (the command may still be running in {container})",
|
"timed out after {}s (the command may still be running in {container})",
|
||||||
@@ -118,6 +195,7 @@ async fn exec_inner(
|
|||||||
workdir: Option<&str>,
|
workdir: Option<&str>,
|
||||||
argv: &[String],
|
argv: &[String],
|
||||||
env: &[String],
|
env: &[String],
|
||||||
|
user: Option<&str>,
|
||||||
) -> Result<ExecOutput, String> {
|
) -> Result<ExecOutput, String> {
|
||||||
let created = docker
|
let created = docker
|
||||||
.create_exec(
|
.create_exec(
|
||||||
@@ -130,6 +208,7 @@ async fn exec_inner(
|
|||||||
} else {
|
} else {
|
||||||
Some(env.to_vec())
|
Some(env.to_vec())
|
||||||
},
|
},
|
||||||
|
user: user.map(str::to_string),
|
||||||
attach_stdout: Some(true),
|
attach_stdout: Some(true),
|
||||||
attach_stderr: Some(true),
|
attach_stderr: Some(true),
|
||||||
..Default::default()
|
..Default::default()
|
||||||
@@ -209,4 +288,76 @@ mod tests {
|
|||||||
assert_eq!(out(Some(1), "a", "b").combined(), "a\nb");
|
assert_eq!(out(Some(1), "a", "b").combined(), "a\nb");
|
||||||
assert_eq!(out(Some(0), " ", "\n").combined(), "");
|
assert_eq!(out(Some(0), " ", "\n").combined(), "");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Mission work is 65532; everything else keeps the daemon's default.
|
||||||
|
///
|
||||||
|
/// The rule is positional so that no caller has to remember it. Twelve call
|
||||||
|
/// sites each passing a uid is twelve chances to forget, and the one that
|
||||||
|
/// forgets leaves debris the other eleven cannot delete — which is the
|
||||||
|
/// actual history: root-owned `target/` directories inside a checkout owned
|
||||||
|
/// by 65532, `root_copy` written to work around them, and a cleanup that had
|
||||||
|
/// to re-enter the container as root to undo its own mess.
|
||||||
|
#[test]
|
||||||
|
fn only_work_inside_the_missions_tree_drops_to_the_mission_uid() {
|
||||||
|
let root = crate::mission_workspace::missions_root();
|
||||||
|
let inside = root.join("019fe785-0f82-7780-8d58-da79fb4c31bc/repo");
|
||||||
|
assert!(is_mission_path(Some(&inside.display().to_string())));
|
||||||
|
assert!(is_mission_path(Some(&root.display().to_string())));
|
||||||
|
|
||||||
|
// Probes and image checks run with no workdir at all, and must not be
|
||||||
|
// forced to a uid the image may not have set up for them.
|
||||||
|
assert!(!is_mission_path(None));
|
||||||
|
assert!(!is_mission_path(Some("/")));
|
||||||
|
assert!(!is_mission_path(Some("/usr/local/cargo")));
|
||||||
|
// A path that merely SHARES A PREFIX is not inside the tree.
|
||||||
|
// `starts_with` on `Path` compares components, so this is already true;
|
||||||
|
// the assertion is here so a switch to string matching cannot pass.
|
||||||
|
let sibling = format!("{}-evil/repo", root.display());
|
||||||
|
assert!(!is_mission_path(Some(&sibling)));
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The non-root exec carries the three variables the image does not give it.
|
||||||
|
///
|
||||||
|
/// Measured in the deployed image: uid 65532's `HOME` (`/zeroclaw-data`)
|
||||||
|
/// and `/usr/local/cargo` are both root-owned and unwritable. Without these
|
||||||
|
/// overrides, dropping execs to 65532 would break every cargo invocation —
|
||||||
|
/// the benchmark runner, the judge's sandbox, the delivery test gate — far
|
||||||
|
/// more quietly than the leak it fixes.
|
||||||
|
#[test]
|
||||||
|
fn the_mission_env_replaces_the_paths_the_image_leaves_unwritable() {
|
||||||
|
let env = mission_env();
|
||||||
|
let root = crate::mission_workspace::missions_root();
|
||||||
|
assert!(env.iter().any(|v| v == &format!("HOME={}/_home", root.display())));
|
||||||
|
assert!(env.iter().any(|v| v == &format!("CARGO_HOME={}/_cargo", root.display())));
|
||||||
|
assert!(env.iter().any(|v| v == "TMPDIR=/tmp"));
|
||||||
|
for v in &env {
|
||||||
|
assert!(
|
||||||
|
!v.contains("/usr/local/cargo") && !v.contains("/zeroclaw-data"),
|
||||||
|
"{v} points back at a root-owned path"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// One place builds an exec, so one place decides its uid.
|
||||||
|
///
|
||||||
|
/// The original bug was not a wrong value — it was an ABSENT one:
|
||||||
|
/// `CreateExecOptions` never set `user`, so the daemon defaulted to root
|
||||||
|
/// and twelve callers inherited that without any of them choosing it. A
|
||||||
|
/// second construction site is how that comes back, so the guard is on the
|
||||||
|
/// number of sites rather than on any particular uid.
|
||||||
|
#[test]
|
||||||
|
fn exactly_one_place_builds_an_exec() {
|
||||||
|
let src = include_str!("container_exec.rs");
|
||||||
|
// Split so this needle does not match itself in this very file.
|
||||||
|
let needle = concat!("CreateExec", "Options {");
|
||||||
|
let sites = src.matches(needle).count();
|
||||||
|
assert_eq!(
|
||||||
|
sites, 1,
|
||||||
|
"exec options must be built in one place; found {sites}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
src.contains(concat!("user: ", "user.map(str::to_string)")),
|
||||||
|
"that one place must set `user` — leaving it unset is the bug"
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -416,28 +416,9 @@ impl Sandbox {
|
|||||||
let Some(root) = self.workdir.parent() else {
|
let Some(root) = self.workdir.parent() else {
|
||||||
return;
|
return;
|
||||||
};
|
};
|
||||||
let Ok(docker) = crate::container_exec::connect() else {
|
// The same purge as the other three copy sites, not a fourth copy of
|
||||||
return;
|
// it: an inlined duplicate is how the reap paths drifted apart before.
|
||||||
};
|
crate::root_copy::purge(&self.container, root).await;
|
||||||
let argv = vec![
|
|
||||||
"rm".to_string(),
|
|
||||||
"-rf".to_string(),
|
|
||||||
root.display().to_string(),
|
|
||||||
];
|
|
||||||
if let Err(e) = crate::container_exec::exec(
|
|
||||||
&docker,
|
|
||||||
&self.container,
|
|
||||||
Some("/"),
|
|
||||||
&argv,
|
|
||||||
COMMAND_TIMEOUT,
|
|
||||||
)
|
|
||||||
.await
|
|
||||||
{
|
|
||||||
eprintln!(
|
|
||||||
"evaluator_tools: could not remove the verification copy at {} ({e})",
|
|
||||||
root.display()
|
|
||||||
);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -48,7 +48,10 @@ pub async fn purge(container: &str, root: &Path) {
|
|||||||
"-rf".to_string(),
|
"-rf".to_string(),
|
||||||
root.display().to_string(),
|
root.display().to_string(),
|
||||||
];
|
];
|
||||||
if let Err(e) = crate::container_exec::exec(
|
// Explicitly root: this exists to delete files an EARLIER root-run exec
|
||||||
|
// created, which uid 65532 cannot touch. Everything else now runs as 65532
|
||||||
|
// (see `container_exec`), so this is cleaning up history, not policy.
|
||||||
|
if let Err(e) = crate::container_exec::exec_as_root(
|
||||||
&docker,
|
&docker,
|
||||||
container,
|
container,
|
||||||
Some("/"),
|
Some("/"),
|
||||||
|
|||||||
@@ -23,6 +23,7 @@
|
|||||||
//! from serving — it should stop us believing a scan that scanned nothing.
|
//! from serving — it should stop us believing a scan that scanned nothing.
|
||||||
|
|
||||||
use crate::container_exec;
|
use crate::container_exec;
|
||||||
|
use bollard::Docker;
|
||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
|
|
||||||
const PROBE_TIMEOUT: Duration = Duration::from_secs(20);
|
const PROBE_TIMEOUT: Duration = Duration::from_secs(20);
|
||||||
@@ -113,9 +114,67 @@ pub async fn probe(container: &str) -> Result<Vec<ToolStatus>, String> {
|
|||||||
};
|
};
|
||||||
out.push(status);
|
out.push(status);
|
||||||
}
|
}
|
||||||
|
out.push(probe_mission_uid_can_write(&docker, container).await);
|
||||||
Ok(out)
|
Ok(out)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Can uid 65532 actually work in the missions tree?
|
||||||
|
///
|
||||||
|
/// `container_exec` now runs every mission exec as 65532 rather than root, so
|
||||||
|
/// that no phase leaves behind files the cleanup (which runs as 65532) cannot
|
||||||
|
/// delete. That only holds while the image gives 65532 a writable `HOME` and
|
||||||
|
/// `CARGO_HOME` — and in the deployed image its default `HOME`
|
||||||
|
/// (`/zeroclaw-data`) and `/usr/local/cargo` are BOTH root-owned, which is why
|
||||||
|
/// `container_exec::mission_env` redirects them into the missions root.
|
||||||
|
///
|
||||||
|
/// If a future image moves that mount or tightens its permissions, every cargo
|
||||||
|
/// invocation starts failing for a reason no error message would connect to a
|
||||||
|
/// uid. So it is probed at boot, alongside the tools, and reported the same way.
|
||||||
|
async fn probe_mission_uid_can_write(docker: &Docker, container: &str) -> ToolStatus {
|
||||||
|
let root = crate::mission_workspace::missions_root();
|
||||||
|
let probe = root.join("_probe-uid");
|
||||||
|
// Through `exec`, not `exec_as_root`: the point is to exercise the exact
|
||||||
|
// policy real mission work gets, including the env it is given.
|
||||||
|
let argv: Vec<String> = [
|
||||||
|
"sh",
|
||||||
|
"-c",
|
||||||
|
&format!(
|
||||||
|
"set -e; mkdir -p \"$HOME\" \"$CARGO_HOME\" {p}; : > {p}/w; rm -rf {p}; echo \"uid=$(id -u) HOME=$HOME CARGO_HOME=$CARGO_HOME\"",
|
||||||
|
p = probe.display()
|
||||||
|
),
|
||||||
|
]
|
||||||
|
.iter()
|
||||||
|
.map(|s| s.to_string())
|
||||||
|
.collect();
|
||||||
|
|
||||||
|
let detail = match container_exec::exec(
|
||||||
|
docker,
|
||||||
|
container,
|
||||||
|
Some(&root.display().to_string()),
|
||||||
|
&argv,
|
||||||
|
PROBE_TIMEOUT,
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
Ok(r) if r.success() => {
|
||||||
|
return ToolStatus {
|
||||||
|
program: "mission-uid".to_string(),
|
||||||
|
present: true,
|
||||||
|
detail: r.combined().trim().chars().take(120).collect(),
|
||||||
|
needed_for: "every mission exec, so no phase leaves root-owned files",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
Ok(r) => r.combined().trim().chars().take(160).collect(),
|
||||||
|
Err(e) => e.chars().take(160).collect(),
|
||||||
|
};
|
||||||
|
ToolStatus {
|
||||||
|
program: "mission-uid".to_string(),
|
||||||
|
present: false,
|
||||||
|
detail,
|
||||||
|
needed_for: "every mission exec, so no phase leaves root-owned files",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Probe at startup and write the result to stderr.
|
/// Probe at startup and write the result to stderr.
|
||||||
///
|
///
|
||||||
/// Spawned rather than awaited so a slow or absent Docker socket cannot delay
|
/// Spawned rather than awaited so a slow or absent Docker socket cannot delay
|
||||||
|
|||||||
Reference in New Issue
Block a user