From b6c77202cba8570677925026365b4f332c093603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 8 Aug 2018 07:56:29 +0200 Subject: [PATCH 1/4] Restore zone database and zone node if cache search results are to be ignored When query processing hits a delegation from a locally configured zone, an attempt may be made to look for a better answer in the cache. In such a case, the zone-sourced delegation data is set aside and the lookup is retried using the cache database. When that lookup is completed, a decision is made whether the answer found in the cache is better than the answer found in the zone. Currently, if the zone-sourced answer turns out to be better than the one found in the cache: - qctx->zdb is not restored into qctx->db, - qctx->node, holding the zone database node found, is not even saved. Thus, in such a case both qctx->db and qctx->node will point at cache data. This is not an issue for BIND versions which do not support mirror zones because in these versions non-recursive queries always cause the zone-sourced delegation to be returned and thus the non-recursive part of query_delegation() is never reached if the delegation is coming from a zone. With mirror zones, however, non-recursive queries may cause cache lookups even after a zone delegation is found. Leaving qctx->db assigned to the cache database when query_delegation() determines that the zone-sourced delegation is the best answer to the client's query prevents DS records from being added to delegations coming from mirror zones. Fix this issue by keeping the zone database and zone node in qctx while the cache is searched for an answer and then restoring them into qctx->db and qctx->node, respectively, if the zone-sourced delegation turns out to be the best answer. Since this change means that qctx->zdb cannot be used as the glue database any more as it will be reset to NULL by RESTORE(), ensure that qctx->db is not a cache database before attaching it to qctx->client->query.gluedb. Furthermore, current code contains a conditional statement which prevents a mirror zone from being used as a source of glue records. Said statement was added to prevent assertion failures caused by attempting to use a zone database's glue cache for finding glue for an NS RRset coming from a cache database. However, that check is overly strict since it completely prevents glue from being added to delegations coming from mirror zones. With the changes described above in place, the scenario this check was preventing can no longer happen, so remove the aforementioned check. If qctx->zdb is not NULL, qctx->zfname will also not be NULL; qctx->zsigrdataset may be NULL in such a case, but query_putrdataset() handles pointers to NULL pointers gracefully. Remove redundant conditional expressions to make the cleanup code in query_freedata() match the corresponding sequences of SAVE() / RESTORE() macros more closely. --- bin/tests/system/mirror/ns2/named.conf.in | 2 +- bin/tests/system/mirror/ns2/sign.sh | 4 ++-- bin/tests/system/mirror/tests.sh | 21 ++++++++++++++++-- lib/ns/include/ns/query.h | 5 +++-- lib/ns/query.c | 26 ++++++++++------------- 5 files changed, 36 insertions(+), 22 deletions(-) diff --git a/bin/tests/system/mirror/ns2/named.conf.in b/bin/tests/system/mirror/ns2/named.conf.in index 7141ec7e00..ebcce5729a 100644 --- a/bin/tests/system/mirror/ns2/named.conf.in +++ b/bin/tests/system/mirror/ns2/named.conf.in @@ -36,7 +36,7 @@ zone "example" { zone "sub.example" { type master; - file "sub.example.db.in"; + file "sub.example.db.signed"; }; zone "initially-unavailable" { diff --git a/bin/tests/system/mirror/ns2/sign.sh b/bin/tests/system/mirror/ns2/sign.sh index 827a11c897..e8239b14e0 100644 --- a/bin/tests/system/mirror/ns2/sign.sh +++ b/bin/tests/system/mirror/ns2/sign.sh @@ -14,7 +14,7 @@ SYSTEMTESTTOP=../.. keys_to_trust="" -for zonename in example initially-unavailable; do +for zonename in sub.example example initially-unavailable; do zone=$zonename infile=$zonename.db.in zonefile=$zonename.db @@ -24,7 +24,7 @@ for zonename in example initially-unavailable; do cat $infile $keyname1.key $keyname2.key > $zonefile - $SIGNER -P -o $zone $zonefile > /dev/null + $SIGNER -P -g -o $zone $zonefile > /dev/null done # Only add the key for "initially-unavailable" to the list of keys trusted by diff --git a/bin/tests/system/mirror/tests.sh b/bin/tests/system/mirror/tests.sh index 6b6ab1b406..73b01f7ae4 100644 --- a/bin/tests/system/mirror/tests.sh +++ b/bin/tests/system/mirror/tests.sh @@ -204,6 +204,21 @@ grep "${UPDATED_SERIAL_GOOD}.*; serial" dig.out.ns3.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` +n=`expr $n + 1` +echo_i "checking delegations sourced from a mirror zone ($n)" +ret=0 +$DIG $DIGOPTS @10.53.0.3 foo.example A +norec > dig.out.ns3.test$n 2>&1 || ret=1 +# Check response code and flags in the answer. +grep "NOERROR" dig.out.ns3.test$n > /dev/null || ret=1 +grep "flags:.* ad" dig.out.ns3.test$n > /dev/null && ret=1 +# Check that a delegation containing a DS RRset and glue is present. +grep "ANSWER: 0" dig.out.ns3.test$n > /dev/null || ret=1 +grep "example.*IN.*NS" dig.out.ns3.test$n > /dev/null || ret=1 +grep "example.*IN.*DS" dig.out.ns3.test$n > /dev/null || ret=1 +grep "ns2.example.*A.*10.53.0.2" dig.out.ns3.test$n > /dev/null || ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi +status=`expr $status + $ret` + n=`expr $n + 1` echo_i "checking that resolution involving a mirror zone works as expected ($n)" ret=0 @@ -238,14 +253,16 @@ ret=0 $DIG $DIGOPTS @10.53.0.3 sub.example. NS > dig.out.ns3.test$n.1 2>&1 || ret=1 # Ensure the child-side NS RRset is returned. grep "NOERROR" dig.out.ns3.test$n.1 > /dev/null || ret=1 -grep "ANSWER: 1" dig.out.ns3.test$n.1 > /dev/null || ret=1 +grep "ANSWER: 2" dig.out.ns3.test$n.1 > /dev/null || ret=1 grep "sub.example.*IN.*NS" dig.out.ns3.test$n.1 > /dev/null || ret=1 # Issue a non-recursive query for something below the cached zone cut. $DIG $DIGOPTS @10.53.0.3 +norec foo.sub.example. A > dig.out.ns3.test$n.2 2>&1 || ret=1 -# Ensure the cached NS RRset is returned in a delegation. +# Ensure the cached NS RRset is returned in a delegation, along with the +# parent-side DS RRset. grep "NOERROR" dig.out.ns3.test$n.2 > /dev/null || ret=1 grep "ANSWER: 0" dig.out.ns3.test$n.2 > /dev/null || ret=1 grep "sub.example.*IN.*NS" dig.out.ns3.test$n.2 > /dev/null || ret=1 +grep "sub.example.*IN.*DS" dig.out.ns3.test$n.2 > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index c9d0cfb43c..e6ca2cf4ae 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -153,8 +153,9 @@ typedef struct query_ctx { dns_dbnode_t *node; /* DB node */ dns_db_t *zdb; /* zone DB values, saved */ - dns_name_t *zfname; /* while searching cache */ - dns_dbversion_t *zversion; /* for a better answer */ + dns_dbnode_t *znode; /* while searching cache */ + dns_name_t *zfname; /* for a better answer */ + dns_dbversion_t *zversion; dns_rdataset_t *zrdataset; dns_rdataset_t *zsigrdataset; diff --git a/lib/ns/query.c b/lib/ns/query.c index e86608ff74..58ffe40c1e 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -5012,6 +5012,7 @@ qctx_init(ns_client_t *client, dns_fetchevent_t *event, qctx->zsigrdataset = NULL; qctx->zversion = NULL; qctx->node = NULL; + qctx->znode = NULL; qctx->db = NULL; qctx->zdb = NULL; qctx->version = NULL; @@ -5077,11 +5078,10 @@ qctx_freedata(query_ctx_t *qctx) { } if (qctx->zdb != NULL) { + query_putrdataset(qctx->client, &qctx->zsigrdataset); query_putrdataset(qctx->client, &qctx->zrdataset); - if (qctx->zsigrdataset != NULL) - query_putrdataset(qctx->client, &qctx->zsigrdataset); - if (qctx->zfname != NULL) - query_releasename(qctx->client, &qctx->zfname); + query_releasename(qctx->client, &qctx->zfname); + dns_db_detachnode(qctx->zdb, &qctx->znode); dns_db_detach(&qctx->zdb); } @@ -7803,8 +7803,8 @@ query_zone_delegation(query_ctx_t *qctx) { * we'll restore these values there. */ query_keepname(qctx->client, qctx->fname, qctx->dbuf); - dns_db_detachnode(qctx->db, &qctx->node); SAVE(qctx->zdb, qctx->db); + SAVE(qctx->znode, qctx->node); SAVE(qctx->zfname, qctx->fname); SAVE(qctx->zversion, qctx->version); SAVE(qctx->zrdataset, qctx->rdataset); @@ -7923,16 +7923,14 @@ query_delegation(query_ctx_t *qctx) { &qctx->sigrdataset); qctx->version = NULL; + dns_db_detachnode(qctx->db, &qctx->node); + dns_db_detach(&qctx->db); + RESTORE(qctx->db, qctx->zdb); + RESTORE(qctx->node, qctx->znode); RESTORE(qctx->fname, qctx->zfname); RESTORE(qctx->version, qctx->zversion); RESTORE(qctx->rdataset, qctx->zrdataset); RESTORE(qctx->sigrdataset, qctx->zsigrdataset); - - /* - * We don't clean up zdb here because we - * may still need it. It will get cleaned - * up by the main cleanup code in query_done(). - */ } if (RECURSIONOK(qctx->client)) { @@ -8010,10 +8008,8 @@ query_delegation(query_ctx_t *qctx) { qctx->client->query.attributes |= NS_QUERYATTR_CACHEGLUEOK; qctx->client->query.isreferral = ISC_TRUE; - if (qctx->zdb != NULL && qctx->client->query.gluedb == NULL && - !(qctx->zone != NULL && dns_zone_ismirror(qctx->zone))) - { - dns_db_attach(qctx->zdb, &qctx->client->query.gluedb); + if (!dns_db_iscache(qctx->db) && qctx->client->query.gluedb == NULL) { + dns_db_attach(qctx->db, &qctx->client->query.gluedb); detach = ISC_TRUE; } From 7db4dedf6b2f910865ebbc5864134e9345e52c5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 8 Aug 2018 07:56:29 +0200 Subject: [PATCH 2/4] Remove unused NS_QUERYATTR_CACHEGLUEOK query attribute The NS_QUERYATTR_CACHEGLUEOK query attribute has no influence on query processing. Remove it. --- lib/ns/include/ns/query.h | 1 - lib/ns/query.c | 5 ----- 2 files changed, 6 deletions(-) diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index e6ca2cf4ae..054c95dfeb 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -101,7 +101,6 @@ struct ns_query { #define NS_QUERYATTR_PARTIALANSWER 0x0004 #define NS_QUERYATTR_NAMEBUFUSED 0x0008 #define NS_QUERYATTR_RECURSING 0x0010 -#define NS_QUERYATTR_CACHEGLUEOK 0x0020 #define NS_QUERYATTR_QUERYOKVALID 0x0040 #define NS_QUERYATTR_QUERYOK 0x0080 #define NS_QUERYATTR_WANTRECURSION 0x0100 diff --git a/lib/ns/query.c b/lib/ns/query.c index 58ffe40c1e..e04b9eefd2 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -112,9 +112,6 @@ do { \ /*% Recursing? */ #define RECURSING(c) (((c)->query.attributes & \ NS_QUERYATTR_RECURSING) != 0) -/*% Cache glue ok? */ -#define CACHEGLUEOK(c) (((c)->query.attributes & \ - NS_QUERYATTR_CACHEGLUEOK) != 0) /*% Want Recursion? */ #define WANTRECURSION(c) (((c)->query.attributes & \ NS_QUERYATTR_WANTRECURSION) != 0) @@ -8005,7 +8002,6 @@ query_delegation(query_ctx_t *qctx) { /* * This is the best answer. */ - qctx->client->query.attributes |= NS_QUERYATTR_CACHEGLUEOK; qctx->client->query.isreferral = ISC_TRUE; if (!dns_db_iscache(qctx->db) && qctx->client->query.gluedb == NULL) { @@ -8026,7 +8022,6 @@ query_delegation(query_ctx_t *qctx) { query_addrrset(qctx->client, &qctx->fname, &qctx->rdataset, sigrdatasetp, qctx->dbuf, DNS_SECTION_AUTHORITY); - qctx->client->query.attributes &= ~NS_QUERYATTR_CACHEGLUEOK; if (detach) { dns_db_detach(&qctx->client->query.gluedb); } From 8e3fc5725fbfac5817eef9715ef9e70120c4ac81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 8 Aug 2018 07:56:29 +0200 Subject: [PATCH 3/4] Extract code preparing a delegation response to a separate function Changes introduced by the previous two commits make the parts of query_delegation() and query_zone_delegation() which prepare a delegation response functionally equivalent. Extract this code into a separate function, query_prepare_delegation_response(), and then call the latter from both query_delegation() and query_zone_delegation() in order to reduce code duplication. Add a comment describing the purpose of the extracted code. Fix coding style issues. --- lib/ns/query.c | 149 +++++++++++++++++-------------------------------- 1 file changed, 50 insertions(+), 99 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index e04b9eefd2..659038b532 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -7725,6 +7725,54 @@ query_notfound(query_ctx_t *qctx) { return (query_delegation(qctx)); } +/*% + * We have a delegation but recursion is not allowed, so return the delegation + * to the client. + */ +static isc_result_t +query_prepare_delegation_response(query_ctx_t *qctx) { + dns_rdataset_t **sigrdatasetp = NULL; + isc_boolean_t detach = ISC_FALSE; + + /* + * qctx->fname could be released in query_addrrset(), so save a copy of + * it here in case we need it. + */ + dns_fixedname_init(&qctx->dsname); + dns_name_copy(qctx->fname, dns_fixedname_name(&qctx->dsname), NULL); + + /* + * This is the best answer. + */ + qctx->client->query.isreferral = ISC_TRUE; + + if (!dns_db_iscache(qctx->db) && qctx->client->query.gluedb == NULL) { + dns_db_attach(qctx->db, &qctx->client->query.gluedb); + detach = ISC_TRUE; + } + + /* + * We must ensure NOADDITIONAL is off, because the generation of + * additional data is required in delegations. + */ + qctx->client->query.attributes &= ~NS_QUERYATTR_NOADDITIONAL; + if (WANTDNSSEC(qctx->client) && qctx->sigrdataset != NULL) { + sigrdatasetp = &qctx->sigrdataset; + } + query_addrrset(qctx->client, &qctx->fname, &qctx->rdataset, + sigrdatasetp, qctx->dbuf, DNS_SECTION_AUTHORITY); + if (detach) { + dns_db_detach(&qctx->client->query.gluedb); + } + + /* + * Add a DS if needed. + */ + query_addds(qctx); + + return (query_done(qctx)); +} + /*% * Handle a delegation response from an authoritative lookup. This * may trigger additional lookups, e.g. from the cache database to @@ -7734,8 +7782,6 @@ query_notfound(query_ctx_t *qctx) { static isc_result_t query_zone_delegation(query_ctx_t *qctx) { isc_result_t result; - dns_rdataset_t **sigrdatasetp = NULL; - isc_boolean_t detach = ISC_FALSE; /* * If the query type is DS, look to see if we are @@ -7812,57 +7858,7 @@ query_zone_delegation(query_ctx_t *qctx) { return (query_lookup(qctx)); } - /* - * qctx->fname could be released in - * query_addrrset(), so save a copy of it - * here in case we need it - */ - dns_fixedname_init(&qctx->dsname); - dns_name_copy(qctx->fname, dns_fixedname_name(&qctx->dsname), NULL); - - /* - * If we don't have a cache, this is the best - * answer. - * - * If the client is making a nonrecursive - * query we always give out the authoritative - * delegation. This way even if we get - * junk in our cache, we won't fail in our - * role as the delegating authority if another - * nameserver asks us about a delegated - * subzone. - * - * We enable the retrieval of glue for this - * database by setting client->query.gluedb. - */ - if (qctx->db != NULL && qctx->client->query.gluedb == NULL) { - dns_db_attach(qctx->db, &qctx->client->query.gluedb); - detach = ISC_TRUE; - } - qctx->client->query.isreferral = ISC_TRUE; - /* - * We must ensure NOADDITIONAL is off, - * because the generation of - * additional data is required in - * delegations. - */ - qctx->client->query.attributes &= - ~NS_QUERYATTR_NOADDITIONAL; - if (WANTDNSSEC(qctx->client) && qctx->sigrdataset != NULL) - sigrdatasetp = &qctx->sigrdataset; - query_addrrset(qctx->client, &qctx->fname, - &qctx->rdataset, sigrdatasetp, - qctx->dbuf, DNS_SECTION_AUTHORITY); - if (detach) { - dns_db_detach(&qctx->client->query.gluedb); - } - - /* - * Add a DS if needed - */ - query_addds(qctx); - - return (query_done(qctx)); + return (query_prepare_delegation_response(qctx)); } /*% @@ -7876,8 +7872,6 @@ query_zone_delegation(query_ctx_t *qctx) { static isc_result_t query_delegation(query_ctx_t *qctx) { isc_result_t result; - dns_rdataset_t **sigrdatasetp = NULL; - isc_boolean_t detach = ISC_FALSE; qctx->authoritative = ISC_FALSE; @@ -7988,50 +7982,7 @@ query_delegation(query_ctx_t *qctx) { return (query_done(qctx)); } - /* - * We have a delegation but recursion is not - * allowed, so return the delegation to the client. - * - * qctx->fname could be released in - * query_addrrset(), so save a copy of it - * here in case we need it - */ - dns_fixedname_init(&qctx->dsname); - dns_name_copy(qctx->fname, dns_fixedname_name(&qctx->dsname), NULL); - - /* - * This is the best answer. - */ - qctx->client->query.isreferral = ISC_TRUE; - - if (!dns_db_iscache(qctx->db) && qctx->client->query.gluedb == NULL) { - dns_db_attach(qctx->db, &qctx->client->query.gluedb); - detach = ISC_TRUE; - } - - /* - * We must ensure NOADDITIONAL is off, - * because the generation of - * additional data is required in - * delegations. - */ - qctx->client->query.attributes &= ~NS_QUERYATTR_NOADDITIONAL; - if (WANTDNSSEC(qctx->client) && qctx->sigrdataset != NULL) { - sigrdatasetp = &qctx->sigrdataset; - } - query_addrrset(qctx->client, &qctx->fname, - &qctx->rdataset, sigrdatasetp, - qctx->dbuf, DNS_SECTION_AUTHORITY); - if (detach) { - dns_db_detach(&qctx->client->query.gluedb); - } - - /* - * Add a DS if needed - */ - query_addds(qctx); - - return (query_done(qctx)); + return (query_prepare_delegation_response(qctx)); } /*% From 1d9c37876b8574736d51eb588e8e11a261af165b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Wed, 8 Aug 2018 07:56:29 +0200 Subject: [PATCH 4/4] Add CHANGES entry 5006. [cleanup] Code preparing a delegation response was extracted from query_delegation() and query_zone_delegation() into a separate function in order to decrease code duplication. [GL #431] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 8de006732d..74d4e5a2d2 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +5006. [cleanup] Code preparing a delegation response was extracted from + query_delegation() and query_zone_delegation() into a + separate function in order to decrease code + duplication. [GL #431] + 5005. [bug] dnssec-verify, and dnssec-signzone at the verification step, failed on some validly signed zones. [GL #442]