feat(supply-chain): default keyless and KMS signing to Rekor v2#1679
Conversation
lockwobr
added
the
theme/supply-chain
github-actions
Bot
added
area/ci
area/tests
area/docs
area/bundler
area/cli
size/XL
labels
|
🌿 Preview your docs: https://nvidia-preview-feat-rekor-v2-signing.docs.buildwithfern.com/aicr |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
|
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 adds Rekor v2 signing support across AICR. It introduces signing-config-based transparency policies, propagates selection through attestation and CLI flows, adds TUF retrieval and export, updates release and Cosign workflows, and documents and tests the new behavior. Estimated code review effort: 4 (Complex) | ~75 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user/cli-reference.md (1)
2833-2856: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a blank line before the synopsis fence.
MD031 requires fenced code blocks to be surrounded by blank lines, and this synopsis opens the fence immediately after the label.
Proposed fix
**Synopsis:** + ```shell aicr trust update [--emit-signing-config <path>]</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/user/cli-reference.mdaround lines 2833 - 2856, The synopsis code fence
in the trust update section is missing a blank line before it, violating the
markdown spacing rule. Update theaicr trust updatedocumentation block so the
fenced shell snippet under “Synopsis” is separated from the preceding label by
an empty line, keeping the rest of the section unchanged.</details> <!-- cr-comment:v1:681cb0096c2f4460b203d351 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details>🤖 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/bundler/attestation/resolver.go`: - Around line 58-69: Update the stale comments on SigningConfigPath and UseTUFSigningConfig in resolver.go so they no longer say “Not supported with KMS signing.” Align the docs with the current behavior in ResolveAttester and ResolveAttesterLazy, where both fields are passed through to NewKMSAttester and KMSAttester.Attest uses transparencyForOptions to honor them. Make the comment for each field reflect that signing config file/TUF targeting is valid with KMS signing, and keep the existing precedence/behavior notes intact. In `@pkg/bundler/attestation/signing.go`: - Around line 134-155: Apply the signing timeout before resolving the transparency policy in SignStatement so the TUF-backed path cannot run with an unbounded context. Update the flow around transparencyForOptions and SignStatementWith so the context passed into NewSigningConfigPolicyFromTUF is already bounded by defaults.SigstoreSignTimeout, or wrap the TUF resolution step with a deadline before calling transparencyForOptions. In `@pkg/cli/bundle.go`: - Around line 108-111: The comment on useTUFSigningConfig in bundle.go is inaccurate because it mentions signingKey as an opt-out even though the actual computation in the opts logic only checks rekorURL and signingConfigPath, with KMS intentionally excluded. Update the struct comment for useTUFSigningConfig to remove signingKey from the listed opt-outs and keep the description aligned with the behavior in the bundle option calculation and kms.go so future maintainers do not change the code to match the comment. - Around line 739-742: Update the help text for the flagSigningKey definition in bundle.go so it no longer says --signing-config is mutually exclusive with --signing-key, and change the Rekor version note to reflect that KMS defaults to Rekor v2. Keep the exclusivity wording limited to --identity-token, --oidc-device-flow, and --fulcio-url, and ensure the Usage string matches the actual behavior of the signing flow. --- Outside diff comments: In `@docs/user/cli-reference.md`: - Around line 2833-2856: The synopsis code fence in the trust update section is missing a blank line before it, violating the markdown spacing rule. Update the `aicr trust update` documentation block so the fenced shell snippet under “Synopsis” is separated from the preceding label by an empty line, keeping the rest of the section unchanged.🪄 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:
17857d99-dd84-47a1-8061-a79325bc32b7📒 Files selected for processing (31)
.github/actions/go-build-release/action.yml.goreleaser.yamldocs/contributor/rekor-v2-signing.mddocs/index.ymldocs/integrator/supply-chain-verification.mddocs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_targeting_test.gopkg/bundler/attestation/signingconfig.gopkg/bundler/attestation/signingconfig_test.gopkg/bundler/attestation/testdata/signing_config_v1.jsonpkg/bundler/attestation/testdata/signing_config_v2.jsonpkg/bundler/attestation/testdata/signing_config_v2_no_tsa.jsonpkg/bundler/attestation/transparency.gopkg/cli/bundle.gopkg/cli/bundle_rekor_v2_test.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/cli/recipe_sign_catalog.gopkg/cli/trust.gopkg/cli/validate_evidence.gopkg/evidence/attestation/emit.gopkg/evidence/attestation/sign.gopkg/trust/trust.gopkg/trust/trust_test.gotests/chainsaw/signing/bundle-headless-oidc-ci/chainsaw-test.yaml
Coverage Report ✅
Coverage BadgeMerging this branch will decrease overall coverage
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. |
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
from
3410fb7 to
738ebe3
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/bundle.go (1)
977-991: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
--signing-configis silently dropped —SigningConfigPathnever reaches the attester.
bundleOIDCResolveOptionssetsUseTUFSigningConfig: opts.useTUFSigningConfigbut never setsSigningConfigPath: opts.signingConfigPath. Compare with the sibling command inpkg/cli/recipe_sign_catalog.go(lines 95-105), which correctly sets bothSigningConfigPath: signingConfigandUseTUFSigningConfig: useV2.As written: when a user passes
--signing-config <path>,parseBundleCmdOptionscorrectly computesuseTUFSigningConfig = false(line 180, sincesigningConfigPath != ""), but the actualattestation.ResolveOptionspassed toResolveAttesterLazy(viaselectAttester) has bothSigningConfigPathandUseTUFSigningConfigeffectively unset/false. The custom signing config the user asked for is never applied — signing falls back to whatever the zero-value resolution path does instead, contradicting the flag's documented behavior (lines 764-769: "Sign --attest bundles with a custom Sigstore signing config JSON").🐛 Proposed fix
func bundleOIDCResolveOptions(opts *bundleCmdOptions) attestation.ResolveOptions { return attestation.ResolveOptions{ Attest: opts.attest, IdentityToken: opts.identityToken, SigningKey: opts.signingKey, AmbientURL: os.Getenv("ACTIONS_ID_TOKEN_REQUEST_URL"), AmbientToken: os.Getenv("ACTIONS_ID_TOKEN_REQUEST_TOKEN"), DeviceFlow: opts.oidcDeviceFlow, FulcioURL: opts.fulcioURL, RekorURL: opts.rekorURL, + SigningConfigPath: opts.signingConfigPath, UseTUFSigningConfig: opts.useTUFSigningConfig, // Prompts (verification URL + user code) go to stderr so they don't // pollute stdout when callers redirect bundle output. PromptWriter: os.Stderr, } }🤖 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/cli/bundle.go` around lines 977 - 991, `bundleOIDCResolveOptions` is dropping the user-provided signing config, so `--signing-config` never reaches attestation resolution. Update the `attestation.ResolveOptions` construction in `bundleOIDCResolveOptions` to pass through `opts.signingConfigPath` as `SigningConfigPath` alongside `opts.useTUFSigningConfig`, matching the behavior in `recipe_sign_catalog.go` and ensuring `selectAttester`/`ResolveAttesterLazy` receives the custom Sigstore config when present.
🤖 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/actions/go-build-release/action.yml:
- Around line 78-98: The new Rekor v2 signing-config fetch in the release
workflow is a single network call with no retry, so transient TUF/CDN failures
can abort the whole release; update the Fetch Rekor v2 signing config step in
action.yml to wrap the existing go run ./cmd/aicr trust update
--emit-signing-config flow in a small retry loop like the other release hooks,
preserving the SLSA_PREDICATE gate and only setting AICR_SIGNING_CONFIG after a
successful fetch.
In `@docs/contributor/rekor-v2-signing.md`:
- Around line 142-149: The documentation currently applies the Cosign v3.0.1+
requirement too broadly; update the paragraph in the Rekor v2 signing docs and
the matching `aicr verify` docs so the minimum Cosign version is tied only to
the `cosign verify-blob-attestation` path. Keep `aicr verify` documented
separately as requiring only the `aicr` binary, and make sure the wording around
`aicr verify`, `cosign verify-blob-attestation`, and the v2 bundle verification
flow reflects that split.
In `@docs/user/cli-reference.md`:
- Around line 2835-2838: The fenced code blocks in the CLI reference section
need surrounding blank lines to satisfy markdownlint MD031. Update the markdown
around the Synopsis and Example blocks in the trust update section so each
fenced block is preceded by a blank line, keeping the formatting consistent in
the same docs section.
In `@pkg/bundler/attestation/kms.go`:
- Around line 44-58: NewKMSAttester has grown into a multi-parameter
constructor, so refactor it to use functional options instead of four positional
args. Update KMSAttester and NewKMSAttester to accept option functions for
rekorURL, signingConfigPath, and useTUFSigningConfig, while keeping keyURI
required and initializing identity via NewKMSIdentity(keyURI). Then update all
NewKMSAttester call sites to pass the corresponding options rather than
positional configuration values.
In `@pkg/bundler/attestation/signing_targeting_test.go`:
- Around line 26-78: Add a new subtest in TestTransparencyForOptions to verify
that UseTUFSigningConfig takes precedence over RekorURL in
transparencyForOptions. Set both fields on SignOptions, then assert the TUF path
is chosen rather than rekorPolicy, using the existing TUF cache-miss/error
behavior as the observable signal since a live fetch is not available. Use the
transparencyForOptions and SignOptions symbols to keep the test aligned with the
precedence contract already covered for SigningConfigPath.
In `@pkg/bundler/attestation/signing.go`:
- Around line 46-47: Update the `RekorURL` field comment in `signing.go` so it
explicitly says the value is ignored not only when `SigningConfigPath` is set,
but also when `UseTUFSigningConfig` is enabled. Keep the note aligned with
`transparencyForOptions` precedence so callers understand `RekorURL` is skipped
whenever either `SigningConfigPath` or `UseTUFSigningConfig` takes control.
In `@pkg/cli/bundle_rekor_v2_test.go`:
- Around line 28-95: The current tests only verify bundleCmdOptions, but they
miss the final attestation.ResolveOptions produced by bundleOIDCResolveOptions.
Update the Rekor v2 bundle tests to assert that bundleOIDCResolveOptions(opts)
carries through SigningConfigPath (and related fields as needed), especially in
the --signing-config subtest, so the wiring into ResolveAttesterLazy is covered
and regressions in the propagation from bundleCmdOptions are caught.
In `@pkg/trust/trust.go`:
- Around line 116-223: The signing config paths duplicate the same ForceCache
TUF client setup and use inconsistent fetch error messages. Extract the shared
client construction and target-fetch/classified error handling from
GetSigningConfig, SigningConfigJSON, and signingConfigFromClient into one helper
that both public functions call, so the Rekor v2 signing config lookup stays
consistent and the transport/verification-specific messages are preserved
everywhere.
---
Outside diff comments:
In `@pkg/cli/bundle.go`:
- Around line 977-991: `bundleOIDCResolveOptions` is dropping the user-provided
signing config, so `--signing-config` never reaches attestation resolution.
Update the `attestation.ResolveOptions` construction in
`bundleOIDCResolveOptions` to pass through `opts.signingConfigPath` as
`SigningConfigPath` alongside `opts.useTUFSigningConfig`, matching the behavior
in `recipe_sign_catalog.go` and ensuring `selectAttester`/`ResolveAttesterLazy`
receives the custom Sigstore config when present.
🪄 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: d4fe9834-da61-4742-86c1-de890914f552
📒 Files selected for processing (31)
.github/actions/go-build-release/action.yml.goreleaser.yamldocs/contributor/rekor-v2-signing.mddocs/index.ymldocs/integrator/supply-chain-verification.mddocs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_targeting_test.gopkg/bundler/attestation/signingconfig.gopkg/bundler/attestation/signingconfig_test.gopkg/bundler/attestation/testdata/signing_config_v1.jsonpkg/bundler/attestation/testdata/signing_config_v2.jsonpkg/bundler/attestation/testdata/signing_config_v2_no_tsa.jsonpkg/bundler/attestation/transparency.gopkg/cli/bundle.gopkg/cli/bundle_rekor_v2_test.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/cli/recipe_sign_catalog.gopkg/cli/trust.gopkg/cli/validate_evidence.gopkg/evidence/attestation/emit.gopkg/evidence/attestation/sign.gopkg/trust/trust.gopkg/trust/trust_test.gotests/chainsaw/signing/bundle-headless-oidc-ci/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
from
738ebe3 to
07d37ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/bundle.go (1)
977-991: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winThread
SigningConfigPathintoResolveOptions.--signing-configis parsed here, but never passed through, sobundle --attest --signing-config ...falls back to the default Rekor policy whenRekorURLis empty. AddSigningConfigPath: opts.signingConfigPathto this builder.🤖 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/cli/bundle.go` around lines 977 - 991, The bundleOIDCResolveOptions builder is not forwarding the parsed --signing-config value into attestation.ResolveOptions, so bundle attestation can ignore the intended signing config and fall back to default Rekor policy. Update the bundleOIDCResolveOptions function to pass opts.signingConfigPath through as SigningConfigPath alongside the existing fields, keeping the wiring consistent with the other OIDC resolve settings.
🤖 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/actions/generate-slsa-predicate/action.yml:
- Around line 55-67: The TUF-backed signing config fetch in the aicr
signing-config step is still a single un-retried network call, so make it
resilient by adding a small retry loop around the `go run ./cmd/aicr trust
update --emit-signing-config` invocation and by verifying the generated
`AICR_SIGNING_CONFIG` file is non-empty before exporting it. Keep the logic in
the same action step that sets `AICR_SIGNING_CONFIG`/uses `RUNNER_TEMP`, and
fail only after retries are exhausted or the file check fails.
In `@pkg/bundler/attestation/signingconfig.go`:
- Around line 76-133: selectSigningServices is over-failing on missing TSA
selection even for Rekor v1-only signing. Adjust the flow in
newSigningConfigPolicy/selectSigningServices so TSA lookup is not a hard
dependency unless the selected Rekor service uses API v2; keep returning an
empty tsa list when no TSA is needed, and only surface the “no usable timestamp
authority” error when v2 signing is actually in play.
---
Outside diff comments:
In `@pkg/cli/bundle.go`:
- Around line 977-991: The bundleOIDCResolveOptions builder is not forwarding
the parsed --signing-config value into attestation.ResolveOptions, so bundle
attestation can ignore the intended signing config and fall back to default
Rekor policy. Update the bundleOIDCResolveOptions function to pass
opts.signingConfigPath through as SigningConfigPath alongside the existing
fields, keeping the wiring consistent with the other OIDC resolve settings.
🪄 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: 093cd619-b946-41cb-bd1e-fded4d16b082
📒 Files selected for processing (32)
.github/actions/generate-slsa-predicate/action.yml.github/actions/go-build-release/action.yml.goreleaser.yamldocs/contributor/rekor-v2-signing.mddocs/index.ymldocs/integrator/supply-chain-verification.mddocs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_targeting_test.gopkg/bundler/attestation/signingconfig.gopkg/bundler/attestation/signingconfig_test.gopkg/bundler/attestation/testdata/signing_config_v1.jsonpkg/bundler/attestation/testdata/signing_config_v2.jsonpkg/bundler/attestation/testdata/signing_config_v2_no_tsa.jsonpkg/bundler/attestation/transparency.gopkg/cli/bundle.gopkg/cli/bundle_rekor_v2_test.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/cli/recipe_sign_catalog.gopkg/cli/trust.gopkg/cli/validate_evidence.gopkg/evidence/attestation/emit.gopkg/evidence/attestation/sign.gopkg/trust/trust.gopkg/trust/trust_test.gotests/chainsaw/signing/bundle-headless-oidc-ci/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
3 times, most recently
from
0921fdb to
0e24e56
Compare
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/bundler/attestation/kms.go`:
- Around line 44-50: Update the documentation on NewKMSAttester to reflect the
constructor’s zero-value behavior: it should not claim Rekor v2 is the default
for the library API when useTUFSigningConfig is false and no signingConfigPath
is provided. Adjust the comment near NewKMSAttester so it distinguishes the
CLI’s v2 default from the constructor’s v1 fallback policy, and make the same
wording change anywhere the duplicated doc appears (including the matching
section around KMSAttester behavior referenced by the review).
In `@pkg/bundler/attestation/signing_targeting_test.go`:
- Around line 26-91: The TestTransparencyForOptions coverage is repetitive and
should be converted to a table-driven test. Refactor the separate t.Run blocks
in TestTransparencyForOptions into a slice of test cases that captures the
different SignOptions inputs and the expected policy/assertions, then iterate
over them with one subtest loop. Keep the existing behaviors for
signingConfigPolicy precedence, UseTUFSigningConfig vs RekorURL, default
rekorPolicy, and explicit RekorURL using the same transparencyForOptions symbol.
In `@pkg/bundler/attestation/signingconfig.go`:
- Around line 119-139: The TSA selection path in selectSigningServices currently
swallows the SelectServices error entirely, which makes later v2 fail-closed
cases hard to diagnose. Update the TSA branch in selectSigningServices to
preserve the discarded error context at least in a debug-level log or equivalent
diagnostic path, while still returning the best-effort empty TSA set. Keep the
existing behavior for Rekor selection and the v2 enforcement in
newSigningConfigPolicy unchanged.
In `@pkg/trust/trust.go`:
- Around line 107-114: The signing config target constant in trust.go is
pointing at an unpublished TUF target, so cold-cache resolution will fail.
Update signingConfigTarget in the trust package to use the published
signing_config.v0.2.json target instead, and keep the surrounding resolution
logic in trust.go aligned with the existing signer flow that consumes this
target.
🪄 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: 3a3db91c-7c36-42ce-9ea7-1a492328de5c
📒 Files selected for processing (32)
.github/actions/generate-slsa-predicate/action.yml.github/actions/go-build-release/action.yml.goreleaser.yamldocs/contributor/rekor-v2-signing.mddocs/index.ymldocs/integrator/supply-chain-verification.mddocs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_targeting_test.gopkg/bundler/attestation/signingconfig.gopkg/bundler/attestation/signingconfig_test.gopkg/bundler/attestation/testdata/signing_config_v1.jsonpkg/bundler/attestation/testdata/signing_config_v2.jsonpkg/bundler/attestation/testdata/signing_config_v2_no_tsa.jsonpkg/bundler/attestation/transparency.gopkg/cli/bundle.gopkg/cli/bundle_rekor_v2_test.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/cli/recipe_sign_catalog.gopkg/cli/trust.gopkg/cli/validate_evidence.gopkg/evidence/attestation/emit.gopkg/evidence/attestation/sign.gopkg/trust/trust.gopkg/trust/trust_test.gotests/chainsaw/signing/bundle-headless-oidc-ci/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
2 times, most recently
from
8faaa05 to
103b131
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/build-attested.yaml:
- Around line 61-67: Extract the duplicated Cosign installation block into a
shared .github/actions/install-cosign/action.yml composite action accepting a
required cosign_version input, preserving the pinned installer, version-floor
rationale, and cosign-release mapping. Replace the identical Install Cosign
steps in the affected top-level workflows with calls to this action, passing
steps.versions.outputs.cosign, and retain the existing CLI E2E wrapper as
appropriate.
In `@pkg/bundler/attestation/signing.go`:
- Around line 135-163: Avoid starting separate Sigstore deadlines for TUF
resolution and signing. Create one timeout context in the caller before invoking
transparencyForOptions, pass that context through TUF resolution and
SignStatementWith, and remove the nested timeout from transparencyForOptions.
Apply the same shared-context change to KMSAttester.Attest so TUF loading and
signing share a single SigstoreSignTimeout window.
🪄 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: b8e73d08-3620-451e-9b5e-064aa196c55d
📒 Files selected for processing (43)
.github/actions/cli-e2e/action.yml.github/actions/generate-slsa-predicate/action.yml.github/actions/go-build-release/action.yml.github/actions/install-aicr-release/action.yml.github/actions/load-versions/action.yml.github/workflows/build-attested.yaml.github/workflows/kms-ministack-e2e.yaml.github/workflows/on-tag.yaml.github/workflows/qualification.yaml.github/workflows/sigstore-scaffolding-e2e.yaml.github/workflows/uat-aws.yaml.github/workflows/uat-gcp.yaml.goreleaser.yamldocs/contributor/rekor-v2-signing.mddocs/index.ymldocs/integrator/supply-chain-verification.mddocs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_targeting_test.gopkg/bundler/attestation/signingconfig.gopkg/bundler/attestation/signingconfig_test.gopkg/bundler/attestation/testdata/signing_config_v1.jsonpkg/bundler/attestation/testdata/signing_config_v2.jsonpkg/bundler/attestation/testdata/signing_config_v2_no_tsa.jsonpkg/bundler/attestation/transparency.gopkg/cli/bundle.gopkg/cli/bundle_rekor_v2_test.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/cli/recipe_sign_catalog.gopkg/cli/trust.gopkg/cli/validate_evidence.gopkg/evidence/attestation/emit.gopkg/evidence/attestation/sign.gopkg/trust/trust.gopkg/trust/trust_test.gotests/chainsaw/signing/bundle-attestation-private-sigstore/chainsaw-test.yamltests/chainsaw/signing/bundle-headless-oidc-ci/chainsaw-test.yaml
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
from
103b131 to
1d03d54
Compare
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Strong PR — the single transparencyForOptions selection shared by keyless and KMS, the SignOptionsFromResolve drift-guard mapper (with the test that caught the evidence-signs-v1 bug), the fail-closed v2-without-TSA rule, the fail-closed release hooks, and the CI assertion that the default actually lands in a v2 shard are all exactly the right shape, and library callers keep their v1 zero-value behavior. CI is fully green. Nothing blocks merge: one medium question on whether a stale TUF cache can silently downgrade end users to v1 after shard rotation (confirm + document, or warn on a v1 selection under the v2 default), one low UX item on the mutual-exclusivity error not naming its env-var sources, and one testdata nit.
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
from
1d03d54 to
741304f
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
🔎 Multi-Persona Review — Rekor v2 signing default
Method: 5 independent persona reviewers (Correctness · Supply-chain Security · Operability/CI-DX · Domain & Architecture · Docs/API-contract) → every finding adversarially re-derived from the resolved code + vendored sigstore-go by a senior meta-reviewer. Anchored to head 1d03d54.
Legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
This is a carefully built, well-tested supply-chain change. The core v2-targeting logic is correct, the v2-requires-TSA invariant fails closed and is thoroughly unit-tested, the CI wiring is complete (every outputs.cosign reference resolves in-job, ordering + Go availability correct, all install-aicr-release/cli-e2e callers pass the new cosign_version, cosign pinned v3.1.1 ≥ the v3.1.0 floor), and the shared SignOptionsFromResolve mapper genuinely closes the "evidence silently signs v1" drift it was introduced for.
No blocker or major finding survived adversarial verification. The headline candidate — a silent v1 downgrade (inline finding on signingconfig.go) — is real but externally-gated, low-probability, and fails safe (a valid signature in the wrong log, not a broken/forgeable one): a hardening gap, not a bug. Everything else is Minor/Nitpick.
Recommendation: Approve with comments. The two highest-value fast-follows are the v1-downgrade assertion and routing KMS through the shared mapper — worth doing, but neither need block merge.
Confirmed non-issues (examined, refuted or cleared)
- KMS
HasRekorEntry()nil-interface assertion — comma-ok on a nil interface returnsfalse, no panic; correctly reportstrue. ResolveSigningConfigcold-cache goroutine — buffered chan + select, identical to the existingUpdatepattern; no leak, TUF-integrity-verified fetch, errors propagate (no silent v1 fallback).selectSigningServicesswallowing the TSA error tonil— safe: yields an empty TSA set, and a v2 selection with empty TSA is caught by the downstream fail-closed gate.//nolint:unparamin resolver.go — still accurate (lazy path returns nil error on all branches).sbom-and-attestcosign "unpinned/orphaned" — refuted: it is called viaattest-image-from-tag(used inattest-images.yaml) and attests images via GitHub attestation, not the v2 signing-config path — out of scope.- Verify-time TSA trust — handled by the public-good trusted root; informational only, no defect.
- OpenAPI update for
--signing-config— not needed; signing is CLI/library-only, thePOST /v1/bundleschema never exposedrekorURL. recipe sign-catalogmissing--signing-configin cli-reference — correct; that command isHidden: true/ CI-only.
Summary
| 🔴 Blocker | 🟠 Major | 🟡 Minor | 🔵 Nitpick | Recommendation |
|---|---|---|---|---|
| 0 | 0 | 8 | 3 | Approve with comments |
Generated by a multi-persona review harness (parallel persona reviewers + adversarial meta-reviewer). Inline comments carry each finding's mechanism, blast radius, and suggested fix.
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
from
741304f to
6f42866
Compare
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Verified all three of my earlier comments are properly fixed in 6f42866: the warnOnV1Downgrade path (with the stale-cache window documented in rekor-v2-signing.md and the warn branch under test), the centralized signingTargetFromFlags with env-source-naming error message, and the testdata newline. @njhensley's threads and CodeRabbit's findings all check out in the diff too — the KMS-through-the-shared-mapper unification with the target equality test is a particularly solid close.
Two new findings, neither blocking: a fail-closed guard for the unenforced cosign_version composite-action input (real cross-PR risk — the in-flight uat-azure workflow already omits it), and a nit on the exclusivity message omitting the spec.bundle.rekorURL config source. CI is fully green; the branch is behind main and needs a rebase before merge.
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
2 times, most recently
from
d607476 to
aaa5792
Compare
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
from
6dab0b0 to
e54637a
Compare
AICR attestations (bundle --attest, recipe sign-catalog, and evidence signing) now record their transparency-log entry in Rekor v2 by default instead of Rekor v1, for both keyless OIDC and KMS-backed signing. v2 is tile-based and integrates witnessing; crucially it makes release-identity monitoring feasible, which the v1 firehose does not. The v2 signing config is fetched from Sigstore's TUF repository, so shard rotation is handled by Sigstore and nothing is hardcoded; a cold cache is fetched on demand. Rekor v2 emits no inline signed timestamp, so the config's timestamp authority is attached and a v2 selection with no TSA fails closed rather than emit unverifiable bundles. Opt out to Rekor v1 with --rekor-url (a private or public v1 endpoint) or supply a custom config with --signing-config. KMS signing (--signing-key) follows the same rules, signing to v2 by default with the same opt-outs. The v2 default is computed at the CLI layer, so library, server, and test callers keep their v1 zero-value behavior, and existing users who set --rekor-url or spec.bundle.rekorURL are unaffected. 'aicr trust update' now refreshes the trusted root and the v2 signing config, and --emit-signing-config writes it to a file for cosign. Releases sign to v2 via the goreleaser hooks, which fail closed if signing is enabled but the config is unset. Verifying v2 bundles requires cosign v3.0.1+; the verify commands are unchanged since a bundle self-describes its log. Related: #1650, #1149 Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
force-pushed
the
feat/rekor-v2-signing
branch
from
e54637a to
33088ed
Compare
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
All 35 review threads are resolved, and I verified the fixes are actually in the head (33088ed) rather than trusting the thread state: the silent-v1-downgrade concern (raised twice) is addressed by warnOnV1Downgrade wired through newSigningConfigPolicy with the bounded-window docs; the triplicated CLI guard is consolidated into signingTargetFromFlags (bundle + sign-catalog); KMS now flows through the shared SignOptionsFromResolve mapper with the test asserting field propagation; the --signing-config read is size-bounded via defaults.MaxSigningConfigBytes; the mutual-exclusivity error names all three sources (flags, env vars, spec.bundle.rekorURL); the cosign_version fail-closed guards landed in both actions plus the live uat-azure caller that was silently omitting it; the live-TUF test case is short-skipped; the chainsaw v2-shard origin assertion covers both invocations; and the docs are consistent on v3.0.1+ verify / v3.1.0+ signer floors with the unsourced v2.6.0 claim dropped. CI is 14/15 green with one check still running — merge gates on it as usual. Nothing outstanding.
Re-enable transparency-log identity monitoring for AICR's release signer on Rekor v2, replacing the v1 identity scan that timed out on every run and never completed a single scan. The v1 firehose scan ran ~50x slower than the log grew, so it could never keep up in a bounded CI job. Rekor v2 is tile-based (bulk 256-entry reads), so a single-worker scan outpaces the log and both the consistency proof and the identity scan complete in one cheap hourly job. This rides on release signing having moved to v2 (#1650 / #1679). - Add a resolve-v2-shard job that reads the current v2 shard from the same TUF signing config release signing uses (aicr trust update --emit-signing-config), so the shard URL is never hardcoded and yearly shard rotation needs no workflow change. - Keep the certIdentities identity config; the reusable MonitorLoop runs the consistency proof then the identity scan in one job. - Document the v2 rationale, automatic shard selection, and the one-time v1->v2 checkpoint reset in docs/contributor/maintaining.md. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
3 participants
Summary
AICR attestations now record their transparency-log entry in Rekor v2 by default instead of Rekor v1, for both keyless OIDC and KMS-backed signing, with opt-outs to v1 and a
cosign v3verification floor.Motivation / Context
Rekor v2 is Sigstore's tile-based transparency log. It matters here because it makes release-identity monitoring feasible: monitoring AICR's release signer on the Rekor v1 firehose is ~50x too slow to keep up (there is no index query for a keyless SAN identity), whereas v2's bulk tile reads make a single-worker scan keep up. This PR is the signing half of that move; the monitoring half rides on it.
The public-good Sigstore default still points other clients at Rekor v1, so AICR opts into v2 explicitly by consuming the Sigstore-maintained v2 signing config from TUF. Pre-1.0, adopting it early is a deliberate, low-cost choice.
Fixes: N/A
Related: #1650, #1149
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)pkg/bundler,pkg/component/*)docs/,examples/)pkg/trust,pkg/evidence,.goreleaser.yaml,.github/actionsImplementation Notes
pkg/trustfetches Sigstore'ssigning_config_rekor_v2target;aicr trust updaterefreshes it alongside the trusted root, signing resolves it from cache with a bounded network fallback on a cold cache, and shard rotation is handled by Sigstore. Selection uses sigstore-go'sroot.SelectServices(prefers highest API version, so v2 wins), matching cosign.ResolveOptionskeep their v1 zero-value behavior; only the signing commands flip. All signing paths (bundle, catalog, evidence, KMS) go through onetransparencyForOptionspolicy selection and oneSignOptionsFromResolvemapper, so a signing field cannot be dropped by one caller.--rekor-url(private/public v1) and--signing-config(custom) are mutually exclusive; existing users on--rekor-url/spec.bundle.rekorURLare unaffected. KMS follows the same rules.go-build-releaseaction runsaicr trust update --emit-signing-config(cosign cannot fetch the TUF v2 config itself) and feedscosign attest-blob --signing-config+aicr recipe sign-catalog --signing-config.Testing
Full
make qualifypasses:make test(-race, 0 failures), coverage 77.8% (floor 75%), lint, tuning-check, e2e (CLI chainsaw, 23/23), scan (no vulnerabilities). Proven end-to-end: a default keyless sign lands inlog2025-1.rekor.sigstore.dev(v2) with an RFC3161 TSA timestamp and verifies through sigstore-go v1.2.2 (AICR's pinned version). A code review (superpowers:code-reviewer) caught and fixed a bug where evidence signing still went to v1; a drift-guard test now covers the shared mapper.bundle-headless-oidc-ciasserts the default lands in a v2 shard.Risk Assessment
Rollout notes: Verifying v2 bundles requires Cosign v3.0.1+; the verify commands are unchanged (a bundle self-describes its log), and releases published before the cutover remain in Rekor v1 and verify with any recent Cosign.
aicr verifyhandles both. KMS+v2 is exercised by thekms-ministacke2e workflow but was not run locally (no KMS key). A private/local v2 e2e stack (rekor-tiles+timestamp-authority) is deferred and tracked on #1650.Checklist
make testwith-race)make lint)git commit -S)