Pull request: filtering: restore rewrite behavior with other question…

… types Updates #4008. Squashed commit of the following: commit babbc29 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Fri Dec 24 18:46:20 2021 +0300 filtering: restore rewrite behavior with other question types
AdguardTeam · Dec 24, 2021 · 52f36f2 · 52f36f2
1 parent 46cd974
commit 52f36f2
Show file tree

Hide file tree

Showing 7 changed files with 232 additions and 195 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -103,32 +103,30 @@
     - 'name': 'Run snapshot build'
       'run': 'make SIGN=0 VERBOSE=1 build-release build-docker'
 
-  # TODO(a.garipov): Remove once AWS/Slack are back online.
-  #
-  # 'notify':
-  #   'needs':
-  #   - 'build-release'
-  #   # Secrets are not passed to workflows that are triggered by a pull request
-  #   # from a fork.
-  #   #
-  #   # Use always() to signal to the runner that this job must run even if the
-  #   # previous ones failed.
-  #   'if':
-  #     ${{ always() &&
-  #       (
-  #         github.event_name == 'push' ||
-  #         github.event.pull_request.head.repo.full_name == github.repository
-  #       )
-  #     }}
-  #   'runs-on': 'ubuntu-latest'
-  #   'steps':
-  #   - 'name': 'Conclusion'
-  #     'uses': 'technote-space/workflow-conclusion-action@v1'
-  #   - 'name': 'Send Slack notif'
-  #     'uses': '8398a7/action-slack@v3'
-  #     'with':
-  #       'status': '${{ env.WORKFLOW_CONCLUSION }}'
-  #       'fields': 'repo, message, commit, author, workflow'
-  #     'env':
-  #       'GITHUB_TOKEN': '${{ secrets.GITHUB_TOKEN }}'
-  #       'SLACK_WEBHOOK_URL': '${{ secrets.SLACK_WEBHOOK_URL }}'
+  'notify':
+    'needs':
+    - 'build-release'
+    # Secrets are not passed to workflows that are triggered by a pull request
+    # from a fork.
+    #
+    # Use always() to signal to the runner that this job must run even if the
+    # previous ones failed.
+    'if':
+      ${{ always() &&
+        (
+          github.event_name == 'push' ||
+          github.event.pull_request.head.repo.full_name == github.repository
+        )
+      }}
+    'runs-on': 'ubuntu-latest'
+    'steps':
+    - 'name': 'Conclusion'
+      'uses': 'technote-space/workflow-conclusion-action@v1'
+    - 'name': 'Send Slack notif'
+      'uses': '8398a7/action-slack@v3'
+      'with':
+        'status': '${{ env.WORKFLOW_CONCLUSION }}'
+        'fields': 'repo, message, commit, author, workflow'
+      'env':
+        'GITHUB_TOKEN': '${{ secrets.GITHUB_TOKEN }}'
+        'SLACK_WEBHOOK_URL': '${{ secrets.SLACK_WEBHOOK_URL }}'
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -33,33 +33,31 @@
     - 'name': 'Run ESLint'
       'run': 'npm --prefix="./client" run lint'
 
-  # TODO(a.garipov): Remove once AWS/Slack are back online.
-  #
-  # 'notify':
-  #   'needs':
-  #   - 'go-lint'
-  #   - 'eslint'
-  #   # Secrets are not passed to workflows that are triggered by a pull request
-  #   # from a fork.
-  #   #
-  #   # Use always() to signal to the runner that this job must run even if the
-  #   # previous ones failed.
-  #   'if':
-  #     ${{ always() &&
-  #       (
-  #         github.event_name == 'push' ||
-  #         github.event.pull_request.head.repo.full_name == github.repository
-  #       )
-  #     }}
-  #   'runs-on': 'ubuntu-latest'
-  #   'steps':
-  #   - 'name': 'Conclusion'
-  #     'uses': 'technote-space/workflow-conclusion-action@v1'
-  #   - 'name': 'Send Slack notif'
-  #     'uses': '8398a7/action-slack@v3'
-  #     'with':
-  #       'status': '${{ env.WORKFLOW_CONCLUSION }}'
-  #       'fields': 'repo, message, commit, author, workflow'
-  #     'env':
-  #       'GITHUB_TOKEN': '${{ secrets.GITHUB_TOKEN }}'
-  #       'SLACK_WEBHOOK_URL': '${{ secrets.SLACK_WEBHOOK_URL }}'
+  'notify':
+    'needs':
+    - 'go-lint'
+    - 'eslint'
+    # Secrets are not passed to workflows that are triggered by a pull request
+    # from a fork.
+    #
+    # Use always() to signal to the runner that this job must run even if the
+    # previous ones failed.
+    'if':
+      ${{ always() &&
+        (
+          github.event_name == 'push' ||
+          github.event.pull_request.head.repo.full_name == github.repository
+        )
+      }}
+    'runs-on': 'ubuntu-latest'
+    'steps':
+    - 'name': 'Conclusion'
+      'uses': 'technote-space/workflow-conclusion-action@v1'
+    - 'name': 'Send Slack notif'
+      'uses': '8398a7/action-slack@v3'
+      'with':
+        'status': '${{ env.WORKFLOW_CONCLUSION }}'
+        'fields': 'repo, message, commit, author, workflow'
+      'env':
+        'GITHUB_TOKEN': '${{ secrets.GITHUB_TOKEN }}'
+        'SLACK_WEBHOOK_URL': '${{ secrets.SLACK_WEBHOOK_URL }}'
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -29,6 +29,8 @@ and this project adheres to
 
 ### Fixed
 
