Repository: sigstore/sigstore-go (go)

Issue:
SCT verification should compare timestamp against CT log validity window
**Description**

<!--What needs to be improved? What should be done? Has there been any related work
before? Any relevant links or material?
-->

As a reminder, the purpose of validity windows is to mitigate two risks:

1. An old signing key for a service is compromised and produces an artifact after the service is turned down
2. A service that was turned down is brought back online and issues a cert or proof after it's been marked as rotated

I've been looking over the code and wanted to confirm if we are verifying validity windows for root metadata. Here's what I've found so far:

* TSA
  * The TSA validity window is compared to the TSA timestamp - https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/tsa.go#L106-L111
  * [**Potential bug**] There is no comparison for the intermediate and root certificates.
* Rekor
  * The Rekor validity window is compared to the entry integrated time from the SET - https://github.com/sigstore/sigstore-go/blob/main/pkg/tlog/entry.go#L271-L277
  * [**Discussion topic**] There is no timestamp comparison during verification of a proof because there is no SET. We should not use the integrated timestamp because it is not signed. This means that we cannot verify a log proof was generated during a validity window.
* Fulcio
  * CA validity window is compared to the certificate's not before/after - https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/certificate.go#L27-L32
  * [**Potential bug**] There is no comparison for the intermediate and root certificates.
* CT log
  * [**Potential bug**] It doesn't look like we compare the SCT timestamp to the CT log validity window in https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/sct.go

In terms of how to structure the code, as is currently implemented, each service (CA, CT log, Rekor, TSA) should be responsible for comparing any of its artifacts that contain timestamps against the validity windows. TODOs below:

- [x] TSA compared against issued signed timestamps
- [x] TSA compared for each certificate in its chain
- [x] Rekor compared against issued SETs
- [x] CA compared against issued leaf certificates
- [x] CA compared for the root and intermediate CA certificates
- [ ] CT log compares against issued SCTs

The metadata I don't know how to verify is if a log proof was created during a log's validity window, since there is no timestamp. Using a certificate's timestamp is not accurate because a certificate may be logged long after a signing event. In the case of BYO PKI, there may also be no timestamps, if you log a signing event with a key. I think there's nothing to compare against in these cases.

Pull request:
Adds a check to ensure SCT time is while a CT log key was valid
<!--
Thanks for opening a pull request! Please do not just delete this text.  The three fields below are mandatory.

