SA: conditionally track issued names in AddPrecertificate. (#4573)

Prev. we inserted data for tracking issued names into the `issuedNames` table
during `sa.AddCertificate`. A more robust solution is to do this during
`sa.AddPrecertificate` since this is when we've truly committed to having
issued for the names.

The new SA `WriteIssuedNamesPrecert` feature flag enables writing this table
during `AddPrecertificate`. The legacy behaviour continues with the flag
enabled or disabled but is updated to tolerate duplicate INSERT errors so that
it is possible to deploy this change across multiple SA instances safely.

Along the way I also updated `SA.AddPrecertificate` to perform its two
`INSERT`s in a transaction using the `db.WithTransaction` wrapper.

Resolves https://github.com/letsencrypt/boulder/issues/4565

cmd/ocsp-updater/main_test.go

@@ -7,7 +7,6 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
-	"io/ioutil"
 	"math/big"
 	"os"
 	"strings"
@@ -450,16 +449,16 @@ func TestGenerateOCSPResponsePrecert(t *testing.T) {
 
 	reg := satest.CreateWorkingRegistration(t, sa)
 
+	// Create a throw-away self signed certificate with some names
+	serial, testCert := test.ThrowAwayCert(t, 5)
+
 	// 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{
+	_, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
-		Der:    certDER,
+		Der:    testCert.Raw,
 		RegID:  &regID,
 		Ocsp:   ocspResp,
 		Issued: &issuedTime,

features/featureflag_string.go

@@ -39,11 +39,12 @@ func _() {
 	_ = x[PrecertificateRevocation-28]
 	_ = x[StripDefaultSchemePort-29]
 	_ = x[StoreIssuerInfo-30]
+	_ = x[WriteIssuedNamesPrecert-31]
 }
 
-const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRANewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitFasterGetOrderForNamesPrecertificateOCSPGetAuthorizationsPerfCAAValidationMethodsCAAAccountURIHeadNonceStatusOKEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGETAllowV1RegistrationParallelCheckFailedValidationDeleteUnusedChallengesV1DisableNewValidationsPrecertificateRevocationStripDefaultSchemePortStoreIssuerInfo"
+const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRANewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitFasterGetOrderForNamesPrecertificateOCSPGetAuthorizationsPerfCAAValidationMethodsCAAAccountURIHeadNonceStatusOKEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGETAllowV1RegistrationParallelCheckFailedValidationDeleteUnusedChallengesV1DisableNewValidationsPrecertificateRevocationStripDefaultSchemePortStoreIssuerInfoWriteIssuedNamesPrecert"
 
-var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 178, 197, 216, 238, 256, 277, 297, 310, 327, 341, 359, 378, 395, 413, 432, 461, 483, 506, 530, 552, 567}
+var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 178, 197, 216, 238, 256, 277, 297, 310, 327, 341, 359, 378, 395, 413, 432, 461, 483, 506, 530, 552, 567, 590}
 
 func (i FeatureFlag) String() string {
 	if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) {

features/features.go

@@ -69,6 +69,10 @@ const (
 	// StoreIssuerInfo enables storage of information identifying the issuer of
 	// a certificate in the certificateStatus table.
 	StoreIssuerInfo
+	// WriteIssuedNamesPrecert moves the issuedNames and fqdnSet insertions from
+	// happening when final certificates are saved by the SA to when
+	// precertificates are saved.
+	WriteIssuedNamesPrecert
 )
 
 // List of features and their default value, protected by fMu
@@ -104,6 +108,7 @@ var features = map[FeatureFlag]bool{
 	StripDefaultSchemePort:        false,
 	GetAuthorizationsPerf:         false,
 	StoreIssuerInfo:               false,
+	WriteIssuedNamesPrecert:       false,
 }
 
 var fMu = new(sync.RWMutex)

sa/precertificates.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/letsencrypt/boulder/core"
 	corepb "github.com/letsencrypt/boulder/core/proto"
+	"github.com/letsencrypt/boulder/db"
 	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/features"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
@@ -49,56 +50,87 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb
 	}
 	issued := time.Unix(0, *req.Issued)
 	serialHex := core.SerialToString(parsed.SerialNumber)
-	err = ssa.dbMap.WithContext(ctx).Insert(&precertificateModel{
+
+	preCertModel := &precertificateModel{
 		Serial:         serialHex,
 		RegistrationID: *req.RegID,
 		DER:            req.Der,
 		Issued:         issued,
 		Expires:        parsed.NotAfter,
-	})
+	}
-	if err != nil {
+
-		if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
+	_, overallError := db.WithTransaction(ctx, ssa.dbMap, func(txWithCtx db.Transaction) (interface{}, error) {
-			return nil, berrors.DuplicateError("cannot add a duplicate precertificate")
+		err = txWithCtx.Insert(preCertModel)
+		if err != nil {
+			if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
+				return nil, berrors.DuplicateError("cannot add a duplicate precertificate")
+			}
+			return nil, err
 		}
-		return nil, err
-	}
 
-	// With feature.StoreIssuerInfo we've added a new field to certStatusModel
+		// With feature.StoreIssuerInfo we've added a new field to certStatusModel
-	// so when we try and use dbMap.Insert it will always try to insert that field.
+		// so when we try and use dbMap.Insert it will always try to insert that field.
-	// That will break when the relevant migration hasn't been applied so we need
+		// That will break when the relevant migration hasn't been applied so we need
-	// to use an explicit INSERT statement that we can manipulate to include the
+		// to use an explicit INSERT statement that we can manipulate to include the
-	// field only when the feature is enabled (and as such the migration has been
+		// field only when the feature is enabled (and as such the migration has been
-	// applied).
+		// applied).
-	csFields := certStatusFields
+		csFields := certStatusFields
-	if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil {
+		if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil {
-		csFields += ", issuerID"
+			csFields += ", issuerID"
-	}
+		}
-	qmarks := []string{}
+		qmarks := []string{}
-	for range strings.Split(csFields, ",") {
+		for range strings.Split(csFields, ",") {
-		qmarks = append(qmarks, "?")
+			qmarks = append(qmarks, "?")
-	}
+		}
-	args := []interface{}{
+		args := []interface{}{
-		serialHex,                   // serial
+			serialHex,                   // serial
-		string(core.OCSPStatusGood), // stauts
+			string(core.OCSPStatusGood), // status
-		ssa.clk.Now(),               // ocspLastUpdated
+			ssa.clk.Now(),               // ocspLastUpdated
-		time.Time{},                 // revokedDate
+			time.Time{},                 // revokedDate
-		0,                           // revokedReason
+			0,                           // revokedReason
-		time.Time{},                 // lastExpirationNagSent
+			time.Time{},                 // lastExpirationNagSent
-		req.Ocsp,                    // ocspResponse
+			req.Ocsp,                    // ocspResponse
-		parsed.NotAfter,             // notAfter
+			parsed.NotAfter,             // notAfter
-		false,                       // isExpired
+			false,                       // isExpired
-	}
+		}
-	if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil {
+		if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil {
-		args = append(args, req.IssuerID)
+			args = append(args, req.IssuerID)
-	}
+		}
 
-	_, err = ssa.dbMap.WithContext(ctx).Exec(fmt.Sprintf(
+		_, err = ssa.dbMap.WithContext(ctx).Exec(fmt.Sprintf(
-		"INSERT INTO certificateStatus (%s) VALUES (%s)",
+			"INSERT INTO certificateStatus (%s) VALUES (%s)",
-		csFields,
+			csFields,
-		strings.Join(qmarks, ","),
+			strings.Join(qmarks, ","),
-	), args...)
+		), args...)
-	if err != nil {
+		if err != nil {
-		return nil, err
+			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
+		// if a certificate we issued were to have a Subj. CN not present as a SAN it
+		// would be a misissuance and miscalculating whether the cert is a renewal or
+		// not for the purpose of rate limiting is the least of our troubles.
+		isRenewal, err := ssa.checkFQDNSetExists(
+			txWithCtx.SelectOne,
+			parsed.DNSNames)
+		if err != nil {
+			return nil, err
+		}
+
+		// If the WriteIssuedNamesPrecert feature flag is *enabled* then we need to
+		// record the issued names from the precertificate being added by the SA.
+		if features.Enabled(features.WriteIssuedNamesPrecert) {
+			if err := addIssuedNames(txWithCtx, parsed, isRenewal); err != nil {
+				return nil, err
+			}
+		}
+
+		return nil, nil
+	})
+	if overallError != nil {
+		return nil, overallError
 	}
 	return &corepb.Empty{}, nil
 }

