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.