From d9510ee7bf0ac3afda1169316f9cda221472c227 Mon Sep 17 00:00:00 2001 From: Grace Smith Date: Mon, 20 Jul 2026 14:22:58 +0100 Subject: [PATCH] refactor(policy): extract shared L7 endpoint validation Move 9 L7 endpoint semantic checks into a shared validate_l7_endpoint_semantics() function in openshell-policy. Both profile lint (openshell-providers) and the runtime validator (openshell-supervisor-network) now call the shared function via a lightweight L7EndpointFields field bag, eliminating duplication and preventing the two implementations from drifting apart. Update server test fixtures that used protocol without rules or access, which the shared validator now correctly rejects. Fixes #1714 Signed-off-by: Grace Smith --- Cargo.lock | 1 + crates/openshell-policy/src/l7_validate.rs | 388 ++++++++++++++++++ crates/openshell-policy/src/lib.rs | 2 + crates/openshell-providers/Cargo.toml | 1 + crates/openshell-providers/src/profiles.rs | 260 ++++++++++-- crates/openshell-server/src/grpc/policy.rs | 1 + crates/openshell-server/src/grpc/provider.rs | 3 + .../src/l7/mod.rs | 107 ++--- 8 files changed, 655 insertions(+), 108 deletions(-) create mode 100644 crates/openshell-policy/src/l7_validate.rs diff --git a/Cargo.lock b/Cargo.lock index b412467610..2a13197c8f 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-policy/src/l7_validate.rs b/crates/openshell-policy/src/l7_validate.rs new file mode 100644 index 0000000000..12530b18a6 --- /dev/null +++ b/crates/openshell-policy/src/l7_validate.rs @@ -0,0 +1,388 @@ +// 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.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, + } + } + + /// 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 but would deny all traffic (e.g. + /// the rules array exists but is empty, or all entries lack 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 = if protocol.is_empty() { + None + } else { + L7Protocol::parse(protocol) + }; + let jsonrpc_family = l7_protocol.is_some_and(L7Protocol::is_jsonrpc_family); + + // 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 protocol == "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 protocol == "json-rpc" && !ep.has_rules { + errors.push(format!( + "protocol {protocol} requires explicit rules with allow.method" + )); + } + + // 5. Non-MCP protocol requires rules or access + if !protocol.is_empty() && protocol != "mcp" && !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 protocol == "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 list cannot be empty (would deny all traffic). \ + Use `access: full` or remove 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 && protocol != "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: false, + 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); + } + + #[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 3c72b19b32..ea838b3b74 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 9b294d7b71..abf6a6f11a 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 c3441de533..590083e602 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}; @@ -1616,6 +1617,28 @@ pub fn validate_profile_set( format!("invalid endpoint '{}:{}'", endpoint.host, endpoint.port), )); } + + let l7_fields = L7EndpointFields { + protocol: &endpoint.protocol, + access: &endpoint.access, + has_rules: !endpoint.rules.is_empty(), + has_deny_rules: !endpoint.deny_rules.is_empty(), + rules_would_deny_all: !endpoint.rules.is_empty() + && endpoint.rules.iter().all(|r| r.allow.is_none()), + 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, + )); + } } for (index, binary) in profile.binaries.iter().enumerate() { @@ -2764,12 +2787,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 +2816,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 +2829,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); } @@ -3431,4 +3462,173 @@ 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_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:?}" + ); + } } diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 59774bbcc8..b9332630bc 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 cae751b42a..337b6f9200 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 c9330a3de1..d712cf982a 100644 --- a/crates/openshell-supervisor-network/src/l7/mod.rs +++ b/crates/openshell-supervisor-network/src/l7/mod.rs @@ -21,6 +21,8 @@ pub mod tls; pub(crate) mod token_grant_injection; pub(crate) mod websocket; +use openshell_policy::{L7EndpointFields, validate_l7_endpoint_semantics}; + /// Application-layer protocol for L7 inspection. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum L7Protocol { @@ -1031,40 +1033,31 @@ 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(Vec::is_empty); + 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 +1147,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 +1204,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);