From 680189913494150d3d5ebc023145a6e61255db80 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 27 Mar 2020 10:28:22 +0100
Subject: [PATCH 1/6] Fix and test migration to dnssec-policy

Migrating from 'auto-dnssec maintain;' to dnssec-policy did not
work properly, mainly because the legacy keys were initialized
badly. Several adjustments in the keymgr are required to get it right:

- Set published time on keys when we calculate prepublication time.
  This is not strictly necessary, but it is weird to have an active
  key without the published time set.

- Initalize key states also before matching keys. Determine the
  target state by looking at existing time metadata: If the time
  data is set and is in the past, it is a hint that the key and
  its corresponding records have been published in the zone already,
  and the state is initialized to RUMOURED. Otherwise, initialize it
  as HIDDEN. This fixes migration to dnssec-policy from existing
  keys.

- Initialize key goal on keys that match key policy to OMNIPRESENT.
  These may be existing legacy keys that are being migrated.

- A key that has its goal to OMNIPRESENT *or* an active key can
  match a kasp key.  The code was changed with CHANGE 5354 that
  was a bugfix to prevent creating new KSK keys for zones in the
  initial stage of signing.  However, this caused problems for
  restarts when rollovers are in progress, because an outroducing
  key can still be an active key.

The test for this introduces a new KEY property 'legacy'.  This is
used to skip tests related to .state files.
---
 bin/tests/system/kasp/ns6/named.conf.in      |  12 +
 bin/tests/system/kasp/ns6/named2.conf.in     |  11 +
 bin/tests/system/kasp/ns6/policies/kasp.conf |   9 +
 bin/tests/system/kasp/ns6/setup.sh           |  13 +
 bin/tests/system/kasp/tests.sh               | 244 ++++++++++++++-----
 lib/dns/keymgr.c                             | 141 +++++++----
 6 files changed, 319 insertions(+), 111 deletions(-)

