fix(evidence): make unsigned-commit CI-sign flow coherent (#1530)#1538
Conversation
njhensley
added
the
theme/supply-chain
|
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:
📝 WalkthroughWalkthroughThis PR implements the commit-flat → CI-sign → CI-relocate-to-nested evidence publishing flow. It adds canonical pointer relocation in the attestation package, introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Solid, well-tested supply-chain change. The commit-flat → CI-sign → CI-relocate flow is coherent, the relocation logic lives in pkg/evidence/attestation (CLI just orchestrates), and the defensive details are right — filepath.IsLocal traversal guard on the recipe segment that doesn't depend on the CI gate, ErrCodeConflict on the immutable-pointer clobber refusal, and thorough tests (move/no-op/clobber/unsigned/no-digest/traversal + the flat-pending gate cases + CLI idempotence). Docs updated coherently.
One non-blocking design question on the contract gate: it now accepts an unsigned flat pending pointer with no ownership/allowlist check on every PR, and nothing enforces the sign+relocate leg ran before merge — so an unsigned flat pointer can land on main and stay unsigned (auto-sign is fork-only). Not a trust bypass since unsigned pointers are unverifiable and ownership is enforced once signed/relocated, but worth confirming the intended merge-time guarantee. Plus one nit on a log line. Nothing blocks merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/evidence-publish.yaml:
- Around line 27-34: The workflow comment in evidence-publish.yaml no longer
matches the behavior of .github/scripts/evidence-sign-unsigned.sh. Update the
comment near the evidence-publish flow description to say it scans all flat
recipes/evidence/*.yaml files except allowlist.yaml and runs aicr evidence sign
--relocate on them, including already-signed flat pointers from partial prior
runs; keep the wording aligned with the actual idempotent CI behavior and remove
the “empty signer block” claim.
In `@docs/contributor/evidence-publishing.md`:
- Around line 74-78: The commit example in the evidence publishing guide
conflicts with the repository’s signing policy. Update the instructions in the
commit workflow to require every contributor to use both sign-off and
cryptographic signing, and make sure the guidance around the commit step points
to the same requirement in CONTRIBUTING.md. Keep the reference tied to the
commit command example in the evidence publishing section so readers see the
correct `git commit -s -S` flow.
In `@pkg/evidence/attestation/pointer.go`:
- Around line 176-186: The no-clobber check in the canonical pointer move path
is racy because `os.Stat(dest)` and `os.Rename(currentPath, dest)` are separate
steps. Update the pointer promotion logic in `pointer.go` so the exclusivity
check and move are enforced atomically, using the existing canonical-path flow
around `os.Stat`, `os.MkdirAll`, and `os.Rename`. Prefer an atomic create/rename
strategy or synchronization mechanism that guarantees `dest` cannot be created
or replaced by another process between the check and the move.
🪄 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: 01589235-f30f-4886-bcd9-5bdb466e1041
📒 Files selected for processing (11)
.github/scripts/evidence-sign-unsigned.sh.github/workflows/evidence-publish.yamldocs/contributor/evidence-publishing.mddocs/user/cli-reference.mdpkg/cli/consts.gopkg/cli/evidence_sign.gopkg/cli/evidence_sign_test.gopkg/evidence/attestation/pointer.gopkg/evidence/attestation/pointer_test.gopkg/evidence/verifier/discover.gopkg/evidence/verifier/discover_test.go
The commit-unsigned-then-CI-sign evidence flow was internally contradictory: the sign script globbed a flat path that matched no committed pointer (and mis-fired on allowlist.yaml), an unsigned pointer could not be placed at its nested <source> path (that segment derives from the signer it lacks), and the blocking contract gate rejected the flat intermediate before signing could run. Implement the coherent commit-flat -> CI-sign -> CI-relocate-to-nested design: - attestation: add RelocatePointerToCanonical, which moves a signed flat pointer to recipes/evidence/<recipe>/<source>/<digest>.yaml. Extract canonicalPointerSuffix as the single source of truth for the per-source layout, shared with PointerCopyToHint. - cli: add 'aicr evidence sign --relocate' (sign-then-relocate; idempotent relocate-only recovery for an already-signed flat pointer). - verifier: CheckEvidenceTree now accepts a flat <recipe>.yaml as an unsigned, single-attestation, bundle-referencing pending pointer; still rejects signed-flat, misnamed, and unpushed files. Path-ownership and allowlist are enforced on the signed, relocated pointer. - ci: evidence-sign-unsigned.sh scans flat pointers, skips allowlist.yaml, and calls sign --relocate; evidence-publish.yaml stages the move with git add -A and commits the relocation. - docs: cli-reference and evidence-publishing describe sign+relocate and the flat-pending convention. Signed-off-by: Nathan Hensley <[email protected]>
njhensley
force-pushed
the
evidence/unsigned-pointer-flow
branch
from
4806d7a to
a61d4d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/contributor/evidence-publishing.md (1)
74-78: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
-s -Sin the example.This still tells external contributors to commit with
-salone, which conflicts with the repo policy that every commit must be both signed off and cryptographically signed. As per coding guidelines:git commit -s -S.🤖 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 `@docs/contributor/evidence-publishing.md` around lines 74 - 78, Update the commit example in the contributor evidence publishing guide to match the repo policy by showing git commit with both DCO sign-off and cryptographic signing. Adjust the example around the evidence publishing steps so it uses the same signing flags expected by the commit workflow, and keep the surrounding guidance consistent with the commit signing requirement referenced in CONTRIBUTING.md.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.
Inline comments:
In `@docs/contributor/evidence-publishing.md`:
- Around line 137-152: The current wording in evidence-publishing.md overstates
that the signed artifact’s “result is identical regardless of which host ran
which leg”; clarify that only the bundle contents are content-addressed and
equivalent, while the committed pointer location still depends on the
signer-derived <src> segment and can differ by host. Update the explanation
around `aicr evidence publish`, `copyTo`, and the canonical per-source path to
emphasize bundle equivalence versus pointer path placement, without implying all
hosts produce the same final path.
In `@pkg/evidence/attestation/pointer.go`:
- Around line 113-132: The relocation path in attestation/pointer.go should fail
closed by validating att.Bundle.Digest before it is turned into the filename in
the pointer path builder. In RelocatePointerToCanonical, keep the existing
non-empty check but also reject any digest containing path separators or
traversal-like segments before the strings.ReplaceAll and filepath.Join steps,
so the resulting <recipe>/<source>/<digest>.yaml leaf cannot escape the intended
directory even if upstream validation misses it.
---
Duplicate comments:
In `@docs/contributor/evidence-publishing.md`:
- Around line 74-78: Update the commit example in the contributor evidence
publishing guide to match the repo policy by showing git commit with both DCO
sign-off and cryptographic signing. Adjust the example around the evidence
publishing steps so it uses the same signing flags expected by the commit
workflow, and keep the surrounding guidance consistent with the commit signing
requirement referenced in CONTRIBUTING.md.
🪄 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: ab19ec3a-5934-4360-9d1d-d07573d8704c
📒 Files selected for processing (11)
.github/scripts/evidence-sign-unsigned.sh.github/workflows/evidence-publish.yamldocs/contributor/evidence-publishing.mddocs/user/cli-reference.mdpkg/cli/consts.gopkg/cli/evidence_sign.gopkg/cli/evidence_sign_test.gopkg/evidence/attestation/pointer.gopkg/evidence/attestation/pointer_test.gopkg/evidence/verifier/discover.gopkg/evidence/verifier/discover_test.go
Review remediation for NVIDIA#1530: - verifier: add a merge-time guard. CheckEvidenceTree takes an allowPending flag and evidence-pointercheck a --allow-pending flag (default false), so the blocking contract gate rejects a flat unsigned pending pointer — it must be signed and relocated under <recipe>/<source>/ before merge, so an unsigned pointer can never land on main (where the fork-only sign leg never runs). Acceptance stays reachable via --allow-pending for local/transient checks. - attestation: make the canonical-path move atomic. Replace os.Stat+os.Rename (TOCTOU; Rename clobbers on Unix) with os.Link+os.Remove, which fails closed on EEXIST. - cli: the idempotent relocate-only path logs "already at canonical path; nothing to do" instead of a misleading "relocated" when dest == path. - docs/ci: align the evidence-publish workflow comment with the script's flat-pointer scan; require git commit -s -S in evidence-publishing.md (matches CONTRIBUTING.md); document the merge-gate reject + --allow-pending. Signed-off-by: Nathan Hensley <[email protected]>
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 `@pkg/evidence/verifier/discover.go`:
- Around line 77-89: The exported CheckEvidenceTree API currently takes a bare
allowPending bool, which makes the call site opaque and harder to extend.
Refactor this configuration into the existing CheckOption pattern by adding a
WithPendingPointersAllowed() option and applying it inside CheckEvidenceTree.
Update any callers and option handling around CheckEvidenceTree and related
CheckOption plumbing so pending-pointer behavior is expressed consistently with
the rest of the codebase.
🪄 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: 8039484f-901d-4226-908f-215fe5841754
📒 Files selected for processing (8)
.github/workflows/evidence-pointer-contract.yaml.github/workflows/evidence-publish.yamldocs/contributor/evidence-publishing.mdpkg/cli/evidence_sign.gopkg/evidence/attestation/pointer.gopkg/evidence/verifier/discover.gopkg/evidence/verifier/discover_test.gotools/evidence-pointercheck/main.go
| // allowPending controls the flat root-level <recipe>.yaml *pending* pointer | ||
| // (unsigned, single-attestation, bundle-referencing) — the transient | ||
| // commit-flat state of the two-phase publish flow (#1530), which the | ||
| // fork-based CI leg signs and relocates under <recipe>/<source>/. When true it | ||
| // is accepted as a valid intermediate; when false (the merge gate's posture) a | ||
| // flat root file is rejected just like any other unexpected root file, so an | ||
| // unsigned pointer cannot land on a protected branch — the relocation must | ||
| // have run first. See checkPendingPointer. | ||
| // | ||
| // It returns the list of problems (empty when the tree is clean) plus a | ||
| // non-nil error only for an operational failure (unreadable allowlist, etc.), | ||
| // keeping policy violations distinct from infrastructure errors. | ||
| func CheckEvidenceTree(root, allowlistPath string) ([]TreeProblem, error) { | ||
| func CheckEvidenceTree(root, allowlistPath string, allowPending bool) ([]TreeProblem, error) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bCheckEvidenceTree\s*\(' pkg toolsRepository: NVIDIA/aicr
Length of output: 1735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== discover.go ==\n'
sed -n '1,240p' pkg/evidence/verifier/discover.go
printf '\n== options patterns in pkg/evidence/verifier ==\n'
rg -n "type .*Option|func With.*Option|opts \.\.\.|functional options|checkOptions" pkg/evidence/verifier pkg/evidence tools -g '*.go'Repository: NVIDIA/aicr
Length of output: 11904
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== types.go ==\n'
sed -n '1,220p' pkg/evidence/verifier/types.go
printf '\n== discover_test excerpts ==\n'
sed -n '110,360p' pkg/evidence/verifier/discover_test.go
printf '\n== direct callers ==\n'
rg -n "CheckEvidenceTree\(" -g '*.go' .Repository: NVIDIA/aicr
Length of output: 17732
Model allowPending as a CheckOption.
allowPending is configuration on an exported API, so a bare bool makes the call site opaque and harder to extend. A WithPendingPointersAllowed() option keeps the surface consistent with the rest of the codebase.
🤖 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/verifier/discover.go` around lines 77 - 89, The exported
CheckEvidenceTree API currently takes a bare allowPending bool, which makes the
call site opaque and harder to extend. Refactor this configuration into the
existing CheckOption pattern by adding a WithPendingPointersAllowed() option and
applying it inside CheckEvidenceTree. Update any callers and option handling
around CheckEvidenceTree and related CheckOption plumbing so pending-pointer
behavior is expressed consistently with the rest of the codebase.
Source: Coding guidelines
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Re-reviewed at 08a117a. The two follow-ups from my first pass are handled: the relocate-only log nit is fixed, and the unsigned-pending merge-gate concern is resolved — CheckEvidenceTree now takes allowPending, and the merge gate runs without --allow-pending so a flat unsigned pointer can't land on main. Resolved that thread. CodeRabbit's substantive items (TOCTOU on the rename, doc/signing-policy wording) are also addressed.
One open item remains (inline): the bundle digest isn't validated as a safe filename before it's joined into the canonical path — a ..-bearing digest passes the prefix-only check and reaches os.Rename. Limited real-world reach but a cheap, correct hardening, and it makes the existing code comment accurate. The allowPending-as-functional-option nit is fine to take or leave. Neither blocks merge once the digest guard is in.
Second review round: - attestation: validate the bundle digest as a safe filename (single, local path element) before joining it into the canonical path. validatePointer only enforces the sha256:<hex> shape when bundle.oci is set, so a digest with a separator or ".." could otherwise reach the os.Link relocation sink. Makes the recipe/digest guard comment accurate. (mchmarny, CodeRabbit) - docs: clarify the local-split fallback — the bundle bytes/digest are content-addressed and host-independent, but the committed pointer path is signer-derived (<src>), so it can differ by host. (CodeRabbit) Skipped: allowPending-as-functional-option (trivial; a single bool is clearer than introducing an options struct for one flag). Signed-off-by: Nathan Hensley <[email protected]>
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 `@pkg/evidence/attestation/pointer_test.go`:
- Around line 227-238: The “rejects a digest that is unsafe as a filename”
subtest in pointer_test should make the failure come from the digest-safety
guard, not from os.Link failing on a missing source file. Update the setup
around RelocatePointerToCanonical to write the flat source first (like the
sibling recipe-traversal test) if you want to keep testing rejection, or remove
the `"sha256:.."` case because `canonicalPointerSuffix` accepts it as a
contained local filename. Keep the test focused on the truly unsafe digests and
reference the existing `validatePointer`/`canonicalPointerSuffix` flow when
choosing the inputs.
🪄 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: 38bfc539-6c25-4c6c-be1d-116a9cf6acab
📒 Files selected for processing (3)
docs/contributor/evidence-publishing.mdpkg/evidence/attestation/pointer.gopkg/evidence/attestation/pointer_test.go
| t.Run("rejects a digest that is unsafe as a filename", func(t *testing.T) { | ||
| // defense-in-depth: a digest carrying a separator or ".." must not | ||
| // reach os.Link as a path leaf (e.g. an OCI-empty pointer that skipped | ||
| // the sha256: shape check in validatePointer). | ||
| for _, digest := range []string{"sha256:../../etc/evil", "sha256:a/b", "sha256:.."} { | ||
| evil := signed() | ||
| evil.Attestations[0].Bundle.Digest = digest | ||
| if _, err := RelocatePointerToCanonical(filepath.Join(t.TempDir(), "x.yaml"), evil); err == nil { | ||
| t.Errorf("digest %q: expected a rejection, got nil", digest) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This subtest passes for the wrong reason for "sha256:..". Unlike the sibling recipe-traversal test (Line 248), it never writes the flat source file, so the rejection comes from os.Link failing on a non-existent source — not from the digest safety check.
Trace the validation for "sha256:..": ReplaceAll(":", "-") → "sha256-..", + ".yaml" → "sha256-...yaml". That stem equals filepath.Base and is filepath.IsLocal, so canonicalPointerSuffix accepts it and returns no error. The two slash-bearing digests are genuinely rejected there, but "sha256:.." is not. If you write the source file to make the rejection actually exercise the guard (as the recipe test does), this case would relocate successfully and the assertion would flip.
Either drop the "sha256:.." case (it yields a benign, contained filename) or write the flat source first and keep only the inputs the guard truly rejects.
💚 Make the rejection come from the guard, not a missing source
t.Run("rejects a digest that is unsafe as a filename", func(t *testing.T) {
// defense-in-depth: a digest carrying a separator or ".." must not
// reach os.Link as a path leaf (e.g. an OCI-empty pointer that skipped
// the sha256: shape check in validatePointer).
- for _, digest := range []string{"sha256:../../etc/evil", "sha256:a/b", "sha256:.."} {
+ for _, digest := range []string{"sha256:../../etc/evil", "sha256:a/b"} {
evil := signed()
evil.Attestations[0].Bundle.Digest = digest
- if _, err := RelocatePointerToCanonical(filepath.Join(t.TempDir(), "x.yaml"), evil); err == nil {
+ root := t.TempDir()
+ flat := filepath.Join(root, "x.yaml")
+ if err := os.WriteFile(flat, []byte("x"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := RelocatePointerToCanonical(flat, evil); err == nil {
t.Errorf("digest %q: expected a rejection, got nil", digest)
}
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("rejects a digest that is unsafe as a filename", func(t *testing.T) { | |
| // defense-in-depth: a digest carrying a separator or ".." must not | |
| // reach os.Link as a path leaf (e.g. an OCI-empty pointer that skipped | |
| // the sha256: shape check in validatePointer). | |
| for _, digest := range []string{"sha256:../../etc/evil", "sha256:a/b", "sha256:.."} { | |
| evil := signed() | |
| evil.Attestations[0].Bundle.Digest = digest | |
| if _, err := RelocatePointerToCanonical(filepath.Join(t.TempDir(), "x.yaml"), evil); err == nil { | |
| t.Errorf("digest %q: expected a rejection, got nil", digest) | |
| } | |
| } | |
| }) | |
| t.Run("rejects a digest that is unsafe as a filename", func(t *testing.T) { | |
| // defense-in-depth: a digest carrying a separator or ".." must not | |
| // reach os.Link as a path leaf (e.g. an OCI-empty pointer that skipped | |
| // the sha256: shape check in validatePointer). | |
| for _, digest := range []string{"sha256:../../etc/evil", "sha256:a/b"} { | |
| evil := signed() | |
| evil.Attestations[0].Bundle.Digest = digest | |
| root := t.TempDir() | |
| flat := filepath.Join(root, "x.yaml") | |
| if err := os.WriteFile(flat, []byte("x"), 0o600); err != nil { | |
| t.Fatal(err) | |
| } | |
| if _, err := RelocatePointerToCanonical(flat, evil); err == nil { | |
| t.Errorf("digest %q: expected a rejection, got nil", digest) | |
| } | |
| } | |
| }) |
🤖 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 227 - 238, The
“rejects a digest that is unsafe as a filename” subtest in pointer_test should
make the failure come from the digest-safety guard, not from os.Link failing on
a missing source file. Update the setup around RelocatePointerToCanonical to
write the flat source first (like the sibling recipe-traversal test) if you want
to keep testing rejection, or remove the `"sha256:.."` case because
`canonicalPointerSuffix` accepts it as a contained local filename. Keep the test
focused on the truly unsafe digests and reference the existing
`validatePointer`/`canonicalPointerSuffix` flow when choosing the inputs.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
A few non-blocking correctness items on the relocate flow — the digest path-traversal from the earlier pass is fixed now, thanks. None are hard blockers, but good to fix before merge if feasible.
One higher-impact item that isn't on a changed line, so noting it here:
Fork sign leg can't publish the signature referrer (.github/workflows/evidence-publish.yaml permissions ~L80-82). The workflow grants contents: write + id-token: write but not packages: write, and there's no GHCR/registry login step. SignExisting pushes the Sigstore bundle as an OCI referrer — a registry write — while public-package visibility only enables the pre-sign pull. So a clean hosted fork runner can pull the bundle but can't attach its signature, leaving the pointer unsigned and the documented flow unable to complete end-to-end. Add packages: write and a registry login before the sign step.
| // per-source pointer is immutable, so a pre-existing dest is a real conflict | ||
| // for the operator to resolve, not something to overwrite. | ||
| if err := os.Link(currentPath, dest); err != nil { | ||
| if os.IsExist(err) { |
There was a problem hiding this comment.
Idempotent recovery breaks on a partial prior move: if os.Link succeeds but os.Remove fails (or the process stops between), both the flat source and the canonical dest exist (same inode). A retry sees the still-present flat source (which doesn't satisfy the no-op suffix check), re-runs os.Link, hits EEXIST, and returns ErrCodeConflict — contradicting the documented idempotent recovery and leaving the flat source stuck. Treat an identical/same-inode dest as a completed placement (remove the stale source); keep the conflict only when dest differs.
There was a problem hiding this comment.
Addressed in 4ba93e3 — os.SameFile now treats a same-inode dest as a completed placement (idempotent recovery), keeping the conflict only when dest differs.
| # --yes: never pause for the interactive identity-disclosure prompt in CI | ||
| # (ambient OIDC does not prompt, but this is belt-and-suspenders). | ||
| if "$AICR" evidence sign "$pointer" --yes "${extra_args[@]}"; then | ||
| if "$AICR" evidence sign "$pointer" --relocate --yes "${extra_args[@]}"; then |
There was a problem hiding this comment.
Sign + relocate run in one call: the referrer is attached and the flat pointer rewritten before relocation. If relocation then fails, the command returns nonzero, the script increments only failed (so signed=0 on the usual single-pointer run), and the workflow skips its commit step — discarding the signer patch. The next dispatch re-signs (duplicate referrer) instead of taking the relocate-only recovery path. Ensure the signer-patched pointer still reaches the commit step, or report partial signing separately.
There was a problem hiding this comment.
Largely mitigated by the idempotent-relocate fix — a rerun now recovers a partial move. A narrow edge remains (a true relocation failure still counts failed and skips the commit), but it's much less likely now; fine as a follow-up.
| // falls through to ValidateSignablePointer, which fails closed on an | ||
| // already-signed pointer rather than re-signing it. LoadAndValidatePointer | ||
| // already guarantees exactly one attestation, so [0] is safe. | ||
| if relocate && pointer.Attestations[0].Signer != nil { |
There was a problem hiding this comment.
The relocate-only recovery path takes any non-nil pointer-supplied signer without materializing or cryptographically verifying the bundle (validation only checks signer strings are non-empty). Combined with the contract gate validating only the claimed signer/source/allowlist metadata, this normalizes the trust gap tracked in #1535. Not a new privilege escalation (a contributor can already construct the nested path directly), but worth verifying the signature/bundle before relocating, or scoping --relocate to CI-signed pointers.
| signed; the commit-back itself is a normal, unsigned GitHub Actions commit, | ||
| which the eventual squash-merge re-signs under the repo's policy. | ||
|
|
||
| > **Run this leg before merge.** The blocking per-source contract gate |
There was a problem hiding this comment.
This states a signed, nested pointer is sufficient, but a new fork signs with the GitHub Actions OIDC issuer + workflow identity, producing a <src> that isn't in the allowlist (whose community entry uses the personal GitHub OAuth issuer). checkPointerFile rejects an unlisted signer, so the PR stays blocked until it also adds its issuer/source to the community/partner allowlist. Add the post-sign allowlist-onboarding step (using the generated <src>), or link to the onboarding instructions.
There was a problem hiding this comment.
Addressed in 4ba93e3 — new "Add your signer to the allowlist" section keyed by the generated <src>.
… doc (NVIDIA#1538) Third review round: 1. attestation: validate the bundle digest as canonical sha256:<hex> (isHexDigest) before deriving the path leaf. validatePointer only enforces the sha256: prefix when bundle.oci is set, so a digest like sha256:x/../escaped could traverse on filepath.Join; a hex-only body cannot. 2. attestation: make relocation idempotent on a partial prior move. When os.Link hits EEXIST, treat a same-inode dest (a prior link that didn't get its source removed) as a completed placement — remove the stale source and return dest — instead of a spurious conflict. A different-inode dest stays a conflict. 3. ci: the commit-back step now runs on always() (no signed-count guard), so a pointer signed but not yet relocated (relocation failed after SignExisting patched it) is still committed. The next dispatch then takes the relocate-only path instead of re-signing and attaching a duplicate referrer. 4. docs: add the required post-sign allowlist step — a new fork's Actions OIDC signer must be added to recipes/evidence/allowlist.yaml (keyed by the generated <src> slug) or the nested signed pointer is rejected by the gate. Signed-off-by: Nathan Hensley <[email protected]>
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)
docs/contributor/evidence-publishing.md (1)
98-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the relocate-only retry path.
This workflow does not only pick up unsigned flat pointers.
.github/scripts/evidence-sign-unsigned.shscans every flatrecipes/evidence/*.yamlexceptallowlist.yaml, andaicr evidence sign --relocaterelocates already-signed flat pointers left by a partial prior run without re-signing. The current wording will mislead contributors about the idempotent recovery behavior.Suggested doc fix
-- discovers every flat pointer in `recipes/evidence/*.yaml` with an empty - `signer` (i.e. unsigned), -- signs the bundle each one already references using the runner's ambient - OIDC token and **relocates** the now-signed pointer to its canonical +- scans every flat pointer in `recipes/evidence/*.yaml` except `allowlist.yaml`, +- signs unsigned pointers and also **relocates** already-signed flat pointers + left by a partial prior run, using the runner's ambient OIDC token where + needed, to the canonicalAs per coding guidelines, “Keep code comments and documentation accurate and helpful, and update API documentation when endpoints change.”
🤖 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 `@docs/contributor/evidence-publishing.md` around lines 98 - 104, Update the evidence publishing docs to cover the relocate-only retry behavior in the workflow described around the unsigned-pointers script and the `aicr evidence sign --relocate` command. Clarify that `.github/scripts/evidence-sign-unsigned.sh` scans all flat `recipes/evidence/*.yaml` files except `allowlist.yaml`, and that `aicr evidence sign --relocate` can move already-signed flat pointers from a partial prior run back to their canonical `recipes/evidence/<recipe>/<src>/<digest>.yaml` path without re-signing them. Keep the explanation tied to the existing workflow section and the `evidence-sign-unsigned.sh` and `aicr evidence sign --relocate` symbols so contributors understand the idempotent recovery path.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 `@docs/contributor/evidence-publishing.md`:
- Around line 98-104: Update the evidence publishing docs to cover the
relocate-only retry behavior in the workflow described around the
unsigned-pointers script and the `aicr evidence sign --relocate` command.
Clarify that `.github/scripts/evidence-sign-unsigned.sh` scans all flat
`recipes/evidence/*.yaml` files except `allowlist.yaml`, and that `aicr evidence
sign --relocate` can move already-signed flat pointers from a partial prior run
back to their canonical `recipes/evidence/<recipe>/<src>/<digest>.yaml` path
without re-signing them. Keep the explanation tied to the existing workflow
section and the `evidence-sign-unsigned.sh` and `aicr evidence sign --relocate`
symbols so contributors understand the idempotent recovery path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fbe93264-07a9-4138-b55d-4dd346513f3c
📒 Files selected for processing (4)
.github/workflows/evidence-publish.yamldocs/contributor/evidence-publishing.mdpkg/evidence/attestation/pointer.gopkg/evidence/attestation/pointer_test.go
…IDIA#1538) Fourth review round (yuanchen8911): - ci: the fork sign leg attaches the Sigstore bundle as an OCI referrer — a registry WRITE — but the workflow granted only contents/id-token and never logged in to the registry. Public-package visibility enables the anonymous pre-sign pull, not the attach, so a clean hosted fork runner could pull the bundle but not publish its signature. Add packages:write and a ./.github/ actions/ghcr-login step before the sign step (ORAS reads Docker creds). - cli: note that the relocate-only path trusts the on-disk signer block without verifying the bundle signature — the same metadata-only trust the contract gate applies and not a new escalation; closing it is tracked in NVIDIA#1535. Not changed: the digest-safety subtest (CodeRabbit) — isHexDigest (added last round) rejects "sha256:..", "sha256:a/b", and the slash form in canonicalPointerSuffix before any filesystem call, so the test exercises the guard, not an incidental os.Link failure. Signed-off-by: Nathan Hensley <[email protected]>
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Re-reviewed at 88375d8 — the prior findings are addressed: packages:write + the ghcr-login step let the sign leg publish the referrer; the digest filename is path-guarded; relocation is idempotent (os.SameFile); and the allowlist-onboarding step is documented. Remaining items are non-blocking — a narrow signed-state edge on relocation failure, and the relocate-trust point tracked by #1535. LGTM.
|
Non-blocking follow-ups (not merge blockers — fine to track as issues or a quick follow-up):
|
…gest Discovery walks the per-source nested layout recipes/evidence/<recipe>/<source>/<digest>.yaml (the <recipe>/<source>/*.yaml glob in pkg/evidence/verifier/discover.go), and a flat recipes/evidence/<recipe>.yaml is rejected as an unexpected root file. The prior 'Forward limitations' note still described discovery as flat with the nested layout as forward-compatible — accurate before NVIDIA#1538 (which made the relocate/nested flow the implemented behavior), stale after it merged. Signed-off-by: Yuan Chen <[email protected]>
3 participants
Summary
Make the commit-unsigned-then-CI-sign evidence-publishing flow internally coherent: a contributor commits an unsigned pointer flat, the fork-based CI leg signs it and relocates it to its canonical per-source path, and the contract gate accepts the flat pending pointer as a valid intermediate.
Motivation / Context
The unsigned-commit flow was broken three ways:
evidence-sign-unsigned.shglobbed a flat path that matched no committed pointer (and mis-fired onallowlist.yaml); an unsigned pointer could not be placed at its nested<source>path because that segment derives from the signer it lacks; and the blocking contract gate rejected the flat intermediate before signing could run. The only coherent design is commit-flat → CI-sign → CI-relocate-to-nested, and the relocate step did not exist.Fixes: #1530
Related: #1401 (per-source pointer contract), #1402 (GP2 ingest)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)pkg/errors,pkg/k8s) —pkg/evidencedocs/,examples/).github/(evidence signing workflow + script)Implementation Notes
RelocatePointerToCanonicalmoves a signed flat pointer torecipes/evidence/<recipe>/<source>/<digest>.yaml;canonicalPointerSuffixis the single source of truth for the per-source layout, shared withPointerCopyToHint. Afilepath.IsLocalguard on the recipe segment blocks path traversal even when the function is called directly (no CI gate in front).aicr evidence sign --relocate— sign-then-relocate, with an idempotent relocate-only recovery path for an already-signed flat pointer (and a no-op when already canonical).CheckEvidenceTreeaccepts a flat<recipe>.yamlas an unsigned, single-attestation, bundle-referencing pending pointer; still rejects signed-flat, misnamed, and unpushed files. Path-ownership and allowlist remain enforced on the signed, relocated pointer — anti-squat is not weakened.evidence-sign-unsigned.shscans flat pointers, skipsallowlist.yaml, callssign --relocate;evidence-publish.yamlstages the move withgit add -Aand commits the relocation.Testing
New tests cover relocation (move, no-op, clobber-refusal, unsigned/no-digest rejection, recipe path-traversal rejection), the flat-pending gate cases, and the CLI
--relocateflag (relocate-only idempotence + already-signed-without-relocate fails closed).Risk Assessment
Rollout notes: No migration. The flat-pending acceptance is additive; existing nested signed pointers are unaffected.
--relocateis opt-in.Checklist
make testwith-race)make lint)git commit -S)