From 6aa48a9a0b8a7789c5ee4019d3eb50089c611f33 Mon Sep 17 00:00:00 2001 From: Mikio Hara Date: Tue, 9 Jun 2015 17:30:00 +0900 Subject: [PATCH] net: don't return DNS query results including the second best records unconditionally This change prevents DNS query results using domain search list overtaking results not using the list unconditionally, which only happens when using builtin DNS stub resolver. The previous internal lookup function lookup is split into lookup and goLookupIPOrder for iteration over a set of names: FQDN or absolute FQDN, with domain label suffixes in search list, without domain label suffixes, and for concurrent A and AAAA record queries. Fixes #11081. Change-Id: I9ff0640f69276e372d97e709b149ed5b153e8601 Reviewed-on: https://go-review.googlesource.com/10836 Reviewed-by: Russ Cox --- src/net/dnsclient.go | 2 +- src/net/dnsclient_unix.go | 125 ++++++++++++++------------ src/net/dnsclient_unix_test.go | 154 +++++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+), 57 deletions(-) diff --git a/src/net/dnsclient.go b/src/net/dnsclient.go index e5d0ae039b..ce48521bc6 100644 --- a/src/net/dnsclient.go +++ b/src/net/dnsclient.go @@ -41,7 +41,7 @@ func answer(name, server string, dns *dnsMsg, qtype uint16) (cname string, addrs addrs = make([]dnsRR, 0, len(dns.answer)) if dns.rcode == dnsRcodeNameError && dns.recursion_available { - return "", nil, &DNSError{Err: errNoSuchHost.Error(), Name: name} + return "", nil, &DNSError{Err: errNoSuchHost.Error(), Name: name, Server: server} } if dns.rcode != dnsRcodeSuccess { // None of the error codes make sense diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go index 6b775f713e..c03c1b1159 100644 --- a/src/net/dnsclient_unix.go +++ b/src/net/dnsclient_unix.go @@ -185,9 +185,9 @@ func tryOneName(cfg *dnsConfig, name string, qtype uint16) (string, []dnsRR, err } continue } - cname, addrs, err := answer(name, server, msg, qtype) - if err == nil || err.(*DNSError).Err == errNoSuchHost.Error() { - return cname, addrs, err + cname, rrs, err := answer(name, server, msg, qtype) + if err == nil || msg.rcode == dnsRcodeSuccess || msg.rcode == dnsRcodeNameError && msg.recursion_available { + return cname, rrs, err } lastErr = err } @@ -299,59 +299,55 @@ func (conf *resolverConfig) releaseSema() { func lookup(name string, qtype uint16) (cname string, rrs []dnsRR, err error) { if !isDomainName(name) { - return name, nil, &DNSError{Err: "invalid domain name", Name: name} + return "", nil, &DNSError{Err: "invalid domain name", Name: name} } - resolvConf.tryUpdate("/etc/resolv.conf") resolvConf.mu.RLock() - resolv := resolvConf.dnsConfig + conf := resolvConf.dnsConfig resolvConf.mu.RUnlock() - - // If name is rooted (trailing dot) or has enough dots, - // try it by itself first. - rooted := len(name) > 0 && name[len(name)-1] == '.' - if rooted || count(name, '.') >= resolv.ndots { - rname := name - if !rooted { - rname += "." - } - // Can try as ordinary name. - cname, rrs, err = tryOneName(resolv, rname, qtype) - if rooted || err == nil { - return - } - } - - // Otherwise, try suffixes. - for _, suffix := range resolv.search { - rname := name + "." + suffix - if rname[len(rname)-1] != '.' { - rname += "." - } - cname, rrs, err = tryOneName(resolv, rname, qtype) + for _, fqdn := range conf.nameList(name) { + cname, rrs, err = tryOneName(conf, fqdn, qtype) if err == nil { - return + break } } - - // Last ditch effort: try unsuffixed only if we haven't already, - // that is, name is not rooted and has less than ndots dots. - if count(name, '.') < resolv.ndots { - cname, rrs, err = tryOneName(resolv, name+".", qtype) - if err == nil { - return - } - } - - if e, ok := err.(*DNSError); ok { + if err, ok := err.(*DNSError); ok { // Show original name passed to lookup, not suffixed one. // In general we might have tried many suffixes; showing // just one is misleading. See also golang.org/issue/6324. - e.Name = name + err.Name = name } return } +// nameList returns a list of names for sequential DNS queries. +func (conf *dnsConfig) nameList(name string) []string { + // If name is rooted (trailing dot), try only that name. + rooted := len(name) > 0 && name[len(name)-1] == '.' + if rooted { + return []string{name} + } + // Build list of search choices. + names := make([]string, 0, 1+len(conf.search)) + // If name has enough dots, try unsuffixed first. + if count(name, '.') >= conf.ndots { + names = append(names, name+".") + } + // Try suffixes. + for _, suffix := range conf.search { + suffixed := name + "." + suffix + if suffixed[len(suffixed)-1] != '.' { + suffixed += "." + } + names = append(names, suffixed) + } + // Try unsuffixed, if not tried first above. + if count(name, '.') < conf.ndots { + names = append(names, name+".") + } + return names +} + // hostLookupOrder specifies the order of LookupHost lookup strategies. // It is basically a simplified representation of nsswitch.conf. // "files" means /etc/hosts. @@ -436,27 +432,44 @@ func goLookupIPOrder(name string, order hostLookupOrder) (addrs []IPAddr, err er return addrs, nil } } + if !isDomainName(name) { + return nil, &DNSError{Err: "invalid domain name", Name: name} + } + resolvConf.tryUpdate("/etc/resolv.conf") + resolvConf.mu.RLock() + conf := resolvConf.dnsConfig + resolvConf.mu.RUnlock() type racer struct { - qtype uint16 - rrs []dnsRR + rrs []dnsRR error } lane := make(chan racer, 1) qtypes := [...]uint16{dnsTypeA, dnsTypeAAAA} - for _, qtype := range qtypes { - go func(qtype uint16) { - _, rrs, err := lookup(name, qtype) - lane <- racer{qtype, rrs, err} - }(qtype) - } var lastErr error - for range qtypes { - racer := <-lane - if racer.error != nil { - lastErr = racer.error - continue + for _, fqdn := range conf.nameList(name) { + for _, qtype := range qtypes { + go func(qtype uint16) { + _, rrs, err := tryOneName(conf, fqdn, qtype) + lane <- racer{rrs, err} + }(qtype) + } + for range qtypes { + racer := <-lane + if racer.error != nil { + lastErr = racer.error + continue + } + addrs = append(addrs, addrRecordList(racer.rrs)...) + } + if len(addrs) > 0 { + break } - addrs = append(addrs, addrRecordList(racer.rrs)...) + } + if lastErr, ok := lastErr.(*DNSError); ok { + // Show original name passed to lookup, not suffixed one. + // In general we might have tried many suffixes; showing + // just one is misleading. See also golang.org/issue/6324. + lastErr.Name = name } sortByRFC6724(addrs) if len(addrs) == 0 { diff --git a/src/net/dnsclient_unix_test.go b/src/net/dnsclient_unix_test.go index c6bfc67abc..a999f8f060 100644 --- a/src/net/dnsclient_unix_test.go +++ b/src/net/dnsclient_unix_test.go @@ -224,6 +224,160 @@ func TestUpdateResolvConf(t *testing.T) { } } +var goLookupIPWithResolverConfigTests = []struct { + name string + lines []string // resolver configuration lines + error + a, aaaa bool // whether response contains A, AAAA-record +}{ + // no records, transport timeout + { + "jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j", + []string{ + "options timeout:1 attempts:1", + "nameserver 255.255.255.255", // please forgive us for abuse of limited broadcast address + }, + &DNSError{Name: "jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j", Server: "255.255.255.255:53", IsTimeout: true}, + false, false, + }, + + // no records, non-existent domain + { + "jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j", + []string{ + "options timeout:3 attempts:1", + "nameserver 8.8.8.8", + }, + &DNSError{Name: "jgahvsekduiv9bw4b3qhn4ykdfgj0493iohkrjfhdvhjiu4j", Server: "8.8.8.8:53", IsTimeout: false}, + false, false, + }, + + // a few A records, no AAAA records + { + "ipv4.google.com.", + []string{ + "nameserver 8.8.8.8", + "nameserver 2001:4860:4860::8888", + }, + nil, + true, false, + }, + { + "ipv4.google.com", + []string{ + "domain golang.org", + "nameserver 2001:4860:4860::8888", + "nameserver 8.8.8.8", + }, + nil, + true, false, + }, + { + "ipv4.google.com", + []string{ + "search x.golang.org y.golang.org", + "nameserver 2001:4860:4860::8888", + "nameserver 8.8.8.8", + }, + nil, + true, false, + }, + + // no A records, a few AAAA records + { + "ipv6.google.com.", + []string{ + "nameserver 2001:4860:4860::8888", + "nameserver 8.8.8.8", + }, + nil, + false, true, + }, + { + "ipv6.google.com", + []string{ + "domain golang.org", + "nameserver 8.8.8.8", + "nameserver 2001:4860:4860::8888", + }, + nil, + false, true, + }, + { + "ipv6.google.com", + []string{ + "search x.golang.org y.golang.org", + "nameserver 8.8.8.8", + "nameserver 2001:4860:4860::8888", + }, + nil, + false, true, + }, + + // both A and AAAA records + { + "hostname.as112.net", // see RFC 7534 + []string{ + "domain golang.org", + "nameserver 2001:4860:4860::8888", + "nameserver 8.8.8.8", + }, + nil, + true, true, + }, + { + "hostname.as112.net", // see RFC 7534 + []string{ + "search x.golang.org y.golang.org", + "nameserver 2001:4860:4860::8888", + "nameserver 8.8.8.8", + }, + nil, + true, true, + }, +} + +func TestGoLookupIPWithResolverConfig(t *testing.T) { + if testing.Short() || !*testExternal { + t.Skip("avoid external network") + } + + conf, err := newResolvConfTest() + if err != nil { + t.Fatal(err) + } + defer conf.teardown() + + for _, tt := range goLookupIPWithResolverConfigTests { + if err := conf.writeAndUpdate(tt.lines); err != nil { + t.Error(err) + continue + } + conf.tryUpdate(conf.path) + addrs, err := goLookupIP(tt.name) + if err != nil { + if err, ok := err.(*DNSError); !ok || (err.Name != tt.error.(*DNSError).Name || err.Server != tt.error.(*DNSError).Server || err.IsTimeout != tt.error.(*DNSError).IsTimeout) { + t.Errorf("got %v; want %v", err, tt.error) + } + continue + } + if len(addrs) == 0 { + t.Errorf("no records for %s", tt.name) + } + if !tt.a && !tt.aaaa && len(addrs) > 0 { + t.Errorf("unexpected %v for %s", addrs, tt.name) + } + for _, addr := range addrs { + if !tt.a && addr.IP.To4() != nil { + t.Errorf("got %v; must not be IPv4 address", addr) + } + if !tt.aaaa && addr.IP.To16() != nil && addr.IP.To4() == nil { + t.Errorf("got %v; must not be IPv6 address", addr) + } + } + } +} + func BenchmarkGoLookupIP(b *testing.B) { testHookUninstaller.Do(uninstallTestHooks) -- 2.50.0