Please remember to:
- This PR requires an issue. If it is a new feature, the issue should proceed the PR and will have allowed sufficient time for discussions to take place. Please use
issue tags such as "Closes #XYZ" or "Resolves sigstore/repo-name#XYZ".
  [documentation](https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
- ensure your commits are signed-off, as sigstore uses the [DCO](https://en.wikipedia.org/wiki/Developer_Certificate_of_Origin) using `git commit -s`, or `git commit -s --amend` if you want to amend already existing commits
- lastly, ensure there are no merge commits!

Thank you :)
-->

#### Summary
<!--
 Explain the **motivation** for making this change. What existing problem does the pull request solve? How can reviewers test this PR?
-->

Fixes #178 - we want to make sure the SCT we're verifying with a CT log key happened while the CT log key was valid.

#### Release Note
<!--
Add a release note for each of the following conditions:

* Config changes (additions, deletions, updates)
* API additions—new endpoint, new response fields, or newly accepted request parameters
* Database changes (any)
* Websocket additions or changes
* Anything noteworthy to an administrator running private sigstore instances (err on the side of over-communicating)
* New features and improvements, including behavioural changes, UI changes and CLI changes
* Bug fixes and fixes of previous known issues
* Deprecation warnings, breaking changes, or compatibility notes

If no release notes are required write NONE. Use past-tense.

-->

NONE

#### Documentation
<!--

Does this change require an update to documentation? How will users implement your new feature?

Please reference a PR within https://docs.sigstore.dev

-->
N/A

Patch:
diff --git a/pkg/verify/sct.go b/pkg/verify/sct.go
index 64e2600..bf447c2 100644
--- a/pkg/verify/sct.go
+++ b/pkg/verify/sct.go
@@ -20,6 +20,7 @@ import (
 	"errors"
 	"fmt"
 
+	ct "github.com/google/certificate-transparency-go"
 	"github.com/google/certificate-transparency-go/ctutil"
 	ctx509 "github.com/google/certificate-transparency-go/x509"
 	"github.com/google/certificate-transparency-go/x509util"
@@ -58,6 +59,17 @@ func VerifySignedCertificateTimestamp(chains [][]*x509.Certificate, threshold in
 			continue
 		}
 
+		// Ensure sct is within ctlog validity window
+		sctTime := ct.TimestampToTime(sct.Timestamp)
+		if !key.ValidityPeriodStart.IsZero() && sctTime.Before(key.ValidityPeriodStart) {
+			// skip entries that were before ctlog key start time
+			continue
+		}
+		if !key.ValidityPeriodEnd.IsZero() && sctTime.After(key.ValidityPeriodEnd) {
+			// skip entries that were after ctlog key end time
+			continue
+		}
+
 		for _, chain := range chains {
 			fulcioChain := make([]*ctx509.Certificate, len(leafCTCert))
 			copy(fulcioChain, leafCTCert)


Test patch:
diff --git a/pkg/verify/sct_test.go b/pkg/verify/sct_test.go
index 2ef224e..0f031c1 100644
--- a/pkg/verify/sct_test.go
+++ b/pkg/verify/sct_test.go
@@ -25,6 +25,7 @@ import (
 	"encoding/hex"
 	"math/big"
 	"testing"
+	"time"
 
 	ct "github.com/google/certificate-transparency-go"
 	"github.com/google/certificate-transparency-go/tls"
@@ -123,7 +124,9 @@ func TestVerifySignedCertificateTimestamp(t *testing.T) {
 			trustedMaterial: &fakeTrustedMaterial{
 				transparencyLog: map[string]*root.TransparencyLog{
 					hex.EncodeToString(logID[:]): {
-						PublicKey: &privateKey.PublicKey,
+						PublicKey:           &privateKey.PublicKey,
+						ValidityPeriodStart: time.UnixMilli(12344),
+						ValidityPeriodEnd:   time.UnixMilli(12346),
 					},
 				},
 				cas: []root.CertificateAuthority{
@@ -314,6 +317,35 @@ func TestVerifySignedCertificateTimestamp(t *testing.T) {
 			threshold:       0,
 			trustedMaterial: &fakeTrustedMaterial{},
 		},
+		{
+			name: "sct not valid for ctlog key time range",
+			getCertFn: func() *x509.Certificate {
+				return embedSCTs(t, privateKey, skid, createBaseCert(t, privateKey, skid, big.NewInt(1)), []ct.SignedCertificateTimestamp{
+					{
+						SCTVersion: ct.V1,
+						Timestamp:  12345,
+						LogID:      ct.LogID{KeyID: logID},
+					},
+				})
+			},
+			chain:     []*x509.Certificate{caCert},
+			threshold: 1,
+			trustedMaterial: &fakeTrustedMaterial{
+				transparencyLog: map[string]*root.TransparencyLog{
+					hex.EncodeToString(logID[:]): {
+						PublicKey:           &privateKey.PublicKey,
+						ValidityPeriodStart: time.UnixMilli(10),
+						ValidityPeriodEnd:   time.UnixMilli(10000),
+					},
+				},
+				cas: []root.CertificateAuthority{
+					&root.FulcioCertificateAuthority{
+						Root: caCert,
+					},
+				},
+			},
+			wantErr: true,
+		},
 		{
 			name: "threshold of 2 with 2 valid scts",
 			getCertFn: func() *x509.Certificate {