From 0a00b5ed294e4595402c08968952e990e72140f9 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 29 Apr 2025 15:01:16 -0700 Subject: [PATCH] fix: overhaul SandboxPolicy and config loading in Rust (#732) Previous to this PR, `SandboxPolicy` was a bit difficult to work with: https://github.com/openai/codex/blob/237f8a11e11fdcc793a09e787e48215676d9b95b/codex-rs/core/src/protocol.rs#L98-L108 Specifically: * It was an `enum` and therefore options were mutually exclusive as opposed to additive. * It defined things in terms of what the agent _could not_ do as opposed to what they _could_ do. This made things hard to support because we would prefer to build up a sandbox config by starting with something extremely restrictive and only granting permissions for things the user as explicitly allowed. This PR changes things substantially by redefining the policy in terms of two concepts: * A `SandboxPermission` enum that defines permissions that can be granted to the agent/sandbox. * A `SandboxPolicy` that internally stores a `Vec`, but externally exposes a simpler API that can be used to configure Seatbelt/Landlock. Previous to this PR, we supported a `--sandbox` flag that effectively mapped to an enum value in `SandboxPolicy`. Though now that `SandboxPolicy` is a wrapper around `Vec`, the single `--sandbox` flag no longer makes sense. While I could have turned it into a flag that the user can specify multiple times, I think the current values to use with such a flag are long and potentially messy, so for the moment, I have dropped support for `--sandbox` altogether and we can bring it back once we have figured out the naming thing. Since `--sandbox` is gone, users now have to specify `--full-auto` to get a sandbox that allows writes in `cwd`. Admittedly, there is no clean way to specify the equivalent of `--full-auto` in your `config.toml` right now, so we will have to revisit that, as well. Because `Config` presents a `SandboxPolicy` field and `SandboxPolicy` changed considerably, I had to overhaul how config loading works, as well. There are now two distinct concepts, `ConfigToml` and `Config`: * `ConfigToml` is the deserialization of `~/.codex/config.toml`. As one might expect, every field is `Optional` and it is `#[derive(Deserialize, Default)]`. Consistent use of `Optional` makes it clear what the user has specified explicitly. * `Config` is the "normalized config" and is produced by merging `ConfigToml` with `ConfigOverrides`. Where `ConfigToml` contains a raw `Option>`, `Config` presents only the final `SandboxPolicy`. The changes to `core/src/exec.rs` and `core/src/linux.rs` merit extra special attention to ensure we are faithfully mapping the `SandboxPolicy` to the Seatbelt and Landlock configs, respectively. Also, take note that `core/src/seatbelt_readonly_policy.sbpl` has been renamed to `codex-rs/core/src/seatbelt_base_policy.sbpl` and that `(allow file-read*)` has been removed from the `.sbpl` file as now this is added to the policy in `core/src/exec.rs` when `sandbox_policy.has_full_disk_read_access()` is `true`. --- codex-rs/cli/src/landlock.rs | 13 +- codex-rs/cli/src/main.rs | 32 ++-- codex-rs/cli/src/seatbelt.rs | 4 +- codex-rs/core/src/approval_mode_cli_arg.rs | 27 --- codex-rs/core/src/codex.rs | 12 +- codex-rs/core/src/config.rs | 136 ++++++++----- codex-rs/core/src/exec.rs | 96 ++++++---- codex-rs/core/src/lib.rs | 2 - codex-rs/core/src/linux.rs | 68 +++---- codex-rs/core/src/protocol.rs | 179 +++++++++++++++--- codex-rs/core/src/safety.rs | 13 +- ..._policy.sbpl => seatbelt_base_policy.sbpl} | 3 - codex-rs/core/tests/live_agent.rs | 2 +- codex-rs/core/tests/previous_response_id.rs | 2 +- codex-rs/core/tests/stream_no_completed.rs | 2 +- codex-rs/exec/src/cli.rs | 9 +- codex-rs/exec/src/lib.rs | 11 +- codex-rs/repl/src/cli.rs | 9 +- codex-rs/repl/src/lib.rs | 15 +- codex-rs/tui/src/cli.rs | 17 +- codex-rs/tui/src/lib.rs | 15 +- 21 files changed, 408 insertions(+), 259 deletions(-) rename codex-rs/core/src/{seatbelt_readonly_policy.sbpl => seatbelt_base_policy.sbpl} (97%) diff --git a/codex-rs/cli/src/landlock.rs b/codex-rs/cli/src/landlock.rs index be2ba1e3..b57591bf 100644 --- a/codex-rs/cli/src/landlock.rs +++ b/codex-rs/cli/src/landlock.rs @@ -5,7 +5,6 @@ use codex_core::protocol::SandboxPolicy; use std::os::unix::process::ExitStatusExt; -use std::path::PathBuf; use std::process; use std::process::Command; use std::process::ExitStatus; @@ -15,7 +14,6 @@ use std::process::ExitStatus; pub(crate) fn run_landlock( command: Vec, sandbox_policy: SandboxPolicy, - writable_roots: Vec, ) -> anyhow::Result<()> { if command.is_empty() { anyhow::bail!("command args are empty"); @@ -23,16 +21,7 @@ pub(crate) fn run_landlock( // Spawn a new thread and apply the sandbox policies there. let handle = std::thread::spawn(move || -> anyhow::Result { - // Apply sandbox policies inside this thread so only the child inherits - // them, not the entire CLI process. - if sandbox_policy.is_network_restricted() { - codex_core::linux::install_network_seccomp_filter_on_current_thread()?; - } - - if sandbox_policy.is_file_write_restricted() { - codex_core::linux::install_filesystem_landlock_rules_on_current_thread(writable_roots)?; - } - + codex_core::linux::apply_sandbox_policy_to_current_thread(sandbox_policy)?; let status = Command::new(&command[0]).args(&command[1..]).status()?; Ok(status) }); diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index d8a58de8..fa0a14e6 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -7,7 +7,7 @@ use std::path::PathBuf; use clap::ArgAction; use clap::Parser; -use codex_core::SandboxModeCliArg; +use codex_core::protocol::SandboxPolicy; use codex_exec::Cli as ExecCli; use codex_repl::Cli as ReplCli; use codex_tui::Cli as TuiCli; @@ -71,9 +71,9 @@ struct SeatbeltCommand { #[arg(long = "writable-root", short = 'w', value_name = "DIR", action = ArgAction::Append, use_value_delimiter = false)] writable_roots: Vec, - /// Configure the process restrictions for the command. - #[arg(long = "sandbox", short = 's')] - sandbox_policy: SandboxModeCliArg, + /// Convenience alias for low-friction sandboxed automatic execution (network-disabled sandbox that can write to cwd and TMPDIR) + #[arg(long = "full-auto", default_value_t = false)] + full_auto: bool, /// Full command args to run under seatbelt. #[arg(trailing_var_arg = true)] @@ -86,9 +86,9 @@ struct LandlockCommand { #[arg(long = "writable-root", short = 'w', value_name = "DIR", action = ArgAction::Append, use_value_delimiter = false)] writable_roots: Vec, - /// Configure the process restrictions for the command. - #[arg(long = "sandbox", short = 's')] - sandbox_policy: SandboxModeCliArg, + /// Convenience alias for low-friction sandboxed automatic execution (network-disabled sandbox that can write to cwd and TMPDIR) + #[arg(long = "full-auto", default_value_t = false)] + full_auto: bool, /// Full command args to run under landlock. #[arg(trailing_var_arg = true)] @@ -118,18 +118,20 @@ async fn main() -> anyhow::Result<()> { Some(Subcommand::Debug(debug_args)) => match debug_args.cmd { DebugCommand::Seatbelt(SeatbeltCommand { command, - sandbox_policy, writable_roots, + full_auto, }) => { - seatbelt::run_seatbelt(command, sandbox_policy.into(), writable_roots).await?; + let sandbox_policy = create_sandbox_policy(full_auto, &writable_roots); + seatbelt::run_seatbelt(command, sandbox_policy).await?; } #[cfg(target_os = "linux")] DebugCommand::Landlock(LandlockCommand { command, - sandbox_policy, writable_roots, + full_auto, }) => { - landlock::run_landlock(command, sandbox_policy.into(), writable_roots)?; + let sandbox_policy = create_sandbox_policy(full_auto, &writable_roots); + landlock::run_landlock(command, sandbox_policy)?; } #[cfg(not(target_os = "linux"))] DebugCommand::Landlock(_) => { @@ -140,3 +142,11 @@ async fn main() -> anyhow::Result<()> { Ok(()) } + +fn create_sandbox_policy(full_auto: bool, writable_roots: &[PathBuf]) -> SandboxPolicy { + if full_auto { + SandboxPolicy::new_full_auto_policy_with_writable_roots(writable_roots) + } else { + SandboxPolicy::new_read_only_policy_with_writable_roots(writable_roots) + } +} diff --git a/codex-rs/cli/src/seatbelt.rs b/codex-rs/cli/src/seatbelt.rs index d328f552..f4a8edde 100644 --- a/codex-rs/cli/src/seatbelt.rs +++ b/codex-rs/cli/src/seatbelt.rs @@ -1,13 +1,11 @@ use codex_core::exec::create_seatbelt_command; use codex_core::protocol::SandboxPolicy; -use std::path::PathBuf; pub(crate) async fn run_seatbelt( command: Vec, sandbox_policy: SandboxPolicy, - writable_roots: Vec, ) -> anyhow::Result<()> { - let seatbelt_command = create_seatbelt_command(command, sandbox_policy, &writable_roots); + let seatbelt_command = create_seatbelt_command(command, &sandbox_policy); let status = tokio::process::Command::new(seatbelt_command[0].clone()) .args(&seatbelt_command[1..]) .spawn() diff --git a/codex-rs/core/src/approval_mode_cli_arg.rs b/codex-rs/core/src/approval_mode_cli_arg.rs index 0da6a89e..8154e49f 100644 --- a/codex-rs/core/src/approval_mode_cli_arg.rs +++ b/codex-rs/core/src/approval_mode_cli_arg.rs @@ -4,7 +4,6 @@ use clap::ValueEnum; use crate::protocol::AskForApproval; -use crate::protocol::SandboxPolicy; #[derive(Clone, Copy, Debug, ValueEnum)] #[value(rename_all = "kebab-case")] @@ -24,19 +23,6 @@ pub enum ApprovalModeCliArg { Never, } -#[derive(Clone, Copy, Debug, ValueEnum)] -#[value(rename_all = "kebab-case")] -pub enum SandboxModeCliArg { - /// Network syscalls will be blocked - NetworkRestricted, - /// Filesystem writes will be restricted - FileWriteRestricted, - /// Network and filesystem writes will be restricted - NetworkAndFileWriteRestricted, - /// No restrictions; full "unsandboxed" mode - DangerousNoRestrictions, -} - impl From for AskForApproval { fn from(value: ApprovalModeCliArg) -> Self { match value { @@ -46,16 +32,3 @@ impl From for AskForApproval { } } } - -impl From for SandboxPolicy { - fn from(value: SandboxModeCliArg) -> Self { - match value { - SandboxModeCliArg::NetworkRestricted => SandboxPolicy::NetworkRestricted, - SandboxModeCliArg::FileWriteRestricted => SandboxPolicy::FileWriteRestricted, - SandboxModeCliArg::NetworkAndFileWriteRestricted => { - SandboxPolicy::NetworkAndFileWriteRestricted - } - SandboxModeCliArg::DangerousNoRestrictions => SandboxPolicy::DangerousNoRestrictions, - } - } -} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index edeaef99..384011e3 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -861,7 +861,7 @@ async fn handle_function_call( assess_command_safety( ¶ms.command, sess.approval_policy, - sess.sandbox_policy, + &sess.sandbox_policy, &state.approved_commands, ) }; @@ -916,14 +916,11 @@ async fn handle_function_call( ) .await; - let roots_snapshot = { sess.writable_roots.lock().unwrap().clone() }; - let output_result = process_exec_tool_call( params.clone(), sandbox_type, - &roots_snapshot, sess.ctrl_c.clone(), - sess.sandbox_policy, + &sess.sandbox_policy, ) .await; @@ -1006,16 +1003,13 @@ async fn handle_function_call( ) .await; - let retry_roots = { sess.writable_roots.lock().unwrap().clone() }; - // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. let retry_output_result = process_exec_tool_call( params.clone(), SandboxType::None, - &retry_roots, sess.ctrl_c.clone(), - sess.sandbox_policy, + &sess.sandbox_policy, ) .await; diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index 95abae52..55efe5a9 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -1,5 +1,6 @@ use crate::flags::OPENAI_DEFAULT_MODEL; use crate::protocol::AskForApproval; +use crate::protocol::SandboxPermission; use crate::protocol::SandboxPolicy; use dirs::home_dir; use serde::Deserialize; @@ -11,27 +12,68 @@ use std::path::PathBuf; const EMBEDDED_INSTRUCTIONS: &str = include_str!("../prompt.md"); /// Application configuration loaded from disk and merged with overrides. -#[derive(Deserialize, Debug, Clone)] +#[derive(Debug, Clone)] pub struct Config { /// Optional override of model selection. - #[serde(default = "default_model")] pub model: String, - /// Default approval policy for executing commands. - #[serde(default)] + + /// Approval policy for executing commands. pub approval_policy: AskForApproval, - #[serde(default)] + pub sandbox_policy: SandboxPolicy, /// Disable server-side response storage (sends the full conversation /// context with every request). Currently necessary for OpenAI customers /// who have opted into Zero Data Retention (ZDR). - #[serde(default)] pub disable_response_storage: bool, /// System instructions. pub instructions: Option, } +/// Base config deserialized from ~/.codex/config.toml. +#[derive(Deserialize, Debug, Clone, Default)] +pub struct ConfigToml { + /// Optional override of model selection. + pub model: Option, + + /// Default approval policy for executing commands. + pub approval_policy: Option, + + pub sandbox_permissions: Option>, + + /// Disable server-side response storage (sends the full conversation + /// context with every request). Currently necessary for OpenAI customers + /// who have opted into Zero Data Retention (ZDR). + pub disable_response_storage: Option, + + /// System instructions. + pub instructions: Option, +} + +impl ConfigToml { + /// Attempt to parse the file at `~/.codex/config.toml`. If it does not + /// exist, return a default config. Though if it exists and cannot be + /// parsed, report that to the user and force them to fix it. + fn load_from_toml() -> std::io::Result { + let config_toml_path = codex_dir()?.join("config.toml"); + match std::fs::read_to_string(&config_toml_path) { + Ok(contents) => toml::from_str::(&contents).map_err(|e| { + tracing::error!("Failed to parse config.toml: {e}"); + std::io::Error::new(std::io::ErrorKind::InvalidData, e) + }), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + tracing::info!("config.toml not found, using defaults"); + Ok(Self::default()) + } + Err(e) => { + tracing::error!("Failed to read config.toml: {e}"); + Err(e) + } + } + } +} + /// Optional overrides for user configuration (e.g., from CLI flags). #[derive(Default, Debug, Clone)] pub struct ConfigOverrides { @@ -46,11 +88,14 @@ impl Config { /// ~/.codex/config.toml, ~/.codex/instructions.md, embedded defaults, and /// any values provided in `overrides` (highest precedence). pub fn load_with_overrides(overrides: ConfigOverrides) -> std::io::Result { - let mut cfg: Config = Self::load_from_toml()?; + let cfg: ConfigToml = ConfigToml::load_from_toml()?; tracing::warn!("Config parsed from config.toml: {cfg:?}"); + Ok(Self::load_from_base_config_with_overrides(cfg, overrides)) + } + fn load_from_base_config_with_overrides(cfg: ConfigToml, overrides: ConfigOverrides) -> Self { // Instructions: user-provided instructions.md > embedded default. - cfg.instructions = + let instructions = Self::load_instructions().or_else(|| Some(EMBEDDED_INSTRUCTIONS.to_string())); // Destructure ConfigOverrides fully to ensure all overrides are applied. @@ -61,57 +106,48 @@ impl Config { disable_response_storage, } = overrides; - if let Some(model) = model { - cfg.model = model; - } - if let Some(approval_policy) = approval_policy { - cfg.approval_policy = approval_policy; - } - if let Some(sandbox_policy) = sandbox_policy { - cfg.sandbox_policy = sandbox_policy; - } - if let Some(disable_response_storage) = disable_response_storage { - cfg.disable_response_storage = disable_response_storage; - } - Ok(cfg) - } - - /// Attempt to parse the file at `~/.codex/config.toml` into a Config. - fn load_from_toml() -> std::io::Result { - let config_toml_path = codex_dir()?.join("config.toml"); - match std::fs::read_to_string(&config_toml_path) { - Ok(contents) => toml::from_str::(&contents).map_err(|e| { - tracing::error!("Failed to parse config.toml: {e}"); - std::io::Error::new(std::io::ErrorKind::InvalidData, e) - }), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - tracing::info!("config.toml not found, using defaults"); - Ok(Self::load_default_config()) - } - Err(e) => { - tracing::error!("Failed to read config.toml: {e}"); - Err(e) + let sandbox_policy = match sandbox_policy { + Some(sandbox_policy) => sandbox_policy, + None => { + // Derive a SandboxPolicy from the permissions in the config. + match cfg.sandbox_permissions { + // Note this means the user can explicitly set permissions + // to the empty list in the config file, granting it no + // permissions whatsoever. + Some(permissions) => SandboxPolicy::from(permissions), + // Default to read only rather than completely locked down. + None => SandboxPolicy::new_read_only_policy(), + } } + }; + + Self { + model: model.or(cfg.model).unwrap_or_else(default_model), + approval_policy: approval_policy + .or(cfg.approval_policy) + .unwrap_or_else(AskForApproval::default), + sandbox_policy, + disable_response_storage: disable_response_storage + .or(cfg.disable_response_storage) + .unwrap_or(false), + instructions, } } - /// Meant to be used exclusively for tests: load_with_overrides() should be - /// used in all other cases. - pub fn load_default_config_for_test() -> Self { - Self::load_default_config() - } - - fn load_default_config() -> Self { - // Load from an empty string to exercise #[serde(default)] to - // get the default values for each field. - toml::from_str::("").expect("empty string should parse as TOML") - } - fn load_instructions() -> Option { let mut p = codex_dir().ok()?; p.push("instructions.md"); std::fs::read_to_string(&p).ok() } + + /// Meant to be used exclusively for tests: `load_with_overrides()` should + /// be used in all other cases. + pub fn load_default_config_for_test() -> Self { + Self::load_from_base_config_with_overrides( + ConfigToml::default(), + ConfigOverrides::default(), + ) + } } fn default_model() -> String { diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 952b4453..cf5fbd61 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -1,7 +1,6 @@ use std::io; #[cfg(target_family = "unix")] use std::os::unix::process::ExitStatusExt; -use std::path::PathBuf; use std::process::ExitStatus; use std::process::Stdio; use std::sync::Arc; @@ -33,7 +32,7 @@ const DEFAULT_TIMEOUT_MS: u64 = 10_000; const SIGKILL_CODE: i32 = 9; const TIMEOUT_CODE: i32 = 64; -const MACOS_SEATBELT_READONLY_POLICY: &str = include_str!("seatbelt_readonly_policy.sbpl"); +const MACOS_SEATBELT_BASE_POLICY: &str = include_str!("seatbelt_base_policy.sbpl"); /// When working with `sandbox-exec`, only consider `sandbox-exec` in `/usr/bin` /// to defend against an attacker trying to inject a malicious version on the @@ -67,19 +66,17 @@ pub enum SandboxType { #[cfg(target_os = "linux")] async fn exec_linux( params: ExecParams, - writable_roots: &[PathBuf], ctrl_c: Arc, - sandbox_policy: SandboxPolicy, + sandbox_policy: &SandboxPolicy, ) -> Result { - crate::linux::exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await + crate::linux::exec_linux(params, ctrl_c, sandbox_policy).await } #[cfg(not(target_os = "linux"))] async fn exec_linux( _params: ExecParams, - _writable_roots: &[PathBuf], _ctrl_c: Arc, - _sandbox_policy: SandboxPolicy, + _sandbox_policy: &SandboxPolicy, ) -> Result { Err(CodexErr::Io(io::Error::new( io::ErrorKind::InvalidInput, @@ -90,9 +87,8 @@ async fn exec_linux( pub async fn process_exec_tool_call( params: ExecParams, sandbox_type: SandboxType, - writable_roots: &[PathBuf], ctrl_c: Arc, - sandbox_policy: SandboxPolicy, + sandbox_policy: &SandboxPolicy, ) -> Result { let start = Instant::now(); @@ -104,7 +100,7 @@ pub async fn process_exec_tool_call( workdir, timeout_ms, } = params; - let seatbelt_command = create_seatbelt_command(command, sandbox_policy, writable_roots); + let seatbelt_command = create_seatbelt_command(command, sandbox_policy); exec( ExecParams { command: seatbelt_command, @@ -115,9 +111,7 @@ pub async fn process_exec_tool_call( ) .await } - SandboxType::LinuxSeccomp => { - exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await - } + SandboxType::LinuxSeccomp => exec_linux(params, ctrl_c, sandbox_policy).await, }; let duration = start.elapsed(); match raw_output_result { @@ -162,41 +156,61 @@ pub async fn process_exec_tool_call( pub fn create_seatbelt_command( command: Vec, - sandbox_policy: SandboxPolicy, - writable_roots: &[PathBuf], + sandbox_policy: &SandboxPolicy, ) -> Vec { - let (policies, cli_args): (Vec, Vec) = writable_roots - .iter() - .enumerate() - .map(|(index, root)| { - let param_name = format!("WRITABLE_ROOT_{index}"); - let policy: String = format!("(subpath (param \"{param_name}\"))"); - let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); - (policy, cli_arg) - }) - .unzip(); - - // TODO(ragona): The seatbelt policy should reflect the SandboxPolicy that - // is passed, but everything is currently hardcoded to use - // MACOS_SEATBELT_READONLY_POLICY. - // TODO(mbolin): apply_patch calls must also honor the SandboxPolicy. - if !matches!(sandbox_policy, SandboxPolicy::NetworkRestricted) { - tracing::error!("specified sandbox policy {sandbox_policy:?} will not be honroed"); - } - - let full_policy = if policies.is_empty() { - MACOS_SEATBELT_READONLY_POLICY.to_string() - } else { - let scoped_write_policy = format!("(allow file-write*\n{}\n)", policies.join(" ")); - format!("{MACOS_SEATBELT_READONLY_POLICY}\n{scoped_write_policy}") + let (file_write_policy, extra_cli_args) = { + if sandbox_policy.has_full_disk_write_access() { + // Allegedly, this is more permissive than `(allow file-write*)`. + ( + r#"(allow file-write* (regex #"^/"))"#.to_string(), + Vec::::new(), + ) + } else { + let writable_roots = sandbox_policy.get_writable_roots(); + let (writable_folder_policies, cli_args): (Vec, Vec) = writable_roots + .iter() + .enumerate() + .map(|(index, root)| { + let param_name = format!("WRITABLE_ROOT_{index}"); + let policy: String = format!("(subpath (param \"{param_name}\"))"); + let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); + (policy, cli_arg) + }) + .unzip(); + if writable_folder_policies.is_empty() { + ("".to_string(), Vec::::new()) + } else { + let file_write_policy = format!( + "(allow file-write*\n{}\n)", + writable_folder_policies.join(" ") + ); + (file_write_policy, cli_args) + } + } }; + let file_read_policy = if sandbox_policy.has_full_disk_read_access() { + "; allow read-only file operations\n(allow file-read*)" + } else { + "" + }; + + // TODO(mbolin): apply_patch calls must also honor the SandboxPolicy. + let network_policy = if sandbox_policy.has_full_network_access() { + "(allow network-outbound)\n(allow network-inbound)\n(allow system-socket)" + } else { + "" + }; + + let full_policy = format!( + "{MACOS_SEATBELT_BASE_POLICY}\n{file_read_policy}\n{file_write_policy}\n{network_policy}" + ); let mut seatbelt_command: Vec = vec![ MACOS_PATH_TO_SEATBELT_EXECUTABLE.to_string(), "-p".to_string(), - full_policy.to_string(), + full_policy, ]; - seatbelt_command.extend(cli_args); + seatbelt_command.extend(extra_cli_args); seatbelt_command.push("--".to_string()); seatbelt_command.extend(command); seatbelt_command diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index e7d4e32a..389694a3 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -27,5 +27,3 @@ pub use codex::Codex; mod approval_mode_cli_arg; #[cfg(feature = "cli")] pub use approval_mode_cli_arg::ApprovalModeCliArg; -#[cfg(feature = "cli")] -pub use approval_mode_cli_arg::SandboxModeCliArg; diff --git a/codex-rs/core/src/linux.rs b/codex-rs/core/src/linux.rs index 9f9d44b0..fac3ab30 100644 --- a/codex-rs/core/src/linux.rs +++ b/codex-rs/core/src/linux.rs @@ -32,14 +32,13 @@ use tokio::sync::Notify; pub async fn exec_linux( params: ExecParams, - writable_roots: &[PathBuf], ctrl_c: Arc, - sandbox_policy: SandboxPolicy, + sandbox_policy: &SandboxPolicy, ) -> Result { // Allow READ on / // Allow WRITE on /dev/null let ctrl_c_copy = ctrl_c.clone(); - let writable_roots_copy = writable_roots.to_vec(); + let sandbox_policy = sandbox_policy.clone(); // Isolate thread to run the sandbox from let tool_call_output = std::thread::spawn(move || { @@ -49,14 +48,7 @@ pub async fn exec_linux( .expect("Failed to create runtime"); rt.block_on(async { - if sandbox_policy.is_network_restricted() { - install_network_seccomp_filter_on_current_thread()?; - } - - if sandbox_policy.is_file_write_restricted() { - install_filesystem_landlock_rules_on_current_thread(writable_roots_copy)?; - } - + apply_sandbox_policy_to_current_thread(sandbox_policy)?; exec(params, ctrl_c_copy).await }) }) @@ -72,15 +64,31 @@ pub async fn exec_linux( } } +/// Apply sandbox policies inside this thread so only the child inherits +/// them, not the entire CLI process. +pub fn apply_sandbox_policy_to_current_thread(sandbox_policy: SandboxPolicy) -> Result<()> { + if !sandbox_policy.has_full_network_access() { + install_network_seccomp_filter_on_current_thread()?; + } + + if !sandbox_policy.has_full_disk_write_access() { + let writable_roots = sandbox_policy.get_writable_roots(); + install_filesystem_landlock_rules_on_current_thread(writable_roots)?; + } + + // TODO(ragona): Add appropriate restrictions if + // `sandbox_policy.has_full_disk_read_access()` is `false`. + + Ok(()) +} + /// Installs Landlock file-system rules on the current thread allowing read /// access to the entire file-system while restricting write access to /// `/dev/null` and the provided list of `writable_roots`. /// /// # Errors /// Returns [`CodexErr::Sandbox`] variants when the ruleset fails to apply. -pub fn install_filesystem_landlock_rules_on_current_thread( - writable_roots: Vec, -) -> Result<()> { +fn install_filesystem_landlock_rules_on_current_thread(writable_roots: Vec) -> Result<()> { let abi = ABI::V5; let access_rw = AccessFs::from_all(abi); let access_ro = AccessFs::from_read(abi); @@ -108,7 +116,7 @@ pub fn install_filesystem_landlock_rules_on_current_thread( /// Installs a seccomp filter that blocks outbound network access except for /// AF_UNIX domain sockets. -pub fn install_network_seccomp_filter_on_current_thread() -> std::result::Result<(), SandboxErr> { +fn install_network_seccomp_filter_on_current_thread() -> std::result::Result<(), SandboxErr> { // Build rule map. let mut rules: BTreeMap> = BTreeMap::new(); @@ -184,15 +192,14 @@ mod tests_linux { workdir: None, timeout_ms: Some(timeout_ms), }; - let res = process_exec_tool_call( - params, - SandboxType::LinuxSeccomp, - writable_roots, - Arc::new(Notify::new()), - SandboxPolicy::NetworkAndFileWriteRestricted, - ) - .await - .unwrap(); + + let sandbox_policy = + SandboxPolicy::new_read_only_policy_with_writable_roots(writable_roots); + let ctrl_c = Arc::new(Notify::new()); + let res = + process_exec_tool_call(params, SandboxType::LinuxSeccomp, ctrl_c, &sandbox_policy) + .await + .unwrap(); if res.exit_code != 0 { println!("stdout:\n{}", res.stdout); @@ -261,14 +268,11 @@ mod tests_linux { timeout_ms: Some(2_000), }; - let result = process_exec_tool_call( - params, - SandboxType::LinuxSeccomp, - &[], - Arc::new(Notify::new()), - SandboxPolicy::NetworkRestricted, - ) - .await; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let ctrl_c = Arc::new(Notify::new()); + let result = + process_exec_tool_call(params, SandboxType::LinuxSeccomp, ctrl_c, &sandbox_policy) + .await; let (exit_code, stdout, stderr) = match result { Ok(output) => (output.exit_code, output.stdout, output.stderr), diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 139e2f2f..23c6e307 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -93,44 +93,169 @@ pub enum AskForApproval { } /// Determines execution restrictions for model shell commands -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] #[serde(rename_all = "kebab-case")] -pub enum SandboxPolicy { - /// Network syscalls will be blocked - NetworkRestricted, - /// Filesystem writes will be restricted - FileWriteRestricted, - /// Network and filesystem writes will be restricted - #[default] - NetworkAndFileWriteRestricted, - /// No restrictions; full "unsandboxed" mode - DangerousNoRestrictions, +pub struct SandboxPolicy { + permissions: Vec, +} + +impl From> for SandboxPolicy { + fn from(permissions: Vec) -> Self { + Self { permissions } + } } impl SandboxPolicy { - pub fn is_dangerous(&self) -> bool { - match self { - SandboxPolicy::NetworkRestricted => false, - SandboxPolicy::FileWriteRestricted => false, - SandboxPolicy::NetworkAndFileWriteRestricted => false, - SandboxPolicy::DangerousNoRestrictions => true, + pub fn new_read_only_policy() -> Self { + Self { + permissions: vec![SandboxPermission::DiskFullReadAccess], } } - pub fn is_network_restricted(&self) -> bool { - matches!( - self, - SandboxPolicy::NetworkRestricted | SandboxPolicy::NetworkAndFileWriteRestricted - ) + pub fn new_read_only_policy_with_writable_roots(writable_roots: &[PathBuf]) -> Self { + let mut permissions = Self::new_read_only_policy().permissions; + permissions.extend(writable_roots.iter().map(|folder| { + SandboxPermission::DiskWriteFolder { + folder: folder.clone(), + } + })); + Self { permissions } } - pub fn is_file_write_restricted(&self) -> bool { - matches!( - self, - SandboxPolicy::FileWriteRestricted | SandboxPolicy::NetworkAndFileWriteRestricted - ) + pub fn new_full_auto_policy() -> Self { + Self { + permissions: vec![ + SandboxPermission::DiskFullReadAccess, + SandboxPermission::DiskWritePlatformUserTempFolder, + SandboxPermission::DiskWriteCwd, + ], + } + } + + pub fn new_full_auto_policy_with_writable_roots(writable_roots: &[PathBuf]) -> Self { + let mut permissions = Self::new_full_auto_policy().permissions; + permissions.extend(writable_roots.iter().map(|folder| { + SandboxPermission::DiskWriteFolder { + folder: folder.clone(), + } + })); + Self { permissions } + } + + pub fn has_full_disk_read_access(&self) -> bool { + self.permissions + .iter() + .any(|perm| matches!(perm, SandboxPermission::DiskFullReadAccess)) + } + + pub fn has_full_disk_write_access(&self) -> bool { + self.permissions + .iter() + .any(|perm| matches!(perm, SandboxPermission::DiskFullWriteAccess)) + } + + pub fn has_full_network_access(&self) -> bool { + self.permissions + .iter() + .any(|perm| matches!(perm, SandboxPermission::NetworkFullAccess)) + } + + pub fn get_writable_roots(&self) -> Vec { + let mut writable_roots = Vec::::new(); + for perm in &self.permissions { + use SandboxPermission::*; + match perm { + DiskWritePlatformUserTempFolder => { + if cfg!(target_os = "macos") { + if let Some(tempdir) = std::env::var_os("TMPDIR") { + // Likely something that starts with /var/folders/... + let tmpdir_path = PathBuf::from(&tempdir); + if tmpdir_path.is_absolute() { + writable_roots.push(tmpdir_path.clone()); + match tmpdir_path.canonicalize() { + Ok(canonicalized) => { + // Likely something that starts with /private/var/folders/... + if canonicalized != tmpdir_path { + writable_roots.push(canonicalized); + } + } + Err(e) => { + tracing::error!("Failed to canonicalize TMPDIR: {e}"); + } + } + } else { + tracing::error!("TMPDIR is not an absolute path: {tempdir:?}"); + } + } + } + + // For Linux, should this be XDG_RUNTIME_DIR, /run/user/, or something else? + } + DiskWritePlatformGlobalTempFolder => { + if cfg!(unix) { + writable_roots.push(PathBuf::from("/tmp")); + } + } + DiskWriteCwd => match std::env::current_dir() { + Ok(cwd) => writable_roots.push(cwd), + Err(err) => { + tracing::error!("Failed to get current working directory: {err}"); + } + }, + DiskWriteFolder { folder } => { + writable_roots.push(folder.clone()); + } + DiskFullReadAccess | NetworkFullAccess => {} + DiskFullWriteAccess => { + // Currently, we expect callers to only invoke this method + // after verifying has_full_disk_write_access() is false. + } + } + } + writable_roots + } + + pub fn is_unrestricted(&self) -> bool { + self.has_full_disk_read_access() + && self.has_full_disk_write_access() + && self.has_full_network_access() } } + +/// Permissions that should be granted to the sandbox in which the agent +/// operates. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum SandboxPermission { + /// Is allowed to read all files on disk. + DiskFullReadAccess, + + /// Is allowed to write to the operating system's temp dir that + /// is restricted to the user the agent is running as. For + /// example, on macOS, this is generally something under + /// `/var/folders` as opposed to `/tmp`. + DiskWritePlatformUserTempFolder, + + /// Is allowed to write to the operating system's shared temp + /// dir. On UNIX, this is generally `/tmp`. + DiskWritePlatformGlobalTempFolder, + + /// Is allowed to write to the current working directory (in practice, this + /// is the `cwd` where `codex` was spawned). + DiskWriteCwd, + + /// Is allowed to the specified folder. `PathBuf` must be an + /// absolute path, though it is up to the caller to canonicalize + /// it if the path contains symlinks. + DiskWriteFolder { folder: PathBuf }, + + /// Is allowed to write to any file on disk. + DiskFullWriteAccess, + + /// Can make arbitrary network requests. + NetworkFullAccess, +} + /// User input #[non_exhaustive] #[derive(Debug, Clone, Deserialize, Serialize)] diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index e7841b2a..50ed3573 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -65,7 +65,7 @@ pub fn assess_patch_safety( pub fn assess_command_safety( command: &[String], approval_policy: AskForApproval, - sandbox_policy: SandboxPolicy, + sandbox_policy: &SandboxPolicy, approved: &HashSet>, ) -> SafetyCheck { let approve_without_sandbox = || SafetyCheck::AutoApprove { @@ -81,11 +81,10 @@ pub fn assess_command_safety( } // Command was not known-safe or allow-listed - match sandbox_policy { - // Only the dangerous sandbox policy will run arbitrary commands outside a sandbox - SandboxPolicy::DangerousNoRestrictions => approve_without_sandbox(), - // All other policies try to run the command in a sandbox if it is available - _ => match get_platform_sandbox() { + if sandbox_policy.is_unrestricted() { + approve_without_sandbox() + } else { + match get_platform_sandbox() { // We have a sandbox, so we can approve the command in all modes Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type }, None => { @@ -99,7 +98,7 @@ pub fn assess_command_safety( _ => SafetyCheck::AskUser, } } - }, + } } } diff --git a/codex-rs/core/src/seatbelt_readonly_policy.sbpl b/codex-rs/core/src/seatbelt_base_policy.sbpl similarity index 97% rename from codex-rs/core/src/seatbelt_readonly_policy.sbpl rename to codex-rs/core/src/seatbelt_base_policy.sbpl index c0632658..c9664651 100644 --- a/codex-rs/core/src/seatbelt_readonly_policy.sbpl +++ b/codex-rs/core/src/seatbelt_base_policy.sbpl @@ -6,9 +6,6 @@ ; start with closed-by-default (deny default) -; allow read-only file operations -(allow file-read*) - ; child processes inherit the policy of their parent (allow process-exec) (allow process-fork) diff --git a/codex-rs/core/tests/live_agent.rs b/codex-rs/core/tests/live_agent.rs index 23876498..7d2be33d 100644 --- a/codex-rs/core/tests/live_agent.rs +++ b/codex-rs/core/tests/live_agent.rs @@ -55,7 +55,7 @@ async fn spawn_codex() -> Codex { model: config.model, instructions: None, approval_policy: config.approval_policy, - sandbox_policy: SandboxPolicy::NetworkAndFileWriteRestricted, + sandbox_policy: SandboxPolicy::new_read_only_policy(), disable_response_storage: false, }, }) diff --git a/codex-rs/core/tests/previous_response_id.rs b/codex-rs/core/tests/previous_response_id.rs index 24c86916..c83d49ee 100644 --- a/codex-rs/core/tests/previous_response_id.rs +++ b/codex-rs/core/tests/previous_response_id.rs @@ -95,7 +95,7 @@ async fn keeps_previous_response_id_between_tasks() { model: config.model, instructions: None, approval_policy: config.approval_policy, - sandbox_policy: SandboxPolicy::NetworkAndFileWriteRestricted, + sandbox_policy: SandboxPolicy::new_read_only_policy(), disable_response_storage: false, }, }) diff --git a/codex-rs/core/tests/stream_no_completed.rs b/codex-rs/core/tests/stream_no_completed.rs index e696ea97..e64281e3 100644 --- a/codex-rs/core/tests/stream_no_completed.rs +++ b/codex-rs/core/tests/stream_no_completed.rs @@ -78,7 +78,7 @@ async fn retries_on_early_close() { model: config.model, instructions: None, approval_policy: config.approval_policy, - sandbox_policy: SandboxPolicy::NetworkAndFileWriteRestricted, + sandbox_policy: SandboxPolicy::new_read_only_policy(), disable_response_storage: false, }, }) diff --git a/codex-rs/exec/src/cli.rs b/codex-rs/exec/src/cli.rs index f5917a77..cd014e71 100644 --- a/codex-rs/exec/src/cli.rs +++ b/codex-rs/exec/src/cli.rs @@ -1,6 +1,5 @@ use clap::Parser; use clap::ValueEnum; -use codex_core::SandboxModeCliArg; use std::path::PathBuf; #[derive(Parser, Debug)] @@ -14,11 +13,9 @@ pub struct Cli { #[arg(long, short = 'm')] pub model: Option, - /// Configure the process restrictions when a command is executed. - /// - /// Uses OS-specific sandboxing tools; Seatbelt on OSX, landlock+seccomp on Linux. - #[arg(long = "sandbox", short = 's')] - pub sandbox_policy: Option, + /// Convenience alias for low-friction sandboxed automatic execution (network-disabled sandbox that can write to cwd and TMPDIR) + #[arg(long = "full-auto", default_value_t = false)] + pub full_auto: bool, /// Allow running Codex outside a Git repository. #[arg(long = "skip-git-repo-check", default_value_t = false)] diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 51e17267..9d5b9531 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -13,6 +13,7 @@ use codex_core::protocol::Event; use codex_core::protocol::EventMsg; use codex_core::protocol::InputItem; use codex_core::protocol::Op; +use codex_core::protocol::SandboxPolicy; use codex_core::util::is_inside_git_repo; use event_processor::EventProcessor; use owo_colors::OwoColorize; @@ -26,7 +27,7 @@ pub async fn run_main(cli: Cli) -> anyhow::Result<()> { let Cli { images, model, - sandbox_policy, + full_auto, skip_git_repo_check, disable_response_storage, color, @@ -61,13 +62,19 @@ pub async fn run_main(cli: Cli) -> anyhow::Result<()> { .with_writer(std::io::stderr) .try_init(); + let sandbox_policy = if full_auto { + Some(SandboxPolicy::new_full_auto_policy()) + } else { + None + }; + // Load configuration and determine approval policy let overrides = ConfigOverrides { model, // This CLI is intended to be headless and has no affordances for asking // the user for approval. approval_policy: Some(AskForApproval::Never), - sandbox_policy: sandbox_policy.map(Into::into), + sandbox_policy, disable_response_storage: if disable_response_storage { Some(true) } else { diff --git a/codex-rs/repl/src/cli.rs b/codex-rs/repl/src/cli.rs index a6b5bb73..567a8ea4 100644 --- a/codex-rs/repl/src/cli.rs +++ b/codex-rs/repl/src/cli.rs @@ -1,7 +1,6 @@ use clap::ArgAction; use clap::Parser; use codex_core::ApprovalModeCliArg; -use codex_core::SandboxModeCliArg; use std::path::PathBuf; /// Command‑line arguments. @@ -37,11 +36,9 @@ pub struct Cli { #[arg(long = "ask-for-approval", short = 'a')] pub approval_policy: Option, - /// Configure the process restrictions when a command is executed. - /// - /// Uses OS-specific sandboxing tools; Seatbelt on OSX, landlock+seccomp on Linux. - #[arg(long = "sandbox", short = 's')] - pub sandbox_policy: Option, + /// Convenience alias for low-friction sandboxed automatic execution (-a on-failure, network-disabled sandbox that can write to cwd and TMPDIR) + #[arg(long = "full-auto", default_value_t = false)] + pub full_auto: bool, /// Allow running Codex outside a Git repository. By default the CLI /// aborts early when the current working directory is **not** inside a diff --git a/codex-rs/repl/src/lib.rs b/codex-rs/repl/src/lib.rs index 17586332..d4bfbc2f 100644 --- a/codex-rs/repl/src/lib.rs +++ b/codex-rs/repl/src/lib.rs @@ -6,7 +6,9 @@ use std::sync::Arc; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::protocol; +use codex_core::protocol::AskForApproval; use codex_core::protocol::FileChange; +use codex_core::protocol::SandboxPolicy; use codex_core::util::is_inside_git_repo; use codex_core::util::notify_on_sigint; use codex_core::Codex; @@ -76,11 +78,20 @@ pub async fn run_main(cli: Cli) -> anyhow::Result<()> { // Initialize logging before any other work so early errors are captured. init_logger(cli.verbose, !cli.no_ansi); + let (sandbox_policy, approval_policy) = if cli.full_auto { + ( + Some(SandboxPolicy::new_full_auto_policy()), + Some(AskForApproval::OnFailure), + ) + } else { + (None, cli.approval_policy.map(Into::into)) + }; + // Load config file and apply CLI overrides (model & approval policy) let overrides = ConfigOverrides { model: cli.model.clone(), - approval_policy: cli.approval_policy.map(Into::into), - sandbox_policy: cli.sandbox_policy.map(Into::into), + approval_policy, + sandbox_policy, disable_response_storage: if cli.disable_response_storage { Some(true) } else { diff --git a/codex-rs/tui/src/cli.rs b/codex-rs/tui/src/cli.rs index f336b0c3..1c00ae08 100644 --- a/codex-rs/tui/src/cli.rs +++ b/codex-rs/tui/src/cli.rs @@ -1,6 +1,5 @@ use clap::Parser; use codex_core::ApprovalModeCliArg; -use codex_core::SandboxModeCliArg; use std::path::PathBuf; #[derive(Parser, Debug)] @@ -21,11 +20,9 @@ pub struct Cli { #[arg(long = "ask-for-approval", short = 'a')] pub approval_policy: Option, - /// Configure the process restrictions when a command is executed. - /// - /// Uses OS-specific sandboxing tools; Seatbelt on OSX, landlock+seccomp on Linux. - #[arg(long = "sandbox", short = 's')] - pub sandbox_policy: Option, + /// Convenience alias for low-friction sandboxed automatic execution (-a on-failure, network-disabled sandbox that can write to cwd and TMPDIR) + #[arg(long = "full-auto", default_value_t = false)] + pub full_auto: bool, /// Allow running Codex outside a Git repository. #[arg(long = "skip-git-repo-check", default_value_t = false)] @@ -34,12 +31,4 @@ pub struct Cli { /// Disable server‑side response storage (sends the full conversation context with every request) #[arg(long = "disable-response-storage", default_value_t = false)] pub disable_response_storage: bool, - - /// Convenience alias for low-friction sandboxed automatic execution (-a on-failure, -s network-and-file-write-restricted) - #[arg(long = "full-auto", default_value_t = true)] - pub full_auto: bool, - - /// Convenience alias for supervised sandboxed execution (-a unless-allow-listed, -s network-and-file-write-restricted) - #[arg(long = "suggest", default_value_t = false)] - pub suggest: bool, } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index bf4ebec4..db43bde6 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -6,6 +6,8 @@ use app::App; use codex_core::config::Config; use codex_core::config::ConfigOverrides; +use codex_core::protocol::AskForApproval; +use codex_core::protocol::SandboxPolicy; use codex_core::util::is_inside_git_repo; use log_layer::TuiLogLayer; use std::fs::OpenOptions; @@ -33,12 +35,21 @@ pub use cli::Cli; pub fn run_main(cli: Cli) -> std::io::Result<()> { assert_env_var_set(); + let (sandbox_policy, approval_policy) = if cli.full_auto { + ( + Some(SandboxPolicy::new_full_auto_policy()), + Some(AskForApproval::OnFailure), + ) + } else { + (None, cli.approval_policy.map(Into::into)) + }; + let config = { // Load configuration and support CLI overrides. let overrides = ConfigOverrides { model: cli.model.clone(), - approval_policy: cli.approval_policy.map(Into::into), - sandbox_policy: cli.sandbox_policy.map(Into::into), + approval_policy, + sandbox_policy, disable_response_storage: if cli.disable_response_storage { Some(true) } else {