kr_rank: improve the API to manipulate ranks

_SECURE and _INSECURE weren't real flags, as their setting was
logically exclusive of the "values".  That made changing ranks rather
cumbersome.

Tests: val_cname_to_unsigned_fake_rrsig gets broken, but I hope this
change just uncovered a hidden bug.

diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c
index 0669ea28b0d85eca441dc81a55c903301a824bc1..53329c39bb0ede9243c0e09c5dd683bb29ec6fbc 100644 (file)
--- a/lib/layer/iterate.c
+++ b/lib/layer/iterate.c
@@ -295,17 +295,16 @@ static int update_cut(knot_pkt_t *pkt, const knot_rrset_t *rr,
 static inline uint8_t get_initial_rank(const knot_rrset_t *rr,
                                       const struct kr_query *qry, bool answer)
 {
-       const uint32_t qflags = qry->flags;
-       uint8_t rank = 0;
-       if (qflags & QUERY_CACHED) {
-               rank = rr->additional ? *(uint8_t *)rr->additional : KR_RANK_OMIT;
+       if (qry->flags & QUERY_CACHED) {
+               return rr->additional ? *(uint8_t *)rr->additional : KR_RANK_OMIT;
                /* ^^ Current use case for "cached" RRs without rank: hints module. */
        } else if (answer || rr->type == KNOT_RRTYPE_DS
                   || rr->type == KNOT_RRTYPE_NSEC || rr->type == KNOT_RRTYPE_NSEC3) {
                /* TODO: this classifier of authoritativity may not be perfect yet. */
-               rank |= KR_RANK_AUTH;
+               return KR_RANK_INITIAL | KR_RANK_AUTH;
+       } else {
+               return KR_RANK_INITIAL;
        }
-       return rank;
 }
 
 static int pick_authority(knot_pkt_t *pkt, struct kr_request *req, bool to_wire)

diff --git a/lib/layer/pktcache.c b/lib/layer/pktcache.c
index f744cebcbebe8de6b918f8c79a8c093086553759..6725fa48b60cf49fb140ac1efc09e7ec3483a2a7 100644 (file)
--- a/lib/layer/pktcache.c
+++ b/lib/layer/pktcache.c
@@ -70,10 +70,11 @@ static int loot_pktcache(struct kr_cache *cache, knot_pkt_t *pkt,
                return ret;
        }
 
-       uint8_t lowest_rank = KR_RANK_AUTH | KR_RANK_INSECURE;
+       uint8_t lowest_rank = KR_RANK_INITIAL | KR_RANK_AUTH;
        /* There's probably little sense for NONAUTH in pktcache. */
-       if (knot_wire_get_cd(req->answer->wire)) {
-               lowest_rank &= ~KR_RANK_INSECURE;
+       if (!knot_wire_get_cd(req->answer->wire)) {
+               // FIXME: only if we can cover the name by a TA
+               kr_rank_set(&lowest_rank, KR_RANK_INSECURE);
        }
        if (entry->rank < lowest_rank) {
                return kr_error(ENOENT);
@@ -93,7 +94,7 @@ static int loot_pktcache(struct kr_cache *cache, knot_pkt_t *pkt,
        }
 
        /* Rank-related fixups.  Add rank into the additional field. */
-       if (entry->rank & KR_RANK_INSECURE) {
+       if (kr_rank_test(entry->rank, KR_RANK_INSECURE)) {
                qry->flags |= QUERY_DNSSEC_INSECURE;
                qry->flags &= ~QUERY_DNSSEC_WANT;
        }
@@ -238,14 +239,14 @@ static int pktcache_stash(kr_layer_t *ctx, knot_pkt_t *pkt)
 
        /* If cd bit is set, make rank bad, otherwise it depends on flags. */
        if (knot_wire_get_cd(req->answer->wire)) {
-               rank_set_value(&header.rank, KR_RANK_OMIT);
+               kr_rank_set(&header.rank, KR_RANK_OMIT);
        } else {
                if (qry->flags & QUERY_DNSSEC_BOGUS) {
-                       rank_set_value(&header.rank, KR_RANK_BOGUS);
+                       kr_rank_set(&header.rank, KR_RANK_BOGUS);
                } else if (qry->flags & QUERY_DNSSEC_INSECURE) {
-                       header.rank |= KR_RANK_INSECURE;
+                       kr_rank_set(&header.rank, KR_RANK_INSECURE);
                } else if (qry->flags & QUERY_DNSSEC_WANT) {
-                       header.rank |= KR_RANK_SECURE;
+                       kr_rank_set(&header.rank, KR_RANK_SECURE);
                }
        }
 

diff --git a/lib/layer/rrcache.c b/lib/layer/rrcache.c
index dd66f311cf2f351652ef920ee586019d6893b650..61ba47e57e597bc11dcec2b21e988d0374a45fe1 100644 (file)
--- a/lib/layer/rrcache.c
+++ b/lib/layer/rrcache.c
@@ -145,17 +145,16 @@ static int loot_rrcache(struct kr_cache *cache, knot_pkt_t *pkt,
         * TODO: move rank handling into the iterator (QUERY_DNSSEC_* flags)? */
        uint8_t rank  = 0;
        uint8_t flags = 0;
-       uint8_t lowest_rank = KR_RANK_AUTH | KR_RANK_INSECURE;
+       uint8_t lowest_rank = KR_RANK_INSECURE | KR_RANK_AUTH;
        if (qry->flags & QUERY_NONAUTH) {
-               lowest_rank &= ~KR_RANK_AUTH;
-               lowest_rank &= ~KR_RANK_INSECURE;
+               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) {
-               lowest_rank &= ~KR_RANK_INSECURE;
+               kr_rank_set(&lowest_rank, KR_RANK_INITIAL);
        }
 
        int ret = loot_rr(cache, pkt, qry->sname, qry->sclass, rrtype, qry,
@@ -169,14 +168,16 @@ static int loot_rrcache(struct kr_cache *cache, knot_pkt_t *pkt,
        if (ret) {
                return ret;
        }
-       if (rank & KR_RANK_INSECURE) {
+
+       if (kr_rank_test(rank, KR_RANK_INSECURE)) {
                qry->flags |= QUERY_DNSSEC_INSECURE;
                qry->flags &= ~QUERY_DNSSEC_WANT;
        }
 
        /* Record may have RRSIGs, try to find them. */
        const bool dobit = (qry->flags & QUERY_DNSSEC_WANT);
-       if (cdbit || (dobit && (rank & KR_RANK_SECURE))) {
+       if (cdbit || (dobit && 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);
        }
@@ -262,7 +263,7 @@ static int commit_rr(const char *key, void *val, void *data)
                return commit_rrsig(baton, rank, KR_CACHE_FLAG_NONE, rr);
        }
        /* Accept only better rank if not secure. */
-       if (!(rank & KR_RANK_SECURE)) {
+       if (!kr_rank_test(rank, KR_RANK_SECURE)) {
                int cached_rank = kr_cache_peek_rank(baton->cache, KR_CACHE_RR, rr->owner, rr->type, baton->timestamp);
                /* If equal rank was accepted, spoofing a single answer would be enough
                 * to e.g. override NS record in AUTHORITY section.
@@ -273,7 +274,7 @@ static int commit_rr(const char *key, void *val, void *data)
        }
 
        uint8_t flags = KR_CACHE_FLAG_NONE;
-       if (rank & KR_RANK_AUTH) {
+       if (kr_rank_test(rank, KR_RANK_AUTH)) {
                if (baton->qry->flags & QUERY_DNSSEC_WEXPAND) {
                        flags |= KR_CACHE_FLAG_WCARD_PROOF;
                }

diff --git a/lib/layer/validate.c b/lib/layer/validate.c
index bc62c815c9e4ef153aafa5bc0f0d60e29435b09f..9ab00de6edf1c1ac4754a62347a84c59d095eba8 100644 (file)
--- a/lib/layer/validate.c
+++ b/lib/layer/validate.c
@@ -76,29 +76,6 @@ static bool pkt_has_type(const knot_pkt_t *pkt, uint16_t type)
        return section_has_type(knot_pkt_section(pkt, KNOT_ADDITIONAL), type);
 }
 
-static inline uint8_t rank_get_flags(uint8_t rank)
-{
-       return rank & ~0x07;
-}
-
-static inline void rank_set_flag(uint8_t *rank, uint8_t flag_to_set)
-{
-       assert(rank);
-       *rank |= rank_get_flags(flag_to_set);
-}
-
-static inline void rank_clear_flag(uint8_t *rank, uint8_t flag_to_clear)
-{
-       assert(rank);
-       uint8_t flag = rank_get_flags(flag_to_clear);
-       *rank &= ~flag;
-}
-
-static inline bool rank_test_flag(uint8_t rank, uint8_t flag)
-{
-       return (flag & rank_get_flags(rank)) != 0;
-}
-
 static int validate_section(kr_rrset_validation_ctx_t *vctx, knot_mm_t *pool)
 {
        if (!vctx) {
@@ -114,47 +91,39 @@ static int validate_section(kr_rrset_validation_ctx_t *vctx, knot_mm_t *pool)
        for (ssize_t i = 0; i < vctx->rrs->len; ++i) {
                ranked_rr_array_entry_t *entry = vctx->rrs->at[i];
                const knot_rrset_t *rr = entry->rr;
-               const uint8_t rank_value = rank_get_value(entry->rank);
-
-               assert((rank_value & (KR_RANK_SECURE | KR_RANK_INSECURE)) != (KR_RANK_SECURE | KR_RANK_INSECURE));
 
                if (entry->yielded || vctx->qry_uid != entry->qry_uid) {
                        continue;
                }
 
-               if ((rank_value != KR_RANK_INITIAL && rank_value != KR_RANK_MISMATCH)) {
-                       continue;
-               }
-
-               if (rank_test_flag(entry->rank, KR_RANK_SECURE)) {
+               if (!kr_rank_test(entry->rank, KR_RANK_INITIAL)
+                   && !kr_rank_test(entry->rank, KR_RANK_MISMATCH)) {
                        continue;
                }
 
                if (rr->type == KNOT_RRTYPE_RRSIG) {
                        const knot_dname_t *signer_name = knot_rrsig_signer_name(&rr->rrs, 0);
-                       rank_clear_flag(&entry->rank, KR_RANK_SECURE | KR_RANK_INSECURE);
                        if (!knot_dname_is_equal(vctx->zone_name, signer_name)) {
-                               rank_set_value(&entry->rank, KR_RANK_MISMATCH);
+                               kr_rank_set(&entry->rank, KR_RANK_MISMATCH);
                                vctx->err_cnt += 1;
                                break;
                        }
-                       rank_set_value(&entry->rank, KR_RANK_OMIT);
+                       kr_rank_set(&entry->rank, KR_RANK_OMIT);
                        continue;
                }
                if ((rr->type == KNOT_RRTYPE_NS) && (vctx->section_id == KNOT_AUTHORITY)) {
-                       rank_clear_flag(&entry->rank, KR_RANK_SECURE | KR_RANK_INSECURE);
-                       rank_set_value(&entry->rank, KR_RANK_OMIT);
+                       kr_rank_set(&entry->rank, KR_RANK_OMIT);
                        continue;
                }
                validation_result = kr_rrset_validate(vctx, rr);
                if (validation_result == kr_ok()) {
-                       rank_set_flag(&entry->rank, KR_RANK_SECURE);
+                       kr_rank_set(&entry->rank, KR_RANK_SECURE);
                } else if (validation_result == kr_error(ENOENT)) {
                        /* no RRSIGs found */
-                       rank_set_flag(&entry->rank, KR_RANK_INSECURE);
+                       kr_rank_set(&entry->rank, KR_RANK_INSECURE);
                        vctx->err_cnt += 1;
                } else {
-                       rank_set_value(&entry->rank, KR_RANK_BOGUS);
+                       kr_rank_set(&entry->rank, KR_RANK_BOGUS);
                        vctx->err_cnt += 1;
                }
        }
@@ -430,7 +399,7 @@ static const knot_dname_t *find_first_signer(ranked_rr_array_t *arr)
        for (size_t i = 0; i < arr->len; ++i) {
                ranked_rr_array_entry_t *entry = arr->at[i];
                const knot_rrset_t *rr = entry->rr;
-               if (entry->yielded || rank_get_value(entry->rank) != KR_RANK_INITIAL) {
+               if (entry->yielded || !kr_rank_test(entry->rank, KR_RANK_INITIAL)) {
                        continue;
                }
                if (rr->type == KNOT_RRTYPE_RRSIG) {
@@ -502,13 +471,13 @@ static int check_validation_result(kr_layer_t *ctx, ranked_rr_array_t *arr)
                if (entry->yielded || entry->qry_uid != qry->uid) {
                        continue;
                }
-               if (rank_get_value(entry->rank) == KR_RANK_MISMATCH) {
+               if (kr_rank_test(entry->rank, KR_RANK_MISMATCH)) {
                        invalid_entry = entry;
                        break;
-               } else if (rank_test_flag(entry->rank, KR_RANK_INSECURE) &&
+               } else if (kr_rank_test(entry->rank, KR_RANK_INSECURE) &&
                           !invalid_entry) {
                        invalid_entry = entry;
-               } else if (!rank_test_flag(entry->rank, KR_RANK_SECURE) &&
+               } else if (!kr_rank_test(entry->rank, KR_RANK_SECURE) &&
                           !invalid_entry) {
                        invalid_entry = entry;
                }
@@ -518,7 +487,7 @@ static int check_validation_result(kr_layer_t *ctx, ranked_rr_array_t *arr)
                return ret;
        }
 
-       if (!rank_test_flag(invalid_entry->rank, KR_RANK_SECURE) &&
+       if (!kr_rank_test(invalid_entry->rank, KR_RANK_SECURE) &&
            (++(invalid_entry->revalidation_cnt) > MAX_REVALIDATION_CNT)) {
                VERBOSE_MSG(qry, "<= continuous revalidation, fails\n");
                qry->flags |= QUERY_DNSSEC_BOGUS;
@@ -526,7 +495,7 @@ static int check_validation_result(kr_layer_t *ctx, ranked_rr_array_t *arr)
        }
 
        const knot_rrset_t *rr = invalid_entry->rr;
-       if (rank_get_value(invalid_entry->rank) == KR_RANK_MISMATCH) {
+       if (kr_rank_test(invalid_entry->rank, KR_RANK_MISMATCH)) {
                const knot_dname_t *signer_name = knot_rrsig_signer_name(&rr->rrs, 0);
                if (knot_dname_is_sub(signer_name, qry->zone_cut.name)) {
                        qry->zone_cut.name = knot_dname_copy(signer_name, &req->pool);
@@ -541,9 +510,9 @@ static int check_validation_result(kr_layer_t *ctx, ranked_rr_array_t *arr)
                }
                VERBOSE_MSG(qry, ">< cut changed (new signer), needs revalidation\n");
                ret = KR_STATE_YIELD;
-       } else if (rank_test_flag(invalid_entry->rank, KR_RANK_INSECURE)) {
+       } else if (kr_rank_test(invalid_entry->rank, KR_RANK_INSECURE)) {
                ret = rrsig_not_found(ctx, rr);
-       } else if (!rank_test_flag(invalid_entry->rank, KR_RANK_SECURE)) {
+       } else if (!kr_rank_test(invalid_entry->rank, KR_RANK_SECURE)) {
                qry->flags |= QUERY_DNSSEC_BOGUS;
                ret = KR_STATE_FAIL;
        }
@@ -619,7 +588,7 @@ static int check_signer(kr_layer_t *ctx, knot_pkt_t *pkt)
        return KR_STATE_DONE;
 }
 
-static void flag_records(kr_layer_t *ctx, uint8_t flag_to_set)
+static void rank_records(kr_layer_t *ctx, uint8_t rank_to_set)
 {
        struct kr_request *req     = ctx->req;
        struct kr_query *qry       = req->current_query;
@@ -631,28 +600,8 @@ static void flag_records(kr_layer_t *ctx, uint8_t flag_to_set)
                        if (entry->qry_uid != qry->uid) {
                                continue;
                        }
-                       if (rank_get_value(entry->rank) == KR_RANK_INITIAL) {
-                               rank_set_flag(&entry->rank, flag_to_set);
-                       }
-               }
-       }
-}
-
-static void rank_records(kr_layer_t *ctx, uint8_t rank_to_set)
-{
-       struct kr_request *req  = ctx->req;
-       struct kr_query *qry    = req->current_query;
-       ranked_rr_array_t *arr  = &req->answ_selected;
-       ranked_rr_array_t *ptrs[2] = { &req->answ_selected, &req->auth_selected };
-       for (size_t i = 0; i < 1; ++i) {
-               ranked_rr_array_t *arr = ptrs[i];
-               for (size_t j = 0; j < arr->len; ++j) {
-                       ranked_rr_array_entry_t *entry = arr->at[j];
-                       if (entry->qry_uid != qry->uid) {
-                               continue;
-                       }
-                       if (rank_get_value(entry->rank) == KR_RANK_INITIAL) {
-                               rank_set_value(&entry->rank, rank_to_set);
+                       if (kr_rank_test(entry->rank, KR_RANK_INITIAL)) {
+                               kr_rank_set(&entry->rank, rank_to_set);
                        }
                }
        }
@@ -678,7 +627,7 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt)
                const uint32_t test_flags = (QUERY_CACHED | QUERY_DNSSEC_INSECURE);
                const bool is_insec = ((qry->flags & test_flags) == test_flags);
                if ((qry->flags & QUERY_DNSSEC_INSECURE)) {
-                       flag_records(ctx, KR_RANK_INSECURE);
+                       rank_records(ctx, KR_RANK_INSECURE);
                }
                if (is_insec && qry->parent != NULL) {
                        /* We have got insecure answer from cache.

diff --git a/lib/resolve.c b/lib/resolve.c
index a766230e342268597622ad4e2b7ea0307ce6e1bc..c2fc9a85fbc03bb42b9cc13436b5d7bb81542887 100644 (file)
--- a/lib/resolve.c
+++ b/lib/resolve.c
@@ -37,6 +37,22 @@
 
 #define VERBOSE_MSG(qry, fmt...) QRVERBOSE((qry), "resl",  fmt)
 
+bool kr_rank_check(uint8_t rank)
+{
+       switch (rank & ~KR_RANK_AUTH) {
+       case KR_RANK_INITIAL:
+       case KR_RANK_OMIT:
+       case KR_RANK_INDET:
+       case KR_RANK_BOGUS:
+       case KR_RANK_MISMATCH:
+       case KR_RANK_INSECURE:
+       case KR_RANK_SECURE:
+               return true;
+       default:
+               return false;
+       }
+}
+
 /** @internal Set @a yielded to all RRs with matching @a qry_uid. */
 static void set_yield(ranked_rr_array_t *array, const uint32_t qry_uid, const bool yielded)
 {

diff --git a/lib/resolve.h b/lib/resolve.h
index acbeb146c902c0a5f0b20e98b78599b12ad63c8b..bbc5ed3a5c2cecf6a5a450b6487080df6e87fdb3 100644 (file)
--- a/lib/resolve.h
+++ b/lib/resolve.h
@@ -76,10 +76,12 @@
 /**
  * RRset rank - for cache and ranked_rr_*.
  *
- * The rank has three one-bit flags and additionally several values.
- * The values are best manipulated via rank_*_value functions.
+ * The rank meaning consists of one independent flag - KR_RANK_AUTH,
+ * and the rest have meaning of values where only one can hold at any time.
+ * You can use one of the enums as a safe initial value, optionally | KR_RANK_AUTH;
+ * otherwise it's best to manipulate ranks via the kr_rank_* functions.
  *
- * @note Be careful about chosen cache rank nominal values.
+ * @note The representation is complicated by restrictions on integer comparison:
  * - AUTH must be > than !AUTH
  * - AUTH INSECURE must be > than AUTH (because it attempted validation)
  * - !AUTH SECURE must be > than AUTH (because it's valid)
@@ -89,7 +91,6 @@
  *   https://tools.ietf.org/html/rfc4035#section-4.3
  */
 enum kr_rank {
-       KR_RANK_BAD = 7,     /**< For simpler manipulation with the values below. */
        KR_RANK_INITIAL = 0, /**< Did not attempt to validate. */
        KR_RANK_OMIT = 1,    /**< Do not attempt to validate. */
        KR_RANK_INDET,       /**< Unable to determine whether it should be secure. */
@@ -107,16 +108,30 @@ enum kr_rank {
        /* @note Rank must not exceed 6 bits */
 };
 
-static inline uint8_t rank_get_value(uint8_t rank)
+/** Check that a rank value is valid.  Meant for assertions. */
+bool kr_rank_check(uint8_t rank) KR_PURE;
+
+/** Test the presence of any flag/state in a rank, i.e. including KR_RANK_AUTH. */
+static inline bool kr_rank_test(uint8_t rank, uint8_t kr_flag)
 {
-       return rank & KR_RANK_BAD;
+       assert(kr_rank_check(rank) && kr_rank_check(kr_flag));
+       if (kr_flag == KR_RANK_AUTH) {
+               return rank & KR_RANK_AUTH;
+       }
+       assert(!(kr_flag & KR_RANK_AUTH));
+       /* The rest are exclusive values - exactly one has to be set. */
+       return (rank & ~KR_RANK_AUTH) == kr_flag;
 }
-static inline void rank_set_value(uint8_t *rank, uint8_t val)
+
+/** Set the rank state. The _AUTH flag is kept as it was. */
+static inline void kr_rank_set(uint8_t *rank, uint8_t kr_flag)
 {
-       assert(rank && (val & KR_RANK_BAD) == val);
-       *rank = (*rank & ~KR_RANK_BAD) | val;
+       assert(rank && kr_rank_check(*rank));
+       assert(kr_rank_check(kr_flag) && !(kr_flag & KR_RANK_AUTH));
+       *rank = kr_flag | (*rank & KR_RANK_AUTH);
 }
 
+
 /** @cond internal Array of modules. */
 typedef array_t(struct kr_module *) module_array_t;
 /* @endcond */
@@ -130,7 +145,7 @@ typedef array_t(struct kr_module *) module_array_t;
  *       be shared between threads.
  */
 struct kr_context
-{      
+{
        uint32_t options;
        knot_rrset_t *opt_rr;
        map_t trust_anchors;

diff --git a/lib/zonecut.c b/lib/zonecut.c
index b3d29d23ad5a12bf67ff883dc1ba0b1224f70786..a61a9054bb39fb042ccff48d00a9ec48852aa2a4 100644 (file)
--- a/lib/zonecut.c
+++ b/lib/zonecut.c
@@ -401,8 +401,8 @@ static int fetch_rrset(knot_rrset_t **rr, struct kr_cache *cache,
        if (ret != 0) {
                return ret;
        }
-       const bool rankOK = (rank & KR_RANK_SECURE)
-               || ((rank & KR_RANK_INSECURE) && (rank & KR_RANK_AUTH));
+       const bool rankOK = kr_rank_test(rank, KR_RANK_SECURE)
+               || (kr_rank_test(rank, KR_RANK_INSECURE) && kr_rank_test(rank, KR_RANK_AUTH));
        if (!rankOK) {
                return kr_error(ENOENT);
        }
@@ -442,7 +442,7 @@ int kr_zonecut_find_cached(struct kr_context *ctx, struct kr_zonecut *cut, const
                const bool is_root = (label[0] == '\0');
                if (fetch_ns(ctx, cut, label, timestamp, &rank, &flags) == 0) {
                        /* Flag as insecure if cached as this */
-                       if ((rank & KR_RANK_INSECURE) ||
+                       if (kr_rank_test(rank, KR_RANK_INSECURE) ||
                            (flags & KR_CACHE_FLAG_NODS)) {
                                *secured = false;
                        }