Skip to content

fix(policy): prevent implicit authorization inheritance - #2499

Open
shiju-nv wants to merge 1 commit into
NVIDIA:mainfrom
shiju-nv:fix/policy-merge-authorization-inheritance
Open

fix(policy): prevent implicit authorization inheritance#2499
shiju-nv wants to merge 1 commit into
NVIDIA:mainfrom
shiju-nv:fix/policy-merge-authorization-inheritance

Conversation

@shiju-nv

Copy link
Copy Markdown
Contributor

Summary

Make policy merging fail closed when an AddRule operation 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

  • Reject empty AddRule endpoint scopes in request order while preserving atomic failure through the private merge copy.
  • Reject new binaries that would inherit undeclared existing endpoints.
  • Reject new or changed endpoints that would be inherited by undeclared existing binaries, and name the binaries the operation still has to declare so the proposer can act on the rejection.
  • Reject an operation that would give one host and port two different effective MCP inspection contracts, reporting the compared contract values rather than raw protobuf options.
  • Make proposal coverage evaluate every binary-to-endpoint pair across loaded rules.
  • Require loaded any-binary scope before reporting an any-binary proposal as covered.
  • Compare effective endpoint authorization, including protocol behavior, ports, L7 restrictions, TLS behavior, allowed IPs, and provenance-sensitive fields.
  • Compare endpoint fields the merge widens by containment and exact-match only the fields the merge never widens, so any policy the gateway accepts reads back as covered.
  • Preserve atomic failure and map malformed operations separately from authorization-state conflicts.
  • Document the declare-the-whole-scope requirement and its rejection reasons in the policy update guide.
  • Add regression tests for REST, MCP, L4 promotion, any-binary, multi-endpoint, multi-port, effective-default, deny, provenance, Cartesian-coverage, and merge-then-cover round-trip cases.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable) — docs/sandboxes/policies.mdx documents the declare-the-whole-scope requirement and the new rejection reasons for openshell policy update

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 johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 new rule_name into 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.md still recommends incremental --add-endpoint updates without explaining the whole-product requirement or when full YAML replacement is needed. Review and update it and generate-sandbox-policy using the sync-agent-infra maintenance 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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:in-review Gator is reviewing or awaiting PR review feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants