Bug 4215 - Changing TSIG keys on the fly
Changing TSIG keys on the fly
Status: ASSIGNED
Product: NSD
Classification: Unclassified
Component: NSD Code
4.1.x
x86_64 FreeBSD
: P5 enhancement
Assigned To: NSD team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-18 15:12 CET by Igor
Modified: 2019-02-15 14:29 CET (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Igor 2018-12-18 15:12:51 CET
Hello!

I have started own experiment to enhance NSD functionality by changing TSIG keys without editing of nsd.conf and reloading the process.

In NSD sources i found the code handling TSIGs adding/replacing. So in remote.c i added my do_logtsig()/do_puttsig() that called from nsd-control utility. The code:

static void
do_logtsig(SSL* ssl, xfrd_state_type* xfrd, char* arg)
{
        tsig_print_secret(xfrd->nsd->options, arg);

        send_ok(ssl);
}

static void
do_puttsig(SSL* ssl, xfrd_state_type* xfrd, char* arg)
{
        char* arg2 = NULL;
        struct key_options* newk;

        if(!find_arg2(ssl, arg, &arg2)) {
                log_msg(LOG_ERR, "no SECRET provided with the key: %s\n", arg);
        } else {
                struct key_options* k = tsig_print_secret(xfrd->nsd->options, arg);
                if(k) {
                        log_msg(LOG_ERR, "changing SECRET provided with the key: %s and algo: %s\n", k->secret, k->algorithm);
                        struct nsd_options* new_opt = tsig_update_secret(xfrd->nsd->options, arg, arg2);
                        repat_keys(xfrd, new_opt);
                        repat_patterns(xfrd, new_opt);
                        repat_options(xfrd, new_opt);
                }
        }

        send_ok(ssl);
}

here tsig_print_secret() - is my function placed in options.c:
struct key_options*
tsig_print_secret(struct nsd_options* opt, char *tsig_name)
{
        if(*tsig_name == '\0') {
                log_msg(LOG_ERR, "the KEY must have a name\n");
                return NULL;
        }

        struct key_options* key_opts = key_options_find(opt, tsig_name);
        if(!key_opts) {
                log_msg(LOG_ERR, "do not know such KEY: %s\n", tsig_name);
        } else {
                log_msg(LOG_ERR, "the KEY: %s has SECRET %s with algo: %s\n", tsig_name, key_opts->secret, key_opts->algorithm);
        }

        return key_opts;
}
just to check the current key_options* structure (secret, algorithm).

So when i recompiled and started the program (as a slave DNS with master BIND) i tried to check if it works, i saw the following:

nsd-control update_tsig testkey "P3tf3TRjvQkVCmJF3/Z9vA=="
here
- "testkey" - TSIG key name
- and P3tf3TRjvQkVCmJF3/Z9vA== - new secret that used by master (nsd.conf's key version is: K2tf3TRjvQkVCmJF3/Z9vA==)

Dec 18 14:28:48 manus nsd[45437]: the KEY: testkey has SECRET K2tf3TRjvQkVCmJF3/Z9vA== with algo: hmac-sha256
Dec 18 14:28:48 manus nsd[45437]: changing SECRET provided with the key: K2tf3TRjvQkVCmJF3/Z9vA== and algo: hmac-sha256
Dec 18 14:28:48 manus nsd[45437]: the KEY: testkey has new SECRET P3tf3TRjvQkVCmJF3/Z9vA== and algorithm: hmac-sha256

and when i checked with: nsd-control print_tsig testkey
i saw new TSIG:
Dec 18 14:29:34 manus nsd[45437]: SECRET: P3tf3TRjvQkVCmJF3/Z9vA==, algo: hmac-sha256

But TSIG-transaction between NSD and master was failed:
Dec 18 14:28:14 manus named[44703]: client @0x803069e00 192.168.255.250#48691: request has invalid signature: TSIG testkey: tsig verify failure (BADSIG)
Dec 18 14:28:14 manus nsd[45437]: xfrd: zone ripn.net received error code SERVER NOT AUTHORITATIVE FOR ZONE from 192.168.255.250
Dec 18 14:28:14 manus nsd[45437]: xfrd: zone ripn.net, from 192.168.255.250: tsig: P3tf3TRjvQkVCmJF3/Z9vA==(hmac-sha256) error (Bad Signature)
Dec 18 14:28:14 manus nsd[45437]: xfrd: zone ripn.net, from 192.168.255.250: bad tsig signature

So i would like to ask you, what are the data structures needed to be updated else with new key options for successfull TSIG-transaction with new secrtet value that differ from nsd.conf's one?


Thank you in advance!
Comment 1 Igor 2018-12-18 15:17:41 CET
i'm sorry but there is my own function tsig_update_secret() called from do_putsig() and placed in options.c:

struct nsd_options*
tsig_update_secret(struct nsd_options* opt, char *tsig_name, char *tsig_new_secret)
{
        if(*tsig_name == '\0') {
                log_msg(LOG_ERR, "the KEY must have a name\n");
                return NULL;
        }

        struct key_options* new_key_opts = key_options_find(opt, tsig_name);
        if(!new_key_opts) {
                log_msg(LOG_ERR, "do not know such KEY: %s\n", tsig_name);
                return NULL;
        } else {
                strcpy(new_key_opts->secret, tsig_new_secret);
                key_options_insert(opt, new_key_opts);
                log_msg(LOG_ERR, "the KEY: %s has new SECRET %s and algorithm: %s\n", tsig_name, new_key_opts->secret, new_key_opts->algorithm);
                return opt;
        }
}
Comment 2 Igor 2018-12-18 15:18:12 CET
i'm sorry but there is my own function tsig_update_secret() called from do_putsig() and placed in options.c:

struct nsd_options*
tsig_update_secret(struct nsd_options* opt, char *tsig_name, char *tsig_new_secret)
{
        if(*tsig_name == '\0') {
                log_msg(LOG_ERR, "the KEY must have a name\n");
                return NULL;
        }

        struct key_options* new_key_opts = key_options_find(opt, tsig_name);
        if(!new_key_opts) {
                log_msg(LOG_ERR, "do not know such KEY: %s\n", tsig_name);
                return NULL;
        } else {
                strcpy(new_key_opts->secret, tsig_new_secret);
                key_options_insert(opt, new_key_opts);
                log_msg(LOG_ERR, "the KEY: %s has new SECRET %s and algorithm: %s\n", tsig_name, new_key_opts->secret, new_key_opts->algorithm);
                return opt;
        }
}
Comment 3 Igor 2018-12-21 16:04:28 CET
Hello!

I solved my problem - i forgot to call key_options_tsig_add().

Thanks anyway && happy holidays! :))
Comment 4 Wouter Wijngaards 2019-01-07 12:53:13 CET
Hi Igor,

May I ask why you created nsd-control update key when you could edit the nsd.conf file (or a file included by it) and nsd-control reconfig?  Then the new secret is safely stored in the file.  Otherwise the change would be lost after a restart?

If reconfig is better, then the feature is not really needed.  If the feature is useful could you send or attach or copy a diff of the entire set of changes, I would like to incorporate the feature (if useful) to the code, and I think if I piece it together I may get it wrong and fall afoul of the bug that you report here.

Best regards, Wouter
Comment 5 Igor 2019-01-09 15:21:44 CET
Hello Wouter!

I know about reconfig but it is not desired operation in my problem. So i found the way to add TSIGs without reconfig (BTW to store new keys in config file is one of my TODO plan). Below a share 4 diff-files with you. Maybe you could see some critical errors especialy with memory regions operations. All the patches and debugs weree made on NSD-4.1.15. But it works with NSD-4.1.26 too. Right now there are 5 commands prividing via nsd-control:
- print_tsig (print TSIG key in log file)
- update_tsig (change the secret value in existing TSIG key)
- add_tsig (add new TSIG)
- assoc_tsig (assign added TSIG to zone)
- del_tsig (remove TSIG from NSD if the key is not issigned with a zone)

Plaese reply me your ideas about these patches regarding to security and stability of service. Although i have made several stress-tests (adding/deleiting/associating of 1000 keys) and it worked. Thank you!


file nsd-patch-nsd-control.c.diff:
--- nsd-4.1.26/nsd-control.c    2017-01-19 18:32:27.000000000 +0300
+++ nsd-4.1.15_patched_tsig_on_the_fly/nsd-control.c    2018-12-24 12:54:17.274068000 +0300
@@ -73,26 +73,31 @@
        printf("  -s ip[@port]  server address, if omitted config is used.\n");
        printf("  -h            show this usage help.\n");
        printf("Commands:\n");
-       printf("  start                         start server; runs nsd(8)\n");
-       printf("  stop                          stops the server\n");
-       printf("  reload [<zone>]               reload modified zonefiles from disk\n");
-       printf("  reconfig                      reload the config file\n");
-       printf("  repattern                     the same as reconfig\n");
-       printf("  log_reopen                    reopen logfile (for log rotate)\n");
-       printf("  status                        display status of server\n");
-       printf("  stats                         print statistics\n");
-       printf("  stats_noreset                 peek at statistics\n");
-       printf("  addzone <name> <pattern>      add a new zone\n");
-       printf("  delzone <name>                remove a zone\n");
-       printf("  addzones                      add zone list on stdin {name space pattern newline}\n");
-       printf("  delzones                      remove zone list on stdin {name newline}\n");
-       printf("  write [<zone>]                write changed zonefiles to disk\n");
-       printf("  notify [<zone>]               send NOTIFY messages to slave servers\n");
-       printf("  transfer [<zone>]             try to update slave zones to newer serial\n");
-       printf("  force_transfer [<zone>]       update slave zones with AXFR, no serial check\n");
-       printf("  zonestatus [<zone>]           print state, serial, activity\n");
-       printf("  serverpid                     get pid of server process\n");
-       printf("  verbosity <number>            change logging detail\n");
+       printf("  start                                 start server; runs nsd(8)\n");
+       printf("  stop                                  stops the server\n");
+       printf("  reload [<zone>]                       reload modified zonefiles from disk\n");
+       printf("  reconfig                              reload the config file\n");
+       printf("  repattern                             the same as reconfig\n");
+       printf("  log_reopen                            reopen logfile (for log rotate)\n");
+       printf("  status                                display status of server\n");
+       printf("  stats                                 print statistics\n");
+       printf("  stats_noreset                         peek at statistics\n");
+       printf("  addzone <name> <pattern>              add a new zone\n");
+       printf("  delzone <name>                        remove a zone\n");
+       printf("  addzones                              add zone list on stdin {name space pattern newline}\n");
+       printf("  delzones                              remove zone list on stdin {name newline}\n");
+       printf("  write [<zone>]                        write changed zonefiles to disk\n");
+       printf("  notify [<zone>]                       send NOTIFY messages to slave servers\n");
+       printf("  transfer [<zone>]                     try to update slave zones to newer serial\n");
+       printf("  force_transfer [<zone>]               update slave zones with AXFR, no serial check\n");
+       printf("  zonestatus [<zone>]                   print state, serial, activity\n");
+       printf("  serverpid                             get pid of server process\n");
+       printf("  verbosity <number>                    change logging detail\n");
+       printf("  print_tsig <key_name>                 print tsig with <name> the secret and algo to syslog \n"); //igorr
+       printf("  update_tsig <name> <secret>           change existing tsig with <name> to a new <secret>\n");
+       printf("  add_tsig <name> <secret> [algo]       add new key with the given parameters\n");
+       printf("  assoc_tsig <zone> <key_name>          associate <zone> with given tsig <key_name> name\n");
+       printf("  del_tsig <key_name>                   delete tsig <key_name> from configuration\n");
        exit(1);
 }


file nsd-patch-options.h.diff:
--- nsd-4.1.26/options.h        2017-01-19 18:32:27.000000000 +0300
+++ nsd-4.1.15_patched_tsig_on_the_fly/options.h        2018-12-25 14:35:50.709058000 +0300
@@ -391,4 +391,10 @@
 /* apply pattern to the existing pattern in the parser */
 void config_apply_pattern(const char* name);

