Pull request: 3184 disable private ptr

Merge in DNS/adguard-home from 3184-disable-ptr to master

Updates #3184.

Squashed commit of the following:

commit b78ac2e
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Wed May 26 17:20:34 2021 +0300

    all: rename dns config field

commit 3651213
Author: Ildar Kamalov <ik@adguard.com>
Date:   Wed May 26 15:55:44 2021 +0300

    client: handle local ips rdns

commit 9a69183
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Wed May 26 14:43:13 2021 +0300

    all: imp naming

commit 771b7a3
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Wed May 26 14:24:38 2021 +0300

    all: imp docs, code

commit be96089
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Wed May 26 13:23:56 2021 +0300

    all: imp docs & log changes

commit 4e645a5
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Wed May 26 12:49:44 2021 +0300

    all: add the field into structs

commit 22b5b61
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue May 25 15:10:31 2021 +0300

    dnsforward: entitle lock, imp code

CHANGELOG.md

@@ -15,6 +15,8 @@ and this project adheres to
 
 ### Added
 
+- The ability to completely disable reverse DNS resolving of IPs from
+  locally-served networks ([#3184]).
 - New flag `--local-frontend` to serve dinamically changeable frontend files
   from disk as opposed to the ones that were compiled into the binary.
 
@@ -35,6 +37,8 @@ released by then.
 
 - Go 1.15 support.
 
+[#3184]: https://github.com/AdguardTeam/AdGuardHome/issues/3184
+
 
 
 ## [v0.106.3] - 2021-05-19

client/src/__locales/en.json

@@ -8,11 +8,13 @@
     "load_balancing_desc": "Query one upstream server at a time. AdGuard Home will use the weighted random algorithm to pick the server so that the fastest server is used more often.",
     "bootstrap_dns": "Bootstrap DNS servers",
     "bootstrap_dns_desc": "Bootstrap DNS servers are used to resolve IP addresses of the DoH/DoT resolvers you specify as upstreams.",
-    "local_ptr_title": "Private DNS servers",
+    "local_ptr_title": "Private reverse DNS servers",
     "local_ptr_desc": "The DNS servers that AdGuard Home uses for local PTR queries. These servers are used to resolve the hostnames of clients with private IP addresses, for example \"192.168.12.34\", using rDNS. If not set, AdGuard Home uses the default DNS resolvers of your OS.",
     "local_ptr_placeholder": "Enter one server address per line",
     "resolve_clients_title": "Enable reverse resolving of clients' IP addresses",
     "resolve_clients_desc": "If enabled, AdGuard Home will attempt to reversely resolve clients' IP addresses into their hostnames by sending PTR queries to corresponding resolvers (private DNS servers for local clients, upstream server for clients with public IP addresses).",
+    "use_private_ptr_resolvers_title": "Use private rDNS resolvers",
+    "use_private_ptr_resolvers_desc": "Perform reverse DNS lookups for locally-served addresses using these upstream servers. If disabled, AdGuard Home responds with NXDOMAIN to all such PTR requests except for clients known from DHCP, /etc/hosts, and so on.",
     "check_dhcp_servers": "Check for DHCP servers",
     "save_config": "Save configuration",
     "enabled_dhcp": "DHCP server enabled",

client/src/components/Settings/Dns/Upstream/Form.js

@@ -178,7 +178,7 @@ const Form = ({
                 <Examples />
                 <hr />
             </div>
-            <div className="col-12 mb-4">
+            <div className="col-12 mb-2">
                 <label
                     className="form__label form__label--with-desc"
                     htmlFor="bootstrap_dns"
@@ -202,7 +202,7 @@ const Form = ({
             <div className="col-12">
                 <hr />
             </div>
-            <div className="col-12 mb-4">
+            <div className="col-12">
                 <label
                     className="form__label form__label--with-desc"
                     htmlFor="local_ptr"
@@ -222,6 +222,19 @@ const Form = ({
                     disabled={processingSetConfig}
                     normalizeOnBlur={removeEmptyLines}
                 />
+                <div className="mt-4">
+                    <Field
+                        name="use_private_ptr_resolvers"
+                        type="checkbox"
+                        component={CheckboxField}
+                        placeholder={t('use_private_ptr_resolvers_title')}
+                        subtitle={t('use_private_ptr_resolvers_desc')}
+                        disabled={processingSetConfig}
+                    />
+                </div>
+            </div>
+            <div className="col-12">
+                <hr />
             </div>
             <div className="col-12 mb-4">
                 <Field

client/src/components/Settings/Dns/Upstream/index.js

@@ -14,6 +14,7 @@ const Upstream = () => {
         upstream_mode,
         resolve_clients,
         local_ptr_upstreams,
+        use_private_ptr_resolvers,
     } = useSelector((state) => state.dnsConfig, shallowEqual);
 
     const upstream_dns_file = useSelector((state) => state.dnsConfig.upstream_dns_file);
@@ -25,13 +26,15 @@ const Upstream = () => {
             upstream_mode,
             resolve_clients,
             local_ptr_upstreams,
+            use_private_ptr_resolvers,
         } = values;
 
         const dnsConfig = {
             bootstrap_dns,
             upstream_mode,
             resolve_clients,
             local_ptr_upstreams,
+            use_private_ptr_resolvers,
             ...(upstream_dns_file ? null : { upstream_dns }),
         };
 
@@ -53,6 +56,7 @@ const Upstream = () => {
                         upstream_mode,
                         resolve_clients,
                         local_ptr_upstreams,
+                        use_private_ptr_resolvers,
                     }}
                     onSubmit={handleSubmit}
                 />

internal/dnsforward/access.go

@@ -142,14 +142,19 @@ type accessListJSON struct {
 	BlockedHosts      []string `json:"blocked_hosts"`
 }
 
-func (s *Server) handleAccessList(w http.ResponseWriter, r *http.Request) {
-	s.RLock()
-	j := accessListJSON{
-		AllowedClients:    s.conf.AllowedClients,
-		DisallowedClients: s.conf.DisallowedClients,
-		BlockedHosts:      s.conf.BlockedHosts,
+func (s *Server) accessListJSON() (j accessListJSON) {
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
+	return accessListJSON{
+		AllowedClients:    aghstrings.CloneSlice(s.conf.AllowedClients),
+		DisallowedClients: aghstrings.CloneSlice(s.conf.DisallowedClients),
+		BlockedHosts:      aghstrings.CloneSlice(s.conf.BlockedHosts),
 	}
-	s.RUnlock()
+}
+
+func (s *Server) handleAccessList(w http.ResponseWriter, r *http.Request) {
+	j := s.accessListJSON()
 
 	w.Header().Set("Content-Type", "application/json")
 	err := json.NewEncoder(w).Encode(j)
@@ -200,14 +205,16 @@ func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	s.Lock()
+	defer log.Debug("Access: updated lists: %d, %d, %d",
+		len(j.AllowedClients), len(j.DisallowedClients), len(j.BlockedHosts))
+
+	defer s.conf.ConfigModified()
+
+	s.serverLock.Lock()
+	defer s.serverLock.Unlock()
+
 	s.conf.AllowedClients = j.AllowedClients
 	s.conf.DisallowedClients = j.DisallowedClients
 	s.conf.BlockedHosts = j.BlockedHosts
 	s.access = a
-	s.Unlock()
-	s.conf.ConfigModified()
-
-	log.Debug("Access: updated lists: %d, %d, %d",
-		len(j.AllowedClients), len(j.DisallowedClients), len(j.BlockedHosts))
 }

internal/dnsforward/config.go

@@ -153,6 +153,10 @@ type ServerConfig struct {
 	// ResolveClients signals if the RDNS should resolve clients' addresses.
 	ResolveClients bool
 
+	// UsePrivateRDNS defines if the PTR requests for unknown addresses from
+	// locally-served networks should be resolved via private PTR resolvers.
+	UsePrivateRDNS bool
+
 	// LocalPTRResolvers is a slice of addresses to be used as upstreams for
 	// resolving PTR queries for local addresses.
 	LocalPTRResolvers []string

internal/dnsforward/dns.go

@@ -414,20 +414,25 @@ func (s *Server) processLocalPTR(ctx *dnsContext) (rc resultCode) {
 		return resultCodeSuccess
 	}
 
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
+
 	if !s.subnetDetector.IsLocallyServedNetwork(ip) {
 		return resultCodeSuccess
 	}
 
-	err := s.localResolvers.Resolve(d)
-	if err != nil {
-		ctx.err = err
+	if s.conf.UsePrivateRDNS {
+		if err := s.localResolvers.Resolve(d); err != nil {
+			ctx.err = err
 
-		return resultCodeError
+			return resultCodeError
+		}
 	}
 
 	if d.Res == nil {
 		d.Res = s.genNXDomain(d.Req)
 
+		// Do not even put into query log.
 		return resultCodeFinish
 	}
 
@@ -443,24 +448,20 @@ func processFilteringBeforeRequest(ctx *dnsContext) (rc resultCode) {
 		return resultCodeSuccess // response is already set - nothing to do
 	}
 
-	s.RLock()
-	// Synchronize access to s.dnsFilter so it won't be suddenly uninitialized while in use.
-	// This could happen after proxy server has been stopped, but its workers are not yet exited.
-	//
-	// A better approach is for proxy.Stop() to wait until all its workers exit,
-	//  but this would require the Upstream interface to have Close() function
-	//  (to prevent from hanging while waiting for unresponsive DNS server to respond).
+	s.serverLock.RLock()
+	defer s.serverLock.RUnlock()
 
-	var err error
 	ctx.protectionEnabled = s.conf.ProtectionEnabled && s.dnsFilter != nil
-	if ctx.protectionEnabled {
-		if ctx.setts == nil {
-			ctx.setts = s.getClientRequestFilteringSettings(ctx)
-		}
-		ctx.result, err = s.filterDNSRequest(ctx)
+	if !ctx.protectionEnabled {
+		return resultCodeSuccess
 	}
-	s.RUnlock()
 
+	if ctx.setts == nil {
+		ctx.setts = s.getClientRequestFilteringSettings(ctx)
+	}
+
+	var err error
+	ctx.result, err = s.filterDNSRequest(ctx)
 	if err != nil {
 		ctx.err = err
 

internal/dnsforward/dns_test.go

@@ -260,7 +260,7 @@ func TestServer_ProcessInternalHosts(t *testing.T) {
 	}
 }
 
-func TestLocalRestriction(t *testing.T) {
+func TestServer_ProcessRestrictLocal(t *testing.T) {
 	ups := &aghtest.TestUpstream{
 		Reverse: map[string][]string{
 			"251.252.253.254.in-addr.arpa.": {"host1.example.net."},
@@ -318,14 +318,64 @@ func TestLocalRestriction(t *testing.T) {
 				IP: tc.cliIP,
 			},
 		}
+
 		t.Run(tc.name, func(t *testing.T) {
 			err = s.handleDNSRequest(nil, pctx)
 			require.NoError(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)
 			}
 		})
 	}
 }
+
+func TestServer_ProcessLocalPTR_usingResolvers(t *testing.T) {
+	const locDomain = "some.local."
+	const reqAddr = "1.1.168.192.in-addr.arpa."
+
+	s := createTestServer(t, &filtering.Config{}, ServerConfig{
+		UDPListenAddrs: []*net.UDPAddr{{}},
+		TCPListenAddrs: []*net.TCPAddr{{}},
+	}, &aghtest.TestUpstream{
+		Reverse: map[string][]string{
+			reqAddr: {locDomain},
+		},
+	})
+
+	var proxyCtx *proxy.DNSContext
+	var dnsCtx *dnsContext
+	setup := func(use bool) {
+		proxyCtx = &proxy.DNSContext{
+			Addr: &net.TCPAddr{
+				IP: net.IP{127, 0, 0, 1},
+			},
+			Req: createTestMessageWithType(reqAddr, dns.TypePTR),
+		}
+		dnsCtx = &dnsContext{
+			proxyCtx:        proxyCtx,
+			unreversedReqIP: net.IP{192, 168, 1, 1},
+		}
+		s.conf.UsePrivateRDNS = use
+	}
+
+	t.Run("enabled", func(t *testing.T) {
+		setup(true)
+
+		rc := s.processLocalPTR(dnsCtx)
+		require.Equal(t, resultCodeSuccess, rc)
+		require.NotEmpty(t, proxyCtx.Res.Answer)
+
+		assert.Equal(t, locDomain, proxyCtx.Res.Answer[0].Header().Name)
+	})
+
+	t.Run("disabled", func(t *testing.T) {
+		setup(false)
+
+		rc := s.processLocalPTR(dnsCtx)
+		require.Equal(t, resultCodeFinish, rc)
+		require.Empty(t, proxyCtx.Res.Answer)
+	})
+}