resolved: only clamp cache TTLs when actually serving stale data

dns_cache_lookup() computes each answer item's expiry ('until', also the
basis of the SD_RESOLVED_CLAMP_TTL TTL patch) as "30s from now" whenever
the caller did not pass SD_RESOLVED_NO_STALE — keyed on the lookup mode,
not on whether the entry being returned is actually stale. That misfires
in both directions:

 * A fresh entry hit by a serve-stale retry (second transaction attempt
   with StaleRetentionSec= set) has its TTL reported as 30s instead of
   its real remaining TTL.

 * A lookup that passes neither SD_RESOLVED_NO_STALE nor
   SD_RESOLVED_CLAMP_TTL — as the mDNS browse path does for clients
   that set no flags — leaves 'current' at 0, so every answer item's
   expiry becomes the constant absolute time 30s after boot,
   permanently in the past. The RFC 6762 section 5.2 cache maintenance
   schedule that resolved-dns-browse-services.c derives from
   item->until then degenerates to its already-expired fallback.

Key the rewrite on the entry's actual staleness instead: an entry whose
TTL has lapsed keeps reporting "30s from now", so clients don't hold on
to stale data for long, and a fresh entry reports its real expiry. For
lookups with SD_RESOLVED_NO_STALE set nothing changes: a stale entry
already makes the whole chain miss, so every item returned in that mode
is fresh.

src/resolve/resolved-dns-cache.c

@@ -21,9 +21,6 @@
 /* We never keep any item longer than 2h in our cache unless StaleRetentionSec is greater than zero. */
 #define CACHE_TTL_MAX_USEC (2 * USEC_PER_HOUR)
 
-/* The max TTL for stale data is set to 30 seconds. See RFC 8767, Section 6. */
-#define CACHE_STALE_TTL_MAX_USEC (30 * USEC_PER_SEC)
-
 /* How long to cache strange rcodes, i.e. rcodes != SUCCESS and != NXDOMAIN (specifically: that's only SERVFAIL for
  * now) */
 #define CACHE_TTL_STRANGE_RCODE_USEC (10 * USEC_PER_SEC)
@@ -1149,9 +1146,16 @@ int dns_cache_lookup(
                         dnssec_result = _DNSSEC_RESULT_INVALID;
                 }
 
