From 6b706bf5ecd6932d6ecfdcef07b7ebf3ad1b8cea Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 30 Jul 2026 13:15:54 -0700 Subject: [PATCH 1/8] feat(sandbox): honor Docker OCI working directories Signed-off-by: Matthew Grossman --- .../skills/debug-openshell-cluster/SKILL.md | 3 + .../skills/generate-sandbox-policy/SKILL.md | 1 - .../generate-sandbox-policy/examples.md | 1 - .agents/skills/openshell-cli/SKILL.md | 14 +- .agents/skills/openshell-cli/cli-reference.md | 4 +- architecture/compute-runtimes.md | 18 +- crates/openshell-cli/src/ssh.rs | 257 +++-- crates/openshell-core/src/driver_mounts.rs | 248 ++++- crates/openshell-driver-docker/README.md | 36 +- crates/openshell-driver-docker/src/lib.rs | 69 +- crates/openshell-driver-docker/src/tests.rs | 159 ++- .../openshell-driver-kubernetes/src/driver.rs | 31 +- .../openshell-driver-podman/src/container.rs | 20 +- crates/openshell-policy/src/lib.rs | 6 +- crates/openshell-prover/src/lib.rs | 31 +- crates/openshell-prover/src/model.rs | 2 +- crates/openshell-prover/src/policy.rs | 16 +- crates/openshell-sandbox/src/lib.rs | 39 +- .../src/identity.rs | 91 ++ .../src/process.rs | 975 ++++++++++++++++-- .../openshell-supervisor-process/src/run.rs | 18 +- .../openshell-supervisor-process/src/ssh.rs | 116 ++- .../tutorials/first-network-policy.mdx | 2 +- docs/get-started/tutorials/github-sandbox.mdx | 1 - docs/reference/policy-schema.mdx | 3 +- docs/reference/sandbox-compute-drivers.mdx | 33 +- docs/sandboxes/manage-sandboxes.mdx | 17 +- docs/sandboxes/policies.mdx | 5 +- docs/security/best-practices.mdx | 2 +- e2e/rust/src/harness/sandbox.rs | 33 + e2e/rust/tests/custom_image.rs | 140 ++- e2e/rust/tests/driver_config_volume.rs | 156 ++- 32 files changed, 2138 insertions(+), 409 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index 0de85fb0a4..9ef35c9425 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -149,6 +149,9 @@ Common findings: - Gateway process stopped: inspect exit status and logs. - Sandbox image missing or pull denied: verify image reference and registry credentials. - Sandbox fails before readiness with an identity-resolution error: inspect the image's OCI `USER` and matching `/etc/passwd` and `/etc/group` entries, or explicitly set both process identity fields in policy. Root and missing identities are rejected. +- 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. OpenShell does not create, chown, or chmod a non-default image workdir. +- Docker also rejects an image `VOLUME` that covers the workdir or one of its parents because the runtime would mask the immutable path before validation. Move the `VOLUME` below the workspace or remove the declaration. +- A workdir rejected as a special filesystem or OpenShell control-path collision cannot be made valid with permissions. Move the image workdir away from kernel-backed mounts and the concrete supervisor, TLS, token, runtime, and socket paths named in the error. - Docker driver cannot initialize because it cannot find `openshell-sandbox`: verify `OPENSHELL_DOCKER_SUPERVISOR_BIN`, the sibling binary next to `openshell-gateway`, or the configured supervisor image contains `/openshell-sandbox`. - Sandbox never registers: check gateway logs and supervisor callback endpoint. - Supervisor image exits before printing `openshell-sandbox --version`: the image should be the scratch supervisor image from `deploy/docker/Dockerfile.supervisor` and must contain a static executable at `/openshell-sandbox`. diff --git a/.agents/skills/generate-sandbox-policy/SKILL.md b/.agents/skills/generate-sandbox-policy/SKILL.md index 3b261b5ded..5ce736d3f0 100644 --- a/.agents/skills/generate-sandbox-policy/SKILL.md +++ b/.agents/skills/generate-sandbox-policy/SKILL.md @@ -466,7 +466,6 @@ filesystem_policy: - /etc - /var/log read_write: - - /sandbox - /tmp - /dev/null diff --git a/.agents/skills/generate-sandbox-policy/examples.md b/.agents/skills/generate-sandbox-policy/examples.md index 179e46c00f..9f7832c1f7 100644 --- a/.agents/skills/generate-sandbox-policy/examples.md +++ b/.agents/skills/generate-sandbox-policy/examples.md @@ -823,7 +823,6 @@ filesystem_policy: - /etc - /var/log read_write: - - /sandbox - /tmp - /dev/null diff --git a/.agents/skills/openshell-cli/SKILL.md b/.agents/skills/openshell-cli/SKILL.md index 346672b912..996ccf6581 100644 --- a/.agents/skills/openshell-cli/SKILL.md +++ b/.agents/skills/openshell-cli/SKILL.md @@ -223,17 +223,23 @@ openshell sandbox ssh-config my-sandbox >> ~/.ssh/config ### Upload and download files ```bash -# Upload local files to sandbox -openshell sandbox upload my-sandbox ./src /sandbox/src +# Upload local files to the sandbox working directory +openshell sandbox upload my-sandbox ./src -# Download files from sandbox -openshell sandbox download my-sandbox /sandbox/output ./local-output +# Download a path relative to the sandbox working directory +openshell sandbox download my-sandbox output ./local-output ``` Uploads honor `.gitignore` by default. Add `--no-git-ignore` only when ignored files are intentionally in scope. Uploads preserve symlinks, including dangling symlinks, instead of dereferencing their targets. A symlink source bypasses Git-aware filtering so the link itself is archived. +When the upload destination is omitted, the CLI discovers the remote working +directory. Uploading a named directory merges it into an existing directory of +the same name, overwriting matching entries without deleting unrelated entries. +Downloads accept paths relative to that working directory or absolute paths +within it. + ### Execute a non-interactive command ```bash diff --git a/.agents/skills/openshell-cli/cli-reference.md b/.agents/skills/openshell-cli/cli-reference.md index a94afe2292..30d6fb7ed3 100644 --- a/.agents/skills/openshell-cli/cli-reference.md +++ b/.agents/skills/openshell-cli/cli-reference.md @@ -268,11 +268,11 @@ Open an interactive SSH shell. The name defaults to the last-used sandbox. `--ed ### `openshell sandbox upload [dest]` -Upload files using tar-over-SSH. The destination defaults to the container working directory. `.gitignore` filtering is enabled unless `--no-git-ignore` is passed. +Upload files using tar-over-SSH. The CLI discovers the canonical remote working directory when the destination is omitted. A named directory merges into an existing directory of the same name, overwriting matching entries without deleting unrelated entries. `.gitignore` filtering is enabled unless `--no-git-ignore` is passed. ### `openshell sandbox download [dest]` -Download files using tar-over-SSH. The local destination defaults to `.`. +Download files using tar-over-SSH. The sandbox source may be relative to the canonical remote working directory or an absolute path within it. The local destination defaults to `.`. ### `openshell sandbox ssh-config [name]` diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 55d7e7a29d..63083d8319 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -185,7 +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`. + its immutable image ID, and pass its raw OCI `Config.User`. Docker also + resolves 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. @@ -198,6 +199,21 @@ 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 +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 +identity, including supplementary groups, must already be able to traverse +every parent and write and enter the workdir; OpenShell does not change that +directory's ownership or mode. Filesystem metadata identifies kernel-managed +mounts, while 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. + Sandbox creation fails before the workload becomes ready when a required image identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0. The supervisor itself remains root so it can establish isolation before diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index f8de99a692..2b0a813d4d 100644 --- a/crates/openshell-cli/src/ssh.rs +++ b/crates/openshell-cli/src/ssh.rs @@ -7,7 +7,6 @@ use crate::tls::{TlsOptions, grpc_client}; use miette::{IntoDiagnostic, Result, WrapErr}; #[cfg(unix)] use nix::sys::signal::{SaFlags, SigAction, SigHandler, SigSet, Signal, sigaction}; -use openshell_core::ObjectId; use openshell_core::forward::{ ForwardSpec, build_proxy_command, format_gateway_url, resolve_ssh_gateway, shell_escape, validate_ssh_session_response, write_forward_pid, @@ -16,6 +15,7 @@ use openshell_core::proto::{ CreateSshSessionRequest, GetSandboxRequest, SshRelayTarget, TcpForwardFrame, TcpForwardInit, tcp_forward_init, }; +use openshell_core::{ObjectId, driver_mounts}; use owo_colors::OwoColorize; use std::fs; use std::future::Future; @@ -308,22 +308,12 @@ pub async fn sandbox_connect_editor( tls: &TlsOptions, workspace: &str, ) -> Result<()> { - // Verify the sandbox exists before writing SSH config / launching the editor. - let mut client = grpc_client(server, tls).await?; - client - .get_sandbox(GetSandboxRequest { - name: name.to_string(), - workspace: workspace.to_string(), - }) - .await - .into_diagnostic()? - .into_inner() - .sandbox - .ok_or_else(|| miette::miette!("sandbox not found: {name}"))?; + let session = ssh_session_config(server, name, tls, workspace).await?; + let workspace_root = discover_workspace_root(&session).await?; let host_alias = host_alias(name, workspace); install_ssh_config(gateway, name, workspace)?; - launch_editor(editor, &host_alias)?; + launch_editor(editor, &host_alias, &workspace_root)?; eprintln!( "{} Opened {} for sandbox {}", "✓".green().bold(), @@ -776,9 +766,8 @@ fn local_upload_path_is_file_like(path: &Path) -> bool { /// sandbox. Callers are responsible for splitting the destination path so /// that `dest_dir` is always a directory. /// -/// When `dest_dir` is `None`, the sandbox user's home directory (`$HOME`) is -/// used as the extraction target. This avoids hard-coding any particular -/// path and works for custom container images with non-default `WORKDIR`. +/// When `dest_dir` is `None`, tar extracts relative to the SSH session's +/// working directory. async fn ssh_tar_upload( server: &str, name: &str, @@ -789,9 +778,8 @@ async fn ssh_tar_upload( ) -> Result<()> { let session = ssh_session_config(server, name, tls, workspace).await?; - // When no explicit destination is given, use the unescaped `$HOME` shell - // variable so the remote shell resolves it at runtime. - let escaped_dest = dest_dir.map_or_else(|| "$HOME".to_string(), shell_escape); + let dest_dir = dest_dir.unwrap_or("."); + let escaped_dest = shell_escape(dest_dir); let mut ssh = ssh_base_command(&session.proxy_command); ssh.arg("-T") @@ -844,10 +832,6 @@ fn split_sandbox_path(path: &str) -> (&str, &str) { } } -/// Writable root inside every sandbox. Used as the boundary for path-traversal -/// checks on sandbox-side source paths in download flows. -const SANDBOX_WORKSPACE_ROOT: &str = "/sandbox"; - /// Lexically clean a POSIX-style absolute path by resolving `.` and `..` /// components, collapsing repeated separators, and stripping any trailing /// slash. Returns `None` if the input is empty or relative — the caller is @@ -883,64 +867,79 @@ fn lexical_clean_absolute_path(path: &str) -> Option { Some(out) } -/// Validate that a sandbox-side source path passed to `sandbox download` -/// resolves under the sandbox writable root. +/// Resolve a sandbox-side source path passed to `sandbox download` under the +/// sandbox writable root. /// /// Returns the cleaned, traversal-resolved path on success. Refuses any -/// path that lexically escapes `/sandbox` (e.g. `/etc/passwd`, -/// `/sandbox/../etc/passwd`) with a user-facing error. +/// path that lexically escapes the discovered workspace root with a user-facing +/// error. Relative paths are interpreted from the workspace root. /// /// This is a lexical guard only — it does not follow symlinks. Call /// `resolve_sandbox_source_path` after this on any path that will be passed -/// to a subsequent SSH I/O operation, so a symlink such as -/// `/sandbox/etc-link -> /etc` cannot leak files outside the workspace. -fn validate_sandbox_source_path(path: &str) -> Result { +/// to a subsequent SSH I/O operation, so a workspace symlink to `/etc` cannot +/// leak files outside the workspace. +fn validate_sandbox_source_path(workspace_root: &str, path: &str) -> Result { if path.is_empty() { return Err(miette::miette!("sandbox source path is empty")); } - let cleaned = lexical_clean_absolute_path(path) - .ok_or_else(|| miette::miette!("sandbox source path must be absolute (got '{path}')"))?; - if !is_under_sandbox_workspace(&cleaned) { + let candidate = if path.starts_with('/') { + path.to_string() + } else { + format!("{workspace_root}/{path}") + }; + let cleaned = lexical_clean_absolute_path(&candidate) + .ok_or_else(|| miette::miette!("sandbox source path is invalid (got '{path}')"))?; + if !driver_mounts::path_is_or_under(Path::new(&cleaned), Path::new(workspace_root)) { return Err(miette::miette!( - "sandbox source path '{path}' is outside the sandbox workspace ({SANDBOX_WORKSPACE_ROOT})" + "sandbox source path '{path}' is outside the sandbox workspace ({workspace_root})" )); } Ok(cleaned) } -/// Pure helper: is `path` equal to `/sandbox` or a descendant of it? -fn is_under_sandbox_workspace(path: &str) -> bool { - path == SANDBOX_WORKSPACE_ROOT || path.starts_with(&format!("{SANDBOX_WORKSPACE_ROOT}/")) -} - -/// Resolve every symlink in `sandbox_path` on the sandbox side and refuse the -/// result if it lands outside `/sandbox`. +/// Discover the workspace root and resolve every symlink in `sandbox_path` in +/// one SSH probe, then refuse the result if it lands outside the workspace. /// /// The lexical guard in `validate_sandbox_source_path` cannot see symlinks; a -/// path such as `/sandbox/etc-link/passwd` (where `etc-link -> /etc`) clears -/// the lexical check but would still leak `/etc/passwd` once `tar -C` follows -/// the link. Resolving symlinks on the remote side and re-validating closes -/// that gap. The returned fully-resolved path is what the caller should hand -/// to probe and tar invocations. +/// workspace path through `etc-link -> /etc` clears the lexical check but +/// would still leak `/etc/passwd` once `tar -C` follows the link. Resolving +/// symlinks on the remote side and re-validating closes that gap. The returned +/// fully-resolved path is what the caller should hand to probe and tar +/// invocations. Combining discovery and resolution also keeps downloads within +/// the gateway's three-connection limit: this probe, the type probe, and tar. async fn resolve_sandbox_source_path( session: &SshSessionConfig, sandbox_path: &str, ) -> Result { - let resolve_cmd = format!("realpath -e -- {path}", path = shell_escape(sandbox_path)); - let resolved = ssh_run_capture_stdout(session, &resolve_cmd) + let resolve_cmd = format!( + "pwd -P && realpath -e -- {path}", + path = shell_escape(sandbox_path) + ); + let output = ssh_run_capture_stdout(session, &resolve_cmd) .await .wrap_err_with(|| format!("failed to resolve sandbox source path '{sandbox_path}'"))?; + let (workspace_root, resolved) = output.split_once('\n').ok_or_else(|| { + miette::miette!("unexpected response while resolving sandbox source path '{sandbox_path}'") + })?; + if resolved.contains('\n') { + return Err(miette::miette!( + "unexpected response while resolving sandbox source path '{sandbox_path}'" + )); + } + + let workspace_root = validate_discovered_workspace_root(workspace_root)?; + validate_sandbox_source_path(&workspace_root, sandbox_path)?; if resolved.is_empty() { return Err(miette::miette!( "sandbox source path '{sandbox_path}' does not exist" )); } - if !is_under_sandbox_workspace(&resolved) { + if !driver_mounts::path_is_or_under(Path::new(resolved), Path::new(&workspace_root)) { return Err(miette::miette!( - "sandbox source path '{sandbox_path}' resolves to '{resolved}', outside the sandbox workspace ({SANDBOX_WORKSPACE_ROOT})" + "sandbox source path '{sandbox_path}' resolves to '{resolved}', outside the sandbox workspace ({workspace_root})" )); } - Ok(resolved) + Ok(resolved.to_string()) } /// Resolve the host-side target path for a downloaded *file*, following @@ -971,7 +970,7 @@ fn resolve_file_download_target( /// /// Files are streamed as a tar archive to `ssh ... tar xf - -C ` on /// the sandbox side. When `dest` is `None`, files are uploaded to the -/// sandbox user's home directory. +/// SSH session's working directory. #[allow(clippy::too_many_arguments)] pub async fn sandbox_sync_up_files( server: &str, @@ -1003,11 +1002,11 @@ pub async fn sandbox_sync_up_files( /// Push a local path (file or directory) into a sandbox using tar-over-SSH. /// -/// When `sandbox_path` is `None`, files are uploaded to the sandbox user's -/// home directory. When uploading a single file to an explicit destination -/// that does not end with `/`, the destination is treated as a file path: -/// the parent directory is created and the file is written with the -/// destination's basename. This matches `cp` / `scp` semantics. +/// When `sandbox_path` is `None`, files are uploaded to the SSH session's +/// working directory. When uploading a single file to an explicit destination +/// that does not end with `/`, the destination is treated as a file path: the +/// parent directory is created and the file is written with the destination's +/// basename. This matches `cp` / `scp` semantics. pub async fn sandbox_sync_up( server: &str, name: &str, @@ -1021,10 +1020,10 @@ pub async fn sandbox_sync_up( // `mkdir -p` creates the parent and tar extracts the file with the right // name. // - // Exception: if splitting would yield "/" as the parent (e.g. the user - // passed "/sandbox"), fall through to directory semantics instead. The - // sandbox user cannot write to "/" and the intent is almost certainly - // "put the file inside /sandbox", not "create a file named sandbox in /". + // Exception: if splitting would yield "/" as the parent, fall through to + // directory semantics instead. The sandbox user cannot write to "/" and + // the intent is almost certainly to place the file inside the named + // top-level directory. let local_path_is_file_like = local_upload_path_is_file_like(local_path); if let Some(path) = sandbox_path && local_path_is_file_like @@ -1124,7 +1123,38 @@ async fn ssh_run_capture_stdout(session: &SshSessionConfig, command: &str) -> Re output.status )); } - Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) + decode_ssh_probe_stdout(output.stdout) +} + +fn decode_ssh_probe_stdout(stdout: Vec) -> Result { + let stdout = String::from_utf8(stdout) + .map_err(|error| miette::miette!("ssh probe returned non-UTF-8 output: {error}"))?; + let stdout = stdout.strip_suffix('\n').unwrap_or(&stdout); + let stdout = stdout.strip_suffix('\r').unwrap_or(stdout); + Ok(stdout.to_string()) +} + +fn validate_discovered_workspace_root(root: &str) -> Result { + let cleaned = lexical_clean_absolute_path(root) + .ok_or_else(|| miette::miette!("remote workspace must be an absolute path"))?; + if cleaned == "/" { + return Err(miette::miette!( + "remote workspace resolved to the container root" + )); + } + if cleaned != root { + return Err(miette::miette!( + "remote workspace '{root}' is not a canonical absolute path" + )); + } + Ok(cleaned) +} + +async fn discover_workspace_root(session: &SshSessionConfig) -> Result { + let root = ssh_run_capture_stdout(session, "pwd -P") + .await + .wrap_err("failed to discover remote workspace")?; + validate_discovered_workspace_root(&root) } #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -1169,8 +1199,8 @@ async fn probe_sandbox_source_kind( /// behaviour for the directory-source case. /// /// The sandbox source path is also subjected to a workspace-boundary check -/// before any SSH command is issued; paths that lexically resolve outside -/// `/sandbox` are refused. +/// before any file probe or archive command is issued; paths that resolve +/// outside the discovered workspace root are refused. pub async fn sandbox_sync_down( server: &str, name: &str, @@ -1179,9 +1209,8 @@ pub async fn sandbox_sync_down( tls: &TlsOptions, workspace: &str, ) -> Result<()> { - let sandbox_path = validate_sandbox_source_path(sandbox_path)?; let session = ssh_session_config(server, name, tls, workspace).await?; - let sandbox_path = resolve_sandbox_source_path(&session, &sandbox_path).await?; + let sandbox_path = resolve_sandbox_source_path(&session, sandbox_path).await?; let kind = probe_sandbox_source_kind(&session, &sandbox_path).await?; match kind { @@ -1631,19 +1660,25 @@ pub fn install_ssh_config(gateway: &str, name: &str, workspace: &str) -> Result< Ok(managed_config) } -fn launch_editor(editor: Editor, host_alias: &str) -> Result<()> { +fn launch_editor(editor: Editor, host_alias: &str, workspace_root: &str) -> Result<()> { launch_editor_command( editor.binary(), editor.label(), &Editor::remote_target(host_alias), + workspace_root, ) } -fn launch_editor_command(binary: &str, label: &str, remote_target: &str) -> Result<()> { +fn launch_editor_command( + binary: &str, + label: &str, + remote_target: &str, + workspace_root: &str, +) -> Result<()> { let status = Command::new(binary) .arg("--remote") .arg(remote_target) - .arg("/sandbox") + .arg(workspace_root) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -1776,6 +1811,7 @@ mod tests { "openshell-test-missing-binary", "Test Editor", "ssh-remote+openshell-demo", + "/workspace/project", ) .unwrap_err(); let text = format!("{err}"); @@ -1968,68 +2004,99 @@ mod tests { #[test] fn validate_sandbox_source_path_accepts_workspace_paths() { + let workspace_root = "/workspace/project"; assert_eq!( - validate_sandbox_source_path("/sandbox/file.txt").unwrap(), - "/sandbox/file.txt" + validate_sandbox_source_path(workspace_root, "/workspace/project/file.txt").unwrap(), + "/workspace/project/file.txt" ); assert_eq!( - validate_sandbox_source_path("/sandbox/.agent/workspace/hello.txt").unwrap(), - "/sandbox/.agent/workspace/hello.txt" + validate_sandbox_source_path( + workspace_root, + "/workspace/project/.agent/workspace/hello.txt" + ) + .unwrap(), + "/workspace/project/.agent/workspace/hello.txt" + ); + assert_eq!( + validate_sandbox_source_path(workspace_root, "/workspace/project").unwrap(), + "/workspace/project" ); assert_eq!( - validate_sandbox_source_path("/sandbox").unwrap(), - "/sandbox" + validate_sandbox_source_path(workspace_root, "/workspace/project/").unwrap(), + "/workspace/project" ); assert_eq!( - validate_sandbox_source_path("/sandbox/").unwrap(), - "/sandbox" + validate_sandbox_source_path(workspace_root, "/workspace/project/sub/../file").unwrap(), + "/workspace/project/file" ); assert_eq!( - validate_sandbox_source_path("/sandbox/sub/../file").unwrap(), - "/sandbox/file" + validate_sandbox_source_path(workspace_root, "output/file.txt").unwrap(), + "/workspace/project/output/file.txt" + ); + assert_eq!( + validate_sandbox_source_path(workspace_root, "./output/../file.txt").unwrap(), + "/workspace/project/file.txt" ); } #[test] fn validate_sandbox_source_path_rejects_traversal_and_escapes() { - let traversal = validate_sandbox_source_path("/etc/passwd").unwrap_err(); + let workspace_root = "/workspace/project"; + let traversal = validate_sandbox_source_path(workspace_root, "/etc/passwd").unwrap_err(); assert!( format!("{traversal}").contains("outside the sandbox workspace"), "unexpected error: {traversal}" ); - let parent_escape = validate_sandbox_source_path("/sandbox/../etc/passwd").unwrap_err(); + let parent_escape = + validate_sandbox_source_path(workspace_root, "/workspace/project/../../etc/passwd") + .unwrap_err(); assert!( format!("{parent_escape}").contains("outside the sandbox workspace"), "unexpected error: {parent_escape}" ); - let prefix_only = validate_sandbox_source_path("/sandboxed/secrets").unwrap_err(); + let prefix_only = + validate_sandbox_source_path(workspace_root, "/workspace/projected/secrets") + .unwrap_err(); assert!( format!("{prefix_only}").contains("outside the sandbox workspace"), "unexpected error: {prefix_only}" ); - let empty = validate_sandbox_source_path("").unwrap_err(); + let empty = validate_sandbox_source_path(workspace_root, "").unwrap_err(); assert!(format!("{empty}").contains("empty")); - let relative = validate_sandbox_source_path("sandbox/file").unwrap_err(); - assert!(format!("{relative}").contains("must be absolute")); + let relative_escape = + validate_sandbox_source_path(workspace_root, "../../etc/passwd").unwrap_err(); + assert!(format!("{relative_escape}").contains("outside the sandbox workspace")); } #[test] - fn is_under_sandbox_workspace_accepts_root_and_descendants() { - assert!(is_under_sandbox_workspace("/sandbox")); - assert!(is_under_sandbox_workspace("/sandbox/file")); - assert!(is_under_sandbox_workspace("/sandbox/sub/nested")); + fn discovered_workspace_root_must_be_canonical_absolute_non_root() { + assert_eq!( + validate_discovered_workspace_root("/workspace/project").unwrap(), + "/workspace/project" + ); + for invalid in ["", "workspace", "/", "/workspace/../etc", "/workspace/"] { + assert!( + validate_discovered_workspace_root(invalid).is_err(), + "expected '{invalid}' to be rejected" + ); + } } #[test] - fn is_under_sandbox_workspace_rejects_outside_paths_and_prefix_collisions() { - assert!(!is_under_sandbox_workspace("/etc/passwd")); - assert!(!is_under_sandbox_workspace("/sandboxed/secrets")); - assert!(!is_under_sandbox_workspace("/")); - assert!(!is_under_sandbox_workspace("")); + fn ssh_probe_output_only_removes_the_protocol_line_ending() { + assert_eq!( + decode_ssh_probe_stdout(b"/workspace/project \n".to_vec()).unwrap(), + "/workspace/project " + ); + assert_eq!( + decode_ssh_probe_stdout(b"/workspace/project\r\n".to_vec()).unwrap(), + "/workspace/project" + ); + assert!(decode_ssh_probe_stdout(vec![0xff]).is_err()); } #[test] diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index 086d992c37..9e4a55d3cf 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -5,6 +5,11 @@ use std::path::Path; +use crate::driver_utils::{ + SANDBOX_TOKEN_MOUNT_PATH, SUPERVISOR_CONTAINER_DIR, TLS_CA_MOUNT_PATH, TLS_CERT_MOUNT_PATH, + TLS_KEY_MOUNT_PATH, UPSTREAM_PROXY_AUTH_MOUNT_PATH, +}; + /// `SELinux` relabelling mode for bind mounts. /// /// On hosts with `SELinux` enabled (e.g. Fedora, RHEL) a bind-mounted path @@ -25,13 +30,28 @@ pub enum SelinuxLabel { Private, } -const RESERVED_MOUNT_TARGETS: &[&str] = &[ - "/opt/openshell", - "/etc/openshell", - "/etc/openshell-tls", +/// Paths mounted or created by the local container supervisor. +/// +/// Keep this list tied to concrete `OpenShell` control resources. Workspace +/// safety itself is enforced from the image's ownership and mode metadata, +/// not by guessing which distro paths are sensitive. +const OPENSHELL_CONTROL_PATHS: &[&str] = &[ + SUPERVISOR_CONTAINER_DIR, + TLS_CA_MOUNT_PATH, + TLS_CERT_MOUNT_PATH, + TLS_KEY_MOUNT_PATH, + SANDBOX_TOKEN_MOUNT_PATH, + UPSTREAM_PROXY_AUTH_MOUNT_PATH, + // Kubernetes mounts the sandbox client certificate secret here. + "/etc/openshell-tls/client", + "/etc/openshell-tls/proxy", "/run/netns", ]; +/// Compatibility workspace used when an OCI image has no usable working +/// directory and by drivers whose workspace remains fixed. +pub const DEFAULT_WORKSPACE_ROOT: &str = "/sandbox"; + /// Validate a non-empty driver mount source. pub fn validate_mount_source(source: &str, field: &str) -> Result<(), String> { if source.is_empty() { @@ -78,51 +98,99 @@ pub fn validate_mount_subpath(subpath: &str) -> Result<(), String> { } /// Validate a container-side mount target for user-supplied driver mounts. +/// +/// Workspace collisions depend on the inspected image's resolved working +/// directory and are checked separately by `validate_workspace_mount_target`. pub fn validate_container_mount_target(target: &str) -> Result<(), String> { - if target.is_empty() { - return Err("mount target must not be empty".to_string()); - } - if target != target.trim() { - return Err("mount target must not contain surrounding whitespace".to_string()); - } - if target.as_bytes().contains(&0) { - return Err("mount target must not contain NUL bytes".to_string()); - } - if !target.starts_with('/') { - return Err("mount target must be an absolute container path".to_string()); - } - if target != "/" { - let segments = target.split('/').skip(1).collect::>(); - let has_internal_empty_segment = segments - .iter() - .take(segments.len().saturating_sub(1)) - .any(|segment| segment.is_empty()); - if has_internal_empty_segment || segments.contains(&".") { - return Err( - "mount target must be normalized and must not contain empty path segments or '.'" - .to_string(), - ); + let normalized = normalize_absolute_container_path(target, "mount target")?; + let path = Path::new(&normalized); + for reserved in OPENSHELL_CONTROL_PATHS { + let reserved = Path::new(reserved); + if paths_overlap(path, reserved) { + return Err(format!( + "mount target '{target}' conflicts with reserved OpenShell path '{}'", + reserved.display() + )); } } - let path = Path::new(target); - if path == Path::new("/") { - return Err("mount target must not be the container root".to_string()); + Ok(()) +} + +/// Resolve an OCI image working directory to the internal workspace root used +/// by local container drivers. +/// +/// Empty declarations and `/` use the compatibility fallback. Non-empty +/// declarations must already be normalized absolute paths so the inspected +/// value and the path passed to the supervisor cannot be interpreted +/// differently. +pub fn resolve_oci_workspace_root(working_dir: &str) -> Result { + if working_dir.is_empty() || working_dir == "/" { + return Ok(DEFAULT_WORKSPACE_ROOT.to_string()); } - if path - .components() - .any(|component| matches!(component, std::path::Component::ParentDir)) - { - return Err("mount target must not contain '..'".to_string()); + let workspace_root = normalize_absolute_container_path(working_dir, "OCI WorkingDir")?; + for control_path in OPENSHELL_CONTROL_PATHS { + validate_workspace_control_path(&workspace_root, control_path)?; } - if path == Path::new("/sandbox") { - return Err("mount target '/sandbox' is reserved for the OpenShell workspace".to_string()); + + Ok(workspace_root) +} + +fn normalize_absolute_container_path(value: &str, field: &str) -> Result { + if value.is_empty() { + return Err(format!("{field} must not be empty")); } - for reserved in RESERVED_MOUNT_TARGETS { - if path_is_or_under(path, Path::new(reserved)) { - return Err(format!( - "mount target '{target}' conflicts with reserved OpenShell path '{reserved}'" - )); - } + if value != value.trim() { + return Err(format!("{field} must not contain surrounding whitespace")); + } + if value.chars().any(char::is_control) { + return Err(format!("{field} must not contain control characters")); + } + if !value.starts_with('/') { + return Err(format!("{field} must be an absolute container path")); + } + + let segments = value.split('/').skip(1).collect::>(); + let has_internal_empty_segment = segments + .iter() + .take(segments.len().saturating_sub(1)) + .any(|segment| segment.is_empty()); + if has_internal_empty_segment || segments.contains(&".") || segments.contains(&"..") { + return Err(format!( + "{field} must be normalized without empty, '.', or '..' path segments" + )); + } + + let normalized = value.trim_end_matches('/'); + if normalized.is_empty() { + return Err(format!("{field} must not be the container root")); + } + Ok(normalized.to_string()) +} + +/// Reject a workspace that contains or is contained by an `OpenShell` control +/// path. Drivers use this for runtime-configured paths such as the SSH socket. +pub fn validate_workspace_control_path( + workspace_root: &str, + control_path: &str, +) -> Result<(), String> { + let workspace = Path::new(workspace_root); + let control = Path::new(control_path); + if paths_overlap(workspace, control) { + return Err(format!( + "OCI WorkingDir '{workspace_root}' conflicts with OpenShell control path '{control_path}'" + )); + } + Ok(()) +} + +/// Reject a user-supplied mount that would replace or contain the resolved +/// workspace root. Mounts below the workspace remain valid. +pub fn validate_workspace_mount_target(target: &str, workspace_root: &str) -> Result<(), String> { + let normalized_target = normalize_mount_target(target); + if path_is_or_under(Path::new(workspace_root), Path::new(&normalized_target)) { + return Err(format!( + "mount target '{target}' is reserved for the OpenShell workspace" + )); } Ok(()) } @@ -140,6 +208,10 @@ pub fn path_is_or_under(path: &Path, parent: &Path) -> bool { path == parent || path.starts_with(parent) } +fn paths_overlap(left: &Path, right: &Path) -> bool { + path_is_or_under(left, right) || path_is_or_under(right, left) +} + #[cfg(test)] mod tests { use super::*; @@ -151,17 +223,91 @@ mod tests { } #[test] - fn container_target_rejects_workspace_root_only() { - let err = validate_container_mount_target("/sandbox/").unwrap_err(); + fn container_target_workspace_reservation_is_dynamic() { + validate_container_mount_target("/sandbox/").unwrap(); + validate_workspace_mount_target("/sandbox/", "/sandbox").unwrap_err(); + validate_workspace_mount_target("/workspace/", "/sandbox").unwrap(); + validate_workspace_mount_target("/workspace/cache", "/workspace").unwrap(); + validate_workspace_mount_target("/workspace", "/workspace/project").unwrap_err(); + validate_workspace_mount_target("/workspace-other", "/workspace/project").unwrap(); + } - assert!(err.contains("reserved for the OpenShell workspace")); + #[test] + fn oci_workspace_root_uses_fallback_and_accepts_normalized_absolute_paths() { + assert_eq!(resolve_oci_workspace_root("").unwrap(), "/sandbox"); + assert_eq!(resolve_oci_workspace_root("/").unwrap(), "/sandbox"); + assert_eq!( + resolve_oci_workspace_root("/workspace/project/").unwrap(), + "/workspace/project" + ); + assert_eq!( + resolve_oci_workspace_root("/workspace with spaces").unwrap(), + "/workspace with spaces" + ); + } + + #[test] + fn oci_workspace_root_rejects_relative_and_malformed_paths() { + for invalid in [ + "workspace", + "./workspace", + "/workspace/../etc", + "/workspace/./project", + "/workspace//project", + "/workspace\0project", + "/workspace ", + "/workspace\nproject", + ] { + assert!( + resolve_oci_workspace_root(invalid).is_err(), + "expected '{invalid}' to be rejected" + ); + } + } + + #[test] + fn oci_workspace_root_rejects_only_openshell_control_path_collisions() { + for invalid in [ + "/etc", + "/opt", + "/opt/openshell", + "/opt/openshell/bin/project", + "/etc/openshell/tls/client", + "/etc/openshell/auth", + "/run", + "/run/netns/project", + ] { + assert!( + resolve_oci_workspace_root(invalid).is_err(), + "expected control-path workspace '{invalid}' to be rejected" + ); + } + + for valid in [ + "/app", + "/etc/project", + "/home/app", + "/opt/app", + "/usr/bin/project", + "/usr/src/app", + "/var/lib/app", + "/var/app/current", + "/var/task", + "/var/www/app", + ] { + assert_eq!( + resolve_oci_workspace_root(valid).unwrap(), + valid, + "expected application workspace '{valid}' to remain valid" + ); + } } #[test] fn container_target_rejects_reserved_openshell_tls_legacy_path() { - let err = validate_container_mount_target("/etc/openshell-tls/client").unwrap_err(); + let err = validate_container_mount_target("/etc/openshell-tls/proxy/client").unwrap_err(); - assert!(err.contains("/etc/openshell-tls")); + assert!(err.contains("/etc/openshell-tls/proxy")); } #[test] @@ -203,15 +349,15 @@ mod tests { fn mount_target_rejects_internal_empty_or_dot_segments() { assert_eq!( validate_container_mount_target("/sandbox/work//tmp").unwrap_err(), - "mount target must be normalized and must not contain empty path segments or '.'" + "mount target must be normalized without empty, '.', or '..' path segments" ); assert_eq!( validate_container_mount_target("/sandbox/work/./tmp").unwrap_err(), - "mount target must be normalized and must not contain empty path segments or '.'" + "mount target must be normalized without empty, '.', or '..' path segments" ); assert_eq!( validate_container_mount_target("/sandbox/work/../../tmp").unwrap_err(), - "mount target must not contain '..'" + "mount target must be normalized without empty, '.', or '..' path segments" ); validate_container_mount_target("/sandbox/work/").unwrap(); } diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index b2e74231ba..0123a6ee85 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -19,13 +19,28 @@ sandbox and starts the `openshell-sandbox` supervisor inside that container. The supervisor then creates the nested sandbox namespace for the agent process. Before creating the container, the driver inspects the final sandbox image and -captures its immutable image ID and raw OCI `Config.User`. Container creation -uses that image ID, preventing a mutable tag from changing between inspection -and launch. The supervisor runs as root, resolves omitted policy identity fields -from the image declaration, and drops only agent children to the resulting -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, preventing a +mutable tag from changing between inspection and launch. The supervisor runs as +root, resolves omitted policy identity fields from the image declaration, and +drops only agent children to the resulting 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. Any other image workdir must +already exist without symlink components. The completed identity, including +supplementary groups, must already be able to traverse every parent and +write and enter the workdir. OpenShell does not change its ownership or mode. +Image `VOLUME` declarations must not cover the workdir or one of its parents +because Docker would mount the volume before the supervisor could validate the +immutable image path. +Kernel-managed filesystems are detected from filesystem metadata, and paths +that overlap concrete OpenShell control resources are rejected. The workspace +is the child cwd and `HOME`. The supervisor starts from `/`, then reports an +invalid workdir as a readiness failure. Docker containers join an OpenShell-managed bridge network. The driver injects `host.openshell.internal` and `host.docker.internal` so supervisors have stable @@ -77,9 +92,10 @@ optional `selinux_label` of `shared` (applies `:z`) or `private` (applies `subpath`. User-supplied bind and volume mounts are read-only by default; set `read_only: false` to make them writable. Mount `source`, `target`, and `subpath` 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`. +absolute container paths and must not replace or contain the resolved workspace +root. Nested workspace mounts remain valid. Mounts also must not overlap +OpenShell supervisor files, `/etc/openshell`, `/etc/openshell-tls`, or +`/run/netns`. Example named-volume usage: diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index a196ba6ad9..c7c3fab032 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -219,6 +219,8 @@ struct DockerProvisioningFailure { struct DockerImageMetadata { id: String, user: String, + working_dir: String, + volumes: Vec, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -624,19 +626,13 @@ impl DockerComputeDriver { Self::validate_sandbox_auth(sandbox)?; self.validate_user_volume_mounts_available(&validated.driver_config) .await?; - let gpu_devices = self + let _ = self .resolve_gpu_cdi_devices( validated.gpu_requirements, &validated.driver_config, CdiGpuDefaultSelector::peek_device_ids, ) .await?; - let _ = build_container_create_body_with_gpu_devices( - sandbox, - &self.config, - &validated.driver_config, - gpu_devices.as_deref(), - )?; if self .find_managed_container_summary(&sandbox.id, &sandbox.name) @@ -1328,11 +1324,22 @@ impl DockerComputeDriver { "docker image '{image}' inspection did not return an immutable image ID" )) })?; - let user = inspect - .config - .and_then(|config| config.user) - .unwrap_or_default(); - Ok(DockerImageMetadata { id, user }) + let (user, working_dir, volumes) = inspect.config.map_or_else( + || (String::new(), String::new(), Vec::new()), + |config| { + ( + config.user.unwrap_or_default(), + config.working_dir.unwrap_or_default(), + config.volumes.unwrap_or_default(), + ) + }, + ); + Ok(DockerImageMetadata { + id, + user, + working_dir, + volumes, + }) } async fn pull_image(&self, sandbox_id: &str, image: &str) -> Result<(), Status> { @@ -2322,6 +2329,7 @@ fn build_container_create_body( build_container_create_body_with_gpu_devices(sandbox, config, &driver_config, cdi_devices) } +#[cfg(test)] fn build_container_create_body_with_gpu_devices( sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig, @@ -2341,6 +2349,8 @@ fn build_container_create_body_with_gpu_devices( &DockerImageMetadata { id: template.image.clone(), user: String::new(), + working_dir: String::new(), + volumes: Vec::new(), }, ) } @@ -2361,6 +2371,32 @@ fn build_container_create_body_for_image( .as_ref() .ok_or_else(|| Status::invalid_argument("sandbox.spec.template is required"))?; let resource_limits = docker_resource_limits(template)?; + let workspace_root = driver_mounts::resolve_oci_workspace_root(&image.working_dir) + .map_err(Status::failed_precondition)?; + driver_mounts::validate_workspace_control_path(&workspace_root, &config.ssh_socket_path) + .map_err(Status::failed_precondition)?; + for volume in &image.volumes { + driver_mounts::validate_container_mount_target(volume).map_err(|error| { + Status::failed_precondition(format!( + "invalid image-declared volume '{volume}': {error}" + )) + })?; + driver_mounts::validate_workspace_mount_target(volume, &workspace_root).map_err(|_| { + Status::failed_precondition(format!( + "image-declared volume '{volume}' masks OCI WorkingDir '{workspace_root}' before workspace validation" + )) + })?; + } + for mount in &driver_config.mounts { + let target = match mount { + DockerDriverMountConfig::Bind { target, .. } + | DockerDriverMountConfig::Volume { target, .. } + | DockerDriverMountConfig::Tmpfs { target, .. } + | DockerDriverMountConfig::Image { target, .. } => target, + }; + driver_mounts::validate_workspace_mount_target(target, &workspace_root) + .map_err(Status::failed_precondition)?; + } let user_mounts = docker_driver_mounts(driver_config)?; let user_bind_strings = docker_driver_bind_strings(driver_config)?; let device_requests = gpu_device_ids.map(|device_ids| { @@ -2393,11 +2429,14 @@ fn build_container_create_body_for_image( Ok(ContainerCreateBody { image: Some(image.id.clone()), user: Some("0".to_string()), + // The image workspace may need to be created or rejected by the + // supervisor, so do not let the OCI runtime chdir there first. + working_dir: Some("/".to_string()), env: Some(build_environment_for_oci_user(sandbox, config, &image.user)), entrypoint: Some(vec![SUPERVISOR_MOUNT_PATH.to_string()]), - // Clear the image CMD so Docker does not append inherited args to the - // supervisor entrypoint. - cmd: Some(Vec::new()), + // Replace the image CMD with the supervisor's resolved workspace + // argument so Docker cannot append inherited image arguments. + cmd: Some(vec!["--workdir".to_string(), workspace_root]), labels: Some(labels), host_config: Some(HostConfig { nano_cpus: resource_limits.nano_cpus, diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index a86a9936f5..7d1ffbcf6b 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -562,6 +562,8 @@ fn container_creation_uses_inspected_immutable_image() { let metadata = DockerImageMetadata { id: "sha256:immutable".to_string(), user: "1234:1235".to_string(), + working_dir: "/workspace/project".to_string(), + volumes: Vec::new(), }; let body = build_container_create_body_for_image( &sandbox, @@ -574,12 +576,160 @@ fn container_creation_uses_inspected_immutable_image() { assert_eq!(body.image.as_deref(), Some("sha256:immutable")); assert_eq!(body.user.as_deref(), Some("0")); + assert_eq!(body.working_dir.as_deref(), Some("/")); + assert_eq!( + body.cmd.as_deref(), + Some(&["--workdir".to_string(), "/workspace/project".to_string()][..]) + ); assert!(body.env.unwrap().contains(&format!( "{}=1234:1235", openshell_core::sandbox_env::OCI_IMAGE_USER ))); } +#[test] +fn container_creation_rejects_invalid_oci_working_dir() { + let metadata = DockerImageMetadata { + id: "sha256:immutable".to_string(), + user: "1234:1235".to_string(), + working_dir: "relative/workspace".to_string(), + volumes: Vec::new(), + }; + let err = build_container_create_body_for_image( + &test_sandbox(), + &runtime_config(), + &DockerSandboxDriverConfig::default(), + None, + &metadata, + ) + .unwrap_err(); + + assert_eq!(err.code(), tonic::Code::FailedPrecondition); + assert!(err.message().contains("must be an absolute container path")); +} + +#[test] +fn container_creation_rejects_openshell_control_path_working_dir() { + let metadata = DockerImageMetadata { + id: "sha256:immutable".to_string(), + user: "1234:1235".to_string(), + working_dir: "/opt/openshell/bin/project".to_string(), + volumes: Vec::new(), + }; + let err = build_container_create_body_for_image( + &test_sandbox(), + &runtime_config(), + &DockerSandboxDriverConfig::default(), + None, + &metadata, + ) + .unwrap_err(); + + assert_eq!(err.code(), tonic::Code::FailedPrecondition); + assert!(err.message().contains("OpenShell control path")); +} + +#[test] +fn container_creation_rejects_image_volume_that_masks_working_dir() { + let sandbox = test_sandbox(); + let metadata = DockerImageMetadata { + id: "sha256:immutable".to_string(), + user: "1234:1235".to_string(), + working_dir: "/workspace/project".to_string(), + volumes: vec!["/workspace".to_string()], + }; + + let error = build_container_create_body_for_image( + &sandbox, + &runtime_config(), + &DockerSandboxDriverConfig::default(), + None, + &metadata, + ) + .unwrap_err(); + + assert!( + error + .message() + .contains("masks OCI WorkingDir '/workspace/project'") + ); +} + +#[test] +fn container_creation_reserves_resolved_workspace_root_but_allows_nested_mounts() { + let metadata = DockerImageMetadata { + id: "sha256:immutable".to_string(), + user: "1234:1235".to_string(), + working_dir: "/workspace".to_string(), + volumes: Vec::new(), + }; + let root_mount: DockerSandboxDriverConfig = serde_json::from_value(serde_json::json!({ + "mounts": [{"type": "tmpfs", "target": "/workspace"}] + })) + .unwrap(); + let err = build_container_create_body_for_image( + &test_sandbox(), + &runtime_config(), + &root_mount, + None, + &metadata, + ) + .unwrap_err(); + assert!( + err.message() + .contains("reserved for the OpenShell workspace") + ); + + let ancestor_mount: DockerSandboxDriverConfig = serde_json::from_value(serde_json::json!({ + "mounts": [{"type": "tmpfs", "target": "/workspace"}] + })) + .unwrap(); + let nested_metadata = DockerImageMetadata { + working_dir: "/workspace/project".to_string(), + volumes: Vec::new(), + ..metadata.clone() + }; + let err = build_container_create_body_for_image( + &test_sandbox(), + &runtime_config(), + &ancestor_mount, + None, + &nested_metadata, + ) + .unwrap_err(); + assert!( + err.message() + .contains("reserved for the OpenShell workspace") + ); + + let nested_mount: DockerSandboxDriverConfig = serde_json::from_value(serde_json::json!({ + "mounts": [{"type": "tmpfs", "target": "/workspace/cache"}] + })) + .unwrap(); + build_container_create_body_for_image( + &test_sandbox(), + &runtime_config(), + &nested_mount, + None, + &metadata, + ) + .expect("nested workspace mounts remain supported"); + + let compatibility_path_mount: DockerSandboxDriverConfig = + serde_json::from_value(serde_json::json!({ + "mounts": [{"type": "tmpfs", "target": "/sandbox"}] + })) + .unwrap(); + build_container_create_body_for_image( + &test_sandbox(), + &runtime_config(), + &compatibility_path_mount, + None, + &metadata, + ) + .expect("/sandbox remains mountable when the inspected workspace is elsewhere"); +} + #[test] fn build_environment_keeps_path_driver_controlled() { let mut sandbox = test_sandbox(); @@ -1115,7 +1265,7 @@ fn driver_config_rejects_reserved_mount_targets() { "mounts": [{ "type": "volume", "source": "work-nfs", - "target": "/etc/openshell/auth/custom" + "target": "/etc/openshell/auth" }] }))); @@ -1210,14 +1360,17 @@ fn managed_container_label_filters_include_gateway_namespace() { } #[test] -fn build_container_create_body_clears_inherited_cmd() { +fn build_container_create_body_replaces_inherited_cmd_with_workspace_arg() { let create_body = build_container_create_body(&test_sandbox(), &runtime_config()).unwrap(); assert_eq!( create_body.entrypoint, Some(vec![SUPERVISOR_MOUNT_PATH.to_string()]) ); - assert_eq!(create_body.cmd, Some(Vec::new())); + assert_eq!( + create_body.cmd, + Some(vec!["--workdir".to_string(), "/sandbox".to_string()]) + ); assert_eq!( create_body .labels diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index 2d947b8a28..56361ef59c 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -312,6 +312,10 @@ fn validate_kubernetes_driver_volume_mounts( } driver_mounts::validate_container_mount_target(&mount.mount_path)?; + driver_mounts::validate_workspace_mount_target( + &mount.mount_path, + driver_mounts::DEFAULT_WORKSPACE_ROOT, + )?; let normalized_mount_path = driver_mounts::normalize_mount_target(&mount.mount_path); if !mount_paths.insert(normalized_mount_path.clone()) { return Err(format!( @@ -1601,7 +1605,11 @@ fn apply_supervisor_sideload( // Override command to use the side-loaded supervisor binary container.insert( "command".to_string(), - serde_json::json!([format!("{}/openshell-sandbox", SUPERVISOR_MOUNT_PATH)]), + serde_json::json!([ + format!("{}/openshell-sandbox", SUPERVISOR_MOUNT_PATH), + "--workdir", + driver_mounts::DEFAULT_WORKSPACE_ROOT + ]), ); // Force the supervisor to run as root (UID 0). Sandbox images may set @@ -1917,7 +1925,9 @@ fn apply_supervisor_sidecar_topology( "command".to_string(), serde_json::json!([ format!("{}/openshell-sandbox", SUPERVISOR_MOUNT_PATH), - "--mode=process" + "--mode=process", + "--workdir", + driver_mounts::DEFAULT_WORKSPACE_ROOT ]), ); @@ -4056,7 +4066,8 @@ mod tests { "init container must not depend on a shell" ); - // Agent container command should be overridden to the emptyDir path + // `--workdir` is optional for standalone supervisor invocations and + // has no implicit default, so Kubernetes must pass its fixed workspace. let command = pod_template["spec"]["containers"][0]["command"] .as_array() .expect("command should be set"); @@ -4064,6 +4075,16 @@ mod tests { command[0].as_str().unwrap(), format!("{SUPERVISOR_MOUNT_PATH}/openshell-sandbox") ); + assert_eq!( + command, + serde_json::json!([ + format!("{SUPERVISOR_MOUNT_PATH}/openshell-sandbox"), + "--workdir", + driver_mounts::DEFAULT_WORKSPACE_ROOT + ]) + .as_array() + .unwrap() + ); // Agent volume mount should be read-only let mounts = pod_template["spec"]["containers"][0]["volumeMounts"] @@ -4211,7 +4232,9 @@ mod tests { agent["command"], serde_json::json!([ format!("{SUPERVISOR_MOUNT_PATH}/openshell-sandbox"), - "--mode=process" + "--mode=process", + "--workdir", + driver_mounts::DEFAULT_WORKSPACE_ROOT ]) ); assert_eq!(agent["securityContext"]["runAsUser"], 1500); diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 90ef0fec21..223d2b43eb 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -963,6 +963,11 @@ 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(), + ]; + command.extend(upstream_proxy_cli_args(config)); let container_spec = ContainerSpec { name, @@ -984,10 +989,11 @@ 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()], - // 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: upstream_proxy_cli_args(config), + // 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. + 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 // supervisor needs root to create network namespaces, set up the @@ -1401,6 +1407,10 @@ mod tests { container["env"][openshell_core::sandbox_env::SANDBOX_GID].as_str(), Some("") ); + assert_eq!( + container["command"], + serde_json::json!(["--workdir", "/sandbox"]) + ); } #[test] @@ -2506,7 +2516,7 @@ mod tests { "mounts": [{ "type": "volume", "source": "work-nfs", - "target": "/etc/openshell/tls/custom" + "target": "/etc/openshell/tls/client" }] }))), ..Default::default() diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index 7937a3757e..1e81451c7b 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -1070,7 +1070,7 @@ pub fn restrictive_default_policy() -> SandboxPolicy { "/etc".into(), "/var/log".into(), ], - read_write: vec!["/sandbox".into(), "/tmp".into(), "/dev/null".into()], + read_write: vec!["/tmp".into(), "/dev/null".into()], }), landlock: Some(LandlockPolicy { compatibility: "best_effort".into(), @@ -1650,8 +1650,8 @@ network_policies: "read_only should contain /usr" ); assert!( - fs.read_write.iter().any(|p| p == "/sandbox"), - "read_write should contain /sandbox" + !fs.read_write.iter().any(|p| p == "/sandbox"), + "the workspace should be granted through include_workdir, not a literal /sandbox path" ); assert!( fs.read_write.iter().any(|p| p == "/tmp"), diff --git a/crates/openshell-prover/src/lib.rs b/crates/openshell-prover/src/lib.rs index 0fb8757577..913045fe7d 100644 --- a/crates/openshell-prover/src/lib.rs +++ b/crates/openshell-prover/src/lib.rs @@ -105,10 +105,11 @@ mod tests { fn test_filesystem_policy() { let path = testdata_dir().join("policy.yaml"); let model = parse_policy(&path).expect("failed to parse policy"); - let readable = model.filesystem_policy.readable_paths(); + let readable = model.filesystem_policy.readable_paths(None); assert!(readable.contains(&"/usr".to_owned())); assert!(readable.contains(&"/sandbox".to_owned())); assert!(readable.contains(&"/tmp".to_owned())); + assert!(readable.contains(&policy::WORKDIR_PATH_SYMBOL.to_owned())); } // 3. Workdir NOT included by default (matches runtime behavior). @@ -121,8 +122,9 @@ filesystem_policy: - /usr "; let model = policy::parse_policy_str(yaml).expect("parse"); - let readable = model.filesystem_policy.readable_paths(); + let readable = model.filesystem_policy.readable_paths(None); assert!(!readable.contains(&"/sandbox".to_owned())); + assert!(!readable.contains(&policy::WORKDIR_PATH_SYMBOL.to_owned())); } // 4. Workdir excluded when include_workdir: false. @@ -136,8 +138,9 @@ filesystem_policy: - /usr "; let model = policy::parse_policy_str(yaml).expect("parse"); - let readable = model.filesystem_policy.readable_paths(); + let readable = model.filesystem_policy.readable_paths(None); assert!(!readable.contains(&"/sandbox".to_owned())); + assert!(!readable.contains(&policy::WORKDIR_PATH_SYMBOL.to_owned())); } // 5. No duplicate when workdir already in read_write. @@ -152,12 +155,30 @@ filesystem_policy: - /tmp "; let model = policy::parse_policy_str(yaml).expect("parse"); - let readable = model.filesystem_policy.readable_paths(); + let readable = model.filesystem_policy.readable_paths(Some("/sandbox")); let sandbox_count = readable.iter().filter(|p| *p == "/sandbox").count(); assert_eq!(sandbox_count, 1); } - // 6. End-to-end: testdata policy with a github credential in scope and a + // 6. A resolved non-default workdir does not replace an explicit path. + #[test] + fn test_include_workdir_preserves_explicit_sandbox_path() { + let yaml = r" +version: 1 +filesystem_policy: + include_workdir: true + read_write: + - /sandbox +"; + let model = policy::parse_policy_str(yaml).expect("parse"); + let readable = model + .filesystem_policy + .readable_paths(Some("/workspace/project")); + assert!(readable.contains(&"/sandbox".to_owned())); + assert!(readable.contains(&"/workspace/project".to_owned())); + } + + // 7. End-to-end: testdata policy with a github credential in scope and a // bypass-L7 binary (git) emits an `l7_bypass_credentialed` finding. // The prover output is categorical, not severity-graded. #[test] diff --git a/crates/openshell-prover/src/model.rs b/crates/openshell-prover/src/model.rs index bf52993d47..3769b17e43 100644 --- a/crates/openshell-prover/src/model.rs +++ b/crates/openshell-prover/src/model.rs @@ -268,7 +268,7 @@ impl ReachabilityModel { } fn encode_filesystem(&mut self) { - for path in self.policy.filesystem_policy.readable_paths() { + for path in self.policy.filesystem_policy.readable_paths(None) { let var = Bool::new_const(format!("fs_readable_{path}")); self.solver.assert(&var); self.filesystem_readable.insert(path, var); diff --git a/crates/openshell-prover/src/policy.rs b/crates/openshell-prover/src/policy.rs index aa40d07560..599c8d131a 100644 --- a/crates/openshell-prover/src/policy.rs +++ b/crates/openshell-prover/src/policy.rs @@ -257,18 +257,26 @@ pub struct FilesystemPolicy { pub read_write: Vec, } +/// Symbol used when the prover does not know the image-resolved workspace. +/// +/// Keeping this distinct from `/sandbox` prevents the model from inventing a +/// literal compatibility workspace for images that declare another workdir. +pub const WORKDIR_PATH_SYMBOL: &str = ""; + impl FilesystemPolicy { /// All readable paths (union of `read_only` and `read_write`), with workdir - /// added when `include_workdir` is true and not already present. - pub fn readable_paths(&self) -> Vec { + /// added when `include_workdir` is true and not already present. When the + /// resolved workdir is unavailable, retain it as a symbolic path. + pub fn readable_paths(&self, resolved_workdir: Option<&str>) -> Vec { let mut paths: Vec = self .read_only .iter() .chain(self.read_write.iter()) .cloned() .collect(); - if self.include_workdir && !paths.iter().any(|p| p == "/sandbox") { - paths.push("/sandbox".to_owned()); + let workdir = resolved_workdir.unwrap_or(WORKDIR_PATH_SYMBOL); + if self.include_workdir && !paths.iter().any(|path| path == workdir) { + paths.push(workdir.to_owned()); } paths } diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 5ba41b9d7e..e30c9b8543 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -186,19 +186,32 @@ 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 and Podman - // fill only omitted policy fields from OCI Config.User. + // OpenShift retain their authoritative numeric pair; Docker fills only + // omitted policy fields from OCI Config.User. #[cfg(unix)] - let resolved_process_identity = { + let (resolved_process_identity, workspace) = { let driver_identity = openshell_supervisor_process::identity::DriverIdentity::from_env()?; - openshell_supervisor_process::identity::resolve_process_identity( + let use_workdir_as_home = matches!( + &driver_identity, + openshell_supervisor_process::identity::DriverIdentity::OciUser { .. } + ); + let resolved = openshell_supervisor_process::identity::resolve_process_identity( &mut policy, &driver_identity, - )? + )?; + ( + resolved, + openshell_supervisor_process::process::ResolvedWorkspace::new( + workdir.clone(), + use_workdir_as_home, + ), + ) }; #[cfg(not(unix))] - let resolved_process_identity = - openshell_supervisor_process::process::ResolvedProcessIdentity::default(); + let (resolved_process_identity, workspace) = ( + openshell_supervisor_process::process::ResolvedProcessIdentity::default(), + openshell_supervisor_process::process::ResolvedWorkspace::new(workdir.clone(), false), + ); #[cfg_attr(not(target_os = "linux"), allow(unused_mut))] let (provider_credentials, mut provider_env) = @@ -683,7 +696,7 @@ pub async fn run_sandbox( let process = openshell_supervisor_process::run::run_process( program, args, - workdir.as_deref(), + workspace, timeout_secs, interactive, sandbox_id.as_deref(), @@ -1151,9 +1164,9 @@ const PROXY_BASELINE_READ_ONLY: &[&str] = &[ "/dev/urandom", ]; -/// Minimum read-write paths required for a proxy-mode sandbox child process: -/// user working directory and temporary files. -const PROXY_BASELINE_READ_WRITE: &[&str] = &["/sandbox", "/tmp"]; +/// Minimum read-write paths required for a proxy-mode sandbox child process. +/// The active workspace is granted separately through `include_workdir`. +const PROXY_BASELINE_READ_WRITE: &[&str] = &["/tmp"]; /// GPU read-only paths. /// @@ -1502,10 +1515,10 @@ mod baseline_tests { } #[test] - fn baseline_read_write_always_includes_sandbox_and_tmp() { + fn baseline_read_write_does_not_hardcode_sandbox() { let (_ro, rw) = baseline_enrichment_paths(); - assert!(rw.contains(&"/sandbox".to_string())); assert!(rw.contains(&"/tmp".to_string())); + assert!(!rw.contains(&"/sandbox".to_string())); } #[test] diff --git a/crates/openshell-supervisor-process/src/identity.rs b/crates/openshell-supervisor-process/src/identity.rs index 6a9a785542..543d1245c8 100644 --- a/crates/openshell-supervisor-process/src/identity.rs +++ b/crates/openshell-supervisor-process/src/identity.rs @@ -332,6 +332,68 @@ fn find_group_by_name(path: &Path, name: &str) -> Result> { }) } +/// Resolve supplementary groups declared for an OCI named user without +/// consulting NSS. Numeric OCI users have no trustworthy group-membership +/// name and therefore receive no supplementary groups. +pub fn resolve_oci_supplementary_gids(declaration: &str, primary_gid: u32) -> Result> { + resolve_oci_supplementary_gids_at(declaration, primary_gid, Path::new(GROUP_PATH)) +} + +fn resolve_oci_supplementary_gids_at( + declaration: &str, + primary_gid: u32, + group_path: &Path, +) -> Result> { + let (user, _) = split_oci_declaration(declaration); + validate_component(user, "OCI user")?; + if user.parse::().is_ok() { + return Ok(Vec::new()); + } + + let content = read_account_file(group_path)?; + let mut gids = vec![primary_gid]; + for line in content.lines() { + if line.is_empty() || line.starts_with('#') { + continue; + } + if line.len() > MAX_ACCOUNT_LINE_SIZE { + return Err(miette::miette!( + "account file '{}' contains an oversized line", + group_path.display() + )); + } + let fields = line.split(':').collect::>(); + if fields.len() != 4 + || fields + .iter() + .any(|field| field.len() > MAX_ACCOUNT_FIELD_SIZE) + { + return Err(miette::miette!( + "group membership entry in '{}' is malformed", + group_path.display() + )); + } + if !fields[3].split(',').any(|member| member == user) { + continue; + } + let gid = fields[2].parse::().map_err(|_| { + miette::miette!( + "group membership GID in '{}' is malformed", + group_path.display() + ) + })?; + if gid == 0 { + return Err(miette::miette!( + "OCI user '{user}' is a member of prohibited GID 0" + )); + } + gids.push(gid); + } + gids.sort_unstable(); + gids.dedup(); + Ok(gids) +} + fn find_unique( path: &Path, mut select: impl FnMut(&[&str]) -> Option>, @@ -665,6 +727,35 @@ mod tests { ); } + #[test] + fn named_oci_user_resolves_bounded_supplementary_groups() { + let (_dir, _passwd, group) = account_files( + "", + "primary:x:1235:\nvideo:x:44:app,other\naudio:x:63:other\nrender:x:107:app\n", + ); + + let gids = resolve_oci_supplementary_gids_at("app:primary", 1235, &group).unwrap(); + assert_eq!(gids, vec![44, 107, 1235]); + } + + #[test] + fn numeric_oci_user_has_no_named_supplementary_groups() { + let dir = tempdir().unwrap(); + let missing_group = dir.path().join("missing-group"); + + let gids = resolve_oci_supplementary_gids_at("1234:1235", 1235, &missing_group).unwrap(); + assert!(gids.is_empty()); + } + + #[test] + fn oci_supplementary_membership_rejects_root_group() { + let (_dir, _passwd, group) = account_files("", "root:x:0:app\n"); + + let error = + resolve_oci_supplementary_gids_at("app", 1235, &group).expect_err("GID 0 must fail"); + assert!(error.to_string().contains("prohibited GID 0")); + } + #[test] fn missing_unknown_ambiguous_and_root_identities_fail() { let (_dir, passwd, group) = account_files( diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 93ab4787b3..c14df9e2df 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -19,6 +19,8 @@ use std::ffi::CString; use std::os::fd::{AsRawFd, OwnedFd, RawFd}; #[cfg(target_os = "linux")] use std::os::unix::ffi::OsStrExt; +#[cfg(unix)] +use std::os::unix::fs::{MetadataExt, PermissionsExt}; #[cfg(any(test, unix))] use std::path::Path; use std::path::PathBuf; @@ -79,6 +81,35 @@ impl ResolvedProcessIdentity { } } +/// Resolved process workspace and its child-environment semantics. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct ResolvedWorkspace { + root: Option, + use_as_home: bool, +} + +impl ResolvedWorkspace { + #[must_use] + pub fn new(root: Option, use_as_home: bool) -> Self { + Self { root, use_as_home } + } + + #[must_use] + pub fn root(&self) -> Option<&str> { + self.root.as_deref() + } + + #[must_use] + pub fn owned_root(&self) -> Option { + self.root.clone() + } + + #[must_use] + pub fn home(&self) -> Option<&str> { + self.use_as_home.then(|| self.root()).flatten() + } +} + impl ProcessEnforcementMode { #[must_use] pub const fn uses_privileged_process_setup(self) -> bool { @@ -527,7 +558,7 @@ impl ProcessHandle { pub fn spawn( program: &str, args: &[String], - workdir: Option<&str>, + workspace: &ResolvedWorkspace, interactive: bool, policy: &SandboxPolicy, resolved_identity: ResolvedProcessIdentity, @@ -539,7 +570,7 @@ impl ProcessHandle { Self::spawn_impl( program, args, - workdir, + workspace, interactive, policy, resolved_identity, @@ -560,7 +591,7 @@ impl ProcessHandle { pub fn spawn( program: &str, args: &[String], - workdir: Option<&str>, + workspace: &ResolvedWorkspace, interactive: bool, policy: &SandboxPolicy, resolved_identity: ResolvedProcessIdentity, @@ -571,7 +602,7 @@ impl ProcessHandle { Self::spawn_impl( program, args, - workdir, + workspace, interactive, policy, resolved_identity, @@ -586,7 +617,7 @@ impl ProcessHandle { fn spawn_impl( program: &str, args: &[String], - workdir: Option<&str>, + workspace: &ResolvedWorkspace, interactive: bool, policy: &SandboxPolicy, resolved_identity: ResolvedProcessIdentity, @@ -611,9 +642,12 @@ impl ProcessHandle { inject_provider_env(&mut cmd, provider_env); - if let Some(dir) = workdir { + if let Some(dir) = workspace.root() { cmd.current_dir(dir); } + if let Some(home) = workspace.home() { + cmd.env("HOME", home); + } if matches!(policy.network.mode, NetworkMode::Proxy) { let proxy = policy.network.proxy.as_ref().ok_or_else(|| { @@ -651,7 +685,7 @@ impl ProcessHandle { // pre_exec context cannot reliably emit structured logs. #[cfg(target_os = "linux")] if enforcement_mode.enforces_child_sandbox() { - sandbox::linux::log_sandbox_readiness(policy, workdir); + sandbox::linux::log_sandbox_readiness(policy, workspace.root()); } // Phase 1: Prepare Landlock ruleset by opening PathFds. @@ -660,7 +694,7 @@ impl ProcessHandle { // runs as the sandbox UID, so inaccessible paths are unavailable to // the workload and best-effort compatibility skips them. #[cfg(target_os = "linux")] - let prepared_sandbox = prepare_child_sandbox(policy, workdir, enforcement_mode) + let prepared_sandbox = prepare_child_sandbox(policy, workspace.root(), enforcement_mode) .map_err(|err| miette::miette!("Failed to prepare sandbox: {err}"))?; #[cfg(target_os = "linux")] let supervisor_identity_mount = if enforcement_mode.uses_privileged_process_setup() { @@ -741,7 +775,7 @@ impl ProcessHandle { fn spawn_impl( program: &str, args: &[String], - workdir: Option<&str>, + workspace: &ResolvedWorkspace, interactive: bool, policy: &SandboxPolicy, resolved_identity: ResolvedProcessIdentity, @@ -763,9 +797,12 @@ impl ProcessHandle { inject_provider_env(&mut cmd, provider_env); - if let Some(dir) = workdir { + if let Some(dir) = workspace.root() { cmd.current_dir(dir); } + if let Some(home) = workspace.home() { + cmd.env("HOME", home); + } if matches!(policy.network.mode, NetworkMode::Proxy) { let proxy = policy.network.proxy.as_ref().ok_or_else(|| { @@ -796,7 +833,7 @@ impl ProcessHandle { #[cfg(unix)] { let policy = policy.clone(); - let workdir = workdir.map(str::to_string); + let workdir = workspace.owned_root(); #[allow(unsafe_code)] unsafe { cmd.pre_exec(move || { @@ -1252,6 +1289,303 @@ fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result Ok(()) } +#[cfg(unix)] +fn prepare_oci_workspace( + root: &Path, + uid: Option, + gid: Option, + supplementary_gids: &[Gid], +) -> Result<()> { + prepare_oci_workspace_with(root, uid, gid, supplementary_gids, &nix::unistd::chown) +} + +/// Validate that selecting an image-provided OCI workdir does not grant the +/// sandbox identity any filesystem authority it lacked in the immutable image. +/// +/// Every path component must be a real directory (never a symlink), every +/// parent must already be traversable, and the final directory must already be +/// writable and traversable. No ownership or mode bits are changed. +#[cfg(unix)] +pub fn validate_oci_workspace( + root: &Path, + uid: Option, + gid: Option, + supplementary_gids: &[Gid], +) -> Result<()> { + let components = validated_workspace_components(root, false)?; + let mut current = PathBuf::from("/"); + validate_workspace_component(¤t, uid, gid, supplementary_gids, false)?; + let last_component = components.len().saturating_sub(1); + for (index, component) in components.into_iter().enumerate() { + current.push(component); + validate_workspace_component( + ¤t, + uid, + gid, + supplementary_gids, + index == last_component, + )?; + } + Ok(()) +} + +#[cfg(unix)] +fn validate_workspace_component( + path: &Path, + uid: Option, + gid: Option, + supplementary_gids: &[Gid], + is_workspace: bool, +) -> Result<()> { + let metadata = std::fs::symlink_metadata(path).map_err(|error| { + if error.kind() == std::io::ErrorKind::NotFound { + miette::miette!( + "image workspace path component '{}' does not exist", + path.display() + ) + } else { + miette::miette!( + "failed to inspect image workspace path component '{}': {error}", + path.display() + ) + } + })?; + if metadata.file_type().is_symlink() { + return Err(miette::miette!( + "workspace path component '{}' is a symlink — refusing to follow it", + path.display() + )); + } + if !metadata.is_dir() { + return Err(miette::miette!( + "workspace path component '{}' is not a directory", + 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 { + "writable and traversable" + } else { + "traversable" + }; + return Err(miette::miette!( + "workspace path component '{}' is not {requirement} by the sandbox identity in the image", + path.display() + )); + } + Ok(()) +} + +#[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(()) +} + +/// Prepare only the resolved `OpenShell` workspace directory itself. +/// +/// Image-provided children retain their declared ownership. This avoids +/// crossing symlinks or user-provided nested mounts. +#[cfg(unix)] +fn prepare_oci_workspace_with( + root: &Path, + uid: Option, + gid: Option, + supplementary_gids: &[Gid], + do_chown: &impl Fn(&Path, Option, Option) -> nix::Result<()>, +) -> Result<()> { + let components = validated_workspace_components(root, true)?; + + let last_component = components.len().saturating_sub(1); + let mut current = PathBuf::from("/"); + for (index, component) in components.into_iter().enumerate() { + current.push(component); + match std::fs::symlink_metadata(¤t) { + Ok(metadata) if metadata.file_type().is_symlink() => { + return Err(miette::miette!( + "workspace path component '{}' is a symlink — refusing to follow it", + current.display() + )); + } + Ok(metadata) if !metadata.is_dir() => { + return Err(miette::miette!( + "workspace path component '{}' is not a directory", + current.display() + )); + } + Ok(metadata) => { + if index != last_component + && !identity_can_traverse(&metadata, uid, gid, supplementary_gids) + { + return Err(miette::miette!( + "workspace parent '{}' is not traversable by the sandbox identity", + current.display() + )); + } + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + std::fs::create_dir(¤t).into_diagnostic()?; + std::fs::set_permissions(¤t, std::fs::Permissions::from_mode(0o755)) + .into_diagnostic()?; + } + Err(error) => return Err(error).into_diagnostic(), + } + } + + do_chown(root, uid, gid).into_diagnostic()?; + + let metadata = std::fs::symlink_metadata(root).into_diagnostic()?; + let mode = metadata.permissions().mode() & 0o7777; + if mode & 0o300 != 0o300 { + std::fs::set_permissions(root, std::fs::Permissions::from_mode(mode | 0o300)) + .into_diagnostic()?; + } + Ok(()) +} + +#[cfg(unix)] +fn validated_workspace_components( + root: &Path, + allow_managed_fallback: bool, +) -> Result> { + let root_str = root + .to_str() + .ok_or_else(|| miette::miette!("workspace path must be valid UTF-8"))?; + let validated_root = openshell_core::driver_mounts::resolve_oci_workspace_root(root_str) + .map_err(|error| miette::miette!(error))?; + if Path::new(&validated_root) != root + || (!allow_managed_fallback + && validated_root == openshell_core::driver_mounts::DEFAULT_WORKSPACE_ROOT) + { + return Err(miette::miette!( + "workspace path '{}' must be a normalized absolute {}path", + root.display(), + if allow_managed_fallback { + "non-root " + } else { + "non-fallback " + } + )); + } + + root.components() + .skip(1) + .map(|component| match component { + std::path::Component::Normal(component) => Ok(component.to_os_string()), + _ => Err(miette::miette!( + "workspace path '{}' must be normalized", + root.display() + )), + }) + .collect() +} + +#[cfg(unix)] +fn identity_can_traverse( + metadata: &std::fs::Metadata, + uid: Option, + gid: Option, + supplementary_gids: &[Gid], +) -> bool { + identity_has_permissions(metadata, uid, gid, supplementary_gids, 0o1) +} + +#[cfg(unix)] +fn identity_has_permissions( + metadata: &std::fs::Metadata, + uid: Option, + gid: Option, + supplementary_gids: &[Gid], + required: u32, +) -> bool { + let user_id = uid.unwrap_or_else(nix::unistd::geteuid).as_raw(); + if user_id == 0 { + return true; + } + + let group_id = gid.unwrap_or_else(nix::unistd::getegid).as_raw(); + let mode = metadata.permissions().mode(); + if metadata.uid() == user_id { + mode & (required << 6) == required << 6 + } else if metadata.gid() == group_id + || supplementary_gids + .iter() + .any(|supplementary_gid| supplementary_gid.as_raw() == metadata.gid()) + { + mode & (required << 3) == required << 3 + } else { + mode & required == required + } +} + +#[cfg(not(any( + target_os = "aix", + target_os = "haiku", + target_os = "illumos", + target_os = "ios", + target_os = "macos", + target_os = "redox", + target_os = "solaris" +)))] +fn named_user_supplementary_groups(user_name: &str, primary_gid: Gid) -> Result> { + let user_name = CString::new(user_name).map_err(|_| miette::miette!("Invalid user name"))?; + nix::unistd::getgrouplist(user_name.as_c_str(), primary_gid).into_diagnostic() +} + +#[cfg(any( + target_os = "aix", + target_os = "haiku", + target_os = "illumos", + target_os = "ios", + target_os = "macos", + target_os = "redox", + target_os = "solaris" +))] +#[allow(clippy::unnecessary_wraps)] +fn named_user_supplementary_groups(_user_name: &str, _primary_gid: Gid) -> Result> { + // Privilege dropping does not call initgroups on these targets. + Ok(Vec::new()) +} + #[cfg(unix)] fn chown_children( dir: &Path, @@ -1315,54 +1649,53 @@ 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()) + prepare_filesystem_with_identity(policy, ResolvedProcessIdentity::default(), None, false) } #[cfg(unix)] pub fn prepare_filesystem_with_identity( policy: &SandboxPolicy, resolved_identity: ResolvedProcessIdentity, + workdir: Option<&str>, + prepare_workspace: bool, ) -> Result<()> { use nix::unistd::chown; - use nix::unistd::{Gid, Uid}; - - let user_name = match policy.process.run_as_user.as_deref() { - Some(name) if !name.is_empty() => Some(name), - _ => None, - }; - let group_name = match policy.process.run_as_group.as_deref() { - Some(name) if !name.is_empty() => Some(name), - _ => None, - }; // If no user/group configured, nothing to do - if user_name.is_none() && group_name.is_none() { + if policy + .process + .run_as_user + .as_deref() + .is_none_or(str::is_empty) + && policy + .process + .run_as_group + .as_deref() + .is_none_or(str::is_empty) + { return Ok(()); } - // Resolve UID: numeric values are passed directly; names resolve via passwd. - let uid = match resolved_identity.uid() { - Some(uid) => Some(Uid::from_raw(uid)), - None => match user_name { - Some(name) if name.parse::().is_ok() => { - Some(Uid::from_raw(name.parse().into_diagnostic()?)) - } - Some(name) => User::from_name(name).into_diagnostic()?.map(|u| u.uid), - _ => None, - }, - }; + let (uid, gid, supplementary_gids) = resolve_filesystem_identity(policy, resolved_identity)?; - // Resolve GID: numeric values are passed directly; names resolve via group. - let gid = match resolved_identity.gid() { - Some(gid) => Some(Gid::from_raw(gid)), - None => match group_name { - Some(name) if name.parse::().is_ok() => { - Some(Gid::from_raw(name.parse().into_diagnostic()?)) - } - Some(name) => Group::from_name(name).into_diagnostic()?.map(|g| g.gid), - _ => None, - }, - }; + // 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. + 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) { + info!(path = %workspace.display(), ?uid, ?gid, "Preparing managed workspace"); + prepare_oci_workspace(workspace, uid, gid, &supplementary_gids)?; + } else { + info!(path = %workspace.display(), ?uid, ?gid, "Validating image workspace authority"); + validate_oci_workspace(workspace, uid, gid, &supplementary_gids)?; + } + } // Create missing read_write paths and only chown the ones we created. for path in &policy.filesystem.read_write { @@ -1378,8 +1711,8 @@ pub fn prepare_filesystem_with_identity( } // Retain the existing Kubernetes/OpenShift behavior for driver-injected - // numeric identities. Docker and Podman clear this variable and do not - // receive identity-specific workspace preparation. + // numeric identities. Docker clears this variable and does 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() { @@ -1391,6 +1724,72 @@ pub fn prepare_filesystem_with_identity( Ok(()) } +#[cfg(unix)] +fn resolve_filesystem_identity( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, +) -> Result<(Option, Option, Vec)> { + let user_name = policy + .process + .run_as_user + .as_deref() + .filter(|name| !name.is_empty()); + let group_name = policy + .process + .run_as_group + .as_deref() + .filter(|name| !name.is_empty()); + + let uid = match resolved_identity.uid() { + Some(uid) => Some(Uid::from_raw(uid)), + None => match user_name { + Some(name) if name.parse::().is_ok() => { + Some(Uid::from_raw(name.parse().into_diagnostic()?)) + } + Some(name) => User::from_name(name).into_diagnostic()?.map(|u| u.uid), + _ => None, + }, + }; + + // Resolve GID: numeric values are passed directly; names resolve via group. + let gid = match resolved_identity.gid() { + Some(gid) => Some(Gid::from_raw(gid)), + None => match group_name { + Some(name) if name.parse::().is_ok() => { + Some(Gid::from_raw(name.parse().into_diagnostic()?)) + } + Some(name) => Group::from_name(name).into_diagnostic()?.map(|g| g.gid), + _ => None, + }, + }; + + let supplementary_gids = match user_name { + Some(name) if name.parse::().is_err() => { + let primary_gid = if let Some(gid) = gid { + gid + } else { + let uid = + uid.ok_or_else(|| miette::miette!("Failed to resolve sandbox user '{name}'"))?; + User::from_uid(uid) + .into_diagnostic()? + .ok_or_else(|| miette::miette!("Failed to resolve user from UID {uid}"))? + .gid + }; + if resolved_identity.uid().is_some() { + crate::identity::resolve_oci_supplementary_gids(name, primary_gid.as_raw())? + .into_iter() + .map(Gid::from_raw) + .collect() + } else { + named_user_supplementary_groups(name, primary_gid)? + } + } + _ => Vec::new(), + }; + + Ok((uid, gid, supplementary_gids)) +} + #[cfg(not(unix))] pub fn prepare_filesystem(_policy: &SandboxPolicy) -> Result<()> { Ok(()) @@ -1404,19 +1803,6 @@ pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> { drop_privileges_with_identity(policy, ResolvedProcessIdentity::default()) } -#[cfg(unix)] -fn should_clear_supplementary_groups( - current_uid: Uid, - target_uid: Uid, - user_name: Option<&str>, - resolved_identity: ResolvedProcessIdentity, -) -> bool { - resolved_identity.uses_oci_user_fallback() - && target_uid != current_uid - && !(user_name.is_some_and(|name| name.parse::().is_err()) - && resolved_identity.uid().is_none()) -} - #[cfg(unix)] #[allow(clippy::similar_names)] pub fn drop_privileges_with_identity( @@ -1512,23 +1898,21 @@ pub fn drop_privileges_with_identity( }; if target_uid != nix::unistd::geteuid() { - if should_clear_supplementary_groups( - nix::unistd::geteuid(), - target_uid, - user_name, - resolved_identity, - ) { - // OCI-derived users do not have a trustworthy NSS - // supplementary-group source. Clear the root supervisor's - // inherited groups before changing UID/GID. Platform-resolved and - // explicit numeric identities retain their pre-OCI behavior. + if resolved_identity.uses_oci_user_fallback() { + // OCI named users use the bounded /etc/group parser shared with + // workspace validation. Numeric OCI users resolve to an empty + // list. Never retain the root supervisor's inherited groups. #[cfg(not(any( target_os = "macos", target_os = "ios", target_os = "haiku", target_os = "redox" )))] - nix::unistd::setgroups(&[]).into_diagnostic()?; + { + let (_, _, supplementary_gids) = + resolve_filesystem_identity(policy, resolved_identity)?; + nix::unistd::setgroups(&supplementary_gids).into_diagnostic()?; + } } else if let Some(ref user_name) = initgroups_name { let user_cstr = CString::new(user_name.as_str()) .map_err(|_| miette::miette!("Invalid user name"))?; @@ -1755,43 +2139,6 @@ mod tests { assert!(validate_sandbox_group_with_identity(&policy, resolved).is_ok()); } - #[test] - #[cfg(unix)] - fn only_oci_numeric_user_paths_clear_supplementary_groups_before_uid_drop() { - let current_uid = Uid::from_raw(0); - let target_uid = Uid::from_raw(1234); - - assert!(!should_clear_supplementary_groups( - current_uid, - target_uid, - Some("1234"), - ResolvedProcessIdentity::default(), - )); - assert!(should_clear_supplementary_groups( - current_uid, - target_uid, - Some("1234"), - ResolvedProcessIdentity::new(None, Some(1235)), - )); - } - - #[test] - #[cfg(unix)] - fn supplementary_group_clearing_preserves_explicit_named_user_behavior() { - assert!(!should_clear_supplementary_groups( - Uid::from_raw(0), - Uid::from_raw(1234), - Some("app"), - ResolvedProcessIdentity::default(), - )); - assert!(!should_clear_supplementary_groups( - Uid::from_raw(1234), - Uid::from_raw(1234), - Some("1234"), - ResolvedProcessIdentity::new(None, Some(1235)), - )); - } - #[test] fn full_enforcement_uses_privileged_setup_and_child_sandbox() { assert!(ProcessEnforcementMode::Full.uses_privileged_process_setup()); @@ -2437,6 +2784,418 @@ mod tests { assert!(result.is_err(), "non-EROFS errors should propagate"); } + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_chowns_only_root() { + use std::sync::{Arc, Mutex}; + + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap().join("sandbox"); + std::fs::create_dir(&root).unwrap(); + let child = root.join("image-content.txt"); + std::fs::write(&child, "image-owned").unwrap(); + + let chowned = Arc::new(Mutex::new(Vec::new())); + let observed = Arc::clone(&chowned); + let fake_chown = + move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + observed.lock().unwrap().push(path.to_path_buf()); + Ok(()) + }; + + prepare_oci_workspace_with( + &root, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + &fake_chown, + ) + .expect("workspace root should be prepared"); + + assert_eq!(*chowned.lock().unwrap(), vec![root]); + assert!(child.exists(), "image-provided child should be untouched"); + } + + #[cfg(unix)] + #[test] + fn validate_oci_workspace_accepts_existing_owner_writable_directory() { + let dir = tempfile::tempdir().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( + &root, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .expect("image owner already has write and traverse authority"); + } + + #[cfg(unix)] + #[test] + fn validate_oci_workspace_accepts_supplementary_group_write_authority() { + 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("project"); + std::fs::create_dir(&root).unwrap(); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o070)).unwrap(); + let metadata = std::fs::symlink_metadata(&root).unwrap(); + + validate_oci_workspace( + &root, + Some(Uid::from_raw(metadata.uid().wrapping_add(1))), + Some(Gid::from_raw(metadata.gid().wrapping_add(1))), + &[Gid::from_raw(metadata.gid())], + ) + .expect("supplementary group already has write and traverse authority"); + } + + #[cfg(unix)] + #[test] + fn validate_oci_workspace_rejects_unwritable_directory() { + 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("project"); + std::fs::create_dir(&root).unwrap(); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o755)).unwrap(); + let metadata = std::fs::symlink_metadata(&root).unwrap(); + + let error = validate_oci_workspace( + &root, + Some(Uid::from_raw(metadata.uid().wrapping_add(1))), + Some(Gid::from_raw(metadata.gid().wrapping_add(1))), + &[], + ) + .unwrap_err(); + assert!(error.to_string().contains("not writable and traversable")); + } + + #[cfg(unix)] + #[test] + fn validate_oci_workspace_rejects_missing_path() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap().join("missing"); + + let error = validate_oci_workspace( + &root, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .unwrap_err(); + assert!(error.to_string().contains("does not exist")); + } + + #[cfg(unix)] + #[test] + fn validate_oci_workspace_rejects_restrictive_parent() { + let dir = tempfile::tempdir().unwrap(); + let parent = dir.path().canonicalize().unwrap().join("private"); + let root = parent.join("project"); + std::fs::create_dir_all(&root).unwrap(); + std::fs::set_permissions(&parent, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o777)).unwrap(); + let metadata = std::fs::symlink_metadata(&parent).unwrap(); + + let error = validate_oci_workspace( + &root, + Some(Uid::from_raw(metadata.uid().wrapping_add(1))), + Some(Gid::from_raw(metadata.gid().wrapping_add(1))), + &[], + ) + .unwrap_err(); + assert!(error.to_string().contains("not traversable")); + } + + #[cfg(unix)] + #[test] + fn validate_oci_workspace_rejects_symlink_component() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let base = dir.path().canonicalize().unwrap(); + let target = base.join("target"); + let link = base.join("link"); + std::fs::create_dir(&target).unwrap(); + symlink(&target, &link).unwrap(); + + let error = validate_oci_workspace( + &link, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .unwrap_err(); + assert!(error.to_string().contains("symlink")); + } + + #[cfg(target_os = "linux")] + #[test] + fn validate_oci_workspace_rejects_kernel_filesystem_by_metadata() { + let error = validate_oci_workspace( + Path::new("/proc"), + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .unwrap_err(); + assert!(error.to_string().contains("kernel-managed filesystem")); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_makes_existing_root_owner_writable() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap().join("sandbox"); + std::fs::create_dir(&root).unwrap(); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o555)).unwrap(); + + prepare_oci_workspace_with(&root, None, None, &[], &|_, _, _| Ok(())) + .expect("read-only workspace root should be prepared"); + + let mode = std::fs::symlink_metadata(&root) + .unwrap() + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o755); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_rejects_symlink_root() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let base = dir.path().canonicalize().unwrap(); + let target = base.join("real"); + let link = base.join("link"); + std::fs::create_dir(&target).unwrap(); + symlink(&target, &link).unwrap(); + + let err = prepare_oci_workspace( + &link, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .unwrap_err(); + assert!( + err.to_string().contains("symlink"), + "expected symlink rejection: {err}" + ); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_rejects_symlink_parent() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let base = dir.path().canonicalize().unwrap(); + let target = base.join("real"); + let parent_link = base.join("parent-link"); + std::fs::create_dir(&target).unwrap(); + symlink(&target, &parent_link).unwrap(); + + let err = prepare_oci_workspace( + &parent_link.join("workspace"), + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .unwrap_err(); + assert!( + err.to_string().contains("symlink"), + "expected parent symlink rejection: {err}" + ); + assert!( + !target.join("workspace").exists(), + "workspace must not be created through a symlink parent" + ); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_rejects_parent_traversal() { + let err = prepare_oci_workspace( + Path::new("/tmp/workspace/../escape"), + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .unwrap_err(); + assert!( + err.to_string().contains("must be normalized"), + "expected traversal rejection: {err}" + ); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_rejects_inaccessible_existing_parent() { + let dir = tempfile::tempdir().unwrap(); + let parent = dir.path().canonicalize().unwrap().join("workspace"); + std::fs::create_dir(&parent).unwrap(); + std::fs::set_permissions(&parent, std::fs::Permissions::from_mode(0o700)).unwrap(); + let metadata = std::fs::symlink_metadata(&parent).unwrap(); + let different_user = Uid::from_raw(metadata.uid().wrapping_add(1)); + let different_group = Gid::from_raw(metadata.gid().wrapping_add(1)); + let root = parent.join("project"); + + let error = prepare_oci_workspace_with( + &root, + Some(different_user), + Some(different_group), + &[], + &|_, _, _| Ok(()), + ) + .unwrap_err(); + + assert!( + error.to_string().contains("is not traversable"), + "unexpected error: {error}" + ); + assert!( + !root.exists(), + "workspace must not be created below an inaccessible parent" + ); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_accepts_supplementary_group_parent() { + let dir = tempfile::tempdir_in("/tmp").unwrap(); + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o710)).unwrap(); + let parent = dir.path().canonicalize().unwrap().join("workspace"); + std::fs::create_dir(&parent).unwrap(); + std::fs::set_permissions(&parent, std::fs::Permissions::from_mode(0o710)).unwrap(); + let metadata = std::fs::symlink_metadata(&parent).unwrap(); + let different_user = Uid::from_raw(metadata.uid().wrapping_add(1)); + let different_group = Gid::from_raw(metadata.gid().wrapping_add(1)); + let supplementary_group = Gid::from_raw(metadata.gid()); + let root = parent.join("project"); + + prepare_oci_workspace_with( + &root, + Some(different_user), + Some(different_group), + &[supplementary_group], + &|_, _, _| Ok(()), + ) + .expect("supplementary group execute permission should allow traversal"); + + assert!(root.is_dir()); + } + + #[cfg(not(any( + target_os = "aix", + target_os = "haiku", + target_os = "illumos", + target_os = "ios", + target_os = "macos", + target_os = "redox", + target_os = "solaris" + )))] + #[test] + fn named_user_supplementary_groups_include_primary_group() { + let user = User::from_uid(nix::unistd::geteuid()) + .expect("resolve current UID") + .expect("current user exists"); + + let groups = named_user_supplementary_groups(&user.name, user.gid) + .expect("resolve named-user supplementary groups"); + + assert!(groups.contains(&user.gid)); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_rejects_non_directory_root() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap().join("sandbox"); + std::fs::write(&root, "not a directory").unwrap(); + + let error = prepare_oci_workspace( + &root, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + ) + .unwrap_err(); + assert!( + error.to_string().contains("is not a directory"), + "unexpected error: {error}" + ); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_propagates_root_chown_error() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().canonicalize().unwrap().join("sandbox"); + std::fs::create_dir(&root).unwrap(); + let fake_chown = |_path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + Err(nix::errno::Errno::EROFS) + }; + + let error = prepare_oci_workspace_with( + &root, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + &fake_chown, + ) + .unwrap_err(); + + assert!( + error.to_string().contains("Read-only file system"), + "unexpected error: {error}" + ); + } + + #[cfg(unix)] + #[test] + fn prepare_oci_workspace_creates_missing_root() { + use std::sync::{Arc, Mutex}; + + let dir = tempfile::tempdir().unwrap(); + let missing = dir + .path() + .canonicalize() + .unwrap() + .join("missing") + .join("sandbox"); + let chowned = Arc::new(Mutex::new(Vec::new())); + let observed = Arc::clone(&chowned); + let fake_chown = + move |path: &Path, _uid: Option, _gid: Option| -> nix::Result<()> { + observed.lock().unwrap().push(path.to_path_buf()); + Ok(()) + }; + + prepare_oci_workspace_with( + &missing, + Some(nix::unistd::geteuid()), + Some(nix::unistd::getegid()), + &[], + &fake_chown, + ) + .expect("missing OCI workspace should be created"); + + assert!(missing.is_dir()); + assert_eq!( + std::fs::symlink_metadata(missing.parent().unwrap()) + .unwrap() + .permissions() + .mode() + & 0o777, + 0o755 + ); + assert_eq!(*chowned.lock().unwrap(), vec![missing]); + } + #[cfg(unix)] #[test] fn rewrite_passwd_modifies_existing_sandbox_entry() { diff --git a/crates/openshell-supervisor-process/src/run.rs b/crates/openshell-supervisor-process/src/run.rs index a5ff0456c9..91e56b7ec8 100644 --- a/crates/openshell-supervisor-process/src/run.rs +++ b/crates/openshell-supervisor-process/src/run.rs @@ -36,6 +36,7 @@ use openshell_core::denial::DenialEvent; use crate::managed_children; use crate::process::{ ProcessEnforcementMode, ProcessHandle, ProcessStatus, ResolvedProcessIdentity, + ResolvedWorkspace, }; fn ocsf_ctx() -> &'static openshell_ocsf::SandboxContext { @@ -53,7 +54,7 @@ fn ocsf_ctx() -> &'static openshell_ocsf::SandboxContext { pub async fn run_process( program: &str, args: &[String], - workdir: Option<&str>, + workspace: ResolvedWorkspace, timeout_secs: u64, interactive: bool, sandbox_id: Option<&str>, @@ -95,7 +96,12 @@ pub async fn run_process( // is forked so the workload sees writable paths it owns. #[cfg(unix)] if enforcement_mode.uses_privileged_process_setup() { - crate::process::prepare_filesystem_with_identity(policy, resolved_process_identity)?; + crate::process::prepare_filesystem_with_identity( + policy, + resolved_process_identity, + workspace.root(), + workspace.home().is_some(), + )?; } // Eagerly fetch initial settings and install the agent skill if the @@ -225,7 +231,7 @@ pub async fn run_process( let ssh_socket_path: Option = ssh_socket_path.map(std::path::PathBuf::from); if let Some(listen_path) = ssh_socket_path.clone() { let policy_clone = policy.clone(); - let workdir_clone = workdir.map(str::to_string); + let workspace_clone = workspace.clone(); let proxy_url = ssh_proxy_url; let netns_fd = ssh_netns_fd; let ca_paths = ca_file_paths.clone(); @@ -243,7 +249,7 @@ pub async fn run_process( listen_path, ssh_ready_tx, policy_clone, - workdir_clone, + workspace_clone, netns_fd, proxy_url, ca_paths, @@ -319,7 +325,7 @@ pub async fn run_process( let mut handle = ProcessHandle::spawn( program, args, - workdir, + &workspace, interactive, policy, resolved_process_identity, @@ -333,7 +339,7 @@ pub async fn run_process( let mut handle = ProcessHandle::spawn( program, args, - workdir, + &workspace, interactive, policy, resolved_process_identity, diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 7fa6482a99..03dd7517fb 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -7,8 +7,8 @@ use crate::child_env; #[cfg(target_os = "linux")] use crate::managed_children; use crate::process::{ - ProcessEnforcementMode, ResolvedProcessIdentity, drop_privileges_with_identity, - is_supervisor_only_env_var, + ProcessEnforcementMode, ResolvedProcessIdentity, ResolvedWorkspace, + drop_privileges_with_identity, is_supervisor_only_env_var, }; use crate::sandbox; use miette::{IntoDiagnostic, Result}; @@ -111,7 +111,7 @@ pub async fn run_ssh_server( listen_path: PathBuf, ready_tx: tokio::sync::oneshot::Sender>, policy: SandboxPolicy, - workdir: Option, + workspace: ResolvedWorkspace, netns_fd: Option, proxy_url: Option, ca_file_paths: Option<(PathBuf, PathBuf)>, @@ -145,7 +145,7 @@ pub async fn run_ssh_server( let (stream, _peer) = listener.accept().await.into_diagnostic()?; let config = config.clone(); let policy = policy.clone(); - let workdir = workdir.clone(); + let workspace = workspace.clone(); let proxy_url = proxy_url.clone(); let ca_paths = ca_paths.clone(); let provider_credentials = provider_credentials.clone(); @@ -156,7 +156,7 @@ pub async fn run_ssh_server( stream, config, policy, - workdir, + workspace, netns_fd, proxy_url, ca_paths, @@ -185,7 +185,7 @@ async fn handle_connection( stream: tokio::net::UnixStream, config: Arc, policy: SandboxPolicy, - workdir: Option, + workspace: ResolvedWorkspace, netns_fd: Option, proxy_url: Option, ca_file_paths: Option>, @@ -210,7 +210,7 @@ async fn handle_connection( let handler = SshHandler::new( policy, - workdir, + workspace, netns_fd, proxy_url, ca_file_paths, @@ -240,7 +240,7 @@ struct ChannelState { struct SshHandler { policy: SandboxPolicy, - workdir: Option, + workspace: ResolvedWorkspace, netns_fd: Option, proxy_url: Option, ca_file_paths: Option>, @@ -255,7 +255,7 @@ impl SshHandler { #[allow(clippy::too_many_arguments)] fn new( policy: SandboxPolicy, - workdir: Option, + workspace: ResolvedWorkspace, netns_fd: Option, proxy_url: Option, ca_file_paths: Option>, @@ -266,7 +266,7 @@ impl SshHandler { ) -> Self { Self { policy, - workdir, + workspace, netns_fd, proxy_url, ca_file_paths, @@ -488,7 +488,7 @@ impl russh::server::Handler for SshHandler { // transfer files into and out of the sandbox. let input_sender = spawn_pipe_exec( &self.policy, - self.workdir.clone(), + &self.workspace, Some("/usr/lib/openssh/sftp-server".to_string()), session.handle(), channel, @@ -585,7 +585,7 @@ impl SshHandler { // exec that explicitly asked for a terminal). let (pty_master, input_sender) = spawn_pty_shell( &self.policy, - self.workdir.clone(), + &self.workspace, command, &pty, handle, @@ -606,7 +606,7 @@ impl SshHandler { // path VSCode Remote-SSH exec commands take. let input_sender = spawn_pipe_exec( &self.policy, - self.workdir.clone(), + &self.workspace, command, handle, channel, @@ -699,28 +699,31 @@ impl Default for PtyRequest { /// For name-based identities, looks up the home directory via `/etc/passwd` /// (or defaults to `/home/{user}`). /// -/// For numeric UIDs, there is no passwd entry — falls back to -/// `("{uid}", "/sandbox")` so the agent session still has a meaningful -/// USER identifier. -fn session_user_and_home(policy: &SandboxPolicy) -> (String, String) { - match policy.process.run_as_user.as_deref() { +/// For numeric UIDs, there is no passwd entry, so the default remains +/// `("{uid}", "/sandbox")`. Docker replaces that default with its 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() => { // Numeric UID — no passwd entry expected; use default HOME. if user.parse::().is_ok() { - return (user.to_string(), "/sandbox".to_string()); + (user.to_string(), "/sandbox".to_string()) + } else { + // Name-based identity — look up home from /etc/passwd. + let home = nix::unistd::User::from_name(user) + .ok() + .flatten() + .map_or_else( + || format!("/home/{user}"), + |u| u.dir.to_string_lossy().into_owned(), + ); + (user.to_string(), home) } - // Name-based identity — look up home from /etc/passwd. - let home = nix::unistd::User::from_name(user) - .ok() - .flatten() - .map_or_else( - || format!("/home/{user}"), - |u| u.dir.to_string_lossy().into_owned(), - ); - (user.to_string(), home) } _ => ("sandbox".to_string(), "/sandbox".to_string()), - } + }; + let home = workdir_home.map_or(default_home, str::to_string); + (user, home) } #[allow(clippy::too_many_arguments)] @@ -773,7 +776,7 @@ fn apply_child_env( #[allow(clippy::too_many_arguments)] fn spawn_pty_shell( policy: &SandboxPolicy, - workdir: Option, + workspace: &ResolvedWorkspace, command: Option, pty: &PtyRequest, handle: Handle, @@ -824,7 +827,7 @@ fn spawn_pty_shell( // Derive USER and HOME from the policy's run_as_user when available, // falling back to "sandbox" / "/sandbox" for backward compatibility. - let (session_user, session_home) = session_user_and_home(policy); + let (session_user, session_home) = session_user_and_home(policy, workspace.home()); apply_child_env( &mut cmd, &session_home, @@ -837,20 +840,20 @@ fn spawn_pty_shell( ); cmd.stdin(stdin).stdout(stdout).stderr(stderr); - if let Some(dir) = workdir.as_deref() { + if let Some(dir) = workspace.root() { cmd.current_dir(dir); } // Probe Landlock availability from the parent process where tracing works. #[cfg(target_os = "linux")] if enforcement_mode.enforces_child_sandbox() { - sandbox::linux::log_sandbox_readiness(policy, workdir.as_deref()); + sandbox::linux::log_sandbox_readiness(policy, workspace.root()); } // Phase 1: Prepare Landlock ruleset before the child applies it. #[cfg(target_os = "linux")] let prepared_sandbox = - crate::process::prepare_child_sandbox(policy, workdir.as_deref(), enforcement_mode) + crate::process::prepare_child_sandbox(policy, workspace.root(), enforcement_mode) .map_err(|err| anyhow::anyhow!("Failed to prepare sandbox: {err}"))?; #[cfg(unix)] @@ -858,7 +861,7 @@ fn spawn_pty_shell( unsafe_pty::install_pre_exec( &mut cmd, policy.clone(), - workdir.clone(), + workspace.owned_root(), slave_fd, netns_fd, resolved_identity, @@ -946,7 +949,7 @@ fn spawn_pty_shell( #[allow(clippy::too_many_arguments)] fn spawn_pipe_exec( policy: &SandboxPolicy, - workdir: Option, + workspace: &ResolvedWorkspace, command: Option, handle: Handle, channel: ChannelId, @@ -978,7 +981,7 @@ fn spawn_pipe_exec( }, ); - let (session_user, session_home) = session_user_and_home(policy); + let (session_user, session_home) = session_user_and_home(policy, workspace.home()); apply_child_env( &mut cmd, &session_home, @@ -993,20 +996,20 @@ fn spawn_pipe_exec( .stdout(Stdio::piped()) .stderr(Stdio::piped()); - if let Some(dir) = workdir.as_deref() { + if let Some(dir) = workspace.root() { cmd.current_dir(dir); } // Probe Landlock availability from the parent process where tracing works. #[cfg(target_os = "linux")] if enforcement_mode.enforces_child_sandbox() { - sandbox::linux::log_sandbox_readiness(policy, workdir.as_deref()); + sandbox::linux::log_sandbox_readiness(policy, workspace.root()); } // Phase 1: Prepare Landlock ruleset before the child applies it. #[cfg(target_os = "linux")] let prepared_sandbox = - crate::process::prepare_child_sandbox(policy, workdir.as_deref(), enforcement_mode) + crate::process::prepare_child_sandbox(policy, workspace.root(), enforcement_mode) .map_err(|err| anyhow::anyhow!("Failed to prepare sandbox: {err}"))?; #[cfg(unix)] @@ -1014,7 +1017,7 @@ fn spawn_pipe_exec( unsafe_pty::install_pre_exec_no_pty( &mut cmd, policy.clone(), - workdir.clone(), + workspace.owned_root(), netns_fd, resolved_identity, enforcement_mode, @@ -1693,12 +1696,33 @@ mod tests { run_as_group: None, }, }; - let (user, home) = session_user_and_home(&policy); + let (user, home) = session_user_and_home(&policy, None); assert_eq!(user, "1000"); // Numeric UID has no passwd entry — defaults to /sandbox. assert_eq!(home, "/sandbox"); } + #[test] + fn session_user_and_home_uses_driver_workspace_when_supplied() { + use openshell_core::policy::{ + FilesystemPolicy, LandlockPolicy, NetworkPolicy, ProcessPolicy, + }; + let policy = SandboxPolicy { + version: 1, + filesystem: FilesystemPolicy::default(), + network: NetworkPolicy::default(), + landlock: LandlockPolicy::default(), + process: ProcessPolicy { + run_as_user: Some("1234".into()), + run_as_group: Some("1235".into()), + }, + }; + + let (user, home) = session_user_and_home(&policy, Some("/workspace/project")); + assert_eq!(user, "1234"); + assert_eq!(home, "/workspace/project"); + } + #[test] fn session_user_and_home_returns_name_from_passwd() { use openshell_core::policy::{ @@ -1714,7 +1738,7 @@ mod tests { run_as_group: None, }, }; - let (user, home) = session_user_and_home(&policy); + let (user, home) = session_user_and_home(&policy, None); assert_eq!(user, "sandbox"); // Name-based — should resolve via passwd (or /home/{user}). assert!(!home.is_empty()); @@ -1735,7 +1759,7 @@ mod tests { run_as_group: None, }, }; - let (user, home) = session_user_and_home(&policy); + let (user, home) = session_user_and_home(&policy, None); assert_eq!(user, "sandbox"); assert_eq!(home, "/sandbox"); } @@ -1755,7 +1779,7 @@ mod tests { run_as_group: None, }, }; - let (user, home) = session_user_and_home(&policy); + let (user, home) = session_user_and_home(&policy, None); assert_eq!(user, "sandbox"); assert_eq!(home, "/sandbox"); } @@ -1775,7 +1799,7 @@ mod tests { run_as_group: None, }, }; - let (user, home) = session_user_and_home(&policy); + let (user, home) = session_user_and_home(&policy, None); assert_eq!(user, "1000660000"); assert_eq!(home, "/sandbox"); } diff --git a/docs/get-started/tutorials/first-network-policy.mdx b/docs/get-started/tutorials/first-network-policy.mdx index 178bd2f095..05888278ba 100644 --- a/docs/get-started/tutorials/first-network-policy.mdx +++ b/docs/get-started/tutorials/first-network-policy.mdx @@ -97,7 +97,7 @@ version: 1 filesystem_policy: include_workdir: true read_only: [/usr, /lib, /proc, /dev/urandom, /app, /etc, /var/log] - read_write: [/sandbox, /tmp, /dev/null] + read_write: [/tmp, /dev/null] landlock: compatibility: best_effort diff --git a/docs/get-started/tutorials/github-sandbox.mdx b/docs/get-started/tutorials/github-sandbox.mdx index 0b11b39345..89b6c0885e 100644 --- a/docs/get-started/tutorials/github-sandbox.mdx +++ b/docs/get-started/tutorials/github-sandbox.mdx @@ -180,7 +180,6 @@ filesystem_policy: - /etc - /var/log read_write: - - /sandbox - /tmp - /dev/null diff --git a/docs/reference/policy-schema.mdx b/docs/reference/policy-schema.mdx index 9c4e877eaf..8e50d004b2 100644 --- a/docs/reference/policy-schema.mdx +++ b/docs/reference/policy-schema.mdx @@ -52,7 +52,7 @@ Controls filesystem access inside the sandbox. Paths not listed in either `read_ |---|---|---|---| | `include_workdir` | bool | No | When `true`, automatically adds the agent's working directory to `read_write`. | | `read_only` | list of strings | No | Paths the agent can read but not modify. Typically system directories like `/usr`, `/lib`, `/etc`. | -| `read_write` | list of strings | No | Paths the agent can read and write. Typically `/sandbox` (working directory) and `/tmp`. | +| `read_write` | list of strings | No | Paths the agent can read and write. Typically `/tmp`; set `include_workdir: true` to add the driver-resolved working directory. | **Validation constraints:** @@ -76,7 +76,6 @@ filesystem_policy: - /dev/urandom - /etc read_write: - - /sandbox - /tmp - /dev/null ``` diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 6700a22c9f..32b552e108 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -168,9 +168,10 @@ Docker mount schema: OpenShell rejects mount `source`, `target`, and Docker volume `subpath` 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. +the workspace root or 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. ## Podman Driver @@ -433,6 +434,22 @@ 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 +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 +already exist in the immutable image without symlink components. The completed +UID/GID and supplementary groups must already be able to traverse every parent +and write and enter the workdir. OpenShell does not change that directory's +ownership or mode. It rejects kernel-managed filesystems based on filesystem +metadata 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. + Sandbox creation fails before readiness if a required `USER` component is missing, malformed, unknown, ambiguous, or resolves to UID/GID 0. An image without `USER` therefore works only when policy explicitly provides both @@ -462,7 +479,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. OpenShell continues to use `/sandbox` as the workspace; it does not -adopt the image's OCI working directory. Until OCI working-directory support is -added, custom images must create `/sandbox` and make it writable by the selected -identity. +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`. diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index e520c55f49..bc408c4ecd 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/docs/sandboxes/manage-sandboxes.mdx @@ -392,22 +392,31 @@ Append the output to `~/.ssh/config` or use `--editor` on `sandbox create`/`sand Upload files from your host into the sandbox: ```shell -openshell sandbox upload my-sandbox ./src /sandbox/src +openshell sandbox upload my-sandbox ./src ``` -When the local path is a named directory, OpenShell preserves that basename at the destination, matching `scp -r` and `cp -r`. The example above creates `/sandbox/src/src`. +When you omit the destination, OpenShell discovers the sandbox's working +directory and uploads there. For a named local directory, OpenShell preserves +the basename, matching `scp -r` and `cp -r`. If that directory already exists, +the upload merges into it and overwrites matching entries without deleting +unrelated entries. OpenShell preserves symlinks during upload. A symlink arrives in the sandbox as a symlink with the same target path instead of an expanded copy of the target file or directory. Dangling symlinks are also preserved. Download files from the sandbox to your host: ```shell -openshell sandbox download my-sandbox /sandbox/output ./local +openshell sandbox download my-sandbox output ./local ``` When the sandbox-side source is a single file, the destination follows `cp`-style placement: if the destination already exists as a directory or ends with `/`, the file lands inside it as `/`; otherwise the file is written at the exact destination path. -The CLI only allows sandbox-side sources that resolve inside the writable workspace (`/sandbox`). Paths that escape lexically (`/etc/passwd`, `/sandbox/../etc/passwd`) and paths that escape through a symlink (`/sandbox/etc-link` pointing at `/etc`) are both refused before any data is transferred. +The CLI discovers the sandbox's canonical working directory and only allows +sandbox-side sources that resolve inside it. Paths that escape lexically, such +as `/etc/passwd` or `/sandbox/../etc/passwd`, and paths that escape through a +symlink are refused before any data is transferred. Relative sources are +resolved from the working directory; absolute sources within the same canonical +directory are also accepted. You can also upload files at creation time with the `--upload` flag on diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index 4c2e12dacf..7a6dbca2d4 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -19,8 +19,9 @@ version: 1 # Static: locked at sandbox creation. Paths the agent can read vs read/write. filesystem_policy: + include_workdir: true read_only: [/usr, /lib, /etc] - read_write: [/sandbox, /tmp] + read_write: [/tmp] # Static: Landlock LSM kernel enforcement. best_effort uses highest ABI the host supports. landlock: @@ -100,7 +101,7 @@ See [Supervisor Middleware](/extensibility/supervisor-middleware) for registrati ## Baseline Filesystem Paths -When a sandbox runs in proxy mode (the default), OpenShell automatically adds baseline filesystem paths required for the sandbox child process to function: `/usr`, `/lib`, `/etc`, `/var/log` (read-only) and `/sandbox`, `/tmp` (read-write). Paths like `/app` are included in the baseline set but are only added if they exist in the container image. +When a sandbox runs in proxy mode (the default), OpenShell automatically adds baseline filesystem paths required for the sandbox child process to function: `/usr`, `/lib`, `/etc`, and `/var/log` (read-only), plus `/tmp` (read-write). When `filesystem.include_workdir` is `true`, OpenShell also adds the resolved working directory as read-write. Paths like `/app` are included in the baseline set but are only added if they exist in the container image. For GPU sandboxes, OpenShell also adds existing GPU device nodes as read-write paths. CUDA workloads require write access to procfs for thread metadata, so GPU baseline enrichment moves `/proc` from read-only to read-write when GPU devices are present. diff --git a/docs/security/best-practices.mdx b/docs/security/best-practices.mdx index 0ac0d5528f..8bbcc604d4 100644 --- a/docs/security/best-practices.mdx +++ b/docs/security/best-practices.mdx @@ -174,7 +174,7 @@ The policy separates filesystem paths into read-only and read-write groups. | Aspect | Detail | |---|---| -| Default | System paths (`/usr`, `/lib`, `/etc`, `/var/log`) are read-only. Working paths (`/sandbox`, `/tmp`) are read-write. `/app` is conditionally included if it exists. | +| Default | System paths (`/usr`, `/lib`, `/etc`, `/var/log`) are read-only. The resolved working directory and `/tmp` are read-write. `/app` is conditionally included if it exists. | | What you can change | Add or remove paths in `filesystem_policy.read_only` and `filesystem_policy.read_write`. | | Risk if relaxed | Making system paths writable lets the agent replace binaries, modify TLS trust stores, or change DNS resolution. Validation rejects broad read-write paths (like `/`). | | Recommendation | Keep system paths read-only. If the agent needs additional writable space, add a specific subdirectory. | diff --git a/e2e/rust/src/harness/sandbox.rs b/e2e/rust/src/harness/sandbox.rs index 0aeb25038c..3475353041 100644 --- a/e2e/rust/src/harness/sandbox.rs +++ b/e2e/rust/src/harness/sandbox.rs @@ -341,6 +341,39 @@ impl SandboxGuard { Ok(combined) } + /// Upload local files to the sandbox's discovered working directory. + /// + /// # Errors + /// + /// Returns an error if the upload command fails. + pub async fn upload_to_workdir(&self, local_path: &str) -> Result { + let mut cmd = openshell_cmd(); + cmd.arg("sandbox") + .arg("upload") + .arg(&self.name) + .arg(local_path) + .arg("--no-git-ignore"); + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + + let output = cmd + .output() + .await + .map_err(|e| format!("failed to spawn openshell upload: {e}"))?; + + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let combined = format!("{stdout}{stderr}"); + + if !output.status.success() { + return Err(format!( + "sandbox upload failed (exit {:?}):\n{combined}", + output.status.code() + )); + } + + Ok(combined) + } + /// Upload local files with `.gitignore` filtering (default behavior). /// /// Unlike [`upload`], this does NOT pass `--no-git-ignore`, so the CLI diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index 4cda100dfe..e43cdb0514 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -10,7 +10,7 @@ //! - The matching container runtime running (for image builds) //! - The `openshell` binary (built automatically from the workspace) -use std::io::Write; +use std::{fs, io::Write}; use openshell_e2e::harness::output::strip_ansi; use openshell_e2e::harness::sandbox::SandboxGuard; @@ -24,6 +24,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ RUN groupadd -g 1235 appstaff && \ useradd -m -u 1234 -g appstaff app +# The final image identity already owns the OCI working directory. Existing +# root-owned content remains root-owned. +WORKDIR /workspace/project +RUN printf root-owned > root-owned.txt && chown app:appstaff . + # Write a marker file so we can verify this is our custom image. # Place under /etc (Landlock baseline read-only path) so the sandbox # can read it when filesystem restrictions are properly enforced. @@ -42,9 +47,22 @@ USER 2345:2346 CMD ["sleep", "infinity"] "#; +const UNWRITABLE_WORKDIR_DOCKERFILE_CONTENT: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim + +RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ + && rm -rf /var/lib/apt/lists/* \ + && groupadd -g 3235 appstaff \ + && useradd -m -u 3234 -g appstaff app + +WORKDIR /workspace/project +USER app +CMD ["sleep", "infinity"] +"#; + const MARKER: &str = "custom-image-e2e-marker"; -/// Direct and SSH children use the same named OCI identity. +/// A named OCI user can write through direct and SSH children when the image +/// already grants that authority; existing content retains its ownership. #[tokio::test] async fn sandbox_from_custom_dockerfile() { // Step 1: Write a temporary Dockerfile. @@ -63,7 +81,11 @@ async fn sandbox_from_custom_dockerfile() { &[ "sh", "-c", - "set -eu; id -u; id -g; cat /etc/marker.txt; echo Ready; sleep infinity", + "set -eu; id -u; id -g; test \"$(pwd -P)\" = /workspace/project; \ + test \"$HOME\" = /workspace/project; test \"$(cat root-owned.txt)\" = root-owned; \ + test \"$(stat -c %u:%g .)\" = 1234:1235; \ + test \"$(stat -c %u:%g root-owned.txt)\" = 0:0; \ + touch direct-oci-user-write; cat /etc/marker.txt; echo Ready; sleep infinity", ], "Ready", ) @@ -85,20 +107,80 @@ async fn sandbox_from_custom_dockerfile() { .exec(&[ "sh", "-c", - "set -eu; test \"$(id -u):$(id -g)\" = 1234:1235; echo ssh-identity-ok", + "set -eu; test \"$(id -u):$(id -g)\" = 1234:1235; \ + test \"$(pwd -P)\" = /workspace/project; test \"$HOME\" = /workspace/project; \ + touch ssh-oci-user-write; echo ssh-write-ok", ]) .await - .expect("SSH child should use OCI identity"); + .expect("SSH child should write to prepared workspace"); assert!( - ssh_output.contains("ssh-identity-ok"), - "expected SSH identity marker:\n{ssh_output}" + ssh_output.contains("ssh-write-ok"), + "expected SSH write marker:\n{ssh_output}" + ); + + let transfer_source = tmpdir.path().join("workspace-transfer.txt"); + fs::write(&transfer_source, "workspace-transfer-ok").expect("write transfer fixture"); + guard + .upload_to_workdir( + transfer_source + .to_str() + .expect("transfer fixture path is UTF-8"), + ) + .await + .expect("upload should default to the OCI workspace"); + let transfer_download = tmpdir.path().join("workspace-transfer-downloaded.txt"); + guard + .download( + "workspace-transfer.txt", + transfer_download + .to_str() + .expect("download destination path is UTF-8"), + ) + .await + .expect("download should resolve relative to the OCI workspace"); + assert_eq!( + fs::read_to_string(transfer_download).expect("read downloaded transfer fixture"), + "workspace-transfer-ok" ); + guard + .exec(&[ + "sh", + "-c", + "set -eu; mkdir -p merge-upload; \ + printf remote-conflict > merge-upload/conflict.txt; \ + printf remote-preserved > merge-upload/unrelated.txt", + ]) + .await + .expect("seed existing remote upload directory"); + let merge_source = tmpdir.path().join("merge-upload"); + fs::create_dir(&merge_source).expect("create local upload directory"); + fs::write(merge_source.join("conflict.txt"), "local-conflict") + .expect("write conflicting local upload file"); + fs::write(merge_source.join("added.txt"), "local-added") + .expect("write added local upload file"); + guard + .upload_to_workdir(merge_source.to_str().expect("merge upload path is UTF-8")) + .await + .expect("upload should merge into the existing remote directory"); + guard + .exec(&[ + "sh", + "-c", + "set -eu; \ + test \"$(cat merge-upload/conflict.txt)\" = local-conflict; \ + test \"$(cat merge-upload/added.txt)\" = local-added; \ + test \"$(cat merge-upload/unrelated.txt)\" = remote-preserved", + ]) + .await + .expect("upload should overwrite conflicts and preserve unrelated remote files"); + // Explicit cleanup (also happens in Drop, but explicit is clearer in tests). guard.cleanup().await; } /// A numeric OCI user/group pair works without passwd or group entries. +/// The image intentionally has no pre-existing `/sandbox`. #[tokio::test] async fn sandbox_from_passwd_less_numeric_oci_user() { let tmpdir = tempfile::tempdir().expect("create tmpdir"); @@ -110,10 +192,17 @@ async fn sandbox_from_passwd_less_numeric_oci_user() { } let dockerfile_str = dockerfile_path.to_str().expect("Dockerfile path is UTF-8"); - let mut guard = - SandboxGuard::create(&["--from", dockerfile_str, "--", "sh", "-c", "id -u; id -g"]) - .await - .expect("sandbox create from numeric OCI Dockerfile"); + let mut guard = SandboxGuard::create(&[ + "--from", + dockerfile_str, + "--", + "sh", + "-c", + "set -eu; id -u; id -g; test \"$(pwd -P)\" = /sandbox; \ + test \"$HOME\" = /sandbox; touch numeric-oci-user-write", + ]) + .await + .expect("sandbox create from numeric OCI Dockerfile"); let clean_output = strip_ansi(&guard.create_output); assert!( @@ -123,3 +212,32 @@ async fn sandbox_from_passwd_less_numeric_oci_user() { guard.cleanup().await; } + +#[tokio::test] +async fn sandbox_rejects_image_workdir_that_would_require_new_authority() { + let tmpdir = tempfile::tempdir().expect("create tmpdir"); + let dockerfile_path = tmpdir.path().join("Dockerfile"); + fs::write(&dockerfile_path, UNWRITABLE_WORKDIR_DOCKERFILE_CONTENT).expect("write Dockerfile"); + let dockerfile_str = dockerfile_path.to_str().expect("Dockerfile path is UTF-8"); + + let result = SandboxGuard::create_keep_with_args( + &["--from", dockerfile_str, "--no-tty"], + &["sh", "-c", "echo should-not-run"], + "should-not-run", + ) + .await; + let error = match result { + Ok(mut guard) => { + guard.cleanup().await; + panic!("root-owned workdir must not be made writable for the image user"); + } + Err(error) => error, + }; + let message = error.to_string(); + assert!( + message.contains("WorkingDir") + || message.contains("workspace") + || message.contains("readiness"), + "expected workspace authority failure, got: {message}" + ); +} diff --git a/e2e/rust/tests/driver_config_volume.rs b/e2e/rust/tests/driver_config_volume.rs index ad8cffc2f9..0702a4637d 100644 --- a/e2e/rust/tests/driver_config_volume.rs +++ b/e2e/rust/tests/driver_config_volume.rs @@ -7,6 +7,8 @@ use std::fs; use std::io::Write; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; +use std::process::Stdio; +use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{SystemTime, UNIX_EPOCH}; use bollard::Docker; @@ -16,19 +18,84 @@ use bollard::query_parameters::{ RemoveVolumeOptionsBuilder, StartContainerOptions, WaitContainerOptions, }; use futures_util::TryStreamExt; -use openshell_e2e::harness::container::e2e_driver; +use openshell_e2e::harness::container::{ContainerEngine, e2e_driver}; use openshell_e2e::harness::sandbox::SandboxGuard; use serde_json::{Map, Value}; const TEST_IMAGE: &str = "ghcr.io/nvidia/openshell-community/sandboxes/base:latest"; const VOLUME_TARGET: &str = "/sandbox/e2e-volume"; const BIND_TARGET: &str = "/sandbox/e2e-bind"; +#[cfg(feature = "e2e-docker")] +const OCI_VOLUME_TARGET: &str = "/workspace/project/e2e-volume"; +#[cfg(feature = "e2e-docker")] +const OCI_USER_DOCKERFILE: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim + +RUN apt-get update && apt-get install -y --no-install-recommends iproute2 \ + && rm -rf /var/lib/apt/lists/* + +WORKDIR /workspace/project +RUN chown 2234:2235 . +USER 2234:2235 +CMD ["sleep", "infinity"] +"#; + +static NEXT_VOLUME_ID: AtomicU64 = AtomicU64::new(0); struct VolumeGuard { docker: Docker, name: String, } +struct ImageGuard { + engine: ContainerEngine, + tag: String, +} + +impl ImageGuard { + fn build(driver: &str, dockerfile: &Path, context: &Path) -> Result { + let engine = ContainerEngine::from_env()?; + let tag = format!("localhost/{}-oci-user:latest", unique_volume_name(driver)); + let output = engine + .command() + .args([ + "build", + "--file", + dockerfile + .to_str() + .ok_or_else(|| "Dockerfile path must be UTF-8".to_string())?, + "--tag", + &tag, + context + .to_str() + .ok_or_else(|| "image context path must be UTF-8".to_string())?, + ]) + .output() + .map_err(|err| format!("run {} build: {err}", engine.name()))?; + if !output.status.success() { + return Err(format!( + "{} build failed (exit {:?}):\n{}{}", + engine.name(), + output.status.code(), + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )); + } + Ok(Self { engine, tag }) + } +} + +impl Drop for ImageGuard { + fn drop(&mut self) { + let _ = self + .engine + .command() + .args(["image", "rm", "--force", &self.tag]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + } +} + impl VolumeGuard { async fn create(driver: &str) -> Result { let name = unique_volume_name(driver); @@ -101,6 +168,73 @@ async fn sandbox_mounts_existing_driver_config_volume() { .expect("verify sandbox wrote to named test volume"); } +#[tokio::test] +#[cfg(feature = "e2e-docker")] +async fn oci_workspace_preparation_skips_nested_volume_ownership() { + let driver = e2e_driver().expect("OPENSHELL_E2E_DRIVER must be set by the e2e wrapper"); + assert!( + driver == "docker", + "OCI workspace mount e2e requires docker, got {driver}" + ); + + let volume = VolumeGuard::create(&driver) + .await + .expect("create named test volume"); + seed_volume(&volume).await.expect("seed named test volume"); + + let image_context = tempfile::tempdir().expect("create OCI image context"); + let dockerfile = image_context.path().join("Dockerfile"); + fs::write(&dockerfile, OCI_USER_DOCKERFILE).expect("write OCI image Dockerfile"); + let image = ImageGuard::build(&driver, &dockerfile, image_context.path()) + .expect("build OCI-user image with selected container engine"); + + let driver_config = format!( + r#"{{"{driver}":{{"mounts":[{{"type":"volume","source":"{}","target":"{OCI_VOLUME_TARGET}","read_only":false}}]}}}}"#, + volume.name + ); + let mut sandbox = SandboxGuard::create_keep_with_args( + &[ + "--from", + &image.tag, + "--driver-config-json", + &driver_config, + "--no-tty", + ], + &[ + "sh", + "-lc", + "set -eu; test \"$(id -u):$(id -g)\" = 2234:2235; \ + test \"$(pwd -P)\" = /workspace/project; test \"$HOME\" = /workspace/project; \ + test \"$(stat -c %u:%g /workspace/project/e2e-volume/input.txt)\" = 0:0; \ + touch direct-write; echo Ready; sleep infinity", + ], + "Ready", + ) + .await + .expect("create OCI-user sandbox with nested volume"); + + let ssh_output = sandbox + .exec(&[ + "sh", + "-lc", + "set -eu; test \"$(pwd -P)\" = /workspace/project; \ + test \"$HOME\" = /workspace/project; \ + test \"$(stat -c %u:%g /workspace/project/e2e-volume/input.txt)\" = 0:0; \ + touch ssh-write; echo nested-mount-owner-ok", + ]) + .await + .expect("SSH child should preserve nested volume ownership"); + assert!( + ssh_output.contains("nested-mount-owner-ok"), + "expected nested mount ownership marker:\n{ssh_output}" + ); + + sandbox.cleanup().await; + verify_volume_ownership(&volume) + .await + .expect("nested volume ownership should remain unchanged"); +} + #[tokio::test] async fn sandbox_mounts_enabled_driver_config_bind() { let driver = e2e_driver().expect("OPENSHELL_E2E_DRIVER must be set by the e2e wrapper"); @@ -208,6 +342,23 @@ async fn verify_volume(volume: &VolumeGuard) -> Result<(), String> { Ok(()) } +#[cfg(feature = "e2e-docker")] +async fn verify_volume_ownership(volume: &VolumeGuard) -> Result<(), String> { + let output = run_volume_container( + volume, + "verify-owner", + true, + "set -eu; test \"$(stat -c %u:%g /vol/input.txt)\" = 0:0; echo owner-ok", + ) + .await?; + if !output.contains("owner-ok") { + return Err(format!( + "volume ownership verification did not print expected marker:\n{output}" + )); + } + Ok(()) +} + async fn run_volume_container( volume: &VolumeGuard, purpose: &str, @@ -413,8 +564,9 @@ fn unique_volume_name(driver: &str) -> String { .duration_since(UNIX_EPOCH) .expect("system clock should be after Unix epoch") .as_nanos(); + let sequence = NEXT_VOLUME_ID.fetch_add(1, Ordering::Relaxed); format!( - "openshell-e2e-driver-config-volume-{driver}-{}-{nanos}", + "openshell-e2e-driver-config-volume-{driver}-{}-{nanos}-{sequence}", std::process::id() ) } From fe98793ff63e1d30ba829329e211982db946832c Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 30 Jul 2026 16:21:12 -0700 Subject: [PATCH 2/8] fix(sandbox): honor effective workspace access Signed-off-by: Matthew Grossman --- .../skills/debug-openshell-cluster/SKILL.md | 2 +- architecture/compute-runtimes.md | 4 +- crates/openshell-driver-docker/README.md | 4 +- crates/openshell-sandbox/src/main.rs | 73 ++++++ .../src/process.rs | 241 ++++++++++++++++++ docs/reference/sandbox-compute-drivers.mdx | 4 +- e2e/rust/tests/custom_image.rs | 4 + 7 files changed, 328 insertions(+), 4 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index 9ef35c9425..1fd95a217a 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -149,7 +149,7 @@ Common findings: - Gateway process stopped: inspect exit status and logs. - Sandbox image missing or pull denied: verify image reference and registry credentials. - Sandbox fails before readiness with an identity-resolution error: inspect the image's OCI `USER` and matching `/etc/passwd` and `/etc/group` entries, or explicitly set both process identity fields in policy. Root and missing identities are rejected. -- 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. OpenShell does not create, chown, or chmod a non-default image workdir. +- 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, and supplementary groups must pass the kernel's effective traverse/write checks, including POSIX ACL and LSM decisions. OpenShell does not create, chown, or chmod a non-default image workdir. - Docker also rejects an image `VOLUME` that covers the workdir or one of its parents because the runtime would mask the immutable path before validation. Move the `VOLUME` below the workspace or remove the declaration. - A workdir rejected as a special filesystem or OpenShell control-path collision cannot be made valid with permissions. Move the image workdir away from kernel-backed mounts and the concrete supervisor, TLS, token, runtime, and socket paths named in the error. - Docker driver cannot initialize because it cannot find `openshell-sandbox`: verify `OPENSHELL_DOCKER_SUPERVISOR_BIN`, the sibling binary next to `openshell-gateway`, or the configured supervisor image contains `/openshell-sandbox`. diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 63083d8319..2b13af0f32 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -205,7 +205,9 @@ OpenShell creates and owns as a compatibility workspace. Any other workdir must exist in the immutable image without symlink components. The completed identity, including supplementary groups, must already be able to traverse every parent and write and enter the workdir; OpenShell does not change that -directory's ownership or mode. Filesystem metadata identifies kernel-managed +directory's ownership or mode. A one-shot validator drops to that identity and +uses kernel effective-access checks so POSIX ACL and LSM decisions are honored. +Filesystem metadata identifies kernel-managed mounts, while 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 diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index 0123a6ee85..40898965f8 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -33,7 +33,9 @@ root (`/`), or explicit `/sandbox` declaration uses `/sandbox`, which OpenShell creates when necessary and owns as a compatibility workspace. Any other image workdir must already exist without symlink components. The completed identity, including supplementary groups, must already be able to traverse every parent and -write and enter the workdir. OpenShell does not change its ownership or mode. +write and enter the workdir. Validation runs under that completed identity and +uses the kernel's effective-access checks, so POSIX ACL grants and LSM denials +are honored. OpenShell does not change its ownership or mode. Image `VOLUME` declarations must not cover the workdir or one of its parents because Docker would mount the volume before the supervisor could validate the immutable image path. diff --git a/crates/openshell-sandbox/src/main.rs b/crates/openshell-sandbox/src/main.rs index 62ae37b5a1..7230a894e0 100644 --- a/crates/openshell-sandbox/src/main.rs +++ b/crates/openshell-sandbox/src/main.rs @@ -32,6 +32,7 @@ const COPY_SELF_SUBCOMMAND: &str = "copy-self"; /// run `openshell-sandbox debug-rpc get-sandbox-config --sandbox-id ` /// to confirm the cross-sandbox IDOR guard fires. const DEBUG_RPC_SUBCOMMAND: &str = "debug-rpc"; +const VALIDATE_WORKSPACE_SUBCOMMAND: &str = "validate-workspace"; /// Default `--mode` value: run both supervisor leaves in a single binary. const DEFAULT_MODE: &str = "network,process"; @@ -231,6 +232,50 @@ struct Args { upstream_proxy_connect_by_hostname: bool, } +/// Internal one-shot command used by the privileged supervisor to validate an +/// image-provided workdir as the final sandbox identity. +#[derive(Parser, Debug)] +#[command(name = "validate-workspace", hide = true)] +struct ValidateWorkspaceArgs { + #[arg(long)] + workdir: String, + #[arg(long)] + expected_uid: u32, + #[arg(long)] + expected_gid: u32, +} + +#[cfg(target_os = "linux")] +fn validate_workspace(args: &[String]) -> Result<()> { + let args = ValidateWorkspaceArgs::try_parse_from( + std::iter::once(VALIDATE_WORKSPACE_SUBCOMMAND.to_string()).chain(args.iter().cloned()), + ) + .into_diagnostic()?; + let actual = ( + nix::unistd::geteuid().as_raw(), + nix::unistd::getegid().as_raw(), + ); + if actual != (args.expected_uid, args.expected_gid) { + return Err(miette::miette!( + "workspace validator privilege drop failed: expected {}:{}, got {}:{}", + args.expected_uid, + args.expected_gid, + actual.0, + actual.1 + )); + } + openshell_supervisor_process::process::validate_oci_workspace_as_effective_identity(Path::new( + &args.workdir, + )) +} + +#[cfg(not(target_os = "linux"))] +fn validate_workspace(_args: &[String]) -> Result<()> { + Err(miette::miette!( + "workspace validation is only supported on Unix" + )) +} + /// Copy the running executable to `dest`, creating parent directories as /// needed and ensuring the result is executable (mode `0755`). /// @@ -479,6 +524,9 @@ fn main() -> Result<()> { std::process::exit(exit); }); } + if raw_args.get(1).map(String::as_str) == Some(VALIDATE_WORKSPACE_SUBCOMMAND) { + return validate_workspace(&raw_args[2..]); + } let args = Args::parse(); @@ -648,6 +696,31 @@ mod tests { use super::*; use std::os::unix::fs::PermissionsExt; + #[cfg(target_os = "linux")] + #[test] + fn workspace_validation_subcommand_uses_final_policy_identity() { + let uid = nix::unistd::geteuid().as_raw(); + let gid = nix::unistd::getegid().as_raw(); + if uid < 1000 || gid < 1000 { + 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(0o700)).unwrap(); + let args = vec![ + "--workdir".to_string(), + root.display().to_string(), + "--expected-uid".to_string(), + uid.to_string(), + "--expected-gid".to_string(), + gid.to_string(), + ]; + + validate_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-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index c14df9e2df..57cdcde098 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1329,6 +1329,71 @@ pub fn validate_oci_workspace( Ok(()) } +/// Validate an image-provided workdir in a clean copy of the supervisor so the +/// main process retains the root authority needed for subsequent setup. +#[cfg(target_os = "linux")] +fn validate_oci_workspace_in_subprocess( + policy: &SandboxPolicy, + resolved_identity: ResolvedProcessIdentity, + workdir: &Path, +) -> Result<()> { + use std::os::unix::process::CommandExt; + + let (uid, gid, supplementary_gids) = resolve_filesystem_identity(policy, resolved_identity)?; + let uid = uid.ok_or_else(|| miette::miette!("workspace validator UID is unresolved"))?; + let gid = gid.ok_or_else(|| miette::miette!("workspace validator GID is unresolved"))?; + let groups = supplementary_gids + .iter() + .map(|group| group.as_raw()) + .collect::>(); + let executable = std::env::current_exe().into_diagnostic()?; + let mut command = std::process::Command::new(executable); + command + .arg("validate-workspace") + .arg("--workdir") + .arg(workdir) + .arg("--expected-uid") + .arg(uid.to_string()) + .arg("--expected-gid") + .arg(gid.to_string()) + .env_clear() + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::piped()); + + // `pre_exec` runs after fork and before exec. These direct credential + // syscalls are async-signal-safe and affect only the one-shot child. + #[allow(unsafe_code)] + unsafe { + command.pre_exec(move || { + if libc::setgroups(groups.len(), groups.as_ptr()) != 0 + || libc::setgid(gid.as_raw()) != 0 + || libc::setuid(uid.as_raw()) != 0 + { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + }); + } + + let output = command.output().into_diagnostic()?; + if output.status.success() { + return Ok(()); + } + + let diagnostic = String::from_utf8_lossy(&output.stderr); + let diagnostic = diagnostic.trim(); + if diagnostic.is_empty() { + return Err(miette::miette!( + "image workspace validation failed with status {}", + output.status + )); + } + Err(miette::miette!( + "image workspace validation failed: {diagnostic}" + )) +} + #[cfg(unix)] fn validate_workspace_component( path: &Path, @@ -1379,6 +1444,104 @@ fn validate_workspace_component( Ok(()) } +#[cfg(target_os = "linux")] +pub fn validate_oci_workspace_as_effective_identity(root: &Path) -> Result<()> { + use rustix::fs::{Access, AtFlags, FileType, Mode, OFlags}; + + let components = validated_workspace_components(root, false)?; + 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, + ".", + Access::EXEC_OK, + AtFlags::EACCESS | AtFlags::SYMLINK_NOFOLLOW, + ) + .map_err(|error| { + miette::miette!( + "workspace path component '{}' is not traversable by the sandbox identity in the image: {error}", + current_path.display() + ) + })?; + + 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( + |error| { + if error == rustix::io::Errno::NOENT { + miette::miette!( + "image workspace path component '{}' does not exist", + current_path.display() + ) + } else { + miette::miette!( + "failed to inspect image workspace path component '{}': {error}", + current_path.display() + ) + } + }, + )?; + let file_type = FileType::from_raw_mode(stat.st_mode); + if file_type.is_symlink() { + return Err(miette::miette!( + "workspace path component '{}' is a symlink — refusing to follow it", + current_path.display() + )); + } + if !file_type.is_dir() { + return Err(miette::miette!( + "workspace path component '{}' is not a directory", + current_path.display() + )); + } + + let is_workspace = index == last_component; + let access = if is_workspace { + Access::WRITE_OK | Access::EXEC_OK + } else { + Access::EXEC_OK + }; + rustix::fs::accessat( + ¤t_fd, + &component, + access, + AtFlags::EACCESS | AtFlags::SYMLINK_NOFOLLOW, + ) + .map_err(|error| { + let requirement = if is_workspace { + "writable and traversable" + } else { + "traversable" + }; + miette::miette!( + "workspace path component '{}' is not {requirement} by the sandbox identity in the image: {error}", + current_path.display() + ) + })?; + + 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)?; + } + + 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. @@ -1693,6 +1856,9 @@ pub fn prepare_filesystem_with_identity( prepare_oci_workspace(workspace, uid, gid, &supplementary_gids)?; } else { info!(path = %workspace.display(), ?uid, ?gid, "Validating image workspace authority"); + #[cfg(target_os = "linux")] + validate_oci_workspace_in_subprocess(policy, resolved_identity, workspace)?; + #[cfg(not(target_os = "linux"))] validate_oci_workspace(workspace, uid, gid, &supplementary_gids)?; } } @@ -2888,6 +3054,81 @@ mod tests { assert!(error.to_string().contains("does not exist")); } + #[cfg(target_os = "linux")] + #[test] + #[allow(unsafe_code)] + fn effective_identity_validation_honors_named_user_acl() { + if !nix::unistd::geteuid().is_root() { + return; + } + + const TEST_UID: u32 = 42_234; + const TEST_GID: u32 = 42_235; + const ACL_XATTR_VERSION: u32 = 2; + const ACL_USER_OBJ: u16 = 0x01; + const ACL_USER: u16 = 0x02; + const ACL_GROUP_OBJ: u16 = 0x04; + const ACL_MASK: u16 = 0x10; + const ACL_OTHER: u16 = 0x20; + const ACL_UNDEFINED_ID: u32 = u32::MAX; + + 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("project"); + std::fs::create_dir(&root).unwrap(); + std::fs::set_permissions(&root, std::fs::Permissions::from_mode(0o700)).unwrap(); + + let mut acl = ACL_XATTR_VERSION.to_ne_bytes().to_vec(); + for (tag, permissions, id) in [ + (ACL_USER_OBJ, 0o7, ACL_UNDEFINED_ID), + (ACL_USER, 0o7, TEST_UID), + (ACL_GROUP_OBJ, 0o0, ACL_UNDEFINED_ID), + (ACL_MASK, 0o7, ACL_UNDEFINED_ID), + (ACL_OTHER, 0o0, ACL_UNDEFINED_ID), + ] { + acl.extend_from_slice(&tag.to_ne_bytes()); + acl.extend_from_slice(&permissions.to_ne_bytes()); + acl.extend_from_slice(&id.to_ne_bytes()); + } + let path = std::ffi::CString::new(root.as_os_str().as_encoded_bytes()).unwrap(); + let name = c"system.posix_acl_access"; + let result = unsafe { + libc::setxattr( + path.as_ptr(), + name.as_ptr(), + acl.as_ptr().cast(), + acl.len(), + 0, + ) + }; + assert_eq!( + result, + 0, + "setxattr failed: {}", + std::io::Error::last_os_error() + ); + + match unsafe { fork() }.expect("fork should succeed") { + ForkResult::Child => { + let credentials_dropped = unsafe { + libc::setgroups(0, std::ptr::null()) == 0 + && libc::setgid(TEST_GID) == 0 + && libc::setuid(TEST_UID) == 0 + }; + let valid = credentials_dropped + && validate_oci_workspace_as_effective_identity(&root).is_ok(); + unsafe { libc::_exit(i32::from(!valid)) }; + } + ForkResult::Parent { child } => { + assert_eq!( + waitpid(child, None).expect("waitpid should succeed"), + WaitStatus::Exited(child, 0), + "named ACL user should retain workspace authority" + ); + } + } + } + #[cfg(unix)] #[test] fn validate_oci_workspace_rejects_restrictive_parent() { diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 32b552e108..33447e2922 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -441,7 +441,9 @@ OpenShell creates and owns that compatibility workspace. Any other workdir must already exist in the immutable image without symlink components. The completed UID/GID and supplementary groups must already be able to traverse every parent and write and enter the workdir. OpenShell does not change that directory's -ownership or mode. It rejects kernel-managed filesystems based on filesystem +ownership or mode. A one-shot validator drops to that identity and uses kernel +effective-access checks, including POSIX ACL grants and LSM denials. It rejects +kernel-managed filesystems based on filesystem metadata 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 diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index e43cdb0514..5652a0011e 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -14,6 +14,7 @@ use std::{fs, io::Write}; use openshell_e2e::harness::output::strip_ansi; use openshell_e2e::harness::sandbox::SandboxGuard; +use serial_test::serial; const DOCKERFILE_CONTENT: &str = r#"FROM public.ecr.aws/docker/library/python:3.13-slim @@ -64,6 +65,7 @@ const MARKER: &str = "custom-image-e2e-marker"; /// A named OCI user can write through direct and SSH children when the image /// already grants that authority; existing content retains its ownership. #[tokio::test] +#[serial(custom_image)] async fn sandbox_from_custom_dockerfile() { // Step 1: Write a temporary Dockerfile. let tmpdir = tempfile::tempdir().expect("create tmpdir"); @@ -182,6 +184,7 @@ async fn sandbox_from_custom_dockerfile() { /// A numeric OCI user/group pair works without passwd or group entries. /// The image intentionally has no pre-existing `/sandbox`. #[tokio::test] +#[serial(custom_image)] async fn sandbox_from_passwd_less_numeric_oci_user() { let tmpdir = tempfile::tempdir().expect("create tmpdir"); let dockerfile_path = tmpdir.path().join("Dockerfile"); @@ -214,6 +217,7 @@ async fn sandbox_from_passwd_less_numeric_oci_user() { } #[tokio::test] +#[serial(custom_image)] async fn sandbox_rejects_image_workdir_that_would_require_new_authority() { let tmpdir = tempfile::tempdir().expect("create tmpdir"); let dockerfile_path = tmpdir.path().join("Dockerfile"); From 7240e3fc1fbb2cfcda1de6ea9802307d378ad4ba Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 30 Jul 2026 16:25:15 -0700 Subject: [PATCH 3/8] test(sandbox): cover enforced workspace denial Signed-off-by: Matthew Grossman --- .../src/process.rs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 57cdcde098..69cc474ce0 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -3129,6 +3129,44 @@ mod tests { } } + #[cfg(target_os = "linux")] + #[test] + #[allow(unsafe_code)] + fn effective_identity_validation_honors_landlock_denial() { + 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(); + + let mut policy = policy_with_process(ProcessPolicy::default()); + policy.filesystem = FilesystemPolicy { + read_only: vec![root.clone()], + read_write: Vec::new(), + include_workdir: false, + }; + policy.landlock = LandlockPolicy { + compatibility: openshell_core::policy::LandlockCompatibility::HardRequirement, + }; + let Ok(prepared) = crate::sandbox::prepare_current_user(&policy, None) else { + return; + }; + + match unsafe { fork() }.expect("fork should succeed") { + ForkResult::Child => { + let denied = crate::sandbox::enforce(prepared).is_ok() + && validate_oci_workspace_as_effective_identity(&root).is_err(); + unsafe { libc::_exit(i32::from(!denied)) }; + } + ForkResult::Parent { child } => { + assert_eq!( + waitpid(child, None).expect("waitpid should succeed"), + WaitStatus::Exited(child, 0), + "kernel-effective validation should honor an enforced LSM denial" + ); + } + } + } + #[cfg(unix)] #[test] fn validate_oci_workspace_rejects_restrictive_parent() { From 7306dd7b24b862a4b5c1330e35158f1d9f85ab69 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 30 Jul 2026 16:30:23 -0700 Subject: [PATCH 4/8] docs(docker): explain effective workdir checks Signed-off-by: Matthew Grossman --- crates/openshell-driver-docker/README.md | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index 40898965f8..135a4ef733 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -30,12 +30,22 @@ 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. Any other image workdir must -already exist without symlink components. The completed identity, including -supplementary groups, must already be able to traverse every parent and -write and enter the workdir. Validation runs under that completed identity and -uses the kernel's effective-access checks, so POSIX ACL grants and LSM denials -are honored. OpenShell does not change its ownership or mode. +creates when necessary and owns as a compatibility workspace. Any other image +workdir must already exist without symlink components. The completed identity, +including supplementary groups, must already be able to traverse every parent +and write and enter the workdir. OpenShell does not change its ownership or +mode. + +OpenShell deliberately asks the Linux kernel to make this access decision +under the completed sandbox identity instead of reproducing permission rules +from ownership and mode bits. Mode-bit inspection alone can reject authority +granted by a POSIX ACL or overlook a denial imposed by a Linux Security Module +such as SELinux or AppArmor. OpenShell does not configure or otherwise manage +ACLs or LSM policy here; the one-shot validator only observes the kernel's +effective decision. This keeps the no-authority-expansion invariant aligned +with the access the eventual workload will receive without adding a separate, +incomplete permission model to OpenShell. + Image `VOLUME` declarations must not cover the workdir or one of its parents because Docker would mount the volume before the supervisor could validate the immutable image path. From a554b062ced98a045a2e2181e77c3bd0412a7173 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Thu, 30 Jul 2026 17:08:10 -0700 Subject: [PATCH 5/8] fix(sandbox): validate effective workspace writes Signed-off-by: Matthew Grossman --- .../src/process.rs | 95 ++++++++++++++----- 1 file changed, 69 insertions(+), 26 deletions(-) diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 69cc474ce0..31ba984db3 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1499,42 +1499,85 @@ pub fn validate_oci_workspace_as_effective_identity(root: &Path) -> Result<()> { } let is_workspace = index == last_component; - let access = if is_workspace { - Access::WRITE_OK | Access::EXEC_OK - } else { - Access::EXEC_OK - }; rustix::fs::accessat( ¤t_fd, &component, - access, + Access::EXEC_OK, AtFlags::EACCESS | AtFlags::SYMLINK_NOFOLLOW, ) .map_err(|error| { - let requirement = if is_workspace { - "writable and traversable" - } else { - "traversable" - }; miette::miette!( - "workspace path component '{}' is not {requirement} by the sandbox identity in the image: {error}", + "workspace path component '{}' is not traversable by the sandbox identity in the image: {error}", current_path.display() ) })?; - current_fd = rustix::fs::openat(¤t_fd, &component, open_flags, Mode::empty()) + let next_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)?; + reject_special_workspace_filesystem_fd(&next_fd, ¤t_path)?; + if is_workspace { + validate_effective_workspace_write(&next_fd, ¤t_path)?; + } + current_fd = next_fd; } 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}; + + let mode = Mode::RUSR | Mode::WUSR; + let tmpfile_flags = OFlags::TMPFILE | OFlags::WRONLY | OFlags::CLOEXEC; + match rustix::fs::openat(fd, ".", tmpfile_flags, mode) { + Ok(_probe) => return Ok(()), + Err(rustix::io::Errno::INVAL | rustix::io::Errno::ISDIR | rustix::io::Errno::NOTSUP) => {} + Err(error) => { + return Err(miette::miette!( + "workspace path component '{}' is not writable by the sandbox identity in the image: {error}", + path.display() + )); + } + } + + // Some filesystems do not implement O_TMPFILE. Fall back to a short-lived, + // no-follow entry. A collision fails closed after bounded retries. + let create_flags = + OFlags::CREATE | OFlags::EXCL | OFlags::WRONLY | OFlags::NOFOLLOW | OFlags::CLOEXEC; + for attempt in 0..16 { + let name = format!(".openshell-workdir-probe-{}-{attempt}", std::process::id()); + match rustix::fs::openat(fd, &name, create_flags, mode) { + Ok(_probe) => { + rustix::fs::unlinkat(fd, &name, AtFlags::empty()).map_err(|error| { + miette::miette!( + "workspace write probe cleanup failed for '{}': {error}", + path.display() + ) + })?; + return Ok(()); + } + Err(rustix::io::Errno::EXIST) => {} + Err(error) => { + return Err(miette::miette!( + "workspace path component '{}' is not writable by the sandbox identity in the image: {error}", + path.display() + )); + } + } + } + + Err(miette::miette!( + "workspace write probe could not allocate a unique entry in '{}'", + path.display() + )) +} + #[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()?; @@ -3058,10 +3101,6 @@ mod tests { #[test] #[allow(unsafe_code)] fn effective_identity_validation_honors_named_user_acl() { - if !nix::unistd::geteuid().is_root() { - return; - } - const TEST_UID: u32 = 42_234; const TEST_GID: u32 = 42_235; const ACL_XATTR_VERSION: u32 = 2; @@ -3072,6 +3111,10 @@ mod tests { const ACL_OTHER: u16 = 0x20; const ACL_UNDEFINED_ID: u32 = u32::MAX; + if !nix::unistd::geteuid().is_root() { + 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("project"); @@ -3080,17 +3123,17 @@ mod tests { let mut acl = ACL_XATTR_VERSION.to_ne_bytes().to_vec(); for (tag, permissions, id) in [ - (ACL_USER_OBJ, 0o7, ACL_UNDEFINED_ID), - (ACL_USER, 0o7, TEST_UID), - (ACL_GROUP_OBJ, 0o0, ACL_UNDEFINED_ID), - (ACL_MASK, 0o7, ACL_UNDEFINED_ID), - (ACL_OTHER, 0o0, ACL_UNDEFINED_ID), + (ACL_USER_OBJ, 0o7_u16, ACL_UNDEFINED_ID), + (ACL_USER, 0o7_u16, TEST_UID), + (ACL_GROUP_OBJ, 0o0_u16, ACL_UNDEFINED_ID), + (ACL_MASK, 0o7_u16, ACL_UNDEFINED_ID), + (ACL_OTHER, 0o0_u16, ACL_UNDEFINED_ID), ] { acl.extend_from_slice(&tag.to_ne_bytes()); acl.extend_from_slice(&permissions.to_ne_bytes()); acl.extend_from_slice(&id.to_ne_bytes()); } - let path = std::ffi::CString::new(root.as_os_str().as_encoded_bytes()).unwrap(); + let path = CString::new(root.as_os_str().as_encoded_bytes()).unwrap(); let name = c"system.posix_acl_access"; let result = unsafe { libc::setxattr( @@ -3147,13 +3190,13 @@ mod tests { policy.landlock = LandlockPolicy { compatibility: openshell_core::policy::LandlockCompatibility::HardRequirement, }; - let Ok(prepared) = crate::sandbox::prepare_current_user(&policy, None) else { + let Ok(prepared) = sandbox::linux::prepare_current_user(&policy, None) else { return; }; match unsafe { fork() }.expect("fork should succeed") { ForkResult::Child => { - let denied = crate::sandbox::enforce(prepared).is_ok() + let denied = sandbox::linux::enforce(prepared).is_ok() && validate_oci_workspace_as_effective_identity(&root).is_err(); unsafe { libc::_exit(i32::from(!denied)) }; } From 4b3530c51173140ac8dde97a52b9028eabfd9297 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Fri, 31 Jul 2026 10:06:51 -0700 Subject: [PATCH 6/8] fix(sandbox): reserve supervisor control roots Signed-off-by: Matthew Grossman --- crates/openshell-core/src/driver_mounts.rs | 71 ++++++++++++++------- crates/openshell-driver-docker/README.md | 7 +- crates/openshell-driver-docker/src/lib.rs | 4 ++ crates/openshell-driver-docker/src/tests.rs | 53 +++++++++++++++ docs/reference/sandbox-compute-drivers.mdx | 8 +-- 5 files changed, 114 insertions(+), 29 deletions(-) diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index 9e4a55d3cf..5359babaae 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -5,11 +5,6 @@ use std::path::Path; -use crate::driver_utils::{ - SANDBOX_TOKEN_MOUNT_PATH, SUPERVISOR_CONTAINER_DIR, TLS_CA_MOUNT_PATH, TLS_CERT_MOUNT_PATH, - TLS_KEY_MOUNT_PATH, UPSTREAM_PROXY_AUTH_MOUNT_PATH, -}; - /// `SELinux` relabelling mode for bind mounts. /// /// On hosts with `SELinux` enabled (e.g. Fedora, RHEL) a bind-mounted path @@ -30,22 +25,20 @@ pub enum SelinuxLabel { Private, } -/// Paths mounted or created by the local container supervisor. +/// High-level namespaces mounted or created by `OpenShell` inside containers. /// -/// Keep this list tied to concrete `OpenShell` control resources. Workspace -/// safety itself is enforced from the image's ownership and mode metadata, -/// not by guessing which distro paths are sensitive. -const OPENSHELL_CONTROL_PATHS: &[&str] = &[ - SUPERVISOR_CONTAINER_DIR, - TLS_CA_MOUNT_PATH, - TLS_CERT_MOUNT_PATH, - TLS_KEY_MOUNT_PATH, - SANDBOX_TOKEN_MOUNT_PATH, - UPSTREAM_PROXY_AUTH_MOUNT_PATH, - // Kubernetes mounts the sandbox client certificate secret here. - "/etc/openshell-tls/client", - "/etc/openshell-tls/proxy", +/// Keep privileged `OpenShell` state within these roots. This is intentionally +/// not a general Linux system-path denylist. Kernel and image-provided paths +/// have separate trust models; see NVIDIA/OpenShell#2578. +const OPENSHELL_CONTROL_ROOTS: &[&str] = &[ + "/opt/openshell", + "/etc/openshell", + "/etc/openshell-tls", + "/run/openshell", + "/run/openshell-sidecar", "/run/netns", + // The supervisor currently uses the conventional iproute2 spelling. + "/var/run/netns", ]; /// Compatibility workspace used when an OCI image has no usable working @@ -104,7 +97,7 @@ pub fn validate_mount_subpath(subpath: &str) -> Result<(), String> { pub fn validate_container_mount_target(target: &str) -> Result<(), String> { let normalized = normalize_absolute_container_path(target, "mount target")?; let path = Path::new(&normalized); - for reserved in OPENSHELL_CONTROL_PATHS { + for reserved in OPENSHELL_CONTROL_ROOTS { let reserved = Path::new(reserved); if paths_overlap(path, reserved) { return Err(format!( @@ -128,7 +121,7 @@ pub fn resolve_oci_workspace_root(working_dir: &str) -> Result { return Ok(DEFAULT_WORKSPACE_ROOT.to_string()); } let workspace_root = normalize_absolute_container_path(working_dir, "OCI WorkingDir")?; - for control_path in OPENSHELL_CONTROL_PATHS { + for control_path in OPENSHELL_CONTROL_ROOTS { validate_workspace_control_path(&workspace_root, control_path)?; } @@ -183,6 +176,23 @@ pub fn validate_workspace_control_path( Ok(()) } +/// Reject a mount that contains or is contained by a runtime-configured +/// `OpenShell` control path, such as the sandbox SSH socket. +pub fn validate_mount_control_path(target: &str, control_path: &str) -> Result<(), String> { + let normalized_target = normalize_absolute_container_path(target, "mount target")?; + let normalized_control = + normalize_absolute_container_path(control_path, "OpenShell control path")?; + if paths_overlap( + Path::new(&normalized_target), + Path::new(&normalized_control), + ) { + return Err(format!( + "mount target '{target}' conflicts with OpenShell control path '{control_path}'" + )); + } + Ok(()) +} + /// Reject a user-supplied mount that would replace or contain the resolved /// workspace root. Mounts below the workspace remain valid. pub fn validate_workspace_mount_target(target: &str, workspace_root: &str) -> Result<(), String> { @@ -274,8 +284,13 @@ mod tests { "/opt/openshell/bin/project", "/etc/openshell/tls/client", "/etc/openshell/auth", + "/etc/openshell/skills", + "/etc/openshell-tls", "/run", + "/run/openshell/cache", + "/run/openshell-sidecar/control.sock", "/run/netns/project", + "/var/run/netns/project", ] { assert!( resolve_oci_workspace_root(invalid).is_err(), @@ -307,7 +322,7 @@ mod tests { fn container_target_rejects_reserved_openshell_tls_legacy_path() { let err = validate_container_mount_target("/etc/openshell-tls/proxy/client").unwrap_err(); - assert!(err.contains("/etc/openshell-tls/proxy")); + assert!(err.contains("/etc/openshell-tls")); } #[test] @@ -320,6 +335,18 @@ mod tests { #[test] fn container_target_does_not_prefix_match_unrelated_paths() { validate_container_mount_target("/etc/openshell-tools").unwrap(); + validate_container_mount_target("/run/openshell-tools").unwrap(); + } + + #[test] + fn mount_target_rejects_runtime_configured_control_path_overlap() { + for target in ["/custom", "/custom/ssh.sock", "/custom/ssh.sock/cache"] { + assert!( + validate_mount_control_path(target, "/custom/ssh.sock").is_err(), + "expected '{target}' to conflict with the configured control path" + ); + } + validate_mount_control_path("/custom-other", "/custom/ssh.sock").unwrap(); } #[test] diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index 135a4ef733..53b92a8eb5 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -105,9 +105,10 @@ optional `selinux_label` of `shared` (applies `:z`) or `private` (applies `read_only: false` to make them writable. Mount `source`, `target`, and `subpath` values must not contain surrounding whitespace. Mount targets must be absolute container paths and must not replace or contain the resolved workspace -root. Nested workspace mounts remain valid. Mounts also must not overlap -OpenShell supervisor files, `/etc/openshell`, `/etc/openshell-tls`, or -`/run/netns`. +root. Nested workspace mounts remain valid. Mounts also must not overlap the +configured SSH socket or the reserved `/opt/openshell`, `/etc/openshell`, +`/etc/openshell-tls`, `/run/openshell`, `/run/openshell-sidecar`, and network +namespace roots. Example named-volume usage: diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index c7c3fab032..c817d66aea 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -2386,6 +2386,8 @@ fn build_container_create_body_for_image( "image-declared volume '{volume}' masks OCI WorkingDir '{workspace_root}' before workspace validation" )) })?; + driver_mounts::validate_mount_control_path(volume, &config.ssh_socket_path) + .map_err(Status::failed_precondition)?; } for mount in &driver_config.mounts { let target = match mount { @@ -2396,6 +2398,8 @@ fn build_container_create_body_for_image( }; driver_mounts::validate_workspace_mount_target(target, &workspace_root) .map_err(Status::failed_precondition)?; + driver_mounts::validate_mount_control_path(target, &config.ssh_socket_path) + .map_err(Status::failed_precondition)?; } let user_mounts = docker_driver_mounts(driver_config)?; let user_bind_strings = docker_driver_bind_strings(driver_config)?; diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index 7d1ffbcf6b..396a908b2d 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -655,6 +655,29 @@ fn container_creation_rejects_image_volume_that_masks_working_dir() { ); } +#[test] +fn container_creation_rejects_image_volume_over_configured_ssh_socket() { + let metadata = DockerImageMetadata { + id: "sha256:immutable".to_string(), + user: "1234:1235".to_string(), + working_dir: "/workspace".to_string(), + volumes: vec!["/custom-runtime".to_string()], + }; + let mut config = runtime_config(); + config.ssh_socket_path = "/custom-runtime/ssh.sock".to_string(); + + let error = build_container_create_body_for_image( + &test_sandbox(), + &config, + &DockerSandboxDriverConfig::default(), + None, + &metadata, + ) + .unwrap_err(); + + assert!(error.message().contains("OpenShell control path")); +} + #[test] fn container_creation_reserves_resolved_workspace_root_but_allows_nested_mounts() { let metadata = DockerImageMetadata { @@ -1275,6 +1298,36 @@ fn driver_config_rejects_reserved_mount_targets() { assert!(err.message().contains("reserved OpenShell path")); } +#[test] +fn driver_config_rejects_mount_over_configured_ssh_socket() { + let mount_config: DockerSandboxDriverConfig = serde_json::from_value(serde_json::json!({ + "mounts": [{ + "type": "tmpfs", + "target": "/custom-runtime" + }] + })) + .unwrap(); + let metadata = DockerImageMetadata { + id: "sha256:immutable".to_string(), + user: "1234:1235".to_string(), + working_dir: "/workspace".to_string(), + volumes: Vec::new(), + }; + let mut config = runtime_config(); + config.ssh_socket_path = "/custom-runtime/ssh.sock".to_string(); + + let error = build_container_create_body_for_image( + &test_sandbox(), + &config, + &mount_config, + None, + &metadata, + ) + .unwrap_err(); + + assert!(error.message().contains("OpenShell control path")); +} + #[test] fn docker_local_volume_with_bind_option_is_bind_backed() { let volume = inspected_volume( diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 33447e2922..8e9c45eba8 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -168,10 +168,10 @@ Docker mount schema: OpenShell rejects mount `source`, `target`, and Docker volume `subpath` values with surrounding whitespace. OpenShell also rejects mount targets that replace -the workspace root or 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. +the workspace root or container root, or contain or are contained by the +configured SSH socket or reserved `/opt/openshell`, `/etc/openshell`, +`/etc/openshell-tls`, `/run/openshell`, `/run/openshell-sidecar`, and network +namespace roots. These checks do not make host bind mounts safe. ## Podman Driver From 044179777de560d4a7025cfff0d990aeb92a5955 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Fri, 31 Jul 2026 10:38:14 -0700 Subject: [PATCH 7/8] refactor(sandbox): centralize control paths Signed-off-by: Matthew Grossman --- crates/openshell-core/src/container_paths.rs | 110 ++++++++++++++++++ crates/openshell-core/src/driver_mounts.rs | 22 +--- crates/openshell-core/src/driver_utils.rs | 48 +------- crates/openshell-core/src/lib.rs | 1 + crates/openshell-driver-docker/src/lib.rs | 2 +- .../openshell-driver-kubernetes/src/config.rs | 2 +- .../openshell-driver-kubernetes/src/driver.rs | 12 +- .../openshell-driver-kubernetes/src/main.rs | 2 +- crates/openshell-driver-podman/src/config.rs | 2 +- .../openshell-driver-podman/src/container.rs | 2 +- crates/openshell-driver-podman/src/main.rs | 2 +- crates/openshell-driver-vm/src/driver.rs | 15 +-- crates/openshell-driver-vm/src/rootfs.rs | 5 +- crates/openshell-policy/src/lib.rs | 2 +- crates/openshell-sandbox/src/lib.rs | 2 +- crates/openshell-sandbox/src/main.rs | 6 +- .../src/policy_local.rs | 2 +- .../openshell-supervisor-network/src/run.rs | 2 +- .../src/netns/mod.rs | 18 +-- 19 files changed, 159 insertions(+), 98 deletions(-) create mode 100644 crates/openshell-core/src/container_paths.rs diff --git a/crates/openshell-core/src/container_paths.rs b/crates/openshell-core/src/container_paths.rs new file mode 100644 index 0000000000..759034c8f8 --- /dev/null +++ b/crates/openshell-core/src/container_paths.rs @@ -0,0 +1,110 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Canonical paths reserved for `OpenShell` control state inside sandboxes. +//! +//! Keep fixed in-container and VM guest paths here so the code that creates or +//! consumes control state cannot drift from the mount-collision validator. + +use std::path::{Path, PathBuf}; + +pub const OPT_ROOT: &str = "/opt/openshell"; +pub const ETC_ROOT: &str = "/etc/openshell"; +pub const TLS_ROOT: &str = "/etc/openshell-tls"; +pub const RUN_ROOT: &str = "/run/openshell"; +pub const SIDECAR_RUN_ROOT: &str = "/run/openshell-sidecar"; +pub const NETNS_MOUNT_ROOT: &str = "/run/netns"; +pub const NETNS_IPROUTE2_ROOT: &str = "/var/run/netns"; + +/// High-level namespaces mounted or created by `OpenShell` inside sandboxes. +/// +/// This is intentionally not a general Linux system-path denylist. Kernel and +/// image-provided paths have separate trust models; see NVIDIA/OpenShell#2578. +pub const CONTROL_ROOTS: &[&str] = &[ + OPT_ROOT, + ETC_ROOT, + TLS_ROOT, + RUN_ROOT, + SIDECAR_RUN_ROOT, + NETNS_MOUNT_ROOT, + // The supervisor currently uses the conventional iproute2 spelling. + NETNS_IPROUTE2_ROOT, +]; + +pub const SUPERVISOR_CONTAINER_DIR: &str = "/opt/openshell/bin"; +pub const SUPERVISOR_CONTAINER_BINARY: &str = "/opt/openshell/bin/openshell-sandbox"; +pub const TLS_CLIENT_DIR: &str = "/etc/openshell/tls/client"; +pub const TLS_CA_MOUNT_PATH: &str = "/etc/openshell/tls/client/ca.crt"; +pub const TLS_CERT_MOUNT_PATH: &str = "/etc/openshell/tls/client/tls.crt"; +pub const TLS_KEY_MOUNT_PATH: &str = "/etc/openshell/tls/client/tls.key"; +pub const SANDBOX_TOKEN_MOUNT_PATH: &str = "/etc/openshell/auth/sandbox.jwt"; +pub const UPSTREAM_PROXY_AUTH_MOUNT_PATH: &str = "/etc/openshell/auth/upstream-proxy"; +pub const CONTAINER_POLICY_PATH: &str = "/etc/openshell/policy.yaml"; +pub const POLICY_ADVISOR_SKILL_PATH: &str = "/etc/openshell/skills/policy_advisor.md"; + +pub const SSH_SOCKET_PATH: &str = "/run/openshell/ssh.sock"; +pub const SIDECAR_CONTROL_SOCKET: &str = "/run/openshell-sidecar/control.sock"; +pub const SIDECAR_TLS_DIR: &str = "/etc/openshell-tls/proxy"; +pub const SIDECAR_CLIENT_TLS_DIR: &str = "/etc/openshell-tls/proxy/client"; +pub const CLIENT_TLS_DIR: &str = "/etc/openshell-tls/client"; +pub const SUPERVISOR_CA_CERT_PATH: &str = "/etc/openshell-tls/openshell-ca.pem"; +pub const SUPERVISOR_CA_BUNDLE_PATH: &str = "/etc/openshell-tls/ca-bundle.pem"; + +pub const VM_GUEST_TLS_CA_PATH: &str = "/opt/openshell/tls/ca.crt"; +pub const VM_GUEST_TLS_CERT_PATH: &str = "/opt/openshell/tls/tls.crt"; +pub const VM_GUEST_TLS_KEY_PATH: &str = "/opt/openshell/tls/tls.key"; +pub const VM_GUEST_SANDBOX_TOKEN_PATH: &str = "/opt/openshell/auth/sandbox.jwt"; +pub const VM_GUEST_INIT_DROPIN_DIR: &str = "/opt/openshell/init.d"; +pub const VM_GUEST_INIT_DROPIN_MANIFEST: &str = "/opt/openshell/init.d.manifest"; +pub const VM_UMOCI_PATH: &str = "/opt/openshell/bin/umoci"; +pub const VM_SANDBOX_OWNER_NORMALIZED_MARKER: &str = "/opt/openshell/.sandbox-owner-normalized"; + +/// Return the conventional iproute2 path for a named network namespace. +pub fn netns_path(name: &str) -> PathBuf { + Path::new(NETNS_IPROUTE2_ROOT).join(name) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn every_fixed_control_path_is_below_a_reserved_root() { + let paths = [ + SUPERVISOR_CONTAINER_DIR, + SUPERVISOR_CONTAINER_BINARY, + TLS_CLIENT_DIR, + TLS_CA_MOUNT_PATH, + TLS_CERT_MOUNT_PATH, + TLS_KEY_MOUNT_PATH, + SANDBOX_TOKEN_MOUNT_PATH, + UPSTREAM_PROXY_AUTH_MOUNT_PATH, + CONTAINER_POLICY_PATH, + POLICY_ADVISOR_SKILL_PATH, + SSH_SOCKET_PATH, + SIDECAR_CONTROL_SOCKET, + SIDECAR_TLS_DIR, + SIDECAR_CLIENT_TLS_DIR, + CLIENT_TLS_DIR, + SUPERVISOR_CA_CERT_PATH, + SUPERVISOR_CA_BUNDLE_PATH, + VM_GUEST_TLS_CA_PATH, + VM_GUEST_TLS_CERT_PATH, + VM_GUEST_TLS_KEY_PATH, + VM_GUEST_SANDBOX_TOKEN_PATH, + VM_GUEST_INIT_DROPIN_DIR, + VM_GUEST_INIT_DROPIN_MANIFEST, + VM_UMOCI_PATH, + VM_SANDBOX_OWNER_NORMALIZED_MARKER, + ]; + + for path in paths { + assert!( + CONTROL_ROOTS + .iter() + .any(|root| Path::new(path).starts_with(root)), + "fixed control path {path} is outside the reserved roots" + ); + } + } +} diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index 5359babaae..5db7e50e74 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -5,6 +5,8 @@ use std::path::Path; +use crate::container_paths::CONTROL_ROOTS; + /// `SELinux` relabelling mode for bind mounts. /// /// On hosts with `SELinux` enabled (e.g. Fedora, RHEL) a bind-mounted path @@ -25,22 +27,6 @@ pub enum SelinuxLabel { Private, } -/// High-level namespaces mounted or created by `OpenShell` inside containers. -/// -/// Keep privileged `OpenShell` state within these roots. This is intentionally -/// not a general Linux system-path denylist. Kernel and image-provided paths -/// have separate trust models; see NVIDIA/OpenShell#2578. -const OPENSHELL_CONTROL_ROOTS: &[&str] = &[ - "/opt/openshell", - "/etc/openshell", - "/etc/openshell-tls", - "/run/openshell", - "/run/openshell-sidecar", - "/run/netns", - // The supervisor currently uses the conventional iproute2 spelling. - "/var/run/netns", -]; - /// Compatibility workspace used when an OCI image has no usable working /// directory and by drivers whose workspace remains fixed. pub const DEFAULT_WORKSPACE_ROOT: &str = "/sandbox"; @@ -97,7 +83,7 @@ pub fn validate_mount_subpath(subpath: &str) -> Result<(), String> { pub fn validate_container_mount_target(target: &str) -> Result<(), String> { let normalized = normalize_absolute_container_path(target, "mount target")?; let path = Path::new(&normalized); - for reserved in OPENSHELL_CONTROL_ROOTS { + for reserved in CONTROL_ROOTS { let reserved = Path::new(reserved); if paths_overlap(path, reserved) { return Err(format!( @@ -121,7 +107,7 @@ pub fn resolve_oci_workspace_root(working_dir: &str) -> Result { return Ok(DEFAULT_WORKSPACE_ROOT.to_string()); } let workspace_root = normalize_absolute_container_path(working_dir, "OCI WorkingDir")?; - for control_path in OPENSHELL_CONTROL_ROOTS { + for control_path in CONTROL_ROOTS { validate_workspace_control_path(&workspace_root, control_path)?; } diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index a5bcc55ad3..9bcca9f11d 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -7,6 +7,11 @@ use std::path::PathBuf; use crate::proto::compute::v1::{DriverSandbox, GetCapabilitiesResponse}; +pub use crate::container_paths::{ + SANDBOX_TOKEN_MOUNT_PATH, SUPERVISOR_CONTAINER_BINARY, SUPERVISOR_CONTAINER_DIR, + TLS_CA_MOUNT_PATH, TLS_CERT_MOUNT_PATH, TLS_KEY_MOUNT_PATH, UPSTREAM_PROXY_AUTH_MOUNT_PATH, +}; + // --------------------------------------------------------------------------- // Sandbox container/pod label keys (openshell.ai/ namespace) // --------------------------------------------------------------------------- @@ -46,49 +51,6 @@ pub fn openshell_sandbox_label_selector() -> String { /// path used when building the `openshell-sandbox` image layer. pub const SUPERVISOR_IMAGE_BINARY_PATH: &str = "/openshell-sandbox"; -/// Directory inside sandbox containers where the supervisor binary is mounted. -/// -/// Compute drivers that side-load the supervisor into a shared volume mount -/// the binary here so the sandbox container can execute it from a fixed path. -pub const SUPERVISOR_CONTAINER_DIR: &str = "/opt/openshell/bin"; - -/// Full path to the supervisor binary inside sandbox containers. -/// -/// Equals `SUPERVISOR_CONTAINER_DIR + "/openshell-sandbox"`. Use this when -/// the full executable path is needed (Docker entrypoint, Podman entrypoint, -/// VM rootfs injection). Use `SUPERVISOR_CONTAINER_DIR` when only the -/// directory mount-point is needed (Kubernetes emptyDir volume mount). -pub const SUPERVISOR_CONTAINER_BINARY: &str = "/opt/openshell/bin/openshell-sandbox"; - -// --------------------------------------------------------------------------- -// In-container mount paths for guest TLS materials and the sandbox token. -// -// All container-based drivers (Docker, Podman, Kubernetes) mount the gateway's -// mTLS client credentials at these fixed paths inside every sandbox container. -// The supervisor reads these paths on startup to establish its gRPC-over-mTLS -// connection back to the gateway. The paths must remain stable across driver -// versions since the supervisor binary is built and packaged separately. -// --------------------------------------------------------------------------- - -/// Container-side mount path for the guest mTLS CA certificate. -pub const TLS_CA_MOUNT_PATH: &str = "/etc/openshell/tls/client/ca.crt"; - -/// Container-side mount path for the guest mTLS client certificate. -pub const TLS_CERT_MOUNT_PATH: &str = "/etc/openshell/tls/client/tls.crt"; - -/// Container-side mount path for the guest mTLS client private key. -pub const TLS_KEY_MOUNT_PATH: &str = "/etc/openshell/tls/client/tls.key"; - -/// Container-side mount path for the per-sandbox JWT token. -pub const SANDBOX_TOKEN_MOUNT_PATH: &str = "/etc/openshell/auth/sandbox.jwt"; - -/// Container-side mount path for the corporate upstream-proxy credentials. -/// -/// The file holds the `user:pass` userinfo used to build the -/// `Proxy-Authorization` header. It is delivered through a root-only secret -/// mount so the credential never appears in container environment/metadata. -pub const UPSTREAM_PROXY_AUTH_MOUNT_PATH: &str = "/etc/openshell/auth/upstream-proxy"; - /// A validated corporate upstream-proxy address. /// /// Produced by [`parse_upstream_proxy_url`], which is the single source of diff --git a/crates/openshell-core/src/lib.rs b/crates/openshell-core/src/lib.rs index 80bfbb046d..faae83837d 100644 --- a/crates/openshell-core/src/lib.rs +++ b/crates/openshell-core/src/lib.rs @@ -12,6 +12,7 @@ pub mod activity; pub mod auth; pub mod config; +pub mod container_paths; pub mod denial; pub mod driver_mounts; pub mod driver_utils; diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index c817d66aea..9ddb73fd60 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -152,7 +152,7 @@ impl Default for DockerComputeConfig { guest_tls_key: None, network_name: DEFAULT_DOCKER_NETWORK_NAME.to_string(), host_gateway_ip: String::new(), - ssh_socket_path: "/run/openshell/ssh.sock".to_string(), + ssh_socket_path: openshell_core::container_paths::SSH_SOCKET_PATH.to_string(), sandbox_pids_limit: DEFAULT_SANDBOX_PIDS_LIMIT, enable_bind_mounts: false, } diff --git a/crates/openshell-driver-kubernetes/src/config.rs b/crates/openshell-driver-kubernetes/src/config.rs index fb471180a9..5311f56436 100644 --- a/crates/openshell-driver-kubernetes/src/config.rs +++ b/crates/openshell-driver-kubernetes/src/config.rs @@ -347,7 +347,7 @@ impl Default for KubernetesComputeConfig { topology: SupervisorTopology::default(), sidecar: KubernetesSidecarConfig::default(), grpc_endpoint: String::new(), - ssh_socket_path: "/run/openshell/ssh.sock".to_string(), + ssh_socket_path: openshell_core::container_paths::SSH_SOCKET_PATH.to_string(), client_tls_secret_name: String::new(), host_gateway_ip: String::new(), enable_user_namespaces: false, diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index 56361ef59c..2f1ea72a32 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -1436,8 +1436,8 @@ const BINARY_AWARE_SIDECAR_PROXY_UID: u32 = 0; /// Shared volume used by the network sidecar and process-only supervisor for /// local coordination in sidecar topology. const SIDECAR_STATE_VOLUME_NAME: &str = "openshell-sidecar-state"; -const SIDECAR_STATE_MOUNT_PATH: &str = "/run/openshell-sidecar"; -const SIDECAR_CONTROL_SOCKET: &str = "/run/openshell-sidecar/control.sock"; +const SIDECAR_STATE_MOUNT_PATH: &str = openshell_core::container_paths::SIDECAR_RUN_ROOT; +const SIDECAR_CONTROL_SOCKET: &str = openshell_core::container_paths::SIDECAR_CONTROL_SOCKET; // Linux abstract socket names are scoped to the pod's shared network namespace. // Unlike a filesystem socket in the shared state volume, the workload cannot // unlink and replace this relay endpoint after the trusted supervisor binds it. @@ -1446,8 +1446,8 @@ const SIDECAR_SSH_SOCKET_FILE: &str = "@openshell-sidecar-ssh"; /// Shared TLS work directory. The network sidecar writes the proxy CA bundle /// here, while the agent container consumes it after sidecar bootstrap. const SIDECAR_TLS_VOLUME_NAME: &str = "openshell-supervisor-tls"; -const SIDECAR_TLS_MOUNT_PATH: &str = "/etc/openshell-tls/proxy"; -const SIDECAR_CLIENT_TLS_MOUNT_PATH: &str = "/etc/openshell-tls/proxy/client"; +const SIDECAR_TLS_MOUNT_PATH: &str = openshell_core::container_paths::SIDECAR_TLS_DIR; +const SIDECAR_CLIENT_TLS_MOUNT_PATH: &str = openshell_core::container_paths::SIDECAR_CLIENT_TLS_DIR; /// Build the emptyDir volume that holds the supervisor binary. /// @@ -1849,7 +1849,7 @@ fn supervisor_network_init_container(params: &SandboxPodParams<'_>) -> serde_jso .expect("volumeMounts is an array") .push(serde_json::json!({ "name": "openshell-client-tls", - "mountPath": "/etc/openshell-tls/client", + "mountPath": openshell_core::container_paths::CLIENT_TLS_DIR, "readOnly": true })); } @@ -2570,7 +2570,7 @@ fn sandbox_template_to_k8s_with_validated_config( if !params.client_tls_secret_name.is_empty() { volume_mounts.push(serde_json::json!({ "name": CLIENT_TLS_VOLUME_NAME, - "mountPath": "/etc/openshell-tls/client", + "mountPath": openshell_core::container_paths::CLIENT_TLS_DIR, "readOnly": true })); } diff --git a/crates/openshell-driver-kubernetes/src/main.rs b/crates/openshell-driver-kubernetes/src/main.rs index c7b0939888..b7d5514ac2 100644 --- a/crates/openshell-driver-kubernetes/src/main.rs +++ b/crates/openshell-driver-kubernetes/src/main.rs @@ -58,7 +58,7 @@ struct Args { #[arg( long, env = "OPENSHELL_SANDBOX_SSH_SOCKET_PATH", - default_value = "/run/openshell/ssh.sock" + default_value = openshell_core::container_paths::SSH_SOCKET_PATH )] sandbox_ssh_socket_path: String, diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index 2d226397b4..50311836ce 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -364,7 +364,7 @@ impl Default for PodmanComputeConfig { image_pull_policy: ImagePullPolicy::default(), grpc_endpoint: String::new(), gateway_port: openshell_core::config::DEFAULT_SERVER_PORT, - sandbox_ssh_socket_path: "/run/openshell/ssh.sock".to_string(), + sandbox_ssh_socket_path: openshell_core::container_paths::SSH_SOCKET_PATH.to_string(), network_name: DEFAULT_NETWORK_NAME.to_string(), host_gateway_ip: Self::default_host_gateway_ip(), stop_timeout_secs: DEFAULT_PODMAN_STOP_TIMEOUT_SECS, diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 223d2b43eb..005f688a19 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -1133,7 +1133,7 @@ pub fn build_container_spec_for_image( let mut m = vec![Mount { kind: "tmpfs".into(), source: "tmpfs".into(), - destination: "/run/netns".into(), + destination: openshell_core::container_paths::NETNS_MOUNT_ROOT.into(), options: vec!["rw".into(), "nosuid".into(), "nodev".into()], }]; // Bind-mount client TLS materials into the container when mTLS diff --git a/crates/openshell-driver-podman/src/main.rs b/crates/openshell-driver-podman/src/main.rs index 4a38643f38..e287075886 100644 --- a/crates/openshell-driver-podman/src/main.rs +++ b/crates/openshell-driver-podman/src/main.rs @@ -67,7 +67,7 @@ struct Args { #[arg( long, env = "OPENSHELL_SANDBOX_SSH_SOCKET_PATH", - default_value = "/run/openshell/ssh.sock" + default_value = openshell_core::container_paths::SSH_SOCKET_PATH )] sandbox_ssh_socket_path: String, diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index 7af0ddc389..060e115681 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -142,12 +142,12 @@ const OPENSHELL_HOST_GATEWAY_ALIAS: &str = "host.openshell.internal"; /// Both names ultimately route through the gvproxy NAT path on /// `GVPROXY_HOST_LOOPBACK_IP` — they do **not** go through the gateway IP. const GVPROXY_HOST_LOOPBACK_ALIAS: &str = OPENSHELL_HOST_GATEWAY_ALIAS; -const GUEST_SSH_SOCKET_PATH: &str = "/run/openshell/ssh.sock"; -const GUEST_TLS_CA_PATH: &str = "/opt/openshell/tls/ca.crt"; -const GUEST_TLS_CERT_PATH: &str = "/opt/openshell/tls/tls.crt"; -const GUEST_TLS_KEY_PATH: &str = "/opt/openshell/tls/tls.key"; -const GUEST_SANDBOX_TOKEN_PATH: &str = "/opt/openshell/auth/sandbox.jwt"; -const GUEST_INIT_DROPIN_DIR: &str = "/opt/openshell/init.d"; +const GUEST_SSH_SOCKET_PATH: &str = openshell_core::container_paths::SSH_SOCKET_PATH; +const GUEST_TLS_CA_PATH: &str = openshell_core::container_paths::VM_GUEST_TLS_CA_PATH; +const GUEST_TLS_CERT_PATH: &str = openshell_core::container_paths::VM_GUEST_TLS_CERT_PATH; +const GUEST_TLS_KEY_PATH: &str = openshell_core::container_paths::VM_GUEST_TLS_KEY_PATH; +const GUEST_SANDBOX_TOKEN_PATH: &str = openshell_core::container_paths::VM_GUEST_SANDBOX_TOKEN_PATH; +const GUEST_INIT_DROPIN_DIR: &str = openshell_core::container_paths::VM_GUEST_INIT_DROPIN_DIR; /// Guest path of the driver-authored manifest enumerating which /// `init.d` drop-ins the guest init script is allowed to execute. /// @@ -155,7 +155,8 @@ const GUEST_INIT_DROPIN_DIR: &str = "/opt/openshell/init.d"; /// else found under `init.d` — e.g. files baked into a user-controlled /// guest image — is ignored. The driver writes this file into the overlay /// upperdir on every launch, so the image cannot forge or shadow it. -const GUEST_INIT_DROPIN_MANIFEST: &str = "/opt/openshell/init.d.manifest"; +const GUEST_INIT_DROPIN_MANIFEST: &str = + openshell_core::container_paths::VM_GUEST_INIT_DROPIN_MANIFEST; const IMAGE_CACHE_ROOT_DIR: &str = "images"; const IMAGE_CACHE_ROOTFS_IMAGE: &str = "rootfs.ext4"; const OVERLAY_TEMPLATE_CACHE_DIR: &str = "overlay-templates"; diff --git a/crates/openshell-driver-vm/src/rootfs.rs b/crates/openshell-driver-vm/src/rootfs.rs index c71ebe6884..9046913c9d 100644 --- a/crates/openshell-driver-vm/src/rootfs.rs +++ b/crates/openshell-driver-vm/src/rootfs.rs @@ -15,8 +15,9 @@ const UMOCI: &[u8] = include_bytes!(concat!(env!("OUT_DIR"), "/umoci.zst")); const ROOTFS_VARIANT_MARKER: &str = ".openshell-rootfs-variant"; const SANDBOX_GUEST_INIT_PATH: &str = "/srv/openshell-vm-sandbox-init.sh"; const SANDBOX_SUPERVISOR_PATH: &str = openshell_core::driver_utils::SUPERVISOR_CONTAINER_BINARY; -const SANDBOX_UMOCI_PATH: &str = "/opt/openshell/bin/umoci"; -const SANDBOX_OWNER_NORMALIZED_MARKER: &str = "/opt/openshell/.sandbox-owner-normalized"; +const SANDBOX_UMOCI_PATH: &str = openshell_core::container_paths::VM_UMOCI_PATH; +const SANDBOX_OWNER_NORMALIZED_MARKER: &str = + openshell_core::container_paths::VM_SANDBOX_OWNER_NORMALIZED_MARKER; const ROOTFS_IMAGE_MIN_SIZE_BYTES: u64 = 512 * 1024 * 1024; const ROOTFS_IMAGE_MIN_HEADROOM_BYTES: u64 = 256 * 1024 * 1024; const EXT4_IMAGE_MIN_HEADROOM_BYTES: u64 = 16 * 1024 * 1024; diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index 1e81451c7b..821bc2bdc5 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -1040,7 +1040,7 @@ pub fn load_sandbox_policy(cli_path: Option<&str>) -> Result { let tls_dir = std::env::var(openshell_core::sandbox_env::PROXY_TLS_DIR) - .unwrap_or_else(|_| "/etc/openshell-tls".to_string()); + .unwrap_or_else(|_| openshell_core::container_paths::TLS_ROOT.to_string()); let tls_dir = std::path::Path::new(&tls_dir); let system_ca_bundle = read_system_ca_bundle(); match write_ca_files(&ca, tls_dir, &system_ca_bundle) { diff --git a/crates/openshell-supervisor-process/src/netns/mod.rs b/crates/openshell-supervisor-process/src/netns/mod.rs index 44e9470931..bd934da14e 100644 --- a/crates/openshell-supervisor-process/src/netns/mod.rs +++ b/crates/openshell-supervisor-process/src/netns/mod.rs @@ -153,9 +153,9 @@ impl NetworkNamespace { } // Open the namespace file descriptor for later use with setns - let ns_path = format!("/var/run/netns/{name}"); + let ns_path = openshell_core::container_paths::netns_path(&name); let ns_fd = match nix::fcntl::open( - ns_path.as_str(), + ns_path.as_path(), nix::fcntl::OFlag::O_RDONLY, nix::sys::stat::Mode::empty(), ) { @@ -731,8 +731,8 @@ fn run_nft_commands_current_namespace( fn run_ip_netns(netns: &str, args: &[&str]) -> Result<()> { let ip_path = find_trusted_binary("ip", IP_SEARCH_PATHS)?; let nsenter_path = find_trusted_binary("nsenter", NSENTER_SEARCH_PATHS)?; - let ns_path = format!("/var/run/netns/{netns}"); - let net_flag = format!("--net={ns_path}"); + let ns_path = openshell_core::container_paths::netns_path(netns); + let net_flag = format!("--net={}", ns_path.display()); let mut full_args = vec![net_flag.as_str(), "--", ip_path]; full_args.extend(args); @@ -751,7 +751,7 @@ fn run_ip_netns(netns: &str, args: &[&str]) -> Result<()> { let stderr = String::from_utf8_lossy(&output.stderr); return Err(miette::miette!( "{nsenter_path} --net={} {ip_path} {} failed: {}", - ns_path, + ns_path.display(), args.join(" "), stderr.trim() )); @@ -770,8 +770,8 @@ fn run_nft_commands_netns( commands: &[nft_ruleset::NftCommand], ) -> Result<()> { let nsenter_path = find_trusted_binary("nsenter", NSENTER_SEARCH_PATHS)?; - let ns_path = format!("/var/run/netns/{netns}"); - let net_flag = format!("--net={ns_path}"); + let ns_path = openshell_core::container_paths::netns_path(netns); + let net_flag = format!("--net={}", ns_path.display()); for cmd in commands { let args_str = cmd.args.join(" "); @@ -972,8 +972,8 @@ fe800000000000000000000000000001 02 40 20 80 eth0 let name = ns.name().to_string(); // Verify namespace exists - let ns_path = format!("/var/run/netns/{name}"); - assert!(Path::new(&ns_path).exists(), "Namespace file should exist"); + let ns_path = openshell_core::container_paths::netns_path(&name); + assert!(ns_path.exists(), "Namespace file should exist"); // Verify IPs are set correctly assert_eq!( From 27eea969d48ab6714f707c07e846b94e1dc8e856 Mon Sep 17 00:00:00 2001 From: Matthew Grossman Date: Fri, 31 Jul 2026 11:47:44 -0700 Subject: [PATCH 8/8] fix(sandbox): reserve OCI runtime mount roots Signed-off-by: Matthew Grossman --- architecture/compute-runtimes.md | 5 +- crates/openshell-core/src/container_paths.rs | 29 ++++++++ crates/openshell-core/src/driver_mounts.rs | 49 ++++++++++++-- crates/openshell-driver-docker/README.md | 8 +-- .../src/process.rs | 66 ------------------- docs/reference/sandbox-compute-drivers.mdx | 4 +- 6 files changed, 81 insertions(+), 80 deletions(-) diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index 2b13af0f32..646b6320bd 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -207,8 +207,9 @@ identity, including supplementary groups, must already be able to traverse every parent and write and enter the workdir; OpenShell does not change that directory's ownership or mode. A one-shot validator drops to that identity and uses kernel effective-access checks so POSIX ACL and LSM decisions are honored. -Filesystem metadata identifies kernel-managed -mounts, while collision checks are derived from actual OpenShell control paths. +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 diff --git a/crates/openshell-core/src/container_paths.rs b/crates/openshell-core/src/container_paths.rs index 759034c8f8..c63e4bcdd8 100644 --- a/crates/openshell-core/src/container_paths.rs +++ b/crates/openshell-core/src/container_paths.rs @@ -16,6 +16,15 @@ pub const SIDECAR_RUN_ROOT: &str = "/run/openshell-sidecar"; pub const NETNS_MOUNT_ROOT: &str = "/run/netns"; pub const NETNS_IPROUTE2_ROOT: &str = "/var/run/netns"; +/// Standard Linux container namespaces that an image-selected workspace must +/// not contain or enter. +/// +/// These roots cover the default filesystems and devices defined by the OCI +/// Runtime Specification: procfs, sysfs, cgroups, device nodes, devpts, shared +/// memory, and POSIX message queues. +/// +pub const OCI_RUNTIME_MOUNT_ROOTS: &[&str] = &["/proc", "/sys", "/dev"]; + /// High-level namespaces mounted or created by `OpenShell` inside sandboxes. /// /// This is intentionally not a general Linux system-path denylist. Kernel and @@ -107,4 +116,24 @@ mod tests { ); } } + + #[test] + fn runtime_roots_cover_standard_oci_mount_destinations() { + for path in [ + "/proc", + "/dev", + "/dev/pts", + "/dev/shm", + "/dev/mqueue", + "/sys", + "/sys/fs/cgroup", + ] { + assert!( + OCI_RUNTIME_MOUNT_ROOTS + .iter() + .any(|root| Path::new(path).starts_with(root)), + "OCI runtime mount {path} is outside the reserved roots" + ); + } + } } diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index 5db7e50e74..1157f0bc24 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -5,7 +5,7 @@ use std::path::Path; -use crate::container_paths::CONTROL_ROOTS; +use crate::container_paths::{CONTROL_ROOTS, OCI_RUNTIME_MOUNT_ROOTS}; /// `SELinux` relabelling mode for bind mounts. /// @@ -107,6 +107,9 @@ pub fn resolve_oci_workspace_root(working_dir: &str) -> Result { return Ok(DEFAULT_WORKSPACE_ROOT.to_string()); } let workspace_root = normalize_absolute_container_path(working_dir, "OCI WorkingDir")?; + for runtime_path in OCI_RUNTIME_MOUNT_ROOTS { + validate_workspace_reserved_path(&workspace_root, runtime_path, "OCI runtime mount")?; + } for control_path in CONTROL_ROOTS { validate_workspace_control_path(&workspace_root, control_path)?; } @@ -152,11 +155,21 @@ pub fn validate_workspace_control_path( workspace_root: &str, control_path: &str, ) -> Result<(), String> { - let workspace = Path::new(workspace_root); - let control = Path::new(control_path); - if paths_overlap(workspace, control) { + validate_workspace_reserved_path(workspace_root, control_path, "OpenShell control path") +} + +fn validate_workspace_reserved_path( + workspace_root: &str, + reserved_path: &str, + description: &str, +) -> Result<(), String> { + let normalized_workspace = normalize_absolute_container_path(workspace_root, "OCI WorkingDir")?; + let normalized_reserved = normalize_absolute_container_path(reserved_path, description)?; + let workspace = Path::new(&normalized_workspace); + let reserved = Path::new(&normalized_reserved); + if paths_overlap(workspace, reserved) { return Err(format!( - "OCI WorkingDir '{workspace_root}' conflicts with OpenShell control path '{control_path}'" + "OCI WorkingDir '{workspace_root}' conflicts with {description} '{reserved_path}'" )); } Ok(()) @@ -262,8 +275,14 @@ mod tests { } #[test] - fn oci_workspace_root_rejects_only_openshell_control_path_collisions() { + fn oci_workspace_root_rejects_runtime_and_openshell_control_path_collisions() { for invalid in [ + "/proc", + "/proc/self", + "/sys", + "/sys/fs/cgroup", + "/dev", + "/dev/shm", "/etc", "/opt", "/opt/openshell", @@ -295,6 +314,9 @@ mod tests { "/var/app/current", "/var/task", "/var/www/app", + "/processor", + "/system", + "/device", ] { assert_eq!( resolve_oci_workspace_root(valid).unwrap(), @@ -335,6 +357,21 @@ mod tests { validate_mount_control_path("/custom-other", "/custom/ssh.sock").unwrap(); } + #[test] + fn workspace_rejects_malformed_runtime_control_paths() { + for control_path in [ + "workspace/ssh.sock", + "/workspace/../run/ssh.sock", + "/workspace//ssh.sock", + "", + ] { + assert!( + validate_workspace_control_path("/workspace", control_path).is_err(), + "expected malformed control path '{control_path}' to be rejected" + ); + } + } + #[test] fn mount_subpath_must_be_relative_without_parent_dirs() { assert!(validate_mount_subpath("project/a").is_ok()); diff --git a/crates/openshell-driver-docker/README.md b/crates/openshell-driver-docker/README.md index 53b92a8eb5..05faf53c5c 100644 --- a/crates/openshell-driver-docker/README.md +++ b/crates/openshell-driver-docker/README.md @@ -49,10 +49,10 @@ incomplete permission model to OpenShell. Image `VOLUME` declarations must not cover the workdir or one of its parents because Docker would mount the volume before the supervisor could validate the immutable image path. -Kernel-managed filesystems are detected from filesystem metadata, and paths -that overlap concrete OpenShell control resources are rejected. The workspace -is the child cwd and `HOME`. The supervisor starts from `/`, then reports an -invalid workdir as a readiness failure. +Workdirs under the standard OCI runtime namespaces `/proc`, `/sys`, and `/dev` +are rejected, as are paths that overlap concrete OpenShell control resources. +The workspace is the child cwd and `HOME`. The supervisor starts from `/`, then +reports an invalid workdir as a readiness failure. Docker containers join an OpenShell-managed bridge network. The driver injects `host.openshell.internal` and `host.docker.internal` so supervisors have stable diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 31ba984db3..659fe3dc06 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1427,8 +1427,6 @@ 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 { @@ -1452,7 +1450,6 @@ 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, ".", @@ -1519,7 +1516,6 @@ pub fn validate_oci_workspace_as_effective_identity(root: &Path) -> Result<()> { current_path.display() ) })?; - reject_special_workspace_filesystem_fd(&next_fd, ¤t_path)?; if is_workspace { validate_effective_workspace_write(&next_fd, ¤t_path)?; } @@ -1578,55 +1574,6 @@ fn validate_effective_workspace_write(fd: &impl std::os::fd::AsFd, path: &Path) )) } -#[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(()) -} - /// Prepare only the resolved `OpenShell` workspace directory itself. /// /// Image-provided children retain their declared ownership. This avoids @@ -3253,19 +3200,6 @@ mod tests { assert!(error.to_string().contains("symlink")); } - #[cfg(target_os = "linux")] - #[test] - fn validate_oci_workspace_rejects_kernel_filesystem_by_metadata() { - let error = validate_oci_workspace( - Path::new("/proc"), - Some(nix::unistd::geteuid()), - Some(nix::unistd::getegid()), - &[], - ) - .unwrap_err(); - assert!(error.to_string().contains("kernel-managed filesystem")); - } - #[cfg(unix)] #[test] fn prepare_oci_workspace_makes_existing_root_owner_writable() { diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 8e9c45eba8..0e5f6435f4 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -443,8 +443,8 @@ UID/GID and supplementary groups must already be able to traverse every parent and write and enter the workdir. OpenShell does not change that directory's ownership or mode. A one-shot validator drops to that identity and uses kernel effective-access checks, including POSIX ACL grants and LSM denials. It rejects -kernel-managed filesystems based on filesystem -metadata and rejects overlap with actual OpenShell control paths. Docker checks +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