diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 8edf83139e..4b8fa8de91 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -1750,6 +1750,34 @@ struct AutoApproveChunkContext<'a> { resolved_from: &'a str, } +// Binding declarations include rule and process scope as well as destination. +// Reusing an approved destination in another rule still needs admin approval. +fn credential_binding_declarations_changed( + current: &ProtoSandboxPolicy, + candidate: &ProtoSandboxPolicy, +) -> bool { + fn contained(left: &ProtoSandboxPolicy, right: &ProtoSandboxPolicy) -> bool { + left.network_policies.iter().all(|(name, rule)| { + rule.endpoints + .iter() + .filter(|endpoint| endpoint.credential_binding.is_some()) + .all(|endpoint| { + right.network_policies.get(name).is_some_and(|other| { + other.binaries == rule.binaries + && other.endpoints.iter().any(|other_endpoint| { + other_endpoint.credential_binding == endpoint.credential_binding + && other_endpoint.host == endpoint.host + && other_endpoint.port == endpoint.port + && other_endpoint.ports == endpoint.ports + && other_endpoint.path == endpoint.path + }) + }) + }) + }) + } + !contained(current, candidate) || !contained(candidate, current) +} + async fn auto_approve_chunk( state: &Arc, chunk_id: &str, @@ -1788,6 +1816,22 @@ async fn auto_approve_chunk( None, ) .await?; + if live_evaluation + .candidate_effective_policy + .as_ref() + .is_some_and(|candidate| { + credential_binding_declarations_changed( + &live_evaluation.current_effective_policy, + candidate, + ) + }) + { + let error = Status::failed_precondition( + "Credential binding changes require workspace admin approval", + ); + persist_pending_application_error(state, chunk_id, &error).await; + return Ok(()); + } let validation_result = live_evaluation.validation_result; if validation_result != "prover: no new findings" { info!( @@ -2540,8 +2584,19 @@ fn is_sandbox_caller(request: &Request) -> bool { ) } -/// Sandbox-class callers may only perform sandbox-scoped policy sync. They -/// must not mutate global config or sandbox settings. +const IMAGE_POLICY_BINDING_ERROR: &str = "Image policy contains credential_binding. A workspace admin must submit the reviewed policy through the gateway."; +const IMAGE_POLICY_INITIALIZED_ERROR: &str = "Sandbox policy is already initialized; a workspace admin must submit policy changes through the gateway."; + +fn image_policy_has_credential_bindings(policy: &ProtoSandboxPolicy) -> bool { + policy.network_policies.values().any(|rule| { + rule.endpoints + .iter() + .any(|endpoint| endpoint.credential_binding.is_some()) + }) +} + +/// Sandbox callers may only submit an initial sandbox policy. Admission and +/// persistence are checked under the mutation lock in `UpdateConfig`. fn validate_sandbox_caller_update(req: &UpdateConfigRequest) -> Result<(), Status> { if req.global { return Err(Status::permission_denied( @@ -2942,7 +2997,17 @@ pub(super) async fn load_sandbox_config( &policy_credential_bindings, &provider_policy_context.endpointless_provider_names, ); - let mut configuration_error = String::new(); + let mut configuration_error = if policy.is_none() + && sandbox + .status + .as_ref() + .and_then(|status| status.configuration_admission.as_ref()) + .is_some_and(|admission| admission.error == IMAGE_POLICY_BINDING_ERROR) + { + IMAGE_POLICY_BINDING_ERROR.to_string() + } else { + String::new() + }; if let Some(effective_policy) = policy.as_mut() { stamp_provider_credentialed_endpoints( effective_policy, @@ -4193,6 +4258,78 @@ async fn handle_update_config_inner( )); } + // Initial adoption is fenced by a fresh row's resource version in the + // atomic policy write, including writes from other gateway replicas. + let sandbox = if sandbox_caller { + state + .store + .get_message::(&sandbox_id) + .await + .map_err(|error| { + super::persistence_error_to_status(error, "fetch sandbox for policy update") + })? + .ok_or_else(|| Status::not_found("sandbox not found"))? + } else { + sandbox + }; + if sandbox_caller + && (sandbox + .spec + .as_ref() + .is_some_and(|spec| spec.policy.is_some()) + || sandbox + .status + .as_ref() + .is_some_and(|status| status.configuration_activated == Some(true)) + || state + .store + .get_latest_policy(&sandbox_id) + .await + .map_err(|error| super::persistence_error_to_status(error, "fetch initial policy"))? + .is_some()) + { + return Err(Status::failed_precondition(IMAGE_POLICY_INITIALIZED_ERROR)); + } + if sandbox_caller + && req + .policy + .as_ref() + .is_some_and(image_policy_has_credential_bindings) + { + // Persist a gateway-authored diagnostic without storing any rejected policy. + let updated = state + .store + .update_message_cas::( + &sandbox_id, + sandbox.get_resource_version(), + |sandbox| { + if let Some(admission) = sandbox + .status + .as_mut() + .and_then(|status| status.configuration_admission.as_mut()) + { + admission.state = + openshell_core::proto::ConfigurationAdmissionState::Rejected.into(); + admission.error = IMAGE_POLICY_BINDING_ERROR.to_string(); + } + crate::compute::apply_configuration_readiness(sandbox); + }, + ) + .await + .map_err(|error| { + if matches!(error, crate::persistence::PersistenceError::Conflict { .. }) { + // An admin repair may have arrived before the rejection. + // Let startup refetch it instead of treating the race as fatal. + Status::failed_precondition(IMAGE_POLICY_BINDING_ERROR) + } else { + super::persistence_error_to_status(error, "record image policy rejection") + } + })?; + state.sandbox_index.update_from_sandbox(&updated); + state.sandbox_watch_bus.notify(&sandbox_id); + return Err(Status::failed_precondition(IMAGE_POLICY_BINDING_ERROR)); + } + // Sandbox-scoped policy update. let mut new_policy = req .policy @@ -4282,6 +4419,10 @@ async fn handle_update_config_inner( .await .map_err(|e| Status::internal(format!("fetch latest policy failed: {e}")))?; + if sandbox_caller && latest.is_some() { + return Err(Status::failed_precondition(IMAGE_POLICY_INITIALIZED_ERROR)); + } + if let Some(ref current) = latest && canonical_policy_record_matches_for_deduplication(current, &hash) && current.provenance == req.annotations @@ -4319,7 +4460,11 @@ async fn handle_update_config_inner( policy_payload: payload.clone(), policy_hash: hash.clone(), provenance: req.annotations.clone(), - expected_resource_version: req.expected_resource_version, + expected_resource_version: if sandbox_caller { + sandbox.get_resource_version() + } else { + req.expected_resource_version + }, annotations: req.annotations.clone(), backfill_policy: backfill_policy.clone(), }; @@ -4330,6 +4475,17 @@ async fn handle_update_config_inner( Some((next_version, sandbox_metadata_annotations(&updated_sandbox))); break; } + Err(error) + if sandbox_caller + && (matches!( + error, + crate::persistence::PersistenceError::Conflict { .. } + ) || error.is_unique_violation_on("objects_version_uq")) => + { + return Err(Status::failed_precondition( + "Initial policy adoption raced with another configuration change; fetch configuration again", + )); + } Err(error) if error.is_unique_violation_on("objects_version_uq") => { warn!( sandbox_id = %sandbox_id, @@ -23138,6 +23294,294 @@ mod tests { assert!(err.message().contains("reserved '_provider_' prefix")); } + #[tokio::test] + async fn auto_mode_leaves_credential_binding_proposal_pending_for_admin() { + use openshell_core::proto::{ProviderProfile, ProviderProfileCategory}; + let state = test_server_state().await; + state + .store + .put_message(&StoredProviderProfile { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: "profile-image-auto".to_string(), + name: "image-auto".to_string(), + workspace: "default".to_string(), + ..Default::default() + }), + profile: Some(ProviderProfile { + id: "image-auto".to_string(), + display_name: "Image Auto".to_string(), + category: ProviderProfileCategory::Other as i32, + ..Default::default() + }), + }) + .await + .unwrap(); + state + .store + .put_message(&test_provider("image-provider", "image-auto")) + .await + .unwrap(); + state + .store + .put_message(&test_sandbox( + "sb-binding-auto", + "binding-auto", + openshell_policy::restrictive_default_policy(), + vec!["image-provider".to_string()], + )) + .await + .unwrap(); + seed_sandbox_approval_mode(&state, "binding-auto", "auto").await; + let mut policy = + test_policy_with_credential_binding("bound", "api.vendor.example", "image-provider"); + let rule = policy.network_policies.get_mut("bound").unwrap(); + rule.endpoints[0].protocol = "rest".to_string(); + rule.endpoints[0].access = openshell_core::proto::NetworkAccessPreset::Full as i32; + rule.binaries.push(NetworkBinary { + path: "/usr/bin/curl".to_string(), + }); + let submitted = handle_submit_policy_analysis( + &state, + with_user(Request::new(SubmitPolicyAnalysisRequest { + workspace_scope: Some(openshell_core::proto::workspace_selector("default")), + name: "binding-auto".to_string(), + analysis_mode: "agent_authored".to_string(), + proposed_chunks: vec![PolicyChunk { + rule_name: "bound".to_string(), + proposed_rule: Some(rule.clone()), + ..Default::default() + }], + ..Default::default() + })), + ) + .await + .unwrap() + .into_inner(); + assert_eq!(submitted.accepted_chunk_ids.len(), 1); + let chunk = state + .store + .get_draft_chunk(&submitted.accepted_chunk_ids[0]) + .await + .unwrap() + .unwrap(); + assert_eq!(chunk.status, "pending"); + assert!( + chunk.application_error.contains("workspace admin approval"), + "{}", + chunk.application_error + ); + assert!( + state + .store + .get_latest_policy("sb-binding-auto") + .await + .unwrap() + .is_none() + ); + } + + #[test] + fn automatic_proposals_require_admin_for_binding_and_process_scope_changes() { + let current = test_policy_with_credential_binding("bound", "api.example.com", "provider"); + assert!(!credential_binding_declarations_changed(¤t, ¤t)); + for mutation in [ + "host", "provider", "path", "port", "binary", "rule", "remove", + ] { + let mut candidate = current.clone(); + let rule = candidate.network_policies.get_mut("bound").unwrap(); + match mutation { + "host" => rule.endpoints[0].host = "other.example.com".to_string(), + "provider" => { + rule.endpoints[0] + .credential_binding + .as_mut() + .unwrap() + .provider = "other".to_string(); + } + "path" => rule.endpoints[0].path = "/other".to_string(), + "port" => rule.endpoints[0].port = 8443, + "binary" => rule.binaries.push(NetworkBinary { + path: "/usr/bin/other".to_string(), + }), + "rule" => { + let rule = rule.clone(); + candidate.network_policies.insert("other".to_string(), rule); + } + "remove" => rule.endpoints[0].credential_binding = None, + _ => unreachable!(), + } + assert!( + credential_binding_declarations_changed(¤t, &candidate), + "{mutation}" + ); + } + let mut plain = current.clone(); + plain.network_policies.get_mut("bound").unwrap().endpoints[0].credential_binding = None; + assert!(credential_binding_declarations_changed(&plain, ¤t)); + let mut unrelated = current.clone(); + unrelated.network_policies.insert( + "ordinary".to_string(), + test_policy_with_rule("ordinary", "other.example.com") + .network_policies + .remove("ordinary") + .unwrap(), + ); + assert!(!credential_binding_declarations_changed( + ¤t, &unrelated + )); + } + + #[tokio::test] + async fn image_policy_adoption_is_once_only_including_concurrent_uploads() { + let state = test_server_state().await; + let mut sandbox = test_sandbox( + "sb-initial-image", + "initial-image", + ProtoSandboxPolicy::default(), + vec![], + ); + sandbox.spec.as_mut().unwrap().policy = None; + state.store.put_message(&sandbox).await.unwrap(); + let upload = || { + with_sandbox( + Request::new(UpdateConfigRequest { + sandbox: "initial-image".to_string(), + workspace_scope: Some(openshell_core::proto::workspace_selector("default")), + policy: Some(test_policy_with_rule("initial", "initial.example.com")), + ..Default::default() + }), + "sb-initial-image", + ) + }; + let (first, second) = tokio::join!( + handle_update_config(&state, upload()), + handle_update_config(&state, upload()) + ); + assert_eq!(usize::from(first.is_ok()) + usize::from(second.is_ok()), 1); + let refused = first.err().or_else(|| second.err()).unwrap(); + assert_eq!(refused.code(), Code::FailedPrecondition); + let refused = handle_update_config(&state, upload()).await.unwrap_err(); + assert_eq!(refused.code(), Code::FailedPrecondition); + assert_eq!(refused.message(), IMAGE_POLICY_INITIALIZED_ERROR); + assert_eq!( + state + .store + .get_latest_policy("sb-initial-image") + .await + .unwrap() + .unwrap() + .version, + 1 + ); + } + + #[tokio::test] + async fn image_policy_binding_refusal_is_visible_and_admin_repair_clears_it() { + use openshell_core::proto::{ConfigurationAdmissionState, SandboxConfigurationAdmission}; + let state = test_server_state().await; + let mut sandbox = test_sandbox( + "sb-image-binding", + "image-binding", + ProtoSandboxPolicy::default(), + vec![], + ); + sandbox.spec.as_mut().unwrap().policy = None; + let status = sandbox.status.get_or_insert_with(Default::default); + status.configuration_activated = Some(false); + status.configuration_admission = Some(SandboxConfigurationAdmission { + instance_id: "supervisor-instance".to_string(), + state: ConfigurationAdmissionState::Pending.into(), + ..Default::default() + }); + state.store.put_message(&sandbox).await.unwrap(); + // Reject before provider validation, including unattached providers and + // entries that would otherwise be stripped as provider-derived rules. + let image = test_policy_with_credential_binding( + "_provider_unattached", + "bound.example.com", + "unattached", + ); + let error = handle_update_config( + &state, + with_sandbox( + Request::new(UpdateConfigRequest { + sandbox: "image-binding".to_string(), + workspace_scope: Some(openshell_core::proto::workspace_selector("default")), + policy: Some(image), + ..Default::default() + }), + "sb-image-binding", + ), + ) + .await + .unwrap_err(); + assert_eq!(error.code(), Code::FailedPrecondition); + assert_eq!(error.message(), IMAGE_POLICY_BINDING_ERROR); + assert!( + state + .store + .get_latest_policy("sb-image-binding") + .await + .unwrap() + .is_none() + ); + let snapshot = || { + with_sandbox( + Request::new(GetSandboxConfigRequest { + name: "image-binding".to_string(), + workspace_scope: Some(openshell_core::proto::workspace_selector("default")), + }), + "sb-image-binding", + ) + }; + let refused = handle_get_sandbox_config(&state, snapshot()) + .await + .unwrap() + .into_inner(); + assert!(!refused.configuration_admitted); + assert!(refused.policy.is_none()); + assert_eq!(refused.configuration_error, IMAGE_POLICY_BINDING_ERROR); + let stored = state + .store + .get_message::("sb-image-binding") + .await + .unwrap() + .unwrap(); + assert_eq!( + stored + .status + .unwrap() + .configuration_admission + .unwrap() + .error, + IMAGE_POLICY_BINDING_ERROR + ); + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + sandbox: "image-binding".to_string(), + workspace_scope: Some(openshell_core::proto::workspace_selector("default")), + policy: Some(test_policy_with_rule("reviewed", "reviewed.example.com")), + ..Default::default() + })), + ) + .await + .unwrap(); + let repaired = handle_get_sandbox_config(&state, snapshot()) + .await + .unwrap() + .into_inner(); + assert!(repaired.configuration_admitted); + assert!(repaired.configuration_error.is_empty()); + assert!( + repaired + .policy + .unwrap() + .network_policies + .contains_key("reviewed") + ); + } + #[tokio::test] async fn update_config_sandbox_sync_strips_reserved_provider_keys_before_persisting() { use openshell_core::proto::{SandboxPhase, SandboxSpec}; diff --git a/crates/openshell-supervisor/src/lib.rs b/crates/openshell-supervisor/src/lib.rs index 8d498e162d..24a8294a1f 100644 --- a/crates/openshell-supervisor/src/lib.rs +++ b/crates/openshell-supervisor/src/lib.rs @@ -1683,23 +1683,6 @@ fn enrich_proto_baseline_paths(proto: &mut openshell_core::proto::SandboxPolicy) modified } -fn strip_proto_provider_policy_entries(proto: &mut openshell_core::proto::SandboxPolicy) -> bool { - openshell_policy::strip_provider_rule_names(proto) -} - -fn proto_sync_payload_for_enriched_policy( - proto: &openshell_core::proto::SandboxPolicy, - enriched: bool, -) -> Option { - if !enriched { - return None; - } - - let mut sync_policy = proto.clone(); - strip_proto_provider_policy_entries(&mut sync_policy); - Some(sync_policy) -} - /// Ensure a `SandboxPolicy` (Rust type) includes the baseline filesystem /// paths required by proxy-mode sandboxes. Used for the /// local-file code path where no proto is available. @@ -1829,89 +1812,6 @@ mod baseline_tests { ); } - #[test] - fn proto_strip_provider_policy_entries_removes_only_reserved_entries() { - let mut policy = openshell_policy::restrictive_default_policy(); - policy.network_policies.insert( - "_provider_work_github".to_string(), - openshell_core::proto::NetworkPolicyRule { - name: "_provider_work_github".to_string(), - ..Default::default() - }, - ); - policy.network_policies.insert( - "sandbox_only".to_string(), - openshell_core::proto::NetworkPolicyRule { - name: "sandbox_only".to_string(), - ..Default::default() - }, - ); - - assert!(strip_proto_provider_policy_entries(&mut policy)); - assert!( - !policy - .network_policies - .contains_key("_provider_work_github") - ); - assert!(policy.network_policies.contains_key("sandbox_only")); - assert!(!strip_proto_provider_policy_entries(&mut policy)); - } - - #[test] - fn proto_sync_payload_not_created_for_provider_entries_without_enrichment() { - let mut runtime_policy = openshell_policy::restrictive_default_policy(); - runtime_policy.network_policies.insert( - "_provider_work_github".to_string(), - openshell_core::proto::NetworkPolicyRule { - name: "_provider_work_github".to_string(), - ..Default::default() - }, - ); - - assert!(proto_sync_payload_for_enriched_policy(&runtime_policy, false).is_none()); - assert!( - runtime_policy - .network_policies - .contains_key("_provider_work_github"), - "provider-derived rules alone must not trigger sync or mutate runtime policy" - ); - } - - #[test] - fn proto_sync_payload_for_enrichment_strips_provider_entries_without_mutating_runtime_policy() { - let mut runtime_policy = openshell_policy::restrictive_default_policy(); - runtime_policy.network_policies.insert( - "_provider_work_github".to_string(), - openshell_core::proto::NetworkPolicyRule { - name: "_provider_work_github".to_string(), - ..Default::default() - }, - ); - runtime_policy.network_policies.insert( - "sandbox_only".to_string(), - openshell_core::proto::NetworkPolicyRule { - name: "sandbox_only".to_string(), - ..Default::default() - }, - ); - - let sync_policy = proto_sync_payload_for_enriched_policy(&runtime_policy, true) - .expect("enrichment should create a sync payload"); - - assert!( - runtime_policy - .network_policies - .contains_key("_provider_work_github"), - "runtime policy must retain provider-derived rules for OPA input" - ); - assert!( - !sync_policy - .network_policies - .contains_key("_provider_work_github") - ); - assert!(sync_policy.network_policies.contains_key("sandbox_only")); - } - #[test] fn no_network_policy_is_unchanged_by_supervisor_baseline() { // A CPU-only workload must start with this policy even when the @@ -1932,7 +1832,6 @@ mod baseline_tests { assert!(!enriched); assert_eq!(policy, original); - assert!(proto_sync_payload_for_enriched_policy(&policy, enriched).is_none()); } #[test] @@ -2405,7 +2304,6 @@ async fn load_policy_with_gateway( // Enrich before syncing so the gateway baseline includes // baseline paths from the start. enrich_proto_baseline_paths(&mut discovered); - strip_proto_provider_policy_entries(&mut discovered); // Sync and re-fetch over a single connection to avoid extra // TLS handshakes. let ws = snapshot.workspace.clone(); @@ -2418,8 +2316,8 @@ async fn load_policy_with_gateway( Err(error) => { // The gateway stored nothing, so report the rejection // against the snapshot this upload was built from and - // upload again on the next pass. Attaching the missing - // provider or setting a sandbox policy repairs startup. + // retry after fetching current configuration. A workspace + // admin can submit a reviewed policy to repair startup. let rejection = startup_write_rejection("image policy", &error).ok_or(error)?; reject_startup_configuration( @@ -2459,51 +2357,9 @@ async fn load_policy_with_gateway( } }; - // Ensure baseline filesystem paths are present for proxy-mode - // sandboxes. If the policy was enriched, sync the updated version - // back to the gateway so users can see the effective policy. Only - // a sandbox-sourced policy is written back. The gateway refuses - // every sandbox policy write while a global policy is active, so a - // global policy keeps the added paths in this process only. - let enriched = enrich_proto_baseline_paths(&mut proto_policy); - let sync_policy = proto_sync_payload_for_enriched_policy(&proto_policy, enriched) - .filter(|_| snapshot.policy_source == openshell_core::proto::PolicySource::Sandbox); - if let Some(sync_policy) = sync_policy { - let synced = grpc_retry("Enriched policy synchronization", || { - gateway.sync(sandbox, &sync_policy, &snapshot.workspace) - }) - .await; - let canonical = match synced { - Ok(canonical) => canonical, - Err(error) => { - // The stored policy is unchanged, so report the - // rejection against the snapshot it was read from and - // read again on the next pass, which picks up a - // replacement policy or a provider repair. - let rejection = startup_write_rejection( - "policy update that adds baseline filesystem paths", - &error, - ) - .ok_or(error)?; - reject_startup_configuration( - gateway, - &mut rejection_log, - id, - &instance_id, - &snapshot, - &rejection.diagnostic, - Some(&rejection.log_key), - ) - .await?; - reconciliation_attempts = 0; - continue; - } - }; - proto_policy = canonical.policy.clone().ok_or_else(|| { - miette::miette!("Gateway returned no effective policy after enrichment") - })?; - snapshot = canonical; - } + // Runtime baseline paths are local requirements. Keep the gateway's + // authored policy and revision identity unchanged. + enrich_proto_baseline_paths(&mut proto_policy); let loaded_policy_revision = Some({ let mut revision = LoadedPolicyRevision::from_snapshot(&snapshot); @@ -3143,7 +2999,7 @@ struct LoadedPolicyRevision { /// Identifies where the policy currently loaded into OPA came from. /// /// A missing gateway revision means the policy was loaded from the gateway but -/// could not be bound to an authoritative snapshot (for example, enrichment +/// could not be bound to an authoritative snapshot (for example, initial adoption /// sync failed). That state must reconcile on the first successful poll. A /// local-file override is different: gateway policy revisions are observed for /// settings/provider refreshes but must never replace the explicit local OPA @@ -6428,30 +6284,6 @@ network_policies: .await; } - #[tokio::test(start_paused = true)] - async fn startup_policy_write_refusal_baseline_write_back_waits_for_repair() { - // The gateway policy has network rules but lacks the proxy baseline - // paths, so startup writes an enriched copy back. The repaired policy - // already carries those paths and needs no write-back. - let mut repaired = proto_tcp_policy_fixture(); - assert!(enrich_proto_baseline_paths(&mut repaired)); - assert_refused_startup_write_waits_for_repair( - settings_poll_result( - Some(proto_tcp_policy_fixture()), - 1, - openshell_core::proto::PolicySource::Sandbox, - ), - ImagePolicyDiscovery::Missing, - "policy update that adds baseline filesystem paths", - vec![SyncRefusal::new( - tonic::Code::FailedPrecondition, - UNATTACHED_PROVIDER_DIAGNOSTIC, - )], - repaired, - ) - .await; - } - /// Install an operator's repair after the refused write but before its /// rejection report. The obsolete report must lead straight to a new /// snapshot, without resending the write or delaying the repaired launch. @@ -6557,24 +6389,6 @@ network_policies: .await; } - #[tokio::test(start_paused = true)] - async fn startup_policy_write_refusal_baseline_repair_before_report_refetches() { - let mut repaired = proto_tcp_policy_fixture(); - assert!(enrich_proto_baseline_paths(&mut repaired)); - Box::pin(assert_startup_repair_before_rejection_refetches( - settings_poll_result( - Some(proto_tcp_policy_fixture()), - 1, - openshell_core::proto::PolicySource::Sandbox, - ), - ImagePolicyDiscovery::Missing, - repaired, - SyncRefusal::new(tonic::Code::InvalidArgument, INVALID_MIDDLEWARE_DIAGNOSTIC), - "policy update that adds baseline filesystem paths", - )) - .await; - } - /// Exercise rejection reporting directly so transport and identity errors /// cannot be mistaken for policy-write failures or registration failures. async fn startup_rejection_report_with_errors( @@ -6700,31 +6514,6 @@ network_policies: .await; } - #[tokio::test(start_paused = true)] - async fn startup_policy_write_refusal_write_back_outlasts_the_attempt_limit() { - // The same limit applies to a refused baseline-path write-back. - let mut repaired = proto_tcp_policy_fixture(); - assert!(enrich_proto_baseline_paths(&mut repaired)); - assert_refused_startup_write_waits_for_repair( - settings_poll_result( - Some(proto_tcp_policy_fixture()), - 1, - openshell_core::proto::PolicySource::Sandbox, - ), - ImagePolicyDiscovery::Missing, - "policy update that adds baseline filesystem paths", - vec![ - SyncRefusal::new( - tonic::Code::FailedPrecondition, - UNATTACHED_PROVIDER_DIAGNOSTIC - ); - 7 - ], - repaired, - ) - .await; - } - /// Collects the sandbox log lines that a test's OCSF events render to. #[derive(Clone, Default)] struct CapturedLog(Arc>>); @@ -6787,16 +6576,24 @@ network_policies: ); } + #[tokio::test(start_paused = true)] + async fn startup_existing_sandbox_policy_enriches_locally_without_upload() { + assert_startup_enriches_locally(openshell_core::proto::PolicySource::Sandbox).await; + } + #[tokio::test(start_paused = true)] async fn startup_policy_write_refusal_global_policy_skips_baseline_write_back() { + assert_startup_enriches_locally(openshell_core::proto::PolicySource::Global).await; + } + + async fn assert_startup_enriches_locally(source: openshell_core::proto::PolicySource) { use openshell_core::proto::{ConfigurationAdmissionState, PolicySource}; // The gateway serves a global policy that has network rules but lacks // the proxy baseline paths. It refuses every sandbox policy write while // that policy is active, so startup must add the paths locally and // install the policy without writing it back. - let mut global = - settings_poll_result(Some(proto_tcp_policy_fixture()), 1, PolicySource::Global); - global.global_policy_version = 1; + let mut global = settings_poll_result(Some(proto_tcp_policy_fixture()), 1, source); + global.global_policy_version = u32::from(source == PolicySource::Global); let global_generation = report_generation(&global); let mut enriched = proto_tcp_policy_fixture(); assert!(enrich_proto_baseline_paths(&mut enriched)); @@ -6864,29 +6661,18 @@ network_policies: else { panic!("startup must bind the global policy to its gateway revision"); }; - assert_eq!(revision.policy_source, PolicySource::Global); + assert_eq!(revision.policy_source, source); } #[tokio::test(start_paused = true)] async fn startup_policy_write_refusal_permanent_codes_still_exit() { - use openshell_core::proto::{ConfigurationAdmissionState, PolicySource}; - // No policy or provider change repairs these refusals, so startup - // keeps exiting instead of holding the sandbox in its repair window. - // Both writes are checked: the image policy upload to a gateway with - // no policy, and the write-back of a stored policy that lacks the - // proxy baseline paths. - let writes = [ - ( - "image policy upload", - unset_policy_snapshot(), - ImagePolicyDiscovery::Policy(Box::new(proto_tcp_policy_fixture())), - ), - ( - "baseline write-back", - settings_poll_result(Some(proto_tcp_policy_fixture()), 1, PolicySource::Sandbox), - ImagePolicyDiscovery::Missing, - ), - ]; + use openshell_core::proto::ConfigurationAdmissionState; + // Authentication failures in initial adoption remain fatal. + let writes = [( + "image policy upload", + unset_policy_snapshot(), + ImagePolicyDiscovery::Policy(Box::new(proto_tcp_policy_fixture())), + )]; for (write, initial, discovery) in writes { for code in [ tonic::Code::PermissionDenied, diff --git a/docs/how-it-works/policies/advisor.mdx b/docs/how-it-works/policies/advisor.mdx index 66e04d68cd..2f07e3875c 100644 --- a/docs/how-it-works/policies/advisor.mdx +++ b/docs/how-it-works/policies/advisor.mdx @@ -153,8 +153,10 @@ including automatically. You can also review proposals in the terminal UI with ## Approve Proposals Automatically By default, every proposal waits for your review. In automatic mode, OpenShell -approves a proposal without review when both of these are true: +approves a proposal without review when all of these are true: +- The proposal keeps credential bindings and their rule and process scope unchanged. + Binding changes require workspace admin approval. - The proposal risk check finds none of the risks described in [Proposal Risk Check](#proposal-risk-check). - OpenShell has not flagged the proposal's destination. OpenShell flags hosts diff --git a/docs/how-it-works/policies/default-policy.mdx b/docs/how-it-works/policies/default-policy.mdx index 3f148dcf70..2e9166e6fb 100644 --- a/docs/how-it-works/policies/default-policy.mdx +++ b/docs/how-it-works/policies/default-policy.mdx @@ -74,12 +74,10 @@ baseline read-write path that you list as read-only stays read-only. If the policy has no `filesystem_policy` section, OpenShell creates one with `include_workdir: true`. -The sandbox saves the enriched filesystem policy as a new revision, so the -added paths appear in `openshell policy get --base`. While a global policy is -active, the sandbox adds the paths without saving a revision, so they do not -appear in the policy views. OpenShell can reject a -replacement policy that removes filesystem paths, so keep the added paths when -you replace the complete policy. +When adopting an image policy, OpenShell saves the initial baseline paths with +that policy. For a policy already saved through the gateway, OpenShell adds +missing baseline paths locally without creating a new policy revision. These +local additions do not appear in `openshell policy get --base`. The runtime also grants the workload read-only access to the sandbox's TLS CA certificates under `/run/openshell-supervisor-ca`. This grant is not saved in diff --git a/docs/how-it-works/policies/manage-policies.mdx b/docs/how-it-works/policies/manage-policies.mdx index c20af7b67b..b326187939 100644 --- a/docs/how-it-works/policies/manage-policies.mdx +++ b/docs/how-it-works/policies/manage-policies.mdx @@ -45,8 +45,11 @@ To include a policy in a sandbox image, add it at `/etc/openshell/policy.yaml`: COPY policy.yaml /etc/openshell/policy.yaml ``` -OpenShell uses the image policy only when the sandbox has no saved policy, so a -`--policy` file or `OPENSHELL_SANDBOX_POLICY` takes precedence. If the image +OpenShell adopts the image policy once, only when the sandbox has no saved policy, so a +`--policy` file or `OPENSHELL_SANDBOX_POLICY` takes precedence. An image policy +must not contain `credential_binding`; a workspace admin must submit policies +with credential bindings through the gateway using `openshell policy set`. +Restarts use the saved policy. If the image policy is invalid, the sandbox does not start until you [replace it with a valid policy](#a-new-sandbox-stays-in-provisioning). OpenShell does not fall back to the default policy. @@ -366,7 +369,7 @@ check the sandbox log for the specific error: openshell sandbox get my-sandbox --output json ``` -The same repair window applies when the gateway refuses an image-policy upload or a policy update that adds baseline filesystem paths with `FAILED_PRECONDITION` or `INVALID_ARGUMENT`. After the gateway acknowledges the rejection report, startup waits two seconds, reads a fresh configuration, and retries. If your repair reaches the gateway before that report, startup immediately reads the repaired configuration. The sandbox log records the gateway's reason; repeated refusals of the same write, status code, and configuration are logged once. Authentication and authorization failures still stop startup. +The same repair window applies when the gateway refuses an initial image-policy upload with `FAILED_PRECONDITION` or `INVALID_ARGUMENT`. After the gateway acknowledges the rejection report, startup waits two seconds, reads a fresh configuration, and retries. If your repair reaches the gateway before that report, startup immediately reads the repaired configuration. The sandbox log records the gateway's reason; repeated refusals of the same write, status code, and configuration are logged once. Authentication and authorization failures still stop startup. You have 300 seconds to fix the configuration after the supervisor starts admission. Image preparation has its own deadline and does not consume that repair time. Replace the policy with `openshell policy set`, or fix the provider configuration. Until the workload first starts, you can also change filesystem, Landlock, and process settings. Each effective change to the policy, providers, or settings restarts the 300 seconds; writing an unchanged value does not. diff --git a/e2e/rust/tests/live_policy_update.rs b/e2e/rust/tests/live_policy_update.rs index b6a35c8898..b59d6a628e 100644 --- a/e2e/rust/tests/live_policy_update.rs +++ b/e2e/rust/tests/live_policy_update.rs @@ -625,22 +625,8 @@ async fn live_policy_update_from_empty_network_policies() { guard.cleanup().await; } -/// Regression for #2159: a sparse initial policy that the supervisor enriches -/// with baseline filesystem paths during startup must have its resulting -/// revision acknowledged as loaded, not left `Pending`. -/// -/// Reproduction (matches the maintainer's triage): create a sandbox with the -/// network-only `examples/policy-advisor/sandbox-policy.yaml`. The supervisor -/// adds baseline filesystem paths, syncs the enriched policy back to the -/// gateway (creating revision 2, superseding revision 1), builds the OPA engine -/// and then acknowledges revision 2 as loaded. Before the fix, revision 2 -/// stayed `Pending` even though the sandbox was `Ready` and the policy was -/// effective. -/// -/// NOTE: This exercises the Docker-backed supervisor built from this branch. -/// The exact `policy list` status wording ("Loaded"/"Superseded") may differ by -/// CLI version; the assertions below key on the effective version reaching 2 and -/// no revision remaining `Pending` once the acknowledgement lands. +/// A sparse gateway policy must be acknowledged as loaded after local runtime +/// filesystem enrichment, without creating a second authored revision. #[tokio::test] async fn initial_sparse_policy_is_acknowledged_as_loaded() { let sparse_policy = write_sparse_policy().expect("write sparse policy fixture"); @@ -663,9 +649,8 @@ async fn initial_sparse_policy_is_acknowledged_as_loaded() { .await .expect("create keep sandbox with sparse policy"); - // The enriched revision (2) is synced during startup; the acknowledgement - // (LOADED) is delivered by the supervisor's poll loop shortly after Ready. - // Poll until the effective policy is version 2 and no revision is Pending. + // Startup enriches locally and acknowledges the original gateway revision. + // Poll until that revision is loaded. let mut acknowledged = false; let mut last_list = String::new(); for _ in 0..30 { @@ -676,7 +661,7 @@ async fn initial_sparse_policy_is_acknowledged_as_loaded() { last_list = list.output.clone(); let pending = list.output.to_lowercase().contains("pending"); - if version == Some(2) && list.success && !pending { + if version == Some(1) && list.success && !pending { acknowledged = true; break; } @@ -685,15 +670,13 @@ async fn initial_sparse_policy_is_acknowledged_as_loaded() { assert!( acknowledged, - "enriched initial policy should reach revision 2 with no Pending revision.\n\ + "initial policy should remain revision 1 with no Pending revision.\n\ last `policy list` output:\n{last_list}" ); - // Both the superseded original (1) and the loaded enriched revision (2) - // must appear in the revision history. assert!( - list_output_contains_version(&last_list, 2), - "policy list should contain revision 2:\n{last_list}" + !list_output_contains_version(&last_list, 2), + "local enrichment must not create revision 2:\n{last_list}" ); assert!( list_output_contains_version(&last_list, 1), diff --git a/e2e/rust/tests/policy_activation.rs b/e2e/rust/tests/policy_activation.rs index 0491e80c3f..fe40c7adae 100644 --- a/e2e/rust/tests/policy_activation.rs +++ b/e2e/rust/tests/policy_activation.rs @@ -381,3 +381,95 @@ binaries: drop(resources); drop(image); } + +#[tokio::test] +async fn image_credential_binding_waits_for_admin_repair_without_launching() { + let suffix = format!("{:016x}", rand::random::()); + let resources = Resources { + sandbox: format!("bi-{suffix}"), + standalone_sandbox: format!("bl-{suffix}"), + provider: format!("bp-{suffix}"), + }; + let context = tempfile::tempdir().unwrap(); + let image_policy = POLICY.replace( + " port: 443", + &format!( + " port: 443\n credential_binding:\n provider: {}", + resources.provider + ), + ); + std::fs::write(context.path().join("policy.yaml"), image_policy).unwrap(); + std::fs::write(context.path().join("Dockerfile"), r#"FROM public.ecr.aws/docker/library/python:3.13-slim +RUN groupadd sandbox && useradd -m -g sandbox sandbox && mkdir -p /sandbox && chown sandbox:sandbox /sandbox +COPY policy.yaml /etc/openshell/policy.yaml +WORKDIR /sandbox +USER sandbox +"#).unwrap(); + let image = ImageGuard::build( + "image-binding", + &context.path().join("Dockerfile"), + context.path(), + ) + .unwrap(); + let mut create = openshell_cmd() + .args([ + "sandbox", + "create", + "--name", + &resources.sandbox, + "--detach", + "--from", + image.tag(), + "--", + "sh", + "-c", + WORKLOAD, + ]) + .stdout(Stdio::null()) + .stderr(Stdio::inherit()) + .spawn() + .unwrap(); + let diagnostic = "Image policy contains credential_binding. A workspace admin must submit the reviewed policy through the gateway."; + tokio::time::timeout(Duration::from_secs(90), async { + loop { + assert!( + create.try_wait().unwrap().is_none(), + "sandbox create exited before configuration rejection" + ); + let (output, code) = + run_cli(&["sandbox", "get", &resources.sandbox, "--output", "json"]).await; + if code == 0 && output.contains(diagnostic) { + let details: serde_json::Value = + serde_json::from_str(&strip_ansi(&output)).unwrap(); + assert_eq!(details["phase"].as_str(), Some("Provisioning"), "{output}"); + assert!(output.contains("ConfigurationReady"), "{output}"); + assert!(output.contains("ConfigurationInvalid"), "{output}"); + break; + } + tokio::time::sleep(Duration::from_millis(500)).await; + } + }) + .await + .expect("image binding must produce an actionable startup diagnostic"); + let engine = ContainerEngine::from_env().unwrap(); + let container = container_id(&engine, &resources.sandbox); + assert_marker(&engine, &container, false); + assert!(create.try_wait().unwrap().is_none()); + let repaired = context.path().join("reviewed.yaml"); + std::fs::write(&repaired, POLICY).unwrap(); + cli_ok(&[ + "policy", + "set", + &resources.sandbox, + "--policy", + repaired.to_str().unwrap(), + "--wait", + ]) + .await; + let status = tokio::time::timeout(Duration::from_secs(120), create.wait()) + .await + .unwrap() + .unwrap(); + assert!(status.success()); + wait_for_marker(&engine, &container).await; +} diff --git a/skills/generate-sandbox-policy/SKILL.md b/skills/generate-sandbox-policy/SKILL.md index db349e0068..5c90d6cdf8 100644 --- a/skills/generate-sandbox-policy/SKILL.md +++ b/skills/generate-sandbox-policy/SKILL.md @@ -182,6 +182,10 @@ When middleware is requested, also read the published [supervisor middleware gui For enforcement concepts and the shipped baseline, read [sandbox policies](https://docs.nvidia.com/openshell/latest/how-it-works/policies/overview) and the [default policy reference](https://docs.nvidia.com/openshell/latest/how-it-works/policies/default-policy). The default policy is built into the OpenShell runtime and applies when no explicit policy is supplied. +Image policies are adopted only during initial sandbox setup and must not +contain `credential_binding`. Submit credential bindings through the gateway +with `openshell policy set` as a workspace admin. + Validate the intended provider combination as well as the authored policy. An image endpoint can become credentialed after provider composition and block startup with `ConfigurationInvalid`. Repair the complete policy or provider diff --git a/skills/openshell-cli/SKILL.md b/skills/openshell-cli/SKILL.md index 472d7fadbb..94cbfaf804 100644 --- a/skills/openshell-cli/SKILL.md +++ b/skills/openshell-cli/SKILL.md @@ -689,6 +689,12 @@ Review the proposed scope, candidate hash, prover findings, and application erro Build a custom container image and run it as a sandbox. +An embedded policy is adopted only during initial setup when no gateway policy +is saved. Embedded policies must not contain `credential_binding`; a workspace +admin supplies those through `openshell policy set`. A rejected image policy +keeps the sandbox in `Provisioning` until repaired, within its existing repair +window. Check `ConfigurationReady` for the actionable diagnostic. + ### Create a sandbox from a pre-built image ```bash