fix(policy): keep internal allowed IP proposals pending (#2416)

Signed-off-by: Adrien Langou <alangou@nvidia.com>

Author: alangou

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=<mode>`,
-   `prover_delta=empty`, and `resolved_from=<gateway|sandbox>` 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=<mode>`, `prover_delta=empty`, and
+   `resolved_from=<gateway|sandbox>` 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

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,90 +440,90 @@ 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));
     }
 
     #[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,25 +532,82 @@ 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));
     }
 
     #[test]
     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]