bugfix/dnssec: sign NSECs correctly

previously, adding RRSIG to the signed node confused
iteration over RRSets, leading to duplicit RRSIGs
of some NSEC(3)s

diff --git a/src/knot/dnssec/zone-sign.c b/src/knot/dnssec/zone-sign.c
index c23042ce5eea25b5856c96a5bc1bc333c51d3c52..a945abad4e05197bc4c770a14605805c8147f42d 100644 (file)
--- a/src/knot/dnssec/zone-sign.c
+++ b/src/knot/dnssec/zone-sign.c
@@ -975,6 +975,20 @@ bool knot_zone_sign_soa_expired(const zone_contents_t *zone,
        return !exist;
 }
 
+static int sign_in_changeset(zone_node_t *node, uint16_t rrtype, knot_rrset_t *rrsigs,
+                             zone_sign_ctx_t *sign_ctx, int ret_prev,
+                             bool skip_crypto, zone_update_t *up)
+{
+       if (ret_prev != KNOT_EOK) {
+               return ret_prev;
+       }
+       knot_rrset_t rr = node_rrset(node, rrtype);
+       if (knot_rrset_empty(&rr)) {
+               return KNOT_EOK;
+       }
+       return add_missing_rrsigs(&rr, rrsigs, sign_ctx, skip_crypto, NULL, up, NULL);
+}
+
 int knot_zone_sign_nsecs_in_changeset(const zone_keyset_t *zone_keys,
                                       const kdnssec_ctx_t *dnssec_ctx,
                                       zone_update_t *update)
@@ -996,14 +1010,9 @@ int knot_zone_sign_nsecs_in_changeset(const zone_keyset_t *zone_keys,
                bool skip_crypto = (n->flags & NODE_FLAGS_RRSIGS_VALID) && !dnssec_ctx->keytag_conflict;
 
                knot_rrset_t rrsigs = node_rrset(n, KNOT_RRTYPE_RRSIG);
-               for (int i = 0; i < n->rrset_count; i++) {
-                       knot_rrset_t rr = node_rrset_at(n, i);
-                       if (rr.type == KNOT_RRTYPE_NSEC ||
-                           rr.type == KNOT_RRTYPE_NSEC3 ||
-                           rr.type == KNOT_RRTYPE_NSEC3PARAM) {
-                               ret =  add_missing_rrsigs(&rr, &rrsigs, sign_ctx, skip_crypto, NULL, update, NULL);
-                       }
-               }
+               ret = sign_in_changeset(n, KNOT_RRTYPE_NSEC, &rrsigs, sign_ctx, ret, skip_crypto, update);
+               ret = sign_in_changeset(n, KNOT_RRTYPE_NSEC3, &rrsigs, sign_ctx, ret, skip_crypto, update);
+               ret = sign_in_changeset(n, KNOT_RRTYPE_NSEC3PARAM, &rrsigs, sign_ctx, ret, skip_crypto, update);
 
                if (ret == KNOT_EOK) {
                        n->flags |= NODE_FLAGS_RRSIGS_VALID; // non-NSEC RRSIGs had been validated in knot_dnssec_sign_update()

diff --git a/tests-extra/tests/dnssec/nsec_update/test.py b/tests-extra/tests/dnssec/nsec_update/test.py
index 666c4d9d19d797c213ba4178f7c6a77d6cee52ce..0ccb6579a23ab92ff87d7be10faffecc57fd3199 100644 (file)
--- a/tests-extra/tests/dnssec/nsec_update/test.py
+++ b/tests-extra/tests/dnssec/nsec_update/test.py
@@ -6,6 +6,40 @@ from dnstest.utils import *
 from dnstest.test import Test
 from dnstest.keys import Keymgr
 import random
+import dns
+
+def check_nsec(server, zone, msg, name="dwidjwoij"):
+
+    q = server.dig(name + "." + zone.name, "AAAA", dnssec=True, udp=False)
+    found_soas = q.count("SOA", section="authority")
+    found_nsecs = q.count("NSEC", section="authority")
+    if found_nsecs == 0:
+        found_nsecs = q.count("NSEC3", section="authority")
+    found_rrsigs = q.count("RRSIG", section="authority")
+
+    check_log("Authority %s: %s" % (zone.name, msg))
+    check_log("SOAs: %d" % found_soas)
+    check_log("NSECs: %d" % found_nsecs)
+    check_log("RRSIGs: %d" % found_rrsigs)
+
+    if found_soas != 1:
+        set_err("No SOA authority (%d): %s" % (found_soas, msg))
+
+    if found_nsecs < 1:
+        set_err("No NSEC(3) authority: %s" % msg)
+
+    if found_nsecs > 3:
+        set_err("Too many NSEC(3)s authority (%d): %s" % (found_nsecs, msg))
+
+    if found_rrsigs != found_soas + found_nsecs:
+        set_err("Unmatching RRSIGs (%d != %d + %d): %s" % (found_rrsigs, found_soas, found_nsecs, msg))
+        detail_log("Unmatching RRSIGs [%s] (%d != %d + %d): %s" % (zone.name, found_rrsigs, found_soas, found_nsecs, msg))
+        for data in q.resp.authority:
+            rrset = data.to_rdataset()
+            if rrset.rdtype == dns.rdatatype.NSEC or rrset.rdtype == dns.rdatatype.NSEC3 or rrset.rdtype == dns.rdatatype.RRSIG:
+                detail_log(str(data))
+
+    detail_log(SEP)
 
 t = Test()
 
@@ -44,6 +78,8 @@ if master.valgrind:
 
 t.start()
 master.zones_wait(zones)
+for z in zones:
+    check_nsec(master, z, "Initial")
 master.ctl("zone-flush")
 slave.ctl("zone-refresh")
 slave.zones_wait(zones)
@@ -67,6 +103,8 @@ t.sleep(1)
 master.ctl("zone-refresh", wait=True)
 
 after_update = master.zones_wait(zones)
+for z in zones:
+    check_nsec(master, z, "After DDNS")
 
 # sync slave with current master's state
 slave.ctl("zone-refresh")
@@ -78,6 +116,8 @@ slave.flush(wait=True)
 # re-sign master and check that the re-sign made nothing
 master.ctl("zone-sign")
 after_update15 = master.zones_wait(zones, after_update, equal=False, greater=True)
+for z in zones:
+    check_nsec(master, z, "After re-sign")
 
 t.xfr_diff(master, slave, zones, no_rrsig_rdata=True)
 for zone in zones:
@@ -114,6 +154,8 @@ t.sleep(1)
 master.ctl("zone-refresh", wait=True)
 
 after_update2 = master.zones_wait(zones, after_update15, equal=False, greater=True)
+for z in zones:
+    check_nsec(master, z, "After delegation update")
 
 # sync slave with current master's state
 slave.ctl("zone-refresh")
@@ -125,6 +167,8 @@ slave.flush(wait=True)
 # re-sign master and check that the re-sign made nothing
 master.ctl("zone-sign")
 after_update25 = master.zones_wait(zones, after_update2, equal=False, greater=True)
+for z in zones:
+    check_nsec(master, z, "After second re-sign")
 
 t.xfr_diff(master, slave, zones, no_rrsig_rdata=True)
 for zone in zones:
@@ -148,4 +192,7 @@ for z in zones1:
     slave.zone_wait(z, after_update25[z.name], equal=False, greater=True)
     slave.zone_verify(z)
 
+for z in zones:
+    check_nsec(master, z, "After re-salt")
+
 t.end()