From 58690ec11cd26b985e5d669f4f06c74ad5fb7eb7 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 6 May 2022 16:08:39 +0200
Subject: [PATCH 1/7] Warn if multiple keys have same role

If a dnssec-policy has multiple keys configured with the
same algorithm and role.

(cherry picked from commit f54dad005e02bd40d353037f562a695f53ed19c0)
---
 bin/tests/system/checkconf/kasp-warning.conf | 46 ++++++++++++++++++++
 bin/tests/system/checkconf/tests.sh          | 13 ++++++
 lib/isccfg/kaspconf.c                        | 29 ++++++++++--
 3 files changed, 85 insertions(+), 3 deletions(-)
 create mode 100644 bin/tests/system/checkconf/kasp-warning.conf

diff --git a/bin/tests/system/checkconf/kasp-warning.conf b/bin/tests/system/checkconf/kasp-warning.conf
new file mode 100644
index 0000000000..765c09b14a
--- /dev/null
+++ b/bin/tests/system/checkconf/kasp-warning.conf
@@ -0,0 +1,46 @@
+/*
+ * 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;
+	};
+};
+
+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";
+};
+
diff --git a/bin/tests/system/checkconf/tests.sh b/bin/tests/system/checkconf/tests.sh
index 5c159924c7..807c79b5dc 100644
--- a/bin/tests/system/checkconf/tests.sh
+++ b/bin/tests/system/checkconf/tests.sh
@@ -536,6 +536,19 @@ 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
+lines=$(wc -l < "checkconf.out$n")
+if [ $lines != 4 ]; 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/lib/isccfg/kaspconf.c b/lib/isccfg/kaspconf.c
index a8a078f0c7..97c8a1466c 100644
--- a/lib/isccfg/kaspconf.c
+++ b/lib/isccfg/kaspconf.c
@@ -324,6 +324,7 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx,
 	(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;
@@ -344,24 +345,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;

From b32a39dd27493f0616f13971f5f5652cf8dcc545 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 6 May 2022 16:21:16 +0200
Subject: [PATCH 2/7] Warn if key lifetime is short

Log a warning if the key lifetime is less than 30 days.

(cherry picked from commit e7322e8f781f94f26268b5da0c5a0ca6f5f04595)
---
 bin/tests/system/checkconf/kasp-warning.conf | 13 +++++++++++++
 bin/tests/system/checkconf/tests.sh          |  3 ++-
 lib/isccfg/kaspconf.c                        |  5 +++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/bin/tests/system/checkconf/kasp-warning.conf b/bin/tests/system/checkconf/kasp-warning.conf
index 765c09b14a..4c05b5ad02 100644
--- a/bin/tests/system/checkconf/kasp-warning.conf
+++ b/bin/tests/system/checkconf/kasp-warning.conf
@@ -32,6 +32,13 @@ dnssec-policy "warn2" {
 	};
 };
 
+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";
@@ -44,3 +51,9 @@ zone "warn2.example.net" {
 	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 807c79b5dc..5abda873fd 100644
--- a/bin/tests/system/checkconf/tests.sh
+++ b/bin/tests/system/checkconf/tests.sh
@@ -544,8 +544,9 @@ grep "dnssec-policy: algorithm 8 has multiple keys with ZSK role" < checkconf.ou
 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 != 4 ]; then ret=1; fi
+if [ $lines != 5 ]; then ret=1; fi
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 
diff --git a/lib/isccfg/kaspconf.c b/lib/isccfg/kaspconf.c
index 97c8a1466c..70c2511490 100644
--- a/lib/isccfg/kaspconf.c
+++ b/lib/isccfg/kaspconf.c
@@ -108,6 +108,11 @@ 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 && key->lifetime < 30 * (24 * 3600)) {
+			cfg_obj_log(obj, logctx, ISC_LOG_WARNING,
+				    "dnssec-policy: key lifetime is shorter "
+				    "than 30 days");
+		}
 
 		obj = cfg_tuple_get(config, "algorithm");
 		alg.base = cfg_obj_asstring(obj);

From 46636b8563b84beab4ffbe24fe5a9f8b8180868f Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 6 May 2022 16:54:49 +0200
Subject: [PATCH 3/7] Error if signatures-refresh is too high

The signatures-refresh should not near the signatures-validity value,
to prevent operational instability. Same is true when checking against
signatures-validity-dnskey.

(cherry picked from commit 82fd89107f2cb784aabc798fbb65bbb44e608c2c)
---
 .../kasp-bad-signatures-refresh.conf          | 44 +++++++++++++++++++
 bin/tests/system/checkconf/tests.sh           | 11 +++++
 lib/isccfg/kaspconf.c                         | 38 +++++++++++++---
 3 files changed, 86 insertions(+), 7 deletions(-)
 create mode 100644 bin/tests/system/checkconf/kasp-bad-signatures-refresh.conf

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/tests.sh b/bin/tests/system/checkconf/tests.sh
index 5abda873fd..21b41f09e5 100644
--- a/bin/tests/system/checkconf/tests.sh
+++ b/bin/tests/system/checkconf/tests.sh
@@ -528,6 +528,17 @@ 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 predefined key length ($n)"
 ret=0
diff --git a/lib/isccfg/kaspconf.c b/lib/isccfg/kaspconf.c
index 70c2511490..de538690b6 100644
--- a/lib/isccfg/kaspconf.c
+++ b/lib/isccfg/kaspconf.c
@@ -268,6 +268,7 @@ 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;
 
 	REQUIRE(kaspp != NULL && *kaspp == NULL);
 
@@ -308,13 +309,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",
+				   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);
+
+	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);
+
+	if (result != ISC_R_SUCCESS) {
+		goto cleanup;
+	}
 
 	/* Configuration: Keys */
 	dns_kasp_setdnskeyttl(

From 2036a8b1613034cf47bdbc49c2e7e38cb09d10de Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 6 May 2022 16:56:13 +0200
Subject: [PATCH 4/7] Update signatures-refresh documentation

Mention in the ARM the new restriction about signatures-refresh.

(cherry picked from commit 74d2e7704fee7d1fa5e47615b29d1ef35b41dd9e)
---
 doc/arm/reference.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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

From 3cfbe31176ff3119666bb1471fc87ba25a647e99 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Mon, 9 May 2022 13:56:45 +0200
Subject: [PATCH 5/7] Error if key lifetime is too short

The key lifetime should not be shorter than the time it costs to
introduce the successor key, otherwise keys will be created faster than
they are removed, resulting in a large key set.

The time it takes to replace a key is determined by the publication
interval (Ipub) of the successor key and the retire interval of the
predecessor key (Iret).

For the ZSK, Ipub is the sum of the DNSKEY TTL and zone propagation
delay (and publish safety). Iret is the sum of Dsgn, the maximum zone
TTL and zone propagation delay (and retire safety). The sign delay is
the signature validity period minus the refresh interval: The time to
ensure that all existing RRsets have been re-signed with the new key.
The ZSK lifetime should be larger than both values.

For the KSK, Ipub is the sum of the DNSKEY TTL and zone propagation
delay (and publish safety). Iret is the sum of the DS TTL and parent
zone propagation delay (and retire safety). The KSK lifetime should be
larger than both values.

(cherry picked from commit 8134d46cdb2ed592c4c4a3b21f7419621c25527f)
---
 .../system/checkconf/kasp-bad-lifetime.conf   | 91 +++++++++++++++++++
 bin/tests/system/checkconf/tests.sh           |  9 ++
 lib/isccfg/kaspconf.c                         | 82 +++++++++++++----
 3 files changed, 164 insertions(+), 18 deletions(-)
 create mode 100644 bin/tests/system/checkconf/kasp-bad-lifetime.conf

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/tests.sh b/bin/tests/system/checkconf/tests.sh
index 21b41f09e5..e1131dab51 100644
--- a/bin/tests/system/checkconf/tests.sh
+++ b/bin/tests/system/checkconf/tests.sh
@@ -539,6 +539,15 @@ 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
diff --git a/lib/isccfg/kaspconf.c b/lib/isccfg/kaspconf.c
index de538690b6..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,10 +110,30 @@ 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 && key->lifetime < 30 * (24 * 3600)) {
-			cfg_obj_log(obj, logctx, ISC_LOG_WARNING,
-				    "dnssec-policy: key lifetime is shorter "
-				    "than 30 days");
+		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");
@@ -269,6 +291,8 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx,
 	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);
 
