From 008d3d2a9c0bb807ce392374bcc63a6f79b9d088 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 19 Aug 2025 12:42:35 +0200 Subject: [PATCH 1/4] Test rndc sign updates the signatures Add a check to the ZSK rollover test case that ensures the zone is signed with the successor key only, after a 'rndc sign' is commanded. --- .../tests_rollover_zsk_prepublication.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py b/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py index c8643022e6..e5d842c535 100644 --- a/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py +++ b/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py @@ -222,6 +222,14 @@ def test_zsk_prepub_step3(tld, alg, size, ns3): } isctest.kasp.check_rollover_step(ns3, CONFIG, policy, step) + # Force full resign and check all signatures have been replaced. + with ns3.watch_log_from_here() as watcher: + ns3.rndc(f"sign {zone}", log=False) + watcher.wait_for_line(f"zone {zone}/IN (signed): sending notifies") + + step["smooth"] = False + isctest.kasp.check_rollover_step(ns3, CONFIG, POLICY, step) + @pytest.mark.parametrize( "tld", From 844bde0c70bca83f9f46d1b6356804c0a44e9dcd Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 19 Aug 2025 12:37:29 +0200 Subject: [PATCH 2/4] Force full sign to generate new signatures When introducing the kasp logic, a full sign of the zone did not generate new signatures for the new active keys during a ZSK rollover. The introduced kasp logic ensured that the rollover is performed smoothly, as in the signatures are only replaced if the old signature is close to expiring (depending on the signatures-refresh option). Fix by maintaining a fullsign boolean value in the signing structure, that will ensure the RRsets are signed with the correct key, rather than a similar good key. In case of a fullsign, we can also remove signatures from inactive keys. Remove the unused dns_zone_signwithkey function. --- lib/dns/include/dns/zone.h | 8 ---- lib/dns/zone.c | 89 +++++++++++++++++++++----------------- 2 files changed, 49 insertions(+), 48 deletions(-) diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index 8812709548..d468256a23 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -2164,14 +2164,6 @@ dns_zone_getsignatures(dns_zone_t *zone); * Get the number of signatures that will be generated per quantum. */ -isc_result_t -dns_zone_signwithkey(dns_zone_t *zone, dst_algorithm_t algorithm, - uint16_t keyid, bool deleteit); -/*%< - * Initiate/resume signing of the entire zone with the zone DNSKEY(s) - * that match the given algorithm and keyid. - */ - isc_result_t dns_zone_addnsec3chain(dns_zone_t *zone, dns_rdata_nsec3param_t *nsec3param); /*%< diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 68685dcad2..fa15131106 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -751,6 +751,7 @@ struct dns_signing { dst_algorithm_t algorithm; uint16_t keyid; bool deleteit; + bool fullsign; bool done; ISC_LINK(dns_signing_t) link; }; @@ -993,7 +994,7 @@ static void dump_done(void *arg, isc_result_t result); static isc_result_t zone_signwithkey(dns_zone_t *zone, dst_algorithm_t algorithm, uint16_t keyid, - bool deleteit); + bool deleteit, bool fullsign); static isc_result_t delete_nsec(dns_db_t *db, dns_dbversion_t *ver, dns_dbnode_t *node, dns_name_t *name, dns_diff_t *diff); @@ -4101,7 +4102,7 @@ resume_signingwithkey(dns_zone_t *zone) { : ((rdata.data[5] << 8) | rdata.data[6]); result = zone_signwithkey(zone, alg, (rdata.data[1] << 8) | rdata.data[2], - rdata.data[3]); + rdata.data[3], false); if (result != ISC_R_SUCCESS) { dnssec_log(zone, ISC_LOG_ERROR, "zone_signwithkey failed: %s", @@ -7907,7 +7908,7 @@ failure: static bool signed_with_good_key(dns_zone_t *zone, dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, dns_rdatatype_t type, - dst_key_t *key) { + dst_key_t *key, bool fullsign) { isc_result_t result; dns_rdataset_t rdataset; dns_rdata_rrsig_t rrsig; @@ -7940,7 +7941,7 @@ signed_with_good_key(dns_zone_t *zone, dns_db_t *db, dns_dbnode_t *node, } } - if (zone->kasp != NULL) { + if (zone->kasp != NULL && !fullsign) { int zsk_count = 0; bool approved; @@ -8041,8 +8042,9 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name, dns_dbnode_t *node, dns_dbversion_t *version, bool build_nsec3, bool build_nsec, dst_key_t *key, isc_stdtime_t now, isc_stdtime_t inception, isc_stdtime_t expire, dns_ttl_t nsecttl, - bool both, bool is_ksk, bool is_zsk, bool is_bottom_of_zone, - dns_diff_t *diff, int32_t *signatures, isc_mem_t *mctx) { + bool both, bool is_ksk, bool is_zsk, bool fullsign, + bool is_bottom_of_zone, dns_diff_t *diff, int32_t *signatures, + isc_mem_t *mctx) { isc_result_t result; dns_rdatasetiter_t *iterator = NULL; dns_rdataset_t rdataset = DNS_RDATASET_INIT; @@ -8152,7 +8154,7 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name, continue; } if (signed_with_good_key(zone, db, node, version, rdataset.type, - key)) + key, fullsign)) { continue; } @@ -10168,8 +10170,8 @@ zone_sign(dns_zone_t *zone) { db, zone, name, node, version, build_nsec3, build_nsec, zone_keys[i], now, inception, expire, zone_nsecttl(zone), both, is_ksk, - is_zsk, is_bottom_of_zone, zonediff.diff, - &signatures, zone->mctx)); + is_zsk, signing->fullsign, is_bottom_of_zone, + zonediff.diff, &signatures, zone->mctx)); /* * If we are adding we are done. Look for other keys * of the same algorithm if deleting. @@ -20407,22 +20409,6 @@ dns_zone_setnotifydelay(dns_zone_t *zone, uint32_t delay) { UNLOCK_ZONE(zone); } -isc_result_t -dns_zone_signwithkey(dns_zone_t *zone, dst_algorithm_t algorithm, - uint16_t keyid, bool deleteit) { - isc_result_t result; - REQUIRE(DNS_ZONE_VALID(zone)); - - dnssec_log(zone, ISC_LOG_NOTICE, - "dns_zone_signwithkey(algorithm=%u, keyid=%u)", algorithm, - keyid); - LOCK_ZONE(zone); - result = zone_signwithkey(zone, algorithm, keyid, deleteit); - UNLOCK_ZONE(zone); - - return result; -} - /* * Called when a dynamic update for an NSEC3PARAM record is received. * @@ -20494,7 +20480,7 @@ dns_zone_getprivatetype(dns_zone_t *zone) { static isc_result_t zone_signwithkey(dns_zone_t *zone, dst_algorithm_t algorithm, uint16_t keyid, - bool deleteit) { + bool deleteit, bool fullsign) { dns_signing_t *signing = NULL; isc_result_t result = ISC_R_SUCCESS; isc_time_t now; @@ -20508,6 +20494,7 @@ zone_signwithkey(dns_zone_t *zone, dst_algorithm_t algorithm, uint16_t keyid, signing->algorithm = algorithm; signing->keyid = keyid; signing->deleteit = deleteit; + signing->fullsign = fullsign; signing->done = false; now = isc_time_now(); @@ -22845,7 +22832,8 @@ zone_rekey(dns_zone_t *zone) { /* Remove any signatures from removed keys. */ ISC_LIST_FOREACH(rmkeys, key, link) { result = zone_signwithkey(zone, dst_key_alg(key->key), - dst_key_id(key->key), true); + dst_key_id(key->key), true, + false); if (result != ISC_R_SUCCESS) { dnssec_log(zone, ISC_LOG_ERROR, "zone_signwithkey failed: " @@ -22874,20 +22862,41 @@ zone_rekey(dns_zone_t *zone) { * with all active keys, whether they're new or not. */ ISC_LIST_FOREACH(dnskeys, key, link) { - if (!key->force_sign && !key->hint_sign) { - continue; - } - - result = zone_signwithkey( - zone, dst_key_alg(key->key), - dst_key_id(key->key), false); - if (result != ISC_R_SUCCESS) { - dnssec_log(zone, ISC_LOG_ERROR, - "zone_signwithkey failed: " - "%s", - isc_result_totext(result)); + if (key->force_sign || key->hint_sign) { + result = zone_signwithkey( + zone, dst_key_alg(key->key), + dst_key_id(key->key), false, + true); + if (result != ISC_R_SUCCESS) { + dnssec_log(zone, ISC_LOG_ERROR, + "zone_signwithkey " + "failed: " + "%s", + isc_result_totext( + result)); + } } } + /* + * ...and remove signatures for all inactive keys. + */ + ISC_LIST_FOREACH(dnskeys, key, link) { + if (!key->force_sign && !key->hint_sign) { + result = zone_signwithkey( + zone, dst_key_alg(key->key), + dst_key_id(key->key), true, + false); + if (result != ISC_R_SUCCESS) { + dnssec_log(zone, ISC_LOG_ERROR, + "zone_signwithkey " + "failed: " + "%s", + isc_result_totext( + result)); + } + } + } + } else if (newalg) { /* * We haven't been told to sign fully, but a new @@ -22902,7 +22911,7 @@ zone_rekey(dns_zone_t *zone) { result = zone_signwithkey( zone, dst_key_alg(key->key), - dst_key_id(key->key), false); + dst_key_id(key->key), false, false); if (result != ISC_R_SUCCESS) { dnssec_log(zone, ISC_LOG_ERROR, "zone_signwithkey failed: " From acbf110b1828946341ac22d568dacd4dfff9c387 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 19 Aug 2025 15:10:59 +0200 Subject: [PATCH 3/4] Test the next key event after full sign After a full sign we no longer have to need to take the sign delay into account. --- bin/tests/system/isctest/kasp.py | 9 ++++++--- .../tests_rollover_zsk_prepublication.py | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/bin/tests/system/isctest/kasp.py b/bin/tests/system/isctest/kasp.py index b750cb1f78..6223e5cbb1 100644 --- a/bin/tests/system/isctest/kasp.py +++ b/bin/tests/system/isctest/kasp.py @@ -72,11 +72,12 @@ def IpubC(config, rollover=True): return config["zone-propagation-delay"] + max(ttl1, ttl2) -def Iret(config, zsk=True, ksk=False, rollover=True): +def Iret(config, zsk=True, ksk=False, rollover=True, smooth=True): sign_delay = timedelta(0) safety_interval = timedelta(0) if rollover: - sign_delay = config["signatures-validity"] - config["signatures-refresh"] + if smooth: + sign_delay = config["signatures-validity"] - config["signatures-refresh"] safety_interval = config["retire-safety"] iretKSK = timedelta(0) @@ -246,7 +247,9 @@ class KeyProperties: if "Lifetime" not in self.metadata or self.metadata["Lifetime"] == 0: return - iret = Iret(config, zsk=self.key.is_zsk(), ksk=self.key.is_ksk()) + sigdel = self.key.get_timing("SigRemoved", must_exist=False) + smooth = sigdel is None + iret = Iret(config, zsk=self.key.is_zsk(), ksk=self.key.is_ksk(), smooth=smooth) self.timing["Removed"] = self.timing["Retired"] + iret def set_expected_keytimes( diff --git a/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py b/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py index e5d842c535..702c0f26ad 100644 --- a/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py +++ b/bin/tests/system/rollover-zsk-prepub/tests_rollover_zsk_prepublication.py @@ -41,7 +41,7 @@ POLICY = "zsk-prepub" ZSK_LIFETIME = TIMEDELTA["P30D"] LIFETIME_POLICY = int(ZSK_LIFETIME.total_seconds()) IPUB = Ipub(CONFIG) -IRET = Iret(CONFIG, rollover=True) +IRET = Iret(CONFIG) KEYTTLPROP = CONFIG["dnskey-ttl"] + CONFIG["zone-propagation-delay"] OFFSETS = {} OFFSETS["step1-p"] = -int(TIMEDELTA["P7D"].total_seconds()) @@ -228,6 +228,7 @@ def test_zsk_prepub_step3(tld, alg, size, ns3): watcher.wait_for_line(f"zone {zone}/IN (signed): sending notifies") step["smooth"] = False + step["nextev"] = Iret(CONFIG, smooth=False) isctest.kasp.check_rollover_step(ns3, CONFIG, POLICY, step) From 489752eb1f7f4e09bcf96d723ae64158cf785f97 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 19 Aug 2025 15:16:39 +0200 Subject: [PATCH 4/4] Update the retire interval after full sign After a full sign we no longer have to need to take the sign delay into account. Update the timing checks in keymgr_transition_time to determine the start of the interval: Either the last change, or if SigPublish/ SigDelete is set. The latter case indicates a full sign was done and so we no longer have to take the sign delay into account. --- lib/dns/dst_api.c | 4 ++- lib/dns/dst_parse.c | 7 +++-- lib/dns/include/dns/keymgr.h | 3 ++- lib/dns/include/dst/dst.h | 4 ++- lib/dns/keymgr.c | 50 +++++++++++++++++++++++++++++++++--- lib/dns/zone.c | 3 +++ 6 files changed, 60 insertions(+), 11 deletions(-) diff --git a/lib/dns/dst_api.c b/lib/dns/dst_api.c index a794c9d5bb..88a488772c 100644 --- a/lib/dns/dst_api.c +++ b/lib/dns/dst_api.c @@ -117,7 +117,7 @@ static const char *timingtags[TIMING_NTAGS] = { "DNSKEYChange:", "ZRRSIGChange:", "KRRSIGChange:", "DSChange:", - "DSRemoved:" + "DSRemoved:", "ZRRSIGPublish", "ZRRSIGRemoved" }; #define KEYSTATES_NTAGS (DST_MAX_KEYSTATES + 1) @@ -1991,6 +1991,8 @@ write_key_state(const dst_key_t *key, int type, const char *directory) { printtime(key, DST_TIME_DELETE, "Removed", fp); printtime(key, DST_TIME_DSPUBLISH, "DSPublish", fp); printtime(key, DST_TIME_DSDELETE, "DSRemoved", fp); + printtime(key, DST_TIME_SIGPUBLISH, "SigPublish", fp); + printtime(key, DST_TIME_SIGDELETE, "SigRemoved", fp); printtime(key, DST_TIME_SYNCPUBLISH, "PublishCDS", fp); printtime(key, DST_TIME_SYNCDELETE, "DeleteCDS", fp); diff --git a/lib/dns/dst_parse.c b/lib/dns/dst_parse.c index 5e53a19e49..479d23c603 100644 --- a/lib/dns/dst_parse.c +++ b/lib/dns/dst_parse.c @@ -54,10 +54,9 @@ #define TIMING_NTAGS (DST_MAX_TIMES + 1) static const char *timetags[TIMING_NTAGS] = { - "Created:", "Publish:", "Activate:", "Revoke:", - "Inactive:", "Delete:", "DSPublish:", "SyncPublish:", - "SyncDelete:", NULL, NULL, NULL, - NULL + "Created:", "Publish:", "Activate:", "Revoke:", "Inactive:", + "Delete:", "DSPublish:", "SyncPublish:", "SyncDelete:", NULL, + NULL, NULL, NULL, NULL, NULL }; #define NUMERIC_NTAGS (DST_MAX_NUMERIC + 1) diff --git a/lib/dns/include/dns/keymgr.h b/lib/dns/include/dns/keymgr.h index 8fcc6dbfbb..8e1508304a 100644 --- a/lib/dns/include/dns/keymgr.h +++ b/lib/dns/include/dns/keymgr.h @@ -24,7 +24,8 @@ #define DNS_KEYMGRATTR_NONE 0x00 /*%< No ordering. */ #define DNS_KEYMGRATTR_S2I 0x01 /*%< Secure to insecure. */ #define DNS_KEYMGRATTR_NOROLL 0x02 /*%< No rollover allowed. */ -#define DNS_KEYMGRATTR_FORCESTEP 0x04 /*%< Force next step in manual-mode */ +#define DNS_KEYMGRATTR_FORCESTEP 0x04 /*%< Force next step in manual-mode. */ +#define DNS_KEYMGRATTR_FULLSIGN 0x08 /*%< Full sign was issued. */ void dns_keymgr_settime_syncpublish(dst_key_t *key, dns_kasp_t *kasp, bool first); diff --git a/lib/dns/include/dst/dst.h b/lib/dns/include/dst/dst.h index f3eaa53834..ecd952e845 100644 --- a/lib/dns/include/dst/dst.h +++ b/lib/dns/include/dst/dst.h @@ -152,7 +152,9 @@ typedef enum dst_algorithm { #define DST_TIME_KRRSIG 11 #define DST_TIME_DS 12 #define DST_TIME_DSDELETE 13 -#define DST_MAX_TIMES 13 +#define DST_TIME_SIGPUBLISH 14 +#define DST_TIME_SIGDELETE 15 +#define DST_MAX_TIMES 15 /* Numeric metadata definitions */ #define DST_NUM_PREDECESSOR 0 diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c index bb1f831673..152c6016bb 100644 --- a/lib/dns/keymgr.c +++ b/lib/dns/keymgr.c @@ -1296,9 +1296,9 @@ keymgr_transition_time(dns_dnsseckey_t *key, int type, dst_key_state_t next_state, dns_kasp_t *kasp, isc_stdtime_t now, isc_stdtime_t *when) { isc_result_t ret; - isc_stdtime_t lastchange, dstime, nexttime = now; + isc_stdtime_t lastchange, dstime, sigtime, nexttime = now; dns_ttl_t ttlsig = dns_kasp_zonemaxttl(kasp, true); - uint32_t dsstate; + uint32_t dsstate, sigstate, signdelay = 0; /* * No need to wait if we move things into an uncertain state. @@ -1352,6 +1352,17 @@ keymgr_transition_time(dns_dnsseckey_t *key, int type, switch (next_state) { case OMNIPRESENT: case HIDDEN: + /* Was there a full sign? */ + sigstate = (next_state == HIDDEN) ? DST_TIME_SIGDELETE + : DST_TIME_SIGPUBLISH; + ret = dst_key_gettime(key->key, sigstate, &sigtime); + if (ret == ISC_R_SUCCESS && sigtime <= now) { + signdelay = 0; + } else { + sigtime = lastchange; + signdelay = dns_kasp_signdelay(kasp); + } + /* * RFC 7583: The retire interval (Iret) is the amount * of time that must elapse after a DNSKEY or @@ -1369,7 +1380,7 @@ keymgr_transition_time(dns_dnsseckey_t *key, int type, * * Dsgn + zone-propagation-delay + max-zone-ttl. */ - nexttime = lastchange + ttlsig + + nexttime = sigtime + ttlsig + dns_kasp_zonepropagationdelay(kasp); /* * Only add the sign delay Dsgn and retire-safety if @@ -1383,7 +1394,7 @@ keymgr_transition_time(dns_dnsseckey_t *key, int type, DST_NUM_SUCCESSOR, &tag); } if (ret == ISC_R_SUCCESS) { - nexttime += dns_kasp_signdelay(kasp) + + nexttime += signdelay + dns_kasp_retiresafety(kasp); } break; @@ -2106,6 +2117,32 @@ dst_key_doublematch(dns_dnsseckey_t *key, dns_kasp_t *kasp) { return matches > 1; } +static void +keymgr_zrrsig(dns_dnsseckeylist_t *keyring, isc_stdtime_t now) { + ISC_LIST_FOREACH(*keyring, dkey, link) { + isc_result_t ret; + bool zsk = false; + + ret = dst_key_getbool(dkey->key, DST_BOOL_ZSK, &zsk); + if (ret == ISC_R_SUCCESS && zsk) { + dst_key_state_t state; + isc_result_t result = dst_key_getstate( + dkey->key, DST_KEY_ZRRSIG, &state); + if (result == ISC_R_SUCCESS) { + if (state == RUMOURED) { + dst_key_settime(dkey->key, + DST_TIME_SIGPUBLISH, + now); + } else if (state == UNRETENTIVE) { + dst_key_settime(dkey->key, + DST_TIME_SIGDELETE, + now); + } + } + } + } +} + /* * Examine 'keys' and match 'kasp' policy. * @@ -2308,6 +2345,11 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass, opts |= DNS_KEYMGRATTR_S2I; } + /* In case of a full sign, store ZRRSIGPublish/ZRRSIGDelete. */ + if ((opts & DNS_KEYMGRATTR_FULLSIGN) != 0) { + keymgr_zrrsig(keyring, now); + } + /* Read to update key states. */ isc_result_t retval = keymgr_update(keyring, kasp, now, nexttime, opts); diff --git a/lib/dns/zone.c b/lib/dns/zone.c index fa15131106..901336780a 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -22460,6 +22460,9 @@ zone_rekey(dns_zone_t *zone) { * fully signed now. */ fullsign = DNS_ZONE_OPTION(zone, DNS_ZONEOPT_FULLSIGN); + if (fullsign) { + options |= DNS_KEYMGRATTR_FULLSIGN; + } /* * True when called from "rndc dnssec -step". Indicates the zone