Protobuf bundle support for subcommand clean
#4539
Conversation
Signed-off-by: Zach Steindler <steiza@github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4539 +/- ##
==========================================
- Coverage 40.10% 36.37% -3.73%
==========================================
Files 155 220 +65
Lines 10044 12271 +2227
==========================================
+ Hits 4028 4464 +436
- Misses 5530 7114 +1584
- Partials 486 693 +207 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| var cleanTags []name.Reference | ||
| switch cleanType { |
There was a problem hiding this comment.
It seems like --type no longer has any meaning when it comes to referrers - when I sign with sign but then clean with --type=attestation, it just removes everything. That could come as a surprise to someone who's expecting those objects to be different.
There was a problem hiding this comment.
Yeah, unfortunately with the new protobuf bundle (where we're always trying to use the OCI1.1 referrer specification) both attest and sign are using CosignSignPredicateType, so I don't think there's an easy way to tell them apart.
I think eventually we might consider merging sign and attest, but of course that isn't where we are today.
Alternatively, we could only clean up referring items on CleanTypeAll? That could also be confusing. 🤷 Definitely open to feedback here.
There was a problem hiding this comment.
What if there was a new --type=referrer just for this? Then we could print a warning if referrers are found but a signature or attestation type is specified to let the user know they need to use specify the new type. I think at some point we just need to accept that referrers are different and can't be cajoled into behaving like classic .sigs or .atts.
| return fmt.Errorf("resolving digest: %w", err) | ||
| } | ||
| } | ||
| idx, err := remote.Referrers(digest, remoteOpts...) |
There was a problem hiding this comment.
Is there any chance a manifest in the referrer index could not have been produced by cosign/sigstore and cause a surprise if it was deleted?
Does this also need to delete the blobs referenced in the manifests or is it enough to just delete the manifests?
There was a problem hiding this comment.
It is certainly possible! Although maybe unlikely.
We queue up deleting the manifest by adding it to referrerRefs on line 106 below, and we queue up the layers in the manifest for deletion on line 120 below.
It's certainly possible to be more clever here - we could only delete layers if they have media type application/vnd.dev.sigstore.bundle.v0.3+json and only delete the manifest if all the layers are removed (otherwise rewrite the manifest to exclude the deleted layers)... but I think this command is really meant more for development so you can sign something, remove the signature, sign it again, and quickly iterate through those steps.
There was a problem hiding this comment.
I think it wouldn't hurt to check for the media type before deleting.
| if err := deleteByDigest(t, remoteOpts...); err != nil { | ||
| if errors.As(err, &te) && te.StatusCode == http.StatusNotFound { //nolint: revive | ||
| tTag, ok := t.(name.Tag) | ||
| if ok { |
There was a problem hiding this comment.
What does this mean? It only tries to delete by digest if the reference is a tag?
There was a problem hiding this comment.
We should only be down here if the remote.Delete() above failed, which will remove references if they are a tag or a digest. Despite its name, deleteByDigest() is really expecting a name.Tag, and so now we're checking to make sure what we're supplying it is a tag (which we probably should have been doing before...)
2 participants
Summary
I think this really for-real completes #4470.
Testing is pretty easy! Run
go run cmd/cosign/main.go clean <some image you signed before>.Release Note
cosign cleanDocumentation
N/A