-                /* If the question is being resolved using stale data, the clamp TTL will be set to CACHE_STALE_TTL_MAX_USEC. */
-                usec_t until = FLAGS_SET(query_flags, SD_RESOLVED_NO_STALE) ? j->until_valid
-                                                                            : usec_add(current, CACHE_STALE_TTL_MAX_USEC);
+                /* An entry served past its TTL expiry (possible only when SD_RESOLVED_NO_STALE is
+                 * unset, see above) reports its expiry as CACHE_STALE_TTL_MAX_USEC from now instead,
+                 * so that clients don't hold on to the stale data for longer than that. The clamp is
+                 * gated on SD_RESOLVED_CLAMP_TTL, its only consumer (see answer_add_clamp_ttl()),
+                 * which also guarantees 'current' is set here. An entry whose TTL has not expired
+                 * always reports its real expiry time, whether or not stale data was acceptable to
+                 * the caller — flag-less lookups (the mDNS browse path) thus always see cache
+                 * truth. */
+                usec_t until = FLAGS_SET(query_flags, SD_RESOLVED_CLAMP_TTL) && j->until_valid < current ?
+                               usec_add(current, CACHE_STALE_TTL_MAX_USEC) : j->until_valid;
 
                 /* Append the answer RRs to our answer. Ideally we have the answer object, which we
                  * preferably use. But if the cached entry was generated as "side-effect" of a reply,

src/resolve/resolved-dns-cache.h

@@ -8,6 +8,9 @@
 #define DEFAULT_CACHE_MAX 4096U
 #define CACHE_MAX_UPPER_LIMIT (1U << 24)
 
+/* The max TTL for stale data is set to 30 seconds. See RFC 8767, Section 6. */
+#define CACHE_STALE_TTL_MAX_USEC (30 * USEC_PER_SEC)
+
 typedef struct DnsCache {
         Hashmap *by_key;
         Prioq *by_expiry;

src/resolve/test-dns-cache.c

@@ -686,6 +686,138 @@ TEST(dns_cache_lookup_clamp_ttl) {
         ASSERT_TRUE(dns_answer_contains(ret_answer, rr));
 }
 
+TEST(dns_cache_lookup_does_not_apply_stale_clamp_to_fresh_entry) {
+        _cleanup_(dns_cache_unrefp) DnsCache cache = new_cache();
+        _cleanup_(put_args_unrefp) PutArgs put_args = mk_put_args();
+        _cleanup_(dns_answer_unrefp) DnsAnswer *ret_answer = NULL;
+        _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL;
+        DnsAnswerItem *item;
+        usec_t t;
+
+        put_args.key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "www.example.com");
+        ASSERT_NOT_NULL(put_args.key);
+        put_args.stale_retention_usec = USEC_PER_HOUR;
+        answer_add_a(&put_args, put_args.key, 0xc0a8017f, 3600, DNS_ANSWER_CACHEABLE);
+
+        t = now(CLOCK_BOOTTIME);
+        ASSERT_OK(cache_put(&cache, &put_args));
+        ASSERT_EQ(dns_cache_size(&cache), 1u);
+
+        /* A serve-stale lookup (SD_RESOLVED_NO_STALE unset) of an entry whose TTL has not expired must
+         * report the entry's real TTL and expiry time, not the stale clamp (CACHE_STALE_TTL_MAX_USEC).
+         * The expiry is bracketed from both sides: at least TTL past the pre-put reading, at most TTL
+         * past a later reading — a regression to reporting the retention-extended 'until' trips the
+         * upper bound. */
+        key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "www.example.com");
+        ASSERT_NOT_NULL(key);
+        ASSERT_OK_POSITIVE(dns_cache_lookup(&cache, key, SD_RESOLVED_CLAMP_TTL,
+                                            /* ret_rcode= */ NULL, &ret_answer,
+                                            /* ret_full_packet= */ NULL, /* ret_query_flags= */ NULL,
+                                            /* ret_dnssec_result= */ NULL));
+
+        ASSERT_EQ(dns_answer_size(ret_answer), 1u);
+        DNS_ANSWER_FOREACH_ITEM(item, ret_answer) {
+                ASSERT_GT(item->rr->ttl, (uint32_t) (CACHE_STALE_TTL_MAX_USEC / USEC_PER_SEC));
+                ASSERT_GE(item->until, usec_add(t, 3600 * USEC_PER_SEC));
+                ASSERT_LE(item->until, usec_add(now(CLOCK_BOOTTIME), 3600 * USEC_PER_SEC));
+        }
+}
+
+TEST(dns_cache_lookup_applies_stale_clamp_to_expired_entry) {
+        _cleanup_(dns_cache_unrefp) DnsCache cache = new_cache();
+        _cleanup_(put_args_unrefp) PutArgs put_args = mk_put_args();
+        _cleanup_(dns_answer_unrefp) DnsAnswer *ret_answer = NULL;
+        _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL;
+        DnsAnswerItem *item;
+        usec_t t_lookup, t_put;
+
+        put_args.key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "www.example.com");
+        ASSERT_NOT_NULL(put_args.key);
+        put_args.stale_retention_usec = USEC_PER_HOUR;
+        answer_add_a(&put_args, put_args.key, 0xc0a8017f, 1, DNS_ANSWER_CACHEABLE);
+
+        t_put = now(CLOCK_BOOTTIME);
+        ASSERT_OK(cache_put(&cache, &put_args));
+        ASSERT_EQ(dns_cache_size(&cache), 1u);
+
+        sleep(2);
+
+        /* The TTL has lapsed but the retention window keeps the entry served in serve-stale mode,
+         * with its expiry stamped CACHE_STALE_TTL_MAX_USEC out — that bracket is the regression pin
+         * for the stale clamp. (The wire TTL stays at the original 1: clamping never raises a
+         * TTL.) */
+        key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "www.example.com");
+        ASSERT_NOT_NULL(key);
+        t_lookup = now(CLOCK_BOOTTIME);
+        ASSERT_OK_POSITIVE(dns_cache_lookup(&cache, key, SD_RESOLVED_CLAMP_TTL,
+                                            /* ret_rcode= */ NULL, &ret_answer,
+                                            /* ret_full_packet= */ NULL, /* ret_query_flags= */ NULL,
+                                            /* ret_dnssec_result= */ NULL));
+
+        ASSERT_EQ(dns_answer_size(ret_answer), 1u);
+        DNS_ANSWER_FOREACH_ITEM(item, ret_answer) {
+                ASSERT_EQ(item->rr->ttl, 1u);
+                ASSERT_GE(item->until, usec_add(t_lookup, CACHE_STALE_TTL_MAX_USEC));
+                ASSERT_LE(item->until, usec_add(now(CLOCK_BOOTTIME), CACHE_STALE_TTL_MAX_USEC));
+        }
+
+        /* The same expired entry must make a SD_RESOLVED_NO_STALE lookup miss entirely. */
+        ret_answer = dns_answer_unref(ret_answer);
+        ASSERT_OK_ZERO(dns_cache_lookup(&cache, key, SD_RESOLVED_CLAMP_TTL|SD_RESOLVED_NO_STALE,
+                                        /* ret_rcode= */ NULL, &ret_answer,
+                                        /* ret_full_packet= */ NULL, /* ret_query_flags= */ NULL,
+                                        /* ret_dnssec_result= */ NULL));
+
+        /* A flag-less lookup of the expired entry reports the real, already-past expiry — cache
+         * truth for the browse bookkeeping — not a clamp. */
+        ret_answer = dns_answer_unref(ret_answer);
+        ASSERT_OK_POSITIVE(dns_cache_lookup(&cache, key, /* query_flags= */ 0,
+                                            /* ret_rcode= */ NULL, &ret_answer,
+                                            /* ret_full_packet= */ NULL, /* ret_query_flags= */ NULL,
+                                            /* ret_dnssec_result= */ NULL));
+
+        ASSERT_EQ(dns_answer_size(ret_answer), 1u);
+        DNS_ANSWER_FOREACH_ITEM(item, ret_answer) {
+                ASSERT_GE(item->until, usec_add(t_put, USEC_PER_SEC));
+                ASSERT_LE(item->until, usec_add(now(CLOCK_BOOTTIME), USEC_PER_SEC));
+        }
+}
+
+TEST(dns_cache_lookup_mdns_reports_real_expiry_without_flags) {
+        _cleanup_(dns_cache_unrefp) DnsCache cache = new_cache();
+        _cleanup_(put_args_unrefp) PutArgs put_args = mk_put_args();
+        _cleanup_(dns_answer_unrefp) DnsAnswer *ret_answer = NULL;
+        _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL;
+        DnsAnswerItem *item;
+        usec_t t;
+
+        put_args.protocol = DNS_PROTOCOL_MDNS;
+
+        /* The mDNS browse path looks up cache entries with neither SD_RESOLVED_NO_STALE nor
+         * SD_RESOLVED_CLAMP_TTL set, and schedules its RFC 6762 § 5.2 cache maintenance from the
+         * expiry time returned on the answer items. */
+        key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "ns1.example.local");
+        ASSERT_NOT_NULL(key);
+        answer_add_a(&put_args, key, 0xc0a8017f, 3600, DNS_ANSWER_CACHEABLE | DNS_ANSWER_SHARED_OWNER);
+
+        t = now(CLOCK_BOOTTIME);
+        ASSERT_OK(cache_put(&cache, &put_args));
+        ASSERT_EQ(dns_cache_size(&cache), 1u);
+
+        ASSERT_OK_POSITIVE(dns_cache_lookup(&cache, key, /* query_flags= */ 0,
+                                            /* ret_rcode= */ NULL, &ret_answer,
+                                            /* ret_full_packet= */ NULL, /* ret_query_flags= */ NULL,
+                                            /* ret_dnssec_result= */ NULL));
+
+        ASSERT_EQ(dns_answer_size(ret_answer), 1u);
+        DNS_ANSWER_FOREACH_ITEM(item, ret_answer) {
+                /* No SD_RESOLVED_CLAMP_TTL: the wire TTL passes through unclamped, too. */
+                ASSERT_EQ(item->rr->ttl, 3600u);
+                ASSERT_GE(item->until, usec_add(t, 3600 * USEC_PER_SEC));
+                ASSERT_LE(item->until, usec_add(now(CLOCK_BOOTTIME), 3600 * USEC_PER_SEC));
+        }
+}
+
 TEST(dns_cache_lookup_returns_most_recent_response) {
         _cleanup_(dns_cache_unrefp) DnsCache cache = new_cache();
         _cleanup_(put_args_unrefp) PutArgs args1 = mk_put_args(), args2 = mk_put_args();