Recycle pending authorizations (letsencrypt#2797)

If the feature flag "ReusePendingAuthz" is enabled, a request to create a new authorization object from an account that already has a pending authorization object for the same identifier will return the already-existing authorization object. This should make it less common for people to get stuck in the "too many pending authorizations" state, and reduce DB storage growth.

Fixes letsencrypt#2768

cmd/boulder-sa/main.go

@@ -57,7 +57,7 @@ func main() {
 
 	go sa.ReportDbConnCount(dbMap, scope)
 
-	sai, err := sa.NewSQLStorageAuthority(dbMap, clock.Default(), logger)
+	sai, err := sa.NewSQLStorageAuthority(dbMap, clock.Default(), logger, scope)
 	cmd.FailOnError(err, "Failed to create SA impl")
 
 	var grpcSrv *grpc.Server

cmd/cert-checker/main_test.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/letsencrypt/boulder/core"
 	blog "github.com/letsencrypt/boulder/log"
+	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/policy"
 	"github.com/letsencrypt/boulder/sa"
 	"github.com/letsencrypt/boulder/sa/satest"
@@ -182,7 +183,7 @@ func TestGetAndProcessCerts(t *testing.T) {
 	fc := clock.NewFake()
 
 	checker := newChecker(saDbMap, fc, pa, expectedValidityPeriod)
-	sa, err := sa.NewSQLStorageAuthority(saDbMap, fc, blog.NewMock())
+	sa, err := sa.NewSQLStorageAuthority(saDbMap, fc, blog.NewMock(), metrics.NewNoopScope())
 	test.AssertNotError(t, err, "Couldn't create SA to insert certificates")
 	saCleanUp := test.ResetSATestDatabase(t)
 	defer func() {

cmd/contact-exporter/main_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/jmhodges/clock"
 	"github.com/letsencrypt/boulder/core"
 	blog "github.com/letsencrypt/boulder/log"
+	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/sa"
 	"github.com/letsencrypt/boulder/test"
 	"github.com/letsencrypt/boulder/test/vars"
@@ -330,7 +331,7 @@ func setup(t *testing.T) testCtx {
 	cleanUp := test.ResetSATestDatabase(t)
 
 	fc := newFakeClock(t)
-	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log)
+	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log, metrics.NewNoopScope())
 	if err != nil {
 		t.Fatalf("unable to create SQLStorageAuthority: %s", err)
 	}

cmd/expiration-mailer/main_test.go

@@ -865,7 +865,7 @@ func setup(t *testing.T, nagTimes []time.Duration) *testCtx {
 		t.Fatalf("Couldn't connect the database: %s", err)
 	}
 	fc := newFakeClock(t)
-	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log)
+	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log, metrics.NewNoopScope())
 	if err != nil {
 		t.Fatalf("unable to create SQLStorageAuthority: %s", err)
 	}

cmd/expired-authz-purger/main_test.go

@@ -24,7 +24,7 @@ func TestPurgeAuthzs(t *testing.T) {
 	log := blog.UseMock()
 	fc := clock.NewFake()
 	fc.Add(time.Hour)
-	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log)
+	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log, metrics.NewNoopScope())
 	if err != nil {
 		t.Fatalf("unable to create SQLStorageAuthority: %s", err)
 	}

cmd/ocsp-updater/main_test.go

