diff --git a/bins/challenge-review/src/main.rs b/bins/challenge-review/src/main.rs index fb230359b..7f728b08d 100644 --- a/bins/challenge-review/src/main.rs +++ b/bins/challenge-review/src/main.rs @@ -1,25 +1,30 @@ //! `challenge-review` — entrypoint of the `design-review` container image. //! //! Reads the staged review request (`/work/_review_request.json`), runs the -//! agentic loop (`OpenRouter` when `OPENROUTER_API_KEY` is set, deterministic -//! `SimAgent` otherwise), and writes the verdict JSON to `/out/verdict.json`. -//! Exit 0 with a verdict; non-zero on infra failure (caller retries). +//! agentic loop (`OpenRouter` when a key file / env key is present, +//! deterministic `SimAgent` otherwise), and writes the verdict JSON to +//! `/out/verdict.json`. Exit 0 with a verdict; non-zero on infra failure +//! (caller retries). #![forbid(unsafe_code)] +use std::path::Path; use std::process::ExitCode; use challenge_agentic::{ - AgentConfig, AgenticBackend, ContainerReviewRequest, OpenRouterAgent, SimAgent, DEFAULT_MODEL, - OPENROUTER_API_BASE, + load_api_key_file, AgentConfig, AgenticBackend, ContainerReviewRequest, OpenRouterAgent, + SimAgent, DEFAULT_MODEL, OPENROUTER_API_BASE, }; fn main() -> ExitCode { + // Prefer file mount so the key never appears in the process's initial + // environ (`/proc//environ` is boot-fixed on Linux). + let openrouter_key = load_openrouter_key(); let req_path = std::env::var("REVIEW_REQUEST_PATH") .unwrap_or_else(|_| "/work/_review_request.json".to_owned()); let out_path = std::env::var("REVIEW_OUT_PATH").unwrap_or_else(|_| "/out/verdict.json".to_owned()); - match run(&req_path, &out_path) { + match run(&req_path, &out_path, openrouter_key) { Ok(()) => ExitCode::SUCCESS, Err(msg) => { eprintln!("challenge-review: {msg}"); @@ -28,23 +33,38 @@ fn main() -> ExitCode { } } -fn run(req_path: &str, out_path: &str) -> Result<(), String> { +fn load_openrouter_key() -> Option { + if let Ok(path) = std::env::var("OPENROUTER_API_KEY_FILE") { + if let Ok(key) = load_api_key_file(Path::new(&path)) { + return Some(key); + } + } + // Legacy env inject: take into memory and clear the live environ map + // (does not rewrite `/proc/*/environ`). + let key = std::env::var("OPENROUTER_API_KEY") + .ok() + .map(|k| k.trim().to_owned()) + .filter(|k| !k.is_empty()); + std::env::remove_var("OPENROUTER_API_KEY"); + key +} + +fn run(req_path: &str, out_path: &str, openrouter_key: Option) -> Result<(), String> { let text = std::fs::read_to_string(req_path).map_err(|e| format!("read request: {e}"))?; let container_req: ContainerReviewRequest = serde_json::from_str(&text).map_err(|e| format!("parse request: {e}"))?; let req = container_req.into_request(std::path::PathBuf::from("/work")); - let backend: Box = match std::env::var("OPENROUTER_API_KEY") { - Ok(key) if !key.trim().is_empty() => { + let backend: Box = match openrouter_key { + Some(key) => { let base = std::env::var("OPENROUTER_BASE_URL") .unwrap_or_else(|_| OPENROUTER_API_BASE.to_owned()); let model = std::env::var("OPENROUTER_MODEL").unwrap_or_else(|_| DEFAULT_MODEL.into()); - let agent = - OpenRouterAgent::with_config(key.trim(), base, model, AgentConfig::default()) - .map_err(|e| format!("agent init: {e}"))?; + let agent = OpenRouterAgent::with_config(key, base, model, AgentConfig::default()) + .map_err(|e| format!("agent init: {e}"))?; Box::new(agent) } - _ => Box::new(SimAgent::new()), + None => Box::new(SimAgent::new()), }; let rt = tokio::runtime::Builder::new_multi_thread() diff --git a/crates/challenge-agentic/src/tools.rs b/crates/challenge-agentic/src/tools.rs index 18abaeb6b..025e9e038 100644 --- a/crates/challenge-agentic/src/tools.rs +++ b/crates/challenge-agentic/src/tools.rs @@ -36,12 +36,9 @@ impl ToolContext { } let mut corpus = Vec::with_capacity(req.corpus.len()); for entry in &req.corpus { - match fingerprint_source(&entry.source) { - Ok(fp) => corpus.push((entry.id.clone(), fp)), - Err(e) => { - // Skip unparseable corpus entries; still usable for byte-hash sim. - let _ = e; - } + // Skip unparseable corpus entries; still usable for byte-hash sim. + if let Ok(fp) = fingerprint_source(&entry.source) { + corpus.push((entry.id.clone(), fp)); } } // `run_command` exists only inside the hardened review container (the @@ -439,9 +436,8 @@ const RUN_COMMAND_TIMEOUT: std::time::Duration = std::time::Duration::from_secs( const RUN_COMMAND_MAX_OUT: usize = 16 * 1024; /// Sandboxed shell for the review container: fixed workdir cwd, scrubbed env -/// (no API keys), hard timeout, truncated output. The container itself (no -/// capabilities, read-only rootfs, no writable mounts besides /out + /tmp) is -/// the security boundary; this tool adds cwd/env/time/output limits. +/// (no API keys), hard timeout, truncated output; refuses procfs and +/// review-secrets paths so file-mounted `OpenRouter` keys stay unread. fn tool_run_command(ctx: &ToolContext, args: &Value) -> Result { if !ctx.enable_run_command { return Err(AgenticError::Tool( @@ -455,6 +451,11 @@ fn tool_run_command(ctx: &ToolContext, args: &Value) -> Result 1_000 || cmd.contains('\0') { return Err(AgenticError::Tool("run_command: bad command".into())); } + // File-mounted OpenRouter key + parent environ must stay unread. + let c = cmd.to_ascii_lowercase().replace('\\', "/"); + if c.contains("/proc") || c.contains("review-secrets") || c.contains("openrouter_api_key") { + return Err(AgenticError::Tool("run_command: forbidden path".into())); + } let rel = args.get("path").and_then(Value::as_str).unwrap_or("."); let cwd = resolve_rel(&ctx.workdir, rel)?; if !cwd.is_dir() { @@ -738,6 +739,17 @@ mod tests { assert!(out.contains("exit=0"), "out={out}"); // Escape attempts fail via resolve_rel on cwd. assert!(tool_run_command(&ctx, &json!({"command": "ls", "path": ".."})).is_err()); + // Parent environ / secrets exfil via shell is refused (defense-in-depth). + for bad in [ + "cat /proc/1/environ", + "python -c 'open(\"//proc/self/environ\").read()'", + "cat /run/review-secrets/openrouter_api_key", + ] { + let err = tool_run_command(&ctx, &json!({"command": bad})) + .unwrap_err() + .to_string(); + assert!(err.contains("forbidden"), "cmd={bad} err={err}"); + } } #[test] diff --git a/crates/design-egress-proxy/src/lib.rs b/crates/design-egress-proxy/src/lib.rs index 7f1d2f9ad..19769f316 100644 --- a/crates/design-egress-proxy/src/lib.rs +++ b/crates/design-egress-proxy/src/lib.rs @@ -55,6 +55,9 @@ const BLOCKED_HOSTNAMES: &[&str] = &[ "updater", "site-api", "evil-gateway", + // Co-located on the master compose network; sandboxes must not reach it + // even when DNS resolves (post-resolution IP block is the real guard). + "validator", ]; /// True when `host` names a loopback / internal control-plane target. @@ -480,9 +483,21 @@ mod tests { #[test] fn control_plane_names_blocked() { - for name in ["gateway", "postgres", "socket-proxy", "design-challenge"] { - assert!(host_blocked_by_name(name)); + for name in [ + "gateway", + "postgres", + "socket-proxy", + "design-challenge", + "prism-challenge", + "design-egress-proxy", + "updater", + "site-api", + "evil-gateway", + "validator", + ] { + assert!(host_blocked_by_name(name), "{name} must be blocked"); } + assert!(host_blocked_by_name("Validator")); // case-insensitive assert!(host_blocked_by_name("localhost")); assert!(host_blocked_by_name("foo.internal")); assert!(!host_blocked_by_name("pypi.org")); diff --git a/crates/review-docker/src/lib.rs b/crates/review-docker/src/lib.rs index 4f70de5fc..3ce1c5504 100644 --- a/crates/review-docker/src/lib.rs +++ b/crates/review-docker/src/lib.rs @@ -3,9 +3,15 @@ //! the most-similar harness mounted read-only, verdict written to `/out`. //! //! The container gets `AGENTIC_ENABLE_RUN_COMMAND=1` so the LLM may use the -//! sandboxed `run_command` tool. With no `OpenRouter` key the inner agent is -//! the deterministic `SimAgent` and the container runs with networking -//! disabled. +//! sandboxed `run_command` tool (required for diffs / grep / AST probes in +//! prod). Mitigations against prompt-injected `OpenRouter` key theft: +//! - key is mounted as a **file** (`OPENROUTER_API_KEY_FILE`), never put in +//! the container's initial process environ (`/proc//environ` is a +//! boot-time snapshot and cannot be scrubbed by `unsetenv`); +//! - `run_command` children get `env_clear` and refuse procfs + secrets paths. +//! +//! With no `OpenRouter` key the inner agent is the deterministic `SimAgent` +//! and the container runs with networking disabled. #![forbid(unsafe_code)] #![allow(clippy::missing_errors_doc)] @@ -25,6 +31,10 @@ pub const DEFAULT_REVIEW_IMAGE: &str = "design-review:0.1.0"; pub const CONTAINER_REQUEST_PATH: &str = "/work/_review_request.json"; /// Verdict output path inside the container. pub const CONTAINER_VERDICT_PATH: &str = "/out/verdict.json"; +/// `OpenRouter` key file path inside the container (RO secrets mount). +pub const CONTAINER_KEY_FILE: &str = "/run/review-secrets/openrouter_api_key"; +/// Secrets mount point inside the container. +pub const CONTAINER_SECRETS_MOUNT: &str = "/run/review-secrets"; /// `DockerAgent` settings. #[derive(Debug, Clone)] @@ -33,7 +43,7 @@ pub struct DockerAgentConfig { pub docker_base: String, /// Review image ref (digest-pinned in deploy). pub image: String, - /// `OpenRouter` key handed to the inner agent (env, never logged). + /// `OpenRouter` key handed to the inner agent (file mount, never logged). pub openrouter_key: Option, /// Optional `OpenRouter` base override. pub openrouter_base: Option, @@ -75,15 +85,20 @@ impl DockerAgent { Ok(Self { client, cfg }) } - /// Container env (key never logged by callers). - fn container_env(&self) -> Vec { + /// Container env (key never logged by callers; never put the raw key in env). + /// + /// Prod keeps `AGENTIC_ENABLE_RUN_COMMAND=1`: agentic review needs shell + /// probes. Do not disable without an alternate inspection path; secrets + /// stay out of process environ and `run_command` denies procfs + the + /// secrets mount. + fn container_env(&self, key_file: bool) -> Vec { let mut env = vec![ "AGENTIC_ENABLE_RUN_COMMAND=1".to_owned(), format!("REVIEW_REQUEST_PATH={CONTAINER_REQUEST_PATH}"), format!("REVIEW_OUT_PATH={CONTAINER_VERDICT_PATH}"), ]; - if let Some(k) = &self.cfg.openrouter_key { - env.push(format!("OPENROUTER_API_KEY={k}")); + if key_file { + env.push(format!("OPENROUTER_API_KEY_FILE={CONTAINER_KEY_FILE}")); } if let Some(b) = &self.cfg.openrouter_base { env.push(format!("OPENROUTER_BASE_URL={b}")); @@ -94,7 +109,7 @@ impl DockerAgent { env } - fn build_spec(&self, work: &Path, out_dir: &Path) -> RunSpec { + fn build_spec(&self, work: &Path, out_dir: &Path, secrets_dir: Option<&Path>) -> RunSpec { let ns = SystemTime::now() .duration_since(UNIX_EPOCH) .map_or(0, |d| d.as_nanos()); @@ -110,11 +125,18 @@ impl DockerAgent { self.cfg.image.clone(), vec![], ); - spec.binds = vec![ + let mut binds = vec![ format!("{}:/work:ro", work.display()), format!("{}:/out:rw", out_dir.display()), ]; - spec.env = self.container_env(); + if let Some(secrets) = secrets_dir { + binds.push(format!( + "{}:{CONTAINER_SECRETS_MOUNT}:ro", + secrets.display() + )); + } + spec.binds = binds; + spec.env = self.container_env(secrets_dir.is_some()); spec.network_mode = None; // Sim inner agent needs no network at all. spec.network_disabled = self.cfg.openrouter_key.is_none(); @@ -161,6 +183,35 @@ pub fn stage_review_mounts(req: &ReviewRequest) -> Result Ok(out_dir) } +/// Stage the `OpenRouter` key into a host-side secrets dir (sibling of `out_dir`) +/// for a RO bind into the review container. Never logs the key. +/// +/// # Errors +/// Staging I/O failures. +pub fn stage_review_secrets(out_dir: &Path, key: &str) -> Result { + let secrets = out_dir.with_file_name(format!( + "{}.secrets", + out_dir + .file_name() + .map_or_else(|| "review.out".into(), |n| n.to_string_lossy().into_owned()) + )); + let _ = std::fs::remove_dir_all(&secrets); + std::fs::create_dir_all(&secrets).map_err(|e| AgenticError::Tool(format!("secrets: {e}")))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(&secrets, std::fs::Permissions::from_mode(0o700)); + } + let path = secrets.join("openrouter_api_key"); + std::fs::write(&path, key).map_err(|e| AgenticError::Tool(format!("secrets: {e}")))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o400)); + } + Ok(secrets) +} + fn most_similar(req: &ReviewRequest) -> Option<(String, String)> { let cand = req .primary_relpaths @@ -187,7 +238,11 @@ fn most_similar(req: &ReviewRequest) -> Option<(String, String)> { impl AgenticBackend for DockerAgent { async fn review(&self, req: &ReviewRequest) -> Result { let out_dir = stage_review_mounts(req)?; - let spec = self.build_spec(&req.workdir, &out_dir); + let secrets_dir = match &self.cfg.openrouter_key { + Some(k) => Some(stage_review_secrets(&out_dir, k)?), + None => None, + }; + let spec = self.build_spec(&req.workdir, &out_dir, secrets_dir.as_deref()); let client = self.client.clone(); let run = tokio::task::spawn_blocking(move || client.run_owned(&spec)) .await @@ -202,6 +257,9 @@ impl AgenticBackend for DockerAgent { )) })?; let _ = std::fs::remove_dir_all(&out_dir); + if let Some(s) = secrets_dir { + let _ = std::fs::remove_dir_all(&s); + } if run.status_code != 0 { return Err(AgenticError::Provider(format!( "review exit={}: {}", @@ -262,4 +320,69 @@ mod tests { assert!(v.get("workdir").is_none()); let _ = std::fs::remove_dir_all(&out); } + + #[test] + fn container_env_uses_key_file_not_environ() { + let agent = DockerAgent::new(DockerAgentConfig { + openrouter_key: Some("sk-test-not-a-real-key".into()), + ..DockerAgentConfig::default() + }) + .unwrap(); + let env = agent.container_env(true); + assert!(env.iter().any(|e| e == "AGENTIC_ENABLE_RUN_COMMAND=1")); + let expected = format!("OPENROUTER_API_KEY_FILE={CONTAINER_KEY_FILE}"); + assert!( + env.iter().any(|e| e == &expected), + "missing key-file env; got {env:?}" + ); + assert!( + !env.iter().any(|e| e.starts_with("OPENROUTER_API_KEY=")), + "raw key must not enter container environ (proc environ is boot-fixed)" + ); + assert!(!env.iter().any(|e| e.contains("sk-test-not-a-real-key"))); + } + + #[test] + fn stage_secrets_writes_key_file() { + let dir = tempdir().unwrap(); + let out = dir.path().join("agentic-run1.out"); + std::fs::create_dir_all(&out).unwrap(); + let secrets = stage_review_secrets(&out, "sk-test-not-a-real-key").unwrap(); + let key_path = secrets.join("openrouter_api_key"); + assert_eq!( + std::fs::read_to_string(&key_path).unwrap(), + "sk-test-not-a-real-key" + ); + let _ = std::fs::remove_dir_all(&secrets); + } + + #[test] + fn build_spec_binds_secrets_mount() { + let dir = tempdir().unwrap(); + let work = dir.path().join("agentic-run1"); + let out = dir.path().join("agentic-run1.out"); + let secrets = dir.path().join("agentic-run1.out.secrets"); + std::fs::create_dir_all(&work).unwrap(); + std::fs::create_dir_all(&out).unwrap(); + std::fs::create_dir_all(&secrets).unwrap(); + let agent = DockerAgent::new(DockerAgentConfig { + openrouter_key: Some("sk-test-not-a-real-key".into()), + ..DockerAgentConfig::default() + }) + .unwrap(); + let spec = agent.build_spec(&work, &out, Some(&secrets)); + assert!(spec + .binds + .iter() + .any(|b| b.contains(":ro") && b.contains(CONTAINER_SECRETS_MOUNT))); + assert!(spec + .env + .iter() + .any(|e| e.starts_with("OPENROUTER_API_KEY_FILE="))); + assert!(!spec + .env + .iter() + .any(|e| e.starts_with("OPENROUTER_API_KEY="))); + assert!(!spec.network_disabled); + } } diff --git a/docs/DESIGN_CHALLENGE.md b/docs/DESIGN_CHALLENGE.md index 648471269..5d3eb7ee5 100644 --- a/docs/DESIGN_CHALLENGE.md +++ b/docs/DESIGN_CHALLENGE.md @@ -393,9 +393,14 @@ as the sandbox: `ReadonlyRootfs`, `CapDrop`, `no-new-privileges:true`, uid 65532) built from `deploy/Dockerfile` target `design-review` (`challenge-agentic` + `challenge-ast`). The container mounts the submitted agent **and** the most-similar harness (`_similar/`) read-only; the LLM may -use the sandboxed `run_command` tool (scrubbed env, cwd-pinned, 15s cap) for -diffs / grep / AST probes. `DESIGN_REVIEW_BACKEND=inline` keeps the legacy -in-process path for local/CI only. +use the sandboxed `run_command` tool (scrubbed child env, cwd-pinned, 15s +cap, procfs + review-secrets paths denied) for diffs / grep / AST probes. +`AGENTIC_ENABLE_RUN_COMMAND=1` stays on in prod (essential for review +quality). The OpenRouter key is file-mounted (`OPENROUTER_API_KEY_FILE` under +`/run/review-secrets`) and is **never** placed in the container's process +environ — Linux `/proc//environ` is a boot-time snapshot and cannot be +scrubbed. `DESIGN_REVIEW_BACKEND=inline` keeps the legacy in-process path for +local/CI only. ### Pre-LLM copy gate (`created_at` ordered) diff --git a/docs/OPERATOR_SECURITY.md b/docs/OPERATOR_SECURITY.md index 65eab3a6a..eec7db5a4 100644 --- a/docs/OPERATOR_SECURITY.md +++ b/docs/OPERATOR_SECURITY.md @@ -13,6 +13,7 @@ Use this before every promote and after every incident. Architecture: [`ARCHITEC - [ ] Challenge signing secrets are **files** mounted into the challenge service, not env values (D11). - [ ] Owner and challenge mini-secrets never committed; only `*.pubkey` / TOML bodies + detached `.sig` in git. - [ ] Cloudflare / DO / Phala tokens live only in operator secret stores, not in docs or CI logs. +- [ ] Design agentic review: OpenRouter key is mounted on `design-challenge` / `design-egress-proxy` as a **file**, never into miner sandboxes. The ephemeral `design-review` container must receive the key via a **file mount** (`OPENROUTER_API_KEY_FILE`), never as `OPENROUTER_API_KEY` in container env (`/proc//environ` is boot-fixed). `run_command` must keep procfs and `/run/review-secrets` denied. Do not turn off `AGENTIC_ENABLE_RUN_COMMAND` in prod without a replacement inspection path. ---