refactor(policy): extract shared L7 endpoint validation#2389
refactor(policy): extract shared L7 endpoint validation#2389gracesmith6504 wants to merge 1 commit into
Conversation
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR is project-valid because it implements the confirmed provider-v2 lint/runtime validation gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: 73f81c6673a825dde5b41c8f1d8710b5cb89a1f9
Review findings:
- Changes are requested for collection-presence regression, mixed-case MCP validation/runtime inconsistency, and divergent malformed-rule classification.
- One additional suggestion asks the author to preserve or explicitly test the intended JSON-RPC diagnostic aggregation.
Docs: Not needed; this restores lint/runtime parity for already-documented endpoint semantics and does not introduce new user-facing behavior.
Next state: gator:in-review pending an author update.
| pub access: &'a str, | ||
|
|
||
| /// `true` when the endpoint has a non-empty rules list. | ||
| pub has_rules: bool, |
There was a problem hiding this comment.
gator-agent
Warning: has_rules conflates an absent rules field with an explicitly empty rules: []. The old runtime validator explicitly rejected an empty list, but that check is removed, so an L4 endpoint with rules: [] now produces no L7 error and rules: [] plus access is accepted. Preserve presence separately or retain the runtime-specific empty-list check, with regression coverage for absent versus empty collections.
| impl L7Protocol { | ||
| /// Parse a protocol string into a known variant. | ||
| pub fn parse(s: &str) -> Option<Self> { | ||
| match s.to_ascii_lowercase().as_str() { |
There was a problem hiding this comment.
gator-agent
Warning: Case-insensitive parsing now drives the shared MCP exemptions, but downstream validation and expand_access_presets still compare the authored string to lowercase "mcp". For example, protocol: MCP with allow_all_known_mcp_methods: true and no rules now passes semantic validation, but the runtime does not synthesize the MCP allow-all rule. Normalize protocol values before all downstream processing or reject noncanonical casing, then add a validation-to-expansion regression test.
| ep.get("rules") | ||
| .and_then(|v| v.as_array()) | ||
| .is_some_and(|rules| { | ||
| !rules.is_empty() && rules.iter().all(|rule| rule.get("allow").is_none()) |
There was a problem hiding this comment.
gator-agent
Warning: The two callers still disagree on what constitutes a missing allow clause: profile lint uses Option::is_none, while this adapter only detects a missing JSON key. allow: null passes here, and proto-to-OPA conversion turns allow: None into an empty allow object, so configurations rejected by lint can pass runtime semantic validation. Preserve optionality through conversion or pass a shared rule-shape classification, and test the same malformed rule through both callers.
| } | ||
|
|
||
| // 4. json-rpc requires explicit rules | ||
| if is_jsonrpc && !ep.has_rules && ep.access.is_empty() { |
There was a problem hiding this comment.
gator-agent
Suggestion: This changes runtime error aggregation: JSON-RPC without rules previously emitted both the explicit-rules and base-allow errors, while JSON-RPC with an access preset also emitted the missing-rules error. If reducing redundant diagnostics is intentional, add exact error-list tests for these combinations; otherwise preserve the previous aggregation.
johntmyers
added
the
gator:in-review
73f81c6 to
e26f89f
Compare
|
Addressed all comments: |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head e26f89ffb1c7f09d3879c1a7422ab1f96b74e2d3 after your July 22 update. The case-sensitive protocol parsing and exact JSON-RPC diagnostic tests resolve those two prior concerns, and the runtime adapter now recognizes protobuf-converted empty allow objects.
Validation: This remains project-valid and implements the confirmed provider-v2 lint/runtime gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: e26f89ffb1c7f09d3879c1a7422ab1f96b74e2d3
Review findings:
- Two lint/runtime parity gaps remain in the provider-profile adapter: explicit
rules: []presence is lost during deserialization, andallow: {}is not classified as deny-all during lint. - Please add adapter-level parity coverage for these representations (and MCP allow-all) so the profile and runtime paths cannot drift while their field bags differ.
Docs: Not needed; this surfaces existing, already-documented endpoint constraints earlier and introduces no new user-facing schema or workflow.
Next state: gator:in-review pending an author update.
| let l7_fields = L7EndpointFields { | ||
| protocol: &endpoint.protocol, | ||
| access: &endpoint.access, | ||
| has_rules: !endpoint.rules.is_empty(), |
There was a problem hiding this comment.
gator-agent
Warning: EndpointProfile.rules is a defaulted Vec, so omitted rules and explicit rules: [] are indistinguishable here. A profile containing access: full and rules: [] passes lint and converts to protobuf without rules, while direct runtime policy validation rejects that contradictory input. Preserve field presence during deserialization and reject explicitly empty lists before conversion.
| 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()), |
There was a problem hiding this comment.
gator-agent
Warning: allow: {} deserializes as Some(L7AllowProfile) with every effective selector empty, so this computes rules_would_deny_all = false. Runtime validation treats the protobuf-converted empty allow object as deny-all, meaning lint accepts a profile that later prevents sandbox startup. Treat None and an allow object whose method, path, command, operation fields, query, and fields are all empty identically, preferably through a shared predicate.
e26f89f to
c422a82
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head c422a8272dadb86024a795a9e1edc6f01e3b5041 after your July 22 update about empty-list handling, case-sensitive protocol parsing, proto-converted deny-all detection, and JSON-RPC diagnostics.
What I checked: the full current diff, both profile/runtime adapters, serde and protobuf conversions, MCP allow-all semantics, and the new regression coverage. The explicit rules: [] and REST allow: {} concerns are resolved.
Validation: This remains project-valid and implements the confirmed provider-v2 lint/runtime gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: c422a8272dadb86024a795a9e1edc6f01e3b5041
Review findings:
- One blocking MCP regression is anchored inline: selector-only MCP rules accepted under
mcp.allow_all_known_mcp_methods: trueare now misclassified as deny-all before alias normalization. - Please also add profile-adapter coverage for MCP allow-all with omitted rules and verify protobuf round-trip preservation; the shared validator unit test alone does not protect the adapter boundary.
Docs: Not needed; the intended MCP behavior is already documented, and this PR introduces no new user-facing schema or workflow.
Next state: gator:in-review pending an author update.
| .unwrap_or("") | ||
| .is_empty() | ||
| }; | ||
| str_empty("method") |
There was a problem hiding this comment.
gator-agent
High: This deny-all predicate only recognizes five string fields, so a supported MCP rule containing only tool or params.name selectors is classified as deny-all when mcp_allow_all_known_mcp_methods is true. validate_jsonrpc_rule_fields permits the omitted method and normalize_l7_rule_aliases later supplies tools/call, but validation now fails before normalization. Make this predicate MCP-profile-aware (and mirror the same semantics in the profile adapter), or normalize aliases before computing it; add a regression test asserting an exact empty error list for a selector-only MCP policy.
c422a82 to
88e367c
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head 88e367cf79e0c58045ef50ca33737c1a960cadd0 after your update addressing empty-list presence, MCP selector handling, and protobuf round-trip coverage.
What I checked: the complete eight-file diff, the profile/YAML → protobuf → runtime JSON → alias-normalization → Rego path, the prior review concerns, and adapter-boundary coverage. The explicit empty-list, case-sensitive protocol, JSON-RPC diagnostic, and selector-only MCP false-positive concerns are resolved.
Validation: This remains project-valid and implements the confirmed Provider V2 lint/runtime validation gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: 88e367cf79e0c58045ef50ca33737c1a960cadd0
Review findings:
- One high-severity MCP authorization regression remains: mixing a tool-specific rule with an empty allow rule under the MCP method profile can normalize to an implicit wildcard and bypass the tool restriction.
- Two profile-adapter parity gaps remain for MCP deny-rule params round-tripping and unsupported params classification.
Docs: Not needed; this restores validation parity for an already documented policy schema and introduces no new workflow or navigation.
Next state: gator:in-review pending an author update.
| .and_then(|v| v.as_array()) | ||
| .is_some_and(|rules| { | ||
| !rules.is_empty() | ||
| && rules.iter().all(|rule| { |
There was a problem hiding this comment.
gator-agent
High — CWE-863 (Incorrect Authorization): Using rules.iter().all(...) only rejects an endpoint when every allow rule is empty. With mcp.allow_all_known_mcp_methods: true, a tool-specific rule plus allow: {} passes validation; post-validation normalization rewrites the empty rule to method: "*", which allows every tool and bypasses the selector. Reject each effectively empty allow entry before normalization, or make the MCP broad-rule conflict check account for this implicit wildcard. Please add a profile → protobuf → OPA regression for this mixed rule set.
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub fields: Vec<String>, | ||
| #[serde(default, skip_serializing_if = "HashMap::is_empty")] | ||
| pub params: HashMap<String, L7QueryMatcherProfile>, |
There was a problem hiding this comment.
gator-agent
Warning: Adding params only to L7AllowProfile leaves deny rules lossful. L7DenyRule.params is supported by the protobuf and runtime, but L7DenyRuleProfile lacks the field, deny_rule_to_proto() writes an empty map, and deny_rule_from_proto() discards it. An MCP tool-specific deny can consequently become a broad deny after profile import/export. Add the corresponding deny-profile field, bidirectional conversions, and a matcher-content round-trip test.
| && self.command.is_empty() | ||
| && self.operation_type.is_empty() | ||
| && self.operation_name.is_empty() | ||
| && self.params.is_empty() |
There was a problem hiding this comment.
gator-agent
Warning: Treating any non-empty params map as a tool selector diverges from the runtime adapter, which only recognizes params.name. A profile containing only unsupported params.arguments therefore avoids the lint deny-all diagnostic but is rejected at runtime. Match the runtime predicate, such as checking contains_key("name"), and add an adapter-parity regression.
88e367c to
4e2a61a
Compare
|
Addressed all three findings from the review on 88e367c:
Ready for another look. |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head 4e2a61a79204f27dfad6cacf59182ea0e00b4890 after your July 22 update about the implicit MCP wildcard, deny-rule params round-tripping, and unsupported params classification.
What I checked: the complete current diff, Provider V2 YAML/profile conversions, shared lint/runtime validation, MCP alias normalization, Rego authorization behavior, and adapter-boundary tests. Your three stated fixes resolve the previously reported representations.
Validation: This remains project-valid and implements the confirmed Provider V2 lint/runtime gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: 4e2a61a79204f27dfad6cacf59182ea0e00b4890
Review findings:
- Two remaining profile-adapter parity defects are anchored inline. One can silently discard the documented MCP
toolselector and broaden authorization; the other misses explicit broad MCP allow/deny conflicts that runtime validation rejects. - Please add adapter coverage for the
toolalias, explicit broad allow/deny matchers, MCP allow-all with omitted rules, and exact matcher values across round trips.
Docs: No Fern update is needed for this refactor; the Provider V2 docs already promise the network-policy endpoint shape, so the tool behavior should be preserved or explicitly rejected in code.
Next state: gator:in-review pending an author update.
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub fields: Vec<String>, | ||
| #[serde(default, skip_serializing_if = "HashMap::is_empty")] | ||
| pub params: HashMap<String, L7QueryMatcherProfile>, |
There was a problem hiding this comment.
gator-agent
Critical — CWE-863 (Incorrect Authorization): This profile rule DTO adds params but still omits the documented MCP tool alias, and unknown fields are silently ignored. With allow_all_known_mcp_methods: true, a profile containing allow: { tool: read_* } plus another non-empty rule can pass lint after losing the selector; protobuf/runtime JSON then contains an empty allow, alias normalization turns it into method: "*", and Rego can authorize every tool call. Add tool to the allow and deny profile DTOs and lower it to params.name, rejecting simultaneous tool and params.name and including it in emptiness/conflict classification. Alternatively, reject the field explicitly instead of silently broadening authorization.
| .allow | ||
| .as_ref() | ||
| .is_none_or(|a| !a.params.contains_key("name")); | ||
| let method_empty = rule.allow.as_ref().is_none_or(|a| a.method.is_empty()); |
There was a problem hiding this comment.
gator-agent
Warning: Profile lint only detects the new empty-method implicit wildcard, while runtime also rejects explicit broad allow matchers such as tools/call/tools/* and equivalent broad deny rules when tool-specific allows exist. The outer allow_all_known_mcp_methods guard also skips explicit conflicts when that option is false. Reuse the runtime method-conflict predicate for profile allow and deny rules, limit the allow-all condition to omitted methods, and add parity tests for broad allow and broad deny cases.
4e2a61a to
b1b8db5
Compare
|
Addressed both findings from the review on 4e2a61a:
Ready for another look. |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head b1b8db51f764945091cba1dc3e2b0dfa044aec2f after your July 22 update about preserving the MCP tool alias and matching runtime broad-method behavior.
What I checked: the complete eight-file diff, provider-profile deserialization and protobuf lowering, shared lint/runtime validation, MCP alias normalization, OPA authorization, and the new adapter tests. The prior tool-discard and broad tools/call/*/tools/* parity findings are resolved for the object-matcher paths covered by this head.
Validation: This remains project-valid and implements the confirmed Provider V2 lint/runtime gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: b1b8db51f764945091cba1dc3e2b0dfa044aec2f
Review findings:
- Two profile-adapter defects are anchored inline: documented scalar MCP matchers do not deserialize, and deny-only dual selectors can bypass lint and silently discard
toolduring lowering. - One additional lint/runtime parity gap remains: MCP
toolorparams.nameselectors are accepted withoutmethod: tools/callwhen allow-all is disabled, and with incompatible methods. Runtime rejects those shapes. Please mirror the runtime selector/method contract for both allow and deny rules and add missing/incompatible-method lint coverage.
Docs: No Fern update is needed for this refactor. Existing docs already describe provider endpoints as the sandbox policy shape and document scalar-or-object MCP matchers; the implementation should preserve that contract.
Next state: gator:in-review pending an author update.
| #[serde(default, skip_serializing_if = "HashMap::is_empty")] | ||
| pub params: HashMap<String, L7QueryMatcherProfile>, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub tool: Option<L7QueryMatcherProfile>, |
There was a problem hiding this comment.
gator-agent
Warning: The documented scalar form tool: search_web cannot deserialize into L7QueryMatcherProfile, which only accepts an object. Scalar params.name has the same problem. Please use a string-or-object matcher representation consistent with the sandbox policy schema and add scalar allow/deny round-trip tests.
| }) | ||
| }); | ||
|
|
||
| if has_tool_allow_selectors { |
There was a problem hiding this comment.
gator-agent
Warning: Move tool plus params.name mutual-exclusion validation outside this has_tool_allow_selectors guard. A deny-only MCP profile can otherwise contain both selectors, pass lint, and silently lose tool during proto lowering. Keep only the broad allow/deny conflict detection under this guard and add a deny-only regression test.
b1b8db5 to
cebeb9e
Compare
|
Addressed all three findings from the review on b1b8db5:
5 new tests added (106 total), all passing. Ready for another look. |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head cebeb9e855c78276a2aef18a25ce18cb85882695 after your July 22 update about preserving the MCP tool alias and matching broad-method behavior.
What I checked: the complete current diff, Provider V2 YAML/profile-to-protobuf import path, server-side lint, runtime validation, alias normalization, Rego authorization behavior, and the new adapter coverage. The scalar matcher and selector/method compatibility fixes resolve the corresponding prior findings.
Validation: This remains project-valid and implements the confirmed Provider V2 lint/runtime gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: cebeb9e855c78276a2aef18a25ce18cb85882695
Review findings:
- Three remaining lint/import parity defects are anchored inline.
- The highest-severity issue lets protobuf lowering erase a conflicting MCP selector before server-side validation, changing the authored authorization intent.
- The CLI-to-server protobuf boundary also loses empty-list presence, and profile lint still omits several MCP matcher checks enforced at runtime.
Docs: No Fern update is needed for the intended parity refactor; this should preserve the already documented policy contract.
Next state: gator:in-review pending an author update.
| .collect(); | ||
| if let Some(tool) = &rule.tool { | ||
| params | ||
| .entry("name".to_string()) |
There was a problem hiding this comment.
gator-agent
High — CWE-20 (Improper Input Validation): The normal import path converts YAML to protobuf before server-side validate_profile_set(). When a deny rule contains both tool and params.name, this insertion keeps params.name and silently discards tool, so the new mutual-exclusion diagnostic never sees both; the allow path has the same issue at line 1122. Validate the raw DTO before lowering or make lowering reject the conflict, and add an end-to-end import test.
| access: endpoint.access.clone(), | ||
| enforcement: endpoint.enforcement.clone(), | ||
| rules: endpoint.rules.iter().map(rule_from_proto).collect(), | ||
| rules: if endpoint.rules.is_empty() { |
There was a problem hiding this comment.
gator-agent
Warning: Protobuf repeated fields cannot distinguish omission from an explicit empty list. Because CLI imports cross protobuf before server lint, this maps both representations to None; rules: [] with access: full and deny_rules: [] therefore bypass the new presence-based diagnostics. Validate before protobuf conversion, add wire-level presence, or otherwise preserve and test these distinctions through the real CLI-to-server path.
| )); | ||
| } | ||
|
|
||
| if endpoint.protocol == "mcp" { |
There was a problem hiding this comment.
gator-agent
Warning: This profile-lint block mirrors only part of runtime MCP rule validation. It still accepts unsupported params keys, malformed or empty matchers, simultaneous glob and any, and wildcard tool matchers with strict_tool_names: false; runtime validation rejects them later at sandbox startup. Share the per-rule validator or mirror those checks here, with profile-import regression coverage.
cebeb9e to
b1d4deb
Compare
|
Comment 16 – pre-lowering validation for tool/params.name Added validate_before_lowering() on ProviderTypeProfile, called in the CLI's Comment 17 – empty list presence checks rules: [] and deny_rules: [] collapse to None after proto round-trip, silently Comment 18 – runtime MCP per-rule parity Added the missing error-level checks from the runtime's per-rule MCP validation to
Six regression tests for the matcher checks plus one each for method globs, path 120 tests passing, CI green. |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head b1d4deb2a3424c6c4a5e1ab2fdce9c81bfc06173 after your July 22 update about pre-lowering selector/list validation and the missing MCP per-rule checks.
What I checked: the complete current diff, CLI profile import/lint path, catalog/static-profile path, profile-to-protobuf conversion, shared and runtime validators, regression coverage, and the existing Fern policy/provider-profile documentation. Your three stated fixes resolve the prior findings.
Validation: This remains project-valid and implements the confirmed Provider V2 lint/runtime validation gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: b1d4deb2a3424c6c4a5e1ab2fdce9c81bfc06173
Review findings:
- One remaining validation-path defect is anchored inline: catalog/static profiles discard new L7 diagnostics that do not map to a legacy
ProfileError. - The existing Fern docs also need correction:
docs/reference/policy-schema.mdxshould state the conditional non-emptyrules/accessrequirement, and the example atdocs/sandboxes/providers-v2.mdx:235currently combines mutually exclusiveaccessandrules. Navigation changes are not needed. - One inline suggestion preserves the structured endpoint location in CLI pre-lowering errors.
Docs: Required because lint-visible policy behavior is changing and the current provider-profile example is rejected by this contract.
E2E: No test:* label is needed; this does not change sandbox lifecycle, gateway/supervisor interaction, enforcement behavior, proxy behavior, credential flow, drivers, or runtime packaging.
Next state: gator:in-review pending an author update.
| .and_then(|opts| opts.allow_all_known_mcp_methods) | ||
| .unwrap_or(false), | ||
| }; | ||
| for msg in validate_l7_endpoint_semantics(&l7_fields) { |
There was a problem hiding this comment.
gator-agent
Warning: parse_profile_catalog_yamls() reaches these new diagnostics through validate_profiles(), but that function returns Ok(()) whenever the first diagnostic does not match one of its legacy ProfileError mappings. That means a built-in/static profile with invalid L7 semantics can still load and fail later at sandbox startup. Add a generic validation error (or otherwise propagate every unmatched error diagnostic) and a catalog-path regression test.
b1d4deb to
1cac830
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @gracesmith6504. I reviewed current head 1cac830a3598637db5251a0eac956cb8a26e4077 after your July 22 updates addressing the catalog/static-profile validation path, structured CLI diagnostics, and Fern documentation.
What I checked: the complete current diff; provider YAML deserialization and pre-lowering validation; protobuf, server, and catalog paths; runtime validation and alias normalization; authorization-sensitive matcher handling; regression coverage; and the corrected policy/provider-profile documentation. The previous catalog propagation, CLI diagnostic-location, conditional-requirement docs, and invalid provider example findings are resolved.
Validation: This remains project-valid and implements the confirmed Provider V2 lint/runtime validation gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: 1cac830a3598637db5251a0eac956cb8a26e4077
Review findings:
- Five remaining findings are anchored inline: one critical fail-open matcher-deserialization risk, three lint/runtime or normalization parity defects, and one documentation mismatch.
- Author changes are required before pipeline monitoring.
Docs: The earlier example and conditional-requirement updates are present, but the new constraints still need the SQL and MCP deny-base exceptions corrected.
E2E: No test:* label is needed; this changes validation/lint paths and documentation without changing sandbox lifecycle, gateway/supervisor interaction, runtime policy enforcement behavior, proxy behavior, credential flow, drivers, or packaging.
Next state: gator:in-review pending an author update.
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<Vec<...>> 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 <gsmith@redhat.com>
Signed-off-by: Grace Smith <grasmith@redhat.com>
1cac830 to
3be623a
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
I reviewed current head 3be623ab1c8d16d70e9763318aeb9c97878ce063 against the complete diff and @gracesmith6504's earlier updates about matcher safety, MCP allow-all normalization, profile/runtime parity, and Fern documentation.
What I checked: Provider V2 YAML deserialization, pre-lowering validation, protobuf conversion, runtime validation and alias normalization, authorization-sensitive MCP rule handling, generic JSON-RPC/query matcher parity, regression coverage, and the updated policy-schema docs. The earlier SQL requirement and MCP deny-base documentation issues are resolved, and runtime enforcement remains fail-closed.
Validation: This remains project-valid and implements the confirmed Provider V2 lint/runtime validation gap in #1714 using the shared policy-layer direction recommended during maintainer triage.
Head SHA: 3be623ab1c8d16d70e9763318aeb9c97878ce063
Review findings:
- Seven actionable findings are anchored inline: one input-shape safety issue, four lint/runtime or normalization parity defects, one protocol parsing compatibility regression, and one Fern documentation omission.
- I found no direct authorization fail-open, but author changes and focused regression tests are required before pipeline monitoring.
Docs: The SQL and MCP exception text is now accurate, but the newly enforced empty deny_rules constraint is not documented. Navigation changes are not needed.
E2E: Deferred until review findings are resolved; no test label is being applied in this cycle.
Next state: gator:in-review pending an author update.
|
Hi @johntmyers, thanks for the thorough reviews. I've learned a lot I wanted to ask about scope. The original issue (#1714) The later rounds have expanded into full MCP Would it make sense to land the core shared validation in |
|
/ok to test 3be623a |
Yes I think that makes perfect sense. |
|
@gracesmith6504 I'm trying out a little guardrail with the gator agent to see if it can stop scope creeping but will try and get this merged today. do you have anything else you're planning to commit? |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Scope-Focused Re-review
Thanks @gracesmith6504 for calling out the scope expansion. @johntmyers agreed that full MCP matcher tool/params parity, generic query matcher validation, JSON-RPC per-rule checks, and the other pre-existing broad parity gaps belong in follow-up work. I re-reviewed the complete current diff on that reduced basis.
Validation: This PR remains project-valid and addresses #1714 by sharing the listed L7 endpoint semantic checks between Provider v2 profile lint and runtime validation.
Head SHA: 3be623ab1c8d16d70e9763318aeb9c97878ce063
Review findings:
- The broad matcher-parity items from the prior review are non-blocking follow-ups for this PR.
- Two PR-introduced, original-scope issues remain and are anchored inline: the shared protocol parser breaks the runtime's prior case-insensitive behavior, and the newly lint-visible rejection of non-empty rule lists with no effective allow clause is undocumented.
- No authorization fail-open or sandbox-integrity regression was found; validation remains fail-closed.
Docs: Navigation changes are unnecessary, but the new deny-all rule-list validation needs the focused policy-schema clarification noted inline.
E2E: test:e2e is required because this changes policy enforcement and network-proxy validation, but it is deferred until these review findings are resolved and a new head is available.
Next state: gator:in-review pending an author update.
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
Scope-Focused Re-review
Thanks @gracesmith6504 for identifying the scope expansion. @johntmyers agreed that full MCP matcher tool/params parity, generic query matcher validation, JSON-RPC per-rule checks, and the other pre-existing broad parity gaps belong in follow-up work. I independently re-reviewed the complete current diff against only the nine shared L7 endpoint semantic checks from #1714.
Validation: This PR remains project-valid and the shared-validator approach is sound.
Head SHA: 3be623ab1c8d16d70e9763318aeb9c97878ce063
Review findings:
- Two PR-introduced original-scope compatibility regressions are anchored inline: protocol parsing no longer preserves the runtime's prior case-insensitive behavior, and MCP allow-all with an empty allow object is incorrectly classified as deny-all before normalization.
- The earlier broad matcher-parity findings are non-blocking follow-up work for this PR.
- No authorization fail-open or sandbox-integrity regression was found; default-deny behavior remains intact.
Docs: One focused clarification is suggested inline for the newly shared rules-would-deny-all constraint; no navigation change is needed.
E2E: test:e2e will be required after the blocking review findings are resolved because this changes policy enforcement and network-proxy validation. It is not being applied on this head.
Next state: gator:in-review pending an author update.
| impl L7Protocol { | ||
| /// Parse a protocol string into a known variant. | ||
| pub fn parse(s: &str) -> Option<Self> { | ||
| match s { |
There was a problem hiding this comment.
gator-agent
Warning: This changes L7Protocol::parse from case-insensitive to case-sensitive. Before this refactor, values such as protocol: REST validated and were interpreted as REST at runtime; they now fail as unknown protocols. Please preserve the prior to_ascii_lowercase() behavior and update the mixed-case assertions. A canonical-lowercase requirement would be a separately scoped breaking change.
| } | ||
|
|
||
| // 7. Rules would deny all traffic | ||
| if ep.rules_would_deny_all { |
There was a problem hiding this comment.
gator-agent
Warning: This falsely rejects protocol: mcp with mcp.allow_all_known_mcp_methods: true and rules: [{allow: {}}]. Runtime normalization turns the omitted method into *, so the MCP method profile supplies an effective allow base and the previous runtime validator accepted it. Please guard this check for MCP allow-all or compute effective rules after normalization consistently in both callers, with profile-lint and runtime regression coverage.
| - `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`. |
There was a problem hiding this comment.
gator-agent
Suggestion: The shared check also rejects non-empty lists when every entry lacks an effective protocol-specific allow clause, while this bullet documents only rules: []. Please clarify that rules must contain at least one effective allow clause. This page is the right location; no navigation change is needed.
2 participants
Summary
Extract 9 L7 endpoint semantic checks into a shared
validate_l7_endpoint_semantics()function inopenshell-policy. Both profile lint (openshell-providers) and the runtime validator (openshell-supervisor-network) now call the shared function via a lightweightL7EndpointFieldsfield bag, eliminating duplication and preventing drift. Also consolidates theL7Protocolenum into a single definition inopenshell-policy.Related Issue
Fixes #1714
Changes
crates/openshell-policy/src/l7_validate.rswithL7Protocolenum,L7EndpointFieldsstruct,validate_l7_endpoint_semantics()function, and 15 unit testscrates/openshell-policy/src/lib.rsopenshell-policydependency tocrates/openshell-providers/Cargo.tomlcrates/openshell-providers/src/profiles.rs; add 5 integration testsL7Protocolenum fromcrates/openshell-supervisor-network/src/l7/mod.rs, import and re-export fromopenshell-policy; replace 9 duplicated checks with shared function call; alignrules_would_deny_allcomputationcrates/openshell-server/src/grpc/policy.rsandprovider.rs: addaccess: "full"to endpoints with protocolTesting
mise run pre-commitpassesChecklist