+
+/*
+ * igorr: My experimental functions
+ */
+struct key_options* tsig_print_secret(struct nsd_options* opt, char *tsig_name);
+
 #endif /* OPTIONS_H */

file nsd-patch-options.c.diff:
--- nsd-4.1.26/options.c        2017-01-19 18:32:27.000000000 +0300
+++ nsd-4.1.15_patched_tsig_on_the_fly/options.c        2018-12-25 14:35:40.917560000 +0300
@@ -15,6 +15,7 @@
 #include "tsig.h"
 #include "difffile.h"
 #include "rrl.h"
+#include "nsd.h"

 #include "configyyrename.h"
 #include "configparser.h"
@@ -2035,3 +2036,21 @@
        return 0;
 #endif /* USE_ZONE_STATS */
 }
+
+/*
+ * igorr: My experimental functions
+ */
+struct key_options*
+tsig_print_secret(struct nsd_options* opt, char *tsig_name)
+{
+        struct key_options* key_opts = key_options_find(opt, tsig_name);
+
+        if(!key_opts) {
+                log_msg(LOG_ERR, "do not know such KEY: %s\n", tsig_name);
+        } else {
+                log_msg(LOG_ERR, "the KEY: %s has SECRET %s with algo: %s\n", tsig_name, key_opts->secret, key_opts->algorithm);
+        }
+
+       return key_opts;
+}
+


file nsd-patch-remote.c.diff:
--- nsd-4.1.15/remote.c 2017-01-19 18:32:27.000000000 +0300
+++ nsd-4.1.15_patched_tsig_on_the_fly/remote.c 2019-01-09 16:46:23.798015000 +0300
@@ -1065,6 +1065,27 @@
        return 0;
 }

+/** igorr: My experimental functions */
+/** find second and third arguments, modifies string */
+static int
+find_arg3(SSL* ssl, char* arg, char** arg2, char** arg3)
+{
+       if(find_arg2(ssl, arg, arg2)) {
+               *arg3 = *arg2;
+               char* as = strrchr(arg, ' ');
+               if(as) {
+                       as[0]=0;
+                       *arg2 = as+1;
+                       while(isspace((unsigned char)*as) && as > arg)
+                               as--;
+                       as[0]=0;
+                       return 1;
+               }
+       }
+       /** TODO: before `return 0' we could set *arg3 to NULL */
+       return 0;
+}
+
 /** do the status command */
 static void
 do_status(SSL* ssl, xfrd_state_type* xfrd)
@@ -1658,6 +1679,15 @@
                *ssl = NULL; /* failed, stop printing */
 }

+/** igorr: My experimental functions */
+static void
+repat_all(xfrd_state_type* xfrd, struct nsd_options* new_opt)
+{
+       repat_keys(xfrd, new_opt);
+       repat_patterns(xfrd, new_opt);
+       repat_options(xfrd, new_opt);
+}
+
 /** do the repattern command: reread config file and apply keys, patterns */
 static void
 do_repattern(SSL* ssl, xfrd_state_type* xfrd)
@@ -1705,6 +1735,244 @@
        (void)ssl_printf(ssl, "%u\n", (unsigned)xfrd->reload_pid);
 }

