From 3be623ab1c8d16d70e9763318aeb9c97878ce063 Mon Sep 17 00:00:00 2001 From: Grace Smith Date: Wed, 22 Jul 2026 10:48:28 +0100 Subject: [PATCH] refactor(policy): extract shared L7 endpoint validation Move L7 endpoint semantic checks into the openshell-policy crate so both profile lint and the runtime validator share one implementation. This eliminates drift between the two validation paths. The shared validator covers 9 checks: unknown protocol, rules/access mutual exclusivity, JSON-RPC family access rejection, json-rpc requires rules, non-JSON-RPC protocol requires rules or access, MCP requires rules when allow_all is false, rules-would-deny-all detection, deny_rules require protocol, and deny_rules require base allow set. Changes rules/deny_rules fields to Option> so absent vs empty is distinguishable at lint time. Adds is_effectively_empty() to L7AllowProfile for deny-all detection of allow: {} objects. Makes rules_would_deny_all MCP-aware by checking tool/params.name selectors before classifying a rule as deny-all. Adds params field to L7AllowProfile so MCP tool selectors survive proto round-trip. Signed-off-by: Grace Smith Signed-off-by: Grace Smith --- Cargo.lock | 1 + crates/openshell-cli/src/run.rs | 11 + crates/openshell-policy/src/l7_validate.rs | 427 ++++ crates/openshell-policy/src/lib.rs | 2 + crates/openshell-providers/Cargo.toml | 1 + crates/openshell-providers/src/profiles.rs | 2061 ++++++++++++++++- crates/openshell-server/src/grpc/policy.rs | 1 + crates/openshell-server/src/grpc/provider.rs | 3 + .../src/l7/mod.rs | 337 ++- docs/reference/policy-schema.mdx | 10 + docs/sandboxes/providers-v2.mdx | 1 - 11 files changed, 2692 insertions(+), 163 deletions(-) create mode 100644 crates/openshell-policy/src/l7_validate.rs diff --git a/Cargo.lock b/Cargo.lock index b41246761..2a13197c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4074,6 +4074,7 @@ version = "0.0.0" dependencies = [ "glob", "openshell-core", + "openshell-policy", "serde", "serde_json", "serde_yml", diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index d9b2df10b..50b6d6258 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -5732,6 +5732,17 @@ fn load_profile_import_item( } .map_err(|err| profile_file_diagnostic(&source, err.to_string()))?; + let pre_lower = profile.validate_before_lowering(&source); + if let Some(diag) = pre_lower.into_iter().find(|d| d.severity == "error") { + return Err(ProviderProfileDiagnostic { + source: diag.source, + profile_id: diag.profile_id, + field: diag.field, + message: diag.message, + severity: diag.severity, + }); + } + Ok(ProviderProfileImportItem { profile: Some(profile.to_proto()), source, diff --git a/crates/openshell-policy/src/l7_validate.rs b/crates/openshell-policy/src/l7_validate.rs new file mode 100644 index 000000000..0701afa0a --- /dev/null +++ b/crates/openshell-policy/src/l7_validate.rs @@ -0,0 +1,427 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Shared L7 endpoint semantic validation. +//! +//! Both profile lint (`openshell-providers`) and the runtime policy +//! validator (`openshell-supervisor-network`) call +//! [`validate_l7_endpoint_semantics`] to enforce the same constraints on +//! L7 endpoint field combinations, preventing drift between lint-time +//! and runtime checks. + +/// Known L7 inspection protocols. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum L7Protocol { + Rest, + Websocket, + Graphql, + Sql, + JsonRpc, + Mcp, +} + +impl L7Protocol { + /// Parse a protocol string into a known variant. + pub fn parse(s: &str) -> Option { + match s { + "rest" => Some(Self::Rest), + "websocket" => Some(Self::Websocket), + "graphql" => Some(Self::Graphql), + "sql" => Some(Self::Sql), + "json-rpc" => Some(Self::JsonRpc), + "mcp" => Some(Self::Mcp), + _ => None, + } + } + + /// Returns `true` for protocols in the JSON-RPC family (`json-rpc`, + /// `mcp`). + pub fn is_jsonrpc_family(self) -> bool { + matches!(self, Self::JsonRpc | Self::Mcp) + } +} + +/// Fields extracted from an endpoint definition needed for L7 semantic +/// validation. Both profile lint and the runtime validator construct this +/// from their own data representation. +#[allow(clippy::struct_excessive_bools)] +pub struct L7EndpointFields<'a> { + /// Protocol string as authored (e.g. `"rest"`, `"mcp"`). Empty + /// string means no L7 protocol was specified. + pub protocol: &'a str, + + /// Access preset string (e.g. `"read-only"`, `"full"`). Empty string + /// means no access preset. + pub access: &'a str, + + /// `true` when the endpoint has a non-empty rules list. + pub has_rules: bool, + + /// `true` when the endpoint has a non-empty `deny_rules` list. + pub has_deny_rules: bool, + + /// `true` when rules are present (non-empty) but would deny all + /// traffic because every entry lacks an allow clause. + pub rules_would_deny_all: bool, + + /// Value of `mcp.allow_all_known_mcp_methods` (defaults to `false`). + pub allow_all_known_mcp_methods: bool, +} + +/// Validate the semantic consistency of an L7 endpoint's field +/// combination. +/// +/// Returns a list of error message strings. An empty list means the +/// endpoint passes validation. Messages are bare — callers prepend +/// their own location context. +pub fn validate_l7_endpoint_semantics(ep: &L7EndpointFields<'_>) -> Vec { + let mut errors = Vec::new(); + let protocol = ep.protocol; + let l7_protocol = L7Protocol::parse(protocol); + let jsonrpc_family = l7_protocol.is_some_and(L7Protocol::is_jsonrpc_family); + let is_mcp = matches!(l7_protocol, Some(L7Protocol::Mcp)); + let is_jsonrpc = matches!(l7_protocol, Some(L7Protocol::JsonRpc)); + + // 1. Unknown protocol + if !protocol.is_empty() && l7_protocol.is_none() { + errors.push(format!( + "unknown protocol '{protocol}' (expected rest, websocket, graphql, sql, json-rpc, or mcp)" + )); + } + + // 2. rules + access mutually exclusive + if ep.has_rules && !ep.access.is_empty() { + errors.push("rules and access are mutually exclusive".to_string()); + } + + // 3. JSON-RPC family cannot use access presets + if jsonrpc_family && !ep.access.is_empty() { + if is_mcp { + errors.push(format!( + "protocol {protocol} does not support access presets; \ + use rules/deny_rules or set mcp.allow_all_known_mcp_methods: true \ + for an allow-all MCP policy" + )); + } else { + errors.push(format!( + "protocol {protocol} does not support access presets; \ + use explicit rules with allow.method such as \"*\"" + )); + } + } + + // 4. json-rpc requires explicit rules + if is_jsonrpc && !ep.has_rules && ep.access.is_empty() { + errors.push(format!( + "protocol {protocol} requires explicit rules with allow.method" + )); + } + + // 5. Non-MCP, non-JSON-RPC protocol requires rules or access (JSON-RPC's + // dedicated message is emitted by rule 4). + if !protocol.is_empty() && !is_mcp && !is_jsonrpc && !ep.has_rules && ep.access.is_empty() { + errors.push("protocol requires rules or access to define allowed traffic".to_string()); + } + + // 6. MCP requires rules when allow_all_known_mcp_methods is false + if is_mcp && !ep.has_rules && ep.access.is_empty() && !ep.allow_all_known_mcp_methods { + errors.push( + "protocol mcp requires rules when mcp.allow_all_known_mcp_methods is false".to_string(), + ); + } + + // 7. Rules would deny all traffic + if ep.rules_would_deny_all { + errors.push( + "rules would deny all traffic (no allow clause found). \ + Use `access: full` or add allow clauses to rules." + .to_string(), + ); + } + + // 8. deny_rules require protocol + if ep.has_deny_rules && protocol.is_empty() { + errors.push("deny_rules require protocol (L7 inspection must be enabled)".to_string()); + } + + // 9. deny_rules require base allow set + if ep.has_deny_rules && !is_mcp && !ep.has_rules && ep.access.is_empty() { + errors.push("deny_rules require rules or access to define the base allow set".to_string()); + } + + errors +} + +#[cfg(test)] +mod tests { + use super::*; + + fn valid_rest_endpoint() -> L7EndpointFields<'static> { + L7EndpointFields { + protocol: "rest", + access: "read-only", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + } + } + + #[test] + fn valid_endpoint_produces_no_errors() { + let errors = validate_l7_endpoint_semantics(&valid_rest_endpoint()); + assert!(errors.is_empty(), "expected no errors, got: {errors:?}"); + } + + #[test] + fn rejects_unknown_protocol() { + let ep = L7EndpointFields { + protocol: "ftp", + access: "", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!(errors.iter().any(|e| e.contains("unknown protocol"))); + } + + #[test] + fn rejects_rules_and_access_together() { + let ep = L7EndpointFields { + protocol: "rest", + access: "full", + has_rules: true, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!(errors.iter().any(|e| e.contains("mutually exclusive"))); + } + + #[test] + fn rejects_jsonrpc_with_access_presets() { + let ep = L7EndpointFields { + protocol: "json-rpc", + access: "full", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!( + errors + .iter() + .any(|e| e.contains("does not support access presets")) + ); + } + + #[test] + fn rejects_mcp_with_access_presets() { + let ep = L7EndpointFields { + protocol: "mcp", + access: "full", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!( + errors + .iter() + .any(|e| e.contains("allow_all_known_mcp_methods")) + ); + } + + #[test] + fn rejects_jsonrpc_without_rules() { + let ep = L7EndpointFields { + protocol: "json-rpc", + access: "", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!(errors.iter().any(|e| e.contains("requires explicit rules"))); + } + + #[test] + fn rejects_protocol_without_rules_or_access() { + let ep = L7EndpointFields { + protocol: "rest", + access: "", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!( + errors + .iter() + .any(|e| e.contains("protocol requires rules or access")) + ); + } + + #[test] + fn rejects_mcp_without_rules_when_allow_all_false() { + let ep = L7EndpointFields { + protocol: "mcp", + access: "", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!(errors.iter().any(|e| e.contains("mcp requires rules when"))); + } + + #[test] + fn accepts_mcp_with_allow_all_true() { + let ep = L7EndpointFields { + protocol: "mcp", + access: "", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: true, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!(errors.is_empty(), "expected no errors, got: {errors:?}"); + } + + #[test] + fn rejects_rules_that_deny_all() { + let ep = L7EndpointFields { + protocol: "rest", + access: "", + has_rules: true, + has_deny_rules: false, + rules_would_deny_all: true, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!(errors.iter().any(|e| e.contains("would deny all traffic"))); + } + + #[test] + fn rejects_deny_rules_without_protocol() { + let ep = L7EndpointFields { + protocol: "", + access: "", + has_rules: false, + has_deny_rules: true, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!( + errors + .iter() + .any(|e| e.contains("deny_rules require protocol")) + ); + } + + #[test] + fn rejects_deny_rules_without_allow_base() { + let ep = L7EndpointFields { + protocol: "rest", + access: "", + has_rules: false, + has_deny_rules: true, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!( + errors + .iter() + .any(|e| e.contains("deny_rules require rules or access")) + ); + } + + #[test] + fn no_protocol_no_errors() { + let ep = L7EndpointFields { + protocol: "", + access: "", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert!(errors.is_empty(), "expected no errors, got: {errors:?}"); + } + + #[test] + fn l7_protocol_parse_known_variants() { + assert_eq!(L7Protocol::parse("rest"), Some(L7Protocol::Rest)); + assert_eq!(L7Protocol::parse("websocket"), Some(L7Protocol::Websocket)); + assert_eq!(L7Protocol::parse("graphql"), Some(L7Protocol::Graphql)); + assert_eq!(L7Protocol::parse("sql"), Some(L7Protocol::Sql)); + assert_eq!(L7Protocol::parse("json-rpc"), Some(L7Protocol::JsonRpc)); + assert_eq!(L7Protocol::parse("mcp"), Some(L7Protocol::Mcp)); + assert_eq!(L7Protocol::parse("unknown"), None); + assert_eq!(L7Protocol::parse(""), None); + assert_eq!(L7Protocol::parse("REST"), None); + assert_eq!(L7Protocol::parse("Mcp"), None); + assert_eq!(L7Protocol::parse("JSON-RPC"), None); + } + + #[test] + fn jsonrpc_with_access_emits_single_diagnostic() { + let ep = L7EndpointFields { + protocol: "json-rpc", + access: "full", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert_eq!( + errors, + vec![ + "protocol json-rpc does not support access presets; \ + use explicit rules with allow.method such as \"*\"" + ], + "should emit only the access-preset error, not the redundant missing-rules error" + ); + } + + #[test] + fn jsonrpc_without_rules_or_access_emits_single_diagnostic() { + let ep = L7EndpointFields { + protocol: "json-rpc", + access: "", + has_rules: false, + has_deny_rules: false, + rules_would_deny_all: false, + allow_all_known_mcp_methods: false, + }; + let errors = validate_l7_endpoint_semantics(&ep); + assert_eq!( + errors, + vec!["protocol json-rpc requires explicit rules with allow.method"], + "should emit only the missing-rules error" + ); + } + + #[test] + fn l7_protocol_jsonrpc_family() { + assert!(L7Protocol::JsonRpc.is_jsonrpc_family()); + assert!(L7Protocol::Mcp.is_jsonrpc_family()); + assert!(!L7Protocol::Rest.is_jsonrpc_family()); + assert!(!L7Protocol::Websocket.is_jsonrpc_family()); + assert!(!L7Protocol::Graphql.is_jsonrpc_family()); + assert!(!L7Protocol::Sql.is_jsonrpc_family()); + } +} diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index 3c72b19b3..ea838b3b7 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -10,6 +10,7 @@ //! these types, ensuring round-trip fidelity. mod compose; +mod l7_validate; mod merge; mod middleware; @@ -29,6 +30,7 @@ pub use compose::{ PROVIDER_RULE_NAME_PREFIX, ProviderPolicyLayer, compose_effective_policy, is_provider_rule_name, provider_rule_name, strip_provider_rule_names, }; +pub use l7_validate::{L7EndpointFields, L7Protocol, validate_l7_endpoint_semantics}; pub use merge::{ PolicyMergeError, PolicyMergeOp, PolicyMergeResult, PolicyMergeWarning, generated_rule_name, merge_policy, policy_covers_rule, diff --git a/crates/openshell-providers/Cargo.toml b/crates/openshell-providers/Cargo.toml index 9b294d7b7..abf6a6f11 100644 --- a/crates/openshell-providers/Cargo.toml +++ b/crates/openshell-providers/Cargo.toml @@ -13,6 +13,7 @@ repository.workspace = true [dependencies] glob = { workspace = true } openshell-core = { path = "../openshell-core", default-features = false } +openshell-policy = { path = "../openshell-policy" } serde = { workspace = true } serde_json = { workspace = true } serde_yml = { workspace = true } diff --git a/crates/openshell-providers/src/profiles.rs b/crates/openshell-providers/src/profiles.rs index c3441de53..6399a528b 100644 --- a/crates/openshell-providers/src/profiles.rs +++ b/crates/openshell-providers/src/profiles.rs @@ -13,6 +13,7 @@ use openshell_core::proto::{ ProviderProfileCredential, ProviderProfileDiscovery, }; use openshell_core::secrets::uses_reserved_revision_namespace; +use openshell_policy::{L7EndpointFields, validate_l7_endpoint_semantics}; use serde::ser::SerializeStruct; use serde::{Deserialize, Deserializer, Serialize, Serializer, de}; use std::collections::{HashMap, HashSet}; @@ -51,6 +52,12 @@ pub enum ProfileError { InvalidEndpoint { id: String, host: String, port: u32 }, #[error("provider profile '{id}' has duplicate credential env var '{env_var}'")] DuplicateCredentialEnvVar { id: String, env_var: String }, + #[error("provider profile '{id}' validation error: {field}: {message}")] + ValidationError { + id: String, + field: String, + message: String, + }, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -200,14 +207,14 @@ pub struct EndpointProfile { pub access: String, #[serde(default, skip_serializing_if = "String::is_empty")] pub enforcement: String, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub rules: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub rules: Option>, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub allowed_ips: Vec, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub ports: Vec, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub deny_rules: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub deny_rules: Option>, #[serde(default, skip_serializing_if = "is_false")] pub allow_encoded_slash: bool, #[serde(default, skip_serializing_if = "is_false")] @@ -264,6 +271,25 @@ pub struct L7AllowProfile { pub operation_name: String, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub fields: Vec, + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub params: HashMap, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub tool: Option, +} + +impl L7AllowProfile { + fn has_tool_selector(&self) -> bool { + self.params.contains_key("name") || self.tool.is_some() + } + + fn is_effectively_empty(&self) -> bool { + self.method.is_empty() + && self.path.is_empty() + && self.command.is_empty() + && self.operation_type.is_empty() + && self.operation_name.is_empty() + && !self.has_tool_selector() + } } #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] @@ -282,9 +308,19 @@ pub struct L7DenyRuleProfile { pub operation_name: String, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub fields: Vec, + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub params: HashMap, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub tool: Option, } -#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +impl L7DenyRuleProfile { + fn has_tool_selector(&self) -> bool { + self.params.contains_key("name") || self.tool.is_some() + } +} + +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] pub struct L7QueryMatcherProfile { #[serde(default, skip_serializing_if = "String::is_empty")] pub glob: String, @@ -292,6 +328,32 @@ pub struct L7QueryMatcherProfile { pub any: Vec, } +impl<'de> Deserialize<'de> for L7QueryMatcherProfile { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + #[derive(Deserialize)] + #[serde(untagged)] + enum StringOrObject { + Scalar(String), + Object { + #[serde(default)] + glob: String, + #[serde(default)] + any: Vec, + }, + } + match StringOrObject::deserialize(deserializer)? { + StringOrObject::Scalar(s) => Ok(Self { + glob: s, + any: vec![], + }), + StringOrObject::Object { glob, any } => Ok(Self { glob, any }), + } + } +} + #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] pub struct GraphqlOperationProfile { #[serde(default, skip_serializing_if = "String::is_empty")] @@ -524,6 +586,63 @@ impl ProviderTypeProfile { binaries: self.binaries.iter().map(binary_to_proto).collect(), } } + + pub fn validate_before_lowering(&self, source: &str) -> Vec { + let mut diagnostics = Vec::new(); + for (index, endpoint) in self.endpoints.iter().enumerate() { + if endpoint.rules.as_ref().is_some_and(Vec::is_empty) { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + &self.id, + format!("endpoints[{index}]"), + "rules list cannot be empty (would deny all traffic). \ + Use `access: full` or remove rules.", + )); + } + if endpoint.deny_rules.as_ref().is_some_and(Vec::is_empty) { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + &self.id, + format!("endpoints[{index}]"), + "deny_rules list cannot be empty (would have no effect). \ + Remove it if no denials are needed.", + )); + } + + if endpoint.protocol != "mcp" { + continue; + } + if let Some(rules) = &endpoint.rules { + for (rule_idx, rule) in rules.iter().enumerate() { + if rule + .allow + .as_ref() + .is_some_and(|a| a.tool.is_some() && a.params.contains_key("name")) + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + &self.id, + format!("endpoints[{index}].rules[{rule_idx}].allow"), + "MCP rules must use either tool or params.name, not both", + )); + } + } + } + if let Some(deny_rules) = &endpoint.deny_rules { + for (deny_idx, deny_rule) in deny_rules.iter().enumerate() { + if deny_rule.tool.is_some() && deny_rule.params.contains_key("name") { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + &self.id, + format!("endpoints[{index}].deny_rules[{deny_idx}]"), + "MCP rules must use either tool or params.name, not both", + )); + } + } + } + } + diagnostics + } } #[allow(clippy::trivially_copy_pass_by_ref)] @@ -928,10 +1047,22 @@ fn endpoint_to_proto(endpoint: &EndpointProfile) -> NetworkEndpoint { tls: endpoint.tls.clone(), enforcement: endpoint.enforcement.clone(), access: endpoint.access.clone(), - rules: endpoint.rules.iter().map(rule_to_proto).collect(), + rules: endpoint + .rules + .as_deref() + .unwrap_or_default() + .iter() + .map(rule_to_proto) + .collect(), allowed_ips: endpoint.allowed_ips.clone(), ports: endpoint.ports.clone(), - deny_rules: endpoint.deny_rules.iter().map(deny_rule_to_proto).collect(), + deny_rules: endpoint + .deny_rules + .as_deref() + .unwrap_or_default() + .iter() + .map(deny_rule_to_proto) + .collect(), allow_encoded_slash: endpoint.allow_encoded_slash, websocket_credential_rewrite: endpoint.websocket_credential_rewrite, request_body_credential_rewrite: endpoint.request_body_credential_rewrite, @@ -960,14 +1091,24 @@ fn endpoint_from_proto(endpoint: &NetworkEndpoint) -> EndpointProfile { tls: endpoint.tls.clone(), access: endpoint.access.clone(), enforcement: endpoint.enforcement.clone(), - rules: endpoint.rules.iter().map(rule_from_proto).collect(), + rules: if endpoint.rules.is_empty() { + None + } else { + Some(endpoint.rules.iter().map(rule_from_proto).collect()) + }, allowed_ips: endpoint.allowed_ips.clone(), ports: endpoint.ports.clone(), - deny_rules: endpoint - .deny_rules - .iter() - .map(deny_rule_from_proto) - .collect(), + deny_rules: if endpoint.deny_rules.is_empty() { + None + } else { + Some( + endpoint + .deny_rules + .iter() + .map(deny_rule_from_proto) + .collect(), + ) + }, allow_encoded_slash: endpoint.allow_encoded_slash, websocket_credential_rewrite: endpoint.websocket_credential_rewrite, request_body_credential_rewrite: endpoint.request_body_credential_rewrite, @@ -1027,7 +1168,65 @@ fn rule_from_proto(rule: &L7Rule) -> L7RuleProfile { } } +fn glob_uses_wildcard(s: &str) -> bool { + s.contains('*') || s.contains('?') || s.contains('[') || s.contains('{') +} + +fn matcher_uses_glob_wildcard(matcher: &L7QueryMatcherProfile) -> bool { + glob_uses_wildcard(&matcher.glob) || matcher.any.iter().any(|s| glob_uses_wildcard(s)) +} + +fn validate_mcp_matcher( + diagnostics: &mut Vec, + source: &str, + profile_id: &str, + field: &str, + matcher: &L7QueryMatcherProfile, + strict_tool_names: bool, +) { + if !matcher.glob.is_empty() && !matcher.any.is_empty() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + field.to_string(), + "matcher cannot specify both glob and any", + )); + } + if matcher.glob.is_empty() && matcher.any.is_empty() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + field.to_string(), + "matcher must specify glob or any", + )); + } + if !strict_tool_names && matcher_uses_glob_wildcard(matcher) { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + field.to_string(), + "wildcard tool-name matchers require mcp.strict_tool_names to remain enabled", + )); + } +} + +fn method_matcher_matches_tools_call(method: &str) -> bool { + method == "tools/call" + || method == "*" + || glob::Pattern::new(method).is_ok_and(|pattern| pattern.matches("tools/call")) +} + fn allow_to_proto(allow: &L7AllowProfile) -> L7Allow { + let mut params: HashMap = allow + .params + .iter() + .map(|(name, matcher)| (name.clone(), query_matcher_to_proto(matcher))) + .collect(); + if let Some(tool) = &allow.tool { + params + .entry("name".to_string()) + .or_insert_with(|| query_matcher_to_proto(tool)); + } L7Allow { method: allow.method.clone(), path: allow.path.clone(), @@ -1040,7 +1239,7 @@ fn allow_to_proto(allow: &L7AllowProfile) -> L7Allow { operation_type: allow.operation_type.clone(), operation_name: allow.operation_name.clone(), fields: allow.fields.clone(), - params: HashMap::new(), + params, } } @@ -1057,10 +1256,26 @@ fn allow_from_proto(allow: &L7Allow) -> L7AllowProfile { operation_type: allow.operation_type.clone(), operation_name: allow.operation_name.clone(), fields: allow.fields.clone(), + params: allow + .params + .iter() + .map(|(name, matcher)| (name.clone(), query_matcher_from_proto(matcher))) + .collect(), + tool: None, } } fn deny_rule_to_proto(rule: &L7DenyRuleProfile) -> L7DenyRule { + let mut params: HashMap = rule + .params + .iter() + .map(|(name, matcher)| (name.clone(), query_matcher_to_proto(matcher))) + .collect(); + if let Some(tool) = &rule.tool { + params + .entry("name".to_string()) + .or_insert_with(|| query_matcher_to_proto(tool)); + } L7DenyRule { method: rule.method.clone(), path: rule.path.clone(), @@ -1073,7 +1288,7 @@ fn deny_rule_to_proto(rule: &L7DenyRuleProfile) -> L7DenyRule { operation_type: rule.operation_type.clone(), operation_name: rule.operation_name.clone(), fields: rule.fields.clone(), - params: HashMap::new(), + params, } } @@ -1090,6 +1305,12 @@ fn deny_rule_from_proto(rule: &L7DenyRule) -> L7DenyRuleProfile { operation_type: rule.operation_type.clone(), operation_name: rule.operation_name.clone(), fields: rule.fields.clone(), + params: rule + .params + .iter() + .map(|(name, matcher)| (name.clone(), query_matcher_from_proto(matcher))) + .collect(), + tool: None, } } @@ -1202,6 +1423,11 @@ fn validate_profiles(profiles: &[ProviderTypeProfile]) -> Result<(), ProfileErro port: endpoint.port, }); } + return Err(ProfileError::ValidationError { + id: diagnostic.profile_id.clone(), + field: diagnostic.field.clone(), + message: diagnostic.message.clone(), + }); } Ok(()) @@ -1616,6 +1842,342 @@ pub fn validate_profile_set( format!("invalid endpoint '{}:{}'", endpoint.host, endpoint.port), )); } + + if endpoint.rules.as_ref().is_some_and(Vec::is_empty) { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}]"), + "rules list cannot be empty (would deny all traffic). Use `access: full` or remove rules.", + )); + } + + if endpoint.deny_rules.as_ref().is_some_and(Vec::is_empty) { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}]"), + "deny_rules list cannot be empty (would have no effect). Remove it if no denials are needed.", + )); + } + + let l7_fields = L7EndpointFields { + protocol: &endpoint.protocol, + access: &endpoint.access, + has_rules: endpoint.rules.as_ref().is_some_and(|r| !r.is_empty()), + has_deny_rules: endpoint.deny_rules.as_ref().is_some_and(|r| !r.is_empty()), + rules_would_deny_all: endpoint.rules.as_ref().is_some_and(|r| { + !r.is_empty() + && r.iter().all(|rule| { + rule.allow + .as_ref() + .is_none_or(L7AllowProfile::is_effectively_empty) + }) + }), + allow_all_known_mcp_methods: endpoint + .mcp + .as_ref() + .and_then(|opts| opts.allow_all_known_mcp_methods) + .unwrap_or(false), + }; + for msg in validate_l7_endpoint_semantics(&l7_fields) { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}]"), + msg, + )); + } + + if endpoint.protocol == "mcp" { + let strict_tool_names = endpoint + .mcp + .as_ref() + .and_then(|opts| opts.strict_tool_names) + .unwrap_or(true); + + if let Some(rules) = &endpoint.rules { + for (rule_idx, rule) in rules.iter().enumerate() { + if let Some(allow) = &rule.allow { + let allow_loc = format!("endpoints[{index}].rules[{rule_idx}].allow"); + if allow.tool.is_some() && allow.params.contains_key("name") { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &allow_loc, + "MCP rules must use either tool or params.name, not both", + )); + } + if allow.has_tool_selector() + && !allow.method.is_empty() + && allow.method != "tools/call" + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &allow_loc, + "method must be tools/call when an MCP rule uses \ + tool or params.name", + )); + } + if allow.method.is_empty() && !l7_fields.allow_all_known_mcp_methods { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &allow_loc, + "method is required when \ + mcp.allow_all_known_mcp_methods is false", + )); + } + if glob_uses_wildcard(&allow.method) + && !allow.method.starts_with("tools/") + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("{allow_loc}.method"), + "MCP method globs are only valid for the tools/ \ + method family; omit method to use the endpoint \ + method profile", + )); + } + if !allow.path.is_empty() || !allow.query.is_empty() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &allow_loc, + "mcp L7 rules must use method/tool, not path/query", + )); + } + for key in allow.params.keys() { + if key != "name" { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("{allow_loc}.params.{key}"), + "MCP tool argument matching is not supported yet", + )); + } + } + if let Some(tool) = &allow.tool { + validate_mcp_matcher( + &mut diagnostics, + source, + profile_id, + &format!("{allow_loc}.tool"), + tool, + strict_tool_names, + ); + } + if let Some(params_name) = allow.params.get("name") { + validate_mcp_matcher( + &mut diagnostics, + source, + profile_id, + &format!("{allow_loc}.params.name"), + params_name, + strict_tool_names, + ); + } + } + } + } + + if let Some(deny_rules) = &endpoint.deny_rules { + for (deny_idx, deny_rule) in deny_rules.iter().enumerate() { + let deny_loc = format!("endpoints[{index}].deny_rules[{deny_idx}]"); + if deny_rule.tool.is_some() && deny_rule.params.contains_key("name") { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &deny_loc, + "MCP rules must use either tool or params.name, not both", + )); + } + if deny_rule.has_tool_selector() + && !deny_rule.method.is_empty() + && deny_rule.method != "tools/call" + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &deny_loc, + "method must be tools/call when an MCP rule uses \ + tool or params.name", + )); + } + if deny_rule.method.is_empty() && !l7_fields.allow_all_known_mcp_methods { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &deny_loc, + "method is required when \ + mcp.allow_all_known_mcp_methods is false", + )); + } + if glob_uses_wildcard(&deny_rule.method) + && !deny_rule.method.starts_with("tools/") + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("{deny_loc}.method"), + "MCP method globs are only valid for the tools/ \ + method family; omit method to use the endpoint \ + method profile", + )); + } + if !deny_rule.path.is_empty() || !deny_rule.query.is_empty() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &deny_loc, + "mcp L7 rules must use method/tool, not path/query", + )); + } + for key in deny_rule.params.keys() { + if key != "name" { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("{deny_loc}.params.{key}"), + "MCP tool argument matching is not supported yet", + )); + } + } + if let Some(tool) = &deny_rule.tool { + validate_mcp_matcher( + &mut diagnostics, + source, + profile_id, + &format!("{deny_loc}.tool"), + tool, + strict_tool_names, + ); + } + if let Some(params_name) = deny_rule.params.get("name") { + validate_mcp_matcher( + &mut diagnostics, + source, + profile_id, + &format!("{deny_loc}.params.name"), + params_name, + strict_tool_names, + ); + } + } + } + + let has_tool_allow_selectors = endpoint.rules.as_ref().is_some_and(|r| { + r.iter().any(|rule| { + rule.allow + .as_ref() + .is_some_and(L7AllowProfile::has_tool_selector) + }) + }); + + if has_tool_allow_selectors { + if let Some(rules) = &endpoint.rules { + for (rule_idx, rule) in rules.iter().enumerate() { + if let Some(allow) = &rule.allow { + if !allow.has_tool_selector() { + let method = allow.method.as_str(); + if method_matcher_matches_tools_call(method) + || (method.is_empty() + && l7_fields.allow_all_known_mcp_methods) + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}].rules[{rule_idx}].allow"), + "method matcher allows every tool call and conflicts \ + with MCP tool allow rules; add tool or params.name \ + to narrow tools/call, or remove the tool allow rules", + )); + } + } + } else if l7_fields.allow_all_known_mcp_methods { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}].rules[{rule_idx}].allow"), + "method matcher allows every tool call and conflicts \ + with MCP tool allow rules; add tool or params.name \ + to narrow tools/call, or remove the tool allow rules", + )); + } + } + } + + if let Some(deny_rules) = &endpoint.deny_rules { + for (deny_idx, deny_rule) in deny_rules.iter().enumerate() { + if !deny_rule.has_tool_selector() + && method_matcher_matches_tools_call(&deny_rule.method) + { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}].deny_rules[{deny_idx}]"), + "method matcher denies every tool call and conflicts \ + with MCP tool allow rules; add tool or params.name to \ + deny specific tools, or remove the tool allow rules", + )); + } + } + } + } + } else { + if endpoint.mcp.is_some() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}]"), + "mcp options are only valid for protocol mcp", + )); + } + if let Some(rules) = &endpoint.rules { + for (rule_idx, rule) in rules.iter().enumerate() { + if let Some(allow) = &rule.allow { + if allow.tool.is_some() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}].rules[{rule_idx}].allow.tool"), + "MCP tool matching is only valid for protocol mcp", + )); + } + if !allow.params.is_empty() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}].rules[{rule_idx}].allow.params"), + "params matching is only valid for protocol mcp", + )); + } + } + } + } + if let Some(deny_rules) = &endpoint.deny_rules { + for (deny_idx, deny_rule) in deny_rules.iter().enumerate() { + if deny_rule.tool.is_some() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}].deny_rules[{deny_idx}].tool"), + "MCP tool matching is only valid for protocol mcp", + )); + } + if !deny_rule.params.is_empty() { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + format!("endpoints[{index}].deny_rules[{deny_idx}].params"), + "params matching is only valid for protocol mcp", + )); + } + } + } + } } for (index, binary) in profile.binaries.iter().enumerate() { @@ -1995,9 +2557,9 @@ mod tests { use openshell_core::proto::ProviderProfileCategory; use super::{ - DiscoveryProfile, ProfileError, ProviderTypeProfile, builtin_profiles, - normalize_profile_id, parse_profile_catalog_yamls, parse_profile_json, parse_profile_yaml, - profile_to_json, profile_to_yaml, validate_profile_set, + DiscoveryProfile, L7AllowProfile, L7QueryMatcherProfile, ProfileError, ProviderTypeProfile, + builtin_profiles, normalize_profile_id, parse_profile_catalog_yamls, parse_profile_json, + parse_profile_yaml, profile_to_json, profile_to_yaml, validate_profile_set, }; fn builtin_profile(id: &str) -> &'static ProviderTypeProfile { @@ -2303,6 +2865,53 @@ binaries: assert!(exported.contains("strict_tool_names: false")); } + #[test] + fn mcp_allow_params_round_trips_through_proto() { + let profile = parse_profile_yaml( + r" +id: mcp-params +display_name: MCP Params +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + params: + name: + glob: read_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let rules = profile.endpoints[0].rules.as_ref().expect("rules present"); + assert_eq!(rules.len(), 1); + let allow = rules[0].allow.as_ref().expect("allow present"); + assert!( + allow.params.contains_key("name"), + "params.name should be set" + ); + + let from_proto = ProviderTypeProfile::from_proto(&profile.to_proto()); + let rt_rules = from_proto.endpoints[0] + .rules + .as_ref() + .expect("rules survive proto round-trip"); + assert_eq!(rt_rules.len(), 1); + let rt_allow = rt_rules[0] + .allow + .as_ref() + .expect("allow survives proto round-trip"); + assert!( + rt_allow.params.contains_key("name"), + "params.name should survive proto round-trip" + ); + } + #[test] fn profile_refresh_metadata_round_trips_through_proto_and_yaml() { let profile = parse_profile_yaml( @@ -2764,12 +3373,23 @@ id: advanced display_name: Advanced category: other endpoints: + - host: graphql.example.com + port: 443 + protocol: graphql + access: read-only + persisted_queries: allow_registered + graphql_persisted_queries: + hash-a: + operation_type: query + operation_name: Viewer + fields: [viewer] + graphql_max_body_bytes: 131072 + path: /graphql - host: api.example.com ports: [443, 8443] protocol: rest tls: terminate enforcement: enforce - access: read-only rules: - allow: method: GET @@ -2782,14 +3402,6 @@ endpoints: - method: POST path: /admin/** allow_encoded_slash: true - persisted_queries: allow_registered - graphql_persisted_queries: - hash-a: - operation_type: query - operation_name: Viewer - fields: [viewer] - graphql_max_body_bytes: 131072 - path: /graphql binaries: - path: /usr/bin/custom harness: true @@ -2803,39 +3415,44 @@ binaries: ); let proto = profile.to_proto(); - let endpoint = proto.endpoints.first().expect("endpoint should exist"); - assert_eq!(endpoint.port, 0); - assert_eq!(endpoint.ports, vec![443, 8443]); - assert_eq!(endpoint.tls, "terminate"); - assert_eq!(endpoint.allowed_ips, vec!["10.0.0.0/24"]); - assert!(endpoint.allow_encoded_slash); - assert_eq!(endpoint.persisted_queries, "allow_registered"); - assert_eq!(endpoint.graphql_max_body_bytes, 131_072); - assert_eq!(endpoint.path, "/graphql"); + + let graphql_ep = &proto.endpoints[0]; + assert_eq!(graphql_ep.access, "read-only"); + assert_eq!(graphql_ep.persisted_queries, "allow_registered"); + assert_eq!(graphql_ep.graphql_max_body_bytes, 131_072); + assert_eq!(graphql_ep.path, "/graphql"); + assert_eq!( + graphql_ep + .graphql_persisted_queries + .get("hash-a") + .map(|operation| operation.operation_name.as_str()), + Some("Viewer") + ); + + let rest_ep = &proto.endpoints[1]; + assert_eq!(rest_ep.port, 0); + assert_eq!(rest_ep.ports, vec![443, 8443]); + assert_eq!(rest_ep.tls, "terminate"); + assert_eq!(rest_ep.allowed_ips, vec!["10.0.0.0/24"]); + assert!(rest_ep.allow_encoded_slash); assert_eq!( - endpoint + rest_ep .rules .first() .and_then(|rule| rule.allow.as_ref()) .map(|allow| allow.method.as_str()), Some("GET") ); - assert_eq!(endpoint.deny_rules[0].method, "POST"); - assert_eq!( - endpoint - .graphql_persisted_queries - .get("hash-a") - .map(|operation| operation.operation_name.as_str()), - Some("Viewer") - ); + assert_eq!(rest_ep.deny_rules[0].method, "POST"); assert!(proto.binaries[0].harness); let reparsed = parse_profile_yaml(&profile_to_yaml(&profile).expect("serialize YAML")) .expect("serialized profile should parse"); let reprotoo = reparsed.to_proto(); - assert_eq!(reprotoo.endpoints[0].rules.len(), 1); - assert_eq!(reprotoo.endpoints[0].deny_rules.len(), 1); - assert_eq!(reprotoo.endpoints[0].ports, vec![443, 8443]); + assert_eq!(reprotoo.endpoints[0].access, "read-only"); + assert_eq!(reprotoo.endpoints[1].rules.len(), 1); + assert_eq!(reprotoo.endpoints[1].deny_rules.len(), 1); + assert_eq!(reprotoo.endpoints[1].ports, vec![443, 8443]); assert!(reprotoo.binaries[0].harness); } @@ -3008,6 +3625,31 @@ endpoints: assert!(matches!(err, ProfileError::InvalidEndpoint { id, .. } if id == "bad-endpoint")); } + #[test] + fn parse_profile_catalog_yamls_rejects_l7_validation_errors() { + let err = parse_profile_catalog_yamls(&[r" +id: bad-l7 +display_name: Bad L7 +endpoints: + - host: api.example.com + port: 443 + protocol: rest + access: read-write + rules: + - allow: + method: GET + path: /v1/** +binaries: + - /usr/bin/example-agent +"]) + .unwrap_err(); + + assert!( + matches!(err, ProfileError::ValidationError { ref id, ref message, .. } if id == "bad-l7" && message.contains("mutually exclusive")), + "expected L7 validation error for access+rules, got: {err}" + ); + } + #[test] fn aws_sts_strategy_serde_roundtrip() { use openshell_core::proto::ProviderCredentialRefreshStrategy; @@ -3431,4 +4073,1327 @@ credentials: "unexpected diagnostics: {diagnostics:?}" ); } + + // -- L7 endpoint semantic validation (shared with runtime) ---------------- + + #[test] + fn validate_rejects_protocol_without_rules_or_access() { + let profile = parse_profile_yaml( + r" +id: opencode-openrouter +display_name: OpenCode (OpenRouter) +credentials: + - name: api_key + env_vars: [OPENROUTER_API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: openrouter.ai + port: 443 + protocol: rest + enforcement: enforce +binaries: + - /usr/bin/opencode +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("protocol requires rules or access")), + "expected lint to reject protocol without rules or access, got: {diagnostics:?}" + ); + } + + #[test] + fn validate_accepts_protocol_with_access() { + let profile = parse_profile_yaml( + r" +id: valid-rest +display_name: Valid REST +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: api.example.com + port: 443 + protocol: rest + access: read-write +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + let errors: Vec<_> = diagnostics + .iter() + .filter(|d| d.severity == "error") + .collect(); + assert!(errors.is_empty(), "unexpected errors: {errors:?}"); + } + + #[test] + fn validate_rejects_unknown_protocol() { + let profile = parse_profile_yaml( + r" +id: bad-protocol +display_name: Bad Protocol +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: api.example.com + port: 443 + protocol: ftp +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("unknown protocol")), + "expected lint to reject unknown protocol, got: {diagnostics:?}" + ); + } + + #[test] + fn validate_rejects_rules_and_access_together() { + let profile = parse_profile_yaml( + r" +id: both-rules-access +display_name: Both +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: api.example.com + port: 443 + protocol: rest + access: full + rules: + - allow: + method: GET + path: /api/** +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("mutually exclusive")), + "expected lint to reject rules + access, got: {diagnostics:?}" + ); + } + + #[test] + fn validate_rejects_empty_rules_list() { + let profile = parse_profile_yaml( + r" +id: empty-rules +display_name: Empty Rules +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: api.example.com + port: 443 + protocol: rest + access: full + rules: [] +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("rules list cannot be empty")), + "expected lint to reject empty rules list, got: {diagnostics:?}" + ); + } + + #[test] + fn validate_rejects_empty_allow_object_as_deny_all() { + let profile = parse_profile_yaml( + r" +id: empty-allow +display_name: Empty Allow +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: api.example.com + port: 443 + protocol: rest + rules: + - allow: {} +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("would deny all traffic")), + "expected lint to reject empty allow object as deny-all, got: {diagnostics:?}" + ); + } + + #[test] + fn validate_rejects_empty_deny_rules_list() { + let profile = parse_profile_yaml( + r" +id: empty-deny-rules +display_name: Empty Deny Rules +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: api.example.com + port: 443 + protocol: rest + access: full + deny_rules: [] +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("deny_rules list cannot be empty")), + "expected lint to reject empty deny_rules list, got: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_params_selector_not_classified_as_deny_all() { + let profile = parse_profile_yaml( + r" +id: mcp-params +display_name: MCP Params +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + params: + name: + glob: read_* +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + !diagnostics + .iter() + .any(|d| d.message.contains("would deny all traffic")), + "MCP rule with params.name selector should not be classified as deny-all: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_empty_allow_with_allow_all_conflicts_with_tool_rules() { + let profile = parse_profile_yaml( + r" +id: mcp-wildcard +display_name: MCP Wildcard +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + params: + name: + glob: read_* + - allow: {} +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("allows every tool call") + && d.message.contains("conflicts with MCP tool allow rules")), + "empty allow with allow_all should conflict with tool-specific rules: {diagnostics:?}" + ); + } + + #[test] + fn validate_unsupported_params_treated_as_deny_all() { + let profile = parse_profile_yaml( + r" +id: unsupported-params +display_name: Unsupported Params +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + params: + arguments: + glob: '*' +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("would deny all traffic")), + "params without name key should be classified as deny-all: {diagnostics:?}" + ); + } + + #[test] + fn mcp_deny_rule_params_round_trips_through_proto() { + let profile = parse_profile_yaml( + r" +id: mcp-deny-params +display_name: MCP Deny Params +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + deny_rules: + - method: tools/call + params: + name: + glob: dangerous_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let deny_rules = profile.endpoints[0] + .deny_rules + .as_ref() + .expect("deny_rules present"); + assert!( + deny_rules[0].params.contains_key("name"), + "params.name should be set" + ); + + let from_proto = ProviderTypeProfile::from_proto(&profile.to_proto()); + let rt_deny = from_proto.endpoints[0] + .deny_rules + .as_ref() + .expect("deny_rules survive proto round-trip"); + assert!( + rt_deny[0].params.contains_key("name"), + "deny rule params.name should survive proto round-trip" + ); + } + + #[test] + fn mcp_tool_allow_round_trips_through_proto() { + let profile = parse_profile_yaml( + r" +id: mcp-tool +display_name: MCP Tool +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + tool: + glob: read_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let rules = profile.endpoints[0].rules.as_ref().expect("rules present"); + let allow = rules[0].allow.as_ref().expect("allow present"); + assert!(allow.tool.is_some(), "tool should be parsed"); + assert_eq!(allow.tool.as_ref().unwrap().glob, "read_*"); + + let from_proto = ProviderTypeProfile::from_proto(&profile.to_proto()); + let rt_allow = from_proto.endpoints[0] + .rules + .as_ref() + .expect("rules survive")[0] + .allow + .as_ref() + .expect("allow survives"); + assert!( + rt_allow.params.contains_key("name"), + "tool should be lowered to params.name in proto: {rt_allow:?}" + ); + assert_eq!(rt_allow.params["name"].glob, "read_*"); + assert!( + rt_allow.tool.is_none(), + "tool should be None after round-trip" + ); + } + + #[test] + fn mcp_tool_deny_round_trips_through_proto() { + let profile = parse_profile_yaml( + r" +id: mcp-deny-tool +display_name: MCP Deny Tool +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + deny_rules: + - method: 'tools/call' + tool: + glob: dangerous_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let deny_rules = profile.endpoints[0] + .deny_rules + .as_ref() + .expect("deny_rules present"); + assert!(deny_rules[0].tool.is_some(), "tool should be parsed"); + + let from_proto = ProviderTypeProfile::from_proto(&profile.to_proto()); + let rt_deny = from_proto.endpoints[0] + .deny_rules + .as_ref() + .expect("deny_rules survive")[0] + .clone(); + assert!( + rt_deny.params.contains_key("name"), + "deny tool should be lowered to params.name in proto" + ); + assert_eq!(rt_deny.params["name"].glob, "dangerous_*"); + } + + #[test] + fn validate_mcp_tool_and_params_name_rejected() { + let profile = parse_profile_yaml( + r" +id: mcp-both +display_name: MCP Both +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + tool: + glob: read_* + params: + name: + glob: write_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics.iter().any(|d| d + .message + .contains("must use either tool or params.name, not both")), + "expected mutual exclusivity error, got: {diagnostics:?}" + ); + } + + #[test] + fn mcp_tool_prevents_classify_as_deny_all() { + let allow = L7AllowProfile { + method: String::new(), + path: String::new(), + command: String::new(), + query: HashMap::new(), + operation_type: String::new(), + operation_name: String::new(), + fields: vec![], + params: HashMap::new(), + tool: Some(L7QueryMatcherProfile { + glob: "read_*".to_string(), + any: vec![], + }), + }; + assert!( + !allow.is_effectively_empty(), + "allow with tool selector should not be classified as deny-all" + ); + } + + #[test] + fn validate_mcp_explicit_broad_method_without_allow_all() { + let profile = parse_profile_yaml( + r" +id: mcp-broad +display_name: MCP Broad +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + method: 'tools/call' + params: + name: + glob: read_* + - allow: + method: 'tools/call' +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("allows every tool call and conflicts")), + "explicit broad method should be detected even without allow_all_known_mcp_methods: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_glob_broad_method_detected() { + let profile = parse_profile_yaml( + r" +id: mcp-glob +display_name: MCP Glob +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + params: + name: + glob: read_* + - allow: + method: 'tools/*' +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("allows every tool call and conflicts")), + "glob method tools/* should be detected: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_deny_rule_broad_method_conflict() { + let profile = parse_profile_yaml( + r" +id: mcp-deny-broad +display_name: MCP Deny Broad +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + params: + name: + glob: read_* + deny_rules: + - method: 'tools/call' +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("denies every tool call and conflicts")), + "deny rule with broad method should be detected: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_deny_rule_with_tool_selector_ok() { + let profile = parse_profile_yaml( + r" +id: mcp-deny-ok +display_name: MCP Deny OK +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + params: + name: + glob: '*' + deny_rules: + - method: 'tools/call' + tool: + glob: dangerous_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + !diagnostics + .iter() + .any(|d| d.message.contains("denies every tool call")), + "deny rule with tool selector should not trigger conflict: {diagnostics:?}" + ); + } + + #[test] + fn mcp_tool_scalar_matcher_round_trips_through_proto() { + let profile = parse_profile_yaml( + r" +id: mcp-scalar-tool +display_name: MCP Scalar Tool +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + method: tools/call + tool: search_web +binaries: + - /usr/bin/example-agent +", + ) + .expect("scalar tool matcher should parse"); + + let rules = profile.endpoints[0].rules.as_ref().expect("rules present"); + let allow = rules[0].allow.as_ref().expect("allow present"); + assert_eq!( + allow.tool.as_ref().unwrap().glob, + "search_web", + "scalar string should deserialize to glob field" + ); + + let from_proto = ProviderTypeProfile::from_proto(&profile.to_proto()); + let rt_allow = from_proto.endpoints[0] + .rules + .as_ref() + .expect("rules survive")[0] + .allow + .as_ref() + .expect("allow survives"); + assert_eq!( + rt_allow.params["name"].glob, "search_web", + "scalar tool should lower to params.name in proto" + ); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + let errors: Vec<_> = diagnostics + .iter() + .filter(|d| d.severity == "error") + .collect(); + assert!( + errors.is_empty(), + "scalar tool matcher should produce no validation errors: {errors:?}" + ); + } + + #[test] + fn validate_mcp_deny_tool_and_params_name_rejected() { + let profile = parse_profile_yaml( + r" +id: mcp-deny-both +display_name: MCP Deny Both +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + deny_rules: + - method: tools/call + tool: + glob: dangerous_* + params: + name: + glob: also_dangerous_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("either tool or params.name")), + "deny rule with both tool and params.name should be rejected: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_method_must_be_tools_call_with_tool_selector() { + let profile = parse_profile_yaml( + r" +id: mcp-bad-method +display_name: MCP Bad Method +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + method: resources/read + tool: + glob: read_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("method must be tools/call")), + "tool selector with wrong method should be rejected: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_deny_method_must_be_tools_call_with_tool_selector() { + let profile = parse_profile_yaml( + r" +id: mcp-deny-bad-method +display_name: MCP Deny Bad Method +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + deny_rules: + - method: resources/read + tool: + glob: dangerous_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("method must be tools/call")), + "deny rule with tool selector and wrong method should be rejected: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_method_required_without_allow_all() { + let profile = parse_profile_yaml( + r" +id: mcp-no-method +display_name: MCP No Method +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + tool: + glob: read_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("method is required")), + "missing method without allow_all should be rejected: {diagnostics:?}" + ); + } + + #[test] + fn validate_before_lowering_catches_tool_params_conflict() { + let profile = parse_profile_yaml( + r" +id: mcp-both +display_name: MCP Both +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + method: tools/call + tool: + glob: read_* + params: + name: + glob: write_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = profile.validate_before_lowering("profile.yaml"); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("either tool or params.name")), + "pre-lowering validation should catch tool + params.name conflict: {diagnostics:?}" + ); + } + + #[test] + fn validate_before_lowering_ok_with_tool_only() { + let profile = parse_profile_yaml( + r" +id: mcp-tool-only +display_name: MCP Tool Only +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + method: tools/call + tool: + glob: read_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = profile.validate_before_lowering("profile.yaml"); + assert!( + diagnostics.is_empty(), + "tool-only rule should pass pre-lowering validation: {diagnostics:?}" + ); + } + + #[test] + fn validate_before_lowering_deny_catches_conflict() { + let profile = parse_profile_yaml( + r" +id: mcp-deny-both +display_name: MCP Deny Both +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + deny_rules: + - method: tools/call + tool: + glob: dangerous_* + params: + name: + glob: also_dangerous_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = profile.validate_before_lowering("profile.yaml"); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("either tool or params.name")), + "deny rule with both should be caught before lowering: {diagnostics:?}" + ); + } + + #[test] + fn validate_rejects_deny_rules_without_protocol() { + let profile = parse_profile_yaml( + r" +id: deny-no-protocol +display_name: Deny No Protocol +credentials: + - name: api_key + env_vars: [API_KEY] + auth_style: bearer + header_name: authorization +discovery: + credentials: [api_key] +endpoints: + - host: api.example.com + port: 443 + deny_rules: + - method: POST +binaries: + - /usr/bin/app +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("deny_rules require protocol")), + "expected lint to reject deny_rules without protocol, got: {diagnostics:?}" + ); + } + + #[test] + fn validate_before_lowering_catches_empty_rules() { + let profile = parse_profile_yaml( + r" +id: empty-rules +display_name: Empty Rules +endpoints: + - host: api.example.com + port: 443 + protocol: mcp + rules: [] +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = profile.validate_before_lowering("profile.yaml"); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("rules list cannot be empty")), + "pre-lowering should catch empty rules list: {diagnostics:?}" + ); + } + + #[test] + fn validate_before_lowering_catches_empty_deny_rules() { + let profile = parse_profile_yaml( + r" +id: empty-deny-rules +display_name: Empty Deny Rules +endpoints: + - host: api.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + method: tools/call + deny_rules: [] +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = profile.validate_before_lowering("profile.yaml"); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("deny_rules list cannot be empty")), + "pre-lowering should catch empty deny_rules list: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_unsupported_params_key() { + let profile = parse_profile_yaml( + r" +id: bad-params +display_name: Bad Params +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + method: tools/call + params: + arguments: + glob: '*' +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics.iter().any(|d| d + .message + .contains("MCP tool argument matching is not supported yet")), + "expected unsupported params key error: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_matcher_glob_and_any_rejected() { + let profile = parse_profile_yaml( + r" +id: glob-and-any +display_name: Glob And Any +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + method: tools/call + tool: + glob: read_* + any: + - write_data +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("cannot specify both glob and any")), + "expected glob+any mutual exclusion error: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_matcher_empty_rejected() { + let yaml = r" +id: empty-matcher +display_name: Empty Matcher +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + method: tools/call + tool: '' +binaries: + - /usr/bin/example-agent +"; + let profile = parse_profile_yaml(yaml).expect("profile should parse"); + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("matcher must specify glob or any")), + "expected empty matcher error: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_wildcard_without_strict_tool_names() { + let profile = parse_profile_yaml( + r" +id: wildcard-no-strict +display_name: Wildcard No Strict +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + strict_tool_names: false + rules: + - allow: + method: tools/call + tool: + glob: '*' +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics.iter().any(|d| d + .message + .contains("wildcard tool-name matchers require mcp.strict_tool_names")), + "expected wildcard policy error: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_wildcard_with_strict_tool_names_ok() { + let profile = parse_profile_yaml( + r" +id: wildcard-strict +display_name: Wildcard Strict +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + strict_tool_names: true + rules: + - allow: + method: tools/call + tool: + glob: '*' +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + !diagnostics.iter().any(|d| d + .message + .contains("wildcard tool-name matchers require mcp.strict_tool_names")), + "wildcard with strict_tool_names enabled should be OK: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_method_glob_restricted_to_tools() { + let profile = parse_profile_yaml( + r" +id: bad-method-glob +display_name: Bad Method Glob +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + method: 'resources/*' +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics.iter().any(|d| d + .message + .contains("MCP method globs are only valid for the tools/ method family")), + "expected method glob restriction error: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_path_rejected() { + let profile = parse_profile_yaml( + r" +id: mcp-path +display_name: MCP Path +endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + method: tools/call + path: /api/tools +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics + .iter() + .any(|d| d.message.contains("must use method/tool, not path/query")), + "expected path rejection for MCP: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_tool_on_non_mcp_rejected() { + let profile = parse_profile_yaml( + r" +id: rest-tool +display_name: REST Tool +endpoints: + - host: api.example.com + port: 443 + protocol: rest + rules: + - allow: + method: GET + tool: + glob: read_* +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics.iter().any(|d| d + .message + .contains("MCP tool matching is only valid for protocol mcp")), + "expected tool-on-non-mcp rejection: {diagnostics:?}" + ); + } + + #[test] + fn validate_mcp_options_on_non_mcp_rejected() { + let profile = parse_profile_yaml( + r" +id: rest-mcp-opts +display_name: REST MCP Options +endpoints: + - host: api.example.com + port: 443 + protocol: rest + mcp: + strict_tool_names: true + rules: + - allow: + method: GET +binaries: + - /usr/bin/example-agent +", + ) + .expect("profile should parse"); + + let diagnostics = validate_profile_set(&[("profile.yaml".to_string(), profile)]); + assert!( + diagnostics.iter().any(|d| d + .message + .contains("mcp options are only valid for protocol mcp")), + "expected mcp-options-on-non-mcp rejection: {diagnostics:?}" + ); + } } diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 59774bbcc..b9332630b 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -8113,6 +8113,7 @@ mod tests { host: "api.github.com".to_string(), port: 443, protocol: "rest".to_string(), + access: "full".to_string(), deny_rules: vec![L7DenyRule { method: "DELETE".to_string(), path: "/repos/*".to_string(), diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index cae751b42..337b6f920 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -2714,6 +2714,7 @@ mod tests { port, path: path.to_string(), protocol: "rest".to_string(), + access: "full".to_string(), ..Default::default() }]; handle_import_provider_profiles( @@ -2842,6 +2843,7 @@ mod tests { port: 443, path: "/v1/**".to_string(), protocol: "rest".to_string(), + access: "full".to_string(), ..Default::default() }]; let response = handle_import_provider_profiles( @@ -3167,6 +3169,7 @@ mod tests { port: 443, path: "/v1/**".to_string(), protocol: "rest".to_string(), + access: "full".to_string(), ..Default::default() }]; let response = handle_update_provider_profiles( diff --git a/crates/openshell-supervisor-network/src/l7/mod.rs b/crates/openshell-supervisor-network/src/l7/mod.rs index c9330a3de..d52985400 100644 --- a/crates/openshell-supervisor-network/src/l7/mod.rs +++ b/crates/openshell-supervisor-network/src/l7/mod.rs @@ -21,34 +21,8 @@ pub mod tls; pub(crate) mod token_grant_injection; pub(crate) mod websocket; -/// Application-layer protocol for L7 inspection. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum L7Protocol { - Rest, - Websocket, - Graphql, - Sql, - JsonRpc, - Mcp, -} - -impl L7Protocol { - pub fn parse(s: &str) -> Option { - match s.to_ascii_lowercase().as_str() { - "rest" => Some(Self::Rest), - "websocket" => Some(Self::Websocket), - "graphql" => Some(Self::Graphql), - "sql" => Some(Self::Sql), - "json-rpc" => Some(Self::JsonRpc), - "mcp" => Some(Self::Mcp), - _ => None, - } - } - - pub fn is_jsonrpc_family(self) -> bool { - matches!(self, Self::JsonRpc | Self::Mcp) - } -} +pub use openshell_policy::L7Protocol; +use openshell_policy::{L7EndpointFields, validate_l7_endpoint_semantics}; /// TLS handling mode for proxy connections. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -1031,40 +1005,51 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< )); } - // rules + access mutual exclusion - if has_rules && !access.is_empty() { - errors.push(format!("{loc}: rules and access are mutually exclusive")); - } - - if jsonrpc_family && !access.is_empty() { - if protocol == "mcp" { - errors.push(format!( - "{loc}: protocol {protocol} does not support access presets; use rules/deny_rules or set mcp.allow_all_known_mcp_methods: true for an allow-all MCP policy" - )); - } else { - errors.push(format!( - "{loc}: protocol {protocol} does not support access presets; use explicit rules with allow.method such as \"*\"" - )); - } - } - - if protocol == "json-rpc" && !has_rules { - errors.push(format!( - "{loc}: protocol {protocol} requires explicit rules with allow.method" - )); - } - - // protocol requires rules or access - if !protocol.is_empty() && protocol != "mcp" && !has_rules && access.is_empty() { - errors.push(format!( - "{loc}: protocol requires rules or access to define allowed traffic" - )); - } - - if !protocol.is_empty() && l7_protocol.is_none() { - errors.push(format!( - "{loc}: unknown protocol '{protocol}' (expected rest, websocket, graphql, sql, json-rpc, or mcp)" - )); + // L7 endpoint semantic validation (shared with profile lint). + // Computed lazily: has_deny_rules and rules_would_deny_all are + // needed here but also referenced by per-rule checks below. + let has_deny_rules = ep + .get("deny_rules") + .and_then(|v| v.as_array()) + .is_some_and(|a| !a.is_empty()); + let rules_would_deny_all = + ep.get("rules") + .and_then(|v| v.as_array()) + .is_some_and(|rules| { + !rules.is_empty() + && rules.iter().all(|rule| { + let allow = rule.get("allow"); + allow.is_none_or(serde_json::Value::is_null) + || allow.is_some_and(|v| { + let str_empty = |key| { + v.get(key) + .and_then(|s| s.as_str()) + .unwrap_or("") + .is_empty() + }; + str_empty("method") + && str_empty("path") + && str_empty("command") + && str_empty("operation_type") + && str_empty("operation_name") + && !mcp_rule_has_tool_selector(v) + }) + }) + }); + let mcp_allow_all_known_mcp_methods = ep + .get("mcp_allow_all_known_mcp_methods") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false); + let l7_fields = L7EndpointFields { + protocol, + access, + has_rules, + has_deny_rules, + rules_would_deny_all, + allow_all_known_mcp_methods: mcp_allow_all_known_mcp_methods, + }; + for msg in validate_l7_endpoint_semantics(&l7_fields) { + errors.push(format!("{loc}: {msg}")); } if let Some(mode) = ep.get("persisted_queries").and_then(|v| v.as_str()) @@ -1154,20 +1139,6 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< .get("mcp_strict_tool_names") .and_then(serde_json::Value::as_bool) .unwrap_or(true); - let mcp_allow_all_known_mcp_methods = ep - .get("mcp_allow_all_known_mcp_methods") - .and_then(serde_json::Value::as_bool) - .unwrap_or(false); - if protocol == "mcp" - && !has_rules - && access.is_empty() - && !mcp_allow_all_known_mcp_methods - { - errors.push(format!( - "{loc}: protocol mcp requires rules when mcp.allow_all_known_mcp_methods is false" - )); - } - if ep .get("websocket_credential_rewrite") .and_then(serde_json::Value::as_bool) @@ -1225,41 +1196,13 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< )); } - // rules with empty list - if ep - .get("rules") - .and_then(|v| v.as_array()) - .is_some_and(Vec::is_empty) - { - errors.push(format!( - "{loc}: rules list cannot be empty (would deny all traffic). Use `access: full` or remove rules." - )); - } - // port 443 + rest + tls: skip — L7 won't work (already handled above) // 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()); + // Per-rule deny_rules validation (semantic checks handled by + // shared validator above). 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 protocol != "mcp" && !has_rules && access.is_empty() { - errors.push(format!( - "{loc}: deny_rules require rules or access to define the base allow set" - )); - } - let has_mcp_tool_allow_selectors = protocol == "mcp" && mcp_endpoint_has_tool_allow_selectors(ep); @@ -1356,6 +1299,17 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< } } + // Empty rules list (explicitly set but empty) + if ep + .get("rules") + .and_then(|v| v.as_array()) + .is_some_and(Vec::is_empty) + { + errors.push(format!( + "{loc}: rules list cannot be empty (would deny all traffic). Use `access: full` or remove rules." + )); + } + // Empty deny_rules list (explicitly set but empty) if ep .get("deny_rules") @@ -1409,16 +1363,20 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< for (rule_idx, rule) in rules.iter().enumerate() { let allow = rule.get("allow").unwrap_or(rule); let rule_loc = format!("{loc}.rules[{rule_idx}].allow"); - if has_mcp_tool_allow_selectors - && allow + if has_mcp_tool_allow_selectors && !mcp_rule_has_tool_selector(allow) { + let method = allow .get("method") .and_then(serde_json::Value::as_str) - .is_some_and(method_matcher_matches_tools_call) - && !mcp_rule_has_tool_selector(allow) - { - errors.push(format!( - "{rule_loc}: method matcher allows every tool call and conflicts with MCP tool allow rules; add tool or params.name to narrow tools/call, or remove the tool allow rules" - )); + .unwrap_or(""); + if method_matcher_matches_tools_call(method) + || (method.is_empty() && mcp_allow_all_known_mcp_methods) + { + errors.push(format!( + "{rule_loc}: method matcher allows every tool call and conflicts \ + with MCP tool allow rules; add tool or params.name to narrow \ + tools/call, or remove the tool allow rules" + )); + } } validate_jsonrpc_rule_fields( &mut errors, @@ -2361,6 +2319,46 @@ mod tests { ); } + #[test] + fn validate_mcp_empty_allow_with_allow_all_conflicts_with_tool_rules() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "mcp.example.com", + "port": 443, + "protocol": "mcp", + "mcp_allow_all_known_mcp_methods": true, + "rules": [{ + "allow": { + "params": { "name": { "glob": "read_*" } } + } + }, { + "allow": { + "method": "", + "path": "", + "command": "", + "operation_type": "", + "operation_name": "" + } + }] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors.iter().any(|e| { + e.contains("rules[1].allow") + && e.contains("allows every tool call") + && e.contains("conflicts with MCP tool allow rules") + }), + "empty allow with allow_all_known_mcp_methods normalizes to method:* and should \ + conflict with tool-specific rules: {errors:?}" + ); + } + #[test] fn validate_jsonrpc_rejects_params_matchers() { let data = serde_json::json!({ @@ -3438,6 +3436,117 @@ mod tests { ); } + #[test] + fn validate_rules_empty_list_rejected() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "access": "full", + "rules": [] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors + .iter() + .any(|e| e.contains("rules list cannot be empty")), + "should reject empty rules: {errors:?}" + ); + } + + #[test] + fn validate_rules_deny_all_with_empty_allow_object() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 443, + "protocol": "rest", + "rules": [{"allow": {"method": "", "path": ""}}] + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors.iter().any(|e| e.contains("would deny all traffic")), + "should detect deny-all with empty allow object: {errors:?}" + ); + } + + #[test] + fn validate_mcp_params_selector_not_classified_as_deny_all() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "mcp.example.com", + "port": 443, + "protocol": "mcp", + "mcp_allow_all_known_mcp_methods": true, + "rules": [{ + "allow": { + "method": "", + "path": "", + "command": "", + "operation_type": "", + "operation_name": "", + "params": { "name": { "glob": "read_*" } } + } + }], + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "MCP rule with params.name selector should not be classified as deny-all: {errors:?}" + ); + } + + #[test] + fn validate_mcp_tool_selector_not_classified_as_deny_all() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "mcp.example.com", + "port": 443, + "protocol": "mcp", + "mcp_allow_all_known_mcp_methods": true, + "rules": [{ + "allow": { + "method": "", + "path": "", + "command": "", + "operation_type": "", + "operation_name": "", + "tool": "my-tool" + } + }], + }], + "binaries": [] + } + } + }); + let (errors, _) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "MCP rule with tool selector should not be classified as deny-all: {errors:?}" + ); + } + #[test] fn validate_deny_rules_empty_list_rejected() { let data = serde_json::json!({ diff --git a/docs/reference/policy-schema.mdx b/docs/reference/policy-schema.mdx index 721f16109..9c93b717c 100644 --- a/docs/reference/policy-schema.mdx +++ b/docs/reference/policy-schema.mdx @@ -179,6 +179,16 @@ Each endpoint defines a reachable destination and optional inspection rules. | `mcp.allow_all_known_mcp_methods` | bool | No | Defaults to `false`. When `true`, enables the endpoint MCP method profile: omitted `rules` allow all MCP-family methods and all tools before `deny_rules`, and omitted rule `method` uses that profile. When unset or `false`, explicit MCP method rules are required; rules with `tool` or `params.name` must set `method: tools/call`. | | `json_rpc` | object | No | JSON-RPC endpoint options. For `protocol: json-rpc`, `json_rpc.max_body_bytes` sets the maximum JSON-RPC-over-HTTP request body bytes buffered for inspection. Defaults to `65536`. | +**Validation constraints:** + +- `access` and `rules` are mutually exclusive; setting both is rejected. +- When `protocol` is set, at least one of `access` or `rules` is required for `rest`, `websocket`, `graphql`, and `sql`. +- `mcp` and `json-rpc` reject `access` presets; use explicit `rules`. +- `json-rpc` requires explicit `rules` with `allow.method`. +- `mcp` requires `rules` unless `mcp.allow_all_known_mcp_methods: true`. +- `deny_rules` require `protocol`. For non-MCP protocols, `deny_rules` also require `rules` or `access` to define the base allow set. MCP `deny_rules` may omit both when `mcp.allow_all_known_mcp_methods: true` supplies the base allow set. +- `rules: []` (empty list) is rejected; use `access: full` or remove `rules`. + Credential rewrite recognizes the canonical `openshell:resolve:env:KEY` placeholder form and whole-token provider-shaped aliases such as `provider-OPENSHELL-RESOLVE-ENV-API_TOKEN` when the referenced environment key exists in the configured provider credentials. #### Access Levels diff --git a/docs/sandboxes/providers-v2.mdx b/docs/sandboxes/providers-v2.mdx index a8890f9f8..b0c729097 100644 --- a/docs/sandboxes/providers-v2.mdx +++ b/docs/sandboxes/providers-v2.mdx @@ -238,7 +238,6 @@ endpoints: path: /v1/** protocol: rest tls: "" - access: read-write enforcement: enforce allowed_ips: [] ports: []