From 1dfe6e74a4313f566db503d79511dcad0d56301e Mon Sep 17 00:00:00 2001 From: mjamiv Date: Sat, 27 Jun 2026 14:07:26 +0000 Subject: [PATCH 1/5] feat(kubernetes): support PVC subPath driver config Signed-off-by: mjamiv --- crates/openshell-driver-kubernetes/README.md | 53 ++ .../openshell-driver-kubernetes/src/driver.rs | 661 +++++++++++++++++- docs/reference/sandbox-compute-drivers.mdx | 60 ++ 3 files changed, 761 insertions(+), 13 deletions(-) diff --git a/crates/openshell-driver-kubernetes/README.md b/crates/openshell-driver-kubernetes/README.md index 688660dbb..699a850d0 100644 --- a/crates/openshell-driver-kubernetes/README.md +++ b/crates/openshell-driver-kubernetes/README.md @@ -117,6 +117,13 @@ nested schema and currently accepts: - `pod.priority_class_name` - `containers.agent.resources.requests` - `containers.agent.resources.limits` +- `volumes[].name` +- `volumes[].persistent_volume_claim.claim_name` +- `volumes[].persistent_volume_claim.read_only` +- `containers.agent.volume_mounts[].name` +- `containers.agent.volume_mounts[].mount_path` +- `containers.agent.volume_mounts[].sub_path` +- `containers.agent.volume_mounts[].read_only` Nested keys inside the `kubernetes` block use snake_case. The top-level `driver_config` envelope is keyed by driver names, so `kubernetes` is not part @@ -139,3 +146,49 @@ driver's configured `default_runtime_class_name`; the typed public public `--gpu` flag for the default GPU request, pass a count to `--gpu` for counted GPU requests, and use `driver_config` only for additional driver-owned resource details. + +Use PVC volumes to mount existing Kubernetes PersistentVolumeClaims into the +agent container. PVC volumes and mounts default to read-only unless +`read_only: false` is set explicitly. A read-only PVC volume cannot be mounted +read-write. The driver rejects duplicate volume names, mounts that reference +unknown volumes, non-normalized or protected mount paths, and absolute or +parent-traversing `sub_path` values. + +Any explicit driver-config mount under `/sandbox/` disables the driver's +default `/sandbox` workspace PVC injection for that sandbox. This keeps image +contents fresh while allowing selected durable data paths to come from an +external PVC. + +```shell +openshell sandbox create \ + --driver-config-json '{ + "kubernetes": { + "volumes": [{ + "name": "pete-user-data", + "persistent_volume_claim": { + "claim_name": "pvc-pete-user-123", + "read_only": false + } + }], + "containers": { + "agent": { + "volume_mounts": [ + { + "name": "pete-user-data", + "mount_path": "/sandbox/.openclaw/workspace", + "sub_path": "workspace", + "read_only": false + }, + { + "name": "pete-user-data", + "mount_path": "/sandbox/.openclaw/memory", + "sub_path": "memory", + "read_only": false + } + ] + } + } + } + }' \ + -- claude +``` diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index db7492d27..9ca198b8d 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -16,6 +16,7 @@ use kube::core::gvk::GroupVersionKind; use kube::core::{DynamicObject, ObjectMeta}; use kube::runtime::watcher::{self, Event}; use kube::{Client, Error as KubeError}; +use openshell_core::driver_mounts; use openshell_core::driver_utils::{ LABEL_MANAGED_BY, LABEL_MANAGED_BY_VALUE, LABEL_SANDBOX_ID, SUPERVISOR_IMAGE_BINARY_PATH, }; @@ -34,7 +35,7 @@ use openshell_core::proto::compute::v1::{ }; use openshell_core::proto_struct::{struct_to_json_object, value_to_json}; use serde::Deserialize; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use std::pin::Pin; use std::sync::Arc; use std::time::Duration; @@ -107,6 +108,7 @@ struct AgentSandboxApi { struct KubernetesSandboxDriverConfig { pod: KubernetesPodDriverConfig, containers: KubernetesDriverContainersConfig, + volumes: Vec, } impl KubernetesSandboxDriverConfig { @@ -128,8 +130,29 @@ impl KubernetesSandboxDriverConfig { }; let json = serde_json::Value::Object(struct_to_json_object(config)); - serde_json::from_value(json) - .map_err(|err| format!("invalid kubernetes driver_config: {err}")) + let config: Self = serde_json::from_value(json) + .map_err(|err| format!("invalid kubernetes driver_config: {err}"))?; + config + .validate() + .map_err(|err| format!("invalid kubernetes driver_config: {err}"))?; + Ok(config) + } + + fn validate(&self) -> Result<(), String> { + validate_kubernetes_driver_volumes(&self.volumes)?; + validate_kubernetes_driver_volume_mounts( + &self.volumes, + &self.containers.agent.volume_mounts, + ) + } + + fn has_explicit_sandbox_data_mount(&self) -> bool { + self.containers + .agent + .volume_mounts + .iter() + .filter_map(|mount| validate_kubernetes_mount_path(&mount.mount_path).ok()) + .any(|mount_path| path_is_or_under(&mount_path, WORKSPACE_MOUNT_PATH)) } } @@ -152,6 +175,7 @@ struct KubernetesDriverContainersConfig { #[serde(default, deny_unknown_fields)] struct KubernetesContainerDriverConfig { resources: KubernetesContainerResourceConfig, + volume_mounts: Vec, } #[derive(Debug, Clone, Default, Deserialize)] @@ -161,6 +185,234 @@ struct KubernetesContainerResourceConfig { limits: BTreeMap, } +#[derive(Debug, Clone, Default, Deserialize)] +#[serde(default, deny_unknown_fields)] +struct KubernetesDriverVolumeConfig { + name: String, + persistent_volume_claim: KubernetesPersistentVolumeClaimConfig, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(default, deny_unknown_fields)] +struct KubernetesPersistentVolumeClaimConfig { + claim_name: String, + #[serde(default = "default_true")] + read_only: bool, +} + +impl Default for KubernetesPersistentVolumeClaimConfig { + fn default() -> Self { + Self { + claim_name: String::new(), + read_only: true, + } + } +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(default, deny_unknown_fields)] +struct KubernetesDriverVolumeMountConfig { + name: String, + mount_path: String, + sub_path: Option, + #[serde(default = "default_true")] + read_only: bool, +} + +impl Default for KubernetesDriverVolumeMountConfig { + fn default() -> Self { + Self { + name: String::new(), + mount_path: String::new(), + sub_path: None, + read_only: true, + } + } +} + +fn default_true() -> bool { + true +} + +const KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES: &[&str] = &[ + "openshell-client-tls", + "openshell-sa-token", + SPIFFE_WORKLOAD_API_VOLUME_NAME, + SUPERVISOR_VOLUME_NAME, + WORKSPACE_VOLUME_NAME, +]; + +const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = &[ + "/etc/openshell", + "/etc/openshell-tls", + "/var/run/secrets/openshell", + "/run/netns", + "/spiffe-workload-api", + openshell_core::driver_utils::SUPERVISOR_CONTAINER_DIR, + openshell_core::driver_utils::SUPERVISOR_CONTAINER_BINARY, +]; + +fn validate_kubernetes_driver_volumes( + volumes: &[KubernetesDriverVolumeConfig], +) -> Result<(), String> { + let mut names = HashSet::new(); + for volume in volumes { + let name = validate_kubernetes_reference_name(&volume.name, "volumes[].name")?; + if KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES.contains(&name.as_str()) { + return Err(format!( + "volume name '{name}' is reserved for OpenShell-managed volumes" + )); + } + if !names.insert(name.clone()) { + return Err(format!( + "duplicate kubernetes driver_config volume '{name}'" + )); + } + validate_kubernetes_reference_name( + &volume.persistent_volume_claim.claim_name, + "volumes[].persistent_volume_claim.claim_name", + )?; + } + Ok(()) +} + +fn validate_kubernetes_driver_volume_mounts( + volumes: &[KubernetesDriverVolumeConfig], + volume_mounts: &[KubernetesDriverVolumeMountConfig], +) -> Result<(), String> { + let mut volume_read_only = BTreeMap::new(); + for volume in volumes { + let name = validate_kubernetes_reference_name(&volume.name, "volumes[].name")?; + volume_read_only.insert(name, volume.persistent_volume_claim.read_only); + } + + let mut mount_paths = HashSet::new(); + for mount in volume_mounts { + let volume_name = validate_kubernetes_reference_name( + &mount.name, + "containers.agent.volume_mounts[].name", + )?; + let Some(volume_is_read_only) = volume_read_only.get(&volume_name) else { + return Err(format!( + "volume mount references unknown kubernetes driver_config volume '{volume_name}'" + )); + }; + if *volume_is_read_only && !mount.read_only { + return Err(format!( + "volume mount '{volume_name}' cannot set read_only=false because the PVC volume is read_only=true" + )); + } + + let mount_path = validate_kubernetes_mount_path(&mount.mount_path)?; + if !mount_paths.insert(mount_path.clone()) { + return Err(format!( + "duplicate kubernetes driver_config volume mount path '{mount_path}'" + )); + } + + if let Some(sub_path) = mount.sub_path.as_ref() { + driver_mounts::validate_mount_subpath(sub_path)?; + } + } + Ok(()) +} + +fn validate_kubernetes_driver_runtime_mounts( + config: &KubernetesSandboxDriverConfig, + provider_spiffe_workload_api_socket_path: Option<&str>, +) -> Result<(), String> { + let Some(socket_path) = provider_spiffe_workload_api_socket_path else { + return Ok(()); + }; + let spiffe_mount_path = spiffe_socket_mount_path(socket_path); + for mount in &config.containers.agent.volume_mounts { + let mount_path = validate_kubernetes_mount_path(&mount.mount_path)?; + if mount_path_conflicts_with_protected_path(&mount_path, &spiffe_mount_path) { + return Err(format!( + "mount path '{mount_path}' conflicts with reserved OpenShell path '{spiffe_mount_path}'" + )); + } + } + Ok(()) +} + +fn validate_kubernetes_reference_name(value: &str, field: &str) -> Result { + let normalized = driver_mounts::validate_mount_source(value, field)?; + if normalized != value { + return Err(format!( + "{field} must not contain leading or trailing whitespace" + )); + } + Ok(normalized) +} + +fn validate_kubernetes_mount_path(mount_path: &str) -> Result { + let raw = mount_path.trim(); + if raw != mount_path { + return Err("mount_path must not contain leading or trailing whitespace".to_string()); + } + if raw != "/" + && raw + .split('/') + .skip(1) + .any(|segment| segment.is_empty() || segment == ".") + { + return Err( + "mount_path must be normalized and must not contain empty path segments or '.'" + .to_string(), + ); + } + + let mount_path = driver_mounts::validate_container_mount_target(raw)?; + for protected_path in KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS { + if mount_path_conflicts_with_protected_path(&mount_path, protected_path) { + return Err(format!( + "mount path '{mount_path}' conflicts with reserved OpenShell path '{protected_path}'" + )); + } + } + Ok(mount_path) +} + +fn mount_path_conflicts_with_protected_path(mount_path: &str, protected_path: &str) -> bool { + path_is_or_under(mount_path, protected_path) || path_is_or_under(protected_path, mount_path) +} + +fn path_is_or_under(path: &str, parent: &str) -> bool { + path == parent + || path + .strip_prefix(parent) + .is_some_and(|rest| rest.starts_with('/')) +} + +fn kubernetes_driver_volume_to_k8s(volume: &KubernetesDriverVolumeConfig) -> serde_json::Value { + serde_json::json!({ + "name": volume.name.as_str(), + "persistentVolumeClaim": { + "claimName": volume.persistent_volume_claim.claim_name.as_str(), + "readOnly": volume.persistent_volume_claim.read_only, + } + }) +} + +fn kubernetes_driver_volume_mount_to_k8s( + mount: &KubernetesDriverVolumeMountConfig, +) -> serde_json::Value { + let mut value = serde_json::json!({ + "name": mount.name.as_str(), + "mountPath": validate_kubernetes_mount_path(&mount.mount_path) + .expect("validated Kubernetes driver_config mount_path"), + "readOnly": mount.read_only, + }); + if let Some(sub_path) = mount.sub_path.as_ref() { + value["subPath"] = serde_json::json!( + driver_mounts::validate_mount_subpath(sub_path) + .expect("validated Kubernetes driver_config sub_path") + ); + } + value +} + // --------------------------------------------------------------------------- // Default workspace persistence (temporary — will be replaced by snapshotting) // --------------------------------------------------------------------------- @@ -274,6 +526,22 @@ impl KubernetesComputeDriver { &self.config.ssh_socket_path } + fn validate_driver_config_for_sandbox( + &self, + sandbox: &Sandbox, + ) -> Result { + let config = KubernetesSandboxDriverConfig::from_sandbox(sandbox)?; + validate_kubernetes_driver_runtime_mounts( + &config, + self.config.provider_spiffe_enabled().then_some( + self.config + .provider_spiffe_workload_api_socket_path + .as_str(), + ), + )?; + Ok(config) + } + fn agent_sandbox_api(&self, client: Client, sandbox_api_version: &str) -> AgentSandboxApi { let gvk = GroupVersionKind::gvk(SANDBOX_GROUP, sandbox_api_version, SANDBOX_KIND); let resource = ApiResource::from_gvk(&gvk); @@ -421,7 +689,8 @@ impl KubernetesComputeDriver { } pub async fn validate_sandbox_create(&self, sandbox: &Sandbox) -> Result<(), tonic::Status> { - let _ = KubernetesSandboxDriverConfig::from_sandbox(sandbox) + let _ = self + .validate_driver_config_for_sandbox(sandbox) .map_err(tonic::Status::invalid_argument)?; let gpu_requirements = sandbox .spec @@ -530,7 +799,8 @@ impl KubernetesComputeDriver { #[allow(clippy::similar_names)] pub async fn create_sandbox(&self, sandbox: &Sandbox) -> Result<(), KubernetesDriverError> { - let _ = KubernetesSandboxDriverConfig::from_sandbox(sandbox) + let _ = self + .validate_driver_config_for_sandbox(sandbox) .map_err(KubernetesDriverError::InvalidArgument)?; let gpu_requirements = sandbox .spec @@ -1904,6 +2174,17 @@ fn sandbox_to_k8s_spec( ) -> serde_json::Value { let mut root = serde_json::Map::new(); + // Determine early whether OpenShell should inject its default workspace + // PVC. Explicit Kubernetes driver-config mounts under /sandbox/ take + // ownership of workspace persistence. + // We need this flag before building the podTemplate because the workspace + // persistence transforms are applied inside sandbox_template_to_k8s. + let template = spec.and_then(|s| s.template.as_ref()); + let user_has_explicit_workspace_mount = template + .map(kubernetes_driver_config) + .is_some_and(|config| config.has_explicit_sandbox_data_mount()); + let inject_workspace = !user_has_explicit_workspace_mount; + if let Some(spec) = spec { let pod_env = spec_pod_env(Some(spec)); if let Some(template) = spec.template.as_ref() { @@ -1913,7 +2194,7 @@ fn sandbox_to_k8s_spec( template, driver_gpu_requirements(spec.resource_requirements.as_ref()), &pod_env, - true, + inject_workspace, params, ), ); @@ -1926,10 +2207,12 @@ fn sandbox_to_k8s_spec( } } - root.insert( - "volumeClaimTemplates".to_string(), - default_workspace_volume_claim_templates(params.workspace_default_storage_size), - ); + if inject_workspace { + root.insert( + "volumeClaimTemplates".to_string(), + default_workspace_volume_claim_templates(params.workspace_default_storage_size), + ); + } // podTemplate is required by the Kubernetes CRD - ensure it's always present if !root.contains_key("podTemplate") { @@ -1940,7 +2223,7 @@ fn sandbox_to_k8s_spec( &SandboxTemplate::default(), driver_gpu_requirements(spec.and_then(|s| s.resource_requirements.as_ref())), &pod_env, - true, + inject_workspace, params, ), ); @@ -2158,6 +2441,14 @@ fn sandbox_template_to_k8s_with_gpu_requirements( "mountPath": "/var/run/secrets/openshell", "readOnly": true, })); + volume_mounts.extend( + driver_config + .containers + .agent + .volume_mounts + .iter() + .map(kubernetes_driver_volume_mount_to_k8s), + ); container.insert( "volumeMounts".to_string(), serde_json::Value::Array(volume_mounts), @@ -2221,6 +2512,12 @@ fn sandbox_template_to_k8s_with_gpu_requirements( "defaultMode": sa_token_default_mode } })); + volumes.extend( + driver_config + .volumes + .iter() + .map(kubernetes_driver_volume_to_k8s), + ); spec.insert("volumes".to_string(), serde_json::Value::Array(volumes)); // Add hostAliases so sandbox pods can reach the Docker host. @@ -2264,8 +2561,8 @@ fn sandbox_template_to_k8s_with_gpu_requirements( } // Inject workspace persistence (init container + PVC volume mount) so - // that /sandbox data survives pod rescheduling. Skipped when the user - // provides custom volumeClaimTemplates to avoid conflicts. + // that /sandbox data survives pod rescheduling. Skipped when the user + // provides custom storage through driver_config. if inject_workspace { apply_workspace_persistence( &mut result, @@ -2819,6 +3116,344 @@ mod tests { assert!(err.contains("gpu_device_ids")); } + #[test] + fn driver_config_pvc_subpath_mounts_render_in_pod_template() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "pete-user-data", + "persistent_volume_claim": { + "claim_name": "pvc-pete-user-123", + "read_only": false + } + }], + "containers": { + "agent": { + "volume_mounts": [ + { + "name": "pete-user-data", + "mount_path": "/sandbox/.openclaw/workspace", + "sub_path": "workspace", + "read_only": false + }, + { + "name": "pete-user-data", + "mount_path": "/sandbox/.openclaw/memory", + "sub_path": "memory" + } + ] + } + } + }))), + ..SandboxTemplate::default() + }; + let spec = SandboxSpec { + template: Some(template), + ..SandboxSpec::default() + }; + + let cr = sandbox_to_k8s_spec(Some(&spec), &SandboxPodParams::default()); + let pod_template = &cr["spec"]["podTemplate"]; + + let volumes = pod_template["spec"]["volumes"] + .as_array() + .expect("volumes should exist"); + let user_volume = volumes + .iter() + .find(|volume| volume["name"] == "pete-user-data") + .expect("user PVC volume should be rendered"); + assert_eq!( + user_volume["persistentVolumeClaim"]["claimName"], + "pvc-pete-user-123" + ); + assert_eq!(user_volume["persistentVolumeClaim"]["readOnly"], false); + + let mounts = pod_template["spec"]["containers"][0]["volumeMounts"] + .as_array() + .expect("volumeMounts should exist"); + let workspace_mount = mounts + .iter() + .find(|mount| mount["mountPath"] == "/sandbox/.openclaw/workspace") + .expect("workspace subPath mount should be rendered"); + assert_eq!(workspace_mount["name"], "pete-user-data"); + assert_eq!(workspace_mount["subPath"], "workspace"); + assert_eq!(workspace_mount["readOnly"], false); + + let memory_mount = mounts + .iter() + .find(|mount| mount["mountPath"] == "/sandbox/.openclaw/memory") + .expect("memory subPath mount should be rendered"); + assert_eq!(memory_mount["name"], "pete-user-data"); + assert_eq!(memory_mount["subPath"], "memory"); + assert_eq!(memory_mount["readOnly"], true); + + let spec_obj = cr["spec"].as_object().expect("spec should be an object"); + assert!( + !spec_obj.contains_key("volumeClaimTemplates"), + "explicit /sandbox driver_config mounts should skip the default workspace VCT" + ); + let has_workspace_init = pod_template["spec"]["initContainers"] + .as_array() + .is_some_and(|containers| { + containers + .iter() + .any(|container| container["name"] == WORKSPACE_INIT_CONTAINER_NAME) + }); + assert!( + !has_workspace_init, + "explicit /sandbox driver_config mounts should skip the default workspace init container" + ); + } + + #[test] + fn driver_config_rejects_duplicate_pvc_volume_names() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [ + { + "name": "pete-user-data", + "persistent_volume_claim": {"claim_name": "pvc-a"} + }, + { + "name": "pete-user-data", + "persistent_volume_claim": {"claim_name": "pvc-b"} + } + ] + }))), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + + assert!(err.contains("duplicate kubernetes driver_config volume")); + } + + #[test] + fn driver_config_rejects_mounts_referencing_unknown_volumes() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "known-data", + "persistent_volume_claim": {"claim_name": "pvc-known"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "missing-data", + "mount_path": "/sandbox/.openclaw/workspace", + "sub_path": "workspace" + }] + } + } + }))), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + + assert!(err.contains("unknown kubernetes driver_config volume 'missing-data'")); + } + + #[test] + fn driver_config_rejects_protected_kubernetes_mount_targets() { + for mount_path in [ + "/", + "/sandbox", + "/etc/openshell", + "/etc/openshell-tls/client", + "/var/run/secrets/openshell", + "/opt/openshell/bin", + "/spiffe-workload-api", + ] { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": mount_path + }] + } + } + }))), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + assert!( + err.contains("mount path") || err.contains("mount target"), + "expected protected mount target {mount_path:?} to be rejected, got {err}" + ); + } + } + + #[test] + fn driver_config_rejects_invalid_kubernetes_sub_paths() { + for sub_path in ["/workspace", "../workspace"] { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": "/sandbox/.openclaw/workspace", + "sub_path": sub_path + }] + } + } + }))), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + assert!( + err.contains("mount subpath must be relative"), + "expected invalid sub_path {sub_path:?} to be rejected, got {err}" + ); + } + } + + #[test] + fn driver_config_defaults_pvc_mounts_to_read_only() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": "/sandbox/.openclaw/workspace", + "sub_path": "workspace" + }] + } + } + }))), + ..SandboxTemplate::default() + }; + + let pod_template = sandbox_template_to_k8s( + &template, + false, + &std::collections::HashMap::new(), + false, + &SandboxPodParams::default(), + ); + + let volume = pod_template["spec"]["volumes"] + .as_array() + .expect("volumes should exist") + .iter() + .find(|volume| volume["name"] == "user-data") + .expect("user volume should exist"); + assert_eq!(volume["persistentVolumeClaim"]["readOnly"], true); + + let mount = pod_template["spec"]["containers"][0]["volumeMounts"] + .as_array() + .expect("volumeMounts should exist") + .iter() + .find(|mount| mount["mountPath"] == "/sandbox/.openclaw/workspace") + .expect("user mount should exist"); + assert_eq!(mount["readOnly"], true); + } + + #[test] + fn driver_config_rejects_read_write_mount_for_read_only_pvc_volume() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": { + "claim_name": "pvc-user-data", + "read_only": true + } + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": "/sandbox/.openclaw/workspace", + "read_only": false + }] + } + } + }))), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + + assert!(err.contains("cannot set read_only=false")); + } + + #[test] + fn driver_config_rejects_reserved_kubernetes_volume_names() { + for volume_name in [ + "openshell-client-tls", + "openshell-sa-token", + SPIFFE_WORKLOAD_API_VOLUME_NAME, + SUPERVISOR_VOLUME_NAME, + WORKSPACE_VOLUME_NAME, + ] { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": volume_name, + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }] + }))), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + assert!( + err.contains("reserved for OpenShell-managed volumes"), + "expected reserved volume name {volume_name:?} to be rejected, got {err}" + ); + } + } + + #[test] + fn driver_config_rejects_runtime_provider_spiffe_mount_path() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": "/custom-spiffe" + }] + } + } + }))), + ..SandboxTemplate::default() + }; + let config = KubernetesSandboxDriverConfig::from_template(&template) + .expect("static driver_config should be valid"); + + let err = validate_kubernetes_driver_runtime_mounts( + &config, + Some("/custom-spiffe/spire-agent.sock"), + ) + .unwrap_err(); + + assert!(err.contains("/custom-spiffe")); + } + #[test] fn validate_rejects_zero_gpu_count() { let sandbox = Sandbox { diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index dc8bdb234..b318e5f6c 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -346,6 +346,66 @@ If Agent Sandbox is upgraded in place, restart the OpenShell gateway after the c `Sandbox.spec.volumeClaimTemplates` is immutable after creation. To change storage configuration, delete the sandbox and create a new one with the updated spec. +### Kubernetes Driver Config PVC Mounts + +Kubernetes driver config can mount existing PersistentVolumeClaims into the +agent container. Use this when storage is provisioned outside OpenShell and a +sandbox should mount selected PVC subpaths instead of using the default +OpenShell-created `/sandbox` workspace PVC. + +```shell +openshell sandbox create \ + --driver-config-json '{ + "kubernetes": { + "volumes": [{ + "name": "pete-user-data", + "persistent_volume_claim": { + "claim_name": "pvc-pete-user-123", + "read_only": false + } + }], + "containers": { + "agent": { + "volume_mounts": [ + { + "name": "pete-user-data", + "mount_path": "/sandbox/.openclaw/workspace", + "sub_path": "workspace", + "read_only": false + }, + { + "name": "pete-user-data", + "mount_path": "/sandbox/.openclaw/memory", + "sub_path": "memory", + "read_only": false + } + ] + } + } + } + }' \ + -- claude +``` + +Kubernetes PVC mount schema: + +| Field | Description | +|---|---| +| `volumes[].name` | Pod volume name. It must be unique and must not use OpenShell-managed volume names. | +| `volumes[].persistent_volume_claim.claim_name` | Existing PVC name in the sandbox namespace. | +| `volumes[].persistent_volume_claim.read_only` | Optional. Defaults to `true`. Set `false` to allow read-write mounts. | +| `containers.agent.volume_mounts[].name` | References a volume declared in `volumes`. | +| `containers.agent.volume_mounts[].mount_path` | Absolute, normalized container path for the agent mount. | +| `containers.agent.volume_mounts[].sub_path` | Optional relative PVC subpath. Absolute paths and `..` are rejected. | +| `containers.agent.volume_mounts[].read_only` | Optional. Defaults to `true`. It cannot be `false` when the PVC volume is read-only. | + +OpenShell rejects duplicate volume names, mounts that reference unknown volumes, +protected mount targets, and mounts that replace OpenShell TLS, supervisor, +ServiceAccount token, or SPIFFE paths. Any driver-config mount below +`/sandbox/` disables the default `/sandbox` workspace PVC injection for that +sandbox, so image-backed files outside the explicit mounts come from the current +sandbox image. + ## Sandbox User Identity OpenShell accepts both the hardcoded username `"sandbox"` and numeric UIDs in `[1000, 2_000_000_000]` for the supervisor's process identity (the policy's `run_as_user` field). The driver resolves the UID at sandbox creation time and passes it to the supervisor via environment variables. From 64885156decf42d5c4d8c348925640df2429f5c3 Mon Sep 17 00:00:00 2001 From: mjamiv Date: Sat, 27 Jun 2026 22:55:50 +0000 Subject: [PATCH 2/5] test(kubernetes): cover writable PVC driver config Signed-off-by: mjamiv --- crates/openshell-driver-kubernetes/README.md | 12 +-- .../openshell-driver-kubernetes/src/driver.rs | 99 +++++++++++++++---- docs/reference/sandbox-compute-drivers.mdx | 12 +-- 3 files changed, 92 insertions(+), 31 deletions(-) diff --git a/crates/openshell-driver-kubernetes/README.md b/crates/openshell-driver-kubernetes/README.md index 699a850d0..830679a61 100644 --- a/crates/openshell-driver-kubernetes/README.md +++ b/crates/openshell-driver-kubernetes/README.md @@ -164,9 +164,9 @@ openshell sandbox create \ --driver-config-json '{ "kubernetes": { "volumes": [{ - "name": "pete-user-data", + "name": "user-data", "persistent_volume_claim": { - "claim_name": "pvc-pete-user-123", + "claim_name": "pvc-user-data-123", "read_only": false } }], @@ -174,14 +174,14 @@ openshell sandbox create \ "agent": { "volume_mounts": [ { - "name": "pete-user-data", - "mount_path": "/sandbox/.openclaw/workspace", + "name": "user-data", + "mount_path": "/sandbox/.openshell/workspace", "sub_path": "workspace", "read_only": false }, { - "name": "pete-user-data", - "mount_path": "/sandbox/.openclaw/memory", + "name": "user-data", + "mount_path": "/sandbox/.openshell/memory", "sub_path": "memory", "read_only": false } diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index 9ca198b8d..b8bee47e0 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -3121,9 +3121,9 @@ mod tests { let template = SandboxTemplate { driver_config: Some(json_struct(serde_json::json!({ "volumes": [{ - "name": "pete-user-data", + "name": "user-data", "persistent_volume_claim": { - "claim_name": "pvc-pete-user-123", + "claim_name": "pvc-user-data-123", "read_only": false } }], @@ -3131,14 +3131,14 @@ mod tests { "agent": { "volume_mounts": [ { - "name": "pete-user-data", - "mount_path": "/sandbox/.openclaw/workspace", + "name": "user-data", + "mount_path": "/sandbox/.openshell/workspace", "sub_path": "workspace", "read_only": false }, { - "name": "pete-user-data", - "mount_path": "/sandbox/.openclaw/memory", + "name": "user-data", + "mount_path": "/sandbox/.openshell/memory", "sub_path": "memory" } ] @@ -3160,11 +3160,11 @@ mod tests { .expect("volumes should exist"); let user_volume = volumes .iter() - .find(|volume| volume["name"] == "pete-user-data") + .find(|volume| volume["name"] == "user-data") .expect("user PVC volume should be rendered"); assert_eq!( user_volume["persistentVolumeClaim"]["claimName"], - "pvc-pete-user-123" + "pvc-user-data-123" ); assert_eq!(user_volume["persistentVolumeClaim"]["readOnly"], false); @@ -3173,17 +3173,17 @@ mod tests { .expect("volumeMounts should exist"); let workspace_mount = mounts .iter() - .find(|mount| mount["mountPath"] == "/sandbox/.openclaw/workspace") + .find(|mount| mount["mountPath"] == "/sandbox/.openshell/workspace") .expect("workspace subPath mount should be rendered"); - assert_eq!(workspace_mount["name"], "pete-user-data"); + assert_eq!(workspace_mount["name"], "user-data"); assert_eq!(workspace_mount["subPath"], "workspace"); assert_eq!(workspace_mount["readOnly"], false); let memory_mount = mounts .iter() - .find(|mount| mount["mountPath"] == "/sandbox/.openclaw/memory") + .find(|mount| mount["mountPath"] == "/sandbox/.openshell/memory") .expect("memory subPath mount should be rendered"); - assert_eq!(memory_mount["name"], "pete-user-data"); + assert_eq!(memory_mount["name"], "user-data"); assert_eq!(memory_mount["subPath"], "memory"); assert_eq!(memory_mount["readOnly"], true); @@ -3205,17 +3205,78 @@ mod tests { ); } + #[test] + fn driver_config_accepts_read_write_pvc_with_multiple_subpath_mounts() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": { + "claim_name": "pvc-user-data", + "read_only": false + } + }], + "containers": { + "agent": { + "volume_mounts": [ + { + "name": "user-data", + "mount_path": "/sandbox/.openshell/workspace", + "sub_path": "workspace", + "read_only": false + }, + { + "name": "user-data", + "mount_path": "/sandbox/.openshell/memory", + "sub_path": "memory", + "read_only": false + }, + { + "name": "user-data", + "mount_path": "/sandbox/.openshell/sessions", + "sub_path": "sessions", + "read_only": false + } + ] + } + } + }))), + ..SandboxTemplate::default() + }; + + let config = KubernetesSandboxDriverConfig::from_template(&template) + .expect("read-write PVC with multiple subPath mounts should validate"); + + assert_eq!(config.volumes.len(), 1); + assert_eq!(config.volumes[0].name, "user-data"); + assert_eq!( + config.volumes[0].persistent_volume_claim.claim_name, + "pvc-user-data" + ); + assert!(!config.volumes[0].persistent_volume_claim.read_only); + assert_eq!(config.containers.agent.volume_mounts.len(), 3); + assert!( + config + .containers + .agent + .volume_mounts + .iter() + .all(|mount| !mount.read_only) + ); + assert!(config.has_explicit_sandbox_data_mount()); + } + #[test] fn driver_config_rejects_duplicate_pvc_volume_names() { let template = SandboxTemplate { driver_config: Some(json_struct(serde_json::json!({ "volumes": [ { - "name": "pete-user-data", + "name": "user-data", "persistent_volume_claim": {"claim_name": "pvc-a"} }, { - "name": "pete-user-data", + "name": "user-data", "persistent_volume_claim": {"claim_name": "pvc-b"} } ] @@ -3240,7 +3301,7 @@ mod tests { "agent": { "volume_mounts": [{ "name": "missing-data", - "mount_path": "/sandbox/.openclaw/workspace", + "mount_path": "/sandbox/.openshell/workspace", "sub_path": "workspace" }] } @@ -3304,7 +3365,7 @@ mod tests { "agent": { "volume_mounts": [{ "name": "user-data", - "mount_path": "/sandbox/.openclaw/workspace", + "mount_path": "/sandbox/.openshell/workspace", "sub_path": sub_path }] } @@ -3333,7 +3394,7 @@ mod tests { "agent": { "volume_mounts": [{ "name": "user-data", - "mount_path": "/sandbox/.openclaw/workspace", + "mount_path": "/sandbox/.openshell/workspace", "sub_path": "workspace" }] } @@ -3362,7 +3423,7 @@ mod tests { .as_array() .expect("volumeMounts should exist") .iter() - .find(|mount| mount["mountPath"] == "/sandbox/.openclaw/workspace") + .find(|mount| mount["mountPath"] == "/sandbox/.openshell/workspace") .expect("user mount should exist"); assert_eq!(mount["readOnly"], true); } @@ -3382,7 +3443,7 @@ mod tests { "agent": { "volume_mounts": [{ "name": "user-data", - "mount_path": "/sandbox/.openclaw/workspace", + "mount_path": "/sandbox/.openshell/workspace", "read_only": false }] } diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index b318e5f6c..5e3cb157e 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -358,9 +358,9 @@ openshell sandbox create \ --driver-config-json '{ "kubernetes": { "volumes": [{ - "name": "pete-user-data", + "name": "user-data", "persistent_volume_claim": { - "claim_name": "pvc-pete-user-123", + "claim_name": "pvc-user-data-123", "read_only": false } }], @@ -368,14 +368,14 @@ openshell sandbox create \ "agent": { "volume_mounts": [ { - "name": "pete-user-data", - "mount_path": "/sandbox/.openclaw/workspace", + "name": "user-data", + "mount_path": "/sandbox/.openshell/workspace", "sub_path": "workspace", "read_only": false }, { - "name": "pete-user-data", - "mount_path": "/sandbox/.openclaw/memory", + "name": "user-data", + "mount_path": "/sandbox/.openshell/memory", "sub_path": "memory", "read_only": false } From e37cd63fbeae6401a0b30e44f4e4746701f840d9 Mon Sep 17 00:00:00 2001 From: mjamiv Date: Mon, 29 Jun 2026 16:11:27 +0000 Subject: [PATCH 3/5] fix(kubernetes): address PVC subPath review feedback Signed-off-by: mjamiv --- crates/openshell-core/src/driver_mounts.rs | 52 ++- crates/openshell-driver-docker/src/lib.rs | 24 +- crates/openshell-driver-docker/src/tests.rs | 33 ++ crates/openshell-driver-kubernetes/README.md | 19 +- .../openshell-driver-kubernetes/src/driver.rs | 321 ++++++++++++------ .../openshell-driver-podman/src/container.rs | 57 +++- docs/reference/sandbox-compute-drivers.mdx | 15 +- 7 files changed, 377 insertions(+), 144 deletions(-) diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index fa43edf53..2161fe12e 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -3,6 +3,7 @@ //! Shared validation helpers for driver-config mounts. +use std::collections::HashSet; use std::path::Path; /// `SELinux` relabelling mode for bind mounts. @@ -32,6 +33,11 @@ const RESERVED_MOUNT_TARGETS: &[&str] = &[ "/run/netns", ]; +/// Serde default helper for mount options that default to read-only. +pub fn default_true() -> bool { + true +} + /// Validate a non-empty driver mount source. pub fn validate_mount_source(source: &str, field: &str) -> Result<(), String> { if source.is_empty() { @@ -122,10 +128,27 @@ pub fn normalize_mount_target(target: &str) -> String { target.trim_end_matches('/').to_string() } -fn path_is_or_under(path: &Path, parent: &Path) -> bool { +/// Return true when `path` is exactly `parent` or is contained below it. +pub fn path_is_or_under(path: &Path, parent: &Path) -> bool { path == parent || path.starts_with(parent) } +/// Validate that already-normalized driver mount targets are unique. +pub fn validate_unique_mount_targets<'a>( + targets: impl IntoIterator, + driver_name: &str, +) -> Result<(), String> { + let mut seen = HashSet::new(); + for target in targets { + if !seen.insert(target) { + return Err(format!( + "duplicate {driver_name} driver_config mount target '{target}'" + )); + } + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -162,6 +185,33 @@ mod tests { validate_container_mount_target("/etc/openshell-tools").unwrap(); } + #[test] + fn path_is_or_under_matches_boundaries() { + assert!(path_is_or_under( + Path::new("/sandbox"), + Path::new("/sandbox") + )); + assert!(path_is_or_under( + Path::new("/sandbox/work"), + Path::new("/sandbox") + )); + assert!(!path_is_or_under( + Path::new("/sandbox-work"), + Path::new("/sandbox") + )); + } + + #[test] + fn unique_mount_targets_rejects_duplicates() { + let err = + validate_unique_mount_targets(["/sandbox/work", "/sandbox/work"], "test").unwrap_err(); + + assert_eq!( + err, + "duplicate test driver_config mount target '/sandbox/work'" + ); + } + #[test] fn mount_subpath_must_be_relative_without_parent_dirs() { assert!(validate_mount_subpath("project/a").is_ok()); diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 06d575a1e..260727fd8 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -49,7 +49,7 @@ use openshell_core::proto_struct::{ deserialize_optional_non_empty_string_list, struct_to_json_value, }; use openshell_core::{Config, Error, Result as CoreResult}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::io::Read; use std::net::{IpAddr, SocketAddr}; use std::path::{Path, PathBuf}; @@ -262,7 +262,7 @@ enum DockerDriverMountConfig { Bind { source: String, target: String, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, #[serde(default)] selinux_label: Option, @@ -270,7 +270,7 @@ enum DockerDriverMountConfig { Volume { source: String, target: String, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, #[serde(default)] subpath: Option, @@ -287,17 +287,13 @@ enum DockerDriverMountConfig { Image { source: String, target: String, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, #[serde(default)] subpath: Option, }, } -fn default_true() -> bool { - true -} - type WatchStream = Pin> + Send + 'static>>; @@ -1845,7 +1841,7 @@ fn validate_docker_driver_mounts( mounts: &[DockerDriverMountConfig], enable_bind_mounts: bool, ) -> Result<(), Status> { - let mut targets = HashSet::new(); + let mut targets = Vec::with_capacity(mounts.len()); for mount in mounts { let target = match mount { DockerDriverMountConfig::Bind { source, target, .. } => { @@ -1899,14 +1895,10 @@ fn validate_docker_driver_mounts( }; driver_mounts::validate_container_mount_target(target) .map_err(Status::failed_precondition)?; - let normalized_target = driver_mounts::normalize_mount_target(target); - if !targets.insert(normalized_target.clone()) { - return Err(Status::failed_precondition(format!( - "duplicate docker driver_config mount target '{normalized_target}'" - ))); - } + targets.push(driver_mounts::normalize_mount_target(target)); } - Ok(()) + driver_mounts::validate_unique_mount_targets(targets.iter().map(String::as_str), "docker") + .map_err(Status::failed_precondition) } fn validate_optional_positive_integral_i64( diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index 560d47de7..594308bce 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -755,6 +755,39 @@ fn driver_config_allows_explicit_writable_volume_mounts() { assert_eq!(mounts[0].read_only, Some(false)); } +#[test] +fn driver_config_rejects_duplicate_mount_targets() { + let mut sandbox = test_sandbox(); + sandbox + .spec + .as_mut() + .unwrap() + .template + .as_mut() + .unwrap() + .driver_config = Some(json_struct(serde_json::json!({ + "mounts": [ + { + "type": "volume", + "source": "work-nfs", + "target": "/sandbox/work" + }, + { + "type": "tmpfs", + "target": "/sandbox/work" + } + ] + }))); + + let err = build_container_create_body(&sandbox, &runtime_config()).unwrap_err(); + + assert_eq!(err.code(), tonic::Code::FailedPrecondition); + assert!( + err.message() + .contains("duplicate docker driver_config mount target") + ); +} + #[test] fn driver_config_rejects_bind_mounts_unless_enabled() { let mut sandbox = test_sandbox(); diff --git a/crates/openshell-driver-kubernetes/README.md b/crates/openshell-driver-kubernetes/README.md index 830679a61..1e75a6ab8 100644 --- a/crates/openshell-driver-kubernetes/README.md +++ b/crates/openshell-driver-kubernetes/README.md @@ -149,15 +149,16 @@ resource details. Use PVC volumes to mount existing Kubernetes PersistentVolumeClaims into the agent container. PVC volumes and mounts default to read-only unless -`read_only: false` is set explicitly. A read-only PVC volume cannot be mounted -read-write. The driver rejects duplicate volume names, mounts that reference -unknown volumes, non-normalized or protected mount paths, and absolute or -parent-traversing `sub_path` values. - -Any explicit driver-config mount under `/sandbox/` disables the driver's -default `/sandbox` workspace PVC injection for that sandbox. This keeps image -contents fresh while allowing selected durable data paths to come from an -external PVC. +`read_only: false` is set explicitly. Read-write access requires +`read_only: false` on both the PVC volume and each writable mount. The driver +rejects duplicate volume names, invalid DNS-1123 volume or PVC claim names, +mounts that reference unknown volumes, non-normalized or protected mount paths, +and absolute or parent-traversing `sub_path` values. + +Any explicit driver-config mount under `/sandbox` disables the driver's +default `/sandbox` workspace PVC injection for that sandbox. Only the explicit +mount paths persist through the external PVC; other `/sandbox` paths come from +the current sandbox image. ```shell openshell sandbox create \ diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index b8bee47e0..dd7a09307 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -36,6 +36,7 @@ use openshell_core::proto::compute::v1::{ use openshell_core::proto_struct::{struct_to_json_object, value_to_json}; use serde::Deserialize; use std::collections::{BTreeMap, HashSet}; +use std::path::Path; use std::pin::Pin; use std::sync::Arc; use std::time::Duration; @@ -130,29 +131,29 @@ impl KubernetesSandboxDriverConfig { }; let json = serde_json::Value::Object(struct_to_json_object(config)); - let config: Self = serde_json::from_value(json) + let mut config: Self = serde_json::from_value(json) .map_err(|err| format!("invalid kubernetes driver_config: {err}"))?; config - .validate() + .normalize() .map_err(|err| format!("invalid kubernetes driver_config: {err}"))?; Ok(config) } - fn validate(&self) -> Result<(), String> { - validate_kubernetes_driver_volumes(&self.volumes)?; - validate_kubernetes_driver_volume_mounts( + fn normalize(&mut self) -> Result<(), String> { + normalize_kubernetes_driver_volumes(&mut self.volumes)?; + normalize_kubernetes_driver_volume_mounts( &self.volumes, - &self.containers.agent.volume_mounts, + &mut self.containers.agent.volume_mounts, ) } fn has_explicit_sandbox_data_mount(&self) -> bool { - self.containers - .agent - .volume_mounts - .iter() - .filter_map(|mount| validate_kubernetes_mount_path(&mount.mount_path).ok()) - .any(|mount_path| path_is_or_under(&mount_path, WORKSPACE_MOUNT_PATH)) + self.containers.agent.volume_mounts.iter().any(|mount| { + driver_mounts::path_is_or_under( + Path::new(&mount.mount_path), + Path::new(WORKSPACE_MOUNT_PATH), + ) + }) } } @@ -196,7 +197,7 @@ struct KubernetesDriverVolumeConfig { #[serde(default, deny_unknown_fields)] struct KubernetesPersistentVolumeClaimConfig { claim_name: String, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, } @@ -215,7 +216,7 @@ struct KubernetesDriverVolumeMountConfig { name: String, mount_path: String, sub_path: Option, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, } @@ -230,34 +231,27 @@ impl Default for KubernetesDriverVolumeMountConfig { } } -fn default_true() -> bool { - true -} +const CLIENT_TLS_VOLUME_NAME: &str = "openshell-client-tls"; +const SERVICE_ACCOUNT_TOKEN_VOLUME_NAME: &str = "openshell-sa-token"; +const SERVICE_ACCOUNT_TOKEN_MOUNT_PATH: &str = "/var/run/secrets/openshell"; const KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES: &[&str] = &[ - "openshell-client-tls", - "openshell-sa-token", + CLIENT_TLS_VOLUME_NAME, + SERVICE_ACCOUNT_TOKEN_VOLUME_NAME, SPIFFE_WORKLOAD_API_VOLUME_NAME, SUPERVISOR_VOLUME_NAME, WORKSPACE_VOLUME_NAME, ]; -const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = &[ - "/etc/openshell", - "/etc/openshell-tls", - "/var/run/secrets/openshell", - "/run/netns", - "/spiffe-workload-api", - openshell_core::driver_utils::SUPERVISOR_CONTAINER_DIR, - openshell_core::driver_utils::SUPERVISOR_CONTAINER_BINARY, -]; +const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = + &[SERVICE_ACCOUNT_TOKEN_MOUNT_PATH, "/spiffe-workload-api"]; -fn validate_kubernetes_driver_volumes( - volumes: &[KubernetesDriverVolumeConfig], +fn normalize_kubernetes_driver_volumes( + volumes: &mut [KubernetesDriverVolumeConfig], ) -> Result<(), String> { let mut names = HashSet::new(); for volume in volumes { - let name = validate_kubernetes_reference_name(&volume.name, "volumes[].name")?; + let name = validate_kubernetes_dns1123_label(&volume.name, "volumes[].name")?; if KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES.contains(&name.as_str()) { return Err(format!( "volume name '{name}' is reserved for OpenShell-managed volumes" @@ -268,7 +262,8 @@ fn validate_kubernetes_driver_volumes( "duplicate kubernetes driver_config volume '{name}'" )); } - validate_kubernetes_reference_name( + volume.name = name; + volume.persistent_volume_claim.claim_name = validate_kubernetes_dns1123_label( &volume.persistent_volume_claim.claim_name, "volumes[].persistent_volume_claim.claim_name", )?; @@ -276,19 +271,21 @@ fn validate_kubernetes_driver_volumes( Ok(()) } -fn validate_kubernetes_driver_volume_mounts( +fn normalize_kubernetes_driver_volume_mounts( volumes: &[KubernetesDriverVolumeConfig], - volume_mounts: &[KubernetesDriverVolumeMountConfig], + volume_mounts: &mut [KubernetesDriverVolumeMountConfig], ) -> Result<(), String> { let mut volume_read_only = BTreeMap::new(); for volume in volumes { - let name = validate_kubernetes_reference_name(&volume.name, "volumes[].name")?; - volume_read_only.insert(name, volume.persistent_volume_claim.read_only); + volume_read_only.insert( + volume.name.clone(), + volume.persistent_volume_claim.read_only, + ); } - let mut mount_paths = HashSet::new(); + let mut mount_paths = Vec::with_capacity(volume_mounts.len()); for mount in volume_mounts { - let volume_name = validate_kubernetes_reference_name( + let volume_name = validate_kubernetes_dns1123_label( &mount.name, "containers.agent.volume_mounts[].name", )?; @@ -302,19 +299,20 @@ fn validate_kubernetes_driver_volume_mounts( "volume mount '{volume_name}' cannot set read_only=false because the PVC volume is read_only=true" )); } + mount.name = volume_name; let mount_path = validate_kubernetes_mount_path(&mount.mount_path)?; - if !mount_paths.insert(mount_path.clone()) { - return Err(format!( - "duplicate kubernetes driver_config volume mount path '{mount_path}'" - )); - } + mount.mount_path.clone_from(&mount_path); + mount_paths.push(mount_path); - if let Some(sub_path) = mount.sub_path.as_ref() { + if let Some(sub_path) = mount.sub_path.as_mut() { driver_mounts::validate_mount_subpath(sub_path)?; } } - Ok(()) + driver_mounts::validate_unique_mount_targets( + mount_paths.iter().map(String::as_str), + "kubernetes", + ) } fn validate_kubernetes_driver_runtime_mounts( @@ -326,8 +324,8 @@ fn validate_kubernetes_driver_runtime_mounts( }; let spiffe_mount_path = spiffe_socket_mount_path(socket_path); for mount in &config.containers.agent.volume_mounts { - let mount_path = validate_kubernetes_mount_path(&mount.mount_path)?; - if mount_path_conflicts_with_protected_path(&mount_path, &spiffe_mount_path) { + let mount_path = mount.mount_path.as_str(); + if mount_path_conflicts_with_protected_path(mount_path, &spiffe_mount_path) { return Err(format!( "mount path '{mount_path}' conflicts with reserved OpenShell path '{spiffe_mount_path}'" )); @@ -336,14 +334,27 @@ fn validate_kubernetes_driver_runtime_mounts( Ok(()) } -fn validate_kubernetes_reference_name(value: &str, field: &str) -> Result { - let normalized = driver_mounts::validate_mount_source(value, field)?; - if normalized != value { +fn validate_kubernetes_dns1123_label(value: &str, field: &str) -> Result { + driver_mounts::validate_mount_source(value, field)?; + let normalized = value; + let is_dns1123_label = normalized.len() <= 63 + && normalized + .bytes() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-') + && normalized + .as_bytes() + .first() + .is_some_and(u8::is_ascii_alphanumeric) + && normalized + .as_bytes() + .last() + .is_some_and(u8::is_ascii_alphanumeric); + if !is_dns1123_label { return Err(format!( - "{field} must not contain leading or trailing whitespace" + "{field} must be a DNS-1123 label: use lowercase alphanumeric characters or '-', start and end with an alphanumeric character, and use at most 63 characters" )); } - Ok(normalized) + Ok(normalized.to_string()) } fn validate_kubernetes_mount_path(mount_path: &str) -> Result { @@ -363,7 +374,8 @@ fn validate_kubernetes_mount_path(mount_path: &str) -> Result { ); } - let mount_path = driver_mounts::validate_container_mount_target(raw)?; + driver_mounts::validate_container_mount_target(raw)?; + let mount_path = driver_mounts::normalize_mount_target(raw); for protected_path in KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS { if mount_path_conflicts_with_protected_path(&mount_path, protected_path) { return Err(format!( @@ -375,14 +387,8 @@ fn validate_kubernetes_mount_path(mount_path: &str) -> Result { } fn mount_path_conflicts_with_protected_path(mount_path: &str, protected_path: &str) -> bool { - path_is_or_under(mount_path, protected_path) || path_is_or_under(protected_path, mount_path) -} - -fn path_is_or_under(path: &str, parent: &str) -> bool { - path == parent - || path - .strip_prefix(parent) - .is_some_and(|rest| rest.starts_with('/')) + driver_mounts::path_is_or_under(Path::new(mount_path), Path::new(protected_path)) + || driver_mounts::path_is_or_under(Path::new(protected_path), Path::new(mount_path)) } fn kubernetes_driver_volume_to_k8s(volume: &KubernetesDriverVolumeConfig) -> serde_json::Value { @@ -400,15 +406,11 @@ fn kubernetes_driver_volume_mount_to_k8s( ) -> serde_json::Value { let mut value = serde_json::json!({ "name": mount.name.as_str(), - "mountPath": validate_kubernetes_mount_path(&mount.mount_path) - .expect("validated Kubernetes driver_config mount_path"), + "mountPath": mount.mount_path.as_str(), "readOnly": mount.read_only, }); if let Some(sub_path) = mount.sub_path.as_ref() { - value["subPath"] = serde_json::json!( - driver_mounts::validate_mount_subpath(sub_path) - .expect("validated Kubernetes driver_config sub_path") - ); + value["subPath"] = serde_json::json!(sub_path); } value } @@ -799,7 +801,7 @@ impl KubernetesComputeDriver { #[allow(clippy::similar_names)] pub async fn create_sandbox(&self, sandbox: &Sandbox) -> Result<(), KubernetesDriverError> { - let _ = self + let driver_config = self .validate_driver_config_for_sandbox(sandbox) .map_err(KubernetesDriverError::InvalidArgument)?; let gpu_requirements = sandbox @@ -887,7 +889,7 @@ impl KubernetesComputeDriver { ..Default::default() }; - obj.data = sandbox_to_k8s_spec(sandbox.spec.as_ref(), ¶ms); + obj.data = sandbox_to_k8s_spec(sandbox.spec.as_ref(), ¶ms, &driver_config); match tokio::time::timeout( KUBE_API_TIMEOUT, agent_sandbox_api.api.create(&PostParams::default(), &obj), @@ -2163,14 +2165,10 @@ fn spec_pod_env(spec: Option<&SandboxSpec>) -> std::collections::HashMap KubernetesSandboxDriverConfig { - KubernetesSandboxDriverConfig::from_template(template) - .expect("validated Kubernetes driver_config") -} - fn sandbox_to_k8s_spec( spec: Option<&SandboxSpec>, params: &SandboxPodParams<'_>, + driver_config: &KubernetesSandboxDriverConfig, ) -> serde_json::Value { let mut root = serde_json::Map::new(); @@ -2179,10 +2177,7 @@ fn sandbox_to_k8s_spec( // ownership of workspace persistence. // We need this flag before building the podTemplate because the workspace // persistence transforms are applied inside sandbox_template_to_k8s. - let template = spec.and_then(|s| s.template.as_ref()); - let user_has_explicit_workspace_mount = template - .map(kubernetes_driver_config) - .is_some_and(|config| config.has_explicit_sandbox_data_mount()); + let user_has_explicit_workspace_mount = driver_config.has_explicit_sandbox_data_mount(); let inject_workspace = !user_has_explicit_workspace_mount; if let Some(spec) = spec { @@ -2190,10 +2185,11 @@ fn sandbox_to_k8s_spec( if let Some(template) = spec.template.as_ref() { root.insert( "podTemplate".to_string(), - sandbox_template_to_k8s_with_gpu_requirements( + sandbox_template_to_k8s_with_validated_config( template, driver_gpu_requirements(spec.resource_requirements.as_ref()), &pod_env, + driver_config, inject_workspace, params, ), @@ -2219,10 +2215,11 @@ fn sandbox_to_k8s_spec( let pod_env = spec_pod_env(spec); root.insert( "podTemplate".to_string(), - sandbox_template_to_k8s_with_gpu_requirements( + sandbox_template_to_k8s_with_validated_config( &SandboxTemplate::default(), driver_gpu_requirements(spec.and_then(|s| s.resource_requirements.as_ref())), &pod_env, + driver_config, inject_workspace, params, ), @@ -2243,15 +2240,19 @@ fn sandbox_template_to_k8s( params: &SandboxPodParams<'_>, ) -> serde_json::Value { let gpu_requirements = gpu.then_some(GpuResourceRequirements { count: None }); - sandbox_template_to_k8s_with_gpu_requirements( + let driver_config = KubernetesSandboxDriverConfig::from_template(template) + .expect("test Kubernetes driver_config should be valid"); + sandbox_template_to_k8s_with_validated_config( template, gpu_requirements.as_ref(), spec_environment, + &driver_config, inject_workspace, params, ) } +#[cfg(test)] fn sandbox_template_to_k8s_with_gpu_requirements( template: &SandboxTemplate, gpu_requirements: Option<&GpuResourceRequirements>, @@ -2259,8 +2260,26 @@ fn sandbox_template_to_k8s_with_gpu_requirements( inject_workspace: bool, params: &SandboxPodParams<'_>, ) -> serde_json::Value { - let driver_config = kubernetes_driver_config(template); + let driver_config = KubernetesSandboxDriverConfig::from_template(template) + .expect("test Kubernetes driver_config should be valid"); + sandbox_template_to_k8s_with_validated_config( + template, + gpu_requirements, + spec_environment, + &driver_config, + inject_workspace, + params, + ) +} +fn sandbox_template_to_k8s_with_validated_config( + template: &SandboxTemplate, + gpu_requirements: Option<&GpuResourceRequirements>, + spec_environment: &std::collections::HashMap, + driver_config: &KubernetesSandboxDriverConfig, + inject_workspace: bool, + params: &SandboxPodParams<'_>, +) -> serde_json::Value { let mut metadata = serde_json::Map::new(); let mut pod_labels = template .labels @@ -2424,7 +2443,7 @@ fn sandbox_template_to_k8s_with_gpu_requirements( let mut volume_mounts: Vec = Vec::new(); if !params.client_tls_secret_name.is_empty() { volume_mounts.push(serde_json::json!({ - "name": "openshell-client-tls", + "name": CLIENT_TLS_VOLUME_NAME, "mountPath": "/etc/openshell-tls/client", "readOnly": true })); @@ -2437,8 +2456,8 @@ fn sandbox_template_to_k8s_with_gpu_requirements( })); } volume_mounts.push(serde_json::json!({ - "name": "openshell-sa-token", - "mountPath": "/var/run/secrets/openshell", + "name": SERVICE_ACCOUNT_TOKEN_VOLUME_NAME, + "mountPath": SERVICE_ACCOUNT_TOKEN_MOUNT_PATH, "readOnly": true, })); volume_mounts.extend( @@ -2474,7 +2493,7 @@ fn sandbox_template_to_k8s_with_gpu_requirements( SupervisorTopology::Sidecar => 0o440, }; volumes.push(serde_json::json!({ - "name": "openshell-client-tls", + "name": CLIENT_TLS_VOLUME_NAME, "secret": { "secretName": params.client_tls_secret_name, "defaultMode": client_tls_default_mode @@ -2500,7 +2519,7 @@ fn sandbox_template_to_k8s_with_gpu_requirements( SupervisorTopology::Sidecar => 0o440, }; volumes.push(serde_json::json!({ - "name": "openshell-sa-token", + "name": SERVICE_ACCOUNT_TOKEN_VOLUME_NAME, "projected": { "sources": [{ "serviceAccountToken": { @@ -2856,9 +2875,9 @@ fn provider_spiffe_socket_path<'a>(params: &'a SandboxPodParams<'a>) -> Option<& } fn spiffe_socket_mount_path(socket_path: &str) -> String { - std::path::Path::new(socket_path) + Path::new(socket_path) .parent() - .and_then(std::path::Path::to_str) + .and_then(Path::to_str) .filter(|path| !path.is_empty() && *path != "/") .expect("provider SPIFFE socket path should be validated before pod rendering") .to_string() @@ -3030,6 +3049,20 @@ mod tests { } } + fn sandbox_to_k8s_spec_for_test( + spec: Option<&SandboxSpec>, + params: &SandboxPodParams<'_>, + ) -> serde_json::Value { + let driver_config = spec + .and_then(|spec| spec.template.as_ref()) + .map(KubernetesSandboxDriverConfig::from_template) + .transpose() + .expect("test Kubernetes driver_config should be valid") + .unwrap_or_default(); + + sandbox_to_k8s_spec(spec, params, &driver_config) + } + fn kube_api_error(code: u16, message: &str) -> KubeError { KubeError::Api(kube::core::ErrorResponse { status: if code == 404 { @@ -3152,7 +3185,7 @@ mod tests { ..SandboxSpec::default() }; - let cr = sandbox_to_k8s_spec(Some(&spec), &SandboxPodParams::default()); + let cr = sandbox_to_k8s_spec_for_test(Some(&spec), &SandboxPodParams::default()); let pod_template = &cr["spec"]["podTemplate"]; let volumes = pod_template["spec"]["volumes"] @@ -3289,6 +3322,72 @@ mod tests { assert!(err.contains("duplicate kubernetes driver_config volume")); } + #[test] + fn driver_config_rejects_duplicate_pvc_volume_mount_targets() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [ + { + "name": "user-data", + "mount_path": "/sandbox/.openshell/workspace" + }, + { + "name": "user-data", + "mount_path": "/sandbox/.openshell/workspace" + } + ] + } + } + }))), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + + assert!(err.contains("duplicate kubernetes driver_config mount target")); + } + + #[test] + fn driver_config_rejects_invalid_dns1123_volume_and_claim_names() { + for (field, config) in [ + ( + "volumes[].name", + serde_json::json!({ + "volumes": [{ + "name": "User_Data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }] + }), + ), + ( + "volumes[].persistent_volume_claim.claim_name", + serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc.user.data"} + }] + }), + ), + ] { + let template = SandboxTemplate { + driver_config: Some(json_struct(config)), + ..SandboxTemplate::default() + }; + + let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); + assert!( + err.contains(field) && err.contains("DNS-1123 label"), + "expected invalid {field} to fail DNS-1123 validation, got {err}" + ); + } + } + #[test] fn driver_config_rejects_mounts_referencing_unknown_volumes() { let template = SandboxTemplate { @@ -3460,8 +3559,8 @@ mod tests { #[test] fn driver_config_rejects_reserved_kubernetes_volume_names() { for volume_name in [ - "openshell-client-tls", - "openshell-sa-token", + CLIENT_TLS_VOLUME_NAME, + SERVICE_ACCOUNT_TOKEN_VOLUME_NAME, SPIFFE_WORKLOAD_API_VOLUME_NAME, SUPERVISOR_VOLUME_NAME, WORKSPACE_VOLUME_NAME, @@ -3484,6 +3583,36 @@ mod tests { } } + #[test] + fn reserved_kubernetes_volume_names_cover_managed_pod_volumes() { + let params = SandboxPodParams { + client_tls_secret_name: "openshell-client-tls-secret", + provider_spiffe_enabled: true, + provider_spiffe_workload_api_socket_path: "/spiffe-workload-api/spire-agent.sock", + ..SandboxPodParams::default() + }; + let pod_template = sandbox_template_to_k8s( + &SandboxTemplate::default(), + false, + &std::collections::HashMap::new(), + true, + ¶ms, + ); + let volume_names = pod_template["spec"]["volumes"] + .as_array() + .expect("volumes should exist") + .iter() + .filter_map(|volume| volume["name"].as_str()) + .collect::>(); + + for volume_name in volume_names { + assert!( + KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES.contains(&volume_name), + "managed volume {volume_name:?} should be reserved" + ); + } + } + #[test] fn driver_config_rejects_runtime_provider_spiffe_mount_path() { let template = SandboxTemplate { @@ -4638,7 +4767,7 @@ mod tests { .expect("volumes should exist"); let tls_vol = volumes .iter() - .find(|v| v["name"] == "openshell-client-tls") + .find(|v| v["name"] == CLIENT_TLS_VOLUME_NAME) .expect("TLS volume should exist"); assert_eq!( tls_vol["secret"]["defaultMode"], @@ -5157,7 +5286,7 @@ mod tests { && volume["csi"]["driver"] == "csi.spiffe.io" })); assert!(volumes.iter().any(|volume| { - volume["name"] == "openshell-sa-token" + volume["name"] == SERVICE_ACCOUNT_TOKEN_VOLUME_NAME && volume["projected"]["sources"][0]["serviceAccountToken"]["path"] == "token" })); @@ -5210,7 +5339,7 @@ mod tests { log_level: "debug".to_string(), ..SandboxSpec::default() }; - let cr = sandbox_to_k8s_spec(Some(&spec), &SandboxPodParams::default()); + let cr = sandbox_to_k8s_spec_for_test(Some(&spec), &SandboxPodParams::default()); let env = cr["spec"]["podTemplate"]["spec"]["containers"][0]["env"] .as_array() .unwrap(); @@ -5237,7 +5366,7 @@ mod tests { )]), ..SandboxSpec::default() }; - let cr = sandbox_to_k8s_spec(Some(&spec), &SandboxPodParams::default()); + let cr = sandbox_to_k8s_spec_for_test(Some(&spec), &SandboxPodParams::default()); let env = cr["spec"]["podTemplate"]["spec"]["containers"][0]["env"] .as_array() .unwrap(); diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 1401f9e7c..16b3d912a 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -13,7 +13,7 @@ use openshell_core::proto_struct::deserialize_optional_non_empty_string_list; use openshell_core::{driver_mounts, proto_struct}; use serde::Serialize; use serde_json::Value; -use std::collections::{BTreeMap, HashSet}; +use std::collections::BTreeMap; #[cfg(target_os = "linux")] use std::path::Path; @@ -108,7 +108,7 @@ enum PodmanDriverMountConfig { Bind { source: String, target: String, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, #[serde(default)] selinux_label: Option, @@ -116,7 +116,7 @@ enum PodmanDriverMountConfig { Volume { source: String, target: String, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, #[serde(default)] subpath: Option, @@ -133,17 +133,13 @@ enum PodmanDriverMountConfig { Image { source: String, target: String, - #[serde(default = "default_true")] + #[serde(default = "driver_mounts::default_true")] read_only: bool, #[serde(default)] subpath: Option, }, } -fn default_true() -> bool { - true -} - /// Build a Podman container name from the sandbox name. #[must_use] pub fn container_name(sandbox_name: &str) -> String { @@ -664,7 +660,7 @@ fn validate_podman_driver_mounts( mounts: &[PodmanDriverMountConfig], enable_bind_mounts: bool, ) -> Result<(), String> { - let mut targets = HashSet::new(); + let mut targets = Vec::with_capacity(mounts.len()); for mount in mounts { let target = match mount { PodmanDriverMountConfig::Bind { source, target, .. } => { @@ -710,14 +706,9 @@ fn validate_podman_driver_mounts( } }; driver_mounts::validate_container_mount_target(target)?; - let normalized_target = driver_mounts::normalize_mount_target(target); - if !targets.insert(normalized_target.clone()) { - return Err(format!( - "duplicate podman driver_config mount target '{normalized_target}'" - )); - } + targets.push(driver_mounts::normalize_mount_target(target)); } - Ok(()) + driver_mounts::validate_unique_mount_targets(targets.iter().map(String::as_str), "podman") } fn reject_subpath(subpath: Option<&str>, mount_type: &str) -> Result<(), String> { @@ -2002,6 +1993,40 @@ mod tests { })); } + #[test] + fn driver_config_rejects_duplicate_mount_targets() { + use openshell_core::proto::compute::v1::{DriverSandboxSpec, DriverSandboxTemplate}; + + let mut sandbox = test_sandbox("test-id", "test-name"); + sandbox.spec = Some(DriverSandboxSpec { + template: Some(DriverSandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "mounts": [ + { + "type": "volume", + "source": "work-nfs", + "target": "/sandbox/work" + }, + { + "type": "tmpfs", + "target": "/sandbox/work" + } + ] + }))), + ..Default::default() + }), + ..Default::default() + }); + let config = test_config(); + + let err = try_build_container_spec_with_token(&sandbox, &config, None).unwrap_err(); + + assert!( + err.to_string() + .contains("duplicate podman driver_config mount target") + ); + } + #[test] fn driver_config_rejects_bind_mounts_unless_enabled() { use openshell_core::proto::compute::v1::{DriverSandboxSpec, DriverSandboxTemplate}; diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index 5e3cb157e..eb40f22a5 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -391,8 +391,8 @@ Kubernetes PVC mount schema: | Field | Description | |---|---| -| `volumes[].name` | Pod volume name. It must be unique and must not use OpenShell-managed volume names. | -| `volumes[].persistent_volume_claim.claim_name` | Existing PVC name in the sandbox namespace. | +| `volumes[].name` | Pod volume name. It must be a DNS-1123 label, unique, and not use OpenShell-managed volume names. | +| `volumes[].persistent_volume_claim.claim_name` | Existing PVC name in the sandbox namespace. It must be a DNS-1123 label. | | `volumes[].persistent_volume_claim.read_only` | Optional. Defaults to `true`. Set `false` to allow read-write mounts. | | `containers.agent.volume_mounts[].name` | References a volume declared in `volumes`. | | `containers.agent.volume_mounts[].mount_path` | Absolute, normalized container path for the agent mount. | @@ -401,10 +401,13 @@ Kubernetes PVC mount schema: OpenShell rejects duplicate volume names, mounts that reference unknown volumes, protected mount targets, and mounts that replace OpenShell TLS, supervisor, -ServiceAccount token, or SPIFFE paths. Any driver-config mount below -`/sandbox/` disables the default `/sandbox` workspace PVC injection for that -sandbox, so image-backed files outside the explicit mounts come from the current -sandbox image. +ServiceAccount token, or SPIFFE paths. Read-write PVC access requires +`read_only: false` on both the PVC volume and each writable mount. + +Any driver-config mount under `/sandbox` disables the default `/sandbox` +workspace PVC injection for that sandbox. Only the explicit mount paths persist +through the external PVC; other `/sandbox` paths come from the current sandbox +image. ## Sandbox User Identity From 30ad385363f4fadf4169effd44ae8e2ea6b96668 Mon Sep 17 00:00:00 2001 From: mjamiv Date: Wed, 1 Jul 2026 13:11:58 +0000 Subject: [PATCH 4/5] fix(kubernetes): address PVC config review follow-up Signed-off-by: mjamiv --- crates/openshell-core/src/driver_mounts.rs | 5 - crates/openshell-driver-docker/src/lib.rs | 10 +- crates/openshell-driver-kubernetes/README.md | 12 +- .../openshell-driver-kubernetes/src/driver.rs | 212 ++++++++++-------- .../openshell-driver-podman/src/container.rs | 10 +- docs/reference/sandbox-compute-drivers.mdx | 2 +- 6 files changed, 142 insertions(+), 109 deletions(-) diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index 2161fe12e..8843883ff 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -33,11 +33,6 @@ const RESERVED_MOUNT_TARGETS: &[&str] = &[ "/run/netns", ]; -/// Serde default helper for mount options that default to read-only. -pub fn default_true() -> bool { - true -} - /// Validate a non-empty driver mount source. pub fn validate_mount_source(source: &str, field: &str) -> Result<(), String> { if source.is_empty() { diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 260727fd8..f5c9fc88a 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -262,7 +262,7 @@ enum DockerDriverMountConfig { Bind { source: String, target: String, - #[serde(default = "driver_mounts::default_true")] + #[serde(default = "default_true")] read_only: bool, #[serde(default)] selinux_label: Option, @@ -270,7 +270,7 @@ enum DockerDriverMountConfig { Volume { source: String, target: String, - #[serde(default = "driver_mounts::default_true")] + #[serde(default = "default_true")] read_only: bool, #[serde(default)] subpath: Option, @@ -287,13 +287,17 @@ enum DockerDriverMountConfig { Image { source: String, target: String, - #[serde(default = "driver_mounts::default_true")] + #[serde(default = "default_true")] read_only: bool, #[serde(default)] subpath: Option, }, } +fn default_true() -> bool { + true +} + type WatchStream = Pin> + Send + 'static>>; diff --git a/crates/openshell-driver-kubernetes/README.md b/crates/openshell-driver-kubernetes/README.md index 1e75a6ab8..96e54ad44 100644 --- a/crates/openshell-driver-kubernetes/README.md +++ b/crates/openshell-driver-kubernetes/README.md @@ -117,13 +117,13 @@ nested schema and currently accepts: - `pod.priority_class_name` - `containers.agent.resources.requests` - `containers.agent.resources.limits` -- `volumes[].name` -- `volumes[].persistent_volume_claim.claim_name` -- `volumes[].persistent_volume_claim.read_only` - `containers.agent.volume_mounts[].name` - `containers.agent.volume_mounts[].mount_path` - `containers.agent.volume_mounts[].sub_path` - `containers.agent.volume_mounts[].read_only` +- `volumes[].name` +- `volumes[].persistent_volume_claim.claim_name` +- `volumes[].persistent_volume_claim.read_only` Nested keys inside the `kubernetes` block use snake_case. The top-level `driver_config` envelope is keyed by driver names, so `kubernetes` is not part @@ -151,9 +151,9 @@ Use PVC volumes to mount existing Kubernetes PersistentVolumeClaims into the agent container. PVC volumes and mounts default to read-only unless `read_only: false` is set explicitly. Read-write access requires `read_only: false` on both the PVC volume and each writable mount. The driver -rejects duplicate volume names, invalid DNS-1123 volume or PVC claim names, -mounts that reference unknown volumes, non-normalized or protected mount paths, -and absolute or parent-traversing `sub_path` values. +rejects duplicate volume names, invalid DNS-1123 volume labels or PVC claim +subdomain names, mounts that reference unknown volumes, non-normalized or +protected mount paths, and absolute or parent-traversing `sub_path` values. Any explicit driver-config mount under `/sandbox` disables the driver's default `/sandbox` workspace PVC injection for that sandbox. Only the explicit diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index dd7a09307..afd9f7464 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -113,37 +113,25 @@ struct KubernetesSandboxDriverConfig { } impl KubernetesSandboxDriverConfig { - fn from_sandbox(sandbox: &Sandbox) -> Result { - let Some(template) = sandbox - .spec - .as_ref() - .and_then(|spec| spec.template.as_ref()) - else { - return Ok(Self::default()); - }; - - Self::from_template(template) - } - fn from_template(template: &SandboxTemplate) -> Result { let Some(config) = template.driver_config.as_ref() else { return Ok(Self::default()); }; let json = serde_json::Value::Object(struct_to_json_object(config)); - let mut config: Self = serde_json::from_value(json) + let config: Self = serde_json::from_value(json) .map_err(|err| format!("invalid kubernetes driver_config: {err}"))?; config - .normalize() + .validate() .map_err(|err| format!("invalid kubernetes driver_config: {err}"))?; Ok(config) } - fn normalize(&mut self) -> Result<(), String> { - normalize_kubernetes_driver_volumes(&mut self.volumes)?; - normalize_kubernetes_driver_volume_mounts( + fn validate(&self) -> Result<(), String> { + validate_kubernetes_driver_volumes(&self.volumes)?; + validate_kubernetes_driver_volume_mounts( &self.volumes, - &mut self.containers.agent.volume_mounts, + &self.containers.agent.volume_mounts, ) } @@ -197,7 +185,6 @@ struct KubernetesDriverVolumeConfig { #[serde(default, deny_unknown_fields)] struct KubernetesPersistentVolumeClaimConfig { claim_name: String, - #[serde(default = "driver_mounts::default_true")] read_only: bool, } @@ -216,7 +203,6 @@ struct KubernetesDriverVolumeMountConfig { name: String, mount_path: String, sub_path: Option, - #[serde(default = "driver_mounts::default_true")] read_only: bool, } @@ -246,24 +232,24 @@ const KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES: &[&str] = &[ const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = &[SERVICE_ACCOUNT_TOKEN_MOUNT_PATH, "/spiffe-workload-api"]; -fn normalize_kubernetes_driver_volumes( - volumes: &mut [KubernetesDriverVolumeConfig], +fn validate_kubernetes_driver_volumes( + volumes: &[KubernetesDriverVolumeConfig], ) -> Result<(), String> { let mut names = HashSet::new(); for volume in volumes { - let name = validate_kubernetes_dns1123_label(&volume.name, "volumes[].name")?; - if KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES.contains(&name.as_str()) { + validate_kubernetes_dns1123_label(&volume.name, "volumes[].name")?; + let name = volume.name.as_str(); + if KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES.contains(&name) { return Err(format!( "volume name '{name}' is reserved for OpenShell-managed volumes" )); } - if !names.insert(name.clone()) { + if !names.insert(name) { return Err(format!( "duplicate kubernetes driver_config volume '{name}'" )); } - volume.name = name; - volume.persistent_volume_claim.claim_name = validate_kubernetes_dns1123_label( + validate_kubernetes_dns1123_subdomain( &volume.persistent_volume_claim.claim_name, "volumes[].persistent_volume_claim.claim_name", )?; @@ -271,25 +257,23 @@ fn normalize_kubernetes_driver_volumes( Ok(()) } -fn normalize_kubernetes_driver_volume_mounts( +fn validate_kubernetes_driver_volume_mounts( volumes: &[KubernetesDriverVolumeConfig], - volume_mounts: &mut [KubernetesDriverVolumeMountConfig], + volume_mounts: &[KubernetesDriverVolumeMountConfig], ) -> Result<(), String> { let mut volume_read_only = BTreeMap::new(); for volume in volumes { volume_read_only.insert( - volume.name.clone(), + volume.name.as_str(), volume.persistent_volume_claim.read_only, ); } let mut mount_paths = Vec::with_capacity(volume_mounts.len()); for mount in volume_mounts { - let volume_name = validate_kubernetes_dns1123_label( - &mount.name, - "containers.agent.volume_mounts[].name", - )?; - let Some(volume_is_read_only) = volume_read_only.get(&volume_name) else { + validate_kubernetes_dns1123_label(&mount.name, "containers.agent.volume_mounts[].name")?; + let volume_name = mount.name.as_str(); + let Some(volume_is_read_only) = volume_read_only.get(volume_name) else { return Err(format!( "volume mount references unknown kubernetes driver_config volume '{volume_name}'" )); @@ -299,20 +283,15 @@ fn normalize_kubernetes_driver_volume_mounts( "volume mount '{volume_name}' cannot set read_only=false because the PVC volume is read_only=true" )); } - mount.name = volume_name; - let mount_path = validate_kubernetes_mount_path(&mount.mount_path)?; - mount.mount_path.clone_from(&mount_path); - mount_paths.push(mount_path); + validate_kubernetes_mount_path(&mount.mount_path)?; + mount_paths.push(mount.mount_path.as_str()); - if let Some(sub_path) = mount.sub_path.as_mut() { + if let Some(sub_path) = mount.sub_path.as_ref() { driver_mounts::validate_mount_subpath(sub_path)?; } } - driver_mounts::validate_unique_mount_targets( - mount_paths.iter().map(String::as_str), - "kubernetes", - ) + driver_mounts::validate_unique_mount_targets(mount_paths, "kubernetes") } fn validate_kubernetes_driver_runtime_mounts( @@ -322,6 +301,10 @@ fn validate_kubernetes_driver_runtime_mounts( let Some(socket_path) = provider_spiffe_workload_api_socket_path else { return Ok(()); }; + // The SPIFFE Workload API mount path is derived from gateway runtime + // configuration, not from the sandbox template. Static protected paths are + // checked during `from_template`; this dynamic conflict can only be checked + // once the driver runtime config is available. let spiffe_mount_path = spiffe_socket_mount_path(socket_path); for mount in &config.containers.agent.volume_mounts { let mount_path = mount.mount_path.as_str(); @@ -334,36 +317,59 @@ fn validate_kubernetes_driver_runtime_mounts( Ok(()) } -fn validate_kubernetes_dns1123_label(value: &str, field: &str) -> Result { - driver_mounts::validate_mount_source(value, field)?; - let normalized = value; - let is_dns1123_label = normalized.len() <= 63 - && normalized +fn validate_kubernetes_name_text(value: &str, field: &str) -> Result<(), String> { + if value != value.trim() { + return Err(format!( + "{field} must not contain leading or trailing whitespace" + )); + } + driver_mounts::validate_mount_source(value, field) +} + +fn is_kubernetes_dns1123_label(value: &str) -> bool { + !value.is_empty() + && value.len() <= 63 + && value .bytes() .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-') - && normalized + && value .as_bytes() .first() .is_some_and(u8::is_ascii_alphanumeric) - && normalized + && value .as_bytes() .last() - .is_some_and(u8::is_ascii_alphanumeric); - if !is_dns1123_label { + .is_some_and(u8::is_ascii_alphanumeric) +} + +fn validate_kubernetes_dns1123_label(value: &str, field: &str) -> Result<(), String> { + validate_kubernetes_name_text(value, field)?; + if !is_kubernetes_dns1123_label(value) { return Err(format!( "{field} must be a DNS-1123 label: use lowercase alphanumeric characters or '-', start and end with an alphanumeric character, and use at most 63 characters" )); } - Ok(normalized.to_string()) + Ok(()) } -fn validate_kubernetes_mount_path(mount_path: &str) -> Result { - let raw = mount_path.trim(); - if raw != mount_path { +fn validate_kubernetes_dns1123_subdomain(value: &str, field: &str) -> Result<(), String> { + validate_kubernetes_name_text(value, field)?; + let is_dns1123_subdomain = + value.len() <= 253 && value.split('.').all(is_kubernetes_dns1123_label); + if !is_dns1123_subdomain { + return Err(format!( + "{field} must be a DNS-1123 subdomain: use lowercase alphanumeric characters, '-' or '.', start and end with an alphanumeric character, and use at most 253 characters" + )); + } + Ok(()) +} + +fn validate_kubernetes_mount_path(mount_path: &str) -> Result<(), String> { + if mount_path != mount_path.trim() { return Err("mount_path must not contain leading or trailing whitespace".to_string()); } - if raw != "/" - && raw + if mount_path != "/" + && mount_path .split('/') .skip(1) .any(|segment| segment.is_empty() || segment == ".") @@ -374,16 +380,15 @@ fn validate_kubernetes_mount_path(mount_path: &str) -> Result { ); } - driver_mounts::validate_container_mount_target(raw)?; - let mount_path = driver_mounts::normalize_mount_target(raw); + driver_mounts::validate_container_mount_target(mount_path)?; for protected_path in KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS { - if mount_path_conflicts_with_protected_path(&mount_path, protected_path) { + if mount_path_conflicts_with_protected_path(mount_path, protected_path) { return Err(format!( "mount path '{mount_path}' conflicts with reserved OpenShell path '{protected_path}'" )); } } - Ok(mount_path) + Ok(()) } fn mount_path_conflicts_with_protected_path(mount_path: &str, protected_path: &str) -> bool { @@ -532,16 +537,14 @@ impl KubernetesComputeDriver { &self, sandbox: &Sandbox, ) -> Result { - let config = KubernetesSandboxDriverConfig::from_sandbox(sandbox)?; - validate_kubernetes_driver_runtime_mounts( - &config, + kubernetes_driver_config_for_spec( + sandbox.spec.as_ref(), self.config.provider_spiffe_enabled().then_some( self.config .provider_spiffe_workload_api_socket_path .as_str(), ), - )?; - Ok(config) + ) } fn agent_sandbox_api(&self, client: Client, sandbox_api_version: &str) -> AgentSandboxApi { @@ -801,9 +804,6 @@ impl KubernetesComputeDriver { #[allow(clippy::similar_names)] pub async fn create_sandbox(&self, sandbox: &Sandbox) -> Result<(), KubernetesDriverError> { - let driver_config = self - .validate_driver_config_for_sandbox(sandbox) - .map_err(KubernetesDriverError::InvalidArgument)?; let gpu_requirements = sandbox .spec .as_ref() @@ -862,6 +862,8 @@ impl KubernetesComputeDriver { }; validate_sidecar_proxy_identity(¶ms)?; + let data = sandbox_to_k8s_spec(sandbox.spec.as_ref(), ¶ms) + .map_err(KubernetesDriverError::InvalidArgument)?; let mut obj = DynamicObject::new(name, &agent_sandbox_api.resource); // Copy only the SCC-related annotations onto the Sandbox CR for // traceability. Copying the full namespace annotation map exposes @@ -889,7 +891,7 @@ impl KubernetesComputeDriver { ..Default::default() }; - obj.data = sandbox_to_k8s_spec(sandbox.spec.as_ref(), ¶ms, &driver_config); + obj.data = data; match tokio::time::timeout( KUBE_API_TIMEOUT, agent_sandbox_api.api.create(&PostParams::default(), &obj), @@ -2165,11 +2167,25 @@ fn spec_pod_env(spec: Option<&SandboxSpec>) -> std::collections::HashMap, + provider_spiffe_workload_api_socket_path: Option<&str>, +) -> Result { + let config = spec + .and_then(|spec| spec.template.as_ref()) + .map(KubernetesSandboxDriverConfig::from_template) + .transpose()? + .unwrap_or_default(); + validate_kubernetes_driver_runtime_mounts(&config, provider_spiffe_workload_api_socket_path)?; + Ok(config) +} + fn sandbox_to_k8s_spec( spec: Option<&SandboxSpec>, params: &SandboxPodParams<'_>, - driver_config: &KubernetesSandboxDriverConfig, -) -> serde_json::Value { +) -> Result { + let driver_config = + kubernetes_driver_config_for_spec(spec, provider_spiffe_socket_path(params))?; let mut root = serde_json::Map::new(); // Determine early whether OpenShell should inject its default workspace @@ -2189,7 +2205,7 @@ fn sandbox_to_k8s_spec( template, driver_gpu_requirements(spec.resource_requirements.as_ref()), &pod_env, - driver_config, + &driver_config, inject_workspace, params, ), @@ -2219,16 +2235,16 @@ fn sandbox_to_k8s_spec( &SandboxTemplate::default(), driver_gpu_requirements(spec.and_then(|s| s.resource_requirements.as_ref())), &pod_env, - driver_config, + &driver_config, inject_workspace, params, ), ); } - serde_json::Value::Object( + Ok(serde_json::Value::Object( std::iter::once(("spec".to_string(), serde_json::Value::Object(root))).collect(), - ) + )) } #[cfg(test)] @@ -3053,14 +3069,7 @@ mod tests { spec: Option<&SandboxSpec>, params: &SandboxPodParams<'_>, ) -> serde_json::Value { - let driver_config = spec - .and_then(|spec| spec.template.as_ref()) - .map(KubernetesSandboxDriverConfig::from_template) - .transpose() - .expect("test Kubernetes driver_config should be valid") - .unwrap_or_default(); - - sandbox_to_k8s_spec(spec, params, &driver_config) + sandbox_to_k8s_spec(spec, params).expect("test Kubernetes driver_config should be valid") } fn kube_api_error(code: u16, message: &str) -> KubeError { @@ -3129,7 +3138,7 @@ mod tests { } #[test] - fn driver_config_from_sandbox_rejects_unknown_fields() { + fn driver_config_for_spec_rejects_unknown_fields() { let sandbox = Sandbox { id: "sandbox-123".to_string(), spec: Some(SandboxSpec { @@ -3144,7 +3153,7 @@ mod tests { ..Default::default() }; - let err = KubernetesSandboxDriverConfig::from_sandbox(&sandbox).unwrap_err(); + let err = kubernetes_driver_config_for_spec(sandbox.spec.as_ref(), None).unwrap_err(); assert!(err.contains("unknown field")); assert!(err.contains("gpu_device_ids")); } @@ -3354,7 +3363,28 @@ mod tests { } #[test] - fn driver_config_rejects_invalid_dns1123_volume_and_claim_names() { + fn driver_config_accepts_dns1123_subdomain_pvc_claim_name() { + let template = SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc.user-data.123"} + }] + }))), + ..SandboxTemplate::default() + }; + + let config = KubernetesSandboxDriverConfig::from_template(&template) + .expect("DNS-1123 subdomain PVC names should validate"); + + assert_eq!( + config.volumes[0].persistent_volume_claim.claim_name, + "pvc.user-data.123" + ); + } + + #[test] + fn driver_config_rejects_invalid_volume_label_and_claim_name() { for (field, config) in [ ( "volumes[].name", @@ -3370,7 +3400,7 @@ mod tests { serde_json::json!({ "volumes": [{ "name": "user-data", - "persistent_volume_claim": {"claim_name": "pvc.user.data"} + "persistent_volume_claim": {"claim_name": "Pvc_User_Data"} }] }), ), @@ -3382,7 +3412,7 @@ mod tests { let err = KubernetesSandboxDriverConfig::from_template(&template).unwrap_err(); assert!( - err.contains(field) && err.contains("DNS-1123 label"), + err.contains(field) && err.contains("DNS-1123"), "expected invalid {field} to fail DNS-1123 validation, got {err}" ); } diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 16b3d912a..7cfc31b12 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -108,7 +108,7 @@ enum PodmanDriverMountConfig { Bind { source: String, target: String, - #[serde(default = "driver_mounts::default_true")] + #[serde(default = "default_true")] read_only: bool, #[serde(default)] selinux_label: Option, @@ -116,7 +116,7 @@ enum PodmanDriverMountConfig { Volume { source: String, target: String, - #[serde(default = "driver_mounts::default_true")] + #[serde(default = "default_true")] read_only: bool, #[serde(default)] subpath: Option, @@ -133,13 +133,17 @@ enum PodmanDriverMountConfig { Image { source: String, target: String, - #[serde(default = "driver_mounts::default_true")] + #[serde(default = "default_true")] read_only: bool, #[serde(default)] subpath: Option, }, } +fn default_true() -> bool { + true +} + /// Build a Podman container name from the sandbox name. #[must_use] pub fn container_name(sandbox_name: &str) -> String { diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index eb40f22a5..03a9b564f 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -392,7 +392,7 @@ Kubernetes PVC mount schema: | Field | Description | |---|---| | `volumes[].name` | Pod volume name. It must be a DNS-1123 label, unique, and not use OpenShell-managed volume names. | -| `volumes[].persistent_volume_claim.claim_name` | Existing PVC name in the sandbox namespace. It must be a DNS-1123 label. | +| `volumes[].persistent_volume_claim.claim_name` | Existing PVC name in the sandbox namespace. It must be a DNS-1123 subdomain name. | | `volumes[].persistent_volume_claim.read_only` | Optional. Defaults to `true`. Set `false` to allow read-write mounts. | | `containers.agent.volume_mounts[].name` | References a volume declared in `volumes`. | | `containers.agent.volume_mounts[].mount_path` | Absolute, normalized container path for the agent mount. | From aafe073f330966ad7f29c8b794db3d89503c5bee Mon Sep 17 00:00:00 2001 From: mjamiv Date: Thu, 2 Jul 2026 15:49:51 +0000 Subject: [PATCH 5/5] fix(kubernetes): address PVC review follow-ups --- crates/openshell-core/src/driver_mounts.rs | 73 ++--- crates/openshell-driver-docker/src/lib.rs | 14 +- .../openshell-driver-kubernetes/src/driver.rs | 277 ++++++++++-------- .../openshell-driver-podman/src/container.rs | 13 +- 4 files changed, 201 insertions(+), 176 deletions(-) diff --git a/crates/openshell-core/src/driver_mounts.rs b/crates/openshell-core/src/driver_mounts.rs index 8843883ff..086d992c3 100644 --- a/crates/openshell-core/src/driver_mounts.rs +++ b/crates/openshell-core/src/driver_mounts.rs @@ -3,7 +3,6 @@ //! Shared validation helpers for driver-config mounts. -use std::collections::HashSet; use std::path::Path; /// `SELinux` relabelling mode for bind mounts. @@ -92,6 +91,19 @@ pub fn validate_container_mount_target(target: &str) -> Result<(), String> { if !target.starts_with('/') { return Err("mount target must be an absolute container path".to_string()); } + if target != "/" { + let segments = target.split('/').skip(1).collect::>(); + let has_internal_empty_segment = segments + .iter() + .take(segments.len().saturating_sub(1)) + .any(|segment| segment.is_empty()); + if has_internal_empty_segment || segments.contains(&".") { + return Err( + "mount target must be normalized and must not contain empty path segments or '.'" + .to_string(), + ); + } + } let path = Path::new(target); if path == Path::new("/") { return Err("mount target must not be the container root".to_string()); @@ -128,22 +140,6 @@ pub fn path_is_or_under(path: &Path, parent: &Path) -> bool { path == parent || path.starts_with(parent) } -/// Validate that already-normalized driver mount targets are unique. -pub fn validate_unique_mount_targets<'a>( - targets: impl IntoIterator, - driver_name: &str, -) -> Result<(), String> { - let mut seen = HashSet::new(); - for target in targets { - if !seen.insert(target) { - return Err(format!( - "duplicate {driver_name} driver_config mount target '{target}'" - )); - } - } - Ok(()) -} - #[cfg(test)] mod tests { use super::*; @@ -180,33 +176,6 @@ mod tests { validate_container_mount_target("/etc/openshell-tools").unwrap(); } - #[test] - fn path_is_or_under_matches_boundaries() { - assert!(path_is_or_under( - Path::new("/sandbox"), - Path::new("/sandbox") - )); - assert!(path_is_or_under( - Path::new("/sandbox/work"), - Path::new("/sandbox") - )); - assert!(!path_is_or_under( - Path::new("/sandbox-work"), - Path::new("/sandbox") - )); - } - - #[test] - fn unique_mount_targets_rejects_duplicates() { - let err = - validate_unique_mount_targets(["/sandbox/work", "/sandbox/work"], "test").unwrap_err(); - - assert_eq!( - err, - "duplicate test driver_config mount target '/sandbox/work'" - ); - } - #[test] fn mount_subpath_must_be_relative_without_parent_dirs() { assert!(validate_mount_subpath("project/a").is_ok()); @@ -230,4 +199,20 @@ mod tests { "mount target must not contain surrounding whitespace" ); } + #[test] + fn mount_target_rejects_internal_empty_or_dot_segments() { + assert_eq!( + validate_container_mount_target("/sandbox/work//tmp").unwrap_err(), + "mount target must be normalized and must not contain empty path segments or '.'" + ); + assert_eq!( + validate_container_mount_target("/sandbox/work/./tmp").unwrap_err(), + "mount target must be normalized and must not contain empty path segments or '.'" + ); + assert_eq!( + validate_container_mount_target("/sandbox/work/../../tmp").unwrap_err(), + "mount target must not contain '..'" + ); + validate_container_mount_target("/sandbox/work/").unwrap(); + } } diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index f5c9fc88a..06d575a1e 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -49,7 +49,7 @@ use openshell_core::proto_struct::{ deserialize_optional_non_empty_string_list, struct_to_json_value, }; use openshell_core::{Config, Error, Result as CoreResult}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::io::Read; use std::net::{IpAddr, SocketAddr}; use std::path::{Path, PathBuf}; @@ -1845,7 +1845,7 @@ fn validate_docker_driver_mounts( mounts: &[DockerDriverMountConfig], enable_bind_mounts: bool, ) -> Result<(), Status> { - let mut targets = Vec::with_capacity(mounts.len()); + let mut targets = HashSet::new(); for mount in mounts { let target = match mount { DockerDriverMountConfig::Bind { source, target, .. } => { @@ -1899,10 +1899,14 @@ fn validate_docker_driver_mounts( }; driver_mounts::validate_container_mount_target(target) .map_err(Status::failed_precondition)?; - targets.push(driver_mounts::normalize_mount_target(target)); + let normalized_target = driver_mounts::normalize_mount_target(target); + if !targets.insert(normalized_target.clone()) { + return Err(Status::failed_precondition(format!( + "duplicate docker driver_config mount target '{normalized_target}'" + ))); + } } - driver_mounts::validate_unique_mount_targets(targets.iter().map(String::as_str), "docker") - .map_err(Status::failed_precondition) + Ok(()) } fn validate_optional_positive_integral_i64( diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index afd9f7464..5315d3040 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -10,7 +10,9 @@ use crate::config::{ SupervisorTopology, }; use futures::{Stream, StreamExt, TryStreamExt}; -use k8s_openapi::api::core::v1::{Event as KubeEventObj, Namespace, Node}; +use k8s_openapi::api::core::v1::{ + Event as KubeEventObj, Namespace, Node, PersistentVolumeClaimVolumeSource, Volume, VolumeMount, +}; use kube::api::{Api, ApiResource, DeleteParams, ListParams, PostParams}; use kube::core::gvk::GroupVersionKind; use kube::core::{DynamicObject, ObjectMeta}; @@ -217,6 +219,31 @@ impl Default for KubernetesDriverVolumeMountConfig { } } +impl From<&KubernetesDriverVolumeConfig> for Volume { + fn from(volume: &KubernetesDriverVolumeConfig) -> Self { + Self { + name: volume.name.clone(), + persistent_volume_claim: Some(PersistentVolumeClaimVolumeSource { + claim_name: volume.persistent_volume_claim.claim_name.clone(), + read_only: Some(volume.persistent_volume_claim.read_only), + }), + ..Default::default() + } + } +} + +impl From<&KubernetesDriverVolumeMountConfig> for VolumeMount { + fn from(mount: &KubernetesDriverVolumeMountConfig) -> Self { + Self { + name: mount.name.clone(), + mount_path: mount.mount_path.clone(), + read_only: Some(mount.read_only), + sub_path: mount.sub_path.clone(), + ..Default::default() + } + } +} + const CLIENT_TLS_VOLUME_NAME: &str = "openshell-client-tls"; const SERVICE_ACCOUNT_TOKEN_VOLUME_NAME: &str = "openshell-sa-token"; const SERVICE_ACCOUNT_TOKEN_MOUNT_PATH: &str = "/var/run/secrets/openshell"; @@ -229,8 +256,7 @@ const KUBERNETES_DRIVER_RESERVED_VOLUME_NAMES: &[&str] = &[ WORKSPACE_VOLUME_NAME, ]; -const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = - &[SERVICE_ACCOUNT_TOKEN_MOUNT_PATH, "/spiffe-workload-api"]; +const KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS: &[&str] = &[SERVICE_ACCOUNT_TOKEN_MOUNT_PATH]; fn validate_kubernetes_driver_volumes( volumes: &[KubernetesDriverVolumeConfig], @@ -269,7 +295,7 @@ fn validate_kubernetes_driver_volume_mounts( ); } - let mut mount_paths = Vec::with_capacity(volume_mounts.len()); + let mut mount_paths = HashSet::new(); for mount in volume_mounts { validate_kubernetes_dns1123_label(&mount.name, "containers.agent.volume_mounts[].name")?; let volume_name = mount.name.as_str(); @@ -284,67 +310,38 @@ fn validate_kubernetes_driver_volume_mounts( )); } - validate_kubernetes_mount_path(&mount.mount_path)?; - mount_paths.push(mount.mount_path.as_str()); + driver_mounts::validate_container_mount_target(&mount.mount_path)?; + let normalized_mount_path = driver_mounts::normalize_mount_target(&mount.mount_path); + if !mount_paths.insert(normalized_mount_path.clone()) { + return Err(format!( + "duplicate kubernetes driver_config mount target '{normalized_mount_path}'" + )); + } if let Some(sub_path) = mount.sub_path.as_ref() { driver_mounts::validate_mount_subpath(sub_path)?; } } - driver_mounts::validate_unique_mount_targets(mount_paths, "kubernetes") -} - -fn validate_kubernetes_driver_runtime_mounts( - config: &KubernetesSandboxDriverConfig, - provider_spiffe_workload_api_socket_path: Option<&str>, -) -> Result<(), String> { - let Some(socket_path) = provider_spiffe_workload_api_socket_path else { - return Ok(()); - }; - // The SPIFFE Workload API mount path is derived from gateway runtime - // configuration, not from the sandbox template. Static protected paths are - // checked during `from_template`; this dynamic conflict can only be checked - // once the driver runtime config is available. - let spiffe_mount_path = spiffe_socket_mount_path(socket_path); - for mount in &config.containers.agent.volume_mounts { - let mount_path = mount.mount_path.as_str(); - if mount_path_conflicts_with_protected_path(mount_path, &spiffe_mount_path) { - return Err(format!( - "mount path '{mount_path}' conflicts with reserved OpenShell path '{spiffe_mount_path}'" - )); - } - } Ok(()) } -fn validate_kubernetes_name_text(value: &str, field: &str) -> Result<(), String> { - if value != value.trim() { - return Err(format!( - "{field} must not contain leading or trailing whitespace" - )); +// TODO: replace with an openshell_core Kubernetes-name helper once available. +fn is_dns_label(label: &str) -> bool { + if label.is_empty() || label.len() > 63 || label.starts_with('-') || label.ends_with('-') { + return false; } - driver_mounts::validate_mount_source(value, field) + label + .bytes() + .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-') } -fn is_kubernetes_dns1123_label(value: &str) -> bool { - !value.is_empty() - && value.len() <= 63 - && value - .bytes() - .all(|byte| byte.is_ascii_lowercase() || byte.is_ascii_digit() || byte == b'-') - && value - .as_bytes() - .first() - .is_some_and(u8::is_ascii_alphanumeric) - && value - .as_bytes() - .last() - .is_some_and(u8::is_ascii_alphanumeric) +// TODO: replace with an openshell_core Kubernetes-name helper once available. +fn is_dns_subdomain(value: &str) -> bool { + value.len() <= 253 && value.split('.').all(is_dns_label) } fn validate_kubernetes_dns1123_label(value: &str, field: &str) -> Result<(), String> { - validate_kubernetes_name_text(value, field)?; - if !is_kubernetes_dns1123_label(value) { + if !is_dns_label(value) { return Err(format!( "{field} must be a DNS-1123 label: use lowercase alphanumeric characters or '-', start and end with an alphanumeric character, and use at most 63 characters" )); @@ -353,10 +350,7 @@ fn validate_kubernetes_dns1123_label(value: &str, field: &str) -> Result<(), Str } fn validate_kubernetes_dns1123_subdomain(value: &str, field: &str) -> Result<(), String> { - validate_kubernetes_name_text(value, field)?; - let is_dns1123_subdomain = - value.len() <= 253 && value.split('.').all(is_kubernetes_dns1123_label); - if !is_dns1123_subdomain { + if !is_dns_subdomain(value) { return Err(format!( "{field} must be a DNS-1123 subdomain: use lowercase alphanumeric characters, '-' or '.', start and end with an alphanumeric character, and use at most 253 characters" )); @@ -364,60 +358,36 @@ fn validate_kubernetes_dns1123_subdomain(value: &str, field: &str) -> Result<(), Ok(()) } -fn validate_kubernetes_mount_path(mount_path: &str) -> Result<(), String> { - if mount_path != mount_path.trim() { - return Err("mount_path must not contain leading or trailing whitespace".to_string()); - } - if mount_path != "/" - && mount_path - .split('/') - .skip(1) - .any(|segment| segment.is_empty() || segment == ".") - { - return Err( - "mount_path must be normalized and must not contain empty path segments or '.'" - .to_string(), - ); - } +fn mount_path_conflicts_with_protected_path(mount_path: &str, protected_path: &str) -> bool { + driver_mounts::path_is_or_under(Path::new(mount_path), Path::new(protected_path)) + || driver_mounts::path_is_or_under(Path::new(protected_path), Path::new(mount_path)) +} - driver_mounts::validate_container_mount_target(mount_path)?; - for protected_path in KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS { - if mount_path_conflicts_with_protected_path(mount_path, protected_path) { - return Err(format!( - "mount path '{mount_path}' conflicts with reserved OpenShell path '{protected_path}'" - )); +fn validate_kubernetes_protected_path_conflicts( + volume_mounts: &[KubernetesDriverVolumeMountConfig], + protected_paths: &[&str], +) -> Result<(), String> { + for mount in volume_mounts { + let mount_path = mount.mount_path.as_str(); + for protected_path in protected_paths { + if mount_path_conflicts_with_protected_path(mount_path, protected_path) { + return Err(format!( + "mount path '{mount_path}' conflicts with reserved OpenShell path '{protected_path}'" + )); + } } } Ok(()) } -fn mount_path_conflicts_with_protected_path(mount_path: &str, protected_path: &str) -> bool { - driver_mounts::path_is_or_under(Path::new(mount_path), Path::new(protected_path)) - || driver_mounts::path_is_or_under(Path::new(protected_path), Path::new(mount_path)) -} - fn kubernetes_driver_volume_to_k8s(volume: &KubernetesDriverVolumeConfig) -> serde_json::Value { - serde_json::json!({ - "name": volume.name.as_str(), - "persistentVolumeClaim": { - "claimName": volume.persistent_volume_claim.claim_name.as_str(), - "readOnly": volume.persistent_volume_claim.read_only, - } - }) + serde_json::to_value(Volume::from(volume)).expect("Volume serializes to JSON") } fn kubernetes_driver_volume_mount_to_k8s( mount: &KubernetesDriverVolumeMountConfig, ) -> serde_json::Value { - let mut value = serde_json::json!({ - "name": mount.name.as_str(), - "mountPath": mount.mount_path.as_str(), - "readOnly": mount.read_only, - }); - if let Some(sub_path) = mount.sub_path.as_ref() { - value["subPath"] = serde_json::json!(sub_path); - } - value + serde_json::to_value(VolumeMount::from(mount)).expect("VolumeMount serializes to JSON") } // --------------------------------------------------------------------------- @@ -2176,7 +2146,16 @@ fn kubernetes_driver_config_for_spec( .map(KubernetesSandboxDriverConfig::from_template) .transpose()? .unwrap_or_default(); - validate_kubernetes_driver_runtime_mounts(&config, provider_spiffe_workload_api_socket_path)?; + let mut protected_paths = KUBERNETES_DRIVER_PROTECTED_MOUNT_PATHS.to_vec(); + let provider_spiffe_mount_path; + if let Some(socket_path) = provider_spiffe_workload_api_socket_path { + provider_spiffe_mount_path = spiffe_socket_mount_path(socket_path); + protected_paths.push(&provider_spiffe_mount_path); + } + validate_kubernetes_protected_path_conflicts( + &config.containers.agent.volume_mounts, + &protected_paths, + )?; Ok(config) } @@ -3445,15 +3424,13 @@ mod tests { } #[test] - fn driver_config_rejects_protected_kubernetes_mount_targets() { + fn driver_config_rejects_shared_reserved_mount_targets() { for mount_path in [ "/", "/sandbox", "/etc/openshell", "/etc/openshell-tls/client", - "/var/run/secrets/openshell", "/opt/openshell/bin", - "/spiffe-workload-api", ] { let template = SandboxTemplate { driver_config: Some(json_struct(serde_json::json!({ @@ -3481,6 +3458,61 @@ mod tests { } } + #[test] + fn driver_config_rejects_kubernetes_static_protected_mount_targets() { + let spec = SandboxSpec { + template: Some(SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": "/var/run/secrets/openshell" + }] + } + } + }))), + ..SandboxTemplate::default() + }), + ..SandboxSpec::default() + }; + + let err = kubernetes_driver_config_for_spec(Some(&spec), None).unwrap_err(); + + assert!(err.contains("/var/run/secrets/openshell")); + } + + #[test] + fn driver_config_allows_spiffe_workload_path_without_provider_spiffe() { + let spec = SandboxSpec { + template: Some(SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": "/spiffe-workload-api" + }] + } + } + }))), + ..SandboxTemplate::default() + }), + ..SandboxSpec::default() + }; + + kubernetes_driver_config_for_spec(Some(&spec), None) + .expect("SPIFFE workload path should only be protected when SPIFFE is enabled"); + } + #[test] fn driver_config_rejects_invalid_kubernetes_sub_paths() { for sub_path in ["/workspace", "../workspace"] { @@ -3645,31 +3677,30 @@ mod tests { #[test] fn driver_config_rejects_runtime_provider_spiffe_mount_path() { - let template = SandboxTemplate { - driver_config: Some(json_struct(serde_json::json!({ - "volumes": [{ - "name": "user-data", - "persistent_volume_claim": {"claim_name": "pvc-user-data"} - }], - "containers": { - "agent": { - "volume_mounts": [{ - "name": "user-data", - "mount_path": "/custom-spiffe" - }] + let spec = SandboxSpec { + template: Some(SandboxTemplate { + driver_config: Some(json_struct(serde_json::json!({ + "volumes": [{ + "name": "user-data", + "persistent_volume_claim": {"claim_name": "pvc-user-data"} + }], + "containers": { + "agent": { + "volume_mounts": [{ + "name": "user-data", + "mount_path": "/custom-spiffe" + }] + } } - } - }))), - ..SandboxTemplate::default() + }))), + ..SandboxTemplate::default() + }), + ..SandboxSpec::default() }; - let config = KubernetesSandboxDriverConfig::from_template(&template) - .expect("static driver_config should be valid"); - let err = validate_kubernetes_driver_runtime_mounts( - &config, - Some("/custom-spiffe/spire-agent.sock"), - ) - .unwrap_err(); + let err = + kubernetes_driver_config_for_spec(Some(&spec), Some("/custom-spiffe/spire-agent.sock")) + .unwrap_err(); assert!(err.contains("/custom-spiffe")); } diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 7cfc31b12..e4a0b7918 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -13,7 +13,7 @@ use openshell_core::proto_struct::deserialize_optional_non_empty_string_list; use openshell_core::{driver_mounts, proto_struct}; use serde::Serialize; use serde_json::Value; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; #[cfg(target_os = "linux")] use std::path::Path; @@ -664,7 +664,7 @@ fn validate_podman_driver_mounts( mounts: &[PodmanDriverMountConfig], enable_bind_mounts: bool, ) -> Result<(), String> { - let mut targets = Vec::with_capacity(mounts.len()); + let mut targets = HashSet::new(); for mount in mounts { let target = match mount { PodmanDriverMountConfig::Bind { source, target, .. } => { @@ -710,9 +710,14 @@ fn validate_podman_driver_mounts( } }; driver_mounts::validate_container_mount_target(target)?; - targets.push(driver_mounts::normalize_mount_target(target)); + let normalized_target = driver_mounts::normalize_mount_target(target); + if !targets.insert(normalized_target.clone()) { + return Err(format!( + "duplicate podman driver_config mount target '{normalized_target}'" + )); + } } - driver_mounts::validate_unique_mount_targets(targets.iter().map(String::as_str), "podman") + Ok(()) } fn reject_subpath(subpath: Option<&str>, mount_type: &str) -> Result<(), String> {