Skip to content

Commit

Permalink
Pull request: 4517 domain specific test
Browse files Browse the repository at this point in the history 
Merge in DNS/adguard-home from 4517-domain-specific-test to master

Updates #4517.

Squashed commit of the following:

commit 03a803f
Merge: 8ea2417 f5959a0
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 19:17:28 2022 +0300

    Merge branch 'master' into 4517-domain-specific-test

commit 8ea2417
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:44:26 2022 +0300

    all: log changes, imp docs

commit aa74c8b
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 18:07:12 2022 +0300

    dnsforward: imp logging

commit 02dccca
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 17:24:08 2022 +0300

    all: imp code, docs

commit 3b21067
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Jul 29 13:34:55 2022 +0300

    client: add warning toast

commit ea2272d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 20:11:55 2022 +0300

    dnsforward: imp err msg, docs

commit fd9ee82
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 19:24:58 2022 +0300

    dnsforward: test doain specific upstreams

commit 9a83ebf
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Jul 28 18:22:49 2022 +0300

    dnsforward: merge some logic
  • Loading branch information
@EugeneOne1
EugeneOne1 committed Jul 29, 2022
1 parent f5959a0 commit f32da12
Show file tree
Hide file tree
Showing 5 changed files with 151 additions and 168 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]).
Expand All @@ -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

Expand Down
1 change: 1 addition & 0 deletions client/src/__locales/en.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 4 additions & 2 deletions client/src/actions/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
}

Expand Down
156 changes: 100 additions & 56 deletions internal/dnsforward/http.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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
}

Expand Down Expand Up @@ -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
}

Expand All @@ -485,53 +491,61 @@ 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:], "/]")
switch len(parts) {
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,
}},
Expand All @@ -541,27 +555,31 @@ 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
}

// 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,
}},
Expand All @@ -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 {
return nil
}

if input, _, err = separateUpstream(input); err != nil {
useDefault, err := validateUpstream(upstreamAddr, domains)
if err != nil {
return fmt.Errorf("wrong upstream format: %w", err)
} else if useDefault {
return nil
}

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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down
Loading

0 comments on commit f32da12

Please sign in to comment.