fix(sandbox,server): fix chunk merge duplicates and OPA variable collision with overlapping policies (#571)

* fix(sandbox,server): fix chunk merge duplicates and OPA variable collision with overlapping policies

Two related bugs triggered when a draft rule approval creates a second
policy entry for the same host:port:

1. merge_chunk_into_policy looked up existing rules by chunk.rule_name
   (auto-generated as allow_{host}_{port}), which never matched the
   user's original rule name.  Now scans all network_policies entries
   for a host:port endpoint match before falling back to insertion,
   and merges allowed_ips into the existing endpoint.

2. The Rego allow_request rule and _matching_endpoint_configs
   comprehension used 'some ep; ep := policy.endpoints[_]' which
   caused regorus to error with 'duplicated definition of local
   variable ep' when multiple policies covered the same host:port.
   Refactored to isolate endpoint iteration inside helper functions
   (_policy_allows_l7, _policy_endpoint_configs) so variables are
   scoped per-policy evaluation.

Refs: #567

* test(e2e): add overlapping policy tests and update FWD-2 for implicit allowed_ips

- Update FWD-2 (test_forward_proxy_denied_without_allowed_ips ->
  test_forward_proxy_allows_private_ip_host_without_allowed_ips):
  literal IP host no longer requires explicit allowed_ips, expects 200.

- Add OVL-1: overlapping L4 policies for same host:port must not crash
  OPA and should allow forward proxy connections.

- Add OVL-2: overlapping L7 policies for same host:port must not crash
  OPA and should allow CONNECT tunnel establishment.

Refs: #567

* style: apply cargo fmt formatting

* test(e2e): update SSRF-3 and SSRF-6 for implicit allowed_ips behavior

SSRF-6: Private IP with literal IP host now gets implicit allowed_ips
from PR #570, so CONNECT returns 200 instead of 403.

SSRF-3: Loopback is still blocked but via the always-blocked path
(implicit allowed_ips is synthesized, then resolve_and_check_allowed_ips
catches it). Log message says 'always-blocked' instead of 'internal
address'.

* fix(e2e): use negative assertion for SSRF-6 when nothing listens on target port

When the SSRF check passes but nothing listens on the target port,
recv() returns empty bytes. Use 'assert 403 not in' (matching SSRF-4
pattern) instead of 'assert 200 in'.

* fix(e2e): update provider tests for redacted credential values

PR #569 changed credential redaction from clearing the map to
replacing values with 'REDACTED'. Update e2e assertions to expect
credential keys with REDACTED values instead of an empty map.

Author: johntmyers

crates/openshell-sandbox/data/sandbox-policy.rego

@@ -171,16 +171,25 @@ network_action := "allow" if {
 
 default allow_request = false
 
-# L7 request allowed if: L4 policy matches AND the specific endpoint's rules allow the request.
+# Per-policy helper: true when this single policy has at least one endpoint
+# matching the L4 request whose L7 rules also permit the specific request.
+# Isolating the endpoint iteration inside a function avoids the regorus
+# "duplicated definition of local variable" error that occurs when the
+# outer `some name` iterates over multiple policies that share a host:port.
+_policy_allows_l7(policy) if {
+	some ep
+	ep := policy.endpoints[_]
+	endpoint_matches_request(ep, input.network)
+	request_allowed_for_endpoint(input.request, ep)
+}
+
+# L7 request allowed if any matching L4 policy also allows the L7 request.
 allow_request if {
 	some name
 	policy := data.network_policies[name]
 	endpoint_allowed(policy, input.network)
 	binary_allowed(policy, input.exec)
-	some ep
-	ep := policy.endpoints[_]
-	endpoint_matches_request(ep, input.network)
-	request_allowed_for_endpoint(input.request, ep)
+	_policy_allows_l7(policy)
 }
 
 # --- L7 deny reason ---
@@ -239,19 +248,24 @@ command_matches(actual, expected) if {
 # Used by Rust to extract L7 config (protocol, tls, enforcement) and/or
 # allowed_ips for SSRF allowlist validation.
 
-# Collect all matching endpoint configs into an array to avoid complete-rule
-# conflicts when multiple policies cover the same endpoint.  Return the first.
-_matching_endpoint_configs := [ep |
-	some name
-	policy := data.network_policies[name]
-	endpoint_allowed(policy, input.network)
-	binary_allowed(policy, input.exec)
+# Per-policy helper: returns matching endpoint configs for a single policy.
+_policy_endpoint_configs(policy) := [ep |
 	some ep
 	ep := policy.endpoints[_]
 	endpoint_matches_request(ep, input.network)
 	endpoint_has_extended_config(ep)
 ]
 
+# Collect matching endpoint configs across all policies.  Iterates over
+# _matching_policy_names (a set, safe from regorus variable collisions)
+# then collects per-policy configs via the helper function.
+_matching_endpoint_configs := [cfg |
+	some pname
+	_matching_policy_names[pname]
+	cfgs := _policy_endpoint_configs(data.network_policies[pname])
+	cfg := cfgs[_]
+]
+
 matched_endpoint_config := _matching_endpoint_configs[0] if {
 	count(_matching_endpoint_configs) > 0
 }

crates/openshell-sandbox/src/opa.rs

@@ -1568,6 +1568,92 @@ process:
         assert_eq!(val, regorus::Value::from(true));
     }
 
+    // ========================================================================
+    // Overlapping policies (duplicate host:port) — regression tests
+    // ========================================================================
+
+    /// Two network_policies entries covering the same host:port with L7 rules.
+    /// Before the fix, this caused regorus to fail with
+    /// "duplicated definition of local variable ep" in allow_request.
+    const OVERLAPPING_L7_TEST_DATA: &str = r#"
+network_policies:
+  test_server:
+    name: test_server
+    endpoints:
+      - host: 192.168.1.100
+        port: 8567
+        protocol: rest
+        enforcement: enforce
+        rules:
+          - allow:
+              method: GET
+              path: "**"
+    binaries:
+      - { path: /usr/bin/curl }
+  allow_192_168_1_100_8567:
+    name: allow_192_168_1_100_8567
+    endpoints:
+      - host: 192.168.1.100
+        port: 8567
+        protocol: rest
+        enforcement: enforce
+        allowed_ips:
+          - 192.168.1.100
+        rules:
+          - allow:
+              method: GET
+              path: "**"
+    binaries:
+      - { path: /usr/bin/curl }
+filesystem_policy:
+  include_workdir: true
+  read_only: []
+  read_write: []
+landlock:
+  compatibility: best_effort
+process:
+  run_as_user: sandbox
+  run_as_group: sandbox
+"#;
+
+    #[test]
+    fn l7_overlapping_policies_allow_request_does_not_crash() {
+        let engine = OpaEngine::from_strings(TEST_POLICY, OVERLAPPING_L7_TEST_DATA)
+            .expect("engine should load overlapping data");
+        let input = l7_input("192.168.1.100", 8567, "GET", "/test");
+        // Should not panic or error — must evaluate to true.
+        assert!(eval_l7(&engine, &input));
+    }
+
+    #[test]
+    fn l7_overlapping_policies_deny_request_does_not_crash() {
+        let engine = OpaEngine::from_strings(TEST_POLICY, OVERLAPPING_L7_TEST_DATA)
+            .expect("engine should load overlapping data");
+        let input = l7_input("192.168.1.100", 8567, "DELETE", "/test");
+        // DELETE is not in the rules, so should deny — but must not crash.
+        assert!(!eval_l7(&engine, &input));
+    }
+
+    #[test]
+    fn overlapping_policies_endpoint_config_returns_result() {
+        let engine = OpaEngine::from_strings(TEST_POLICY, OVERLAPPING_L7_TEST_DATA)
+            .expect("engine should load overlapping data");
+        let input = NetworkInput {
+            host: "192.168.1.100".into(),
+            port: 8567,
+            binary_path: PathBuf::from("/usr/bin/curl"),
+            binary_sha256: String::new(),
+            ancestors: vec![],
+            cmdline_paths: vec![],
+        };
+        // Should return config from one of the entries without error.
+        let config = engine.query_endpoint_config(&input).unwrap();
+        assert!(
+            config.is_some(),
+            "Expected endpoint config for overlapping policies"
+        );
+    }
+
     // ========================================================================
     // network_action tests
     // ========================================================================