validator: use rank BOGUS where appropriate instead of MISSING

MISSING triggers re-query to auth in attempt to find missing RRSIGs.
It causes reduntant queries and also puts some BOGUS RRsets in answers.
(It sounds bad but we were correctly setting rcode=SERVFAIL and AD=0
even before this commit.)

Formerly RRSIG ranks did not reflect results of validation.
Now we mark them as BOGUS and upgrade them to SECURE if they validate.

New validator phase answer_finalize prevents BOGUS RRsets from being
put even into SERVFAIL answers.

Closes: #396

diff --git a/NEWS b/NEWS
index 355efbd950211168bd11ba71f76ad1a06d29ebc0..9bedf67100278d593138e397ba04886fba5c6e0d 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,7 @@ Bugfixes
 - fix a strict aliasing problem that might've lead to "miscompilation" (!962)
 - lua resolve(): correctly include EDNS0 in the virtual packet (!963)
   Custom modules might have been confused by that.
+- do not leak bogus data into SERVFAIL answers (#396)
 
 Incompatible changes
 --------------------

diff --git a/lib/cache/api.c b/lib/cache/api.c
index 9962fcc78964b701306f3734b1f89ff82fc7f6a4..df5c574ab308cd3ac6d0b9ed56b17ca02048d56f 100644 (file)
--- a/lib/cache/api.c
+++ b/lib/cache/api.c
@@ -477,6 +477,13 @@ static ssize_t stash_rrset(struct kr_cache *cache, const struct kr_query *qry,
                const knot_rrset_t *rr, const knot_rrset_t *rr_sigs, uint32_t timestamp,
                uint8_t rank, trie_t *nsec_pmap, bool *needs_pkt)
 {
+       if (kr_rank_test(rank, KR_RANK_BOGUS)) {
+               WITH_VERBOSE(qry) {
+                       auto_free char *type_str = kr_rrtype_text(rr->type);
+                       VERBOSE_MSG(qry, "=> skipping bogus RR set %s\n", type_str);
+               }
+               return kr_ok();
+       }
        assert(stash_rrset_precond(rr, qry) > 0);
        if (!cache) {
                assert(!EINVAL);

diff --git a/lib/dnssec.c b/lib/dnssec.c
index 43b0405a9c794627ab7f7f14e2fdf59f41b572ff..50b53f2e0f06a0f73be82e09140168d1e7f71da4 100644 (file)
--- a/lib/dnssec.c
+++ b/lib/dnssec.c
@@ -208,6 +208,7 @@ static int kr_rrset_validate_with_key(kr_rrset_validation_ctx_t *vctx,
                        if (knot_rrsig_type_covered(rdata_j) != covered->type) {
                                continue;
                        }
+                       kr_rank_set(&vctx->rrs->at[i]->rank, KR_RANK_BOGUS); /* defensive style */
                        vctx->rrs_counters.matching_name_type++;
                        int retv = validate_rrsig_rr(&val_flgs, covered_labels, rdata_j,
                                              keys->owner, key_rdata, keytag,
@@ -265,6 +266,7 @@ static int kr_rrset_validate_with_key(kr_rrset_validation_ctx_t *vctx,
 
                        kr_dnssec_key_free(&created_key);
                        vctx->result = kr_ok();
+                       kr_rank_set(&vctx->rrs->at[i]->rank, KR_RANK_SECURE); /* upgrade from bogus */
                        return vctx->result;
                }
        }

diff --git a/lib/layer/validate.c b/lib/layer/validate.c
index 5f01d2326da1b9911a422d3603d1cbfa3aa07159..c60858b430566c7be008d9bc67be3d64b9977d23 100644 (file)
--- a/lib/layer/validate.c
+++ b/lib/layer/validate.c
@@ -115,7 +115,8 @@ static int validate_section(kr_rrset_validation_ctx_t *vctx, const struct kr_que
                                vctx->err_cnt += 1;
                                break;
                        }
-                       kr_rank_set(&entry->rank, KR_RANK_OMIT);
+                       if (!kr_rank_test(entry->rank, KR_RANK_BOGUS))
+                               kr_rank_set(&entry->rank, KR_RANK_OMIT);
                        continue;
                }
 
@@ -137,7 +138,8 @@ static int validate_section(kr_rrset_validation_ctx_t *vctx, const struct kr_que
                         * to MISMATCH on revalidation, e.g. in test val_referral_nods :-/
                         */
 
-               } else if (validation_result == kr_error(ENOENT)) {
+               } else if (validation_result == kr_error(ENOENT)
+                               && vctx->rrs_counters.matching_name_type == 0) {
                        /* no RRSIGs found */
                        kr_rank_set(&entry->rank, KR_RANK_MISSING);
                        vctx->err_cnt += 1;
@@ -1110,11 +1112,40 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt)
        return KR_STATE_DONE;
 }
 
+/** Hide RRsets which did not validate from clients. */
+static int hide_bogus(kr_layer_t *ctx) {
+       if (knot_wire_get_cd(ctx->req->qsource.packet->wire)) {
+               return ctx->state;
+       }
+       /* We don't want to send bogus answers to clients, not even in SERVFAIL
+        * answers, but we cannot drop whole sections. If a CNAME chain
+        * SERVFAILs somewhere, the steps that were OK should be put into
+        * answer.
+        *
+        * There is one specific issue: currently we follow CNAME *before*
+        * we validate it, because... iterator comes before validator.
+        * Therefore some rrsets might be added into req->*_selected before
+        * we detected failure in validator.
+        * TODO: better approach, probably during work on parallel queries.
+        */
+       const ranked_rr_array_t *sel[] = kr_request_selected(ctx->req);
+       for (knot_section_t sect = KNOT_ANSWER; sect <= KNOT_ADDITIONAL; ++sect) {
+                for (size_t i = 0; i < sel[sect]->len; ++i) {
+                         ranked_rr_array_entry_t *e = sel[sect]->at[i];
+                         if (kr_rank_test(e->rank, KR_RANK_BOGUS)) {
+                                  e->to_wire = false;
+                         }
+                }
+       }
+       return ctx->state;
+}
+
 /** Module implementation. */
 int validate_init(struct kr_module *self)
 {
        static const kr_layer_api_t layer = {
                .consume = &validate,
+               .answer_finalize = &hide_bogus,
        };
        self->layer = &layer;
        return kr_ok();

diff --git a/tests/integration/deckard b/tests/integration/deckard
index 84d16f4bf169e1365e6e2bc68efd44592a7e37a1..5f50864d6f4ca79ca77f91d65eea4b23c18736c4 160000 (submodule)
--- a/tests/integration/deckard
+++ b/tests/integration/deckard
@@ -1 +1 @@
-Subproject commit 84d16f4bf169e1365e6e2bc68efd44592a7e37a1
+Subproject commit 5f50864d6f4ca79ca77f91d65eea4b23c18736c4