From 2ab6ea3c18d306a26e5b19ee52dd1982a37aff98 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 30 Jul 2026 13:19:21 -0700 Subject: [PATCH 1/5] feat(podman): honor OCI image working directories Signed-off-by: Matthew Grossman --- .../skills/debug-openshell-cluster/SKILL.md | 2 + architecture/compute-runtimes.md | 23 +- crates/openshell-core/src/sandbox_env.rs | 7 + crates/openshell-driver-docker/src/lib.rs | 8 + crates/openshell-driver-docker/src/tests.rs | 9 + crates/openshell-driver-podman/README.md | 40 +- crates/openshell-driver-podman/src/client.rs | 128 +++++- .../openshell-driver-podman/src/container.rs | 419 +++++++++++++++++- crates/openshell-driver-podman/src/driver.rs | 307 +++++++++++-- crates/openshell-sandbox/src/lib.rs | 34 +- crates/openshell-sandbox/src/main.rs | 105 ++++- crates/openshell-server/src/compute/mod.rs | 42 ++ .../src/process.rs | 209 ++++++++- .../openshell-supervisor-process/src/ssh.rs | 4 +- docs/reference/sandbox-compute-drivers.mdx | 36 +- e2e/rust/tests/custom_image.rs | 3 + e2e/rust/tests/driver_config_volume.rs | 12 +- e2e/rust/tests/podman_oci_identity.rs | 51 ++- proto/compute_driver.proto | 11 + 19 files changed, 1320 insertions(+), 130 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index cbff462d45..4a9859b366 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -202,6 +202,8 @@ Common findings: - 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. +- Sandbox fails before readiness with an OCI workspace validation error: inspect the image's `WorkingDir` using the immutable image ID reported by the gateway. Empty, `/`, and explicit `/sandbox` use the managed `/sandbox` compatibility workspace. Any other workdir must be an absolute normalized directory with no symlink components; the final policy UID, primary GID, or supplementary groups must already be able to traverse every parent and write and enter the directory. Podman checks the original image in a networkless temporary probe before attaching the workspace volume, so inspect the probe failure in gateway logs. +- If Podman reports probe cleanup or timeout failures, inspect temporary containers with `podman ps -a --filter name=workdir-probe` and gateway logs. The driver force-removes the probe on every normal success or failure path. - Supervisor cannot call back: check callback endpoint and gateway logs. - Gateway exits before becoming healthy with a callback-listener discovery error: inspect `podman info --debug`, the configured Podman network, and the diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 646b6320bd..7ae07a934e 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -185,8 +185,8 @@ 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`. Docker also - resolves the workspace from OCI `Config.WorkingDir` during that inspection. + its immutable image ID, and pass its raw OCI `Config.User`. They also resolve + the workspace from OCI `Config.WorkingDir` during that inspection. - Kubernetes passes its platform-resolved numeric UID/GID, including OpenShift SCC-derived values. - VM keeps its existing guest identity behavior. @@ -199,7 +199,7 @@ 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. -Docker uses an absolute OCI working directory as the workspace. An +Docker and Podman use an absolute OCI working directory as the workspace. An empty, root (`/`), or explicit `/sandbox` declaration uses `/sandbox`, which OpenShell creates and owns as a compatibility workspace. Any other workdir must already exist in the immutable image without symlink components. The completed @@ -211,11 +211,18 @@ Path checks reserve the standard OCI runtime namespaces under `/proc`, `/sys`, and `/dev`, while separate collision checks are derived from actual OpenShell control paths. Docker performs the check in the final container before workload launch and -rejects image `VOLUME` declarations that would mask the workdir ancestry. The -resolved workspace is the child cwd and `HOME`; when -`filesystem.include_workdir` is enabled, it becomes the automatic writable -policy path. Podman, Kubernetes/OpenShift, and VM retain their existing -`/sandbox` workspace behavior. +rejects image `VOLUME` declarations that would mask the workdir ancestry. +Podman performs it in a minimal networkless container from the same pinned +image ID before its managed workspace volume covers the path. The probe retains +only the capabilities needed to adopt the completed process identity, drops to +that identity, and asks the kernel to validate access. It emits a normalized +identity attestation that the final supervisor must match, including when the +image supplies the default process policy. The driver captures a bounded, +sanitized diagnostic before removing a failed probe. The resolved workspace is +the child cwd and `HOME`; when `filesystem.include_workdir` is enabled, it +becomes the automatic writable policy path. Kubernetes/OpenShift keep their +`/sandbox` PVC and `fsGroup` behavior, and VM keeps its `/sandbox` 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-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index 1549258fa3..22a82e110c 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -126,6 +126,13 @@ pub const SANDBOX_GID: &str = "OPENSHELL_SANDBOX_GID"; /// OCI only for the former contract. pub const OCI_IMAGE_USER: &str = "OPENSHELL_OCI_IMAGE_USER"; +/// Normalized UID/GID/supplementary-group identity attested by the Podman +/// immutable-image workspace probe. +/// +/// A non-empty value also asserts that the +/// original workspace was validated before Podman's managed volume covered it. +pub const OCI_WORKSPACE_IDENTITY: &str = "OPENSHELL_OCI_WORKSPACE_IDENTITY"; + // 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/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index dd4d9ef0f0..d1c096e0e9 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -2195,6 +2195,7 @@ fn build_environment_for_oci_user( user_env.extend(template.environment.clone()); } user_env.extend(spec.environment.clone()); + user_env.remove(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY); environment.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) @@ -2250,6 +2251,13 @@ fn build_environment_for_oci_user( environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); + // Docker never uses the Podman prevalidation contract. Emit an explicit + // driver-owned empty value so image-baked ENV entries cannot select the + // managed-workspace mutation path for an image-provided workdir. + environment.insert( + openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY.to_string(), + String::new(), + ); environment.insert( openshell_core::sandbox_env::OCI_IMAGE_USER.to_string(), oci_user.to_string(), diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index ac525c705c..c7b83b0cec 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -45,6 +45,7 @@ fn test_sandbox() -> DriverSandbox { }), resource_requirements: None, sandbox_token: String::new(), + workspace_validation_identity: None, }), status: None, workspace: String::new(), @@ -578,6 +579,10 @@ fn build_environment_protects_oci_identity_metadata() { (openshell_core::sandbox_env::OCI_IMAGE_USER, "spoofed"), (openshell_core::sandbox_env::SANDBOX_UID, "9999"), (openshell_core::sandbox_env::SANDBOX_GID, "9999"), + ( + openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY, + "9999:9999:", + ), ] { spec.environment.insert(key.to_string(), value.to_string()); } @@ -590,6 +595,10 @@ fn build_environment_protects_oci_identity_metadata() { ))); assert!(env.contains(&format!("{}=", openshell_core::sandbox_env::SANDBOX_UID))); assert!(env.contains(&format!("{}=", openshell_core::sandbox_env::SANDBOX_GID))); + assert!(env.contains(&format!( + "{}=", + openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY + ))); assert!(!env.iter().any(|entry| entry.ends_with("=spoofed"))); assert!(!env.iter().any(|entry| entry.ends_with("=9999"))); } diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 965a295d19..4762d92480 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -8,14 +8,30 @@ 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 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. +captures its immutable image ID, raw OCI `Config.User`, and OCI +`Config.WorkingDir`. 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 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. + +An absolute OCI working directory becomes the agent workspace. An empty, +root (`/`), or explicit `/sandbox` declaration uses `/sandbox`, which OpenShell +creates when necessary and owns as a compatibility workspace. For any other workdir, Podman +first starts a minimal, networkless probe from the pinned image ID without the +workspace volume, tokens, or TLS secrets. The probe retains only `SETUID` and +`SETGID`, resolves and adopts the completed identity, including supplementary +groups, and uses the kernel to verify that it can traverse every parent and +write and enter the workdir. The path must be a real directory without symlink +components. The probe also rejects kernel-managed filesystems and overlaps with +concrete OpenShell control resources. It emits a normalized identity +attestation; the final supervisor must resolve to the same identity, including +when the image supplies the default process policy. On failure, the driver +captures a bounded, sanitized diagnostic before removing the probe. Only then +does Podman mount and prepare the managed workspace volume at that path; normal +copy-up preserves image content. The workspace is the child cwd and `HOME`. For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md). @@ -87,9 +103,11 @@ optional `selinux_label` of `shared` (applies `:z`) or `private` (applies read-only by default; set `read_only: false` to make them writable. Podman image and volume mounts do not support `subpath` in OpenShell driver config. Mount `source` and `target` values must not contain surrounding whitespace. -Mount targets must be absolute container paths and must not replace -the workspace root (`/sandbox`) or overlap OpenShell supervisor files, -`/etc/openshell`, `/etc/openshell-tls`, or `/run/netns`. +Mount targets must be absolute container paths and must not replace the +resolved workspace root or any of its parents. Nested workspace mounts remain +valid. Mounts also must not contain or be contained by concrete OpenShell +control targets such as the supervisor mount, TLS and token files, runtime +socket, or `/run/netns`. Example named-volume usage: diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 9fe39cf7e2..1c5e7e05bf 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -25,6 +25,7 @@ const API_TIMEOUT: Duration = Duration::from_secs(30); /// Maximum allowed size for the event stream line buffer (1 MB). const MAX_EVENT_BUFFER: usize = 1_048_576; +const MAX_CONTAINER_LOG_BYTES: usize = 16 * 1024; #[derive(Debug, thiserror::Error)] pub enum PodmanApiError { @@ -117,6 +118,16 @@ pub struct ContainerState { pub finished_at: Option, } +#[derive(Debug, serde::Deserialize)] +#[serde(untagged)] +enum ContainerWaitResponse { + ExitCode(i64), + Status { + #[serde(rename = "StatusCode")] + status_code: i64, + }, +} + #[derive(Debug, Clone, serde::Deserialize)] #[serde(rename_all = "PascalCase")] pub struct HealthState { @@ -177,6 +188,8 @@ pub struct ImageInspect { pub struct ImageConfig { #[serde(default)] pub user: String, + #[serde(default)] + pub working_dir: String, } /// A container summary returned by the list API. @@ -352,6 +365,43 @@ impl PodmanClient { Ok((status, bytes)) } + /// Send a request while retaining at most `max_bytes` of the response. + async fn send_request_bounded( + &self, + req: Request>, + timeout: Duration, + max_bytes: usize, + ) -> Result<(hyper::StatusCode, Bytes), PodmanApiError> { + use hyper::body::Body; + + let mut sender = self.connect().await?; + let response = tokio::time::timeout(timeout, sender.send_request(req)) + .await + .map_err(|_| PodmanApiError::Timeout(timeout))? + .map_err(|error| PodmanApiError::Connection(error.to_string()))?; + let status = response.status(); + let deadline = tokio::time::Instant::now() + timeout; + let mut body = response.into_body(); + let mut bytes = Vec::with_capacity(max_bytes.min(4096)); + while bytes.len() < max_bytes { + let frame = tokio::time::timeout_at( + deadline, + std::future::poll_fn(|context| Pin::new(&mut body).poll_frame(context)), + ) + .await + .map_err(|_| PodmanApiError::Timeout(timeout))?; + let Some(frame) = frame else { + break; + }; + let frame = frame.map_err(|error| PodmanApiError::Connection(error.to_string()))?; + if let Some(data) = frame.data_ref() { + let remaining = max_bytes - bytes.len(); + bytes.extend_from_slice(&data[..data.len().min(remaining)]); + } + } + Ok((status, Bytes::from(bytes))) + } + /// Perform a versioned HTTP request and return status + body bytes. async fn request( &self, @@ -457,6 +507,22 @@ impl PodmanClient { .await } + /// Wait for a container to exit and return its exit code. + pub async fn wait_container(&self, name: &str) -> Result { + validate_name(name)?; + let response: ContainerWaitResponse = self + .request_json( + hyper::Method::POST, + &format!("/libpod/containers/{name}/wait?condition=exited"), + None, + ) + .await?; + Ok(match response { + ContainerWaitResponse::ExitCode(code) => code, + ContainerWaitResponse::Status { status_code } => status_code, + }) + } + /// Stop a container with a grace period in seconds. pub async fn stop_container( &self, @@ -524,6 +590,27 @@ impl PodmanClient { .await } + /// Fetch a bounded tail of stdout and stderr from a container. + pub async fn container_logs(&self, name: &str) -> Result { + validate_name(name)?; + let req = Self::build_request( + hyper::Method::GET, + &format!( + "/{API_VERSION}/libpod/containers/{name}/logs?stdout=true&stderr=true&tail=20×tamps=false" + ), + Full::new(Bytes::new()), + None, + ); + let (status, bytes) = self + .send_request_bounded(req, API_TIMEOUT, MAX_CONTAINER_LOG_BYTES) + .await?; + if status.is_success() { + Ok(String::from_utf8_lossy(&bytes).into_owned()) + } else { + Err(error_from_response(status.as_u16(), &bytes)) + } + } + /// List containers matching label filters (e.g. `&["openshell.managed=true"]`). pub async fn list_containers( &self, @@ -973,12 +1060,12 @@ mod tests { } #[tokio::test] - async fn inspect_image_reads_immutable_id_and_oci_user() { + async fn inspect_image_reads_immutable_id_and_oci_config() { let (socket_path, request_log, handle) = spawn_podman_stub( "inspect-image", vec![StubResponse::new( StatusCode::OK, - r#"{"Id":"sha256:immutable","Config":{"User":"app:staff"}}"#, + r#"{"Id":"sha256:immutable","Config":{"User":"app:staff","WorkingDir":"/workspace/project"}}"#, )], ); let client = PodmanClient::new(socket_path.clone()); @@ -993,6 +1080,13 @@ mod tests { image.config.as_ref().map(|config| config.user.as_str()), Some("app:staff") ); + assert_eq!( + image + .config + .as_ref() + .map(|config| config.working_dir.as_str()), + Some("/workspace/project") + ); handle.await.expect("stub task should finish"); assert_eq!( request_log @@ -1055,4 +1149,34 @@ mod tests { handle.await.expect("stub task should finish"); let _ = std::fs::remove_file(socket_path); } + + #[tokio::test] + async fn container_logs_fetches_bounded_stdout_and_stderr_tail() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "container-logs", + vec![StubResponse::new( + StatusCode::OK, + "workspace validation failed\n", + )], + ); + let client = PodmanClient::new(socket_path.clone()); + + let logs = client + .container_logs("workdir-probe") + .await + .expect("container logs should be returned"); + + assert_eq!(logs, "workspace validation failed\n"); + 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/containers/workdir-probe/logs?stdout=true&stderr=true&tail=20×tamps=false" + ] + ); + 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 005f688a19..4b778bd3af 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -3,6 +3,7 @@ //! Container spec construction for the Podman driver. +use crate::client::ImageInspect; use crate::config::PodmanComputeConfig; use openshell_core::ComputeDriverError; use openshell_core::driver_mounts::SelinuxLabel; @@ -190,6 +191,9 @@ struct ContainerSpec { volumes: Vec, image_volumes: Vec, hostname: String, + /// Start the supervisor independently of the image workspace. The + /// supervisor validates and prepares that workspace before child launch. + work_dir: String, /// Overrides the image's ENTRYPOINT. In Podman's libpod API, `command` /// only overrides CMD (appended as args to the entrypoint). We must set /// `entrypoint` explicitly so the supervisor binary runs directly, @@ -423,6 +427,7 @@ fn build_env( user_env.insert(k.clone(), v.clone()); } } + user_env.remove(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY); env.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) @@ -483,6 +488,13 @@ fn build_env( env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); + // The final spec overwrites this only after a successful immutable-image + // probe. An explicit empty value prevents image ENV from forging the + // attestation contract on the /sandbox compatibility path. + env.insert( + openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY.into(), + String::new(), + ); env.insert( openshell_core::sandbox_env::OCI_IMAGE_USER.into(), oci_user.to_string(), @@ -495,7 +507,6 @@ fn build_env( 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. if let Some(s) = spec @@ -612,6 +623,7 @@ pub fn podman_driver_image_mount_sources( fn podman_user_mounts( sandbox: &DriverSandbox, enable_bind_mounts: bool, + workspace_root: &str, ) -> Result { let template = sandbox .spec @@ -623,6 +635,13 @@ fn podman_user_mounts( let config = podman_driver_config(template, enable_bind_mounts)?; let mut result = PodmanUserMounts::default(); for mount in config.mounts { + let target = match &mount { + PodmanDriverMountConfig::Bind { target, .. } + | PodmanDriverMountConfig::Volume { target, .. } + | PodmanDriverMountConfig::Tmpfs { target, .. } + | PodmanDriverMountConfig::Image { target, .. } => target, + }; + driver_mounts::validate_workspace_mount_target(target, workspace_root)?; match mount { PodmanDriverMountConfig::Bind { source, @@ -903,8 +922,11 @@ pub fn build_container_spec_with_token_and_gpu_devices( token_secret_name, gpu_device_ids, image, - image, - "", + &ImageInspect { + id: image.to_string(), + config: None, + }, + None, ) } @@ -914,16 +936,31 @@ pub fn build_container_spec_for_image( token_secret_name: Option<&str>, gpu_device_ids: Option<&[String]>, requested_image: &str, - image_id: &str, - oci_user: &str, + inspected_image: &ImageInspect, + workspace_identity: Option<&str>, ) -> Result { let name = container_name(&sandbox.workspace, &sandbox.name, &sandbox.id); let vol = volume_name(&sandbox.id); + let oci_user = inspected_image + .config + .as_ref() + .map_or("", |config| config.user.as_str()); + let oci_working_dir = inspected_image + .config + .as_ref() + .map_or("", |config| config.working_dir.as_str()); + let workspace_root = driver_mounts::resolve_oci_workspace_root(oci_working_dir) + .map_err(ComputeDriverError::Precondition)?; + driver_mounts::validate_workspace_control_path( + &workspace_root, + &config.sandbox_ssh_socket_path, + ) + .map_err(ComputeDriverError::Precondition)?; - let env = build_env(sandbox, config, requested_image, oci_user); + let mut 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) + let user_mounts = podman_user_mounts(sandbox, config.enable_bind_mounts, &workspace_root) .map_err(ComputeDriverError::InvalidArgument)?; if sandbox .spec @@ -952,7 +989,7 @@ pub fn build_container_spec_for_image( let mut volumes = vec![NamedVolume { name: vol, - dest: "/sandbox".into(), + dest: workspace_root.clone(), options: vec!["rw".into()], }]; volumes.extend(user_mounts.volumes); @@ -963,15 +1000,19 @@ pub fn build_container_spec_for_image( rw: false, }]; image_volumes.extend(user_mounts.image_volumes); - let mut command = vec![ - "--workdir".to_string(), - driver_mounts::DEFAULT_WORKSPACE_ROOT.to_string(), - ]; + + let mut command = vec!["--workdir".to_string(), workspace_root]; command.extend(upstream_proxy_cli_args(config)); + if let Some(identity) = workspace_identity { + env.insert( + openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY.into(), + identity.into(), + ); + } let container_spec = ContainerSpec { name, - image: image_id.to_string(), + image: inspected_image.id.clone(), labels, env, volumes, @@ -982,6 +1023,7 @@ pub fn build_container_spec_for_image( // /openshell-sandbox, so it appears at /opt/openshell/bin/openshell-sandbox. image_volumes, hostname: format!("sandbox-{}", sandbox.name), + work_dir: "/".to_string(), // Override the image's ENTRYPOINT so the supervisor binary runs // directly. Sandbox images (e.g. the community base image) set // ENTRYPOINT ["/bin/bash"], and Podman's `command` field only @@ -989,10 +1031,9 @@ pub fn build_container_spec_for_image( // Without this, the container would run the entrypoint binary with // the supervisor path as an argument instead of executing it directly. entrypoint: vec![SUPERVISOR_BINARY_PATH.into()], - // Keep Podman's existing /sandbox workspace contract explicit while - // the supervisor supports driver-selected workdirs. Operator-owned - // corporate proxy flags follow it; the workload command comes from - // the reserved environment variable. + // Operator-owned corporate proxy flags. The workload command is not + // part of argv (the supervisor takes it from the reserved command + // env var), so these flags are the whole command list. command, // Force the supervisor to run as root (UID 0). Sandbox images may // set a non-root USER directive (e.g. `USER sandbox`), but the @@ -1188,6 +1229,77 @@ pub fn build_container_spec_for_image( Ok(serde_json::to_value(container_spec).expect("ContainerSpec serialization cannot fail")) } +/// Build a minimal one-shot container that validates the image's original +/// workdir before the final Podman workspace volume covers it. +pub fn build_workspace_probe_spec( + sandbox: &DriverSandbox, + config: &PodmanComputeConfig, + inspected_image: &ImageInspect, + probe_name: &str, +) -> Result, ComputeDriverError> { + let image_config = inspected_image.config.as_ref(); + let oci_user = image_config.map_or("", |config| config.user.as_str()); + let oci_working_dir = image_config.map_or("", |config| config.working_dir.as_str()); + let workspace_root = driver_mounts::resolve_oci_workspace_root(oci_working_dir) + .map_err(ComputeDriverError::Precondition)?; + driver_mounts::validate_workspace_control_path( + &workspace_root, + &config.sandbox_ssh_socket_path, + ) + .map_err(ComputeDriverError::Precondition)?; + if workspace_root == driver_mounts::DEFAULT_WORKSPACE_ROOT { + return Ok(None); + } + + let spec = sandbox + .spec + .as_ref() + .ok_or_else(|| ComputeDriverError::Precondition("sandbox.spec is required".into()))?; + let mut command = vec![ + "probe-workspace".to_string(), + "--workdir".to_string(), + workspace_root, + "--oci-user".to_string(), + oci_user.to_string(), + ]; + if let Some(identity) = &spec.workspace_validation_identity { + for (flag, value) in [ + ("--run-as-user", identity.run_as_user.as_str()), + ("--run-as-group", identity.run_as_group.as_str()), + ] { + if !value.is_empty() { + command.push(flag.to_string()); + command.push(value.to_string()); + } + } + if identity.discover_from_image_policy { + command.push("--discover-policy-identity".to_string()); + } + } + + Ok(Some(serde_json::json!({ + "name": probe_name, + "image": inspected_image.id, + "entrypoint": [SUPERVISOR_BINARY_PATH], + "command": command, + "user": "0:0", + "work_dir": "/", + "image_volumes": [{ + "source": config.supervisor_image, + "destination": SUPERVISOR_MOUNT_DIR, + "rw": false + }], + // Do not materialize OCI VOLUME declarations: the probe must inspect + // the immutable image layer rather than a fresh anonymous volume. + "image_volume_mode": "ignore", + "netns": {"nsmode": "none"}, + "no_new_privileges": true, + "cap_drop": ["ALL"], + "cap_add": ["SETUID", "SETGID"], + "image_pull_policy": "never" + }))) +} + fn hostadd_entries(config: &PodmanComputeConfig) -> Vec { let host_gateway_ip = config.host_gateway_ip.trim(); if host_gateway_ip.is_empty() { @@ -1260,6 +1372,7 @@ fn parse_memory_to_bytes(quantity: &str) -> Option { #[cfg(test)] mod tests { use super::*; + use crate::client::ImageConfig; use openshell_core::proto::compute::v1::{GpuResourceRequirements, ResourceRequirements}; static ENV_LOCK: std::sync::LazyLock> = @@ -1279,6 +1392,16 @@ mod tests { } } + fn inspected_image(id: &str, user: &str, working_dir: &str) -> ImageInspect { + ImageInspect { + id: id.to_string(), + config: Some(ImageConfig { + user: user.to_string(), + working_dir: working_dir.to_string(), + }), + } + } + #[test] fn parse_cpu_millicore() { assert_eq!(parse_cpu_to_microseconds("500m"), Some(50_000)); @@ -1373,6 +1496,10 @@ mod tests { (openshell_core::sandbox_env::OCI_IMAGE_USER, "spoofed"), (openshell_core::sandbox_env::SANDBOX_UID, "9999"), (openshell_core::sandbox_env::SANDBOX_GID, "9999"), + ( + openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY, + "9999:9999:", + ), ] { spec.environment.insert(key.to_string(), value.to_string()); } @@ -1383,8 +1510,8 @@ mod tests { None, None, "registry.example/app:latest", - "sha256:immutable", - "app:staff", + &inspected_image("sha256:immutable", "app:staff", "/workspace/project"), + Some("1000:1000:"), ) .unwrap(); @@ -1395,6 +1522,18 @@ mod tests { ); assert_eq!(container["user"].as_str(), Some("0:0")); assert_eq!(container["image_pull_policy"].as_str(), Some("never")); + assert_eq!( + container["command"], + serde_json::json!(["--workdir", "/workspace/project"]) + ); + assert_eq!(container["work_dir"].as_str(), Some("/")); + assert!(container["volumes"].as_array().is_some_and(|volumes| { + volumes.iter().any(|volume| { + volume["name"].as_str() == Some("openshell-sandbox-test-id-workspace") + && volume["dest"].as_str() == Some("/workspace/project") + && volume["options"] == serde_json::json!(["rw"]) + }) + })); assert_eq!( container["env"][openshell_core::sandbox_env::OCI_IMAGE_USER].as_str(), Some("app:staff") @@ -1408,11 +1547,249 @@ mod tests { Some("") ); assert_eq!( - container["command"], - serde_json::json!(["--workdir", "/sandbox"]) + container["env"][openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY].as_str(), + Some("1000:1000:") + ); + } + + #[test] + fn compatibility_workspace_emits_empty_attestation() { + let container = build_container_spec_for_image( + &test_sandbox("test-id", "test-name"), + &test_config(), + None, + None, + "registry.example/app:latest", + &inspected_image("sha256:immutable", "app:staff", ""), + None, + ) + .unwrap(); + + assert_eq!( + container["env"][openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY].as_str(), + Some("") + ); + } + + #[test] + fn container_spec_rejects_invalid_oci_working_dir() { + let err = build_container_spec_for_image( + &test_sandbox("test-id", "test-name"), + &test_config(), + None, + None, + "registry.example/app:latest", + &inspected_image("sha256:immutable", "app:staff", "relative/workspace"), + None, + ) + .unwrap_err(); + + assert!( + err.to_string() + .contains("must be an absolute container path") + ); + } + + #[test] + fn container_spec_rejects_openshell_control_path_working_dir() { + let err = build_container_spec_for_image( + &test_sandbox("test-id", "test-name"), + &test_config(), + None, + None, + "registry.example/app:latest", + &inspected_image( + "sha256:immutable", + "app:staff", + "/opt/openshell/bin/project", + ), + None, + ) + .unwrap_err(); + + assert!(err.to_string().contains("OpenShell control path")); + } + + #[test] + fn workspace_probe_uses_pinned_image_without_workspace_or_network() { + let mut sandbox = test_sandbox("test-id", "test-name"); + let spec = sandbox.spec.get_or_insert_default(); + spec.workspace_validation_identity = Some( + openshell_core::proto::compute::v1::WorkspaceValidationIdentity { + run_as_user: "policy-user".into(), + run_as_group: "policy-group".into(), + discover_from_image_policy: false, + }, + ); + let probe = build_workspace_probe_spec( + &sandbox, + &test_config(), + &inspected_image("sha256:immutable", "app:staff", "/workspace/project"), + "openshell-test-probe", + ) + .unwrap() + .unwrap(); + + assert_eq!(probe["image"], "sha256:immutable"); + assert_eq!(probe["netns"]["nsmode"], "none"); + assert_eq!(probe["image_volume_mode"], "ignore"); + assert_eq!(probe["cap_drop"], serde_json::json!(["ALL"])); + assert_eq!(probe["cap_add"], serde_json::json!(["SETUID", "SETGID"])); + assert!(probe.get("volumes").is_none()); + assert!(probe.get("secrets").is_none()); + assert!(probe.get("env").is_none()); + assert_eq!( + probe["command"], + serde_json::json!([ + "probe-workspace", + "--workdir", + "/workspace/project", + "--oci-user", + "app:staff", + "--run-as-user", + "policy-user", + "--run-as-group", + "policy-group" + ]) ); } + #[test] + fn workspace_probe_discovers_image_policy_identity_when_requested() { + let mut sandbox = test_sandbox("test-id", "test-name"); + sandbox + .spec + .get_or_insert_default() + .workspace_validation_identity = Some( + openshell_core::proto::compute::v1::WorkspaceValidationIdentity { + discover_from_image_policy: true, + ..Default::default() + }, + ); + + let probe = build_workspace_probe_spec( + &sandbox, + &test_config(), + &inspected_image("sha256:immutable", "app:staff", "/home/app/project"), + "openshell-test-probe", + ) + .unwrap() + .unwrap(); + + assert_eq!( + probe["command"], + serde_json::json!([ + "probe-workspace", + "--workdir", + "/home/app/project", + "--oci-user", + "app:staff", + "--discover-policy-identity" + ]) + ); + } + + #[test] + fn workspace_probe_skips_sandbox_compatibility_fallback() { + let probe = build_workspace_probe_spec( + &test_sandbox("test-id", "test-name"), + &test_config(), + &inspected_image("sha256:immutable", "sandbox:sandbox", "/"), + "openshell-test-probe", + ) + .unwrap(); + assert!(probe.is_none()); + } + + #[test] + fn container_spec_reserves_resolved_workspace_root_but_allows_nested_mounts() { + let mut sandbox = test_sandbox("test-id", "test-name"); + sandbox.spec = Some(openshell_core::proto::compute::v1::DriverSandboxSpec { + template: Some(DriverSandboxTemplate::default()), + ..Default::default() + }); + sandbox + .spec + .as_mut() + .unwrap() + .template + .as_mut() + .unwrap() + .driver_config = Some(json_struct(serde_json::json!({ + "mounts": [{ + "type": "tmpfs", + "target": "/workspace" + }] + }))); + + let err = build_container_spec_for_image( + &sandbox, + &test_config(), + None, + None, + "registry.example/app:latest", + &inspected_image("sha256:immutable", "app:staff", "/workspace"), + None, + ) + .unwrap_err(); + assert!( + err.to_string() + .contains("reserved for the OpenShell workspace") + ); + + sandbox + .spec + .as_mut() + .unwrap() + .template + .as_mut() + .unwrap() + .driver_config = Some(json_struct(serde_json::json!({ + "mounts": [{ + "type": "tmpfs", + "target": "/workspace" + }] + }))); + let err = build_container_spec_for_image( + &sandbox, + &test_config(), + None, + None, + "registry.example/app:latest", + &inspected_image("sha256:immutable", "app:staff", "/workspace/project"), + None, + ) + .unwrap_err(); + assert!( + err.to_string() + .contains("reserved for the OpenShell workspace") + ); + + sandbox + .spec + .as_mut() + .unwrap() + .template + .as_mut() + .unwrap() + .driver_config = Some(json_struct(serde_json::json!({ + "mounts": [{ + "type": "tmpfs", + "target": "/workspace/cache" + }] + }))); + build_container_spec_for_image( + &sandbox, + &test_config(), + None, + None, + "registry.example/app:latest", + &inspected_image("sha256:immutable", "app:staff", "/workspace"), + None, + ) + .expect("nested workspace mounts remain supported"); + } + #[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 51c689fb29..00d62cfde6 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -54,6 +54,10 @@ pub struct PodmanComputeDriver { rootless: bool, /// Rootless network helper reported by Podman, such as `pasta`. rootless_network_cmd: String, + /// Serializes container image-volume attachment and removal. Rootless + /// Podman can otherwise detach a shared type=image mount while another + /// sandbox container is starting. + container_lifecycle_lock: Arc>, gpu_selector: Arc, gpu_inventory_refresh: Arc (CdiGpuInventory, bool) + Send + Sync>, } @@ -87,6 +91,15 @@ fn validated_container_name(sandbox: &DriverSandbox) -> Result String { + const SUFFIX: &str = "-workdir-probe"; + let keep = 255usize.saturating_sub(SUFFIX.len()); + format!( + "{}{SUFFIX}", + &container_name[..container_name.len().min(keep)] + ) +} + fn podman_volume_is_bind_backed(volume: &VolumeInspect) -> bool { (volume.driver.is_empty() || volume.driver == "local") && volume.options.get("o").is_some_and(|options| { @@ -222,6 +235,36 @@ fn podman_gpu_selection_error(err: CdiGpuSelectionError) -> ComputeDriverError { ComputeDriverError::Precondition(err.to_string()) } +const WORKSPACE_IDENTITY_MARKER: &str = "OPENSHELL_WORKSPACE_IDENTITY="; + +fn parse_workspace_identity(logs: &str) -> Result { + logs.match_indices(WORKSPACE_IDENTITY_MARKER) + .filter_map(|(start, _)| { + logs[start + WORKSPACE_IDENTITY_MARKER.len()..] + .split(|character: char| character.is_control()) + .next() + }) + .find_map(|identity| serde_json::from_str::(identity).ok()) + .ok_or_else(|| { + ComputeDriverError::Precondition( + "OCI WorkingDir validation probe did not report its process identity".to_string(), + ) + }) +} + +fn sanitized_probe_diagnostic(logs: &str) -> String { + let sanitized = logs + .chars() + .filter(|character| !character.is_control() || matches!(character, '\n' | '\t')) + .collect::(); + let sanitized = sanitized.trim(); + if sanitized.is_empty() { + "no probe diagnostic was emitted".to_string() + } else { + sanitized.chars().take(2048).collect() + } +} + /// Resolve the socket to connect to: explicit configuration wins, otherwise /// fall back to `detect`. Returns an error if neither resolves. /// @@ -243,6 +286,80 @@ fn resolve_socket_path( } impl PodmanComputeDriver { + async fn validate_image_workspace( + &self, + sandbox: &DriverSandbox, + container_name: &str, + inspected_image: &crate::client::ImageInspect, + ) -> Result, ComputeDriverError> { + let probe_name = workspace_probe_name(container_name); + crate::client::validate_name(&probe_name) + .map_err(|error| ComputeDriverError::Precondition(error.to_string()))?; + let Some(spec) = container::build_workspace_probe_spec( + sandbox, + &self.config, + inspected_image, + &probe_name, + )? + else { + return Ok(None); + }; + + let start = { + let _lifecycle_guard = self.container_lifecycle_lock.lock().await; + self.client + .create_container(&spec) + .await + .map_err(ComputeDriverError::from)?; + self.client + .start_container(&probe_name) + .await + .map_err(ComputeDriverError::from) + }; + let validation = async { + start?; + let exit_code = self + .client + .wait_container(&probe_name) + .await + .map_err(ComputeDriverError::from)?; + let logs = self.client.container_logs(&probe_name).await; + if exit_code == 0 { + let logs = logs.map_err(ComputeDriverError::from)?; + parse_workspace_identity(&logs).map(Some) + } else { + let diagnostic = logs.map_or_else( + |error| format!("unable to read probe diagnostic: {error}"), + |logs| sanitized_probe_diagnostic(&logs), + ); + Err(ComputeDriverError::Precondition(format!( + "OCI WorkingDir validation failed for image '{}' (probe exited with code {exit_code}): {diagnostic}", + inspected_image.id, + ))) + } + } + .await; + let cleanup = { + let _lifecycle_guard = self.container_lifecycle_lock.lock().await; + self.client + .remove_container(&probe_name, self.config.stop_timeout_secs) + .await + }; + match (validation, cleanup) { + (Ok(identity), Ok(())) => Ok(identity), + (Ok(_), Err(error)) => Err(ComputeDriverError::from(error)), + (Err(error), Ok(())) => Err(error), + (Err(error), Err(cleanup_error)) => { + warn!( + probe = %probe_name, + %cleanup_error, + "Failed to remove workspace validation probe" + ); + Err(error) + } + } + } + /// Create a new driver, verifying the Podman socket is reachable. pub async fn new(mut config: PodmanComputeConfig) -> Result { const MAX_PING_RETRIES: u32 = 5; @@ -391,6 +508,7 @@ impl PodmanComputeDriver { network_gateway_ip, rootless, rootless_network_cmd, + container_lifecycle_lock: Arc::new(tokio::sync::Mutex::new(())), gpu_selector: Arc::new(CdiGpuDefaultSelector::new( gpu_inventory, allow_all_default_gpu, @@ -692,11 +810,9 @@ impl PodmanComputeDriver { "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()); - + let workspace_identity = self + .validate_image_workspace(sandbox, &name, &inspected_image) + .await?; for image in container::podman_driver_image_mount_sources(sandbox, self.config.enable_bind_mounts) .map_err(ComputeDriverError::Precondition)? @@ -761,8 +877,8 @@ impl PodmanComputeDriver { token_secret_name.as_deref(), gpu_devices.as_deref(), image, - &inspected_image.id, - image_user, + &inspected_image, + workspace_identity.as_deref(), ) { Ok(spec) => spec, Err(e) => { @@ -770,8 +886,31 @@ impl PodmanComputeDriver { return Err(e); } }; - match self.client.create_container(&spec).await { - Ok(_) => {} + // Podman implements the supervisor as a shared type=image mount. + // Keep attach/start atomic with respect to another sandbox's removal. + let lifecycle_result = { + let _lifecycle_guard = self.container_lifecycle_lock.lock().await; + match self.client.create_container(&spec).await { + Ok(_) => match self.client.start_container(&name).await { + Ok(()) => Ok(()), + Err(e) => { + warn!( + sandbox_name = %sandbox.name, + error = %e, + "Failed to start container; cleaning up" + ); + let _ = self + .client + .remove_container(&name, self.config.stop_timeout_secs) + .await; + Err(e) + } + }, + Err(e) => Err(e), + } + }; + match lifecycle_result { + Ok(()) => {} Err(PodmanApiError::Conflict(_)) => { // Clean up the volume we just created. It is keyed by *this* // sandbox's ID, not the conflicting container's ID (which @@ -786,21 +925,6 @@ impl PodmanComputeDriver { } } - // 5. Start container. - if let Err(e) = self.client.start_container(&name).await { - warn!( - sandbox_name = %sandbox.name, - error = %e, - "Failed to start container; cleaning up" - ); - let _ = self - .client - .remove_container(&name, self.config.stop_timeout_secs) - .await; - cleanup_created().await; - return Err(ComputeDriverError::from(e)); - } - info!( sandbox_id = %sandbox.id, sandbox_name = %sandbox.name, @@ -862,17 +986,20 @@ impl PodmanComputeDriver { }; info!(sandbox_id = %sandbox_id, container = %container_id, "Deleting sandbox container"); - // Keep stop, timeout, and removal in one Podman operation. Splitting - // stop and remove can race with another container starting an image - // mount when the stop reaches its timeout. - let container_existed = match self - .client - .remove_container(&container_id, self.config.stop_timeout_secs) - .await - { - Ok(()) => true, - Err(PodmanApiError::NotFound(_)) => false, - Err(e) => return Err(ComputeDriverError::from(e)), + // Keep stop, timeout, and removal in one Podman operation. Removing a + // container detaches its type=image mounts, so do not overlap it with + // another sandbox container's create/start window. + let container_existed = { + let _lifecycle_guard = self.container_lifecycle_lock.lock().await; + match self + .client + .remove_container(&container_id, self.config.stop_timeout_secs) + .await + { + Ok(()) => true, + Err(PodmanApiError::NotFound(_)) => false, + Err(e) => return Err(ComputeDriverError::from(e)), + } }; // Remove workspace volume. @@ -1009,6 +1136,7 @@ impl PodmanComputeDriver { network_gateway_ip: None, rootless: false, rootless_network_cmd: String::new(), + container_lifecycle_lock: Arc::new(tokio::sync::Mutex::new(())), gpu_selector: Arc::new(CdiGpuDefaultSelector::new( gpu_inventory, allow_all_default_gpu, @@ -2058,6 +2186,90 @@ mod tests { } } + #[tokio::test] + async fn workspace_probe_waits_for_success_and_always_removes_container() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "workspace-probe-success", + vec![ + StubResponse::new(StatusCode::CREATED, "{}"), + StubResponse::new(StatusCode::NO_CONTENT, ""), + StubResponse::new(StatusCode::OK, r#"{"StatusCode":0}"#), + StubResponse::new( + StatusCode::OK, + "OPENSHELL_WORKSPACE_IDENTITY=\"1234:1235:\"\n", + ), + StubResponse::new(StatusCode::NO_CONTENT, ""), + ], + ); + let driver = test_driver(socket_path.clone()); + let mut sandbox = plain_sandbox("sandbox-probe", "demo"); + sandbox.spec = Some(DriverSandboxSpec::default()); + let image = crate::client::ImageInspect { + id: "sha256:immutable".into(), + config: Some(crate::client::ImageConfig { + user: "1234:1235".into(), + working_dir: "/workspace".into(), + }), + }; + let name = validated_container_name(&sandbox).unwrap(); + + let identity = driver + .validate_image_workspace(&sandbox, &name, &image) + .await + .expect("successful probe should pass"); + assert_eq!(identity.as_deref(), Some("1234:1235:")); + + handle.await.expect("stub task should finish"); + let requests = request_log.lock().unwrap(); + assert_eq!(requests.len(), 5); + assert!(requests[0].contains("/libpod/containers/create")); + assert!(requests[1].contains("/start")); + assert!(requests[2].contains("/wait?condition=exited")); + assert!(requests[3].contains("/logs?")); + assert!(requests[4].starts_with("DELETE ")); + let _ = fs::remove_file(socket_path); + } + + #[tokio::test] + async fn workspace_probe_removes_container_after_validation_failure() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "workspace-probe-failure", + vec![ + StubResponse::new(StatusCode::CREATED, "{}"), + StubResponse::new(StatusCode::NO_CONTENT, ""), + StubResponse::new(StatusCode::OK, r#"{"StatusCode":1}"#), + StubResponse::new( + StatusCode::OK, + "image workspace path component '/workspace' is not writable and traversable\n", + ), + StubResponse::new(StatusCode::NO_CONTENT, ""), + ], + ); + let driver = test_driver(socket_path.clone()); + let mut sandbox = plain_sandbox("sandbox-probe-fail", "demo"); + sandbox.spec = Some(DriverSandboxSpec::default()); + let image = crate::client::ImageInspect { + id: "sha256:immutable".into(), + config: Some(crate::client::ImageConfig { + user: "1234:1235".into(), + working_dir: "/workspace".into(), + }), + }; + let name = validated_container_name(&sandbox).unwrap(); + + let error = driver + .validate_image_workspace(&sandbox, &name, &image) + .await + .unwrap_err(); + assert!(error.to_string().contains("exited with code 1")); + assert!(error.to_string().contains("not writable and traversable")); + + handle.await.expect("stub task should finish"); + let requests = request_log.lock().unwrap(); + assert!(requests.last().unwrap().starts_with("DELETE ")); + let _ = fs::remove_file(socket_path); + } + fn secret_delete_request(sandbox_id: &str) -> String { format!( "DELETE {}", @@ -2245,4 +2457,29 @@ mod tests { ); let _ = fs::remove_file(socket_path); } + + #[test] + fn workspace_identity_parser_accepts_plain_and_multiplexed_logs() { + assert_eq!( + parse_workspace_identity( + "probe startup\nOPENSHELL_WORKSPACE_IDENTITY=\"1234:1235:7,8\"\n" + ) + .unwrap(), + "1234:1235:7,8" + ); + assert_eq!( + parse_workspace_identity( + "\u{1}\0\0\0\0\0\0.OPENSHELL_WORKSPACE_IDENTITY=\"1234:1235:\"\n" + ) + .unwrap(), + "1234:1235:" + ); + assert!( + parse_workspace_identity( + "OPENSHELL_WORKSPACE_IDENTITY=not-json\n\ + OPENSHELL_WORKSPACE_IDENTITY={\"uid\":1234}\n" + ) + .is_err() + ); + } } diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 956fed927c..39fa050b27 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -189,8 +189,8 @@ pub async fn run_sandbox( // 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 fills only - // omitted policy fields from OCI Config.User. + // OpenShift retain their authoritative numeric pair; Docker and Podman + // fill only omitted policy fields from OCI Config.User. #[cfg(unix)] let (resolved_process_identity, workspace) = { let driver_identity = openshell_supervisor_process::identity::DriverIdentity::from_env()?; @@ -202,6 +202,24 @@ pub async fn run_sandbox( &mut policy, &driver_identity, )?; + if matches!( + &driver_identity, + openshell_supervisor_process::identity::DriverIdentity::OciUser { .. } + ) && std::env::var_os(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY) + .is_some_and(|value| !value.is_empty()) + { + let expected = std::env::var(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY) + .map_err(|_| miette::miette!("Podman workspace identity attestation is missing"))?; + let actual = + openshell_supervisor_process::process::resolved_workspace_identity_attestation( + &policy, resolved, + )?; + if expected != actual { + return Err(miette::miette!( + "process identity changed after OCI workspace validation" + )); + } + } ( resolved, openshell_supervisor_process::process::ResolvedWorkspace::new( @@ -2156,6 +2174,18 @@ fn discover_policy_from_disk_or_default() -> openshell_core::proto::SandboxPolic discover_policy_from_path(primary) } +/// Discover only the process identity portion of the immutable image policy. +/// +/// The Podman workspace probe uses this when no policy existed at create time, +/// matching the final supervisor's disk-policy discovery before the gateway +/// backfills that policy. +pub fn discover_process_policy_from_disk_or_default() -> openshell_core::policy::ProcessPolicy { + discover_policy_from_disk_or_default() + .process + .map(Into::into) + .unwrap_or_default() +} + /// Try to read a sandbox policy YAML from `path`, falling back to the /// hardcoded restrictive default if the file is missing or invalid. fn discover_policy_from_path(path: &std::path::Path) -> openshell_core::proto::SandboxPolicy { diff --git a/crates/openshell-sandbox/src/main.rs b/crates/openshell-sandbox/src/main.rs index 98af7f9ea9..8297c8145e 100644 --- a/crates/openshell-sandbox/src/main.rs +++ b/crates/openshell-sandbox/src/main.rs @@ -33,6 +33,7 @@ const COPY_SELF_SUBCOMMAND: &str = "copy-self"; /// to confirm the cross-sandbox IDOR guard fires. const DEBUG_RPC_SUBCOMMAND: &str = "debug-rpc"; const VALIDATE_WORKSPACE_SUBCOMMAND: &str = "validate-workspace"; +const PROBE_WORKSPACE_SUBCOMMAND: &str = "probe-workspace"; /// Default `--mode` value: run both supervisor leaves in a single binary. const DEFAULT_MODE: &str = "network,process"; @@ -272,7 +273,76 @@ fn validate_workspace(args: &[String]) -> Result<()> { #[cfg(not(target_os = "linux"))] fn validate_workspace(_args: &[String]) -> Result<()> { Err(miette::miette!( - "workspace validation is only supported on Unix" + "workspace validation is only supported on Linux" + )) +} + +/// Internal one-shot command used by Podman to validate the immutable image +/// before the final workspace volume covers its OCI workdir. +#[derive(Parser, Debug)] +#[command(name = "probe-workspace", hide = true)] +struct ProbeWorkspaceArgs { + #[arg(long)] + workdir: String, + #[arg(long)] + oci_user: String, + #[arg(long)] + run_as_user: Option, + #[arg(long)] + run_as_group: Option, + #[arg(long)] + discover_policy_identity: bool, +} + +#[cfg(unix)] +fn probe_workspace(args: &[String]) -> Result<()> { + use openshell_core::policy::{ + FilesystemPolicy, LandlockPolicy, NetworkPolicy, ProcessPolicy, SandboxPolicy, + }; + use openshell_supervisor_process::identity::{DriverIdentity, resolve_process_identity}; + + let args = ProbeWorkspaceArgs::try_parse_from( + std::iter::once(PROBE_WORKSPACE_SUBCOMMAND.to_string()).chain(args.iter().cloned()), + ) + .into_diagnostic()?; + let process = if args.discover_policy_identity { + openshell_sandbox::discover_process_policy_from_disk_or_default() + } else { + ProcessPolicy { + run_as_user: args.run_as_user, + run_as_group: args.run_as_group, + } + }; + let mut policy = SandboxPolicy { + version: 0, + filesystem: FilesystemPolicy::default(), + network: NetworkPolicy::default(), + landlock: LandlockPolicy::default(), + process, + }; + let resolved_identity = resolve_process_identity( + &mut policy, + &DriverIdentity::OciUser { + declaration: args.oci_user, + }, + )?; + let identity = + openshell_supervisor_process::process::validate_oci_workspace_as_process_identity( + &policy, + resolved_identity, + Path::new(&args.workdir), + )?; + println!( + "OPENSHELL_WORKSPACE_IDENTITY={}", + serde_json::to_string(&identity).into_diagnostic()? + ); + Ok(()) +} + +#[cfg(not(unix))] +fn probe_workspace(_args: &[String]) -> Result<()> { + Err(miette::miette!( + "workspace probing is only supported on Unix" )) } @@ -527,6 +597,9 @@ fn main() -> Result<()> { if raw_args.get(1).map(String::as_str) == Some(VALIDATE_WORKSPACE_SUBCOMMAND) { return validate_workspace(&raw_args[2..]); } + if raw_args.get(1).map(String::as_str) == Some(PROBE_WORKSPACE_SUBCOMMAND) { + return probe_workspace(&raw_args[2..]); + } let args = Args::parse(); @@ -721,6 +794,36 @@ mod tests { validate_workspace(&args).expect("current identity should retain workspace authority"); } + #[cfg(unix)] + #[test] + fn workspace_probe_subcommand_resolves_final_policy_identity() { + let uid = nix::unistd::geteuid().as_raw(); + let gid = nix::unistd::getegid().as_raw(); + if uid < 1000 || gid < 1000 { + // Process policies intentionally reject system identities. Local + // macOS users commonly have IDs below 1000, so this test cannot + // exercise the credential transition for those accounts. + return; + } + let dir = tempfile::tempdir_in("/tmp").unwrap(); + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o711)).unwrap(); + let root = dir.path().canonicalize().unwrap().join("workspace"); + std::fs::create_dir(&root).unwrap(); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o777)).unwrap(); + let args = vec![ + "--workdir".to_string(), + root.display().to_string(), + "--oci-user".to_string(), + "unused".to_string(), + "--run-as-user".to_string(), + uid.to_string(), + "--run-as-group".to_string(), + gid.to_string(), + ]; + + probe_workspace(&args).expect("current identity should retain workspace authority"); + } + /// Drives `copy_self`'s file-copy logic against an arbitrary source path /// so tests don't depend on `current_exe()`. fn copy_executable(src: &Path, dest: &Path) -> Result<()> { diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index a1c33e49ff..b2d70be671 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -2515,6 +2515,17 @@ fn driver_sandbox_spec_from_public( spec: &SandboxSpec, driver_name: &str, ) -> Result> { + let workspace_validation_identity = matches!(driver_name, "docker" | "podman").then(|| { + let process = spec + .policy + .as_ref() + .and_then(|policy| policy.process.as_ref()); + openshell_core::proto::compute::v1::WorkspaceValidationIdentity { + run_as_user: process.map_or_else(String::new, |process| process.run_as_user.clone()), + run_as_group: process.map_or_else(String::new, |process| process.run_as_group.clone()), + discover_from_image_policy: spec.policy.is_none(), + } + }); Ok(DriverSandboxSpec { log_level: spec.log_level.clone(), environment: spec.environment.clone(), @@ -2532,6 +2543,7 @@ fn driver_sandbox_spec_from_public( } }), sandbox_token: String::new(), + workspace_validation_identity, }) } @@ -3283,6 +3295,36 @@ mod tests { assert_eq!(gpu.count, Some(2)); } + #[test] + fn driver_sandbox_spec_projects_process_identity_for_local_image_probe() { + let public = SandboxSpec { + policy: Some(openshell_core::proto::SandboxPolicy { + process: Some(openshell_core::proto::ProcessPolicy { + run_as_user: "app".into(), + run_as_group: "staff".into(), + }), + ..Default::default() + }), + ..Default::default() + }; + + let driver = + driver_sandbox_spec_from_public(&public, "podman").expect("driver spec should map"); + let identity = driver.workspace_validation_identity.unwrap(); + assert_eq!(identity.run_as_user, "app"); + assert_eq!(identity.run_as_group, "staff"); + assert!(!identity.discover_from_image_policy); + + let without_policy = + driver_sandbox_spec_from_public(&SandboxSpec::default(), "podman").unwrap(); + assert!( + without_policy + .workspace_validation_identity + .unwrap() + .discover_from_image_policy + ); + } + #[test] fn select_driver_config_forwards_only_matching_driver_block() { let config = prost_types::Struct { diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 659fe3dc06..ce6c1043ef 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -142,6 +142,7 @@ pub(crate) fn prepare_child_sandbox( const SUPERVISOR_ONLY_ENV_VARS: &[&str] = &[ openshell_core::sandbox_env::OCI_IMAGE_USER, + openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY, openshell_core::sandbox_env::SANDBOX_UID, openshell_core::sandbox_env::SANDBOX_GID, openshell_core::sandbox_env::SANDBOX_TOKEN, @@ -1394,6 +1395,101 @@ fn validate_oci_workspace_in_subprocess( )) } +/// Drop a one-shot workspace probe to the completed sandbox identity, validate +/// using the kernel's real path-access checks, and return a normalized identity +/// attestation for the final supervisor. +#[cfg(unix)] +pub fn validate_oci_workspace_as_process_identity( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, + workdir: &Path, +) -> Result { + validate_sandbox_user_with_identity(policy, resolved_identity)?; + validate_sandbox_group_with_identity(policy, resolved_identity)?; + let (uid, gid, mut supplementary_gids) = + resolve_filesystem_identity(policy, resolved_identity)?; + let uid = uid.ok_or_else(|| miette::miette!("workspace probe UID is unresolved"))?; + let gid = gid.ok_or_else(|| miette::miette!("workspace probe GID is unresolved"))?; + supplementary_gids.sort_unstable_by_key(|group| group.as_raw()); + supplementary_gids.dedup(); + + if nix::unistd::geteuid().is_root() { + #[cfg(not(any( + target_os = "macos", + target_os = "ios", + target_os = "haiku", + target_os = "redox" + )))] + nix::unistd::setgroups(&supplementary_gids).into_diagnostic()?; + nix::unistd::setgid(gid).into_diagnostic()?; + nix::unistd::setuid(uid).into_diagnostic()?; + } + + let effective_identity = (nix::unistd::geteuid(), nix::unistd::getegid()); + if effective_identity != (uid, gid) { + return Err(miette::miette!( + "workspace probe privilege drop failed: expected {uid}:{gid}, got {}:{}", + effective_identity.0, + effective_identity.1 + )); + } + #[cfg(target_os = "linux")] + validate_oci_workspace_as_effective_identity(workdir)?; + #[cfg(not(target_os = "linux"))] + validate_oci_workspace( + workdir, + Some(effective_identity.0), + Some(effective_identity.1), + &supplementary_gids, + )?; + Ok(workspace_identity_attestation( + Some(uid), + Some(gid), + &supplementary_gids, + )) +} + +/// Normalize a completed process identity for comparison with the immutable +/// image probe. +#[cfg(unix)] +pub fn workspace_identity_attestation( + uid: Option, + gid: Option, + supplementary_gids: &[Gid], +) -> String { + let mut groups = supplementary_gids + .iter() + .map(|group| group.as_raw()) + .collect::>(); + groups.sort_unstable(); + groups.dedup(); + format!( + "{}:{}:{}", + uid.map_or_else(String::new, |value| value.as_raw().to_string()), + gid.map_or_else(String::new, |value| value.as_raw().to_string()), + groups + .iter() + .map(u32::to_string) + .collect::>() + .join(",") + ) +} + +/// Resolve and normalize the completed policy/driver identity without changing +/// process credentials. +#[cfg(unix)] +pub fn resolved_workspace_identity_attestation( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, +) -> Result { + let (uid, gid, supplementary_gids) = resolve_filesystem_identity(policy, resolved_identity)?; + Ok(workspace_identity_attestation( + uid, + gid, + &supplementary_gids, + )) +} + #[cfg(unix)] fn validate_workspace_component( path: &Path, @@ -1427,6 +1523,7 @@ fn validate_workspace_component( path.display() )); } + reject_special_workspace_filesystem(path)?; let required = if is_workspace { 0o3 } else { 0o1 }; if !identity_has_permissions(&metadata, uid, gid, supplementary_gids, required) { let requirement = if is_workspace { @@ -1450,6 +1547,7 @@ pub fn validate_oci_workspace_as_effective_identity(root: &Path) -> Result<()> { let open_flags = OFlags::PATH | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC; let mut current_path = PathBuf::from("/"); let mut current_fd = rustix::fs::open("/", open_flags, Mode::empty()).into_diagnostic()?; + reject_special_workspace_filesystem_fd(¤t_fd, ¤t_path)?; rustix::fs::accessat( ¤t_fd, ".", @@ -1464,6 +1562,7 @@ pub fn validate_oci_workspace_as_effective_identity(root: &Path) -> Result<()> { })?; let last_component = components.len().saturating_sub(1); + for (index, component) in components.into_iter().enumerate() { current_path.push(&component); let stat = rustix::fs::statat(¤t_fd, &component, AtFlags::SYMLINK_NOFOLLOW).map_err( @@ -1509,22 +1608,71 @@ pub fn validate_oci_workspace_as_effective_identity(root: &Path) -> Result<()> { ) })?; - let next_fd = rustix::fs::openat(¤t_fd, &component, open_flags, Mode::empty()) + current_fd = rustix::fs::openat(¤t_fd, &component, open_flags, Mode::empty()) .map_err(|error| { miette::miette!( "failed to open image workspace path component '{}': {error}", current_path.display() ) })?; + reject_special_workspace_filesystem_fd(¤t_fd, ¤t_path)?; if is_workspace { - validate_effective_workspace_write(&next_fd, ¤t_path)?; + validate_effective_workspace_write(¤t_fd, ¤t_path)?; } - current_fd = next_fd; } Ok(()) } +#[cfg(target_os = "linux")] +fn reject_special_workspace_filesystem_fd(fd: &impl std::os::fd::AsFd, path: &Path) -> Result<()> { + let fs = rustix::fs::fstatfs(fd).into_diagnostic()?; + #[allow(clippy::cast_sign_loss)] + reject_special_workspace_filesystem_type(path, fs.f_type as u64) +} + +#[cfg(target_os = "linux")] +fn reject_special_workspace_filesystem(path: &Path) -> Result<()> { + // Linux filesystem magic values for virtual/kernel-managed filesystems. + // The decision is based on the mounted filesystem, not its conventional + // distro path, so renamed or unusually mounted control filesystems remain + // protected without rejecting ordinary application directories. + let fs = rustix::fs::statfs(path).into_diagnostic()?; + #[allow(clippy::cast_sign_loss)] + let filesystem_type = fs.f_type as u64; + reject_special_workspace_filesystem_type(path, filesystem_type) +} + +#[cfg(target_os = "linux")] +fn reject_special_workspace_filesystem_type(path: &Path, filesystem_type: u64) -> Result<()> { + const SPECIAL_FILESYSTEMS: &[u64] = &[ + 0x0000_1cd1, // devpts + 0x0000_9fa0, // proc + 0x0102_1994, // tmpfs (including the container /dev tree) + 0x1980_0202, // mqueue + 0x0027_e0eb, // cgroup + 0x4249_4e4d, // bpf + 0x6265_6572, // sysfs + 0x6367_7270, // cgroup2 + 0x6462_6720, // debugfs + 0x7363_6673, // securityfs + 0x7472_6163, // tracefs + ]; + if SPECIAL_FILESYSTEMS.contains(&filesystem_type) { + return Err(miette::miette!( + "workspace path component '{}' is on a kernel-managed filesystem (type {filesystem_type:#x})", + path.display() + )); + } + Ok(()) +} + +#[cfg(not(target_os = "linux"))] +#[allow(clippy::unnecessary_wraps)] +fn reject_special_workspace_filesystem(_path: &Path) -> Result<()> { + Ok(()) +} + #[cfg(target_os = "linux")] fn validate_effective_workspace_write(fd: &impl std::os::fd::AsFd, path: &Path) -> Result<()> { use rustix::fs::{AtFlags, Mode, OFlags}; @@ -1831,17 +1979,20 @@ pub fn prepare_filesystem_with_identity( let (uid, gid, supplementary_gids) = resolve_filesystem_identity(policy, resolved_identity)?; - // Docker owns workspace resolution and must make the selected root usable - // by the final effective identity, including when both policy identity - // fields were explicit. Validate it before processing any user-authored - // read-write paths so an unsafe image path fails first. Other drivers - // retain their preparation. + // Docker and Podman own workspace resolution and must make the selected + // root usable by the final effective identity, including when both policy + // identity fields were explicit. Validate it before processing any + // user-authored read-write paths so an unsafe image path fails first. + // Other drivers retain their preparation. if prepare_workspace { let workspace = workdir.ok_or_else(|| { miette::miette!("local container driver did not supply a workspace workdir") })?; let workspace = Path::new(workspace); - if workspace == Path::new(openshell_core::driver_mounts::DEFAULT_WORKSPACE_ROOT) { + if workspace == Path::new(openshell_core::driver_mounts::DEFAULT_WORKSPACE_ROOT) + || std::env::var_os(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY) + .is_some_and(|value| !value.is_empty()) + { info!(path = %workspace.display(), ?uid, ?gid, "Preparing managed workspace"); prepare_oci_workspace(workspace, uid, gid, &supplementary_gids)?; } else { @@ -1867,8 +2018,8 @@ pub fn prepare_filesystem_with_identity( } // Retain the existing Kubernetes/OpenShift behavior for driver-injected - // numeric identities. Docker clears this variable and does not receive - // identity-specific workspace preparation. + // 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"); if sandbox_home.exists() { @@ -3008,6 +3159,23 @@ mod tests { .expect("supplementary group already has write and traverse authority"); } + #[cfg(unix)] + #[test] + fn workspace_identity_attestation_sorts_and_deduplicates_groups() { + assert_eq!( + workspace_identity_attestation( + Some(Uid::from_raw(1000)), + Some(Gid::from_raw(1001)), + &[ + Gid::from_raw(1003), + Gid::from_raw(1002), + Gid::from_raw(1003), + ], + ), + "1000:1001:1002,1003" + ); + } + #[cfg(unix)] #[test] fn validate_oci_workspace_rejects_unwritable_directory() { @@ -3157,6 +3325,25 @@ mod tests { } } + #[cfg(target_os = "linux")] + #[test] + fn effective_identity_validation_uses_kernel_access_checks() { + if nix::unistd::geteuid().is_root() { + return; + } + + let dir = tempfile::tempdir_in("/tmp").unwrap(); + let root = dir.path().canonicalize().unwrap().join("project"); + std::fs::create_dir(&root).unwrap(); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o700)).unwrap(); + validate_oci_workspace_as_effective_identity(&root) + .expect("current identity can write and enter its directory"); + + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o500)).unwrap(); + let error = validate_oci_workspace_as_effective_identity(&root).unwrap_err(); + assert!(error.to_string().contains("not writable")); + } + #[cfg(unix)] #[test] fn validate_oci_workspace_rejects_restrictive_parent() { diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index cb9115d9ea..df7c5ca03b 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -699,8 +699,8 @@ impl Default for PtyRequest { /// (or defaults to `/home/{user}`). /// /// For numeric UIDs, there is no passwd entry, so the default remains -/// `("{uid}", "/sandbox")`. Docker replaces that default with its resolved -/// image workspace. +/// `("{uid}", "/sandbox")`. Docker and Podman replace that default with their +/// resolved image workspace. fn session_user_and_home(policy: &SandboxPolicy, workdir_home: Option<&str>) -> (String, String) { let (user, default_home) = match policy.process.run_as_user.as_deref() { Some(user) if !user.is_empty() => { diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 2132f3360e..593fdee8cd 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -251,9 +251,10 @@ Podman mount schema: Podman `volume` and `image` mounts do not support `subpath` in OpenShell driver config, and OpenShell rejects `subpath` for those mount types. OpenShell rejects mount `source` and `target` values with surrounding whitespace. OpenShell also -rejects mount targets that replace the workspace root, container root, supervisor -files, `/etc/openshell`, `/etc/openshell-tls`, authentication material, or -network namespace paths. These checks do not make host bind mounts safe. +rejects mount targets that replace or contain the workspace root, target the +container root, or contain or are contained by concrete OpenShell control +targets such as the supervisor mount, TLS and token files, runtime socket, or +network namespace mount. These checks do not make host bind mounts safe. ## MicroVM Driver @@ -444,7 +445,7 @@ 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`. -Docker also inspects OCI `WorkingDir`. An absolute value becomes the +Docker and Podman also inspect OCI `WorkingDir`. An absolute value becomes the agent workspace; an empty, root (`/`), or explicit `/sandbox` value uses the managed `/sandbox` compatibility workspace. OpenShell creates and owns that compatibility workspace. Any other workdir must @@ -457,10 +458,17 @@ workdirs that overlap the OCI runtime namespaces under `/proc`, `/sys`, or `/dev`, and rejects overlap with actual OpenShell control paths. Docker checks the original image filesystem in the final supervisor and rejects image `VOLUME` declarations that would mask the workdir or one of its parents before -validation. The resolved workspace is the cwd and `HOME` for direct and SSH -children. The supervisor itself starts from `/`, so a missing or invalid -workspace is handled during readiness instead of preventing the container -runtime from starting it. +validation. Podman checks the same +pinned image ID in a temporary networkless probe without the workspace volume, +token, or TLS secrets. The probe retains only `SETUID` and `SETGID`, drops to +the completed process identity, and uses the kernel to validate access. The +final supervisor must match the probe's normalized identity attestation, +including when the image supplies the default process policy. Podman captures +a bounded, sanitized failure diagnostic, removes the probe, and only then +creates the final volume-backed sandbox. The resolved workspace is the cwd and +`HOME` for direct and SSH children. The supervisor itself starts from `/`, so a +missing or invalid workspace is handled during readiness instead of preventing +the container runtime from starting it. Sandbox creation fails before readiness if a required `USER` component is missing, malformed, unknown, ambiguous, or resolves to UID/GID 0. An image @@ -491,9 +499,9 @@ 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. For Docker, declare an absolute OCI `WORKDIR` to select the workspace. -Images with no working directory, `WORKDIR /`, or `WORKDIR /sandbox` use -OpenShell's managed `/sandbox` compatibility workspace. For any other Docker -path, create the directory in the image and grant the final process identity -write and execute permission in the Dockerfile. Podman, Kubernetes/OpenShift, -and VM sandboxes continue to use `/sandbox`. +does not. Declare an absolute OCI `WORKDIR` to select the workspace. Images with +no working directory, `WORKDIR /`, or `WORKDIR /sandbox` use OpenShell's +managed `/sandbox` compatibility workspace. +For any other path, create the directory in the image and grant the final +process identity write and execute permission in the Dockerfile. +Kubernetes/OpenShift and VM sandboxes continue to use `/sandbox`. diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index 5652a0011e..7b2a3bd759 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -56,6 +56,9 @@ RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ && useradd -m -u 3234 -g appstaff app WORKDIR /workspace/project +# Image metadata must not be able to forge the driver's successful-validation +# attestation. +ENV OPENSHELL_OCI_WORKSPACE_IDENTITY=3234:3235: USER app CMD ["sleep", "infinity"] "#; diff --git a/e2e/rust/tests/driver_config_volume.rs b/e2e/rust/tests/driver_config_volume.rs index 0702a4637d..2a91f9ce1d 100644 --- a/e2e/rust/tests/driver_config_volume.rs +++ b/e2e/rust/tests/driver_config_volume.rs @@ -25,9 +25,9 @@ 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")] +#[cfg(any(feature = "e2e-docker", feature = "e2e-podman"))] const OCI_VOLUME_TARGET: &str = "/workspace/project/e2e-volume"; -#[cfg(feature = "e2e-docker")] +#[cfg(any(feature = "e2e-docker", feature = "e2e-podman"))] 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 \ @@ -169,12 +169,12 @@ async fn sandbox_mounts_existing_driver_config_volume() { } #[tokio::test] -#[cfg(feature = "e2e-docker")] +#[cfg(any(feature = "e2e-docker", feature = "e2e-podman"))] 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!( - driver == "docker", - "OCI workspace mount e2e requires docker, got {driver}" + matches!(driver.as_str(), "docker" | "podman"), + "OCI workspace mount e2e requires docker or podman, got {driver}" ); let volume = VolumeGuard::create(&driver) @@ -342,7 +342,7 @@ async fn verify_volume(volume: &VolumeGuard) -> Result<(), String> { Ok(()) } -#[cfg(feature = "e2e-docker")] +#[cfg(any(feature = "e2e-docker", feature = "e2e-podman"))] async fn verify_volume_ownership(volume: &VolumeGuard) -> Result<(), String> { let output = run_volume_container( volume, diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs index e30516bf09..8dcc25155c 100644 --- a/e2e/rust/tests/podman_oci_identity.rs +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -3,14 +3,14 @@ #![cfg(feature = "e2e-podman")] -//! Podman-specific E2E coverage for OCI identity inspection and immutable-image -//! launch. +//! Podman-specific E2E coverage for OCI identity/workspace inspection, +//! workspace-volume copy-up, 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. +//! sandbox from its mutable tag, and verifies the child identity, workspace, +//! copied image content, and 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; @@ -54,7 +54,16 @@ impl ImageGuard { let containerfile = context.path().join("Containerfile"); std::fs::write( &containerfile, - format!("FROM {BASE_IMAGE}\nUSER {OCI_UID}:{OCI_GID}\n"), + format!( + "FROM {BASE_IMAGE}\n\ + USER 0:0\n\ + RUN mkdir -p /home/app/project && \ + chown {OCI_UID}:{OCI_GID} /home/app /home/app/project && \ + chmod 0700 /home/app\n\ + WORKDIR /home/app/project\n\ + RUN printf root-owned > root-owned.txt && chown {OCI_UID}:{OCI_GID} .\n\ + USER {OCI_UID}:{OCI_GID}\n" + ), ) .map_err(|err| format!("write Containerfile: {err}"))?; @@ -164,7 +173,7 @@ fn normalized_image_id(image_id: &str) -> &str { } #[tokio::test] -async fn podman_uses_oci_identity_and_inspected_image_id() { +async fn podman_uses_oci_identity_workspace_and_inspected_image_id() { if !is_e2e_driver("podman") { eprintln!("Skipping Podman OCI identity test: e2e driver is not podman"); return; @@ -178,17 +187,19 @@ async fn podman_uses_oci_identity_and_inspected_image_id() { 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, - "--policy", - policy_path, - "--no-tty", - ], + &["--from", &image.tag, "--policy", policy_path, "--no-tty"], &[ "sh", "-c", - "set -eu; printf 'direct-identity=%s:%s\n' \"$(id -u)\" \"$(id -g)\"; echo podman-oci-identity-ready; sleep infinity", + "set -eu; \ + test \"$(pwd -P)\" = /home/app/project; \ + test \"$HOME\" = /home/app/project; \ + test \"$(cat root-owned.txt)\" = root-owned; \ + test \"$(stat -c %u:%g .)\" = 2345:2346; \ + test \"$(stat -c %u:%g root-owned.txt)\" = 0:0; \ + touch direct-workspace-write; \ + printf 'direct-identity=%s:%s\n' \"$(id -u)\" \"$(id -g)\"; \ + echo podman-oci-identity-ready; sleep infinity", ], READY_MARKER, ) @@ -205,7 +216,13 @@ async fn podman_uses_oci_identity_and_inspected_image_id() { .exec(&[ "sh", "-c", - "test \"$(id -u):$(id -g)\" = 2345:2346; echo podman-ssh-identity-ok", + "set -eu; \ + test \"$(id -u):$(id -g)\" = 2345:2346; \ + test \"$(pwd -P)\" = /home/app/project; \ + test \"$HOME\" = /home/app/project; \ + test -f direct-workspace-write; \ + touch ssh-workspace-write; \ + echo podman-ssh-identity-ok", ]) .await .expect("SSH child should use Podman OCI identity"); diff --git a/proto/compute_driver.proto b/proto/compute_driver.proto index e3f18af19f..fd3e58554c 100644 --- a/proto/compute_driver.proto +++ b/proto/compute_driver.proto @@ -132,6 +132,17 @@ message DriverSandboxSpec { // ServiceAccount token bootstrap instead). Never echoed to the public // Sandbox proto. string sandbox_token = 11 [(openshell.options.v1.secret) = true]; + // Final process-identity inputs needed by local container drivers when they + // validate an OCI image workspace before launching the supervisor. + WorkspaceValidationIdentity workspace_validation_identity = 12; +} + +message WorkspaceValidationIdentity { + string run_as_user = 1; + string run_as_group = 2; + // No public policy was supplied, so the supervisor must discover an + // image-provided policy before resolving the final process identity. + bool discover_from_image_policy = 3; } message ResourceRequirements { From d7d5cf7fe00c494393f72762a2297bb71c6768f6 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 4 Aug 2026 11:59:58 -0700 Subject: [PATCH 2/5] fix(podman): harden workspace probe lifecycle Signed-off-by: Matthew Grossman --- crates/openshell-driver-podman/README.md | 12 +- .../openshell-driver-podman/src/container.rs | 102 ++++- crates/openshell-driver-podman/src/driver.rs | 358 +++++++++++++++--- crates/openshell-server/src/compute/mod.rs | 7 +- e2e/rust/tests/podman_oci_identity.rs | 19 +- 5 files changed, 406 insertions(+), 92 deletions(-) diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 4762d92480..1c1978b3f6 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -28,10 +28,14 @@ write and enter the workdir. The path must be a real directory without symlink components. The probe also rejects kernel-managed filesystems and overlaps with concrete OpenShell control resources. It emits a normalized identity attestation; the final supervisor must resolve to the same identity, including -when the image supplies the default process policy. On failure, the driver -captures a bounded, sanitized diagnostic before removing the probe. Only then -does Podman mount and prepare the managed workspace volume at that path; normal -copy-up preserves image content. The workspace is the child cwd and `HOME`. +when the image supplies the default process policy. The probe inherits the +sandbox CPU, memory, and PID limits and has a bounded runtime. Its lifecycle +continues to forced cleanup if the create request is cancelled, and a dedicated +label lets a restarted gateway remove any probe left by process termination. +On failure, the driver captures a bounded, sanitized diagnostic before removing +the probe. Only then does Podman mount and prepare the managed workspace volume +at that path; normal copy-up preserves image content. The workspace is the +child cwd and `HOME`. For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md). diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 4b778bd3af..5a5cd6de6c 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -45,6 +45,10 @@ pub use openshell_core::driver_utils::{ pub const LABEL_MANAGED: &str = "openshell.managed"; /// Label filter string for list/event queries. pub const LABEL_MANAGED_FILTER: &str = "openshell.managed=true"; +/// Label applied only to temporary OCI workdir validation probes. +pub const LABEL_WORKSPACE_PROBE: &str = "openshell.workspace-probe"; +/// Label filter used to recover probes left behind by a stopped gateway. +pub const LABEL_WORKSPACE_PROBE_FILTER: &str = "openshell.workspace-probe=true"; /// Container name prefix to avoid collisions with user containers. const CONTAINER_PREFIX: &str = "openshell-"; @@ -235,6 +239,30 @@ struct ContainerSpec { portmappings: Vec, } +/// Minimal Podman spec for an OpenShell-owned auxiliary container. +/// +/// Keep this typed separately from [`ContainerSpec`] so probes cannot +/// accidentally inherit sandbox networking, secrets, or workspace mounts, +/// while still sharing resource-limit construction and recovery metadata. +#[derive(Serialize)] +struct WorkspaceProbeSpec { + name: String, + image: String, + labels: BTreeMap, + entrypoint: Vec, + command: Vec, + user: String, + work_dir: String, + image_volumes: Vec, + image_volume_mode: String, + netns: NetNS, + no_new_privileges: bool, + cap_drop: Vec, + cap_add: Vec, + image_pull_policy: String, + resource_limits: ResourceLimits, +} + /// A port mapping entry for the libpod `SpecGenerator`. #[derive(Serialize)] struct PortMapping { @@ -544,6 +572,16 @@ fn build_labels(sandbox: &DriverSandbox) -> BTreeMap { labels } +fn build_workspace_probe_labels(sandbox: &DriverSandbox) -> BTreeMap { + BTreeMap::from([ + (LABEL_WORKSPACE_PROBE.into(), "true".into()), + (LABEL_SANDBOX_ID.into(), sandbox.id.clone()), + (LABEL_SANDBOX_NAME.into(), sandbox.name.clone()), + (LABEL_SANDBOX_NAMESPACE.into(), sandbox.namespace.clone()), + (LABEL_SANDBOX_WORKSPACE.into(), sandbox.workspace.clone()), + ]) +} + /// Parse resource limits from the sandbox template, falling back to defaults. fn build_resource_limits(sandbox: &DriverSandbox, config: &PodmanComputeConfig) -> ResourceLimits { let resources = sandbox @@ -1277,27 +1315,35 @@ pub fn build_workspace_probe_spec( } } - Ok(Some(serde_json::json!({ - "name": probe_name, - "image": inspected_image.id, - "entrypoint": [SUPERVISOR_BINARY_PATH], - "command": command, - "user": "0:0", - "work_dir": "/", - "image_volumes": [{ - "source": config.supervisor_image, - "destination": SUPERVISOR_MOUNT_DIR, - "rw": false + let probe_spec = WorkspaceProbeSpec { + name: probe_name.to_string(), + image: inspected_image.id.clone(), + labels: build_workspace_probe_labels(sandbox), + entrypoint: vec![SUPERVISOR_BINARY_PATH.into()], + command, + user: "0:0".into(), + work_dir: "/".into(), + image_volumes: vec![ImageVolume { + source: config.supervisor_image.clone(), + destination: SUPERVISOR_MOUNT_DIR.into(), + rw: false, }], // Do not materialize OCI VOLUME declarations: the probe must inspect // the immutable image layer rather than a fresh anonymous volume. - "image_volume_mode": "ignore", - "netns": {"nsmode": "none"}, - "no_new_privileges": true, - "cap_drop": ["ALL"], - "cap_add": ["SETUID", "SETGID"], - "image_pull_policy": "never" - }))) + image_volume_mode: "ignore".into(), + netns: NetNS { + nsmode: "none".into(), + }, + no_new_privileges: true, + cap_drop: vec!["ALL".into()], + cap_add: vec!["SETUID".into(), "SETGID".into()], + image_pull_policy: "never".into(), + resource_limits: build_resource_limits(sandbox, config), + }; + + Ok(Some( + serde_json::to_value(probe_spec).expect("WorkspaceProbeSpec serialization cannot fail"), + )) } fn hostadd_entries(config: &PodmanComputeConfig) -> Vec { @@ -1614,6 +1660,13 @@ mod tests { fn workspace_probe_uses_pinned_image_without_workspace_or_network() { let mut sandbox = test_sandbox("test-id", "test-name"); let spec = sandbox.spec.get_or_insert_default(); + spec.template.get_or_insert_default().resources = Some( + openshell_core::proto::compute::v1::DriverResourceRequirements { + cpu_limit: "750m".into(), + memory_limit: "1Gi".into(), + ..Default::default() + }, + ); spec.workspace_validation_identity = Some( openshell_core::proto::compute::v1::WorkspaceValidationIdentity { run_as_user: "policy-user".into(), @@ -1621,9 +1674,11 @@ mod tests { discover_from_image_policy: false, }, ); + let mut config = test_config(); + config.sandbox_pids_limit = 37; let probe = build_workspace_probe_spec( &sandbox, - &test_config(), + &config, &inspected_image("sha256:immutable", "app:staff", "/workspace/project"), "openshell-test-probe", ) @@ -1635,6 +1690,15 @@ mod tests { assert_eq!(probe["image_volume_mode"], "ignore"); assert_eq!(probe["cap_drop"], serde_json::json!(["ALL"])); assert_eq!(probe["cap_add"], serde_json::json!(["SETUID", "SETGID"])); + assert_eq!(probe["labels"][LABEL_WORKSPACE_PROBE], "true"); + assert_eq!(probe["labels"][LABEL_SANDBOX_ID], "test-id"); + assert!(probe["labels"].get(LABEL_MANAGED).is_none()); + assert_eq!(probe["resource_limits"]["cpu"]["quota"], 75_000); + assert_eq!( + probe["resource_limits"]["memory"]["limit"], + 1024 * 1024 * 1024_u64 + ); + assert_eq!(probe["resource_limits"]["PidsLimit"], 37); assert!(probe.get("volumes").is_none()); assert!(probe.get("secrets").is_none()); assert!(probe.get("env").is_none()); diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 00d62cfde6..66579f8109 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -5,7 +5,10 @@ use crate::client::{PodmanApiError, PodmanClient, VolumeInspect}; use crate::config::PodmanComputeConfig; -use crate::container::{self, LABEL_MANAGED_FILTER, LABEL_SANDBOX_ID, PodmanSandboxDriverConfig}; +use crate::container::{ + self, LABEL_MANAGED_FILTER, LABEL_SANDBOX_ID, LABEL_WORKSPACE_PROBE_FILTER, + PodmanSandboxDriverConfig, +}; use crate::watcher::{ self, WatchStream, driver_sandbox_from_inspect, driver_sandbox_from_list_entry, }; @@ -32,6 +35,8 @@ use std::time::Duration; use tracing::{debug, info, warn}; use url::Url; +const WORKSPACE_PROBE_TIMEOUT: Duration = Duration::from_secs(30); + impl From for ComputeDriverError { fn from(value: PodmanApiError) -> Self { match value { @@ -265,6 +270,101 @@ fn sanitized_probe_diagnostic(logs: &str) -> String { } } +async fn remove_workspace_probes( + client: &PodmanClient, + lifecycle_lock: &tokio::sync::Mutex<()>, + stop_timeout_secs: u32, +) -> Result { + let probes = client + .list_containers(&[LABEL_WORKSPACE_PROBE_FILTER]) + .await?; + let mut removed = 0; + for probe in probes { + let _lifecycle_guard = lifecycle_lock.lock().await; + match client.remove_container(&probe.id, stop_timeout_secs).await { + Ok(()) => removed += 1, + Err(PodmanApiError::NotFound(_)) => {} + Err(error) => return Err(error), + } + } + Ok(removed) +} + +async fn run_workspace_probe( + client: PodmanClient, + lifecycle_lock: Arc>, + stop_timeout_secs: u32, + spec: serde_json::Value, + probe_name: String, + image_id: String, + timeout: Duration, +) -> Result, ComputeDriverError> { + let mut created = false; + let validation = tokio::time::timeout(timeout, async { + { + let _lifecycle_guard = lifecycle_lock.lock().await; + client + .create_container(&spec) + .await + .map_err(ComputeDriverError::from)?; + created = true; + client + .start_container(&probe_name) + .await + .map_err(ComputeDriverError::from)?; + } + + let exit_code = client + .wait_container(&probe_name) + .await + .map_err(ComputeDriverError::from)?; + let logs = client.container_logs(&probe_name).await; + if exit_code == 0 { + let logs = logs.map_err(ComputeDriverError::from)?; + parse_workspace_identity(&logs).map(Some) + } else { + let diagnostic = logs.map_or_else( + |error| format!("unable to read probe diagnostic: {error}"), + |logs| sanitized_probe_diagnostic(&logs), + ); + Err(ComputeDriverError::Precondition(format!( + "OCI WorkingDir validation failed for image '{image_id}' (probe exited with code {exit_code}): {diagnostic}", + ))) + } + }) + .await + .unwrap_or_else(|_| { + Err(ComputeDriverError::Precondition(format!( + "OCI WorkingDir validation timed out for image '{image_id}' after {} seconds", + timeout.as_secs() + ))) + }); + + if !created { + return validation; + } + + let cleanup = { + let _lifecycle_guard = lifecycle_lock.lock().await; + client + .remove_container(&probe_name, stop_timeout_secs) + .await + }; + match (validation, cleanup) { + (Ok(identity), Ok(())) => Ok(identity), + (Ok(_), Err(error)) => Err(ComputeDriverError::from(error)), + (Err(error), Ok(())) => Err(error), + (Err(error), Err(cleanup_error)) => { + warn!( + probe = %probe_name, + %cleanup_error, + "Failed to remove workspace validation probe" + ); + Err(error) + } + } +} + /// Resolve the socket to connect to: explicit configuration wins, otherwise /// fall back to `detect`. Returns an error if neither resolves. /// @@ -305,59 +405,20 @@ impl PodmanComputeDriver { return Ok(None); }; - let start = { - let _lifecycle_guard = self.container_lifecycle_lock.lock().await; - self.client - .create_container(&spec) - .await - .map_err(ComputeDriverError::from)?; - self.client - .start_container(&probe_name) - .await - .map_err(ComputeDriverError::from) - }; - let validation = async { - start?; - let exit_code = self - .client - .wait_container(&probe_name) - .await - .map_err(ComputeDriverError::from)?; - let logs = self.client.container_logs(&probe_name).await; - if exit_code == 0 { - let logs = logs.map_err(ComputeDriverError::from)?; - parse_workspace_identity(&logs).map(Some) - } else { - let diagnostic = logs.map_or_else( - |error| format!("unable to read probe diagnostic: {error}"), - |logs| sanitized_probe_diagnostic(&logs), - ); - Err(ComputeDriverError::Precondition(format!( - "OCI WorkingDir validation failed for image '{}' (probe exited with code {exit_code}): {diagnostic}", - inspected_image.id, - ))) - } - } - .await; - let cleanup = { - let _lifecycle_guard = self.container_lifecycle_lock.lock().await; - self.client - .remove_container(&probe_name, self.config.stop_timeout_secs) - .await - }; - match (validation, cleanup) { - (Ok(identity), Ok(())) => Ok(identity), - (Ok(_), Err(error)) => Err(ComputeDriverError::from(error)), - (Err(error), Ok(())) => Err(error), - (Err(error), Err(cleanup_error)) => { - warn!( - probe = %probe_name, - %cleanup_error, - "Failed to remove workspace validation probe" - ); - Err(error) - } - } + let task = tokio::spawn(run_workspace_probe( + self.client.clone(), + Arc::clone(&self.container_lifecycle_lock), + self.config.stop_timeout_secs, + spec, + probe_name, + inspected_image.id.clone(), + WORKSPACE_PROBE_TIMEOUT, + )); + task.await.map_err(|error| { + ComputeDriverError::Message(format!( + "OCI WorkingDir validation probe task failed: {error}" + )) + })? } /// Create a new driver, verifying the Podman socket is reachable. @@ -494,6 +555,17 @@ impl PodmanComputeDriver { "Bridge network ready" ); + let container_lifecycle_lock = Arc::new(tokio::sync::Mutex::new(())); + let removed_probes = + remove_workspace_probes(&client, &container_lifecycle_lock, config.stop_timeout_secs) + .await?; + if removed_probes > 0 { + info!( + count = removed_probes, + "Removed workspace validation probes left by a previous gateway process" + ); + } + let (gpu_inventory, allow_all_default_gpu) = local_podman_gpu_selector_state(); if !gpu_inventory.is_empty() { info!( @@ -508,7 +580,7 @@ impl PodmanComputeDriver { network_gateway_ip, rootless, rootless_network_cmd, - container_lifecycle_lock: Arc::new(tokio::sync::Mutex::new(())), + container_lifecycle_lock, gpu_selector: Arc::new(CdiGpuDefaultSelector::new( gpu_inventory, allow_all_default_gpu, @@ -1607,6 +1679,7 @@ mod tests { }"#, ), StubResponse::new(StatusCode::CREATED, "{}"), + StubResponse::new(StatusCode::OK, "[]"), ], ); let config = PodmanComputeConfig { @@ -1622,17 +1695,70 @@ mod tests { assert!(driver.network_gateway_ip().is_none()); assert!(driver.gateway_listener_requirements().unwrap().is_empty()); handle.await.expect("stub task should finish"); + let requests = request_log + .lock() + .expect("request log lock should not be poisoned"); assert_eq!( - request_log - .lock() - .expect("request log lock should not be poisoned") - .as_slice(), + &requests[..3], [ "GET /_ping".to_string(), format!("GET {}", api_path("/libpod/info")), format!("POST {}", api_path("/libpod/networks/create")), ] ); + assert!(requests[3].contains("/libpod/containers/json?all=true&filters=")); + } + + #[tokio::test] + async fn constructor_removes_workspace_probes_left_by_previous_process() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "stale-workspace-probe", + vec![ + StubResponse::new(StatusCode::OK, ""), + StubResponse::new( + StatusCode::OK, + r#"{ + "host": { + "cgroupVersion": "v2", + "networkBackend": "netavark", + "security": {"rootless": false} + } + }"#, + ), + StubResponse::new(StatusCode::CREATED, "{}"), + StubResponse::new( + StatusCode::OK, + r#"[{ + "Id": "stale-probe-id", + "Names": ["openshell-stale-workdir-probe"], + "State": "exited", + "Labels": {"openshell.workspace-probe": "true"} + }]"#, + ), + StubResponse::new(StatusCode::NO_CONTENT, ""), + ], + ); + let config = PodmanComputeConfig { + socket_path: Some(socket_path.clone()), + grpc_endpoint: "https://gateway.example.test:9443".to_string(), + ..PodmanComputeConfig::default() + }; + + PodmanComputeDriver::new(config) + .await + .expect("startup should remove a stale workspace probe"); + + handle.await.expect("stub task should finish"); + let requests = request_log.lock().unwrap(); + assert!(requests[3].contains("/libpod/containers/json?all=true&filters=")); + assert!(requests[3].contains("openshell.workspace-probe")); + assert_eq!( + requests[4], + format!( + "DELETE {}?force=true&volumes=true&timeout=45", + api_path("/libpod/containers/stale-probe-id") + ) + ); } #[test] @@ -2270,6 +2396,120 @@ mod tests { let _ = fs::remove_file(socket_path); } + #[tokio::test] + async fn workspace_probe_cleanup_survives_caller_cancellation() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "workspace-probe-cancelled-caller", + vec![ + StubResponse::new(StatusCode::CREATED, "{}"), + StubResponse::new(StatusCode::NO_CONTENT, ""), + StubResponse::new(StatusCode::OK, r#"{"StatusCode":0}"#) + .with_delay(Duration::from_millis(100)), + StubResponse::new( + StatusCode::OK, + "OPENSHELL_WORKSPACE_IDENTITY=\"1234:1235:\"\n", + ), + StubResponse::new(StatusCode::NO_CONTENT, ""), + ], + ); + let driver = test_driver(socket_path.clone()); + let mut sandbox = plain_sandbox("sandbox-probe-cancel", "demo"); + sandbox.spec = Some(DriverSandboxSpec::default()); + let image = crate::client::ImageInspect { + id: "sha256:immutable".into(), + config: Some(crate::client::ImageConfig { + user: "1234:1235".into(), + working_dir: "/workspace".into(), + }), + }; + let name = validated_container_name(&sandbox).unwrap(); + + let caller = tokio::spawn(async move { + driver + .validate_image_workspace(&sandbox, &name, &image) + .await + }); + tokio::time::timeout(Duration::from_secs(2), async { + loop { + if request_log + .lock() + .unwrap() + .iter() + .any(|request| request.contains("/wait?condition=exited")) + { + break; + } + tokio::task::yield_now().await; + } + }) + .await + .expect("probe should reach wait before cancellation"); + caller.abort(); + let _ = caller.await; + + tokio::time::timeout(Duration::from_secs(2), handle) + .await + .expect("owned probe task should finish after caller cancellation") + .expect("stub task should finish"); + let requests = request_log.lock().unwrap(); + assert!(requests.last().unwrap().starts_with("DELETE ")); + let _ = fs::remove_file(socket_path); + } + + #[tokio::test] + async fn workspace_probe_timeout_forces_container_cleanup() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "workspace-probe-timeout", + vec![ + StubResponse::new(StatusCode::CREATED, "{}"), + StubResponse::new(StatusCode::NO_CONTENT, ""), + StubResponse::new(StatusCode::OK, r#"{"StatusCode":0}"#) + .with_delay(Duration::from_millis(100)), + StubResponse::new(StatusCode::NO_CONTENT, ""), + ], + ); + let driver = test_driver(socket_path.clone()); + let mut sandbox = plain_sandbox("sandbox-probe-timeout", "demo"); + sandbox.spec = Some(DriverSandboxSpec::default()); + let image = crate::client::ImageInspect { + id: "sha256:immutable".into(), + config: Some(crate::client::ImageConfig { + user: "1234:1235".into(), + working_dir: "/workspace".into(), + }), + }; + let name = validated_container_name(&sandbox).unwrap(); + let spec = container::build_workspace_probe_spec( + &sandbox, + &driver.config, + &image, + &workspace_probe_name(&name), + ) + .unwrap() + .unwrap(); + + let error = run_workspace_probe( + driver.client.clone(), + Arc::clone(&driver.container_lifecycle_lock), + driver.config.stop_timeout_secs, + spec, + workspace_probe_name(&name), + image.id, + Duration::from_millis(25), + ) + .await + .unwrap_err(); + assert!(error.to_string().contains("timed out")); + + tokio::time::timeout(Duration::from_secs(2), handle) + .await + .expect("timed-out probe should be force-removed") + .expect("stub task should finish"); + let requests = request_log.lock().unwrap(); + assert!(requests.last().unwrap().starts_with("DELETE ")); + let _ = fs::remove_file(socket_path); + } + fn secret_delete_request(sandbox_id: &str) -> String { format!( "DELETE {}", diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index b2d70be671..070e6563dd 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -2515,7 +2515,7 @@ fn driver_sandbox_spec_from_public( spec: &SandboxSpec, driver_name: &str, ) -> Result> { - let workspace_validation_identity = matches!(driver_name, "docker" | "podman").then(|| { + let workspace_validation_identity = (driver_name == "podman").then(|| { let process = spec .policy .as_ref() @@ -3296,7 +3296,7 @@ mod tests { } #[test] - fn driver_sandbox_spec_projects_process_identity_for_local_image_probe() { + fn driver_sandbox_spec_projects_process_identity_only_for_podman_probe() { let public = SandboxSpec { policy: Some(openshell_core::proto::SandboxPolicy { process: Some(openshell_core::proto::ProcessPolicy { @@ -3323,6 +3323,9 @@ mod tests { .unwrap() .discover_from_image_policy ); + + let docker = driver_sandbox_spec_from_public(&public, "docker").unwrap(); + assert!(docker.workspace_validation_identity.is_none()); } #[test] diff --git a/e2e/rust/tests/podman_oci_identity.rs b/e2e/rust/tests/podman_oci_identity.rs index 8dcc25155c..ff492bbb0b 100644 --- a/e2e/rust/tests/podman_oci_identity.rs +++ b/e2e/rust/tests/podman_oci_identity.rs @@ -22,7 +22,7 @@ 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 +const IMAGE_POLICY: &str = r#"version: 1 filesystem_policy: include_workdir: true @@ -32,6 +32,10 @@ landlock: compatibility: best_effort network_policies: {} + +process: + run_as_user: "2345" + run_as_group: "2346" "#; struct ImageGuard { @@ -52,6 +56,8 @@ impl ImageGuard { let context = tempfile::tempdir().map_err(|err| format!("create build context: {err}"))?; let containerfile = context.path().join("Containerfile"); + std::fs::write(context.path().join("policy.yaml"), IMAGE_POLICY) + .map_err(|err| format!("write image policy: {err}"))?; std::fs::write( &containerfile, format!( @@ -60,6 +66,7 @@ impl ImageGuard { RUN mkdir -p /home/app/project && \ chown {OCI_UID}:{OCI_GID} /home/app /home/app/project && \ chmod 0700 /home/app\n\ + COPY policy.yaml /etc/openshell/policy.yaml\n\ WORKDIR /home/app/project\n\ RUN printf root-owned > root-owned.txt && chown {OCI_UID}:{OCI_GID} .\n\ USER {OCI_UID}:{OCI_GID}\n" @@ -180,14 +187,10 @@ async fn podman_uses_oci_identity_workspace_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"); + // Do not supply a policy at create time. The probe must discover the + // image's policy before the workspace volume hides the immutable layer. let mut sandbox = SandboxGuard::create_keep_with_args( - &["--from", &image.tag, "--policy", policy_path, "--no-tty"], + &["--from", &image.tag, "--no-tty"], &[ "sh", "-c", From e1f6745657592635bba57f6783b33ab193e2a0e6 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 4 Aug 2026 13:10:08 -0700 Subject: [PATCH 3/5] refactor(podman): simplify workspace probe flow Signed-off-by: Matthew Grossman --- architecture/compute-runtimes.md | 11 +- crates/openshell-driver-podman/README.md | 25 +- crates/openshell-driver-podman/src/client.rs | 25 +- .../openshell-driver-podman/src/container.rs | 284 ++++++++---------- crates/openshell-driver-podman/src/driver.rs | 175 ++++------- crates/openshell-sandbox/src/lib.rs | 25 +- .../src/process.rs | 26 +- .../openshell-supervisor-process/src/run.rs | 1 + 8 files changed, 266 insertions(+), 306 deletions(-) diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 7ae07a934e..b043f883a0 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -213,13 +213,10 @@ control paths. Docker performs the check in the final container before workload launch and rejects image `VOLUME` declarations that would mask the workdir ancestry. Podman performs it in a minimal networkless container from the same pinned -image ID before its managed workspace volume covers the path. The probe retains -only the capabilities needed to adopt the completed process identity, drops to -that identity, and asks the kernel to validate access. It emits a normalized -identity attestation that the final supervisor must match, including when the -image supplies the default process policy. The driver captures a bounded, -sanitized diagnostic before removing a failed probe. The resolved workspace is -the child cwd and `HOME`; when `filesystem.include_workdir` is enabled, it +image ID before its managed workspace volume covers the path. The probe adopts +the completed process identity and emits an attestation that the final +supervisor must match. The resolved workspace is the child cwd and `HOME`; +when `filesystem.include_workdir` is enabled, it becomes the automatic writable policy path. Kubernetes/OpenShift keep their `/sandbox` PVC and `fsGroup` behavior, and VM keeps its `/sandbox` guest initialization path. diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 1c1978b3f6..54391f402a 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -19,23 +19,14 @@ with the user's numeric primary GID. Explicit `process.run_as_user` and An absolute OCI working directory becomes the agent workspace. An empty, root (`/`), or explicit `/sandbox` declaration uses `/sandbox`, which OpenShell -creates when necessary and owns as a compatibility workspace. For any other workdir, Podman -first starts a minimal, networkless probe from the pinned image ID without the -workspace volume, tokens, or TLS secrets. The probe retains only `SETUID` and -`SETGID`, resolves and adopts the completed identity, including supplementary -groups, and uses the kernel to verify that it can traverse every parent and -write and enter the workdir. The path must be a real directory without symlink -components. The probe also rejects kernel-managed filesystems and overlaps with -concrete OpenShell control resources. It emits a normalized identity -attestation; the final supervisor must resolve to the same identity, including -when the image supplies the default process policy. The probe inherits the -sandbox CPU, memory, and PID limits and has a bounded runtime. Its lifecycle -continues to forced cleanup if the create request is cancelled, and a dedicated -label lets a restarted gateway remove any probe left by process termination. -On failure, the driver captures a bounded, sanitized diagnostic before removing -the probe. Only then does Podman mount and prepare the managed workspace volume -at that path; normal copy-up preserves image content. The workspace is the -child cwd and `HOME`. +creates and owns as a compatibility workspace. For any other workdir, a +resource-limited, networkless probe verifies the original pinned image before +Podman covers the path with the managed workspace volume. The completed process +identity must already be able to traverse every parent and write and enter the +directory, without symlink components or OpenShell control-path collisions. +The final supervisor must match the probe's identity before preparing the +volume. See [Compute runtimes](../../architecture/compute-runtimes.md#process-identity) +for the invariant and probe lifecycle. For a rootless networking deep dive, see [NETWORKING.md](NETWORKING.md). diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 1c5e7e05bf..f7b3cc83a3 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -345,17 +345,26 @@ impl PodmanClient { builder.body(body).expect("valid request") } - /// Send a pre-built HTTP request and return status + body bytes. - async fn send_request( + /// Send a pre-built HTTP request for bounded or complete body collection. + async fn send_response( &self, req: Request>, timeout: Duration, - ) -> Result<(hyper::StatusCode, Bytes), PodmanApiError> { + ) -> Result, PodmanApiError> { let mut sender = self.connect().await?; - let response = tokio::time::timeout(timeout, sender.send_request(req)) + tokio::time::timeout(timeout, sender.send_request(req)) .await .map_err(|_| PodmanApiError::Timeout(timeout))? - .map_err(|e| PodmanApiError::Connection(e.to_string()))?; + .map_err(|error| PodmanApiError::Connection(error.to_string())) + } + + /// Send a pre-built HTTP request and return status + body bytes. + async fn send_request( + &self, + req: Request>, + timeout: Duration, + ) -> Result<(hyper::StatusCode, Bytes), PodmanApiError> { + let response = self.send_response(req, timeout).await?; let status = response.status(); let bytes = tokio::time::timeout(timeout, response.into_body().collect()) .await @@ -374,11 +383,7 @@ impl PodmanClient { ) -> Result<(hyper::StatusCode, Bytes), PodmanApiError> { use hyper::body::Body; - let mut sender = self.connect().await?; - let response = tokio::time::timeout(timeout, sender.send_request(req)) - .await - .map_err(|_| PodmanApiError::Timeout(timeout))? - .map_err(|error| PodmanApiError::Connection(error.to_string()))?; + let response = self.send_response(req, timeout).await?; let status = response.status(); let deadline = tokio::time::Instant::now() + timeout; let mut body = response.into_body(); diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 5a5cd6de6c..0e4da403ec 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -186,6 +186,39 @@ pub fn short_id(id: &str) -> String { // Typed container spec structs for the Podman libpod create API. // --------------------------------------------------------------------------- +/// Immutable image metadata normalized once for both probe and final launch. +#[derive(Debug, Clone)] +pub struct ResolvedPodmanImage { + id: String, + oci_user: String, + workspace_root: String, +} + +impl ResolvedPodmanImage { + pub fn from_inspect( + inspected: &ImageInspect, + config: &PodmanComputeConfig, + ) -> Result { + let image_config = inspected.config.as_ref(); + let workspace_root = driver_mounts::resolve_oci_workspace_root( + image_config.map_or("", |config| config.working_dir.as_str()), + ) + .map_err(ComputeDriverError::Precondition)?; + driver_mounts::validate_workspace_control_path( + &workspace_root, + &config.sandbox_ssh_socket_path, + ) + .map_err(ComputeDriverError::Precondition)?; + Ok(Self { + id: inspected.id.clone(), + oci_user: image_config + .map_or("", |config| config.user.as_str()) + .to_string(), + workspace_root, + }) + } +} + #[derive(Serialize)] struct ContainerSpec { name: String, @@ -245,7 +278,7 @@ struct ContainerSpec { /// accidentally inherit sandbox networking, secrets, or workspace mounts, /// while still sharing resource-limit construction and recovery metadata. #[derive(Serialize)] -struct WorkspaceProbeSpec { +pub struct WorkspaceProbeSpec { name: String, image: String, labels: BTreeMap, @@ -263,6 +296,20 @@ struct WorkspaceProbeSpec { resource_limits: ResourceLimits, } +impl WorkspaceProbeSpec { + pub fn name(&self) -> &str { + &self.name + } + + pub fn image_id(&self) -> &str { + &self.image + } + + pub fn to_value(&self) -> Value { + serde_json::to_value(self).expect("WorkspaceProbeSpec serialization cannot fail") + } +} + /// A port mapping entry for the libpod `SpecGenerator`. #[derive(Serialize)] struct PortMapping { @@ -572,14 +619,14 @@ fn build_labels(sandbox: &DriverSandbox) -> BTreeMap { labels } -fn build_workspace_probe_labels(sandbox: &DriverSandbox) -> BTreeMap { - BTreeMap::from([ - (LABEL_WORKSPACE_PROBE.into(), "true".into()), - (LABEL_SANDBOX_ID.into(), sandbox.id.clone()), - (LABEL_SANDBOX_NAME.into(), sandbox.name.clone()), - (LABEL_SANDBOX_NAMESPACE.into(), sandbox.namespace.clone()), - (LABEL_SANDBOX_WORKSPACE.into(), sandbox.workspace.clone()), - ]) +fn workspace_probe_name(sandbox: &DriverSandbox) -> String { + const SUFFIX: &str = "-workdir-probe"; + let container_name = container_name(&sandbox.workspace, &sandbox.name, &sandbox.id); + let keep = 255usize.saturating_sub(SUFFIX.len()); + format!( + "{}{SUFFIX}", + &container_name[..container_name.len().min(keep)] + ) } /// Parse resource limits from the sandbox template, falling back to defaults. @@ -954,16 +1001,20 @@ pub fn build_container_spec_with_token_and_gpu_devices( gpu_device_ids: Option<&[String]>, ) -> Result { let image = resolve_image(sandbox, config); + let resolved_image = ResolvedPodmanImage::from_inspect( + &ImageInspect { + id: image.to_string(), + config: None, + }, + config, + )?; build_container_spec_for_image( sandbox, config, token_secret_name, gpu_device_ids, image, - &ImageInspect { - id: image.to_string(), - config: None, - }, + &resolved_image, None, ) } @@ -974,31 +1025,16 @@ pub fn build_container_spec_for_image( token_secret_name: Option<&str>, gpu_device_ids: Option<&[String]>, requested_image: &str, - inspected_image: &ImageInspect, + image: &ResolvedPodmanImage, workspace_identity: Option<&str>, ) -> Result { let name = container_name(&sandbox.workspace, &sandbox.name, &sandbox.id); let vol = volume_name(&sandbox.id); - let oci_user = inspected_image - .config - .as_ref() - .map_or("", |config| config.user.as_str()); - let oci_working_dir = inspected_image - .config - .as_ref() - .map_or("", |config| config.working_dir.as_str()); - let workspace_root = driver_mounts::resolve_oci_workspace_root(oci_working_dir) - .map_err(ComputeDriverError::Precondition)?; - driver_mounts::validate_workspace_control_path( - &workspace_root, - &config.sandbox_ssh_socket_path, - ) - .map_err(ComputeDriverError::Precondition)?; - let mut env = build_env(sandbox, config, requested_image, oci_user); + let mut env = build_env(sandbox, config, requested_image, &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, &workspace_root) + let user_mounts = podman_user_mounts(sandbox, config.enable_bind_mounts, &image.workspace_root) .map_err(ComputeDriverError::InvalidArgument)?; if sandbox .spec @@ -1027,7 +1063,7 @@ pub fn build_container_spec_for_image( let mut volumes = vec![NamedVolume { name: vol, - dest: workspace_root.clone(), + dest: image.workspace_root.clone(), options: vec!["rw".into()], }]; volumes.extend(user_mounts.volumes); @@ -1039,7 +1075,7 @@ pub fn build_container_spec_for_image( }]; image_volumes.extend(user_mounts.image_volumes); - let mut command = vec!["--workdir".to_string(), workspace_root]; + let mut command = vec!["--workdir".to_string(), image.workspace_root.clone()]; command.extend(upstream_proxy_cli_args(config)); if let Some(identity) = workspace_identity { env.insert( @@ -1050,7 +1086,7 @@ pub fn build_container_spec_for_image( let container_spec = ContainerSpec { name, - image: inspected_image.id.clone(), + image: image.id.clone(), labels, env, volumes, @@ -1272,20 +1308,9 @@ pub fn build_container_spec_for_image( pub fn build_workspace_probe_spec( sandbox: &DriverSandbox, config: &PodmanComputeConfig, - inspected_image: &ImageInspect, - probe_name: &str, -) -> Result, ComputeDriverError> { - let image_config = inspected_image.config.as_ref(); - let oci_user = image_config.map_or("", |config| config.user.as_str()); - let oci_working_dir = image_config.map_or("", |config| config.working_dir.as_str()); - let workspace_root = driver_mounts::resolve_oci_workspace_root(oci_working_dir) - .map_err(ComputeDriverError::Precondition)?; - driver_mounts::validate_workspace_control_path( - &workspace_root, - &config.sandbox_ssh_socket_path, - ) - .map_err(ComputeDriverError::Precondition)?; - if workspace_root == driver_mounts::DEFAULT_WORKSPACE_ROOT { + image: &ResolvedPodmanImage, +) -> Result, ComputeDriverError> { + if image.workspace_root == driver_mounts::DEFAULT_WORKSPACE_ROOT { return Ok(None); } @@ -1296,9 +1321,9 @@ pub fn build_workspace_probe_spec( let mut command = vec![ "probe-workspace".to_string(), "--workdir".to_string(), - workspace_root, + image.workspace_root.clone(), "--oci-user".to_string(), - oci_user.to_string(), + image.oci_user.clone(), ]; if let Some(identity) = &spec.workspace_validation_identity { for (flag, value) in [ @@ -1316,9 +1341,12 @@ pub fn build_workspace_probe_spec( } let probe_spec = WorkspaceProbeSpec { - name: probe_name.to_string(), - image: inspected_image.id.clone(), - labels: build_workspace_probe_labels(sandbox), + name: workspace_probe_name(sandbox), + image: image.id.clone(), + labels: BTreeMap::from([ + (LABEL_WORKSPACE_PROBE.into(), "true".into()), + (LABEL_SANDBOX_ID.into(), sandbox.id.clone()), + ]), entrypoint: vec![SUPERVISOR_BINARY_PATH.into()], command, user: "0:0".into(), @@ -1341,9 +1369,7 @@ pub fn build_workspace_probe_spec( resource_limits: build_resource_limits(sandbox, config), }; - Ok(Some( - serde_json::to_value(probe_spec).expect("WorkspaceProbeSpec serialization cannot fail"), - )) + Ok(Some(probe_spec)) } fn hostadd_entries(config: &PodmanComputeConfig) -> Vec { @@ -1438,7 +1464,7 @@ mod tests { } } - fn inspected_image(id: &str, user: &str, working_dir: &str) -> ImageInspect { + fn image_inspect(id: &str, user: &str, working_dir: &str) -> ImageInspect { ImageInspect { id: id.to_string(), config: Some(ImageConfig { @@ -1448,6 +1474,11 @@ mod tests { } } + fn resolved_image(id: &str, user: &str, working_dir: &str) -> ResolvedPodmanImage { + ResolvedPodmanImage::from_inspect(&image_inspect(id, user, working_dir), &test_config()) + .unwrap() + } + #[test] fn parse_cpu_millicore() { assert_eq!(parse_cpu_to_microseconds("500m"), Some(50_000)); @@ -1556,7 +1587,7 @@ mod tests { None, None, "registry.example/app:latest", - &inspected_image("sha256:immutable", "app:staff", "/workspace/project"), + &resolved_image("sha256:immutable", "app:staff", "/workspace/project"), Some("1000:1000:"), ) .unwrap(); @@ -1606,7 +1637,7 @@ mod tests { None, None, "registry.example/app:latest", - &inspected_image("sha256:immutable", "app:staff", ""), + &resolved_image("sha256:immutable", "app:staff", ""), None, ) .unwrap(); @@ -1619,14 +1650,9 @@ mod tests { #[test] fn container_spec_rejects_invalid_oci_working_dir() { - let err = build_container_spec_for_image( - &test_sandbox("test-id", "test-name"), + let err = ResolvedPodmanImage::from_inspect( + &image_inspect("sha256:immutable", "app:staff", "relative/workspace"), &test_config(), - None, - None, - "registry.example/app:latest", - &inspected_image("sha256:immutable", "app:staff", "relative/workspace"), - None, ) .unwrap_err(); @@ -1638,18 +1664,13 @@ mod tests { #[test] fn container_spec_rejects_openshell_control_path_working_dir() { - let err = build_container_spec_for_image( - &test_sandbox("test-id", "test-name"), - &test_config(), - None, - None, - "registry.example/app:latest", - &inspected_image( + let err = ResolvedPodmanImage::from_inspect( + &image_inspect( "sha256:immutable", "app:staff", "/opt/openshell/bin/project", ), - None, + &test_config(), ) .unwrap_err(); @@ -1679,11 +1700,11 @@ mod tests { let probe = build_workspace_probe_spec( &sandbox, &config, - &inspected_image("sha256:immutable", "app:staff", "/workspace/project"), - "openshell-test-probe", + &resolved_image("sha256:immutable", "app:staff", "/workspace/project"), ) .unwrap() - .unwrap(); + .unwrap() + .to_value(); assert_eq!(probe["image"], "sha256:immutable"); assert_eq!(probe["netns"]["nsmode"], "none"); @@ -1734,11 +1755,11 @@ mod tests { let probe = build_workspace_probe_spec( &sandbox, &test_config(), - &inspected_image("sha256:immutable", "app:staff", "/home/app/project"), - "openshell-test-probe", + &resolved_image("sha256:immutable", "app:staff", "/home/app/project"), ) .unwrap() - .unwrap(); + .unwrap() + .to_value(); assert_eq!( probe["command"], @@ -1758,8 +1779,7 @@ mod tests { let probe = build_workspace_probe_spec( &test_sandbox("test-id", "test-name"), &test_config(), - &inspected_image("sha256:immutable", "sandbox:sandbox", "/"), - "openshell-test-probe", + &resolved_image("sha256:immutable", "sandbox:sandbox", "/"), ) .unwrap(); assert!(probe.is_none()); @@ -1772,83 +1792,45 @@ mod tests { template: Some(DriverSandboxTemplate::default()), ..Default::default() }); - sandbox - .spec - .as_mut() - .unwrap() - .template - .as_mut() - .unwrap() - .driver_config = Some(json_struct(serde_json::json!({ - "mounts": [{ - "type": "tmpfs", - "target": "/workspace" - }] - }))); - - let err = build_container_spec_for_image( - &sandbox, - &test_config(), - None, - None, - "registry.example/app:latest", - &inspected_image("sha256:immutable", "app:staff", "/workspace"), - None, - ) - .unwrap_err(); - assert!( - err.to_string() - .contains("reserved for the OpenShell workspace") - ); + let set_tmpfs_target = |sandbox: &mut DriverSandbox, target: &str| { + sandbox + .spec + .as_mut() + .unwrap() + .template + .as_mut() + .unwrap() + .driver_config = Some(json_struct(serde_json::json!({ + "mounts": [{"type": "tmpfs", "target": target}] + }))); + }; - sandbox - .spec - .as_mut() - .unwrap() - .template - .as_mut() - .unwrap() - .driver_config = Some(json_struct(serde_json::json!({ - "mounts": [{ - "type": "tmpfs", - "target": "/workspace" - }] - }))); - let err = build_container_spec_for_image( - &sandbox, - &test_config(), - None, - None, - "registry.example/app:latest", - &inspected_image("sha256:immutable", "app:staff", "/workspace/project"), - None, - ) - .unwrap_err(); - assert!( - err.to_string() - .contains("reserved for the OpenShell workspace") - ); + for workspace in ["/workspace", "/workspace/project"] { + set_tmpfs_target(&mut sandbox, "/workspace"); + let err = build_container_spec_for_image( + &sandbox, + &test_config(), + None, + None, + "registry.example/app:latest", + &resolved_image("sha256:immutable", "app:staff", workspace), + None, + ) + .unwrap_err(); + assert!( + err.to_string() + .contains("reserved for the OpenShell workspace") + ); + } - sandbox - .spec - .as_mut() - .unwrap() - .template - .as_mut() - .unwrap() - .driver_config = Some(json_struct(serde_json::json!({ - "mounts": [{ - "type": "tmpfs", - "target": "/workspace/cache" - }] - }))); + set_tmpfs_target(&mut sandbox, "/workspace/cache"); build_container_spec_for_image( &sandbox, &test_config(), None, None, "registry.example/app:latest", - &inspected_image("sha256:immutable", "app:staff", "/workspace"), + &resolved_image("sha256:immutable", "app:staff", "/workspace"), None, ) .expect("nested workspace mounts remain supported"); diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 66579f8109..69422083b0 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -96,15 +96,6 @@ fn validated_container_name(sandbox: &DriverSandbox) -> Result String { - const SUFFIX: &str = "-workdir-probe"; - let keep = 255usize.saturating_sub(SUFFIX.len()); - format!( - "{}{SUFFIX}", - &container_name[..container_name.len().min(keep)] - ) -} - fn podman_volume_is_bind_backed(volume: &VolumeInspect) -> bool { (volume.driver.is_empty() || volume.driver == "local") && volume.options.get("o").is_some_and(|options| { @@ -272,7 +263,6 @@ fn sanitized_probe_diagnostic(logs: &str) -> String { async fn remove_workspace_probes( client: &PodmanClient, - lifecycle_lock: &tokio::sync::Mutex<()>, stop_timeout_secs: u32, ) -> Result { let probes = client @@ -280,7 +270,6 @@ async fn remove_workspace_probes( .await?; let mut removed = 0; for probe in probes { - let _lifecycle_guard = lifecycle_lock.lock().await; match client.remove_container(&probe.id, stop_timeout_secs).await { Ok(()) => removed += 1, Err(PodmanApiError::NotFound(_)) => {} @@ -294,17 +283,18 @@ async fn run_workspace_probe( client: PodmanClient, lifecycle_lock: Arc>, stop_timeout_secs: u32, - spec: serde_json::Value, - probe_name: String, - image_id: String, + probe: container::WorkspaceProbeSpec, timeout: Duration, -) -> Result, ComputeDriverError> { +) -> Result { + let probe_name = probe.name().to_string(); + let image_id = probe.image_id().to_string(); + let create_body = probe.to_value(); let mut created = false; let validation = tokio::time::timeout(timeout, async { { let _lifecycle_guard = lifecycle_lock.lock().await; client - .create_container(&spec) + .create_container(&create_body) .await .map_err(ComputeDriverError::from)?; created = true; @@ -321,7 +311,7 @@ async fn run_workspace_probe( let logs = client.container_logs(&probe_name).await; if exit_code == 0 { let logs = logs.map_err(ComputeDriverError::from)?; - parse_workspace_identity(&logs).map(Some) + parse_workspace_identity(&logs) } else { let diagnostic = logs.map_or_else( |error| format!("unable to read probe diagnostic: {error}"), @@ -350,19 +340,17 @@ async fn run_workspace_probe( .remove_container(&probe_name, stop_timeout_secs) .await }; - match (validation, cleanup) { - (Ok(identity), Ok(())) => Ok(identity), - (Ok(_), Err(error)) => Err(ComputeDriverError::from(error)), - (Err(error), Ok(())) => Err(error), - (Err(error), Err(cleanup_error)) => { - warn!( - probe = %probe_name, - %cleanup_error, - "Failed to remove workspace validation probe" - ); - Err(error) + if let Err(cleanup_error) = cleanup { + if validation.is_ok() { + return Err(ComputeDriverError::from(cleanup_error)); } + warn!( + probe = %probe_name, + %cleanup_error, + "Failed to remove workspace validation probe" + ); } + validation } /// Resolve the socket to connect to: explicit configuration wins, otherwise @@ -389,36 +377,28 @@ impl PodmanComputeDriver { async fn validate_image_workspace( &self, sandbox: &DriverSandbox, - container_name: &str, - inspected_image: &crate::client::ImageInspect, + image: &container::ResolvedPodmanImage, ) -> Result, ComputeDriverError> { - let probe_name = workspace_probe_name(container_name); - crate::client::validate_name(&probe_name) - .map_err(|error| ComputeDriverError::Precondition(error.to_string()))?; - let Some(spec) = container::build_workspace_probe_spec( - sandbox, - &self.config, - inspected_image, - &probe_name, - )? + let Some(probe) = container::build_workspace_probe_spec(sandbox, &self.config, image)? else { return Ok(None); }; + crate::client::validate_name(probe.name()) + .map_err(|error| ComputeDriverError::Precondition(error.to_string()))?; let task = tokio::spawn(run_workspace_probe( self.client.clone(), Arc::clone(&self.container_lifecycle_lock), self.config.stop_timeout_secs, - spec, - probe_name, - inspected_image.id.clone(), + probe, WORKSPACE_PROBE_TIMEOUT, )); - task.await.map_err(|error| { + let identity = task.await.map_err(|error| { ComputeDriverError::Message(format!( "OCI WorkingDir validation probe task failed: {error}" )) - })? + })??; + Ok(Some(identity)) } /// Create a new driver, verifying the Podman socket is reachable. @@ -555,10 +535,7 @@ impl PodmanComputeDriver { "Bridge network ready" ); - let container_lifecycle_lock = Arc::new(tokio::sync::Mutex::new(())); - let removed_probes = - remove_workspace_probes(&client, &container_lifecycle_lock, config.stop_timeout_secs) - .await?; + let removed_probes = remove_workspace_probes(&client, config.stop_timeout_secs).await?; if removed_probes > 0 { info!( count = removed_probes, @@ -580,7 +557,7 @@ impl PodmanComputeDriver { network_gateway_ip, rootless, rootless_network_cmd, - container_lifecycle_lock, + container_lifecycle_lock: Arc::new(tokio::sync::Mutex::new(())), gpu_selector: Arc::new(CdiGpuDefaultSelector::new( gpu_inventory, allow_all_default_gpu, @@ -882,8 +859,10 @@ impl PodmanComputeDriver { "podman image '{image}' inspection did not return an immutable image ID" ))); } + let resolved_image = + container::ResolvedPodmanImage::from_inspect(&inspected_image, &self.config)?; let workspace_identity = self - .validate_image_workspace(sandbox, &name, &inspected_image) + .validate_image_workspace(sandbox, &resolved_image) .await?; for image in container::podman_driver_image_mount_sources(sandbox, self.config.enable_bind_mounts) @@ -949,7 +928,7 @@ impl PodmanComputeDriver { token_secret_name.as_deref(), gpu_devices.as_deref(), image, - &inspected_image, + &resolved_image, workspace_identity.as_deref(), ) { Ok(spec) => spec, @@ -2312,6 +2291,26 @@ mod tests { } } + fn workspace_probe_sandbox(id: &str) -> DriverSandbox { + let mut sandbox = plain_sandbox(id, "demo"); + sandbox.spec = Some(DriverSandboxSpec::default()); + sandbox + } + + fn workspace_probe_image(config: &PodmanComputeConfig) -> container::ResolvedPodmanImage { + container::ResolvedPodmanImage::from_inspect( + &crate::client::ImageInspect { + id: "sha256:immutable".into(), + config: Some(crate::client::ImageConfig { + user: "1234:1235".into(), + working_dir: "/workspace".into(), + }), + }, + config, + ) + .unwrap() + } + #[tokio::test] async fn workspace_probe_waits_for_success_and_always_removes_container() { let (socket_path, request_log, handle) = spawn_podman_stub( @@ -2328,19 +2327,11 @@ mod tests { ], ); let driver = test_driver(socket_path.clone()); - let mut sandbox = plain_sandbox("sandbox-probe", "demo"); - sandbox.spec = Some(DriverSandboxSpec::default()); - let image = crate::client::ImageInspect { - id: "sha256:immutable".into(), - config: Some(crate::client::ImageConfig { - user: "1234:1235".into(), - working_dir: "/workspace".into(), - }), - }; - let name = validated_container_name(&sandbox).unwrap(); + let sandbox = workspace_probe_sandbox("sandbox-probe"); + let image = workspace_probe_image(&driver.config); let identity = driver - .validate_image_workspace(&sandbox, &name, &image) + .validate_image_workspace(&sandbox, &image) .await .expect("successful probe should pass"); assert_eq!(identity.as_deref(), Some("1234:1235:")); @@ -2372,19 +2363,11 @@ mod tests { ], ); let driver = test_driver(socket_path.clone()); - let mut sandbox = plain_sandbox("sandbox-probe-fail", "demo"); - sandbox.spec = Some(DriverSandboxSpec::default()); - let image = crate::client::ImageInspect { - id: "sha256:immutable".into(), - config: Some(crate::client::ImageConfig { - user: "1234:1235".into(), - working_dir: "/workspace".into(), - }), - }; - let name = validated_container_name(&sandbox).unwrap(); + let sandbox = workspace_probe_sandbox("sandbox-probe-fail"); + let image = workspace_probe_image(&driver.config); let error = driver - .validate_image_workspace(&sandbox, &name, &image) + .validate_image_workspace(&sandbox, &image) .await .unwrap_err(); assert!(error.to_string().contains("exited with code 1")); @@ -2413,22 +2396,11 @@ mod tests { ], ); let driver = test_driver(socket_path.clone()); - let mut sandbox = plain_sandbox("sandbox-probe-cancel", "demo"); - sandbox.spec = Some(DriverSandboxSpec::default()); - let image = crate::client::ImageInspect { - id: "sha256:immutable".into(), - config: Some(crate::client::ImageConfig { - user: "1234:1235".into(), - working_dir: "/workspace".into(), - }), - }; - let name = validated_container_name(&sandbox).unwrap(); + let sandbox = workspace_probe_sandbox("sandbox-probe-cancel"); + let image = workspace_probe_image(&driver.config); - let caller = tokio::spawn(async move { - driver - .validate_image_workspace(&sandbox, &name, &image) - .await - }); + let caller = + tokio::spawn(async move { driver.validate_image_workspace(&sandbox, &image).await }); tokio::time::timeout(Duration::from_secs(2), async { loop { if request_log @@ -2469,32 +2441,17 @@ mod tests { ], ); let driver = test_driver(socket_path.clone()); - let mut sandbox = plain_sandbox("sandbox-probe-timeout", "demo"); - sandbox.spec = Some(DriverSandboxSpec::default()); - let image = crate::client::ImageInspect { - id: "sha256:immutable".into(), - config: Some(crate::client::ImageConfig { - user: "1234:1235".into(), - working_dir: "/workspace".into(), - }), - }; - let name = validated_container_name(&sandbox).unwrap(); - let spec = container::build_workspace_probe_spec( - &sandbox, - &driver.config, - &image, - &workspace_probe_name(&name), - ) - .unwrap() - .unwrap(); + let sandbox = workspace_probe_sandbox("sandbox-probe-timeout"); + let image = workspace_probe_image(&driver.config); + let probe = container::build_workspace_probe_spec(&sandbox, &driver.config, &image) + .unwrap() + .unwrap(); let error = run_workspace_probe( driver.client.clone(), Arc::clone(&driver.container_lifecycle_lock), driver.config.stop_timeout_secs, - spec, - workspace_probe_name(&name), - image.id, + probe, Duration::from_millis(25), ) .await diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 39fa050b27..80377ee602 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -202,14 +202,20 @@ pub async fn run_sandbox( &mut policy, &driver_identity, )?; - if matches!( + let workspace_attestation = if matches!( &driver_identity, openshell_supervisor_process::identity::DriverIdentity::OciUser { .. } - ) && std::env::var_os(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY) - .is_some_and(|value| !value.is_empty()) - { - let expected = std::env::var(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY) - .map_err(|_| miette::miette!("Podman workspace identity attestation is missing"))?; + ) { + match std::env::var_os(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY) { + Some(value) if !value.is_empty() => Some(value.into_string().map_err(|_| { + miette::miette!("Podman workspace identity attestation is not valid UTF-8") + })?), + _ => None, + } + } else { + None + }; + if let Some(expected) = workspace_attestation.as_deref() { let actual = openshell_supervisor_process::process::resolved_workspace_identity_attestation( &policy, resolved, @@ -225,13 +231,18 @@ pub async fn run_sandbox( openshell_supervisor_process::process::ResolvedWorkspace::new( workdir.clone(), use_workdir_as_home, + workspace_attestation.is_some(), ), ) }; #[cfg(not(unix))] let (resolved_process_identity, workspace) = ( openshell_supervisor_process::process::ResolvedProcessIdentity::default(), - openshell_supervisor_process::process::ResolvedWorkspace::new(workdir.clone(), false), + openshell_supervisor_process::process::ResolvedWorkspace::new( + workdir.clone(), + false, + false, + ), ); #[cfg_attr(not(target_os = "linux"), allow(unused_mut))] diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index ce6c1043ef..90d70bd82c 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -86,12 +86,17 @@ impl ResolvedProcessIdentity { pub struct ResolvedWorkspace { root: Option, use_as_home: bool, + prevalidated: bool, } impl ResolvedWorkspace { #[must_use] - pub fn new(root: Option, use_as_home: bool) -> Self { - Self { root, use_as_home } + pub fn new(root: Option, use_as_home: bool, prevalidated: bool) -> Self { + Self { + root, + use_as_home, + prevalidated, + } } #[must_use] @@ -108,6 +113,11 @@ impl ResolvedWorkspace { pub fn home(&self) -> Option<&str> { self.use_as_home.then(|| self.root()).flatten() } + + #[must_use] + pub const fn prevalidated(&self) -> bool { + self.prevalidated + } } impl ProcessEnforcementMode { @@ -1950,7 +1960,13 @@ 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(), None, false) + prepare_filesystem_with_identity( + policy, + ResolvedProcessIdentity::default(), + None, + false, + false, + ) } #[cfg(unix)] @@ -1959,6 +1975,7 @@ pub fn prepare_filesystem_with_identity( resolved_identity: ResolvedProcessIdentity, workdir: Option<&str>, prepare_workspace: bool, + workspace_prevalidated: bool, ) -> Result<()> { use nix::unistd::chown; @@ -1990,8 +2007,7 @@ pub fn prepare_filesystem_with_identity( })?; let workspace = Path::new(workspace); if workspace == Path::new(openshell_core::driver_mounts::DEFAULT_WORKSPACE_ROOT) - || std::env::var_os(openshell_core::sandbox_env::OCI_WORKSPACE_IDENTITY) - .is_some_and(|value| !value.is_empty()) + || workspace_prevalidated { info!(path = %workspace.display(), ?uid, ?gid, "Preparing managed workspace"); prepare_oci_workspace(workspace, uid, gid, &supplementary_gids)?; diff --git a/crates/openshell-supervisor-process/src/run.rs b/crates/openshell-supervisor-process/src/run.rs index 91e56b7ec8..9e375a3361 100644 --- a/crates/openshell-supervisor-process/src/run.rs +++ b/crates/openshell-supervisor-process/src/run.rs @@ -101,6 +101,7 @@ pub async fn run_process( resolved_process_identity, workspace.root(), workspace.home().is_some(), + workspace.prevalidated(), )?; } From 5348906b6898400aaa5d8e0bc0c76aa7633e3880 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 4 Aug 2026 13:18:28 -0700 Subject: [PATCH 4/5] refactor(podman): clarify request execution flow Signed-off-by: Matthew Grossman --- crates/openshell-driver-podman/src/client.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index f7b3cc83a3..e43deed8cf 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -345,8 +345,8 @@ impl PodmanClient { builder.body(body).expect("valid request") } - /// Send a pre-built HTTP request for bounded or complete body collection. - async fn send_response( + /// Execute a pre-built HTTP request and return the streaming response. + async fn execute_request( &self, req: Request>, timeout: Duration, @@ -364,7 +364,7 @@ impl PodmanClient { req: Request>, timeout: Duration, ) -> Result<(hyper::StatusCode, Bytes), PodmanApiError> { - let response = self.send_response(req, timeout).await?; + let response = self.execute_request(req, timeout).await?; let status = response.status(); let bytes = tokio::time::timeout(timeout, response.into_body().collect()) .await @@ -383,7 +383,7 @@ impl PodmanClient { ) -> Result<(hyper::StatusCode, Bytes), PodmanApiError> { use hyper::body::Body; - let response = self.send_response(req, timeout).await?; + let response = self.execute_request(req, timeout).await?; let status = response.status(); let deadline = tokio::time::Instant::now() + timeout; let mut body = response.into_body(); From 2069f04785a9ae0742057684c25f479176e0902e Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Tue, 4 Aug 2026 17:01:37 -0700 Subject: [PATCH 5/5] fix(podman): recover ambiguous workspace probes Signed-off-by: Matthew Grossman --- .agents/skills/openshell-cli/SKILL.md | 7 ++ Cargo.lock | 1 + crates/openshell-driver-podman/Cargo.toml | 1 + .../openshell-driver-podman/src/container.rs | 23 ++++--- crates/openshell-driver-podman/src/driver.rs | 64 ++++++++++++++++--- 5 files changed, 78 insertions(+), 18 deletions(-) diff --git a/.agents/skills/openshell-cli/SKILL.md b/.agents/skills/openshell-cli/SKILL.md index 866e57008a..1b51187dde 100644 --- a/.agents/skills/openshell-cli/SKILL.md +++ b/.agents/skills/openshell-cli/SKILL.md @@ -446,6 +446,13 @@ field wins independently; omitted fields fall back to the image declaration. An image with no `USER` fails before readiness unless policy supplies both fields. +Docker and Podman gateways also honor the image's OCI `WORKDIR`. An empty +value, `/`, or `/sandbox` uses the compatibility workspace at `/sandbox`. +Any other value must be an absolute, normalized path that already exists in +the image, contains no symlink components, and is traversable and writable by +the sandbox UID, GID, and supplementary groups. Podman validates that access +against the pinned image before its managed workspace volume covers the path. + ### Forward ports ```bash diff --git a/Cargo.lock b/Cargo.lock index 9f3f7dcdca..c78e385148 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3818,6 +3818,7 @@ dependencies = [ "tracing", "tracing-subscriber", "url", + "uuid", ] [[package]] diff --git a/crates/openshell-driver-podman/Cargo.toml b/crates/openshell-driver-podman/Cargo.toml index e46d2eed85..0397f86238 100644 --- a/crates/openshell-driver-podman/Cargo.toml +++ b/crates/openshell-driver-podman/Cargo.toml @@ -35,6 +35,7 @@ tracing-subscriber = { workspace = true } thiserror = { workspace = true } miette = { workspace = true } url = { workspace = true } +uuid = { workspace = true } [dev-dependencies] prost-types = { workspace = true } diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 0e4da403ec..94424644ba 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -619,14 +619,8 @@ fn build_labels(sandbox: &DriverSandbox) -> BTreeMap { labels } -fn workspace_probe_name(sandbox: &DriverSandbox) -> String { - const SUFFIX: &str = "-workdir-probe"; - let container_name = container_name(&sandbox.workspace, &sandbox.name, &sandbox.id); - let keep = 255usize.saturating_sub(SUFFIX.len()); - format!( - "{}{SUFFIX}", - &container_name[..container_name.len().min(keep)] - ) +fn workspace_probe_name() -> String { + format!("openshell-workdir-probe-{}", uuid::Uuid::new_v4().simple()) } /// Parse resource limits from the sandbox template, falling back to defaults. @@ -1341,7 +1335,9 @@ pub fn build_workspace_probe_spec( } let probe_spec = WorkspaceProbeSpec { - name: workspace_probe_name(sandbox), + // An attempt-unique name prevents an indeterminate create response + // from making a retry conflict with the first probe. + name: workspace_probe_name(), image: image.id.clone(), labels: BTreeMap::from([ (LABEL_WORKSPACE_PROBE.into(), "true".into()), @@ -1739,6 +1735,15 @@ mod tests { ); } + #[test] + fn workspace_probe_names_are_attempt_unique() { + let first = workspace_probe_name(); + let second = workspace_probe_name(); + + assert!(first.starts_with("openshell-workdir-probe-")); + assert_ne!(first, second); + } + #[test] fn workspace_probe_discovers_image_policy_identity_when_requested() { let mut sandbox = test_sandbox("test-id", "test-name"); diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 69422083b0..8dd39058ff 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -289,15 +289,21 @@ async fn run_workspace_probe( let probe_name = probe.name().to_string(); let image_id = probe.image_id().to_string(); let create_body = probe.to_value(); - let mut created = false; + // Set before awaiting create because a timeout or transport error can + // leave us unable to tell whether Podman created the container. + let mut cleanup_required = false; let validation = tokio::time::timeout(timeout, async { { let _lifecycle_guard = lifecycle_lock.lock().await; - client - .create_container(&create_body) - .await - .map_err(ComputeDriverError::from)?; - created = true; + cleanup_required = true; + if let Err(error) = client.create_container(&create_body).await { + if matches!(error, PodmanApiError::Conflict(_)) { + // Do not remove a container that this attempt did not + // create, even though UUID name collisions are unlikely. + cleanup_required = false; + } + return Err(ComputeDriverError::from(error)); + } client .start_container(&probe_name) .await @@ -330,7 +336,7 @@ async fn run_workspace_probe( ))) }); - if !created { + if !cleanup_required { return validation; } @@ -340,7 +346,9 @@ async fn run_workspace_probe( .remove_container(&probe_name, stop_timeout_secs) .await }; - if let Err(cleanup_error) = cleanup { + if let Err(cleanup_error) = cleanup + && !matches!(cleanup_error, PodmanApiError::NotFound(_)) + { if validation.is_ok() { return Err(ComputeDriverError::from(cleanup_error)); } @@ -2458,7 +2466,7 @@ mod tests { .unwrap_err(); assert!(error.to_string().contains("timed out")); - tokio::time::timeout(Duration::from_secs(2), handle) + tokio::time::timeout(Duration::from_secs(10), handle) .await .expect("timed-out probe should be force-removed") .expect("stub task should finish"); @@ -2467,6 +2475,44 @@ mod tests { let _ = fs::remove_file(socket_path); } + #[tokio::test] + async fn workspace_probe_create_timeout_still_attempts_cleanup() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "workspace-probe-create-timeout", + vec![ + StubResponse::new(StatusCode::CREATED, "{}").with_delay(Duration::from_millis(100)), + StubResponse::new(StatusCode::NO_CONTENT, ""), + ], + ); + let driver = test_driver(socket_path.clone()); + let sandbox = workspace_probe_sandbox("sandbox-probe-create-timeout"); + let image = workspace_probe_image(&driver.config); + let probe = container::build_workspace_probe_spec(&sandbox, &driver.config, &image) + .unwrap() + .unwrap(); + + let error = run_workspace_probe( + driver.client.clone(), + Arc::clone(&driver.container_lifecycle_lock), + driver.config.stop_timeout_secs, + probe, + Duration::from_millis(25), + ) + .await + .unwrap_err(); + assert!(error.to_string().contains("timed out")); + + tokio::time::timeout(Duration::from_secs(2), handle) + .await + .expect("ambiguous create should be reconciled") + .expect("stub task should finish"); + let requests = request_log.lock().unwrap(); + assert_eq!(requests.len(), 2); + assert!(requests[0].contains("/libpod/containers/create")); + assert!(requests[1].starts_with("DELETE ")); + let _ = fs::remove_file(socket_path); + } + fn secret_delete_request(sandbox_id: &str) -> String { format!( "DELETE {}",