ra, wfe: use TimestampsForWindow to check renewal (#7888)

And in the RA, log the notBefore of the previous issuance.

To make this happen, I had to hoist the "check for previous certificate"
up a level into `issueCertificateOuter`. That meant I also had to hoist
the "split off a WithoutCancel context" logic all the way up to
`FinalizeOrder`.

ra/ra.go

@@ -337,6 +337,10 @@ type certificateRequestEvent struct {
 	// CertProfileHash is SHA256 sum over every exported field of an
 	// issuance.ProfileConfig, represented here as a hexadecimal string.
 	CertProfileHash string `json:",omitempty"`
+	// PreviousCertificateIssued is present when this certificate uses the same set
+	// of FQDNs as a previous certificate (from any account) and contains the
+	// notBefore of the most recent such certificate.
+	PreviousCertificateIssued time.Time `json:",omitempty"`
 }
 
 // certificateRevocationEvent is a struct for holding information that is logged
@@ -1010,6 +1014,10 @@ func (ra *RegistrationAuthorityImpl) FinalizeOrder(ctx context.Context, req *rap
 		// that it can wait for all goroutines to drain during shutdown.
 		ra.drainWG.Add(1)
 		go func() {
+			// The original context will be canceled in the RPC layer when FinalizeOrder returns,
+			// so split off a context that won't be canceled (and has its own timeout).
+			ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), ra.finalizeTimeout)
+			defer cancel()
 			_, err := ra.issueCertificateOuter(ctx, proto.Clone(order).(*corepb.Order), csr, logEvent)
 			if err != nil {
 				// We only log here, because this is in a background goroutine with
@@ -1135,9 +1143,23 @@ func (ra *RegistrationAuthorityImpl) issueCertificateOuter(
 	ra.inflightFinalizes.Inc()
 	defer ra.inflightFinalizes.Dec()
 
+	isRenewal := false
+	timestamps, err := ra.SA.FQDNSetTimestampsForWindow(ctx, &sapb.CountFQDNSetsRequest{
+		DnsNames: order.DnsNames,
+		Window:   durationpb.New(120 * 24 * time.Hour),
+		Limit:    1,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("checking if certificate is a renewal: %w", err)
+	}
+	if len(timestamps.Timestamps) > 0 {
+		isRenewal = true
+		logEvent.PreviousCertificateIssued = timestamps.Timestamps[0].AsTime()
+	}
+
 	// Step 3: Issue the Certificate
 	cert, cpId, err := ra.issueCertificateInner(
-		ctx, csr, order.CertificateProfileName, accountID(order.RegistrationID), orderID(order.Id))
+		ctx, csr, isRenewal, order.CertificateProfileName, accountID(order.RegistrationID), orderID(order.Id))
 
 	// Step 4: Fail the order if necessary, and update metrics and log fields
 	var result string
@@ -1245,16 +1267,10 @@ type certProfileID struct {
 func (ra *RegistrationAuthorityImpl) issueCertificateInner(
 	ctx context.Context,
 	csr *x509.CertificateRequest,
+	isRenewal bool,
 	profileName string,
 	acctID accountID,
 	oID orderID) (*x509.Certificate, *certProfileID, error) {
-	if features.Get().AsyncFinalize {
-		// If we're in async mode, use a context with a much longer timeout.
-		var cancel func()
-		ctx, cancel = context.WithTimeout(context.WithoutCancel(ctx), ra.finalizeTimeout)
-		defer cancel()
-	}
-
 	// wrapError adds a prefix to an error. If the error is a boulder error then
 	// the problem detail is updated with the prefix. Otherwise a new error is
 	// returned with the message prefixed using `fmt.Errorf`
@@ -1290,12 +1306,6 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner(
 		return nil, nil, wrapError(err, "getting SCTs")
 	}
 
-	exists, err := ra.SA.FQDNSetExists(ctx, &sapb.FQDNSetExistsRequest{DnsNames: parsedPrecert.DNSNames})
-	if err != nil {
-		return nil, nil, wrapError(err, "checking if certificate is a renewal")
-	}
-	isRenewal := exists.Exists
-
 	cert, err := ra.CA.IssueCertificateForPrecertificate(ctx, &capb.IssueCertificateForPrecertificateRequest{
 		DER:             precert.DER,
 		SCTs:            scts,

ra/ra_test.go

@@ -3655,7 +3655,7 @@ func TestIssueCertificateInnerErrs(t *testing.T) {
 			// Mock the CA
 			ra.CA = tc.Mock
 			// Attempt issuance
-			_, _, err = ra.issueCertificateInner(ctx, csrOb, order.CertificateProfileName, accountID(Registration.Id), orderID(order.Id))
+			_, _, err = ra.issueCertificateInner(ctx, csrOb, false, order.CertificateProfileName, accountID(Registration.Id), orderID(order.Id))
 			// We expect all of the testcases to fail because all use mocked CAs that deliberately error
 			test.AssertError(t, err, "issueCertificateInner with failing mock CA did not fail")
 			// If there is an expected `error` then match the error message
@@ -3698,8 +3698,12 @@ func (sa *mockSAWithFinalize) FinalizeOrder(ctx context.Context, req *sapb.Final
 	return &emptypb.Empty{}, nil
 }
 
-func (sa *mockSAWithFinalize) FQDNSetExists(ctx context.Context, in *sapb.FQDNSetExistsRequest, opts ...grpc.CallOption) (*sapb.Exists, error) {
-	return &sapb.Exists{}, nil
+func (sa *mockSAWithFinalize) FQDNSetTimestampsForWindow(ctx context.Context, in *sapb.CountFQDNSetsRequest, opts ...grpc.CallOption) (*sapb.Timestamps, error) {
+	return &sapb.Timestamps{
+		Timestamps: []*timestamppb.Timestamp{
+			timestamppb.Now(),
+		},
+	}, nil
 }
 
 func TestIssueCertificateInnerWithProfile(t *testing.T) {
@@ -3732,7 +3736,7 @@ func TestIssueCertificateInnerWithProfile(t *testing.T) {
 
 	// Call issueCertificateInner with the CSR generated above and the profile
 	// name "default", which will cause the mockCA to return a specific hash.
-	_, cpId, err := ra.issueCertificateInner(context.Background(), csr, "default", 1, 1)
+	_, cpId, err := ra.issueCertificateInner(context.Background(), csr, false, "default", 1, 1)
 	test.AssertNotError(t, err, "issuing cert with profile name")
 	test.AssertEquals(t, mockCA.profileName, cpId.name)
 	test.AssertByteEquals(t, mockCA.profileHash, cpId.hash)

sa/db/boulder_sa/20230419000000_CombinedSchema.sql

@@ -86,6 +86,8 @@ CREATE TABLE `fqdnSets` (
   `id` bigint(20) UNSIGNED NOT NULL AUTO_INCREMENT,
   `setHash` binary(32) NOT NULL,
   `serial` varchar(255) NOT NULL,
+  -- Note: This should actually be called "notBefore" since it is set
+  -- based on the certificate's notBefore field, not the issuance time.
   `issued` datetime NOT NULL,
   `expires` datetime NOT NULL,
   PRIMARY KEY (`id`),

test/inmem/sa/sa.go

@@ -121,6 +121,10 @@ func (sa SA) FQDNSetExists(ctx context.Context, req *sapb.FQDNSetExistsRequest,
 	return sa.Impl.FQDNSetExists(ctx, req)
 }
 
+func (sa SA) FQDNSetTimestampsForWindow(ctx context.Context, req *sapb.CountFQDNSetsRequest, _ ...grpc.CallOption) (*sapb.Timestamps, error) {
+	return sa.Impl.FQDNSetTimestampsForWindow(ctx, req)
+}
+
 func (sa SA) PauseIdentifiers(ctx context.Context, req *sapb.PauseRequest, _ ...grpc.CallOption) (*sapb.PauseIdentifiersResponse, error) {
 	return sa.Impl.PauseIdentifiers(ctx, req)
 }

wfe2/wfe.go

@@ -21,6 +21,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 	"go.opentelemetry.io/otel/trace"
+	"google.golang.org/protobuf/types/known/durationpb"
 	"google.golang.org/protobuf/types/known/emptypb"
 
 	"github.com/letsencrypt/boulder/core"
@@ -2348,12 +2349,16 @@ func (wfe *WebFrontEndImpl) NewOrder(
 		// The Subscriber does not have an ARI exemption. However, we can check
 		// if the order is a renewal, and thus exempt from the NewOrdersPerAccount
 		// and CertificatesPerDomain limits.
-		exists, err := wfe.sa.FQDNSetExists(ctx, &sapb.FQDNSetExistsRequest{DnsNames: names})
+		timestamps, err := wfe.sa.FQDNSetTimestampsForWindow(ctx, &sapb.CountFQDNSetsRequest{
+			DnsNames: names,
+			Window:   durationpb.New(120 * 24 * time.Hour),
+			Limit:    1,
+		})
 		if err != nil {
 			wfe.sendError(response, logEvent, web.ProblemDetailsForError(err, "While checking renewal exemption status"), err)
 			return
 		}
-		isRenewal = exists.Exists
+		isRenewal = len(timestamps.Timestamps) > 0
 	}
 
 	err = wfe.validateCertificateProfileName(newOrderRequest.Profile)

wfe2/wfe_test.go

@@ -3863,13 +3863,17 @@ func Test_sendErrorInternalServerError(t *testing.T) {
 
 // mockSAForARI provides a mock SA with the methods required for an issuance and
 // a renewal with the ARI `Replaces` field.
+//
+// Note that FQDNSetTimestampsForWindow always return an empty list, which allows us to act
+// as if a certificate is not getting the renewal exemption, even when we are repeatedly
+// issuing for the same names.
 type mockSAForARI struct {
 	sapb.StorageAuthorityReadOnlyClient
 	cert *corepb.Certificate
 }
 
-func (sa *mockSAForARI) FQDNSetExists(ctx context.Context, in *sapb.FQDNSetExistsRequest, opts ...grpc.CallOption) (*sapb.Exists, error) {
-	return &sapb.Exists{Exists: false}, nil
+func (sa *mockSAForARI) FQDNSetTimestampsForWindow(ctx context.Context, in *sapb.CountFQDNSetsRequest, opts ...grpc.CallOption) (*sapb.Timestamps, error) {
+	return &sapb.Timestamps{Timestamps: nil}, nil
 }
 
 // GetCertificate returns the inner certificate if it matches the given serial.
@@ -4170,6 +4174,9 @@ func TestNewOrderRateLimits(t *testing.T) {
 		`{"Identifiers": [{"type": "dns", "value": "example.com"}]}`)
 	responseWriter = httptest.NewRecorder()
 	mux.ServeHTTP(responseWriter, r)
+	features.Set(features.Config{
+		UseKvLimitsForNewOrder: true,
+	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusTooManyRequests)
 
 	// Make a request with the "Replaces" field, which should satisfy ARI checks