Skip to content

Commit

Permalink
ratelimits: Skip Spends on CertificatesPerDomain for renewals (#7676)
Browse files Browse the repository at this point in the history
This bug was introduced in
#7669.

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

Part of #7664
Blocks #7666
  • Loading branch information
beautifulentropy committed Aug 21, 2024
1 parent 9b08fa5 commit 4bf6e2f
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 6 deletions.
25 changes: 19 additions & 6 deletions ra/ra.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions ra/ra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit 4bf6e2f

Please sign in to comment.