feat(gpu): derive sandbox requirements from CDI specs#2265
Conversation
|
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. |
61d751d to
c8ebad0
Compare
|
🌿 Preview your docs: https://nvidia-preview-pr-2265.docs.buildwithfern.com/openshell |
d7f06ff to
5cd240a
Compare
|
Label |
|
Label |
5cd240a to
c8b3044
Compare
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
c8b3044 to
c040b8d
Compare
| openshell_core::sandbox_env::TELEMETRY_ENABLED.to_string(), | ||
| openshell_core::telemetry::enabled_env_value().to_string(), | ||
| ); | ||
| if include_cdi_context { |
There was a problem hiding this comment.
[P1] Prevent container images from supplying their own CDI context. This only inserts OPENSHELL_CDI_CONTEXT when GPUs are selected, so a non-GPU container does not override an OPENSHELL_CDI_CONTEXT value inherited from the selected image. A crafted image can embed its own context and CDI spec; the supervisor accepts any nonempty context path, converts CDI deviceNodes into read-write Landlock paths without verifying that they are device nodes, and applies additionalGids. That lets an image expand its effective policy even though no trusted CDI context was uploaded. Please explicitly override this variable with an empty value for non-CDI containers. As defense in depth, also restrict the supervisor to the fixed context path/spec mount root and verify device-node types before granting read-write access.
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR is project-valid because it implements roadmap issue #1606 for Docker GPU/CDI sandbox policy derivation, includes user-facing documentation, and has the required E2E/GPU E2E labels.
Head SHA: c040b8dd5d7adcb126d1c5199e6203ba5dfdc93b
Thanks @drew, I checked the CDI context trust-boundary concern you raised and confirmed it is still a blocking issue on this head. The independent review also found related supervisor-side hardening gaps and a Docker token cleanup regression.
Review findings:
- Blocking findings remain on CDI context trust, CDI device-node validation, and failed-provisioning token cleanup.
Docs: Fern docs were updated under docs/; no navigation change appears necessary for these existing pages.
Checks: Required checks are currently green, including DCO, Branch Checks, Helm Lint, Core E2E, and GPU E2E.
E2E: test:e2e and test:e2e-gpu are applied and passing.
Next state: gator:in-review
| openshell_core::sandbox_env::TELEMETRY_ENABLED.to_string(), | ||
| openshell_core::telemetry::enabled_env_value().to_string(), | ||
| ); | ||
| if include_cdi_context { |
There was a problem hiding this comment.
gator-agent
Critical: OPENSHELL_CDI_CONTEXT is only inserted for GPU/CDI requests. Docker inherits image ENV, so a non-GPU image can provide its own CDI context path and cause the supervisor to apply image-controlled CDI paths and supplemental groups. Please set this variable for every container, using the fixed context path only when CDI is expected and an empty value otherwise.
| fn resolve_cdi_requirements_from_env( | ||
| writable_file_allowlist: &HashSet<String>, | ||
| ) -> Result<Option<openshell_core::cdi::CdiDerivedRequirements>> { | ||
| let Ok(context_path) = std::env::var(openshell_core::sandbox_env::CDI_CONTEXT) else { |
There was a problem hiding this comment.
gator-agent
Critical: The supervisor currently accepts any non-empty OPENSHELL_CDI_CONTEXT path. Even with Docker clearing the env for non-CDI containers, this should fail closed unless the path is the driver-owned fixed context path, and context spec dirs should be constrained to the supervisor CDI spec root.
| F: Fn(&str) -> Option<CdiPathKind>, | ||
| { | ||
| for device_node in &edits.device_nodes { | ||
| let path = normalize_policy_path(&device_node.path)?; |
There was a problem hiding this comment.
gator-agent
Critical: CDI deviceNodes are normalized but not verified as device nodes before they become read-write Landlock paths. A malformed or malicious spec could list a regular file and expand write policy. Please verify char/block device type before adding a CDI device node path.
| } | ||
| DockerProvisioningFailure::new("ContainerCreateFailed", status.message()) | ||
| })?; | ||
| write_sandbox_token_file(sandbox, &self.config) |
There was a problem hiding this comment.
gator-agent
Warning: Token file creation now happens before container creation, but the later create/upload/start failure paths no longer clean it up. Please restore cleanup_sandbox_token_file on each post-write provisioning failure path so failed sandboxes do not leave credential artifacts behind.
Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR is project-valid because it implements roadmap issue #1606 for Docker GPU/CDI sandbox policy derivation, includes architecture and Fern documentation updates, and has the required test:e2e and test:e2e-gpu labels.
Head SHA: cf6a99519f85611da0558468ce42f84c6940976d
Thanks @drew, I re-checked the CDI context trust-boundary concern you raised against the latest head. The Docker env cleanup and token cleanup paths are improved, but the independent review found the supervisor still needs a stronger driver-attestation boundary before CDI-derived policy is trusted.
Review findings:
- Blocking finding remains on CDI context trust/attestation.
- Warning remains on explicit CDI device ID namespace validation.
Docs: Fern docs were updated under docs/; no navigation change appears necessary for these existing pages.
Checks: Current head has DCO and Helm Lint passing, with Branch Checks, E2E, and GPU E2E still pending/queued.
E2E: test:e2e and test:e2e-gpu are applied; results are still pending for this head.
Next state: gator:in-review
| fn resolve_cdi_requirements_from_env( | ||
| writable_file_allowlist: &HashSet<String>, | ||
| ) -> Result<Option<openshell_core::cdi::CdiDerivedRequirements>> { | ||
| let Ok(context_path) = std::env::var(openshell_core::sandbox_env::CDI_CONTEXT) else { |
There was a problem hiding this comment.
gator-agent
Critical: resolve_cdi_requirements_from_env still treats any OPENSHELL_CDI_CONTEXT in the supervisor environment as authoritative. Docker now clears/sets it, but this supervisor code is shared with other runtimes and local modes, so an image-provided env plus image-provided files under /run/openshell/supervisor/... can trigger CDI enrichment, including /proc read-write and supplemental groups. Please require driver-attested CDI context: reject empty selected/spec dirs, verify each spec dir is an actual driver-provided mount under CDI_SPEC_DIR_BASE, ensure non-CDI drivers clear the env, and add a regression test for image-supplied OPENSHELL_CDI_CONTEXT.
| let cdi_context = self | ||
| .config | ||
| .gpu | ||
| .cdi_context(gpu_devices.as_deref()) |
There was a problem hiding this comment.
gator-agent
Warning: The selected IDs passed into the CDI context can come from explicit driver_config.cdi_devices, and current validation only checks count/duplicates, not that each ID is in the NVIDIA GPU CDI namespace. Since this PR now derives device, mount, and group policy from those specs, arbitrary CDI vendors can broaden sandbox permissions behind a GPU request. Please require nvidia.com/gpu= for explicit IDs, matching the default inventory filter, and add a negative unit test.
Summary
Derive sandbox filesystem and process requirements from resolved CDI GPU specs so Docker GPU sandboxes can carry the effective device, mount, and group policy into the supervisor.
Related Issue
Related: #1606
Changes
Testing
mise run pre-commitpassesChecklist