fix(onboard): report a dropped corporate CA import - #9410
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe onboarding Dockerfile patch now warns when fallback corporate CA baking is skipped. Tests cover missing CA build arguments and uncontrolled runtime-user declarations while confirming that Dockerfile state remains unchanged. ChangesCorporate CA fallback handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to This change improves diagnostics when a corporate CA is dropped, but it modifies onboarding and security-sensitive trust handling. The required sensitive-path review is still incomplete, so merge should wait for maintainer review or an approved waiver. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jyaunches
left a comment
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
The production change correctly reuses the existing warnCorporateCa owner, but the new tests recreate two fallback scenarios that this file already covers:
dockerfile-patch-corporate-ca.test.ts:303-314already exercises the missing runtime-user branch recreated at lines 363-383.dockerfile-patch-corporate-ca.test.ts:337-343already exercises the missing corporate-CA argument branch recreated at lines 345-361.
Both additions repeat environment setup, a Dockerfile fixture, the patch call, and the final-state assertion solely to inspect console.error. This adds roughly 40 test lines without adding a control-flow branch.
Refactor direction
Add the warning assertions to the two tests that already own these fallback branches. Preserve both the no-bake outcome and warning contract. Introduce a warning-capture helper only if it produces a net reduction across those existing cases.
Expected result
The same two fallback behaviors and diagnostics remain covered, while most of the duplicated test setup disappears and each production branch has one owning scenario.
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology 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. |
When a conventional CA variable or a host trust-store anchor resolved a corporate CA and the staged Dockerfile declared neither build argument that root-owned runtime trust needs, onboarding dropped the validated anchor as a silent no-op. The operator saw no diagnostic, so the sandbox started without the corporate trust anchor and external TLS through the corporate proxy failed later with nothing pointing back at onboarding. The explicit NEMOCLAW_CORPORATE_CA_BUNDLE source already fails closed. That branch now reports the source variable, the source path, and the reason through the existing corporate-CA warning helper. This does not close NVIDIA#8454. The reported run bakes the anchor into the shipped Dockerfile and never reaches this branch; the placement half of that report needs maintainer direction. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
dd8c426 to
ddd4338
Compare
|
Thanks — refactored as directed. The two new tests are gone. The warning assertions now live in the two tests that already own No warning-capture helper — factoring one out was line-neutral here, so it did not earn itself. Net lines vs |
2 participants
Summary
Onboarding resolved and validated a corporate CA from a conventional CA variable or a host
trust-store anchor, then dropped it with no diagnostic when the staged Dockerfile declared neither
build argument that root-owned runtime trust needs. Onboarding now reports the source variable,
the source path, and the reason.
Related Issue
Contributes to #8454. This PR does not close #8454.
The reported run does not reach the branch this PR changes. On the shipped
Dockerfilethat runbakes the anchor and onboarding prints a positive bake message. This PR corrects one adjacent
silent drop found while tracing the report. See "Root cause" for what the trace established and
what it did not.
Changes
patchStagedDockerfilewith an operator warning when aresolved corporate CA cannot be baked into the staged Dockerfile. The explicit
NEMOCLAW_CORPORATE_CA_BUNDLEsource keeps its existing fail-closed behavior.warnCorporateCa()helper. No new helper, configuration, or fallback path is added.Root cause
Established, and it explains
MERGED_CA_EXIT:1in the report:The documented runtime trust contract in
docs/security/configure-corporate-ca-trust.mdx—root-owned
/usr/local/share/nemoclaw/corporate-ca.pemplus a merged/run/nemoclaw/managed-startup-ca-bundle.pem— is implemented only bymergeCorporateCa()atsrc/lib/onboard/managed-startup/image-runtime.ts:1031-1059. An onboard that is nottempManagedRuntimeand is not a rebuild getsmanagedImages: nullatsrc/lib/onboard/managed-workload/onboard-orchestration.ts:141-148, which makesmanagedImageRuntimeSupportError()return a reason atsrc/lib/onboard/workload/source.ts:131-133,which makes
src/lib/onboard/workload/preparation.ts:234-253return a legacy-dockerfile workloadwith
fallbackDiagnostic: null. No managed startup profile is built, somergeCorporateCa()neverruns. The legacy path writes its merged bundle to
/tmp/nemoclaw-ca-bundle.pem(
scripts/lib/corporate-ca-runtime.sh:23).scripts/nemoclaw-start.shhas never written/run/nemoclaw/managed-startup-ca-bundle.pem, so theshared-helper extraction in #9244 preserved behavior and is not the reported regression.
Not established, and this is why the PR does not close the issue:
CORPORATE_CA_EXIT:1does not follow from the checked-in source. Patching the shippedDockerfilewith the reporter's variable and agent bakes the anchor, sets
ARG NEMOCLAW_MANAGED_IMAGE_RUNTIME_USER=root, and prints the bake message.Dockerfile:659-665writes the anchor as
root:root 0444in the single final stage. Distinguishing a skipped or cachedimage rebuild needs the reporter's build log, which I requested on the issue.
QA escape
Two detection gaps, at different sizes.
The gap this PR closes: the two branches that dropped a resolved corporate CA already had tests in
src/lib/onboard/dockerfile-patch-corporate-ca.test.ts—does not bake a fallback corporate CA without root startup selection (#8803)andstays a no-op for a fallback CA when a custom Dockerfile lacks the ARG. Both assert the correctoutcome, that the build argument stays empty and that onboarding does not throw. Neither asserts
that NemoClaw tells the operator, so a validated CA could be dropped in silence. Both tests keep
their assertions unchanged and still pass. The two tests added here assert the diagnostic.
The larger gap, which this PR does not close, has two parts.
Onboarding prints
baking corporate proxy CA ... into the sandbox image trustwhen it patches thestaged Dockerfile, before any image build, and nothing later verifies that the built image carries
the anchor. A skipped or cached rebuild therefore produces a positive bake message, exit code 0,
and a Ready phase with no anchor in the sandbox.
End-to-end coverage asserts the documented runtime contract only for a managed image.
scripts/checks/run-managed-image-direct-e2e.ts:638-658andscripts/checks/run-managed-image-openshell-e2e.ts:468-472require/run/nemoclaw/managed-startup-ca-bundle.pemat0:0:444. The legacy path that an ordinaryonboard takes has no equivalent assertion, so the difference the reporter measured was never
observed by a test.
Closing this gap needs maintainer direction on where trust state belongs in readiness output, so I
have not built it here.
What this PR does not change
The legacy direct-start path still writes
/tmp/nemoclaw-ca-bundle.pemrather than/run/nemoclaw/managed-startup-ca-bundle.pem. Making the legacy image publish the managed runtimebundle adds a root-owned trust anchor to an image that
docs/security/configure-corporate-ca-trust.mdxdescribes as read-only by mode and not a privilegeboundary. Under the Product Scope Gate in
CLAUDE.md, that needs an accepted issue or designdecision defining ownership, lifecycle, compatibility, security, and validation. I requested
maintainer direction on the issue rather than establishing it in a bug fix.
The second comment on the issue reports an
inference.localHTTP 503 on an OpenAI-compatibleroute. The reporter states that gateway-side TLS diagnostics are still needed to confirm the cause,
so this PR does not claim to address it.
Type of Change
Quality Gates
runner, sandbox, or messaging)
reviewer/approval link/justification: not obtained. This change touches onboarding and
security paths and needs maintainer review before merge.
Verification
Signed-off-by:line. The commit is signed with an SSH signingkey; confirm the
Verifiedbadge on GitHub after the branch is pushed.npm run validate:prpassed. Everypre-commit,commit-msg, andpre-pushhook reportedPassed, includingCodebase growth guardrails,TypeScript (CLI), andTypeScript (plugin).npx vitest run --project cli src/lib/onboard/dockerfile-patch-corporate-ca.test.ts→ 1 file passed, 16 tests passed.
Additional evidence:
src/lib/onboard/dockerfile-patch.tsto the parent commit gives 2 failed and 14 passed.npm run typecheck:cli→ 0 errors, on this commit and on the parent commit. An unbuiltdist/reports 73 resolution errors on both commits with identical text; building
dist/clears all73. No error references either changed file in either state.
npx oxlint src/lib/onboard/dockerfile-patch.ts src/lib/onboard/dockerfile-patch-corporate-ca.test.ts→ exit code 0, no findings.
npx commitlint --from HEAD~1 --to HEAD→ exit code 0.npm testandnpm run test:changedwere not run.Signed-off-by: Udaya Tejas udayatejas2004@gmail.com
Summary by CodeRabbit