caches: improvements to STUB and future FORWARD modes

Fixes #122, mostly.  CNAME chains aren't answered from cache in STUB
mode so far, because the current iterator would be unable to follow them.

Previously the caches avoided repeated lookups by checking if it has
a NS address for the query, which disabled any lookup in forwarding modes.
Now it sets the QUERY_NO_CACHE flag instead to stop repeating.

Also those more expensive kr_ta_covers_qry checks are deferred, so that
they're not done when not needed, e.g. in STUB or +cd mode.

diff --git a/NEWS b/NEWS
index a183473643dfcd64782dec421aaf0295d4a925cb..fab3ecfbb497eb3d5a078d7009c440bad3f8a86a 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,10 @@ Security
   insecure, even for a TLD, leading to disabled validation.
   It also fixes answering with non-authoritative data about nameservers.
 
+Improvements
+------------
+- allow answering from cache in non-iterative modes (#122)
+
 
 Knot Resolver 1.2.6 (2017-04-24)
 ================================

diff --git a/lib/layer/pktcache.c b/lib/layer/pktcache.c
index cabdcfc886a9e167d85ab214b04a1663200a12d0..1c3f820c9b5c9f9ee8cac953ca324dc6e82ee0b5 100644 (file)
--- a/lib/layer/pktcache.c
+++ b/lib/layer/pktcache.c
@@ -75,14 +75,15 @@ static int loot_pktcache(struct kr_context *ctx, knot_pkt_t *pkt,
        }
 
        uint8_t lowest_rank = KR_RANK_INITIAL | KR_RANK_AUTH;
-
-       /* Records not present under any TA don't have their security verified at all. */
-       const bool ta_covers = kr_ta_covers_qry(ctx, qry->sname, qry->stype);
-       /* ^ TODO: performance? */
-
        /* There's probably little sense for NONAUTH in pktcache. */
-       if (!knot_wire_get_cd(req->answer->wire) && ta_covers) {
-               kr_rank_set(&lowest_rank, KR_RANK_INSECURE);
+
+       if (!knot_wire_get_cd(req->answer->wire) && !(qry->flags & QUERY_STUB)) {
+               /* Records not present under any TA don't have their security verified at all. */
+               bool ta_covers = kr_ta_covers_qry(ctx, qry->sname, qry->stype);
+               /* ^ TODO: performance? */
+               if (ta_covers) {
+                       kr_rank_set(&lowest_rank, KR_RANK_INSECURE);
+               }
        }
        const bool rank_enough = entry->rank >= lowest_rank;
        VERBOSE_MSG(qry, "=> rank: 0%0.2o, lowest 0%0.2o -> satisfied=%d\n",
@@ -144,9 +145,12 @@ static int pktcache_peek(kr_layer_t *ctx, knot_pkt_t *pkt)
            (qry->flags & QUERY_NO_CACHE)) {
                return ctx->state; /* Already resolved/failed */
        }
-       if (qry->ns.addr[0].ip.sa_family != AF_UNSPEC) {
-               return ctx->state; /* Only lookup before asking a query */
-       }
+       /* Both caches only peek for qry->sname and that would be useless
+        * to repeat on every iteration, so disable it from now on.
+        * Note: it's important to skip this if rrcache sets KR_STATE_DONE,
+        * as CNAME chains need more iterations to get fetched. */
+       qry->flags |= QUERY_NO_CACHE;
+
        if (knot_pkt_qclass(pkt) != KNOT_CLASS_IN) {
                return ctx->state; /* Only IN class */
        }
@@ -246,8 +250,9 @@ static int pktcache_stash(kr_layer_t *ctx, knot_pkt_t *pkt)
                .count = data.len
        };
 
-       /* If cd bit is set, make rank bad, otherwise it depends on flags. */
-       if (knot_wire_get_cd(req->answer->wire)) {
+       /* If cd bit is set or we got answer via non-validated forwarding,
+        * make the rank bad; otherwise it depends on flags. */
+       if (knot_wire_get_cd(req->answer->wire) || qry->flags & QUERY_STUB) {
                kr_rank_set(&header.rank, KR_RANK_OMIT);
        } else {
                if (qry->flags & QUERY_DNSSEC_BOGUS) {

diff --git a/lib/layer/rrcache.c b/lib/layer/rrcache.c
index b1d90695296cc9d9099a82196d7a1a6b5ac5cc2a..dc69e4000b09832daf1abaec4c56e992f460710c 100644 (file)
--- a/lib/layer/rrcache.c
+++ b/lib/layer/rrcache.c
@@ -145,35 +145,43 @@ enomem:
 }
 
 /** @internal Try to find a shortcut directly to searched record. */
-static int loot_rrcache(struct kr_context *ctx, knot_pkt_t *pkt,
-                       struct kr_query *qry, uint16_t rrtype, const bool cdbit)
+static int loot_rrcache(struct kr_request *req, knot_pkt_t *pkt,
+                       struct kr_query *qry, uint16_t rrtype)
 {
-       /* Records not present under any TA don't have their security verified at all. */
-       const bool ta_covers = kr_ta_covers_qry(ctx, qry->sname, qry->stype);
-       /* ^ TODO: performance? */
-
+       const bool allow_unverified = knot_wire_get_cd(req->answer->wire)
+                                       || qry->flags & QUERY_STUB;
        /* Lookup direct match first; only consider authoritative records.
         * TODO: move rank handling into the iterator (QUERY_DNSSEC_* flags)? */
        uint8_t rank  = 0;
        uint8_t flags = 0;
-       uint8_t lowest_rank = (ta_covers ? KR_RANK_INSECURE : KR_RANK_INITIAL)
-               | KR_RANK_AUTH;
+       uint8_t lowest_rank = KR_RANK_INITIAL | KR_RANK_AUTH;
        if (qry->flags & QUERY_NONAUTH) {
                lowest_rank = KR_RANK_INITIAL;
                /* Note: there's little sense in validation status for non-auth records.
                 * In case of using NONAUTH to get NS IPs, knowing that you ask correct
                 * IP doesn't matter much for security; it matters whether you can
-                * validate the answers from the NS. */
-       }
-       if (cdbit) {
-               kr_rank_set(&lowest_rank, KR_RANK_INITIAL);
+                * validate the answers from the NS.
+                */
+       } else if (!allow_unverified) {
+                               /* ^^ in stub mode we don't trust RRs anyway */
+               /* Records not present under any TA don't have their security
+                * verified at all, so we also accept low ranks in that case. */
+               const bool ta_covers = kr_ta_covers_qry(req->ctx, qry->sname, rrtype);
+               /* ^ TODO: performance? */
+               if (ta_covers) {
+                       kr_rank_set(&lowest_rank, KR_RANK_INSECURE);
+               }
        }
 
-       struct kr_cache *cache = &ctx->cache;
+       struct kr_cache *cache = &req->ctx->cache;
        int ret = loot_rr(cache, pkt, qry->sname, qry->sclass, rrtype, qry,
                          &rank, &flags, 0, lowest_rank);
-       if (ret != 0 && rrtype != KNOT_RRTYPE_CNAME) {
-               /* Chase CNAME if no direct hit */
+       if (ret != 0 && rrtype != KNOT_RRTYPE_CNAME
+           && !(qry->flags & QUERY_STUB)) {
+               /* Chase CNAME if no direct hit.
+                * We avoid this in STUB mode because the current iterator
+                * (process_stub()) is unable to iterate in STUB mode to follow
+                * the CNAME chain. */
                rrtype = KNOT_RRTYPE_CNAME;
                ret = loot_rr(cache, pkt, qry->sname, qry->sclass, rrtype, qry,
                              &rank, &flags, 0, lowest_rank);
@@ -188,13 +196,19 @@ static int loot_rrcache(struct kr_context *ctx, knot_pkt_t *pkt,
        }
 
        /* Record may have RRSIGs, try to find them. */
-       const bool dobit = (qry->flags & QUERY_DNSSEC_WANT);
-       if (cdbit || (dobit && kr_rank_test(rank, KR_RANK_SECURE))) {
+       if (allow_unverified
+           || ((qry->flags & QUERY_DNSSEC_WANT) && kr_rank_test(rank, KR_RANK_SECURE))) {
                kr_rank_set(&lowest_rank, KR_RANK_INITIAL); /* no security for RRSIGs */
                ret = loot_rr(cache, pkt, qry->sname, qry->sclass, rrtype, qry,
                              &rank, &flags, true, lowest_rank);
+               if (allow_unverified) {
+                       /* TODO: in STUB mode, if we cached from a query without
+                        * DO bit, we will return without RRSIGs even if they
+                        * would be contained in upstream answer with DO. */
+                       ret = 0;
+               }
                if (ret) {
-                       VERBOSE_MSG(qry, "=> RRSIG(s) expected but not found, skipping");
+                       VERBOSE_MSG(qry, "=> RRSIG(s) expected but not found, skipping\n");
                        /* In some cases, e.g. due to bugs, this may fail.
                         * A possible good example is that a cache backend
                         * (such as redis) chose to evict RRSIG but not RRset.
@@ -217,25 +231,21 @@ static int rrcache_peek(kr_layer_t *ctx, knot_pkt_t *pkt)
        struct kr_request *req = ctx->req;
        struct kr_query *qry = req->current_query;
        if (ctx->state & (KR_STATE_FAIL|KR_STATE_DONE) || (qry->flags & QUERY_NO_CACHE)) {
-               return ctx->state; /* Already resolved/failed */
-       }
-       if (qry->ns.addr[0].ip.sa_family != AF_UNSPEC) {
-               return ctx->state; /* Only lookup before asking a query */
+               return ctx->state; /* Already resolved/failed or already tried, etc. */
        }
-       const bool cd_is_set = knot_wire_get_cd(req->answer->wire);
        /* Reconstruct the answer from the cache,
         * it may either be a CNAME chain or direct answer.
         * Only one step of the chain is resolved at a time.
         */
        int ret = -1;
        if (qry->stype != KNOT_RRTYPE_ANY) {
-               ret = loot_rrcache(req->ctx, pkt, qry, qry->stype, cd_is_set);
+               ret = loot_rrcache(req, pkt, qry, qry->stype);
        } else {
                /* ANY query are used by either qmail or certain versions of Firefox.
                 * Probe cache for a few interesting records. */
                static uint16_t any_types[] = { KNOT_RRTYPE_A, KNOT_RRTYPE_AAAA, KNOT_RRTYPE_MX };
                for (size_t i = 0; i < sizeof(any_types)/sizeof(any_types[0]); ++i) {
-                       if (loot_rrcache(req->ctx, pkt, qry, any_types[i], cd_is_set) == 0) {
+                       if (loot_rrcache(req, pkt, qry, any_types[i]) == 0) {
                                ret = 0; /* At least single record matches */
                        }
                }