Commit dc300136 authored by Marek Vavrusa's avatar Marek Vavrusa

lib/iterate: QUERY_PERMISSIVE mode

in permissive mode, resolver is free to use
(but not cache) non-mandatory glue records even
if they're not resolvable. this is great as a 
workaround for broken child-side zones, but
not great for security of, well, insecure
delegations. it's off by default.
parent 6e2c36a2
......@@ -224,6 +224,8 @@ static int update_cut(knot_pkt_t *pkt, const knot_rrset_t *rr, struct kr_request
#endif
}
/* Remember current bailiwick for NS processing. */
const knot_dname_t *current_cut = cut->name;
/* Update zone cut name */
if (!knot_dname_is_equal(rr->owner, cut->name)) {
/* Remember parent cut and descend to new (keep keys and TA). */
......@@ -250,7 +252,10 @@ static int update_cut(knot_pkt_t *pkt, const knot_rrset_t *rr, struct kr_request
continue;
}
kr_zonecut_add(cut, ns_name, NULL);
fetch_glue(pkt, ns_name, qry);
/* Use glue only in permissive mode or when in bailiwick. */
if ((qry->flags & QUERY_PERMISSIVE) || knot_dname_in(current_cut, ns_name)) {
fetch_glue(pkt, ns_name, qry);
}
}
return state;
......
......@@ -181,10 +181,9 @@ static int pktcache_stash(knot_layer_t *ctx, knot_pkt_t *pkt)
return ctx->state;
}
/* Cache only NODATA/NXDOMAIN or metatype/RRSIG or
* wildcard expanded answers. */
* wildcard expanded answers. */
const uint16_t qtype = knot_pkt_qtype(pkt);
const bool is_eligible = (knot_rrtype_is_metatype(qtype) ||
qtype == KNOT_RRTYPE_RRSIG);
const bool is_eligible = (knot_rrtype_is_metatype(qtype) || qtype == KNOT_RRTYPE_RRSIG);
int pkt_class = kr_response_classify(pkt);
if (!(is_eligible || (pkt_class & (PKT_NODATA|PKT_NXDOMAIN)) ||
(qry->flags & QUERY_DNSSEC_WEXPAND))) {
......
......@@ -268,7 +268,10 @@ static int stash_authority(struct kr_query *qry, knot_pkt_t *pkt, map_t *stash,
}
/* Look up glue records for NS */
if (rr->type == KNOT_RRTYPE_NS) {
stash_glue(stash, pkt, knot_ns_name(&rr->rrs, 0), pool);
const knot_dname_t *ns_name = knot_ns_name(&rr->rrs, 0);
if (qry->flags & QUERY_PERMISSIVE || knot_dname_in(qry->zone_cut.name, ns_name)) {
stash_glue(stash, pkt, ns_name, pool);
}
  • This hunk is baffling to me. In commit description you say the glue should not be cached, even in permissive mode, but you do the contrary in the code. @vavrusam: can you remember/clarify what was the real intention?

    I currently can't find any RFC that describes (recommendations on) this issue. At the very least it feels wrong to cache arbitrary glue with the same trust level as mandatory glue.

    (We'll fix the case of having multiple NS names in a different commit; I consider that a bug of this commit, but there it's clear what to do.)

    Edited by Vladimír Čunát
  • Glue is either for in-bailiwick NS (e.g. a.ns.cz for cz) or outside (e.g. a.ns.com for cz). It should be cached with EXTRA rank (which is lower priority than authority) for in-bailiwick in case child-side NS doesn't work or child-side is broken and doesn't advertise NS. In such case, parent-side NS is the only viable option. (This is what knot_dname_in check is for, to check bailiwick).

    It may be cached even if it's not in-bailiwick (permissive mode) for the same reason, but it's not recommended and fujiwara's new draft prohibits this afaik. There is often a gray area when something suits better operators that have to work around broken zones, but it's not exactly standardised or generally recommended.

    My opinion is that glue is glue, it should be replaced with child-side information as soon as possible so cache rank of either type of a glue record shouldn't matter that much. It would be a little bit more complicated to change server behaviour to always fetch child-side NS, but doable. rfc2171 should provide some guidance, but afaik it doesn't decide this problem explicitly, so decide what you think is best for users.

  • I had checked 2181, but I can't see it differentiating the glue types; it even uses the other meaning for "glue", as noted by 7719. I didn't realize these should normally get overwritten by authoritative information fast; thus I agree it seems of little importance.

Please register or sign in to reply
}
/* Stash record */
kr_rrmap_add(stash, rr, KR_RANK_NONAUTH, pool);
......@@ -334,9 +337,11 @@ static int rrcache_stash(knot_layer_t *ctx, knot_pkt_t *pkt)
bool is_auth = knot_wire_get_aa(pkt->wire);
if (is_auth) {
ret = stash_answer(qry, pkt, &stash, &req->pool);
}
if (ret == 0) {
ret = stash_authority(qry, pkt, &stash, &req->pool);
}
/* Cache authority only if chasing referral/cname chain */
if (!is_auth || qry != array_tail(req->rplan.pending)) {
} else if (!is_auth || qry != array_tail(req->rplan.pending)) {
  • This part is strange, as is_auth == 0 in this if-expression (still the same in current master). It was/is reported by coverity – I'm not sure you've got access to it.

  • I'm not certain in which cases exactly should authority be cached, but currently we always cache it which is wrong, most likely. @gdemidov: do you remember when it should be done, by chance?

  • It should be cached as long as it's complete with various level of trust (rfc2181), and higher ranking source should overwrite lower ranking source. I think in here it doesn't consider BIND-style NS records in authoritative answer, since resolver now supports rfc2181 trust ranking, it maybe could permit it.

Please register or sign in to reply
ret = stash_authority(qry, pkt, &stash, &req->pool);
}
/* Cache DS records in referrals */
......
......@@ -44,7 +44,8 @@
X(DNSSEC_INSECURE, 1 << 16) /**< Query response is DNSSEC insecure. */ \
X(STUB, 1 << 17) /**< Stub resolution, accept received answer as solved. */ \
X(ALWAYS_CUT, 1 << 18) /**< Always recover zone cut (even if cached). */ \
X(DNSSEC_WEXPAND, 1 << 19) /**< Query response has wildcard expansion. */
X(DNSSEC_WEXPAND, 1 << 19) /**< Query response has wildcard expansion. */ \
X(PERMISSIVE, 1 << 20) /**< Permissive referral path resolution. */
/** Query flags */
enum kr_query_flag {
......
Subproject commit 8bdd07d0a238b861b399ac2158978aa0066063da
Subproject commit 93766685eb05898b5591e2b243906af9f9fc75b9
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment