validate nitpick fix: unsupported algo edge case

kr_dnskeys_trusted() semantics is changed, but I do NOT consider that
a part of public API.

Go insecure due to algorithm support even if DNSKEY is NODATA.
I can't see how that's relevant to practical usage, but I think this new
behavior makes more sense.  We still do try to fetch the DNSKEY even
though we have information about its un-usability beforehand.
I'd consider fixing that a premature optimization.
We'll still be affected if the DNSKEY query SERVFAILs or something.

Thanks to PowerDNS people for catching this!

diff --git a/NEWS b/NEWS
index f6a02544163c83d90ea354c1f1f035a1f33f98a0..33b8e68593c65c01c1b75c080504ce7abf349ee0 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -37,6 +37,7 @@ Bugfixes
 - fix flushing of messages to logs in some cases (!781)
 - fix fallback when SERVFAIL or REFUSED is received from upstream (!784)
 - fix crash when dealing with unknown TA key algorhitm (#449)
+- go insecure due to algorithm support even if DNSKEY is NODATA (!798)
 
 Module API changes
 ------------------

diff --git a/lib/dnssec.c b/lib/dnssec.c
index 4f8ad8a260746963b3730ffc09f7d180878a88fb..efcdb1fb87f8b6637ea97dee6773121351cf8120 100644 (file)
--- a/lib/dnssec.c
+++ b/lib/dnssec.c
@@ -268,8 +268,10 @@ static int kr_rrset_validate_with_key(kr_rrset_validation_ctx_t *vctx,
        return vctx->result;
 }
 
-static bool kr_ds_algo_support(const knot_rrset_t *ta)
+bool kr_ds_algo_support(const knot_rrset_t *ta)
 {
+       assert(ta && ta->type == KNOT_RRTYPE_DS && ta->rclass == KNOT_CLASS_IN);
+       /* Check if at least one DS has a usable algorithm pair. */
        knot_rdata_t *rdata_i = ta->rrs.rdata;
        for (uint16_t i = 0; i < ta->rrs.count;
                        ++i, rdata_i = knot_rdataset_next(rdata_i)) {
@@ -293,12 +295,6 @@ int kr_dnskeys_trusted(kr_rrset_validation_ctx_t *vctx, const knot_rrset_t *ta)
                return kr_error(EINVAL);
        }
 
-       /* Check if at least one DS has a usable algorithm pair. */
-       if (!kr_ds_algo_support(ta)) {
-               /* See RFC6840 5.2. */
-               return vctx->result = kr_error(DNSSEC_INVALID_DS_ALGORITHM);
-       }
-
        /* RFC4035 5.2, bullet 1
         * The supplied DS record has been authenticated.
         * It has been validated or is part of a configured trust anchor.

diff --git a/lib/dnssec.h b/lib/dnssec.h
index b54908c18171751467379d3acc654bf82dfdb3dd..8383048b2fa0d5e0d9ab97c47acccfa4caab9f6d 100644 (file)
--- a/lib/dnssec.h
+++ b/lib/dnssec.h
@@ -81,13 +81,17 @@ typedef struct kr_rrset_validation_ctx kr_rrset_validation_ctx_t;
 int kr_rrset_validate(kr_rrset_validation_ctx_t *vctx,
                        const knot_rrset_t *covered);
 
+/**
+ * Return true iff the RRset contains at least one usable DS.  See RFC6840 5.2.
+ */
+KR_EXPORT KR_PURE
+bool kr_ds_algo_support(const knot_rrset_t *ta);
+
 /**
  * Check whether the DNSKEY rrset matches the supplied trust anchor RRSet.
  * @param vctx  Pointer to validation context.
  * @param ta    Trust anchor RRSet against which to validate the DNSKEY RRSet.
- * @return      0 or error code, same as vctx->result.  In particular,
- *             DNSSEC_INVALID_DS_ALGORITHM if *each* DS records is unusable
- *             due to unimplemented DNSKEY or DS algorithm.
+ * @return      0 or error code, same as vctx->result.
  */
 int kr_dnskeys_trusted(kr_rrset_validation_ctx_t *vctx, const knot_rrset_t *ta);
 

diff --git a/lib/layer/validate.c b/lib/layer/validate.c
index 55c3ad774513a3f8f47e74ca8d976893b291723d..16e339602111ea786d8946fa8c9d0576a02ca17d 100644 (file)
--- a/lib/layer/validate.c
+++ b/lib/layer/validate.c
@@ -972,11 +972,8 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt)
        }
 
        if (knot_wire_get_aa(pkt->wire) && qtype == KNOT_RRTYPE_DNSKEY) {
-               ret = validate_keyset(req, pkt, has_nsec3);
-               if (ret == kr_error(EAGAIN)) {
-                       VERBOSE_MSG(qry, ">< cut changed, needs revalidation\n");
-                       return KR_STATE_YIELD;
-               } else if (ret == kr_error(DNSSEC_INVALID_DS_ALGORITHM)) {
+               const knot_rrset_t *ds = qry->zone_cut.trust_anchor;
+               if (ds && !kr_ds_algo_support(ds)) {
                        VERBOSE_MSG(qry, ">< all DS entries use unsupported algorithm pairs, going insecure\n");
                        /* ^ the message is a bit imprecise to avoid being too verbose */
                        qry->flags.DNSSEC_WANT = false;
@@ -984,6 +981,12 @@ static int validate(kr_layer_t *ctx, knot_pkt_t *pkt)
                        rank_records(ctx, KR_RANK_INSECURE, qry->zone_cut.name);
                        mark_insecure_parents(qry);
                        return KR_STATE_DONE;
+               }
+
+               ret = validate_keyset(req, pkt, has_nsec3);
+               if (ret == kr_error(EAGAIN)) {
+                       VERBOSE_MSG(qry, ">< cut changed, needs revalidation\n");
+                       return KR_STATE_YIELD;
                } else if (ret != 0) {
                        VERBOSE_MSG(qry, "<= bad keys, broken trust chain\n");
                        qry->flags.DNSSEC_BOGUS = true;