resolved: refactor DNSSEC answer validation

This changes answer validation to be more accepting to unordered RRs in
responses. The agorithm we now implement goes something like this:

  1. populate validated keys list for this transaction from DS RRs
  2. as long as the following changes the unvalidated answer list:
    2a. try to validate the first RRset we find in unvalidated answer
        list
    2b. if that worked: add to validated answer; if DNSKEY also add to
        validated keys list; remove from unvalidated answer.
    2c. continue at 2a, with the next RRset, or restart from the
        beginning when we hit the end
  3. as long as the following changes the unvalidated answer list:
    3a. try to validate the first RRset again. This will necessarily
        fail, but we learn the precise error
    3b. If this was a "primary" response to the question, fail the
        entire transaction. "Primary" in this context means that it is
        directly a response to the query, or a CNAME/DNAME for it.
    3c. Otherwise, remove the RRset from the unvalidated answer list.

Note that we the too loops in 2 + 3 are actually coded as a single one,
but the dnskeys_finalized bool indicates which loop we are currently
processing.

Note that loop 2 does not drop any invalidated RRsets yet, that's
something only loop 3 does. This is because loop 2 might still encounter
additional DNSKEYS which might validate more stuff, and if we'd already
have dropped those RRsets we couldn't validate those anymore. The first
loop is hence a "constructive" loop, the second loop a "destructive"
one: the first one validates whatever is possible, the second one then
deletes whatever still isn't.

1 files changed, 80 insertions, 58 deletions

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index 7e8f4d888b..8b3e59a1ea 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -1280,10 +1280,59 @@ void dns_transaction_notify(DnsTransaction *t, DnsTransaction *source) {
                 dns_transaction_process_dnssec(t);
 }
 
+static int dns_transaction_is_primary_response(DnsTransaction *t, DnsResourceRecord *rr) {
+        int r;
+
+        assert(t);
+        assert(rr);
+
+        /* Check if the specified RR is the "primary" response,
+         * i.e. either matches the question precisely or is a
+         * CNAME/DNAME for it */
+
+        r = dns_resource_key_match_rr(t->key, rr, NULL);
+        if (r != 0)
+                return r;
+
+        r = dns_resource_key_match_cname_or_dname(t->key, rr->key, NULL);
+        if (r != 0)
+                return r;
+
+        return 0;
+}
+
+static int dns_transaction_validate_dnskey_by_ds(DnsTransaction *t) {
+        DnsResourceRecord *rr;
+        int ifindex, r;
+
+        assert(t);
+
+        /* Add all DNSKEY RRs from the answer that are validated by DS
+         * RRs from the list of validated keys to the lis of validated
+         * keys. */
+
+        DNS_ANSWER_FOREACH_IFINDEX(rr, ifindex, t->answer) {
+
+                r = dnssec_verify_dnskey_search(rr, t->validated_keys);
+                if (r < 0)
+                        return r;
+                if (r == 0)
+                        continue;
+
+                /* If so, the DNSKEY is validated too. */
+                r = dns_answer_add_extend(&t->validated_keys, rr, ifindex);
+                if (r < 0)
+                        return r;
+        }
+
+        return 0;
+}
+
 int dns_transaction_validate_dnssec(DnsTransaction *t) {
         _cleanup_(dns_answer_unrefp) DnsAnswer *validated = NULL;
+        bool dnskeys_finalized = false;
         DnsResourceRecord *rr;
-        int ifindex, r;
+        int r;
 
         assert(t);
 
@@ -1312,22 +1361,12 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
         }
 
         /* First see if there are DNSKEYs we already known a validated DS for. */
