diff --git a/CHANGES b/CHANGES index 9583c7030a..e978a40968 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,12 @@ +5424. [bug] With kasp, when creating a successor key, the goal + state of the current active key (predecessor) was not + changed and thus was never is removed from the zone. + [GL #1846] + +5423. [bug] Fix a bug in keymgr_key_has_successor: it would + return a false positive if any other key in the + keyring has a successor. [GL #1845] + 5422. [bug] When using dnssec-policy, print correct keytiming metadata. [GL #1843] diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh index b0f2d727e2..6547053d05 100644 --- a/bin/tests/system/kasp/tests.sh +++ b/bin/tests/system/kasp/tests.sh @@ -2533,6 +2533,7 @@ set_keyalgorithm "KEY3" "13" "ECDSAP256SHA256" "256" set_keysigning "KEY3" "no" set_zonesigning "KEY3" "no" # Key states. +set_keystate "KEY2" "GOAL" "hidden" set_keystate "KEY3" "GOAL" "omnipresent" set_keystate "KEY3" "STATE_DNSKEY" "rumoured" set_keystate "KEY3" "STATE_ZRRSIG" "hidden" @@ -2570,7 +2571,6 @@ set_server "ns3" "10.53.0.3" # ZSK (KEY2) no longer is actively signing, RRSIG state in UNRETENTIVE. # New ZSK (KEY3) is now actively signing, RRSIG state in RUMOURED. set_zonesigning "KEY2" "no" -set_keystate "KEY2" "GOAL" "hidden" set_keystate "KEY2" "STATE_ZRRSIG" "unretentive" set_zonesigning "KEY3" "yes" set_keystate "KEY3" "STATE_DNSKEY" "omnipresent" @@ -2749,6 +2749,7 @@ set_keyalgorithm "KEY3" "13" "ECDSAP256SHA256" "256" set_keysigning "KEY3" "yes" set_zonesigning "KEY3" "no" # Key states. +set_keystate "KEY1" "GOAL" "hidden" set_keystate "KEY3" "GOAL" "omnipresent" set_keystate "KEY3" "STATE_DNSKEY" "rumoured" set_keystate "KEY3" "STATE_KRRSIG" "rumoured" @@ -2792,7 +2793,6 @@ set_zone "step3.ksk-doubleksk.autosign" set_policy "ksk-doubleksk" "3" "7200" set_server "ns3" "10.53.0.3" # KSK (KEY1) DS will be removed, so it is UNRETENTIVE. -set_keystate "KEY1" "GOAL" "hidden" set_keystate "KEY1" "STATE_DS" "unretentive" # New KSK (KEY3) has its DS submitted. set_keystate "KEY3" "STATE_DNSKEY" "omnipresent" @@ -2978,6 +2978,7 @@ set_keyalgorithm "KEY2" "13" "ECDSAP256SHA256" "256" set_keysigning "KEY2" "yes" set_zonesigning "KEY2" "no" # Key states. +set_keystate "KEY1" "GOAL" "hidden" set_keystate "KEY2" "GOAL" "omnipresent" set_keystate "KEY2" "STATE_DNSKEY" "rumoured" set_keystate "KEY2" "STATE_KRRSIG" "rumoured" @@ -3019,7 +3020,6 @@ set_server "ns3" "10.53.0.3" set_zonesigning "KEY1" "no" set_zonesigning "KEY2" "yes" # CSK (KEY1) DS and ZRRSIG will be removed, so it is UNRETENTIVE. -set_keystate "KEY1" "GOAL" "hidden" set_keystate "KEY1" "STATE_ZRRSIG" "unretentive" set_keystate "KEY1" "STATE_DS" "unretentive" # New CSK (KEY2) has its DS submitted, and is signing, so the DS and ZRRSIG @@ -3277,6 +3277,7 @@ set_keyalgorithm "KEY2" "13" "ECDSAP256SHA256" "256" set_keysigning "KEY2" "yes" set_zonesigning "KEY2" "no" # Key states. +set_keystate "KEY1" "GOAL" "hidden" set_keystate "KEY2" "GOAL" "omnipresent" set_keystate "KEY2" "STATE_DNSKEY" "rumoured" set_keystate "KEY2" "STATE_KRRSIG" "rumoured" @@ -3315,7 +3316,6 @@ set_policy "csk-roll2" "2" "3600" set_server "ns3" "10.53.0.3" # CSK (KEY1) DS and ZRRSIG will be removed, so it is UNRETENTIVE. set_zonesigning "KEY1" "no" -set_keystate "KEY1" "GOAL" "hidden" set_keystate "KEY1" "STATE_ZRRSIG" "unretentive" set_keystate "KEY1" "STATE_DS" "unretentive" # New CSK (KEY2) has its DS submitted, and is signing, so the DS and ZRRSIG diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 4f71ff1e04..aa4bd02457 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -128,3 +128,12 @@ Bug Fixes - ``named`` could crash with an assertion failure if the name of a database node was looked up while the database was being modified. [GL #1857] + +- Fix a bug in dnssec-policy keymgr where the check if a key has a + successor would return a false positive if any other key in the + keyring has a successor. [GL #1845] + +- With dnssec-policy, when creating a successor key, the goal state of + the current active key (the predecessor) was not changed and thus was + never is removed from the zone. [GL #1846] + diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c index 9a5480e24c..6ede7f975b 100644 --- a/lib/dns/keymgr.c +++ b/lib/dns/keymgr.c @@ -287,6 +287,10 @@ keymgr_prepublication_time(dns_dnsseckey_t *key, dns_kasp_t *kasp, /* * Publish successor 'prepub' time before the 'retire' time of 'key'. */ + if (prepub > retire) { + /* We should have already prepublished the new key. */ + return (now); + } return (retire - prepub); } @@ -633,11 +637,16 @@ keymgr_key_exists_with_state(dns_dnsseckeylist_t *keyring, dns_dnsseckey_t *key, * Check if a key has a successor. */ static bool -keymgr_key_has_successor(dns_dnsseckey_t *key, dns_dnsseckeylist_t *keyring) { - /* Don't worry about key states. */ - dst_key_state_t na[4] = { NA, NA, NA, NA }; - return (keymgr_key_exists_with_state(keyring, key, DST_KEY_DNSKEY, NA, - na, na, true, true)); +keymgr_key_has_successor(dns_dnsseckey_t *predecessor, + dns_dnsseckeylist_t *keyring) { + for (dns_dnsseckey_t *successor = ISC_LIST_HEAD(*keyring); + successor != NULL; successor = ISC_LIST_NEXT(successor, link)) + { + if (keymgr_key_is_successor(predecessor->key, successor->key)) { + return (true); + } + } + return (false); } /* @@ -1436,6 +1445,203 @@ keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) { } } +static isc_result_t +keymgr_key_rollover(dns_kasp_key_t *kaspkey, dns_dnsseckey_t *active_key, + dns_dnsseckeylist_t *keyring, dns_dnsseckeylist_t *newkeys, + const dns_name_t *origin, dns_rdataclass_t rdclass, + dns_kasp_t *kasp, uint32_t lifetime, isc_stdtime_t now, + isc_stdtime_t *nexttime, isc_mem_t *mctx) { + char keystr[DST_KEY_FORMATSIZE]; + isc_stdtime_t retire = 0, active = 0, prepub = 0; + dns_dnsseckey_t *new_key = NULL; + dns_dnsseckey_t *candidate = NULL; + dst_key_t *dst_key = NULL; + + /* Do we need to create a successor for the active key? */ + if (active_key != NULL) { + if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { + dst_key_format(active_key->key, keystr, sizeof(keystr)); + isc_log_write( + dns_lctx, DNS_LOGCATEGORY_DNSSEC, + DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), + "keymgr: DNSKEY %s (%s) is active in policy %s", + keystr, keymgr_keyrole(active_key->key), + dns_kasp_getname(kasp)); + } + + /* + * Calculate when the successor needs to be published + * in the zone. + */ + prepub = keymgr_prepublication_time(active_key, kasp, lifetime, + now); + if (prepub == 0 || prepub > now) { + if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { + dst_key_format(active_key->key, keystr, + sizeof(keystr)); + isc_log_write( + dns_lctx, DNS_LOGCATEGORY_DNSSEC, + DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), + "keymgr: new successor needed for " + "DNSKEY %s (%s) (policy %s) in %u " + "seconds", + keystr, keymgr_keyrole(active_key->key), + dns_kasp_getname(kasp), (prepub - now)); + } + + /* No need to start rollover now. */ + if (*nexttime == 0 || prepub < *nexttime) { + *nexttime = prepub; + } + return (ISC_R_SUCCESS); + } + + if (keymgr_key_has_successor(active_key, keyring)) { + /* Key already has successor. */ + if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { + dst_key_format(active_key->key, keystr, + sizeof(keystr)); + isc_log_write( + dns_lctx, DNS_LOGCATEGORY_DNSSEC, + DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), + "keymgr: key DNSKEY %s (%s) (policy " + "%s) already has successor", + keystr, keymgr_keyrole(active_key->key), + dns_kasp_getname(kasp)); + } + return (ISC_R_SUCCESS); + } + + if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { + dst_key_format(active_key->key, keystr, sizeof(keystr)); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC, + DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), + "keymgr: need successor for DNSKEY %s " + "(%s) (policy %s)", + keystr, keymgr_keyrole(active_key->key), + dns_kasp_getname(kasp)); + } + } else if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { + char namestr[DNS_NAME_FORMATSIZE]; + dns_name_format(origin, namestr, sizeof(namestr)); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC, + DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), + "keymgr: no active key found for %s (policy %s)", + namestr, dns_kasp_getname(kasp)); + } + + /* It is time to do key rollover, we need a new key. */ + + /* + * Check if there is a key available in pool because keys + * may have been pregenerated with dnssec-keygen. + */ + for (candidate = ISC_LIST_HEAD(*keyring); candidate != NULL; + candidate = ISC_LIST_NEXT(candidate, link)) + { + if (keymgr_dnsseckey_kaspkey_match(candidate, kaspkey) && + dst_key_is_unused(candidate->key)) + { + /* Found a candidate in keyring. */ + break; + } + } + + if (candidate == NULL) { + /* No key available in keyring, create a new one. */ + isc_result_t result = keymgr_createkey(kaspkey, origin, rdclass, + mctx, keyring, &dst_key); + if (result != ISC_R_SUCCESS) { + return (result); + } + dst_key_setttl(dst_key, dns_kasp_dnskeyttl(kasp)); + dst_key_settime(dst_key, DST_TIME_CREATED, now); + result = dns_dnsseckey_create(mctx, &dst_key, &new_key); + if (result != ISC_R_SUCCESS) { + return (result); + } + keymgr_key_init(new_key, kasp, now); + } else { + new_key = candidate; + } + dst_key_setnum(new_key->key, DST_NUM_LIFETIME, lifetime); + + /* Got a key. */ + if (active_key == NULL) { + /* + * If there is no active key found yet for this kasp + * key configuration, immediately make this key active. + */ + dst_key_settime(new_key->key, DST_TIME_PUBLISH, now); + dst_key_settime(new_key->key, DST_TIME_ACTIVATE, now); + keymgr_settime_syncpublish(new_key, kasp, true); + active = now; + } else { + /* + * This is a successor. Mark the relationship. + */ + isc_stdtime_t created; + (void)dst_key_gettime(new_key->key, DST_TIME_CREATED, &created); + + dst_key_setnum(new_key->key, DST_NUM_PREDECESSOR, + dst_key_id(active_key->key)); + dst_key_setnum(active_key->key, DST_NUM_SUCCESSOR, + dst_key_id(new_key->key)); + (void)dst_key_gettime(active_key->key, DST_TIME_INACTIVE, + &retire); + active = retire; + + /* + * If prepublication time and/or retire time are + * in the past (before the new key was created), use + * creation time as published and active time, + * effectively immediately making the key active. + */ + if (prepub < created) { + active += (created - prepub); + prepub = created; + } + if (active < created) { + active = created; + } + dst_key_settime(new_key->key, DST_TIME_PUBLISH, prepub); + dst_key_settime(new_key->key, DST_TIME_ACTIVATE, active); + keymgr_settime_syncpublish(new_key, kasp, false); + + /* + * Retire predecessor. + */ + dst_key_setstate(active_key->key, DST_KEY_GOAL, HIDDEN); + } + + /* This key wants to be present. */ + dst_key_setstate(new_key->key, DST_KEY_GOAL, OMNIPRESENT); + + /* Do we need to set retire time? */ + if (lifetime > 0) { + dst_key_settime(new_key->key, DST_TIME_INACTIVE, + (active + lifetime)); + keymgr_settime_remove(new_key, kasp); + } + + /* Append dnsseckey to list of new keys. */ + dns_dnssec_get_hints(new_key, now); + new_key->source = dns_keysource_repository; + INSIST(!new_key->legacy); + if (candidate == NULL) { + ISC_LIST_APPEND(*newkeys, new_key, link); + } + + /* Logging. */ + dst_key_format(new_key->key, keystr, sizeof(keystr)); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC, DNS_LOGMODULE_DNSSEC, + ISC_LOG_INFO, "keymgr: DNSKEY %s (%s) %s for policy %s", + keystr, keymgr_keyrole(new_key->key), + (candidate != NULL) ? "selected" : "created", + dns_kasp_getname(kasp)); + return (ISC_R_SUCCESS); +} + /* * Examine 'keys' and match 'kasp' policy. * @@ -1448,9 +1654,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass, isc_result_t result = ISC_R_SUCCESS; dns_dnsseckeylist_t newkeys; dns_kasp_key_t *kkey; - dns_dnsseckey_t *candidate = NULL; dns_dnsseckey_t *newkey = NULL; - dst_key_t *dst_key = NULL; isc_dir_t dir; bool dir_open = false; int options = (DST_TYPE_PRIVATE | DST_TYPE_PUBLIC | DST_TYPE_STATE); @@ -1460,6 +1664,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass, REQUIRE(keyring != NULL); ISC_LIST_INIT(newkeys); + isc_dir_init(&dir); if (directory == NULL) { directory = "."; @@ -1520,7 +1725,6 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass, for (kkey = ISC_LIST_HEAD(dns_kasp_keys(kasp)); kkey != NULL; kkey = ISC_LIST_NEXT(kkey, link)) { - isc_stdtime_t retire = 0, active = 0, prepub = 0; uint32_t lifetime = dns_kasp_key_lifetime(kkey); dns_dnsseckey_t *active_key = NULL; @@ -1597,163 +1801,10 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass, } } - /* Do we need to create a successor for the active key? */ - if (active_key != NULL) { - if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { - dst_key_format(active_key->key, keystr, - sizeof(keystr)); - isc_log_write( - dns_lctx, DNS_LOGCATEGORY_DNSSEC, - DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), - "keymgr: DNSKEY %s (%s) is active in " - "policy %s", - keystr, keymgr_keyrole(active_key->key), - dns_kasp_getname(kasp)); - } - - /* - * Calculate when the successor needs to be published - * in the zone. - */ - prepub = keymgr_prepublication_time(active_key, kasp, - lifetime, now); - if (prepub == 0 || prepub > now) { - /* No need to start rollover now. */ - if (*nexttime == 0 || prepub < *nexttime) { - *nexttime = prepub; - } - continue; - } - if (keymgr_key_has_successor(active_key, keyring)) { - /* Key already has successor. */ - continue; - } - - if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { - dst_key_format(active_key->key, keystr, - sizeof(keystr)); - isc_log_write( - dns_lctx, DNS_LOGCATEGORY_DNSSEC, - DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), - "keymgr: need successor for " - "DNSKEY %s (%s) (policy %s)", - keystr, keymgr_keyrole(active_key->key), - dns_kasp_getname(kasp)); - } - } else if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { - char namestr[DNS_NAME_FORMATSIZE]; - dns_name_format(origin, namestr, sizeof(namestr)); - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC, - DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1), - "keymgr: no active key found for %s " - "(policy %s)", - namestr, dns_kasp_getname(kasp)); - } - - /* It is time to do key rollover, we need a new key. */ - - /* - * Check if there is a key available in pool because keys - * may have been pregenerated with dnssec-keygen. - */ - for (candidate = ISC_LIST_HEAD(*keyring); candidate != NULL; - candidate = ISC_LIST_NEXT(candidate, link)) - { - if (keymgr_dnsseckey_kaspkey_match(candidate, kkey) && - dst_key_is_unused(candidate->key)) - { - /* Found a candidate in keyring. */ - break; - } - } - - if (candidate == NULL) { - /* No key available in keyring, create a new one. */ - RETERR(keymgr_createkey(kkey, origin, rdclass, mctx, - keyring, &dst_key)); - dst_key_setttl(dst_key, dns_kasp_dnskeyttl(kasp)); - dst_key_settime(dst_key, DST_TIME_CREATED, now); - RETERR(dns_dnsseckey_create(mctx, &dst_key, &newkey)); - keymgr_key_init(newkey, kasp, now); - } else { - newkey = candidate; - } - dst_key_setnum(newkey->key, DST_NUM_LIFETIME, lifetime); - - /* Got a key. */ - if (active_key == NULL) { - /* - * If there is no active key found yet for this kasp - * key configuration, immediately make this key active. - */ - dst_key_settime(newkey->key, DST_TIME_PUBLISH, now); - dst_key_settime(newkey->key, DST_TIME_ACTIVATE, now); - keymgr_settime_syncpublish(newkey, kasp, true); - active = now; - } else { - /* - * This is a successor. Mark the relationship. - */ - isc_stdtime_t created; - (void)dst_key_gettime(newkey->key, DST_TIME_CREATED, - &created); - - dst_key_setnum(newkey->key, DST_NUM_PREDECESSOR, - dst_key_id(active_key->key)); - dst_key_setnum(active_key->key, DST_NUM_SUCCESSOR, - dst_key_id(newkey->key)); - (void)dst_key_gettime(active_key->key, - DST_TIME_INACTIVE, &retire); - active = retire; - - /* - * If prepublication time and/or retire time are - * in the past (before the new key was created), use - * creation time as published and active time, - * effectively immediately making the key active. - */ - if (prepub < created) { - active += (created - prepub); - prepub = created; - } - if (active < created) { - active = created; - } - dst_key_settime(newkey->key, DST_TIME_PUBLISH, prepub); - dst_key_settime(newkey->key, DST_TIME_ACTIVATE, active); - keymgr_settime_syncpublish(newkey, kasp, false); - } - - /* This key wants to be present. */ - dst_key_setstate(newkey->key, DST_KEY_GOAL, OMNIPRESENT); - - /* Do we need to set retire time? */ - if (lifetime > 0) { - dst_key_settime(newkey->key, DST_TIME_INACTIVE, - (active + lifetime)); - keymgr_settime_remove(newkey, kasp); - } - - /* Append dnsseckey to list of new keys. */ - dns_dnssec_get_hints(newkey, now); - newkey->source = dns_keysource_repository; - INSIST(!newkey->legacy); - if (candidate == NULL) { - ISC_LIST_APPEND(newkeys, newkey, link); - } - - /* Logging. */ - dst_key_format(newkey->key, keystr, sizeof(keystr)); - isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC, - DNS_LOGMODULE_DNSSEC, ISC_LOG_INFO, - "keymgr: DNSKEY %s (%s) %s for policy %s", keystr, - keymgr_keyrole(newkey->key), - (candidate != NULL) ? "selected" : "created", - dns_kasp_getname(kasp)); - - /* Onwards to next kasp key configuration. */ - candidate = NULL; - newkey = NULL; + /* See if this key requires a rollover. */ + RETERR(keymgr_key_rollover(kkey, active_key, keyring, &newkeys, + origin, rdclass, kasp, lifetime, now, + nexttime, mctx)); } /* Walked all kasp key configurations. Append new keys. */ @@ -1779,7 +1830,6 @@ failure: isc_dir_close(&dir); } - INSIST(newkey == NULL); if (result != ISC_R_SUCCESS) { while ((newkey = ISC_LIST_HEAD(newkeys)) != NULL) { ISC_LIST_UNLINK(newkeys, newkey, link);