Refactor KSK processing

There are multiple almost identical code blocks, time to make a
function.

diff --git a/lib/dns/include/dst/dst.h b/lib/dns/include/dst/dst.h
index 3cca998bcb603964419204e7dbb2566f8ffcdf53..c0912f3e67ad6c80674177d03704885b2f83c936 100644 (file)
--- a/lib/dns/include/dst/dst.h
+++ b/lib/dns/include/dst/dst.h
@@ -774,6 +774,25 @@ dst_key_iszonekey(const dst_key_t *key);
 bool
 dst_key_isnullkey(const dst_key_t *key);
 
+bool
+dst_key_have_ksk_and_zsk(dst_key_t **keys, unsigned int nkeys, unsigned int i,
+                        bool check_offline, bool ksk, bool zsk, bool *have_ksk,
+                        bool *have_zsk);
+/*%<
+ *
+ * Check the list of 'keys' to see if both a KSK and ZSK are present, given key
+ * 'i'. The values stored in 'ksk' and 'zsk' tell whether key 'i' is a KSK, ZSK,
+ * or both (CSK). If 'check_offline' is true, don't consider KSKs that are
+ * currently offline (e.g. their private key file is not available).
+ *
+ * Requires:
+ *\li  "keys" is not NULL.
+ *
+ * Returns:
+ *\li  true if there is one or more keys such that both the KSK and ZSK roles
+ *are covered, false otherwise.
+ */
+
 isc_result_t
 dst_key_buildfilename(const dst_key_t *key, int type, const char *directory,
                      isc_buffer_t *out);

diff --git a/lib/dns/key.c b/lib/dns/key.c
index 7d4f378e6955d1c372418696252404293c9679f8..d039ea44449c74318631e4e93fdb786afb4a8838 100644 (file)
--- a/lib/dns/key.c
+++ b/lib/dns/key.c
@@ -159,6 +159,68 @@ dst_key_isnullkey(const dst_key_t *key) {
        return (true);
 }
 
