search

sigstore / sigstore-go

Go library for Sigstore signing and verification
Apache License 2.0
49 stars 26 forks source link

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

Open codysoyland opened 4 weeks ago

codysoyland commented 4 weeks ago

Summary

Release Note

Documentation

codysoyland commented 4 weeks ago

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 commented 4 weeks ago

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?