diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index 17628e980b..b5f4f43da5 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -439,10 +439,16 @@ helm -n openshell get values openshell | grep sandboxNamespace Then inspect sandbox resources in that namespace. -Check the configured sandbox service account when TokenReview bootstrap or +Check the accepted sandbox service accounts when TokenReview bootstrap or sandbox registration fails. Helm creates a dedicated sandbox service account by default and writes it to `[openshell.drivers.kubernetes].service_account_name`; -the gateway rejects projected tokens from other service accounts. +the gateway rejects projected tokens from any service account outside that name +plus `additional_bootstrap_service_account_names` and +`selectable_service_account_names`. A create that fails with +`service_account_name '' is not selectable` is asking for an account +missing from `selectable_service_account_names`; the gateway log records the +requested name and the selectable set. The `gateway.toml` dump below shows both +settings. ```bash helm -n openshell get values openshell | grep -A3 sandboxServiceAccount diff --git a/architecture/gateway.md b/architecture/gateway.md index 0c428526d8..273742c220 100644 --- a/architecture/gateway.md +++ b/architecture/gateway.md @@ -210,7 +210,7 @@ Sandbox secrets are gateway-signed JWTs bound to a single sandbox ID. Docker, Podman, and VM drivers deliver the initial token through supervisor-only runtime material; Kubernetes supervisors exchange a projected ServiceAccount token through `IssueSandboxToken`. The gateway validates that projected token -with Kubernetes `TokenReview`, requires the configured sandbox service account, +with Kubernetes `TokenReview`, requires an accepted sandbox service account, checks the returned pod binding against the live pod UID, and verifies the pod's controlling `Sandbox` ownerReference against the live Sandbox CR UID and sandbox-id label before minting the gateway JWT. The bootstrap path accepts diff --git a/crates/openshell-driver-kubernetes/README.md b/crates/openshell-driver-kubernetes/README.md index ec37c4e67a..23c2dcdf7f 100644 --- a/crates/openshell-driver-kubernetes/README.md +++ b/crates/openshell-driver-kubernetes/README.md @@ -84,7 +84,8 @@ The driver injects gateway callback configuration, sandbox identity, TLS client material, and the supervisor SSH socket path into the workload. Driver-owned values must override image-provided environment variables. -Sandbox pods run as `service_account_name` and keep +Sandbox pods run as `service_account_name`, or as an account the caller +selected from `selectable_service_account_names` and keep `automountServiceAccountToken: false`. The only Kubernetes token exposed to the supervisor is an explicit, audience-bound projected token mounted at `/var/run/secrets/openshell/token` for the one-shot `IssueSandboxToken` diff --git a/crates/openshell-driver-kubernetes/src/config.rs b/crates/openshell-driver-kubernetes/src/config.rs index 1cdc98f6e1..a8fdf34028 100644 --- a/crates/openshell-driver-kubernetes/src/config.rs +++ b/crates/openshell-driver-kubernetes/src/config.rs @@ -300,9 +300,50 @@ pub struct KubernetesComputeConfig { /// operator mode. Hot-reloaded on change. Delivered via `ConfigMap` volume mount. #[serde(default, skip_serializing_if = "Option::is_none")] pub operator_namespace_file: Option, - /// Kubernetes `ServiceAccount` assigned to sandbox pods and accepted by - /// the gateway's `TokenReview` bootstrap authenticator. + /// Default Kubernetes `ServiceAccount` for sandbox pods, used unless a + /// request selects another selectable account, and accepted by the + /// gateway's `TokenReview` bootstrap authenticator. pub service_account_name: String, + /// Extra `ServiceAccount` names the gateway's `TokenReview` bootstrap + /// authenticator accepts, on top of [`Self::service_account_name`]. + /// + /// This is deliberately a separate setting rather than a widening of + /// `service_account_name`, because a pod spec has a single + /// `serviceAccountName` field and a list has no valid rendering into it. + /// Use this where something other than the driver assigns the pod's + /// `ServiceAccount`, such as a mutating admission policy or an external + /// controller that owns the sandbox pods. + /// + /// Names are matched exactly and without regard to namespace, so under + /// managed and operator workspace modes an entry is accepted in every + /// namespace the gateway accepts. Empty by default; on its own it does not + /// widen the accepted set. + /// + /// Accepting a name here does not let a caller ask for it. Use + /// [`Self::selectable_service_account_names`] for that. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub additional_bootstrap_service_account_names: Vec, + /// `ServiceAccount` names a caller may request for an individual sandbox + /// through `SandboxTemplate.service_account_name`, and which the gateway's + /// bootstrap authenticator therefore also accepts. + /// + /// This is separate from [`Self::additional_bootstrap_service_account_names`] + /// on purpose. That setting says "accept this identity if something outside + /// the driver assigned it to the pod"; this one says "any caller who can + /// create a sandbox may run as this identity". Enrolling a privileged + /// account for bootstrap should not silently put it on the menu, so the two + /// are opted into independently. + /// + /// A request naming an account outside this set is rejected; the driver's + /// own [`Self::service_account_name`] is selectable whenever it is set. + /// Empty by default, which leaves the driver default as the only account a + /// caller can request. + /// + /// Members are available to every caller who can create a sandbox, and the + /// driver only ever provisions [`Self::service_account_name`], so the rest + /// have to exist already in each namespace the gateway uses. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub selectable_service_account_names: Vec, pub default_image: String, pub image_pull_policy: String, /// Kubernetes `imagePullSecrets` names attached to sandbox pods. @@ -436,6 +477,8 @@ impl Default for KubernetesComputeConfig { operator_namespace_label: None, operator_namespace_file: None, service_account_name: DEFAULT_SANDBOX_SERVICE_ACCOUNT_NAME.to_string(), + additional_bootstrap_service_account_names: Vec::new(), + selectable_service_account_names: Vec::new(), default_image: openshell_core::image::default_sandbox_image(), // Default empty so the gateway omits `imagePullPolicy` from pod // specs and Kubernetes applies its own default (Always for `latest`, @@ -652,6 +695,115 @@ impl KubernetesComputeConfig { }) } + /// `ServiceAccount` names the gateway's bootstrap authenticator accepts: + /// the pod default, the names enrolled for bootstrap only, and every name a + /// caller may select. A selectable account has to authenticate, so it is + /// necessarily accepted. + pub fn accepted_bootstrap_service_account_names(&self) -> BTreeSet { + service_account_name_set( + std::iter::once(self.service_account_name.as_str()) + .chain( + self.additional_bootstrap_service_account_names + .iter() + .map(String::as_str), + ) + .chain( + self.selectable_service_account_names + .iter() + .map(String::as_str), + ), + ) + } + + /// `ServiceAccount` names a caller may request for a single sandbox. The + /// driver's own default is always selectable; anything else has to be + /// opted in through `selectable_service_account_names`. + pub fn selectable_pod_service_account_names(&self) -> BTreeSet { + service_account_name_set( + std::iter::once(self.service_account_name.as_str()).chain( + self.selectable_service_account_names + .iter() + .map(String::as_str), + ), + ) + } + + /// Resolve the `ServiceAccount` for one sandbox's pod. + /// + /// `requested` is the caller's `SandboxTemplate.service_account_name` when + /// the field was set. An absent field takes [`Self::service_account_name`]. + /// A field that was set has to name a selectable account: anything else, + /// including a blank or whitespace-only value, is an error rather than a + /// silent fall back to the default, because running a sandbox under a + /// different identity than the caller asked for is the kind of surprise + /// that is discovered by an unexpected IAM denial much later. + /// + /// The error names the requested account but not the selectable set, which + /// goes to the gateway log instead: the caller already knows what it asked + /// for, and the set of identities a gateway can assume is not something to + /// hand out on request. + pub fn resolve_pod_service_account(&self, requested: Option<&str>) -> Result { + let Some(requested) = requested else { + return Ok(self.service_account_name.trim().to_string()); + }; + let requested = requested.trim(); + if requested.is_empty() { + return Err( + "service_account_name was set but is blank; omit the field to use \ + the gateway's default sandbox ServiceAccount" + .to_string(), + ); + } + if !self + .selectable_pod_service_account_names() + .contains(requested) + { + return Err(format!( + "service_account_name '{requested}' is not selectable on this gateway" + )); + } + Ok(requested.to_string()) + } + + /// Reject configured `ServiceAccount` names that Kubernetes could never + /// issue, so a typo fails at gateway startup instead of surfacing later as + /// a rejected sandbox create or a pod the apiserver refuses to admit. + pub fn validate_service_account_names(&self) -> Result<(), String> { + let lists = [ + ( + "service_account_name", + std::slice::from_ref(&self.service_account_name), + ), + ( + "additional_bootstrap_service_account_names", + self.additional_bootstrap_service_account_names.as_slice(), + ), + ( + "selectable_service_account_names", + self.selectable_service_account_names.as_slice(), + ), + ]; + for (setting, names) in lists { + for raw in names.iter().filter(|name| !name.is_empty()) { + if raw != raw.trim() { + return Err(format!( + "{setting} entry '{raw}' has leading or trailing whitespace; \ + a ServiceAccount name never does" + )); + } + } + for name in service_account_name_set(names.iter().map(String::as_str)) { + if !is_service_account_name(&name) { + return Err(format!( + "{setting} entry '{name}' is not a valid Kubernetes ServiceAccount name \ + (DNS-1123 subdomain, at most 253 characters)" + )); + } + } + } + Ok(()) + } + /// Validate that configured `sandbox_uid` and `sandbox_gid` fall within /// the policy-enforced UID/GID range. Called during driver initialization /// before any pod parameters are rendered. @@ -809,6 +961,30 @@ pub fn managed_namespace_prefix(gateway_id: &str) -> String { format!("openshell-{gateway_id}-") } +/// Check whether a string is a name Kubernetes could issue a `ServiceAccount` +/// for: a DNS-1123 subdomain of at most 253 characters. +#[must_use] +pub fn is_service_account_name(name: &str) -> bool { + !name.is_empty() && name.len() <= 253 && name.split('.').all(is_dns_1123_label) +} + +/// Normalize a list of configured `ServiceAccount` names into a set. +/// +/// Names are trimmed, because a `ServiceAccount` name never carries +/// surrounding whitespace and a padded config entry would otherwise sit in the +/// set as a member nothing can ever match. Entries that are empty after +/// trimming are dropped, so a blank config line cannot widen the set. +#[must_use] +pub fn service_account_name_set<'a>(names: impl IntoIterator) -> BTreeSet { + names + .into_iter() + .filter_map(|name| { + let trimmed = name.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) + }) + .collect() +} + /// Check whether a string is a valid DNS-1123 label (lowercase alphanumeric /// and hyphens, 1-63 chars, must start and end with alphanumeric). #[must_use] @@ -1152,6 +1328,244 @@ mod tests { assert_eq!(cfg.service_account_name, "openshell-sandbox"); } + #[test] + fn additional_bootstrap_service_account_names_default_to_empty() { + let cfg: KubernetesComputeConfig = serde_json::from_value(serde_json::json!({})).unwrap(); + assert!(cfg.additional_bootstrap_service_account_names.is_empty()); + } + + #[test] + fn serde_override_additional_bootstrap_service_account_names() { + let json = serde_json::json!({ + "service_account_name": "openshell-sandbox", + "additional_bootstrap_service_account_names": [ + "openshell-sandbox-2", + "openshell-sandbox-3" + ] + }); + let cfg: KubernetesComputeConfig = serde_json::from_value(json).unwrap(); + assert_eq!(cfg.service_account_name, "openshell-sandbox"); + assert_eq!( + cfg.additional_bootstrap_service_account_names, + vec![ + "openshell-sandbox-2".to_string(), + "openshell-sandbox-3".to_string() + ] + ); + } + + fn config_with_service_accounts( + pod_default: &str, + additional: &[&str], + selectable: &[&str], + ) -> KubernetesComputeConfig { + KubernetesComputeConfig { + service_account_name: pod_default.to_string(), + additional_bootstrap_service_account_names: additional + .iter() + .map(ToString::to_string) + .collect(), + selectable_service_account_names: selectable.iter().map(ToString::to_string).collect(), + ..Default::default() + } + } + + #[test] + fn selectable_service_account_names_default_to_empty() { + let cfg: KubernetesComputeConfig = serde_json::from_value(serde_json::json!({})).unwrap(); + assert!(cfg.selectable_service_account_names.is_empty()); + } + + /// Everything that can appear on a sandbox pod has to be able to + /// authenticate, so the bootstrap set is the union of both lists. + #[test] + fn accepted_bootstrap_names_union_both_lists_and_the_pod_default() { + let cfg = config_with_service_accounts( + "openshell-sandbox", + [" openshell-sandbox-2 ", ""].as_slice(), + ["openshell-sandbox-3", "openshell-sandbox"].as_slice(), + ); + + let accepted = cfg.accepted_bootstrap_service_account_names(); + assert!(accepted.contains("openshell-sandbox")); + assert!(accepted.contains("openshell-sandbox-2"), "trimmed and kept"); + assert!(accepted.contains("openshell-sandbox-3")); + assert_eq!(accepted.len(), 3, "blank dropped, duplicate collapsed"); + } + + /// The security property that keeps the two lists separate: enrolling an + /// account for bootstrap must not put it on the menu for callers. + #[test] + fn bootstrap_only_names_are_not_selectable() { + let cfg = config_with_service_accounts( + "openshell-sandbox", + ["openshell-sandbox-privileged"].as_slice(), + ["openshell-sandbox-3"].as_slice(), + ); + + let selectable = cfg.selectable_pod_service_account_names(); + assert!(selectable.contains("openshell-sandbox"), "pod default"); + assert!(selectable.contains("openshell-sandbox-3")); + assert!( + !selectable.contains("openshell-sandbox-privileged"), + "a bootstrap-only account must not be selectable by a caller" + ); + assert!( + cfg.accepted_bootstrap_service_account_names() + .contains("openshell-sandbox-privileged"), + "but it still authenticates" + ); + } + + #[test] + fn resolve_pod_service_account_falls_back_to_the_driver_default() { + let cfg = config_with_service_accounts("openshell-sandbox", &[], &["openshell-sandbox-3"]); + + assert_eq!( + cfg.resolve_pod_service_account(None).unwrap(), + "openshell-sandbox" + ); + } + + /// The default is trimmed on the way out, so a padded config value cannot + /// reach the pod as a name the apiserver rejects while the accepted set + /// holds the trimmed spelling. + #[test] + fn resolve_pod_service_account_trims_the_driver_default() { + let cfg = config_with_service_accounts(" openshell-sandbox ", &[], &[]); + + assert_eq!( + cfg.resolve_pod_service_account(None).unwrap(), + "openshell-sandbox" + ); + } + + /// A field that was set but is blank is a mistake, not a request for the + /// default: defaulting it would run the sandbox under an identity the + /// caller did not ask for. + #[test] + fn resolve_pod_service_account_rejects_a_blank_request() { + let cfg = config_with_service_accounts("openshell-sandbox", &[], &["openshell-sandbox-3"]); + + for blank in ["", " ", "\t"] { + let err = cfg.resolve_pod_service_account(Some(blank)).unwrap_err(); + assert!(err.contains("blank"), "{err}"); + } + } + + /// The set of identities a gateway can assume is not something to hand out + /// to whoever asks; it goes to the log instead. + #[test] + fn resolve_pod_service_account_error_does_not_disclose_the_selectable_set() { + let cfg = config_with_service_accounts( + "openshell-sandbox", + &[], + ["openshell-sandbox-privileged"].as_slice(), + ); + + let err = cfg + .resolve_pod_service_account(Some("openshell-sandbox-typo")) + .unwrap_err(); + assert!(err.contains("openshell-sandbox-typo"), "{err}"); + assert!( + !err.contains("openshell-sandbox-privileged"), + "the selectable set must not leak to the caller: {err}" + ); + } + + #[test] + fn validate_service_account_names_accepts_valid_names() { + let cfg = config_with_service_accounts( + "openshell-sandbox", + ["openshell-sandbox-2", "openshell-sandbox-3"].as_slice(), + ["sa.with.dots", "a"].as_slice(), + ); + + assert!(cfg.validate_service_account_names().is_ok()); + } + + /// A padded config entry is a mistake worth failing on: the accepted set + /// and the resolved pod name are both trimmed, so the untrimmed spelling + /// would only ever appear in the `ServiceAccount` the driver provisions. + #[test] + fn validate_service_account_names_rejects_untrimmed_entries() { + for cfg in [ + config_with_service_accounts(" openshell-sandbox ", &[], &[]), + config_with_service_accounts("openshell-sandbox", [" padded "].as_slice(), &[]), + config_with_service_accounts("openshell-sandbox", &[], ["padded\t"].as_slice()), + ] { + let err = cfg.validate_service_account_names().unwrap_err(); + assert!(err.contains("whitespace"), "{err}"); + } + } + + #[test] + fn validate_service_account_names_rejects_names_kubernetes_cannot_issue() { + for bad in [ + "Openshell-Sandbox", + "has space", + "-leading-hyphen", + "under_score", + ] { + let cfg = config_with_service_accounts("openshell-sandbox", &[], [bad].as_slice()); + let err = cfg.validate_service_account_names().unwrap_err(); + assert!(err.contains("selectable_service_account_names"), "{err}"); + assert!(err.contains(bad), "{err}"); + } + + let too_long = "a".repeat(254); + let cfg = + config_with_service_accounts("openshell-sandbox", [too_long.as_str()].as_slice(), &[]); + let err = cfg.validate_service_account_names().unwrap_err(); + assert!( + err.contains("additional_bootstrap_service_account_names"), + "{err}" + ); + } + + #[test] + fn resolve_pod_service_account_accepts_a_selectable_request() { + let cfg = config_with_service_accounts("openshell-sandbox", &[], &["openshell-sandbox-3"]); + + assert_eq!( + cfg.resolve_pod_service_account(Some("openshell-sandbox-3")) + .unwrap(), + "openshell-sandbox-3" + ); + assert_eq!( + cfg.resolve_pod_service_account(Some(" openshell-sandbox-3 ")) + .unwrap(), + "openshell-sandbox-3" + ); + assert_eq!( + cfg.resolve_pod_service_account(Some("openshell-sandbox")) + .unwrap(), + "openshell-sandbox", + "asking for the default explicitly is fine" + ); + } + + #[test] + fn resolve_pod_service_account_rejects_a_non_selectable_request() { + let cfg = config_with_service_accounts( + "openshell-sandbox", + ["openshell-sandbox-privileged"].as_slice(), + ["openshell-sandbox-3"].as_slice(), + ); + + let err = cfg + .resolve_pod_service_account(Some("openshell-sandbox-privileged")) + .unwrap_err(); + assert!(err.contains("openshell-sandbox-privileged"), "{err}"); + assert!(err.contains("not selectable"), "{err}"); + + assert!( + cfg.resolve_pod_service_account(Some("openshell-sandbox-typo")) + .is_err(), + "an unknown account is rejected, not silently defaulted" + ); + } + #[test] fn serde_override_default_runtime_class_name() { let json = serde_json::json!({ diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index b9b9de72ad..34e3c32c2c 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -486,6 +486,9 @@ impl KubernetesComputeDriver { config .validate_upstream_proxy_config() .map_err(KubernetesDriverError::Precondition)?; + config + .validate_service_account_names() + .map_err(KubernetesDriverError::Precondition)?; let base_config = match kube::Config::incluster() { Ok(c) => c, Err(_) => kube::Config::infer() @@ -782,7 +785,10 @@ impl KubernetesComputeDriver { let sa_api: Api = Api::namespaced(self.client.clone(), namespace); let sa = ServiceAccount { metadata: ObjectMeta { - name: Some(self.config.service_account_name.clone()), + // Trimmed to match the name pods reference: the accepted set + // and the resolved pod value are both trimmed, so creating the + // untrimmed spelling would provision an account nothing uses. + name: Some(self.config.service_account_name.trim().to_string()), labels: Some(BTreeMap::from([( LABEL_MANAGED_BY.to_string(), LABEL_MANAGED_BY_VALUE.to_string(), @@ -1194,6 +1200,26 @@ impl KubernetesComputeDriver { })) } + /// Resolve the `ServiceAccount` for a sandbox's pod, logging a rejection. + /// + /// The log carries the selectable set and the rejection does not: an + /// operator needs to see which name was asked for and what the gateway + /// offers, while a caller only needs to know its own request was refused. + fn resolve_requested_service_account(&self, sandbox: &Sandbox) -> Result { + let requested = requested_pod_service_account(sandbox); + self.config + .resolve_pod_service_account(requested.as_deref()) + .inspect_err(|err| { + warn!( + sandbox_id = %sandbox.id, + requested_service_account = ?requested, + selectable_service_accounts = ?self.config.selectable_pod_service_account_names(), + error = %err, + "rejected a sandbox ServiceAccount request that is not selectable" + ); + }) + } + pub async fn validate_sandbox_create(&self, sandbox: &Sandbox) -> Result<(), tonic::Status> { let _ = self .validate_driver_config_for_sandbox(sandbox) @@ -1207,6 +1233,11 @@ impl KubernetesComputeDriver { .map_err(tonic::Status::invalid_argument)?; } } + // Reject an unselectable ServiceAccount here, before the gateway + // persists the sandbox record or mints its JWT, so nothing has to be + // rolled back. `create_sandbox` resolves the name again for the value. + self.resolve_requested_service_account(sandbox) + .map_err(tonic::Status::invalid_argument)?; let gpu_requirements = sandbox .spec .as_ref() @@ -1352,6 +1383,14 @@ impl KubernetesComputeDriver { validate_kubernetes_dns1123_label(&sandbox.name, "sandbox name") .map_err(KubernetesDriverError::InvalidArgument)?; + // Resolved before any namespace or secret is created, so a request + // rejected here leaves nothing behind. The resolved name is always a + // member of the bootstrap authenticator's accepted set, because the + // selectable set is a subset of it. + let service_account_name = self + .resolve_requested_service_account(sandbox) + .map_err(KubernetesDriverError::InvalidArgument)?; + let name = sandbox.name.as_str(); let workspace = sandbox.workspace.as_str(); self.validate_workspace_namespace(workspace)?; @@ -1417,7 +1456,7 @@ impl KubernetesComputeDriver { proxy_auth_secret_key: self.config.proxy_auth_secret_key.as_deref(), proxy_auth_allow_insecure: self.config.proxy_auth_allow_insecure == Some(true), proxy_connect_by_hostname: self.config.proxy_connect_by_hostname == Some(true), - service_account_name: &self.config.service_account_name, + service_account_name: &service_account_name, sandbox_id: &sandbox.id, sandbox_name: &sandbox.name, grpc_endpoint: &self.config.grpc_endpoint, @@ -3267,6 +3306,19 @@ impl Default for SandboxPodParams<'_> { } } +/// The `ServiceAccount` a sandbox's template asked for, if it asked for one. +/// +/// Split out from `create_sandbox` so the read is testable without a cluster: +/// it is the only thing standing between a caller's request and the identity +/// its pod runs as. +fn requested_pod_service_account(sandbox: &Sandbox) -> Option { + sandbox + .spec + .as_ref() + .and_then(|spec| spec.template.as_ref()) + .and_then(|template| platform_config_string(template, "service_account_name")) +} + fn validate_sidecar_proxy_identity( params: &SandboxPodParams<'_>, ) -> Result<(), KubernetesDriverError> { @@ -7064,6 +7116,90 @@ mod tests { ); } + fn sandbox_requesting_service_account(value: Option) -> Sandbox { + Sandbox { + id: "sandbox-123".to_string(), + spec: Some(SandboxSpec { + template: Some(SandboxTemplate { + platform_config: value.map(|v| Struct { + fields: std::iter::once(("service_account_name".to_string(), v)).collect(), + }), + ..Default::default() + }), + ..Default::default() + }), + ..Default::default() + } + } + + /// This read is the only thing between a caller's request and the identity + /// its pod runs as, so it is worth pinning on its own. + #[test] + fn requested_pod_service_account_reads_the_platform_config_key() { + let sandbox = sandbox_requesting_service_account(Some(Value { + kind: Some(Kind::StringValue("openshell-sandbox-3".to_string())), + })); + + assert_eq!( + requested_pod_service_account(&sandbox).as_deref(), + Some("openshell-sandbox-3") + ); + } + + #[test] + fn requested_pod_service_account_is_none_when_unset() { + assert_eq!(requested_pod_service_account(&Sandbox::default()), None); + assert_eq!( + requested_pod_service_account(&sandbox_requesting_service_account(None)), + None + ); + } + + /// A non-string value must not read as a request, or a malformed one would + /// silently take the driver default. + #[test] + fn requested_pod_service_account_ignores_a_non_string_value() { + let sandbox = sandbox_requesting_service_account(Some(Value { + kind: Some(Kind::NumberValue(42.0)), + })); + + assert_eq!(requested_pod_service_account(&sandbox), None); + } + + /// The join between the read and the config: a selectable request lands on + /// the pod, a bootstrap-only one is refused. + #[test] + fn requested_service_account_resolves_against_the_selectable_set() { + let config = KubernetesComputeConfig { + service_account_name: "openshell-sandbox".to_string(), + additional_bootstrap_service_account_names: vec![ + "openshell-sandbox-external".to_string(), + ], + selectable_service_account_names: vec!["openshell-sandbox-3".to_string()], + ..Default::default() + }; + let requested = |name: &str| { + let sandbox = sandbox_requesting_service_account(Some(Value { + kind: Some(Kind::StringValue(name.to_string())), + })); + config.resolve_pod_service_account(requested_pod_service_account(&sandbox).as_deref()) + }; + + assert_eq!( + requested("openshell-sandbox-3").unwrap(), + "openshell-sandbox-3" + ); + assert!(requested("openshell-sandbox-external").is_err()); + assert_eq!( + config + .resolve_pod_service_account( + requested_pod_service_account(&Sandbox::default()).as_deref() + ) + .unwrap(), + "openshell-sandbox" + ); + } + #[test] fn platform_config_bool_extracts_value() { let template = SandboxTemplate { diff --git a/crates/openshell-driver-kubernetes/src/lib.rs b/crates/openshell-driver-kubernetes/src/lib.rs index 1a234385c6..601e9b7e09 100644 --- a/crates/openshell-driver-kubernetes/src/lib.rs +++ b/crates/openshell-driver-kubernetes/src/lib.rs @@ -9,7 +9,8 @@ pub use config::{ AppArmorProfile, DEFAULT_GATEWAY_ID, DEFAULT_PROXY_UID, DEFAULT_SANDBOX_SERVICE_ACCOUNT_NAME, DEFAULT_WORKSPACE_STORAGE_SIZE, KubernetesComputeConfig, KubernetesSidecarConfig, ManagedSshIngressConfig, OperatorNamespaceAllowlist, SupervisorSideloadMethod, - SupervisorTopology, WorkspaceMode, managed_namespace_prefix, + SupervisorTopology, WorkspaceMode, is_service_account_name, managed_namespace_prefix, + service_account_name_set, }; pub use driver::{KubernetesComputeDriver, KubernetesDriverError}; pub use grpc::ComputeDriverService; diff --git a/crates/openshell-driver-kubernetes/src/main.rs b/crates/openshell-driver-kubernetes/src/main.rs index 30b4fcada9..20c6856a3b 100644 --- a/crates/openshell-driver-kubernetes/src/main.rs +++ b/crates/openshell-driver-kubernetes/src/main.rs @@ -233,6 +233,15 @@ async fn main() -> Result<()> { operator_namespace_label: args.operator_namespace_label, operator_namespace_file: args.operator_namespace_file, service_account_name: args.sandbox_service_account, + // Bootstrap TokenReview runs in the gateway, which reads its own + // `[openshell.drivers.kubernetes]` table for the accepted set. + additional_bootstrap_service_account_names: Vec::new(), + // This binary has no config file and no flag for the selectable + // set, and resolution runs in the driver, so per-sandbox selection + // is unavailable out-of-process: any account other than + // --sandbox-service-account is rejected. Selection needs the + // gateway's built-in driver, which reads the TOML table. + selectable_service_account_names: Vec::new(), default_image: args.sandbox_image.unwrap_or_default(), image_pull_policy: args.sandbox_image_pull_policy.unwrap_or_default(), image_pull_secrets: args.sandbox_image_pull_secrets, diff --git a/crates/openshell-server/src/auth/k8s_sa.rs b/crates/openshell-server/src/auth/k8s_sa.rs index 32cb2e119c..6e47fb2d3c 100644 --- a/crates/openshell-server/src/auth/k8s_sa.rs +++ b/crates/openshell-server/src/auth/k8s_sa.rs @@ -26,7 +26,8 @@ use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; use kube::Error as KubeError; use kube::api::{Api, ApiResource, PostParams}; use kube::core::{DynamicObject, gvk::GroupVersionKind}; -use openshell_driver_kubernetes::OperatorNamespaceAllowlist; +use openshell_driver_kubernetes::{KubernetesComputeConfig, OperatorNamespaceAllowlist}; +use std::collections::BTreeSet; use std::sync::Arc; use tonic::Status; use tracing::{debug, info, warn}; @@ -159,9 +160,76 @@ impl NamespaceValidator { } } +/// Validates the `ServiceAccount` name extracted from an SA token username +/// against the set an operator accepts for sandbox bootstrap. +/// +/// The Kubernetes driver writes one resolved `serviceAccountName` onto each +/// sandbox pod, `service_account_name` unless the request selected another +/// account, and every name it can resolve to is a member whenever it is +/// non-empty. Deployments where something other than the driver assigns the +/// pod's `ServiceAccount`, such as a mutating admission policy or an external +/// controller that owns the sandbox pods, add those identities through +/// `additional_bootstrap_service_account_names`. Accounts a caller may request +/// per sandbox (`selectable_service_account_names`) are members too, since a +/// pod running as one has to authenticate. The set stays operator-configured +/// and closed; this widens which identities may bootstrap, not how they are +/// verified. +/// +/// Matching is an exact comparison of the bare name and is independent of the +/// namespace, which [`NamespaceValidator`] checks separately. Under managed and +/// operator workspace modes a name is therefore accepted in every namespace +/// that validator accepts. +#[derive(Debug)] +pub struct ServiceAccountValidator { + accepted: BTreeSet, +} + +impl ServiceAccountValidator { + /// `pod_default` is the driver's `service_account_name`; `additional` are + /// the names accepted for bootstrap only. + /// + /// Names are trimmed, because a `ServiceAccount` name never carries + /// surrounding whitespace and a padded config entry would otherwise sit in + /// the set as a member nothing can ever match. Entries that are empty + /// after trimming are dropped, so a blank config line cannot widen the + /// set. An empty `pod_default` is dropped on the same rule: the driver + /// omits `serviceAccountName` from the pod in that case, and the resulting + /// set accepts only what `additional` supplies. + #[cfg(test)] + pub fn new(pod_default: &str, additional: impl IntoIterator>) -> Self { + let owned: Vec = std::iter::once(pod_default.to_string()) + .chain(additional.into_iter().map(Into::into)) + .collect(); + Self { + accepted: openshell_driver_kubernetes::service_account_name_set( + owned.iter().map(String::as_str), + ), + } + } + + /// Build the accepted set from the Kubernetes driver configuration the + /// gateway reads for bootstrap. Selectable accounts are included, since an + /// account a caller may run a sandbox as has to be able to authenticate. + pub fn from_kubernetes_config(config: &KubernetesComputeConfig) -> Self { + Self { + accepted: config.accepted_bootstrap_service_account_names(), + } + } + + pub fn accepts(&self, service_account: &str) -> bool { + self.accepted.contains(service_account) + } + + /// The accepted names, for startup and rejection diagnostics. + pub fn accepted(&self) -> &BTreeSet { + &self.accepted + } +} + #[derive(Debug)] struct TokenReviewIdentity { namespace: String, + service_account: String, pod_name: String, pod_uid: String, } @@ -180,7 +248,7 @@ pub struct LiveK8sResolver { token_reviews_api: Api, expected_audience: String, namespace_validator: NamespaceValidator, - expected_service_account: String, + service_account_validator: ServiceAccountValidator, } impl LiveK8sResolver { @@ -188,7 +256,7 @@ impl LiveK8sResolver { client: kube::Client, namespace_validator: NamespaceValidator, expected_audience: String, - expected_service_account: String, + service_account_validator: ServiceAccountValidator, ) -> Self { let token_reviews_api: Api = Api::all(client.clone()); Self { @@ -196,7 +264,7 @@ impl LiveK8sResolver { token_reviews_api, expected_audience, namespace_validator, - expected_service_account, + service_account_validator, } } @@ -262,7 +330,7 @@ impl K8sIdentityResolver for LiveK8sResolver { &status, &self.expected_audience, &self.namespace_validator, - &self.expected_service_account, + &self.service_account_validator, )? else { return Ok(None); @@ -272,7 +340,7 @@ impl K8sIdentityResolver for LiveK8sResolver { pod_name = %identity.pod_name, pod_uid = %identity.pod_uid, namespace = %identity.namespace, - service_account = %self.expected_service_account, + service_account = %identity.service_account, "validated K8s SA token via TokenReview" ); @@ -346,7 +414,7 @@ fn token_review_identity( status: &TokenReviewStatus, expected_audience: &str, namespace_validator: &NamespaceValidator, - expected_service_account: &str, + service_account_validator: &ServiceAccountValidator, ) -> Result, Status> { if status.authenticated != Some(true) { debug!( @@ -383,15 +451,15 @@ fn token_review_identity( Status::permission_denied("SA token username format not recognized") })?; - if sa_name != expected_service_account { + if !service_account_validator.accepts(&sa_name) { warn!( username = %username, service_account = %sa_name, - expected = %expected_service_account, - "K8s TokenReview principal is not the configured sandbox service account" + accepted_service_accounts = ?service_account_validator.accepted(), + "K8s TokenReview principal is not an accepted sandbox service account" ); return Err(Status::permission_denied( - "SA token is not from the configured sandbox service account", + "SA token is not from an accepted sandbox service account", )); } @@ -410,6 +478,7 @@ fn token_review_identity( let pod_uid = user_extra_one(user, POD_UID_EXTRA)?; Ok(Some(TokenReviewIdentity { namespace, + service_account: sa_name, pod_name, pod_uid, })) @@ -717,6 +786,10 @@ mod tests { NamespaceValidator::Exact(ns.to_string()) } + fn sa_validator(pod_default: &str) -> ServiceAccountValidator { + ServiceAccountValidator::new(pod_default, std::iter::empty::<&str>()) + } + #[test] fn token_review_identity_extracts_pod_binding() { let status = token_review_status( @@ -730,9 +803,14 @@ mod tests { ); let validator = exact_validator("openshell"); - let identity = token_review_identity(&status, "openshell-gateway", &validator, "default") - .unwrap() - .expect("authenticated token should resolve"); + let identity = token_review_identity( + &status, + "openshell-gateway", + &validator, + &sa_validator("default"), + ) + .unwrap() + .expect("authenticated token should resolve"); assert_eq!(identity.namespace, "openshell"); assert_eq!(identity.pod_name, "openshell-sandbox-a"); @@ -749,9 +827,14 @@ mod tests { let validator = exact_validator("openshell"); assert!( - token_review_identity(&status, "openshell-gateway", &validator, "default") - .unwrap() - .is_none() + token_review_identity( + &status, + "openshell-gateway", + &validator, + &sa_validator("default") + ) + .unwrap() + .is_none() ); } @@ -768,8 +851,13 @@ mod tests { ); let validator = exact_validator("openshell"); - let err = token_review_identity(&status, "openshell-gateway", &validator, "default") - .expect_err("wrong audience must fail closed"); + let err = token_review_identity( + &status, + "openshell-gateway", + &validator, + &sa_validator("default"), + ) + .expect_err("wrong audience must fail closed"); assert_eq!(err.code(), tonic::Code::Unauthenticated); } @@ -786,8 +874,13 @@ mod tests { ); let validator = exact_validator("openshell"); - let err = token_review_identity(&status, "openshell-gateway", &validator, "default") - .expect_err("other namespace must be rejected"); + let err = token_review_identity( + &status, + "openshell-gateway", + &validator, + &sa_validator("default"), + ) + .expect_err("other namespace must be rejected"); assert_eq!(err.code(), tonic::Code::PermissionDenied); } @@ -804,8 +897,13 @@ mod tests { ); let validator = exact_validator("openshell"); - let err = token_review_identity(&status, "openshell-gateway", &validator, "default") - .expect_err("other service account must be rejected"); + let err = token_review_identity( + &status, + "openshell-gateway", + &validator, + &sa_validator("default"), + ) + .expect_err("other service account must be rejected"); assert_eq!(err.code(), tonic::Code::PermissionDenied); } @@ -819,11 +917,165 @@ mod tests { ); let validator = exact_validator("openshell"); - let err = token_review_identity(&status, "openshell-gateway", &validator, "default") - .expect_err("non pod-bound tokens must be rejected"); + let err = token_review_identity( + &status, + "openshell-gateway", + &validator, + &sa_validator("default"), + ) + .expect_err("non pod-bound tokens must be rejected"); assert_eq!(err.code(), tonic::Code::PermissionDenied); } + /// A non-empty pod default is a member, so a gateway with no additional + /// names behaves exactly as it did before the accepted set existed. + #[test] + fn service_account_validator_accepts_only_pod_default_by_default() { + let v = sa_validator("openshell-sandbox"); + + assert!(v.accepts("openshell-sandbox")); + assert!(!v.accepts("openshell-sandbox-2")); + assert_eq!(v.accepted().len(), 1); + } + + #[test] + fn service_account_validator_accepts_every_configured_name() { + let v = ServiceAccountValidator::new( + "openshell-sandbox", + ["openshell-sandbox-2", "openshell-sandbox-3"], + ); + + assert!(v.accepts("openshell-sandbox")); + assert!(v.accepts("openshell-sandbox-2")); + assert!(v.accepts("openshell-sandbox-3")); + assert!(!v.accepts("openshell-sandbox-other")); + assert!(!v.accepts("")); + assert_eq!(v.accepted().len(), 3); + } + + /// A blank or padded config entry must not enter the accepted set as a + /// member nothing can match. `parse_sa_username` already rejects an empty + /// name, so this is hygiene for the startup and rejection logs and it + /// keeps an empty pod default from looking like a configured identity. + #[test] + fn service_account_validator_drops_blank_and_trims_padded_names() { + let v = ServiceAccountValidator::new("", ["", " ", " openshell-sandbox-2 "]); + + assert!(!v.accepts("")); + assert!( + v.accepts("openshell-sandbox-2"), + "padded entry should be trimmed" + ); + assert_eq!(v.accepted().len(), 1); + } + + /// The pod default and an identical additional entry collapse, so neither + /// operator-facing log record prints a duplicate. + #[test] + fn service_account_validator_collapses_duplicates() { + let v = ServiceAccountValidator::new( + "openshell-sandbox", + ["openshell-sandbox", "openshell-sandbox-2"], + ); + + assert_eq!(v.accepted().len(), 2); + } + + /// A caller-selectable account has to authenticate when its pod bootstraps, + /// so the validator accepts both lists even though only one of them is + /// selectable. + #[test] + fn service_account_validator_accepts_bootstrap_and_selectable_names() { + let config = KubernetesComputeConfig { + service_account_name: "openshell-sandbox".to_string(), + additional_bootstrap_service_account_names: vec![ + "openshell-sandbox-external".to_string(), + ], + selectable_service_account_names: vec!["openshell-sandbox-3".to_string()], + ..Default::default() + }; + + let v = ServiceAccountValidator::from_kubernetes_config(&config); + + assert!(v.accepts("openshell-sandbox")); + assert!(v.accepts("openshell-sandbox-external")); + assert!(v.accepts("openshell-sandbox-3")); + assert!(!v.accepts("openshell-sandbox-other")); + assert_eq!(v.accepted().len(), 3); + } + + #[test] + fn token_review_identity_accepts_additional_service_account() { + let status = token_review_status( + true, + vec!["openshell-gateway"], + "system:serviceaccount:openshell:openshell-sandbox-2", + vec![ + (POD_NAME_EXTRA, "openshell-sandbox-a"), + (POD_UID_EXTRA, "uid-a"), + ], + ); + let validator = exact_validator("openshell"); + let service_accounts = + ServiceAccountValidator::new("openshell-sandbox", ["openshell-sandbox-2"]); + + let identity = + token_review_identity(&status, "openshell-gateway", &validator, &service_accounts) + .unwrap() + .expect("an enrolled service account should authenticate"); + + assert_eq!(identity.service_account, "openshell-sandbox-2"); + assert_eq!(identity.pod_name, "openshell-sandbox-a"); + } + + #[test] + fn token_review_identity_rejects_service_account_outside_accepted_set() { + let status = token_review_status( + true, + vec!["openshell-gateway"], + "system:serviceaccount:openshell:openshell-sandbox-other", + vec![ + (POD_NAME_EXTRA, "openshell-sandbox-a"), + (POD_UID_EXTRA, "uid-a"), + ], + ); + let validator = exact_validator("openshell"); + let service_accounts = + ServiceAccountValidator::new("openshell-sandbox", ["openshell-sandbox-2"]); + + let err = + token_review_identity(&status, "openshell-gateway", &validator, &service_accounts) + .expect_err("a non-member service account must be rejected"); + assert_eq!(err.code(), tonic::Code::PermissionDenied); + } + + /// The presented account is carried out of validation, so the success log + /// names the identity that actually bootstrapped rather than the pod + /// default. The two differ here, which is what makes the assertion mean + /// something. + #[test] + fn token_review_identity_reports_presented_service_account() { + let status = token_review_status( + true, + vec!["openshell-gateway"], + "system:serviceaccount:openshell:openshell-sandbox-2", + vec![ + (POD_NAME_EXTRA, "openshell-sandbox-a"), + (POD_UID_EXTRA, "uid-a"), + ], + ); + let validator = exact_validator("openshell"); + let service_accounts = + ServiceAccountValidator::new("openshell-sandbox", ["openshell-sandbox-2"]); + + let identity = + token_review_identity(&status, "openshell-gateway", &validator, &service_accounts) + .unwrap() + .expect("authenticated token should resolve"); + + assert_eq!(identity.service_account, "openshell-sandbox-2"); + } + #[test] fn namespace_validator_exact_accepts_matching() { let v = NamespaceValidator::Exact("openshell".to_string()); @@ -842,7 +1094,7 @@ mod tests { #[test] fn namespace_validator_allowlist_accepts_known_namespaces() { - let al = OperatorNamespaceAllowlist::from_set(std::collections::BTreeSet::from([ + let al = OperatorNamespaceAllowlist::from_set(BTreeSet::from([ "ns-a".to_string(), "ns-b".to_string(), ])); @@ -865,9 +1117,14 @@ mod tests { ); let validator = NamespaceValidator::Prefix("openshell-gw1-".to_string()); - let identity = token_review_identity(&status, "openshell-gateway", &validator, "default") - .unwrap() - .expect("managed namespace token should resolve"); + let identity = token_review_identity( + &status, + "openshell-gateway", + &validator, + &sa_validator("default"), + ) + .unwrap() + .expect("managed namespace token should resolve"); assert_eq!(identity.namespace, "openshell-gw1-workspace-a"); } diff --git a/crates/openshell-server/src/compute/driver_config/builtin.rs b/crates/openshell-server/src/compute/driver_config/builtin.rs index 948c982dd2..dcaa440877 100644 --- a/crates/openshell-server/src/compute/driver_config/builtin.rs +++ b/crates/openshell-server/src/compute/driver_config/builtin.rs @@ -192,6 +192,62 @@ service_account_name = "sandbox-sa" let cfg = kubernetes_config_for_k8s_sa_bootstrap(Some(&file)).unwrap(); assert_eq!(cfg.namespace, "sandboxes"); assert_eq!(cfg.service_account_name, "sandbox-sa"); + assert!(cfg.additional_bootstrap_service_account_names.is_empty()); + } + + /// The TOML key the Helm chart renders must reach the field the bootstrap + /// authenticator reads. `KubernetesComputeConfig` denies unknown fields, so + /// a key/field mismatch would refuse to start every gateway in a fleet. + #[test] + fn k8s_sa_bootstrap_reads_additional_service_accounts_from_driver_table() { + let file: config_file::ConfigFile = toml::from_str( + r#" +[openshell.gateway] + +[openshell.drivers.kubernetes] +namespace = "sandboxes" +service_account_name = "sandbox-sa" +additional_bootstrap_service_account_names = ["sandbox-alt", "sandbox-legacy"] +selectable_service_account_names = ["sandbox-selectable"] +"#, + ) + .expect("valid config"); + + let cfg = kubernetes_config_for_k8s_sa_bootstrap(Some(&file)).unwrap(); + assert_eq!( + cfg.additional_bootstrap_service_account_names, + vec!["sandbox-alt".to_string(), "sandbox-legacy".to_string()] + ); + + assert_eq!( + cfg.selectable_service_account_names, + vec!["sandbox-selectable".to_string()] + ); + + let validator = crate::auth::k8s_sa::ServiceAccountValidator::from_kubernetes_config(&cfg); + assert!( + validator.accepts("sandbox-sa"), + "pod default stays accepted" + ); + assert!(validator.accepts("sandbox-alt")); + assert!(validator.accepts("sandbox-legacy")); + assert!( + validator.accepts("sandbox-selectable"), + "an account a caller may select has to authenticate" + ); + assert!(!validator.accepts("sandbox-other")); + + // The selectable key governs what a caller may ask for; the + // bootstrap-only key must not. + assert_eq!( + cfg.resolve_pod_service_account(Some("sandbox-selectable")) + .unwrap(), + "sandbox-selectable" + ); + assert!( + cfg.resolve_pod_service_account(Some("sandbox-alt")) + .is_err() + ); } #[test] diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index fe6bf62c91..6de880092f 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -75,7 +75,7 @@ use tonic::transport::Endpoint; use tonic::{Code, Request, Status}; #[cfg(unix)] use tower::service_fn; -use tracing::{Instrument as _, debug, info, warn}; +use tracing::{Instrument as _, debug, error, info, warn}; type DriverWatchStream = Pin> + Send>>; type SharedComputeDriver = @@ -964,32 +964,31 @@ impl ComputeRuntime { } Ok(sandbox) } - Err(status) if status.code() == Code::AlreadyExists => { - let _ = self + Err(status) => { + if let Err(error) = self .store .delete(Sandbox::object_type(), sandbox.object_id()) - .await; - self.sandbox_index.remove_sandbox(sandbox.object_id()); - Err(Status::already_exists("sandbox already exists")) - } - Err(status) if status.code() == Code::FailedPrecondition => { - let _ = self - .store - .delete(Sandbox::object_type(), sandbox.object_id()) - .await; - self.sandbox_index.remove_sandbox(sandbox.object_id()); - Err(Status::failed_precondition(status.message().to_string())) - } - Err(err) => { - let _ = self - .store - .delete(Sandbox::object_type(), sandbox.object_id()) - .await; + .await + { + error!( + sandbox_id = %sandbox.object_id(), + %error, + "failed to roll back the sandbox record after the compute driver \ + rejected the create; the record is orphaned" + ); + } self.sandbox_index.remove_sandbox(sandbox.object_id()); - Err(Status::internal(format!( - "create sandbox failed: {}", - err.message() - ))) + Err(match status.code() { + Code::AlreadyExists => Status::already_exists("sandbox already exists"), + Code::FailedPrecondition => { + Status::failed_precondition(status.message().to_string()) + } + // A driver rejecting the request is the caller's problem, + // not a server fault: preserve the code so clients do not + // retry a request that cannot succeed. + Code::InvalidArgument => Status::invalid_argument(status.message().to_string()), + _ => Status::internal(format!("create sandbox failed: {}", status.message())), + }) } } } @@ -3307,6 +3306,18 @@ fn driver_sandbox_template_from_public( template: &SandboxTemplate, driver_name: &str, ) -> Result> { + // Only the Kubernetes driver has ServiceAccounts. Rejecting here names the + // field the caller actually set: the Docker and VM drivers refuse any + // populated `platform_config` with a message that does not, and the Podman + // driver ignores it entirely, which would acknowledge a request to confine + // a sandbox to an identity and then quietly not do it. + if !template.service_account_name.is_empty() && driver_name != "kubernetes" { + return Err(Box::new(Status::invalid_argument(format!( + "template.service_account_name is only supported by the kubernetes compute driver; \ + this gateway runs the '{driver_name}' driver" + )))); + } + Ok(DriverSandboxTemplate { image: template.image.clone(), agent_socket_path: template.agent_socket.clone(), @@ -3397,6 +3408,17 @@ fn build_platform_config(template: &SandboxTemplate) -> Option Self { + Self(TestDriver::default(), status) + } + } - #[tonic::async_trait] - impl ComputeDriver for FailingDriver { - type WatchSandboxesStream = DriverWatchStream; + #[tonic::async_trait] + impl ComputeDriver for FailingDriver { + type WatchSandboxesStream = DriverWatchStream; - async fn create_sandbox( - &self, - _request: Request, - ) -> Result, Status> { - Err(Status::unavailable("driver is down")) - } + async fn create_sandbox( + &self, + _request: Request, + ) -> Result, Status> { + Err(self.1.clone()) + } - async fn get_capabilities( - &self, - request: Request, - ) -> Result, Status> { - self.0.get_capabilities(request).await - } + async fn get_capabilities( + &self, + request: Request, + ) -> Result, Status> { + self.0.get_capabilities(request).await + } - async fn get_gateway_listener_requirements( - &self, - request: Request, - ) -> Result, Status> - { - self.0.get_gateway_listener_requirements(request).await - } + async fn get_gateway_listener_requirements( + &self, + request: Request, + ) -> Result, Status> { + self.0.get_gateway_listener_requirements(request).await + } - async fn validate_sandbox_create( - &self, - request: Request, - ) -> Result, Status> { - self.0.validate_sandbox_create(request).await - } + async fn validate_sandbox_create( + &self, + request: Request, + ) -> Result, Status> { + self.0.validate_sandbox_create(request).await + } - async fn get_sandbox( - &self, - request: Request, - ) -> Result, Status> { - self.0.get_sandbox(request).await - } + async fn get_sandbox( + &self, + request: Request, + ) -> Result, Status> { + self.0.get_sandbox(request).await + } - async fn list_sandboxes( - &self, - request: Request, - ) -> Result< - tonic::Response, - Status, - > { - self.0.list_sandboxes(request).await - } + async fn list_sandboxes( + &self, + request: Request, + ) -> Result< + tonic::Response, + Status, + > { + self.0.list_sandboxes(request).await + } - async fn stop_sandbox( - &self, - request: Request, - ) -> Result, Status> { - self.0.stop_sandbox(request).await - } + async fn stop_sandbox( + &self, + request: Request, + ) -> Result, Status> { + self.0.stop_sandbox(request).await + } - async fn start_sandbox( - &self, - request: Request, - ) -> Result, Status> { - self.0.start_sandbox(request).await - } + async fn start_sandbox( + &self, + request: Request, + ) -> Result, Status> { + self.0.start_sandbox(request).await + } - async fn delete_sandbox( - &self, - request: Request, - ) -> Result, Status> { - self.0.delete_sandbox(request).await - } + async fn delete_sandbox( + &self, + request: Request, + ) -> Result, Status> { + self.0.delete_sandbox(request).await + } - async fn watch_sandboxes( - &self, - request: Request, - ) -> Result, Status> { - self.0.watch_sandboxes(request).await - } + async fn watch_sandboxes( + &self, + request: Request, + ) -> Result, Status> { + self.0.watch_sandboxes(request).await + } - async fn ensure_workspace( - &self, - request: Request, - ) -> Result, Status> { - self.0.ensure_workspace(request).await - } + async fn ensure_workspace( + &self, + request: Request, + ) -> Result, Status> { + self.0.ensure_workspace(request).await + } - async fn delete_workspace( - &self, - request: Request, - ) -> Result, Status> { - self.0.delete_workspace(request).await - } + async fn delete_workspace( + &self, + request: Request, + ) -> Result, Status> { + self.0.delete_workspace(request).await } + } + + /// A driver rejecting the request must reach the caller as + /// `InvalidArgument` carrying the driver's own message, not as `Internal` + /// with a wrapper: clients retry `Internal`, and no retry fixes a bad + /// request. The rejected create must also leave no record behind. + #[tokio::test] + async fn create_sandbox_preserves_invalid_argument_from_the_driver() { + let runtime = test_runtime(Arc::new(FailingDriver::new(Status::invalid_argument( + "service_account_name 'nope' is not selectable on this gateway", + )))) + .await; + let sandbox = sandbox_record("sb-reject", "sandbox-reject", SandboxPhase::Provisioning); + + let err = runtime + .create_sandbox(sandbox, None) + .await + .expect_err("driver rejects the request"); - let runtime = test_runtime(Arc::new(FailingDriver::default())).await; + assert_eq!(err.code(), Code::InvalidArgument); + assert_eq!( + err.message(), + "service_account_name 'nope' is not selectable on this gateway", + "the driver's message must reach the caller unwrapped" + ); + assert!( + runtime + .store + .get_message::("sb-reject") + .await + .unwrap() + .is_none(), + "a rejected create must not leave a sandbox record behind" + ); + } + + /// A failing driver call must be visible as a failure in the trace, not + /// just as a span that happens to be followed by nothing. + #[tokio::test] + async fn failed_driver_calls_are_marked_on_the_span() { + use tracing::Instrument as _; + + use crate::otel_tracing::test_exporter; + + let runtime = test_runtime(Arc::new(FailingDriver::new(Status::unavailable( + "driver is down", + )))) + .await; let sandbox = sandbox_record("sb-fail", "sandbox-fail", SandboxPhase::Provisioning); let traced = test_exporter::install_traced(); @@ -8580,6 +8644,61 @@ mod tests { } } + /// The requested account has to reach the driver, which is the only layer + /// that knows which accounts an operator made selectable. + #[test] + fn build_platform_config_forwards_requested_service_account() { + use prost_types::value::Kind; + + let template = SandboxTemplate { + service_account_name: "openshell-sandbox-3".to_string(), + ..SandboxTemplate::default() + }; + let config = build_platform_config(&template).expect("config should be Some"); + assert_eq!( + config + .fields + .get("service_account_name") + .and_then(|v| v.kind.clone()), + Some(Kind::StringValue("openshell-sandbox-3".to_string())) + ); + } + + /// Only the Kubernetes driver has `ServiceAccount`s. The other drivers must + /// not silently ignore the request (Podman) or fail with a message naming + /// `platform_config`, which the caller never set (Docker, VM). + #[test] + fn driver_template_rejects_a_service_account_request_for_other_drivers() { + let template = SandboxTemplate { + service_account_name: "openshell-sandbox-3".to_string(), + ..SandboxTemplate::default() + }; + + for driver in ["podman", "docker", "vm"] { + let err = driver_sandbox_template_from_public(&template, driver) + .expect_err("only the kubernetes driver supports the field"); + assert_eq!(err.code(), Code::InvalidArgument); + assert!( + err.message().contains("service_account_name"), + "{}", + err.message() + ); + assert!(err.message().contains(driver), "{}", err.message()); + } + + assert!(driver_sandbox_template_from_public(&template, "kubernetes").is_ok()); + } + + #[test] + fn build_platform_config_omits_an_unset_service_account() { + let template = SandboxTemplate { + runtime_class_name: "gvisor".to_string(), + ..SandboxTemplate::default() + }; + let config = build_platform_config(&template).expect("config should be Some"); + assert!(!config.fields.contains_key("service_account_name")); + } + #[test] fn build_platform_config_inverts_user_namespaces_to_host_users() { use prost_types::value::Kind; diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index f64a539d9d..f289d11963 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -222,6 +222,7 @@ fn validate_sandbox_template(tmpl: &SandboxTemplate) -> Result<(), Status> { ("template.image", &tmpl.image), ("template.runtime_class_name", &tmpl.runtime_class_name), ("template.agent_socket", &tmpl.agent_socket), + ("template.service_account_name", &tmpl.service_account_name), ] { if value.len() > MAX_TEMPLATE_STRING_LEN { return Err(Status::invalid_argument(format!( @@ -231,6 +232,18 @@ fn validate_sandbox_template(tmpl: &SandboxTemplate) -> Result<(), Status> { } } + // A requested ServiceAccount is echoed back in the driver's rejection and + // reaches the apiserver as `spec.serviceAccountName`, so reject anything + // Kubernetes could not have issued before it travels any further. + if !tmpl.service_account_name.is_empty() + && !openshell_driver_kubernetes::is_service_account_name(tmpl.service_account_name.trim()) + { + return Err(Status::invalid_argument( + "template.service_account_name must be a valid Kubernetes ServiceAccount name \ + (DNS-1123 subdomain, at most 253 characters)", + )); + } + // Map fields. validate_string_map( &tmpl.labels, @@ -938,6 +951,42 @@ pub(super) fn level_matches(log_level: &str, min_level: &str) -> bool { #[cfg(test)] mod tests { + + /// The requested name is echoed in the driver's rejection and reaches the + /// apiserver, so an over-long or malformed value must stop at the gateway. + #[test] + fn validate_sandbox_template_bounds_the_service_account_name() { + let long = SandboxTemplate { + service_account_name: "a".repeat(MAX_TEMPLATE_STRING_LEN + 1), + ..SandboxTemplate::default() + }; + let err = validate_sandbox_template(&long).unwrap_err(); + assert!( + err.message().contains("template.service_account_name"), + "{}", + err.message() + ); + + for bad in ["Openshell-Sandbox", "has space", "under_score", " "] { + let tmpl = SandboxTemplate { + service_account_name: bad.to_string(), + ..SandboxTemplate::default() + }; + let err = validate_sandbox_template(&tmpl).unwrap_err(); + assert!( + err.message() + .contains("valid Kubernetes ServiceAccount name"), + "{}", + err.message() + ); + } + + let ok = SandboxTemplate { + service_account_name: "openshell-sandbox-3".to_string(), + ..SandboxTemplate::default() + }; + assert!(validate_sandbox_template(&ok).is_ok()); + } use super::*; use openshell_core::proto::SandboxSpec; use std::collections::HashMap; diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index 218e9396f0..2e8a68abdb 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -668,14 +668,16 @@ pub(crate) async fn run_server( // and without the issuer there's nothing to exchange the SA token for. #[cfg(not(target_os = "windows"))] if state.sandbox_jwt_issuer.is_some() && std::env::var_os("KUBERNETES_SERVICE_HOST").is_some() { - // Pod lookups and TokenReview identity checks must match the sandbox - // namespace and service account used by the Kubernetes driver. + // Pod lookups use the Kubernetes driver's sandbox namespace; the + // TokenReview identity check accepts the driver's sandbox service + // account plus any additional names configured for bootstrap. let kubernetes_config = compute::driver_config::builtin::kubernetes_config_for_k8s_sa_bootstrap( config_file.as_ref(), )?; let sandbox_namespace = kubernetes_config.namespace.clone(); - let sandbox_service_account = kubernetes_config.service_account_name.clone(); + let service_account_validator = + auth::k8s_sa::ServiceAccountValidator::from_kubernetes_config(&kubernetes_config); let namespace_validator = match kubernetes_config.workspace_mode { openshell_driver_kubernetes::WorkspaceMode::Shared => { auth::k8s_sa::NamespaceValidator::Exact(kubernetes_config.namespace) @@ -694,19 +696,19 @@ pub(crate) async fn run_server( }; match kube::Client::try_default().await { Ok(client) => { + info!( + namespace = %sandbox_namespace, + accepted_service_accounts = ?service_account_validator.accepted(), + "K8s ServiceAccount bootstrap authenticator enabled" + ); let resolver = Arc::new(auth::k8s_sa::LiveK8sResolver::new( client, namespace_validator, "openshell-gateway".to_string(), - sandbox_service_account.clone(), + service_account_validator, )); let authenticator = auth::k8s_sa::K8sServiceAccountAuthenticator::new(resolver); state.k8s_sa_authenticator = Some(Arc::new(authenticator)); - info!( - namespace = %sandbox_namespace, - service_account = %sandbox_service_account, - "K8s ServiceAccount bootstrap authenticator enabled" - ); } Err(e) => warn!( error = %e, diff --git a/deploy/helm/openshell/README.md b/deploy/helm/openshell/README.md index 93dab354b6..0e082b8aa1 100644 --- a/deploy/helm/openshell/README.md +++ b/deploy/helm/openshell/README.md @@ -229,8 +229,10 @@ add `ci/values-spire.yaml` to the OpenShell release values files. | server.dbUrl | string | `"sqlite:/var/openshell/openshell.db"` | Gateway database URL (used for the default SQLite backend). | | server.defaultRuntimeClassName | string | `""` | Default Kubernetes runtimeClassName for sandbox pods. Applied when a CreateSandbox request does not specify one. Empty (default) = omit the field, using the cluster's default RuntimeClass. Set to a RuntimeClass name (e.g. "kata-containers", "nvidia") to apply it to all sandboxes that don't explicitly override it. | | server.disableTls | bool | `false` | Disable TLS entirely - the server listens on plaintext HTTP. Set to true when a reverse proxy / tunnel terminates TLS at the edge. | +| server.drivers.kubernetes.additionalBootstrapServiceAccountNames | list | `[]` | Extra ServiceAccount names the gateway accepts when a sandbox bootstraps, on top of the sandbox ServiceAccount the driver assigns. A pod spec has a single serviceAccountName field, so this is a separate setting. Use it where something else assigns that field, such as a mutating admission policy or an external controller that owns the sandbox pods. Names are matched exactly and ignore the namespace, so under managed and operator workspace modes an entry is accepted in every namespace the gateway accepts. Empty (default) adds nothing to the accepted set. | | server.drivers.kubernetes.operatorNamespaceFile | string | `""` | Path to a JSON file containing an array of namespace names allowed in operator mode. Hot-reloaded on change. | | server.drivers.kubernetes.operatorNamespaceLabel | string | `""` | K8s label selector for namespace discovery in operator mode. The driver watches namespaces matching this label. | +| server.drivers.kubernetes.selectableServiceAccountNames | list | `[]` | ServiceAccount names a caller may request for an individual sandbox through SandboxTemplate.service_account_name. These are also accepted for bootstrap, since a sandbox running as one has to authenticate. Kept separate from additionalBootstrapServiceAccountNames on purpose: accepting an identity that something else assigns is not the same decision as letting any caller ask to run as it. The driver's own sandbox ServiceAccount is selectable whenever it is set. Empty (default) leaves the driver default as the only account a caller can request. Every member is available to every caller who can create a sandbox; selection is not scoped per caller or per workspace. The accounts must already exist in each namespace the gateway uses, so this suits workspace_mode shared or operator rather than managed, where namespaces are created on demand. | | server.drivers.kubernetes.workspaceMode | string | `"shared"` | How workspaces map to Kubernetes namespaces. "shared" (default): all sandboxes in a single namespace. "managed": auto-creates per-workspace namespaces. "operator": uses pre-provisioned namespaces. | | server.enableLoopbackServiceHttp | bool | `true` | Enable plaintext HTTP routing for loopback sandbox service URLs on TLS-enabled gateways. | | server.enableUserNamespaces | bool | `false` | Enable Kubernetes user namespace isolation (hostUsers: false) for sandbox pods. Requires Kubernetes 1.33+ with user namespace support available (beta through 1.35, GA in 1.36+), plus a supporting container runtime and Linux 5.12+. When enabled, container UID 0 maps to an unprivileged host UID and capabilities become namespaced. | diff --git a/deploy/helm/openshell/templates/gateway-config.yaml b/deploy/helm/openshell/templates/gateway-config.yaml index 9d24dbd917..ab3ff427e9 100644 --- a/deploy/helm/openshell/templates/gateway-config.yaml +++ b/deploy/helm/openshell/templates/gateway-config.yaml @@ -139,6 +139,22 @@ data: gateway_id = {{ .Values.server.sandboxJwt.gatewayId | default (include "openshell.fullname" .) | quote }} grpc_endpoint = {{ include "openshell.grpcEndpoint" . | quote }} service_account_name = {{ include "openshell.sandboxServiceAccountName" . | quote }} + {{- with .Values.server.drivers.kubernetes.additionalBootstrapServiceAccountNames }} + {{- range $name := . }} + {{- if not (and (kindIs "string" $name) (trim $name)) }} + {{- fail "server.drivers.kubernetes.additionalBootstrapServiceAccountNames entries must be non-empty strings" }} + {{- end }} + {{- end }} + additional_bootstrap_service_account_names = [{{- range $i, $name := . }}{{ if $i }}, {{ end }}{{ $name | quote }}{{- end }}] + {{- end }} + {{- with .Values.server.drivers.kubernetes.selectableServiceAccountNames }} + {{- range $name := . }} + {{- if not (and (kindIs "string" $name) (trim $name)) }} + {{- fail "server.drivers.kubernetes.selectableServiceAccountNames entries must be non-empty strings" }} + {{- end }} + {{- end }} + selectable_service_account_names = [{{- range $i, $name := . }}{{ if $i }}, {{ end }}{{ $name | quote }}{{- end }}] + {{- end }} {{- if .Values.server.drivers.kubernetes.operatorNamespaceLabel }} operator_namespace_label = {{ .Values.server.drivers.kubernetes.operatorNamespaceLabel | quote }} {{- end }} diff --git a/deploy/helm/openshell/tests/gateway_config_test.yaml b/deploy/helm/openshell/tests/gateway_config_test.yaml index afacd01eb4..7b0da20e73 100644 --- a/deploy/helm/openshell/tests/gateway_config_test.yaml +++ b/deploy/helm/openshell/tests/gateway_config_test.yaml @@ -103,6 +103,99 @@ tests: path: data["gateway.toml"] pattern: '(?ms)\[openshell\.drivers\.kubernetes\].*?service_account_name\s*=\s*"openshell-sandbox"' + - it: omits additional bootstrap service accounts by default + template: templates/gateway-config.yaml + asserts: + - notMatchRegex: + path: data["gateway.toml"] + pattern: 'additional_bootstrap_service_account_names\s*=' + + - it: renders additional bootstrap service accounts under [openshell.drivers.kubernetes] + template: templates/gateway-config.yaml + set: + server.drivers.kubernetes.additionalBootstrapServiceAccountNames: + - openshell-sandbox-2 + - openshell-sandbox-3 + asserts: + # Line-anchored: a containment match cannot be used once a rendered array + # literal sits between the table header and the key, and both of these + # settings render arrays. + - matchRegex: + path: data["gateway.toml"] + pattern: '(?m)^\s*additional_bootstrap_service_account_names = \["openshell-sandbox-2", "openshell-sandbox-3"\]$' + # The pod default is a separate setting and must be untouched. + - matchRegex: + path: data["gateway.toml"] + pattern: '(?m)^\s*service_account_name\s+= "openshell-sandbox"$' + + - it: renders selectable service accounts under [openshell.drivers.kubernetes] + template: templates/gateway-config.yaml + set: + server.drivers.kubernetes.selectableServiceAccountNames: + - openshell-sandbox-3 + asserts: + - matchRegex: + path: data["gateway.toml"] + pattern: '(?m)^\s*selectable_service_account_names = \["openshell-sandbox-3"\]$' + + - it: omits selectable service accounts by default + template: templates/gateway-config.yaml + asserts: + - notMatchRegex: + path: data["gateway.toml"] + pattern: 'selectable_service_account_names\s*=' + + # Both keys render into the same table; a containment regex would silently + # stop matching here, which is why the assertions above are line-anchored. + - it: renders both service account lists together + template: templates/gateway-config.yaml + set: + server.drivers.kubernetes.additionalBootstrapServiceAccountNames: + - openshell-sandbox-external + server.drivers.kubernetes.selectableServiceAccountNames: + - openshell-sandbox-3 + asserts: + - matchRegex: + path: data["gateway.toml"] + pattern: '(?m)^\s*additional_bootstrap_service_account_names = \["openshell-sandbox-external"\]$' + - matchRegex: + path: data["gateway.toml"] + pattern: '(?m)^\s*selectable_service_account_names = \["openshell-sandbox-3"\]$' + + - it: fails to render when a selectable service account is blank + template: templates/statefulset.yaml + set: + server.drivers.kubernetes.selectableServiceAccountNames: + - openshell-sandbox-3 + - "" + asserts: + - failedTemplate: + errorMessage: "server.drivers.kubernetes.selectableServiceAccountNames entries must be non-empty strings" + + # `kindIs "string"` exists for this case: a null item is not a blank string. + - it: fails to render when a selectable service account is not a string + template: templates/statefulset.yaml + set: + server.drivers.kubernetes.selectableServiceAccountNames: + - 5 + asserts: + - failedTemplate: + errorMessage: "server.drivers.kubernetes.selectableServiceAccountNames entries must be non-empty strings" + + # A blank or null list item renders away silently through `quote`, leaving an + # operator with a shorter array than they configured and no error anywhere. + # Asserted against statefulset.yaml because it pulls gateway-config.yaml into + # its checksum annotation, so rendering it evaluates the guard. + - it: fails to render when an additional bootstrap service account is blank + template: templates/statefulset.yaml + set: + server.drivers.kubernetes.additionalBootstrapServiceAccountNames: + - openshell-sandbox-2 + - "" + asserts: + - failedTemplate: + errorMessage: "server.drivers.kubernetes.additionalBootstrapServiceAccountNames entries must be non-empty strings" + - it: renders combined supervisor topology by default under [openshell.drivers.kubernetes] template: templates/gateway-config.yaml asserts: diff --git a/deploy/helm/openshell/values.yaml b/deploy/helm/openshell/values.yaml index 33337c768e..29f09744de 100644 --- a/deploy/helm/openshell/values.yaml +++ b/deploy/helm/openshell/values.yaml @@ -257,6 +257,32 @@ server: # -- Path to a JSON file containing an array of namespace names # allowed in operator mode. Hot-reloaded on change. operatorNamespaceFile: "" + # -- Extra ServiceAccount names the gateway accepts when a sandbox + # bootstraps, on top of the sandbox ServiceAccount the driver assigns. + # A pod spec has a single serviceAccountName field, so this is a separate + # setting. Use it where something else assigns that field, such as a + # mutating admission policy or an external controller that owns the + # sandbox pods. Names are matched exactly and ignore the namespace, so + # under managed and operator workspace modes an entry is accepted in + # every namespace the gateway accepts. Empty (default) adds nothing to the + # accepted set. + additionalBootstrapServiceAccountNames: [] + # -- ServiceAccount names a caller may request for an individual sandbox + # through SandboxTemplate.service_account_name. These are also accepted + # for bootstrap, since a sandbox running as one has to authenticate. + # Kept separate from additionalBootstrapServiceAccountNames on purpose: + # accepting an identity that something else assigns is not the same + # decision as letting any caller ask to run as it. The driver's own + # sandbox ServiceAccount is selectable whenever it is set. Empty + # (default) leaves the driver default as the only account a caller can + # request. + # + # Every member is available to every caller who can create a sandbox; + # selection is not scoped per caller or per workspace. The accounts must + # already exist in each namespace the gateway uses, so this suits + # workspace_mode shared or operator rather than managed, where namespaces + # are created on demand. + selectableServiceAccountNames: [] # -- Disable TLS entirely - the server listens on plaintext HTTP. # Set to true when a reverse proxy / tunnel terminates TLS at the edge. disableTls: false diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index cefae0b5cb..a0f4e61419 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -453,6 +453,13 @@ workspace_mode = "shared" # gateway_id = "openshell" namespace = "agents" service_account_name = "openshell-sandbox" +# Extra ServiceAccounts accepted when a sandbox bootstraps, on top of +# service_account_name. Only needed when something other than the driver +# assigns the pod's ServiceAccount. Matched exactly, namespace-independent. +# additional_bootstrap_service_account_names = ["openshell-sandbox-2"] +# ServiceAccounts a caller may request per sandbox via +# SandboxTemplate.service_account_name. Also accepted at bootstrap. +# selectable_service_account_names = ["openshell-sandbox-restricted"] default_image = "ghcr.io/nvidia/openshell/sandbox:latest" image_pull_policy = "IfNotPresent" image_pull_secrets = ["regcred"] diff --git a/docs/reference/sandbox-compute-drivers.mdx b/docs/reference/sandbox-compute-drivers.mdx index a3c3d85444..4cdc728e3f 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -364,6 +364,8 @@ For maintainer-level implementation details, refer to the [Kubernetes driver REA | `compute_drivers = ["kubernetes"]` | Not applicable | Select the Kubernetes compute driver. | | `[openshell.drivers.kubernetes].namespace` | `server.sandboxNamespace` | Set the namespace for sandbox resources. The Helm chart defaults to the release namespace when left empty. | | `service_account_name` | `sandboxServiceAccount.name` | Set the Kubernetes service account assigned to sandbox pods and accepted by the gateway TokenReview bootstrap path. The Helm chart creates a dedicated sandbox service account by default. | +| `additional_bootstrap_service_account_names` | `server.drivers.kubernetes.additionalBootstrapServiceAccountNames` | Accept additional service accounts when a sandbox bootstraps, on top of `service_account_name`. A pod spec has a single `serviceAccountName` field, so this is a separate setting rather than a list form of that one. Set it where something other than the driver assigns the pod's service account, such as a mutating admission policy or an external controller that owns the sandbox pods. Empty by default. Refer to [ServiceAccount Bootstrap Identities](#serviceaccount-bootstrap-identities). | +| `selectable_service_account_names` | `server.drivers.kubernetes.selectableServiceAccountNames` | Service accounts a caller may request for a single sandbox through `SandboxTemplate.service_account_name`, and which are therefore also accepted at bootstrap. Separate from `additional_bootstrap_service_account_names`, because accepting an identity that something else assigns is a different decision from letting any caller ask to run as it. The driver's own `service_account_name` is always selectable. Empty by default, which means no caller can choose. Refer to [ServiceAccount Bootstrap Identities](#serviceaccount-bootstrap-identities). | | `default_image` | `server.sandboxImage` | Set the default sandbox image. | | `image_pull_policy` | `server.sandboxImagePullPolicy` | Set the Kubernetes image pull policy for sandbox pods. | | `image_pull_secrets` | `server.sandboxImagePullSecrets` | Attach Kubernetes image-pull Secrets to sandbox pods. Managed mode copies these explicitly named Secrets from the configured source namespace into each workspace namespace. In shared and operator modes, the Secrets must already exist in the sandbox namespace. | @@ -432,6 +434,135 @@ 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. +### ServiceAccount Bootstrap Identities + +A sandbox bootstraps by presenting its projected ServiceAccount token to +`IssueSandboxToken`. The gateway verifies the token through the apiserver's +`TokenReview` API, requires it to be pod-bound, and requires the authenticated +principal to be a ServiceAccount the operator accepts. + +Three settings control which accounts those are. + +| Setting | Purpose | +|---|---| +| `service_account_name` | The ServiceAccount the driver assigns to sandbox pods. Accepted for bootstrap whenever it is set. | +| `additional_bootstrap_service_account_names` | Further ServiceAccounts accepted for bootstrap. Never assigned to pods. | +| `selectable_service_account_names` | ServiceAccounts a caller may request per sandbox. Also accepted for bootstrap. | + +`additional_bootstrap_service_account_names` is a separate setting rather than +a list form of `service_account_name` because a pod spec has a single +`serviceAccountName` field, so a list has no valid rendering into it. +`selectable_service_account_names` is separate for a different reason, covered +under [Selecting an Account Per Sandbox](#selecting-an-account-per-sandbox). +Leave both lists empty, which is the default, and the gateway accepts only the +ServiceAccount the driver assigns. That is the behavior of releases before +these settings existed. + +Add names when something other than the driver decides the pod's +ServiceAccount, such as a mutating admission policy, an external controller +that owns the sandbox pods, or a separately deployed Kubernetes driver +configured independently of the gateway. Without this, bootstrap fails and the +gateway logs: + +```text +K8s TokenReview principal is not an accepted sandbox service account +``` + +That record names the presented principal alongside the accepted set. The +gateway also logs the accepted set once at startup: + +```text +K8s ServiceAccount bootstrap authenticator enabled accepted_service_accounts={"openshell-sandbox", "openshell-sandbox-2"} +``` + +```toml +[openshell.drivers.kubernetes] +service_account_name = "openshell-sandbox" +additional_bootstrap_service_account_names = ["openshell-sandbox-2"] +selectable_service_account_names = ["openshell-sandbox-restricted"] +``` + +Names are matched exactly. Prefixes and wildcards are not supported, and +surrounding whitespace is trimmed. Entries are validated as Kubernetes +ServiceAccount names at gateway startup, so a malformed name fails the gateway +rather than surfacing later, but nothing checks that the account exists: a +well-formed name that was never created surfaces as a rejected bootstrap, or as +a pod the apiserver refuses to admit if a caller selects it. Matching also ignores the namespace, which is validated +separately against the active `workspace_mode`, so under managed and operator +modes a name is accepted in every namespace the gateway accepts. Adding a name +does not widen which namespaces may bootstrap. + +Accepting a name does not change how a token is verified. It is still checked +through `TokenReview`, still required to carry the pod-name and pod-UID extras, +the live pod UID must still match the token, and the pod must still be owned by +a `Sandbox` CR whose `openshell.ai/sandbox-id` label matches the pod's +`openshell.io/sandbox-id` annotation. A ServiceAccount is also the identity that +cloud workload-identity systems attach permissions to, so a name you accept here +may carry authority outside the cluster. Add only identities that should be able +to bootstrap a sandbox on this gateway. + +The bootstrap authenticator is constructed only when the gateway runs in-cluster +with sandbox JWT issuing enabled. If the startup record above is absent, the +bootstrap path is disabled and the two acceptance lists have no effect. +`selectable_service_account_names` still applies, because the driver enforces +it when it renders the pod. + +#### Selecting an Account Per Sandbox + +`selectable_service_account_names` lets a caller choose which account a single +sandbox runs as: + +```json +{ + "spec": { + "template": { + "image": "ghcr.io/nvidia/openshell/sandbox:latest", + "serviceAccountName": "openshell-sandbox-restricted" + } + } +} +``` + +There is no CLI flag for this field, so gRPC or an SDK is the only way to set +it. + +The driver resolves the pod's account when it validates the create, before the +gateway persists anything. Omitting the field uses `service_account_name`. A +value naming a selectable account uses that account. Anything else fails with +`InvalidArgument`, including a value that is present but blank, rather than +falling back to the default: a sandbox silently running as a different identity +than the caller asked for surfaces much later as a confusing cloud IAM denial. + +The two lists are opted into separately. + +| Configured in | Assigned by | A caller may request it | +|---|---|---| +| `service_account_name` | the driver, on any pod that does not select another | yes | +| `additional_bootstrap_service_account_names` | something outside the driver | no | +| `selectable_service_account_names` | the driver, when requested | yes | + +So accepting an account so an externally-owned pod can bootstrap does not also +make it requestable. Since a ServiceAccount is the identity cloud +workload-identity systems attach permissions to, treat +`selectable_service_account_names` as the set of cloud identities any caller who +can create a sandbox may assume, and scope its members accordingly. Selection +is not restricted per caller or per workspace, so on a multi-tenant gateway any +tenant may request any account in that set. + +Three further constraints: + +- The accounts must already exist in every namespace the gateway creates + sandboxes in. The driver creates only `service_account_name`, so a selected + account that is missing yields a pod the apiserver refuses to admit. Under + `workspace_mode = "managed"` namespaces are created on demand, so an operator + cannot pre-create accounts in them; selection suits `shared` and `operator` + mode. +- Selection requires the gateway's built-in Kubernetes driver. A separately + deployed driver process reads its own flags, which do not include these + lists, so it rejects every request that names a non-default account. +- Only the Kubernetes driver supports the field. The gateway rejects a request + that sets it when another compute driver is active. + ### Kubernetes Driver Config PVC Mounts Kubernetes driver config can mount existing PersistentVolumeClaims into the diff --git a/proto/openshell.proto b/proto/openshell.proto index 4dd290090d..c741adbc74 100644 --- a/proto/openshell.proto +++ b/proto/openshell.proto @@ -860,6 +860,18 @@ message SandboxTemplate { // forwards only that inner Struct to DriverSandboxTemplate.driver_config. // The selected driver owns nested schema validation. google.protobuf.Struct driver_config = 11; + // Optional Kubernetes ServiceAccount for this sandbox's pod. + // + // Must name an account the operator made selectable + // (`[openshell.drivers.kubernetes] selectable_service_account_names`, plus + // the driver's own `service_account_name`); anything else is rejected rather + // than silently replaced by the default, including a value that is present + // but blank. Leave it unset to get the driver's configured account, which is + // the behavior of every deployment that does not set this. + // + // Kubernetes only: the gateway rejects a request that sets this field when + // the active compute driver is not the Kubernetes driver. + string service_account_name = 12; } // User-facing sandbox status derived by the gateway from compute-driver observations. diff --git a/sdk/go/proto/openshellv1/openshell.pb.go b/sdk/go/proto/openshellv1/openshell.pb.go index 102688df4f..4b2c7af3a9 100644 --- a/sdk/go/proto/openshellv1/openshell.pb.go +++ b/sdk/go/proto/openshellv1/openshell.pb.go @@ -1308,9 +1308,18 @@ type SandboxTemplate struct { // The gateway selects the block matching the active compute driver and // forwards only that inner Struct to DriverSandboxTemplate.driver_config. // The selected driver owns nested schema validation. - DriverConfig *structpb.Struct `protobuf:"bytes,11,opt,name=driver_config,json=driverConfig,proto3" json:"driver_config,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + DriverConfig *structpb.Struct `protobuf:"bytes,11,opt,name=driver_config,json=driverConfig,proto3" json:"driver_config,omitempty"` + // Optional Kubernetes ServiceAccount for this sandbox's pod. + // + // Must name an account the operator made selectable + // (`[openshell.drivers.kubernetes] selectable_service_account_names`, plus + // the driver's own `service_account_name`); anything else is rejected rather + // than silently replaced by the default. When unset the driver's configured + // account is used, which is the behavior of every deployment that does not + // set this. Ignored by non-Kubernetes compute drivers. + ServiceAccountName string `protobuf:"bytes,12,opt,name=service_account_name,json=serviceAccountName,proto3" json:"service_account_name,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *SandboxTemplate) Reset() { @@ -1406,6 +1415,13 @@ func (x *SandboxTemplate) GetDriverConfig() *structpb.Struct { return nil } +func (x *SandboxTemplate) GetServiceAccountName() string { + if x != nil { + return x.ServiceAccountName + } + return "" +} + // User-facing sandbox status derived by the gateway from compute-driver observations. // // Public status does not embed driver-only flags such as `deleting`. @@ -13236,7 +13252,7 @@ const file_openshell_proto_rawDesc = "" + "\x03gpu\x18\x01 \x01(\v2%.openshell.v1.GpuResourceRequirementsR\x03gpu\">\n" + "\x17GpuResourceRequirements\x12\x19\n" + "\x05count\x18\x01 \x01(\rH\x00R\x05count\x88\x01\x01B\b\n" + - "\x06_count\"\xef\x05\n" + + "\x06_count\"\xa1\x06\n" + "\x0fSandboxTemplate\x12\x14\n" + "\x05image\x18\x01 \x01(\tR\x05image\x12,\n" + "\x12runtime_class_name\x18\x02 \x01(\tR\x10runtimeClassName\x12!\n" + @@ -13247,7 +13263,8 @@ const file_openshell_proto_rawDesc = "" + "\tresources\x18\a \x01(\v2\x17.google.protobuf.StructR\tresources\x12,\n" + "\x0fuser_namespaces\x18\n" + " \x01(\bH\x00R\x0euserNamespaces\x88\x01\x01\x12<\n" + - "\rdriver_config\x18\v \x01(\v2\x17.google.protobuf.StructR\fdriverConfig\x1a9\n" + + "\rdriver_config\x18\v \x01(\v2\x17.google.protobuf.StructR\fdriverConfig\x120\n" + + "\x14service_account_name\x18\f \x01(\tR\x12serviceAccountName\x1a9\n" + "\vLabelsEntry\x12\x10\n" + "\x03key\x18\x01 \x01(\tR\x03key\x12\x14\n" + "\x05value\x18\x02 \x01(\tR\x05value:\x028\x01\x1a>\n" +