From: Evan Hunt Date: Thu, 20 Nov 2025 01:52:39 +0000 (-0800) Subject: fix allow-recursion/allow-query-cache inheritance X-Git-Tag: v9.21.16~39^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1a77ae2a7afaaa24d9c181dc2ca33c6853ec2246;p=thirdparty%2Fbind9.git 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. --- diff --git a/bin/named/server.c b/bin/named/server.c index 84411486068..20dbb74ed30 100644 --- a/bin/named/server.c +++ b/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)); + isc_g_mctx, &aq, &view->queryacl)); + CHECK(configure_view_acl(vconfig, config, "allow-query-on", NULL, + aclctx, isc_g_mctx, NULL, &view->queryonacl)); - /* named.conf only */ CHECK(configure_view_acl(vconfig, config, "allow-query-cache", NULL, - aclctx, isc_g_mctx, &view->cacheacl)); - /* named.conf only */ + aclctx, isc_g_mctx, &aqc, &view->cacheacl)); CHECK(configure_view_acl(vconfig, config, "allow-query-cache-on", NULL, - aclctx, isc_g_mctx, &view->cacheonacl)); - - CHECK(configure_view_acl(vconfig, config, "allow-query-on", NULL, - aclctx, isc_g_mctx, &view->queryonacl)); + 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)); + + /* + * "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); + } } - } - /* - * 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-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; diff --git a/lib/isccfg/include/isccfg/cfg.h b/lib/isccfg/include/isccfg/cfg.h index ec3076f3ce4..9e74dc4d7b8 100644 --- a/lib/isccfg/include/isccfg/cfg.h +++ b/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); diff --git a/lib/isccfg/include/isccfg/grammar.h b/lib/isccfg/include/isccfg/grammar.h index e0839e9e64e..659a953e2c1 100644 --- a/lib/isccfg/include/isccfg/grammar.h +++ b/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; diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c index e2d5796ed9d..b949a443040 100644 --- a/lib/isccfg/namedconf.c +++ b/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; } diff --git a/lib/isccfg/parser.c b/lib/isccfg/parser.c index 15c2c53e460..69938d7c4ac 100644 --- a/lib/isccfg/parser.c +++ b/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); } diff --git a/tests/isccfg/grammar_test.c b/tests/isccfg/grammar_test.c index 15610a6d11f..59ea8241125 100644 --- a/tests/isccfg/grammar_test.c +++ b/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);