Pull request 2018: 6231 filter local addrs

Merge in DNS/adguard-home from 6231-filter-local-addrs to master

Updates #6231.

Squashed commit of the following:

commit 9a60d4e33f25c7dd7eaa4366d8397389196156ac
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Sep 28 18:59:51 2023 +0300

    dnsforward: imp code

commit f0c3452525c227b0ee6e761c4a6b68543900d5b5
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Sep 27 18:12:47 2023 +0300

    all: don't match nets

commit 572dc0f24e74560adaa4d89ddc921dfd7e1fed02
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Sep 27 13:37:48 2023 +0300

    dnsforward: move some code, rm dups

commit 3af627ce9c7f6f4d2aa695a7660b8a0027fa241c
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Sep 25 19:21:05 2023 +0300

    dnsforward: imp naming

commit cad1e4e71662836d1dfc79bc2979599b7a29fea1
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Sep 25 19:17:53 2023 +0300

    dnsforward: imp code

commit 23d69700789d5652bd25cc089f16afb8b38e51f8
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Sep 25 19:08:48 2023 +0300

    dnsforward: add upstream matcher

commit 5819c594a2a8d8bf2cd42883133e21ca7ed2681a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Sep 22 18:31:37 2023 +0300

    all: imp code, docs

commit d07ea96bb568161e029e22d69329a368d9eeb729
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Sep 22 18:09:09 2023 +0300

    all: imp code

commit 38a912a62c63247c4c5bb61b67ccc9bfd255feff
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Sep 22 15:48:25 2023 +0300

    all: imp code

commit 811212fa16bc231a8da990c075d7231c471c7e3b
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Sep 21 19:05:07 2023 +0300

    all: imp addrs detection

CHANGELOG.md

@@ -32,9 +32,12 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 ### Fixed
 
