code review feedback

Signed-off-by: Jake Sanders <[email protected]>

pkg/oci/mutate/options.go

@@ -15,7 +15,10 @@
 
 package mutate
 
-import "github.com/sigstore/cosign/pkg/oci"
+import (
+	"github.com/google/go-containerregistry/pkg/v1/types"
+	"github.com/sigstore/cosign/pkg/oci"
+)
 
 // DupeDetector scans a list of signatures looking for a duplicate.
 type DupeDetector interface {
@@ -54,3 +57,46 @@ func WithReplaceOp(ro ReplaceOp) SignOption {
 		so.ro = ro
 	}
 }
+
+type signatureOpts struct {
+	annotations map[string]string
+	bundle      *oci.Bundle
+	cert        []byte
+	chain       []byte
+	mediaType   types.MediaType
+}
+
+type SignatureOption func(*signatureOpts)
+
+func WithAnnotations(annotations map[string]string) SignatureOption {
+	return func(so *signatureOpts) {
+		so.annotations = annotations
+	}
+}
+
+func WithBundle(bundle *oci.Bundle) SignatureOption {
+	return func(so *signatureOpts) {
+		so.bundle = bundle
+	}
+}
+
+func WithCertChain(cert, chain []byte) SignatureOption {
+	return func(so *signatureOpts) {
+		so.cert = cert
+		so.chain = chain
+	}
+}
+
+func WithMediaType(mediaType types.MediaType) SignatureOption {
+	return func(so *signatureOpts) {
+		so.mediaType = mediaType
+	}
+}
+
+func makeSignatureOption(opts ...SignatureOption) *signatureOpts {
+	so := &signatureOpts{}
+	for _, opt := range opts {
+		opt(so)
+	}
+	return so
+}

pkg/oci/mutate/signature.go

@@ -124,70 +124,68 @@ func (sw *sigWrapper) Size() (int64, error) {
 	return sw.wrapped.Size()
 }
 
-// SignatureAnnotations returns a new `oci.Signature` based on the provided one
-func SignatureAnnotations(sig oci.Signature, newAnnotations map[string]string) (oci.Signature, error) {
-	newAnnotations = copyAnnotations(newAnnotations)
-	oldAnnotations, err := sig.Annotations()
+func Signature(original oci.Signature, opts ...SignatureOption) (oci.Signature, error) {
+	newSig := sigWrapper{wrapped: original}
+
+	so := makeSignatureOption(opts...)
+	oldAnn, err := original.Annotations()
 	if err != nil {
 		return nil, errors.Wrap(err, "could not get annotations from signature to mutate")
 	}
-	newAnnotations[static.SignatureAnnotationKey] = oldAnnotations[static.SignatureAnnotationKey]
-	for _, key := range []string{static.BundleAnnotationKey, static.CertificateAnnotationKey, static.ChainAnnotationKey} {
-		if val, isSet := oldAnnotations[key]; isSet {
-			newAnnotations[key] = val
-		} else {
-			delete(newAnnotations, key)
+
+	var newAnn map[string]string
+	if so.annotations != nil {
+		newAnn = copyAnnotations(so.annotations)
+		newAnn[static.SignatureAnnotationKey] = oldAnn[static.SignatureAnnotationKey]
+		for _, key := range []string{static.BundleAnnotationKey, static.CertificateAnnotationKey, static.ChainAnnotationKey} {
+			if val, isSet := oldAnn[key]; isSet {
+				newAnn[key] = val
+			} else {
+				delete(newAnn, key)
+			}
 		}
+	} else {
+		newAnn = copyAnnotations(oldAnn)
 	}
 
-	return &sigWrapper{wrapped: sig, annotations: newAnnotations}, nil
-}
-
-func SignatureBundle(sig oci.Signature, newBundle *oci.Bundle) (oci.Signature, error) {
-	annotations, err := sig.Annotations()
-	if err != nil {
-		return nil, errors.Wrap(err, "could not get annotations from signature to mutate")
-	}
-	delete(annotations, static.BundleAnnotationKey)
-	if newBundle != nil {
-		b, err := json.Marshal(newBundle)
+	if so.bundle != nil {
+		newSig.bundle = so.bundle
+		b, err := json.Marshal(so.bundle)
 		if err != nil {
 			return nil, err
 		}
-		annotations[static.BundleAnnotationKey] = string(b)
+		newAnn[static.BundleAnnotationKey] = string(b)
 	}
-	return &sigWrapper{wrapped: sig, bundle: newBundle, annotations: annotations}, nil
-}
 
-func SignatureCertAndChain(sig oci.Signature, newCert, newChain []byte) (oci.Signature, error) {
-	var cert *x509.Certificate
-	var chain []*x509.Certificate
-	var err error
-	annotations, err := sig.Annotations()
-	if err != nil {
-		return nil, errors.Wrap(err, "could not get annotations from signature to mutate")
-	}
-	delete(annotations, static.CertificateAnnotationKey)
-	delete(annotations, static.ChainAnnotationKey)
-	if newCert != nil {
-		certs, err := cryptoutils.LoadCertificatesFromPEM(bytes.NewReader(newCert))
+	if so.cert != nil {
+		var cert *x509.Certificate
+		var chain []*x509.Certificate
+
+		certs, err := cryptoutils.LoadCertificatesFromPEM(bytes.NewReader(so.cert))
 		if err != nil {
 			return nil, err
 		}
-		annotations[static.CertificateAnnotationKey] = string(newCert)
+		newAnn[static.CertificateAnnotationKey] = string(so.cert)
 		cert = certs[0]
-	}
-	if newChain != nil {
-		chain, err = cryptoutils.LoadCertificatesFromPEM(bytes.NewReader(newChain))
-		if err != nil {
-			return nil, err
+
+		delete(newAnn, static.ChainAnnotationKey)
+		if so.chain != nil {
+			chain, err = cryptoutils.LoadCertificatesFromPEM(bytes.NewReader(so.chain))
+			if err != nil {
+				return nil, err
+			}
+			newAnn[static.ChainAnnotationKey] = string(so.chain)
 		}
-		annotations[static.ChainAnnotationKey] = string(newChain)
+
+		newSig.cert = cert
+		newSig.chain = chain
 	}
 
-	return &sigWrapper{wrapped: sig, cert: cert, chain: chain, annotations: annotations}, nil
-}
+	if so.mediaType != "" {
+		newSig.mediaType = so.mediaType
+	}
+
+	newSig.annotations = newAnn
 
-func SignatureMediaType(sig oci.Signature, newMT types.MediaType) oci.Signature {
-	return &sigWrapper{wrapped: sig, mediaType: newMT}
+	return &newSig, nil
 }

pkg/oci/mutate/signature_test.go

@@ -188,8 +188,8 @@ func assertSignaturesEqual(t *testing.T, wanted, got oci.Signature) {
 	}
 }
 
-func TestSignatureAnnotations(t *testing.T) {
-	payload := "this is the TestSignatureAnnotations content!"
+func TestSignatureWithAnnotations(t *testing.T) {
+	payload := "this is the TestSignatureWithAnnotations content!"
 	b64sig := "b64 content1="
 	annotations := map[string]string{
 		"foo":  "bar",
@@ -198,16 +198,16 @@ func TestSignatureAnnotations(t *testing.T) {
 	originalSig := mustCreateSignature(t, []byte(payload), b64sig)
 	expectedSig := mustCreateSignature(t, []byte(payload), b64sig, static.WithAnnotations(annotations))
 
-	newSig, err := SignatureAnnotations(originalSig, annotations)
+	newSig, err := Signature(originalSig, WithAnnotations(annotations))
 	if err != nil {
-		t.Fatalf("SignatureAnnotations() returned error: %v", err)
+		t.Fatalf("Signature(WithAnnotations()) returned error: %v", err)
 	}
 
 	assertSignaturesEqual(t, expectedSig, newSig)
 }
 
-func TestSignatureBundle(t *testing.T) {
-	payload := "this is the TestSignatureBundle content!"
+func TestSignatureWithBundle(t *testing.T) {
+	payload := "this is the TestSignatureWithBundle content!"
 	b64sig := "b64 content2="
 	bundle := &oci.Bundle{
 		SignedEntryTimestamp: mustBase64Decode(t, "MEUCIQClUkUqZNf+6dxBc/pxq22JIluTB7Kmip1G0FIF5E0C1wIgLqXm+IM3JYW/P/qjMZSXW+J8bt5EOqNfe3R+0A9ooFE="),
@@ -221,44 +221,47 @@ func TestSignatureBundle(t *testing.T) {
 	originalSig := mustCreateSignature(t, []byte(payload), b64sig)
 	expectedSig := mustCreateSignature(t, []byte(payload), b64sig, static.WithBundle(bundle))
 
-	newSig, err := SignatureBundle(originalSig, bundle)
+	newSig, err := Signature(originalSig, WithBundle(bundle))
 	if err != nil {
-		t.Fatalf("SignatureBundle() returned error: %v", err)
+		t.Fatalf("Signature(WithBundle()) returned error: %v", err)
 	}
 
 	assertSignaturesEqual(t, expectedSig, newSig)
 }
 
-func TestSignatureCertAndChain(t *testing.T) {
-	payload := "this is the TestSignatureCertAndChain content!"
+func TestSignatureWithCertChain(t *testing.T) {
+	payload := "this is the TestSignatureWithCertChain content!"
 	b64sig := "b64 content3="
 
 	originalSig := mustCreateSignature(t, []byte(payload), b64sig)
 	expectedSig := mustCreateSignature(t, []byte(payload), b64sig, static.WithCertChain(testCertBytes, testChainBytes))
 
-	newSig, err := SignatureCertAndChain(originalSig, testCertBytes, testChainBytes)
+	newSig, err := Signature(originalSig, WithCertChain(testCertBytes, testChainBytes))
 	if err != nil {
-		t.Fatalf("SignatureCertAndChain() returned error: %v", err)
+		t.Fatalf("Signature(WithCertChain()) returned error: %v", err)
 	}
 
 	assertSignaturesEqual(t, expectedSig, newSig)
 }
 
-func TestSignatureMediaType(t *testing.T) {
-	payload := "this is the TestSignatureMediaType content!"
+func TestSignatureWithMediaType(t *testing.T) {
+	payload := "this is the TestSignatureWithMediaType content!"
 	b64sig := "b64 content4="
 	mediaType := types.MediaType("test/media.type")
 
 	originalSig := mustCreateSignature(t, []byte(payload), b64sig)
 	expectedSig := mustCreateSignature(t, []byte(payload), b64sig, static.WithLayerMediaType(mediaType))
 
-	newSig := SignatureMediaType(originalSig, mediaType)
+	newSig, err := Signature(originalSig, WithMediaType(mediaType))
+	if err != nil {
+		t.Fatalf("Signature(WithMediaType()) returned error: %v", err)
+	}
 
 	assertSignaturesEqual(t, expectedSig, newSig)
 }
 
-func TestSignatureMutateEverything(t *testing.T) {
-	payload := "this is the TestSignatureMutateEverything content!"
+func TestSignatureWithEverything(t *testing.T) {
+	payload := "this is the TestSignatureWithEverything content!"
 	b64sig := "b64 content5="
 	annotations := map[string]string{
 		"foo":  "bar",
@@ -282,19 +285,14 @@ func TestSignatureMutateEverything(t *testing.T) {
 		static.WithCertChain(testCertBytes, testChainBytes),
 		static.WithLayerMediaType(mediaType))
 
-	newSig, err := SignatureAnnotations(originalSig, annotations)
-	if err != nil {
-		t.Fatalf("SignatureAnnotations() returned error: %v", err)
-	}
-	newSig, err = SignatureBundle(newSig, bundle)
-	if err != nil {
-		t.Fatalf("SignatureBundle() returned error: %v", err)
-	}
-	newSig, err = SignatureCertAndChain(newSig, testCertBytes, testChainBytes)
+	newSig, err := Signature(originalSig,
+		WithAnnotations(annotations),
+		WithBundle(bundle),
+		WithCertChain(testCertBytes, testChainBytes),
+		WithMediaType(mediaType))
 	if err != nil {
-		t.Fatalf("SignatureCertAndChain() returned error: %v", err)
+		t.Fatalf("Signature(With...) returned error: %v", err)
 	}
-	newSig = SignatureMediaType(newSig, mediaType)
 
 	assertSignaturesEqual(t, expectedSig, newSig)
 }