From 557bbcbf37a8f1d6092a51064e329ecc50bebb9d Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Wed, 26 May 2021 17:55:19 +0300 Subject: [PATCH] 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 b78ac2eeb1b408586808ddbd1c87107f373b11b0 Author: Eugene Burkov Date: Wed May 26 17:20:34 2021 +0300 all: rename dns config field commit 36512134822a5f6b8b296ccbd7e7d5a9b8e87f26 Author: Ildar Kamalov Date: Wed May 26 15:55:44 2021 +0300 client: handle local ips rdns commit 9a691830d45db93e078332d85bc0efa7dc7b6ac6 Author: Eugene Burkov Date: Wed May 26 14:43:13 2021 +0300 all: imp naming commit 771b7a3d5d25f91408dd97ba3287efb641028ccf Author: Eugene Burkov Date: Wed May 26 14:24:38 2021 +0300 all: imp docs, code commit be960893e8bbb7375a944ca0345b50c857a2d7cf Author: Eugene Burkov Date: Wed May 26 13:23:56 2021 +0300 all: imp docs & log changes commit 4e645a520f6bb584ef951435ee833ad30769af98 Author: Eugene Burkov Date: Wed May 26 12:49:44 2021 +0300 all: add the field into structs commit 22b5b6163f086560a3189234532ba877be7ba940 Author: Eugene Burkov Date: Tue May 25 15:10:31 2021 +0300 dnsforward: entitle lock, imp code --- CHANGELOG.md | 4 + client/src/__locales/en.json | 4 +- .../components/Settings/Dns/Upstream/Form.js | 17 ++- .../components/Settings/Dns/Upstream/index.js | 4 + internal/dnsforward/access.go | 33 +++--- internal/dnsforward/config.go | 4 + internal/dnsforward/dns.go | 41 +++---- internal/dnsforward/dns_test.go | 52 ++++++++- internal/dnsforward/dnsforward.go | 109 +++++++++++------- internal/dnsforward/dnsforward_test.go | 27 ++--- internal/dnsforward/filter.go | 45 +++++--- internal/dnsforward/http.go | 15 ++- internal/dnsforward/stats.go | 5 +- .../TestDNSForwardHTTTP_handleGetConfig.json | 3 + .../TestDNSForwardHTTTP_handleSetConfig.json | 16 +++ internal/home/clients.go | 2 + internal/home/config.go | 13 ++- internal/home/dns.go | 3 +- internal/home/rdns.go | 46 +++++++- internal/home/rdns_test.go | 40 ++++++- openapi/CHANGELOG.md | 9 +- openapi/openapi.yaml | 2 + 22 files changed, 372 insertions(+), 122 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 358bfa94..d784fb76 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/client/src/__locales/en.json b/client/src/__locales/en.json index e9599b9d..b50675f5 100644 --- a/client/src/__locales/en.json +++ b/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", diff --git a/client/src/components/Settings/Dns/Upstream/Form.js b/client/src/components/Settings/Dns/Upstream/Form.js index 33b746e3..99364549 100644 --- a/client/src/components/Settings/Dns/Upstream/Form.js +++ b/client/src/components/Settings/Dns/Upstream/Form.js @@ -178,7 +178,7 @@ const Form = ({
-
+
-
+
+
+
{ 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,6 +26,7 @@ const Upstream = () => { upstream_mode, resolve_clients, local_ptr_upstreams, + use_private_ptr_resolvers, } = values; const dnsConfig = { @@ -32,6 +34,7 @@ const Upstream = () => { 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} /> diff --git a/internal/dnsforward/access.go b/internal/dnsforward/access.go index f27eb667..62c1e35b 100644 --- a/internal/dnsforward/access.go +++ b/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)) } diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go index 03710238..0fea1bd9 100644 --- a/internal/dnsforward/config.go +++ b/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 diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index 5fbd4070..364d447a 100644 --- a/internal/dnsforward/dns.go +++ b/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() + + ctx.protectionEnabled = s.conf.ProtectionEnabled && s.dnsFilter != nil + if !ctx.protectionEnabled { + return resultCodeSuccess + } + + if ctx.setts == nil { + ctx.setts = s.getClientRequestFilteringSettings(ctx) + } 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) - } - s.RUnlock() - + ctx.result, err = s.filterDNSRequest(ctx) if err != nil { ctx.err = err diff --git a/internal/dnsforward/dns_test.go b/internal/dnsforward/dns_test.go index 7e016909..06a3c0e1 100644 --- a/internal/dnsforward/dns_test.go +++ b/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) + }) +} diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go index cf31500c..15ddbc57 100644 --- a/internal/dnsforward/dnsforward.go +++ b/internal/dnsforward/dnsforward.go @@ -89,8 +89,9 @@ type Server struct { isRunning bool - sync.RWMutex conf ServerConfig + // serverLock protects Server. + serverLock sync.RWMutex } // defaultLocalDomainSuffix is the default suffix used to detect internal hosts @@ -167,25 +168,31 @@ func NewCustomServer(internalProxy *proxy.Proxy) *Server { return s } -// Close - close object +// Close gracefully closes the server. It is safe for concurrent use. +// +// TODO(e.burkov): A better approach would be making Stop method waiting for all +// its workers finished. But it would require the upstream.Upstream to have the +// Close method to prevent from hanging while waiting for unresponsive server to +// respond. func (s *Server) Close() { - s.Lock() + s.serverLock.Lock() + defer s.serverLock.Unlock() + s.dnsFilter = nil s.stats = nil s.queryLog = nil s.dnsProxy = nil - err := s.ipset.Close() - if err != nil { + if err := s.ipset.Close(); err != nil { log.Error("closing ipset: %s", err) } - - s.Unlock() } // WriteDiskConfig - write configuration func (s *Server) WriteDiskConfig(c *FilteringConfig) { - s.RLock() + s.serverLock.RLock() + defer s.serverLock.RUnlock() + sc := s.conf.FilteringConfig *c = sc c.RatelimitWhitelist = aghstrings.CloneSlice(sc.RatelimitWhitelist) @@ -194,15 +201,16 @@ func (s *Server) WriteDiskConfig(c *FilteringConfig) { c.DisallowedClients = aghstrings.CloneSlice(sc.DisallowedClients) c.BlockedHosts = aghstrings.CloneSlice(sc.BlockedHosts) c.UpstreamDNS = aghstrings.CloneSlice(sc.UpstreamDNS) - s.RUnlock() } // RDNSSettings returns the copy of actual RDNS configuration. -func (s *Server) RDNSSettings() (localPTRResolvers []string, resolveClients bool) { - s.RLock() - defer s.RUnlock() +func (s *Server) RDNSSettings() (localPTRResolvers []string, resolveClients, resolvePTR bool) { + s.serverLock.RLock() + defer s.serverLock.RUnlock() - return aghstrings.CloneSlice(s.conf.LocalPTRResolvers), s.conf.ResolveClients + return aghstrings.CloneSlice(s.conf.LocalPTRResolvers), + s.conf.ResolveClients, + s.conf.UsePrivateRDNS } // Resolve - get IP addresses by host name from an upstream server. @@ -210,8 +218,9 @@ func (s *Server) RDNSSettings() (localPTRResolvers []string, resolveClients bool // Query log and Stats are not updated. // This method may be called before Start(). func (s *Server) Resolve(host string) ([]net.IPAddr, error) { - s.RLock() - defer s.RUnlock() + s.serverLock.RLock() + defer s.serverLock.RUnlock() + return s.internalProxy.LookupIPAddr(host) } @@ -220,6 +229,9 @@ type RDNSExchanger interface { // Exchange tries to resolve the ip in a suitable way, e.g. either as // local or as external. Exchange(ip net.IP) (host string, err error) + // ResolvesPrivatePTR returns true if the RDNSExchanger is able to + // resolve PTR requests for locally-served addresses. + ResolvesPrivatePTR() (ok bool) } const ( @@ -234,13 +246,20 @@ const ( // Exchange implements the RDNSExchanger interface for *Server. func (s *Server) Exchange(ip net.IP) (host string, err error) { - s.RLock() - defer s.RUnlock() + s.serverLock.RLock() + defer s.serverLock.RUnlock() if !s.conf.ResolveClients { return "", nil } + var resolver *proxy.Proxy = s.localResolvers + if !s.subnetDetector.IsLocallyServedNetwork(ip) { + resolver = s.internalProxy + } else if !s.conf.UsePrivateRDNS { + return "", nil + } + arpa := dns.Fqdn(aghnet.ReverseAddr(ip)) req := &dns.Msg{ MsgHdr: dns.MsgHdr{ @@ -259,13 +278,8 @@ func (s *Server) Exchange(ip net.IP) (host string, err error) { Req: req, StartTime: time.Now(), } - var resp *dns.Msg - if s.subnetDetector.IsLocallyServedNetwork(ip) { - err = s.localResolvers.Resolve(ctx) - } else { - err = s.internalProxy.Resolve(ctx) - } + err = resolver.Resolve(ctx) if err != nil { return "", err } @@ -284,10 +298,19 @@ func (s *Server) Exchange(ip net.IP) (host string, err error) { return strings.TrimSuffix(ptr.Ptr, "."), nil } +// ResolvesPrivatePTR implements the RDNSExchanger interface for *Server. +func (s *Server) ResolvesPrivatePTR() (ok bool) { + s.serverLock.RLock() + defer s.serverLock.RUnlock() + + return s.conf.UsePrivateRDNS +} + // Start starts the DNS server. func (s *Server) Start() error { - s.Lock() - defer s.Unlock() + s.serverLock.Lock() + defer s.serverLock.Unlock() + return s.startLocked() } @@ -306,7 +329,7 @@ func (s *Server) startLocked() error { const defaultLocalTimeout = 1 * time.Second // collectDNSIPAddrs returns IP addresses the server is listening on without -// port numbers as a map. For internal use only. +// 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 @@ -472,8 +495,9 @@ func (s *Server) Prepare(config *ServerConfig) error { // Stop stops the DNS server. func (s *Server) Stop() error { - s.Lock() - defer s.Unlock() + s.serverLock.Lock() + defer s.serverLock.Unlock() + return s.stopLocked() } @@ -490,17 +514,18 @@ func (s *Server) stopLocked() error { return nil } -// IsRunning returns true if the DNS server is running +// IsRunning returns true if the DNS server is running. func (s *Server) IsRunning() bool { - s.RLock() - defer s.RUnlock() + s.serverLock.RLock() + defer s.serverLock.RUnlock() + return s.isRunning } -// Reconfigure applies the new configuration to the DNS server +// Reconfigure applies the new configuration to the DNS server. func (s *Server) Reconfigure(config *ServerConfig) error { - s.Lock() - defer s.Unlock() + s.serverLock.Lock() + defer s.serverLock.Unlock() log.Print("Start reconfiguring the server") err := s.stopLocked() @@ -525,12 +550,18 @@ func (s *Server) Reconfigure(config *ServerConfig) error { return nil } -// ServeHTTP is a HTTP handler method we use to provide DNS-over-HTTPS +// ServeHTTP is a HTTP handler method we use to provide DNS-over-HTTPS. func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { - s.RLock() - p := s.dnsProxy - s.RUnlock() - if p != nil { // an attempt to protect against race in case we're here after Close() was called + var p *proxy.Proxy + + func() { + s.serverLock.RLock() + defer s.serverLock.RUnlock() + + p = s.dnsProxy + }() + + if p != nil { p.ServeHTTP(w, r) } } diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go index 8cf48ff4..6ced4805 100644 --- a/internal/dnsforward/dnsforward_test.go +++ b/internal/dnsforward/dnsforward_test.go @@ -86,11 +86,12 @@ func createTestServer( err = s.Prepare(nil) require.NoError(t, err) - s.Lock() - defer s.Unlock() + s.serverLock.Lock() + defer s.serverLock.Unlock() if localUps != nil { s.localResolvers.Config.UpstreamConfig.Upstreams = []upstream.Upstream{localUps} + s.conf.UsePrivateRDNS = true } return s @@ -1207,17 +1208,18 @@ func TestServer_Exchange(t *testing.T) { Block: true, } - dns := NewCustomServer(&proxy.Proxy{ + srv := NewCustomServer(&proxy.Proxy{ Config: proxy.Config{ UpstreamConfig: &proxy.UpstreamConfig{ Upstreams: []upstream.Upstream{extUpstream}, }, }, }) - dns.conf.ResolveClients = true + srv.conf.ResolveClients = true + srv.conf.UsePrivateRDNS = true var err error - dns.subnetDetector, err = aghnet.NewSubnetDetector() + srv.subnetDetector, err = aghnet.NewSubnetDetector() require.NoError(t, err) localIP := net.IP{192, 168, 1, 1} @@ -1265,12 +1267,12 @@ func TestServer_Exchange(t *testing.T) { Upstreams: []upstream.Upstream{tc.locUpstream}, }, } - dns.localResolvers = &proxy.Proxy{ + srv.localResolvers = &proxy.Proxy{ Config: pcfg, } t.Run(tc.name, func(t *testing.T) { - host, eerr := dns.Exchange(tc.req) + host, eerr := srv.Exchange(tc.req) require.ErrorIs(t, eerr, tc.wantErr) assert.Equal(t, tc.want, host) @@ -1278,12 +1280,11 @@ func TestServer_Exchange(t *testing.T) { } t.Run("resolving_disabled", func(t *testing.T) { - dns.conf.ResolveClients = false - for _, tc := range testCases { - host, eerr := dns.Exchange(tc.req) + srv.conf.UsePrivateRDNS = false - require.NoError(t, eerr) - assert.Empty(t, host) - } + host, eerr := srv.Exchange(localIP) + + require.NoError(t, eerr) + assert.Empty(t, host) }) } diff --git a/internal/dnsforward/filter.go b/internal/dnsforward/filter.go index 1bacc98c..3dd3530d 100644 --- a/internal/dnsforward/filter.go +++ b/internal/dnsforward/filter.go @@ -117,8 +117,31 @@ func (s *Server) filterDNSRequest(ctx *dnsContext) (*filtering.Result, error) { return &res, err } -// If response contains CNAME, A or AAAA records, we apply filtering to each canonical host name or IP address. -// If this is a match, we set a new response in d.Res and return. +// checkHostRules checks the host against filters. It is safe for concurrent +// use. +func (s *Server) checkHostRules(host string, qtype uint16, setts *filtering.Settings) ( + r *filtering.Result, + err error, +) { + s.serverLock.RLock() + defer s.serverLock.RUnlock() + + if s.dnsFilter == nil { + return nil, nil + } + + var res filtering.Result + res, err = s.dnsFilter.CheckHostRules(host, qtype, setts) + if err != nil { + return nil, err + } + + return &res, err +} + +// If response contains CNAME, A or AAAA records, we apply filtering to each +// canonical host name or IP address. If this is a match, we set a new response +// in d.Res and return. func (s *Server) filterDNSResponse(ctx *dnsContext) (*filtering.Result, error) { d := ctx.proxyCtx for _, a := range d.Res.Answer { @@ -141,22 +164,16 @@ func (s *Server) filterDNSResponse(ctx *dnsContext) (*filtering.Result, error) { continue } - 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. - if !s.conf.ProtectionEnabled || s.dnsFilter == nil { - s.RUnlock() - continue - } - res, err := s.dnsFilter.CheckHostRules(host, d.Req.Question[0].Qtype, ctx.setts) - s.RUnlock() - + res, err := s.checkHostRules(host, d.Req.Question[0].Qtype, ctx.setts) if err != nil { return nil, err + } else if res == nil { + continue } else if res.IsFiltered { - d.Res = s.genDNSFilterMessage(d, &res) + d.Res = s.genDNSFilterMessage(d, res) log.Debug("DNSFwd: Matched %s by response: %s", d.Req.Question[0].Name, host) - return &res, nil + + return res, nil } } diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index 35c1d9d4..8ec9313d 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -41,12 +41,13 @@ type dnsConfig struct { CacheMinTTL *uint32 `json:"cache_ttl_min"` CacheMaxTTL *uint32 `json:"cache_ttl_max"` ResolveClients *bool `json:"resolve_clients"` + UsePrivateRDNS *bool `json:"use_private_ptr_resolvers"` LocalPTRUpstreams *[]string `json:"local_ptr_upstreams"` } func (s *Server) getDNSConfig() dnsConfig { - s.RLock() - defer s.RUnlock() + s.serverLock.RLock() + defer s.serverLock.RUnlock() upstreams := aghstrings.CloneSliceOrEmpty(s.conf.UpstreamDNS) upstreamFile := s.conf.UpstreamDNSFileName @@ -63,6 +64,7 @@ func (s *Server) getDNSConfig() dnsConfig { cacheMinTTL := s.conf.CacheMinTTL cacheMaxTTL := s.conf.CacheMaxTTL resolveClients := s.conf.ResolveClients + usePrivateRDNS := s.conf.UsePrivateRDNS localPTRUpstreams := aghstrings.CloneSliceOrEmpty(s.conf.LocalPTRResolvers) var upstreamMode string if s.conf.FastestAddr { @@ -88,6 +90,7 @@ func (s *Server) getDNSConfig() dnsConfig { CacheMaxTTL: &cacheMaxTTL, UpstreamMode: &upstreamMode, ResolveClients: &resolveClients, + UsePrivateRDNS: &usePrivateRDNS, LocalPTRUpstreams: &localPTRUpstreams, } } @@ -280,8 +283,8 @@ func (s *Server) setConfigRestartable(dc dnsConfig) (restart bool) { } func (s *Server) setConfig(dc dnsConfig) (restart bool) { - s.Lock() - defer s.Unlock() + s.serverLock.Lock() + defer s.serverLock.Unlock() if dc.ProtectionEnabled != nil { s.conf.ProtectionEnabled = *dc.ProtectionEnabled @@ -312,6 +315,10 @@ func (s *Server) setConfig(dc dnsConfig) (restart bool) { s.conf.ResolveClients = *dc.ResolveClients } + if dc.UsePrivateRDNS != nil { + s.conf.UsePrivateRDNS = *dc.UsePrivateRDNS + } + return s.setConfigRestartable(dc) } diff --git a/internal/dnsforward/stats.go b/internal/dnsforward/stats.go index f03f6f64..d0eb39ca 100644 --- a/internal/dnsforward/stats.go +++ b/internal/dnsforward/stats.go @@ -25,7 +25,9 @@ func processQueryLogsAndStats(ctx *dnsContext) (rc resultCode) { shouldLog = false } - s.RLock() + s.serverLock.RLock() + defer s.serverLock.RUnlock() + // Synchronize access to s.queryLog and s.stats so they won't be suddenly uninitialized while in use. // This can happen after proxy server has been stopped, but its workers haven't yet exited. if shouldLog && s.queryLog != nil { @@ -61,7 +63,6 @@ func processQueryLogsAndStats(ctx *dnsContext) (rc resultCode) { } s.updateStats(ctx, elapsed, *ctx.result) - s.RUnlock() return resultCodeSuccess } diff --git a/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleGetConfig.json b/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleGetConfig.json index 562f0fcc..4d810c94 100644 --- a/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleGetConfig.json +++ b/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleGetConfig.json @@ -24,6 +24,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] }, "fastest_addr": { @@ -51,6 +52,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] }, "parallel": { @@ -78,6 +80,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } } \ No newline at end of file diff --git a/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json b/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json index e7f98c6e..2d2e34b5 100644 --- a/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json +++ b/internal/dnsforward/testdata/TestDNSForwardHTTTP_handleSetConfig.json @@ -31,6 +31,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -62,6 +63,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -94,6 +96,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -126,6 +129,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -158,6 +162,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -190,6 +195,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -222,6 +228,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -254,6 +261,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -286,6 +294,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -318,6 +327,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -352,6 +362,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -386,6 +397,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -419,6 +431,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -451,6 +464,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } }, @@ -485,6 +499,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [ "123.123.123.123" ] @@ -519,6 +534,7 @@ "cache_ttl_min": 0, "cache_ttl_max": 0, "resolve_clients": false, + "use_private_ptr_resolvers": false, "local_ptr_upstreams": [] } } diff --git a/internal/home/clients.go b/internal/home/clients.go index 00caa34e..5ccb73ff 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -53,6 +53,8 @@ type Client struct { type clientSource uint // Client sources. The order determines the priority. +// +// TODO(e.burkov): Is ARP a higher priority source than DHCP? const ( ClientSourceWHOIS clientSource = iota ClientSourceRDNS diff --git a/internal/home/config.go b/internal/home/config.go index 7071e364..53b0e303 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -108,6 +108,10 @@ type dnsConfig struct { // ResolveClients enables and disables resolving clients with RDNS. ResolveClients bool `yaml:"resolve_clients"` + // UsePrivateRDNS defines if the PTR requests for unknown addresses from + // locally-served networks should be resolved via private PTR resolvers. + UsePrivateRDNS bool `yaml:"use_private_ptr_resolvers"` + // LocalPTRResolvers is the slice of addresses to be used as upstreams // for PTR queries for locally-served networks. LocalPTRResolvers []string `yaml:"local_ptr_upstreams"` @@ -166,6 +170,7 @@ var config = configuration{ FiltersUpdateIntervalHours: 24, LocalDomainName: "lan", ResolveClients: true, + UsePrivateRDNS: true, }, TLS: tlsConfigSettings{ PortHTTPS: 443, @@ -314,9 +319,11 @@ func (c *configuration) write() error { if s := Context.dnsServer; s != nil { c := dnsforward.FilteringConfig{} s.WriteDiskConfig(&c) - config.DNS.FilteringConfig = c - - config.DNS.LocalPTRResolvers, config.DNS.ResolveClients = s.RDNSSettings() + dns := &config.DNS + dns.FilteringConfig = c + dns.LocalPTRResolvers, + dns.ResolveClients, + dns.UsePrivateRDNS = s.RDNSSettings() } if Context.dhcpServer != nil { diff --git a/internal/home/dns.go b/internal/home/dns.go index 809bb011..82ed4fb2 100644 --- a/internal/home/dns.go +++ b/internal/home/dns.go @@ -94,7 +94,7 @@ func initDNSServer() error { return fmt.Errorf("dnsServer.Prepare: %w", err) } - Context.rdns = NewRDNS(Context.dnsServer, &Context.clients) + Context.rdns = NewRDNS(Context.dnsServer, &Context.clients, config.DNS.UsePrivateRDNS) Context.whois = initWhois(&Context.clients) Context.filters.Init() @@ -200,6 +200,7 @@ func generateServerConfig() (newConf dnsforward.ServerConfig, err error) { newConf.GetCustomUpstreamByClient = Context.clients.FindUpstreams newConf.ResolveClients = dnsConf.ResolveClients + newConf.UsePrivateRDNS = dnsConf.UsePrivateRDNS newConf.LocalPTRResolvers = dnsConf.LocalPTRResolvers return newConf, nil diff --git a/internal/home/rdns.go b/internal/home/rdns.go index 81d7ca5a..d19af7c8 100644 --- a/internal/home/rdns.go +++ b/internal/home/rdns.go @@ -3,6 +3,7 @@ package home import ( "encoding/binary" "net" + "sync/atomic" "time" "github.com/AdguardTeam/AdGuardHome/internal/dnsforward" @@ -15,6 +16,10 @@ type RDNS struct { exchanger dnsforward.RDNSExchanger clients *clientsContainer + // usePrivate is used to store the state of current private RDNS + // resolving settings and to react to it's changes. + usePrivate uint32 + // ipCh used to pass client's IP to rDNS workerLoop. ipCh chan net.IP @@ -37,6 +42,7 @@ const ( func NewRDNS( exchanger dnsforward.RDNSExchanger, clients *clientsContainer, + usePrivate bool, ) (rDNS *RDNS) { rDNS = &RDNS{ exchanger: exchanger, @@ -47,19 +53,39 @@ func NewRDNS( }), ipCh: make(chan net.IP, defaultRDNSIPChSize), } + if usePrivate { + rDNS.usePrivate = 1 + } go rDNS.workerLoop() return rDNS } -// Begin adds the ip to the resolving queue if it is not cached or already -// resolved. -func (r *RDNS) Begin(ip net.IP) { +// ensurePrivateCache ensures that the state of the RDNS cache is consistent +// with the current private client RDNS resolving settings. +// +// TODO(e.burkov): Clearing cache each time this value changed is not a perfect +// approach since only unresolved locally-served addresses should be removed. +// Implement when improving the cache. +func (r *RDNS) ensurePrivateCache() { + var usePrivate uint32 + if r.exchanger.ResolvesPrivatePTR() { + usePrivate = 1 + } + + if atomic.CompareAndSwapUint32(&r.usePrivate, 1-usePrivate, usePrivate) { + r.ipCache.Clear() + } +} + +// isCached returns true if ip is already cached and not expired yet. It also +// caches it otherwise. +func (r *RDNS) isCached(ip net.IP) (ok bool) { now := uint64(time.Now().Unix()) if expire := r.ipCache.Get(ip); len(expire) != 0 { if binary.BigEndian.Uint64(expire) > now { - return + return true } } @@ -68,6 +94,18 @@ func (r *RDNS) Begin(ip net.IP) { binary.BigEndian.PutUint64(ttl, now+defaultRDNSCacheTTL) r.ipCache.Set(ip, ttl) + return false +} + +// Begin adds the ip to the resolving queue if it is not cached or already +// resolved. +func (r *RDNS) Begin(ip net.IP) { + r.ensurePrivateCache() + + if r.isCached(ip) { + return + } + id := ip.String() if r.clients.Exists(id, ClientSourceRDNS) { return diff --git a/internal/home/rdns_test.go b/internal/home/rdns_test.go index 329e0bf4..3655b6b4 100644 --- a/internal/home/rdns_test.go +++ b/internal/home/rdns_test.go @@ -16,6 +16,7 @@ import ( "github.com/AdguardTeam/golibs/log" "github.com/miekg/dns" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRDNS_Begin(t *testing.T) { @@ -78,7 +79,8 @@ func TestRDNS_Begin(t *testing.T) { binary.BigEndian.PutUint64(ttl, uint64(time.Now().Add(100*time.Hour).Unix())) rdns := &RDNS{ - ipCache: ipCache, + ipCache: ipCache, + exchanger: &rDNSExchanger{}, clients: &clientsContainer{ list: map[string]*Client{}, idIndex: tc.cliIDIndex, @@ -105,7 +107,8 @@ func TestRDNS_Begin(t *testing.T) { // rDNSExchanger is a mock dnsforward.RDNSExchanger implementation for tests. type rDNSExchanger struct { - aghtest.Exchanger + ex aghtest.Exchanger + usePrivate bool } // Exchange implements dnsforward.RDNSExchanger interface for *RDNSExchanger. @@ -117,7 +120,7 @@ func (e *rDNSExchanger) Exchange(ip net.IP) (host string, err error) { }}, } - resp, err := e.Exchanger.Exchange(req) + resp, err := e.ex.Exchange(req) if err != nil { return "", err } @@ -129,6 +132,35 @@ func (e *rDNSExchanger) Exchange(ip net.IP) (host string, err error) { return resp.Answer[0].Header().Name, nil } +// Exchange implements dnsforward.RDNSExchanger interface for *RDNSExchanger. +func (e *rDNSExchanger) ResolvesPrivatePTR() (ok bool) { + return e.usePrivate +} + +func TestRDNS_ensurePrivateCache(t *testing.T) { + data := []byte{1, 2, 3, 4} + + ipCache := cache.New(cache.Config{ + EnableLRU: true, + MaxCount: defaultRDNSCacheSize, + }) + + ex := &rDNSExchanger{} + + rdns := &RDNS{ + ipCache: ipCache, + exchanger: ex, + } + + rdns.ipCache.Set(data, data) + require.NotZero(t, rdns.ipCache.Stats().Count) + + ex.usePrivate = !ex.usePrivate + + rdns.ensurePrivateCache() + require.Zero(t, rdns.ipCache.Stats().Count) +} + func TestRDNS_WorkerLoop(t *testing.T) { aghtest.ReplaceLogLevel(t, log.DEBUG) w := &bytes.Buffer{} @@ -178,7 +210,7 @@ func TestRDNS_WorkerLoop(t *testing.T) { ch := make(chan net.IP) rdns := &RDNS{ exchanger: &rDNSExchanger{ - Exchanger: aghtest.Exchanger{ + ex: aghtest.Exchanger{ Ups: tc.ups, }, }, diff --git a/openapi/CHANGELOG.md b/openapi/CHANGELOG.md index 0fd19bb8..edccb1ee 100644 --- a/openapi/CHANGELOG.md +++ b/openapi/CHANGELOG.md @@ -2,11 +2,18 @@ +## v0.107: API changes + +### The field `"use_private_ptr_resolvers"` in DNS configuration + +* The new optional field `"use_private_ptr_resolvers"` of `"DNSConfig"` + specifies if the DNS server should use `"local_ptr_upstreams"` at all. + ## v0.106: API changes ### The field `"supported_tags"` in `GET /control/clients` -* Prefiously undocumented field `"supported_tags"` in the response is now +* Previously undocumented field `"supported_tags"` in the response is now documented. ### The field `"whois_info"` in `GET /control/clients` diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index 187d764e..917f1c95 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -1300,6 +1300,8 @@ - '' - 'parallel' - 'fastest_addr' + 'use_private_ptr_resolvers': + 'type': 'boolean' 'resolve_clients': 'type': 'boolean' 'local_ptr_upstreams':