sa/precertificates_test.go

@@ -2,19 +2,35 @@ package sa
 
 import (
 	"bytes"
+	"database/sql"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/letsencrypt/boulder/db"
 	berrors "github.com/letsencrypt/boulder/errors"
+	"github.com/letsencrypt/boulder/features"
 	sapb "github.com/letsencrypt/boulder/sa/proto"
 	"github.com/letsencrypt/boulder/sa/satest"
 	"github.com/letsencrypt/boulder/test"
 )
 
+// findIssuedName is a small helper test function to directly query the
+// issuedNames table for a given name to find a serial (or return an err).
+func findIssuedName(dbMap db.OneSelector, name string) (string, error) {
+	var issuedNamesSerial string
+	err := dbMap.SelectOne(
+		&issuedNamesSerial,
+		`SELECT serial FROM issuedNames
+		WHERE reversedName = ?
+		ORDER BY notBefore DESC
+		LIMIT 1`,
+		ReverseName(name))
+	return issuedNamesSerial, err
+}
+
 func TestAddPrecertificate(t *testing.T) {
 	if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") {
 		return
@@ -25,43 +41,79 @@ func TestAddPrecertificate(t *testing.T) {
 
 	reg := satest.CreateWorkingRegistration(t, sa)
 
-	certDER, err := ioutil.ReadFile("test-cert2.der")
+	addPrecert := func(expectIssuedNamesUpdate bool) {
-	test.AssertNotError(t, err, "Couldn't read example cert DER")
+		// Create a throw-away self signed certificate with a random name and
-	serial := "ffa0160630d618b2eb5c0510824b14274856"
+		// serial number
-	ocspResp := []byte{0, 0, 1}
+		serial, testCert := test.ThrowAwayCert(t, 1)
-	regID := reg.ID
-	issuedTime := time.Date(2018, 4, 1, 7, 0, 0, 0, time.UTC).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")
 
-	certStatus, err := sa.GetCertificateStatus(ctx, serial)
+		// Add the cert as a precertificate
-	test.AssertNotError(t, err, "Couldn't get status for test-cert2.der")
+		ocspResp := []byte{0, 0, 1}
-	test.Assert(
+		regID := reg.ID
-		t,
+		issuedTime := time.Date(2018, 4, 1, 7, 0, 0, 0, time.UTC)
-		bytes.Compare(certStatus.OCSPResponse, ocspResp) == 0,
+		issuedTimeNano := issuedTime.UnixNano()
-		fmt.Sprintf("OCSP responses don't match, expected: %x, got %x", certStatus.OCSPResponse, ocspResp),
+		_, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
-	)
+			Der:    testCert.Raw,
-	test.Assert(
+			RegID:  &regID,
-		t,
+			Ocsp:   ocspResp,
-		clk.Now().Equal(certStatus.OCSPLastUpdated),
+			Issued: &issuedTimeNano,
-		fmt.Sprintf("OCSPLastUpdated doesn't match, expected %s, got %s", clk.Now(), certStatus.OCSPLastUpdated),
+		})
-	)
+		test.AssertNotError(t, err, "Couldn't add test cert")
 
-	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		// It should have the expected certificate status
-		Der:    certDER,
+		certStatus, err := sa.GetCertificateStatus(ctx, serial)
-		RegID:  &regID,
+		test.AssertNotError(t, err, "Couldn't get status for test cert")
-		Ocsp:   ocspResp,
+		test.Assert(
-		Issued: &issuedTime,
+			t,
-	})
+			bytes.Compare(certStatus.OCSPResponse, ocspResp) == 0,
-	if err == nil {
+			fmt.Sprintf("OCSP responses don't match, expected: %x, got %x", certStatus.OCSPResponse, ocspResp),
-		t.Fatalf("Expected error inserting duplicate precertificate, got none")
+		)
-	}
+		test.Assert(
-	if !berrors.Is(err, berrors.Duplicate) {
+			t,
-		t.Fatalf("Expected berrors.Duplicate inserting duplicate precertificate, got %#v", err)
+			clk.Now().Equal(certStatus.OCSPLastUpdated),
+			fmt.Sprintf("OCSPLastUpdated doesn't match, expected %s, got %s", clk.Now(), certStatus.OCSPLastUpdated),
+		)
+
+		issuedNamesSerial, err := findIssuedName(sa.dbMap, testCert.DNSNames[0])
+		if expectIssuedNamesUpdate {
+			// If we expectIssuedNamesUpdate then there should be no err and the
+			// expected serial
+			test.AssertNotError(t, err, "expected no err querying issuedNames for precert")
+			test.AssertEquals(t, issuedNamesSerial, serial)
+
+			// We should also be able to call AddCertificate with the same cert
+			// without it being an error. The duplicate err on inserting to
+			// issuedNames should be ignored.
+			_, err := sa.AddCertificate(ctx, testCert.Raw, regID, nil, &issuedTime)
+			test.AssertNotError(t, err, "unexpected err adding final cert after precert")
+		} else {
+			// Otherwise we expect sql.ErrNoRows because AddCertificate not
+			// AddPrecertificate will be updating this table.
+			test.AssertEquals(t, err, sql.ErrNoRows)
+		}
+
+		// Adding the same certificate with the same serial should result in an
+		// error
+		_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+			Der:    testCert.Raw,
+			RegID:  &regID,
+			Ocsp:   ocspResp,
+			Issued: &issuedTimeNano,
+		})
+		if err == nil {
+			t.Fatalf("Expected error inserting duplicate precertificate, got none")
+		}
+		if !berrors.Is(err, berrors.Duplicate) {
+			t.Fatalf("Expected berrors.Duplicate inserting duplicate precertificate, got %#v", err)
+		}
 	}