diff --git a/bin/tests/system/kasp/ns6/named.conf.in b/bin/tests/system/kasp/ns6/named.conf.in
index 5a6ca042c4..41c1157f56 100644
--- a/bin/tests/system/kasp/ns6/named.conf.in
+++ b/bin/tests/system/kasp/ns6/named.conf.in
@@ -24,6 +24,7 @@ options {
 	listen-on-v6 { none; };
 	allow-transfer { any; };
 	recursion no;
+	key-directory ".";
 };
 
 key rndc_key {
@@ -35,6 +36,17 @@ controls {
 	inet 10.53.0.6 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
 };
 
+/* This is a zone that migrates to dnssec-policy. */
+zone "migrate.kasp" {
+	type master;
+	file "migrate.kasp.db";
+	auto-dnssec maintain;
+	allow-update { any; };
+	dnssec-dnskey-kskonly yes;
+	update-check-ksk yes;
+};
+
+/* These are alorithm rollover test zones. */
 zone "step1.algorithm-roll.kasp" {
 	type master;
 	file "step1.algorithm-roll.kasp.db";
diff --git a/bin/tests/system/kasp/ns6/named2.conf.in b/bin/tests/system/kasp/ns6/named2.conf.in
index 52660c6084..6c5a1f3001 100644
--- a/bin/tests/system/kasp/ns6/named2.conf.in
+++ b/bin/tests/system/kasp/ns6/named2.conf.in
@@ -35,6 +35,17 @@ controls {
 	inet 10.53.0.6 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
 };
 
+/* This is a zone that migrates to dnssec-policy. */
+zone "migrate.kasp" {
+	type master;
+	file "migrate.kasp.db";
+	allow-update { any; };
+	dnssec-policy "migrate";
+};
+
+/*
+ * Zones for testing KSK/ZSK algorithm roll.
+ */
 zone "step1.algorithm-roll.kasp" {
 	type master;
 	file "step1.algorithm-roll.kasp.db";
diff --git a/bin/tests/system/kasp/ns6/policies/kasp.conf b/bin/tests/system/kasp/ns6/policies/kasp.conf
index ad7028ed0c..a9424ec050 100644
--- a/bin/tests/system/kasp/ns6/policies/kasp.conf
+++ b/bin/tests/system/kasp/ns6/policies/kasp.conf
@@ -48,3 +48,12 @@ dnssec-policy "ecdsa256" {
 	parent-propagation-delay pt1h;
 	parent-ds-ttl 7200;
 };
+
+dnssec-policy "migrate" {
+	dnskey-ttl 300;
+
+	keys {
+		ksk key-directory lifetime unlimited algorithm ECDSAP256SHA256;
+		zsk key-directory lifetime P60D algorithm ECDSAP256SHA256;
+	};
+};
diff --git a/bin/tests/system/kasp/ns6/setup.sh b/bin/tests/system/kasp/ns6/setup.sh
index cae3475535..b22d36d21e 100644
--- a/bin/tests/system/kasp/ns6/setup.sh
+++ b/bin/tests/system/kasp/ns6/setup.sh
@@ -39,6 +39,18 @@ R="RUMOURED"
 O="OMNIPRESENT"
 U="UNRETENTIVE"
 
+# Set up a zone with auto-dnssec maintain to migrate to dnssec-policy.
+setup migrate.kasp
+echo "$zone" >> zones
+KSK=$($KEYGEN -a ECDSAP256SHA256 -f KSK -L 300 $zone 2> keygen.out.$zone.1)
+ZSK=$($KEYGEN -a ECDSAP256SHA256 -L 300 $zone 2> keygen.out.$zone.2)
+$SETTIME -P now -P sync now -A now "$KSK" > settime.out.$zone.1 2>&1
+$SETTIME -P now -A now "$ZSK" > settime.out.$zone.2 2>&1
+cat template.db.in "${KSK}.key" "${ZSK}.key" > "$infile"
+private_type_record $zone 5 "$KSK" >> "$infile"
+private_type_record $zone 5 "$ZSK" >> "$infile"
+$SIGNER -S -x -s now-1h -e now+2w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
+
 #
 # The zones at algorithm-roll.kasp represent the various steps of a ZSK/KSK
 # algorithm rollover.
@@ -47,6 +59,7 @@ U="UNRETENTIVE"
 # Step 1:
 # Introduce the first key. This will immediately be active.
 setup step1.algorithm-roll.kasp
+echo "$zone" >> zones
 KSK=$($KEYGEN -a RSASHA1 -f KSK -L 3600 $zone 2> keygen.out.$zone.1)
 ZSK=$($KEYGEN -a RSASHA1 -L 3600 $zone 2> keygen.out.$zone.2)
 TactN="now"
diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh
index 896731e5b5..8e79abf91c 100644
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -55,6 +55,7 @@ VIEW2="4xILSZQnuO1UKubXHkYUsvBRPu8="
 # STATE_DS=18
 # EXPECT_ZRRSIG=19
 # EXPECT_KRRSIG=20
+# LEGACY=21
 
 key_key() {
 	echo "${1}__${2}"
@@ -93,6 +94,7 @@ key_clear() {
 	key_set "$1" "STATE_DS" 'none'
 	key_set "$1" "EXPECT_ZRRSIG" 'no'
 	key_set "$1" "EXPECT_KRRSIG" 'no'
+	key_set "$1" "LEGACY" 'no'
 }
 
 # Start clear.
@@ -238,6 +240,7 @@ check_key() {
 	_length=$(key_get "$1" "ALG_LEN")
 	_dnskey_ttl="$DNSKEY_TTL"
 	_lifetime=$(key_get "$1" LIFETIME)
+	_legacy=$(key_get "$1" LEGACY)
 
 	_published=$(key_get "$1" PUBLISHED)
 	_active=$(key_get "$1" ACTIVE)
@@ -277,7 +280,10 @@ check_key() {
 	# Check file existence.
 	[ -s "$KEY_FILE" ] || ret=1
 	[ -s "$PRIVATE_FILE" ] || ret=1
-	[ -s "$STATE_FILE" ] || ret=1
+	if [ "$_legacy" == "no" ]; then
+		[ -s "$STATE_FILE" ] || ret=1
+	fi
+	[ "$ret" -eq 0 ] || log_error "${BASE_FILE} files missing"
 	[ "$ret" -eq 0 ] || return
 
 	test $_log -eq 1 && echo_i "check key $BASE_FILE"
@@ -289,106 +295,130 @@ check_key() {
 	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"
 	# Now check the key state file.
-	grep "This is the state of key ${_key_id}, for ${_zone}." "$STATE_FILE" > /dev/null || log_error "mismatch top comment in $STATE_FILE"
-	grep "Lifetime: ${_lifetime}" "$STATE_FILE" > /dev/null || log_error "mismatch lifetime in $STATE_FILE"
-	grep "Algorithm: ${_alg_num}" "$STATE_FILE" > /dev/null || log_error "mismatch algorithm in $STATE_FILE"
-	grep "Length: ${_length}" "$STATE_FILE" > /dev/null || log_error "mismatch length in $STATE_FILE"
-	grep "KSK: ${_ksk}" "$STATE_FILE" > /dev/null || log_error "mismatch ksk in $STATE_FILE"
-	grep "ZSK: ${_zsk}" "$STATE_FILE" > /dev/null || log_error "mismatch zsk in $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"
+		grep "Lifetime: ${_lifetime}" "$STATE_FILE" > /dev/null || log_error "mismatch lifetime in $STATE_FILE"
+		grep "Algorithm: ${_alg_num}" "$STATE_FILE" > /dev/null || log_error "mismatch algorithm in $STATE_FILE"
+		grep "Length: ${_length}" "$STATE_FILE" > /dev/null || log_error "mismatch length in $STATE_FILE"
+		grep "KSK: ${_ksk}" "$STATE_FILE" > /dev/null || log_error "mismatch ksk in $STATE_FILE"
+		grep "ZSK: ${_zsk}" "$STATE_FILE" > /dev/null || log_error "mismatch zsk in $STATE_FILE"
 
-	# Check key states.
-	if [ "$_goal" = "none" ]; then
-		grep "GoalState: " "$STATE_FILE" > /dev/null && log_error "unexpected goal state in $STATE_FILE"
-	else
-		grep "GoalState: ${_goal}" "$STATE_FILE" > /dev/null || log_error "mismatch goal state in $STATE_FILE"
-	fi
+		# Check key states.
+		if [ "$_goal" = "none" ]; then
+			grep "GoalState: " "$STATE_FILE" > /dev/null && log_error "unexpected goal state in $STATE_FILE"
+		else
+			grep "GoalState: ${_goal}" "$STATE_FILE" > /dev/null || log_error "mismatch goal state in $STATE_FILE"
+		fi
 
-	if [ "$_state_dnskey" = "none" ]; then
-		grep "DNSKEYState: " "$STATE_FILE" > /dev/null && log_error "unexpected dnskey state in $STATE_FILE"
-		grep "DNSKEYChange: " "$STATE_FILE" > /dev/null && log_error "unexpected dnskey change in $STATE_FILE"
-	else
-		grep "DNSKEYState: ${_state_dnskey}" "$STATE_FILE" > /dev/null || log_error "mismatch dnskey state in $STATE_FILE"
-		grep "DNSKEYChange: " "$STATE_FILE" > /dev/null || log_error "mismatch dnskey change in $STATE_FILE"
-	fi
+		if [ "$_state_dnskey" = "none" ]; then
+			grep "DNSKEYState: " "$STATE_FILE" > /dev/null && log_error "unexpected dnskey state in $STATE_FILE"
+			grep "DNSKEYChange: " "$STATE_FILE" > /dev/null && log_error "unexpected dnskey change in $STATE_FILE"
+		else
+			grep "DNSKEYState: ${_state_dnskey}" "$STATE_FILE" > /dev/null || log_error "mismatch dnskey state in $STATE_FILE"
+			grep "DNSKEYChange: " "$STATE_FILE" > /dev/null || log_error "mismatch dnskey change in $STATE_FILE"
+		fi
 
-	if [ "$_state_zrrsig" = "none" ]; then
-		grep "ZRRSIGState: " "$STATE_FILE" > /dev/null && log_error "unexpected zrrsig state in $STATE_FILE"
-		grep "ZRRSIGChange: " "$STATE_FILE" > /dev/null && log_error "unexpected zrrsig change in $STATE_FILE"
-	else
-		grep "ZRRSIGState: ${_state_zrrsig}" "$STATE_FILE" > /dev/null || log_error "mismatch zrrsig state in $STATE_FILE"
-		grep "ZRRSIGChange: " "$STATE_FILE" > /dev/null || log_error "mismatch zrrsig change in $STATE_FILE"
-	fi
+		if [ "$_state_zrrsig" = "none" ]; then
+			grep "ZRRSIGState: " "$STATE_FILE" > /dev/null && log_error "unexpected zrrsig state in $STATE_FILE"
+			grep "ZRRSIGChange: " "$STATE_FILE" > /dev/null && log_error "unexpected zrrsig change in $STATE_FILE"
+		else
+			grep "ZRRSIGState: ${_state_zrrsig}" "$STATE_FILE" > /dev/null || log_error "mismatch zrrsig state in $STATE_FILE"
+			grep "ZRRSIGChange: " "$STATE_FILE" > /dev/null || log_error "mismatch zrrsig change in $STATE_FILE"
+		fi
 
-	if [ "$_state_krrsig" = "none" ]; then
-		grep "KRRSIGState: " "$STATE_FILE" > /dev/null && log_error "unexpected krrsig state in $STATE_FILE"
-		grep "KRRSIGChange: " "$STATE_FILE" > /dev/null && log_error "unexpected krrsig change in $STATE_FILE"
-	else
-		grep "KRRSIGState: ${_state_krrsig}" "$STATE_FILE" > /dev/null || log_error "mismatch krrsig state in $STATE_FILE"
-		grep "KRRSIGChange: " "$STATE_FILE" > /dev/null || log_error "mismatch krrsig change in $STATE_FILE"
-	fi
+		if [ "$_state_krrsig" = "none" ]; then
+			grep "KRRSIGState: " "$STATE_FILE" > /dev/null && log_error "unexpected krrsig state in $STATE_FILE"
+			grep "KRRSIGChange: " "$STATE_FILE" > /dev/null && log_error "unexpected krrsig change in $STATE_FILE"
+		else
+			grep "KRRSIGState: ${_state_krrsig}" "$STATE_FILE" > /dev/null || log_error "mismatch krrsig state in $STATE_FILE"
+			grep "KRRSIGChange: " "$STATE_FILE" > /dev/null || log_error "mismatch krrsig change in $STATE_FILE"
+		fi
 
-	if [ "$_state_ds" = "none" ]; then
-		grep "DSState: " "$STATE_FILE" > /dev/null && log_error "unexpected ds state in $STATE_FILE"
-		grep "DSChange: " "$STATE_FILE" > /dev/null && log_error "unexpected ds change in $STATE_FILE"
-	else
-		grep "DSState: ${_state_ds}" "$STATE_FILE" > /dev/null || log_error "mismatch ds state in $STATE_FILE"
-		grep "DSChange: " "$STATE_FILE" > /dev/null || log_error "mismatch ds change in $STATE_FILE"
+		if [ "$_state_ds" = "none" ]; then
+			grep "DSState: " "$STATE_FILE" > /dev/null && log_error "unexpected ds state in $STATE_FILE"
+			grep "DSChange: " "$STATE_FILE" > /dev/null && log_error "unexpected ds change in $STATE_FILE"
+		else
+			grep "DSState: ${_state_ds}" "$STATE_FILE" > /dev/null || log_error "mismatch ds state in $STATE_FILE"
+			grep "DSChange: " "$STATE_FILE" > /dev/null || log_error "mismatch ds change in $STATE_FILE"
+		fi
 	fi
 
 	# Check timing metadata.
 	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"
-		grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE"
+		fi
 	else
-		grep "; Publish:" "$KEY_FILE" > /dev/null || log_error "mismatch publish comment in $KEY_FILE ($KEY_PUBLISHED)"
-		grep "Publish:" "$PRIVATE_FILE" > /dev/null || log_error "mismatch publish in $PRIVATE_FILE ($KEY_PUBLISHED)"
-		grep "Published:" "$STATE_FILE" > /dev/null || log_error "mismatch publish in $STATE_FILE ($KEY_PUBLISHED)"
+		grep "; Publish:" "$KEY_FILE" > /dev/null || log_error "mismatch publish comment in $KEY_FILE"
+		grep "Publish:" "$PRIVATE_FILE" > /dev/null || log_error "mismatch publish in $PRIVATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Published:" "$STATE_FILE" > /dev/null || log_error "mismatch publish in $STATE_FILE"
+		fi
 	fi
 
 	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"
-		grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE"
+		fi
 	else
 		grep "; Activate:" "$KEY_FILE" > /dev/null || log_error "mismatch active comment in $KEY_FILE"
 		grep "Activate:" "$PRIVATE_FILE" > /dev/null || log_error "mismatch active in $PRIVATE_FILE"
-		grep "Active: " "$STATE_FILE" > /dev/null || log_error "mismatch active in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Active: " "$STATE_FILE" > /dev/null || log_error "mismatch active in $STATE_FILE"
+		fi
 	fi
 
 	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"
-		grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE"
+		fi
 	else
 		grep "; Inactive:" "$KEY_FILE" > /dev/null || log_error "mismatch retired comment in $KEY_FILE"
 		grep "Inactive:" "$PRIVATE_FILE" > /dev/null || log_error "mismatch retired in $PRIVATE_FILE"
-		grep "Retired: " "$STATE_FILE" > /dev/null || log_error "mismatch retired in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Retired: " "$STATE_FILE" > /dev/null || log_error "mismatch retired in $STATE_FILE"
+		fi
 	fi
 
 	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"
-		grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE"
+		fi
 	else
 		grep "; Revoke:" "$KEY_FILE" > /dev/null || log_error "mismatch revoked comment in $KEY_FILE"
 		grep "Revoke:" "$PRIVATE_FILE" > /dev/null || log_error "mismatch revoked in $PRIVATE_FILE"
-		grep "Revoked: " "$STATE_FILE" > /dev/null || log_error "mismatch revoked in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Revoked: " "$STATE_FILE" > /dev/null || log_error "mismatch revoked in $STATE_FILE"
+		fi
 	fi
 
 	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"
-		grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE"
+		fi
 	else
 		grep "; Delete:" "$KEY_FILE" > /dev/null || log_error "mismatch removed comment in $KEY_FILE"
 		grep "Delete:" "$PRIVATE_FILE" > /dev/null || log_error "mismatch removed in $PRIVATE_FILE"
-		grep "Removed: " "$STATE_FILE" > /dev/null || log_error "mismatch removed in $STATE_FILE"
+		if [ "$_legacy" == "no" ]; then
+			grep "Removed: " "$STATE_FILE" > /dev/null || log_error "mismatch removed in $STATE_FILE"
+		fi
 	fi
 
 	grep "; Created:" "$KEY_FILE" > /dev/null || log_error "mismatch created comment in $KEY_FILE"
 	grep "Created:" "$PRIVATE_FILE" > /dev/null || log_error "mismatch created in $PRIVATE_FILE"
-	grep "Generated: " "$STATE_FILE" > /dev/null || log_error "mismatch generated in $STATE_FILE"
+	if [ "$_legacy" == "no" ]; then
+		grep "Generated: " "$STATE_FILE" > /dev/null || log_error "mismatch generated in $STATE_FILE"
+	fi
 }
 
 # Check the key with key id $1 and see if it is unused.
@@ -424,20 +454,24 @@ key_unused() {
 
 	# Check timing metadata.
 	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"
-	grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE"
 	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"
-	grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE"
 	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"
-	grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE"
 	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"
-	grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE"
 	grep "; Delete:" "$KEY_FILE" > /dev/null && log_error "unexpected removed comment in $KEY_FILE"
+
+	grep "Publish:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected publish in $PRIVATE_FILE"
+	grep "Activate:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected active in $PRIVATE_FILE"
+	grep "Inactive:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected retired in $PRIVATE_FILE"
+	grep "Revoke:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected revoked in $PRIVATE_FILE"
 	grep "Delete:" "$PRIVATE_FILE" > /dev/null && log_error "unexpected removed in $PRIVATE_FILE"
-	grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE"
+
+	if [ "$_legacy" == "no" ]; then
+		grep "Published: " "$STATE_FILE" > /dev/null && log_error "unexpected publish in $STATE_FILE"
+		grep "Active: " "$STATE_FILE" > /dev/null && log_error "unexpected active in $STATE_FILE"
+		grep "Retired: " "$STATE_FILE" > /dev/null && log_error "unexpected retired in $STATE_FILE"
+		grep "Revoked: " "$STATE_FILE" > /dev/null && log_error "unexpected revoked in $STATE_FILE"
+		grep "Removed: " "$STATE_FILE" > /dev/null && log_error "unexpected removed in $STATE_FILE"
+	fi
 }
 
 # Test: dnssec-verify zone $1.
@@ -2821,7 +2855,62 @@ dnssec_verify
 # interval.
 check_next_key_event 3600
 
-# Reconfig dnssec-policy (triggering algorithm roll).
+#
+# Testing good migration.
+#
+set_zone "migrate.kasp"
+set_policy "none" "2" "300"
+set_server "ns6" "10.53.0.6"
+
+init_migration_match() {
+	key_clear        "KEY1"
+	key_set          "KEY1" "LEGACY" "yes"
+	set_keyrole      "KEY1" "ksk"
+	set_keylifetime  "KEY1" "0"
+	set_keyalgorithm "KEY1" "13" "ECDSAP256SHA256" "256"
+	set_keysigning   "KEY1" "yes"
+	set_zonesigning  "KEY1" "no"
+
+	key_clear        "KEY2"
+	key_set          "KEY2" "LEGACY" "yes"
+	set_keyrole      "KEY2" "zsk"
+	set_keylifetime  "KEY2" "5184000"
+	set_keyalgorithm "KEY2" "13" "ECDSAP256SHA256" "256"
+	set_keysigning   "KEY2" "no"
+	set_zonesigning  "KEY2" "yes"
+
+	key_clear        "KEY3"
+	key_clear        "KEY4"
+
+	set_keytime  "KEY1" "PUBLISHED"    "yes"
+	set_keytime  "KEY1" "ACTIVE"       "yes"
+	set_keytime  "KEY1" "RETIRED"      "none"
+	set_keystate "KEY1" "GOAL"         "omnipresent"
+	set_keystate "KEY1" "STATE_DNSKEY" "rumoured"
+	set_keystate "KEY1" "STATE_KRRSIG" "rumoured"
+	set_keystate "KEY1" "STATE_DS"     "rumoured"
+
+	set_keytime  "KEY2" "PUBLISHED"    "yes"
+	set_keytime  "KEY2" "ACTIVE"       "yes"
+	set_keytime  "KEY2" "RETIRED"      "none"
+	set_keystate "KEY2" "GOAL"         "omnipresent"
+	set_keystate "KEY2" "STATE_DNSKEY" "rumoured"
+	set_keystate "KEY2" "STATE_ZRRSIG" "rumoured"
+}
+init_migration_match
+
+# Make sure the zone is signed with legacy keys.
+check_keys
+check_apex
+check_subdomain
+dnssec_verify
+
+# Remember legacy key tags.
+_migrate_ksk=$(key_get KEY1 ID)
+_migrate_zsk=$(key_get KEY2 ID)
+
+# Reconfig dnssec-policy (triggering algorithm roll and other dnssec-policy
+# changes).
 echo_i "reconfig dnssec-policy to trigger algorithm rollover"
 copy_setports ns6/named2.conf.in ns6/named.conf
 rndc_reconfig ns6 10.53.0.6
@@ -2854,6 +2943,35 @@ status=$((status+ret))
 
 next_key_event_threshold=$((next_key_event_threshold+i))
 
+#
+# Testing migration.
+#
+set_zone "migrate.kasp"
+set_policy "migrate" "2" "300"
+set_server "ns6" "10.53.0.6"
+
+# Key properties, timings and metadata should be the same as legacy keys above.
+# However, because the zsk has a lifetime, kasp will set the retired time.
+init_migration_match
+
+key_set     "KEY1" "LEGACY"  "no"
+
+key_set     "KEY2" "LEGACY"  "no"
+set_keytime "KEY2" "RETIRED" "yes"
+
+check_keys
+check_apex
+check_subdomain
+dnssec_verify
+
+# Check key tags, should be the same.
+n=$((n+1))
+echo_i "check that of zone ${ZONE} migration to dnssec-policy uses the same keys ($n)"
+ret=0
+[ $_migrate_ksk == $(key_get KEY1 ID) ] || log_error "mismatch ksk tag"
+[ $_migrate_zsk == $(key_get KEY2 ID) ] || log_error "mismatch zsk tag"
+status=$((status+ret))
+
 #
 # Testing KSK/ZSK algorithm rollover.
 #
diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index 8a54ea9771..e5d7f08edd 100644
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -43,13 +43,13 @@
  * Set key state to HIDDEN and change last changed to now,
  * only if key state has not been set before.
  */
-#define INITIALIZE_STATE(key, state, time)                                    \
+#define INITIALIZE_STATE(key, state, time, target)                            \
 	do {                                                                  \
 		dst_key_state_t s;                                            \
 		if (dst_key_getstate((key), (state), &s) == ISC_R_NOTFOUND) { \
 			isc_stdtime_t t;                                      \
 			dst_key_gettime((key), DST_TIME_CREATED, &t);         \
-			dst_key_setstate((key), (state), HIDDEN);             \
+			dst_key_setstate((key), (state), target);             \
 			dst_key_settime((key), (time), t);                    \
 		}                                                             \
 	} while (0)
@@ -105,7 +105,7 @@ static isc_stdtime_t
 keymgr_prepublication_time(dns_dnsseckey_t *key, dns_kasp_t *kasp,
 			   uint32_t lifetime, isc_stdtime_t now) {
 	isc_result_t ret;
-	isc_stdtime_t active, retire, prepub;
+	isc_stdtime_t active, retire, pub, prepub;
 	bool ksk = false;
 
 	REQUIRE(key != NULL);
@@ -125,7 +125,8 @@ keymgr_prepublication_time(dns_dnsseckey_t *key, dns_kasp_t *kasp,
 	if (ret != ISC_R_SUCCESS) {
 		uint32_t klifetime = 0;
 		/*
-		 * An active key must have an activate timing metadata.
+		 * An active key must have publish and activate timing
+		 * metadata.
 		 */
 		ret = dst_key_gettime(key->key, DST_TIME_ACTIVATE, &active);
 		if (ret != ISC_R_SUCCESS) {
@@ -133,6 +134,12 @@ keymgr_prepublication_time(dns_dnsseckey_t *key, dns_kasp_t *kasp,
 			dst_key_settime(key->key, DST_TIME_ACTIVATE, now);
 			active = now;
 		}
+		ret = dst_key_gettime(key->key, DST_TIME_PUBLISH, &pub);
+		if (ret != ISC_R_SUCCESS) {
+			/* Super weird, but if it happens, set it to now. */
+			dst_key_settime(key->key, DST_TIME_PUBLISH, now);
+			pub = now;
+		}
 
 		ret = dst_key_getnum(key->key, DST_NUM_LIFETIME, &klifetime);
 		if (ret != ISC_R_SUCCESS) {
@@ -1214,19 +1221,25 @@ transition:
 }
 
 /*
- * See if this key needs to be initialized with a role.  A key created and
- * derived from a dnssec-policy will have the required metadata available,
- * otherwise these may be missing and need to be initialized.
+ * See if this key needs to be initialized with properties.  A key created
+ * and derived from a dnssec-policy will have the required metadata available,
+ * otherwise these may be missing and need to be initialized.  The key states
+ * will be initialized according to existing timing metadata.
  *
  */
 static void
-keymgr_key_init_role(dns_dnsseckey_t *key) {
+keymgr_key_init(dns_dnsseckey_t *key, isc_stdtime_t now) {
 	bool ksk, zsk;
 	isc_result_t ret;
+	isc_stdtime_t active = 0, pub = 0, syncpub = 0;
+	dst_key_state_t dnskey_state = HIDDEN;
+	dst_key_state_t ds_state = HIDDEN;
+	dst_key_state_t zrrsig_state = HIDDEN;
 
 	REQUIRE(key != NULL);
 	REQUIRE(key->key != NULL);
 
+	/* Initialize role. */
 	ret = dst_key_getbool(key->key, DST_BOOL_KSK, &ksk);
 	if (ret != ISC_R_SUCCESS) {
 		ksk = ((dst_key_flags(key->key) & DNS_KEYFLAG_KSK) != 0);
@@ -1237,6 +1250,33 @@ keymgr_key_init_role(dns_dnsseckey_t *key) {
 		zsk = ((dst_key_flags(key->key) & DNS_KEYFLAG_KSK) == 0);
 		dst_key_setbool(key->key, DST_BOOL_ZSK, zsk);
 	}
+
+	/* Get time metadata. */
+	ret = dst_key_gettime(key->key, DST_TIME_ACTIVATE, &active);
+	if (active <= now && ret == ISC_R_SUCCESS) {
+		dnskey_state = RUMOURED;
+	}
+	ret = dst_key_gettime(key->key, DST_TIME_PUBLISH, &pub);
+	if (pub <= now && ret == ISC_R_SUCCESS) {
+		zrrsig_state = RUMOURED;
+	}
+	ret = dst_key_gettime(key->key, DST_TIME_SYNCPUBLISH, &syncpub);
+	if (syncpub <= now && ret == ISC_R_SUCCESS) {
+		ds_state = RUMOURED;
+	}
+
+	/* Set key states for all keys that do not have them. */
+	INITIALIZE_STATE(key->key, DST_KEY_DNSKEY, DST_TIME_DNSKEY,
+			 dnskey_state);
+	if (ksk) {
+		INITIALIZE_STATE(key->key, DST_KEY_KRRSIG, DST_TIME_KRRSIG,
+				 dnskey_state);
+		INITIALIZE_STATE(key->key, DST_KEY_DS, DST_TIME_DS, ds_state);
+	}
+	if (zsk) {
+		INITIALIZE_STATE(key->key, DST_KEY_ZRRSIG, DST_TIME_ZRRSIG,
+				 zrrsig_state);
+	}
 }
 
 /*
@@ -1302,23 +1342,13 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 	{
 		bool found_match = false;
 
-		/* Make sure this key knows about roles. */
-		keymgr_key_init_role(dkey);
+		keymgr_key_init(dkey, now);
 
 		for (kkey = ISC_LIST_HEAD(dns_kasp_keys(kasp)); kkey != NULL;
 		     kkey = ISC_LIST_NEXT(kkey, link))
 		{
 			if (keymgr_dnsseckey_kaspkey_match(dkey, kkey)) {
 				found_match = true;
-				dst_key_format(dkey->key, keystr,
-					       sizeof(keystr));
-				isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC,
-					      DNS_LOGMODULE_DNSSEC,
-					      ISC_LOG_DEBUG(1),
-					      "keymgr: DNSKEY %s (%s) matches "
-					      "policy %s",
-					      keystr, keymgr_keyrole(dkey->key),
-					      dns_kasp_getname(kasp));
 				break;
 			}
 		}
@@ -1343,8 +1373,17 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 		{
 			if (keymgr_dnsseckey_kaspkey_match(dkey, kkey)) {
 				/* Found a match. */
+				dst_key_format(dkey->key, keystr,
+					       sizeof(keystr));
+				isc_log_write(dns_lctx, DNS_LOGCATEGORY_DNSSEC,
+					      DNS_LOGMODULE_DNSSEC,
+					      ISC_LOG_DEBUG(1),
+					      "keymgr: DNSKEY %s (%s) matches "
+					      "policy %s",
+					      keystr, keymgr_keyrole(dkey->key),
+					      dns_kasp_getname(kasp));
 
-				/* Initialize lifetime. */
+				/* Initialize lifetime and goal, if not set. */
 				uint32_t l;
 				if (dst_key_getnum(dkey->key, DST_NUM_LIFETIME,
 						   &l) != ISC_R_SUCCESS) {
@@ -1353,18 +1392,44 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 						       lifetime);
 				}
 
-				if (dst_key_goal(dkey->key) == OMNIPRESENT) {
-					if (active_key != NULL) {
+				dst_key_state_t goal;
+				if (dst_key_getstate(dkey->key, DST_KEY_GOAL,
+						     &goal) != ISC_R_SUCCESS) {
+					dst_key_setstate(dkey->key,
+							 DST_KEY_GOAL,
+							 OMNIPRESENT);
+				}
+
+				if (active_key) {
+					/* We already have an active key that
+					 * matches the kasp policy.
+					 */
+					if (!dst_key_is_unused(dkey->key) &&
+					    (dst_key_goal(dkey->key) ==
+					     OMNIPRESENT) &&
+					    !keymgr_key_is_successor(
+						    dkey->key,
+						    active_key->key) &&
+					    !keymgr_key_is_successor(
+						    active_key->key, dkey->key))
+					{
 						/*
 						 * Multiple signing keys match
 						 * the kasp key configuration.
-						 * Retire excess keys.
+						 * Retire excess keys in use.
 						 */
 						keymgr_key_retire(dkey, now);
-					} else {
-						/* Save the matched key. */
-						active_key = dkey;
 					}
+					continue;
+				}
+
+				/*
+				 * Save the matched key only if it is active
+				 * or desires to be active.
+				 */
+				if (dst_key_goal(dkey->key) == OMNIPRESENT ||
+				    dst_key_is_active(dkey->key, now)) {
+					active_key = dkey;
 				}
 			}
 		}
@@ -1377,7 +1442,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 				isc_log_write(
 					dns_lctx, DNS_LOGCATEGORY_DNSSEC,
 					DNS_LOGMODULE_DNSSEC, ISC_LOG_DEBUG(1),
-					"keymgr: DNSKEY %s (%s) matches "
+					"keymgr: DNSKEY %s (%s) is active in "
 					"policy %s",
 					keystr, keymgr_keyrole(active_key->key),
 					dns_kasp_getname(kasp));
@@ -1446,6 +1511,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 			dst_key_setttl(dst_key, dns_kasp_dnskeyttl(kasp));
 			dst_key_settime(dst_key, DST_TIME_CREATED, now);
 			RETERR(dns_dnsseckey_create(mctx, &dst_key, &newkey));
+			keymgr_key_init(newkey, now);
 		} else {
 			newkey = candidate;
 			dst_key_setnum(newkey->key, DST_NUM_LIFETIME, lifetime);
@@ -1512,27 +1578,6 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 		ISC_LIST_APPENDLIST(*keyring, newkeys, link);
 	}
 
-	/* Initialize key states (for keys that don't have them yet). */
-	for (dns_dnsseckey_t *dkey = ISC_LIST_HEAD(*keyring); dkey != NULL;
-	     dkey = ISC_LIST_NEXT(dkey, link))
-	{
-		bool ksk = false, zsk = false;
-
-		/* Set key states for all keys that do not have them. */
-		INITIALIZE_STATE(dkey->key, DST_KEY_DNSKEY, DST_TIME_DNSKEY);
-		(void)dst_key_getbool(dkey->key, DST_BOOL_KSK, &ksk);
-		if (ksk) {
-			INITIALIZE_STATE(dkey->key, DST_KEY_KRRSIG,
-					 DST_TIME_KRRSIG);
-			INITIALIZE_STATE(dkey->key, DST_KEY_DS, DST_TIME_DS);
-		}
-		(void)dst_key_getbool(dkey->key, DST_BOOL_ZSK, &zsk);
-		if (zsk) {
-			INITIALIZE_STATE(dkey->key, DST_KEY_ZRRSIG,
-					 DST_TIME_ZRRSIG);
-		}
-	}
-
 	/* Read to update key states. */
 	keymgr_update(keyring, kasp, now, nexttime);
 

From a224754d59c348ea9b1680179a7b7a9f7e4c4ad0 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Wed, 1 Apr 2020 14:09:55 +0200
Subject: [PATCH 2/6] Tweak kasp system test

A few kasp system test tweaks to improve test failure debugging and
deal with tests related to migration to dnssec-policy.

1. When clearing a key, set lifetime to "none".  If "none", skip
   expect no lifetime set in the state file.  Legacy keys that
   are migrated but don't match the dnssec-policy will not have a
   lifetime.

2. The kasp system test prints which key id and file it is checking.
   Log explicitly if we are checking the id or a file.

3. Add quotes around "ID" when setting the key id, for consistency.

4. Fix a typo (non -> none).

5. Print which key ids are found, this way it is easier to see what
   KEY[1-4] failed to match one of the key files.
---
 bin/tests/system/kasp/tests.sh | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh
index 8e79abf91c..b403fc2101 100644
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -78,7 +78,7 @@ key_clear() {
 	key_set "$1" "ROLE" 'none'
 	key_set "$1" "KSK" 'no'
 	key_set "$1" "ZSK" 'no'
-	key_set "$1" "LIFETIME" '0'
+	key_set "$1" "LIFETIME" 'none'
 	key_set "$1" "ALG_NUM" '0'
 	key_set "$1" "ALG_STR" 'none'
 	key_set "$1" "ALG_LEN" '0'
@@ -286,7 +286,7 @@ check_key() {
 	[ "$ret" -eq 0 ] || log_error "${BASE_FILE} files missing"
 	[ "$ret" -eq 0 ] || return
 
-	test $_log -eq 1 && echo_i "check key $BASE_FILE"
+	test $_log -eq 1 && echo_i "check key file $BASE_FILE"
 
 	# Check the public key file.
 	grep "This is a ${_role2} key, keyid ${_key_id}, for ${_zone}." "$KEY_FILE" > /dev/null || log_error "mismatch top comment in $KEY_FILE"
@@ -297,7 +297,11 @@ check_key() {
 	# 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"
-		grep "Lifetime: ${_lifetime}" "$STATE_FILE" > /dev/null || log_error "mismatch lifetime in $STATE_FILE"
+		if [ "$_lifetime" == "none" ]; then
+			grep "Lifetime: " "$STATE_FILE" > /dev/null && log_error "unexpected lifetime in $STATE_FILE"
+		else
+			grep "Lifetime: ${_lifetime}" "$STATE_FILE" > /dev/null || log_error "mismatch lifetime in $STATE_FILE"
+		fi
 		grep "Algorithm: ${_alg_num}" "$STATE_FILE" > /dev/null || log_error "mismatch algorithm in $STATE_FILE"
 		grep "Length: ${_length}" "$STATE_FILE" > /dev/null || log_error "mismatch length in $STATE_FILE"
 		grep "KSK: ${_ksk}" "$STATE_FILE" > /dev/null || log_error "mismatch ksk in $STATE_FILE"
@@ -882,7 +886,7 @@ check_keys()
 	for _id in $_ids; do
 		# There are three key files with the same algorithm.
 		# Check them until a match is found.
-		echo_i "check key $_id"
+		echo_i "check key id $_id"
 
 		if [ "no" = "$(key_get KEY1 ID)" ] && [ "$(key_get KEY1 EXPECT)" = "yes" ]; then
 			ret=0
@@ -897,19 +901,19 @@ check_keys()
 		if [ "no" = "$(key_get KEY3 ID)" ] && [ "$(key_get KEY3 EXPECT)" = "yes"  ]; then
 			ret=0
 			check_key "KEY3" "$_id"
-			test "$ret" -eq 0 && key_set KEY3 ID "$KEY_ID" && continue
+			test "$ret" -eq 0 && key_set KEY3 "ID" "$KEY_ID" && continue
 		fi
 		if [ "no" = "$(key_get KEY4 ID)" ] && [ "$(key_get KEY4 EXPECT)" = "yes"  ]; then
 			ret=0
 			check_key "KEY4" "$_id"
-			test "$ret" -eq 0 && key_set KEY4 ID "$KEY_ID" && continue
+			test "$ret" -eq 0 && key_set KEY4 "ID" "$KEY_ID" && continue
 		fi
 
 		# This may be an unused key. Assume algorithm of KEY1.
 		ret=0 && key_unused "$_id" "$(key_get KEY1 ALG_NUM)"
 		test "$ret" -eq 0 && continue
 
-		# If ret is still non-zero, non of the files matched.
+		# If ret is still non-zero, none of the files matched.
 		test "$ret" -eq 0 || echo_i "failed"
 		status=$((status+1))
 	done
@@ -919,15 +923,19 @@ check_keys()
 
 	ret=0
 	if [ "$(key_get KEY1 EXPECT)" = "yes" ]; then
+		echo_i "KEY1 ID $(key_get KEY1 ID)"
 		test "no" = "$(key_get KEY1 ID)" && log_error "No KEY1 found for zone ${ZONE}"
 	fi
 	if [ "$(key_get KEY2 EXPECT)" = "yes" ]; then
+		echo_i "KEY2 ID $(key_get KEY2 ID)"
 		test "no" = "$(key_get KEY2 ID)" && log_error "No KEY2 found for zone ${ZONE}"
 	fi
 	if [ "$(key_get KEY3 EXPECT)" = "yes" ]; then
+		echo_i "KEY3 ID $(key_get KEY3 ID)"
 		test "no" = "$(key_get KEY3 ID)" && log_error "No KEY3 found for zone ${ZONE}"
 	fi
 	if [ "$(key_get KEY4 EXPECT)" = "yes" ]; then
+		echo_i "KEY4 ID $(key_get KEY4 ID)"
 		test "no" = "$(key_get KEY4 ID)" && log_error "No KEY4 found for zone ${ZONE}"
 	fi
 	test "$ret" -eq 0 || echo_i "failed"

From 7f43520893912fa97809569f866b2c3dd9fa4f33 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Wed, 1 Apr 2020 14:29:49 +0200
Subject: [PATCH 3/6] Test migration to dnssec-policy, retire old keys

Migrating from 'auto-dnssec maintain;' to dnssec-policy did not
work properly, mainly because the legacy keys were initialized
badly.  Earlier commit deals with migration where existing keys
match the policy.  This commit deals with migration where existing
keys do not match the policy.  In that case, named must not
immediately delete the existing keys, but gracefully roll to the
dnssec-policy.

However, named did remove the existing keys immediately.  This is
because the legacy key states were initialized badly.  Because
those keys had their states initialized to HIDDEN or RUMOURED, the
keymgr decides that they can be removed (because only when the key
has its states in OMNIPRESENT it can be used safely).

The original thought to initialize key states to HIDDEN (and
RUMOURED to deal with existing keys) was to ensure that those keys
will go through the required propagation time before the keymgr
decides they can be used safely.  However, those keys are already
in the zone for a long time and making the key states represent
otherwise is dangerous: keys may be pulled out of the zone while
in fact they are required to establish the chain of trust.

Fix initializing key states for existing keys by looking more closely
at the time metadata.  Add TTL and propagation delays to the time
metadata and see if the DNSSEC records have been propagated.
Initialize the state to OMNIPRESENT if so, otherwise initialize to
RUMOURED.  If the time metadata is in the future, or does not exist,
keep initializing the state to HIDDEN.

The added test makes sure that new keys matching the policy are
introduced, but existing keys are kept in the zone until the new
keys have been propagated.
---
 bin/tests/system/kasp/ns6/named.conf.in      |  11 +-
 bin/tests/system/kasp/ns6/named2.conf.in     |   9 +-
 bin/tests/system/kasp/ns6/policies/kasp.conf |  18 +++
 bin/tests/system/kasp/ns6/setup.sh           |  16 +++
 bin/tests/system/kasp/tests.sh               | 110 +++++++++++++++++++
 lib/dns/keymgr.c                             |  31 +++++-
 6 files changed, 187 insertions(+), 8 deletions(-)

diff --git a/bin/tests/system/kasp/ns6/named.conf.in b/bin/tests/system/kasp/ns6/named.conf.in
index 41c1157f56..5cecb8c542 100644
--- a/bin/tests/system/kasp/ns6/named.conf.in
+++ b/bin/tests/system/kasp/ns6/named.conf.in
@@ -36,7 +36,7 @@ controls {
 	inet 10.53.0.6 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
 };
 
-/* This is a zone that migrates to dnssec-policy. */
+/* These are zones that migrate to dnssec-policy. */
 zone "migrate.kasp" {
 	type master;
 	file "migrate.kasp.db";
@@ -46,6 +46,15 @@ zone "migrate.kasp" {
 	update-check-ksk yes;
 };
 
+zone "migrate-nomatch.kasp" {
+	type master;
+	file "migrate-nomatch.kasp.db";
+	auto-dnssec maintain;
+	allow-update { any; };
+	dnssec-dnskey-kskonly yes;
+	update-check-ksk yes;
+};
+
 /* These are alorithm rollover test zones. */
 zone "step1.algorithm-roll.kasp" {
 	type master;
diff --git a/bin/tests/system/kasp/ns6/named2.conf.in b/bin/tests/system/kasp/ns6/named2.conf.in
index 6c5a1f3001..0428dcbdf8 100644
--- a/bin/tests/system/kasp/ns6/named2.conf.in
+++ b/bin/tests/system/kasp/ns6/named2.conf.in
@@ -35,7 +35,7 @@ controls {
 	inet 10.53.0.6 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
 };
 
-/* This is a zone that migrates to dnssec-policy. */
+/* These are zones that migrate to dnssec-policy. */
 zone "migrate.kasp" {
 	type master;
 	file "migrate.kasp.db";
@@ -43,6 +43,13 @@ zone "migrate.kasp" {
 	dnssec-policy "migrate";
 };
 
+zone "migrate-nomatch.kasp" {
+	type master;
+	file "migrate-nomatch.kasp.db";
+	allow-update { any; };
+	dnssec-policy "migrate-nomatch";
+};
+
 /*
  * Zones for testing KSK/ZSK algorithm roll.
  */
diff --git a/bin/tests/system/kasp/ns6/policies/kasp.conf b/bin/tests/system/kasp/ns6/policies/kasp.conf
index a9424ec050..16fb3d60aa 100644
--- a/bin/tests/system/kasp/ns6/policies/kasp.conf
+++ b/bin/tests/system/kasp/ns6/policies/kasp.conf
@@ -57,3 +57,21 @@ dnssec-policy "migrate" {
 		zsk key-directory lifetime P60D algorithm ECDSAP256SHA256;
 	};
 };
+
+dnssec-policy "migrate-nomatch" {
+	dnskey-ttl 300;
+
+	keys {
+		ksk key-directory lifetime unlimited algorithm rsasha1 2048;
+		zsk key-directory lifetime P60D algorithm rsasha1 2048;
+	};
+
+	// Together 12h
+	zone-propagation-delay 3600;
+	max-zone-ttl 11h;
+
+	// Together 24h
+	parent-registration-delay 21h;
+	parent-propagation-delay pt1h;
+	parent-ds-ttl 7200;
+};
diff --git a/bin/tests/system/kasp/ns6/setup.sh b/bin/tests/system/kasp/ns6/setup.sh
index b22d36d21e..5c489e774f 100644
--- a/bin/tests/system/kasp/ns6/setup.sh
+++ b/bin/tests/system/kasp/ns6/setup.sh
@@ -51,6 +51,22 @@ private_type_record $zone 5 "$KSK" >> "$infile"
 private_type_record $zone 5 "$ZSK" >> "$infile"
 $SIGNER -S -x -s now-1h -e now+2w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
 
+# Set up a zone with auto-dnssec maintain to migrate to dnssec-policy, but this
+# time the existing keys do not match the policy.
+setup migrate-nomatch.kasp
+echo "$zone" >> zones
+KSK=$($KEYGEN -a RSASHA1 -b 1024 -f KSK -L 300 $zone 2> keygen.out.$zone.1)
+ZSK=$($KEYGEN -a RSASHA1 -b 1024 -L 300 $zone 2> keygen.out.$zone.2)
+Tds="now-24h"    # Time according to dnssec-policy that DS will be OMNIPRESENT
+Tkey="now-3900s" # DNSKEY TTL + propagation delay
+Tsig="now-12h"   # Zone's maximum TTL + propagation delay
+$SETTIME -P $Tkey -P sync $Tds -A $Tkey "$KSK" > settime.out.$zone.1 2>&1
+$SETTIME -P $Tsig -A $Tsig "$ZSK" > settime.out.$zone.2 2>&1
+cat template.db.in "${KSK}.key" "${ZSK}.key" > "$infile"
+private_type_record $zone 5 "$KSK" >> "$infile"
+private_type_record $zone 5 "$ZSK" >> "$infile"
+$SIGNER -S -x -s now-1h -e now+2w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
+
 #
 # The zones at algorithm-roll.kasp represent the various steps of a ZSK/KSK
 # algorithm rollover.
diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh
index b403fc2101..ed24aedf58 100644
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -2917,6 +2917,58 @@ dnssec_verify
 _migrate_ksk=$(key_get KEY1 ID)
 _migrate_zsk=$(key_get KEY2 ID)
 
+#
+# Testing migration with unmatched existing keys.
+#
+set_zone "migrate-nomatch.kasp"
+set_policy "none" "2" "300"
+set_server "ns6" "10.53.0.6"
+
+init_migration_nomatch() {
+	key_clear        "KEY1"
+	key_set          "KEY1" "LEGACY" "yes"
+	set_keyrole      "KEY1" "ksk"
+	set_keyalgorithm "KEY1" "5" "RSASHA1" "1024"
+	set_keysigning   "KEY1" "yes"
+	set_zonesigning  "KEY1" "no"
+
+	key_clear        "KEY2"
+	key_set          "KEY2" "LEGACY" "yes"
+	set_keyrole      "KEY2" "zsk"
+	set_keyalgorithm "KEY2" "5" "RSASHA1" "1024"
+	set_keysigning   "KEY2" "no"
+	set_zonesigning  "KEY2" "yes"
+
+	key_clear        "KEY3"
+	key_clear        "KEY4"
+
+	set_keytime  "KEY1" "PUBLISHED"    "yes"
+	set_keytime  "KEY1" "ACTIVE"       "yes"
+	set_keytime  "KEY1" "RETIRED"      "none"
+	set_keystate "KEY1" "GOAL"         "omnipresent"
+	set_keystate "KEY1" "STATE_DNSKEY" "omnipresent"
+	set_keystate "KEY1" "STATE_KRRSIG" "omnipresent"
+	set_keystate "KEY1" "STATE_DS"     "omnipresent"
+
+	set_keytime  "KEY2" "PUBLISHED"    "yes"
+	set_keytime  "KEY2" "ACTIVE"       "yes"
+	set_keytime  "KEY2" "RETIRED"      "none"
+	set_keystate "KEY2" "GOAL"         "omnipresent"
+	set_keystate "KEY2" "STATE_DNSKEY" "omnipresent"
+	set_keystate "KEY2" "STATE_ZRRSIG" "omnipresent"
+}
+init_migration_nomatch
+
+# Make sure the zone is signed with legacy keys.
+check_keys
+check_apex
+check_subdomain
+dnssec_verify
+
+# Remember legacy key tags.
+_migratenomatch_ksk=$(key_get KEY1 ID)
+_migratenomatch_zsk=$(key_get KEY2 ID)
+
 # Reconfig dnssec-policy (triggering algorithm roll and other dnssec-policy
 # changes).
 echo_i "reconfig dnssec-policy to trigger algorithm rollover"
@@ -2980,6 +3032,64 @@ ret=0
 [ $_migrate_zsk == $(key_get KEY2 ID) ] || log_error "mismatch zsk tag"
 status=$((status+ret))
 
+# Test migration to dnssec-policy, existing keys do not match.
+set_zone "migrate-nomatch.kasp"
+set_policy "migrate-nomatch" "4" "300"
+set_server "ns6" "10.53.0.6"
+
+# The legacy keys need to be retired, but otherwise stay present until the
+# new keys are omnipresent, and can be used to construct a chain of trust.
+init_migration_nomatch
+
+key_set      "KEY1" "LEGACY"  "no"
+set_keytime  "KEY1" "RETIRED" "yes"
+set_keystate "KEY1" "GOAL"    "hidden"
+
+key_set      "KEY2" "LEGACY"  "no"
+set_keytime  "KEY2" "RETIRED" "yes"
+set_keystate "KEY2" "GOAL"    "hidden"
+
+set_keyrole      "KEY3" "ksk"
+set_keylifetime  "KEY3" "0"
+set_keyalgorithm "KEY3" "5" "RSASHA1" "2048"
+set_keysigning   "KEY3" "yes"
+set_zonesigning  "KEY3" "no"
+
+set_keyrole      "KEY4" "zsk"
+set_keylifetime  "KEY4" "5184000"
+set_keyalgorithm "KEY4" "5" "RSASHA1" "2048"
+set_keysigning   "KEY4" "no"
+# This key is not active yet, first the DNSKEY needs to be omnipresent.
+set_zonesigning  "KEY4" "no"
+
+set_keytime  "KEY3" "PUBLISHED"    "yes"
+set_keytime  "KEY3" "ACTIVE"       "yes"
+set_keytime  "KEY3" "RETIRED"      "none"
+set_keystate "KEY3" "GOAL"         "omnipresent"
+set_keystate "KEY3" "STATE_DNSKEY" "rumoured"
+set_keystate "KEY3" "STATE_KRRSIG" "rumoured"
+set_keystate "KEY3" "STATE_DS"     "hidden"
+
+set_keytime  "KEY4" "PUBLISHED"    "yes"
+set_keytime  "KEY4" "ACTIVE"       "yes"
+set_keytime  "KEY4" "RETIRED"      "yes"
+set_keystate "KEY4" "GOAL"         "omnipresent"
+set_keystate "KEY4" "STATE_DNSKEY" "rumoured"
+set_keystate "KEY4" "STATE_ZRRSIG" "hidden"
+
+check_keys
+check_apex
+check_subdomain
+dnssec_verify
+
+# Check key tags, should be the same.
+n=$((n+1))
+echo_i "check that of zone ${ZONE} migration to dnssec-policy keeps existing keys ($n)"
+ret=0
+[ $_migratenomatch_ksk == $(key_get KEY1 ID) ] || log_error "mismatch ksk tag"
+[ $_migratenomatch_zsk == $(key_get KEY2 ID) ] || log_error "mismatch zsk tag"
+status=$((status+ret))
+
 #
 # Testing KSK/ZSK algorithm rollover.
 #
diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index e5d7f08edd..268db30d2b 100644
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -1228,7 +1228,7 @@ transition:
  *
  */
 static void
-keymgr_key_init(dns_dnsseckey_t *key, isc_stdtime_t now) {
+keymgr_key_init(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) {
 	bool ksk, zsk;
 	isc_result_t ret;
 	isc_stdtime_t active = 0, pub = 0, syncpub = 0;
@@ -1254,15 +1254,34 @@ keymgr_key_init(dns_dnsseckey_t *key, isc_stdtime_t now) {
 	/* Get time metadata. */
 	ret = dst_key_gettime(key->key, DST_TIME_ACTIVATE, &active);
 	if (active <= now && ret == ISC_R_SUCCESS) {
-		dnskey_state = RUMOURED;
+		dns_ttl_t key_ttl = dst_key_getttl(key->key);
+		key_ttl += dns_kasp_zonepropagationdelay(kasp);
+		if ((active + key_ttl) <= now) {
+			dnskey_state = OMNIPRESENT;
+		} else {
+			dnskey_state = RUMOURED;
+		}
 	}
 	ret = dst_key_gettime(key->key, DST_TIME_PUBLISH, &pub);
 	if (pub <= now && ret == ISC_R_SUCCESS) {
-		zrrsig_state = RUMOURED;
+		dns_ttl_t zone_ttl = dns_kasp_zonemaxttl(kasp);
+		zone_ttl += dns_kasp_zonepropagationdelay(kasp);
+		if ((pub + zone_ttl) <= now) {
+			zrrsig_state = OMNIPRESENT;
+		} else {
+			zrrsig_state = RUMOURED;
+		}
 	}
 	ret = dst_key_gettime(key->key, DST_TIME_SYNCPUBLISH, &syncpub);
 	if (syncpub <= now && ret == ISC_R_SUCCESS) {
-		ds_state = RUMOURED;
+		dns_ttl_t ds_ttl = dns_kasp_dsttl(kasp);
+		ds_ttl += dns_kasp_parentregistrationdelay(kasp);
+		ds_ttl += dns_kasp_parentpropagationdelay(kasp);
+		if ((syncpub + ds_ttl) <= now) {
+			ds_state = OMNIPRESENT;
+		} else {
+			ds_state = RUMOURED;
+		}
 	}
 
 	/* Set key states for all keys that do not have them. */
@@ -1342,7 +1361,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 	{
 		bool found_match = false;
 
-		keymgr_key_init(dkey, now);
+		keymgr_key_init(dkey, kasp, now);
 
 		for (kkey = ISC_LIST_HEAD(dns_kasp_keys(kasp)); kkey != NULL;
 		     kkey = ISC_LIST_NEXT(kkey, link))
@@ -1511,7 +1530,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 			dst_key_setttl(dst_key, dns_kasp_dnskeyttl(kasp));
 			dst_key_settime(dst_key, DST_TIME_CREATED, now);
 			RETERR(dns_dnsseckey_create(mctx, &dst_key, &newkey));
-			keymgr_key_init(newkey, now);
+			keymgr_key_init(newkey, kasp, now);
 		} else {
 			newkey = candidate;
 			dst_key_setnum(newkey->key, DST_NUM_LIFETIME, lifetime);

From f47e697da3ef468b663f43f219e352a2300c84d7 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Wed, 1 Apr 2020 14:35:22 +0200
Subject: [PATCH 4/6] Update documentation with !1706 fix

---
 CHANGES                  | 3 +++
 doc/arm/notes-9.17.1.xml | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/CHANGES b/CHANGES
index a041b0d5ac..52981c221f 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,6 @@
+5372.	[bug]		Fix migration from existing DNSSEC key files using
+			auto-dnssec maintain to dnssec-policy. [GL #1706]
+
 5371.	[bug]		Improve incremental updates of the RPZ summary
 			database to reduce delays that could occur when
 			a policy zone update included a large number of
diff --git a/doc/arm/notes-9.17.1.xml b/doc/arm/notes-9.17.1.xml
index 1e6c5a28b2..aaad722969 100644
--- a/doc/arm/notes-9.17.1.xml
+++ b/doc/arm/notes-9.17.1.xml
@@ -63,6 +63,14 @@
 	  reducing such delays. [GL #1447]
         </para>
       </listitem>
+      <listitem>
+        <para>
+	  Migration to dnssec-policy from existing DNSSEC strategy with
+	  auto-dnssec maintain did not work due to bad initializing of the
+	  key states.  Fixed by looking closely at the time metadata to
+	  set the key states to the correct values. [GL #1706]
+        </para>
+      </listitem>
     </itemizedlist>
   </section>
 

From 2389fcb4dcb55a1598a3b8b0f48e8d9ddfb429cf Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Wed, 1 Apr 2020 16:35:06 +0200
Subject: [PATCH 5/6] Only initialize goal on active keys

If we initialize goals on all keys, superfluous keys that match
the policy all desire to be active.  For example, there are six
keys available for a policy that needs just two, we only want to
set the goal state to OMNIPRESENT on two keys, not six.
---
 bin/tests/system/kasp/ns6/named.conf.in      |  4 ++--
 bin/tests/system/kasp/ns6/named2.conf.in     |  6 ++---
 bin/tests/system/kasp/ns6/policies/kasp.conf |  6 ++++-
 bin/tests/system/kasp/ns6/setup.sh           |  6 +++--
 bin/tests/system/kasp/tests.sh               | 22 +++++++++----------
 lib/dns/keymgr.c                             | 23 ++++++++++++--------
 6 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/bin/tests/system/kasp/ns6/named.conf.in b/bin/tests/system/kasp/ns6/named.conf.in
index 5cecb8c542..f9aa284ef4 100644
--- a/bin/tests/system/kasp/ns6/named.conf.in
+++ b/bin/tests/system/kasp/ns6/named.conf.in
@@ -46,9 +46,9 @@ zone "migrate.kasp" {
 	update-check-ksk yes;
 };
 
-zone "migrate-nomatch.kasp" {
+zone "migrate-nomatch-alglen.kasp" {
 	type master;
-	file "migrate-nomatch.kasp.db";
+	file "migrate-nomatch-alglen.kasp.db";
 	auto-dnssec maintain;
 	allow-update { any; };
 	dnssec-dnskey-kskonly yes;
diff --git a/bin/tests/system/kasp/ns6/named2.conf.in b/bin/tests/system/kasp/ns6/named2.conf.in
index 0428dcbdf8..d63318c6ac 100644
--- a/bin/tests/system/kasp/ns6/named2.conf.in
+++ b/bin/tests/system/kasp/ns6/named2.conf.in
@@ -43,11 +43,11 @@ zone "migrate.kasp" {
 	dnssec-policy "migrate";
 };
 
-zone "migrate-nomatch.kasp" {
+zone "migrate-nomatch-alglen.kasp" {
 	type master;
-	file "migrate-nomatch.kasp.db";
+	file "migrate-nomatch-alglen.kasp.db";
 	allow-update { any; };
-	dnssec-policy "migrate-nomatch";
+	dnssec-policy "migrate-nomatch-alglen";
 };
 
 /*
diff --git a/bin/tests/system/kasp/ns6/policies/kasp.conf b/bin/tests/system/kasp/ns6/policies/kasp.conf
index 16fb3d60aa..ae36e0934f 100644
--- a/bin/tests/system/kasp/ns6/policies/kasp.conf
+++ b/bin/tests/system/kasp/ns6/policies/kasp.conf
@@ -58,7 +58,11 @@ dnssec-policy "migrate" {
 	};
 };
 
-dnssec-policy "migrate-nomatch" {
+/*
+ * This policy tests migration from existing keys with 1024 bits RSASHA1 keys
+ * to 2048 bits RSASHA1 keys.
+ */
+dnssec-policy "migrate-nomatch-alglen" {
 	dnskey-ttl 300;
 
 	keys {
diff --git a/bin/tests/system/kasp/ns6/setup.sh b/bin/tests/system/kasp/ns6/setup.sh
index 5c489e774f..7a9ed929c1 100644
--- a/bin/tests/system/kasp/ns6/setup.sh
+++ b/bin/tests/system/kasp/ns6/setup.sh
@@ -52,8 +52,10 @@ private_type_record $zone 5 "$ZSK" >> "$infile"
 $SIGNER -S -x -s now-1h -e now+2w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
 
 # Set up a zone with auto-dnssec maintain to migrate to dnssec-policy, but this
-# time the existing keys do not match the policy.
-setup migrate-nomatch.kasp
+# time the existing keys do not match the policy.  The existing keys are
+# 1024 bits RSASHA1 keys, and will be migrated to a dnssec-policy that
+# dictates 2048 bits RSASHA1 keys.
+setup migrate-nomatch-alglen.kasp
 echo "$zone" >> zones
 KSK=$($KEYGEN -a RSASHA1 -b 1024 -f KSK -L 300 $zone 2> keygen.out.$zone.1)
 ZSK=$($KEYGEN -a RSASHA1 -b 1024 -L 300 $zone 2> keygen.out.$zone.2)
diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh
index ed24aedf58..50de840ad6 100644
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -2920,11 +2920,11 @@ _migrate_zsk=$(key_get KEY2 ID)
 #
 # Testing migration with unmatched existing keys.
 #
-set_zone "migrate-nomatch.kasp"
+set_zone "migrate-nomatch-alglen.kasp"
 set_policy "none" "2" "300"
 set_server "ns6" "10.53.0.6"
 
-init_migration_nomatch() {
+init_migration_nomatch_alglen() {
 	key_clear        "KEY1"
 	key_set          "KEY1" "LEGACY" "yes"
 	set_keyrole      "KEY1" "ksk"
@@ -2957,7 +2957,7 @@ init_migration_nomatch() {
 	set_keystate "KEY2" "STATE_DNSKEY" "omnipresent"
 	set_keystate "KEY2" "STATE_ZRRSIG" "omnipresent"
 }
-init_migration_nomatch
+init_migration_nomatch_alglen
 
 # Make sure the zone is signed with legacy keys.
 check_keys
@@ -2966,8 +2966,8 @@ check_subdomain
 dnssec_verify
 
 # Remember legacy key tags.
-_migratenomatch_ksk=$(key_get KEY1 ID)
-_migratenomatch_zsk=$(key_get KEY2 ID)
+_migratenomatch_alglen_ksk=$(key_get KEY1 ID)
+_migratenomatch_alglen_zsk=$(key_get KEY2 ID)
 
 # Reconfig dnssec-policy (triggering algorithm roll and other dnssec-policy
 # changes).
@@ -3033,13 +3033,13 @@ ret=0
 status=$((status+ret))
 
 # Test migration to dnssec-policy, existing keys do not match.
-set_zone "migrate-nomatch.kasp"
-set_policy "migrate-nomatch" "4" "300"
+set_zone "migrate-nomatch-alglen.kasp"
+set_policy "migrate-nomatch-alglen" "4" "300"
 set_server "ns6" "10.53.0.6"
 
 # The legacy keys need to be retired, but otherwise stay present until the
 # new keys are omnipresent, and can be used to construct a chain of trust.
-init_migration_nomatch
+init_migration_nomatch_alglen
 
 key_set      "KEY1" "LEGACY"  "no"
 set_keytime  "KEY1" "RETIRED" "yes"
@@ -3059,7 +3059,7 @@ set_keyrole      "KEY4" "zsk"
 set_keylifetime  "KEY4" "5184000"
 set_keyalgorithm "KEY4" "5" "RSASHA1" "2048"
 set_keysigning   "KEY4" "no"
-# This key is not active yet, first the DNSKEY needs to be omnipresent.
+# This key is considered to be prepublished, so it is not yet signing.
 set_zonesigning  "KEY4" "no"
 
 set_keytime  "KEY3" "PUBLISHED"    "yes"
@@ -3086,8 +3086,8 @@ dnssec_verify
 n=$((n+1))
 echo_i "check that of zone ${ZONE} migration to dnssec-policy keeps existing keys ($n)"
 ret=0
-[ $_migratenomatch_ksk == $(key_get KEY1 ID) ] || log_error "mismatch ksk tag"
-[ $_migratenomatch_zsk == $(key_get KEY2 ID) ] || log_error "mismatch zsk tag"
+[ $_migratenomatch_alglen_ksk == $(key_get KEY1 ID) ] || log_error "mismatch ksk tag"
+[ $_migratenomatch_alglen_zsk == $(key_get KEY2 ID) ] || log_error "mismatch zsk tag"
 status=$((status+ret))
 
 #
diff --git a/lib/dns/keymgr.c b/lib/dns/keymgr.c
index 268db30d2b..09ba4dc9c4 100644
--- a/lib/dns/keymgr.c
+++ b/lib/dns/keymgr.c
@@ -1402,7 +1402,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 					      keystr, keymgr_keyrole(dkey->key),
 					      dns_kasp_getname(kasp));
 
-				/* Initialize lifetime and goal, if not set. */
+				/* Initialize lifetime if not set. */
 				uint32_t l;
 				if (dst_key_getnum(dkey->key, DST_NUM_LIFETIME,
 						   &l) != ISC_R_SUCCESS) {
@@ -1411,14 +1411,6 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 						       lifetime);
 				}
 
-				dst_key_state_t goal;
-				if (dst_key_getstate(dkey->key, DST_KEY_GOAL,
-						     &goal) != ISC_R_SUCCESS) {
-					dst_key_setstate(dkey->key,
-							 DST_KEY_GOAL,
-							 OMNIPRESENT);
-				}
-
 				if (active_key) {
 					/* We already have an active key that
 					 * matches the kasp policy.
@@ -1442,6 +1434,19 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 					continue;
 				}
 
+				/*
+				 * This is possibly an active key created
+				 * outside dnssec-policy.  Initialize goal,
+				 * if not set.
+				 */
+				dst_key_state_t goal;
+				if (dst_key_getstate(dkey->key, DST_KEY_GOAL,
+						     &goal) != ISC_R_SUCCESS) {
+					dst_key_setstate(dkey->key,
+							 DST_KEY_GOAL,
+							 OMNIPRESENT);
+				}
+
 				/*
 				 * Save the matched key only if it is active
 				 * or desires to be active.

From 551acb44f41a599937878a0226beb979676da23a Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 2 Apr 2020 09:05:12 +0200
Subject: [PATCH 6/6] Test migration to dnssec-policy, change algorithm

Add a test to ensure migration from 'auto-dnssec maintain;' to
dnssec-policy works even if the algorithm is changed.  The existing
keys should not be removed immediately, but their goal should be
changed to become hidden, and the new keys with the different
algorithm should be introduced immediately.
---
 bin/tests/system/kasp/ns6/named.conf.in      |   9 ++
 bin/tests/system/kasp/ns6/named2.conf.in     |   7 ++
 bin/tests/system/kasp/ns6/policies/kasp.conf |  22 ++++
 bin/tests/system/kasp/ns6/setup.sh           |  18 +++
 bin/tests/system/kasp/tests.sh               | 113 ++++++++++++++++++-
 5 files changed, 167 insertions(+), 2 deletions(-)

diff --git a/bin/tests/system/kasp/ns6/named.conf.in b/bin/tests/system/kasp/ns6/named.conf.in
index f9aa284ef4..943c95f120 100644
--- a/bin/tests/system/kasp/ns6/named.conf.in
+++ b/bin/tests/system/kasp/ns6/named.conf.in
@@ -46,6 +46,15 @@ zone "migrate.kasp" {
 	update-check-ksk yes;
 };
 
+zone "migrate-nomatch-algnum.kasp" {
+	type master;
+	file "migrate-nomatch-algnum.kasp.db";
+	auto-dnssec maintain;
+	allow-update { any; };
+	dnssec-dnskey-kskonly yes;
+	update-check-ksk yes;
+};
+
 zone "migrate-nomatch-alglen.kasp" {
 	type master;
 	file "migrate-nomatch-alglen.kasp.db";
diff --git a/bin/tests/system/kasp/ns6/named2.conf.in b/bin/tests/system/kasp/ns6/named2.conf.in
index d63318c6ac..4405939321 100644
--- a/bin/tests/system/kasp/ns6/named2.conf.in
+++ b/bin/tests/system/kasp/ns6/named2.conf.in
@@ -43,6 +43,13 @@ zone "migrate.kasp" {
 	dnssec-policy "migrate";
 };
 
+zone "migrate-nomatch-algnum.kasp" {
+	type master;
+	file "migrate-nomatch-algnum.kasp.db";
+	allow-update { any; };
+	dnssec-policy "migrate-nomatch-algnum";
+};
+
 zone "migrate-nomatch-alglen.kasp" {
 	type master;
 	file "migrate-nomatch-alglen.kasp.db";
diff --git a/bin/tests/system/kasp/ns6/policies/kasp.conf b/bin/tests/system/kasp/ns6/policies/kasp.conf
index ae36e0934f..6b58eaf825 100644
--- a/bin/tests/system/kasp/ns6/policies/kasp.conf
+++ b/bin/tests/system/kasp/ns6/policies/kasp.conf
@@ -58,6 +58,28 @@ dnssec-policy "migrate" {
 	};
 };
 
+/*
+ * This policy tests migration from existing keys with 1024 bits RSASHA1 keys
+ * to ECDSAP256SHA256 keys.
+ */
+dnssec-policy "migrate-nomatch-algnum" {
+	dnskey-ttl 300;
+
+	keys {
+		ksk key-directory lifetime unlimited algorithm ecdsa256;
+		zsk key-directory lifetime P60D algorithm ecdsa256;
+	};
+
+	// Together 12h
+	zone-propagation-delay 3600;
+	max-zone-ttl 11h;
+
+	// Together 24h
+	parent-registration-delay 21h;
+	parent-propagation-delay pt1h;
+	parent-ds-ttl 7200;
+};
+
 /*
  * This policy tests migration from existing keys with 1024 bits RSASHA1 keys
  * to 2048 bits RSASHA1 keys.
diff --git a/bin/tests/system/kasp/ns6/setup.sh b/bin/tests/system/kasp/ns6/setup.sh
index 7a9ed929c1..430e1c9e3e 100644
--- a/bin/tests/system/kasp/ns6/setup.sh
+++ b/bin/tests/system/kasp/ns6/setup.sh
@@ -51,6 +51,24 @@ private_type_record $zone 5 "$KSK" >> "$infile"
 private_type_record $zone 5 "$ZSK" >> "$infile"
 $SIGNER -S -x -s now-1h -e now+2w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
 
+# Set up a zone with auto-dnssec maintain to migrate to dnssec-policy, but this
+# time the existing keys do not match the policy.  The existing keys are
+# RSASHA1 keys, and will be migrated to a dnssec-policy that dictates
+# ECDSAP256SHA256 keys.
+setup migrate-nomatch-algnum.kasp
+echo "$zone" >> zones
+KSK=$($KEYGEN -a RSASHA1 -b 2048 -f KSK -L 300 $zone 2> keygen.out.$zone.1)
+ZSK=$($KEYGEN -a RSASHA1 -b 1024 -L 300 $zone 2> keygen.out.$zone.2)
+Tds="now-24h"    # Time according to dnssec-policy that DS will be OMNIPRESENT
+Tkey="now-3900s" # DNSKEY TTL + propagation delay
+Tsig="now-12h"   # Zone's maximum TTL + propagation delay
+$SETTIME -P $Tkey -P sync $Tds -A $Tkey "$KSK" > settime.out.$zone.1 2>&1
+$SETTIME -P $Tsig -A $Tsig "$ZSK" > settime.out.$zone.2 2>&1
+cat template.db.in "${KSK}.key" "${ZSK}.key" > "$infile"
+private_type_record $zone 5 "$KSK" >> "$infile"
+private_type_record $zone 5 "$ZSK" >> "$infile"
+$SIGNER -S -x -s now-1h -e now+2w -o $zone -O full -f $zonefile $infile > signer.out.$zone.1 2>&1
+
 # Set up a zone with auto-dnssec maintain to migrate to dnssec-policy, but this
 # time the existing keys do not match the policy.  The existing keys are
 # 1024 bits RSASHA1 keys, and will be migrated to a dnssec-policy that
diff --git a/bin/tests/system/kasp/tests.sh b/bin/tests/system/kasp/tests.sh
index 50de840ad6..cda9d87241 100644
--- a/bin/tests/system/kasp/tests.sh
+++ b/bin/tests/system/kasp/tests.sh
@@ -2918,7 +2918,59 @@ _migrate_ksk=$(key_get KEY1 ID)
 _migrate_zsk=$(key_get KEY2 ID)
 
 #
-# Testing migration with unmatched existing keys.
+# Testing migration with unmatched existing keys (different algorithm).
+#
+set_zone "migrate-nomatch-algnum.kasp"
+set_policy "none" "2" "300"
+set_server "ns6" "10.53.0.6"
+
+init_migration_nomatch_algnum() {
+	key_clear        "KEY1"
+	key_set          "KEY1" "LEGACY" "yes"
+	set_keyrole      "KEY1" "ksk"
+	set_keyalgorithm "KEY1" "5" "RSASHA1" "2048"
+	set_keysigning   "KEY1" "yes"
+	set_zonesigning  "KEY1" "no"
+
+	key_clear        "KEY2"
+	key_set          "KEY2" "LEGACY" "yes"
+	set_keyrole      "KEY2" "zsk"
+	set_keyalgorithm "KEY2" "5" "RSASHA1" "1024"
+	set_keysigning   "KEY2" "no"
+	set_zonesigning  "KEY2" "yes"
+
+	key_clear        "KEY3"
+	key_clear        "KEY4"
+
+	set_keytime  "KEY1" "PUBLISHED"    "yes"
+	set_keytime  "KEY1" "ACTIVE"       "yes"
+	set_keytime  "KEY1" "RETIRED"      "none"
+	set_keystate "KEY1" "GOAL"         "omnipresent"
+	set_keystate "KEY1" "STATE_DNSKEY" "omnipresent"
+	set_keystate "KEY1" "STATE_KRRSIG" "omnipresent"
+	set_keystate "KEY1" "STATE_DS"     "omnipresent"
+
+	set_keytime  "KEY2" "PUBLISHED"    "yes"
+	set_keytime  "KEY2" "ACTIVE"       "yes"
+	set_keytime  "KEY2" "RETIRED"      "none"
+	set_keystate "KEY2" "GOAL"         "omnipresent"
+	set_keystate "KEY2" "STATE_DNSKEY" "omnipresent"
+	set_keystate "KEY2" "STATE_ZRRSIG" "omnipresent"
+}
+init_migration_nomatch_algnum
+
+# Make sure the zone is signed with legacy keys.
+check_keys
+check_apex
+check_subdomain
+dnssec_verify
+
+# Remember legacy key tags.
+_migratenomatch_algnum_ksk=$(key_get KEY1 ID)
+_migratenomatch_algnum_zsk=$(key_get KEY2 ID)
+
+#
+# Testing migration with unmatched existing keys (different length).
 #
 set_zone "migrate-nomatch-alglen.kasp"
 set_policy "none" "2" "300"
@@ -3032,7 +3084,64 @@ ret=0
 [ $_migrate_zsk == $(key_get KEY2 ID) ] || log_error "mismatch zsk tag"
 status=$((status+ret))
 
-# Test migration to dnssec-policy, existing keys do not match.
+# Test migration to dnssec-policy, existing keys do not match key algorithm.
+set_zone "migrate-nomatch-algnum.kasp"
+set_policy "migrate-nomatch-algnum" "4" "300"
+set_server "ns6" "10.53.0.6"
+
+# The legacy keys need to be retired, but otherwise stay present until the
+# new keys are omnipresent, and can be used to construct a chain of trust.
+init_migration_nomatch_algnum
+
+key_set      "KEY1" "LEGACY"  "no"
+set_keytime  "KEY1" "RETIRED" "yes"
+set_keystate "KEY1" "GOAL"    "hidden"
+
+key_set      "KEY2" "LEGACY"  "no"
+set_keytime  "KEY2" "RETIRED" "yes"
+set_keystate "KEY2" "GOAL"    "hidden"
+
+set_keyrole      "KEY3" "ksk"
+set_keylifetime  "KEY3" "0"
+set_keyalgorithm "KEY3" "13" "ECDSAP256SHA256" "256"
+set_keysigning   "KEY3" "yes"
+set_zonesigning  "KEY3" "no"
+
+set_keyrole      "KEY4" "zsk"
+set_keylifetime  "KEY4" "5184000"
+set_keyalgorithm "KEY4" "13" "ECDSAP256SHA256" "256"
+set_keysigning   "KEY4" "no"
+set_zonesigning  "KEY4" "yes"
+
+set_keytime  "KEY3" "PUBLISHED"    "yes"
+set_keytime  "KEY3" "ACTIVE"       "yes"
+set_keytime  "KEY3" "RETIRED"      "none"
+set_keystate "KEY3" "GOAL"         "omnipresent"
+set_keystate "KEY3" "STATE_DNSKEY" "rumoured"
+set_keystate "KEY3" "STATE_KRRSIG" "rumoured"
+set_keystate "KEY3" "STATE_DS"     "hidden"
+
+set_keytime  "KEY4" "PUBLISHED"    "yes"
+set_keytime  "KEY4" "ACTIVE"       "yes"
+set_keytime  "KEY4" "RETIRED"      "yes"
+set_keystate "KEY4" "GOAL"         "omnipresent"
+set_keystate "KEY4" "STATE_DNSKEY" "rumoured"
+set_keystate "KEY4" "STATE_ZRRSIG" "rumoured"
+
+check_keys
+check_apex
+check_subdomain
+dnssec_verify
+
+# Check key tags, should be the same.
+n=$((n+1))
+echo_i "check that of zone ${ZONE} migration to dnssec-policy keeps existing keys ($n)"
+ret=0
+[ $_migratenomatch_algnum_ksk == $(key_get KEY1 ID) ] || log_error "mismatch ksk tag"
+[ $_migratenomatch_algnum_zsk == $(key_get KEY2 ID) ] || log_error "mismatch zsk tag"
+status=$((status+ret))
+
+# Test migration to dnssec-policy, existing keys do not match key length.
 set_zone "migrate-nomatch-alglen.kasp"
 set_policy "migrate-nomatch-alglen" "4" "300"
 set_server "ns6" "10.53.0.6"