diff --git a/sandbox/src/spawn/container.rs b/sandbox/src/spawn/container.rs index 416c4ed6..1854dfee 100644 --- a/sandbox/src/spawn/container.rs +++ b/sandbox/src/spawn/container.rs @@ -81,19 +81,38 @@ impl Sandbox { })) } + /// The sandbox's space environment as the container spells each + /// directory, through the workspace mount. + fn container_space_environment( + &self, + runtime: &ContainerShellRuntime, + ) -> Result, SpawnError> { + self.inner + .space_environment + .iter() + .map(|(name, path)| { + let path = Self::container_path(runtime, path)?; + Ok((name.clone(), path.as_str().to_owned())) + }) + .collect() + } + /// `script` as the container's managed shell runs it: the source's - /// environment exported, then the spec's, ahead of it, the programs' - /// directory first on its `PATH`. + /// environment exported, then the space's, then the spec's, ahead of + /// it, the programs' directory first on its `PATH`. fn exec_script( &self, launch: &ContainerLaunch, script: String, + space_env: Vec<(String, String)>, env: Vec<(String, String)>, info: &LaunchInfo, ) -> Result { let failed = |error: &dyn std::fmt::Display| SpawnError::Setup(error.to_string()); let spec_env: Arc = Arc::new(env); + let space_env: Arc = Arc::new(space_env); let script = inject_env_exports(script, Some(&spec_env), info).map_err(|e| failed(&e))?; + let script = inject_env_exports(script, Some(&space_env), info).map_err(|e| failed(&e))?; let script = inject_env_exports(script, self.inner.env_source.as_ref(), info) .map_err(|e| failed(&e))?; let script = launch.wrap_script(&script).map_err(|e| failed(&e))?; @@ -241,7 +260,8 @@ impl Sandbox { let confinement = self.container_confinement(runtime, container_grants)?; // The exec channel accepts only a script, so the environment travels - // as exports ahead of it; the source's come first, so the spec's win. + // as exports ahead of it; the source's come first, then the space's, + // so the spec's win. let script = match command { SpawnCommand::Script(script) => script, SpawnCommand::Program { program, args } => { @@ -249,7 +269,8 @@ impl Sandbox { } }; let script = script_reading(stdin, script)?; - let script = self.exec_script(launch, script, env, info)?; + let space_env = self.container_space_environment(runtime)?; + let script = self.exec_script(launch, script, space_env, env, info)?; // A confined process runs in a sandbox rooted at the sandbox's own // directory; the directory it was asked to start in is entered inside. let script = match (&confinement, cwd.is_some()) { diff --git a/sandbox/src/spawn/host.rs b/sandbox/src/spawn/host.rs index 185aed68..a66d0469 100644 --- a/sandbox/src/spawn/host.rs +++ b/sandbox/src/spawn/host.rs @@ -119,19 +119,28 @@ impl Sandbox { .unwrap_or_else(|_| self.inner.working_dir.clone()) } - /// The environment of a host spawn: the sandbox's own, its source's, then - /// the spec's, each overriding the one before. + /// The environment of a host spawn: the sandbox's own, its source's, its + /// space's, then the spec's, each overriding the one before. fn host_environment( &self, spec_env: Vec<(String, String)>, info: &LaunchInfo, - ) -> Vec<(String, String)> { + ) -> Result, SpawnError> { let mut env = self.inner.environment.clone(); if let Some(source) = &self.inner.env_source { env.extend(source.env(info)); } + for (name, path) in &self.inner.space_environment { + let value = path.to_str().ok_or_else(|| { + SpawnError::Setup(format!( + "{} is not UTF-8, so the variable {name} cannot name it", + path.display() + )) + })?; + env.push((name.clone(), value.to_owned())); + } env.extend(spec_env); - env + Ok(env) } /// The grants a confined spawn carries: the embedder's fixed ones, every @@ -267,7 +276,7 @@ impl Sandbox { }) .collect::, _>>()?; let marker = JobMarker::new(); - let mut env = self.host_environment(env, &info); + let mut env = self.host_environment(env, &info)?; env.push(marker.entry()); let launch = Launch { program, diff --git a/sandbox/src/spawn/mod.rs b/sandbox/src/spawn/mod.rs index a3866626..16eeaea8 100644 --- a/sandbox/src/spawn/mod.rs +++ b/sandbox/src/spawn/mod.rs @@ -326,6 +326,9 @@ struct SandboxInner { fixed_grants: Vec, environment: Vec<(String, String)>, env_source: Option>, + /// Variables naming directories inside a subagent's own space, by their + /// host paths: the layer between the source and a spec's own variables. + space_environment: Vec<(String, PathBuf)>, write_denylist: Option, network: Option, approver: Option, @@ -377,6 +380,7 @@ impl Sandbox { fixed_grants: Vec::new(), environment: Vec::new(), env_source: None, + space_environment: Vec::new(), write_denylist: None, network: None, approver: None, @@ -427,6 +431,14 @@ impl Sandbox { self.edit(|inner| inner.env_source = Some(Arc::new(source))) } + /// The variables of a subagent's space: each names a directory inside + /// it, by its host path. They override the environment and its source + /// and yield to a spec's own variables; a container sees each directory + /// through its workspace mount, and an SSH server none. + pub(crate) fn with_space_environment(self, environment: Vec<(String, PathBuf)>) -> Self { + self.edit(|inner| inner.space_environment = environment) + } + /// The paths a spawn on the host must not write, however they are /// granted. Not supported on Windows, whose backend refuses a denylist /// rather than run weaker, so it is not applied there. @@ -474,13 +486,16 @@ impl Sandbox { /// grants [derived](Grants::derived) from this sandbox's with `own`, and /// no agent label until [`with_agent`](Self::with_agent) gives one. It /// cannot widen this sandbox: its grants never reach it, and a container - /// confines its processes to its own writable grants. + /// confines its processes to its own writable grants. The variables of + /// this sandbox's space are not the child's, which has a space of its + /// own. #[must_use] pub fn derived(&self, own: GrantSnapshot) -> Self { let grants = self.inner.grants.derived(own); self.clone().edit(|inner| { inner.grants = grants; inner.agent = None; + inner.space_environment = Vec::new(); }) } diff --git a/sandbox/src/spawn/tests.rs b/sandbox/src/spawn/tests.rs index fc66a432..5235284f 100644 --- a/sandbox/src/spawn/tests.rs +++ b/sandbox/src/spawn/tests.rs @@ -269,6 +269,49 @@ async fn environment_layers_resolve_spec_over_source_over_sandbox() { assert_eq!(out, "sandbox source spec\n"); } +/// A space's variables lead the source's and the sandbox's and yield to the +/// spec's, and a sandbox derived from the space's carries none of them. +#[tokio::test] +async fn the_space_environment_leads_the_source_and_yields_to_the_spec() { + let (_work, work) = canonical_tempdir(); + let (tmp, cache) = (work.join("tmp"), work.join("cache")); + let pair = |name: &str, value: &str| (name.to_string(), value.to_string()); + let parent = host_sandbox(&work) + .await + .with_environment(vec![ + pair("SPACE_TMP", "sandbox"), + pair("SPACE_CACHE", "sandbox"), + pair("SPACE_HOME", "sandbox"), + ]) + .with_env_source(vec![ + pair("SPACE_CACHE", "source"), + pair("SPACE_HOME", "source"), + ]); + let child = parent + .derived(GrantSnapshot::default()) + .with_space_environment(vec![ + ("SPACE_TMP".to_string(), tmp.clone()), + ("SPACE_CACHE".to_string(), cache.clone()), + ("SPACE_HOME".to_string(), work.join("home")), + ]); + let spec = || { + SpawnSpec::new("/bin/sh") + .args(["-c", "echo $SPACE_TMP $SPACE_CACHE $SPACE_HOME"]) + .env([pair("SPACE_HOME", "spec")]) + }; + let (exit, out, _) = run_spec(&child, spec()).await; + assert!(exit.success(), "{exit:?}"); + assert_eq!(out, format!("{} {} spec\n", tmp.display(), cache.display())); + + let nested = child.derived(GrantSnapshot::default()); + let (exit, out, _) = run_spec(&nested, spec()).await; + assert!(exit.success(), "{exit:?}"); + assert_eq!( + out, "sandbox source spec\n", + "a derived sandbox has no space variables of its parent's" + ); +} + /// A container exec that records its script, prints once and then waits for /// the kill switch. struct HangingExec { @@ -600,6 +643,85 @@ async fn a_container_setup_runs_outside_the_sandbox_that_a_confined_child_gets() ); } +/// A container child exports its space's variables with each directory as +/// the container spells it, after the source's exports and before the +/// spec's, and a sandbox derived from it exports none of them. +#[tokio::test] +async fn a_container_child_exports_its_space_environment_through_the_mount() { + let (_work, work) = canonical_tempdir(); + let space = work.join("space"); + let inner = space.join("inner"); + std::fs::create_dir_all(&inner).unwrap(); + let started = Arc::new(Mutex::new(Vec::new())); + let mount = crate::MountRoot::new( + work.clone(), + crate::ContainerPath::new("/workspace").unwrap(), + ); + let pair = |name: &str, value: &str| (name.to_string(), value.to_string()); + let parent = reporting_sandbox( + ReportingExec { + started: started.clone(), + killed: Vec::new(), + }, + &work, + Some(mount), + ) + .with_env_source(vec![pair("TMPDIR", "source-tmp")]); + let child = parent + .derived(GrantSnapshot { + writable: vec![space.clone()], + ..GrantSnapshot::default() + }) + .with_working_dir(&space) + .with_space_environment(vec![ + ("TMPDIR".to_string(), space.join("tmp")), + ("XDG_CACHE_HOME".to_string(), space.join("cache")), + ]); + let spec = || SpawnSpec::script("make").env([pair("TMPDIR", "spec-tmp")]); + let process = child.spawn(spec()).await.expect("the exec starts"); + assert!(process.wait().await.success()); + let nested = child + .derived(GrantSnapshot { + writable: vec![inner.clone()], + ..GrantSnapshot::default() + }) + .with_working_dir(&inner); + let process = nested.spawn(spec()).await.expect("the exec starts"); + assert!(process.wait().await.success()); + + let scripts: Vec = started + .lock() + .unwrap() + .iter() + .map(|(script, _)| script.clone()) + .collect(); + let script = &scripts[0]; + let at = |needle: &str| { + script + .find(needle) + .unwrap_or_else(|| panic!("{needle} is exported: {script}")) + }; + assert!( + at("source-tmp") < at("/workspace/space/tmp") + && at("/workspace/space/tmp") < at("spec-tmp"), + "the space's exports follow the source's and precede the spec's: {script}" + ); + assert!( + script.contains("export XDG_CACHE_HOME=") && script.contains("/workspace/space/cache"), + "{script}" + ); + assert!( + !script.contains(&space.join("cache").display().to_string()), + "the container sees no host spelling: {script}" + ); + let nested_script = &scripts[1]; + assert!( + !nested_script.contains("XDG_CACHE_HOME") + && !nested_script.contains("/workspace/space/tmp"), + "a derived sandbox exports no space variables of its parent's: {nested_script}" + ); +} + /// A command a spawn's router offers. struct Probe; diff --git a/sandbox/src/subagent.rs b/sandbox/src/subagent.rs index b670bf82..198599ee 100644 --- a/sandbox/src/subagent.rs +++ b/sandbox/src/subagent.rs @@ -215,15 +215,15 @@ pub enum SubagentGrantError { /// The embedder's error. source: SpacePreparationError, }, - /// The embedder named a working copy outside the space it prepared. The - /// space was removed. + /// The embedder named a working copy or an environment directory outside + /// the space it prepared. The space was removed. #[error( - "the working copy {} prepared for a subagent is outside its space {}", + "the path {} prepared for a subagent is outside its space {}", .path.display(), .space.display() )] PreparedOutsideSpace { - /// The working copy. + /// The working copy or environment directory, by its canonical path. path: PathBuf, /// The space. space: PathBuf, diff --git a/sandbox/src/subagent/space.rs b/sandbox/src/subagent/space.rs index 8dd7d100..97a53113 100644 --- a/sandbox/src/subagent/space.rs +++ b/sandbox/src/subagent/space.rs @@ -4,8 +4,9 @@ //! launcher's working directory that it alone can write. An embedder that //! gives its terminal [`SubagentSpaces`] fills each new space before the //! subagent's terminal exists, typically with a clone of the repository the -//! launcher works in, and names the working copies it made there. The run -//! fails with the embedder's error when that fails. +//! launcher works in, and names the working copies it made there and the +//! directories its commands' environment points at. The run fails with the +//! embedder's error when that fails. //! //! The space belongs to the launcher: it lies inside the launcher's working //! directory, stays when the run ends so the launcher can review and take @@ -18,6 +19,12 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use futures::future::BoxFuture; +use futures::{StreamExt as _, TryStreamExt as _}; + +/// How many prepared paths are resolved at once. Each resolution is one +/// filesystem call on the blocking pool, and an embedder names a handful, so +/// the bound matches the import's ref checks rather than any measurement. +const RESOLUTIONS_IN_FLIGHT: usize = 8; /// Prepares the space each subagent run works in. /// @@ -29,8 +36,8 @@ pub trait SubagentSpaces: Send + Sync + 'static { /// Why a space could not be prepared. type Error: std::error::Error + Send + Sync + 'static; - /// Fills `run.space` for the run and names the working copies made in - /// it. Each must lie inside the space. + /// Fills `run.space` for the run and names the working copies and + /// environment directories made in it. Each must lie inside the space. fn prepare( &self, run: RunSpace<'_>, @@ -57,6 +64,7 @@ pub struct RunSpace<'a> { #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct PreparedSpace { working_copies: Vec, + env_dirs: Vec<(String, PathBuf)>, } impl PreparedSpace { @@ -73,6 +81,20 @@ impl PreparedSpace { self } + /// Records the environment variable `name` as `path`, a directory inside + /// the space, for every command the run executes: the temporary, home or + /// cache directory a launcher's environment would otherwise point at a + /// directory the run cannot write. It overrides the launcher's sandbox + /// environment and [`EnvSource`](crate::EnvSource), never a command's own + /// variables, and reaches no subagent the run launches, whose space is + /// prepared on its own. A container sees `path` as it spells it through + /// the workspace mount. + #[must_use] + pub fn with_env_dir(mut self, name: impl Into, path: impl Into) -> Self { + self.env_dirs.push((name.into(), path.into())); + self + } + /// The working copies the run works in, by their canonical paths once /// the terminal has accepted the space. #[must_use] @@ -80,22 +102,44 @@ impl PreparedSpace { &self.working_copies } - /// Resolves every working copy and confirms it lies inside `space`, which - /// is canonical. + /// The environment directories, by their canonical paths once the + /// terminal has accepted the space. + pub(crate) fn env_dirs(&self) -> &[(String, PathBuf)] { + &self.env_dirs + } + + /// Resolves every working copy and environment directory and confirms it + /// lies inside `space`, which is canonical. pub(crate) async fn confine(self, space: &Path) -> Result { - let mut working_copies = Vec::with_capacity(self.working_copies.len()); - for path in self.working_copies { - let canonical = super::canonical(&path).await?; - if !canonical.starts_with(space) { - return Err(super::SubagentGrantError::PreparedOutsideSpace { - path: canonical, - space: space.to_path_buf(), - }); - } - working_copies.push(canonical); - } - Ok(Self { working_copies }) + let working_copies = futures::stream::iter(self.working_copies) + .map(|path| inside(path, space)) + .buffered(RESOLUTIONS_IN_FLIGHT) + .try_collect::>(); + let env_dirs = futures::stream::iter(self.env_dirs) + .map(|(name, path)| async move { + Ok::<_, super::SubagentGrantError>((name, inside(path, space).await?)) + }) + .buffered(RESOLUTIONS_IN_FLIGHT) + .try_collect::>(); + let (working_copies, env_dirs) = + futures::future::try_join(working_copies, env_dirs).await?; + Ok(Self { + working_copies, + env_dirs, + }) + } +} + +/// `path` by its canonical path, refused unless it lies inside `space`. +async fn inside(path: PathBuf, space: &Path) -> Result { + let canonical = super::canonical(&path).await?; + if !canonical.starts_with(space) { + return Err(super::SubagentGrantError::PreparedOutsideSpace { + path: canonical, + space: space.to_path_buf(), + }); } + Ok(canonical) } /// An embedder's failure to prepare a space, whatever its own error type. diff --git a/sandbox/src/terminal.rs b/sandbox/src/terminal.rs index b890d724..34dbeb5e 100644 --- a/sandbox/src/terminal.rs +++ b/sandbox/src/terminal.rs @@ -1465,6 +1465,12 @@ where self.executor .spawn(async move { job_registry_service.serve().await }) .detach(); + // The derived sandbox carries none of the launcher's space variables; + // the run's are those its own space was prepared with. + let space_environment = prepared + .as_ref() + .map(|prepared| prepared.env_dirs().to_vec()) + .unwrap_or_default(); let sandbox = self .sandbox .derived(GrantSnapshot { @@ -1473,6 +1479,7 @@ where ..GrantSnapshot::default() }) .with_working_dir(space.clone()) + .with_space_environment(space_environment) .with_scanner(job_registry.scanner()); let jobs = Arc::new(KillJobsOnDrop::new( job_registry.clone(), @@ -9421,6 +9428,8 @@ mod tests { Fail, /// Names the launcher's working directory as the working copy. Outside, + /// Names the launcher's working directory as `TMPDIR`. + EnvOutside, } #[cfg(unix)] @@ -9470,7 +9479,39 @@ mod tests { Err(NoClone) } Filling::Outside => Ok(PreparedSpace::new().with_working_copy(run.launcher)), + Filling::EnvOutside => { + Ok(PreparedSpace::new().with_env_dir("TMPDIR", run.launcher)) + } + } + } + } + + /// Gives every space a temporary directory as `TMPDIR`, and the first + /// space it prepares also a cache directory as `AITHER_TEST_CACHE`. + #[cfg(unix)] + #[derive(Default)] + struct EnvSpaces { + prepared: std::sync::atomic::AtomicUsize, + } + + #[cfg(unix)] + impl SubagentSpaces for EnvSpaces { + type Error = std::io::Error; + + async fn prepare(&self, run: RunSpace<'_>) -> Result { + let tmp = run.space.join("tmp"); + async_fs::create_dir(&tmp).await?; + let prepared = PreparedSpace::new().with_env_dir("TMPDIR", tmp); + if self + .prepared + .fetch_add(1, std::sync::atomic::Ordering::Relaxed) + > 0 + { + return Ok(prepared); } + let cache = run.space.join("cache"); + async_fs::create_dir(&cache).await?; + Ok(prepared.with_env_dir("AITHER_TEST_CACHE", cache)) } } @@ -9637,6 +9678,137 @@ mod tests { assert!(!asked.exists(), "the refused space is removed"); } + /// An environment directory named outside the space fails the launch, + /// and the space is removed. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn an_environment_directory_outside_the_space_fails_the_launch() { + let dir = tempfile::tempdir().expect("temp dir should be created"); + let (parent, seen) = spaced_terminal(dir.path(), Filling::EnvOutside).await; + let error = launch(&parent, SubagentGrant::new()) + .await + .expect_err("the launch fails"); + let (asked, launcher, _) = seen.try_recv().unwrap(); + let SubagentGrantError::PreparedOutsideSpace { path, space } = &error else { + panic!("the launch fails on the environment directory: {error}"); + }; + assert_eq!((path, space), (&launcher, &asked)); + assert!(!asked.exists(), "the refused space is removed"); + } + + /// A subagent's commands see the directories its space was prepared with + /// over the launcher's environment, and write them; a subagent it + /// launches sees its own space's alone. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn a_subagent_sees_the_environment_directories_of_its_own_space() { + let dir = tempfile::tempdir().expect("temp dir should be created"); + let parent = authority_terminal(dir.path()) + .await + .with_subagent_spaces(EnvSpaces::default()); + let child = launch(&parent, SubagentGrant::new()) + .await + .expect("the space is prepared"); + let space = child.working_dir().to_path_buf(); + let (tmp, cache) = (space.join("tmp"), space.join("cache")); + assert_eq!( + child.prepared_space().unwrap().env_dirs().to_vec(), + vec![ + ("TMPDIR".to_owned(), tmp.clone()), + ("AITHER_TEST_CACHE".to_owned(), cache.clone()), + ] + ); + assert_eq!( + stdout_of(&child, "echo \"$TMPDIR|$AITHER_TEST_CACHE\"").await, + format!("{}|{}", tmp.display(), cache.display()) + ); + assert_eq!( + exit_code_of( + &child, + "touch \"$TMPDIR/written\" \"$AITHER_TEST_CACHE/written\"" + ) + .await, + 0, + "the subagent writes the directories its environment names" + ); + assert!(tmp.join("written").exists() && cache.join("written").exists()); + assert_ne!( + stdout_of(&parent, "echo \"$TMPDIR\"").await, + tmp.display().to_string(), + "the launcher keeps its own environment" + ); + + let nested = launch(&child, SubagentGrant::new()) + .await + .expect("a nested space is prepared"); + assert_eq!( + stdout_of(&nested, "echo \"$TMPDIR|${AITHER_TEST_CACHE-unset}\"").await, + format!("{}|unset", nested.working_dir().join("tmp").display()), + "a nested subagent carries its own space's variables alone" + ); + } + + /// In a container, a subagent's commands export the directories its + /// space was prepared with as the container spells them, and a subagent + /// it launches exports its own space's alone. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn a_container_subagent_exports_its_spaces_environment_through_the_mount() { + let dir = tempfile::tempdir().expect("temp dir should be created"); + let (scripts, parent) = recording_container_terminal(dir.path(), Some(dir.path())).await; + let parent = parent.with_subagent_spaces(EnvSpaces::default()); + let child = launch(&parent, SubagentGrant::new()) + .await + .expect("a space inside the workspace can be prepared"); + let nested = launch(&child, SubagentGrant::new()) + .await + .expect("a nested space is prepared"); + assert_eq!(exit_code_of(&child, "echo child-env-probe").await, 0); + assert_eq!(exit_code_of(&nested, "echo nested-env-probe").await, 0); + + let root = dunce::canonicalize(dir.path()).unwrap(); + let in_container = |path: &Path| { + format!( + "/workspace/{}", + path.strip_prefix(&root).unwrap().to_str().unwrap() + ) + }; + let scripts = scripts.lock().unwrap().clone(); + let script_of = |probe: &str| { + scripts + .iter() + .find(|script| script.contains(probe)) + .unwrap_or_else(|| panic!("the {probe} command ran")) + .clone() + }; + let child_script = script_of("child-env-probe"); + let space = child.working_dir(); + for (name, path) in [ + ("TMPDIR", space.join("tmp")), + ("AITHER_TEST_CACHE", space.join("cache")), + ] { + assert!( + child_script.contains(&format!("export {name}=")) + && child_script.contains(&in_container(&path)), + "{name} is exported as the container spells it: {child_script}" + ); + assert!( + !child_script.contains(&path.display().to_string()), + "the container sees no host spelling: {child_script}" + ); + } + let nested_script = script_of("nested-env-probe"); + assert!( + nested_script.contains(&in_container(&nested.working_dir().join("tmp"))), + "{nested_script}" + ); + assert!( + !nested_script.contains("AITHER_TEST_CACHE") + && !nested_script.contains(&in_container(&space.join("tmp"))), + "a nested subagent exports its own space's variables alone: {nested_script}" + ); + } + #[cfg(unix)] #[tokio::test(flavor = "multi_thread")] async fn subagent_cannot_write_the_session_root_but_writes_its_space_and_granted_worktrees() {