Add interface types for TimestampingAuthority and CertificateAuthority #300
Conversation
Signed-off-by: Cody Soyland <codysoyland@github.com>
Signed-off-by: Cody Soyland <codysoyland@github.com>
|
|
||
| timestampingAuthority.URI = certAuthority.Uri | ||
|
|
||
| // TODO: Should we inspect/enforce ca.Subject? |
There was a problem hiding this comment.
Can we either file issues for these TODOs or decide if these are needed?
My two cents:
- Subject is more of a human-readable value and not something we need to check. It's also the subject of the root, so there's not really anything to verify, as we implicitly trust the root.
- What is there to handle with the validity period? It seems like the above is reading it.
| @@ -0,0 +1,64 @@ | |||
| // Copyright 2023 The Sigstore Authors. | |||
There was a problem hiding this comment.
Can we add tests for this file and the next? Given these are verification functions, having some coverage would be good.
|
|
||
| scts, err := x509util.ParseSCTsFromCertificate(leafCert.Raw) | ||
| scts, err := x509util.ParseSCTsFromCertificate(chains[0][0].Raw) |
There was a problem hiding this comment.
Can we add a comment noting that chains[][0] is always the leaf certificate? This just stands out as looking problematic when it should be safe.
| @@ -98,39 +84,16 @@ func VerifyTimestampAuthorityWithThreshold(entity SignedEntity, trustedMaterial | |||
| return verifiedTimestamps, nil | |||
| } | |||
|
|
|||
| func verifySignedTimestamp(signedTimestamp []byte, dsseSignatureBytes []byte, trustedMaterial root.TrustedMaterial, verificationContent VerificationContent) (Timestamp, error) { | |||
| certAuthorities := trustedMaterial.TimestampingAuthorities() | |||
| func verifySignedTimestamp(signedTimestamp []byte, dsseSignatureBytes []byte, trustedMaterial root.TrustedMaterial) (*root.Timestamp, error) { | |||
There was a problem hiding this comment.
nit on naming, this isn't always a dsse signature
|
|
||
| // Check tlog entry time against bundle certificates | ||
| // TODO: technically no longer needed since we check the cert validity period in the main Verify loop | ||
| if !verificationContent.ValidAtTime(timestamp.Time, trustedMaterial) { |
There was a problem hiding this comment.
Just wanted to check, we removed this - this was intentional? I agree that I think it's fine to remove as we should decouple certificate and timestamp verification.
This PR modifies the
TrustedMaterialinterface with interface types forCertificateAuthorityandTimestampingAuthority. This decouples the originalCertificateAuthoritystruct from the verifier and allows clients to have more control over the verification of certificates and timestamps.Additionally, this PR modifies the verifier to output the chains of certificates that were used to verify the certificate, which has the added benefit of improving performance of the SCT verifier by only attempting SCT verification on the chain that was used to verify the certificate.
This is a breaking change to the
TrustedMaterialinterface, so clients that directly modifyTrustedMaterialmust be updated to use the new interface types.Fixes #293
Summary
Release Note
Documentation