fix(evidence): relocate idempotency, strict digest, package docs by njhensley · Pull Request #1543 · NVIDIA/aicr

njhensley · 2026-06-29T21:07:08Z

Summary

Non-blocking follow-ups to the merged evidence relocate flow (#1538 / #1530): close the relocation idempotency gap on a committed-both retry, tighten the bundle-digest guard to a canonical sha256:<64-hex>, and document the fork package-write prerequisites for the CI signing leg.

Motivation / Context

These were raised as non-blocking review items on #1538 and tracked for a quick follow-up.

Fixes: N/A
Related: #1538, #1530, #1535

Type of Change

Bug fix (non-breaking change that fixes an issue)
Documentation update

Component(s) Affected

CLI (cmd/aicr, pkg/cli)
Core libraries (pkg/errors, pkg/k8s) — pkg/evidence/attestation
Docs/examples (docs/, examples/)

Implementation Notes

Relocation content-equality recovery (pkg/evidence/attestation/pointer.go): the os.Link EEXIST recovery recognized only same-inode dests (os.SameFile), so after a git round trip — where the flat source and the committed nested copy are distinct inodes but byte-equal — a retry hit a spurious ErrCodeConflict. Added a bounded-read + bytes.Equal fallback (pointerAlreadyPlaced); only a dest holding different bytes is a real immutable-pointer conflict.
Strict digest (isHexDigest): now requires exactly sha256: + 64 lowercase hex, rejecting sha256:nothex, truncated hashes, and wrong-algorithm prefixes instead of any hex-ish string. Test fixtures updated to the real 64-char h100 digest.
Fork package-write docs (docs/contributor/evidence-publishing.md): documented the two GHCR write prerequisites the sign leg needs — the package's Actions access must include the fork with the Write role, and a locally-pushed package may be linked to NVIDIA/aicr rather than the fork and must be re-linked.
Not in scope: --relocate trusts the on-disk signer block without cryptographic bundle verification — the same metadata-only trust the contract gate applies (not a new escalation), tracked repo-wide in Evidence pointer-contract gate trusts pointer-supplied signer (no crypto verification) #1535.

Testing

make test    # -race, evidence + cli packages green
make lint    # golangci-lint 0 issues; gofmt clean; MDX OK

New/updated tests: relocation recovery on byte-identical dest (distinct inode), conflict on different-content dest, same-inode partial-move recovery, and digest-guard cases for non-hex / too-short / too-long / wrong-prefix.

Risk Assessment

Low — Isolated to the evidence relocate path + docs; idempotency change only widens the already-placed recognition (a stricter conflict definition), and the digest guard only tightens validation.

Rollout notes: No migration. N/A.

Checklist

Tests pass locally (make test with -race)
Linter passes (make lint)
I did not skip/disable tests to make CI green
I added/updated tests for new functionality
I updated docs if user-facing behavior changed
Changes follow existing patterns in the codebase
Commits are cryptographically signed (git commit -S)

…kage-write docs Non-blocking follow-ups to NVIDIA#1538 (NVIDIA#1530): - attestation: relocation idempotency now closes the committed-both case. The EEXIST recovery recognized only same-inode dests (os.SameFile), so after a git round trip — where the flat source and the committed nested copy are distinct inodes but byte-equal — a retry hit a spurious conflict. Add a content-equality fallback (bounded read + bytes.Equal); only a dest holding DIFFERENT bytes is a real immutable-pointer conflict. - attestation: isHexDigest now requires exactly sha256:<64 lowercase hex>, so a malformed digest (sha256:nothex, truncated, wrong algo) is rejected rather than any hex-ish string. Test fixtures updated to full-length digests. - docs: document the fork package-WRITE prerequisites for the CI sign leg — the package's Actions access must include the fork with the Write role, and a locally-pushed package may be linked to NVIDIA/aicr rather than the fork. Not changed here: --relocate trusts the on-disk signer without cryptographic bundle verification — the same metadata-only trust the contract gate applies (not a new escalation), tracked repo-wide in NVIDIA#1535. Signed-off-by: Nathan Hensley <[email protected]>

github-actions · 2026-06-29T21:09:37Z

🌿 Preview your docs: https://nvidia-preview-evidence-relocate-followups.docs.buildwithfern.com/aicr

github-actions · 2026-06-29T21:10:02Z

Recipe evidence check

No leaf overlays affected by this PR.

This gate is warning-only and never blocks merge.

coderabbitai · 2026-06-29T21:11:37Z

📝 Walkthrough

Walkthrough

The changes tighten evidence pointer handling by requiring full canonical sha256: digests, changing canonical relocation to treat byte-identical existing destinations as already placed, and bounding pointer reads during equality checks. The related tests and fixtures were updated to use full 64-hex digest filenames and to cover the new conflict and recovery behavior. The PR also adds contributor documentation for the GHCR Actions permissions needed by CI signing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

NVIDIA/aicr#1538: Directly changes the same pointer relocation and digest-validation path in pkg/evidence/attestation/pointer.go.

Suggested labels

theme/validation

Suggested reviewers

yuanchen8911

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main evidence relocate, digest validation, and docs changes.
Description check	✅ Passed	The description matches the PR scope and explains the relocation, digest, and GHCR permission updates.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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 `@pkg/evidence/attestation/pointer_test.go`:
- Around line 207-209: The RelocatePointerToCanonical conflict test currently
only checks for any error, so it could miss a regression that returns the wrong
error type. Update the assertion in the RelocatePointerToCanonical test to
verify the structured conflict code by checking ErrCodeConflict on the returned
error, keeping the existing canonical-path conflict contract pinned.

🪄 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: ASSERTIVE

Plan: Enterprise

Run ID: a025db1e-7659-4dfe-9aef-dd9202f1128f

📥 Commits

Reviewing files that changed from the base of the PR and between e19674c and f672ab1.

📒 Files selected for processing (4)

docs/contributor/evidence-publishing.md
pkg/cli/evidence_sign_test.go
pkg/evidence/attestation/pointer.go
pkg/evidence/attestation/pointer_test.go

…1543) Review feedback (CodeRabbit): the different-content clobber test only checked err != nil, so an INTERNAL regression would still pass. Assert the structured ErrCodeConflict via stderrors.Is so the EEXIST conflict contract stays pinned. Signed-off-by: Nathan Hensley <[email protected]>

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/evidence/attestation/pointer_test.go (1)

