From 39793551438cbea71e6ec78d0e05bee2d8dba3e5 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Mon, 14 Feb 2022 15:08:36 +0300 Subject: [PATCH] all: imp code, docs --- CHANGELOG.md | 7 +- internal/aghnet/subnetdetector.go | 8 +-- internal/dnsforward/http.go | 114 +++++++++++++++++------------- internal/dnsforward/http_test.go | 6 +- internal/home/clients.go | 2 +- 5 files changed, 76 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d80413f783e..2976e276d6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,10 +21,11 @@ and this project adheres to ### Changed -- Domain specific private reverse DNS upstream servers are now validated to +- Domain-specific private reverse DNS upstream servers are now validated to allow only `*.in-addr.arpa` and `*.ip6.arpa` domains pointing to - locally-served networks ([#3381]). **Note:** If you have invalid entries - configured, consider to remove those manually. + locally-served networks ([#3381]). **Note:** If you already have invalid + entires in your configuration, consider removing them manually, since they + essentially had no effect. - Response filtering is now performed using the record types of the answer section of messages as opposed to the type of the question ([#4238]). - Instead of adding the build time information, the build scripts now use the diff --git a/internal/aghnet/subnetdetector.go b/internal/aghnet/subnetdetector.go index fc9e351b2fd..20f438882ef 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 is the slice of special-purpose address registries as defined by - // the RFC 6890. + // spNets stores the collection of special-purpose address registries as + // defined by RFC 6890. spNets []*net.IPNet - // locServedNets is the slice of locally-served networks as defined by the - // RFC 6303. + // locServedNets stores 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 c3e73abe2d3..f870d72cd97 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -168,14 +168,14 @@ func (req *dnsConfig) checkBootstrap() (err error) { // validate returns an error if any field of req is invalid. func (req *dnsConfig) validate(snd *aghnet.SubnetDetector) (err error) { if req.Upstreams != nil { - err = ValidateUpstreams(*req.Upstreams, nil) + err = ValidateUpstreams(*req.Upstreams) if err != nil { return err } } if req.LocalPTRUpstreams != nil { - err = ValidateUpstreams(*req.LocalPTRUpstreams, snd) + err = ValidateUpstreamsPrivate(*req.LocalPTRUpstreams, snd) if err != nil { return err } @@ -349,85 +349,99 @@ func IsCommentOrEmpty(s string) (ok bool) { return len(s) == 0 || s[0] == '#' } -// PrivatenessChecker is used to check if the IP address belongs to a local +// LocalNetChecker is used to check if the IP address belongs to a local // network. -type PrivatenessChecker interface { +type LocalNetChecker interface { // IsLocallyServedNetwork returns true if ip is contained in any of address // registries defined by RFC 6303. IsLocallyServedNetwork(ip net.IP) (ok bool) } // type check -var _ PrivatenessChecker = (*aghnet.SubnetDetector)(nil) +var _ LocalNetChecker = (*aghnet.SubnetDetector)(nil) -// checkPrivatePTR returns an error if any of keys of the domainsMap isn't a -// valid ARPA domain pointing to a locally-served network. pc must not be nil. -func checkPrivatePTR(domainsMap map[string][]upstream.Upstream, pc PrivatenessChecker) (err error) { - var errs []error - - for domain := range domainsMap { - var subnet *net.IPNet - subnet, err = netutil.SubnetFromReversedAddr(domain) - if err != nil { - errs = append(errs, err) - - continue - } - - if !pc.IsLocallyServedNetwork(subnet.IP) { - errs = append( - errs, - fmt.Errorf("arpa domain %q should point to a locally-served network", domain), - ) - } - } - - if len(errs) > 0 { - return errors.List("checking domain-specific upstreams", errs...) - } - - return nil -} - -// ValidateUpstreams validates each upstream and returns an error if any -// upstream is invalid or if there are no default upstreams specified. If pc is -// not nil, it's used to check each domain of domain-specific upstreams. +// assembleConfig validates upstreams and returns an appropriate upstream +// configuration or nil if it can't be built. // -// TODO(e.burkov): Move into aghnet or even into dnsproxy. Perhaps -// proxy.ParseUpstreamsConfig should validate upstreams slice already so that -// this function may be considered useless. -func ValidateUpstreams(upstreams []string, pc PrivatenessChecker) (err error) { +// 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) { // 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 + return nil, nil } - var upsConf *proxy.UpstreamConfig - upsConf, err = proxy.ParseUpstreamsConfig( + conf, err = proxy.ParseUpstreamsConfig( upstreams, &upstream.Options{Bootstrap: []string{}, Timeout: DefaultTimeout}, ) if err != nil { - return err - } else if len(upsConf.Upstreams) == 0 { - return errors.Error("no default upstreams specified") + return nil, err + } else if len(conf.Upstreams) == 0 { + return nil, errors.Error("no default upstreams specified") } for _, u := range upstreams { _, err = validateUpstream(u) if err != nil { - return err + return nil, err } } - if pc == nil { + return conf, 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): Move into aghnet or even into dnsproxy. +func ValidateUpstreams(upstreams []string) (err error) { + _, err = assembleConfig(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. lnc must not be nil. +func ValidateUpstreamsPrivate(upstreams []string, lnc LocalNetChecker) (err error) { + conf, err := assembleConfig(upstreams) + if err != nil { + return err + } + + if conf == nil { return nil } - return checkPrivatePTR(upsConf.DomainReservedUpstreams, pc) + var errs []error + + for domain := range conf.DomainReservedUpstreams { + var subnet *net.IPNet + subnet, err = netutil.SubnetFromReversedAddr(domain) + if err != nil { + errs = append(errs, err) + + continue + } + + if !lnc.IsLocallyServedNetwork(subnet.IP) { + errs = append( + errs, + fmt.Errorf("arpa domain %q should point to a locally-served network", domain), + ) + } + } + + if len(errs) > 0 { + return errors.List("checking domain-specific upstreams", errs...) + } + + return nil } var protocols = []string{"tls://", "https://", "tcp://", "sdns://", "quic://"} diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 7bb8b9d6191..175d27d4686 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -390,13 +390,13 @@ func TestValidateUpstreams(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err := ValidateUpstreams(tc.set, nil) + err := ValidateUpstreams(tc.set) testutil.AssertErrorMsg(t, tc.wantErr, err) }) } } -func TestValidateUpstreams_privateness(t *testing.T) { +func TestValidateUpstreamsPrivate(t *testing.T) { snd, err := aghnet.NewSubnetDetector() require.NoError(t, err) @@ -439,7 +439,7 @@ func TestValidateUpstreams_privateness(t *testing.T) { set := []string{"192.168.0.1", tc.u} t.Run(tc.name, func(t *testing.T) { - err = ValidateUpstreams(set, snd) + err = ValidateUpstreamsPrivate(set, snd) testutil.AssertErrorMsg(t, tc.wantErr, err) }) } diff --git a/internal/home/clients.go b/internal/home/clients.go index 5a5fc437873..f539adbeb66 100644 --- a/internal/home/clients.go +++ b/internal/home/clients.go @@ -544,7 +544,7 @@ func (clients *clientsContainer) check(c *Client) (err error) { sort.Strings(c.Tags) - err = dnsforward.ValidateUpstreams(c.Upstreams, nil) + err = dnsforward.ValidateUpstreams(c.Upstreams) if err != nil { return fmt.Errorf("invalid upstream servers: %w", err) }