Fix: Drop the CosignPredicate wrapper around SBOM attestations.

[mattmoor]

🐛 This change drops the CosignPredicate that cosign wraps around SPDX/CycloneDX attestations.

Currently cosign wraps SPDX and CycloneDX attestations produced via their shortnames (cosign attest --type {spdxjson|cyclonedx}) in a CosignPredicate envelope.

However, the whole point of the in-toto predicateType is to specify the schema of the predicate, and despite using the SPDX and Cyclone predicate type URIs, this envelope violates their schema with the extra layer.

Moreover, if users were to attest these SBOMs with the explicit predicate type URI:

cosign attest --type https://spdx.dev/Document ...

Then cosign will NOT add this additional envelope, which makes it effectively impossible to know the schema to use for policy validation based strictly on the predicateType because even cosign will produce these attestations both ways.

Fixes: #2126

/kind bug

Release Note

BREAKING: This change removes the CosignPredicate envelope that wraps the predicates of SPDX and CycloneDX attestations, which was a violation of the schema specified via their predicateType field.

Documentation

[codecov-commenter]

Codecov Report

Merging #2718 (7c68406) into main (eb8f39d) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main    #2718   +/-   ##
=======================================
  Coverage   30.15%   30.15%           
=======================================
  Files         150      150           
  Lines        9470     9470           
=======================================
  Hits         2856     2856           
  Misses       6177     6177           
  Partials      437      437           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[hectorj2f]

Please, sign off your commits.

[mattmoor]

@hectorj2f yeah, I was just letting CI run to tell me what all I broke as I juggle this and the other commit. I already signed it locally, I was just wondering whether I could save MS some money by waiting to update things 😁

[mattmoor]

🚨 🚨 Let's not merge this until (at least) Monday 🚨 🚨

... since I understand I'm sending breaking changes on the weekend, and I'd like to get more 👀 on this.

[hectorj2f]

Lgtm

[znewman01]

I think this is a good fix and I'm okay with a breaking change here but I want to understand who this would break before I approve.

AFAICT there're no Sigstore tools (other than policy-controller) that we'd be breaking, and they already know about both versions of this.

But from #2126:

But changing this now can lead to issues elsewhere. For example, Trivy explicitly supports the current attestation format: https://aquasecurity.github.io/trivy/v0.31.3/docs/attestation/sbom/

So the question is: does trivy sbom work with proper SBOM attestations (e.g., no CosignPredicate)? If not, do we just accept the breakage with Cosign 2.0-produced SBOMs? (It is technically a bug since folks should have been handling the --type https://spdx.dev/... case but I wouldn't be shocked otherwise.)

Same question for tools other than trivy that might be depending on this.

The alternative might be:

1. Deprecate the old behavior. Allow folks to opt-in to the new behavior with a flag.
2. Proactively try to find consumers and get them to fix.
3. Wait.
4. Switch over to the new behavior. Probably still cause a lot of breakage.

I'm not sure it's worth that much effort so I think I'd be okay with just merging this. Just wanted to think it through.

[developer-guy]

Totally agree with you

[mattmoor]

@znewman01 So I believe that everything in the Trivy example you link will continue to work just fine because this doesn't change anything about the signatures, and these are just signing and attesting.

What breaks is when the user has a Cue/Rego policy over the attestation content, then they previously had an extra Data: layer, which is now gone.

[znewman01]

@znewman01 So I believe that everything in the Trivy example you link will continue to work just fine because this doesn't change anything about the signatures, and these are just signing and attesting.

They have an example at the bottom where they parse the output of verify-attestation. Does verify-attestation strip the Data: wrapper?

$ cosign verify-attestation --key /path/to/cosign.pub --type cyclonedx <IMAGE> > sbom.cdx.intoto.jsonl
$ trivy sbom ./sbom.cdx.intoto.jsonl

What breaks is when the user has a Cue/Rego policy over the attestation content, then they previously had an extra Data: layer, which is now gone.

Yeah, that's a good point. I think I've been talked into just ripping off the band-aid, though, so I'm okay if you want to merge.

[mattmoor]

@znewman01 Thanks, I missed that bit.

Does verify-attestation strip the Data: wrapper?

I'm pretty sure it doesn't, but I could be wrong.

🤔 Now you have me wondering what trivy sbom FILE actually does because (as a naive user) I'd assume that it would support taking the output of trivy image --format cyclonedx -o sbom.cdx.json <IMAGE>, which is missing Data (but also all of the attestation wrapping).

If Trivy doesn't support both ways today, then it is already broken today for cosign attest --type https://cyclonedx.com/... unfortunately. Put a bit more generally... any tooling that is broken by this change is already partially broken in the case where users are attesting with an explicit predicate type.

I dislike that this could break some user policies, but at the same time because of this strange duality we also can't really advise users on how to write policies against a schema because the current mechanism violates the schema things should have. My hope is that if we "stop the bleeding" now, we can get ahead of things before this stuff goes too mainstream to walk back 🤞

[ChristianCiach]

Does verify-attestation strip the Data: wrapper?

I'm pretty sure it doesn't, but I could be wrong.

It does not, which is exactly why I created the issue:

• Feature: Provide an easier way to retrieve SBOM from In-Toto attestation #2307

As commented in #2307 (comment), this would also make the newly added attestation-support in Trivy obsolete.