From: Aram Sargsyan <aram@isc.org>
Date: Thu, 2 Oct 2025 12:52:12 +0000 (+0000)
Subject: Fix dnssec-keygen key collision checking for KEY rrtype keys
X-Git-Tag: v9.21.15~50^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=49b7ce9a54790aa3adcf2f7028d8e62576bd527a;p=thirdparty%2Fbind9.git

Fix dnssec-keygen key collision checking for KEY rrtype keys

When generating a new key, dnssec-keygen checks for possible
key ID collisions with existing keys. The dnssec.c:findmatchingkeys()
function, which is supposed to get the list of the existing keys,
fails to do that for the existing KEY rrtype keys (i.e. generated
using 'dnssec-keygen -T KEY') because it doesn't pass down to the
dst_key_fromnamedfile() -> dst_key_read_public() functions the type
of the keys it's interested in. Fix the issue by introducing a new
function parameter which tells in which type of keys the caller is
currently interested in.
---

diff --git a/bin/dnssec/dnssec-ksr.c b/bin/dnssec/dnssec-ksr.c
index bea88940452..86eb1362fc3 100644
--- a/bin/dnssec/dnssec-ksr.c
+++ b/bin/dnssec/dnssec-ksr.c
@@ -217,7 +217,8 @@ get_dnskeys(ksr_ctx_t *ksr, dns_dnsseckeylist_t *keys) {
 	ISC_LIST_INIT(*keys);
 	ISC_LIST_INIT(keys_read);
 	ret = dns_dnssec_findmatchingkeys(name, NULL, ksr->keydir, NULL,
-					  ksr->now, isc_g_mctx, &keys_read);
+					  ksr->now, false, isc_g_mctx,
+					  &keys_read);
 	if (ret != ISC_R_SUCCESS && ret != ISC_R_NOTFOUND) {
 		fatal("failed to load existing keys from %s: %s", ksr->keydir,
 		      isc_result_totext(ret));
diff --git a/bin/dnssec/dnssec-signzone.c b/bin/dnssec/dnssec-signzone.c
index 61bb17be4e1..d7fcc1ceafa 100644
--- a/bin/dnssec/dnssec-signzone.c
+++ b/bin/dnssec/dnssec-signzone.c
@@ -2738,7 +2738,8 @@ findkeys:
 	 * Find keys that match this zone in the key repository.
 	 */
 	result = dns_dnssec_findmatchingkeys(gorigin, NULL, directory, NULL,
-					     now, isc_g_mctx, &matchkeys);
+					     now, false, isc_g_mctx,
+					     &matchkeys);
 	if (result == ISC_R_NOTFOUND) {
 		result = ISC_R_SUCCESS;
 	}
diff --git a/bin/dnssec/dnssectool.c b/bin/dnssec/dnssectool.c
index a0eb9f87025..150411ee4ce 100644
--- a/bin/dnssec/dnssectool.c
+++ b/bin/dnssec/dnssectool.c
@@ -486,10 +486,17 @@ key_collision(dst_key_t *dstkey, dns_name_t *name, const char *dir,
 	}
 
 	ISC_LIST_INIT(matchkeys);
-	result = dns_dnssec_findmatchingkeys(name, NULL, dir, NULL, now, mctx,
-					     &matchkeys);
+	bool keykey = false;
+
+	/*
+	 * DNSKEY and KEY both use the same file names patterns so
+	 * we have to look for both sets of keys.
+	 */
+again:
+	result = dns_dnssec_findmatchingkeys(name, NULL, dir, NULL, now, keykey,
+					     mctx, &matchkeys);
 	if (result == ISC_R_NOTFOUND) {
-		return false;
+		goto try_key;
 	}
 
 	while (!ISC_LIST_EMPTY(matchkeys) && !conflict) {
@@ -531,6 +538,11 @@ key_collision(dst_key_t *dstkey, dns_name_t *name, const char *dir,
 		dns_dnsseckey_destroy(mctx, &key);
 	}
 
+try_key:
+	if (!conflict && !keykey) {
+		keykey = true;
+		goto again;
+	}
 	return conflict;
 }
 
diff --git a/bin/dnssec/dnssectool.h b/bin/dnssec/dnssectool.h
index 0702034c7e7..0b981b6aad2 100644
--- a/bin/dnssec/dnssectool.h
+++ b/bin/dnssec/dnssectool.h
@@ -102,7 +102,7 @@ void
 set_keyversion(dst_key_t *key);
 
 bool
-key_collision(dst_key_t *key, dns_name_t *name, const char *dir,
+key_collision(dst_key_t *dstkey, dns_name_t *name, const char *dir,
 	      isc_mem_t *mctx, uint16_t min, uint16_t max, bool *exact);
 
 bool
diff --git a/lib/dns/dnssec.c b/lib/dns/dnssec.c
index dffddd9a0b1..7eed87ce168 100644
--- a/lib/dns/dnssec.c
+++ b/lib/dns/dnssec.c
@@ -1221,12 +1221,12 @@ dns_dnssec_get_hints(dns_dnsseckey_t *key, isc_stdtime_t now) {
 }
 
 static isc_result_t
-findmatchingkeys(const char *directory, char *namebuf, unsigned int len,
-		 isc_mem_t *mctx, isc_stdtime_t now,
+findmatchingkeys(const char *directory, bool rrtypekey, char *namebuf,
+		 unsigned int len, isc_mem_t *mctx, isc_stdtime_t now,
 		 dns_dnsseckeylist_t *list) {
-	isc_result_t result = ISC_R_SUCCESS;
+	isc_result_t result;
 	isc_dir_t dir;
-	bool dir_open = false;
+	bool dir_open = false, match = false;
 	unsigned int i, alg;
 	dns_dnsseckey_t *key = NULL;
 	dst_key_t *dstkey = NULL;
@@ -1235,6 +1235,7 @@ findmatchingkeys(const char *directory, char *namebuf, unsigned int len,
 	if (directory == NULL) {
 		directory = ".";
 	}
+
 	RETERR(isc_dir_open(&dir, directory));
 	dir_open = true;
 
@@ -1283,11 +1284,13 @@ findmatchingkeys(const char *directory, char *namebuf, unsigned int len,
 			continue;
 		}
 
+		int type = DST_TYPE_PUBLIC | DST_TYPE_PRIVATE | DST_TYPE_STATE;
+		if (rrtypekey) {
+			type |= DST_TYPE_KEY;
+		}
 		dstkey = NULL;
-		result = dst_key_fromnamedfile(
-			dir.entry.name, directory,
-			DST_TYPE_PUBLIC | DST_TYPE_PRIVATE | DST_TYPE_STATE,
-			mctx, &dstkey);
+		result = dst_key_fromnamedfile(dir.entry.name, directory, type,
+					       mctx, &dstkey);
 
 		switch (alg) {
 		case DST_ALG_HMACMD5:
@@ -1319,9 +1322,11 @@ findmatchingkeys(const char *directory, char *namebuf, unsigned int len,
 			dns_dnsseckey_destroy(mctx, &key);
 		} else {
 			ISC_LIST_APPEND(*list, key, link);
+			match = true;
 			key = NULL;
 		}
 	}
+	result = match ? ISC_R_SUCCESS : ISC_R_NOTFOUND;
 
 failure:
 	if (dir_open) {
@@ -1334,12 +1339,13 @@ failure:
 }
 
 /*%
- * Get a list of DNSSEC keys from the key repository.
+ * Get a list of KEY or DNSKEY keys from the key repository. If rrtypekey
+ * is true KEY keys will be returned otherwise DNSSEC keys.
  */
 isc_result_t
 dns_dnssec_findmatchingkeys(const dns_name_t *origin, dns_kasp_t *kasp,
 			    const char *keydir, dns_keystorelist_t *keystores,
-			    isc_stdtime_t now, isc_mem_t *mctx,
+			    isc_stdtime_t now, bool rrtypekey, isc_mem_t *mctx,
 			    dns_dnsseckeylist_t *keylist) {
 	isc_result_t result = ISC_R_SUCCESS;
 	dns_dnsseckeylist_t list;
@@ -1358,8 +1364,8 @@ dns_dnssec_findmatchingkeys(const dns_name_t *origin, dns_kasp_t *kasp,
 	if (kasp == NULL || (strcmp(dns_kasp_getname(kasp), "none") == 0) ||
 	    (strcmp(dns_kasp_getname(kasp), "insecure") == 0))
 	{
-		RETERR(findmatchingkeys(keydir, namebuf, len, mctx, now,
-					&list));
+		RETERR(findmatchingkeys(keydir, rrtypekey, namebuf, len, mctx,
+					now, &list));
 	} else if (keystores != NULL) {
 		ISC_LIST_FOREACH(*keystores, keystore, link) {
 			ISC_LIST_FOREACH(dns_kasp_keys(kasp), kkey, link) {
@@ -1368,8 +1374,8 @@ dns_dnssec_findmatchingkeys(const dns_name_t *origin, dns_kasp_t *kasp,
 						dns_keystore_directory(keystore,
 								       keydir);
 					RETERR(findmatchingkeys(
-						directory, namebuf, len, mctx,
-						now, &list));
+						directory, rrtypekey, namebuf,
+						len, mctx, now, &list));
 					break;
 				}
 			}
diff --git a/lib/dns/include/dns/dnssec.h b/lib/dns/include/dns/dnssec.h
index 38883b6343d..eed02b00da5 100644
--- a/lib/dns/include/dns/dnssec.h
+++ b/lib/dns/include/dns/dnssec.h
@@ -295,13 +295,16 @@ dns_dnssec_get_hints(dns_dnsseckey_t *key, isc_stdtime_t now);
 isc_result_t
 dns_dnssec_findmatchingkeys(const dns_name_t *origin, dns_kasp_t *kasp,
 			    const char *keydir, dns_keystorelist_t *keystores,
-			    isc_stdtime_t now, isc_mem_t *mctx,
+			    isc_stdtime_t now, bool rrtypekey, isc_mem_t *mctx,
 			    dns_dnsseckeylist_t *keylist);
 /*%<
  * Search for K* key files matching the name in 'origin'. If 'kasp' is not
  * NULL, search in the directories used in 'keystores'. Otherwise search in the
  * key-directory 'keydir'.
  *
+ * If 'rrtypekey' is true, then KEY type keys are matched (e.g. for SIG(0)),
+ * otherwise DNSKEY type keys are matched for DNSSEC.
+ *
  * Append all such keys, along with use hints gleaned from their
  * metadata, onto 'keylist'.  Skip any unsupported algorithms.
  *
diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index 63177b68d55..892fcaafda7 100644
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -496,7 +496,8 @@ static isc_result_t
 keymgr_createkey(dns_kasp_key_t *kkey, const dns_name_t *origin,
 		 dns_kasp_t *kasp, dns_rdataclass_t rdclass, isc_mem_t *mctx,
 		 const char *keydir, dns_dnsseckeylist_t *keylist,
-		 dns_dnsseckeylist_t *newkeys, dst_key_t **dst_key) {
+		 isc_stdtime_t now, dns_dnsseckeylist_t *newkeys,
+		 dst_key_t **dst_key) {
 	isc_result_t result = ISC_R_SUCCESS;
 	bool conflict = false;
 	int flags = DNS_KEYOWNER_ZONE;
@@ -505,11 +506,23 @@ keymgr_createkey(dns_kasp_key_t *kkey, const dns_name_t *origin,
 	dns_keystore_t *keystore = dns_kasp_key_keystore(kkey);
 	const char *dir = NULL;
 	int size = dns_kasp_key_size(kkey);
+	dns_dnsseckeylist_t keykeys;
+
+	ISC_LIST_INIT(keykeys);
 
 	if (dns_kasp_key_ksk(kkey)) {
 		flags |= DNS_KEYFLAG_KSK;
 	}
 
+	/*
+	 * We also need to check against K* files for KEYs.
+	 */
+	result = dns_dnssec_findmatchingkeys(origin, NULL, keydir, NULL, now,
+					     true, mctx, &keykeys);
+	if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND) {
+		goto failure;
+	}
+
 	do {
 		if (keystore == NULL) {
 			RETERR(dst_key_generate(origin, alg, size, 0, flags,
@@ -524,6 +537,10 @@ keymgr_createkey(dns_kasp_key_t *kkey, const dns_name_t *origin,
 		/* Key collision? */
 		conflict = keymgr_keyid_conflict(newkey, kkey->tag_min,
 						 kkey->tag_max, keylist);
+		if (!conflict) {
+			conflict = keymgr_keyid_conflict(
+				newkey, kkey->tag_min, kkey->tag_max, &keykeys);
+		}
 		if (!conflict) {
 			conflict = keymgr_keyid_conflict(
 				newkey, kkey->tag_min, kkey->tag_max, newkeys);
@@ -548,9 +565,14 @@ keymgr_createkey(dns_kasp_key_t *kkey, const dns_name_t *origin,
 		dst_key_setdirectory(newkey, dir);
 	}
 	*dst_key = newkey;
-	return ISC_R_SUCCESS;
+	result = ISC_R_SUCCESS;
 
 failure:
+	while (!ISC_LIST_EMPTY(keykeys)) {
+		dns_dnsseckey_t *key = ISC_LIST_HEAD(keykeys);
+		ISC_LIST_UNLINK(keykeys, key, link);
+		dns_dnsseckey_destroy(mctx, &key);
+	}
 	return result;
 }
 
@@ -1920,9 +1942,9 @@ keymgr_key_rollover(dns_kasp_key_t *kaspkey, dns_dnsseckey_t *active_key,
 		bool csk = (dns_kasp_key_ksk(kaspkey) &&
 			    dns_kasp_key_zsk(kaspkey));
 
-		isc_result_t result =
-			keymgr_createkey(kaspkey, origin, kasp, rdclass, mctx,
-					 keydir, keyring, newkeys, &dst_key);
+		isc_result_t result = keymgr_createkey(
+			kaspkey, origin, kasp, rdclass, mctx, keydir, keyring,
+			now, newkeys, &dst_key);
 		if (result != ISC_R_SUCCESS) {
 			return result;
 		}
diff --git a/lib/dns/update.c b/lib/dns/update.c
index f228a96ceac..ff4ae03c61e 100644
--- a/lib/dns/update.c
+++ b/lib/dns/update.c
@@ -1035,8 +1035,8 @@ find_zone_keys(dns_zone_t *zone, isc_mem_t *mctx, unsigned int maxkeys,
 
 	dns_zone_lock_keyfiles(zone);
 	result = dns_dnssec_findmatchingkeys(dns_zone_getorigin(zone), kasp,
-					     keydir, keystores, now, mctx,
-					     &keylist);
+					     keydir, keystores, now, false,
+					     mctx, &keylist);
 	dns_zone_unlock_keyfiles(zone);
 
 	if (result != ISC_R_SUCCESS) {
diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index ff9d9a4ada9..b14b7fffb48 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -6996,9 +6996,9 @@ dns_zone_getdnsseckeys(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver,
 
 	/* Get keys from private key files. */
 	dns_zone_lock_keyfiles(zone);
-	result = dns_dnssec_findmatchingkeys(origin, kasp, dir,
-					     dns_zone_getkeystores(zone), now,
-					     dns_zone_getmctx(zone), keys);
+	result = dns_dnssec_findmatchingkeys(
+		origin, kasp, dir, dns_zone_getkeystores(zone), now, false,
+		dns_zone_getmctx(zone), keys);
 	dns_zone_unlock_keyfiles(zone);
 
 	if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND) {
@@ -16819,8 +16819,8 @@ dns_zone_dnskey_inuse(dns_zone_t *zone, dns_rdata_t *rdata, bool *inuse) {
 
 	dns_zone_lock_keyfiles(zone);
 	result = dns_dnssec_findmatchingkeys(dns_zone_getorigin(zone), kasp,
-					     keydir, keystores, now, mctx,
-					     &keylist);
+					     keydir, keystores, now, false,
+					     mctx, &keylist);
 	dns_zone_unlock_keyfiles(zone);
 	if (result == ISC_R_NOTFOUND) {
 		return ISC_R_SUCCESS;
@@ -22512,7 +22512,7 @@ zone_rekey(dns_zone_t *zone) {
 	dns_zone_lock_keyfiles(zone);
 	result = dns_dnssec_findmatchingkeys(&zone->origin, kasp, dir,
 					     dns_zone_getkeystores(zone), now,
-					     mctx, &keys);
+					     false, mctx, &keys);
 	dns_zone_unlock_keyfiles(zone);
 
 	if (result != ISC_R_SUCCESS) {
diff --git a/lib/isccfg/check.c b/lib/isccfg/check.c
index fe607b20a7d..0e044a3b087 100644
--- a/lib/isccfg/check.c
+++ b/lib/isccfg/check.c
@@ -2962,7 +2962,7 @@ check_keydir(const cfg_obj_t *config, const cfg_obj_t *zconfig,
 
 		ISC_LIST_INIT(keys);
 		ret = dns_dnssec_findmatchingkeys(origin, kasp, keydir, &kslist,
-						  now, mctx, &keys);
+						  now, false, mctx, &keys);
 		if (ret != ISC_R_SUCCESS) {
 			result = ret;
 		}
diff --git a/tests/dns/skr_test.c b/tests/dns/skr_test.c
index 3de994ab791..ec41f3b3dd3 100644
--- a/tests/dns/skr_test.c
+++ b/tests/dns/skr_test.c
@@ -448,7 +448,7 @@ ISC_RUN_TEST_IMPL(skr_read) {
 	ISC_LIST_INIT(keys);
 	result = dns_dnssec_findmatchingkeys(dname, NULL,
 					     TESTS_DIR "/testdata/skr/", NULL,
-					     0, isc_g_mctx, &keys);
+					     0, false, isc_g_mctx, &keys);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	/* Create/read the SKR file */