Set keytimes appropriately when using kasp

While kasp relies on key states to determine when a key needs to
be published or be used for signing, the keytimes are used by
operators to get some expectation of key publication and usage.

Update the code such that these keytimes are set appropriately.
That means:
- Print "PublishCDS" and "DeleteCDS" times in the state files.
- The keymgr sets the "Removed" and "PublishCDS" times and derives
  those from the dnssec-policy.
- Tweak setting of the "Retired" time, when retiring keys, only
  update the time to now when the retire time is not yet set, or is
  in the future.

This also fixes a bug in "keymgr_transition_time" where we may wait
too long before zone signatrues become omnipresent or hidden. Not
only can we skip waiting the sign delay Dsgn if there is no
predecessor, we can also skip it if there is no successor.

Finally, this commit moves setting the lifetime, reducing two calls
to one.

lib/dns/dst_api.c

@@ -2021,6 +2021,8 @@ write_key_state(const dst_key_t *key, int type, const char *directory) {
 		printtime(key, DST_TIME_INACTIVE, "Retired", fp);
 		printtime(key, DST_TIME_REVOKE, "Revoked", fp);
 		printtime(key, DST_TIME_DELETE, "Removed", fp);
+		printtime(key, DST_TIME_SYNCPUBLISH, "PublishCDS", fp);
+		printtime(key, DST_TIME_SYNCDELETE, "DeleteCDS", fp);
 
 		printtime(key, DST_TIME_DNSKEY, "DNSKEYChange", fp);
 		printtime(key, DST_TIME_ZRRSIG, "ZRRSIGChange", fp);

lib/dns/keymgr.c

@@ -88,12 +88,91 @@ keymgr_keyrole(dst_key_t *key) {
 	return ("NOSIGN");
 }
 
+/*
+ * Set the remove time on key given its retire time.
+ *
+ */
+static void
+keymgr_settime_remove(dns_dnsseckey_t *key, dns_kasp_t *kasp) {
+	isc_stdtime_t retire = 0, remove = 0, ksk_remove = 0, zsk_remove = 0;
+	bool zsk = false, ksk = false;
+	isc_result_t ret;
+
+	REQUIRE(key != NULL);
+	REQUIRE(key->key != NULL);
+
+	ret = dst_key_gettime(key->key, DST_TIME_INACTIVE, &retire);
+	if (ret != ISC_R_SUCCESS) {
+		return;
+	}
+
+	ret = dst_key_getbool(key->key, DST_BOOL_ZSK, &zsk);
+	if (ret == ISC_R_SUCCESS && zsk) {
+		/* ZSK: Iret = Dsgn + Dprp + TTLsig */
+		zsk_remove = retire + dns_kasp_zonemaxttl(kasp) +
+			     dns_kasp_zonepropagationdelay(kasp) +
+			     dns_kasp_retiresafety(kasp) +
+			     dns_kasp_signdelay(kasp);
+	}
+	ret = dst_key_getbool(key->key, DST_BOOL_KSK, &ksk);
+	if (ret == ISC_R_SUCCESS && ksk) {
+		/* KSK: Iret = DprpP + TTLds */
+		ksk_remove = retire + dns_kasp_dsttl(kasp) +
+			     dns_kasp_parentregistrationdelay(kasp) +
+			     dns_kasp_parentpropagationdelay(kasp) +
+			     dns_kasp_retiresafety(kasp);
+	}
+	remove = ksk_remove > zsk_remove ? ksk_remove : zsk_remove;
+	dst_key_settime(key->key, DST_TIME_DELETE, remove);
+}
+
+/*
+ * Set the SyncPublish time (when the DS may be submitted to the parent)
+ *
+ */
+static void
+keymgr_settime_syncpublish(dns_dnsseckey_t *key, dns_kasp_t *kasp, bool first) {
+	isc_stdtime_t published, syncpublish;
+	bool ksk = false;
+	isc_result_t ret;
+
+	REQUIRE(key != NULL);
+	REQUIRE(key->key != NULL);
+
+	ret = dst_key_gettime(key->key, DST_TIME_PUBLISH, &published);
+	if (ret != ISC_R_SUCCESS) {
+		return;
+	}
+
+	ret = dst_key_getbool(key->key, DST_BOOL_KSK, &ksk);
+	if (ret != ISC_R_SUCCESS || !ksk) {
+		return;
+	}
+
+	syncpublish = published + dst_key_getttl(key->key) +
+		      dns_kasp_zonepropagationdelay(kasp) +
+		      dns_kasp_publishsafety(kasp);
+	if (first) {
+		/* Also need to wait until the signatures are omnipresent. */
+		isc_stdtime_t zrrsig_present;
+		zrrsig_present = published + dns_kasp_zonemaxttl(kasp) +
+				 dns_kasp_zonepropagationdelay(kasp) +
+				 dns_kasp_publishsafety(kasp);
+		if (zrrsig_present > syncpublish) {
+			syncpublish = zrrsig_present;
+		}
+	}
+	dst_key_settime(key->key, DST_TIME_SYNCPUBLISH, syncpublish);
+}
+
 /*
  * Calculate prepublication time of a successor key of 'key'.
  * This function can have side effects:
- * If the lifetime is not set, it will be set now.
+ * 1. If there is no active time set, which would be super weird, set it now.
- * If there should be a retire time and it is not set, it will be set now.
+ * 2. If there is no published time set, also super weird, set it now.
- * If there is no active time set, which would be super weird, set it now.
+ * 3. If the lifetime is not set, it will be set now.
+ * 4. If there should be a retire time and it is not set, it will be set now.
+ * 5. The removed time is adjusted accordingly.
  *
  * This returns when the successor key needs to be published in the zone.
  * A special value of 0 means there is no need for a successor.
@@ -156,6 +235,11 @@ keymgr_prepublication_time(dns_dnsseckey_t *key, dns_kasp_t *kasp,
 		dst_key_settime(key->key, DST_TIME_INACTIVE, retire);
 	}
 
+	/*
+	 * Update remove time.
+	 */
+	keymgr_settime_remove(key, kasp);
+
 	/*
 	 * Publish successor 'prepub' time before the 'retire' time of 'key'.
 	 */
@@ -163,8 +247,10 @@ keymgr_prepublication_time(dns_dnsseckey_t *key, dns_kasp_t *kasp,
 }
 
 static void
-keymgr_key_retire(dns_dnsseckey_t *key, isc_stdtime_t now) {
+keymgr_key_retire(dns_dnsseckey_t *key, dns_kasp_t *kasp, isc_stdtime_t now) {
 	char keystr[DST_KEY_FORMATSIZE];
+	isc_result_t ret;
+	isc_stdtime_t retire;
 	dst_key_state_t s;
 	bool ksk, zsk;
 
@@ -172,8 +258,12 @@ keymgr_key_retire(dns_dnsseckey_t *key, isc_stdtime_t now) {
 	REQUIRE(key->key != NULL);
 
 	/* This key wants to retire and hide in a corner. */
-	dst_key_settime(key->key, DST_TIME_INACTIVE, now);
+	ret = dst_key_gettime(key->key, DST_TIME_INACTIVE, &retire);
+	if (ret != ISC_R_SUCCESS || (retire > now)) {
+		dst_key_settime(key->key, DST_TIME_INACTIVE, now);
+	}
 	dst_key_setstate(key->key, DST_KEY_GOAL, HIDDEN);
+	keymgr_settime_remove(key, kasp);
 
 	/* This key may not have key states set yet. Pretend as if they are
 	 * in the OMNIPRESENT state.
@@ -1013,11 +1103,16 @@ keymgr_transition_time(dns_dnsseckey_t *key, int type,
 				   dns_kasp_retiresafety(kasp);
 			/*
 			 * Only add the sign delay Dsgn if there is an actual
-			 * predecessor key.
+			 * predecessor or successor key.
 			 */
-			uint32_t pre;
+			uint32_t tag;
-			if (dst_key_getnum(key->key, DST_NUM_PREDECESSOR,
+			ret = dst_key_getnum(key->key, DST_NUM_PREDECESSOR,
-					   &pre) == ISC_R_SUCCESS) {
+					     &tag);
+			if (ret != ISC_R_SUCCESS) {
+				ret = dst_key_getnum(key->key,
+						     DST_NUM_SUCCESSOR, &tag);
+			}
+			if (ret == ISC_R_SUCCESS) {
 				nexttime += dns_kasp_signdelay(kasp);
 			}
 			break;
@@ -1373,7 +1468,7 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 
 		/* No match, so retire unwanted retire key. */
 		if (!found_match) {
-			keymgr_key_retire(dkey, now);
+			keymgr_key_retire(dkey, kasp, now);
 		}
 	}
 
@@ -1428,7 +1523,8 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 						 * the kasp key configuration.
 						 * Retire excess keys in use.
 						 */
-						keymgr_key_retire(dkey, now);
+						keymgr_key_retire(dkey, kasp,
+								  now);
 					}
 					continue;
 				}
@@ -1537,8 +1633,8 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 			keymgr_key_init(newkey, kasp, now);
 		} else {
 			newkey = candidate;
-			dst_key_setnum(newkey->key, DST_NUM_LIFETIME, lifetime);
 		}
+		dst_key_setnum(newkey->key, DST_NUM_LIFETIME, lifetime);
 
 		/* Got a key. */
 		if (active_key == NULL) {
@@ -1548,19 +1644,38 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 			 */
 			dst_key_settime(newkey->key, DST_TIME_PUBLISH, now);
 			dst_key_settime(newkey->key, DST_TIME_ACTIVATE, now);
+			keymgr_settime_syncpublish(newkey, kasp, true);
 			active = now;
 		} else {
 			/*
 			 * This is a successor.  Mark the relationship.
 			 */
+			isc_stdtime_t created;
+			(void)dst_key_gettime(newkey->key, DST_TIME_CREATED,
+					      &created);
+
 			dst_key_setnum(newkey->key, DST_NUM_PREDECESSOR,
 				       dst_key_id(active_key->key));
 			dst_key_setnum(active_key->key, DST_NUM_SUCCESSOR,
 				       dst_key_id(newkey->key));
 			(void)dst_key_gettime(active_key->key,
 					      DST_TIME_INACTIVE, &retire);
+			/*
+			 * If prepublication time and/or retire time are
+			 * in the past (before the new key was created), use
+			 * creation time as published and active time,
+			 * effectively immediately making the key active.
+			 */
+			if (prepub < created) {
+				retire += (created - prepub);
+				prepub = created;
+			}
+			if (retire < created) {
+				retire = created;
+			}
 			dst_key_settime(newkey->key, DST_TIME_PUBLISH, prepub);
 			dst_key_settime(newkey->key, DST_TIME_ACTIVATE, retire);
+			keymgr_settime_syncpublish(newkey, kasp, false);
 			active = retire;
 		}
 
@@ -1568,10 +1683,10 @@ dns_keymgr_run(const dns_name_t *origin, dns_rdataclass_t rdclass,
 		dst_key_setstate(newkey->key, DST_KEY_GOAL, OMNIPRESENT);
 
 		/* Do we need to set retire time? */
-		(void)dst_key_getnum(newkey->key, DST_NUM_LIFETIME, &lifetime);
 		if (lifetime > 0) {
 			dst_key_settime(newkey->key, DST_TIME_INACTIVE,
 					(active + lifetime));
+			keymgr_settime_remove(newkey, kasp);
 		}
 
 		/* Append dnsseckey to list of new keys. */