From 921db61b5e15689fcd2a092ed6592c972a69e002 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Mon, 13 Apr 2026 12:46:39 -0700 Subject: [PATCH 1/2] feat(policy): add deny rules to network policy schema Closes #565 Add L7 deny rules that block specific requests even when allowed by access presets or explicit allow rules. Deny rules mirror the full capability set of allow rules (method, path, query params, SQL command) and take precedence -- if a request matches any deny rule, it is blocked regardless of allow rules. This enables the "allow everything except these specific operations" pattern without enumerating every allowed endpoint. For example, granting read-write access to GitHub while blocking PR approvals, branch protection changes, and ruleset modifications. --- .../skills/generate-sandbox-policy/SKILL.md | 27 ++ crates/openshell-policy/src/lib.rs | 172 ++++++++++- .../data/sandbox-policy.rego | 69 ++++- crates/openshell-sandbox/src/l7/mod.rs | 108 +++++++ crates/openshell-sandbox/src/opa.rs | 273 ++++++++++++++++++ docs/reference/policy-schema.mdx | 33 +++ proto/sandbox.proto | 19 ++ 7 files changed, 699 insertions(+), 2 deletions(-) diff --git a/.agents/skills/generate-sandbox-policy/SKILL.md b/.agents/skills/generate-sandbox-policy/SKILL.md index 953302724..7149bbd61 100644 --- a/.agents/skills/generate-sandbox-policy/SKILL.md +++ b/.agents/skills/generate-sandbox-policy/SKILL.md @@ -266,6 +266,33 @@ network_policies: - { path: } ``` +### Deny Rules + +Use `deny_rules` to block specific dangerous operations while allowing broad access. Deny rules are evaluated after allow rules and take precedence. This is the inverse of the `rules` approach — instead of enumerating every allowed operation, you grant broad access and block a small set of dangerous ones. + +```yaml +# Example: Allow full access to GitHub but block admin operations +github_api: + name: github_api + endpoints: + - host: api.github.com + port: 443 + protocol: rest + enforcement: enforce + access: read-write + deny_rules: + - method: POST + path: "/repos/*/pulls/*/reviews" + - method: PUT + path: "/repos/*/branches/*/protection" + - method: "*" + path: "/repos/*/rulesets" + binaries: + - { path: /usr/bin/curl } +``` + +Deny rules support the same matching capabilities as allow rules: `method`, `path`, `command` (SQL), and `query` parameter matchers. When generating policies, prefer deny rules when the user needs broad access with a small set of blocked operations — it produces a shorter, more maintainable policy than enumerating 60+ allow rules. + ### Private IP Destinations When the endpoint resolves to a private IP (RFC 1918), the proxy's SSRF protection blocks the connection by default. Use `allowed_ips` to selectively allow specific private IP ranges: diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index 84358ea67..e3c26061a 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -15,7 +15,7 @@ use std::path::Path; use miette::{IntoDiagnostic, Result, WrapErr}; use openshell_core::proto::{ - FilesystemPolicy, L7Allow, L7QueryMatcher, L7Rule, LandlockPolicy, NetworkBinary, + FilesystemPolicy, L7Allow, L7DenyRule, L7QueryMatcher, L7Rule, LandlockPolicy, NetworkBinary, NetworkEndpoint, NetworkPolicyRule, ProcessPolicy, SandboxPolicy, }; use serde::{Deserialize, Serialize}; @@ -100,6 +100,8 @@ struct NetworkEndpointDef { rules: Vec, #[serde(default, skip_serializing_if = "Vec::is_empty")] allowed_ips: Vec, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + deny_rules: Vec, } fn is_zero(v: &u16) -> bool { @@ -139,6 +141,19 @@ struct QueryAnyDef { any: Vec, } +#[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct L7DenyRuleDef { + #[serde(default, skip_serializing_if = "String::is_empty")] + method: String, + #[serde(default, skip_serializing_if = "String::is_empty")] + path: String, + #[serde(default, skip_serializing_if = "String::is_empty")] + command: String, + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + query: BTreeMap, +} + #[derive(Debug, Serialize, Deserialize)] #[serde(deny_unknown_fields)] struct NetworkBinaryDef { @@ -214,6 +229,31 @@ fn to_proto(raw: PolicyFile) -> SandboxPolicy { }) .collect(), allowed_ips: e.allowed_ips, + deny_rules: e + .deny_rules + .into_iter() + .map(|d| L7DenyRule { + method: d.method, + path: d.path, + command: d.command, + query: d + .query + .into_iter() + .map(|(key, matcher)| { + let proto = match matcher { + QueryMatcherDef::Glob(glob) => { + L7QueryMatcher { glob, any: vec![] } + } + QueryMatcherDef::Any(any) => L7QueryMatcher { + glob: String::new(), + any: any.any, + }, + }; + (key, proto) + }) + .collect(), + }) + .collect(), } }) .collect(), @@ -330,6 +370,29 @@ fn from_proto(policy: &SandboxPolicy) -> PolicyFile { }) .collect(), allowed_ips: e.allowed_ips.clone(), + deny_rules: e + .deny_rules + .iter() + .map(|d| L7DenyRuleDef { + method: d.method.clone(), + path: d.path.clone(), + command: d.command.clone(), + query: d + .query + .iter() + .map(|(key, matcher)| { + let yaml_matcher = if !matcher.any.is_empty() { + QueryMatcherDef::Any(QueryAnyDef { + any: matcher.any.clone(), + }) + } else { + QueryMatcherDef::Glob(matcher.glob.clone()) + }; + (key.clone(), yaml_matcher) + }) + .collect(), + }) + .collect(), } }) .collect(), @@ -1323,6 +1386,113 @@ network_policies: ); } + #[test] + fn parse_deny_rules_from_yaml() { + let yaml = r#" +version: 1 +network_policies: + github: + name: github + endpoints: + - host: api.github.com + port: 443 + protocol: rest + access: read-write + deny_rules: + - method: POST + path: "/repos/*/pulls/*/reviews" + - method: PUT + path: "/repos/*/branches/*/protection" + binaries: + - path: /usr/bin/curl +"#; + let proto = parse_sandbox_policy(yaml).expect("parse failed"); + let ep = &proto.network_policies["github"].endpoints[0]; + assert_eq!(ep.deny_rules.len(), 2); + assert_eq!(ep.deny_rules[0].method, "POST"); + assert_eq!(ep.deny_rules[0].path, "/repos/*/pulls/*/reviews"); + assert_eq!(ep.deny_rules[1].method, "PUT"); + assert_eq!(ep.deny_rules[1].path, "/repos/*/branches/*/protection"); + } + + #[test] + fn round_trip_preserves_deny_rules() { + let yaml = r#" +version: 1 +network_policies: + github: + name: github + endpoints: + - host: api.github.com + port: 443 + protocol: rest + access: full + deny_rules: + - method: POST + path: "/repos/*/pulls/*/reviews" + - method: DELETE + path: "/repos/*/branches/*/protection" + query: + force: "true" + binaries: + - path: /usr/bin/curl +"#; + let proto1 = parse_sandbox_policy(yaml).expect("parse failed"); + let yaml_out = serialize_sandbox_policy(&proto1).expect("serialize failed"); + let proto2 = parse_sandbox_policy(&yaml_out).expect("re-parse failed"); + + let ep1 = &proto1.network_policies["github"].endpoints[0]; + let ep2 = &proto2.network_policies["github"].endpoints[0]; + assert_eq!(ep1.deny_rules.len(), ep2.deny_rules.len()); + assert_eq!(ep2.deny_rules[0].method, "POST"); + assert_eq!(ep2.deny_rules[0].path, "/repos/*/pulls/*/reviews"); + assert_eq!(ep2.deny_rules[1].method, "DELETE"); + assert_eq!(ep2.deny_rules[1].query["force"].glob, "true"); + } + + #[test] + fn parse_deny_rules_with_query_any() { + let yaml = r#" +version: 1 +network_policies: + test: + name: test + endpoints: + - host: api.example.com + port: 443 + protocol: rest + access: full + deny_rules: + - method: POST + path: /action + query: + type: + any: ["admin-*", "root-*"] + binaries: + - path: /usr/bin/curl +"#; + let proto = parse_sandbox_policy(yaml).expect("parse failed"); + let deny = &proto.network_policies["test"].endpoints[0].deny_rules[0]; + assert_eq!(deny.query["type"].any, vec!["admin-*", "root-*"]); + } + + #[test] + fn parse_rejects_unknown_fields_in_deny_rule() { + let yaml = r#" +version: 1 +network_policies: + test: + endpoints: + - host: example.com + port: 443 + deny_rules: + - method: POST + path: /foo + bogus: true +"#; + assert!(parse_sandbox_policy(yaml).is_err()); + } + #[test] fn rejects_port_above_65535() { let yaml = r#" diff --git a/crates/openshell-sandbox/data/sandbox-policy.rego b/crates/openshell-sandbox/data/sandbox-policy.rego index ce4400a66..513f618a8 100644 --- a/crates/openshell-sandbox/data/sandbox-policy.rego +++ b/crates/openshell-sandbox/data/sandbox-policy.rego @@ -183,19 +183,86 @@ _policy_allows_l7(policy) if { request_allowed_for_endpoint(input.request, ep) } -# L7 request allowed if any matching L4 policy also allows the L7 request. +# L7 request allowed if any matching L4 policy also allows the L7 request +# AND no deny rule blocks it. Deny rules take precedence over allow rules. allow_request if { some name policy := data.network_policies[name] endpoint_allowed(policy, input.network) binary_allowed(policy, input.exec) _policy_allows_l7(policy) + not deny_request +} + +# --- L7 deny rules --- +# +# Deny rules are evaluated after allow rules and take precedence. +# If a request matches any deny rule on any matching endpoint, it is blocked +# even if it would otherwise be allowed. + +default deny_request = false + +# Per-policy helper: true when this policy has at least one endpoint matching +# the L4 request whose deny_rules also match the specific L7 request. +_policy_denies_l7(policy) if { + some ep + ep := policy.endpoints[_] + endpoint_matches_request(ep, input.network) + request_denied_for_endpoint(input.request, ep) +} + +deny_request if { + some name + policy := data.network_policies[name] + endpoint_allowed(policy, input.network) + binary_allowed(policy, input.exec) + _policy_denies_l7(policy) +} + +# --- L7 deny rule matching: REST method + path + query --- + +request_denied_for_endpoint(request, endpoint) if { + some deny_rule + deny_rule := endpoint.deny_rules[_] + deny_rule.method + method_matches(request.method, deny_rule.method) + path_matches(request.path, deny_rule.path) + deny_query_params_match(request, deny_rule) +} + +# --- L7 deny rule matching: SQL command --- + +request_denied_for_endpoint(request, endpoint) if { + some deny_rule + deny_rule := endpoint.deny_rules[_] + deny_rule.command + command_matches(request.command, deny_rule.command) +} + +# Deny query matching: if no query rules on deny, match any query params. +# If query rules present, all configured keys must match. +deny_query_params_match(request, deny_rule) if { + deny_query_rules := object.get(deny_rule, "query", {}) + not deny_query_mismatch(request, deny_query_rules) +} + +deny_query_mismatch(request, query_rules) if { + some key + matcher := query_rules[key] + not query_key_matches(request, key, matcher) } # --- L7 deny reason --- request_deny_reason := reason if { input.request + deny_request + reason := sprintf("%s %s blocked by deny rule", [input.request.method, input.request.path]) +} + +request_deny_reason := reason if { + input.request + not deny_request not allow_request reason := sprintf("%s %s not permitted by policy", [input.request.method, input.request.path]) } diff --git a/crates/openshell-sandbox/src/l7/mod.rs b/crates/openshell-sandbox/src/l7/mod.rs index af403aead..0ccbb617b 100644 --- a/crates/openshell-sandbox/src/l7/mod.rs +++ b/crates/openshell-sandbox/src/l7/mod.rs @@ -331,6 +331,114 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< // The old warning about missing `tls: terminate` is no longer needed // because TLS termination is now automatic. + // Validate deny_rules + let has_deny_rules = ep + .get("deny_rules") + .and_then(|v| v.as_array()) + .is_some_and(|a| !a.is_empty()); + if has_deny_rules { + // deny_rules require L7 inspection + if protocol.is_empty() { + errors.push(format!( + "{loc}: deny_rules require protocol (L7 inspection must be enabled)" + )); + } + + // deny_rules require some allow base (access or rules) + if !has_rules && access.is_empty() { + errors.push(format!( + "{loc}: deny_rules require rules or access to define the base allow set" + )); + } + + if let Some(deny_rules) = ep.get("deny_rules").and_then(|v| v.as_array()) { + for (deny_idx, deny_rule) in deny_rules.iter().enumerate() { + let deny_loc = format!("{loc}.deny_rules[{deny_idx}]"); + + // Validate method + if let Some(method) = deny_rule.get("method").and_then(|m| m.as_str()) + && !method.is_empty() + && protocol == "rest" + { + let valid_methods = [ + "GET", "HEAD", "POST", "PUT", "DELETE", "PATCH", "OPTIONS", "*", + ]; + if !valid_methods.contains(&method.to_ascii_uppercase().as_str()) { + warnings.push(format!( + "{deny_loc}: Unknown HTTP method '{method}'. Standard methods: GET, HEAD, POST, PUT, DELETE, PATCH, OPTIONS." + )); + } + } + + // Validate path glob syntax + if let Some(path) = deny_rule.get("path").and_then(|p| p.as_str()) { + if let Some(warning) = check_glob_syntax(path) { + warnings.push(format!("{deny_loc}.path: {warning}")); + } + } + + // Validate query matchers (same rules as allow query matchers) + if let Some(query) = deny_rule.get("query").filter(|v| !v.is_null()) { + if let Some(query_obj) = query.as_object() { + for (param, matcher) in query_obj { + if let Some(glob_str) = matcher.as_str() { + if let Some(warning) = check_glob_syntax(glob_str) { + warnings.push(format!( + "{deny_loc}.query.{param}: {warning}" + )); + } + continue; + } + if let Some(matcher_obj) = matcher.as_object() { + let has_any = matcher_obj.get("any").is_some(); + let has_glob = matcher_obj.get("glob").is_some(); + let has_unknown = + matcher_obj.keys().any(|k| k != "any" && k != "glob"); + if has_unknown { + errors.push(format!( + "{deny_loc}.query.{param}: unknown matcher keys; only `glob` or `any` are supported" + )); + } else if has_glob && has_any { + errors.push(format!( + "{deny_loc}.query.{param}: matcher cannot specify both `glob` and `any`" + )); + } else if !has_glob && !has_any { + errors.push(format!( + "{deny_loc}.query.{param}: object matcher requires `glob` string or non-empty `any` list" + )); + } + } + } + } else { + errors.push(format!( + "{deny_loc}.query: expected map of query matchers" + )); + } + } + + // SQL command validation + if let Some(command) = deny_rule.get("command").and_then(|c| c.as_str()) { + if !command.is_empty() && protocol == "rest" { + warnings.push(format!( + "{deny_loc}: command is for SQL protocol, not REST" + )); + } + } + } + } + } + + // Empty deny_rules list (explicitly set but empty) + if ep + .get("deny_rules") + .and_then(|v| v.as_array()) + .is_some_and(Vec::is_empty) + { + errors.push(format!( + "{loc}: deny_rules list cannot be empty (would have no effect). Remove it if no denials are needed." + )); + } + // Validate HTTP methods in rules if has_rules && protocol == "rest" { let valid_methods = [ diff --git a/crates/openshell-sandbox/src/opa.rs b/crates/openshell-sandbox/src/opa.rs index 32f1d6c19..673ad2fed 100644 --- a/crates/openshell-sandbox/src/opa.rs +++ b/crates/openshell-sandbox/src/opa.rs @@ -874,6 +874,43 @@ fn proto_to_opa_data_json(proto: &ProtoSandboxPolicy, entrypoint_pid: u32) -> St if !e.allowed_ips.is_empty() { ep["allowed_ips"] = e.allowed_ips.clone().into(); } + if !e.deny_rules.is_empty() { + let deny_rules: Vec = e + .deny_rules + .iter() + .map(|d| { + let mut deny = serde_json::json!({}); + if !d.method.is_empty() { + deny["method"] = d.method.clone().into(); + } + if !d.path.is_empty() { + deny["path"] = d.path.clone().into(); + } + if !d.command.is_empty() { + deny["command"] = d.command.clone().into(); + } + let query: serde_json::Map = d + .query + .iter() + .map(|(key, matcher)| { + let mut matcher_json = serde_json::json!({}); + if !matcher.glob.is_empty() { + matcher_json["glob"] = matcher.glob.clone().into(); + } + if !matcher.any.is_empty() { + matcher_json["any"] = matcher.any.clone().into(); + } + (key.clone(), matcher_json) + }) + .collect(); + if !query.is_empty() { + deny["query"] = query.into(); + } + deny + }) + .collect(); + ep["deny_rules"] = deny_rules.into(); + } ep }) .collect(); @@ -1934,6 +1971,242 @@ process: assert_eq!(val, regorus::Value::from(true)); } + // ======================================================================== + // Deny rules tests + // ======================================================================== + + const L7_DENY_TEST_DATA: &str = r#" +network_policies: + github_api: + name: github_api + endpoints: + - host: api.github.com + port: 443 + protocol: rest + enforcement: enforce + access: read-write + deny_rules: + - method: POST + path: "/repos/*/pulls/*/reviews" + - method: PUT + path: "/repos/*/branches/*/protection" + - method: "*" + path: "/repos/*/rulesets" + binaries: + - { path: /usr/bin/curl } + deny_with_query: + name: deny_with_query + endpoints: + - host: api.restricted.com + port: 443 + protocol: rest + enforcement: enforce + access: full + deny_rules: + - method: POST + path: "/admin/**" + query: + force: "true" + 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 +"#; + + fn l7_deny_engine() -> OpaEngine { + OpaEngine::from_strings(TEST_POLICY, L7_DENY_TEST_DATA) + .expect("Failed to load deny test data") + } + + #[test] + fn l7_deny_rule_blocks_allowed_method_path() { + let engine = l7_deny_engine(); + // POST to reviews is allowed by read-write preset but denied by deny rule + let input = l7_input( + "api.github.com", + 443, + "POST", + "/repos/myorg/pulls/123/reviews", + ); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(false), + "deny rule should block POST to reviews" + ); + } + + #[test] + fn l7_deny_rule_allows_non_matching_requests() { + let engine = l7_deny_engine(); + // GET repos/issues is allowed and not denied + let input = l7_input("api.github.com", 443, "GET", "/repos/myorg/issues"); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(true), + "non-denied GET should be allowed" + ); + } + + #[test] + fn l7_deny_rule_allows_same_method_different_path() { + let engine = l7_deny_engine(); + // POST to issues is allowed (deny only targets reviews) + let input = l7_input("api.github.com", 443, "POST", "/repos/myorg/issues"); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(true), + "POST to issues should be allowed" + ); + } + + #[test] + fn l7_deny_rule_blocks_wildcard_method() { + let engine = l7_deny_engine(); + // GET /repos/myorg/rulesets should be denied (method: "*") + let input = l7_input("api.github.com", 443, "GET", "/repos/myorg/rulesets"); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(false), + "wildcard method deny should block GET" + ); + } + + #[test] + fn l7_deny_rule_blocks_put_protection() { + let engine = l7_deny_engine(); + let input = l7_input( + "api.github.com", + 443, + "PUT", + "/repos/myorg/branches/main/protection", + ); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(false), + "PUT to branch protection should be denied" + ); + } + + #[test] + fn l7_deny_reason_populated_when_deny_rule_matches() { + let engine = l7_deny_engine(); + let input = l7_input( + "api.github.com", + 443, + "POST", + "/repos/myorg/pulls/123/reviews", + ); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.request_deny_reason".into()) + .unwrap(); + let reason = match val { + regorus::Value::String(s) => s.to_string(), + _ => String::new(), + }; + assert!( + reason.contains("deny rule"), + "Expected deny rule reason, got: {reason}" + ); + } + + #[test] + fn l7_deny_rule_with_query_blocks_matching_params() { + let engine = l7_deny_engine(); + // POST /admin/settings with force=true should be denied + let input = l7_input_with_query( + "api.restricted.com", + 443, + "POST", + "/admin/settings", + serde_json::json!({"force": ["true"]}), + ); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(false), + "deny with matching query should block" + ); + } + + #[test] + fn l7_deny_rule_with_query_allows_non_matching_params() { + let engine = l7_deny_engine(); + // POST /admin/settings with force=false should be allowed (query doesn't match deny) + let input = l7_input_with_query( + "api.restricted.com", + 443, + "POST", + "/admin/settings", + serde_json::json!({"force": ["false"]}), + ); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(true), + "deny with non-matching query should allow" + ); + } + + #[test] + fn l7_deny_rule_without_matching_query_key_allows() { + let engine = l7_deny_engine(); + // POST /admin/settings with no query params -- deny rule has query.force=true, + // so no match (key not present) and request should be allowed + let input = l7_input("api.restricted.com", 443, "POST", "/admin/settings"); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(true), + "deny without matching query key should allow" + ); + } + // ======================================================================== // Overlapping policies (duplicate host:port) — regression tests // ======================================================================== diff --git a/docs/reference/policy-schema.mdx b/docs/reference/policy-schema.mdx index e9795475d..3e505cf3e 100644 --- a/docs/reference/policy-schema.mdx +++ b/docs/reference/policy-schema.mdx @@ -159,6 +159,7 @@ Each endpoint defines a reachable destination and optional inspection rules. | `enforcement` | string | No | `enforce` actively blocks disallowed requests. `audit` logs violations but allows traffic through. | | `access` | string | No | HTTP access level. One of `read-only`, `read-write`, or `full`. Mutually exclusive with `rules`. | | `rules` | list of rule objects | No | Fine-grained per-method, per-path allow rules. Mutually exclusive with `access`. | +| `deny_rules` | list of deny rule objects | No | L7 deny rules that block specific requests even when allowed by `access` or `rules`. Deny rules take precedence over allow rules. | | `allowed_ips` | list of string | No | CIDR or IP allowlist for SSRF override. Entries overlapping loopback (`127.0.0.0/8`), link-local (`169.254.0.0/16`), or unspecified (`0.0.0.0`) are rejected at load time. | #### Access Levels @@ -198,6 +199,38 @@ rules: any: ["v1.*", "v2.*"] ``` +#### Deny Rule Object + +Blocks specific operations on endpoints that otherwise have broad access. Each deny rule +matches a method and path (and optionally query parameters or SQL commands). Deny rules are +evaluated after allow rules and take precedence -- if a request matches any deny rule, it is +blocked regardless of what the allow rules or access preset permit. + +| Field | Type | Required | Description | +|---|---|---|---| +| `method` | string | No | HTTP method to deny (for example, `POST`, `DELETE`). `*` matches any method. | +| `path` | string | No | URL path pattern. Same glob syntax as allow rules. | +| `command` | string | No | SQL command to deny (`SELECT`, `INSERT`, etc.). For `protocol: sql` only. | +| `query` | map | No | Query parameter matchers. Same syntax as allow rule `query`. | + +Example -- grant read-write access to GitHub but block admin operations: + +```yaml showLineNumbers={false} +endpoints: + - host: api.github.com + port: 443 + protocol: rest + enforcement: enforce + access: read-write + deny_rules: + - method: POST + path: "/repos/*/pulls/*/reviews" + - method: PUT + path: "/repos/*/branches/*/protection" + - method: "*" + path: "/repos/*/rulesets" +``` + ### Binary Object Identifies an executable that is permitted to use the associated endpoints. diff --git a/proto/sandbox.proto b/proto/sandbox.proto index 61948a527..e9b81363e 100644 --- a/proto/sandbox.proto +++ b/proto/sandbox.proto @@ -85,6 +85,25 @@ message NetworkEndpoint { // If `port` is set and `ports` is empty, `port` is normalized to `ports: [port]`. // If both are set, `ports` takes precedence. repeated uint32 ports = 9; + // Explicit L7 deny rules. When present, requests matching any deny rule + // are blocked even if they match an allow rule or access preset. + // Deny rules take precedence over allow rules. + repeated L7DenyRule deny_rules = 10; +} + +// An L7 deny rule that blocks specific requests. +// Mirrors L7Allow — same fields, same matching semantics, inverted effect. +// Deny rules are evaluated after allow rules and take precedence. +message L7DenyRule { + // HTTP method (REST): GET, POST, etc. or "*" for any. + string method = 1; + // URL path glob pattern (REST): "/repos/*/pulls/*/reviews", "**" for any. + string path = 2; + // SQL command (SQL): SELECT, INSERT, etc. or "*" for any. + string command = 3; + // Query parameter matcher map (REST). + // Same semantics as L7Allow.query. + map query = 4; } // An L7 policy rule (allow-only). From e016129d6641d57e18d0e9d830910f89fb2b347c Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Mon, 13 Apr 2026 13:52:17 -0700 Subject: [PATCH 2/2] fix(policy): deny query matching fails closed, mirror allow-side validation Addresses PR review findings P1 and P3: P1: Deny-side query matching now uses fail-closed semantics. If ANY value for a query key matches the deny matcher, the deny fires. The previous implementation reused allow-side "all values must match" logic which allowed ?force=true&force=false to bypass a deny on force=true. P3: Deny-side query validation now mirrors the full allow-side checks: empty any lists, non-string matcher values, glob+any mutual exclusion, glob type checks, and glob syntax warnings are all validated. --- .../data/sandbox-policy.rego | 42 ++- crates/openshell-sandbox/src/l7/mod.rs | 307 ++++++++++++++++-- crates/openshell-sandbox/src/opa.rs | 25 ++ 3 files changed, 340 insertions(+), 34 deletions(-) diff --git a/crates/openshell-sandbox/data/sandbox-policy.rego b/crates/openshell-sandbox/data/sandbox-policy.rego index 513f618a8..939c2e5cb 100644 --- a/crates/openshell-sandbox/data/sandbox-policy.rego +++ b/crates/openshell-sandbox/data/sandbox-policy.rego @@ -239,17 +239,49 @@ request_denied_for_endpoint(request, endpoint) if { command_matches(request.command, deny_rule.command) } -# Deny query matching: if no query rules on deny, match any query params. -# If query rules present, all configured keys must match. +# Deny query matching: fail-closed semantics. +# If no query rules on the deny rule, match unconditionally (any query params). +# If query rules present, trigger the deny if ANY value for a configured key +# matches the matcher. This is the inverse of allow-side semantics where ALL +# values must match. For deny logic, a single matching value is enough to block. deny_query_params_match(request, deny_rule) if { deny_query_rules := object.get(deny_rule, "query", {}) - not deny_query_mismatch(request, deny_query_rules) + count(deny_query_rules) == 0 } -deny_query_mismatch(request, query_rules) if { +deny_query_params_match(request, deny_rule) if { + deny_query_rules := object.get(deny_rule, "query", {}) + count(deny_query_rules) > 0 + not deny_query_key_missing(request, deny_query_rules) + not deny_query_value_mismatch_all(request, deny_query_rules) +} + +# A configured deny query key is missing from the request entirely. +# Missing key means the deny rule doesn't apply (fail-open on absence). +deny_query_key_missing(request, query_rules) if { + some key + query_rules[key] + request_query := object.get(request, "query_params", {}) + values := object.get(request_query, key, null) + values == null +} + +# ALL values for a configured key fail to match the matcher. +# If even one value matches, deny fires. This rule checks the opposite: +# true when NO value matches (i.e., every value is a mismatch). +deny_query_value_mismatch_all(request, query_rules) if { some key matcher := query_rules[key] - not query_key_matches(request, key, matcher) + request_query := object.get(request, "query_params", {}) + values := object.get(request_query, key, []) + count(values) > 0 + not deny_any_value_matches(values, matcher) +} + +# True if at least one value in the list matches the matcher. +deny_any_value_matches(values, matcher) if { + some i + query_value_matches(values[i], matcher) } # --- L7 deny reason --- diff --git a/crates/openshell-sandbox/src/l7/mod.rs b/crates/openshell-sandbox/src/l7/mod.rs index 0ccbb617b..d354f562b 100644 --- a/crates/openshell-sandbox/src/l7/mod.rs +++ b/crates/openshell-sandbox/src/l7/mod.rs @@ -377,42 +377,102 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< } } - // Validate query matchers (same rules as allow query matchers) + // Validate query matchers — mirrors allow-side validation exactly if let Some(query) = deny_rule.get("query").filter(|v| !v.is_null()) { - if let Some(query_obj) = query.as_object() { - for (param, matcher) in query_obj { - if let Some(glob_str) = matcher.as_str() { - if let Some(warning) = check_glob_syntax(glob_str) { - warnings.push(format!( - "{deny_loc}.query.{param}: {warning}" - )); - } - continue; + let Some(query_obj) = query.as_object() else { + errors.push(format!( + "{deny_loc}.query: expected map of query matchers" + )); + continue; + }; + + for (param, matcher) in query_obj { + if let Some(glob_str) = matcher.as_str() { + if let Some(warning) = check_glob_syntax(glob_str) { + warnings + .push(format!("{deny_loc}.query.{param}: {warning}")); } - if let Some(matcher_obj) = matcher.as_object() { - let has_any = matcher_obj.get("any").is_some(); - let has_glob = matcher_obj.get("glob").is_some(); - let has_unknown = - matcher_obj.keys().any(|k| k != "any" && k != "glob"); - if has_unknown { - errors.push(format!( - "{deny_loc}.query.{param}: unknown matcher keys; only `glob` or `any` are supported" - )); - } else if has_glob && has_any { - errors.push(format!( - "{deny_loc}.query.{param}: matcher cannot specify both `glob` and `any`" - )); - } else if !has_glob && !has_any { + continue; + } + + let Some(matcher_obj) = matcher.as_object() else { + errors.push(format!( + "{deny_loc}.query.{param}: expected string glob or object with `any`" + )); + continue; + }; + + let has_any = matcher_obj.get("any").is_some(); + let has_glob = matcher_obj.get("glob").is_some(); + let has_unknown = + matcher_obj.keys().any(|k| k != "any" && k != "glob"); + if has_unknown { + errors.push(format!( + "{deny_loc}.query.{param}: unknown matcher keys; only `glob` or `any` are supported" + )); + continue; + } + + if has_glob && has_any { + errors.push(format!( + "{deny_loc}.query.{param}: matcher cannot specify both `glob` and `any`" + )); + continue; + } + + if !has_glob && !has_any { + errors.push(format!( + "{deny_loc}.query.{param}: object matcher requires `glob` string or non-empty `any` list" + )); + continue; + } + + if has_glob { + match matcher_obj.get("glob").and_then(|v| v.as_str()) { + None => { errors.push(format!( - "{deny_loc}.query.{param}: object matcher requires `glob` string or non-empty `any` list" + "{deny_loc}.query.{param}.glob: expected glob string" )); } + Some(g) => { + if let Some(warning) = check_glob_syntax(g) { + warnings.push(format!( + "{deny_loc}.query.{param}.glob: {warning}" + )); + } + } + } + continue; + } + + let any = matcher_obj.get("any").and_then(|v| v.as_array()); + let Some(any) = any else { + errors.push(format!( + "{deny_loc}.query.{param}.any: expected array of glob strings" + )); + continue; + }; + + if any.is_empty() { + errors.push(format!( + "{deny_loc}.query.{param}.any: list must not be empty" + )); + continue; + } + + if any.iter().any(|v| v.as_str().is_none()) { + errors.push(format!( + "{deny_loc}.query.{param}.any: all values must be strings" + )); + } + + for item in any.iter().filter_map(|v| v.as_str()) { + if let Some(warning) = check_glob_syntax(item) { + warnings.push(format!( + "{deny_loc}.query.{param}.any: {warning}" + )); } } - } else { - errors.push(format!( - "{deny_loc}.query: expected map of query matchers" - )); } } @@ -1269,4 +1329,193 @@ mod tests { "valid query matcher shapes should not error: {errors:?}" ); } + + // --- Deny rules validation tests --- + + #[test] + fn validate_deny_rules_require_protocol() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "deny_rules": [{ "method": "POST", "path": "/admin" }] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors + .iter() + .any(|e| e.contains("deny_rules require protocol")), + "should require protocol for deny_rules: {errors:?}" + ); + } + + #[test] + fn validate_deny_rules_require_allow_base() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "deny_rules": [{ "method": "POST", "path": "/admin" }] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors + .iter() + .any(|e| e.contains("deny_rules require rules or access")), + "should require rules or access for deny_rules: {errors:?}" + ); + } + + #[test] + fn validate_deny_rules_empty_list_rejected() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "access": "full", + "deny_rules": [] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors + .iter() + .any(|e| e.contains("deny_rules list cannot be empty")), + "should reject empty deny_rules: {errors:?}" + ); + } + + #[test] + fn validate_deny_rules_valid_config_accepted() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "access": "read-write", + "deny_rules": [ + { "method": "POST", "path": "/repos/*/pulls/*/reviews" }, + { "method": "PUT", "path": "/repos/*/branches/*/protection" } + ] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "valid deny_rules should not error: {errors:?}" + ); + } + + #[test] + fn validate_deny_rules_query_empty_any_rejected() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "access": "full", + "deny_rules": [{ + "method": "POST", + "path": "/admin", + "query": { "type": { "any": [] } } + }] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors + .iter() + .any(|e| e.contains("any: list must not be empty")), + "should reject empty any list in deny query: {errors:?}" + ); + } + + #[test] + fn validate_deny_rules_query_non_string_rejected() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "access": "full", + "deny_rules": [{ + "method": "POST", + "path": "/admin", + "query": { "force": 123 } + }] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors + .iter() + .any(|e| e.contains("expected string glob or object")), + "should reject non-string/non-object matcher in deny query: {errors:?}" + ); + } + + #[test] + fn validate_deny_rules_query_valid_matchers_accepted() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "access": "full", + "deny_rules": [{ + "method": "POST", + "path": "/admin/**", + "query": { + "force": "true", + "type": { "any": ["admin-*", "root-*"] }, + "scope": { "glob": "org-*" } + } + }] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "valid deny query matchers should not error: {errors:?}" + ); + } } diff --git a/crates/openshell-sandbox/src/opa.rs b/crates/openshell-sandbox/src/opa.rs index 673ad2fed..0069bcc3e 100644 --- a/crates/openshell-sandbox/src/opa.rs +++ b/crates/openshell-sandbox/src/opa.rs @@ -2189,6 +2189,31 @@ process: ); } + #[test] + fn l7_deny_rule_with_query_blocks_when_any_value_matches() { + let engine = l7_deny_engine(); + // POST /admin/settings with force=true&force=false should STILL be denied + // because at least one value ("true") matches the deny rule. + // This is fail-closed: any matching value triggers the deny. + let input = l7_input_with_query( + "api.restricted.com", + 443, + "POST", + "/admin/settings", + serde_json::json!({"force": ["true", "false"]}), + ); + let mut eng = engine.engine.lock().unwrap(); + eng.set_input_json(&input.to_string()).unwrap(); + let val = eng + .eval_rule("data.openshell.sandbox.allow_request".into()) + .unwrap(); + assert_eq!( + val, + regorus::Value::from(false), + "deny should fire when ANY value matches, even with mixed values" + ); + } + #[test] fn l7_deny_rule_without_matching_query_key_allows() { let engine = l7_deny_engine();