From 501dc87d75386d50bb713ae3c23c1122e6a5af57 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Wed, 10 Aug 2022 15:29:59 +0200 Subject: [PATCH] Wait with NSEC3 during a DNSSEC policy change When doing a dnssec-policy reconfiguration from a zone with NSEC only keys to a zone that uses NSEC3, figure out to wait with building the NSEC3 chain. Previously, BIND 9 would attempt to sign such a zone, but failed to do so because the NSEC3 chain conflicted with existing DNSKEY records in the zone that were not compatible with NSEC3. There exists logic for detecting such a case in the functions dnskey_sane() (in lib/dns/zone.c) and check_dnssec() (in lib/ns/update.c). Both functions look very similar so refactor them to use the same code and call the new function (called dns_zone_check_dnskey_nsec3()). Also update the dns_nsec_nseconly() function to take an additional parameter 'diff' that, if provided, will be checked whether an offending NSEC only DNSKEY will be deleted from the zone. If so, this key will not be considered when checking the zone for NSEC only DNSKEYs. This is needed to allow a transition from an NSEC zone with NSEC only DNSKEYs to an NSEC3 zone. --- bin/dnssec/dnssec-signzone.c | 2 +- lib/dns/include/dns/nsec.h | 7 +- lib/dns/include/dns/zone.h | 18 ++++ lib/dns/nsec.c | 34 ++++++- lib/dns/zone.c | 192 +++++++++++++++++++++++------------ lib/ns/update.c | 51 +--------- 6 files changed, 189 insertions(+), 115 deletions(-) diff --git a/bin/dnssec/dnssec-signzone.c b/bin/dnssec/dnssec-signzone.c index 889ca79ab5..1780a93ae8 100644 --- a/bin/dnssec/dnssec-signzone.c +++ b/bin/dnssec/dnssec-signzone.c @@ -3901,7 +3901,7 @@ main(int argc, char *argv[]) { hashlist_init(&hashlist, dns_db_nodecount(gdb, dns_dbtree_main) * 2, hash_length); - result = dns_nsec_nseconly(gdb, gversion, &answer); + result = dns_nsec_nseconly(gdb, gversion, NULL, &answer); if (result == ISC_R_NOTFOUND) { fprintf(stderr, "%s: warning: NSEC3 generation " diff --git a/lib/dns/include/dns/nsec.h b/lib/dns/include/dns/nsec.h index 04029e2ff6..e68ea35ebf 100644 --- a/lib/dns/include/dns/nsec.h +++ b/lib/dns/include/dns/nsec.h @@ -19,6 +19,7 @@ #include +#include #include #include @@ -60,11 +61,15 @@ dns_nsec_typepresent(dns_rdata_t *nsec, dns_rdatatype_t type); */ isc_result_t -dns_nsec_nseconly(dns_db_t *db, dns_dbversion_t *version, bool *answer); +dns_nsec_nseconly(dns_db_t *db, dns_dbversion_t *version, dns_diff_t *diff, + bool *answer); /* * Report whether the DNSKEY RRset has a NSEC only algorithm. Unknown * algorithms are assumed to support NSEC3. If DNSKEY is not found, * *answer is set to false, and ISC_R_NOTFOUND is returned. + * If 'diff' is provided, check if the NSEC only DNSKEY will be deleted. + * If so, and there are no other NSEC only DNSKEYs that will stay in 'db', + * consider the DNSKEY RRset to have no NSEC only DNSKEYs. * * Requires: * 'answer' to be non NULL. diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index c9a1487928..0f035cf5a8 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -2761,3 +2762,20 @@ dns_zone_gettid(dns_zone_t *zone); * * \return thread id associated with the zone */ + +bool +dns_zone_check_dnskey_nsec3(dns_zone_t *zone, dns_db_t *db, + dns_dbversion_t *ver, dns_diff_t *diff, + dst_key_t **keys, unsigned int numkeys); +/**< + * Return whether the zone would enter an inconsistent state where NSEC only + * DNSKEYs are present along NSEC3 chains. + * + * Requires: + * \li 'zone' to be a valid zone. + * \li 'db'is not NULL. + * + * Returns: + * \li 'true' if the check passes, that is the zone remains consistent, + * 'false' if the zone would have NSEC only DNSKEYs and an NSEC3 chain. + */ diff --git a/lib/dns/nsec.c b/lib/dns/nsec.c index afbe9c80f9..84c47d6c21 100644 --- a/lib/dns/nsec.c +++ b/lib/dns/nsec.c @@ -247,7 +247,8 @@ dns_nsec_typepresent(dns_rdata_t *nsec, dns_rdatatype_t type) { } isc_result_t -dns_nsec_nseconly(dns_db_t *db, dns_dbversion_t *version, bool *answer) { +dns_nsec_nseconly(dns_db_t *db, dns_dbversion_t *version, dns_diff_t *diff, + bool *answer) { dns_dbnode_t *node = NULL; dns_rdataset_t rdataset; dns_rdata_dnskey_t dnskey; @@ -282,8 +283,35 @@ dns_nsec_nseconly(dns_db_t *db, dns_dbversion_t *version, bool *answer) { RUNTIME_CHECK(result == ISC_R_SUCCESS); if (dnskey.algorithm == DST_ALG_RSAMD5 || - dnskey.algorithm == DST_ALG_RSASHA1) { - break; + dnskey.algorithm == DST_ALG_DH || + dnskey.algorithm == DST_ALG_DSA || + dnskey.algorithm == DST_ALG_RSASHA1) + { + bool deleted = false; + if (diff != NULL) { + for (dns_difftuple_t *tuple = + ISC_LIST_HEAD(diff->tuples); + tuple != NULL; + tuple = ISC_LIST_NEXT(tuple, link)) + { + if (tuple->rdata.type != + dns_rdatatype_dnskey || + tuple->op != DNS_DIFFOP_DEL) { + continue; + } + + if (dns_rdata_compare( + &rdata, &tuple->rdata) == 0) + { + deleted = true; + break; + } + } + } + + if (!deleted) { + break; + } } } dns_rdataset_disassociate(&rdataset); diff --git a/lib/dns/zone.c b/lib/dns/zone.c index d932b1b841..600fad94e5 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -3726,7 +3726,7 @@ zone_addnsec3chain(dns_zone_t *zone, dns_rdata_nsec3param_t *nsec3param) { * latter to exist in the first place. */ dns_db_currentversion(db, &version); - result = dns_nsec_nseconly(db, version, &nseconly); + result = dns_nsec_nseconly(db, version, NULL, &nseconly); nsec3ok = (result == ISC_R_SUCCESS && !nseconly); dns_db_closeversion(db, &version, false); if (!nsec3ok && (nsec3param->flags & DNS_NSEC3FLAG_REMOVE) == 0) { @@ -3918,7 +3918,7 @@ resume_addnsec3chain(dns_zone_t *zone) { * In order to create NSEC3 chains we need the DNSKEY RRset at zone * apex to exist and contain no keys using NSEC-only algorithms. */ - result = dns_nsec_nseconly(db, version, &nseconly); + result = dns_nsec_nseconly(db, version, NULL, &nseconly); nsec3ok = (result == ISC_R_SUCCESS && !nseconly); /* @@ -8186,7 +8186,7 @@ try_private: goto add; } - result = dns_nsec_nseconly(db, ver, &nseconly); + result = dns_nsec_nseconly(db, ver, diff, &nseconly); nsec3ok = (result == ISC_R_SUCCESS && !nseconly); /* @@ -9508,6 +9508,105 @@ failure: return (result); } +/* + * Prevent the zone entering a inconsistent state where + * NSEC only DNSKEYs are present with NSEC3 chains. + */ +bool +dns_zone_check_dnskey_nsec3(dns_zone_t *zone, dns_db_t *db, + dns_dbversion_t *ver, dns_diff_t *diff, + dst_key_t **keys, unsigned int numkeys) { + uint8_t alg; + dns_rdatatype_t privatetype; + ; + bool nseconly = false, nsec3 = false; + isc_result_t result; + + REQUIRE(DNS_ZONE_VALID(zone)); + REQUIRE(db != NULL); + + privatetype = dns_zone_getprivatetype(zone); + + /* Scan the tuples for an NSEC-only DNSKEY */ + if (diff != NULL) { + for (dns_difftuple_t *tuple = ISC_LIST_HEAD(diff->tuples); + tuple != NULL; tuple = ISC_LIST_NEXT(tuple, link)) + { + if (nseconly && nsec3) { + break; + } + + if (tuple->op != DNS_DIFFOP_ADD) { + continue; + } + + if (tuple->rdata.type == dns_rdatatype_nsec3param) { + nsec3 = true; + } + + if (tuple->rdata.type != dns_rdatatype_dnskey) { + continue; + } + + alg = tuple->rdata.data[3]; + if (alg == DNS_KEYALG_RSAMD5 || alg == DNS_KEYALG_DH || + alg == DNS_KEYALG_DSA || alg == DNS_KEYALG_RSASHA1) + { + nseconly = true; + } + } + } + /* Scan the zone keys for an NSEC-only DNSKEY */ + if (keys != NULL && !nseconly) { + for (unsigned int i = 0; i < numkeys; i++) { + alg = dst_key_alg(keys[i]); + if (alg == DNS_KEYALG_RSAMD5 || alg == DNS_KEYALG_DH || + alg == DNS_KEYALG_DSA || alg == DNS_KEYALG_RSASHA1) + { + nseconly = true; + break; + } + } + } + + /* Check DB for NSEC-only DNSKEY */ + if (!nseconly) { + result = dns_nsec_nseconly(db, ver, diff, &nseconly); + /* + * Adding an NSEC3PARAM record can proceed without a + * DNSKEY (it will trigger a delayed change), so we can + * ignore ISC_R_NOTFOUND here. + */ + if (result == ISC_R_NOTFOUND) { + result = ISC_R_SUCCESS; + } + CHECK(result); + } + + /* Check existing DB for NSEC3 */ + if (!nsec3) { + CHECK(dns_nsec3_activex(db, ver, false, privatetype, &nsec3)); + } + + /* Check kasp for NSEC3PARAM settings */ + if (!nsec3) { + dns_kasp_t *kasp = dns_zone_getkasp(zone); + if (kasp != NULL) { + nsec3 = dns_kasp_nsec3(kasp); + } + } + + /* Refuse to allow NSEC3 with NSEC-only keys */ + if (nseconly && nsec3) { + goto failure; + } + + return (true); + +failure: + return (false); +} + /* * Incrementally sign the zone using the keys requested. * Builds the NSEC chain if required. @@ -9643,6 +9742,15 @@ zone_sign(dns_zone_t *zone) { /* Determine which type of chain to build */ if (use_kasp) { build_nsec3 = dns_kasp_nsec3(kasp); + if (!dns_zone_check_dnskey_nsec3(zone, db, version, NULL, + (dst_key_t **)&zone_keys, + nkeys)) + { + dnssec_log(zone, ISC_LOG_INFO, + "wait building NSEC3 chain until NSEC only " + "DNSKEYs are removed"); + build_nsec3 = false; + } build_nsec = !build_nsec3; } else { CHECK(dns_private_chains(db, version, zone->privatetype, @@ -20506,63 +20614,6 @@ failure: return (result); } -/* - * Prevent the zone entering a inconsistent state where - * NSEC only DNSKEYs are present with NSEC3 chains. - * See update.c:check_dnssec() - */ -static bool -dnskey_sane(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, - dns_diff_t *diff) { - isc_result_t result; - dns_difftuple_t *tuple; - bool nseconly = false, nsec3 = false; - dns_rdatatype_t privatetype = dns_zone_getprivatetype(zone); - - /* Scan the tuples for an NSEC-only DNSKEY */ - for (tuple = ISC_LIST_HEAD(diff->tuples); tuple != NULL; - tuple = ISC_LIST_NEXT(tuple, link)) - { - uint8_t alg; - if (tuple->rdata.type != dns_rdatatype_dnskey || - tuple->op != DNS_DIFFOP_ADD) { - continue; - } - - alg = tuple->rdata.data[3]; - if (alg == DST_ALG_RSASHA1) { - nseconly = true; - break; - } - } - - /* Check existing DB for NSEC-only DNSKEY */ - if (!nseconly) { - result = dns_nsec_nseconly(db, ver, &nseconly); - if (result == ISC_R_NOTFOUND) { - result = ISC_R_SUCCESS; - } - CHECK(result); - } - - /* Check existing DB for NSEC3 */ - if (!nsec3) { - CHECK(dns_nsec3_activex(db, ver, false, privatetype, &nsec3)); - } - - /* Refuse to allow NSEC3 with NSEC-only keys */ - if (nseconly && nsec3) { - dnssec_log(zone, ISC_LOG_ERROR, - "NSEC only DNSKEYs and NSEC3 chains not allowed"); - goto failure; - } - - return (true); - -failure: - return (false); -} - static isc_result_t clean_nsec3param(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, dns_diff_t *diff) { @@ -21558,6 +21609,7 @@ zone_rekey(dns_zone_t *zone) { if (result == ISC_R_SUCCESS) { bool cdsdel = false; bool cdnskeydel = false; + bool sane_diff, sane_dnskey; isc_stdtime_t when; /* @@ -21726,9 +21778,21 @@ zone_rekey(dns_zone_t *zone) { } } - if ((newactive || fullsign || !ISC_LIST_EMPTY(diff.tuples)) && - dnskey_sane(zone, db, ver, &diff)) - { + /* + * A sane diff is one that is not empty, and that does not + * introduce a zone with NSEC only DNSKEYs along with NSEC3 + * chains. + */ + sane_dnskey = dns_zone_check_dnskey_nsec3(zone, db, ver, &diff, + NULL, 0); + sane_diff = !ISC_LIST_EMPTY(diff.tuples) && sane_dnskey; + if (!sane_dnskey) { + dnssec_log(zone, ISC_LOG_ERROR, + "NSEC only DNSKEYs and NSEC3 chains not " + "allowed"); + } + + if (newactive || fullsign || sane_diff) { CHECK(dns_diff_apply(&diff, db, ver)); CHECK(clean_nsec3param(zone, db, ver, &diff)); CHECK(add_signing_records(db, zone->privatetype, ver, @@ -22913,7 +22977,7 @@ rss_post(dns_zone_t *zone, isc_event_t *event) { dns_rdata_init(&rdata); np->data[2] |= DNS_NSEC3FLAG_CREATE; - result = dns_nsec_nseconly(db, newver, &nseconly); + result = dns_nsec_nseconly(db, newver, NULL, &nseconly); if (result == ISC_R_NOTFOUND || nseconly) { np->data[2] |= DNS_NSEC3FLAG_INITIAL; } diff --git a/lib/ns/update.c b/lib/ns/update.c index 28b59a5484..4b32b7508f 100644 --- a/lib/ns/update.c +++ b/lib/ns/update.c @@ -2096,56 +2096,12 @@ failure: static isc_result_t check_dnssec(ns_client_t *client, dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, dns_diff_t *diff) { - dns_difftuple_t *tuple; - bool nseconly = false, nsec3 = false; isc_result_t result; unsigned int iterations = 0; dns_rdatatype_t privatetype = dns_zone_getprivatetype(zone); - /* Scan the tuples for an NSEC-only DNSKEY or an NSEC3PARAM */ - for (tuple = ISC_LIST_HEAD(diff->tuples); tuple != NULL; - tuple = ISC_LIST_NEXT(tuple, link)) - { - if (tuple->op != DNS_DIFFOP_ADD) { - continue; - } - - if (tuple->rdata.type == dns_rdatatype_dnskey) { - uint8_t alg; - alg = tuple->rdata.data[3]; - if (alg == DST_ALG_RSASHA1) { - nseconly = true; - break; - } - } else if (tuple->rdata.type == dns_rdatatype_nsec3param) { - nsec3 = true; - break; - } - } - - /* Check existing DB for NSEC-only DNSKEY */ - if (!nseconly) { - result = dns_nsec_nseconly(db, ver, &nseconly); - - /* - * An NSEC3PARAM update can proceed without a DNSKEY (it - * will trigger a delayed change), so we can ignore - * ISC_R_NOTFOUND here. - */ - if (result == ISC_R_NOTFOUND) { - result = ISC_R_SUCCESS; - } - - CHECK(result); - } - - /* Check existing DB for NSEC3 */ - if (!nsec3) { - CHECK(dns_nsec3_activex(db, ver, false, privatetype, &nsec3)); - } - /* Refuse to allow NSEC3 with NSEC-only keys */ - if (nseconly && nsec3) { + if (!dns_zone_check_dnskey_nsec3(zone, db, ver, diff, NULL, 0)) { update_log(client, zone, ISC_LOG_ERROR, "NSEC only DNSKEYs and NSEC3 chains not allowed"); result = DNS_R_REFUSED; @@ -2346,8 +2302,11 @@ add_nsec3param_records(ns_client_t *client, dns_zone_t *zone, dns_db_t *db, * supporting an NSEC3 chain, then we set the * INITIAL flag to indicate that these parameters * are to be used later. + * + * Don't provide a 'diff' here because we want to + * know the capability of the current database. */ - result = dns_nsec_nseconly(db, ver, &nseconly); + result = dns_nsec_nseconly(db, ver, NULL, &nseconly); if (result == ISC_R_NOTFOUND || nseconly) { buf[2] |= DNS_NSEC3FLAG_INITIAL; }