Pro-actively set cookie_secret_file value

Right after config is read, so no wrapper is needed anymore to determine the value.
NLnetLabs · Oct 23, 2024 · 97fec31 · 97fec31
1 parent b090af7
commit 97fec31
Show file tree

Hide file tree

Showing 7 changed files with 112 additions and 86 deletions.
diff --git a/configparser.y b/configparser.y
@@ -541,7 +541,17 @@ server_option:
       }
     }
   | VAR_COOKIE_SECRET_FILE STRING
-    { cfg_parser->opt->cookie_secret_file = region_strdup(cfg_parser->opt->region, $2); }
+    {
+      /* Empty filename means explicitly disabled cookies from file, internally
+       * represented as NULL.
+       * Note that after parsing, if no value was configured, then
+       * cookie_secret_file_is_default is still 1, then the default cookie
+       * secret file value will be assigned to cookie_secret_file.
+       */
+      if(*$2) cfg_parser->opt->cookie_secret_file = region_strdup(cfg_parser->opt->region, $2);
+      cfg_parser->opt->cookie_secret_file_is_default = 0;
+    }
+
   | VAR_XFRD_TCP_MAX number
     { cfg_parser->opt->xfrd_tcp_max = (int)$2; }
   | VAR_XFRD_TCP_PIPELINE number

diff --git a/nsd-checkconf.c b/nsd-checkconf.c
@@ -166,23 +166,13 @@ usage(void)
 }
 
 static void
