fixup! feat(gpu): derive sandbox requirements from CDI specs

Signed-off-by: Evan Lezar <elezar@nvidia.com>

Author: elezar

crates/openshell-core/src/cdi.rs

@@ -70,7 +70,7 @@ impl CdiSpecDirectory {
 pub struct CdiDerivedRequirements {
     pub read_only_paths: Vec<String>,
     pub read_write_paths: Vec<String>,
-    pub supplemental_gids: Vec<u32>,
+    pub additional_gids: Vec<u32>,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -104,22 +104,12 @@ pub enum CdiError {
     },
     #[error("unsupported CDI context version {0}")]
     UnsupportedContextVersion(u32),
-    #[error("CDI context selected_devices must not be empty")]
-    EmptySelection,
-    #[error("CDI context spec_dirs must not be empty")]
-    EmptySpecDirs,
-    #[error("invalid CDI selected device ID '{0}'; expected '<kind>=<name>'")]
-    InvalidSelectedDevice(String),
     #[error("CDI spec dir '{path}' from source '{diagnostic_source}' is unsafe: {reason}")]
     UnsafeSpecDir {
         path: String,
         diagnostic_source: String,
         reason: &'static str,
     },
-    #[error("no CDI spec files were found in the mounted spec dirs")]
-    NoSpecFiles,
-    #[error("failed to refresh CDI cache from mounted spec dirs: {error}")]
-    CacheRefresh { error: String },
     #[error("selected CDI device '{0}' was not found in mounted CDI specs")]
     MissingDevice(String),
     #[error("failed to merge CDI edits for '{device}': {error}")]
@@ -140,8 +130,6 @@ pub enum CdiError {
     WritableMountNotFile { path: String, kind: String },
     #[error("CDI mount '{path}' has conflicting ro/rw options")]
     ConflictingMountOptions { path: String },
-    #[error("CDI additionalGids must not include root GID 0")]
-    RootAdditionalGid,
 }
 
 // Temporary view over upstream-resolved CDI edits. The Rust CDI crate currently
@@ -180,10 +168,22 @@ enum CdiAccess {
 #[derive(Debug, Default)]
 struct RequirementAccumulator {
     paths: BTreeMap<String, CdiAccess>,
-    gids: BTreeSet<u32>,
+    writable_mount_paths: BTreeSet<String>,
+    additional_gids: BTreeSet<u32>,
 }
 
 impl RequirementAccumulator {
+    fn add_device_node(&mut self, path: String) -> Result<(), CdiError> {
+        self.add_path(path, CdiAccess::ReadWrite)
+    }
+
+    fn add_mount(&mut self, path: String, access: CdiAccess) -> Result<(), CdiError> {
+        if access == CdiAccess::ReadWrite {
+            self.writable_mount_paths.insert(path.clone());
+        }
+        self.add_path(path, access)
+    }
+
     fn add_path(&mut self, path: String, access: CdiAccess) -> Result<(), CdiError> {
         match self.paths.get(&path).copied() {
             Some(existing) if existing != access => Err(CdiError::ConflictingAccess { path }),
@@ -195,11 +195,34 @@ impl RequirementAccumulator {
         }
     }
 
-    fn add_gid(&mut self, gid: u32) -> Result<(), CdiError> {
+    fn add_gid(&mut self, gid: u32) {
         if gid == 0 {
-            return Err(CdiError::RootAdditionalGid);
+            tracing::debug!("Skipping CDI additionalGids entry for root GID 0");
+            return;
+        }
+        self.additional_gids.insert(gid);
+    }
+
+    fn validate_writable_mounts<F>(
+        &self,
+        normalized_writable_file_allowlist: &HashSet<String>,
+        path_kind: &F,
+    ) -> Result<(), CdiError>
+    where
+        F: Fn(&str) -> Option<CdiPathKind>,
+    {
+        for path in &self.writable_mount_paths {
+            if !normalized_writable_file_allowlist.contains(path) {
+                return Err(CdiError::WritableMountNotAllowed { path: path.clone() });
+            }
+            let kind = path_kind(path);
+            if kind != Some(CdiPathKind::File) {
+                return Err(CdiError::WritableMountNotFile {
+                    path: path.clone(),
+                    kind: kind.map_or_else(|| "missing".to_string(), |kind| kind.to_string()),
+                });
+            }
         }
-        self.gids.insert(gid);
         Ok(())
     }
 
@@ -211,7 +234,7 @@ impl RequirementAccumulator {
                 CdiAccess::ReadWrite => requirements.read_write_paths.push(path),
             }
         }
-        requirements.supplemental_gids = self.gids.into_iter().collect();
+        requirements.additional_gids = self.additional_gids.into_iter().collect();
         requirements
     }
 }
@@ -245,28 +268,26 @@ where
     S: BuildHasher,
 {
     validate_context(context)?;
-    let selected_devices = parse_selected_devices(&context.selected_devices)?;
+    let selected_devices = selected_cdi_devices(&context.selected_devices);
+    if selected_devices.is_empty() || context.spec_dirs.is_empty() {
+        return Ok(CdiDerivedRequirements::default());
+    }
+
     let normalized_allowlist = writable_file_allowlist
         .iter()
         .map(|path| normalize_path(path))
         .collect::<HashSet<_>>();
 
     let edits = resolve_container_edits(context, &selected_devices)?;
     let mut accumulator = RequirementAccumulator::default();
-    apply_edits(&edits, &normalized_allowlist, &path_kind, &mut accumulator)?;
+    accumulate_requirements(&edits, &normalized_allowlist, &path_kind, &mut accumulator)?;
     Ok(accumulator.build())
 }
 
 fn validate_context(context: &CdiContext) -> Result<(), CdiError> {
     if context.version != CDI_CONTEXT_VERSION {
         return Err(CdiError::UnsupportedContextVersion(context.version));
     }
-    if context.selected_devices.is_empty() {
-        return Err(CdiError::EmptySelection);
-    }
-    if context.spec_dirs.is_empty() {
-        return Err(CdiError::EmptySpecDirs);
-    }
     for spec_dir in &context.spec_dirs {
         validate_absolute_no_parent(&spec_dir.path).map_err(|reason| CdiError::UnsafeSpecDir {
             path: spec_dir.path.clone(),
@@ -277,29 +298,26 @@ fn validate_context(context: &CdiContext) -> Result<(), CdiError> {
     Ok(())
 }
 
-fn parse_selected_devices(device_ids: &[String]) -> Result<Vec<String>, CdiError> {
+fn selected_cdi_devices(device_ids: &[String]) -> Vec<String> {
     let mut seen = HashSet::new();
     let mut parsed = Vec::new();
     for raw in device_ids {
         let raw = raw.trim();
-        let Some((kind, name)) = raw.rsplit_once('=') else {
-            return Err(CdiError::InvalidSelectedDevice(raw.to_string()));
-        };
-        if kind.trim().is_empty() || name.trim().is_empty() {
-            return Err(CdiError::InvalidSelectedDevice(raw.to_string()));
+        if raw.is_empty() {
+            continue;
         }
         if seen.insert(raw.to_string()) {
             parsed.push(raw.to_string());
         }
     }
-    Ok(parsed)
+    parsed
 }
 
 fn resolve_container_edits(
     context: &CdiContext,
     selected_devices: &[String],
 ) -> Result<CdiContainerEdits, CdiError> {
-    let mut cache = build_cache(&context.spec_dirs)?;
+    let mut cache = build_cache(&context.spec_dirs);
     let mut merged = UpstreamContainerEdits::new();
     let mut applied_specs = BTreeSet::new();
 
@@ -333,7 +351,7 @@ fn resolve_container_edits(
     serde_json::from_value(value).map_err(|source| CdiError::EditDecode { source })
 }
 
-fn build_cache(spec_dirs: &[CdiSpecDirectory]) -> Result<Cache, CdiError> {
+fn build_cache(spec_dirs: &[CdiSpecDirectory]) -> Cache {
     let spec_dir_paths = spec_dirs
         .iter()
         .map(|spec_dir| spec_dir.path.as_str())
@@ -343,16 +361,16 @@ fn build_cache(spec_dirs: &[CdiSpecDirectory]) -> Result<Cache, CdiError> {
         with_spec_dirs(&spec_dir_paths),
         with_auto_refresh(false),
     ]);
-    cache.refresh().map_err(|err| CdiError::CacheRefresh {
-        error: err.to_string(),
-    })?;
-    if cache.list_devices().is_empty() {
-        return Err(CdiError::NoSpecFiles);
+    if let Err(err) = cache.refresh() {
+        tracing::debug!(
+            error = %err,
+            "Ignoring CDI cache refresh error; requested device lookup will determine availability"
+        );
     }
-    Ok(cache)
+    cache
 }
 
-fn apply_edits<F>(
+fn accumulate_requirements<F>(
     edits: &CdiContainerEdits,
     normalized_writable_file_allowlist: &HashSet<String>,
     path_kind: &F,
@@ -363,33 +381,20 @@ where
 {
     for device_node in &edits.device_nodes {
         let path = normalize_policy_path(&device_node.path)?;
-        accumulator.add_path(path, CdiAccess::ReadWrite)?;
+        accumulator.add_device_node(path)?;
     }
 
     for mount in &edits.mounts {
         let path = normalize_policy_path(&mount.container_path)?;
-        match mount_access(&path, &mount.options)? {
-            CdiAccess::ReadOnly => accumulator.add_path(path, CdiAccess::ReadOnly)?,
-            CdiAccess::ReadWrite => {
-                if !normalized_writable_file_allowlist.contains(&path) {
-                    return Err(CdiError::WritableMountNotAllowed { path });
-                }
-                let kind = path_kind(&path);
-                if kind != Some(CdiPathKind::File) {
-                    return Err(CdiError::WritableMountNotFile {
-                        path,
-                        kind: kind.map_or_else(|| "missing".to_string(), |kind| kind.to_string()),
-                    });
-                }
-                accumulator.add_path(path, CdiAccess::ReadWrite)?;
-            }
-        }
+        let access = mount_access(&path, &mount.options)?;
+        accumulator.add_mount(path, access)?;
     }
 
     for gid in &edits.additional_gids {
-        accumulator.add_gid(*gid)?;
+        accumulator.add_gid(*gid);
     }
 
+    accumulator.validate_writable_mounts(normalized_writable_file_allowlist, path_kind)?;
     Ok(())
 }
 
@@ -400,6 +405,9 @@ fn mount_access(path: &str, options: &[String]) -> Result<CdiAccess, CdiError> {
     let read_write_requested = options
         .iter()
         .any(|option| option.eq_ignore_ascii_case("rw"));
+    // CDI mount options are stringly typed and runtime-specific normalization
+    // can differ. Treat simultaneous ro/rw as malformed instead of guessing
+    // which option a later mount syscall would effectively apply.
     if read_only_requested && read_write_requested {
         return Err(CdiError::ConflictingMountOptions {
             path: path.to_string(),
@@ -624,7 +632,7 @@ devices:
         )
         .unwrap();
 
-        assert_eq!(requirements.supplemental_gids, vec![44]);
+        assert_eq!(requirements.additional_gids, vec![44]);
         assert_eq!(
             requirements.read_write_paths,
             vec!["/dev/nvhost-gpu", "/dev/nvmap"]
@@ -717,7 +725,7 @@ devices:
     }
 
     #[test]
-    fn rejects_malformed_spec() {
+    fn defers_malformed_spec_errors_to_device_resolution() {
         let dir = tempfile::tempdir().unwrap();
         write_spec(dir.path(), "broken.yaml", "kind: [");
 
@@ -728,7 +736,49 @@ devices:
         )
         .unwrap_err();
 
-        assert!(matches!(err, CdiError::CacheRefresh { .. }));
+        assert!(matches!(err, CdiError::MissingDevice(device) if device == "nvidia.com/gpu=0"));
+    }
+
+    #[test]
+    fn empty_selection_is_noop() {
+        let dir = tempfile::tempdir().unwrap();
+        let requirements = resolve_with_kind(&context(dir.path(), &[]), &[], always_missing)
+            .expect("empty selection should resolve to empty requirements");
+
+        assert_eq!(requirements, CdiDerivedRequirements::default());
+    }
+
+    #[test]
+    fn empty_spec_dirs_are_noop() {
+        let context = CdiContext::new("test", vec!["nvidia.com/gpu=0".to_string()], Vec::new());
+
+        let requirements = resolve_with_kind(&context, &[], always_missing)
+            .expect("empty spec dirs should resolve to empty requirements");
+
+        assert_eq!(requirements, CdiDerivedRequirements::default());
+    }
+
+    #[test]
+    fn defers_device_id_shape_to_upstream_resolution() {
+        let dir = tempfile::tempdir().unwrap();
+        write_spec(
+            dir.path(),
+            "nvidia.yaml",
+            r#"
+cdiVersion: 1.1.0
+kind: nvidia.com/gpu
+devices:
+  - name: "0"
+    containerEdits:
+      env:
+        - OPEN_SHELL_TEST=1
+"#,
+        );
+
+        let err = resolve_with_kind(&context(dir.path(), &["not-a-cdi-id"]), &[], always_missing)
+            .unwrap_err();
+
+        assert!(matches!(err, CdiError::MissingDevice(device) if device == "not-a-cdi-id"));
     }
 
     #[test]
@@ -799,7 +849,7 @@ devices:
     }
 
     #[test]
-    fn rejects_root_additional_gid() {
+    fn skips_root_additional_gid() {
         let dir = tempfile::tempdir().unwrap();
         write_spec(
             dir.path(),
@@ -810,17 +860,17 @@ kind: nvidia.com/gpu
 devices:
   - name: "0"
     containerEdits:
-      additionalGids: [0]
+      additionalGids: [0, 44]
 "#,
         );
 
-        let err = resolve_with_kind(
+        let requirements = resolve_with_kind(
             &context(dir.path(), &["nvidia.com/gpu=0"]),
             &[],
             always_missing,
         )
-        .unwrap_err();
+        .unwrap();
 
-        assert!(matches!(err, CdiError::RootAdditionalGid));
+        assert_eq!(requirements.additional_gids, vec![44]);
     }
 }

crates/openshell-core/src/policy.rs

@@ -84,7 +84,10 @@ pub struct ProcessPolicy {
     /// Group name to run the sandboxed process as.
     pub run_as_group: Option<String>,
 
-    /// Runtime-derived supplemental GIDs to apply before dropping privileges.
+    /// Linux supplemental groups to apply before dropping privileges.
+    ///
+    /// Runtime-specific inputs can use different terminology; CDI
+    /// `additionalGids` are converted into this process-level representation.
     pub supplemental_groups: Vec<u32>,
 }