From 7f778d4fc684c4e439247b0e7064524b895cbaae Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Wed, 11 Mar 2026 10:15:08 -0700 Subject: [PATCH] fix(policy): enforce run_as_user/run_as_group must be 'sandbox' Replace the clear_process_identity + maybe_infer_sandbox_user workaround with strict enforcement: run_as_user and run_as_group must always be 'sandbox'. If omitted from YAML, they are defaulted server-side via ensure_sandbox_process_identity(). Any other value is rejected. This fixes 'policy set' failing with 'process policy cannot be changed on a live sandbox' for custom-image sandboxes, where the CLI was clearing identity fields but the YAML had 'sandbox' explicitly set. - Remove clear_process_identity() from CLI and TUI - Add ensure_sandbox_process_identity() to default missing fields - Rename RootProcessIdentity -> InvalidProcessIdentity - Replace maybe_infer_sandbox_user with validate_sandbox_user (fail-fast) - Update OPA test policies to use run_as_user: sandbox --- crates/navigator-cli/src/run.rs | 13 +--- crates/navigator-policy/src/lib.rs | 115 +++++++++++++++++++--------- crates/navigator-sandbox/src/lib.rs | 84 +++++++------------- crates/navigator-sandbox/src/opa.rs | 24 +++--- crates/navigator-server/src/grpc.rs | 11 ++- crates/navigator-tui/src/lib.rs | 11 ++- 6 files changed, 134 insertions(+), 124 deletions(-) diff --git a/crates/navigator-cli/src/run.rs b/crates/navigator-cli/src/run.rs index bdce826bbe..6996d17dfc 100644 --- a/crates/navigator-cli/src/run.rs +++ b/crates/navigator-cli/src/run.rs @@ -1517,18 +1517,7 @@ pub async fn sandbox_create( ) .await?; - let mut policy = load_sandbox_policy(policy)?; - - // When a custom image is specified, clear the default run_as_user/group - // to prevent failures on images that lack the "sandbox" user/group. - // If no explicit policy was provided, start from the restrictive default - // so the server stores a policy with cleared identity — otherwise the - // sandbox falls back to disk discovery which may re-introduce - // run_as_user: sandbox for images that don't have that user. - if image.is_some() { - let p = policy.get_or_insert_with(navigator_policy::restrictive_default_policy); - navigator_policy::clear_process_identity(p); - } + let policy = load_sandbox_policy(policy)?; let template = image.map(|img| SandboxTemplate { image: img, diff --git a/crates/navigator-policy/src/lib.rs b/crates/navigator-policy/src/lib.rs index 68e6ee1049..98b3383d14 100644 --- a/crates/navigator-policy/src/lib.rs +++ b/crates/navigator-policy/src/lib.rs @@ -363,14 +363,20 @@ pub fn restrictive_default_policy() -> SandboxPolicy { } } -/// Clear `run_as_user` / `run_as_group` from the policy's process section. +/// Ensure the policy has `run_as_user: sandbox` and `run_as_group: sandbox`. /// -/// Call this when a custom image is specified, since the image may lack the -/// default "sandbox" user/group. -pub fn clear_process_identity(policy: &mut SandboxPolicy) { - if let Some(ref mut process) = policy.process { - process.run_as_user = String::new(); - process.run_as_group = String::new(); +/// If the process section is missing, or either field is empty, this fills in +/// the required `"sandbox"` value. Call this before validation so that +/// policies without an explicit process section get the correct default. +pub fn ensure_sandbox_process_identity(policy: &mut SandboxPolicy) { + let process = policy + .process + .get_or_insert_with(|| ProcessPolicy::default()); + if process.run_as_user.is_empty() { + process.run_as_user = "sandbox".into(); + } + if process.run_as_group.is_empty() { + process.run_as_group = "sandbox".into(); } } @@ -387,8 +393,8 @@ const MAX_PATH_LENGTH: usize = 4096; /// A safety violation found in a sandbox policy. #[derive(Debug, Clone, PartialEq, Eq)] pub enum PolicyViolation { - /// `run_as_user` or `run_as_group` is "root" or "0". - RootProcessIdentity { field: &'static str, value: String }, + /// `run_as_user` or `run_as_group` is not "sandbox". + InvalidProcessIdentity { field: &'static str, value: String }, /// A filesystem path contains `..` components. PathTraversal { path: String }, /// A filesystem path is not absolute (does not start with `/`). @@ -404,8 +410,8 @@ pub enum PolicyViolation { impl fmt::Display for PolicyViolation { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::RootProcessIdentity { field, value } => { - write!(f, "{field} cannot be '{value}' (root is not allowed)") + Self::InvalidProcessIdentity { field, value } => { + write!(f, "{field} must be 'sandbox', got '{value}'") } Self::PathTraversal { path } => { write!(f, "path contains '..' traversal component: {path}") @@ -439,7 +445,7 @@ impl fmt::Display for PolicyViolation { /// error vs. logged warning). /// /// Checks performed: -/// - `run_as_user` / `run_as_group` must not be "root" or "0" +/// - `run_as_user` / `run_as_group` must be "sandbox" /// - Filesystem paths must be absolute (start with `/`) /// - Filesystem paths must not contain `..` components /// - Read-write paths must not be overly broad (just `/`) @@ -450,16 +456,18 @@ pub fn validate_sandbox_policy( ) -> std::result::Result<(), Vec> { let mut violations = Vec::new(); - // Check process identity + // Check process identity — must be "sandbox". + // `ensure_sandbox_process_identity` should be called before this to + // fill in defaults; anything other than "sandbox" is rejected. if let Some(ref process) = policy.process { - if is_root_identity(&process.run_as_user) { - violations.push(PolicyViolation::RootProcessIdentity { + if process.run_as_user != "sandbox" { + violations.push(PolicyViolation::InvalidProcessIdentity { field: "run_as_user", value: process.run_as_user.clone(), }); } - if is_root_identity(&process.run_as_group) { - violations.push(PolicyViolation::RootProcessIdentity { + if process.run_as_group != "sandbox" { + violations.push(PolicyViolation::InvalidProcessIdentity { field: "run_as_group", value: process.run_as_group.clone(), }); @@ -519,15 +527,6 @@ pub fn validate_sandbox_policy( } } -/// Check if a user/group identity string refers to root. -fn is_root_identity(value: &str) -> bool { - if value.is_empty() { - return false; - } - let trimmed = value.trim(); - trimmed == "root" || trimmed == "0" -} - /// Truncate a string for safe inclusion in error messages. fn truncate_for_display(s: &str) -> String { if s.len() <= 80 { @@ -724,13 +723,35 @@ network_policies: } #[test] - fn clear_process_identity_clears_fields() { + fn ensure_sandbox_process_identity_fills_defaults() { let mut policy = restrictive_default_policy(); - assert_eq!(policy.process.as_ref().unwrap().run_as_user, "sandbox"); - clear_process_identity(&mut policy); + policy.process = None; + ensure_sandbox_process_identity(&mut policy); + let proc = policy.process.unwrap(); + assert_eq!(proc.run_as_user, "sandbox"); + assert_eq!(proc.run_as_group, "sandbox"); + } + + #[test] + fn ensure_sandbox_process_identity_fills_empty_strings() { + let mut policy = restrictive_default_policy(); + policy.process = Some(ProcessPolicy { + run_as_user: String::new(), + run_as_group: String::new(), + }); + ensure_sandbox_process_identity(&mut policy); let proc = policy.process.unwrap(); - assert!(proc.run_as_user.is_empty()); - assert!(proc.run_as_group.is_empty()); + assert_eq!(proc.run_as_user, "sandbox"); + assert_eq!(proc.run_as_group, "sandbox"); + } + + #[test] + fn ensure_sandbox_process_identity_preserves_sandbox() { + let mut policy = restrictive_default_policy(); + ensure_sandbox_process_identity(&mut policy); + let proc = policy.process.unwrap(); + assert_eq!(proc.run_as_user, "sandbox"); + assert_eq!(proc.run_as_group, "sandbox"); } #[test] @@ -750,7 +771,7 @@ network_policies: let violations = validate_sandbox_policy(&policy).unwrap_err(); assert!(violations.iter().any(|v| matches!( v, - PolicyViolation::RootProcessIdentity { + PolicyViolation::InvalidProcessIdentity { field: "run_as_user", .. } @@ -768,6 +789,28 @@ network_policies: assert_eq!(violations.len(), 2); } + #[test] + fn validate_rejects_non_sandbox_user() { + let mut policy = restrictive_default_policy(); + policy.process = Some(ProcessPolicy { + run_as_user: "nobody".into(), + run_as_group: "nogroup".into(), + }); + let violations = validate_sandbox_policy(&policy).unwrap_err(); + assert_eq!(violations.len(), 2); + assert!( + violations + .iter() + .all(|v| matches!(v, PolicyViolation::InvalidProcessIdentity { .. })) + ); + } + + #[test] + fn validate_accepts_sandbox_identity() { + let policy = restrictive_default_policy(); + assert!(validate_sandbox_policy(&policy).is_ok()); + } + #[test] fn validate_rejects_path_traversal() { let mut policy = restrictive_default_policy(); @@ -835,13 +878,14 @@ network_policies: } #[test] - fn validate_accepts_empty_run_as_user() { + fn validate_rejects_empty_run_as_user() { let mut policy = restrictive_default_policy(); policy.process = Some(ProcessPolicy { run_as_user: String::new(), run_as_group: String::new(), }); - assert!(validate_sandbox_policy(&policy).is_ok()); + let violations = validate_sandbox_policy(&policy).unwrap_err(); + assert_eq!(violations.len(), 2); } #[test] @@ -893,12 +937,13 @@ network_policies: #[test] fn policy_violation_display() { - let v = PolicyViolation::RootProcessIdentity { + let v = PolicyViolation::InvalidProcessIdentity { field: "run_as_user", value: "root".into(), }; let s = format!("{v}"); assert!(s.contains("root")); assert!(s.contains("run_as_user")); + assert!(s.contains("sandbox")); } } diff --git a/crates/navigator-sandbox/src/lib.rs b/crates/navigator-sandbox/src/lib.rs index f657db63f0..3094e01efd 100644 --- a/crates/navigator-sandbox/src/lib.rs +++ b/crates/navigator-sandbox/src/lib.rs @@ -163,7 +163,7 @@ pub async fn run_sandbox( // Load policy and initialize OPA engine let navigator_endpoint_for_proxy = navigator_endpoint.clone(); - let (mut policy, opa_engine) = load_policy( + let (policy, opa_engine) = load_policy( sandbox_id.clone(), sandbox, navigator_endpoint.clone(), @@ -172,13 +172,10 @@ pub async fn run_sandbox( ) .await?; - // When no run_as_user/group is configured (e.g. custom/community images - // where the CLI cleared identity) and the supervisor is running as root - // (forced via runAsUser: 0 in the pod spec), auto-detect the image's - // intended sandbox user. This prevents child processes from running as - // root unnecessarily. + // Validate that the required "sandbox" user exists in this image. + // All sandbox images must include this user for privilege dropping. #[cfg(unix)] - maybe_infer_sandbox_user(&mut policy); + validate_sandbox_user(&policy)?; // Fetch provider environment variables from the server. // This is done after loading the policy so the sandbox can still start @@ -1026,61 +1023,36 @@ fn discover_policy_from_path(path: &std::path::Path) -> navigator_core::proto::S } } -/// When the policy has no `run_as_user`/`run_as_group` and the supervisor is -/// running as root, try to detect the image's intended non-root user for -/// child process privilege dropping. +/// Validate that the `sandbox` user exists in this image. /// -/// This handles the BYOC / community-image case where the CLI clears -/// `run_as_user` (to avoid failures if the default "sandbox" user doesn't -/// exist), but the K8s pod spec forces `runAsUser: 0` so the supervisor can -/// perform privileged setup. Without this, child processes would inherit -/// root — undermining the image author's security intent. -/// -/// Detection strategy: look for a `sandbox` user in `/etc/passwd`. This is -/// the conventional non-root user in OpenShell sandbox images. +/// All sandbox images must include a `sandbox` user for privilege dropping. +/// This check runs at supervisor startup (inside the container) where we can +/// inspect `/etc/passwd`. If the user is missing, the sandbox fails fast +/// with a clear error instead of silently running child processes as root. #[cfg(unix)] -fn maybe_infer_sandbox_user(policy: &mut SandboxPolicy) { - use nix::unistd::{Uid, User}; +fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { + use nix::unistd::User; - // Only infer when both fields are empty and we're running as root. - let has_user = policy - .process - .run_as_user - .as_ref() - .is_some_and(|u| !u.is_empty()); - let has_group = policy - .process - .run_as_group - .as_ref() - .is_some_and(|g| !g.is_empty()); - if has_user || has_group || !Uid::effective().is_root() { - return; - } + let user_name = policy.process.run_as_user.as_deref().unwrap_or("sandbox"); - // Check if a "sandbox" user exists in the image. - match User::from_name("sandbox") { - Ok(Some(_)) => { - info!( - "Policy has no run_as_user but image has 'sandbox' user; \ - inferring run_as_user=sandbox, run_as_group=sandbox" - ); - policy.process.run_as_user = Some("sandbox".to_string()); - policy.process.run_as_group = Some("sandbox".to_string()); - } - Ok(None) => { - warn!( - "Policy has no run_as_user and image has no 'sandbox' user; \ - child processes will run as root. Set process.run_as_user in \ - your sandbox policy to specify a non-root user." - ); - } - Err(e) => { - warn!( - error = %e, - "Failed to look up 'sandbox' user; child processes will run as root" - ); + if user_name.is_empty() || user_name == "sandbox" { + match User::from_name("sandbox") { + Ok(Some(_)) => { + info!("Validated 'sandbox' user exists in image"); + } + Ok(None) => { + return Err(miette::miette!( + "sandbox user 'sandbox' not found in image; \ + all sandbox images must include a 'sandbox' user and group" + )); + } + Err(e) => { + return Err(miette::miette!("failed to look up 'sandbox' user: {e}")); + } } } + + Ok(()) } /// Prepare filesystem for the sandboxed process. diff --git a/crates/navigator-sandbox/src/opa.rs b/crates/navigator-sandbox/src/opa.rs index 2cd1f942bc..1b7813890a 100644 --- a/crates/navigator-sandbox/src/opa.rs +++ b/crates/navigator-sandbox/src/opa.rs @@ -569,8 +569,8 @@ fn proto_to_opa_data_json(proto: &ProtoSandboxPolicy) -> String { let process = proto.process.as_ref().map_or_else( || { serde_json::json!({ - "run_as_user": "", - "run_as_group": "", + "run_as_user": "sandbox", + "run_as_group": "sandbox", }) }, |p| { @@ -1292,8 +1292,8 @@ filesystem_policy: landlock: compatibility: best_effort process: - run_as_user: "" - run_as_group: "" + run_as_user: sandbox + run_as_group: sandbox "#; fn l7_engine() -> OpaEngine { @@ -1535,8 +1535,8 @@ filesystem_policy: landlock: compatibility: best_effort process: - run_as_user: "" - run_as_group: "" + run_as_user: sandbox + run_as_group: sandbox "#; const NO_INFERENCE_TEST_DATA: &str = r#" @@ -1554,8 +1554,8 @@ filesystem_policy: landlock: compatibility: best_effort process: - run_as_user: "" - run_as_group: "" + run_as_user: sandbox + run_as_group: sandbox "#; fn inference_engine() -> OpaEngine { @@ -1777,8 +1777,8 @@ filesystem_policy: landlock: compatibility: best_effort process: - run_as_user: "" - run_as_group: "" + run_as_user: sandbox + run_as_group: sandbox "#; fn allowed_ips_engine() -> OpaEngine { @@ -1918,8 +1918,8 @@ process: compatibility: "best_effort".to_string(), }), process: Some(ProtoProc { - run_as_user: "".to_string(), - run_as_group: "".to_string(), + run_as_user: "sandbox".to_string(), + run_as_group: "sandbox".to_string(), }), network_policies, }; diff --git a/crates/navigator-server/src/grpc.rs b/crates/navigator-server/src/grpc.rs index 8712f6bd48..1737e4bfc0 100644 --- a/crates/navigator-server/src/grpc.rs +++ b/crates/navigator-server/src/grpc.rs @@ -163,8 +163,10 @@ impl Navigator for NavigatorService { template.image = self.state.sandbox_client.default_image().to_string(); } - // Validate policy safety before persisting. - if let Some(ref policy) = spec.policy { + // Ensure process identity defaults to "sandbox" when missing or + // empty, then validate policy safety before persisting. + if let Some(ref mut policy) = spec.policy { + navigator_policy::ensure_sandbox_process_identity(policy); validate_policy_safety(policy)?; } @@ -968,7 +970,7 @@ impl Navigator for NavigatorService { if req.name.is_empty() { return Err(Status::invalid_argument("name is required")); } - let new_policy = req + let mut new_policy = req .policy .ok_or_else(|| Status::invalid_argument("policy is required"))?; @@ -989,6 +991,9 @@ impl Navigator for NavigatorService { .as_ref() .ok_or_else(|| Status::internal("sandbox has no spec"))?; + // Ensure process identity defaults to "sandbox" when missing or empty. + navigator_policy::ensure_sandbox_process_identity(&mut new_policy); + if let Some(baseline_policy) = spec.policy.as_ref() { // Validate static fields haven't changed. validate_static_fields_unchanged(baseline_policy, &new_policy)?; diff --git a/crates/navigator-tui/src/lib.rs b/crates/navigator-tui/src/lib.rs index 5181cbdb75..9cef8b47dd 100644 --- a/crates/navigator-tui/src/lib.rs +++ b/crates/navigator-tui/src/lib.rs @@ -1163,14 +1163,13 @@ fn spawn_create_sandbox(app: &mut App, tx: mpsc::UnboundedSender) { None }; + // For custom images, provide a restrictive default policy so the + // server has a baseline. The server ensures process identity is set + // to "sandbox". For the default image, let the server apply the + // sandbox's own default policy. let policy = if has_custom_image { - // Custom images may lack the default "sandbox" user/group, so - // use a restrictive default with cleared process identity. - let mut p = navigator_policy::restrictive_default_policy(); - navigator_policy::clear_process_identity(&mut p); - Some(p) + Some(navigator_policy::restrictive_default_policy()) } else { - // Let the server apply the sandbox's own default policy. None };