Skip to content

Protobuf bundle support for subcommand clean #4539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+82 −13
Open

Protobuf bundle support for subcommand clean #4539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 60 additions & 13 deletions cmd/cosign/cli/clean.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,32 +62,76 @@ func CleanCmd(ctx context.Context, regOpts options.RegistryOptions, cleanType op
}

remoteOpts := regOpts.GetRegistryClientOpts(ctx)
ociRemoteOpts := ociremote.WithRemoteOptions(remoteOpts...)

sigRef, err := ociremote.SignatureTag(ref, ociremote.WithRemoteOptions(remoteOpts...))
sigRef, err := ociremote.SignatureTag(ref, ociRemoteOpts)
if err != nil {
return err
}

attRef, err := ociremote.AttestationTag(ref, ociremote.WithRemoteOptions(remoteOpts...))
attRef, err := ociremote.AttestationTag(ref, ociRemoteOpts)
if err != nil {
return err
}

sbomRef, err := ociremote.SBOMTag(ref, ociremote.WithRemoteOptions(remoteOpts...))
sbomRef, err := ociremote.SBOMTag(ref, ociRemoteOpts)
if err != nil {
return err
}

var cleanTags []name.Tag
referrerRefs := []name.Reference{}
digest, ok := ref.(name.Digest)
if !ok {
var err error
digest, err = ociremote.ResolveDigest(ref, ociRemoteOpts)
if err != nil {
return fmt.Errorf("resolving digest: %w", err)
}
}
idx, err := remote.Referrers(digest, remoteOpts...)
Copy link
Contributor

@cmurphy cmurphy Nov 25, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@steiza steiza Nov 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cmurphy cmurphy Nov 26, 2025

Choose a reason for hiding this comment

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

I think it wouldn't hurt to check for the media type before deleting.

if err != nil {
return err
}
if idx != nil {
// Delete manifest
imgDigest, err := idx.Digest()
if err != nil {
return err
}
referrerDigestStr := fmt.Sprintf("%s@%s", ref.Context().Name(), imgDigest.String())
referrerDigest, err := name.NewDigest(referrerDigestStr)
if err != nil {
return err
}
referrerRefs = append(referrerRefs, referrerDigest)

// Delete layers in the manifest
idxManifest, err := idx.IndexManifest()
if err != nil {
return err
}
if idxManifest != nil {
for _, manifest := range idxManifest.Manifests {
layerDigestStr := fmt.Sprintf("%s@%s", ref.Context().Name(), manifest.Digest.String())
layerDigest, err := name.NewDigest(layerDigestStr)
if err != nil {
return err
}
referrerRefs = append(referrerRefs, layerDigest)
}
}
}

var cleanTags []name.Reference
switch cleanType {
Copy link
Contributor

@cmurphy cmurphy Nov 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@steiza steiza Nov 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cmurphy cmurphy Nov 26, 2025

Choose a reason for hiding this comment

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

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.

case options.CleanTypeSignature:
cleanTags = []name.Tag{sigRef}
cleanTags = append([]name.Reference{sigRef}, referrerRefs...)
case options.CleanTypeSbom:
cleanTags = []name.Tag{sbomRef}
cleanTags = []name.Reference{sbomRef}
case options.CleanTypeAttestation:
cleanTags = []name.Tag{attRef}
cleanTags = append([]name.Reference{attRef}, referrerRefs...)
case options.CleanTypeAll:
cleanTags = []name.Tag{sigRef, attRef, sbomRef}
cleanTags = append([]name.Reference{sigRef, attRef, sbomRef}, referrerRefs...)
default:
panic("invalid CleanType value")
}
Expand All @@ -103,13 +147,16 @@ func CleanCmd(ctx context.Context, regOpts options.RegistryOptions, cleanType op
case errors.As(err, &te) && te.StatusCode == http.StatusBadRequest:
// Docker registry >=v2.3 requires does not allow deleting the OCI object name directly, must use the digest instead.
// See https://github.com/distribution/distribution/blob/main/docs/content/spec/api.md#deleting-an-image
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 {
Copy link
Contributor

@cmurphy cmurphy Nov 25, 2025

Choose a reason for hiding this comment

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

What does this mean? It only tries to delete by digest if the reference is a tag?

Copy link
Member Author

@steiza steiza Nov 25, 2025

Choose a reason for hiding this comment

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

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...)

if err := deleteByDigest(tTag, remoteOpts...); err != nil {
if errors.As(err, &te) && te.StatusCode == http.StatusNotFound { //nolint: revive
} else {
fmt.Fprintf(os.Stderr, "could not delete %s by digest from %s:\n%v\n", t, imageRef, err)
}
} else {
fmt.Fprintf(os.Stderr, "could not delete %s by digest from %s:\n%v\n", t, imageRef, err)
fmt.Fprintf(os.Stderr, "Removed %s from %s\n", t, imageRef)
}
} else {
fmt.Fprintf(os.Stderr, "Removed %s from %s\n", t, imageRef)
}
default:
fmt.Fprintf(os.Stderr, "could not delete %s from %s:\n%v\n", t, imageRef, err)
Expand Down
22 changes: 22 additions & 0 deletions test/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,28 @@ func TestImportSignVerifyClean(t *testing.T) {

// It doesn't work
mustErr(verify(pubKeyPath, imgName, true, nil, "", false), t)

// Sign with new bundle format
so.NewBundleFormat = true
must(sign.SignCmd(ctx, ro, ko, so, []string{imgName}), t)

// Verify should work again
trustedRootPath := prepareTrustedRoot(t, "")
bundleVerifyCmd := cliverify.VerifyCommand{
CommonVerifyOptions: options.CommonVerifyOptions{
TrustedRootPath: trustedRootPath,
},
KeyRef: pubKeyPath,
NewBundleFormat: true,
UseSignedTimestamps: false,
}
must(bundleVerifyCmd.Exec(ctx, []string{imgName}), t)

// Clean again
must(cli.CleanCmd(ctx, options.RegistryOptions{}, "all", imgName, true), t)

// Verify should fail again
mustErr(bundleVerifyCmd.Exec(ctx, []string{imgName}), t)
}

type targetInfo struct {
Expand Down
Loading