-        DNS_ANSWER_FOREACH_IFINDEX(rr, ifindex, t->answer) {
-
-                r = dnssec_verify_dnskey_search(rr, t->validated_keys);
-                if (r < 0)
-                        return r;
-                if (r == 0)
-                        continue;
-
-                /* If so, the DNSKEY is validated too. */
-                r = dns_answer_add_extend(&t->validated_keys, rr, ifindex);
-                if (r < 0)
-                        return r;
-        }
+        r = dns_transaction_validate_dnskey_by_ds(t);
+        if (r < 0)
+                return r;
 
         for (;;) {
-                bool changed = false, missing_key_for_transaction = false;
+                bool changed = false;
 
                 DNS_ANSWER_FOREACH(rr, t->answer) {
                         DnssecResult result;
@@ -1346,9 +1385,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
                                 log_debug("Looking at %s: %s", rrs ? strstrip(rrs) : "???", dnssec_result_to_string(result));
                         }
 
-                        switch (result) {
-
-                        case DNSSEC_VALIDATED:
+                        if (result == DNSSEC_VALIDATED) {
 
                                 /* Add the validated RRset to the new list of validated RRsets */
                                 r = dns_answer_copy_by_key(&validated, t->answer, rr->key);
@@ -1372,71 +1409,56 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
                                 if (r < 0)
                                         return r;
 
+                                /* Exit the loop, we dropped something from the answer, start from the beginning */
                                 changed = true;
                                 break;
 
-                        case DNSSEC_INVALID:
-                        case DNSSEC_NO_SIGNATURE:
-                        case DNSSEC_SIGNATURE_EXPIRED:
-
-                                /* Is this the RRset that we were looking for? If so, this is fatal for the whole transaction */
-                                r = dns_resource_key_match_rr(t->key, rr, NULL);
-                                if (r < 0)
-                                        return r;
-                                if (r > 0) {
-                                        t->dnssec_result = result;
-                                        return 0;
-                                }
+                        } else if (dnskeys_finalized) {
+                                /* If we haven't read all DNSKEYs yet
+                                 * a negative result of the validation
+                                 * is irrelevant, as there might be
+                                 * more DNSKEYs coming. */
 
-                                /* Is this a CNAME for a record we were looking for? If so, it's also fatal for the whole transaction */
-                                r = dns_resource_key_match_cname_or_dname(t->key, rr->key, NULL);
+                                r = dns_transaction_is_primary_response(t, rr);
                                 if (r < 0)
                                         return r;
                                 if (r > 0) {
+                                        /* This is a primary response
+                                         * to our question, and it
+                                         * failed validation. That's
+                                         * fatal. */
                                         t->dnssec_result = result;
                                         return 0;
                                 }
 
-                                /* This is just something auxiliary. Just remove the RRset and continue. */
+                                /* This is just some auxiliary
+                                 * data. Just remove the RRset and
+                                 * continue. */
                                 r = dns_answer_remove_by_key(&t->answer, rr->key);
                                 if (r < 0)
                                         return r;
 
+                                /* Exit the loop, we dropped something from the answer, start from the beginning */
                                 changed = true;
                                 break;
-
-                        case DNSSEC_MISSING_KEY:
-                                /* They key is missing? Let's continue
-                                 * with the next iteration, maybe
-                                 * we'll find it in an DNSKEY RRset
-                                 * later on. */
-
-                                r = dns_resource_key_equal(rr->key, t->key);
-                                if (r < 0)
-                                        return r;
-                                if (r > 0)
-                                        missing_key_for_transaction = true;
-
-                                break;
-
-                        default:
-                                assert_not_reached("Unexpected DNSSEC result");
                         }
-
-                        if (changed)
-                                break;
                 }
 
                 if (changed)
                         continue;
 
-                /* This didn't work either, there's no point in
-                 * continuing. */
-                if (missing_key_for_transaction) {
-                        t->dnssec_result = DNSSEC_MISSING_KEY;
-                        return 0;
+                if (!dnskeys_finalized) {
+                        /* OK, now we know we have added all DNSKEYs
+                         * we possibly could to our validated
+                         * list. Now run the whole thing once more,
+                         * and strip everything we still cannot
+                         * validate.
+                         */
+                        dnskeys_finalized = true;
+                        continue;
                 }
 
+                /* We're done */
                 break;
         }