From cf17698f877e460cf24a49ce5cbe150ace094fc3 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Tue, 6 Apr 2021 11:31:35 +0200
Subject: [PATCH 1/9] Fix a kasp lock issue

The kasp lock would stay locked if 'dns_keymgr_run' failed.
---
 lib/dns/zone.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 7ccacf6e80..2f685f4bf6 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -19971,22 +19971,20 @@ zone_rekey(dns_zone_t *zone) {
 			   isc_result_totext(result));
 	}
 
-	if (kasp != NULL &&
-	    (result == ISC_R_SUCCESS || result == ISC_R_NOTFOUND)) {
-		result = dns_keymgr_run(&zone->origin, zone->rdclass, dir, mctx,
-					&keys, kasp, now, &nexttime);
-		if (result != ISC_R_SUCCESS) {
-			if (kasp != NULL) {
-				UNLOCK(&kasp->lock);
-			}
-			dnssec_log(zone, ISC_LOG_ERROR,
-				   "zone_rekey:dns_dnssec_keymgr failed: %s",
-				   isc_result_totext(result));
-			goto failure;
-		}
-	}
-
 	if (kasp != NULL) {
+		if (result == ISC_R_SUCCESS || result == ISC_R_NOTFOUND) {
+			result = dns_keymgr_run(&zone->origin, zone->rdclass,
+						dir, mctx, &keys, kasp, now,
+						&nexttime);
+			if (result != ISC_R_SUCCESS) {
+				dnssec_log(zone, ISC_LOG_ERROR,
+					   "zone_rekey:dns_dnssec_keymgr "
+					   "failed: %s",
+					   isc_result_totext(result));
+				UNLOCK(&kasp->lock);
+				goto failure;
+			}
+		}
 		UNLOCK(&kasp->lock);
 	}
 
@@ -20343,6 +20341,10 @@ failure:
 		 * Something went wrong; try again in ten minutes or
 		 * after a key refresh interval, whichever is shorter.
 		 */
+		dnssec_log(zone, ISC_LOG_DEBUG(3),
+			   "zone_rekey failure: %s (retry in %u seconds)",
+			   isc_result_totext(result),
+			   ISC_MIN(zone->refreshkeyinterval, 600));
 		isc_interval_set(&ival, ISC_MIN(zone->refreshkeyinterval, 600),
 				 0);
 		isc_time_nowplusinterval(&zone->refreshkeytime, &ival);

From fa05c1b8da1ee9dfe5b005a00edf8178c2e884d4 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 8 Apr 2021 11:32:48 +0200
Subject: [PATCH 2/9] When reading public key from file, also read state

The 'dst_key_fromnamedfile()' function did not read and store the
key state from the .state file when reading a public key file.
---
 lib/dns/dst_api.c | 77 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 49 insertions(+), 28 deletions(-)

