feat(recipe): signed catalog provenance for V1 closed supply chain#1216
Conversation
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (2 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
mchmarny
force-pushed
the
feat/recipe-catalog-provenance
branch
from
e216f3b to
53a9913
Compare
Adds retrievable provenance and a consumer verification path for the recipe catalog (registry.yaml + validators/catalog.yaml), filling the recipe-data signing workstream of the V1 closed supply chain bar. Implementation reuses pkg/bundler/attestation (signing primitive, Sigstore keyless OIDC, DSSE + in-toto + SLSA Provenance v1) and pkg/bundler/verifier (Fulcio identity pinning) so catalog attestations are structurally identical to bundle attestations — same bundle file format, same predicate type, different buildType URI (https://aicr.nvidia.com/recipe-catalog/v1). Highlights: - pkg/recipe/catalog: ComputeDigest (deterministic SHA-256 over serializer.MarshalYAMLDeterministic of both files), Sign (delegates to attestation.Attester), Verify (delegates to verifier.VerifyBinaryAttestation with NVIDIA CI identity pinning) - pkg/bundler/attestation: StatementMetadata.BuildType (optional; empty falls back to BundleBuildType — no regression for existing callers) + CatalogBuildType constant - pkg/recipe: export RegistryFileName/CatalogFileName (unexported aliases preserved for internal callers) - CLI: hidden 'aicr recipe sign-catalog' (CI-only) + user-facing 'aicr recipe verify-catalog <bundle-path>' - Release: goreleaser after-hook signs the catalog (gated on linux/amd64 + SLSA_PREDICATE so local builds skip OIDC); published as recipe-catalog.sigstore.json release asset and bundled into the aicr tarball Closes #1155.
mchmarny
force-pushed
the
feat/recipe-catalog-provenance
branch
from
53a9913 to
4e89973
Compare
📝 WalkthroughWalkthroughThis PR implements signed recipe catalog provenance for AICR by adding deterministic SHA-256 digest computation over embedded registry and validator catalogs, Sigstore keyless signing via OIDC attesters, and verification against bundled signatures. Changes include new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/recipe/catalog/digest.go`:
- Around line 40-41: The return in digest.go that currently uses errors.Wrap
when deterministicYAML (and similar calls) fail should use
errors.PropagateOrWrap so existing structured error codes (e.g.,
ErrCodeInternal, ErrCodeTimeout) are preserved; replace the errors.Wrap usage in
the function that returns attestation.AttestSubject (the error path calling
deterministicYAML) with errors.PropagateOrWrap and change the message to
accurately reflect the operation (e.g., "failed to process registry/catalog
content" or "failed to parse/serialize registry file") so parse/marshal/timeout
errors are not masked; apply the same replacement pattern to the similar wrap at
the other occurrence mentioned (lines 46-47).
- Line 87: The call to serializer.MarshalYAMLDeterministic(v) should have its
returned error wrapped using errors.PropagateOrWrap to preserve context; replace
the direct return with capturing the bytes and error, and if err != nil return
errors.PropagateOrWrap(err, "serializer.MarshalYAMLDeterministic failed")
(keeping the original return value on success). Update the surrounding function
that currently returns serializer.MarshalYAMLDeterministic (the line with
serializer.MarshalYAMLDeterministic(v)) to use this pattern so errors are
wrapped consistently.
In `@pkg/recipe/catalog/sign.go`:
- Around line 28-90: Add a unit test that exercises Sign's bundle-write path by
calling Sign with a SignOptions whose Attester returns non-nil bundle bytes and
whose Output is a temp file path; verify writeBundle(path, data) is invoked by
asserting the file exists and its contents equal the attester's bytes.
Specifically, create a mock attester implementing attestation.Attester that
returns []byte(`{"test":"bundle"}`) from its Attest method, call Sign(ctx,
provider, SignOptions{Attester: mockAttester, Output: outPath, ToolVersion:
"v0.0.0-test"}), then os.Stat/out.ReadFile the outPath and compare content to
the expected bytes to cover the Sign -> writeBundle branch.
In `@pkg/recipe/catalog/verify_test.go`:
- Around line 28-52: Add a positive-path unit test that asserts catalog.Verify
returns no error and expected result for a valid bundle: create a new test
(e.g., TestVerify_Success) that uses either a pre-signed bundle fixture written
to a temp file or injects a mock provider/verifier which causes
verifier.VerifyBinaryAttestation to succeed for a known artifact digest; call
catalog.Verify(ctx, bundlePath, newFullProvider() or the mocked provider,
catalog.VerifyOptions{}) and assert err == nil and the returned verification
object matches expected values. Ensure the test references catalog.Verify and
the verifier interface used by newFullProvider (or the mock) so it exercises the
success path without altering existing error-path tests.
🪄 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: b9a54c67-b59b-448f-ac12-4cc41a4569a4
📒 Files selected for processing (14)
.goreleaser.yamlpkg/bundler/attestation/statement.gopkg/cli/recipe.gopkg/cli/recipe_sign_catalog.gopkg/cli/recipe_test.gopkg/cli/recipe_verify_catalog.gopkg/recipe/catalog/digest.gopkg/recipe/catalog/digest_test.gopkg/recipe/catalog/doc.gopkg/recipe/catalog/sign.gopkg/recipe/catalog/sign_test.gopkg/recipe/catalog/verify.gopkg/recipe/catalog/verify_test.gopkg/recipe/provider.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/cli/recipe_verify_catalog.go`:
- Around line 28-55: Add support for standard output formats by adding an
output-format flag to recipeVerifyCatalogCmd and implementing format-aware
output in runRecipeVerifyCatalogCmd: add a cli.StringFlag (e.g.,
"output-format") to recipeVerifyCatalogCmd with accepted values "json", "yaml",
"table" (and an env var if desired), then in runRecipeVerifyCatalogCmd detect
the requested format and emit a structured result object (success boolean,
verified_digest string, bundle_path string, error message when applicable)
serialized as JSON/YAML or rendered as a simple table for the "table" mode; keep
human-readable text only for the default/interactive mode and ensure non-zero
exit on verification failure while returning machine-readable output in the
chosen format.
In `@pkg/recipe/catalog/digest.go`:
- Around line 53-56: The combined digest is ambiguous because it hashes regBytes
|| catBytes without a delimiter; change the construction in digest.go (the
sha256.New() / combined.Write sequence that produces combinedHex) to encode the
pair injectively by prefixing each byte slice with its length (e.g., write a
fixed-size or varint length for regBytes then regBytes, then length for catBytes
then catBytes) before hashing, so the combined.Sum uses an unambiguous
representation; update the variables/usage around combined, regBytes, catBytes
and combinedHex accordingly.
In `@pkg/recipe/catalog/sign.go`:
- Around line 52-69: In Sign, guard against a nil Attester on the SignOptions to
avoid a panic: check opts.Attester == nil near the top of the Sign function
(before calling opts.Attester.Attest) and return a structured invalid-request
error (use the project's error helper similar to errors.PropagateOrWrap with an
appropriate invalid-request code, e.g., errors.ErrCodeInvalidRequest) with a
clear message like "missing attester in SignOptions"; reference Sign,
SignOptions, opts.Attester.Attest, and errors.PropagateOrWrap when making the
change.
🪄 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: a245b628-5018-4c2e-8421-5a4af3586487
📒 Files selected for processing (14)
.goreleaser.yamlpkg/bundler/attestation/statement.gopkg/cli/recipe.gopkg/cli/recipe_sign_catalog.gopkg/cli/recipe_test.gopkg/cli/recipe_verify_catalog.gopkg/recipe/catalog/digest.gopkg/recipe/catalog/digest_test.gopkg/recipe/catalog/doc.gopkg/recipe/catalog/sign.gopkg/recipe/catalog/sign_test.gopkg/recipe/catalog/verify.gopkg/recipe/catalog/verify_test.gopkg/recipe/provider.go
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Multi-persona review — signed catalog provenance
Verdict: Request changes. One blocking security defect; otherwise a clean, well-structured PR that closes a real V1 supply-chain gap by reusing the existing attestation/verifier primitives. The reuse keeping catalog attestations structurally identical to bundle attestations is the right call.
Detailed, line-anchored findings are in the inline comments below. Index:
| # | Sev | Where | Finding |
|---|---|---|---|
| 1 | 🔴 Blocking | catalog/verify.go:65-70 |
Identity pinning is bypassable — --identity-pattern skips ValidateIdentityPattern |
| 2 | 💡 Suggestion | cli/recipe.go:135 |
Consider a top-level aicr catalog namespace (mirrors evidence) instead of recipe-nested verbs |
| 3 | 🟠 High | catalog/digest.go:40 |
ComputeDigest clobbers inner error code (timeout/internal → NotFound) |
| 4 | 🟠 High | docs | Missing docs/user/cli-reference.md entry for the new user-facing verify-catalog (CLAUDE.md requires it in-PR) |
| 5 | 🟠 High | catalog/sign.go:83, cli/recipe_sign_catalog.go:103 |
Real-bundle write path + both CLI Actions are 0% covered; error tests assert err != nil not codes |
| 6 | 🟡 Medium | catalog/digest.go:84 |
Digest hashes re-serialized YAML, not shipped bytes (drops comments, silently truncates multi-doc) |
| 7 | 🟡 Medium | .goreleaser.yaml:48,162 |
Catalog bundle ships in all 4 archives; "same gate" comment inaccurate; loose asset not in checksums.txt |
| 8 | 🟡 Medium | .goreleaser.yaml:48 |
Catalog-signing failure aborts the whole release (Fulcio dependency, no retry) |
Low / nits (not separately inlined): combined digest has no length delimiter (digest.go:54-55, also inlined); duplicated "recipe-catalog" literal (digest.go/sign.go:59) — extract a const; Sign(ctx, provider, opts) vs Verify(ctx, bundlePath, provider, opts) arg-order asymmetry; ComputeDigest exported with no external caller; redundant os.Stat in Verify (benign); SLSA_PREDICATE reused as a presence sentinel; verify-catalog never asserts the predicate buildType (defense-in-depth — replay already blocked by digest binding).
Confirmed compliant (no action): writeBundle Close()-error handling, the CI-only //nolint:gosec, slog-vs-cmd.Root().Writer split, the RegistryFileName/CatalogFileName export + unexported-alias trick, and {{ .Path }} / darwin-arm clean-skip in the goreleaser hook. Context-timeout omission on the package entry points is acceptable (the provider honors ctx.Err(); the sigstore paths bound their own I/O).
Bottom line: Fix #1 before merge — it nullifies the PR's central guarantee. #3–#5 should land in the same PR. #2 is a suggestion worth weighing before release (CLI surface is compatibility-sensitive), but it's the author's call.
Reviewed across four personas (supply-chain security, Go conventions, release engineering, API/test design) and meta-reviewed for accuracy.
- Add TestSign_WritesBundleToFile exercising Sign's bundle-write path via a fake Attester that returns canned bytes; lifts package coverage from 69.5% → 82.3%. - Wrap serializer.MarshalYAMLDeterministic errors with PropagateOrWrap so structured codes from the serializer survive the boundary, matching the pattern applied to provider.ReadFile and Attester.Attest calls.
…tests Address review findings from #1216: - BLOCKING: verify.go was passing --identity-pattern straight to VerifyBinaryAttestation without calling ValidateIdentityPattern, so --identity-pattern '.*' (or env override) silently disabled effective repo pinning. Enforce the documented "must contain NVIDIA/aicr" invariant up front and return ErrCodeInvalidRequest on violation. - Hash raw provider.ReadFile bytes directly (drop the yaml.Unmarshal → MarshalYAMLDeterministic round-trip) so the digest covers the exact shipped bytes — preserves comments and tolerates multi-document YAML the round-trip would have silently truncated. - Combine registry + catalog bytes with an injective length-prefixed encoding (u64-BE(len)||bytes) so two file-boundary splits cannot collide on the same combined digest. - Sign now rejects a nil Attester with ErrCodeInvalidRequest instead of panicking on opts.Attester.Attest. - Tests: assert structured error codes (ErrCodeNotFound / ErrCodeInvalidRequest) instead of just err != nil; add coverage for the --identity-pattern '.*' rejection, the nil-Attester guard, and the CLI Action's missing-positional path. Coverage rises from 82.3% to 85.2%. - docs/user/cli-reference.md: add the aicr recipe verify-catalog reference entry (CLAUDE.md requires user-facing docs in-PR). - .goreleaser.yaml: correct the inaccurate "same gate" comment on the signing hook; document why the bundle ships in every archive (signed once, arch-independent) and why the loose release asset is intentional despite living outside checksums.txt.
|
@njhensley thanks for the multi-persona pass — that's exactly the level of scrutiny this corner of the supply chain needs. Disposition for all 8 findings from your review:
Plus the low / nit items from your tail paragraph:
All 13 inline threads resolved. cc @coderabbitai — the |
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/cli/recipe_test.go`:
- Around line 849-851: The test should assert the structured invalid-request
error contract instead of only checking message text: replace the brittle
strings.Contains(err.Error(), "usage") check with an errors.Is(err,
ErrInvalidRequest) (or the actual sentinel error name used by the package, e.g.,
ErrInvalidUsage/ErrInvalidRequest) to verify identity, and optionally keep the
message substring check for extra context; ensure you import the "errors"
package and reference the existing err variable in recipe_test.go so the
assertion becomes errors.Is(err, <SentinelError>) (and then t.Errorf if that
fails).
🪄 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: 4c6b5a35-46de-4c0e-81e4-28785a148641
📒 Files selected for processing (9)
.goreleaser.yamldocs/user/cli-reference.mdpkg/cli/recipe_test.gopkg/recipe/catalog/digest.gopkg/recipe/catalog/digest_test.gopkg/recipe/catalog/sign.gopkg/recipe/catalog/sign_test.gopkg/recipe/catalog/verify.gopkg/recipe/catalog/verify_test.go
|
Accepted as-fixed: Items 1, 3, 4, 5, 6, 7, and the low/nit items (u64-BE length prefix, Accepted as-confirmed-acceptable: Items 2 and 8. The On the All 13 threads resolved, (≡^• ᴥ •^≡) 🐾 ✅ Action performedComments resolved. Approval is disabled; enable |
Review reconciliation — remaining open itemsReconciled across the three reviews on this PR (CodeRabbit, @njhensley's multi-persona review, and a separate pass of my own) against the current head ✅ Resolved since the original reviews
Remaining (none blocking)
Bottom line: no blockers remain. Item 1 (namespace) and item 2 (confirm release-abort is intended) are the two worth a decision before merge; 3–5 are optional polish. Nice, clean reuse of the existing attestation/verifier primitives. |
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Approving — thanks for the fast, thorough turnaround.
The blocking identity-pin bypass is fixed (Verify now calls verifier.ValidateIdentityPattern and returns ErrCodeInvalidRequest, with TestVerify_RejectsOverlyBroadIdentityPattern locking in that --identity-pattern '.*' is rejected). The error-code clobbering (PropagateOrWrap), the docs entry, and the test gaps (real-bundle write path via fakeAttester, nil-attester guard, NArg guard, code-based assertions) are all addressed.
Nice resolution on the digest: hashing the raw ReadFile bytes with length-prefixed framing (u64-BE(len) || bytes) fixes both the re-serialization concern (what ships is what's signed, multi-doc safe) and the boundary-ambiguity nit in one change.
Remaining items are non-blocking and your calls: keeping the commands under recipe (reasonable either way), skipping the positive Verify path test (consistent with how aicr verify / evidence verify are tested), and the release-abort note (#8), which mirrors the existing binary hook. LGTM.
|
Thanks @lockwobr — taking each of the remaining items in turn: 1. CLI namespace ( 2. Release-abort on Fulcio failure — confirmed intentional. Fail-closed is the right posture for a supply-chain release: a release missing its catalog signature is worse than a delayed release. The Fulcio dependency surface is identical to the existing binary-attestation hook above it (same endpoints, same OIDC token, same network path), so this PR doesn't introduce a new failure mode, it just doubles the per-release call count to public-good Sigstore. If Fulcio outage ever becomes a recurring release pain point, the right fix is workflow-level retry-with-backoff wrapping the goreleaser invocation, not hook-level — tracked under the supply-chain hardening epic #1149. 3. 4. Success-path tests — agreed, accepting the gap. The sigstore-go verification is exercised end-to-end by the existing 5. Arg-order asymmetry ( All five threads were addressed via individual inline replies and resolved earlier. CI re-run on |
3 participants
Summary
Adds retrievable provenance and a consumer verification path for the recipe catalog (
registry.yaml+validators/catalog.yaml), filling the recipe-data signing workstream of the V1 closed-supply-chain bar.Motivation / Context
The V1 "Closed Supply Chain" acceptance criterion requires every artifact — binary, image, recipe data, bundle — to have retrievable provenance and a documented verification path. Today the recipe catalog is shipped unsigned with no verification path; this PR closes that gap.
Fixes: #1155
Related: #1149 (parent epic)
Type of Change
Component(s) Affected
pkg/cli)pkg/recipe, newpkg/recipe/catalog)pkg/bundler/attestation— additiveBuildTypefield).goreleaser.yaml)Implementation Notes
Implementation reuses
pkg/bundler/attestation(Sigstore keyless OIDC + DSSE + in-toto + SLSA Provenance v1) andpkg/bundler/verifier(Fulcio identity pinning), so catalog attestations are structurally identical to bundle attestations — same bundle file format, same predicate type, differentbuildTypeURI (https://aicr.nvidia.com/recipe-catalog/v1).pkg/recipe/catalog(new):ComputeDigest(deterministic SHA-256 overserializer.MarshalYAMLDeterministicof both files),Sign(delegates toattestation.Attester),Verify(delegates toverifier.VerifyBinaryAttestationwith NVIDIA CI identity pinning).pkg/bundler/attestation:StatementMetadata.BuildTypeis optional — empty falls back toBundleBuildType, so existing bundle attestation callers are unchanged. NewCatalogBuildTypeconstant.pkg/recipe:RegistryFileName/CatalogFileNameexported; unexported aliases preserved for internal callers (zero churn elsewhere).aicr recipe sign-catalog --output <path>(CI-only, reuses the same OIDC precedence asaicr bundle --attest); user-facingaicr recipe verify-catalog <bundle-path>with optional--identity-patternfor testing.linux/amd64+SLSA_PREDICATEenv var, same guard as the binary attestation hook so local goreleaser runs skip the OIDC flow). The bundle is published as arecipe-catalog.sigstore.jsonrelease asset and included in the aicr tarball.LayeredDataProviderwas intentionally not implemented; the CLI subcommand satisfies the V1 verification-path requirement and avoids arecipe → catalog → recipeimport cycle.Testing
Results:
pkg/recipe/catalog69.5% statement coverage)go vet: passgolangci-lint: 0 issuesNew tests cover determinism, missing-file error paths, NoOp signing, digest equivalence between
ComputeDigestandSign, bundle-not-found and invalid-bundle verification paths, and CLI subcommand registration (visible/hidden).Risk Assessment
BuildTypefield defaults to empty →BundleBuildType(no regression). The goreleaser after-hook is gated onSLSA_PREDICATEenv presence so local/PR builds are unaffected.Rollout notes: The first release after this PR merges will publish the signature; older releases remain unsigned. Verification is opt-in via the new CLI subcommand.
Checklist
make testwith-race)make lint)--help)git commit -S)