+- Wrong algorithm for filtering self addresses from the list of private upstream
+  DNS servers ([#6231]).
 - An accidental change in DNS rewrite priority ([#6226]).
 
 [#6226]: https://github.com/AdguardTeam/AdGuardHome/issues/6226
+[#6231]: https://github.com/AdguardTeam/AdGuardHome/issues/6231
 
 <!--
 NOTE: Add new changes ABOVE THIS COMMENT.

internal/aghnet/net.go

@@ -9,6 +9,7 @@ import (
 	"io"
 	"net"
 	"net/netip"
+	"net/url"
 	"syscall"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
@@ -263,7 +264,7 @@ func IsAddrInUse(err error) (ok bool) {
 
 // CollectAllIfacesAddrs returns the slice of all network interfaces IP
 // addresses without port number.
-func CollectAllIfacesAddrs() (addrs []string, err error) {
+func CollectAllIfacesAddrs() (addrs []netip.Addr, err error) {
 	var ifaceAddrs []net.Addr
 	ifaceAddrs, err = netInterfaceAddrs()
 	if err != nil {
@@ -271,19 +272,41 @@ func CollectAllIfacesAddrs() (addrs []string, err error) {
 	}
 
 	for _, addr := range ifaceAddrs {
-		cidr := addr.String()
+		var p netip.Prefix
-		var ip net.IP
+		p, err = netip.ParsePrefix(addr.String())
-		ip, _, err = net.ParseCIDR(cidr)
 		if err != nil {
-			return nil, fmt.Errorf("parsing cidr: %w", err)
+			// Don't wrap the error since it's informative enough as is.
+			return nil, err
 		}
 
-		addrs = append(addrs, ip.String())
+		addrs = append(addrs, p.Addr())
 	}
 
 	return addrs, nil
 }
 
+// ParseAddrPort parses an [netip.AddrPort] from s, which should be either a
+// valid IP, optionally with port, or a valid URL with plain IP address.  The
+// defaultPort is used if s doesn't contain port number.
+func ParseAddrPort(s string, defaultPort uint16) (ipp netip.AddrPort, err error) {
+	u, err := url.Parse(s)
+	if err == nil && u.Host != "" {
+		s = u.Host
+	}
+
+	ipp, err = netip.ParseAddrPort(s)
+	if err != nil {
+		ip, parseErr := netip.ParseAddr(s)
+		if parseErr != nil {
+			return ipp, errors.Join(err, parseErr)
+		}
+
+		return netip.AddrPortFrom(ip, defaultPort), nil
+	}
+
+	return ipp, nil
+}
+
 // BroadcastFromPref calculates the broadcast IP address for p.
 func BroadcastFromPref(p netip.Prefix) (bc netip.Addr) {
 	bc = p.Addr().Unmap()

internal/aghnet/net_internal_test.go

@@ -230,7 +230,7 @@ func TestCollectAllIfacesAddrs(t *testing.T) {
 		name       string
 		wantErrMsg string
 		addrs      []net.Addr
-		wantAddrs  []string
+		wantAddrs  []netip.Addr
 	}{{
 		name:       "success",
 		wantErrMsg: ``,
@@ -241,10 +241,13 @@ func TestCollectAllIfacesAddrs(t *testing.T) {
 			IP:   net.IP{4, 3, 2, 1},
 			Mask: net.CIDRMask(16, netutil.IPv4BitLen),
 		}},
-		wantAddrs: []string{"1.2.3.4", "4.3.2.1"},
+		wantAddrs: []netip.Addr{
+			netip.MustParseAddr("1.2.3.4"),
+			netip.MustParseAddr("4.3.2.1"),
+		},
 	}, {
 		name:       "not_cidr",
-		wantErrMsg: `parsing cidr: invalid CIDR address: 1.2.3.4`,
+		wantErrMsg: `netip.ParsePrefix("1.2.3.4"): no '/'`,
 		addrs: []net.Addr{&net.IPAddr{
 			IP: net.IP{1, 2, 3, 4},
 		}},
@@ -269,12 +272,11 @@ func TestCollectAllIfacesAddrs(t *testing.T) {
 
 	t.Run("internal_error", func(t *testing.T) {
 		const errAddrs errors.Error = "can't get addresses"
-		const wantErrMsg string = `getting interfaces addresses: ` + string(errAddrs)
 
 		substNetInterfaceAddrs(t, func() ([]net.Addr, error) { return nil, errAddrs })
 
 		_, err := CollectAllIfacesAddrs()
-		testutil.AssertErrorMsg(t, wantErrMsg, err)
+		assert.ErrorIs(t, err, errAddrs)
 	})
 }
 

internal/aghnet/net_test.go

@@ -2,10 +2,15 @@ package aghnet_test
 
 import (
 	"io/fs"
+	"net/netip"
+	"net/url"
 	"os"
 	"testing"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
+	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/testutil"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestMain(m *testing.M) {
@@ -14,3 +19,76 @@ func TestMain(m *testing.M) {
 
 // testdata is the filesystem containing data for testing the package.
 var testdata fs.FS = os.DirFS("./testdata")
+
+func TestParseAddrPort(t *testing.T) {
+	const defaultPort = 1
+
+	v4addr := netip.MustParseAddr("1.2.3.4")
+
+	testCases := []struct {
+		name       string
+		input      string
+		wantErrMsg string
+		want       netip.AddrPort
+	}{{
+		name:       "success_ip",
+		input:      v4addr.String(),
+		wantErrMsg: "",
+		want:       netip.AddrPortFrom(v4addr, defaultPort),
+	}, {
+		name:       "success_ip_port",
+		input:      netutil.JoinHostPort(v4addr.String(), 5),
+		wantErrMsg: "",
+		want:       netip.AddrPortFrom(v4addr, 5),
+	}, {
+		name: "success_url",
+		input: (&url.URL{
+			Scheme: "tcp",
+			Host:   v4addr.String(),
+		}).String(),
+		wantErrMsg: "",
+		want:       netip.AddrPortFrom(v4addr, defaultPort),
+	}, {
+		name: "success_url_port",
+		input: (&url.URL{
+			Scheme: "tcp",
+			Host:   netutil.JoinHostPort(v4addr.String(), 5),
+		}).String(),
+		wantErrMsg: "",
+		want:       netip.AddrPortFrom(v4addr, 5),
+	}, {
+		name:  "error_invalid_ip",
+		input: "256.256.256.256",
+		wantErrMsg: `not an ip:port
+ParseAddr("256.256.256.256"): IPv4 field has value >255`,
+		want: netip.AddrPort{},
+	}, {
+		name:  "error_invalid_port",
+		input: netutil.JoinHostPort(v4addr.String(), -5),
+		wantErrMsg: `invalid port "-5" parsing "1.2.3.4:-5"
+ParseAddr("1.2.3.4:-5"): unexpected character (at ":-5")`,
+		want: netip.AddrPort{},
+	}, {
+		name:  "error_invalid_url",
+		input: "tcp:://1.2.3.4",
+		wantErrMsg: `invalid port "//1.2.3.4" parsing "tcp:://1.2.3.4"
+ParseAddr("tcp:://1.2.3.4"): each colon-separated field must have at least ` +
+			`one digit (at "tcp:://1.2.3.4")`,
+		want: netip.AddrPort{},
+	}, {
+		name:  "empty",
+		input: "",
+		want:  netip.AddrPort{},
+		wantErrMsg: `not an ip:port
+ParseAddr(""): unable to parse IP`,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ap, err := aghnet.ParseAddrPort(tc.input, defaultPort)
+			testutil.AssertErrorMsg(t, tc.wantErrMsg, err)
+
+			assert.Equal(t, tc.want, ap)
+		})
+	}
+}

internal/aghnet/systemresolvers_windows.go

@@ -138,7 +138,7 @@ func (sr *systemResolvers) getAddrs() (addrs []string, err error) {
 
 	// Don't close StdoutPipe since Wait do it for us in ¿most? cases.
 	//
-	// See go doc os/exec.Cmd.StdoutPipe.
+	// See [exec.Cmd.StdoutPipe].
 
 	return addrs, nil
 }

internal/dnsforward/config.go

@@ -12,10 +12,12 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
 	"github.com/AdguardTeam/AdGuardHome/internal/client"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
 	"github.com/AdguardTeam/dnsproxy/proxy"
+	"github.com/AdguardTeam/dnsproxy/upstream"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
@@ -392,6 +394,124 @@ func (s *Server) prepareIpsetListSettings() (err error) {
 	return s.ipset.init(ipsets)
 }
 
+// collectListenAddr adds addrPort to addrs.  It also adds its port to
+// unspecPorts if its address is unspecified.
+func collectListenAddr(
+	addrPort netip.AddrPort,
+	addrs map[netip.AddrPort]unit,
+	unspecPorts map[uint16]unit,
+) {
+	if addrPort == (netip.AddrPort{}) {
+		return
+	}
+
+	addrs[addrPort] = unit{}
+	if addrPort.Addr().IsUnspecified() {
+		unspecPorts[addrPort.Port()] = unit{}
+	}
+}
+
+// collectDNSAddrs returns configured set of listening addresses.  It also
+// returns a set of ports of each unspecified listening address.
+func (conf *ServerConfig) collectDNSAddrs() (
+	addrs map[netip.AddrPort]unit,
+	unspecPorts map[uint16]unit,
+) {
+	// TODO(e.burkov):  Perhaps, we shouldn't allocate as much memory, since the
+	// TCP and UDP listening addresses are currently the same.
+	addrs = make(map[netip.AddrPort]unit, len(conf.TCPListenAddrs)+len(conf.UDPListenAddrs))
+	unspecPorts = map[uint16]unit{}
+
+	for _, laddr := range conf.TCPListenAddrs {
+		collectListenAddr(laddr.AddrPort(), addrs, unspecPorts)
+	}
+
+	for _, laddr := range conf.UDPListenAddrs {
+		collectListenAddr(laddr.AddrPort(), addrs, unspecPorts)
+	}
+
+	return addrs, unspecPorts
+}
+
+// defaultPlainDNSPort is the default port for plain DNS.
+const defaultPlainDNSPort uint16 = 53
+
+// upstreamMatcher is a function that matches address of an upstream.
+type upstreamMatcher func(addr netip.AddrPort) (ok bool)
+
+// filterOut filters out all the upstreams that match um.  It returns all the
+// closing errors joined.
+func (um upstreamMatcher) filterOut(upsConf *proxy.UpstreamConfig) (err error) {
+	var errs []error
+	delFunc := func(u upstream.Upstream) (ok bool) {
+		// TODO(e.burkov):  We should probably consider the protocol of u to
+		// only filter out the listening addresses of the same protocol.
+		addr, parseErr := aghnet.ParseAddrPort(u.Address(), defaultPlainDNSPort)
+		if parseErr != nil || !um(addr) {
+			// Don't filter out the upstream if it either cannot be parsed, or
+			// does not match um.
+			return false
+		}
+
+		errs = append(errs, u.Close())
+
+		return true
+	}
+
+	upsConf.Upstreams = slices.DeleteFunc(upsConf.Upstreams, delFunc)
+	for d, ups := range upsConf.DomainReservedUpstreams {
+		upsConf.DomainReservedUpstreams[d] = slices.DeleteFunc(ups, delFunc)
+	}
+	for d, ups := range upsConf.SpecifiedDomainUpstreams {
+		upsConf.SpecifiedDomainUpstreams[d] = slices.DeleteFunc(ups, delFunc)
+	}
+
+	return errors.Join(errs...)
+}
+
+// filterOurAddrs filters out all the upstreams that pointing to the local
+// listening addresses to avoid recursive queries.  upsConf may appear empty
+// after the filtering.  All the filtered upstreams are closed and these
+// closings errors are joined.
+func (conf *ServerConfig) filterOurAddrs(upsConf *proxy.UpstreamConfig) (err error) {
+	addrs, unspecPorts := conf.collectDNSAddrs()
+	if len(addrs) == 0 {
+		log.Debug("dnsforward: no listen addresses")
+
+		return nil
+	}
+
+	var matcher upstreamMatcher
+	if len(unspecPorts) == 0 {
+		log.Debug("dnsforward: filtering out addresses %s", addrs)
+
+		matcher = func(a netip.AddrPort) (ok bool) {
+			_, ok = addrs[a]
+
+			return ok
+		}
+	} else {
+		var ifaceAddrs []netip.Addr
+		ifaceAddrs, err = aghnet.CollectAllIfacesAddrs()
+		if err != nil {
+			// Don't wrap the error since it's informative enough as is.
+			return err
+		}
+
+		log.Debug("dnsforward: filtering out addresses %s on ports %d", ifaceAddrs, unspecPorts)
+
+		matcher = func(a netip.AddrPort) (ok bool) {
+			if _, ok = unspecPorts[a.Port()]; ok {
+				return slices.Contains(ifaceAddrs, a.Addr())
+			}
+
+			return false
+		}
+	}
+
+	return matcher.filterOut(upsConf)
+}
+
 // prepareTLS - prepares TLS configuration for the DNS proxy
 func (s *Server) prepareTLS(proxyConfig *proxy.Config) (err error) {
 	if len(s.conf.CertificateChainData) == 0 || len(s.conf.PrivateKeyData) == 0 {

internal/dnsforward/dnsforward.go

@@ -439,57 +439,6 @@ func (s *Server) startLocked() error {
 // faster than ordinary upstreams.
 const defaultLocalTimeout = 1 * time.Second
 
-// collectDNSIPAddrs returns IP addresses the server is listening on without
-// port numbers.  For internal use only.
-func (s *Server) collectDNSIPAddrs() (addrs []string, err error) {
-	addrs = make([]string, len(s.conf.TCPListenAddrs)+len(s.conf.UDPListenAddrs))
-	var i int
-	var ip net.IP
-	for _, addr := range s.conf.TCPListenAddrs {
-		if addr == nil {
-			continue
-		}
-
-		if ip = addr.IP; ip.IsUnspecified() {
-			return aghnet.CollectAllIfacesAddrs()
-		}
-
-		addrs[i] = ip.String()
-		i++
-	}
-	for _, addr := range s.conf.UDPListenAddrs {
-		if addr == nil {
-			continue
-		}
-
-		if ip = addr.IP; ip.IsUnspecified() {
-			return aghnet.CollectAllIfacesAddrs()
-		}
-
-		addrs[i] = ip.String()
-		i++
-	}
-
-	return addrs[:i], nil
-}
-
-func (s *Server) filterOurDNSAddrs(addrs []string) (filtered []string, err error) {
-	var ourAddrs []string
-	ourAddrs, err = s.collectDNSIPAddrs()
-	if err != nil {
-		return nil, err
-	}
-
-	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
-	// interfaces we should check the whole interface's network to cut off
-	// all the loopback addresses as well.
-	return stringutil.FilterOut(addrs, ourAddrsSet.Has), nil
-}
-
 // setupLocalResolvers initializes the resolvers for local addresses.  For
 // internal use only.
 func (s *Server) setupLocalResolvers() (err error) {
@@ -503,18 +452,12 @@ func (s *Server) setupLocalResolvers() (err error) {
 		resolvers = stringutil.FilterOut(resolvers, IsCommentOrEmpty)
 	}
 
-	resolvers, err = s.filterOurDNSAddrs(resolvers)
-	if err != nil {
-		return err
-	}
-
 	log.Debug("dnsforward: upstreams to resolve ptr for local addresses: %v", resolvers)
 
-	uc, err := s.prepareUpstreamConfig(resolvers, nil, &upstream.Options{
+	uc, err := s.prepareLocalUpstreamConfig(resolvers, nil, &upstream.Options{
 		Bootstrap: bootstraps,
 		Timeout:   defaultLocalTimeout,
 		// TODO(e.burkov): Should we verify server's certificates?
-
 		PreferIPv6: s.conf.BootstrapPreferIPv6,
 	})
 	if err != nil {

internal/dnsforward/http.go

@@ -142,9 +142,9 @@ func (s *Server) getDNSConfig() (c *jsonDNSConfig) {
 		upstreamMode = "parallel"
 	}
 
-	defLocalPTRUps, err := s.filterOurDNSAddrs(s.sysResolvers.Get())
+	defPTRUps, err := s.defaultLocalPTRUpstreams()
 	if err != nil {
-		log.Debug("getting dns configuration: %s", err)
+		log.Error("dnsforward: %s", err)
 	}
 
 	return &jsonDNSConfig{
@@ -171,11 +171,30 @@ func (s *Server) getDNSConfig() (c *jsonDNSConfig) {
 		ResolveClients:           &resolveClients,
 		UsePrivateRDNS:           &usePrivateRDNS,
 		LocalPTRUpstreams:        &localPTRUpstreams,
-		DefaultLocalPTRUpstreams: defLocalPTRUps,
+		DefaultLocalPTRUpstreams: defPTRUps,
 		DisabledUntil:            protectionDisabledUntil,
 	}
 }
 
+// defaultLocalPTRUpstreams returns the list of default local PTR resolvers
+// filtered of AdGuard Home's own DNS server addresses.  It may appear empty.
+func (s *Server) defaultLocalPTRUpstreams() (ups []string, err error) {
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
+	uc, err := s.prepareLocalUpstreamConfig(s.sysResolvers.Get(), nil, nil)
+	if err != nil {
+		return nil, fmt.Errorf("getting system upstream config: %w", err)
+	}
+	defer func() { err = errors.Join(err, uc.Close()) }()
+
+	for _, u := range uc.Upstreams {
+		ups = append(ups, u.Address())
+	}
+
+	return ups, nil
+}
+
 // handleGetConfig handles requests to the GET /control/dns_info endpoint.
 func (s *Server) handleGetConfig(w http.ResponseWriter, r *http.Request) {
 	resp := s.getDNSConfig()

internal/dnsforward/upstreams.go

@@ -69,8 +69,8 @@ func (s *Server) prepareUpstreamSettings() (err error) {
 	return nil
 }
 
-// prepareUpstreamConfig sets upstream configuration based on upstreams and
+// prepareUpstreamConfig returns the upstream configuration based on upstreams
-// configuration of s.
+// and configuration of s.
 func (s *Server) prepareUpstreamConfig(
 	upstreams []string,
 	defaultUpstreams []string,
@@ -103,6 +103,27 @@ func (s *Server) prepareUpstreamConfig(
 	return uc, nil
 }
 
+// prepareLocalUpstreamConfig returns the upstream configuration for private
+// upstreams based on upstreams and configuration of s.  It also filters out
+// the own listening addresses from the upstreams, so it may appear empty.
+func (s *Server) prepareLocalUpstreamConfig(
+	upstreams []string,
+	defaultUpstreams []string,
+	opts *upstream.Options,
+) (uc *proxy.UpstreamConfig, err error) {
+	uc, err = s.prepareUpstreamConfig(upstreams, defaultUpstreams, opts)
+	if err != nil {
+		return nil, fmt.Errorf("preparing private upstreams: %w", err)
+	}
+
+	err = s.conf.filterOurAddrs(uc)
+	if err != nil {
+		return nil, fmt.Errorf("filtering private upstreams: %w", err)
+	}
+
+	return uc, nil
+}
+
 // replaceUpstreamsWithHosts replaces unique upstreams with their resolved
 // versions based on the system hosts file.
 //