fix allow-recursion/allow-query-cache inheritance

the merging of options and defaults into the effective configuration
broke the mutual inheritance of the allow-recursion, allow-query, and
allow-query-cache ACLs, and of the allow-recursion-on and
allow-query-cache-on ACLs.

this has been corrected by adding a 'cloned' flag to the cfg_obj
structure to indicate whether it was configured explicitly or
cloned from the defaults during parsing. we can then adjust the
ACLs while configuring a view, favoring user-configured values
when they're available over cloned defaults.

currently the adjustments to the ACLs are done in configure_view();
later they'll be moved into the effective configuration and this
special handling can be removed.

bin/named/server.c

@@ -541,7 +541,7 @@ load_nzf(dns_view_t *view);
 static isc_result_t
 configure_view_acl(const cfg_obj_t *vconfig, const cfg_obj_t *config,
 		   const char *aclname, const char *acltuplename,
-		   cfg_aclconfctx_t *aclctx, isc_mem_t *mctx,
+		   cfg_aclconfctx_t *aclctx, isc_mem_t *mctx, bool *expp,
 		   dns_acl_t **aclp) {
 	isc_result_t result;
 	const cfg_obj_t *maps[4];
@@ -580,6 +580,14 @@ configure_view_acl(const cfg_obj_t *vconfig, const cfg_obj_t *config,
 		aclobj = cfg_tuple_get(aclobj, acltuplename);
 	}
 
+	if (expp != NULL) {
+		/*
+		 * Note whether the ACL was explicitly configured,
+		 * not cloned during the merge process.
+		 */
+		*expp = !cfg_obj_iscloned(aclobj);
+	}
+
 	result = cfg_acl_fromconfig(aclobj, config, aclctx, mctx, 0, aclp);
 
 	return result;
@@ -4773,9 +4781,10 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	 * can be retrieved.)
 	 */
 	CHECK(configure_view_acl(vconfig, config, "match-clients", NULL, aclctx,
-				 isc_g_mctx, &view->matchclients));
+				 isc_g_mctx, NULL, &view->matchclients));
 	CHECK(configure_view_acl(vconfig, config, "match-destinations", NULL,
-				 aclctx, isc_g_mctx, &view->matchdestinations));
+				 aclctx, isc_g_mctx, NULL,
+				 &view->matchdestinations));
 
 	/*
 	 * Configure the "match-recursive-only" option.
@@ -4860,72 +4869,83 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	view->root_key_sentinel = cfg_obj_asboolean(obj);
 
 	/*
-	 * Set the "allow-query", "allow-query-cache", "allow-recursion",
-	 * "allow-recursion-on" and "allow-query-cache-on" ACLs if
-	 * configured in named.conf, but NOT from the global defaults.
-	 * This is done by leaving the third argument to configure_view_acl()
-	 * NULL.
-	 *
-	 * We ignore the global defaults here because these ACLs
-	 * can inherit from each other.  If any are still unset after
-	 * applying the inheritance rules, we'll look up the defaults at
-	 * that time.
+	 * The "allow-query", "allow-query-cache", "allow-recursion",
+	 * "allow-recursion-on" and "allow-query-cache-on" ACLs can
+	 * inherit from one other, so there's special handling based
+	 * on whether they're explicitly set in named.conf or cloned
+	 * from the defaults.
 	 */
