From 4bf6e2f5a9a49704f5bc2ad110a04a6f30aa3d3e Mon Sep 17 00:00:00 2001
From: Samantha Frank <hello@entropy.cat>
Date: Wed, 21 Aug 2024 15:25:22 -0400
Subject: [PATCH] ratelimits: Skip Spends on CertificatesPerDomain for renewals
 (#7676)

This bug was introduced in
https://github.com/letsencrypt/boulder/pull/7669.

Also, make calls to ra.countCertificateIssued() non-blocking like
ra.countFailedValidation().

Part of #7664
Blocks #7666
---
 ra/ra.go      | 25 +++++++++++++++++++------
 ra/ra_test.go |  4 ++++
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/ra/ra.go b/ra/ra.go
index dc4d01959da..c6d3888eddc 100644
--- a/ra/ra.go
+++ b/ra/ra.go
@@ -1302,18 +1302,20 @@ func (ra *RegistrationAuthorityImpl) issueCertificateOuter(
 // account) and duplicate certificate rate limits. There is no reason to surface
 // errors from this function to the Subscriber, spends against these limit are
 // best effort.
-func (ra *RegistrationAuthorityImpl) countCertificateIssued(ctx context.Context, regId int64, orderDomains []string) {
+func (ra *RegistrationAuthorityImpl) countCertificateIssued(ctx context.Context, regId int64, orderDomains []string, isRenewal bool) {
 	if ra.limiter == nil || ra.txnBuilder == nil {
 		// Limiter is disabled.
 		return
 	}
 
 	var transactions []ratelimits.Transaction
-	txns, err := ra.txnBuilder.CertificatesPerDomainSpendOnlyTransactions(regId, orderDomains)
-	if err != nil {
-		ra.log.Warningf("building rate limit transactions at finalize: %s", err)
+	if !isRenewal {
+		txns, err := ra.txnBuilder.CertificatesPerDomainSpendOnlyTransactions(regId, orderDomains)
+		if err != nil {
+			ra.log.Warningf("building rate limit transactions at finalize: %s", err)
+		}
+		transactions = append(transactions, txns...)
 	}
-	transactions = append(transactions, txns...)
 
 	txn, err := ra.txnBuilder.CertificatesPerFQDNSetSpendOnlyTransaction(orderDomains)
 	if err != nil {
@@ -1402,6 +1404,17 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner(
 		return nil, nil, wrapError(err, "getting SCTs")
 	}
 
+	var isRenewal bool
+	if len(parsedPrecert.DNSNames) > 0 {
+		// This should never happen under normal operation, but it sometimes
+		// occurs under test.
+		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,
@@ -1418,7 +1431,7 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner(
 		return nil, nil, wrapError(err, "parsing final certificate")
 	}
 
-	ra.countCertificateIssued(ctx, int64(acctID), parsedCertificate.DNSNames)
+	go ra.countCertificateIssued(ctx, int64(acctID), slices.Clone(parsedCertificate.DNSNames), isRenewal)
 
 	// Asynchronously submit the final certificate to any configured logs
 	go ra.ctpolicy.SubmitFinalCert(cert.Der, parsedCertificate.NotAfter)
diff --git a/ra/ra_test.go b/ra/ra_test.go
index 73348261650..461a2ad8cd2 100644
--- a/ra/ra_test.go
+++ b/ra/ra_test.go
@@ -3709,6 +3709,10 @@ 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 TestIssueCertificateInnerWithProfile(t *testing.T) {
 	_, _, ra, fc, cleanup := initAuthorities(t)
 	defer cleanup()