From e8f9337d2c132653bedd6368b60a1a161a5d278c Mon Sep 17 00:00:00 2001
From: Nicky Semenza <nicky@cloudflare.com>
Date: Tue, 13 Oct 2020 15:36:41 -0700
Subject: [PATCH] fix(certdb): allow reading other null columns (part 2 of
 #1135)

This follows up #1135 to properly handle the case when columns written pre-migration have null values.
---
 certdb/certdb.go                | 21 +++++++++++----------
 certdb/sql/database_accessor.go | 13 +++++++++++--
 certdb/sql/sql_test.go          |  7 ++++++-
 signer/local/local.go           |  5 +++--
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/certdb/certdb.go b/certdb/certdb.go
index e9a6f292e..9c3afb2b3 100644
--- a/certdb/certdb.go
+++ b/certdb/certdb.go
@@ -11,16 +11,17 @@ import (
 // CertificateRecord encodes a certificate and its metadata
 // that will be recorded in a database.
 type CertificateRecord struct {
-	Serial       string         `db:"serial_number"`
-	AKI          string         `db:"authority_key_identifier"`
-	CALabel      string         `db:"ca_label"`
-	Status       string         `db:"status"`
-	Reason       int            `db:"reason"`
-	Expiry       time.Time      `db:"expiry"`
-	RevokedAt    time.Time      `db:"revoked_at"`
-	PEM          string         `db:"pem"`
-	IssuedAt     time.Time      `db:"issued_at"`
-	NotBefore    time.Time      `db:"not_before"`
+	Serial    string    `db:"serial_number"`
+	AKI       string    `db:"authority_key_identifier"`
+	CALabel   string    `db:"ca_label"`
+	Status    string    `db:"status"`
+	Reason    int       `db:"reason"`
+	Expiry    time.Time `db:"expiry"`
+	RevokedAt time.Time `db:"revoked_at"`
+	PEM       string    `db:"pem"`
+	// the following fields will be empty for data inserted before migrate 002 has been run.
+	IssuedAt     *time.Time     `db:"issued_at"`
+	NotBefore    *time.Time     `db:"not_before"`
 	MetadataJSON types.JSONText `db:"metadata"`
 	SANsJSON     types.JSONText `db:"sans"`
 	CommonName   sql.NullString `db:"common_name"`
diff --git a/certdb/sql/database_accessor.go b/certdb/sql/database_accessor.go
index d5b878aa7..a0c45b731 100644
--- a/certdb/sql/database_accessor.go
+++ b/certdb/sql/database_accessor.go
@@ -101,6 +101,15 @@ func (d *Accessor) InsertCertificate(cr certdb.CertificateRecord) error {
 		return err
 	}
 
+	var issuedAt, notBefore *time.Time
+	if cr.IssuedAt != nil {
+		t := cr.IssuedAt.UTC()
+		issuedAt = &t
+	}
+	if cr.NotBefore != nil {
+		t := cr.NotBefore.UTC()
+		notBefore = &t
+	}
 	res, err := d.db.NamedExec(insertSQL, &certdb.CertificateRecord{
 		Serial:       cr.Serial,
 		AKI:          cr.AKI,
@@ -110,8 +119,8 @@ func (d *Accessor) InsertCertificate(cr certdb.CertificateRecord) error {
 		Expiry:       cr.Expiry.UTC(),
 		RevokedAt:    cr.RevokedAt.UTC(),
 		PEM:          cr.PEM,
-		IssuedAt:     cr.IssuedAt.UTC(),
-		NotBefore:    cr.NotBefore.UTC(),
+		IssuedAt:     issuedAt,
+		NotBefore:    notBefore,
 		MetadataJSON: cr.MetadataJSON,
 		SANsJSON:     cr.SANsJSON,
 		CommonName:   cr.CommonName,
diff --git a/certdb/sql/sql_test.go b/certdb/sql/sql_test.go
index acf39573c..38238a3cf 100644
--- a/certdb/sql/sql_test.go
+++ b/certdb/sql/sql_test.go
@@ -172,7 +172,12 @@ func testInsertCertificateAndGetUnexpiredCertificateNullCommonName(ta TestAccess
 	}
 
 	// simulate situation where there are rows before migrate 002 has been run
-	ta.DB.MustExec("update certificates set common_name = NULL")
+	ta.DB.MustExec(`update certificates
+	set issued_at = NULL,
+	not_before = NULL,
+	metadata = NULL,
+	sans = NULL,
+	common_name = NULL;`)
 
 	rets, err := ta.Accessor.GetCertificate(want.Serial, want.AKI)
 	if err != nil {
diff --git a/signer/local/local.go b/signer/local/local.go
index b611cf1dd..d0dbad18c 100644
--- a/signer/local/local.go
+++ b/signer/local/local.go
@@ -507,6 +507,7 @@ func (s *Signer) Sign(req signer.SignRequest) (cert []byte, err error) {
 	parsedCert, _ := helpers.ParseCertificatePEM(signedCert)
 
 	if s.dbAccessor != nil {
+		now := time.Now()
 		var certRecord = certdb.CertificateRecord{
 			Serial: certTBS.SerialNumber.String(),
 			// this relies on the specific behavior of x509.CreateCertificate
@@ -516,8 +517,8 @@ func (s *Signer) Sign(req signer.SignRequest) (cert []byte, err error) {
 			Status:     "good",
 			Expiry:     certTBS.NotAfter,
 			PEM:        string(signedCert),
-			IssuedAt:   time.Now(),
-			NotBefore:  certTBS.NotBefore,
+			IssuedAt:   &now,
+			NotBefore:  &certTBS.NotBefore,
 			CommonName: sql.NullString{String: certTBS.Subject.CommonName, Valid: true},
 		}