Skip to content

fix(network): restore SDK gvproxy integration tests#874

Merged
DorianZheng merged 2 commits into
boxlite-ai:mainfrom
G4614:fix/sdk-gvproxy-network-tests
Jun 26, 2026
Merged

fix(network): restore SDK gvproxy integration tests#874
DorianZheng merged 2 commits into
boxlite-ai:mainfrom
G4614:fix/sdk-gvproxy-network-tests

Conversation

@G4614

@G4614 G4614 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Fix SDK network/secrets integration failures by avoiding the current VMM seccomp/gvproxy conflict and making MITM TLS validation work inside the jail.

Test plan:

Summary by CodeRabbit

  • Bug Fixes
    • Improved secure upstream TLS validation by extending sandbox trust to include existing system CA bundles and cert files (read-only).
    • Prevented syscall filtering from disrupting network-enabled runs by skipping the VMM syscall filter when networking is active.
    • Updated network interception integration coverage to use a new upstream endpoint, keeping secret-substitution checks stable.

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ad2c424-76fb-4b57-ac4c-91fb4e09279a

📥 Commits

Reviewing files that changed from the base of the PR and between b632cd7 and 1d45238.

📒 Files selected for processing (1)
  • src/boxlite/src/jailer/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/boxlite/src/jailer/mod.rs

📝 Walkthrough

Walkthrough

The PR updates secret-substitution integration tests to use httpbingo.org, adds read-only system CA paths to the jailer sandbox, loads system root CAs for gvproxy MITM TLS, and skips the VMM seccomp filter when networking is enabled.

Changes

Network secret trust path

Layer / File(s) Summary
Sandbox CA paths
src/boxlite/src/jailer/mod.rs
build_path_access() adds existing system CA paths as read-only PathAccess entries, with a helper list and unit test coverage.
Upstream Root CAs
src/deps/libgvproxy-sys/gvproxy-bridge/mitm.go
resolveUpstreamTLS() caches x509.SystemCertPool() and sets RootCAs when loading succeeds.
httpbingo integration tests
sdks/node/tests/network-secrets.integration.test.ts, sdks/python/tests/test_secret_substitution.py
The secret-substitution integration tests update allowed hosts, secret host lists, and request URLs from httpbin.org to httpbingo.org.

Networked VM seccomp gate

Layer / File(s) Summary
Skip VMM seccomp for networked VMs
src/shim/src/main.rs
run_shim now applies the VMM seccomp filter only when jailer and seccomp are enabled and no network config is present; the networked branch logs a warning and skips the filter.

Sequence Diagram(s)

sequenceDiagram
  participant resolveUpstreamTLS
  participant loadUpstreamRootCAs
  participant "x509.SystemCertPool" as x509SystemCertPool
  resolveUpstreamTLS->>loadUpstreamRootCAs: request cached RootCAs
  loadUpstreamRootCAs->>x509SystemCertPool: load system cert pool once
  x509SystemCertPool-->>loadUpstreamRootCAs: cert pool or error
  loadUpstreamRootCAs-->>resolveUpstreamTLS: return RootCAs and error
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hopped through certs in the morning dew,
And found httpbingo shimmering through.
The jailer kept its CA leaves near,
The shim skipped seccomp when networks appeared,
And secret bytes twinkled, safe and true.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main goal: fixing network-related gvproxy integration test failures.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@G4614 G4614 force-pushed the fix/sdk-gvproxy-network-tests branch 2 times, most recently from d48d4b6 to 09dbea7 Compare June 26, 2026 12:19
@G4614 G4614 force-pushed the fix/sdk-gvproxy-network-tests branch from 09dbea7 to b632cd7 Compare June 26, 2026 12:29
@G4614 G4614 marked this pull request as ready for review June 26, 2026 12:35
@G4614 G4614 requested a review from a team as a code owner June 26, 2026 12:35
@G4614 G4614 enabled auto-merge June 26, 2026 12:35

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/shim/src/main.rs (1)

156-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Update the stale seccomp comment to match this gate.

Line 151 still says the TSYNC filter covers gvproxy, but this branch now skips the filter whenever gvproxy is created in-process. Please update that nearby comment so it does not contradict the warning path.

Suggested wording
-    // Apply VMM seccomp filter with TSYNC (covers all threads including gvproxy)
+    // Apply the VMM seccomp filter only when gvproxy is not running in-process.

As per coding guidelines, “Update nearby comments when behavior changes.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shim/src/main.rs` around lines 156 - 175, The nearby seccomp comment is
stale and contradicts the current gating in main, where the TSYNC filter is
skipped when network_config is present and gvproxy runs in-process. Update that
comment to describe the actual behavior of the jailer_enabled/seccomp_enabled
branch in src/shim/src/main.rs, keeping it consistent with the warning path and
the apply_vmm_filter logic.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/boxlite/src/jailer/mod.rs`:
- Around line 292-299: Update the empty-box test to match the new implicit CA
grants returned by build_path_access(); test_build_path_access_empty_box_dir()
should no longer assert that paths is empty, and instead verify that any
returned entries are only existing system_ca_paths() marked read-only. If these
CA grants are meant to be conditional, adjust the logic in build_path_access()
or the test setup so the behavior is gated consistently.

---

Nitpick comments:
In `@src/shim/src/main.rs`:
- Around line 156-175: The nearby seccomp comment is stale and contradicts the
current gating in main, where the TSYNC filter is skipped when network_config is
present and gvproxy runs in-process. Update that comment to describe the actual
behavior of the jailer_enabled/seccomp_enabled branch in src/shim/src/main.rs,
keeping it consistent with the warning path and the apply_vmm_filter logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1dcc4b9d-1b02-4359-9de1-99d2ba32af4f

📥 Commits

Reviewing files that changed from the base of the PR and between 36b228d and b632cd7.

📒 Files selected for processing (5)
  • sdks/node/tests/network-secrets.integration.test.ts
  • sdks/python/tests/test_secret_substitution.py
  • src/boxlite/src/jailer/mod.rs
  • src/deps/libgvproxy-sys/gvproxy-bridge/mitm.go
  • src/shim/src/main.rs

Comment thread src/boxlite/src/jailer/mod.rs
@DorianZheng DorianZheng disabled auto-merge June 26, 2026 14:10
@DorianZheng DorianZheng merged commit b4890f5 into boxlite-ai:main Jun 26, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants