From f32da12a86ec21c9a5e41e71b2620a3899d1bcf1 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Fri, 29 Jul 2022 19:27:15 +0300 Subject: [PATCH] Pull request: 4517 domain specific test Merge in DNS/adguard-home from 4517-domain-specific-test to master Updates #4517. Squashed commit of the following: commit 03a803f831749a060923ec966592696f99591786 Merge: 8ea24170 f5959a0d Author: Eugene Burkov Date: Fri Jul 29 19:17:28 2022 +0300 Merge branch 'master' into 4517-domain-specific-test commit 8ea2417036547996bb2d39b75b0ff31de4fe9b21 Author: Eugene Burkov Date: Fri Jul 29 18:44:26 2022 +0300 all: log changes, imp docs commit aa74c8be64f2796a2dfa7166f0155fff5bb395b6 Author: Eugene Burkov Date: Fri Jul 29 18:07:12 2022 +0300 dnsforward: imp logging commit 02dccca4e7d766bbfbe0826933e8be70fcd93f58 Author: Eugene Burkov Date: Fri Jul 29 17:24:08 2022 +0300 all: imp code, docs commit 3b21067d07b208baf574a34fb06ec808b37c4ee3 Author: Eugene Burkov Date: Fri Jul 29 13:34:55 2022 +0300 client: add warning toast commit ea2272dc77f87e34dc6aff0af99c7a51a04e3770 Author: Eugene Burkov Date: Thu Jul 28 20:11:55 2022 +0300 dnsforward: imp err msg, docs commit fd9ee82afef9d93961c30ebafcc7a11d984247b5 Author: Eugene Burkov Date: Thu Jul 28 19:24:58 2022 +0300 dnsforward: test doain specific upstreams commit 9a83ebfa7a73bf4e03eaf1ff4a33f79771159fc7 Author: Eugene Burkov Date: Thu Jul 28 18:22:49 2022 +0300 dnsforward: merge some logic --- CHANGELOG.md | 3 + client/src/__locales/en.json | 1 + client/src/actions/index.js | 6 +- internal/dnsforward/http.go | 156 ++++++++++++++++++++----------- internal/dnsforward/http_test.go | 153 +++++++++--------------------- 5 files changed, 151 insertions(+), 168 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ae6e50a..9c95b887 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ and this project adheres to ### Added +- Domain-specific upstream servers test. Such test fails with an appropriate + warning message ([#4517]). - Support for Discovery of Designated Resolvers (DDR) according to the [RFC draft][ddr-draft] ([#4463]). - `windows/arm64` support ([#3057]). @@ -37,6 +39,7 @@ and this project adheres to [#2993]: https://github.com/AdguardTeam/AdGuardHome/issues/2993 [#3057]: https://github.com/AdguardTeam/AdGuardHome/issues/3057 +[#4517]: https://github.com/AdguardTeam/AdGuardHome/issues/4517 [ddr-draft]: https://datatracker.ietf.org/doc/html/draft-ietf-add-ddr-08 diff --git a/client/src/__locales/en.json b/client/src/__locales/en.json index 58008418..46e75fe6 100644 --- a/client/src/__locales/en.json +++ b/client/src/__locales/en.json @@ -222,6 +222,7 @@ "updated_upstream_dns_toast": "Upstream servers successfully saved", "dns_test_ok_toast": "Specified DNS servers are working correctly", "dns_test_not_ok_toast": "Server \"{{key}}\": could not be used, please check that you've written it correctly", + "dns_test_warning_toast": "Server \"{{key}}\" does not respond to test requests and may not work properly", "unblock": "Unblock", "block": "Block", "disallow_this_client": "Disallow this client", diff --git a/client/src/actions/index.js b/client/src/actions/index.js index 8e153224..e1b4e96c 100644 --- a/client/src/actions/index.js +++ b/client/src/actions/index.js @@ -314,13 +314,15 @@ export const testUpstream = ( const testMessages = Object.keys(upstreamResponse) .map((key) => { const message = upstreamResponse[key]; - if (message !== 'OK') { + if (message.startsWith('WARNING:')) { + dispatch(addErrorToast({ error: i18next.t('dns_test_warning_toast', { key }) })); + } else if (message !== 'OK') { dispatch(addErrorToast({ error: i18next.t('dns_test_not_ok_toast', { key }) })); } return message; }); - if (testMessages.every((message) => message === 'OK')) { + if (testMessages.every((message) => message === 'OK' || message.startsWith('WARNING:'))) { dispatch(addSuccessToast('dns_test_ok_toast')); } diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index 50ab9643..e62d9f24 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -363,6 +363,21 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro return nil, nil } + for _, u := range upstreams { + var ups string + var domains []string + ups, domains, err = separateUpstream(u) + if err != nil { + // Don't wrap the error since it's informative enough as is. + return nil, err + } + + _, err = validateUpstream(ups, domains) + if err != nil { + return nil, fmt.Errorf("validating upstream %q: %w", u, err) + } + } + conf, err = proxy.ParseUpstreamsConfig( upstreams, &upstream.Options{Bootstrap: []string{}, Timeout: DefaultTimeout}, @@ -373,13 +388,6 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro return nil, errors.Error("no default upstreams specified") } - for _, u := range upstreams { - _, err = validateUpstream(u) - if err != nil { - return nil, err - } - } - return conf, nil } @@ -449,16 +457,14 @@ func ValidateUpstreamsPrivate(upstreams []string, privateNets netutil.SubnetSet) var protocols = []string{"udp://", "tcp://", "tls://", "https://", "sdns://", "quic://"} -func validateUpstream(u string) (useDefault bool, err error) { - // Check if the user tries to specify upstream for domain. - var isDomainSpec bool - u, isDomainSpec, err = separateUpstream(u) - if err != nil { - return !isDomainSpec, err - } - +// validateUpstream returns an error if u alongside with domains is not a valid +// upstream configuration. useDefault is true if the upstream is +// domain-specific and is configured to point at the default upstream server +// which is validated separately. The upstream is considered domain-specific +// only if domains is at least not nil. +func validateUpstream(u string, domains []string) (useDefault bool, err error) { // The special server address '#' means that default server must be used. - if useDefault = !isDomainSpec; u == "#" && isDomainSpec { + if useDefault = u == "#" && domains != nil; useDefault { return useDefault, nil } @@ -485,12 +491,14 @@ func validateUpstream(u string) (useDefault bool, err error) { return useDefault, nil } -// separateUpstream returns the upstream without the specified domains. -// isDomainSpec is true when the upstream is domains-specific. -func separateUpstream(upstreamStr string) (upstream string, isDomainSpec bool, err error) { +// separateUpstream returns the upstream and the specified domains. domains is +// nil when the upstream is not domains-specific. Otherwise it may also be +// empty. +func separateUpstream(upstreamStr string) (ups string, domains []string, err error) { if !strings.HasPrefix(upstreamStr, "[/") { - return upstreamStr, false, nil + return upstreamStr, nil, nil } + defer func() { err = errors.Annotate(err, "bad upstream for domain %q: %w", upstreamStr) }() parts := strings.Split(upstreamStr[2:], "/]") @@ -498,40 +506,46 @@ func separateUpstream(upstreamStr string) (upstream string, isDomainSpec bool, e case 2: // Go on. case 1: - return "", false, errors.Error("missing separator") + return "", nil, errors.Error("missing separator") default: - return "", true, errors.Error("duplicated separator") + return "", []string{}, errors.Error("duplicated separator") } - var domains string - domains, upstream = parts[0], parts[1] - for i, host := range strings.Split(domains, "/") { + for i, host := range strings.Split(parts[0], "/") { if host == "" { continue } - host = strings.TrimPrefix(host, "*.") - err = netutil.ValidateDomainName(host) + err = netutil.ValidateDomainName(strings.TrimPrefix(host, "*.")) if err != nil { - return "", true, fmt.Errorf("domain at index %d: %w", i, err) + return "", domains, fmt.Errorf("domain at index %d: %w", i, err) } + + domains = append(domains, host) } - return upstream, true, nil + return parts[1], domains, nil } -// excFunc is a signature of function to check if upstream exchanges correctly. -type excFunc func(u upstream.Upstream) (err error) +// healthCheckFunc is a signature of function to check if upstream exchanges +// properly. +type healthCheckFunc func(u upstream.Upstream) (err error) // checkDNSUpstreamExc checks if the DNS upstream exchanges correctly. func checkDNSUpstreamExc(u upstream.Upstream) (err error) { + // testTLD is the special-use fully-qualified domain name for testing the + // DNS server reachability. + // + // See https://datatracker.ietf.org/doc/html/rfc6761#section-6.2. + const testTLD = "test." + req := &dns.Msg{ MsgHdr: dns.MsgHdr{ Id: dns.Id(), RecursionDesired: true, }, Question: []dns.Question{{ - Name: "google-public-dns-a.google.com.", + Name: testTLD, Qtype: dns.TypeA, Qclass: dns.ClassINET, }}, @@ -541,12 +555,8 @@ func checkDNSUpstreamExc(u upstream.Upstream) (err error) { reply, err = u.Exchange(req) if err != nil { return fmt.Errorf("couldn't communicate with upstream: %w", err) - } - - if len(reply.Answer) != 1 { - return fmt.Errorf("wrong response") - } else if a, ok := reply.Answer[0].(*dns.A); !ok || !a.A.Equal(net.IP{8, 8, 8, 8}) { - return fmt.Errorf("wrong response") + } else if len(reply.Answer) != 0 { + return errors.Error("wrong response") } return nil @@ -554,14 +564,22 @@ func checkDNSUpstreamExc(u upstream.Upstream) (err error) { // checkPrivateUpstreamExc checks if the upstream for resolving private // addresses exchanges correctly. +// +// TODO(e.burkov): Think about testing the ip6.arpa. as well. func checkPrivateUpstreamExc(u upstream.Upstream) (err error) { + // inAddrArpaTLD is the special-use fully-qualified domain name for PTR IP + // address resolution. + // + // See https://datatracker.ietf.org/doc/html/rfc1035#section-3.5. + const inAddrArpaTLD = "in-addr.arpa." + req := &dns.Msg{ MsgHdr: dns.MsgHdr{ Id: dns.Id(), RecursionDesired: true, }, Question: []dns.Question{{ - Name: "1.0.0.127.in-addr.arpa.", + Name: inAddrArpaTLD, Qtype: dns.TypePTR, Qclass: dns.ClassINET, }}, @@ -574,46 +592,66 @@ func checkPrivateUpstreamExc(u upstream.Upstream) (err error) { return nil } -func checkDNS(input string, bootstrap []string, timeout time.Duration, ef excFunc) (err error) { - if IsCommentOrEmpty(input) { +// domainSpecificTestError is a wrapper for errors returned by checkDNS to mark +// the tested upstream domain-specific and therefore consider its errors +// non-critical. +// +// TODO(a.garipov): Some common mechanism of distinguishing between errors and +// warnings (non-critical errors) is desired. +type domainSpecificTestError struct { + error +} + +// checkDNS checks the upstream server defined by upstreamConfigStr using +// healthCheck for actually exchange messages. It uses bootstrap to resolve the +// upstream's address. +func checkDNS( + upstreamConfigStr string, + bootstrap []string, + timeout time.Duration, + healthCheck healthCheckFunc, +) (err error) { + if IsCommentOrEmpty(upstreamConfigStr) { return nil } // Separate upstream from domains list. - var useDefault bool - if useDefault, err = validateUpstream(input); err != nil { + upstreamAddr, domains, err := separateUpstream(upstreamConfigStr) + if err != nil { return fmt.Errorf("wrong upstream format: %w", err) } - // No need to check this DNS server. - if !useDefault { + useDefault, err := validateUpstream(upstreamAddr, domains) + if err != nil { + return fmt.Errorf("wrong upstream format: %w", err) + } else if useDefault { return nil } - if input, _, err = separateUpstream(input); err != nil { - return fmt.Errorf("wrong upstream format: %w", err) - } - if len(bootstrap) == 0 { bootstrap = defaultBootstrap } - log.Debug("checking if upstream %s works", input) + log.Debug("dnsforward: checking if upstream %q works", upstreamAddr) - var u upstream.Upstream - u, err = upstream.AddressToUpstream(input, &upstream.Options{ + u, err := upstream.AddressToUpstream(upstreamAddr, &upstream.Options{ Bootstrap: bootstrap, Timeout: timeout, }) if err != nil { - return fmt.Errorf("failed to choose upstream for %q: %w", input, err) + return fmt.Errorf("failed to choose upstream for %q: %w", upstreamAddr, err) } - if err = ef(u); err != nil { - return fmt.Errorf("upstream %q fails to exchange: %w", input, err) + if err = healthCheck(u); err != nil { + err = fmt.Errorf("upstream %q fails to exchange: %w", upstreamAddr, err) + if domains != nil { + return domainSpecificTestError{error: err} + } + + return err } - log.Debug("upstream %s is ok", input) + log.Debug("dnsforward: upstream %q is ok", upstreamAddr) return nil } @@ -636,6 +674,9 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) { if err != nil { log.Info("%v", err) result[host] = err.Error() + if _, ok := err.(domainSpecificTestError); ok { + result[host] = fmt.Sprintf("WARNING: %s", result[host]) + } continue } @@ -651,6 +692,9 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) { // above, we rewriting the error for it. These cases should be // handled properly instead. result[host] = err.Error() + if _, ok := err.(domainSpecificTestError); ok { + result[host] = fmt.Sprintf("WARNING: %s", result[host]) + } continue } diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index f468f7ae..4042c57c 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -185,7 +185,8 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { wantSet: "", }, { name: "upstream_dns_bad", - wantSet: `validating upstream servers: bad ipport address "!!!": ` + + wantSet: `validating upstream servers: ` + + `validating upstream "!!!": bad ipport address "!!!": ` + `address !!!: missing port in address`, }, { name: "bootstraps_bad", @@ -256,112 +257,6 @@ func TestIsCommentOrEmpty(t *testing.T) { } } -func TestValidateUpstream(t *testing.T) { - testCases := []struct { - wantDef assert.BoolAssertionFunc - name string - upstream string - wantErr string - }{{ - wantDef: assert.True, - name: "invalid", - upstream: "1.2.3.4.5", - wantErr: `bad ipport address "1.2.3.4.5": address 1.2.3.4.5: missing port in address`, - }, { - wantDef: assert.True, - name: "invalid", - upstream: "123.3.7m", - wantErr: `bad ipport address "123.3.7m": address 123.3.7m: missing port in address`, - }, { - wantDef: assert.True, - name: "invalid", - upstream: "htttps://google.com/dns-query", - wantErr: `wrong protocol`, - }, { - wantDef: assert.True, - name: "invalid", - upstream: "[/host.com]tls://dns.adguard.com", - wantErr: `bad upstream for domain "[/host.com]tls://dns.adguard.com": missing separator`, - }, { - wantDef: assert.True, - name: "invalid", - upstream: "[host.ru]#", - wantErr: `bad ipport address "[host.ru]#": address [host.ru]#: missing port in address`, - }, { - wantDef: assert.True, - name: "valid_default", - upstream: "1.1.1.1", - wantErr: ``, - }, { - wantDef: assert.True, - name: "valid_default", - upstream: "tls://1.1.1.1", - wantErr: ``, - }, { - wantDef: assert.True, - name: "valid_default", - upstream: "https://dns.adguard.com/dns-query", - wantErr: ``, - }, { - wantDef: assert.True, - name: "valid_default", - upstream: "sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20", - wantErr: ``, - }, { - wantDef: assert.True, - name: "default_udp_host", - upstream: "udp://dns.google", - }, { - wantDef: assert.True, - name: "default_udp_ip", - upstream: "udp://8.8.8.8", - }, { - wantDef: assert.False, - name: "valid", - upstream: "[/host.com/]1.1.1.1", - wantErr: ``, - }, { - wantDef: assert.False, - name: "valid", - upstream: "[//]tls://1.1.1.1", - wantErr: ``, - }, { - wantDef: assert.False, - name: "valid", - upstream: "[/www.host.com/]#", - wantErr: ``, - }, { - wantDef: assert.False, - name: "valid", - upstream: "[/host.com/google.com/]8.8.8.8", - wantErr: ``, - }, { - wantDef: assert.False, - name: "valid", - upstream: "[/host/]sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20", - wantErr: ``, - }, { - wantDef: assert.False, - name: "idna", - upstream: "[/пример.рф/]8.8.8.8", - wantErr: ``, - }, { - wantDef: assert.False, - name: "bad_domain", - upstream: "[/!/]8.8.8.8", - wantErr: `bad upstream for domain "[/!/]8.8.8.8": domain at index 0: ` + - `bad domain name "!": bad domain name label "!": bad domain name label rune '!'`, - }} - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - defaultUpstream, err := validateUpstream(tc.upstream) - testutil.AssertErrorMsg(t, tc.wantErr, err) - tc.wantDef(t, defaultUpstream) - }) - } -} - func TestValidateUpstreams(t *testing.T) { testCases := []struct { name string @@ -376,7 +271,7 @@ func TestValidateUpstreams(t *testing.T) { wantErr: ``, set: []string{"# comment"}, }, { - name: "valid_no_default", + name: "no_default", wantErr: `no default upstreams specified`, set: []string{ "[/host.com/]1.1.1.1", @@ -386,7 +281,7 @@ func TestValidateUpstreams(t *testing.T) { "[/host/]sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20", }, }, { - name: "valid_with_default", + name: "with_default", wantErr: ``, set: []string{ "[/host.com/]1.1.1.1", @@ -398,8 +293,46 @@ func TestValidateUpstreams(t *testing.T) { }, }, { name: "invalid", - wantErr: `cannot prepare the upstream dhcp://fake.dns ([]): unsupported url scheme: dhcp`, + wantErr: `validating upstream "dhcp://fake.dns": wrong protocol`, set: []string{"dhcp://fake.dns"}, + }, { + name: "invalid", + wantErr: `validating upstream "1.2.3.4.5": bad ipport address "1.2.3.4.5": address 1.2.3.4.5: missing port in address`, + set: []string{"1.2.3.4.5"}, + }, { + name: "invalid", + wantErr: `validating upstream "123.3.7m": bad ipport address "123.3.7m": address 123.3.7m: missing port in address`, + set: []string{"123.3.7m"}, + }, { + name: "invalid", + wantErr: `bad upstream for domain "[/host.com]tls://dns.adguard.com": missing separator`, + set: []string{"[/host.com]tls://dns.adguard.com"}, + }, { + name: "invalid", + wantErr: `validating upstream "[host.ru]#": bad ipport address "[host.ru]#": address [host.ru]#: missing port in address`, + set: []string{"[host.ru]#"}, + }, { + name: "valid_default", + wantErr: ``, + set: []string{ + "1.1.1.1", + "tls://1.1.1.1", + "https://dns.adguard.com/dns-query", + "sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20", + "udp://dns.google", + "udp://8.8.8.8", + "[/host.com/]1.1.1.1", + "[//]tls://1.1.1.1", + "[/www.host.com/]#", + "[/host.com/google.com/]8.8.8.8", + "[/host/]sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20", + "[/пример.рф/]8.8.8.8", + }, + }, { + name: "bad_domain", + wantErr: `bad upstream for domain "[/!/]8.8.8.8": domain at index 0: ` + + `bad domain name "!": bad domain name label "!": bad domain name label rune '!'`, + set: []string{"[/!/]8.8.8.8"}, }} for _, tc := range testCases {