diff --git a/CHANGES b/CHANGES index 35fe7239a8..5c8236d6f9 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5896. [func] Add some more dnssec-policy checks to detect weird + policies. [GL #1611] + 5895. [test] Add new set of unit test macros and move the unit tests under single namespace in /tests/. [GL !6243] diff --git a/bin/tests/system/checkconf/kasp-bad-lifetime.conf b/bin/tests/system/checkconf/kasp-bad-lifetime.conf new file mode 100644 index 0000000000..225b38690c --- /dev/null +++ b/bin/tests/system/checkconf/kasp-bad-lifetime.conf @@ -0,0 +1,91 @@ +/* + * 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 "bad-lifetime-ksk" { + /* + * The KSK lifetime is too short. + * The ZSK lifetime is good enough but should trigger a warning. + */ + keys { + ksk lifetime PT3H algorithm 13; + zsk lifetime P8DT2H1S algorithm 13; + }; + + dnskey-ttl PT1H; + publish-safety PT1H; + retire-safety PT1H; + zone-propagation-delay PT1H; + max-zone-ttl P1D; + signatures-validity P10D; + signatures-refresh P3D; + parent-ds-ttl PT1H; + parent-propagation-delay PT5M; +}; + +dnssec-policy "bad-lifetime-zsk" { + /* + * The ZSK lifetime is too short. + * The KSK lifetime is good enough but should trigger a warning. + */ + keys { + ksk lifetime PT3H1S algorithm 13; + zsk lifetime P8DT2H algorithm 13; + }; + + dnskey-ttl PT1H; + publish-safety PT1H; + retire-safety PT1H; + zone-propagation-delay PT1H; + max-zone-ttl P1D; + signatures-validity P10D; + signatures-refresh P3D; + parent-ds-ttl PT1H; + parent-propagation-delay PT5M; +}; + +dnssec-policy "bad-lifetime-csk" { + /* + * The CSK lifetime is too short. + */ + keys { + csk lifetime PT3H algorithm 13; + }; + + dnskey-ttl PT1H; + publish-safety PT1H; + retire-safety PT1H; + zone-propagation-delay PT1H; + max-zone-ttl P1D; + signatures-validity P10D; + signatures-refresh P3D; + parent-ds-ttl PT1H; + parent-propagation-delay PT5M; +}; + +zone "bad-lifetime-ksk.example.net" { + type primary; + file "bad-lifetime-ksk.example.db"; + dnssec-policy "bad-lifetime-ksk"; +}; + +zone "bad-lifetime-zsk.example.net" { + type primary; + file "bad-lifetime-zsk.example.db"; + dnssec-policy "bad-lifetime-zsk"; +}; + +zone "bad-lifetime-csk.example.net" { + type primary; + file "bad-lifetime-csk.example.db"; + dnssec-policy "bad-lifetime-csk"; +}; diff --git a/bin/tests/system/checkconf/kasp-bad-signatures-refresh.conf b/bin/tests/system/checkconf/kasp-bad-signatures-refresh.conf new file mode 100644 index 0000000000..dd907dddd2 --- /dev/null +++ b/bin/tests/system/checkconf/kasp-bad-signatures-refresh.conf @@ -0,0 +1,44 @@ +/* + * 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 "bad-sigrefresh" { + keys { + csk lifetime unlimited algorithm 13; + }; + + signatures-validity P10D; + signatures-validity-dnskey P20D; + signatures-refresh P9DT1S; +}; + +dnssec-policy "bad-sigrefresh-dnskey" { + keys { + csk lifetime unlimited algorithm 13; + }; + + signatures-validity P20D; + signatures-validity-dnskey P10D; + signatures-refresh P9DT1S; +}; + +zone "sigrefresh.example.net" { + type primary; + file "sigrefresh.example.db"; + dnssec-policy "bad-sigrefresh"; +}; + +zone "dnskey.example.net" { + type primary; + file "dnskey.example.db"; + dnssec-policy "bad-sigrefresh-dnskey"; +}; diff --git a/bin/tests/system/checkconf/kasp-warning.conf b/bin/tests/system/checkconf/kasp-warning.conf new file mode 100644 index 0000000000..4c05b5ad02 --- /dev/null +++ b/bin/tests/system/checkconf/kasp-warning.conf @@ -0,0 +1,59 @@ +/* + * 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 "warn1" { + keys { + // This policy has keys in the same algorithm with the same + // role, this should trigger a warning. + ksk lifetime unlimited algorithm ecdsa256; + zsk lifetime unlimited algorithm ecdsa256; + zsk lifetime unlimited algorithm ecdsa256; + ksk lifetime unlimited algorithm ecdsa256; + }; +}; + +dnssec-policy "warn2" { + keys { + // This policy has keys in the same algorithm with the same + // role, this should trigger a warning. + csk lifetime unlimited algorithm rsasha256; + ksk lifetime unlimited algorithm rsasha256; + zsk lifetime unlimited algorithm rsasha256; + }; +}; + +dnssec-policy "warn3" { + keys { + // This policy has a key with a very short lifetime. + csk lifetime PT2591999S algorithm rsasha256; + }; +}; + +zone "warn1.example.net" { + type primary; + file "warn1.example.db"; + dnssec-policy "warn1"; +}; + +zone "warn2.example.net" { + type primary; + file "warn2.example.db"; + dnssec-policy "warn2"; +}; + +zone "warn3.example.net" { + type primary; + file "warn3.example.db"; + dnssec-policy "warn3"; +}; + diff --git a/bin/tests/system/checkconf/tests.sh b/bin/tests/system/checkconf/tests.sh index 5c159924c7..e1131dab51 100644 --- a/bin/tests/system/checkconf/tests.sh +++ b/bin/tests/system/checkconf/tests.sh @@ -528,6 +528,26 @@ grep "dnssec-policy: key with algorithm rsasha1 has invalid key length 511" < ch if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` +n=`expr $n + 1` +echo_i "checking named-checkconf kasp signatures refresh errors ($n)" +ret=0 +$CHECKCONF kasp-bad-signatures-refresh.conf > checkconf.out$n 2>&1 && ret=1 +grep "dnssec-policy: policy 'bad-sigrefresh' signatures-refresh must be at most 90% of the signatures-validity" < checkconf.out$n > /dev/null || ret=1 +grep "dnssec-policy: policy 'bad-sigrefresh-dnskey' signatures-refresh must be at most 90% of the signatures-validity-dnskey" < checkconf.out$n > /dev/null || ret=1 +lines=$(wc -l < "checkconf.out$n") +if [ $lines != 2 ]; then ret=1; fi +if [ $ret != 0 ]; then echo_i "failed"; fi +status=`expr $status + $ret` + +n=`expr $n + 1` +echo_i "checking named-checkconf kasp key lifetime errors ($n)" +ret=0 +$CHECKCONF kasp-bad-lifetime.conf > checkconf.out$n 2>&1 && ret=1 +lines=$(grep "dnssec-policy: key lifetime is shorter than the time it takes to do a rollover" < checkconf.out$n | wc -l) || ret=1 +if [ $lines != 3 ]; then ret=1; fi +if [ $ret != 0 ]; then echo_i "failed"; fi +status=`expr $status + $ret` + n=`expr $n + 1` echo_i "checking named-checkconf kasp predefined key length ($n)" ret=0 @@ -536,6 +556,20 @@ grep "dnssec-policy: key algorithm ecdsa256 has predefined length; ignoring leng if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` +n=`expr $n + 1` +echo_i "checking named-checkconf kasp warns about weird policies ($n)" +ret=0 +$CHECKCONF kasp-warning.conf > checkconf.out$n 2>&1 || ret=1 +grep "dnssec-policy: algorithm 8 has multiple keys with ZSK role" < checkconf.out$n > /dev/null || ret=1 +grep "dnssec-policy: algorithm 8 has multiple keys with ZSK role" < checkconf.out$n > /dev/null || ret=1 +grep "dnssec-policy: algorithm 13 has multiple keys with KSK role" < checkconf.out$n > /dev/null || ret=1 +grep "dnssec-policy: algorithm 13 has multiple keys with ZSK role" < checkconf.out$n > /dev/null || ret=1 +grep "dnssec-policy: key lifetime is shorter than 30 days" < checkconf.out$n > /dev/null || ret=1 +lines=$(wc -l < "checkconf.out$n") +if [ $lines != 5 ]; then ret=1; fi +if [ $ret != 0 ]; then echo_i "failed"; fi +status=`expr $status + $ret` + n=`expr $n + 1` echo_i "check that a good 'kasp' configuration is accepted ($n)" ret=0 diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 0d6d23f7bf..a21cef9c2a 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -5357,7 +5357,9 @@ The following options can be specified in a ``dnssec-policy`` statement: refreshed. The signature is renewed when the time until the expiration time is less than the specified interval. The default is ``P5D`` (5 days), meaning signatures that expire in 5 days or sooner - are refreshed. + are refreshed. The ``signatures-refresh`` value must be less than + 90% of the minimum value of ``signatures-validity`` and + ``signatures-validity-dnskey``. ``signatures-validity`` This indicates the validity period of an RRSIG record (subject to diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index e786ef5e99..3a6ddd8b5d 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -35,7 +35,9 @@ Removed Features Feature Changes ~~~~~~~~~~~~~~~ -- None. +- Some more ``dnssec-policy`` configuration checks have been added to + detect weird policies such as missing KSK and/or ZSK, and too short + key lifetimes and re-sign periods. :gl:`#1611`. Bug Fixes ~~~~~~~~~ diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c index ddd9a9a583..990c442ca1 100644 --- a/lib/dns/keymgr.c +++ b/lib/dns/keymgr.c @@ -1669,7 +1669,7 @@ keymgr_key_rollover(dns_kasp_key_t *kaspkey, dns_dnsseckey_t *active_key, */ prepub = keymgr_prepublication_time(active_key, kasp, lifetime, now); - if (prepub == 0 || prepub > now) { + if (prepub > now) { if (isc_log_wouldlog(dns_lctx, ISC_LOG_DEBUG(1))) { dst_key_format(active_key->key, keystr, sizeof(keystr)); @@ -1682,7 +1682,8 @@ keymgr_key_rollover(dns_kasp_key_t *kaspkey, dns_dnsseckey_t *active_key, keystr, keymgr_keyrole(active_key->key), dns_kasp_getname(kasp), (prepub - now)); } - + } + if (prepub == 0 || prepub > now) { /* No need to start rollover now. */ if (*nexttime == 0 || prepub < *nexttime) { *nexttime = prepub; diff --git a/lib/isccfg/kaspconf.c b/lib/isccfg/kaspconf.c index a8a078f0c7..d7a01ccd04 100644 --- a/lib/isccfg/kaspconf.c +++ b/lib/isccfg/kaspconf.c @@ -72,7 +72,8 @@ get_duration(const cfg_obj_t **maps, const char *option, uint32_t dfl) { */ static isc_result_t cfg_kaspkey_fromconfig(const cfg_obj_t *config, dns_kasp_t *kasp, - isc_log_t *logctx) { + isc_log_t *logctx, uint32_t ksk_min_lifetime, + uint32_t zsk_min_lifetime) { isc_result_t result; dns_kasp_key_t *key = NULL; @@ -92,6 +93,7 @@ cfg_kaspkey_fromconfig(const cfg_obj_t *config, dns_kasp_t *kasp, const char *rolestr = NULL; const cfg_obj_t *obj = NULL; isc_consttextregion_t alg; + bool error = false; rolestr = cfg_obj_asstring(cfg_tuple_get(config, "role")); if (strcmp(rolestr, "ksk") == 0) { @@ -108,6 +110,31 @@ cfg_kaspkey_fromconfig(const cfg_obj_t *config, dns_kasp_t *kasp, if (cfg_obj_isduration(obj)) { key->lifetime = cfg_obj_asduration(obj); } + if (key->lifetime > 0) { + if (key->lifetime < 30 * (24 * 3600)) { + cfg_obj_log(obj, logctx, ISC_LOG_WARNING, + "dnssec-policy: key lifetime is " + "shorter than 30 days"); + } + if ((key->role & DNS_KASP_KEY_ROLE_KSK) != 0 && + key->lifetime <= ksk_min_lifetime) + { + error = true; + } + if ((key->role & DNS_KASP_KEY_ROLE_ZSK) != 0 && + key->lifetime <= zsk_min_lifetime) + { + error = true; + } + if (error) { + cfg_obj_log(obj, logctx, ISC_LOG_ERROR, + "dnssec-policy: key lifetime is " + "shorter than the time it takes to " + "do a rollover"); + result = ISC_R_FAILURE; + goto cleanup; + } + } obj = cfg_tuple_get(config, "algorithm"); alg.base = cfg_obj_asstring(obj); @@ -263,6 +290,9 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx, const char *kaspname = NULL; dns_kasp_t *kasp = NULL; size_t i = 0; + uint32_t sigrefresh = 0, sigvalidity = 0; + uint32_t ipub = 0, iret = 0; + uint32_t ksk_min_lifetime = 0, zsk_min_lifetime = 0; REQUIRE(kaspp != NULL && *kaspp == NULL); @@ -303,13 +333,36 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx, maps[i] = NULL; /* Configuration: Signatures */ - dns_kasp_setsigrefresh(kasp, get_duration(maps, "signatures-refresh", - DNS_KASP_SIG_REFRESH)); - dns_kasp_setsigvalidity(kasp, get_duration(maps, "signatures-validity", - DNS_KASP_SIG_VALIDITY)); - dns_kasp_setsigvalidity_dnskey( - kasp, get_duration(maps, "signatures-validity-dnskey", - DNS_KASP_SIG_VALIDITY_DNSKEY)); + sigrefresh = get_duration(maps, "signatures-refresh", + DNS_KASP_SIG_REFRESH); + dns_kasp_setsigrefresh(kasp, sigrefresh); + + sigvalidity = get_duration(maps, "signatures-validity-dnskey", + DNS_KASP_SIG_VALIDITY_DNSKEY); + if (sigrefresh >= (sigvalidity * 0.9)) { + cfg_obj_log( + config, logctx, ISC_LOG_ERROR, + "dnssec-policy: policy '%s' signatures-refresh must be " + "at most 90%% of the signatures-validity-dnskey", + kaspname); + result = ISC_R_FAILURE; + } + dns_kasp_setsigvalidity_dnskey(kasp, sigvalidity); + + sigvalidity = get_duration(maps, "signatures-validity", + DNS_KASP_SIG_VALIDITY); + if (sigrefresh >= (sigvalidity * 0.9)) { + cfg_obj_log(config, logctx, ISC_LOG_ERROR, + "dnssec-policy: policy '%s' signatures-refresh " + "must be at most 90%% of the signatures-validity", + kaspname); + result = ISC_R_FAILURE; + } + dns_kasp_setsigvalidity(kasp, sigvalidity); + + if (result != ISC_R_SUCCESS) { + goto cleanup; + } /* Configuration: Keys */ dns_kasp_setdnskeyttl( @@ -321,16 +374,39 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx, dns_kasp_setpurgekeys( kasp, get_duration(maps, "purge-keys", DNS_KASP_PURGE_KEYS)); + ipub = get_duration(maps, "dnskey-ttl", DNS_KASP_KEY_TTL) + + get_duration(maps, "publish-safety", DNS_KASP_PUBLISH_SAFETY) + + get_duration(maps, "zone-propagation-delay", + DNS_KASP_ZONE_PROPDELAY); + + iret = get_duration(maps, "parent-ds-ttl", DNS_KASP_DS_TTL) + + get_duration(maps, "retire-safety", DNS_KASP_RETIRE_SAFETY) + + get_duration(maps, "parent-propagation-delay", + DNS_KASP_PARENT_PROPDELAY); + + ksk_min_lifetime = ISC_MAX(ipub, iret); + + iret = (sigvalidity - sigrefresh) + + get_duration(maps, "max-zone-ttl", DNS_KASP_ZONE_MAXTTL) + + get_duration(maps, "retire-safety", DNS_KASP_RETIRE_SAFETY) + + get_duration(maps, "zone-propagation-delay", + DNS_KASP_ZONE_PROPDELAY); + + zsk_min_lifetime = ISC_MAX(ipub, iret); + (void)confget(maps, "keys", &keys); if (keys != NULL) { char role[256] = { 0 }; + bool warn[256][2] = { { false } }; dns_kasp_key_t *kkey = NULL; for (element = cfg_list_first(keys); element != NULL; element = cfg_list_next(element)) { cfg_obj_t *kobj = cfg_listelt_value(element); - result = cfg_kaspkey_fromconfig(kobj, kasp, logctx); + result = cfg_kaspkey_fromconfig(kobj, kasp, logctx, + ksk_min_lifetime, + zsk_min_lifetime); if (result != ISC_R_SUCCESS) { goto cleanup; } @@ -344,24 +420,46 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx, INSIST(keyalg < ARRAY_SIZE(role)); if (dns_kasp_key_zsk(kkey)) { + if ((role[keyalg] & DNS_KASP_KEY_ROLE_ZSK) != 0) + { + warn[keyalg][0] = true; + } role[keyalg] |= DNS_KASP_KEY_ROLE_ZSK; } if (dns_kasp_key_ksk(kkey)) { + if ((role[keyalg] & DNS_KASP_KEY_ROLE_KSK) != 0) + { + warn[keyalg][1] = true; + } role[keyalg] |= DNS_KASP_KEY_ROLE_KSK; } } dns_kasp_thaw(kasp); for (i = 0; i < ARRAY_SIZE(role); i++) { - if (role[i] != 0 && role[i] != (DNS_KASP_KEY_ROLE_ZSK | - DNS_KASP_KEY_ROLE_KSK)) - { + if (role[i] == 0) { + continue; + } + if (role[i] != + (DNS_KASP_KEY_ROLE_ZSK | DNS_KASP_KEY_ROLE_KSK)) { cfg_obj_log(keys, logctx, ISC_LOG_ERROR, "dnssec-policy: algorithm %zu " "requires both KSK and ZSK roles", i); result = ISC_R_FAILURE; } + if (warn[i][0]) { + cfg_obj_log(keys, logctx, ISC_LOG_WARNING, + "dnssec-policy: algorithm %zu has " + "multiple keys with ZSK role", + i); + } + if (warn[i][1]) { + cfg_obj_log(keys, logctx, ISC_LOG_WARNING, + "dnssec-policy: algorithm %zu has " + "multiple keys with KSK role", + i); + } } if (result != ISC_R_SUCCESS) { goto cleanup; @@ -372,7 +470,7 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx, INSIST(dns_kasp_keylist_empty(kasp)); } else { /* No keys clause configured, use the "default". */ - result = cfg_kaspkey_fromconfig(NULL, kasp, logctx); + result = cfg_kaspkey_fromconfig(NULL, kasp, logctx, 0, 0); if (result != ISC_R_SUCCESS) { goto cleanup; }