+/** igorr: My experimental functions */
+static void
+do_logtsig(SSL* ssl, xfrd_state_type* xfrd, char* arg)
+{
+       if(*arg == '\0') {
+               log_msg(LOG_ERR, "no key NAME provided\n");
+       } else {
+               tsig_print_secret(xfrd->nsd->options, arg);
+       }
+
+        send_ok(ssl);
+}
+
+static void
+do_puttsig(SSL* ssl, xfrd_state_type* xfrd, char* arg)
+{
+        char* arg2 = NULL;
+       uint8_t data[16384]; /* 16KB */
+
+       if(*arg == '\0') {
+               log_msg(LOG_ERR, "no key NAME provided\n");
+       } else {
+               if(!find_arg2(ssl, arg, &arg2)) {
+                       log_msg(LOG_ERR, "no SECRET provided with the key: %s\n", arg);
+               } else {
+                       struct key_options* key_opt = tsig_print_secret(xfrd->nsd->options, arg);
+                       if(key_opt) {
+                               if(b64_pton(arg2, data, sizeof(data)) == -1) {
+                                       log_msg(LOG_ERR, "the SECRET: %s is in unknown format and it should be checked\n", arg2);
+                               } else {
+                                       log_msg(LOG_ERR, "changing SECRET provided with the key: %s and algo: %s\n", key_opt->secret, key_opt->algorithm);
+                                       /* struct nsd_options* new_opt = tsig_update_secret(xfrd->nsd->options, arg, arg2); */
+                                       strcpy(key_opt->secret, arg2);
+                                       key_options_insert(xfrd->nsd->options, key_opt);
+                                       log_msg(LOG_ERR, "the KEY: %s has new SECRET %s and algorithm: %s\n", arg, key_opt->secret, key_opt->algorithm);
+
+                                       repat_all(xfrd, xfrd->nsd->options);
+                                       key_options_tsig_add(xfrd->nsd->options);
+                               }
+                       }
+               }
+       }
+
+        send_ok(ssl);
+}
+
+static void
+do_addtsig(SSL* ssl, xfrd_state_type* xfrd, char* arg)
+{
+       char* arg2 = NULL;
+       char* arg3 = NULL;
+       uint8_t data[16384]; /* 16KB */
+       uint8_t dname[MAXDOMAINLEN];
+       char algo[16];
+
+       if(*arg == '\0') {
+               log_msg(LOG_ERR, "no key NAME provided\n");
+       } else {
+               if(!find_arg3(ssl, arg, &arg2, &arg3)) {
+                       strcpy(algo, "hmac-sha256");
+               } else {
+                       strcpy(algo, arg3);
+               }
+               if(!arg2) {
+                       log_msg(LOG_ERR, "no SECRET provided with the key: %s\n", arg);
+               } else {
+                       if(tsig_print_secret(xfrd->nsd->options, arg)) {
+                               log_msg(LOG_ERR, "the KEY %s allready exists\n", arg);
+                       } else {
+                               if(b64_pton(arg2, data, sizeof(data)) == -1) {
+                                       log_msg(LOG_ERR, "the SECRET: %s is in unknown format and it should be checked\n", arg2);
+                               } else {
+                                       region_type* region = region_create(xalloc, free);
+                                       if(!dname_parse_wire(dname, arg)) {
+                                               log_msg(LOG_ERR, "the key NAME: %s is incorrect and it should be checked\n", arg);
+                                       } else {
+                                               if(tsig_get_algorithm_by_name(algo) == NULL) {
+                                                       log_msg(LOG_ERR, "the key ALGO: %s is unknown and it should be checked\n", algo);
+                                               } else {
+                                                       log_msg(LOG_ERR, "adding the key with NAME: %s and SECRET: %s with ALGO: %s\n", arg, arg2, algo);
+                                                       struct key_options* new_key_opt = key_options_create(region);
+                                                       new_key_opt->name = region_strdup(region, arg);
+                                                       new_key_opt->secret = region_strdup(region, arg2);
+                                                       new_key_opt->algorithm = region_strdup(region, algo);
+                                                       /** major bugfix: call add_key() needed to reload XFRD-state between nsd child processes */
+                                                       add_key(xfrd, new_key_opt);
+                                                       key_options_insert(xfrd->nsd->options, new_key_opt);
+
+                                                       key_options_tsig_add(xfrd->nsd->options);
+                                                       repat_all(xfrd, xfrd->nsd->options);
+                                               }
+                                       }
+                               }
+                       }
+               }
+       }
+
+       send_ok(ssl);
+}
+
+static void
+do_asctsig(SSL* ssl, xfrd_state_type* xfrd, char* arg)
+{
+       char* arg2 = NULL;
+
+       if(*arg == '\0') {
+               log_msg(LOG_ERR, "no ZONE name provided\n");
+       } else {
+               if(!find_arg2(ssl, arg, &arg2)) {
+                       log_msg(LOG_ERR, "no KEY provided to associate it with the ZONE: %s\n", arg);
+               } else {
+                       struct zone_options* zone;
+                       struct key_options* key_opt;
+                       struct acl_options* acl = NULL;
+
+                       key_opt = tsig_print_secret(xfrd->nsd->options, arg2);
+                       if(!key_opt) {
+                               log_msg(LOG_ERR, "the KEY: %s does not exist and it should be added first\n", arg2);
+                       } else {
+                               RBTREE_FOR(zone, struct zone_options*, xfrd->nsd->options->zone_options)
+                               {
+                                       if(strcmp(zone->name, arg))
+                                               continue;
+
+                                       region_type* region = region_create(xalloc, free);
+
+                                       if(zone_is_slave(zone)) {
+                                               log_msg(LOG_ERR, "SLAVE/ALLOW-NOTIFY: associating the ZONE: %s with the KEY: %s\n", arg, arg2);
+                                               acl = zone->pattern->allow_notify;
+                                       } else {
+                                               log_msg(LOG_ERR, "MASTER/NOTIFY: associating the ZONE: %s with the KEY: %s\n", arg, arg2);
+                                               acl = zone->pattern->notify;
+                                       }
+
+                                       do {    /** TODO: add IPv6 handing */
+                                               if(!acl->is_ipv6 && inet_ntoa(acl->addr.addr) && strcmp(inet_ntoa(acl->addr.addr), "127.0.0.1")) {
+                                                       log_msg(LOG_ERR, " ip: %s\n", inet_ntoa(acl->addr.addr));
+                                                       acl->nokey = 0;
+                                                       acl->blocked = 0;
+                                                       acl->key_name = region_strdup(region, arg2);
+                                                       acl->key_options = key_opt;
+                                               }
+                                               acl = acl->next;
+                                       }
+                                       while(acl);
+
+                                       if(zone_is_slave(zone)) {
+                                               log_msg(LOG_ERR, "SLAVE/REQUEST-XFR: associating the ZONE: %s with the KEY: %s\n", arg, arg2);
+                                               acl = zone->pattern->request_xfr;
+                                       } else {
+                                               log_msg(LOG_ERR, "MASTER/PROVIDE-XFR: associating the ZONE: %s with the KEY: %s\n", arg, arg2);
+                                               acl = zone->pattern->provide_xfr;
+                                       }
+
+                                       do {    /** TODO: add IPv6 handing */
+                                               if(!acl->is_ipv6 && inet_ntoa(acl->addr.addr) && strcmp(inet_ntoa(acl->addr.addr), "127.0.0.1")) {
+                                                       log_msg(LOG_ERR, " ip: %s\n", inet_ntoa(acl->addr.addr));
+                                                       acl->nokey = 0;
+                                                       acl->blocked = 0;
+                                                       acl->key_name = region_strdup(region, arg2);
+                                                       acl->key_options = key_opt;
+                                               }
+                                               acl = acl->next;
+                                       }
+                                       while(acl);
+
+                                       key_options_tsig_add(xfrd->nsd->options);
+                                       repat_all(xfrd, xfrd->nsd->options);
+                               }
+                       }
+               }
+       }
+
+       send_ok(ssl);
+}
+
+static void
+do_deltsig(SSL* ssl, xfrd_state_type* xfrd, char* arg) {
+       int used_key = 0;
+       struct zone_options* zone;
+       struct key_options* key_opt;
+       struct acl_options* acl = NULL;
+
+       if(*arg == '\0') {
+               log_msg(LOG_ERR, "no KEY name provided\n");
+       } else {
+               key_opt = tsig_print_secret(xfrd->nsd->options, arg);
+               if(!key_opt) {
+                       log_msg(LOG_ERR, "the KEY: %s does not exist, nothing to be deleted\n", arg);
+               } else {
+                       RBTREE_FOR(zone, struct zone_options*, xfrd->nsd->options->zone_options)
+                       {
+                               acl = zone_is_slave(zone)!=0?zone->pattern->allow_notify:zone->pattern->notify;
+
+                               do {
+                                       if(acl->key_name) {
+                                               if(!strcmp(acl->key_name, arg)) {
+                                                       used_key = 1;
+                                                       break;
+                                               }
+                                       }
+                                       acl = acl->next;
+                               }
+                               while(acl);
+
+                               if(used_key)
+                                       break;
+
+                               acl = zone_is_slave(zone)!=0?zone->pattern->request_xfr:zone->pattern->provide_xfr;
+
+                               do {
+                                       if(acl->key_name) {
+                                               if(!strcmp(acl->key_name, arg)) {
+                                                       used_key = 1;
+                                                       break;
+                                               }
+                                       }
+                                       acl = acl->next;
+                               }
+                               while(acl);
+
+                               if(used_key)
+                                       break;
+                       }
+
+                       if(used_key) {
+                               log_msg(LOG_ERR, "the KEY: %s is in use and it cannot be deleted\n", arg);
+                       } else {
+                               key_options_remove(xfrd->nsd->options, arg);
+                               repat_all(xfrd, xfrd->nsd->options);
+                               log_msg(LOG_ERR, "the KEY: %s is successfully deleted\n", arg);
+                       }
+               }
+       }
+
+       send_ok(ssl);
+}
+
 /** check for name with end-of-string, space or tab after it */
 static int
 cmdcmp(char* p, const char* cmd, size_t len)
