Merge branch '2463-kasp-views-different-policy-same-keydirectory' into 'main'

Check key-directory duplicates for kasp zones

Closes #2463

See merge request isc-projects/bind9!4989

CHANGES

@@ -1,3 +1,7 @@
+5642.	[bug]		Check "key-directory" conflicts in "named.conf" for
+			zones in multiple views with different "dnssec-policy".
+			[GL #2463].
+
 5641.	[bug]		Address potential memory leak in dst_key_fromnamedfile.
 			[GL #2689]
 

bin/tests/system/checkconf/bad-kasp-keydir1.conf

@@ -0,0 +1,42 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+key "keyforview1" {
+	algorithm "hmac-sha1";
+	secret "YPfMoAk6h+3iN8MDRQC004iSNHY=";
+};
+
+key "keyforview2" {
+	algorithm "hmac-sha1";
+	secret "4xILSZQnuO1UKubXHkYUsvBRPu8=";
+};
+
+view "example1" {
+	match-clients { key "keyforview1"; };
+
+	zone "example.net" {
+		type primary;
+		dnssec-policy "default";
+		key-directory ".";
+		file "example1.db";
+	};
+};
+
+view "example2" {
+	match-clients { key "keyforview2"; };
+
+	zone "example.net" {
+		type primary;
+		dnssec-policy "insecure";
+		key-directory ".";
+		file "example2.db";
+	};
+};

bin/tests/system/checkconf/bad-kasp-keydir2.conf

@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+key "keyforview1" {
+	algorithm "hmac-sha1";
+	secret "YPfMoAk6h+3iN8MDRQC004iSNHY=";
+};
+
+key "keyforview2" {
+	algorithm "hmac-sha1";
+	secret "4xILSZQnuO1UKubXHkYUsvBRPu8=";
+};
+
+view "example1" {
+	match-clients { key "keyforview1"; };
+
+	zone "example.net" {
+		type primary;
+		dnssec-policy "default";
+		file "example1.db";
+	};
+};
+
+view "example2" {
+	match-clients { key "keyforview2"; };
+
+	zone "example.net" {
+		type primary;
+		dnssec-policy "insecure";
+		file "example2.db";
+	};
+};

bin/tests/system/checkconf/good.conf

@@ -192,7 +192,53 @@ view "fourth" {
 		file "dnssec-none.db";
 		dnssec-policy "none";
 	};
+	zone "dnssec-view1" {
+		type master;
+		file "dnssec-view41.db";
+		dnssec-policy "test";
+	};
+	zone "dnssec-view2" {
+		type master;
+		file "dnssec-view42.db";
+	};
+	zone "dnssec-view3" {
+		type master;
+		file "dnssec-view43.db";
+		dnssec-policy "none";
+		key-directory "keys";
+	};
+	zone "dnssec-view4" {
+		type master;
+		file "dnssec-view44.db";
+		dnssec-policy "none";
+	};
 	dnssec-policy "default";
+	key-directory ".";
+};
+view "fifth" {
+	zone "dnssec-view1" {
+		type master;
+		file "dnssec-view51.db";
+		dnssec-policy "test";
+	};
+	zone "dnssec-view2" {
+		type master;
+		file "dnssec-view52.db";
+		dnssec-policy "test";
+		key-directory "keys";
+	};
+	zone "dnssec-view3" {
+		type master;
+		file "dnssec-view53.db";
+		dnssec-policy "default";
+		key-directory "keys";
+	};
+	zone "dnssec-view4" {
+		type master;
+		file "dnssec-view54.db";
+		dnssec-policy "none";
+	};
+	key-directory ".";
 };
 view "chaos" chaos {
 	zone "hostname.bind" chaos {

bin/tests/system/checkconf/good.zonelist

@@ -13,4 +13,12 @@ dnssec-test IN fourth master
 dnssec-default IN fourth master
 dnssec-inherit IN fourth master
 dnssec-none IN fourth master
+dnssec-view1 IN fourth master
+dnssec-view2 IN fourth master
+dnssec-view3 IN fourth master
+dnssec-view4 IN fourth master
+dnssec-view1 IN fifth master
+dnssec-view2 IN fifth master
+dnssec-view3 IN fifth master
+dnssec-view4 IN fifth master
 hostname.bind chaos chaos master

bin/tests/system/checkconf/tests.sh

@@ -12,6 +12,8 @@
 status=0
 n=0
 
+mkdir keys
+
 n=`expr $n + 1`
 echo_i "checking that named-checkconf handles a known good config ($n)"
 ret=0
@@ -549,5 +551,7 @@ grep "exceeds 100%" < checkconf.out$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; ret=1; fi
 status=`expr $status + $ret`
 
+rmdir keys
+
 echo_i "exit status: $status"
 [ $status -eq 0 ] || exit 1

doc/notes/notes-current.rst

@@ -98,3 +98,7 @@ Bug Fixes
 
 - ``named-checkconf`` now complains if zones with ``dnssec-policy`` reference
   the same zone file more than once. :gl:`#2603`
+
+- Check ``key-directory`` conflicts in ``named.conf`` for zones in multiple
+  views with different ``dnssec-policy``. Using the same ``key-directory`` for
+  such zones is not allowed. :gl:`#2463`

lib/bind9/check.c

@@ -72,6 +72,9 @@ static isc_result_t
 fileexist(const cfg_obj_t *obj, isc_symtab_t *symtab, bool writeable,
 	  isc_log_t *logctxlogc);
 
+static isc_result_t
+keydirexist(const cfg_obj_t *zcgf, const char *dir, const char *kaspnamestr,
+	    isc_symtab_t *symtab, isc_log_t *logctx, isc_mem_t *mctx);
 static void
 freekey(char *key, unsigned int type, isc_symvalue_t value, void *userarg) {
 	UNUSED(type);
@@ -2377,9 +2380,9 @@ cleanup:
 static isc_result_t
 check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions,
 	       const cfg_obj_t *config, isc_symtab_t *symtab,
-	       isc_symtab_t *files, isc_symtab_t *inview, const char *viewname,
-	       dns_rdataclass_t defclass, cfg_aclconfctx_t *actx,
-	       isc_log_t *logctx, isc_mem_t *mctx) {
+	       isc_symtab_t *files, isc_symtab_t *keydirs, isc_symtab_t *inview,
+	       const char *viewname, dns_rdataclass_t defclass,
+	       cfg_aclconfctx_t *actx, isc_log_t *logctx, isc_mem_t *mctx) {
 	const char *znamestr;
 	const char *typestr = NULL;
 	const char *target = NULL;
@@ -2404,6 +2407,8 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions,
 	bool has_dnssecpolicy = false;
 	const void *clauses = NULL;
 	const char *option = NULL;
+	const char *kaspname = NULL;
+	const char *dir = NULL;
 	static const char *acls[] = {
 		"allow-notify",
 		"allow-transfer",
@@ -2633,8 +2638,8 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions,
 	(void)cfg_map_get(zoptions, "dnssec-policy", &obj);
 	if (obj != NULL) {
 		const cfg_obj_t *kasps = NULL;
-		const char *kaspname = cfg_obj_asstring(obj);
 
+		kaspname = cfg_obj_asstring(obj);
 		if (strcmp(kaspname, "default") == 0) {
 			has_dnssecpolicy = true;
 		} else if (strcmp(kaspname, "insecure") == 0) {
@@ -3188,7 +3193,8 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions,
 	obj = NULL;
 	tresult = cfg_map_get(zoptions, "key-directory", &obj);
 	if (tresult == ISC_R_SUCCESS) {
-		const char *dir = cfg_obj_asstring(obj);
+		dir = cfg_obj_asstring(obj);
+
 		tresult = isc_file_isdirectory(dir);
 		switch (tresult) {
 		case ISC_R_SUCCESS:
@@ -3210,6 +3216,25 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions,
 		}
 	}
 
+	/*
+	 * Make sure there is no other zone with the same
+	 * key-directory and a different dnssec-policy.
+	 */
+	if (zname != NULL) {
+		char keydirbuf[DNS_NAME_FORMATSIZE + 128];
+		char *tmp = keydirbuf;
+		size_t len = sizeof(keydirbuf);
+		dns_name_format(zname, keydirbuf, sizeof(keydirbuf));
+		tmp += strlen(tmp);
+		len -= strlen(tmp);
+		(void)snprintf(tmp, len, "/%s", (dir == NULL) ? "(null)" : dir);
+		tresult = keydirexist(zconfig, (const char *)keydirbuf,
+				      kaspname, keydirs, logctx, mctx);
+		if (tresult != ISC_R_SUCCESS) {
+			result = tresult;
+		}
+	}
+
 	/*
 	 * Check various options.
 	 */
@@ -3420,6 +3445,56 @@ fileexist(const cfg_obj_t *obj, isc_symtab_t *symtab, bool writeable,
 	return (result);
 }
 
+static isc_result_t
+keydirexist(const cfg_obj_t *zcfg, const char *keydir, const char *kaspnamestr,
+	    isc_symtab_t *symtab, isc_log_t *logctx, isc_mem_t *mctx) {
+	isc_result_t result;
+	isc_symvalue_t symvalue;
+	char *symkey;
+
+	if (kaspnamestr == NULL || strcmp(kaspnamestr, "none") == 0) {
+		return (ISC_R_SUCCESS);
+	}
+
+	result = isc_symtab_lookup(symtab, keydir, 0, &symvalue);
+	if (result == ISC_R_SUCCESS) {
+		const cfg_obj_t *kasp = NULL;
+		const cfg_obj_t *exist = symvalue.as_cpointer;
+		const char *file = cfg_obj_file(exist);
+		unsigned int line = cfg_obj_line(exist);
+
+		/*
+		 * Having the same key-directory for the same zone is fine
+		 * iff the zone is using the same policy, or has no policy.
+		 */
+		(void)cfg_map_get(cfg_tuple_get(exist, "options"),
+				  "dnssec-policy", &kasp);
+		if (kasp == NULL ||
+		    strcmp(cfg_obj_asstring(kasp), "none") == 0 ||
+		    strcmp(cfg_obj_asstring(kasp), kaspnamestr) == 0)
+		{
+			return (ISC_R_SUCCESS);
+		}
+
+		cfg_obj_log(zcfg, logctx, ISC_LOG_ERROR,
+			    "key-directory '%s' already in use by zone %s with "
+			    "policy %s: %s:%u",
+			    keydir,
+			    cfg_obj_asstring(cfg_tuple_get(exist, "name")),
+			    cfg_obj_asstring(kasp), file, line);
+		return (ISC_R_EXISTS);
+	}
+
+	/*
+	 * Add the new zone plus key-directory.
+	 */
+	symkey = isc_mem_strdup(mctx, keydir);
+	symvalue.as_cpointer = zcfg;
+	result = isc_symtab_define(symtab, symkey, 2, symvalue,
+				   isc_symexists_reject);
+	return (result);
+}
+
 /*
  * Check key list for duplicates key names and that the key names
  * are valid domain names as these keys are used for TSIG.
@@ -4379,8 +4454,8 @@ check_dnstap(const cfg_obj_t *voptions, const cfg_obj_t *config,
 static isc_result_t
 check_viewconf(const cfg_obj_t *config, const cfg_obj_t *voptions,
 	       const char *viewname, dns_rdataclass_t vclass,
-	       isc_symtab_t *files, bool check_plugins, isc_symtab_t *inview,
-	       isc_log_t *logctx, isc_mem_t *mctx) {
+	       isc_symtab_t *files, isc_symtab_t *keydirs, bool check_plugins,
+	       isc_symtab_t *inview, isc_log_t *logctx, isc_mem_t *mctx) {
 	const cfg_obj_t *zones = NULL;
 	const cfg_obj_t *view_tkeys = NULL, *global_tkeys = NULL;
 	const cfg_obj_t *view_mkeys = NULL, *global_mkeys = NULL;
@@ -4437,8 +4512,8 @@ check_viewconf(const cfg_obj_t *config, const cfg_obj_t *voptions,
 		const cfg_obj_t *zone = cfg_listelt_value(element);
 
 		tresult = check_zoneconf(zone, voptions, config, symtab, files,
-					 inview, viewname, vclass, actx, logctx,
-					 mctx);
+					 keydirs, inview, viewname, vclass,
+					 actx, logctx, mctx);
 		if (tresult != ISC_R_SUCCESS) {
 			result = ISC_R_FAILURE;
 		}
@@ -5035,6 +5110,7 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins,
 	isc_result_t tresult;
 	isc_symtab_t *symtab = NULL;
 	isc_symtab_t *files = NULL;
+	isc_symtab_t *keydirs = NULL;
 	isc_symtab_t *inview = NULL;
 
 	static const char *builtin[] = { "localhost", "localnets", "any",
@@ -5086,6 +5162,12 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins,
 		goto cleanup;
 	}
 
+	tresult = isc_symtab_create(mctx, 100, freekey, mctx, false, &keydirs);
+	if (tresult != ISC_R_SUCCESS) {
+		result = tresult;
+		goto cleanup;
+	}
+
 	tresult = isc_symtab_create(mctx, 100, freekey, mctx, true, &inview);
 	if (tresult != ISC_R_SUCCESS) {
 		result = tresult;
@@ -5094,8 +5176,8 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins,
 
 	if (views == NULL) {
 		tresult = check_viewconf(config, NULL, NULL, dns_rdataclass_in,
-					 files, check_plugins, inview, logctx,
-					 mctx);
+					 files, keydirs, check_plugins, inview,
+					 logctx, mctx);
 		if (result == ISC_R_SUCCESS && tresult != ISC_R_SUCCESS) {
 			result = ISC_R_FAILURE;
 		}
@@ -5186,8 +5268,8 @@ bind9_check_namedconf(const cfg_obj_t *config, bool check_plugins,
 		}
 		if (tresult == ISC_R_SUCCESS) {
 			tresult = check_viewconf(config, voptions, key, vclass,
-						 files, check_plugins, inview,
-						 logctx, mctx);
+						 files, keydirs, check_plugins,
+						 inview, logctx, mctx);
 		}
 		if (tresult != ISC_R_SUCCESS) {
 			result = ISC_R_FAILURE;
@@ -5306,6 +5388,9 @@ cleanup:
 	if (files != NULL) {
 		isc_symtab_destroy(&files);
 	}
+	if (keydirs != NULL) {
+		isc_symtab_destroy(&keydirs);
+	}
 
 	return (result);
 }