all: imp code, docs

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

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
 }
 

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://"}

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)
 		})
 	}

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)
 	}