From 494da52fc63f75c04f905b80c6df545e644847d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20T=C3=B3th?= Date: Wed, 22 Apr 2026 20:47:01 +0200 Subject: [PATCH] Replace setenv with env list supporting host passthrough --- src/cli.rs | 22 ++---- src/config.rs | 166 +++++++++++++++++++++++++++++++++++-------- src/env.rs | 53 ++++++++++++++ src/errors.rs | 8 +++ src/lib.rs | 17 ++++- src/sandbox.rs | 18 ++++- tests/integration.rs | 44 +++++++++--- 7 files changed, 266 insertions(+), 62 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index d90d98a..61ecee8 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -86,14 +86,10 @@ pub struct Args { #[arg(long = "mask", value_name = "PATH", action = clap::ArgAction::Append)] pub mask: Vec, - /// Force-set an environment variable inside the sandbox (repeatable) - #[arg( - long = "setenv", - value_name = "KEY=VALUE", - value_parser = parse_key_value, - action = clap::ArgAction::Append, - )] - pub setenv: Vec<(String, String)>, + /// Configure an env var inside the sandbox (repeatable). + /// `KEY` passes through from the host if set; `KEY=VALUE` sets explicitly. + #[arg(long = "env", value_name = "KEY[=VALUE]", action = clap::ArgAction::Append)] + pub env: Vec, /// Force-unset an environment variable inside the sandbox (repeatable) #[arg(long = "unsetenv", value_name = "KEY", action = clap::ArgAction::Append)] @@ -111,13 +107,3 @@ pub struct Args { #[arg(trailing_var_arg = true, allow_hyphen_values = true)] pub command_and_args: Vec, } - -fn parse_key_value(raw: &str) -> Result<(String, String), String> { - let (key, value) = raw - .split_once('=') - .ok_or_else(|| format!("expected KEY=VALUE, got {raw:?}"))?; - if key.is_empty() { - return Err(format!("empty key in {raw:?}")); - } - Ok((key.to_string(), value.to_string())) -} diff --git a/src/config.rs b/src/config.rs index fa7a3b3..12cb9f6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,11 +1,11 @@ -use std::collections::{BTreeMap, HashMap}; +use std::collections::{HashMap, HashSet}; use std::ffi::OsString; use std::path::{Path, PathBuf}; use serde::Deserialize; use crate::cli::Args; -use crate::{SandboxConfig, SandboxError, SandboxMode}; +use crate::{EnvEntry, SandboxConfig, SandboxError, SandboxMode}; pub fn build(args: Args, file_config: Option) -> Result { let (mut globals, mut profile) = match file_config { @@ -26,6 +26,14 @@ pub fn build(args: Args, file_config: Option) -> Result) -> Result(cli: Vec, profile: &[T], globals: &[T]) -> Vec { globals.iter().chain(profile).cloned().chain(cli).collect() } -fn merge_setenv( - cli: Vec<(String, String)>, - profile: &BTreeMap, - globals: &BTreeMap, -) -> Vec<(String, String)> { - let mut merged: BTreeMap = globals.clone(); - for (k, v) in profile { - merged.insert(k.clone(), v.clone()); +fn parse_env_entries(raw: &[String]) -> Result, SandboxError> { + raw.iter() + .map(|s| match s.split_once('=') { + None if s.is_empty() => Err(SandboxError::InvalidEnvEntry(s.clone())), + None => Ok(EnvEntry::Keep(s.clone())), + Some(("", _)) => Err(SandboxError::InvalidEnvEntry(s.clone())), + Some((key, value)) => Ok(EnvEntry::Set(key.to_string(), value.to_string())), + }) + .collect() +} + +fn dedupe_env_last_wins(entries: Vec) -> Vec { + let mut seen: HashSet = HashSet::new(); + let mut result: Vec = Vec::new(); + for entry in entries.into_iter().rev() { + if seen.insert(entry.key().to_string()) { + result.push(entry); + } } - for (k, v) in cli { - merged.insert(k, v); + result.reverse(); + result +} + +fn reject_env_key_conflicts(env: &[EnvEntry], unsetenv: &[String]) -> Result<(), SandboxError> { + let env_keys: HashSet<&str> = env.iter().map(EnvEntry::key).collect(); + let unsetenv_keys: HashSet<&str> = unsetenv.iter().map(String::as_str).collect(); + match env_keys.intersection(&unsetenv_keys).next() { + Some(&key) => Err(SandboxError::ConflictingEnvKey(key.to_string())), + None => Ok(()), } - merged.into_iter().collect() } fn resolve_command( @@ -295,7 +320,7 @@ pub struct Options { #[serde(default)] pub mask: Vec, #[serde(default)] - pub setenv: BTreeMap, + pub env: Vec, #[serde(default)] pub unsetenv: Vec, #[serde(default)] @@ -937,22 +962,16 @@ mod tests { } #[test] - fn build_setenv_merges_globals_profile_cli() { + fn build_env_accumulates_disjoint_keys() { let file_config = FileConfig { options: Options { - setenv: BTreeMap::from([ - ("A".into(), "global".into()), - ("B".into(), "global".into()), - ]), + env: vec!["A=global".into(), "B".into()], ..Options::default() }, profile: HashMap::from([( "p".into(), Options { - setenv: BTreeMap::from([ - ("B".into(), "profile".into()), - ("C".into(), "profile".into()), - ]), + env: vec!["C=profile".into()], ..Options::default() }, )]), @@ -960,21 +979,106 @@ mod tests { }; let args = Args { profile: Some("p".into()), - setenv: vec![("C".into(), "cli".into()), ("D".into(), "cli".into())], + env: vec!["D".into()], ..args_with_command() }; let config = build(args, Some(file_config)).unwrap(); assert_eq!( - config.setenv, + config.env, vec![ - ("A".into(), "global".into()), - ("B".into(), "profile".into()), - ("C".into(), "cli".into()), - ("D".into(), "cli".into()), + EnvEntry::Set("A".into(), "global".into()), + EnvEntry::Keep("B".into()), + EnvEntry::Set("C".into(), "profile".into()), + EnvEntry::Keep("D".into()), ] ); } + #[test] + fn build_env_later_tier_overrides_earlier() { + let file_config = FileConfig { + options: Options { + env: vec!["A=global".into()], + ..Options::default() + }, + profile: HashMap::from([( + "p".into(), + Options { + env: vec!["A=profile".into()], + ..Options::default() + }, + )]), + ..FileConfig::default() + }; + let args = Args { + profile: Some("p".into()), + env: vec!["A=cli".into()], + ..args_with_command() + }; + let config = build(args, Some(file_config)).unwrap(); + assert_eq!(config.env, vec![EnvEntry::Set("A".into(), "cli".into())]); + } + + #[test] + fn build_env_conflicts_with_unsetenv() { + let file_config = FileConfig { + options: Options { + env: vec!["X=1".into()], + unsetenv: vec!["X".into()], + ..Options::default() + }, + ..FileConfig::default() + }; + assert!(matches!( + build(args_with_command(), Some(file_config)), + Err(SandboxError::ConflictingEnvKey(k)) if k == "X" + )); + } + + #[test] + fn build_env_keep_conflicts_with_unsetenv() { + let file_config = FileConfig { + options: Options { + env: vec!["X".into()], + unsetenv: vec!["X".into()], + ..Options::default() + }, + ..FileConfig::default() + }; + assert!(matches!( + build(args_with_command(), Some(file_config)), + Err(SandboxError::ConflictingEnvKey(k)) if k == "X" + )); + } + + #[test] + fn build_env_rejects_empty_key() { + let file_config = FileConfig { + options: Options { + env: vec!["=foo".into()], + ..Options::default() + }, + ..FileConfig::default() + }; + assert!(matches!( + build(args_with_command(), Some(file_config)), + Err(SandboxError::InvalidEnvEntry(_)) + )); + } + + #[test] + fn build_env_set_to_empty_string_is_allowed() { + let file_config = FileConfig { + options: Options { + env: vec!["DEBUG=".into()], + ..Options::default() + }, + ..FileConfig::default() + }; + let config = build(args_with_command(), Some(file_config)).unwrap(); + assert_eq!(config.env, vec![EnvEntry::Set("DEBUG".into(), "".into())]); + } + #[test] fn build_unsetenv_accumulates() { let file_config = FileConfig { diff --git a/src/env.rs b/src/env.rs index c128a2f..28b9e74 100644 --- a/src/env.rs +++ b/src/env.rs @@ -1,3 +1,15 @@ +pub fn keepenv_args(keys: &[String], parent_env: &[(String, String)]) -> Vec { + let mut args = Vec::new(); + for key in keys { + if let Some(value) = parent_env.iter().find_map(|(k, v)| (k == key).then_some(v)) { + args.push("--setenv".to_string()); + args.push(key.clone()); + args.push(value.clone()); + } + } + args +} + pub fn whitelist_env_args(parent_env: &[(String, String)]) -> Vec { let mut args = vec!["--clearenv".to_string()]; for (key, value) in parent_env { @@ -134,3 +146,44 @@ const BLACKLIST_DROP_SUFFIXES: &[&str] = &[ "_PRIVATE_KEY", "_CLIENT_SECRET", ]; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn keepenv_emits_setenv_for_present_key() { + let parent = vec![("XDG_RUNTIME_DIR".into(), "/run/user/1000".into())]; + let args = keepenv_args(&["XDG_RUNTIME_DIR".into()], &parent); + assert_eq!(args, vec!["--setenv", "XDG_RUNTIME_DIR", "/run/user/1000"]); + } + + #[test] + fn keepenv_skips_absent_keys() { + let parent = vec![("HOME".into(), "/home/me".into())]; + let args = keepenv_args(&["XDG_RUNTIME_DIR".into()], &parent); + assert!(args.is_empty()); + } + + #[test] + fn keepenv_preserves_caller_key_order() { + let parent = vec![ + ("B".into(), "2".into()), + ("A".into(), "1".into()), + ("C".into(), "3".into()), + ]; + let args = keepenv_args(&["A".into(), "B".into(), "C".into()], &parent); + assert_eq!( + args, + vec![ + "--setenv", "A", "1", "--setenv", "B", "2", "--setenv", "C", "3" + ] + ); + } + + #[test] + fn keepenv_empty_keys_yields_nothing() { + let parent = vec![("A".into(), "1".into())]; + assert!(keepenv_args(&[], &parent).is_empty()); + } +} diff --git a/src/errors.rs b/src/errors.rs index 98a5523..e28eb44 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -22,6 +22,8 @@ pub enum SandboxError { }, ProfileNotFound(String), ConflictingMode, + ConflictingEnvKey(String), + InvalidEnvEntry(String), UnknownConfigKey(String), ConfigPathNotAbsolute(PathBuf), InvalidBwrapArg(String), @@ -65,6 +67,12 @@ impl std::fmt::Display for SandboxError { f, "config section sets both blacklist and whitelist to true" ), + Self::ConflictingEnvKey(key) => { + write!(f, "conflicting environment variable options for key: {key}") + } + Self::InvalidEnvEntry(raw) => { + write!(f, "invalid env entry (expected KEY or KEY=VALUE): {raw:?}") + } Self::UnknownConfigKey(key) => write!(f, "unknown config key: {key}"), Self::ConfigPathNotAbsolute(p) => { write!(f, "config path is not absolute: {}", p.display()) diff --git a/src/lib.rs b/src/lib.rs index b39e4f3..8269f6b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -20,6 +20,21 @@ pub enum SandboxMode { Whitelist, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum EnvEntry { + Keep(String), + Set(String, String), +} + +impl EnvEntry { + pub fn key(&self) -> &str { + match self { + Self::Keep(k) => k, + Self::Set(k, _) => k, + } + } +} + pub struct SandboxConfig { pub mode: SandboxMode, pub hardened: bool, @@ -29,7 +44,7 @@ pub struct SandboxConfig { pub extra_rw: Vec, pub extra_ro: Vec, pub mask: Vec, - pub setenv: Vec<(String, String)>, + pub env: Vec, pub unsetenv: Vec, pub bwrap_args: Vec, pub command: PathBuf, diff --git a/src/sandbox.rs b/src/sandbox.rs index 277d881..1e889cc 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -5,7 +5,7 @@ use crate::agents; use crate::blacklist; use crate::env; use crate::seccomp; -use crate::{SandboxConfig, SandboxError, SandboxMode}; +use crate::{EnvEntry, SandboxConfig, SandboxError, SandboxMode}; pub fn build_command(config: &SandboxConfig) -> Result { let mut cmd = Command::new("bwrap"); @@ -71,9 +71,21 @@ fn add_env_policy(cmd: &mut Command, config: &SandboxConfig) { } fn add_user_env_overrides(cmd: &mut Command, config: &SandboxConfig) { - for (key, value) in &config.setenv { - cmd.arg("--setenv").arg(key).arg(value); + let mut keep_keys: Vec = Vec::new(); + for entry in &config.env { + match entry { + EnvEntry::Set(key, value) => { + cmd.arg("--setenv").arg(key).arg(value); + } + EnvEntry::Keep(key) => keep_keys.push(key.clone()), + } } + + if !keep_keys.is_empty() { + let parent_env: Vec<(String, String)> = std::env::vars().collect(); + cmd.args(env::keepenv_args(&keep_keys, &parent_env)); + } + for key in &config.unsetenv { cmd.arg("--unsetenv").arg(key); } diff --git a/tests/integration.rs b/tests/integration.rs index 7f69e6b..6855018 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -1163,13 +1163,39 @@ fn whitelist_strips_dbus_vars() { } #[test] -fn whitelist_setenv_injects_user_var() { +fn whitelist_env_sets_user_var() { let stdout = printenv_inside( - &["--whitelist", "--setenv", "USER_INJECTED=forced"], + &["--whitelist", "--env", "USER_INJECTED=forced"], &[], &["USER_INJECTED"], ); - assert!(stdout.contains("forced"), "setenv not applied: {stdout}"); + assert!(stdout.contains("forced"), "env not applied: {stdout}"); +} + +#[test] +fn whitelist_env_keep_passes_through_host_var() { + let stdout = printenv_inside( + &["--whitelist", "--env", "PASSED_THROUGH"], + &[("PASSED_THROUGH", "from-host")], + &["PASSED_THROUGH"], + ); + assert!( + stdout.contains("from-host"), + "expected --env KEY to pass host value through: {stdout}" + ); +} + +#[test] +fn whitelist_env_keep_absent_host_var_is_skipped() { + let stdout = printenv_inside( + &["--whitelist", "--env", "NEVER_SET_ON_HOST"], + &[], + &["NEVER_SET_ON_HOST"], + ); + assert!( + stdout.contains("MISSING:NEVER_SET_ON_HOST"), + "expected absent keep-var to remain unset: {stdout}" + ); } #[test] @@ -1301,28 +1327,28 @@ fn no_env_filter_blacklist_keeps_secrets() { } #[test] -fn no_env_filter_still_honors_user_setenv() { +fn no_env_filter_still_honors_user_env() { let stdout = printenv_inside( - &["--no-env-filter", "--setenv", "FORCED=yes"], + &["--no-env-filter", "--env", "FORCED=yes"], &[], &["FORCED"], ); assert!( stdout.contains("yes"), - "expected user --setenv to still work with --no-env-filter, got: {stdout}" + "expected user --env to still work with --no-env-filter, got: {stdout}" ); } #[test] -fn blacklist_setenv_overrides_builtin_deny() { +fn blacklist_env_overrides_builtin_deny() { let stdout = printenv_inside( - &["--setenv", "GH_TOKEN=overridden"], + &["--env", "GH_TOKEN=overridden"], &[("GH_TOKEN", "original")], &["GH_TOKEN"], ); assert!( stdout.contains("overridden"), - "expected --setenv to override deny, got: {stdout}" + "expected --env to override deny, got: {stdout}" ); assert!(!stdout.contains("original")); }