Repository: sigstore/sigstore-go (go)

Issue:
Validity periods are handled incorrectly
This was found during #457: The handling for ValidFor time ranges is incorrect in multiple places

* The code uses `Time.IsZero()` as a proxy for _"was the time stamp value unset in the protobuf?"_
* This is incorrect because IsZero compares to date 01-01-01 whereas the the protobuf Timestamp default value is 1970-01-01
* This issue impacts all ValidFor fields in SigningConfig but also impacts at least Timestamp authority validity verification
* Most verification path cases do not seem impacted since default end dates are not meaningful in e.g. rekor log instances (we verify rekor entries even if we are past the logs best before date) and because in some cases this "magic value" difference does seem to get handled

**Potential fixes:** 

* The minimal fix for this would be to consider 19070-01-01 a magic date value and replace all checks for `IsZero()` with a comparison to the magic date
* A more reasonable but larger fix would be to create a TimeRange type that can handle unset start and end dates and manages the comparison

 I will have a look at how big of a job the latter one is

Pull request description:
Fix SigningConfig ValidFor when dates are missing
Fixes #464: Make sure we handle missing dates in ValidFor protobuf as a special case. The time that we deserialize in that case needs to be 01-01-01 and not the protobuf default 1970-01-01 because we want to be able to check for it with `Time.IsZero()`

Patch:
diff --git a/pkg/root/signing_config.go b/pkg/root/signing_config.go
index 1490e14..6842618 100644
--- a/pkg/root/signing_config.go
+++ b/pkg/root/signing_config.go
@@ -46,6 +46,27 @@ type ServiceConfiguration struct {
 	Count    uint32
 }
 
+func NewService(s *prototrustroot.Service) Service {
+	validFor := s.GetValidFor()
+
+	var start time.Time
+	if validFor.GetStart() != nil {
+		start = validFor.GetStart().AsTime()
+	}
+
+	var end time.Time
+	if validFor.GetEnd() != nil {
+		end = validFor.GetEnd().AsTime()
+	}
+
+	return Service{
+		URL:                 s.GetUrl(),
+		MajorAPIVersion:     s.GetMajorApiVersion(),
+		ValidityPeriodStart: start,
+		ValidityPeriodEnd:   end,
+	}
+}
+
 // SelectService returns which service endpoint should be used based on supported API versions
 // and current time. It will select the first service with the highest API version that matches
 // the criteria. Services should be sorted from newest to oldest validity period start time, to
@@ -178,13 +199,9 @@ func (sc ServiceConfiguration) ToConfigProtobuf() *prototrustroot.ServiceConfigu
 
 func (sc *SigningConfig) FulcioCertificateAuthorityURLs() []Service {
 	var services []Service
+
 	for _, s := range sc.signingConfig.GetCaUrls() {
-		services = append(services, Service{
-			URL:                 s.GetUrl(),
-			MajorAPIVersion:     s.GetMajorApiVersion(),
-			ValidityPeriodStart: s.GetValidFor().GetStart().AsTime(),
-			ValidityPeriodEnd:   s.GetValidFor().GetEnd().AsTime(),
-		})
+		services = append(services, NewService(s))
 	}
 	return services
 }
