Skip to content

fix(policy)!: keep MCP tool selectors in agent proposals #4736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shiju-nv wants to merge 1 commit into
base: main
Choose a base branch
from
+365 −107
Open

fix(policy)!: keep MCP tool selectors in agent proposals #4736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 120 additions & 4 deletions crates/openshell-cli/src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> = Vec::new();
Expand All @@ -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<String, openshell_core::proto::L7QueryMatcher>,
) {
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)]
Expand Down Expand Up @@ -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]
Expand Down
13 changes: 6 additions & 7 deletions crates/openshell-policy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -534,14 +536,11 @@ fn to_proto(raw: PolicyFile) -> Result<SandboxPolicy> {
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,
Expand Down
129 changes: 89 additions & 40 deletions crates/openshell-supervisor-network/src/opa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,47 +469,38 @@ impl OpaEngine {
entrypoint_pid: u32,
require_binary_identity: bool,
) -> Result<Self> {
// 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");

Expand Down Expand Up @@ -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<openshell_policy::EndpointAmbiguity>),
L7(Vec<String>),
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<String> = 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<String>), 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
Expand Down
Loading
Loading