From: Vladimír Čunát <vladimir.cunat@nic.cz>
Date: Mon, 16 Nov 2020 13:28:49 +0000 (+0100)
Subject: iterate: accept parent-side records for nameservers
X-Git-Tag: v5.3.0~30^2~9
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bb719defd5d163bd9c7cf1e729bf0705924f47b6;p=thirdparty%2Fknot-resolver.git

iterate: accept parent-side records for nameservers

When resolving just NS names and addresses which won't go to answers,
our cache can satisfy them with just parent-side records.
Now we also make iterator consistent with that, and it will cut short
if a delegation satisfies what the kr_query wants.

There's a general risk that we will never get the child-side records,
and in practice the parent-side ones are sometimes "less accurate".
This change may increase this risk (to NS addresses in particular),
but we'd better consider addressing the risk later and systematically.
A suggestion is to refresh the records asynchronously:
https://tools.ietf.org/html/draft-ietf-dnsop-ns-revalidation

---
State before this commit lead to a weird behaviour where some IPv4-only
tests in Deckard (namely `iter_pcdiff.rpl`) were failing with IPv6
turned off.

This was due to the resolvers' internal preference towards AAAA records
for NS names.  With IPv6 networking on, NS name resolution was first
done for AAAA record and the glue (containing A record for the NS name
in question) from parent zone was put into cache.  As the AAAA
resolution failed (there is no AAAA for this NS name), A was queried
next and was satisfied from cache.

With IPv6 off, there is no query for the AAAA record, so no A record
from glue gets put in to the cache.  A record is resolved first, and
resolution ignores the glue in parent zone and continue to the child
zone which might be broken (intentionally in the case of that
`iter_pcdiff.rpl` test).
---

diff --git a/NEWS b/NEWS
index 8c18cd51c..446371e1b 100644
--- a/NEWS
+++ b/NEWS
@@ -1,6 +1,10 @@
 Knot Resolver 5.2.2 (2021-0m-dd)
 ================================
 
+Improvements
+------------
+- more consistency in using parent-side records for NS addresses (!1097)
+
 Bugfixes
 --------
 - view: fail config if bad subnet is specified (!1112)
diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c
index 4afd5d0fc..e689c4321 100644
--- a/lib/layer/iterate.c
+++ b/lib/layer/iterate.c
@@ -973,6 +973,24 @@ static int prepare_query(kr_layer_t *ctx, knot_pkt_t *pkt)
 	return KR_STATE_CONSUME;
 }
 
+static bool satisfied_by_additional(const struct kr_query *qry)
+{
+	const bool prereq = !qry->flags.STUB && !qry->flags.FORWARD && qry->flags.NONAUTH;
+	if (!prereq)
+		return false;
+	const struct kr_request *req = qry->request;
+	for (ssize_t i = req->add_selected.len - 1; i >= 0; --i) {
+		ranked_rr_array_entry_t *entry = req->add_selected.at[i];
+		if (entry->qry_uid != qry->uid)
+			break;
+		if (entry->rr->type == qry->stype
+		    && knot_dname_is_equal(entry->rr->owner, qry->sname)) {
+			return true;
+		}
+	}
+	return false;
+}
+
 static int resolve_badmsg(knot_pkt_t *pkt, struct kr_request *req, struct kr_query *query)
 {
 
@@ -1103,7 +1121,19 @@ static int resolve(kr_layer_t *ctx, knot_pkt_t *pkt)
 		break;
 	case KR_STATE_DONE: /* Referral */
 		state = process_referral_answer(pkt,req);
-		VERBOSE_MSG("<= referral response, follow\n");
+		if (satisfied_by_additional(query)) { /* This is a little hacky.
+			 * We found sufficient information in ADDITIONAL section
+			 * and it was selected for caching in this CONSUME round.
+			 * To make iterator accept the record in a simple way,
+			 * we trigger another cache *reading* attempt
+			 * for the subsequent PRODUCE round.
+			 */
+			assert(query->flags.NONAUTH);
+			query->flags.CACHE_TRIED = false;
+			VERBOSE_MSG("<= referral response, but cache should stop us short now\n");
+		} else {
+			VERBOSE_MSG("<= referral response, follow\n");
+		}
 		break;
 	default:
 		break;