@@ -1756,6 +2024,16 @@
                do_repattern(ssl, rc->xfrd);
        } else if(cmdcmp(p, "serverpid", 9)) {
                do_serverpid(ssl, rc->xfrd);
+       } else if(cmdcmp(p, "print_tsig", 10)) { // igorr: My experimental stuff
+               do_logtsig(ssl, rc->xfrd, skipwhite(p+10));
+        } else if(cmdcmp(p, "update_tsig", 11)) { // igorr: My experimental stuff
+                do_puttsig(ssl, rc->xfrd, skipwhite(p+11));
+        } else if(cmdcmp(p, "add_tsig", 8)) { // igorr: My experimental stuff
+                do_addtsig(ssl, rc->xfrd, skipwhite(p+8));
+        } else if(cmdcmp(p, "assoc_tsig", 10)) { // igorr: My experimental stuff
+                do_asctsig(ssl, rc->xfrd, skipwhite(p+10));
+        } else if(cmdcmp(p, "del_tsig", 8)) { // igorr: My experimental stuff
+                do_deltsig(ssl, rc->xfrd, skipwhite(p+8));
        } else {
                (void)ssl_printf(ssl, "error unknown command '%s'\n", p);
        }
Comment 6 Wouter Wijngaards 2019-01-29 15:48:34 CET
Hi Igor,

I have applied the patch.  Made a large number of edits.  The commands print to the nsd-control output, instead of syslog now.  Also the print_tsig command (I could add another log_tsig command that prints to syslog if that is what was wanted).  Also removed a potential buffer overflow, and memory allocation issues (allocated in the wrong areas), around the string copies.

Other than the neatening, the code is pretty much as you sent.  And for the assoc_tsig and del_tsig commands, it applied to all the acl list options lists, so that the master or slave of the zone does not matter for the outcome.  Also makes sure there are no other acl elements that could then still refer to the wrong elements.  The assoc routine skips BLOCK acls, so that those stay the way they are (I guess blocking certain users).

Also in your code you only update for 127.0.0.1, but I changed it to update always for acls.  In the assoc_tsig routine.  Because I think that is what the user would expect to happen.  That then also supports IPv6 for it.

Thank you for the patch, I hope things work as desired.

Best regards, Wouter
Comment 7 Igor 2019-01-29 16:43:19 CET
Hello, Wouter!

BIG thank you for reply!

And i would like to ask you for - where can i get your patch version? Or maybe it will be applied to next NSD release?
Comment 8 Wouter Wijngaards 2019-01-29 16:47:17 CET
Hi Igor,

Yes in the next release, and it is also in the code repository, the files
https://nlnetlabs.nl/svn/nsd/trunk/nsd-control.c
https://nlnetlabs.nl/svn/nsd/trunk/remote.c
https://nlnetlabs.nl/svn/nsd/trunk/nsd-control.8.in

These files contain the new code.  svn can also create a diff if you would like.

Best regards, Wouter
Comment 9 Igor 2019-01-29 16:54:36 CET
Ok, Wouter!
BIG thanks for your great help!

Tomorrow i will try to dig into the new code in these files ;)
Comment 10 Igor 2019-02-01 14:56:17 CET
Hello, Wouter!

I have made some stress-tests and found the following exception when the NSD was shutting down:
- its the command: nsd-control assoc_tsig <my_zone> <my_some_existing_key>

I saw into the code in remote.c, function do_assoc_tsig() where you stated: struct zone_options* zone;
But i didn't find the zone structure initialization. Do we have to initialize this structure? Just like we are doing with, for example, key_opt: key_opt = key_options_find(xfrd->nsd->options, arg2);

Some interesting thing: if i'm commenting the line with calling region_recycle() in function zopt_set_acl_to_tsig(), NSD is not shutting down when i send assoc_tsig command.


Thank you for help in advance!
Comment 11 Wouter Wijngaards 2019-02-01 15:03:50 CET
Hi Igor,

Thank you for the test.

This is a bug you mention, it is the strlen call that fails on a NULL pointer.  The zone variable initialisation happens in the get zone arg function, but I also add a check that it worked.

This should fix it:

Index: remote.c
===================================================================
--- remote.c	(revision 4964)
+++ remote.c	(working copy)
@@ -2119,8 +2119,9 @@
 			continue;
 		}
 		acl->nokey = 0;
-		region_recycle(region, (void*)acl->key_name,
-			strlen(acl->key_name)+1);
+		if(acl->key_name)
+			region_recycle(region, (void*)acl->key_name,
+				strlen(acl->key_name)+1);
 		acl->key_name = region_strdup(region, key_name);
 		acl->key_options = key_opt;
 		acl = acl->next;
