diff --git a/internal/aghnet/subnetdetector.go b/internal/aghnet/subnetdetector.go index 20f438882ef..fd338aaa7a6 100644 --- a/internal/aghnet/subnetdetector.go +++ b/internal/aghnet/subnetdetector.go @@ -6,12 +6,12 @@ import ( // SubnetDetector describes IP address properties. type SubnetDetector struct { - // spNets stores the collection of special-purpose address registries as - // defined by RFC 6890. + // spNets is the collection of special-purpose address registries as defined + // by RFC 6890. spNets []*net.IPNet - // locServedNets stores the collection of locally-served networks as defined - // by RFC 6303. + // locServedNets is the collection of locally-served networks as defined by + // RFC 6303. locServedNets []*net.IPNet } diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index f870d72cd97..d653fa1edc8 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -5,6 +5,7 @@ import ( "fmt" "net" "net/http" + "sort" "strings" "time" @@ -170,14 +171,14 @@ func (req *dnsConfig) validate(snd *aghnet.SubnetDetector) (err error) { if req.Upstreams != nil { err = ValidateUpstreams(*req.Upstreams) if err != nil { - return err + return fmt.Errorf("validating upstream servers: %w", err) } } if req.LocalPTRUpstreams != nil { err = ValidateUpstreamsPrivate(*req.LocalPTRUpstreams, snd) if err != nil { - return err + return fmt.Errorf("validating private upstream servers: %w", err) } } @@ -360,12 +361,12 @@ type LocalNetChecker interface { // type check var _ LocalNetChecker = (*aghnet.SubnetDetector)(nil) -// assembleConfig validates upstreams and returns an appropriate upstream +// 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 assembleConfig(upstreams []string) (conf *proxy.UpstreamConfig, err error) { +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 { @@ -399,17 +400,31 @@ func assembleConfig(upstreams []string) (conf *proxy.UpstreamConfig, err error) // // TODO(e.burkov): Move into aghnet or even into dnsproxy. func ValidateUpstreams(upstreams []string) (err error) { - _, err = assembleConfig(upstreams) + _, err = newUpstreamConfig(upstreams) return err } +// stringKeysSorted returns the sorted slice of string keys of m. +// +// TODO(e.burkov): Use generics in Go 1.18. Move into golibs. +func stringKeysSorted(m map[string][]upstream.Upstream) (sorted []string) { + sorted = make([]string, 0, len(m)) + for s := range m { + sorted = append(sorted, s) + } + + sort.Strings(sorted) + + return sorted +} + // 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. lnc must not be nil. func ValidateUpstreamsPrivate(upstreams []string, lnc LocalNetChecker) (err error) { - conf, err := assembleConfig(upstreams) + conf, err := newUpstreamConfig(upstreams) if err != nil { return err } @@ -420,7 +435,7 @@ func ValidateUpstreamsPrivate(upstreams []string, lnc LocalNetChecker) (err erro var errs []error - for domain := range conf.DomainReservedUpstreams { + for _, domain := range stringKeysSorted(conf.DomainReservedUpstreams) { var subnet *net.IPNet subnet, err = netutil.SubnetFromReversedAddr(domain) if err != nil { diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 175d27d4686..66931f4dd8a 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -183,8 +183,9 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { name: "upstream_mode_fastest_addr", wantSet: "", }, { - name: "upstream_dns_bad", - wantSet: `bad ipport address "!!!": address !!!: missing port in address`, + name: "upstream_dns_bad", + wantSet: `validating upstream servers: bad ipport address "!!!": ` + + `address !!!: missing port in address`, }, { name: "bootstraps_bad", wantSet: `checking bootstrap a: invalid address: ` + @@ -198,6 +199,10 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { }, { name: "local_ptr_upstreams_good", wantSet: "", + }, { + name: "local_ptr_upstreams_bad", + wantSet: `validating private upstream servers: checking domain-specific upstreams: ` + + `bad arpa domain name "non.arpa": not a reversed ip network`, }, { name: "local_ptr_upstreams_null", wantSet: "", @@ -430,8 +435,8 @@ func TestValidateUpstreamsPrivate(t *testing.T) { }, { name: "several_bad", wantErr: `checking domain-specific upstreams: 2 errors: ` + - `"bad arpa domain name \"non.arpa\": not a reversed ip network", ` + - `"arpa domain \"1.2.3.4.in-addr.arpa.\" should point to a locally-served network"`, + `"arpa domain \"1.2.3.4.in-addr.arpa.\" should point to a locally-served network", ` + + `"bad arpa domain name \"non.arpa\": not a reversed ip network"`, u: "[/non.arpa/1.2.3.4.in-addr.arpa/127.in-addr.arpa/]#", }} diff --git a/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json b/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json index b594029ced8..830bf491420 100644 --- a/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json +++ b/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json @@ -520,6 +520,43 @@ ] } }, + "local_ptr_upstreams_bad": { + "req": { + "local_ptr_upstreams": [ + "123.123.123.123", + "[/non.arpa/]#" + ] + }, + "want": { + "upstream_dns": [ + "8.8.8.8:53", + "8.8.4.4:53" + ], + "upstream_dns_file": "", + "bootstrap_dns": [ + "9.9.9.10", + "149.112.112.10", + "2620:fe::10", + "2620:fe::fe:10" + ], + "protection_enabled": true, + "ratelimit": 0, + "blocking_mode": "", + "blocking_ipv4": "", + "blocking_ipv6": "", + "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": [] + } + }, "local_ptr_upstreams_null": { "req": { "local_ptr_upstreams": null