From f236ca522f0907dda45f4fe4e833b3772bbdd089 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 12 Jun 2017 07:35:22 -0700 Subject: [PATCH] Improve validation error messages. (#2791) Previously, a lot of validations problems would give the message "Failed to connect to X for ..." This was misleading because the issue was not always a connection error, and when it was, it was valuable to distinguish between connection refused and timeout. Also, for HTTP, this message would echo the first URL in a redirect chain, when we really want the URL that failed. Renames the misleading "parseHTTPConnError" and removes an inaccurate check for temporary errors. It also eliminates the "detail" argument, instead generating all messages inside the function. Improves the handling of tls.alert errors to actually pass through the error message, rather than just quietly changing the problem type (which was very easy to miss). Gives a specific error message for timeouts. Preserves the URL from url.Error types and incorporates it into error messages. Splits the HTTP timeout test into its own test case. --- va/va.go | 38 ++++++++++++++++++++------------------ va/va_test.go | 26 +++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/va/va.go b/va/va.go index 832aca8a2..675749f7b 100644 --- a/va/va.go +++ b/va/va.go @@ -338,8 +338,7 @@ func (va *ValidationAuthorityImpl) fetchHTTP(ctx context.Context, identifier cor validationRecords = append(validationRecords, dialer.record) if err != nil { va.log.Info(fmt.Sprintf("HTTP request to %s failed. err=[%#v] errStr=[%s]", url, err, err)) - return nil, validationRecords, - parseHTTPConnError(fmt.Sprintf("Could not connect to %s", urlHost), err) + return nil, validationRecords, detailedError(err) } body, err := ioutil.ReadAll(&io.LimitedReader{R: httpResponse.Body, N: maxResponseSize}) @@ -521,8 +520,7 @@ func (va *ValidationAuthorityImpl) getTLSSNICerts(hostPort string, identifier co if err != nil { va.log.Info(fmt.Sprintf("%s connection failure for %s. err=[%#v] errStr=[%s]", challenge.Type, identifier, err, err)) - return nil, - parseHTTPConnError(fmt.Sprintf("Failed to connect to %s for %s challenge", hostPort, challenge.Type), err) + return nil, detailedError(err) } // close errors are not important here defer func() { @@ -607,30 +605,34 @@ func (va *ValidationAuthorityImpl) validateTLSSNI02(ctx context.Context, identif // we try to talk TLS to a server that only talks HTTP var badTLSHeader = []byte{0x48, 0x54, 0x54, 0x50, 0x2f} -// parseHTTPConnError returns a ProblemDetails corresponding to an error -// that occurred during domain validation. -func parseHTTPConnError(detail string, err error) *probs.ProblemDetails { +// detailedError returns a ProblemDetails corresponding to an error +// that occurred during HTTP-01 or TLS-SNI domain validation. Specifically it +// tries to unwrap known Go error types and present something a little more +// meaningful. +func detailedError(err error) *probs.ProblemDetails { + // net/http wraps net.OpError in a url.Error. Unwrap them. if urlErr, ok := err.(*url.Error); ok { - err = urlErr.Err + prob := detailedError(urlErr.Err) + prob.Detail = fmt.Sprintf("Fetching %s: %s", urlErr.URL, prob.Detail) + return prob } if tlsErr, ok := err.(tls.RecordHeaderError); ok && bytes.Compare(tlsErr.RecordHeader[:], badTLSHeader) == 0 { - return probs.Malformed(fmt.Sprintf("%s: Server only speaks HTTP, not TLS", detail)) + return probs.Malformed(fmt.Sprintf("Server only speaks HTTP, not TLS")) } - // XXX: On all of the resolvers I tested that validate DNSSEC, there is - // no differentiation between a DNSSEC failure and an unknown host. If we - // do not verify DNSSEC ourselves, this function should be modified. if netErr, ok := err.(*net.OpError); ok { - dnsErr, ok := netErr.Err.(*net.DNSError) - if ok && !dnsErr.Timeout() && !dnsErr.Temporary() { - return probs.UnknownHost(detail) - } else if fmt.Sprintf("%T", netErr.Err) == "tls.alert" { - return probs.TLSError(detail) + if fmt.Sprintf("%T", netErr.Err) == "tls.alert" { + // As of Go 1.8, all the tls.alert error strings are reasonable to hand back to a + // user. + return probs.TLSError(netErr.Error()) } } + if err, ok := err.(net.Error); ok && err.Timeout() { + return probs.ConnectionFailure("Timeout") + } - return probs.ConnectionFailure(detail) + return probs.ConnectionFailure("Error getting validation data") } func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, identifier core.AcmeIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { diff --git a/va/va_test.go b/va/va_test.go index 94bf86c9c..3259bd873 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -17,6 +17,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "regexp" "strconv" "strings" "testing" @@ -316,10 +317,20 @@ func TestHTTP(t *testing.T) { t.Fatalf("Domain name is invalid.") } test.AssertEquals(t, prob.Type, probs.UnknownHostProblem) +} + +func TestHTTPTimeout(t *testing.T) { + chall := core.HTTPChallenge01() + setChallengeToken(&chall, expectedToken) + + hs := httpSrv(t, chall.Token) + // TODO(#1989): close hs + + va, _ := setup(hs) setChallengeToken(&chall, pathWaitLong) started := time.Now() - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob := va.validateHTTP01(ctx, ident, chall) took := time.Since(started) // Check that the HTTP connection times out after 5 seconds and doesn't block for 10 seconds test.Assert(t, (took > (time.Second * 5)), "HTTP timed out before 5 seconds") @@ -328,6 +339,12 @@ func TestHTTP(t *testing.T) { t.Fatalf("Connection should've timed out") } test.AssertEquals(t, prob.Type, probs.ConnectionProblem) + expectMatch := regexp.MustCompile( + "Fetching http://localhost:\\d+/.well-known/acme-challenge/wait-long: Timeout") + if !expectMatch.MatchString(prob.Detail) { + t.Errorf("Problem details incorrect. Got %q, expected to match %q", + prob.Detail, expectMatch) + } } func TestHTTPRedirectLookup(t *testing.T) { @@ -388,7 +405,7 @@ func TestHTTPRedirectLookup(t *testing.T) { setChallengeToken(&chall, pathRedirectToFailingURL) _, prob = va.validateHTTP01(ctx, ident, chall) test.AssertNotNil(t, prob, "Problem Details should not be nil") - test.AssertEquals(t, prob.Detail, "Could not connect to other.valid") + test.AssertEquals(t, prob.Detail, "Fetching http://other.valid/500: Error getting validation data") } func TestHTTPRedirectLoop(t *testing.T) { @@ -609,7 +626,10 @@ func TestTLSError(t *testing.T) { if prob == nil { t.Fatalf("TLS validation should have failed: What cert was used?") } - test.AssertEquals(t, prob.Type, probs.TLSProblem) + if prob.Type != probs.TLSProblem { + t.Errorf("Wrong problem type: got %s, expected type %s", + prob, probs.TLSProblem) + } } // misconfiguredTLSSrv is a TLS HTTP test server that returns a certificate