diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index eb224685ed..034f705909 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -7082,9 +7082,9 @@ fn format_endpoints(rule: &openshell_core::proto::NetworkPolicyRule) -> String { /// `openshell rule get` useful for approval review. fn format_endpoint(endpoint: &openshell_core::proto::NetworkEndpoint) -> String { let host_port = if endpoint.port > 0 { - format!("{}:{}", endpoint.host, endpoint.port) + format!("{}:{}{}", endpoint.host, endpoint.port, endpoint.path) } else { - endpoint.host.clone() + format!("{}{}", endpoint.host, endpoint.path) }; let mut tags: Vec = Vec::new(); @@ -7108,16 +7108,67 @@ fn format_endpoint(endpoint: &openshell_core::proto::NetworkEndpoint) -> String if let Some(allow) = &r.allow { let method = non_empty_or(&allow.method, "*"); let path = non_empty_or(&allow.path, "*"); - tags.push(format!("allow {method} {path}")); + let mut tag = format!("allow {method} {path}"); + append_graphql( + &mut tag, + &allow.operation_type, + &allow.operation_name, + &allow.fields, + ); + append_matchers(&mut tag, &allow.query); + append_matchers(&mut tag, &allow.params); + tags.push(tag); } } for r in &endpoint.deny_rules { let method = non_empty_or(&r.method, "*"); let path = non_empty_or(&r.path, "*"); - tags.push(format!("deny {method} {path}")); + let mut tag = format!("deny {method} {path}"); + append_graphql(&mut tag, &r.operation_type, &r.operation_name, &r.fields); + append_matchers(&mut tag, &r.query); + append_matchers(&mut tag, &r.params); + tags.push(tag); } format!("{host_port} [{}]", tags.join(", ")) + .escape_debug() + .to_string() +} + +/// Append GraphQL operation selectors, such as `graphql mutation createIssue`. +fn append_graphql(tag: &mut String, operation_type: &str, operation_name: &str, fields: &[String]) { + use std::fmt::Write as _; + + if !operation_type.is_empty() || !operation_name.is_empty() { + let _ = write!( + tag, + " graphql {} {}", + non_empty_or(operation_type, "*"), + non_empty_or(operation_name, "*") + ); + } + if !fields.is_empty() { + let _ = write!(tag, " fields {}", fields.join(",")); + } +} + +/// Append query or MCP params matchers in key order, so an MCP tool rule +/// shows as `allow tools/call * name=read_wiki_structure`. +fn append_matchers( + tag: &mut String, + matchers: &HashMap, +) { + use std::fmt::Write as _; + + let mut entries: Vec<_> = matchers.iter().collect(); + entries.sort_by_key(|(key, _)| *key); + for (key, matcher) in entries { + if matcher.any.is_empty() { + let _ = write!(tag, " {key}={}", matcher.glob); + } else { + let _ = write!(tag, " {key} in [{}]", matcher.any.join(",")); + } + } } #[cfg(test)] @@ -8603,6 +8654,71 @@ mod tests { format_endpoint(&l7_scoped), "host.example.test:443 [L7 rest, allow PUT /v1/example/resource, deny DELETE /v1/example/resource]" ); + + let mcp_tool = NetworkEndpoint { + host: "mcp.example.test".to_string(), + port: 443, + path: "/mcp".to_string(), + protocol: "mcp".to_string(), + rules: vec![L7Rule { + allow: Some(L7Allow { + method: "tools/call".to_string(), + params: std::collections::HashMap::from([( + "name".to_string(), + openshell_core::proto::L7QueryMatcher { + glob: "read_wiki_structure".to_string(), + any: vec![], + }, + )]), + ..Default::default() + }), + }], + ..Default::default() + }; + let graphql = NetworkEndpoint { + host: "api.example.test".to_string(), + port: 443, + protocol: "graphql".to_string(), + rules: vec![L7Rule { + allow: Some(L7Allow { + operation_type: "query".to_string(), + operation_name: "Viewer".to_string(), + ..Default::default() + }), + }], + ..Default::default() + }; + assert_eq!( + format_endpoint(&graphql), + "api.example.test:443 [graphql, allow * * graphql query Viewer]" + ); + + assert_eq!( + format_endpoint(&mcp_tool), + "mcp.example.test:443/mcp [mcp, allow tools/call * name=read_wiki_structure]" + ); + + let mut untrusted = mcp_tool; + let allow = untrusted.rules[0].allow.as_mut().unwrap(); + allow.params.get_mut("name").unwrap().any = vec![ + "*".to_string(), + "\r\u{1b}[2Kname=read_wiki_structure".to_string(), + ]; + allow.params.insert( + "line\n".to_string(), + openshell_core::proto::L7QueryMatcher { + glob: "\r\u{1b}[2K".to_string(), + any: vec![], + }, + ); + allow.operation_name = "\r\u{1b}[2K".to_string(); + allow.fields = vec!["field\n".to_string()]; + let rendered = format_endpoint(&untrusted); + assert!(!rendered.chars().any(char::is_control)); + assert!(rendered.contains(r"name in [*,\r\u{1b}[2Kname=read_wiki_structure]")); + assert!(rendered.contains(r"line\n=\r\u{1b}[2K")); + assert!(rendered.contains(r"graphql * \r\u{1b}[2K")); + assert!(rendered.contains(r"fields field\n")); } #[test] diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index a84bca7868..f2cec20ee9 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -279,7 +279,8 @@ fn params_with_tool( params } -fn allow_def_to_proto(_protocol: &str, allow: L7AllowDef) -> L7Allow { +/// Convert an authored L7 allow rule to protobuf, carrying MCP `tool` as `params.name`. +pub fn allow_def_to_proto(allow: L7AllowDef) -> L7Allow { let params = flatten_param_matchers(params_with_tool(allow.params, allow.tool)); L7Allow { method: allow.method, @@ -300,7 +301,8 @@ fn allow_def_to_proto(_protocol: &str, allow: L7AllowDef) -> L7Allow { } } -fn deny_def_to_proto(_protocol: &str, deny: L7DenyRuleDef) -> L7DenyRule { +/// Convert an authored L7 deny rule to protobuf, carrying MCP `tool` as `params.name`. +pub fn deny_def_to_proto(deny: L7DenyRuleDef) -> L7DenyRule { let params = flatten_param_matchers(params_with_tool(deny.params, deny.tool)); L7DenyRule { method: deny.method, @@ -534,14 +536,11 @@ fn to_proto(raw: PolicyFile) -> Result { rules: allow_rules .into_iter() .map(|r| L7Rule { - allow: Some(allow_def_to_proto(&protocol, r.allow)), + allow: Some(allow_def_to_proto(r.allow)), }) .collect(), allowed_ips: e.allowed_ips, - deny_rules: deny_rules - .into_iter() - .map(|deny| deny_def_to_proto(&protocol, deny)) - .collect(), + deny_rules: deny_rules.into_iter().map(deny_def_to_proto).collect(), allow_encoded_slash: e.allow_encoded_slash, websocket_credential_rewrite: e.websocket_credential_rewrite, request_body_credential_rewrite: e.request_body_credential_rewrite, diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index cc83789417..04c80106f7 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -606,6 +606,7 @@ fn summarize_draft_chunk_rule(chunk: &DraftChunkRecord) -> Result: ` /// line per finding path (categorical shorthand from `openshell-prover`) @@ -635,37 +636,7 @@ fn validation_result_for_agent_proposal( return format!("policy invalid: {}", one_line(&error.to_string())); } - let merged_findings = match run_prover_findings(&merged, credentials) { - Ok(findings) => findings, - Err(error) => { - warn!(error = %error, "prover validation unavailable for merged policy"); - return "validation unavailable".to_string(); - } - }; - // A delta requires conclusive results for both policies. - let base_findings = match run_prover_findings(¤t_policy, credentials) { - Ok(findings) => findings, - Err(error) => { - warn!(error = %error, "prover validation unavailable for baseline policy"); - return "validation unavailable".to_string(); - } - }; - - let new_findings = finding_delta(&base_findings, &merged_findings); - if new_findings.is_empty() { - return "prover: no new findings".to_string(); - } - let count = new_findings.len(); - let mut out = format!( - "prover: {} new finding{}", - count, - if count == 1 { "" } else { "s" } - ); - for finding in &new_findings { - out.push_str("\n "); - out.push_str(&finding_shorthand(finding)); - } - out + proposal_prover_result(¤t_policy, &merged, credentials) } #[derive(Debug)] @@ -697,6 +668,14 @@ fn proposal_prover_result( candidate_effective_policy: &ProtoSandboxPolicy, credentials: &CredentialSet, ) -> String { + if [current_effective_policy, candidate_effective_policy] + .into_iter() + .flat_map(|policy| policy.network_policies.values()) + .flat_map(|rule| &rule.endpoints) + .any(|endpoint| endpoint.protocol.eq_ignore_ascii_case("mcp")) + { + return "prover: unsupported MCP rules; manual review required".to_string(); + } let candidate_findings = match run_prover_findings(candidate_effective_policy, credentials) { Ok(findings) => findings, Err(error) => { @@ -735,7 +714,7 @@ fn compute_proposal_review_token( credentials: &CredentialSet, ) -> String { let mut hasher = Sha256::new(); - hasher.update(b"openshell-proposal-evaluator-v3\0"); + hasher.update(b"openshell-proposal-evaluator-v4\0"); hasher.update(rule_name.as_bytes()); hasher.update(canonical_rule_bytes(rule)); hasher.update(deterministic_policy_hash(current_effective_policy).as_bytes()); @@ -773,7 +752,7 @@ fn compute_failed_proposal_evaluation_hash( application_error: &str, ) -> String { let mut hasher = Sha256::new(); - hasher.update(b"openshell-proposal-evaluator-v3-failed\0"); + hasher.update(b"openshell-proposal-evaluator-v4-failed\0"); hasher.update(rule_name.as_bytes()); hasher.update(canonical_rule_bytes(rule)); hasher.update(deterministic_policy_hash(current_effective_policy).as_bytes()); @@ -1848,7 +1827,7 @@ async fn auto_approve_chunk( validation_result = %validation_result, source = %context.source, resolved_from = %context.resolved_from, - "Auto-approval skipped: current stored rule has prover findings" + "Auto-approval skipped: current stored rule requires human review" ); return Ok(()); } @@ -19116,6 +19095,171 @@ mod tests { } } + #[test] + fn proposal_prover_mcp_rules_require_manual_review_on_either_side() { + let mcp = openshell_policy::parse_sandbox_policy( + r" +version: 1 +network_policies: + mcp: + endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + method: tools/call + tool: read_public + binaries: + - path: /usr/bin/curl +", + ) + .unwrap(); + let empty = ProtoSandboxPolicy::default(); + for (base, candidate) in [(&empty, &mcp), (&mcp, &empty), (&mcp, &mcp)] { + assert_eq!( + proposal_prover_result(base, candidate, &CredentialSet::default()), + "prover: unsupported MCP rules; manual review required" + ); + } + } + + #[tokio::test] + async fn mcp_proposals_refresh_legacy_clean_verdict_and_require_manual_approval() { + let state = test_server_state().await; + let sandbox = test_sandbox( + "sb-mcp-review", + "mcp-review", + ProtoSandboxPolicy::default(), + vec![], + ); + state.store.put_message(&sandbox).await.unwrap(); + seed_sandbox_approval_mode(&state, "mcp-review", "auto").await; + let policy = openshell_policy::parse_sandbox_policy( + r" +version: 1 +network_policies: + mcp: + endpoints: + - host: mcp.example.com + port: 443 + protocol: mcp + rules: + - allow: + method: tools/call + tool: read_public + binaries: + - path: /usr/bin/curl +", + ) + .unwrap(); + let response = handle_submit_policy_analysis( + &state, + with_user(Request::new(SubmitPolicyAnalysisRequest { + workspace_scope: Some(openshell_core::proto::workspace_selector("default")), + name: "mcp-review".to_string(), + analysis_mode: "agent_authored".to_string(), + proposed_chunks: vec![PolicyChunk { + rule_name: "mcp".to_string(), + proposed_rule: Some(policy.network_policies["mcp"].clone()), + ..Default::default() + }], + ..Default::default() + })), + ) + .await + .unwrap() + .into_inner(); + let chunk_id = &response.accepted_chunk_ids[0]; + let mut chunk = state + .store + .get_draft_chunk(chunk_id) + .await + .unwrap() + .unwrap(); + assert_eq!(chunk.status, "pending"); + assert_eq!( + chunk.validation_result, + "prover: unsupported MCP rules; manual review required" + ); + assert!(chunk.application_error.is_empty()); + let evaluation = + evaluate_stored_chunk_against_live_inputs(&state, "default", &sandbox, &chunk, None) + .await + .unwrap(); + let mut legacy = Sha256::new(); + legacy.update(b"openshell-proposal-evaluator-v3\0"); + legacy.update(evaluation.rule_name.as_bytes()); + legacy.update(canonical_rule_bytes(&evaluation.rule)); + legacy.update(evaluation.current_hash().as_bytes()); + legacy.update(evaluation.candidate_hash().as_bytes()); + chunk.review_token = hex::encode(legacy.finalize()); + chunk.validation_result = "prover: no new findings".to_string(); + assert!( + state + .store + .update_draft_chunk_evaluation(&chunk) + .await + .unwrap() + ); + let error = auto_approve_chunk( + &state, + chunk_id, + AutoApproveChunkContext { + sandbox: &sandbox, + workspace: "default", + source: "agent_authored", + resolved_from: "sandbox", + }, + ) + .await + .unwrap_err(); + assert_eq!(error.code(), Code::FailedPrecondition); + let refreshed = state + .store + .get_draft_chunk(chunk_id) + .await + .unwrap() + .unwrap(); + assert_ne!(refreshed.review_token, chunk.review_token); + assert_eq!(refreshed.status, "pending"); + assert_eq!( + refreshed.validation_result, + "prover: unsupported MCP rules; manual review required" + ); + assert!( + current_base_policy_for_sandbox(state.store.as_ref(), &sandbox) + .await + .unwrap() + .network_policies + .is_empty() + ); + handle_approve_draft_chunk( + &state, + with_user(Request::new(ApproveDraftChunkRequest { + sandbox: "mcp-review".to_string(), + workspace_scope: Some(openshell_core::proto::workspace_selector("default")), + chunk_id: chunk_id.clone(), + review_token: refreshed.review_token, + ..Default::default() + })), + ) + .await + .unwrap(); + let approved = current_base_policy_for_sandbox(state.store.as_ref(), &sandbox) + .await + .unwrap(); + assert_eq!( + approved.network_policies["mcp"].endpoints[0].rules[0] + .allow + .as_ref() + .unwrap() + .params["name"] + .glob, + "read_public" + ); + } + /// `protocol: rest, access: full` on a host where the binary had no /// prior credentialed reach: the prover emits /// `credential_reach_expansion`. (The per-method `capability_expansion` diff --git a/crates/openshell-supervisor-network/src/opa.rs b/crates/openshell-supervisor-network/src/opa.rs index 257bd3ce2e..7d886422ba 100644 --- a/crates/openshell-supervisor-network/src/opa.rs +++ b/crates/openshell-supervisor-network/src/opa.rs @@ -469,47 +469,38 @@ impl OpaEngine { entrypoint_pid: u32, require_binary_identity: bool, ) -> Result { - // Protobuf cannot distinguish an omitted repeated MCP version field - // from an empty one. Canonicalize before any runtime consumer reads - // the policy so both representations select the pinned default. - let proto = openshell_policy::validate_and_canonicalize_sandbox_policy(proto.clone()) - .map_err(|error| { - miette::miette!(render_bounded_validation_diagnostics( - "policy validation failed", - error - .violations() - .iter() - .map(redacted_policy_violation_category), - )) - })?; - - let ambiguities = openshell_policy::find_endpoint_ambiguities(&proto); - if !ambiguities.is_empty() { - return Err(miette::miette!(render_repeated_validation_diagnostic( - "network endpoint ambiguity validation failed", - ambiguities.len(), - "ambiguous network endpoint selectors", - ))); - } - + let (mut data, warnings) = + validated_proto_data(proto, entrypoint_pid, require_binary_identity).map_err( + |invalid| match invalid { + ProtoPolicyInvalid::Policy(error) => { + miette::miette!(render_bounded_validation_diagnostics( + "policy validation failed", + error + .violations() + .iter() + .map(redacted_policy_violation_category), + )) + } + ProtoPolicyInvalid::Ambiguous(ambiguities) => { + miette::miette!(render_repeated_validation_diagnostic( + "network endpoint ambiguity validation failed", + ambiguities.len(), + "ambiguous network endpoint selectors", + )) + } + ProtoPolicyInvalid::L7(errors) => { + miette::miette!(render_repeated_validation_diagnostic( + "L7 policy validation failed", + errors.len(), + "invalid L7 policy configuration", + )) + } + ProtoPolicyInvalid::Internal => { + miette::miette!("internal: failed to parse proto JSON") + } + }, + )?; emit_binary_identity_mode(require_binary_identity, "proto"); - let data_json_str = proto_to_opa_data_json(&proto, entrypoint_pid); - - // Parse back to Value for preprocessing, then re-serialize - let mut data: serde_json::Value = serde_json::from_str(&data_json_str) - .map_err(|_| miette::miette!("internal: failed to parse proto JSON"))?; - inject_runtime_policy_data(&mut data, require_binary_identity); - normalize_endpoint_protocols(&mut data); - - // Validate BEFORE expanding presets - let (errors, warnings) = crate::l7::validate_l7_policies(&data); - if !errors.is_empty() { - return Err(miette::miette!(render_repeated_validation_diagnostic( - "L7 policy validation failed", - errors.len(), - "invalid L7 policy configuration", - ))); - } // Rejected candidates must not leak authored values through warnings. emit_l7_config_warnings(&warnings, "L7 policy validation warning"); @@ -2237,6 +2228,64 @@ fn l7_matchers_to_json( .collect() } +/// Why [`validated_proto_data`] rejected a policy. The messages name authored +/// values, so the runtime loader redacts them; policy.local returns them to +/// the agent that wrote the proposal. +#[derive(Debug)] +pub(crate) enum ProtoPolicyInvalid { + Policy(openshell_policy::PolicyValidationError), + Ambiguous(Vec), + L7(Vec), + Internal, +} + +impl std::fmt::Display for ProtoPolicyInvalid { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Policy(error) => write!(f, "{error}"), + Self::Ambiguous(ambiguities) => { + let messages: Vec = ambiguities.iter().map(ToString::to_string).collect(); + write!(f, "{}", messages.join("; ")) + } + Self::L7(errors) => write!(f, "{}", errors.join("; ")), + Self::Internal => write!(f, "internal: failed to parse proto JSON"), + } + } +} + +/// Validate a proto policy and project it to OPA data, without emitting +/// events. Returns the data and the L7 warnings for the caller to emit. +pub(crate) fn validated_proto_data( + proto: &ProtoSandboxPolicy, + entrypoint_pid: u32, + require_binary_identity: bool, +) -> std::result::Result<(serde_json::Value, Vec), ProtoPolicyInvalid> { + // Protobuf cannot distinguish an omitted repeated MCP version field + // from an empty one. Canonicalize before any runtime consumer reads + // the policy so both representations select the pinned default. + let proto = openshell_policy::validate_and_canonicalize_sandbox_policy(proto.clone()) + .map_err(ProtoPolicyInvalid::Policy)?; + + let ambiguities = openshell_policy::find_endpoint_ambiguities(&proto); + if !ambiguities.is_empty() { + return Err(ProtoPolicyInvalid::Ambiguous(ambiguities)); + } + + // Parse back to Value for preprocessing, then re-serialize + let mut data: serde_json::Value = + serde_json::from_str(&proto_to_opa_data_json(&proto, entrypoint_pid)) + .map_err(|_| ProtoPolicyInvalid::Internal)?; + inject_runtime_policy_data(&mut data, require_binary_identity); + normalize_endpoint_protocols(&mut data); + + // Validate BEFORE expanding presets + let (errors, warnings) = crate::l7::validate_l7_policies(&data); + if !errors.is_empty() { + return Err(ProtoPolicyInvalid::L7(errors)); + } + Ok((data, warnings)) +} + /// Convert typed proto policy fields to JSON suitable for `engine.add_data_json()`. /// /// The rego rules reference `data.*` directly, so the JSON structure has diff --git a/crates/openshell-supervisor-network/src/policy_local.rs b/crates/openshell-supervisor-network/src/policy_local.rs index bdf8898986..f9e62d0411 100644 --- a/crates/openshell-supervisor-network/src/policy_local.rs +++ b/crates/openshell-supervisor-network/src/policy_local.rs @@ -8,7 +8,7 @@ use miette::{IntoDiagnostic, Result}; use openshell_core::grpc_client::GatewayEndpoint; use openshell_core::proposals::AgentProposals; use openshell_core::proto::{ - L7Allow, L7DenyRule, L7Rule, NetworkBinary, NetworkEndpoint, NetworkPolicyRule, PolicyChunk, + L7Rule, McpOptions, NetworkBinary, NetworkEndpoint, NetworkPolicyRule, PolicyChunk, SandboxPolicy as ProtoSandboxPolicy, }; use openshell_ocsf::{ @@ -1227,11 +1227,18 @@ fn network_rule_from_json( .map(|binary| NetworkBinary { path: binary.path }) .collect(); - Ok(NetworkPolicyRule { + let rule = NetworkPolicyRule { name: rule.name.unwrap_or_default(), endpoints, binaries, - }) + }; + // Reject a rule the supervisor would refuse to load after approval. + let policy = ProtoSandboxPolicy { + network_policies: HashMap::from([("proposal".to_string(), rule.clone())]), + ..Default::default() + }; + crate::opa::validated_proto_data(&policy, 0, true).map_err(|invalid| invalid.to_string())?; + Ok(rule) } fn network_endpoint_from_json( @@ -1261,6 +1268,27 @@ fn network_endpoint_from_json( if ports.is_empty() { return Err("endpoint.port or endpoint.ports is required".to_string()); } + // A more specific endpoint path can replace the protocol and enforcement + // of an operator endpoint on the same host, so only operators set it. + if !endpoint.path.is_empty() { + return Err( + "endpoint.path is not supported in proposals; ask an administrator to add it" + .to_string(), + ); + } + // `tool` is shorthand for `params.name`; when both are set the policy + // conversion keeps `params.name` and drops `tool`. + if endpoint + .rules + .iter() + .any(|rule| rule.allow.tool.is_some() && rule.allow.params.contains_key("name")) + || endpoint + .deny_rules + .iter() + .any(|rule| rule.tool.is_some() && rule.params.contains_key("name")) + { + return Err("set either `tool` or `params.name` on a rule, not both".to_string()); + } if endpoint .rules .iter() @@ -1274,34 +1302,13 @@ fn network_endpoint_from_json( .rules .into_iter() .map(|rule| L7Rule { - allow: Some(L7Allow { - method: rule.allow.method, - path: rule.allow.path, - command: rule.allow.command, - query: HashMap::new(), - // GraphQL fields default empty — agent-authored proposals from - // policy.local target REST/SQL/L4 endpoints; GraphQL operation - // matching is set on the policy server side or via direct YAML. - operation_type: String::new(), - operation_name: String::new(), - fields: Vec::new(), - params: HashMap::new(), - }), + allow: Some(openshell_policy::allow_def_to_proto(rule.allow)), }) .collect(); let deny_rules = endpoint .deny_rules .into_iter() - .map(|rule| L7DenyRule { - method: rule.method, - path: rule.path, - command: rule.command, - query: HashMap::new(), - operation_type: String::new(), - operation_name: String::new(), - fields: Vec::new(), - params: HashMap::new(), - }) + .map(openshell_policy::deny_def_to_proto) .collect(); Ok(NetworkEndpoint { @@ -1327,13 +1334,16 @@ fn network_endpoint_from_json( allow_uninspected_credentials: false, provider_credentialed: false, advisor_proposed: false, - // GraphQL persisted-query knobs and path scoping default empty — - // agent proposals don't author them today. + // GraphQL persisted-query knobs default empty — agent proposals + // don't author them today. persisted_queries: String::new(), graphql_persisted_queries: HashMap::new(), graphql_max_body_bytes: 0, json_rpc_max_body_bytes: 0, - mcp: None, + mcp: endpoint.mcp.map(|mcp| McpOptions { + versions: mcp.versions, + ..Default::default() + }), path: String::new(), credential_signing: String::new(), signing_service: String::new(), @@ -1457,7 +1467,10 @@ struct ProposalRequest { operations: Vec, } +// Proposal rule structs reject unknown keys: a dropped field could widen the +// rule the reviewer approves. #[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] struct AddNetworkRuleJson { #[serde(default, rename = "ruleName")] rule_name: Option, @@ -1465,6 +1478,7 @@ struct AddNetworkRuleJson { } #[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] struct NetworkPolicyRuleJson { #[serde(default)] name: Option, @@ -1475,9 +1489,12 @@ struct NetworkPolicyRuleJson { } #[derive(Debug, Deserialize)] +#[serde(deny_unknown_fields)] struct NetworkEndpointJson { host: String, #[serde(default)] + path: String, + #[serde(default)] port: u32, #[serde(default)] ports: Vec, @@ -1494,45 +1511,36 @@ struct NetworkEndpointJson { #[serde(default)] allowed_ips: Vec, #[serde(default)] - deny_rules: Vec, + deny_rules: Vec, #[serde(default)] allow_encoded_slash: bool, + #[serde(default)] + mcp: Option, } #[derive(Debug, Deserialize)] -struct NetworkBinaryJson { - path: String, -} - -#[derive(Debug, Deserialize)] -struct L7RuleJson { - allow: L7AllowJson, +#[serde(deny_unknown_fields)] +struct McpJson { + versions: Vec, } #[derive(Debug, Deserialize)] -struct L7AllowJson { - #[serde(default)] - method: String, - #[serde(default)] +#[serde(deny_unknown_fields)] +struct NetworkBinaryJson { path: String, - #[serde(default)] - command: String, } #[derive(Debug, Deserialize)] -struct L7DenyRuleJson { - #[serde(default)] - method: String, - #[serde(default)] - path: String, - #[serde(default)] - command: String, +#[serde(deny_unknown_fields)] +struct L7RuleJson { + allow: openshell_policy_schema::L7Allow, } #[cfg(test)] mod tests { use super::*; use openshell_core::proposals::AgentProposals; + use openshell_core::proto::L7Allow; fn test_workspace_rx() -> tokio::sync::watch::Receiver { tokio::sync::watch::channel(String::new()).1 @@ -1693,6 +1701,83 @@ mod tests { ); } + #[test] + fn proposal_chunks_from_body_keeps_mcp_tool() { + let body = br#"{ + "operations": [{"addRule": {"ruleName": "wiki_mcp", "rule": { + "endpoints": [{"host": "mcp.example.org", "port": 443, + "protocol": "mcp", "enforcement": "enforce", + "mcp": {"versions": ["2025-11-25"]}, + "rules": [{"allow": {"method": "initialize"}}, + {"allow": {"method": "tools/call", "tool": "read_wiki_structure"}}], + "deny_rules": [{"method": "tools/call", "tool": "delete_*"}]}], + "binaries": [{"path": "/usr/bin/curl"}]}}}] + }"#; + + let chunks = proposal_chunks_from_body(body).unwrap(); + let endpoint = &chunks[0].proposed_rule.as_ref().unwrap().endpoints[0]; + assert_eq!( + endpoint.mcp.as_ref().unwrap().versions, + vec!["2025-11-25".to_string()] + ); + let tool_call = endpoint.rules[1].allow.as_ref().unwrap(); + assert_eq!(tool_call.method, "tools/call"); + assert_eq!(tool_call.params["name"].glob, "read_wiki_structure"); + assert_eq!(endpoint.deny_rules[0].params["name"].glob, "delete_*"); + } + + #[test] + fn proposal_chunks_from_body_rejects_rules_the_supervisor_cannot_load() { + for (endpoint, reason) in [ + ( + r#"{"host":"mcp.example.org","port":443,"path":"/mcp","protocol":"mcp"}"#, + "endpoint.path", + ), + ( + r#"{"host":"mcp.example.org","port":443,"protocol":"mcp","rules":[{"allow":{"method":"tools/call","tool":"read_x","params":{"name":"*"}}}]}"#, + "not both", + ), + ( + r#"{"host":"api.example.com","port":443,"protocol":"rest","rules":[{"allow":{"method":"POST","path":"/x","tool":"read_x"}}]}"#, + "params matching is only valid for protocol mcp", + ), + ( + r#"{"host":"mcp.example.org","port":443,"protocol":"mcp","rules":[{"allow":{"method":"tools/call","params":{"arguments":"*"}}}]}"#, + "MCP tool argument matching is not supported yet", + ), + ( + r#"{"host":"api.example.com","port":443,"protocol":"rest","mcp":{"versions":["2025-11-25"]}}"#, + "cannot configure mcp options", + ), + ] { + let body = format!( + r#"{{"operations": [{{"addRule": {{"ruleName": "bad", "rule": {{"endpoints": [{endpoint}]}}}}}}]}}"# + ); + + let error = proposal_chunks_from_body(body.as_bytes()).unwrap_err(); + assert!( + error.contains(reason), + "{endpoint}: unexpected error: {error}" + ); + } + } + + #[test] + fn proposal_chunks_from_body_rejects_unknown_rule_fields() { + for endpoint in [ + r#"{"host":"api.example.com","port":443,"credential_signing":"aws_sigv4"}"#, + r#"{"host":"api.example.com","port":443,"rules":[{"allow":{"method":"GET","unknown":"x"}}]}"#, + r#"{"host":"mcp.example.org","port":443,"protocol":"mcp","mcp":{"allow_all_known_mcp_methods":true}}"#, + ] { + let body = format!( + r#"{{"operations": [{{"addRule": {{"ruleName": "extra", "rule": {{"endpoints": [{endpoint}]}}}}}}]}}"# + ); + + let error = proposal_chunks_from_body(body.as_bytes()).unwrap_err(); + assert!(error.contains("unknown field"), "unexpected error: {error}"); + } + } + #[test] fn parse_last_query_clamps_to_max() { assert_eq!(parse_last_query("last=5"), Some(5)); diff --git a/crates/openshell-supervisor-process/src/skills/policy_advisor.md b/crates/openshell-supervisor-process/src/skills/policy_advisor.md index d8facc839b..ec8877b2b3 100644 --- a/crates/openshell-supervisor-process/src/skills/policy_advisor.md +++ b/crates/openshell-supervisor-process/src/skills/policy_advisor.md @@ -146,6 +146,10 @@ agent's `/wait` returns approved in ~1 second. When the prover does find something — or the setting is `"manual"`/unset — the chunk lands in `pending` for human review. +MCP tool permissions are not modeled by the prover. A policy containing MCP +rules reports `prover: unsupported MCP rules; manual review required` and stays +pending for human approval, even in automatic mode. + The prover answers four formal questions about each proposed change. Each "yes" answer is its own categorical finding — there is no severity grade. Any finding blocks auto-approval. @@ -213,6 +217,8 @@ The new submission wins by structural overlap. TCP or a raw TLS tunnel, ask an administrator to add that access explicitly. - Do not include query strings, tokens, credentials, or secret values in paths. +- For an MCP server, name the tool with `tool` on the `tools/call` allow + rule. A `tools/call` allow without `tool` permits every tool. - Explain uncertainty in `intent_summary` instead of widening the rule. - If pushing with `git` fails, that is a separate L4 or protocol-specific path from GitHub REST API access. Propose it separately. diff --git a/crates/openshell-tui/src/ui/sandbox_draft.rs b/crates/openshell-tui/src/ui/sandbox_draft.rs index df764744c9..3bff9c387e 100644 --- a/crates/openshell-tui/src/ui/sandbox_draft.rs +++ b/crates/openshell-tui/src/ui/sandbox_draft.rs @@ -897,10 +897,15 @@ fn endpoint_layer_label(endpoint: &NetworkEndpoint) -> &str { fn format_allow_rule(allow: &L7Allow) -> String { let mut parts = Vec::new(); - if !allow.method.is_empty() || !allow.path.is_empty() { + let method = if allow.method.is_empty() && allow.params.contains_key("name") { + "tools/call" + } else { + &allow.method + }; + if !method.is_empty() || !allow.path.is_empty() { parts.push(format!( "{} {}", - non_empty_or(&allow.method, "*"), + non_empty_or(method, "*"), non_empty_or(&allow.path, "*") )); } @@ -917,20 +922,26 @@ fn format_allow_rule(allow: &L7Allow) -> String { if !allow.fields.is_empty() { parts.push(format!("fields {}", allow.fields.join(","))); } - append_query_matchers(&mut parts, &allow.query); + append_matchers(&mut parts, "query", &allow.query); + append_matchers(&mut parts, "params", &allow.params); if parts.is_empty() { "*".to_string() } else { - parts.join("; ") + parts.join("; ").escape_debug().to_string() } } fn format_deny_rule(deny: &L7DenyRule) -> String { let mut parts = Vec::new(); - if !deny.method.is_empty() || !deny.path.is_empty() { + let method = if deny.method.is_empty() && deny.params.contains_key("name") { + "tools/call" + } else { + &deny.method + }; + if !method.is_empty() || !deny.path.is_empty() { parts.push(format!( "{} {}", - non_empty_or(&deny.method, "*"), + non_empty_or(method, "*"), non_empty_or(&deny.path, "*") )); } @@ -947,22 +958,24 @@ fn format_deny_rule(deny: &L7DenyRule) -> String { if !deny.fields.is_empty() { parts.push(format!("fields {}", deny.fields.join(","))); } - append_query_matchers(&mut parts, &deny.query); + append_matchers(&mut parts, "query", &deny.query); + append_matchers(&mut parts, "params", &deny.params); if parts.is_empty() { "*".to_string() } else { - parts.join("; ") + parts.join("; ").escape_debug().to_string() } } -fn append_query_matchers( +fn append_matchers( parts: &mut Vec, - query: &std::collections::HashMap, + label: &str, + matchers: &std::collections::HashMap, ) { - if query.is_empty() { + if matchers.is_empty() { return; } - let mut entries: Vec<_> = query.iter().collect(); + let mut entries: Vec<_> = matchers.iter().collect(); entries.sort_by_key(|(key, _)| *key); let formatted = entries .into_iter() @@ -975,7 +988,7 @@ fn append_query_matchers( }) .collect::>() .join(", "); - parts.push(format!("query {formatted}")); + parts.push(format!("{label} {formatted}")); } fn non_empty_or<'a>(value: &'a str, fallback: &'a str) -> &'a str { @@ -1001,6 +1014,109 @@ mod tests { use ratatui::Terminal; use ratatui::backend::TestBackend; + #[test] + fn draft_mcp_tool_selectors_are_visible_in_summary_and_details() { + for (selector, expected) in [ + ("delete_repo", "params name=delete_repo"), + ("{any: [read_*, list_*]}", "params name in [read_*,list_*]"), + ] { + let policy = openshell_policy::parse_sandbox_policy(&format!( + r" +version: 1 +network_policies: + mcp: + endpoints: + - host: allow.example.com + port: 443 + protocol: mcp + mcp: + allow_all_known_mcp_methods: true + rules: + - allow: + tool: {selector} + - host: deny.example.com + port: 443 + protocol: mcp + enforcement: enforce + rules: + - allow: + method: tools/call + deny_rules: + - method: tools/call + tool: {selector} + binaries: + - path: /usr/bin/curl +" + )) + .unwrap(); + let endpoints = &policy.network_policies["mcp"].endpoints; + assert!(format_endpoint_summary(&endpoints[0]).contains(expected)); + assert!( + format_endpoint_details(&endpoints[0]) + .iter() + .any(|line| line == &format!("Allow: tools/call *; {expected}")) + ); + assert!(format_endpoint_summary(&endpoints[1]).contains(expected)); + let details = format_endpoint_details(&endpoints[1]); + assert!(details.iter().any(|line| line == "Allow: tools/call *")); + assert!( + details + .iter() + .any(|line| line == &format!("Deny: tools/call *; {expected}")) + ); + } + } + + #[test] + fn draft_rule_matchers_escape_controls_and_preserve_other_protocols() { + let params = std::collections::HashMap::from([( + "name".to_string(), + L7QueryMatcher { + glob: "read_public\u{1b}[2J\nWRITE".to_string(), + ..Default::default() + }, + )]); + let allow = L7Allow { + method: "tools/call".to_string(), + params: params.clone(), + ..Default::default() + }; + let deny = L7DenyRule { + method: "tools/call".to_string(), + params, + ..Default::default() + }; + for rendered in [format_allow_rule(&allow), format_deny_rule(&deny)] { + assert!(!rendered.chars().any(char::is_control)); + assert!(rendered.contains(r"read_public\u{1b}[2J\nWRITE")); + } + let query = std::collections::HashMap::from([( + "state".to_string(), + L7QueryMatcher { + glob: "open".to_string(), + ..Default::default() + }, + )]); + assert_eq!( + format_allow_rule(&L7Allow { + method: "GET".to_string(), + path: "/repos/**".to_string(), + query, + ..Default::default() + }), + "GET /repos/**; query state=open" + ); + assert_eq!( + format_deny_rule(&L7DenyRule { + operation_type: "mutation".to_string(), + operation_name: "DeleteRepo".to_string(), + fields: vec!["deleteRepository".to_string()], + ..Default::default() + }), + "graphql mutation DeleteRepo; fields deleteRepository" + ); + } + /// Column width computed independently of the production `display_width`. /// /// Asserting with the helper under test would be tautological: if it diff --git a/docs/how-it-works/policies/advisor.mdx b/docs/how-it-works/policies/advisor.mdx index c77e2a77d4..14eb9ee04f 100644 --- a/docs/how-it-works/policies/advisor.mdx +++ b/docs/how-it-works/policies/advisor.mdx @@ -210,15 +210,25 @@ A proposal with any prover finding or flagged destination needs your review, even in automatic mode. When you reject a proposal, the agent receives the findings with your reason, so it can narrow its next attempt. -If either policy cannot be analyzed, the proposal shows `validation unavailable` -and cannot be approved until validation succeeds. +The prover does not model MCP tool permissions. If either policy contains MCP +rules, the proposal reports `prover: unsupported MCP rules; manual review required` +and waits for human approval, even in automatic mode. + +Other analysis failures show `validation unavailable` and block approval until +validation succeeds. ## What Agents Can Propose +This stricter proposal validation targets the next minor OpenShell release. +Earlier releases ignored unsupported proposal fields. When upgrading, remove +those fields and resubmit the proposal. Put settings reserved for operators, +such as endpoint path selectors, in your policy file. + An agent proposes endpoints and binaries. An endpoint can include an access -preset, method and path allow rules, deny rules, `allowed_ips`, and -`allow_encoded_slash`. A good proposal allows one method on the narrowest path -the task needs: +preset, allow and deny rules, `allowed_ips`, `allow_encoded_slash`, and +`mcp.versions`. Allow and deny rules take the same fields as in a policy file, +including query matchers and MCP `tool` and `params`. A good proposal allows +one method on the narrowest path the task needs: ```json { @@ -258,9 +268,10 @@ the task needs: ``` Agents cannot propose `protocol: tcp` or `tls: skip`, because OpenShell cannot -inspect that traffic. The API also has no fields for GraphQL operation rules, -MCP tool rules, query matchers, credential settings, or endpoint path selectors, -and it ignores them if an agent sends them. Review the rule that +inspect that traffic. The API also has no fields for endpoint path selectors, +credential settings, GraphQL persisted queries, or MCP options other than +`versions`, and it rejects a proposal that sends a field it does not support or +a rule the sandbox could not load. Review the rule that `openshell rule get` shows, not the agent's description of it. Add rules that need those fields yourself, as described in [Manage Sandbox Policies](/how-it-works/policies/manage-policies).