middleware/file: fix DS handling (#344)

The DS record is handled specially in the server ServeDNS mux, but there
was no code that actually called the correct middleware handler chain
when encountering a DS.

This PR fixes that behavoir, additonal bugs has been files to look into
how we are handling delegation (secure and non-secure ones).

4 files changed, 227 insertions, 0 deletions

diff --git a/core/dnsserver/server.go b/core/dnsserver/server.go
index b4e4bc13e..aa94dcd8c 100644
--- a/core/dnsserver/server.go
+++ b/core/dnsserver/server.go
@@ -177,6 +177,8 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 	off, end := 0, false
 	ctx := context.Background()
 
+	var dshandler *Config
+
 	for {
 		l := len(q[off:])
 		for i := 0; i < l; i++ {
@@ -195,12 +197,28 @@ func (s *Server) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
 				}
 				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
+			// queries to a possibly grand parent, but there is no way for us to know at this point
+			// if there is an actually delegation from grandparent -> parent -> zone.
+			// In all fairness: direct DS queries should not be needed.
+			dshandler = h
 		}
 		off, end = dns.NextLabel(q, off)
 		if end {
 			break
 		}
 	}
+
+	if dshandler != nil {
+		// DS request, and we found a zone, use the handler for the query
+		rcode, _ := dshandler.middlewareChain.ServeDNS(ctx, w, r)
+		if rcodeNoClientWrite(rcode) {
+			DefaultErrorFunc(w, r, rcode)
+		}
+		return
+	}
+
 	// Wildcard match, if we have found nothing try the root zone as a last resort.
 	if h, ok := s.zones["."]; ok {
 		rcode, _ := h.middlewareChain.ServeDNS(ctx, w, r)
diff --git a/middleware/file/ds_test.go b/middleware/file/ds_test.go
new file mode 100644
index 000000000..a7ba06263
--- /dev/null
+++ b/middleware/file/ds_test.go
@@ -0,0 +1,92 @@
+package file
+
+import (
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/miekg/coredns/middleware/pkg/dnsrecorder"
+	"github.com/miekg/coredns/middleware/test"
+
+	"github.com/miekg/dns"
+	"golang.org/x/net/context"
+)
+
+var dsTestCases = []test.Case{
+	{
+		Qname: "a.delegated.miek.nl.", Qtype: dns.TypeDS,
+		Ns: []dns.RR{
+			test.NS("delegated.miek.nl.	1800	IN	NS	a.delegated.miek.nl."),
+			test.NS("delegated.miek.nl.	1800	IN	NS	ns-ext.nlnetlabs.nl."),
+		},
+		Extra: []dns.RR{
+			test.A("a.delegated.miek.nl. 1800 IN A 139.162.196.78"),
+			test.AAAA("a.delegated.miek.nl. 1800 IN AAAA 2a01:7e00::f03c:91ff:fef1:6735"),
+		},
+	},
+	{
+		Qname: "_udp.delegated.miek.nl.", Qtype: dns.TypeDS,
+		Ns: []dns.RR{
+			test.NS("delegated.miek.nl.	1800	IN	NS	a.delegated.miek.nl."),
+			test.NS("delegated.miek.nl.	1800	IN	NS	ns-ext.nlnetlabs.nl."),
+		},
+		Extra: []dns.RR{
+			test.A("a.delegated.miek.nl. 1800 IN A 139.162.196.78"),
+			test.AAAA("a.delegated.miek.nl. 1800 IN AAAA 2a01:7e00::f03c:91ff:fef1:6735"),
+		},
+	},
+	{
+		// This works *here* because we skip the server routing for DS in core/dnsserver/server.go
+		Qname: "_udp.miek.nl.", Qtype: dns.TypeDS,
+		Rcode: dns.RcodeNameError,
+		Ns: []dns.RR{
+			test.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
+		},
+	},
+	{
+		Qname: "miek.nl.", Qtype: dns.TypeDS,
+		Ns: []dns.RR{
+			test.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
+		},
+	},
+}
+
+func TestLookupDS(t *testing.T) {
+	zone, err := Parse(strings.NewReader(dbMiekNLDelegation), testzone, "stdin")
+	if err != nil {
+		t.Fatalf("expect no error when reading zone, got %q", err)
+	}
+
+	fm := File{Next: test.ErrorHandler(), Zones: Zones{Z: map[string]*Zone{testzone: zone}, Names: []string{testzone}}}
+	ctx := context.TODO()
+
+	for _, tc := range dsTestCases {
+		m := tc.Msg()
+
+		rec := dnsrecorder.New(&test.ResponseWriter{})
+		_, err := fm.ServeDNS(ctx, rec, m)
+		if err != nil {
+			t.Errorf("expected no error, got %v\n", err)
+			return
+		}
+
+		resp := rec.Msg
+		sort.Sort(test.RRSet(resp.Answer))
+		sort.Sort(test.RRSet(resp.Ns))
+		sort.Sort(test.RRSet(resp.Extra))
+
+		if !test.Header(t, tc, resp) {
+			t.Logf("%v\n", resp)
+			continue
+		}
+		if !test.Section(t, tc, test.Answer, resp.Answer) {
+			t.Logf("%v\n", resp)
+		}
+		if !test.Section(t, tc, test.Ns, resp.Ns) {
+			t.Logf("%v\n", resp)
+		}
+		if !test.Section(t, tc, test.Extra, resp.Extra) {
+			t.Logf("%v\n", resp)
+		}
+	}
+}
diff --git a/test/ds_file_test.go b/test/ds_file_test.go
new file mode 100644
index 000000000..407e04b42
--- /dev/null
+++ b/test/ds_file_test.go
@@ -0,0 +1,86 @@
+package test
+
+import (
+	"io/ioutil"
+	"log"
+	"sort"
+	"testing"
+
+	"github.com/miekg/coredns/middleware/proxy"
+	mtest "github.com/miekg/coredns/middleware/test"
+	"github.com/miekg/coredns/request"
+
+	"github.com/miekg/dns"
+)
+
+// Using miek.nl here because this is the easiest zone to get access to and it's masters
+// run both NSD and BIND9, making checks like "what should we actually return" super easy.
+var dsTestCases = []mtest.Case{
+	{
+		Qname: "_udp.miek.nl.", Qtype: dns.TypeDS,
+		Rcode: dns.RcodeNameError,
+		Ns: []dns.RR{
+			mtest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
+		},
+	},
+	{
+		Qname: "miek.nl.", Qtype: dns.TypeDS,
+		Ns: []dns.RR{
+			mtest.SOA("miek.nl.	1800	IN	SOA	linode.atoom.net. miek.miek.nl. 1282630057 14400 3600 604800 14400"),
+		},
+	},
+}
+
+func TestLookupDS(t *testing.T) {
+	name, rm, err := TempFile(".", miekNL)
+	if err != nil {
+		t.Fatalf("failed to created zone: %s", err)
+	}
+	defer rm()
+
+	corefile := `miek.nl:0 {
+       file ` + name + `
+}
+`
+
+	i, err := CoreDNSServer(corefile)
+	if err != nil {
+		t.Fatalf("Could not get CoreDNS serving instance: %s", err)
+	}
+
+	udp, _ := CoreDNSServerPorts(i, 0)
+	if udp == "" {
+		t.Fatalf("Could not get UDP listening port")
+	}
+	defer i.Stop()
+
+	log.SetOutput(ioutil.Discard)
+
+	p := proxy.New([]string{udp})
+	state := request.Request{W: &mtest.ResponseWriter{}, Req: new(dns.Msg)}
+
+	for _, tc := range dsTestCases {
+		resp, err := p.Lookup(state, tc.Qname, tc.Qtype)
+		if err != nil || resp == nil {
+			t.Fatalf("Expected to receive reply, but didn't for %s %d", tc.Qname, tc.Qtype)
+		}
+
+		sort.Sort(mtest.RRSet(resp.Answer))
+		sort.Sort(mtest.RRSet(resp.Ns))
+		sort.Sort(mtest.RRSet(resp.Extra))
+
+		if !mtest.Header(t, tc, resp) {
+			t.Logf("%v\n", resp)
+			continue
+		}
+		if !mtest.Section(t, tc, mtest.Answer, resp.Answer) {
+			t.Logf("%v\n", resp)
+		}
+		if !mtest.Section(t, tc, mtest.Ns, resp.Ns) {
+			t.Logf("%v\n", resp)
+		}
+		if !mtest.Section(t, tc, mtest.Extra, resp.Extra) {
+			t.Logf("%v\n", resp)
+		}
+	}
+}
diff --git a/test/miek_test.go b/test/miek_test.go
new file mode 100644
index 000000000..2778817a3
--- /dev/null
+++ b/test/miek_test.go
@@ -0,0 +1,31 @@
+package test
+
+const miekNL = `; miek.nl test zone
+$TTL    30M
+$ORIGIN miek.nl.
+@       IN      SOA     linode.atoom.net. miek.miek.nl. (
+			     1282630059 ; Serial
+                             4H         ; Refresh
+                             1H         ; Retry
+                             7D         ; Expire
+                             4H )       ; Negative Cache TTL
+                IN      NS      linode.atoom.net.
+		IN	NS	ns-ext.nlnetlabs.nl.
+		IN	NS	omval.tednet.nl.
+                IN      NS      ext.ns.whyscream.net.
+
+                IN      MX      1  aspmx.l.google.com.
+                IN      MX      5  alt1.aspmx.l.google.com.
+                IN      MX      5  alt2.aspmx.l.google.com.
+                IN      MX      10 aspmx2.googlemail.com.
+                IN      MX      10 aspmx3.googlemail.com.
+
+		IN 	A       176.58.119.54
+		IN 	AAAA    2a01:7e00::f03c:91ff:fe79:234c
+                IN      HINFO "Please stop asking for ANY" "See draft-ietf-dnsop-refuse-any"
+
+a		IN 	A       176.58.119.54
+		IN 	AAAA    2a01:7e00::f03c:91ff:fe79:234c
+www     	IN 	CNAME 	a
+archive         IN      CNAME   a
+`