From: Matthijs Mekking <matthijs@isc.org>
Date: Tue, 11 Jul 2023 12:01:36 +0000 (+0200)
Subject: Refactor KSK processing
X-Git-Tag: v9.19.16~32^2~5
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a8f71f67ac0d223bcc32dff2f8cfa48fe4c47621;p=thirdparty%2Fbind9.git

Refactor KSK processing

There are multiple almost identical code blocks, time to make a
function.
---

diff --git a/lib/dns/include/dst/dst.h b/lib/dns/include/dst/dst.h
index 3cca998bcb6..c0912f3e67a 100644
--- a/lib/dns/include/dst/dst.h
+++ b/lib/dns/include/dst/dst.h
@@ -774,6 +774,25 @@ dst_key_iszonekey(const dst_key_t *key);
 bool
 dst_key_isnullkey(const dst_key_t *key);
 
+bool
+dst_key_have_ksk_and_zsk(dst_key_t **keys, unsigned int nkeys, unsigned int i,
+			 bool check_offline, bool ksk, bool zsk, bool *have_ksk,
+			 bool *have_zsk);
+/*%<
+ *
+ * Check the list of 'keys' to see if both a KSK and ZSK are present, given key
+ * 'i'. The values stored in 'ksk' and 'zsk' tell whether key 'i' is a KSK, ZSK,
+ * or both (CSK). If 'check_offline' is true, don't consider KSKs that are
+ * currently offline (e.g. their private key file is not available).
+ *
+ * Requires:
+ *\li	"keys" is not NULL.
+ *
+ * Returns:
+ *\li	true if there is one or more keys such that both the KSK and ZSK roles
+ *are covered, false otherwise.
+ */
+
 isc_result_t
 dst_key_buildfilename(const dst_key_t *key, int type, const char *directory,
 		      isc_buffer_t *out);
diff --git a/lib/dns/key.c b/lib/dns/key.c
index 7d4f378e695..d039ea44449 100644
--- a/lib/dns/key.c
+++ b/lib/dns/key.c
@@ -159,6 +159,68 @@ dst_key_isnullkey(const dst_key_t *key) {
 	return (true);
 }
 
+#define REVOKE(x) ((dst_key_flags(x) & DNS_KEYFLAG_REVOKE) != 0)
+#define KSK(x)	  ((dst_key_flags(x) & DNS_KEYFLAG_KSK) != 0)
+#define ID(x)	  dst_key_id(x)
+#define ALG(x)	  dst_key_alg(x)
+
+bool
+dst_key_have_ksk_and_zsk(dst_key_t **keys, unsigned int nkeys, unsigned int i,
+			 bool check_offline, bool ksk, bool zsk, bool *have_ksk,
+			 bool *have_zsk) {
+	bool hksk = ksk;
+	bool hzsk = zsk;
+	isc_result_t result;
+
+	REQUIRE(keys != NULL);
+
+	for (unsigned int j = 0; j < nkeys && !(hksk && hzsk); j++) {
+		if (j == i || ALG(keys[i]) != ALG(keys[j])) {
+			continue;
+		}
+		/*
+		 * Don't consider inactive keys.
+		 */
+		if (dst_key_inactive(keys[j])) {
+			continue;
+		}
+		/*
+		 * Don't consider offline keys.
+		 */
+		if (check_offline && !dst_key_isprivate(keys[j])) {
+			continue;
+		}
+		if (REVOKE(keys[j])) {
+			continue;
+		}
+
+		if (!hksk) {
+			result = dst_key_getbool(keys[j], DST_BOOL_KSK, &hksk);
+			if (result != ISC_R_SUCCESS) {
+				if (KSK(keys[j])) {
+					hksk = true;
+				}
+			}
+		}
+		if (!hzsk) {
+			result = dst_key_getbool(keys[j], DST_BOOL_ZSK, &hzsk);
+			if (result != ISC_R_SUCCESS) {
+				if (!KSK(keys[j])) {
+					hzsk = dst_key_isprivate(keys[j]);
+				}
+			}
+		}
+	}
+
+	if (have_ksk != NULL) {
+		*have_ksk = hksk;
+	}
+	if (have_zsk != NULL) {
+		*have_zsk = hzsk;
+	}
+	return (hksk && hzsk);
+}
+
 void
 dst_key_setbits(dst_key_t *key, uint16_t bits) {
 	unsigned int maxbits;
diff --git a/lib/dns/update.c b/lib/dns/update.c
index 710e98f28e3..17ebcb8e2d3 100644
--- a/lib/dns/update.c
+++ b/lib/dns/update.c
@@ -1118,7 +1118,7 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
 	dns_stats_t *dnssecsignstats = dns_zone_getdnssecsignstats(zone);
 	isc_buffer_t buffer;
 	unsigned char data[1024]; /* XXX */
-	unsigned int i, j;
+	unsigned int i;
 	bool added_sig = false;
 	bool use_kasp = false;
 	isc_mem_t *mctx = diff->mctx;
@@ -1164,41 +1164,14 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
 		}
 
 		if (check_ksk && !REVOKE(keys[i])) {
-			bool have_ksk, have_nonksk;
-			if (KSK(keys[i])) {
-				have_ksk = true;
-				have_nonksk = false;
-			} else {
-				have_ksk = false;
-				have_nonksk = true;
-			}
-			for (j = 0; j < nkeys; j++) {
-				if (j == i || ALG(keys[i]) != ALG(keys[j])) {
-					continue;
-				}
-
-				/* Don't consider inactive keys, however
-				 * the KSK may be temporary offline, so do
-				 * consider KSKs which private key files are
-				 * unavailable.
-				 */
-				if (dst_key_inactive(keys[j])) {
-					continue;
-				}
-
-				if (REVOKE(keys[j])) {
-					continue;
-				}
-				if (KSK(keys[j])) {
-					have_ksk = true;
-				} else if (dst_key_isprivate(keys[j])) {
-					have_nonksk = true;
-				}
-				both = have_ksk && have_nonksk;
-				if (both) {
-					break;
-				}
-			}
+			/*
+			 * Don't consider inactive keys, however the KSK may be
+			 * temporary offline, so do consider KSKs which private
+			 * key files are unavailable.
+			 */
+			both = dst_key_have_ksk_and_zsk(
+				keys, nkeys, i, false, KSK(keys[i]),
+				!KSK(keys[i]), NULL, NULL);
 		}
 
 		if (use_kasp) {
diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index ca72af636c9..945346967b0 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -6554,7 +6554,7 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 	dns_rdata_t sig_rdata = DNS_RDATA_INIT;
 	unsigned char data[1024]; /* XXX */
 	isc_buffer_t buffer;
-	unsigned int i, j;
+	unsigned int i;
 	bool use_kasp = false;
 
 	if (zone->kasp != NULL) {
@@ -6590,8 +6590,6 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 	}
 
 	for (i = 0; i < nkeys; i++) {
-		bool both = false;
-
 		/* Don't add signatures for offline or inactive keys */
 		if (!dst_key_isprivate(keys[i])) {
 			continue;
@@ -6601,43 +6599,14 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 		}
 
 		if (check_ksk && !REVOKE(keys[i])) {
-			bool have_ksk, have_nonksk;
-			if (KSK(keys[i])) {
-				have_ksk = true;
-				have_nonksk = false;
-			} else {
-				have_ksk = false;
-				have_nonksk = true;
-			}
-
-			for (j = 0; j < nkeys; j++) {
-				if (j == i || ALG(keys[i]) != ALG(keys[j])) {
-					continue;
-				}
-
-				/*
-				 * Don't consider inactive keys, however
-				 * the KSK may be temporary offline, so do
-				 * consider keys which private key files are
-				 * unavailable.
-				 */
-				if (dst_key_inactive(keys[j])) {
-					continue;
-				}
-
-				if (REVOKE(keys[j])) {
-					continue;
-				}
-				if (KSK(keys[j])) {
-					have_ksk = true;
-				} else if (dst_key_isprivate(keys[j])) {
-					have_nonksk = true;
-				}
-				both = have_ksk && have_nonksk;
-				if (both) {
-					break;
-				}
-			}
+			/*
+			 * Don't consider inactive keys, however the KSK may be
+			 * temporary offline, so do consider keys which private
+			 * key files are unavailable.
+			 */
+			both = dst_key_have_ksk_and_zsk(
+				keys, nkeys, i, false, KSK(keys[i]),
+				!KSK(keys[i]), NULL, NULL);
 		}
 		if (use_kasp) {
 			/*
@@ -6648,7 +6617,6 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 			isc_stdtime_t when;
 			bool ksk = false;
 			bool zsk = false;
-			bool have_ksk = false;
 			bool have_zsk = false;
 
 			kresult = dst_key_getbool(keys[i], DST_BOOL_KSK, &ksk);
@@ -6664,55 +6632,12 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 				}
 			}
 
-			have_ksk = ksk;
-			have_zsk = zsk;
-			both = have_ksk && have_zsk;
-
-			for (j = 0; j < nkeys; j++) {
-				if (both) {
-					break;
-				}
-
-				if (j == i || ALG(keys[i]) != ALG(keys[j])) {
-					continue;
-				}
-
-				/*
-				 * Don't consider inactive keys or offline keys.
-				 */
-				if (!dst_key_isprivate(keys[j])) {
-					continue;
-				}
-				if (dst_key_inactive(keys[j])) {
-					continue;
-				}
-
-				if (REVOKE(keys[j])) {
-					continue;
-				}
-
-				if (!have_ksk) {
-					kresult = dst_key_getbool(keys[j],
-								  DST_BOOL_KSK,
-								  &have_ksk);
-					if (kresult != ISC_R_SUCCESS) {
-						if (KSK(keys[j])) {
-							have_ksk = true;
-						}
-					}
-				}
-				if (!have_zsk) {
-					kresult = dst_key_getbool(keys[j],
-								  DST_BOOL_ZSK,
-								  &have_zsk);
-					if (kresult != ISC_R_SUCCESS) {
-						if (!KSK(keys[j])) {
-							have_zsk = true;
-						}
-					}
-				}
-				both = have_ksk && have_zsk;
-			}
+			/*
+			 * Don't consider inactive keys or offline keys.
+			 */
+			(void)dst_key_have_ksk_and_zsk(keys, nkeys, i, true,
+						       ksk, zsk, NULL,
+						       &have_zsk);
 
 			if (dns_rdatatype_iskeymaterial(type)) {
 				/*
@@ -9375,44 +9300,15 @@ zone_sign(dns_zone_t *zone) {
 			 * Do we do KSK processing?
 			 */
 			if (check_ksk && !REVOKE(zone_keys[i])) {
-				bool have_ksk, have_nonksk;
-				if (KSK(zone_keys[i])) {
-					have_ksk = true;
-					have_nonksk = false;
-				} else {
-					have_ksk = false;
-					have_nonksk = true;
-				}
-				for (j = 0; j < nkeys; j++) {
-					if (j == i || (ALG(zone_keys[i]) !=
-						       ALG(zone_keys[j])))
-					{
-						continue;
-					}
-					/*
-					 * Don't consider inactive keys, however
-					 * the key may be temporary offline, so
-					 * do consider KSKs which private key
-					 * files are unavailable.
-					 */
-					if (dst_key_inactive(zone_keys[j])) {
-						continue;
-					}
-					if (REVOKE(zone_keys[j])) {
-						continue;
-					}
-					if (KSK(zone_keys[j])) {
-						have_ksk = true;
-					} else if (dst_key_isprivate(
-							   zone_keys[j]))
-					{
-						have_nonksk = true;
-					}
-					both = have_ksk && have_nonksk;
-					if (both) {
-						break;
-					}
-				}
+				/*
+				 * Don't consider inactive keys, however the key
+				 * may be temporary offline, so do consider KSKs
+				 * which private key files are unavailable.
+				 */
+				both = dst_key_have_ksk_and_zsk(
+					zone_keys, nkeys, i, false,
+					KSK(zone_keys[i]), !KSK(zone_keys[i]),
+					NULL, NULL);
 			}
 			if (use_kasp) {
 				/*