resolved: fix how we detect whether auxiliary DNSSEC transactions are ready

Previously, when getting notified about a completed auxiliary DNSSEC transaction we'd immediately act on it, and
possibly abort the main transaction. This is problematic, as DNS transactions that already completed at the time we
started using them will never get the notification event, and hence never be acted on in the same way.

Hence, introduce a new call dns_transaction_dnssec_ready() that checks the state of auxiliary DNSSEC transactions, and
returns 1 when we are ready for the actual DNSSEC validation step. Then, make sure this is invoked when the auxiliary
transactions are first acquired (and thus possibly reused) as well when the notifications explained above take place.

This fixes problems particularly when doing combined A and AAAA lookups  where the auxiliary DNSSEC transactions get
reused between them, and where we got confused if we reused an auxiliary DNSSEC transaction from one when it already
got completed from the other.

1 files changed, 100 insertions, 61 deletions

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index 393171a2ad..434eab53e7 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -556,13 +556,76 @@ static bool dns_transaction_dnssec_is_live(DnsTransaction *t) {
         return false;
 }
 
+static int dns_transaction_dnssec_ready(DnsTransaction *t) {
+        DnsTransaction *dt;
+        Iterator i;
+
+        assert(t);
+
+        /* Checks whether the auxiliary DNSSEC transactions of our transaction have completed, or are still
+         * ongoing. Returns 0, if we aren't ready for the DNSSEC validation, positive if we are. */
+
+        SET_FOREACH(dt, t->dnssec_transactions, i) {
+
+                switch (dt->state) {
+
+                case DNS_TRANSACTION_NULL:
+                case DNS_TRANSACTION_PENDING:
+                case DNS_TRANSACTION_VALIDATING:
+                        /* Still ongoing */
+                        return 0;
+
+                case DNS_TRANSACTION_RCODE_FAILURE:
+                        if (dt->answer_rcode != DNS_RCODE_NXDOMAIN) {
+                                log_debug("Auxiliary DNSSEC RR query failed with rcode=%s.", dns_rcode_to_string(dt->answer_rcode));
+                                goto fail;
+                        }
+
+                        /* Fall-through: NXDOMAIN is good enough for us. This is because some DNS servers erronously
+                         * return NXDOMAIN for empty non-terminals (Akamai...), and we need to handle that nicely, when
+                         * asking for parent SOA or similar RRs to make unsigned proofs. */
+
+                case DNS_TRANSACTION_SUCCESS:
+                        /* All good. */
+                        break;
+
+                case DNS_TRANSACTION_DNSSEC_FAILED:
+                        /* We handle DNSSEC failures different from other errors, as we care about the DNSSEC
+                         * validationr result */
+
+                        log_debug("Auxiliary DNSSEC RR query failed validation: %s", dnssec_result_to_string(dt->answer_dnssec_result));
+                        t->answer_dnssec_result = dt->answer_dnssec_result; /* Copy error code over */
+                        dns_transaction_complete(t, DNS_TRANSACTION_DNSSEC_FAILED);
+                        return 0;
+
+
+                default:
+                        log_debug("Auxiliary DNSSEC RR query failed with %s", dns_transaction_state_to_string(dt->state));
+                        goto fail;
+                }
+        }
+
+        /* All is ready, we can go and validate */
+        return 1;
+
+fail:
+        t->answer_dnssec_result = DNSSEC_FAILED_AUXILIARY;
+        dns_transaction_complete(t, DNS_TRANSACTION_DNSSEC_FAILED);
+        return 0;
+}
+
 static void dns_transaction_process_dnssec(DnsTransaction *t) {
         int r;
 
         assert(t);
 
         /* Are there ongoing DNSSEC transactions? If so, let's wait for them. */
-        if (dns_transaction_dnssec_is_live(t))
+        r = dns_transaction_dnssec_ready(t);
+        if (r < 0) {
+                dns_transaction_complete(t, DNS_TRANSACTION_RESOURCES);
+                return;
+        }
+        if (r == 0) /* We aren't ready yet (or one of our auxiliary transactions failed, and we shouldn't validate now */
                 return;
 
         /* See if we learnt things from the additional DNSSEC transactions, that we didn't know before, and better
@@ -1932,69 +1995,15 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) {
 }
 
 void dns_transaction_notify(DnsTransaction *t, DnsTransaction *source) {
-        int r;
-
         assert(t);
         assert(source);
 
-        if (!IN_SET(t->state, DNS_TRANSACTION_PENDING, DNS_TRANSACTION_VALIDATING))
-                return;
+        /* Invoked whenever any of our auxiliary DNSSEC transactions completed its work. If the state is still PENDING,
+           we are still in the loop that adds further DNSSEC transactions, hence don't check if we are ready yet. If
+           the state is VALIDATING however, we should check if we are complete now. */
 
-        /* Invoked whenever any of our auxiliary DNSSEC transactions
-           completed its work. We copy any RRs from that transaction
-           over into our list of validated keys -- but only if the
-           answer is authenticated.
-
-           Note that we fail our transaction if the auxiliary
-           transaction failed, except on NXDOMAIN. This is because
-           some broken DNS servers (Akamai...) will return NXDOMAIN
-           for empty non-terminals. */
-
-        switch (source->state) {
-
-        case DNS_TRANSACTION_DNSSEC_FAILED:
-
-                log_debug("Auxiliary DNSSEC RR query failed validation: %s", dnssec_result_to_string(source->answer_dnssec_result));
-                t->answer_dnssec_result = source->answer_dnssec_result; /* Copy error code over */
-                dns_transaction_complete(t, DNS_TRANSACTION_DNSSEC_FAILED);
-                break;
-
-        case DNS_TRANSACTION_RCODE_FAILURE:
-
-                if (source->answer_rcode != DNS_RCODE_NXDOMAIN) {
-                        log_debug("Auxiliary DNSSEC RR query failed with rcode=%i.", source->answer_rcode);
-                        goto fail;
-                }
-
-                /* fall-through: NXDOMAIN is good enough for us */
-
-        case DNS_TRANSACTION_SUCCESS:
-                if (source->answer_authenticated) {
-                        r = dns_answer_extend(&t->validated_keys, source->answer);
-                        if (r < 0) {
-                                log_error_errno(r, "Failed to merge validated DNSSEC key data: %m");
-                                goto fail;
-                        }
-                }
-
-                /* If the state is still PENDING, we are still in the loop
-                 * that adds further DNSSEC transactions, hence don't check if
-                 * we are ready yet. If the state is VALIDATING however, we
-                 * should check if we are complete now. */
-                if (t->state == DNS_TRANSACTION_VALIDATING)
-                        dns_transaction_process_dnssec(t);
-                break;
-
-        default:
-                log_debug("Auxiliary DNSSEC RR query failed with %s", dns_transaction_state_to_string(source->state));
-                goto fail;
-        }
-
-        return;
-
-fail:
-        t->answer_dnssec_result = DNSSEC_FAILED_AUXILIARY;
-        dns_transaction_complete(t, DNS_TRANSACTION_DNSSEC_FAILED);
+        if (t->state == DNS_TRANSACTION_VALIDATING)
+                dns_transaction_process_dnssec(t);
 }
 
 static int dns_transaction_validate_dnskey_by_ds(DnsTransaction *t) {
@@ -2437,6 +2446,31 @@ static int dns_transaction_invalidate_revoked_keys(DnsTransaction *t) {
         return 0;
 }
 
+static int dns_transaction_copy_validated(DnsTransaction *t) {
+        DnsTransaction *dt;
+        Iterator i;
+        int r;
+
+        assert(t);
+
+        /* Copy all validated RRs from the auxiliary DNSSEC transactions into our set of validated RRs */
+
+        SET_FOREACH(dt, t->dnssec_transactions, i) {
+
+                if (DNS_TRANSACTION_IS_LIVE(dt->state))
+                        continue;
+
+                if (!dt->answer_authenticated)
+                        continue;
+
+                r = dns_answer_extend(&t->validated_keys, dt->answer);
+                if (r < 0)
+                        return r;
+        }
+
+        return 0;
+}
+
 int dns_transaction_validate_dnssec(DnsTransaction *t) {
         _cleanup_(dns_answer_unrefp) DnsAnswer *validated = NULL;
         enum {
@@ -2487,13 +2521,18 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
         if (r < 0)
                 return r;
 
+        /* Third, copy all RRs we acquired successfully from auxiliary RRs over. */
+        r = dns_transaction_copy_validated(t);
+        if (r < 0)
+                return r;
+
         /* Second, see if there are DNSKEYs we already know a
          * validated DS for. */
         r = dns_transaction_validate_dnskey_by_ds(t);
         if (r < 0)
                 return r;
 
-        /* Third, remove all DNSKEY and DS RRs again that our trust
+        /* Fourth, remove all DNSKEY and DS RRs again that our trust
          * anchor says are revoked. After all we might have marked
          * some keys revoked above, but they might still be lingering
          * in our validated_keys list. */