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 |
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
| 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: |
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
|
|
||
| ##### JSON-RPC Deny Rule (`protocol: json-rpc`) | ||
|
|
||
| JSON-RPC deny rules use the same field names as JSON-RPC allow rules, but they appear directly under each `deny_rules` entry instead of under an `allow` wrapper. Deny rules take precedence over allow rules. In a batch request, one denied call denies the full batch. |
There was a problem hiding this comment.
The sibling rules and deny_rules feels like an awkward pairing, especially since each rules is scoped by an allow. I'm guessing that the intent is to not complicate the deny > allow precedence as seen in a list, but I feel like that may be better managed as:
rules:
deny:
- rpc.method: tools/call
params:
name: delete_resource
- rpc.method: something/else
allow:
- rpc.method: tools/call
params:
name: init
- ...
Stating that "deny rules are applied first," is probably less complicated, than splitting at rules & deny_rules.
Re-check After Reviewer UpdateI re-evaluated latest head Disposition: not resolved. There has not been a new commit after the latest gator review pass, and the new schema feedback needs an author response or a maintainer decision before this can advance. Remaining items:
Next state: |
| // GraphQL root field globs. Deny rules match when any selected root field | ||
| // matches any configured glob. | ||
| repeated string fields = 7; | ||
| // JSON-RPC method name (JSON-RPC): exact name or glob, e.g. "tools/call". |
There was a problem hiding this comment.
nit:
From the perspective of JSON-RPC 2.0, * is a valid method name, any string, not starting with rpc. is available for use.
From the perspective of MCP, there is (currently) an enumerated list of methods per version of the spec.
Re-check After Reviewer UpdateI re-evaluated latest head Disposition: not resolved. There has not been a new commit after the latest gator review pass, and existing review feedback plus failing E2E still block advancement. Remaining items:
Next state: |
PR Review Follow-UpHead SHA: The required independent reviewer pass re-checked the current head. This PR remains blocked in Blocking items:
Non-blocking warnings:
Next state: |
PR Review Follow-UpHead SHA: The required independent reviewer pass re-checked the current head. This PR remains in Blocking items:
Non-blocking warning:
Next state: |
PR Review Follow-UpHead SHA: The required independent reviewer pass re-checked the current head. This PR remains in Blocking items:
Non-blocking warnings:
Next state: |
PR Review Follow-UpHead SHA: The required independent reviewer pass re-checked the current head. Disposition: partially resolved. One previously listed blocker is no longer blocking on code read: the current route-selected JSON-RPC path does include request body inspection, parse-error fail-closed handling, batch evaluation semantics, and denied batch logging. Blocking items:
Docs: present for the JSON-RPC policy schema and sandbox policy behavior. Next state: |
L7 HTTP relays previously treated unframed HTTP/1.1 responses without `Connection: close` as empty bodies. That is correct for ordinary keep-alive responses, but it drops Server-Sent Events streams because SSE responses often use `Content-Type: text/event-stream` without `Content-Length` or `Transfer-Encoding`. Detect `text/event-stream` responses in the no-framing branch and relay them until EOF or idle timeout, flushing each chunk as it arrives. This preserves streaming behavior while keeping the existing empty-body handling for other unframed keep-alive responses. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Add a Rust e2e test that drives MCP-style JSON-RPC requests through both the forward proxy and CONNECT tunnel paths. Cover method rules, params rules, batch handling, and invalid JSON denial expectations so the JSON-RPC implementation can be built against one failing scenario. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Add json-rpc as a policy protocol and carry JSON-RPC rule fields through policy parsing and validation. Wire the protocol into the L7 dispatcher with a passthrough placeholder so later commits can add enforcement without changing endpoint recognition. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Move HTTP request body buffering and chunked-body normalization out of the GraphQL module so other HTTP-carried L7 protocols can inspect request bodies without depending on GraphQL internals. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Add the JSON-RPC HTTP parser and relay path, extract request methods, and pass JSON-RPC metadata into L7 policy evaluation. Wire rpc_method through proto and policy conversion, add Rego matching for JSON-RPC methods, and inspect forward-proxy JSON-RPC bodies before relaying upstream. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Carry JSON-RPC max body bytes from policy into runtime endpoint config and use it on both CONNECT and forward JSON-RPC inspection paths instead of hardcoding 64 KiB. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Add JSON-RPC params matcher maps to proto and YAML policy conversion, including shared matcher conversion helpers. Flatten object params into dot-separated keys for policy input and extend Rego allow and deny matching to filter JSON-RPC calls by params. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Parse JSON-RPC batch arrays into per-call metadata and evaluate each batch item with the existing method and params policy rules. Deny the whole batch when any call is denied. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Log JSON-RPC endpoint, RPC methods, params SHA-256 digest, and policy version without recording raw params. Use <empty> when no params are present. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Document JSON-RPC endpoint configuration, rpc_method and params matchers, batch denial behavior, current directionality limits, matcher scope, and the current policy update CLI limitation. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Reject ambiguous JSON-RPC params before policy evaluation by refusing literal dotted object keys and flattened selector collisions. Force-deny JSON-RPC parse errors in the forward-proxy path so broad REST-style access presets cannot bypass malformed JSON-RPC bodies. Require JSON-RPC 2.0 request objects, use JSON-RPC-specific forward audit logs with RPC methods, params digest, and policy generation, and reject unsupported json_rpc YAML knobs instead of accepting unused fields. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Add a reusable MCP conformance workflow that runs upstream client scenarios through an OpenShell sandbox. Add a client image, wrapper, policy template, and expected-failures baseline for expanding MCP conformance coverage. Remove stale JSON-RPC e2e policy fields that are no longer accepted. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Signed-off-by: Kris Hicks <khicks@nvidia.com>
PR Review Follow-UpHead SHA: The required independent reviewer pass re-checked the current head. This PR remains in Blocking items:
Non-blocking warning:
Next state: |
MCP elicitation sends JSON-RPC result messages from the client back to the server without a method field. Treat those as valid response-only POSTs once the endpoint has already matched so method allowlists keep applying to request calls. Also wire the MCP conformance runner into the e2e task and keep known upstream client gaps in the expected-failure list. Signed-off-by: Kris Hicks <khicks@nvidia.com>
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
7101a37 to
3730691
Compare
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. The prior Remaining blocking items:
Resolved or non-blocking items:
Next state: |
PR Review Follow-UpHead SHA: The required independent reviewer pass re-checked the current head after the latest JSON-RPC fixes and MCP conformance updates. Disposition: partially resolved. The earlier route-selected JSON-RPC parsing concern is resolved on code read: path-selected JSON-RPC endpoints now populate JSON-RPC request info before policy evaluation, and the added E2E covers forward proxy plus CONNECT allowed/denied params cases. The PR still remains in Blocking items:
Docs: present for the JSON-RPC policy schema and sandbox policy behavior on existing Fern pages; no navigation update appears necessary. Next state: |
|
I think the remaining JSON-RPC boundary is response-shaped traffic from the client side. If the sandbox is enforcing client-to-server JSON-RPC request authority, a client-to-server POST that contains only a JSON-RPC result or error should not bypass rpc_method/params matching. A response frame is not a method call, but when it is emitted by sandboxed code toward the server, it is still outbound data. I would either default-deny response-only frames in that direction, or require an explicit correlation record proving that the server first issued an allowed request with the same id. The regression shape I would pin:
Separately, I would keep the conformance job credentials scoped to the steps that actually need registry or GitHub auth. The third-party checkout, install, and build path should run with persisted credentials disabled and no broad job-level secrets. That keeps the parser useful without creating a blind spot where arbitrary outbound payloads can be wrapped as JSON-RPC responses and pass the policy path. |
Re-check After Reviewer UpdateI re-evaluated latest head Disposition: not resolved. The concern is confirmed by the current code and by the required independent reviewer pass. This PR remains in Remaining blocking items:
Resolved or non-blocking items:
Next state: |
4 participants
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