ocsp-updater: fix generateResponse for precerts w/o certs (#4468)

Since 9906c93217 when
`features.PrecertificateOCSP` is enabled it is possible for there to be
`certificateStatus` rows that correspond to `precertificates` that do not have
a matching final `certificates` row. This happens in the case where we began
serving OCSP for a precert and weren't able to issue a final certificate.

Prior to the fix in this branch when the `ocsp-updater` would find stale OCSP
responses by querying the `certificateStatus` table it would error in
`generateResponse` when it couldn't find a matching `certificates` row. This
branch updates the logic so that when `features.PrecertificateOCSP` is enabled
it will also try finding the ocsp update DER from the `precertificates` table
when there is no matching serial in the `certificates` table.

cmd/ocsp-updater/main.go

@@ -161,7 +161,20 @@ func (updater *OCSPUpdater) generateResponse(ctx context.Context, status core.Ce
 		status.Serial,
 	)
 	if err != nil {
-		return nil, err
+		// If PrecertificateOCSP is enabled and the error indicates there was no
+		// certificates table row then try to find a precertificate table row before
+		// giving up with an error.
+		if features.Enabled(features.PrecertificateOCSP) && err == sql.ErrNoRows {
+			cert, err = sa.SelectPrecertificate(updater.dbMap, status.Serial)
+			// If there was still a non-nil error return it. If we can't find
+			// a precert row something is amiss, we have a certificateStatus row with
+			// no matching certificate or precertificate.
+			if err != nil {
+				return nil, err
+			}
+		} else {
+			return nil, err
+		}
 	}
 
 	signRequest := core.OCSPSigningRequest{

cmd/ocsp-updater/main_test.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io/ioutil"
+	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -12,6 +15,7 @@ import (
 	caPB "github.com/letsencrypt/boulder/ca/proto"
 	"github.com/letsencrypt/boulder/cmd"
 	"github.com/letsencrypt/boulder/core"
+	"github.com/letsencrypt/boulder/features"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/sa"
@@ -402,3 +406,69 @@ func TestLoopTickBackoff(t *testing.T) {
 	test.AssertEquals(t, l.failures, 0)
 	test.AssertEquals(t, l.clk.Now(), start)
 }
+
+func TestGenerateOCSPResponsePrecert(t *testing.T) {
+	// The schema required to insert a precertificate is only available in
+	// config-next at the time of writing.
+	if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
+		return
+	}
+
+	updater, sa, dbMap, fc, cleanUp := setup(t)
+	defer cleanUp()
+
+	reg := satest.CreateWorkingRegistration(t, sa)
+
+	// Use AddPrecertificate to set up a precertificate, serials, and
+	// certificateStatus row for the testcert.
+	certDER, err := ioutil.ReadFile("../../test/test-ca.der")
+	test.AssertNotError(t, err, "Couldn't read example cert DER")
+	serial := "00000000000000000000000000000000124d"
+	ocspResp := []byte{0, 0, 1}
+	regID := reg.ID
+	issuedTime := fc.Now().UnixNano()
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    certDER,
+		RegID:  &regID,
+		Ocsp:   ocspResp,
+		Issued: &issuedTime,
+	})
+	test.AssertNotError(t, err, "Couldn't add test-cert2.der")
+
+	// We need to set a fake "ocspLastUpdated" value for the precert we created
+	// in order to satisfy the "ocspStaleMaxAge" constraint.
+	fakeLastUpdate := fc.Now().Add(-time.Hour * 24 * 3)
+	_, err = dbMap.Exec(
+		"UPDATE certificateStatus SET ocspLastUpdated = ? WHERE serial = ?",
+		fakeLastUpdate,
+		serial)
+	test.AssertNotError(t, err, "Couldn't update ocspLastUpdated")
+
+	// There should be one stale ocsp response found for the precert
+	earliest := fc.Now().Add(-time.Hour)
+	certs, err := updater.findStaleOCSPResponses(earliest, 10)
+	test.AssertNotError(t, err, "Couldn't find stale responses")
+	test.AssertEquals(t, len(certs), 1)
+	test.AssertEquals(t, certs[0].Serial, serial)
+
+	// Disable PrecertificateOCSP.
+	err = features.Set(map[string]bool{"PrecertificateOCSP": false})
+	test.AssertNotError(t, err, "setting PrecertificateOCSP feature to off")
+
+	// Directly call generateResponse with the result, when the PrecertificateOCSP
+	// feature flag is disabled we expect this to error because no matching
+	// certificates row will be found.
+	updater.cac = &mockCA{time.Second}
+	_, err = updater.generateResponse(ctx, certs[0])
+	test.AssertError(t, err, "generateResponse for precert without PrecertificateOCSP did not error")
+
+	// Now enable PrecertificateOCSP.
+	err = features.Set(map[string]bool{"PrecertificateOCSP": true})
+	test.AssertNotError(t, err, "setting PrecertificateOCSP feature to off")
+
+	// Directly call generateResponse again with the same result. It should not
+	// error and should instead update the precertificate's OCSP status even
+	// though no certificate row exists.
+	_, err = updater.generateResponse(ctx, certs[0])
+	test.AssertNotError(t, err, "generateResponse for precert with PrecertificateOCSP errored")
+}

sa/model.go

@@ -134,9 +134,9 @@ func SelectCertificate(s dbOneSelector, q string, args ...interface{}) (core.Cer
 
 const precertFields = "registrationID, serial, der, issued, expires"
 
-// selectPrecertificate selects all fields of one precertificate object
+// SelectPrecertificate selects all fields of one precertificate object
 // identified by serial.
-func selectPrecertificate(s dbOneSelector, serial string) (core.Certificate, error) {
+func SelectPrecertificate(s dbOneSelector, serial string) (core.Certificate, error) {
 	var model precertificateModel
 	err := s.SelectOne(
 		&model,

sa/sa.go

@@ -472,7 +472,7 @@ func (ssa *SQLStorageAuthority) GetPrecertificate(ctx context.Context, reqSerial
 		return nil,
 			fmt.Errorf("Invalid precertificate serial %q", *reqSerial.Serial)
 	}
-	cert, err := selectPrecertificate(ssa.dbMap.WithContext(ctx), *reqSerial.Serial)
+	cert, err := SelectPrecertificate(ssa.dbMap.WithContext(ctx), *reqSerial.Serial)
 	if err == sql.ErrNoRows {
 		return nil,
 			berrors.NotFoundError("precertificate with serial %q not found", *reqSerial.Serial)

test/config-next/ocsp-updater.json

@@ -27,6 +27,7 @@
       "timeout": "15s"
     },
     "features": {
+      "PrecertificateOCSP": true
     }
   },