Pull request: AG-31778-fix-safesearch-https

Squashed commit of the following: commit 85ea3d9 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Thu Apr 18 15:19:38 2024 +0200 all: imp docs commit b0695da Merge: a79f98f 48c6242 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Wed Apr 17 11:06:49 2024 +0200 Merge remote-tracking branch 'origin/master' into AG-31778-fix-safesearch-https # Conflicts: # CHANGELOG.md commit a79f98f Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Wed Apr 17 11:05:34 2024 +0200 dnsforward: imp code commit b901a11 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Tue Apr 16 11:03:52 2024 +0200 dnsforward: imp code commit fb6e669 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Tue Apr 16 10:08:51 2024 +0200 all: safesearch rewrites commit 88add21 Merge: b78ad8f 201ac73 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Tue Apr 16 09:43:20 2024 +0200 Merge remote-tracking branch 'origin/master' into AG-31778-fix-safesearch-https # Conflicts: # CHANGELOG.md commit b78ad8f Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Fri Apr 12 13:34:39 2024 +0200 all: safesearch rewrites commit fb3efbb Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Thu Apr 11 13:15:37 2024 +0200 safesearch: imp code commit 1193c70 Merge: 14e823d ff7c715 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Thu Apr 11 13:13:44 2024 +0200 Merge remote-tracking branch 'origin/master' into AG-31778-fix-safesearch-https # Conflicts: # CHANGELOG.md commit 14e823d Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Thu Apr 11 13:11:43 2024 +0200 all: safesearch https commit cd403a2 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Thu Apr 11 12:09:27 2024 +0200 Revert "all: safesearch https" This reverts commit 1c9564b. commit 1c9564b Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Wed Apr 10 12:41:47 2024 +0200 all: safesearch https commit 5f42688 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Wed Apr 10 09:22:30 2024 +0200 filtering: imp code commit eb9bd9f Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Wed Apr 10 09:19:22 2024 +0200 all: changelog commit 0c77c70 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Wed Apr 10 08:55:22 2024 +0200 safesearch: imp tests commit 492a93f Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Tue Apr 9 14:45:16 2024 +0200 all: changelog commit a665e72 Author: Dimitry Kolyshev <dkolyshev@adguard.com> Date: Tue Apr 9 14:41:24 2024 +0200 safesearch: https req
annguyen0 · Apr 22, 2024 · 762ef4a · 762ef4a
1 parent 48c6242
commit 762ef4a
Show file tree

Hide file tree

