Clean up certificate timestamp comparisons

[haydentherapper]

Description

There are three places we compare certificates against SET or TSA timestamps:

• We aggregate all timestamps from SETs and TSAs and verify the certificate's validity with those timestamps here. The timestamp needs to fall in the range of "Not Before" <= timestamp <= "Not After" (go source code)
• The code signing certificate is compared against the TSA timestamp here, which does the same comparison against "not before" and "not after"
• We verify the certificate against the SET timestamp here, which will do the same not before/after comparison.

The latter two are redundant. The first uses all aggregated timestamps, while the latter two do comparisons for each respective timestamp (TSA or SET).

[cmurphy]

I could be misunderstanding but I think each of these instances are different from each other and necessary:

Verify() -> VerifyLeafCertificate() (link) - aggregates SETs and TSAs and verifies them against the fulcio CA

Verify() -> VerifyObserverTimestamps() -> VerifyTimestampAuthority() -> verifySignedTimestamp() (link) - verifies each timestamp, if any, against the bundle certificates. The signed entity doesn't always have a timestamps issued by a TSA. I think the comment here is mistaken.

Verify() -> VerifyTransparencyLogInclusion() -> VerifyArtifactTransparencyLog() (link) - verifies the tlog entry time against the bundle certificates

@haydentherapper do you still think these are superfluous?

[haydentherapper]

For the first, the SET and TSA timestamps are used to verify the code-signing/leaf certificate. That timestamp, CurrentTime, will be used when comparing NotBefore and NotAfter (go source link). The CA certificate validity window is also compared (link).

That is the same check that we do in the second, comparing the certificate's NotBefore and NotAfter (link). That will be the same as the third check as well, just using the tlog entry timestamp instead.

I think the latter two are still superfluous. With that said, I think there are two things to discuss:

• As an API, should we do timestamp comparisons as part of VerifyTimestampAuthority and VerifyArtifactTransparencyLog, even if it's repeated in signed_entity.Verify()? I'd lean towards no, that the responsibility of each of these functions is to verify either a signed timestamp and return the timestamp or verify log inclusion and return a timestamp if available, and what to do with the timestamps are for another API (which is VerifyLeafCertificate).
• We've mostly been thinking about certificate verification, but not verification with keys. Keys provided as part of the verification material will only be a hint, not the key itself, so there is no validity window (link). However when constructing a trust root, you can provide an expiring key (example), and when comparing a bundle's verification material that contains a key, it will look up that expiring key by the hint (link). If we do remove these additional timestamp checks from the latter two APIs, then we would need to add back support for comparing the key's expiration somewhere, since we only do timestamp comparisons if the verification content contains a certificate.

Thoughts? Either we need to a) update the comments on the APIs and leave everything as is, or b) remove the timestamp comparisons and also add timestamp comparisons back for key verification.

[cmurphy]

@haydentherapper would you mind updating the links in the original issue with permalinks so I can be sure I'm looking at the same code you're talking about? The first seems to link to a call to VerifyObserverTimestamps and the second seems to link to another function that's called from within VerifyObserverTimestamps so it's not clicking for me where the duplication is.

[haydentherapper]

@cmurphy Updated the links in the first comment, lemme know if that helps.