Merge pull request #1279 from teg/resolved-cname-cache

resolevd: cache - improve CNAME handling

12 files changed, 122 insertions, 58 deletions

diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c
index 12c17003e9..bf1b7c8ab4 100644
--- a/src/resolve/resolved-bus.c
+++ b/src/resolve/resolved-bus.c
@@ -191,7 +191,7 @@ static void bus_method_resolve_hostname_complete(DnsQuery *q) {
 
                 /* This has a cname? Then update the query with the
                  * new cname. */
-                r = dns_query_cname_redirect(q, cname->cname.name);
+                r = dns_query_cname_redirect(q, cname);
                 if (r < 0) {
                         if (r == -ELOOP)
                                 r = sd_bus_reply_method_errorf(q->request, BUS_ERROR_CNAME_LOOP, "CNAME loop on '%s'", q->request_hostname);
@@ -220,8 +220,6 @@ static void bus_method_resolve_hostname_complete(DnsQuery *q) {
                         added++;
                 }
 
-                // what about the cache?
-
                 /* If we didn't find anything, then let's restart the
                  * query, this time with the cname */
                 if (added <= 0) {
diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c
index 13ad4ca6bd..89b9b0e1ea 100644
--- a/src/resolve/resolved-dns-answer.c
+++ b/src/resolve/resolved-dns-answer.c
@@ -149,6 +149,19 @@ int dns_answer_contains(DnsAnswer *a, DnsResourceKey *key) {
         return 0;
 }
 
+int dns_answer_match_soa(DnsResourceKey *key, DnsResourceKey *soa) {
+        if (soa->class != DNS_CLASS_IN)
+                return 0;
+
+        if (soa->type != DNS_TYPE_SOA)
+                return 0;
+
+        if (!dns_name_endswith(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(soa)))
+                return 0;
+
+        return 1;
+}
+
 int dns_answer_find_soa(DnsAnswer *a, DnsResourceKey *key, DnsResourceRecord **ret) {
         unsigned i;
 
@@ -164,13 +177,7 @@ int dns_answer_find_soa(DnsAnswer *a, DnsResourceKey *key, DnsResourceRecord **r
 
         for (i = 0; i < a->n_rrs; i++) {
 
-                if (a->items[i].rr->key->class != DNS_CLASS_IN)
-                        continue;
-
-                if (a->items[i].rr->key->type != DNS_TYPE_SOA)
-                        continue;
-
-                if (dns_name_endswith(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(a->items[i].rr->key))) {
+                if (dns_answer_match_soa(key, a->items[i].rr->key)) {
                         *ret = a->items[i].rr;
                         return 1;
                 }
diff --git a/src/resolve/resolved-dns-answer.h b/src/resolve/resolved-dns-answer.h
index 0757dd60d0..044d73b19c 100644
--- a/src/resolve/resolved-dns-answer.h
+++ b/src/resolve/resolved-dns-answer.h
@@ -49,6 +49,7 @@ DnsAnswer *dns_answer_unref(DnsAnswer *a);
 int dns_answer_add(DnsAnswer *a, DnsResourceRecord *rr, int ifindex);
 int dns_answer_add_soa(DnsAnswer *a, const char *name, uint32_t ttl);
 int dns_answer_contains(DnsAnswer *a, DnsResourceKey *key);
+int dns_answer_match_soa(DnsResourceKey *key, DnsResourceKey *soa);
 int dns_answer_find_soa(DnsAnswer *a, DnsResourceKey *key, DnsResourceRecord **ret);
 
 DnsAnswer *dns_answer_merge(DnsAnswer *a, DnsAnswer *b);
diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c
index cbbbed8c8a..ab13636bc1 100644
--- a/src/resolve/resolved-dns-cache.c
+++ b/src/resolve/resolved-dns-cache.c
@@ -277,13 +277,14 @@ static int dns_cache_put_positive(
 
         /* New TTL is 0? Delete the entry... */
         if (rr->ttl <= 0) {
-                if (dns_cache_remove(c, rr->key)) {
-                        r = dns_resource_key_to_string(rr->key, &key_str);
-                        if (r < 0)
-                                return r;
+                r = dns_resource_key_to_string(rr->key, &key_str);
+                if (r < 0)
+                        return r;
 
+                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);
 
                 return 0;
         }
@@ -361,7 +362,7 @@ static int dns_cache_put_negative(
                 if (r < 0)
                         return r;
 
-                log_debug("Ignored negative cache entry with zero SOA TTL: %s", key_str);
+                log_debug("Not caching negative entry with zero SOA TTL: %s", key_str);
 
                 return 0;
         }
@@ -402,7 +403,7 @@ static int dns_cache_put_negative(
 
 int dns_cache_put(
                 DnsCache *c,
-                DnsQuestion *q,
+                DnsResourceKey *key,
                 int rcode,
                 DnsAnswer *answer,
                 unsigned max_rrs,
@@ -410,16 +411,16 @@ int dns_cache_put(
                 int owner_family,
                 const union in_addr_union *owner_address) {
 
+        DnsResourceRecord *soa = NULL;
         unsigned cache_keys, i;
         int r;
 
         assert(c);
 
-        if (q) {
-                /* First, if we were passed a question, delete all matching old RRs,
+        if (key) {
+                /* First, if we were passed a key, delete all matching old RRs,
                  * so that we only keep complete by_key in place. */
-                for (i = 0; i < q->n_keys; i++)
-                        dns_cache_remove(c, q->keys[i]);
+                dns_cache_remove(c, key);
         }
 
         if (!answer)
@@ -437,8 +438,8 @@ int dns_cache_put(
 
         cache_keys = answer->n_rrs;
 
-        if (q)
-                cache_keys += q->n_keys;
+        if (key)
+                cache_keys ++;
 
         /* Make some space for our new entries */
         dns_cache_make_space(c, cache_keys);
@@ -453,44 +454,63 @@ int dns_cache_put(
                         goto fail;
         }
 
-        if (!q)
+        if (!key)
                 return 0;
 
-        /* Third, add in negative entries for all keys with no RR */
-        for (i = 0; i < q->n_keys; i++) {
-                DnsResourceRecord *soa = NULL;
+        /* Third, add in negative entries if the key has no RR */
+        r = dns_answer_contains(answer, key);
+        if (r < 0)
+                goto fail;
+        if (r > 0)
+                return 0;
 
-                r = dns_answer_contains(answer, q->keys[i]);
-                if (r < 0)
-                        goto fail;
-                if (r > 0)
-                        continue;
+        /* See https://tools.ietf.org/html/rfc2308, which
+         * say that a matching SOA record in the packet
+         * is used to to enable negative caching. */
 
-                /* See https://tools.ietf.org/html/rfc2308, which
-                 * say that a matching SOA record in the packet
-                 * is used to to enable negative caching. */
+        r = dns_answer_find_soa(answer, key, &soa);
+        if (r < 0)
+                goto fail;
+        if (r == 0)
+                return 0;
 
-                r = dns_answer_find_soa(answer, q->keys[i], &soa);
-                if (r < 0)
-                        goto fail;
-                if (r == 0)
-                        continue;
+        /* Also, if the requested key is an alias, the negative response should
+           be cached for each name in the redirect chain. Any CNAME record in
+           the response is from the redirection chain, though only the final one
+           is guaranteed to be included. This means that we cannot verify the
+           chain and that we need to cache them all as it may be incomplete. */
+        for (i = 0; i < answer->n_rrs; i++) {
+                DnsResourceRecord *answer_rr = answer->items[i].rr;
 
-                r = dns_cache_put_negative(c, q->keys[i], rcode, timestamp, MIN(soa->soa.minimum, soa->ttl), owner_family, owner_address);
-                if (r < 0)
-                        goto fail;
+                if (answer_rr->key->type == DNS_TYPE_CNAME) {
+                        _cleanup_(dns_resource_key_unrefp) DnsResourceKey *canonical_key = NULL;
+
+                        canonical_key = dns_resource_key_new_redirect(key, answer_rr);
+                        if (!canonical_key)
+                                goto fail;
+
+                        /* Let's not add negative cache entries for records outside the current zone. */
+                        if (!dns_answer_match_soa(canonical_key, soa->key))
+                                continue;
+
+                        r = dns_cache_put_negative(c, canonical_key, rcode, timestamp, MIN(soa->soa.minimum, soa->ttl), owner_family, owner_address);
+                        if (r < 0)
+                                goto fail;
+                }
         }
 
+        r = dns_cache_put_negative(c, key, rcode, timestamp, MIN(soa->soa.minimum, soa->ttl), owner_family, owner_address);
+        if (r < 0)
+                goto fail;
+
         return 0;
 
 fail:
         /* Adding all RRs failed. Let's clean up what we already
          * added, just in case */
 
-        if (q) {
-                for (i = 0; i < q->n_keys; i++)
-                        dns_cache_remove(c, q->keys[i]);
-        }
+        if (key)
+                dns_cache_remove(c, key);
 
         for (i = 0; i < answer->n_rrs; i++)
                 dns_cache_remove(c, answer->items[i].rr->key);
@@ -498,6 +518,29 @@ fail:
         return r;
 }
 
+static DnsCacheItem *dns_cache_get_by_key_follow_cname(DnsCache *c, DnsResourceKey *k) {
+        _cleanup_(dns_resource_key_unrefp) DnsResourceKey *cname_key = NULL;
+        DnsCacheItem *i, *j;
+
+        assert(c);
+        assert(k);
+
+        i = hashmap_get(c->by_key, k);
+        if (i || k->type == DNS_TYPE_CNAME)
+                return i;
+
+        /* check if we have a CNAME record instead */
+        cname_key = dns_resource_key_new_cname(k);
+        if (!cname_key)
+                return NULL;
+
+        j = hashmap_get(c->by_key, cname_key);
+        if (j)
+                return j;
+
+        return i;
+}
+
 int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **ret) {
         _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL;
         unsigned n = 0;
@@ -527,7 +570,7 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r
                 return 0;
         }
 
-        first = hashmap_get(c->by_key, key);
+        first = dns_cache_get_by_key_follow_cname(c, key);
         if (!first) {
                 /* If one question cannot be answered we need to refresh */
 
diff --git a/src/resolve/resolved-dns-cache.h b/src/resolve/resolved-dns-cache.h
index 1225e58de4..60cf6a4784 100644
--- a/src/resolve/resolved-dns-cache.h
+++ b/src/resolve/resolved-dns-cache.h
@@ -39,7 +39,7 @@ typedef struct DnsCache {
 void dns_cache_flush(DnsCache *c);
 void dns_cache_prune(DnsCache *c);
 
-int dns_cache_put(DnsCache *c, DnsQuestion *q, int rcode, DnsAnswer *answer, unsigned max_rrs, usec_t timestamp, int owner_family, const union in_addr_union *owner_address);
+int dns_cache_put(DnsCache *c, DnsResourceKey *key, int rcode, DnsAnswer *answer, unsigned max_rrs, usec_t timestamp, int owner_family, const union in_addr_union *owner_address);
 int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **answer);
 
 int dns_cache_check_conflicts(DnsCache *cache, DnsResourceRecord *rr, int owner_family, const union in_addr_union *owner_address);
diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c
index c0b4c8ba81..4b1d18b2ef 100644
--- a/src/resolve/resolved-dns-query.c
+++ b/src/resolve/resolved-dns-query.c
@@ -831,7 +831,7 @@ void dns_query_ready(DnsQuery *q) {
         dns_query_complete(q, state);
 }
 
-int dns_query_cname_redirect(DnsQuery *q, const char *name) {
+int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname) {
         _cleanup_(dns_question_unrefp) DnsQuestion *nq = NULL;
         int r;
 
@@ -840,7 +840,7 @@ int dns_query_cname_redirect(DnsQuery *q, const char *name) {
         if (q->n_cname_redirects > CNAME_MAX)
                 return -ELOOP;
 
-        r = dns_question_cname_redirect(q->question, name, &nq);
+        r = dns_question_cname_redirect(q->question, cname, &nq);
         if (r < 0)
                 return r;
 
diff --git a/src/resolve/resolved-dns-query.h b/src/resolve/resolved-dns-query.h
index 93d49301fa..e7063d9678 100644
--- a/src/resolve/resolved-dns-query.h
+++ b/src/resolve/resolved-dns-query.h
@@ -72,7 +72,7 @@ DnsQuery *dns_query_free(DnsQuery *q);
 int dns_query_go(DnsQuery *q);
 void dns_query_ready(DnsQuery *q);
 
-int dns_query_cname_redirect(DnsQuery *q, const char *name);
+int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname);
 
 int dns_query_bus_track(DnsQuery *q, sd_bus_message *m);
 
diff --git a/src/resolve/resolved-dns-question.c b/src/resolve/resolved-dns-question.c
index c94928d725..1507f22da0 100644
--- a/src/resolve/resolved-dns-question.c
+++ b/src/resolve/resolved-dns-question.c
@@ -242,13 +242,13 @@ int dns_question_is_equal(DnsQuestion *a, DnsQuestion *b) {
         return 1;
 }
 
-int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion **ret) {
+int dns_question_cname_redirect(DnsQuestion *q, const DnsResourceRecord *cname, DnsQuestion **ret) {
         _cleanup_(dns_question_unrefp) DnsQuestion *n = NULL;
         bool same = true;
         unsigned i;
         int r;
 
-        assert(name);
+        assert(cname);
         assert(ret);
 
         if (!q) {
@@ -262,7 +262,7 @@ int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion **
         }
 
         for (i = 0; i < q->n_keys; i++) {
-                r = dns_name_equal(DNS_RESOURCE_KEY_NAME(q->keys[i]), name);
+                r = dns_name_equal(DNS_RESOURCE_KEY_NAME(q->keys[i]), cname->cname.name);
                 if (r < 0)
                         return r;
 
@@ -286,7 +286,7 @@ int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion **
         for (i = 0; i < q->n_keys; i++) {
                 _cleanup_(dns_resource_key_unrefp) DnsResourceKey *k = NULL;
 
-                k = dns_resource_key_new(q->keys[i]->class, q->keys[i]->type, name);
+                k = dns_resource_key_new_redirect(q->keys[i], cname);
                 if (!k)
                         return -ENOMEM;
 
diff --git a/src/resolve/resolved-dns-question.h b/src/resolve/resolved-dns-question.h
index 77de0c7a2c..13cd1f20f3 100644
--- a/src/resolve/resolved-dns-question.h
+++ b/src/resolve/resolved-dns-question.h
@@ -46,6 +46,6 @@ int dns_question_is_superset(DnsQuestion *q, DnsQuestion *other);
 int dns_question_contains(DnsQuestion *a, DnsResourceKey *k);
 int dns_question_is_equal(DnsQuestion *a, DnsQuestion *b);
 
-int dns_question_cname_redirect(DnsQuestion *q, const char *name, DnsQuestion **ret);
+int dns_question_cname_redirect(DnsQuestion *q, const DnsResourceRecord *cname, DnsQuestion **ret);
 
 DEFINE_TRIVIAL_CLEANUP_FUNC(DnsQuestion*, dns_question_unref);
diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c
index f31644eebc..fd2f53f40b 100644
--- a/src/resolve/resolved-dns-rr.c
+++ b/src/resolve/resolved-dns-rr.c
@@ -48,6 +48,19 @@ DnsResourceKey* dns_resource_key_new(uint16_t class, uint16_t type, const char *
         return k;
 }
 
+DnsResourceKey* dns_resource_key_new_cname(const DnsResourceKey *key) {
+        assert(key);
+
+        return dns_resource_key_new(key->class, DNS_TYPE_CNAME, DNS_RESOURCE_KEY_NAME(key));
+}
+
+DnsResourceKey* dns_resource_key_new_redirect(const DnsResourceKey *key, const DnsResourceRecord *cname) {
+        assert(key);
+        assert(cname);
+
+        return dns_resource_key_new(key->class, key->type, cname->cname.name);
+}
+
 DnsResourceKey* dns_resource_key_new_consume(uint16_t class, uint16_t type, char *name) {
         DnsResourceKey *k;
 
diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h
index 8986a298af..9e2207c0aa 100644
--- a/src/resolve/resolved-dns-rr.h
+++ b/src/resolve/resolved-dns-rr.h
@@ -177,6 +177,8 @@ static inline const char* DNS_RESOURCE_KEY_NAME(const DnsResourceKey *key) {
 }
 
 DnsResourceKey* dns_resource_key_new(uint16_t class, uint16_t type, const char *name);
+DnsResourceKey* dns_resource_key_new_cname(const DnsResourceKey *key);
+DnsResourceKey* dns_resource_key_new_redirect(const DnsResourceKey *key, const DnsResourceRecord *cname);
 DnsResourceKey* dns_resource_key_new_consume(uint16_t class, uint16_t type, char *name);
 DnsResourceKey* dns_resource_key_ref(DnsResourceKey *key);
 DnsResourceKey* dns_resource_key_unref(DnsResourceKey *key);
diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index 8092bb514d..b30473dd7e 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -458,7 +458,7 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) {
         }
 
         /* According to RFC 4795, section 2.9. only the RRs from the answer section shall be cached */
-        dns_cache_put(&t->scope->cache, p->question, DNS_PACKET_RCODE(p), p->answer, DNS_PACKET_ANCOUNT(p), 0, p->family, &p->sender);
+        dns_cache_put(&t->scope->cache, t->key, DNS_PACKET_RCODE(p), p->answer, DNS_PACKET_ANCOUNT(p), 0, p->family, &p->sender);
 
         if (DNS_PACKET_RCODE(p) == DNS_RCODE_SUCCESS)
                 dns_transaction_complete(t, DNS_TRANSACTION_SUCCESS);