Pull request: 4865-dhcp-rewrites

Updates #4865.

Squashed commit of the following:

commit b26575b72299126f2ce7535104800cc6750698f3
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Sep 2 16:47:25 2022 +0300

    dnsforward: imp code, docs, logs

commit c60942c1432175866ac1d182709de33429534de0
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Sep 2 16:24:44 2022 +0300

    dnsforward: process unknown queries in dhcp domain

CHANGELOG.md

@@ -32,6 +32,9 @@ and this project adheres to
 
 ### Changed
 
+- When the DHCP server is enabled, queries for domain names under
+  `dhcp.local_domain_name` not pointing to real DHCP client hostnames are now
+  processed by filters ([#4865]).
 - The DHCPREQUEST handling is now closer to the [RFC 2131][rfc-2131] ([#4863]).
 - The internal DNS client, used to resolve hostnames of external clients and
   also during automatic updates, now respects the upstream mode settings for the
@@ -57,6 +60,7 @@ and this project adheres to
 [#4745]: https://github.com/AdguardTeam/AdGuardHome/issues/4745
 [#4850]: https://github.com/AdguardTeam/AdGuardHome/issues/4850
 [#4863]: https://github.com/AdguardTeam/AdGuardHome/issues/4863
+[#4865]: https://github.com/AdguardTeam/AdGuardHome/issues/4865
 
 [rfc-2131]: https://datatracker.ietf.org/doc/html/rfc2131
 

internal/dnsforward/config.go

@@ -381,7 +381,7 @@ func (s *Server) prepareUpstreamSettings() error {
 }
 
 // prepareInternalProxy initializes the DNS proxy that is used for internal DNS
-// queries, such at client PTR resolving and udpater hostname resolving.
+// queries, such at client PTR resolving and updater hostname resolving.
 func (s *Server) prepareInternalProxy() {
 	conf := &proxy.Config{
 		CacheEnabled:   true,

internal/dnsforward/dns.go

@@ -250,11 +250,11 @@ func (s *Server) onDHCPLeaseChanged(flags int) {
 	s.setTableIPToHost(ipToHost)
 }
 
-// processDDRQuery responds to SVCB query for a special use domain name
+// processDDRQuery responds to Discovery of Designated Resolvers (DDR) SVCB
-// ‘_dns.resolver.arpa’.  The result contains different types of encryption
+// queries.  The response contains different types of encryption supported by
-// supported by current user configuration.
+// current user configuration.
 //
-// See https://www.ietf.org/archive/id/draft-ietf-add-ddr-06.html.
+// See https://www.ietf.org/archive/id/draft-ietf-add-ddr-10.html.
 func (s *Server) processDDRQuery(dctx *dnsContext) (rc resultCode) {
 	pctx := dctx.proxyCtx
 	q := pctx.Req.Question[0]
@@ -279,11 +279,13 @@ func (s *Server) processDDRQuery(dctx *dnsContext) (rc resultCode) {
 	return resultCodeSuccess
 }
 
-// makeDDRResponse creates DDR answer according to server configuration.  The
+// makeDDRResponse creates a DDR answer based on the server configuration.  The
-// contructed SVCB resource records have the priority of 1 for each entry,
+// constructed SVCB resource records have the priority of 1 for each entry,
-// similar to examples provided by https://www.ietf.org/archive/id/draft-ietf-add-ddr-06.html.
+// similar to examples provided by the [draft standard].
 //
 // TODO(a.meshkov):  Consider setting the priority values based on the protocol.
+//
+// [draft standard]: https://www.ietf.org/archive/id/draft-ietf-add-ddr-10.html.
 func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) {
 	resp = s.makeResponse(req)
 	// TODO(e.burkov):  Think about storing the FQDN version of the server's
@@ -357,10 +359,10 @@ func (s *Server) processDetermineLocal(dctx *dnsContext) (rc resultCode) {
 	return rc
 }
 
-// hostToIP tries to get an IP leased by DHCP and returns the copy of address
+// dhcpHostToIP tries to get an IP leased by DHCP and returns the copy of
-// since the data inside the internal table may be changed while request
+// 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) {
+func (s *Server) dhcpHostToIP(host string) (ip net.IP, ok bool) {
 	s.tableHostToIPLock.Lock()
 	defer s.tableHostToIPLock.Unlock()
 
@@ -385,46 +387,32 @@ func (s *Server) hostToIP(host string) (ip net.IP, ok bool) {
 //
 // TODO(a.garipov): Adapt to AAAA as well.
 func (s *Server) processDHCPHosts(dctx *dnsContext) (rc resultCode) {
-	if !s.dhcpServer.Enabled() {
-		return resultCodeSuccess
-	}
-
-	req := dctx.proxyCtx.Req
-	q := req.Question[0]
-
-	// Go on processing the AAAA request despite the fact that we don't support
-	// it yet.  The expected behavior here is to respond with an empty answer
-	// and not NXDOMAIN.
-	if q.Qtype != dns.TypeA && q.Qtype != dns.TypeAAAA {
-		return resultCodeSuccess
-	}
-
-	reqHost := strings.ToLower(q.Name[:len(q.Name)-1])
-	// TODO(a.garipov): Move everything related to DHCP local domain to the DHCP
-	// server.
-	if !strings.HasSuffix(reqHost, s.localDomainSuffix) {
-		return resultCodeSuccess
-	}
-
 	pctx := dctx.proxyCtx
+	req := pctx.Req
+	q := req.Question[0]
+	reqHost, ok := s.isDHCPClientHostQ(q)
+	if !ok {
+		return resultCodeSuccess
+	}
+
 	if !dctx.isLocalClient {
-		log.Debug("dns: %q requests for internal host", pctx.Addr)
+		log.Debug("dns: %q requests for dhcp host %q", pctx.Addr, reqHost)
 		pctx.Res = s.genNXDomain(req)
 
 		// Do not even put into query log.
 		return resultCodeFinish
 	}
 
-	ip, ok := s.hostToIP(reqHost)
+	ip, ok := s.dhcpHostToIP(reqHost)
 	if !ok {
-		// TODO(e.burkov): Inspect special cases when user want to apply some
+		// Go on and process them with filters, including dnsrewrite ones, and
-		// rules handled by other processors to the hosts with TLD.
+		// possibly route them to a domain-specific upstream.
-		pctx.Res = s.genNXDomain(req)
+		log.Debug("dns: no dhcp record for %q", reqHost)
 
-		return resultCodeFinish
+		return resultCodeSuccess
 	}
 
-	log.Debug("dns: internal record: %s -> %s", q.Name, ip)
+	log.Debug("dns: dhcp record for %q is %s", reqHost, ip)
 
 	resp := s.makeResponse(req)
 	if q.Qtype == dns.TypeA {
@@ -502,9 +490,9 @@ func (s *Server) processRestrictLocal(dctx *dnsContext) (rc resultCode) {
 	return resultCodeSuccess
 }
 
-// ipToHost tries to get a hostname leased by DHCP.  It's safe for concurrent
+// ipToDHCPHost tries to get a hostname leased by DHCP.  It's safe for
-// use.
+// concurrent use.
-func (s *Server) ipToHost(ip net.IP) (host string, ok bool) {
+func (s *Server) ipToDHCPHost(ip net.IP) (host string, ok bool) {
 	s.tableIPToHostLock.Lock()
 	defer s.tableIPToHostLock.Unlock()
 
@@ -527,8 +515,8 @@ func (s *Server) ipToHost(ip net.IP) (host string, ok bool) {
 	return host, true
 }
 
-// Respond to PTR requests if the target IP is leased by our DHCP server and the
+// processDHCPAddrs responds to PTR requests if the target IP is leased by the
-// requestor is inside the local network.
+// DHCP server.
 func (s *Server) processDHCPAddrs(dctx *dnsContext) (rc resultCode) {
 	pctx := dctx.proxyCtx
 	if pctx.Res != nil {
@@ -540,12 +528,12 @@ func (s *Server) processDHCPAddrs(dctx *dnsContext) (rc resultCode) {
 		return resultCodeSuccess
 	}
 
-	host, ok := s.ipToHost(ip)
+	host, ok := s.ipToDHCPHost(ip)
 	if !ok {
 		return resultCodeSuccess
 	}
 
-	log.Debug("dns: reverse-lookup: %s -> %s", ip, host)
+	log.Debug("dns: dhcp reverse record for %s is %q", ip, host)
 
 	req := pctx.Req
 	resp := s.makeResponse(req)
@@ -639,14 +627,25 @@ func ipStringFromAddr(addr net.Addr) (ipStr string) {
 // processUpstream passes request to upstream servers and handles the response.
 func (s *Server) processUpstream(dctx *dnsContext) (rc resultCode) {
 	pctx := dctx.proxyCtx
+	req := pctx.Req
+	q := req.Question[0]
 	if pctx.Res != nil {
 		// The response has already been set.
 		return resultCodeSuccess
+	} else if reqHost, ok := s.isDHCPClientHostQ(q); ok {
+		// A DHCP client hostname query that hasn't been handled or filtered.
+		// Respond with an NXDOMAIN.
+		//
+		// TODO(a.garipov): Route such queries to a custom upstream for the
+		// local domain name if there is one.
+		log.Debug("dns: dhcp client hostname %q was not filtered", reqHost)
+		pctx.Res = s.genNXDomain(req)
+
+		return resultCodeFinish
 	}
 
 	s.setCustomUpstream(pctx, dctx.clientID)
 
-	req := pctx.Req
 	origReqAD := false
 	if s.conf.EnableDNSSEC {
 		if req.AuthenticatedData {
@@ -679,6 +678,28 @@ func (s *Server) processUpstream(dctx *dnsContext) (rc resultCode) {
 	return resultCodeSuccess
 }
 
+// isDHCPClientHostQ returns true if q is from a request for a DHCP client
+// hostname.  If ok is true, reqHost contains the requested hostname.
+func (s *Server) isDHCPClientHostQ(q dns.Question) (reqHost string, ok bool) {
+	if !s.dhcpServer.Enabled() {
+		return "", false
+	}
+
+	// Include AAAA here, because despite the fact that we don't support it yet,
+	// the expected behavior here is to respond with an empty answer and not
+	// NXDOMAIN.
+	if qt := q.Qtype; qt != dns.TypeA && qt != dns.TypeAAAA {
+		return "", false
+	}
+
+	reqHost = strings.ToLower(q.Name[:len(q.Name)-1])
+	if strings.HasSuffix(reqHost, s.localDomainSuffix) {
+		return reqHost, true
+	}
+
+	return "", false
+}
+
 // setCustomUpstream sets custom upstream settings in pctx, if necessary.
 func (s *Server) setCustomUpstream(pctx *proxy.DNSContext, clientID string) {
 	customUpsByClient := s.conf.GetCustomUpstreamByClient

internal/dnsforward/dns_test.go

@@ -248,7 +248,7 @@ func TestServer_ProcessDHCPHosts_localRestriction(t *testing.T) {
 		name:       "local_client_unknown_host",
 		host:       "wronghost.lan",
 		wantIP:     nil,
-		wantRes:    resultCodeFinish,
+		wantRes:    resultCodeSuccess,
 		isLocalCli: true,
 	}, {
 		name:       "external_client_known_host",
@@ -358,7 +358,7 @@ func TestServer_ProcessDHCPHosts(t *testing.T) {
 		host:    "example-new.lan",
 		suffix:  defaultLocalDomainSuffix,
 		wantIP:  nil,
-		wantRes: resultCodeFinish,
+		wantRes: resultCodeSuccess,
 		qtyp:    dns.TypeA,
 	}, {
 		name:    "success_internal_aaaa",