Improve errors for DNS challenge (#3317)

Before this change, we would just log "Correct value not found for DNS challenge"
when we got a TXT record that didn't match what we expected. This was different
from the error when no TXT records were found at all, but viewing the error out of
context doesn't make that clear. This change improves the error to specifically say
that we found a TXT record, but it was the wrong one.

Also in this change: if we found multiple TXT records, we mention the number;
and we trim the length of the echoed TXT record.

bdns/mocks.go

@@ -26,6 +26,15 @@ func (mock *MockDNSClient) LookupTXT(_ context.Context, hostname string) ([]stri
 		// expected token + test account jwk thumbprint
 		return []string{"LPsIwTo7o8BoG0-vjCyGQGBWSVIPxI-i_X336eUOQZo"}, []string{"respect my authority!"}, nil
 	}
+	if hostname == "_acme-challenge.wrong-dns01.com" {
+		return []string{"a"}, []string{"respect my authority!"}, nil
+	}
+	if hostname == "_acme-challenge.wrong-many-dns01.com" {
+		return []string{"a", "b", "c", "d", "e"}, []string{"respect my authority!"}, nil
+	}
+	if hostname == "_acme-challenge.long-dns01.com" {
+		return []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, []string{"respect my authority!"}, nil
+	}
 	if hostname == "_acme-challenge.no-authority-dns01.com" {
 		// base64(sha256("LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0"
 		//               + "." + "9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI"))

va/va.go

@@ -743,7 +743,7 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, identifier
 	txts, authorities, err := va.dnsClient.LookupTXT(ctx, challengeSubdomain)
 
 	if err != nil {
-		va.log.Info(fmt.Sprintf("Failed to lookup txt records for %s. err=[%#v] errStr=[%s]", identifier, err, err))
+		va.log.Info(fmt.Sprintf("Failed to lookup TXT records for %s. err=[%#v] errStr=[%s]", identifier, err, err))
 
 		return nil, probs.ConnectionFailure(err.Error())
 	}
@@ -752,7 +752,8 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, identifier
 	// troubleshooters to differentiate between no TXT records and
 	// invalid/incorrect TXT records.
 	if len(txts) == 0 {
-		return nil, probs.Unauthorized("No TXT records found for DNS challenge")
+		return nil, probs.Unauthorized(fmt.Sprintf(
+			"No TXT record found at %s", challengeSubdomain))
 	}
 
 	for _, element := range txts {
@@ -765,7 +766,17 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, identifier
 		}
 	}
 
-	return nil, probs.Unauthorized("Correct value not found for DNS challenge")
+	invalidRecord := txts[0]
+	if len(invalidRecord) > 100 {
+		invalidRecord = invalidRecord[0:100] + "..."
+	}
+	var andMore string
+	if len(txts) > 1 {
+		andMore = fmt.Sprintf(" (and %d more)", len(txts)-1)
+	}
+	return nil, probs.Unauthorized(fmt.Sprintf(
+		"Incorrect TXT record %q%s found at %s",
+		invalidRecord, andMore, challengeSubdomain))
 }
 
 // validateChallengeAndCAA performs a challenge validation and CAA validation

va/va_test.go

@@ -830,7 +830,7 @@ func TestDNSValidationEmpty(t *testing.T) {
 		"empty-txts.com",
 		chalDNS,
 		core.Authorization{})
-	test.AssertEquals(t, prob.Error(), "unauthorized :: No TXT records found for DNS challenge")
+	test.AssertEquals(t, prob.Error(), "unauthorized :: No TXT record found at _acme-challenge.empty-txts.com")
 
 	samples := test.CountHistogramSamples(va.metrics.validationTime.With(prometheus.Labels{
 		"type":   "dns-01",
@@ -841,6 +841,51 @@ func TestDNSValidationEmpty(t *testing.T) {
 	}
 }
 
+func TestDNSValidationWrong(t *testing.T) {
+	va, _ := setup(nil, 0)
+
+	chalDNS := createChallenge(core.ChallengeTypeDNS01)
+	_, prob := va.PerformValidation(
+		context.Background(),
+		"wrong-dns01.com",
+		chalDNS,
+		core.Authorization{})
+	if prob == nil {
+		t.Fatalf("Successful DNS validation with wrong TXT record")
+	}
+	test.AssertEquals(t, prob.Error(), "unauthorized :: Incorrect TXT record \"a\" found at _acme-challenge.wrong-dns01.com")
+}
+
+func TestDNSValidationWrongMany(t *testing.T) {
+	va, _ := setup(nil, 0)
+
+	chalDNS := createChallenge(core.ChallengeTypeDNS01)
+	_, prob := va.PerformValidation(
+		context.Background(),
+		"wrong-many-dns01.com",
+		chalDNS,
+		core.Authorization{})
+	if prob == nil {
+		t.Fatalf("Successful DNS validation with wrong TXT record")
+	}
+	test.AssertEquals(t, prob.Error(), "unauthorized :: Incorrect TXT record \"a\" (and 4 more) found at _acme-challenge.wrong-many-dns01.com")
+}
+
+func TestDNSValidationWrongLong(t *testing.T) {
+	va, _ := setup(nil, 0)
+
+	chalDNS := createChallenge(core.ChallengeTypeDNS01)
+	_, prob := va.PerformValidation(
+		context.Background(),
+		"long-dns01.com",
+		chalDNS,
+		core.Authorization{})
+	if prob == nil {
+		t.Fatalf("Successful DNS validation with wrong TXT record")
+	}
+	test.AssertEquals(t, prob.Error(), "unauthorized :: Incorrect TXT record \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...\" found at _acme-challenge.long-dns01.com")
+}
+
 func TestPerformValidationValid(t *testing.T) {
 	va, mockLog := setup(nil, 0)