From 9988fe6f250e9bffcba19ff64a01dd56f36da883 Mon Sep 17 00:00:00 2001 From: Prashant S Khodade Date: Mon, 14 Sep 2026 23:23:49 -0700 Subject: [PATCH] fix(mxc): address PR #3296 review findings on credential lifetime and env keys Address the three blocking findings from the automated review of NVIDIA/OpenShell#3296 (head 550b81af): - Already-expired static provider credentials were silently withheld by the shared resolver before the create-time fail-closed check ran, so an MXC sandbox could be created without the configured credential instead of rejecting the request. Track withheld expired keys separately in ProviderEnvironment so the MXC create-time validator can still reject them. - Provider credential environment keys that collided case-insensitively with each other, or with the reserved TLS trust env vars injected later, were not validated before staging, producing an ambiguous or silently overwritten sandbox environment on Windows. Reject these synchronously at CreateSandbox instead. - The Global Policy Override docs said the global payload only supplies dynamic policy fields, but the implementation replaces the complete effective policy except UI. Corrected the description. Signed-off-by: Prashant S Khodade --- crates/openshell-driver-mxc/src/driver.rs | 91 ++++++++++++++++++++ crates/openshell-server/src/grpc/policy.rs | 27 +++++- crates/openshell-server/src/grpc/provider.rs | 10 +++ docs/sandboxes/policies.mdx | 5 +- 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/crates/openshell-driver-mxc/src/driver.rs b/crates/openshell-driver-mxc/src/driver.rs index d0024a3e09..042eb8c0a5 100644 --- a/crates/openshell-driver-mxc/src/driver.rs +++ b/crates/openshell-driver-mxc/src/driver.rs @@ -318,6 +318,50 @@ fn append_provider_child_env( ); } +/// Rejects provider credential environment keys that would collide once +/// injected into the sandbox process, before any staging or launch happens. +/// +/// Windows environment variables are case-insensitive, so two provider keys +/// that differ only by case (or shadow one of the reserved TLS trust keys +/// `append_tls_env_vars` injects later) would otherwise merge or get silently +/// overwritten with no diagnostic, leaving the sandbox with an ambiguous, +/// wrong, or missing credential. +fn validate_provider_child_env_keys( + provider_credentials: Option<&ProviderCredentialState>, +) -> Result<(), tonic::Status> { + let Some(provider_credentials) = provider_credentials else { + return Ok(()); + }; + let mut seen: HashMap = HashMap::new(); + let mut collisions: Vec = Vec::new(); + let mut keys = provider_credentials + .child_env_with_gcp_resolved() + .into_keys() + .collect::>(); + keys.sort_unstable(); + for key in keys { + let folded = key.to_ascii_uppercase(); + if TLS_ENV_KEYS.iter().any(|reserved| folded == *reserved) { + collisions.push(format!("{key} (reserved for TLS trust configuration)")); + continue; + } + if let Some(existing) = seen.insert(folded, key.clone()) + && existing != key + { + collisions.push(format!("{key} (collides with {existing})")); + } + } + if collisions.is_empty() { + Ok(()) + } else { + Err(tonic::Status::failed_precondition(format!( + "provider credential environment keys are ambiguous on Windows (case-insensitive) \ + or reserved: {}", + collisions.join(", ") + ))) + } +} + fn configured_egress_addr(config: &MxcComputeConfig) -> Result, tonic::Status> { if !config.egress_proxy { return Ok(None); @@ -588,6 +632,7 @@ impl MxcComputeBackend { .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .remove(&sandbox_id); + validate_provider_child_env_keys(provider_credentials.as_ref())?; Self::validate_sandbox_fields(sandbox)?; let sandbox_config = sandbox_config(sandbox)?; @@ -1479,6 +1524,52 @@ mod lifecycle_tests { assert!(!env.iter().any(|entry| entry.contains("raw-test-token"))); } + #[test] + fn provider_child_env_keys_reject_case_insensitive_collision() { + let credentials = ProviderCredentialState::from_bound_environment( + 1, + HashMap::from([ + ("github_token".to_string(), "a".to_string()), + ("GITHUB_TOKEN".to_string(), "b".to_string()), + ]), + HashMap::new(), + HashMap::new(), + HashMap::new(), + vec!["github_token".to_string(), "GITHUB_TOKEN".to_string()], + ) + .expect("valid provider credential state"); + + let error = validate_provider_child_env_keys(Some(&credentials)) + .expect_err("case-colliding provider keys must fail closed"); + assert_eq!(error.code(), tonic::Code::FailedPrecondition); + assert!(error.message().contains("collides")); + } + + #[test] + fn provider_child_env_keys_reject_tls_reserved_name() { + let credentials = ProviderCredentialState::from_bound_environment( + 1, + HashMap::from([("SSL_CERT_FILE".to_string(), "not-a-ca-bundle".to_string())]), + HashMap::new(), + HashMap::new(), + HashMap::new(), + vec!["SSL_CERT_FILE".to_string()], + ) + .expect("valid provider credential state"); + + let error = validate_provider_child_env_keys(Some(&credentials)) + .expect_err("TLS-reserved provider keys must fail closed"); + assert_eq!(error.code(), tonic::Code::FailedPrecondition); + assert!(error.message().contains("reserved for TLS")); + } + + #[test] + fn provider_child_env_keys_allow_distinct_names() { + let credentials = github_provider_credentials(); + validate_provider_child_env_keys(Some(&credentials)) + .expect("non-colliding, non-reserved provider keys are allowed"); + } + #[tokio::test] async fn provider_state_requiring_resolution_fails_closed_without_governed_egress() { let backend = MxcComputeBackend::new_mocked(MxcComputeConfig::default()); diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 1ae8eac3b4..e5110f8d37 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -3387,7 +3387,10 @@ fn validate_create_time_provider_credential_lifetimes( .get(*key) .is_some_and(|expires_at_ms| *expires_at_ms > 0) }) + .chain(provider_environment.expired_static_keys.iter()) .cloned() + .collect::>() + .into_iter() .collect::>(); expiring_static_keys.sort(); @@ -3395,7 +3398,7 @@ fn validate_create_time_provider_credential_lifetimes( Ok(()) } else { Err(Status::failed_precondition(format!( - "compute driver cannot refresh expiring provider credentials for running sandbox '{sandbox_id}'; recreate the sandbox with non-expiring credentials (expiring keys: {})", + "compute driver cannot refresh expiring or already-expired provider credentials for sandbox '{sandbox_id}'; recreate the sandbox with non-expiring, current credentials (affected keys: {})", expiring_static_keys.join(", ") ))) } @@ -11831,6 +11834,28 @@ mod tests { .expect("non-expiring static credentials are supported"); } + #[test] + fn create_time_provider_credentials_reject_already_expired_static_values() { + // The shared resolver withholds already-expired static credentials + // entirely -- they never appear in `static_credential_keys` or + // `credential_expires_at_ms` -- so this check must consult + // `expired_static_keys` independently instead of silently allowing + // sandbox creation without the configured credential. + let provider_environment = ProviderEnvironment { + expired_static_keys: HashSet::from(["GITHUB_TOKEN".to_string()]), + ..Default::default() + }; + + let error = validate_create_time_provider_credential_lifetimes( + "sandbox-expired", + &provider_environment, + ) + .expect_err("already-expired static credentials must fail closed"); + + assert_eq!(error.code(), Code::FailedPrecondition); + assert!(error.message().contains("GITHUB_TOKEN")); + } + #[tokio::test] async fn provider_environment_withholds_static_credentials_from_legacy_supervisors() { use openshell_core::proto::GetSandboxProviderEnvironmentRequest; diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 849123106b..ab0ee34bf4 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -68,6 +68,13 @@ pub(super) struct ProviderEnvironment { pub dynamic_credentials: HashMap, pub static_credential_bindings: HashMap, pub static_credential_keys: HashSet, + /// Static credential keys withheld because they were already expired at + /// resolution time. Excluded from `environment`/`static_credential_keys` + /// like any other withheld key, but tracked separately so create-time + /// callers (see `validate_create_time_provider_credential_lifetimes`) can + /// fail closed instead of silently creating a sandbox without the + /// configured credential. + pub expired_static_keys: HashSet, } /// Immutable provider records used to build one provider-environment response. @@ -1117,6 +1124,7 @@ pub(super) async fn resolve_provider_environment_from_records_with_policy_bindin let mut expires = HashMap::new(); let mut static_credential_bindings = HashMap::new(); let mut static_credential_keys = HashSet::new(); + let mut expired_static_keys = HashSet::new(); let now_ms = crate::persistence::current_time_ms(); validate_provider_environment_records_unique_at(store, catalog, records, now_ms).await?; let registry = openshell_providers::ProviderRegistry::new(); @@ -1235,6 +1243,7 @@ pub(super) async fn resolve_provider_environment_from_records_with_policy_bindin expires_at_ms, "skipping expired provider credential" ); + expired_static_keys.insert(key.clone()); continue; } if expires_at_ms > 0 { @@ -1354,6 +1363,7 @@ pub(super) async fn resolve_provider_environment_from_records_with_policy_bindin dynamic_credentials: resolve_dynamic_credentials_from_records(catalog, records), static_credential_bindings, static_credential_keys, + expired_static_keys, }) } diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index e05ea2a294..fd16adb11c 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -609,7 +609,10 @@ openshell policy set --global --policy ./global-policy.yaml When a global policy is configured: -- The global payload supplies the dynamic policy fields for all sandboxes. +- The global payload replaces every sandbox's complete effective policy, + including static sections such as `filesystem_policy`, `landlock`, and + `process`, not only the dynamic `network_policies` and + `network_middlewares` fields. - Each sandbox keeps the startup-only `ui` section from its own creation policy; a global policy containing `ui` is rejected. - Sandbox-level policy updates are rejected until the global policy is removed.