From 6d72f5ba416f4e49d502d7d53703c504e867b236 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 3 Jun 2025 14:38:28 +0200 Subject: [PATCH 1/2] Test purge-keys with views Create a test scenario where a signed zone is in multiple views and then a key may be purged. This is a bug case where the key files are removed by one view and then the other view starts complaining. Note: This commit was manually modified because 9.18 does not have pytest based kasp system tests. The test was translated to a shell script style test case. (cherry picked from commit 752d8617f558130cc552cae0e903aca318a3ef02) --- bin/tests/system/kasp/ns4/named.conf.in | 16 +++++++++++++ bin/tests/system/kasp/ns4/purgekeys1.conf | 28 +++++++++++++++++++++++ bin/tests/system/kasp/ns4/purgekeys2.conf | 21 +++++++++++++++++ bin/tests/system/kasp/ns4/setup.sh | 19 +++++++++++++++ bin/tests/system/kasp/tests.sh | 27 ++++++++++++++++++++++ bin/tests/system/kasp/tests_sh_kasp.py | 2 ++ 6 files changed, 113 insertions(+) create mode 100644 bin/tests/system/kasp/ns4/purgekeys1.conf create mode 100644 bin/tests/system/kasp/ns4/purgekeys2.conf diff --git a/bin/tests/system/kasp/ns4/named.conf.in b/bin/tests/system/kasp/ns4/named.conf.in index 459ea73a89..67d89fe82f 100644 --- a/bin/tests/system/kasp/ns4/named.conf.in +++ b/bin/tests/system/kasp/ns4/named.conf.in @@ -13,6 +13,8 @@ // NS4 +include "purgekeys.conf"; + key rndc_key { secret "1234abcd8765"; algorithm @DEFAULT_HMAC@; @@ -157,6 +159,13 @@ view "example1" { type primary; file "example1.db"; }; + + zone "purgekeys.kasp" { + type primary; + file "purgekeys.kasp.example1.db"; + dnssec-policy "purgekeys"; + inline-signing yes; + }; }; view "example2" { @@ -167,6 +176,13 @@ view "example2" { file "example2.db"; inline-signing yes; }; + + zone "purgekeys.kasp" { + type primary; + file "purgekeys.kasp.example2.db"; + dnssec-policy "purgekeys"; + inline-signing yes; + }; }; view "example3" { diff --git a/bin/tests/system/kasp/ns4/purgekeys1.conf b/bin/tests/system/kasp/ns4/purgekeys1.conf new file mode 100644 index 0000000000..9845c8936c --- /dev/null +++ b/bin/tests/system/kasp/ns4/purgekeys1.conf @@ -0,0 +1,28 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +dnssec-policy "purgekeys" { + keys { + ksk key-directory lifetime 0 algorithm 13; + zsk key-directory lifetime P30D algorithm 13; + }; + /* + * Initially set to 0, so no keys are purged. Keys that are no longer + * in use will still be in the zone's keyring, one per view. After + * reconfig the purge-keys value is set to 7 days, at least one key + * will be eligible for purging, and should be purged from both + * keyrings without issues. + */ + purge-keys 0; + //purge-keys P7D; +}; diff --git a/bin/tests/system/kasp/ns4/purgekeys2.conf b/bin/tests/system/kasp/ns4/purgekeys2.conf new file mode 100644 index 0000000000..62376c1fd7 --- /dev/null +++ b/bin/tests/system/kasp/ns4/purgekeys2.conf @@ -0,0 +1,21 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +dnssec-policy "purgekeys" { + keys { + ksk key-directory lifetime 0 algorithm 13; + zsk key-directory lifetime P30D algorithm 13; + }; + //purge-keys 0; + purge-keys P7D; +}; diff --git a/bin/tests/system/kasp/ns4/setup.sh b/bin/tests/system/kasp/ns4/setup.sh index c488bc4588..2b1c9d7944 100644 --- a/bin/tests/system/kasp/ns4/setup.sh +++ b/bin/tests/system/kasp/ns4/setup.sh @@ -30,3 +30,22 @@ done cp example1.db.in example1.db cp example2.db.in example2.db + +# Regression test for GL #5315 +cp purgekeys1.conf purgekeys.conf +cp example1.db.in purgekeys.kasp.example1.db +cp example2.db.in purgekeys.kasp.example2.db + +zone="purgekeys.kasp" +H="HIDDEN" +O="OMNIPRESENT" +T="now-9mo" +# KSK omnipresent +KSK=$($KEYGEN -fk -a 13 -L 3600 $zone 2>keygen.out.$zone.1) +$SETTIME -s -g $O -d $O $T -k $O $T -r $O $T "$KSK" >settime.out.$zone.1 2>&1 +# ZSK omnipresent +ZSK1=$($KEYGEN -a 13 -L 3600 $zone 2>keygen.out.$zone.2) +$SETTIME -s -g $O -k $O $T -z $O $T "$ZSK1" >settime.out.$zone.2 2>&1 +# ZSK hidden (may be purged) +ZSK2=$($KEYGEN -a 13 -L 3600 $zone 2>keygen.out.$zone.2) +$SETTIME -s -g $H -k $H $T -z $H $T "$ZSK2" >settime.out.$zone.2 2>&1 diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh index ceb4e3b80a..9f3c6823c0 100644 --- a/bin/tests/system/kasp/tests.sh +++ b/bin/tests/system/kasp/tests.sh @@ -2105,6 +2105,33 @@ check_signatures TXT "dig.out.$DIR.test$n.txt" "ZSK" test "$ret" -eq 0 || echo_i "failed" status=$((status + ret)) +# +# Test purge-keys in combination with views [GL #5315]. +# +set_zone "purgekeys.kasp" +set_policy "purgekeys" "2" "3600" +set_server "ns4" "10.53.0.4" + +TSIG="$DEFAULT_HMAC:keyforview1:$VIEW1" +wait_for_nsec +dnssec_verify + +TSIG="$DEFAULT_HMAC:keyforview2:$VIEW2" +wait_for_nsec +dnssec_verify + +# Reconfig, make sure the purged key is not an issue when verifying keys. +cp ns4/purgekeys2.conf ns4/purgekeys.conf || ret=1 +nextpart ns4/named.run >/dev/null +rndccmd 10.53.0.4 reconfig || ret=1 +wait_for_log 3 "keymgr: $ZONE done" ns4/named.run || ret=1 + +grep "zone $ZONE/IN/example1 (signed): zone_rekey:zone_verifykeys failed: some key files are missing" ns4/named.run && ret=1 +grep "zone $ZONE/IN/example2 (signed): zone_rekey:zone_verifykeys failed: some key files are missing" ns4/named.run && ret=1 + +test "$ret" -eq 0 || echo_i "failed" +status=$((status + ret)) + # Clear TSIG. TSIG="" diff --git a/bin/tests/system/kasp/tests_sh_kasp.py b/bin/tests/system/kasp/tests_sh_kasp.py index fa7a7f64bd..022aa6b324 100644 --- a/bin/tests/system/kasp/tests_sh_kasp.py +++ b/bin/tests/system/kasp/tests_sh_kasp.py @@ -58,6 +58,8 @@ pytestmark = pytest.mark.extra_artifacts( "ns*/*.zsk2", "ns3/legacy-keys.*", "ns3/dynamic-signed-inline-signing.kasp.db.signed.signed", + "ns4/purgekeys.conf", + "ns4/purgekeys2.conf", ] ) From 5f589541bce029d99c208342879cc51e5a6e10be Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 22 May 2025 11:23:48 +0200 Subject: [PATCH 2/2] Fix spurious missing key files log messages This happens because old key is purged by one zone view, then the other is freaking out about it. Keys that are unused or being purged should not be taken into account when verifying key files are available. The keyring is maintained per zone. So in one zone, a key in the keyring is being purged. The corresponding key file is removed. The key maintenance is done for the other zone view. The key in that keyring is not yet set to purge, but its corresponding key file is removed. This leads to "some keys are missing" log errors. We should not check the purge variable at this point, but the current time and purge-keys duration. This commit fixes this erroneous logic. (cherry picked from commit d494698852e21e25d65d1e2453813a7b19a0a755) --- lib/dns/dst_api.c | 4 ++-- lib/dns/include/dns/keymgr.h | 13 +++++++++++++ lib/dns/include/dst/dst.h | 4 ++-- lib/dns/keymgr.c | 11 ++++++----- lib/dns/zone.c | 9 +++++++-- 5 files changed, 30 insertions(+), 11 deletions(-) diff --git a/lib/dns/dst_api.c b/lib/dns/dst_api.c index 537a13920e..5ee6098777 100644 --- a/lib/dns/dst_api.c +++ b/lib/dns/dst_api.c @@ -2449,7 +2449,7 @@ dst_key_tkeytoken(const dst_key_t *key) { * */ bool -dst_key_is_unused(dst_key_t *key) { +dst_key_is_unused(const dst_key_t *key) { isc_stdtime_t val; dst_key_state_t st; int state_type; @@ -2751,7 +2751,7 @@ dst_key_is_removed(dst_key_t *key, isc_stdtime_t now, isc_stdtime_t *remove) { } dst_key_state_t -dst_key_goal(dst_key_t *key) { +dst_key_goal(const dst_key_t *key) { dst_key_state_t state; isc_result_t result; diff --git a/lib/dns/include/dns/keymgr.h b/lib/dns/include/dns/keymgr.h index bf08fbb549..e1bd0366c6 100644 --- a/lib/dns/include/dns/keymgr.h +++ b/lib/dns/include/dns/keymgr.h @@ -128,4 +128,17 @@ dns_keymgr_status(dns_kasp_t *kasp, dns_dnsseckeylist_t *keyring, * */ +bool +dns_keymgr_key_may_be_purged(const dst_key_t *key, uint32_t after, + isc_stdtime_t now); +/*%< + * Checks if the key files for 'key' may be removed from disk. + * + * Requires: + *\li 'key' is a valid key. + * + * Returns: + *\li true if the key files may be purged, false otherwise. + */ + ISC_LANG_ENDDECLS diff --git a/lib/dns/include/dst/dst.h b/lib/dns/include/dst/dst.h index f845e9bd2e..49be618790 100644 --- a/lib/dns/include/dst/dst.h +++ b/lib/dns/include/dst/dst.h @@ -1154,7 +1154,7 @@ dst_key_haskasp(dst_key_t *key); */ bool -dst_key_is_unused(dst_key_t *key); +dst_key_is_unused(const dst_key_t *key); /*%< * Check if this key is unused. * @@ -1211,7 +1211,7 @@ dst_key_is_removed(dst_key_t *key, isc_stdtime_t now, isc_stdtime_t *remove); */ dst_key_state_t -dst_key_goal(dst_key_t *key); +dst_key_goal(const dst_key_t *key); /*%< * Get the key goal. Should be OMNIPRESENT or HIDDEN. * This can be used to determine if the key is being introduced or diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c index cbcd3c992e..ff2f6c85ce 100644 --- a/lib/dns/keymgr.c +++ b/lib/dns/keymgr.c @@ -594,7 +594,7 @@ keymgr_desiredstate(dns_dnsseckey_t *key, dst_key_state_t state) { * */ static bool -keymgr_key_match_state(dst_key_t *key, dst_key_t *subject, int type, +keymgr_key_match_state(const dst_key_t *key, const dst_key_t *subject, int type, dst_key_state_t next_state, dst_key_state_t states[NUM_KEYSTATES]) { REQUIRE(key != NULL); @@ -1933,8 +1933,9 @@ keymgr_key_rollover(dns_kasp_key_t *kaspkey, dns_dnsseckey_t *active_key, return ISC_R_SUCCESS; } -static bool -keymgr_key_may_be_purged(dst_key_t *key, uint32_t after, isc_stdtime_t now) { +bool +dns_keymgr_key_may_be_purged(const dst_key_t *key, uint32_t after, + isc_stdtime_t now) { bool ksk = false; bool zsk = false; dst_key_state_t hidden[NUM_KEYSTATES] = { HIDDEN, NA, NA, NA }; @@ -2117,8 +2118,8 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass, } /* Check purge-keys interval. */ - if (keymgr_key_may_be_purged(dkey->key, - dns_kasp_purgekeys(kasp), now)) + if (dns_keymgr_key_may_be_purged(dkey->key, + dns_kasp_purgekeys(kasp), now)) { dst_key_format(dkey->key, keystr, sizeof(keystr)); isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC, diff --git a/lib/dns/zone.c b/lib/dns/zone.c index e91c2f6bde..2f61d860be 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -22016,7 +22016,8 @@ update_ttl(dns_rdataset_t *rdataset, dns_name_t *name, dns_ttl_t ttl, } static isc_result_t -zone_verifykeys(dns_zone_t *zone, dns_dnsseckeylist_t *newkeys) { +zone_verifykeys(dns_zone_t *zone, dns_dnsseckeylist_t *newkeys, + uint32_t purgeval, isc_stdtime_t now) { dns_dnsseckey_t *key1, *key2, *next; /* @@ -22029,6 +22030,9 @@ zone_verifykeys(dns_zone_t *zone, dns_dnsseckeylist_t *newkeys) { if (dst_key_is_unused(key1->key)) { continue; } + if (dns_keymgr_key_may_be_purged(key1->key, purgeval, now)) { + continue; + } if (key1->purge) { continue; } @@ -22224,7 +22228,8 @@ zone_rekey(dns_zone_t *zone) { if (kasp != NULL) { /* Verify new keys. */ - isc_result_t ret = zone_verifykeys(zone, &keys); + isc_result_t ret = zone_verifykeys( + zone, &keys, dns_kasp_purgekeys(kasp), now); if (ret != ISC_R_SUCCESS) { dnssec_log(zone, ISC_LOG_ERROR, "zone_rekey:zone_verifykeys failed: "