diff options
author | 2019-12-06 19:54:31 +0000 | |
---|---|---|
committer | 2019-12-06 19:54:31 +0000 | |
commit | a53321d9d6bdc80cc4aa88982f401ef2e3267e34 (patch) | |
tree | 92d2917113a570c43e1635fcc4a308bae234c569 | |
parent | 799dce4bffeb6d40b5b15cca47f34d4a7d24e6d3 (diff) | |
download | coredns-a53321d9d6bdc80cc4aa88982f401ef2e3267e34.tar.gz coredns-a53321d9d6bdc80cc4aa88982f401ef2e3267e34.tar.zst coredns-a53321d9d6bdc80cc4aa88982f401ef2e3267e34.zip |
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>
-rw-r--r-- | plugin/file/tree/auth_walk.go | 58 | ||||
-rw-r--r-- | plugin/file/tree/walk.go | 11 | ||||
-rw-r--r-- | plugin/sign/README.md | 27 | ||||
-rw-r--r-- | plugin/sign/nsec.go | 23 | ||||
-rw-r--r-- | plugin/sign/nsec_test.go | 27 | ||||
-rw-r--r-- | plugin/sign/resign_test.go | 2 | ||||
-rw-r--r-- | plugin/sign/signer.go | 51 | ||||
-rw-r--r-- | plugin/sign/signer_test.go | 96 | ||||
-rw-r--r-- | plugin/sign/testdata/db.miek.nl_ns | 10 |
9 files changed, 235 insertions, 70 deletions
diff --git a/plugin/file/tree/auth_walk.go b/plugin/file/tree/auth_walk.go new file mode 100644 index 000000000..1f436716f --- /dev/null +++ b/plugin/file/tree/auth_walk.go @@ -0,0 +1,58 @@ +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 +} diff --git a/plugin/file/tree/walk.go b/plugin/file/tree/walk.go index 00accbafa..e315eb03f 100644 --- a/plugin/file/tree/walk.go +++ b/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 diff --git a/plugin/sign/README.md b/plugin/sign/README.md index d2782816a..90d687e59 100644 --- a/plugin/sign/README.md +++ b/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. diff --git a/plugin/sign/nsec.go b/plugin/sign/nsec.go index d726ade72..d7c6a30a3 100644 --- a/plugin/sign/nsec.go +++ b/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. diff --git a/plugin/sign/nsec_test.go b/plugin/sign/nsec_test.go new file mode 100644 index 000000000..f272651fc --- /dev/null +++ b/plugin/sign/nsec_test.go @@ -0,0 +1,27 @@ +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]) + } + } +} diff --git a/plugin/sign/resign_test.go b/plugin/sign/resign_test.go index e2571cb5b..2f67f52aa 100644 --- a/plugin/sign/resign_test.go +++ b/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 { diff --git a/plugin/sign/signer.go b/plugin/sign/signer.go index a9a5f2e5a..b0e2f02fe 100644 --- a/plugin/sign/signer.go +++ b/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 } diff --git a/plugin/sign/signer_test.go b/plugin/sign/signer_test.go index 6ba94247d..bb364728e 100644 --- a/plugin/sign/signer_test.go +++ b/plugin/sign/signer_test.go @@ -1,6 +1,7 @@ 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) +} diff --git a/plugin/sign/testdata/db.miek.nl_ns b/plugin/sign/testdata/db.miek.nl_ns new file mode 100644 index 000000000..bd2371f16 --- /dev/null +++ b/plugin/sign/testdata/db.miek.nl_ns @@ -0,0 +1,10 @@ +$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 |