diff --git a/internal/client/persistent.go b/internal/client/persistent.go index 06e346f4dfc..317dc72b522 100644 --- 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 index 12767cc1876..995ca5d1d16 100644 --- 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[len(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[len(reply.Answer)-1]) + assert.Equal(t, net.IP(tc.want.AsSlice()), a.A) + } }) } } diff --git a/internal/dnsforward/filter.go b/internal/dnsforward/filter.go index 4225e18d134..cdd4bd33e1e 100644 --- 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,20 @@ 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, - ) + case res.IsFiltered && res.CanonName == "": + 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.RewrittenRule) && + case res.Reason.In( + filtering.Rewritten, + filtering.RewrittenRule, + filtering.FilteredSafeSearch) && res.CanonName != "" && len(res.IPList) == 0: // 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.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 { diff --git a/internal/dnsforward/process.go b/internal/dnsforward/process.go index 4f8ec63ab2b..1dd9e664b0d 100644 --- 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 diff --git a/internal/filtering/safesearch.go b/internal/filtering/safesearch.go index 57d117b4a60..39c05140ae6 100644 --- a/internal/filtering/safesearch.go +++ b/internal/filtering/safesearch.go @@ -14,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"` diff --git a/internal/filtering/safesearch/safesearch.go b/internal/filtering/safesearch/safesearch.go index d0d02aa9b68..2ed40d5b813 100644 --- a/internal/filtering/safesearch/safesearch.go +++ b/internal/filtering/safesearch/safesearch.go @@ -3,11 +3,9 @@ package safesearch import ( "bytes" - "context" "encoding/binary" "encoding/gob" "fmt" - "net" "net/netip" "strings" "sync" @@ -67,7 +65,6 @@ type Default struct { engine *urlfilter.DNSEngine cache cache.Cache - resolver filtering.Resolver logPrefix string cacheTTL time.Duration } @@ -80,11 +77,6 @@ func NewDefault( cacheSize uint, cacheTTL time.Duration, ) (ss *Default, err error) { - var resolver filtering.Resolver = net.DefaultResolver - if conf.CustomResolver != nil { - resolver = conf.CustomResolver - } - ss = &Default{ mu: &sync.RWMutex{}, @@ -92,7 +84,6 @@ func NewDefault( EnableLRU: true, MaxSize: cacheSize, }), - resolver: resolver, // Use %s, because the client safe-search names already contain double // quotes. logPrefix: fmt.Sprintf("safesearch %s: ", name), @@ -228,18 +219,11 @@ func (ss *Default) searchHost(host string, qtype rules.RRType) (res *rules.DNSRe // newResult creates Result object from rewrite rule. qtype must be either // [dns.TypeA] or [dns.TypeAAAA], or [dns.TypeHTTPS]. If err is nil, res is // never nil, so that the empty result is converted into a NODATA response. -// -// TODO(a.garipov): Use the main rewrite result mechanism used in -// [dnsforward.Server.filterDNSRequest]. Now we resolve IPs for CNAME to save -// them in the safe search cache. func (ss *Default) newResult( rewrite *rules.DNSRewrite, qtype rules.RRType, ) (res *filtering.Result, err error) { res = &filtering.Result{ - Rules: []*filtering.ResultRule{{ - FilterListID: rulelist.URLFilterIDSafeSearch, - }}, Reason: filtering.FilteredSafeSearch, IsFiltered: true, } @@ -250,72 +234,19 @@ func (ss *Default) newResult( return nil, fmt.Errorf("expected ip rewrite value, got %T(%[1]v)", rewrite.Value) } - res.Rules[0].IP = ip - - return res, nil - } - - host := rewrite.NewCNAME - if host == "" { - return res, nil - } + res.Rules = []*filtering.ResultRule{{ + FilterListID: rulelist.URLFilterIDSafeSearch, + IP: ip, + }} - res.CanonName = host - if qtype == dns.TypeHTTPS { return res, nil } - ss.log(log.DEBUG, "resolving %q", host) - - ips, err := ss.resolver.LookupIP(context.Background(), qtypeToProto(qtype), host) - if err != nil { - return nil, fmt.Errorf("resolving cname: %w", err) - } - - ss.log(log.DEBUG, "resolved %s", ips) - - for _, ip := range ips { - // TODO(a.garipov): Remove this filtering once the resolver we use - // actually learns about network. - addr := fitToProto(ip, qtype) - if addr == (netip.Addr{}) { - continue - } - - // TODO(e.burkov): Rules[0]? - res.Rules[0].IP = addr - } + res.CanonName = rewrite.NewCNAME return res, nil } -// qtypeToProto returns "ip4" for [dns.TypeA] and "ip6" for [dns.TypeAAAA]. -// It panics for other types. -func qtypeToProto(qtype rules.RRType) (proto string) { - switch qtype { - case dns.TypeA: - return "ip4" - case dns.TypeAAAA: - return "ip6" - default: - panic(fmt.Errorf("safesearch: unsupported question type %s", dns.Type(qtype))) - } -} - -// fitToProto returns a non-nil IP address if ip is the correct protocol version -// for qtype. qtype is expected to be either [dns.TypeA] or [dns.TypeAAAA]. -func fitToProto(ip net.IP, qtype rules.RRType) (res netip.Addr) { - if ip4 := ip.To4(); qtype == dns.TypeA { - if ip4 != nil { - return netip.AddrFrom4([4]byte(ip4)) - } - } else if ip = ip.To16(); ip != nil && qtype == dns.TypeAAAA { - return netip.AddrFrom16([16]byte(ip)) - } - - return netip.Addr{} -} - // setCacheResult stores data in cache for host. qtype is expected to be either // [dns.TypeA] or [dns.TypeAAAA]. func (ss *Default) setCacheResult(host string, qtype rules.RRType, res filtering.Result) { diff --git a/internal/filtering/safesearch/safesearch_internal_test.go b/internal/filtering/safesearch/safesearch_internal_test.go index 807bf9cc567..c6790dc3e4f 100644 --- a/internal/filtering/safesearch/safesearch_internal_test.go +++ b/internal/filtering/safesearch/safesearch_internal_test.go @@ -1,13 +1,10 @@ package safesearch import ( - "context" - "net" "net/netip" "testing" "time" - "github.com/AdguardTeam/AdGuardHome/internal/aghtest" "github.com/AdguardTeam/AdGuardHome/internal/filtering" "github.com/AdguardTeam/urlfilter/rules" "github.com/miekg/dns" @@ -79,47 +76,6 @@ func TestSafeSearchCacheYandex(t *testing.T) { assert.Equal(t, cachedValue.Rules[0].IP, yandexIP) } -func TestSafeSearchCacheGoogle(t *testing.T) { - const domain = "www.google.ru" - - ss := newForTest(t, filtering.SafeSearchConfig{Enabled: false}) - - res, err := ss.CheckHost(domain, testQType) - require.NoError(t, err) - - assert.False(t, res.IsFiltered) - assert.Empty(t, res.Rules) - - 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 - }, - } - - ss = newForTest(t, defaultSafeSearchConf) - ss.resolver = resolver - - // Lookup for safesearch domain. - rewrite := ss.searchHost(domain, testQType) - - wantIP, _ := aghtest.HostToIPs(rewrite.NewCNAME) - - res, err = ss.CheckHost(domain, testQType) - require.NoError(t, err) - require.Len(t, res.Rules, 1) - - assert.Equal(t, wantIP, res.Rules[0].IP) - - // Check cache. - cachedValue, isFound := ss.getCachedResult(domain, testQType) - require.True(t, isFound) - require.Len(t, cachedValue.Rules, 1) - - assert.Equal(t, wantIP, cachedValue.Rules[0].IP) -} - const googleHost = "www.google.com" var dnsRewriteSink *rules.DNSRewrite diff --git a/internal/filtering/safesearch/safesearch_test.go b/internal/filtering/safesearch/safesearch_test.go index 029158355ed..ae62da567bd 100644 --- a/internal/filtering/safesearch/safesearch_test.go +++ b/internal/filtering/safesearch/safesearch_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/AdguardTeam/AdGuardHome/internal/aghtest" "github.com/AdguardTeam/AdGuardHome/internal/filtering" "github.com/AdguardTeam/AdGuardHome/internal/filtering/rulelist" "github.com/AdguardTeam/AdGuardHome/internal/filtering/safesearch" @@ -31,8 +30,6 @@ const ( // testConf is the default safe search configuration for tests. var testConf = filtering.SafeSearchConfig{ - CustomResolver: nil, - Enabled: true, Bing: true, @@ -88,34 +85,23 @@ func TestDefault_CheckHost_yandex(t *testing.T) { require.NoError(t, err) assert.True(t, res.IsFiltered) + assert.Equal(t, filtering.FilteredSafeSearch, res.Reason) - // TODO(a.garipov): In case of AAAA and HTTPS requests, the - // safe-search filter returns a single rule with a nil IP - // address. This isn't really necessary and should be changed - // once the TODO in [safesearch.Default.newResult] is resolved. - require.Len(t, res.Rules, 1) + if tc.want == (netip.Addr{}) { + assert.Empty(t, res.Rules) + } else { + require.Len(t, res.Rules, 1) - assert.Equal(t, tc.want, res.Rules[0].IP) - assert.Equal(t, rulelist.URLFilterIDSafeSearch, res.Rules[0].FilterListID) + assert.Equal(t, tc.want, res.Rules[0].IP) + assert.Equal(t, rulelist.URLFilterIDSafeSearch, res.Rules[0].FilterListID) + } } }) } } func TestDefault_CheckHost_google(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 - }, - } - - wantIP, _ := aghtest.HostToIPs("forcesafesearch.google.com") - - conf := testConf - conf.CustomResolver = resolver - ss, err := safesearch.NewDefault(conf, "", testCacheSize, testCacheTTL) + ss, err := safesearch.NewDefault(testConf, "", testCacheSize, testCacheTTL) require.NoError(t, err) // Check host for each domain. @@ -134,11 +120,9 @@ func TestDefault_CheckHost_google(t *testing.T) { require.NoError(t, err) assert.True(t, res.IsFiltered) - - require.Len(t, res.Rules, 1) - - assert.Equal(t, wantIP, res.Rules[0].IP) - assert.Equal(t, rulelist.URLFilterIDSafeSearch, res.Rules[0].FilterListID) + assert.Equal(t, filtering.FilteredSafeSearch, res.Reason) + assert.Equal(t, "forcesafesearch.google.com", res.CanonName) + assert.Empty(t, res.Rules) }) } } @@ -163,17 +147,7 @@ func (r *testResolver) LookupIP( } func TestDefault_CheckHost_duckduckgoAAAA(t *testing.T) { - conf := testConf - conf.CustomResolver = &testResolver{ - OnLookupIP: func(_ context.Context, network, host string) (ips []net.IP, err error) { - assert.Equal(t, "ip6", network) - assert.Equal(t, "safe.duckduckgo.com", host) - - return nil, nil - }, - } - - ss, err := safesearch.NewDefault(conf, "", testCacheSize, testCacheTTL) + ss, err := safesearch.NewDefault(testConf, "", testCacheSize, testCacheTTL) require.NoError(t, err) // The DuckDuckGo safe-search addresses are resolved through CNAMEs, but @@ -183,14 +157,9 @@ func TestDefault_CheckHost_duckduckgoAAAA(t *testing.T) { require.NoError(t, err) assert.True(t, res.IsFiltered) - - // TODO(a.garipov): Currently, the safe-search filter returns a single rule - // with a nil IP address. This isn't really necessary and should be changed - // once the TODO in [safesearch.Default.newResult] is resolved. - require.Len(t, res.Rules, 1) - - assert.Empty(t, res.Rules[0].IP) - assert.Equal(t, rulelist.URLFilterIDSafeSearch, res.Rules[0].FilterListID) + assert.Equal(t, filtering.FilteredSafeSearch, res.Reason) + assert.Equal(t, "safe.duckduckgo.com", res.CanonName) + assert.Empty(t, res.Rules) } func TestDefault_Update(t *testing.T) { diff --git a/internal/home/clients.go b/internal/home/clients.go index 5c20497375e..1474002bc5f 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -249,8 +249,6 @@ func (o *clientObject) toPersistent( } if o.SafeSearchConf.Enabled { - o.SafeSearchConf.CustomResolver = safeSearchResolver{} - err = cli.SetSafeSearch( o.SafeSearchConf, filteringConf.SafeSearchCacheSize, diff --git a/internal/home/dns.go b/internal/home/dns.go index 0ca9717d50f..c949ddb9cfd 100644 --- a/internal/home/dns.go +++ b/internal/home/dns.go @@ -1,7 +1,6 @@ package home import ( - "context" "fmt" "net" "net/netip" @@ -528,36 +527,6 @@ func closeDNSServer() { log.Debug("all dns modules are closed") } -// safeSearchResolver is a [filtering.Resolver] implementation used for safe -// search. -type safeSearchResolver struct{} - -// type check -var _ filtering.Resolver = safeSearchResolver{} - -// LookupIP implements [filtering.Resolver] interface for safeSearchResolver. -// It returns the slice of net.Addr with IPv4 and IPv6 instances. -func (r safeSearchResolver) LookupIP( - ctx context.Context, - network string, - host string, -) (ips []net.IP, err error) { - addrs, err := Context.dnsServer.Resolve(ctx, network, host) - if err != nil { - return nil, err - } - - if len(addrs) == 0 { - return nil, fmt.Errorf("couldn't lookup host: %s", host) - } - - for _, a := range addrs { - ips = append(ips, a.AsSlice()) - } - - return ips, nil -} - // checkStatsAndQuerylogDirs checks and returns directory paths to store // statistics and query log. func checkStatsAndQuerylogDirs( diff --git a/internal/home/home.go b/internal/home/home.go index 8060467a0ac..f7db7da0d6d 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -439,7 +439,6 @@ func setupDNSFilteringConf(conf *filtering.Config) (err error) { conf.ParentalBlockHost = host } - conf.SafeSearchConf.CustomResolver = safeSearchResolver{} conf.SafeSearch, err = safesearch.NewDefault( conf.SafeSearchConf, "default",