Merge pull request #2096 from teg/resolved-cache

Misc resolved cache fixes

9 files changed, 174 insertions, 84 deletions

diff --git a/TODO b/TODO
index 9ce915e35f..369805fcee 100644
--- a/TODO
+++ b/TODO
@@ -183,6 +183,7 @@ Features:
     we always process them before we process client requests
   - DNSSEC
         - add display of private key types (http://tools.ietf.org/html/rfc4034#appendix-A.1.1)?
+        - synthesize negative cache entries from NSEC/NSEC3 and drop explicit negative caching of authenticated answers
   - mDNS/DNS-SD
         - mDNS RR resolving
         - service registration
diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c
index de8c4d9dd3..55e6ffbad7 100644
--- a/src/resolve/resolved-dns-answer.c
+++ b/src/resolve/resolved-dns-answer.c
@@ -249,6 +249,29 @@ int dns_answer_find_soa(DnsAnswer *a, const DnsResourceKey *key, DnsResourceReco
         return 0;
 }
 
+int dns_answer_find_cname_or_dname(DnsAnswer *a, const DnsResourceKey *key, DnsResourceRecord **ret) {
+        DnsResourceRecord *rr;
+
+        assert(key);
+
+        if (!a)
+                return 0;
+
+        /* For a {C,D}NAME record we can never find a matching {C,D}NAME record */
+        if (key->type == DNS_TYPE_CNAME || key->type == DNS_TYPE_DNAME)
+                return 0;
+
+        DNS_ANSWER_FOREACH(rr, a) {
+                if (dns_resource_key_match_cname_or_dname(key, rr->key, NULL)) {
+                        if (ret)
+                                *ret = rr;
+                        return 1;
+                }
+        }
+
+        return 0;
+}
+
 int dns_answer_merge(DnsAnswer *a, DnsAnswer *b, DnsAnswer **ret) {
         _cleanup_(dns_answer_unrefp) DnsAnswer *k = NULL;
         int r;
diff --git a/src/resolve/resolved-dns-answer.h b/src/resolve/resolved-dns-answer.h
index 8d95131dbe..56b462ed7e 100644
--- a/src/resolve/resolved-dns-answer.h
+++ b/src/resolve/resolved-dns-answer.h
@@ -57,6 +57,7 @@ int dns_answer_match_key(DnsAnswer *a, const DnsResourceKey *key);
 int dns_answer_contains_rr(DnsAnswer *a, DnsResourceRecord *rr);
 
 int dns_answer_find_soa(DnsAnswer *a, const DnsResourceKey *key, DnsResourceRecord **ret);
+int dns_answer_find_cname_or_dname(DnsAnswer *a, const DnsResourceKey *key, DnsResourceRecord **ret);
 
 int dns_answer_merge(DnsAnswer *a, DnsAnswer *b, DnsAnswer **ret);
 int dns_answer_extend(DnsAnswer **a, DnsAnswer *b);
diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c
index 4aacc268e2..9ab44400bd 100644
--- a/src/resolve/resolved-dns-cache.c
+++ b/src/resolve/resolved-dns-cache.c
@@ -21,6 +21,7 @@
 
 #include "alloc-util.h"
 #include "dns-domain.h"
+#include "resolved-dns-answer.h"
 #include "resolved-dns-cache.h"
 #include "resolved-dns-packet.h"
 #include "string-util.h"
@@ -275,7 +276,7 @@ static int dns_cache_put_positive(
         _cleanup_(dns_cache_item_freep) DnsCacheItem *i = NULL;
         _cleanup_free_ char *key_str = NULL;
         DnsCacheItem *existing;
-        int r;
+        int r, k;
 
         assert(c);
         assert(rr);
@@ -283,14 +284,18 @@ static int dns_cache_put_positive(
 
         /* New TTL is 0? Delete the entry... */
         if (rr->ttl <= 0) {
-                r = dns_resource_key_to_string(rr->key, &key_str);
-                if (r < 0)
-                        return r;
+                k = dns_cache_remove(c, rr->key);
 
-                if (dns_cache_remove(c, rr->key))
-                        log_debug("Removed zero TTL entry from cache: %s", key_str);
-                else
-                        log_debug("Not caching zero TTL cache entry: %s", key_str);
+                if (log_get_max_level() >= LOG_DEBUG) {
+                        r = dns_resource_key_to_string(rr->key, &key_str);
+                        if (r < 0)
+                                return r;
+
+                        if (k > 0)
+                                log_debug("Removed zero TTL entry from cache: %s", key_str);
+                        else
+                                log_debug("Not caching zero TTL cache entry: %s", key_str);
+                }
 
                 return 0;
         }
@@ -331,11 +336,13 @@ static int dns_cache_put_positive(
         if (r < 0)
                 return r;
 
-        r = dns_resource_key_to_string(i->key, &key_str);
-        if (r < 0)
-                return r;
+        if (log_get_max_level() >= LOG_DEBUG) {
+                r = dns_resource_key_to_string(i->key, &key_str);
+                if (r < 0)
+                        return r;
 
-        log_debug("Added cache entry for %s", key_str);
+                log_debug("Added cache entry for %s", key_str);
+        }
 
         i = NULL;
         return 0;
@@ -364,13 +371,17 @@ static int dns_cache_put_negative(
         if (key->class == DNS_CLASS_ANY)
                 return 0;
         if (key->type == DNS_TYPE_ANY)
+                /* This is particularly important to filter out as we use this as a
+                 * pseudo-type for NXDOMAIN entries */
                 return 0;
         if (soa_ttl <= 0) {
-                r = dns_resource_key_to_string(key, &key_str);
-                if (r < 0)
-                        return r;
+                if (log_get_max_level() >= LOG_DEBUG) {
+                        r = dns_resource_key_to_string(key, &key_str);
+                        if (r < 0)
+                                return r;
 
-                log_debug("Not caching negative entry with zero SOA TTL: %s", key_str);
+                        log_debug("Not caching negative entry with zero SOA TTL: %s", key_str);
+                }
 
                 return 0;
         }
@@ -389,22 +400,32 @@ static int dns_cache_put_negative(
                 return -ENOMEM;
 
         i->type = rcode == DNS_RCODE_SUCCESS ? DNS_CACHE_NODATA : DNS_CACHE_NXDOMAIN;
-        i->key = dns_resource_key_ref(key);
         i->until = timestamp + MIN(soa_ttl * USEC_PER_SEC, CACHE_TTL_MAX_USEC);
         i->prioq_idx = PRIOQ_IDX_NULL;
         i->owner_family = owner_family;
         i->owner_address = *owner_address;
         i->authenticated = authenticated;
 
+        if (i->type == DNS_CACHE_NXDOMAIN) {
+                /* NXDOMAIN entries should apply equally to all types, so we use ANY as
+                 * a pseudo type for this purpose here. */
+                i->key = dns_resource_key_new(key->class, DNS_TYPE_ANY, DNS_RESOURCE_KEY_NAME(key));
+                if (!i->key)
+                        return -ENOMEM;
+        } else
+                i->key = dns_resource_key_ref(key);
+
         r = dns_cache_link_item(c, i);
         if (r < 0)
                 return r;
 
-        r = dns_resource_key_to_string(i->key, &key_str);
-        if (r < 0)
-                return r;
+        if (log_get_max_level() >= LOG_DEBUG) {
+                r = dns_resource_key_to_string(i->key, &key_str);
+                if (r < 0)
+                        return r;
 
-        log_debug("Added %s cache entry for %s", i->type == DNS_CACHE_NODATA ? "NODATA" : "NXDOMAIN", key_str);
+                log_debug("Added %s cache entry for %s", i->type == DNS_CACHE_NODATA ? "NODATA" : "NXDOMAIN", key_str);
+        }
 
         i = NULL;
         return 0;
@@ -421,7 +442,7 @@ int dns_cache_put(
                 int owner_family,
                 const union in_addr_union *owner_address) {
 
-        DnsResourceRecord *soa = NULL;
+        DnsResourceRecord *soa = NULL, *rr;
         unsigned cache_keys, i;
         int r;
 
@@ -433,11 +454,23 @@ int dns_cache_put(
                 dns_cache_remove(c, key);
         }
 
-        if (!answer)
+        if (!answer) {
+                if (log_get_max_level() >= LOG_DEBUG) {
+                        _cleanup_free_ char *key_str = NULL;
+
+                        r = dns_resource_key_to_string(key, &key_str);
+                        if (r < 0)
+                                return r;
+
+                        log_debug("Not caching negative entry without a SOA record: %s", key_str);
+                }
+
                 return 0;
+        }
 
-        for (i = 0; i < answer->n_rrs; i++)
-                dns_cache_remove(c, answer->items[i].rr->key);
+        DNS_ANSWER_FOREACH(rr, answer)
+                if (rr->key->cache_flush)
+                        dns_cache_remove(c, rr->key);
 
         /* We only care for positive replies and NXDOMAINs, on all
          * other replies we will simply flush the respective entries,
@@ -459,10 +492,7 @@ int dns_cache_put(
 
         /* Second, add in positive entries for all contained RRs */
         for (i = 0; i < MIN(max_rrs, answer->n_rrs); i++) {
-                DnsResourceRecord *rr = answer->items[i].rr;
-
-                if (rr->key->cache_flush)
-                        dns_cache_remove(c, rr->key);
+                rr = answer->items[i].rr;
 
                 r = dns_cache_put_positive(c, rr, authenticated, timestamp, owner_family, owner_address);
                 if (r < 0)
@@ -479,6 +509,15 @@ int dns_cache_put(
         if (r > 0)
                 return 0;
 
+        /* But not if it has a matching CNAME/DNAME (the negative
+         * caching will be done on the canonical name, not on the
+         * alias) */
+        r = dns_answer_find_cname_or_dname(answer, key, NULL);
+        if (r < 0)
+                goto fail;
+        if (r > 0)
+                return 0;
+
         /* See https://tools.ietf.org/html/rfc2308, which say that a
          * matching SOA record in the packet is used to to enable
          * negative caching. */
@@ -520,36 +559,50 @@ static DnsCacheItem *dns_cache_get_by_key_follow_cname_dname_nsec(DnsCache *c, D
          * much, after all this is just a cache */
 
         i = hashmap_get(c->by_key, k);
-        if (i || IN_SET(k->type, DNS_TYPE_CNAME, DNS_TYPE_DNAME, DNS_TYPE_NSEC))
+        if (i)
                 return i;
 
         n = DNS_RESOURCE_KEY_NAME(k);
 
-        /* Check if we have an NSEC record instead for the name. */
-        i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_NSEC, n));
-        if (i)
+        /* Check if we have an NXDOMAIN cache item for the name, notice that we use
+         * the pseudo-type ANY for NXDOMAIN cache items. */
+        i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_ANY, n));
+        if (i && i->type == DNS_CACHE_NXDOMAIN)
                 return i;
 
-        /* Check if we have a CNAME record instead */
-        i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_CNAME, n));
-        if (i)
-                return i;
+        /* The following record types should never be redirected. See
+         * <https://tools.ietf.org/html/rfc4035#section-2.5>. */
+        if (!IN_SET(k->type, DNS_TYPE_CNAME, DNS_TYPE_DNAME,
+                            DNS_TYPE_NSEC3, DNS_TYPE_NSEC, DNS_TYPE_RRSIG,
+                            DNS_TYPE_NXT, DNS_TYPE_SIG, DNS_TYPE_KEY)) {
+                /* Check if we have a CNAME record instead */
+                i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_CNAME, n));
+                if (i)
+                        return i;
 
-        /* OK, let's look for cached DNAME records. */
-        for (;;) {
-                char label[DNS_LABEL_MAX];
+                /* OK, let's look for cached DNAME records. */
+                for (;;) {
+                        char label[DNS_LABEL_MAX];
+
+                        if (isempty(n))
+                                return NULL;
+
+                        i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_DNAME, n));
+                        if (i)
+                                return i;
 
-                if (isempty(n))
-                        return NULL;
+                        /* Jump one label ahead */
+                        r = dns_label_unescape(&n, label, sizeof(label));
+                        if (r <= 0)
+                                return NULL;
+                }
+        }
 
-                i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_DNAME, n));
+        if (k-> type != DNS_TYPE_NSEC) {
+                /* Check if we have an NSEC record instead for the name. */
+                i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_NSEC, n));
                 if (i)
                         return i;
