Skip to content

Require new bundle format for signing and verification - #4959

Open
aaronlew02 wants to merge 12 commits into
sigstore:mainfrom
aaronlew02:new-bundle-sc
Open

Require new bundle format for signing and verification#4959
aaronlew02 wants to merge 12 commits into
sigstore:mainfrom
aaronlew02:new-bundle-sc

Conversation

@aaronlew02

@aaronlew02 aaronlew02 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

This change mandates the new bundle format for signing and verification.

For verification, this enables the exclusive use of sigstore-go and requires a trusted root. For signing, this disables individual service inputs (i.e., outside of a signing config) and detached outputs.

Release Note

  • Signers requiring services MUST use a signing config (either from TUF via --use-signing-config [true by default] or explicitly provided via --signing-config).

    • This applies to ALL keyless signers.
    • This applies to key-based signers requesting a certificate, RFC 3161 timestamp, and/or transparency log entry.
  • Signers NOT requiring services MUST set --use-signing-config=false.

    • This applies to offline key-based signers.
  • Verifiers validating service-provided verification material MUST use a trusted root (either fetched from TUF automatically or explicitly provided via --trusted-root).

    • This applies to ALL keyless verifiers.
    • This applies to key-based verifiers with a certificate, RFC 3161 timestamp, and/or transparency log entry.
  • Several flags have been removed:

    • Service configuration flags obsoleted by the signing config and trusted root:
      • --rekor-url (for signing, attestation, and verification only), --fulcio-url, --oidc-issuer, --timestamp-server-url, --timestamp-certificate-chain, --ca-roots, --ca-intermediates, --certificate-chain, --certificate, --tlog-upload, --offline
    • Flags obsoleted by the new bundle format:
      • --new-bundle-format
      • --rekor-entry-type
      • Input overrides: --record-creation-timestamp, --signature/--signature-path, --payload, --signature-digest-algorithm
      • Detached outputs: --sct, --rfc3161-timestamp/--rfc3161-timestamp-bundle, --b64, --output/--output-signature/--output-payload/--output-attestation/--output-certificate
    • Flags obsoleted by the shift to OCI 1.1 Referrers:
      • --replace, --attachment, --experimental-oci11, --sign-container-identity
    • Protocol and verification flow adjustments:
      • --insecure-skip-verify (SCT verification is no longer performed during signing)
      • --private-infrastructure (Obsoleted in favor of --insecure-ignore-tlog)
  • Service configuration environment variables are no longer used in production (but remain used in testing):

    • SIGSTORE_ROOT_FILE, SIGSTORE_REKOR_PUBLIC_KEY, SIGSTORE_CT_LOG_PUBLIC_KEY_FILE, SIGSTORE_TSA_CERTIFICATE_FILE
  • ALL signers using a signing config containing at least one transparency log must agree to the immutable record privacy statement.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 34.24125% with 169 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.67%. Comparing base (2ef6022) to head (6c0cc72).
⚠️ Report is 821 commits behind head on main.

Files with missing lines Patch % Lines
cmd/cosign/cli/attest.go 9.09% 20 Missing ⚠️
cmd/cosign/cli/attest_blob.go 23.07% 20 Missing ⚠️
cmd/cosign/cli/signcommon/common.go 0.00% 17 Missing ⚠️
cmd/cosign/cli/verify/verify_blob_attestation.go 63.15% 7 Missing and 7 partials ⚠️
cmd/cosign/cli/signblob.go 38.09% 13 Missing ⚠️
cmd/cosign/cli/attest/attest.go 0.00% 12 Missing ⚠️
cmd/cosign/cli/sign.go 7.69% 12 Missing ⚠️
cmd/cosign/cli/verify.go 0.00% 12 Missing ⚠️
cmd/cosign/cli/verify/verify.go 28.57% 10 Missing ⚠️
cmd/cosign/cli/sign/sign.go 0.00% 9 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4959      +/-   ##
==========================================
- Coverage   40.10%   39.67%   -0.43%     
==========================================
  Files         155      206      +51     
  Lines       10044    11382    +1338     
