Skip to content

fix(mxc): redact injected secrets from gateway diagnostics - #3853

Merged
shailendra-nv merged 2 commits into
windowsfrom
fix/mxc-redact-injected-secrets-in-gateway-log
Oct 1, 2026
Merged

shailendra-nv merged 2 commits into
windowsfrom
fix/mxc-redact-injected-secrets-in-gateway-log

Conversation

@pkhodade-NV

@pkhodade-NV pkhodade-NV commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Redact injected environment values and the per-sandbox proxy password from MXC captured output and decoded relay launch failures. Gateway logs and published failure status use scrubbed diagnostics, including when configured values overlap.

Related Issue

Private tracking: NVBug 6843156. This is a localized redaction fix; no public security issue is created.

Changes

  • Build the deduplicated redaction set once per sandbox and scrub captured stdout/stderr before logging.
  • Scrub decoded relay launch-failure diagnostics before both logging and publishing sandbox status/watch events, preserving control-channel payloads.
  • Find matches in the original text and redact their combined coverage, including self-overlap and Unicode boundaries. Unmatched text remains borrowed; replacement markers are never rescanned.
  • Add six regression tests for overlap, Unicode, replacement markers, unmatched input, routed target failures, and routed responses. Retain the four original redaction tests.
  • Document case-sensitive literal matching, the four-UTF-8-byte minimum, transformed-value limitations, and provider placeholders in the architecture, driver README, and published reference.

Testing

Local validation: native Windows ARM64, Rust 1.95.0. Pre-commit uses x64 Python and pinned x64 Biome 2.5.4 to work around ARM64 tooling failures on this host.

Check Result
mise run --skip-tools windows:check:arm64 Passed
mise run --skip-tools windows:test:arm64 5,030 passed, 0 failed, 29 skipped; nextest flagged six passing tests as leaky
mise run pre-commit Passed, including the installed commit hook; Unix-only checks are covered by CI
mise run --skip-tools windows:test:mxc-real:arm64 12 executed passes, 2 capability skips, 1 HTTPS failure; the same HTTP 403 reproduces on original PR head 098897780efe520a5cb1d77f94d99ac487f4deb9
git diff --check Passed
Branch Checks Passed on the final PR head
Windows x64 CI Lint passed; 5,030 tests passed, 29 skipped, five passing tests flagged leaky
Windows ARM64 CI Lint passed; 5,030 tests passed, 29 skipped, five passing tests flagged leaky
  • mise run pre-commit passes
  • Unit tests added/updated
  • Native x64 and ARM64 CI validation
  • Real-MXC integration results recorded

Real-MXC's 14 reported passes include two early-return skips: isolation-session provisioning and the AppContainer admin-access check's host privilege requirement. The HTTPS test receives HTTP 403 from example.com on both this change and an isolated archive of the original PR head. Linux sandbox E2E is outside this Windows diagnostic-only change.

The first ARM64 CI attempt passed 5,029 tests and failed an unchanged DNS fixture's UDP socket bind (os error 10013). Retrying only the failed job on a fresh runner passed all 5,030 tests without code changes.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated

@copy-pr-bot

copy-pr-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

A sandboxed cmd.exe entry point without `@echo off` echoes its own
expanded command line to stdout before running it. The driver captured
wxc-exec's stdout verbatim into the gateway log at INFO level, so a
workload that merely references an injected --env/--env-from secret
(e.g. `echo %SECRET_VAR%`) leaked the literal value twice: once in the
echoed command line, once in the command's own output. No debug flag
or elevated verbosity was required, and the leak bypassed redaction
entirely since the secret arrived as ordinary unstructured stdout text.

Build a redaction set from the sandbox's injected environment values
and the generated egress-proxy password, and scrub every captured
stdout/stderr line against it before logging. Provider credentials are
excluded deliberately -- per append_provider_child_env's existing doc
comment, those are already revision-scoped placeholders by the time
they reach process.env, not the raw secret.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Prashant Khodade <pkhodade@nvidia.com>
@pkhodade-NV
pkhodade-NV force-pushed the fix/mxc-redact-injected-secrets-in-gateway-log branch from cae29f4 to 0988977 Compare September 29, 2026 14:46

@shailendra-nv shailendra-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on commit 0988977. I reviewed the complete diff (one file, 135 additions and two deletions), surrounding lifecycle/control-channel code, provider environment handling, Windows validation guidance, and CI status.