@@ -122,7 +122,7 @@ func setup(t *testing.T) (*OCSPUpdater, core.StorageAuthority, *gorp.DbMap, cloc
 	fc := clock.NewFake()
 	fc.Add(1 * time.Hour)
 
-	sa, err := sa.NewSQLStorageAuthority(dbMap, fc, log)
+	sa, err := sa.NewSQLStorageAuthority(dbMap, fc, log, metrics.NewNoopScope())
 	test.AssertNotError(t, err, "Failed to create SA")
 
 	cleanUp := test.ResetSATestDatabase(t)

features/features.go

@@ -20,6 +20,9 @@ const (
 	UseAIAIssuerURL
 	AllowTLS02Challenges
 	GenerateOCSPEarly
+	// For new-authz requests, if there is no valid authz, but there is a pending
+	// authz, return that instead of creating a new one.
+	ReusePendingAuthz
 	CountCertificatesExact
 	RandomDirectoryEntry
 	IPv6First
@@ -37,6 +40,7 @@ var features = map[FeatureFlag]bool{
 	UseAIAIssuerURL:          false,
 	AllowTLS02Challenges:     false,
 	GenerateOCSPEarly:        false,
+	ReusePendingAuthz:        false,
 	CountCertificatesExact:   false,
 	RandomDirectoryEntry:     false,
 	IPv6First:                false,

ra/ra_test.go

@@ -222,7 +222,7 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAut
 	if err != nil {
 		t.Fatalf("Failed to create dbMap: %s", err)
 	}
-	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log)
+	ssa, err := sa.NewSQLStorageAuthority(dbMap, fc, log, metrics.NewNoopScope())
 	if err != nil {
 		t.Fatalf("Failed to create SA: %s", err)
 	}

sa/database_test.go

@@ -63,6 +63,9 @@ func TestNewDbMap(t *testing.T) {
 
 func TestStrictness(t *testing.T) {
 	dbMap, err := NewDbMap(vars.DBConnSA, 1)
+	if err != nil {
+		t.Fatal(err)
+	}
 	_, err = dbMap.Exec(`insert into authz set
 		id="hi", identifier="foo", status="pending", combinations="combos",
 		registrationID=999999999999999999999999999;`)

sa/sa.go

@@ -20,6 +20,7 @@ import (
 	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/features"
 	blog "github.com/letsencrypt/boulder/log"
+	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/revocation"
 	sapb "github.com/letsencrypt/boulder/sa/proto"
 )
@@ -29,6 +30,7 @@ type SQLStorageAuthority struct {
 	dbMap *gorp.DbMap
 	clk   clock.Clock
 	log   blog.Logger
+	scope metrics.Scope
 }
 
 func digest256(data []byte) []byte {
@@ -50,13 +52,19 @@ type authzModel struct {
 
 // NewSQLStorageAuthority provides persistence using a SQL backend for
 // Boulder. It will modify the given gorp.DbMap by adding relevant tables.
-func NewSQLStorageAuthority(dbMap *gorp.DbMap, clk clock.Clock, logger blog.Logger) (*SQLStorageAuthority, error) {
+func NewSQLStorageAuthority(
+	dbMap *gorp.DbMap,
+	clk clock.Clock,
+	logger blog.Logger,
+	scope metrics.Scope,
+) (*SQLStorageAuthority, error) {
 	SetSQLDebug(dbMap, logger)
 
 	ssa := &SQLStorageAuthority{
 		dbMap: dbMap,
 		clk:   clk,
 		log:   logger,
+		scope: scope,
 	}
 
 	return ssa, nil
@@ -634,15 +642,38 @@ func (ssa *SQLStorageAuthority) UpdateRegistration(ctx context.Context, reg core
 	return nil
 }
 
-// NewPendingAuthorization stores a new Pending Authorization
+// NewPendingAuthorization retrieves a pending authorization for
+// authz.Identifier if one exists, or creates a new one otherwise.
 func (ssa *SQLStorageAuthority) NewPendingAuthorization(ctx context.Context, authz core.Authorization) (core.Authorization, error) {
 	var output core.Authorization
+
+	// Check if we can recycle an existing, pending authz.
+	if features.Enabled(features.ReusePendingAuthz) {
+		idJSON, err := json.Marshal(authz.Identifier)
+		if err != nil {
+			return output, err
+		}
+
+		pa, err := selectPendingAuthz(ssa.dbMap, "WHERE identifier = ?", idJSON)
+		if err == sql.ErrNoRows {
+			// No existing authz found, proceed to create one.
+		} else if err == nil {
+			// We found an authz, but we still need to fetch its challenges. To
+			// simplify things, just call GetAuthorization, which takes care of that.
+			ssa.scope.Inc("reused_authz", 1)
+			return ssa.GetAuthorization(ctx, pa.ID)
+		} else {
+			// Any error other than ErrNoRows; return the error
+			return output, err
+		}
+	}
+
 	tx, err := ssa.dbMap.Begin()
 	if err != nil {
 		return output, err
 	}
 
-	// Check that it doesn't exist already
+	// Create a random ID and check that it doesn't exist already
 	authz.ID = core.NewToken()
 	for existingPending(tx, authz.ID) || existingFinal(tx, authz.ID) {
 		authz.ID = core.NewToken()

sa/sa_test.go

@@ -24,6 +24,7 @@ import (
 	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/features"
 	blog "github.com/letsencrypt/boulder/log"
+	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/revocation"
 	sapb "github.com/letsencrypt/boulder/sa/proto"
 	"github.com/letsencrypt/boulder/sa/satest"
@@ -45,7 +46,7 @@ func initSA(t *testing.T) (*SQLStorageAuthority, clock.FakeClock, func()) {
 	fc := clock.NewFake()
 	fc.Set(time.Date(2015, 3, 4, 5, 0, 0, 0, time.UTC))
 
-	sa, err := NewSQLStorageAuthority(dbMap, fc, log)
+	sa, err := NewSQLStorageAuthority(dbMap, fc, log, metrics.NewNoopScope())
 	if err != nil {
 		t.Fatalf("Failed to create SA: %s", err)
 	}
@@ -204,6 +205,58 @@ func TestAddAuthorization(t *testing.T) {
 	test.AssertNotError(t, err, "Couldn't get authorization with ID "+PA.ID)
 }
 
+func TestRecyclePendingDisabled(t *testing.T) {
+	sa, _, cleanUp := initSA(t)
+	defer cleanUp()
+
+	reg := satest.CreateWorkingRegistration(t, sa)
+	pendingAuthz, err := sa.NewPendingAuthorization(ctx, core.Authorization{RegistrationID: reg.ID})
+
+	test.AssertNotError(t, err, "Couldn't create new pending authorization")
+	test.Assert(t, pendingAuthz.ID != "", "ID shouldn't be blank")
+
+	pendingAuthz2, err := sa.NewPendingAuthorization(ctx, core.Authorization{RegistrationID: reg.ID})
+
+	test.AssertNotError(t, err, "Couldn't create new pending authorization")
+	test.AssertNotEquals(t, pendingAuthz.ID, pendingAuthz2.ID)
+}
+
+func TestRecyclePendingEnabled(t *testing.T) {
+	_ = features.Set(map[string]bool{"ReusePendingAuthz": true})
+
+	sa, _, cleanUp := initSA(t)
+	defer cleanUp()
+
+	reg := satest.CreateWorkingRegistration(t, sa)
+	authz := core.Authorization{
+		RegistrationID: reg.ID,
+		Identifier: core.AcmeIdentifier{
+			Type:  "dns",
+			Value: "example.letsencrypt.org",
+		},
+		Challenges: []core.Challenge{
+			core.Challenge{
+				URI:    "https://acme-example.letsencrypt.org/challenge123",
+				Type:   "http-01",
+				Status: "pending",
+				Token:  "abc",
+			},
+		},
+	}
+	pendingAuthz, err := sa.NewPendingAuthorization(ctx, authz)
+
+	test.AssertNotError(t, err, "Couldn't create new pending authorization")
+	test.Assert(t, pendingAuthz.ID != "", "ID shouldn't be blank")
+
+	authz.Challenges = nil
+	pendingAuthz2, err := sa.NewPendingAuthorization(ctx, authz)
+
+	test.AssertNotError(t, err, "Couldn't create new pending authorization")
+	test.AssertEquals(t, pendingAuthz.ID, pendingAuthz2.ID)
+	test.Assert(t, len(pendingAuthz.Challenges) > 0, "no challenges")
+	test.AssertEquals(t, pendingAuthz.Challenges[0].Token, "abc")
+}
+
 func CreateDomainAuth(t *testing.T, domainName string, sa *SQLStorageAuthority) (authz core.Authorization) {
 	return CreateDomainAuthWithRegID(t, domainName, sa, 42)
 }