Make CAA checking more like DCV checking (letsencrypt#8491)

If the primary perspective CAA check returns an error, return early
rather than always kicking off the remote checks anyway.

As part of this change, rearrange the code in DoCAA to more closely
mirror the code in DoDCV. In particular, move the creation of the result
protobuf into a helper function which can be called at both the
early-return and late-return locations.

Author: aarongable

grpc/pb-marshalling.go

@@ -227,6 +227,18 @@ func pbToValidationResult(in *vapb.ValidationResult) ([]core.ValidationRecord, *
 	return recordAry, prob, nil
 }
 
+func CAAResultToPB(prob *probs.ProblemDetails, perspective, rir string) (*vapb.IsCAAValidResponse, error) {
+	marshalledProb, err := ProblemDetailsToPB(prob)
+	if err != nil {
+		return nil, err
+	}
+	return &vapb.IsCAAValidResponse{
+		Problem:     marshalledProb,
+		Perspective: perspective,
+		Rir:         rir,
+	}, nil
+}
+
 func RegistrationToPB(reg core.Registration) (*corepb.Registration, error) {
 	keyBytes, err := reg.Key.MarshalJSON()
 	if err != nil {

va/caa.go

@@ -15,8 +15,8 @@ import (
 
 	"github.com/letsencrypt/boulder/bdns"
 	"github.com/letsencrypt/boulder/core"
-	corepb "github.com/letsencrypt/boulder/core/proto"
 	berrors "github.com/letsencrypt/boulder/errors"
+	bgrpc "github.com/letsencrypt/boulder/grpc"
 	"github.com/letsencrypt/boulder/identifier"
 	"github.com/letsencrypt/boulder/probs"
 	vapb "github.com/letsencrypt/boulder/va/proto"
@@ -45,12 +45,6 @@ func (va *ValidationAuthorityImpl) DoCAA(ctx context.Context, req *vapb.IsCAAVal
 		return nil, berrors.MalformedError("Identifier type for CAA check was not DNS")
 	}
 
-	logEvent := validationLogEvent{
-		AuthzID:    req.AuthzID,
-		Requester:  req.AccountURIID,
-		Identifier: ident,
-	}
-
 	challType := core.AcmeChallenge(req.ValidationMethod)
 	if !challType.IsValid() {
 		return nil, berrors.InternalServerError("unrecognized validation method %q", req.ValidationMethod)
@@ -66,10 +60,13 @@ func (va *ValidationAuthorityImpl) DoCAA(ctx context.Context, req *vapb.IsCAAVal
 	// redeclare `prob`, `localLatency`, or `summary` below this point.
 	var prob *probs.ProblemDetails
 	var summary *mpicSummary
-	var internalErr error
 	var localLatency time.Duration
 	start := va.clk.Now()
-
+	logEvent := validationLogEvent{
+		AuthzID:    req.AuthzID,
+		Requester:  req.AccountURIID,
+		Identifier: ident,
+	}
 	defer func() {
 		probType := ""
 		outcome := fail
@@ -81,28 +78,32 @@ func (va *ValidationAuthorityImpl) DoCAA(ctx context.Context, req *vapb.IsCAAVal
 			// 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))
 			logEvent.Summary = summary
 		}
+
 		// Log the total check latency.
 		logEvent.Latency = va.clk.Since(start).Round(time.Millisecond).Seconds()
-
 		va.log.AuditObject("CAA check result", logEvent)
 	}()
 
-	internalErr = va.checkCAA(ctx, ident, params)
+	// Do the local checks. We do these before kicking off the remote checks to
+	// ensure that we don't waste effort on remote checks if the local ones fail.
+	err := va.checkCAA(ctx, ident, params)
 
 	// Stop the clock for local check latency.
 	localLatency = va.clk.Since(start)
 
-	if internalErr != nil {
-		logEvent.InternalError = internalErr.Error()
-		prob = detailedError(internalErr)
+	if err != nil {
+		logEvent.InternalError = err.Error()
+		prob = detailedError(err)
 		prob.Detail = fmt.Sprintf("While processing CAA for %s: %s", ident.Value, prob.Detail)
+		return bgrpc.CAAResultToPB(filterProblemDetails(prob), va.perspective, va.rir)
 	}
 
 	if va.isPrimaryVA() {
@@ -113,35 +114,10 @@ func (va *ValidationAuthorityImpl) DoCAA(ctx context.Context, req *vapb.IsCAAVal
 			}
 			return remoteva.DoCAA(ctx, checkRequest)
 		}
-		var remoteProb *probs.ProblemDetails
-		summary, remoteProb = va.doRemoteOperation(ctx, op, req)
-		// If the remote result was a non-nil problem then fail the CAA check
-		if remoteProb != nil {
-			prob = remoteProb
-			va.log.Infof("CAA check failed due to remote failures: identifier=%v err=%s",
-				ident.Value, remoteProb)
-		}
+		summary, prob = va.doRemoteOperation(ctx, op, req)
 	}
 
-	if prob != nil {
-		// The ProblemDetails will be serialized through gRPC, which requires UTF-8.
-		// It will also later be serialized in JSON, which defaults to UTF-8. Make
-		// sure it is UTF-8 clean now.
-		prob = filterProblemDetails(prob)
-		return &vapb.IsCAAValidResponse{
-			Problem: &corepb.ProblemDetails{
-				ProblemType: string(prob.Type),
-				Detail:      replaceInvalidUTF8([]byte(prob.Detail)),
-			},
-			Perspective: va.perspective,
-			Rir:         va.rir,
-		}, nil
-	} else {
-		return &vapb.IsCAAValidResponse{
-			Perspective: va.perspective,
-			Rir:         va.rir,
-		}, nil
-	}
+	return bgrpc.CAAResultToPB(filterProblemDetails(prob), va.perspective, va.rir)
 }
 
 // checkCAA performs a CAA lookup & validation for the provided identifier. If

va/va.go

@@ -700,6 +700,7 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV
 			logEvent.Challenge.Status = core.StatusValid
 			outcome = pass
 		}
+
 		// Observe local validation latency (primary|remote).
 		va.observeLatency(opDCV, va.perspective, string(chall.Type), probType, outcome, localLatency)
 		if va.isPrimaryVA() {
@@ -762,5 +763,6 @@ func (va *ValidationAuthorityImpl) DoDCV(ctx context.Context, req *vapb.PerformV
 		}
 		summary, prob = va.doRemoteOperation(ctx, op, req)
 	}
+
 	return bgrpc.ValidationResultToPB(records, filterProblemDetails(prob), va.perspective, va.rir)
 }