fix parsing error in --only for cosign copy (#4049)

* fix parsing error in --only Signed-off-by: Bob Callaway <[email protected]> * make docgen Signed-off-by: Bob Callaway <[email protected]> * fix lint, make error message better Signed-off-by: Bob Callaway <[email protected]> --------- Signed-off-by: Bob Callaway <[email protected]>
sigstore · Feb 6, 2025 · c4299b1 · c4299b1
1 parent 7fc127b
commit c4299b1
Show file tree

Hide file tree

Showing 4 changed files with 130 additions and 12 deletions.
diff --git a/cmd/cosign/cli/copy/copy.go b/cmd/cosign/cli/copy/copy.go
@@ -21,7 +21,6 @@ import (
 	"net/http"
 	"os"
 	"runtime"
-	"strings"
 
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -38,7 +37,7 @@ import (
 
 // CopyCmd implements the logic to copy the supplied container image and signatures.
 // nolint
-func CopyCmd(ctx context.Context, regOpts options.RegistryOptions, srcImg, dstImg string, sigOnly, force bool, copyOnly, platform string) error {
+func CopyCmd(ctx context.Context, regOpts options.RegistryOptions, srcImg, dstImg string, sigOnly, force bool, copyOnly []string, platform string) error {
 	no := regOpts.NameOptions()
 	srcRef, err := name.ParseReference(srcImg, no...)
 	if err != nil {
@@ -183,19 +182,20 @@ func remoteCopy(ctx context.Context, pusher *remote.Pusher, src, dest name.Refer
 	return pusher.Push(ctx, dest, got)
 }
 
-func parseOnlyOpt(onlyFlag string, sigOnly bool) ([]tagMap, error) {
+func parseOnlyOpt(onlyFlag []string, sigOnly bool) ([]tagMap, error) {
 	var tags []tagMap
-	tagSet := sets.New(strings.Split(onlyFlag, ",")...)
+	tagSet := sets.New(onlyFlag...)
 
 	if sigOnly {
 		fmt.Fprintf(os.Stderr, "--sig-only is deprecated, use --only=sig instead")
 		tagSet.Insert("sig")
 	}
 
-	validTags := sets.New("sig", "sbom", "att")
+	validTags := []string{"sig", "sbom", "att"}
+	validTagsSet := sets.New(validTags...)
 	for tag := range tagSet {
-		if !validTags.Has(tag) {
-			return nil, fmt.Errorf("invalid value for --only: %s, only following values are supported, %s", tag, validTags)
+		if !validTagsSet.Has(tag) {
+			return nil, fmt.Errorf("invalid value for --only: %s, only the following values are supported: %s", tag, validTags)
 		}
 	}
 

diff --git a/cmd/cosign/cli/copy/copy_test.go b/cmd/cosign/cli/copy/copy_test.go
@@ -16,9 +16,11 @@ package copy
 
 import (
 	"context"
+	"reflect"
 	"testing"
 
 	"github.com/sigstore/cosign/v2/cmd/cosign/cli/options"
+	ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote"
 )
 
 func TestCopyAttachmentTagPrefix(t *testing.T) {
@@ -33,7 +35,7 @@ func TestCopyAttachmentTagPrefix(t *testing.T) {
 
 	err := CopyCmd(ctx, options.RegistryOptions{
 		RefOpts: refOpts,
-	}, srcImg, destImg, false, true, "", "")
+	}, srcImg, destImg, false, true, []string{}, "")
 	if err == nil {
 		t.Fatal("failed to copy with attachment-tag-prefix")
 	}
@@ -45,8 +47,124 @@ func TestCopyPlatformOpt(t *testing.T) {
 	srcImg := "alpine"
 	destImg := "test-alpine"
 
-	err := CopyCmd(ctx, options.RegistryOptions{}, srcImg, destImg, false, true, "", "linux/amd64")
+	err := CopyCmd(ctx, options.RegistryOptions{}, srcImg, destImg, false, true, []string{}, "linux/amd64")
 	if err == nil {
 		t.Fatal("failed to copy with platform")
 	}
 }
+
+func TestParseOnlyOpt(t *testing.T) {
+	tests := []struct {
+		only         []string
+		sigOnly      bool
+		expectErr    bool
+		expectTagMap []tagMap
+	}{
+		{
+			only:      []string{"bogus"},
+			sigOnly:   true,
+			expectErr: true,
+		},
+		{
+			only:         []string{},
+			sigOnly:      true,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SignatureTag},
+		},
+		{
+			only:         []string{"sig"},
+			sigOnly:      false,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SignatureTag},
+		},
+		{
+			only:         []string{"sig"},
+			sigOnly:      true,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SignatureTag},
+		},
+		{
+			only:         []string{"sbom"},
+			sigOnly:      true,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SBOMTag, ociremote.SignatureTag},
+		},
+		{
+			only:         []string{"att"},
+			sigOnly:      true,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.AttestationTag, ociremote.SignatureTag},
+		},
+		{
+			only:         []string{"sbom"},
+			sigOnly:      false,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SBOMTag},
+		},
+		{
+			only:         []string{"att"},
+			sigOnly:      false,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.AttestationTag},
+		},
+		{
+			only:         []string{"att", "sbom"},
+			sigOnly:      false,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.AttestationTag, ociremote.SBOMTag},
+		},
+		{
+			only:         []string{"sig", "sbom"},
+			sigOnly:      false,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SignatureTag, ociremote.SBOMTag},
+		},
+		{
+			only:         []string{"sig", "att"},
+			sigOnly:      false,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SignatureTag, ociremote.AttestationTag},
+		},
+		{
+			only:         []string{"sig", "att", "sbom"},
+			sigOnly:      false,
+			expectErr:    false,
+			expectTagMap: []tagMap{ociremote.SignatureTag, ociremote.AttestationTag, ociremote.SBOMTag},
+		},
+		{
+			only:      []string{"sig", "att", "sbom", "bad"},
+			sigOnly:   false,
+			expectErr: true,
+		},
+	}
+
+	for _, test := range tests {
+		result, err := parseOnlyOpt(test.only, test.sigOnly)
+		if (err != nil) != test.expectErr {
+			t.Errorf("unexpected failure from parseOnlyOpt: expectErr=%v, err = %v", test.expectErr, err)
+		} else if !compareTagMaps(result, test.expectTagMap) {
+			t.Errorf("result tag map did not match expected value: result: %v expected: %v", result, test.expectTagMap)
+		}
+	}
+}
+
+func compareTagMaps(slice1, slice2 []tagMap) bool {
+	if len(slice1) != len(slice2) {
+		return false // Different lengths can't be equal
+	}
+
+	for _, fn1 := range slice1 {
+		found := false
+		for _, fn2 := range slice2 {
+			if reflect.DeepEqual(reflect.ValueOf(fn1), reflect.ValueOf(fn2)) {
+				found = true
+				break // Found a match, move to the next fn1
+			}
+		}
+		if !found {
+			return false // fn1 not found in slice2
+		}
+	}
+
+	return true // All functions in slice1 found in slice2
+}
diff --git a/cmd/cosign/cli/options/copy.go b/cmd/cosign/cli/options/copy.go
@@ -21,7 +21,7 @@ import (
 
 // CopyOptions is the top level wrapper for the copy command.
 type CopyOptions struct {
-	CopyOnly      string
+	CopyOnly      []string
 	SignatureOnly bool
 	Force         bool
 	Platform      string
@@ -34,7 +34,7 @@ var _ Interface = (*CopyOptions)(nil)
 func (o *CopyOptions) AddFlags(cmd *cobra.Command) {
 	o.Registry.AddFlags(cmd)
 
-	cmd.Flags().StringVar(&o.CopyOnly, "only", "",
+	cmd.Flags().StringSliceVar(&o.CopyOnly, "only", []string{},
 		"custom string array to only copy specific items, this flag is comma delimited. ex: --only=sig,att,sbom")
 
 	cmd.Flags().BoolVar(&o.SignatureOnly, "sig-only", false,

diff --git a/doc/cosign_copy.md b/doc/cosign_copy.md