-
-	/* named.conf only */
+	bool aq, aqc, ar, aro, aqco;
 	CHECK(configure_view_acl(vconfig, config, "allow-query", NULL, aclctx,
-				 isc_g_mctx, &view->queryacl));
-
-	/* named.conf only */
-	CHECK(configure_view_acl(vconfig, config, "allow-query-cache", NULL,
-				 aclctx, isc_g_mctx, &view->cacheacl));
-	/* named.conf only */
-	CHECK(configure_view_acl(vconfig, config, "allow-query-cache-on", NULL,
-				 aclctx, isc_g_mctx, &view->cacheonacl));
-
+				 isc_g_mctx, &aq, &view->queryacl));
 	CHECK(configure_view_acl(vconfig, config, "allow-query-on", NULL,
-				 aclctx, isc_g_mctx, &view->queryonacl));
+				 aclctx, isc_g_mctx, NULL, &view->queryonacl));
+
+	CHECK(configure_view_acl(vconfig, config, "allow-query-cache", NULL,
+				 aclctx, isc_g_mctx, &aqc, &view->cacheacl));
+	CHECK(configure_view_acl(vconfig, config, "allow-query-cache-on", NULL,
+				 aclctx, isc_g_mctx, &aqco, &view->cacheonacl));
 
 	CHECK(configure_view_acl(vconfig, config, "allow-proxy", NULL, aclctx,
-				 isc_g_mctx, &view->proxyacl));
+				 isc_g_mctx, NULL, &view->proxyacl));
 
 	CHECK(configure_view_acl(vconfig, config, "allow-proxy-on", NULL,
-				 aclctx, isc_g_mctx, &view->proxyonacl));
+				 aclctx, isc_g_mctx, NULL, &view->proxyonacl));
 
 	if (strcmp(view->name, "_bind") != 0 &&
 	    view->rdclass != dns_rdataclass_chaos)
 	{
-		/* named.conf only */
 		CHECK(configure_view_acl(vconfig, config, "allow-recursion",
-					 NULL, aclctx, isc_g_mctx,
+					 NULL, aclctx, isc_g_mctx, &ar,
 					 &view->recursionacl));
-		/* named.conf only */
 		CHECK(configure_view_acl(vconfig, config, "allow-recursion-on",
-					 NULL, aclctx, isc_g_mctx,
+					 NULL, aclctx, isc_g_mctx, &aro,
 					 &view->recursiononacl));
 	}
 
-	if (view->recursion == false) {
+	if (view->recursion) {
 		/*
-		 * We're not recursive; if the query-cache ACLs haven't
-		 * been set at the options/view level, set them to none.
+		 * "allow-query-cache" inherits from "allow-recursion" if set,
+		 * otherwise from "allow-query" if set.
 		 */
-		if (view->cacheacl == NULL) {
-			CHECK(dns_acl_none(mctx, &view->cacheacl));
+		if (!aqc) {
+			if (ar) {
+				dns_acl_detach(&view->cacheacl);
+				dns_acl_attach(view->recursionacl,
+					       &view->cacheacl);
+			} else if (aq) {
+				dns_acl_detach(&view->cacheacl);
+				dns_acl_attach(view->queryacl, &view->cacheacl);
+			}
 		}
-		if (view->cacheonacl == NULL) {
-			CHECK(dns_acl_none(mctx, &view->cacheonacl));
-		}
-	}
 
-	/*
-	 * Finished setting recursion and query-cache ACLs, so now we
-	 * can get the allow-query default if it wasn't set in named.conf
-	 */
-	if (view->queryacl == NULL) {
-		/* global default only */
-		CHECK(configure_view_acl(NULL, NULL, "allow-query", NULL,
-					 aclctx, isc_g_mctx, &view->queryacl));
+		/*
+		 * "allow-recursion" inherits from "allow-query-cache" if set,
+		 * otherwise from "allow-query" if set.
+		 */
+		if (!ar) {
+			if (aqc) {
+				dns_acl_detach(&view->recursionacl);
+				dns_acl_attach(view->cacheacl,
+					       &view->recursionacl);
+			} else if (aq) {
+				dns_acl_detach(&view->recursionacl);
+				dns_acl_attach(view->queryacl,
+					       &view->recursionacl);
+			}
+		}
+
+		/*
+		 * "allow-query-cache-on" inherits from "allow-recursion-on"
+		 * if set, and vice versa.
+		 */
+		if (!aqco && aro) {
+			dns_acl_detach(&view->cacheonacl);
+			dns_acl_attach(view->recursiononacl, &view->cacheonacl);
+		} else if (!aro && aqco) {
+			dns_acl_detach(&view->recursiononacl);
+			dns_acl_attach(view->cacheonacl, &view->recursiononacl);
+		}
 	}
 
 	/*
@@ -4934,7 +4954,8 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	 * and is needed by some broken clients.
 	 */
 	CHECK(configure_view_acl(vconfig, config, "no-case-compress", NULL,
-				 aclctx, isc_g_mctx, &view->nocasecompress));
+				 aclctx, isc_g_mctx, NULL,
+				 &view->nocasecompress));
 
 	/*
 	 * Disable name compression completely, this is a tradeoff
@@ -4949,7 +4970,7 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	 * Filter setting on addresses in the answer section.
 	 */
 	CHECK(configure_view_acl(vconfig, config, "deny-answer-addresses",
-				 "acl", aclctx, isc_g_mctx,
+				 "acl", aclctx, isc_g_mctx, NULL,
 				 &view->denyansweracl));
 	CHECK(configure_view_nametable(vconfig, config, "deny-answer-addresses",
 				       "except-from", isc_g_mctx,
@@ -4971,12 +4992,13 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	 */
 	if (view->transferacl == NULL) {
 		CHECK(configure_view_acl(vconfig, config, "allow-transfer",
-					 NULL, aclctx, isc_g_mctx,
+					 NULL, aclctx, isc_g_mctx, NULL,
 					 &view->transferacl));
 	}
 	if (view->notifyacl == NULL) {
 		CHECK(configure_view_acl(vconfig, config, "allow-notify", NULL,
-					 aclctx, isc_g_mctx, &view->notifyacl));
+					 aclctx, isc_g_mctx, NULL,
+					 &view->notifyacl));
 	}
 
 	obj = NULL;
@@ -8120,7 +8142,7 @@ apply_configuration(cfg_obj_t *effectiveconfig, cfg_obj_t *bindkeys,
 	 * no default.
 	 */
 	result = configure_view_acl(NULL, effectiveconfig, "blackhole", NULL,
-				    aclctx, isc_g_mctx,
+				    aclctx, isc_g_mctx, NULL,
 				    &server->sctx->blackholeacl);
 	if (result != ISC_R_SUCCESS) {
 		goto cleanup_tls;

lib/isccfg/include/isccfg/cfg.h

@@ -549,6 +549,12 @@ cfg_obj_istype(const cfg_obj_t *obj, const cfg_type_t *type);
  * Return true iff 'obj' is of type 'type'.
  */
 
+bool
+cfg_obj_iscloned(const cfg_obj_t *obj);
+/*%<
+ * Return true iff 'obj' has the 'CFG_OBJ_CLONED' flag.
+ */
+
 void
 cfg_obj_log(const cfg_obj_t *obj, int level, const char *fmt, ...)
 	ISC_FORMAT_PRINTF(3, 4);
@@ -578,6 +584,9 @@ const cfg_clausedef_t *
 cfg_map_nextclause(const cfg_type_t *map, const void **clauses,
 		   unsigned int *idx);
 
+const cfg_clausedef_t *
+cfg_map_findclause(const cfg_type_t *map, const char *name);
+
 typedef isc_result_t(pluginlist_cb_t)(
 	const cfg_obj_t *config, const cfg_obj_t *obj, cfg_aclconfctx_t *aclctx,
 	const char *plugin_path, const char *parameters, void *callback_data);

lib/isccfg/include/isccfg/grammar.h

@@ -201,6 +201,13 @@ struct cfg_obj {
 	isc_mem_t     *mctx;
 	isc_refcount_t references;
 
+	/*%
+	 * Indicates that an object was cloned from the defaults
+	 * or otherwise generated during the configuration merge
+	 * process:
+	 */
+	bool cloned;
+
 	const cfg_type_t *type;
 	union {
 		uint32_t	 uint32;

lib/isccfg/namedconf.c

@@ -1219,6 +1219,16 @@ merge_append(cfg_obj_t *effectiveobj, const cfg_obj_t *defaultobj) {
 	cfg_list_addclone(effectiveobj, defaultobj, false);
 }
 
+static void
+cloneto(cfg_obj_t *options, const cfg_obj_t *obj, const char *clausename) {
+	isc_result_t result;
+	const cfg_clausedef_t *clause = cfg_map_findclause(options->type,
+							   clausename);
+
+	result = cfg_map_addclone(options, obj, clause);
+	INSIST(result == ISC_R_SUCCESS);
+}
+
 static void
 options_merge_defaultacl(cfg_obj_t *effectiveoptions,
 			 const cfg_obj_t *defaultoptions, const char *aclname,
@@ -1233,9 +1243,7 @@ options_merge_defaultacl(cfg_obj_t *effectiveoptions,
 	result = cfg_map_get(defaultoptions, aclname, &obj);
 	INSIST(result == ISC_R_SUCCESS);
 
-	cfg_obj_ref(UNCONST(obj));
-	result = cfg_map_add(effectiveoptions, UNCONST(obj), aclname);
-	INSIST(result == ISC_R_SUCCESS);
+	cloneto(effectiveoptions, obj, aclname);
 }
 
 static void
@@ -1265,19 +1273,13 @@ options_merge(cfg_obj_t *effectiveoptions, const cfg_obj_t *defaultoptions) {
 	if (result != ISC_R_SUCCESS) {
 		result = cfg_map_get(effectiveoptions, "allow-recursion", &obj);
 		if (result == ISC_R_SUCCESS) {
-			cfg_obj_ref(UNCONST(obj));
-			result = cfg_map_add(effectiveoptions, UNCONST(obj),
-					     "allow-query-cache");
-			INSIST(result == ISC_R_SUCCESS);
+			cloneto(effectiveoptions, obj, "allow-query-cache");
 		} else {
 			result = cfg_map_get(effectiveoptions, "allow-query",
 					     &obj);
 			if (result == ISC_R_SUCCESS) {
-				cfg_obj_ref(UNCONST(obj));
-				result = cfg_map_add(effectiveoptions,
-						     UNCONST(obj),
-						     "allow-query-cache");
-				INSIST(result == ISC_R_SUCCESS);
+				cloneto(effectiveoptions, obj,
+					"allow-query-cache");
 			} else {
 				noquerycacheacl = true;
 			}
@@ -1290,19 +1292,13 @@ options_merge(cfg_obj_t *effectiveoptions, const cfg_obj_t *defaultoptions) {
 		result = cfg_map_get(effectiveoptions, "allow-query-cache",
 				     &obj);
 		if (result == ISC_R_SUCCESS) {
-			cfg_obj_ref(UNCONST(obj));
-			result = cfg_map_add(effectiveoptions, UNCONST(obj),
-					     "allow-recursion");
-			INSIST(result == ISC_R_SUCCESS);
+			cloneto(effectiveoptions, obj, "allow-recursion");
 		} else {
 			result = cfg_map_get(effectiveoptions, "allow-query",
 					     &obj);
 			if (result == ISC_R_SUCCESS) {
-				cfg_obj_ref(UNCONST(obj));
-				result = cfg_map_add(effectiveoptions,
-						     UNCONST(obj),
-						     "allow-recursion");
-				INSIST(result == ISC_R_SUCCESS);
+				cloneto(effectiveoptions, obj,
+					"allow-recursion");
 			} else {
 				norecursionacl = true;
 			}
@@ -1315,10 +1311,7 @@ options_merge(cfg_obj_t *effectiveoptions, const cfg_obj_t *defaultoptions) {
 		result = cfg_map_get(effectiveoptions, "allow-recursion-on",
 				     &obj);
 		if (result == ISC_R_SUCCESS) {
-			cfg_obj_ref(UNCONST(obj));
-			result = cfg_map_add(effectiveoptions, UNCONST(obj),
-					     "allow-query-cache-on");
-			INSIST(result == ISC_R_SUCCESS);
+			cloneto(effectiveoptions, obj, "allow-query-cache-on");
 		} else {
 			noquerycacheonacl = true;
 		}
@@ -1330,10 +1323,7 @@ options_merge(cfg_obj_t *effectiveoptions, const cfg_obj_t *defaultoptions) {
 		result = cfg_map_get(effectiveoptions, "allow-query-cache-on",
 				     &obj);
 		if (result == ISC_R_SUCCESS) {
-			cfg_obj_ref(UNCONST(obj));
-			result = cfg_map_add(effectiveoptions, UNCONST(obj),
-					     "allow-recursion-on");
-			INSIST(result == ISC_R_SUCCESS);
+			cloneto(effectiveoptions, obj, "allow-recursion-on");
 		} else {
 			norecursiononacl = true;
 		}

lib/isccfg/parser.c

@@ -175,6 +175,7 @@ cfg_obj_clone(const cfg_obj_t *source, cfg_obj_t **target) {
 
 	cfg_obj_create(source->mctx, source->file, source->line, source->type,
 		       target);
+	(*target)->cloned = source->cloned;
 	source->type->rep->copy(*target, source);
 }
 
@@ -2989,6 +2990,23 @@ cfg_map_nextclause(const cfg_type_t *map, const void **clauses,
 	return &(*clauseset)[*idx];
 }
 
+const cfg_clausedef_t *
+cfg_map_findclause(const cfg_type_t *map, const char *name) {
+	const cfg_clausedef_t *found = NULL;
+	const void *clauses = NULL;
+	unsigned int idx;
+
+	REQUIRE(map != NULL && map->rep == &cfg_rep_map);
+	REQUIRE(name != NULL);
+
+	found = cfg_map_firstclause(map, &clauses, &idx);
+	while (name != NULL && strcasecmp(name, found->name)) {
+		found = cfg_map_nextclause(map, &clauses, &idx);
+	}
+
+	return ((cfg_clausedef_t *)clauses) + idx;
+}
+
 /* Parse an arbitrary token, storing its raw text representation. */
 static isc_result_t
 parse_token(cfg_parser_t *pctx, const cfg_type_t *type ISC_ATTR_UNUSED,
@@ -3986,6 +4004,12 @@ cfg_obj_istype(const cfg_obj_t *obj, const cfg_type_t *type) {
 	return obj->type == type;
 }
 
+bool
+cfg_obj_iscloned(const cfg_obj_t *obj) {
+	REQUIRE(VALID_CFGOBJ(obj));
+	return obj->cloned;
+}
+
 /*
  * Destroy 'obj'.
  */
@@ -4037,24 +4061,6 @@ cfg_print_grammar(const cfg_type_t *type, unsigned int flags,
 	cfg_doc_obj(&pctx, type);
 }
 
-static const cfg_clausedef_t *
-map_lookup_clause(const cfg_obj_t *mapobj, const char *clausename) {
-	const cfg_map_t *map;
-	const cfg_clausedef_t *const *clauseset = NULL;
-	const cfg_clausedef_t *clause = NULL;
-
-	map = &mapobj->value.map;
-	for (clauseset = map->clausesets; *clauseset != NULL; clauseset++) {
-		for (clause = *clauseset; clause->name != NULL; clause++) {
-			if (strcasecmp(clause->name, clausename) == 0) {
-				return clause;
-			}
-		}
-	}
-
-	return NULL;
-}
-
 static isc_result_t
 map_define(cfg_obj_t *mapobj, cfg_obj_t *obj, const cfg_clausedef_t *clause) {
 	isc_result_t result;
@@ -4110,7 +4116,7 @@ cfg_map_add(cfg_obj_t *mapobj, cfg_obj_t *obj, const char *clausename) {
 	REQUIRE(mapobj->type->rep == &cfg_rep_map);
 	REQUIRE(clausename != NULL);
 
-	clause = map_lookup_clause(mapobj, clausename);
+	clause = cfg_map_findclause(mapobj->type, clausename);
 	if (clause == NULL || clause->name == NULL) {
 		return ISC_R_FAILURE;
 	}
@@ -4142,6 +4148,8 @@ cfg_map_addclone(cfg_obj_t *map, const cfg_obj_t *obj,
 		elt = cfg_list_first(obj);
 		while (elt != NULL && result == ISC_R_SUCCESS) {
 			cfg_obj_clone(elt->obj, &clone);
+			clone->cloned = true;
+
 			result = map_define(map, clone, clause);
 			elt = cfg_list_next(elt);
 
@@ -4153,6 +4161,7 @@ cfg_map_addclone(cfg_obj_t *map, const cfg_obj_t *obj,
 		}
 	} else {
 		cfg_obj_clone(obj, &clone);
+		clone->cloned = true;
 		result = map_define(map, clone, clause);
 	}
 

tests/isccfg/grammar_test.c

@@ -72,29 +72,15 @@ assert_text(const char *text) {
 	memset(gtext, 0, sizeof(gtext));
 }
 
-static const cfg_clausedef_t *
-find_clause(const cfg_type_t *map, const char *name) {
-	const cfg_clausedef_t *found = NULL;
-	const void *clauses = NULL;
-	unsigned int idx;
-
-	found = cfg_map_firstclause(map, &clauses, &idx);
-	while (name != NULL && strcasecmp(name, found->name)) {
-		found = cfg_map_nextclause(map, &clauses, &idx);
-	}
-
-	return ((cfg_clausedef_t *)clauses) + idx;
-}
-
 static void
 test__querysource(const char *clause_name, const char *name,
 		  const char *expected) {
 	const cfg_clausedef_t *options_clause = NULL;
-	options_clause = find_clause(&cfg_type_namedconf, clause_name);
+	options_clause = cfg_map_findclause(&cfg_type_namedconf, clause_name);
 	assert_non_null(options_clause);
 
 	const cfg_clausedef_t *querysource_clause = NULL;
-	querysource_clause = find_clause(options_clause->type, name);
+	querysource_clause = cfg_map_findclause(options_clause->type, name);
 	assert_non_null(querysource_clause);
 	querysource_clause->type->doc(&gprinter, querysource_clause->type);
 	assert_text(expected);