From f5508202e5de30185d8e79154dec3712947f9b87 Mon Sep 17 00:00:00 2001 From: loveRhythm1990 Date: Sun, 6 Sep 2026 17:00:06 +0800 Subject: [PATCH] fix(kubernetes): preserve sandbox ownership when seeding workspace The workspace-init container seeds a fresh PVC as root and extracted with --no-same-owner, so every seeded path landed owned by uid 0. A mode-0700 home directory shipped by the image (~/.config, ~/.cache) was then unreachable for the workload. fsGroup does not compensate: kubelet applies it when the volume is mounted, which is before the init container writes anything. Under the sidecar topology the supervisor's privileged workspace reconciliation does not run, so the seeded ownership is load-bearing. Rewrite ownership to the resolved sandbox identity while building the transfer archive instead. Seeding stays root, so images that ship root-owned private content still seed, and no recursive chown runs over the workspace (the pattern that broke on read-only submounts in #2294). Modes and timestamps are still not restored. --owner/--group are GNU extensions and this init container runs the sandbox image itself, so the script probes tar once and falls back to the previous flags on a minimal base such as Alpine, which provides BusyBox tar. Those images keep the behavior they have today rather than failing to start. Signed-off-by: loveRhythm1990 --- architecture/compute-runtimes.md | 15 ++ .../openshell-driver-kubernetes/src/driver.rs | 177 +++++++++++++++++- docs/reference/sandbox-compute-drivers.mdx | 3 +- 3 files changed, 185 insertions(+), 10 deletions(-) diff --git a/architecture/compute-runtimes.md b/architecture/compute-runtimes.md index e1e731a0ce..06433fcc29 100644 --- a/architecture/compute-runtimes.md +++ b/architecture/compute-runtimes.md @@ -391,6 +391,21 @@ identity is absent, malformed, unknown, ambiguous, or resolves to UID/GID 0. The supervisor itself remains root so it can establish isolation before starting unprivileged children. +When the Kubernetes driver seeds a fresh workspace PVC from the image, the +seeded tree is written as the resolved sandbox UID/GID. The init container runs +as root so it can read every source path, and rewrites ownership while building +the transfer archive rather than chowning the workspace afterwards. Ownership +must be established during seeding: `fsGroup` is applied by kubelet when the +volume is mounted, which is before the init container writes anything, and the +sidecar topology does not perform the supervisor's privileged workspace +reconciliation. Modes and timestamps are not restored, so a nested read-only +mount under the workspace is never chmod'ed during seeding. + +The ownership rewrite uses GNU tar extensions, and the init container runs the +sandbox image itself. Images whose tar lacks them — a minimal base such as +Alpine, which provides BusyBox tar — seed with the previous flags instead, so +they keep the behavior they have today rather than failing to start. + Kubernetes can run the supervisor in the default combined topology or in a sidecar topology. Combined mode keeps network and process supervision in the agent container. Sidecar mode runs network enforcement, the proxy, and gateway diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index bb1b75e8a9..46e9c738c8 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -3408,11 +3408,15 @@ fn apply_supervisor_sidecar_topology( /// The init container mounts the PVC at a temporary path so it can still see /// the image's `/sandbox` directory. It checks for a sentinel file and skips /// the copy if the PVC was already initialised. +/// +/// Seeded content is written as `sandbox_uid`/`sandbox_gid` so the workload can +/// use it. See the extended note on the tar invocation below. #[allow(clippy::similar_names)] fn apply_workspace_persistence( pod_template: &mut serde_json::Value, image: &str, image_pull_policy: &str, + sandbox_uid: u32, sandbox_gid: u32, ) { let Some(spec) = pod_template.get_mut("spec").and_then(|v| v.as_object_mut()) else { @@ -3470,8 +3474,32 @@ fn apply_workspace_persistence( // is valid. `tar` copies the tree without dereferencing those links. // Archive only the contents, not the `/sandbox` directory entry // itself, so extraction never tries to chmod the PVC mount root. - // Extract without restoring owner, mode, or timestamps so the - // non-root init container can seed kubelet-owned PVCs. + // + // Ownership is rewritten to the resolved sandbox identity while + // *building* the archive (`--owner`/`--group`/`--numeric-owner`), and + // extraction then restores it. Seeding as root with `--no-same-owner` + // used to leave every seeded path owned by uid 0; a mode-0700 home + // directory from the image (`~/.config`, `~/.cache`) was then + // unreachable for the workload. `fsGroup` does not compensate: kubelet + // applies it when the volume is mounted, which is before this init + // container writes anything. + // + // Rewriting at archive time — rather than extracting as the sandbox + // user, or chowning the tree afterwards — keeps root's ability to read + // every source path, so images that ship root-owned private content + // still seed. It also avoids a recursive chown over the workspace, the + // pattern that broke on read-only submounts in #2294. + // + // Modes and timestamps are still not restored, so a nested read-only + // mount under the PVC is never chmod'ed during seeding. + // + // `--owner`/`--group` at archive time are GNU extensions, and this init + // container runs the sandbox image itself — a minimal base such as + // Alpine seeds with BusyBox tar, which rejects them and would fail the + // init container, leaving the pod unable to start. The script probes + // tar once and falls back to the previous flags when the extensions are + // unavailable: such images keep seeding exactly as they do today rather + // than regressing into a startup failure. // // The inner `[ -d ... ]` guard handles custom images that don't have // a /sandbox directory — the copy is skipped but the sentinel is @@ -3479,10 +3507,14 @@ fn apply_workspace_persistence( let copy_cmd = format!( "if [ ! -f {WORKSPACE_INIT_MOUNT_PATH}/{WORKSPACE_SENTINEL} ]; then \ if [ -d {WORKSPACE_MOUNT_PATH} ]; then \ + own=\"--owner={sandbox_uid} --group={sandbox_gid} --numeric-owner\"; \ + ext=\"--numeric-owner\"; \ + tar $own -cf /dev/null -T /dev/null 2>/dev/null || \ + {{ own=\"\"; ext=\"--no-same-owner\"; }}; \ tmp=$(mktemp) && rm -f \"$tmp\" && \ - (cd {WORKSPACE_MOUNT_PATH} && find . -mindepth 1 -maxdepth 1 -exec tar -cf \"$tmp\" {{}} +) && \ + (cd {WORKSPACE_MOUNT_PATH} && find . -mindepth 1 -maxdepth 1 -exec tar $own -cf \"$tmp\" {{}} +) && \ if [ -f \"$tmp\" ]; then \ - tar -C {WORKSPACE_INIT_MOUNT_PATH} --no-same-owner --no-same-permissions --touch -xf \"$tmp\" && \ + tar -C {WORKSPACE_INIT_MOUNT_PATH} $ext --no-same-permissions --touch -xf \"$tmp\" && \ rm -f \"$tmp\"; \ fi; \ fi && \ @@ -4130,6 +4162,7 @@ fn sandbox_template_to_k8s_with_validated_config( &mut result, image, params.image_pull_policy, + params.sandbox_uid, params.sandbox_gid, ); } @@ -7355,6 +7388,7 @@ mod tests { &mut pod_template, "openshell/sandbox:latest", "IfNotPresent", + 1000, // sandbox_uid 1000, // sandbox_gid ); @@ -7415,6 +7449,7 @@ mod tests { "my-custom-image:v2", "IfNotPresent", 1000, + 1000, ); let init_image = pod_template["spec"]["initContainers"][0]["image"] @@ -7437,7 +7472,7 @@ mod tests { } }); - apply_workspace_persistence(&mut pod_template, "img:latest", "Always", 1000); + apply_workspace_persistence(&mut pod_template, "img:latest", "Always", 1000, 1000); let cmd = pod_template["spec"]["initContainers"][0]["command"] .as_array() @@ -7456,10 +7491,134 @@ mod tests { "init script must archive sandbox contents without the mount root entry" ); assert!( - script.contains("--no-same-owner") - && script.contains("--no-same-permissions") - && script.contains("--touch"), - "init script must avoid restoring metadata onto the PVC root" + script.contains("--no-same-permissions") && script.contains("--touch"), + "init script must not restore modes or timestamps onto the PVC" + ); + } + + /// Regression: seeding the PVC as root with `--no-same-owner` left every + /// seeded path owned by uid 0, so a mode-0700 home directory from the + /// image (`~/.config`, `~/.cache`) was unreachable for the workload. + /// Ownership must be rewritten to the resolved sandbox identity instead. + #[test] + fn workspace_init_seeds_content_owned_by_the_sandbox_identity() { + let mut pod_template = serde_json::json!({ + "spec": { + "containers": [{ + "name": "agent", + "image": "img:latest" + }] + } + }); + + apply_workspace_persistence(&mut pod_template, "img:latest", "Always", 1234, 5678); + + let script = pod_template["spec"]["initContainers"][0]["command"][2] + .as_str() + .expect("init script should be the third command element") + .to_string(); + + assert!( + script.contains("--owner=1234") && script.contains("--group=5678"), + "init script must rewrite seeded ownership to the resolved sandbox identity, got: {script}" + ); + assert!( + script.contains("--numeric-owner"), + "ownership rewrite must be numeric so it does not depend on image account files" + ); + assert!( + script.contains("ext=\"--numeric-owner\""), + "extraction must restore the rewritten ownership, not discard it" + ); + assert!( + !script.contains(&format!( + "tar -C {WORKSPACE_INIT_MOUNT_PATH} --no-same-owner" + )), + "extraction must not discard ownership unconditionally — that is the \ + regression being fixed" + ); + assert!( + !script.contains("chown"), + "seeding must not chown the workspace tree — that pattern broke on \ + read-only submounts (#2294)" + ); + } + + /// `--owner`/`--group` are GNU extensions and this init container runs the + /// sandbox image itself, so a minimal base such as Alpine seeds with + /// `BusyBox` tar. Rejecting those options fails the init container and the + /// pod never starts, so the script probes tar once and degrades to the + /// previous flags instead of hard-failing. + #[test] + fn workspace_init_falls_back_when_tar_lacks_ownership_extensions() { + let mut pod_template = serde_json::json!({ + "spec": { + "containers": [{ + "name": "agent", + "image": "img:latest" + }] + } + }); + + apply_workspace_persistence(&mut pod_template, "img:latest", "Always", 1234, 5678); + + let script = pod_template["spec"]["initContainers"][0]["command"][2] + .as_str() + .expect("init script should be the third command element") + .to_string(); + + assert!( + script.contains("tar $own -cf /dev/null -T /dev/null 2>/dev/null"), + "the script must probe tar for the ownership extensions, got: {script}" + ); + assert!( + script.contains("own=\"\"; ext=\"--no-same-owner\""), + "the probe must fall back to the previous flags, got: {script}" + ); + assert!( + script.contains("tar $own -cf \"$tmp\"") + && script.contains("$ext --no-same-permissions --touch -xf"), + "both tar invocations must use the probed flags, got: {script}" + ); + } + + /// The seeded identity must track the driver-resolved UID/GID rather than + /// a hardcoded 1000, so images built for a different UID range (or an + /// OpenShift-allocated one) still produce a usable workspace. + #[test] + fn workspace_init_ownership_tracks_resolved_identity() { + let params = SandboxPodParams { + sandbox_uid: 1_000_660_000, + sandbox_gid: 1_000_660_000, + ..SandboxPodParams::default() + }; + + let pod_template = sandbox_template_to_k8s( + &SandboxTemplate { + image: "img:latest".to_string(), + ..SandboxTemplate::default() + }, + false, + &std::collections::HashMap::new(), + true, + ¶ms, + ); + + let init_containers = pod_template["spec"]["initContainers"] + .as_array() + .expect("initContainers should exist"); + let workspace_init = init_containers + .iter() + .find(|c| c["name"] == WORKSPACE_INIT_CONTAINER_NAME) + .expect("workspace init container should exist"); + + let script = workspace_init["command"][2] + .as_str() + .expect("init script should be the third command element"); + + assert!( + script.contains("--owner=1000660000") && script.contains("--group=1000660000"), + "seeded ownership must follow the resolved sandbox identity, got: {script}" ); } diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 987e66b0d9..9d1eed4f0e 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -590,7 +590,8 @@ You can override autodetection with explicit `sandbox_uid` / `sandbox_gid` confi The resolved UID/GID appear in: - Supervisor container environment variables (`OPENSHELL_SANDBOX_UID`, `OPENSHELL_SANDBOX_GID`) for direct kernel-level privilege dropping without `/etc/passwd` lookups. -- PVC init container `securityContext.runAsUser/runAsGroup/fsGroup` for workspace ownership operations. +- Agent container `securityContext.runAsUser/runAsGroup`, and the pod-level `securityContext.fsGroup` that kubelet applies to the workspace volume at mount time. +- Ownership of the content the `workspace-init` container seeds into a fresh workspace PVC. That container runs as root so it can read every path the image ships, and writes the seeded tree as the resolved UID/GID. Images whose `tar` lacks the GNU ownership extensions — a minimal base such as Alpine, which provides BusyBox `tar` — seed as root instead, so a mode-`0700` directory from such an image stays unreadable to the workload. ### VM Driver