Merge branch '2406-kasp-init-inactive-delete-metadata' into 'main'

Resolve "kasp: look at Inactive/Delete when initializing state files" Closes #2406 See merge request isc-projects/bind9!4599
2025-08-30 22:15:20 +00:00 · 2021-02-03 08:49:14 +00:00
parent 6b79a0ce9d 82632fa6d9
commit 62af115fa9
8 changed files with 180 additions and 42 deletions
--- a/4
+++ b/4
@@ -1,3 +1,7 @@
+5575.	[bug]		When migrating to dnssec-policy, BIND considered keys
+			with the "Inactive" and/or "Delete" timing metadata as
+			possible active keys. This has been fixed. [GL #2406]
+
 5574.	[func]		Incoming zone transfers can now use TLS.
 			Addresses in a "primaries" list take an optional
 			"tls" argument, specifying either a previously
--- a/bin/tests/system/kasp/clean.sh
+++ b/bin/tests/system/kasp/clean.sh
@@ -25,7 +25,9 @@ rm -f ns*/managed-keys.bind
 rm -f ns*/*.mkeys
 rm -f ns*/zones ns*/*.db.infile
 rm -f ns*/*.zsk1 ns*/*.zsk2
+rm -f ns3/legacy-keys.*
 rm -f *.created published.test* retired.test*
 rm -f rndc.dnssec.*.out.*
 rm -f python.out.*
 rm -f *-supported.file
+rm -f created.key-* unused.key-*
--- a/bin/tests/system/kasp/ns3/named.conf.in
+++ b/bin/tests/system/kasp/ns3/named.conf.in
@@ -155,7 +155,7 @@ zone "some-keys.kasp" {
 zone "legacy-keys.kasp" {
 	type primary;
 	file "legacy-keys.kasp.db";
-	dnssec-policy "rsasha1";
+	dnssec-policy "migrate-to-dnssec-policy";
 };

 /*
--- a/bin/tests/system/kasp/ns3/policies/kasp.conf.in
+++ b/bin/tests/system/kasp/ns3/policies/kasp.conf.in
@@ -45,6 +45,15 @@ dnssec-policy "rsasha1" {
 	};
 };

+dnssec-policy "migrate-to-dnssec-policy" {
+	dnskey-ttl 1234;
+
+	keys {
+		ksk key-directory lifetime P6M algorithm 5;
+		zsk key-directory lifetime P6M algorithm 5;
+	};
+};
+
 dnssec-policy "rsasha1-nsec3" {
 	dnskey-ttl 1234;

--- a/bin/tests/system/kasp/ns3/setup.sh
+++ b/bin/tests/system/kasp/ns3/setup.sh
@@ -87,9 +87,18 @@ zone="some-keys.kasp"
 $KEYGEN -G -a RSASHA1 -b 2000 -L 1234 $zone > keygen.out.$zone.1 2>&1
 $KEYGEN -G -a RSASHA1 -f KSK  -L 1234 $zone > keygen.out.$zone.2 2>&1

-zone="legacy.kasp"
-$KEYGEN -a RSASHA1 -b 2000 -L 1234 $zone > keygen.out.$zone.1 2>&1
-$KEYGEN -a RSASHA1 -f KSK  -L 1234 $zone > keygen.out.$zone.2 2>&1
+zone="legacy-keys.kasp"
+ZSK=$($KEYGEN -a RSASHA1 -b 2048 -L 1234 $zone 2> keygen.out.$zone.1)
+KSK=$($KEYGEN -a RSASHA1 -f KSK  -L 1234 $zone 2> keygen.out.$zone.2)
+echo $ZSK > legacy-keys.kasp.zsk
+echo $KSK > legacy-keys.kasp.ksk
+# Predecessor keys:
+Tact="now-9mo"
+Tret="now-3mo"
+ZSK=$($KEYGEN -a RSASHA1 -b 2048 -L 1234 $zone 2> keygen.out.$zone.3)
+KSK=$($KEYGEN -a RSASHA1 -f KSK  -L 1234 $zone 2> keygen.out.$zone.4)
+$SETTIME -P $Tact -A $Tact -I $Tret -D $Tret "$ZSK"  > settime.out.$zone.1 2>&1
+$SETTIME -P $Tact -A $Tact -I $Tret -D $Tret "$KSK"  > settime.out.$zone.2 2>&1

 zone="pregenerated.kasp"
 $KEYGEN -G -k rsasha1 -l policies/kasp.conf $zone > keygen.out.$zone.1 2>&1
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -587,7 +587,17 @@ key_unused() {
 	[ -s "$STATE_FILE" ] || ret=1
 	[ "$ret" -eq 0 ] || return

-	# Check timing metadata.
+	# Treat keys that have been removed from the zone as unused.
+	_check_removed=1
+	grep "; Created:" "$KEY_FILE" > created.key-${KEY_ID}.test${n} || _check_removed=0
+	grep "; Delete:" "$KEY_FILE" > unused.key-${KEY_ID}.test${n} || _check_removed=0
+	if [ "$_check_removed" -eq 1 ]; then
+		_created=$(awk '{print $3}' < created.key-${KEY_ID}.test${n})
+		_removed=$(awk '{print $3}' < unused.key-${KEY_ID}.test${n})
+		[ "$_removed" -le "$_created" ] && return
+	fi
+
+	# If no timing metadata is set, this key is unused.
 	grep "; Publish:" "$KEY_FILE" > /dev/null && log_error "unexpected publish comment in $KEY_FILE"
 	grep "; Activate:" "$KEY_FILE" > /dev/null && log_error "unexpected active comment in $KEY_FILE"
 	grep "; Inactive:" "$KEY_FILE" > /dev/null && log_error "unexpected retired comment in $KEY_FILE"
@@ -600,13 +610,11 @@ key_unused() {
 	grep "Revoke:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected revoked in $PRIVATE_FILE"
 	grep "Delete:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected removed in $PRIVATE_FILE"

-	if [ "$_legacy" = "no" ]; then
-		grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE"
-		grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE"
-		grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE"
-		grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE"
-		grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE"
-	fi
+	grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE"
+	grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE"
+	grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE"
+	grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE"
+	grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE"
 }

 # Test: dnssec-verify zone $1.
@@ -1902,22 +1910,6 @@ check_apex
 check_subdomain
 dnssec_verify

-#
-# Zone: legacy-keys.kasp.
-#
-set_zone "legacy-keys.kasp"
-set_policy "rsasha1" "3" "1234"
-set_server "ns3" "10.53.0.3"
-# Key properties, timings and states same as above.
-
-check_keys
-check_dnssecstatus "$SERVER" "$POLICY" "$ZONE"
-set_keytimes_algorithm_policy
-check_keytimes
-check_apex
-check_subdomain
-dnssec_verify
-
 #
 # Zone: pregenerated.kasp.
 #
@@ -2407,6 +2399,102 @@ check_subdomain
 dnssec_verify
 check_rrsig_refresh

+#
+# Zone: legacy-keys.kasp.
+#
+set_zone "legacy-keys.kasp"
+# This zone has two active keys and two old keys left in key directory, so
+# expect 4 key files.
+set_policy "migrate-to-dnssec-policy" "4" "1234"
+set_server "ns3" "10.53.0.3"
+
+# Key properties.
+key_clear        "KEY1"
+set_keyrole      "KEY1" "ksk"
+set_keylifetime  "KEY1" "16070400"
+set_keyalgorithm "KEY1" "5" "RSASHA1" "2048"
+set_keysigning   "KEY1" "yes"
+set_zonesigning  "KEY1" "no"
+
+key_clear        "KEY2"
+set_keyrole      "KEY2" "zsk"
+set_keylifetime  "KEY2" "16070400"
+set_keyalgorithm "KEY2" "5" "RSASHA1" "2048"
+set_keysigning   "KEY2" "no"
+set_zonesigning  "KEY2" "yes"
+# KSK: DNSKEY, RRSIG (ksk) published. DS needs to wait.
+# ZSK: DNSKEY, RRSIG (zsk) published.
+set_keystate "KEY1" "GOAL"         "omnipresent"
+set_keystate "KEY1" "STATE_DNSKEY" "rumoured"
+set_keystate "KEY1" "STATE_KRRSIG" "rumoured"
+set_keystate "KEY1" "STATE_DS"     "hidden"
+
+set_keystate "KEY2" "GOAL"         "omnipresent"
+set_keystate "KEY2" "STATE_DNSKEY" "rumoured"
+set_keystate "KEY2" "STATE_ZRRSIG" "rumoured"
+# Two keys only.
+key_clear "KEY3"
+key_clear "KEY4"
+
+check_keys
+check_dnssecstatus "$SERVER" "$POLICY" "$ZONE"
+
+# Make sure the correct legacy keys were used (and not the removed predecessor
+# keys).
+n=$((n+1))
+echo_i "check correct keys were used when migrating zone ${ZONE} to dnssec-policy ($n)"
+ret=0
+kskfile=$(cat ns3/legacy-keys.kasp.ksk)
+basefile=$(key_get KEY1 BASEFILE)
+echo_i "filename: $basefile (expect $kskfile)"
+test "$DIR/$kskfile" = "$basefile" || ret=1
+zskfile=$(cat ns3/legacy-keys.kasp.zsk)
+basefile=$(key_get KEY2 BASEFILE)
+echo_i "filename: $basefile (expect $zskfile)"
+test "$DIR/$zskfile" = "$basefile" || ret=1
+test "$ret" -eq 0 || echo_i "failed"
+status=$((status+ret))
+
+# KSK times.
+created=$(key_get KEY1 CREATED)
+keyfile=$(key_get KEY1 BASEFILE)
+grep "; Publish:" "${keyfile}.key" > published.test${n}.key1
+published=$(awk '{print $3}' < published.test${n}.key1)
+set_keytime "KEY1" "PUBLISHED" "${published}"
+set_keytime "KEY1" "ACTIVE"    "${published}"
+published=$(key_get KEY1 PUBLISHED)
+# The DS can be published if the DNSKEY and RRSIG records are OMNIPRESENT.
+#  This happens after max-zone-ttl (1d) plus publish-safety (1h) plus
+# zone-propagation-delay (300s) = 86400 + 3600 + 300 = 90300.
+set_addkeytime "KEY1" "SYNCPUBLISH" "${published}" 90300
+# Key lifetime is 6 months, 315360000 seconds.
+set_addkeytime "KEY1" "RETIRED"     "${published}" 16070400
+# The key is removed after the retire time plus DS TTL (1d), parent
+# propagation delay (1h), and retire safety (1h) = 86400 + 3600 + 3600 = 93600.
+retired=$(key_get KEY1 RETIRED)
+set_addkeytime "KEY1" "REMOVED"     "${retired}"   93600
+
+# ZSK times.
+created=$(key_get KEY2 CREATED)
+keyfile=$(key_get KEY2 BASEFILE)
+grep "; Publish:" "${keyfile}.key" > published.test${n}.key2
+published=$(awk '{print $3}' < published.test${n}.key2)
+set_keytime "KEY2" "PUBLISHED" "${published}"
+set_keytime "KEY2" "ACTIVE"    "${published}"
+published=$(key_get KEY2 PUBLISHED)
+# Key lifetime is 6 months, 315360000 seconds.
+set_addkeytime "KEY2" "RETIRED"     "${published}" 16070400
+# The key is removed after the retire time plus max zone ttl (1d), zone
+# propagation delay (300s), retire safety (1h), and sign delay (signature
+# validity minus refresh, 9d) = 86400 + 300 + 3600 + 777600 = 867900.
+retired=$(key_get KEY2 RETIRED)
+set_addkeytime "KEY2" "REMOVED"   "${retired}"   867900
+
+check_keytimes
+check_apex
+check_subdomain
+dnssec_verify
+
 #
 # Test dnssec-policy inheritance.
 #
--- a/doc/notes/notes-current.rst
+++ b/doc/notes/notes-current.rst
@@ -92,3 +92,7 @@ Bug Fixes

 - Named ``allow-update`` acls where broken in BIND 9.17.9 and BIND 9.16.11
  preventing ``named`` starting. [GL #2413]
+
+- When migrating to ``dnssec-policy``, BIND considered keys with the "Inactive"
+  and/or "Delete" timing metadata as possible active keys. This has been fixed.
+  [GL #2406]
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -1407,10 +1407,11 @@ static void
 keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) {
 	bool ksk, zsk;
 	isc_result_t ret;
-	isc_stdtime_t active = 0, pub = 0, syncpub = 0;
+	isc_stdtime_t active = 0, pub = 0, syncpub = 0, retire = 0, remove = 0;
 	dst_key_state_t dnskey_state = HIDDEN;
 	dst_key_state_t ds_state = HIDDEN;
 	dst_key_state_t zrrsig_state = HIDDEN;
+	dst_key_state_t goal_state = HIDDEN;

 	REQUIRE(key != NULL);
 	REQUIRE(key->key != NULL);
@@ -1437,6 +1438,7 @@ keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) {
 		} else {
 			dnskey_state = RUMOURED;
 		}
+		goal_state = OMNIPRESENT;
 	}
 	ret = dst_key_gettime(key->key, DST_TIME_PUBLISH, &pub);
 	if (pub <= now && ret == ISC_R_SUCCESS) {
@@ -1447,6 +1449,7 @@ keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) {
 		} else {
 			zrrsig_state = RUMOURED;
 		}
+		goal_state = OMNIPRESENT;
 	}
 	ret = dst_key_gettime(key->key, DST_TIME_SYNCPUBLISH, &syncpub);
 	if (syncpub <= now && ret == ISC_R_SUCCESS) {
@@ -1457,6 +1460,38 @@ keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) {
 		} else {
 			ds_state = RUMOURED;
 		}
+		goal_state = OMNIPRESENT;
+	}
+	ret = dst_key_gettime(key->key, DST_TIME_INACTIVE, &retire);
+	if (retire <= now && ret == ISC_R_SUCCESS) {
+		dns_ttl_t zone_ttl = dns_kasp_zonemaxttl(kasp);
+		zone_ttl += dns_kasp_zonepropagationdelay(kasp);
+		if ((retire + zone_ttl) <= now) {
+			zrrsig_state = HIDDEN;
+		} else {
+			zrrsig_state = UNRETENTIVE;
+		}
+		ds_state = UNRETENTIVE;
+		goal_state = HIDDEN;
+	}
+	ret = dst_key_gettime(key->key, DST_TIME_DELETE, &remove);
+	if (remove <= now && ret == ISC_R_SUCCESS) {
+		dns_ttl_t key_ttl = dst_key_getttl(key->key);
+		key_ttl += dns_kasp_zonepropagationdelay(kasp);
+		if ((remove + key_ttl) <= now) {
+			dnskey_state = HIDDEN;
+		} else {
+			dnskey_state = UNRETENTIVE;
+		}
+		zrrsig_state = HIDDEN;
+		ds_state = HIDDEN;
+		goal_state = HIDDEN;
+	}
+
+	/* Set goal if not already set. */
+	if (dst_key_getstate(key->key, DST_KEY_GOAL, &goal_state) !=
+	    ISC_R_SUCCESS) {
+		dst_key_setstate(key->key, DST_KEY_GOAL, goal_state);
 	}

 	/* Set key states for all keys that do not have them. */
@@ -1807,19 +1842,6 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 					continue;
 				}

-				/*
-				 * This is possibly an active key created
-				 * outside dnssec-policy.  Initialize goal,
-				 * if not set.
-				 */
-				dst_key_state_t goal;
-				if (dst_key_getstate(dkey->key, DST_KEY_GOAL,
-						     &goal) != ISC_R_SUCCESS) {
-					dst_key_setstate(dkey->key,
-							 DST_KEY_GOAL,
-							 OMNIPRESENT);
-				}
-
 				/*
 				 * Save the matched key only if it is active
 				 * or desires to be active.