feat(drivers): inject local sandbox identity#2335
Conversation
Make Docker and Podman select a validated numeric agent identity and inject it into the supervisor, allowing userless images to run without a baked-in sandbox account.\n\nCloses #2331 Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
Thank you for your interest in contributing to OpenShell, @matthewgrossman. This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer. To get vouched:
See CONTRIBUTING.md for details. |
E2E Test AttestationThe local Docker E2E test passed against the committed implementation. Podman is not installed on this development host; the PR adds the equivalent test to the repository's rootless Podman E2E lane.
Test SummaryTests Executed
The fixture asserts that the image contains neither a named |
|
Thank you for your interest in contributing to OpenShell, @matthewgrossman. This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer. To get vouched:
See CONTRIBUTING.md for details. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-2335.docs.buildwithfern.com/openshell |
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
| Ok(()) | ||
| } | ||
|
|
||
| fn resolve_sandbox_identity(config: &DockerComputeConfig) -> CoreResult<(u32, u32)> { |
There was a problem hiding this comment.
The effective duplication of this across both drivers and the fact that it returns a CoreResult<(u32, u32)>, which is used by the caller as sandbox_identity.0 and sandbox_identity.1 made me want an alternate implementation that could be shared and use a proper type.
This is what gpt-5.6-sol medium came up with:
Details
Since Docker and Podman have identical defaulting, range validation, tuple return values, and nearly identical tests, a shared implementation is preferable. The most natural current home is openshell-policy, because it already owns: MIN_SANDBOX_UID
MAX_SANDBOX_UID
Putting it in openshell-core::driver_utils would create a dependency problem: openshell-policy already depends on openshell-core, so core cannot depend back on policy.
I’d introduce a driver-neutral validated type:
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct SandboxIdentity {
pub uid: u32,
pub gid: u32,
}
impl SandboxIdentity {
pub fn new(
uid: u32,
gid: Option<u32>,
) -> Result<Self, SandboxIdentityError> {
let identity = Self {
uid,
gid: gid.unwrap_or(uid),
};
identity.validate()?;
Ok(identity)
}
fn validate(&self) -> Result<(), SandboxIdentityError> {
for (field, value) in [
("sandbox_uid", self.uid),
("sandbox_gid", self.gid),
] {
if !(MIN_SANDBOX_UID..=MAX_SANDBOX_UID).contains(&value) {
return Err(SandboxIdentityError {
field,
value,
});
}
}
Ok(())
}
}With a shared error that does not mention Docker or Podman:
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct SandboxIdentityError {
field: &'static str,
value: u32,
}
impl std::fmt::Display for SandboxIdentityError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(
f,
"{} must be in [{MIN_SANDBOX_UID}, {MAX_SANDBOX_UID}], got {}",
self.field,
self.value,
)
}
}
impl std::error::Error for SandboxIdentityError {}Docker maps that into its configuration error:
let sandbox_identity = openshell_policy::SandboxIdentity::new(
docker_config.sandbox_uid,
docker_config.sandbox_gid,
)
.map_err(|error| Error::config(format!("docker {error}")))?;Then:
sandbox_uid: sandbox_identity.uid,
sandbox_gid: sandbox_identity.gid,Podman can keep a thin configuration method if that fits its current organization:
pub fn sandbox_identity(
&self,
) -> Result<openshell_policy::SandboxIdentity, PodmanApiError> {
openshell_policy::SandboxIdentity::new(
self.sandbox_uid,
self.sandbox_gid,
)
.map_err(|error| PodmanApiError::InvalidInput(error.to_string()))
}And its environment construction becomes:
let sandbox_identity = config
.sandbox_identity()
.expect("Podman configuration is validated before build_env");
env.insert(
openshell_core::sandbox_env::SANDBOX_UID.into(),
sandbox_identity.uid.to_string(),
);
env.insert(
openshell_core::sandbox_env::SANDBOX_GID.into(),
sandbox_identity.gid.to_string(),
);The defaulting and boundary tests should move into openshell-policy. Docker and Podman would retain only tests proving that their configuration and error mapping use the shared type correctly.
That establishes one definition of the invariant across both drivers: a constructed SandboxIdentity always has a resolved GID and policy-valid UID/GID.
| | `apparmor=unconfined` | Avoids Docker's default profile blocking required mount operations. | | ||
| | `restart_policy = unless-stopped` | Keeps managed sandboxes resumable across daemon or gateway restarts. | | ||
| | `PidsLimit` | Enforces the sandbox PID budget at the Docker cgroup layer. Set `[openshell.drivers.docker].sandbox_pids_limit = 0` to inherit the Docker/runtime default. | | ||
| | `sandbox_uid` / `sandbox_gid` | Operator-controlled numeric identity for the agent child. The supervisor still starts as root; the UID defaults to `10001` and the GID defaults to the resolved UID. | |
There was a problem hiding this comment.
Just a nit, but the UID is never "resolved"; it's always at least the default value of 10_001. The GID will be set to the UID unless otherwise set.
| // get a clear error instead of a silent fallback to plaintext HTTP. | ||
| config.validate_tls_config()?; | ||
| config.validate_runtime_limits()?; | ||
| config.resolve_sandbox_identity()?; |
There was a problem hiding this comment.
This seems like an outlier; is this actually necessary here? It doesn't line up with the comment about TLS configuration, anyway.
| )] | ||
| sandbox_pids_limit: i64, | ||
|
|
||
| /// Numeric UID for the agent process. Defaults to 10001. |
There was a problem hiding this comment.
| /// Numeric UID for the agent process. Defaults to 10001. | |
| /// Numeric UID for the agent process. |
This is leaking a value that this crate doesn't know/own, and which may change. It defaults to openshell_driver_podman::config::DEFAULT_SANDBOX_UID, whatever that may be.
|
|
||
| # The compute driver must override image-provided identity values. | ||
| ENV OPENSHELL_SANDBOX_UID=1234 | ||
| ENV OPENSHELL_SANDBOX_GID=1234 |
There was a problem hiding this comment.
Should the GID be different to exercise that it's actually setting the GID and not just copying the UID in all cases?
| name = "custom_image" | ||
| path = "tests/custom_image.rs" | ||
| required-features = ["e2e-docker"] | ||
| required-features = ["e2e-local-container-driver"] |
There was a problem hiding this comment.
What's the reason for this change? The only file that has this feature is driver_config_volume.rs, which isn't modified in this PR.
|
Additional review feedback from High: numeric privilege drop retains supplementary groupsThe numeric-UID path deliberately skips initgroups(), but it never clears inherited supplementary groups before setgid() and setuid() (crates/openshell-supervisor-process/src/process.rs:1447). setgid() changes only the primary/effective GID; it does not clear supplementary groups inherited from the root supervisor. The agent could therefore retain supplementary group 0 or another supervisor group after dropping to UID/GID 10001. This PR makes numeric identity the default Docker and Podman path, so it materially expands exposure to that behavior. I would clear supplementary groups before dropping privileges: if user_name_is_numeric {
nix::unistd::setgroups(&[]).into_diagnostic()?;
} else if let Some(ref user) = user {
nix::unistd::initgroups(/* ... */)?;
}The E2E should run id -G and assert that group 0 is absent. Currently it checks only id -u and id -g, which cannot detect retained supplementary groups (e2e/rust/tests/custom_image.rs:46). Medium: the supervisor change also affects Kubernetes and VMThe documentation and new E2E coverage describe this primarily as Docker/Podman behavior, but update_sandbox_passwd_entries() and recursive home ownership changes are in shared supervisor startup. Kubernetes and VM already inject OPENSHELL_SANDBOX_UID, so they will now also:
The PR explicitly omits cross-driver E2E coverage, but these are real behavioral changes for those drivers. I would either add focused Kubernetes/VM regression coverage or scope the new mutation so those existing paths are demonstrably unchanged. Medium: recursive /sandbox chown happens on every startupWhenever OPENSHELL_SANDBOX_UID exists, the supervisor recursively chowns the entire workspace (crates/openshell-supervisor-process/src/process.rs:1345). After this PR, that condition is always true for Docker and Podman, as well as existing Kubernetes/VM paths. For a large persisted workspace this could make every sandbox restart expensive. It also overwrites intentionally distinct ownership anywhere beneath /sandbox. I would consider:
Test-contract improvementThe userless E2E currently accepts any two output lines equal to 10001 (e2e/rust/tests/custom_image.rs:22). A single labeled line would be more robust: That would also force a decision about an existing inconsistency: the PR adds a sandbox passwd entry for the numeric UID, so whoami may report sandbox, while USER and LOGNAME are deliberately set to 10001. That may be acceptable, but it should be intentional and tested. |
|
thanks @krishicks for the feedback; reviewing tomorrow! |
Direction changeAfter revisiting the local-container identity model, this branch is going to pivot substantially. The current fixed Docker/Podman identity design, including the default The revised direction is:
The supervisor already resolves the legacy named Please hold further review of the current fixed-identity implementation. I will update the code, tests, documentation, and PR description to match this revised architecture before requesting review again. |
|
Closing this PR because the fixed Docker/Podman identity design is being superseded rather than incrementally revised. The replacement will start from a clean branch and make OCI image |
Summary
Make the Docker and Podman compute drivers choose and inject a validated numeric agent identity at runtime, so OpenShell-managed sandboxes no longer require a baked-in
sandboxuser or group. The supervisor presents that identity consistently to primary and SSH-launched agent processes while retaining/sandboxas the identity-independent managed workspace and home.Related Issue
Closes #2331
Follow-up: #2422 tracks reusable cross-driver userless-image conformance coverage.
Changes
10001:10001, support operator overrides, validate both values against the policy identity range, and override image/user-supplied identity environment variables.HOME,USER, andLOGNAMEconsistently when dropping to a numeric identity and centralizes its managed/sandboxhome path.USERis not authoritative for an OpenShell-managed agent.Deviations from Plan
Local Docker validation completed. Podman is not installed on this development host, so the Podman E2E target was compiled but not runtime-validated locally. It is wired into the repository's rootless Podman E2E suite, but that suite has not run on the current PR because its E2E jobs were not selected.
Cross-driver userless-image conformance requires driver-specific image provisioning and identity expectations; that broader reusable test work is tracked in #2422.
Testing
mise run pre-commitpassesTests added or updated:
custom_imagecompile.docker_sandbox_from_userless_dockerfilepasses locally.podman_sandbox_from_userless_imageis included in the rootless Podman suite but has not received runtime validation on this host or in the current PR checks.openshell-supervisor-processtests pass locally (80 passed, 1 ignored because it requiresCAP_SETGID).Checklist
Documentation updated:
architecture/compute-runtimes.mdandarchitecture/sandbox.md: runtime identity ownership, stable workspace home, and flow.