Merge branch '1611-detect-insane-dnssec-policies' into 'main'

Add some dnssec-policy configuration checks

Closes #1611 and #1601

See merge request isc-projects/bind9!6273

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]
 

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";
+};

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";
+};

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";
+};
+

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

doc/arm/reference.rst

@@ -5351,7 +5351,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

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
 ~~~~~~~~~

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;

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",
+	sigrefresh = get_duration(maps, "signatures-refresh",
-						  DNS_KASP_SIG_REFRESH));
+				  DNS_KASP_SIG_REFRESH);
-	dns_kasp_setsigvalidity(kasp, get_duration(maps, "signatures-validity",
+	dns_kasp_setsigrefresh(kasp, sigrefresh);
-						   DNS_KASP_SIG_VALIDITY));
+
-	dns_kasp_setsigvalidity_dnskey(
+	sigvalidity = get_duration(maps, "signatures-validity-dnskey",
-		kasp, get_duration(maps, "signatures-validity-dnskey",
+				   DNS_KASP_SIG_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 |
+			if (role[i] == 0) {
-							DNS_KASP_KEY_ROLE_KSK))
+				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;
 		}