From 705810d577bca7d8da7ff87eae00e7e75b6ad7c9 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 26 Mar 2020 16:02:36 +0100 Subject: [PATCH 1/8] Redesign dnssec sign statistics The first attempt to add DNSSEC sign statistics was naive: for each zone we allocated 64K counters, twice. In reality each zone has at most four keys, so the new approach only has room for four keys per zone. If after a rollover more keys have signed the zone, existing keys are rotated out. The DNSSEC sign statistics has three counters per key, so twelve counters per zone. First counter is actually a key id, so it is clear what key contributed to the metrics. The second counter tracks the number of generated signatures, and the third tracks how many of those are refreshes. This means that in the zone structure we no longer need two separate references to DNSSEC sign metrics: both the resign and refresh stats are kept in a single dns_stats structure. Incrementing dnssecsignstats: Whenever a dnssecsignstat is incremented, we look up the key id to see if we already are counting metrics for this key. If so, we update the corresponding operation counter (resign or refresh). If the key is new, store the value in a new counter and increment corresponding counter. If all slots are full, we rotate the keys and overwrite the last slot with the new key. Dumping dnssecsignstats: Dumping dnssecsignstats is no longer a simple wrapper around isc_stats_dump, but uses the same principle. The difference is that rather than dumping the index (key tag) and counter, we have to look up the corresponding counter. --- CHANGES | 7 +++ bin/named/statschannel.c | 18 +++--- bin/named/zoneconf.c | 8 --- lib/dns/include/dns/stats.h | 8 ++- lib/dns/include/dns/zone.h | 17 ------ lib/dns/stats.c | 118 +++++++++++++++++++++++++++++++++--- lib/dns/update.c | 4 +- lib/dns/win32/libdns.def.in | 2 - lib/dns/zone.c | 51 +++------------- 9 files changed, 142 insertions(+), 91 deletions(-) diff --git a/CHANGES b/CHANGES index 52981c221f..dbd9318f31 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,10 @@ +5373. [bug] Collecting DNSSEC signing operations introduced by + GL #513 (change 5254) allocated counters for every + possible key id per zone which results in a lot of + wasted memory. Fix by tracking up to four keys + per zone, rotate counters when keys are replaced. + [GL #1179] + 5372. [bug] Fix migration from existing DNSSEC key files using auto-dnssec maintain to dnssec-policy. [GL #1706] diff --git a/bin/named/statschannel.c b/bin/named/statschannel.c index c3568b1216..2bbe01de8a 100644 --- a/bin/named/statschannel.c +++ b/bin/named/statschannel.c @@ -1814,7 +1814,6 @@ zone_xmlrender(dns_zone_t *zone, void *arg) { isc_stats_t *gluecachestats; dns_stats_t *rcvquerystats; dns_stats_t *dnssecsignstats; - dns_stats_t *dnssecrefreshstats; uint64_t nsstat_values[ns_statscounter_max]; uint64_t gluecachestats_values[dns_gluecachestatscounter_max]; @@ -1880,6 +1879,7 @@ zone_xmlrender(dns_zone_t *zone, void *arg) { dnssecsignstats = dns_zone_getdnssecsignstats(zone); if (dnssecsignstats != NULL) { + /* counters type="dnssec-sign"*/ TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "counters")); TRY0(xmlTextWriterWriteAttribute( @@ -1887,7 +1887,7 @@ zone_xmlrender(dns_zone_t *zone, void *arg) { ISC_XMLCHAR "dnssec-sign")); dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecsignstats, + dns_dnssecsignstats_dump(dnssecsignstats, false, dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { @@ -1896,10 +1896,8 @@ zone_xmlrender(dns_zone_t *zone, void *arg) { /* counters type="dnssec-sign"*/ TRY0(xmlTextWriterEndElement(writer)); - } - dnssecrefreshstats = dns_zone_getdnssecrefreshstats(zone); - if (dnssecrefreshstats != NULL) { + /* counters type="dnssec-refresh"*/ TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "counters")); TRY0(xmlTextWriterWriteAttribute( @@ -1907,7 +1905,7 @@ zone_xmlrender(dns_zone_t *zone, void *arg) { ISC_XMLCHAR "dnssec-refresh")); dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecrefreshstats, + dns_dnssecsignstats_dump(dnssecsignstats, true, dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { @@ -2626,7 +2624,6 @@ zone_jsonrender(dns_zone_t *zone, void *arg) { isc_stats_t *gluecachestats; dns_stats_t *rcvquerystats; dns_stats_t *dnssecsignstats; - dns_stats_t *dnssecrefreshstats; uint64_t nsstat_values[ns_statscounter_max]; uint64_t gluecachestats_values[dns_gluecachestatscounter_max]; @@ -2714,7 +2711,7 @@ zone_jsonrender(dns_zone_t *zone, void *arg) { dumparg.type = isc_statsformat_json; dumparg.arg = counters; dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecsignstats, + dns_dnssecsignstats_dump(dnssecsignstats, false, dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { @@ -2730,8 +2727,7 @@ zone_jsonrender(dns_zone_t *zone, void *arg) { } } - dnssecrefreshstats = dns_zone_getdnssecrefreshstats(zone); - if (dnssecrefreshstats != NULL) { + if (dnssecsignstats != NULL) { stats_dumparg_t dumparg; json_object *counters = json_object_new_object(); CHECKMEM(counters); @@ -2739,7 +2735,7 @@ zone_jsonrender(dns_zone_t *zone, void *arg) { dumparg.type = isc_statsformat_json; dumparg.arg = counters; dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecrefreshstats, + dns_dnssecsignstats_dump(dnssecsignstats, true, dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { diff --git a/bin/named/zoneconf.c b/bin/named/zoneconf.c index 6ceb710346..7cad6c6b7a 100644 --- a/bin/named/zoneconf.c +++ b/bin/named/zoneconf.c @@ -890,7 +890,6 @@ named_zone_configure(const cfg_obj_t *config, const cfg_obj_t *vconfig, isc_stats_t *zoneqrystats; dns_stats_t *rcvquerystats; dns_stats_t *dnssecsignstats; - dns_stats_t *dnssecrefreshstats; dns_zonestat_level_t statlevel = dns_zonestat_none; int seconds; dns_zone_t *mayberaw = (raw != NULL) ? raw : zone; @@ -1187,18 +1186,15 @@ named_zone_configure(const cfg_obj_t *config, const cfg_obj_t *vconfig, zoneqrystats = NULL; rcvquerystats = NULL; dnssecsignstats = NULL; - dnssecrefreshstats = NULL; if (statlevel == dns_zonestat_full) { RETERR(isc_stats_create(mctx, &zoneqrystats, ns_statscounter_max)); RETERR(dns_rdatatypestats_create(mctx, &rcvquerystats)); RETERR(dns_dnssecsignstats_create(mctx, &dnssecsignstats)); - RETERR(dns_dnssecsignstats_create(mctx, &dnssecrefreshstats)); } dns_zone_setrequeststats(zone, zoneqrystats); dns_zone_setrcvquerystats(zone, rcvquerystats); dns_zone_setdnssecsignstats(zone, dnssecsignstats); - dns_zone_setdnssecrefreshstats(zone, dnssecrefreshstats); if (zoneqrystats != NULL) { isc_stats_detach(&zoneqrystats); @@ -1212,10 +1208,6 @@ named_zone_configure(const cfg_obj_t *config, const cfg_obj_t *vconfig, dns_stats_detach(&dnssecsignstats); } - if (dnssecrefreshstats != NULL) { - dns_stats_detach(&dnssecrefreshstats); - } - /* * Configure master functionality. This applies * to primary masters (type "master") and slaves diff --git a/lib/dns/include/dns/stats.h b/lib/dns/include/dns/stats.h index 4526e68e2d..79c3fe70a0 100644 --- a/lib/dns/include/dns/stats.h +++ b/lib/dns/include/dns/stats.h @@ -684,9 +684,11 @@ dns_rcodestats_increment(dns_stats_t *stats, dns_opcode_t code); */ void -dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id); +dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, + bool refresh); /*%< - * Increment the statistics counter for the DNSKEY 'id'. + * Increment the statistics counter for the DNSKEY 'id'. If 'refresh' is set + * to true, update the refresh counter, otherwise update the sign counter. * * Requires: *\li 'stats' is a valid dns_stats_t created by dns_dnssecsignstats_create(). @@ -737,7 +739,7 @@ dns_rdatasetstats_dump(dns_stats_t *stats, dns_rdatatypestats_dumper_t dump_fn, */ void -dns_dnssecsignstats_dump(dns_stats_t * stats, +dns_dnssecsignstats_dump(dns_stats_t *stats, bool refresh, dns_dnssecsignstats_dumper_t dump_fn, void *arg, unsigned int options); /*%< diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index c5c363ab0e..093e467598 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -1957,9 +1957,6 @@ dns_zone_setrcvquerystats(dns_zone_t *zone, dns_stats_t *stats); void dns_zone_setdnssecsignstats(dns_zone_t *zone, dns_stats_t *stats); - -void -dns_zone_setdnssecrefreshstats(dns_zone_t *zone, dns_stats_t *stats); /*%< * Set additional statistics sets to zone. These are attached to the zone * but are not counted in the zone module; only the caller updates the @@ -1979,9 +1976,6 @@ dns_zone_getrcvquerystats(dns_zone_t *zone); dns_stats_t * dns_zone_getdnssecsignstats(dns_zone_t *zone); - -dns_stats_t * -dns_zone_getdnssecrefreshstats(dns_zone_t *zone); /*%< * Get the additional statistics for zone, if one is installed. * @@ -1993,17 +1987,6 @@ dns_zone_getdnssecrefreshstats(dns_zone_t *zone); * otherwise NULL. */ -/*%< - * Set additional statistics sets to zone. These are attached to the zone - * but are not counted in the zone module; only the caller updates the - * counters. - * - * Requires: - * \li 'zone' to be a valid zone. - * - *\li stats is a valid statistics. - */ - void dns_zone_dialup(dns_zone_t *zone); /*%< diff --git a/lib/dns/stats.c b/lib/dns/stats.c index 50f9308f23..605be5f71a 100644 --- a/lib/dns/stats.c +++ b/lib/dns/stats.c @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -93,8 +94,18 @@ typedef enum { */ #define RDTYPECOUNTER_MAXVAL 0x0602 -/* dnssec maximum key id */ -static int dnssec_keyid_max = 65535; +/* + * DNSSEC sign statistics. + * + * Per key we maintain 3 counters. The first is actually no counter but + * a key id reference. The second is the number of signatures the key created. + * The third is the number of signatures refreshed by the key. + */ + +/* Maximum number of keys to keep track of for DNSSEC signing statistics. */ +static int dnssec_max_keys = 4; +/* Attribute to signal whether a counter is actually a key id. */ +#define DNSSECSIGNSTATS_IS_KEY 0x10000 struct dns_stats { unsigned int magic; @@ -228,7 +239,11 @@ isc_result_t dns_dnssecsignstats_create(isc_mem_t *mctx, dns_stats_t **statsp) { REQUIRE(statsp != NULL && *statsp == NULL); - return (create_stats(mctx, dns_statstype_dnssec, dnssec_keyid_max, + /* + * Create two counters per key, one is the key id, the other two are + * the actual counters for creating and refreshing signatures. + */ + return (create_stats(mctx, dns_statstype_dnssec, dnssec_max_keys * 3, statsp)); } @@ -342,10 +357,64 @@ dns_rcodestats_increment(dns_stats_t *stats, dns_rcode_t code) { } void -dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id) { +dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, + bool refresh) { + isc_statscounter_t operation; + uint32_t kval; + REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec); - isc_stats_increment(stats->counters, (isc_statscounter_t)id); + kval = (uint32_t)id; + kval |= DNSSECSIGNSTATS_IS_KEY; + + /* What operation are we counting? */ + if (refresh) { + operation = (isc_statscounter_t)dnssec_max_keys * 2; + } else { + operation = (isc_statscounter_t)dnssec_max_keys; + } + + /* Look up correct counter. */ + for (int i = 0; i < dnssec_max_keys; i++) { + uint32_t counter = isc_stats_get_counter(stats->counters, i); + if (counter == kval) { + /* Match */ + isc_stats_increment(stats->counters, operation + i); + return; + } + } + + /* No match found. Store key in unused slot. */ + for (int i = 0; i < dnssec_max_keys; i++) { + uint32_t counter = isc_stats_get_counter(stats->counters, i); + if (counter == 0) { + isc_stats_set(stats->counters, kval, i); + isc_stats_increment(stats->counters, operation + i); + return; + } + } + + /* No room, rotate keys. */ + for (int i = 1; i < dnssec_max_keys; i++) { + uint32_t keyv = isc_stats_get_counter(stats->counters, i); + uint32_t sign = isc_stats_get_counter(stats->counters, + (dnssec_max_keys + i)); + uint32_t refr = isc_stats_get_counter( + stats->counters, (dnssec_max_keys * 2 + i)); + + isc_stats_set(stats->counters, keyv, i - 1); + isc_stats_set(stats->counters, sign, dnssec_max_keys + i - 1); + isc_stats_set(stats->counters, refr, + dnssec_max_keys * 2 + i - 1); + } + + /* Reset counters for new key. */ + isc_stats_set(stats->counters, kval, dnssec_max_keys - 1); + isc_stats_set(stats->counters, 0, 2 * dnssec_max_keys - 1); + isc_stats_set(stats->counters, 0, 3 * dnssec_max_keys - 1); + + /* And increment the counter for the given operation. */ + isc_stats_increment(stats->counters, operation + dnssec_max_keys - 1); } /*% @@ -452,8 +521,41 @@ dnssec_dumpcb(isc_statscounter_t counter, uint64_t value, void *arg) { dnssecarg->fn((dns_keytag_t)counter, value, dnssecarg->arg); } +static void +dnssec_statsdump(isc_stats_t *stats, bool refresh, isc_stats_dumper_t dump_fn, + void *arg, unsigned int options) { + int i; + isc_statscounter_t operation; + + if (refresh) { + operation = (isc_statscounter_t)dnssec_max_keys * 2; + } else { + operation = (isc_statscounter_t)dnssec_max_keys; + } + + for (i = 0; i < dnssec_max_keys; i++) { + uint32_t kval, val; + dns_keytag_t id; + + kval = isc_stats_get_counter(stats, i); + if (kval == 0) { + continue; + } + + val = isc_stats_get_counter(stats, (operation + i)); + if ((options & ISC_STATSDUMP_VERBOSE) == 0 && val == 0) { + continue; + } + + id = (dns_keytag_t)kval; + id &= ~DNSSECSIGNSTATS_IS_KEY; + + dump_fn((isc_statscounter_t)id, val, arg); + } +} + void -dns_dnssecsignstats_dump(dns_stats_t *stats, +dns_dnssecsignstats_dump(dns_stats_t *stats, bool refresh, dns_dnssecsignstats_dumper_t dump_fn, void *arg0, unsigned int options) { dnssecsigndumparg_t arg; @@ -462,7 +564,9 @@ dns_dnssecsignstats_dump(dns_stats_t *stats, arg.fn = dump_fn; arg.arg = arg0; - isc_stats_dump(stats->counters, dnssec_dumpcb, &arg, options); + + dnssec_statsdump(stats->counters, refresh, dnssec_dumpcb, &arg, + options); } static void diff --git a/lib/dns/update.c b/lib/dns/update.c index e2a062e0c3..476c3e4b17 100644 --- a/lib/dns/update.c +++ b/lib/dns/update.c @@ -1259,8 +1259,8 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db, added_sig = true; /* Update DNSSEC sign statistics. */ if (dnssecsignstats != NULL) { - dns_dnssecsignstats_increment(dnssecsignstats, - dst_key_id(keys[i])); + dns_dnssecsignstats_increment( + dnssecsignstats, dst_key_id(keys[i]), false); } } if (!added_sig) { diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in index 71c85d9ef7..50e42bae36 100644 --- a/lib/dns/win32/libdns.def.in +++ b/lib/dns/win32/libdns.def.in @@ -1194,7 +1194,6 @@ dns_zone_getchecknames dns_zone_getclass dns_zone_getdb dns_zone_getdbtype -dns_zone_getdnssecrefreshstats dns_zone_getdnssecsignstats dns_zone_getexpiretime dns_zone_getfile @@ -1298,7 +1297,6 @@ dns_zone_setclass dns_zone_setdb dns_zone_setdbtype dns_zone_setdialup -dns_zone_setdnssecrefreshstats dns_zone_setdnssecsignstats dns_zone_setfile dns_zone_setflag diff --git a/lib/dns/zone.c b/lib/dns/zone.c index aefb71b866..23c8a5f92b 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -338,7 +338,6 @@ struct dns_zone { isc_stats_t *requeststats; dns_stats_t *rcvquerystats; dns_stats_t *dnssecsignstats; - dns_stats_t *dnssecrefreshstats; uint32_t notifydelay; dns_isselffunc_t isself; void *isselfarg; @@ -1091,7 +1090,6 @@ dns_zone_create(dns_zone_t **zonep, isc_mem_t *mctx) { zone->requeststats = NULL; zone->rcvquerystats = NULL; zone->dnssecsignstats = NULL; - zone->dnssecrefreshstats = NULL; zone->notifydelay = 5; zone->isself = NULL; zone->isselfarg = NULL; @@ -1270,9 +1268,6 @@ zone_free(dns_zone_t *zone) { if (zone->dnssecsignstats != NULL) { dns_stats_detach(&zone->dnssecsignstats); } - if (zone->dnssecrefreshstats != NULL) { - dns_stats_detach(&zone->dnssecrefreshstats); - } if (zone->db != NULL) { zone_detachdb(zone); } @@ -6752,7 +6747,6 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone, dns_dbnode_t *node = NULL; dns_kasp_t *kasp = dns_zone_getkasp(zone); dns_stats_t *dnssecsignstats; - dns_stats_t *dnssecrefreshstats; dns_rdataset_t rdataset; dns_rdata_t sig_rdata = DNS_RDATA_INIT; unsigned char data[1024]; /* XXX */ @@ -6926,16 +6920,13 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone, /* Update DNSSEC sign statistics. */ dnssecsignstats = dns_zone_getdnssecsignstats(zone); - dnssecrefreshstats = dns_zone_getdnssecrefreshstats(zone); if (dnssecsignstats != NULL) { + /* Generated a new signature. */ dns_dnssecsignstats_increment( - dns_zone_getdnssecsignstats(zone), - dst_key_id(keys[i])); - } - if (dnssecrefreshstats != NULL) { + dnssecsignstats, dst_key_id(keys[i]), false); + /* This is a refresh. */ dns_dnssecsignstats_increment( - dns_zone_getdnssecrefreshstats(zone), - dst_key_id(keys[i])); + dnssecsignstats, dst_key_id(keys[i]), true); } } @@ -7396,7 +7387,6 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name, dns_rdataset_t rdataset; dns_rdata_t rdata = DNS_RDATA_INIT; dns_stats_t *dnssecsignstats; - dns_stats_t *dnssecrefreshstats; isc_buffer_t buffer; unsigned char data[1024]; @@ -7515,16 +7505,13 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name, /* Update DNSSEC sign statistics. */ dnssecsignstats = dns_zone_getdnssecsignstats(zone); - dnssecrefreshstats = dns_zone_getdnssecrefreshstats(zone); if (dnssecsignstats != NULL) { - dns_dnssecsignstats_increment( - dns_zone_getdnssecsignstats(zone), - dst_key_id(key)); - } - if (dnssecrefreshstats != NULL) { - dns_dnssecsignstats_increment( - dns_zone_getdnssecrefreshstats(zone), - dst_key_id(key)); + /* Generated a new signature. */ + dns_dnssecsignstats_increment(dnssecsignstats, + dst_key_id(key), false); + /* This is a refresh. */ + dns_dnssecsignstats_increment(dnssecsignstats, + dst_key_id(key), true); } (*signatures)--; @@ -18458,17 +18445,6 @@ dns_zone_setdnssecsignstats(dns_zone_t *zone, dns_stats_t *stats) { UNLOCK_ZONE(zone); } -void -dns_zone_setdnssecrefreshstats(dns_zone_t *zone, dns_stats_t *stats) { - REQUIRE(DNS_ZONE_VALID(zone)); - - LOCK_ZONE(zone); - if (stats != NULL && zone->dnssecrefreshstats == NULL) { - dns_stats_attach(stats, &zone->dnssecrefreshstats); - } - UNLOCK_ZONE(zone); -} - dns_stats_t * dns_zone_getdnssecsignstats(dns_zone_t *zone) { REQUIRE(DNS_ZONE_VALID(zone)); @@ -18476,13 +18452,6 @@ dns_zone_getdnssecsignstats(dns_zone_t *zone) { return (zone->dnssecsignstats); } -dns_stats_t * -dns_zone_getdnssecrefreshstats(dns_zone_t *zone) { - REQUIRE(DNS_ZONE_VALID(zone)); - - return (zone->dnssecrefreshstats); -} - isc_stats_t * dns_zone_getrequeststats(dns_zone_t *zone) { /* From 31e8b2b13c0ae71f168517f65e0cec70add8adcf Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 26 Mar 2020 16:13:55 +0100 Subject: [PATCH 2/8] Add test for many keys Add a statschannel test case for DNSSEC sign metrics that has more keys than there are allocated stats counters for. This will produce gibberish, but at least it should not crash. --- bin/tests/system/statschannel/clean.sh | 3 +- .../system/statschannel/ns2/named.conf.in | 18 +++++++++ bin/tests/system/statschannel/ns2/sign.sh | 19 ++++++++- bin/tests/system/statschannel/tests.sh | 39 +++++++++++++++---- bin/tests/system/statschannel/zones-json.pl | 5 ++- bin/tests/system/statschannel/zones-xml.pl | 3 +- 6 files changed, 73 insertions(+), 14 deletions(-) diff --git a/bin/tests/system/statschannel/clean.sh b/bin/tests/system/statschannel/clean.sh index f38d6ac0b1..c9edbde9ee 100644 --- a/bin/tests/system/statschannel/clean.sh +++ b/bin/tests/system/statschannel/clean.sh @@ -22,5 +22,6 @@ rm -f xml.*mem json.*mem rm -f compressed.headers regular.headers compressed.out regular.out rm -f ns*/managed-keys.bind* rm -f ns2/Kdnssec* ns2/dnssec.*.id -rm -f ns2/dnssec.db.signed* ns2/dsset-dnssec. +rm -f ns2/Kmanykeys* ns2/manykeys.*.id +rm -f ns2/*.db.signed* ns2/dsset-*. ns2/*.jbk rm -f ns2/core diff --git a/bin/tests/system/statschannel/ns2/named.conf.in b/bin/tests/system/statschannel/ns2/named.conf.in index fc2952fa2a..70aadf8b28 100644 --- a/bin/tests/system/statschannel/ns2/named.conf.in +++ b/bin/tests/system/statschannel/ns2/named.conf.in @@ -34,6 +34,17 @@ controls { inet 10.53.0.2 port @CONTROLPORT@ allow { any; } keys { rndc_key; }; }; +dnssec-policy "manykeys" { + keys { + ksk lifetime unlimited algorithm 5; + zsk lifetime unlimited algorithm 5; + ksk lifetime unlimited algorithm 13; + zsk lifetime unlimited algorithm 13; + ksk lifetime unlimited algorithm 14; + zsk lifetime unlimited algorithm 14; + }; +}; + zone "example" { type master; file "example.db"; @@ -49,3 +60,10 @@ zone "dnssec" { dnssec-dnskey-kskonly yes; update-check-ksk yes; }; + +zone "manykeys" { + type master; + file "manykeys.db.signed"; + zone-statistics full; + dnssec-policy "manykeys"; +}; diff --git a/bin/tests/system/statschannel/ns2/sign.sh b/bin/tests/system/statschannel/ns2/sign.sh index 3a90654d75..669adec3ec 100644 --- a/bin/tests/system/statschannel/ns2/sign.sh +++ b/bin/tests/system/statschannel/ns2/sign.sh @@ -17,12 +17,27 @@ set -e zone=dnssec. infile=dnssec.db.in zonefile=dnssec.db.signed - ksk=$("$KEYGEN" -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" -f KSK "$zone") zsk=$("$KEYGEN" -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" "$zone") # Sign deliberately with a very short expiration date. "$SIGNER" -S -x -O full -e "now"+1s -o "$zone" -f "$zonefile" "$infile" > /dev/null 2>&1 - keyfile_to_key_id "$ksk" > dnssec.ksk.id keyfile_to_key_id "$zsk" > dnssec.zsk.id +zone=manykeys. +infile=manykeys.db.in +zonefile=manykeys.db.signed +ksk8=$("$KEYGEN" -q -a RSASHA256 -b 2048 -f KSK "$zone") +zsk8=$("$KEYGEN" -q -a RSASHA256 -b 2048 "$zone") +ksk13=$("$KEYGEN" -q -a ECDSAP256SHA256 -b 256 -f KSK "$zone") +zsk13=$("$KEYGEN" -q -a ECDSAP256SHA256 -b 256 "$zone") +ksk14=$("$KEYGEN" -q -a ECDSAP384SHA384 -b 384 -f KSK "$zone") +zsk14=$("$KEYGEN" -q -a ECDSAP384SHA384 -b 384 "$zone") +# Sign deliberately with a very short expiration date. +"$SIGNER" -S -x -O full -e "now"+1s -o "$zone" -f "$zonefile" "$infile" > /dev/null 2>&1 +keyfile_to_key_id "$ksk8" > manykeys.ksk8.id +keyfile_to_key_id "$zsk8" > manykeys.zsk8.id +keyfile_to_key_id "$ksk13" > manykeys.ksk13.id +keyfile_to_key_id "$zsk13" > manykeys.zsk13.id +keyfile_to_key_id "$ksk14" > manykeys.ksk14.id +keyfile_to_key_id "$zsk14" > manykeys.zsk14.id diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh index 333018a03a..32a9da451c 100644 --- a/bin/tests/system/statschannel/tests.sh +++ b/bin/tests/system/statschannel/tests.sh @@ -70,8 +70,8 @@ getzones() { *) return 1 ;; esac file=`$PERL fetch.pl -p ${EXTRAPORT1} $path` - cp $file $file.$1.$2 - $PERL zones-${1}.pl $file 2>/dev/null | sort > zones.out.$2 + cp $file $file.$1.$3 + $PERL zones-${1}.pl $file $2 2>/dev/null | sort > zones.out.$3 result=$? return $result } @@ -292,11 +292,11 @@ rm -f zones.expect # Fetch and check the dnssec sign statistics. echo_i "fetching zone stats data after zone maintenance at startup ($n)" if [ $PERL_XML ]; then - getzones xml x$n || ret=1 + getzones xml $zone x$n || ret=1 cmp zones.out.x$n zones.expect.$n || ret=1 fi if [ $PERL_JSON ]; then - getzones json j$n || ret=1 + getzones json $zone j$n || ret=1 cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi @@ -322,11 +322,11 @@ rm -f zones.expect # Fetch and check the dnssec sign statistics. echo_i "fetching zone stats data after dynamic update ($n)" if [ $PERL_XML ]; then - getzones xml x$n || ret=1 + getzones xml $zone x$n || ret=1 cmp zones.out.x$n zones.expect.$n || ret=1 fi if [ $PERL_JSON ]; then - getzones json j$n || ret=1 + getzones json $zone j$n || ret=1 cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi @@ -349,16 +349,39 @@ cat zones.expect | sort > zones.expect.$n rm -f zones.expect # Fetch and check the dnssec sign statistics. if [ $PERL_XML ]; then - getzones xml x$n || ret=1 + getzones xml $zone x$n || ret=1 cmp zones.out.x$n zones.expect.$n || ret=1 fi if [ $PERL_JSON ]; then - getzones json j$n || ret=1 + getzones json $zone j$n || ret=1 cmp zones.out.j$n zones.expect.$n || ret=1 fi if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` n=`expr $n + 1` +# 4. Test a zone with more than four keys. +zone="manykeys" +ksk8_id=`cat ns2/$zone.ksk8.id` +zsk8_id=`cat ns2/$zone.zsk8.id` +ksk13_id=`cat ns2/$zone.ksk13.id` +zsk13_id=`cat ns2/$zone.zsk13.id` +ksk14_id=`cat ns2/$zone.ksk14.id` +zsk14_id=`cat ns2/$zone.zsk14.id` + +ret=0 +echo_i "fetch zone stats data for a zone with many keys ($n)" +# Fetch and check the dnssec sign statistics. +if [ $PERL_XML ]; then + getzones xml $zone x$n || ret=1 +fi +if [ $PERL_JSON ]; then + getzones json $zone j$n || ret=1 +fi +# The output is gibberish, but at least make sure it does not crash. +if [ $ret != 0 ]; then echo_i "failed"; fi +status=`expr $status + $ret` +n=`expr $n + 1` + echo_i "exit status: $status" [ $status -eq 0 ] || exit 1 diff --git a/bin/tests/system/statschannel/zones-json.pl b/bin/tests/system/statschannel/zones-json.pl index deef35b0f1..51f26ac0f3 100644 --- a/bin/tests/system/statschannel/zones-json.pl +++ b/bin/tests/system/statschannel/zones-json.pl @@ -16,6 +16,7 @@ use JSON; my $file = $ARGV[0]; +my $zone = $ARGV[1]; open(INPUT, "<$file"); my $text = do{local$/;}; close(INPUT); @@ -23,12 +24,12 @@ close(INPUT); my $ref = decode_json($text); -my $dnssecsign = $ref->{views}->{_default}->{zones}[0]->{"dnssec-sign"}; +my $dnssecsign = $ref->{views}->{_default}->{zones}[$zone]->{"dnssec-sign"}; my $type = "dnssec-sign operations "; foreach $key (keys %{$dnssecsign}) { print $type . $key . ": ". $dnssecsign->{$key} ."\n"; } -my $dnssecrefresh = $ref->{views}->{_default}->{zones}[0]->{"dnssec-refresh"}; +my $dnssecrefresh = $ref->{views}->{_default}->{zones}[$zone]->{"dnssec-refresh"}; my $type = "dnssec-refresh operations "; foreach $key (keys %{$dnssecrefresh}) { print $type . $key . ": ". $dnssecrefresh->{$key} ."\n"; diff --git a/bin/tests/system/statschannel/zones-xml.pl b/bin/tests/system/statschannel/zones-xml.pl index f078b054bb..45a3fe6cbf 100644 --- a/bin/tests/system/statschannel/zones-xml.pl +++ b/bin/tests/system/statschannel/zones-xml.pl @@ -16,10 +16,11 @@ use XML::Simple; my $file = $ARGV[0]; +my $zone = $ARGV[1]; my $ref = XMLin($file); -my $counters = $ref->{views}->{view}->{_default}->{zones}->{zone}->{dnssec}->{counters}; +my $counters = $ref->{views}->{view}->{_default}->{zones}->{zone}->{$zone}->{counters}; foreach $group (@$counters) { From eb6a8b47d79beed5be357ebf6dc082dc34341824 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 2 Apr 2020 10:50:16 +0200 Subject: [PATCH 3/8] Group the keyid with the counters Rather than group key ids together, group key id with its corresponding counters. This should make growing / shrinking easier than having keyids then counters. --- lib/dns/stats.c | 60 ++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/lib/dns/stats.c b/lib/dns/stats.c index 605be5f71a..711cf880b5 100644 --- a/lib/dns/stats.c +++ b/lib/dns/stats.c @@ -106,6 +106,9 @@ typedef enum { static int dnssec_max_keys = 4; /* Attribute to signal whether a counter is actually a key id. */ #define DNSSECSIGNSTATS_IS_KEY 0x10000 +/* DNSSEC sign operation (sign or refresh) */ +#define DNSSECSIGNSTATS_SIGN 1 +#define DNSSECSIGNSTATS_REFRESH 2 struct dns_stats { unsigned int magic; @@ -359,7 +362,7 @@ dns_rcodestats_increment(dns_stats_t *stats, dns_rcode_t code) { void dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, bool refresh) { - isc_statscounter_t operation; + isc_statscounter_t operation = DNSSECSIGNSTATS_SIGN; uint32_t kval; REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec); @@ -369,52 +372,54 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, /* What operation are we counting? */ if (refresh) { - operation = (isc_statscounter_t)dnssec_max_keys * 2; - } else { - operation = (isc_statscounter_t)dnssec_max_keys; + operation = DNSSECSIGNSTATS_REFRESH; } /* Look up correct counter. */ for (int i = 0; i < dnssec_max_keys; i++) { - uint32_t counter = isc_stats_get_counter(stats->counters, i); + int idx = 3 * i; + uint32_t counter = isc_stats_get_counter(stats->counters, idx); if (counter == kval) { /* Match */ - isc_stats_increment(stats->counters, operation + i); + isc_stats_increment(stats->counters, (idx + operation)); return; } } /* No match found. Store key in unused slot. */ for (int i = 0; i < dnssec_max_keys; i++) { - uint32_t counter = isc_stats_get_counter(stats->counters, i); + int idx = 3 * i; + uint32_t counter = isc_stats_get_counter(stats->counters, idx); if (counter == 0) { - isc_stats_set(stats->counters, kval, i); - isc_stats_increment(stats->counters, operation + i); + isc_stats_set(stats->counters, kval, idx); + isc_stats_increment(stats->counters, (idx + operation)); return; } } /* No room, rotate keys. */ for (int i = 1; i < dnssec_max_keys; i++) { - uint32_t keyv = isc_stats_get_counter(stats->counters, i); + int gidx = 3 * i; /* Get key (get index, gidx) */ + uint32_t keyv = isc_stats_get_counter(stats->counters, gidx); uint32_t sign = isc_stats_get_counter(stats->counters, - (dnssec_max_keys + i)); - uint32_t refr = isc_stats_get_counter( - stats->counters, (dnssec_max_keys * 2 + i)); + (gidx + 1)); + uint32_t refr = isc_stats_get_counter(stats->counters, + (gidx + 2)); - isc_stats_set(stats->counters, keyv, i - 1); - isc_stats_set(stats->counters, sign, dnssec_max_keys + i - 1); - isc_stats_set(stats->counters, refr, - dnssec_max_keys * 2 + i - 1); + int sidx = gidx - 3; /* Set key, (set index, sidx) */ + isc_stats_set(stats->counters, keyv, sidx); + isc_stats_set(stats->counters, sign, (sidx + 1)); + isc_stats_set(stats->counters, refr, (sidx + 2)); } - /* Reset counters for new key. */ - isc_stats_set(stats->counters, kval, dnssec_max_keys - 1); - isc_stats_set(stats->counters, 0, 2 * dnssec_max_keys - 1); - isc_stats_set(stats->counters, 0, 3 * dnssec_max_keys - 1); + /* Reset counters for new key (new index, nidx). */ + int nidx = 3 * (dnssec_max_keys - 1); + isc_stats_set(stats->counters, kval, nidx); + isc_stats_set(stats->counters, 0, (nidx + 1)); + isc_stats_set(stats->counters, 0, (nidx + 2)); /* And increment the counter for the given operation. */ - isc_stats_increment(stats->counters, operation + dnssec_max_keys - 1); + isc_stats_increment(stats->counters, (nidx + operation)); } /*% @@ -525,24 +530,23 @@ static void dnssec_statsdump(isc_stats_t *stats, bool refresh, isc_stats_dumper_t dump_fn, void *arg, unsigned int options) { int i; - isc_statscounter_t operation; + isc_statscounter_t operation = DNSSECSIGNSTATS_SIGN; if (refresh) { - operation = (isc_statscounter_t)dnssec_max_keys * 2; - } else { - operation = (isc_statscounter_t)dnssec_max_keys; + operation = DNSSECSIGNSTATS_REFRESH; } for (i = 0; i < dnssec_max_keys; i++) { + int idx = 3 * i; uint32_t kval, val; dns_keytag_t id; - kval = isc_stats_get_counter(stats, i); + kval = isc_stats_get_counter(stats, idx); if (kval == 0) { continue; } - val = isc_stats_get_counter(stats, (operation + i)); + val = isc_stats_get_counter(stats, (idx + operation)); if ((options & ISC_STATSDUMP_VERBOSE) == 0 && val == 0) { continue; } From b2028e26da02cec99881c5ab52ec23837bf0ae13 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 2 Apr 2020 11:59:35 +0200 Subject: [PATCH 4/8] Embed algorithm in key tag counter Key tags are not unique across algorithms. --- lib/dns/include/dns/stats.h | 2 +- lib/dns/stats.c | 13 ++++++------- lib/dns/update.c | 4 +++- lib/dns/zone.c | 15 +++++++++------ 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/lib/dns/include/dns/stats.h b/lib/dns/include/dns/stats.h index 79c3fe70a0..2883a31d38 100644 --- a/lib/dns/include/dns/stats.h +++ b/lib/dns/include/dns/stats.h @@ -684,7 +684,7 @@ dns_rcodestats_increment(dns_stats_t *stats, dns_opcode_t code); */ void -dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, +dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, bool refresh); /*%< * Increment the statistics counter for the DNSKEY 'id'. If 'refresh' is set diff --git a/lib/dns/stats.c b/lib/dns/stats.c index 711cf880b5..1b3ef4d24d 100644 --- a/lib/dns/stats.c +++ b/lib/dns/stats.c @@ -104,8 +104,8 @@ typedef enum { /* Maximum number of keys to keep track of for DNSSEC signing statistics. */ static int dnssec_max_keys = 4; -/* Attribute to signal whether a counter is actually a key id. */ -#define DNSSECSIGNSTATS_IS_KEY 0x10000 +/* Key id mask */ +#define DNSSECSIGNSTATS_KEY_ID_MASK 0x0000FFFF /* DNSSEC sign operation (sign or refresh) */ #define DNSSECSIGNSTATS_SIGN 1 #define DNSSECSIGNSTATS_REFRESH 2 @@ -360,15 +360,15 @@ dns_rcodestats_increment(dns_stats_t *stats, dns_rcode_t code) { } void -dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, +dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, bool refresh) { isc_statscounter_t operation = DNSSECSIGNSTATS_SIGN; uint32_t kval; REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec); - kval = (uint32_t)id; - kval |= DNSSECSIGNSTATS_IS_KEY; + /* Shift algorithm in front of key tag, which is 16 bits */ + kval = (uint32_t)(alg << 16 | id); /* What operation are we counting? */ if (refresh) { @@ -551,8 +551,7 @@ dnssec_statsdump(isc_stats_t *stats, bool refresh, isc_stats_dumper_t dump_fn, continue; } - id = (dns_keytag_t)kval; - id &= ~DNSSECSIGNSTATS_IS_KEY; + id = (dns_keytag_t)kval & DNSSECSIGNSTATS_KEY_ID_MASK; dump_fn((isc_statscounter_t)id, val, arg); } diff --git a/lib/dns/update.c b/lib/dns/update.c index 476c3e4b17..657cc777b0 100644 --- a/lib/dns/update.c +++ b/lib/dns/update.c @@ -1117,6 +1117,7 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db, #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) /* @@ -1260,7 +1261,8 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db, /* Update DNSSEC sign statistics. */ if (dnssecsignstats != NULL) { dns_dnssecsignstats_increment( - dnssecsignstats, dst_key_id(keys[i]), false); + dnssecsignstats, ID(keys[i]), + (uint8_t)ALG(keys[i]), false); } } if (!added_sig) { diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 23c8a5f92b..273aa3c591 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -120,6 +120,7 @@ */ #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) /* @@ -6923,10 +6924,12 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone, if (dnssecsignstats != NULL) { /* Generated a new signature. */ dns_dnssecsignstats_increment( - dnssecsignstats, dst_key_id(keys[i]), false); + dnssecsignstats, ID(keys[i]), + (uint8_t)ALG(keys[i]), false); /* This is a refresh. */ dns_dnssecsignstats_increment( - dnssecsignstats, dst_key_id(keys[i]), true); + dnssecsignstats, ID(keys[i]), + (uint8_t)ALG(keys[i]), true); } } @@ -7507,11 +7510,11 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name, dnssecsignstats = dns_zone_getdnssecsignstats(zone); if (dnssecsignstats != NULL) { /* Generated a new signature. */ - dns_dnssecsignstats_increment(dnssecsignstats, - dst_key_id(key), false); + dns_dnssecsignstats_increment(dnssecsignstats, ID(key), + ALG(key), false); /* This is a refresh. */ - dns_dnssecsignstats_increment(dnssecsignstats, - dst_key_id(key), true); + dns_dnssecsignstats_increment(dnssecsignstats, ID(key), + ALG(key), true); } (*signatures)--; From 44b49955e1a67e59e5a4cac7cec41baec7d5d0aa Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 2 Apr 2020 16:12:10 +0200 Subject: [PATCH 5/8] Replace sign operation bool with enum --- bin/named/statschannel.c | 24 ++++++++++---------- lib/dns/include/dns/stats.h | 16 ++++++++++---- lib/dns/stats.c | 44 ++++++++++++++----------------------- lib/dns/update.c | 7 +++--- lib/dns/zone.c | 18 +++++++++------ 5 files changed, 55 insertions(+), 54 deletions(-) diff --git a/bin/named/statschannel.c b/bin/named/statschannel.c index 2bbe01de8a..6dc031a0bb 100644 --- a/bin/named/statschannel.c +++ b/bin/named/statschannel.c @@ -1887,9 +1887,9 @@ zone_xmlrender(dns_zone_t *zone, void *arg) { ISC_XMLCHAR "dnssec-sign")); dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecsignstats, false, - dnssecsignstat_dump, &dumparg, - 0); + dns_dnssecsignstats_dump( + dnssecsignstats, dns_dnssecsignstats_sign, + dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { goto error; } @@ -1905,9 +1905,9 @@ zone_xmlrender(dns_zone_t *zone, void *arg) { ISC_XMLCHAR "dnssec-refresh")); dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecsignstats, true, - dnssecsignstat_dump, &dumparg, - 0); + dns_dnssecsignstats_dump( + dnssecsignstats, dns_dnssecsignstats_refresh, + dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { goto error; } @@ -2711,9 +2711,9 @@ zone_jsonrender(dns_zone_t *zone, void *arg) { dumparg.type = isc_statsformat_json; dumparg.arg = counters; dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecsignstats, false, - dnssecsignstat_dump, &dumparg, - 0); + dns_dnssecsignstats_dump( + dnssecsignstats, dns_dnssecsignstats_sign, + dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { json_object_put(counters); goto error; @@ -2735,9 +2735,9 @@ zone_jsonrender(dns_zone_t *zone, void *arg) { dumparg.type = isc_statsformat_json; dumparg.arg = counters; dumparg.result = ISC_R_SUCCESS; - dns_dnssecsignstats_dump(dnssecsignstats, true, - dnssecsignstat_dump, &dumparg, - 0); + dns_dnssecsignstats_dump( + dnssecsignstats, dns_dnssecsignstats_refresh, + dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { json_object_put(counters); goto error; diff --git a/lib/dns/include/dns/stats.h b/lib/dns/include/dns/stats.h index 2883a31d38..8c1d04aed9 100644 --- a/lib/dns/include/dns/stats.h +++ b/lib/dns/include/dns/stats.h @@ -491,6 +491,14 @@ LIBDNS_EXTERNAL_DATA extern const char *dns_statscounter_names[]; #define DNS_RDATASTATSTYPE_ATTR(type) ((type) >> 16) #define DNS_RDATASTATSTYPE_VALUE(b, a) (((a) << 16) | (b)) +/*% + * Types of DNSSEC sign statistics operations. + */ +typedef enum { + dns_dnssecsignstats_sign = 1, + dns_dnssecsignstats_refresh = 2 +} dnssecsignstats_type_t; + /*%< * Types of dump callbacks. */ @@ -685,10 +693,10 @@ dns_rcodestats_increment(dns_stats_t *stats, dns_opcode_t code); void dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, - bool refresh); + dnssecsignstats_type_t operation); /*%< - * Increment the statistics counter for the DNSKEY 'id'. If 'refresh' is set - * to true, update the refresh counter, otherwise update the sign counter. + * Increment the statistics counter for the DNSKEY 'id'. The 'operation' + * determines what counter is incremented. * * Requires: *\li 'stats' is a valid dns_stats_t created by dns_dnssecsignstats_create(). @@ -739,7 +747,7 @@ dns_rdatasetstats_dump(dns_stats_t *stats, dns_rdatatypestats_dumper_t dump_fn, */ void -dns_dnssecsignstats_dump(dns_stats_t *stats, bool refresh, +dns_dnssecsignstats_dump(dns_stats_t *stats, dnssecsignstats_type_t operation, dns_dnssecsignstats_dumper_t dump_fn, void *arg, unsigned int options); /*%< diff --git a/lib/dns/stats.c b/lib/dns/stats.c index 1b3ef4d24d..6fe762c673 100644 --- a/lib/dns/stats.c +++ b/lib/dns/stats.c @@ -106,9 +106,6 @@ typedef enum { static int dnssec_max_keys = 4; /* Key id mask */ #define DNSSECSIGNSTATS_KEY_ID_MASK 0x0000FFFF -/* DNSSEC sign operation (sign or refresh) */ -#define DNSSECSIGNSTATS_SIGN 1 -#define DNSSECSIGNSTATS_REFRESH 2 struct dns_stats { unsigned int magic; @@ -361,8 +358,7 @@ dns_rcodestats_increment(dns_stats_t *stats, dns_rcode_t code) { void dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, - bool refresh) { - isc_statscounter_t operation = DNSSECSIGNSTATS_SIGN; + dnssecsignstats_type_t operation) { uint32_t kval; REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec); @@ -370,11 +366,6 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, /* Shift algorithm in front of key tag, which is 16 bits */ kval = (uint32_t)(alg << 16 | id); - /* What operation are we counting? */ - if (refresh) { - operation = DNSSECSIGNSTATS_REFRESH; - } - /* Look up correct counter. */ for (int i = 0; i < dnssec_max_keys; i++) { int idx = 3 * i; @@ -401,22 +392,24 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, for (int i = 1; i < dnssec_max_keys; i++) { int gidx = 3 * i; /* Get key (get index, gidx) */ uint32_t keyv = isc_stats_get_counter(stats->counters, gidx); - uint32_t sign = isc_stats_get_counter(stats->counters, - (gidx + 1)); - uint32_t refr = isc_stats_get_counter(stats->counters, - (gidx + 2)); + uint32_t sign = isc_stats_get_counter( + stats->counters, (gidx + dns_dnssecsignstats_sign)); + uint32_t refr = isc_stats_get_counter( + stats->counters, (gidx + dns_dnssecsignstats_refresh)); - int sidx = gidx - 3; /* Set key, (set index, sidx) */ + int sidx = 3 * (i - 1); /* Set key, (set index, sidx) */ isc_stats_set(stats->counters, keyv, sidx); - isc_stats_set(stats->counters, sign, (sidx + 1)); - isc_stats_set(stats->counters, refr, (sidx + 2)); + isc_stats_set(stats->counters, sign, + (sidx + dns_dnssecsignstats_sign)); + isc_stats_set(stats->counters, refr, + (sidx + dns_dnssecsignstats_refresh)); } /* Reset counters for new key (new index, nidx). */ int nidx = 3 * (dnssec_max_keys - 1); isc_stats_set(stats->counters, kval, nidx); - isc_stats_set(stats->counters, 0, (nidx + 1)); - isc_stats_set(stats->counters, 0, (nidx + 2)); + isc_stats_set(stats->counters, 0, (nidx + dns_dnssecsignstats_sign)); + isc_stats_set(stats->counters, 0, (nidx + dns_dnssecsignstats_refresh)); /* And increment the counter for the given operation. */ isc_stats_increment(stats->counters, (nidx + operation)); @@ -527,14 +520,9 @@ dnssec_dumpcb(isc_statscounter_t counter, uint64_t value, void *arg) { } static void -dnssec_statsdump(isc_stats_t *stats, bool refresh, isc_stats_dumper_t dump_fn, - void *arg, unsigned int options) { +dnssec_statsdump(isc_stats_t *stats, dnssecsignstats_type_t operation, + isc_stats_dumper_t dump_fn, void *arg, unsigned int options) { int i; - isc_statscounter_t operation = DNSSECSIGNSTATS_SIGN; - - if (refresh) { - operation = DNSSECSIGNSTATS_REFRESH; - } for (i = 0; i < dnssec_max_keys; i++) { int idx = 3 * i; @@ -558,7 +546,7 @@ dnssec_statsdump(isc_stats_t *stats, bool refresh, isc_stats_dumper_t dump_fn, } void -dns_dnssecsignstats_dump(dns_stats_t *stats, bool refresh, +dns_dnssecsignstats_dump(dns_stats_t *stats, dnssecsignstats_type_t operation, dns_dnssecsignstats_dumper_t dump_fn, void *arg0, unsigned int options) { dnssecsigndumparg_t arg; @@ -568,7 +556,7 @@ dns_dnssecsignstats_dump(dns_stats_t *stats, bool refresh, arg.fn = dump_fn; arg.arg = arg0; - dnssec_statsdump(stats->counters, refresh, dnssec_dumpcb, &arg, + dnssec_statsdump(stats->counters, operation, dnssec_dumpcb, &arg, options); } diff --git a/lib/dns/update.c b/lib/dns/update.c index 657cc777b0..76d4cee4ef 100644 --- a/lib/dns/update.c +++ b/lib/dns/update.c @@ -1260,9 +1260,10 @@ add_sigs(dns_update_log_t *log, dns_zone_t *zone, dns_db_t *db, added_sig = true; /* Update DNSSEC sign statistics. */ if (dnssecsignstats != NULL) { - dns_dnssecsignstats_increment( - dnssecsignstats, ID(keys[i]), - (uint8_t)ALG(keys[i]), false); + dns_dnssecsignstats_increment(dnssecsignstats, + ID(keys[i]), + (uint8_t)ALG(keys[i]), + dns_dnssecsignstats_sign); } } if (!added_sig) { diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 273aa3c591..4ba75ef52d 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -6923,13 +6923,15 @@ add_sigs(dns_db_t *db, dns_dbversion_t *ver, dns_name_t *name, dns_zone_t *zone, dnssecsignstats = dns_zone_getdnssecsignstats(zone); if (dnssecsignstats != NULL) { /* Generated a new signature. */ - dns_dnssecsignstats_increment( - dnssecsignstats, ID(keys[i]), - (uint8_t)ALG(keys[i]), false); + dns_dnssecsignstats_increment(dnssecsignstats, + ID(keys[i]), + (uint8_t)ALG(keys[i]), + dns_dnssecsignstats_sign); /* This is a refresh. */ dns_dnssecsignstats_increment( dnssecsignstats, ID(keys[i]), - (uint8_t)ALG(keys[i]), true); + (uint8_t)ALG(keys[i]), + dns_dnssecsignstats_refresh); } } @@ -7511,10 +7513,12 @@ sign_a_node(dns_db_t *db, dns_zone_t *zone, dns_name_t *name, if (dnssecsignstats != NULL) { /* Generated a new signature. */ dns_dnssecsignstats_increment(dnssecsignstats, ID(key), - ALG(key), false); + ALG(key), + dns_dnssecsignstats_sign); /* This is a refresh. */ - dns_dnssecsignstats_increment(dnssecsignstats, ID(key), - ALG(key), true); + dns_dnssecsignstats_increment( + dnssecsignstats, ID(key), ALG(key), + dns_dnssecsignstats_refresh); } (*signatures)--; From 1596d3b498cf9363ddb3c5978a3f01729cefcd09 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 2 Apr 2020 18:53:53 +0200 Subject: [PATCH 6/8] Merge if blocks in statschannel.c --- bin/named/statschannel.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/bin/named/statschannel.c b/bin/named/statschannel.c index 6dc031a0bb..25bec658bf 100644 --- a/bin/named/statschannel.c +++ b/bin/named/statschannel.c @@ -2705,49 +2705,49 @@ zone_jsonrender(dns_zone_t *zone, void *arg) { dnssecsignstats = dns_zone_getdnssecsignstats(zone); if (dnssecsignstats != NULL) { stats_dumparg_t dumparg; - json_object *counters = json_object_new_object(); - CHECKMEM(counters); + json_object *sign_counters = json_object_new_object(); + CHECKMEM(sign_counters); dumparg.type = isc_statsformat_json; - dumparg.arg = counters; + dumparg.arg = sign_counters; dumparg.result = ISC_R_SUCCESS; dns_dnssecsignstats_dump( dnssecsignstats, dns_dnssecsignstats_sign, dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { - json_object_put(counters); + json_object_put(sign_counters); goto error; } - if (json_object_get_object(counters)->count != 0) { + if (json_object_get_object(sign_counters)->count != 0) { json_object_object_add(zoneobj, "dnssec-sign", - counters); + sign_counters); } else { - json_object_put(counters); + json_object_put(sign_counters); } - } - if (dnssecsignstats != NULL) { - stats_dumparg_t dumparg; - json_object *counters = json_object_new_object(); - CHECKMEM(counters); + json_object *refresh_counters = + json_object_new_object(); + CHECKMEM(refresh_counters); dumparg.type = isc_statsformat_json; - dumparg.arg = counters; + dumparg.arg = refresh_counters; dumparg.result = ISC_R_SUCCESS; dns_dnssecsignstats_dump( dnssecsignstats, dns_dnssecsignstats_refresh, dnssecsignstat_dump, &dumparg, 0); if (dumparg.result != ISC_R_SUCCESS) { - json_object_put(counters); + json_object_put(refresh_counters); goto error; } - if (json_object_get_object(counters)->count != 0) { - json_object_object_add( - zoneobj, "dnssec-refresh", counters); + if (json_object_get_object(refresh_counters)->count != + 0) { + json_object_object_add(zoneobj, + "dnssec-refresh", + refresh_counters); } else { - json_object_put(counters); + json_object_put(refresh_counters); } } } From c1723b2535c3f2be3a2e948897c8200258a66013 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Fri, 3 Apr 2020 08:14:22 +0200 Subject: [PATCH 7/8] Replace hard coded value with constant --- CHANGES | 4 ++++ lib/dns/stats.c | 29 ++++++++++++++++------------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/CHANGES b/CHANGES index dbd9318f31..f78947e808 100644 --- a/CHANGES +++ b/CHANGES @@ -3,6 +3,10 @@ possible key id per zone which results in a lot of wasted memory. Fix by tracking up to four keys per zone, rotate counters when keys are replaced. + This fixes the immediate problem of the high + memory usage, but should be improved in a future + release by growing and shrinking the number of + keys to track triggered by key rollover events. [GL #1179] 5372. [bug] Fix migration from existing DNSSEC key files using diff --git a/lib/dns/stats.c b/lib/dns/stats.c index 6fe762c673..c06c0fc2dd 100644 --- a/lib/dns/stats.c +++ b/lib/dns/stats.c @@ -103,7 +103,8 @@ typedef enum { */ /* Maximum number of keys to keep track of for DNSSEC signing statistics. */ -static int dnssec_max_keys = 4; +static int dnssecsign_max_keys = 4; +static int dnssecsign_block_size = 3; /* Key id mask */ #define DNSSECSIGNSTATS_KEY_ID_MASK 0x0000FFFF @@ -243,8 +244,8 @@ dns_dnssecsignstats_create(isc_mem_t *mctx, dns_stats_t **statsp) { * Create two counters per key, one is the key id, the other two are * the actual counters for creating and refreshing signatures. */ - return (create_stats(mctx, dns_statstype_dnssec, dnssec_max_keys * 3, - statsp)); + return (create_stats(mctx, dns_statstype_dnssec, + dnssecsign_max_keys * 3, statsp)); } /*% @@ -367,8 +368,8 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, kval = (uint32_t)(alg << 16 | id); /* Look up correct counter. */ - for (int i = 0; i < dnssec_max_keys; i++) { - int idx = 3 * i; + for (int i = 0; i < dnssecsign_max_keys; i++) { + int idx = i * dnssecsign_block_size; uint32_t counter = isc_stats_get_counter(stats->counters, idx); if (counter == kval) { /* Match */ @@ -378,8 +379,8 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, } /* No match found. Store key in unused slot. */ - for (int i = 0; i < dnssec_max_keys; i++) { - int idx = 3 * i; + for (int i = 0; i < dnssecsign_max_keys; i++) { + int idx = i * dnssecsign_block_size; uint32_t counter = isc_stats_get_counter(stats->counters, idx); if (counter == 0) { isc_stats_set(stats->counters, kval, idx); @@ -389,15 +390,17 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, } /* No room, rotate keys. */ - for (int i = 1; i < dnssec_max_keys; i++) { - int gidx = 3 * i; /* Get key (get index, gidx) */ + for (int i = 1; i < dnssecsign_max_keys; i++) { + int gidx = i * dnssecsign_block_size; /* Get key (get index, + gidx) */ uint32_t keyv = isc_stats_get_counter(stats->counters, gidx); uint32_t sign = isc_stats_get_counter( stats->counters, (gidx + dns_dnssecsignstats_sign)); uint32_t refr = isc_stats_get_counter( stats->counters, (gidx + dns_dnssecsignstats_refresh)); - int sidx = 3 * (i - 1); /* Set key, (set index, sidx) */ + int sidx = (i - 1) * dnssecsign_block_size; /* Set key, (set + index, sidx) */ isc_stats_set(stats->counters, keyv, sidx); isc_stats_set(stats->counters, sign, (sidx + dns_dnssecsignstats_sign)); @@ -406,7 +409,7 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg, } /* Reset counters for new key (new index, nidx). */ - int nidx = 3 * (dnssec_max_keys - 1); + int nidx = (dnssecsign_max_keys - 1) * dnssecsign_block_size; isc_stats_set(stats->counters, kval, nidx); isc_stats_set(stats->counters, 0, (nidx + dns_dnssecsignstats_sign)); isc_stats_set(stats->counters, 0, (nidx + dns_dnssecsignstats_refresh)); @@ -524,8 +527,8 @@ dnssec_statsdump(isc_stats_t *stats, dnssecsignstats_type_t operation, isc_stats_dumper_t dump_fn, void *arg, unsigned int options) { int i; - for (i = 0; i < dnssec_max_keys; i++) { - int idx = 3 * i; + for (i = 0; i < dnssecsign_max_keys; i++) { + int idx = dnssecsign_block_size * i; uint32_t kval, val; dns_keytag_t id; From 386890a1610acc7cc3569b48a40e5811d5c0ca21 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Fri, 3 Apr 2020 08:36:58 +0200 Subject: [PATCH 8/8] Update release notes --- doc/arm/notes-9.17.1.xml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/arm/notes-9.17.1.xml b/doc/arm/notes-9.17.1.xml index aaad722969..b807089039 100644 --- a/doc/arm/notes-9.17.1.xml +++ b/doc/arm/notes-9.17.1.xml @@ -45,7 +45,9 @@ - None. + The DNSSEC sign statistics used lots of memory. The number of keys + to track is reduced to four per zone, which should be enough for + 99% of all signed zones. [GL #1179]