From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 31 Jul 2025 12:30:20 +0000 (+0200)
Subject: Make keymgr state machine more robust
X-Git-Tag: v9.21.17~60^2~4
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b19871f8a23f83ed004d86690cb8de39794e0931;p=thirdparty%2Fbind9.git

Make keymgr state machine more robust

If the keymgr state machine is in an invalid state, it tries to move
it self to a valid state. But when you do key rollovers during an
invalid state, and the next state is also an invalid state, the keymgr
will happily do the transition.

It would be good to not do key rollovers if there is not a KSK and ZSK
fully omnipresent. But also it would be good to safeguard against
unexpected transitions.

This commit does that by not moving things to unretentive (which is
the state where we would remove the corresponding record from the zone)
if the state machine is currently in an invalid state.
---

diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index f38936aef80..704acca5ec3 100644
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -1254,17 +1254,18 @@ keymgr_policy_approval(dns_dnsseckeylist_t *keyring, dns_dnsseckey_t *key,
 static bool
 keymgr_transition_allowed(dns_dnsseckeylist_t *keyring, dns_dnsseckey_t *key,
 			  int type, dst_key_state_t next_state, uint8_t opts) {
+	bool rule1a, rule1b, rule2a, rule2b, rule3a, rule3b;
+	rule1a = keymgr_have_ds(keyring, key, type, NA, opts);
+	rule1b = keymgr_have_ds(keyring, key, type, next_state, opts);
+	rule2a = keymgr_have_dnskey(keyring, key, type, NA);
+	rule2b = keymgr_have_dnskey(keyring, key, type, next_state);
+	rule3a = keymgr_have_rrsig(keyring, key, type, NA);
+	rule3b = keymgr_have_rrsig(keyring, key, type, next_state);
+
 	/* Debug logging. */
 	if (isc_log_wouldlog(ISC_LOG_DEBUG(1))) {
-		bool rule1a, rule1b, rule2a, rule2b, rule3a, rule3b;
 		char keystr[DST_KEY_FORMATSIZE];
 		dst_key_format(key->key, keystr, sizeof(keystr));
-		rule1a = keymgr_have_ds(keyring, key, type, NA, opts);
-		rule1b = keymgr_have_ds(keyring, key, type, next_state, opts);
-		rule2a = keymgr_have_dnskey(keyring, key, type, NA);
-		rule2b = keymgr_have_dnskey(keyring, key, type, next_state);
-		rule3a = keymgr_have_rrsig(keyring, key, type, NA);
-		rule3b = keymgr_have_rrsig(keyring, key, type, next_state);
 		isc_log_write(
 			DNS_LOGCATEGORY_DNSSEC, DNS_LOGMODULE_DNSSEC,
 			ISC_LOG_DEBUG(1),
@@ -1277,29 +1278,40 @@ keymgr_transition_allowed(dns_dnsseckeylist_t *keyring, dns_dnsseckey_t *key,
 			rule3a ? "true" : "false", rule3b ? "true" : "false");
 	}
 
-	return
-		/*
-		 * Rule 1: There must be a DS at all times.
-		 * First check the current situation: if the rule check fails,
-		 * we allow the transition to attempt to move us out of the
-		 * invalid state.  If the rule check passes, also check if
-		 * the next state is also still a valid situation.
-		 */
-		(!keymgr_have_ds(keyring, key, type, NA, opts) ||
-		 keymgr_have_ds(keyring, key, type, next_state, opts)) &&
-		/*
-		 * Rule 2: There must be a DNSKEY at all times.  Again, first
-		 * check the current situation, then assess the next state.
-		 */
-		(!keymgr_have_dnskey(keyring, key, type, NA) ||
-		 keymgr_have_dnskey(keyring, key, type, next_state)) &&
-		/*
-		 * Rule 3: There must be RRSIG records at all times. Again,
-		 * first check the current situation, then assess the next
-		 * state.
-		 */
-		(!keymgr_have_rrsig(keyring, key, type, NA) ||
-		 keymgr_have_rrsig(keyring, key, type, next_state));
+	/*
+	 * Rule checking:
+	 * First check the current situation: if the rule check fails,
+	 * we allow the transition to attempt to move us out of the
+	 * invalid state.  If the rule check passes, also check if
+	 * the next state is also still a valid situation.
+	 */
+	char keystr2[DST_KEY_FORMATSIZE];
+	dst_key_format(key->key, keystr2, sizeof(keystr2));
+
+	/*
+	 * Rule 1: There must be a DS at all times.
+	 */
+	if (!rule1a && !rule1b && next_state == UNRETENTIVE) {
+		return false;
+	}
+	/*
+	 * Rule 2: There must be a DNSKEY at all times.  Again, first
+	 * check the current situation, then assess the next state.
+	 */
+	if (!rule2a && !rule2b && next_state == UNRETENTIVE) {
+		return false;
+	}
+	/*
+	 * Rule 3: There must be RRSIG records at all times. Again,
+	 * first check the current situation, then assess the next
+	 * state.
+	 */
+	if (!rule3a && !rule3b && next_state == UNRETENTIVE) {
+		return false;
+	}
+
+	return (!rule1a || rule1b) && (!rule2a || rule2b) &&
+	       (!rule3a || rule3b);
 }
 
 /*