Skip to content

Commit 14bbbee

Browse files
elezarelezar
committed
fix(supervisor): gate sbin path repair on CDI policy
Signed-off-by: Evan Lezar <elezar@nvidia.com>
1 parent 5519726 commit 14bbbee

4 files changed

Lines changed: 284 additions & 39 deletions

File tree

‎crates/openshell-supervisor-process/src/child_env.rs‎

Lines changed: 135 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@
44
use std::io::{Read, Write};
55
use std::path::Path;
66

7+
use openshell_core::policy::SandboxPolicy;
8+
79
const LOCAL_NO_PROXY: &str = "127.0.0.1,localhost,::1";
8-
pub const DEFAULT_CHILD_PATH: &str = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
10+
pub const DEFAULT_CHILD_PATH: &str = "/usr/local/bin:/usr/bin:/bin";
911
const STANDARD_SBIN_PATHS: &[&str] = &["/usr/local/sbin", "/usr/sbin", "/sbin"];
1012
const ENSURE_STANDARD_SBIN_PATHS_SCRIPT: &str = "for dir in /usr/local/sbin /usr/sbin /sbin; do case \":${PATH:-}:\" in *:\"$dir\":*) ;; *) PATH=\"${PATH:+$PATH:}$dir\" ;; esac; done; export PATH";
1113
const STARTUP_SNIPPET_MARKER: &str = "# OpenShell standard sbin PATH";
@@ -17,14 +19,45 @@ enum StartupFile {
1719
Unsafe,
1820
}
1921

