fix(network): fail closed when credential placeholders cannot be rewritten#2162
fix(network): fail closed when credential placeholders cannot be rewritten#2162TonyLuo-NV wants to merge 6 commits into
Conversation
…itten When the credential rewriter degrades internally, the proxy forwarded the literal `openshell:resolve:env:<NAME>` placeholder (or its provider alias marker) to the upstream instead of the resolved secret, leaking the reserved token on the wire and causing upstream auth failures (NVIDIA#2161). Two fail-open paths are closed: - secrets: `rewrite_http_header_block` returned the header block verbatim when no `SecretResolver` was available, so the fail-closed marker scan (which ran only on the resolved path) never saw the placeholder. It now scans the header region for reserved markers even with no resolver and returns `UnresolvedPlaceholderError` when one is present. Marker-free traffic still passes through unchanged. - proxy: when TLS was detected on a CONNECT but `tls_state` was `None` (ephemeral CA generation or CA file write failed at startup), the handler fell back to a raw `copy_bidirectional` tunnel, bypassing credential rewrite. Inside the proxy handler `tls_state` is `None` only on CA-init failure (`mode != Proxy` never starts the handler, and `tls: skip` is handled earlier), so it now refuses the connection with a 503 and a High-severity denial event instead of tunneling. The two startup CA-failure logs are raised from Medium to High. Tests: resolver=None with a placeholder in the request line, a header value, and the provider-alias form now fail closed; marker-free passthrough is unchanged; the relay integration test asserts the request is rejected before any byte reaches upstream; and the 503 fail-closed response contract is locked. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
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. |
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
#6379) (#6382) <!-- markdownlint-disable MD041 --> ## Summary `mcp status` could report all-green provider metadata while OpenShell never rewrote the `openshell:resolve:env:` credential placeholder on egress, so every agent request failed with the literal placeholder as the bearer token and the managed MCP server was silently skipped. This PR adds a wire-level credential-resolution probe to `mcp status` and `mcp add` so that failure mode is loud and attributable. ## Related Issue Fixes the NemoClaw-side diagnostic gap of #6379 (keep that issue open until the upstream fix lands). Full chain for reviewers: - **Bug report (NemoClaw)**: #6379 — managed MCP unusable at runtime, `mcp status` stays all-green - **This PR (NemoClaw, diagnostic surface)**: wire-level differential credential-resolution probe in `mcp status` / `mcp add` - **Root-cause issue (upstream)**: NVIDIA/OpenShell#2161 — gateway never rewrites the `openshell:resolve:env:` placeholder on egress - **Root-cause fix (upstream)**: NVIDIA/OpenShell#2162 — fail closed when credential placeholders cannot be rewritten ## Changes - New `src/lib/actions/sandbox/mcp-bridge-resolution-probe.ts`: builds an in-sandbox MCP `initialize` probe (curl wrapped in the adapter runtime so the generated `protocol: mcp` policy attributes it to the adapter binary ancestry, same construction as the live E2E DNS-rebinding probe), classifies the outcome, and renders the operator warning. All output is redacted via `redactBridgeSecretsForDisplay`. - `statusMcpBridge` gains a `probeCredentialResolution` option; the verdict is surfaced as `provider.credentialResolution` (`ok: true | false | null`, `httpStatus`, `detail`) plus a warning naming the OpenShell host defect on failure. The probe shares the existing legacy-credential skip gate. - `mcp status <server>` probes by default; bare `mcp status` and `mcp list` never probe; `--probe` / `--no-probe` override both directions and combining them is rejected. - `mcp add` runs the probe once after the durable add commits and prints a loud warning on failure without failing the add (exit code stays 0; `--no-probe` skips). - Classification never blames the credential rewrite for endpoint problems: HTTP 2xx = resolved, 400/401/403 = resolution failure, 404/5xx/CONNECT-403/timeout/unreachable = indeterminate with detail. - Human-readable status renders a `credential resolution: verified / FAILED / unknown` line; help text documents the new flags. - Docs: `docs/deployment/set-up-mcp-bridge.mdx` (operate + troubleshooting) and `docs/reference/commands.mdx` (+ synced `commands-nemohermes.mdx`). ## Type of Change - [x] Code change with doc updates ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Docs updated for user-facing behavior changes - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: requesting maintainer review on this PR ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: `npx vitest run --project cli src/lib/actions/sandbox/mcp-bridge-resolution-probe.test.ts src/lib/actions/sandbox/mcp-bridge-status-resolution.test.ts` → 22/22 passed; full `src/lib/actions/sandbox/` CLI lane → 1291 passed (3 unrelated pre-existing local-env failures: two 5s-timeout flakes that pass standalone, one host-python missing `yaml`); `npx vitest run --project integration test/mcp-add-crash-consistency.test.ts test/mcp-bridge-servers.test.ts test/mcp-openshell-workflow.test.ts test/mcp-provider-ownership.test.ts test/mcp-artifact-secret-scan.test.ts` → 41/41 passed; `npm run typecheck:cli` clean - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without warnings (doc changes only) Signed-off-by: Tony Luo <xialuo@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional wire-level credential-resolution probing for MCP `status` (runs by default when exactly one server is named). * Added `--probe` / `--no-probe` support for `add` and `status`, including a post-`add` verification that warns on failure without failing the command. * `status` now can display a “credential resolution” line with HTTP indicators. * **Bug Fixes** * Improved verdict semantics and safer probing/skipping rules (including “unknown” vs “FAILED” outcomes). * **Tests** * Added coverage for probe generation, classification, and CLI flag behavior. * **Documentation** * Updated `mcp`, Hermes/NemoClaw, and troubleshooting docs to reflect the new probe reporting and guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tony Luo <xialuo@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
PR Review StatusValidation: this PR is project-valid because it is a concentrated security hardening fix for linked issue #2161, which documents a reproducible sandbox egress credential-placeholder leak path. Review findings:
Docs: no Fern docs update is required for this code-only degraded-state hardening. Next state: |
…ilable The fail-closed refusal for a terminating CONNECT with no TLS termination state (ephemeral CA init failed) was written after the 200 Connection Established response. Because a CONNECT client only sends its TLS ClientHello after reading the 200, the peek-based TLS detection is inherently post-200, so the 503 landed inside the established tunnel and surfaced to the client as a TLS protocol error rather than a readable status. An 'allowed CONNECT' event was also logged first. Move the decision to a pre-200 gate: query_tls_mode resolves purely from the policy decision + host/port (no peeked bytes), so the route's TLS treatment is known before the tunnel is acknowledged. When TLS state is absent and the route is not tls: skip, write the 503 as the first bytes on the socket, emit the High-severity Denied event, and close. tls: skip routes tunnel raw exactly as before, and no allowed-CONNECT event is emitted on the refusal path. The now-unreachable post-200 branch is kept as defense in depth but no longer writes an in-tunnel 503; it fails closed by dropping the connection instead. Add connection-level regression tests over a real loopback socket: the gate refuses with HTTP/1.1 503 as the first bytes for a terminating route, and writes nothing when TLS termination is present or the route is tls: skip. Signed-off-by: Tony Luo <xialuo@nvidia.com>
Thanks — this was a real flaw. Fixed in What changed (pre-200 decision):
Defense-in-depth choice: the old post- New connection-level tests (
Validation (run with sandbox proxy env unset): No Fern docs change (code-only degraded-state hardening), matching your docs note. |
Re-check After Author UpdateThanks @TonyLuo-NV. I re-evaluated latest head What I checked: Disposition: partially resolved. The original post- Remaining items:
Next state: |
Addresses the gator re-check on NVIDIA#2162. Ordering: the pre-200 fail-closed refusal ran before SSRF/allowed_ips validation, so during CA-init failure an internal-address CONNECT got a 503 tls_termination_unavailable instead of the normal 403 ssrf_denied, weakening operator visibility in degraded state. The SSRF branches now return validated addresses; the refusal runs after that validation (an internal address has already been denied with 403) but still before the upstream connect and before 200 Connection Established. effective_tls_skip is still resolved up front since the refusal consumes it. Tests: add connection-level regressions through the real handle_tcp_connection, driving a CONNECT from a child /bin/bash copy so the /proc process-identity binding resolves it against a permissive policy (the hot-swap test's identity pattern). They assert the first bytes are HTTP/1.1 503 for a terminating route with no TLS state, a 403 (not 503) for an internal address, and no refusal for a tls: skip route. These are gated to Linux at runtime (evaluate_opa_tcp needs /proc); a companion test verifies the OPA policy shape (glob allow, tls mode) on every platform so the precondition is locked where /proc is unavailable. rest.rs: tighten the fail-closed relay test to assert the forwarded buffer is_empty() rather than merely lacking the placeholder/secret. Signed-off-by: Tony Luo <xialuo@nvidia.com>
All three remaining items are addressed in 1. Ordering — refusal now runs after SSRF/ 2. Handler-level regression through
Approach and why it's shaped this way: Note: I develop on macOS, so the three Linux-gated handler tests execute on CI, not locally. The policy-shape test and the connection-level helper tests below run and pass locally. 3. Validation (sandbox proxy env unset): No Fern docs change (code-only degraded-state hardening), matching your note. |
Re-check After Author UpdateThanks @TonyLuo-NV. I re-evaluated latest head What I checked: Disposition: resolved. The previous gator findings are addressed, and the independent reviewer found no blocking correctness, security, regression, or maintainability findings remaining. Remaining items:
Docs: no Fern docs update is required for this code-only degraded-state hardening. E2E: I applied Next state: |
|
Label |
The handler regression tests forked cat to read the proxy reply, so the client socket fd was inherited by a second process with a different binary. The identity resolver correctly denies that as ambiguous shared-socket ownership (the same invariant resolve_process_identity_denies_fork_exec_shared_socket_ambiguity pins), so the tests exercised the deny path instead of the allow path — and on busy CI runners the deny-path /proc fallback scan exceeded the test budget and looked like a hang. The client script now uses only bash builtins (exec, printf, read -d '') so exactly one process owns the socket, and the child is left to exit on EOF instead of being killed mid-read. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
CI failure on the two handler tests root-caused and fixed in f694fc0 — it was a test-client bug, not a product bug: The test's bash client forked Fix: the client script now uses only bash builtins ( Verified in a Linux (aarch64) container with rust:1.95.0:
The product-side ordering fix from the previous commit is unchanged. |
Re-check After Author UpdateThanks @TonyLuo-NV. I re-evaluated latest head What I checked: Disposition: not resolved. The product-side fail-closed ordering still looks correct, but the current head's Remaining items:
Docs: no Fern docs update is required for this code-only degraded-state hardening. E2E: Next state: |
The child-process client (even fork-free) made the handler tests environment-sensitive: on CI runners with a busy or restricted /proc, resolving the child's socket ownership degraded into the whole-/proc fallback scan and a deny, which surfaced as a hang. The client is now an in-process TcpStream and the test policy allows current_exe(), so identity resolution binds the socket to the test process itself in the descendant scan — the same in-process pattern the passing resolve_process_identity tests rely on. The tls: skip test additionally asserts that the handler emitted no DenialEvent at any stage, so it can no longer pass vacuously on a policy or identity deny. Refusal budgets widened to 30s as a belt for slow runners; the refusals themselves return in milliseconds. Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
The remaining handler-test flake is fixed in cc8aaf4 by removing the child process entirely: Why the previous two attempts still failed on CI: any child-process client (even fork-free bash) makes the test depend on resolving the child's socket ownership through Redesign: the client is now an in-process Non-vacuous tls: skip: the harness now threads a Verified in a Linux (aarch64) rust:1.95.0 container, both with an isolated pid namespace and with |
Re-check After Author UpdateThanks @TonyLuo-NV. I re-evaluated latest head What I checked: Disposition: partially resolved. The product-side fail-closed path looks correct now: SSRF/ Remaining items:
E2E: Next state: |
The no-resolver scan already catches the percent-encoded canonical
marker through its decoded pass; this regression pins it: a request
line carrying openshell%3Aresolve%3Aenv%3AKEY with no resolver must
fail closed with UnresolvedPlaceholderError { location: header }.
Signed-off-by: Tony Luo <xialuo@nvidia.com>
|
@johntmyers The remaining item is addressed in cb5bc51: added E2E: understood that |
Re-check After Author UpdateThanks @TonyLuo-NV. I re-evaluated latest head What I checked: Disposition: resolved. The previous gator finding is addressed, and the independent reviewer found no blocking correctness, security, regression, maintainability, or missing-test issues remaining. Remaining items:
Docs: no Fern docs update is required for this code-only degraded-state hardening. E2E: Next state: |
|
E2E status on cb5bc51: Branch Checks are green (the redesigned in-process handler tests passed on both Linux runners), and the only failure is That surface is disjoint from this PR: the diff touches the HTTP CONNECT proxy path (SSRF→TLS-gate ordering) and the credential-placeholder header rewrite, neither of which is on the sandbox upload/download SSH tar path, and the fail-closed branches only trigger on CA-init failure or reserved markers — neither occurs in that test. The other three E2E matrix legs (docker, podman root, GPU) passed. @johntmyers could you re-run the failed E2E job when you get a chance? (Contributors can't re-run workflows on this repo.) |
|
Additional evidence that the |
Summary
Makes the sandbox egress proxy fail closed when it cannot rewrite a credential placeholder, instead of forwarding the literal
openshell:resolve:env:<NAME>token (or its provider-alias form) to the upstream. Closes two fail-open paths that leak the reserved placeholder on the wire and cause upstream auth failures when internal state is degraded.Related Issue
Fixes #2161 —
[DGX Spark] gateway never rewrites openshell:resolve:env: credential placeholder on sandbox egress — literal placeholder sent on the wire.Full chain for reviewers:
mcp statusstayed all-greenmcp status/mcp addThe invariant enforced here: a live credential placeholder must never leave the proxy unresolved. Degraded internal state fails closed; explicit policy opt-outs (
tls: skip) keep working.Changes
Leak G3 — resolver-
Nonefail-open (crates/openshell-core/src/secrets.rs)rewrite_http_header_blocktook an early return that forwarded the header block verbatim wheneverresolverwasNone. The existing fail-closed marker scan runs only after that early return (on the resolved path), so a header block carrying a live placeholder was passed through untouched. It now scans the header region for the reserved markers (openshell:resolve:env:prefix and theOPENSHELL-RESOLVE-ENV-alias marker, raw and percent-decoded) even when there is no resolver, and returnsUnresolvedPlaceholderError { location: "header" }if one is present. The scan window mirrors the resolved-path scan (header_end + 256). Marker-free traffic still passes through unchanged, so credential-free endpoints are unaffected.Leak G2 —
tls_state == Noneraw-tunnel fallback (crates/openshell-supervisor-network/src/proxy.rs)When TLS was detected on a CONNECT but
tls_statewasNone(ephemeral CA generation or CA file write failed at startup, seerun.rs),handle_tcp_connectionemitted a Low-severity event and fell back to a rawcopy_bidirectionaltunnel — silently bypassing credential rewrite. Inside the proxy handlertls_stateisNoneonly on CA-init failure: themode != Proxycase never starts this handler, andtls: skipis handled by an earlier early return. The handler now refuses the connection with a503 Service UnavailableJSON error and a High-severityDenied/Blockeddenial event instead of tunneling. The explicittls: skipopt-out and all other modes are untouched.Severity escalation (
crates/openshell-supervisor-network/src/run.rs)The two startup CA-failure logs ("Failed to write CA files…" and "Failed to generate ephemeral CA…") are raised from Medium to High, since a disabled TLS termination now fails closed on TLS-bearing connections.
Testing
All run locally with the sandbox HTTP proxy unset (the sandbox
HTTP_PROXY=127.0.0.1:8118intercepts loopback connections and makes the inference-routing socket tests fail spuriously; they pass with it unset and are unrelated to this change).cargo test -p openshell-core secrets::→ 58 passed, 0 failed. New unit tests: placeholder in request line / header value / provider-alias form withresolver=Noneall fail closed; the pre-existing marker-freeresolver=Nonepassthrough test still passes.cargo test -p openshell-supervisor-network→ 812 lib tests + integration passed, 0 failed. The relay integration test (relay_request_without_resolver_fails_closed_on_placeholder) now asserts the request is rejected before any byte reaches upstream; a newtest_json_error_response_503_tls_termination_unavailablelocks the G2 fail-closed response contract.cargo fmt --all -- --check→ clean.cargo clippy -p openshell-core -p openshell-supervisor-network --all-targets -- -D warnings→ clean.A true connection-level test of the G2 branch was not added:
handle_tcp_connectionrequires a realTcpStreamwith a peeked TLS ClientHello and no existing test drives it, so the observable fail-closed response contract is locked via thebuild_json_error_responsetest instead.Two pre-existing tests that documented the vulnerable behavior were updated to assert the fail-closed outcome:
secrets.rs:no_resolver_passes_through_without_scanning→ split into request-line / header-value / alias-marker fail-closed tests.l7/rest.rs:relay_request_without_resolver_leaks_placeholders→relay_request_without_resolver_fails_closed_on_placeholder.Unit tests added/updated
E2E tests added/updated (if applicable)
Checklist
Signed-off-by: Tony Luo xialuo@nvidia.com
🤖 Generated with Claude Code