feat(bundle): KMS-backed signing via --signing-key (#407)#1205
Merged
Conversation
Contributor
Coverage Report ✅
Coverage BadgeMerging this branch will increase 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/kms-backed-signing
branch
from
fd706e2 to
38f3bcb
Compare
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. 📝 WalkthroughWalkthroughThis PR adds KMS-backed signing support to bundle attestation as an alternative to keyless OIDC-based signing. It introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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
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)
1137-1143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
--attestprerequisite text to match KMS support.Line 1137 still states
--attestrequires OIDC, which conflicts with the new--signing-keyKMS path documented in Line 1143 and later sections. Please reword to “requires keyless OIDC or--signing-keyKMS signing material.”As per coding guidelines, documentation should clearly reflect current behavior and avoid ambiguity.
🤖 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/user/cli-reference.md` around lines 1137 - 1143, Update the `--attest` flag description to reflect that attestation can use either keyless OIDC or a KMS-backed key by changing the prerequisite text from "requires OIDC authentication" to something like "requires keyless OIDC or `--signing-key` KMS signing material"; ensure references to related flags (`--signing-key`, `--identity-token`, `--oidc-device-flow`, `--fulcio-url`, `--rekor-url`) remain consistent with the new wording so the doc no longer implies OIDC is the only option.
🤖 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/kmsidentity.go`:
- Around line 61-90: Wrap the remote KMS I/O in kmsIdentity.Keypair with a
bounded context using context.WithTimeout (use the default timeout from
pkg/defaults, e.g. defaults.DefaultIOTimeout), ensure you call cancel() with
defer, and use that derived ctx for both kms.Get(k.keyURI, ...) and for
sv.PublicKey(options.WithContext(...)); keep the existing error-wrapping logic
but replace uses of the incoming ctx for I/O with the new timed context to
prevent indefinite hangs.
In `@pkg/bundler/attestation/signing.go`:
- Around line 125-133: The call to id.Keypair(ctx) happens before creating the
bounded signCtx, so keypair resolution can block past
defaults.SigstoreSignTimeout; change the code to derive the timeout context
first (use context.WithTimeout(ctx, defaults.SigstoreSignTimeout) to create
signCtx and defer cancel) and pass signCtx into id.Keypair(...) so both key
resolution and subsequent Fulcio/Rekor calls honor the same deadline (keep
existing cancel defer and use the same signCtx for later operations).
- Around line 115-140: SignStatementWith currently dereferences the id and tlog
interfaces (calling id.Keypair, id.CertProvider and tlog.Logs) without guarding
for nil, which can cause a panic; add nil checks at the start of
SignStatementWith to validate id and tlog (and any values returned by
id.CertProvider() if applicable) and return a structured invalid-request error
(errors.New(errors.ErrCodeInvalidRequest, "...")) when they are nil instead of
proceeding to call Keypair, CertProvider or Logs. Ensure the checks reference
the same symbols (SignStatementWith, id, tlog, Keypair, CertProvider, Logs) so
the logic runs before any method invocation.
In `@pkg/cli/bundle.go`:
- Around line 348-350: The handler currently only uses cmd.IsSet(flagSigningKey)
to detect presence but doesn’t reject an explicitly provided empty signing key;
update the logic around flagSigningKey in the function that reads the flag (the
block using cmd.IsSet(flagSigningKey)) to also check the flag value (e.g., value
:= cmd.Flag(flagSigningKey).Value.String() or equivalent) and return a non-nil
error when the flag is set but the value is empty, instead of silently falling
back to keyless resolution; ensure the error message references the
--signing-key flag for clarity.
---
Outside diff comments:
In `@docs/user/cli-reference.md`:
- Around line 1137-1143: Update the `--attest` flag description to reflect that
attestation can use either keyless OIDC or a KMS-backed key by changing the
prerequisite text from "requires OIDC authentication" to something like
"requires keyless OIDC or `--signing-key` KMS signing material"; ensure
references to related flags (`--signing-key`, `--identity-token`,
`--oidc-device-flow`, `--fulcio-url`, `--rekor-url`) remain consistent with the
new wording so the doc no longer implies OIDC is the only option.
🪄 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: 15766973-4079-494b-9d2d-7028d22cf8a9
⛔ Files ignored due to path filters (277)
go.sumis excluded by!**/*.sumvendor/cloud.google.com/go/.gitignoreis excluded by!vendor/**vendor/cloud.google.com/go/.release-please-manifest-individual.jsonis excluded by!vendor/**vendor/cloud.google.com/go/.release-please-manifest-submodules.jsonis excluded by!vendor/**vendor/cloud.google.com/go/.release-please-manifest.jsonis excluded by!vendor/**vendor/cloud.google.com/go/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/CODE_OF_CONDUCT.mdis excluded by!vendor/**vendor/cloud.google.com/go/CONTRIBUTING.mdis excluded by!vendor/**vendor/cloud.google.com/go/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/RELEASING.mdis excluded by!vendor/**vendor/cloud.google.com/go/SECURITY.mdis excluded by!vendor/**vendor/cloud.google.com/go/auth/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/auth/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/auth/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/auth/auth.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/compute.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/detect.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/filetypes.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/aws_provider.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/executable_provider.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/externalaccount.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/file_provider.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/info.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/programmatic_provider.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/url_provider.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccount/x509_provider.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/externalaccountuser/externalaccountuser.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/gdch/gdch.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/impersonate/idtoken.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/impersonate/impersonate.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/internal/stsexchange/sts_exchange.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/credentials/selfsignedjwt.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/grpctransport/dial_socketopt.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/grpctransport/directpath.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/grpctransport/grpctransport.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/grpctransport/pool.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/httptransport/httptransport.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/httptransport/transport.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/compute/compute.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/compute/manufacturer.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/compute/manufacturer_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/compute/manufacturer_windows.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/credsfile/credsfile.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/credsfile/filetype.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/credsfile/parse.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/internal.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/jwt/jwt.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/retry/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/cba.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/default_cert.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/enterprise_cert.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/secureconnect_cert.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/cert/workload_cert.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/headers/headers.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/s2a.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/transport/transport.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/trustboundary/external_accounts_config_providers.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/trustboundary/trust_boundary.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/internal/version.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/oauth2adapt/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/auth/oauth2adapt/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/auth/oauth2adapt/oauth2adapt.gois excluded by!vendor/**vendor/cloud.google.com/go/auth/threelegged.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/log.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/metadata.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/retry_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_linux.gois excluded by!vendor/**vendor/cloud.google.com/go/compute/metadata/syscheck_windows.gois excluded by!vendor/**vendor/cloud.google.com/go/debug.mdis excluded by!vendor/**vendor/cloud.google.com/go/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/iam/.repo-metadata.jsonis excluded by!vendor/**vendor/cloud.google.com/go/iam/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/iam/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/iam/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/iam_policy.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/iam_policy_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/options.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/policy.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/iam/apiv1/iampb/resource_policy_member.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/iam/iam.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/.repo-metadata.jsonis excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/autokey_admin_client.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/autokey_client.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/auxiliary.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/auxiliary_go123.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/ekm_client.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/gapic_metadata.jsonis excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/helpers.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/hsm_management_client.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/iam.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/key_management_client.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/autokey.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/autokey_admin.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/autokey_admin_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/autokey_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/ekm_service.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/ekm_service_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/hsm_management.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/hsm_management_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/resources.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/service.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/kmspb/service_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/kms/apiv1/version.gois excluded by!vendor/**vendor/cloud.google.com/go/kms/internal/version.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/CHANGES.mdis excluded by!vendor/**vendor/cloud.google.com/go/longrunning/LICENSEis excluded by!vendor/**vendor/cloud.google.com/go/longrunning/README.mdis excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/.repo-metadata.jsonis excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/auxiliary.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/auxiliary_go123.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/doc.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/from_conn.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/gapic_metadata.jsonis excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/helpers.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/info.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/longrunningpb/operations.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/longrunning/autogen/longrunningpb/operations_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**vendor/cloud.google.com/go/longrunning/autogen/operations_client.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/autogen/version.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/internal/version.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/longrunning.gois excluded by!vendor/**vendor/cloud.google.com/go/longrunning/tidyfix.gois excluded by!vendor/**vendor/cloud.google.com/go/migration.mdis excluded by!vendor/**vendor/cloud.google.com/go/release-please-config-individual.jsonis excluded by!vendor/**vendor/cloud.google.com/go/release-please-config-yoshi-submodules.jsonis excluded by!vendor/**vendor/cloud.google.com/go/release-please-config.jsonis excluded by!vendor/**vendor/cloud.google.com/go/testing.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txtis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.ymlis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/poller.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/response.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_wasm.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/progress.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/to/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/to/to.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/constants.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/tracing.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/.gitignoreis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/BREAKING_CHANGES.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/CHANGELOG.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/LICENSE.txtis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/MIGRATION.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/README.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/TOKEN_CACHING.MDis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/TROUBLESHOOTING.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/assets.jsonis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/authentication_record.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azidentity.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_cli_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_developer_cli_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_pipelines_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_powershell_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/chained_token_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/ci.ymlis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/client_assertion_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/client_certificate_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/client_secret_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/confidential_client.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/default_azure_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/developer_credential_util.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/developer_credential_util_nonwindows.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/developer_credential_util_windows.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/device_code_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/environment_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/errors.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/interactive_browser_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/internal/cache.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/logging.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/managed-identity-matrix.jsonis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/managed_identity_client.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/managed_identity_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/on_behalf_of_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/public_client.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/test-resources-post.ps1is excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/test-resources-pre.ps1is excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/test-resources.bicepis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/username_password_credential.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/version.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/workload_identity.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txtis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/exported/exported.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/temporal/resource.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/doc.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/CHANGELOG.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/LICENSE.txtis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/MIGRATION.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/README.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/TROUBLESHOOTING.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/assets.jsonis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/build.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/ci.ymlis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/client.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/constants.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/custom_client.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/models.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/models_serde.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/options.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/platform-matrix.jsonis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/responses.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/test-resources-post.ps1is excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/test-resources.jsonis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/time_unix.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/tsp-location.yamlis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys/version.gois excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal/CHANGELOG.mdis excluded by!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/internal/LICENSE.txtis excluded by!vendor/**
📒 Files selected for processing (23)
.claude/skills/creating-slide-decks/skeleton.htmlcmd/gate/chainsaw-config.yamldocs/user/cli-reference.mdgo.modpkg/bundler/attestation/doc.gopkg/bundler/attestation/identity.gopkg/bundler/attestation/keylessidentity.gopkg/bundler/attestation/keylessidentity_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/kmsidentity.gopkg/bundler/attestation/kmsidentity_test.gopkg/bundler/attestation/kmskeypair.gopkg/bundler/attestation/kmskeypair_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_test.gopkg/bundler/attestation/transparency.gopkg/bundler/attestation/transparency_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.go
lockwobr
force-pushed
the
feat/kms-backed-signing
branch
from
38f3bcb to
09f273f
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 `@docs/user/cli-reference.md`:
- Line 2001: The attestation step list currently presents OIDC token acquisition
as unconditional; update the documentation around the five-step signing flow to
make the OIDC/Keyless step conditional: when using default keyless signing (no
--signing-key flag) include the OIDC token acquisition + Sigstore/Fulcio/Rekor
steps, and when --signing-key is supplied show an alternative KMS-backed signing
flow that omits OIDC acquisition and references the --signing-key flag and the
"KMS-Backed Signing" section; also adjust nearby cross-reference to `aicr
verify` to remain valid for both flows.
In `@pkg/bundler/attestation/kmskeypair.go`:
- Around line 110-136: Update classifyPublicKey to provide a clearer hint about
supported and future RSA sizes when rejecting keys: when rsa.PublicKey size
check fails in classifyPublicKey, change the returned error text (or add a debug
log before returning) to explicitly list supported size(s) ("only 2048-bit
supported") and mention common alternatives (e.g., "RSA-3072 and RSA-4096 are
not supported but may be considered in future versions") so users know why it
failed and that larger RSA sizes exist; similarly, make the ECDSA error message
from the *ecdsa.PublicKey case explicitly state "only P-256 supported" and
include a short hint to consult docs for supported key types/sizes using the
same error/logging path.
🪄 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: aa5038f3-68bc-4bdb-a344-f86fa4e4a7fa
📒 Files selected for processing (22)
.claude/skills/creating-slide-decks/skeleton.htmlcmd/gate/chainsaw-config.yamldocs/user/cli-reference.mdpkg/bundler/attestation/doc.gopkg/bundler/attestation/identity.gopkg/bundler/attestation/keylessidentity.gopkg/bundler/attestation/keylessidentity_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/kmsidentity.gopkg/bundler/attestation/kmsidentity_test.gopkg/bundler/attestation/kmskeypair.gopkg/bundler/attestation/kmskeypair_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_test.gopkg/bundler/attestation/transparency.gopkg/bundler/attestation/transparency_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.go
lockwobr
force-pushed
the
feat/kms-backed-signing
branch
from
09f273f to
1d4857f
Compare
mchmarny
reviewed
Jun 5, 2026
mchmarny
left a comment
mchmarny
left a comment
Member
There was a problem hiding this comment.
Clean refactor — the SigningIdentity × TransparencyPolicy split keeps keyless behaviorally unchanged while making KMS a single composition apart, and the error classification at kms.Get (InvalidRequest vs Unavailable) is right for the future server path. Fail-closed key-type validation (P-256 / RSA-2048 only) with explicit ed25519/RSA-4096 test coverage is solid.
A few non-blocking observations inline: two unrelated copyright-header additions worth splitting out, a dead _ string parameter on NewKeylessIdentity, per-Attest KMS RPC re-resolution worth caching before #1150, hardcoded HasRekorEntry() that #409 will want driven from tlog, and a Usage-string nit about verification still needing cosign verify --key. None block merge.
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/bundler/attestation/doc.go (1)
48-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPackage docs still overstate certificate presence for all signing modes.
These lines state every
.sigstore.jsonincludes a certificate, but KMS signing is documented above as key-based without Fulcio cert. Please make this wording mode-agnostic.Suggested wording
-// - Sigstore bundle (.sigstore.json) packaging the signed envelope, -// certificate, and Rekor inclusion proof +// - Sigstore bundle (.sigstore.json) packaging the signed envelope, +// verification material (Fulcio certificate for keyless, public-key +// material for KMS), and Rekor inclusion proofAs per coding guidelines, code comments must stay accurate and helpful.
🤖 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/bundler/attestation/doc.go` around lines 48 - 49, Package documentation in doc.go inaccurately claims every .sigstore.json contains a certificate; update the package comment in the attestation package to be mode-agnostic (e.g., say the Sigstore bundle may include a certificate, or a key identifier/proof depending on signing mode such as KMS key-only signing vs. Fulcio-issued certs) so it does not assert a certificate is always present; edit the descriptive bullet mentioning ".sigstore.json" to use language like "may include" or "contains a certificate or key identifier/proof depending on signing mode" to cover both KMS key-based and Fulcio certificate-based signing.
🤖 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/signing_test.go`:
- Around line 282-287: Update the stale test comment above
TestSignStatementWithRejectsNilStrategies to describe that the test validates
rejection of nil signing strategies (i.e., ensures the signer/composer returns
an error when given nil strategies), replacing the current key-only/no-tlog
description; locate the block of comment text immediately preceding the
TestSignStatementWithRejectsNilStrategies function and rewrite it to succinctly
state the test's purpose (nil strategy validation) and expected behavior.
In `@pkg/cli/bundle.go`:
- Around line 336-363: validateSigningKeyExclusivity currently only checks
cmd.IsSet(...) and misses flags supplied via config; update the exclusivity
checks to use the config-aware helpers instead of IsSet: for boolean flags call
boolFlagOrConfig(cmd, flagOIDCDeviceFlow) (and boolFlagOrConfig for
flagIdentityToken if applicable) and for string flags call
stringFlagOrConfig(cmd, flagFulcioURL) when deciding conflicts, keeping the same
error messages and the existing empty-signing-key rejection (use
stringFlagOrConfig(cmd, flagSigningKey) if signing-key can also come from
config), and return the same errors when any config-derived keyless option is
present so KMS signing is not silently bypassed.
---
Outside diff comments:
In `@pkg/bundler/attestation/doc.go`:
- Around line 48-49: Package documentation in doc.go inaccurately claims every
.sigstore.json contains a certificate; update the package comment in the
attestation package to be mode-agnostic (e.g., say the Sigstore bundle may
include a certificate, or a key identifier/proof depending on signing mode such
as KMS key-only signing vs. Fulcio-issued certs) so it does not assert a
certificate is always present; edit the descriptive bullet mentioning
".sigstore.json" to use language like "may include" or "contains a certificate
or key identifier/proof depending on signing mode" to cover both KMS key-based
and Fulcio certificate-based signing.
🪄 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: 96166589-808d-4a37-9201-73c3cb210a8d
📒 Files selected for processing (22)
.claude/skills/creating-slide-decks/skeleton.htmlcmd/gate/chainsaw-config.yamldocs/user/cli-reference.mdpkg/bundler/attestation/doc.gopkg/bundler/attestation/identity.gopkg/bundler/attestation/keylessidentity.gopkg/bundler/attestation/keylessidentity_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/kmsidentity.gopkg/bundler/attestation/kmsidentity_test.gopkg/bundler/attestation/kmskeypair.gopkg/bundler/attestation/kmskeypair_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_test.gopkg/bundler/attestation/transparency.gopkg/bundler/attestation/transparency_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.go
lockwobr
force-pushed
the
feat/kms-backed-signing
branch
from
1d4857f to
832c11f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/user/cli-reference.md`:
- Line 2071: Replace the sentence "HashiCorp Vault (`hashivault://`) is not
currently supported." with a clear, permanent-licensing-based statement that
Vault is intentionally excluded due to its MPL-2.0 client libraries (e.g.,
"HashiCorp Vault (`hashivault://`) is intentionally excluded for licensing
reasons — its client libraries are MPL-2.0 and therefore not supported"). Locate
the exact string "HashiCorp Vault (`hashivault://`) is not currently supported."
in the docs and update the wording to explicitly mention "intentionally
excluded" and "MPL-2.0" so readers understand this is a deliberate, permanent
choice rather than a temporary limitation.
In `@pkg/bundler/attestation/kms.go`:
- Around line 53-56: Replace the unconditional errors.Wrap call so that upstream
structured error codes from BuildStatement are preserved: use
errors.PropagateOrWrap(err, errors.ErrCodeInternal, "failed to build attestation
statement") when handling the error returned by BuildStatement(subject,
metadata) (the variable statementJSON and the BuildStatement call are the places
to modify) so existing coded errors propagate while still adding the contextual
message.
In `@pkg/bundler/attestation/kmskeypair.go`:
- Around line 101-108: The SignData method in kmsKeypair currently wraps errors
from k.signer.SignDigest with errors.Wrap causing double-wrapping; update
kmsKeypair.SignData to use errors.PropagateOrWrap (or the package's equivalent)
when handling err returned by k.signer.SignDigest so that if the signer already
set an error code (ErrCodeUnavailable) it is propagated, otherwise the error is
wrapped with the "KMS sign failed" message; keep the same return values (sig,
digest[:], error) and only change the error handling in the SignData function.
In `@pkg/bundler/attestation/resolver_test.go`:
- Around line 149-178: Update the two tests to assert precedence by including
keyless OIDC fields alongside SigningKey: in TestResolveAttesterKMS and
TestResolveAttesterLazyKMS call ResolveAttester / ResolveAttesterLazy with
ResolveOptions that sets SigningKey to "awskms://..." and also sets one or more
keyless fields (e.g., OIDCIssuer and/or OIDCClientID) and then assert the
returned attester is still a *KMSAttester; this pins the conflict case and
ensures SigningKey takes precedence over keyless OIDC options.
In `@pkg/bundler/attestation/transparency_test.go`:
- Around line 19-40: Update the TestRekorPolicyLogs test to explicitly assert
the empty-URL fallback: when calling NewRekorPolicy("") inspect the attached log
client returned by p.Logs() (e.g., first element) and assert that its configured
base URL equals defaults.SigstoreRekorURL (or its string value), rather than
only checking the count; this ensures NewRekorPolicy sets the default Rekor URL
when given an empty string.
🪄 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: e2e65a84-6acb-4817-ba18-00c78a7c9d3f
📒 Files selected for processing (20)
docs/user/cli-reference.mdpkg/bundler/attestation/doc.gopkg/bundler/attestation/identity.gopkg/bundler/attestation/keylessidentity.gopkg/bundler/attestation/keylessidentity_test.gopkg/bundler/attestation/kms.gopkg/bundler/attestation/kms_test.gopkg/bundler/attestation/kmsidentity.gopkg/bundler/attestation/kmsidentity_test.gopkg/bundler/attestation/kmskeypair.gopkg/bundler/attestation/kmskeypair_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/attestation/signing_test.gopkg/bundler/attestation/transparency.gopkg/bundler/attestation/transparency_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.go
2 tasks
lockwobr
force-pushed
the
feat/kms-backed-signing
branch
2 times, most recently
from
c8e2f5a to
9af2cb1
Compare
Add `aicr bundle --attest --signing-key <kms-uri>` to sign bundles with a
cloud-KMS-backed key instead of keyless OIDC, for CI/CD environments
without an OIDC token (Jenkins, internal pipelines).
Signing is decomposed into two composable axes consumed by a new
SignStatementWith primitive:
- SigningIdentity: ephemeral key + Fulcio (keyless) or a KMS key with
no certificate (kmsIdentity).
- TransparencyPolicy: Rekor log, or none (reserved for #409).
SignStatement (keyless) now delegates to SignStatementWith with no
behavioral change; KMSAttester pairs a KMS identity with Rekor.
KMS-signed bundles carry public-key verification material (no Fulcio
cert) and upload to Rekor by default. Supported schemes: awskms://,
gcpkms://, azurekms://. HashiCorp Vault (hashivault://) is excluded
because its client libraries are MPL-2.0, which the license policy
disallows.
--signing-key is mutually exclusive with --identity-token,
--oidc-device-flow, and --fulcio-url; --rekor-url may be combined with it
to log to a private Rekor.
Verification today is via cosign verify with --key <kms-uri>; native
aicr verify --key is tracked in #1152.
Note: drops a debug-level "signing in-toto statement" log line whose
Fulcio/Rekor URLs are now encapsulated in the identity/policy objects.
lockwobr
force-pushed
the
feat/kms-backed-signing
branch
from
9af2cb1 to
6f124ad
Compare
mchmarny
approved these changes
Jun 8, 2026
mchmarny
left a comment
mchmarny
left a comment
Member
There was a problem hiding this comment.
Full review (CodeRabbit was down). Walked through every changed pkg/bundler/attestation/*.go, pkg/cli/bundle.go, docs/user/cli-reference.md, and re-verified each open review thread against the head SHA.
All 7 outstanding comments are addressed in the code, even though the threads weren't marked resolved on GitHub:
| Thread | Verdict |
|---|---|
| Vault exclusion wording (CodeRabbit) | ✅ cli-reference.md:2147 now reads "deliberate, ongoing exclusion… MPL-2.0" |
PropagateOrWrap for BuildStatement (CodeRabbit) |
✅ kms.go:56 |
PropagateOrWrap on SignData (CodeRabbit) |
✅ kmskeypair.go:101 |
Precedence test pins SigningKey over keyless (CodeRabbit) |
✅ resolver_test.go — both tests now include IdentityToken:"should-be-ignored" + DeviceFlow:true |
| Empty-URL fallback assertion (CodeRabbit) | ✅ transparency_test.go — white-box assert on rekorPolicy.url == defaults.SigstoreRekorURL |
HasRekorEntry driven by tlog policy (my own #409 nit) |
✅ kms.go:76 — _, noTLog := k.tlog.(noTLogPolicy); return !noTLog |
cosign verify --key hint in --signing-key Usage (my own nit) |
✅ bundle.go:702 |
Resolving the 6 stale threads since the concerns are addressed.
One thing worth calling out beyond the comments: the new extractIssuerExtension in signing.go properly ASN.1-decodes the current Fulcio OID 1.3.6.1.4.1.57264.1.8 as a UTF8String (with trailing-byte rejection) and falls back to the legacy OID as raw bytes. The previous code treated both as raw bytes, which would have silently embedded the ASN.1 tag/length prefix in the issuer string for any Fulcio cert using the current OID. Nice quiet fix.
validateSigningKeyExclusivity correctly fixes the config-bypass concern from the earlier CodeRabbit comment — checks both cmd.IsSet (for empty-value fail-fast) and the resolved opts (catching config-sourced keyless flags). The --rekor-url deliberate omission is well-justified in the comment (enterprise "KMS key + private Rekor").
CI: all required checks green except tests / E2E still in_progress; CodeQL conclusion is neutral (informational, not blocking).
Approving. Architecture (composable SigningIdentity × TransparencyPolicy) sets up #409 and #1150 cleanly; failure-mode classification (ProviderNotFoundError → ErrCodeInvalidRequest vs. init failures → ErrCodeUnavailable) is exactly what the future server path needs.
lockwobr
added a commit
that referenced
this pull request
Jun 10, 2026
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
lockwobr
added a commit
that referenced
this pull request
Jun 10, 2026
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
lockwobr
added a commit
that referenced
this pull request
Jun 10, 2026
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
lockwobr
added a commit
that referenced
this pull request
Jun 10, 2026
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
lockwobr
added a commit
that referenced
this pull request
Jun 11, 2026
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
lockwobr
added a commit
that referenced
this pull request
Jun 11, 2026
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
lockwobr
added a commit
that referenced
this pull request
Jun 11, 2026
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
aicr bundle --attest --signing-key <kms-uri>to sign bundles with a cloud-KMS-backed key (AWS / GCP / Azure KMS) instead of keyless OIDC, for CI/CD environments that have no OIDC token.Motivation / Context
CI environments without OIDC (Jenkins, TeamCity, internal pipelines) cannot attest bundles via keyless Fulcio signing. This adds a key-based signing path and is the foundation issue of the closed-supply-chain epic: it unblocks air-gapped signing (#409), server-side
/v1/bundlesigning (#1150), and KMS verification (#1152).Fixes: #407
Related: #1149 (epic), #409, #1150, #1152
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
Signing is decomposed into two orthogonal, composable axes consumed by a new
SignStatementWithprimitive inpkg/bundler/attestation:SigningIdentity(identity.go): supplies the signing keypair and, for keyless, the Fulcio certificate provider.keylessIdentityuses an ephemeral key + Fulcio;kmsIdentityuses a KMS key and returns no certificate.TransparencyPolicy(transparency.go): selects the Rekor transparency log, or none (the no-tlog policy is reserved for offline signing, feat(bundle): air-gapped signing with --tlog-upload=false for restricted networks #409).SignStatement(keyless) now delegates toSignStatementWithwith no behavioral change;KMSAttesterpairs a KMS identity with Rekor. Key points:sigstore/sigstoreKMSSignerVerifierto sigstore-go'ssign.Keypair; signing passes a precomputed SHA-256 digest viaoptions.WithDigestso there is no double-hashing (the signature verifies oversha256(data)).awskms://,gcpkms://,azurekms://. HashiCorp Vault (hashivault://) is intentionally excluded because its client libraries are MPL-2.0, which the project license policy disallows. (Vault/OpenBao can be supported later via sigstore's out-of-process KMS plugin, which keeps the binary MPL-free.)--signing-keyis mutually exclusive with--identity-token,--oidc-device-flow, and--fulcio-url;--rekor-urlmay be combined with it (KMS + private Rekor).kms.Getfailures are classified: unknown scheme (kms.ProviderNotFoundError) →ErrCodeInvalidRequest; provider init/auth/network failure →ErrCodeUnavailable(matters for the future server path, feat(server): bundle attestation for /v1/bundle (non-interactive signing) #1150).Verification today is via
cosign verify ... --key <kms-uri>; nativeaicr verify --keyis tracked in #1152. The new sigstore KMS provider dependencies are vendored in a separate commit.Testing
make qualifypasses (test-race, lint, e2e, scan, repo checks; 0 failed tests). New tests are fully offline (local ECDSA signer + no-tlog policy; no network/cloud/cluster):pkg/bundler/attestation: 78.1% coverage; every new exported function/method covered (the key-only/no-tlog path is asserted end-to-end to produce public-key verification material with no tlog entries;classifyPublicKeyfails closed on non-P256 / non-2048 keys).make test-coverage: 75.9% vs the 75% floor.make scan: no new findings from the KMS provider deps. (Grype reports two pre-existingk8s.io/kubernetesadvisories that arrived via the rebase ontomain, unrelated to this change.)Risk Assessment
Rollout notes: Additive and opt-in behind a new
--signing-keyflag. The keyless signing path is unchanged (it now delegates to the shared composer with identical behavior). No migration required.Checklist
make testwith-race)make lint)git commit -S)