Skip to content

Commit

Permalink
net: fail fast for DNS rcode success with no answers of requested type
Browse files Browse the repository at this point in the history 
DNS responses which do not contain answers of the requested type return
errNoSuchHost, the same error as rcode name error. Prior to
golang.org/cl/37879, both cases resulted in no additional name servers
being consulted for the question. That CL changed the behavior for both
cases. Issue golang#25336 was filed about the rcode name error case and
golang.org/cl/113815 fixed it. This CL fixes the no answers of requested
type case as well.

Fixes golang#27525

Change-Id: I52fadedcd195f16adf62646b76bea2ab3b15d117
Reviewed-on: https://go-review.googlesource.com/133675
Run-TryBot: Ian Gudger <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
  • Loading branch information
@iangudger @bradfitz
iangudger authored and bradfitz committed Sep 26, 2018
1 parent da0d1a4 commit 94f48dd
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 100 deletions.
115 changes: 70 additions & 45 deletions src/net/dnsclient_unix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,20 @@ import (
"golang_org/x/net/dns/dnsmessage"
)

var (
errLameReferral = errors.New("lame referral")
errCannotUnmarshalDNSMessage = errors.New("cannot unmarshal DNS message")
errCannotMarshalDNSMessage = errors.New("cannot marshal DNS message")
errServerMisbehaving = errors.New("server misbehaving")
errInvalidDNSResponse = errors.New("invalid DNS response")
errNoAnswerFromDNSServer = errors.New("no answer from DNS server")

// errServerTemporarlyMisbehaving is like errServerMisbehaving, except
// that when it gets translated to a DNSError, the IsTemporary field
// gets set to true.
errServerTemporarlyMisbehaving = errors.New("server misbehaving")
)

func newRequest(q dnsmessage.Question) (id uint16, udpReq, tcpReq []byte, err error) {
id = uint16(rand.Int()) ^ uint16(time.Now().UnixNano())
b := dnsmessage.NewBuilder(make([]byte, 2, 514), dnsmessage.Header{ID: id, RecursionDesired: true})
Expand Down Expand Up @@ -105,14 +119,14 @@ func dnsStreamRoundTrip(c Conn, id uint16, query dnsmessage.Question, b []byte)
var p dnsmessage.Parser
h, err := p.Start(b[:n])
if err != nil {
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("cannot unmarshal DNS message")
return dnsmessage.Parser{}, dnsmessage.Header{}, errCannotUnmarshalDNSMessage
}
q, err := p.Question()
if err != nil {
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("cannot unmarshal DNS message")
return dnsmessage.Parser{}, dnsmessage.Header{}, errCannotUnmarshalDNSMessage
}
if !checkResponse(id, query, h, q) {
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("invalid DNS response")
return dnsmessage.Parser{}, dnsmessage.Header{}, errInvalidDNSResponse
}
return p, h, nil
}
Expand All @@ -122,7 +136,7 @@ func (r *Resolver) exchange(ctx context.Context, server string, q dnsmessage.Que
q.Class = dnsmessage.ClassINET
id, udpReq, tcpReq, err := newRequest(q)
if err != nil {
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("cannot marshal DNS message")
return dnsmessage.Parser{}, dnsmessage.Header{}, errCannotMarshalDNSMessage
}
for _, network := range []string{"udp", "tcp"} {
ctx, cancel := context.WithDeadline(ctx, time.Now().Add(timeout))
Expand All @@ -147,31 +161,31 @@ func (r *Resolver) exchange(ctx context.Context, server string, q dnsmessage.Que
return dnsmessage.Parser{}, dnsmessage.Header{}, mapErr(err)
}
if err := p.SkipQuestion(); err != dnsmessage.ErrSectionDone {
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("invalid DNS response")
return dnsmessage.Parser{}, dnsmessage.Header{}, errInvalidDNSResponse
}
if h.Truncated { // see RFC 5966
continue
}
return p, h, nil
}
return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("no answer from DNS server")
return dnsmessage.Parser{}, dnsmessage.Header{}, errNoAnswerFromDNSServer
}

// checkHeader performs basic sanity checks on the header.
func checkHeader(p *dnsmessage.Parser, h dnsmessage.Header, name, server string) error {
if h.RCode == dnsmessage.RCodeNameError {
return errNoSuchHost
}

_, err := p.AnswerHeader()
if err != nil && err != dnsmessage.ErrSectionDone {
return &DNSError{
Err: "cannot unmarshal DNS message",
Name: name,
Server: server,
}
return errCannotUnmarshalDNSMessage
}

// libresolv continues to the next server when it receives
// an invalid referral response. See golang.org/issue/15434.
if h.RCode == dnsmessage.RCodeSuccess && !h.Authoritative && !h.RecursionAvailable && err == dnsmessage.ErrSectionDone {
return &DNSError{Err: "lame referral", Name: name, Server: server}
return errLameReferral
}

if h.RCode != dnsmessage.RCodeSuccess && h.RCode != dnsmessage.RCodeNameError {
Expand All @@ -180,11 +194,10 @@ func checkHeader(p *dnsmessage.Parser, h dnsmessage.Header, name, server string)
// a name error and we didn't get success,
// the server is behaving incorrectly or
// having temporary trouble.
err := &DNSError{Err: "server misbehaving", Name: name, Server: server}
if h.RCode == dnsmessage.RCodeServerFailure {
err.IsTemporary = true
return errServerTemporarlyMisbehaving
}
return err
return errServerMisbehaving
}

return nil
Expand All @@ -194,28 +207,16 @@ func skipToAnswer(p *dnsmessage.Parser, qtype dnsmessage.Type, name, server stri
for {
h, err := p.AnswerHeader()
if err == dnsmessage.ErrSectionDone {
return &DNSError{
Err: errNoSuchHost.Error(),
Name: name,
Server: server,
}
return errNoSuchHost
}
if err != nil {
return &DNSError{
Err: "cannot unmarshal DNS message",
Name: name,
Server: server,
}
return errCannotUnmarshalDNSMessage
}
if h.Type == qtype {
return nil
}
if err := p.SkipAnswer(); err != nil {
return &DNSError{
Err: "cannot unmarshal DNS message",
Name: name,
Server: server,
}
return errCannotUnmarshalDNSMessage
}
}
}
Expand All @@ -229,7 +230,7 @@ func (r *Resolver) tryOneName(ctx context.Context, cfg *dnsConfig, name string,

n, err := dnsmessage.NewName(name)
if err != nil {
return dnsmessage.Parser{}, "", errors.New("cannot marshal DNS message")
return dnsmessage.Parser{}, "", errCannotMarshalDNSMessage
}
q := dnsmessage.Question{
Name: n,
Expand All @@ -243,38 +244,62 @@ func (r *Resolver) tryOneName(ctx context.Context, cfg *dnsConfig, name string,

p, h, err := r.exchange(ctx, server, q, cfg.timeout)
if err != nil {
lastErr = &DNSError{
dnsErr := &DNSError{
Err: err.Error(),
Name: name,
Server: server,
}
if nerr, ok := err.(Error); ok && nerr.Timeout() {
lastErr.(*DNSError).IsTimeout = true
dnsErr.IsTimeout = true
}
// Set IsTemporary for socket-level errors. Note that this flag
// may also be used to indicate a SERVFAIL response.
if _, ok := err.(*OpError); ok {
lastErr.(*DNSError).IsTemporary = true
dnsErr.IsTemporary = true
}
lastErr = dnsErr
continue
}

// The name does not exist, so trying another server won't help.
//
// TODO: indicate this in a more obvious way, such as a field on DNSError?
if h.RCode == dnsmessage.RCodeNameError {
return dnsmessage.Parser{}, "", &DNSError{Err: errNoSuchHost.Error(), Name: name, Server: server}
}

lastErr = checkHeader(&p, h, name, server)
if lastErr != nil {
if err := checkHeader(&p, h, name, server); err != nil {
dnsErr := &DNSError{
Err: err.Error(),
Name: name,
Server: server,
}
if err == errServerTemporarlyMisbehaving {
dnsErr.IsTemporary = true
}
if err == errNoSuchHost {
// The name does not exist, so trying
// another server won't help.
//
// TODO: indicate this in a more
// obvious way, such as a field on
// DNSError?
return p, server, dnsErr
}
lastErr = dnsErr
continue
}

lastErr = skipToAnswer(&p, qtype, name, server)
if lastErr == nil {
err = skipToAnswer(&p, qtype, name, server)
if err == nil {
return p, server, nil
}
lastErr = &DNSError{
Err: err.Error(),
Name: name,
Server: server,
}
if err == errNoSuchHost {
// The name does not exist, so trying another
// server won't help.
//
// TODO: indicate this in a more obvious way,
// such as a field on DNSError?
return p, server, lastErr
}
}
}
return dnsmessage.Parser{}, "", lastErr
Expand Down
143 changes: 88 additions & 55 deletions src/net/dnsclient_unix_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1427,28 +1427,35 @@ func TestDNSGoroutineRace(t *testing.T) {
}
}

func lookupWithFake(fake fakeDNSServer, name string, typ dnsmessage.Type) error {
r := Resolver{PreferGo: true, Dial: fake.DialContext}

resolvConf.mu.RLock()
conf := resolvConf.dnsConfig
resolvConf.mu.RUnlock()

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

_, _, err := r.tryOneName(ctx, conf, name, typ)
return err
}

// Issue 8434: verify that Temporary returns true on an error when rcode
// is SERVFAIL
func TestIssue8434(t *testing.T) {
msg := dnsmessage.Message{
Header: dnsmessage.Header{
RCode: dnsmessage.RCodeServerFailure,
err := lookupWithFake(fakeDNSServer{
rh: func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
return dnsmessage.Message{
Header: dnsmessage.Header{
ID: q.ID,
Response: true,
RCode: dnsmessage.RCodeServerFailure,
},
Questions: q.Questions,
}, nil
},
}
b, err := msg.Pack()
if err != nil {
t.Fatal("Pack failed:", err)
}
var p dnsmessage.Parser
h, err := p.Start(b)
if err != nil {
t.Fatal("Start failed:", err)
}
if err := p.SkipAllQuestions(); err != nil {
t.Fatal("SkipAllQuestions failed:", err)
}

err = checkHeader(&p, h, "golang.org", "foo:53")
}, "golang.org.", dnsmessage.TypeALL)
if err == nil {
t.Fatal("expected an error")
}
Expand All @@ -1464,50 +1471,76 @@ func TestIssue8434(t *testing.T) {
}
}

// Issue 12778: verify that NXDOMAIN without RA bit errors as
// "no such host" and not "server misbehaving"
// TestNoSuchHost verifies that tryOneName works correctly when the domain does
// not exist.
//
// Issue 12778: verify that NXDOMAIN without RA bit errors as "no such host"
// and not "server misbehaving"
//
// Issue 25336: verify that NXDOMAIN errors fail fast.
func TestIssue12778(t *testing.T) {
lookups := 0
fake := fakeDNSServer{
rh: func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
lookups++
return dnsmessage.Message{
Header: dnsmessage.Header{
ID: q.ID,
Response: true,
RCode: dnsmessage.RCodeNameError,
RecursionAvailable: false,
},
Questions: q.Questions,
}, nil
//
// Issue 27525: verify that empty answers fail fast.
func TestNoSuchHost(t *testing.T) {
tests := []struct {
name string
f func(string, string, dnsmessage.Message, time.Time) (dnsmessage.Message, error)
}{
{
"NXDOMAIN",
func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
return dnsmessage.Message{
Header: dnsmessage.Header{
ID: q.ID,
Response: true,
RCode: dnsmessage.RCodeNameError,
RecursionAvailable: false,
},
Questions: q.Questions,
}, nil
},
},
{
"no answers",
func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) {
return dnsmessage.Message{
Header: dnsmessage.Header{
ID: q.ID,
Response: true,
RCode: dnsmessage.RCodeSuccess,
RecursionAvailable: false,
Authoritative: true,
},
Questions: q.Questions,
}, nil
},
},
}
r := Resolver{PreferGo: true, Dial: fake.DialContext}

resolvConf.mu.RLock()
conf := resolvConf.dnsConfig
resolvConf.mu.RUnlock()

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

_, _, err := r.tryOneName(ctx, conf, ".", dnsmessage.TypeALL)
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
lookups := 0
err := lookupWithFake(fakeDNSServer{
rh: func(n, s string, q dnsmessage.Message, d time.Time) (dnsmessage.Message, error) {
lookups++
return test.f(n, s, q, d)
},
}, ".", dnsmessage.TypeALL)

if lookups != 1 {
t.Errorf("got %d lookups, wanted 1", lookups)
}
if lookups != 1 {
t.Errorf("got %d lookups, wanted 1", lookups)
}

if err == nil {
t.Fatal("expected an error")
}
de, ok := err.(*DNSError)
if !ok {
t.Fatalf("err = %#v; wanted a *net.DNSError", err)
}
if de.Err != errNoSuchHost.Error() {
t.Fatalf("Err = %#v; wanted %q", de.Err, errNoSuchHost.Error())
if err == nil {
t.Fatal("expected an error")
}
de, ok := err.(*DNSError)
if !ok {
t.Fatalf("err = %#v; wanted a *net.DNSError", err)
}
if de.Err != errNoSuchHost.Error() {
t.Fatalf("Err = %#v; wanted %q", de.Err, errNoSuchHost.Error())
}
})
}
}

Expand Down

0 comments on commit 94f48dd

Please sign in to comment.