Two blocking findings are attached inline, ordered by severity:

  1. High: extend redaction to decoded relay failure diagnostics before logging or publishing sandbox failure status.
  2. Medium: redact overlapping matches against the original input so earlier replacements cannot leave fragments of another configured value.

Nonblocking suggestions:

  • Document exact-match behavior and the four-byte minimum in the existing Windows architecture/driver documentation.
  • Precompute secret ordering once per sandbox instead of sorting every output line.
  • Remove the AI co-author attribution from the commit, as required by the repository's commit instructions.

Provider-credential exclusion matches the existing placeholder contract. The implementation remains Windows-gated and introduces no architecture-specific operations; I found no distinct x64/ARM64 correctness issue. Gateway validation rejects multiline environment values.

Validation:

  • Exact-head Rust formatting and git diff --check passed.
  • The four new tests exercise helpers but do not verify lifecycle logging or routed failures.
  • The PR reports 118 passing tests; I did not independently run the crate, Windows lanes, or real-MXC suite.
  • At review time, Branch Checks and Helm Lint were pending, awaiting the /ok to test mirror. Windows validation had not run and test:windows was absent. E2E/GPU statuses were green because their labels were absent, not because the suites ran. DCO and informational checks passed.

Merge conflicts: none in the local merge calculation against the fetched windows tip 7ba7a39. The PR head was rechecked immediately before submission and is unchanged.

Comment thread crates/openshell-driver-mxc/src/driver.rs
Comment thread crates/openshell-driver-mxc/src/driver.rs Outdated
Signed-off-by: Shailendra Singh <shailendras@nvidia.com>
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

@shailendra-nv shailendra-nv added the test:windows Run native Windows x64 and ARM64 lint/tests on PR mirrors label Oct 1, 2026
@shailendra-nv shailendra-nv changed the title fix(mxc): redact injected secrets echoed into the gateway log fix(mxc): redact injected secrets from gateway diagnostics Oct 1, 2026

@shailendra-nv shailendra-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the cumulative change at d13a141d98bec26e7990accdfb36310824bb2f8c. Both blocking findings are addressed:

  • Decoded relay failure diagnostics pass through the production launch-failure helper before either warning, registry status, or watch-event publication. Regression tests cover routed target failures and response failures while verifying that protocol payloads remain unchanged.
  • Redaction uses the union of matches in the original input, covering containment, partial overlap, adjacent and self-overlapping matches, Unicode boundaries, and replacement-marker text. Unmatched lines remain borrowed, and the secret set is prepared once per sandbox.

The architecture, driver README, and published reference now document literal case-sensitive matching and the four-UTF-8-byte minimum. The follow-up commit is signed off and has no agent attribution; the final squash message will also omit attribution from the original commit.

Local validation: native ARM64 check passed; all 5,030 workspace tests passed (29 skipped, six passing tests flagged leaky by nextest); full mise run pre-commit and the installed commit hook passed. Real MXC validation executed 12 passing checks, skipped two capability-dependent checks, and failed one outbound HTTPS check with HTTP 403. An isolated archive of the original PR head produces the identical real-MXC result, so that failure is not introduced by this follow-up. The PR description records the commands and limitations.

No remaining blocking code-review findings. Ready for CI testing on this head. Merge remains contingent on the standard PR checks and native Windows x64/ARM64 validation.

@shailendra-nv

Copy link
Copy Markdown
Collaborator

/ok to test d13a141

@shailendra-nv shailendra-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving d13a141. Both blocking findings are fixed, the regression tests cover decoded failure logging/status and overlapping matches, and the documented redaction limits are accurate. No unresolved review threads or remaining blocking findings.

Local pre-commit and all 5,030 native ARM64 workspace tests passed. Branch Checks passed, and native Windows x64/ARM64 CI each passed all 5,030 tests (29 skipped). The ARM64 retry passed after an isolated UDP-bind failure in an unchanged DNS fixture; no code changes were needed. The pre-existing real-MXC HTTPS 403 and capability skips are recorded in the PR description.

Ready for squash merge with the prepared signed-off commit message, which omits agent attribution.

@shailendra-nv
shailendra-nv merged commit 9ceec26 into windows Oct 1, 2026
71 of 72 checks passed
@shailendra-nv
shailendra-nv deleted the fix/mxc-redact-injected-secrets-in-gateway-log branch October 1, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:windows Run native Windows x64 and ARM64 lint/tests on PR mirrors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants