reverse zones (#4538)

* core: fix v4 non-octet reverse zones

This fixes the reverse zones handling. Add expanstion of the reverse
notation to all octet boundary subnets and add those to the config - just as if
they were directly typed in the config.

This takes inspiration from #4501, but that (even with DCO!!) seems to
be just using https://github.com/apparentlymart/go-cidr/ so use that
instead - I think a minor function is still needed that one is copied
from #4501.

Also sort the zones we are listing on startup - caught in this PR
because of the expanded zones being not listed next to each other.

This also removes the need for FilterFunc from the config, so this is
now gone as well, making the whole thing slightly more efficient.

Add couple of reverse unit tests and a e2e test that queries for the
correct (and incorrect) reverse zones and checks the reply.

Closes: #4501
Fixes: #2779

Signed-off-by: Miek Gieben <miek@miek.nl>

* Add more test cases

Add test from origin bug report: #2779

Signed-off-by: Miek Gieben <miek@miek.nl>

* Rebase and fix conflicts

Signed-off-by: Miek Gieben <miek@miek.nl>

core/dnsserver/config.go

@@ -37,11 +37,6 @@ type Config struct {
 	// may depend on it.
 	HTTPRequestValidateFunc func(*http.Request) bool
 
-	// If this function is not nil it will be used to further filter access
-	// to this handler. The primary use is to limit access to a reverse zone
-	// on a non-octet boundary, i.e. /17
-	FilterFunc func(string) bool
-
 	// TLSConfig when listening for encrypted connections (gRPC, DNS-over-TLS).
 	TLSConfig *tls.Config
 

core/dnsserver/onstartup.go

@@ -1,6 +1,9 @@
 package dnsserver
 
-import "fmt"
+import (
+	"fmt"
+	"sort"
+)
 
 // startUpZones creates the text that we show when starting up:
 // grpc://example.com.:1055
@@ -8,7 +11,15 @@ import "fmt"
 func startUpZones(protocol, addr string, zones map[string]*Config) string {
 	s := ""
 
-	for zone := range zones {
+	keys := make([]string, len(zones))
+	i := 0
+	for k := range zones {
+		keys[i] = k
+		i++
+	}
+	sort.Strings(keys)
+
+	for _, zone := range keys {
 		// split addr into protocol, IP and Port
 		_, ip, port, err := SplitProtocolHostPort(addr)
 

core/dnsserver/register.go

@@ -4,13 +4,11 @@ import (
 	"flag"
 	"fmt"
 	"net"
-	"strings"
 	"time"
 
 	"github.com/coredns/caddy"
 	"github.com/coredns/caddy/caddyfile"
 	"github.com/coredns/coredns/plugin"
-	"github.com/coredns/coredns/plugin/pkg/dnsutil"
 	"github.com/coredns/coredns/plugin/pkg/parse"
 	"github.com/coredns/coredns/plugin/pkg/transport"
 )
@@ -60,6 +58,28 @@ var _ caddy.Context = &dnsContext{}
 func (h *dnsContext) InspectServerBlocks(sourceFile string, serverBlocks []caddyfile.ServerBlock) ([]caddyfile.ServerBlock, error) {
 	// Normalize and check all the zone names and check for duplicates
 	for ib, s := range serverBlocks {
+		// Walk the s.Keys and expand any reverse address in their proper DNS in-addr zones. If the expansions leads for
+		// more than one reverse zone, replace the current value and add the rest to s.Keys.
+		for ik, k := range s.Keys {
+			_, k1 := parse.Transport(k) // get rid of any dns:// or other scheme.
+			_, port, ipnet, err := plugin.SplitHostPort(k1)
+			if ipnet == nil || err != nil { // err will be caught below
+				continue
+			}
+			if port != "" {
+				port = ":" + port
+			}
+			nets := classFromCIDR(ipnet)
+			if len(nets) > 1 {
+				s.Keys[ik] = nets[0] + port  // replace for the first
+				for _, n := range nets[1:] { // add  the rest
+					s.Keys = append(s.Keys, n+port)
+				}
+			}
+		}
+
+		serverBlocks[ib].Keys = s.Keys // important to save back the new keys that are potentially created here.
+
 		for ik, k := range s.Keys {
 			za, err := normalizeZone(k)
 			if err != nil {
@@ -74,22 +94,6 @@ func (h *dnsContext) InspectServerBlocks(sourceFile string, serverBlocks []caddy
 				Transport:   za.Transport,
 			}
 			keyConfig := keyForConfig(ib, ik)
-			if za.IPNet == nil {
-				h.saveConfig(keyConfig, cfg)
-				continue
-			}
-
-			ones, bits := za.IPNet.Mask.Size()
-			if (bits-ones)%8 != 0 { // only do this for non-octet boundaries
-				cfg.FilterFunc = func(s string) bool {
-					// TODO(miek): strings.ToLower! Slow and allocates new string.
-					addr := dnsutil.ExtractAddressFromReverse(strings.ToLower(s))
-					if addr == "" {
-						return true
-					}
-					return za.IPNet.Contains(net.ParseIP(addr))
-				}
-			}
 			h.saveConfig(keyConfig, cfg)
 		}
 	}
@@ -222,7 +226,6 @@ func (h *dnsContext) validateZonesAndListeningAddresses() error {
 // address (what you pass into net.Listen) to the list of site configs.
 // This function does NOT vet the configs to ensure they are compatible.
 func groupConfigsByListenAddr(configs []*Config) (map[string][]*Config, error) {
-
 	groups := make(map[string][]*Config)
 	for _, conf := range configs {
 		for _, h := range conf.ListenHosts {

core/dnsserver/reverse.go

@@ -0,0 +1,38 @@
+package dnsserver
+
+import (
+	"math"
+	"net"
+
+	"github.com/apparentlymart/go-cidr/cidr"
+)
+
+// classFromCIDR return slice of "classful" (/8, /16, /24 or /32 only) CIDR's from the CIDR in net.
+func classFromCIDR(n *net.IPNet) []string {
+	ones, _ := n.Mask.Size()
+	if ones%8 == 0 {
+		return []string{n.String()}
+	}
+
+	mask := int(math.Ceil(float64(ones)/8)) * 8
+	networks := subnets(n, mask)
+	cidrs := make([]string, len(networks))
+	for i := range networks {
+		cidrs[i] = networks[i].String()
+	}
+	return cidrs
+}
+
+// subnets return a slice of prefixes with the desired mask subnetted from original network.
+func subnets(network *net.IPNet, newPrefixLen int) []*net.IPNet {
+	prefixLen, _ := network.Mask.Size()
+	maxSubnets := int(math.Exp2(float64(newPrefixLen)) / math.Exp2(float64(prefixLen)))
+	nets := []*net.IPNet{{network.IP, net.CIDRMask(newPrefixLen, 8*len(network.IP))}}
+
+	for i := 1; i < maxSubnets; i++ {
+		next, _ := cidr.NextSubnet(nets[len(nets)-1], newPrefixLen)
+		nets = append(nets, next)
+	}
+
+	return nets
+}

core/dnsserver/reverse_test.go

@@ -0,0 +1,31 @@
+package dnsserver
+
+import (
+	"net"
+	"testing"
+)
+
+func TestClassFromCIDR(t *testing.T) {
+	tests := []struct {
+		in       string
+		expected []string
+	}{
+		{"10.0.0.0/15", []string{"10.0.0.0/16", "10.1.0.0/16"}},
+		{"10.0.0.0/16", []string{"10.0.0.0/16"}},
+		{"192.168.1.1/23", []string{"192.168.0.0/24", "192.168.1.0/24"}},
+		{"10.129.60.0/22", []string{"10.129.60.0/24", "10.129.61.0/24", "10.129.62.0/24", "10.129.63.0/24"}},
+	}
+	for i, tc := range tests {
+		_, n, _ := net.ParseCIDR(tc.in)
+		nets := classFromCIDR(n)
+		if len(nets) != len(tc.expected) {
+			t.Errorf("Test %d, expected %d subnets, got %d", i, len(tc.expected), len(nets))
+			continue
+		}
+		for j := range nets {
+			if nets[j] != tc.expected[j] {
+				t.Errorf("Test %d, expected %s, got %s", i, tc.expected[j], nets[j])
+			}
+		}
+	}
+}

core/dnsserver/server.go

@@ -247,22 +247,11 @@ func (s *Server) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 				return
 			}
 			if r.Question[0].Qtype != dns.TypeDS {
-				if h.FilterFunc == nil {
+				rcode, _ := h.pluginChain.ServeDNS(ctx, w, r)
-					rcode, _ := h.pluginChain.ServeDNS(ctx, w, r)
+				if !plugin.ClientWrite(rcode) {
-					if !plugin.ClientWrite(rcode) {
+					errorFunc(s.Addr, w, r, rcode)
-						errorFunc(s.Addr, w, r, rcode)
-					}
-					return
-				}
-				// FilterFunc is set, call it to see if we should use this handler.
-				// This is given to full query name.
-				if h.FilterFunc(q) {
-					rcode, _ := h.pluginChain.ServeDNS(ctx, w, r)
-					if !plugin.ClientWrite(rcode) {
-						errorFunc(s.Addr, w, r, rcode)
-					}
-					return
 				}
+				return
 			}
 			// The type is DS, keep the handler, but keep on searching as maybe we are serving
 			// the parent as well and the DS should be routed to it - this will probably *misroute* DS

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/Azure/go-autorest/autorest/azure/auth v0.5.7
 	github.com/Azure/go-autorest/autorest/to v0.2.0 // indirect
 	github.com/DataDog/datadog-go v3.5.0+incompatible // indirect
+	github.com/apparentlymart/go-cidr v1.1.0 // indirect
 	github.com/aws/aws-sdk-go v1.38.30
 	github.com/coredns/caddy v1.1.0
 	github.com/dnstap/golang-dnstap v0.4.0

go.sum

@@ -70,6 +70,8 @@ github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRF
 github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d/go.mod h1:rBZYJk541a8SKzHPHnH3zbiI+7dagKZ0cgpgrD7Fyho=
 github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
 github.com/apache/thrift v0.13.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ=
+github.com/apparentlymart/go-cidr v1.1.0 h1:2mAhrMoF+nhXqxTzSZMUzDHkLjmIHC+Zzn4tdgBZjnU=
+github.com/apparentlymart/go-cidr v1.1.0/go.mod h1:EBcsNrHc3zQeuaeCeCtQruQm+n9/YjEn/vI25Lg7Gwc=
 github.com/armon/circbuf v0.0.0-20150827004946-bbbad097214e/go.mod h1:3U/XgcO3hCbHZ8TKRvWD2dDTCfh9M9ya+I9JpbB7O8o=
 github.com/armon/go-metrics v0.0.0-20180917152333-f0300d1749da/go.mod h1:Q73ZrmVTwzkszR9V5SSuryQ31EELlFMUz1kKyl939pY=
 github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8=

test/server_test.go

@@ -53,3 +53,57 @@ func chaosTest(t *testing.T, server string) {
 		t.Fatalf("Expected version.bind. reply, got %s", r.Answer[0].String())
 	}
 }
+
+func TestReverseExpansion(t *testing.T) {
+	// this test needs a fixed port, because with :0 the expanded reverse zone will listen on different
+	// addresses and we can't check which ones...
+	corefile := `10.0.0.0/15:5053 {
+		whoami
+	}`
+
+	server, udp, _, err := CoreDNSServerAndPorts(corefile)
+	if err != nil {
+		t.Fatalf("Could not get CoreDNS serving instance: %s", err)
+	}
+
+	defer server.Stop()
+
+	m := new(dns.Msg)
+	m.SetQuestion("whoami.0.10.in-addr.arpa.", dns.TypeA)
+
+	r, err := dns.Exchange(m, udp)
+	if err != nil {
+		t.Fatalf("Could not send message: %s", err)
+	}
+	if r.Rcode != dns.RcodeSuccess {
+		t.Errorf("Expected NOERROR, got %d", r.Rcode)
+	}
+	if len(r.Extra) != 2 {
+		t.Errorf("Expected 2 RRs in additional section, got %d", len(r.Extra))
+	}
+
+	m.SetQuestion("whoami.1.10.in-addr.arpa.", dns.TypeA)
+	r, err = dns.Exchange(m, udp)
+	if err != nil {
+		t.Fatalf("Could not send message: %s", err)
+	}
+	if r.Rcode != dns.RcodeSuccess {
+		t.Errorf("Expected NOERROR, got %d", r.Rcode)
+	}
+	if len(r.Extra) != 2 {
+		t.Errorf("Expected 2 RRs in additional section, got %d", len(r.Extra))
+	}
+
+	// should be refused
+	m.SetQuestion("whoami.2.10.in-addr.arpa.", dns.TypeA)
+	r, err = dns.Exchange(m, udp)
+	if err != nil {
+		t.Fatalf("Could not send message: %s", err)
+	}
+	if r.Rcode != dns.RcodeRefused {
+		t.Errorf("Expected REFUSED, got %d", r.Rcode)
+	}
+	if len(r.Extra) != 0 {
+		t.Errorf("Expected 0 RRs in additional section, got %d", len(r.Extra))
+	}
+}