fix(mxc): address PR #3296 review findings on credential lifetime and env keys

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<String, String> = HashMap::new();
+    let mut collisions: Vec<String> = Vec::new();
+    let mut keys = provider_credentials
+        .child_env_with_gcp_resolved()
+        .into_keys()
+        .collect::<Vec<_>>();
+    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<Option<SocketAddr>, 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());

crates/openshell-server/src/grpc/policy.rs

@@ -3387,15 +3387,18 @@ 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::<HashSet<_>>()
+        .into_iter()
         .collect::<Vec<_>>();
     expiring_static_keys.sort();
 
     if expiring_static_keys.is_empty() {
         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;

crates/openshell-server/src/grpc/provider.rs

@@ -68,6 +68,13 @@ pub(super) struct ProviderEnvironment {
     pub dynamic_credentials: HashMap<String, ProviderProfileCredential>,
     pub static_credential_bindings: HashMap<String, StaticCredentialBinding>,
     pub static_credential_keys: HashSet<String>,
+    /// 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<String>,
 }
 
 /// 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,
     })
 }
 

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.