From: Vladimír Čunát Date: Thu, 14 Jul 2022 08:53:27 +0000 (+0200) Subject: TTL bounds: improve the logic X-Git-Tag: v5.6.0~6^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a4455c78c47a09e5d30bc6e212b4afb71aff5aab;p=thirdparty%2Fknot-resolver.git TTL bounds: improve the logic - apply to first (uncached) answer already - don't extend over signature validity Nit: the tests were using too high TTL (RFCs disallow the "sign bit"). It was working because (manual) cache-insertion was applying bounds, but now the bounds don't get applied anymore, so it would fail. --- diff --git a/daemon/cache.test/insert_ns.test.integr/kresd_config.j2 b/daemon/cache.test/insert_ns.test.integr/kresd_config.j2 index ac83d20f5..bf2165b81 100644 --- a/daemon/cache.test/insert_ns.test.integr/kresd_config.j2 +++ b/daemon/cache.test/insert_ns.test.integr/kresd_config.j2 @@ -14,7 +14,7 @@ local ffi = require('ffi') local c = kres.context().cache ns_name = todname('ns.example.com') local ns_addr = '\1\2\3\4' -local rr = kres.rrset(ns_name, kres.type.A, kres.class.IN, 3600999999) +local rr = kres.rrset(ns_name, kres.type.A, kres.class.IN, 2147483647) assert(rr:add_rdata(ns_addr, #ns_addr)) assert(c:insert(rr, nil, ffi.C.KR_RANK_SECURE)) diff --git a/lib/cache/api.c b/lib/cache/api.c index 6a572b034..530592aa6 100644 --- a/lib/cache/api.c +++ b/lib/cache/api.c @@ -597,14 +597,11 @@ static ssize_t stash_rrset(struct kr_cache *cache, const struct kr_query *qry, if (kr_fails_assert(val_new_entry.data)) return kr_error(EFAULT); - const uint32_t ttl = rr->ttl; - /* FIXME: consider TTLs and expirations of RRSIGs as well, just in case. */ - /* Write the entry itself. */ struct entry_h *eh = val_new_entry.data; memset(eh, 0, offsetof(struct entry_h, data)); eh->time = timestamp; - eh->ttl = MAX(MIN(ttl, cache->ttl_max), cache->ttl_min); + eh->ttl = rr->ttl; eh->rank = rank; rdataset_dematerialize(&rr->rrs, eh->data); rdataset_dematerialize(rds_sigs, eh->data + rr_ssize); diff --git a/lib/cache/api.h b/lib/cache/api.h index 76cbebc1b..48cc0d2f2 100644 --- a/lib/cache/api.h +++ b/lib/cache/api.h @@ -25,7 +25,7 @@ struct kr_cache kr_cdb_pt db; /**< Storage instance */ const struct kr_cdb_api *api; /**< Storage engine */ struct kr_cdb_stats stats; - uint32_t ttl_min, ttl_max; /**< TTL limits */ + uint32_t ttl_min, ttl_max; /**< TTL limits; enforced primarily in iterator actually. */ /* A pair of stamps for detection of real-time shifts during runtime. */ struct timeval checkpoint_walltime; /**< Wall time on the last check-point. */ diff --git a/lib/dnssec.c b/lib/dnssec.c index f56ab759f..3eb6a2ada 100644 --- a/lib/dnssec.c +++ b/lib/dnssec.c @@ -144,16 +144,24 @@ int kr_rrset_validate(kr_rrset_validation_ctx_t *vctx, knot_rrset_t *covered) /** Assuming `rrs` was validated with `sig`, trim its TTL in case it's over-extended. */ static bool trim_ttl(knot_rrset_t *rrs, const knot_rdata_t *sig, - uint32_t timestamp, const struct kr_query *log_qry) + const kr_rrset_validation_ctx_t *vctx) { - const uint32_t ttl_max = MIN(knot_rrsig_original_ttl(sig), - knot_rrsig_sig_expiration(sig) - timestamp); + /* The trimming logic is a bit complicated. + * + * We respect configured ttl_min over the (signed) original TTL, + * but we very much want to avoid TTLs over signature expiration, + * as that could cause serious issues with downstream validators. + */ + const uint32_t ttl_max = MIN( + MAX(knot_rrsig_original_ttl(sig), vctx->ttl_min), + knot_rrsig_sig_expiration(sig) - vctx->timestamp + ); if (likely(rrs->ttl <= ttl_max)) return false; - if (kr_log_is_debug_qry(VALIDATOR, log_qry)) { + if (kr_log_is_debug_qry(VALIDATOR, vctx->log_qry)) { auto_free char *name_str = kr_dname_text(rrs->owner), *type_str = kr_rrtype_text(rrs->type); - kr_log_q(log_qry, VALIDATOR, "trimming TTL of %s %s: %d -> %d\n", + kr_log_q(vctx->log_qry, VALIDATOR, "trimming TTL of %s %s: %d -> %d\n", name_str, type_str, (int)rrs->ttl, (int)ttl_max); } rrs->ttl = ttl_max; @@ -204,7 +212,7 @@ struct kr_svldr_ctx * kr_svldr_new_ctx(const knot_rrset_t *ds, knot_rrset_t *dns struct kr_svldr_ctx *ctx = calloc(1, sizeof(*ctx)); if (unlikely(!ctx)) return NULL; - ctx->vctx.timestamp = timestamp; + ctx->vctx.timestamp = timestamp; // .ttl_min is implicitly zero ctx->vctx.zone_name = knot_dname_copy(ds->owner, NULL); if (unlikely(!ctx->vctx.zone_name)) goto fail; @@ -254,7 +262,7 @@ static int kr_svldr_rrset_with_key(knot_rrset_t *rrs, const knot_rdataset_t *rrs // that also means we don't need to perform non-existence proofs. const int trim_labels = (val_flgs & FLG_WILDCARD_EXPANSION) ? 1 : 0; if (kr_check_signature(rdata_j, key->key, rrs, trim_labels) == 0) { - trim_ttl(rrs, rdata_j, vctx->timestamp, vctx->log_qry); + trim_ttl(rrs, rdata_j, vctx); vctx->result = kr_ok(); return vctx->result; } else { @@ -382,7 +390,7 @@ static int kr_rrset_validate_with_key(kr_rrset_validation_ctx_t *vctx, vctx->flags |= KR_DNSSEC_VFLG_WEXPAND; } - trim_ttl(covered, rdata_j, vctx->timestamp, vctx->log_qry); + trim_ttl(covered, rdata_j, vctx); kr_dnssec_key_free(&created_key); vctx->result = kr_ok(); diff --git a/lib/dnssec.h b/lib/dnssec.h index 97c8831d4..d9369978e 100644 --- a/lib/dnssec.h +++ b/lib/dnssec.h @@ -38,6 +38,7 @@ struct kr_rrset_validation_ctx { knot_rrset_t *keys; /*!< DNSKEY RRSet; TTLs may get lowered when validating this set. */ const knot_dname_t *zone_name; /*!< Name of the zone containing the RRSIG RRSet. */ uint32_t timestamp; /*!< Validation time. */ + uint32_t ttl_min; /*!< See trim_ttl() for details. */ bool has_nsec3; /*!< Whether to use NSEC3 validation. */ uint32_t qry_uid; /*!< Current query uid. */ uint32_t flags; /*!< Output - Flags. */ diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c index 03366acea..2a37e6856 100644 --- a/lib/layer/iterate.c +++ b/lib/layer/iterate.c @@ -1010,6 +1010,22 @@ static bool satisfied_by_additional(const struct kr_query *qry) return false; } +/** Restrict all RRset TTLs to the specified bounds (if matching qry_uid). */ +static void bound_ttls(ranked_rr_array_t *array, uint32_t qry_uid, + uint32_t ttl_min, uint32_t ttl_max) +{ + for (ssize_t i = 0; i < array->len; ++i) { + if (array->at[i]->qry_uid != qry_uid) + continue; + uint32_t *ttl = &array->at[i]->rr->ttl; + if (*ttl < ttl_min) { + *ttl = ttl_min; + } else if (*ttl > ttl_max) { + *ttl = ttl_max; + } + } +} + /** Resolve input query or continue resolution with followups. * * This roughly corresponds to RFC1034, 5.3.3 4a-d. @@ -1186,12 +1202,14 @@ rrarray_finalize: /* Finish construction of libknot-format RRsets. * We do this even if dropping the answer, though it's probably useless. */ (void)0; + const struct kr_cache *cache = &req->ctx->cache; ranked_rr_array_t *selected[] = kr_request_selected(req); for (knot_section_t i = KNOT_ANSWER; i <= KNOT_ADDITIONAL; ++i) { ret = kr_ranked_rrarray_finalize(selected[i], query->uid, &req->pool); - if (unlikely(ret)) { + if (unlikely(ret)) return KR_STATE_FAIL; - } + if (!query->flags.CACHED) + bound_ttls(selected[i], query->uid, cache->ttl_min, cache->ttl_max); } return state; diff --git a/lib/layer/validate.c b/lib/layer/validate.c index ef21e3010..68e85659c 100644 --- a/lib/layer/validate.c +++ b/lib/layer/validate.c @@ -268,6 +268,7 @@ static int validate_records(struct kr_request *req, knot_pkt_t *answer, knot_mm_ .keys = qry->zone_cut.key, .zone_name = qry->zone_cut.name, .timestamp = qry->timestamp.tv_sec, + .ttl_min = req->ctx->cache.ttl_min, .qry_uid = qry->uid, .has_nsec3 = has_nsec3, .flags = 0, @@ -377,6 +378,7 @@ static int validate_keyset(struct kr_request *req, knot_pkt_t *answer, bool has_ .keys = qry->zone_cut.key, .zone_name = qry->zone_cut.name, .timestamp = qry->timestamp.tv_sec, + .ttl_min = req->ctx->cache.ttl_min, .qry_uid = qry->uid, .has_nsec3 = has_nsec3, .flags = 0, diff --git a/modules/policy/policy.rpz.test.lua b/modules/policy/policy.rpz.test.lua index 70ef9fb6f..94fb9ceb4 100644 --- a/modules/policy/policy.rpz.test.lua +++ b/modules/policy/policy.rpz.test.lua @@ -7,7 +7,7 @@ local function prepare_cache() local c = kres.context().cache local passthru_addr = '\127\0\0\9' - rr_passthru = kres.rrset(todname('rpzpassthru.'), kres.type.A, kres.class.IN, 3600999999) + rr_passthru = kres.rrset(todname('rpzpassthru.'), kres.type.A, kres.class.IN, 2147483647) assert(rr_passthru:add_rdata(passthru_addr, #passthru_addr)) assert(c:insert(rr_passthru, nil, ffi.C.KR_RANK_SECURE + ffi.C.KR_RANK_AUTH))