+
+	// With no feature flag set we don't expect addPrecertficate to update
+	// issuedNames
+	addPrecert(false)
+	defer features.Reset()
+	// With the feature flag set we do expect addPrecertificate to update
+	// issuedNames
+	err := features.Set(map[string]bool{"WriteIssuedNamesPrecert": true})
+	test.AssertNotError(t, err, "failed to set WriteIssuedNamesPrecert feature flag")
+	addPrecert(true)
 }

sa/sa.go

@@ -463,6 +463,7 @@ func (ssa *SQLStorageAuthority) AddCertificate(
 	}
 
 	_, overallError := db.WithTransaction(ctx, ssa.dbMap, func(txWithCtx db.Transaction) (interface{}, error) {
+		// Save the final certificate
 		err = txWithCtx.Insert(cert)
 		if err != nil {
 			if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
@@ -484,33 +485,41 @@ func (ssa *SQLStorageAuthority) AddCertificate(
 			return nil, err
 		}
 
-		err = addIssuedNames(txWithCtx, parsedCertificate, isRenewal)
+		// Record the issued names from the final certificate being added by the SA.
-		if err != nil {
+		// If features.WriteIssuedNamesPrecert was enabled when the corresponding
-			return nil, err
+		// precertificate was added in AddPrecertificate then this will prompt
+		// a duplicate entry error that we can safely ignore.
+		//
+		// TODO(@cpu): Once features.WriteIssuedNamesPrecert has been deployed
+		// globally we can remove this call to ssa.addIssuedNames from
+		// AddCertificate
+		if err := addIssuedNames(txWithCtx, parsedCertificate, isRenewal); err != nil {
+			// if it wasn't a duplicate entry error, return the err. Otherwise ignore
+			// it.
+			if !strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") {
+				return nil, err
+			}
 		}
 
 		// Add to the rate limit table, but only for new certificates. Renewals
 		// don't count against the certificatesPerName limit.
 		if !isRenewal {
 			timeToTheHour := parsedCertificate.NotBefore.Round(time.Hour)
-			err = ssa.addCertificatesPerName(ctx, txWithCtx, parsedCertificate.DNSNames, timeToTheHour)
+			if err := ssa.addCertificatesPerName(ctx, txWithCtx, parsedCertificate.DNSNames, timeToTheHour); err != nil {
-			if err != nil {
 				return nil, err
 			}
 		}
 
+		// Update the FQDN sets now that there is a final certificate to ensure rate
+		// limits are calculated correctly.
 		err = addFQDNSet(
 			txWithCtx,
 			parsedCertificate.DNSNames,
-			serial,
+			core.SerialToString(parsedCertificate.SerialNumber),
 			parsedCertificate.NotBefore,
 			parsedCertificate.NotAfter,
 		)
-		if err != nil {
+		return nil, err
-			return nil, err
-		}
-
-		return digest, nil
 	})
 	if overallError != nil {
 		return "", overallError

sa/sa_test.go

@@ -185,6 +185,7 @@ func TestNoSuchRegistrationErrors(t *testing.T) {
 		t.Errorf("UpdateRegistration: expected a berrors.NotFound type error, got %T type error (%v)", err, err)
 	}
 }
+
 func TestAddCertificate(t *testing.T) {
 	sa, clk, cleanUp := initSA(t)
 	defer cleanUp()
@@ -233,6 +234,26 @@ func TestAddCertificate(t *testing.T) {
 	ocspResp := []byte{0, 0, 1}
 	_, err = sa.AddCertificate(ctx, certDER3, reg.ID, ocspResp, &issuedTime)
 	test.AssertNotError(t, err, "Couldn't add test-cert2.der")
+
+	// Test adding a certificate with the features.WriteIssuedNamesPrecert feature
+	// flag enabled doesn't result in issuedNames and fqdnSet updates since we
+	// expect AddPrecertificate to handle it in this case.
+	err = features.Set(map[string]bool{"WriteIssuedNamesPrecert": true})
+	test.AssertNotError(t, err, "failed to set WriteIssuedNamesPrecert feature flag")
+
+	// Create a throw-away self signed certificate with a random name and
+	// serial number
+	_, testCert := test.ThrowAwayCert(t, 1)
+
+	// Add the test cert
+	_, err = sa.AddCertificate(ctx, testCert.Raw, reg.ID, ocspResp, &issuedTime)
+	test.AssertNotError(t, err, "unexpected error adding testcert")
+
+	// Check the issuedNames table
+	_, err = findIssuedName(sa.dbMap, testCert.DNSNames[0])
+	// We expect no error because AddCertificate should have updated the issued
+	// names table.
+	test.AssertNotError(t, err, "unexpected error finding issued names after addCert")
 }
 
 func TestCountCertificatesByNames(t *testing.T) {

test/config-next/sa.json

@@ -26,7 +26,8 @@
     "features": {
       "DeleteUnusedChallenges": true,
       "FasterGetOrderForNames": true,
-      "StoreIssuerInfo": true
+      "StoreIssuerInfo": true,
+      "WriteIssuedNamesPrecert": true
     }
   },
 

test/test-tools.go

@@ -2,9 +2,14 @@ package test
 
 import (
 	"bytes"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
 	"encoding/base64"
+	"encoding/hex"
 	"encoding/json"
 	"fmt"
+	"math/big"
 	"reflect"
 	"runtime"
 	"strings"
@@ -204,3 +209,34 @@ func GaugeValueWithLabels(vecGauge *prometheus.GaugeVec, labels prometheus.Label
 
 	return int(iom.Gauge.GetValue()), nil
 }
+
+// ThrowAwayCert is a small test helper function that creates a self-signed
+// certificate for nameCount random example.com subdomains and returns the
+// parsed certificate  and the random serial in string form or aborts the test.
+// The certificate returned from this function is the bare minimum needed for
+// most tests and isn't a robust example of a complete end entity certificate.
+func ThrowAwayCert(t *testing.T, nameCount int) (string, *x509.Certificate) {
+	k, err := rsa.GenerateKey(rand.Reader, 512)
+	AssertNotError(t, err, "rsa.GenerateKey failed")
+
+	var serialBytes [16]byte
+	_, _ = rand.Read(serialBytes[:])
+	serialNum := big.NewInt(0).SetBytes(serialBytes[:])
+
+	var names []string
+	for i := 0; i < nameCount; i++ {
+		var nameBytes [3]byte
+		_, _ = rand.Read(nameBytes[:])
+		names = append(names, fmt.Sprintf("%s.example.com", hex.EncodeToString(nameBytes[:])))
+	}
+
+	template := &x509.Certificate{
+		SerialNumber: serialNum,
+		DNSNames:     names,
+	}
+	testCertDER, err := x509.CreateCertificate(rand.Reader, template, template, &k.PublicKey, k)
+	AssertNotError(t, err, "x509.CreateCertificate failed")
+	testCert, err := x509.ParseCertificate(testCertDER)
+	AssertNotError(t, err, "failed to parse self-signed cert DER")
+	return fmt.Sprintf("%036x", serialNum), testCert
+}