From 056c8f82792d58c0305902e9dbc27c80b4fc9e32 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 19 Nov 2025 13:55:29 -0800 Subject: [PATCH] fix: prepare ExecPolicy in exec-server for execpolicy2 cutover (#6888) This PR introduces an extra layer of abstraction to prepare us for the migration to execpolicy2: - introduces a new trait, `EscalationPolicy`, whose `determine_action()` method is responsible for producing the `EscalateAction` - the existing `ExecPolicy` typedef is changed to return an intermediate `ExecPolicyOutcome` instead of `EscalateAction` - the default implementation of `EscalationPolicy`, `McpEscalationPolicy`, composes `ExecPolicy` - the `ExecPolicyOutcome` includes `codex_execpolicy2::Decision`, which has a `Prompt` variant - when `McpEscalationPolicy` gets `Decision::Prompt` back from `ExecPolicy`, it prompts the user via an MCP elicitation and maps the result into an `ElicitationAction` - now that the end user can reply to an elicitation with `Decline` or `Cancel`, we introduce a new variant, `EscalateAction::Deny`, which the client handles by returning exit code `1` without running anything Note the way the elicitation is created is still not quite right, but I will fix that once we have things running end-to-end for real in a follow-up PR. --- codex-rs/Cargo.lock | 2 + codex-rs/exec-server/Cargo.toml | 2 + codex-rs/exec-server/src/posix.rs | 74 +++++++++-- .../exec-server/src/posix/escalate_client.rs | 7 ++ .../src/posix/escalate_protocol.rs | 2 + .../exec-server/src/posix/escalate_server.rs | 71 ++++++++--- .../src/posix/escalation_policy.rs | 14 +++ codex-rs/exec-server/src/posix/mcp.rs | 33 ++--- .../src/posix/mcp_escalation_policy.rs | 117 ++++++++++++++++++ 9 files changed, 266 insertions(+), 56 deletions(-) create mode 100644 codex-rs/exec-server/src/posix/escalation_policy.rs create mode 100644 codex-rs/exec-server/src/posix/mcp_escalation_policy.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 6ace22c76..66a1c208e 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1188,6 +1188,7 @@ name = "codex-exec-server" version = "0.0.0" dependencies = [ "anyhow", + "async-trait", "clap", "codex-core", "libc", @@ -1196,6 +1197,7 @@ dependencies = [ "rmcp", "serde", "serde_json", + "shlex", "socket2 0.6.0", "tempfile", "tokio", diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index 24ee460f5..60e0facff 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -12,6 +12,7 @@ workspace = true [dependencies] anyhow = { workspace = true } +async-trait = { workspace = true } clap = { workspace = true, features = ["derive"] } codex-core = { workspace = true } libc = { workspace = true } @@ -31,6 +32,7 @@ rmcp = { workspace = true, default-features = false, features = [ ] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +shlex = { workspace = true } socket2 = { workspace = true } tokio = { workspace = true, features = [ "io-std", diff --git a/codex-rs/exec-server/src/posix.rs b/codex-rs/exec-server/src/posix.rs index 179fc7a6f..bbb624df0 100644 --- a/codex-rs/exec-server/src/posix.rs +++ b/codex-rs/exec-server/src/posix.rs @@ -64,25 +64,17 @@ use tracing_subscriber::{self}; use crate::posix::escalate_protocol::EscalateAction; use crate::posix::escalate_server::EscalateServer; +use crate::posix::escalation_policy::EscalationPolicy; +use crate::posix::mcp_escalation_policy::ExecPolicyOutcome; mod escalate_client; mod escalate_protocol; mod escalate_server; +mod escalation_policy; mod mcp; +mod mcp_escalation_policy; mod socket; -fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> EscalateAction { - // TODO: execpolicy - if file == Path::new("/opt/homebrew/bin/gh") - && let [_, arg1, arg2, ..] = argv - && arg1 == "issue" - && arg2 == "list" - { - return EscalateAction::Escalate; - } - EscalateAction::Run -} - #[derive(Parser)] #[command(version)] pub struct Cli { @@ -135,7 +127,7 @@ pub async fn main() -> anyhow::Result<()> { } Some(Commands::ShellExec(args)) => { let bash_path = mcp::get_bash_path()?; - let escalate_server = EscalateServer::new(bash_path, dummy_exec_policy); + let escalate_server = EscalateServer::new(bash_path, DummyEscalationPolicy {}); let result = escalate_server .exec( args.command.clone(), @@ -162,3 +154,59 @@ pub async fn main() -> anyhow::Result<()> { } } } + +// TODO: replace with execpolicy2 + +struct DummyEscalationPolicy; + +#[async_trait::async_trait] +impl EscalationPolicy for DummyEscalationPolicy { + async fn determine_action( + &self, + file: &Path, + argv: &[String], + workdir: &Path, + ) -> Result { + let outcome = dummy_exec_policy(file, argv, workdir); + let action = match outcome { + ExecPolicyOutcome::Allow { + run_with_escalated_permissions, + } => { + if run_with_escalated_permissions { + EscalateAction::Escalate + } else { + EscalateAction::Run + } + } + ExecPolicyOutcome::Forbidden => EscalateAction::Deny { + reason: Some("Execution forbidden by policy".to_string()), + }, + ExecPolicyOutcome::Prompt { .. } => EscalateAction::Deny { + reason: Some("Could not prompt user for permission".to_string()), + }, + }; + Ok(action) + } +} + +fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> ExecPolicyOutcome { + if file.ends_with("/rm") { + ExecPolicyOutcome::Forbidden + } else if file.ends_with("/git") { + ExecPolicyOutcome::Prompt { + run_with_escalated_permissions: false, + } + } else if file == Path::new("/opt/homebrew/bin/gh") + && let [_, arg1, arg2, ..] = argv + && arg1 == "issue" + && arg2 == "list" + { + ExecPolicyOutcome::Allow { + run_with_escalated_permissions: true, + } + } else { + ExecPolicyOutcome::Allow { + run_with_escalated_permissions: false, + } + } +} diff --git a/codex-rs/exec-server/src/posix/escalate_client.rs b/codex-rs/exec-server/src/posix/escalate_client.rs index 2add1dade..bea4b6fa5 100644 --- a/codex-rs/exec-server/src/posix/escalate_client.rs +++ b/codex-rs/exec-server/src/posix/escalate_client.rs @@ -98,5 +98,12 @@ pub(crate) async fn run(file: String, argv: Vec) -> anyhow::Result Err(err.into()) } + EscalateAction::Deny { reason } => { + match reason { + Some(reason) => eprintln!("Execution denied: {reason}"), + None => eprintln!("Execution denied"), + } + Ok(1) + } } } diff --git a/codex-rs/exec-server/src/posix/escalate_protocol.rs b/codex-rs/exec-server/src/posix/escalate_protocol.rs index 09d33fc98..e3fc27d07 100644 --- a/codex-rs/exec-server/src/posix/escalate_protocol.rs +++ b/codex-rs/exec-server/src/posix/escalate_protocol.rs @@ -34,6 +34,8 @@ pub(super) enum EscalateAction { Run, /// The command should be escalated to the server for execution. Escalate, + /// The command should not be executed. + Deny { reason: Option }, } /// The client sends this to the server to forward its open FDs. diff --git a/codex-rs/exec-server/src/posix/escalate_server.rs b/codex-rs/exec-server/src/posix/escalate_server.rs index 6d058ff0f..4f259f050 100644 --- a/codex-rs/exec-server/src/posix/escalate_server.rs +++ b/codex-rs/exec-server/src/posix/escalate_server.rs @@ -1,8 +1,8 @@ use std::collections::HashMap; use std::os::fd::AsRawFd; -use std::path::Path; use std::path::PathBuf; use std::process::Stdio; +use std::sync::Arc; use std::time::Duration; use anyhow::Context as _; @@ -21,25 +21,24 @@ use crate::posix::escalate_protocol::EscalateRequest; use crate::posix::escalate_protocol::EscalateResponse; use crate::posix::escalate_protocol::SuperExecMessage; use crate::posix::escalate_protocol::SuperExecResult; +use crate::posix::escalation_policy::EscalationPolicy; use crate::posix::socket::AsyncDatagramSocket; use crate::posix::socket::AsyncSocket; -/// This is the policy which decides how to handle an exec() call. -/// -/// `file` is the absolute, canonical path to the executable to run, i.e. the first arg to exec. -/// `argv` is the argv, including the program name (`argv[0]`). -/// `workdir` is the absolute, canonical path to the working directory in which to execute the -/// command. -pub(crate) type ExecPolicy = fn(file: &Path, argv: &[String], workdir: &Path) -> EscalateAction; - pub(crate) struct EscalateServer { bash_path: PathBuf, - policy: ExecPolicy, + policy: Arc, } impl EscalateServer { - pub fn new(bash_path: PathBuf, policy: ExecPolicy) -> Self { - Self { bash_path, policy } + pub fn new

(bash_path: PathBuf, policy: P) -> Self + where + P: EscalationPolicy + Send + Sync + 'static, + { + Self { + bash_path, + policy: Arc::new(policy), + } } pub async fn exec( @@ -53,7 +52,7 @@ impl EscalateServer { let client_socket = escalate_client.into_inner(); client_socket.set_cloexec(false)?; - let escalate_task = tokio::spawn(escalate_task(escalate_server, self.policy)); + let escalate_task = tokio::spawn(escalate_task(escalate_server, self.policy.clone())); let mut env = env.clone(); env.insert( ESCALATE_SOCKET_ENV_VAR.to_string(), @@ -96,7 +95,10 @@ impl EscalateServer { } } -async fn escalate_task(socket: AsyncDatagramSocket, policy: ExecPolicy) -> anyhow::Result<()> { +async fn escalate_task( + socket: AsyncDatagramSocket, + policy: Arc, +) -> anyhow::Result<()> { loop { let (_, mut fds) = socket.receive_with_fds().await?; if fds.len() != 1 { @@ -104,6 +106,7 @@ async fn escalate_task(socket: AsyncDatagramSocket, policy: ExecPolicy) -> anyho continue; } let stream_socket = AsyncSocket::from_fd(fds.remove(0))?; + let policy = policy.clone(); tokio::spawn(async move { if let Err(err) = handle_escalate_session_with_policy(stream_socket, policy).await { tracing::error!("escalate session failed: {err:?}"); @@ -122,7 +125,7 @@ pub(crate) struct ExecResult { async fn handle_escalate_session_with_policy( socket: AsyncSocket, - policy: ExecPolicy, + policy: Arc, ) -> anyhow::Result<()> { let EscalateRequest { file, @@ -132,8 +135,12 @@ async fn handle_escalate_session_with_policy( } = socket.receive::().await?; let file = PathBuf::from(&file).absolutize()?.into_owned(); let workdir = PathBuf::from(&workdir).absolutize()?.into_owned(); - let action = policy(file.as_path(), &argv, &workdir); + let action = policy + .determine_action(file.as_path(), &argv, &workdir) + .await?; + tracing::debug!("decided {action:?} for {file:?} {argv:?} {workdir:?}"); + match action { EscalateAction::Run => { socket @@ -195,6 +202,13 @@ async fn handle_escalate_session_with_policy( }) .await?; } + EscalateAction::Deny { reason } => { + socket + .send(EscalateResponse { + action: EscalateAction::Deny { reason }, + }) + .await?; + } } Ok(()) } @@ -204,14 +218,33 @@ mod tests { use super::*; use pretty_assertions::assert_eq; use std::collections::HashMap; + use std::path::Path; use std::path::PathBuf; + struct DeterministicEscalationPolicy { + action: EscalateAction, + } + + #[async_trait::async_trait] + impl EscalationPolicy for DeterministicEscalationPolicy { + async fn determine_action( + &self, + _file: &Path, + _argv: &[String], + _workdir: &Path, + ) -> Result { + Ok(self.action.clone()) + } + } + #[tokio::test] async fn handle_escalate_session_respects_run_in_sandbox_decision() -> anyhow::Result<()> { let (server, client) = AsyncSocket::pair()?; let server_task = tokio::spawn(handle_escalate_session_with_policy( server, - |_file, _argv, _workdir| EscalateAction::Run, + Arc::new(DeterministicEscalationPolicy { + action: EscalateAction::Run, + }), )); client @@ -238,7 +271,9 @@ mod tests { let (server, client) = AsyncSocket::pair()?; let server_task = tokio::spawn(handle_escalate_session_with_policy( server, - |_file, _argv, _workdir| EscalateAction::Escalate, + Arc::new(DeterministicEscalationPolicy { + action: EscalateAction::Escalate, + }), )); client diff --git a/codex-rs/exec-server/src/posix/escalation_policy.rs b/codex-rs/exec-server/src/posix/escalation_policy.rs new file mode 100644 index 000000000..a7fcc4f62 --- /dev/null +++ b/codex-rs/exec-server/src/posix/escalation_policy.rs @@ -0,0 +1,14 @@ +use std::path::Path; + +use crate::posix::escalate_protocol::EscalateAction; + +/// Decides what action to take in response to an execve request from a client. +#[async_trait::async_trait] +pub(crate) trait EscalationPolicy: Send + Sync { + async fn determine_action( + &self, + file: &Path, + argv: &[String], + workdir: &Path, + ) -> Result; +} diff --git a/codex-rs/exec-server/src/posix/mcp.rs b/codex-rs/exec-server/src/posix/mcp.rs index f4e2b19d4..2a6e84dd9 100644 --- a/codex-rs/exec-server/src/posix/mcp.rs +++ b/codex-rs/exec-server/src/posix/mcp.rs @@ -18,9 +18,10 @@ use rmcp::tool_handler; use rmcp::tool_router; use rmcp::transport::stdio; -use crate::posix::escalate_server; use crate::posix::escalate_server::EscalateServer; -use crate::posix::escalate_server::ExecPolicy; +use crate::posix::escalate_server::{self}; +use crate::posix::mcp_escalation_policy::ExecPolicy; +use crate::posix::mcp_escalation_policy::McpEscalationPolicy; /// Path to our patched bash. const CODEX_BASH_PATH_ENV_VAR: &str = "CODEX_BASH_PATH"; @@ -81,10 +82,13 @@ impl ExecTool { #[tool] async fn shell( &self, - _context: RequestContext, + context: RequestContext, Parameters(params): Parameters, ) -> Result { - let escalate_server = EscalateServer::new(self.bash_path.clone(), self.policy); + let escalate_server = EscalateServer::new( + self.bash_path.clone(), + McpEscalationPolicy::new(self.policy, context), + ); let result = escalate_server .exec( params.command, @@ -99,27 +103,6 @@ impl ExecTool { ExecResult::from(result), )?])) } - - #[allow(dead_code)] - async fn prompt( - &self, - command: String, - workdir: String, - context: RequestContext, - ) -> Result { - context - .peer - .create_elicitation(CreateElicitationRequestParam { - message: format!("Allow Codex to run `{command:?}` in `{workdir:?}`?"), - #[allow(clippy::expect_used)] - requested_schema: ElicitationSchema::builder() - .property("dummy", PrimitiveSchema::String(StringSchema::new())) - .build() - .expect("failed to build elicitation schema"), - }) - .await - .map_err(|e| McpError::internal_error(e.to_string(), None)) - } } #[tool_handler] diff --git a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs new file mode 100644 index 000000000..24f42fe91 --- /dev/null +++ b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs @@ -0,0 +1,117 @@ +use std::path::Path; + +use rmcp::ErrorData as McpError; +use rmcp::RoleServer; +use rmcp::model::CreateElicitationRequestParam; +use rmcp::model::CreateElicitationResult; +use rmcp::model::ElicitationAction; +use rmcp::model::ElicitationSchema; +use rmcp::model::PrimitiveSchema; +use rmcp::model::StringSchema; +use rmcp::service::RequestContext; + +use crate::posix::escalate_protocol::EscalateAction; +use crate::posix::escalation_policy::EscalationPolicy; + +/// This is the policy which decides how to handle an exec() call. +/// +/// `file` is the absolute, canonical path to the executable to run, i.e. the first arg to exec. +/// `argv` is the argv, including the program name (`argv[0]`). +/// `workdir` is the absolute, canonical path to the working directory in which to execute the +/// command. +pub(crate) type ExecPolicy = fn(file: &Path, argv: &[String], workdir: &Path) -> ExecPolicyOutcome; + +pub(crate) enum ExecPolicyOutcome { + Allow { + run_with_escalated_permissions: bool, + }, + Prompt { + run_with_escalated_permissions: bool, + }, + Forbidden, +} + +/// ExecPolicy with access to the MCP RequestContext so that it can leverage +/// elicitations. +pub(crate) struct McpEscalationPolicy { + policy: ExecPolicy, + context: RequestContext, +} + +impl McpEscalationPolicy { + pub(crate) fn new(policy: ExecPolicy, context: RequestContext) -> Self { + Self { policy, context } + } + + async fn prompt( + &self, + _file: &Path, + argv: &[String], + workdir: &Path, + context: RequestContext, + ) -> Result { + let command = shlex::try_join(argv.iter().map(String::as_str)).unwrap_or_default(); + context + .peer + .create_elicitation(CreateElicitationRequestParam { + message: format!("Allow Codex to run `{command:?}` in `{workdir:?}`?"), + #[allow(clippy::expect_used)] + requested_schema: ElicitationSchema::builder() + .property("dummy", PrimitiveSchema::String(StringSchema::new())) + .build() + .expect("failed to build elicitation schema"), + }) + .await + .map_err(|e| McpError::internal_error(e.to_string(), None)) + } +} + +#[async_trait::async_trait] +impl EscalationPolicy for McpEscalationPolicy { + async fn determine_action( + &self, + file: &Path, + argv: &[String], + workdir: &Path, + ) -> Result { + let outcome = (self.policy)(file, argv, workdir); + let action = match outcome { + ExecPolicyOutcome::Allow { + run_with_escalated_permissions, + } => { + if run_with_escalated_permissions { + EscalateAction::Escalate + } else { + EscalateAction::Run + } + } + ExecPolicyOutcome::Prompt { + run_with_escalated_permissions, + } => { + let result = self + .prompt(file, argv, workdir, self.context.clone()) + .await?; + // TODO: Extract reason from `result.content`. + match result.action { + ElicitationAction::Accept => { + if run_with_escalated_permissions { + EscalateAction::Escalate + } else { + EscalateAction::Run + } + } + ElicitationAction::Decline => EscalateAction::Deny { + reason: Some("User declined execution".to_string()), + }, + ElicitationAction::Cancel => EscalateAction::Deny { + reason: Some("User cancelled execution".to_string()), + }, + } + } + ExecPolicyOutcome::Forbidden => EscalateAction::Deny { + reason: Some("Execution forbidden by policy".to_string()), + }, + }; + Ok(action) + } +}