Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Switch to newer DSSE rekor type #3299

Merged
merged 8 commits into from Mar 26, 2024

Conversation

haydentherapper
Copy link
Contributor

The intoto v001 type does not persist signatures of the DSSE envelope, as noted in sigstore/rekor#973. We introduced an intoto v002 type shortly after to fix this, but since then, we've introduced another newer type, DSSE v001, which also does not persist the attestation in Rekor (as we discourage using Rekor as storage).

I also updated the verifier in slsa-framework/slsa-verifier#742 to search for both Rekor entry types.

Summary

...

Testing Process

...

Checklist

  • Review the contributing guidelines
  • Add a reference to related issues in the PR description.
  • Update documentation if applicable.
  • Add unit tests if applicable.
  • Add changes to the CHANGELOG if applicable.

The intoto v001 type does not persist signatures of the DSSE envelope,
as noted in sigstore/rekor#973. We introduced an
intoto v002 type shortly after to fix this, but since then, we've
introduced another newer type, DSSE v001, which also does not persist
the attestation in Rekor (as we discourage using Rekor as storage).

I also updated the verifier in slsa-framework/slsa-verifier#742
to search for both Rekor entry types.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

@kpk47 @laurentsimon, a Sigstore community noticed that the signature from the DSSE envelope was missing in Rekor (thread). This was a known issue in the intoto v001 type, and we've since introduced a newer DSSE type that a) fixes this and b) stops persisting the attestation since Rekor shouldn't be attestation storage.

Lemme know if this is an issue. This should also get merged with slsa-framework/slsa-verifier#742.

Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @haydentherapper . I think this change works. Shall we consider it a breaking change or not? I think so. Wdut?

nit: Can you update the CHANGELOG.md file?

@laurentsimon
Copy link
Collaborator

/cc @ramonpetgrave64

@haydentherapper
Copy link
Contributor Author

haydentherapper commented Mar 4, 2024

It's sort of a breaking change. It's not a breaking change for the generator itself, but it's a breaking change in that it'll require verifiers to be updated. When we made this change in Cosign, we didn't call it a breaking change since Cosign is both the signer and verifier, but given the verification tooling is separate here, probably would be good to have this as a minor rather than patch release.

Edit: Also given the attestation is no longer persisted in rekor, calling this a breaking change would be reasonable in case anyone was relying on it.

Signed-off-by: Hayden B <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

Updated changelog

@laurentsimon
Copy link
Collaborator

It's sort of a breaking change. It's not a breaking change for the generator itself, but it's a breaking change in that it'll require verifiers to be updated. When we made this change in Cosign, we didn't call it a breaking change since Cosign is both the signer and verifier, but given the verification tooling is separate here, probably would be good to have this as a minor rather than patch release.

but it could be possible that a signer uses a more recent version than the verifier (e.g., another team), so I think it probably was a breaking change.

Edit: Also given the attestation is no longer persisted in rekor, calling this a breaking change would be reasonable in case anyone was relying on it.

SG, thanks.

I'll wait to merge until we're 100% sure we'll bump the major version of the next release.

@laurentsimon
Copy link
Collaborator

There's a linter failing.

Signed-off-by: Hayden B <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

Fixed

Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Signed-off-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
@laurentsimon laurentsimon enabled auto-merge (squash) March 26, 2024 15:18
@haydentherapper
Copy link
Contributor Author

Thanks for fixing the PR Laurent. Failing test seems like it just wants a rebase?

@haydentherapper
Copy link
Contributor Author

Oh it is already. Hm..

@laurentsimon
Copy link
Collaborator

pre-submit failing is not your fault. I'll send a PR to fix it.

@laurentsimon
Copy link
Collaborator

PR is out from this morning #3454 :) Need to be merged first

@laurentsimon
Copy link
Collaborator

Bocking PR is merged. I've rebased and enabled auto-merge. Thanks @haydentherapper !

@laurentsimon laurentsimon merged commit 8869c8a into slsa-framework:main Mar 26, 2024
74 checks passed
@haydentherapper haydentherapper deleted the dsse-entry branch March 26, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants