va: clean up DNS error handling for HTTP-01 challenges.

The `va.getAddrs` function is internal to the VA and can return
`berrors.BoulderError`s with a DNS type when there is an error,
allowing the calling code to convert this to a problem when required
using the existing `detailedError` function. This avoids some clunky
conversion the HTTP-01 code was doing that misrepresented DNS level
errors as connection problems with a DNS detail message.

In order to add an integration test for challenge validation that
results in `getAddrs` DNS level errors the Boulder tools image had to be
bumped to a tag that includes the latest `pebble-challtestsrv` that
supports mocking SERVFAILs. It isn't possible to mock this case with
internal IP addresses because our VA test configuration does not filter
internal addresses to support the testing context.

docker-compose.yml

@@ -2,7 +2,7 @@ version: '3'
 services:
     boulder:
         # To minimize fetching this should be the same version used below
-        image: letsencrypt/boulder-tools-go${TRAVIS_GO_VERSION:-1.12.8}:2019-08-13
+        image: letsencrypt/boulder-tools-go${TRAVIS_GO_VERSION:-1.12.8}:2019-08-28
         environment:
             FAKE_DNS: 10.77.77.77
             PKCS11_PROXY_SOCKET: tcp://boulder-hsm:5657
@@ -55,7 +55,7 @@ services:
         working_dir: /go/src/github.com/letsencrypt/boulder
     bhsm:
         # To minimize fetching this should be the same version used above
-        image: letsencrypt/boulder-tools-go${TRAVIS_GO_VERSION:-1.12.8}:2019-08-13
+        image: letsencrypt/boulder-tools-go${TRAVIS_GO_VERSION:-1.12.8}:2019-08-28
         environment:
             PKCS11_DAEMON_SOCKET: tcp://0.0.0.0:5657
         command: /usr/local/bin/pkcs11-daemon /usr/lib/softhsm/libsofthsm2.so
@@ -77,7 +77,7 @@ services:
         logging:
             driver: none
     netaccess:
-        image: letsencrypt/boulder-tools-go${TRAVIS_GO_VERSION:-1.12.8}:2019-08-13
+        image: letsencrypt/boulder-tools-go${TRAVIS_GO_VERSION:-1.12.8}:2019-08-28
         environment:
             GO111MODULE: "on"
             GOFLAGS: "-mod=vendor"

errors/errors.go

@@ -24,6 +24,7 @@ const (
 	MissingSCTs
 	Duplicate
 	OrderNotReady
+	DNS
 )
 
 // BoulderError represents internal Boulder errors
@@ -125,3 +126,7 @@ func DuplicateError(msg string, args ...interface{}) error {
 func OrderNotReadyError(msg string, args ...interface{}) error {
 	return New(OrderNotReady, msg, args...)
 }
+
+func DNSError(msg string, args ...interface{}) error {
+	return New(DNS, msg, args...)
+}

test/challtestsrv.py

@@ -32,6 +32,8 @@ class ChallTestServer:
             "del-txt": "/clear-txt",
             "add-alpn": "/add-tlsalpn01",
             "del-alpn": "/del-tlsalpn01",
+            "add-servfail": "/set-servfail",
+            "del-servfail": "/clear-servfail",
             }
 
     def __init__(self, url=None):
@@ -189,6 +191,25 @@ class ChallTestServer:
                 self._URL("del-http"),
                 { "token": token })
 
