From 92f16893102a02a02b1795b4630cfe6a23835926 Mon Sep 17 00:00:00 2001 From: Jeff Hodges Date: Mon, 28 Dec 2015 13:09:32 -0800 Subject: [PATCH] make DNS ProblemDetails more clear Fixes #1259 --- bdns/problem.go | 26 +++++++++++++++----------- bdns/problem_test.go | 5 +++-- ra/registration-authority.go | 11 +++++------ ra/registration-authority_test.go | 4 ++-- va/validation-authority.go | 21 +++++++++++---------- va/validation-authority_test.go | 4 ++-- 6 files changed, 38 insertions(+), 33 deletions(-) diff --git a/bdns/problem.go b/bdns/problem.go index 4a54ec674..b257325ec 100644 --- a/bdns/problem.go +++ b/bdns/problem.go @@ -6,6 +6,7 @@ package bdns import ( + "fmt" "net" "github.com/letsencrypt/boulder/probs" @@ -15,20 +16,23 @@ const detailDNSTimeout = "DNS query timed out" const detailDNSNetFailure = "DNS networking error" const detailServerFailure = "Server failure at resolver" -// ProblemDetailsFromDNSError checks the error returned from Lookup... -// methods and tests if the error was an underlying net.OpError or an error -// caused by resolver returning SERVFAIL or other invalid Rcodes and returns -// the relevant core.ProblemDetails. -func ProblemDetailsFromDNSError(err error) *probs.ProblemDetails { - problem := &probs.ProblemDetails{Type: probs.ConnectionProblem} +// ProblemDetailsFromDNSError checks the error returned from Lookup... methods +// and tests if the error was an underlying net.OpError or an error caused by +// resolver returning SERVFAIL or other invalid Rcodes and returns the relevant +// core.ProblemDetails. The detail string will contain a mention of the DNS +// record type and domain given. +func ProblemDetailsFromDNSError(recordType, domain string, err error) *probs.ProblemDetails { + detail := detailServerFailure if netErr, ok := err.(*net.OpError); ok { if netErr.Timeout() { - problem.Detail = detailDNSTimeout + detail = detailDNSTimeout } else { - problem.Detail = detailDNSNetFailure + detail = detailDNSNetFailure } - } else { - problem.Detail = detailServerFailure } - return problem + detail = fmt.Sprintf("%s during %s-record lookup of %s", detail, recordType, domain) + return &probs.ProblemDetails{ + Type: probs.ConnectionProblem, + Detail: detail, + } } diff --git a/bdns/problem_test.go b/bdns/problem_test.go index bc9122bf1..f2ef0b3ef 100644 --- a/bdns/problem_test.go +++ b/bdns/problem_test.go @@ -31,11 +31,12 @@ func TestProblemDetailsFromDNSError(t *testing.T) { }, } for _, tc := range testCases { - err := ProblemDetailsFromDNSError(tc.err) + err := ProblemDetailsFromDNSError("TXT", "example.com", tc.err) if err.Type != probs.ConnectionProblem { t.Errorf("ProblemDetailsFromDNSError(%q).Type = %q, expected %q", tc.err, err.Type, probs.ConnectionProblem) } - if err.Detail != tc.expected { + exp := tc.expected + " during TXT-record lookup of example.com" + if err.Detail != exp { t.Errorf("ProblemDetailsFromDNSError(%q).Detail = %q, expected %q", tc.err, err.Detail, tc.expected) } } diff --git a/ra/registration-authority.go b/ra/registration-authority.go index 872e2fba5..628e54128 100644 --- a/ra/registration-authority.go +++ b/ra/registration-authority.go @@ -97,9 +97,10 @@ func validateEmail(address string, resolver bdns.DNSResolver) (prob *probs.Probl var resultMX []string var resultA []net.IP resultMX, err = resolver.LookupMX(domain) - + recQ := "MX" if err == nil && len(resultMX) == 0 { resultA, err = resolver.LookupHost(domain) + recQ = "A" if err == nil && len(resultA) == 0 { return &probs.ProblemDetails{ Type: probs.InvalidEmailProblem, @@ -108,11 +109,9 @@ func validateEmail(address string, resolver bdns.DNSResolver) (prob *probs.Probl } } if err != nil { - dnsProblem := bdns.ProblemDetailsFromDNSError(err) - return &probs.ProblemDetails{ - Type: probs.InvalidEmailProblem, - Detail: dnsProblem.Detail, - } + prob := bdns.ProblemDetailsFromDNSError(recQ, domain, err) + prob.Type = probs.InvalidEmailProblem + return prob } return nil diff --git a/ra/registration-authority_test.go b/ra/registration-authority_test.go index be00653e2..f4e14d62b 100644 --- a/ra/registration-authority_test.go +++ b/ra/registration-authority_test.go @@ -317,8 +317,8 @@ func TestValidateEmail(t *testing.T) { }{ {"an email`", unparseableEmailDetail}, {"a@always.invalid", emptyDNSResponseDetail}, - {"a@always.timeout", "DNS query timed out"}, - {"a@always.error", "DNS networking error"}, + {"a@always.timeout", "DNS query timed out during A-record lookup of always.timeout"}, + {"a@always.error", "DNS networking error during A-record lookup of always.error"}, } testSuccesses := []string{ "a@email.com", diff --git a/va/validation-authority.go b/va/validation-authority.go index 241c2224f..9615f4380 100644 --- a/va/validation-authority.go +++ b/va/validation-authority.go @@ -88,24 +88,24 @@ type verificationRequestEvent struct { // This is the same choice made by the Go internal resolution library used by // net/http, except we only send A queries and accept IPv4 addresses. // TODO(#593): Add IPv6 support -func (va ValidationAuthorityImpl) getAddr(hostname string) (addr net.IP, addrs []net.IP, problem *probs.ProblemDetails) { +func (va ValidationAuthorityImpl) getAddr(hostname string) (net.IP, []net.IP, *probs.ProblemDetails) { addrs, err := va.DNSResolver.LookupHost(hostname) if err != nil { - problem = bdns.ProblemDetailsFromDNSError(err) va.log.Debug(fmt.Sprintf("%s DNS failure: %s", hostname, err)) - return + problem := bdns.ProblemDetailsFromDNSError("A", hostname, err) + return net.IP{}, nil, problem } if len(addrs) == 0 { - problem = &probs.ProblemDetails{ + problem := &probs.ProblemDetails{ Type: probs.UnknownHostProblem, Detail: fmt.Sprintf("No IPv4 addresses found for %s", hostname), } - return + return net.IP{}, nil, problem } - addr = addrs[0] + addr := addrs[0] va.log.Info(fmt.Sprintf("Resolved addresses for %s [using %s]: %s", hostname, addr, addrs)) - return + return addr, addrs, nil } type dialer struct { @@ -433,7 +433,8 @@ func (va *ValidationAuthorityImpl) validateDNS01(identifier core.AcmeIdentifier, if err != nil { va.log.Debug(fmt.Sprintf("%s [%s] DNS failure: %s", challenge.Type, identifier, err)) - return nil, bdns.ProblemDetailsFromDNSError(err) + + return nil, bdns.ProblemDetailsFromDNSError("TXT", challengeSubdomain, err) } for _, element := range txts { @@ -454,14 +455,14 @@ func (va *ValidationAuthorityImpl) checkCAA(identifier core.AcmeIdentifier, regI present, valid, err := va.CheckCAARecords(identifier) if err != nil { va.log.Warning(fmt.Sprintf("Problem checking CAA: %s", err)) - return bdns.ProblemDetailsFromDNSError(err) + return bdns.ProblemDetailsFromDNSError("CAA", identifier.Value, err) } // AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c va.log.Audit(fmt.Sprintf("Checked CAA records for %s, registration ID %d [Present: %t, Valid for issuance: %t]", identifier.Value, regID, present, valid)) if !valid { return &probs.ProblemDetails{ Type: probs.ConnectionProblem, - Detail: "CAA check for identifier failed", + Detail: fmt.Sprintf("CAA check for %s failed", identifier.Value), } } return nil diff --git a/va/validation-authority_test.go b/va/validation-authority_test.go index 0ece9608b..8b2f44f62 100644 --- a/va/validation-authority_test.go +++ b/va/validation-authority_test.go @@ -655,9 +655,9 @@ func TestCAATimeout(t *testing.T) { if err.Type != probs.ConnectionProblem { t.Errorf("Expected timeout error type %s, got %s", probs.ConnectionProblem, err.Type) } - expected := "DNS query timed out" + expected := "DNS query timed out during CAA-record lookup of caa-timeout.com" if err.Detail != expected { - t.Errorf("checkCAA: got %s, expected %s", err.Detail, expected) + t.Errorf("checkCAA: got %#v, expected %#v", err.Detail, expected) } }