Cleanup leftovers from PrecertificateOCSP deprecation (#4551)

Cleans up a few things that were left out of #4465.

ca/ca.go

@@ -688,9 +688,8 @@ func (ca *CertificateAuthorityImpl) storeCertificate(
 	serialBigInt *big.Int,
 	certDER []byte) (core.Certificate, error) {
 	var err error
-	var ocspResp []byte
 	now := ca.clk.Now()
-	_, err = ca.sa.AddCertificate(ctx, certDER, regID, ocspResp, &now)
+	_, err = ca.sa.AddCertificate(ctx, certDER, regID, nil, &now)
 	if err != nil {
 		err = berrors.InternalServerError(err.Error())
 		// Note: This log line is parsed by cmd/orphan-finder. If you make any
@@ -699,9 +698,8 @@ func (ca *CertificateAuthorityImpl) storeCertificate(
 			core.SerialToString(serialBigInt), hex.EncodeToString(certDER), err, regID, orderID)
 		if ca.orphanQueue != nil {
 			ca.queueOrphan(&orphanedCert{
-				DER:      certDER,
+				DER:   certDER,
-				OCSPResp: ocspResp,
+				RegID: regID,
-				RegID:    regID,
 			})
 		}
 		return core.Certificate{}, err
@@ -773,7 +771,7 @@ func (ca *CertificateAuthorityImpl) integrateOrphan() error {
 			Issued: &issuedNanos,
 		})
 	} else {
-		_, err = ca.sa.AddCertificate(context.Background(), orphan.DER, orphan.RegID, orphan.OCSPResp, &issued)
+		_, err = ca.sa.AddCertificate(context.Background(), orphan.DER, orphan.RegID, nil, &issued)
 	}
 	if err != nil && !berrors.Is(err, berrors.Duplicate) {
 		return fmt.Errorf("failed to store orphaned certificate: %s", err)

ca/ca_test.go

@@ -960,8 +960,6 @@ func TestPrecertOrphanQueue(t *testing.T) {
 		orphanQueue)
 	test.AssertNotError(t, err, "Failed to create CA")
 
-	_ = features.Set(map[string]bool{"PrecertificateOCSP": true})
-
 	err = ca.integrateOrphan()
 	if err != goque.ErrEmpty {
 		t.Fatalf("Unexpected error, wanted %q, got %q", goque.ErrEmpty, err)

cmd/ocsp-updater/main.go

@@ -161,10 +161,7 @@ func (updater *OCSPUpdater) generateResponse(ctx context.Context, status core.Ce
 		status.Serial,
 	)
 	if err != nil {
-		// If PrecertificateOCSP is enabled and the error indicates there was no
+		if err == sql.ErrNoRows {
-		// 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

cmd/ocsp-updater/main_test.go

@@ -5,8 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
-	"os"
-	"strings"
 	"testing"
 	"time"
 
@@ -15,7 +13,6 @@ 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"
@@ -94,8 +91,13 @@ func TestGenerateAndStoreOCSPResponse(t *testing.T) {
 	reg := satest.CreateWorkingRegistration(t, sa)
 	parsedCert, err := core.LoadCert("test-cert.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	issued := fc.Now()
+	issued := fc.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, parsedCert.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCert.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert.pem")
 
 	status, err := sa.GetCertificateStatus(ctx, core.SerialToString(parsedCert.SerialNumber))
@@ -114,12 +116,22 @@ func TestGenerateOCSPResponses(t *testing.T) {
 	reg := satest.CreateWorkingRegistration(t, sa)
 	parsedCertA, err := core.LoadCert("test-cert.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	issued := fc.Now()
+	issued := fc.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, parsedCertA.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCertA.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert.pem")
 	parsedCertB, err := core.LoadCert("test-cert-b.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	_, err = sa.AddCertificate(ctx, parsedCertB.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCertB.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert-b.pem")
 
 	// We need to set a fake "ocspLastUpdated" value for the two certs we created
@@ -164,8 +176,13 @@ func TestFindStaleOCSPResponses(t *testing.T) {
 	reg := satest.CreateWorkingRegistration(t, sa)
 	parsedCert, err := core.LoadCert("test-cert.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	issued := fc.Now()
+	issued := fc.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, parsedCert.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCert.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert.pem")
 
 	// We need to set a fake "ocspLastUpdated" value for the cert we created
@@ -202,12 +219,22 @@ func TestFindStaleOCSPResponsesStaleMaxAge(t *testing.T) {
 	reg := satest.CreateWorkingRegistration(t, sa)
 	parsedCertA, err := core.LoadCert("test-cert.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	issued := fc.Now()
+	issued := fc.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, parsedCertA.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCertA.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert.pem")
 	parsedCertB, err := core.LoadCert("test-cert-b.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	_, err = sa.AddCertificate(ctx, parsedCertB.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCertB.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert-b.pem")
 
 	// Set a "ocspLastUpdated" value of 3 days ago for parsedCertA
@@ -243,8 +270,13 @@ func TestOldOCSPResponsesTick(t *testing.T) {
 	reg := satest.CreateWorkingRegistration(t, sa)
 	parsedCert, err := core.LoadCert("test-cert.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	issued := fc.Now()
+	issued := fc.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, parsedCert.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCert.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert.pem")
 
 	updater.ocspMinTimeToExpiry = 1 * time.Hour
@@ -270,8 +302,13 @@ func TestOldOCSPResponsesTickIsExpired(t *testing.T) {
 	serial := core.SerialToString(parsedCert.SerialNumber)
 
 	// Add a new test certificate
-	issued := fc.Now()
+	issued := fc.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, parsedCert.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCert.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert.pem")
 
 	// We need to set a fake "ocspLastUpdated" value for the cert we created
@@ -313,8 +350,13 @@ func TestStoreResponseGuard(t *testing.T) {
 	reg := satest.CreateWorkingRegistration(t, sa)
 	parsedCert, err := core.LoadCert("test-cert.pem")
 	test.AssertNotError(t, err, "Couldn't read test certificate")
-	issued := fc.Now()
+	issued := fc.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, parsedCert.Raw, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    parsedCert.Raw,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add test-cert.pem")
 
 	status, err := sa.GetCertificateStatus(ctx, core.SerialToString(parsedCert.SerialNumber))
@@ -396,12 +438,6 @@ func TestLoopTickBackoff(t *testing.T) {
 }
 
 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()
 
@@ -439,24 +475,9 @@ func TestGenerateOCSPResponsePrecert(t *testing.T) {
 	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.ogc = &mockOCSP{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")
+	test.AssertNotError(t, err, "generateResponse for precert errored")
 }

features/featureflag_string.go

@@ -22,27 +22,27 @@ func _() {
 	_ = x[DisableAuthz2Orders-11]
 	_ = x[EarlyOrderRateLimit-12]
 	_ = x[FasterGetOrderForNames-13]
-	_ = x[CAAValidationMethods-14]
+	_ = x[PrecertificateOCSP-14]
-	_ = x[CAAAccountURI-15]
+	_ = x[CAAValidationMethods-15]
-	_ = x[HeadNonceStatusOK-16]
+	_ = x[CAAAccountURI-16]
-	_ = x[EnforceMultiVA-17]
+	_ = x[HeadNonceStatusOK-17]
-	_ = x[MultiVAFullResults-18]
+	_ = x[EnforceMultiVA-18]
-	_ = x[RemoveWFE2AccountID-19]
+	_ = x[MultiVAFullResults-19]
-	_ = x[CheckRenewalFirst-20]
+	_ = x[RemoveWFE2AccountID-20]
-	_ = x[MandatoryPOSTAsGET-21]
+	_ = x[CheckRenewalFirst-21]
-	_ = x[AllowV1Registration-22]
+	_ = x[MandatoryPOSTAsGET-22]
-	_ = x[ParallelCheckFailedValidation-23]
+	_ = x[AllowV1Registration-23]
-	_ = x[DeleteUnusedChallenges-24]
+	_ = x[ParallelCheckFailedValidation-24]
-	_ = x[V1DisableNewValidations-25]
+	_ = x[DeleteUnusedChallenges-25]
-	_ = x[PrecertificateOCSP-26]
+	_ = x[V1DisableNewValidations-26]
 	_ = x[PrecertificateRevocation-27]
 	_ = x[StripDefaultSchemePort-28]
 	_ = x[GetAuthorizationsPerf-29]
 }
 
-const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRANewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitFasterGetOrderForNamesCAAValidationMethodsCAAAccountURIHeadNonceStatusOKEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGETAllowV1RegistrationParallelCheckFailedValidationDeleteUnusedChallengesV1DisableNewValidationsPrecertificateOCSPPrecertificateRevocationStripDefaultSchemePortGetAuthorizationsPerf"
+const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRANewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitFasterGetOrderForNamesPrecertificateOCSPCAAValidationMethodsCAAAccountURIHeadNonceStatusOKEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGETAllowV1RegistrationParallelCheckFailedValidationDeleteUnusedChallengesV1DisableNewValidationsPrecertificateRevocationStripDefaultSchemePortGetAuthorizationsPerf"
 
-var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 178, 197, 216, 238, 258, 271, 288, 302, 320, 339, 356, 374, 393, 422, 444, 467, 485, 509, 531, 552}
+var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 178, 197, 216, 238, 256, 276, 289, 306, 320, 338, 357, 374, 392, 411, 440, 462, 485, 509, 531, 552}
 
 func (i FeatureFlag) String() string {
 	if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) {

features/features.go

@@ -25,6 +25,7 @@ const (
 	DisableAuthz2Orders
 	EarlyOrderRateLimit
 	FasterGetOrderForNames
+	PrecertificateOCSP
 
 	//   Currently in-use features
 	// Check CAA and respect validationmethods parameter.
@@ -58,10 +59,6 @@ const (
 	// V1DisableNewValidations disables validations for new domain names in the V1
 	// API.
 	V1DisableNewValidations
-	// PrecertificateOCSP ensures that we write an OCSP response immediately upon
-	// generating a precertificate. This also changes the issuance / storage flow,
-	// adding two new calls from CA to SA: AddSerial and AddPrecertificate.
-	PrecertificateOCSP
 	// PrecertificateRevocation allows revocation of precertificates with the
 	// ACMEv2 interface.
 	PrecertificateRevocation

sa/sa.go

@@ -465,20 +465,6 @@ func (ssa *SQLStorageAuthority) AddCertificate(
 		Expires:        parsedCertificate.NotAfter,
 	}
 
-	certStatus := &certStatusModel{
-		Status:          core.OCSPStatus("good"),
-		OCSPLastUpdated: time.Time{},
-		OCSPResponse:    []byte{},
-		Serial:          serial,
-		RevokedDate:     time.Time{},
-		RevokedReason:   0,
-		NotAfter:        parsedCertificate.NotAfter,
-	}
-	if len(ocspResponse) != 0 {
-		certStatus.OCSPResponse = ocspResponse
-		certStatus.OCSPLastUpdated = ssa.clk.Now()
-	}
-
 	_, overallError := db.WithTransaction(ctx, ssa.dbMap, func(txWithCtx db.Transaction) (interface{}, error) {
 		err = txWithCtx.Insert(cert)
 		if err != nil {
@@ -488,16 +474,6 @@ func (ssa *SQLStorageAuthority) AddCertificate(
 			return nil, err
 		}
 
-		err = txWithCtx.Insert(certStatus)
-		if err != nil {
-			// We ignore "duplicate entry" on insert to the certificateStatus table
-			// because we may be inserting a certificate after a call to
-			// AddPrecertificate, which also adds a certificateStatus entry.
-			if !strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
-				return nil, err
-			}
-		}
-
 		// NOTE(@cpu): When we collect up names to check if an FQDN set exists (e.g.
 		// that it is a renewal) we use just the DNSNames from the certificate and
 		// ignore the Subject Common Name (if any). This is a safe assumption because

sa/sa_test.go

@@ -1,7 +1,6 @@
 package sa
 
 import (
-	"bytes"
 	"context"
 	"crypto/rand"
 	"crypto/rsa"
@@ -209,12 +208,6 @@ func TestAddCertificate(t *testing.T) {
 	// with an issued time equal to now
 	test.AssertEquals(t, retrievedCert.Issued, clk.Now())
 
-	certificateStatus, err := sa.GetCertificateStatus(ctx, "000000000000000000000000000000021bd4")
-	test.AssertNotError(t, err, "Couldn't get status for www.eff.org.der")
-	test.Assert(t, certificateStatus.Status == core.OCSPStatusGood, "OCSP Status should be good")
-	test.Assert(t, certificateStatus.OCSPLastUpdated.IsZero(), "OCSPLastUpdated should be nil")
-	test.AssertEquals(t, certificateStatus.NotAfter, retrievedCert.Expires)
-
 	// Test cert generated locally by Boulder / CFSSL, names [example.com,
 	// www.example.com, admin.example.com]
 	certDER2, err := ioutil.ReadFile("test-cert.der")
@@ -234,31 +227,12 @@ func TestAddCertificate(t *testing.T) {
 	// as the issued field.
 	test.AssertEquals(t, retrievedCert2.Issued, issuedTime)
 
-	certificateStatus2, err := sa.GetCertificateStatus(ctx, serial)
-	test.AssertNotError(t, err, "Couldn't get status for test-cert.der")
-	test.Assert(t, certificateStatus2.Status == core.OCSPStatusGood, "OCSP Status should be good")
-	test.Assert(t, certificateStatus2.OCSPLastUpdated.IsZero(), "OCSPLastUpdated should be nil")
-
 	// Test adding OCSP response with cert
 	certDER3, err := ioutil.ReadFile("test-cert2.der")
 	test.AssertNotError(t, err, "Couldn't read example cert DER")
-	serial = "ffa0160630d618b2eb5c0510824b14274856"
 	ocspResp := []byte{0, 0, 1}
 	_, err = sa.AddCertificate(ctx, certDER3, reg.ID, ocspResp, &issuedTime)
 	test.AssertNotError(t, err, "Couldn't add test-cert2.der")
-
-	certificateStatus3, err := sa.GetCertificateStatus(ctx, serial)
-	test.AssertNotError(t, err, "Couldn't get status for test-cert2.der")
-	test.Assert(
-		t,
-		bytes.Compare(certificateStatus3.OCSPResponse, ocspResp) == 0,
-		fmt.Sprintf("OCSP responses don't match, expected: %x, got %x", certificateStatus3.OCSPResponse, ocspResp),
-	)
-	test.Assert(
-		t,
-		clk.Now().Equal(certificateStatus3.OCSPLastUpdated),
-		fmt.Sprintf("OCSPLastUpdated doesn't match, expected %s, got %s", clk.Now(), certificateStatus3.OCSPLastUpdated),
-	)
 }
 
 func TestCountCertificatesByNames(t *testing.T) {
@@ -1592,8 +1566,13 @@ func TestRevokeCertificate(t *testing.T) {
 	// Add a cert to the DB to test with.
 	certDER, err := ioutil.ReadFile("www.eff.org.der")
 	test.AssertNotError(t, err, "Couldn't read example cert DER")
-	issued := sa.clk.Now()
+	issued := sa.clk.Now().UnixNano()
-	_, err = sa.AddCertificate(ctx, certDER, reg.ID, nil, &issued)
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:    certDER,
+		RegID:  &reg.ID,
+		Ocsp:   nil,
+		Issued: &issued,
+	})
 	test.AssertNotError(t, err, "Couldn't add www.eff.org.der")
 
 	serial := "000000000000000000000000000000021bd4"

test/config-next/ca-a.json

@@ -137,7 +137,6 @@
     "maxConcurrentRPCServerRequests": 100000,
     "orphanQueueDir": "/tmp/orphaned-certificates-a",
     "features": {
-      "PrecertificateOCSP": true
     }
   },
 

test/config-next/ca-b.json

@@ -138,7 +138,6 @@
     "maxConcurrentRPCServerRequests": 100000,
     "orphanQueueDir": "/tmp/orphaned-certificates-b",
     "features": {
-      "PrecertificateOCSP": true
     }
   },
 

test/config-next/ocsp-updater.json

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

test/integration/ocsp_test.go

@@ -13,10 +13,6 @@ import (
 
 func TestPrecertificateOCSP(t *testing.T) {
 	t.Parallel()
-	// This test is gated on the PrecertificateOCSP feature flag.
-	if !strings.Contains(os.Getenv("BOULDER_CONFIG_DIR"), "test/config-next") {
-		return
-	}
 	domain := random_domain()
 	err := ctAddRejectHost(domain)
 	if err != nil {