feat(l7): add JSON-RPC and MCP policy enforcement#1865
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-1865.docs.buildwithfern.com/openshell |
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
2 times, most recently
from
62da29d to
8dc2a54
Compare
krishicks
requested review from
a team,
derekwaynecarr and
mrunalp
as code owners
johntmyers
added
the
gator:in-review
PR Review StatusValidation: This maintainer-authored PR is project-valid because it implements the JSON-RPC/MCP method-level policy work discussed in #1793, with documented v1 scope around sandbox-to-server HTTP request inspection. Review findings:
Docs: Fern docs were updated for the new policy schema and sandbox policy behavior. Next state: |
PR Review Follow-UpHead SHA: The required independent reviewer pass confirmed the two blocking findings from the previous gator review:
Additional non-blocking warning from the independent review:
The earlier warnings also still apply: forward-proxy JSON-RPC audit logs are less detailed than CONNECT logs, and Next state: |
|
Label |
|
I would recommend adding an Action to add a test the functionality from and to modelcontextprotocol/conformance and through OpenShell. There is an action, but I don't think it's setup by default in a useful way for testing through OpenShell. There |
Re-check After Maintainer UpdateI re-evaluated latest head Disposition: not resolved yet. There has not been a new commit or author response after the existing gator review feedback, and the maintainer testing recommendation is additional review feedback to address or have explicitly waived by a maintainer before this can advance. Remaining items:
Checks: required branch, Helm, DCO, docs preview, and standard Rust/Python checks are currently passing. Next state: |
Re-check After CI UpdateI re-evaluated latest head Disposition: not resolved. The PR still has unresolved review feedback, and the required Remaining items:
Next state: |
Re-check After CI Log ReviewI re-evaluated latest head Disposition: not resolved. The PR still has unresolved review feedback, and the failed E2E logs now show an actionable policy regression. Remaining items:
Next state: |
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
2 times, most recently
from
e9786e2 to
3516a0b
Compare
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Resolved items:
Remaining items:
Checks: current branch and E2E checks are queued or pending for this head, with Next state: |
BlockedGator is blocked because GitHub reports this PR is not mergeable against Next action: @krishicks, please rebase or merge |
johntmyers
added
gator:blocked
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
3516a0b to
6d61408
Compare
Re-check After Author and Reviewer UpdatesI re-evaluated latest head Disposition: still blocked. GitHub still reports this PR is not mergeable against Remaining items:
Checks: required branch, Helm, DCO, Rust, Python, and license/header checks are passing for this head. Next state: |
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
6d61408 to
9af3648
Compare
Re-check After Author UpdateI re-evaluated latest head Disposition: still blocked. GitHub still reports this PR is not mergeable against Remaining items:
Checks: branch, DCO, Rust, Python, license/header, and Helm checks are currently passing or skipped for this head. Next state: |
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
9af3648 to
7101a37
Compare
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
f7027d7 to
c489300
Compare
krishicks
changed the title
PR Review StatusValidation: This PR remains project-valid because it implements the JSON-RPC/MCP method-level policy work tracked by #1793, with docs and E2E coverage for the current policy schema and conformance path. Review findings:
Docs: Fern docs are present and the generic JSON-RPC allow/deny docs now describe exact method names or Checks: Next state: |
johntmyers
added
gator:watch-pipeline
|
I agree the split between rules and deny_rules is awkward if the allow side already uses an allow wrapper. The product model should probably be one visible rule grammar and one explicit precedence rule, rather than two different containers that users have to mentally merge. The invariant I would preserve is not the current field shape; it is deny precedence plus a typed target for each rule. A migration-safe shape could be:
That gives the docs a simpler story: users write allow and deny entries with the same fields, while the engine records the effect and applies deny-before-allow deterministically. The regression I would add is a dual-shape parity test. Feed the old sibling shape and the new unified shape into the loader, assert the same normalized policy graph, then prove one denied tool in an otherwise allowed MCP batch blocks the whole batch. That catches both migration drift and precedence drift. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, implementation correctness, merge readiness, security review, production readiness, partnership, customer interest, official alignment, NVIDIA usage, OpenShell usage, conformance certification, or Neura usage. |
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
c489300 to
adbfacd
Compare
PR Review StatusValidation: This PR remains project-valid because it implements the JSON-RPC/MCP method-level policy work tracked by #1793, with docs and E2E coverage for the policy schema, JSON-RPC/MCP enforcement, and conformance path. Review findings:
Docs: Fern docs under Checks: Next state: |
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
adbfacd to
c594e2d
Compare
johntmyers
added
gator:in-review
PR Review StatusValidation: This PR remains project-valid because it implements the JSON-RPC/MCP method-level policy work tracked by #1793, with docs and E2E coverage for the policy schema, JSON-RPC/MCP enforcement, and conformance path. Review findings:
Docs: Fern docs under Checks: Next state: |
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
c594e2d to
470f882
Compare
PR Review StatusValidation: This PR remains project-valid because it implements the JSON-RPC/MCP method-level policy work tracked by #1793, with docs and E2E coverage for the policy schema, JSON-RPC/MCP enforcement, and conformance path. Review findings:
Docs: Fern docs under Checks: Next state: |
|
The remaining blocker looks like a useful compile-time version of the same policy-boundary problem: the proto path has to preserve MCP endpoint identity without taking ownership of a borrowed endpoint object. I would make the conversion contract explicit:
That last point matters because the policy loader should not make unsupported configuration disappear before validation. Alias normalization is fine as a compatibility layer, but it should happen after the parser has decided whether the original config shape is valid enough to normalize. Otherwise an invalid stanza can become invisible on one load path while the typed parser rejects it on another. The regression I would add is two-part. First, compile-level coverage or a focused unit test for the borrowed endpoint conversion path so endpoint.mcp cannot be moved out of a shared endpoint again. Second, YAML-versus-proto parity coverage where the same MCP endpoint profile lowers to the same internal policy record, including max body size, tool identity support, and unsupported-field rejection. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, implementation correctness, merge readiness, security review, production readiness, partnership, customer interest, official alignment, NVIDIA usage, OpenShell usage, conformance certification, or Neura usage. |
This feedback was not applied because it generated a linter error: |
Add policy schema, proto, provider profile, OPA, and L7 proxy support for `protocol: json-rpc` and `protocol: mcp`. Generic JSON-RPC endpoints match exact method names only, with `method: "*"` as the all-method sentinel; wildcard/glob methods and params matchers are rejected. Parse JSON-RPC request bodies and batches in the forward proxy, deny response-shaped client frames, limit receive-stream GET allowance to MCP endpoints, and redact params in decision logs. Preserve L7 rule params on the proto load path so MCP `tools/call` tool filters behave like YAML-loaded policies. Add MCP conformance coverage, JSON-RPC L7 e2e coverage, and docs for the new protocols and current matcher limitations. Co-authored-by: ddurst <267424412+ddurst-nvidia@users.noreply.github.com> Signed-off-by: Kris Hicks <khicks@nvidia.com>
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
470f882 to
f5dbcc5
Compare
PR Review StatusValidation: This PR remains project-valid because it implements the JSON-RPC/MCP method-level policy work tracked by #1793, with docs and E2E coverage for the policy schema, JSON-RPC/MCP enforcement, and conformance path. Review findings:
Docs: Fern docs under Checks: Next state: |
|
The useful distinction here is between ownership transfer and representation lowering. If the proto helper wants a by-value MCP options object for lint and ergonomics, the call site still has to make the ownership boundary explicit. I would keep the invariant as:
That keeps the code honest about where ownership changes. Passing by reference hides the move but can fight the local lint rule. Moving directly from endpoint.mcp violates the borrowed endpoint boundary. Cloning the optional proto field at the conversion boundary is the clearest contract: shared endpoint in, owned policy profile out. The regression I would add is targeted and small. Build or unit coverage should exercise a borrowed NetworkEndpoint containing MCP options and prove conversion succeeds without moving out of the borrowed endpoint. A parity test should then assert that the YAML and proto versions of the same MCP endpoint produce the same max body size and tool identity policy. That catches both the compile failure and the subtler policy-widening failure if MCP options disappear during conversion. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, implementation correctness, merge readiness, security review, production readiness, partnership, customer interest, official alignment, NVIDIA usage, OpenShell usage, conformance certification, or Neura usage. |
|
@rpelevin do you have specific interest around this PR? |
|
Gator - tests are passing, are you sure on the compilation claims you're making? |
|
Yes. My interest is specifically the policy and approval boundary around agent network egress, not a claim about using OpenShell or representing a customer. This PR is one of the cleaner places where that boundary becomes concrete: generic JSON-RPC, MCP tool calls, proto and YAML parity, and conformance all meet at the point where an agent request leaves the sandbox. The part I care about is whether the runtime can prove what was allowed before the request crosses that boundary, and whether denial or unsupported cases stay visible instead of being normalized away. The useful product invariant, in my view, is:
That is why I have been focusing on compile and test boundaries rather than feature preference. Ownership bugs, selector ambiguity, and YAML/proto drift are not just implementation details here. They decide whether the eventual authority record can be trusted by another operator, reviewer, or system. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, implementation correctness, merge readiness, security review, production readiness, partnership, customer interest, official alignment, NVIDIA usage, OpenShell usage, conformance certification, or Neura usage. |
johntmyers
added
gator:approval-needed
Re-check After Maintainer UpdateThanks @johntmyers. I re-checked latest head What I checked: the current Disposition: resolved. The prior compile-failure blocker was not valid for this head, so I am retracting it. No blocking gator review findings remain. Review: no blocking findings remain from gator for this SHA. Next state: |
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: gator had moved this PR to I removed the active |
5 participants
Summary
Add first-class L7 policy support for
protocol: json-rpcandprotocol: mcp, so sandbox egress can be constrained at the JSON-RPC method and MCP tool layer rather than just host/path. The L7 forward proxy now parses JSON-RPC request bodies (single and batch), enforces per-call decisions, fails closed on ambiguous or response-shaped frames, and relays MCP SSE responses without prematurely closing streams.Related Issue
Closes #1793
Changes
json-rpcandmcpprotocols (openshell-policy,proto/sandbox.proto,openshell-supervisor-network/opa.rs,data/sandbox-policy.rego, provider profiles).tool/params.namematching to narrowtools/call, enforced identically on the YAML and proto load paths.l7/jsonrpc.rs,l7/http.rs,l7/mod.rs,l7/proxy.rs): parse JSON-RPC bodies and batches with bounded reads (json_rpc_max_body_bytes), fail closed on invalid/ambiguous/response-shaped frames, deny a batch if any element is denied, and redact params in decision logs.l7/relay.rs,l7/rest.rs): relay unframed or sparse MCP SSE responses without prematurely closing streams.McpOptionsproto message (strict_tool_names,allow_all_known_mcp_methods).docs/reference/policy-schema.mdx,docs/sandboxes/policies.mdx,architecture/sandbox.md).e2e/mcp-conformance*), JSON-RPC L7 e2e coverage (e2e/rust/tests/forward_proxy_jsonrpc_l7.rs), plus extensive OPA/policy unittests.
proto_to_opa_data_jsonnow carries the per-ruleparamsmatcher map for both allow and deny rules. Previously it was dropped on the proto load path, silently degrading an MCP tool-scoped allow rule into allow-any-tool in production (the YAML path enforced correctly). Covered by a newfrom_protoregression test.Testing
mise run pre-commitpassesAdditional targeted checks:
cargo test -p openshell-sandbox jsonrpcmise run e2e:rust -- --test forward_proxy_jsonrpc_l7Checklist