+- Legacy DNS rewrites responding from upstream when a request other than `A` or
+  `AAAA` is received ([#4008]).
 - Panic on port availability check during installation ([#3987]).
 
 ### Removed
@@ -37,6 +39,7 @@ and this project adheres to
 
 [#3057]: https://github.com/AdguardTeam/AdGuardHome/issues/3057
 [#3987]: https://github.com/AdguardTeam/AdGuardHome/issues/3987
+[#4008]: https://github.com/AdguardTeam/AdGuardHome/issues/4008
 
 
 

diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go
@@ -507,59 +507,74 @@ func (d *DNSFilter) matchSysHostsIntl(
 	return res, nil
 }
 
-// Process rewrites table
-// . Find CNAME for a domain name (exact match or by wildcard)
-//  . if found and CNAME equals to domain name - this is an exception;  exit
-//  . if found, set domain name to canonical name
-//  . repeat for the new domain name (Note: we return only the last CNAME)
-// . Find A or AAAA record for a domain name (exact match or by wildcard)
-//  . if found, set IP addresses (IPv4 or IPv6 depending on qtype) in Result.IPList array
+// processRewrites performs filtering based on the legacy rewrite records.
+//
+// Firstly, it finds CNAME rewrites for host.  If the CNAME is the same as host,
+// this query isn't filtered.  If it's different, repeat the process for the new
+// CNAME, breaking loops in the process.
+//
+// Secondly, it finds A or AAAA rewrites for host and, if found, sets res.IPList
+// accordingly.  If the found rewrite has a special value of "A" or "AAAA", the
+// result is an exception.
 func (d *DNSFilter) processRewrites(host string, qtype uint16) (res Result) {
 	d.confLock.RLock()
 	defer d.confLock.RUnlock()
 
-	rr := findRewrites(d.Rewrites, host, qtype)
-	if len(rr) != 0 {
-		res.Reason = Rewritten
+	rewrites, matched := findRewrites(d.Rewrites, host, qtype)
+	if !matched {
+		return Result{}
 	}
 
+	res.Reason = Rewritten
+
 	cnames := stringutil.NewSet()
 	origHost := host
-	for len(rr) != 0 && rr[0].Type == dns.TypeCNAME {
-		log.Debug("rewrite: CNAME for %s is %s", host, rr[0].Answer)
+	for matched && len(rewrites) > 0 && rewrites[0].Type == dns.TypeCNAME {
+		rwAns := rewrites[0].Answer
 
-		if host == rr[0].Answer { // "host == CNAME" is an exception
+		log.Debug("rewrite: cname for %s is %s", host, rwAns)
+
+		if host == rwAns {
+			// Rewrite of a domain onto itself is an exception rule.
 			res.Reason = NotFilteredNotFound
 
 			return res
 		}
 
-		host = rr[0].Answer
+		host = rwAns
 		if cnames.Has(host) {
-			log.Info("rewrite: breaking CNAME redirection loop: %s.  Question: %s", host, origHost)
+			log.Info("rewrite: cname loop for %q on %q", origHost, host)
 
 			return res
 		}
 
 		cnames.Add(host)
-		res.CanonName = rr[0].Answer
-		rr = findRewrites(d.Rewrites, host, qtype)
+		res.CanonName = host
+		rewrites, matched = findRewrites(d.Rewrites, host, qtype)
 	}
 
-	for _, r := range rr {
-		if r.Type == qtype && (qtype == dns.TypeA || qtype == dns.TypeAAAA) {
-			if r.IP == nil { // IP exception
+	setRewriteResult(&res, host, rewrites, qtype)
+
+	return res
+}
+
+// setRewriteResult sets the Reason or IPList of res if necessary.  res must not
+// be nil.
+func setRewriteResult(res *Result, host string, rewrites []RewriteEntry, qtype uint16) {
+	for _, rw := range rewrites {
+		if rw.Type == qtype && (qtype == dns.TypeA || qtype == dns.TypeAAAA) {
+			if rw.IP == nil {
+				// "A"/"AAAA" exception: allow getting from upstream.
 				res.Reason = NotFilteredNotFound
 
-				return res
+				return
 			}
 
-			res.IPList = append(res.IPList, r.IP)
-			log.Debug("rewrite: A/AAAA for %s is %s", host, r.IP)
+			res.IPList = append(res.IPList, rw.IP)
+
+			log.Debug("rewrite: a/aaaa for %s is %s", host, rw.IP)
 		}
 	}
-
-	return res
 }
 
 // matchBlockedServicesRules checks the host against the blocked services rules

diff --git a/internal/filtering/rewrites.go b/internal/filtering/rewrites.go
@@ -18,12 +18,15 @@ import (
 type RewriteEntry struct {
 	// Domain is the domain for which this rewrite should work.
 	Domain string `yaml:"domain"`
+
 	// Answer is the IP address, canonical name, or one of the special
 	// values: "A" or "AAAA".
 	Answer string `yaml:"answer"`
+
 	// IP is the IP address that should be used in the response if Type is
 	// A or AAAA.
 	IP net.IP `yaml:"-"`
+
 	// Type is the DNS record type: A, AAAA, or CNAME.
 	Type uint16 `yaml:"-"`
 }
@@ -143,39 +146,46 @@ func (d *DNSFilter) prepareRewrites() {
 	}
 }
 
-// findRewrites returns the list of matched rewrite entries.  The priority is:
-// CNAME, then A and AAAA; exact, then wildcard.  If the host is matched
-// exactly, wildcard entries aren't returned.  If the host matched by wildcards,
-// return the most specific for the question type.
-func findRewrites(entries []RewriteEntry, host string, qtype uint16) (matched []RewriteEntry) {
-	rr := rewritesSorted{}
+// findRewrites returns the list of matched rewrite entries.  If rewrites are
+// empty, but matched is true, the domain is found among the rewrite rules but
+// not for this question type.
+//
+// The result priority is: CNAME, then A and AAAA; exact, then wildcard.  If the
+// host is matched exactly, wildcard entries aren't returned.  If the host
+// matched by wildcards, return the most specific for the question type.
+func findRewrites(
+	entries []RewriteEntry,
+	host string,
+	qtype uint16,
+) (rewrites []RewriteEntry, matched bool) {
 	for _, e := range entries {
 		if e.Domain != host && !matchDomainWildcard(host, e.Domain) {
 			continue
 		}
 
+		matched = true
 		if e.matchesQType(qtype) {
-			rr = append(rr, e)
+			rewrites = append(rewrites, e)
 		}
 	}
 
-	if len(rr) == 0 {
-		return nil
+	if len(rewrites) == 0 {
+		return nil, matched
 	}
 
-	sort.Sort(rr)
+	sort.Sort(rewritesSorted(rewrites))
 
-	for i, r := range rr {
+	for i, r := range rewrites {
 		if isWildcard(r.Domain) {
-			// Don't use rr[:0], because we need to return at least
-			// one item here.
-			rr = rr[:max(1, i)]
+			// Don't use rewrites[:0], because we need to return at least one
+			// item here.
+			rewrites = rewrites[:max(1, i)]
 
 			break
 		}
 	}
 
-	return rr
+	return rewrites, matched
 }
 
 func max(a, b int) int {
@@ -230,8 +240,7 @@ func (d *DNSFilter) handleRewriteAdd(w http.ResponseWriter, r *http.Request) {
 	d.confLock.Lock()
 	d.Config.Rewrites = append(d.Config.Rewrites, ent)
 	d.confLock.Unlock()
-	log.Debug("Rewrites: added element: %s -> %s [%d]",
-		ent.Domain, ent.Answer, len(d.Config.Rewrites))
+	log.Debug("rewrite: added element: %s -> %s [%d]", ent.Domain, ent.Answer, len(d.Config.Rewrites))
 
 	d.Config.ConfigModified()
 }
@@ -253,9 +262,11 @@ func (d *DNSFilter) handleRewriteDelete(w http.ResponseWriter, r *http.Request)
 	d.confLock.Lock()
 	for _, ent := range d.Config.Rewrites {
 		if ent.equal(entDel) {
-			log.Debug("Rewrites: removed element: %s -> %s", ent.Domain, ent.Answer)
+			log.Debug("rewrite: removed element: %s -> %s", ent.Domain, ent.Answer)
+
 			continue
 		}
+
 		arr = append(arr, ent)
 	}
 	d.Config.Rewrites = arr