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() ) }