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

Check if entry has inclusion proof rather than entity #310

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

adityasaky
Copy link
Member

Summary

I stumbled upon an inconsistency when verifying an entry's inclusion proof (for offline verification). I think it should be checking if the specific entry has an inclusion proof instead of going by the entity (eg. a bundle). A bundle could have entries with inclusion proofs and without, and the bundle returns true for this check if even one entry has an inclusion proof, so this could lead to an error when verifying an entry without the proof.

Release Note

Fixed check for whether a tlog entry has an inclusion proof

Documentation

NONE

Signed-off-by: Aditya Sirish A Yelgundhalli <ayelgundhall@bloomberg.net>
@adityasaky adityasaky requested a review from a team as a code owner October 10, 2024 20:56
@adityasaky
Copy link
Member Author

adityasaky commented Oct 10, 2024

Amending my original message: right now, this should never be a problem because of how online vs offline verification is decided. If WithOnlineVerification() is not used, then all entries must have the inclusion proof (meaning no mix of entries with and without the proof) or fail indicating the entry doesn't have the proof (as it currently would). If WithOnlineVerification() is used, we don't hit this at all. Overall, it might be okay to leave it as is?

This can change if the online vs offline control becomes a per-log control, though I don't know of contexts where that makes sense.

Copy link
Member

@steiza steiza left a comment

Choose a reason for hiding this comment

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

Oops! Yes, it was definitely my intention to use entry here, not entity. With how bundles are constructed today I don't think this ends up being much of a functionality difference, but I do think we should make this change.

Copy link
Member

@codysoyland codysoyland left a comment

Choose a reason for hiding this comment

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

Nice find, thank you!

@codysoyland codysoyland merged commit 6d84685 into sigstore:main Oct 11, 2024
10 checks passed
@adityasaky adityasaky deleted the check-per-entry branch October 11, 2024 16:09
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.

3 participants