]> Cypherpunks repositories - gostls13.git/commitdiff
net: fail fast for DNS rcode success with no answers of requested type
authorIan Gudger <igudger@google.com>
Thu, 6 Sep 2018 06:53:36 +0000 (23:53 -0700)
committerBrad Fitzpatrick <bradfitz@golang.org>
Wed, 26 Sep 2018 23:09:55 +0000 (23:09 +0000)
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 #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 #27525

Change-Id: I52fadedcd195f16adf62646b76bea2ab3b15d117
Reviewed-on: https://go-review.googlesource.com/133675
Run-TryBot: Ian Gudger <igudger@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
src/net/dnsclient_unix.go
src/net/dnsclient_unix_test.go

index 2fee3346e92dcffea35a604cddfbd41644d0fbef..9a0b1d69a81b109c308cd24314e630284cac3f09 100644 (file)
@@ -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})
@@ -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
 }
@@ -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))
@@ -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 {
@@ -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
@@ -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
                }
        }
 }
@@ -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,
@@ -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
index bb014b903ab9335a1d205a94a7813ff93b9cca3d..9e4ebcc7bbef890b4761dff9078e7f0bb29dd7d4 100644 (file)
@@ -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")
        }
@@ -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())
+                       }
+               })
        }
 }