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

Return errors from TSA verification if there are no valid timestamps #325

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

codysoyland
Copy link
Member

Summary

Release Note

Documentation

Signed-off-by: Cody Soyland <codysoyland@github.com>
@codysoyland codysoyland requested a review from a team as a code owner October 29, 2024 16:23
Signed-off-by: Cody Soyland <codysoyland@github.com>
@codysoyland
Copy link
Member Author

There is still a test failure here, as the test expects that there is no returned error even if there are no valid timestamps (as the count of timestamps is used to test against the threshold, which may be zero). Should we update that test? Or is there a valid reason to expect that behavior?

@phillmv
Copy link
Member

phillmv commented Oct 29, 2024

as the test expects that there is no returned error even if there are no valid timestamps (as the count of timestamps is used to test against the threshold, which may be zero). Should we update that test? Or is there a valid reason to expect that behavior?

Thinking out loud, should VerifyTimestampAuthority error if there are no entity.Timestamps() to verify?

On the one hand, I'm inclined to say "yes". It feels dangerous to be have a function named "Verify" that can return successfully when it has not actually verified anything. I probably had a hand in writing this function, so I will admit to this being a principle or vibe I only recently adopted; this behaviour got us in trouble in gh at verify, where we returned successfully when supplied with an empty list of attestations.

On the other hand, as currently implemented there is some ambiguity around how VerifyObserverTimestamps calls this function; as currently implemented, returning a zero-len array is expected behaviour. However I'm not a huge fan of how VerifyObserverTimestamps ended up & would be willing to endorse a polite refactor - do we remember why the TlogInclusion is verified independently of this function?

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

This was set up intentionally to support verification with multiple timestamp sources. VerifyTimestampAuthorityWithThreshold requires a threshold, while VerifyTimestampAuthority does not. VerifyTimestampAuthority must be used with another function that will implement a threshold, which is done here.

If we add a baseline threshold check to VerifyTimestampAuthority, then verification would fail if both TSA timestamps and SETs are provided where the trust root only specifies verification material for SETs.

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