==========================================
+ Hits         4028     4516     +488     
- Misses       5530     6281     +751     
- Partials      486      585      +99     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aaronlew02
aaronlew02 force-pushed the new-bundle-sc branch 5 times, most recently from bf1189d to e6262f8 Compare June 18, 2026 22:10
@aaronlew02 aaronlew02 changed the title Require new bundle format and signing config Require new bundle format for signing and verification Jun 22, 2026
@aaronlew02
aaronlew02 force-pushed the new-bundle-sc branch 15 times, most recently from 45cff47 to d4a7237 Compare June 29, 2026 20:46
@aaronlew02
aaronlew02 force-pushed the new-bundle-sc branch 4 times, most recently from 562178c to f5bd6ed Compare July 6, 2026 14:39
@aaronlew02

Copy link
Copy Markdown
Contributor Author

Testing

The following commands can be used to test the changes in this PR.

Setup

Before running the tests, ensure that you have generated a local test keypair (which is used for key-based tests):

# Generate a keypair (writes to cosign.key and cosign.pub)
cosign generate-key-pair

1. TUF-provided Signing Config & Trusted Root

Verify that a TUF-provided signing config and trusted root are resolved correctly for keyless operations and key-based, certificate-backed operations:

Keyless Operations

Image signing & verification

# Sign using TUF signing config
cosign sign --use-signing-config=true [IMAGE_URI]

# Verify using TUF trusted root
cosign verify --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [IMAGE_URI]

Blob signing & verification

# Sign a blob to generate a bundle
cosign sign-blob --use-signing-config=true --bundle blob.bundle [BLOB_FILE]

# Verify the blob using the bundle and TUF trusted root
cosign verify-blob --bundle blob.bundle --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [BLOB_FILE]

Image attestation & verification

# Attest the image
cosign attest --use-signing-config=true --predicate [PREDICATE_FILE] [IMAGE_URI]

# Verify the attestation
cosign verify-attestation --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [IMAGE_URI]

Blob attestation & verification

# Attest a blob to generate a bundle
cosign attest-blob --use-signing-config=true --predicate [PREDICATE_FILE] --bundle attest.bundle [BLOB_FILE]

# Verify the blob attestation
cosign verify-blob-attestation --bundle attest.bundle --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [BLOB_FILE]

Key-based / Certificate-backed Operations

Key-based, certificate-backed signing & verification

# Sign using local key and request Fulcio certificate using TUF signing config
cosign sign --key cosign.key --issue-certificate --use-signing-config=true [IMAGE_URI]

# Verify using TUF trusted root and identity checks
cosign verify --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [IMAGE_URI]

Key-based signing with transparency log and RFC 3161 timestamp

# Sign a blob using key and request log entry/timestamp
cosign sign-blob --key cosign.key --use-signing-config=true --bundle blob.bundle [BLOB_FILE]

# Verify the blob bundle (verifying the signature, log proof, and timestamp against TUF trusted root)
cosign verify-blob --key cosign.pub --bundle blob.bundle [BLOB_FILE]

2. Explicit Local Signing Config & Trusted Root

Verify that explicitly provided local config and trusted root files are loaded and utilized correctly:

Keyless Operations

Image signing & verification

# Sign using a local signing config file
cosign sign --signing-config [PATH_TO_SIGNING_CONFIG] [IMAGE_URI]

# Verify using an explicit trusted root file
cosign verify --trusted-root [PATH_TO_TRUSTED_ROOT] --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [IMAGE_URI]

Blob signing & verification

# Sign a blob using a local signing config file
cosign sign-blob --signing-config [PATH_TO_SIGNING_CONFIG] --bundle blob.bundle [BLOB_FILE]

# Verify the blob using an explicit trusted root file
cosign verify-blob --trusted-root [PATH_TO_TRUSTED_ROOT] --bundle blob.bundle --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [BLOB_FILE]

Image attestation & verification

# Attest the image using a local signing config file
cosign attest --signing-config [PATH_TO_SIGNING_CONFIG] --predicate [PREDICATE_FILE] [IMAGE_URI]

# Verify the attestation using an explicit trusted root file
cosign verify-attestation --trusted-root [PATH_TO_TRUSTED_ROOT] --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [IMAGE_URI]

Blob attestation & verification

# Attest a blob using a local signing config file
cosign attest-blob --signing-config [PATH_TO_SIGNING_CONFIG] --predicate [PREDICATE_FILE] --bundle attest.bundle [BLOB_FILE]

# Verify the blob attestation using an explicit trusted root file
cosign verify-blob-attestation --trusted-root [PATH_TO_TRUSTED_ROOT] --bundle attest.bundle --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [BLOB_FILE]

Key-based Operations

Key-based, certificate-backed signing & verification

# Sign using local key and request Fulcio certificate using local signing config
cosign sign --key cosign.key --issue-certificate --signing-config [PATH_TO_SIGNING_CONFIG] [IMAGE_URI]

# Verify using local trusted root file and identity checks
cosign verify --trusted-root [PATH_TO_TRUSTED_ROOT] --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [IMAGE_URI]

Key-based signing with transparency log and RFC 3161 timestamp

# Sign a blob using key and request log entry/timestamp via local signing config
cosign sign-blob --key cosign.key --signing-config [PATH_TO_SIGNING_CONFIG] --bundle blob.bundle [BLOB_FILE]

# Verify the blob bundle against local trusted root file
cosign verify-blob --key cosign.pub --trusted-root [PATH_TO_TRUSTED_ROOT] --bundle blob.bundle [BLOB_FILE]

Service Omission Tests (Config-driven behavior)

Verify that omitting specific services in the signing config successfully skips their use during signing:

Omitting Rekor (Skips log upload)

# 1. Generate a signing config file without Rekor configured
cosign signing-config create --with-default-services --no-default-rekor --out signing-config-no-rekor.json

# 2. Sign a blob (succeeds without attempting log upload; generated bundle will not contain a log entry)
cosign sign-blob --signing-config signing-config-no-rekor.json --bundle blob-no-rekor.bundle [BLOB_FILE]

# 3. Verify the bundle (should FAIL by default due to missing log entry/SET proof)
cosign verify-blob --trusted-root [PATH_TO_TRUSTED_ROOT] --bundle blob-no-rekor.bundle --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [BLOB_FILE]

# 4. Verify the bundle with tlog bypassed (should SUCCEED)
cosign verify-blob --trusted-root [PATH_TO_TRUSTED_ROOT] --bundle blob-no-rekor.bundle --insecure-ignore-tlog --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [BLOB_FILE]

Omitting TSA (Skips RFC 3161 timestamping)

# 1. Generate a signing config file without TSA configured
cosign signing-config create --with-default-services --no-default-tsa --out signing-config-no-tsa.json

# 2. Sign a blob (succeeds without requesting timestamp; generated bundle will not contain a timestamp)
cosign sign-blob --signing-config signing-config-no-tsa.json --bundle blob-no-tsa.bundle [BLOB_FILE]

# 3. Verify the bundle (should SUCCEED; falls back to using the tlog integrated time to verify certificate expiry)
cosign verify-blob --trusted-root [PATH_TO_TRUSTED_ROOT] --bundle blob-no-tsa.bundle --certificate-identity [IDENTITY] --certificate-oidc-issuer [ISSUER] [BLOB_FILE]

3. Simple Key-based Signing & Verification (No services)

Verify that simple key-based operations (which do not interact with Fulcio, Rekor, or a timestamp authority) work cleanly:

# Sign an image using a private key (no signing config or OCI upload)
cosign sign --key cosign.key [IMAGE_URI]

# Verify the image using the public key
cosign verify --key cosign.pub [IMAGE_URI]

# Sign a blob using a private key (generates a simple bundle without log/timestamp verification material)
cosign sign-blob --key cosign.key --bundle blob.bundle [BLOB_FILE]

# Verify the blob using the public key and bundle
cosign verify-blob --key cosign.pub --bundle blob.bundle [BLOB_FILE]

4. CLI Flag Validations (Negative Testing)

Verify that the CLI rejects removed flags and enforces mandatory new arguments:

# 1. Rejects removed flags (should fail with unknown flag errors)
cosign sign --rekor-url [URL] [IMAGE_URI]
cosign sign --timestamp-server-url [URL] [IMAGE_URI]
cosign verify --rekor-url [URL] [IMAGE_URI]
cosign verify --signature [SIG_PATH] --payload [PAYLOAD_PATH] [IMAGE_URI]
cosign verify-blob --signature [SIG_PATH] [BLOB_FILE]

