From f21d61dabc0d0e5b22256b1e1d9cf51d3613b55f Mon Sep 17 00:00:00 2001 From: Adrien Langou Date: Wed, 22 Jul 2026 15:42:14 +0200 Subject: [PATCH] fix(policy): keep internal allowed IP proposals pending Signed-off-by: Adrien Langou --- architecture/security-policy.md | 54 +- crates/openshell-core/src/net.rs | 159 +++- crates/openshell-server/src/grpc/policy.rs | 899 +++++++++++++++++++-- docs/sandboxes/policy-advisor.mdx | 28 +- 4 files changed, 1014 insertions(+), 126 deletions(-) diff --git a/architecture/security-policy.md b/architecture/security-policy.md index c91ba445eb..b4f0bdb912 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -122,17 +122,25 @@ through the proposal loop instead of treating the denial as terminal. policy plus the sandbox's attached-provider credential set, then computes the delta of findings between the current baseline and the merged policy. 3. **Auto-approval gate (proposer-agnostic, opt-in).** Auto-approval fires - when *both* (a) the prover delta is empty (`prover: no new findings`) AND - (b) the `proposal_approval_mode` setting resolves to `"auto"` — gateway - scope wins, sandbox scope is the per-sandbox override, default is - `"manual"`. When both hold, the gateway internally invokes the approve - path with actor identity `system:auto`. The audit event uses - `CONFIG:APPROVED` and carries `auto=true`, `source=`, - `prover_delta=empty`, and `resolved_from=` as unmapped - fields, with message text `"auto-approved: no new prover findings"` — - never `safe`. The opt-in gate preserves OpenShell's default-deny - posture: with no setting at either scope, every proposal lands in - `pending` for human review, even when the prover sees no findings. + only when *all three* conditions hold: (a) `proposal_approval_mode` + resolves to `"auto"` — gateway scope wins, sandbox scope is the + per-sandbox override, default is `"manual"`; (b) the prover delta is empty + (`prover: no new findings`); and (c) the security notes recomputed from + the chunk's current proposed rule are empty (see + [Security-notes gate](#security-notes-gate)). Before merging, the gateway + reloads the stored chunk and reruns both checks on its current rule. This is + important after edits and mechanistic deduplication: the stored rule, not a + duplicate incoming payload or stale persisted analysis, controls the + decision. The recalculated prover verdict is decision-local rather than + persisted, so `validation_result` reads can still show the submit-time + verdict after an edit or deduplication. Decode, prover, or merge failures + leave the chunk pending. The audit event uses `CONFIG:APPROVED` and carries + `auto=true`, `source=`, `prover_delta=empty`, and + `resolved_from=` as unmapped fields, with message text + `"auto-approved: no new prover findings"` — never `safe`. The opt-in gate + preserves OpenShell's default-deny posture: with no setting at either + scope, every proposal lands in `pending` for human review, even + when the prover sees no findings. 4. **Implicit supersede.** On any successful submission, the gateway scans the sandbox's pending chunks for matches on `(host, port, binary)` and auto-rejects the older ones with reason `"superseded by chunk X"`. This @@ -152,6 +160,30 @@ through the proposal loop instead of treating the denial as terminal. policy. 6. **Escalation.** Anything else lands in `pending` for human review. +### Security-notes gate + +Separately from the prover, each chunk carries advisory `security_notes`. +Reads, bulk approval, and auto-approval regenerate them from the current +stored proposed rule instead of trusting a persisted value that may be stale +after an edit. Non-empty notes block auto-approval and make +`ApproveAllDraftChunks` skip the chunk unless `include_security_flagged` is +set. The chunk stays `pending`; an explicit human approval can still merge a +flagged chunk. + +Private/internal destinations are advisory, not blocking. A literal endpoint +IP, `allowed_ips` entry, or CIDR intersection in RFC 1918, CGNAT +`100.64.0.0/10`, IPv6 ULA `fc00::/7`, or another special-use range covered by +`openshell-core` `net::is_internal_net` produces a note. A hostless rule +carrying `allowed_ips` earns an extra note because it can match any hostname +resolving into the range. + +Always-blocked destinations are separate from this advisory classification. +Loopback, link-local, and unspecified IPs/CIDRs, plus `localhost` and known +metadata endpoint hostnames, are excluded from security notes. Submit and edit +may store such a draft, but existing merge validation rejects it when an +approval attempts to add it to policy; runtime SSRF protections remain the +final enforcement boundary. + ## What the prover decides The prover answers four formal questions about each proposed policy diff --git a/crates/openshell-core/src/net.rs b/crates/openshell-core/src/net.rs index 59bfdfc5d5..3f14a397b8 100644 --- a/crates/openshell-core/src/net.rs +++ b/crates/openshell-core/src/net.rs @@ -10,6 +10,7 @@ //! - The mechanistic mapper for proposal filtering //! - The gateway server for defense-in-depth validation on approval +use ipnet::{IpNet, Ipv4Net, Ipv6Net}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; /// Check if a hostname is a known cloud metadata hostname that resolves to an @@ -80,9 +81,9 @@ pub fn is_always_blocked_ip(ip: IpAddr) -> bool { /// /// Used at policy load time and server-side approval to reject entries that /// would be silently blocked at runtime by [`is_always_blocked_ip`]. -pub fn is_always_blocked_net(net: ipnet::IpNet) -> bool { +pub fn is_always_blocked_net(net: IpNet) -> bool { match net { - ipnet::IpNet::V4(v4net) => { + IpNet::V4(v4net) => { let network = v4net.network(); let broadcast = v4net.broadcast(); @@ -107,7 +108,7 @@ pub fn is_always_blocked_net(net: ipnet::IpNet) -> bool { false } - ipnet::IpNet::V6(v6net) => { + IpNet::V6(v6net) => { // For IPv6, check the network address itself and representative // addresses within the range. let network = v6net.network(); @@ -155,6 +156,43 @@ pub fn is_always_blocked_net(net: ipnet::IpNet) -> bool { } } +const RFC1918_10_NET: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(10, 0, 0, 0), 8); +const RFC1918_172_NET: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(172, 16, 0, 0), 12); +const RFC1918_192_NET: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(192, 168, 0, 0), 16); +const CGNAT_NET: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(100, 64, 0, 0), 10); +const IETF_PROTOCOL_ASSIGNMENTS_NET: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(192, 0, 0, 0), 24); +const TEST_NET_1: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(192, 0, 2, 0), 24); +const BENCHMARKING_NET: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(198, 18, 0, 0), 15); +const TEST_NET_2: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(198, 51, 100, 0), 24); +const TEST_NET_3: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::new(203, 0, 113, 0), 24); +const LIMITED_BROADCAST_NET: Ipv4Net = Ipv4Net::new_assert(Ipv4Addr::BROADCAST, 32); +const IPV6_ULA_NET: Ipv6Net = Ipv6Net::new_assert(Ipv6Addr::new(0xfc00, 0, 0, 0, 0, 0, 0, 0), 7); + +const NON_HARD_INTERNAL_V4_NETS: [Ipv4Net; 10] = [ + RFC1918_10_NET, + RFC1918_172_NET, + RFC1918_192_NET, + CGNAT_NET, + IETF_PROTOCOL_ASSIGNMENTS_NET, + TEST_NET_1, + BENCHMARKING_NET, + TEST_NET_2, + TEST_NET_3, + LIMITED_BROADCAST_NET, +]; + +fn ipv4_nets_intersect(left: Ipv4Net, right: Ipv4Net) -> bool { + left.network() <= right.broadcast() && right.network() <= left.broadcast() +} + +fn ipv6_nets_intersect(left: Ipv6Net, right: Ipv6Net) -> bool { + left.network() <= right.broadcast() && right.network() <= left.broadcast() +} + +fn ipv4_net_to_mapped_ipv6(net: Ipv4Net) -> Ipv6Net { + Ipv6Net::new_assert(net.network().to_ipv6_mapped(), 96_u8 + net.prefix_len()) +} + /// Check if an IP address is internal (loopback, private RFC 1918, link-local, /// or unspecified). /// @@ -188,6 +226,26 @@ pub fn is_internal_ip(ip: IpAddr) -> bool { } } +/// Check if a CIDR network intersects any address range classified by +/// [`is_internal_ip`]. +pub fn is_internal_net(net: IpNet) -> bool { + if is_always_blocked_net(net) { + return true; + } + + match net { + IpNet::V4(net) => NON_HARD_INTERNAL_V4_NETS + .iter() + .any(|internal| ipv4_nets_intersect(net, *internal)), + IpNet::V6(net) => { + ipv6_nets_intersect(net, IPV6_ULA_NET) + || NON_HARD_INTERNAL_V4_NETS + .iter() + .any(|internal| ipv6_nets_intersect(net, ipv4_net_to_mapped_ipv6(*internal))) + } + } +} + /// IPv4 internal address check covering RFC 1918, CGNAT (RFC 6598), and other /// special-use ranges that should never be reachable from sandbox egress. fn is_internal_v4(v4: Ipv4Addr) -> bool { @@ -382,45 +440,45 @@ mod tests { #[test] fn test_always_blocked_net_loopback_v4() { - let net: ipnet::IpNet = "127.0.0.0/8".parse().unwrap(); + let net: IpNet = "127.0.0.0/8".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_link_local_v4() { - let net: ipnet::IpNet = "169.254.0.0/16".parse().unwrap(); + let net: IpNet = "169.254.0.0/16".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_unspecified_v4() { - let net: ipnet::IpNet = "0.0.0.0/32".parse().unwrap(); + let net: IpNet = "0.0.0.0/32".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_loopback_v6() { - let net: ipnet::IpNet = "::1/128".parse().unwrap(); + let net: IpNet = "::1/128".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_link_local_v6() { - let net: ipnet::IpNet = "fe80::/10".parse().unwrap(); + let net: IpNet = "fe80::/10".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_ipv4_mapped_v6_loopback() { - let net: ipnet::IpNet = "::ffff:127.0.0.1/128".parse().unwrap(); + let net: IpNet = "::ffff:127.0.0.1/128".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_allows_rfc1918() { - let net10: ipnet::IpNet = "10.0.0.0/8".parse().unwrap(); - let net172: ipnet::IpNet = "172.16.0.0/12".parse().unwrap(); - let net192: ipnet::IpNet = "192.168.0.0/16".parse().unwrap(); + let net10: IpNet = "10.0.0.0/8".parse().unwrap(); + let net172: IpNet = "172.16.0.0/12".parse().unwrap(); + let net192: IpNet = "192.168.0.0/16".parse().unwrap(); assert!(!is_always_blocked_net(net10)); assert!(!is_always_blocked_net(net172)); assert!(!is_always_blocked_net(net192)); @@ -428,44 +486,44 @@ mod tests { #[test] fn test_always_blocked_net_allows_public() { - let net: ipnet::IpNet = "8.8.8.0/24".parse().unwrap(); + let net: IpNet = "8.8.8.0/24".parse().unwrap(); assert!(!is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_single_ip_loopback() { - let net: ipnet::IpNet = "127.0.0.1/32".parse().unwrap(); + let net: IpNet = "127.0.0.1/32".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_single_ip_metadata() { - let net: ipnet::IpNet = "169.254.169.254/32".parse().unwrap(); + let net: IpNet = "169.254.169.254/32".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_broad_cidr_containing_blocked() { // 0.0.0.0/0 contains everything including unspecified, loopback, link-local - let net: ipnet::IpNet = "0.0.0.0/0".parse().unwrap(); + let net: IpNet = "0.0.0.0/0".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_v6_broad_containing_loopback() { - let net: ipnet::IpNet = "::/0".parse().unwrap(); + let net: IpNet = "::/0".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_v6_ipv4_mapped_loopback_single() { - let net: ipnet::IpNet = "::ffff:127.0.0.1/128".parse().unwrap(); + let net: IpNet = "::ffff:127.0.0.1/128".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_v6_ipv4_mapped_link_local_single() { - let net: ipnet::IpNet = "::ffff:169.254.0.1/128".parse().unwrap(); + let net: IpNet = "::ffff:169.254.0.1/128".parse().unwrap(); assert!(is_always_blocked_net(net)); } @@ -474,7 +532,7 @@ mod tests { // ::ffff:168.0.0.0/103 has a public network address (168.0.0.0) but // the range covers 168.0.0.0–169.255.255.255, which includes the // link-local block 169.254.0.0/16. - let net: ipnet::IpNet = "::ffff:168.0.0.0/103".parse().unwrap(); + let net: IpNet = "::ffff:168.0.0.0/103".parse().unwrap(); assert!(is_always_blocked_net(net)); } @@ -482,17 +540,74 @@ mod tests { fn test_always_blocked_net_v6_ipv4_mapped_broad_spans_loopback() { // ::ffff:64.0.0.0/98 has a public network address (64.0.0.0) but the // range covers 64.0.0.0–127.255.255.255, which includes loopback. - let net: ipnet::IpNet = "::ffff:64.0.0.0/98".parse().unwrap(); + let net: IpNet = "::ffff:64.0.0.0/98".parse().unwrap(); assert!(is_always_blocked_net(net)); } #[test] fn test_always_blocked_net_v6_ipv4_mapped_allows_public() { // ::ffff:8.8.8.8/128 is a public address — should not be blocked. - let net: ipnet::IpNet = "::ffff:8.8.8.8/128".parse().unwrap(); + let net: IpNet = "::ffff:8.8.8.8/128".parse().unwrap(); assert!(!is_always_blocked_net(net)); } + // -- is_internal_net -- + + #[test] + fn test_internal_net_rfc1918_contained_and_supernet() { + for cidr in ["10.1.0.0/16", "8.0.0.0/5"] { + assert!(is_internal_net(cidr.parse().unwrap()), "{cidr}"); + } + } + + #[test] + fn test_internal_net_cgnat_contained_and_supernet() { + for cidr in ["100.64.0.0/10", "100.0.0.0/9"] { + assert!(is_internal_net(cidr.parse().unwrap()), "{cidr}"); + } + } + + #[test] + fn test_internal_net_ipv4_special_use_ranges() { + for cidr in [ + "192.0.0.0/24", + "192.0.2.0/24", + "198.18.0.0/15", + "198.51.100.0/24", + "203.0.113.0/24", + "255.255.255.255/32", + ] { + assert!(is_internal_net(cidr.parse().unwrap()), "{cidr}"); + } + } + + #[test] + fn test_internal_net_ipv6_ula() { + for cidr in ["fc00::/7", "fd00::/8"] { + assert!(is_internal_net(cidr.parse().unwrap()), "{cidr}"); + } + } + + #[test] + fn test_internal_net_ipv4_mapped_cgnat_supernet() { + let net: IpNet = "::ffff:100.0.0.0/105".parse().unwrap(); + assert!(is_internal_net(net)); + } + + #[test] + fn test_internal_net_always_blocked() { + for cidr in ["127.0.0.0/8", "fe80::/10"] { + assert!(is_internal_net(cidr.parse().unwrap()), "{cidr}"); + } + } + + #[test] + fn test_internal_net_allows_large_public_ranges() { + for cidr in ["8.0.0.0/8", "::ffff:8.0.0.0/104"] { + assert!(!is_internal_net(cidr.parse().unwrap()), "{cidr}"); + } + } + // -- is_internal_ip -- #[test] diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 12f563d17b..0703771846 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -19,7 +19,7 @@ use crate::policy_store::{AtomicPolicyRevisionWrite, PolicyStoreExt}; use crate::provider_profile_sources::EffectiveProviderProfileCatalog; #[cfg(test)] use crate::provider_profile_sources::ProviderProfileSources; -use openshell_core::net::is_internal_ip; +use openshell_core::net::{is_always_blocked_ip, is_internal_ip}; use openshell_core::proto::policy_merge_operation; use openshell_core::proto::setting_value; use openshell_core::proto::{ @@ -906,15 +906,23 @@ async fn resolve_proposal_approval_mode( Ok((false, "default")) } +struct AutoApproveChunkContext<'a> { + sandbox: &'a Sandbox, + workspace: &'a str, + source: &'a str, + resolved_from: &'a str, + current_policy: &'a ProtoSandboxPolicy, + credential_set: &'a CredentialSet, +} + async fn auto_approve_chunk( state: &Arc, - sandbox_id: &str, - workspace: &str, - sandbox_name: &str, chunk_id: &str, - source: &str, - resolved_from: &str, + context: AutoApproveChunkContext<'_>, ) -> Result<(), Status> { + let sandbox_id = context.sandbox.object_id(); + let sandbox_name = context.sandbox.object_name(); + // Same gate the human-driven approve paths apply: if a global policy is // active, sandbox-scoped chunk approvals are meaningless because // `GetSandboxConfig` prefers the global policy. Auto-approving here @@ -937,8 +945,47 @@ async fn auto_approve_chunk( return Ok(()); } + // Mechanistic dedup may return an existing row whose proposed rule was + // edited after its original validation. Re-run the prover against that + // stored rule instead of trusting the incoming proposal's verdict. + let rule = decode_draft_chunk_rule(&chunk)? + .ok_or_else(|| Status::failed_precondition("draft chunk has no proposed rule"))?; + let validation_result = validation_result_for_agent_proposal( + context.current_policy.clone(), + &chunk.rule_name, + &rule, + context.credential_set, + ); + if validation_result != "prover: no new findings" { + info!( + sandbox_id = %sandbox_id, + chunk_id = %chunk_id, + rule_name = %chunk.rule_name, + validation_result = %validation_result, + source = %context.source, + resolved_from = %context.resolved_from, + "Auto-approval skipped: current stored rule has prover findings" + ); + return Ok(()); + } + + let security_notes = current_draft_chunk_security_notes(&chunk)?; + if !security_notes.is_empty() { + info!( + sandbox_id = %sandbox_id, + chunk_id = %chunk_id, + rule_name = %chunk.rule_name, + security_notes = %security_notes, + source = %context.source, + resolved_from = %context.resolved_from, + "Auto-approval skipped: current rule is security-flagged" + ); + return Ok(()); + } + let (version, hash) = - merge_chunk_into_policy(state.store.as_ref(), sandbox_id, workspace, &chunk).await?; + merge_chunk_into_policy(state.store.as_ref(), sandbox_id, context.workspace, &chunk) + .await?; let chunk_summary = summarize_draft_chunk_rule(&chunk)?; let now_ms = current_time_ms(); @@ -950,10 +997,10 @@ async fn auto_approve_chunk( state.sandbox_watch_bus.notify(sandbox_id); - let source_label = if source.is_empty() { + let source_label = if context.source.is_empty() { "unspecified" } else { - source + context.source }; emit_gateway_policy_auto_approve_audit_log( sandbox_id, @@ -964,7 +1011,7 @@ async fn auto_approve_chunk( version, &hash, source_label, - resolved_from, + context.resolved_from, ); info!( @@ -974,7 +1021,7 @@ async fn auto_approve_chunk( version = version, policy_hash = %hash, source = %source_label, - resolved_from = %resolved_from, + resolved_from = %context.resolved_from, "Auto-approved chunk: no new prover findings" ); @@ -2669,13 +2716,15 @@ pub(super) async fn handle_submit_policy_analysis( .map(Message::encode_to_vec) .unwrap_or_default(); - let rule_ref = chunk.proposed_rule.as_ref(); + let rule_ref = chunk.proposed_rule.as_ref().expect("checked above"); let (ep_host, ep_port) = rule_ref - .and_then(|r| r.endpoints.first()) + .endpoints + .first() .map(|ep| (ep.host.to_lowercase(), ep.port as i32)) .unwrap_or_default(); let ep_binary = rule_ref - .and_then(|r| r.binaries.first()) + .binaries + .first() .map(|b| b.path.clone()) .unwrap_or_default(); @@ -2686,7 +2735,7 @@ pub(super) async fn handle_submit_policy_analysis( let validation_result = validation_result_for_agent_proposal( current_policy.clone(), &chunk.rule_name, - chunk.proposed_rule.as_ref().expect("checked above"), + rule_ref, &credential_set, ); @@ -2701,10 +2750,7 @@ pub(super) async fn handle_submit_policy_analysis( rule_name: chunk.rule_name.clone(), proposed_rule: proposed_rule_bytes, rationale: chunk.rationale.clone(), - security_notes: generate_security_notes( - &ep_host, - u16::try_from(ep_port as u32).unwrap_or(0), - ), + security_notes: generate_security_notes(rule_ref), confidence: f64::from(chunk.confidence.clamp(0.0, 1.0)), created_at_ms: now_ms, decided_at_ms: None, @@ -2784,15 +2830,17 @@ pub(super) async fn handle_submit_policy_analysis( // string means findings or infrastructure error, both of which // require human attention. if auto_approve_enabled - && validation_result == "prover: no new findings" && let Err(err) = auto_approve_chunk( state, - &sandbox_id, - &workspace, - sandbox.object_name(), &effective_id, - &req.analysis_mode, - resolved_from, + AutoApproveChunkContext { + sandbox: &sandbox, + workspace: &workspace, + source: &req.analysis_mode, + resolved_from, + current_policy: ¤t_policy, + credential_set: &credential_set, + }, ) .await { @@ -3152,12 +3200,13 @@ async fn handle_approve_all_draft_chunks_inner( let mut last_hash = String::new(); for chunk in &pending_chunks { - if !req.include_security_flagged && !chunk.security_notes.is_empty() { + let security_notes = current_draft_chunk_security_notes(chunk)?; + if !req.include_security_flagged && !security_notes.is_empty() { info!( sandbox_id = %sandbox_id, chunk_id = %chunk.id, rule_name = %chunk.rule_name, - security_notes = %chunk.security_notes, + security_notes = %security_notes, "ApproveAllDraftChunks: skipping security-flagged chunk" ); chunks_skipped += 1; @@ -3577,17 +3626,27 @@ fn compute_config_revision( u64::from_le_bytes(bytes) } -fn draft_chunk_record_to_proto(record: &DraftChunkRecord) -> Result { - use openshell_core::proto::NetworkPolicyRule; - - let proposed_rule = if record.proposed_rule.is_empty() { - None +fn decode_draft_chunk_rule(record: &DraftChunkRecord) -> Result, Status> { + if record.proposed_rule.is_empty() { + Ok(None) } else { - Some( - NetworkPolicyRule::decode(record.proposed_rule.as_slice()) - .map_err(|e| Status::internal(format!("decode proposed_rule failed: {e}")))?, - ) - }; + NetworkPolicyRule::decode(record.proposed_rule.as_slice()) + .map(Some) + .map_err(|e| Status::internal(format!("decode proposed_rule failed: {e}"))) + } +} + +fn current_draft_chunk_security_notes(record: &DraftChunkRecord) -> Result { + Ok(decode_draft_chunk_rule(record)? + .as_ref() + .map_or_else(String::new, generate_security_notes)) +} + +fn draft_chunk_record_to_proto(record: &DraftChunkRecord) -> Result { + let proposed_rule = decode_draft_chunk_rule(record)?; + let security_notes = proposed_rule + .as_ref() + .map_or_else(String::new, generate_security_notes); Ok(PolicyChunk { id: record.id.clone(), @@ -3595,7 +3654,7 @@ fn draft_chunk_record_to_proto(record: &DraftChunkRecord) -> Result San } } +fn allowed_ip_is_internal(entry: &str) -> bool { + use openshell_core::net::{is_always_blocked_net, is_internal_net}; + + let parsed = entry.parse::().or_else(|_| { + entry.parse::().map(|ip| match ip { + IpAddr::V4(v4) => ipnet::IpNet::V4(ipnet::Ipv4Net::from(v4)), + IpAddr::V6(v6) => ipnet::IpNet::V6(ipnet::Ipv6Net::from(v6)), + }) + }); + let Ok(net) = parsed else { + return false; + }; + + // These remain hard validation failures, not warning-only destinations. + if is_always_blocked_net(net) { + return false; + } + + is_internal_net(net) +} + /// Re-validate security notes server-side for a proposed policy chunk. -fn generate_security_notes(host: &str, port: u16) -> String { +fn generate_security_notes(rule: &NetworkPolicyRule) -> String { let mut notes = Vec::new(); - // Flag destinations that are an internal/private address. Parse the host as - // an IP literal and defer to the canonical RFC-accurate classifier - // (openshell-core net::is_internal_ip) rather than naive string prefixes: - // `starts_with("172.")` wrongly matched 172.0-15 / 172.32-255 (RFC 1918 is - // only 172.16.0.0/12) and missed CGNAT (100.64.0.0/10), IPv6 ULA, etc. The - // "localhost" hostname is not an IP literal, so it is checked separately. - // See #1777. - let resolves_internal = host.parse::().is_ok_and(is_internal_ip); - if resolves_internal || host == "localhost" { - notes.push(format!( - "Destination '{host}' appears to be an internal/private address." - )); - } + for endpoint in &rule.endpoints { + let host = endpoint.host.to_lowercase(); + + // Flag destinations that are an internal/private address. Parse the host as + // an IP literal and defer to the canonical RFC-accurate classifier + // (openshell-core net::is_internal_ip) rather than naive string prefixes: + // `starts_with("172.")` wrongly matched 172.0-15 / 172.32-255 (RFC 1918 is + // only 172.16.0.0/12) and missed CGNAT (100.64.0.0/10), IPv6 ULA, etc. The + // Always-blocked destinations are rejected by merge validation instead + // of being presented as advisory findings. See #1777. + let resolves_advisory_internal = host + .parse::() + .is_ok_and(|ip| is_internal_ip(ip) && !is_always_blocked_ip(ip)); + if resolves_advisory_internal { + notes.push(format!( + "Destination '{host}' appears to be an internal/private address." + )); + } - if host.contains('*') { - notes.push(format!( - "Host '{host}' contains a wildcard — this may match unintended destinations." - )); - } + if host.contains('*') { + notes.push(format!( + "Host '{host}' contains a wildcard — this may match unintended destinations." + )); + } - if port > 49152 { - notes.push(format!( - "Port {port} is in the ephemeral range — this may be a temporary service." - )); - } + for allowed_ip in &endpoint.allowed_ips { + if allowed_ip_is_internal(allowed_ip) { + notes.push(format!( + "allowed_ips includes private/internal range '{allowed_ip}'." + )); + } + } + if host.trim().is_empty() && !endpoint.allowed_ips.is_empty() { + notes.push( + "allowed_ips allowlist is hostless and may match any hostname resolving within the configured range." + .to_string(), + ); + } - const DB_PORTS: [u16; 7] = [5432, 3306, 6379, 27017, 9200, 11211, 5672]; - if DB_PORTS.contains(&port) { - notes.push(format!( - "Port {port} is a well-known database/service port." - )); + let ports = if endpoint.ports.is_empty() { + std::slice::from_ref(&endpoint.port) + } else { + endpoint.ports.as_slice() + }; + for raw_port in ports { + let port = u16::try_from(*raw_port).unwrap_or(0); + if port > 49152 { + notes.push(format!( + "Port {port} is in the ephemeral range — this may be a temporary service." + )); + } + + const DB_PORTS: [u16; 7] = [5432, 3306, 6379, 27017, 9200, 11211, 5672]; + if DB_PORTS.contains(&port) { + notes.push(format!( + "Port {port} is a well-known database/service port." + )); + } + } } notes.join(" ") @@ -4444,25 +4552,159 @@ mod tests { request } + fn security_notes_for_host(host: &str) -> String { + generate_security_notes(&NetworkPolicyRule { + endpoints: vec![NetworkEndpoint { + host: host.to_string(), + port: 80, + ..Default::default() + }], + ..Default::default() + }) + } + #[test] fn security_notes_use_canonical_internal_ip_classifier() { // RFC 1918 is 172.16.0.0/12 only: the old starts_with("172.") prefix // wrongly flagged 172.15/172.32 and missed CGNAT (100.64.0.0/10). #1777. - assert!(generate_security_notes("172.16.0.1", 80).contains("internal/private")); - assert!(!generate_security_notes("172.15.0.1", 80).contains("internal/private")); - assert!(!generate_security_notes("172.32.0.1", 80).contains("internal/private")); - assert!(generate_security_notes("100.64.0.1", 80).contains("internal/private")); - assert!(generate_security_notes("10.0.0.1", 80).contains("internal/private")); - assert!(generate_security_notes("192.168.1.1", 80).contains("internal/private")); - assert!(generate_security_notes("127.0.0.1", 80).contains("internal/private")); - assert!(generate_security_notes("localhost", 80).contains("internal/private")); - assert!(!generate_security_notes("8.8.8.8", 80).contains("internal/private")); + assert!(security_notes_for_host("172.16.0.1").contains("internal/private")); + assert!(!security_notes_for_host("172.15.0.1").contains("internal/private")); + assert!(!security_notes_for_host("172.32.0.1").contains("internal/private")); + assert!(security_notes_for_host("100.64.0.1").contains("internal/private")); + assert!(security_notes_for_host("10.0.0.1").contains("internal/private")); + assert!(security_notes_for_host("192.168.1.1").contains("internal/private")); + assert!(!security_notes_for_host("8.8.8.8").contains("internal/private")); // Hostnames that merely start with a private-range prefix must NOT be // flagged: classification parses an IP literal, not a string prefix. #1824. - assert!(!generate_security_notes("10.example.com", 80).contains("internal/private")); - assert!(!generate_security_notes("172.example.com", 80).contains("internal/private")); + assert!(!security_notes_for_host("10.example.com").contains("internal/private")); + assert!(!security_notes_for_host("172.example.com").contains("internal/private")); // IPv6 ULA (fc00::/7, RFC 4193) is internal/private. - assert!(generate_security_notes("fd00::1", 80).contains("internal/private")); + assert!(security_notes_for_host("fd00::1").contains("internal/private")); + } + + #[test] + fn security_notes_exclude_always_blocked_destinations() { + let notes = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![ + NetworkEndpoint { + host: "127.0.0.1".to_string(), + port: 443, + ..Default::default() + }, + NetworkEndpoint { + host: "169.254.169.254".to_string(), + port: 443, + ..Default::default() + }, + NetworkEndpoint { + host: "localhost".to_string(), + port: 443, + ..Default::default() + }, + NetworkEndpoint { + host: "metadata.google.internal".to_string(), + port: 443, + allowed_ips: vec![ + "127.0.0.0/8".to_string(), + "169.254.0.0/16".to_string(), + "::/128".to_string(), + ], + ..Default::default() + }, + ], + ..Default::default() + }); + + assert!(notes.is_empty(), "{notes}"); + } + + #[test] + fn security_notes_flag_private_allowed_ips() { + let notes = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + allowed_ips: vec!["10.0.0.0/8".to_string()], + ..Default::default() + }], + ..Default::default() + }); + + assert!(notes.contains("allowed_ips includes private/internal range '10.0.0.0/8'.")); + } + + #[test] + fn security_notes_flag_cidr_overlapping_cgnat() { + let notes = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + allowed_ips: vec!["100.0.0.0/9".to_string()], + ..Default::default() + }], + ..Default::default() + }); + + assert!(notes.contains("allowed_ips includes private/internal range '100.0.0.0/9'.")); + } + + #[test] + fn security_notes_do_not_flag_large_public_allowed_ips() { + let notes = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + allowed_ips: vec!["8.8.0.0/16".to_string(), "8.8.8.0/24".to_string()], + ..Default::default() + }], + ..Default::default() + }); + + assert!(notes.is_empty(), "{notes}"); + } + + #[test] + fn security_notes_flag_hostless_allowed_ips() { + let notes = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![NetworkEndpoint { + allowed_ips: vec!["192.168.0.0/16".to_string()], + port: 443, + ..Default::default() + }], + ..Default::default() + }); + + assert!(notes.contains("allowed_ips includes private/internal range '192.168.0.0/16'.")); + assert!(notes.contains( + "allowed_ips allowlist is hostless and may match any hostname resolving within the configured range." + )); + } + + #[test] + fn security_notes_inspect_all_endpoints_and_effective_ports() { + let notes = generate_security_notes(&NetworkPolicyRule { + endpoints: vec![ + NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + ..Default::default() + }, + NetworkEndpoint { + host: "*.internal.example".to_string(), + port: 3306, + ports: vec![5432, 50_000], + allowed_ips: vec!["172.16.0.0/12".to_string()], + ..Default::default() + }, + ], + ..Default::default() + }); + + assert!(notes.contains("Host '*.internal.example' contains a wildcard")); + assert!(notes.contains("allowed_ips includes private/internal range '172.16.0.0/12'.")); + assert!(notes.contains("Port 5432 is a well-known database/service port.")); + assert!(notes.contains("Port 50000 is in the ephemeral range")); + assert!(!notes.contains("Port 3306")); } #[test] @@ -6672,6 +6914,497 @@ mod tests { .unwrap(); } + #[tokio::test] + async fn approve_all_skips_private_allowed_ips_unless_included() { + let state = test_server_state().await; + let sandbox_name = "private-allowed-ips"; + state + .store + .put_message(&test_sandbox( + "sb-private-allowed-ips", + sandbox_name, + ProtoSandboxPolicy::default(), + vec![], + )) + .await + .unwrap(); + + let submit = handle_submit_policy_analysis( + &state, + with_user(Request::new(SubmitPolicyAnalysisRequest { + name: sandbox_name.to_string(), + analysis_mode: "agent_authored".to_string(), + proposed_chunks: vec![PolicyChunk { + rule_name: "private_service".to_string(), + proposed_rule: Some(NetworkPolicyRule { + name: "private_service".to_string(), + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + allowed_ips: vec!["10.0.0.0/8".to_string()], + ..Default::default() + }], + binaries: vec![NetworkBinary { + path: "/usr/bin/curl".to_string(), + ..Default::default() + }], + }), + ..Default::default() + }], + ..Default::default() + })), + ) + .await + .unwrap() + .into_inner(); + let chunk_id = submit.accepted_chunk_ids[0].clone(); + let chunk = state + .store + .get_draft_chunk(&chunk_id) + .await + .unwrap() + .unwrap(); + assert!( + chunk + .security_notes + .contains("allowed_ips includes private/internal range '10.0.0.0/8'.") + ); + + let skipped = handle_approve_all_draft_chunks( + &state, + with_user(Request::new(ApproveAllDraftChunksRequest { + name: sandbox_name.to_string(), + include_security_flagged: false, + workspace: "default".to_string(), + })), + ) + .await + .unwrap() + .into_inner(); + assert_eq!(skipped.chunks_approved, 0); + assert_eq!(skipped.chunks_skipped, 1); + assert_eq!( + state + .store + .get_draft_chunk(&chunk_id) + .await + .unwrap() + .unwrap() + .status, + "pending" + ); + + let approved = handle_approve_all_draft_chunks( + &state, + with_user(Request::new(ApproveAllDraftChunksRequest { + name: sandbox_name.to_string(), + include_security_flagged: true, + workspace: "default".to_string(), + })), + ) + .await + .unwrap() + .into_inner(); + assert_eq!(approved.chunks_approved, 1); + assert_eq!(approved.chunks_skipped, 0); + assert_eq!( + state + .store + .get_draft_chunk(&chunk_id) + .await + .unwrap() + .unwrap() + .status, + "approved" + ); + } + + #[tokio::test] + async fn edit_recomputes_security_notes_before_read_and_approve_all() { + let state = test_server_state().await; + let sandbox_id = "sb-edit-security-notes"; + let sandbox_name = "edit-security-notes"; + state + .store + .put_message(&test_sandbox( + sandbox_id, + sandbox_name, + ProtoSandboxPolicy::default(), + vec![], + )) + .await + .unwrap(); + + let safe_rule = NetworkPolicyRule { + name: "edited_service".to_string(), + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + ..Default::default() + }], + binaries: vec![NetworkBinary { + path: "/usr/bin/curl".to_string(), + ..Default::default() + }], + }; + let submit = handle_submit_policy_analysis( + &state, + with_user(Request::new(SubmitPolicyAnalysisRequest { + name: sandbox_name.to_string(), + analysis_mode: "agent_authored".to_string(), + proposed_chunks: vec![PolicyChunk { + rule_name: "edited_service".to_string(), + proposed_rule: Some(safe_rule.clone()), + ..Default::default() + }], + ..Default::default() + })), + ) + .await + .unwrap() + .into_inner(); + let chunk_id = submit.accepted_chunk_ids[0].clone(); + + let mut private_rule = safe_rule; + private_rule.endpoints[0].allowed_ips = vec!["10.0.0.0/8".to_string()]; + handle_edit_draft_chunk( + &state, + with_user(Request::new(EditDraftChunkRequest { + name: sandbox_name.to_string(), + chunk_id: chunk_id.clone(), + proposed_rule: Some(private_rule), + workspace: "default".to_string(), + })), + ) + .await + .unwrap(); + + let stored = state + .store + .get_draft_chunk(&chunk_id) + .await + .unwrap() + .unwrap(); + assert!(stored.security_notes.is_empty()); + + let draft = handle_get_draft_policy( + &state, + with_user(Request::new(GetDraftPolicyRequest { + name: sandbox_name.to_string(), + status_filter: String::new(), + workspace: "default".to_string(), + })), + ) + .await + .unwrap() + .into_inner(); + assert!( + draft.chunks[0] + .security_notes + .contains("allowed_ips includes private/internal range '10.0.0.0/8'.") + ); + + let skipped = handle_approve_all_draft_chunks( + &state, + with_user(Request::new(ApproveAllDraftChunksRequest { + name: sandbox_name.to_string(), + include_security_flagged: false, + workspace: "default".to_string(), + })), + ) + .await + .unwrap() + .into_inner(); + assert_eq!(skipped.chunks_approved, 0); + assert_eq!(skipped.chunks_skipped, 1); + assert_eq!( + state + .store + .get_draft_chunk(&chunk_id) + .await + .unwrap() + .unwrap() + .status, + "pending" + ); + } + + #[tokio::test] + async fn approve_all_recomputes_empty_legacy_security_notes() { + let state = test_server_state().await; + let sandbox_id = "sb-stale-security-notes"; + let sandbox_name = "stale-security-notes"; + state + .store + .put_message(&test_sandbox( + sandbox_id, + sandbox_name, + ProtoSandboxPolicy::default(), + vec![], + )) + .await + .unwrap(); + + let rule = NetworkPolicyRule { + name: "legacy_private".to_string(), + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + allowed_ips: vec!["10.0.0.0/8".to_string()], + ..Default::default() + }], + binaries: vec![NetworkBinary { + path: "/usr/bin/curl".to_string(), + ..Default::default() + }], + }; + let mut chunk = pending_draft_chunk("legacy-private", sandbox_id); + chunk.rule_name = rule.name.clone(); + chunk.proposed_rule = rule.encode_to_vec(); + chunk.host = "service.example.com".to_string(); + chunk.port = 443; + state + .store + .put_draft_chunk(&chunk, None, "default") + .await + .unwrap(); + + let skipped = handle_approve_all_draft_chunks( + &state, + with_user(Request::new(ApproveAllDraftChunksRequest { + name: sandbox_name.to_string(), + include_security_flagged: false, + workspace: "default".to_string(), + })), + ) + .await + .unwrap() + .into_inner(); + assert_eq!(skipped.chunks_approved, 0); + assert_eq!(skipped.chunks_skipped, 1); + assert_eq!( + state + .store + .get_draft_chunk(&chunk.id) + .await + .unwrap() + .unwrap() + .status, + "pending" + ); + } + + #[tokio::test] + async fn security_flagged_empty_delta_does_not_auto_approve() { + let state = test_server_state().await; + let sandbox_name = "security-flagged-auto"; + state + .store + .put_message(&test_sandbox( + "sb-security-flagged-auto", + sandbox_name, + ProtoSandboxPolicy::default(), + vec![], + )) + .await + .unwrap(); + seed_sandbox_approval_mode(&state, sandbox_name, "auto").await; + + handle_submit_policy_analysis( + &state, + with_user(Request::new(SubmitPolicyAnalysisRequest { + name: sandbox_name.to_string(), + analysis_mode: "mechanistic".to_string(), + proposed_chunks: vec![PolicyChunk { + rule_name: "private_service".to_string(), + proposed_rule: Some(NetworkPolicyRule { + name: "private_service".to_string(), + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + allowed_ips: vec!["10.0.0.0/8".to_string()], + ..Default::default() + }], + binaries: vec![NetworkBinary { + path: "/usr/bin/curl".to_string(), + ..Default::default() + }], + }), + ..Default::default() + }], + ..Default::default() + })), + ) + .await + .unwrap(); + + let draft = handle_get_draft_policy( + &state, + with_user(Request::new(GetDraftPolicyRequest { + name: sandbox_name.to_string(), + status_filter: String::new(), + workspace: "default".to_string(), + })), + ) + .await + .unwrap() + .into_inner(); + assert_eq!(draft.chunks[0].validation_result, "prover: no new findings"); + assert_eq!(draft.chunks[0].status, "pending"); + assert!( + draft.chunks[0] + .security_notes + .contains("allowed_ips includes private/internal range '10.0.0.0/8'.") + ); + } + + #[tokio::test] + async fn mechanistic_dedup_auto_approval_rechecks_edited_stored_rule() { + let state = test_server_state().await; + let sandbox_id = "sb-stored-prover-verdict"; + let sandbox_name = "stored-prover-verdict"; + state + .store + .put_message(&test_provider("github-pat", "github")) + .await + .unwrap(); + state + .store + .put_message(&test_sandbox( + sandbox_id, + sandbox_name, + ProtoSandboxPolicy::default(), + vec!["github-pat".to_string()], + )) + .await + .unwrap(); + + // The first observation is clean and remains pending in the default + // manual mode. Its normalized endpoint columns become the dedup key. + let safe_rule = NetworkPolicyRule { + name: "safe_service".to_string(), + endpoints: vec![NetworkEndpoint { + host: "service.example.com".to_string(), + port: 443, + ..Default::default() + }], + binaries: vec![NetworkBinary { + path: "/usr/bin/curl".to_string(), + ..Default::default() + }], + }; + let first = handle_submit_policy_analysis( + &state, + with_user(Request::new(SubmitPolicyAnalysisRequest { + name: sandbox_name.to_string(), + analysis_mode: "mechanistic".to_string(), + proposed_chunks: vec![PolicyChunk { + rule_name: safe_rule.name.clone(), + proposed_rule: Some(safe_rule.clone()), + ..Default::default() + }], + ..Default::default() + })), + ) + .await + .unwrap() + .into_inner(); + let chunk_id = first.accepted_chunk_ids[0].clone(); + let original = state + .store + .get_draft_chunk(&chunk_id) + .await + .unwrap() + .unwrap(); + assert_eq!(original.status, "pending"); + assert_eq!(original.validation_result, "prover: no new findings"); + + // Editing only replaces the stored protobuf payload; the normalized + // dedup columns remain service.example.com:443 + /usr/bin/curl. This + // replacement has no advisory notes but does add credentialed reach. + let finding_rule = NetworkPolicyRule { + name: safe_rule.name.clone(), + endpoints: vec![NetworkEndpoint { + host: "api.github.com".to_string(), + port: 443, + ..Default::default() + }], + binaries: safe_rule.binaries.clone(), + }; + assert!(generate_security_notes(&finding_rule).is_empty()); + let credential_set = CredentialSet { + credentials: vec![Credential { + name: "github-pat".to_string(), + cred_type: "github".to_string(), + scopes: Vec::new(), + injected_via: String::new(), + target_hosts: vec!["api.github.com".to_string()], + }], + api_registries: HashMap::new(), + }; + assert!( + validation_result_for_agent_proposal( + ProtoSandboxPolicy::default(), + &finding_rule.name, + &finding_rule, + &credential_set, + ) + .contains("credential_reach_expansion") + ); + handle_edit_draft_chunk( + &state, + with_user(Request::new(EditDraftChunkRequest { + name: sandbox_name.to_string(), + chunk_id: chunk_id.clone(), + proposed_rule: Some(finding_rule), + workspace: "default".to_string(), + })), + ) + .await + .unwrap(); + + seed_sandbox_approval_mode(&state, sandbox_name, "auto").await; + + // The duplicate incoming proposal is clean. Dedup returns the edited + // row, so auto-approval must evaluate that stored payload instead. + let duplicate = handle_submit_policy_analysis( + &state, + with_user(Request::new(SubmitPolicyAnalysisRequest { + name: sandbox_name.to_string(), + analysis_mode: "mechanistic".to_string(), + proposed_chunks: vec![PolicyChunk { + rule_name: safe_rule.name.clone(), + proposed_rule: Some(safe_rule), + ..Default::default() + }], + ..Default::default() + })), + ) + .await + .unwrap() + .into_inner(); + assert_eq!(duplicate.accepted_chunk_ids, vec![chunk_id.clone()]); + + let stored = state + .store + .get_draft_chunk(&chunk_id) + .await + .unwrap() + .unwrap(); + assert_eq!(stored.status, "pending"); + let stored_rule = NetworkPolicyRule::decode(stored.proposed_rule.as_slice()).unwrap(); + assert_eq!(stored_rule.endpoints[0].host, "api.github.com"); + assert!(generate_security_notes(&stored_rule).is_empty()); + assert!( + state + .store + .get_latest_policy(sandbox_id) + .await + .unwrap() + .is_none() + ); + } + #[tokio::test] async fn draft_chunk_handler_lifecycle_round_trip() { use openshell_core::proto::{ @@ -9106,9 +9839,9 @@ mod tests { seed_sandbox_approval_mode(&state, &sandbox_name, "auto").await; let proposed_rule = NetworkPolicyRule { - name: "allow_10_0_0_5_8080".to_string(), + name: "allow_example_8080".to_string(), endpoints: vec![NetworkEndpoint { - host: "10.0.0.5".to_string(), + host: "example.com".to_string(), port: 8080, ..Default::default() }], @@ -9128,7 +9861,7 @@ mod tests { name: sandbox_name, analysis_mode: "mechanistic".to_string(), proposed_chunks: vec![PolicyChunk { - rule_name: "allow_10_0_0_5_8080".to_string(), + rule_name: "allow_example_8080".to_string(), proposed_rule: Some(rule), ..Default::default() }], @@ -9207,7 +9940,7 @@ mod tests { .expect("auto-approve must have persisted a policy revision"); let policy = SandboxPolicy::decode(latest.policy_payload.as_slice()).unwrap(); assert!( - policy.network_policies.contains_key("allow_10_0_0_5_8080"), + policy.network_policies.contains_key("allow_example_8080"), "approved rule must stay merged after resubmit; keys: {:?}", policy.network_policies.keys().collect::>() ); diff --git a/docs/sandboxes/policy-advisor.mdx b/docs/sandboxes/policy-advisor.mdx index 6139aafb38..76c5e1f45b 100644 --- a/docs/sandboxes/policy-advisor.mdx +++ b/docs/sandboxes/policy-advisor.mdx @@ -10,7 +10,7 @@ position: 6 Policy advisor lets a running sandboxed agent ask for a narrow network policy change after OpenShell denies a request. The agent submits a draft through `policy.local`, a developer approves or rejects it from outside the sandbox, and approved network policy hot-reloads into the same sandbox. -Policy advisor preserves OpenShell's default-deny posture. The structured rule is the approval contract, and the agent's rationale is supporting context. By default every proposal lands in the draft inbox for human review. Opt-in [auto mode](#approval-modes) lets the gateway approve provably safe proposals — those whose [prover delta](#what-auto-approval-checks) is empty — without a reviewer in the loop; proposals with any prover finding still require human approval. +Policy advisor preserves OpenShell's default-deny posture. The structured rule is the approval contract, and the agent's rationale is supporting context. By default every accepted proposal lands in the draft inbox for human review. Opt-in [auto mode](#approval-modes) approves a proposal without a reviewer only when its [prover delta](#what-auto-approval-checks) is empty and the current draft rule produces no security notes. A prover finding or security note keeps the proposal pending for human review. ## Enable Policy Advisor @@ -49,16 +49,16 @@ Set the value before creating a sandbox when you want the first denied request t ## Approval Modes -Every proposal — mechanistic or agent-authored — is routed through the [policy prover](#what-auto-approval-checks). The `proposal_approval_mode` setting decides what happens when the prover finds nothing to flag. +Every proposal, mechanistic or agent-authored, is routed through the [policy prover](#what-auto-approval-checks). The gateway also recalculates security notes from the current draft rule before auto-approval. The `proposal_approval_mode` setting decides whether proposals that pass both checks require human review. | Mode | When unset / `manual` | `auto` | |---|---|---| -| Empty prover delta | Lands in the draft inbox for human review. | Approved automatically; the sandbox hot-reloads the new rule and the agent retries. | -| Any prover finding | Lands in the draft inbox. | Lands in the draft inbox — auto-approval is gated on an empty delta. | +| Empty prover delta and no security notes | Lands in the draft inbox for human review. | Approved automatically. The sandbox hot-reloads the new rule and the agent retries. | +| Any prover finding or security note | Lands in the draft inbox. | Remains pending for human review. | `manual` is the default. Auto mode is an explicit opt-in; OpenShell's default-deny posture is preserved unless you choose otherwise. -Enable auto mode at gateway scope when you want every sandbox on this gateway to auto-approve safe proposals: +Enable auto mode at gateway scope when you want every sandbox on this gateway to auto-approve eligible proposals: ```shell openshell settings set --global \ @@ -103,7 +103,7 @@ The loop has seven steps: 3. The agent reads the policy advisor skill, inspects the current policy, and optionally reads recent denial log lines. 4. The agent submits one or more `addRule` proposals to `http://policy.local/v1/proposals`. 5. The gateway stores accepted proposals as pending draft chunks for the sandbox and runs the [policy prover](#what-auto-approval-checks) against the proposed delta. -6. Under `auto` mode, proposals with an empty prover delta are approved immediately and skipped past human review. Under `manual` mode (the default), every proposal — and under `auto` mode, every proposal with a prover finding — lands in the draft inbox for a developer to approve or reject. +6. Before auto-approval, the gateway reloads the current stored rule, recalculates its prover verdict, and regenerates its security notes. Under `auto` mode, it approves the proposal only when both the prover delta and security notes are empty. Any finding or security note keeps the proposal pending. Under `manual` mode, every accepted proposal lands in the draft inbox for a developer to approve or reject. 7. The agent waits on `/v1/proposals/{chunk_id}/wait` until a decision is available. Approved proposals hot-reload into the sandbox; rejected proposals return `rejection_reason` and `validation_result` so the agent can revise. When a proposal is approved, `/wait` reports `policy_reloaded: true` only after the local sandbox policy covers the approved rule. At that point the agent can retry the original denied action once. If a proposal is rejected, `/wait` returns `rejection_reason` and `validation_result` so the agent can revise or stop. `validation_result` carries the categorical prover findings — `link_local_reach`, `l7_bypass_credentialed`, `credential_reach_expansion`, `capability_expansion` — so the agent can narrow the next attempt to the specific concern the prover flagged. @@ -160,18 +160,26 @@ The current `policy.local` JSON shape covers L4 endpoints and REST method or pat Policy advisor proposals do not add `allowed_ips` automatically. If an advisor-proposed hostname resolves to an internal or private address, OpenShell's SSRF protections still block the connection until a developer explicitly adds the required `allowed_ips` entry. Exact hostname trust for user-declared policy endpoints does not apply to advisor-generated proposal binaries. +Private RFC 1918, CGNAT, IPv6 ULA, and other special-use destinations classified as internal produce advisory security notes when they appear as literal endpoint IPs or in `allowed_ips`. CIDR intersections are included, and hostless `allowed_ips` rules receive an additional warning because they can match any hostname resolving into the configured range. + +Always-blocked destinations are not advisory. Loopback, link-local, and unspecified IPs or CIDRs, plus `localhost` and known metadata endpoint hostnames, are excluded from security notes. Submit and edit can store such a draft, but approval fails when merge validation prevents it from entering the active policy. Runtime SSRF protections continue to enforce the same boundary. + ## What Auto-Approval Checks -The policy prover runs against every proposal — mechanistic and agent-authored alike — and asks four formal questions about the proposed change. Each "yes" is one categorical finding. Any finding blocks auto-approval; only an empty delta is eligible. +Auto-approval requires all three conditions: the effective mode is `auto`, the prover delta is empty, and recalculating security notes from the current stored rule produces none. + +The policy prover runs against mechanistic and agent-authored proposals alike and asks four formal questions about the proposed change. Each "yes" is one categorical finding. Any finding blocks auto-approval. An empty delta is necessary but not sufficient. | Category | Triggered when | |---|---| -| `link_local_reach` | The proposal reaches a host in `169.254.0.0/16`, `fe80::/10`, or a known metadata hostname such as `metadata.google.internal` (cloud-metadata territory, which serves credentials regardless of sandbox state). Unconditional. | +| `link_local_reach` | A rule reaches `169.254.0.0/16`, `fe80::/10`, or a known metadata hostname. | | `l7_bypass_credentialed` | A binary using a wire protocol the L7 proxy cannot inspect (`git-remote-https`, `ssh`, `nc`) gains reach to a host where a credential is in scope. | | `credential_reach_expansion` | A binary gains credentialed reach to a `(host, port)` it could not reach before. | | `capability_expansion` | On a `(binary, host, port)` that already had credentialed reach, the proposal adds a new HTTP method. The finding cites the specific method. | -Findings are categorical — there is no severity tier. The reviewer reads the category and the structured evidence to decide. When the prover delta is empty, the proposal is provably safe under the model and auto-approval (if enabled) can fire. +Findings are categorical. There is no severity tier. The reviewer reads the category and the structured evidence to decide. + +Before auto-approval, the gateway reloads the draft chunk, recomputes its prover verdict, and regenerates security notes from its current rule. These checks apply after edits and deduplicated resubmissions: an edited stored rule controls the decision even when the duplicate incoming proposal has an empty prover delta. The recalculated verdict is used for this decision but is not written back, so a later `validation_result` read can still show the submit-time verdict. Failures leave the chunk pending. Security notes flag concerns such as internal or private destinations and `allowed_ips`, wildcard hosts, hostless `allowed_ips`, ephemeral ports, and well-known database or service ports. Draft reads and bulk approval also regenerate notes from the stored rule. Any prover finding or security note keeps the chunk pending. The full reasoning model lives in [`crates/openshell-prover/README.md`](https://github.com/NVIDIA/OpenShell/blob/main/crates/openshell-prover/README.md). Provider profiles composed in via [Providers v2](/sandboxes/providers-v2) are part of the effective policy the prover reasons over. @@ -183,7 +191,7 @@ Review pending chunks from the host: openshell rule get --status pending ``` -Under `auto` mode, only proposals the prover flagged appear here; empty-delta proposals are already approved and visible under `--status approved` with the auto-approval audit fields described in [Approval Modes](#approval-modes). Under `manual` mode, every proposal — regardless of prover verdict — shows up as pending. +Under `auto` mode, proposals with a prover finding or any recalculated security note remain pending for human review. Proposals that pass both checks are visible under `--status approved` with the auto-approval audit fields described in [Approval Modes](#approval-modes). Under `manual` mode, every accepted proposal shows up as pending regardless of the prover verdict or security notes. The output shows the chunk ID, status, rationale, binary, and endpoint summary. For L7 proposals, the endpoint summary includes the protocol, method, and path: