Merge branch '3302-keymgr-updates-key-files-each-run' into 'main'

Fix "keymgr updates key files each run"

Closes #3302

See merge request isc-projects/bind9!6250

CHANGES

@@ -1,3 +1,6 @@
+5888.	[bug]		Only write key files if the dnssec-policy keymgr has
+			changed the metadata. [GL #3302]
+
 5887.	[cleanup]	Remove the on-shutdown mechanics from isc_task API.
 			Replace it by isc_task_send() when we are shutting
 			down. [GL !6275]

bin/tests/system/kasp.sh

@@ -64,6 +64,9 @@ VIEW3="C1Azf+gGPMmxrUg/WQINP6eV9Y0="
 # EXPECT_KRRSIG
 # LEGACY
 # PRIVATE
+# PRIVKEY_STAT
+# PUBKEY_STAT
+# STATE_STAT
 
 key_key() {
 	echo "${1}__${2}"
@@ -86,6 +89,10 @@ key_save()
 	key_set "$1" BASEFILE "$BASE_FILE"
 	# Save creation date.
 	key_set "$1" CREATED "${KEY_CREATED}"
+	# Save key change time.
+	key_set "$1" PRIVKEY_STAT $(stat -c '%Z' "${BASE_FILE}.private")
+	key_set "$1" PUBKEY_STAT $(stat -c '%Z' "${BASE_FILE}.key")
+	key_set "$1" STATE_STAT $(stat -c '%Z' "${BASE_FILE}.state")
 }
 
 # Clear key state.
@@ -98,6 +105,7 @@ key_clear() {
 	key_set "$1" "ROLE" 'none'
 	key_set "$1" "KSK" 'no'
 	key_set "$1" "ZSK" 'no'
+	key_set "$1" "FLAGS" '0'
 	key_set "$1" "LIFETIME" 'none'
 	key_set "$1" "ALG_NUM" '0'
 	key_set "$1" "ALG_STR" 'none'
@@ -118,7 +126,9 @@ key_clear() {
 	key_set "$1" "EXPECT_KRRSIG" 'no'
 	key_set "$1" "LEGACY" 'no'
 	key_set "$1" "PRIVATE" 'yes'
-	key_set "$1" "FLAGS" '0'
+	key_set "$1" "PRIVKEY_STAT" '0'
+	key_set "$1" "PUBKEY_STAT" '0'
+	key_set "$1" "STATE_STAT" '0'
 }
 
 # Start clear.

bin/tests/system/kasp/tests.sh

@@ -293,6 +293,44 @@ check_apex
 check_subdomain
 dnssec_verify
 
+# Trigger a keymgr run. Make sure the key files are not touched if there are
+# no modifications to the key metadata.
+n=$((n+1))
+echo_i "make sure key files are untouched if metadata does not change ($n)"
+ret=0
+basefile=$(key_get KEY1 BASEFILE)
+privkey_stat=$(key_get KEY1 PRIVKEY_STAT)
+pubkey_stat=$(key_get KEY1 PUBKEY_STAT)
+state_stat=$(key_get KEY1 STATE_STAT)
+
+nextpart $DIR/named.run > /dev/null
+rndccmd 10.53.0.3 loadkeys "$ZONE" > /dev/null || log_error "rndc loadkeys zone ${ZONE} failed"
+wait_for_log 3 "keymgr: $ZONE done" $DIR/named.run
+privkey_stat2=$(stat -c '%Z' "${basefile}.private")
+pubkey_stat2=$(stat -c '%Z' "${basefile}.key")
+state_stat2=$(stat -c '%Z' "${basefile}.state")
+test "$privkey_stat" = "$privkey_stat2" || log_error "wrong private key file stat (expected $privkey_stat got $privkey_stat2)"
+test "$pubkey_stat" = "$pubkey_stat2" || log_error "wrong public key file stat (expected $pubkey_stat got $pubkey_stat2)"
+test "$state_stat" = "$state_stat2" || log_error "wrong state file stat (expected $state_stat got $state_stat2)"
+test "$ret" -eq 0 || echo_i "failed"
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "again ($n)"
+ret=0
+
+nextpart $DIR/named.run > /dev/null
+rndccmd 10.53.0.3 loadkeys "$ZONE" > /dev/null || log_error "rndc loadkeys zone ${ZONE} failed"
+wait_for_log 3 "keymgr: done" $DIR/named.run
+privkey_stat2=$(stat -c '%Z' "${basefile}.private")
+pubkey_stat2=$(stat -c '%Z' "${basefile}.key")
+state_stat2=$(stat -c '%Z' "${basefile}.state")
+test "$privkey_stat" = "$privkey_stat2" || log_error "wrong private key file stat (expected $privkey_stat got $privkey_stat2)"
+test "$pubkey_stat" = "$pubkey_stat2" || log_error "wrong public key file stat (expected $pubkey_stat got $pubkey_stat2)"
+test "$state_stat" = "$state_stat2" || log_error "wrong state file stat (expected $state_stat got $state_stat2)"
+test "$ret" -eq 0 || echo_i "failed"
+status=$((status+ret))
+
 # Update zone.
 n=$((n+1))
 echo_i "modify unsigned zone file and check that new record is signed for zone ${ZONE} ($n)"

doc/notes/notes-current.rst

@@ -40,4 +40,6 @@ Feature Changes
 Bug Fixes
 ~~~~~~~~~
 
-- None.
+- Key files were updated every time the ``dnssec-policy`` key manager ran,
+  whether the metadata has changed or not. BIND now checks if changes were
+  applied before writing out the key files. :gl:`#3302`.

lib/dns/dst_api.c

@@ -469,6 +469,16 @@ dst_key_isexternal(dst_key_t *key) {
 	return (key->external);
 }
 
+void
+dst_key_setmodified(dst_key_t *key, bool value) {
+	key->modified = value;
+}
+
+bool
+dst_key_ismodified(dst_key_t *key) {
+	return (key->modified);
+}
+
 isc_result_t
 dst_key_getfilename(dns_name_t *name, dns_keytag_t id, unsigned int alg,
 		    int type, const char *directory, isc_mem_t *mctx,
@@ -610,6 +620,7 @@ dst_key_fromnamedfile(const char *filename, const char *dirname, int type,
 	    (pubkey->key_flags & DNS_KEYFLAG_TYPEMASK) == DNS_KEYTYPE_NOKEY)
 	{
 		RETERR(computeid(pubkey));
+		pubkey->modified = false;
 		*keyp = pubkey;
 		pubkey = NULL;
 		goto out;
@@ -663,6 +674,7 @@ dst_key_fromnamedfile(const char *filename, const char *dirname, int type,
 		RETERR(DST_R_INVALIDPRIVATEKEY);
 	}
 
+	key->modified = false;
 	*keyp = key;
 	key = NULL;
 
@@ -1016,6 +1028,8 @@ dst_key_setbool(dst_key_t *key, int type, bool value) {
 	REQUIRE(type <= DST_MAX_BOOLEAN);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || !key->boolset[type] ||
+			key->bools[type] != value;
 	key->bools[type] = value;
 	key->boolset[type] = true;
 	isc_mutex_unlock(&key->mdlock);
@@ -1027,6 +1041,7 @@ dst_key_unsetbool(dst_key_t *key, int type) {
 	REQUIRE(type <= DST_MAX_BOOLEAN);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || key->boolset[type];
 	key->boolset[type] = false;
 	isc_mutex_unlock(&key->mdlock);
 }
@@ -1054,6 +1069,8 @@ dst_key_setnum(dst_key_t *key, int type, uint32_t value) {
 	REQUIRE(type <= DST_MAX_NUMERIC);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || !key->numset[type] ||
+			key->nums[type] != value;
 	key->nums[type] = value;
 	key->numset[type] = true;
 	isc_mutex_unlock(&key->mdlock);
@@ -1065,6 +1082,7 @@ dst_key_unsetnum(dst_key_t *key, int type) {
 	REQUIRE(type <= DST_MAX_NUMERIC);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || key->numset[type];
 	key->numset[type] = false;
 	isc_mutex_unlock(&key->mdlock);
 }
@@ -1091,6 +1109,8 @@ dst_key_settime(dst_key_t *key, int type, isc_stdtime_t when) {
 	REQUIRE(type <= DST_MAX_TIMES);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || !key->timeset[type] ||
+			key->times[type] != when;
 	key->times[type] = when;
 	key->timeset[type] = true;
 	isc_mutex_unlock(&key->mdlock);
@@ -1102,6 +1122,7 @@ dst_key_unsettime(dst_key_t *key, int type) {
 	REQUIRE(type <= DST_MAX_TIMES);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || key->timeset[type];
 	key->timeset[type] = false;
 	isc_mutex_unlock(&key->mdlock);
 }
@@ -1129,6 +1150,8 @@ dst_key_setstate(dst_key_t *key, int type, dst_key_state_t state) {
 	REQUIRE(type <= DST_MAX_KEYSTATES);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || !key->keystateset[type] ||
+			key->keystates[type] != state;
 	key->keystates[type] = state;
 	key->keystateset[type] = true;
 	isc_mutex_unlock(&key->mdlock);
@@ -1140,6 +1163,7 @@ dst_key_unsetstate(dst_key_t *key, int type) {
 	REQUIRE(type <= DST_MAX_KEYSTATES);
 
 	isc_mutex_lock(&key->mdlock);
+	key->modified = key->modified || key->keystateset[type];
 	key->keystateset[type] = false;
 	isc_mutex_unlock(&key->mdlock);
 }
@@ -2704,4 +2728,6 @@ dst_key_copy_metadata(dst_key_t *to, dst_key_t *from) {
 			dst_key_unsetstate(to, i);
 		}
 	}
+
+	dst_key_setmodified(to, dst_key_ismodified(from));
 }

lib/dns/dst_internal.h

@@ -122,6 +122,7 @@ struct dst_key {
 	bool inactive; /*%< private key not present as it is
 			* inactive */
 	bool external; /*%< external key */
+	bool modified; /*%< set to true if key file metadata has changed */
 
 	int fmt_major; /*%< private key format, major version
 			* */

lib/dns/include/dst/dst.h

@@ -1106,6 +1106,26 @@ dst_key_isexternal(dst_key_t *key);
  *	'key' to be valid.
  */
 
+void
+dst_key_setmodified(dst_key_t *key, bool value);
+/*%<
+ * If 'value' is true, this marks the key to indicate that key file metadata
+ * has been modified. If 'value' is false, this resets the value, for example
+ * after you have written the key to file.
+ *
+ * Requires:
+ *	'key' to be valid.
+ */
+
+bool
+dst_key_ismodified(dst_key_t *key);
+/*%<
+ * Check if the key file has been modified.
+ *
+ * Requires:
+ *	'key' to be valid.
+ */
+
 bool
 dst_key_haskasp(dst_key_t *key);
 /*%<

lib/dns/keymgr.c

@@ -1511,6 +1511,7 @@ transition:
 			/* It is safe to make the transition. */
 			dst_key_setstate(dkey->key, i, next_state);
 			dst_key_settime(dkey->key, keystatetimes[i], now);
+			INSIST(dst_key_ismodified(dkey->key));
 			changed = true;
 		}
 	}
@@ -2179,9 +2180,10 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 	for (dns_dnsseckey_t *dkey = ISC_LIST_HEAD(*keyring); dkey != NULL;
 	     dkey = ISC_LIST_NEXT(dkey, link))
 	{
-		if (!dkey->purge) {
+		if (dst_key_ismodified(dkey->key) && !dkey->purge) {
 			dns_dnssec_get_hints(dkey, now);
 			RETERR(dst_key_tofile(dkey->key, options, directory));
+			dst_key_setmodified(dkey->key, false);
 		}
 	}
 
@@ -2201,6 +2203,13 @@ failure:
 		}
 	}
 
+	if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(3))) {
+		char namebuf[DNS_NAME_FORMATSIZE];
+		dns_name_format(origin, namebuf, sizeof(namebuf));
+		isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC,
+			      DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(3),
+			      "keymgr: %s done", namebuf);
+	}
 	return (result);
 }
 
@@ -2278,6 +2287,9 @@ keymgr_checkds(dns_kasp_t *kasp, dns_dnsseckeylist_t *keyring,
 
 	dns_dnssec_get_hints(ksk_key, now);
 	result = dst_key_tofile(ksk_key->key, options, directory);
+	if (result == ISC_R_SUCCESS) {
+		dst_key_setmodified(ksk_key->key, false);
+	}
 	isc_dir_close(&dir);
 
 	return (result);
@@ -2578,6 +2590,9 @@ dns_keymgr_rollover(dns_kasp_t *kasp, dns_dnsseckeylist_t *keyring,
 
 	dns_dnssec_get_hints(key, now);
 	result = dst_key_tofile(key->key, options, directory);
+	if (result == ISC_R_SUCCESS) {
+		dst_key_setmodified(key->key, false);
+	}
 	isc_dir_close(&dir);
 
 	return (result);