Repository: cloudflare/cfssl
Pull request #1136 (2020-10-13 22:42:29)

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.


Patch:
diff --git a/certdb/certdb.go b/certdb/certdb.go
index e9a6f292..9c3afb2b 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 d5b878aa..a0c45b73 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/doc/api/endpoint_revoke.txt b/doc/api/endpoint_revoke.txt
index 32b09428..7eaf6de9 100644
--- a/doc/api/endpoint_revoke.txt
+++ b/doc/api/endpoint_revoke.txt
@@ -5,10 +5,10 @@ Method:   POST
 
 Required parameters:
 
-    * serial: a string specifying the serial number of a certificate in decimal format
+    * serial: a string specifying the serial number of a certificate
     * authority_key_id: a string specifying the authority key identifier
       of the certificate to be revoked; this is used to distinguish
-      which private key was used to sign the certificate. Should be formatted without colons and all lowercase.
+      which private key was used to sign the certificate.
     * reason: a string identifying why the certificate was revoked; see,
       for example, ReasonStringToCode in the ocsp package or section
       4.2.1.13 of RFC 5280. The "reasons" used here are the ReasonFlag
@@ -21,6 +21,6 @@ Result:
 Example:
 
     $ curl -d '{"serial": "7961067322630364137",        \
-            "authority_key_id": "a0b1c2d3e4f5",              \
+            "authority_key_id": "00:01:02:03:04:05:07",              \
             "reason": "superseded"}'                    \
           ${CFSSL_HOST}/api/v1/cfssl/revoke
diff --git a/signer/local/local.go b/signer/local/local.go
index b611cf1d..d0dbad18 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},
 		}
 


Test patch:
diff --git a/certdb/sql/sql_test.go b/certdb/sql/sql_test.go
index acf39573..38238a3c 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 {