plugin/sign: fix signing of authoritative data (#3479)

Don't sign data we are not authoritative for. This adds an AuthWalk
which skips names we should not authoritative for. Adds a few tests to
check this is the case. Generates zones have been compared to
dnssec-signzone.

A number of changes have been made:

* don't add DS records to the apex
* NSEC TTL is the SOA's minttl value (copying bind9)
* Various cleanups
* signer struct was cleaned up: doesn't need ttl, nor expiration or
  inception.
* plugin/sign: remove apex stuff from names()
  This is never used because we will always have other types in the
  apex, because we *ADD* them ourselves, before we sign (DNSKEY, CDS and
  CDNSKEY).
Signed-off-by: Miek Gieben <miek@miek.nl>
Co-Authored-By: Chris O'Haver <cohaver@infoblox.com>

plugin/file/tree/auth_walk.go

+package tree
+
+import (
+	"github.com/miekg/dns"
+)
+
+// AuthWalk performs fn on all authoritative values stored in the tree in
+// pre-order depth first. If a non-nil error is returned the AuthWalk was interrupted
+// by an fn returning that error. If fn alters stored values' sort
+// relationships, future tree operation behaviors are undefined.
+//
+// The fn function will be called with 3 arguments, the current element, a map containing all
+// the RRs for this element and a boolean if this name is considered authoritative.
+func (t *Tree) AuthWalk(fn func(*Elem, map[uint16][]dns.RR, bool) error) error {
+	if t.Root == nil {
+		return nil
+	}
+	return t.Root.authwalk(make(map[string]struct{}), fn)
+}
+
+func (n *Node) authwalk(ns map[string]struct{}, fn func(*Elem, map[uint16][]dns.RR, bool) error) error {
+	if n.Left != nil {
+		if err := n.Left.authwalk(ns, fn); err != nil {
+			return err
+		}
+	}
+
+	// Check if the current name is a subdomain of *any* of the delegated names we've seen, if so, skip this name.
+	// The ordering of the tree and how we walk if guarantees we see parents first.
+	if n.Elem.Type(dns.TypeNS) != nil {
+		ns[n.Elem.Name()] = struct{}{}
+	}
+
+	auth := true
+	i := 0
+	for {
+		j, end := dns.NextLabel(n.Elem.Name(), i)
+		if end {
+			break
+		}
+		if _, ok := ns[n.Elem.Name()[j:]]; ok {
+			auth = false
+			break
+		}
+		i++
+	}
+
+	if err := fn(n.Elem, n.Elem.m, auth); err != nil {
+		return err
+	}
+
+	if n.Right != nil {
+		if err := n.Right.authwalk(ns, fn); err != nil {
+			return err
+		}
+	}
+	return nil
+}

plugin/file/tree/walk.go

@@ -2,25 +2,28 @@ package tree
 
 import "github.com/miekg/dns"
 
-// Walk performs fn on all values stored in the tree. If a non-nil error is returned the
-// Walk was interrupted by an fn returning that error. If fn alters stored values' sort
+// Walk performs fn on all authoritative values stored in the tree in
+// in-order depth first. If a non-nil error is returned the Walk was interrupted
+// by an fn returning that error. If fn alters stored values' sort
 // relationships, future tree operation behaviors are undefined.
-func (t *Tree) Walk(fn func(e *Elem, rrs map[uint16][]dns.RR) error) error {
+func (t *Tree) Walk(fn func(*Elem, map[uint16][]dns.RR) error) error {
 	if t.Root == nil {
 		return nil
 	}
 	return t.Root.walk(fn)
 }
 
-func (n *Node) walk(fn func(e *Elem, rrs map[uint16][]dns.RR) error) error {
+func (n *Node) walk(fn func(*Elem, map[uint16][]dns.RR) error) error {
 	if n.Left != nil {
 		if err := n.Left.walk(fn); err != nil {
 			return err
 		}
 	}
+
 	if err := fn(n.Elem, n.Elem.m); err != nil {
 		return err
 	}
+
 	if n.Right != nil {
 		if err := n.Right.walk(fn); err != nil {
 			return err

plugin/sign/README.md

@@ -9,8 +9,7 @@
 The *sign* plugin is used to sign (see RFC 6781) zones. In this process DNSSEC resource records are
 added. The signatures that sign the resource records sets have an expiration date, this means the
 signing process must be repeated before this expiration data is reached. Otherwise the zone's data
-will go BAD (RFC 4035, Section 5.5). The *sign* plugin takes care of this. *Sign* works, but has
-a couple of limitations, see the "Bugs" section.
+will go BAD (RFC 4035, Section 5.5). The *sign* plugin takes care of this.
 
 Only NSEC is supported, *sign* does not support NSEC3.
 
@@ -32,17 +31,21 @@ it do key or algorithm rollovers - it just signs.
 
     Both these dates are only checked on the SOA's signature(s).
 
- *  Create signatures that have an inception of -3 hours (minus a jitter between 0 and 18 hours)
+ *  Create RRSIGs that have an inception of -3 hours (minus a jitter between 0 and 18 hours)
     and a expiration of +32 days for every given DNSKEY.
 
+ *  Add NSEC records for all names in the zone. The TTL for these is the negative cache TTL from the
+    SOA record.
+
  *  Add or replace *all* apex CDS/CDNSKEY records with the ones derived from the given keys. For
     each key two CDS are created one with SHA1 and another with SHA256.
 
  *  Update the SOA's serial number to the *Unix epoch* of when the signing happens. This will
     overwrite *any* previous serial number.
 
-Thus there are two ways that dictate when a zone is signed. Normally every 6 days (plus jitter) it
-will be resigned. If for some reason we fail this check, the 14 days before expiring kicks in.
+
+There are two ways that dictate when a zone is signed. Normally every 6 days (plus jitter) it will
+be resigned. If for some reason we fail this check, the 14 days before expiring kicks in.
 
 Keys are named (following BIND9): `K<name>+<alg>+<id>.key` and `K<name>+<alg>+<id>.private`.
 The keys **must not** be included in your zone; they will be added by *sign*. These keys can be
@@ -144,8 +147,8 @@ example.org example.net {
 This will lead to `db.example.org` be signed *twice*, as this entire section is parsed twice because
 you have specified the origins `example.org` and `example.net` in the server block.
 
-Forcibly resigning a zone can be accomplished by removing the signed zone file (CoreDNS will keep on
-serving it from memory), and sending SIGUSR1 to the process to make it reload and resign the zone
+Forcibly resigning a zone can be accomplished by removing the signed zone file (CoreDNS will keep
+on serving it from memory), and sending SIGUSR1 to the process to make it reload and resign the zone
 file.
 
 ## Also See
@@ -153,9 +156,13 @@ file.
 The DNSSEC RFCs: RFC 4033, RFC 4034 and RFC 4035. And the BCP on DNSSEC, RFC 6781. Further more the
 manual pages coredns-keygen(1) and dnssec-keygen(8). And the *file* plugin's documentation.
 
-Coredns-keygen can be found at <https://github.com/coredns/coredns-utils> in the coredns-keygen directory.
+Coredns-keygen can be found at
+[https://github.com/coredns/coredns-utils](https://github.com/coredns/coredns-utils) in the
+coredns-keygen directory.
+
+Other useful DNSSEC tools can be found in [ldns](https://nlnetlabs.nl/projects/ldns/about/), e.g.
+`ldns-key2ds` to create DS records from DNSKEYs.
 
 ## Bugs
 
-`keys directory` is not implemented. Glue records are currently signed, and no DS records are added
-for child zones.
+`keys directory` is not implemented.

plugin/sign/nsec.go

@@ -9,24 +9,19 @@ import (
 	"github.com/miekg/dns"
 )
 
-// names returns the elements of the zone in nsec order. If the returned boolean is true there were
-// no other apex records than SOA and NS, which are stored separately.
-func names(origin string, z *file.Zone) ([]string, bool) {
-	// if there are no apex records other than NS and SOA we'll miss the origin
-	// in this list. Check the first element and if not origin prepend it.
+// names returns the elements of the zone in nsec order.
+func names(origin string, z *file.Zone) []string {
+	// There will also be apex records other than NS and SOA (who are kept separate), as we
+	// are adding DNSKEY and CDS/CDNSKEY records in the apex *before* we sign.
 	n := []string{}
-	z.Walk(func(e *tree.Elem, _ map[uint16][]dns.RR) error {
+	z.AuthWalk(func(e *tree.Elem, _ map[uint16][]dns.RR, auth bool) error {
+		if !auth {
+			return nil
+		}
 		n = append(n, e.Name())
 		return nil
 	})
-	if len(n) == 0 {
-		return nil, false
-	}
-	if n[0] != origin {
-		n = append([]string{origin}, n...)
-		return n, true
-	}
-	return n, false
+	return n
 }
 
 // NSEC returns an NSEC record according to name, next, ttl and bitmap. Note that the bitmap is sorted before use.

plugin/sign/nsec_test.go

+package sign
+
+import (
+	"os"
+	"testing"
+
+	"github.com/coredns/coredns/plugin/file"
+)
+
+func TestNames(t *testing.T) {
+	f, err := os.Open("testdata/db.miek.nl_ns")
+	if err != nil {
+		t.Error(err)
+	}
+	z, err := file.Parse(f, "db.miek.nl_ns", "miek.nl", 0)
+	if err != nil {
+		t.Error(err)
+	}
+
+	names := names("miek.nl.", z)
+	expected := []string{"miek.nl.", "child.miek.nl.", "www.miek.nl."}
+	for i := range names {
+		if names[i] != expected[i] {
+			t.Errorf("Expected %s, got %s", expected[i], names[i])
+		}
+	}
+}

plugin/sign/resign_test.go

@@ -13,7 +13,6 @@ func TestResignInception(t *testing.T) {
 	if x := resign(zr, then); x != nil {
 		t.Errorf("Expected RRSIG to be valid for %s, got invalid: %s", then.Format(timeFmt), x)
 	}
-
 	// inception starts after this date.
 	zr = strings.NewReader(`miek.nl.	1800	IN	RRSIG	SOA 13 2 1800 20190808191936 20190731161936 59725 miek.nl. eU6gI1OkSEbyt`)
 	if x := resign(zr, then); x == nil {
@@ -33,7 +32,6 @@ func TestResignExpire(t *testing.T) {
 	if x := resign(zr, then); x != nil {
 		t.Errorf("Expected RRSIG to be valid for %s, got invalid: %s", then.Format(timeFmt), x)
 	}
-
 	// expired yesterday
 	zr = strings.NewReader(`miek.nl.	1800	IN	RRSIG	SOA 13 2 1800 20190721191936 20190717161936 59725 miek.nl. eU6gI1OkSEbyt`)
 	if x := resign(zr, then); x == nil {

plugin/sign/signer.go

@@ -26,10 +26,6 @@ type Signer struct {
 
 	signedfile string
 	stop       chan struct{}
-
-	expiration uint32
-	inception  uint32
-	ttl        uint32
 }
 
 // Sign signs a zone file according to the parameters in s.
@@ -44,46 +40,31 @@ func (s *Signer) Sign(now time.Time) (*file.Zone, error) {
 		return nil, err
 	}
 
-	s.inception, s.expiration = lifetime(now, s.jitter)
-
-	s.ttl = z.Apex.SOA.Header().Ttl
+	mttl := z.Apex.SOA.Minttl
+	ttl := z.Apex.SOA.Header().Ttl
+	inception, expiration := lifetime(now, s.jitter)
 	z.Apex.SOA.Serial = uint32(now.Unix())
 
 	for _, pair := range s.keys {
-		pair.Public.Header().Ttl = s.ttl // set TTL on key so it matches the RRSIG.
+		pair.Public.Header().Ttl = ttl // set TTL on key so it matches the RRSIG.
 		z.Insert(pair.Public)
-		z.Insert(pair.Public.ToDS(dns.SHA1))
-		z.Insert(pair.Public.ToDS(dns.SHA256))
 		z.Insert(pair.Public.ToDS(dns.SHA1).ToCDS())
 		z.Insert(pair.Public.ToDS(dns.SHA256).ToCDS())
 		z.Insert(pair.Public.ToCDNSKEY())
 	}
 
-	names, apex := names(s.origin, z)
+	names := names(s.origin, z)
 	ln := len(names)
 
-	var nsec *dns.NSEC
-	if apex {
-		nsec = NSEC(s.origin, names[(ln+1)%ln], s.ttl, []uint16{dns.TypeSOA, dns.TypeNS, dns.TypeRRSIG, dns.TypeNSEC})
-		z.Insert(nsec)
-	}
-
 	for _, pair := range s.keys {
-		rrsig, err := pair.signRRs([]dns.RR{z.Apex.SOA}, s.origin, s.ttl, s.inception, s.expiration)
+		rrsig, err := pair.signRRs([]dns.RR{z.Apex.SOA}, s.origin, ttl, inception, expiration)
 		if err != nil {
 			return nil, err
 		}
 		z.Insert(rrsig)
 		// NS apex may not be set if RR's have been discarded because the origin doesn't match.
 		if len(z.Apex.NS) > 0 {
-			rrsig, err = pair.signRRs(z.Apex.NS, s.origin, s.ttl, s.inception, s.expiration)
-			if err != nil {
-				return nil, err
-			}
-			z.Insert(rrsig)
-		}
-		if apex {
-			rrsig, err = pair.signRRs([]dns.RR{nsec}, s.origin, s.ttl, s.inception, s.expiration)
+			rrsig, err = pair.signRRs(z.Apex.NS, s.origin, ttl, inception, expiration)
 			if err != nil {
 				return nil, err
 			}
@@ -93,21 +74,27 @@ func (s *Signer) Sign(now time.Time) (*file.Zone, error) {
 
 	// We are walking the tree in the same direction, so names[] can be used here to indicated the next element.
 	i := 1
-	err = z.Walk(func(e *tree.Elem, zrrs map[uint16][]dns.RR) error {
-		if !apex && e.Name() == s.origin {
-			nsec := NSEC(e.Name(), names[(ln+i)%ln], s.ttl, append(e.Types(), dns.TypeNS, dns.TypeSOA, dns.TypeNSEC, dns.TypeRRSIG))
+	err = z.AuthWalk(func(e *tree.Elem, zrrs map[uint16][]dns.RR, auth bool) error {
+		if !auth {
+			return nil
+		}
+
+		if e.Name() == s.origin {
+			nsec := NSEC(e.Name(), names[(ln+i)%ln], mttl, append(e.Types(), dns.TypeNS, dns.TypeSOA, dns.TypeRRSIG, dns.TypeNSEC))
 			z.Insert(nsec)
 		} else {
-			nsec := NSEC(e.Name(), names[(ln+i)%ln], s.ttl, append(e.Types(), dns.TypeNSEC, dns.TypeRRSIG))
+			nsec := NSEC(e.Name(), names[(ln+i)%ln], mttl, append(e.Types(), dns.TypeRRSIG, dns.TypeNSEC))
 			z.Insert(nsec)
 		}
 
 		for t, rrs := range zrrs {
-			if t == dns.TypeRRSIG {
+			// RRSIGs are not signed and NS records are not signed because we are never authoratiative for them.
+			// The zone's apex nameservers records are not kept in this tree and are signed separately.
+			if t == dns.TypeRRSIG || t == dns.TypeNS {
 				continue
 			}
 			for _, pair := range s.keys {
-				rrsig, err := pair.signRRs(rrs, s.origin, s.ttl, s.inception, s.expiration)
+				rrsig, err := pair.signRRs(rrs, s.origin, rrs[0].Header().Ttl, inception, expiration)
 				if err != nil {
 					return err
 				}

plugin/sign/signer_test.go

 package sign
 
 import (
+	"bytes"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -29,8 +30,8 @@ func TestSign(t *testing.T) {
 	}
 
 	apex, _ := z.Search("miek.nl.")
-	if x := apex.Type(dns.TypeDS); len(x) != 2 {
-		t.Errorf("Expected %d DS records, got %d", 2, len(x))
+	if x := apex.Type(dns.TypeDS); len(x) != 0 {
+		t.Errorf("Expected %d DS records, got %d", 0, len(x))
 	}
 	if x := apex.Type(dns.TypeCDS); len(x) != 2 {
 		t.Errorf("Expected %d CDS records, got %d", 2, len(x))
@@ -75,14 +76,14 @@ $ORIGIN example.org.
 	if x := nsec[0].(*dns.NSEC).NextDomain; x != "example.org." {
 		t.Errorf("Expected NSEC NextDomain %s, got %s", "example.org.", x)
 	}
-	if x := nsec[0].(*dns.NSEC).TypeBitMap; len(x) != 8 {
-		t.Errorf("Expected NSEC bitmap to be %d elements, got %d", 8, x)
+	if x := nsec[0].(*dns.NSEC).TypeBitMap; len(x) != 7 {
+		t.Errorf("Expected NSEC bitmap to be %d elements, got %d", 7, x)
 	}
-	if x := nsec[0].(*dns.NSEC).TypeBitMap; x[7] != dns.TypeCDNSKEY {
-		t.Errorf("Expected NSEC bitmap element 6 to be %d, got %d", dns.TypeCDNSKEY, x[7])
+	if x := nsec[0].(*dns.NSEC).TypeBitMap; x[6] != dns.TypeCDNSKEY {
+		t.Errorf("Expected NSEC bitmap element 5 to be %d, got %d", dns.TypeCDNSKEY, x[6])
 	}
-	if x := nsec[0].(*dns.NSEC).TypeBitMap; x[5] != dns.TypeDNSKEY {
-		t.Errorf("Expected NSEC bitmap element 5 to be %d, got %d", dns.TypeDNSKEY, x[5])
+	if x := nsec[0].(*dns.NSEC).TypeBitMap; x[4] != dns.TypeDNSKEY {
+		t.Errorf("Expected NSEC bitmap element 4 to be %d, got %d", dns.TypeDNSKEY, x[4])
 	}
 	dnskey := el.Type(dns.TypeDNSKEY)
 	if x := dnskey[0].Header().Ttl; x != 1800 {
@@ -100,3 +101,82 @@ $ORIGIN example.org.
 		}
 	}
 }
+
+func TestSignGlue(t *testing.T) {
+	input := `sign testdata/db.miek.nl miek.nl {
+               key file testdata/Kmiek.nl.+013+59725
+               directory testdata
+       }`
+	c := caddy.NewTestController("dns", input)
+	sign, err := parse(c)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(sign.signers) != 1 {
+		t.Fatalf("Expected 1 signer, got %d", len(sign.signers))
+	}
+	z, err := sign.signers[0].Sign(time.Now().UTC())
+	if err != nil {
+		t.Error(err)
+	}
+
+	e, _ := z.Search("ns2.bla.miek.nl.")
+	sigs := e.Type(dns.TypeRRSIG)
+	if len(sigs) != 0 {
+		t.Errorf("Expected no RRSIG for %s, got %d", "ns2.bla.miek.nl.", len(sigs))
+	}
+}
+
+func TestSignDS(t *testing.T) {
+	input := `sign testdata/db.miek.nl_ns miek.nl {
+               key file testdata/Kmiek.nl.+013+59725
+               directory testdata
+       }`
+	c := caddy.NewTestController("dns", input)
+	sign, err := parse(c)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(sign.signers) != 1 {
+		t.Fatalf("Expected 1 signer, got %d", len(sign.signers))
+	}
+	z, err := sign.signers[0].Sign(time.Now().UTC())
+	if err != nil {
+		t.Error(err)
+	}
+
+	// dnssec-signzone outputs this for db.miek.nl_ns:
+	//
+	// child.miek.nl.	1800	IN	NS	ns.child.miek.nl.
+	// child.miek.nl.	1800	IN	DS	34385 13 2 fc7397c77afbccb6742fc....
+	// child.miek.nl.	1800	IN	RRSIG	DS 13 3 1800 20191223121229 20191123121229 59725 miek.nl. ZwptLzVVs....
+	// child.miek.nl.	14400	IN	NSEC	www.miek.nl. NS DS RRSIG NSEC
+	// child.miek.nl.	14400	IN	RRSIG	NSEC 13 3 14400 20191223121229 20191123121229 59725 miek.nl. w+CcA8...
+
+	name := "child.miek.nl."
+	e, _ := z.Search(name)
+	if x := len(e.Types()); x != 4 { // NS DS NSEC and 2x RRSIG
+		t.Errorf("Expected 4 records for %s, got %d", name, x)
+	}
+
+	ds := e.Type(dns.TypeDS)
+	if len(ds) != 1 {
+		t.Errorf("Expected DS for %s, got %d", name, len(ds))
+	}
+	sigs := e.Type(dns.TypeRRSIG)
+	if len(sigs) != 2 {
+		t.Errorf("Expected no RRSIG for %s, got %d", name, len(sigs))
+	}
+	nsec := e.Type(dns.TypeNSEC)
+	if x := nsec[0].(*dns.NSEC).NextDomain; x != "www.miek.nl." {
+		t.Errorf("Expected no NSEC NextDomain to be %s for %s, got %s", "www.miek.nl.", name, x)
+	}
+	minttl := z.Apex.SOA.Minttl
+	if x := nsec[0].Header().Ttl; x != minttl {
+		t.Errorf("Expected no NSEC TTL to be %d for %s, got %d", minttl, "www.miek.nl.", x)
+	}
+	// print zone on error
+	buf := &bytes.Buffer{}
+	write(buf, z)
+	t.Logf("%s\n", buf)
+}

plugin/sign/testdata/db.miek.nl_ns

+$TTL    30M
+$ORIGIN miek.nl.
+@       IN      SOA     linode.atoom.net. miek.miek.nl. ( 1282630060 4H 1H 7D 4H )
+                NS      linode.atoom.net.
+		DNSKEY  257 3 13 sfzRg5nDVxbeUc51su4MzjgwpOpUwnuu81SlRHqJuXe3SOYOeypR69tZ52XLmE56TAmPHsiB8Rgk+NTpf0o1Cw==
+
+www 	        AAAA    ::1
+child           NS      ns.child
+ns.child        AAAA    ::1
+child	        DS	34385 13 2 fc7397c77afbccb6742fcff19c7b1410d0044661e7085fc200ae1ab3d15a5842