From e42614edc5e69c923177bac00868e7336abca8ba Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 11 May 2021 09:50:18 +0200 Subject: [PATCH] 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 * Add more test cases Add test from origin bug report: #2779 Signed-off-by: Miek Gieben * Rebase and fix conflicts Signed-off-by: Miek Gieben --- core/dnsserver/config.go | 5 ---- core/dnsserver/onstartup.go | 15 ++++++++-- core/dnsserver/register.go | 41 ++++++++++++++------------ core/dnsserver/reverse.go | 38 ++++++++++++++++++++++++ core/dnsserver/reverse_test.go | 31 +++++++++++++++++++ core/dnsserver/server.go | 19 +++--------- go.mod | 1 + go.sum | 2 ++ test/server_test.go | 54 ++++++++++++++++++++++++++++++++++ 9 files changed, 165 insertions(+), 41 deletions(-) create mode 100644 core/dnsserver/reverse.go create mode 100644 core/dnsserver/reverse_test.go diff --git a/core/dnsserver/config.go b/core/dnsserver/config.go index 4ff2ecda1..d0a432a85 100644 --- a/core/dnsserver/config.go +++ b/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 diff --git a/core/dnsserver/onstartup.go b/core/dnsserver/onstartup.go index 7aa519e98..113bf15e5 100644 --- a/core/dnsserver/onstartup.go +++ b/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) diff --git a/core/dnsserver/register.go b/core/dnsserver/register.go index b4e9f4fde..505aa8580 100644 --- a/core/dnsserver/register.go +++ b/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 { diff --git a/core/dnsserver/reverse.go b/core/dnsserver/reverse.go new file mode 100644 index 000000000..2d0a7c740 --- /dev/null +++ b/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 +} diff --git a/core/dnsserver/reverse_test.go b/core/dnsserver/reverse_test.go new file mode 100644 index 000000000..bc6909b8a --- /dev/null +++ b/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]) + } + } + } +} diff --git a/core/dnsserver/server.go b/core/dnsserver/server.go index c7304d763..638cbb8fe 100644 --- a/core/dnsserver/server.go +++ b/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) - if !plugin.ClientWrite(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 + rcode, _ := h.pluginChain.ServeDNS(ctx, w, r) + if !plugin.ClientWrite(rcode) { + errorFunc(s.Addr, w, r, rcode) } + 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 diff --git a/go.mod b/go.mod index fe06668e3..6f146dd84 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index eba9b9157..1f476c8f0 100644 --- a/go.sum +++ b/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= diff --git a/test/server_test.go b/test/server_test.go index f03a27749..c58915ee7 100644 --- a/test/server_test.go +++ b/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)) + } +}