feat(sandbox): import host corporate proxy CA into sandbox trust (#6210) - #6292
Merged
Conversation
On networks where a corporate MITM proxy sits in front of the host and re-signs external TLS with its own root, that root is absent from the sandbox trust path. OpenShell injects only its own L7-proxy CA, so external endpoints (e.g. api.telegram.org) fail verification even when the network policy allows the connection (NET:OPEN then NET:FAIL). Import an operator-supplied corporate CA without replacing the OpenShell CA: - Add src/lib/onboard/corporate-ca.ts: validate a host CA bundle (regular file, non-symlink, non-world-writable, bounded size, PEM) from NEMOCLAW_CORPORATE_CA_BUNDLE (explicit, fail-loud) or the conventional REQUESTS_CA_BUNDLE / CURL_CA_BUNDLE / SSL_CERT_FILE fallbacks (skip-on-invalid). Opt out with NEMOCLAW_CORPORATE_CA_IMPORT=0. - Bake it via a base64 NEMOCLAW_CORPORATE_CA_B64 build arg decoded to a root-owned 0444 file in the OpenClaw and Hermes Dockerfiles. - At entrypoint startup, append the baked CA to the OpenShell bundle into a merged /tmp bundle (never replacing the OpenShell CA, preserving #1828) and repoint SSL_CERT_FILE, CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE, GIT_SSL_CAINFO, and NODE_EXTRA_CA_CERTS at it, including connect sessions. Tests: host CA validation unit tests, dockerfile-patch baking, runtime merge for both entrypoints, a simulated-MITM TLS test proving a corporate-CA-signed endpoint verifies only after the merge while the OpenShell root stays trusted, and troubleshooting docs. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds corporate proxy CA discovery, validation, Dockerfile baking, runtime trust-bundle merging, connect-session propagation, host-anchor fallback discovery, troubleshooting documentation, and integration coverage. ChangesCorporate proxy CA onboarding and runtime trust
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Onboard
participant DockerBuild
participant SandboxStartup
participant ConnectSession
participant TLSClient
Onboard->>DockerBuild: provide NEMOCLAW_CORPORATE_CA_B64
DockerBuild->>SandboxStartup: install corporate-ca.pem
SandboxStartup->>SandboxStartup: merge OpenShell and corporate bundles
SandboxStartup->>ConnectSession: export merged CA variables
TLSClient->>SandboxStartup: use merged trust bundle
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6292.docs.buildwithfern.com/nemoclaw |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 10 items to resolve/justify, 1 in-scope improvement
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Move the branching openssl/TLS/shell-block helpers out of the corporate CA .test.ts files into test/helpers/corporate-ca-support.ts so the changed test files add no if statements (codebase-growth-guardrails keeps test bodies linear). Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
| export function httpsGetStatus(port: number, caBundlePath: string): Promise<number> { | ||
| return new Promise((resolve, reject) => { | ||
| const req = https.get( | ||
| { host: "127.0.0.1", port, path: "/", ca: fs.readFileSync(caBundlePath) }, |
Collaborator
Author
There was a problem hiding this comment.
This is a test-only false positive. corporate-ca-support.ts is a Vitest support helper (never shipped); httpsGetStatus makes a loopback (127.0.0.1) request to an in-test https.createServer and reads the ca bundle from a temp file the test itself generated via openssl. There is no untrusted file data or real outbound egress — the whole point is to verify TLS against a locally-generated corporate CA. Safe to dismiss as test infrastructure.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/corporate-ca-tls-e2e.test.ts (1)
1-224: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCI guardrail failure: added
ifstatements in a test file.Pipeline reports the file's
ifcount grew from 0 to 5 at PR head (Lines 107, 108, 132, 170, and one more). This is flagged by the Codebase Growth Guardrails check and needs to be resolved or explicitly justified/waived before merge — several of these (e.g., the CI-vs-local skip guard at Lines 107-113) are legitimate environment-detection logic, but the guardrail still needs to pass or be exempted.🤖 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 `@test/corporate-ca-tls-e2e.test.ts` around lines 1 - 224, The test file now introduces several new if statements that trigger the Codebase Growth Guardrails check. Refactor the environment/setup flow in corporate-ca-tls-e2e.test.ts by moving the conditional logic out of the test body into small helper functions or existing hooks around trySetup, afterAll, and describe.skipIf so the file’s if count does not increase, or explicitly request a guardrail waiver if the logic must remain as-is.Source: Pipeline failures
🧹 Nitpick comments (4)
test/corporate-ca-runtime-merge.test.ts (1)
63-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest titles mostly missing the required
(#6210)suffix.Only the
describeblocks and oneittitle include the issue reference; the otherit()titles (Lines 63, 97, 116, 150) omit it. As per coding guidelines, root-level integration tests undertest/should "use behavior-oriented titles with local issue refs in a final(#1234)suffix."🤖 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 `@test/corporate-ca-runtime-merge.test.ts` around lines 63 - 200, Update the affected `it()` titles in `corporate-ca-runtime-merge.test.ts` so each behavior-oriented test name ends with the required `(`#6210`)` suffix; keep the existing meaning but append the local issue reference consistently to every top-level test case, including the OpenClaw merge, no-op, Hermes merge, and connect-session merge tests.Source: Coding guidelines
test/corporate-ca-tls-e2e.test.ts (1)
198-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMost
it()titles omit the required issue-ref suffix.Only "still trusts the OpenShell root through the merged bundle (
#6210)" (Line 214) carries the suffix; "verifies a corporate-CA-signed endpoint only after the merge" (Line 198) does not. As per coding guidelines, root-level integration tests undertest/should "use behavior-oriented titles with local issue refs in a final(#1234)suffix."🤖 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 `@test/corporate-ca-tls-e2e.test.ts` around lines 198 - 222, The root-level integration test title in this block is missing the required local issue-ref suffix; update the unsuffixed it() description in the corporate CA merge test so it ends with a behavior-oriented “(`#1234`)” style reference, matching the existing convention already used by the neighboring it() case in corporate-ca-tls-e2e.test.ts.Source: Coding guidelines
docs/reference/troubleshooting.mdx (1)
977-987: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit multi-sentence lines to one sentence per line.
Lines 977 and 986 each pack 3 sentences onto a single source line, violating the docs formatting convention.
📝 Proposed reflow
-On networks where a corporate proxy in front of the host re-signs external TLS with its own root CA, external endpoints such as `api.telegram.org` fail certificate verification even when the network policy allows the connection. Logs show the request opening (`NET:OPEN ... api.telegram.org:443`) followed by `NET:FAIL`. OpenShell injects only its own L7-proxy CA into the sandbox, so the separate corporate root is missing from the trust path. +On networks where a corporate proxy in front of the host re-signs external TLS with its own root CA, external endpoints such as `api.telegram.org` fail certificate verification even when the network policy allows the connection. +Logs show the request opening (`NET:OPEN ... api.telegram.org:443`) followed by `NET:FAIL`. +OpenShell injects only its own L7-proxy CA into the sandbox, so the separate corporate root is missing from the trust path.-`REQUESTS_CA_BUNDLE`, `CURL_CA_BUNDLE`, and `SSL_CERT_FILE` are also honored as fallbacks (in that order) when `NEMOCLAW_CORPORATE_CA_BUNDLE` is unset, so an environment that already exports one of those for the corporate proxy works without extra configuration. The bundle must be a readable, non-symlink, non-world-writable PEM file that contains at least one certificate. To disable the import entirely, set `NEMOCLAW_CORPORATE_CA_IMPORT=0`. +`REQUESTS_CA_BUNDLE`, `CURL_CA_BUNDLE`, and `SSL_CERT_FILE` are also honored as fallbacks (in that order) when `NEMOCLAW_CORPORATE_CA_BUNDLE` is unset, so an environment that already exports one of those for the corporate proxy works without extra configuration. +The bundle must be a readable, non-symlink, non-world-writable PEM file that contains at least one certificate. +To disable the import entirely, set `NEMOCLAW_CORPORATE_CA_IMPORT=0`.Based on learnings and coding guidelines: "Keep one sentence per line in Markdown and MDX source files."
🤖 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 `@docs/reference/troubleshooting.mdx` around lines 977 - 987, The troubleshooting section in the MDX source has multiple sentences on the same line, which violates the one-sentence-per-line convention. Reflow the prose in the affected paragraph so each sentence is on its own source line, including the sentences describing the proxy/TLS behavior and the fallback environment variables, while keeping the existing wording and structure intact.Source: Coding guidelines
src/lib/onboard/dockerfile-patch.test.ts (1)
160-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore env vars after test instead of leaving them deleted.
Test 1 correctly restores
NEMOCLAW_CORPORATE_CA_BUNDLEin afinally, but this test permanently deletesREQUESTS_CA_BUNDLE,CURL_CA_BUNDLE, andSSL_CERT_FILEwith no save/restore, unlike the careful pattern used elsewhere. If a prior test or the host environment set any of these, this silently strips them for the rest of the suite run.🧪 Proposed fix to save/restore prior values
- it("leaves NEMOCLAW_CORPORATE_CA_B64 empty when no corporate CA is configured", () => { - delete process.env.NEMOCLAW_CORPORATE_CA_BUNDLE; - delete process.env.REQUESTS_CA_BUNDLE; - delete process.env.CURL_CA_BUNDLE; - delete process.env.SSL_CERT_FILE; + it("leaves NEMOCLAW_CORPORATE_CA_B64 empty when no corporate CA is configured", () => { + const savedEnv = { + NEMOCLAW_CORPORATE_CA_BUNDLE: process.env.NEMOCLAW_CORPORATE_CA_BUNDLE, + REQUESTS_CA_BUNDLE: process.env.REQUESTS_CA_BUNDLE, + CURL_CA_BUNDLE: process.env.CURL_CA_BUNDLE, + SSL_CERT_FILE: process.env.SSL_CERT_FILE, + }; + for (const key of Object.keys(savedEnv)) delete process.env[key]; + try {Wrap the remainder of the test body and restore
savedEnventries in afinally.🤖 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/lib/onboard/dockerfile-patch.test.ts` around lines 160 - 165, The test that exercises dockerfileWith for the “no corporate CA configured” case is mutating REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE, and SSL_CERT_FILE without restoring their previous values. Save the original values at the start of the test, run the existing assertions using dockerfileWith, and restore each env var in a finally block the same way the neighboring NEMOCLAW_CORPORATE_CA_BUNDLE test does so the suite does not leak environment changes.
🤖 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 `@agents/hermes/start.sh`:
- Around line 1406-1440: `merge_corporate_proxy_ca` in `agents/hermes/start.sh`
can silently build an incomplete trust bundle because `cat` failures are
swallowed and the merged CA is still exported. Update the function so any
write/read failure while assembling `$_merged` causes the merge to fail closed:
check each append step in `merge_corporate_proxy_ca` and only set
`SSL_CERT_FILE`, `NODE_EXTRA_CA_CERTS`, and `_NEMOCLAW_CORPORATE_CA_MERGED`
after a fully successful merge. Keep the existing behavior of appending the
corporate CA to the base bundle, but do not mark the merge successful if the
bundle could not be written completely.
In `@scripts/nemoclaw-start.sh`:
- Around line 2726-2751: The merge_corporate_proxy_ca helper is repointing
CA-related environment variables even when building the merged bundle fails
partway. In merge_corporate_proxy_ca, stop suppressing the append failures for
the base bundle and corporate CA, verify the merged file contains both inputs
successfully, and only then export SSL_CERT_FILE, CURL_CA_BUNDLE,
REQUESTS_CA_BUNDLE, GIT_SSL_CAINFO, NODE_EXTRA_CA_CERTS and set
_NEMOCLAW_CORPORATE_CA_MERGED. If either append fails, leave the original trust
settings untouched and fail closed.
In `@src/lib/onboard/corporate-ca.ts`:
- Around line 68-102: `validateCorporateCaFile` has a TOCTOU race because it
validates `filePath` with `fs.lstatSync` and then re-reads the same path with
`fs.readFileSync`. Update the function to validate and read through a single
file descriptor so the checked file is the one whose contents are loaded, while
keeping the existing `lstatSync` symlink rejection and the current
size/permission/content checks in `validateCorporateCaFile` and
`CorporateCaValidationError` handling.
In `@test/corporate-ca-runtime-merge.test.ts`:
- Around line 63-148: Add a Hermes negative-path test alongside
merge_corporate_proxy_ca that mirrors the OpenClaw no-op case: when the
corporate CA file is absent, the Hermes merge block should leave SSL_CERT_FILE
and NODE_EXTRA_CA_CERTS unchanged and should not set
_NEMOCLAW_CORPORATE_CA_MERGED. Reuse the existing test helpers and symbols in
corporate-ca-runtime-merge.test.ts such as HERMES_START, sliceBlock, mergeBlock,
and runShell to build the scenario and assert the merge marker stays unset while
the original CA path remains intact.
- Around line 1-201: The Codebase Growth Guardrails check is failing because
sliceBlock introduced a new explicit if guard. Refactor sliceBlock so the
start/end validation no longer uses an added if statement, or move that
validation into an existing helper already counted in this file; keep the same
error behavior while preserving the runtime extraction logic used by mergeBlock
and the Hermes/OpenClaw tests.
---
Outside diff comments:
In `@test/corporate-ca-tls-e2e.test.ts`:
- Around line 1-224: The test file now introduces several new if statements that
trigger the Codebase Growth Guardrails check. Refactor the environment/setup
flow in corporate-ca-tls-e2e.test.ts by moving the conditional logic out of the
test body into small helper functions or existing hooks around trySetup,
afterAll, and describe.skipIf so the file’s if count does not increase, or
explicitly request a guardrail waiver if the logic must remain as-is.
---
Nitpick comments:
In `@docs/reference/troubleshooting.mdx`:
- Around line 977-987: The troubleshooting section in the MDX source has
multiple sentences on the same line, which violates the one-sentence-per-line
convention. Reflow the prose in the affected paragraph so each sentence is on
its own source line, including the sentences describing the proxy/TLS behavior
and the fallback environment variables, while keeping the existing wording and
structure intact.
In `@src/lib/onboard/dockerfile-patch.test.ts`:
- Around line 160-165: The test that exercises dockerfileWith for the “no
corporate CA configured” case is mutating REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE,
and SSL_CERT_FILE without restoring their previous values. Save the original
values at the start of the test, run the existing assertions using
dockerfileWith, and restore each env var in a finally block the same way the
neighboring NEMOCLAW_CORPORATE_CA_BUNDLE test does so the suite does not leak
environment changes.
In `@test/corporate-ca-runtime-merge.test.ts`:
- Around line 63-200: Update the affected `it()` titles in
`corporate-ca-runtime-merge.test.ts` so each behavior-oriented test name ends
with the required `(`#6210`)` suffix; keep the existing meaning but append the
local issue reference consistently to every top-level test case, including the
OpenClaw merge, no-op, Hermes merge, and connect-session merge tests.
In `@test/corporate-ca-tls-e2e.test.ts`:
- Around line 198-222: The root-level integration test title in this block is
missing the required local issue-ref suffix; update the unsuffixed it()
description in the corporate CA merge test so it ends with a behavior-oriented
“(`#1234`)” style reference, matching the existing convention already used by the
neighboring it() case in corporate-ca-tls-e2e.test.ts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6561ae0c-4297-4e8e-b95e-68be73060d9c
📒 Files selected for processing (11)
Dockerfileagents/hermes/Dockerfileagents/hermes/start.shdocs/reference/troubleshooting.mdxscripts/nemoclaw-start.shsrc/lib/onboard/corporate-ca.test.tssrc/lib/onboard/corporate-ca.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/dockerfile-patch.tstest/corporate-ca-runtime-merge.test.tstest/corporate-ca-tls-e2e.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/helpers/corporate-ca-support.ts (1)
38-67: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDynamic
execSynccalls flagged by static analysis.All interpolated values (
cn,keyOut,certOut,caCert,caKey) are hardcoded literals from call sites, so there's no current injection vector. Still, usingexecFileSync/array-args here would remove the pattern entirely and avoid relying on call-site discipline if these helpers are reused with less-trusted inputs later.🤖 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 `@test/helpers/corporate-ca-support.ts` around lines 38 - 67, The openssl helper commands in opensslReqX509 and signLeaf are built with interpolated strings that static analysis flags as dynamic exec usage. Replace the execSync shell-string calls with a non-shell process invocation pattern such as execFileSync plus explicit argument arrays, and keep the same behavior for generating the CA, CSR, and signed leaf certificate while preserving the existing path.join-based file targets.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@test/helpers/corporate-ca-support.ts`:
- Around line 38-67: The openssl helper commands in opensslReqX509 and signLeaf
are built with interpolated strings that static analysis flags as dynamic exec
usage. Replace the execSync shell-string calls with a non-shell process
invocation pattern such as execFileSync plus explicit argument arrays, and keep
the same behavior for generating the CA, CSR, and signed leaf certificate while
preserving the existing path.join-based file targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f7f8ff80-1825-49c8-8548-cf26c3c66bea
📒 Files selected for processing (3)
test/corporate-ca-runtime-merge.test.tstest/corporate-ca-tls-e2e.test.tstest/helpers/corporate-ca-support.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/corporate-ca-runtime-merge.test.ts
Address PR review advisor findings on the corporate-proxy CA import: - corporate-ca.ts: open the candidate once with O_NOFOLLOW and validate the opened fd via fstat + read-from-fd (closes the validate-then-reopen TOCTOU); reduce the size cap to 128 KiB, add a certificate-count cap, and structurally validate the leading block as X.509 so a full OS trust store or corrupt PEM is rejected. - dockerfile-patch.ts: fail loudly when NEMOCLAW_CORPORATE_CA_BUNDLE is set explicitly but the staged Dockerfile lacks the ARG; log which host source (env + path) is baked so a fallback import is never silent. - nemoclaw-start.sh / hermes start.sh: build the merged bundle in a mktemp sibling with every write checked, then atomically rename into place; only export the CA env + marker after the complete bundle exists (no partial/predictable-path bundle, no success-on-append-failure). - Dockerfile / hermes Dockerfile: decode with `base64 --decode`. - Extract the dockerfile-patch corporate-CA tests into their own focused file; document explicit-first usage, host trust-store guidance, and the fallback silent-skip behavior in troubleshooting. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
|
Addressed the PR Review Advisor findings in Fixed
Justified
Test follow-ups (PRA-T)*: runtime is covered by executed tests — The earlier |
21 tasks
Second review round on the corporate-proxy CA import:
- hermes start.sh: the merge now exports CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE,
and GIT_SSL_CAINFO explicitly. Previously it set only SSL_CERT_FILE and
relied on the downstream ${VAR:-…} defaulting, which kept an
OpenShell-preset value pointing at the OpenShell-only bundle so Hermes
curl/python/git would not trust the corporate CA after a merge.
- corporate-ca.ts: structurally validate every PEM block as X.509, not just
the first, so a valid leading cert cannot smuggle in a corrupt later block.
- troubleshooting.mdx: stop suggesting NEMOCLAW_CORPORATE_CA_BUNDLE point at
the full OS trust store (which the size/cert caps reject); tell users to
export just their corporate root/intermediates into a PEM.
- Tests: assert the Hermes merge overrides preset CURL/REQUESTS/GIT, cover a
later-block structural rejection, and add a merge-failure negative test
proving the entrypoint bails without exporting when the bundle can't be
written.
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
|
Second review round (advisors both now report 0 required fixes, Fixed
Justified
The earlier |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/corporate-ca-runtime-merge.test.ts`:
- Around line 144-164: The failure-mode coverage is incomplete because this test
only exercises mergeBlock(OPENCLAW_START, ...) and not the equivalent Hermes
path. Add a matching negative test for agents/hermes/start.sh that uses the
Hermes mergeBlock flow and asserts the same bail-out behavior when the merged
bundle cannot be written:
SSL_CERT_FILE/CURL_CA_BUNDLE/REQUESTS_CA_BUNDLE/GIT_SSL_CAINFO stay unchanged,
the merge marker is not set, and the merged file does not exist. Keep the test
structure parallel to the existing corporate-ca-runtime-merge.test case so both
OpenClaw and Hermes merge implementations are covered symmetrically.
- Line 144: The test title in the root-level integration suite needs a
behavior-oriented issue-ref suffix; update the `it(...)` description in
`corporate-ca-runtime-merge.test.ts` to keep the behavior focus and end with a
local issue reference in the required `(`#1234`)` format. Use the existing test
case name as the anchor and adjust only the title string so it matches the repo
convention for `test/**/*.test.ts` cases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fcd87a35-1c60-4f33-9783-6ee6b8ce7ba4
📒 Files selected for processing (5)
agents/hermes/start.shdocs/reference/troubleshooting.mdxsrc/lib/onboard/corporate-ca.test.tssrc/lib/onboard/corporate-ca.tstest/corporate-ca-runtime-merge.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/onboard/corporate-ca.test.ts
- docs/reference/troubleshooting.mdx
- agents/hermes/start.sh
- src/lib/onboard/corporate-ca.ts
Collaborator
Author
|
Round-3 advisor re-review (head
This is a complete fix for the reported failure, not a workaround, so Other resolve/justify items: ambient-fallback trust is validated + capped + opt-out ( Full disposition record retained locally. A maintainer makes the final merge decision. |
- Add the `(#6210)` issue-ref suffix to the runtime-merge test titles per the root-level test convention. - Add a Hermes bail-path negative test mirroring the OpenClaw one, proving the Hermes entrypoint also leaves OpenShell-only trust intact and sets no merge marker when the merged bundle cannot be written. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
21 tasks
…se64 Address the required PR-advisor findings on #6210: - PRA-3: the corporate-CA bundle merged into a predictable /tmp path (/tmp/nemoclaw-ca-bundle.pem). Before the rename, drop any pre-planted symlink at the target so we rename into a fresh regular file we own rather than through an attacker-controlled link. Applied to both scripts/nemoclaw-start.sh and agents/hermes/start.sh. - PRA-4: the Dockerfile decoded NEMOCLAW_CORPORATE_CA_B64 with `base64 --decode` without checking the tool exists; add a build-time `command -v base64` guard that fails the build with a clear message. - PRA-5 (justify, no change): GIT_SSL_CAINFO is already propagated to connect sessions by the pre-existing #2270 block whenever set (the merge exports it to the same bundle), so the corporate-CA propagation loop intentionally omits it to avoid a duplicate export; documented inline. corporate-ca-runtime-merge + corporate-ca-tls-e2e suites pass; shell syntax verified. SKIP=test-cli: shell/Dockerfile-only change; the full vitest hook can trip on pre-existing macOS bash 3.2 noise if this branch predates #6140. CI runs bash 5.x green. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
21 tasks
rebuild-prepared-recovery.test.ts relied on NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE being set ambiently by whatever ran earlier. The cli project runs fileParallelism:false in a single fork, so when rebuild-usage-notice (which exercises the not-accepted path) runs earlier in a shard it leaves the flag unset, and the rebuild preflight bails with 'Third-party software notice was not accepted' — an order-dependent flake that surfaced on cli-test-shards (2). Set the flag in beforeEach and snapshot/restore it so the test is deterministic regardless of file order. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28813628210
|
cv
requested changes
Jul 9, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Re-review for exact head 51ed9da7989b026216dbcd979f0fa3c43680028f narrows the remaining source work:
- Host-anchor candidate validation must distinguish an actual CA from a valid leaf certificate, or explicitly document and test a legacy leaf-certificate policy. On a supported Ubuntu 24.04 host with empty administrator anchor directories, the current literal
/etc/ssl/certs/diagnostic treatsssl-cert-snakeoil.pemas a possible corporate CA even though its X.509 Basic Constraints reportca=false, producing a noisy false warning on the normal host. - Fail closed if
chmod 0444 "$_tmp"fails in bothscripts/nemoclaw-start.shandagents/hermes/start.sh.mktempcreates mode 0600; continuing after chmod failure can install/export a merged bundle the later non-root agent cannot read. Add negative coverage for both entrypoints. - The cohesion rationale accepted on the earlier, smaller module no longer covers the current 609-line
src/lib/onboard/corporate-ca.ts, which now combines trust policy, X.509 validation, env resolution, recursive scanning, literal-directory diagnostics, orchestration, and encoding, alongside duplicated runtime merge implementations. Split shared responsibilities, or obtain an explicit architecture override with a recorded bounded follow-up before merge.
The exact-head live evidence is acknowledged as green: https://github.com/NVIDIA/NemoClaw/actions/runs/29045770653 passed cloud-onboard, onboard-repair, and onboard-resume. No additional live rerun is requested until the source changes above move the head. After those changes, sync current main, resume CodeRabbit, and refresh the advisors and required exact-head live evidence.
Contributor
E2E Target Results — ✅ All selected jobs passedRun: 29051939394
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
E2E Target Results — ✅ All selected jobs passedRun: 29052388718
|
Contributor
E2E Target Results — ✅ All selected jobs passedRun: 29053237188
|
Collaborator
|
Update at head
Verification is green:
Could you please re-review when you get a chance? |
Collaborator
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Collaborator
|
Exact-head maintainer/security disposition for
I have asked CodeRabbit to resume. No new source change is requested from this review. Please merge current |
cv
requested changes
Jul 10, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Exact-head re-review at 16a8e53954658c2a90934df083034dcf92bf9fa6: the main merge is mechanical (9ac7e4dc + ad518b39, empty remerge diff), so the prior production security review remains clean across all nine categories. CodeRabbit also reports no actionable exact-head findings.
Two small current-tree fixes remain:
cloud-onboard.test.tsandonboard-repair.test.tscreate their corporate-CA fixtures before the skip-capable Docker prerequisite probe, but register cleanup only after that probe. Register fixture cleanup immediately after creation, asonboard-resume.test.tsalready does, so a local prerequisite skip cannot leak the temporary CA directory.- Remove the unused
CORPORATE_CA_EXPLICIT_ENVimport identified by CodeQL.
The Nemotron job failure is a provider/tool-result failure, not a required source finding; its partial duplication warnings remain dispositioned as nonblocking. After the two fixes, sync current main (now one docs-only commit ahead), refresh both exact-head advisors/ordinary checks, and rerun the required cloud-onboard, onboard-repair, and onboard-resume targets. The last live run passed at 9ac7e4dc, not this head.
21 tasks
Contributor
E2E Target Results — ✅ All selected jobs passedRun: 29103158958
|
Collaborator
|
Exact-head follow-up at |
cv
approved these changes
Jul 10, 2026
21 tasks
cv
pushed a commit
that referenced
this pull request
Jul 11, 2026
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - #6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - #6584 / #6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - #6210 / #6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - #6624 / #6623 / #6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - #6629 / #6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - #6634 / #6626 / #6596 / #5569 / #6610 / #6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - #6578 / #5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@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 * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…DIA#6210) (NVIDIA#6292) ## Summary On networks where a corporate MITM proxy sits in front of the host and re-signs external TLS with its own root CA, that corporate root is absent from the sandbox trust path. OpenShell injects only its own L7-proxy CA, so external channel endpoints (e.g. `api.telegram.org`) fail certificate verification even when the network policy allows the connection (logs show `NET:OPEN` then `NET:FAIL`). This imports an operator-supplied corporate CA into the sandbox trust bundle without replacing the OpenShell CA. ## Related Issue Fixes NVIDIA#6210 ## Changes - Add `src/lib/onboard/corporate-ca.ts`: validate a host CA bundle (regular file, non-symlink, non-world-writable, bounded size, contains a PEM certificate) from `NEMOCLAW_CORPORATE_CA_BUNDLE` (explicit, fail-loud) or the conventional `REQUESTS_CA_BUNDLE` / `CURL_CA_BUNDLE` / `SSL_CERT_FILE` fallbacks (skip-on-invalid). Opt out with `NEMOCLAW_CORPORATE_CA_IMPORT=0`. - Bake the validated CA into the OpenClaw and Hermes images via a base64 `NEMOCLAW_CORPORATE_CA_B64` build arg decoded to a root-owned `0444` file (`dockerfile-patch.ts` + both Dockerfiles). - At entrypoint startup (`scripts/nemoclaw-start.sh`, `agents/hermes/start.sh`), append the baked CA to the OpenShell bundle into a merged `/tmp` bundle — **never replacing** the OpenShell CA (preserves NVIDIA#1828) — and repoint `SSL_CERT_FILE`, `CURL_CA_BUNDLE`, `REQUESTS_CA_BUNDLE`, `GIT_SSL_CAINFO`, and `NODE_EXTRA_CA_CERTS` at it, including for `connect` sessions. No-corporate-CA path is byte-for-byte unchanged. - Document the flow in `docs/reference/troubleshooting.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) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: host CA input is validated (no symlink/world-writable/oversize; PEM required); the CA is a public certificate baked as a root-owned `0444` file; OpenShell CA is appended, never replaced; no secrets involved. ## 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 — all 28 prek pre-commit/pre-push hooks passed (shellcheck, hadolint, biome, source-shape budget, test-file-size budget, env-var docs, test-title-style); commit-msg clean on this commit. - [x] Targeted behavior tests pass for the current change set — `vitest run` on `corporate-ca.test.ts`, `dockerfile-patch.test.ts`, `corporate-ca-runtime-merge.test.ts`, `corporate-ca-tls-e2e.test.ts` → 52 passed. The TLS test spins a server whose leaf is signed only by a simulated corporate CA and proves verification succeeds only after the real entrypoint merge while the OpenShell root stays trusted. - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without warnings (doc changes only) ### E2E note The reporter's DGX Station + real corporate MITM proxy is not available in CI. The trust-store behavior is reproduced hermetically without that hardware: `test/corporate-ca-tls-e2e.test.ts` generates a corporate root CA, signs a leaf with it, runs the **actual** `merge_corporate_proxy_ca` block extracted from `scripts/nemoclaw-start.sh`, and asserts a Node TLS request verifies the corporate-signed endpoint only after the merge (pre-fix: `unable to verify` / `UNABLE_TO_GET_ISSUER`; post-fix: `200`), and that the OpenShell root remains trusted through the merged bundle. --- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added operator-controlled corporate proxy CA import with validation and optional image baking, plus host anchor scanning fallback. * Merges corporate and OpenShell CA trust at startup and updates TLS trust settings for curl/Python/Git/Node; connect-session shells inherit the merged trust (including Hermes). * **Bug Fixes** * Improves reliability of external TLS connections behind corporate MITM proxies while preserving OpenShell-root trust; includes safer merge/guard behavior. * **Documentation** * Added troubleshooting guidance for corporate MITM TLS failures and how to supply/override corporate CA trust. * **Tests** * Added runtime merge, Dockerfile patching, and end-to-end TLS coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - NVIDIA#6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - NVIDIA#6584 / NVIDIA#6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - NVIDIA#6210 / NVIDIA#6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - NVIDIA#6624 / NVIDIA#6623 / NVIDIA#6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - NVIDIA#6629 / NVIDIA#6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - NVIDIA#6634 / NVIDIA#6626 / NVIDIA#6596 / NVIDIA#5569 / NVIDIA#6610 / NVIDIA#6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - NVIDIA#6578 / NVIDIA#5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@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 * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On networks where a corporate MITM proxy sits in front of the host and re-signs external TLS with its own root CA, that corporate root is absent from the sandbox trust path. OpenShell injects only its own L7-proxy CA, so external channel endpoints (e.g.
api.telegram.org) fail certificate verification even when the network policy allows the connection (logs showNET:OPENthenNET:FAIL). This imports an operator-supplied corporate CA into the sandbox trust bundle without replacing the OpenShell CA.Related Issue
Fixes #6210
Changes
src/lib/onboard/corporate-ca.ts: validate a host CA bundle (regular file, non-symlink, non-world-writable, bounded size, contains a PEM certificate) fromNEMOCLAW_CORPORATE_CA_BUNDLE(explicit, fail-loud) or the conventionalREQUESTS_CA_BUNDLE/CURL_CA_BUNDLE/SSL_CERT_FILEfallbacks (skip-on-invalid). Opt out withNEMOCLAW_CORPORATE_CA_IMPORT=0.NEMOCLAW_CORPORATE_CA_B64build arg decoded to a root-owned0444file (dockerfile-patch.ts+ both Dockerfiles).scripts/nemoclaw-start.sh,agents/hermes/start.sh), append the baked CA to the OpenShell bundle into a merged/tmpbundle — never replacing the OpenShell CA (preserves [All Platforms]Proxy CA certificate not injected into sandbox trust store — all TLS connections fail #1828) — and repointSSL_CERT_FILE,CURL_CA_BUNDLE,REQUESTS_CA_BUNDLE,GIT_SSL_CAINFO, andNODE_EXTRA_CA_CERTSat it, including forconnectsessions. No-corporate-CA path is byte-for-byte unchanged.docs/reference/troubleshooting.mdx.Type of Change
Quality Gates
0444file; OpenShell CA is appended, never replaced; no secrets involved.Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailable — all 28 prek pre-commit/pre-push hooks passed (shellcheck, hadolint, biome, source-shape budget, test-file-size budget, env-var docs, test-title-style); commit-msg clean on this commit.vitest runoncorporate-ca.test.ts,dockerfile-patch.test.ts,corporate-ca-runtime-merge.test.ts,corporate-ca-tls-e2e.test.ts→ 52 passed. The TLS test spins a server whose leaf is signed only by a simulated corporate CA and proves verification succeeds only after the real entrypoint merge while the OpenShell root stays trusted.npm run docsbuilds without warnings (doc changes only)E2E note
The reporter's DGX Station + real corporate MITM proxy is not available in CI. The trust-store behavior is reproduced hermetically without that hardware:
test/corporate-ca-tls-e2e.test.tsgenerates a corporate root CA, signs a leaf with it, runs the actualmerge_corporate_proxy_cablock extracted fromscripts/nemoclaw-start.sh, and asserts a Node TLS request verifies the corporate-signed endpoint only after the merge (pre-fix:unable to verify/UNABLE_TO_GET_ISSUER; post-fix:200), and that the OpenShell root remains trusted through the merged bundle.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit