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

Require inclusion proofs, make promises optional #84

Merged
merged 3 commits into from
Jul 3, 2023

Conversation

haydentherapper
Copy link
Collaborator

The log always generates inclusion proofs, so we will make it a requirement that clients verify the proof. Promises will be deprecated over time, but for now, we'll make them optional.

Fixes #82
Ref sigstore/rekor#1566

Summary

Release Note

Documentation

The log always generates inclusion proofs, so we will make it a
requirement that clients verify the proof. Promises will be deprecated
over time, but for now, we'll make them optional.

Fixes sigstore#82
Ref sigstore/rekor#1566

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

@feelepxyz feelepxyz left a comment

Choose a reason for hiding this comment

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

Just a quick drive by comment, could we also do a major version bump of the mime type version? Clients could then choose to key a verification strategy off this.

@haydentherapper
Copy link
Collaborator Author

@feelepxyz, can you check the last commit to make sure that's correct? Haven't done a version bump before.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
znewman01
znewman01 previously approved these changes Jun 30, 2023
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

LGTM. A little weird that we're requiring clients to verify inclusion promises if present, but intending to totally get rid of them. I understand why though.

+1 to bumping the minor version.

@haydentherapper
Copy link
Collaborator Author

Probably should mention how to handle older bundles. Maybe something like: "Older bundles will contain only an inclusion promise, which must be verified if present." We could also mark the field as deprecated, or hold off for a little before doing so. Thoughts?

@znewman01
Copy link
Contributor

We have to be a little careful about "downgrade attacks" here (in scare quotes because the inclusion promise should still not be possible to get for an attacker). Maybe something like:

Client verification libraries MAY provide an option to support v0.1 bundles for backwards compatibility, which sometimes contain an inclusion promise and not an inclusion proof. In this case, validate the promise.

Verifiers SHOULD NOT allow v0.1 bundles if they're used in an ecosystem which never produced them

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

Added a comment in the bundle, and updated the comment for inclusion_promise. PTAL

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

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

LGTM

@kommendorkapten kommendorkapten merged commit b532639 into sigstore:main Jul 3, 2023
21 checks passed
@haydentherapper haydentherapper deleted the require-inclusion branch July 10, 2023 06:13
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.

[Proposal] Require inclusion proofs
5 participants