Repository navigation
Require new bundle format for signing and verification - #4959
aaronlew02 wants to merge 12 commits into
Conversation
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
bf1189d to
e6262f8
Compare
aaronlew02
changed the title
45cff47 to
d4a7237
Compare
d4a7237 to
11cb99b
Compare
562178c to
f5bd6ed
Compare
TestingThe following commands can be used to test the changes in this PR. SetupBefore 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-pair1. TUF-provided Signing Config & Trusted RootVerify that a TUF-provided signing config and trusted root are resolved correctly for keyless operations and key-based, certificate-backed operations: Keyless OperationsImage 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 OperationsKey-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 RootVerify that explicitly provided local config and trusted root files are loaded and utilized correctly: Keyless OperationsImage 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 OperationsKey-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>
e0f1135 to
6c0cc72
Compare
| } | ||
|
|
||
| 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) |
There was a problem hiding this comment.
This is throwing away the result of ShouldUploadToTlog, so the attestation would be unconditionally uploaded to the transparency log.
There was a problem hiding this comment.
Whether the attestation would be uploaded to the transparency log depends on:
- The value of
len(c.SigningConfig.RekorLogURLs()) > 0, which iftruewould 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- The user will be prompted to confirm that they wish to upload to Rekor despite the repository being private. If the user disagrees,
shouldUploadreturns false, and execution proceeds without a Rekor upload. If the user agrees... - The user will receive the Immutable Records confirmation prompt. If the user disagrees, execution stops with an error. If the user agrees,
shouldUploadreturns true and the signing config goes tosigstore-go, which would upload because a log is present.
This matches current functionality.
| Offline bool | ||
| UseSignedTimestamps bool | ||
| IgnoreTlog bool | ||
| HashAlgorithm crypto.Hash |
There was a problem hiding this comment.
To confirm, hash algorithm will now only be determined by the default mapping between signing scheme and hash alg?
There was a problem hiding this comment.
And, how does this interact with when you parse out the algorithm from the payloadDigest?
There was a problem hiding this comment.
To confirm, hash algorithm will now only be determined by the default mapping between signing scheme and hash alg?
Yes.
And, how does this interact with when you parse out the algorithm from the payloadDigest?
The parsed algorithm isn't used for bundles with a message signature (see here). In that case, sigstore-go compares the payloadDigest bytes to the message digest before passing them to the verifier (see here), which uses the key's default hash and only checks that the digest length matches that hash's size (see here).
For DSSE bundles, though, the parsed algorithm is checked against the in-toto subject's digest key (see here).
| } | ||
|
|
||
| sig, err := base64signature(c.SigRef, c.BundlePath) | ||
| _, err = cosign.VerifyNewBundle(ctx, co, artifactPolicyOption, bundle) |
There was a problem hiding this comment.
For a later PR - we should implement the change mentioned in
cosign/pkg/cosign/verify_bundle.go
Lines 25 to 29 in 98ff280
| var shouldUpload bool | ||
| var err error | ||
|
|
||
| if ko.SigningConfig == nil { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
This will be addressed in a later PR once the --use-signing-config flag is removed.
| decodedSig, err := base64.StdEncoding.DecodeString(string(sigBytes)) | ||
| if err != nil { | ||
| t.Fatalf("failed to decode base64 signature: %v", err) | ||
| var b1 struct { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
also in the example, we should denote --key is optional
| return fmt.Errorf("please specify --bundle") | ||
| } | ||
|
|
||
| var signType string |
There was a problem hiding this comment.
what's the purpose of signType?
There was a problem hiding this comment.
To specify either keyless or certificate-based attesting as the source of the error here (small detail).
| OIDCClientSecret: oidcClientSecret, | ||
| OIDCRedirectURL: o.OIDC.RedirectURL, | ||
| OIDCDisableProviders: o.OIDC.DisableAmbientProviders, | ||
| OIDCProvider: o.OIDC.Provider, |
There was a problem hiding this comment.
was this missing before? if so, can we split out that change?
There was a problem hiding this comment.
Yes. I've submitted #5079 to address this.
|
|
||
| for _, blob := range args { | ||
| // TODO: remove when the output flag has been deprecated | ||
| if o.Output != "" { |
There was a problem hiding this comment.
are we removing this flag in another commit/PR?
Hayden-IO
left a comment
Hayden-IO
left a comment
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
For another change, should we pass through timeout for sign/sign-blob as well?
| // 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. |
There was a problem hiding this comment.
@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) |
There was a problem hiding this comment.
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
| 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> |
There was a problem hiding this comment.
--key should be optional
| if o.NoUpload && o.BundlePath == "" { | ||
| return fmt.Errorf("must enable upload to the OCI registry or specify a local --bundle path") | ||
| } | ||
| var attestType string |
There was a problem hiding this comment.
Same here, what's attestType for?
There was a problem hiding this comment.
To specify either keyless or certificate-based attesting as the source of the error here (small detail).
| 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...) |
There was a problem hiding this comment.
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.
| Args: cobra.MinimumNArgs(1), | ||
| PersistentPreRun: options.BindViper, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if o.CommonVerifyOptions.PrivateInfrastructure { |
There was a problem hiding this comment.
if we're keeping IgnoreTlog, can we keep PrivateInfrastructure? This was just meant as an alias.
There was a problem hiding this comment.
I've kept PrivateInfrastructure in #5093.
Hayden-IO
left a comment
Hayden-IO
left a comment
There was a problem hiding this comment.
overall lgtm, other comments from other threads apply here
Hayden-IO
left a comment
Hayden-IO
left a comment
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
nit, i'd add a comment for why it's ok to not check the returned error value
There was a problem hiding this comment.
I've added the following comment in #5083:
// root.NewSigningConfig only errors when an unsupported media type is provided.
// Since root.SigningConfigMediaType02 is a supported constant, this call cannot fail.
Hayden-IO
left a comment
Hayden-IO
left a comment
There was a problem hiding this comment.
Here is well, ideally this commit would only drop dead code, and so we can take care of that in its own PR.
| 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") { |
There was a problem hiding this comment.
Can we move small changes like this out into their own PRs with their own tests?
There was a problem hiding this comment.
This change has been extracted into #5080.
| @@ -138,9 +124,6 @@ type CheckOpts struct { | |||
| IgnoreSCT bool | |||
| // Detached SCT. Optional, as the SCT is usually embedded in the certificate. | |||
| SCT []byte | |||
There was a problem hiding this comment.
SCT fields no longer needed?
There was a problem hiding this comment.
IgnoreSCT is still needed for --insecure-ignore-sct, but SCT and CTLogPubKeys will be deleted in a follow-up PR for cleanup.
| _, err = ValidateAndUnpackCert(chain[0], co) | ||
| if err != nil { | ||
| t.Errorf("ValidateAndUnpackCert expected no error, got err = %v", err) | ||
| func appendSlices(slices [][]byte) []byte { |
There was a problem hiding this comment.
Can also use bytes.Join which should do the same thing
There was a problem hiding this comment.
This PR simply shifts the pre-existing appendSlices, but I've replaced it with bytes.Join in #5093.
| require.Contains(t, err.Error(), "none of the expected identities matched what was in the certificate") | ||
| } | ||
|
|
||
| func TestValidateAndUnpackCertInvalidGithubWorkflowTrigger(t *testing.T) { |
There was a problem hiding this comment.
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?
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestValidateAndUnpackCertWithSCT(t *testing.T) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 { | |||
There was a problem hiding this comment.
Do we have any tests for this case?
There was a problem hiding this comment.
Yes, this case is covered by the e2e tests TestSignVerifyBundleOffline, TestSignBlobVerifyBlob, and TestSignBlobNonSHA256, as well as the unit test TestVerifyBlobWithUnsetHashAlgorithmMatchesKeyDefault.
| @@ -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. | |||
There was a problem hiding this comment.
What does attach do now after this change?
There was a problem hiding this comment.
attach signature doesn't change here. However, it still supports legacy outputs, which can no longer be verified.
The state described by the TODO is inaccurate, though. attach signature does upload signatures in the new bundle format, but only when provided a new bundle.
We do test the new-bundle codepath in TestSignDownloadAttachNewBundle, but that test doesn't call verify or use an RFC 3161 timestamp.
I've updated the TODO in #5093 while I think about how to move forward here.
| return err | ||
| } | ||
|
|
||
| verifiedOutput, err := transformOutput(verified, ref.Name()) |
There was a problem hiding this comment.
It looks like this was dropped, is that intentional?
There was a problem hiding this comment.
This remains, but the code has shifted (see here).
| return err | ||
| } | ||
|
|
||
| for _, blob := range args { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
co.ClaimVerifier is still used in verifyImageAttestationsSigstoreBundles, which is called by VerifyImageAttestations. It enables image signature annotation verification, which is not handled by sigstore-go.
However, co.ClaimVerifier is unused in verify_blob_attestation.go, so I've removed it in #5089.
| TSAClientKey: o.TSAClientKey, | ||
| TSAServerName: o.TSAServerName, | ||
| TSAServerURL: o.TSAServerURL, | ||
| IssueCertificateForExistingKey: o.IssueCertificate, |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
This is an outdated error, we won't support fetching the individual target files.
cmurphy
left a comment
cmurphy
left a comment
There was a problem hiding this comment.
I'm still working on reviewing this in-depth, but I have some high-level thoughts to start with. Thanks for working so diligently on this.
I really appreciate you breaking this PR into multiple commits, it really helps make this more reviewable.
The other goal I like to have with chunking PRs into separate commits is for debuggability and rollback safety, and the way these are separated doesn't quite hit that. If I revert to any commit before "Update shared flags and helper logic", cosign doesn't build for me. That means tools like "git bisect" won't work if we need to trace a bug back to a particular commit, and if we find any bug in this we can't just roll back to the prior commit, we have to revert the whole pull request (if it's not something we can just fix right away). If there's any way to break up that "Update shared flags and helper logic" commit so that the prior commits work without it, or reorder it to have it first in line so that every individual commit builds, that would be ideal. But I'll understand if you say the refactor is too complex or it would be an unreasonable amount of work to mess with git just to keep the history clean.
Along with splitting up the commits, it also helps for debuggability if each commit has a semi-detailed commit message. For me, I like to know how each commit fits into the overarching pull request goal, and have any changes that may be surprising or weird or not be fully aligned with the goal be well explained so I don't have to ask a million questions during the review or later when I'm looking through the git history to debug. Don't assume that the people reviewing the code already know everything - it really helps to spell it out. So a commit with subject "Update core logic and delete legacy verification code" isn't quite verbose enough me, it's more helpful to have a hint of what you mean by "updating core logic" without me having to dive into the whole 1000 line commit.
I see that some pieces of this were already broken out into separate PRs, thanks for doing that! As I review deeper I'll try to point out any other pieces that might be good candidates to split out if you haven't already found them. From the PR description, I see "ALL signers using a signing config containing at least one transparency log must agree to the immutable record privacy statement." - I wonder if that's a potential candidate for its own PR?
I see Hayden left a few comments a few weeks ago, it'd be nice to see those addressed.
A more general question for you and for @Hayden-IO and @steiza - this PR removes support for all old bundle and disparate signature materials for both signing and verifying. Is that the approach that has been widely agreed on for v4? There are going to be millions of artifacts that exist with the old formats that will still need to be verified. Would it be a smoother approach to delete support for signing with the legacy formats, but keep support for verifying legacy formats until v5?
And in that vein, I see the command docs were updated for the deleted flags, but since this is such a huge change in functionality, I think it would be good to have an entry in the README explaining this change. It would be good to have an update to docs.sigstore.dev ready to go too, at least before this is released.
|
@cmurphy Thank you so much for reviewing this very large change. I appreciate the advice and would like this to be as reviewable as possible. I have a series of PRs in draft, which are still under construction, that attempt to split up this PR into buildable chunks while incorporating several merged and unmerged auxiliary changes as well address comments from this PR.
My understanding is that v4 is intended to use
That makes sense -- I'm open to that. It would reduce the number of users broken by an update. Alternatively, v3 could be maintained for legacy verification until v5.
Agreed. I'd be happy to update those. |
I second all of this!
My understanding (although this is of course up for discussion among Cosign maintainers): v3 (where we're at today)
v4 (what this PR is working towards)
v5 (the future)
Agreed, and I think we've mostly done that, modulo the README update. The docs updates should mostly be done, as sigstore/docs#418 moved us off of deprecated Cosign v3 behavior, which should continue to be supported in Cosign v4. |
This does sound familiar, now that I think about it. I didn't think about it then but I worry a bit now that this is the kind of change that could be very frustrating to users (but that they won't tell us about until after we've done it). Let's chat about it in today's meeting? |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
|
This PR has been split into #5083 and its descendants. |
Summary
This change mandates the new bundle format for signing and verification.
For verification, this enables the exclusive use of
sigstore-goand 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[trueby default] or explicitly provided via--signing-config).Signers NOT requiring services MUST set
--use-signing-config=false.Verifiers validating service-provided verification material MUST use a trusted root (either fetched from TUF automatically or explicitly provided via
--trusted-root).Several flags have been removed:
--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--new-bundle-format--rekor-entry-type--record-creation-timestamp,--signature/--signature-path,--payload,--signature-digest-algorithm--sct,--rfc3161-timestamp/--rfc3161-timestamp-bundle,--b64,--output/--output-signature/--output-payload/--output-attestation/--output-certificate--replace,--attachment,--experimental-oci11,--sign-container-identity--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_FILEALL signers using a signing config containing at least one transparency log must agree to the immutable record privacy statement.