Skip to content

feat(driver-podman): add per-sandbox user namespace mode#2141

Open
bergmannf wants to merge 1 commit into
NVIDIA:mainfrom
bergmannf:feat/podman-userns-param
Open

feat(driver-podman): add per-sandbox user namespace mode#2141
bergmannf wants to merge 1 commit into
NVIDIA:mainfrom
bergmannf:feat/podman-userns-param

Conversation

@bergmannf

@bergmannf bergmannf commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Add support for selecting a per-sandbox Podman user namespace mode through driver config.

This feature is required if SELinux is active, but the user ID inside the sandbox container is not the same as the user ID of the user creating it. In that case reading a SELinux shared mount is possible, but writing is not, unless the keep-id map is supplied with the correct values inside the sandbox (e.g. in the default image it must use: '{"podman":{"userns_mode": "keep-id:uid=998,gid=998"}})

Related Issue

None

Changes

  • Added userns_mode to Podman sandbox driver config
  • Parsed --userns-style values into the libpod namespace spec
  • Added unit tests for unset, whitespace, and colon-split parsing
  • Updated Podman driver and gateway config docs

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)

Add a `userns_mode` field to `PodmanSandboxDriverConfig` so each
sandbox can independently select its user namespace mode via
`--driver-config-json`.

Accepts the same values as `podman run --userns`, e.g. "keep-id",
"keep-id:uid=200,gid=210", "auto", "nomap", or "host". Empty (default)
leaves Podman's default behavior unchanged.

The `build_userns` helper reads from the per-sandbox driver config
and parses the string into a libpod `Namespace` object, splitting on
the first colon: the left side becomes `nsmode` and the right side
becomes `value`. When unset or whitespace-only, the `userns` field is
omitted from the container spec JSON so Podman uses its default
(host namespace).

Includes unit tests covering all parsing paths, whitespace trimming,
and coexistence with mounts in driver config. Updates the driver
README with usage examples.

Signed-off-by: Florian Bergmann <bergmann.f@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 4, 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.

@bergmannf

Copy link
Copy Markdown
Contributor Author

Should not be needed once #1959 is implemented.

@TaylorMutch TaylorMutch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the contribution. I found two points I would like feedback on before approval:

  1. [P1] Published docs place userns_mode in an unsupported gateway configuration block. docs/reference/gateway-config.mdx:283-286 and docs/reference/sandbox-compute-drivers.mdx:182-186 tell users to set userns_mode under [openshell.drivers.podman]. The implementation adds it only to per-sandbox PodmanSandboxDriverConfig, while PodmanComputeConfig uses deny_unknown_fields. Uncommenting the documented TOML example should therefore fail gateway configuration parsing. Should these sections instead document the per-sandbox --driver-config-json form shown in the README?

  2. [P2] The UID mapping explanation appears reversed. crates/openshell-driver-podman/src/container.rs:205-208 says keep-id:uid=200,gid=210 maps container UID 0 to host UID 200. Podman documents this as mapping the host user running Podman to UID 200/GID 210 inside the container. The serialization code looks correct, but the comment could cause future mount-permission or security misunderstandings.

Validation performed locally: cargo test -p openshell-driver-podman passed all 115 tests, including all 10 new userns-focused tests.

@russellb

Copy link
Copy Markdown
Contributor

Should not be needed once #1959 is implemented.

If this is the ideal fix, maybe we should just focus on getting that done? What do you think?

@russellb

Copy link
Copy Markdown
Contributor

Note my last comment, but if we proceed with this, here is an agent-assisted code review.

Two things should block merge:

1. The reference docs describe userns_mode as a gateway-level [openshell.drivers.podman] field, but it's only implemented per-sandbox (--driver-config-json). PodmanComputeConfig has no such field and is #[serde(deny_unknown_fields)], so an operator who uncomments the userns_mode line in gateway-config.mdx gets a gateway that fails to start (unknown field 'userns_mode'). The README is correct; the two docs/reference/*.mdx edits need to drop the gateway-level framing and document it as per-sandbox only.

2. Tenant-supplied userns_mode flows into the libpod spec with no gate and no validation. A sandbox can set userns_mode = "host" to disable user-namespace isolation, or ns:/proc/<pid>/ns/user / container:<id> to join another namespace. Bind mounts already model the right pattern — gated by enable_bind_mounts in the gateway config (container.rs:642-656). Suggest gating this behind a gateway opt-in and validating nsmode against an allow-list (keep-id, auto, nomap, private), rejecting host/ns:/container: unless explicitly enabled.

Smaller items:

  • build_userns on ":foo" yields an empty nsmode that's still sent to libpod — reject it explicitly.
  • Only outer whitespace is trimmed; "keep-id : uid=200" leaves a trailing space in nsmode. Trim each half.
  • Tests cover the per-sandbox path but not host mode or the empty-nsmode/inner-whitespace edges.

@russellb

Copy link
Copy Markdown
Contributor

Should not be needed once #1959 is implemented.

If this is the ideal fix, maybe we should just focus on getting that done? What do you think?

actually, #1959 is already done.

@russellb

Copy link
Copy Markdown
Contributor

Agent-assisted review.

Following up now that #1959 has actually landed (#1973).

First: please confirm this is still needed. You noted earlier that this "should not be needed once #1959 is implemented" — but I don't think what landed in #1959 covers this case, so I'd like to verify before we invest more in it:

Those are different layers. So my read is that the merged #1959 work does not resolve the SELinux/keep-id write problem here. Could you test against the landed behavior and confirm?

  • If it does resolve it → let's close this as superseded and note which knob replaces it.
  • If it doesn't (my expectation) → the feature is still wanted, but I'd like to reshape it (below) rather than merge as-is.

If we proceed, the two blockers from the earlier review still stand (both re-confirmed against current main):

  1. Docs describe userns_mode as a gateway-level [openshell.drivers.podman] field, but it's only implemented per-sandbox. PodmanComputeConfig has no such field and is #[serde(deny_unknown_fields)], so an operator who uncomments the documented line gets a gateway that fails to start.
  2. Tenant-supplied userns_mode flows into the libpod spec with no gate and no validation — a sandbox can set host/ns:/container: to weaken or escape user-namespace isolation. Bind mounts already model the right pattern (gated by enable_bind_mounts).

Suggested reshape: make this an operator (gateway-level) control resolved server-side, ideally derived from the same sandbox_uid/sandbox_gid identity #1959 just standardized, rather than a free-form string the tenant passes in. That closes the security gap and makes the gateway-level docs framing correct at the same time.

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