-
-                /* Jump one label ahead */
-                r = dns_label_unescape(&n, label, sizeof(label));
-                if (r <= 0)
-                        return NULL;
         }
 
         return NULL;
@@ -576,11 +629,13 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r
                 /* If we have ANY lookups we don't use the cache, so
                  * that the caller refreshes via the network. */
 
-                r = dns_resource_key_to_string(key, &key_str);
-                if (r < 0)
-                        return r;
+                if (log_get_max_level() >= LOG_DEBUG) {
+                        r = dns_resource_key_to_string(key, &key_str);
+                        if (r < 0)
+                                return r;
 
-                log_debug("Ignoring cache for ANY lookup: %s", key_str);
+                        log_debug("Ignoring cache for ANY lookup: %s", key_str);
+                }
 
                 *ret = NULL;
                 *rcode = DNS_RCODE_SUCCESS;
@@ -591,11 +646,13 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r
         if (!first) {
                 /* If one question cannot be answered we need to refresh */
 
-                r = dns_resource_key_to_string(key, &key_str);
-                if (r < 0)
-                        return r;
+                if (log_get_max_level() >= LOG_DEBUG) {
+                        r = dns_resource_key_to_string(key, &key_str);
+                        if (r < 0)
+                                return r;
 
-                log_debug("Cache miss for %s", key_str);
+                        log_debug("Cache miss for %s", key_str);
+                }
 
                 *ret = NULL;
                 *rcode = DNS_RCODE_SUCCESS;
@@ -617,15 +674,17 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r
                         have_non_authenticated = true;
         }
 