+#define REVOKE(x) ((dst_key_flags(x) & DNS_KEYFLAG_REVOKE) != 0)
+#define KSK(x)   ((dst_key_flags(x) & DNS_KEYFLAG_KSK) != 0)
+#define ID(x)    dst_key_id(x)
+#define ALG(x)   dst_key_alg(x)
+
+bool
+dst_key_have_ksk_and_zsk(dst_key_t **keys, unsigned int nkeys, unsigned int i,
+                        bool check_offline, bool ksk, bool zsk, bool *have_ksk,
+                        bool *have_zsk) {
+       bool hksk = ksk;
+       bool hzsk = zsk;
+       isc_result_t result;
+
+       REQUIRE(keys != NULL);
+
+       for (unsigned int j = 0; j < nkeys && !(hksk && hzsk); j++) {
+               if (j == i || ALG(keys[i]) != ALG(keys[j])) {
+                       continue;
+               }
+               /*
+                * Don't consider inactive keys.
+                */
+               if (dst_key_inactive(keys[j])) {
+                       continue;
+               }
+               /*
+                * Don't consider offline keys.
+                */
+               if (check_offline && !dst_key_isprivate(keys[j])) {
+                       continue;
+               }
+               if (REVOKE(keys[j])) {
+                       continue;
+               }
+
+               if (!hksk) {
+                       result = dst_key_getbool(keys[j], DST_BOOL_KSK, &hksk);
+                       if (result != ISC_R_SUCCESS) {
+                               if (KSK(keys[j])) {
+                                       hksk = true;
+                               }
+                       }
+               }
+               if (!hzsk) {
+                       result = dst_key_getbool(keys[j], DST_BOOL_ZSK, &hzsk);
+                       if (result != ISC_R_SUCCESS) {
+                               if (!KSK(keys[j])) {
+                                       hzsk = dst_key_isprivate(keys[j]);
+                               }
+                       }
+               }
+       }
+
+       if (have_ksk != NULL) {
+               *have_ksk = hksk;
+       }
+       if (have_zsk != NULL) {
+               *have_zsk = hzsk;
+       }
+       return (hksk && hzsk);
+}
+
 void
 dst_key_setbits(dst_key_t *key, uint16_t bits) {
        unsigned int maxbits;

diff --git a/lib/dns/update.c b/lib/dns/update.c
index 710e98f28e35546fb106b282cc04253b837c184e..17ebcb8e2d3c2b22092882eeeceff6aeae2c43ea 100644 (file)
--- a/lib/dns/update.c
+++ b/lib/dns/update.c
@@ -1118,7 +1118,7 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
        dns_stats_t *dnssecsignstats = dns_zone_getdnssecsignstats(zone);
        isc_buffer_t buffer;
        unsigned char data[1024]; /* XXX */
-       unsigned int i, j;
+       unsigned int i;
        bool added_sig = false;
        bool use_kasp = false;
        isc_mem_t *mctx = diff->mctx;
@@ -1164,41 +1164,14 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db,
                }
 
                if (check_ksk && !REVOKE(keys[i])) {
-                       bool have_ksk, have_nonksk;
-                       if (KSK(keys[i])) {
-                               have_ksk = true;
-                               have_nonksk = false;
-                       } else {
-                               have_ksk = false;
-                               have_nonksk = true;
-                       }
-                       for (j = 0; j < nkeys; j++) {
-                               if (j == i || ALG(keys[i]) != ALG(keys[j])) {
-                                       continue;
-                               }
-
-                               /* Don't consider inactive keys, however
-                                * the KSK may be temporary offline, so do
-                                * consider KSKs which private key files are
-                                * unavailable.
-                                */
-                               if (dst_key_inactive(keys[j])) {
-                                       continue;
-                               }
-
-                               if (REVOKE(keys[j])) {
-                                       continue;
-                               }
-                               if (KSK(keys[j])) {
-                                       have_ksk = true;
-                               } else if (dst_key_isprivate(keys[j])) {
-                                       have_nonksk = true;
-                               }
-                               both = have_ksk && have_nonksk;
-                               if (both) {
-                                       break;
-                               }
-                       }
+                       /*
+                        * Don't consider inactive keys, however the KSK may be
+                        * temporary offline, so do consider KSKs which private
+                        * key files are unavailable.
+                        */
+                       both = dst_key_have_ksk_and_zsk(
+                               keys, nkeys, i, false, KSK(keys[i]),
+                               !KSK(keys[i]), NULL, NULL);
                }
 
                if (use_kasp) {

diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index ca72af636c97bdd27f904d0401240bcee9c0a550..945346967b0b166796582473e3c9cbfe68ee8519 100644 (file)
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -6554,7 +6554,7 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
        dns_rdata_t sig_rdata = DNS_RDATA_INIT;
        unsigned char data[1024]; /* XXX */
        isc_buffer_t buffer;
-       unsigned int i, j;
+       unsigned int i;
        bool use_kasp = false;
 
        if (zone->kasp != NULL) {
@@ -6590,8 +6590,6 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
        }
 
        for (i = 0; i < nkeys; i++) {
-               bool both = false;
-
                /* Don't add signatures for offline or inactive keys */
                if (!dst_key_isprivate(keys[i])) {
                        continue;
@@ -6601,43 +6599,14 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone,
                }
 
                if (check_ksk && !REVOKE(keys[i])) {
-                       bool have_ksk, have_nonksk;
-                       if (KSK(keys[i])) {
-                               have_ksk = true;
-                               have_nonksk = false;
-                       } else {
-                               have_ksk = false;
-                               have_nonksk = true;
-                       }
-
-                       for (j = 0; j < nkeys; j++) {
-                               if (j == i || ALG(keys[i]) != ALG(keys[j])) {
-                                       continue;
-                               }
-
-                               /*
-                                * Don't consider inactive keys, however
-                                * the KSK may be temporary offline, so do
-                                * consider keys which private key files are
-                                * unavailable.
-                                */
-                               if (dst_key_inactive(keys[j])) {
-                                       continue;
-                               }
-
-                               if (REVOKE(keys[j])) {
-                                       continue;
-                               }
-                               if (KSK(keys[j])) {
-                                       have_ksk = true;
-                               } else if (dst_key_isprivate(keys[j])) {
-                                       have_nonksk = true;
-                               }
-                               both = have_ksk && have_nonksk;
-                               if (both) {
-                                       break;
-                               }
-                       }
+                       /*
+                        * Don't consider inactive keys, however the KSK may be
+                        * temporary offline, so do consider keys which private
+                        * key files are unavailable.
+                        */
+                       both = dst_key_have_ksk_and_zsk(
+                               keys, nkeys, i, false, KSK(keys[i]),
+                               !KSK(keys[i]), NULL, NULL);
                }
                if (use_kasp) {
                        /*
@@ -6648,7 +6617,6 @@ 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);
@@ -6664,55 +6632,12 @@ 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;
-                       }
+                       /*
+                        * Don't consider inactive keys or offline keys.
+                        */
+                       (void)dst_key_have_ksk_and_zsk(keys, nkeys, i, true,
+                                                      ksk, zsk, NULL,
+                                                      &have_zsk);
 
                        if (dns_rdatatype_iskeymaterial(type)) {
                                /*
@@ -9375,44 +9300,15 @@ zone_sign(dns_zone_t *zone) {
                         * Do we do KSK processing?
                         */
                        if (check_ksk && !REVOKE(zone_keys[i])) {
-                               bool have_ksk, have_nonksk;
-                               if (KSK(zone_keys[i])) {
-                                       have_ksk = true;
-                                       have_nonksk = false;
-                               } else {
-                                       have_ksk = false;
-                                       have_nonksk = true;
-                               }
-                               for (j = 0; j < nkeys; j++) {
-                                       if (j == i || (ALG(zone_keys[i]) !=
-                                                      ALG(zone_keys[j])))
-                                       {
-                                               continue;
-                                       }
-                                       /*
-                                        * Don't consider inactive keys, however
-                                        * the key may be temporary offline, so
-                                        * do consider KSKs which private key
-                                        * files are unavailable.
-                                        */
-                                       if (dst_key_inactive(zone_keys[j])) {
-                                               continue;
-                                       }
-                                       if (REVOKE(zone_keys[j])) {
-                                               continue;
-                                       }
-                                       if (KSK(zone_keys[j])) {
-                                               have_ksk = true;
-                                       } else if (dst_key_isprivate(
-                                                          zone_keys[j]))
-                                       {
-                                               have_nonksk = true;
-                                       }
-                                       both = have_ksk && have_nonksk;
-                                       if (both) {
-                                               break;
-                                       }
-                               }
+                               /*
+                                * Don't consider inactive keys, however the key
+                                * may be temporary offline, so do consider KSKs
+                                * which private key files are unavailable.
+                                */
+                               both = dst_key_have_ksk_and_zsk(
+                                       zone_keys, nkeys, i, false,
+                                       KSK(zone_keys[i]), !KSK(zone_keys[i]),
+                                       NULL, NULL);
                        }
                        if (use_kasp) {
                                /*