NSEC/3 Cover+Match cleanup (#479)

* Initial NSEC cleanup

* Use readable names

* Finish tests

* Use existing functions, simplify hash/zone split

* Make IsSubDomain case insensitive, add tests

* Use internal Split instead of strings.Split

labels.go

@@ -1,5 +1,7 @@
 package dns
 
+import "strings"
+
 // Holds a bunch of helper functions for dealing with labels.
 
 // SplitDomainName splits a name string into it's labels.
@@ -50,6 +52,7 @@ func SplitDomainName(s string) (labels []string) {
 //
 // s1 and s2 must be syntactically valid domain names.
 func CompareDomainName(s1, s2 string) (n int) {
+	s1, s2 = strings.ToLower(s1), strings.ToLower(s2)
 	s1 = Fqdn(s1)
 	s2 = Fqdn(s2)
 	l1 := Split(s1)

labels_test.go

@@ -33,6 +33,9 @@ func TestCompareDomainName(t *testing.T) {
 	if CompareDomainName(".", ".") != 0 {
 		t.Errorf("%s with %s should be %d", ".", ".", 0)
 	}
+	if CompareDomainName("test.com.", "TEST.COM.") != 2 {
+		t.Errorf("test.com. and TEST.COM. should be an exact match")
+	}
 }
 
 func TestSplit(t *testing.T) {

nsecx.go

@@ -48,62 +48,50 @@ func HashName(label string, ha uint8, iter uint16, salt string) string {
 	return toBase32(nsec3)
 }
 
-// Denialer is an interface that should be implemented by types that are used to denial
+// Cover returns true if a name is covered by the NSEC3 record
-// answers in DNSSEC.
-type Denialer interface {
-	// Cover will check if the (unhashed) name is being covered by this NSEC or NSEC3.
-	Cover(name string) bool
-	// Match will check if the ownername matches the (unhashed) name for this NSEC3 or NSEC3.
-	Match(name string) bool
-}
-
-// Cover implements the Denialer interface.
-func (rr *NSEC) Cover(name string) bool {
-	return true
-}
-
-// Match implements the Denialer interface.
-func (rr *NSEC) Match(name string) bool {
-	return true
-}
-
-// Cover implements the Denialer interface.
 func (rr *NSEC3) Cover(name string) bool {
-	// FIXME(miek): check if the zones match
+	nameHash := HashName(name, rr.Hash, rr.Iterations, rr.Salt)
-	// FIXME(miek): check if we're not dealing with parent nsec3
+	owner := strings.ToUpper(rr.Hdr.Name)
-	hname := HashName(name, rr.Hash, rr.Iterations, rr.Salt)
+	labelIndices := Split(owner)
-	labels := Split(rr.Hdr.Name)
+	if len(labelIndices) < 2 {
-	if len(labels) < 2 {
 		return false
 	}
-	hash := strings.ToUpper(rr.Hdr.Name[labels[0] : labels[1]-1]) // -1 to remove the dot
+	ownerHash := owner[:labelIndices[1]-1]
-	if hash == rr.NextDomain {
+	ownerZone := owner[labelIndices[1]:]
-		return false // empty interval
+	if !IsSubDomain(ownerZone, strings.ToUpper(name)) { // name is outside owner zone
-	}
-	if hash > rr.NextDomain { // last name, points to apex
-		// hname > hash
-		// hname > rr.NextDomain
-		// TODO(miek)
-	}
-	if hname <= hash {
 		return false
 	}
-	if hname >= rr.NextDomain {
+
+	nextHash := rr.NextDomain
+	if ownerHash == nextHash { // empty interval
 		return false
 	}
-	return true
+	if ownerHash > nextHash { // end of zone
+		if nameHash > ownerHash { // covered since there is nothing after ownerHash
+			return true
+		}
+		return nameHash < nextHash // if nameHash is before beginning of zone it is covered
+	}
+	if nameHash < ownerHash { // nameHash is before ownerHash, not covered
+		return false
+	}
+	return nameHash < nextHash // if nameHash is before nextHash is it covered (between ownerHash and nextHash)
 }
 
-// Match implements the Denialer interface.
+// Match returns true if a name matches the NSEC3 record
 func (rr *NSEC3) Match(name string) bool {
-	// FIXME(miek): Check if we are in the same zone
+	nameHash := HashName(name, rr.Hash, rr.Iterations, rr.Salt)
-	hname := HashName(name, rr.Hash, rr.Iterations, rr.Salt)
+	owner := strings.ToUpper(rr.Hdr.Name)
-	labels := Split(rr.Hdr.Name)
+	labelIndices := Split(owner)
-	if len(labels) < 2 {
+	if len(labelIndices) < 2 {
 		return false
 	}
-	hash := strings.ToUpper(rr.Hdr.Name[labels[0] : labels[1]-1]) // -1 to remove the .
+	ownerHash := owner[:labelIndices[1]-1]
-	if hash == hname {
+	ownerZone := owner[labelIndices[1]:]
+	if !IsSubDomain(ownerZone, strings.ToUpper(name)) { // name is outside owner zone
+		return false
+	}
+	if ownerHash == nameHash {
 		return true
 	}
 	return false

nsecx_test.go

@@ -1,8 +1,6 @@
 package dns
 
-import (
+import "testing"
-	"testing"
-)
 
 func TestPackNsec3(t *testing.T) {
 	nsec3 := HashName("dnsex.nl.", SHA1, 0, "DEAD")
@@ -17,13 +15,119 @@ func TestPackNsec3(t *testing.T) {
 }
 
 func TestNsec3(t *testing.T) {
-	// examples taken from .nl
+	nsec3, _ := NewRR("sk4e8fj94u78smusb40o1n0oltbblu2r.nl. IN NSEC3 1 1 5 F10E9F7EA83FC8F3 SK4F38CQ0ATIEI8MH3RGD0P5I4II6QAN NS SOA TXT RRSIG DNSKEY NSEC3PARAM")
-	nsec3, _ := NewRR("39p91242oslggest5e6a7cci4iaeqvnk.nl. IN NSEC3 1 1 5 F10E9F7EA83FC8F3 39P99DCGG0MDLARTCRMCF6OFLLUL7PR6 NS DS RRSIG")
+	if !nsec3.(*NSEC3).Match("nl.") { // name hash = sk4e8fj94u78smusb40o1n0oltbblu2r
-	if !nsec3.(*NSEC3).Cover("snasajsksasasa.nl.") { // 39p94jrinub66hnpem8qdpstrec86pg3
+		t.Fatal("sk4e8fj94u78smusb40o1n0oltbblu2r.nl. should match sk4e8fj94u78smusb40o1n0oltbblu2r.nl.")
-		t.Error("39p94jrinub66hnpem8qdpstrec86pg3. should be covered by 39p91242oslggest5e6a7cci4iaeqvnk.nl. - 39P99DCGG0MDLARTCRMCF6OFLLUL7PR6")
 	}
-	nsec3, _ = NewRR("sk4e8fj94u78smusb40o1n0oltbblu2r.nl. IN NSEC3 1 1 5 F10E9F7EA83FC8F3 SK4F38CQ0ATIEI8MH3RGD0P5I4II6QAN NS SOA TXT RRSIG DNSKEY NSEC3PARAM")
+	if !nsec3.(*NSEC3).Match("NL.") { // name hash = sk4e8fj94u78smusb40o1n0oltbblu2r
-	if !nsec3.(*NSEC3).Match("nl.") { // sk4e8fj94u78smusb40o1n0oltbblu2r.nl.
+		t.Fatal("sk4e8fj94u78smusb40o1n0oltbblu2r.NL. should match sk4e8fj94u78smusb40o1n0oltbblu2r.nl.")
-		t.Error("sk4e8fj94u78smusb40o1n0oltbblu2r.nl. should match sk4e8fj94u78smusb40o1n0oltbblu2r.nl.")
+	}
+	if nsec3.(*NSEC3).Match("com.") { //
+		t.Fatal("com. is not in the zone nl.")
+	}
+	if nsec3.(*NSEC3).Match("test.nl.") { // name hash = gd0ptr5bnfpimpu2d3v6gd4n0bai7s0q
+		t.Fatal("gd0ptr5bnfpimpu2d3v6gd4n0bai7s0q.nl. should not match sk4e8fj94u78smusb40o1n0oltbblu2r.nl.")
+	}
+	nsec3, _ = NewRR("nl. IN NSEC3 1 1 5 F10E9F7EA83FC8F3 SK4F38CQ0ATIEI8MH3RGD0P5I4II6QAN NS SOA TXT RRSIG DNSKEY NSEC3PARAM")
+	if nsec3.(*NSEC3).Match("nl.") {
+		t.Fatal("sk4e8fj94u78smusb40o1n0oltbblu2r.nl. should not match a record without a owner hash")
+	}
+
+	for _, tc := range []struct {
+		rr     *NSEC3
+		name   string
+		covers bool
+	}{
+		// positive tests
+		{ // name hash between owner hash and next hash
+			rr: &NSEC3{
+				Hdr:        RR_Header{Name: "2N1TB3VAIRUOBL6RKDVII42N9TFMIALP.com."},
+				Hash:       1,
+				Flags:      1,
+				Iterations: 5,
+				Salt:       "F10E9F7EA83FC8F3",
+				NextDomain: "PT3RON8N7PM3A0OE989IB84OOSADP7O8",
+			},
+			name:   "bsd.com.",
+			covers: true,
+		},
+		{ // end of zone, name hash is after owner hash
+			rr: &NSEC3{
+				Hdr:        RR_Header{Name: "3v62ulr0nre83v0rja2vjgtlif9v6rab.com."},
+				Hash:       1,
+				Flags:      1,
+				Iterations: 5,
+				Salt:       "F10E9F7EA83FC8F3",
+				NextDomain: "2N1TB3VAIRUOBL6RKDVII42N9TFMIALP",
+			},
+			name:   "csd.com.",
+			covers: true,
+		},
+		{ // end of zone, name hash is before beginning of zone
+			rr: &NSEC3{
+				Hdr:        RR_Header{Name: "PT3RON8N7PM3A0OE989IB84OOSADP7O8.com."},
+				Hash:       1,
+				Flags:      1,
+				Iterations: 5,
+				Salt:       "F10E9F7EA83FC8F3",
+				NextDomain: "3V62ULR0NRE83V0RJA2VJGTLIF9V6RAB",
+			},
+			name:   "asd.com.",
+			covers: true,
+		},
+		// negative tests
+		{ // too short owner name
+			rr: &NSEC3{
+				Hdr:        RR_Header{Name: "nl."},
+				Hash:       1,
+				Flags:      1,
+				Iterations: 5,
+				Salt:       "F10E9F7EA83FC8F3",
+				NextDomain: "39P99DCGG0MDLARTCRMCF6OFLLUL7PR6",
+			},
+			name:   "asd.com.",
+			covers: false,
+		},
+		{ // outside of zone
+			rr: &NSEC3{
+				Hdr:        RR_Header{Name: "39p91242oslggest5e6a7cci4iaeqvnk.nl."},
+				Hash:       1,
+				Flags:      1,
+				Iterations: 5,
+				Salt:       "F10E9F7EA83FC8F3",
+				NextDomain: "39P99DCGG0MDLARTCRMCF6OFLLUL7PR6",
+			},
+			name:   "asd.com.",
+			covers: false,
+		},
+		{ // empty interval
+			rr: &NSEC3{
+				Hdr:        RR_Header{Name: "2n1tb3vairuobl6rkdvii42n9tfmialp.com."},
+				Hash:       1,
+				Flags:      1,
+				Iterations: 5,
+				Salt:       "F10E9F7EA83FC8F3",
+				NextDomain: "2N1TB3VAIRUOBL6RKDVII42N9TFMIALP",
+			},
+			name:   "asd.com.",
+			covers: false,
+		},
+		{ // name hash is before owner hash, not covered
+			rr: &NSEC3{
+				Hdr:        RR_Header{Name: "3V62ULR0NRE83V0RJA2VJGTLIF9V6RAB.com."},
+				Hash:       1,
+				Flags:      1,
+				Iterations: 5,
+				Salt:       "F10E9F7EA83FC8F3",
+				NextDomain: "PT3RON8N7PM3A0OE989IB84OOSADP7O8",
+			},
+			name:   "asd.com.",
+			covers: false,
+		},
+	} {
+		covers := tc.rr.Cover(tc.name)
+		if tc.covers != covers {
+			t.Fatalf("Cover failed for %s: expected %t, got %t [record: %s]", tc.name, tc.covers, covers, tc.rr)
+		}
 	}
 }