From 705810d577bca7d8da7ff87eae00e7e75b6ad7c9 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 26 Mar 2020 16:02:36 +0100 Subject: [PATCH] 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) { /*