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 8441148606851a026e556cd4673dd01a463c4e64..20dbb74ed30de25a711fe3d6900925f3ee6d3eae 100644 (file)
--- 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 ec3076f3ce490464c4a55de1607e0833bd4e4d2b..9e74dc4d7b874928715b413ec73aa8c91adcf6eb 100644 (file)
--- 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 e0839e9e64eb34aaa0e189d116ae994572735ee1..659a953e2c15ddb52eb8bb7ce2d21e83ab3d293d 100644 (file)
--- 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 e2d5796ed9dd6c286713dcba44c7ef2e3cce8af5..b949a4430408cb7039f816a74731d724160ed346 100644 (file)
--- 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 15c2c53e460a4a4d7cdab6ce42777667f020c627..69938d7c4accf7424db6fad6a8c25f713ba6f34c 100644 (file)
--- 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 15610a6d11fc9aecaa2bed8a564dc9af36a8ecd5..59ea8241125135bcd4d8d12eadd17ad8763fffe5 100644 (file)
--- 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);