Pull request: 2704 local addresses vol.2

Merge in DNS/adguard-home from 2704-local-addresses-vol.2 to master Updates #2704. Updates #2829. Squashed commit of the following: commit 507d038c2709de59246fc0b65c3c4ab8e38d1990 Author: Eugene Burkov <e.burkov@adguard.com> Date: Wed Mar 31 14:33:05 2021 +0300 aghtest: fix file name commit 8e19f99337bee1d88ad6595adb96f9bb23fa3c41 Author: Eugene Burkov <e.burkov@adguard.com> Date: Wed Mar 31 14:06:43 2021 +0300 aghnet: rm redundant mutexes commit 361fa418b33ed160ca20862be1c455ab9378c03f Author: Eugene Burkov <e.burkov@adguard.com> Date: Wed Mar 31 13:45:30 2021 +0300 all: fix names, docs commit 14034f4f0230d7aaa3645054946ae5c278089a99 Merge: 35e265cc a72ce1cf Author: Eugene Burkov <e.burkov@adguard.com> Date: Wed Mar 31 13:38:15 2021 +0300 Merge branch 'master' into 2704-local-addresses-vol.2 commit 35e265cc8cd308ef1fda414b58c0217cb5f258e4 Author: Eugene Burkov <e.burkov@adguard.com> Date: Wed Mar 31 13:33:35 2021 +0300 aghnet: imp naming commit 7a7edac7208a40697d7bc50682b923a144e28e2b Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Mar 30 20:59:54 2021 +0300 changelog: oops, nope yet commit d26a5d2513daf662ac92053b5e235189a64cc022 Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Mar 30 20:55:53 2021 +0300 all: some renaming for the glory of semantics commit 9937fa619452b0742616217b975e3ff048d58acb Author: Eugene Burkov <e.burkov@adguard.com> Date: Mon Mar 29 15:34:42 2021 +0300 all: log changes commit d8d9e6dfeea8474466ee25f27021efdd3ddb1592 Author: Eugene Burkov <e.burkov@adguard.com> Date: Fri Mar 26 18:32:23 2021 +0300 all: imp localresolver, imp cutting off own addresses commit 344140df449b85925f19b460fd7dc7c08e29c35a Author: Eugene Burkov <e.burkov@adguard.com> Date: Fri Mar 26 14:53:33 2021 +0300 all: imp code quality commit 1c5c0babec73b125044e23dd3aa75d8eefc19b28 Author: Eugene Burkov <e.burkov@adguard.com> Date: Thu Mar 25 20:44:08 2021 +0300 all: fix go.mod commit 0b9fb3c2369a752e893af8ddc45a86bb9fb27ce5 Author: Eugene Burkov <e.burkov@adguard.com> Date: Thu Mar 25 20:38:51 2021 +0300 all: add error handling commit a7a2e51f57fc6f8f74b95a264ad345cd2a9e026e Merge: c13be634 27f4f052 Author: Eugene Burkov <e.burkov@adguard.com> Date: Thu Mar 25 19:48:36 2021 +0300 Merge branch 'master' into 2704-local-addresses-vol.2 commit c13be634f47bcaed9320a732a51c0e4752d0dad0 Author: Eugene Burkov <e.burkov@adguard.com> Date: Thu Mar 25 18:52:28 2021 +0300 all: cover rdns with tests, imp aghnet functionality commit 48bed9025944530c613ee53e7961d6d5fbabf8be Author: Eugene Burkov <e.burkov@adguard.com> Date: Wed Mar 24 20:18:07 2021 +0300 home: make rdns great again commit 1dbacfc8d5b6895807797998317fe3cc814617c1 Author: Eugene Burkov <e.burkov@adguard.com> Date: Wed Mar 24 16:07:52 2021 +0300 all: imp external client restriction commit 1208a319a7f4ffe7b7fa8956f245d7a19437c0a4 Author: Eugene Burkov <e.burkov@adguard.com> Date: Mon Mar 22 15:26:45 2021 +0300 all: finish local ptr processor commit c8827fc3db289e1a5d7a11d057743bab39957b02 Author: Eugene Burkov <e.burkov@adguard.com> Date: Tue Mar 2 13:41:22 2021 +0300 all: imp ipdetector, add local ptr processor
2021-03-31 15:00:47 +03:00
parent a72ce1cfae
commit 86444eacc2
30 changed files with 1418 additions and 360 deletions
--- a/internal/dnsforward/dns.go
+++ b/internal/dnsforward/dns.go
@@ -1,13 +1,14 @@
 package dnsforward

 import (
+	"errors"
 	"net"
 	"strings"
 	"time"

+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsfilter"
-	"github.com/AdguardTeam/AdGuardHome/internal/util"
 	"github.com/AdguardTeam/dnsproxy/proxy"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/miekg/dns"
@@ -26,6 +27,9 @@ type dnsContext struct {
 	// origResp is the response received from upstream.  It is set when the
 	// response is modified by filters.
 	origResp *dns.Msg
+	// unreversedReqIP stores an IP address obtained from PTR request if it
+	// was successfully parsed.
+	unreversedReqIP net.IP
 	// err is the error returned from a processing function.
 	err error
 	// clientID is the clientID from DOH, DOQ, or DOT, if provided.
@@ -78,9 +82,11 @@ func (s *Server) handleDNSRequest(_ *proxy.Proxy, d *proxy.DNSContext) error {
 	mods := []modProcessFunc{
 		processInitial,
 		s.processInternalHosts,
-		processInternalIPAddrs,
+		s.processRestrictLocal,
+		s.processInternalIPAddrs,
 		processClientID,
 		processFilteringBeforeRequest,
+		s.processLocalPTR,
 		processUpstream,
 		processDNSSECAfterResponse,
 		processFilteringAfterResponse,
@@ -185,6 +191,29 @@ func (s *Server) onDHCPLeaseChanged(flags int) {
 	s.tablePTRLock.Unlock()
 }

+// hostToIP tries to get an IP leased by DHCP and returns the copy of address
+// since the data inside the internal table may be changed while request
+// processing.  It's safe for concurrent use.
+func (s *Server) hostToIP(host string) (ip net.IP, ok bool) {
+	s.tableHostToIPLock.Lock()
+	defer s.tableHostToIPLock.Unlock()
+
+	if s.tableHostToIP == nil {
+		return nil, false
+	}
+
+	var ipFromTable net.IP
+	ipFromTable, ok = s.tableHostToIP[host]
+	if !ok {
+		return nil, false
+	}
+
+	ip = make(net.IP, len(ipFromTable))
+	copy(ip, ipFromTable)
+
+	return ip, true
+}
+
 // processInternalHosts respond to A requests if the target hostname is known to
 // the server.
 //
@@ -206,13 +235,9 @@ func (s *Server) processInternalHosts(dctx *dnsContext) (rc resultCode) {
 		return resultCodeSuccess
 	}

-	s.tableHostToIPLock.Lock()
-	if s.tableHostToIP == nil {
-		s.tableHostToIPLock.Unlock()
-		return resultCodeSuccess
-	}
-	ip, ok := s.tableHostToIP[host]
-	s.tableHostToIPLock.Unlock()
+	// TODO(e.burkov): Restrict the access for external clients.
+
+	ip, ok := s.hostToIP(host)
 	if !ok {
 		return resultCodeSuccess
 	}
@@ -220,62 +245,143 @@ func (s *Server) processInternalHosts(dctx *dnsContext) (rc resultCode) {
 	log.Debug("dns: internal record: %s -> %s", q.Name, ip)

 	resp := s.makeResponse(req)
-
 	if q.Qtype == dns.TypeA {
 		a := &dns.A{
 			Hdr: s.hdr(req, dns.TypeA),
-			A:   make([]byte, len(ip)),
+			A:   ip,
 		}
-
-		copy(a.A, ip)
 		resp.Answer = append(resp.Answer, a)
 	}
-
 	dctx.proxyCtx.Res = resp

 	return resultCodeSuccess
 }

-// Respond to PTR requests if the target IP address is leased by our DHCP server
-func processInternalIPAddrs(ctx *dnsContext) (rc resultCode) {
-	s := ctx.srv
-	req := ctx.proxyCtx.Req
-	if req.Question[0].Qtype != dns.TypePTR {
+// processRestrictLocal responds with empty answers to PTR requests for IP
+// addresses in locally-served network from external clients.
+func (s *Server) processRestrictLocal(ctx *dnsContext) (rc resultCode) {
+	d := ctx.proxyCtx
+	req := d.Req
+	q := req.Question[0]
+	if q.Qtype != dns.TypePTR {
+		// No need for restriction.
 		return resultCodeSuccess
 	}

-	arpa := req.Question[0].Name
-	arpa = strings.TrimSuffix(arpa, ".")
-	arpa = strings.ToLower(arpa)
-	ip := util.DNSUnreverseAddr(arpa)
+	ip := aghnet.UnreverseAddr(q.Name)
+	if ip == nil {
+		// That's weird.
+		//
+		// TODO(e.burkov): Research the cases when it could happen.
+		return resultCodeSuccess
+	}
+
+	// Restrict an access to local addresses for external clients.  We also
+	// assume that all the DHCP leases we give are locally-served or at
+	// least don't need to be unaccessable externally.
+	if s.subnetDetector.IsLocallyServedNetwork(ip) {
+		clientIP := IPFromAddr(d.Addr)
+		if !s.subnetDetector.IsLocallyServedNetwork(clientIP) {
+			log.Debug("dns: %q requests for internal ip", clientIP)
+			d.Res = s.makeResponse(req)
+
+			// Do not even put into query log.
+			return resultCodeFinish
+		}
+	}
+
+	// Do not perform unreversing ever again.
+	ctx.unreversedReqIP = ip
+
+	// Nothing to restrict.
+	return resultCodeSuccess
+}
+
+// ipToHost tries to get a hostname leased by DHCP.  It's safe for concurrent
+// use.
+func (s *Server) ipToHost(ip net.IP) (host string, ok bool) {
+	s.tablePTRLock.Lock()
+	defer s.tablePTRLock.Unlock()
+
+	if s.tablePTR == nil {
+		return "", false
+	}
+
+	host, ok = s.tablePTR[ip.String()]
+
+	return host, ok
+}
+
+// Respond to PTR requests if the target IP is leased by our DHCP server and the
+// requestor is inside the local network.
+func (s *Server) processInternalIPAddrs(ctx *dnsContext) (rc resultCode) {
+	d := ctx.proxyCtx
+	if d.Res != nil {
+		return resultCodeSuccess
+	}
+
+	ip := ctx.unreversedReqIP
 	if ip == nil {
 		return resultCodeSuccess
 	}

-	s.tablePTRLock.Lock()
-	if s.tablePTR == nil {
-		s.tablePTRLock.Unlock()
-		return resultCodeSuccess
-	}
-	host, ok := s.tablePTR[ip.String()]
-	s.tablePTRLock.Unlock()
+	host, ok := s.ipToHost(ip)
 	if !ok {
 		return resultCodeSuccess
 	}

-	log.Debug("dns: reverse-lookup: %s -> %s", arpa, host)
+	log.Debug("dns: reverse-lookup: %s -> %s", ip, host)

+	req := d.Req
 	resp := s.makeResponse(req)
-	ptr := &dns.PTR{}
-	ptr.Hdr = dns.RR_Header{
-		Name:   req.Question[0].Name,
-		Rrtype: dns.TypePTR,
-		Ttl:    s.conf.BlockedResponseTTL,
-		Class:  dns.ClassINET,
+	ptr := &dns.PTR{
+		Hdr: dns.RR_Header{
+			Name:   req.Question[0].Name,
+			Rrtype: dns.TypePTR,
+			Ttl:    s.conf.BlockedResponseTTL,
+			Class:  dns.ClassINET,
+		},
+		Ptr: dns.Fqdn(host),
 	}
-	ptr.Ptr = host + "."
 	resp.Answer = append(resp.Answer, ptr)
-	ctx.proxyCtx.Res = resp
+	d.Res = resp
+
+	return resultCodeSuccess
+}
+
+// processLocalPTR responds to PTR requests if the target IP is detected to be
+// inside the local network and the query was not answered from DHCP.
+func (s *Server) processLocalPTR(ctx *dnsContext) (rc resultCode) {
+	d := ctx.proxyCtx
+	if d.Res != nil {
+		return resultCodeSuccess
+	}
+
+	ip := ctx.unreversedReqIP
+	if ip == nil {
+		return resultCodeSuccess
+	}
+
+	if !s.subnetDetector.IsLocallyServedNetwork(ip) {
+		return resultCodeSuccess
+	}
+
+	req := d.Req
+	resp, err := s.localResolvers.Exchange(req)
+	if err != nil {
+		if errors.Is(err, aghnet.NoUpstreamsErr) {
+			d.Res = s.genNXDomain(req)
+
+			return resultCodeFinish
+		}
+
+		ctx.err = err
+
+		return resultCodeError
+	}
+
+	d.Res = resp
+
 	return resultCodeSuccess
 }

--- a/internal/dnsforward/dns_test.go
+++ b/internal/dnsforward/dns_test.go
@@ -4,7 +4,10 @@ import (
 	"net"
 	"testing"

+	"github.com/AdguardTeam/AdGuardHome/internal/aghtest"
+	"github.com/AdguardTeam/AdGuardHome/internal/dnsfilter"
 	"github.com/AdguardTeam/dnsproxy/proxy"
+	"github.com/AdguardTeam/dnsproxy/upstream"
 	"github.com/miekg/dns"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -120,3 +123,74 @@ func TestServer_ProcessInternalHosts(t *testing.T) {
 		})
 	}
 }
+
+func TestLocalRestriction(t *testing.T) {
+	s := createTestServer(t, &dnsfilter.Config{}, ServerConfig{
+		UDPListenAddrs: []*net.UDPAddr{{}},
+		TCPListenAddrs: []*net.TCPAddr{{}},
+	})
+	ups := &aghtest.TestUpstream{
+		Reverse: map[string][]string{
+			"251.252.253.254.in-addr.arpa.": {"host1.example.net."},
+			"1.1.168.192.in-addr.arpa.":     {"some.local-client."},
+		},
+	}
+	s.localResolvers = &aghtest.Exchanger{Ups: ups}
+	s.conf.UpstreamConfig.Upstreams = []upstream.Upstream{ups}
+	startDeferStop(t, s)
+
+	testCases := []struct {
+		name     string
+		want     string
+		question net.IP
+		cliIP    net.IP
+		wantLen  int
+	}{{
+		name:     "from_local_to_external",
+		want:     "host1.example.net.",
+		question: net.IP{254, 253, 252, 251},
+		cliIP:    net.IP{192, 168, 10, 10},
+		wantLen:  1,
+	}, {
+		name:     "from_external_for_local",
+		want:     "",
+		question: net.IP{192, 168, 1, 1},
+		cliIP:    net.IP{254, 253, 252, 251},
+		wantLen:  0,
+	}, {
+		name:     "from_local_for_local",
+		want:     "some.local-client.",
+		question: net.IP{192, 168, 1, 1},
+		cliIP:    net.IP{192, 168, 1, 2},
+		wantLen:  1,
+	}, {
+		name:     "from_external_for_external",
+		want:     "host1.example.net.",
+		question: net.IP{254, 253, 252, 251},
+		cliIP:    net.IP{254, 253, 252, 255},
+		wantLen:  1,
+	}}
+
+	for _, tc := range testCases {
+		reqAddr, err := dns.ReverseAddr(tc.question.String())
+		require.NoError(t, err)
+		req := createTestMessageWithType(reqAddr, dns.TypePTR)
+
+		pctx := &proxy.DNSContext{
+			Proto: proxy.ProtoTCP,
+			Req:   req,
+			Addr: &net.TCPAddr{
+				IP: tc.cliIP,
+			},
+		}
+		t.Run(tc.name, func(t *testing.T) {
+			err = s.handleDNSRequest(nil, pctx)
+			require.Nil(t, err)
+			require.NotNil(t, pctx.Res)
+			require.Len(t, pctx.Res.Answer, tc.wantLen)
+			if tc.wantLen > 0 {
+				assert.Equal(t, tc.want, pctx.Res.Answer[0].Header().Name)
+			}
+		})
+	}
+}
--- a/internal/dnsforward/dnsforward.go
+++ b/internal/dnsforward/dnsforward.go
@@ -11,6 +11,7 @@ import (
 	"sync"
 	"time"

+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsfilter"
 	"github.com/AdguardTeam/AdGuardHome/internal/querylog"
@@ -60,7 +61,9 @@ type Server struct {
 	// be a valid top-level domain plus dots on each side.
 	autohostSuffix string

-	ipset ipsetCtx
+	ipset          ipsetCtx
+	subnetDetector *aghnet.SubnetDetector
+	localResolvers aghnet.Exchanger

 	tableHostToIP     map[string]net.IP // "hostname -> IP" table for internal addresses (DHCP)
 	tableHostToIPLock sync.Mutex
@@ -84,11 +87,13 @@ const defaultAutohostSuffix = ".lan."

 // DNSCreateParams are parameters to create a new server.
 type DNSCreateParams struct {
-	DNSFilter   *dnsfilter.DNSFilter
-	Stats       stats.Stats
-	QueryLog    querylog.QueryLog
-	DHCPServer  dhcpd.ServerInterface
-	AutohostTLD string
+	DNSFilter      *dnsfilter.DNSFilter
+	Stats          stats.Stats
+	QueryLog       querylog.QueryLog
+	DHCPServer     dhcpd.ServerInterface
+	SubnetDetector *aghnet.SubnetDetector
+	LocalResolvers aghnet.Exchanger
+	AutohostTLD    string
 }

 // tldToSuffix converts a top-level domain into an autohost suffix.
@@ -121,6 +126,8 @@ func NewServer(p DNSCreateParams) (s *Server, err error) {
 		dnsFilter:      p.DNSFilter,
 		stats:          p.Stats,
 		queryLog:       p.QueryLog,
+		subnetDetector: p.SubnetDetector,
+		localResolvers: p.LocalResolvers,
 		autohostSuffix: autohostSuffix,
 	}

--- a/internal/dnsforward/dnsforward_test.go
+++ b/internal/dnsforward/dnsforward_test.go
@@ -18,6 +18,7 @@ import (
 	"testing"
 	"time"

+	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghtest"
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsfilter"
@@ -64,7 +65,16 @@ func createTestServer(t *testing.T, filterConf *dnsfilter.Config, forwardConf Se

 	f := dnsfilter.New(filterConf, filters)

-	s, err := NewServer(DNSCreateParams{DNSFilter: f})
+	snd, err := aghnet.NewSubnetDetector()
+	require.NoError(t, err)
+	require.NotNil(t, snd)
+
+	var s *Server
+	s, err = NewServer(DNSCreateParams{
+		DNSFilter:      f,
+		SubnetDetector: snd,
+		LocalResolvers: &aghtest.Exchanger{},
+	})
 	require.NoError(t, err)

 	s.conf = forwardConf
@@ -710,8 +720,15 @@ func TestBlockedCustomIP(t *testing.T) {
 		Data: []byte(rules),
 	}}

-	s, err := NewServer(DNSCreateParams{
-		DNSFilter: dnsfilter.New(&dnsfilter.Config{}, filters),
+	snd, err := aghnet.NewSubnetDetector()
+	require.NoError(t, err)
+	require.NotNil(t, snd)
+
+	var s *Server
+	s, err = NewServer(DNSCreateParams{
+		DNSFilter:      dnsfilter.New(&dnsfilter.Config{}, filters),
+		SubnetDetector: snd,
+		LocalResolvers: &aghtest.Exchanger{},
 	})
 	require.NoError(t, err)

@@ -841,18 +858,26 @@ func TestRewrite(t *testing.T) {
 	}
 	f := dnsfilter.New(c, nil)

-	s, err := NewServer(DNSCreateParams{DNSFilter: f})
+	snd, err := aghnet.NewSubnetDetector()
+	require.NoError(t, err)
+	require.NotNil(t, snd)
+
+	var s *Server
+	s, err = NewServer(DNSCreateParams{
+		DNSFilter:      f,
+		SubnetDetector: snd,
+		LocalResolvers: &aghtest.Exchanger{},
+	})
 	require.NoError(t, err)

-	err = s.Prepare(&ServerConfig{
+	assert.NoError(t, s.Prepare(&ServerConfig{
 		UDPListenAddrs: []*net.UDPAddr{{}},
 		TCPListenAddrs: []*net.TCPAddr{{}},
 		FilteringConfig: FilteringConfig{
 			ProtectionEnabled: true,
 			UpstreamDNS:       []string{"8.8.8.8:53"},
 		},
-	})
-	assert.NoError(t, err)
+	}))

 	s.conf.UpstreamConfig.Upstreams = []upstream.Upstream{
 		&aghtest.TestUpstream{
@@ -1134,9 +1159,16 @@ func (d *testDHCP) Leases(flags int) []dhcpd.Lease {
 func (d *testDHCP) SetOnLeaseChanged(onLeaseChanged dhcpd.OnLeaseChangedT) {}

 func TestPTRResponseFromDHCPLeases(t *testing.T) {
-	s, err := NewServer(DNSCreateParams{
-		DNSFilter:  dnsfilter.New(&dnsfilter.Config{}, nil),
-		DHCPServer: &testDHCP{},
+	snd, err := aghnet.NewSubnetDetector()
+	require.NoError(t, err)
+	require.NotNil(t, snd)
+
+	var s *Server
+	s, err = NewServer(DNSCreateParams{
+		DNSFilter:      dnsfilter.New(&dnsfilter.Config{}, nil),
+		DHCPServer:     &testDHCP{},
+		SubnetDetector: snd,
+		LocalResolvers: &aghtest.Exchanger{},
 	})
 	require.NoError(t, err)

@@ -1192,7 +1224,17 @@ func TestPTRResponseFromHosts(t *testing.T) {
 	c.AutoHosts.Init(hf.Name())
 	t.Cleanup(c.AutoHosts.Close)

-	s, err := NewServer(DNSCreateParams{DNSFilter: dnsfilter.New(&c, nil)})
+	var snd *aghnet.SubnetDetector
+	snd, err = aghnet.NewSubnetDetector()
+	require.NoError(t, err)
+	require.NotNil(t, snd)
+
+	var s *Server
+	s, err = NewServer(DNSCreateParams{
+		DNSFilter:      dnsfilter.New(&c, nil),
+		SubnetDetector: snd,
+		LocalResolvers: &aghtest.Exchanger{},
+	})
 	require.NoError(t, err)

 	s.conf.UDPListenAddrs = []*net.UDPAddr{{}}
--- a/internal/dnsforward/filter.go
+++ b/internal/dnsforward/filter.go
@@ -46,6 +46,8 @@ func (s *Server) getClientRequestFilteringSettings(ctx *dnsContext) *dnsfilter.F
 // was filtered.
 func (s *Server) filterDNSRequest(ctx *dnsContext) (*dnsfilter.Result, error) {
 	d := ctx.proxyCtx
+	// TODO(e.burkov): Consistently use req instead of d.Req since it is
+	// declared.
 	req := d.Req
 	host := strings.TrimSuffix(req.Question[0].Name, ".")
 	res, err := s.dnsFilter.CheckHost(host, d.Req.Question[0].Qtype, ctx.setts)