fix(onboard): protect corporate CA bundle ownership - #8812
Conversation
📝 WalkthroughWalkthroughThe change updates corporate CA Dockerfile patching and onboarding wiring. OpenClaw CA builds select ChangesCorporate CA trust handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant DockerfilePatch
participant Dockerfile
participant TrustedEntrypoint
Onboarding->>DockerfilePatch: pass managed agent name
DockerfilePatch->>Dockerfile: validate startup contract
DockerfilePatch->>Dockerfile: select root for CA baking
Dockerfile->>TrustedEntrypoint: start trusted entrypoint
TrustedEntrypoint->>Dockerfile: create root-owned 0444 CA 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 |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit f8f06f0 in the TypeScript / code-coverage/cliThe overall coverage in commit f8f06f0 in the Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8812.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
Maintainer security review: no blocking findings. I reviewed the change across corporate CA input handling, Dockerfile argument validation, command and build-argument construction, filesystem ownership and symlink protections, root-to-sandbox privilege separation, secrets and logging, failure behavior, sibling-agent isolation, dependency impact, and documentation.
Local evidence: 21 targeted onboarding tests and 14 runtime trust tests pass. CLI build and type-check, documentation validation, secret scanning, repository checks, and all pull-request-scoped hooks also pass. The OpenClaw and Deep Agents managed-image jobs passed; the first Hermes job built and exercised startup successfully before a registry publication token was denied, so that non-code failure was safely rerun. |
|
CI follow-up:
All executable code, image, documentation, DCO, commit-verification, and security checks otherwise pass. The remaining requirements are a successful required advisor check and human approval. |
Signed-off-by: San Dang <sdang@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/onboard/dockerfile-patch.ts`:
- Around line 119-121: Update the regex in the runtimeUserArgs filter to use
case-insensitive matching so uppercase, lowercase, and mixed-case ARG
instructions are accepted. Add a regression test covering a non-uppercase ARG
instruction and verifying explicit corporate CA configuration succeeds.
🪄 Autofix
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: c32f5393-958b-4c3c-b99e-49e40a3705ea
📒 Files selected for processing (3)
docs/security/configure-corporate-ca-trust.mdxsrc/lib/onboard/dockerfile-patch-corporate-ca.test.tssrc/lib/onboard/dockerfile-patch.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/dockerfile-patch-corporate-ca.test.ts
- docs/security/configure-corporate-ca-trust.mdx
Signed-off-by: San Dang <sdang@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Blocking security finding on the latest PR commit a9e3eaf13.
openClawRootStartupArg proves only that the final USER ${NEMOCLAW_MANAGED_IMAGE_RUNTIME_USER} precedes some final ENTRYPOINT. It does not require that entrypoint to be the trusted /usr/local/bin/nemoclaw-start launcher. Corporate-CA patching then rewrites the runtime-user ARG to root.
A custom Dockerfile can therefore satisfy the current shape with a different entrypoint and run its agent command as root. Require the trusted OpenClaw entrypoint (or an equivalent verified step-down contract) before selecting root startup. Add a regression with a different final ENTRYPOINT and assert explicit CA baking rejects it without rewriting the runtime-user ARG.
The current commit-lint failure and pending managed-runtime check are separate hard gates.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard/dockerfile-patch.ts (1)
553-589: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winFail closed when
agentNameis omitted.The production flow passes
agentName, butPatchStagedDockerfileOptions.agentNameis optional. Require the identity or reject CA baking when it is absent, and add a regression test for this path.🤖 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.ts` around lines 553 - 589, Update the Dockerfile patch logic around openClawRootStartup and corporateCaArgPattern so corporate CA baking fails closed when options.agentName is absent, rather than treating the missing identity as a non-OpenClaw flow. Require a valid agent identity before applying the CA replacement, preserve the existing OpenClaw validation behavior, and add a regression test covering omitted agentName.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.
Outside diff comments:
In `@src/lib/onboard/dockerfile-patch.ts`:
- Around line 553-589: Update the Dockerfile patch logic around
openClawRootStartup and corporateCaArgPattern so corporate CA baking fails
closed when options.agentName is absent, rather than treating the missing
identity as a non-OpenClaw flow. Require a valid agent identity before applying
the CA replacement, preserve the existing OpenClaw validation behavior, and add
a regression test covering omitted agentName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e8a7ae8-662a-44c2-b844-4f5cebe0befe
📒 Files selected for processing (3)
docs/security/configure-corporate-ca-trust.mdxsrc/lib/onboard/dockerfile-patch-corporate-ca.test.tssrc/lib/onboard/dockerfile-patch.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/dockerfile-patch-corporate-ca.test.ts
- docs/security/configure-corporate-ca-trust.mdx
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-review of latest PR commit c588b765d: the previously requested root-startup fix is correctly implemented.
The final-stage contract now requires the exact exec-form ENTRYPOINT ["/usr/local/bin/nemoclaw-start"], preserves the exact case-sensitive build-argument identity while accepting case-insensitive Dockerfile instruction keywords, and rejects a custom entrypoint without rewriting the runtime-user ARG. The new negative test covers the bypass shape. I found no replacement blocker in this delta.
I am not approving yet: base 97ee9ce0b is behind current main, and the full exact-commit security, build, CodeQL, advisor, managed-startup, and CLI check set is still running. My earlier change request is substantively addressed; final approval must wait for those hard gates.
Addressed in c588b76: the trusted exec-form entrypoint is now required and the bypass regression is present. This dismissal is not approval; exact-commit hard gates remain pending.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security follow-up review: pass with no new blocker for validated revision The corporate-CA root-startup boundary now fails closed on all reviewed identity paths:
No new dependency, network, credential-storage, logging, path-traversal, or unsafe parsing surface was introduced. The CA bundle is a public certificate, the existing encoding and Docker argument sanitization remain in place, and the secret scan passed. Validation completed:
Fresh hosted checks and required reviewer approval remain merge gates; this review does not waive them. |
wscurran
added
area: install
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed the current head, including the fail-closed agent identity guard and corporate CA ownership/startup contract, and found no remaining security or correctness issue. I am withholding approval because this branch does not contain current main; refresh it and rerun required checks so approval can bind to the integrated exact head.
5 participants
Summary
OpenClaw images that bake a corporate CA now select root startup, so the entrypoint creates the merged runtime trust bundle as
root:rootwith mode0444before it starts sandbox-user commands.Explicit corporate CA configuration now fails when a custom Dockerfile cannot prove that the runtime-user argument controls the final image user and that NemoClaw's trusted entrypoint performs the later privilege drop.
Related Issue
Fixes #8803
Changes
0, group GID0, and mode0444on the merged bundle.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/security/configure-corporate-ca-trust.mdx; the follow-up review confirmed the omitted-agent guard is an internal fail-closed invariant and requires no additional documentation.Codex DesktopDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run test:changed -- --maxWorkers=50%1,180/1,180; semantic E2E collection 125 tests across 81 files;npm run docsexited 0 with 0 errors and 2 Fern warnings.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests