From caf655353eeef758c15f4b7961ba09440ae20821 Mon Sep 17 00:00:00 2001
From: Daniel McCarney <cpu@letsencrypt.org>
Date: Fri, 31 May 2019 18:36:48 -0400
Subject: [PATCH] RA: Use suberrors when rechecking CAA. (#4240)

When Boulder's RA rechecks CAA for a set of authorization identifiers it
should use suberrors to make it easy to identify which of a possible 100
identifiers had a CAA issue at order finalization time.

Updates #4193
Resolves #4235
---
 ra/ra.go      | 37 +++++++++++++++++++++++++++++--------
 ra/ra_test.go | 36 +++++++++++++++++++++++++++++-------
 2 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/ra/ra.go b/ra/ra.go
index 15ef56998..9dc2ba1e1 100644
--- a/ra/ra.go
+++ b/ra/ra.go
@@ -894,16 +894,37 @@ func (ra *RegistrationAuthorityImpl) recheckCAA(ctx context.Context, authzs []*c
 			ch <- err
 		}(authz)
 	}
-	var caaFailures []string
-	for _ = range authzs {
-		if err := <-ch; berrors.Is(err, berrors.CAA) {
-			caaFailures = append(caaFailures, err.Error())
-		} else if err != nil {
-			return err
+	var subErrors []berrors.SubBoulderError
+	for _, authz := range authzs {
+		err := <-ch
+		if err != nil {
+			if bErr, _ := err.(*berrors.BoulderError); berrors.Is(err, berrors.CAA) {
+				subErrors = append(subErrors, berrors.SubBoulderError{
+					Identifier:   authz.Identifier,
+					BoulderError: bErr})
+			} else {
+				return err
+			}
 		}
 	}
-	if len(caaFailures) > 0 {
-		return berrors.CAAError("Rechecking CAA: %v", strings.Join(caaFailures, ", "))
+	if len(subErrors) > 0 {
+		var detail string
+		if len(subErrors) == 1 {
+			detail = fmt.Sprintf(
+				"Rechecking CAA for %q: %s",
+				subErrors[0].Identifier.Value,
+				subErrors[0].BoulderError.Detail)
+		} else {
+			detail = fmt.Sprintf(
+				"Rechecking CAA for %q and %d more identifiers failed. "+
+					"Refer to sub-problems for more information",
+				subErrors[0].Identifier.Value,
+				len(subErrors)-1)
+		}
+		return (&berrors.BoulderError{
+			Type:   berrors.CAA,
+			Detail: detail,
+		}).WithSubErrors(subErrors)
 	}
 	return nil
 }
diff --git a/ra/ra_test.go b/ra/ra_test.go
index d355e5da1..4b33ad880 100644
--- a/ra/ra_test.go
+++ b/ra/ra_test.go
@@ -2063,15 +2063,37 @@ func TestRecheckCAAFail(t *testing.T) {
 		makeHTTP01Authorization("b.com"),
 		makeHTTP01Authorization("c.com"),
 	}
-	if err := ra.recheckCAA(context.Background(), authzs); err == nil {
-		t.Errorf("expected err, got nil")
+	err := ra.recheckCAA(context.Background(), authzs)
+
+	if err == nil {
+		t.Fatalf("expected err, got nil")
 	} else if !berrors.Is(err, berrors.CAA) {
-		t.Errorf("expected CAA error, got %T", err)
-	} else if !strings.Contains(err.Error(), "CAA invalid for a.com") {
-		t.Errorf("expected error to contain error for a.com, got %q", err)
-	} else if !strings.Contains(err.Error(), "CAA invalid for c.com") {
-		t.Errorf("expected error to contain error for c.com, got %q", err)
+		t.Fatalf("expected CAA error, got %T", err)
 	}
+
+	// NOTE(@cpu): Safe to skip the cast check here because we already checked err
+	// with `berrors.Is(err, berrors.CAA)`
+	berr, _ := err.(*berrors.BoulderError)
+
+	// There should be two sub errors
+	test.AssertEquals(t, len(berr.SubErrors), 2)
+
+	// The top level error should reference the first failing identifier
+	if !strings.Contains(berr.Detail, `Rechecking CAA for "a.com" and 1 more identifiers`) {
+		t.Errorf("expected error to ref first failed identiifer, got %q", err)
+	}
+
+	// There should be a sub error for both a.com and c.com with the correct type
+	subErrMap := make(map[string]berrors.SubBoulderError, len(berr.SubErrors))
+	for _, subErr := range berr.SubErrors {
+		subErrMap[subErr.Identifier.Value] = subErr
+	}
+	subErrA, foundA := subErrMap["a.com"]
+	subErrB, foundB := subErrMap["c.com"]
+	test.AssertEquals(t, foundA, true)
+	test.AssertEquals(t, foundB, true)
+	test.AssertEquals(t, subErrA.Type, berrors.CAA)
+	test.AssertEquals(t, subErrB.Type, berrors.CAA)
 }
 
 func TestRecheckCAAInternalServerError(t *testing.T) {