From 62ec0d5adc1f5c0281de2299b1c501b1d0393f1f Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Fri, 27 Oct 2023 20:18:29 +0300 Subject: [PATCH] Pull request 2052: 4977-multiple-domain-specific-upstreams Updates #4977. Squashed commit of the following: commit da28c1b508b1aa4838d753fbb5fcac64a5fcebb9 Author: Stanislav Chzhen Date: Fri Oct 27 17:24:38 2023 +0300 all: fix typo commit d6bca6b252c9bd264737c93072869499afa24864 Author: Stanislav Chzhen Date: Fri Oct 27 14:44:20 2023 +0300 all: add todo commit 30875515942c58881305aa963220d57d31e0e67d Author: Stanislav Chzhen Date: Wed Oct 25 20:00:17 2023 +0300 all: imp docs commit 04003c342fcf82aeb671938fb89592fd6baff16d Author: Stanislav Chzhen Date: Wed Oct 25 16:59:14 2023 +0300 all: multiple domain specific upstreams --- CHANGELOG.md | 6 + client/src/__locales/en.json | 1 + .../Settings/Dns/Upstream/Examples.js | 16 ++ go.mod | 2 +- go.sum | 4 +- internal/dnsforward/access.go | 2 + internal/dnsforward/http.go | 226 ++++++++++-------- internal/dnsforward/http_test.go | 28 ++- .../TestDNSForwardHTTP_handleSetConfig.json | 42 ++++ 9 files changed, 222 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39124a66..b67bc8cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,11 @@ See also the [v0.107.41 GitHub milestone][ms-v0.107.41]. NOTE: Add new changes BELOW THIS COMMENT. --> +### Added + +- Ability to specify multiple domain specific upstreams per line, e.g. + `[/domain1/../domain2/]upstream1 upstream2 .. upstreamN` ([#4977]). + ### Fixed - `$important,dnsrewrite` rules do not take precedence over allowlist rules @@ -30,6 +35,7 @@ NOTE: Add new changes BELOW THIS COMMENT. - Dark mode DNS rewrite background ([#6329]). - Issues with QUIC and HTTP/3 upstreams on Linux ([#6335]). +[#4977]: https://github.com/AdguardTeam/AdGuardHome/issues/4977 [#6204]: https://github.com/AdguardTeam/AdGuardHome/issues/6204 [#6329]: https://github.com/AdguardTeam/AdGuardHome/issues/6329 [#6335]: https://github.com/AdguardTeam/AdGuardHome/issues/6335 diff --git a/client/src/__locales/en.json b/client/src/__locales/en.json index 5f5a9ca8..f7c572ef 100644 --- a/client/src/__locales/en.json +++ b/client/src/__locales/en.json @@ -1,6 +1,7 @@ { "client_settings": "Client settings", "example_upstream_reserved": "an upstream <0>for specific domains;", + "example_multiple_upstreams_reserved": "multiple upstreams <0>for specific domains;", "example_upstream_comment": "a comment.", "upstream_parallel": "Use parallel queries to speed up resolving by querying all upstream servers simultaneously.", "parallel_requests": "Parallel requests", diff --git a/client/src/components/Settings/Dns/Upstream/Examples.js b/client/src/components/Settings/Dns/Upstream/Examples.js index a975e444..c35d65cc 100644 --- a/client/src/components/Settings/Dns/Upstream/Examples.js +++ b/client/src/components/Settings/Dns/Upstream/Examples.js @@ -137,6 +137,22 @@ const Examples = (props) => ( example_upstream_reserved +
  • + [/example.local/]94.140.14.140 2a10:50c0::1:ff: + Link + , + ]} + > + example_multiple_upstreams_reserved + +
  • {COMMENT_LINE_DEFAULT_TOKEN} comment: example_upstream_comment diff --git a/go.mod b/go.mod index 19be8199..289d740c 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/AdguardTeam/AdGuardHome go 1.20 require ( - github.com/AdguardTeam/dnsproxy v0.56.2 + github.com/AdguardTeam/dnsproxy v0.56.3 github.com/AdguardTeam/golibs v0.17.2 github.com/AdguardTeam/urlfilter v0.17.3 github.com/NYTimes/gziphandler v1.1.1 diff --git a/go.sum b/go.sum index 9fda18ac..11cd3cca 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/AdguardTeam/dnsproxy v0.56.2 h1:+k1iUmp05QIqkgXWyPn70fki4FouHe6vHIyHguelKao= -github.com/AdguardTeam/dnsproxy v0.56.2/go.mod h1:ZvkbM71HwpilgkCnTubDiR4Ba6x5Qvnhy2iasMWaTDM= +github.com/AdguardTeam/dnsproxy v0.56.3 h1:WP1FooLfZQPHEH2SuwMtJsOurDt32rubGx0OddcsKT0= +github.com/AdguardTeam/dnsproxy v0.56.3/go.mod h1:ZvkbM71HwpilgkCnTubDiR4Ba6x5Qvnhy2iasMWaTDM= github.com/AdguardTeam/golibs v0.17.2 h1:vg6wHMjUKscnyPGRvxS5kAt7Uw4YxcJiITZliZ476W8= github.com/AdguardTeam/golibs v0.17.2/go.mod h1:DKhCIXHcUYtBhU8ibTLKh1paUL96n5zhQBlx763sj+U= github.com/AdguardTeam/urlfilter v0.17.3 h1:fg/ObbnO0Cv6aw0tW6N/ETDMhhNvmcUUOZ7HlmKC3rw= diff --git a/internal/dnsforward/access.go b/internal/dnsforward/access.go index 2c0d0dc4..21b4a758 100644 --- a/internal/dnsforward/access.go +++ b/internal/dnsforward/access.go @@ -182,6 +182,7 @@ func (s *Server) accessListJSON() (j accessListJSON) { } } +// handleAccessList handles requests to the GET /control/access/list endpoint. func (s *Server) handleAccessList(w http.ResponseWriter, r *http.Request) { aghhttp.WriteJSONResponseOK(w, r, s.accessListJSON()) } @@ -224,6 +225,7 @@ func validateStrUniq(clients []string) (uc aghalg.UniqChecker[string], err error return uc, uc.Validate() } +// handleAccessSet handles requests to the POST /control/access/set endpoint. func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) { list := &accessListJSON{} err := json.NewDecoder(r.Body).Decode(&list) diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index 02432ce2..99ced3ef 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -7,6 +7,7 @@ import ( "net/http" "net/netip" "strings" + "sync" "time" "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" @@ -444,19 +445,10 @@ 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) - } + err = validateUpstreamConfig(upstreams) + if err != nil { + // Don't wrap the error since it's informative enough as is. + return nil, err } conf, err = proxy.ParseUpstreamsConfig( @@ -467,6 +459,7 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro }, ) if err != nil { + // Don't wrap the error since it's informative enough as is. return nil, err } else if len(conf.Upstreams) == 0 { return nil, errors.Error("no default upstreams specified") @@ -475,6 +468,31 @@ func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err erro return conf, nil } +// validateUpstreamConfig validates each upstream from the upstream +// configuration and returns an error if any upstream is invalid. +// +// TODO(e.burkov): Move into aghnet or even into dnsproxy. +func validateUpstreamConfig(conf []string) (err error) { + for _, u := range conf { + 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 err + } + + for _, addr := range ups { + _, err = validateUpstream(addr, domains) + if err != nil { + return fmt.Errorf("validating upstream %q: %w", addr, err) + } + } + } + + return nil +} + // ValidateUpstreams validates each upstream and returns an error if any // upstream is invalid or if there are no default upstreams specified. // @@ -567,12 +585,12 @@ func validateUpstream(u string, domains []string) (useDefault bool, err error) { return false, err } -// separateUpstream returns the upstream and the specified domains. domains is -// nil when the upstream is not domains-specific. Otherwise it may also be +// separateUpstream returns the upstreams 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) { +func separateUpstream(upstreamStr string) (upstreams, domains []string, err error) { if !strings.HasPrefix(upstreamStr, "[/") { - return upstreamStr, nil, nil + return []string{upstreamStr}, nil, nil } defer func() { err = errors.Annotate(err, "bad upstream for domain %q: %w", upstreamStr) }() @@ -582,9 +600,9 @@ func separateUpstream(upstreamStr string) (ups string, domains []string, err err case 2: // Go on. case 1: - return "", nil, errors.Error("missing separator") + return nil, nil, errors.Error("missing separator") default: - return "", []string{}, errors.Error("duplicated separator") + return nil, nil, errors.Error("duplicated separator") } for i, host := range strings.Split(parts[0], "/") { @@ -594,13 +612,13 @@ func separateUpstream(upstreamStr string) (ups string, domains []string, err err err = netutil.ValidateDomainName(strings.TrimPrefix(host, "*.")) if err != nil { - return "", domains, fmt.Errorf("domain at index %d: %w", i, err) + return nil, nil, fmt.Errorf("domain at index %d: %w", i, err) } domains = append(domains, host) } - return parts[1], domains, nil + return strings.Fields(parts[1]), domains, nil } // healthCheckFunc is a signature of function to check if upstream exchanges @@ -683,30 +701,65 @@ func (err domainSpecificTestError) Error() (msg string) { return fmt.Sprintf("WARNING: %s", err.error) } -// parseUpstreamLine parses line and creates the [upstream.Upstream] using opts -// and information from [s.dnsFilter.EtcHosts]. It returns an error if the line -// is not a valid upstream line, see [upstream.AddressToUpstream]. It's a -// caller's responsibility to close u. -func (s *Server) parseUpstreamLine( +// checkDNS parses line, creates DNS upstreams using opts, and checks if the +// upstreams are exchanging correctly. It returns a map where key is an +// upstream address and value is "OK", if the upstream exchanges correctly, or +// text of the error. +func (s *Server) checkDNS( line string, opts *upstream.Options, -) (u upstream.Upstream, specific bool, err error) { - // Separate upstream from domains list. - upstreamAddr, domains, err := separateUpstream(line) + check healthCheckFunc, +) (result map[string]string) { + result = map[string]string{} + upstreams, domains, err := separateUpstream(line) if err != nil { - return nil, false, fmt.Errorf("wrong upstream format: %w", err) + return nil } - specific = len(domains) > 0 + specific := len(domains) > 0 - useDefault, err := validateUpstream(upstreamAddr, domains) - if err != nil { - return nil, specific, fmt.Errorf("wrong upstream format: %w", err) - } else if useDefault { - return nil, specific, nil + for _, upstreamAddr := range upstreams { + var useDefault bool + useDefault, err = validateUpstream(upstreamAddr, domains) + if err != nil { + err = fmt.Errorf("wrong upstream format: %w", err) + result[upstreamAddr] = err.Error() + + continue + } + + if useDefault { + continue + } + + log.Debug("dnsforward: checking if upstream %q works", upstreamAddr) + + err = s.checkUpstreamAddr(upstreamAddr, specific, opts, check) + if err != nil { + result[upstreamAddr] = err.Error() + } else { + result[upstreamAddr] = "OK" + } } - log.Debug("dnsforward: checking if upstream %q works", upstreamAddr) + return result +} + +// checkUpstreamAddr creates the DNS upstream using opts and information from +// [s.dnsFilter.EtcHosts]. Checks if the DNS upstream exchanges correctly. It +// returns an error if addr is not valid DNS upstream address or the upstream +// is not exchanging correctly. +func (s *Server) checkUpstreamAddr( + addr string, + specific bool, + opts *upstream.Options, + check healthCheckFunc, +) (err error) { + defer func() { + if err != nil && specific { + err = domainSpecificTestError{error: err} + } + }() opts = &upstream.Options{ Bootstrap: opts.Bootstrap, @@ -716,42 +769,25 @@ func (s *Server) parseUpstreamLine( // dnsFilter can be nil during application update. if s.dnsFilter != nil { - recs := s.dnsFilter.EtcHostsRecords(extractUpstreamHost(upstreamAddr)) + recs := s.dnsFilter.EtcHostsRecords(extractUpstreamHost(addr)) for _, rec := range recs { opts.ServerIPAddrs = append(opts.ServerIPAddrs, rec.Addr.AsSlice()) } sortNetIPAddrs(opts.ServerIPAddrs, opts.PreferIPv6) } - u, err = upstream.AddressToUpstream(upstreamAddr, opts) + + u, err := upstream.AddressToUpstream(addr, opts) if err != nil { - return nil, specific, fmt.Errorf("creating upstream for %q: %w", upstreamAddr, err) + return fmt.Errorf("creating upstream for %q: %w", addr, err) } - return u, specific, nil -} - -func (s *Server) checkDNS(line string, opts *upstream.Options, check healthCheckFunc) (err error) { - if IsCommentOrEmpty(line) { - return nil - } - - var u upstream.Upstream - var specific bool - defer func() { - if err != nil && specific { - err = domainSpecificTestError{error: err} - } - }() - - u, specific, err = s.parseUpstreamLine(line, opts) - if err != nil || u == nil { - return err - } defer func() { err = errors.WithDeferred(err, u.Close()) }() return check(u) } +// handleTestUpstreamDNS handles requests to the POST /control/test_upstream_dns +// endpoint. func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) { req := &upstreamJSON{} err := json.NewDecoder(r.Body).Decode(req) @@ -761,6 +797,10 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) { return } + req.Upstreams = stringutil.FilterOut(req.Upstreams, IsCommentOrEmpty) + req.FallbackDNS = stringutil.FilterOut(req.FallbackDNS, IsCommentOrEmpty) + req.PrivateUpstreams = stringutil.FilterOut(req.PrivateUpstreams, IsCommentOrEmpty) + opts := &upstream.Options{ Bootstrap: req.BootstrapDNS, Timeout: s.conf.UpstreamTimeout, @@ -770,54 +810,44 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) { opts.Bootstrap = defaultBootstrap } - type upsCheckResult = struct { - err error - host string + wg := &sync.WaitGroup{} + m := &sync.Map{} + + // TODO(s.chzhen): Separate to a different structure/file. + worker := func(upstreamLine string, check healthCheckFunc) { + defer log.OnPanic("dnsforward: checking upstreams") + + res := s.checkDNS(upstreamLine, opts, check) + for ups, status := range res { + m.Store(ups, status) + } + + wg.Done() } - req.Upstreams = stringutil.FilterOut(req.Upstreams, IsCommentOrEmpty) - req.FallbackDNS = stringutil.FilterOut(req.FallbackDNS, IsCommentOrEmpty) - req.PrivateUpstreams = stringutil.FilterOut(req.PrivateUpstreams, IsCommentOrEmpty) - - upsNum := len(req.Upstreams) + len(req.FallbackDNS) + len(req.PrivateUpstreams) - result := make(map[string]string, upsNum) - resCh := make(chan upsCheckResult, upsNum) + wg.Add(len(req.Upstreams) + len(req.FallbackDNS) + len(req.PrivateUpstreams)) for _, ups := range req.Upstreams { - go func(ups string) { - resCh <- upsCheckResult{ - host: ups, - err: s.checkDNS(ups, opts, checkDNSUpstreamExc), - } - }(ups) + go worker(ups, checkDNSUpstreamExc) } for _, ups := range req.FallbackDNS { - go func(ups string) { - resCh <- upsCheckResult{ - host: ups, - err: s.checkDNS(ups, opts, checkDNSUpstreamExc), - } - }(ups) + go worker(ups, checkDNSUpstreamExc) } for _, ups := range req.PrivateUpstreams { - go func(ups string) { - resCh <- upsCheckResult{ - host: ups, - err: s.checkDNS(ups, opts, checkPrivateUpstreamExc), - } - }(ups) + go worker(ups, checkPrivateUpstreamExc) } - for i := 0; i < upsNum; i++ { - // TODO(e.burkov): The upstreams used for both common and private - // resolving should be reported separately. - pair := <-resCh - if pair.err != nil { - result[pair.host] = pair.err.Error() - } else { - result[pair.host] = "OK" - } - } + wg.Wait() + + result := map[string]string{} + m.Range(func(k, v any) bool { + ups := k.(string) + status := v.(string) + + result[ups] = status + + return true + }) aghhttp.WriteJSONResponseOK(w, r, result) } diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 8524acc4..dfd1862a 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -49,13 +49,18 @@ func loadTestData(t *testing.T, casesFileName string, cases any) { require.NoError(t, err) } -const jsonExt = ".json" +const ( + jsonExt = ".json" + + // testBlockedRespTTL is the TTL for blocked responses to use in tests. + testBlockedRespTTL = 10 +) func TestDNSForwardHTTP_handleGetConfig(t *testing.T) { filterConf := &filtering.Config{ ProtectionEnabled: true, BlockingMode: filtering.BlockingModeDefault, - BlockedResponseTTL: 10, + BlockedResponseTTL: testBlockedRespTTL, SafeBrowsingEnabled: true, SafeBrowsingCacheSize: 1000, SafeSearchConf: filtering.SafeSearchConfig{Enabled: true}, @@ -133,7 +138,7 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { filterConf := &filtering.Config{ ProtectionEnabled: true, BlockingMode: filtering.BlockingModeDefault, - BlockedResponseTTL: 10, + BlockedResponseTTL: testBlockedRespTTL, SafeBrowsingEnabled: true, SafeBrowsingCacheSize: 1000, SafeSearchConf: filtering.SafeSearchConfig{Enabled: true}, @@ -229,6 +234,9 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { }, { name: "blocked_response_ttl", wantSet: "", + }, { + name: "multiple_domain_specific_upstreams", + wantSet: "", }} var data map[string]struct { @@ -250,6 +258,7 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { s.dnsFilter.SetBlockingMode(filtering.BlockingModeDefault, netip.Addr{}, netip.Addr{}) s.conf = defaultConf s.conf.Config.EDNSClientSubnet = &EDNSClientSubnet{} + s.dnsFilter.SetBlockedResponseTTL(testBlockedRespTTL) }) rBody := io.NopCloser(bytes.NewReader(caseData.Req)) @@ -547,7 +556,7 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) { "upstream_dns": []string{"[/domain.example/]" + badUps}, }, wantResp: map[string]any{ - "[/domain.example/]" + badUps: `WARNING: couldn't communicate ` + + badUps: `WARNING: couldn't communicate ` + `with upstream: exchanging with ` + badUps + ` over tcp: ` + `dns: id mismatch`, }, @@ -585,6 +594,17 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) { goodUps: "OK", }, name: "fallback_comment_mix", + }, { + body: map[string]any{ + "upstream_dns": []string{"[/domain.example/]" + goodUps + " " + badUps}, + }, + wantResp: map[string]any{ + goodUps: "OK", + badUps: `WARNING: couldn't communicate ` + + `with upstream: exchanging with ` + badUps + ` over tcp: ` + + `dns: id mismatch`, + }, + name: "multiple_domain_specific_upstreams", }} for _, tc := range testCases { diff --git a/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json b/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json index b20e1adc..b94bb82f 100644 --- a/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json +++ b/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json @@ -839,5 +839,47 @@ "edns_cs_use_custom": false, "edns_cs_custom_ip": "" } + }, + "multiple_domain_specific_upstreams": { + "req": { + "upstream_dns": [ + "8.8.8.8:77", + "[/example.com/]8.8.4.4:77 9.9.9.10 https://1.1.1.1" + ] + }, + "want": { + "upstream_dns": [ + "8.8.8.8:77", + "[/example.com/]8.8.4.4:77 9.9.9.10 https://1.1.1.1" + ], + "upstream_dns_file": "", + "bootstrap_dns": [ + "9.9.9.10", + "149.112.112.10", + "2620:fe::10", + "2620:fe::fe:10" + ], + "fallback_dns": [], + "protection_enabled": true, + "protection_disabled_until": null, + "ratelimit": 0, + "blocking_mode": "default", + "blocking_ipv4": "", + "blocking_ipv6": "", + "blocked_response_ttl": 10, + "edns_cs_enabled": false, + "dnssec_enabled": false, + "disable_ipv6": false, + "upstream_mode": "", + "cache_size": 0, + "cache_ttl_min": 0, + "cache_ttl_max": 0, + "cache_optimistic": false, + "resolve_clients": false, + "use_private_ptr_resolvers": false, + "local_ptr_upstreams": [], + "edns_cs_use_custom": false, + "edns_cs_custom_ip": "" + } } }