fix(sandbox): export GIT_SSL_CAINFO so git trusts proxy CA#2345
fix(sandbox): export GIT_SSL_CAINFO so git trusts proxy CA#2345
Conversation
OpenShell's L7 proxy does MITM TLS termination and re-signs with its own CA. The proxy injects SSL_CERT_FILE pointing at the CA bundle, but git does not read that variable — it needs GIT_SSL_CAINFO. Without it, `git clone` inside the sandbox fails with "server certificate verification failed". - Detect SSL_CERT_FILE in nemoclaw-start.sh and export GIT_SSL_CAINFO - Persist GIT_SSL_CAINFO into proxy-env.sh for connect sessions - Add GIT_SSL_CAINFO, GIT_SSL_CAPATH, CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE to the TLS allowlist in subprocess-env.ts (CLI + plugin) - Add tests for the new allowlist entries and entrypoint behaviour Closes #2270 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEntrypoint and proxy env scripts now export/persist Changes
Sequence Diagram(s)sequenceDiagram
participant Entrypoint as Entrypoint Script
participant FS as Filesystem (/tmp/nemoclaw-proxy-env.sh)
participant Sandbox as Sandbox Session
participant Subprocess as Spawned Subprocess
Entrypoint->>Entrypoint: read SSL_CERT_FILE
alt SSL_CERT_FILE exists
Entrypoint->>Entrypoint: export GIT_SSL_CAINFO=SSL_CERT_FILE
end
Entrypoint->>FS: emit proxy env script (includes export if set)
Sandbox->>FS: source proxy env script on connect
Sandbox->>Subprocess: spawn process (inherits forwarded env vars)
Subprocess->>Subprocess: git/curl/requests honor CA env vars
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
# Conflicts: # scripts/nemoclaw-start.sh
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/credential-exposure.test.ts (1)
121-124: Consider making TLS extraction less formatting-sensitive.Current comparison depends on raw source slicing. Extracting quoted entries before compare would reduce test brittleness.
Possible tweak
- const extractTLS = (src) => { - const match = src.match(/const TLS = \[([\s\S]*?)\];/); - return match ? match[1].replace(/\s/g, "") : ""; - }; + const extractTLS = (src) => { + const match = src.match(/const TLS = \[([\s\S]*?)\];/); + if (!match) return ""; + const entries = match[1].match(/"[^"]+"/g) ?? []; + return entries.join(","); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credential-exposure.test.ts` around lines 121 - 124, The extractTLS helper is brittle because it returns the raw slice of the TLS array and strips whitespace, making tests sensitive to formatting; change extractTLS to parse the array entries and return the list of quoted string entries (e.g., by finding all single/double-quoted tokens inside the matched array using a global regex and returning them joined or as an array) so comparisons operate on the actual values rather than source formatting; update places that call extractTLS to compare normalized entries (order/stable join) instead of raw string slices.scripts/nemoclaw-start.sh (1)
866-869: Escape persistedGIT_SSL_CAINFObefore writing sourced shell content.This value is written into a file that is later
sourced. Using direct interpolation can allow unintended re-expansion of special characters.Proposed hardening
- if [ -n "${GIT_SSL_CAINFO:-}" ]; then - echo "export GIT_SSL_CAINFO=\"$GIT_SSL_CAINFO\"" - fi + if [ -n "${GIT_SSL_CAINFO:-}" ]; then + printf 'export GIT_SSL_CAINFO=%q\n' "$GIT_SSL_CAINFO" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 866 - 869, The script currently echoes GIT_SSL_CAINFO directly into a file that will later be sourced, which risks unintended shell re-expansion of special characters; modify the block that checks if [ -n "${GIT_SSL_CAINFO:-}" ] (the if ... echo ... fi sequence) to write an escaped/quoted form of the variable instead of raw interpolation (e.g., use a shell-safe quoting/escaping approach such as printf with %q or properly single-quote-escape the value) so the persisted export line is safe to source and cannot be re-expanded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 866-869: The script currently echoes GIT_SSL_CAINFO directly into
a file that will later be sourced, which risks unintended shell re-expansion of
special characters; modify the block that checks if [ -n "${GIT_SSL_CAINFO:-}" ]
(the if ... echo ... fi sequence) to write an escaped/quoted form of the
variable instead of raw interpolation (e.g., use a shell-safe quoting/escaping
approach such as printf with %q or properly single-quote-escape the value) so
the persisted export line is safe to source and cannot be re-expanded.
In `@test/credential-exposure.test.ts`:
- Around line 121-124: The extractTLS helper is brittle because it returns the
raw slice of the TLS array and strips whitespace, making tests sensitive to
formatting; change extractTLS to parse the array entries and return the list of
quoted string entries (e.g., by finding all single/double-quoted tokens inside
the matched array using a global regex and returning them joined or as an array)
so comparisons operate on the actual values rather than source formatting;
update places that call extractTLS to compare normalized entries (order/stable
join) instead of raw string slices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad75f5f7-2dff-4f46-ad73-c82ca59c1e24
📒 Files selected for processing (5)
nemoclaw/src/lib/subprocess-env.tsscripts/nemoclaw-start.shsrc/lib/subprocess-env.tstest/credential-exposure.test.tstest/service-env.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 973-976: The line echo "export GIT_SSL_CAINFO=\"$GIT_SSL_CAINFO\""
writes an unescaped value into the generated proxy-env.sh and can allow shell
substitution; instead, escape the variable before emitting it (e.g., replace
each single quote in $GIT_SSL_CAINFO with '\'' and then wrap the result in
single quotes, or use a portable printf %q where available) and write the safe
quoted value into the export line so the emitted "export GIT_SSL_CAINFO='...'"
cannot be interpreted as code when proxy-env.sh is sourced.
🪄 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: Pro Plus
Run ID: 1c577252-eb8b-4607-95ad-efaf8b63dc01
📒 Files selected for processing (2)
scripts/nemoclaw-start.shtest/service-env.test.ts
- Use printf %q to escape GIT_SSL_CAINFO in proxy-env.sh to prevent shell re-expansion when sourced - Make extractTLS test helper less formatting-sensitive by matching quoted entries instead of raw source slicing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Nemotron fix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/credential-exposure.test.ts`:
- Around line 120-125: The function extractTLS is missing TypeScript
annotations; update its signature to explicitly type the parameter and return
value (e.g., annotate src as string and the function return as string) so the
declaration becomes something like const extractTLS = (src: string): string => {
... }, keeping the existing implementation unchanged and ensuring
entries.join(",") still returns a string.
🪄 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: 135c4da2-06ce-4518-9d0b-cbfe0f235c9a
📒 Files selected for processing (3)
scripts/nemoclaw-start.shtest/credential-exposure.test.tstest/service-env.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/service-env.test.ts
## Summary Refreshes user-facing docs for the last 24 hours of merged NemoClaw history and bumps the docs metadata to 0.0.29, the next version after v0.0.28. The updates are limited to behavior supported by merged PR descriptions and diffs. ## Changes - `docs/reference/commands.md`: documented `nemoclaw <name> policy-add --from-file` and `--from-dir`, including custom preset review guidance, from #2077 / commit `7720b175`. - `docs/deployment/deploy-to-remote-gpu.md`: clarified that non-loopback `CHAT_UI_URL` disables OpenClaw device pairing for remote browser-only deployments, from #2449 / commit `f5ee8a4d`. - `docs/inference/inference-options.md`: documented provider-aware credential retry validation and the NVIDIA-only `nvapi-` prefix check, from #2389 / commit `6f7f0c6d`. - `docs/inference/switch-inference-providers.md`: documented `NEMOCLAW_INFERENCE_INPUTS` for text/image-capable model metadata baked into `openclaw.json`, from #2441 / commit `f4391892`. - `docs/reference/troubleshooting.md`: added the Git certificate verification entry for proxy CA propagation through `GIT_SSL_CAINFO`, `GIT_SSL_CAPATH`, `CURL_CA_BUNDLE`, and `REQUESTS_CA_BUNDLE`, from #2345 / commit `fa0dc1ab`. - `docs/versions1.json` and `docs/project.json`: promoted docs version `0.0.29`; `docs/versions1.json` omits unpublished `0.0.26`, `0.0.27`, and `0.0.28` entries. - `.agents/skills/nemoclaw-user-*`: regenerated derived user skill references from the updated docs. - Reviewed with no extra doc changes: #2575 / `d392ec07`, #2565 / `a3231049`, #1965 / `db1ef3ca`, #1990 / `db665834`, #2495 / `7da86fa3`, #2496 / `3192f4f4`, #2490 / `8c209058`, #2487 / `1f615e2f`, #2483 / `5653d33a`, #2482 / `31c782c0`, #2464 / `23bb5703`, #2472 / `a54f9a34`, and #2437 / `6bc860d7`. - Skipped per docs policy: #2420 / `7b76df6b` touched the experimental sandbox config path listed in `docs/.docs-skip`; #2466 / `cc15689c` touched a skipped term and CI-only sandbox image files. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. --> - [x] `npx prek run --all-files` passes - [ ] `npm test` passes — failed locally in installer-integration tests and one onboard helper timeout; the doc-scoped hook test projects passed under `prek`. - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) — build succeeded, but local Sphinx emitted the existing version-switcher file read message. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## AI Disclosure <!-- If an AI agent authored or co-authored this PR, check the box and name the tool. Remove this section for fully human-authored PRs. --> - [x] AI-assisted — tool: Codex --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Support for custom YAML presets in policy configuration via --from-file and --from-dir. * New build-time inference input option to declare accepted modalities (text or text,image). * **Improvements** * Credential validation now offers interactive recovery: re-enter key, retry, choose another provider, or exit. * Clarified provider-specific API key prefix handling (nvapi- only applies to NVIDIA keys). * **Documentation** * TLS certificate troubleshooting for inspected networks. * Clarified remote dashboard security/device-pairing behavior; command docs updated; docs version bumped. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
Summary
SSL_CERT_FILEinnemoclaw-start.shand exportGIT_SSL_CAINFOso git trusts the L7 proxy CAGIT_SSL_CAINFOintoproxy-env.shfor connect sessionsGIT_SSL_CAINFO,GIT_SSL_CAPATH,CURL_CA_BUNDLE,REQUESTS_CA_BUNDLEto the TLS allowlist insubprocess-env.ts(CLI + plugin)Closes #2270
Test plan
npm test— credential-exposure and service-env tests passgit clonesucceedsproxy-env.shincludesGIT_SSL_CAINFOin connect sessions🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests