mirror of
https://github.com/openai/codex.git
synced 2026-02-01 22:47:52 +00:00
chore: refactor exec-server to prepare it for standalone MCP use (#6944)
This PR reorganizes things slightly so that:
- Instead of a single multitool executable, `codex-exec-server`, we now
have two executables:
- `codex-exec-mcp-server` to launch the MCP server
- `codex-execve-wrapper` is the `execve(2)` wrapper to use with the
`BASH_EXEC_WRAPPER` environment variable
- `BASH_EXEC_WRAPPER` must be a single executable: it cannot be a
command string composed of an executable with args (i.e., it no longer
adds the `escalate` subcommand, as before)
- `codex-exec-mcp-server` takes `--bash` and `--execve` as options.
Though if `--execve` is not specified, the MCP server will check the
directory containing `std::env::current_exe()` and attempt to use the
file named `codex-execve-wrapper` within it. In development, this works
out since these executables are side-by-side in the `target/debug`
folder.
With respect to testing, this also fixes an important bug in
`dummy_exec_policy()`, as I was using `ends_with()` as if it applied to
a `String`, but in this case, it is used with a `&Path`, so the
semantics are slightly different.
Putting this all together, I was able to test this by running the
following:
```
~/code/codex/codex-rs$ npx @modelcontextprotocol/inspector \
./target/debug/codex-exec-mcp-server --bash ~/code/bash/bash
```
If I try to run `git status` in `/Users/mbolin/code/codex` via the
`shell` tool from the MCP server:
<img width="1589" height="1335" alt="image"
src="https://github.com/user-attachments/assets/9db6aea8-7fbc-4675-8b1f-ec446685d6c4"
/>
then I get prompted with the following elicitation, as expected:
<img width="1589" height="1335" alt="image"
src="https://github.com/user-attachments/assets/21b68fe0-494d-4562-9bad-0ddc55fc846d"
/>
Though a current limitation is that the `shell` tool defaults to a
timeout of 10s, which means I only have 10s to respond to the
elicitation. Ideally, the time spent waiting for a response from a human
should not count against the timeout for the command execution. I will
address this in a subsequent PR.
---
Note `~/code/bash/bash` was created by doing:
```
cd ~/code
git clone https://github.com/bminor/bash
cd bash
git checkout a8a1c2fac029404d3f42cd39f5a20f24b6e4fe4b
<apply the patch below>
./configure
make
```
The patch:
```
diff --git a/execute_cmd.c b/execute_cmd.c
index 070f5119..d20ad2b9 100644
--- a/execute_cmd.c
+++ b/execute_cmd.c
@@ -6129,6 +6129,19 @@ shell_execve (char *command, char **args, char **env)
char sample[HASH_BANG_BUFSIZ];
size_t larray;
+ char* exec_wrapper = getenv("BASH_EXEC_WRAPPER");
+ if (exec_wrapper && *exec_wrapper && !whitespace (*exec_wrapper))
+ {
+ char *orig_command = command;
+
+ larray = strvec_len (args);
+
+ memmove (args + 2, args, (++larray) * sizeof (char *));
+ args[0] = exec_wrapper;
+ args[1] = orig_command;
+ command = exec_wrapper;
+ }
+
```
This commit is contained in:
@@ -4,8 +4,16 @@ name = "codex-exec-server"
|
||||
version = { workspace = true }
|
||||
|
||||
[[bin]]
|
||||
name = "codex-exec-server"
|
||||
path = "src/main.rs"
|
||||
name = "codex-execve-wrapper"
|
||||
path = "src/bin/main_execve_wrapper.rs"
|
||||
|
||||
[[bin]]
|
||||
name = "codex-exec-mcp-server"
|
||||
path = "src/bin/main_mcp_server.rs"
|
||||
|
||||
[lib]
|
||||
name = "codex_exec_server"
|
||||
path = "src/lib.rs"
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
8
codex-rs/exec-server/src/bin/main_execve_wrapper.rs
Normal file
8
codex-rs/exec-server/src/bin/main_execve_wrapper.rs
Normal file
@@ -0,0 +1,8 @@
|
||||
#[cfg(not(unix))]
|
||||
fn main() {
|
||||
eprintln!("codex-execve-wrapper is only implemented for UNIX");
|
||||
std::process::exit(1);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
pub use codex_exec_server::main_execve_wrapper as main;
|
||||
8
codex-rs/exec-server/src/bin/main_mcp_server.rs
Normal file
8
codex-rs/exec-server/src/bin/main_mcp_server.rs
Normal file
@@ -0,0 +1,8 @@
|
||||
#[cfg(not(unix))]
|
||||
fn main() {
|
||||
eprintln!("codex-exec-mcp-server is only implemented for UNIX");
|
||||
std::process::exit(1);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
pub use codex_exec_server::main_mcp_server as main;
|
||||
8
codex-rs/exec-server/src/lib.rs
Normal file
8
codex-rs/exec-server/src/lib.rs
Normal file
@@ -0,0 +1,8 @@
|
||||
#[cfg(unix)]
|
||||
mod posix;
|
||||
|
||||
#[cfg(unix)]
|
||||
pub use posix::main_execve_wrapper;
|
||||
|
||||
#[cfg(unix)]
|
||||
pub use posix::main_mcp_server;
|
||||
@@ -1,11 +0,0 @@
|
||||
#[cfg(target_os = "windows")]
|
||||
fn main() {
|
||||
eprintln!("codex-exec-server is not implemented on Windows targets");
|
||||
std::process::exit(1);
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
mod posix;
|
||||
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
pub use posix::main;
|
||||
@@ -56,15 +56,12 @@
|
||||
//! o<-----x
|
||||
//!
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use clap::Parser;
|
||||
use clap::Subcommand;
|
||||
use tracing_subscriber::EnvFilter;
|
||||
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;
|
||||
@@ -75,124 +72,84 @@ mod mcp;
|
||||
mod mcp_escalation_policy;
|
||||
mod socket;
|
||||
|
||||
/// Default value of --execve option relative to the current executable.
|
||||
/// Note this must match the name of the binary as specified in Cargo.toml.
|
||||
const CODEX_EXECVE_WRAPPER_EXE_NAME: &str = "codex-execve-wrapper";
|
||||
|
||||
#[derive(Parser)]
|
||||
#[command(version)]
|
||||
pub struct Cli {
|
||||
#[command(subcommand)]
|
||||
subcommand: Option<Commands>,
|
||||
}
|
||||
struct McpServerCli {
|
||||
/// Executable to delegate execve(2) calls to in Bash.
|
||||
#[arg(long = "execve")]
|
||||
execve_wrapper: Option<PathBuf>,
|
||||
|
||||
#[derive(Subcommand)]
|
||||
enum Commands {
|
||||
Escalate(EscalateArgs),
|
||||
ShellExec(ShellExecArgs),
|
||||
}
|
||||
|
||||
/// Invoked from within the sandbox to (potentially) escalate permissions.
|
||||
#[derive(Parser, Debug)]
|
||||
struct EscalateArgs {
|
||||
file: String,
|
||||
|
||||
#[arg(trailing_var_arg = true)]
|
||||
argv: Vec<String>,
|
||||
}
|
||||
|
||||
impl EscalateArgs {
|
||||
/// This is the escalate client. It talks to the escalate server to determine whether to exec()
|
||||
/// the command directly or to proxy to the escalate server.
|
||||
async fn run(self) -> anyhow::Result<i32> {
|
||||
let EscalateArgs { file, argv } = self;
|
||||
escalate_client::run(file, argv).await
|
||||
}
|
||||
}
|
||||
|
||||
/// Debugging command to emulate an MCP "shell" tool call.
|
||||
#[derive(Parser, Debug)]
|
||||
struct ShellExecArgs {
|
||||
command: String,
|
||||
/// Path to Bash that has been patched to support execve() wrapping.
|
||||
#[arg(long = "bash")]
|
||||
bash_path: Option<PathBuf>,
|
||||
}
|
||||
|
||||
#[tokio::main]
|
||||
pub async fn main() -> anyhow::Result<()> {
|
||||
let cli = Cli::parse();
|
||||
pub async fn main_mcp_server() -> anyhow::Result<()> {
|
||||
tracing_subscriber::fmt()
|
||||
.with_env_filter(EnvFilter::from_default_env())
|
||||
.with_writer(std::io::stderr)
|
||||
.with_ansi(false)
|
||||
.init();
|
||||
|
||||
match cli.subcommand {
|
||||
Some(Commands::Escalate(args)) => {
|
||||
std::process::exit(args.run().await?);
|
||||
}
|
||||
Some(Commands::ShellExec(args)) => {
|
||||
let bash_path = mcp::get_bash_path()?;
|
||||
let escalate_server = EscalateServer::new(bash_path, DummyEscalationPolicy {});
|
||||
let result = escalate_server
|
||||
.exec(
|
||||
args.command.clone(),
|
||||
std::env::vars().collect(),
|
||||
std::env::current_dir()?,
|
||||
None,
|
||||
)
|
||||
.await?;
|
||||
println!("{result:?}");
|
||||
std::process::exit(result.exit_code);
|
||||
}
|
||||
let cli = McpServerCli::parse();
|
||||
let execve_wrapper = match cli.execve_wrapper {
|
||||
Some(path) => path,
|
||||
None => {
|
||||
let bash_path = mcp::get_bash_path()?;
|
||||
|
||||
tracing::info!("Starting MCP server");
|
||||
let service = mcp::serve(bash_path, dummy_exec_policy)
|
||||
.await
|
||||
.inspect_err(|e| {
|
||||
tracing::error!("serving error: {:?}", e);
|
||||
})?;
|
||||
|
||||
service.waiting().await?;
|
||||
Ok(())
|
||||
let cwd = std::env::current_exe()?;
|
||||
cwd.parent()
|
||||
.map(|p| p.join(CODEX_EXECVE_WRAPPER_EXE_NAME))
|
||||
.ok_or_else(|| {
|
||||
anyhow::anyhow!("failed to determine execve wrapper path from current exe")
|
||||
})?
|
||||
}
|
||||
}
|
||||
};
|
||||
let bash_path = match cli.bash_path {
|
||||
Some(path) => path,
|
||||
None => mcp::get_bash_path()?,
|
||||
};
|
||||
|
||||
tracing::info!("Starting MCP server");
|
||||
let service = mcp::serve(bash_path, execve_wrapper, dummy_exec_policy)
|
||||
.await
|
||||
.inspect_err(|e| {
|
||||
tracing::error!("serving error: {:?}", e);
|
||||
})?;
|
||||
|
||||
service.waiting().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[derive(Parser)]
|
||||
pub struct ExecveWrapperCli {
|
||||
file: String,
|
||||
|
||||
#[arg(trailing_var_arg = true)]
|
||||
argv: Vec<String>,
|
||||
}
|
||||
|
||||
#[tokio::main]
|
||||
pub async fn main_execve_wrapper() -> anyhow::Result<()> {
|
||||
tracing_subscriber::fmt()
|
||||
.with_env_filter(EnvFilter::from_default_env())
|
||||
.with_writer(std::io::stderr)
|
||||
.with_ansi(false)
|
||||
.init();
|
||||
|
||||
let ExecveWrapperCli { file, argv } = ExecveWrapperCli::parse();
|
||||
let exit_code = escalate_client::run(file, argv).await?;
|
||||
std::process::exit(exit_code);
|
||||
}
|
||||
|
||||
// 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<EscalateAction, rmcp::ErrorData> {
|
||||
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") {
|
||||
if file.ends_with("rm") {
|
||||
ExecPolicyOutcome::Forbidden
|
||||
} else if file.ends_with("/git") {
|
||||
} else if file.ends_with("git") {
|
||||
ExecPolicyOutcome::Prompt {
|
||||
run_with_escalated_permissions: false,
|
||||
}
|
||||
|
||||
@@ -27,16 +27,18 @@ use crate::posix::socket::AsyncSocket;
|
||||
|
||||
pub(crate) struct EscalateServer {
|
||||
bash_path: PathBuf,
|
||||
execve_wrapper: PathBuf,
|
||||
policy: Arc<dyn EscalationPolicy>,
|
||||
}
|
||||
|
||||
impl EscalateServer {
|
||||
pub fn new<P>(bash_path: PathBuf, policy: P) -> Self
|
||||
pub fn new<P>(bash_path: PathBuf, execve_wrapper: PathBuf, policy: P) -> Self
|
||||
where
|
||||
P: EscalationPolicy + Send + Sync + 'static,
|
||||
{
|
||||
Self {
|
||||
bash_path,
|
||||
execve_wrapper,
|
||||
policy: Arc::new(policy),
|
||||
}
|
||||
}
|
||||
@@ -60,8 +62,15 @@ impl EscalateServer {
|
||||
);
|
||||
env.insert(
|
||||
BASH_EXEC_WRAPPER_ENV_VAR.to_string(),
|
||||
format!("{} escalate", std::env::current_exe()?.to_string_lossy()),
|
||||
self.execve_wrapper.to_string_lossy().to_string(),
|
||||
);
|
||||
|
||||
// TODO: use the sandbox policy and cwd from the calling client.
|
||||
// Note that sandbox_cwd is ignored for ReadOnly, but needs to be legit
|
||||
// for `SandboxPolicy::WorkspaceWrite`.
|
||||
let sandbox_policy = SandboxPolicy::ReadOnly;
|
||||
let sandbox_cwd = PathBuf::from("/__NONEXISTENT__");
|
||||
|
||||
let result = process_exec_tool_call(
|
||||
codex_core::exec::ExecParams {
|
||||
command: vec![
|
||||
@@ -77,9 +86,8 @@ impl EscalateServer {
|
||||
arg0: None,
|
||||
},
|
||||
get_platform_sandbox().unwrap_or(SandboxType::None),
|
||||
// TODO: use the sandbox policy and cwd from the calling client
|
||||
&SandboxPolicy::ReadOnly,
|
||||
&PathBuf::from("/__NONEXISTENT__"), // This is ignored for ReadOnly
|
||||
&sandbox_policy,
|
||||
&sandbox_cwd,
|
||||
&None,
|
||||
None,
|
||||
)
|
||||
|
||||
@@ -65,15 +65,17 @@ impl From<escalate_server::ExecResult> for ExecResult {
|
||||
pub struct ExecTool {
|
||||
tool_router: ToolRouter<ExecTool>,
|
||||
bash_path: PathBuf,
|
||||
execve_wrapper: PathBuf,
|
||||
policy: ExecPolicy,
|
||||
}
|
||||
|
||||
#[tool_router]
|
||||
impl ExecTool {
|
||||
pub fn new(bash_path: PathBuf, policy: ExecPolicy) -> Self {
|
||||
pub fn new(bash_path: PathBuf, execve_wrapper: PathBuf, policy: ExecPolicy) -> Self {
|
||||
Self {
|
||||
tool_router: Self::tool_router(),
|
||||
bash_path,
|
||||
execve_wrapper,
|
||||
policy,
|
||||
}
|
||||
}
|
||||
@@ -87,6 +89,7 @@ impl ExecTool {
|
||||
) -> Result<CallToolResult, McpError> {
|
||||
let escalate_server = EscalateServer::new(
|
||||
self.bash_path.clone(),
|
||||
self.execve_wrapper.clone(),
|
||||
McpEscalationPolicy::new(self.policy, context),
|
||||
);
|
||||
let result = escalate_server
|
||||
@@ -130,8 +133,9 @@ impl ServerHandler for ExecTool {
|
||||
|
||||
pub(crate) async fn serve(
|
||||
bash_path: PathBuf,
|
||||
execve_wrapper: PathBuf,
|
||||
policy: ExecPolicy,
|
||||
) -> Result<RunningService<RoleServer, ExecTool>, rmcp::service::ServerInitializeError> {
|
||||
let tool = ExecTool::new(bash_path, policy);
|
||||
let tool = ExecTool::new(bash_path, execve_wrapper, policy);
|
||||
tool.serve(stdio()).await
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user