@@ -192,12 +209,7 @@ func (sc *SigningConfig) FulcioCertificateAuthorityURLs() []Service {
 func (sc *SigningConfig) OIDCProviderURLs() []Service {
 	var services []Service
 	for _, s := range sc.signingConfig.GetOidcUrls() {
-		services = append(services, Service{
-			URL:                 s.GetUrl(),
-			MajorAPIVersion:     s.GetMajorApiVersion(),
-			ValidityPeriodStart: s.GetValidFor().GetStart().AsTime(),
-			ValidityPeriodEnd:   s.GetValidFor().GetEnd().AsTime(),
-		})
+		services = append(services, NewService(s))
 	}
 	return services
 }
@@ -205,12 +217,7 @@ func (sc *SigningConfig) OIDCProviderURLs() []Service {
 func (sc *SigningConfig) RekorLogURLs() []Service {
 	var services []Service
 	for _, s := range sc.signingConfig.GetRekorTlogUrls() {
-		services = append(services, Service{
-			URL:                 s.GetUrl(),
-			MajorAPIVersion:     s.GetMajorApiVersion(),
-			ValidityPeriodStart: s.GetValidFor().GetStart().AsTime(),
-			ValidityPeriodEnd:   s.GetValidFor().GetEnd().AsTime(),
-		})
+		services = append(services, NewService(s))
 	}
 	return services
 }
@@ -226,12 +233,7 @@ func (sc *SigningConfig) RekorLogURLsConfig() ServiceConfiguration {
 func (sc *SigningConfig) TimestampAuthorityURLs() []Service {
 	var services []Service
 	for _, s := range sc.signingConfig.GetTsaUrls() {
-		services = append(services, Service{
-			URL:                 s.GetUrl(),
-			MajorAPIVersion:     s.GetMajorApiVersion(),
-			ValidityPeriodStart: s.GetValidFor().GetStart().AsTime(),
-			ValidityPeriodEnd:   s.GetValidFor().GetEnd().AsTime(),
-		})
+		services = append(services, NewService(s))
 	}
 	return services
 }
@@ -247,14 +249,7 @@ func (sc *SigningConfig) TimestampAuthorityURLsConfig() ServiceConfiguration {
 func (sc *SigningConfig) WithFulcioCertificateAuthorityURLs(fulcioURLs ...Service) *SigningConfig {
 	var services []*prototrustroot.Service
 	for _, u := range fulcioURLs {
-		services = append(services, &prototrustroot.Service{
-			Url:             u.URL,
-			MajorApiVersion: u.MajorAPIVersion,
-			ValidFor: &v1.TimeRange{
-				Start: timestamppb.New(u.ValidityPeriodStart),
-				End:   timestamppb.New(u.ValidityPeriodEnd),
-			},
-		})
+		services = append(services, u.ToServiceProtobuf())
 	}
 	sc.signingConfig.CaUrls = services
 	return sc


Test patch:
diff --git a/pkg/root/signing_config_test.go b/pkg/root/signing_config_test.go
index a339d67..e2baa4a 100644
--- a/pkg/root/signing_config_test.go
+++ b/pkg/root/signing_config_test.go
@@ -760,6 +760,26 @@ func TestSigningConfig_TimestampAuthorityURLs(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "valid, unset end date",
+			signingConfig: &prototrustroot.SigningConfig{
+				TsaUrls: []*prototrustroot.Service{
+					{
+						Url:             "https://timestamp.sigstore.dev",
+						MajorApiVersion: 1,
+						ValidFor:        &v1.TimeRange{Start: timestamppb.New(now), End: nil},
+					},
+				},
+			},
+			want: []Service{
+				{
+					URL:                 "https://timestamp.sigstore.dev",
+					MajorAPIVersion:     1,
+					ValidityPeriodStart: now,
+					ValidityPeriodEnd:   time.Time{},
+				},
+			},
+		},
 		{
 			name: "empty",
 			signingConfig: &prototrustroot.SigningConfig{
diff --git a/pkg/root/trusted_root_test.go b/pkg/root/trusted_root_test.go
index 38f33c6..5797ae0 100644
--- a/pkg/root/trusted_root_test.go
+++ b/pkg/root/trusted_root_test.go
@@ -206,3 +206,17 @@ func TestFromJSONToJSON(t *testing.T) {
 
 	assert.JSONEq(t, trJSONTrimmedTime, string(jsonBytes))
 }
+
+func TestValidityPeriods(t *testing.T) {
+	trustedrootJSON, err := os.ReadFile("../../examples/trusted-root-public-good.json")
+	assert.NoError(t, err)
+
+	trustedRoot, err := NewTrustedRootFromJSON(trustedrootJSON)
+	assert.NoError(t, err)
+
+	// confirm that ValidityPeriodEnd.IsZero() is true for services without end validity date
+	assert.True(t, trustedRoot.ctLogs["dd3d306ac6c7113263191e1c99673702a24a5eb8de3cadff878a72802f29ee8e"].ValidityPeriodEnd.IsZero())
+	assert.True(t, trustedRoot.certificateAuthorities[1].(*FulcioCertificateAuthority).ValidityPeriodEnd.IsZero())
+	assert.True(t, trustedRoot.rekorLogs["c0d23d6ad406973f9559f3ba2d1ca01f84147d8ffc5b8445c224f98b9591801d"].ValidityPeriodEnd.IsZero())
+	assert.True(t, trustedRoot.timestampingAuthorities[0].(*SigstoreTimestampingAuthority).ValidityPeriodEnd.IsZero())
+}