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

@@ -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) {

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