From 9430cef2a914a13ef27ac1c602f0729dd3069bab Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Wed, 24 Jun 2026 11:58:48 -0700 Subject: [PATCH] fix(policy): reserve provider rule namespace Closes #1982 Reject user-authored _provider_* network policy keys at gateway boundaries, strip provider-derived rules from sandbox sync, and document the round-trip workflow. Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> --- crates/openshell-cli/src/main.rs | 41 ++- crates/openshell-cli/src/run.rs | 79 +++-- .../sandbox_name_fallback_integration.rs | 95 +++++- crates/openshell-policy/src/compose.rs | 89 ++++- crates/openshell-policy/src/lib.rs | 5 +- crates/openshell-policy/src/merge.rs | 6 +- crates/openshell-sandbox/src/lib.rs | 107 +++++- crates/openshell-server/src/grpc/policy.rs | 309 +++++++++++++++++- crates/openshell-server/src/grpc/sandbox.rs | 36 +- .../openshell-server/src/grpc/validation.rs | 53 +++ docs/sandboxes/policies.mdx | 6 +- docs/sandboxes/providers-v2.mdx | 16 +- 12 files changed, 754 insertions(+), 88 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index fbed00e1a..3b9cd9993 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -1693,10 +1693,14 @@ enum PolicyCommands { #[arg(long = "rev", default_value_t = 0)] rev: u32, - /// Include the full policy payload. - #[arg(long)] + /// Include the effective policy payload, including provider-composed entries. + #[arg(long, conflicts_with = "base")] full: bool, + /// Include the base policy payload without provider-composed entries. + #[arg(long)] + base: bool, + /// Output format. #[arg(short = 'o', long = "output", value_enum, default_value_t = PolicyGetOutput::Table)] output: PolicyGetOutput, @@ -2378,14 +2382,16 @@ async fn main() -> Result<()> { name, rev, full, + base, output, global, } => { + let view = run::PolicyGetView::from_flags(base, full); if global { run::sandbox_policy_get_global( &ctx.endpoint, rev, - full, + view, output.as_str(), &tls, ) @@ -2396,7 +2402,7 @@ async fn main() -> Result<()> { &ctx.endpoint, &name, rev, - full, + view, output.as_str(), &tls, ) @@ -4364,17 +4370,42 @@ mod tests { Some(Commands::Policy { command: Some(PolicyCommands::Get { - name, full, output, .. + name, + full, + base, + output, + .. }), }) => { assert_eq!(name.as_deref(), Some("my-sandbox")); assert!(full); + assert!(!base); assert!(matches!(output, PolicyGetOutput::Json)); } other => panic!("expected policy get command, got: {other:?}"), } } + #[test] + fn policy_get_base_output_parses() { + let cli = Cli::try_parse_from(["openshell", "policy", "get", "my-sandbox", "--base"]) + .expect("policy get --base should parse"); + + match cli.command { + Some(Commands::Policy { + command: + Some(PolicyCommands::Get { + name, full, base, .. + }), + }) => { + assert_eq!(name.as_deref(), Some("my-sandbox")); + assert!(!full); + assert!(base); + } + other => panic!("expected policy get command, got: {other:?}"), + } + } + #[test] fn policy_delete_global_parses() { let cli = Cli::try_parse_from(["openshell", "policy", "delete", "--global", "--yes"]) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 29468aa49..b3b179a70 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -63,6 +63,7 @@ use openshell_providers::{ profile_to_json, profile_to_yaml, profiles_to_json, profiles_to_yaml, }; use owo_colors::OwoColorize; +use std::borrow::Cow; use std::collections::{HashMap, HashSet}; use std::io::{ErrorKind, IsTerminal, Read, Write}; use std::path::{Path, PathBuf}; @@ -80,6 +81,27 @@ pub use openshell_core::forward::{ find_forward_by_port, list_forwards, stop_forward, stop_forwards_for_sandbox, }; +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum PolicyGetView { + Metadata, + Base, + Full, +} + +impl PolicyGetView { + pub fn from_flags(base: bool, full: bool) -> Self { + match (base, full) { + (true, _) => Self::Base, + (false, true) => Self::Full, + (false, false) => Self::Metadata, + } + } + + fn includes_policy(self) -> bool { + matches!(self, Self::Base | Self::Full) + } +} + #[derive(Debug, PartialEq, Eq)] enum SandboxUploadPlan { GitAware { @@ -6867,7 +6889,7 @@ pub async fn sandbox_policy_get( server: &str, name: &str, version: u32, - full: bool, + view: PolicyGetView, output: &str, tls: &TlsOptions, ) -> Result<()> { @@ -6877,7 +6899,7 @@ pub async fn sandbox_policy_get( server, name, version, - full, + view, output, tls, (&mut stdout, &mut stderr), @@ -6901,7 +6923,7 @@ pub async fn sandbox_policy_get_to_writer( server: &str, name: &str, version: u32, - full: bool, + view: PolicyGetView, output: &str, tls: &TlsOptions, writers: (&mut W, &mut E), @@ -6911,7 +6933,7 @@ where E: Write + Send, { if version == 0 { - return sandbox_policy_get_effective_to_writer(server, name, full, output, tls, writers) + return sandbox_policy_get_effective_to_writer(server, name, view, output, tls, writers) .await; } @@ -6938,7 +6960,7 @@ where Some(inner.active_version), &rev, status, - full, + view, )?; writeln!( stdout, @@ -6966,10 +6988,11 @@ where writeln!(stdout, "Error: {}", rev.load_error).into_diagnostic()?; } - if full { + if view.includes_policy() { if let Some(ref policy) = rev.policy { writeln!(stdout, "---").into_diagnostic()?; - let yaml_str = openshell_policy::serialize_sandbox_policy(policy) + let policy = policy_for_view(policy, view); + let yaml_str = openshell_policy::serialize_sandbox_policy(policy.as_ref()) .wrap_err("failed to serialize policy to YAML")?; write!(stdout, "{yaml_str}").into_diagnostic()?; } else { @@ -6987,7 +7010,7 @@ where async fn sandbox_policy_get_effective_to_writer( server: &str, name: &str, - full: bool, + view: PolicyGetView, output: &str, tls: &TlsOptions, writers: (&mut W, &mut E), @@ -7060,10 +7083,11 @@ where serde_json::json!(config.global_policy_version), ); } - if full { + if view.includes_policy() { + let policy = policy_for_view(policy, view); obj.insert( "policy".to_string(), - openshell_policy::sandbox_policy_to_json_value(policy)?, + openshell_policy::sandbox_policy_to_json_value(policy.as_ref())?, ); } writeln!( @@ -7083,9 +7107,10 @@ where writeln!(stdout, "Global: {}", config.global_policy_version) .into_diagnostic()?; } - if full { + if view.includes_policy() { writeln!(stdout, "---").into_diagnostic()?; - let yaml_str = openshell_policy::serialize_sandbox_policy(policy) + let policy = policy_for_view(policy, view); + let yaml_str = openshell_policy::serialize_sandbox_policy(policy.as_ref()) .wrap_err("failed to serialize policy to YAML")?; write!(stdout, "{yaml_str}").into_diagnostic()?; } @@ -7099,7 +7124,7 @@ where pub async fn sandbox_policy_get_global( server: &str, version: u32, - full: bool, + view: PolicyGetView, output: &str, tls: &TlsOptions, ) -> Result<()> { @@ -7119,7 +7144,7 @@ pub async fn sandbox_policy_get_global( let status = PolicyStatus::try_from(rev.status).unwrap_or(PolicyStatus::Unspecified); match output { "json" => { - let obj = policy_revision_to_json("global", None, None, &rev, status, full)?; + let obj = policy_revision_to_json("global", None, None, &rev, status, view)?; println!("{}", serde_json::to_string_pretty(&obj).into_diagnostic()?); return Ok(()); } @@ -7138,10 +7163,11 @@ pub async fn sandbox_policy_get_global( println!("Loaded: {} ms", rev.loaded_at_ms); } - if full { + if view.includes_policy() { if let Some(ref policy) = rev.policy { println!("---"); - let yaml_str = openshell_policy::serialize_sandbox_policy(policy) + let policy = policy_for_view(policy, view); + let yaml_str = openshell_policy::serialize_sandbox_policy(policy.as_ref()) .wrap_err("failed to serialize policy to YAML")?; print!("{yaml_str}"); } else { @@ -7171,7 +7197,7 @@ fn policy_revision_to_json( active_version: Option, rev: &openshell_core::proto::SandboxPolicyRevision, status: PolicyStatus, - full: bool, + view: PolicyGetView, ) -> Result { let mut obj = serde_json::Map::new(); obj.insert("scope".to_string(), serde_json::json!(scope)); @@ -7205,9 +7231,12 @@ fn policy_revision_to_json( if !rev.load_error.is_empty() { obj.insert("load_error".to_string(), serde_json::json!(rev.load_error)); } - if full { + if view.includes_policy() { let policy = match rev.policy.as_ref() { - Some(policy) => openshell_policy::sandbox_policy_to_json_value(policy)?, + Some(policy) => { + let policy = policy_for_view(policy, view); + openshell_policy::sandbox_policy_to_json_value(policy.as_ref())? + } None => serde_json::Value::Null, }; obj.insert("policy".to_string(), policy); @@ -7215,6 +7244,18 @@ fn policy_revision_to_json( Ok(serde_json::Value::Object(obj)) } +fn policy_for_view(policy: &SandboxPolicy, view: PolicyGetView) -> Cow<'_, SandboxPolicy> { + if view != PolicyGetView::Base { + return Cow::Borrowed(policy); + } + + let mut base_policy = policy.clone(); + base_policy + .network_policies + .retain(|name, _| !openshell_policy::is_provider_rule_name(name)); + Cow::Owned(base_policy) +} + pub async fn sandbox_policy_list( server: &str, name: &str, diff --git a/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs b/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs index d4052ff68..8e799f821 100644 --- a/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs +++ b/crates/openshell-cli/tests/sandbox_name_fallback_integration.rs @@ -132,21 +132,39 @@ impl OpenShell for TestOpenShell { Ok(Response::new(GetSandboxConfigResponse { policy: Some(SandboxPolicy { version: 9, - network_policies: std::iter::once(( - "_provider_api".to_string(), - NetworkPolicyRule { - name: "_provider_api".to_string(), - endpoints: vec![NetworkEndpoint { - host: "api.provider.example.com".to_string(), - port: 443, - protocol: "rest".to_string(), - enforcement: "enforce".to_string(), - access: "read-only".to_string(), + network_policies: [ + ( + "user_api".to_string(), + NetworkPolicyRule { + name: "user_api".to_string(), + endpoints: vec![NetworkEndpoint { + host: "api.user.example.com".to_string(), + port: 443, + protocol: "rest".to_string(), + enforcement: "enforce".to_string(), + access: "read-only".to_string(), + ..Default::default() + }], ..Default::default() - }], - ..Default::default() - }, - )) + }, + ), + ( + "_provider_api".to_string(), + NetworkPolicyRule { + name: "_provider_api".to_string(), + endpoints: vec![NetworkEndpoint { + host: "api.provider.example.com".to_string(), + port: 443, + protocol: "rest".to_string(), + enforcement: "enforce".to_string(), + access: "read-only".to_string(), + ..Default::default() + }], + ..Default::default() + }, + ), + ] + .into_iter() .collect(), ..Default::default() }), @@ -675,7 +693,7 @@ async fn policy_get_full_json_cli_prints_policy_payload() { &ts.endpoint, "my-sandbox", 0, - true, + run::PolicyGetView::Full, "json", &ts.tls, (&mut stdout, &mut stderr), @@ -707,6 +725,51 @@ async fn policy_get_full_json_cli_prints_policy_payload() { json["policy"]["network_policies"]["_provider_api"]["endpoints"][0]["host"], "api.provider.example.com" ); + assert_eq!( + json["policy"]["network_policies"]["user_api"]["endpoints"][0]["host"], + "api.user.example.com" + ); +} + +#[tokio::test] +async fn policy_get_base_json_cli_prints_round_trippable_policy_payload() { + let ts = run_server().await; + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + run::sandbox_policy_get_to_writer( + &ts.endpoint, + "my-sandbox", + 0, + run::PolicyGetView::Base, + "json", + &ts.tls, + (&mut stdout, &mut stderr), + ) + .await + .expect("policy get --base should succeed"); + + assert!( + stderr.is_empty(), + "policy get --base should not print stderr: {}", + String::from_utf8_lossy(&stderr) + ); + + let json: serde_json::Value = + serde_json::from_slice(&stdout).expect("stdout should be valid JSON"); + assert_eq!(json["scope"], "sandbox"); + assert_eq!(json["sandbox"], "my-sandbox"); + assert_eq!(json["status"], "effective"); + assert!( + json["policy"]["network_policies"] + .get("_provider_api") + .is_none(), + "base policy output must omit provider-composed policy entries" + ); + assert_eq!( + json["policy"]["network_policies"]["user_api"]["endpoints"][0]["host"], + "api.user.example.com" + ); } #[tokio::test] @@ -719,7 +782,7 @@ async fn policy_get_explicit_revision_uses_stored_policy_status() { &ts.endpoint, "my-sandbox", 3, - true, + run::PolicyGetView::Full, "json", &ts.tls, (&mut stdout, &mut stderr), diff --git a/crates/openshell-policy/src/compose.rs b/crates/openshell-policy/src/compose.rs index 427fa4cda..7ca8584d9 100644 --- a/crates/openshell-policy/src/compose.rs +++ b/crates/openshell-policy/src/compose.rs @@ -5,12 +5,19 @@ use openshell_core::proto::{NetworkPolicyRule, SandboxPolicy}; +pub const PROVIDER_RULE_NAME_PREFIX: &str = "_provider_"; + #[derive(Debug, Clone, PartialEq)] pub struct ProviderPolicyLayer { pub rule_name: String, pub rule: NetworkPolicyRule, } +#[must_use] +pub fn is_provider_rule_name(rule_name: &str) -> bool { + rule_name.starts_with(PROVIDER_RULE_NAME_PREFIX) +} + #[must_use] pub fn provider_rule_name(provider_name: &str) -> String { let sanitized = provider_name @@ -27,19 +34,27 @@ pub fn provider_rule_name(provider_name: &str) -> String { .to_string(); if sanitized.is_empty() { - "_provider_unnamed".to_string() + format!("{PROVIDER_RULE_NAME_PREFIX}unnamed") } else { - format!("_provider_{sanitized}") + format!("{PROVIDER_RULE_NAME_PREFIX}{sanitized}") } } +pub fn strip_provider_rule_names(policy: &mut SandboxPolicy) -> bool { + let original_len = policy.network_policies.len(); + policy + .network_policies + .retain(|key, _| !is_provider_rule_name(key)); + policy.network_policies.len() != original_len +} + /// Compose a normal sandbox policy from user-authored policy plus provider /// policy layers. /// /// The returned policy is derived data. It preserves the source policy's /// static fields and user-authored network policies, then concatenates each -/// provider rule under a reserved `_provider_*` key. Existing user keys are not -/// overwritten; a numeric suffix is added if needed. +/// provider rule under a reserved `_provider_*` key. Existing keys are not +/// overwritten; a numeric suffix is added if provider rule names collide. #[must_use] pub fn compose_effective_policy( source_policy: &SandboxPolicy, @@ -76,7 +91,10 @@ fn unique_provider_rule_key(policy: &SandboxPolicy, preferred: &str) -> String { #[cfg(test)] mod tests { - use super::{ProviderPolicyLayer, compose_effective_policy, provider_rule_name}; + use super::{ + PROVIDER_RULE_NAME_PREFIX, ProviderPolicyLayer, compose_effective_policy, + is_provider_rule_name, provider_rule_name, strip_provider_rule_names, + }; use openshell_core::proto::{NetworkEndpoint, NetworkPolicyRule, SandboxPolicy}; fn rule(name: &str, host: &str) -> NetworkPolicyRule { @@ -107,6 +125,38 @@ mod tests { assert_eq!(provider_rule_name("..."), "_provider_unnamed"); } + #[test] + fn provider_rule_name_prefix_identifies_reserved_keys() { + assert_eq!(PROVIDER_RULE_NAME_PREFIX, "_provider_"); + assert!(is_provider_rule_name("_provider_work_github")); + assert!(is_provider_rule_name("_provider_work_github_2")); + assert!(is_provider_rule_name("_provider_")); + assert!(!is_provider_rule_name("provider_work_github")); + assert!(!is_provider_rule_name("custom_provider_work_github")); + } + + #[test] + fn strip_provider_rule_names_removes_only_reserved_keys() { + let mut policy = SandboxPolicy::default(); + policy.network_policies.insert( + "_provider_work_github".to_string(), + rule("_provider_work_github", "api.github.com"), + ); + policy.network_policies.insert( + "sandbox_only".to_string(), + rule("sandbox_only", "sandbox.example.com"), + ); + + assert!(strip_provider_rule_names(&mut policy)); + assert!( + !policy + .network_policies + .contains_key("_provider_work_github") + ); + assert!(policy.network_policies.contains_key("sandbox_only")); + assert!(!strip_provider_rule_names(&mut policy)); + } + #[test] fn compose_concatenates_provider_rules_without_overwriting_user_rules() { let mut source = SandboxPolicy::default(); @@ -114,17 +164,19 @@ mod tests { "custom_github".to_string(), rule("custom_github", "api.github.com"), ); - source.network_policies.insert( - "_provider_work_github".to_string(), - rule("_provider_work_github", "example.com"), - ); let effective = compose_effective_policy( &source, - &[ProviderPolicyLayer { - rule_name: "_provider_work_github".to_string(), - rule: rule("_provider_work_github", "github.com"), - }], + &[ + ProviderPolicyLayer { + rule_name: "_provider_work_github".to_string(), + rule: rule("_provider_work_github", "github.com"), + }, + ProviderPolicyLayer { + rule_name: "_provider_work_github".to_string(), + rule: rule("_provider_work_github", "github.example.com"), + }, + ], ); assert!(effective.network_policies.contains_key("custom_github")); @@ -138,7 +190,16 @@ mod tests { .network_policies .contains_key("_provider_work_github_2") ); - assert_eq!(source.network_policies.len(), 2); + assert_eq!( + effective + .network_policies + .get("custom_github") + .unwrap() + .endpoints[0] + .host, + "api.github.com" + ); + assert_eq!(source.network_policies.len(), 1); assert_eq!(effective.network_policies.len(), 3); } } diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index aaabbf926..22af7093e 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -24,7 +24,10 @@ use openshell_core::proto::{ }; use serde::{Deserialize, Serialize}; -pub use compose::{ProviderPolicyLayer, compose_effective_policy, provider_rule_name}; +pub use compose::{ + PROVIDER_RULE_NAME_PREFIX, ProviderPolicyLayer, compose_effective_policy, + is_provider_rule_name, provider_rule_name, strip_provider_rule_names, +}; pub use merge::{ PolicyMergeError, PolicyMergeOp, PolicyMergeResult, PolicyMergeWarning, generated_rule_name, merge_policy, policy_covers_rule, diff --git a/crates/openshell-policy/src/merge.rs b/crates/openshell-policy/src/merge.rs index f191cd272..89c8bf8b3 100644 --- a/crates/openshell-policy/src/merge.rs +++ b/crates/openshell-policy/src/merge.rs @@ -7,6 +7,8 @@ use openshell_core::proto::{ L7Allow, L7DenyRule, L7Rule, NetworkBinary, NetworkEndpoint, NetworkPolicyRule, SandboxPolicy, }; +use crate::is_provider_rule_name; + #[derive(Debug, Clone, PartialEq)] pub enum PolicyMergeOp { AddRule { @@ -413,7 +415,7 @@ fn add_rule( let mut keys: Vec<_> = policy.network_policies.keys().cloned().collect(); keys.sort(); keys.into_iter() - .filter(|k| !k.starts_with("_provider_")) + .filter(|k| !is_provider_rule_name(k)) .find(|key| { policy .network_policies @@ -653,7 +655,7 @@ fn find_endpoint_mut<'a>( keys.sort(); let target_key = keys .into_iter() - .filter(|k| !k.starts_with("_provider_")) + .filter(|k| !is_provider_rule_name(k)) .find(|key| { policy.network_policies.get(key).is_some_and(|rule| { rule.endpoints diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 63ad1117c..47550a971 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -886,6 +886,23 @@ 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 and GPU runtimes. Used for the /// local-file code path where no proto is available. @@ -1050,6 +1067,89 @@ 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 proto_gpu_enrichment_promotes_proc_without_network_policy() { let mut policy = openshell_policy::restrictive_default_policy(); @@ -1298,6 +1398,7 @@ async fn load_policy( // 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); let sandbox = sandbox.as_deref().ok_or_else(|| { miette::miette!( "Cannot sync discovered policy: sandbox not available.\n\ @@ -1322,11 +1423,11 @@ async fn load_policy( // sandboxes. If the policy was enriched, sync the updated version // back to the gateway so users can see the effective policy. let enriched = enrich_proto_baseline_paths(&mut proto_policy); - if enriched + let sync_policy = proto_sync_payload_for_enriched_policy(&proto_policy, enriched); + if let Some(sync_policy) = sync_policy && let Some(sandbox_name) = sandbox.as_deref() && let Err(e) = - openshell_core::grpc_client::sync_policy(endpoint, sandbox_name, &proto_policy) - .await + openshell_core::grpc_client::sync_policy(endpoint, sandbox_name, &sync_policy).await { warn!( error = %e, diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index b4fd093ea..8b3b20bec 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -71,7 +71,8 @@ use tonic::{Request, Response, Status}; use tracing::{debug, info, warn}; use super::validation::{ - level_matches, source_matches, validate_policy_safety, validate_static_fields_unchanged, + level_matches, source_matches, validate_no_reserved_provider_policy_keys, + validate_policy_safety, validate_static_fields_unchanged, }; use super::{MAX_PAGE_SIZE, StoredSettingValue, StoredSettings, clamp_limit}; use crate::persistence::current_time_ms; @@ -1507,6 +1508,7 @@ async fn handle_update_config_inner( Status::invalid_argument("policy is required for global policy update") })?; openshell_policy::ensure_sandbox_process_identity(&mut new_policy); + validate_no_reserved_provider_policy_keys(&new_policy)?; validate_policy_safety(&new_policy)?; let payload = new_policy.encode_to_vec(); @@ -1814,6 +1816,16 @@ async fn handle_update_config_inner( .ok_or_else(|| Status::internal("sandbox has no spec"))?; openshell_policy::ensure_sandbox_process_identity(&mut new_policy); + if sandbox_caller { + if openshell_policy::strip_provider_rule_names(&mut new_policy) { + debug!( + sandbox_id = %sandbox_id, + "UpdateConfig: stripped provider-derived policy entries from sandbox sync" + ); + } + } else { + validate_no_reserved_provider_policy_keys(&new_policy)?; + } if let Some(baseline_policy) = spec.policy.as_ref() { validate_static_fields_unchanged(baseline_policy, &new_policy)?; @@ -2270,7 +2282,7 @@ pub(super) async fn handle_submit_policy_analysis( // own rule so the prover sees their contribution honestly. Reject at // the entry boundary — the agent never has reason to address a // provider rule by name. - if chunk.rule_name.starts_with("_provider_") { + if openshell_policy::is_provider_rule_name(&chunk.rule_name) { rejected += 1; rejection_reasons.push(format!( "chunk '{}' uses reserved '_provider_' rule-name prefix", @@ -3459,7 +3471,14 @@ fn parse_proto_add_allow_rules( fn validate_merge_operations_for_server(operations: &[PolicyMergeOp]) -> Result<(), Status> { for operation in operations { match operation { - PolicyMergeOp::AddRule { rule, .. } => validate_rule_not_always_blocked(rule)?, + PolicyMergeOp::AddRule { rule_name, rule } => { + if openshell_policy::is_provider_rule_name(rule_name) { + return Err(Status::invalid_argument(format!( + "merge operation add_rule rule_name '{rule_name}' uses reserved '_provider_' prefix for provider composition" + ))); + } + validate_rule_not_always_blocked(rule)?; + } PolicyMergeOp::AddAllowRules { host, .. } => validate_host_not_always_blocked(host)?, _ => {} } @@ -3577,16 +3596,12 @@ pub(super) async fn merge_chunk_into_policy( ) -> Result<(i64, String), Status> { let rule = NetworkPolicyRule::decode(chunk.proposed_rule.as_slice()) .map_err(|e| Status::internal(format!("decode proposed_rule failed: {e}")))?; - apply_merge_operations_with_retry( - store, - sandbox_id, - None, - &[PolicyMergeOp::AddRule { - rule_name: chunk.rule_name.clone(), - rule, - }], - ) - .await + let operations = [PolicyMergeOp::AddRule { + rule_name: chunk.rule_name.clone(), + rule, + }]; + validate_merge_operations_for_server(&operations)?; + apply_merge_operations_with_retry(store, sandbox_id, None, &operations).await } async fn remove_chunk_from_policy( @@ -4019,6 +4034,19 @@ mod tests { assert!(!is_sandbox_caller(&req)); } + #[test] + fn merge_operation_validation_rejects_reserved_provider_add_rule_name() { + let err = validate_merge_operations_for_server(&[PolicyMergeOp::AddRule { + rule_name: "_provider_work_github".to_string(), + rule: NetworkPolicyRule::default(), + }]) + .unwrap_err(); + + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("_provider_work_github")); + assert!(err.message().contains("reserved '_provider_' prefix")); + } + // ---- Sandbox IDOR guard (issue #1354) ---- #[tokio::test] @@ -4938,7 +4966,7 @@ mod tests { } #[tokio::test] - async fn sandbox_config_preserves_overlapping_user_and_provider_rules() { + async fn sandbox_config_composes_user_and_provider_rules() { let state = test_server_state().await; enable_providers_v2(&state).await; state @@ -4951,7 +4979,7 @@ mod tests { .put_message(&test_sandbox( "sb-overlap", "overlap", - test_policy_with_rule("_provider_work_github", "api.github.com"), + test_policy_with_rule("custom_github", "api.github.com"), vec!["work-github".to_string()], )) .await @@ -4962,17 +4990,17 @@ mod tests { assert!( effective_policy .network_policies - .contains_key("_provider_work_github") + .contains_key("custom_github") ); assert!( effective_policy .network_policies - .contains_key("_provider_work_github_2") + .contains_key("_provider_work_github") ); assert_eq!( effective_policy .network_policies - .get("_provider_work_github") + .get("custom_github") .unwrap() .endpoints[0] .host, @@ -7030,6 +7058,94 @@ mod tests { ); } + #[tokio::test] + async fn approve_draft_chunk_rejects_stored_reserved_provider_rule_name() { + use openshell_core::proto::{NetworkBinary, NetworkEndpoint, NetworkPolicyRule}; + + let state = test_server_state().await; + let sandbox_id = "sb-approve-provider-prefix"; + let sandbox_name = "approve-provider-prefix"; + state + .store + .put_message(&test_sandbox( + sandbox_id, + sandbox_name, + ProtoSandboxPolicy::default(), + vec![], + )) + .await + .unwrap(); + + let proposed_rule = NetworkPolicyRule { + name: "_provider_work_github".to_string(), + endpoints: vec![NetworkEndpoint { + host: "api.github.com".to_string(), + port: 443, + ..Default::default() + }], + binaries: vec![NetworkBinary { + path: "/usr/bin/curl".to_string(), + ..Default::default() + }], + }; + let chunk = DraftChunkRecord { + id: "chunk-provider-prefix".to_string(), + sandbox_id: sandbox_id.to_string(), + draft_version: 1, + status: "pending".to_string(), + rule_name: "_provider_work_github".to_string(), + proposed_rule: proposed_rule.encode_to_vec(), + rationale: "stored legacy/proposal chunk should not approve".to_string(), + security_notes: String::new(), + confidence: 1.0, + created_at_ms: 0, + decided_at_ms: None, + host: "api.github.com".to_string(), + port: 443, + binary: "/usr/bin/curl".to_string(), + hit_count: 1, + first_seen_ms: 0, + last_seen_ms: 0, + validation_result: String::new(), + rejection_reason: String::new(), + }; + state + .store + .put_draft_chunk(&chunk, None) + .await + .expect("draft chunk should persist"); + + let err = handle_approve_draft_chunk( + &state, + with_user(Request::new(ApproveDraftChunkRequest { + name: sandbox_name.to_string(), + chunk_id: chunk.id.clone(), + })), + ) + .await + .expect_err("reserved provider rule names must be rejected at approval"); + + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("_provider_work_github")); + assert!(err.message().contains("reserved '_provider_' prefix")); + let stored_chunk = state + .store + .get_draft_chunk(&chunk.id) + .await + .unwrap() + .expect("chunk should still exist"); + assert_eq!(stored_chunk.status, "pending"); + assert!( + state + .store + .get_latest_policy(sandbox_id) + .await + .unwrap() + .is_none(), + "failed approval must not persist a policy revision" + ); + } + /// v1 calibration row: **L4 with a credential in scope → HIGH finding.** /// The sandbox has a github provider attached, so a credential is in /// scope for api.github.com. A broad L4 proposal therefore lands in @@ -8986,6 +9102,29 @@ mod tests { ); } + #[tokio::test] + async fn update_config_global_policy_rejects_reserved_provider_key() { + let state = test_server_state().await; + + let err = handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + global: true, + policy: Some(test_policy_with_rule( + "_provider_work_github", + "api.github.com", + )), + ..Default::default() + })), + ) + .await + .unwrap_err(); + + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("_provider_work_github")); + assert!(err.message().contains("reserved '_provider_' prefix")); + } + #[test] fn merge_effective_settings_global_overrides_sandbox_key() { let global = StoredSettings { @@ -9665,6 +9804,140 @@ mod tests { ); } + #[tokio::test] + async fn update_config_user_policy_rejects_reserved_provider_key() { + let state = test_server_state().await; + state + .store + .put_message(&test_sandbox( + "sb-user-reserved-key", + "user-reserved-key", + test_policy_with_rule("sandbox_only", "sandbox.example.com"), + Vec::new(), + )) + .await + .unwrap(); + + let err = handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "user-reserved-key".to_string(), + policy: Some(test_policy_with_rule( + "_provider_work_github", + "api.github.com", + )), + ..Default::default() + })), + ) + .await + .unwrap_err(); + + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("_provider_work_github")); + assert!(err.message().contains("reserved '_provider_' prefix")); + } + + #[tokio::test] + async fn update_config_sandbox_sync_strips_reserved_provider_keys_before_persisting() { + use openshell_core::proto::{SandboxPhase, SandboxSpec}; + + let state = test_server_state().await; + let mut sandbox = Sandbox { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: "sb-sync-strip".to_string(), + name: "sync-strip".to_string(), + created_at_ms: 1_000_000, + labels: HashMap::new(), + resource_version: 0, + }), + spec: Some(SandboxSpec { + policy: None, + providers: Vec::new(), + ..Default::default() + }), + ..Default::default() + }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); + state.store.put_message(&sandbox).await.unwrap(); + + let current = state + .store + .get_message_by_name::("sync-strip") + .await + .unwrap() + .unwrap(); + let current_version = current.metadata.as_ref().unwrap().resource_version; + + let mut synced_policy = test_policy_with_rule("sandbox_only", "sandbox.example.com"); + synced_policy.network_policies.insert( + "_provider_work_github".to_string(), + NetworkPolicyRule { + name: "_provider_work_github".to_string(), + endpoints: vec![NetworkEndpoint { + host: "api.github.com".to_string(), + port: 443, + ..Default::default() + }], + ..Default::default() + }, + ); + + let response = handle_update_config( + &state, + with_sandbox( + Request::new(UpdateConfigRequest { + name: "sync-strip".to_string(), + policy: Some(synced_policy), + expected_resource_version: current_version, + ..Default::default() + }), + "sb-sync-strip", + ), + ) + .await + .unwrap() + .into_inner(); + + assert_eq!(response.version, 1); + + let updated_sandbox = state + .store + .get_message_by_name::("sync-strip") + .await + .unwrap() + .unwrap(); + let spec_policy = updated_sandbox + .spec + .as_ref() + .and_then(|spec| spec.policy.as_ref()) + .expect("spec.policy should be backfilled"); + assert!(spec_policy.network_policies.contains_key("sandbox_only")); + assert!( + !spec_policy + .network_policies + .contains_key("_provider_work_github") + ); + + let persisted = state + .store + .get_latest_policy("sb-sync-strip") + .await + .unwrap() + .expect("policy revision should be persisted"); + let persisted_policy = ProtoSandboxPolicy::decode(persisted.policy_payload.as_slice()) + .expect("persisted policy should decode"); + assert!( + persisted_policy + .network_policies + .contains_key("sandbox_only") + ); + assert!( + !persisted_policy + .network_policies + .contains_key("_provider_work_github") + ); + } + #[tokio::test] async fn update_config_policy_backfill_cas_rejects_stale_version() { use openshell_core::proto::{SandboxPhase, SandboxSpec}; diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 28377394f..1570b8e03 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -46,8 +46,8 @@ use super::provider::{ get_provider_record, is_valid_env_key, validate_provider_environment_keys_unique, }; use super::validation::{ - level_matches, source_matches, validate_exec_request_fields, validate_policy_safety, - validate_sandbox_spec, + level_matches, source_matches, validate_exec_request_fields, + validate_no_reserved_provider_policy_keys, validate_policy_safety, validate_sandbox_spec, }; use super::{MAX_PAGE_SIZE, MAX_PROVIDERS, clamp_limit}; use crate::persistence::current_time_ms; @@ -162,6 +162,7 @@ async fn handle_create_sandbox_inner( // empty, then validate policy safety before persisting. if let Some(ref mut policy) = spec.policy { openshell_policy::ensure_sandbox_process_identity(policy); + validate_no_reserved_provider_policy_keys(policy)?; validate_policy_safety(policy)?; } @@ -2596,6 +2597,37 @@ mod tests { assert!(err.message().contains("provider-b")); } + #[tokio::test] + async fn create_sandbox_rejects_reserved_provider_policy_key() { + let state = test_server_state().await; + let mut policy = openshell_core::proto::SandboxPolicy::default(); + policy.network_policies.insert( + "_provider_work_github".to_string(), + openshell_core::proto::NetworkPolicyRule { + name: "_provider_work_github".to_string(), + ..Default::default() + }, + ); + + let err = handle_create_sandbox( + &state, + Request::new(CreateSandboxRequest { + name: "reserved-policy-key".to_string(), + spec: Some(openshell_core::proto::SandboxSpec { + policy: Some(policy), + ..Default::default() + }), + labels: HashMap::new(), + }), + ) + .await + .unwrap_err(); + + assert_eq!(err.code(), tonic::Code::InvalidArgument); + assert!(err.message().contains("_provider_work_github")); + assert!(err.message().contains("reserved '_provider_' prefix")); + } + #[tokio::test] async fn create_sandbox_with_providers_waits_for_sandbox_sync_guard() { let state = test_server_state().await; diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index b3680c6e7..09e9f1cad 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -638,6 +638,22 @@ pub(super) fn validate_policy_safety(policy: &ProtoSandboxPolicy) -> Result<(), Ok(()) } +/// Validate that user-authored policy does not use provider-derived rule keys. +pub(super) fn validate_no_reserved_provider_policy_keys( + policy: &ProtoSandboxPolicy, +) -> Result<(), Status> { + if let Some(key) = policy + .network_policies + .keys() + .find(|key| openshell_policy::is_provider_rule_name(key)) + { + return Err(Status::invalid_argument(format!( + "network_policies key '{key}' uses reserved '_provider_' prefix for provider composition; use 'openshell policy get --base' for a round-trippable base policy, or use 'openshell policy get --full' to inspect the effective policy including provider entries" + ))); + } + Ok(()) +} + /// Validate that static policy fields (filesystem, landlock, process) haven't changed /// from the baseline (version 1) policy. pub(super) fn validate_static_fields_unchanged( @@ -1605,6 +1621,43 @@ mod tests { assert!(err.message().contains("TLD wildcard")); } + #[test] + fn validate_no_reserved_provider_policy_keys_rejects_reserved_key() { + use openshell_core::proto::NetworkPolicyRule; + + let mut policy = openshell_policy::restrictive_default_policy(); + policy.network_policies.insert( + "_provider_work_github".into(), + NetworkPolicyRule { + name: "_provider_work_github".into(), + ..Default::default() + }, + ); + + let err = validate_no_reserved_provider_policy_keys(&policy).unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("_provider_work_github")); + assert!(err.message().contains("reserved '_provider_' prefix")); + assert!(err.message().contains("policy get --base")); + assert!(err.message().contains("round-trippable base policy")); + } + + #[test] + fn validate_no_reserved_provider_policy_keys_accepts_user_key() { + use openshell_core::proto::NetworkPolicyRule; + + let mut policy = openshell_policy::restrictive_default_policy(); + policy.network_policies.insert( + "provider_work_github".into(), + NetworkPolicyRule { + name: "provider_work_github".into(), + ..Default::default() + }, + ); + + assert!(validate_no_reserved_provider_policy_keys(&policy).is_ok()); + } + // ---- Static field validation ---- #[test] diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index 406ed12b8..434ff7c6e 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -140,13 +140,13 @@ The following steps outline the hot-reload policy update workflow. `--add-allow` and `--add-deny` target existing `protocol: rest` or `protocol: websocket` endpoints. If you pass multiple update flags in one command, OpenShell applies them as one atomic merge batch and persists at most one new revision. -4. For larger edits, pull the current effective policy and edit the YAML directly. Before reusing the file, strip the metadata header above the `---` line. If the sandbox has attached Providers v2 providers, remove generated `_provider_*` entries before reapplying the policy; those entries are derived from provider profiles. +4. For larger edits, pull the current base policy and edit the YAML directly. The base policy is the user-authored policy without provider-composed `_provider_*` entries, so it is safe to round-trip through `openshell policy set`. Before reusing the file, strip the metadata header above the `---` line. ```shell - openshell policy get --full > current-policy.yaml + openshell policy get --base > current-policy.yaml ``` - To inspect a stored sandbox-authored revision instead of the current effective policy, pass `--rev `. + To inspect the effective policy that the sandbox enforces, including provider-composed entries, use `openshell policy get --full`. To inspect a stored sandbox-authored revision instead of the current effective policy, pass `--rev `. 5. Edit the YAML: add or adjust `network_policies` entries, binaries, `access`, or `rules`. diff --git a/docs/sandboxes/providers-v2.mdx b/docs/sandboxes/providers-v2.mdx index 896ac641e..49e7120fd 100644 --- a/docs/sandboxes/providers-v2.mdx +++ b/docs/sandboxes/providers-v2.mdx @@ -547,7 +547,7 @@ openshell sandbox create \ -- claude ``` -When `providers_v2_enabled=true`, each attached provider with a matching profile contributes a provider policy layer to the sandbox effective policy. When the setting is disabled, the sandbox receives provider credentials but not provider-derived policy entries. +When `providers_v2_enabled=true`, each attached provider with a matching profile contributes a provider policy layer to the sandbox effective policy. The base policy is the user-authored sandbox policy that you can edit and apply. The effective policy is the composed policy that the sandbox enforces: base policy plus provider policy layers. When the setting is disabled, the sandbox receives provider credentials but not provider-derived policy entries. Updating a custom provider profile affects every provider instance whose `type` matches that profile ID. Provider instances are not rewritten, and sandbox-authored policies are not modified. Running sandboxes observe the updated provider-derived policy on their next config sync. If a gateway-global policy is active, provider-derived policy layers remain suppressed. @@ -565,7 +565,7 @@ The list output includes provider name, provider type, credential key count, and ## Policy Composition -OpenShell stores sandbox-authored policy and provider attachments separately. When a sandbox asks for its effective policy, the gateway composes the current sandbox policy with provider policy layers just in time. +OpenShell stores the base policy and provider attachments separately. When a sandbox asks for its effective policy, the gateway composes the current base policy with provider policy layers just in time. For example, the built-in GitHub profile contains these endpoints and binaries: @@ -643,14 +643,20 @@ network_policies: Inspect the effective policy: ```shell -openshell policy get provider-demo +openshell policy get provider-demo --full +``` + +Pull the round-trippable base policy without provider entries: + +```shell +openshell policy get provider-demo --base ``` Composition follows these rules: - Provider policy entries use reserved `_provider_*` keys derived from provider instance names. -- Provider policy entries are derived data. OpenShell does not persist them back into the sandbox-authored policy. -- If a user-authored rule already uses the same key, OpenShell keeps the user rule and adds a numeric suffix to the provider rule. +- Provider policy entries are derived data. OpenShell does not persist them back into the base policy. +- User-authored policies cannot define `_provider_*` network policy keys. The gateway rejects those keys on sandbox create and full policy replacement, and sandbox-originated policy sync strips them before persistence. - Provider and user rules are concatenated. Overlapping endpoints remain separate rules. - A gateway global policy override suppresses provider-derived policy layers.