From dae304fc3cc24b178657b668568d62880432b4b3 Mon Sep 17 00:00:00 2001 From: Dimitry Kolyshev Date: Tue, 12 Dec 2023 16:20:55 +0300 Subject: [PATCH] Pull request: safesearch cname Updates #6352. Squashed commit of the following: commit 79d24e0e44a19d05750101e2baa4129c9b62e7ac Merge: 04c2759bf c908eec5d Author: Dimitry Kolyshev Date: Tue Dec 12 12:26:57 2023 +0200 Merge remote-tracking branch 'origin/master' into 6352-safesearch-cname # Conflicts: # CHANGELOG.md commit 04c2759bf779d124673c9b3d5c6d95a1dc11a7d0 Author: Dimitry Kolyshev Date: Tue Dec 12 11:14:13 2023 +0200 all: fix changelog commit 78d726e912d2066e8137f10e4057fd9179227884 Merge: 2d2c17436 79d7a1ef4 Author: Dimitry Kolyshev Date: Tue Dec 12 11:12:58 2023 +0200 Merge remote-tracking branch 'origin/master' into 6352-safesearch-cname commit 2d2c17436266a82b6fadd436df33ffc0bf55e26f Merge: 2b1c1eabb 7b5cce517 Author: Dimitry Kolyshev Date: Mon Dec 11 11:08:08 2023 +0200 Merge remote-tracking branch 'origin/master' into 6352-safesearch-cname # Conflicts: # CHANGELOG.md commit 2b1c1eabb274351cbaffaeb1c92eb62aeccb384e Author: Dimitry Kolyshev Date: Fri Dec 8 15:24:02 2023 +0200 all: changelog commit 38afdbab686d2ec3c322c34ede032f313ba85ddc Author: Dimitry Kolyshev Date: Fri Dec 8 15:21:23 2023 +0200 safesearch: imp docs commit e941f5e76efcf1872f7e24bb2378f33e56f06db9 Author: Dimitry Kolyshev Date: Fri Dec 8 14:43:39 2023 +0200 dnsforward: imp code commit 8dedb4a01db8d3f9005c602bd4c6e54637667101 Author: Dimitry Kolyshev Date: Fri Dec 8 14:26:51 2023 +0200 dnsforward: imp tests commit 8f23adeae9d10d7b2ffc30dd76d3e18192cc774e Author: Dimitry Kolyshev Date: Fri Dec 8 13:33:50 2023 +0200 all: safesearch cnames commit 061a6deeacf801a71d1a027355d67f3fc6455eac Author: Dimitry Kolyshev Date: Fri Dec 8 13:09:32 2023 +0200 all: changelog commit 6f7ff7f9e61f492c7ded9c79fb2499cfeeaa5883 Author: Dimitry Kolyshev Date: Fri Dec 8 13:07:36 2023 +0200 all: safesearch cnames --- CHANGELOG.md | 7 +++ internal/dnsforward/dnsforward_test.go | 58 ++++++++++++++------- internal/dnsforward/filter.go | 33 +----------- internal/dnsforward/msg.go | 36 ++++++++++++- internal/filtering/safesearch/safesearch.go | 5 +- 5 files changed, 86 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 780265e8..18d04dd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,13 @@ NOTE: Add new changes BELOW THIS COMMENT. - Ability to disable plain-DNS serving via UI if an encrypted protocol is already used ([#1660]). +### Fixed + +- Omitted CNAME records in safe search results, which can cause YouTube to not + work on iOS ([#6352]). + +[#6352]: https://github.com/AdguardTeam/AdGuardHome/issues/6352 + diff --git a/internal/dnsforward/dnsforward_test.go b/internal/dnsforward/dnsforward_test.go index 2c30ff6c..bdd3308d 100644 --- a/internal/dnsforward/dnsforward_test.go +++ b/internal/dnsforward/dnsforward_test.go @@ -547,32 +547,41 @@ func TestSafeSearch(t *testing.T) { googleIP, _ := aghtest.HostToIPs("forcesafesearch.google.com") testCases := []struct { - host string - want netip.Addr + host string + want netip.Addr + wantCNAME string }{{ - host: "yandex.com.", - want: yandexIP, + host: "yandex.com.", + want: yandexIP, + wantCNAME: "", }, { - host: "yandex.by.", - want: yandexIP, + host: "yandex.by.", + want: yandexIP, + wantCNAME: "", }, { - host: "yandex.kz.", - want: yandexIP, + host: "yandex.kz.", + want: yandexIP, + wantCNAME: "", }, { - host: "yandex.ru.", - want: yandexIP, + host: "yandex.ru.", + want: yandexIP, + wantCNAME: "", }, { - host: "www.google.com.", - want: googleIP, + host: "www.google.com.", + want: googleIP, + wantCNAME: "forcesafesearch.google.com.", }, { - host: "www.google.com.af.", - want: googleIP, + host: "www.google.com.af.", + want: googleIP, + wantCNAME: "forcesafesearch.google.com.", }, { - host: "www.google.be.", - want: googleIP, + host: "www.google.be.", + want: googleIP, + wantCNAME: "forcesafesearch.google.com.", }, { - host: "www.google.by.", - want: googleIP, + host: "www.google.by.", + want: googleIP, + wantCNAME: "forcesafesearch.google.com.", }} for _, tc := range testCases { @@ -582,7 +591,18 @@ func TestSafeSearch(t *testing.T) { var reply *dns.Msg reply, _, err = client.Exchange(req, addr) require.NoErrorf(t, err, "couldn't talk to server %s: %s", addr, err) - assertResponse(t, reply, tc.want) + + if tc.wantCNAME != "" { + require.Len(t, reply.Answer, 2) + + cname := testutil.RequireTypeAssert[*dns.CNAME](t, reply.Answer[0]) + assert.Equal(t, tc.wantCNAME, cname.Target) + } 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) }) } } diff --git a/internal/dnsforward/filter.go b/internal/dnsforward/filter.go index e627122e..a92cccd6 100644 --- a/internal/dnsforward/filter.go +++ b/internal/dnsforward/filter.go @@ -95,7 +95,7 @@ func (s *Server) filterDNSRequest(dctx *dnsContext) (res *filtering.Result, err dctx.origQuestion = q req.Question[0].Name = dns.Fqdn(res.CanonName) case res.Reason == filtering.Rewritten: - pctx.Res = s.filterRewritten(req, host, res, q.Qtype) + 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 { return nil, err @@ -105,37 +105,6 @@ func (s *Server) filterDNSRequest(dctx *dnsContext) (res *filtering.Result, err return res, err } -// filterRewritten handles DNS rewrite filters. It returns a DNS response with -// the data from the filtering result. All parameters must not be nil. -func (s *Server) filterRewritten( - req *dns.Msg, - host string, - res *filtering.Result, - qt uint16, -) (resp *dns.Msg) { - resp = s.makeResponse(req) - name := host - if len(res.CanonName) != 0 { - resp.Answer = append(resp.Answer, s.genAnswerCNAME(req, res.CanonName)) - name = res.CanonName - } - - for _, ip := range res.IPList { - switch qt { - case dns.TypeA: - a := s.genAnswerA(req, ip) - a.Hdr.Name = dns.Fqdn(name) - resp.Answer = append(resp.Answer, a) - case dns.TypeAAAA: - a := s.genAnswerAAAA(req, ip) - a.Hdr.Name = dns.Fqdn(name) - resp.Answer = append(resp.Answer, a) - } - } - - return resp -} - // 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 index 6685c861..eeb2c963 100644 --- a/internal/dnsforward/msg.go +++ b/internal/dnsforward/msg.go @@ -66,12 +66,46 @@ func (s *Server) genDNSFilterMessage( // If Safe Search generated the necessary IP addresses, use them. // Otherwise, if there were no errors, there are no addresses for the // requested IP version, so produce a NODATA response. - return s.genResponseWithIPs(req, ipsFromRules(res.Rules)) + return s.getCNAMEWithIPs(req, ipsFromRules(res.Rules), res.CanonName) default: return s.genForBlockingMode(req, ipsFromRules(res.Rules)) } } +// getCNAMEWithIPs generates a filtered response to req for with CNAME record +// and provided ips. +func (s *Server) getCNAMEWithIPs(req *dns.Msg, ips []netip.Addr, cname string) (resp *dns.Msg) { + resp = s.makeResponse(req) + + originalName := req.Question[0].Name + + var ans []dns.RR + if cname != "" { + ans = append(ans, s.genAnswerCNAME(req, cname)) + + // The given IPs actually are resolved for this cname. + req.Question[0].Name = dns.Fqdn(cname) + defer func() { req.Question[0].Name = originalName }() + } + + switch req.Question[0].Qtype { + case dns.TypeA: + ans = append(ans, s.genAnswersWithIPv4s(req, ips)...) + case dns.TypeAAAA: + for _, ip := range ips { + if ip.Is6() { + ans = append(ans, s.genAnswerAAAA(req, ip)) + } + } + default: + // Go on and return an empty response. + } + + resp.Answer = ans + + return resp +} + // genForBlockingMode generates a filtered response to req based on the server's // blocking mode. func (s *Server) genForBlockingMode(req *dns.Msg, ips []netip.Addr) (resp *dns.Msg) { diff --git a/internal/filtering/safesearch/safesearch.go b/internal/filtering/safesearch/safesearch.go index f2d2b70c..47e66ac6 100644 --- a/internal/filtering/safesearch/safesearch.go +++ b/internal/filtering/safesearch/safesearch.go @@ -226,7 +226,8 @@ func (ss *Default) searchHost(host string, qtype rules.RRType) (res *rules.DNSRe // empty result is converted into a NODATA response. // // TODO(a.garipov): Use the main rewrite result mechanism used in -// [dnsforward.Server.filterDNSRequest]. +// [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, @@ -255,6 +256,8 @@ func (ss *Default) newResult( return res, nil } + res.CanonName = host + ss.log(log.DEBUG, "resolving %q", host) ips, err := ss.resolver.LookupIP(context.Background(), qtypeToProto(qtype), host)