Repository: sigstore/cosign
Pull request #3292 (2023-10-11 02:21:01)

Fix ReplaceSignatures
This was correctly replacing the base of the signatures but was also
appending those signatures to the Get() response. So the v1.Image
representation was getting out of sync with the oci.Signatures
representation.

Normally you won't see this because cosign doesn't do multiple
attestations in the same command for the same image, so you'd roundtrip
the signatures through the v1.Image being pushed and pulled, which
corrects the discrepancy.

If you (specifically, me) attempt to attach an attestation multiple
times, the Get() will get very out of sync with the v1.Image and Replace
no longer does what you'd expect because it's operating on incorrect
Get() results instead of directly on the v1.Image representation.

jonjohnsonjr: cc @developer-guy I might be missing something
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/sigstore/cosign/pull/3292?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) Report
> Merging [#3292](https://app.codecov.io/gh/sigstore/cosign/pull/3292?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (0ca6c05) into [main](https://app.codecov.io/gh/sigstore/cosign/commit/4c5669d7ea677749c434c5e5943fc90268a5dcd9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (4c5669d) will **increase** coverage by `0.10%`.
> The diff coverage is `0.00%`.

```diff
@@            Coverage Diff             @@
##             main    #3292      +/-   ##
==========================================
+ Coverage   30.32%   30.42%   +0.10%     
==========================================
  Files         155      155              
  Lines        9853     9820      -33     
==========================================
  Hits         2988     2988              
+ Misses       6418     6385      -33     
  Partials      447      447              
```


| [Files](https://app.codecov.io/gh/sigstore/cosign/pull/3292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | Coverage Δ | |
|---|---|---|
| [pkg/oci/mutate/signatures.go](https://app.codecov.io/gh/sigstore/cosign/pull/3292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL29jaS9tdXRhdGUvc2lnbmF0dXJlcy5nbw==) | `65.38% <ø> (+31.38%)` | :arrow_up: |
| [pkg/oci/mutate/mutate.go](https://app.codecov.io/gh/sigstore/cosign/pull/3292?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL29jaS9tdXRhdGUvbXV0YXRlLmdv) | `86.69% <0.00%> (+2.86%)` | :arrow_up: |


hectorj2f: @jonjohnsonjr I assume it is related to this issue https://github.com/sigstore/cosign/issues/923 but it isn't doing what it was meant to do. You have removed the code and the test is still passing 🤔 https://github.com/sigstore/cosign/pull/1366/files#diff-9e762d0df1502ee31a1de1a3bc67be6c2707ec3e6117be3147907a60ef5fb854R260. 
hectorj2f: I can see how the replace tests are failing now.
jonjohnsonjr: Added a test, without my diff it fails with:

```
--- FAIL: TestSignEntity (0.01s)
    --- FAIL: TestSignEntity/with_replace_op_(attestation) (0.00s)
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
```

Which confirms my assertion about the duplicated `Get()` results.

Patch:
diff --git a/go.mod b/go.mod
index 9031c12d..dd64ae65 100644
--- a/go.mod
+++ b/go.mod
@@ -16,7 +16,7 @@ require (
 	github.com/go-openapi/swag v0.22.4
 	github.com/go-piv/piv-go v1.11.0
 	github.com/google/certificate-transparency-go v1.1.6
-	github.com/google/go-cmp v0.5.9
+	github.com/google/go-cmp v0.6.0
 	github.com/google/go-containerregistry v0.16.1
 	github.com/google/go-github/v55 v55.0.0
 	github.com/in-toto/in-toto-golang v0.9.0
@@ -30,7 +30,7 @@ require (
 	github.com/open-policy-agent/opa v0.57.0
 	github.com/pkg/errors v0.9.1
 	github.com/secure-systems-lab/go-securesystemslib v0.7.0
-	github.com/sigstore/fulcio v1.4.1
+	github.com/sigstore/fulcio v1.4.2
 	github.com/sigstore/rekor v1.2.2
 	github.com/sigstore/sigstore v1.7.3
 	github.com/sigstore/sigstore/pkg/signature/kms/aws v1.7.3
@@ -46,7 +46,7 @@ require (
 	github.com/transparency-dev/merkle v0.0.2
 	github.com/withfig/autocomplete-tools/integrations/cobra v1.2.1
 	github.com/xanzy/go-gitlab v0.93.1
-	go.step.sm/crypto v0.35.1
+	go.step.sm/crypto v0.36.0
 	golang.org/x/crypto v0.14.0
 	golang.org/x/oauth2 v0.13.0
 	golang.org/x/sync v0.4.0
@@ -105,7 +105,7 @@ require (
 	github.com/alibabacloud-go/tea-xml v1.1.2 // indirect
 	github.com/aliyun/credentials-go v1.2.3 // indirect
 	github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2 // indirect
-	github.com/aws/aws-sdk-go v1.45.19 // indirect
+	github.com/aws/aws-sdk-go v1.45.20 // indirect
 	github.com/aws/aws-sdk-go-v2 v1.21.0 // indirect
 	github.com/aws/aws-sdk-go-v2/config v1.18.37 // indirect
 	github.com/aws/aws-sdk-go-v2/credentials v1.13.35 // indirect
@@ -286,7 +286,7 @@ require (
 	google.golang.org/genproto v0.0.0-20230913181813-007df8e322eb // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20230913181813-007df8e322eb // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20230920204549-e6e6cdab5c13 // indirect
-	google.golang.org/grpc v1.58.2 // indirect
+	google.golang.org/grpc v1.58.3 // indirect
 	google.golang.org/protobuf v1.31.0 // indirect
 	gopkg.in/DataDog/dd-trace-go.v1 v1.55.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
diff --git a/go.sum b/go.sum
index dba6b388..f0e6073a 100644
--- a/go.sum
+++ b/go.sum
@@ -182,8 +182,8 @@ github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj
 github.com/asaskevich/govalidator v0.0.0-20200907205600-7a23bdc65eef/go.mod h1:WaHUgvxTVq04UNunO+XhnAqY/wQc+bxr74GqbsZ/Jqw=
 github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2 h1:DklsrG3dyBCFEj5IhUbnKptjxatkF07cF2ak3yi77so=
 github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2/go.mod h1:WaHUgvxTVq04UNunO+XhnAqY/wQc+bxr74GqbsZ/Jqw=
-github.com/aws/aws-sdk-go v1.45.19 h1:+4yXWhldhCVXWFOQRF99ZTJ92t4DtoHROZIbN7Ujk/U=
-github.com/aws/aws-sdk-go v1.45.19/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI=
+github.com/aws/aws-sdk-go v1.45.20 h1:U/wLZEwqVB6o2XlcJ7um8kczx+A1X2MgO2y4wdKDQTs=
+github.com/aws/aws-sdk-go v1.45.20/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI=
 github.com/aws/aws-sdk-go-v2 v1.7.1/go.mod h1:L5LuPC1ZgDr2xQS7AmIec/Jlc7O/Y1u2KxJyNVab250=
 github.com/aws/aws-sdk-go-v2 v1.14.0/go.mod h1:ZA3Y8V0LrlWj63MQAnRHgKf/5QB//LSZCPNWlWrNGLU=
 github.com/aws/aws-sdk-go-v2 v1.21.0 h1:gMT0IW+03wtYJhRqTVYn0wLzwdnK9sRMcxmtfGzRdJc=
@@ -537,8 +537,8 @@ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/
 github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8/DtOE=
 github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
-github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
-github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/go-containerregistry v0.16.1 h1:rUEt426sR6nyrL3gt+18ibRcvYpKYdpsa5ZW7MA08dQ=
 github.com/google/go-containerregistry v0.16.1/go.mod h1:u0qB2l7mvtWVR5kNcbFIhFY1hLbf8eeGapA+vbFDCtQ=
 github.com/google/go-github/v55 v55.0.0 h1:4pp/1tNMB9X/LuAhs5i0KQAE40NmiR/y6prLNb9x9cg=
@@ -880,8 +880,8 @@ github.com/segmentio/ksuid v1.0.4/go.mod h1:/XUiZBD3kVx5SmUOl55voK5yeAbBNNIed+2O
 github.com/shibumi/go-pathspec v1.3.0 h1:QUyMZhFo0Md5B8zV8x2tesohbb5kfbpTi9rBnKh5dkI=
 github.com/shibumi/go-pathspec v1.3.0/go.mod h1:Xutfslp817l2I1cZvgcfeMQJG5QnU2lh5tVaaMCl3jE=
 github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc=
-github.com/sigstore/fulcio v1.4.1 h1:EYcbWYSd+Lo0Su8c7mPlOd6glhZv+v6FxCFjF9Nah9g=
-github.com/sigstore/fulcio v1.4.1/go.mod h1:7vKoHlIl1QqOU3iCoM+FPHmga1T1ZUJmmXILd89oPwY=
+github.com/sigstore/fulcio v1.4.2 h1:YWCKgqh8PDH0SkmDFBnTTOiCdDlX5MYWgexfIsFoxZw=
+github.com/sigstore/fulcio v1.4.2/go.mod h1:H/zPKnbe2Yl7uR+Y1imnVHGHDESwsUA1L7pHZTjhwQ0=
 github.com/sigstore/rekor v1.2.2 h1:5JK/zKZvcQpL/jBmHvmFj3YbpDMBQnJQ6ygp8xdF3bY=
 github.com/sigstore/rekor v1.2.2/go.mod h1:FGnWBGWzeNceJnp0x9eDFd41mI8aQqCjj+Zp0IEs0Qg=
 github.com/sigstore/sigstore v1.7.3 h1:HVVTfrMezJeLyl2xhJ8edzkrEGBa4KxjQZB4FlQ4JLU=
@@ -1049,8 +1049,8 @@ go.opentelemetry.io/otel/sdk v1.19.0/go.mod h1:NedEbbS4w3C6zElbLdPJKOpJQOrGUJ+Gf
 go.opentelemetry.io/otel/trace v1.19.0 h1:DFVQmlVbfVeOuBRrwdtaehRrWiL1JoVs9CPIQ1Dzxpg=
 go.opentelemetry.io/otel/trace v1.19.0/go.mod h1:mfaSyvGyEJEI0nyV2I4qhNQnbBOUUmYZpYojqMnX2vo=
 go.opentelemetry.io/proto/otlp v1.0.0 h1:T0TX0tmXU8a3CbNXzEKGeU5mIVOdf0oykP+u2lIVU/I=
-go.step.sm/crypto v0.35.1 h1:QAZZ7Q8xaM4TdungGSAYw/zxpyH4fMYTkfaXVV9H7pY=
-go.step.sm/crypto v0.35.1/go.mod h1:vn8Vkx/Mbqgoe7AG8btC0qZ995Udm3e+JySuDS1LCJA=
+go.step.sm/crypto v0.36.0 h1:njrVQqjWVyCzHxqQUDprR7HXypZX8IxtOqe4IIHSjgU=
+go.step.sm/crypto v0.36.0/go.mod h1:XMRbgkhoigPLaPMjZeY1VFIYXVw03Wul0+5lENF6l7c=
 go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
 go.uber.org/atomic v1.9.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
 go.uber.org/atomic v1.10.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
@@ -1483,8 +1483,8 @@ google.golang.org/grpc v1.31.1/go.mod h1:N36X2cJ7JwdamYAgDz+s+rVMFjt3numwzf/HckM
 google.golang.org/grpc v1.33.2/go.mod h1:JMHMWHQWaTccqQQlmk3MJZS+GWXOdAesneDmEnv2fbc=
 google.golang.org/grpc v1.34.0/go.mod h1:WotjhfgOW/POjDeRt8vscBtXq+2VjORFy659qA51WJ8=
 google.golang.org/grpc v1.35.0/go.mod h1:qjiiYl8FncCW8feJPdyg3v6XW24KsRHe+dy9BAGRRjU=
-google.golang.org/grpc v1.58.2 h1:SXUpjxeVF3FKrTYQI4f4KvbGD5u2xccdYdurwowix5I=
-google.golang.org/grpc v1.58.2/go.mod h1:tgX3ZQDlNJGU96V6yHh1T/JeoBQ2TXdr43YbYSsCJk0=
+google.golang.org/grpc v1.58.3 h1:BjnpXut1btbtgN/6sp+brB2Kbm2LjNXnidYujAVbSoQ=
+google.golang.org/grpc v1.58.3/go.mod h1:tgX3ZQDlNJGU96V6yHh1T/JeoBQ2TXdr43YbYSsCJk0=
 google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8=
 google.golang.org/protobuf v0.0.0-20200221191635-4d8936d0db64/go.mod h1:kwYJMbMJ01Woi6D6+Kah6886xMZcty6N08ah7+eCXa0=
 google.golang.org/protobuf v0.0.0-20200228230310-ab0ca4ff8a60/go.mod h1:cfTl7dwQJ+fmap5saPgwCLgHXTUD7jkjRqWcaiX5VyM=
diff --git a/pkg/oci/mutate/signatures.go b/pkg/oci/mutate/signatures.go
index 4ac356fe..648cab04 100644
--- a/pkg/oci/mutate/signatures.go
+++ b/pkg/oci/mutate/signatures.go
@@ -74,7 +74,7 @@ func ReplaceSignatures(base oci.Signatures) (oci.Signatures, error) {
 	return &sigAppender{
 		Image: img,
 		base:  base,
-		sigs:  sigs,
+		sigs:  []oci.Signature{},
 	}, nil
 }
 


Test patch:
diff --git a/pkg/oci/mutate/mutate_test.go b/pkg/oci/mutate/mutate_test.go
index 4a315268..b0e85be2 100644
--- a/pkg/oci/mutate/mutate_test.go
+++ b/pkg/oci/mutate/mutate_test.go
@@ -351,7 +351,7 @@ func TestSignEntity(t *testing.T) {
 				if al, err := atts.Get(); err != nil {
 					t.Fatalf("Get() = %v", err)
 				} else if len(al) != 1 {
-					t.Errorf("len(Get()) = %d, wanted %d", len(al), i)
+					t.Errorf("len(Get()) = %d, wanted %d", len(al), 1)
 				}
 			}
 		}
@@ -424,6 +424,43 @@ func TestSignEntity(t *testing.T) {
 			}
 		}
 	})
+
+	t.Run("with replace op (attestation)", func(t *testing.T) {
+		for _, se := range []oci.SignedEntity{si, sii, sunk} {
+			orig, err := static.NewAttestation([]byte("blah"))
+			if err != nil {
+				t.Fatalf("static.NewAttestation() = %v", err)
+			}
+			se, err = AttachAttestationToEntity(se, orig)
+			if err != nil {
+				t.Fatalf("AttachAttestationToEntity() = %v", err)
+			}
+
+			ro := &replaceAll{}
+
+			for i := 2; i < 10; i++ {
+				sig, err := static.NewAttestation([]byte(fmt.Sprintf("%d", i)))
+				if err != nil {
+					t.Fatalf("static.NewAttestation() = %v", err)
+				}
+
+				se, err = AttachAttestationToEntity(se, sig, WithReplaceOp(ro))
+				if err != nil {
+					t.Fatalf("AttachAttestationToEntity() = %v", err)
+				}
+
+				atts, err := se.Attestations()
+				if err != nil {
+					t.Fatalf("Attestations() = %v", err)
+				}
+				if al, err := atts.Get(); err != nil {
+					t.Fatalf("Get() = %v", err)
+				} else if len(al) != 1 {
+					t.Errorf("len(Get()) = %d, wanted %d", len(al), 1)
+				}
+			}
+		}
+	})
 }
 
 type dupe struct {
@@ -437,3 +474,22 @@ var _ DupeDetector = (*dupe)(nil)
 func (d *dupe) Find(oci.Signatures, oci.Signature) (oci.Signature, error) {
 	return d.sig, d.err
 }
+
+type replaceAll struct {
+}
+
+func (r *replaceAll) Replace(signatures oci.Signatures, o oci.Signature) (oci.Signatures, error) {
+	return &replaceOCISignatures{
+		Signatures:   signatures,
+		attestations: []oci.Signature{o},
+	}, nil
+}
+
+type replaceOCISignatures struct {
+	oci.Signatures
+	attestations []oci.Signature
+}
+
+func (r *replaceOCISignatures) Get() ([]oci.Signature, error) {
+	return r.attestations, nil
+}