+    def add_servfail_response(self, host):
+        """
+        add_servfail_response configures the challenge test server to return
+        SERVFAIL for all queries made for the provided host. This will override
+        any other mocks for the host until removed with remove_servfail_response.
+        """
+        return self._postURL(
+                self._URL("add-servfail"),
+                { "host": host})
+
+    def remove_servfail_response(self, host):
+        """
+        remove_servfail_response undoes the work of add_servfail_response,
+        removing the SERVFAIL configuration for the given host.
+        """
+        return self._postURL(
+                self._URL("del-servfail"),
+                { "host": host})
+
     def add_dns01_response(self, host, value):
         """
         add_dns01_response adds an ACME DNS-01 challenge response for the

test/v2_integration.py

@@ -9,6 +9,7 @@ import datetime
 import time
 import os
 import json
+import re
 
 import OpenSSL
 
@@ -67,6 +68,77 @@ def rand_http_chall(client):
                 return d, c.chall
     raise Exception("No HTTP-01 challenge found for random domain authz")
 
+def check_challenge_dns_err(chalType):
+    """
+    check_challenge_dns_err tests that performing an ACME challenge of the
+    specified type to a hostname that is configured to return SERVFAIL for all
+    queries produces the correct problem type and detail message.
+    """
+    client = chisel2.make_client()
+
+    # Create a random domains.
+    d = random_domain()
+
+    # Configure the chall srv to SERVFAIL all queries for that domain.
+    challSrv.add_servfail_response(d)
+
+    # Expect a DNS problem with a detail that matches a regex
+    expectedProbType = "dns"
+    expectedProbRegex = re.compile(r"DNS problem: SERVFAIL looking up (A|AAAA|CAA) for {0}".format(d))
+
+    # Try and issue for the domain with the given challenge type.
+    failed = False
+    try:
+        chisel2.auth_and_issue([d], client=client, chall_type=chalType)
+    except acme_errors.ValidationError as e:
+        # Mark that the auth_and_issue failed
+        failed = True
+        # Extract the failed challenge from each failed authorization
+        for authzr in e.failed_authzrs:
+            c = None
+            if chalType == "http-01":
+                c = chisel2.get_chall(authzr, challenges.HTTP01)
+            elif chalType == "dns-01":
+                c = chisel2.get_chall(authzr, challenges.DNS01)
+            elif chalType == "tls-alpn-01":
+                c = chisel2.get_chall(authzr, challenges.TLSALPN01)
+            else:
+                raise Exception("Invalid challenge type requested: {0}".format(challType))
+
+            # The failed challenge's error should match expected
+            error = c.error
+            if error is None or error.typ != "urn:ietf:params:acme:error:{0}".format(expectedProbType):
+                raise Exception("Expected {0} prob, got {1}".format(expectedProbType, error.typ))
+            if not expectedProbRegex.match(error.detail):
+                raise Exception("Prob detail did not match expectedProbRegex, got \"{0}\"".format(error.detail))
+    finally:
+        challSrv.remove_servfail_response(d)
+
+    # If there was no exception that means something went wrong. The test should fail.
+    if failed is False:
+        raise Exception("No problem generated issuing for broken DNS identifier")
+
+def test_http_challenge_dns_err():
+    """
+    test_http_challenge_dns_err tests that a HTTP-01 challenge for a domain
+    with broken DNS produces the correct problem response.
+    """
+    check_challenge_dns_err("http-01")
+
+def test_dns_challenge_dns_err():
+    """
+    test_dns_challenge_dns_err tests that a DNS-01 challenge for a domain
+    with broken DNS produces the correct problem response.
+    """
+    check_challenge_dns_err("dns-01")
+
+def test_tls_alpn_challenge_dns_err():
+    """
+    test_tls_alpn_challenge_dns_err tests that a TLS-ALPN-01 challenge for a domain
+    with broken DNS produces the correct problem response.
+    """
+    check_challenge_dns_err("tls-alpn-01")
+
 def test_http_challenge_broken_redirect():
     """
     test_http_challenge_broken_redirect tests that a common webserver

va/dns.go

@@ -9,6 +9,7 @@ import (
 	"net"
 
 	"github.com/letsencrypt/boulder/core"
+	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/identifier"
 	"github.com/letsencrypt/boulder/probs"
 )
@@ -16,16 +17,17 @@ import (
 // getAddr will query for all A/AAAA records associated with hostname and return
 // the preferred address, the first net.IP in the addrs slice, and all addresses
 // resolved. This is the same choice made by the Go internal resolution library
-// used by net/http.
-func (va ValidationAuthorityImpl) getAddrs(ctx context.Context, hostname string) ([]net.IP, *probs.ProblemDetails) {
+// used by net/http. If there is an error resolving the hostname, or if no
+// usable IP addresses are available then a berrors.DNSError instance is
+// returned with a nil net.IP slice.
+func (va ValidationAuthorityImpl) getAddrs(ctx context.Context, hostname string) ([]net.IP, error) {
 	addrs, err := va.dnsClient.LookupHost(ctx, hostname)
 	if err != nil {
-		problem := probs.DNS("%v", err)
-		return nil, problem
+		return nil, berrors.DNSError("%v", err)
 	}
 
 	if len(addrs) == 0 {
-		return nil, probs.UnknownHost("No valid IP addresses found for %s", hostname)
+		return nil, berrors.DNSError("No valid IP addresses found for %s", hostname)
 	}
 	va.log.Debugf("Resolved addresses for %s: %s", hostname, addrs)
 	return addrs, nil

va/http.go

@@ -213,9 +213,7 @@ func (va *ValidationAuthorityImpl) newHTTPValidationTarget(
 	// Resolve IP addresses for the hostname
 	addrs, err := va.getAddrs(ctx, host)
 	if err != nil {
-		// Convert the error into a ConnectionFailureError so it is presented to the
-		// end user in a problem after being fed through detailedError.
-		return nil, berrors.ConnectionFailureError(err.Error())
+		return nil, err
 	}
 
 	target := &httpValidationTarget{

va/http_test.go

@@ -127,7 +127,7 @@ func TestHTTPValidationTarget(t *testing.T) {
 		{
 			Name:          "No IPs for host",
 			Host:          "always.invalid",
-			ExpectedError: berrors.ConnectionFailureError("unknownHost :: No valid IP addresses found for always.invalid"),
+			ExpectedError: berrors.DNSError("No valid IP addresses found for always.invalid"),
 		},
 		{
 			Name:        "Only IPv4 addrs for host",
@@ -668,8 +668,8 @@ func TestFetchHTTP(t *testing.T) {
 			Name: "No IPs for host",
 			Host: "always.invalid",
 			Path: "/.well-known/whatever",
-			ExpectedProblem: probs.ConnectionFailure(
-				"unknownHost :: No valid IP addresses found for always.invalid"),
+			ExpectedProblem: probs.DNS(
+				"No valid IP addresses found for always.invalid"),
 			// There are no validation records in this case because the base record
 			// is only constructed once a URL is made.
 			ExpectedRecords: nil,
@@ -1101,7 +1101,7 @@ func TestHTTP(t *testing.T) {
 	if prob == nil {
 		t.Fatalf("Domain name is invalid.")
 	}
-	test.AssertEquals(t, prob.Type, probs.ConnectionProblem)
+	test.AssertEquals(t, prob.Type, probs.DNSProblem)
 }
 
 func TestHTTPTimeout(t *testing.T) {

va/tlsalpn.go

@@ -67,7 +67,7 @@ func (va *ValidationAuthorityImpl) tryGetTLSCerts(ctx context.Context,
 		},
 	}
 	if problem != nil {
-		return nil, nil, validationRecords, problem
+		return nil, nil, validationRecords, detailedError(problem)
 	}
 	thisRecord := &validationRecords[0]
 

va/tlsalpn_test.go

@@ -294,6 +294,22 @@ func TestTLSError(t *testing.T) {
 	}
 }
 
+func TestDNSError(t *testing.T) {
+	chall := createChallenge(core.ChallengeTypeTLSALPN01)
+	hs := brokenTLSSrv()
+
+	va, _ := setup(hs, 0, "", nil)
+
+	_, prob := va.validateTLSALPN01(ctx, dnsi("always.invalid"), chall)
+	if prob == nil {
+		t.Fatalf("TLS validation should have failed: what IP was used?")
+	}
+	if prob.Type != probs.DNSProblem {
+		t.Errorf("Wrong problem type: got %s, expected type %s",
+			prob, probs.DNSProblem)
+	}
+}
+
 func TestCertNames(t *testing.T) {
 	// We duplicate names inside the SAN set
 	names := []string{

va/va.go

@@ -268,6 +268,9 @@ func detailedError(err error) *probs.ProblemDetails {
 	if berrors.Is(err, berrors.Unauthorized) {
 		return probs.Unauthorized(err.Error())
 	}
+	if berrors.Is(err, berrors.DNS) {
+		return probs.DNS(err.Error())
+	}
 
 	if h2SettingsFrameErrRegex.MatchString(err.Error()) {
 		return probs.ConnectionFailure("Server is speaking HTTP/2 over HTTP")