Merge branch 'master' into 1453-stats-tests

CHANGELOG.md

@@ -28,7 +28,13 @@ NOTE: Add new changes BELOW THIS COMMENT.
 - Two new metrics showing total number of responses from each upstream DNS
   server and their average processing time in the Web UI ([#1453]).
 
+### Fixed
+
+- Client hostnames not resolving when upstream server responds with zero-TTL
+  records ([#6046]).
+
 [#1453]: https://github.com/AdguardTeam/AdGuardHome/issues/1453
+[#6046]: https://github.com/AdguardTeam/AdGuardHome/issues/6046
 
 <!--
 NOTE: Add new changes ABOVE THIS COMMENT.

internal/dnsforward/dnsforward.go

@@ -346,19 +346,21 @@ func (s *Server) Exchange(ip netip.Addr) (host string, ttl time.Duration, err er
 	}
 
 	var resolver *proxy.Proxy
+	var errMsg string
 	if s.privateNets.Contains(ip.AsSlice()) {
 		if !s.conf.UsePrivateRDNS {
 			return "", 0, nil
 		}
 
 		resolver = s.localResolvers
+		errMsg = "resolving a private address: %w"
 		s.recDetector.add(*req)
 	} else {
 		resolver = s.internalProxy
+		errMsg = "resolving an address: %w"
 	}
-
 	if err = resolver.Resolve(dctx); err != nil {
-		return "", 0, err
+		return "", 0, fmt.Errorf(errMsg, err)
 	}
 
 	return hostFromPTR(dctx.Res)
@@ -377,13 +379,18 @@ func hostFromPTR(resp *dns.Msg) (host string, ttl time.Duration, err error) {
 
 	var ttlSec uint32
 
+	log.Debug("dnsforward: resolving ptr, received %d answers", len(resp.Answer))
 	for _, ans := range resp.Answer {
 		ptr, ok := ans.(*dns.PTR)
 		if !ok {
 			continue
 		}
 
-		if ptr.Hdr.Ttl > ttlSec {
+		// Respect zero TTL records since some DNS servers use it to
+		// locally-resolved addresses.
+		//
+		// See https://github.com/AdguardTeam/AdGuardHome/issues/6046.
+		if ptr.Hdr.Ttl >= ttlSec {
 			host = ptr.Ptr
 			ttlSec = ptr.Hdr.Ttl
 		}
@@ -465,6 +472,7 @@ func (s *Server) filterOurDNSAddrs(addrs []string) (filtered []string, err error
 	}
 
 	ourAddrsSet := stringutil.NewSet(ourAddrs...)
+	log.Debug("dnsforward: filtering out %s", ourAddrsSet.String())
 
 	// TODO(e.burkov): The approach of subtracting sets of strings is not
 	// really applicable here since in case of listening on all network
@@ -501,7 +509,7 @@ func (s *Server) setupLocalResolvers() (err error) {
 		PreferIPv6: s.conf.BootstrapPreferIPv6,
 	})
 	if err != nil {
-		return fmt.Errorf("parsing private upstreams: %w", err)
+		return fmt.Errorf("preparing private upstreams: %w", err)
 	}
 
 	s.localResolvers = &proxy.Proxy{

internal/dnsforward/dnsforward_test.go

@@ -1374,6 +1374,24 @@ func TestServer_Exchange(t *testing.T) {
 	refusingUpstream := aghtest.NewUpstreamMock(func(req *dns.Msg) (resp *dns.Msg, err error) {
 		return new(dns.Msg).SetRcode(req, dns.RcodeRefused), nil
 	})
+	zeroTTLUps := &aghtest.UpstreamMock{
+		OnAddress: func() (addr string) { return "zero.ttl.example" },
+		OnExchange: func(req *dns.Msg) (resp *dns.Msg, err error) {
+			resp = new(dns.Msg).SetReply(req)
+			hdr := dns.RR_Header{
+				Name:   req.Question[0].Name,
+				Rrtype: dns.TypePTR,
+				Class:  dns.ClassINET,
+				Ttl:    0,
+			}
+			resp.Answer = []dns.RR{&dns.PTR{
+				Hdr: hdr,
+				Ptr: localDomainHost,
+			}}
+
+			return resp, nil
+		},
+	}
 
 	srv := &Server{
 		recDetector: newRecursionDetector(0, 1),
@@ -1445,6 +1463,13 @@ func TestServer_Exchange(t *testing.T) {
 		locUpstream: nil,
 		req:         twosIP,
 		wantTTL:     defaultTTL * 2,
+	}, {
+		name:        "zero_ttl",
+		want:        localDomainHost,
+		wantErr:     nil,
+		locUpstream: zeroTTLUps,
+		req:         localIP,
+		wantTTL:     0,
 	}}
 
 	for _, tc := range testCases {
@@ -1468,6 +1493,7 @@ func TestServer_Exchange(t *testing.T) {
 
 	t.Run("resolving_disabled", func(t *testing.T) {
 		srv.conf.UsePrivateRDNS = false
+		t.Cleanup(func() { srv.conf.UsePrivateRDNS = true })
 
 		host, _, eerr := srv.Exchange(localIP)
 

internal/dnsforward/process.go

@@ -719,6 +719,8 @@ func (s *Server) processLocalPTR(dctx *dnsContext) (rc resultCode) {
 	if s.conf.UsePrivateRDNS {
 		s.recDetector.add(*pctx.Req)
 		if err := s.localResolvers.Resolve(pctx); err != nil {
+			log.Debug("dnsforward: resolving private address: %s", err)
+
 			// Generate the server failure if the private upstream configuration
 			// is empty.
 			//

internal/rdns/rdns.go

@@ -101,6 +101,8 @@ func (r *Default) Process(ip netip.Addr) (host string, changed bool) {
 		log.Debug("rdns: cache: adding item %q: %s", ip, err)
 	}
 
+	// TODO(e.burkov):  The name doesn't change if it's neither stored in cache
+	// nor resolved successfully.  Is it correct?
 	return host, fromCache == "" || host != fromCache
 }
 

internal/rdns/rdns_test.go

@@ -25,11 +25,6 @@ func TestDefault_Process(t *testing.T) {
 	localRevAddr1, err := netutil.IPToReversedAddr(localIP.AsSlice())
 	require.NoError(t, err)
 
-	config := &rdns.Config{
-		CacheSize: 100,
-		CacheTTL:  time.Hour,
-	}
-
 	testCases := []struct {
 		name string
 		addr netip.Addr
@@ -60,21 +55,21 @@ func TestDefault_Process(t *testing.T) {
 
 				switch ip {
 				case ip1:
-					return revAddr1, 0, nil
+					return revAddr1, time.Hour, nil
 				case ip2:
-					return revAddr2, 0, nil
+					return revAddr2, time.Hour, nil
 				case localIP:
-					return localRevAddr1, 0, nil
+					return localRevAddr1, time.Hour, nil
 				default:
-					return "", 0, nil
+					return "", time.Hour, nil
 				}
 			}
-			exchanger := &aghtest.Exchanger{
-				OnExchange: onExchange,
-			}
 
-			config.Exchanger = exchanger
-			r := rdns.New(config)
+			r := rdns.New(&rdns.Config{
+				CacheSize: 100,
+				CacheTTL:  time.Hour,
+				Exchanger: &aghtest.Exchanger{OnExchange: onExchange},
+			})
 
 			got, changed := r.Process(tc.addr)
 			require.True(t, changed)
@@ -90,4 +85,40 @@ func TestDefault_Process(t *testing.T) {
 			assert.Equal(t, 1, hit)
 		})
 	}
+
+	t.Run("zero_ttl", func(t *testing.T) {
+		const cacheTTL = time.Second / 2
+
+		zeroTTLExchanger := &aghtest.Exchanger{
+			OnExchange: func(ip netip.Addr) (host string, ttl time.Duration, err error) {
+				return revAddr1, 0, nil
+			},
+		}
+
+		r := rdns.New(&rdns.Config{
+			CacheSize: 1,
+			CacheTTL:  cacheTTL,
+			Exchanger: zeroTTLExchanger,
+		})
+
+		got, changed := r.Process(ip1)
+		require.True(t, changed)
+		assert.Equal(t, revAddr1, got)
+
+		zeroTTLExchanger.OnExchange = func(ip netip.Addr) (host string, ttl time.Duration, err error) {
+			return revAddr2, time.Hour, nil
+		}
+
+		require.EventuallyWithT(t, func(t *assert.CollectT) {
+			got, changed = r.Process(ip1)
+			assert.True(t, changed)
+			assert.Equal(t, revAddr2, got)
+		}, 2*cacheTTL, time.Millisecond*100)
+
+		assert.Never(t, func() (changed bool) {
+			_, changed = r.Process(ip1)
+
+			return changed
+		}, 2*cacheTTL, time.Millisecond*100)
+	})
 }