diff --git a/lib/dns/dst_api.c b/lib/dns/dst_api.c
index 9692ac66be..1c48a26d22 100644
--- a/lib/dns/dst_api.c
+++ b/lib/dns/dst_api.c
@@ -569,8 +569,8 @@ dst_key_fromnamedfile(const char *filename, const char *dirname, int type,
 		      isc_mem_t *mctx, dst_key_t **keyp) {
 	isc_result_t result;
 	dst_key_t *pubkey = NULL, *key = NULL;
-	char *newfilename = NULL;
-	int newfilenamelen = 0;
+	char *newfilename = NULL, *statefilename = NULL;
+	int newfilenamelen = 0, statefilenamelen = 0;
 	isc_lex_t *lex = NULL;
 
 	REQUIRE(dst_initialized);
@@ -604,9 +604,39 @@ dst_key_fromnamedfile(const char *filename, const char *dirname, int type,
 	newfilename = NULL;
 	RETERR(result);
 
+	/*
+	 * Read the state file, if requested by type.
+	 */
+	if ((type & DST_TYPE_STATE) != 0) {
+		statefilenamelen = strlen(filename) + 7;
+		if (dirname != NULL) {
+			statefilenamelen += strlen(dirname) + 1;
+		}
+		statefilename = isc_mem_get(mctx, statefilenamelen);
+		result = addsuffix(statefilename, statefilenamelen, dirname,
+				   filename, ".state");
+		INSIST(result == ISC_R_SUCCESS);
+	}
+
+	pubkey->kasp = false;
+	if ((type & DST_TYPE_STATE) != 0) {
+		result = dst_key_read_state(statefilename, mctx, &pubkey);
+		if (result == ISC_R_SUCCESS) {
+			pubkey->kasp = true;
+		} else if (result == ISC_R_FILENOTFOUND) {
+			/* Having no state is valid. */
+			result = ISC_R_SUCCESS;
+		}
+		RETERR(result);
+	}
+
 	if ((type & (DST_TYPE_PRIVATE | DST_TYPE_PUBLIC)) == DST_TYPE_PUBLIC ||
 	    (pubkey->key_flags & DNS_KEYFLAG_TYPEMASK) == DNS_KEYTYPE_NOKEY)
 	{
+		if (statefilename != NULL) {
+			isc_mem_put(mctx, statefilename, statefilenamelen);
+		}
+
 		result = computeid(pubkey);
 		if (result != ISC_R_SUCCESS) {
 			dst_key_free(&pubkey);
@@ -636,32 +666,6 @@ dst_key_fromnamedfile(const char *filename, const char *dirname, int type,
 		RETERR(DST_R_UNSUPPORTEDALG);
 	}
 
-	/*
-	 * Read the state file, if requested by type.
-	 */
-	if ((type & DST_TYPE_STATE) != 0) {
-		newfilenamelen = strlen(filename) + 7;
-		if (dirname != NULL) {
-			newfilenamelen += strlen(dirname) + 1;
-		}
-		newfilename = isc_mem_get(mctx, newfilenamelen);
-		result = addsuffix(newfilename, newfilenamelen, dirname,
-				   filename, ".state");
-		INSIST(result == ISC_R_SUCCESS);
-
-		key->kasp = false;
-		result = dst_key_read_state(newfilename, mctx, &key);
-		if (result == ISC_R_SUCCESS) {
-			key->kasp = true;
-		} else if (result == ISC_R_FILENOTFOUND) {
-			/* Having no state is valid. */
-			result = ISC_R_SUCCESS;
-		}
-		isc_mem_put(mctx, newfilename, newfilenamelen);
-		newfilename = NULL;
-		RETERR(result);
-	}
-
 	newfilenamelen = strlen(filename) + 9;
 	if (dirname != NULL) {
 		newfilenamelen += strlen(dirname) + 1;
@@ -678,6 +682,20 @@ dst_key_fromnamedfile(const char *filename, const char *dirname, int type,
 	RETERR(key->func->parse(key, lex, pubkey));
 	isc_lex_destroy(&lex);
 
+	key->kasp = false;
+	if ((type & DST_TYPE_STATE) != 0) {
+		result = dst_key_read_state(statefilename, mctx, &key);
+		if (result == ISC_R_SUCCESS) {
+			key->kasp = true;
+		} else if (result == ISC_R_FILENOTFOUND) {
+			/* Having no state is valid. */
+			result = ISC_R_SUCCESS;
+		}
+		isc_mem_put(mctx, statefilename, statefilenamelen);
+		statefilename = NULL;
+	}
+	RETERR(result);
+
 	RETERR(computeid(key));
 
 	if (pubkey->key_id != key->key_id) {
@@ -695,6 +713,9 @@ out:
 	if (newfilename != NULL) {
 		isc_mem_put(mctx, newfilename, newfilenamelen);
 	}
+	if (statefilename != NULL) {
+		isc_mem_put(mctx, statefilename, statefilenamelen);
+	}
 	if (lex != NULL) {
 		isc_lex_destroy(&lex);
 	}

From 7ed089576f295b8330d4527ff0fefda0b228be03 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 8 Apr 2021 11:35:03 +0200
Subject: [PATCH 3/9] Try to read state when reading keylist from rdata

The function 'dns_dnssec_keylistfromrdataset()' creates a keylist from
the DNSKEY RRset. If we attempt to read the private key, we also store
the key state. However, if the private key is offline, the key state
will not be stored. To fix this, first attempt to read the public key
file. If then reading the private key file fails, and we do have a
public key, add that to the keylist, with appropriate state. If we
also failed to read the public key file, add the DNSKEY to the keylist,
as we did before.
---
 lib/dns/dnssec.c | 67 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 22 deletions(-)

diff --git a/lib/dns/dnssec.c b/lib/dns/dnssec.c
index 97316b7d33..f2a107b5fd 100644
--- a/lib/dns/dnssec.c
+++ b/lib/dns/dnssec.c
@@ -1657,7 +1657,7 @@ dns_dnssec_keylistfromrdataset(const dns_name_t *origin, const char *directory,
 			       dns_dnsseckeylist_t *keylist) {
 	dns_rdataset_t keys;
 	dns_rdata_t rdata = DNS_RDATA_INIT;
-	dst_key_t *pubkey = NULL, *privkey = NULL;
+	dst_key_t *dnskey = NULL, *pubkey = NULL, *privkey = NULL;
 	isc_result_t result;
 
 	REQUIRE(keyset != NULL && dns_rdataset_isassociated(keyset));
@@ -1680,27 +1680,38 @@ dns_dnssec_keylistfromrdataset(const dns_name_t *origin, const char *directory,
 			goto skip;
 		}
 
-		RETERR(dns_dnssec_keyfromrdata(origin, &rdata, mctx, &pubkey));
-		dst_key_setttl(pubkey, keys.ttl);
+		RETERR(dns_dnssec_keyfromrdata(origin, &rdata, mctx, &dnskey));
+		dst_key_setttl(dnskey, keys.ttl);
 
-		if (!is_zone_key(pubkey) ||
-		    (dst_key_flags(pubkey) & DNS_KEYTYPE_NOAUTH) != 0) {
+		if (!is_zone_key(dnskey) ||
+		    (dst_key_flags(dnskey) & DNS_KEYTYPE_NOAUTH) != 0) {
 			goto skip;
 		}
 
 		/* Corrupted .key file? */
-		if (!dns_name_equal(origin, dst_key_name(pubkey))) {
+		if (!dns_name_equal(origin, dst_key_name(dnskey))) {
 			goto skip;
 		}
 
 		if (publickey) {
-			RETERR(addkey(keylist, &pubkey, savekeys, mctx));
+			RETERR(addkey(keylist, &dnskey, savekeys, mctx));
 			goto skip;
 		}
 
+		/* Try to read the public key. */
 		result = dst_key_fromfile(
-			dst_key_name(pubkey), dst_key_id(pubkey),
-			dst_key_alg(pubkey),
+			dst_key_name(dnskey), dst_key_id(dnskey),
+			dst_key_alg(dnskey), (DST_TYPE_PUBLIC | DST_TYPE_STATE),
+			directory, mctx, &pubkey);
+		if (result == ISC_R_FILENOTFOUND || result == ISC_R_NOPERM) {
+			result = ISC_R_SUCCESS;
+		}
+		RETERR(result);
+
+		/* Now read the private key. */
+		result = dst_key_fromfile(
+			dst_key_name(dnskey), dst_key_id(dnskey),
+			dst_key_alg(dnskey),
 			(DST_TYPE_PUBLIC | DST_TYPE_PRIVATE | DST_TYPE_STATE),
 			directory, mctx, &privkey);
 
@@ -1711,22 +1722,22 @@ dns_dnssec_keylistfromrdataset(const dns_name_t *origin, const char *directory,
 		 */
 		if (result == ISC_R_FILENOTFOUND) {
 			uint32_t flags;
-			flags = dst_key_flags(pubkey);
+			flags = dst_key_flags(dnskey);
 			if ((flags & DNS_KEYFLAG_REVOKE) != 0) {
-				dst_key_setflags(pubkey,
+				dst_key_setflags(dnskey,
 						 flags & ~DNS_KEYFLAG_REVOKE);
 				result = dst_key_fromfile(
-					dst_key_name(pubkey),
-					dst_key_id(pubkey), dst_key_alg(pubkey),
+					dst_key_name(dnskey),
+					dst_key_id(dnskey), dst_key_alg(dnskey),
 					(DST_TYPE_PUBLIC | DST_TYPE_PRIVATE |
 					 DST_TYPE_STATE),
 					directory, mctx, &privkey);
 				if (result == ISC_R_SUCCESS &&
-				    dst_key_pubcompare(pubkey, privkey, false))
+				    dst_key_pubcompare(dnskey, privkey, false))
 				{
 					dst_key_setflags(privkey, flags);
 				}
-				dst_key_setflags(pubkey, flags);
+				dst_key_setflags(dnskey, flags);
 			}
 		}
 
@@ -1739,8 +1750,8 @@ dns_dnssec_keylistfromrdataset(const dns_name_t *origin, const char *directory,
 
 			isc_buffer_init(&buf, filename, NAME_MAX);
 			result2 = dst_key_getfilename(
-				dst_key_name(pubkey), dst_key_id(pubkey),
-				dst_key_alg(pubkey),
+				dst_key_name(dnskey), dst_key_id(dnskey),
+				dst_key_alg(dnskey),
 				(DST_TYPE_PUBLIC | DST_TYPE_PRIVATE |
 				 DST_TYPE_STATE),
 				directory, mctx, &buf);
@@ -1748,13 +1759,13 @@ dns_dnssec_keylistfromrdataset(const dns_name_t *origin, const char *directory,
 				char namebuf[DNS_NAME_FORMATSIZE];
 				char algbuf[DNS_SECALG_FORMATSIZE];
 
-				dns_name_format(dst_key_name(pubkey), namebuf,
+				dns_name_format(dst_key_name(dnskey), namebuf,
 						sizeof(namebuf));
-				dns_secalg_format(dst_key_alg(pubkey), algbuf,
+				dns_secalg_format(dst_key_alg(dnskey), algbuf,
 						  sizeof(algbuf));
 				snprintf(filename, sizeof(filename) - 1,
 					 "key file for %s/%s/%d", namebuf,
-					 algbuf, dst_key_id(pubkey));
+					 algbuf, dst_key_id(dnskey));
 			}
 
 			isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL,
@@ -1765,7 +1776,13 @@ dns_dnssec_keylistfromrdataset(const dns_name_t *origin, const char *directory,
 		}
 
 		if (result == ISC_R_FILENOTFOUND || result == ISC_R_NOPERM) {
-			RETERR(addkey(keylist, &pubkey, savekeys, mctx));
+			if (pubkey != NULL) {
+				RETERR(addkey(keylist, &pubkey, savekeys,
+					      mctx));
+			} else {
+				RETERR(addkey(keylist, &dnskey, savekeys,
+					      mctx));
+			}
 			goto skip;
 		}
 		RETERR(result);
@@ -1779,10 +1796,13 @@ dns_dnssec_keylistfromrdataset(const dns_name_t *origin, const char *directory,
 		 * Whatever the key's default TTL may have
 		 * been, the rdataset TTL takes priority.
 		 */
-		dst_key_setttl(privkey, dst_key_getttl(pubkey));
+		dst_key_setttl(privkey, dst_key_getttl(dnskey));
 
 		RETERR(addkey(keylist, &privkey, savekeys, mctx));
 	skip:
+		if (dnskey != NULL) {
+			dst_key_free(&dnskey);
+		}
 		if (pubkey != NULL) {
 			dst_key_free(&pubkey);
 		}
@@ -1809,6 +1829,9 @@ failure:
 	if (dns_rdataset_isassociated(&keys)) {
 		dns_rdataset_disassociate(&keys);
 	}
+	if (dnskey != NULL) {
+		dst_key_free(&dnskey);
+	}
 	if (pubkey != NULL) {
 		dst_key_free(&pubkey);
 	}

From b3a5859a9bc60f693c3e21a2d268917ade9d0880 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 8 Apr 2021 12:01:32 +0200
Subject: [PATCH 4/9] rndc dnssec -status should include offline keys

The rndc command 'dnssec -status' only considered keys from
'dns_dnssec_findmatchingkeys' which only includes keys with accessible
private keys. Change it so that offline keys are also listed in the
status.
---
 bin/named/server.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 3 deletions(-)

diff --git a/bin/named/server.c b/bin/named/server.c
index 6342ff31a1..e306fd63b5 100644
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -14935,8 +14935,8 @@ named_server_dnssec(named_server_t *server, isc_lex_t *lex,
 	isc_result_t result = ISC_R_SUCCESS;
 	dns_zone_t *zone = NULL;
 	dns_kasp_t *kasp = NULL;
-	dns_dnsseckeylist_t keys;
-	dns_dnsseckey_t *key;
+	dns_dnsseckeylist_t keys, dnskeys;
+	dns_dnsseckey_t *key, *key_next = NULL;
 	char *ptr, *zonetext = NULL;
 	const char *msg = NULL;
 	/* variables for -checkds */
@@ -14953,6 +14953,11 @@ named_server_dnssec(named_server_t *server, isc_lex_t *lex,
 	isc_stdtime_t now, when;
 	isc_time_t timenow, timewhen;
 	const char *dir;
+	dns_name_t *origin;
+	dns_db_t *db = NULL;
+	dns_dbnode_t *node = NULL;
+	dns_dbversion_t *version = NULL;
+	dns_rdataset_t keyset;
 
 	/* Skip the command name. */
 	ptr = next_token(lex, text);
@@ -14971,7 +14976,9 @@ named_server_dnssec(named_server_t *server, isc_lex_t *lex,
 	now = isc_time_seconds(&timenow);
 	when = now;
 
+	ISC_LIST_INIT(dnskeys);
 	ISC_LIST_INIT(keys);
+	dns_rdataset_init(&keyset);
 
 	if (strcasecmp(ptr, "-status") == 0) {
 		status = true;
@@ -15084,13 +15091,46 @@ named_server_dnssec(named_server_t *server, isc_lex_t *lex,
 
 	/* Get DNSSEC keys. */
 	dir = dns_zone_getkeydirectory(zone);
+	origin = dns_zone_getorigin(zone);
+	CHECK(dns_zone_getdb(zone, &db));
+	CHECK(dns_db_findnode(db, origin, false, &node));
+	dns_db_currentversion(db, &version);
+	/* Get keys from private key files. */
 	LOCK(&kasp->lock);
-	result = dns_dnssec_findmatchingkeys(dns_zone_getorigin(zone), dir, now,
+	result = dns_dnssec_findmatchingkeys(origin, dir, now,
 					     dns_zone_getmctx(zone), &keys);
 	UNLOCK(&kasp->lock);
 	if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND) {
 		goto cleanup;
 	}
+	/* Get public keys (dnskeys). */
+	result = dns_db_findrdataset(db, node, version, dns_rdatatype_dnskey,
+				     dns_rdatatype_none, 0, &keyset, NULL);
+	if (result == ISC_R_SUCCESS) {
+		CHECK(dns_dnssec_keylistfromrdataset(
+			origin, dir, dns_zone_getmctx(zone), &keyset, NULL,
+			NULL, false, false, &dnskeys));
+	} else if (result != ISC_R_NOTFOUND) {
+		CHECK(result);
+	}
+	/* Add new 'dnskeys' to 'keys'. */
+	for (dns_dnsseckey_t *k1 = ISC_LIST_HEAD(dnskeys); k1 != NULL;
+	     k1 = key_next) {
+		dns_dnsseckey_t *k2 = NULL;
+		key_next = ISC_LIST_NEXT(k1, link);
+
+		for (k2 = ISC_LIST_HEAD(keys); k2 != NULL;
+		     k2 = ISC_LIST_NEXT(k2, link)) {
+			if (dst_key_compare(k1->key, k2->key)) {
+				break;
+			}
+		}
+		/* No match found, add the new key. */
+		if (k2 == NULL) {
+			ISC_LIST_UNLINK(dnskeys, k1, link);
+			ISC_LIST_APPEND(keys, k1, link);
+		}
+	}
 
 	if (status) {
 		/*
@@ -15210,6 +15250,24 @@ cleanup:
 		(void)putnull(text);
 	}
 
+	if (dns_rdataset_isassociated(&keyset)) {
+		dns_rdataset_disassociate(&keyset);
+	}
+	if (node != NULL) {
+		dns_db_detachnode(db, &node);
+	}
+	if (version != NULL) {
+		dns_db_closeversion(db, &version, false);
+	}
+	if (db != NULL) {
+		dns_db_detach(&db);
+	}
+
+	while (!ISC_LIST_EMPTY(dnskeys)) {
+		key = ISC_LIST_HEAD(dnskeys);
+		ISC_LIST_UNLINK(dnskeys, key, link);
+		dns_dnsseckey_destroy(dns_zone_getmctx(zone), &key);
+	}
 	while (!ISC_LIST_EMPTY(keys)) {
 		key = ISC_LIST_HEAD(keys);
 		ISC_LIST_UNLINK(keys, key, link);

From 3e6fc49c165c203b3d2c3c58b93bcb18d18fcf40 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Mon, 12 Apr 2021 14:40:46 +0200
Subject: [PATCH 5/9] Don't roll offline keys

When checking the current DNSSEC state against the policy, consider
offline keys. If we didn't found an active key, check if the key is
offline by checking the public key list. If there is a match in the
public key list (the key data is retrieved from the .key and the
.state files), treat the key as offline and don't create a successor
key for it.
---
 lib/dns/include/dns/keymgr.h |  8 ++--
 lib/dns/keymgr.c             | 76 +++++++++++++++++++++++++++++++-----
 lib/dns/zone.c               |  4 +-
 3 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/lib/dns/include/dns/keymgr.h b/lib/dns/include/dns/keymgr.h
index 6c7e17ceee..ae4ab5da13 100644
--- a/lib/dns/include/dns/keymgr.h
+++ b/lib/dns/include/dns/keymgr.h
@@ -26,12 +26,12 @@ ISC_LANG_BEGINDECLS
 isc_result_t
 dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 	       const char *directory, isc_mem_t *mctx,
-	       dns_dnsseckeylist_t *keyring, dns_kasp_t *kasp,
-	       isc_stdtime_t now, isc_stdtime_t *nexttime);
+	       dns_dnsseckeylist_t *keyring, dns_dnsseckeylist_t *dnskeys,
+	       dns_kasp_t *kasp, isc_stdtime_t now, isc_stdtime_t *nexttime);
 /*%<
- * Manage keys in 'keylist' and update timing data according to 'kasp' policy.
+ * Manage keys in 'keyring' and update timing data according to 'kasp' policy.
  * Create new keys for 'origin' if necessary in 'directory'.  Append all such
- * keys, along with use hints gleaned from their metadata, onto 'keylist'.
+ * keys, along with use hints gleaned from their metadata, onto 'keyring'.
  *
  * Update key states and store changes back to disk. Store when to run next
  * in 'nexttime'.
diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index b4efb3aeb2..02dbd711e2 100644
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -1640,8 +1640,9 @@ 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) {
+		    dns_kasp_t *kasp, uint32_t lifetime, bool rollover,
+		    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;
@@ -1723,6 +1724,20 @@ keymgr_key_rollover(dns_kasp_key_t *kaspkey, dns_dnsseckey_t *active_key,
 
 	/* It is time to do key rollover, we need a new key. */
 
+	/*
+	 * If rollover is not allowed, warn.
+	 */
+	if (!rollover) {
+		dst_key_format(active_key->key, keystr, sizeof(keystr));
+		isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC,
+			      DNS_LOGMODULE_DNSSEC, ISC_LOG_WARNING,
+			      "keymgr: DNSKEY %s (%s) is offline in policy %s, "
+			      "cannot start rollover",
+			      keystr, keymgr_keyrole(active_key->key),
+			      dns_kasp_getname(kasp));
+		return (ISC_R_SUCCESS);
+	}
+
 	/*
 	 * Check if there is a key available in pool because keys
 	 * may have been pregenerated with dnssec-keygen.
@@ -1929,8 +1944,8 @@ keymgr_purge_keyfile(dst_key_t *key, const char *dir, int type) {
 isc_result_t
 dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 	       const char *directory, isc_mem_t *mctx,
-	       dns_dnsseckeylist_t *keyring, dns_kasp_t *kasp,
-	       isc_stdtime_t now, isc_stdtime_t *nexttime) {
+	       dns_dnsseckeylist_t *keyring, dns_dnsseckeylist_t *dnskeys,
+	       dns_kasp_t *kasp, isc_stdtime_t now, isc_stdtime_t *nexttime) {
 	isc_result_t result = ISC_R_SUCCESS;
 	dns_dnsseckeylist_t newkeys;
 	dns_kasp_key_t *kkey;
@@ -1974,8 +1989,17 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 			dst_key_format(dkey->key, keystr, sizeof(keystr));
 			isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC,
 				      DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1),
-				      "keymgr: keyring: dnskey %s (policy %s)",
-				      keystr, dns_kasp_getname(kasp));
+				      "keymgr: keyring: %s (policy %s)", keystr,
+				      dns_kasp_getname(kasp));
+		}
+		for (dns_dnsseckey_t *dkey = ISC_LIST_HEAD(*dnskeys);
+		     dkey != NULL; dkey = ISC_LIST_NEXT(dkey, link))
+		{
+			dst_key_format(dkey->key, keystr, sizeof(keystr));
+			isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC,
+				      DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1),
+				      "keymgr: dnskeys: %s (policy %s)", keystr,
+				      dns_kasp_getname(kasp));
 		}
 	}
 
@@ -2029,6 +2053,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 	{
 		uint32_t lifetime = dns_kasp_key_lifetime(kkey);
 		dns_dnsseckey_t *active_key = NULL;
+		bool rollover_allowed = true;
 
 		/* Do we have keys available for this kasp key? */
 		for (dns_dnsseckey_t *dkey = ISC_LIST_HEAD(*keyring);
@@ -2089,10 +2114,43 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 			}
 		}
 
+		if (active_key == NULL) {
+			/*
+			 * We didn't found an active key, perhaps the .private
+			 * key file is offline. If so, we don't want to create
+			 * a successor key. Check if we have an appropriate
+			 * state file.
+			 */
+			for (dns_dnsseckey_t *dnskey = ISC_LIST_HEAD(*dnskeys);
+			     dnskey != NULL;
+			     dnskey = ISC_LIST_NEXT(dnskey, link))
+			{
+				if (keymgr_dnsseckey_kaspkey_match(dnskey,
+								   kkey)) {
+					/* Found a match. */
+					dst_key_format(dnskey->key, keystr,
+						       sizeof(keystr));
+					isc_log_write(
+						dns_lctx,
+						DNS_LOGCATEGORY_DNSSEC,
+						DNS_LOGMODULE_DNSSEC,
+						ISC_LOG_DEBUG(1),
+						"keymgr: DNSKEY %s (%s) "
+						"offline, policy %s",
+						keystr,
+						keymgr_keyrole(dnskey->key),
+						dns_kasp_getname(kasp));
+					rollover_allowed = false;
+					active_key = dnskey;
+					break;
+				}
+			}
+		}
+
 		/* See if this key requires a rollover. */
-		RETERR(keymgr_key_rollover(kkey, active_key, keyring, &newkeys,
-					   origin, rdclass, kasp, lifetime, now,
-					   nexttime, mctx));
+		RETERR(keymgr_key_rollover(
+			kkey, active_key, keyring, &newkeys, origin, rdclass,
+			kasp, lifetime, rollover_allowed, now, nexttime, mctx));
 	}
 
 	/* Walked all kasp key configurations.  Append new keys. */
diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 2f685f4bf6..7d4c910917 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -19974,8 +19974,8 @@ zone_rekey(dns_zone_t *zone) {
 	if (kasp != NULL) {
 		if (result == ISC_R_SUCCESS || result == ISC_R_NOTFOUND) {
 			result = dns_keymgr_run(&zone->origin, zone->rdclass,
-						dir, mctx, &keys, kasp, now,
-						&nexttime);
+						dir, mctx, &keys, &dnskeys,
+						kasp, now, &nexttime);
 			if (result != ISC_R_SUCCESS) {
 				dnssec_log(zone, ISC_LOG_ERROR,
 					   "zone_rekey:dns_dnssec_keymgr "

From 6a60bf637d40c48520ec4c24c443f066b6464de3 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Mon, 12 Apr 2021 14:45:31 +0200
Subject: [PATCH 6/9] Update smart signing when key is offline

BIND 9 is smart about when to sign with what key. If a key is offline,
BIND will delete the old signature anyway if there is another key to
sign the RRset with.

With KASP we don't want to fallback to the KSK if the ZSK is missing,
only for the SOA RRset. If the KSK is missing, but we do have a ZSK,
deleting the signature is fine. Otherwise it depends on if we use KASP
or not. Update the 'delsig_ok' function to reflect that.
---
 lib/dns/zone.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 6 deletions(-)

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 7d4c910917..5b8f14c3f2 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -6438,7 +6438,7 @@ set_key_expiry_warning(dns_zone_t *zone, isc_stdtime_t when,
  */
 static bool
 delsig_ok(dns_rdata_rrsig_t *rrsig_ptr, dst_key_t **keys, unsigned int nkeys,
-	  bool *warn) {
+	  bool kasp, bool *warn) {
 	unsigned int i = 0;
 	isc_result_t ret;
 	bool have_ksk = false, have_zsk = false;
@@ -6482,12 +6482,27 @@ delsig_ok(dns_rdata_rrsig_t *rrsig_ptr, dst_key_t **keys, unsigned int nkeys,
 		*warn = true;
 	}
 
+	if (have_pksk && have_pzsk) {
+		return (true);
+	}
+
 	/*
-	 * It's okay to delete a signature if there is an active key
-	 * with the same algorithm to replace it.
+	 * Deleting the SOA RRSIG is always okay.
+	 */
+	if (rrsig_ptr->covered == dns_rdatatype_soa) {
+		return (true);
+	}
+
+	/*
+	 * It's okay to delete a signature if there is an active key with the
+	 * same algorithm to replace it, unless that violates the DNSSEC
+	 * policy.
 	 */
 	if (have_pksk || have_pzsk) {
-		return (true);
+		if (kasp && have_pzsk) {
+			return (true);
+		}
+		return (!kasp);
 	}
 
 	/*
@@ -6521,6 +6536,7 @@ del_sigs(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name,
 	dns_rdataset_t rdataset;
 	unsigned int i;
 	dns_rdata_rrsig_t rrsig;
+	bool kasp = (dns_zone_getkasp(zone) != NULL);
 	bool found;
 	int64_t timewarn = 0, timemaybe = 0;
 
@@ -6563,7 +6579,7 @@ del_sigs(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name,
 		    type != dns_rdatatype_cdnskey)
 		{
 			bool warn = false, deleted = false;
-			if (delsig_ok(&rrsig, keys, nkeys, &warn)) {
+			if (delsig_ok(&rrsig, keys, nkeys, kasp, &warn)) {
 				result = update_one_rr(db, ver, zonediff->diff,
 						       DNS_DIFFOP_DELRESIGN,
 						       name, rdataset.ttl,
@@ -6807,6 +6823,8 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 			isc_stdtime_t when;
 			bool ksk = false;
 			bool zsk = false;
+			bool have_ksk = false;
+			bool have_zsk = false;
 
 			kresult = dst_key_getbool(keys[i], DST_BOOL_KSK, &ksk);
 			if (kresult != ISC_R_SUCCESS) {
@@ -6821,6 +6839,56 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 				}
 			}
 
+			have_ksk = ksk;
+			have_zsk = zsk;
+			both = have_ksk && have_zsk;
+
+			for (j = 0; j < nkeys; j++) {
+				if (both) {
+					break;
+				}
+
+				if (j == i || ALG(keys[i]) != ALG(keys[j])) {
+					continue;
+				}
+
+				/*
+				 * Don't consider inactive keys or offline keys.
+				 */
+				if (!dst_key_isprivate(keys[j])) {
+					continue;
+				}
+				if (dst_key_inactive(keys[j])) {
+					continue;
+				}
+
+				if (REVOKE(keys[j])) {
+					continue;
+				}
+
+				if (!have_ksk) {
+					kresult = dst_key_getbool(keys[j],
+								  DST_BOOL_KSK,
+								  &have_ksk);
+					if (kresult != ISC_R_SUCCESS) {
+						if (KSK(keys[j])) {
+							have_ksk = true;
+						}
+					}
+				}
+				if (!have_zsk) {
+					kresult = dst_key_getbool(keys[j],
+								  DST_BOOL_ZSK,
+								  &have_zsk);
+					if (kresult != ISC_R_SUCCESS) {
+						if (!KSK(keys[j])) {
+							have_zsk = true;
+						}
+					}
+				}
+				both = have_ksk && have_zsk;
+			}
+
 			if (type == dns_rdatatype_dnskey ||
 			    type == dns_rdatatype_cdnskey ||
 			    type == dns_rdatatype_cds)
@@ -6836,7 +6904,13 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
 				/*
 				 * Other RRsets are signed with ZSK.
 				 */
-				continue;
+				if (type != dns_rdatatype_soa &&
+				    type != zone->privatetype) {
+					continue;
+				}
+				if (have_zsk) {
+					continue;
+				}
 			} else if (!dst_key_is_signing(keys[i], DST_BOOL_ZSK,
 						       inception, &when)) {
 				/*

From 4a8ad0a77f7f8baa0fda4d30760b6fbd2e617845 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Mon, 12 Apr 2021 15:26:34 +0200
Subject: [PATCH 7/9] Add kasp tests for offline keys

Add a test for default.kasp that if we remove the private key file,
no successor key is created for it. We need to update the kasp script
to deal with a missing private key. If this is the case, skip checks
for private key files.

Add a test with a zone for which the private key of the ZSK is missing.

Add a test with a zone for which the private key of the KSK is missing.
---
 bin/tests/system/kasp.sh                | 69 +++++++++++++++++++------
 bin/tests/system/kasp/ns3/named.conf.in |  9 ++++
 bin/tests/system/kasp/ns3/setup.sh      | 25 ++++++++-
 bin/tests/system/kasp/tests.sh          | 60 ++++++++++++++++++++-
 4 files changed, 143 insertions(+), 20 deletions(-)

diff --git a/bin/tests/system/kasp.sh b/bin/tests/system/kasp.sh
index 94e395fb4d..d21839a08b 100644
--- a/bin/tests/system/kasp.sh
+++ b/bin/tests/system/kasp.sh
@@ -59,6 +59,7 @@ VIEW2="4xILSZQnuO1UKubXHkYUsvBRPu8="
 # EXPECT_ZRRSIG
 # EXPECT_KRRSIG
 # LEGACY
+# PRIVATE
 
 key_key() {
 	echo "${1}__${2}"
@@ -112,6 +113,7 @@ key_clear() {
 	key_set "$1" "EXPECT_ZRRSIG" 'no'
 	key_set "$1" "EXPECT_KRRSIG" 'no'
 	key_set "$1" "LEGACY" 'no'
+	key_set "$1" "PRIVATE" 'yes'
 }
 
 # Start clear.
@@ -303,6 +305,7 @@ check_key() {
 	_dnskey_ttl="$DNSKEY_TTL"
 	_lifetime=$(key_get "$1" LIFETIME)
 	_legacy=$(key_get "$1" LEGACY)
+	_private=$(key_get "$1" PRIVATE)
 
 	_published=$(key_get "$1" PUBLISHED)
 	_active=$(key_get "$1" ACTIVE)
@@ -341,7 +344,9 @@ check_key() {
 
 	# Check file existence.
 	[ -s "$KEY_FILE" ] || ret=1
-	[ -s "$PRIVATE_FILE" ] || ret=1
+	if [ "$_private" = "yes" ]; then
+		[ -s "$PRIVATE_FILE" ] || ret=1
+	fi
 	if [ "$_legacy" = "no" ]; then
 		[ -s "$STATE_FILE" ] || ret=1
 	fi
@@ -352,7 +357,9 @@ check_key() {
 	grep "; Created:" "$KEY_FILE" > "${ZONE}.${KEY_ID}.${_alg_num}.created" || _log_error "mismatch created comment in $KEY_FILE"
 	KEY_CREATED=$(awk '{print $3}' < "${ZONE}.${KEY_ID}.${_alg_num}.created")
 
-	grep "Created: ${KEY_CREATED}" "$PRIVATE_FILE" > /dev/null || _log_error "mismatch created in $PRIVATE_FILE"
+	if [ "$_private" = "yes" ]; then
+		grep "Created: ${KEY_CREATED}" "$PRIVATE_FILE" > /dev/null || _log_error "mismatch created in $PRIVATE_FILE"
+	fi
 	if [ "$_legacy" = "no" ]; then
 		grep "Generated: ${KEY_CREATED}" "$STATE_FILE" > /dev/null || _log_error "mismatch generated in $STATE_FILE"
 	fi
@@ -363,8 +370,10 @@ check_key() {
 	grep "This is a ${_role2} key, keyid ${_key_id}, for ${_zone}." "$KEY_FILE" > /dev/null || _log_error "mismatch top comment in $KEY_FILE"
 	grep "${_zone}\. ${_dnskey_ttl} IN DNSKEY ${_flags} 3 ${_alg_num}" "$KEY_FILE" > /dev/null || _log_error "mismatch DNSKEY record in $KEY_FILE"
 	# Now check the private key file.
-	grep "Private-key-format: v1.3" "$PRIVATE_FILE" > /dev/null || _log_error "mismatch private key format in $PRIVATE_FILE"
-	grep "Algorithm: ${_alg_num} (${_alg_string})" "$PRIVATE_FILE" > /dev/null || _log_error "mismatch algorithm in $PRIVATE_FILE"
+	if [ "$_private" = "yes" ]; then
+		grep "Private-key-format: v1.3" "$PRIVATE_FILE" > /dev/null || _log_error "mismatch private key format in $PRIVATE_FILE"
+		grep "Algorithm: ${_alg_num} (${_alg_string})" "$PRIVATE_FILE" > /dev/null || _log_error "mismatch algorithm in $PRIVATE_FILE"
+	fi
 	# Now check the key state file.
 	if [ "$_legacy" = "no" ]; then
 		grep "This is the state of key ${_key_id}, for ${_zone}." "$STATE_FILE" > /dev/null || _log_error "mismatch top comment in $STATE_FILE"
@@ -444,6 +453,8 @@ check_timingmetadata() {
 	_key_file="${_base_file}.key"
 	_private_file="${_base_file}.private"
 	_state_file="${_base_file}.state"
+	_legacy=$(key_get "$1" LEGACY)
+	_private=$(key_get "$1" PRIVATE)
 
 	_published=$(key_get "$1" PUBLISHED)
 	_syncpublish=$(key_get "$1" SYNCPUBLISH)
@@ -459,13 +470,17 @@ check_timingmetadata() {
 
 	if [ "$_published" = "none" ]; then
 		grep "; Publish:" "${_key_file}" > /dev/null && _log_error "unexpected publish comment in ${_key_file}"
-		grep "Publish:" "${_private_file}" > /dev/null && _log_error "unexpected publish in ${_private_file}"
+		if [ "$_private" = "yes" ]; then
+			grep "Publish:" "${_private_file}" > /dev/null && _log_error "unexpected publish in ${_private_file}"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Published: " "${_state_file}" > /dev/null && _log_error "unexpected publish in ${_state_file}"
 		fi
 	else
 		grep "; Publish: $_published" "${_key_file}" > /dev/null || _log_error "mismatch publish comment in ${_key_file} (expected ${_published})"
-		grep "Publish: $_published" "${_private_file}" > /dev/null || _log_error "mismatch publish in ${_private_file} (expected ${_published})"
+		if [ "$_private" = "yes" ]; then
+			grep "Publish: $_published" "${_private_file}" > /dev/null || _log_error "mismatch publish in ${_private_file} (expected ${_published})"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Published: $_published" "${_state_file}" > /dev/null || _log_error "mismatch publish in ${_state_file} (expected ${_published})"
 		fi
@@ -473,13 +488,17 @@ check_timingmetadata() {
 
 	if [ "$_syncpublish" = "none" ]; then
 		grep "; SyncPublish:" "${_key_file}" > /dev/null && _log_error "unexpected syncpublish comment in ${_key_file}"
-		grep "SyncPublish:" "${_private_file}" > /dev/null && _log_error "unexpected syncpublish in ${_private_file}"
+		if [ "$_private" = "yes" ]; then
+			grep "SyncPublish:" "${_private_file}" > /dev/null && _log_error "unexpected syncpublish in ${_private_file}"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "PublishCDS: " "${_state_file}" > /dev/null && _log_error "unexpected syncpublish in ${_state_file}"
 		fi
 	else
 		grep "; SyncPublish: $_syncpublish" "${_key_file}" > /dev/null || _log_error "mismatch syncpublish comment in ${_key_file} (expected ${_syncpublish})"
-		grep "SyncPublish: $_syncpublish" "${_private_file}" > /dev/null || _log_error "mismatch syncpublish in ${_private_file} (expected ${_syncpublish})"
+		if [ "$_private" = "yes" ]; then
+			grep "SyncPublish: $_syncpublish" "${_private_file}" > /dev/null || _log_error "mismatch syncpublish in ${_private_file} (expected ${_syncpublish})"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "PublishCDS: $_syncpublish" "${_state_file}" > /dev/null || _log_error "mismatch syncpublish in ${_state_file} (expected ${_syncpublish})"
 		fi
@@ -487,13 +506,17 @@ check_timingmetadata() {
 
 	if [ "$_active" = "none" ]; then
 		grep "; Activate:" "${_key_file}" > /dev/null && _log_error "unexpected active comment in ${_key_file}"
-		grep "Activate:" "${_private_file}" > /dev/null && _log_error "unexpected active in ${_private_file}"
+		if [ "$_private" = "yes" ]; then
+			grep "Activate:" "${_private_file}" > /dev/null && _log_error "unexpected active in ${_private_file}"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Active: " "${_state_file}" > /dev/null && _log_error "unexpected active in ${_state_file}"
 		fi
 	else
 		grep "; Activate: $_active" "${_key_file}" > /dev/null || _log_error "mismatch active comment in ${_key_file} (expected ${_active})"
-		grep "Activate: $_active" "${_private_file}" > /dev/null || _log_error "mismatch active in ${_private_file} (expected ${_active})"
+		if [ "$_private" = "yes" ]; then
+			grep "Activate: $_active" "${_private_file}" > /dev/null || _log_error "mismatch active in ${_private_file} (expected ${_active})"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Active: $_active" "${_state_file}" > /dev/null || _log_error "mismatch active in ${_state_file} (expected ${_active})"
 		fi
@@ -501,13 +524,17 @@ check_timingmetadata() {
 
 	if [ "$_retired" = "none" ]; then
 		grep "; Inactive:" "${_key_file}" > /dev/null && _log_error "unexpected retired comment in ${_key_file}"
-		grep "Inactive:" "${_private_file}" > /dev/null && _log_error "unexpected retired in ${_private_file}"
+		if [ "$_private" = "yes" ]; then
+			grep "Inactive:" "${_private_file}" > /dev/null && _log_error "unexpected retired in ${_private_file}"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Retired: " "${_state_file}" > /dev/null && _log_error "unexpected retired in ${_state_file}"
 		fi
 	else
 		grep "; Inactive: $_retired" "${_key_file}" > /dev/null || _log_error "mismatch retired comment in ${_key_file} (expected ${_retired})"
-		grep "Inactive: $_retired" "${_private_file}" > /dev/null || _log_error "mismatch retired in ${_private_file} (expected ${_retired})"
+		if [ "$_private" = "yes" ]; then
+			grep "Inactive: $_retired" "${_private_file}" > /dev/null || _log_error "mismatch retired in ${_private_file} (expected ${_retired})"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Retired: $_retired" "${_state_file}" > /dev/null || _log_error "mismatch retired in ${_state_file} (expected ${_retired})"
 		fi
@@ -515,13 +542,17 @@ check_timingmetadata() {
 
 	if [ "$_revoked" = "none" ]; then
 		grep "; Revoke:" "${_key_file}" > /dev/null && _log_error "unexpected revoked comment in ${_key_file}"
-		grep "Revoke:" "${_private_file}" > /dev/null && _log_error "unexpected revoked in ${_private_file}"
+		if [ "$_private" = "yes" ]; then
+			grep "Revoke:" "${_private_file}" > /dev/null && _log_error "unexpected revoked in ${_private_file}"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Revoked: " "${_state_file}" > /dev/null && _log_error "unexpected revoked in ${_state_file}"
 		fi
 	else
 		grep "; Revoke: $_revoked" "${_key_file}" > /dev/null || _log_error "mismatch revoked comment in ${_key_file} (expected ${_revoked})"
-		grep "Revoke: $_revoked" "${_private_file}" > /dev/null || _log_error "mismatch revoked in ${_private_file} (expected ${_revoked})"
+		if [ "$_private" = "yes" ]; then
+			grep "Revoke: $_revoked" "${_private_file}" > /dev/null || _log_error "mismatch revoked in ${_private_file} (expected ${_revoked})"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Revoked: $_revoked" "${_state_file}" > /dev/null || _log_error "mismatch revoked in ${_state_file} (expected ${_revoked})"
 		fi
@@ -529,13 +560,17 @@ check_timingmetadata() {
 
 	if [ "$_removed" = "none" ]; then
 		grep "; Delete:" "${_key_file}" > /dev/null && _log_error "unexpected removed comment in ${_key_file}"
-		grep "Delete:" "${_private_file}" > /dev/null && _log_error "unexpected removed in ${_private_file}"
+		if [ "$_private" = "yes" ]; then
+			grep "Delete:" "${_private_file}" > /dev/null && _log_error "unexpected removed in ${_private_file}"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Removed: " "${_state_file}" > /dev/null && _log_error "unexpected removed in ${_state_file}"
 		fi
 	else
 		grep "; Delete: $_removed" "${_key_file}" > /dev/null || _log_error "mismatch removed comment in ${_key_file} (expected ${_removed})"
-		grep "Delete: $_removed" "${_private_file}" > /dev/null || _log_error "mismatch removed in ${_private_file} (expected ${_removed})"
+		if [ "$_private" = "yes" ]; then
+			grep "Delete: $_removed" "${_private_file}" > /dev/null || _log_error "mismatch removed in ${_private_file} (expected ${_removed})"
+		fi
 		if [ "$_legacy" = "no" ]; then
 			grep "Removed: $_removed" "${_state_file}" > /dev/null || _log_error "mismatch removed in ${_state_file} (expected ${_removed})"
 		fi
@@ -672,7 +707,7 @@ _check_keys() {
 	# Check key files.
 	_ids=$(get_keyids "$DIR" "$ZONE")
 	for _id in $_ids; do
-		# There are three key files with the same algorithm.
+		# There are multiple key files with the same algorithm.
 		# Check them until a match is found.
 		ret=0
 		echo_i "check key id $_id"
diff --git a/bin/tests/system/kasp/ns3/named.conf.in b/bin/tests/system/kasp/ns3/named.conf.in
index 6e6f7bfa06..a2731099e9 100644
--- a/bin/tests/system/kasp/ns3/named.conf.in
+++ b/bin/tests/system/kasp/ns3/named.conf.in
@@ -252,6 +252,15 @@ zone "unfresh-sigs.autosign" {
 	dnssec-policy "autosign";
 };
 
+/*
+ * Zone that has missing private KSK.
+ */
+zone "ksk-missing.autosign" {
+	type primary;
+	file "ksk-missing.autosign.db";
+	dnssec-policy "autosign";
+};
+
 /*
  * Zone that has missing private ZSK.
  */
diff --git a/bin/tests/system/kasp/ns3/setup.sh b/bin/tests/system/kasp/ns3/setup.sh
index 55e862856c..7e45193438 100644
--- a/bin/tests/system/kasp/ns3/setup.sh
+++ b/bin/tests/system/kasp/ns3/setup.sh
@@ -194,7 +194,25 @@ private_type_record $zone $DEFAULT_ALGORITHM_NUMBER "$KSK" >> "$infile"
 private_type_record $zone $DEFAULT_ALGORITHM_NUMBER "$ZSK" >> "$infile"
 $SIGNER -S -x -s now-1w -e now+1w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
 
-# These signatures are already expired, and the private ZSK is missing.
+# These signatures are still good, but the private KSK is missing.
+setup ksk-missing.autosign
+T="now-6mo"
+ksktimes="-P $T -A $T -P sync $T"
+zsktimes="-P $T -A $T"
+KSK=$($KEYGEN -a $DEFAULT_ALGORITHM -L 300 -f KSK $ksktimes $zone 2> keygen.out.$zone.1)
+ZSK=$($KEYGEN -a $DEFAULT_ALGORITHM -L 300        $zsktimes $zone 2> keygen.out.$zone.2)
+$SETTIME -s -g $O -d $O $T -k $O $T -r $O $T "$KSK" > settime.out.$zone.1 2>&1
+$SETTIME -s -g $O -k $O $T -z $O $T          "$ZSK" > settime.out.$zone.2 2>&1
+cat template.db.in "${KSK}.key" "${ZSK}.key" > "$infile"
+private_type_record $zone $DEFAULT_ALGORITHM_NUMBER "$KSK" >> "$infile"
+private_type_record $zone $DEFAULT_ALGORITHM_NUMBER "$ZSK" >> "$infile"
+$SIGNER -S -x -s now-1w -e now+1w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
+echo "KSK: yes" >> "${KSK}".state
+echo "ZSK: no" >> "${KSK}".state
+echo "Lifetime: 63072000" >> "${KSK}".state # PT2Y
+rm -f "${KSK}".private
+
+# These signatures are still good, but the private ZSK is missing.
 setup zsk-missing.autosign
 T="now-6mo"
 ksktimes="-P $T -A $T -P sync $T"
@@ -206,7 +224,10 @@ $SETTIME -s -g $O -k $O $T -z $O $T          "$ZSK" > settime.out.$zone.2 2>&1
 cat template.db.in "${KSK}.key" "${ZSK}.key" > "$infile"
 private_type_record $zone $DEFAULT_ALGORITHM_NUMBER "$KSK" >> "$infile"
 private_type_record $zone $DEFAULT_ALGORITHM_NUMBER "$ZSK" >> "$infile"
-$SIGNER -PS -x -s now-2w -e now-1mi -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
+$SIGNER -S -x -s now-1w -e now+1w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
+echo "KSK: no" >> "${ZSK}".state
+echo "ZSK: yes" >> "${ZSK}".state
+echo "Lifetime: 31536000" >> "${ZSK}".state # PT1Y
 rm -f "${ZSK}".private
 
 # These signatures are already expired, and the private ZSK is retired.
diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh
index be49ac7574..d8a96c3d9f 100644
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -325,6 +325,27 @@ retry_quiet 10 update_is_signed "10.0.0.11" "10.0.0.44" || ret=1
 test "$ret" -eq 0 || echo_i "failed"
 status=$((status+ret))
 
+# Move the private key file, a rekey event should not introduce replacement
+# keys.
+ret=0
+echo_i "test that if private key files are inaccessible this doesn't trigger a rollover ($n)"
+basefile=$(key_get KEY1 BASEFILE)
+mv "${basefile}.private" "${basefile}.offline"
+rndccmd 10.53.0.3 loadkeys "$ZONE" > /dev/null || log_error "rndc loadkeys zone ${ZONE} failed"
+wait_for_log 3 "offline, policy default" $DIR/named.run || ret=1
+mv "${basefile}.offline" "${basefile}.private"
+test "$ret" -eq 0 || echo_i "failed"
+status=$((status+ret))
+
+# Nothing has changed.
+check_keys
+check_dnssecstatus "$SERVER" "$POLICY" "$ZONE"
+set_keytimes_csk_policy
+check_keytimes
+check_apex
+check_subdomain
+dnssec_verify
+
 #
 # Zone: dynamic.kasp
 #
@@ -1339,6 +1360,25 @@ check_subdomain
 dnssec_verify
 check_rrsig_refresh
 
+#
+# Zone: ksk-missing.autosign.
+#
+set_zone "ksk-missing.autosign"
+set_policy "autosign" "2" "300"
+set_server "ns3" "10.53.0.3"
+# Key properties, timings and states same as above.
+# Skip checking the private file, because it is missing.
+key_set "KEY1" "PRIVATE" "no"
+
+check_keys
+check_dnssecstatus "$SERVER" "$POLICY" "$ZONE"
+check_apex
+check_subdomain
+dnssec_verify
+
+# Restore the PRIVATE variable.
+key_set "KEY1" "PRIVATE" "yes"
+
 #
 # Zone: zsk-missing.autosign.
 #
@@ -1346,7 +1386,25 @@ set_zone "zsk-missing.autosign"
 set_policy "autosign" "2" "300"
 set_server "ns3" "10.53.0.3"
 # Key properties, timings and states same as above.
-# TODO.
+# Skip checking the private file, because it is missing.
+key_set "KEY2" "PRIVATE" "no"
+
+check_keys
+check_dnssecstatus "$SERVER" "$POLICY" "$ZONE"
+# For the apex, we expect the SOA to be signed with the KSK because the ZSK is
+# offline. Temporary treat KEY1 as a zone signing key too.
+set_keyrole "KEY1" "csk"
+set_zonesigning "KEY1" "yes"
+set_zonesigning "KEY2" "no"
+check_apex
+set_keyrole "KEY1" "ksk"
+set_zonesigning "KEY1" "no"
+set_zonesigning "KEY2" "yes"
+check_subdomain
+dnssec_verify
+
+# Restore the PRIVATE variable.
+key_set "KEY2" "PRIVATE" "yes"
 
 #
 # Zone: zsk-retired.autosign.

From 366ed047ddebf14bffa82f991af9195f7c5eadda Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Mon, 12 Apr 2021 15:57:38 +0200
Subject: [PATCH 8/9] Changes and release notes for [#2596]

---
 CHANGES                     | 3 +++
 doc/notes/notes-current.rst | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/CHANGES b/CHANGES
index 3c752cc442..2d214f7d91 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,6 @@
+5634.	[bug]		Don't roll keys when the private key file is offline.
+			[GL #2596]
+
 5633.	[doc]		Inline-signing was incorrectly described as being
 			inherited from the options / view levels and was
 			incorrectly accepted at those levels without effect.
diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst
index b9b48a46c4..aa3cdde892 100644
--- a/doc/notes/notes-current.rst
+++ b/doc/notes/notes-current.rst
@@ -82,3 +82,6 @@ Bug Fixes
   between the new keys too. :gl:`#2628`
 
 - Update ZONEMD to match RFC 8976. :gl:`#2658`
+
+- With ``dnssec-policy```, don't roll keys if the private key file is offline.
+  :gl:`#2596`

From 636ff1e15cb15db9252bd664a6f051761db824ca Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Tue, 4 May 2021 09:49:14 +0200
Subject: [PATCH 9/9] No longer need to strcmp for "none"

When we introduced "dnssec-policy insecure" we could have removed the
'strcmp' check for "none", because if it was set to "none", the 'kasp'
variable would have been set to NULL.
---
 lib/bind9/check.c | 3 +++
 lib/dns/zone.c    | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/bind9/check.c b/lib/bind9/check.c
index ee00d2482a..464797e8a3 100644
--- a/lib/bind9/check.c
+++ b/lib/bind9/check.c
@@ -890,6 +890,9 @@ kasp_name_allowed(const cfg_listelt_t *element) {
 	if (strcmp("none", name) == 0) {
 		return (false);
 	}
+	if (strcmp("insecure", name) == 0) {
+		return (false);
+	}
 	if (strcmp("default", name) == 0) {
 		return (false);
 	}
diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 5b8f14c3f2..9553ade3e8 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -20078,8 +20078,7 @@ zone_rekey(dns_zone_t *zone) {
 		/*
 		 * Only update DNSKEY TTL if we have a policy.
 		 */
-		if (kasp != NULL && strcmp(dns_kasp_getname(kasp), "none") != 0)
-		{
+		if (kasp != NULL) {
 			ttl = dns_kasp_dnskeyttl(kasp);
 		}