From ef6593d06b23883025f2a1bfae9310e3c29fc7bc Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 6 Jan 2025 10:16:53 -0800 Subject: [PATCH] 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 | 38 ++++++++++++------- ra/ra_test.go | 12 ++++-- .../20230419000000_CombinedSchema.sql | 2 + test/inmem/sa/sa.go | 4 ++ wfe2/wfe.go | 9 ++++- wfe2/wfe_test.go | 11 +++++- 6 files changed, 54 insertions(+), 22 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index bf882e5a8..3c0f53e22 100644 --- a/ra/ra.go +++ b/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, diff --git a/ra/ra_test.go b/ra/ra_test.go index 143178e4e..972716de8 100644 --- a/ra/ra_test.go +++ b/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) diff --git a/sa/db/boulder_sa/20230419000000_CombinedSchema.sql b/sa/db/boulder_sa/20230419000000_CombinedSchema.sql index ff8e54320..42c489be9 100644 --- a/sa/db/boulder_sa/20230419000000_CombinedSchema.sql +++ b/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`), diff --git a/test/inmem/sa/sa.go b/test/inmem/sa/sa.go index 14727d632..624f254e3 100644 --- a/test/inmem/sa/sa.go +++ b/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) } diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 18a9d224a..a41472e54 100644 --- a/wfe2/wfe.go +++ b/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) diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 04aac18ac..10a435fa4 100644 --- a/wfe2/wfe_test.go +++ b/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