fix(policy): prevent implicit authorization inheritance - #2499
Conversation
A network rule authorizes every listed binary to reach every listed endpoint, so unioning an AddRule operation's binaries and endpoints independently grants binary-by-endpoint pairs the operation never declared. Require AddRule to declare the complete product before merging, and reject an operation that would give one host and port two different MCP inspection contracts. The rejection names the binaries the operation still has to declare. Fix proposal coverage on the same surface. An any-binary proposal was vacuously covered by a binary-restricted loaded rule, and a complete product split across several loaded rules was reported as uncovered. Coverage compares merge-widened endpoint fields by containment and exact-matches only the fields the merge never widens, so a policy the gateway just merged always reads back as covered and the sandbox policy.local /wait long-poll cannot spin to its deadline. Signed-off-by: Shiju <shiju@nvidia.com>
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 addresses the concrete fail-open policy-merge defect documented in #2497 and preserves OpenShell's default-deny authorization boundary.
Head SHA: 5e825768d55ea469e5e56d5816a9d6df50cf763c
Review findings:
- One critical and three warning findings are attached inline and require author changes.
- Warning —
crates/openshell-policy/src/merge.rs:648: The documented remediation to put partial authorization in a separate rule cannot work for an overlapping endpoint because the unchanged overlap fallback folds the newrule_nameinto the existing rule, where inheritance validation rejects it. This line is outside the current diff, so it cannot be anchored inline. Preserve the requested key when folding would cause an inheritance conflict, or provide an explicit no-fold path, with an end-to-end regression covering a new binary granted only one endpoint from an existing multi-endpoint rule. - Warning — agent workflow drift: This changes policy-update behavior, but
.agents/skills/openshell-cli/SKILL.mdstill recommends incremental--add-endpointupdates without explaining the whole-product requirement or when full YAML replacement is needed. Review and update it andgenerate-sandbox-policyusing thesync-agent-inframaintenance map.
Docs: The relevant Fern policy page is updated; its folder is already navigation-managed, so docs/index.yml does not need a new entry.
E2E: test:e2e will be required after the review findings are resolved because this changes policy-enforcement behavior.
Next state: gator:in-review
| || !ports_cover(loaded, proposed) | ||
| || !protocols_match(&loaded.protocol, &proposed.protocol) | ||
| || effective_tls(&loaded.tls) != effective_tls(&proposed.tls) | ||
| || effective_enforcement(&loaded.enforcement) |
There was a problem hiding this comment.
gator-agent
Critical: merge_endpoint treats an empty incoming enforcement as “retain the existing value,” but coverage converts it to effective audit and requires equality. Adding a rule to an existing enforce endpoint can therefore succeed while policy.local /wait never observes coverage and times out. Treat omitted retained fields as unspecified during coverage, or normalize/reject them consistently during merge, and add a regression with an enforce endpoint plus an incoming proposal that omits enforcement.
| && rule | ||
| .endpoints | ||
| .iter() | ||
| .any(|endpoint| endpoint_authorization_covers(endpoint, target_endpoint)) |
There was a problem hiding this comment.
gator-agent
Warning: This requires one loaded endpoint to cover every proposed port. A proposed [443, 8443] endpoint can be fully covered by two loaded rules for the same binary, one per port, yet still be reported uncovered despite the documented rule-union semantics. Evaluate atomic host/path/port pairs across the loaded union and add a split-port coverage test.
| )?; | ||
|
|
||
| existing_rule.endpoints = merged_endpoints; | ||
| append_unique_binaries(&mut existing_rule.binaries, &incoming_rule.binaries); |
There was a problem hiding this comment.
gator-agent
Warning: Elsewhere this change defines an empty incoming binary list as any-binary, but appending an empty list leaves an existing restricted scope unchanged. An any-binary AddRule folded into a restricted rule therefore succeeds without applying its requested scope, and coverage can never become true. Either promote the merged scope to any-binary after validating all endpoints or retain the proposal as a separate rule; add a regression test.
| && !incoming_rule | ||
| .endpoints | ||
| .iter() | ||
| .any(|declared| endpoint_authorization_covers(declared, endpoint)) |
There was a problem hiding this comment.
gator-agent
Warning: A full declaration may be split across incoming endpoints. If the existing endpoint covers [443, 8443] and the new binary declares separate [443] and [8443] endpoints, no single declaration covers the merged endpoint, so this rejects a complete Cartesian declaration. Aggregate incoming authorization by atomic endpoint/port pairs before validating, and test this shape.
johntmyers
added
the
gator:in-review
|
This guards the This is admin-only, not agent-reachable — |
Coverage treated an unset proposal value for a field the merge retains as a request for the default, so a proposal that merged cleanly into an endpoint carrying enforcement, protocol, or tls read back as uncovered and left the policy.local /wait long poll spinning. Unset now means unspecified. Ports are a set on the wire but each port is an independent authorization. Coverage and the inheritance check both resolve one binary, host, path, and port at a time, so ports spread across loaded rules resolve and a complete declaration split across incoming endpoints is accepted. An incoming empty binary list means any binary. It now has to declare every merged endpoint like a new concrete path does, and once declared the promotion is applied instead of appending an empty list and leaving the restricted scope in place. The endpoint-overlap fallback folded a new rule name into an existing rule where inheritance validation then rejected it, leaving no way to grant a binary part of a rule. Folding now keeps the requested rule name when it would widen, and reports that it did. An MCP contract conflict still propagates because one host and port carry a single inspection contract. AddAllowRules and AddDenyRules select an endpoint by host and port alone and now reject a target that resolves more than once, including two paths on one rule. RemoveBinary rejects an any-binary rule rather than reporting a success that leaves the binary authorized. Signed-off-by: Shiju <shiju@nvidia.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR remains project-valid because it fixes the fail-open policy-merge defect in #2497.
Head SHA: 9c80c5b81a485fd98b8904843b219b26bc3910b8
Review findings:
- Two critical authorization-boundary findings and one warning are attached inline and require author changes.
- The six findings from the prior-head gator review are resolved for their reported scenarios; the new findings cover distinct multi-port, cross-endpoint MCP-contract, and any-binary cases.
Docs: The Fern policy page and routed agent skills are updated; no navigation change is needed.
E2E: test:e2e remains required for this policy-enforcement change, but it will be applied only after blocking review findings are resolved.
Next state: gator:in-review
| .any(|existing| authorization_unit_unchanged(existing, endpoint, *port)) | ||
| }) | ||
| .collect(); | ||
| if !changed_ports.is_empty() && !undeclared_binaries.is_empty() { |
There was a problem hiding this comment.
gator-agent
Critical (CWE-863): changed_ports is rejected only when an existing binary is missing from the request. With an existing [443, 8443] endpoint carrying GET, an AddRule that lists every existing binary but declares only port 443 with POST merges POST into the shared endpoint and authorizes it on undeclared port 8443. Require each changed port to be covered by the incoming declarations independently of binary-scope coverage, or split the endpoint before applying port-local changes. Add a regression covering L7 rules and other widened fields on multi-port endpoints.
| .unwrap_or(0); | ||
|
|
||
| let promotes_l4_to_mcp = promotes_l4_endpoint_to_mcp(existing, incoming); | ||
| ensure_mcp_contract_compatible(existing, incoming, operation_index, &host, port)?; |
There was a problem hiding this comment.
gator-agent
Critical (CWE-693): MCP compatibility is checked only after path-aware endpoint folding. A second MCP endpoint on the same host/port with another path is appended without validation, and provider-composed rules are excluded from overlap folding. Runtime selects one matching extended configuration by host/port, so conflicting strict-tool-name, method-profile, or body-limit contracts can be selected unpredictably. Enforce one effective MCP contract per host/port across the final effective policy, including provider-composed rules, and add same-rule, cross-rule, and provider-composition regressions.
| // declare every merged endpoint before the widening is allowed. | ||
| existing_rule.binaries.clear(); | ||
| } else { | ||
| append_unique_binaries(&mut existing_rule.binaries, &incoming_rule.binaries); |
There was a problem hiding this comment.
gator-agent
Warning: An empty existing binary list means any binary, but appending an incoming named list makes the list non-empty and silently narrows the rule. An additive AddRule can therefore revoke access for every other process, including through overlap fallback under a new rule name. Preserve an already-empty scope when incoming binaries are named; only clear a restricted scope when the incoming scope is empty. Add same-key and overlap-fallback regressions.
…ary scope The changed-port check only rejected when the operation also left an existing binary undeclared. An operation listing every existing binary could therefore declare one port of a multi-port endpoint and have its widened fields land on the endpoint the merge shares across all of them, authorizing L7 rules on a port it never named. Each changed port must now be named by the operation on its own, independently of binary-scope coverage. MCP contract compatibility was checked inside the endpoint fold, which only compares endpoints agreeing on host, path, and a shared port. The sandbox resolves one extended configuration per host and port and never consults the path, so a second MCP endpoint under another path, or in another rule, left the effective strict-tool-name, method-profile, and body-limit contract decided by match order. Contract agreement is now enforced across the whole merged policy, including provider-composed rules. A conflict already present in the baseline is left alone so unrelated updates still apply. An empty binary list authorizes any binary. Appending an incoming named list made it non-empty and revoked every process the operation did not name, turning an additive update into a silent mass revocation. An already-empty scope is now kept and reported; only a restricted scope is replaced by an incoming any-binary scope. Warnings raised during a fold now name the rule that was actually modified rather than the rule name the operation requested, which differ when the endpoint-overlap fallback redirects the operation. Signed-off-by: Shiju <shiju@nvidia.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @shiju-nv. I checked the latest author delta against the prior multi-port, MCP-contract, and any-binary obligations. The any-binary path is resolved for the reported scenario, and the two carried Critical proposals were downgraded by evidence normalization and are not being republished as blockers. One new, machine-validated Warning remains: the new undeclared-port error bypasses the separate-rule fallback and reintroduces the previously resolved narrow-update failure for multi-port endpoints.
Validation: This PR remains project-valid because it addresses the fail-open policy-merge defect in #2497.
Head SHA: b23bb432d8673c9b6f3ba22688eef92c8bee7f06
Base SHA: 39bf94e528a58da6e9112b5b2278c2a1957b29e3
Merge base SHA: 39bf94e528a58da6e9112b5b2278c2a1957b29e3
Patch ID: 53c34d612935f3d776bad27e35d15fff974787e9
Gator payload: 2
Review mode: follow_up
Previous reviewed SHA: 9c80c5b81a485fd98b8904843b219b26bc3910b8
Blocking findings:
GATOR-b23bb432-01: The newUndeclaredPortWouldChangeresult is not recognized as a fold-only authorization conflict, so a safe differently named narrow rule is rejected instead of being retained separately. The inline comment contains the complete reproducer and requested fix.
Carried findings:
- None republished. The two carried Critical proposals normalized as non-blocking hypotheses; the existing threads remain the durable history.
Non-blocking suggestions: omitted on follow-up.
Docs: The Fern policy page and routed agent skills remain updated; no navigation change is needed.
E2E: test:e2e remains required for this policy-enforcement change after blocking review feedback is resolved.
Next state: gator:in-review
| .filter(|port| !operation_names_port(incoming_rule, endpoint, *port)) | ||
| .collect(); | ||
| if !undeclared_changed_ports.is_empty() { | ||
| return Err(PolicyMergeError::UndeclaredPortWouldChange { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-b23bb432-01
Invariant: The endpoint-overlap fallback must retain the requested rule name whenever folding alone creates a scope conflict that disappears when the incoming authorization remains separate.
Prerequisite: An operator uses a distinct rule name to add a capability for only a subset of binaries on one port of an existing multi-port endpoint.
Entry point → sink: UpdateConfig.merge_operations / openshell policy update --rule-name → add_rule attempts convenience folding and returns UndeclaredPortWouldChange instead of inserting the requested separate rule.
Base → head: At the previous reviewed head, the attempted fold reached ExistingBinariesWouldInheritAuthorization; the fallback recognized that conflict and inserted the requested narrow rule. This delta introduces UndeclaredPortWouldChange first, but is_authorization_inheritance_conflict does not include the new variant, so the safe fallback is bypassed. This materially invalidates the prior resolution.
Impact: A supported, safe incremental update fails with FailedPrecondition even though retaining the requested rule would express exactly the requested binary, endpoint, port, and capability scope. The documented narrow-update remediation is therefore false for multi-port endpoints.
Reproducer: Start with rule broad for binaries [/usr/bin/a, /usr/bin/b], with one REST endpoint api.example.com on ports [443, 8443] allowing GET. Submit differently named rule narrow for /usr/bin/a on api.example.com:443 adding POST. The candidate fold changes undeclared port 8443 and returns UndeclaredPortWouldChange; because that variant is not classified as fold-only, narrow is never inserted separately.
PR ownership: This latest delta introduces the new error and places it before the prior existing-binary conflict without routing it through the fallback added by this PR.
Requested change: Include UndeclaredPortWouldChange in the fold-only authorization-conflict classification so a differently named rule remains separate; same-key updates should still return the error. Add a regression for a subset of existing binaries on one port of an existing multi-port rule.
3 participants
Summary
Make policy merging fail closed when an
AddRuleoperation would implicitly widen authorization. One network rule authorizes the Cartesian product of its binaries and endpoints, so every resulting pair must be declared before the merge, and proposal coverage must confirm that the loaded policy contains the whole proposed product.Related Issue
#2497
Changes
AddRuleendpoint scopes in request order while preserving atomic failure through the private merge copy.Testing
mise run pre-commitpassesChecklist
docs/sandboxes/policies.mdxdocuments the declare-the-whole-scope requirement and the new rejection reasons foropenshell policy update