From 4f521debe92d5005bc75500005406cd38da18606 Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Fri, 17 Jul 2026 09:50:40 +0800 Subject: [PATCH] fix(supervisor): opt-in reconcile of drifted sandbox state ownership Rootless Podman/Docker sandboxes attach no pinned user-namespace mapping and bind-mount host state as plain rbinds, so a persisted state tree can come back owned by an unrelated host user after a host reboot shifts the rootless subuid base -- leaving the sandbox unable to read its own state (readonly sqlite, EACCES). prepare_filesystem only chowns newly-created read_write paths, so an existing tree is never repaired. Add an opt-in OPENSHELL_RECONCILE_SANDBOX_OWNERSHIP toggle (default off). When enabled, prepare_filesystem recursively re-owns an existing sandbox-writable path whose owner no longer matches the configured identity. A correctly-owned tree is skipped after a single stat, so healthy launches are a no-op and the existing create-only chown contract -- and its test -- are preserved. Symlinks are never followed. Only policy-declared read_write paths are considered. Contained stop-gap; the durable fix (pinning the userns mapping / idmapped mounts) is tracked in #2336. Signed-off-by: Tony Luo --- crates/openshell-core/src/sandbox_env.rs | 10 ++ .../src/process.rs | 155 ++++++++++++++++++ 2 files changed, 165 insertions(+) diff --git a/crates/openshell-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index f066580636..08833791c1 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -114,3 +114,13 @@ pub const SANDBOX_UID: &str = "OPENSHELL_SANDBOX_UID"; /// Used alongside UID for PVC init container `chown` operations and when the /// supervisor drops privileges to a group other than the UID's primary group. pub const SANDBOX_GID: &str = "OPENSHELL_SANDBOX_GID"; + +/// Opt-in toggle to reconcile ownership of existing sandbox-writable paths. +/// +/// The default (unset) preserves the create-only chown contract: an existing +/// `read_write` path is never re-owned. When set to `"1"` or `"true"`, the +/// supervisor recursively repairs a persisted state tree whose host-side +/// ownership no longer matches the configured sandbox identity — for example +/// after a host reboot shifted the rootless user-namespace subuid base and left +/// the sandbox unable to read its own state. See NVIDIA/OpenShell#2336. +pub const RECONCILE_SANDBOX_OWNERSHIP: &str = "OPENSHELL_RECONCILE_SANDBOX_OWNERSHIP"; diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 72effa98f8..5aaaf2faf5 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -1202,6 +1202,66 @@ fn chown_sandbox_home(root: &Path, uid: Option, gid: Option) -> Result Ok(()) } +/// Whether an existing sandbox-writable path is already owned by the configured +/// identity. +/// +/// Symlinks are treated as "matching" so the reconcile pass never chowns +/// through them (mirrors the symlink refusal in [`chown_sandbox_home`]). A +/// `None` uid/gid component is ignored, matching how `chown` leaves that +/// component untouched. +#[cfg(unix)] +fn path_owner_matches(path: &Path, uid: Option, gid: Option) -> Result { + use std::os::unix::fs::MetadataExt; + + let meta = std::fs::symlink_metadata(path).into_diagnostic()?; + if meta.file_type().is_symlink() { + return Ok(true); + } + let uid_ok = uid.is_none_or(|u| meta.uid() == u.as_raw()); + let gid_ok = gid.is_none_or(|g| meta.gid() == g.as_raw()); + Ok(uid_ok && gid_ok) +} + +/// Repair ownership of existing sandbox-writable paths that have drifted away +/// from the configured identity. +/// +/// This is the opt-in counterpart to the create-only chown in +/// [`prepare_filesystem`]: a persisted `read_write` tree (e.g. a bind-mounted +/// state dir) can come back owned by an unrelated host user after a reboot +/// shifts the rootless user-namespace subuid base, leaving the sandbox unable +/// to read its own state. A path already owned by the target identity is left +/// untouched (a single `stat`), so this is a no-op on healthy launches. Only +/// paths the policy already declares sandbox-writable are considered. See +/// NVIDIA/OpenShell#2336. +#[cfg(unix)] +fn reconcile_read_write_ownership( + paths: &[PathBuf], + uid: Option, + gid: Option, +) -> Result<()> { + for path in paths { + if !path.exists() || path_owner_matches(path, uid, gid)? { + continue; + } + info!( + path = %path.display(), + ?uid, + ?gid, + "Reconciling stale ownership on existing sandbox-writable path" + ); + chown_sandbox_home(path, uid, gid)?; + } + Ok(()) +} + +/// Whether the opt-in ownership reconcile pass is enabled via +/// [`RECONCILE_SANDBOX_OWNERSHIP`](openshell_core::sandbox_env::RECONCILE_SANDBOX_OWNERSHIP). +#[cfg(unix)] +fn sandbox_ownership_reconcile_enabled() -> bool { + std::env::var_os(openshell_core::sandbox_env::RECONCILE_SANDBOX_OWNERSHIP) + .is_some_and(|value| value == "1" || value == "true") +} + /// Prepare filesystem for the sandboxed process. /// /// Creates `read_write` directories if they don't exist and sets ownership @@ -1260,6 +1320,13 @@ pub fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> { } } + // Opt-in: repair existing sandbox-writable trees whose ownership drifted + // (e.g. after a host reboot shifted the rootless userns subuid base). Off by + // default, so the create-only chown contract above is unchanged. + if sandbox_ownership_reconcile_enabled() { + reconcile_read_write_ownership(&policy.filesystem.read_write, uid, gid)?; + } + // When a driver injects a custom UID/GID via environment variables, the // /sandbox home directory may already exist with image-default ownership // (e.g. UID 1000) that differs from the driver-assigned identity. @@ -2032,6 +2099,94 @@ mod tests { assert_eq!(after.gid(), before.gid()); } + #[cfg(unix)] + #[test] + #[allow(clippy::similar_names)] + fn path_owner_matches_detects_current_and_mismatched_owner() { + use std::os::unix::fs::MetadataExt; + + let dir = tempfile::tempdir().unwrap(); + let file = dir.path().join("state"); + std::fs::write(&file, "x").unwrap(); + let meta = std::fs::metadata(&file).unwrap(); + let cur_uid = Uid::from_raw(meta.uid()); + let cur_gid = Gid::from_raw(meta.gid()); + + assert!(path_owner_matches(&file, Some(cur_uid), Some(cur_gid)).unwrap()); + let other_uid = Uid::from_raw(meta.uid().wrapping_add(1)); + assert!(!path_owner_matches(&file, Some(other_uid), Some(cur_gid)).unwrap()); + // A `None` component is ignored, matching `chown` semantics. + assert!(path_owner_matches(&file, None, Some(cur_gid)).unwrap()); + } + + #[cfg(unix)] + #[test] + fn path_owner_matches_treats_symlink_as_matching() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("real"); + let link = dir.path().join("link"); + std::fs::create_dir(&target).unwrap(); + symlink(&target, &link).unwrap(); + + // Symlinks are never chowned through, so they always report "matching". + assert!(path_owner_matches(&link, Some(Uid::from_raw(0)), Some(Gid::from_raw(0))).unwrap()); + } + + #[cfg(unix)] + #[test] + #[allow(clippy::similar_names)] + fn reconcile_read_write_ownership_is_noop_when_already_owned() { + use std::os::unix::fs::MetadataExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state"); + std::fs::create_dir(&path).unwrap(); + std::fs::write(path.join("f"), "x").unwrap(); + let before = std::fs::metadata(&path).unwrap(); + let uid = Uid::from_raw(before.uid()); + let gid = Gid::from_raw(before.gid()); + + // Already the target identity: no chown attempted, so this succeeds even + // without privilege and leaves ownership unchanged. + reconcile_read_write_ownership(std::slice::from_ref(&path), Some(uid), Some(gid)).unwrap(); + + let after = std::fs::metadata(&path).unwrap(); + assert_eq!(after.uid(), before.uid()); + assert_eq!(after.gid(), before.gid()); + } + + #[cfg(unix)] + #[test] + fn reconcile_read_write_ownership_skips_missing_paths() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("does-not-exist"); + reconcile_read_write_ownership(&[missing], Some(Uid::from_raw(0)), Some(Gid::from_raw(0))) + .unwrap(); + } + + #[cfg(unix)] + #[test] + fn reconcile_read_write_ownership_triggers_on_mismatch() { + // As non-root a genuine ownership mismatch forces a chown that EPERMs, + // proving the reconcile pass fires — the inverse of the create-only skip + // contract exercised above. + use std::os::unix::fs::MetadataExt; + + if nix::unistd::geteuid().is_root() { + return; + } + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state"); + std::fs::create_dir(&path).unwrap(); + let gid = Gid::from_raw(std::fs::metadata(&path).unwrap().gid()); + + assert!( + reconcile_read_write_ownership(&[path], Some(Uid::from_raw(0)), Some(gid)).is_err() + ); + } + #[cfg(unix)] #[test] #[allow(clippy::similar_names)]