From 93013876ff81e0c1291a9a12e87b3b31ef5cab15 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 27 Jul 2026 18:15:37 -0700 Subject: [PATCH 01/12] feat(sandbox): use policy-first OCI image identity Closes #2331 Preserve per-field policy omission, derive Docker and Podman fallbacks from the inspected immutable image, and resolve the final numeric identity before starting agent children. Signed-off-by: Matthew Grossman --- .../skills/debug-openshell-cluster/SKILL.md | 2 + .../skills/generate-sandbox-policy/SKILL.md | 12 +- .../generate-sandbox-policy/examples.md | 9 +- .agents/skills/openshell-cli/SKILL.md | 6 + architecture/compute-runtimes.md | 23 + architecture/sandbox.md | 2 +- crates/openshell-core/src/sandbox_env.rs | 9 + crates/openshell-driver-docker/README.md | 8 + crates/openshell-driver-docker/src/lib.rs | 123 +++- crates/openshell-driver-docker/src/tests.rs | 48 ++ crates/openshell-driver-podman/README.md | 11 +- crates/openshell-driver-podman/src/client.rs | 59 ++ .../openshell-driver-podman/src/container.rs | 77 ++- crates/openshell-driver-podman/src/driver.rs | 26 +- crates/openshell-policy/src/lib.rs | 116 ++-- crates/openshell-sandbox/src/lib.rs | 49 +- crates/openshell-server/src/grpc/policy.rs | 38 +- crates/openshell-server/src/grpc/sandbox.rs | 68 ++- .../src/identity.rs | 532 ++++++++++++++++++ .../openshell-supervisor-process/src/lib.rs | 2 + .../src/process.rs | 45 +- .../openshell-supervisor-process/src/run.rs | 10 +- docs/reference/policy-schema.mdx | 12 +- docs/reference/sandbox-compute-drivers.mdx | 26 +- docs/sandboxes/policies.mdx | 10 +- docs/security/best-practices.mdx | 6 +- e2e/rust/tests/custom_image.rs | 67 ++- examples/bring-your-own-container/Dockerfile | 16 +- examples/bring-your-own-container/README.md | 11 +- 29 files changed, 1187 insertions(+), 236 deletions(-) create mode 100644 crates/openshell-supervisor-process/src/identity.rs diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index 07de687103..5446cf4007 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -148,6 +148,7 @@ Common findings: - Docker daemon unavailable: start Docker Desktop or Docker Engine. - Gateway process stopped: inspect exit status and logs. - Sandbox image missing or pull denied: verify image reference and registry credentials. +- Sandbox fails before readiness with an identity-resolution error: inspect the image's OCI `USER` and matching `/etc/passwd` and `/etc/group` entries, or explicitly set both process identity fields in policy. Root and missing identities are rejected. - Docker driver cannot initialize because it cannot find `openshell-sandbox`: verify `OPENSHELL_DOCKER_SUPERVISOR_BIN`, the sibling binary next to `openshell-gateway`, or the configured supervisor image contains `/openshell-sandbox`. - Sandbox never registers: check gateway logs and supervisor callback endpoint. - Supervisor image exits before printing `openshell-sandbox --version`: the image should be the scratch supervisor image from `deploy/docker/Dockerfile.supervisor` and must contain a static executable at `/openshell-sandbox`. @@ -173,6 +174,7 @@ Common findings: - Podman socket unavailable: start or expose the user socket. - Rootless networking unavailable: inspect Podman network configuration. - Sandbox image missing or pull denied: verify image reference and registry credentials. +- Sandbox fails before readiness with an identity-resolution error: inspect the image's OCI `USER` and matching `/etc/passwd` and `/etc/group` entries, or explicitly set both process identity fields in policy. Root and missing identities are rejected. - Supervisor cannot call back: check callback endpoint and gateway logs. ### Step 6: Check Kubernetes Helm Gateways diff --git a/.agents/skills/generate-sandbox-policy/SKILL.md b/.agents/skills/generate-sandbox-policy/SKILL.md index 8da14420c9..3b261b5ded 100644 --- a/.agents/skills/generate-sandbox-policy/SKILL.md +++ b/.agents/skills/generate-sandbox-policy/SKILL.md @@ -473,15 +473,17 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - network_policies: # ``` -The `filesystem_policy`, `landlock`, and `process` sections above are sensible defaults. Tell the user these are defaults and may need adjustment for their environment. Gateway inference is configured separately through `openshell inference set/get`. The generated `network_policies` block is the primary output. +The `filesystem_policy` and `landlock` sections above are sensible defaults. +Process identity is omitted so the selected compute driver can choose it. For +Docker and Podman, each omitted identity field falls back to the image's OCI +`USER`. Tell the user these are defaults and may need adjustment for their +environment. Gateway inference is configured separately through `openshell +inference set/get`. The generated `network_policies` block is the primary +output. If the user provides a file path, write to it. Otherwise, ask where to place it. A common convention is a project-local policy file (e.g., `sandbox-policy.yaml`) passed to `openshell sandbox create --policy ` or set via the `OPENSHELL_SANDBOX_POLICY` env var. diff --git a/.agents/skills/generate-sandbox-policy/examples.md b/.agents/skills/generate-sandbox-policy/examples.md index b6acbee8bf..179e46c00f 100644 --- a/.agents/skills/generate-sandbox-policy/examples.md +++ b/.agents/skills/generate-sandbox-policy/examples.md @@ -830,10 +830,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - network_policies: github_readonly: name: github_readonly @@ -858,7 +854,10 @@ network_policies: - { path: /usr/local/bin/claude } ``` -The agent notes that `filesystem_policy`, `landlock`, and `process` are sensible defaults that may need adjustment, and that gateway inference is configured separately via `openshell inference set/get` rather than an `inference` policy block. +The agent notes that `filesystem_policy` and `landlock` are sensible defaults +that may need adjustment. Process identity is omitted so the compute driver can +select it. Gateway inference is configured separately via `openshell inference +set/get` rather than an `inference` policy block. --- diff --git a/.agents/skills/openshell-cli/SKILL.md b/.agents/skills/openshell-cli/SKILL.md index a36a372df7..06b06e4470 100644 --- a/.agents/skills/openshell-cli/SKILL.md +++ b/.agents/skills/openshell-cli/SKILL.md @@ -422,6 +422,12 @@ The `--from` flag accepts a Dockerfile path, a directory containing a Dockerfile Local Dockerfile and directory builds require a local gateway because the CLI builds through the local Docker daemon. Use a registry image reference for remote gateways. Bare community names resolve under `ghcr.io/nvidia/openshell-community/sandboxes` unless `OPENSHELL_COMMUNITY_REGISTRY` overrides the prefix. +For Docker and Podman gateways, custom images should declare a non-root OCI +`USER`. Each explicit `process.run_as_user` or `process.run_as_group` policy +field wins independently; omitted fields fall back to the image declaration. +An image with no `USER` fails before readiness unless policy supplies both +fields. + ### Forward ports ```bash diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index f211f906d6..5584034c3c 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -125,6 +125,29 @@ Driver-controlled environment variables must override sandbox image or template values for sandbox ID, sandbox name, gateway endpoint, relay socket path, TLS paths, and command metadata. +## Process Identity + +The gateway preserves whether each policy process field was omitted. The active +driver then supplies one authoritative identity input to the supervisor: + +- Docker and Podman inspect the final sandbox image, pin container creation to + its immutable image ID, and pass its raw OCI `Config.User`. +- Kubernetes passes its platform-resolved numeric UID/GID, including OpenShift + SCC-derived values. +- VM keeps its existing guest identity behavior. + +For Docker and Podman, policy values take precedence independently. An omitted +`run_as_user` or `run_as_group` falls back to the corresponding identity from +the image. The supervisor resolves names from the image's `/etc/passwd` and +`/etc/group`, converts the result to a numeric UID/GID, and uses the same +privilege-drop path for direct and SSH children. It does not rewrite the account +files. + +Sandbox creation fails before the workload becomes ready when a required image +identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0. +The supervisor itself remains root so it can establish isolation before +starting unprivileged children. + Kubernetes can run the supervisor in the default combined topology or in a sidecar topology. Combined mode keeps network and process supervision in the agent container. Sidecar mode runs network enforcement, the proxy, and gateway diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 4f95e1ef69..4f7dfe4272 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -32,7 +32,7 @@ only when the set is already empty; any other outcome fails the spawn. 4. It starts the policy proxy and local SSH server. 5. It opens a supervisor session back to the gateway for connect, exec, file sync, config polling, and log push. -6. It launches the agent command as the restricted sandbox user. +6. It launches the agent command as the resolved restricted identity. ## Isolation Layers diff --git a/crates/openshell-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index f15ce34bb1..ec0167bbd3 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -115,6 +115,15 @@ pub const SANDBOX_UID: &str = "OPENSHELL_SANDBOX_UID"; /// supervisor drops privileges to a group other than the UID's primary group. pub const SANDBOX_GID: &str = "OPENSHELL_SANDBOX_GID"; +/// Raw OCI `Config.User` declaration from the immutable image selected by a +/// local container driver. +/// +/// Docker and Podman always overwrite this value, including with an empty +/// string when the image has no `USER`, and clear [`SANDBOX_UID`] and +/// [`SANDBOX_GID`]. The supervisor resolves omitted policy identity fields +/// from this declaration inside the launched image filesystem. +pub const OCI_IMAGE_USER: &str = "OPENSHELL_OCI_IMAGE_USER"; + // The corporate upstream-proxy configuration deliberately has no reserved // environment variables: it travels on the supervisor's argv // (`--upstream-proxy` and friends), which a sandbox image cannot forge the diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index e17791e747..aaffa6ea79 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -18,6 +18,14 @@ The gateway runs as a host process. The Docker driver creates one container per sandbox and starts the `openshell-sandbox` supervisor inside that container. The supervisor then creates the nested sandbox namespace for the agent process. +Before creating the container, the driver inspects the final sandbox image and +captures its immutable image ID and raw OCI `Config.User`. Container creation +uses that image ID, preventing a mutable tag from changing between inspection +and launch. The supervisor runs as root, resolves omitted policy identity fields +from the image declaration, and drops only agent children to the resulting +numeric UID/GID. Explicit `process.run_as_user` and `process.run_as_group` +values take precedence independently. + Docker containers join an OpenShell-managed bridge network. The driver injects `host.openshell.internal` and `host.docker.internal` so supervisors have stable names for reaching the gateway host. On Docker Desktop, Colima, Rancher diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 2f89c2229e..1b3fd25ef1 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -227,6 +227,12 @@ struct DockerProvisioningFailure { message: String, } +#[derive(Debug, Clone, PartialEq, Eq)] +struct DockerImageMetadata { + id: String, + user: String, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] struct DockerResourceLimits { nano_cpus: Option, @@ -710,7 +716,8 @@ impl DockerComputeDriver { DockerProvisioningFailure::new("ContainerCreateFailed", status.message()) })?; let template = validated.template; - self.ensure_image_available(&sandbox.id, &template.image) + let image = self + .ensure_image_available(&sandbox.id, &template.image) .await .map_err(|status| { DockerProvisioningFailure::new("ImagePullFailed", status.message()) @@ -735,11 +742,12 @@ impl DockerComputeDriver { } DockerProvisioningFailure::new("ContainerCreateFailed", status.message()) })?; - let create_body = build_container_create_body_with_gpu_devices( + let create_body = build_container_create_body_for_image( sandbox, &self.config, &validated.driver_config, gpu_devices.as_deref(), + &image, ) .map_err(|status| { if token_file_created { @@ -1280,41 +1288,71 @@ impl DockerComputeDriver { })) } - async fn ensure_image_available(&self, sandbox_id: &str, image: &str) -> Result<(), Status> { + async fn ensure_image_available( + &self, + sandbox_id: &str, + image: &str, + ) -> Result { let policy = self.config.image_pull_policy.trim().to_ascii_lowercase(); - match policy.as_str() { + let inspect = match policy.as_str() { "" | "ifnotpresent" => { - if self.docker.inspect_image(image).await.is_ok() { + if let Ok(inspect) = self.docker.inspect_image(image).await { self.publish_docker_progress( sandbox_id, "ImagePresent", format!("Docker image \"{image}\" is already present"), HashMap::from([("image_ref".to_string(), image.to_string())]), ); - return Ok(()); + inspect + } else { + self.pull_image(sandbox_id, image).await?; + self.docker + .inspect_image(image) + .await + .map_err(|err| internal_status("inspect Docker image after pull", err))? } - self.pull_image(sandbox_id, image).await } - "always" => self.pull_image(sandbox_id, image).await, + "always" => { + self.pull_image(sandbox_id, image).await?; + self.docker + .inspect_image(image) + .await + .map_err(|err| internal_status("inspect Docker image after pull", err))? + } "never" => match self.docker.inspect_image(image).await { - Ok(_) => { + Ok(inspect) => { self.publish_docker_progress( sandbox_id, "ImagePresent", format!("Docker image \"{image}\" is already present"), HashMap::from([("image_ref".to_string(), image.to_string())]), ); - Ok(()) + inspect + } + Err(err) if is_not_found_error(&err) => { + return Err(Status::failed_precondition(format!( + "docker image '{image}' is not present locally and image_pull_policy=Never" + ))); } - Err(err) if is_not_found_error(&err) => Err(Status::failed_precondition(format!( - "docker image '{image}' is not present locally and image_pull_policy=Never" - ))), - Err(err) => Err(internal_status("inspect Docker image", err)), + Err(err) => return Err(internal_status("inspect Docker image", err)), }, - other => Err(Status::failed_precondition(format!( - "unsupported docker image_pull_policy '{other}'; expected Always, IfNotPresent, or Never", - ))), - } + other => { + return Err(Status::failed_precondition(format!( + "unsupported docker image_pull_policy '{other}'; expected Always, IfNotPresent, or Never", + ))); + } + }; + + let id = inspect.id.ok_or_else(|| { + Status::failed_precondition(format!( + "docker image '{image}' inspection did not return an immutable image ID" + )) + })?; + let user = inspect + .config + .and_then(|config| config.user) + .unwrap_or_default(); + Ok(DockerImageMetadata { id, user }) } async fn pull_image(&self, sandbox_id: &str, image: &str) -> Result<(), Status> { @@ -2132,7 +2170,16 @@ fn cleanup_sandbox_token_file_by_id(sandbox_id: &str, config: &DockerDriverRunti } } +#[cfg(test)] fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig) -> Vec { + build_environment_for_oci_user(sandbox, config, "") +} + +fn build_environment_for_oci_user( + sandbox: &DriverSandbox, + config: &DockerDriverRuntimeConfig, + oci_user: &str, +) -> Vec { let mut environment = HashMap::from([ ("HOME".to_string(), "/root".to_string()), ("PATH".to_string(), SUPERVISOR_PATH.to_string()), @@ -2204,6 +2251,18 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); + environment.insert( + openshell_core::sandbox_env::OCI_IMAGE_USER.to_string(), + oci_user.to_string(), + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX_UID.to_string(), + String::new(), + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX_GID.to_string(), + String::new(), + ); // Gateway-minted sandbox JWT. Keep the raw bearer out of container // metadata; the supervisor reads it from this driver-owned bind mount. @@ -2288,6 +2347,30 @@ fn build_container_create_body_with_gpu_devices( config: &DockerDriverRuntimeConfig, driver_config: &DockerSandboxDriverConfig, gpu_device_ids: Option<&[String]>, +) -> Result { + let template = sandbox + .spec + .as_ref() + .and_then(|spec| spec.template.as_ref()) + .ok_or_else(|| Status::invalid_argument("sandbox.spec.template is required"))?; + build_container_create_body_for_image( + sandbox, + config, + driver_config, + gpu_device_ids, + &DockerImageMetadata { + id: template.image.clone(), + user: String::new(), + }, + ) +} + +fn build_container_create_body_for_image( + sandbox: &DriverSandbox, + config: &DockerDriverRuntimeConfig, + driver_config: &DockerSandboxDriverConfig, + gpu_device_ids: Option<&[String]>, + image: &DockerImageMetadata, ) -> Result { let spec = sandbox .spec @@ -2328,9 +2411,9 @@ fn build_container_create_body_with_gpu_devices( ); Ok(ContainerCreateBody { - image: Some(template.image.clone()), + image: Some(image.id.clone()), user: Some("0".to_string()), - env: Some(build_environment(sandbox, config)), + env: Some(build_environment_for_oci_user(sandbox, config, &image.user)), entrypoint: Some(vec![SUPERVISOR_MOUNT_PATH.to_string()]), // Clear the image CMD so Docker does not append inherited args to the // supervisor entrypoint. diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index 67036b2192..7562e4a8fb 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -541,6 +541,54 @@ fn build_environment_sets_docker_tls_paths() { assert!(env.contains(&"OPENSHELL_SANDBOX_COMMAND=sleep infinity".to_string())); } +#[test] +fn build_environment_protects_oci_identity_metadata() { + let mut sandbox = test_sandbox(); + let spec = sandbox.spec.as_mut().unwrap(); + for (key, value) in [ + (openshell_core::sandbox_env::OCI_IMAGE_USER, "spoofed"), + (openshell_core::sandbox_env::SANDBOX_UID, "9999"), + (openshell_core::sandbox_env::SANDBOX_GID, "9999"), + ] { + spec.environment.insert(key.to_string(), value.to_string()); + } + + let env = build_environment_for_oci_user(&sandbox, &runtime_config(), "app:staff"); + + assert!(env.contains(&format!( + "{}=app:staff", + openshell_core::sandbox_env::OCI_IMAGE_USER + ))); + assert!(env.contains(&format!("{}=", openshell_core::sandbox_env::SANDBOX_UID))); + assert!(env.contains(&format!("{}=", openshell_core::sandbox_env::SANDBOX_GID))); + assert!(!env.iter().any(|entry| entry.ends_with("=spoofed"))); + assert!(!env.iter().any(|entry| entry.ends_with("=9999"))); +} + +#[test] +fn container_creation_uses_inspected_immutable_image() { + let sandbox = test_sandbox(); + let metadata = DockerImageMetadata { + id: "sha256:immutable".to_string(), + user: "1234:1235".to_string(), + }; + let body = build_container_create_body_for_image( + &sandbox, + &runtime_config(), + &DockerSandboxDriverConfig::default(), + None, + &metadata, + ) + .unwrap(); + + assert_eq!(body.image.as_deref(), Some("sha256:immutable")); + assert_eq!(body.user.as_deref(), Some("0")); + assert!(body.env.unwrap().contains(&format!( + "{}=1234:1235", + openshell_core::sandbox_env::OCI_IMAGE_USER + ))); +} + #[test] fn build_environment_keeps_path_driver_controlled() { let mut sandbox = test_sandbox(); diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 90cbac6169..7e8dc89108 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -7,6 +7,15 @@ driver runs in-process within the gateway server and delegates all sandbox isolation enforcement to the `openshell-sandbox` supervisor binary, which is sideloaded into each container via an OCI image volume mount. +Before creating the container, the driver inspects the final sandbox image and +captures its immutable image ID and raw OCI `Config.User`. Container creation +uses that image ID with pulling disabled, preventing a mutable tag from changing +between inspection and launch. The supervisor runs as root, resolves omitted +policy identity fields from the image declaration, and drops only agent +children to the resulting numeric UID/GID. Explicit +`process.run_as_user` and `process.run_as_group` values take precedence +independently. + For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md). ## Architecture @@ -186,7 +195,7 @@ graph TB subgraph Container["Sandbox Container"] SV["Supervisor
(root in user ns)"] subgraph NestedNS["Nested Network Namespace"] - SP["Sandbox Process
(sandbox user)"] + SP["Sandbox Process
(resolved non-root identity)"] VE2["veth1: 10.200.0.2"] end VE1["veth0: 10.200.0.1
(CONNECT proxy)"] diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 59cbda545d..952938d47a 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -162,6 +162,23 @@ pub struct ContainerConfig { pub labels: HashMap, } +/// Immutable image metadata needed to bind OCI identity inspection to launch. +#[derive(Debug, Clone, serde::Deserialize)] +#[serde(rename_all = "PascalCase")] +pub struct ImageInspect { + #[serde(alias = "ID")] + pub id: String, + #[serde(default)] + pub config: Option, +} + +#[derive(Debug, Clone, Default, serde::Deserialize)] +#[serde(rename_all = "PascalCase")] +pub struct ImageConfig { + #[serde(default)] + pub user: String, +} + /// A container summary returned by the list API. #[derive(Debug, Clone, serde::Deserialize)] #[serde(rename_all = "PascalCase")] @@ -675,6 +692,16 @@ impl PodmanClient { Ok(()) } + /// Inspect a locally selected image for immutable ID and OCI config. + pub async fn inspect_image(&self, reference: &str) -> Result { + self.request_json( + hyper::Method::GET, + &format!("/libpod/images/{}/json", url_encode(reference)), + None, + ) + .await + } + // ── System operations ──────────────────────────────────────────────── /// Ping the Podman API to verify connectivity. @@ -903,4 +930,36 @@ mod tests { ); let _ = std::fs::remove_file(socket_path); } + + #[tokio::test] + async fn inspect_image_reads_immutable_id_and_oci_user() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "inspect-image", + vec![StubResponse::new( + StatusCode::OK, + r#"{"Id":"sha256:immutable","Config":{"User":"app:staff"}}"#, + )], + ); + let client = PodmanClient::new(socket_path.clone()); + + let image = client + .inspect_image("example/image:latest") + .await + .expect("image inspect should parse"); + + assert_eq!(image.id, "sha256:immutable"); + assert_eq!( + image.config.as_ref().map(|config| config.user.as_str()), + Some("app:staff") + ); + handle.await.expect("stub task should finish"); + assert_eq!( + request_log + .lock() + .expect("request log lock should not be poisoned") + .as_slice(), + ["GET /v5.0.0/libpod/images/example%2Fimage%3Alatest/json"] + ); + let _ = std::fs::remove_file(socket_path); + } } diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index e417358c6e..2eb07f6e0a 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -397,6 +397,7 @@ fn build_env( sandbox: &DriverSandbox, config: &PodmanComputeConfig, image: &str, + oci_user: &str, ) -> BTreeMap { let spec = sandbox.spec.as_ref(); let template = spec.and_then(|s| s.template.as_ref()); @@ -482,6 +483,18 @@ fn build_env( env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); + env.insert( + openshell_core::sandbox_env::OCI_IMAGE_USER.into(), + oci_user.to_string(), + ); + env.insert( + openshell_core::sandbox_env::SANDBOX_UID.into(), + String::new(), + ); + env.insert( + openshell_core::sandbox_env::SANDBOX_GID.into(), + String::new(), + ); // 4. Gateway-minted sandbox JWT. Keep the raw bearer out of container // metadata; the supervisor reads it from a driver-owned bind mount. @@ -876,6 +889,7 @@ pub fn try_build_container_spec_with_token( build_container_spec_with_token_and_gpu_devices(sandbox, config, token_secret_name, cdi_devices) } +#[cfg(test)] pub fn build_container_spec_with_token_and_gpu_devices( sandbox: &DriverSandbox, config: &PodmanComputeConfig, @@ -883,10 +897,28 @@ pub fn build_container_spec_with_token_and_gpu_devices( gpu_device_ids: Option<&[String]>, ) -> Result { let image = resolve_image(sandbox, config); + build_container_spec_for_image( + sandbox, + config, + token_secret_name, + gpu_device_ids, + image, + "", + ) +} + +pub fn build_container_spec_for_image( + sandbox: &DriverSandbox, + config: &PodmanComputeConfig, + token_secret_name: Option<&str>, + gpu_device_ids: Option<&[String]>, + image_id: &str, + oci_user: &str, +) -> Result { let name = container_name(&sandbox.workspace, &sandbox.name, &sandbox.id); let vol = volume_name(&sandbox.id); - let env = build_env(sandbox, config, image); + let env = build_env(sandbox, config, image_id, oci_user); let labels = build_labels(sandbox); let resource_limits = build_resource_limits(sandbox, config); let user_mounts = podman_user_mounts(sandbox, config.enable_bind_mounts) @@ -932,7 +964,7 @@ pub fn build_container_spec_with_token_and_gpu_devices( let container_spec = ContainerSpec { name, - image: image.to_string(), + image: image_id.to_string(), labels, env, volumes, @@ -1030,7 +1062,7 @@ pub fn build_container_spec_with_token_and_gpu_devices( // locks itself down. no_new_privileges: true, seccomp_profile_path: "unconfined".into(), - image_pull_policy: config.image_pull_policy.as_str().to_string(), + image_pull_policy: "never".to_string(), healthconfig: HealthConfig { test: vec![ "CMD-SHELL".into(), @@ -1325,6 +1357,45 @@ mod tests { ); } + #[test] + fn container_spec_pins_inspected_image_and_protects_oci_identity() { + let mut sandbox = test_sandbox("test-id", "test-name"); + let spec = sandbox.spec.get_or_insert_default(); + for (key, value) in [ + (openshell_core::sandbox_env::OCI_IMAGE_USER, "spoofed"), + (openshell_core::sandbox_env::SANDBOX_UID, "9999"), + (openshell_core::sandbox_env::SANDBOX_GID, "9999"), + ] { + spec.environment.insert(key.to_string(), value.to_string()); + } + + let container = build_container_spec_for_image( + &sandbox, + &test_config(), + None, + None, + "sha256:immutable", + "app:staff", + ) + .unwrap(); + + assert_eq!(container["image"].as_str(), Some("sha256:immutable")); + assert_eq!(container["user"].as_str(), Some("0:0")); + assert_eq!(container["image_pull_policy"].as_str(), Some("never")); + assert_eq!( + container["env"][openshell_core::sandbox_env::OCI_IMAGE_USER].as_str(), + Some("app:staff") + ); + assert_eq!( + container["env"][openshell_core::sandbox_env::SANDBOX_UID].as_str(), + Some("") + ); + assert_eq!( + container["env"][openshell_core::sandbox_env::SANDBOX_GID].as_str(), + Some("") + ); + } + #[test] fn volume_name_uses_id() { assert_eq!( diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 3878f59836..18723ec40c 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -571,6 +571,20 @@ impl PodmanComputeDriver { .pull_image(image, pull_policy) .await .map_err(ComputeDriverError::from)?; + let inspected_image = self + .client + .inspect_image(image) + .await + .map_err(ComputeDriverError::from)?; + if inspected_image.id.is_empty() { + return Err(ComputeDriverError::Precondition(format!( + "podman image '{image}' inspection did not return an immutable image ID" + ))); + } + let image_user = inspected_image + .config + .as_ref() + .map_or("", |config| config.user.as_str()); for image in container::podman_driver_image_mount_sources(sandbox, self.config.enable_bind_mounts) @@ -630,11 +644,13 @@ impl PodmanComputeDriver { return Err(e); } }; - let spec = match container::build_container_spec_with_token_and_gpu_devices( + let spec = match container::build_container_spec_for_image( sandbox, &self.config, token_secret_name.as_deref(), gpu_devices.as_deref(), + &inspected_image.id, + image_user, ) { Ok(spec) => spec, Err(e) => { @@ -1653,6 +1669,10 @@ mod tests { vec![ StubResponse::new(StatusCode::OK, "{}"), // pull supervisor image StubResponse::new(StatusCode::OK, "{}"), // pull sandbox image + StubResponse::new( + StatusCode::OK, + r#"{"Id":"sha256:sandbox","Config":{"User":"1234:1235"}}"#, + ), // inspect sandbox image StubResponse::new(StatusCode::CREATED, "{}"), // create volume StubResponse::new(StatusCode::CREATED, "{}"), // create proxy-auth secret StubResponse::new(StatusCode::INTERNAL_SERVER_ERROR, r#"{"message":"boom"}"#), // create container @@ -1691,6 +1711,10 @@ mod tests { vec![ StubResponse::new(StatusCode::OK, "{}"), // pull supervisor image StubResponse::new(StatusCode::OK, "{}"), // pull sandbox image + StubResponse::new( + StatusCode::OK, + r#"{"Id":"sha256:sandbox","Config":{"User":"1234:1235"}}"#, + ), // inspect sandbox image StubResponse::new(StatusCode::CREATED, "{}"), // create volume StubResponse::new(StatusCode::CREATED, "{}"), // create proxy-auth secret StubResponse::new(StatusCode::CREATED, "{}"), // create container diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index 6c92b1b567..0a4cb96564 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -954,8 +954,7 @@ const SANDBOX_NAME: &str = "sandbox"; /// `u32` within the range `[MIN_SANDBOX_UID, MAX_SANDBOX_UID]`. /// /// Rejects: -/// - The empty string (callers should use `ensure_sandbox_process_identity` -/// to fill defaults before validation) +/// - The empty string (represents an omitted policy field) /// - UID 0 or values below `MIN_SANDBOX_UID` /// - Values above `MAX_SANDBOX_UID` /// - Non-numeric strings other than `"sandbox"` (e.g. `"root"`, `"nobody"`) @@ -1053,9 +1052,10 @@ pub const LEGACY_CONTAINER_POLICY_PATH: &str = "/etc/navigator/policy.yaml"; /// Return a restrictive default policy suitable for sandboxes that have no /// explicit policy configured. /// -/// This policy grants filesystem access to standard system paths, runs as the -/// `sandbox` user, enables Landlock in best-effort mode, and **blocks all -/// network access** (no network policies, no inference routing). +/// This policy grants filesystem access to standard system paths, leaves +/// process identity selection to the compute runtime, enables Landlock in +/// best-effort mode, and **blocks all network access** (no network policies, +/// no inference routing). pub fn restrictive_default_policy() -> SandboxPolicy { SandboxPolicy { version: 1, @@ -1075,30 +1075,12 @@ pub fn restrictive_default_policy() -> SandboxPolicy { landlock: Some(LandlockPolicy { compatibility: "best_effort".into(), }), - process: Some(ProcessPolicy { - run_as_user: "sandbox".into(), - run_as_group: "sandbox".into(), - }), + process: None, network_policies: HashMap::new(), network_middlewares: HashMap::default(), } } -/// Ensure the policy has `run_as_user: sandbox` and `run_as_group: sandbox`. -/// -/// 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(); - } -} - // --------------------------------------------------------------------------- // Policy safety validation // --------------------------------------------------------------------------- @@ -1112,7 +1094,7 @@ 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 not "sandbox". + /// An explicit `run_as_user` or `run_as_group` is unsafe. InvalidProcessIdentity { field: &'static str, value: String }, /// A filesystem path contains `..` components. PathTraversal { path: String }, @@ -1277,7 +1259,7 @@ impl fmt::Display for PolicyViolation { /// error vs. logged warning). /// /// Checks performed: -/// - `run_as_user` / `run_as_group` must be "sandbox" +/// - Explicit `run_as_user` / `run_as_group` fields must be safe identities /// - Filesystem paths must be absolute (start with `/`) /// - Filesystem paths must not contain `..` components /// - Read-write paths must not be overly broad (just `/`) @@ -1292,18 +1274,17 @@ pub fn validate_sandbox_policy( ) -> std::result::Result<(), Vec> { let mut violations = Vec::new(); - // Check process identity — must be "sandbox" or a numeric UID/GID - // within the acceptable sandbox range. - // `ensure_sandbox_process_identity` should be called before this to - // fill in defaults; any invalid value is rejected. + // Omitted process identity fields are resolved by the compute runtime. + // Explicit fields must be "sandbox" or a numeric UID/GID within the + // acceptable sandbox range. if let Some(ref process) = policy.process { - if !is_valid_sandbox_identity(&process.run_as_user) { + if !process.run_as_user.is_empty() && !is_valid_sandbox_identity(&process.run_as_user) { violations.push(PolicyViolation::InvalidProcessIdentity { field: "run_as_user", value: process.run_as_user.clone(), }); } - if !is_valid_sandbox_identity(&process.run_as_group) { + if !process.run_as_group.is_empty() && !is_valid_sandbox_identity(&process.run_as_group) { violations.push(PolicyViolation::InvalidProcessIdentity { field: "run_as_group", value: process.run_as_group.clone(), @@ -1664,11 +1645,9 @@ network_policies: } #[test] - fn restrictive_default_has_process_identity() { + fn restrictive_default_omits_process_identity() { let policy = restrictive_default_policy(); - let proc = policy.process.expect("must have process policy"); - assert_eq!(proc.run_as_user, "sandbox"); - assert_eq!(proc.run_as_group, "sandbox"); + assert!(policy.process.is_none()); } #[test] @@ -1693,6 +1672,22 @@ network_policies: assert!(policy.filesystem.is_none()); } + #[test] + fn process_identity_omission_survives_yaml_round_trip() { + let policy = parse_sandbox_policy("version: 1\nprocess:\n run_as_user: \"1234\"\n") + .expect("partial process identity should parse"); + let process = policy.process.as_ref().expect("process section"); + assert_eq!(process.run_as_user, "1234"); + assert!(process.run_as_group.is_empty()); + assert!(validate_sandbox_policy(&policy).is_ok()); + + let yaml = serialize_sandbox_policy(&policy).expect("partial identity should serialize"); + assert!(yaml.contains("run_as_user")); + assert!(!yaml.contains("run_as_group")); + let reparsed = parse_sandbox_policy(&yaml).expect("round trip should parse"); + assert!(reparsed.process.unwrap().run_as_group.is_empty()); + } + #[test] fn parse_policy_with_network_rules() { let yaml = r" @@ -1819,38 +1814,6 @@ network_policies: assert!(err.to_string().contains("on_parse_error")); } - #[test] - fn ensure_sandbox_process_identity_fills_defaults() { - let mut policy = restrictive_default_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_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] fn container_policy_path_is_expected() { assert_eq!(CONTAINER_POLICY_PATH, "/etc/openshell/policy.yaml"); @@ -2363,14 +2326,25 @@ network_policies: } #[test] - fn validate_rejects_empty_run_as_user() { + fn validate_accepts_omitted_process_fields() { let mut policy = restrictive_default_policy(); policy.process = Some(ProcessPolicy { run_as_user: String::new(), run_as_group: String::new(), }); - let violations = validate_sandbox_policy(&policy).unwrap_err(); - assert_eq!(violations.len(), 2); + assert!(validate_sandbox_policy(&policy).is_ok()); + + policy.process = Some(ProcessPolicy { + run_as_user: "sandbox".into(), + run_as_group: String::new(), + }); + assert!(validate_sandbox_policy(&policy).is_ok()); + + policy.process = Some(ProcessPolicy { + run_as_user: String::new(), + run_as_group: "1234".into(), + }); + assert!(validate_sandbox_policy(&policy).is_ok()); } #[test] diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index e696403f39..1ee013df08 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -184,37 +184,17 @@ pub async fn run_sandbox( .await? }; - // Override the policy's process identity with the driver-resolved UID/GID - // from the pod environment. The policy defaults to the name "sandbox" which - // resolves via /etc/passwd, but the driver may have chosen a different - // numeric UID (e.g. from OpenShift SCC annotations). - // Validate overrides against the same rules as the policy layer to prevent - // env-injected values (e.g. GID 0) from bypassing policy restrictions. - if let Ok(uid) = std::env::var(openshell_core::sandbox_env::SANDBOX_UID) - && !uid.is_empty() - { - if !openshell_policy::is_valid_sandbox_identity(&uid) { - return Err(miette::miette!( - "OPENSHELL_SANDBOX_UID contains invalid sandbox identity '{uid}'; \ - expected 'sandbox' or a numeric UID in range [{}, {}]", - openshell_policy::MIN_SANDBOX_UID, - openshell_policy::MAX_SANDBOX_UID, - )); - } - policy.process.run_as_user = Some(uid); - } - if let Ok(gid) = std::env::var(openshell_core::sandbox_env::SANDBOX_GID) - && !gid.is_empty() + // Normalize the active driver's identity contract once, while both the + // policy and launched image filesystem are available. Kubernetes and + // OpenShift retain their authoritative numeric pair; Docker and Podman + // fill only omitted policy fields from OCI Config.User. + #[cfg(unix)] { - if !openshell_policy::is_valid_sandbox_identity(&gid) { - return Err(miette::miette!( - "OPENSHELL_SANDBOX_GID contains invalid sandbox identity '{gid}'; \ - expected 'sandbox' or a numeric GID in range [{}, {}]", - openshell_policy::MIN_SANDBOX_UID, - openshell_policy::MAX_SANDBOX_UID, - )); - } - policy.process.run_as_group = Some(gid); + let driver_identity = openshell_supervisor_process::identity::DriverIdentity::from_env()?; + openshell_supervisor_process::identity::resolve_process_identity( + &mut policy, + &driver_identity, + )?; } #[cfg_attr(not(target_os = "linux"), allow(unused_mut))] @@ -3367,9 +3347,10 @@ mod tests { let policy = discover_policy_from_path(path); // Restrictive default has no network policies. assert!(policy.network_policies.is_empty()); - // But does have filesystem and process policies. + // It keeps filesystem restrictions while leaving identity to the + // active compute driver. assert!(policy.filesystem.is_some()); - assert!(policy.process.is_some()); + assert!(policy.process.is_none()); } #[test] @@ -3439,9 +3420,7 @@ filesystem_policy: let policy = discover_policy_from_path(&path); // Falls back to restrictive default because of root user. - let proc = policy.process.unwrap(); - assert_eq!(proc.run_as_user, "sandbox"); - assert_eq!(proc.run_as_group, "sandbox"); + assert!(policy.process.is_none()); } #[test] diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 53124261e6..d0b3d63808 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -1735,10 +1735,9 @@ async fn handle_update_config_inner( "delete_setting cannot be combined with policy payload", )); } - let mut new_policy = req.policy.ok_or_else(|| { + let new_policy = req.policy.ok_or_else(|| { Status::invalid_argument("policy is required for global policy update") })?; - openshell_policy::ensure_sandbox_process_identity(&mut new_policy); validate_no_reserved_provider_policy_keys(&new_policy)?; validate_policy_safety(&new_policy)?; crate::middleware::validate_policy(state.middleware_registry.as_ref(), &new_policy) @@ -2097,7 +2096,6 @@ async fn handle_update_config_inner( .as_ref() .ok_or_else(|| Status::internal("sandbox has no spec"))?; - openshell_policy::ensure_sandbox_process_identity(&mut new_policy); if sandbox_caller { if openshell_policy::strip_provider_rule_names(&mut new_policy) { debug!( @@ -12078,7 +12076,13 @@ mod tests { let current_version = current.metadata.as_ref().unwrap().resource_version; // Backfill the policy with correct expected_resource_version - let new_policy = ProtoSandboxPolicy::default(); + let new_policy = ProtoSandboxPolicy { + process: Some(openshell_core::proto::ProcessPolicy { + run_as_user: "1234".to_string(), + run_as_group: String::new(), + }), + ..Default::default() + }; let response = handle_update_config( &state, @@ -12109,6 +12113,14 @@ mod tests { .await .unwrap() .unwrap(); + let process = updated_sandbox + .spec + .as_ref() + .and_then(|spec| spec.policy.as_ref()) + .and_then(|policy| policy.process.as_ref()) + .expect("partial process identity should be persisted"); + assert_eq!(process.run_as_user, "1234"); + assert!(process.run_as_group.is_empty()); assert_eq!( updated_sandbox.metadata.as_ref().unwrap().resource_version, current_version + 1, @@ -12225,8 +12237,7 @@ mod tests { #[tokio::test] async fn update_config_same_policy_hash_with_new_provenance_creates_revision() { let state = test_server_state().await; - let mut policy = test_policy_with_rule("sandbox_only", "sandbox.example.com"); - openshell_policy::ensure_sandbox_process_identity(&mut policy); + let policy = test_policy_with_rule("sandbox_only", "sandbox.example.com"); let hash = deterministic_policy_hash(&policy); let sandbox = test_sandbox("sb-same-hash", "same-hash", policy.clone(), Vec::new()); state.store.put_message(&sandbox).await.unwrap(); @@ -12303,8 +12314,7 @@ mod tests { #[tokio::test] async fn update_config_same_policy_and_provenance_is_idempotent() { let state = test_server_state().await; - let mut policy = test_policy_with_rule("sandbox_only", "sandbox.example.com"); - openshell_policy::ensure_sandbox_process_identity(&mut policy); + let policy = test_policy_with_rule("sandbox_only", "sandbox.example.com"); state .store .put_message(&test_sandbox( @@ -12360,8 +12370,7 @@ mod tests { #[tokio::test] async fn update_config_full_policy_empty_annotations_preserves_existing_annotations() { let state = test_server_state().await; - let mut baseline = test_policy_with_rule("sandbox_only", "old.example.com"); - openshell_policy::ensure_sandbox_process_identity(&mut baseline); + let baseline = test_policy_with_rule("sandbox_only", "old.example.com"); let mut sandbox = test_sandbox( "sb-preserve-full", "preserve-full", @@ -12386,8 +12395,7 @@ mod tests { .await .unwrap(); - let mut updated = test_policy_with_rule("sandbox_only", "new.example.com"); - openshell_policy::ensure_sandbox_process_identity(&mut updated); + let updated = test_policy_with_rule("sandbox_only", "new.example.com"); let response = handle_update_config( &state, with_user(Request::new(UpdateConfigRequest { @@ -12428,8 +12436,7 @@ mod tests { #[tokio::test] async fn update_config_merge_empty_annotations_preserves_existing_annotations() { let state = test_server_state().await; - let mut baseline = test_policy_with_rule("sandbox_only", "sandbox.example.com"); - openshell_policy::ensure_sandbox_process_identity(&mut baseline); + let baseline = test_policy_with_rule("sandbox_only", "sandbox.example.com"); let mut sandbox = test_sandbox("sb-preserve-merge", "preserve-merge", baseline, Vec::new()); sandbox.metadata.as_mut().unwrap().annotations.insert( "openshell.nvidia.com/policy-provenance".to_string(), @@ -12492,8 +12499,7 @@ mod tests { #[tokio::test] async fn update_config_merge_stores_revision_provenance_atomically() { let state = test_server_state().await; - let mut baseline = test_policy_with_rule("sandbox_only", "sandbox.example.com"); - openshell_policy::ensure_sandbox_process_identity(&mut baseline); + let baseline = test_policy_with_rule("sandbox_only", "sandbox.example.com"); state .store .put_message(&test_sandbox( diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index c87fc01436..e6e4fbb069 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -172,10 +172,9 @@ async fn handle_create_sandbox_inner( template.image = state.compute.default_image().to_string(); } - // Ensure process identity defaults to "sandbox" when missing or - // empty, then validate policy safety before persisting. + // Preserve omitted process identity fields so the compute runtime can + // apply its driver-specific fallback. if let Some(ref mut policy) = spec.policy { - openshell_policy::ensure_sandbox_process_identity(policy); validate_no_reserved_provider_policy_keys(policy)?; validate_policy_safety(policy)?; crate::middleware::validate_policy(state.middleware_registry.as_ref(), policy).await?; @@ -2969,6 +2968,69 @@ mod tests { ); } + #[tokio::test] + async fn create_and_get_preserve_partial_process_identity() { + let state = test_server_state().await; + let policy = openshell_core::proto::SandboxPolicy { + version: 1, + process: Some(openshell_core::proto::ProcessPolicy { + run_as_user: String::new(), + run_as_group: "1234".to_string(), + }), + ..Default::default() + }; + + let response = handle_create_sandbox( + &state, + Request::new(CreateSandboxRequest { + name: "partial-id".to_string(), + spec: Some(openshell_core::proto::SandboxSpec { + policy: Some(policy), + ..Default::default() + }), + labels: HashMap::new(), + annotations: HashMap::new(), + workspace: String::new(), + }), + ) + .await + .expect("partial process identity should be accepted") + .into_inner(); + + let created_process = response + .sandbox + .unwrap() + .spec + .unwrap() + .policy + .unwrap() + .process + .unwrap(); + assert!(created_process.run_as_user.is_empty()); + assert_eq!(created_process.run_as_group, "1234"); + + let fetched_process = handle_get_sandbox( + &state, + Request::new(GetSandboxRequest { + name: "partial-id".to_string(), + workspace: String::new(), + }), + ) + .await + .unwrap() + .into_inner() + .sandbox + .unwrap() + .spec + .unwrap() + .policy + .unwrap() + .process + .unwrap(); + assert!(fetched_process.run_as_user.is_empty()); + assert_eq!(fetched_process.run_as_group, "1234"); + } + #[tokio::test] async fn create_sandbox_still_rejects_long_label_values() { let state = test_server_state().await; diff --git a/crates/openshell-supervisor-process/src/identity.rs b/crates/openshell-supervisor-process/src/identity.rs new file mode 100644 index 0000000000..fe78a7344d --- /dev/null +++ b/crates/openshell-supervisor-process/src/identity.rs @@ -0,0 +1,532 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Driver identity normalization and OCI `USER` resolution. + +use miette::{IntoDiagnostic, Result}; +use openshell_core::policy::SandboxPolicy; +use std::fs::{File, OpenOptions}; +use std::io::Read; +use std::os::unix::fs::OpenOptionsExt; +use std::path::Path; + +const PASSWD_PATH: &str = "/etc/passwd"; +const GROUP_PATH: &str = "/etc/group"; +const MAX_ACCOUNT_FILE_SIZE: u64 = 1024 * 1024; +const MAX_ACCOUNT_LINE_SIZE: usize = 8 * 1024; +const MAX_ACCOUNT_FIELD_SIZE: usize = 1024; + +/// Identity input selected by the active compute driver. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum DriverIdentity { + /// Platform-selected identity used by Kubernetes and `OpenShift`. + Resolved { uid: u32, gid: u32 }, + /// Raw OCI `Config.User` selected by Docker and Podman. + OciUser { declaration: String }, + /// Drivers with no authoritative identity metadata. + None, +} + +impl DriverIdentity { + /// Normalize the protected driver environment into one identity variant. + pub fn from_env() -> Result { + let oci_user = optional_utf8_env(openshell_core::sandbox_env::OCI_IMAGE_USER)?; + let uid = optional_nonempty_utf8_env(openshell_core::sandbox_env::SANDBOX_UID)?; + let gid = optional_nonempty_utf8_env(openshell_core::sandbox_env::SANDBOX_GID)?; + Self::from_values(oci_user, uid, gid) + } + + fn from_values( + oci_user: Option, + uid: Option, + gid: Option, + ) -> Result { + match (oci_user, uid, gid) { + (Some(declaration), None, None) => Ok(Self::OciUser { declaration }), + (None, Some(uid), Some(gid)) => { + let uid = uid.parse::().ok().filter(|uid| { + (openshell_policy::MIN_SANDBOX_UID..=openshell_policy::MAX_SANDBOX_UID) + .contains(uid) + }); + let gid = gid.parse::().ok().filter(|gid| { + (openshell_policy::MIN_SANDBOX_UID..=openshell_policy::MAX_SANDBOX_UID) + .contains(gid) + }); + let (Some(uid), Some(gid)) = (uid, gid) else { + return Err(miette::miette!( + "driver UID/GID must be numeric identities in range [{}, {}]", + openshell_policy::MIN_SANDBOX_UID, + openshell_policy::MAX_SANDBOX_UID + )); + }; + Ok(Self::Resolved { uid, gid }) + } + (None, None, None) => Ok(Self::None), + (Some(_), _, _) => Err(miette::miette!( + "{} conflicts with non-empty {}/{} driver identity", + openshell_core::sandbox_env::OCI_IMAGE_USER, + openshell_core::sandbox_env::SANDBOX_UID, + openshell_core::sandbox_env::SANDBOX_GID + )), + (None, _, _) => Err(miette::miette!( + "{} and {} must be supplied together", + openshell_core::sandbox_env::SANDBOX_UID, + openshell_core::sandbox_env::SANDBOX_GID + )), + } + } +} + +/// Apply a driver identity before any workload child becomes reachable. +pub fn resolve_process_identity( + policy: &mut SandboxPolicy, + driver_identity: &DriverIdentity, +) -> Result<()> { + match driver_identity { + DriverIdentity::Resolved { uid, gid } => { + policy.process.run_as_user = Some(uid.to_string()); + policy.process.run_as_group = Some(gid.to_string()); + Ok(()) + } + DriverIdentity::OciUser { declaration } => resolve_oci_process_identity_at( + policy, + declaration, + Path::new(PASSWD_PATH), + Path::new(GROUP_PATH), + ), + DriverIdentity::None => Ok(()), + } +} + +fn resolve_oci_process_identity_at( + policy: &mut SandboxPolicy, + declaration: &str, + passwd_path: &Path, + group_path: &Path, +) -> Result<()> { + let explicit_user = policy + .process + .run_as_user + .as_deref() + .filter(|value| !value.is_empty()); + let explicit_group = policy + .process + .run_as_group + .as_deref() + .filter(|value| !value.is_empty()); + + if let (Some(user), Some(group)) = (explicit_user, explicit_group) { + let uid = resolve_user(user, passwd_path)?; + let gid = resolve_group(group, group_path)?; + install_numeric_identity(policy, uid, gid, "explicit policy")?; + return Ok(()); + } + + let (oci_user, oci_group) = split_oci_declaration(declaration); + let uid = match explicit_user { + Some(user) => resolve_user(user, passwd_path)?, + None => resolve_required_oci_user(oci_user, passwd_path, declaration)?.0, + }; + let gid = match explicit_group { + Some(group) => resolve_group(group, group_path)?, + None => match oci_group { + Some(group) if !group.is_empty() => resolve_group(group, group_path)?, + Some(_) => { + return Err(miette::miette!( + "OCI USER '{declaration}' has an empty group component" + )); + } + None => resolve_required_oci_user(oci_user, passwd_path, declaration)? + .1 + .ok_or_else(|| { + miette::miette!( + "OCI USER '{declaration}' uses a numeric UID without an explicit group, \ + but /etc/passwd has no matching primary GID" + ) + })?, + }, + }; + + install_numeric_identity(policy, uid, gid, declaration) +} + +fn split_oci_declaration(declaration: &str) -> (&str, Option<&str>) { + declaration + .split_once(':') + .map_or((declaration, None), |(user, group)| (user, Some(group))) +} + +fn resolve_required_oci_user( + user: &str, + passwd_path: &Path, + declaration: &str, +) -> Result<(u32, Option)> { + if user.is_empty() { + return Err(miette::miette!( + "OCI USER is required because run_as_user is omitted" + )); + } + validate_component(user, "OCI user")?; + if user == "root" { + return Err(miette::miette!("OCI USER '{declaration}' selects root")); + } + if let Ok(uid) = user.parse::() { + if uid == 0 { + return Err(miette::miette!("OCI USER '{declaration}' selects UID 0")); + } + let primary_gid = find_passwd_by_uid(passwd_path, uid)?.map(|entry| entry.gid); + return Ok((uid, primary_gid)); + } + let entry = find_passwd_by_name(passwd_path, user)? + .ok_or_else(|| miette::miette!("OCI USER name '{user}' was not found in /etc/passwd"))?; + Ok((entry.uid, Some(entry.gid))) +} + +fn resolve_user(value: &str, passwd_path: &Path) -> Result { + validate_component(value, "user")?; + if value == "root" { + return Err(miette::miette!("process user must not select root")); + } + if let Ok(uid) = value.parse::() { + return Ok(uid); + } + Ok(find_passwd_by_name(passwd_path, value)? + .ok_or_else(|| miette::miette!("process user '{value}' was not found in /etc/passwd"))? + .uid) +} + +fn resolve_group(value: &str, group_path: &Path) -> Result { + validate_component(value, "group")?; + if value == "root" { + return Err(miette::miette!("process group must not select root")); + } + if let Ok(gid) = value.parse::() { + return Ok(gid); + } + Ok(find_group_by_name(group_path, value)? + .ok_or_else(|| miette::miette!("process group '{value}' was not found in /etc/group"))? + .gid) +} + +fn install_numeric_identity( + policy: &mut SandboxPolicy, + uid: u32, + gid: u32, + source: &str, +) -> Result<()> { + if uid == 0 || gid == 0 { + return Err(miette::miette!( + "process identity from '{source}' resolves to prohibited UID/GID {uid}:{gid}" + )); + } + policy.process.run_as_user = Some(uid.to_string()); + policy.process.run_as_group = Some(gid.to_string()); + Ok(()) +} + +fn validate_component(value: &str, kind: &str) -> Result<()> { + if value.is_empty() + || value.len() > MAX_ACCOUNT_FIELD_SIZE + || value.trim() != value + || value.chars().any(|ch| ch.is_control() || ch == ':') + { + return Err(miette::miette!("{kind} component '{value}' is malformed")); + } + Ok(()) +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct PasswdEntry { + uid: u32, + gid: u32, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct GroupEntry { + gid: u32, +} + +fn find_passwd_by_name(path: &Path, name: &str) -> Result> { + find_unique(path, |fields| { + (fields.first().copied() == Some(name)).then(|| parse_passwd(fields)) + }) +} + +fn find_passwd_by_uid(path: &Path, uid: u32) -> Result> { + find_unique(path, |fields| { + fields + .get(2) + .and_then(|value| value.parse::().ok()) + .filter(|candidate| *candidate == uid) + .map(|_| parse_passwd(fields)) + }) +} + +fn find_group_by_name(path: &Path, name: &str) -> Result> { + find_unique(path, |fields| { + (fields.first().copied() == Some(name)).then(|| parse_group(fields)) + }) +} + +fn find_unique( + path: &Path, + mut select: impl FnMut(&[&str]) -> Option>, +) -> Result> { + let content = read_account_file(path)?; + let mut found = None; + for line in content.lines() { + if line.is_empty() || line.starts_with('#') { + continue; + } + if line.len() > MAX_ACCOUNT_LINE_SIZE { + return Err(miette::miette!( + "account file '{}' contains an oversized line", + path.display() + )); + } + let fields = line.split(':').collect::>(); + if fields + .iter() + .any(|field| field.len() > MAX_ACCOUNT_FIELD_SIZE) + { + return Err(miette::miette!( + "account file '{}' contains an oversized field", + path.display() + )); + } + let Some(candidate) = select(&fields) else { + continue; + }; + let candidate = candidate?; + if found.replace(candidate).is_some() { + return Err(miette::miette!( + "account identity is ambiguous in '{}'", + path.display() + )); + } + } + Ok(found) +} + +fn parse_passwd(fields: &[&str]) -> Result { + if fields.len() != 7 { + return Err(miette::miette!("matching /etc/passwd entry is malformed")); + } + Ok(PasswdEntry { + uid: fields[2] + .parse() + .map_err(|_| miette::miette!("matching /etc/passwd UID is malformed"))?, + gid: fields[3] + .parse() + .map_err(|_| miette::miette!("matching /etc/passwd GID is malformed"))?, + }) +} + +fn parse_group(fields: &[&str]) -> Result { + if fields.len() != 4 { + return Err(miette::miette!("matching /etc/group entry is malformed")); + } + Ok(GroupEntry { + gid: fields[2] + .parse() + .map_err(|_| miette::miette!("matching /etc/group GID is malformed"))?, + }) +} + +fn read_account_file(path: &Path) -> Result { + let mut options = OpenOptions::new(); + options + .read(true) + .custom_flags(libc::O_CLOEXEC | libc::O_NOFOLLOW); + let mut file = options + .open(path) + .into_diagnostic() + .map_err(|error| miette::miette!("failed to open '{}': {error}", path.display()))?; + validate_account_file(&file, path)?; + + let mut bytes = Vec::new(); + file.by_ref() + .take(MAX_ACCOUNT_FILE_SIZE + 1) + .read_to_end(&mut bytes) + .into_diagnostic()?; + if bytes.len() as u64 > MAX_ACCOUNT_FILE_SIZE { + return Err(miette::miette!( + "account file '{}' exceeds {MAX_ACCOUNT_FILE_SIZE} bytes", + path.display() + )); + } + String::from_utf8(bytes) + .map_err(|_| miette::miette!("account file '{}' is not valid UTF-8", path.display())) +} + +fn validate_account_file(file: &File, path: &Path) -> Result<()> { + let metadata = file.metadata().into_diagnostic()?; + if !metadata.is_file() { + return Err(miette::miette!( + "account path '{}' is not a regular file", + path.display() + )); + } + if metadata.len() > MAX_ACCOUNT_FILE_SIZE { + return Err(miette::miette!( + "account file '{}' exceeds {MAX_ACCOUNT_FILE_SIZE} bytes", + path.display() + )); + } + Ok(()) +} + +fn optional_utf8_env(name: &str) -> Result> { + std::env::var_os(name) + .map(|value| { + value + .into_string() + .map_err(|_| miette::miette!("{name} is not valid UTF-8")) + }) + .transpose() +} + +fn optional_nonempty_utf8_env(name: &str) -> Result> { + Ok(optional_utf8_env(name)?.filter(|value| !value.is_empty())) +} + +#[cfg(test)] +mod tests { + use super::*; + use openshell_core::policy::SandboxPolicy; + use std::fs; + use tempfile::tempdir; + + fn account_files( + passwd: &str, + group: &str, + ) -> (tempfile::TempDir, std::path::PathBuf, std::path::PathBuf) { + let dir = tempdir().unwrap(); + let passwd_path = dir.path().join("passwd"); + let group_path = dir.path().join("group"); + fs::write(&passwd_path, passwd).unwrap(); + fs::write(&group_path, group).unwrap(); + (dir, passwd_path, group_path) + } + + fn policy(user: Option<&str>, group: Option<&str>) -> SandboxPolicy { + let mut policy = SandboxPolicy { + version: 1, + filesystem: openshell_core::policy::FilesystemPolicy::default(), + network: openshell_core::policy::NetworkPolicy::default(), + landlock: openshell_core::policy::LandlockPolicy::default(), + process: openshell_core::policy::ProcessPolicy::default(), + }; + policy.process.run_as_user = user.map(str::to_string); + policy.process.run_as_group = group.map(str::to_string); + policy + } + + #[test] + fn per_field_policy_precedence_resolves_complete_pair() { + let (_dir, passwd, group) = account_files( + "app:x:1234:1235::/home/app:/bin/sh\nsandbox:x:2000:2001::/sandbox:/bin/sh\n", + "staff:x:1235:\nsandbox:x:2001:\n", + ); + let cases = [ + (Some("2000"), Some("2001"), "root", "2000", "2001"), + (Some("2000"), None, "app:staff", "2000", "1235"), + (None, Some("2001"), "app:root", "1234", "2001"), + (None, None, "app", "1234", "1235"), + ]; + for (user, group_name, declaration, expected_uid, expected_gid) in cases { + let mut policy = policy(user, group_name); + resolve_oci_process_identity_at(&mut policy, declaration, &passwd, &group).unwrap(); + assert_eq!(policy.process.run_as_user.as_deref(), Some(expected_uid)); + assert_eq!(policy.process.run_as_group.as_deref(), Some(expected_gid)); + } + } + + #[test] + fn numeric_pair_does_not_require_account_entries() { + let (_dir, passwd, group) = account_files("", ""); + let mut policy = policy(None, None); + resolve_oci_process_identity_at(&mut policy, "1234:1235", &passwd, &group).unwrap(); + assert_eq!(policy.process.run_as_user.as_deref(), Some("1234")); + assert_eq!(policy.process.run_as_group.as_deref(), Some("1235")); + } + + #[test] + fn driver_identity_inputs_are_mutually_exclusive_and_complete() { + assert_eq!( + DriverIdentity::from_values(Some("app".into()), None, None).unwrap(), + DriverIdentity::OciUser { + declaration: "app".into() + } + ); + assert_eq!( + DriverIdentity::from_values(None, Some("1234".into()), Some("1235".into())).unwrap(), + DriverIdentity::Resolved { + uid: 1234, + gid: 1235 + } + ); + assert_eq!( + DriverIdentity::from_values(None, None, None).unwrap(), + DriverIdentity::None + ); + assert!( + DriverIdentity::from_values( + Some("app".into()), + Some("1234".into()), + Some("1235".into()) + ) + .is_err() + ); + assert!(DriverIdentity::from_values(None, Some("1234".into()), None).is_err()); + } + + #[test] + fn numeric_uid_uses_passwd_primary_gid() { + let (_dir, passwd, group) = account_files("app:x:1234:4321::/home/app:/bin/sh\n", ""); + let mut policy = policy(None, None); + resolve_oci_process_identity_at(&mut policy, "1234", &passwd, &group).unwrap(); + assert_eq!(policy.process.run_as_group.as_deref(), Some("4321")); + } + + #[test] + fn missing_unknown_ambiguous_and_root_identities_fail() { + let (_dir, passwd, group) = account_files( + "app:x:1234:1235::/home/app:/bin/sh\napp:x:2234:2235::/home/app2:/bin/sh\n", + "staff:x:1235:\nstaff:x:2235:\n", + ); + for declaration in ["", "unknown", "app", "9999", "0:1235", "1234:0"] { + let mut policy = policy(None, None); + assert!( + resolve_oci_process_identity_at(&mut policy, declaration, &passwd, &group).is_err(), + "{declaration:?} unexpectedly resolved" + ); + } + } + + #[test] + fn selected_component_is_validated_independently() { + let (_dir, passwd, group) = + account_files("app:x:1234:1235::/home/app:/bin/sh\n", "staff:x:1235:\n"); + + let mut explicit_user = policy(Some("1234"), None); + resolve_oci_process_identity_at(&mut explicit_user, "root:staff", &passwd, &group).unwrap(); + + let mut explicit_group = policy(None, Some("1235")); + resolve_oci_process_identity_at(&mut explicit_group, "app:root", &passwd, &group).unwrap(); + } + + #[cfg(unix)] + #[test] + fn account_file_symlinks_are_rejected() { + use std::os::unix::fs::symlink; + + let (_dir, passwd, group) = + account_files("app:x:1234:1235::/home/app:/bin/sh\n", "staff:x:1235:\n"); + let link = passwd.with_file_name("passwd-link"); + symlink(&passwd, &link).unwrap(); + + let mut policy = policy(None, None); + assert!(resolve_oci_process_identity_at(&mut policy, "app:staff", &link, &group).is_err()); + } +} diff --git a/crates/openshell-supervisor-process/src/lib.rs b/crates/openshell-supervisor-process/src/lib.rs index 842b62f9df..ca93230929 100644 --- a/crates/openshell-supervisor-process/src/lib.rs +++ b/crates/openshell-supervisor-process/src/lib.rs @@ -10,6 +10,8 @@ pub mod child_env; pub mod debug_rpc; +#[cfg(unix)] +pub mod identity; pub mod log_push; pub mod managed_children; pub mod process; diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 3733c7c7e3..ea5b5e110a 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -71,6 +71,9 @@ pub(crate) fn prepare_child_sandbox( } const SUPERVISOR_ONLY_ENV_VARS: &[&str] = &[ + openshell_core::sandbox_env::OCI_IMAGE_USER, + openshell_core::sandbox_env::SANDBOX_UID, + openshell_core::sandbox_env::SANDBOX_GID, openshell_core::sandbox_env::SANDBOX_TOKEN, openshell_core::sandbox_env::SANDBOX_TOKEN_FILE, openshell_core::sandbox_env::K8S_SA_TOKEN_FILE, @@ -855,15 +858,10 @@ impl Drop for ProcessHandle { } } -/// Validate that the configured sandbox identity exists in this image. +/// Validate the configured process user. /// -/// When the identity is the literal `"sandbox"`, verifies the user exists -/// in `/etc/passwd` (all sandbox images ship with one). -/// -/// When the identity is a numeric UID, skips the passwd lookup entirely — -/// the kernel will use the resolved UID regardless of whether an entry -/// exists in `/etc/passwd`. Logs an OCSF event confirming numeric UID usage. -/// Non-numeric, non-"sandbox" values are rejected. +/// Numeric identities do not require a passwd entry. The legacy explicit +/// `"sandbox"` identity and other names must resolve in `/etc/passwd`. #[cfg(unix)] pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { let identity = policy.process.run_as_user.as_deref().unwrap_or("sandbox"); @@ -883,7 +881,7 @@ pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { return Ok(()); } - // "sandbox" name — must exist in /etc/passwd. + // Legacy explicit "sandbox" name — must exist in /etc/passwd. if identity == "sandbox" { match User::from_name("sandbox") { Ok(Some(_)) => { @@ -898,8 +896,7 @@ pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { } Ok(None) => { return Err(miette::miette!( - "sandbox user 'sandbox' not found in image; \ - all sandbox images must include a 'sandbox' user and group" + "explicit process user 'sandbox' was not found in the image" )); } Err(e) => { @@ -907,15 +904,11 @@ pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { } } } else if !identity.is_empty() { - // Non-numeric, non-sandbox string — attempt passwd lookup. - // This catches cases where someone accidentally put "root" or similar. + // Other names are supported by local/offline policy paths and must + // resolve before privilege dropping. match User::from_name(identity) { Ok(Some(_)) => { - tracing::warn!( - identity, - "non-sandbox user accepted via passwd entry; \ - consider using a numeric UID for UID-injected images" - ); + tracing::warn!(identity, "named process user accepted via passwd entry"); } Ok(None) => { return Err(miette::miette!( @@ -936,9 +929,7 @@ pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { /// Validate that the configured sandbox group identity is acceptable. /// -/// Mirrors [`validate_sandbox_user`] for the group dimension: numeric GIDs -/// must fall within the allowed sandbox range, the literal `"sandbox"` must -/// resolve via `/etc/group`, and unrecognised strings are rejected. +/// Mirrors [`validate_sandbox_user`] for the group dimension. #[cfg(unix)] pub fn validate_sandbox_group(policy: &SandboxPolicy) -> Result<()> { let identity = policy.process.run_as_group.as_deref().unwrap_or("sandbox"); @@ -971,8 +962,7 @@ pub fn validate_sandbox_group(policy: &SandboxPolicy) -> Result<()> { } Ok(None) => { return Err(miette::miette!( - "sandbox group 'sandbox' not found in image; \ - all sandbox images must include a 'sandbox' user and group" + "explicit process group 'sandbox' was not found in the image" )); } Err(e) => { @@ -982,11 +972,7 @@ pub fn validate_sandbox_group(policy: &SandboxPolicy) -> Result<()> { } else if !identity.is_empty() { match Group::from_name(identity) { Ok(Some(_)) => { - tracing::warn!( - identity, - "non-sandbox group accepted via group entry; \ - consider using a numeric GID for GID-injected images" - ); + tracing::warn!(identity, "named process group accepted via group entry"); } Ok(None) => { return Err(miette::miette!( @@ -1338,8 +1324,7 @@ pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> { // If no user/group is configured and we are running as root, fall back to // "sandbox:sandbox" instead of silently keeping root. This covers the - // local/dev-mode path where policies are loaded from disk and never pass - // through the server-side `ensure_sandbox_process_identity` normalization. + // local/dev-mode path for drivers that provide no identity metadata. // For non-root runtimes, the no-op is safe -- we are already unprivileged. if user_name.is_none() && group_name.is_none() { if nix::unistd::geteuid().is_root() { diff --git a/crates/openshell-supervisor-process/src/run.rs b/crates/openshell-supervisor-process/src/run.rs index ba6d446dea..1eb58857b2 100644 --- a/crates/openshell-supervisor-process/src/run.rs +++ b/crates/openshell-supervisor-process/src/run.rs @@ -72,17 +72,15 @@ pub async fn run_process( >, #[cfg(target_os = "linux")] bypass_activity_tx: Option, ) -> Result { - // When a driver injects a custom UID/GID, update /etc/passwd and - // /etc/group so the "sandbox" entry matches. Must run before - // validate_sandbox_user so passwd lookups see the correct identity. + // Platform drivers with a resolved numeric UID/GID retain the legacy + // account-file update. OCI-image identity leaves those environment values + // empty, so the image's account files remain unchanged. #[cfg(unix)] if enforcement_mode.uses_privileged_process_setup() { crate::process::update_sandbox_passwd_entries()?; } - // Validate that the sandbox user exists in the image. All sandbox images - // must include a "sandbox" user for privilege dropping; failing fast here - // beats silently running children as root. + // Validate the completed process identity before exposing a child. #[cfg(unix)] if enforcement_mode.uses_privileged_process_setup() { crate::process::validate_sandbox_user(policy)?; diff --git a/docs/reference/policy-schema.mdx b/docs/reference/policy-schema.mdx index d585517d13..b1da42ec3f 100644 --- a/docs/reference/policy-schema.mdx +++ b/docs/reference/policy-schema.mdx @@ -119,17 +119,21 @@ Sets the OS-level identity for the agent process inside the sandbox. | Field | Type | Required | Description | |---|---|---|---| -| `run_as_user` | string | No | The user name or UID the agent process runs as. Default: `sandbox`. | -| `run_as_group` | string | No | The group name or GID the agent process runs as. Default: `sandbox`. | +| `run_as_user` | string | No | Overrides the user name or UID selected by the compute driver. Docker and Podman fall back to the image's OCI `USER`. | +| `run_as_group` | string | No | Overrides the group name or GID selected by the compute driver. Docker and Podman fall back to the image's OCI `USER`. | **Validation constraint:** Neither `run_as_user` nor `run_as_group` can be set to `root` or `0`. Policies that request root process identity are rejected at creation or update time. +Omission is preserved independently for each field. For example, setting only +`run_as_user` keeps that explicit user while allowing the active driver to +select the group. + Example: ```yaml showLineNumbers={false} process: - run_as_user: sandbox - run_as_group: sandbox + run_as_user: "1500" + run_as_group: "1500" ``` ## Network Policies diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index a616823eea..a535cc258d 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -415,7 +415,26 @@ image. ## Sandbox User Identity -OpenShell accepts both the hardcoded username `"sandbox"` and numeric UIDs in `[1000, 2_000_000_000]` for the supervisor's process identity (the policy's `run_as_user` field). The driver resolves the UID at sandbox creation time and passes it to the supervisor via environment variables. +The policy can set `process.run_as_user` and `process.run_as_group` +independently. Each explicit field wins. The active compute driver supplies the +identity for omitted fields. + +### Docker / Podman + +Docker and Podman inspect the final image and use its OCI `USER` declaration as +a per-field fallback. Supported forms include `app`, `app:staff`, a numeric UID +whose passwd entry supplies its primary GID, and an accountless numeric pair +such as `1234:1235`. + +The driver pins container creation to the immutable image ID it inspected. The +supervisor resolves any required names inside that image, then uses the numeric +UID/GID for both direct and SSH children. It does not modify `/etc/passwd` or +`/etc/group`. + +Sandbox creation fails before readiness if a required `USER` component is +missing, malformed, unknown, ambiguous, or resolves to UID/GID 0. An image +without `USER` therefore works only when policy explicitly provides both +identity fields. ### Kubernetes / OpenShift @@ -438,4 +457,7 @@ The VM driver injects the sandbox UID into the rootfs guest's `/etc/passwd`, `/e ### Custom Images -Custom sandbox images no longer need a baked-in `"sandbox"` user. If your image requires a passwd entry for tools like `sudo` or `ssh`, add one manually (e.g. `RUN useradd -m -u 1500 deploy`). The supervisor resolves the numeric UID directly via `setuid()` without needing `/etc/passwd`. +Docker and Podman custom images do not need a baked-in `"sandbox"` user. Declare +a non-root OCI `USER`, or set both process identity fields explicitly in policy. +Named image users require matching account entries; a numeric `UID:GID` pair +does not. diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index 990f260cea..a459c22cf6 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -26,10 +26,10 @@ filesystem_policy: landlock: compatibility: best_effort -# Static: Unprivileged user/group the agent process runs as. -process: - run_as_user: sandbox - run_as_group: sandbox +# Static, optional: override the identity selected by the compute driver. +# process: +# run_as_user: "1500" +# run_as_group: "1500" # Dynamic: hot-reloadable. Named blocks of endpoints + binaries allowed to reach them. network_policies: @@ -68,7 +68,7 @@ Raw streams are connection-scoped and outside L7 live-reload guarantees. This in |---|---|---| | `filesystem_policy` | Static | Controls which directories the agent can access on disk. Paths are split into `read_only` and `read_write` lists. Any path not listed in either list is inaccessible. Set `include_workdir: true` to automatically add the agent's working directory to `read_write`. [Landlock LSM](https://docs.kernel.org/security/landlock.html) enforces these restrictions at the kernel level. | | `landlock` | Static | Configures Landlock LSM enforcement behavior. Set `compatibility` to `best_effort` (skip individual inaccessible paths while applying remaining rules) or `hard_requirement` (fail if any path is inaccessible or the required kernel ABI is unavailable). Refer to the [Policy Schema Reference](/reference/policy-schema#landlock) for the full behavior table. | -| `process` | Static | Sets the OS-level identity for the agent process. `run_as_user` and `run_as_group` default to `sandbox`. Root (`root` or `0`) is rejected. The agent also runs with seccomp filters that block dangerous system calls. | +| `process` | Static | Optionally overrides the OS-level identity for the agent process. Docker and Podman fall back per field to the image's OCI `USER`; Kubernetes uses its platform-selected numeric identity. Root (`root` or `0`) is rejected. The agent also runs with seccomp filters that block dangerous system calls. | | `network_policies` | Dynamic | Controls network access for ordinary outbound traffic from the sandbox. Each block has a name, a list of endpoints (host, port, protocol, and optional rules), and a list of binaries allowed to use those endpoints.
Every outbound connection except `https://inference.local` goes through the proxy, which queries the [policy engine](/about/how-it-works#core-components) with the destination and calling binary. A connection is allowed only when both match an entry in the same policy block.
For endpoints with `protocol: rest`, the proxy auto-detects TLS and terminates it so each HTTP request can be checked against that endpoint's `rules` (method and path). For endpoints with `protocol: websocket`, the proxy validates the RFC 6455 upgrade and evaluates `GET` rules for the handshake plus either `WEBSOCKET_TEXT` rules for raw client text messages or GraphQL operation rules for GraphQL-over-WebSocket messages. Set `websocket_credential_rewrite: true` only when a WebSocket or REST compatibility endpoint must keep placeholder credentials in sandbox-owned text frames and resolve them at the OpenShell relay boundary.
Endpoints without `protocol` allow the TCP stream through without inspecting payloads.
If no endpoint matches, the connection is denied. Configure managed inference separately through [Inference Routing](/sandboxes/inference-routing). | | `network_middlewares` | Dynamic | Declares keyed HTTP request middleware configs. After network and L7 policy admit a request, OpenShell matches each config's host selectors independently and runs matching entries by their unique ascending `order` before credential injection. | diff --git a/docs/security/best-practices.mdx b/docs/security/best-practices.mdx index b63883c2b8..2653a17ea4 100644 --- a/docs/security/best-practices.mdx +++ b/docs/security/best-practices.mdx @@ -201,10 +201,10 @@ The sandbox process runs as a non-root user after explicit privilege dropping. | Aspect | Detail | |---|---| -| Default | `run_as_user: sandbox`, `run_as_group: sandbox`. The supervisor calls `setuid()`/`setgid()` with post-condition verification, disables core dumps with `RLIMIT_CORE=0`, and on Linux sets `PR_SET_DUMPABLE=0`. | -| What you can change | Set `run_as_user` and `run_as_group` in the `process` section. Validation rejects root (`root` or `0`). | +| Default | The compute driver selects a non-root identity. Docker and Podman use the image's OCI `USER` as a per-field fallback. The supervisor calls `setuid()`/`setgid()` with post-condition verification, disables core dumps with `RLIMIT_CORE=0`, and on Linux sets `PR_SET_DUMPABLE=0`. | +| What you can change | Set either or both `run_as_user` and `run_as_group` fields in the `process` section. Each explicit field takes precedence. Validation rejects root (`root` or `0`). | | Risk if relaxed | Running as a higher-privilege user increases the impact of container escape vulnerabilities. | -| Recommendation | Keep the `sandbox` user. Do not attempt to set root. | +| Recommendation | Use a dedicated non-root image identity or explicit numeric policy identity. Do not attempt to set root. | ### Seccomp Filters diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index fa905bbf19..a1f56bbb2f 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -21,17 +21,24 @@ const DOCKERFILE_CONTENT: &str = r#"FROM public.ecr.aws/docker/library/python:3. RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ && rm -rf /var/lib/apt/lists/* -# Create the sandbox user/group so the supervisor can switch to it. -# Use a high UID range to avoid conflicts with host users when running without -# user namespace remapping (UID in container = UID on host). -RUN groupadd -g 1000660000 sandbox && \ - useradd -m -u 1000660000 -g sandbox sandbox +RUN groupadd -g 1235 appstaff && \ + useradd -m -u 1234 -g appstaff app # Write a marker file so we can verify this is our custom image. # Place under /etc (Landlock baseline read-only path) so the sandbox # can read it when filesystem restrictions are properly enforced. RUN echo "custom-image-e2e-marker" > /etc/marker.txt +USER app +CMD ["sleep", "infinity"] +"#; + +const NUMERIC_DOCKERFILE_CONTENT: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim + +RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ + && rm -rf /var/lib/apt/lists/* + +USER 2345:2346 CMD ["sleep", "infinity"] "#; @@ -52,10 +59,16 @@ async fn sandbox_from_custom_dockerfile() { // Step 2: Create a sandbox from the Dockerfile. let dockerfile_str = dockerfile_path.to_str().expect("Dockerfile path is UTF-8"); - let mut guard = - SandboxGuard::create(&["--from", dockerfile_str, "--", "cat", "/etc/marker.txt"]) - .await - .expect("sandbox create from Dockerfile"); + let mut guard = SandboxGuard::create(&[ + "--from", + dockerfile_str, + "--", + "sh", + "-c", + "id -u; id -g; touch /sandbox/oci-user-write; cat /etc/marker.txt", + ]) + .await + .expect("sandbox create from Dockerfile"); // Step 3: Verify the marker file content appears in the output. let clean_output = strip_ansi(&guard.create_output); @@ -63,7 +76,43 @@ async fn sandbox_from_custom_dockerfile() { clean_output.contains(MARKER), "expected marker '{MARKER}' in sandbox output:\n{clean_output}" ); + assert!( + clean_output.contains("1234") && clean_output.contains("1235"), + "expected named OCI identity 1234:1235 in sandbox output:\n{clean_output}" + ); // Explicit cleanup (also happens in Drop, but explicit is clearer in tests). guard.cleanup().await; } + +/// A numeric OCI user/group pair works without passwd or group entries. +#[tokio::test] +async fn sandbox_from_passwd_less_numeric_oci_user() { + let tmpdir = tempfile::tempdir().expect("create tmpdir"); + let dockerfile_path = tmpdir.path().join("Dockerfile"); + { + let mut f = std::fs::File::create(&dockerfile_path).expect("create Dockerfile"); + f.write_all(NUMERIC_DOCKERFILE_CONTENT.as_bytes()) + .expect("write Dockerfile"); + } + + let dockerfile_str = dockerfile_path.to_str().expect("Dockerfile path is UTF-8"); + let mut guard = SandboxGuard::create(&[ + "--from", + dockerfile_str, + "--", + "sh", + "-c", + "id -u; id -g; touch /sandbox/numeric-oci-user-write", + ]) + .await + .expect("sandbox create from numeric OCI Dockerfile"); + + let clean_output = strip_ansi(&guard.create_output); + assert!( + clean_output.contains("2345") && clean_output.contains("2346"), + "expected numeric OCI identity 2345:2346 in sandbox output:\n{clean_output}" + ); + + guard.cleanup().await; +} diff --git a/examples/bring-your-own-container/Dockerfile b/examples/bring-your-own-container/Dockerfile index fc65bd6956..6e5d3492fa 100644 --- a/examples/bring-your-own-container/Dockerfile +++ b/examples/bring-your-own-container/Dockerfile @@ -14,15 +14,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ curl iproute2 nftables \ && rm -rf /var/lib/apt/lists/* -# The sandbox user is injected at runtime by the compute driver. -# Kubernetes: resolved from OpenShift SCC namespace annotations or explicit -# sandbox_uid config. VM: resolves to 10001 by default, configurable in -# gateway TOML. -# -# Images no longer need a baked-in "sandbox" user — numeric UIDs are accepted -# and the driver passes them directly to setuid()/chown() at sandbox start. -# If your image requires a passwd entry for tools like ssh or sudo, add one -# manually (e.g. RUN useradd -m -u 1500 deploy). +RUN groupadd --gid 1500 app \ + && useradd --uid 1500 --gid app --create-home app RUN install -d /sandbox WORKDIR /sandbox @@ -30,6 +23,11 @@ COPY app.py . EXPOSE 8080 +# Docker and Podman use this non-root identity when policy omits either process +# identity field. OpenShell starts the supervisor as root and drops only agent +# children to this account. +USER app + # NOTE: The sandbox supervisor replaces CMD at runtime. Pass the start # command explicitly: openshell sandbox create ... -- python /sandbox/app.py CMD ["python", "app.py"] diff --git a/examples/bring-your-own-container/README.md b/examples/bring-your-own-container/README.md index ea4f1cb9e6..dfb97b0be9 100644 --- a/examples/bring-your-own-container/README.md +++ b/examples/bring-your-own-container/README.md @@ -59,17 +59,14 @@ key requirements are: - **Pass your start command explicitly** — use `-- ` on the CLI. The image's `CMD` / `ENTRYPOINT` is replaced by the sandbox supervisor at runtime. -- **Create a `sandbox` user** (uid/gid 1000660000) for non-root execution. - Use a high UID (1000000000+) to avoid conflicts with host users when running - without user namespace remapping. -- **Make your application workdir writable by `sandbox`**. This example creates - `/sandbox` with `sandbox:sandbox` ownership before copying `app.py`. +- **Declare a non-root OCI `USER`** for Docker and Podman. Use a named account + such as `app`, a numeric UID with a passwd entry that supplies its primary + GID, or a numeric pair such as `1500:1500`. You can instead set both + `process.run_as_user` and `process.run_as_group` explicitly in policy. - **Install `iproute2`** for full network namespace isolation. - **Use a standard Linux base image** — distroless and `FROM scratch` images are not supported. -TODO(#70): Remove the sandbox user note once custom images are secure by default without requiring manual setup. - ## How it works OpenShell handles all the wiring automatically. You build a standard From e82f3904a38a6e6c6c9bc69cf27e75f5479ad81e Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 27 Jul 2026 21:47:47 -0700 Subject: [PATCH 02/12] fix(sandbox): preserve declared process identities Keep explicit policy values and OCI-declared names intact, defer passwd lookup until a primary GID is required, and refresh stale policy examples. Signed-off-by: Matthew Grossman --- architecture/compute-runtimes.md | 7 +- crates/openshell-driver-docker/README.md | 5 +- crates/openshell-driver-podman/README.md | 3 +- .../src/identity.rs | 170 ++++++++++++------ .../src/process.rs | 42 ++++- .../tutorials/first-network-policy.mdx | 3 - docs/get-started/tutorials/github-sandbox.mdx | 4 - docs/reference/sandbox-compute-drivers.mdx | 7 +- examples/sandbox-policy-quickstart/README.md | 7 +- .../sandbox-policy-quickstart/policy.yaml | 5 +- 10 files changed, 168 insertions(+), 85 deletions(-) diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 5584034c3c..1629c50de4 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -139,9 +139,10 @@ driver then supplies one authoritative identity input to the supervisor: For Docker and Podman, policy values take precedence independently. An omitted `run_as_user` or `run_as_group` falls back to the corresponding identity from the image. The supervisor resolves names from the image's `/etc/passwd` and -`/etc/group`, converts the result to a numeric UID/GID, and uses the same -privilege-drop path for direct and SSH children. It does not rewrite the account -files. +`/etc/group` before readiness, preserves declared name or numeric components, +and uses the same privilege-drop path for direct and SSH children. When a +declaration omits the group, the supervisor fills it with the user's numeric +primary GID. It does not rewrite the account files. Sandbox creation fails before the workload becomes ready when a required image identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0. diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index aaffa6ea79..b2e74231ba 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -23,8 +23,9 @@ captures its immutable image ID and raw OCI `Config.User`. Container creation uses that image ID, preventing a mutable tag from changing between inspection and launch. The supervisor runs as root, resolves omitted policy identity fields from the image declaration, and drops only agent children to the resulting -numeric UID/GID. Explicit `process.run_as_user` and `process.run_as_group` -values take precedence independently. +identity. Named OCI components remain names after validation; a missing group +is filled with the user's numeric primary GID. Explicit `process.run_as_user` +and `process.run_as_group` values take precedence independently. Docker containers join an OpenShell-managed bridge network. The driver injects `host.openshell.internal` and `host.docker.internal` so supervisors have stable diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 7e8dc89108..965a295d19 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -12,7 +12,8 @@ captures its immutable image ID and raw OCI `Config.User`. Container creation uses that image ID with pulling disabled, preventing a mutable tag from changing between inspection and launch. The supervisor runs as root, resolves omitted policy identity fields from the image declaration, and drops only agent -children to the resulting numeric UID/GID. Explicit +children to the completed identity. Named OCI components remain names after +validation; a missing group is filled with the user's numeric primary GID. Explicit `process.run_as_user` and `process.run_as_group` values take precedence independently. diff --git a/crates/openshell-supervisor-process/src/identity.rs b/crates/openshell-supervisor-process/src/identity.rs index fe78a7344d..cb140d6259 100644 --- a/crates/openshell-supervisor-process/src/identity.rs +++ b/crates/openshell-supervisor-process/src/identity.rs @@ -108,46 +108,58 @@ fn resolve_oci_process_identity_at( .process .run_as_user .as_deref() - .filter(|value| !value.is_empty()); + .is_some_and(|value| !value.is_empty()); let explicit_group = policy .process .run_as_group .as_deref() - .filter(|value| !value.is_empty()); + .is_some_and(|value| !value.is_empty()); - if let (Some(user), Some(group)) = (explicit_user, explicit_group) { - let uid = resolve_user(user, passwd_path)?; - let gid = resolve_group(group, group_path)?; - install_numeric_identity(policy, uid, gid, "explicit policy")?; + if explicit_user && explicit_group { return Ok(()); } let (oci_user, oci_group) = split_oci_declaration(declaration); - let uid = match explicit_user { - Some(user) => resolve_user(user, passwd_path)?, - None => resolve_required_oci_user(oci_user, passwd_path, declaration)?.0, + let needs_primary_gid = !explicit_group && oci_group.is_none(); + let resolved_user = if !explicit_user || needs_primary_gid { + Some(resolve_required_oci_user( + oci_user, + passwd_path, + declaration, + needs_primary_gid, + )?) + } else { + None }; - let gid = match explicit_group { - Some(group) => resolve_group(group, group_path)?, - None => match oci_group { - Some(group) if !group.is_empty() => resolve_group(group, group_path)?, + + if !explicit_user { + policy.process.run_as_user = Some(oci_user.to_string()); + } + + if !explicit_group { + policy.process.run_as_group = Some(match oci_group { + Some(group) if !group.is_empty() => { + validate_oci_group(group, group_path, declaration)?; + group.to_string() + } Some(_) => { return Err(miette::miette!( "OCI USER '{declaration}' has an empty group component" )); } - None => resolve_required_oci_user(oci_user, passwd_path, declaration)? - .1 + None => resolved_user + .and_then(|(_, primary_gid)| primary_gid) .ok_or_else(|| { miette::miette!( "OCI USER '{declaration}' uses a numeric UID without an explicit group, \ but /etc/passwd has no matching primary GID" ) - })?, - }, - }; + })? + .to_string(), + }); + } - install_numeric_identity(policy, uid, gid, declaration) + Ok(()) } fn split_oci_declaration(declaration: &str) -> (&str, Option<&str>) { @@ -160,6 +172,7 @@ fn resolve_required_oci_user( user: &str, passwd_path: &Path, declaration: &str, + require_primary_gid: bool, ) -> Result<(u32, Option)> { if user.is_empty() { return Err(miette::miette!( @@ -174,53 +187,52 @@ fn resolve_required_oci_user( if uid == 0 { return Err(miette::miette!("OCI USER '{declaration}' selects UID 0")); } - let primary_gid = find_passwd_by_uid(passwd_path, uid)?.map(|entry| entry.gid); + let primary_gid = if require_primary_gid { + find_passwd_by_uid(passwd_path, uid)?.map(|entry| entry.gid) + } else { + None + }; + if primary_gid == Some(0) { + return Err(miette::miette!( + "OCI USER '{declaration}' resolves to prohibited primary GID 0" + )); + } return Ok((uid, primary_gid)); } let entry = find_passwd_by_name(passwd_path, user)? .ok_or_else(|| miette::miette!("OCI USER name '{user}' was not found in /etc/passwd"))?; - Ok((entry.uid, Some(entry.gid))) -} - -fn resolve_user(value: &str, passwd_path: &Path) -> Result { - validate_component(value, "user")?; - if value == "root" { - return Err(miette::miette!("process user must not select root")); + if entry.uid == 0 { + return Err(miette::miette!( + "OCI USER '{declaration}' resolves to prohibited UID 0" + )); } - if let Ok(uid) = value.parse::() { - return Ok(uid); + if require_primary_gid && entry.gid == 0 { + return Err(miette::miette!( + "OCI USER '{declaration}' resolves to prohibited primary GID 0" + )); } - Ok(find_passwd_by_name(passwd_path, value)? - .ok_or_else(|| miette::miette!("process user '{value}' was not found in /etc/passwd"))? - .uid) + Ok((entry.uid, require_primary_gid.then_some(entry.gid))) } -fn resolve_group(value: &str, group_path: &Path) -> Result { - validate_component(value, "group")?; +fn validate_oci_group(value: &str, group_path: &Path, declaration: &str) -> Result<()> { + validate_component(value, "OCI group")?; if value == "root" { - return Err(miette::miette!("process group must not select root")); - } - if let Ok(gid) = value.parse::() { - return Ok(gid); + return Err(miette::miette!( + "OCI USER '{declaration}' selects root group" + )); } - Ok(find_group_by_name(group_path, value)? - .ok_or_else(|| miette::miette!("process group '{value}' was not found in /etc/group"))? - .gid) -} - -fn install_numeric_identity( - policy: &mut SandboxPolicy, - uid: u32, - gid: u32, - source: &str, -) -> Result<()> { - if uid == 0 || gid == 0 { + let gid = if let Ok(gid) = value.parse::() { + gid + } else { + find_group_by_name(group_path, value)? + .ok_or_else(|| miette::miette!("OCI group '{value}' was not found in /etc/group"))? + .gid + }; + if gid == 0 { return Err(miette::miette!( - "process identity from '{source}' resolves to prohibited UID/GID {uid}:{gid}" + "OCI USER '{declaration}' resolves to prohibited GID 0" )); } - policy.process.run_as_user = Some(uid.to_string()); - policy.process.run_as_group = Some(gid.to_string()); Ok(()) } @@ -430,9 +442,10 @@ mod tests { ); let cases = [ (Some("2000"), Some("2001"), "root", "2000", "2001"), - (Some("2000"), None, "app:staff", "2000", "1235"), - (None, Some("2001"), "app:root", "1234", "2001"), - (None, None, "app", "1234", "1235"), + (Some("2000"), None, "app:staff", "2000", "staff"), + (None, Some("2001"), "app:root", "app", "2001"), + (None, None, "app:staff", "app", "staff"), + (None, None, "app", "app", "1235"), ]; for (user, group_name, declaration, expected_uid, expected_gid) in cases { let mut policy = policy(user, group_name); @@ -444,13 +457,32 @@ mod tests { #[test] fn numeric_pair_does_not_require_account_entries() { - let (_dir, passwd, group) = account_files("", ""); + let dir = tempdir().unwrap(); + let passwd = dir.path().join("missing-passwd"); + let group = dir.path().join("missing-group"); let mut policy = policy(None, None); resolve_oci_process_identity_at(&mut policy, "1234:1235", &passwd, &group).unwrap(); assert_eq!(policy.process.run_as_user.as_deref(), Some("1234")); assert_eq!(policy.process.run_as_group.as_deref(), Some("1235")); } + #[test] + fn explicit_identity_is_preserved_without_inspecting_oci_or_accounts() { + let dir = tempdir().unwrap(); + let mut policy = policy(Some("sandbox"), Some("sandbox")); + + resolve_oci_process_identity_at( + &mut policy, + "root:root", + &dir.path().join("missing-passwd"), + &dir.path().join("missing-group"), + ) + .unwrap(); + + assert_eq!(policy.process.run_as_user.as_deref(), Some("sandbox")); + assert_eq!(policy.process.run_as_group.as_deref(), Some("sandbox")); + } + #[test] fn driver_identity_inputs_are_mutually_exclusive_and_complete() { assert_eq!( @@ -511,9 +543,33 @@ mod tests { let mut explicit_user = policy(Some("1234"), None); resolve_oci_process_identity_at(&mut explicit_user, "root:staff", &passwd, &group).unwrap(); + assert_eq!(explicit_user.process.run_as_user.as_deref(), Some("1234")); + assert_eq!(explicit_user.process.run_as_group.as_deref(), Some("staff")); let mut explicit_group = policy(None, Some("1235")); resolve_oci_process_identity_at(&mut explicit_group, "app:root", &passwd, &group).unwrap(); + assert_eq!(explicit_group.process.run_as_user.as_deref(), Some("app")); + assert_eq!(explicit_group.process.run_as_group.as_deref(), Some("1235")); + } + + #[test] + fn named_oci_components_mapping_to_root_are_rejected() { + let (_dir, passwd, group) = account_files( + "root_alias:x:0:1235::/root:/bin/sh\napp:x:1234:1235::/home/app:/bin/sh\n", + "root_alias:x:0:\nstaff:x:1235:\n", + ); + + let mut root_user = policy(None, None); + assert!( + resolve_oci_process_identity_at(&mut root_user, "root_alias:staff", &passwd, &group) + .is_err() + ); + + let mut root_group = policy(None, None); + assert!( + resolve_oci_process_identity_at(&mut root_group, "app:root_alias", &passwd, &group) + .is_err() + ); } #[cfg(unix)] diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index ea5b5e110a..6b0e1742d6 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -866,8 +866,13 @@ impl Drop for ProcessHandle { pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { let identity = policy.process.run_as_user.as_deref().unwrap_or("sandbox"); - // Numeric UID — no passwd entry required; kernel resolves directly. - if openshell_policy::is_valid_sandbox_identity(identity) && identity.parse::().is_ok() { + // Explicit policy identities were range-checked before the runtime policy + // reached the supervisor. OCI-derived numeric identities may use system + // account IDs, so this completed-runtime check only prohibits root. + if let Ok(uid) = identity.parse::() { + if uid == 0 { + return Err(miette::miette!("process user must not select UID 0")); + } openshell_ocsf::ocsf_emit!( openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx()) .severity(openshell_ocsf::SeverityId::Informational) @@ -934,7 +939,10 @@ pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { pub fn validate_sandbox_group(policy: &SandboxPolicy) -> Result<()> { let identity = policy.process.run_as_group.as_deref().unwrap_or("sandbox"); - if openshell_policy::is_valid_sandbox_identity(identity) && identity.parse::().is_ok() { + if let Ok(gid) = identity.parse::() { + if gid == 0 { + return Err(miette::miette!("process group must not select GID 0")); + } openshell_ocsf::ocsf_emit!( openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx()) .severity(openshell_ocsf::SeverityId::Informational) @@ -1549,6 +1557,34 @@ mod tests { ); } + #[test] + #[cfg(unix)] + fn completed_runtime_identity_accepts_non_root_system_ids() { + let policy = policy_with_process(ProcessPolicy { + run_as_user: Some("101".into()), + run_as_group: Some("102".into()), + }); + + assert!(validate_sandbox_user(&policy).is_ok()); + assert!(validate_sandbox_group(&policy).is_ok()); + } + + #[test] + #[cfg(unix)] + fn completed_runtime_identity_rejects_numeric_root() { + let root_user = policy_with_process(ProcessPolicy { + run_as_user: Some("0".into()), + run_as_group: Some("102".into()), + }); + let root_group = policy_with_process(ProcessPolicy { + run_as_user: Some("101".into()), + run_as_group: Some("0".into()), + }); + + assert!(validate_sandbox_user(&root_user).is_err()); + assert!(validate_sandbox_group(&root_group).is_err()); + } + #[test] fn full_enforcement_uses_privileged_setup_and_child_sandbox() { assert!(ProcessEnforcementMode::Full.uses_privileged_process_setup()); diff --git a/docs/get-started/tutorials/first-network-policy.mdx b/docs/get-started/tutorials/first-network-policy.mdx index 5071f3e2d4..0639ec9a7f 100644 --- a/docs/get-started/tutorials/first-network-policy.mdx +++ b/docs/get-started/tutorials/first-network-policy.mdx @@ -100,9 +100,6 @@ filesystem_policy: read_write: [/sandbox, /tmp, /dev/null] landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox network_policies: github_api: diff --git a/docs/get-started/tutorials/github-sandbox.mdx b/docs/get-started/tutorials/github-sandbox.mdx index 7c76d4e411..1fdcea4e90 100644 --- a/docs/get-started/tutorials/github-sandbox.mdx +++ b/docs/get-started/tutorials/github-sandbox.mdx @@ -187,10 +187,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - # ── Dynamic (hot-reloadable) ───────────────────────────────────── network_policies: diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index a535cc258d..d572572991 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -427,9 +427,10 @@ whose passwd entry supplies its primary GID, and an accountless numeric pair such as `1234:1235`. The driver pins container creation to the immutable image ID it inspected. The -supervisor resolves any required names inside that image, then uses the numeric -UID/GID for both direct and SSH children. It does not modify `/etc/passwd` or -`/etc/group`. +supervisor validates any required names inside that image and preserves the +declared name or numeric components for both direct and SSH children. When +`USER` omits the group, the supervisor uses the user's numeric primary GID. It +does not modify `/etc/passwd` or `/etc/group`. Sandbox creation fails before readiness if a required `USER` component is missing, malformed, unknown, ambiguous, or resolves to UID/GID 0. An image diff --git a/examples/sandbox-policy-quickstart/README.md b/examples/sandbox-policy-quickstart/README.md index 34ecfbc9d6..30ee1d092e 100644 --- a/examples/sandbox-policy-quickstart/README.md +++ b/examples/sandbox-policy-quickstart/README.md @@ -81,8 +81,8 @@ cat examples/sandbox-policy-quickstart/policy.yaml ```yaml version: 1 -# Default sandbox filesystem and process settings. -# These static fields are required when using `openshell policy set` +# Default sandbox filesystem settings. +# These filesystem fields are required when using `openshell policy set` # because it replaces the entire policy. filesystem_policy: include_workdir: true @@ -90,9 +90,6 @@ filesystem_policy: read_write: [/sandbox, /tmp, /dev/null] landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox network_policies: github_api: diff --git a/examples/sandbox-policy-quickstart/policy.yaml b/examples/sandbox-policy-quickstart/policy.yaml index 6bb0cb7d02..3d4b034377 100644 --- a/examples/sandbox-policy-quickstart/policy.yaml +++ b/examples/sandbox-policy-quickstart/policy.yaml @@ -7,7 +7,7 @@ version: 1 # Default sandbox filesystem and process settings. -# These static fields are required when using `openshell policy set` +# These filesystem fields are required when using `openshell policy set` # because it replaces the entire policy. filesystem_policy: include_workdir: true @@ -15,9 +15,6 @@ filesystem_policy: read_write: [/sandbox, /tmp, /dev/null] landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox network_policies: github_api: From 3cb345018d574fcd9ca490a411cb670c92d377aa Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Mon, 27 Jul 2026 23:08:40 -0700 Subject: [PATCH 03/12] fix(supervisor): reuse resolved OCI identity Signed-off-by: Matthew Grossman --- crates/openshell-sandbox/src/lib.rs | 10 +- .../src/identity.rs | 148 +++++++-- .../src/process.rs | 291 +++++++++++++----- .../openshell-supervisor-process/src/run.rs | 14 +- .../openshell-supervisor-process/src/ssh.rs | 87 +++++- .../tutorials/first-network-policy.mdx | 2 +- docs/get-started/tutorials/github-sandbox.mdx | 4 +- examples/sandbox-policy-quickstart/README.md | 6 +- .../sandbox-policy-quickstart/policy.yaml | 6 +- 9 files changed, 438 insertions(+), 130 deletions(-) diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 1ee013df08..5ba41b9d7e 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -189,13 +189,16 @@ pub async fn run_sandbox( // OpenShift retain their authoritative numeric pair; Docker and Podman // fill only omitted policy fields from OCI Config.User. #[cfg(unix)] - { + let resolved_process_identity = { let driver_identity = openshell_supervisor_process::identity::DriverIdentity::from_env()?; openshell_supervisor_process::identity::resolve_process_identity( &mut policy, &driver_identity, - )?; - } + )? + }; + #[cfg(not(unix))] + let resolved_process_identity = + openshell_supervisor_process::process::ResolvedProcessIdentity::default(); #[cfg_attr(not(target_os = "linux"), allow(unused_mut))] let (provider_credentials, mut provider_env) = @@ -688,6 +691,7 @@ pub async fn run_sandbox( ssh_socket_path, sidecar_network_enforcement, &process_policy, + resolved_process_identity, process_enforcement_mode, entrypoint_pid, entrypoint_started_tx, diff --git a/crates/openshell-supervisor-process/src/identity.rs b/crates/openshell-supervisor-process/src/identity.rs index cb140d6259..adaa0a077f 100644 --- a/crates/openshell-supervisor-process/src/identity.rs +++ b/crates/openshell-supervisor-process/src/identity.rs @@ -3,6 +3,7 @@ //! Driver identity normalization and OCI `USER` resolution. +use crate::process::ResolvedProcessIdentity; use miette::{IntoDiagnostic, Result}; use openshell_core::policy::SandboxPolicy; use std::fs::{File, OpenOptions}; @@ -81,12 +82,14 @@ impl DriverIdentity { pub fn resolve_process_identity( policy: &mut SandboxPolicy, driver_identity: &DriverIdentity, -) -> Result<()> { +) -> Result { match driver_identity { DriverIdentity::Resolved { uid, gid } => { policy.process.run_as_user = Some(uid.to_string()); policy.process.run_as_group = Some(gid.to_string()); - Ok(()) + // Kubernetes/OpenShift already supply numeric policy values and + // retain their existing privilege-drop path. + Ok(ResolvedProcessIdentity::default()) } DriverIdentity::OciUser { declaration } => resolve_oci_process_identity_at( policy, @@ -94,16 +97,17 @@ pub fn resolve_process_identity( Path::new(PASSWD_PATH), Path::new(GROUP_PATH), ), - DriverIdentity::None => Ok(()), + DriverIdentity::None => Ok(ResolvedProcessIdentity::default()), } } +#[allow(clippy::similar_names)] fn resolve_oci_process_identity_at( policy: &mut SandboxPolicy, declaration: &str, passwd_path: &Path, group_path: &Path, -) -> Result<()> { +) -> Result { let explicit_user = policy .process .run_as_user @@ -116,7 +120,7 @@ fn resolve_oci_process_identity_at( .is_some_and(|value| !value.is_empty()); if explicit_user && explicit_group { - return Ok(()); + return Ok(ResolvedProcessIdentity::default()); } let (oci_user, oci_group) = split_oci_declaration(declaration); @@ -132,34 +136,51 @@ fn resolve_oci_process_identity_at( None }; + let oci_uid = if explicit_user { + None + } else { + Some( + resolved_user + .as_ref() + .expect("omitted OCI user must have been resolved") + .0, + ) + }; + if !explicit_user { policy.process.run_as_user = Some(oci_user.to_string()); } - if !explicit_group { - policy.process.run_as_group = Some(match oci_group { + let oci_gid = if explicit_group { + None + } else { + let (group_value, gid) = match oci_group { Some(group) if !group.is_empty() => { - validate_oci_group(group, group_path, declaration)?; - group.to_string() + let gid = validate_oci_group(group, group_path, declaration)?; + (group.to_string(), gid) } Some(_) => { return Err(miette::miette!( "OCI USER '{declaration}' has an empty group component" )); } - None => resolved_user - .and_then(|(_, primary_gid)| primary_gid) - .ok_or_else(|| { + None => { + let gid = resolved_user + .and_then(|(_, primary_gid)| primary_gid) + .ok_or_else(|| { miette::miette!( "OCI USER '{declaration}' uses a numeric UID without an explicit group, \ but /etc/passwd has no matching primary GID" ) - })? - .to_string(), - }); - } + })?; + (gid.to_string(), gid) + } + }; + policy.process.run_as_group = Some(group_value); + Some(gid) + }; - Ok(()) + Ok(ResolvedProcessIdentity::new(oci_uid, oci_gid)) } fn split_oci_declaration(declaration: &str) -> (&str, Option<&str>) { @@ -214,7 +235,7 @@ fn resolve_required_oci_user( Ok((entry.uid, require_primary_gid.then_some(entry.gid))) } -fn validate_oci_group(value: &str, group_path: &Path, declaration: &str) -> Result<()> { +fn validate_oci_group(value: &str, group_path: &Path, declaration: &str) -> Result { validate_component(value, "OCI group")?; if value == "root" { return Err(miette::miette!( @@ -233,7 +254,7 @@ fn validate_oci_group(value: &str, group_path: &Path, declaration: &str) -> Resu "OCI USER '{declaration}' resolves to prohibited GID 0" )); } - Ok(()) + Ok(gid) } fn validate_component(value: &str, kind: &str) -> Result<()> { @@ -441,17 +462,61 @@ mod tests { "staff:x:1235:\nsandbox:x:2001:\n", ); let cases = [ - (Some("2000"), Some("2001"), "root", "2000", "2001"), - (Some("2000"), None, "app:staff", "2000", "staff"), - (None, Some("2001"), "app:root", "app", "2001"), - (None, None, "app:staff", "app", "staff"), - (None, None, "app", "app", "1235"), + ( + Some("2000"), + Some("2001"), + "root", + "2000", + "2001", + None, + None, + ), + ( + Some("2000"), + None, + "app:staff", + "2000", + "staff", + None, + Some(1235), + ), + ( + None, + Some("2001"), + "app:root", + "app", + "2001", + Some(1234), + None, + ), + ( + None, + None, + "app:staff", + "app", + "staff", + Some(1234), + Some(1235), + ), + (None, None, "app", "app", "1235", Some(1234), Some(1235)), ]; - for (user, group_name, declaration, expected_uid, expected_gid) in cases { + for ( + user, + group_name, + declaration, + expected_user, + expected_group, + resolved_uid, + resolved_gid, + ) in cases + { let mut policy = policy(user, group_name); - resolve_oci_process_identity_at(&mut policy, declaration, &passwd, &group).unwrap(); - assert_eq!(policy.process.run_as_user.as_deref(), Some(expected_uid)); - assert_eq!(policy.process.run_as_group.as_deref(), Some(expected_gid)); + let resolved = + resolve_oci_process_identity_at(&mut policy, declaration, &passwd, &group).unwrap(); + assert_eq!(policy.process.run_as_user.as_deref(), Some(expected_user)); + assert_eq!(policy.process.run_as_group.as_deref(), Some(expected_group)); + assert_eq!(resolved.uid(), resolved_uid); + assert_eq!(resolved.gid(), resolved_gid); } } @@ -461,9 +526,14 @@ mod tests { let passwd = dir.path().join("missing-passwd"); let group = dir.path().join("missing-group"); let mut policy = policy(None, None); - resolve_oci_process_identity_at(&mut policy, "1234:1235", &passwd, &group).unwrap(); + let resolved = + resolve_oci_process_identity_at(&mut policy, "1234:1235", &passwd, &group).unwrap(); assert_eq!(policy.process.run_as_user.as_deref(), Some("1234")); assert_eq!(policy.process.run_as_group.as_deref(), Some("1235")); + assert_eq!( + resolved, + ResolvedProcessIdentity::new(Some(1234), Some(1235)) + ); } #[test] @@ -471,7 +541,7 @@ mod tests { let dir = tempdir().unwrap(); let mut policy = policy(Some("sandbox"), Some("sandbox")); - resolve_oci_process_identity_at( + let resolved = resolve_oci_process_identity_at( &mut policy, "root:root", &dir.path().join("missing-passwd"), @@ -481,6 +551,7 @@ mod tests { assert_eq!(policy.process.run_as_user.as_deref(), Some("sandbox")); assert_eq!(policy.process.run_as_group.as_deref(), Some("sandbox")); + assert_eq!(resolved, ResolvedProcessIdentity::default()); } #[test] @@ -517,8 +588,13 @@ mod tests { fn numeric_uid_uses_passwd_primary_gid() { let (_dir, passwd, group) = account_files("app:x:1234:4321::/home/app:/bin/sh\n", ""); let mut policy = policy(None, None); - resolve_oci_process_identity_at(&mut policy, "1234", &passwd, &group).unwrap(); + let resolved = + resolve_oci_process_identity_at(&mut policy, "1234", &passwd, &group).unwrap(); assert_eq!(policy.process.run_as_group.as_deref(), Some("4321")); + assert_eq!( + resolved, + ResolvedProcessIdentity::new(Some(1234), Some(4321)) + ); } #[test] @@ -542,14 +618,20 @@ mod tests { account_files("app:x:1234:1235::/home/app:/bin/sh\n", "staff:x:1235:\n"); let mut explicit_user = policy(Some("1234"), None); - resolve_oci_process_identity_at(&mut explicit_user, "root:staff", &passwd, &group).unwrap(); + let resolved = + resolve_oci_process_identity_at(&mut explicit_user, "root:staff", &passwd, &group) + .unwrap(); assert_eq!(explicit_user.process.run_as_user.as_deref(), Some("1234")); assert_eq!(explicit_user.process.run_as_group.as_deref(), Some("staff")); + assert_eq!(resolved, ResolvedProcessIdentity::new(None, Some(1235))); let mut explicit_group = policy(None, Some("1235")); - resolve_oci_process_identity_at(&mut explicit_group, "app:root", &passwd, &group).unwrap(); + let resolved = + resolve_oci_process_identity_at(&mut explicit_group, "app:root", &passwd, &group) + .unwrap(); assert_eq!(explicit_group.process.run_as_user.as_deref(), Some("app")); assert_eq!(explicit_group.process.run_as_group.as_deref(), Some("1235")); + assert_eq!(resolved, ResolvedProcessIdentity::new(Some(1234), None)); } #[test] diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 6b0e1742d6..13fd6e2efa 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -40,6 +40,35 @@ pub enum ProcessEnforcementMode { NetworkOnly, } +/// Numeric identity components resolved once from driver-owned metadata. +/// +/// A component is `None` when the corresponding policy field was explicit and +/// must continue through the existing policy identity path. OCI-derived +/// components are carried numerically so later filesystem setup and direct/SSH +/// privilege drops cannot resolve them differently through NSS. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct ResolvedProcessIdentity { + uid: Option, + gid: Option, +} + +impl ResolvedProcessIdentity { + #[must_use] + pub const fn new(uid: Option, gid: Option) -> Self { + Self { uid, gid } + } + + #[must_use] + pub const fn uid(self) -> Option { + self.uid + } + + #[must_use] + pub const fn gid(self) -> Option { + self.gid + } +} + impl ProcessEnforcementMode { #[must_use] pub const fn uses_privileged_process_setup(self) -> bool { @@ -491,6 +520,7 @@ impl ProcessHandle { workdir: Option<&str>, interactive: bool, policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, netns: Option<&NetworkNamespace>, ca_paths: Option<&(PathBuf, PathBuf)>, @@ -502,6 +532,7 @@ impl ProcessHandle { workdir, interactive, policy, + resolved_identity, enforcement_mode, netns.and_then(NetworkNamespace::ns_fd), ca_paths, @@ -522,6 +553,7 @@ impl ProcessHandle { workdir: Option<&str>, interactive: bool, policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, ca_paths: Option<&(PathBuf, PathBuf)>, provider_env: &HashMap, @@ -532,6 +564,7 @@ impl ProcessHandle { workdir, interactive, policy, + resolved_identity, enforcement_mode, ca_paths, provider_env, @@ -546,6 +579,7 @@ impl ProcessHandle { workdir: Option<&str>, interactive: bool, policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, netns_fd: Option, ca_paths: Option<&(PathBuf, PathBuf)>, @@ -663,7 +697,7 @@ impl ProcessHandle { // /etc/group and /etc/passwd which would be blocked if // Landlock were already enforced. if enforcement_mode.uses_privileged_process_setup() { - drop_privileges(&policy) + drop_privileges_with_identity(&policy, resolved_identity) .map_err(|err| std::io::Error::other(err.to_string()))?; } @@ -700,6 +734,7 @@ impl ProcessHandle { workdir: Option<&str>, interactive: bool, policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, ca_paths: Option<&(PathBuf, PathBuf)>, provider_env: &HashMap, @@ -764,7 +799,7 @@ impl ProcessHandle { // initgroups/setgid/setuid need access to /etc/group and /etc/passwd // which may be blocked by Landlock. if enforcement_mode.uses_privileged_process_setup() { - drop_privileges(&policy) + drop_privileges_with_identity(&policy, resolved_identity) .map_err(|err| std::io::Error::other(err.to_string()))?; } @@ -999,6 +1034,32 @@ pub fn validate_sandbox_group(policy: &SandboxPolicy) -> Result<()> { Ok(()) } +#[cfg(unix)] +pub fn validate_sandbox_user_with_identity( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, +) -> Result<()> { + let Some(uid) = resolved_identity.uid() else { + return validate_sandbox_user(policy); + }; + let mut effective_policy = policy.clone(); + effective_policy.process.run_as_user = Some(uid.to_string()); + validate_sandbox_user(&effective_policy) +} + +#[cfg(unix)] +pub fn validate_sandbox_group_with_identity( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, +) -> Result<()> { + let Some(gid) = resolved_identity.gid() else { + return validate_sandbox_group(policy); + }; + let mut effective_policy = policy.clone(); + effective_policy.process.run_as_group = Some(gid.to_string()); + validate_sandbox_group(&effective_policy) +} + pub use openshell_policy::{MAX_SANDBOX_UID, MIN_SANDBOX_UID}; /// Prepare a `read_write` path for the sandboxed process. @@ -1247,6 +1308,14 @@ fn chown_recursive( /// UIDs/GIDs (passed directly to `chown` without a passwd lookup). #[cfg(unix)] pub fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> { + prepare_filesystem_with_identity(policy, ResolvedProcessIdentity::default()) +} + +#[cfg(unix)] +pub fn prepare_filesystem_with_identity( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, +) -> Result<()> { use nix::unistd::chown; use nix::unistd::{Gid, Uid}; @@ -1265,21 +1334,27 @@ pub fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> { } // Resolve UID: numeric values are passed directly; names resolve via passwd. - let uid = match user_name { - Some(name) if name.parse::().is_ok() => { - Some(Uid::from_raw(name.parse().into_diagnostic()?)) - } - Some(name) => User::from_name(name).into_diagnostic()?.map(|u| u.uid), - _ => None, + let uid = match resolved_identity.uid() { + Some(uid) => Some(Uid::from_raw(uid)), + None => match user_name { + Some(name) if name.parse::().is_ok() => { + Some(Uid::from_raw(name.parse().into_diagnostic()?)) + } + Some(name) => User::from_name(name).into_diagnostic()?.map(|u| u.uid), + _ => None, + }, }; // Resolve GID: numeric values are passed directly; names resolve via group. - let gid = match group_name { - Some(name) if name.parse::().is_ok() => { - Some(Gid::from_raw(name.parse().into_diagnostic()?)) - } - Some(name) => Group::from_name(name).into_diagnostic()?.map(|g| g.gid), - _ => None, + let gid = match resolved_identity.gid() { + Some(gid) => Some(Gid::from_raw(gid)), + None => match group_name { + Some(name) if name.parse::().is_ok() => { + Some(Gid::from_raw(name.parse().into_diagnostic()?)) + } + Some(name) => Group::from_name(name).into_diagnostic()?.map(|g| g.gid), + _ => None, + }, }; // Create missing read_write paths and only chown the ones we created. @@ -1321,6 +1396,15 @@ pub fn prepare_filesystem(_policy: &SandboxPolicy) -> Result<()> { #[cfg(unix)] #[allow(clippy::similar_names)] pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> { + drop_privileges_with_identity(policy, ResolvedProcessIdentity::default()) +} + +#[cfg(unix)] +#[allow(clippy::similar_names)] +pub fn drop_privileges_with_identity( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, +) -> Result<()> { let user_name = match policy.process.run_as_user.as_deref() { Some(name) if !name.is_empty() => Some(name), _ => None, @@ -1339,86 +1423,109 @@ pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> { let mut fallback = policy.clone(); fallback.process.run_as_user = Some("sandbox".into()); fallback.process.run_as_group = Some("sandbox".into()); - return drop_privileges(&fallback); + return drop_privileges_with_identity(&fallback, resolved_identity); } return Ok(()); } // Resolve UID: numeric values are used directly; names resolve via passwd. - let target_uid = match user_name { - Some(name) if name.parse::().is_ok() => Uid::from_raw(name.parse().into_diagnostic()?), - Some(name) => { - User::from_name(name) - .into_diagnostic()? - .ok_or_else(|| miette::miette!("Sandbox user not found: {name}"))? - .uid - } - None => nix::unistd::geteuid(), + let target_uid = match resolved_identity.uid() { + Some(uid) => Uid::from_raw(uid), + None => match user_name { + Some(name) if name.parse::().is_ok() => { + Uid::from_raw(name.parse().into_diagnostic()?) + } + Some(name) => { + User::from_name(name) + .into_diagnostic()? + .ok_or_else(|| miette::miette!("Sandbox user not found: {name}"))? + .uid + } + None => nix::unistd::geteuid(), + }, }; // Resolve group: if a numeric GID is configured use it directly. // Otherwise try name resolution, then fall back to current user's primary group. - let target_gid = match group_name { - Some(name) if name.parse::().is_ok() => Gid::from_raw(name.parse().into_diagnostic()?), - Some(name) => { - Group::from_name(name) - .into_diagnostic()? - .ok_or_else(|| miette::miette!("Sandbox group not found: {name}"))? - .gid - } - None => match target_uid.as_raw() { - 0 => nix::unistd::getegid(), - _ => Group::from_gid( - User::from_uid(target_uid) + let target_gid = match resolved_identity.gid() { + Some(gid) => Gid::from_raw(gid), + None => match group_name { + Some(name) if name.parse::().is_ok() => { + Gid::from_raw(name.parse().into_diagnostic()?) + } + Some(name) => { + Group::from_name(name) .into_diagnostic()? - .ok_or_else(|| miette::miette!("Failed to resolve user from UID {target_uid}"))? - .gid, - ) - .into_diagnostic()? - .map_or_else(nix::unistd::getegid, |g| g.gid), + .ok_or_else(|| miette::miette!("Sandbox group not found: {name}"))? + .gid + } + None => match target_uid.as_raw() { + 0 => nix::unistd::getegid(), + _ => Group::from_gid( + User::from_uid(target_uid) + .into_diagnostic()? + .ok_or_else(|| { + miette::miette!("Failed to resolve user from UID {target_uid}") + })? + .gid, + ) + .into_diagnostic()? + .map_or_else(nix::unistd::getegid, |g| g.gid), + }, }, }; - // Resolve the user record for initgroups only when identity is name-based. - // Numeric UIDs may not have a /etc/passwd entry; skip the lookup rather than - // failing with a spurious "user record not found" error. + // Resolve the name for initgroups only for the existing explicit-policy + // path. OCI-derived users carry a numeric UID from the bounded parser and + // must not be looked up again through NSS. let user_name_is_numeric = user_name.is_some_and(|n| n.parse::().is_ok()); - let user = if user_name.is_some() && !user_name_is_numeric { - Some( - User::from_uid(target_uid) - .into_diagnostic()? - .ok_or_else(|| { - miette::miette!("Failed to resolve user record for UID {target_uid}") - })?, - ) - } else { - None - }; + let initgroups_name = + if user_name.is_some() && !user_name_is_numeric && resolved_identity.uid().is_none() { + Some( + User::from_uid(target_uid) + .into_diagnostic()? + .ok_or_else(|| { + miette::miette!("Failed to resolve user record for UID {target_uid}") + })? + .name, + ) + } else { + None + }; - // Set supplementary groups only when we have a name-based identity. - // Numeric UIDs may not have a passwd entry, so initgroups would fail. - if let Some(ref user) = user - && target_uid != nix::unistd::geteuid() - { - let user_cstr = - CString::new(user.name.clone()).map_err(|_| miette::miette!("Invalid user name"))?; - #[cfg(any( - target_os = "macos", - target_os = "ios", - target_os = "haiku", - target_os = "redox" - ))] - { - let _ = user_cstr; - } - #[cfg(not(any( - target_os = "macos", - target_os = "ios", - target_os = "haiku", - target_os = "redox" - )))] - { - nix::unistd::initgroups(user_cstr.as_c_str(), target_gid).into_diagnostic()?; + if target_uid != nix::unistd::geteuid() { + if resolved_identity.uid().is_some() { + // OCI Config.User defines the selected UID/GID pair, not a second + // NSS-dependent supplementary group set. Clear the root + // supervisor's inherited supplementary groups before dropping. + #[cfg(not(any( + target_os = "macos", + target_os = "ios", + target_os = "haiku", + target_os = "redox" + )))] + nix::unistd::setgroups(&[]).into_diagnostic()?; + } else if let Some(ref user_name) = initgroups_name { + let user_cstr = CString::new(user_name.as_str()) + .map_err(|_| miette::miette!("Invalid user name"))?; + #[cfg(any( + target_os = "macos", + target_os = "ios", + target_os = "haiku", + target_os = "redox" + ))] + { + let _ = user_cstr; + } + #[cfg(not(any( + target_os = "macos", + target_os = "ios", + target_os = "haiku", + target_os = "redox" + )))] + { + nix::unistd::initgroups(user_cstr.as_c_str(), target_gid).into_diagnostic()?; + } } } @@ -1585,6 +1692,32 @@ mod tests { assert!(validate_sandbox_group(&root_group).is_err()); } + #[test] + #[cfg(unix)] + fn resolved_oci_components_do_not_repeat_nss_validation() { + let policy = policy_with_process(ProcessPolicy { + run_as_user: Some("__oci_name_not_in_host_nss__".into()), + run_as_group: Some("__oci_group_not_in_host_nss__".into()), + }); + let resolved = ResolvedProcessIdentity::new(Some(1234), Some(1235)); + + assert!(validate_sandbox_user_with_identity(&policy, resolved).is_ok()); + assert!(validate_sandbox_group_with_identity(&policy, resolved).is_ok()); + } + + #[test] + #[cfg(unix)] + fn explicit_policy_components_keep_existing_validation_path() { + let policy = policy_with_process(ProcessPolicy { + run_as_user: Some("__explicit_name_not_in_host_nss__".into()), + run_as_group: Some("__oci_group_not_in_host_nss__".into()), + }); + let resolved = ResolvedProcessIdentity::new(None, Some(1235)); + + assert!(validate_sandbox_user_with_identity(&policy, resolved).is_err()); + assert!(validate_sandbox_group_with_identity(&policy, resolved).is_ok()); + } + #[test] fn full_enforcement_uses_privileged_setup_and_child_sandbox() { assert!(ProcessEnforcementMode::Full.uses_privileged_process_setup()); diff --git a/crates/openshell-supervisor-process/src/run.rs b/crates/openshell-supervisor-process/src/run.rs index 1eb58857b2..a5ff0456c9 100644 --- a/crates/openshell-supervisor-process/src/run.rs +++ b/crates/openshell-supervisor-process/src/run.rs @@ -34,7 +34,9 @@ use openshell_core::denial::DenialEvent; #[cfg(target_os = "linux")] use crate::managed_children; -use crate::process::{ProcessEnforcementMode, ProcessHandle, ProcessStatus}; +use crate::process::{ + ProcessEnforcementMode, ProcessHandle, ProcessStatus, ResolvedProcessIdentity, +}; fn ocsf_ctx() -> &'static openshell_ocsf::SandboxContext { openshell_ocsf::ctx::ctx() @@ -59,6 +61,7 @@ pub async fn run_process( ssh_socket_path: Option, shared_ssh_socket: bool, policy: &SandboxPolicy, + resolved_process_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, entrypoint_pid: Arc, entrypoint_started_tx: Option>, @@ -83,8 +86,8 @@ pub async fn run_process( // Validate the completed process identity before exposing a child. #[cfg(unix)] if enforcement_mode.uses_privileged_process_setup() { - crate::process::validate_sandbox_user(policy)?; - crate::process::validate_sandbox_group(policy)?; + crate::process::validate_sandbox_user_with_identity(policy, resolved_process_identity)?; + crate::process::validate_sandbox_group_with_identity(policy, resolved_process_identity)?; } // Create read_write directories and chown newly-created ones to the @@ -92,7 +95,7 @@ pub async fn run_process( // is forked so the workload sees writable paths it owns. #[cfg(unix)] if enforcement_mode.uses_privileged_process_setup() { - crate::process::prepare_filesystem(policy)?; + crate::process::prepare_filesystem_with_identity(policy, resolved_process_identity)?; } // Eagerly fetch initial settings and install the agent skill if the @@ -246,6 +249,7 @@ pub async fn run_process( ca_paths, provider_credentials_clone, user_env_clone, + resolved_process_identity, enforcement_mode, shared_ssh_socket, ) @@ -318,6 +322,7 @@ pub async fn run_process( workdir, interactive, policy, + resolved_process_identity, enforcement_mode, netns, ca_file_paths.as_ref(), @@ -331,6 +336,7 @@ pub async fn run_process( workdir, interactive, policy, + resolved_process_identity, enforcement_mode, ca_file_paths.as_ref(), &provider_env, diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index f5a3ee0793..0e555abff8 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -6,7 +6,10 @@ use crate::child_env; #[cfg(target_os = "linux")] use crate::managed_children; -use crate::process::{ProcessEnforcementMode, drop_privileges, is_supervisor_only_env_var}; +use crate::process::{ + ProcessEnforcementMode, ResolvedProcessIdentity, drop_privileges_with_identity, + is_supervisor_only_env_var, +}; use crate::sandbox; use miette::{IntoDiagnostic, Result}; use nix::pty::{Winsize, openpty}; @@ -114,6 +117,7 @@ pub async fn run_ssh_server( ca_file_paths: Option<(PathBuf, PathBuf)>, provider_credentials: ProviderCredentialState, user_environment: HashMap, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, shared_socket: bool, ) -> Result<()> { @@ -158,6 +162,7 @@ pub async fn run_ssh_server( ca_paths, provider_credentials, user_environment, + resolved_identity, enforcement_mode, ) .await @@ -186,6 +191,7 @@ async fn handle_connection( ca_file_paths: Option>, provider_credentials: ProviderCredentialState, user_environment: HashMap, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, ) -> Result<()> { // Access is gated by the Unix-socket filesystem permissions (root-only), @@ -210,6 +216,7 @@ async fn handle_connection( ca_file_paths, provider_credentials, user_environment, + resolved_identity, enforcement_mode, ); russh::server::run_stream(config, stream, handler) @@ -239,6 +246,7 @@ struct SshHandler { ca_file_paths: Option>, provider_credentials: ProviderCredentialState, user_environment: HashMap, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, channels: HashMap, } @@ -253,6 +261,7 @@ impl SshHandler { ca_file_paths: Option>, provider_credentials: ProviderCredentialState, user_environment: HashMap, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, ) -> Self { Self { @@ -263,6 +272,7 @@ impl SshHandler { ca_file_paths, provider_credentials, user_environment, + resolved_identity, enforcement_mode, channels: HashMap::new(), } @@ -487,6 +497,7 @@ impl russh::server::Handler for SshHandler { self.ca_file_paths.clone(), &self.provider_credentials.child_env_with_gcp_resolved(), &self.user_environment, + self.resolved_identity, self.enforcement_mode, )?; let state = self.channels.get_mut(&channel).ok_or_else(|| { @@ -584,6 +595,7 @@ impl SshHandler { self.ca_file_paths.clone(), &provider_env, &self.user_environment, + self.resolved_identity, self.enforcement_mode, )?; state.pty_master = Some(pty_master); @@ -603,6 +615,7 @@ impl SshHandler { self.ca_file_paths.clone(), &provider_env, &self.user_environment, + self.resolved_identity, self.enforcement_mode, )?; state.input_sender = Some(input_sender); @@ -770,6 +783,7 @@ fn spawn_pty_shell( ca_file_paths: Option>, provider_env: &HashMap, user_environment: &HashMap, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, ) -> anyhow::Result<(std::fs::File, mpsc::Sender>)> { let winsize = Winsize { @@ -847,6 +861,7 @@ fn spawn_pty_shell( workdir.clone(), slave_fd, netns_fd, + resolved_identity, enforcement_mode, #[cfg(target_os = "linux")] prepared_sandbox, @@ -940,6 +955,7 @@ fn spawn_pipe_exec( ca_file_paths: Option>, provider_env: &HashMap, user_environment: &HashMap, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, ) -> anyhow::Result>> { let mut cmd = command.map_or_else( @@ -1000,6 +1016,7 @@ fn spawn_pipe_exec( policy.clone(), workdir.clone(), netns_fd, + resolved_identity, enforcement_mode, #[cfg(target_os = "linux")] prepared_sandbox, @@ -1101,7 +1118,8 @@ mod unsafe_pty { #[cfg(not(target_os = "linux"))] use super::sandbox; use super::{ - Command, ProcessEnforcementMode, RawFd, SandboxPolicy, Winsize, drop_privileges, setsid, + Command, ProcessEnforcementMode, RawFd, ResolvedProcessIdentity, SandboxPolicy, Winsize, + drop_privileges_with_identity, setsid, }; #[cfg(unix)] use std::os::unix::process::CommandExt; @@ -1141,6 +1159,7 @@ mod unsafe_pty { _workdir: Option, slave_fd: RawFd, netns_fd: Option, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, #[cfg(target_os = "linux")] prepared: Option, ) -> anyhow::Result<()> { @@ -1164,6 +1183,7 @@ mod unsafe_pty { enter_netns_and_sandbox( netns_fd, &policy, + resolved_identity, enforcement_mode, #[cfg(target_os = "linux")] supervisor_identity_mount, @@ -1191,6 +1211,7 @@ mod unsafe_pty { policy: SandboxPolicy, _workdir: Option, netns_fd: Option, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, #[cfg(target_os = "linux")] prepared: Option, ) -> anyhow::Result<()> { @@ -1209,6 +1230,7 @@ mod unsafe_pty { enter_netns_and_sandbox( netns_fd, &policy, + resolved_identity, enforcement_mode, #[cfg(target_os = "linux")] supervisor_identity_mount, @@ -1223,6 +1245,7 @@ mod unsafe_pty { fn enter_netns_and_sandbox( netns_fd: Option, policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, #[cfg(target_os = "linux")] supervisor_identity_mount: Option< &crate::process::SupervisorIdentityMountNamespace, @@ -1253,7 +1276,8 @@ mod unsafe_pty { // Drop privileges. initgroups/setgid/setuid need /etc/group and // /etc/passwd which would be blocked if Landlock were already enforced. if enforcement_mode.uses_privileged_process_setup() { - drop_privileges(policy).map_err(|err| std::io::Error::other(err.to_string()))?; + drop_privileges_with_identity(policy, resolved_identity) + .map_err(|err| std::io::Error::other(err.to_string()))?; } crate::process::harden_child_process() .map_err(|err| std::io::Error::other(err.to_string()))?; @@ -1795,6 +1819,7 @@ mod tests { policy, None, None, // no netns fd + ResolvedProcessIdentity::default(), ProcessEnforcementMode::Full, #[cfg(target_os = "linux")] Some( @@ -1827,4 +1852,60 @@ mod tests { "echo output should contain 'drop-privileges-ok'" ); } + + /// SSH pre-exec uses the numeric identity resolved from OCI metadata rather + /// than looking the preserved declaration up through host NSS. + #[cfg(unix)] + #[test] + fn pre_exec_uses_resolved_oci_identity() { + use openshell_core::policy::{ + FilesystemPolicy, LandlockPolicy, NetworkPolicy, ProcessPolicy, SandboxPolicy, + }; + + if rustix::process::geteuid().is_root() { + return; + } + + let policy = SandboxPolicy { + version: 0, + filesystem: FilesystemPolicy::default(), + network: NetworkPolicy::default(), + landlock: LandlockPolicy::default(), + process: ProcessPolicy { + run_as_user: Some("__oci_user_not_in_host_nss__".into()), + run_as_group: Some("__oci_group_not_in_host_nss__".into()), + }, + }; + let resolved = ResolvedProcessIdentity::new( + Some(rustix::process::geteuid().as_raw()), + Some(rustix::process::getegid().as_raw()), + ); + + let mut cmd = Command::new("echo"); + cmd.arg("resolved-identity-ok"); + cmd.stdout(Stdio::piped()); + + unsafe_pty::install_pre_exec_no_pty( + &mut cmd, + policy, + None, + None, + resolved, + ProcessEnforcementMode::Full, + #[cfg(target_os = "linux")] + None, + ) + .expect("install pre_exec should succeed"); + + let output = cmd + .spawn() + .expect("spawn should use resolved numeric identity") + .wait_with_output() + .expect("wait should succeed"); + assert!(output.status.success()); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim(), + "resolved-identity-ok" + ); + } } diff --git a/docs/get-started/tutorials/first-network-policy.mdx b/docs/get-started/tutorials/first-network-policy.mdx index 0639ec9a7f..178bd2f095 100644 --- a/docs/get-started/tutorials/first-network-policy.mdx +++ b/docs/get-started/tutorials/first-network-policy.mdx @@ -114,7 +114,7 @@ network_policies: - { path: /usr/bin/curl } ``` -The `filesystem_policy`, `landlock`, and `process` sections preserve the default sandbox settings. This is required because `policy set` replaces the entire policy. The `network_policies` section is the key part: `curl` can make GET, HEAD, and OPTIONS requests to `api.github.com` over HTTPS. Everything else is denied. The proxy auto-detects TLS on HTTPS endpoints and terminates it to inspect each HTTP request and enforce the `read-only` access preset at the method level. +The `filesystem_policy` and `landlock` sections preserve the default sandbox settings, while process identity is omitted so the active compute driver can select it. These sections are required because `policy set` replaces the entire policy. The `network_policies` section is the key part: `curl` can make GET, HEAD, and OPTIONS requests to `api.github.com` over HTTPS. Everything else is denied. The proxy auto-detects TLS on HTTPS endpoints and terminates it to inspect each HTTP request and enforce the `read-only` access preset at the method level. Apply it: diff --git a/docs/get-started/tutorials/github-sandbox.mdx b/docs/get-started/tutorials/github-sandbox.mdx index 1fdcea4e90..0b11b39345 100644 --- a/docs/get-started/tutorials/github-sandbox.mdx +++ b/docs/get-started/tutorials/github-sandbox.mdx @@ -145,7 +145,7 @@ In terminal 2, paste the deny reason from the previous step into your coding age ```md title="Prompt" wordWrap showLineNumbers={false} Based on the following deny reasons, recommend a sandbox policy update that allows GitHub pushes to `https://github.com//`, and save to `/tmp/sandbox-policy-update.yaml`: -The `filesystem_policy`, `landlock`, and `process` sections are static. They are read once at sandbox creation and cannot be changed by a hot-reload. They are included here for completeness so the file is self-contained, but only the `network_policies` section takes effect when you apply this to a running sandbox. +The `filesystem_policy` and `landlock` sections are static. They are read once at sandbox creation and cannot be changed by a hot reload. They are included here for completeness so the file is self-contained. Process identity is omitted so the active compute driver can select it, and only the `network_policies` section takes effect when you apply this to a running sandbox. ``` The following steps outline the expected process done by the agent: @@ -162,7 +162,7 @@ Refer to the following policy example to compare with the generated policy befor The following YAML shows a complete policy that extends the [default policy](/reference/default-policy) with GitHub access for a single repository. Replace `` with your GitHub organization or username and `` with your repository name. -The `filesystem_policy`, `landlock`, and `process` sections are static. OpenShell reads them at sandbox creation, and a hot reload cannot change them. They are included here for completeness so the file is self-contained, but only the `network_policies` section takes effect when you apply this to a running sandbox. +The `filesystem_policy` and `landlock` sections are static. OpenShell reads them at sandbox creation, and a hot reload cannot change them. They are included here for completeness so the file is self-contained. Process identity is omitted so the active compute driver can select it, and only the `network_policies` section takes effect when you apply this to a running sandbox. ```yaml version: 1 diff --git a/examples/sandbox-policy-quickstart/README.md b/examples/sandbox-policy-quickstart/README.md index 30ee1d092e..ce6b16bfb3 100644 --- a/examples/sandbox-policy-quickstart/README.md +++ b/examples/sandbox-policy-quickstart/README.md @@ -105,8 +105,10 @@ network_policies: - { path: /usr/bin/curl } ``` -The top section preserves the default sandbox filesystem and process -settings (required because `policy set` replaces the entire policy). +The top section preserves the default sandbox filesystem and Landlock +settings while omitting process identity so the active compute driver can +select it. These settings are required because `policy set` replaces the +entire policy. The `network_policies` section is the interesting part: **curl may make GET, HEAD, and OPTIONS requests to `api.github.com` over HTTPS. Everything else is denied.** The proxy terminates TLS (`tls: terminate`) diff --git a/examples/sandbox-policy-quickstart/policy.yaml b/examples/sandbox-policy-quickstart/policy.yaml index 3d4b034377..a17b359ebc 100644 --- a/examples/sandbox-policy-quickstart/policy.yaml +++ b/examples/sandbox-policy-quickstart/policy.yaml @@ -6,9 +6,9 @@ version: 1 -# Default sandbox filesystem and process settings. -# These filesystem fields are required when using `openshell policy set` -# because it replaces the entire policy. +# Default sandbox filesystem and Landlock settings. Process identity is omitted +# so the active compute driver can select it. These fields are required when +# using `openshell policy set` because it replaces the entire policy. filesystem_policy: include_workdir: true read_only: [/usr, /lib, /proc, /dev/urandom, /app, /etc, /var/log] From b89509f29f3b9f12301ed826f0178b3419eabea2 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 08:02:31 -0700 Subject: [PATCH 04/12] fix(supervisor): allow Linux pre-exec arguments Signed-off-by: Matthew Grossman --- crates/openshell-supervisor-process/src/ssh.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 0e555abff8..7fa6482a99 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -1146,6 +1146,7 @@ mod unsafe_pty { } #[allow(unsafe_code)] + #[allow(clippy::too_many_arguments)] #[cfg_attr( not(target_os = "linux"), allow( From 2881ed13aa4fcba76155127b52ddd7ffe697929c Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 08:45:11 -0700 Subject: [PATCH 05/12] fix(kubernetes): protect resolved sandbox identity Signed-off-by: Matthew Grossman --- crates/openshell-core/src/sandbox_env.rs | 10 +- .../openshell-driver-kubernetes/src/driver.rs | 128 ++++++++++++++---- .../src/identity.rs | 28 ++++ e2e/rust/tests/live_policy_update.rs | 8 -- 4 files changed, 132 insertions(+), 42 deletions(-) diff --git a/crates/openshell-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index ec0167bbd3..1549258fa3 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -118,10 +118,12 @@ pub const SANDBOX_GID: &str = "OPENSHELL_SANDBOX_GID"; /// Raw OCI `Config.User` declaration from the immutable image selected by a /// local container driver. /// -/// Docker and Podman always overwrite this value, including with an empty -/// string when the image has no `USER`, and clear [`SANDBOX_UID`] and -/// [`SANDBOX_GID`]. The supervisor resolves omitted policy identity fields -/// from this declaration inside the launched image filesystem. +/// Docker and Podman overwrite this value with the image declaration, +/// including an empty string when the image has no `USER`, and clear +/// [`SANDBOX_UID`] and [`SANDBOX_GID`]. Drivers with an authoritative numeric +/// identity overwrite this value with an empty string while supplying both +/// numeric fields. The supervisor resolves omitted policy identity fields from +/// OCI only for the former contract. pub const OCI_IMAGE_USER: &str = "OPENSHELL_OCI_IMAGE_USER"; // The corporate upstream-proxy configuration deliberately has no reserved diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index c784f10db9..513aaafe0d 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -1624,21 +1624,15 @@ fn apply_supervisor_sideload( volume_mounts.push(supervisor_volume_mount()); } - // Inject resolved sandbox UID/GID as environment variables so the - // supervisor can use them directly without /etc/passwd lookups. + // Inject the protected resolved identity contract. Clearing the OCI + // input prevents image or user environment from selecting a + // conflicting identity path. let env = container .entry("env") .or_insert_with(|| serde_json::json!([])) .as_array_mut(); if let Some(env) = env { - env.push(serde_json::json!({ - "name": openshell_core::sandbox_env::SANDBOX_UID.to_string(), - "value": sandbox_uid.to_string(), - })); - env.push(serde_json::json!({ - "name": openshell_core::sandbox_env::SANDBOX_GID.to_string(), - "value": sandbox_gid.to_string(), - })); + apply_resolved_identity_env(env, sandbox_uid, sandbox_gid); } } } @@ -1728,16 +1722,7 @@ fn supervisor_sidecar_env( openshell_core::sandbox_env::PROXY_TLS_DIR, SIDECAR_TLS_MOUNT_PATH, ); - upsert_env( - &mut env, - openshell_core::sandbox_env::SANDBOX_UID, - ¶ms.sandbox_uid.to_string(), - ); - upsert_env( - &mut env, - openshell_core::sandbox_env::SANDBOX_GID, - ¶ms.sandbox_gid.to_string(), - ); + apply_resolved_identity_env(&mut env, params.sandbox_uid, params.sandbox_gid); if !params.process_binary_aware_network_policy { upsert_env( &mut env, @@ -2015,16 +2000,7 @@ fn apply_supervisor_sidecar_topology( openshell_core::sandbox_env::PROXY_TLS_DIR, SIDECAR_TLS_MOUNT_PATH, ); - upsert_env( - env, - openshell_core::sandbox_env::SANDBOX_UID, - ¶ms.sandbox_uid.to_string(), - ); - upsert_env( - env, - openshell_core::sandbox_env::SANDBOX_GID, - ¶ms.sandbox_gid.to_string(), - ); + apply_resolved_identity_env(env, params.sandbox_uid, params.sandbox_gid); } } @@ -3017,6 +2993,23 @@ fn upsert_env(env: &mut Vec, name: &str, value: &str) { env.push(serde_json::json!({"name": name, "value": value})); } +fn apply_resolved_identity_env(env: &mut Vec, uid: u32, gid: u32) { + remove_env(env, openshell_core::sandbox_env::OCI_IMAGE_USER); + remove_env(env, openshell_core::sandbox_env::SANDBOX_UID); + remove_env(env, openshell_core::sandbox_env::SANDBOX_GID); + upsert_env(env, openshell_core::sandbox_env::OCI_IMAGE_USER, ""); + upsert_env( + env, + openshell_core::sandbox_env::SANDBOX_UID, + &uid.to_string(), + ); + upsert_env( + env, + openshell_core::sandbox_env::SANDBOX_GID, + &gid.to_string(), + ); +} + fn remove_env(env: &mut Vec, name: &str) { env.retain(|item| item.get("name").and_then(|value| value.as_str()) != Some(name)); } @@ -3908,6 +3901,59 @@ mod tests { ); } + #[test] + fn supervisor_sideload_replaces_spoofed_identity_environment() { + let mut pod_template = serde_json::json!({ + "spec": { + "containers": [{ + "name": "agent", + "image": "custom-image:latest", + "env": [ + {"name": openshell_core::sandbox_env::OCI_IMAGE_USER, "value": "spoofed"}, + {"name": openshell_core::sandbox_env::SANDBOX_UID, "value": "9999"}, + {"name": openshell_core::sandbox_env::SANDBOX_GID, "value": "9999"}, + {"name": openshell_core::sandbox_env::OCI_IMAGE_USER, "value": "duplicate"} + ] + }] + } + }); + + apply_supervisor_sideload( + &mut pod_template, + "supervisor-image:latest", + "IfNotPresent", + SupervisorSideloadMethod::InitContainer, + 1500, + 1600, + ); + + let agent = &pod_template["spec"]["containers"][0]; + let env = agent["env"].as_array().unwrap(); + for name in [ + openshell_core::sandbox_env::OCI_IMAGE_USER, + openshell_core::sandbox_env::SANDBOX_UID, + openshell_core::sandbox_env::SANDBOX_GID, + ] { + assert_eq!( + env.iter().filter(|item| item["name"] == name).count(), + 1, + "{name} must have one driver-owned value" + ); + } + assert_eq!( + rendered_env(agent, openshell_core::sandbox_env::OCI_IMAGE_USER), + Some("") + ); + assert_eq!( + rendered_env(agent, openshell_core::sandbox_env::SANDBOX_UID), + Some("1500") + ); + assert_eq!( + rendered_env(agent, openshell_core::sandbox_env::SANDBOX_GID), + Some("1600") + ); + } + #[test] fn supervisor_sideload_adds_security_context_when_missing() { let mut pod_template = serde_json::json!({ @@ -4112,6 +4158,20 @@ mod tests { let pod_template = sandbox_template_to_k8s( &SandboxTemplate { image: "agent-image:latest".to_string(), + environment: std::collections::HashMap::from([ + ( + openshell_core::sandbox_env::OCI_IMAGE_USER.to_string(), + "spoofed".to_string(), + ), + ( + openshell_core::sandbox_env::SANDBOX_UID.to_string(), + "9999".to_string(), + ), + ( + openshell_core::sandbox_env::SANDBOX_GID.to_string(), + "9999".to_string(), + ), + ]), ..SandboxTemplate::default() }, false, @@ -4188,6 +4248,10 @@ mod tests { rendered_env(agent, openshell_core::sandbox_env::SANDBOX_UID), Some("1500") ); + assert_eq!( + rendered_env(agent, openshell_core::sandbox_env::OCI_IMAGE_USER), + Some("") + ); let sidecar = containers .iter() @@ -4233,6 +4297,10 @@ mod tests { rendered_env(sidecar, openshell_core::sandbox_env::SANDBOX_GID), Some("1500") ); + assert_eq!( + rendered_env(sidecar, openshell_core::sandbox_env::OCI_IMAGE_USER), + Some("") + ); assert_eq!( rendered_env(sidecar, openshell_core::sandbox_env::SIDECAR_CONTROL_SOCKET), Some(SIDECAR_CONTROL_SOCKET) diff --git a/crates/openshell-supervisor-process/src/identity.rs b/crates/openshell-supervisor-process/src/identity.rs index adaa0a077f..61e5a0ee41 100644 --- a/crates/openshell-supervisor-process/src/identity.rs +++ b/crates/openshell-supervisor-process/src/identity.rs @@ -42,6 +42,16 @@ impl DriverIdentity { uid: Option, gid: Option, ) -> Result { + // Resolved-identity drivers explicitly clear the OCI declaration so + // an image-baked or user-supplied value cannot select the OCI path. + // Preserve an empty declaration when no resolved pair is present: + // Docker and Podman use that state to reject images without USER. + let oci_user = if oci_user.as_deref() == Some("") && (uid.is_some() || gid.is_some()) { + None + } else { + oci_user + }; + match (oci_user, uid, gid) { (Some(declaration), None, None) => Ok(Self::OciUser { declaration }), (None, Some(uid), Some(gid)) => { @@ -569,6 +579,24 @@ mod tests { gid: 1235 } ); + assert_eq!( + DriverIdentity::from_values( + Some(String::new()), + Some("1234".into()), + Some("1235".into()) + ) + .unwrap(), + DriverIdentity::Resolved { + uid: 1234, + gid: 1235 + } + ); + assert_eq!( + DriverIdentity::from_values(Some(String::new()), None, None).unwrap(), + DriverIdentity::OciUser { + declaration: String::new() + } + ); assert_eq!( DriverIdentity::from_values(None, None, None).unwrap(), DriverIdentity::None diff --git a/e2e/rust/tests/live_policy_update.rs b/e2e/rust/tests/live_policy_update.rs index 423b260946..d38cb18c95 100644 --- a/e2e/rust/tests/live_policy_update.rs +++ b/e2e/rust/tests/live_policy_update.rs @@ -103,10 +103,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - network_policies: {network_rules}" ); @@ -141,10 +137,6 @@ filesystem_policy: landlock: compatibility: best_effort - -process: - run_as_user: sandbox - run_as_group: sandbox "; file.write_all(policy.as_bytes()) From 51476e81a21ea910d7a51f414c17ec64f9fbff31 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 11:20:56 -0700 Subject: [PATCH 06/12] fix(sandbox): prepare workspace for OCI identity Signed-off-by: Matthew Grossman --- architecture/compute-runtimes.md | 7 + crates/openshell-driver-docker/README.md | 6 + crates/openshell-driver-podman/README.md | 6 + .../src/process.rs | 193 ++++++++++++------ docs/reference/sandbox-compute-drivers.mdx | 11 +- e2e/rust/Cargo.toml | 2 +- e2e/rust/tests/custom_image.rs | 51 +++-- e2e/rust/tests/driver_config_volume.rs | 86 ++++++++ e2e/rust/tests/live_policy_update.rs | 75 +++---- examples/bring-your-own-container/Dockerfile | 4 +- examples/bring-your-own-container/README.md | 3 + 11 files changed, 317 insertions(+), 127 deletions(-) diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 1629c50de4..9bcf9eb59e 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -144,6 +144,13 @@ and uses the same privilege-drop path for direct and SSH children. When a declaration omits the group, the supervisor fills it with the user's numeric primary GID. It does not rewrite the account files. +When either component falls back to the image identity, Docker and Podman +prepare the fixed `/sandbox` workspace for the completed UID/GID before direct +or SSH children start. They create it when absent and recursively transfer +ownership of image-provided contents, while skipping symlinks and nested mount +points. Kubernetes/OpenShift keep their PVC and `fsGroup` behavior, and VM +keeps its guest initialization path. + Sandbox creation fails before the workload becomes ready when a required image identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0. The supervisor itself remains root so it can establish isolation before diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index b2e74231ba..ae2f97dc42 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -27,6 +27,12 @@ identity. Named OCI components remain names after validation; a missing group is filled with the user's numeric primary GID. Explicit `process.run_as_user` and `process.run_as_group` values take precedence independently. +When either policy field falls back to OCI `USER`, the supervisor creates the +fixed `/sandbox` workspace if necessary and transfers ownership of its existing +image contents to the completed identity before direct or SSH children start. +It skips symlinks and nested bind or volume mounts so externally owned content +retains its ownership. A read-only workspace root fails sandbox startup. + Docker containers join an OpenShell-managed bridge network. The driver injects `host.openshell.internal` and `host.docker.internal` so supervisors have stable names for reaching the gateway host. On Docker Desktop, Colima, Rancher diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 965a295d19..c8e793f121 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -17,6 +17,12 @@ validation; a missing group is filled with the user's numeric primary GID. Expli `process.run_as_user` and `process.run_as_group` values take precedence independently. +When either policy field falls back to OCI `USER`, the supervisor creates the +fixed `/sandbox` workspace if necessary and transfers ownership of its existing +image contents to the completed identity before direct or SSH children start. +It skips symlinks and nested bind or volume mounts so externally owned content +retains its ownership. A read-only workspace root fails sandbox startup. + For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md). ## Architecture diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 13fd6e2efa..b656a2793d 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -14,6 +14,8 @@ use nix::sys::signal::{self, Signal}; use nix::unistd::{Gid, Group, Pid, Uid, User}; use openshell_core::policy::{NetworkMode, SandboxPolicy}; use std::collections::HashMap; +#[cfg(unix)] +use std::collections::HashSet; use std::ffi::CString; #[cfg(target_os = "linux")] use std::os::fd::{AsRawFd, OwnedFd, RawFd}; @@ -67,6 +69,16 @@ impl ResolvedProcessIdentity { pub const fn gid(self) -> Option { self.gid } + + /// Whether at least one process identity component came from OCI `USER`. + /// + /// Platform-resolved identities are written directly into the policy and + /// return the default value, so this is specific to Docker/Podman OCI + /// fallback without adding another driver contract. + #[must_use] + pub const fn uses_oci_user_fallback(self) -> bool { + self.uid.is_some() || self.gid.is_some() + } } impl ProcessEnforcementMode { @@ -1221,13 +1233,16 @@ fn rewrite_group_at(path: &Path, gid: &str) -> Result<()> { /// malicious container images. The TOCTOU window is not exploitable because /// no untrusted process is running yet. /// -/// The root path is chowned unconditionally — EROFS there is a hard error -/// (a read-only `/sandbox` is a misconfiguration). For children, `EROFS` -/// causes the walker to skip that path and its entire subtree — descending -/// into a read-only mount we do not control would be a TOCTOU risk -/// (CWE-367/CWE-59). Siblings of the read-only path are still visited. +/// The root path is chowned unconditionally — EROFS there is a hard error (a +/// read-only `/sandbox` is a misconfiguration). Nested mount points are skipped +/// before chown so user-provided mounts retain their ownership. #[cfg(unix)] -fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result<()> { +fn chown_sandbox_home( + root: &Path, + uid: Option, + gid: Option, + nested_mounts: &HashSet, +) -> Result<()> { let meta = std::fs::symlink_metadata(root).into_diagnostic()?; if meta.file_type().is_symlink() { return Err(miette::miette!( @@ -1239,19 +1254,20 @@ fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result nix::unistd::chown(root, uid, gid).into_diagnostic()?; if meta.is_dir() { - chown_children(root, uid, gid, &nix::unistd::chown)?; + chown_children(root, uid, gid, nested_mounts, &nix::unistd::chown)?; } Ok(()) } -/// Walk directory children and chown each entry, skipping symlinks and -/// EROFS subtrees. Called after the parent has already been chowned. +/// Walk directory children and chown each entry, skipping symlinks and nested +/// mount points. Called after the parent has already been chowned. #[cfg(unix)] fn chown_children( dir: &Path, uid: Option, gid: Option, + nested_mounts: &HashSet, do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, ) -> Result<()> { match std::fs::read_dir(dir) { @@ -1259,7 +1275,7 @@ fn chown_children( for entry in entries { let entry = entry.into_diagnostic()?; let child = entry.path(); - chown_recursive(&child, uid, gid, do_chown)?; + chown_recursive(&child, uid, gid, nested_mounts, do_chown)?; } } Err(e) => { @@ -1274,8 +1290,14 @@ fn chown_recursive( path: &Path, uid: Option, gid: Option, + nested_mounts: &HashSet, do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, ) -> Result<()> { + if nested_mounts.contains(path) { + debug!(path = %path.display(), "Skipping nested mount during sandbox home chown"); + return Ok(()); + } + let meta = std::fs::symlink_metadata(path).into_diagnostic()?; if meta.file_type().is_symlink() { @@ -1283,21 +1305,76 @@ fn chown_recursive( return Ok(()); } - if let Err(e) = do_chown(path, uid, gid) { - if e == nix::errno::Errno::EROFS { - debug!(path = %path.display(), "Skipping read-only path during sandbox home chown"); - return Ok(()); - } - return Err(e).into_diagnostic(); - } + do_chown(path, uid, gid).into_diagnostic()?; if meta.is_dir() { - chown_children(path, uid, gid, do_chown)?; + chown_children(path, uid, gid, nested_mounts, do_chown)?; } Ok(()) } +/// Return mount points strictly beneath `root`. +/// +/// Linux mountinfo escapes whitespace and backslashes as octal sequences. +/// Snapshotting before any workload child starts lets the ownership walk avoid +/// crossing bind mounts and volumes without a race with untrusted code. +#[cfg(target_os = "linux")] +fn nested_mount_points(root: &Path) -> Result> { + use std::os::unix::ffi::OsStringExt; + + fn decode_mountinfo_path(value: &str) -> PathBuf { + let input = value.as_bytes(); + let mut decoded = Vec::with_capacity(input.len()); + let mut index = 0; + while index < input.len() { + if input[index] == b'\\' + && index + 3 < input.len() + && input[index + 1..=index + 3] + .iter() + .all(|byte| (b'0'..=b'7').contains(byte)) + { + let octal = &input[index + 1..=index + 3]; + let value = (octal[0] - b'0') * 64 + (octal[1] - b'0') * 8 + (octal[2] - b'0'); + decoded.push(value); + index += 4; + } else { + decoded.push(input[index]); + index += 1; + } + } + PathBuf::from(std::ffi::OsString::from_vec(decoded)) + } + + let mountinfo = std::fs::read_to_string("/proc/self/mountinfo").into_diagnostic()?; + Ok(mountinfo + .lines() + .filter_map(|line| line.split_whitespace().nth(4)) + .map(decode_mountinfo_path) + .filter(|mount| mount != root && mount.starts_with(root)) + .collect()) +} + +#[cfg(all(unix, not(target_os = "linux")))] +#[allow(clippy::unnecessary_wraps)] +fn nested_mount_points(_root: &Path) -> Result> { + Ok(HashSet::new()) +} + +#[cfg(unix)] +fn prepare_oci_workspace(root: &Path, uid: Option, gid: Option) -> Result<()> { + match std::fs::symlink_metadata(root) { + Ok(_) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + std::fs::create_dir_all(root).into_diagnostic()?; + } + Err(error) => return Err(error).into_diagnostic(), + } + + let mounts = nested_mount_points(root)?; + chown_sandbox_home(root, uid, gid, &mounts) +} + /// Prepare filesystem for the sandboxed process. /// /// Creates `read_write` directories if they don't exist and sets ownership @@ -1370,17 +1447,14 @@ pub fn prepare_filesystem_with_identity( } } - // When a driver injects a custom UID/GID via environment variables, the - // /sandbox home directory may already exist with image-default ownership - // (e.g. UID 1000) that differs from the driver-assigned identity. - // Recursively chown /sandbox so the sandbox process can use its home - // directory. - if std::env::var(openshell_core::sandbox_env::SANDBOX_UID).is_ok() { + // Docker and Podman must make OpenShell's fixed workspace usable when any + // identity component came from OCI USER. Explicit policy identity and + // platform-resolved Kubernetes/OpenShift identity retain their existing + // filesystem paths. + if resolved_identity.uses_oci_user_fallback() { let sandbox_home = Path::new("/sandbox"); - if sandbox_home.exists() { - info!(?uid, ?gid, "Chowning /sandbox for driver-injected UID/GID"); - chown_sandbox_home(sandbox_home, uid, gid)?; - } + info!(?uid, ?gid, "Preparing /sandbox for OCI image identity"); + prepare_oci_workspace(sandbox_home, uid, gid)?; } Ok(()) @@ -2243,7 +2317,13 @@ mod tests { let expected_uid = nix::unistd::geteuid(); let expected_gid = nix::unistd::getegid(); - chown_sandbox_home(&root, Some(expected_uid), Some(expected_gid)).unwrap(); + chown_sandbox_home( + &root, + Some(expected_uid), + Some(expected_gid), + &HashSet::new(), + ) + .unwrap(); for path in &[ root.clone(), @@ -2282,6 +2362,7 @@ mod tests { &link, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), + &HashSet::new(), ) .unwrap_err(); assert!( @@ -2306,22 +2387,23 @@ mod tests { &root, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), + &HashSet::new(), ) .expect("should skip symlink children without error"); } #[cfg(unix)] #[test] - fn chown_recursive_skips_erofs_subtree_but_continues_siblings() { + fn chown_recursive_skips_nested_mount_but_continues_siblings() { use std::sync::{Arc, Mutex}; let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); std::fs::create_dir(&root).unwrap(); - let readonly_dir = root.join("ro-mount"); - std::fs::create_dir(&readonly_dir).unwrap(); - std::fs::write(readonly_dir.join("child-under-ro.txt"), "data").unwrap(); + let mount_dir = root.join("mounted"); + std::fs::create_dir(&mount_dir).unwrap(); + std::fs::write(mount_dir.join("mounted-file.txt"), "data").unwrap(); std::fs::write(root.join("writable-sibling.txt"), "data").unwrap(); @@ -2331,22 +2413,24 @@ mod tests { let chowned: Arc>> = Arc::new(Mutex::new(Vec::new())); let chowned_ref = Arc::clone(&chowned); - let readonly_dir_clone = readonly_dir.clone(); let fake_chown = move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { - if path == readonly_dir_clone { - return Err(nix::errno::Errno::EROFS); - } chowned_ref.lock().unwrap().push(path.to_path_buf()); Ok(()) }; + let nested_mounts = HashSet::from([mount_dir.clone()]); - chown_children(&root, uid, gid, &fake_chown).expect("EROFS should be handled gracefully"); + chown_children(&root, uid, gid, &nested_mounts, &fake_chown) + .expect("nested mount should be skipped"); let chowned = chowned.lock().unwrap(); assert!( - !chowned.contains(&readonly_dir.join("child-under-ro.txt")), - "children under EROFS directory must NOT be descended into" + !chowned.contains(&mount_dir), + "nested mount root must not be chowned" + ); + assert!( + !chowned.contains(&mount_dir.join("mounted-file.txt")), + "nested mount contents must not be descended into" ); assert!( chowned.contains(&root.join("writable-sibling.txt")), @@ -2356,7 +2440,7 @@ mod tests { #[cfg(unix)] #[test] - fn chown_recursive_propagates_non_erofs_errors() { + fn chown_recursive_propagates_chown_errors() { let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); std::fs::create_dir(&root).unwrap(); @@ -2368,29 +2452,24 @@ mod tests { Err(nix::errno::Errno::EPERM) }; - let result = chown_recursive(&root, uid, gid, &fake_chown); - assert!(result.is_err(), "non-EROFS errors should propagate"); + let result = chown_recursive(&root, uid, gid, &HashSet::new(), &fake_chown); + assert!(result.is_err(), "chown errors should propagate"); } #[cfg(unix)] #[test] - fn chown_children_skips_all_erofs_children_gracefully() { + fn prepare_oci_workspace_creates_missing_root() { let dir = tempfile::tempdir().unwrap(); - let root = dir.path().join("sandbox"); - std::fs::create_dir(&root).unwrap(); - std::fs::create_dir(root.join("a")).unwrap(); - std::fs::write(root.join("b.txt"), "data").unwrap(); - - let uid = Some(nix::unistd::geteuid()); - let gid = Some(nix::unistd::getegid()); + let missing = dir.path().join("missing").join("sandbox"); - let always_erofs = |_path: &Path, - _uid: Option, - _gid: Option| - -> nix::Result<()> { Err(nix::errno::Errno::EROFS) }; + prepare_oci_workspace( + &missing, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + ) + .expect("missing OCI workspace should be created"); - chown_children(&root, uid, gid, &always_erofs) - .expect("EROFS on all children should be skipped gracefully"); + assert!(missing.is_dir()); } #[cfg(unix)] diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index d572572991..695ca29e4e 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -432,6 +432,14 @@ declared name or numeric components for both direct and SSH children. When `USER` omits the group, the supervisor uses the user's numeric primary GID. It does not modify `/etc/passwd` or `/etc/group`. +When either identity component falls back to OCI `USER`, OpenShell creates the +fixed `/sandbox` workspace if it is absent and transfers ownership of its +existing image contents to the completed UID/GID. The ownership walk skips +symlinks and nested bind or volume mounts. Sandbox startup fails before +readiness when the workspace root cannot be prepared, including when it is +read-only. Images may pre-own `/sandbox` as a build optimization, but this is +not required. + Sandbox creation fails before readiness if a required `USER` component is missing, malformed, unknown, ambiguous, or resolves to UID/GID 0. An image without `USER` therefore works only when policy explicitly provides both @@ -461,4 +469,5 @@ The VM driver injects the sandbox UID into the rootfs guest's `/etc/passwd`, `/e Docker and Podman custom images do not need a baked-in `"sandbox"` user. Declare a non-root OCI `USER`, or set both process identity fields explicitly in policy. Named image users require matching account entries; a numeric `UID:GID` pair -does not. +does not. OpenShell continues to use `/sandbox` as the workspace; it does not +adopt the image's OCI working directory. diff --git a/e2e/rust/Cargo.toml b/e2e/rust/Cargo.toml index b36a32203f..4d44ac55d1 100644 --- a/e2e/rust/Cargo.toml +++ b/e2e/rust/Cargo.toml @@ -35,7 +35,7 @@ e2e-vm = ["e2e", "e2e-host-gateway"] [[test]] name = "custom_image" path = "tests/custom_image.rs" -required-features = ["e2e-docker"] +required-features = ["e2e-local-container-driver"] [[test]] name = "docker_preflight" diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index a1f56bbb2f..23fe63773e 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -1,13 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -#![cfg(feature = "e2e")] +#![cfg(feature = "e2e-local-container-driver")] -//! E2E test: build a custom container image and run a sandbox with it. +//! E2E test: build custom container images and run sandboxes with them. //! //! Prerequisites: -//! - A running Docker-backed openshell gateway (`mise run gateway:docker`) -//! - Docker daemon running (for image build) +//! - A running Docker- or Podman-backed openshell gateway +//! - The matching container runtime running (for image builds) //! - The `openshell` binary (built automatically from the workspace) use std::io::Write; @@ -24,6 +24,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ RUN groupadd -g 1235 appstaff && \ useradd -m -u 1234 -g appstaff app +# A custom image may already contain a root-owned OpenShell workspace and +# root-owned files. The supervisor prepares both for the OCI identity. +RUN install -d /sandbox && \ + printf root-owned > /sandbox/root-owned.txt + # Write a marker file so we can verify this is our custom image. # Place under /etc (Landlock baseline read-only path) so the sandbox # can read it when filesystem restrictions are properly enforced. @@ -44,8 +49,8 @@ CMD ["sleep", "infinity"] const MARKER: &str = "custom-image-e2e-marker"; -/// Build a custom Docker image from a Dockerfile and verify that a sandbox -/// created from it contains the expected marker file. +/// A named OCI user can write through direct and SSH children even when the +/// image starts with a root-owned `/sandbox` tree. #[tokio::test] async fn sandbox_from_custom_dockerfile() { // Step 1: Write a temporary Dockerfile. @@ -59,14 +64,17 @@ async fn sandbox_from_custom_dockerfile() { // Step 2: Create a sandbox from the Dockerfile. let dockerfile_str = dockerfile_path.to_str().expect("Dockerfile path is UTF-8"); - let mut guard = SandboxGuard::create(&[ - "--from", - dockerfile_str, - "--", - "sh", - "-c", - "id -u; id -g; touch /sandbox/oci-user-write; cat /etc/marker.txt", - ]) + let mut guard = SandboxGuard::create_keep_with_args( + &["--from", dockerfile_str, "--no-tty"], + &[ + "sh", + "-c", + "set -eu; id -u; id -g; test \"$(cat /sandbox/root-owned.txt)\" = root-owned; \ + test \"$(stat -c %u:%g /sandbox/root-owned.txt)\" = 1234:1235; \ + touch /sandbox/direct-oci-user-write; cat /etc/marker.txt; echo Ready; sleep infinity", + ], + "Ready", + ) .await .expect("sandbox create from Dockerfile"); @@ -81,11 +89,26 @@ async fn sandbox_from_custom_dockerfile() { "expected named OCI identity 1234:1235 in sandbox output:\n{clean_output}" ); + let ssh_output = guard + .exec(&[ + "sh", + "-c", + "set -eu; test \"$(id -u):$(id -g)\" = 1234:1235; \ + touch /sandbox/ssh-oci-user-write; echo ssh-write-ok", + ]) + .await + .expect("SSH child should write to prepared workspace"); + assert!( + ssh_output.contains("ssh-write-ok"), + "expected SSH write marker:\n{ssh_output}" + ); + // Explicit cleanup (also happens in Drop, but explicit is clearer in tests). guard.cleanup().await; } /// A numeric OCI user/group pair works without passwd or group entries. +/// The image intentionally has no pre-existing `/sandbox`. #[tokio::test] async fn sandbox_from_passwd_less_numeric_oci_user() { let tmpdir = tempfile::tempdir().expect("create tmpdir"); diff --git a/e2e/rust/tests/driver_config_volume.rs b/e2e/rust/tests/driver_config_volume.rs index ad8cffc2f9..bebc0cb09e 100644 --- a/e2e/rust/tests/driver_config_volume.rs +++ b/e2e/rust/tests/driver_config_volume.rs @@ -23,6 +23,14 @@ use serde_json::{Map, Value}; const TEST_IMAGE: &str = "ghcr.io/nvidia/openshell-community/sandboxes/base:latest"; const VOLUME_TARGET: &str = "/sandbox/e2e-volume"; const BIND_TARGET: &str = "/sandbox/e2e-bind"; +const OCI_USER_DOCKERFILE: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim + +RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ + && rm -rf /var/lib/apt/lists/* + +USER 2234:2235 +CMD ["sleep", "infinity"] +"#; struct VolumeGuard { docker: Docker, @@ -101,6 +109,68 @@ async fn sandbox_mounts_existing_driver_config_volume() { .expect("verify sandbox wrote to named test volume"); } +#[tokio::test] +async fn oci_workspace_preparation_skips_nested_volume_ownership() { + let driver = e2e_driver().expect("OPENSHELL_E2E_DRIVER must be set by the e2e wrapper"); + assert!( + matches!(driver.as_str(), "docker" | "podman"), + "OCI workspace mount e2e requires docker or podman, got {driver}" + ); + + let volume = VolumeGuard::create(&driver) + .await + .expect("create named test volume"); + seed_volume(&volume).await.expect("seed named test volume"); + + let image_context = tempfile::tempdir().expect("create OCI image context"); + let dockerfile = image_context.path().join("Dockerfile"); + fs::write(&dockerfile, OCI_USER_DOCKERFILE).expect("write OCI image Dockerfile"); + let dockerfile = dockerfile.to_str().expect("Dockerfile path must be UTF-8"); + + let driver_config = format!( + r#"{{"{driver}":{{"mounts":[{{"type":"volume","source":"{}","target":"{VOLUME_TARGET}","read_only":false}}]}}}}"#, + volume.name + ); + let mut sandbox = SandboxGuard::create_keep_with_args( + &[ + "--from", + dockerfile, + "--driver-config-json", + &driver_config, + "--no-tty", + ], + &[ + "sh", + "-lc", + "set -eu; test \"$(id -u):$(id -g)\" = 2234:2235; \ + test \"$(stat -c %u:%g /sandbox/e2e-volume/input.txt)\" = 0:0; \ + touch /sandbox/direct-write; echo Ready; sleep infinity", + ], + "Ready", + ) + .await + .expect("create OCI-user sandbox with nested volume"); + + let ssh_output = sandbox + .exec(&[ + "sh", + "-lc", + "set -eu; test \"$(stat -c %u:%g /sandbox/e2e-volume/input.txt)\" = 0:0; \ + touch /sandbox/ssh-write; echo nested-mount-owner-ok", + ]) + .await + .expect("SSH child should preserve nested volume ownership"); + assert!( + ssh_output.contains("nested-mount-owner-ok"), + "expected nested mount ownership marker:\n{ssh_output}" + ); + + sandbox.cleanup().await; + verify_volume_ownership(&volume) + .await + .expect("nested volume ownership should remain unchanged"); +} + #[tokio::test] async fn sandbox_mounts_enabled_driver_config_bind() { let driver = e2e_driver().expect("OPENSHELL_E2E_DRIVER must be set by the e2e wrapper"); @@ -208,6 +278,22 @@ async fn verify_volume(volume: &VolumeGuard) -> Result<(), String> { Ok(()) } +async fn verify_volume_ownership(volume: &VolumeGuard) -> Result<(), String> { + let output = run_volume_container( + volume, + "verify-owner", + true, + "set -eu; test \"$(stat -c %u:%g /vol/input.txt)\" = 0:0; echo owner-ok", + ) + .await?; + if !output.contains("owner-ok") { + return Err(format!( + "volume ownership verification did not print expected marker:\n{output}" + )); + } + Ok(()) +} + async fn run_volume_container( volume: &VolumeGuard, purpose: &str, diff --git a/e2e/rust/tests/live_policy_update.rs b/e2e/rust/tests/live_policy_update.rs index d38cb18c95..7a1e12923a 100644 --- a/e2e/rust/tests/live_policy_update.rs +++ b/e2e/rust/tests/live_policy_update.rs @@ -245,8 +245,8 @@ fn list_output_contains_version(output: &str, version: u32) -> bool { /// Test the full live policy update lifecycle: /// -/// 1. Create sandbox with `--keep` -/// 2. Set policy A, verify initial version >= 1 +/// 1. Create sandbox with policy A and `--keep` +/// 2. Verify initial version >= 1 /// 3. Push same policy A -> version unchanged (idempotent) /// 4. Push policy B (adds example.com) with `--wait` -> new version /// 5. Push policy B again -> idempotent @@ -269,29 +269,14 @@ async fn live_policy_update_round_trip() { .expect("policy B path should be utf-8") .to_string(); - // --- Create a long-running sandbox --- - let mut guard = - SandboxGuard::create_keep(&["sh", "-c", "echo Ready && sleep infinity"], "Ready") - .await - .expect("create keep sandbox"); - - // --- Set initial policy A --- - let r = run_cli(&[ - "policy", - "set", - &guard.name, - "--policy", - &policy_a_path, - "--wait", - "--timeout", - "120", - ]) - .await; - assert!( - r.success, - "policy set A should succeed (exit {:?}):\n{}", - r.exit_code, r.output - ); + // --- Create a long-running sandbox with its startup-only policy fields --- + let mut guard = SandboxGuard::create_keep_with_args( + &["--policy", &policy_a_path, "--no-tty"], + &["sh", "-c", "echo Ready && sleep infinity"], + "Ready", + ) + .await + .expect("create keep sandbox with policy A"); // --- Verify initial policy version --- let r = run_cli(&["policy", "get", &guard.name]).await; @@ -443,10 +428,9 @@ async fn live_policy_update_round_trip() { /// Test live policy update from an initially empty network policy: /// -/// 1. Create sandbox with `--keep` -/// 2. Set policy with no network rules -/// 3. Push policy with a network rule using `--wait` -/// 4. Verify the version bumped +/// 1. Create sandbox with no network rules and `--keep` +/// 2. Push policy with a network rule using `--wait` +/// 3. Verify the version bumped #[tokio::test] async fn live_policy_update_from_empty_network_policies() { let empty_policy = write_empty_network_policy().expect("write empty network policy"); @@ -463,29 +447,16 @@ async fn live_policy_update_from_empty_network_policies() { .expect("full policy path should be utf-8") .to_string(); - // Create sandbox with empty network policy. - let mut guard = - SandboxGuard::create_keep(&["sh", "-c", "echo Ready && sleep infinity"], "Ready") - .await - .expect("create keep sandbox"); - - // Set initial empty policy. - let r = run_cli(&[ - "policy", - "set", - &guard.name, - "--policy", - &empty_path, - "--wait", - "--timeout", - "120", - ]) - .await; - assert!( - r.success, - "policy set (empty) should succeed (exit {:?}):\n{}", - r.exit_code, r.output - ); + // Create the sandbox with the empty network policy so subsequent live + // updates retain the same startup-only filesystem, landlock, and process + // fields. + let mut guard = SandboxGuard::create_keep_with_args( + &["--policy", &empty_path, "--no-tty"], + &["sh", "-c", "echo Ready && sleep infinity"], + "Ready", + ) + .await + .expect("create keep sandbox with empty network policy"); let r = run_cli(&["policy", "get", &guard.name]).await; assert!( diff --git a/examples/bring-your-own-container/Dockerfile b/examples/bring-your-own-container/Dockerfile index 6e5d3492fa..4b8ccf8abe 100644 --- a/examples/bring-your-own-container/Dockerfile +++ b/examples/bring-your-own-container/Dockerfile @@ -17,9 +17,9 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ RUN groupadd --gid 1500 app \ && useradd --uid 1500 --gid app --create-home app -RUN install -d /sandbox +RUN install -d -o app -g app /sandbox WORKDIR /sandbox -COPY app.py . +COPY --chown=app:app app.py . EXPOSE 8080 diff --git a/examples/bring-your-own-container/README.md b/examples/bring-your-own-container/README.md index dfb97b0be9..0b3ce014e0 100644 --- a/examples/bring-your-own-container/README.md +++ b/examples/bring-your-own-container/README.md @@ -63,6 +63,9 @@ key requirements are: such as `app`, a numeric UID with a passwd entry that supplies its primary GID, or a numeric pair such as `1500:1500`. You can instead set both `process.run_as_user` and `process.run_as_group` explicitly in policy. +- **Use `/sandbox` as the workspace.** Pre-owning it in the image is good image + hygiene and avoids startup ownership work, but Docker and Podman create and + prepare it automatically when OCI `USER` supplies any identity component. - **Install `iproute2`** for full network namespace isolation. - **Use a standard Linux base image** — distroless and `FROM scratch` images are not supported. From 1b2583acd87a0e1c685200cd5819697a9826cd81 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 11:36:47 -0700 Subject: [PATCH 07/12] refactor(sandbox): own only workspace root Signed-off-by: Matthew Grossman --- architecture/compute-runtimes.md | 8 +- crates/openshell-driver-docker/README.md | 8 +- crates/openshell-driver-podman/README.md | 8 +- .../src/process.rs | 303 +++++------------- docs/reference/sandbox-compute-drivers.mdx | 12 +- e2e/rust/tests/custom_image.rs | 9 +- examples/bring-your-own-container/README.md | 7 +- 7 files changed, 100 insertions(+), 255 deletions(-) diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 9bcf9eb59e..85124644e6 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -146,10 +146,10 @@ primary GID. It does not rewrite the account files. When either component falls back to the image identity, Docker and Podman prepare the fixed `/sandbox` workspace for the completed UID/GID before direct -or SSH children start. They create it when absent and recursively transfer -ownership of image-provided contents, while skipping symlinks and nested mount -points. Kubernetes/OpenShift keep their PVC and `fsGroup` behavior, and VM -keeps its guest initialization path. +or SSH children start. They create it when absent and transfer ownership of +only the workspace directory. Image-provided contents and nested mounts retain +their ownership. Kubernetes/OpenShift keep their PVC and `fsGroup` behavior, +and VM keeps its guest initialization path. Sandbox creation fails before the workload becomes ready when a required image identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0. diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index ae2f97dc42..bec7f500f3 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -28,10 +28,10 @@ is filled with the user's numeric primary GID. Explicit `process.run_as_user` and `process.run_as_group` values take precedence independently. When either policy field falls back to OCI `USER`, the supervisor creates the -fixed `/sandbox` workspace if necessary and transfers ownership of its existing -image contents to the completed identity before direct or SSH children start. -It skips symlinks and nested bind or volume mounts so externally owned content -retains its ownership. A read-only workspace root fails sandbox startup. +fixed `/sandbox` workspace if necessary and transfers ownership of only that +directory to the completed identity before direct or SSH children start. +Image-provided contents and nested bind or volume mounts retain their ownership. +A read-only workspace root fails sandbox startup. Docker containers join an OpenShell-managed bridge network. The driver injects `host.openshell.internal` and `host.docker.internal` so supervisors have stable diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index c8e793f121..f075a61b95 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -18,10 +18,10 @@ validation; a missing group is filled with the user's numeric primary GID. Expli independently. When either policy field falls back to OCI `USER`, the supervisor creates the -fixed `/sandbox` workspace if necessary and transfers ownership of its existing -image contents to the completed identity before direct or SSH children start. -It skips symlinks and nested bind or volume mounts so externally owned content -retains its ownership. A read-only workspace root fails sandbox startup. +fixed `/sandbox` workspace if necessary and transfers ownership of only that +directory to the completed identity before direct or SSH children start. +Image-provided contents and nested bind or volume mounts retain their ownership. +A read-only workspace root fails sandbox startup. For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md). diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index b656a2793d..9b497faf10 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -14,8 +14,6 @@ use nix::sys::signal::{self, Signal}; use nix::unistd::{Gid, Group, Pid, Uid, User}; use openshell_core::policy::{NetworkMode, SandboxPolicy}; use std::collections::HashMap; -#[cfg(unix)] -use std::collections::HashSet; use std::ffi::CString; #[cfg(target_os = "linux")] use std::os::fd::{AsRawFd, OwnedFd, RawFd}; @@ -1227,143 +1225,36 @@ fn rewrite_group_at(path: &Path, gid: &str) -> Result<()> { Ok(()) } -/// Recursively chown a directory tree to the given UID/GID. -/// -/// Symlinks are skipped (not followed) to prevent privilege escalation via -/// malicious container images. The TOCTOU window is not exploitable because -/// no untrusted process is running yet. -/// -/// The root path is chowned unconditionally — EROFS there is a hard error (a -/// read-only `/sandbox` is a misconfiguration). Nested mount points are skipped -/// before chown so user-provided mounts retain their ownership. #[cfg(unix)] -fn chown_sandbox_home( - root: &Path, - uid: Option, - gid: Option, - nested_mounts: &HashSet, -) -> Result<()> { - let meta = std::fs::symlink_metadata(root).into_diagnostic()?; - if meta.file_type().is_symlink() { - return Err(miette::miette!( - "path '{}' is a symlink — refusing to chown (potential privilege escalation)", - root.display() - )); - } - - nix::unistd::chown(root, uid, gid).into_diagnostic()?; - - if meta.is_dir() { - chown_children(root, uid, gid, nested_mounts, &nix::unistd::chown)?; - } - - Ok(()) +fn prepare_oci_workspace(root: &Path, uid: Option, gid: Option) -> Result<()> { + prepare_oci_workspace_with(root, uid, gid, &nix::unistd::chown) } -/// Walk directory children and chown each entry, skipping symlinks and nested -/// mount points. Called after the parent has already been chowned. +/// Prepare only the fixed `OpenShell` workspace directory itself. +/// +/// Image-provided children retain their declared ownership. This avoids +/// crossing symlinks or user-provided nested mounts and keeps OCI working +/// directory/content semantics out of the identity-focused change. #[cfg(unix)] -fn chown_children( - dir: &Path, +fn prepare_oci_workspace_with( + root: &Path, uid: Option, gid: Option, - nested_mounts: &HashSet, do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, ) -> Result<()> { - match std::fs::read_dir(dir) { - Ok(entries) => { - for entry in entries { - let entry = entry.into_diagnostic()?; - let child = entry.path(); - chown_recursive(&child, uid, gid, nested_mounts, do_chown)?; - } - } - Err(e) => { - debug!(path = %dir.display(), error = %e, "Cannot list directory during sandbox home chown"); + match std::fs::symlink_metadata(root) { + Ok(metadata) if metadata.file_type().is_symlink() => { + return Err(miette::miette!( + "workspace path '{}' is a symlink — refusing to chown", + root.display() + )); } - } - Ok(()) -} - -#[cfg(unix)] -fn chown_recursive( - path: &Path, - uid: Option, - gid: Option, - nested_mounts: &HashSet, - do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, -) -> Result<()> { - if nested_mounts.contains(path) { - debug!(path = %path.display(), "Skipping nested mount during sandbox home chown"); - return Ok(()); - } - - let meta = std::fs::symlink_metadata(path).into_diagnostic()?; - - if meta.file_type().is_symlink() { - debug!(path = %path.display(), "Skipping symlink during sandbox home chown"); - return Ok(()); - } - - do_chown(path, uid, gid).into_diagnostic()?; - - if meta.is_dir() { - chown_children(path, uid, gid, nested_mounts, do_chown)?; - } - - Ok(()) -} - -/// Return mount points strictly beneath `root`. -/// -/// Linux mountinfo escapes whitespace and backslashes as octal sequences. -/// Snapshotting before any workload child starts lets the ownership walk avoid -/// crossing bind mounts and volumes without a race with untrusted code. -#[cfg(target_os = "linux")] -fn nested_mount_points(root: &Path) -> Result> { - use std::os::unix::ffi::OsStringExt; - - fn decode_mountinfo_path(value: &str) -> PathBuf { - let input = value.as_bytes(); - let mut decoded = Vec::with_capacity(input.len()); - let mut index = 0; - while index < input.len() { - if input[index] == b'\\' - && index + 3 < input.len() - && input[index + 1..=index + 3] - .iter() - .all(|byte| (b'0'..=b'7').contains(byte)) - { - let octal = &input[index + 1..=index + 3]; - let value = (octal[0] - b'0') * 64 + (octal[1] - b'0') * 8 + (octal[2] - b'0'); - decoded.push(value); - index += 4; - } else { - decoded.push(input[index]); - index += 1; - } + Ok(metadata) if !metadata.is_dir() => { + return Err(miette::miette!( + "workspace path '{}' is not a directory", + root.display() + )); } - PathBuf::from(std::ffi::OsString::from_vec(decoded)) - } - - let mountinfo = std::fs::read_to_string("/proc/self/mountinfo").into_diagnostic()?; - Ok(mountinfo - .lines() - .filter_map(|line| line.split_whitespace().nth(4)) - .map(decode_mountinfo_path) - .filter(|mount| mount != root && mount.starts_with(root)) - .collect()) -} - -#[cfg(all(unix, not(target_os = "linux")))] -#[allow(clippy::unnecessary_wraps)] -fn nested_mount_points(_root: &Path) -> Result> { - Ok(HashSet::new()) -} - -#[cfg(unix)] -fn prepare_oci_workspace(root: &Path, uid: Option, gid: Option) -> Result<()> { - match std::fs::symlink_metadata(root) { Ok(_) => {} Err(error) if error.kind() == std::io::ErrorKind::NotFound => { std::fs::create_dir_all(root).into_diagnostic()?; @@ -1371,8 +1262,7 @@ fn prepare_oci_workspace(root: &Path, uid: Option, gid: Option) -> Res Err(error) => return Err(error).into_diagnostic(), } - let mounts = nested_mount_points(root)?; - chown_sandbox_home(root, uid, gid, &mounts) + do_chown(root, uid, gid).into_diagnostic() } /// Prepare filesystem for the sandboxed process. @@ -2303,53 +2193,38 @@ mod tests { #[cfg(unix)] #[test] - #[allow(clippy::similar_names)] - fn chown_sandbox_home_changes_ownership_recursively() { - use std::os::unix::fs::MetadataExt; + fn prepare_oci_workspace_chowns_only_root() { + use std::sync::{Arc, Mutex}; let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); std::fs::create_dir(&root).unwrap(); - std::fs::write(root.join("file.txt"), "hello").unwrap(); - std::fs::create_dir(root.join("subdir")).unwrap(); - std::fs::write(root.join("subdir").join("nested.txt"), "world").unwrap(); + let child = root.join("image-content.txt"); + std::fs::write(&child, "image-owned").unwrap(); - let expected_uid = nix::unistd::geteuid(); - let expected_gid = nix::unistd::getegid(); + let chowned = Arc::new(Mutex::new(Vec::new())); + let observed = Arc::clone(&chowned); + let fake_chown = + move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + observed.lock().unwrap().push(path.to_path_buf()); + Ok(()) + }; - chown_sandbox_home( + prepare_oci_workspace_with( &root, - Some(expected_uid), - Some(expected_gid), - &HashSet::new(), + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &fake_chown, ) - .unwrap(); + .expect("workspace root should be prepared"); - for path in &[ - root.clone(), - root.join("file.txt"), - root.join("subdir"), - root.join("subdir").join("nested.txt"), - ] { - let meta = std::fs::metadata(path).unwrap(); - assert_eq!( - meta.uid(), - expected_uid.as_raw(), - "uid mismatch for {}", - path.display() - ); - assert_eq!( - meta.gid(), - expected_gid.as_raw(), - "gid mismatch for {}", - path.display() - ); - } + assert_eq!(*chowned.lock().unwrap(), vec![root]); + assert!(child.exists(), "image-provided child should be untouched"); } #[cfg(unix)] #[test] - fn chown_sandbox_home_rejects_symlink_root() { + fn prepare_oci_workspace_rejects_symlink_root() { use std::os::unix::fs::symlink; let dir = tempfile::tempdir().unwrap(); @@ -2358,11 +2233,10 @@ mod tests { std::fs::create_dir(&target).unwrap(); symlink(&target, &link).unwrap(); - let err = chown_sandbox_home( + let err = prepare_oci_workspace( &link, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), - &HashSet::new(), ) .unwrap_err(); assert!( @@ -2373,103 +2247,72 @@ mod tests { #[cfg(unix)] #[test] - fn chown_sandbox_home_skips_symlink_children() { - use std::os::unix::fs::symlink; - + fn prepare_oci_workspace_rejects_non_directory_root() { let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); - std::fs::create_dir(&root).unwrap(); - let target = dir.path().join("outside"); - std::fs::write(&target, "secret").unwrap(); - symlink(&target, root.join("link")).unwrap(); + std::fs::write(&root, "not a directory").unwrap(); - chown_sandbox_home( + let error = prepare_oci_workspace( &root, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), - &HashSet::new(), ) - .expect("should skip symlink children without error"); - } - - #[cfg(unix)] - #[test] - fn chown_recursive_skips_nested_mount_but_continues_siblings() { - use std::sync::{Arc, Mutex}; - - let dir = tempfile::tempdir().unwrap(); - let root = dir.path().join("sandbox"); - std::fs::create_dir(&root).unwrap(); - - let mount_dir = root.join("mounted"); - std::fs::create_dir(&mount_dir).unwrap(); - std::fs::write(mount_dir.join("mounted-file.txt"), "data").unwrap(); - - std::fs::write(root.join("writable-sibling.txt"), "data").unwrap(); - - let uid = Some(nix::unistd::geteuid()); - let gid = Some(nix::unistd::getegid()); - - let chowned: Arc>> = Arc::new(Mutex::new(Vec::new())); - let chowned_ref = Arc::clone(&chowned); - - let fake_chown = - move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { - chowned_ref.lock().unwrap().push(path.to_path_buf()); - Ok(()) - }; - let nested_mounts = HashSet::from([mount_dir.clone()]); - - chown_children(&root, uid, gid, &nested_mounts, &fake_chown) - .expect("nested mount should be skipped"); - - let chowned = chowned.lock().unwrap(); - assert!( - !chowned.contains(&mount_dir), - "nested mount root must not be chowned" - ); - assert!( - !chowned.contains(&mount_dir.join("mounted-file.txt")), - "nested mount contents must not be descended into" - ); + .unwrap_err(); assert!( - chowned.contains(&root.join("writable-sibling.txt")), - "writable sibling should still be chowned" + error.to_string().contains("is not a directory"), + "unexpected error: {error}" ); } #[cfg(unix)] #[test] - fn chown_recursive_propagates_chown_errors() { + fn prepare_oci_workspace_propagates_root_chown_error() { let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); std::fs::create_dir(&root).unwrap(); - - let uid = Some(nix::unistd::geteuid()); - let gid = Some(nix::unistd::getegid()); - let fake_chown = |_path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { - Err(nix::errno::Errno::EPERM) + Err(nix::errno::Errno::EROFS) }; - let result = chown_recursive(&root, uid, gid, &HashSet::new(), &fake_chown); - assert!(result.is_err(), "chown errors should propagate"); + let error = prepare_oci_workspace_with( + &root, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &fake_chown, + ) + .unwrap_err(); + + assert!( + error.to_string().contains("Read-only file system"), + "unexpected error: {error}" + ); } #[cfg(unix)] #[test] fn prepare_oci_workspace_creates_missing_root() { + use std::sync::{Arc, Mutex}; + let dir = tempfile::tempdir().unwrap(); let missing = dir.path().join("missing").join("sandbox"); + let chowned = Arc::new(Mutex::new(Vec::new())); + let observed = Arc::clone(&chowned); + let fake_chown = + move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + observed.lock().unwrap().push(path.to_path_buf()); + Ok(()) + }; - prepare_oci_workspace( + prepare_oci_workspace_with( &missing, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), + &fake_chown, ) .expect("missing OCI workspace should be created"); assert!(missing.is_dir()); + assert_eq!(*chowned.lock().unwrap(), vec![missing]); } #[cfg(unix)] diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 695ca29e4e..2592171e76 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -433,12 +433,12 @@ declared name or numeric components for both direct and SSH children. When does not modify `/etc/passwd` or `/etc/group`. When either identity component falls back to OCI `USER`, OpenShell creates the -fixed `/sandbox` workspace if it is absent and transfers ownership of its -existing image contents to the completed UID/GID. The ownership walk skips -symlinks and nested bind or volume mounts. Sandbox startup fails before -readiness when the workspace root cannot be prepared, including when it is -read-only. Images may pre-own `/sandbox` as a build optimization, but this is -not required. +fixed `/sandbox` workspace if it is absent and transfers ownership of only that +directory to the completed UID/GID. Existing image contents and nested bind or +volume mounts retain their ownership. Sandbox startup fails before readiness +when the workspace root cannot be prepared, including when it is read-only. +Images only need to pre-own files beneath `/sandbox` when the agent must modify +those existing files. Sandbox creation fails before readiness if a required `USER` component is missing, malformed, unknown, ambiguous, or resolves to UID/GID 0. An image diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index 23fe63773e..7e3d4d9ea4 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -25,7 +25,7 @@ RUN groupadd -g 1235 appstaff && \ useradd -m -u 1234 -g appstaff app # A custom image may already contain a root-owned OpenShell workspace and -# root-owned files. The supervisor prepares both for the OCI identity. +# root-owned files. The supervisor prepares only the workspace directory. RUN install -d /sandbox && \ printf root-owned > /sandbox/root-owned.txt @@ -49,8 +49,8 @@ CMD ["sleep", "infinity"] const MARKER: &str = "custom-image-e2e-marker"; -/// A named OCI user can write through direct and SSH children even when the -/// image starts with a root-owned `/sandbox` tree. +/// A named OCI user can write through direct and SSH children when the image +/// starts with a root-owned `/sandbox`; existing content retains its ownership. #[tokio::test] async fn sandbox_from_custom_dockerfile() { // Step 1: Write a temporary Dockerfile. @@ -70,7 +70,8 @@ async fn sandbox_from_custom_dockerfile() { "sh", "-c", "set -eu; id -u; id -g; test \"$(cat /sandbox/root-owned.txt)\" = root-owned; \ - test \"$(stat -c %u:%g /sandbox/root-owned.txt)\" = 1234:1235; \ + test \"$(stat -c %u:%g /sandbox)\" = 1234:1235; \ + test \"$(stat -c %u:%g /sandbox/root-owned.txt)\" = 0:0; \ touch /sandbox/direct-oci-user-write; cat /etc/marker.txt; echo Ready; sleep infinity", ], "Ready", diff --git a/examples/bring-your-own-container/README.md b/examples/bring-your-own-container/README.md index 0b3ce014e0..14d3a23613 100644 --- a/examples/bring-your-own-container/README.md +++ b/examples/bring-your-own-container/README.md @@ -63,9 +63,10 @@ key requirements are: such as `app`, a numeric UID with a passwd entry that supplies its primary GID, or a numeric pair such as `1500:1500`. You can instead set both `process.run_as_user` and `process.run_as_group` explicitly in policy. -- **Use `/sandbox` as the workspace.** Pre-owning it in the image is good image - hygiene and avoids startup ownership work, but Docker and Podman create and - prepare it automatically when OCI `USER` supplies any identity component. +- **Use `/sandbox` as the workspace.** Docker and Podman create and prepare the + directory automatically when OCI `USER` supplies any identity component. + Pre-own any files beneath it that the agent must modify; OpenShell does not + recursively rewrite image content ownership. - **Install `iproute2`** for full network namespace isolation. - **Use a standard Linux base image** — distroless and `FROM scratch` images are not supported. From 0c3b055e0371ab506d79d39307b70a36630df854 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 12:06:51 -0700 Subject: [PATCH 08/12] fix(sandbox): harden partial identity drops Signed-off-by: Matthew Grossman --- .../src/identity.rs | 42 ++++++++++- .../src/process.rs | 72 +++++++++++++++++-- 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/crates/openshell-supervisor-process/src/identity.rs b/crates/openshell-supervisor-process/src/identity.rs index 61e5a0ee41..6a9a785542 100644 --- a/crates/openshell-supervisor-process/src/identity.rs +++ b/crates/openshell-supervisor-process/src/identity.rs @@ -107,7 +107,28 @@ pub fn resolve_process_identity( Path::new(PASSWD_PATH), Path::new(GROUP_PATH), ), - DriverIdentity::None => Ok(ResolvedProcessIdentity::default()), + DriverIdentity::None => { + // VM/offline drivers retain the pre-OCI per-field fallback. A + // partial policy must never leave the omitted component at the + // root supervisor identity. + if policy + .process + .run_as_user + .as_deref() + .is_none_or(str::is_empty) + { + policy.process.run_as_user = Some("sandbox".into()); + } + if policy + .process + .run_as_group + .as_deref() + .is_none_or(str::is_empty) + { + policy.process.run_as_group = Some("sandbox".into()); + } + Ok(ResolvedProcessIdentity::default()) + } } } @@ -612,6 +633,25 @@ mod tests { assert!(DriverIdentity::from_values(None, Some("1234".into()), None).is_err()); } + #[test] + fn no_driver_identity_completes_partial_policy_with_sandbox() { + let cases = [ + (None, Some("staff"), "sandbox", "staff"), + (Some("app"), None, "app", "sandbox"), + (None, None, "sandbox", "sandbox"), + (Some("app"), Some("staff"), "app", "staff"), + ]; + + for (user, group, expected_user, expected_group) in cases { + let mut policy = policy(user, group); + let resolved = resolve_process_identity(&mut policy, &DriverIdentity::None).unwrap(); + + assert_eq!(policy.process.run_as_user.as_deref(), Some(expected_user)); + assert_eq!(policy.process.run_as_group.as_deref(), Some(expected_group)); + assert_eq!(resolved, ResolvedProcessIdentity::default()); + } + } + #[test] fn numeric_uid_uses_passwd_primary_gid() { let (_dir, passwd, group) = account_files("app:x:1234:4321::/home/app:/bin/sh\n", ""); diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 9b497faf10..95a17daae9 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1363,6 +1363,18 @@ pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> { drop_privileges_with_identity(policy, ResolvedProcessIdentity::default()) } +#[cfg(unix)] +fn should_clear_supplementary_groups( + current_uid: Uid, + target_uid: Uid, + user_name: Option<&str>, + resolved_identity: ResolvedProcessIdentity, +) -> bool { + target_uid != current_uid + && !(user_name.is_some_and(|name| name.parse::().is_err()) + && resolved_identity.uid().is_none()) +} + #[cfg(unix)] #[allow(clippy::similar_names)] pub fn drop_privileges_with_identity( @@ -1458,10 +1470,15 @@ pub fn drop_privileges_with_identity( }; if target_uid != nix::unistd::geteuid() { - if resolved_identity.uid().is_some() { - // OCI Config.User defines the selected UID/GID pair, not a second - // NSS-dependent supplementary group set. Clear the root - // supervisor's inherited supplementary groups before dropping. + if should_clear_supplementary_groups( + nix::unistd::geteuid(), + target_uid, + user_name, + resolved_identity, + ) { + // Numeric explicit users and OCI-derived users do not have a + // trustworthy NSS supplementary-group source. Clear the root + // supervisor's inherited groups before changing UID/GID. #[cfg(not(any( target_os = "macos", target_os = "ios", @@ -1682,6 +1699,53 @@ mod tests { assert!(validate_sandbox_group_with_identity(&policy, resolved).is_ok()); } + #[test] + #[cfg(unix)] + fn numeric_user_paths_clear_supplementary_groups_before_uid_drop() { + let current_uid = Uid::from_raw(0); + let target_uid = Uid::from_raw(1234); + + let cases = [ + ( + "explicit numeric UID/GID", + ResolvedProcessIdentity::default(), + ), + ( + "explicit numeric UID with OCI-derived GID", + ResolvedProcessIdentity::new(None, Some(1235)), + ), + ]; + + for (description, resolved_identity) in cases { + assert!( + should_clear_supplementary_groups( + current_uid, + target_uid, + Some("1234"), + resolved_identity, + ), + "{description}" + ); + } + } + + #[test] + #[cfg(unix)] + fn supplementary_group_clearing_preserves_explicit_named_user_behavior() { + assert!(!should_clear_supplementary_groups( + Uid::from_raw(0), + Uid::from_raw(1234), + Some("app"), + ResolvedProcessIdentity::default(), + )); + assert!(!should_clear_supplementary_groups( + Uid::from_raw(1234), + Uid::from_raw(1234), + Some("1234"), + ResolvedProcessIdentity::new(None, Some(1235)), + )); + } + #[test] fn full_enforcement_uses_privileged_setup_and_child_sandbox() { assert!(ProcessEnforcementMode::Full.uses_privileged_process_setup()); From b2b3114cca40b1ee48f951ea6e842c106345b4ef Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 12:34:56 -0700 Subject: [PATCH 09/12] test(sandbox): scope OCI image e2e to Docker Signed-off-by: Matthew Grossman --- e2e/rust/Cargo.toml | 2 +- e2e/rust/tests/driver_config_volume.rs | 3 +++ examples/agent-driven-policy-management/policy.template.yaml | 4 ---- examples/governance-interceptor/policy.yaml | 4 ---- examples/governance-interceptor/smoke.sh | 4 ---- examples/local-inference/sandbox-policy.yaml | 4 ---- examples/multi-agent-notepad/policy.template.yaml | 4 ---- 7 files changed, 4 insertions(+), 21 deletions(-) diff --git a/e2e/rust/Cargo.toml b/e2e/rust/Cargo.toml index 4d44ac55d1..b36a32203f 100644 --- a/e2e/rust/Cargo.toml +++ b/e2e/rust/Cargo.toml @@ -35,7 +35,7 @@ e2e-vm = ["e2e", "e2e-host-gateway"] [[test]] name = "custom_image" path = "tests/custom_image.rs" -required-features = ["e2e-local-container-driver"] +required-features = ["e2e-docker"] [[test]] name = "docker_preflight" diff --git a/e2e/rust/tests/driver_config_volume.rs b/e2e/rust/tests/driver_config_volume.rs index bebc0cb09e..b4701f0f81 100644 --- a/e2e/rust/tests/driver_config_volume.rs +++ b/e2e/rust/tests/driver_config_volume.rs @@ -23,6 +23,7 @@ use serde_json::{Map, Value}; const TEST_IMAGE: &str = "ghcr.io/nvidia/openshell-community/sandboxes/base:latest"; const VOLUME_TARGET: &str = "/sandbox/e2e-volume"; const BIND_TARGET: &str = "/sandbox/e2e-bind"; +#[cfg(feature = "e2e-docker")] const OCI_USER_DOCKERFILE: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ @@ -110,6 +111,7 @@ async fn sandbox_mounts_existing_driver_config_volume() { } #[tokio::test] +#[cfg(feature = "e2e-docker")] async fn oci_workspace_preparation_skips_nested_volume_ownership() { let driver = e2e_driver().expect("OPENSHELL_E2E_DRIVER must be set by the e2e wrapper"); assert!( @@ -278,6 +280,7 @@ async fn verify_volume(volume: &VolumeGuard) -> Result<(), String> { Ok(()) } +#[cfg(feature = "e2e-docker")] async fn verify_volume_ownership(volume: &VolumeGuard) -> Result<(), String> { let output = run_volume_container( volume, diff --git a/examples/agent-driven-policy-management/policy.template.yaml b/examples/agent-driven-policy-management/policy.template.yaml index 0498ecfcc8..01f2d6e3b0 100644 --- a/examples/agent-driven-policy-management/policy.template.yaml +++ b/examples/agent-driven-policy-management/policy.template.yaml @@ -29,10 +29,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - network_policies: codex: name: codex diff --git a/examples/governance-interceptor/policy.yaml b/examples/governance-interceptor/policy.yaml index 021e635db2..1ffe34a9f1 100644 --- a/examples/governance-interceptor/policy.yaml +++ b/examples/governance-interceptor/policy.yaml @@ -11,10 +11,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - network_policies: my_api: name: my-api diff --git a/examples/governance-interceptor/smoke.sh b/examples/governance-interceptor/smoke.sh index 34f93fa2c6..88610cf1ee 100755 --- a/examples/governance-interceptor/smoke.sh +++ b/examples/governance-interceptor/smoke.sh @@ -512,10 +512,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - network_policies: example_api: name: example-api diff --git a/examples/local-inference/sandbox-policy.yaml b/examples/local-inference/sandbox-policy.yaml index 79fde8ea29..a0d7ba1f41 100644 --- a/examples/local-inference/sandbox-policy.yaml +++ b/examples/local-inference/sandbox-policy.yaml @@ -21,10 +21,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - # Allow PyPI access so pip can install dependencies inside the sandbox. network_policies: pypi: diff --git a/examples/multi-agent-notepad/policy.template.yaml b/examples/multi-agent-notepad/policy.template.yaml index bb12863676..30be728754 100644 --- a/examples/multi-agent-notepad/policy.template.yaml +++ b/examples/multi-agent-notepad/policy.template.yaml @@ -11,10 +11,6 @@ filesystem_policy: landlock: compatibility: best_effort -process: - run_as_user: sandbox - run_as_group: sandbox - network_policies: codex: name: codex From c185166ce12fbd44695c27c10d785287443d0e5e Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 14:13:21 -0700 Subject: [PATCH 10/12] fix(sandbox): narrow OCI identity fallback scope Signed-off-by: Matthew Grossman --- architecture/compute-runtimes.md | 7 - crates/openshell-driver-docker/README.md | 6 - crates/openshell-driver-podman/README.md | 6 - .../openshell-driver-podman/src/container.rs | 9 +- crates/openshell-driver-podman/src/driver.rs | 1 + crates/openshell-policy/src/lib.rs | 39 +++ crates/openshell-server/src/compute/mod.rs | 9 +- crates/openshell-server/src/grpc/mod.rs | 13 +- crates/openshell-server/src/grpc/policy.rs | 26 +- crates/openshell-server/src/grpc/sandbox.rs | 59 +++- .../openshell-server/src/grpc/validation.rs | 56 +++ .../src/process.rs | 326 +++++++++++------- docs/reference/policy-schema.mdx | 5 +- docs/reference/sandbox-compute-drivers.mdx | 12 +- e2e/rust/tests/custom_image.rs | 37 +- e2e/rust/tests/driver_config_volume.rs | 89 ----- examples/bring-your-own-container/README.md | 7 +- 17 files changed, 406 insertions(+), 301 deletions(-) diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 85124644e6..1629c50de4 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -144,13 +144,6 @@ and uses the same privilege-drop path for direct and SSH children. When a declaration omits the group, the supervisor fills it with the user's numeric primary GID. It does not rewrite the account files. -When either component falls back to the image identity, Docker and Podman -prepare the fixed `/sandbox` workspace for the completed UID/GID before direct -or SSH children start. They create it when absent and transfer ownership of -only the workspace directory. Image-provided contents and nested mounts retain -their ownership. Kubernetes/OpenShift keep their PVC and `fsGroup` behavior, -and VM keeps its guest initialization path. - Sandbox creation fails before the workload becomes ready when a required image identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0. The supervisor itself remains root so it can establish isolation before diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index bec7f500f3..b2e74231ba 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -27,12 +27,6 @@ identity. Named OCI components remain names after validation; a missing group is filled with the user's numeric primary GID. Explicit `process.run_as_user` and `process.run_as_group` values take precedence independently. -When either policy field falls back to OCI `USER`, the supervisor creates the -fixed `/sandbox` workspace if necessary and transfers ownership of only that -directory to the completed identity before direct or SSH children start. -Image-provided contents and nested bind or volume mounts retain their ownership. -A read-only workspace root fails sandbox startup. - Docker containers join an OpenShell-managed bridge network. The driver injects `host.openshell.internal` and `host.docker.internal` so supervisors have stable names for reaching the gateway host. On Docker Desktop, Colima, Rancher diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index f075a61b95..965a295d19 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -17,12 +17,6 @@ validation; a missing group is filled with the user's numeric primary GID. Expli `process.run_as_user` and `process.run_as_group` values take precedence independently. -When either policy field falls back to OCI `USER`, the supervisor creates the -fixed `/sandbox` workspace if necessary and transfers ownership of only that -directory to the completed identity before direct or SSH children start. -Image-provided contents and nested bind or volume mounts retain their ownership. -A read-only workspace root fails sandbox startup. - For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md). ## Architecture diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 2eb07f6e0a..90ef0fec21 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -903,6 +903,7 @@ pub fn build_container_spec_with_token_and_gpu_devices( token_secret_name, gpu_device_ids, image, + image, "", ) } @@ -912,13 +913,14 @@ pub fn build_container_spec_for_image( config: &PodmanComputeConfig, token_secret_name: Option<&str>, gpu_device_ids: Option<&[String]>, + requested_image: &str, image_id: &str, oci_user: &str, ) -> Result { let name = container_name(&sandbox.workspace, &sandbox.name, &sandbox.id); let vol = volume_name(&sandbox.id); - let env = build_env(sandbox, config, image_id, oci_user); + let env = build_env(sandbox, config, requested_image, oci_user); let labels = build_labels(sandbox); let resource_limits = build_resource_limits(sandbox, config); let user_mounts = podman_user_mounts(sandbox, config.enable_bind_mounts) @@ -1374,12 +1376,17 @@ mod tests { &test_config(), None, None, + "registry.example/app:latest", "sha256:immutable", "app:staff", ) .unwrap(); assert_eq!(container["image"].as_str(), Some("sha256:immutable")); + assert_eq!( + container["env"]["OPENSHELL_CONTAINER_IMAGE"].as_str(), + Some("registry.example/app:latest") + ); assert_eq!(container["user"].as_str(), Some("0:0")); assert_eq!(container["image_pull_policy"].as_str(), Some("never")); assert_eq!( diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 18723ec40c..aa3df9cbd7 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -649,6 +649,7 @@ impl PodmanComputeDriver { &self.config, token_secret_name.as_deref(), gpu_devices.as_deref(), + image, &inspected_image.id, image_user, ) { diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index 0a4cb96564..7937a3757e 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -1081,6 +1081,21 @@ pub fn restrictive_default_policy() -> SandboxPolicy { } } +/// Fill omitted process identity fields with the legacy `sandbox` defaults. +/// +/// Docker and Podman preserve omission so their supervisors can fall back to +/// OCI `Config.User`. Other drivers call this before validation and +/// persistence to retain the existing public policy representation. +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(); + } +} + // --------------------------------------------------------------------------- // Policy safety validation // --------------------------------------------------------------------------- @@ -1688,6 +1703,30 @@ network_policies: assert!(reparsed.process.unwrap().run_as_group.is_empty()); } + #[test] + fn ensure_sandbox_process_identity_fills_each_omitted_field() { + let cases = [ + (None, None, "sandbox", "sandbox"), + (Some("1234"), None, "1234", "sandbox"), + (None, Some("1235"), "sandbox", "1235"), + (Some("1234"), Some("1235"), "1234", "1235"), + ]; + + for (user, group, expected_user, expected_group) in cases { + let mut policy = restrictive_default_policy(); + policy.process = Some(ProcessPolicy { + run_as_user: user.unwrap_or_default().to_string(), + run_as_group: group.unwrap_or_default().to_string(), + }); + + ensure_sandbox_process_identity(&mut policy); + + let process = policy.process.expect("normalized process policy"); + assert_eq!(process.run_as_user, expected_user); + assert_eq!(process.run_as_group, expected_group); + } + } + #[test] fn parse_policy_with_network_rules() { let yaml = r" diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index cf6b17c7e9..60e97126c7 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -2743,11 +2743,16 @@ impl ComputeDriver for NoopTestDriver { #[cfg(test)] pub async fn new_test_runtime(store: Arc) -> ComputeRuntime { + new_test_runtime_for_driver(store, "test").await +} + +#[cfg(test)] +pub async fn new_test_runtime_for_driver(store: Arc, driver_name: &str) -> ComputeRuntime { ComputeRuntime { driver: Arc::new(NoopTestDriver), driver_info: ComputeDriverInfoSnapshot { - name: "test".to_string(), - driver_name: "test".to_string(), + name: driver_name.to_string(), + driver_name: driver_name.to_string(), driver_version: "test".to_string(), }, shutdown_cleanup: None, diff --git a/crates/openshell-server/src/grpc/mod.rs b/crates/openshell-server/src/grpc/mod.rs index 8618fd8013..eed96c3598 100644 --- a/crates/openshell-server/src/grpc/mod.rs +++ b/crates/openshell-server/src/grpc/mod.rs @@ -788,7 +788,7 @@ pub mod test_support { use std::sync::Arc; use crate::ServerState; - use crate::compute::new_test_runtime; + use crate::compute::{new_test_runtime, new_test_runtime_for_driver}; use crate::persistence::Store; use crate::sandbox_index::SandboxIndex; use crate::sandbox_watch::SandboxWatchBus; @@ -798,13 +798,22 @@ pub mod test_support { /// Build an in-memory `ServerState` for unit tests. pub async fn test_server_state() -> Arc { + test_server_state_with_driver("test").await + } + + /// Build an in-memory `ServerState` with a selected built-in driver name. + pub async fn test_server_state_with_driver(driver_name: &str) -> Arc { let store = Arc::new( Store::connect("sqlite::memory:?cache=shared") .await .unwrap(), ); crate::ensure_default_workspace(&store).await.unwrap(); - let compute = new_test_runtime(store.clone()).await; + let compute = if driver_name == "test" { + new_test_runtime(store.clone()).await + } else { + new_test_runtime_for_driver(store.clone(), driver_name).await + }; Arc::new(ServerState::new( Config::new(None).with_database_url("sqlite::memory:?cache=shared"), store, diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index d0b3d63808..e99ea1a30d 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -76,8 +76,9 @@ use tonic::{Request, Response, Status}; use tracing::{debug, info, warn}; use super::validation::{ - level_matches, source_matches, validate_annotations, validate_no_reserved_provider_policy_keys, - validate_policy_safety, validate_static_fields_unchanged, + level_matches, normalize_process_identity_for_driver, source_matches, validate_annotations, + validate_no_reserved_provider_policy_keys, validate_policy_safety, + validate_static_fields_unchanged, }; use super::{MAX_PAGE_SIZE, StoredSettingValue, StoredSettings, clamp_limit}; use crate::persistence::current_time_ms; @@ -1735,9 +1736,10 @@ async fn handle_update_config_inner( "delete_setting cannot be combined with policy payload", )); } - let new_policy = req.policy.ok_or_else(|| { + let mut new_policy = req.policy.ok_or_else(|| { Status::invalid_argument("policy is required for global policy update") })?; + normalize_process_identity_for_driver(&mut new_policy, state.compute.driver_kind()); validate_no_reserved_provider_policy_keys(&new_policy)?; validate_policy_safety(&new_policy)?; crate::middleware::validate_policy(state.middleware_registry.as_ref(), &new_policy) @@ -2013,11 +2015,15 @@ async fn handle_update_config_inner( provenance: &req.annotations, annotations: &req.annotations, }; + let mut baseline_policy = spec.policy.clone(); + if let Some(policy) = baseline_policy.as_mut() { + normalize_process_identity_for_driver(policy, state.compute.driver_kind()); + } let (version, hash, updated_sandbox) = apply_merge_operations_with_retry( state.store.as_ref(), &sandbox_id, &workspace, - spec.policy.as_ref(), + baseline_policy.as_ref(), &merge_ops, Some(&atomic_context), ) @@ -2083,6 +2089,7 @@ async fn handle_update_config_inner( let mut new_policy = req .policy .ok_or_else(|| Status::invalid_argument("policy is required"))?; + normalize_process_identity_for_driver(&mut new_policy, state.compute.driver_kind()); let global_settings = load_global_settings(state.store.as_ref()).await?; if global_settings.settings.contains_key(POLICY_SETTING_KEY) { @@ -2108,7 +2115,12 @@ async fn handle_update_config_inner( } let backfill_policy = if let Some(baseline_policy) = spec.policy.as_ref() { - validate_static_fields_unchanged(baseline_policy, &new_policy)?; + let mut comparable_baseline = baseline_policy.clone(); + normalize_process_identity_for_driver( + &mut comparable_baseline, + state.compute.driver_kind(), + ); + validate_static_fields_unchanged(&comparable_baseline, &new_policy)?; None } else { Some(new_policy.clone()) @@ -12118,9 +12130,9 @@ mod tests { .as_ref() .and_then(|spec| spec.policy.as_ref()) .and_then(|policy| policy.process.as_ref()) - .expect("partial process identity should be persisted"); + .expect("legacy process identity should be persisted"); assert_eq!(process.run_as_user, "1234"); - assert!(process.run_as_group.is_empty()); + assert_eq!(process.run_as_group, "sandbox"); assert_eq!( updated_sandbox.metadata.as_ref().unwrap().resource_version, current_version + 1, diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index e6e4fbb069..0d6a4e277f 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -47,8 +47,9 @@ use super::provider::{ get_provider_record, is_valid_env_key, validate_provider_environment_keys_unique, }; use super::validation::{ - level_matches, source_matches, validate_exec_request_fields, - validate_no_reserved_provider_policy_keys, validate_policy_safety, validate_sandbox_spec, + level_matches, normalize_process_identity_for_driver, source_matches, + validate_exec_request_fields, validate_no_reserved_provider_policy_keys, + validate_policy_safety, validate_sandbox_spec, }; use super::{MAX_PAGE_SIZE, MAX_PROVIDERS, MAX_ROUTABLE_NAME_LEN, clamp_limit}; use crate::persistence::current_time_ms; @@ -172,9 +173,10 @@ async fn handle_create_sandbox_inner( template.image = state.compute.default_image().to_string(); } - // Preserve omitted process identity fields so the compute runtime can - // apply its driver-specific fallback. + // Docker and Podman preserve omitted identity fields for OCI USER + // fallback. Other drivers retain the legacy persisted sandbox defaults. if let Some(ref mut policy) = spec.policy { + normalize_process_identity_for_driver(policy, state.compute.driver_kind()); validate_no_reserved_provider_policy_keys(policy)?; validate_policy_safety(policy)?; crate::middleware::validate_policy(state.middleware_registry.as_ref(), policy).await?; @@ -2105,7 +2107,7 @@ async fn run_exec_with_russh( #[cfg(test)] mod tests { use super::*; - use crate::grpc::test_support::test_server_state; + use crate::grpc::test_support::{test_server_state, test_server_state_with_driver}; use openshell_core::proto::datamodel::v1::ObjectMeta; // ---- shell_escape ---- @@ -2970,7 +2972,8 @@ mod tests { #[tokio::test] async fn create_and_get_preserve_partial_process_identity() { - let state = test_server_state().await; + let state = + test_server_state_with_driver(openshell_core::ComputeDriverKind::Docker.as_str()).await; let policy = openshell_core::proto::SandboxPolicy { version: 1, process: Some(openshell_core::proto::ProcessPolicy { @@ -3031,6 +3034,50 @@ mod tests { assert_eq!(fetched_process.run_as_group, "1234"); } + #[tokio::test] + async fn create_and_get_restore_legacy_identity_defaults_for_non_local_driver() { + let state = + test_server_state_with_driver(openshell_core::ComputeDriverKind::Kubernetes.as_str()) + .await; + let policy = openshell_core::proto::SandboxPolicy { + version: 1, + process: Some(openshell_core::proto::ProcessPolicy { + run_as_user: String::new(), + run_as_group: "1234".to_string(), + }), + ..Default::default() + }; + + let response = handle_create_sandbox( + &state, + Request::new(CreateSandboxRequest { + name: "kube-partial-id".to_string(), + spec: Some(openshell_core::proto::SandboxSpec { + policy: Some(policy), + ..Default::default() + }), + labels: HashMap::new(), + annotations: HashMap::new(), + workspace: String::new(), + }), + ) + .await + .expect("Kubernetes identity defaults should be accepted") + .into_inner(); + + let process = response + .sandbox + .unwrap() + .spec + .unwrap() + .policy + .unwrap() + .process + .unwrap(); + assert_eq!(process.run_as_user, "sandbox"); + assert_eq!(process.run_as_group, "1234"); + } + #[tokio::test] async fn create_sandbox_still_rejects_long_label_values() { let state = test_server_state().await; diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 2f0ad8d139..c5d10c2b3a 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -8,6 +8,7 @@ #![allow(clippy::result_large_err)] // Validation returns Result<_, Status> +use openshell_core::ComputeDriverKind; use openshell_core::proto::{ ExecSandboxRequest, Provider, SandboxPolicy as ProtoSandboxPolicy, SandboxTemplate, }; @@ -25,6 +26,23 @@ use super::{ // Exec request validation // --------------------------------------------------------------------------- +/// Preserve process-identity omission only for the local OCI-aware drivers. +/// +/// Kubernetes, VM, and unknown/remote drivers retain the legacy persisted +/// `sandbox:sandbox` defaults so existing policy hashes and live-update +/// workflows do not change. +pub(super) fn normalize_process_identity_for_driver( + policy: &mut ProtoSandboxPolicy, + driver_kind: Option, +) { + if !matches!( + driver_kind, + Some(ComputeDriverKind::Docker | ComputeDriverKind::Podman) + ) { + openshell_policy::ensure_sandbox_process_identity(policy); + } +} + /// Maximum number of arguments in the command array. pub(super) const MAX_EXEC_COMMAND_ARGS: usize = 1024; /// Maximum length of a single command argument or environment value (bytes). @@ -1636,6 +1654,44 @@ mod tests { // ---- Policy safety ---- + #[test] + fn process_identity_omission_is_driver_scoped() { + use openshell_core::proto::ProcessPolicy; + + for driver in [ComputeDriverKind::Docker, ComputeDriverKind::Podman] { + let mut policy = ProtoSandboxPolicy { + process: Some(ProcessPolicy { + run_as_user: "1234".into(), + run_as_group: String::new(), + }), + ..Default::default() + }; + normalize_process_identity_for_driver(&mut policy, Some(driver)); + assert!( + policy.process.unwrap().run_as_group.is_empty(), + "{driver:?} must preserve omission" + ); + } + + for driver in [ + Some(ComputeDriverKind::Kubernetes), + Some(ComputeDriverKind::Vm), + None, + ] { + let mut policy = ProtoSandboxPolicy { + process: Some(ProcessPolicy { + run_as_user: "1234".into(), + run_as_group: String::new(), + }), + ..Default::default() + }; + normalize_process_identity_for_driver(&mut policy, driver); + let process = policy.process.unwrap(); + assert_eq!(process.run_as_user, "1234"); + assert_eq!(process.run_as_group, "sandbox"); + } + } + #[test] fn validate_policy_safety_rejects_root_user() { use openshell_core::proto::{FilesystemPolicy, ProcessPolicy}; diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 95a17daae9..93ab4787b3 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -911,12 +911,11 @@ impl Drop for ProcessHandle { pub fn validate_sandbox_user(policy: &SandboxPolicy) -> Result<()> { let identity = policy.process.run_as_user.as_deref().unwrap_or("sandbox"); - // Explicit policy identities were range-checked before the runtime policy - // reached the supervisor. OCI-derived numeric identities may use system - // account IDs, so this completed-runtime check only prohibits root. if let Ok(uid) = identity.parse::() { - if uid == 0 { - return Err(miette::miette!("process user must not select UID 0")); + if !(MIN_SANDBOX_UID..=MAX_SANDBOX_UID).contains(&uid) { + return Err(miette::miette!( + "process user UID must be in range [{MIN_SANDBOX_UID}, {MAX_SANDBOX_UID}]" + )); } openshell_ocsf::ocsf_emit!( openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx()) @@ -985,8 +984,10 @@ pub fn validate_sandbox_group(policy: &SandboxPolicy) -> Result<()> { let identity = policy.process.run_as_group.as_deref().unwrap_or("sandbox"); if let Ok(gid) = identity.parse::() { - if gid == 0 { - return Err(miette::miette!("process group must not select GID 0")); + if !(MIN_SANDBOX_UID..=MAX_SANDBOX_UID).contains(&gid) { + return Err(miette::miette!( + "process group GID must be in range [{MIN_SANDBOX_UID}, {MAX_SANDBOX_UID}]" + )); } openshell_ocsf::ocsf_emit!( openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx()) @@ -1052,9 +1053,10 @@ pub fn validate_sandbox_user_with_identity( let Some(uid) = resolved_identity.uid() else { return validate_sandbox_user(policy); }; - let mut effective_policy = policy.clone(); - effective_policy.process.run_as_user = Some(uid.to_string()); - validate_sandbox_user(&effective_policy) + if uid == 0 { + return Err(miette::miette!("process user must not select UID 0")); + } + Ok(()) } #[cfg(unix)] @@ -1065,9 +1067,10 @@ pub fn validate_sandbox_group_with_identity( let Some(gid) = resolved_identity.gid() else { return validate_sandbox_group(policy); }; - let mut effective_policy = policy.clone(); - effective_policy.process.run_as_group = Some(gid.to_string()); - validate_sandbox_group(&effective_policy) + if gid == 0 { + return Err(miette::miette!("process group must not select GID 0")); + } + Ok(()) } pub use openshell_policy::{MAX_SANDBOX_UID, MIN_SANDBOX_UID}; @@ -1225,44 +1228,81 @@ fn rewrite_group_at(path: &Path, gid: &str) -> Result<()> { Ok(()) } +/// Recursively chown a directory tree to the given UID/GID. +/// +/// This retains the Kubernetes/OpenShift workspace reconciliation from before +/// OCI image identity fallback. Symlinks are skipped, and read-only nested +/// mounts are not traversed. #[cfg(unix)] -fn prepare_oci_workspace(root: &Path, uid: Option, gid: Option) -> Result<()> { - prepare_oci_workspace_with(root, uid, gid, &nix::unistd::chown) +fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result<()> { + let meta = std::fs::symlink_metadata(root).into_diagnostic()?; + if meta.file_type().is_symlink() { + return Err(miette::miette!( + "path '{}' is a symlink — refusing to chown (potential privilege escalation)", + root.display() + )); + } + + nix::unistd::chown(root, uid, gid).into_diagnostic()?; + + if meta.is_dir() { + chown_children(root, uid, gid, &nix::unistd::chown)?; + } + + Ok(()) } -/// Prepare only the fixed `OpenShell` workspace directory itself. -/// -/// Image-provided children retain their declared ownership. This avoids -/// crossing symlinks or user-provided nested mounts and keeps OCI working -/// directory/content semantics out of the identity-focused change. #[cfg(unix)] -fn prepare_oci_workspace_with( - root: &Path, +fn chown_children( + dir: &Path, uid: Option, gid: Option, do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, ) -> Result<()> { - match std::fs::symlink_metadata(root) { - Ok(metadata) if metadata.file_type().is_symlink() => { - return Err(miette::miette!( - "workspace path '{}' is a symlink — refusing to chown", - root.display() - )); + match std::fs::read_dir(dir) { + Ok(entries) => { + for entry in entries { + let entry = entry.into_diagnostic()?; + chown_recursive(&entry.path(), uid, gid, do_chown)?; + } } - Ok(metadata) if !metadata.is_dir() => { - return Err(miette::miette!( - "workspace path '{}' is not a directory", - root.display() - )); + Err(error) => { + debug!( + path = %dir.display(), + %error, + "Cannot list directory during sandbox home chown" + ); } - Ok(_) => {} - Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - std::fs::create_dir_all(root).into_diagnostic()?; + } + Ok(()) +} + +#[cfg(unix)] +fn chown_recursive( + path: &Path, + uid: Option, + gid: Option, + do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, +) -> Result<()> { + let meta = std::fs::symlink_metadata(path).into_diagnostic()?; + if meta.file_type().is_symlink() { + debug!(path = %path.display(), "Skipping symlink during sandbox home chown"); + return Ok(()); + } + + if let Err(error) = do_chown(path, uid, gid) { + if error == nix::errno::Errno::EROFS { + debug!(path = %path.display(), "Skipping read-only path during sandbox home chown"); + return Ok(()); } - Err(error) => return Err(error).into_diagnostic(), + return Err(error).into_diagnostic(); } - do_chown(root, uid, gid).into_diagnostic() + if meta.is_dir() { + chown_children(path, uid, gid, do_chown)?; + } + + Ok(()) } /// Prepare filesystem for the sandboxed process. @@ -1337,14 +1377,15 @@ pub fn prepare_filesystem_with_identity( } } - // Docker and Podman must make OpenShell's fixed workspace usable when any - // identity component came from OCI USER. Explicit policy identity and - // platform-resolved Kubernetes/OpenShift identity retain their existing - // filesystem paths. - if resolved_identity.uses_oci_user_fallback() { + // Retain the existing Kubernetes/OpenShift behavior for driver-injected + // numeric identities. Docker and Podman clear this variable and do not + // receive identity-specific workspace preparation. + if std::env::var(openshell_core::sandbox_env::SANDBOX_UID).is_ok_and(|uid| !uid.is_empty()) { let sandbox_home = Path::new("/sandbox"); - info!(?uid, ?gid, "Preparing /sandbox for OCI image identity"); - prepare_oci_workspace(sandbox_home, uid, gid)?; + if sandbox_home.exists() { + info!(?uid, ?gid, "Chowning /sandbox for driver-injected UID/GID"); + chown_sandbox_home(sandbox_home, uid, gid)?; + } } Ok(()) @@ -1370,7 +1411,8 @@ fn should_clear_supplementary_groups( user_name: Option<&str>, resolved_identity: ResolvedProcessIdentity, ) -> bool { - target_uid != current_uid + resolved_identity.uses_oci_user_fallback() + && target_uid != current_uid && !(user_name.is_some_and(|name| name.parse::().is_err()) && resolved_identity.uid().is_none()) } @@ -1476,9 +1518,10 @@ pub fn drop_privileges_with_identity( user_name, resolved_identity, ) { - // Numeric explicit users and OCI-derived users do not have a - // trustworthy NSS supplementary-group source. Clear the root - // supervisor's inherited groups before changing UID/GID. + // OCI-derived users do not have a trustworthy NSS + // supplementary-group source. Clear the root supervisor's + // inherited groups before changing UID/GID. Platform-resolved and + // explicit numeric identities retain their pre-OCI behavior. #[cfg(not(any( target_os = "macos", target_os = "ios", @@ -1647,14 +1690,27 @@ mod tests { #[test] #[cfg(unix)] - fn completed_runtime_identity_accepts_non_root_system_ids() { + fn explicit_identity_rejects_non_root_system_ids() { let policy = policy_with_process(ProcessPolicy { run_as_user: Some("101".into()), run_as_group: Some("102".into()), }); - assert!(validate_sandbox_user(&policy).is_ok()); - assert!(validate_sandbox_group(&policy).is_ok()); + assert!(validate_sandbox_user(&policy).is_err()); + assert!(validate_sandbox_group(&policy).is_err()); + } + + #[test] + #[cfg(unix)] + fn resolved_oci_identity_accepts_non_root_system_ids() { + let policy = policy_with_process(ProcessPolicy { + run_as_user: Some("app".into()), + run_as_group: Some("staff".into()), + }); + let resolved = ResolvedProcessIdentity::new(Some(101), Some(102)); + + assert!(validate_sandbox_user_with_identity(&policy, resolved).is_ok()); + assert!(validate_sandbox_group_with_identity(&policy, resolved).is_ok()); } #[test] @@ -1701,32 +1757,22 @@ mod tests { #[test] #[cfg(unix)] - fn numeric_user_paths_clear_supplementary_groups_before_uid_drop() { + fn only_oci_numeric_user_paths_clear_supplementary_groups_before_uid_drop() { let current_uid = Uid::from_raw(0); let target_uid = Uid::from_raw(1234); - let cases = [ - ( - "explicit numeric UID/GID", - ResolvedProcessIdentity::default(), - ), - ( - "explicit numeric UID with OCI-derived GID", - ResolvedProcessIdentity::new(None, Some(1235)), - ), - ]; - - for (description, resolved_identity) in cases { - assert!( - should_clear_supplementary_groups( - current_uid, - target_uid, - Some("1234"), - resolved_identity, - ), - "{description}" - ); - } + assert!(!should_clear_supplementary_groups( + current_uid, + target_uid, + Some("1234"), + ResolvedProcessIdentity::default(), + )); + assert!(should_clear_supplementary_groups( + current_uid, + target_uid, + Some("1234"), + ResolvedProcessIdentity::new(None, Some(1235)), + )); } #[test] @@ -2257,38 +2303,36 @@ mod tests { #[cfg(unix)] #[test] - fn prepare_oci_workspace_chowns_only_root() { - use std::sync::{Arc, Mutex}; + #[allow(clippy::similar_names)] + fn chown_sandbox_home_changes_ownership_recursively() { + use std::os::unix::fs::MetadataExt; let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); std::fs::create_dir(&root).unwrap(); - let child = root.join("image-content.txt"); - std::fs::write(&child, "image-owned").unwrap(); - - let chowned = Arc::new(Mutex::new(Vec::new())); - let observed = Arc::clone(&chowned); - let fake_chown = - move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { - observed.lock().unwrap().push(path.to_path_buf()); - Ok(()) - }; - - prepare_oci_workspace_with( - &root, - Some(nix::unistd::geteuid()), - Some(nix::unistd::getegid()), - &fake_chown, - ) - .expect("workspace root should be prepared"); - - assert_eq!(*chowned.lock().unwrap(), vec![root]); - assert!(child.exists(), "image-provided child should be untouched"); + std::fs::write(root.join("file.txt"), "hello").unwrap(); + std::fs::create_dir(root.join("subdir")).unwrap(); + std::fs::write(root.join("subdir").join("nested.txt"), "world").unwrap(); + + let expected_uid = nix::unistd::geteuid(); + let expected_gid = nix::unistd::getegid(); + chown_sandbox_home(&root, Some(expected_uid), Some(expected_gid)).unwrap(); + + for path in &[ + root.clone(), + root.join("file.txt"), + root.join("subdir"), + root.join("subdir").join("nested.txt"), + ] { + let meta = std::fs::metadata(path).unwrap(); + assert_eq!(meta.uid(), expected_uid.as_raw()); + assert_eq!(meta.gid(), expected_gid.as_raw()); + } } #[cfg(unix)] #[test] - fn prepare_oci_workspace_rejects_symlink_root() { + fn chown_sandbox_home_rejects_symlink_root() { use std::os::unix::fs::symlink; let dir = tempfile::tempdir().unwrap(); @@ -2297,7 +2341,7 @@ mod tests { std::fs::create_dir(&target).unwrap(); symlink(&target, &link).unwrap(); - let err = prepare_oci_workspace( + let err = chown_sandbox_home( &link, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), @@ -2311,72 +2355,86 @@ mod tests { #[cfg(unix)] #[test] - fn prepare_oci_workspace_rejects_non_directory_root() { + fn chown_sandbox_home_skips_symlink_children() { + use std::os::unix::fs::symlink; + let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); - std::fs::write(&root, "not a directory").unwrap(); + std::fs::create_dir(&root).unwrap(); + let target = dir.path().join("outside"); + std::fs::write(&target, "secret").unwrap(); + symlink(&target, root.join("link")).unwrap(); - let error = prepare_oci_workspace( + chown_sandbox_home( &root, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), ) - .unwrap_err(); - assert!( - error.to_string().contains("is not a directory"), - "unexpected error: {error}" - ); + .expect("symlink children should be skipped"); } #[cfg(unix)] #[test] - fn prepare_oci_workspace_propagates_root_chown_error() { + fn chown_recursive_skips_erofs_subtree_but_continues_siblings() { + use std::sync::{Arc, Mutex}; + let dir = tempfile::tempdir().unwrap(); let root = dir.path().join("sandbox"); std::fs::create_dir(&root).unwrap(); - let fake_chown = |_path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { - Err(nix::errno::Errno::EROFS) - }; - let error = prepare_oci_workspace_with( + let readonly_dir = root.join("ro-mount"); + std::fs::create_dir(&readonly_dir).unwrap(); + std::fs::write(readonly_dir.join("child-under-ro.txt"), "data").unwrap(); + std::fs::write(root.join("writable-sibling.txt"), "data").unwrap(); + + let chowned = Arc::new(Mutex::new(Vec::new())); + let observed = Arc::clone(&chowned); + let readonly_dir_for_chown = readonly_dir.clone(); + let fake_chown = + move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + if path == readonly_dir_for_chown { + return Err(nix::errno::Errno::EROFS); + } + observed.lock().unwrap().push(path.to_path_buf()); + Ok(()) + }; + + chown_children( &root, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), &fake_chown, ) - .unwrap_err(); + .expect("read-only subtree should be skipped"); + let chowned = chowned.lock().unwrap(); assert!( - error.to_string().contains("Read-only file system"), - "unexpected error: {error}" + !chowned.contains(&readonly_dir.join("child-under-ro.txt")), + "children under EROFS directory must not be traversed" + ); + assert!( + chowned.contains(&root.join("writable-sibling.txt")), + "writable sibling should still be chowned" ); } #[cfg(unix)] #[test] - fn prepare_oci_workspace_creates_missing_root() { - use std::sync::{Arc, Mutex}; - + fn chown_recursive_propagates_non_erofs_errors() { let dir = tempfile::tempdir().unwrap(); - let missing = dir.path().join("missing").join("sandbox"); - let chowned = Arc::new(Mutex::new(Vec::new())); - let observed = Arc::clone(&chowned); - let fake_chown = - move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { - observed.lock().unwrap().push(path.to_path_buf()); - Ok(()) - }; + let root = dir.path().join("sandbox"); + std::fs::create_dir(&root).unwrap(); + let fake_chown = |_path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + Err(nix::errno::Errno::EPERM) + }; - prepare_oci_workspace_with( - &missing, + let result = chown_recursive( + &root, Some(nix::unistd::geteuid()), Some(nix::unistd::getegid()), &fake_chown, - ) - .expect("missing OCI workspace should be created"); - - assert!(missing.is_dir()); - assert_eq!(*chowned.lock().unwrap(), vec![missing]); + ); + assert!(result.is_err(), "non-EROFS errors should propagate"); } #[cfg(unix)] diff --git a/docs/reference/policy-schema.mdx b/docs/reference/policy-schema.mdx index b1da42ec3f..9c4e877eaf 100644 --- a/docs/reference/policy-schema.mdx +++ b/docs/reference/policy-schema.mdx @@ -122,7 +122,10 @@ Sets the OS-level identity for the agent process inside the sandbox. | `run_as_user` | string | No | Overrides the user name or UID selected by the compute driver. Docker and Podman fall back to the image's OCI `USER`. | | `run_as_group` | string | No | Overrides the group name or GID selected by the compute driver. Docker and Podman fall back to the image's OCI `USER`. | -**Validation constraint:** Neither `run_as_user` nor `run_as_group` can be set to `root` or `0`. Policies that request root process identity are rejected at creation or update time. +**Validation constraint:** An explicit policy value must be `sandbox` or a +numeric UID/GID in the allowed sandbox range. Docker and Podman may select +other named identities or non-root system IDs only through OCI `USER` +fallback. Root identities are always rejected. Omission is preserved independently for each field. For example, setting only `run_as_user` keeps that explicit user while allowing the active driver to diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 2592171e76..d260cb5257 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -432,14 +432,6 @@ declared name or numeric components for both direct and SSH children. When `USER` omits the group, the supervisor uses the user's numeric primary GID. It does not modify `/etc/passwd` or `/etc/group`. -When either identity component falls back to OCI `USER`, OpenShell creates the -fixed `/sandbox` workspace if it is absent and transfers ownership of only that -directory to the completed UID/GID. Existing image contents and nested bind or -volume mounts retain their ownership. Sandbox startup fails before readiness -when the workspace root cannot be prepared, including when it is read-only. -Images only need to pre-own files beneath `/sandbox` when the agent must modify -those existing files. - Sandbox creation fails before readiness if a required `USER` component is missing, malformed, unknown, ambiguous, or resolves to UID/GID 0. An image without `USER` therefore works only when policy explicitly provides both @@ -470,4 +462,6 @@ Docker and Podman custom images do not need a baked-in `"sandbox"` user. Declare a non-root OCI `USER`, or set both process identity fields explicitly in policy. Named image users require matching account entries; a numeric `UID:GID` pair does not. OpenShell continues to use `/sandbox` as the workspace; it does not -adopt the image's OCI working directory. +adopt the image's OCI working directory. Until OCI working-directory support is +added, custom images must create `/sandbox` and make it writable by the selected +identity. diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index 7e3d4d9ea4..4cda100dfe 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -24,11 +24,6 @@ RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ RUN groupadd -g 1235 appstaff && \ useradd -m -u 1234 -g appstaff app -# A custom image may already contain a root-owned OpenShell workspace and -# root-owned files. The supervisor prepares only the workspace directory. -RUN install -d /sandbox && \ - printf root-owned > /sandbox/root-owned.txt - # Write a marker file so we can verify this is our custom image. # Place under /etc (Landlock baseline read-only path) so the sandbox # can read it when filesystem restrictions are properly enforced. @@ -49,8 +44,7 @@ CMD ["sleep", "infinity"] const MARKER: &str = "custom-image-e2e-marker"; -/// A named OCI user can write through direct and SSH children when the image -/// starts with a root-owned `/sandbox`; existing content retains its ownership. +/// Direct and SSH children use the same named OCI identity. #[tokio::test] async fn sandbox_from_custom_dockerfile() { // Step 1: Write a temporary Dockerfile. @@ -69,10 +63,7 @@ async fn sandbox_from_custom_dockerfile() { &[ "sh", "-c", - "set -eu; id -u; id -g; test \"$(cat /sandbox/root-owned.txt)\" = root-owned; \ - test \"$(stat -c %u:%g /sandbox)\" = 1234:1235; \ - test \"$(stat -c %u:%g /sandbox/root-owned.txt)\" = 0:0; \ - touch /sandbox/direct-oci-user-write; cat /etc/marker.txt; echo Ready; sleep infinity", + "set -eu; id -u; id -g; cat /etc/marker.txt; echo Ready; sleep infinity", ], "Ready", ) @@ -94,14 +85,13 @@ async fn sandbox_from_custom_dockerfile() { .exec(&[ "sh", "-c", - "set -eu; test \"$(id -u):$(id -g)\" = 1234:1235; \ - touch /sandbox/ssh-oci-user-write; echo ssh-write-ok", + "set -eu; test \"$(id -u):$(id -g)\" = 1234:1235; echo ssh-identity-ok", ]) .await - .expect("SSH child should write to prepared workspace"); + .expect("SSH child should use OCI identity"); assert!( - ssh_output.contains("ssh-write-ok"), - "expected SSH write marker:\n{ssh_output}" + ssh_output.contains("ssh-identity-ok"), + "expected SSH identity marker:\n{ssh_output}" ); // Explicit cleanup (also happens in Drop, but explicit is clearer in tests). @@ -109,7 +99,6 @@ async fn sandbox_from_custom_dockerfile() { } /// A numeric OCI user/group pair works without passwd or group entries. -/// The image intentionally has no pre-existing `/sandbox`. #[tokio::test] async fn sandbox_from_passwd_less_numeric_oci_user() { let tmpdir = tempfile::tempdir().expect("create tmpdir"); @@ -121,16 +110,10 @@ async fn sandbox_from_passwd_less_numeric_oci_user() { } let dockerfile_str = dockerfile_path.to_str().expect("Dockerfile path is UTF-8"); - let mut guard = SandboxGuard::create(&[ - "--from", - dockerfile_str, - "--", - "sh", - "-c", - "id -u; id -g; touch /sandbox/numeric-oci-user-write", - ]) - .await - .expect("sandbox create from numeric OCI Dockerfile"); + let mut guard = + SandboxGuard::create(&["--from", dockerfile_str, "--", "sh", "-c", "id -u; id -g"]) + .await + .expect("sandbox create from numeric OCI Dockerfile"); let clean_output = strip_ansi(&guard.create_output); assert!( diff --git a/e2e/rust/tests/driver_config_volume.rs b/e2e/rust/tests/driver_config_volume.rs index b4701f0f81..ad8cffc2f9 100644 --- a/e2e/rust/tests/driver_config_volume.rs +++ b/e2e/rust/tests/driver_config_volume.rs @@ -23,15 +23,6 @@ use serde_json::{Map, Value}; const TEST_IMAGE: &str = "ghcr.io/nvidia/openshell-community/sandboxes/base:latest"; const VOLUME_TARGET: &str = "/sandbox/e2e-volume"; const BIND_TARGET: &str = "/sandbox/e2e-bind"; -#[cfg(feature = "e2e-docker")] -const OCI_USER_DOCKERFILE: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim - -RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ - && rm -rf /var/lib/apt/lists/* - -USER 2234:2235 -CMD ["sleep", "infinity"] -"#; struct VolumeGuard { docker: Docker, @@ -110,69 +101,6 @@ async fn sandbox_mounts_existing_driver_config_volume() { .expect("verify sandbox wrote to named test volume"); } -#[tokio::test] -#[cfg(feature = "e2e-docker")] -async fn oci_workspace_preparation_skips_nested_volume_ownership() { - let driver = e2e_driver().expect("OPENSHELL_E2E_DRIVER must be set by the e2e wrapper"); - assert!( - matches!(driver.as_str(), "docker" | "podman"), - "OCI workspace mount e2e requires docker or podman, got {driver}" - ); - - let volume = VolumeGuard::create(&driver) - .await - .expect("create named test volume"); - seed_volume(&volume).await.expect("seed named test volume"); - - let image_context = tempfile::tempdir().expect("create OCI image context"); - let dockerfile = image_context.path().join("Dockerfile"); - fs::write(&dockerfile, OCI_USER_DOCKERFILE).expect("write OCI image Dockerfile"); - let dockerfile = dockerfile.to_str().expect("Dockerfile path must be UTF-8"); - - let driver_config = format!( - r#"{{"{driver}":{{"mounts":[{{"type":"volume","source":"{}","target":"{VOLUME_TARGET}","read_only":false}}]}}}}"#, - volume.name - ); - let mut sandbox = SandboxGuard::create_keep_with_args( - &[ - "--from", - dockerfile, - "--driver-config-json", - &driver_config, - "--no-tty", - ], - &[ - "sh", - "-lc", - "set -eu; test \"$(id -u):$(id -g)\" = 2234:2235; \ - test \"$(stat -c %u:%g /sandbox/e2e-volume/input.txt)\" = 0:0; \ - touch /sandbox/direct-write; echo Ready; sleep infinity", - ], - "Ready", - ) - .await - .expect("create OCI-user sandbox with nested volume"); - - let ssh_output = sandbox - .exec(&[ - "sh", - "-lc", - "set -eu; test \"$(stat -c %u:%g /sandbox/e2e-volume/input.txt)\" = 0:0; \ - touch /sandbox/ssh-write; echo nested-mount-owner-ok", - ]) - .await - .expect("SSH child should preserve nested volume ownership"); - assert!( - ssh_output.contains("nested-mount-owner-ok"), - "expected nested mount ownership marker:\n{ssh_output}" - ); - - sandbox.cleanup().await; - verify_volume_ownership(&volume) - .await - .expect("nested volume ownership should remain unchanged"); -} - #[tokio::test] async fn sandbox_mounts_enabled_driver_config_bind() { let driver = e2e_driver().expect("OPENSHELL_E2E_DRIVER must be set by the e2e wrapper"); @@ -280,23 +208,6 @@ async fn verify_volume(volume: &VolumeGuard) -> Result<(), String> { Ok(()) } -#[cfg(feature = "e2e-docker")] -async fn verify_volume_ownership(volume: &VolumeGuard) -> Result<(), String> { - let output = run_volume_container( - volume, - "verify-owner", - true, - "set -eu; test \"$(stat -c %u:%g /vol/input.txt)\" = 0:0; echo owner-ok", - ) - .await?; - if !output.contains("owner-ok") { - return Err(format!( - "volume ownership verification did not print expected marker:\n{output}" - )); - } - Ok(()) -} - async fn run_volume_container( volume: &VolumeGuard, purpose: &str, diff --git a/examples/bring-your-own-container/README.md b/examples/bring-your-own-container/README.md index 14d3a23613..c79e571f51 100644 --- a/examples/bring-your-own-container/README.md +++ b/examples/bring-your-own-container/README.md @@ -63,10 +63,9 @@ key requirements are: such as `app`, a numeric UID with a passwd entry that supplies its primary GID, or a numeric pair such as `1500:1500`. You can instead set both `process.run_as_user` and `process.run_as_group` explicitly in policy. -- **Use `/sandbox` as the workspace.** Docker and Podman create and prepare the - directory automatically when OCI `USER` supplies any identity component. - Pre-own any files beneath it that the agent must modify; OpenShell does not - recursively rewrite image content ownership. +- **Prepare `/sandbox` as the workspace.** Until OCI working-directory support + is added, create `/sandbox` and make it writable by the selected identity. + The example does this with `install -d -o app -g app /sandbox`. - **Install `iproute2`** for full network namespace isolation. - **Use a standard Linux base image** — distroless and `FROM scratch` images are not supported. From 5cc531f04d2930b97d37f24afa18a7459a12bc87 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 15:30:43 -0700 Subject: [PATCH 11/12] test(podman): cover OCI identity launch Signed-off-by: Matthew Grossman --- docs/sandboxes/policies.mdx | 2 +- docs/security/best-practices.mdx | 2 +- e2e/rust/Cargo.toml | 5 + e2e/rust/tests/podman_oci_identity.rs | 205 ++++++++++++++++++++++++++ 4 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 e2e/rust/tests/podman_oci_identity.rs diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index a459c22cf6..8eee13bee5 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -68,7 +68,7 @@ Raw streams are connection-scoped and outside L7 live-reload guarantees. This in |---|---|---| | `filesystem_policy` | Static | Controls which directories the agent can access on disk. Paths are split into `read_only` and `read_write` lists. Any path not listed in either list is inaccessible. Set `include_workdir: true` to automatically add the agent's working directory to `read_write`. [Landlock LSM](https://docs.kernel.org/security/landlock.html) enforces these restrictions at the kernel level. | | `landlock` | Static | Configures Landlock LSM enforcement behavior. Set `compatibility` to `best_effort` (skip individual inaccessible paths while applying remaining rules) or `hard_requirement` (fail if any path is inaccessible or the required kernel ABI is unavailable). Refer to the [Policy Schema Reference](/reference/policy-schema#landlock) for the full behavior table. | -| `process` | Static | Optionally overrides the OS-level identity for the agent process. Docker and Podman fall back per field to the image's OCI `USER`; Kubernetes uses its platform-selected numeric identity. Root (`root` or `0`) is rejected. The agent also runs with seccomp filters that block dangerous system calls. | +| `process` | Static | Optionally overrides the OS-level identity for the agent process. Explicit values must be `sandbox` or numeric UID/GID values in the allowed sandbox range. Docker and Podman may use named identities or non-root system IDs only through per-field OCI `USER` fallback; Kubernetes uses its platform-selected numeric identity. Root identities are always rejected. The agent also runs with seccomp filters that block dangerous system calls. | | `network_policies` | Dynamic | Controls network access for ordinary outbound traffic from the sandbox. Each block has a name, a list of endpoints (host, port, protocol, and optional rules), and a list of binaries allowed to use those endpoints.
Every outbound connection except `https://inference.local` goes through the proxy, which queries the [policy engine](/about/how-it-works#core-components) with the destination and calling binary. A connection is allowed only when both match an entry in the same policy block.
For endpoints with `protocol: rest`, the proxy auto-detects TLS and terminates it so each HTTP request can be checked against that endpoint's `rules` (method and path). For endpoints with `protocol: websocket`, the proxy validates the RFC 6455 upgrade and evaluates `GET` rules for the handshake plus either `WEBSOCKET_TEXT` rules for raw client text messages or GraphQL operation rules for GraphQL-over-WebSocket messages. Set `websocket_credential_rewrite: true` only when a WebSocket or REST compatibility endpoint must keep placeholder credentials in sandbox-owned text frames and resolve them at the OpenShell relay boundary.
Endpoints without `protocol` allow the TCP stream through without inspecting payloads.
If no endpoint matches, the connection is denied. Configure managed inference separately through [Inference Routing](/sandboxes/inference-routing). | | `network_middlewares` | Dynamic | Declares keyed HTTP request middleware configs. After network and L7 policy admit a request, OpenShell matches each config's host selectors independently and runs matching entries by their unique ascending `order` before credential injection. | diff --git a/docs/security/best-practices.mdx b/docs/security/best-practices.mdx index 2653a17ea4..0ac0d5528f 100644 --- a/docs/security/best-practices.mdx +++ b/docs/security/best-practices.mdx @@ -202,7 +202,7 @@ The sandbox process runs as a non-root user after explicit privilege dropping. | Aspect | Detail | |---|---| | Default | The compute driver selects a non-root identity. Docker and Podman use the image's OCI `USER` as a per-field fallback. The supervisor calls `setuid()`/`setgid()` with post-condition verification, disables core dumps with `RLIMIT_CORE=0`, and on Linux sets `PR_SET_DUMPABLE=0`. | -| What you can change | Set either or both `run_as_user` and `run_as_group` fields in the `process` section. Each explicit field takes precedence. Validation rejects root (`root` or `0`). | +| What you can change | Set either or both `run_as_user` and `run_as_group` fields in the `process` section. Each explicit field takes precedence and must be `sandbox` or a numeric UID/GID value in the allowed sandbox range. Docker and Podman may use named identities or non-root system IDs only through OCI `USER` fallback. Root identities are always rejected. | | Risk if relaxed | Running as a higher-privilege user increases the impact of container escape vulnerabilities. | | Recommendation | Use a dedicated non-root image identity or explicit numeric policy identity. Do not attempt to set root. | diff --git a/e2e/rust/Cargo.toml b/e2e/rust/Cargo.toml index b36a32203f..466a97c573 100644 --- a/e2e/rust/Cargo.toml +++ b/e2e/rust/Cargo.toml @@ -67,6 +67,11 @@ name = "podman_corporate_proxy" path = "tests/podman_corporate_proxy.rs" required-features = ["e2e-podman"] +[[test]] +name = "podman_oci_identity" +path = "tests/podman_oci_identity.rs" +required-features = ["e2e-podman"] + [[test]] name = "vm_gateway_resume" path = "tests/vm_gateway_resume.rs" diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs new file mode 100644 index 0000000000..fc98c85bb2 --- /dev/null +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -0,0 +1,205 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#![cfg(feature = "e2e-podman")] + +//! Podman-specific E2E coverage for OCI identity inspection and immutable-image +//! launch. +//! +//! The test builds an image through the selected Podman engine, creates a +//! sandbox from its mutable tag, and verifies both the child identity and the +//! image ID recorded on the real sandbox container. This exercises the Podman +//! API inspect → protected metadata → create path rather than only its unit +//! serialization boundaries. + +use std::process::Stdio; + +use openshell_e2e::harness::container::{ContainerEngine, is_e2e_driver}; +use openshell_e2e::harness::output::strip_ansi; +use openshell_e2e::harness::sandbox::SandboxGuard; + +const BASE_IMAGE: &str = "ghcr.io/nvidia/openshell-community/sandboxes/base:latest"; +const READY_MARKER: &str = "podman-oci-identity-ready"; +const OCI_UID: &str = "2345"; +const OCI_GID: &str = "2346"; + +struct ImageGuard { + engine: ContainerEngine, + tag: String, + id: String, +} + +impl ImageGuard { + fn build() -> Result { + let engine = ContainerEngine::from_env()?; + if engine.name() != "podman" { + return Err(format!( + "Podman OCI identity E2E requires podman, got {}", + engine.name() + )); + } + + let context = tempfile::tempdir().map_err(|err| format!("create build context: {err}"))?; + let containerfile = context.path().join("Containerfile"); + std::fs::write( + &containerfile, + format!("FROM {BASE_IMAGE}\nUSER {OCI_UID}:{OCI_GID}\n"), + ) + .map_err(|err| format!("write Containerfile: {err}"))?; + + let tag = format!( + "localhost/openshell-e2e-podman-oci-identity:{}", + std::process::id() + ); + run_engine( + &engine, + &[ + "build", + "--pull=never", + "--file", + containerfile + .to_str() + .ok_or_else(|| "Containerfile path is not UTF-8".to_string())?, + "--tag", + &tag, + context + .path() + .to_str() + .ok_or_else(|| "build context path is not UTF-8".to_string())?, + ], + )?; + let id = run_engine(&engine, &["image", "inspect", "--format", "{{.Id}}", &tag])?; + + Ok(Self { engine, tag, id }) + } +} + +impl Drop for ImageGuard { + fn drop(&mut self) { + let _ = self + .engine + .command() + .args(["image", "rm", "--force", &self.tag]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + } +} + +fn run_engine(engine: &ContainerEngine, args: &[&str]) -> Result { + let output = engine + .command() + .args(args) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .map_err(|err| format!("failed to run {} {}: {err}", engine.name(), args.join(" ")))?; + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + if !output.status.success() { + return Err(format!( + "{} {} failed (exit {:?}):\n{stdout}{stderr}", + engine.name(), + args.join(" "), + output.status.code() + )); + } + Ok(stdout.trim().to_string()) +} + +fn sandbox_container_id(engine: &ContainerEngine, sandbox_name: &str) -> Result { + let name_filter = format!("label=openshell.ai/sandbox-name={sandbox_name}"); + let stdout = run_engine( + engine, + &[ + "ps", + "-aq", + "--filter", + "label=openshell.managed=true", + "--filter", + &name_filter, + ], + )?; + let ids = stdout + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .collect::>(); + match ids.as_slice() { + [id] => Ok((*id).to_string()), + [] => Err(format!( + "no Podman container found for sandbox '{sandbox_name}'" + )), + _ => Err(format!( + "multiple Podman containers found for sandbox '{sandbox_name}': {ids:?}" + )), + } +} + +fn normalized_image_id(image_id: &str) -> &str { + image_id + .trim() + .strip_prefix("sha256:") + .unwrap_or(image_id.trim()) +} + +#[tokio::test] +async fn podman_uses_oci_identity_and_inspected_image_id() { + if !is_e2e_driver("podman") { + eprintln!("Skipping Podman OCI identity test: e2e driver is not podman"); + return; + } + + let image = ImageGuard::build().expect("build Podman OCI identity image"); + let mut sandbox = SandboxGuard::create_keep_with_args( + &["--from", &image.tag, "--no-tty"], + &[ + "sh", + "-c", + "set -eu; printf 'direct-identity=%s:%s\n' \"$(id -u)\" \"$(id -g)\"; echo podman-oci-identity-ready; sleep infinity", + ], + READY_MARKER, + ) + .await + .expect("create sandbox from Podman-built OCI identity image"); + + let direct_output = strip_ansi(&sandbox.create_output); + assert!( + direct_output.contains("direct-identity=2345:2346"), + "expected direct child identity {OCI_UID}:{OCI_GID}:\n{direct_output}" + ); + + let ssh_output = sandbox + .exec(&[ + "sh", + "-c", + "test \"$(id -u):$(id -g)\" = 2345:2346; echo podman-ssh-identity-ok", + ]) + .await + .expect("SSH child should use Podman OCI identity"); + assert!( + ssh_output.contains("podman-ssh-identity-ok"), + "expected SSH identity marker:\n{ssh_output}" + ); + + let container_id = + sandbox_container_id(&image.engine, &sandbox.name).expect("find Podman sandbox container"); + let launched_image_id = run_engine( + &image.engine, + &[ + "container", + "inspect", + "--format", + "{{.Image}}", + &container_id, + ], + ) + .expect("inspect Podman sandbox container image"); + assert_eq!( + normalized_image_id(&launched_image_id), + normalized_image_id(&image.id), + "Podman sandbox must launch the immutable image ID inspected before creation" + ); + + sandbox.cleanup().await; +} From 254364a2fab50b83c2ceb847b8f4b183bfbc4801 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 28 Jul 2026 16:03:48 -0700 Subject: [PATCH 12/12] fix(podman): exercise OCI fallback in E2E Signed-off-by: Matthew Grossman --- e2e/rust/tests/podman_oci_identity.rs | 34 ++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs index fc98c85bb2..e30516bf09 100644 --- a/e2e/rust/tests/podman_oci_identity.rs +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -22,6 +22,17 @@ const BASE_IMAGE: &str = "ghcr.io/nvidia/openshell-community/sandboxes/base:late const READY_MARKER: &str = "podman-oci-identity-ready"; const OCI_UID: &str = "2345"; const OCI_GID: &str = "2346"; +const OCI_FALLBACK_POLICY: &str = r#"version: 1 + +filesystem_policy: + include_workdir: true + read_only: [/usr, /lib, /lib64, /proc, /dev/urandom, /etc] + read_write: [/sandbox, /tmp, /dev/null] +landlock: + compatibility: best_effort + +network_policies: {} +"#; struct ImageGuard { engine: ContainerEngine, @@ -69,6 +80,15 @@ impl ImageGuard { ], )?; let id = run_engine(&engine, &["image", "inspect", "--format", "{{.Id}}", &tag])?; + let user = run_engine( + &engine, + &["image", "inspect", "--format", "{{.Config.User}}", &tag], + )?; + if user != format!("{OCI_UID}:{OCI_GID}") { + return Err(format!( + "Podman-built image has OCI user '{user}', expected {OCI_UID}:{OCI_GID}" + )); + } Ok(Self { engine, tag, id }) } @@ -151,8 +171,20 @@ async fn podman_uses_oci_identity_and_inspected_image_id() { } let image = ImageGuard::build().expect("build Podman OCI identity image"); + // The community base image contains a baked default policy with an + // explicit `sandbox` process identity. Supply a complete policy that + // intentionally omits `process` so this test exercises OCI fallback. + let policy = tempfile::NamedTempFile::new().expect("create OCI fallback policy"); + std::fs::write(policy.path(), OCI_FALLBACK_POLICY).expect("write OCI fallback policy"); + let policy_path = policy.path().to_str().expect("policy path is UTF-8"); let mut sandbox = SandboxGuard::create_keep_with_args( - &["--from", &image.tag, "--no-tty"], + &[ + "--from", + &image.tag, + "--policy", + policy_path, + "--no-tty", + ], &[ "sh", "-c",