20-
pub fn child_path_from_env() -> String {
21-
std::env::var("PATH")
22+
pub fn standard_sbin_path_repair_enabled(policy: &SandboxPolicy) -> bool {
23+
let cdi_context = std::env::var(openshell_core::sandbox_env::CDI_CONTEXT);
24+
standard_sbin_path_repair_enabled_for_context(policy, cdi_context.as_deref().ok())
25+
}
26+
27+
fn standard_sbin_path_repair_enabled_for_context(
28+
policy: &SandboxPolicy,
29+
cdi_context: Option<&str>,
30+
) -> bool {
31+
cdi_context
32+
.map(str::trim)
33+
.is_some_and(|path| path == openshell_core::cdi::CDI_CONTEXT_PATH)
34+
&& policy_has_standard_sbin_path(policy)
35+
}
36+
37+
fn policy_has_standard_sbin_path(policy: &SandboxPolicy) -> bool {
38+
policy
39+
.filesystem
40+
.read_only
41+
.iter()
42+
.chain(policy.filesystem.read_write.iter())
43+
.any(|path| STANDARD_SBIN_PATHS.iter().any(|dir| path.starts_with(dir)))
44+
}
45+
46+
pub fn child_path_from_env(repair_standard_sbin: bool) -> String {
47+
let path = std::env::var("PATH")
2248
.ok()
2349
.filter(|path| !path.trim().is_empty())
24-
.map_or_else(
25-
|| DEFAULT_CHILD_PATH.to_string(),
26-
|path| path_with_standard_sbin_paths(&path),
27-
)
50+
.unwrap_or_else(|| DEFAULT_CHILD_PATH.to_string());
51+
52+
maybe_path_with_standard_sbin_paths(&path, repair_standard_sbin)
53+
}
54+
55+
pub fn maybe_path_with_standard_sbin_paths(path: &str, repair_standard_sbin: bool) -> String {
56+
if repair_standard_sbin {
57+
path_with_standard_sbin_paths(path)
58+
} else {
59+
path.to_string()
60+
}
2861
}
2962

3063
pub fn path_with_standard_sbin_paths(path: &str) -> String {
@@ -50,6 +83,17 @@ pub fn shell_command_with_standard_sbin_paths(command: &str) -> String {
5083
format!("{ENSURE_STANDARD_SBIN_PATHS_SCRIPT}\n{command}")
5184
}
5285

86+
pub fn maybe_shell_command_with_standard_sbin_paths(
87+
command: &str,
88+
repair_standard_sbin: bool,
89+
) -> String {
90+
if repair_standard_sbin {
91+
shell_command_with_standard_sbin_paths(command)
92+
} else {
93+
command.to_string()
94+
}
95+
}
96+
5397
pub fn install_standard_sbin_path_startup_files(home: Option<&str>) {
5498
let profile_path = Path::new(PROFILE_D_SNIPPET_PATH);
5599
if let Err(error) = write_profile_snippet(profile_path) {
@@ -271,7 +315,10 @@ mod tests {
271315

272316
#[test]
273317
fn path_with_standard_sbin_paths_uses_default_for_empty_path() {
274-
assert_eq!(path_with_standard_sbin_paths(""), DEFAULT_CHILD_PATH);
318+
assert_eq!(
319+
path_with_standard_sbin_paths(""),
320+
"/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"
321+
);
275322
}
276323

277324
#[test]
@@ -284,12 +331,90 @@ mod tests {
284331

285332
#[test]
286333
fn path_with_standard_sbin_paths_is_idempotent() {
334+
let path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
335+
assert_eq!(path_with_standard_sbin_paths(path), path);
336+
}
337+
338+
#[test]
339+
fn maybe_path_with_standard_sbin_paths_respects_gate() {
340+
let path = "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin";
341+
342+
assert_eq!(maybe_path_with_standard_sbin_paths(path, false), path);
287343
assert_eq!(
288-
path_with_standard_sbin_paths(DEFAULT_CHILD_PATH),
289-
DEFAULT_CHILD_PATH
344+
maybe_path_with_standard_sbin_paths(path, true),
345+
"/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin"
290346
);
291347
}
292348

349+
#[test]
350+
fn maybe_shell_command_with_standard_sbin_paths_respects_gate() {
351+
assert_eq!(
352+
maybe_shell_command_with_standard_sbin_paths("nvidia-smi -L", false),
353+
"nvidia-smi -L"
354+
);
355+
assert!(
356+
maybe_shell_command_with_standard_sbin_paths("nvidia-smi -L", true)
357+
.contains("/usr/sbin")
358+
);
359+
}
360+
361+
#[test]
362+
fn standard_sbin_repair_requires_expected_cdi_context_and_policy_path() {
363+
let policy = policy_with_read_only(["/usr/sbin/nvidia-smi"]);
364+
365+
assert!(standard_sbin_path_repair_enabled_for_context(
366+
&policy,
367+
Some(openshell_core::cdi::CDI_CONTEXT_PATH)
368+
));
369+
assert!(!standard_sbin_path_repair_enabled_for_context(
370+
&policy,
371+
Some("/tmp/cdi-context.json")
372+
));
373+
assert!(!standard_sbin_path_repair_enabled_for_context(
374+
&policy, None
375+
));
376+
}
377+
378+
#[test]
379+
fn standard_sbin_repair_requires_standard_sbin_policy_path() {
380+
let policy = policy_with_read_only(["/usr/local/bin/nvidia-smi"]);
381+
382+
assert!(!standard_sbin_path_repair_enabled_for_context(
383+
&policy,
384+
Some(openshell_core::cdi::CDI_CONTEXT_PATH)
385+
));
386+
}
387+
388+
#[test]
389+
fn standard_sbin_repair_accepts_read_write_standard_sbin_policy_path() {
390+
let mut policy = policy_with_read_only(std::iter::empty::<&str>());
391+
policy
392+
.filesystem
393+
.read_write
394+
.push("/sbin/vendor-tool".into());
395+
396+
assert!(standard_sbin_path_repair_enabled_for_context(
397+
&policy,
398+
Some(openshell_core::cdi::CDI_CONTEXT_PATH)
399+
));
400+
}
401+
402+
fn policy_with_read_only(
403+
paths: impl IntoIterator<Item = impl Into<std::path::PathBuf>>,
404+
) -> SandboxPolicy {
405+
SandboxPolicy {
406+
version: 1,
407+
filesystem: openshell_core::policy::FilesystemPolicy {
408+
read_only: paths.into_iter().map(Into::into).collect(),
409+
read_write: Vec::new(),
410+
include_workdir: false,
411+
},
412+
network: openshell_core::policy::NetworkPolicy::default(),
413+
landlock: openshell_core::policy::LandlockPolicy::default(),
414+
process: openshell_core::policy::ProcessPolicy::default(),
415+
}
416+
}
417+
293418
#[test]
294419
fn shell_command_with_standard_sbin_paths_extends_runtime_path() {
295420
let command = shell_command_with_standard_sbin_paths("printf '%s' \"$PATH\"");

‎crates/openshell-supervisor-process/src/process.rs‎

Lines changed: 66 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -185,13 +185,20 @@ fn strip_supervisor_only_env(cmd: &mut Command) {
185185
}
186186
}
187187

188-
fn inject_provider_env(cmd: &mut Command, provider_env: &HashMap<String, String>) {
188+
fn inject_provider_env(
189+
cmd: &mut Command,
190+
provider_env: &HashMap<String, String>,
191+
repair_standard_sbin: bool,
192+
) {
189193
for (key, value) in provider_env {
190194
if is_supervisor_only_env_var(key) {
191195
continue;
192196
}
193197
if key == "PATH" {
194-
cmd.env(key, child_env::path_with_standard_sbin_paths(value));
198+
cmd.env(
199+
key,
200+
child_env::maybe_path_with_standard_sbin_paths(value, repair_standard_sbin),
201+
);
195202
} else {
196203
cmd.env(key, value);
197204
}
@@ -688,8 +695,16 @@ fn shell_command_arg_index(args: &[String]) -> Option<usize> {
688695
None
689696
}
690697

691-
fn process_args_with_standard_sbin_paths(program: &str, args: &[String]) -> Vec<String> {
698+
fn process_args_with_standard_sbin_paths(
699+
program: &str,
700+
args: &[String],
701+
repair_standard_sbin: bool,
702+
) -> Vec<String> {
692703
let mut args = args.to_vec();
704+
if !repair_standard_sbin {
705+
return args;
706+
}
707+
693708
let basename = program.rsplit('/').next().unwrap_or(program);
694709
if matches!(basename, "bash" | "sh")
695710
&& let Some(command_index) = shell_command_arg_index(&args)
@@ -779,12 +794,13 @@ impl ProcessHandle {
779794
ca_paths: Option<&(PathBuf, PathBuf)>,
780795
provider_env: &HashMap<String, String>,
781796
) -> Result<Self> {
782-
let args = process_args_with_standard_sbin_paths(program, args);
797+
let repair_standard_sbin = child_env::standard_sbin_path_repair_enabled(policy);
798+
let args = process_args_with_standard_sbin_paths(program, args, repair_standard_sbin);
783799
let mut cmd = Command::new(program);
784800
cmd.args(args)
785801
.kill_on_drop(true)
786802
.env(openshell_core::sandbox_env::SANDBOX, "1")
787-
.env("PATH", child_env::child_path_from_env());
803+
.env("PATH", child_env::child_path_from_env(repair_standard_sbin));
788804

789805
let mut pty_master = None;
790806
let mut terminal_slave_fd = None;
@@ -815,7 +831,7 @@ impl ProcessHandle {
815831
// supervisor credentials from its inherited environment.
816832
strip_supervisor_only_env(&mut cmd);
817833

818-
inject_provider_env(&mut cmd, provider_env);
834+
inject_provider_env(&mut cmd, provider_env, repair_standard_sbin);
819835
apply_canonical_process_environment(
820836
&mut cmd,
821837
policy,
@@ -981,12 +997,13 @@ impl ProcessHandle {
981997
ca_paths: Option<&(PathBuf, PathBuf)>,
982998
provider_env: &HashMap<String, String>,
983999
) -> Result<Self> {
984-
let args = process_args_with_standard_sbin_paths(program, args);
1000+
let repair_standard_sbin = child_env::standard_sbin_path_repair_enabled(policy);
1001+
let args = process_args_with_standard_sbin_paths(program, args, repair_standard_sbin);
9851002
let mut cmd = Command::new(program);
9861003
cmd.args(args)
9871004
.kill_on_drop(true)
9881005
.env(openshell_core::sandbox_env::SANDBOX, "1")
989-
.env("PATH", child_env::child_path_from_env());
1006+
.env("PATH", child_env::child_path_from_env(repair_standard_sbin));
9901007

9911008
let mut pty_master = None;
9921009
let mut terminal_slave_fd = None;
@@ -1017,7 +1034,7 @@ impl ProcessHandle {
10171034
// inherited environment.
10181035
strip_supervisor_only_env(&mut cmd);
10191036

1020-
inject_provider_env(&mut cmd, provider_env);
1037+
inject_provider_env(&mut cmd, provider_env, repair_standard_sbin);
10211038
apply_canonical_process_environment(
10221039
&mut cmd,
10231040
policy,
@@ -2485,7 +2502,7 @@ mod tests {
24852502
fn process_args_wrap_shell_c_command_with_standard_sbin_paths() {
24862503
let args = vec!["-lc".to_string(), "nvidia-smi -L".to_string()];
24872504

2488-
let wrapped = process_args_with_standard_sbin_paths("sh", &args);
2505+
let wrapped = process_args_with_standard_sbin_paths("sh", &args, true);
24892506

24902507
assert_eq!(wrapped[0], "-lc");
24912508
assert!(wrapped[1].contains("/usr/sbin"));
@@ -2496,7 +2513,16 @@ mod tests {
24962513
fn process_args_leave_non_shell_commands_unchanged() {
24972514
let args = vec!["nvidia-smi -L".to_string()];
24982515

2499-
let wrapped = process_args_with_standard_sbin_paths("python", &args);
2516+
let wrapped = process_args_with_standard_sbin_paths("python", &args, true);
2517+
2518+
assert_eq!(wrapped, args);
2519+
}
2520+
2521+
#[test]
2522+
fn process_args_do_not_wrap_when_standard_sbin_repair_disabled() {
2523+
let args = vec!["-lc".to_string(), "nvidia-smi -L".to_string()];
2524+
2525+
let wrapped = process_args_with_standard_sbin_paths("sh", &args, false);
25002526

25012527
assert_eq!(wrapped, args);
25022528
}
@@ -3059,15 +3085,15 @@ mod tests {
30593085
))
30603086
.collect();
30613087

3062-
inject_provider_env(&mut cmd, &provider_env);
3088+
inject_provider_env(&mut cmd, &provider_env, false);
30633089

30643090
let output = cmd.output().await.expect("spawn env");
30653091
let stdout = String::from_utf8(output.stdout).expect("utf8");
30663092
assert!(stdout.contains("ANTHROPIC_API_KEY=openshell:resolve:env:ANTHROPIC_API_KEY"));
30673093
}
30683094

30693095
#[tokio::test]
3070-
async fn inject_provider_env_appends_standard_sbin_to_path() {
3096+
async fn inject_provider_env_appends_standard_sbin_to_path_when_enabled() {
30713097
let mut cmd = Command::new("/usr/bin/env");
30723098
cmd.env_clear()
30733099
.stdin(StdStdio::null())
@@ -3079,7 +3105,7 @@ mod tests {
30793105
"/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin".to_string(),
30803106
)]);
30813107

3082-
inject_provider_env(&mut cmd, &provider_env);
3108+
inject_provider_env(&mut cmd, &provider_env, true);
30833109

30843110
let output = cmd.output().await.expect("spawn env");
30853111
assert!(output.status.success());
@@ -3089,6 +3115,31 @@ mod tests {
30893115
}));
30903116
}
30913117

3118+
#[tokio::test]
3119+
async fn inject_provider_env_leaves_path_unchanged_when_standard_sbin_repair_disabled() {
3120+
let mut cmd = Command::new("/usr/bin/env");
3121+
cmd.env_clear()
3122+
.stdin(StdStdio::null())
3123+
.stdout(StdStdio::piped())
3124+
.stderr(StdStdio::null());
3125+
3126+
let provider_env = HashMap::from([(
3127+
"PATH".to_string(),
3128+
"/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin".to_string(),
3129+
)]);
3130+
3131+
inject_provider_env(&mut cmd, &provider_env, false);
3132+
3133+
let output = cmd.output().await.expect("spawn env");
3134+
assert!(output.status.success());
3135+
let stdout = String::from_utf8(output.stdout).expect("utf8");
3136+
assert!(
3137+
stdout
3138+
.lines()
3139+
.any(|line| { line == "PATH=/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin" })
3140+
);
3141+
}
3142+
30923143
#[cfg(unix)]
30933144
fn sandbox_policy_with_read_write(
30943145
path: PathBuf,
@@ -3966,7 +4017,7 @@ mod tests {
39664017
),
39674018
]);
39684019

3969-
inject_provider_env(&mut cmd, &provider_env);
4020+
inject_provider_env(&mut cmd, &provider_env, false);
39704021

39714022
let output = cmd.output().await.expect("spawn env");
39724023
assert!(output.status.success());

‎crates/openshell-supervisor-process/src/run.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,9 @@ pub async fn run_process(
119119
)?;
120120
}
121121

122-
crate::child_env::install_standard_sbin_path_startup_files(workspace.home());
122+
if crate::child_env::standard_sbin_path_repair_enabled(policy) {
123+
crate::child_env::install_standard_sbin_path_startup_files(workspace.home());
124+
}
123125

124126
// Eagerly fetch initial settings and install the agent skill if the
125127
// proposals flag is on at startup, rather than waiting for the policy

0 commit comments

Comments
 (0)