Fix bug in keymgr_key_has_successor

The logic in `keymgr_key_has_successor(key, keyring)` is flawed, it
returns true if there is any key in the keyring that has a successor,
while what we really want here is to make sure that the given key
has a successor in the given keyring.

Rather than relying on `keymgr_key_exists_with_state`, walk the
list of keys in the keyring and check if the key is a successor of
the given predecessor key.

diff --git a/CHANGES b/CHANGES
index 9583c7030adab61e75e645a71a9f79f5fc3819a8..a98a447cf03bd6cc39cb6a6656c5ddc5140b36e9 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,7 @@
+5423.  [bug]           Fix a bug in keymgr_key_has_successor: it would
+                       return a false positive if any other key in the
+                       keyring has a successor. [GL #1845]
+
 5422.  [bug]           When using dnssec-policy, print correct keytiming
                        metadata. [GL #1843]
 

diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh
index b0f2d727e23638b6656cafa6ae406abf31215450..6547053d059cb601ad0f81c0b2ccdc1421472505 100644 (file)
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -2533,6 +2533,7 @@ set_keyalgorithm "KEY3" "13" "ECDSAP256SHA256" "256"
 set_keysigning   "KEY3" "no"
 set_zonesigning  "KEY3" "no"
 # Key states.
+set_keystate "KEY2" "GOAL"         "hidden"
 set_keystate "KEY3" "GOAL"         "omnipresent"
 set_keystate "KEY3" "STATE_DNSKEY" "rumoured"
 set_keystate "KEY3" "STATE_ZRRSIG" "hidden"
@@ -2570,7 +2571,6 @@ set_server "ns3" "10.53.0.3"
 # ZSK (KEY2) no longer is actively signing, RRSIG state in UNRETENTIVE.
 # New ZSK (KEY3) is now actively signing, RRSIG state in RUMOURED.
 set_zonesigning  "KEY2" "no"
-set_keystate     "KEY2" "GOAL" "hidden"
 set_keystate     "KEY2" "STATE_ZRRSIG" "unretentive"
 set_zonesigning  "KEY3" "yes"
 set_keystate     "KEY3" "STATE_DNSKEY" "omnipresent"
@@ -2749,6 +2749,7 @@ set_keyalgorithm "KEY3" "13" "ECDSAP256SHA256" "256"
 set_keysigning   "KEY3" "yes"
 set_zonesigning  "KEY3" "no"
 # Key states.
+set_keystate "KEY1" "GOAL"         "hidden"
 set_keystate "KEY3" "GOAL"         "omnipresent"
 set_keystate "KEY3" "STATE_DNSKEY" "rumoured"
 set_keystate "KEY3" "STATE_KRRSIG" "rumoured"
@@ -2792,7 +2793,6 @@ set_zone "step3.ksk-doubleksk.autosign"
 set_policy "ksk-doubleksk" "3" "7200"
 set_server "ns3" "10.53.0.3"
 # KSK (KEY1) DS will be removed, so it is UNRETENTIVE.
-set_keystate "KEY1" "GOAL"         "hidden"
 set_keystate "KEY1" "STATE_DS"     "unretentive"
 # New KSK (KEY3) has its DS submitted.
 set_keystate "KEY3" "STATE_DNSKEY" "omnipresent"
@@ -2978,6 +2978,7 @@ set_keyalgorithm "KEY2" "13" "ECDSAP256SHA256" "256"
 set_keysigning   "KEY2" "yes"
 set_zonesigning  "KEY2" "no"
 # Key states.
+set_keystate "KEY1" "GOAL"         "hidden"
 set_keystate "KEY2" "GOAL"         "omnipresent"
 set_keystate "KEY2" "STATE_DNSKEY" "rumoured"
 set_keystate "KEY2" "STATE_KRRSIG" "rumoured"
@@ -3019,7 +3020,6 @@ set_server "ns3" "10.53.0.3"
 set_zonesigning  "KEY1" "no"
 set_zonesigning  "KEY2" "yes"
 # CSK (KEY1) DS and ZRRSIG will be removed, so it is UNRETENTIVE.
-set_keystate "KEY1" "GOAL"         "hidden"
 set_keystate "KEY1" "STATE_ZRRSIG" "unretentive"
 set_keystate "KEY1" "STATE_DS"     "unretentive"
 # New CSK (KEY2) has its DS submitted, and is signing, so the DS and ZRRSIG
@@ -3277,6 +3277,7 @@ set_keyalgorithm "KEY2" "13" "ECDSAP256SHA256" "256"
 set_keysigning   "KEY2" "yes"
 set_zonesigning  "KEY2" "no"
 # Key states.
+set_keystate "KEY1" "GOAL"         "hidden"
 set_keystate "KEY2" "GOAL"         "omnipresent"
 set_keystate "KEY2" "STATE_DNSKEY" "rumoured"
 set_keystate "KEY2" "STATE_KRRSIG" "rumoured"
@@ -3315,7 +3316,6 @@ set_policy "csk-roll2" "2" "3600"
 set_server "ns3" "10.53.0.3"
 # CSK (KEY1) DS and ZRRSIG will be removed, so it is UNRETENTIVE.
 set_zonesigning  "KEY1" "no"
-set_keystate     "KEY1" "GOAL"         "hidden"
 set_keystate     "KEY1" "STATE_ZRRSIG" "unretentive"
 set_keystate     "KEY1" "STATE_DS"     "unretentive"
 # New CSK (KEY2) has its DS submitted, and is signing, so the DS and ZRRSIG

diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst
index 4f71ff1e04fdd89d46735a947cde1906fb8ecb40..c371e2e5cde02a24afd4a1a413d33915eaae37a8 100644 (file)
--- a/doc/notes/notes-current.rst
+++ b/doc/notes/notes-current.rst
@@ -128,3 +128,7 @@ Bug Fixes
 -  ``named`` could crash with an assertion failure if the name of a
    database node was looked up while the database was being modified.
    [GL #1857]
+
+-  Fix a bug in dnssec-policy keymgr where the check if a key has a
+   successor would return a false positive if any other key in the
+   keyring has a successor. [GL #1845]

diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index 9a5480e24c7712e446c5207aafd5a388dfd916ee..7a0831db504f7d62295a1adee7ba1e61d064a600 100644 (file)
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -633,11 +633,16 @@ keymgr_key_exists_with_state(dns_dnsseckeylist_t *keyring, dns_dnsseckey_t *key,
  * Check if a key has a successor.
  */
 static bool
-keymgr_key_has_successor(dns_dnsseckey_t *key, dns_dnsseckeylist_t *keyring) {
-       /* Don't worry about key states. */
-       dst_key_state_t na[4] = { NA, NA, NA, NA };
-       return (keymgr_key_exists_with_state(keyring, key, DST_KEY_DNSKEY, NA,
-                                            na, na, true, true));
+keymgr_key_has_successor(dns_dnsseckey_t *predecessor,
+                        dns_dnsseckeylist_t *keyring) {
+       for (dns_dnsseckey_t *successor = ISC_LIST_HEAD(*keyring);
+            successor != NULL; successor = ISC_LIST_NEXT(successor, link))
+       {
+               if (keymgr_key_is_successor(predecessor->key, successor->key)) {
+                       return (true);
+               }
+       }
+       return (false);
 }
 
 /*