# 2. Enforces signing configuration for keyless and certificate-based signing/attestation (should fail with specific keyless or certificate-based error messages)
cosign sign --use-signing-config=false [IMAGE_URI]
cosign sign --key cosign.key --issue-certificate --use-signing-config=false [IMAGE_URI]
cosign sign-blob --use-signing-config=false --bundle blob.bundle [BLOB_FILE]
cosign attest --use-signing-config=false [IMAGE_URI]
cosign attest-blob --use-signing-config=false --bundle attest.bundle [BLOB_FILE]

# 3. Enforces bundle for blob signing and attestation (should fail with error requiring bundle)
cosign sign-blob [BLOB_FILE]
cosign attest-blob [BLOB_FILE]

Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
Signed-off-by: Aaron Lew <64337293+aaronlew02@users.noreply.github.com>
}

bundleBytes, _, _, err := signcommon.NewAttestationBundle(ctx, c.KeyOpts, c.CertPath, c.CertChainPath, bundleOpts, c.SigningConfig, c.TrustedMaterial)
_, err = signcommon.ShouldUploadToTlog(ctx, c.KeyOpts, nil, len(c.SigningConfig.RekorLogURLs()) > 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is throwing away the result of ShouldUploadToTlog, so the attestation would be unconditionally uploaded to the transparency log.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether the attestation would be uploaded to the transparency log depends on:

  • The value of len(c.SigningConfig.RekorLogURLs()) > 0, which if true would enable the upload confirmation prompt.
  • Whether the user accepts the prompt. If they don't, execution stops with an error. If they do, the signing config goes to sigstore-go, which would upload because a log is present.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so this is just about presenting the prompt? I see we do the same in sign/attest but override what's in the signing config, what's the reason for that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's to support users who want to sign a private container image without uploading to the transparency log.

If a user uses a signing config with a Rekor URL, but a private repository is detected:

  1. The user will be prompted to confirm that they wish to upload to Rekor despite the repository being private. If the user disagrees, shouldUpload returns false, and execution proceeds without a Rekor upload. If the user agrees...
  2. The user will receive the Immutable Records confirmation prompt. If the user disagrees, execution stops with an error. If the user agrees, shouldUpload returns true and the signing config goes to sigstore-go, which would upload because a log is present.

This matches current functionality.

Offline bool
UseSignedTimestamps bool
IgnoreTlog bool
HashAlgorithm crypto.Hash

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm, hash algorithm will now only be determined by the default mapping between signing scheme and hash alg?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, how does this interact with when you parse out the algorithm from the payloadDigest?

}

sig, err := base64signature(c.SigRef, c.BundlePath)
_, err = cosign.VerifyNewBundle(ctx, co, artifactPolicyOption, bundle)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a later PR - we should implement the change mentioned in