@@ -313,17 +337,6 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx,
 				  DNS_KASP_SIG_REFRESH);
 	dns_kasp_setsigrefresh(kasp, sigrefresh);
 
-	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);
-
 	sigvalidity = get_duration(maps, "signatures-validity-dnskey",
 				   DNS_KASP_SIG_VALIDITY_DNSKEY);
 	if (sigrefresh >= (sigvalidity * 0.9)) {
@@ -336,6 +349,17 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx,
 	}
 	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;
 	}
@@ -350,6 +374,26 @@ 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 };
@@ -360,7 +404,9 @@ cfg_kasp_fromconfig(const cfg_obj_t *config, const char *name, isc_mem_t *mctx,
 		     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;
 			}
@@ -424,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;
 		}

From c18dce4c499581b772c4c00137f6c5ba12320aaa Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Mon, 9 May 2022 14:28:45 +0200
Subject: [PATCH 6/7] Add CHANGE and release note for #1611

Feature change.

(cherry picked from commit 92f98002e75a1e679e1561dd0c6ae8708bda14f0)
---
 CHANGES                     | 3 +++
 doc/notes/notes-current.rst | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

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

From 42711dae880fda80345dcf5fb112daf3a030f422 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Tue, 10 May 2022 10:25:27 +0200
Subject: [PATCH 7/7] Only log "new successor in ..." if prepub != 0

If 'prepub' is 0, this has the special meaning that no rollover is
scheduled. If so, don't log "new successor in x seconds".

(cherry picked from commit 955a69109ef21e18466e62100eecd6babbc27e90)
---
 lib/dns/keymgr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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;