Skip to content

fix(sandbox): undo tmpfs overlay when setns fails during SPIFFE mount namespace setup#2184

Open
zanetworker wants to merge 1 commit into
NVIDIA:mainfrom
zanetworker:fix/spiffe-mount-namespace-setns-eperm
Open

fix(sandbox): undo tmpfs overlay when setns fails during SPIFFE mount namespace setup#2184
zanetworker wants to merge 1 commit into
NVIDIA:mainfrom
zanetworker:fix/spiffe-mount-namespace-setns-eperm

Conversation

@zanetworker

@zanetworker zanetworker commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • When setns(CLONE_NEWNS) fails with EPERM (common on OpenShift and hardened K8s without CAP_SYS_ADMIN), create_supervisor_identity_mount_namespace leaves an empty tmpfs mounted over the SPIFFE workload API socket directory. Since PID 1 is stuck in the new mount namespace, the socket becomes invisible to all processes in the container, even though the CSI volume is healthy.
  • This fix calls umount2(MNT_DETACH) in the setns() error path to remove the tmpfs overlay before returning, keeping the SPIFFE socket accessible.

Related Issue

N/A (discovered during SPIFFE + token grant integration testing on OpenShift)

Changes

  • crates/openshell-supervisor-process/src/process.rs:
    • create_supervisor_identity_mount_namespace: on setns() failure, umount2(target, MNT_DETACH) before returning error

Testing

  • Built patched supervisor binary (static musl, cross-compiled from macOS via cargo-zigbuild)
  • Deployed as custom supervisor image on OpenShift 4.x cluster with SPIRE CSI driver
  • Confirmed supervisor logs show: "undid tmpfs overlay so SPIFFE socket remains accessible"
  • Confirmed /spiffe-workload-api/spire-agent.sock is visible inside sandbox container
  • Confirmed sandbox proxy allows traffic to provider endpoints (401 from protected service, not 403 policy_denied)
  • All existing tests pass (cargo test --workspace)

Checklist

  • Conventional commit format
  • cargo fmt clean
  • cargo clippy clean
  • All tests pass
  • Signed off (DCO)

@copy-pr-bot

copy-pr-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@TaylorMutch

Copy link
Copy Markdown
Collaborator

/ok to test bec39d9

@lbelyaev lbelyaev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking down the setns/EPERM case — the umount2(MNT_DETACH) in the error path (R348–R354) is a real fix and stands on its own.

Two concerns, both flagged inline, and both about the same thing: the PR pairs that fix with changes that let a sandbox keep serving traffic after a core safety property has failed. R242–R246 continues without workload identity when mount-namespace setup fails; R236–R241 symlinks around the ancestor-integrity check. For a runtime whose value is that these properties hold, degrading past either silently is a worse failure than not starting.

Would it be possible to land the umount2 fix on its own — the focused version in #2012 looked like it did exactly that — and treat the fail-open behaviours as a separate discussion? Happy to help however's useful; we're running this fail-closed downstream and would rather converge on an upstream shape than carry a patch.

eprintln!(
"WARN: supervisor identity mount namespace setup failed ({err}), continuing without isolation"
);
let _ = SUPERVISOR_IDENTITY_MOUNT_NS.set(None);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this catches setup failure, logs a warning, sets the namespace to None, and continues "without isolation." — on the same path that carries the SVID the whole identity model depends on. For a governance control plane an agent that silently runs without workload identity is worse than one that fails to start. The umount2 fix in the same PR is correct and separable; the fail-open wiring is not.

Comment thread crates/openshell-sandbox/src/main.rs Outdated
Comment on lines +236 to +242
if let Ok(exe_proc) = std::env::current_exe() {
if exe_proc != *exe_argv0 && !exe_proc.exists() && exe_argv0.exists() {
if let Some(parent) = exe_proc.parent() {
let _ = std::fs::create_dir_all(parent);
}
let _ = std::os::unix::fs::symlink(exe_argv0, &exe_proc);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This symlink works around the ancestor-integrity check rather than satisfying it. That check stats /proc/self/exe on every proxied request and denies traffic when the path is absent — it's a runtime integrity guarantee on the request path, not a startup nicety. Creating a symlink so the check passes means a proxied request no longer verifies the running binary is the one that was launched. The errors are dropped (let _ =), so a partial failure leaves the check silently satisfied rather than failing closed. The stale /proc/self/exe is a genuine side effect of the mount-namespace change, but the fix should restore honest resolution — not make an integrity check pass unconditionally.

… namespace setup

On OpenShift and other container runtimes that do not grant
CAP_SYS_ADMIN, setns(CLONE_NEWNS) fails with EPERM after
create_supervisor_identity_mount_namespace has already mounted
an empty tmpfs over the SPIFFE workload API socket directory.
PID 1 remains stuck in the new mount namespace where the tmpfs
hides the socket, making it invisible to all processes.

Fix: when setns() fails, call umount2(MNT_DETACH) to remove
the tmpfs overlay before returning the error, so the SPIFFE
socket stays accessible even without namespace isolation.

Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
@zanetworker
zanetworker force-pushed the fix/spiffe-mount-namespace-setns-eperm branch from 4d0c97a to 44d7a88 Compare July 26, 2026 17:15
@zanetworker

Copy link
Copy Markdown
Contributor Author

@lbelyaev thanks for the careful review — you're right on both counts. I've restructured the PR to land only the umount2 fix:

  • Dropped the fail-open wiring in prepare_supervisor_identity_mount_namespace_from_env. Restored the original ? propagation so mount namespace setup failure is fatal, not silently degraded.
  • Dropped the symlink workaround in main.rs and the prepare_supervisor_identity_mount_namespace_from_env() call that required it. The wiring question (originally fix(supervisor): wire prepare_supervisor_identity_mount_namespace_from_env #2012) can be discussed separately from this fix.

The diff is now a single change in create_supervisor_identity_mount_namespace(): umount2(MNT_DETACH) in the setns() error path so the SPIFFE socket stays reachable when PID 1 can't restore the original mount namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants