Fix comparison in replace option for attestation

[bburky]

Summary

Currently the replace option compares r.predicateURI == payloadData["payloadType"] when evaluating each signature:

cosign/pkg/cosign/remote/remote.go

Line 142 in 21e6b80

if r.predicateURI == payloadData["payloadType"] {

This appears to be comparing the payload's predicateType (e.g a URI https://example.com/predicate or cosign.sigstore.dev/attestation/v1 for custom) to the signature's payloadType (which is always(?) application/vnd.in-toto+json). This appears to be a bug unless I'm misunderstanding something.

Fix this by making the replace option replace all attestations which match the current predicate type. I think this is the intended behavior?

The command line output has been updated to clarify the actions taken, clearly listing all the attestations retained and deleted:

$ cosign attest --type https://example.com/predicate --key "$key" --predicate predicate.json --replace "localhost:5000/$image"
Not replacing attestation predicate: https://example.com/aaaa
Replacing attestation predicate: https://example.com/predicate
Replacing attestation predicate: https://example.com/predicate

This feature was initially requested in #923 and implemented in #1039.

Ticket Link

No ticket.

Release Note

* Fixed predicate type comparison in replace option of attestation

[bburky]

I think this broke with #1248 which changed the DSSE envelope to always be the in-toto media type.

[dlorenc]

cc @priyawadhwa could you take a look when you get a chance here?

[priyawadhwa]

Thanks for fixing this @bburky! Just left a couple nits. It might also be useful to test the --replace flag so that this type of bug can't be reintroduced.

I think modifying the existing test would probably work:

cosign/test/e2e_test.go

Line 204 in d2781b8

func TestAttestVerify(t *testing.T) {

You could try attest a second time with the --replace flag and make sure the # of attestations on the image is still 1 (if replacement fails, then there should be an additional attestation, on which the test would fail)

lmk if you run into any issues!

[bburky]

@priyawadhwa I can't easily add the test to TestAttestVerify(). The high level command interfaces of VerifyAttestationCommand or download/AttestationCmd only gives you an error result. I cannot easily get any other output from it unless I redirect stdout and parse JSON. I can do that, but I wanted to see if you had a simpler design in mind. Is there a lower level function I can easily invoke to retrieve an array of attestations?

Edit: actually I see now cosign.FetchAttestationsForReference may work

[bburky]

I added an e2e test for the the replace option using cosign.FetchAttestationsForReference() to download the attestations.

Somehow this is only failing on random.Image() generated containers. Somehow this causes only the replace option to not upload any new attestations. It silently fails.

I used skopeo to copy real images to a local registry when testing, the replace option works perfectly fine on them. I can reproduce the buggy behavior on the command line with --replace if I upload a random.Image() to a local registry, it's not unique to the unit tests.

I honestly have no idea what's wrong. This bug predates my changes, it occurs with the v1.5.0 release cosign binary on any image from random.Image() when using --replace.

[priyawadhwa]

Thanks for trying to add the test @bburky ! I left a comment for simplifying the test for now, & if you wouldn't mind maybe you could open an issue for the bug you found.

[dlorenc]

Looks like DCO got dropped somehow, you'll need to add that back.

[bburky]

I think everything should be ready now.

[priyawadhwa]

thanks @bburky !