fix(security): pin sandbox image by digest in blueprint.yaml (#1438) - #1655
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughTop-level Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
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 `@nemoclaw-blueprint/blueprint.yaml`:
- Around line 21-26: The top-level blueprint `digest:` field is still empty and
needs to be populated to enforce blueprint-level integrity; update the YAML by
setting the top-level `digest:` value to the appropriate release digest (the
same release tooling that updates the pinned `image:` digest should set this),
ensuring the `digest` field at the top of blueprint.yaml is filled with the
expected SHA256 string so integrity detection is enabled alongside the pinned
`image:` entry.
🪄 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
Run ID: 3d84cc6c-e5d3-41bf-8333-6cb759619680
📒 Files selected for processing (2)
nemoclaw-blueprint/blueprint.yamltest/validate-blueprint.test.ts
…nifest CodeRabbit on NVIDIA#1655 flagged that the top-level 'digest:' field at the top of blueprint.yaml was still empty after the image-pinning change, leaving the issue (NVIDIA#1438) only partially addressed: image pinning was fixed but the documented blueprint-level integrity field was still 'Computed at release time' with no actual value. Mirror the components.sandbox.image manifest digest into the top-level field so: - A downstream consumer (or release tooling) can read a single field to know what sandbox image this blueprint pins to, without having to parse the components tree. - A future contributor who bumps the pinned image but forgets the top-level field is caught at CI time by the new test below — the trivially-bypassable 'image moved but digest stayed stale' scenario is now blocked. - The release tooling that bumps the pinned image only has to also rewrite the top-level field (one matching value, not two unrelated ones). The comment block on the digest field calls this out. Adds a second regression test in test/validate-blueprint.test.ts that asserts both: 1. The top-level blueprint.digest is a non-empty 'sha256:<64-hex>' string. 2. The top-level digest exactly matches the @sha256: portion of the pinned components.sandbox.image string. Both regression tests verified by stashing the YAML fix and re-running: the new tests fail on an unfixed blueprint and pass with the fix in place. Addresses CodeRabbit major flag on NVIDIA#1655. Still part of NVIDIA#1438.
|
✨ Thanks for submitting this PR, which proposes a way to improve security by pinning the sandbox image and may prevent unauthorized changes to the sandbox environment. Possibly related open issues: |
|
I approved the blocked fork workflows so CI could start. Current blocker:
Failed check: https://github.com/NVIDIA/NemoClaw/actions/runs/24171304666 Once you add the sign-off to the PR body, the DCO check can be rerun. The remaining |
…nifest CodeRabbit on NVIDIA#1655 flagged that the top-level 'digest:' field at the top of blueprint.yaml was still empty after the image-pinning change, leaving the issue (NVIDIA#1438) only partially addressed: image pinning was fixed but the documented blueprint-level integrity field was still 'Computed at release time' with no actual value. Mirror the components.sandbox.image manifest digest into the top-level field so: - A downstream consumer (or release tooling) can read a single field to know what sandbox image this blueprint pins to, without having to parse the components tree. - A future contributor who bumps the pinned image but forgets the top-level field is caught at CI time by the new test below — the trivially-bypassable 'image moved but digest stayed stale' scenario is now blocked. - The release tooling that bumps the pinned image only has to also rewrite the top-level field (one matching value, not two unrelated ones). The comment block on the digest field calls this out. Adds a second regression test in test/validate-blueprint.test.ts that asserts both: 1. The top-level blueprint.digest is a non-empty 'sha256:<64-hex>' string. 2. The top-level digest exactly matches the @sha256: portion of the pinned components.sandbox.image string. Both regression tests verified by stashing the YAML fix and re-running: the new tests fail on an unfixed blueprint and pass with the fix in place. Addresses CodeRabbit major flag on NVIDIA#1655. Still part of NVIDIA#1438. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
359ca9d to
f627e9a
Compare
|
Added DCO sign-off to both commits. Should unblock the check. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw-blueprint/blueprint.yaml (1)
29-31: Consider automating digest refresh in release tooling.Lines 29-31 call out manual bumping; adding a release step that resolves the manifest digest and updates both
digestfields atomically would prevent future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/blueprint.yaml` around lines 29 - 31, Add a release-tooling step that programmatically resolves the image manifest digest for the :latest tag and atomically updates both occurrences of the digest in blueprint.yaml (the top-level `digest:` and the inline comment's resolved sha256) to prevent drift; implement this as part of your release pipeline (e.g., a script or CI job) that fetches the registry manifest, extracts the sha256 digest, and replaces both `digest` fields in the file in one commit/transaction so they remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw-blueprint/blueprint.yaml`:
- Around line 29-31: Add a release-tooling step that programmatically resolves
the image manifest digest for the :latest tag and atomically updates both
occurrences of the digest in blueprint.yaml (the top-level `digest:` and the
inline comment's resolved sha256) to prevent drift; implement this as part of
your release pipeline (e.g., a script or CI job) that fetches the registry
manifest, extracts the sha256 digest, and replaces both `digest` fields in the
file in one commit/transaction so they remain in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e90e29e-c09d-41d3-9a98-6ee40eb990e1
📒 Files selected for processing (2)
nemoclaw-blueprint/blueprint.yamltest/validate-blueprint.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/validate-blueprint.test.ts
The blueprint referenced the sandbox image by the mutable :latest tag:
image: "ghcr.io/nvidia/openshell-community/sandboxes/openclaw:latest"
A registry compromise or accidental force-push to :latest would silently
swap the sandbox image without any blueprint-side change, defeating the
top-level 'digest:' field's stated purpose. The reported issue (NVIDIA#1438)
identifies this as a supply-chain risk.
Switch to a digest-pinned reference:
image: "ghcr.io/nvidia/openshell-community/sandboxes/openclaw@sha256:..."
The digest below was the resolved sha256 of the :latest tag at the time
of this commit, fetched directly from ghcr.io. Release tooling should
bump this digest together with the top-level 'digest:' field on every
release.
Adds a regression test in test/validate-blueprint.test.ts that walks the
parsed blueprint, finds components.sandbox.image, and asserts:
- the image string contains '@sha256:' (digest pin present)
- the image string does NOT match a ':latest' suffix
- the digest is a 64-hex-char sha256
Verified: the new test fails on an unfixed blueprint and passes with
the digest pin in place.
Closes NVIDIA#1438
Signed-off-by: ColinM-sys <cmcdonough@50words.com>
…nifest CodeRabbit on NVIDIA#1655 flagged that the top-level 'digest:' field at the top of blueprint.yaml was still empty after the image-pinning change, leaving the issue (NVIDIA#1438) only partially addressed: image pinning was fixed but the documented blueprint-level integrity field was still 'Computed at release time' with no actual value. Mirror the components.sandbox.image manifest digest into the top-level field so: - A downstream consumer (or release tooling) can read a single field to know what sandbox image this blueprint pins to, without having to parse the components tree. - A future contributor who bumps the pinned image but forgets the top-level field is caught at CI time by the new test below — the trivially-bypassable 'image moved but digest stayed stale' scenario is now blocked. - The release tooling that bumps the pinned image only has to also rewrite the top-level field (one matching value, not two unrelated ones). The comment block on the digest field calls this out. Adds a second regression test in test/validate-blueprint.test.ts that asserts both: 1. The top-level blueprint.digest is a non-empty 'sha256:<64-hex>' string. 2. The top-level digest exactly matches the @sha256: portion of the pinned components.sandbox.image string. Both regression tests verified by stashing the YAML fix and re-running: the new tests fail on an unfixed blueprint and pass with the fix in place. Addresses CodeRabbit major flag on NVIDIA#1655. Still part of NVIDIA#1438. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
|
I merged the latest
That passed. CI is rerunning on the updated branch now. |
56537ea to
b6b47db
Compare
…1438) (NVIDIA#1655) ## Summary Closes NVIDIA#1438. `nemoclaw-blueprint/blueprint.yaml` referenced the sandbox image by the mutable `:latest` tag: ```yaml image: "ghcr.io/nvidia/openshell-community/sandboxes/openclaw:latest" ``` A registry compromise or accidental force-push to `:latest` would silently swap the sandbox image without any blueprint-side change. The top-level `digest:` field in the same file is documented as *"Computed at release time"* but is empty in main, so there's no integrity backstop today. ## Fix Pin the image by digest: ```yaml image: "ghcr.io/nvidia/openshell-community/sandboxes/openclaw@sha256:b3d832b596ab6b7184a9dcb4ae93337ca32851a4f93b00765cc12de26baa3a9a" ``` The digest above was the resolved sha256 of `:latest` at the time of this commit, fetched directly from `ghcr.io/v2/nvidia/openshell-community/sandboxes/openclaw/manifests/latest` via the standard OCI registry API. A new regression test in `test/validate-blueprint.test.ts` walks the parsed blueprint, finds `components.sandbox.image`, and asserts: - the image string contains `@sha256:` (digest pin present) - the image string does NOT match a `:latest` suffix - the digest is a 64-hex-char sha256 ## Test plan - [x] `vitest run test/validate-blueprint.test.ts` — 28/28 pass - [x] **Negative case verified**: stashed the YAML fix, re-ran the test against the unfixed `:latest` blueprint. The new `regression NVIDIA#1438` test correctly **fails** with a `digest pin missing` assertion error, proving the test catches the regression. - [x] **Positive case verified**: re-applied the YAML fix, re-ran. All 28 tests pass. ## Honest scope and follow-up needed This PR is a "stop the bleeding now" fix. It closes the immediate supply-chain hole but creates a small maintenance burden: the static digest needs to be bumped whenever a new sandbox image is published. **Recommended follow-up (out of scope here):** add release tooling (CI workflow or `scripts/update-blueprint-digest.sh`) that resolves the current `:latest` digest from `ghcr.io` and rewrites both the `image:` line and the top-level `digest:` field in `blueprint.yaml` on every release. The infrastructure for this doesn't exist today (the top-level `digest:` field's "Computed at release time" comment describes intent that was never implemented). Until that tooling lands, contributors bumping the sandbox image will need to update this digest by hand. The new regression test makes that hand-update obvious — anyone who lets it lapse back to `:latest` or any other mutable tag fails CI. ## Why digest pinning over version tags The `ghcr.io/nvidia/openshell-community/sandboxes/openclaw` registry currently exposes only commit-SHA tags (e.g. `21aa171`, `436b8c9`) and `latest`. There is no semver tag like `v0.0.10` to pin to. A digest is the only integrity-preserving reference available without changing how sandbox images are published. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Switched the sandbox container reference to an immutable image digest and aligned the blueprint's top-level digest for reproducible deployments; added inline notes to keep them synchronized. * **Tests** * Added regression tests to enforce immutable sandbox image references and to ensure the blueprint-level digest matches the sandbox image digest. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: ColinM-sys <cmcdonough@50words.com> --------- Signed-off-by: ColinM-sys <cmcdonough@50words.com>
…1438) (NVIDIA#1655) ## Summary Closes NVIDIA#1438. `nemoclaw-blueprint/blueprint.yaml` referenced the sandbox image by the mutable `:latest` tag: ```yaml image: "ghcr.io/nvidia/openshell-community/sandboxes/openclaw:latest" ``` A registry compromise or accidental force-push to `:latest` would silently swap the sandbox image without any blueprint-side change. The top-level `digest:` field in the same file is documented as *"Computed at release time"* but is empty in main, so there's no integrity backstop today. ## Fix Pin the image by digest: ```yaml image: "ghcr.io/nvidia/openshell-community/sandboxes/openclaw@sha256:b3d832b596ab6b7184a9dcb4ae93337ca32851a4f93b00765cc12de26baa3a9a" ``` The digest above was the resolved sha256 of `:latest` at the time of this commit, fetched directly from `ghcr.io/v2/nvidia/openshell-community/sandboxes/openclaw/manifests/latest` via the standard OCI registry API. A new regression test in `test/validate-blueprint.test.ts` walks the parsed blueprint, finds `components.sandbox.image`, and asserts: - the image string contains `@sha256:` (digest pin present) - the image string does NOT match a `:latest` suffix - the digest is a 64-hex-char sha256 ## Test plan - [x] `vitest run test/validate-blueprint.test.ts` — 28/28 pass - [x] **Negative case verified**: stashed the YAML fix, re-ran the test against the unfixed `:latest` blueprint. The new `regression NVIDIA#1438` test correctly **fails** with a `digest pin missing` assertion error, proving the test catches the regression. - [x] **Positive case verified**: re-applied the YAML fix, re-ran. All 28 tests pass. ## Honest scope and follow-up needed This PR is a "stop the bleeding now" fix. It closes the immediate supply-chain hole but creates a small maintenance burden: the static digest needs to be bumped whenever a new sandbox image is published. **Recommended follow-up (out of scope here):** add release tooling (CI workflow or `scripts/update-blueprint-digest.sh`) that resolves the current `:latest` digest from `ghcr.io` and rewrites both the `image:` line and the top-level `digest:` field in `blueprint.yaml` on every release. The infrastructure for this doesn't exist today (the top-level `digest:` field's "Computed at release time" comment describes intent that was never implemented). Until that tooling lands, contributors bumping the sandbox image will need to update this digest by hand. The new regression test makes that hand-update obvious — anyone who lets it lapse back to `:latest` or any other mutable tag fails CI. ## Why digest pinning over version tags The `ghcr.io/nvidia/openshell-community/sandboxes/openclaw` registry currently exposes only commit-SHA tags (e.g. `21aa171`, `436b8c9`) and `latest`. There is no semver tag like `v0.0.10` to pin to. A digest is the only integrity-preserving reference available without changing how sandbox images are published. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Switched the sandbox container reference to an immutable image digest and aligned the blueprint's top-level digest for reproducible deployments; added inline notes to keep them synchronized. * **Tests** * Added regression tests to enforce immutable sandbox image references and to ensure the blueprint-level digest matches the sandbox image digest. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: ColinM-sys <cmcdonough@50words.com> --------- Signed-off-by: ColinM-sys <cmcdonough@50words.com>
Summary
Closes #1438.
nemoclaw-blueprint/blueprint.yamlreferenced the sandbox image by the mutable:latesttag:A registry compromise or accidental force-push to
:latestwould silently swap the sandbox image without any blueprint-side change. The top-leveldigest:field in the same file is documented as "Computed at release time" but is empty in main, so there's no integrity backstop today.Fix
Pin the image by digest:
The digest above was the resolved sha256 of
:latestat the time of this commit, fetched directly fromghcr.io/v2/nvidia/openshell-community/sandboxes/openclaw/manifests/latestvia the standard OCI registry API.A new regression test in
test/validate-blueprint.test.tswalks the parsed blueprint, findscomponents.sandbox.image, and asserts:@sha256:(digest pin present):latestsuffixTest plan
vitest run test/validate-blueprint.test.ts— 28/28 pass:latestblueprint. The newregression #1438test correctly fails with adigest pin missingassertion error, proving the test catches the regression.Honest scope and follow-up needed
This PR is a "stop the bleeding now" fix. It closes the immediate supply-chain hole but creates a small maintenance burden: the static digest needs to be bumped whenever a new sandbox image is published.
Recommended follow-up (out of scope here): add release tooling (CI workflow or
scripts/update-blueprint-digest.sh) that resolves the current:latestdigest fromghcr.ioand rewrites both theimage:line and the top-leveldigest:field inblueprint.yamlon every release. The infrastructure for this doesn't exist today (the top-leveldigest:field's "Computed at release time" comment describes intent that was never implemented).Until that tooling lands, contributors bumping the sandbox image will need to update this digest by hand. The new regression test makes that hand-update obvious — anyone who lets it lapse back to
:latestor any other mutable tag fails CI.Why digest pinning over version tags
The
ghcr.io/nvidia/openshell-community/sandboxes/openclawregistry currently exposes only commit-SHA tags (e.g.21aa171,436b8c9) andlatest. There is no semver tag likev0.0.10to pin to. A digest is the only integrity-preserving reference available without changing how sandbox images are published.Summary by CodeRabbit
Chores
Tests
Signed-off-by: ColinM-sys cmcdonough@50words.com