feat(l7): add JSON-RPC 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 |
62da29d to
8dc2a54
Compare
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: |
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 |
3516a0b to
6d61408
Compare
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 | ||
| with: | ||
| repository: modelcontextprotocol/conformance | ||
| ref: v0.1.11 |
There was a problem hiding this comment.
My last note on this got disappeared from history.
v0.1.11is an older version of the tests: https://github.com/modelcontextprotocol/conformance/releases/tag/v0.1.16
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: |
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: |
9af3648 to
7101a37
Compare
Signed-off-by: Kris Hicks <khicks@nvidia.com>
Signed-off-by: Kris Hicks <khicks@nvidia.com>
Signed-off-by: Kris Hicks <khicks@nvidia.com>
Signed-off-by: Kris Hicks <khicks@nvidia.com>
Signed-off-by: Kris Hicks <khicks@nvidia.com>
Signed-off-by: Kris Hicks <khicks@nvidia.com>
0f54c83 to
e3a525e
Compare
Signed-off-by: Kris Hicks <khicks@nvidia.com>
e3a525e to
91dac00
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 current policy schema, bridge hardening, and conformance path. Review findings:
Docs: Fern docs and MCP conformance README updates are present for the current JSON-RPC policy schema, directional behavior, SSE behavior, and NAT-tolerant bridge model. Checks: Next state: |
|
I would treat this head as a pipeline-verification moment for three invariants, not as more schema debate. The important thing to preserve is the boundary shape:
The regression set I would want before merge:
If those pass on the new head, the remaining review question becomes operational confidence in the E2E gate rather than another authorization-boundary issue. Boundary: architecture and test feedback only; no claim about using this project or running its code. |
| WebSocket upgrade and text-message rules, GraphQL operation rules, and | ||
| JSON-RPC method and params rules on sandbox-to-server request bodies. JSON-RPC | ||
| request inspection buffers up to the endpoint `json_rpc.max_body_bytes` limit. | ||
| Literal dotted keys in JSON-RPC params are rejected before policy evaluation so |
There was a problem hiding this comment.
As defined by JSON-RPC 2.0, params are allowed to have .'s in them. This also pushes up to the MCP layer.
We shouldn't sacrifice compatibility, for easier enforcement/policy writing.
We could "escape.dots" or we could params.name[]?
|
I agree with that compatibility boundary. I would not reject the request just because a JSON-RPC params object contains a literal dotted key. The unsafe case is not the dot itself. The unsafe case is letting the policy selector language confuse a literal property name with a nested object path. The cleaner split is:
If the policy language keeps dot-path selectors, I would add an explicit literal-key escape or bracket form for params keys that contain dots. Then the evaluator can distinguish:
The tests I would want are:
So I would move the compatibility restriction from the JSON-RPC request into the policy selector grammar. Runtime compatibility stays broad; enforcement stays precise. Boundary: architecture and test feedback only; no claim about using this project or running its code. |
Signed-off-by: Kris Hicks <khicks@nvidia.com>
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: present, but need the selector-semantics correction above. Checks: Next state: |
|
I agree with the blocking direction here: literal dotted keys and nested path selectors should not collapse into the same authorization key. The safe contract I would test is:
The important invariant is that payload compatibility cannot become selector aliasing. If "arguments.scope" and arguments.scope can both satisfy the same selector, then a deny rule can be evaluated against one value while execution receives another. That makes the policy result non-replayable and non-verifiable. I would split the fixture set into four cases:
The docs should also avoid saying literal key takes precedence at the authorization boundary. Precedence is fine for generic JSON convenience, but not for a policy selector. For policy, the choices should be explicit literal selector, explicit nested selector, or collision denial. Boundary: architecture and test feedback only; no claim about using this project or running its code. |
| let yaml = r" | ||
| version: 1 | ||
| network_policies: | ||
| mcp: |
There was a problem hiding this comment.
For here and other examples, I'd recommend not calling these examples mcp, because I'm seeing it lead to confusion.
It's a valid name, but because it's appears as an object level yaml name, and is immediately afterwards name: mcp, it can lead to some folks considering this to be actual MCP enforcement.
Overall, that feels like a larger problem with the YAML syntax, but for this place in particular, it's causing more confusion than it should.
There was a problem hiding this comment.
That's fair. I'll update it so the only mentions of mcp are in the mcp conformance test.
|
I agree with the naming concern. The issue is not that mcp is an invalid identifier; it is that the example makes four different concepts look like the same authority boundary:
For generic JSON-RPC policy examples, I would use neutral names like jsonrpc_demo, tool_server_api, or local_jsonrpc_server, and reserve mcp for examples that intentionally exercise MCP-specific semantics or MCP conformance behavior. The invariant I would document is:
That keeps the examples compatible while making the authority boundary less ambiguous. A reader should be able to tell whether the policy is proving JSON-RPC parsing, MCP tool semantics, or the host bridge conformance path without inferring it from a repeated example name. The cleanup I would pin is small: rename the generic examples, add one sentence in the docs that policy keys and display names do not imply MCP enforcement, and keep the MCP conformance examples separately named so they are obviously tied to the conformance runner path. Boundary: architecture and test-shape feedback only; no claim about using this project or running its code. |
|
/ok to test 0c1e237 |
Summary
Adds JSON-RPC L7 policy enforcement for sandbox proxy traffic. The implementation supports JSON-RPC endpoint configuration,
rpc_methodmatching, scalar objectparamsmatching, forward-proxy inspection, CONNECT tunnel inspection, and deny-if-any-denied batch handling.JSON-RPC enforcement applies to sandbox-to-server HTTP request bodies sent to the configured endpoint. It does not yet enforce policy on server-to-client JSON-RPC messages carried on MCP SSE streams or response bodies. Tool results continue to pass because responses are relayed, not matched against
rpc_method.Related Issue
Closes #1793
Changes
rpc_methodand flattened scalar objectparamsmatchers for allow and deny rules.Testing
mise run pre-commitpassesAdditional targeted checks:
cargo test -p openshell-sandbox jsonrpcmise run e2e:rust -- --test forward_proxy_jsonrpc_l7Checklist