Showing 14 changed files with 115 additions and 280 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,8 +27,13 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 - Support for comments in the ipset file ([#5345]).
 
+### Changed
+
+- Rewrite rules mechanics was changed due to improve resolving in safe search.
+
 ### Fixed
 
+- YouTube restricted mode is not enforced by HTTPS queries on Firefox.
 - Support for link-local subnets, i.e. `fe80::/16`, in the access settings
   ([#6192]).
 - The ability to apply an invalid configuration for private RDNS, which led to
@@ -58,7 +63,7 @@ See also the [v0.107.48 GitHub milestone][ms-v0.107.48].
 
 ### Fixed
 
-- Access settings not being applied to encrypted protocols ([#6890])
+- Access settings not being applied to encrypted protocols ([#6890]).
 
 [#6890]: https://github.com/AdguardTeam/AdGuardHome/issues/6890
 

diff --git a/internal/client/persistent.go b/internal/client/persistent.go
@@ -64,9 +64,7 @@ type Persistent struct {
 	// upstream must be used.
 	UpstreamConfig *proxy.CustomUpstreamConfig
 
-	// TODO(d.kolyshev): Make SafeSearchConf a pointer.
-	SafeSearchConf filtering.SafeSearchConfig
-	SafeSearch     filtering.SafeSearch
+	SafeSearch filtering.SafeSearch
 
 	// BlockedServices is the configuration of blocked services of a client.
 	BlockedServices *filtering.BlockedServices
@@ -95,6 +93,9 @@ type Persistent struct {
 	UseOwnBlockedServices bool
 	IgnoreQueryLog        bool
 	IgnoreStatistics      bool
+
+	// TODO(d.kolyshev): Make SafeSearchConf a pointer.
+	SafeSearchConf filtering.SafeSearchConfig
 }
 
 // SetTags sets the tags if they are known, otherwise logs an unknown tag.

diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go
@@ -2,7 +2,6 @@ package dnsforward
 
 import (
 	"cmp"
-	"context"
 	"crypto/ecdsa"
 	"crypto/rand"
 	"crypto/rsa"
@@ -491,19 +490,10 @@ func TestServerRace(t *testing.T) {
 }
 
 func TestSafeSearch(t *testing.T) {
-	resolver := &aghtest.Resolver{
-		OnLookupIP: func(_ context.Context, _, host string) (ips []net.IP, err error) {
-			ip4, ip6 := aghtest.HostToIPs(host)
-
-			return []net.IP{ip4.AsSlice(), ip6.AsSlice()}, nil
-		},
-	}
-
 	safeSearchConf := filtering.SafeSearchConfig{
-		Enabled:        true,
-		Google:         true,
-		Yandex:         true,
-		CustomResolver: resolver,
+		Enabled: true,
+		Google:  true,
+		Yandex:  true,
 	}
 
 	filterConf := &filtering.Config{
@@ -540,7 +530,6 @@ func TestSafeSearch(t *testing.T) {
 	client := &dns.Client{}
 
 	yandexIP := netip.AddrFrom4([4]byte{213, 180, 193, 56})
-	googleIP, _ := aghtest.HostToIPs("forcesafesearch.google.com")
 
 	testCases := []struct {
 		host      string
@@ -564,19 +553,19 @@ func TestSafeSearch(t *testing.T) {
 		wantCNAME: "",
 	}, {
 		host:      "www.google.com.",
-		want:      googleIP,
+		want:      netip.Addr{},
 		wantCNAME: "forcesafesearch.google.com.",
 	}, {
 		host:      "www.google.com.af.",
-		want:      googleIP,
+		want:      netip.Addr{},
 		wantCNAME: "forcesafesearch.google.com.",
 	}, {
 		host:      "www.google.be.",
-		want:      googleIP,
+		want:      netip.Addr{},
 		wantCNAME: "forcesafesearch.google.com.",
 	}, {
 		host:      "www.google.by.",
-		want:      googleIP,
+		want:      netip.Addr{},
 		wantCNAME: "forcesafesearch.google.com.",
 	}}
 
@@ -593,12 +582,15 @@ func TestSafeSearch(t *testing.T) {
 
 				cname := testutil.RequireTypeAssert[*dns.CNAME](t, reply.Answer[0])
 				assert.Equal(t, tc.wantCNAME, cname.Target)
+
+				a := testutil.RequireTypeAssert[*dns.A](t, reply.Answer[1])
+				assert.NotEmpty(t, a.A)
 			} else {
 				require.Len(t, reply.Answer, 1)
-			}
 
-			a := testutil.RequireTypeAssert[*dns.A](t, reply.Answer[len(reply.Answer)-1])
-			assert.Equal(t, net.IP(tc.want.AsSlice()), a.A)
+				a := testutil.RequireTypeAssert[*dns.A](t, reply.Answer[0])
+				assert.Equal(t, net.IP(tc.want.AsSlice()), a.A)
+			}
 		})
 	}
 }

diff --git a/internal/dnsforward/filter.go b/internal/dnsforward/filter.go
@@ -31,6 +31,7 @@ func (s *Server) filterDNSRequest(dctx *dnsContext) (res *filtering.Result, err
 	req := pctx.Req
 	q := req.Question[0]
 	host := strings.TrimSuffix(q.Name, ".")
+
 	resVal, err := s.dnsFilter.CheckHost(host, q.Qtype, dctx.setts)
 	if err != nil {
 		return nil, fmt.Errorf("checking host %q: %w", host, err)
@@ -39,22 +40,15 @@ func (s *Server) filterDNSRequest(dctx *dnsContext) (res *filtering.Result, err
 	// TODO(a.garipov): Make CheckHost return a pointer.
 	res = &resVal
 	switch {
-	case res.IsFiltered:
-		log.Debug(
-			"dnsforward: host %q is filtered, reason: %q; rule: %q",
-			host,
-			res.Reason,
-			res.Rules[0].Text,
-		)
-		pctx.Res = s.genDNSFilterMessage(pctx, res)
-	case res.Reason.In(filtering.Rewritten, filtering.RewrittenRule) &&
-		res.CanonName != "" &&
-		len(res.IPList) == 0:
+	case isRewrittenCNAME(res):
 		// Resolve the new canonical name, not the original host name.  The
 		// original question is readded in processFilteringAfterResponse.
 		dctx.origQuestion = q
 		req.Question[0].Name = dns.Fqdn(res.CanonName)
-	case res.Reason == filtering.Rewritten:
+	case res.IsFiltered:
+		log.Debug("dnsforward: host %q is filtered, reason: %q", host, res.Reason)
+		pctx.Res = s.genDNSFilterMessage(pctx, res)
+	case res.Reason.In(filtering.Rewritten, filtering.FilteredSafeSearch):
 		pctx.Res = s.getCNAMEWithIPs(req, res.IPList, res.CanonName)
 	case res.Reason.In(filtering.RewrittenRule, filtering.RewrittenAutoHosts):
 		if err = s.filterDNSRewrite(req, res, pctx); err != nil {
@@ -65,6 +59,17 @@ func (s *Server) filterDNSRequest(dctx *dnsContext) (res *filtering.Result, err
 	return res, err
 }
 
+// isRewrittenCNAME returns true if the request considered to be rewritten with
+// CNAME and has no resolved IPs.
+func isRewrittenCNAME(res *filtering.Result) (ok bool) {
+	return res.Reason.In(
+		filtering.Rewritten,
+		filtering.RewrittenRule,
+		filtering.FilteredSafeSearch) &&
+		res.CanonName != "" &&
+		len(res.IPList) == 0
+}
+
 // checkHostRules checks the host against filters.  It is safe for concurrent
 // use.
 func (s *Server) checkHostRules(

diff --git a/internal/dnsforward/msg.go b/internal/dnsforward/msg.go
@@ -52,7 +52,7 @@ func (s *Server) genDNSFilterMessage(
 ) (resp *dns.Msg) {
 	req := dctx.Req
 	qt := req.Question[0].Qtype
-	if qt != dns.TypeA && qt != dns.TypeAAAA {
+	if qt != dns.TypeA && qt != dns.TypeAAAA && qt != dns.TypeHTTPS {
 		m, _, _ := s.dnsFilter.BlockingMode()
 		if m == filtering.BlockingModeNullIP {
 			return s.replyCompressed(req)

diff --git a/internal/dnsforward/process.go b/internal/dnsforward/process.go
@@ -598,7 +598,8 @@ func (s *Server) processFilteringAfterResponse(dctx *dnsContext) (rc resultCode)
 		return resultCodeSuccess
 	case
 		filtering.Rewritten,
-		filtering.RewrittenRule:
+		filtering.RewrittenRule,
+		filtering.FilteredSafeSearch:
 
 		if dctx.origQuestion.Name == "" {
 			// origQuestion is set in case we get only CNAME without IP from
@@ -608,11 +609,10 @@ func (s *Server) processFilteringAfterResponse(dctx *dnsContext) (rc resultCode)
 
 		pctx := dctx.proxyCtx
 		pctx.Req.Question[0], pctx.Res.Question[0] = dctx.origQuestion, dctx.origQuestion
-		if len(pctx.Res.Answer) > 0 {
-			rr := s.genAnswerCNAME(pctx.Req, res.CanonName)
-			answer := append([]dns.RR{rr}, pctx.Res.Answer...)
-			pctx.Res.Answer = answer
-		}
+
+		rr := s.genAnswerCNAME(pctx.Req, res.CanonName)
+		answer := append([]dns.RR{rr}, pctx.Res.Answer...)
+		pctx.Res.Answer = answer
 
 		return resultCodeSuccess
 	default:

diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go
@@ -559,6 +559,8 @@ type Result struct {
 	Reason Reason `json:",omitempty"`
 
 	// IsFiltered is true if the request is filtered.
+	//
+	// TODO(d.kolyshev): Get rid of this flag.
 	IsFiltered bool `json:",omitempty"`
 }
 

diff --git a/internal/filtering/safesearch.go b/internal/filtering/safesearch.go
@@ -1,7 +1,5 @@
 package filtering
 
-import "github.com/miekg/dns"
-
 // SafeSearch interface describes a service for search engines hosts rewrites.
 type SafeSearch interface {
 	// CheckHost checks host with safe search filter.  CheckHost must be safe
@@ -16,9 +14,6 @@ type SafeSearch interface {
 
 // SafeSearchConfig is a struct with safe search related settings.
 type SafeSearchConfig struct {
-	// CustomResolver is the resolver used by safe search.
-	CustomResolver Resolver `yaml:"-" json:"-"`
-
 	// Enabled indicates if safe search is enabled entirely.
 	Enabled bool `yaml:"enabled" json:"enabled"`
 
@@ -40,13 +35,7 @@ func (d *DNSFilter) checkSafeSearch(
 	qtype uint16,
 	setts *Settings,
 ) (res Result, err error) {
-	if !setts.ProtectionEnabled ||
-		!setts.SafeSearchEnabled ||
-		(qtype != dns.TypeA && qtype != dns.TypeAAAA) {
-		return Result{}, nil
-	}
-
-	if d.safeSearch == nil {
+	if d.safeSearch == nil || !setts.ProtectionEnabled || !setts.SafeSearchEnabled {
 		return Result{}, nil
 	}