-print_string_var_default(const char* varname, const char* value,
-		const char* default_value)
+print_string_var(const char* varname, const char* value)
 {
-	if (value) {
-		printf("\t%s \"%s\"\n", varname, value);
-	} else if (default_value) {
-		printf("\t#%s \"%s\"\n", varname, default_value);
-	} else {
+	if (!value) {
 		printf("\t#%s\n", varname);
+	} else {
+		printf("\t%s \"%s\"\n", varname, value);
 	}
-
-}
-
-static void
-print_string_var(const char* varname, const char* value)
-{
-	print_string_var_default(varname, value, NULL);
 }
 
 static void
@@ -731,7 +721,13 @@ config_test_print_server(nsd_options_type* opt)
 	printf("\tanswer-cookie: %s\n", opt->answer_cookie?"yes":"no");
 	print_string_var("cookie-secret:", opt->cookie_secret);
 	print_string_var("cookie-staging-secret:", opt->cookie_staging_secret);
-	print_string_var_default("cookie-secret-file:", opt->cookie_secret_file, COOKIESECRETSFILE);
+	if(opt->cookie_secret_file_is_default) {
+		print_string_var("#cookie-secret-file:", opt->cookie_secret_file);
+	} else if(opt->cookie_secret_file) {
+		print_string_var("cookie-secret-file:", opt->cookie_secret_file);
+	} else {
+		print_string_var("cookie-secret-file:", "");
+	}
 	if(opt->proxy_protocol_port) {
 		struct proxy_protocol_port_list* p;
 		for(p = opt->proxy_protocol_port; p; p = p->next)

diff --git a/options.c b/options.c
@@ -154,6 +154,7 @@ nsd_options_create(region_type* region)
 	opt->cookie_secret = NULL;
 	opt->cookie_staging_secret = NULL;
 	opt->cookie_secret_file = NULL;
+	opt->cookie_secret_file_is_default = 1;
 	opt->control_enable = 0;
 	opt->control_interface = NULL;
 	opt->control_port = NSD_CONTROL_PORT;
@@ -258,6 +259,11 @@ parse_options_file(struct nsd_options* opt, const char* file,
 
 	opt->configfile = region_strdup(opt->region, file);
 
+	/* Set default cookie_secret_file value */
+	if(opt->cookie_secret_file_is_default && !opt->cookie_secret_file) {
+		opt->cookie_secret_file =
+			region_strdup(opt->region, COOKIESECRETSFILE);
+	}
 	/* Semantic errors */
 	if(opt->cookie_staging_secret && !opt->cookie_secret) {
 		c_error("a cookie-staging-secret cannot be configured without "

diff --git a/options.h b/options.h
@@ -213,6 +213,8 @@ struct nsd_options {
 	char *cookie_staging_secret;
 	/** path to cookie secret store */
 	char *cookie_secret_file;
+	/** set when the cookie_secret_file whas not explicitely configured */
+	uint8_t cookie_secret_file_is_default;
 	/** enable verify */
 	int verify_enable;
 	/** list of ip addresses used to serve zones for verification */
@@ -493,15 +495,6 @@ int nsd_options_insert_zone(struct nsd_options* opt, struct zone_options* zone);
 int nsd_options_insert_pattern(struct nsd_options* opt,
 	struct pattern_options* pat);
 
-/* return the configured cookie secrets filename or NULL if disabled */
-static inline const char* cookie_secret_file(struct nsd_options* opt)
-{
-	/* NULL means the default of COOKIESECRETSFILE, "" means disabled */
-	return opt->cookie_secret_file
-	     ? ( *opt->cookie_secret_file ? opt->cookie_secret_file : NULL )
-	     : COOKIESECRETSFILE;
-}
-
 /* parses options file. Returns false on failure. callback, if nonNULL,
  * gets called with error strings, default prints. */
 int parse_options_file(struct nsd_options* opt, const char* file,

diff --git a/remote.c b/remote.c
@@ -1981,6 +1981,8 @@ repat_cookie_options_changed(struct nsd_options* old, struct nsd_options* new)
 	                      , new->cookie_secret)
 	    || opt_str_changed( old->cookie_staging_secret
 	                      , new->cookie_staging_secret)
+	    || old->cookie_secret_file_is_default !=
+	       new->cookie_secret_file_is_default
 	    || opt_str_changed( old->cookie_secret_file
 	                      , new->cookie_secret_file);
 }
@@ -2011,6 +2013,8 @@ repat_options(xfrd_state_type* xfrd, struct nsd_options* newopt)
 		region_str_replace(  oldopt->region
 		                  , &oldopt->cookie_staging_secret
 		                  ,  newopt->cookie_staging_secret);
+		oldopt->cookie_secret_file_is_default =
+			newopt->cookie_secret_file_is_default;
 		region_str_replace(  oldopt->region
 		                  , &oldopt->cookie_secret_file
 		                  ,  newopt->cookie_secret_file);
@@ -2385,7 +2389,7 @@ do_del_tsig(RES* ssl, xfrd_state_type* xfrd, char* arg) {
 static int
 can_dump_cookie_secrets(RES* ssl, nsd_type* const nsd)
 {
-	if(!cookie_secret_file(nsd->options))
+	if(!nsd->options->cookie_secret_file)
 		(void)ssl_printf(ssl, "error: empty cookie-secret-file\n");
 
 	else if(nsd->cookie_secrets_source == COOKIE_SECRETS_FROM_CONFIG)
@@ -2408,14 +2412,14 @@ cookie_secret_file_dump_and_reload(RES* ssl, nsd_type* const nsd) {
 	size_t i;
 
 	/* open write only and truncate */
-	if(!cookie_secret_file(nsd->options)) {
+	if(!nsd->options->cookie_secret_file) {
 		(void)ssl_printf(ssl, "cookie-secret-file empty\n");
 		return 0;
 	}
-	else if((f = fopen(cookie_secret_file(nsd->options), "w")) == NULL ) {
+	else if((f = fopen(nsd->options->cookie_secret_file, "w")) == NULL ) {
 		(void)ssl_printf( ssl
 		                , "unable to open cookie secret file %s: %s\n"
-		                , cookie_secret_file(nsd->options)
+		                , nsd->options->cookie_secret_file
 		                , strerror(errno));
 		return 0;
 	}
@@ -2432,7 +2436,7 @@ cookie_secret_file_dump_and_reload(RES* ssl, nsd_type* const nsd) {
 	fclose(f);
 	nsd->cookie_secrets_source = COOKIE_SECRETS_FROM_FILE;
 	region_str_replace(nsd->region, &nsd->cookie_secrets_filename
-	                              , cookie_secret_file(nsd->options));
+	                              , nsd->options->cookie_secret_file);
 	task_new_cookies(xfrd->nsd->task[xfrd->nsd->mytask], xfrd->last_task,
 		nsd->do_answer_cookie, nsd->cookie_count, nsd->cookie_secrets);
 	xfrd_set_reload_now(xfrd);
@@ -2533,7 +2537,7 @@ do_add_cookie_secret(RES* ssl, xfrd_state_type* xrfd, char* arg) {
 	if(!cookie_secret_file_dump_and_reload(ssl, nsd)) {
 		explicit_bzero(arg, strlen(arg));
 		(void)ssl_printf(ssl, "error: writing to cookie secret file: \"%s\"\n"
-		                , cookie_secret_file(nsd->options));
+		                , nsd->options->cookie_secret_file);
 		memcpy( nsd->cookie_secrets, backup_cookie_secrets
 		      , sizeof(cookie_secrets_type));
 		nsd->cookie_count = backup_cookie_count;

diff --git a/simdzone b/simdzone
diff --git a/util.c b/util.c
@@ -1175,30 +1175,69 @@ void drop_cookie_secret(struct nsd* nsd)
 	nsd->cookie_count -= 1;
 }
 
-static
-int cookie_secret_file_read(nsd_type* nsd) {
+void reconfig_cookies(struct nsd* nsd, struct nsd_options* options)
+{
 	cookie_secret_type cookie_secrets[NSD_COOKIE_HISTORY_SIZE];
 	char secret[NSD_COOKIE_SECRET_SIZE * 2 + 2/*'\n' and '\0'*/];
-	FILE* f;
+	FILE* f = NULL;
 	size_t count = 0;
 	const char* fn;
+	size_t i, j;
 
-	if(!(fn = cookie_secret_file(nsd->options)))
-		return 1; /* Explicitely disabled with empty filename */
+	nsd->do_answer_cookie = options->answer_cookie;
+
+	/* Cookie secrets in the configuration file take precedence */
+	if(options->cookie_secret) {
+		ssize_t len = hex_pton(options->cookie_secret,
+				nsd->cookie_secrets[0].cookie_secret,
+				NSD_COOKIE_SECRET_SIZE);
+
+		/* Cookie length guaranteed in configparser.y */
+		assert(len == NSD_COOKIE_SECRET_SIZE);
+		nsd->cookie_count = 1;
+		if(options->cookie_staging_secret) {
+			len = hex_pton(options->cookie_staging_secret,
+					nsd->cookie_secrets[1].cookie_secret,
+					NSD_COOKIE_SECRET_SIZE);
+			/* Cookie length guaranteed in configparser.y */
+			assert(len == NSD_COOKIE_SECRET_SIZE);
+			nsd->cookie_count = 2;
+		}
+		/*************************************************************/
+		nsd->cookie_secrets_source = COOKIE_SECRETS_FROM_CONFIG;
+		return;
+		/*************************************************************/
+	}
+	/* Are cookies from file explicitly disabled? */
+	if(!(fn = nsd->options->cookie_secret_file))
+		goto generate_cookie_secrets;
 
 	else if((f = fopen(fn, "r")) != NULL)
 		; /* pass */
 
-	/* a non-existing cookie file is not an error */
+	/* a non-existing cookie file is not necessarily an error */
 	else if(errno != ENOENT) {
 		log_msg( LOG_ERR
 		       , "error reading cookie secret file \"%s\": \"%s\""
 		       , fn, strerror(errno));
-		return 0;
-	}
-	else if(nsd->options->cookie_secret_file != NULL /* explicit name */
-	     ||!(f = fopen((fn = CONFIGDIR"/nsd_cookiesecrets.txt"), "r")))
-		return 1;
+		goto generate_cookie_secrets;
+
+	/* Only at startup cookie_secrets_source == COOKIE_SECRETS_NONE.
+	 * Only then the previous default file location will be tried
+	 * when the current default file location didn't exist.
+	 */
+	} else if(nsd->cookie_secrets_source == COOKIE_SECRETS_NONE
+	       && nsd->options->cookie_secret_file_is_default
+	       && (f = fopen((fn = CONFIGDIR"/nsd_cookiesecrets.txt"),"r")))
+		; /* pass */
+
+	else if(errno != ENOENT) {
+		log_msg( LOG_ERR
+		       , "error reading cookie secret file \"%s\": \"%s\""
+		       , fn, strerror(errno));
+		goto generate_cookie_secrets;
+	} else
+		goto generate_cookie_secrets;
 
 	/* cookie secret file exists and is readable */
 	for( count = 0; count < NSD_COOKIE_HISTORY_SIZE; count++ ) {
@@ -1216,7 +1255,7 @@ int cookie_secret_file_read(nsd_type* nsd) {
 			       , fn);
 			explicit_bzero(cookie_secrets, sizeof(cookie_secrets));
 			explicit_bzero(secret, sizeof(secret));
-			return 0;
+			goto generate_cookie_secrets;
 		}
 		/* needed for `hex_pton`; stripping potential `\n` */
 		secret[secret_len] = '\0';
@@ -1229,61 +1268,39 @@ int cookie_secret_file_read(nsd_type* nsd) {
 			       , fn);
 			explicit_bzero(cookie_secrets, sizeof(cookie_secrets));
 			explicit_bzero(secret, sizeof(secret));
-			return 0;
-		explicit_bzero(secret, sizeof(secret));
+			goto generate_cookie_secrets;
 		}
+		explicit_bzero(secret, sizeof(secret));
 	}
 	fclose(f);
-	if(count && nsd->cookie_secrets_source != COOKIE_SECRETS_FROM_FILE) {
+	if(count) {
 		nsd->cookie_count = count;
 		memcpy(nsd->cookie_secrets, cookie_secrets, sizeof(cookie_secrets));
-		nsd->cookie_secrets_source = COOKIE_SECRETS_FROM_FILE;
 		region_str_replace(  nsd->region
 		                  , &nsd->cookie_secrets_filename, fn );
+		explicit_bzero(cookie_secrets, sizeof(cookie_secrets));
+		/*************************************************************/
+		nsd->cookie_secrets_source = COOKIE_SECRETS_FROM_FILE;
+		return;
+		/*************************************************************/
 	}
 	explicit_bzero(cookie_secrets, sizeof(cookie_secrets));
-	return 1;
-}
-
-
-void reconfig_cookies(struct nsd* nsd, struct nsd_options* options)
-{
-	nsd->do_answer_cookie = options->answer_cookie;
-	if(options->cookie_secret) {
-		ssize_t len = hex_pton(options->cookie_secret,
-				nsd->cookie_secrets[0].cookie_secret,
-				NSD_COOKIE_SECRET_SIZE);
 
-		/* Cookie length guaranteed in configparser.y */
-		assert(len == NSD_COOKIE_SECRET_SIZE);
-		nsd->cookie_count = 1;
-		nsd->cookie_secrets_source = COOKIE_SECRETS_FROM_CONFIG;
-		if(options->cookie_staging_secret) {
-			len = hex_pton(options->cookie_staging_secret,
-					nsd->cookie_secrets[1].cookie_secret,
-					NSD_COOKIE_SECRET_SIZE);
-			/* Cookie length guaranteed in configparser.y */
-			assert(len == NSD_COOKIE_SECRET_SIZE);
-			nsd->cookie_count = 2;
-		}
-	} else {
-		size_t i, j;
-
-		/* Calculate a new random secret */
-		srandom(getpid() ^ time(NULL));
+generate_cookie_secrets:
+	/* Calculate a new random secret */
+	srandom(getpid() ^ time(NULL));
 
-		for( j = 0; j < NSD_COOKIE_HISTORY_SIZE; j++) {
+	for( j = 0; j < NSD_COOKIE_HISTORY_SIZE; j++) {
 #if defined(HAVE_SSL)
-			if (!RAND_status()
-			||  !RAND_bytes(nsd->cookie_secrets[j].cookie_secret, NSD_COOKIE_SECRET_SIZE))
+		if (!RAND_status()
+		||  !RAND_bytes(nsd->cookie_secrets[j].cookie_secret, NSD_COOKIE_SECRET_SIZE))
 #endif
-			for (i = 0; i < NSD_COOKIE_SECRET_SIZE; i++)
-				nsd->cookie_secrets[j].cookie_secret[i] = random_generate(256);
-		}
-		nsd->cookie_count = 1;
-		nsd->cookie_secrets_source = COOKIE_SECRETS_GENERATED;
-		if(cookie_secret_file(nsd->options))
-			cookie_secret_file_read(nsd);
+		for (i = 0; i < NSD_COOKIE_SECRET_SIZE; i++)
+			nsd->cookie_secrets[j].cookie_secret[i] = random_generate(256);
 	}
+	nsd->cookie_count = 1;
+	/*********************************************************************/
+	nsd->cookie_secrets_source = COOKIE_SECRETS_GENERATED;
+	/*********************************************************************/
 }