-        r = dns_resource_key_to_string(key, &key_str);
-        if (r < 0)
-                return r;
-
         if (nsec && key->type != DNS_TYPE_NSEC) {
-                log_debug("NSEC NODATA cache hit for %s", key_str);
+                if (log_get_max_level() >= LOG_DEBUG) {
+                        r = dns_resource_key_to_string(key, &key_str);
+                        if (r < 0)
+                                return r;
+
+                        log_debug("NSEC NODATA cache hit for %s", key_str);
+                }
 
                 /* We only found an NSEC record that matches our name.
-                 * If it says the type doesn't exit report
+                 * If it says the type doesn't exist report
                  * NODATA. Otherwise report a cache miss. */
 
                 *ret = NULL;
@@ -637,10 +696,16 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r
                        !bitmap_isset(nsec->rr->nsec.types, DNS_TYPE_DNAME);
         }
 
-        log_debug("%s cache hit for %s",
-                  n > 0    ? "Positive" :
-                  nxdomain ? "NXDOMAIN" : "NODATA",
-                  key_str);
+        if (log_get_max_level() >= LOG_DEBUG) {
+                r = dns_resource_key_to_string(key, &key_str);
+                if (r < 0)
+                        return r;
+
+                log_debug("%s cache hit for %s",
+                          n > 0    ? "Positive" :
+                          nxdomain ? "NXDOMAIN" : "NODATA",
+                          key_str);
+        }
 
         if (n <= 0) {
                 *ret = NULL;
diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c
index 399ba59749..e90500ce70 100644
--- a/src/resolve/resolved-dns-packet.c
+++ b/src/resolve/resolved-dns-packet.c
@@ -1451,7 +1451,7 @@ fail:
 
 int dns_packet_read_key(DnsPacket *p, DnsResourceKey **ret, size_t *start) {
         _cleanup_free_ char *name = NULL;
-        bool cache_flush = false;
+        bool cache_flush = true;
         uint16_t class, type;
         DnsResourceKey *key;
         size_t saved_rindex;
@@ -1477,10 +1477,10 @@ int dns_packet_read_key(DnsPacket *p, DnsResourceKey **ret, size_t *start) {
         if (p->protocol == DNS_PROTOCOL_MDNS) {
                 /* See RFC6762, Section 10.2 */
 
-                if (class & MDNS_RR_CACHE_FLUSH) {
+                if (class & MDNS_RR_CACHE_FLUSH)
                         class &= ~MDNS_RR_CACHE_FLUSH;
-                        cache_flush = true;
-                }
+                else
+                        cache_flush = false;
         }
 
         key = dns_resource_key_new_consume(class, type, name);
diff --git a/src/resolve/resolved-dns-question.c b/src/resolve/resolved-dns-question.c
index 3249448d3b..4ed7434d3c 100644
--- a/src/resolve/resolved-dns-question.c
+++ b/src/resolve/resolved-dns-question.c
@@ -117,7 +117,7 @@ int dns_question_matches_cname(DnsQuestion *q, DnsResourceRecord *rr, const char
                 return 0;
 
         for (i = 0; i < q->n_keys; i++) {
-                r = dns_resource_key_match_cname(q->keys[i], rr, search_domain);
+                r = dns_resource_key_match_cname_or_dname(q->keys[i], rr->key, search_domain);
                 if (r != 0)
                         return r;
         }
diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c
index 55e85eec2b..74c9d87319 100644
--- a/src/resolve/resolved-dns-rr.c
+++ b/src/resolve/resolved-dns-rr.c
@@ -220,19 +220,19 @@ int dns_resource_key_match_rr(const DnsResourceKey *key, const DnsResourceRecord
         return 0;
 }
 
-int dns_resource_key_match_cname(const DnsResourceKey *key, const DnsResourceRecord *rr, const char *search_domain) {
+int dns_resource_key_match_cname_or_dname(const DnsResourceKey *key, const DnsResourceKey *cname, const char *search_domain) {
         int r;
 
         assert(key);
-        assert(rr);
+        assert(cname);
 
-        if (rr->key->class != key->class && key->class != DNS_CLASS_ANY)
+        if (cname->class != key->class && key->class != DNS_CLASS_ANY)
                 return 0;
 
-        if (rr->key->type == DNS_TYPE_CNAME)
-                r = dns_name_equal(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(rr->key));
-        else if (rr->key->type == DNS_TYPE_DNAME)
-                r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(rr->key));
+        if (cname->type == DNS_TYPE_CNAME)
+                r = dns_name_equal(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(cname));
+        else if (cname->type == DNS_TYPE_DNAME)
+                r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(cname));
         else
                 return 0;
 
@@ -246,10 +246,10 @@ int dns_resource_key_match_cname(const DnsResourceKey *key, const DnsResourceRec
                 if (r < 0)
                         return r;
 
-                if (rr->key->type == DNS_TYPE_CNAME)
-                        return dns_name_equal(joined, DNS_RESOURCE_KEY_NAME(rr->key));
-                else if (rr->key->type == DNS_TYPE_DNAME)
-                        return dns_name_endswith(joined, DNS_RESOURCE_KEY_NAME(rr->key));
+                if (cname->type == DNS_TYPE_CNAME)
+                        return dns_name_equal(joined, DNS_RESOURCE_KEY_NAME(cname));
+                else if (cname->type == DNS_TYPE_DNAME)
+                        return dns_name_endswith(joined, DNS_RESOURCE_KEY_NAME(cname));
         }
 
         return 0;
diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h
index 4c0f72eea3..632ee59994 100644
--- a/src/resolve/resolved-dns-rr.h
+++ b/src/resolve/resolved-dns-rr.h
@@ -246,7 +246,7 @@ DnsResourceKey* dns_resource_key_unref(DnsResourceKey *key);
 bool dns_resource_key_is_address(const DnsResourceKey *key);
 int dns_resource_key_equal(const DnsResourceKey *a, const DnsResourceKey *b);
 int dns_resource_key_match_rr(const DnsResourceKey *key, const DnsResourceRecord *rr, const char *search_domain);
-int dns_resource_key_match_cname(const DnsResourceKey *key, const DnsResourceRecord *rr, const char *search_domain);
+int dns_resource_key_match_cname_or_dname(const DnsResourceKey *key, const DnsResourceKey *cname, const char *search_domain);
 int dns_resource_key_match_soa(const DnsResourceKey *key, const DnsResourceKey *soa);
 int dns_resource_key_to_string(const DnsResourceKey *key, char **ret);
 DEFINE_TRIVIAL_CLEANUP_FUNC(DnsResourceKey*, dns_resource_key_unref);
diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index c03aa5d287..bcf6d5c810 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -1389,7 +1389,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
                                 }
 
                                 /* 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(t->key, rr, NULL);
+                                r = dns_resource_key_match_cname_or_dname(t->key, rr->key, NULL);
                                 if (r < 0)
                                         return r;
                                 if (r > 0) {