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
Re-check After CI Log ReviewI re-evaluated latest head Disposition: not resolved. The PR still has unresolved review feedback, and the required E2E gate is failing with an actionable L7 regression. Remaining blocking items:
Additional follow-up:
Next state: |
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Resolved items:
Remaining items:
Checks: current required gates are pending because the copy-pr mirror is waiting for validation at this head. I am posting the bare Next state: |
|
/ok to test 0e4d7af |
|
One boundary I would keep explicit after the latest fixes: a JSON-RPC object that contains method plus result or error should be classified before method allow-list evaluation. If the parser checks method first, a mixed client-to-server frame can look like an allowed request even though it also carries response semantics. I would make this a hard invalid-envelope case and add negative tests for method-plus-result and method-plus-error. For the MCP conformance job, I would treat the third-party runner as untrusted code until proven otherwise: no generic token env on the MCP matrix step, checkout with persisted credentials disabled, and clear any Docker or registry auth before invoking the external conformance build/run. The useful invariant is that conformance may observe behavior through OpenShell, but it should not inherit repository credentials or ambient container-registry auth. One smaller Rego guard would make the policy side easier to reason about: JSON-RPC rpc_method matching should only run when the selected endpoint is JSON-RPC and the request method is POST. Everything else should stay in the non-JSON-RPC path or fail closed before those clauses can authorize it. |
Re-check After Reviewer UpdateI re-evaluated latest head Disposition: not resolved. The comment confirms the remaining review concerns from the latest gator pass, and there has not been a newer author commit or maintainer waiver after that feedback. Remaining items:
Checks: Next state: |
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>
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>
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>
The upstream MCP conformance runner is untrusted node, so run it in a plain Docker container on the e2e network instead of on the host. Unlike an OpenShell sandbox (egress-only, so an inbound test server is unreachable), the container has a routable address the client sandbox can reach, fixing the scenario connection timeout. The runner holds no gateway credentials: its client command posts the test server URL back to a host-side bridge that runs the real MCP client in an OpenShell sandbox and returns the result. That callback is the trust boundary that keeps gateway access off the untrusted runner. - host-bridge.py: host HTTP server that launches the client in a sandbox - runner-shim.mjs: the runner's client command; calls back to the host bridge - render-policy.py: policy renderer shared with client-through-openshell.sh - build dist in the client image and run `node dist/index.js` so scenarios do not pay the tsx transpile cost on every run - drop the host Node.js/npm build (and the CI Node.js setup step) and the DROP_DOCKER_AUTH knob, both unnecessary now that node runs in a container Signed-off-by: Kris Hicks <khicks@nvidia.com>
krishicks
force-pushed
the
hicks/push-nvuozlywzuwu
branch
from
0e4d7af to
fb9c396
Compare
| max_body_bytes: 131072 | ||
| rules: | ||
| - allow: | ||
| rpc_method: initialize |
There was a problem hiding this comment.
similar to the mcp review, could this just be method?
There was a problem hiding this comment.
🤔 I think so. That would mean it could be any method as applied to whatever the protocol is, which seems fair to me, particularly if we have a json-rpc protocol now and a more strict mcp protocol later.
There was a problem hiding this comment.
Ah, L7AllowDef already has method, I see. That simplifies things nicely.
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. The core JSON-RPC enforcement issues from the previous head appear addressed, but one blocking MCP conformance bridge trust-boundary issue remains. Resolved items:
Remaining items:
Additional follow-up:
Checks: Next state: |
Signed-off-by: Kris Hicks <khicks@nvidia.com>
|
The remaining host-bridge issue looks like a confused-deputy boundary, not just a test-runner hardening detail. I would make the bridge derive the allowed target from host-owned state, not from runner-supplied request fields. A safe shape is: mint a per-run bridge capability on the host, bind it to the runner container identity and expected upstream endpoint, require each bridge request to present that capability, and reject any target host that does not match the container identity and host-issued endpoint binding. For environment forwarding, I would invert the default: the bridge starts from an empty env, copies only named MCP conformance variables, and fails closed if a token-shaped or registry credential variable is present outside the allowlist. That keeps the conformance runner able to observe OpenShell behavior without giving it a host-side network reconfiguration primitive or an ambient credential channel. Two regression tests would make the boundary durable: one where the runner asks the bridge to render a policy for a different private host, and one where the runner supplies a token-bearing env key. Both should be denied before policy rendering or subprocess launch. |
Signed-off-by: Kris Hicks <khicks@nvidia.com>
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 updates for the current schema. Review findings:
Additional follow-up:
Docs: Fern docs are present for the JSON-RPC policy schema and sandbox policy behavior. Checks: branch checks, Helm, DCO, docs preview, Rust, Python, build, and image jobs are passing for this head. Next state: |
Signed-off-by: Kris Hicks <khicks@nvidia.com>
Signed-off-by: Kris Hicks <khicks@nvidia.com>
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 updates for the current schema and conformance path. Review findings:
Docs: Fern docs are present for JSON-RPC policy schema, directional enforcement, params matching, and current response/SSE limitations. Checks: Next state: |
johntmyers
added
gator:watch-pipeline
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