Merge branch 'main' into dependabot/go_modules/aws-e559bdec09

Author: beautifulentropy

ra/ra.go

@@ -1358,7 +1358,7 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner(
 		return nil, nil, wrapError(err, "parsing final certificate")
 	}
 
-	go ra.countCertificateIssued(ctx, int64(acctID), slices.Clone(parsedCertificate.DNSNames), isRenewal)
+	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)
@@ -1998,12 +1998,10 @@ func (ra *RegistrationAuthorityImpl) PerformValidation(
 		if prob != nil {
 			challenge.Status = core.StatusInvalid
 			challenge.Error = prob
-			go func() {
-				err := ra.countFailedValidations(vaCtx, authz.RegistrationID, authz.Identifier)
-				if err != nil {
-					ra.log.Warningf("incrementing failed validations: %s", err)
-				}
-			}()
+			err := ra.countFailedValidations(vaCtx, authz.RegistrationID, authz.Identifier)
+			if err != nil {
+				ra.log.Warningf("incrementing failed validations: %s", err)
+			}
 		} else {
 			challenge.Status = core.StatusValid
 			if features.Get().AutomaticallyPauseZombieClients {

ratelimits/gcra_test.go

@@ -11,7 +11,7 @@ import (
 
 func TestDecide(t *testing.T) {
 	clk := clock.NewFake()
-	limit := limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
+	limit := &limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
 	limit.precompute()
 
 	// Begin by using 1 of our 10 requests.
@@ -138,7 +138,7 @@ func TestDecide(t *testing.T) {
 
 func TestMaybeRefund(t *testing.T) {
 	clk := clock.NewFake()
-	limit := limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
+	limit := &limit{Burst: 10, Count: 1, Period: config.Duration{Duration: time.Second}}
 	limit.precompute()
 
 	// Begin by using 1 of our 10 requests.

ratelimits/limit.go

@@ -63,7 +63,7 @@ func (l *limit) precompute() {
 	l.burstOffset = l.emissionInterval * l.Burst
 }
 
-func validateLimit(l limit) error {
+func validateLimit(l *limit) error {
 	if l.Burst <= 0 {
 		return fmt.Errorf("invalid burst '%d', must be > 0", l.Burst)
 	}
@@ -76,7 +76,7 @@ func validateLimit(l limit) error {
 	return nil
 }
 
-type limits map[string]limit
+type limits map[string]*limit
 
 // loadDefaults marshals the defaults YAML file at path into a map of limits.
 func loadDefaults(path string) (limits, error) {
@@ -156,7 +156,8 @@ func loadAndParseOverrideLimits(path string) (limits, error) {
 
 	for _, ov := range fromFile {
 		for k, v := range ov {
-			err = validateLimit(v.limit)
+			limit := &v.limit
+			err = validateLimit(limit)
 			if err != nil {
 				return nil, fmt.Errorf("validating override limit %q: %w", k, err)
 			}
@@ -167,7 +168,6 @@ func loadAndParseOverrideLimits(path string) (limits, error) {
 			v.limit.name = name
 
 			for _, entry := range v.Ids {
-				limit := v.limit
 				id := entry.Id
 				err = validateIdForName(name, id)
 				if err != nil {
@@ -248,11 +248,11 @@ func newLimitRegistry(defaults, overrides string) (*limitRegistry, error) {
 // required, bucketKey is optional. If bucketkey is empty, the default for the
 // limit specified by name is returned. If no default limit exists for the
 // specified name, errLimitDisabled is returned.
-func (l *limitRegistry) getLimit(name Name, bucketKey string) (limit, error) {
+func (l *limitRegistry) getLimit(name Name, bucketKey string) (*limit, error) {
 	if !name.isValid() {
 		// This should never happen. Callers should only be specifying the limit
 		// Name enums defined in this package.
-		return limit{}, fmt.Errorf("specified name enum %q, is invalid", name)
+		return nil, fmt.Errorf("specified name enum %q, is invalid", name)
 	}
 	if bucketKey != "" {
 		// Check for override.
@@ -265,5 +265,5 @@ func (l *limitRegistry) getLimit(name Name, bucketKey string) (limit, error) {
 	if ok {
 		return dl, nil
 	}
-	return limit{}, errLimitDisabled
+	return nil, errLimitDisabled
 }

ratelimits/limit_test.go

@@ -42,11 +42,11 @@ func TestParseOverrideNameId(t *testing.T) {
 }
 
 func TestValidateLimit(t *testing.T) {
-	err := validateLimit(limit{Burst: 1, Count: 1, Period: config.Duration{Duration: time.Second}})
+	err := validateLimit(&limit{Burst: 1, Count: 1, Period: config.Duration{Duration: time.Second}})
 	test.AssertNotError(t, err, "valid limit")
 
 	// All of the following are invalid.
-	for _, l := range []limit{
+	for _, l := range []*limit{
 		{Burst: 0, Count: 1, Period: config.Duration{Duration: time.Second}},
 		{Burst: 1, Count: 0, Period: config.Duration{Duration: time.Second}},
 		{Burst: 1, Count: 1, Period: config.Duration{Duration: 0}},

ratelimits/limiter_test.go

@@ -497,7 +497,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 5 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   NewRegistrationsPerIPAddress,
 						Burst:  10,
 						Period: config.Duration{Duration: time.Hour},
@@ -513,7 +513,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 10 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   NewRegistrationsPerIPv6Range,
 						Burst:  5,
 						Period: config.Duration{Duration: time.Hour},
@@ -529,7 +529,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 10 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   NewOrdersPerAccount,
 						Burst:  2,
 						Period: config.Duration{Duration: time.Hour},
@@ -545,7 +545,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 15 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   FailedAuthorizationsPerDomainPerAccount,
 						Burst:  7,
 						Period: config.Duration{Duration: time.Hour},
@@ -562,7 +562,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 20 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   CertificatesPerDomain,
 						Burst:  3,
 						Period: config.Duration{Duration: time.Hour},
@@ -579,7 +579,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 20 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name:   CertificatesPerDomainPerAccount,
 						Burst:  3,
 						Period: config.Duration{Duration: time.Hour},
@@ -596,7 +596,7 @@ func TestRateLimitError(t *testing.T) {
 				allowed: false,
 				retryIn: 30 * time.Second,
 				transaction: Transaction{
-					limit: limit{
+					limit: &limit{
 						name: 9999999,
 					},
 				},

ratelimits/transaction.go

@@ -107,7 +107,7 @@ func newFQDNSetBucketKey(name Name, orderNames []string) (string, error) { //nol
 // it would fail validateTransaction (for instance because cost and burst are zero).
 type Transaction struct {
 	bucketKey string
-	limit     limit
+	limit     *limit
 	cost      int64
 	check     bool
 	spend     bool
@@ -143,7 +143,7 @@ func validateTransaction(txn Transaction) (Transaction, error) {
 	return txn, nil
 }
 
-func newTransaction(limit limit, bucketKey string, cost int64) (Transaction, error) {
+func newTransaction(limit *limit, bucketKey string, cost int64) (Transaction, error) {
 	return validateTransaction(Transaction{
 		bucketKey: bucketKey,
 		limit:     limit,
@@ -153,7 +153,7 @@ func newTransaction(limit limit, bucketKey string, cost int64) (Transaction, err
 	})
 }
 
-func newCheckOnlyTransaction(limit limit, bucketKey string, cost int64) (Transaction, error) {
+func newCheckOnlyTransaction(limit *limit, bucketKey string, cost int64) (Transaction, error) {
 	return validateTransaction(Transaction{
 		bucketKey: bucketKey,
 		limit:     limit,
@@ -162,7 +162,7 @@ func newCheckOnlyTransaction(limit limit, bucketKey string, cost int64) (Transac
 	})
 }
 
-func newSpendOnlyTransaction(limit limit, bucketKey string, cost int64) (Transaction, error) {
+func newSpendOnlyTransaction(limit *limit, bucketKey string, cost int64) (Transaction, error) {
 	return validateTransaction(Transaction{
 		bucketKey: bucketKey,
 		limit:     limit,

va/caa.go

@@ -11,7 +11,6 @@ import (
 	"time"
 
 	"github.com/miekg/dns"
-	"github.com/prometheus/client_golang/prometheus"
 
 	"github.com/letsencrypt/boulder/bdns"
 	"github.com/letsencrypt/boulder/canceled"
@@ -42,19 +41,46 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC
 		Requester: req.AccountURIID,
 		Hostname:  req.Domain,
 	}
-	checkStartTime := va.clk.Now()
 
-	validationMethod := core.AcmeChallenge(req.ValidationMethod)
-	if !validationMethod.IsValid() {
+	challType := core.AcmeChallenge(req.ValidationMethod)
+	if !challType.IsValid() {
 		return nil, berrors.InternalServerError("unrecognized validation method %q", req.ValidationMethod)
 	}
 
 	acmeID := identifier.NewDNS(req.Domain)
 	params := &caaParams{
 		accountURIID:     req.AccountURIID,
-		validationMethod: validationMethod,
+		validationMethod: challType,
 	}
 
+	var prob *probs.ProblemDetails
+	var internalErr error
+	var localLatency time.Duration
+	start := va.clk.Now()
+
+	defer func() {
+		probType := ""
+		outcome := fail
+		if prob != nil {
+			// CAA check failed.
+			probType = string(prob.Type)
+			logEvent.Error = prob.Error()
+		} else {
+			// CAA check passed.
+			outcome = pass
+		}
+		// Observe local check latency (primary|remote).
+		va.observeLatency(opCAA, va.perspective, string(challType), probType, outcome, localLatency)
+		if va.isPrimaryVA() {
+			// Observe total check latency (primary+remote).
+			va.observeLatency(opCAA, allPerspectives, string(challType), probType, outcome, va.clk.Since(start))
+		}
+		// Log the total check latency.
+		logEvent.ValidationLatency = va.clk.Since(start).Round(time.Millisecond).Seconds()
+
+		va.log.AuditObject("CAA check result", logEvent)
+	}()
+
 	var remoteCAAResults chan *remoteVAResult
 	if features.Get().EnforceMultiCAA {
 		if remoteVACount := len(va.remoteVAs); remoteVACount > 0 {
@@ -63,16 +89,10 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC
 		}
 	}
 
-	checkResult := "success"
-	err := va.checkCAA(ctx, acmeID, params)
-	localCheckLatency := time.Since(checkStartTime)
-	var prob *probs.ProblemDetails
-	if err != nil {
-		prob = detailedError(err)
-		logEvent.Error = prob.Error()
-		logEvent.InternalError = err.Error()
+	internalErr = va.checkCAA(ctx, acmeID, params)
+	if internalErr != nil {
+		prob = detailedError(internalErr)
 		prob.Detail = fmt.Sprintf("While processing CAA for %s: %s", req.Domain, prob.Detail)
-		checkResult = "failure"
 	} else if remoteCAAResults != nil {
 		if !features.Get().EnforceMultiCAA && features.Get().MultiCAAFullResults {
 			// If we're not going to enforce multi CAA but we are logging the
@@ -82,40 +102,24 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC
 				_ = va.processRemoteCAAResults(
 					req.Domain,
 					req.AccountURIID,
-					string(validationMethod),
+					string(challType),
 					remoteCAAResults)
 			}()
 		} else if features.Get().EnforceMultiCAA {
 			remoteProb := va.processRemoteCAAResults(
 				req.Domain,
 				req.AccountURIID,
-				string(validationMethod),
+				string(challType),
 				remoteCAAResults)
 
 			// If the remote result was a non-nil problem then fail the CAA check
 			if remoteProb != nil {
 				prob = remoteProb
-				// We only set .Error here, not InternalError, because the remote VA doesn't send
-				// us the internal error. But that's okay, because it got logged at the remote VA.
-				logEvent.Error = remoteProb.Error()
-				checkResult = "failure"
 				va.log.Infof("CAA check failed due to remote failures: identifier=%v err=%s",
 					req.Domain, remoteProb)
-				va.metrics.remoteCAACheckFailures.Inc()
 			}
 		}
 	}
-	checkLatency := time.Since(checkStartTime)
-	logEvent.ValidationLatency = checkLatency.Round(time.Millisecond).Seconds()
-
-	va.metrics.localCAACheckTime.With(prometheus.Labels{
-		"result": checkResult,
-	}).Observe(localCheckLatency.Seconds())
-	va.metrics.caaCheckTime.With(prometheus.Labels{
-		"result": checkResult,
-	}).Observe(checkLatency.Seconds())
-
-	va.log.AuditObject("CAA check result", logEvent)
 
 	if prob != nil {
 		// The ProblemDetails will be serialized through gRPC, which requires UTF-8.
@@ -154,15 +158,6 @@ func (va *ValidationAuthorityImpl) processRemoteCAAResults(
 	challengeType string,
 	remoteResultsChan <-chan *remoteVAResult) *probs.ProblemDetails {
 
-	state := "failure"
-	start := va.clk.Now()
-
-	defer func() {
-		va.metrics.remoteCAACheckTime.With(prometheus.Labels{
-			"result": state,
-		}).Observe(va.clk.Since(start).Seconds())
-	}()
-
 	required := len(va.remoteVAs) - va.maxRemoteFailures
 	good := 0
 	bad := 0
@@ -190,7 +185,6 @@ func (va *ValidationAuthorityImpl) processRemoteCAAResults(
 		// success or failure threshold is met.
 		if !features.Get().MultiCAAFullResults {
 			if good >= required {
-				state = "success"
 				return nil
 			} else if bad > va.maxRemoteFailures {
 				modifiedProblem := *result.Problem
@@ -217,7 +211,6 @@ func (va *ValidationAuthorityImpl) processRemoteCAAResults(
 
 	// Based on the threshold of good/bad return nil or a problem.
 	if good >= required {
-		state = "success"
 		return nil
 	} else if bad > va.maxRemoteFailures {
 		modifiedProblem := *firstProb