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.

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]
 

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

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]

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) {
+keymgr_key_has_successor(dns_dnsseckey_t *predecessor,
-	/* Don't worry about key states. */
+			 dns_dnsseckeylist_t *keyring) {
-	dst_key_state_t na[4] = { NA, NA, NA, NA };
+	for (dns_dnsseckey_t *successor = ISC_LIST_HEAD(*keyring);
-	return (keymgr_key_exists_with_state(keyring, key, DST_KEY_DNSKEY, NA,
+	     successor != NULL; successor = ISC_LIST_NEXT(successor, link))
-					     na, na, true, true));
+	{
+		if (keymgr_key_is_successor(predecessor->key, successor->key)) {
+			return (true);
+		}
+	}
+	return (false);
 }
 
 /*