diff --git a/client/src/__locales/en.json b/client/src/__locales/en.json index 112928be..ecac7f0d 100644 --- a/client/src/__locales/en.json +++ b/client/src/__locales/en.json @@ -236,6 +236,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_parsing_error_toast": "Section {{section}}: line {{line}}: could not be used, please check that you've written it correctly", "dns_test_warning_toast": "Upstream \"{{key}}\" does not respond to test requests and may not work properly", "unblock": "Unblock", "block": "Block", diff --git a/client/src/actions/index.js b/client/src/actions/index.js index e0e50841..c7af4fa0 100644 --- a/client/src/actions/index.js +++ b/client/src/actions/index.js @@ -403,6 +403,11 @@ export const testUpstream = ( const message = upstreamResponse[key]; if (message.startsWith('WARNING:')) { dispatch(addErrorToast({ error: i18next.t('dns_test_warning_toast', { key }) })); + } else if (message.endsWith(': parsing error')) { + const info = message.substring(0, message.indexOf(':')); + const [sectionKey, line] = info.split(' '); + const section = i18next.t(sectionKey); + dispatch(addErrorToast({ error: i18next.t('dns_test_parsing_error_toast', { section, line }) })); } else if (message !== 'OK') { dispatch(addErrorToast({ error: i18next.t('dns_test_not_ok_toast', { key }) })); } diff --git a/go.mod b/go.mod index b7227ec9..c767ab70 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.63.1 + github.com/AdguardTeam/dnsproxy v0.64.1 github.com/AdguardTeam/golibs v0.19.0 github.com/AdguardTeam/urlfilter v0.17.3 github.com/NYTimes/gziphandler v1.1.1 diff --git a/go.sum b/go.sum index 0c2a46dd..465d60f5 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -github.com/AdguardTeam/dnsproxy v0.63.1 h1:CilxSuLYcuYpbPCGB7w41UUqWRMu3dvj4c9TvkIrpBg= -github.com/AdguardTeam/dnsproxy v0.63.1/go.mod h1:dRRAFOjrq4QYM92jGs4lt4BoY0Dm3EY3HkaleoM2Feo= +github.com/AdguardTeam/dnsproxy v0.64.1 h1:Cv2nyNYjUeUxouTQmM0aVTR7LWuhCr/Lu+h3DIAWhG8= +github.com/AdguardTeam/dnsproxy v0.64.1/go.mod h1:dRRAFOjrq4QYM92jGs4lt4BoY0Dm3EY3HkaleoM2Feo= github.com/AdguardTeam/golibs v0.19.0 h1:y/x+Xn3pDg1ZfQ+QEZapPJqaeVYUIMp/EODMtVhn7PM= github.com/AdguardTeam/golibs v0.19.0/go.mod h1:3WunclLLfrVAq7fYQRhd6f168FHOEMssnipVXCxDL/w= github.com/AdguardTeam/urlfilter v0.17.3 h1:fg/ObbnO0Cv6aw0tW6N/ETDMhhNvmcUUOZ7HlmKC3rw= diff --git a/internal/dnsforward/configvalidator.go b/internal/dnsforward/configvalidator.go index b55f53cb..fe5d0c80 100644 --- a/internal/dnsforward/configvalidator.go +++ b/internal/dnsforward/configvalidator.go @@ -2,54 +2,56 @@ package dnsforward import ( "fmt" - "strings" "sync" + "github.com/AdguardTeam/dnsproxy/proxy" "github.com/AdguardTeam/dnsproxy/upstream" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" "github.com/miekg/dns" - "golang.org/x/exp/slices" ) -// upstreamConfigValidator parses the [*proxy.UpstreamConfig] and checks the -// actual DNS availability of each upstream. +// upstreamConfigValidator parses each section of an upstream configuration into +// a corresponding [*proxy.UpstreamConfig] and checks the actual DNS +// availability of each upstream. type upstreamConfigValidator struct { - // general is the general upstream configuration. - general []*upstreamResult + // generalUpstreamResults contains upstream results of a general section. + generalUpstreamResults map[string]*upstreamResult - // fallback is the fallback upstream configuration. - fallback []*upstreamResult + // fallbackUpstreamResults contains upstream results of a fallback section. + fallbackUpstreamResults map[string]*upstreamResult - // private is the private upstream configuration. - private []*upstreamResult + // privateUpstreamResults contains upstream results of a private section. + privateUpstreamResults map[string]*upstreamResult + + // generalParseResults contains parsing results of a general section. + generalParseResults []*parseResult + + // fallbackParseResults contains parsing results of a fallback section. + fallbackParseResults []*parseResult + + // privateParseResults contains parsing results of a private section. + privateParseResults []*parseResult } -// upstreamResult is a result of validation of an [upstream.Upstream] within an +// upstreamResult is a result of parsing of an [upstream.Upstream] within an // [proxy.UpstreamConfig]. type upstreamResult struct { - // server is the parsed upstream. It is nil when there was an error during - // parsing. + // server is the parsed upstream. server upstream.Upstream - // err is the error either from parsing or from checking the upstream. + // err is the upstream check error. err error - // original is the piece of configuration that have either been turned to an - // upstream or caused an error. - original string - // isSpecific is true if the upstream is domain-specific. isSpecific bool } -// compare compares two [upstreamResult]s. It returns 0 if they are equal, -1 -// if ur should be sorted before other, and 1 otherwise. -// -// TODO(e.burkov): Perhaps it makes sense to sort the results with errors near -// the end. -func (ur *upstreamResult) compare(other *upstreamResult) (res int) { - return strings.Compare(ur.original, other.original) +// parseResult contains a original piece of upstream configuration and a +// corresponding error. +type parseResult struct { + err *proxy.ParseError + original string } // newUpstreamConfigValidator parses the upstream configuration and returns a @@ -61,97 +63,99 @@ func newUpstreamConfigValidator( private []string, opts *upstream.Options, ) (cv *upstreamConfigValidator) { - cv = &upstreamConfigValidator{} + cv = &upstreamConfigValidator{ + generalUpstreamResults: map[string]*upstreamResult{}, + fallbackUpstreamResults: map[string]*upstreamResult{}, + privateUpstreamResults: map[string]*upstreamResult{}, + } - for _, line := range general { - cv.general = cv.insertLineResults(cv.general, line, opts) - } - for _, line := range fallback { - cv.fallback = cv.insertLineResults(cv.fallback, line, opts) - } - for _, line := range private { - cv.private = cv.insertLineResults(cv.private, line, opts) - } + conf, err := proxy.ParseUpstreamsConfig(general, opts) + cv.generalParseResults = collectErrResults(general, err) + insertConfResults(conf, cv.generalUpstreamResults) + + conf, err = proxy.ParseUpstreamsConfig(fallback, opts) + cv.fallbackParseResults = collectErrResults(fallback, err) + insertConfResults(conf, cv.fallbackUpstreamResults) + + conf, err = proxy.ParseUpstreamsConfig(private, opts) + cv.privateParseResults = collectErrResults(private, err) + insertConfResults(conf, cv.privateUpstreamResults) return cv } -// insertLineResults parses line and inserts the result into s. It can insert -// multiple results as well as none. -func (cv *upstreamConfigValidator) insertLineResults( - s []*upstreamResult, - line string, - opts *upstream.Options, -) (result []*upstreamResult) { - upstreams, isSpecific, err := splitUpstreamLine(line) - if err != nil { - return cv.insert(s, &upstreamResult{ - err: err, - original: line, - }) +// collectErrResults parses err and returns parsing results containing the +// original upstream configuration line and the corresponding error. err can be +// nil. +func collectErrResults(lines []string, err error) (results []*parseResult) { + if err == nil { + return nil } - for _, upstreamAddr := range upstreams { - var res *upstreamResult - if upstreamAddr != "#" { - res = cv.parseUpstream(upstreamAddr, opts) - } else if !isSpecific { - res = &upstreamResult{ - err: errNotDomainSpecific, - original: upstreamAddr, - } - } else { + // limit is a maximum length for upstream configuration lines. + const limit = 80 + + wrapper, ok := err.(errors.WrapperSlice) + if !ok { + log.Debug("dnsforward: configvalidator: unwrapping: %s", err) + + return nil + } + + errs := wrapper.Unwrap() + results = make([]*parseResult, 0, len(errs)) + for i, e := range errs { + var parseErr *proxy.ParseError + if !errors.As(e, &parseErr) { + log.Debug("dnsforward: configvalidator: inserting unexpected error %d: %s", i, err) + continue } - res.isSpecific = isSpecific - s = cv.insert(s, res) - } - - return s -} - -// insert inserts r into slice in a sorted order, except duplicates. slice must -// not be nil. -func (cv *upstreamConfigValidator) insert( - s []*upstreamResult, - r *upstreamResult, -) (result []*upstreamResult) { - i, has := slices.BinarySearchFunc(s, r, (*upstreamResult).compare) - if has { - log.Debug("dnsforward: duplicate configuration %q", r.original) - - return s - } - - return slices.Insert(s, i, r) -} - -// parseUpstream parses addr and returns the result of parsing. It returns nil -// if the specified server points at the default upstream server which is -// validated separately. -func (cv *upstreamConfigValidator) parseUpstream( - addr string, - opts *upstream.Options, -) (r *upstreamResult) { - // Check if the upstream has a valid protocol prefix. - // - // TODO(e.burkov): Validate the domain name. - if proto, _, ok := strings.Cut(addr, "://"); ok { - if !slices.Contains(protocols, proto) { - return &upstreamResult{ - err: fmt.Errorf("bad protocol %q", proto), - original: addr, - } + idx := parseErr.Idx + line := []rune(lines[idx]) + if len(line) > limit { + line = line[:limit] + line[limit-1] = '…' } + + results = append(results, &parseResult{ + original: string(line), + err: parseErr, + }) } - ups, err := upstream.AddressToUpstream(addr, opts) + return results +} - return &upstreamResult{ - server: ups, - err: err, - original: addr, +// insertConfResults parses conf and inserts the upstream result into results. +// It can insert multiple results as well as none. +func insertConfResults(conf *proxy.UpstreamConfig, results map[string]*upstreamResult) { + insertListResults(conf.Upstreams, results, false) + + for _, ups := range conf.DomainReservedUpstreams { + insertListResults(ups, results, true) + } + + for _, ups := range conf.SpecifiedDomainUpstreams { + insertListResults(ups, results, true) + } +} + +// insertListResults constructs upstream results from the upstream list and +// inserts them into results. It can insert multiple results as well as none. +func insertListResults(ups []upstream.Upstream, results map[string]*upstreamResult, specific bool) { + for _, u := range ups { + addr := u.Address() + _, ok := results[addr] + if ok { + continue + } + + results[addr] = &upstreamResult{ + server: u, + isSpecific: specific, + } } } @@ -187,35 +191,30 @@ func (cv *upstreamConfigValidator) check() { } wg := &sync.WaitGroup{} - wg.Add(len(cv.general) + len(cv.fallback) + len(cv.private)) + wg.Add(len(cv.generalUpstreamResults) + + len(cv.fallbackUpstreamResults) + + len(cv.privateUpstreamResults)) - for _, res := range cv.general { - go cv.checkSrv(res, wg, commonChecker) + for _, res := range cv.generalUpstreamResults { + go checkSrv(res, wg, commonChecker) } - for _, res := range cv.fallback { - go cv.checkSrv(res, wg, commonChecker) + for _, res := range cv.fallbackUpstreamResults { + go checkSrv(res, wg, commonChecker) } - for _, res := range cv.private { - go cv.checkSrv(res, wg, arpaChecker) + for _, res := range cv.privateUpstreamResults { + go checkSrv(res, wg, arpaChecker) } wg.Wait() } // checkSrv runs hc on the server from res, if any, and stores any occurred -// error in res. wg is always marked done in the end. It used to be called in -// a separate goroutine. -func (cv *upstreamConfigValidator) checkSrv( - res *upstreamResult, - wg *sync.WaitGroup, - hc *healthchecker, -) { +// error in res. wg is always marked done in the end. It is intended to be +// used as a goroutine. +func checkSrv(res *upstreamResult, wg *sync.WaitGroup, hc *healthchecker) { + defer log.OnPanic(fmt.Sprintf("dnsforward: checking upstream %s", res.server.Address())) defer wg.Done() - if res.server == nil { - return - } - res.err = hc.check(res.server) if res.err != nil && res.isSpecific { res.err = domainSpecificTestError{Err: res.err} @@ -225,65 +224,126 @@ func (cv *upstreamConfigValidator) checkSrv( // close closes all the upstreams that were successfully parsed. It enriches // the results with deferred closing errors. func (cv *upstreamConfigValidator) close() { - for _, slice := range [][]*upstreamResult{cv.general, cv.fallback, cv.private} { - for _, r := range slice { - if r.server != nil { - r.err = errors.WithDeferred(r.err, r.server.Close()) - } + all := []map[string]*upstreamResult{ + cv.generalUpstreamResults, + cv.fallbackUpstreamResults, + cv.privateUpstreamResults, + } + + for _, m := range all { + for _, r := range m { + r.err = errors.WithDeferred(r.err, r.server.Close()) } } } +// sections of the upstream configuration according to the text label of the +// localization. +// +// Keep in sync with client/src/__locales/en.json. +// +// TODO(s.chzhen): Refactor. +const ( + generalTextLabel = "upstream_dns" + fallbackTextLabel = "fallback_dns_title" + privateTextLabel = "local_ptr_title" +) + // status returns all the data collected during parsing, healthcheck, and // closing of the upstreams. The returned map is keyed by the original upstream // configuration piece and contains the corresponding error or "OK" if there was // no error. func (cv *upstreamConfigValidator) status() (results map[string]string) { - result := map[string]string{} + // Names of the upstream configuration sections for logging. + const ( + generalSection = "general" + fallbackSection = "fallback" + privateSection = "private" + ) - for _, res := range cv.general { - resultToStatus("general", res, result) + results = map[string]string{} + + for original, res := range cv.generalUpstreamResults { + upstreamResultToStatus(generalSection, string(original), res, results) } - for _, res := range cv.fallback { - resultToStatus("fallback", res, result) + for original, res := range cv.fallbackUpstreamResults { + upstreamResultToStatus(fallbackSection, string(original), res, results) } - for _, res := range cv.private { - resultToStatus("private", res, result) + for original, res := range cv.privateUpstreamResults { + upstreamResultToStatus(privateSection, string(original), res, results) } - return result + parseResultToStatus(generalTextLabel, generalSection, cv.generalParseResults, results) + parseResultToStatus(fallbackTextLabel, fallbackSection, cv.fallbackParseResults, results) + parseResultToStatus(privateTextLabel, privateSection, cv.privateParseResults, results) + + return results } -// resultToStatus puts "OK" or an error message from res into resMap. section -// is the name of the upstream configuration section, i.e. "general", +// upstreamResultToStatus puts "OK" or an error message from res into resMap. +// section is the name of the upstream configuration section, i.e. "general", // "fallback", or "private", and only used for logging. // // TODO(e.burkov): Currently, the HTTP handler expects that all the results are // put together in a single map, which may lead to collisions, see AG-27539. // Improve the results compilation. -func resultToStatus(section string, res *upstreamResult, resMap map[string]string) { +func upstreamResultToStatus( + section string, + original string, + res *upstreamResult, + resMap map[string]string, +) { val := "OK" if res.err != nil { val = res.err.Error() } - prevVal := resMap[res.original] + prevVal := resMap[original] switch prevVal { case "": - resMap[res.original] = val + resMap[original] = val case val: - log.Debug("dnsforward: duplicating %s config line %q", section, res.original) + log.Debug("dnsforward: duplicating %s config line %q", section, original) default: log.Debug( "dnsforward: warning: %s config line %q (%v) had different result %v", section, val, - res.original, + original, prevVal, ) } } +// parseResultToStatus puts parsing error messages from results into resMap. +// section is the name of the upstream configuration section, i.e. "general", +// "fallback", or "private", and only used for logging. +// +// Parsing error message has the following format: +// +// sectionTextLabel line: parsing error +// +// Where sectionTextLabel is a section text label of a localization and line is +// a line number. +func parseResultToStatus( + textLabel string, + section string, + results []*parseResult, + resMap map[string]string, +) { + for _, res := range results { + original := res.original + _, ok := resMap[original] + if ok { + log.Debug("dnsforward: duplicating %s parsing error %q", section, original) + + continue + } + + resMap[original] = fmt.Sprintf("%s %d: parsing error", textLabel, res.err.Idx+1) + } +} + // domainSpecificTestError is a wrapper for errors returned by checkDNS to mark // the tested upstream domain-specific and therefore consider its errors // non-critical. @@ -342,7 +402,7 @@ func (h *healthchecker) check(u upstream.Upstream) (err error) { if err != nil { return fmt.Errorf("couldn't communicate with upstream: %w", err) } else if h.ansEmpty && len(reply.Answer) > 0 { - return errWrongResponse + return errors.Error("wrong response") } return nil diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index 2c222d07..73acda6e 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -10,6 +10,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" "github.com/AdguardTeam/AdGuardHome/internal/filtering" + "github.com/AdguardTeam/dnsproxy/proxy" "github.com/AdguardTeam/dnsproxy/upstream" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" @@ -294,7 +295,7 @@ func (req *jsonDNSConfig) checkFallbacks() (err error) { return nil } - err = ValidateUpstreams(*req.Fallbacks) + _, err = proxy.ParseUpstreamsConfig(*req.Fallbacks, &upstream.Options{}) if err != nil { return fmt.Errorf("fallback servers: %w", err) } @@ -344,7 +345,7 @@ func (req *jsonDNSConfig) validate(privateNets netutil.SubnetSet) (err error) { // validateUpstreamDNSServers returns an error if any field of req is invalid. func (req *jsonDNSConfig) validateUpstreamDNSServers(privateNets netutil.SubnetSet) (err error) { if req.Upstreams != nil { - err = ValidateUpstreams(*req.Upstreams) + _, err = proxy.ParseUpstreamsConfig(*req.Upstreams, &upstream.Options{}) if err != nil { return fmt.Errorf("upstream servers: %w", err) } @@ -580,9 +581,6 @@ 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) req.BootstrapDNS = stringutil.FilterOut(req.BootstrapDNS, IsCommentOrEmpty) opts := &upstream.Options{ diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 2e28039f..b642eb2c 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -223,8 +223,9 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { wantSet: "", }, { name: "upstream_dns_bad", - wantSet: `validating dns config: ` + - `upstream servers: validating upstream "!!!": not an ip:port`, + wantSet: `validating dns config: upstream servers: parsing error at index 0: ` + + `cannot prepare the upstream: invalid address !!!: bad hostname "!!!": ` + + `bad top-level domain name label "!!!": bad top-level domain name label rune '!'`, }, { name: "bootstraps_bad", wantSet: `validating dns config: checking bootstrap a: not a bootstrap: ParseAddr("a"): ` + @@ -313,98 +314,6 @@ func TestIsCommentOrEmpty(t *testing.T) { } } -func TestValidateUpstreams(t *testing.T) { - const sdnsStamp = `sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_J` + - `S3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczE` + - `uYWRndWFyZC5jb20` - - testCases := []struct { - name string - wantErr string - set []string - }{{ - name: "empty", - wantErr: ``, - set: nil, - }, { - name: "comment", - wantErr: ``, - set: []string{"# comment"}, - }, { - name: "no_default", - wantErr: `no default upstreams specified`, - set: []string{ - "[/host.com/]1.1.1.1", - "[//]tls://1.1.1.1", - "[/www.host.com/]#", - "[/host.com/google.com/]8.8.8.8", - "[/host/]" + sdnsStamp, - }, - }, { - name: "with_default", - wantErr: ``, - set: []string{ - "[/host.com/]1.1.1.1", - "[//]tls://1.1.1.1", - "[/www.host.com/]#", - "[/host.com/google.com/]8.8.8.8", - "[/host/]" + sdnsStamp, - "8.8.8.8", - }, - }, { - name: "invalid", - wantErr: `validating upstream "dhcp://fake.dns": bad protocol "dhcp"`, - set: []string{"dhcp://fake.dns"}, - }, { - name: "invalid", - wantErr: `validating upstream "1.2.3.4.5": not an ip:port`, - set: []string{"1.2.3.4.5"}, - }, { - name: "invalid", - wantErr: `validating upstream "123.3.7m": not an ip:port`, - set: []string{"123.3.7m"}, - }, { - name: "invalid", - wantErr: `splitting upstream line "[/host.com]tls://dns.adguard.com": ` + - `missing separator`, - set: []string{"[/host.com]tls://dns.adguard.com"}, - }, { - name: "invalid", - wantErr: `validating upstream "[host.ru]#": not an ip:port`, - 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", - sdnsStamp, - "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/]" + sdnsStamp, - "[/пример.рф/]8.8.8.8", - }, - }, { - name: "bad_domain", - wantErr: `splitting upstream line "[/!/]8.8.8.8": domain at index 0: ` + - `bad domain name "!": bad top-level domain name label "!": ` + - `bad top-level domain name label rune '!'`, - set: []string{"[/!/]8.8.8.8"}, - }} - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - err := ValidateUpstreams(tc.set) - testutil.AssertErrorMsg(t, tc.wantErr, err) - }) - } -} - func TestValidateUpstreamsPrivate(t *testing.T) { ss := netutil.SubnetSetFunc(netutil.IsLocallyServed) diff --git a/internal/dnsforward/upstreams.go b/internal/dnsforward/upstreams.go index 5fed582a..b28b5f90 100644 --- a/internal/dnsforward/upstreams.go +++ b/internal/dnsforward/upstreams.go @@ -2,10 +2,8 @@ package dnsforward import ( "fmt" - "net" "net/netip" "os" - "strings" "time" "github.com/AdguardTeam/AdGuardHome/internal/aghnet" @@ -19,28 +17,6 @@ import ( "golang.org/x/exp/slices" ) -const ( - // errNotDomainSpecific is returned when the upstream should be - // domain-specific, but isn't. - errNotDomainSpecific errors.Error = "not a domain-specific upstream" - - // errMissingSeparator is returned when the domain-specific part of the - // upstream configuration line isn't closed. - errMissingSeparator errors.Error = "missing separator" - - // errDupSeparator is returned when the domain-specific part of the upstream - // configuration line contains more than one ending separator. - errDupSeparator errors.Error = "duplicated separator" - - // errNoDefaultUpstreams is returned when there are no default upstreams - // specified in the upstream configuration. - errNoDefaultUpstreams errors.Error = "no default upstreams specified" - - // errWrongResponse is returned when the checked upstream replies in an - // unexpected way. - errWrongResponse errors.Error = "wrong response" -) - // loadUpstreams parses upstream DNS servers from the configured file or from // the configuration itself. func (s *Server) loadUpstreams() (upstreams []string, err error) { @@ -199,84 +175,12 @@ func IsCommentOrEmpty(s string) (ok bool) { return len(s) == 0 || s[0] == '#' } -// newUpstreamConfig validates upstreams and returns an appropriate upstream -// configuration or nil if it can't be built. -// -// TODO(e.burkov): Perhaps proxy.ParseUpstreamsConfig should validate upstreams -// slice already so that this function may be considered useless. -func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err error) { - // No need to validate comments and empty lines. - upstreams = stringutil.FilterOut(upstreams, IsCommentOrEmpty) - if len(upstreams) == 0 { - // Consider this case valid since it means the default server should be - // used. - return nil, nil - } - - 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( - upstreams, - &upstream.Options{ - Bootstrap: net.DefaultResolver, - Timeout: DefaultTimeout, - }, - ) - 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, errNoDefaultUpstreams - } - - return conf, nil -} - -// validateUpstreamConfig validates each upstream from the upstream -// configuration and returns an error if any upstream is invalid. -// -// TODO(e.burkov): Merge with [upstreamConfigValidator] somehow. -func validateUpstreamConfig(conf []string) (err error) { - for _, u := range conf { - var ups []string - var isSpecific bool - ups, isSpecific, err = splitUpstreamLine(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, isSpecific) - 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. -// -// TODO(e.burkov): Merge with [upstreamConfigValidator] somehow. -func ValidateUpstreams(upstreams []string) (err error) { - _, err = newUpstreamConfig(upstreams) - - return err -} - // ValidateUpstreamsPrivate validates each upstream and returns an error if any // upstream is invalid or if there are no default upstreams specified. It also // checks each domain of domain-specific upstreams for being ARPA pointing to // a locally-served network. privateNets must not be nil. func ValidateUpstreamsPrivate(upstreams []string, privateNets netutil.SubnetSet) (err error) { - conf, err := newUpstreamConfig(upstreams) + conf, err := proxy.ParseUpstreamsConfig(upstreams, &upstream.Options{}) if err != nil { return fmt.Errorf("creating config: %w", err) } @@ -308,66 +212,3 @@ func ValidateUpstreamsPrivate(upstreams []string, privateNets netutil.SubnetSet) return errors.Annotate(errors.Join(errs...), "checking domain-specific upstreams: %w") } - -// protocols are the supported URL schemes for upstreams. -var protocols = []string{"h3", "https", "quic", "sdns", "tcp", "tls", "udp"} - -// 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, isSpecific bool) (useDefault bool, err error) { - // The special server address '#' means that default server must be used. - if useDefault = u == "#" && isSpecific; useDefault { - return useDefault, nil - } - - // Check if the upstream has a valid protocol prefix. - // - // TODO(e.burkov): Validate the domain name. - if proto, _, ok := strings.Cut(u, "://"); ok { - if !slices.Contains(protocols, proto) { - return false, fmt.Errorf("bad protocol %q", proto) - } - } else if _, err = netip.ParseAddr(u); err == nil { - return false, nil - } else if _, err = netip.ParseAddrPort(u); err == nil { - return false, nil - } - - return false, err -} - -// splitUpstreamLine returns the upstreams and the specified domains. domains -// is nil when the upstream is not domains-specific. Otherwise it may also be -// empty. -func splitUpstreamLine(upstreamStr string) (upstreams []string, isSpecific bool, err error) { - if !strings.HasPrefix(upstreamStr, "[/") { - return []string{upstreamStr}, false, nil - } - - defer func() { err = errors.Annotate(err, "splitting upstream line %q: %w", upstreamStr) }() - - doms, ups, found := strings.Cut(upstreamStr[2:], "/]") - if !found { - return nil, false, errMissingSeparator - } else if strings.Contains(ups, "/]") { - return nil, false, errDupSeparator - } - - for i, host := range strings.Split(doms, "/") { - if host == "" { - continue - } - - err = netutil.ValidateDomainName(strings.TrimPrefix(host, "*.")) - if err != nil { - return nil, false, fmt.Errorf("domain at index %d: %w", i, err) - } - - isSpecific = true - } - - return strings.Fields(ups), isSpecific, nil -} diff --git a/internal/dnsforward/upstreams_internal_test.go b/internal/dnsforward/upstreams_internal_test.go index 9bf2f5cf..128c3eeb 100644 --- a/internal/dnsforward/upstreams_internal_test.go +++ b/internal/dnsforward/upstreams_internal_test.go @@ -100,8 +100,7 @@ func TestUpstreamConfigValidator(t *testing.T) { name: "bad_specification", general: []string{"[/domain.example/]/]1.2.3.4"}, want: map[string]string{ - "[/domain.example/]/]1.2.3.4": `splitting upstream line ` + - `"[/domain.example/]/]1.2.3.4": duplicated separator`, + "[/domain.example/]/]1.2.3.4": generalTextLabel + " 1: parsing error", }, }, { name: "all_different", @@ -120,23 +119,9 @@ func TestUpstreamConfigValidator(t *testing.T) { fallback: []string{"[/example/" + goodUps}, private: []string{"[/example//bad.123/]" + goodUps}, want: map[string]string{ - `[/example/]/]` + goodUps: `splitting upstream line ` + - `"[/example/]/]` + goodUps + `": duplicated separator`, - `[/example/` + goodUps: `splitting upstream line ` + - `"[/example/` + goodUps + `": missing separator`, - `[/example//bad.123/]` + goodUps: `splitting upstream line ` + - `"[/example//bad.123/]` + goodUps + `": domain at index 2: ` + - `bad domain name "bad.123": ` + - `bad top-level domain name label "123": all octets are numeric`, - }, - }, { - name: "non-specific_default", - general: []string{ - "#", - "[/example/]#", - }, - want: map[string]string{ - "#": "not a domain-specific upstream", + "[/example/]/]" + goodUps: generalTextLabel + " 1: parsing error", + "[/example/" + goodUps: fallbackTextLabel + " 1: parsing error", + "[/example//bad.123/]" + goodUps: privateTextLabel + " 1: parsing error", }, }, { name: "bad_proto", @@ -144,7 +129,15 @@ func TestUpstreamConfigValidator(t *testing.T) { "bad://1.2.3.4", }, want: map[string]string{ - "bad://1.2.3.4": `bad protocol "bad"`, + "bad://1.2.3.4": generalTextLabel + " 1: parsing error", + }, + }, { + name: "truncated_line", + general: []string{ + "This is a very long line. It will cause a parsing error and will be truncated here.", + }, + want: map[string]string{ + "This is a very long line. It will cause a parsing error and will be truncated …": "upstream_dns 1: parsing error", }, }} diff --git a/internal/home/clients.go b/internal/home/clients.go index 07256fea..63b9472f 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -613,7 +613,7 @@ func (clients *clientsContainer) check(c *persistentClient) (err error) { // TODO(s.chzhen): Move to the constructor. slices.Sort(c.Tags) - err = dnsforward.ValidateUpstreams(c.Upstreams) + _, err = proxy.ParseUpstreamsConfig(c.Upstreams, &upstream.Options{}) if err != nil { return fmt.Errorf("invalid upstream servers: %w", err) }