Deduplicate code in sign/attest* and verify* commands #4449
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4449 +/- ##
==========================================
- Coverage 40.10% 35.35% -4.75%
==========================================
Files 155 220 +65
Lines 10044 15155 +5111
==========================================
+ Hits 4028 5358 +1330
- Misses 5530 9109 +3579
- Partials 486 688 +202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Move the nearly identical code for parsing key options and creating a key pair and token out of attest, attest-blob, sign, and sign-blob, and into a common helper package. Move functions that had been shared out of sign.go into the helper package too so that other commands do not have to import the sign command package. Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
| if err != nil { | ||
| return nil, nil, fmt.Errorf("marshalling timestamp: %w", err) | ||
| } | ||
| if err := os.WriteFile(ko.RFC3161TimestampPath, ts, 0600); err != nil { |
There was a problem hiding this comment.
When calling cosign sign or cosign attest, this file doesn't need to be written since it's included as an annotation on the image.
It looks like we support fetching a timestamp only with the new bundle format for cosign sign (I think this might have been a regression, I recall supporting it before the bundle format was added), but we support it with the old and new bundle format for cosign attest. Can we gate whether we write this file on if we're signing a container vs a blob?
There was a problem hiding this comment.
When calling cosign sign or cosign attest, this file doesn't need to be written since it's included as an annotation on the image.
I see where I went wrong, in cosign sign this function is only called by signDigestBundle when NewBundleFormat is set, so this path is skipped so no file is written. It's not the case for cosign attest which is my mistake.
It looks like we support fetching a timestamp only with the new bundle format for cosign sign (I think this might have been a regression, I recall supporting it before the bundle format was added),
Fetching a timestamp without the new bundle format should still work, it's part of this I think:
cosign/cmd/cosign/cli/sign/sign.go
Lines 426 to 437 in dccda70
Can we gate whether we write this file on if we're signing a container vs a blob?
Yes will fix
cmd/cosign/cli/attest/attest_blob.go
Outdated
| return fmt.Errorf("create bundle file: %w", err) | ||
| } | ||
| ui.Infof(ctx, "Wrote bundle to file %s", c.BundlePath) | ||
| return nil |
There was a problem hiding this comment.
Do you think the above should be de-duped as well? There's commonality between attest, sign and attest-blob with the construction of the attestation, but the latter is written to a file rather than OCI. Not sure if this deduplication is worth it to add complexity to WriteNewBundleWithSigningConfig, which I guess would have to conditionally write to OCI or a file.
There was a problem hiding this comment.
Yes I'll update these
| return fmt.Errorf("setting trusted material: %w", err) | ||
| } | ||
|
|
||
| if err = CheckSigstoreBundleUnsupportedOptions(*c, co); err != nil { |
There was a problem hiding this comment.
To confirm, we call SetTrustedMaterial before CheckSigstoreBundleUnsupportedOptions because we need to know if a trusted root was fetched when using the bundle format? Just wondering if we can exit out earlier, but seems like no.
There was a problem hiding this comment.
Right, this is also consistent with its behavior before.
|
This is fantastic! Thank you for splitting this change into the many commits, made this so straightforward to review! |
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Move flag checks when --new-bundle-format is used to a common helper module and have all four verify commands use it. Remove redundant flag checker code. Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
RootCerts and IntermediateCerts are already set on CheckOpts during loadCertsKeylessVerification. Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Move the setting of SigVerifier based on the key ref, key slot, or cert and cert chain, to the common file. For verifying blobs and blob attestations with a certificate instead of a key, we return the cert which is used directly in the options list for verification. For images, the cert and cert chain must be validated and then unpacked into the SigVerifier, where the cosign Verify* functions check its validity by extracting it from the verifier. Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
3 participants
The sign/sign-blob/attest/attest-blob and verify/verify-blob/verify-attestation/verify-blob-attestation commands contained a lot of code that was either copy and pasted to each command, or, more insidiously, functionally the same but written slightly differently between commands. This PR is an effort to reduce the existing duplication, make common updates simpler, prevent the commands from further diverging, and overall reduce LOC. The result is hopefully that the remainder of the code left in each command is mostly specific to that command itself.
This PR is best reviewed commit-by-commit, but can be merged by squash-and-merge.
Summary
Release Note
Documentation