From 58e544160977a89b62b163b68ff9dbf4ff8cfe5c Mon Sep 17 00:00:00 2001 From: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:51:25 +0000 Subject: [PATCH 1/3] feat(server): accept a configured set of sandbox service accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bootstrap auth compared the TokenReview principal against a single value, `[openshell.drivers.kubernetes] service_account_name`, which is also the name the driver stamps onto every sandbox pod. One field served both purposes, so every sandbox on a gateway necessarily shared one Kubernetes identity — and where something other than the driver assigns the pod's ServiceAccount, bootstrap failed outright. The ServiceAccount is the unit of cloud IAM federation: an EKS, GCP or Azure workload-identity trust policy conditions on system:serviceaccount::. One accepted account per gateway therefore means one cloud identity per gateway. Accept a set instead. additional_bootstrap_service_account_names enrols further identities for bootstrap only; service_account_name remains the one name the driver puts on pods. The two are deliberately separate settings rather than one widened field, because a pod carries exactly one ServiceAccount and a list there would render invalid pods. The list is empty by default, so a gateway that does not set it behaves exactly as before. Rejections now name the presented principal and the enrolled set, and the success path logs the account that actually authenticated rather than the configured default. The enrolled set is also logged once at startup. Verification is unchanged: still TokenReview, still required to be pod-bound, still validated against the pod's owning Sandbox CR. This widens which identities an operator may enrol, not how they are verified. Refs: #2806 Signed-off-by: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> --- .../skills/debug-openshell-cluster/SKILL.md | 5 +- architecture/gateway.md | 2 +- .../openshell-driver-kubernetes/src/config.rs | 43 +++ .../openshell-driver-kubernetes/src/main.rs | 3 + crates/openshell-server/src/auth/k8s_sa.rs | 288 ++++++++++++++++-- .../src/compute/driver_config/builtin.rs | 34 +++ crates/openshell-server/src/lib.rs | 20 +- deploy/helm/openshell/README.md | 1 + .../openshell/templates/gateway-config.yaml | 8 + .../openshell/tests/gateway_config_test.yaml | 37 +++ deploy/helm/openshell/values.yaml | 10 + docs/reference/gateway-config.mdx | 4 + docs/reference/sandbox-compute-drivers.mdx | 64 ++++ 13 files changed, 478 insertions(+), 41 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index 17628e980b..6ac48a9f4c 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -439,10 +439,11 @@ 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`. ```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/src/config.rs b/crates/openshell-driver-kubernetes/src/config.rs index 1cdc98f6e1..7a53100f7f 100644 --- a/crates/openshell-driver-kubernetes/src/config.rs +++ b/crates/openshell-driver-kubernetes/src/config.rs @@ -303,6 +303,22 @@ pub struct KubernetesComputeConfig { /// Kubernetes `ServiceAccount` assigned to sandbox pods 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, which keeps the + /// single-account behavior unchanged. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub additional_bootstrap_service_account_names: Vec, pub default_image: String, pub image_pull_policy: String, /// Kubernetes `imagePullSecrets` names attached to sandbox pods. @@ -436,6 +452,7 @@ 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(), 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`, @@ -1152,6 +1169,32 @@ 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() + ] + ); + } + #[test] fn serde_override_default_runtime_class_name() { let json = serde_json::json!({ diff --git a/crates/openshell-driver-kubernetes/src/main.rs b/crates/openshell-driver-kubernetes/src/main.rs index 30b4fcada9..a874be3a52 100644 --- a/crates/openshell-driver-kubernetes/src/main.rs +++ b/crates/openshell-driver-kubernetes/src/main.rs @@ -233,6 +233,9 @@ 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(), 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..50b9ac2edc 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,72 @@ 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 a single `serviceAccountName` onto sandbox pods +/// (`service_account_name`), and that name 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`. 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. + pub fn new(pod_default: &str, additional: impl IntoIterator>) -> Self { + let accepted = std::iter::once(pod_default.to_string()) + .chain(additional.into_iter().map(Into::into)) + .filter_map(|name| { + let trimmed = name.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_string()) + }) + .collect(); + Self { accepted } + } + + /// Build the accepted set from the Kubernetes driver configuration the + /// gateway reads for bootstrap. + pub fn from_kubernetes_config(config: &KubernetesComputeConfig) -> Self { + Self::new( + &config.service_account_name, + &config.additional_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 +244,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 +252,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 +260,7 @@ impl LiveK8sResolver { token_reviews_api, expected_audience, namespace_validator, - expected_service_account, + service_account_validator, } } @@ -262,7 +326,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 +336,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 +410,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 +447,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 +474,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 +782,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 +799,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 +823,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 +847,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 +870,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 +893,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 +913,142 @@ 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); + } + + #[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 +1067,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 +1090,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..761d9fb11e 100644 --- a/crates/openshell-server/src/compute/driver_config/builtin.rs +++ b/crates/openshell-server/src/compute/driver_config/builtin.rs @@ -192,6 +192,40 @@ 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"] +"#, + ) + .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()] + ); + + 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-other")); } #[test] 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..83d98498cc 100644 --- a/deploy/helm/openshell/README.md +++ b/deploy/helm/openshell/README.md @@ -229,6 +229,7 @@ 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) accepts only the driver's own sandbox ServiceAccount. | | 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.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. | diff --git a/deploy/helm/openshell/templates/gateway-config.yaml b/deploy/helm/openshell/templates/gateway-config.yaml index 9d24dbd917..b22ee9d4db 100644 --- a/deploy/helm/openshell/templates/gateway-config.yaml +++ b/deploy/helm/openshell/templates/gateway-config.yaml @@ -139,6 +139,14 @@ 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 }} {{- 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..d2da6edc21 100644 --- a/deploy/helm/openshell/tests/gateway_config_test.yaml +++ b/deploy/helm/openshell/tests/gateway_config_test.yaml @@ -103,6 +103,43 @@ 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: + # `[^\[]*?` keeps the match inside the kubernetes table: the key must not + # drift into a nested table such as [openshell.drivers.kubernetes.sidecar], + # which denies unknown fields and would fail the gateway at startup. + - matchRegex: + path: data["gateway.toml"] + pattern: '(?ms)\[openshell\.drivers\.kubernetes\][^\[]*?additional_bootstrap_service_account_names\s*=\s*\["openshell-sandbox-2", "openshell-sandbox-3"\]' + # The pod default is a separate setting and must be untouched. + - matchRegex: + path: data["gateway.toml"] + pattern: '(?ms)\[openshell\.drivers\.kubernetes\][^\[]*?service_account_name\s*=\s*"openshell-sandbox"' + + # A null YAML list item renders away silently through `quote`, leaving an + # operator with an empty or short array they believe is populated. + - 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..2961803e36 100644 --- a/deploy/helm/openshell/values.yaml +++ b/deploy/helm/openshell/values.yaml @@ -257,6 +257,16 @@ 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) accepts only the + # driver's own sandbox ServiceAccount. + additionalBootstrapServiceAccountNames: [] # -- 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..bd3857507e 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -453,6 +453,10 @@ 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"] 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..a613f3e752 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -364,6 +364,7 @@ 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). | | `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 +433,69 @@ 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. + +Two distinct 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. | + +They are separate settings because a pod spec has a single `serviceAccountName` +field, so a list has no valid rendering into it. Leave the additional list +empty, which is the default, and the gateway accepts only the ServiceAccount it +assigns. That is the behavior of releases before this setting 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"] +``` + +Names are matched exactly. Prefixes and wildcards are not supported, and +entries are neither validated as DNS-1123 names nor checked against existing +ServiceAccounts, so a typo surfaces only as a rejected bootstrap. Surrounding +whitespace is trimmed. 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 these settings have no effect. + ### Kubernetes Driver Config PVC Mounts Kubernetes driver config can mount existing PersistentVolumeClaims into the From ccc57d3b4f1fd69236beb0fa52c79481abb566b4 Mon Sep 17 00:00:00 2001 From: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> Date: Thu, 20 Aug 2026 22:43:32 +0300 Subject: [PATCH 2/3] feat(kubernetes): allow a sandbox to select its ServiceAccount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part 1 made bootstrap auth accept a set, but the driver still stamps one name onto every pod it creates, so a deployment where the gateway owns the pods still gets one Kubernetes identity for every sandbox — and therefore one cloud identity, since the ServiceAccount is what EKS, GKE and Azure workload identity federate on. Granting the union of permissions to that shared account gives every sandbox the most-privileged set. Add SandboxTemplate.service_account_name, resolved by the Kubernetes driver against selectable_service_account_names plus the driver default. Resolution runs in validate_sandbox_create, so a request naming an account that is not selectable is refused before the gateway persists the sandbox or mints its JWT, and before the driver creates a namespace or copies a secret. A blank value is refused too: defaulting it would run the sandbox as an identity the caller did not ask for. The selectable list is deliberately separate from additional_bootstrap_service_account_names. That setting means "accept this identity when something outside the driver assigned it"; this one means "any caller who can create a sandbox may run as this identity". Enrolling a privileged account so an externally-owned pod can bootstrap should not silently make it requestable, so the two are opted into independently. Both feed the accepted-for-auth set, since a selectable account has to authenticate. Configured names are validated at driver startup and a requested name at the gateway, so a malformed account fails early rather than reaching the apiserver or being echoed back in an error. The rejection names the account the caller asked for; the selectable set goes to the gateway log instead. Only the Kubernetes driver supports the field, so the gateway rejects a request that sets it under another driver rather than ignoring it (Podman) or failing with a message about platform_config the caller never set (Docker, VM). Refs: #2806 Signed-off-by: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> --- .../skills/debug-openshell-cluster/SKILL.md | 7 +- crates/openshell-driver-kubernetes/README.md | 3 +- .../openshell-driver-kubernetes/src/config.rs | 379 +++++++++++++++++- .../openshell-driver-kubernetes/src/driver.rs | 140 ++++++- crates/openshell-driver-kubernetes/src/lib.rs | 3 +- .../openshell-driver-kubernetes/src/main.rs | 6 + crates/openshell-server/src/auth/k8s_sa.rs | 59 ++- .../src/compute/driver_config/builtin.rs | 22 + crates/openshell-server/src/compute/mod.rs | 80 +++- .../openshell-server/src/grpc/validation.rs | 49 +++ deploy/helm/openshell/README.md | 3 +- .../openshell/templates/gateway-config.yaml | 8 + .../openshell/tests/gateway_config_test.yaml | 70 +++- deploy/helm/openshell/values.yaml | 20 +- docs/reference/gateway-config.mdx | 3 + docs/reference/sandbox-compute-drivers.mdx | 85 +++- proto/openshell.proto | 12 + sdk/go/proto/openshellv1/openshell.pb.go | 27 +- 18 files changed, 926 insertions(+), 50 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index 6ac48a9f4c..b5f4f43da5 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -443,7 +443,12 @@ 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 any service account outside that name -plus `additional_bootstrap_service_account_names`. +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/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 7a53100f7f..a8fdf34028 100644 --- a/crates/openshell-driver-kubernetes/src/config.rs +++ b/crates/openshell-driver-kubernetes/src/config.rs @@ -300,8 +300,9 @@ 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`]. @@ -315,10 +316,34 @@ pub struct KubernetesComputeConfig { /// /// 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, which keeps the - /// single-account behavior unchanged. + /// 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. @@ -453,6 +478,7 @@ impl Default for KubernetesComputeConfig { 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`, @@ -669,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. @@ -826,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] @@ -1195,6 +1354,218 @@ mod tests { ); } + 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 a874be3a52..20c6856a3b 100644 --- a/crates/openshell-driver-kubernetes/src/main.rs +++ b/crates/openshell-driver-kubernetes/src/main.rs @@ -236,6 +236,12 @@ async fn main() -> Result<()> { // 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 50b9ac2edc..6e47fb2d3c 100644 --- a/crates/openshell-server/src/auth/k8s_sa.rs +++ b/crates/openshell-server/src/auth/k8s_sa.rs @@ -163,14 +163,17 @@ 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 a single `serviceAccountName` onto sandbox pods -/// (`service_account_name`), and that name is a member whenever it is +/// 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`. The set stays -/// operator-configured and closed; this widens which identities may bootstrap, -/// not how they are verified. +/// `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 @@ -192,24 +195,25 @@ impl ServiceAccountValidator { /// 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 accepted = std::iter::once(pod_default.to_string()) + let owned: Vec = std::iter::once(pod_default.to_string()) .chain(additional.into_iter().map(Into::into)) - .filter_map(|name| { - let trimmed = name.trim(); - (!trimmed.is_empty()).then(|| trimmed.to_string()) - }) .collect(); - Self { accepted } + 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. + /// 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::new( - &config.service_account_name, - &config.additional_bootstrap_service_account_names, - ) + Self { + accepted: config.accepted_bootstrap_service_account_names(), + } } pub fn accepts(&self, service_account: &str) -> bool { @@ -977,6 +981,29 @@ mod tests { 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( diff --git a/crates/openshell-server/src/compute/driver_config/builtin.rs b/crates/openshell-server/src/compute/driver_config/builtin.rs index 761d9fb11e..dcaa440877 100644 --- a/crates/openshell-server/src/compute/driver_config/builtin.rs +++ b/crates/openshell-server/src/compute/driver_config/builtin.rs @@ -208,6 +208,7 @@ service_account_name = "sandbox-sa" 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"); @@ -218,6 +219,11 @@ additional_bootstrap_service_account_names = ["sandbox-alt", "sandbox-legacy"] 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"), @@ -225,7 +231,23 @@ additional_bootstrap_service_account_names = ["sandbox-alt", "sandbox-legacy"] ); 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..976f59485d 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 = @@ -3307,6 +3307,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 +3409,17 @@ fn build_platform_config(template: &SandboxTemplate) -> Option 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/deploy/helm/openshell/README.md b/deploy/helm/openshell/README.md index 83d98498cc..0e082b8aa1 100644 --- a/deploy/helm/openshell/README.md +++ b/deploy/helm/openshell/README.md @@ -229,9 +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) accepts only the driver's own sandbox ServiceAccount. | +| 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 b22ee9d4db..ab3ff427e9 100644 --- a/deploy/helm/openshell/templates/gateway-config.yaml +++ b/deploy/helm/openshell/templates/gateway-config.yaml @@ -147,6 +147,14 @@ data: {{- 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 d2da6edc21..7b0da20e73 100644 --- a/deploy/helm/openshell/tests/gateway_config_test.yaml +++ b/deploy/helm/openshell/tests/gateway_config_test.yaml @@ -117,19 +117,75 @@ tests: - openshell-sandbox-2 - openshell-sandbox-3 asserts: - # `[^\[]*?` keeps the match inside the kubernetes table: the key must not - # drift into a nested table such as [openshell.drivers.kubernetes.sidecar], - # which denies unknown fields and would fail the gateway at startup. + # 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: '(?ms)\[openshell\.drivers\.kubernetes\][^\[]*?additional_bootstrap_service_account_names\s*=\s*\["openshell-sandbox-2", "openshell-sandbox-3"\]' + 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: '(?ms)\[openshell\.drivers\.kubernetes\][^\[]*?service_account_name\s*=\s*"openshell-sandbox"' + pattern: '(?m)^\s*service_account_name\s+= "openshell-sandbox"$' - # A null YAML list item renders away silently through `quote`, leaving an - # operator with an empty or short array they believe is populated. + - 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: diff --git a/deploy/helm/openshell/values.yaml b/deploy/helm/openshell/values.yaml index 2961803e36..29f09744de 100644 --- a/deploy/helm/openshell/values.yaml +++ b/deploy/helm/openshell/values.yaml @@ -264,9 +264,25 @@ server: # 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) accepts only the - # driver's own sandbox ServiceAccount. + # 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 bd3857507e..a0f4e61419 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -457,6 +457,9 @@ service_account_name = "openshell-sandbox" # 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 a613f3e752..4cdc728e3f 100644 --- a/docs/reference/sandbox-compute-drivers.mdx +++ b/docs/reference/sandbox-compute-drivers.mdx @@ -365,6 +365,7 @@ For maintainer-level implementation details, refer to the [Kubernetes driver REA | `[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. | @@ -440,17 +441,22 @@ A sandbox bootstraps by presenting its projected ServiceAccount token to `TokenReview` API, requires it to be pod-bound, and requires the authenticated principal to be a ServiceAccount the operator accepts. -Two distinct settings control which accounts those are. +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. | -They are separate settings because a pod spec has a single `serviceAccountName` -field, so a list has no valid rendering into it. Leave the additional list -empty, which is the default, and the gateway accepts only the ServiceAccount it -assigns. That is the behavior of releases before this setting existed. +`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 @@ -473,12 +479,15 @@ K8s ServiceAccount bootstrap authenticator enabled accepted_service_accounts={"o [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 -entries are neither validated as DNS-1123 names nor checked against existing -ServiceAccounts, so a typo surfaces only as a rejected bootstrap. Surrounding -whitespace is trimmed. Matching also ignores the namespace, which is validated +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. @@ -494,7 +503,65 @@ 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 these settings have no effect. +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 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" + From a970d224f909704b6d2f76353fac09cb4c1957da Mon Sep 17 00:00:00 2001 From: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> Date: Thu, 20 Aug 2026 22:44:23 +0300 Subject: [PATCH 3/3] fix(server): preserve driver error codes on sandbox create create_sandbox forwarded AlreadyExists and FailedPrecondition from the driver and collapsed everything else into Internal, so a request the driver rejected as malformed reached the caller as a server fault. Clients retry Internal; no retry gets them out of an invalid argument. InvalidArgument is now preserved with the driver's message intact. The four arms only differed in the status they returned, so they are one arm now. The compensating store delete they each ran was also discarding its error, which leaves a sandbox row with no compute object behind and no way to notice; that failure is logged. This is not required by the ServiceAccount selector, which the Kubernetes driver refuses in validate_sandbox_create before anything is persisted. It is the same class of problem one layer down, and the driver's own sandbox-name and GPU validation reach this path when a caller bypasses the pre-create hook. Signed-off-by: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> --- crates/openshell-server/src/compute/mod.rs | 269 ++++++++++++--------- 1 file changed, 155 insertions(+), 114 deletions(-) diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 976f59485d..6de880092f 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -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())), + }) } } } @@ -5458,113 +5457,155 @@ mod tests { ); } - /// 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; + /// A driver that behaves normally except that creates fail with the + /// status it was built with. + #[derive(Debug)] + struct FailingDriver(TestDriver, Status); - /// A driver that behaves normally except that creates fail, so the - /// test exercises only the failure attribute. - #[derive(Debug, Default)] - struct FailingDriver(TestDriver); + impl FailingDriver { + fn new(status: Status) -> 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"); + + 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::default())).await; + 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();