@@ -2149,6 +2150,11 @@
 
 	if(!get_zone_arg(ssl, xfrd, arg, &zone))
 		return;
+	if(!zone) {
+		if(!ssl_printf(ssl, "error: missing argument (zone)"))
+			return;
+		return;
+	}
 	key_opt = key_options_find(xfrd->nsd->options, arg2);
 	if(!key_opt) {
 		if(!ssl_printf(ssl, "error: key: %s does not exist", arg2))

Best regards, Wouter
Comment 12 Igor 2019-02-01 15:56:08 CET
Now it works!

Thank you for detail explanation. On monday i'll continue the tests.
Comment 13 Igor 2019-02-01 16:24:47 CET
And one more exception i've found:
- when i do 'del_tsig' command the NSD either 100% eats CPU or is just shutting down. In function acl_contains_tsig_key() inside while() loop i've added (at the end of cycle) the line: acl = acl->next;

Am i right?
Comment 14 Wouter Wijngaards 2019-02-01 16:27:41 CET
Hi Igor,

Yes, that was a stupid mistake of me.

Index: remote.c
===================================================================
--- remote.c	(revision 4965)
+++ remote.c	(working copy)
@@ -2183,6 +2183,7 @@
 	while(acl) {
 		if(acl->key_name && strcmp(acl->key_name, name) == 0)
 			return 1;
+		acl = acl->next;
 	}
 	return 0;
 }

Best regards, Wouter
Comment 15 Igor 2019-02-01 16:39:38 CET
Thanks a lot, Wouter!

I've reloaded svn trunk, so on Monday i will do other stress tests! Have a good weekend ;)
Comment 16 Igor 2019-02-11 12:07:36 CET
Hello, Wouter!

I have done all stress-tests on the new code. The functions do_add_tsig(), do_assoc_tsig() and do_del_tsig() work fine.
But there is one function do_update_tsig() in which one problem takes place:
When i'm updating a secret for current key (nsd-control update_tsig <mykey> <mysecdret>) and then changing master zone contents (NSD here as a slave) with the zone reload (i.e. master sends NOTIFY to NSD), in NSD log i'm seeing the message "BAD tsig". And if i do: nsd-control force_transfer <myzone> (i.e. NSD do AXFR from master), transaction succeeds. The function do_print_tsig() prints correct (updated) tsig key.

I had same problem in the case of function do_add_tsig(). And the solution was the call of add_key() that did xfrd_set_reload_now() to propogate added key to all NSD processes (if i understood correctly). In do_update_tsig() this way does not work, the service is shutting down during update operation.

So could you please help me, where we must change the secret with new value else?
Comment 17 Wouter Wijngaards 2019-02-14 17:08:55 CET
Hi Igor,

This long commit, also available from the code repository, should fix the issues.  I fixed some more.  It is a fixup for state updates for the TSIG information in the server processes, it has nicer printout for tsig_print, and you can also tsig_print without arguments (this lists all the tsigs) and I removed memory leaks (tsig config information).

Best regards, Wouter

Index: options.h
===================================================================
--- options.h	(revision 4966)
+++ options.h	(working copy)
@@ -337,6 +337,8 @@
 void key_options_remove(struct nsd_options* opt, const char* name);
 int key_options_equal(struct key_options* p, struct key_options* q);
 void key_options_add_modify(struct nsd_options* opt, struct key_options* key);
+void key_options_setup(region_type* region, struct key_options* key);
+void key_options_desetup(region_type* region, struct key_options* key);
 /* read in zone list file. Returns false on failure */
 int parse_zone_list_file(struct nsd_options* opt);
 /* create zone entry and add to the zonelist file */
Index: remote.c
===================================================================
--- remote.c	(revision 4966)
+++ remote.c	(working copy)
@@ -1907,15 +1907,6 @@
 		*ssl = NULL; /* failed, stop printing */
 }
 
-/* repat all (keys, patterns, options) for new options */
-static void
-repat_all(xfrd_state_type* xfrd, struct nsd_options* new_opt)
-{
-	repat_keys(xfrd, new_opt);
-	repat_patterns(xfrd, new_opt);
-	repat_options(xfrd, new_opt);
-}
-
 /** do the repattern command: reread config file and apply keys, patterns */
 static void
 do_repattern(RES* ssl, xfrd_state_type* xfrd)
@@ -1968,22 +1959,23 @@
 do_print_tsig(RES* ssl, xfrd_state_type* xfrd, char* arg)
 {
 	if(*arg == '\0') {
-		if(!ssl_printf(ssl, "error: missing argument (keyname)"))
-			return;
+		struct key_options* key;
+		RBTREE_FOR(key, struct key_options*, xfrd->nsd->options->keys) {
+			if(!ssl_printf(ssl, "key: name: \"%s\" secret: \"%s\" algorithm: %s\n", key->name, key->secret, key->algorithm))
+				return;
+		}
 		return;
 	} else {
 		struct key_options* key_opts = key_options_find(xfrd->nsd->options, arg);
 		if(!key_opts) {
-			if(!ssl_printf(ssl, "error: no such key with name: %s", arg))
+			if(!ssl_printf(ssl, "error: no such key with name: %s\n", arg))
 				return;
 			return;
 		} else {
-			if(!ssl_printf(ssl, "the KEY: %s has SECRET %s with algo: %s", arg, key_opts->secret, key_opts->algorithm))
+			if(!ssl_printf(ssl, "key: name: \"%s\" secret: \"%s\" algorithm: %s\n", arg, key_opts->secret, key_opts->algorithm))
 				return;
 		}
 	}
-
-	send_ok(ssl);
 }
 
 /** do the update_tsig command: change existing tsig to new secret */
@@ -1996,30 +1988,30 @@
 	struct key_options* key_opt;
 
 	if(*arg == '\0') {
-		if(!ssl_printf(ssl, "error: missing argument (keyname)"))
+		if(!ssl_printf(ssl, "error: missing argument (keyname)\n"))
 			return;
 		return;
 	}
 	if(!find_arg2(ssl, arg, &arg2)) {
-		if(!ssl_printf(ssl, "error: missing argument (secret)"))
+		if(!ssl_printf(ssl, "error: missing argument (secret)\n"))
 			return;
 		return;
 	}
 	key_opt = key_options_find(xfrd->nsd->options, arg);
 	if(!key_opt) {
-		if(!ssl_printf(ssl, "error: no such key with name: %s", arg))
+		if(!ssl_printf(ssl, "error: no such key with name: %s\n", arg))
 			return;
 		memset(arg2, 0xdd, strlen(arg2));
 		return;
 	}
 	if(b64_pton(arg2, data, sizeof(data)) == -1) {
-		if(!ssl_printf(ssl, "error: the secret: %s is not in b64 format", arg2))
+		if(!ssl_printf(ssl, "error: the secret: %s is not in b64 format\n", arg2))
 			return;
 		memset(data, 0xdd, sizeof(data)); /* wipe secret */
 		memset(arg2, 0xdd, strlen(arg2));
 		return;
 	}
-	log_msg(LOG_INFO, "changing secret provided with the key: %s and algo: %s", key_opt->secret, key_opt->algorithm);
+	log_msg(LOG_INFO, "changing secret provided with the key: %s and algo: %s\n", key_opt->secret, key_opt->algorithm);
 	if(key_opt->secret) {
 		/* wipe old secret */
 		memset(key_opt->secret, 0xdd, strlen(key_opt->secret));
@@ -2027,14 +2019,16 @@
 			strlen(key_opt->secret)+1);
 	}
 	key_opt->secret = region_strdup(region, arg2);
-	key_options_insert(xfrd->nsd->options, key_opt);
-	log_msg(LOG_INFO, "the key: %s has new secret %s and algorithm: %s", arg, key_opt->secret, key_opt->algorithm);
+	log_msg(LOG_INFO, "the key: %s has new secret %s and algorithm: %s\n", arg, key_opt->secret, key_opt->algorithm);
 	/* wipe secret from temp parse buffer */
 	memset(arg2, 0xdd, strlen(arg2));
 	memset(data, 0xdd, sizeof(data));
 
-	repat_all(xfrd, xfrd->nsd->options);
-	key_options_tsig_add(xfrd->nsd->options);
+	key_options_desetup(region, key_opt);
+	key_options_setup(region, key_opt);
+	task_new_add_key(xfrd->nsd->task[xfrd->nsd->mytask], xfrd->last_task,
+		key_opt);
+	xfrd_set_reload_now(xfrd);
 
 	send_ok(ssl);
 }
@@ -2052,7 +2046,7 @@
 	struct key_options* new_key_opt;
 
 	if(*arg == '\0') {
-		if(!ssl_printf(ssl, "error: missing argument (keyname)"))
+		if(!ssl_printf(ssl, "error: missing argument (keyname)\n"))
 			return;
 		return;
 	}
@@ -2062,18 +2056,18 @@
 		strlcpy(algo, arg3, sizeof(algo));
 	}
 	if(!arg2) {
-		if(!ssl_printf(ssl, "error: missing argument (secret)"))
+		if(!ssl_printf(ssl, "error: missing argument (secret)\n"))
 			return;
 		return;
 	}
 	if(key_options_find(xfrd->nsd->options, arg)) {
-		if(!ssl_printf(ssl, "error: key %s already exists", arg))
+		if(!ssl_printf(ssl, "error: key %s already exists\n", arg))
 			return;
 		memset(arg2, 0xdd, strlen(arg2));
 		return;
 	}
 	if(b64_pton(arg2, data, sizeof(data)) == -1) {
-		if(!ssl_printf(ssl, "error: the secret: %s is not in b64 format", arg2))
+		if(!ssl_printf(ssl, "error: the secret: %s is not in b64 format\n", arg2))
 			return;
 		memset(data, 0xdd, sizeof(data)); /* wipe secret */
 		memset(arg2, 0xdd, strlen(arg2));
@@ -2081,28 +2075,24 @@
 	}
 	memset(data, 0xdd, sizeof(data)); /* wipe secret from temp buffer */
 	if(!dname_parse_wire(dname, arg)) {
-		if(!ssl_printf(ssl, "error: could not parse key name: %s", arg))
+		if(!ssl_printf(ssl, "error: could not parse key name: %s\n", arg))
 			return;
 		memset(arg2, 0xdd, strlen(arg2));
 		return;
 	}
 	if(tsig_get_algorithm_by_name(algo) == NULL) {
-		if(!ssl_printf(ssl, "error: unknown algorithm: %s", algo))
+		if(!ssl_printf(ssl, "error: unknown algorithm: %s\n", algo))
 			return;
 		memset(arg2, 0xdd, strlen(arg2));
 		return;
 	}
-	log_msg(LOG_INFO, "adding key with name: %s and secret: %s with algo: %s", arg, arg2, algo);
+	log_msg(LOG_INFO, "adding key with name: %s and secret: %s with algo: %s\n", arg, arg2, algo);
 	new_key_opt = key_options_create(region);
 	new_key_opt->name = region_strdup(region, arg);
 	new_key_opt->secret = region_strdup(region, arg2);
 	new_key_opt->algorithm = region_strdup(region, algo);
 	add_key(xfrd, new_key_opt);
-	key_options_insert(xfrd->nsd->options, new_key_opt);
 
-	key_options_tsig_add(xfrd->nsd->options);
-	repat_all(xfrd, xfrd->nsd->options);
-
 	/* wipe secret from temp buffer */
 	memset(arg2, 0xdd, strlen(arg2));
 	send_ok(ssl);
@@ -2138,12 +2128,12 @@
 	struct key_options* key_opt;
 
 	if(*arg == '\0') {
-		if(!ssl_printf(ssl, "error: missing argument (zonename)"))
+		if(!ssl_printf(ssl, "error: missing argument (zonename)\n"))
 			return;
 		return;
 	}
 	if(!find_arg2(ssl, arg, &arg2)) {
-		if(!ssl_printf(ssl, "error: missing argument (keyname)"))
+		if(!ssl_printf(ssl, "error: missing argument (keyname)\n"))
 			return;
 		return;
 	}
