diff --git a/va/caa.go b/va/caa.go index e90087a1193..374fa3dea49 100644 --- a/va/caa.go +++ b/va/caa.go @@ -67,9 +67,11 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC } checkResult := "success" - prob := va.checkCAA(ctx, acmeID, params) + err := va.checkCAA(ctx, acmeID, params) localCheckLatency := time.Since(checkStartTime) - if prob != nil { + var prob *probs.ProblemDetails + if err != nil { + prob = detailedError(err) logEvent.Error = prob.Error() prob.Detail = fmt.Sprintf("While processing CAA for %s: %s", req.Domain, prob.Detail) checkResult = "failure" @@ -116,6 +118,10 @@ func (va *ValidationAuthorityImpl) IsCAAValid(ctx context.Context, req *vapb.IsC va.log.AuditObject("CAA check result", logEvent) 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)), @@ -273,20 +279,20 @@ func (va *ValidationAuthorityImpl) performRemoteCAACheck( func (va *ValidationAuthorityImpl) checkCAA( ctx context.Context, identifier identifier.ACMEIdentifier, - params *caaParams) *probs.ProblemDetails { + params *caaParams) error { if core.IsAnyNilOrZero(params, params.validationMethod, params.accountURIID) { return probs.ServerInternal("expected validationMethod or accountURIID not provided to checkCAA") } foundAt, valid, response, err := va.checkCAARecords(ctx, identifier, params) if err != nil { - return probs.DNS(err.Error()) + return berrors.DNSError("%s", err) } va.log.AuditInfof("Checked CAA records for %s, [Present: %t, Account ID: %d, Challenge: %s, Valid for issuance: %t, Found at: %q] Response=%q", identifier.Value, foundAt != "", params.accountURIID, params.validationMethod, valid, foundAt, response) if !valid { - return probs.CAA(fmt.Sprintf("CAA record for %s prevents issuance", foundAt)) + return berrors.CAAError("CAA record for %s prevents issuance", foundAt) } return nil } diff --git a/va/caa_test.go b/va/caa_test.go index 758fda4c2b0..0170dc0ed3b 100644 --- a/va/caa_test.go +++ b/va/caa_test.go @@ -12,6 +12,7 @@ import ( "github.com/letsencrypt/boulder/bdns" "github.com/letsencrypt/boulder/core" + berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/probs" @@ -197,14 +198,8 @@ func TestCAATimeout(t *testing.T) { } err := va.checkCAA(ctx, identifier.DNSIdentifier("caa-timeout.com"), params) - if err.Type != probs.DNSProblem { - t.Errorf("Expected timeout error type %s, got %s", probs.DNSProblem, err.Type) - } - - expected := "error" - if err.Detail != expected { - t.Errorf("checkCAA: got %#v, expected %#v", err.Detail, expected) - } + test.AssertErrorIs(t, err, berrors.DNS) + test.AssertContains(t, err.Error(), "error") } func TestCAAChecking(t *testing.T) { @@ -970,16 +965,17 @@ func TestCAAFailure(t *testing.T) { va, _ := setup(hs, 0, "", nil, caaMockDNS{}) - _, prob := va.validate(ctx, dnsi("reserved.com"), 1, chall) - if prob == nil { + _, err := va.validate(ctx, dnsi("reserved.com"), 1, chall) + if err == nil { t.Fatalf("Expected CAA rejection for reserved.com, got success") } - test.AssertEquals(t, prob.Type, probs.CAAProblem) + test.AssertErrorIs(t, err, berrors.CAA) - _, prob = va.validate(ctx, dnsi("example.gonetld"), 1, chall) - if prob == nil { + _, err = va.validate(ctx, dnsi("example.gonetld"), 1, chall) + if err == nil { t.Fatalf("Expected CAA rejection for gonetld, got success") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.DNSProblem) test.AssertContains(t, prob.Error(), "NXDOMAIN") } diff --git a/va/dns.go b/va/dns.go index baf3c57e414..93ce38511f6 100644 --- a/va/dns.go +++ b/va/dns.go @@ -12,7 +12,6 @@ import ( "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/identifier" - "github.com/letsencrypt/boulder/probs" ) // getAddr will query for all A/AAAA records associated with hostname and return @@ -49,10 +48,10 @@ func availableAddresses(allAddrs []net.IP) (v4 []net.IP, v6 []net.IP) { return } -func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { +func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, error) { if ident.Type != identifier.DNS { va.log.Infof("Identifier type for DNS challenge was not DNS: %s", ident) - return nil, probs.Malformed("Identifier type for DNS was not itself DNS") + return nil, berrors.MalformedError("Identifier type for DNS was not itself DNS") } // Compute the digest of the key authorization file @@ -64,14 +63,14 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident iden challengeSubdomain := fmt.Sprintf("%s.%s", core.DNSPrefix, ident.Value) txts, resolvers, err := va.dnsClient.LookupTXT(ctx, challengeSubdomain) if err != nil { - return nil, probs.DNS(err.Error()) + return nil, berrors.DNSError("%s", err) } // If there weren't any TXT records return a distinct error message to allow // troubleshooters to differentiate between no TXT records and // invalid/incorrect TXT records. if len(txts) == 0 { - return nil, probs.Unauthorized(fmt.Sprintf("No TXT record found at %s", challengeSubdomain)) + return nil, berrors.UnauthorizedError("No TXT record found at %s", challengeSubdomain) } for _, element := range txts { @@ -89,6 +88,6 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, ident iden 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)) + return nil, berrors.UnauthorizedError("Incorrect TXT record %q%s found at %s", + invalidRecord, andMore, challengeSubdomain) } diff --git a/va/dns_test.go b/va/dns_test.go index c5f7e191916..edac03b8429 100644 --- a/va/dns_test.go +++ b/va/dns_test.go @@ -41,37 +41,41 @@ func TestDNSValidationEmpty(t *testing.T) { func TestDNSValidationWrong(t *testing.T) { va, _ := setup(nil, 0, "", nil, nil) - _, prob := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), dnsChallenge()) - if prob == nil { + _, err := va.validateDNS01(context.Background(), dnsi("wrong-dns01.com"), dnsChallenge()) + if err == nil { t.Fatalf("Successful DNS validation with wrong TXT record") } + prob := detailedError(err) 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, "", nil, nil) - _, prob := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), dnsChallenge()) - if prob == nil { + _, err := va.validateDNS01(context.Background(), dnsi("wrong-many-dns01.com"), dnsChallenge()) + if err == nil { t.Fatalf("Successful DNS validation with wrong TXT record") } + prob := detailedError(err) 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, "", nil, nil) - _, prob := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), dnsChallenge()) - if prob == nil { + _, err := va.validateDNS01(context.Background(), dnsi("long-dns01.com"), dnsChallenge()) + if err == nil { t.Fatalf("Successful DNS validation with wrong TXT record") } + prob := detailedError(err) test.AssertEquals(t, prob.Error(), "unauthorized :: Incorrect TXT record \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...\" found at _acme-challenge.long-dns01.com") } func TestDNSValidationFailure(t *testing.T) { va, _ := setup(nil, 0, "", nil, nil) - _, prob := va.validateDNS01(ctx, dnsi("localhost"), dnsChallenge()) + _, err := va.validateDNS01(ctx, dnsi("localhost"), dnsChallenge()) + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) } @@ -84,7 +88,8 @@ func TestDNSValidationInvalid(t *testing.T) { va, _ := setup(nil, 0, "", nil, nil) - _, prob := va.validateDNS01(ctx, notDNS, dnsChallenge()) + _, err := va.validateDNS01(ctx, notDNS, dnsChallenge()) + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.MalformedProblem) } @@ -94,7 +99,8 @@ func TestDNSValidationNotSane(t *testing.T) { chall := dnsChallenge() chall.Token = "" - _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) + _, err := va.validateChallenge(ctx, dnsi("localhost"), chall) + prob := detailedError(err) if prob.Type != probs.MalformedProblem { t.Errorf("Got wrong error type: expected %s, got %s", prob.Type, probs.MalformedProblem) @@ -104,7 +110,8 @@ func TestDNSValidationNotSane(t *testing.T) { } chall.Token = "yfCBb-bRTLz8Wd1C0lTUQK3qlKj3-t2tYGwx5Hj7r_" - _, prob = va.validateChallenge(ctx, dnsi("localhost"), chall) + _, err = va.validateChallenge(ctx, dnsi("localhost"), chall) + prob = detailedError(err) if prob.Type != probs.MalformedProblem { t.Errorf("Got wrong error type: expected %s, got %s", prob.Type, probs.MalformedProblem) @@ -114,7 +121,8 @@ func TestDNSValidationNotSane(t *testing.T) { } chall.ProvidedKeyAuthorization = "a" - _, prob = va.validateChallenge(ctx, dnsi("localhost"), chall) + _, err = va.validateChallenge(ctx, dnsi("localhost"), chall) + prob = detailedError(err) if prob.Type != probs.MalformedProblem { t.Errorf("Got wrong error type: expected %s, got %s", prob.Type, probs.MalformedProblem) @@ -128,8 +136,9 @@ func TestDNSValidationNotSane(t *testing.T) { func TestDNSValidationServFail(t *testing.T) { va, _ := setup(nil, 0, "", nil, nil) - _, prob := va.validateChallenge(ctx, dnsi("servfail.com"), dnsChallenge()) + _, err := va.validateChallenge(ctx, dnsi("servfail.com"), dnsChallenge()) + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.DNSProblem) } @@ -147,8 +156,8 @@ func TestDNSValidationNoServer(t *testing.T) { log, nil) - _, prob := va.validateChallenge(ctx, dnsi("localhost"), dnsChallenge()) - + _, err = va.validateChallenge(ctx, dnsi("localhost"), dnsChallenge()) + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.DNSProblem) } diff --git a/va/http.go b/va/http.go index 8700b2a0305..a85c0b715bf 100644 --- a/va/http.go +++ b/va/http.go @@ -19,7 +19,6 @@ import ( berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/iana" "github.com/letsencrypt/boulder/identifier" - "github.com/letsencrypt/boulder/probs" ) const ( @@ -395,11 +394,10 @@ func (va *ValidationAuthorityImpl) setupHTTPValidation( func (va *ValidationAuthorityImpl) fetchHTTP( ctx context.Context, host string, - path string) ([]byte, []core.ValidationRecord, *probs.ProblemDetails) { + path string) ([]byte, []core.ValidationRecord, error) { body, records, err := va.processHTTPValidation(ctx, host, path) if err != nil { - // Use detailedError to convert the error into a problem - return body, records, detailedError(err) + return body, records, err } return body, records, nil } @@ -653,24 +651,24 @@ func (va *ValidationAuthorityImpl) processHTTPValidation( return body, records, nil } -func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { +func (va *ValidationAuthorityImpl) validateHTTP01(ctx context.Context, ident identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, error) { if ident.Type != identifier.DNS { va.log.Infof("Got non-DNS identifier for HTTP validation: %s", ident) - return nil, probs.Malformed("Identifier type for HTTP validation was not DNS") + return nil, berrors.MalformedError("Identifier type for HTTP validation was not DNS") } // Perform the fetch path := fmt.Sprintf(".well-known/acme-challenge/%s", challenge.Token) - body, validationRecords, prob := va.fetchHTTP(ctx, ident.Value, "/"+path) - if prob != nil { - return validationRecords, prob + body, validationRecords, err := va.fetchHTTP(ctx, ident.Value, "/"+path) + if err != nil { + return validationRecords, err } payload := strings.TrimRightFunc(string(body), unicode.IsSpace) if payload != challenge.ProvidedKeyAuthorization { - problem := probs.Unauthorized(fmt.Sprintf("The key authorization file from the server did not match this challenge. Expected %q (got %q)", - challenge.ProvidedKeyAuthorization, payload)) - va.log.Infof("%s for %s", problem.Detail, ident) + problem := berrors.UnauthorizedError("The key authorization file from the server did not match this challenge. Expected %q (got %q)", + challenge.ProvidedKeyAuthorization, payload) + va.log.Infof("%s for %s", problem, ident) return validationRecords, problem } diff --git a/va/http_test.go b/va/http_test.go index 19f3ec7486c..f6320cd1ee6 100644 --- a/va/http_test.go +++ b/va/http_test.go @@ -75,31 +75,33 @@ func TestPreresolvedDialerTimeout(t *testing.T) { // is to connect to an unrouteable IP address. This usually generates // a connection timeout, but will rarely return "Network unreachable" instead. // If we get that, just retry until we get something other than "Network unreachable". - var prob *probs.ProblemDetails + var err error var took time.Duration for i := 0; i < 20; i++ { started := time.Now() - _, _, prob = va.fetchHTTP(ctx, "unroutable.invalid", "/.well-known/acme-challenge/whatever") + _, _, err = va.fetchHTTP(ctx, "unroutable.invalid", "/.well-known/acme-challenge/whatever") took = time.Since(started) - if prob != nil && strings.Contains(prob.Detail, "Network unreachable") { + if err != nil && strings.Contains(err.Error(), "Network unreachable") { continue } else { break } } - if prob == nil { + if err == nil { t.Fatalf("Connection should've timed out") } // Check that the HTTP connection doesn't return too fast, and times // out after the expected time if took < va.singleDialTimeout { - t.Fatalf("fetch returned before %s (took: %s) with %#v", va.singleDialTimeout, took, prob) + t.Fatalf("fetch returned before %s (took: %s) with %#v", va.singleDialTimeout, took, err) } if took > 2*va.singleDialTimeout { t.Fatalf("fetch didn't timeout after %s (took: %s)", va.singleDialTimeout, took) } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) + expectMatch := regexp.MustCompile( "Fetching http://unroutable.invalid/.well-known/acme-challenge/.*: Timeout during connect") if !expectMatch.MatchString(prob.Detail) { @@ -1101,12 +1103,13 @@ func TestFetchHTTP(t *testing.T) { t.Run(tc.Name, func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) defer cancel() - body, records, prob := va.fetchHTTP(ctx, tc.Host, tc.Path) - if prob != nil && tc.ExpectedProblem == nil { - t.Errorf("expected nil prob, got %#v\n", prob) - } else if prob == nil && tc.ExpectedProblem != nil { + body, records, err := va.fetchHTTP(ctx, tc.Host, tc.Path) + if err != nil && tc.ExpectedProblem == nil { + t.Errorf("expected nil prob, got %#v\n", err) + } else if err == nil && tc.ExpectedProblem != nil { t.Errorf("expected %#v prob, got nil", tc.ExpectedProblem) - } else if prob != nil && tc.ExpectedProblem != nil { + } else if err != nil && tc.ExpectedProblem != nil { + prob := detailedError(err) test.AssertMarshaledEquals(t, prob, tc.ExpectedProblem) } else { test.AssertEquals(t, string(body), tc.ExpectedBody) @@ -1219,10 +1222,11 @@ func TestHTTPBadPort(t *testing.T) { badPort := 40000 + mrand.Intn(25000) va.httpPort = badPort - _, prob := va.validateHTTP01(ctx, dnsi("localhost"), httpChallenge()) - if prob == nil { + _, err := va.validateHTTP01(ctx, dnsi("localhost"), httpChallenge()) + if err == nil { t.Fatalf("Server's down; expected refusal. Where did we connect?") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) if !strings.Contains(prob.Detail, "Connection refused") { t.Errorf("Expected a connection refused error, got %q", prob.Detail) @@ -1238,14 +1242,14 @@ func TestHTTPKeyAuthorizationFileMismatch(t *testing.T) { hs.Start() va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateHTTP01(ctx, dnsi("localhost.com"), httpChallenge()) + _, err := va.validateHTTP01(ctx, dnsi("localhost.com"), httpChallenge()) - if prob == nil { + if err == nil { t.Fatalf("Expected validation to fail when file mismatched.") } expected := `The key authorization file from the server did not match this challenge. Expected "LoqXcYV8q5ONbJQxbmR7SCTNo3tiAXDfowyjxAjEuX0.9jg46WB3rR_AHD-EBXdN7cBkH1WOu0tA3M9fm21mqTI" (got "\xef\xffAABBCC")` - if prob.Detail != expected { - t.Errorf("validation failed with %s, expected %s", prob.Detail, expected) + if err.Error() != expected { + t.Errorf("validation failed with %s, expected %s", err, expected) } } @@ -1264,36 +1268,37 @@ func TestHTTP(t *testing.T) { chall := httpChallenge() t.Logf("Trying to validate: %+v\n", chall) - _, prob := va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob != nil { - t.Errorf("Unexpected failure in HTTP validation: %s", prob) + _, err := va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err != nil { + t.Errorf("Unexpected failure in HTTP validation: %s", err) } test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) log.Clear() setChallengeToken(&chall, path404) - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob == nil { + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err == nil { t.Fatalf("Should have found a 404 for the challenge.") } - test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) + test.AssertErrorIs(t, err, berrors.Unauthorized) test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) log.Clear() setChallengeToken(&chall, pathWrongToken) // The "wrong token" will actually be the expectedToken. It's wrong // because it doesn't match pathWrongToken. - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob == nil { + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err == nil { t.Fatalf("Should have found the wrong token value.") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) test.AssertEquals(t, len(log.GetAllMatching(`\[AUDIT\] `)), 1) log.Clear() setChallengeToken(&chall, pathMoved) - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob != nil { + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err != nil { t.Fatalf("Failed to follow http.StatusMovedPermanently redirect") } redirectValid := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathValid + `"` @@ -1302,8 +1307,8 @@ func TestHTTP(t *testing.T) { log.Clear() setChallengeToken(&chall, pathFound) - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob != nil { + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err != nil { t.Fatalf("Failed to follow http.StatusFound redirect") } redirectMoved := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathMoved + `"` @@ -1312,16 +1317,17 @@ func TestHTTP(t *testing.T) { test.AssertEquals(t, len(matchedMovedRedirect), 1) ipIdentifier := identifier.ACMEIdentifier{Type: identifier.IdentifierType("ip"), Value: "127.0.0.1"} - _, prob = va.validateHTTP01(ctx, ipIdentifier, chall) - if prob == nil { + _, err = va.validateHTTP01(ctx, ipIdentifier, chall) + if err == nil { t.Fatalf("IdentifierType IP shouldn't have worked.") } - test.AssertEquals(t, prob.Type, probs.MalformedProblem) + test.AssertErrorIs(t, err, berrors.Malformed) - _, prob = va.validateHTTP01(ctx, identifier.ACMEIdentifier{Type: identifier.DNS, Value: "always.invalid"}, chall) - if prob == nil { + _, err = va.validateHTTP01(ctx, identifier.ACMEIdentifier{Type: identifier.DNS, Value: "always.invalid"}, chall) + if err == nil { t.Fatalf("Domain name is invalid.") } + prob = detailedError(err) test.AssertEquals(t, prob.Type, probs.DNSProblem) } @@ -1338,8 +1344,8 @@ func TestHTTPTimeout(t *testing.T) { timeout := 250 * time.Millisecond ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) - if prob == nil { + _, err := va.validateHTTP01(ctx, dnsi("localhost"), chall) + if err == nil { t.Fatalf("Connection should've timed out") } @@ -1347,11 +1353,12 @@ func TestHTTPTimeout(t *testing.T) { // Check that the HTTP connection doesn't return before a timeout, and times // out after the expected time if took < timeout-200*time.Millisecond { - t.Fatalf("HTTP timed out before %s: %s with %s", timeout, took, prob) + t.Fatalf("HTTP timed out before %s: %s with %s", timeout, took, err) } if took > 2*timeout { t.Fatalf("HTTP connection didn't timeout after %s", timeout) } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) test.AssertEquals(t, prob.Detail, "127.0.0.1: Fetching http://localhost/.well-known/acme-challenge/wait-long: Timeout after connect (your server may be slow or overloaded)") } @@ -1383,27 +1390,28 @@ func TestHTTPDialTimeout(t *testing.T) { // connect to an unrouteable IP address. This usually generates a connection // timeout, but will rarely return "Network unreachable" instead. If we get // that, just retry until we get something other than "Network unreachable". - var prob *probs.ProblemDetails + var err error for i := 0; i < 20; i++ { - _, prob = va.validateHTTP01(ctx, dnsi("unroutable.invalid"), httpChallenge()) - if prob != nil && strings.Contains(prob.Detail, "Network unreachable") { + _, err = va.validateHTTP01(ctx, dnsi("unroutable.invalid"), httpChallenge()) + if err != nil && strings.Contains(err.Error(), "Network unreachable") { continue } else { break } } - if prob == nil { + if err == nil { t.Fatalf("Connection should've timed out") } took := time.Since(started) // Check that the HTTP connection doesn't return too fast, and times // out after the expected time if took < (timeout-200*time.Millisecond)/2 { - t.Fatalf("HTTP returned before %s (%s) with %#v", timeout, took, prob) + t.Fatalf("HTTP returned before %s (%s) with %#v", timeout, took, err) } if took > 2*timeout { t.Fatalf("HTTP connection didn't timeout after %s seconds", timeout) } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) expectMatch := regexp.MustCompile( "Fetching http://unroutable.invalid/.well-known/acme-challenge/.*: Timeout during connect") @@ -1420,9 +1428,9 @@ func TestHTTPRedirectLookup(t *testing.T) { chall := httpChallenge() setChallengeToken(&chall, pathMoved) - _, prob := va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob != nil { - t.Fatalf("Unexpected failure in redirect (%s): %s", pathMoved, prob) + _, err := va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err != nil { + t.Fatalf("Unexpected failure in redirect (%s): %s", pathMoved, err) } redirectValid := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathValid + `"` matchedValidRedirect := log.GetAllMatching(redirectValid) @@ -1431,9 +1439,9 @@ func TestHTTPRedirectLookup(t *testing.T) { log.Clear() setChallengeToken(&chall, pathFound) - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob != nil { - t.Fatalf("Unexpected failure in redirect (%s): %s", pathFound, prob) + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err != nil { + t.Fatalf("Unexpected failure in redirect (%s): %s", pathFound, err) } redirectMoved := `following redirect to host "" url "http://localhost.com/.well-known/acme-challenge/` + pathMoved + `"` matchedMovedRedirect := log.GetAllMatching(redirectMoved) @@ -1442,16 +1450,17 @@ func TestHTTPRedirectLookup(t *testing.T) { log.Clear() setChallengeToken(&chall, pathReLookupInvalid) - _, err := va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) test.AssertError(t, err, chall.Token) test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost.com: \[127.0.0.1\]`)), 1) - test.AssertDeepEquals(t, err, probs.Connection(`127.0.0.1: Fetching http://invalid.invalid/path: Invalid hostname in redirect target, must end in IANA registered TLD`)) + prob := detailedError(err) + test.AssertDeepEquals(t, prob, probs.Connection(`127.0.0.1: Fetching http://invalid.invalid/path: Invalid hostname in redirect target, must end in IANA registered TLD`)) log.Clear() setChallengeToken(&chall, pathReLookup) - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - if prob != nil { - t.Fatalf("Unexpected error in redirect (%s): %s", pathReLookup, prob) + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + if err != nil { + t.Fatalf("Unexpected error in redirect (%s): %s", pathReLookup, err) } redirectPattern := `following redirect to host "" url "http://other.valid.com:\d+/path"` test.AssertEquals(t, len(log.GetAllMatching(redirectPattern)), 1) @@ -1460,8 +1469,9 @@ func TestHTTPRedirectLookup(t *testing.T) { log.Clear() setChallengeToken(&chall, pathRedirectInvalidPort) - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - test.AssertNotNil(t, prob, "Problem details for pathRedirectInvalidPort should not be nil") + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + test.AssertNotNil(t, err, "error for pathRedirectInvalidPort should not be nil") + prob = detailedError(err) test.AssertEquals(t, prob.Detail, fmt.Sprintf( "127.0.0.1: Fetching http://other.valid.com:8080/path: Invalid port in redirect target. "+ "Only ports %d and %d are supported, not 8080", va.httpPort, va.httpsPort)) @@ -1471,8 +1481,9 @@ func TestHTTPRedirectLookup(t *testing.T) { // is referencing the redirected to host, instead of the original host. log.Clear() setChallengeToken(&chall, pathRedirectToFailingURL) - _, prob = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) - test.AssertNotNil(t, prob, "Problem Details should not be nil") + _, err = va.validateHTTP01(ctx, dnsi("localhost.com"), chall) + test.AssertNotNil(t, err, "err should not be nil") + prob = detailedError(err) test.AssertDeepEquals(t, prob, probs.Unauthorized( fmt.Sprintf("127.0.0.1: Invalid response from http://other.valid.com:%d/500: 500", @@ -1549,13 +1560,14 @@ func TestLimitedReader(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) defer hs.Close() - _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) + _, err := va.validateChallenge(ctx, dnsi("localhost"), chall) + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) test.Assert(t, strings.HasPrefix(prob.Detail, "127.0.0.1: Invalid response from "), "Expected failure due to truncation") - if !utf8.ValidString(prob.Detail) { + if !utf8.ValidString(err.Error()) { t.Errorf("Problem Detail contained an invalid UTF-8 string") } } diff --git a/va/tlsalpn.go b/va/tlsalpn.go index f73e8e9e298..052a67ba144 100644 --- a/va/tlsalpn.go +++ b/va/tlsalpn.go @@ -17,8 +17,8 @@ import ( "strings" "github.com/letsencrypt/boulder/core" + berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/identifier" - "github.com/letsencrypt/boulder/probs" ) const ( @@ -58,7 +58,7 @@ func certAltNames(cert *x509.Certificate) []string { func (va *ValidationAuthorityImpl) tryGetChallengeCert(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge, - tlsConfig *tls.Config) (*x509.Certificate, *tls.ConnectionState, []core.ValidationRecord, *probs.ProblemDetails) { + tlsConfig *tls.Config) (*x509.Certificate, *tls.ConnectionState, []core.ValidationRecord, error) { allAddrs, resolvers, err := va.getAddrs(ctx, identifier.Value) validationRecords := []core.ValidationRecord{ @@ -70,7 +70,7 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert(ctx context.Context, }, } if err != nil { - return nil, nil, validationRecords, detailedError(err) + return nil, nil, validationRecords, err } thisRecord := &validationRecords[0] @@ -80,7 +80,7 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert(ctx context.Context, // This shouldn't happen, but be defensive about it anyway if len(addresses) < 1 { - return nil, nil, validationRecords, probs.Malformed("no IP addresses found for %q", identifier.Value) + return nil, nil, validationRecords, berrors.MalformedError("no IP addresses found for %q", identifier.Value) } // If there is at least one IPv6 address then try it first @@ -88,11 +88,11 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert(ctx context.Context, address := net.JoinHostPort(v6[0].String(), thisRecord.Port) thisRecord.AddressUsed = v6[0] - cert, cs, prob := va.getChallengeCert(ctx, address, identifier, challenge, tlsConfig) + cert, cs, err := va.getChallengeCert(ctx, address, identifier, challenge, tlsConfig) // If there is no problem, return immediately - if prob == nil { - return cert, cs, validationRecords, prob + if err == nil { + return cert, cs, validationRecords, nil } // Otherwise, we note that we tried an address and fall back to trying IPv4 @@ -103,20 +103,20 @@ func (va *ValidationAuthorityImpl) tryGetChallengeCert(ctx context.Context, // If there are no IPv4 addresses and we tried an IPv6 address return // an error - there's nothing left to try if len(v4) == 0 && len(thisRecord.AddressesTried) > 0 { - return nil, nil, validationRecords, probs.Malformed("Unable to contact %q at %q, no IPv4 addresses to try as fallback", + return nil, nil, validationRecords, berrors.MalformedError("Unable to contact %q at %q, no IPv4 addresses to try as fallback", thisRecord.Hostname, thisRecord.AddressesTried[0]) } else if len(v4) == 0 && len(thisRecord.AddressesTried) == 0 { // It shouldn't be possible that there are no IPv4 addresses and no previous // attempts at an IPv6 address connection but be defensive about it anyway - return nil, nil, validationRecords, probs.Malformed("No IP addresses found for %q", thisRecord.Hostname) + return nil, nil, validationRecords, berrors.MalformedError("No IP addresses found for %q", thisRecord.Hostname) } // Otherwise if there are no IPv6 addresses, or there was an error // talking to the first IPv6 address, try the first IPv4 address thisRecord.AddressUsed = v4[0] - cert, cs, prob := va.getChallengeCert(ctx, net.JoinHostPort(v4[0].String(), thisRecord.Port), + cert, cs, err := va.getChallengeCert(ctx, net.JoinHostPort(v4[0].String(), thisRecord.Port), identifier, challenge, tlsConfig) - return cert, cs, validationRecords, prob + return cert, cs, validationRecords, err } func (va *ValidationAuthorityImpl) getChallengeCert( @@ -125,7 +125,7 @@ func (va *ValidationAuthorityImpl) getChallengeCert( identifier identifier.ACMEIdentifier, challenge core.Challenge, config *tls.Config, -) (*x509.Certificate, *tls.ConnectionState, *probs.ProblemDetails) { +) (*x509.Certificate, *tls.ConnectionState, error) { va.log.Info(fmt.Sprintf("%s [%s] Attempting to validate for %s %s", challenge.Type, identifier, hostPort, config.ServerName)) // We expect a self-signed challenge certificate, do not verify it here. config.InsecureSkipVerify = true @@ -142,9 +142,9 @@ func (va *ValidationAuthorityImpl) getChallengeCert( // Wrap the validation error and the IP of the remote host in an // IPError so we can display the IP in the problem details returned // to the client. - return nil, nil, detailedError(ipError{net.ParseIP(host), err}) + return nil, nil, ipError{net.ParseIP(host), err} } - return nil, nil, detailedError(err) + return nil, nil, err } defer conn.Close() @@ -153,7 +153,7 @@ func (va *ValidationAuthorityImpl) getChallengeCert( certs := cs.PeerCertificates if len(certs) == 0 { va.log.Infof("%s challenge for %s resulted in no certificates", challenge.Type, identifier.Value) - return nil, nil, probs.Unauthorized(fmt.Sprintf("No certs presented for %s challenge", challenge.Type)) + return nil, nil, berrors.UnauthorizedError("No certs presented for %s challenge", challenge.Type) } for i, cert := range certs { va.log.AuditInfof("%s challenge for %s received certificate (%d of %d): cert=[%s]", @@ -207,10 +207,10 @@ func checkAcceptableExtensions(exts []pkix.Extension, requiredOIDs []asn1.Object return nil } -func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { +func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, error) { if identifier.Type != "dns" { va.log.Info(fmt.Sprintf("Identifier type for TLS-ALPN-01 was not DNS: %s", identifier)) - return nil, probs.Malformed("Identifier type for TLS-ALPN-01 was not DNS") + return nil, berrors.MalformedError("Identifier type for TLS-ALPN-01 was not DNS") } cert, cs, validationRecords, problem := va.tryGetChallengeCert(ctx, identifier, challenge, &tls.Config{ @@ -223,23 +223,19 @@ func (va *ValidationAuthorityImpl) validateTLSALPN01(ctx context.Context, identi } if cs.NegotiatedProtocol != ACMETLS1Protocol { - errText := fmt.Sprintf( + return validationRecords, berrors.UnauthorizedError( "Cannot negotiate ALPN protocol %q for %s challenge", ACMETLS1Protocol, - core.ChallengeTypeTLSALPN01, - ) - return validationRecords, probs.Unauthorized(errText) + core.ChallengeTypeTLSALPN01) } - badCertErr := func(msg string) *probs.ProblemDetails { + badCertErr := func(msg string) error { hostPort := net.JoinHostPort(validationRecords[0].AddressUsed.String(), validationRecords[0].Port) - return probs.Unauthorized(fmt.Sprintf( + return berrors.UnauthorizedError( "Incorrect validation certificate for %s challenge. "+ - "Requested %s from %s. "+ - "%s", - challenge.Type, identifier.Value, hostPort, msg, - )) + "Requested %s from %s. %s", + challenge.Type, identifier.Value, hostPort, msg) } // The certificate must be self-signed. diff --git a/va/tlsalpn_test.go b/va/tlsalpn_test.go index 20cdfe75448..846de340d90 100644 --- a/va/tlsalpn_test.go +++ b/va/tlsalpn_test.go @@ -144,13 +144,14 @@ func TestTLSALPN01FailIP(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) port := getPort(hs) - _, prob := va.validateTLSALPN01(ctx, identifier.ACMEIdentifier{ + _, err = va.validateTLSALPN01(ctx, identifier.ACMEIdentifier{ Type: identifier.IdentifierType("ip"), Value: net.JoinHostPort("127.0.0.1", strconv.Itoa(port)), }, chall) - if prob == nil { + if err == nil { t.Fatalf("IdentifierType IP shouldn't have worked.") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.MalformedProblem) } @@ -177,8 +178,8 @@ func TestTLSALPNTimeoutAfterConnect(t *testing.T) { defer cancel() started := time.Now() - _, prob := va.validateTLSALPN01(ctx, dnsi("slow.server"), chall) - if prob == nil { + _, err := va.validateTLSALPN01(ctx, dnsi("slow.server"), chall) + if err == nil { t.Fatalf("Validation should've failed") } // Check that the TLS connection doesn't return before a timeout, and times @@ -187,15 +188,16 @@ func TestTLSALPNTimeoutAfterConnect(t *testing.T) { // Check that the HTTP connection doesn't return too fast, and times // out after the expected time if took < timeout/2 { - t.Fatalf("TLSSNI returned before %s (%s) with %#v", timeout, took, prob) + t.Fatalf("TLSSNI returned before %s (%s) with %#v", timeout, took, err) } if took > 2*timeout { t.Fatalf("TLSSNI didn't timeout after %s (took %s to return %#v)", timeout, - took, prob) + took, err) } - if prob == nil { + if err == nil { t.Fatalf("Connection should've timed out") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) expected := "127.0.0.1: Timeout after connect (your server may be slow or overloaded)" @@ -218,17 +220,17 @@ func TestTLSALPN01DialTimeout(t *testing.T) { // connect to an unrouteable IP address. This usually generates a connection // timeout, but will rarely return "Network unreachable" instead. If we get // that, just retry until we get something other than "Network unreachable". - var prob *probs.ProblemDetails + var err error for i := 0; i < 20; i++ { - _, prob = va.validateTLSALPN01(ctx, dnsi("unroutable.invalid"), chall) - if prob != nil && strings.Contains(prob.Detail, "Network unreachable") { + _, err = va.validateTLSALPN01(ctx, dnsi("unroutable.invalid"), chall) + if err != nil && strings.Contains(err.Error(), "Network unreachable") { continue } else { break } } - if prob == nil { + if err == nil { t.Fatalf("Validation should've failed") } // Check that the TLS connection doesn't return before a timeout, and times @@ -237,14 +239,15 @@ func TestTLSALPN01DialTimeout(t *testing.T) { // Check that the HTTP connection doesn't return too fast, and times // out after the expected time if took < timeout/2 { - t.Fatalf("TLSSNI returned before %s (%s) with %#v", timeout, took, prob) + t.Fatalf("TLSSNI returned before %s (%s) with %#v", timeout, took, err) } if took > 2*timeout { t.Fatalf("TLSSNI didn't timeout after %s", timeout) } - if prob == nil { + if err == nil { t.Fatalf("Connection should've timed out") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) expected := "198.51.100.1: Timeout during connect (likely firewall problem)" if prob.Detail != expected { @@ -260,10 +263,11 @@ func TestTLSALPN01Refused(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) // Take down validation server and check that validation fails. hs.Close() - _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) - if prob == nil { + _, err = va.validateTLSALPN01(ctx, dnsi("expected"), chall) + if err == nil { t.Fatalf("Server's down; expected refusal. Where did we connect?") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) expected := "127.0.0.1: Connection refused" if prob.Detail != expected { @@ -280,11 +284,12 @@ func TestTLSALPN01TalkingToHTTP(t *testing.T) { httpOnly := httpSrv(t, "") va.tlsPort = getPort(httpOnly) - _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) - test.AssertError(t, prob, "TLS-SNI-01 validation passed when talking to a HTTP-only server") + _, err = va.validateTLSALPN01(ctx, dnsi("expected"), chall) + test.AssertError(t, err, "TLS-SNI-01 validation passed when talking to a HTTP-only server") + prob := detailedError(err) expected := "Server only speaks HTTP, not TLS" - if !strings.HasSuffix(prob.Detail, expected) { - t.Errorf("Got wrong error detail. Expected %q, got %q", expected, prob.Detail) + if !strings.HasSuffix(prob.Error(), expected) { + t.Errorf("Got wrong error detail. Expected %q, got %q", expected, prob) } } @@ -305,10 +310,11 @@ func TestTLSError(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) - if prob == nil { + _, err := va.validateTLSALPN01(ctx, dnsi("expected"), chall) + if err == nil { t.Fatalf("TLS validation should have failed: What cert was used?") } + prob := detailedError(err) if prob.Type != probs.TLSProblem { t.Errorf("Wrong problem type: got %s, expected type %s", prob, probs.TLSProblem) @@ -321,10 +327,11 @@ func TestDNSError(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateTLSALPN01(ctx, dnsi("always.invalid"), chall) - if prob == nil { + _, err := va.validateTLSALPN01(ctx, dnsi("always.invalid"), chall) + if err == nil { t.Fatalf("TLS validation should have failed: what IP was used?") } + prob := detailedError(err) if prob.Type != probs.DNSProblem { t.Errorf("Wrong problem type: got %s, expected type %s", prob, probs.DNSProblem) @@ -436,19 +443,21 @@ func TestValidateTLSALPN01BadChallenge(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) + _, err = va.validateTLSALPN01(ctx, dnsi("expected"), chall) - if prob == nil { + if err == nil { t.Fatalf("TLS ALPN validation should have failed.") } + + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) expectedDigest := sha256.Sum256([]byte(chall.ProvidedKeyAuthorization)) badDigest := sha256.Sum256([]byte(chall2.ProvidedKeyAuthorization)) - test.AssertContains(t, prob.Detail, string(core.ChallengeTypeTLSALPN01)) - test.AssertContains(t, prob.Detail, hex.EncodeToString(expectedDigest[:])) - test.AssertContains(t, prob.Detail, hex.EncodeToString(badDigest[:])) + test.AssertContains(t, err.Error(), string(core.ChallengeTypeTLSALPN01)) + test.AssertContains(t, err.Error(), hex.EncodeToString(expectedDigest[:])) + test.AssertContains(t, err.Error(), hex.EncodeToString(badDigest[:])) } func TestValidateTLSALPN01BrokenSrv(t *testing.T) { @@ -457,10 +466,11 @@ func TestValidateTLSALPN01BrokenSrv(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) - if prob == nil { + _, err := va.validateTLSALPN01(ctx, dnsi("expected"), chall) + if err == nil { t.Fatalf("TLS ALPN validation should have failed.") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.TLSProblem) } @@ -470,10 +480,11 @@ func TestValidateTLSALPN01UnawareSrv(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) - if prob == nil { + _, err := va.validateTLSALPN01(ctx, dnsi("expected"), chall) + if err == nil { t.Fatalf("TLS ALPN validation should have failed.") } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.TLSProblem) } @@ -522,14 +533,15 @@ func TestValidateTLSALPN01MalformedExtnValue(t *testing.T) { hs := tlsalpn01SrvWithCert(t, acmeCert, 0) va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateTLSALPN01(ctx, dnsi("expected"), chall) + _, err := va.validateTLSALPN01(ctx, dnsi("expected"), chall) hs.Close() - if prob == nil { + if err == nil { t.Errorf("TLS ALPN validation should have failed for acmeValidation extension %+v.", badExt) continue } + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) test.AssertContains(t, prob.Detail, string(core.ChallengeTypeTLSALPN01)) test.AssertContains(t, prob.Detail, "malformed acmeValidationV1 extension value") @@ -660,9 +672,9 @@ func TestTLSALPN01NotSelfSigned(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) - test.AssertError(t, prob, "validation should have failed") - test.AssertContains(t, prob.Detail, "not self-signed") + _, err = va.validateChallenge(ctx, dnsi("expected"), chall) + test.AssertError(t, err, "validation should have failed") + test.AssertContains(t, err.Error(), "not self-signed") } func TestTLSALPN01ExtraIdentifiers(t *testing.T) { @@ -767,10 +779,11 @@ func TestTLSALPN01ExtraSANs(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) - test.AssertError(t, prob, "validation should have failed") + _, err = va.validateChallenge(ctx, dnsi("expected"), chall) + test.AssertError(t, err, "validation should have failed") // In go >= 1.19, the TLS client library detects that the certificate has // a duplicate extension and terminates the connection itself. + prob := detailedError(err) test.AssertContains(t, prob.Error(), "Error getting validation data") } @@ -822,8 +835,9 @@ func TestTLSALPN01ExtraAcmeExtensions(t *testing.T) { va, _ := setup(hs, 0, "", nil, nil) - _, prob := va.validateChallenge(ctx, dnsi("expected"), chall) - test.AssertError(t, prob, "validation should have failed") + _, err = va.validateChallenge(ctx, dnsi("expected"), chall) + test.AssertError(t, err, "validation should have failed") + prob := detailedError(err) // In go >= 1.19, the TLS client library detects that the certificate has // a duplicate extension and terminates the connection itself. test.AssertContains(t, prob.Error(), "Error getting validation data") diff --git a/va/va.go b/va/va.go index cd41c4e2eea..1de02588d33 100644 --- a/va/va.go +++ b/va/va.go @@ -403,6 +403,12 @@ func detailedError(err error) *probs.ProblemDetails { if errors.Is(err, berrors.DNS) { return probs.DNS(err.Error()) } + if errors.Is(err, berrors.Malformed) { + return probs.Malformed(err.Error()) + } + if errors.Is(err, berrors.CAA) { + return probs.CAA(err.Error()) + } if h2SettingsFrameErrRegex.MatchString(err.Error()) { return probs.Connection("Server is speaking HTTP/2 over HTTP") @@ -419,7 +425,7 @@ func (va *ValidationAuthorityImpl) validate( identifier identifier.ACMEIdentifier, regid int64, challenge core.Challenge, -) ([]core.ValidationRecord, *probs.ProblemDetails) { +) ([]core.ValidationRecord, error) { // If the identifier is a wildcard domain we need to validate the base // domain by removing the "*." wildcard prefix. We create a separate @@ -432,7 +438,7 @@ func (va *ValidationAuthorityImpl) validate( // Create this channel outside of the feature-conditional block so that it is // also in scope for the matching block below. - ch := make(chan *probs.ProblemDetails, 1) + ch := make(chan error, 1) if !features.Get().CAAAfterValidation { // va.checkCAA accepts wildcard identifiers and handles them appropriately so // we can dispatch `checkCAA` with the provided `identifier` instead of @@ -447,20 +453,15 @@ func (va *ValidationAuthorityImpl) validate( } // TODO(#1292): send into another goroutine - validationRecords, prob := va.validateChallenge(ctx, baseIdentifier, challenge) - 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 validationRecords, prob + validationRecords, err := va.validateChallenge(ctx, baseIdentifier, challenge) + if err != nil { + return validationRecords, err } if !features.Get().CAAAfterValidation { for i := 0; i < cap(ch); i++ { - if extraProblem := <-ch; extraProblem != nil { - return validationRecords, extraProblem + if extraErr := <-ch; extraErr != nil { + return validationRecords, extraErr } } } else { @@ -468,19 +469,19 @@ func (va *ValidationAuthorityImpl) validate( accountURIID: regid, validationMethod: challenge.Type, } - prob := va.checkCAA(ctx, identifier, params) - if prob != nil { - return validationRecords, prob + err := va.checkCAA(ctx, identifier, params) + if err != nil { + return validationRecords, err } } return validationRecords, nil } -func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { +func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identifier identifier.ACMEIdentifier, challenge core.Challenge) ([]core.ValidationRecord, error) { err := challenge.CheckConsistencyForValidation() if err != nil { - return nil, probs.Malformed("Challenge failed consistency check: %s", err) + return nil, berrors.MalformedError("Challenge failed consistency check: %s", err) } switch challenge.Type { case core.ChallengeTypeHTTP01: @@ -490,7 +491,7 @@ func (va *ValidationAuthorityImpl) validateChallenge(ctx context.Context, identi case core.ChallengeTypeTLSALPN01: return va.validateTLSALPN01(ctx, identifier, challenge) } - return nil, probs.Malformed("invalid challenge type %s", challenge.Type) + return nil, berrors.MalformedError("invalid challenge type %s", challenge.Type) } // performRemoteValidation calls `PerformValidation` for each of the configured @@ -717,29 +718,28 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v challenge, err := bgrpc.PBToChallenge(req.Challenge) if err != nil { - return nil, probs.ServerInternal("Challenge failed to deserialize") + return nil, errors.New("Challenge failed to deserialize") } - records, prob := va.validate(ctx, identifier.DNSIdentifier(req.Domain), req.Authz.RegID, challenge) + records, err := va.validate(ctx, identifier.DNSIdentifier(req.Domain), req.Authz.RegID, challenge) challenge.ValidationRecord = records localValidationLatency := time.Since(vStart) // Check for malformed ValidationRecords - if !challenge.RecordsSane() && prob == nil { - prob = probs.ServerInternal("Records for validation failed sanity check") + if !challenge.RecordsSane() && err == nil { + err = errors.New("Records for validation failed sanity check") } var problemType string - if prob != nil { + var prob *probs.ProblemDetails + if err != nil { + prob = detailedError(err) problemType = string(prob.Type) challenge.Status = core.StatusInvalid challenge.Error = prob logEvent.Error = prob.Error() } else if remoteResults != nil { if !features.Get().EnforceMultiVA && features.Get().MultiVAFullResults { - // If we're not going to enforce multi VA but we are logging the - // differentials then collect and log the remote results in a separate go - // routine to avoid blocking the primary VA. go func() { _ = va.processRemoteValidationResults( req.Domain, @@ -793,5 +793,9 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, req *v va.log.AuditObject("Validation result", logEvent) + // 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 bgrpc.ValidationResultToPB(records, prob) } diff --git a/va/va_test.go b/va/va_test.go index a333a225fad..27486760732 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -257,8 +257,9 @@ func (inmem inMemVA) IsCAAValid(ctx context.Context, req *vapb.IsCAAValidRequest func TestValidateMalformedChallenge(t *testing.T) { va, _ := setup(nil, 0, "", nil, nil) - _, prob := va.validateChallenge(ctx, dnsi("example.com"), createChallenge("fake-type-01")) + _, err := va.validateChallenge(ctx, dnsi("example.com"), createChallenge("fake-type-01")) + prob := detailedError(err) test.AssertEquals(t, prob.Type, probs.MalformedProblem) }