func VerifyNewBundle(_ context.Context, co *CheckOpts, artifactPolicyOption verify.ArtifactPolicyOption, bundle verify.SignedEntity) (*verify.VerificationResult, error) {
// Copy co so rekorV2Bundle's UseSignedTimestamps write stays per-call and
// doesn't race a *CheckOpts shared across goroutines.
// TODO(cody)(cosign v4): Consider changing function signature to take a
// non-pointer CheckOpts and avoid this copy.
. We should move the function into an internal package as well when we do. I also think we could drop the first return value since no caller is using it.

var shouldUpload bool
var err error

if ko.SigningConfig == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what case will this be nil? Currently, you can either provide a signing config, or specify --use-signing-config to fetch it from TUF. If we're removing that latter flag, then the signing config would always populated, either by TUF or with one explicitly provided (which could be empty).

(this might be answered in a later commit, feel free to point to that if so)

decodedSig, err := base64.StdEncoding.DecodeString(string(sigBytes))
if err != nil {
t.Fatalf("failed to decode base64 signature: %v", err)
var b1 struct {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, a better var name would help readability, even if this is just an inlined struct

Signature string `json:"signature"`
} `json:"messageSignature"`
}
bytes1, _ := os.ReadFile(bundlePath)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though it's unnecessary, i'd check the second return val and fail fatally if not nil

Use: "sign-blob",
Short: "Sign the supplied blob, outputting the base64-encoded signature to stdout",
Example: ` cosign sign-blob --key <key path>|<kms uri> <blob>
Short: "Sign the supplied blob, outputting the bundle to a file",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bundle -> "signature bundle"


# sign a blob with a local key pair file
cosign sign-blob --key cosign.key <FILE>
cosign sign-blob --key cosign.key --bundle <bundle.json> <FILE>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're here, can you also add an example of calling sign-blob without a key? i think this was a side effect of keys being the default back in v1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also in the example, we should denote --key is optional

return fmt.Errorf("please specify --bundle")
}

var signType string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of signType?

OIDCClientSecret: oidcClientSecret,
OIDCRedirectURL: o.OIDC.RedirectURL,
OIDCDisableProviders: o.OIDC.DisableAmbientProviders,
OIDCProvider: o.OIDC.Provider,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this missing before? if so, can we split out that change?


for _, blob := range args {
// TODO: remove when the output flag has been deprecated
if o.Output != "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we removing this flag in another commit/PR?

@Hayden-IO Hayden-IO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Hayden-IO Hayden-IO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments left on the sign-blob commit are relevent here as well, but otherwise lgtm

PredicateType: o.Predicate.Type,
PredicatePath: o.Predicate.Path,
StatementPath: o.Predicate.Statement,
Timeout: ro.Timeout,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For another change, should we pass through timeout for sign/sign-blob as well?

@Hayden-IO Hayden-IO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, lgtm

// provided in an attached bundle or OCI annotation. LoadVerifierFromKeyOrCert must be called
// after initializing trust material in order to verify certificate chain.
// User provides a key. Otherwise, verification requires a Fulcio certificate
// provided in an attached bundle or OCI annotation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steiza I didn't think about this! What should we be doing about verification where the signature is stored in an annotation? Do we deprecate this with v4, or wait?

I'm OK with waiting, this is unrelated to the sigstore-go migration.

if err != nil {
return fmt.Errorf("loading verifier: %w", err)
}
dd := cremote.NewDupeDetector(ddVerifier)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be outdated at this point, but I just want to explicitly confirm that there's no reason to keep this around. Duplications aren't relevant because we're not attaching a signature multiple times to an annotation. I'm not certain why record creation timestamp was added though, that's the one that gives me pause.

cc @steiza

Comment thread cmd/cosign/cli/attest.go
Use: "attest",
Short: "Attest the supplied container image",
Example: ` cosign attest --key <key path>|<kms uri> [--predicate <path>] [--a key=value] [--no-upload=true|false] [--record-creation-timestamp=true|false] [--f] [--r] <image uri>
Example: ` cosign attest --key <key path>|<kms uri> [--predicate <path>] [--no-upload=true|false] [--yes] <image uri>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--key should be optional

Comment thread cmd/cosign/cli/attest.go
if o.NoUpload && o.BundlePath == "" {
return fmt.Errorf("must enable upload to the OCI registry or specify a local --bundle path")
}
var attestType string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, what's attestType for?

return fmt.Errorf("resolving attachment type %s for image %s: %w", c.Attachment, img, err)
}
// OCI bundle always contains attestation
verified, bundleVerified, err = cosign.VerifyImageAttestations(ctx, ref, co, c.NameOptions...)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of the scope for this PR, but verify and verify-attestation are now looking very similar, so we might be able to merge them as part of v4.

Comment thread cmd/cosign/cli/verify.go
Args: cobra.MinimumNArgs(1),
PersistentPreRun: options.BindViper,
RunE: func(cmd *cobra.Command, args []string) error {
if o.CommonVerifyOptions.PrivateInfrastructure {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're keeping IgnoreTlog, can we keep PrivateInfrastructure? This was just meant as an alias.

@Hayden-IO Hayden-IO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall lgtm, other comments from other threads apply here

@Hayden-IO Hayden-IO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first of the commits where I'd suggest we need to split this out into its own PR, because this commit does a lot and it's not as obvious that there aren't subtle changes, in comparison to the other commits where for the most part we're just dropping the !bundle{} block.

Are there any functions that could be added in order to support other changes in this PR, and then in later PRs, we do smaller refactors removing dead code?

return root.NewSigningConfig(
// NewEmptySigningConfig returns a signing config with no services configured.
func NewEmptySigningConfig() *root.SigningConfig {
sc, _ := root.NewSigningConfig(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, i'd add a comment for why it's ok to not check the returned error value

@Hayden-IO Hayden-IO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is well, ideally this commit would only drop dead code, and so we can take care of that in its own PR.

Comment thread pkg/cosign/verify.go
var targetDigest v1.Hash
for _, m := range manifest.Manifests {
if val, ok := m.Annotations["kind"]; ok && val == "dev.cosignproject.cosign/image" {
if val, ok := m.Annotations["kind"]; ok && (val == "dev.cosignproject.cosign/image" || val == "dev.cosignproject.cosign/imageIndex") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move small changes like this out into their own PRs with their own tests?

Comment thread pkg/cosign/verify.go
@@ -138,9 +124,6 @@ type CheckOpts struct {
IgnoreSCT bool
// Detached SCT. Optional, as the SCT is usually embedded in the certificate.
SCT []byte

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCT fields no longer needed?

Comment thread pkg/cosign/verify_test.go
_, err = ValidateAndUnpackCert(chain[0], co)
if err != nil {
t.Errorf("ValidateAndUnpackCert expected no error, got err = %v", err)
func appendSlices(slices [][]byte) []byte {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also use bytes.Join which should do the same thing

Comment thread pkg/cosign/verify_test.go
require.Contains(t, err.Error(), "none of the expected identities matched what was in the certificate")
}

func TestValidateAndUnpackCertInvalidGithubWorkflowTrigger(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we are losing some test coverage with these being removed. verificationOptions() (pkg/cosign/verify.go:213-219) still builds a verification policy with these options. Can we make sure that we have matching test coverage, either in a e2e test or with bundle verification?

Comment thread pkg/cosign/verify_test.go
assert.NoError(t, err)
}

func TestValidateAndUnpackCertWithSCT(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any tests when IgnoreSCT is set to false, and that a certificate without an embedded SCT is rejected? This would cover pkg/cosign/verify.go:242-245.

return bundlePath
}

func TestVerifyBlobCmdWithBundle(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have coverage for the success case, but this drops coverage for failing cases. Can we either update these tests to use sigstore-go's mocks for verification, or confirm we have adequate test coverage elsewhere?

})
}

func TestVerifyBlobCmdInvalidRootCA(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here, there's a number of failure edge cases we should confirm are tested.

@@ -183,18 +76,9 @@ func SetTrustedMaterial(ctx context.Context, trustedRootPath, certChain, caRoots
if verifyOnlyWithKey {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any tests for this case?

Comment thread test/e2e_test.go
@@ -3408,7 +3101,8 @@ func TestAttachWithRFC3161Timestamp(t *testing.T) {
t.Fatal(err)
}

must(verifyKeylessTSA(imgName, file.Name(), pemrootRef, true, true), t)
// TODO: Re-enable verification if/once attach.SignatureCmd uploads signatures in the new bundle format.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does attach do now after this change?

return err
}

verifiedOutput, err := transformOutput(verified, ref.Name())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this was dropped, is that intentional?

return err
}

for _, blob := range args {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When multiple blobs are provided, this would overwrite the same bundle file with each iteration. This looks like a bug at HEAD as well.

Either, we change the command to not allow signing multiple blobs as once (i didn't realize you could, I think it'd be a fine change), or we need to prepend an artifact identifier to the name of the bundle (not as much of a fan of this, since we'd then need to document that the bundle name isn't actually the file name, it's the suffix).

} else {
co.ClaimVerifier = cosign.SimpleClaimVerifier
}
co.ClaimVerifier = cosign.IntotoSubjectClaimVerifier

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't ClaimVerifier checked in the now deleted Cosign verification library, https://github.com/sigstore/cosign/pull/4959/changes#diff-8a85c8e688d61e16b8af8e09832ed2bef89c1163b0e9601a8363c782c387c006L932-L936? Does sigstore-go handle this and this is now dead code? Or is there missing functionality?

Comment thread cmd/cosign/cli/sign.go
TSAClientKey: o.TSAClientKey,
TSAServerName: o.TSAServerName,
TSAServerURL: o.TSAServerURL,
IssueCertificateForExistingKey: o.IssueCertificate,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BundlePath is missing here, but optionally used within SignCmd().

} else {
ko.TrustedMaterial, err = cosign.TrustedRoot()
if err != nil {
ui.Warnf(ctx, "Could not fetch trusted_root.json from the TUF repository. Continuing with individual targets. Error from TUF: %v", err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an outdated error, we won't support fetching the individual target files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants