diff --git a/CHANGES b/CHANGES index 5abbe7dc97..92a04312ee 100644 --- a/CHANGES +++ b/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] diff --git a/bin/tests/system/kasp.sh b/bin/tests/system/kasp.sh index 01bcce3fd0..f41911a68e 100644 --- a/bin/tests/system/kasp.sh +++ b/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. diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh index bcea02f65d..0607b8e941 100644 --- a/bin/tests/system/kasp/tests.sh +++ b/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)" diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index aa545fc852..d22aea1b55 100644 --- a/doc/notes/notes-current.rst +++ b/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`. diff --git a/lib/dns/dst_api.c b/lib/dns/dst_api.c index d7d28aace6..f36f087fd0 100644 --- a/lib/dns/dst_api.c +++ b/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)); } diff --git a/lib/dns/dst_internal.h b/lib/dns/dst_internal.h index 9a0b3ac905..9f5e8ee3b6 100644 --- a/lib/dns/dst_internal.h +++ b/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 * */ diff --git a/lib/dns/include/dst/dst.h b/lib/dns/include/dst/dst.h index c53a1adccf..8fe32ab15c 100644 --- a/lib/dns/include/dst/dst.h +++ b/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); /*%< diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c index 660b8cd26f..4d9ccfbd9e 100644 --- a/lib/dns/keymgr.c +++ b/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);