323-324: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Pin ErrCodeInvalidRequest in the invalid-digest cases.

This only checks err != nil, so the test still passes if the canonical-digest guard regresses and relocation later fails on the missing x.yaml with an internal filesystem error. Assert the structured invalid-request code here to keep the pre-filesystem rejection contract pinned.

Suggested change

-			if _, err := RelocatePointerToCanonical(filepath.Join(t.TempDir(), "x.yaml"), evil); err == nil {
-				t.Errorf("digest %q: expected a rejection, got nil", digest)
-			}
+			if _, err := RelocatePointerToCanonical(filepath.Join(t.TempDir(), "x.yaml"), evil); !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) {
+				t.Errorf("digest %q: expected ErrCodeInvalidRequest, got %v", digest, err)
+			}

🤖 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 `@pkg/evidence/attestation/pointer_test.go` around lines 323 - 324, The
invalid-digest test currently only checks for a non-nil error, so it can miss a
regression where `RelocatePointerToCanonical` fails later for the wrong reason.
Update the invalid-digest cases in `pointer_test.go` to assert the structured
`ErrCodeInvalidRequest` from the error returned by `RelocatePointerToCanonical`,
keeping the canonical-digest rejection contract pinned before any filesystem
lookup happens.

🤖 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 `@pkg/evidence/attestation/pointer_test.go`:
- Around line 323-324: The invalid-digest test currently only checks for a
non-nil error, so it can miss a regression where `RelocatePointerToCanonical`
fails later for the wrong reason. Update the invalid-digest cases in
`pointer_test.go` to assert the structured `ErrCodeInvalidRequest` from the
error returned by `RelocatePointerToCanonical`, keeping the canonical-digest
rejection contract pinned before any filesystem lookup happens.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: aca290aa-4948-4d92-8c3a-20394bbaca42

📥 Commits

Reviewing files that changed from the base of the PR and between f672ab1 and 1a56c4a.

📒 Files selected for processing (1)

pkg/evidence/attestation/pointer_test.go

mchmarny

Approving — clean, well-tested follow-up to #1538. (Re-reviewed at 7d99f4d: the only change since the prior head was tightening the conflict test to assert ErrCodeConflict via stderrors.Is instead of bare non-nil — strictly better.)

Strict digest (isHexDigest → exact sha256:+64 lowercase hex) is correctly wired into canonicalPointerSuffix before filepath.Join, alongside the filepath.IsLocal(p.Recipe) guard. Both attacker-influenced path segments now fail closed — this fully closes the digest-traversal concern from #1538, and the previously-spurious sha256:.. case now genuinely exercises the guard.
Idempotent recovery via pointerAlreadyPlaced (same-inode OR byte-identical) correctly handles the committed-both git-round-trip case; deterministic serialization means byte-equality ⟺ same signing output, so only differing bytes trip the conflict. Bounded os.Open + io.LimitReader read, fail-closed on read error — matches repo patterns.
Package-write docs are the right operator-side complement to the workflow packages: write grant.

Tests cover the new paths well (byte-identical-distinct-inode recovery, different-content conflict with code assertion, same-inode recovery, full strict-digest matrix). The --relocate unverified-signer item is appropriately scoped out to #1535. Nothing blocks merge once e2e lands.

njhensley requested a review from a team as a code owner June 29, 2026 21:07

njhensley changed the title ~~fix(evidence): relocate idempotency + strict digest + package-write docs~~ fix(evidence): relocate idempotency, strict digest, package docs Jun 29, 2026

njhensley added the theme/supply-chain SLSA, SBOM, Sigstore, and provenance verification label Jun 29, 2026

github-actions Bot added area/docs area/cli size/L labels Jun 29, 2026

coderabbitai Bot reviewed Jun 29, 2026

View reviewed changes

Comment thread pkg/evidence/attestation/pointer_test.go Outdated

mchmarny assigned njhensley Jun 29, 2026

njhensley and others added 2 commits June 29, 2026 14:15

Merge branch 'main' into evidence/relocate-followups

7d99f4d

mchmarny enabled auto-merge (squash) June 29, 2026 21:18

coderabbitai Bot reviewed Jun 29, 2026

View reviewed changes

mchmarny approved these changes Jun 29, 2026

View reviewed changes

Merge branch 'main' into evidence/relocate-followups

01eab08

mchmarny disabled auto-merge June 29, 2026 21:22

mchmarny merged commit 7839464 into NVIDIA:main Jun 29, 2026
9 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(evidence): relocate idempotency, strict digest, package docs#1543

fix(evidence): relocate idempotency, strict digest, package docs#1543
mchmarny merged 4 commits into
NVIDIA:mainfrom
njhensley:evidence/relocate-followups

njhensley commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

coderabbitai Bot commented Jun 29, 2026 •

edited

Loading

Walkthrough

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

mchmarny left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

njhensley commented Jun 29, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

github-actions Bot commented Jun 29, 2026

Uh oh!

github-actions Bot commented Jun 29, 2026

Recipe evidence check

Uh oh!

coderabbitai Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mchmarny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jun 29, 2026 •

edited

Loading