letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Improve readability of A and AAAA lookup errors (#5843) 

When we query DNS for a host, and both the A and AAAA lookups fail or
are empty, combine both errors into a single error rather than only
returning the error from the A lookup.

Fixes #5819
Fixes #5319
This commit is contained in:
Aaron Gable 2022-01-03 10:39:25 -08:00 committed by GitHub
parent ad75295f51
commit 2f2bac4bf2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 53 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
bdns/dns.go
View File

@ -459,10 +459,9 @@ func (dnsClient *impl) lookupIP(ctx context.Context, hostname string, ipType uin
// LookupHost sends a DNS query to find all A and AAAA records associated with
// the provided hostname. This method assumes that the external resolver will
// chase CNAME/DNAME aliases and return relevant records. It will retry
// requests in the case of temporary network errors. It can return net package,
// context.Canceled, and context.DeadlineExceeded errors, all wrapped in the
// DNSError type.
// chase CNAME/DNAME aliases and return relevant records. It will retry
// requests in the case of temporary network errors. It returns an error if
// both the A and AAAA lookups fail or are empty, but succeeds otherwise.
func (dnsClient *impl) LookupHost(ctx context.Context, hostname string) ([]net.IP, error) {
var recordsA, recordsAAAA []dns.RR
var errA, errAAAA error
@ -480,28 +479,46 @@ func (dnsClient *impl) LookupHost(ctx context.Context, hostname string) ([]net.I
}()
wg.Wait()
var addrsA []net.IP
if errA == nil {
for _, answer := range recordsA {
if answer.Header().Rrtype == dns.TypeA {
a, ok := answer.(*dns.A)
if ok && a.A.To4() != nil && (!isPrivateV4(a.A) || dnsClient.allowRestrictedAddresses) {
addrsA = append(addrsA, a.A)
}
}
}
if len(addrsA) == 0 {
errA = fmt.Errorf("no valid A records found for %s", hostname)
}
}
var addrsAAAA []net.IP
if errAAAA == nil {
for _, answer := range recordsAAAA {
if answer.Header().Rrtype == dns.TypeAAAA {
aaaa, ok := answer.(*dns.AAAA)
if ok && aaaa.AAAA.To16() != nil && (!isPrivateV6(aaaa.AAAA) || dnsClient.allowRestrictedAddresses) {
addrsAAAA = append(addrsAAAA, aaaa.AAAA)
}
}
}
if len(addrsAAAA) == 0 {
errAAAA = fmt.Errorf("no valid AAAA records found for %s", hostname)
}
}
if errA != nil && errAAAA != nil {
return nil, errA
// Construct a new error from both underlying errors. We can only use %w for
// one of them, because the go error unwrapping protocol doesn't support
// branching. We don't use ProblemDetails and SubProblemDetails here, because
// this error will get wrapped in a DNSError and further munged by higher
// layers in the stack.
return nil, fmt.Errorf("%w; %s", errA, errAAAA)
}
var addrs []net.IP
for _, answer := range recordsA {
if answer.Header().Rrtype == dns.TypeA {
if a, ok := answer.(*dns.A); ok && a.A.To4() != nil && (!isPrivateV4(a.A) || dnsClient.allowRestrictedAddresses) {
addrs = append(addrs, a.A)
}
}
}
for _, answer := range recordsAAAA {
if answer.Header().Rrtype == dns.TypeAAAA {
if aaaa, ok := answer.(*dns.AAAA); ok && aaaa.AAAA.To16() != nil && (!isPrivateV6(aaaa.AAAA) || dnsClient.allowRestrictedAddresses) {
addrs = append(addrs, aaaa.AAAA)
}
}
}
return addrs, nil
return append(addrsA, addrsAAAA...), nil
}
// LookupCAA sends a DNS query to find all CAA records associated with

18
bdns/dns_test.go
View File

@ -264,7 +264,7 @@ func TestDNSOneServer(t *testing.T) {
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
_, err = obj.LookupHost(context.Background(), "letsencrypt.org")
_, err = obj.LookupHost(context.Background(), "cps.letsencrypt.org")
test.AssertNotError(t, err, "No message")
}
@ -275,7 +275,7 @@ func TestDNSDuplicateServers(t *testing.T) {
obj := NewTest(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
_, err = obj.LookupHost(context.Background(), "letsencrypt.org")
_, err = obj.LookupHost(context.Background(), "cps.letsencrypt.org")
test.AssertNotError(t, err, "No message")
}
@ -328,7 +328,7 @@ func TestDNSLookupHost(t *testing.T) {
ip, err = obj.LookupHost(context.Background(), "nonexistent.letsencrypt.org")
t.Logf("nonexistent.letsencrypt.org - IP: %s, Err: %s", ip, err)
test.AssertNotError(t, err, "Not an error to not exist")
test.AssertError(t, err, "No valid A or AAAA records should error")
test.Assert(t, len(ip) == 0, "Should not have IPs")
// Single IPv4 address
@ -374,13 +374,13 @@ func TestDNSLookupHost(t *testing.T) {
test.Assert(t, ip[0].To16().Equal(expected), "wrong ipv6 address")
// IPv6 error, IPv4 error
// Should return the IPv4 error (Refused) and not IPv6 error (NotImplemented)
// Should return both the IPv4 error (Refused) and the IPv6 error (NotImplemented)
hostname := "dualstackerror.letsencrypt.org"
ip, err = obj.LookupHost(context.Background(), hostname)
t.Logf("%s - IP: %s, Err: %s", hostname, ip, err)
test.AssertError(t, err, "Should be an error")
expectedErr := &Error{dns.TypeA, hostname, nil, dns.RcodeRefused}
test.AssertDeepEquals(t, err, expectedErr)
test.AssertContains(t, err.Error(), "REFUSED looking up A for")
test.AssertContains(t, err.Error(), "NOTIMP looking up AAAA for")
}
func TestDNSNXDOMAIN(t *testing.T) {
@ -391,11 +391,11 @@ func TestDNSNXDOMAIN(t *testing.T) {
hostname := "nxdomain.letsencrypt.org"
_, err = obj.LookupHost(context.Background(), hostname)
expected := &Error{dns.TypeA, hostname, nil, dns.RcodeNameError}
test.AssertDeepEquals(t, err, expected)
test.AssertContains(t, err.Error(), "NXDOMAIN looking up A for")
test.AssertContains(t, err.Error(), "NXDOMAIN looking up AAAA for")
_, err = obj.LookupTXT(context.Background(), hostname)
expected.recordType = dns.TypeTXT
expected := &Error{dns.TypeTXT, hostname, nil, dns.RcodeNameError}
test.AssertDeepEquals(t, err, expected)
}

4
test/v2_integration.py
View File

@ -84,7 +84,7 @@ def check_challenge_dns_err(chalType):
# Expect a DNS problem with a detail that matches a regex
expectedProbType = "dns"
expectedProbRegex = re.compile(r"DNS problem: SERVFAIL looking up (A|AAAA|TXT|CAA) for {0}".format(d))
expectedProbRegex = re.compile(r"SERVFAIL looking up (A|AAAA|TXT|CAA) for {0}".format(d))
# Try and issue for the domain with the given challenge type.
failed = False
@ -109,7 +109,7 @@ def check_challenge_dns_err(chalType):
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):
if not expectedProbRegex.search(error.detail):
raise(Exception("Prob detail did not match expectedProbRegex, got \"{0}\"".format(error.detail)))
finally:
challSrv.remove_servfail_response(d)

2
va/dns.go
View File

@ -27,6 +27,8 @@ func (va ValidationAuthorityImpl) getAddrs(ctx context.Context, hostname string)
}
if len(addrs) == 0 {
// This should be unreachable, as no valid IP addresses being found results
// in an error being returned from LookupHost.
return nil, berrors.DNSError("No valid IP addresses found for %s", hostname)
}
va.log.Debugf("Resolved addresses for %s: %s", hostname, addrs)