@@ -2151,13 +2141,13 @@
 	if(!get_zone_arg(ssl, xfrd, arg, &zone))
 		return;
 	if(!zone) {
-		if(!ssl_printf(ssl, "error: missing argument (zone)"))
+		if(!ssl_printf(ssl, "error: missing argument (zone)\n"))
 			return;
 		return;
 	}
 	key_opt = key_options_find(xfrd->nsd->options, arg2);
 	if(!key_opt) {
-		if(!ssl_printf(ssl, "error: key: %s does not exist", arg2))
+		if(!ssl_printf(ssl, "error: key: %s does not exist\n", arg2))
 			return;
 		return;
 	}
@@ -2170,8 +2160,9 @@
 	zopt_set_acl_to_tsig(zone->pattern->provide_xfr, region, arg2,
 		key_opt);
 
-	key_options_tsig_add(xfrd->nsd->options);
-	repat_all(xfrd, xfrd->nsd->options);
+	task_new_add_pattern(xfrd->nsd->task[xfrd->nsd->mytask],
+		xfrd->last_task, zone->pattern);
+	xfrd_set_reload_now(xfrd);
 
 	send_ok(ssl);
 }
@@ -2196,13 +2187,13 @@
 	struct key_options* key_opt;
 
 	if(*arg == '\0') {
-		if(!ssl_printf(ssl, "error: missing argument (keyname)"))
+		if(!ssl_printf(ssl, "error: missing argument (keyname)\n"))
 			return;
 		return;
 	}
 	key_opt = key_options_find(xfrd->nsd->options, arg);
 	if(!key_opt) {
-		if(!ssl_printf(ssl, "key %s does not exist, nothing to be deleted", arg))
+		if(!ssl_printf(ssl, "key %s does not exist, nothing to be deleted\n", arg))
 			return;
 		return;
 	}
@@ -2212,7 +2203,7 @@
 		   acl_contains_tsig_key(zone->pattern->notify, arg) ||
 		   acl_contains_tsig_key(zone->pattern->request_xfr, arg) ||
 		   acl_contains_tsig_key(zone->pattern->provide_xfr, arg)) {
-			if(!ssl_printf(ssl, "zone %s uses key %s",
+			if(!ssl_printf(ssl, "zone %s uses key %s\n",
 				zone->name, arg))
 				return;
 			used_key = 1;
@@ -2221,13 +2212,12 @@
 	}
 
 	if(used_key) {
-		if(!ssl_printf(ssl, "error: key: %s is in use and cannot be deleted", arg))
+		if(!ssl_printf(ssl, "error: key: %s is in use and cannot be deleted\n", arg))
 			return;
 		return;
 	} else {
-		key_options_remove(xfrd->nsd->options, arg);
-		repat_all(xfrd, xfrd->nsd->options);
-		log_msg(LOG_INFO, "key: %s is successfully deleted", arg);
+		remove_key(xfrd, arg);
+		log_msg(LOG_INFO, "key: %s is successfully deleted\n", arg);
 	}
 
 	send_ok(ssl);
Index: nsd-control.8.in
===================================================================
--- nsd-control.8.in	(revision 4966)
+++ nsd-control.8.in	(working copy)
@@ -153,8 +153,9 @@
 .B verbosity <number>
 Change logging verbosity.
 .TP
-.B print_tsig <key_name>
+.B print_tsig [<key_name>]
 print the secret and algorithm for the TSIG key with that name.
+Or list all the tsig keys with their name, secret and algorithm.
 .TP
 .B update_tsig <name> <secret>
 Change existing TSIG key with name to the new secret.  The secret is
Index: nsd-control.c
===================================================================
--- nsd-control.c	(revision 4966)
+++ nsd-control.c	(working copy)
@@ -101,7 +101,7 @@
 	printf("  zonestatus [<zone>]		print state, serial, activity\n");
 	printf("  serverpid			get pid of server process\n");
 	printf("  verbosity <number>		change logging detail\n");
-	printf("  print_tsig <key_name>		print tsig with <name> the secret and algo\n");
+	printf("  print_tsig [<key_name>]	print tsig with <name> the secret and algo\n");
 	printf("  update_tsig <name> <secret>	change existing tsig with <name> to a new <secret>\n");
 	printf("  add_tsig <name> <secret> [algo] add new key with the given parameters\n");
 	printf("  assoc_tsig <zone> <key_name>	associate <zone> with given tsig <key_name> name\n");
Comment 18 Igor 2019-02-15 13:41:09 CET
Hello, Wouter!

Now all the tests look very good! Thank you for your great help in understanding of NSD internals! So i'm ready to start NSD with new code in my production. I have to do some minor stress-tests but i'm sure the code works well. Memory leaks went away too.

Digging the code, in general changes, you have removed my some additional functions replacing them with 'task_new_add_pattern()' and 'xfrd_set_reload_now()' calls. What also prevents from additional creating of memory regions. And also in the do_update_tsig() you have added key_options_desetup()/key_options_setup() calls to update TSIG secret. These calls are recreating memory region for the existing TSIG, am i right?

Well, for now i will do some tests and wait for 4.1.27 release ;)
Comment 19 Wouter Wijngaards 2019-02-15 13:49:11 CET
Hi Igor,

Nice to hear it works!

The task_add_pattern functions send a message to the other processes of the change.  This makes the server-process not give 'bad tsig' errors.

The tsig desetup and setup functions update tsig copy of the un-base64-ed version of the secret string.  This is in the current process, where the TSIG key can then be used with the new value.  The task_.. functions send a message that makes other processes do the same.

The xfrd_set_reload_now function is used to instruct that the message(s) have to be sent to the other processes.  It actually activates after the nsd-control command is done and then the messages get sent in the event loop.

Best regards, Wouter
Comment 20 Igor 2019-02-15 14:29:59 CET
Thank you for the cool explanation, Wouter!

It helps me to understand your great NSD, how it works from inside. Maybe in future i will share a couple of my another thoughts with you ;)