From 1ffb67a13524ea626ce558c31d7ad44b54e59564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 29 Jan 2025 11:11:32 +0100 Subject: [PATCH 1/7] Split and simplify the use of EDE list implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of mixing the dns_resolver and dns_validator units directly with the EDE code, split-out the dns_ede functionality into own separate compilation unit and hide the implementation details behind abstraction. Additionally, the EDE codes are directly copied into the ns_client buffers by passing the EDE context to dns_resolver_createfetch(). This makes the dns_ede implementation simpler to use, although sligtly more complicated on the inside. Co-authored-by: Colin Vidal Co-authored-by: Ondřej Surý (cherry picked from commit 2f8e0edf3b25fe4d9df4be8331ca0520ea5d764c) --- bin/dig/dighost.c | 1 + bin/named/server.c | 2 +- lib/dns/Makefile.am | 2 + lib/dns/adb.c | 2 +- lib/dns/client.c | 2 +- lib/dns/ede.c | 168 ++++++++++++++++++++++++++++++++ lib/dns/include/dns/ede.h | 55 +++++++++++ lib/dns/include/dns/message.h | 55 +---------- lib/dns/include/dns/resolver.h | 35 +------ lib/dns/include/dns/validator.h | 11 +-- lib/dns/message.c | 44 --------- lib/dns/nta.c | 2 +- lib/dns/resolver.c | 106 ++++++-------------- lib/dns/validator.c | 60 ++++++++---- lib/dns/zone.c | 5 +- lib/ns/client.c | 99 ++----------------- lib/ns/include/ns/client.h | 8 +- lib/ns/query.c | 48 ++++----- tests/dns/ede_test.c | 88 +++++++++-------- tests/ns/client_test.c | 43 ++++---- 20 files changed, 413 insertions(+), 423 deletions(-) create mode 100644 lib/dns/ede.c create mode 100644 lib/dns/include/dns/ede.h diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c index 96e734da3f..3e1aa5a8ea 100644 --- a/bin/dig/dighost.c +++ b/bin/dig/dighost.c @@ -60,6 +60,7 @@ #include #include +#include #include #include #include diff --git a/bin/named/server.c b/bin/named/server.c index a50ec9c558..a24929a49b 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -7394,7 +7394,7 @@ tat_send(void *arg) { result = dns_resolver_createfetch( tat->view->resolver, tatname, dns_rdatatype_null, domain, &nameservers, NULL, NULL, 0, 0, 0, NULL, NULL, - tat->loop, tat_done, tat, &tat->rdataset, + tat->loop, tat_done, tat, NULL, &tat->rdataset, &tat->sigrdataset, &tat->fetch); } diff --git a/lib/dns/Makefile.am b/lib/dns/Makefile.am index 9865d5b18e..3428131fcc 100644 --- a/lib/dns/Makefile.am +++ b/lib/dns/Makefile.am @@ -72,6 +72,7 @@ libdns_la_HEADERS = \ include/dns/dnstap.h \ include/dns/dyndb.h \ include/dns/ecs.h \ + include/dns/ede.h \ include/dns/edns.h \ include/dns/fixedname.h \ include/dns/forward.h \ @@ -182,6 +183,7 @@ libdns_la_SOURCES = \ dst_parse.h \ dyndb.c \ ecs.c \ + ede.c \ fixedname.c \ forward.c \ gssapictx.c \ diff --git a/lib/dns/adb.c b/lib/dns/adb.c index 281377e376..8c9fb2fa44 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -3012,7 +3012,7 @@ fetch_name(dns_adbname_t *adbname, bool start_at_zone, unsigned int depth, result = dns_resolver_createfetch( adb->res, adbname->name, type, name, nameservers, NULL, NULL, 0, options, depth, qc, gqc, isc_loop(), fetch_callback, adbname, - &fetch->rdataset, NULL, &fetch->fetch); + NULL, &fetch->rdataset, NULL, &fetch->fetch); if (result != ISC_R_SUCCESS) { DP(ENTER_LEVEL, "fetch_name: createfetch failed with %s", isc_result_totext(result)); diff --git a/lib/dns/client.c b/lib/dns/client.c index fa4ba6ee84..e8ee80672b 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -470,7 +470,7 @@ start_fetch(resctx_t *rctx) { result = dns_resolver_createfetch( rctx->view->resolver, dns_fixedname_name(&rctx->name), rctx->type, NULL, NULL, NULL, NULL, 0, fopts, 0, NULL, rctx->qc, - rctx->client->loop, fetch_done, rctx, rctx->rdataset, + rctx->client->loop, fetch_done, rctx, NULL, rctx->rdataset, rctx->sigrdataset, &rctx->fetch); return result; diff --git a/lib/dns/ede.c b/lib/dns/ede.c new file mode 100644 index 0000000000..d5c7e2da48 --- /dev/null +++ b/lib/dns/ede.c @@ -0,0 +1,168 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +/*! \file */ + +#include +#include + +#include + +#define DNS_EDE_MAGIC ISC_MAGIC('E', 'D', 'E', '!') +#define DNS_EDE_VALID(v) ISC_MAGIC_VALID(v, DNS_EDE_MAGIC) + +void +dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text) { + REQUIRE(DNS_EDE_VALID(edectx)); + + size_t pos = 0; + uint16_t becode = htobe16(code); + dns_ednsopt_t *edns = NULL; + size_t textlen = 0; + + for (pos = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { + edns = edectx->ede[pos]; + + if (edns == NULL) { + break; + } + + if (memcmp(&becode, edns->value, sizeof(becode)) == 0) { + isc_log_write(DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), + "ignoring duplicate ede %u %s", code, + text == NULL ? "(null)" : text); + return; + } + } + + if (pos >= DNS_EDE_MAX_ERRORS) { + isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, + ISC_LOG_DEBUG(1), "too many ede, ignoring %u %s", + code, text == NULL ? "(null)" : text); + return; + } + + isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, + ISC_LOG_DEBUG(1), "set ede: info-code %u extra-text %s", + code, text == NULL ? "(null)" : text); + + if (text != NULL) { + textlen = strlen(text); + + if (textlen > DNS_EDE_EXTRATEXT_LEN) { + isc_log_write(DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), + "truncate EDE code %hu text: %s", code, + text); + textlen = DNS_EDE_EXTRATEXT_LEN; + } + } + + edns = isc_mem_get(edectx->mctx, + sizeof(*edns) + sizeof(becode) + textlen); + *edns = (dns_ednsopt_t){ + .code = DNS_OPT_EDE, + .length = sizeof(becode) + textlen, + .value = (uint8_t *)edns + sizeof(*edns), + }; + + memmove(edns->value, &becode, sizeof(becode)); + if (textlen > 0) { + memmove(edns->value + sizeof(becode), text, textlen); + } + + edectx->ede[pos] = edns; +} + +void +dns_ede_init(isc_mem_t *mctx, dns_edectx_t *edectx) { + REQUIRE(mctx != NULL); + + /* + * Memory context is assigned, not attached here, + * thus there's no detach in dns_ede_reset(). + */ + *edectx = (dns_edectx_t){ + .magic = DNS_EDE_MAGIC, + .mctx = mctx, + }; +} + +void +dns_ede_reset(dns_edectx_t *edectx) { + REQUIRE(DNS_EDE_VALID(edectx)); + + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + dns_ednsopt_t *edns = edectx->ede[i]; + if (edns == NULL) { + break; + } + + isc_mem_put(edectx->mctx, edns, sizeof(*edns) + edns->length); + edectx->ede[i] = NULL; + } +} + +void +dns_ede_invalidate(dns_edectx_t *edectx) { + REQUIRE(DNS_EDE_VALID(edectx)); + + dns_ede_reset(edectx); + + edectx->magic = 0; + edectx->mctx = NULL; +} + +void +dns_ede_copy(dns_edectx_t *edectx_to, dns_edectx_t *edectx_from) { + REQUIRE(DNS_EDE_VALID(edectx_to)); + REQUIRE(DNS_EDE_VALID(edectx_from)); + + size_t nextede = 0; + + for (nextede = 0; nextede < DNS_EDE_MAX_ERRORS; nextede++) { + if (edectx_to->ede[nextede] == NULL) { + break; + } + } + + for (size_t pos = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { + if (edectx_from->ede[pos] == NULL) { + break; + } + + if (nextede >= DNS_EDE_MAX_ERRORS) { + isc_log_write(DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), + "too many ede from subfetch"); + break; + } + + INSIST(edectx_to->ede[nextede] == NULL); + + dns_ednsopt_t *edns = isc_mem_get( + edectx_to->mctx, + sizeof(*edns) + edectx_from->ede[pos]->length); + *edns = (dns_ednsopt_t){ + .code = DNS_OPT_EDE, + .length = edectx_from->ede[pos]->length, + .value = (uint8_t *)edns + sizeof(*edns), + }; + memmove(edns->value, edectx_from->ede[pos]->value, + edectx_from->ede[pos]->length); + + edectx_to->ede[nextede] = edns; + nextede++; + } +} diff --git a/lib/dns/include/dns/ede.h b/lib/dns/include/dns/ede.h new file mode 100644 index 0000000000..b14f16a715 --- /dev/null +++ b/lib/dns/include/dns/ede.h @@ -0,0 +1,55 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +#pragma once + +#include + +#include + +/* + * From RFC 8914: + * Because long EXTRA-TEXT fields may trigger truncation (which is undesirable + * given the supplemental nature of EDE), implementers and operators creating + * EDE options SHOULD avoid lengthy EXTRA-TEXT contents. + * + * Following this advice we limit the EXTRA-TEXT length to 64 characters. + */ +#define DNS_EDE_EXTRATEXT_LEN 64 + +#define DNS_EDE_MAX_ERRORS 3 + +typedef struct dns_edectx dns_edectx_t; +struct dns_edectx { + int magic; + isc_mem_t *mctx; + dns_ednsopt_t *ede[DNS_EDE_MAX_ERRORS]; +}; + +void +dns_ede_init(isc_mem_t *mctx, dns_edectx_t *edectx); + +void +dns_ede_reset(dns_edectx_t *edectx); + +void +dns_ede_invalidate(dns_edectx_t *edectx); + +void +dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text); +/*%< + * Set extended error with INFO-CODE and EXTRA-TEXT . + */ + +void +dns_ede_copy(dns_edectx_t *edectx_to, dns_edectx_t *edectx_from); diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index b461992334..6de8b3d3f4 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -151,27 +151,6 @@ #define DNS_EDE_NETWORKERROR 23 /*%< Network Error */ #define DNS_EDE_INVALIDDATA 24 /*%< Invalid Data */ -typedef struct dns_ede dns_ede_t; -struct dns_ede { - uint16_t info_code; - char *extra_text; - ISC_LINK(dns_ede_t) link; -}; - -typedef ISC_LIST(dns_ede_t) dns_edelist_t; - -/* - * From RFC 8914: - * Because long EXTRA-TEXT fields may trigger truncation (which is undesirable - * given the supplemental nature of EDE), implementers and operators creating - * EDE options SHOULD avoid lengthy EXTRA-TEXT contents. - * - * Following this advice we limit the EXTRA-TEXT length to 64 characters. - */ -#define DNS_EDE_EXTRATEXT_LEN 64 - -#define DNS_EDE_MAX_ERRORS 3 - #define DNS_MESSAGE_REPLYPRESERVE (DNS_MESSAGEFLAG_RD | DNS_MESSAGEFLAG_CD) #define DNS_MESSAGEEXTFLAG_REPLYPRESERVE (DNS_MESSAGEEXTFLAG_DO) @@ -358,9 +337,9 @@ struct dns_message { }; struct dns_ednsopt { - uint16_t code; - uint16_t length; - unsigned char *value; + uint16_t code; + uint16_t length; + uint8_t *value; }; typedef void (*dns_message_cb_t)(void *arg, isc_result_t result); @@ -1531,32 +1510,4 @@ dns_message_createpools(isc_mem_t *mctx, isc_mempool_t **namepoolp, void dns_message_destroypools(isc_mempool_t **namepoolp, isc_mempool_t **rdspoolp); -void -dns_ede_append(isc_mem_t *mctx, dns_edelist_t *list, uint16_t info_code, - const char *extra_text); -/*%< - * Adds a new EDE message at the end of 'list'. If 'extra_text' is non - * NULL, the string is synchronously copied internally, so the called - * doesn't have to keep it alive once this call returns. RFC8914 - * section 4 define the valid range of possible numbers for - * 'info_code'. - * - * Requires: - * \li mctx to be non NULL; - * \li list to be non NULL; - * \li info_code to be valid; - * \li extra_text can be NULL or non NULL, do not take ownership. - */ - -void -dns_ede_unlinkall(isc_mem_t *mctx, dns_edelist_t *list); -/*%< - * Unlink all elements from from 'list' and free it from - * memory. Optional text owned by elements is also freed. - * - * Requires: - * \li mctx to be non NULL; - * \li list to be non NULL; - */ - ISC_LANG_ENDDECLS diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h index 155e35b8de..f4bfa4c8ba 100644 --- a/lib/dns/include/dns/resolver.h +++ b/lib/dns/include/dns/resolver.h @@ -56,6 +56,7 @@ #include #include +#include #include #include #include @@ -82,7 +83,7 @@ struct dns_fetchresponse { isc_mem_t *mctx; isc_result_t result; isc_result_t vresult; - dns_edelist_t edelist; + dns_edectx_t *edectx; dns_rdatatype_t qtype; dns_db_t *db; dns_dbnode_t *node; @@ -276,8 +277,8 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, unsigned int options, unsigned int depth, isc_counter_t *qc, isc_counter_t *gqc, isc_loop_t *loop, isc_job_cb cb, void *arg, - dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, - dns_fetch_t **fetchp); + dns_edectx_t *edectx, dns_rdataset_t *rdataset, + dns_rdataset_t *sigrdataset, dns_fetch_t **fetchp); /*%< * Recurse to answer a question. * @@ -648,32 +649,4 @@ dns_resolver_freefresp(dns_fetchresponse_t **frespp); * \li 'frespp' is valid. No-op if *frespp == NULL */ -void -dns_resolver_edeappend(fetchctx_t *fctx, uint16_t info_code, const char *what, - const dns_name_t *name, dns_rdatatype_t type); -/*%< - * Helper for EDE message creation in resolver context. Creates message - * containing the "what" context message as well as the "name"/"type" being - * resolved - * - * Requires: - * \li "fctx" is valid - * \li "what" is valid - * \li "info_code" is within the range of defined EDE codes - * \li "name" is valid - */ - -void -dns_resolver_copyede(dns_fetch_t *from, fetchctx_t *to); -/*%< - * Copy all EDE messages from the fetchctx_t "from->private" to the fetchctx_t - * "to". The fetchctx_t from "from" is not locked. This is reponsability of the - * caller to lock it if this function is called in a context needing "from" - * synchronization. - * - * Requires: - * \li "from" is valid - * \li "to" is valid - */ - ISC_LANG_ENDDECLS diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index 973adbc510..871f99c662 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -56,6 +56,7 @@ #include #include #include /* for dns_rdata_rrsig_t */ +#include #include #include @@ -157,11 +158,7 @@ struct dns_validator { isc_counter_t *qc; isc_counter_t *gqc; - /* - * opaque type here, used to send EDE errors during DNSSEC valiration - * to the fetch context. - */ - fetchctx_t *fctx; + dns_edectx_t *edectx; }; /*% @@ -180,8 +177,8 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, dns_message_t *message, unsigned int options, isc_loop_t *loop, isc_job_cb cb, void *arg, uint32_t *nvalidations, uint32_t *nfails, - isc_counter_t *qc, isc_counter_t *gqc, fetchctx_t *fctx, - dns_validator_t **validatorp); + isc_counter_t *qc, isc_counter_t *gqc, + dns_edectx_t *edectx, dns_validator_t **validatorp); /*%< * Start a DNSSEC validation. * diff --git a/lib/dns/message.c b/lib/dns/message.c index 94f2d92ab1..143f2fa49b 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -5013,47 +5013,3 @@ dns_message_destroypools(isc_mempool_t **namepoolp, isc_mempool_t **rdspoolp) { isc_mempool_destroy(rdspoolp); isc_mempool_destroy(namepoolp); } - -void -dns_ede_append(isc_mem_t *mctx, dns_edelist_t *list, uint16_t info_code, - const char *extra_text) { - REQUIRE(mctx); - REQUIRE(list); - REQUIRE(info_code <= 24); - - dns_ede_t *ede = isc_mem_get(mctx, sizeof(*ede)); - *ede = (dns_ede_t){ - .info_code = info_code, - .extra_text = NULL, - .link = ISC_LINK_INITIALIZER, - }; - - if (extra_text) { - ede->extra_text = isc_mem_allocate(mctx, - strlen(extra_text) + 1); - strcpy(ede->extra_text, extra_text); - } - - ISC_LIST_APPEND(*list, ede, link); -} - -void -dns_ede_unlinkall(isc_mem_t *mctx, dns_edelist_t *list) { - dns_ede_t *ede, *next; - - REQUIRE(mctx); - REQUIRE(list); - - for (ede = ISC_LIST_HEAD(*list); ede != NULL; ede = next) { - next = ISC_LIST_NEXT(ede, link); - - ISC_LIST_UNLINK(*list, ede, link); - if (ede->extra_text) { - isc_mem_free(mctx, ede->extra_text); - ede->extra_text = NULL; - } - isc_mem_put(mctx, ede, sizeof(*ede)); - } - - INSIST(ISC_LIST_EMPTY(*list)); -} diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 8b81d1bbda..7d5b4ee677 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -242,7 +242,7 @@ checkbogus(void *arg) { result = dns_resolver_createfetch( resolver, &nta->name, dns_rdatatype_nsec, NULL, NULL, NULL, NULL, 0, DNS_FETCHOPT_NONTA, 0, NULL, NULL, nta->loop, - fetch_done, nta, &nta->rdataset, &nta->sigrdataset, + fetch_done, nta, NULL, &nta->rdataset, &nta->sigrdataset, &nta->fetch); if (result != ISC_R_SUCCESS) { dns__nta_detach(&nta); /* for dns_resolver_createfetch() */ diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 6f6803d45c..12368d0e5b 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -46,6 +46,7 @@ #include #include #include +#include #include #include #include @@ -348,6 +349,8 @@ struct fetchctx { isc_loop_t *loop; unsigned int tid; + dns_edectx_t edectx; + /* Atomic */ isc_refcount_t references; @@ -359,7 +362,6 @@ struct fetchctx { bool spilled; ISC_LINK(struct fetchctx) link; ISC_LIST(dns_fetchresponse_t) resps; - dns_edelist_t edelist; /*% Locked by loop event serialization. */ dns_fixedname_t dfname; @@ -988,7 +990,7 @@ valcreate(fetchctx_t *fctx, dns_message_t *message, dns_adbaddrinfo_t *addrinfo, result = dns_validator_create( fctx->res->view, name, type, rdataset, sigrdataset, message, valoptions, fctx->loop, validated, valarg, &fctx->nvalidations, - &fctx->nfails, fctx->qc, fctx->gqc, fctx, &validator); + &fctx->nfails, fctx->qc, fctx->gqc, &fctx->edectx, &validator); RUNTIME_CHECK(result == ISC_R_SUCCESS); inc_stats(fctx->res, dns_resstatscounter_val); if ((valoptions & DNS_VALIDATOR_DEFER) == 0) { @@ -1600,14 +1602,11 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) { } /* - * Copy EDE that occured during the resolution to all - * clients. + * Finalize the EDE context, so it becomes "constant" and assign + * it to all clients. */ - for (dns_ede_t *ede = ISC_LIST_HEAD(fctx->edelist); ede != NULL; - ede = ISC_LIST_NEXT(ede, link)) - { - dns_ede_append(resp->mctx, &resp->edelist, - ede->info_code, ede->extra_text); + if (resp->edectx != NULL) { + dns_ede_copy(resp->edectx, &fctx->edectx); } FCTXTRACE("post response event"); @@ -4112,7 +4111,7 @@ fctx_try(fetchctx_t *fctx, bool retrying) { result = dns_resolver_createfetch( fctx->res, fctx->qminname, fctx->qmintype, fctx->domain, &fctx->nameservers, NULL, NULL, 0, options, 0, fctx->qc, - fctx->gqc, fctx->loop, resume_qmin, fctx, + fctx->gqc, fctx->loop, resume_qmin, fctx, &fctx->edectx, &fctx->qminrrset, NULL, &fctx->qminfetch); if (result != ISC_R_SUCCESS) { fetchctx_unref(fctx); @@ -4199,7 +4198,6 @@ resume_qmin(void *arg) { } UNLOCK(&fctx->lock); - dns_resolver_copyede(fctx->qminfetch, fctx); dns_resolver_destroyfetch(&fctx->qminfetch); /* @@ -4373,8 +4371,6 @@ fctx_destroy(fetchctx_t *fctx) { isc_mem_put(fctx->mctx, sa, sizeof(*sa)); } - dns_ede_unlinkall(fctx->mctx, &fctx->edelist); - isc_counter_detach(&fctx->qc); if (fctx->gqc != NULL) { isc_counter_detach(&fctx->gqc); @@ -4389,6 +4385,8 @@ fctx_destroy(fetchctx_t *fctx) { dns_resolver_detach(&fctx->res); + dns_ede_invalidate(&fctx->edectx); + isc_mutex_destroy(&fctx->lock); isc_mem_free(fctx->mctx, fctx->info); @@ -4407,10 +4405,7 @@ fctx_expired(void *arg) { "shut down hung fetch while resolving %p(%s)", fctx, fctx->info); - LOCK(&fctx->lock); - dns_ede_append(fctx->mctx, &fctx->edelist, DNS_EDE_NOREACHABLEAUTH, - NULL); - UNLOCK(&fctx->lock); + dns_ede_add(&fctx->edectx, DNS_EDE_NOREACHABLEAUTH, NULL); fctx_done_detach(&fctx, DNS_R_SERVFAIL); } @@ -4466,8 +4461,8 @@ detach: static void fctx_add_event(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, dns_messageid_t id, isc_job_cb cb, void *arg, - dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, - dns_fetch_t *fetch) { + dns_edectx_t *edectx, dns_rdataset_t *rdataset, + dns_rdataset_t *sigrdataset, dns_fetch_t *fetch) { dns_fetchresponse_t *resp = NULL; FCTXTRACE("addevent"); @@ -4475,7 +4470,6 @@ fctx_add_event(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, resp = isc_mem_get(fctx->mctx, sizeof(*resp)); *resp = (dns_fetchresponse_t){ .result = DNS_R_SERVFAIL, - .edelist = ISC_LIST_INITIALIZER, .qtype = fctx->type, .rdataset = rdataset, .sigrdataset = sigrdataset, @@ -4486,6 +4480,7 @@ fctx_add_event(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, .cb = cb, .arg = arg, .link = ISC_LINK_INITIALIZER, + .edectx = edectx, }; isc_mem_attach(fctx->mctx, &resp->mctx); @@ -4504,15 +4499,15 @@ fctx_add_event(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, static void fctx_join(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, - dns_messageid_t id, isc_job_cb cb, void *arg, + dns_messageid_t id, isc_job_cb cb, void *arg, dns_edectx_t *edectx, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, dns_fetch_t *fetch) { FCTXTRACE("join"); REQUIRE(!SHUTTINGDOWN(fctx)); - fctx_add_event(fctx, loop, client, id, cb, arg, rdataset, sigrdataset, - fetch); + fctx_add_event(fctx, loop, client, id, cb, arg, edectx, rdataset, + sigrdataset, fetch); fetch->magic = DNS_FETCH_MAGIC; fetchctx_attach(fctx, &fetch->private); @@ -4566,7 +4561,6 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, .loop = loop, .nvalidations = atomic_load_relaxed(&res->maxvalidations), .nfails = atomic_load_relaxed(&res->maxvalidationfails), - .edelist = ISC_LIST_INITIALIZER, }; isc_mem_attach(mctx, &fctx->mctx); @@ -4574,6 +4568,8 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, isc_mutex_init(&fctx->lock); + dns_ede_init(fctx->mctx, &fctx->edectx); + /* * Make fctx->info point to a copy of a formatted string * "name/type". FCTXTRACE won't work until this is done. @@ -7193,8 +7189,8 @@ resume_dslookup(void *arg) { result = dns_resolver_createfetch( res, fctx->nsname, dns_rdatatype_ns, domain, nsrdataset, NULL, NULL, 0, fctx->options, 0, fctx->qc, fctx->gqc, - loop, resume_dslookup, fctx, &fctx->nsrrset, NULL, - &fctx->nsfetch); + loop, resume_dslookup, fctx, &fctx->edectx, + &fctx->nsrrset, NULL, &fctx->nsfetch); if (result != ISC_R_SUCCESS) { fetchctx_unref(fctx); if (result == DNS_R_DUPLICATE) { @@ -7208,7 +7204,6 @@ resume_dslookup(void *arg) { } cleanup: - dns_resolver_copyede(fetch, fctx); dns_resolver_destroyfetch(&fetch); if (result != ISC_R_SUCCESS) { @@ -9627,7 +9622,8 @@ rctx_chaseds(respctx_t *rctx, dns_message_t *message, result = dns_resolver_createfetch( fctx->res, fctx->nsname, dns_rdatatype_ns, NULL, NULL, NULL, NULL, 0, fctx->options, 0, fctx->qc, fctx->gqc, fctx->loop, - resume_dslookup, fctx, &fctx->nsrrset, NULL, &fctx->nsfetch); + resume_dslookup, fctx, &fctx->edectx, &fctx->nsrrset, NULL, + &fctx->nsfetch); if (result != ISC_R_SUCCESS) { if (result == DNS_R_DUPLICATE) { result = DNS_R_SERVFAIL; @@ -10202,7 +10198,7 @@ dns_resolver_prime(dns_resolver_t *res) { result = dns_resolver_createfetch( res, dns_rootname, dns_rdatatype_ns, NULL, NULL, NULL, NULL, 0, DNS_FETCHOPT_NOFORWARD, 0, NULL, NULL, - isc_loop(), prime_done, res, rdataset, NULL, + isc_loop(), prime_done, res, NULL, rdataset, NULL, &res->primefetch); UNLOCK(&res->primelock); @@ -10479,8 +10475,8 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, unsigned int options, unsigned int depth, isc_counter_t *qc, isc_counter_t *gqc, isc_loop_t *loop, isc_job_cb cb, void *arg, - dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, - dns_fetch_t **fetchp) { + dns_edectx_t *edectx, dns_rdataset_t *rdataset, + dns_rdataset_t *sigrdataset, dns_fetch_t **fetchp) { dns_fetch_t *fetch = NULL; fetchctx_t *fctx = NULL; isc_result_t result = ISC_R_SUCCESS; @@ -10580,8 +10576,8 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, fctx->depth = depth; } - fctx_join(fctx, loop, client, id, cb, arg, rdataset, sigrdataset, - fetch); + fctx_join(fctx, loop, client, id, cb, arg, edectx, rdataset, + sigrdataset, fetch); if (new_fctx) { fetchctx_ref(fctx); @@ -11108,7 +11104,7 @@ dns_resolver_getquerystats(dns_resolver_t *res, dns_stats_t **statsp) { void dns_resolver_freefresp(dns_fetchresponse_t **frespp) { - REQUIRE(frespp); + REQUIRE(frespp != NULL); if (*frespp == NULL) { return; @@ -11117,47 +11113,5 @@ dns_resolver_freefresp(dns_fetchresponse_t **frespp) { dns_fetchresponse_t *fresp = *frespp; *frespp = NULL; - dns_ede_unlinkall(fresp->mctx, &fresp->edelist); isc_mem_putanddetach(&fresp->mctx, fresp, sizeof(*fresp)); } - -void -dns_resolver_edeappend(fetchctx_t *fctx, uint16_t info_code, const char *what, - const dns_name_t *name, dns_rdatatype_t type) { - REQUIRE(VALID_FCTX(fctx)); - REQUIRE(what); - REQUIRE(name); - - char extra[DNS_NAME_FORMATSIZE + DNS_RDATATYPE_FORMATSIZE + - DNS_EDE_EXTRATEXT_LEN]; - size_t offset = 0; - - /* - * -2 to leave room for separator "/" and NULL terminator - */ - snprintf(extra, DNS_EDE_EXTRATEXT_LEN - 2, "%s ", what); - offset += strlen(extra); - dns_name_format(name, extra + offset, DNS_NAME_FORMATSIZE); - offset = strlcat(extra, "/", sizeof(extra)); - dns_rdatatype_format(type, extra + offset, - DNS_RDATATYPE_FORMATSIZE + 1); - - LOCK(&fctx->lock); - dns_ede_append(fctx->mctx, &fctx->edelist, info_code, extra); - UNLOCK(&fctx->lock) -} - -void -dns_resolver_copyede(dns_fetch_t *from, fetchctx_t *to) { - REQUIRE(DNS_FETCH_VALID(from)); - REQUIRE(VALID_FCTX(to)); - - LOCK(&to->lock); - for (dns_ede_t *ede = ISC_LIST_HEAD(from->private->edelist); - ede != NULL; ede = ISC_LIST_NEXT(ede, link)) - { - dns_ede_append(to->mctx, &to->edelist, ede->info_code, - ede->extra_text); - } - UNLOCK(&to->lock); -} diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 30a8b8de51..38da65f0f1 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -413,7 +414,7 @@ fetch_callback_dnskey(void *arg) { } validator_log(val, ISC_LOG_DEBUG(3), "in fetch_callback_dnskey"); - dns_resolver_copyede(val->fetch, val->fctx); + dns_resolver_destroyfetch(&val->fetch); if (CANCELED(val) || CANCELING(val)) { @@ -488,7 +489,7 @@ fetch_callback_ds(void *arg) { } validator_log(val, ISC_LOG_DEBUG(3), "in fetch_callback_ds"); - dns_resolver_copyede(val->fetch, val->fctx); + dns_resolver_destroyfetch(&val->fetch); if (CANCELED(val) || CANCELING(val)) { @@ -944,7 +945,7 @@ create_fetch(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, result = dns_resolver_createfetch( val->view->resolver, name, type, NULL, NULL, NULL, NULL, 0, fopts, 0, val->qc, val->gqc, val->loop, callback, val, - &val->frdataset, &val->fsigrdataset, &val->fetch); + val->edectx, &val->frdataset, &val->fsigrdataset, &val->fetch); if (result != ISC_R_SUCCESS) { dns_validator_detach(&val); } @@ -978,10 +979,10 @@ create_validator(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, (DNS_VALIDATOR_NOCDFLAG | DNS_VALIDATOR_NONTA)); validator_logcreate(val, name, type, caller, "validator"); - result = dns_validator_create(val->view, name, type, rdataset, sig, - NULL, vopts, val->loop, cb, val, - val->nvalidations, val->nfails, val->qc, - val->gqc, val->fctx, &val->subvalidator); + result = dns_validator_create( + val->view, name, type, rdataset, sig, NULL, vopts, val->loop, + cb, val, val->nvalidations, val->nfails, val->qc, val->gqc, + val->edectx, &val->subvalidator); if (result == ISC_R_SUCCESS) { dns_validator_attach(val, &val->subvalidator->parent); val->subvalidator->depth = val->depth + 1; @@ -3420,8 +3421,8 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, dns_message_t *message, unsigned int options, isc_loop_t *loop, isc_job_cb cb, void *arg, uint32_t *nvalidations, uint32_t *nfails, - isc_counter_t *qc, isc_counter_t *gqc, fetchctx_t *fctx, - dns_validator_t **validatorp) { + isc_counter_t *qc, isc_counter_t *gqc, + dns_edectx_t *edectx, dns_validator_t **validatorp) { isc_result_t result = ISC_R_FAILURE; dns_validator_t *val = NULL; dns_keytable_t *kt = NULL; @@ -3453,7 +3454,7 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, .rdata = DNS_RDATA_INIT, .nvalidations = nvalidations, .nfails = nfails, - .fctx = fctx, + .edectx = edectx, }; isc_refcount_init(&val->references, 1); @@ -3665,20 +3666,43 @@ validator_logcreate(dns_validator_t *val, dns_name_t *name, static void validate_extendederror(dns_validator_t *val) { - char txt[32]; - REQUIRE(VALID_VALIDATOR(val)); + char extra[DNS_NAME_FORMATSIZE + DNS_RDATATYPE_FORMATSIZE + + DNS_EDE_EXTRATEXT_LEN]; + isc_buffer_t b; + dns_validator_t *edeval = val; + + while (edeval->parent != NULL) { + edeval = edeval->parent; + } + if (val->unsupported_algorithm != 0) { - dns_secalg_format(val->unsupported_algorithm, txt, sizeof(txt)); - dns_resolver_edeappend(val->fctx, DNS_EDE_DNSKEYALG, txt, - val->name, val->type); + isc_buffer_init(&b, extra, sizeof(extra)); + dns_secalg_totext(val->unsupported_algorithm, &b); + + isc_buffer_putuint8(&b, ' '); + dns_name_totext(val->name, DNS_NAME_OMITFINALDOT, &b); + isc_buffer_putuint8(&b, '/'); + dns_rdatatype_totext(val->type, &b); + isc_buffer_putuint8(&b, '\0'); + + dns_ede_add(val->edectx, DNS_EDE_DNSKEYALG, extra); } if (val->unsupported_digest != 0) { - dns_dsdigest_format(val->unsupported_digest, txt, sizeof(txt)); - dns_resolver_edeappend(val->fctx, DNS_EDE_DSDIGESTTYPE, txt, - val->name, val->type); + isc_buffer_init(&b, extra, sizeof(extra)); + + dns_dsdigest_totext(val->unsupported_digest, &b); + isc_buffer_putuint8(&b, ' '); + dns_name_totext(val->name, DNS_NAME_OMITFINALDOT, &b); + isc_buffer_putuint8(&b, '/'); + dns_rdatatype_totext(val->type, &b); + isc_buffer_putuint8(&b, '\0'); + + dns_ede_add(val->edectx, DNS_EDE_DSDIGESTTYPE, extra); + + isc_buffer_invalidate(&b); } } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index e8f824359d..19c9f97523 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -10895,7 +10895,8 @@ do_keyfetch(void *arg) { result = dns_resolver_createfetch( resolver, kname, dns_rdatatype_dnskey, NULL, NULL, NULL, NULL, 0, options, 0, NULL, NULL, zone->loop, keyfetch_done, kfetch, - &kfetch->dnskeyset, &kfetch->dnskeysigset, &kfetch->fetch); + NULL, &kfetch->dnskeyset, &kfetch->dnskeysigset, + &kfetch->fetch); dns_resolver_detach(&resolver); if (result == ISC_R_SUCCESS) { @@ -21846,7 +21847,7 @@ do_nsfetch(void *arg) { result = dns_resolver_createfetch( resolver, &nsfetch->pname, dns_rdatatype_ns, NULL, NULL, NULL, NULL, 0, options, 0, NULL, NULL, zone->loop, nsfetch_done, - nsfetch, &nsfetch->nsrrset, &nsfetch->nssigset, + nsfetch, NULL, &nsfetch->nsrrset, &nsfetch->nssigset, &nsfetch->fetch); dns_resolver_detach(&resolver); diff --git a/lib/ns/client.c b/lib/ns/client.c index 63b806d522..0121f9aea9 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -223,91 +223,6 @@ ns_client_settimeout(ns_client_t *client, unsigned int seconds) { /* XXXWPK TODO use netmgr to set timeout */ } -static void -client_extendederror_reset(ns_client_t *client) { - for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { - if (client->ede[i]) { - dns_ednsopt_t *ede = client->ede[i]; - - isc_mem_put(client->manager->mctx, ede->value, - ede->length); - isc_mem_put(client->manager->mctx, ede, sizeof(*ede)); - client->ede[i] = NULL; - } - } -} - -void -ns_client_extendederror(ns_client_t *client, uint16_t code, const char *text) { - const uint16_t codelen = sizeof(code); - uint16_t textlen = 0; - size_t pos = 0; - unsigned char *ede = NULL; - dns_ednsopt_t *edns = NULL; - - REQUIRE(NS_CLIENT_VALID(client)); - - /* - * As ede will be directly put in the DNS message we need to make sure - * the code is in big-endian format - */ - code = htobe16(code); - - for (pos = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { - edns = client->ede[pos]; - - if (edns == NULL) { - break; - } - - if (memcmp(&code, edns->value, sizeof(code)) == 0) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), - "ignoring duplicate ede %u %s", code, - text == NULL ? "(null)" : text); - return; - } - } - - if (pos >= DNS_EDE_MAX_ERRORS) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), - "too many ede, ignoring %u %s", code, - text == NULL ? "(null)" : text); - return; - } - - ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, - ISC_LOG_DEBUG(1), "set ede: info-code %u extra-text %s", - code, text == NULL ? "(null)" : text); - - if (text != NULL && strlen(text) > 0) { - textlen = strlen(text); - - if (textlen > DNS_EDE_EXTRATEXT_LEN) { - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), - "truncate EDE code %hu text: %s", code, - text); - textlen = DNS_EDE_EXTRATEXT_LEN; - } - } - - ede = isc_mem_get(client->manager->mctx, codelen + textlen); - - memcpy(ede, &code, sizeof(code)); - if (textlen > 0) { - memcpy(ede + codelen, text, textlen); - } - - edns = isc_mem_get(client->manager->mctx, sizeof(*edns)); - *edns = (dns_ednsopt_t){ .code = DNS_OPT_EDE, - .length = codelen + textlen, - .value = ede }; - - client->ede[pos] = edns; -} - static void ns_client_endrequest(ns_client_t *client) { INSIST(client->state == NS_CLIENTSTATE_WORKING || @@ -348,7 +263,6 @@ ns_client_endrequest(ns_client_t *client) { dns_message_puttemprdataset(client->message, &client->opt); } - client_extendederror_reset(client); client->signer = NULL; client->udpsize = 512; client->extflags = 0; @@ -1268,7 +1182,7 @@ no_nsid: } for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { - dns_ednsopt_t *ede = client->ede[i]; + dns_ednsopt_t *ede = client->edectx.ede[i]; if (ede == NULL) { break; @@ -1785,7 +1699,7 @@ ns__client_put_cb(void *client0) { * Call this first because it requires a valid client. */ ns_query_free(client); - client_extendederror_reset(client); + dns_ede_invalidate(&client->edectx); client->magic = 0; @@ -2250,7 +2164,7 @@ ns_client_request_continue(void *arg) { "no matching view in class"); } - ns_client_extendederror(client, DNS_EDE_PROHIBITED, NULL); + dns_ede_add(&client->edectx, DNS_EDE_PROHIBITED, NULL); ns_client_error(client, DNS_R_REFUSED); goto cleanup; @@ -2581,6 +2495,8 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { */ client->magic = NS_CLIENT_MAGIC; ns_query_init(client); + + dns_ede_init(client->manager->mctx, &client->edectx); } else { REQUIRE(NS_CLIENT_VALID(client)); REQUIRE(client->manager->tid == isc_tid()); @@ -2593,8 +2509,11 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { .magic = 0, .manager = client->manager, .message = client->message, + .edectx = client->edectx, .query = client->query, }; + + dns_ede_reset(&client->edectx); } client->query.attributes &= ~NS_QUERYATTR_ANSWERED; @@ -2769,7 +2688,7 @@ ns_client_checkacl(ns_client_t *client, isc_sockaddr_t *sockaddr, NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(3), "%s approved", opname); } else { - ns_client_extendederror(client, DNS_EDE_PROHIBITED, NULL); + dns_ede_add(&client->edectx, DNS_EDE_PROHIBITED, NULL); ns_client_log(client, DNS_LOGCATEGORY_SECURITY, NS_LOGMODULE_CLIENT, log_level, "%s denied", opname); diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index e62fd8db07..440febcc05 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -180,7 +180,7 @@ struct ns_client { size_t tcpbuf_size; dns_message_t *message; dns_rdataset_t *opt; - dns_ednsopt_t *ede[DNS_EDE_MAX_ERRORS]; + dns_edectx_t edectx; uint16_t udpsize; uint16_t extflags; int16_t ednsversion; /* -1 noedns */ @@ -302,12 +302,6 @@ ns_client_error(ns_client_t *client, isc_result_t result); * will have an RCODE determined by 'result'. */ -void -ns_client_extendederror(ns_client_t *client, uint16_t code, const char *text); -/*%< - * Set extended error with INFO-CODE and EXTRA-TEXT . - */ - void ns_client_drop(ns_client_t *client, isc_result_t result); /*%< diff --git a/lib/ns/query.c b/lib/ns/query.c index c7b95818d1..0c426d3167 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -42,6 +42,7 @@ #include #include #include +#include #include #include #include @@ -902,7 +903,7 @@ ns_query_init(ns_client_t *client) { ISC_LIST_INIT(client->query.namebufs); ISC_LIST_INIT(client->query.activeversions); ISC_LIST_INIT(client->query.freeversions); - memset(client->ede, 0, sizeof(dns_ednsopt_t *) * DNS_EDE_MAX_ERRORS); + /* * This mutex is destroyed when the client is destroyed in * exit_check(). @@ -985,8 +986,7 @@ query_checkcacheaccess(ns_client_t *client, const dns_name_t *name, * since it is cleared by query_reset(), before query * processing starts. */ - ns_client_extendederror(client, DNS_EDE_PROHIBITED, - NULL); + dns_ede_add(&client->edectx, DNS_EDE_PROHIBITED, NULL); if (!options.nolog) { ns_client_aclmsg("query (cache)", name, qtype, @@ -1125,8 +1125,7 @@ query_validatezonedb(ns_client_t *client, const dns_name_t *name, ns_client_log(client, DNS_LOGCATEGORY_SECURITY, NS_LOGMODULE_QUERY, ISC_LOG_INFO, "%s denied", msg); - ns_client_extendederror(client, DNS_EDE_PROHIBITED, - NULL); + dns_ede_add(&client->edectx, DNS_EDE_PROHIBITED, NULL); } } @@ -1156,8 +1155,7 @@ query_validatezonedb(ns_client_t *client, const dns_name_t *name, result = ns_client_checkaclsilent(client, &client->destaddr, queryonacl, true); if (result != ISC_R_SUCCESS) { - ns_client_extendederror(client, DNS_EDE_PROHIBITED, - NULL); + dns_ede_add(&client->edectx, DNS_EDE_PROHIBITED, NULL); } if (!options.nolog && result != ISC_R_SUCCESS) { ns_client_log(client, DNS_LOGCATEGORY_SECURITY, @@ -2513,8 +2511,8 @@ validate(ns_client_t *client, dns_db_t *db, dns_name_t *name, isc_buffer_putstr(&buffer, " (cached)"); isc_buffer_putuint8(&buffer, 0); - ns_client_extendederror(client, DNS_EDE_DNSKEYALG, - isc_buffer_base(&buffer)); + dns_ede_add(&client->edectx, DNS_EDE_DNSKEYALG, + isc_buffer_base(&buffer)); continue; } if (!dns_name_issubdomain(name, &rrsig.signer)) { @@ -2823,7 +2821,7 @@ fetch_and_forget(ns_client_t *client, dns_name_t *qname, dns_rdatatype_t qtype, client->view->resolver, qname, qtype, NULL, NULL, NULL, peeraddr, client->message->id, options, 0, NULL, client->query.qc, client->manager->loop, cb, client, - tmprdataset, NULL, fetchp); + &client->edectx, tmprdataset, NULL, fetchp); if (result != ISC_R_SUCCESS) { ns_client_putrdataset(client, &tmprdataset); isc_nmhandle_detach(handlep); @@ -6166,8 +6164,8 @@ query_lookup(query_ctx_t *qctx) { stale_found ? "used" : "unavailable", isc_result_totext(result)); if (stale_found) { - ns_client_extendederror(qctx->client, ede, - "resolver failure"); + dns_ede_add(&qctx->client->edectx, ede, + "resolver failure"); } else if (!answer_found) { /* * Resolver failure, no stale data, nothing more we @@ -6190,9 +6188,8 @@ query_lookup(query_ctx_t *qctx) { isc_result_totext(result)); if (stale_found) { - ns_client_extendederror( - qctx->client, ede, - "query within stale refresh time window"); + dns_ede_add(&qctx->client->edectx, ede, + "query within stale refresh time window"); } else if (!answer_found) { /* * During the stale refresh window explicitly do not try @@ -6236,8 +6233,8 @@ query_lookup(query_ctx_t *qctx) { namebuf, typebuf); qctx->refresh_rrset = STALE(qctx->rdataset); if (stale_found) { - ns_client_extendederror( - qctx->client, ede, + dns_ede_add( + &qctx->client->edectx, ede, "stale data prioritized over " "lookup"); } @@ -6385,13 +6382,6 @@ fetch_callback(void *arg) { client->query.attributes &= ~NS_QUERYATTR_RECURSING; client->state = NS_CLIENTSTATE_WORKING; - for (dns_ede_t *ede = ISC_LIST_HEAD(resp->edelist); ede != NULL; - ede = ISC_LIST_NEXT(ede, link)) - { - ns_client_extendederror(client, ede->info_code, - ede->extra_text); - } - /* * Initialize a new qctx and use it to either resume from * recursion or clean up after cancelation. Transfer @@ -6610,7 +6600,7 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, client->view->resolver, qname, qtype, qdomain, nameservers, NULL, peeraddr, client->message->id, client->query.fetchoptions, 0, NULL, client->query.qc, client->manager->loop, - fetch_callback, client, rdataset, sigrdataset, + fetch_callback, client, &client->edectx, rdataset, sigrdataset, &FETCH_RECTYPE_NORMAL(client)); if (result != ISC_R_SUCCESS) { release_recursionquota(client); @@ -7475,8 +7465,8 @@ query_checkrpz(query_ctx_t *qctx, isc_result_t result) { if (qctx->rpz_st->m.rpz->ede != 0 && qctx->rpz_st->m.rpz->ede != UINT16_MAX) { - ns_client_extendederror(qctx->client, - qctx->rpz_st->m.rpz->ede, NULL); + dns_ede_add(&qctx->client->edectx, + qctx->rpz_st->m.rpz->ede, NULL); } /* @@ -11732,8 +11722,8 @@ ns_query_done(query_ctx_t *qctx) { */ partial_result_with_servfail = true; - ns_client_extendederror(qctx->client, 0, - "max. restarts reached"); + dns_ede_add(&qctx->client->edectx, 0, + "max. restarts reached"); ns_client_log(qctx->client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_QUERY, ISC_LOG_INFO, "query iterations limit reached"); diff --git a/tests/dns/ede_test.c b/tests/dns/ede_test.c index 74194a4ee2..db1340f519 100644 --- a/tests/dns/ede_test.c +++ b/tests/dns/ede_test.c @@ -15,65 +15,77 @@ #include /* IWYU pragma: keep */ #include #include +#include #include +#include #include #include #define UNIT_TESTING #include -#include -#include -#include -#include -#include +#include -#include +#include + +#include "../../lib/dns/ede.c" #include -ISC_RUN_TEST_IMPL(ede_enqueue_unlink) { - dns_edelist_t list; - dns_ede_t *ede = NULL; - const char *msg1 = "abcd"; - const char *msg2 = "abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabc" - "dabcdabcdadcdabcd"; +const struct { + uint16_t info_code; + char *extra_text; +} vectors[DNS_EDE_MAX_ERRORS] = { + { + 22, + NULL, + }, + { + 12, + (char *)"abcd", + }, + { + 4, + (char *)"abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabc" + "dabcdabcdadcdabcd", + }, +}; - ISC_LIST_INIT(list); +ISC_RUN_TEST_IMPL(dns_edectx) { + dns_edectx_t edectx = { 0 }; + size_t pos = 0; + uint16_t becode; + uint8_t buf[sizeof(becode) + DNS_EDE_EXTRATEXT_LEN]; - dns_ede_append(mctx, &list, 22, NULL); - dns_ede_append(mctx, &list, 12, msg1); - dns_ede_append(mctx, &list, 4, msg2); + dns_ede_init(mctx, &edectx); - ede = ISC_LIST_HEAD(list); - assert_non_null(ede); - assert_int_equal(ede->info_code, 22); - assert_null(ede->extra_text); + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + dns_ede_add(&edectx, vectors[i].info_code, + vectors[i].extra_text); + } - ede = ISC_LIST_NEXT(ede, link); - assert_non_null(ede); - assert_int_equal(ede->info_code, 12); - assert_string_equal(ede->extra_text, msg1); - assert_ptr_not_equal(ede->extra_text, msg1); + for (size_t i = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { + dns_ednsopt_t *edns = edectx.ede[i]; + size_t textlen = 0; - /* - * Even though we limit the length of an EDE message to 64 bytes, - * this is done only at the ns/client.c level (to make sure to cover all - * the flows). - */ - ede = ISC_LIST_NEXT(ede, link); - assert_non_null(ede); - assert_int_equal(ede->info_code, 4); - assert_string_equal(ede->extra_text, msg2); - assert_ptr_not_equal(ede->extra_text, msg2); + becode = htobe16(vectors[i].info_code); + memmove(buf, &becode, sizeof(becode)); + if (vectors[i].extra_text != NULL) { + textlen = strlen(vectors[i].extra_text); + memcpy(edns->value + sizeof(becode), + vectors[i].extra_text, textlen); + } - dns_ede_unlinkall(mctx, &list); - assert_true(ISC_LIST_EMPTY(list)); + assert_memory_equal(buf, edectx.ede[i]->value, + sizeof(becode) + textlen); + } + + dns_ede_reset(&edectx); } ISC_TEST_LIST_START -ISC_TEST_ENTRY(ede_enqueue_unlink) +ISC_TEST_ENTRY(dns_edectx) ISC_TEST_LIST_END diff --git a/tests/ns/client_test.c b/tests/ns/client_test.c index 83a174c1d4..b19f8b8ed3 100644 --- a/tests/ns/client_test.c +++ b/tests/ns/client_test.c @@ -49,20 +49,14 @@ client_ede_test_initclient(void) { client->magic = NS_CLIENT_MAGIC; client->manager = &client_ede_test_dummy_manager; + dns_ede_init(mctx, &client->edectx); + return client; } static void client_ede_test_free(ns_client_t *client) { - for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { - dns_ednsopt_t *ede = client->ede[i]; - - if (ede) { - isc_mem_put(mctx, ede->value, ede->length); - isc_mem_put(mctx, ede, sizeof(*ede)); - } - client->ede[i] = NULL; - } + dns_ede_reset(&client->edectx); isc_mem_put(mctx, client, sizeof(*client)); } @@ -72,14 +66,13 @@ client_ede_test_equals(const client_tests_ede_expected_t *expected, size_t count = 0; for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { - dns_ednsopt_t *edns = client->ede[i]; + dns_ednsopt_t *edns = client->edectx.ede[i]; if (edns == NULL) { break; } uint16_t code; - const size_t codelen = sizeof(code); const unsigned char *txt; assert_in_range(count, 0, expected_count); @@ -88,11 +81,11 @@ client_ede_test_equals(const client_tests_ede_expected_t *expected, code = ISC_U8TO16_BE(edns->value); assert_int_equal(code, expected[count].code); - if (edns->length > codelen) { + if (edns->length > sizeof(code)) { assert_non_null(expected[count].txt); - txt = edns->value + codelen; + txt = edns->value + sizeof(code); assert_memory_equal(expected[count].txt, txt, - edns->length - codelen); + edns->length - sizeof(code)); } else { assert_null(expected[count].txt); } @@ -109,9 +102,9 @@ ISC_RUN_TEST_IMPL(client_ede_test_text_max_count) { const char *txt2 = "It's been a long time since I rock-and-rolled" "Ooh, let me get it back, let me get it back"; - ns_client_extendederror(client, 2, txt1); - ns_client_extendederror(client, 22, NULL); - ns_client_extendederror(client, 3, txt2); + dns_ede_add(&client->edectx, 2, txt1); + dns_ede_add(&client->edectx, 22, NULL); + dns_ede_add(&client->edectx, 3, txt2); const client_tests_ede_expected_t expected[3] = { { .code = 2, .txt = "foobar" }, @@ -128,11 +121,11 @@ ISC_RUN_TEST_IMPL(client_ede_test_text_max_count) { ISC_RUN_TEST_IMPL(client_ede_test_max_count) { ns_client_t *client = client_ede_test_initclient(); - ns_client_extendederror(client, 1, NULL); - ns_client_extendederror(client, 22, "two"); - ns_client_extendederror(client, 3, "three"); - ns_client_extendederror(client, 4, "four"); - ns_client_extendederror(client, 5, "five"); + dns_ede_add(&client->edectx, 1, NULL); + dns_ede_add(&client->edectx, 22, "two"); + dns_ede_add(&client->edectx, 3, "three"); + dns_ede_add(&client->edectx, 4, "four"); + dns_ede_add(&client->edectx, 5, "five"); const client_tests_ede_expected_t expected[3] = { { .code = 1, .txt = NULL }, @@ -147,9 +140,9 @@ ISC_RUN_TEST_IMPL(client_ede_test_max_count) { ISC_RUN_TEST_IMPL(client_ede_test_duplicates) { ns_client_t *client = client_ede_test_initclient(); - ns_client_extendederror(client, 1, NULL); - ns_client_extendederror(client, 1, "two"); - ns_client_extendederror(client, 1, "three"); + dns_ede_add(&client->edectx, 1, NULL); + dns_ede_add(&client->edectx, 1, "two"); + dns_ede_add(&client->edectx, 1, "three"); const client_tests_ede_expected_t expected[] = { { .code = 1, .txt = NULL }, From 7e3a650ae221b697d95cfb231a34c0ad77194bef Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Wed, 29 Jan 2025 18:34:51 +0100 Subject: [PATCH 2/7] Refactor test covering dns_ede API Migrate tests cases in client_test code which were exclusively testing code which is now all wrapped inside ede compilation unit. Those are testing maximum number of EDE, duplicate EDE as well as truncation of text of an EDE. Also add coverage for the copy of EDE from an edectx to another one, as well as checking the assertion of the maximum EDE info code which can be used. (cherry picked from commit f9f41190b397429b151e48dac369ab5797ed6082) --- lib/dns/ede.c | 1 + lib/dns/include/dns/message.h | 1 + tests/dns/ede_test.c | 184 ++++++++++++++++++++++++++-------- tests/ns/Makefile.am | 3 +- tests/ns/client_test.c | 164 ------------------------------ 5 files changed, 146 insertions(+), 207 deletions(-) delete mode 100644 tests/ns/client_test.c diff --git a/lib/dns/ede.c b/lib/dns/ede.c index d5c7e2da48..7dcb05f281 100644 --- a/lib/dns/ede.c +++ b/lib/dns/ede.c @@ -24,6 +24,7 @@ void dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text) { REQUIRE(DNS_EDE_VALID(edectx)); + REQUIRE(code <= DNS_EDE_MAX_CODE); size_t pos = 0; uint16_t becode = htobe16(code); diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index 6de8b3d3f4..e97c1a0674 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -150,6 +150,7 @@ #define DNS_EDE_NOREACHABLEAUTH 22 /*%< No Reachable Authority */ #define DNS_EDE_NETWORKERROR 23 /*%< Network Error */ #define DNS_EDE_INVALIDDATA 24 /*%< Invalid Data */ +#define DNS_EDE_MAX_CODE DNS_EDE_INVALIDDATA #define DNS_MESSAGE_REPLYPRESERVE (DNS_MESSAGEFLAG_RD | DNS_MESSAGEFLAG_CD) #define DNS_MESSAGEEXTFLAG_REPLYPRESERVE (DNS_MESSAGEEXTFLAG_DO) diff --git a/tests/dns/ede_test.c b/tests/dns/ede_test.c index db1340f519..b509fc1c45 100644 --- a/tests/dns/ede_test.c +++ b/tests/dns/ede_test.c @@ -32,60 +32,162 @@ #include -const struct { - uint16_t info_code; - char *extra_text; -} vectors[DNS_EDE_MAX_ERRORS] = { - { - 22, - NULL, - }, - { - 12, - (char *)"abcd", - }, - { - 4, - (char *)"abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabc" - "dabcdabcdadcdabcd", - }, -}; +typedef struct { + uint16_t code; + const char *txt; +} ede_test_expected_t; -ISC_RUN_TEST_IMPL(dns_edectx) { - dns_edectx_t edectx = { 0 }; - size_t pos = 0; - uint16_t becode; - uint8_t buf[sizeof(becode) + DNS_EDE_EXTRATEXT_LEN]; +static void +dns_ede_test_equals(const ede_test_expected_t *expected, size_t expected_count, + dns_edectx_t *edectx) { + size_t count = 0; + + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + dns_ednsopt_t *edns = edectx->ede[i]; + + if (edns == NULL) { + break; + } + + uint16_t code; + const unsigned char *txt; + + assert_in_range(count, 0, expected_count); + assert_int_equal(edns->code, DNS_OPT_EDE); + + code = ISC_U8TO16_BE(edns->value); + assert_int_equal(code, expected[count].code); + + if (edns->length > sizeof(code)) { + assert_non_null(expected[count].txt); + txt = edns->value + sizeof(code); + assert_memory_equal(expected[count].txt, txt, + edns->length - sizeof(code)); + } else { + assert_null(expected[count].txt); + } + + count++; + } + assert_int_equal(count, expected_count); +} + +ISC_RUN_TEST_IMPL(dns_ede_test_text_max_count) { + dns_edectx_t edectx; dns_ede_init(mctx, &edectx); - for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { - dns_ede_add(&edectx, vectors[i].info_code, - vectors[i].extra_text); - } + const char *txt1 = "foobar"; + const char *txt2 = "It's been a long time since I rock-and-rolled" + "Ooh, let me get it back, let me get it back"; - for (size_t i = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { - dns_ednsopt_t *edns = edectx.ede[i]; - size_t textlen = 0; + dns_ede_add(&edectx, 2, txt1); + dns_ede_add(&edectx, 22, NULL); + dns_ede_add(&edectx, 3, txt2); - becode = htobe16(vectors[i].info_code); - memmove(buf, &becode, sizeof(becode)); - if (vectors[i].extra_text != NULL) { - textlen = strlen(vectors[i].extra_text); - memcpy(edns->value + sizeof(becode), - vectors[i].extra_text, textlen); - } + const ede_test_expected_t expected[3] = { + { .code = 2, .txt = "foobar" }, + { .code = 22, .txt = NULL }, + { .code = 3, + .txt = "It's been a long time since I rock-and-rolledOoh, " + "let me get it " } + }; - assert_memory_equal(buf, edectx.ede[i]->value, - sizeof(becode) + textlen); - } + dns_ede_test_equals(expected, 3, &edectx); dns_ede_reset(&edectx); } +ISC_RUN_TEST_IMPL(dns_ede_test_max_count) { + dns_edectx_t edectx; + + dns_ede_init(mctx, &edectx); + + dns_ede_add(&edectx, 1, NULL); + dns_ede_add(&edectx, 22, "two"); + dns_ede_add(&edectx, 3, "three"); + dns_ede_add(&edectx, 4, "four"); + dns_ede_add(&edectx, 5, "five"); + + const ede_test_expected_t expected[3] = { + { .code = 1, .txt = NULL }, + { .code = 22, .txt = "two" }, + { .code = 3, .txt = "three" }, + }; + + dns_ede_test_equals(expected, 3, &edectx); + + dns_ede_reset(&edectx); +} + +ISC_RUN_TEST_IMPL(dns_ede_test_duplicates) { + dns_edectx_t edectx; + + dns_ede_init(mctx, &edectx); + + dns_ede_add(&edectx, 1, NULL); + dns_ede_add(&edectx, 1, "two"); + dns_ede_add(&edectx, 1, "three"); + + const ede_test_expected_t expected[] = { + { .code = 1, .txt = NULL }, + }; + dns_ede_test_equals(expected, 1, &edectx); + + dns_ede_reset(&edectx); + + const ede_test_expected_t expectedempty[] = {}; + dns_ede_test_equals(expectedempty, 0, &edectx); +} + +ISC_RUN_TEST_IMPL(dns_ede_test_infocode_range) { + dns_edectx_t edectx; + + dns_ede_init(mctx, &edectx); + + dns_ede_add(&edectx, 1, NULL); + expect_assert_failure(dns_ede_add(&edectx, 32, NULL)); + + const ede_test_expected_t expected[] = { + { .code = 1, .txt = NULL }, + }; + dns_ede_test_equals(expected, 1, &edectx); + + dns_ede_reset(&edectx); +} + +ISC_RUN_TEST_IMPL(dns_ede_test_copy) { + dns_edectx_t edectx1; + dns_edectx_t edectx2; + + dns_ede_init(mctx, &edectx1); + dns_ede_init(mctx, &edectx2); + + dns_ede_add(&edectx1, 1, NULL); + dns_ede_add(&edectx1, 2, "two"); + dns_ede_add(&edectx1, 3, "three"); + + const ede_test_expected_t expected[] = { + { .code = 1, .txt = NULL }, + { .code = 2, .txt = "two" }, + { .code = 3, .txt = "three" }, + }; + + dns_ede_test_equals(expected, 3, &edectx1); + dns_ede_copy(&edectx2, &edectx1); + dns_ede_test_equals(expected, 3, &edectx2); + + dns_ede_reset(&edectx1); + dns_ede_reset(&edectx2); +} + ISC_TEST_LIST_START -ISC_TEST_ENTRY(dns_edectx) +ISC_TEST_ENTRY(dns_ede_test_text_max_count) +ISC_TEST_ENTRY(dns_ede_test_max_count) +ISC_TEST_ENTRY(dns_ede_test_duplicates) +ISC_TEST_ENTRY(dns_ede_test_infocode_range) +ISC_TEST_ENTRY(dns_ede_test_copy) ISC_TEST_LIST_END diff --git a/tests/ns/Makefile.am b/tests/ns/Makefile.am index 23b3ccbf7b..c26bec85e6 100644 --- a/tests/ns/Makefile.am +++ b/tests/ns/Makefile.am @@ -18,8 +18,7 @@ check_PROGRAMS = \ listenlist_test \ notify_test \ plugin_test \ - query_test \ - client_test + query_test notify_test_SOURCES = \ notify_test.c \ diff --git a/tests/ns/client_test.c b/tests/ns/client_test.c deleted file mode 100644 index b19f8b8ed3..0000000000 --- a/tests/ns/client_test.c +++ /dev/null @@ -1,164 +0,0 @@ -/* - * Copyright (C) Internet Systems Consortium, Inc. ("ISC") - * - * SPDX-License-Identifier: MPL-2.0 - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * See the COPYRIGHT file distributed with this work for additional - * information regarding copyright ownership. - */ - -#include -#include /* IWYU pragma: keep */ -#include -#include -#include -#include -#include - -#define UNIT_TESTING -#include - -#include -#include -#include -#include -#include -#include - -#include - -#include - -typedef struct { - uint16_t code; - const char *txt; -} client_tests_ede_expected_t; - -static ns_clientmgr_t client_ede_test_dummy_manager; - -static ns_client_t * -client_ede_test_initclient(void) { - client_ede_test_dummy_manager.mctx = mctx; - - ns_client_t *client = isc_mem_get(mctx, sizeof(*client)); - memset(client, 0, sizeof(*client)); - client->magic = NS_CLIENT_MAGIC; - client->manager = &client_ede_test_dummy_manager; - - dns_ede_init(mctx, &client->edectx); - - return client; -} - -static void -client_ede_test_free(ns_client_t *client) { - dns_ede_reset(&client->edectx); - isc_mem_put(mctx, client, sizeof(*client)); -} - -static void -client_ede_test_equals(const client_tests_ede_expected_t *expected, - size_t expected_count, const ns_client_t *client) { - size_t count = 0; - - for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { - dns_ednsopt_t *edns = client->edectx.ede[i]; - - if (edns == NULL) { - break; - } - - uint16_t code; - const unsigned char *txt; - - assert_in_range(count, 0, expected_count); - assert_int_equal(edns->code, DNS_OPT_EDE); - - code = ISC_U8TO16_BE(edns->value); - assert_int_equal(code, expected[count].code); - - if (edns->length > sizeof(code)) { - assert_non_null(expected[count].txt); - txt = edns->value + sizeof(code); - assert_memory_equal(expected[count].txt, txt, - edns->length - sizeof(code)); - } else { - assert_null(expected[count].txt); - } - - count++; - } - assert_int_equal(count, expected_count); -} - -ISC_RUN_TEST_IMPL(client_ede_test_text_max_count) { - ns_client_t *client = client_ede_test_initclient(); - - const char *txt1 = "foobar"; - const char *txt2 = "It's been a long time since I rock-and-rolled" - "Ooh, let me get it back, let me get it back"; - - dns_ede_add(&client->edectx, 2, txt1); - dns_ede_add(&client->edectx, 22, NULL); - dns_ede_add(&client->edectx, 3, txt2); - - const client_tests_ede_expected_t expected[3] = { - { .code = 2, .txt = "foobar" }, - { .code = 22, .txt = NULL }, - { .code = 3, - .txt = "It's been a long time since I rock-and-rolledOoh, " - "let me get it " } - }; - - client_ede_test_equals(expected, 3, client); - client_ede_test_free(client); -} - -ISC_RUN_TEST_IMPL(client_ede_test_max_count) { - ns_client_t *client = client_ede_test_initclient(); - - dns_ede_add(&client->edectx, 1, NULL); - dns_ede_add(&client->edectx, 22, "two"); - dns_ede_add(&client->edectx, 3, "three"); - dns_ede_add(&client->edectx, 4, "four"); - dns_ede_add(&client->edectx, 5, "five"); - - const client_tests_ede_expected_t expected[3] = { - { .code = 1, .txt = NULL }, - { .code = 22, .txt = "two" }, - { .code = 3, .txt = "three" }, - }; - - client_ede_test_equals(expected, 3, client); - client_ede_test_free(client); -} - -ISC_RUN_TEST_IMPL(client_ede_test_duplicates) { - ns_client_t *client = client_ede_test_initclient(); - - dns_ede_add(&client->edectx, 1, NULL); - dns_ede_add(&client->edectx, 1, "two"); - dns_ede_add(&client->edectx, 1, "three"); - - const client_tests_ede_expected_t expected[] = { - { .code = 1, .txt = NULL }, - }; - - client_ede_test_equals(expected, 1, client); - - client_ede_test_free(client); -} - -ISC_TEST_LIST_START - -ISC_TEST_ENTRY(client_ede_test_text_max_count) -ISC_TEST_ENTRY(client_ede_test_max_count) -ISC_TEST_ENTRY(client_ede_test_duplicates) - -ISC_TEST_LIST_END - -ISC_TEST_MAIN From f390108f8cc04ac6ea49faa6deda169b1ed3c9b9 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Wed, 29 Jan 2025 22:32:33 +0100 Subject: [PATCH 3/7] add lib/dns/ede.c documentation Add documentation usage of EDE compilation unit as well as centralize all EDE-related macros in the same lib/dns/include/dns/ede.h header. (cherry picked from commit 7b01cbfb0413257318d534d2ee320e28a5dd52a4) --- lib/dns/include/dns/ede.h | 94 ++++++++++++++++++++++++++++++++++- lib/dns/include/dns/message.h | 29 +---------- 2 files changed, 94 insertions(+), 29 deletions(-) diff --git a/lib/dns/include/dns/ede.h b/lib/dns/include/dns/ede.h index b14f16a715..8f6cf3bf9a 100644 --- a/lib/dns/include/dns/ede.h +++ b/lib/dns/include/dns/ede.h @@ -17,6 +17,35 @@ #include +/*%< EDNS0 extended DNS errors */ +#define DNS_EDE_OTHER 0 /*%< Other Error */ +#define DNS_EDE_DNSKEYALG 1 /*%< Unsupported DNSKEY Algorithm */ +#define DNS_EDE_DSDIGESTTYPE 2 /*%< Unsupported DS Digest Type */ +#define DNS_EDE_STALEANSWER 3 /*%< Stale Answer */ +#define DNS_EDE_FORGEDANSWER 4 /*%< Forged Answer */ +#define DNS_EDE_DNSSECINDETERMINATE 5 /*%< DNSSEC Indeterminate */ +#define DNS_EDE_DNSSECBOGUS 6 /*%< DNSSEC Bogus */ +#define DNS_EDE_SIGNATUREEXPIRED 7 /*%< Signature Expired */ +#define DNS_EDE_SIGNATURENOTYETVALID 8 /*%< Signature Not Yet Valid */ +#define DNS_EDE_DNSKEYMISSING 9 /*%< DNSKEY Missing */ +#define DNS_EDE_RRSIGSMISSING 10 /*%< RRSIGs Missing */ +#define DNS_EDE_NOZONEKEYBITSET 11 /*%< No Zone Key Bit Set */ +#define DNS_EDE_NSECMISSING 12 /*%< NSEC Missing */ +#define DNS_EDE_CACHEDERROR 13 /*%< Cached Error */ +#define DNS_EDE_NOTREADY 14 /*%< Not Ready */ +#define DNS_EDE_BLOCKED 15 /*%< Blocked */ +#define DNS_EDE_CENSORED 16 /*%< Censored */ +#define DNS_EDE_FILTERED 17 /*%< Filtered */ +#define DNS_EDE_PROHIBITED 18 /*%< Prohibited */ +#define DNS_EDE_STALENXANSWER 19 /*%< Stale NXDomain Answer */ +#define DNS_EDE_NOTAUTH 20 /*%< Not Authoritative */ +#define DNS_EDE_NOTSUPPORTED 21 /*%< Not Supported */ +#define DNS_EDE_NOREACHABLEAUTH 22 /*%< No Reachable Authority */ +#define DNS_EDE_NETWORKERROR 23 /*%< Network Error */ +#define DNS_EDE_INVALIDDATA 24 /*%< Invalid Data */ + +#define DNS_EDE_MAX_CODE DNS_EDE_INVALIDDATA + /* * From RFC 8914: * Because long EXTRA-TEXT fields may trigger truncation (which is undesirable @@ -35,21 +64,84 @@ struct dns_edectx { isc_mem_t *mctx; dns_ednsopt_t *ede[DNS_EDE_MAX_ERRORS]; }; +/*%< + * Multiple extended DNS errors (EDE) (defined in RFC 8914) can be raised during + * a DNS resolution and in various area of the code base. "dns_edectx_t" object + * abstract and holds pending EDE and the set of dns_ede_ API enable to + * manipulate its state (adding EDE, transfer to another context, etc.). EDE are + * internally stored in the wire format, so it can be directly consumed to build + * the response client message. + */ void dns_ede_init(isc_mem_t *mctx, dns_edectx_t *edectx); +/*%< + * Initialize "edectx" so it is valid to use. Can be called after + * dns_ede_invalidate" is being called to reuse the object. + * + * Requires: + * + * \li "mctx" to be valid + * \li "edectx" to be valid + */ void dns_ede_reset(dns_edectx_t *edectx); +/*%< + * Reset "edectx" internal state and free all its EDE from memory. "edectx" is + * still valid to use, in the same state than after dns_ede_init is called. + * + * Requires: + * + * \li "edectx" to be valid + */ void dns_ede_invalidate(dns_edectx_t *edectx); +/*%< + * Reset "edectx" and remove its memory context as well as its magic number. It + * is not valid to use anymore. + * + * Requires: + * + * \li "edectx" to be valid + */ void dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text); /*%< - * Set extended error with INFO-CODE and EXTRA-TEXT . + * Add a new extended error in "edectx". "code" must be one of the INFO-CODE + * values defined in RFC8914, see DNS_EDE_ macros above. "text" is optional, it + * is immediately copied internally if provided. + * + * Rules: + * + * \li If "text" is non NULL, it must be NULL terminated. If its length is more + * than DNS_EDE_EXTRATEXT_LEN, it is trucated. + * + * \li If an EDE with the same code has already been added to "edectx", the + * following ones with the same code are ignored. + * + * \li If more than DNS_EDE_MAX_ERRORS EDE have been already added to this + * context, the following ones are ignored. + * + * Requires: + * + * \li "edectx" to be valid + * \li "code" to be one of the INFO-CODE defied in RFC8914, see DNS_EDE_ macros. */ void dns_ede_copy(dns_edectx_t *edectx_to, dns_edectx_t *edectx_from); +/*%< + * Copy all EDE from "edectx_from" into "edectx_to". If "edectx_to" reaches the + * maximum number of EDE (see DNS_EDE_MAX_ERRORS), the copy stops and + * remaining EDE in "edectx_from" are not copied. + * + * Rules defined in "dns_ede_add" applies. + * + * Requires: + * + * \li "edectx_from" to be valid + * \li "edectx_to" to be valid + */ diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index e97c1a0674..f9c82f023d 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -124,34 +125,6 @@ */ #define DNS_EDNSOPTIONS 7 + DNS_EDE_MAX_ERRORS -/*%< EDNS0 extended DNS errors */ -#define DNS_EDE_OTHER 0 /*%< Other Error */ -#define DNS_EDE_DNSKEYALG 1 /*%< Unsupported DNSKEY Algorithm */ -#define DNS_EDE_DSDIGESTTYPE 2 /*%< Unsupported DS Digest Type */ -#define DNS_EDE_STALEANSWER 3 /*%< Stale Answer */ -#define DNS_EDE_FORGEDANSWER 4 /*%< Forged Answer */ -#define DNS_EDE_DNSSECINDETERMINATE 5 /*%< DNSSEC Indeterminate */ -#define DNS_EDE_DNSSECBOGUS 6 /*%< DNSSEC Bogus */ -#define DNS_EDE_SIGNATUREEXPIRED 7 /*%< Signature Expired */ -#define DNS_EDE_SIGNATURENOTYETVALID 8 /*%< Signature Not Yet Valid */ -#define DNS_EDE_DNSKEYMISSING 9 /*%< DNSKEY Missing */ -#define DNS_EDE_RRSIGSMISSING 10 /*%< RRSIGs Missing */ -#define DNS_EDE_NOZONEKEYBITSET 11 /*%< No Zone Key Bit Set */ -#define DNS_EDE_NSECMISSING 12 /*%< NSEC Missing */ -#define DNS_EDE_CACHEDERROR 13 /*%< Cached Error */ -#define DNS_EDE_NOTREADY 14 /*%< Not Ready */ -#define DNS_EDE_BLOCKED 15 /*%< Blocked */ -#define DNS_EDE_CENSORED 16 /*%< Censored */ -#define DNS_EDE_FILTERED 17 /*%< Filtered */ -#define DNS_EDE_PROHIBITED 18 /*%< Prohibited */ -#define DNS_EDE_STALENXANSWER 19 /*%< Stale NXDomain Answer */ -#define DNS_EDE_NOTAUTH 20 /*%< Not Authoritative */ -#define DNS_EDE_NOTSUPPORTED 21 /*%< Not Supported */ -#define DNS_EDE_NOREACHABLEAUTH 22 /*%< No Reachable Authority */ -#define DNS_EDE_NETWORKERROR 23 /*%< Network Error */ -#define DNS_EDE_INVALIDDATA 24 /*%< Invalid Data */ -#define DNS_EDE_MAX_CODE DNS_EDE_INVALIDDATA - #define DNS_MESSAGE_REPLYPRESERVE (DNS_MESSAGEFLAG_RD | DNS_MESSAGEFLAG_CD) #define DNS_MESSAGEEXTFLAG_REPLYPRESERVE (DNS_MESSAGEEXTFLAG_DO) From e5fc9f5fcbd7f3d0dae8468bec1b801d5a692dba Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Wed, 29 Jan 2025 23:27:34 +0100 Subject: [PATCH 4/7] detect dup EDE with bitmap and store next pos In order to avoid to loop to find the next position to store an EDE in a dns_edectx_t, add a "nextede" state which holds the next available position. Also, in order ot avoid to loop to find if an EDE is already existing in a dns_edectx_t, and avoid a duplicate, use a bitmap to immediately know if the EDE is there or not. Those both changes applies for adding or copying EDE. Also make the direction of dns_ede_copy more explicit/avoid errors by making "edectx_from" a const pointer. (cherry picked from commit 9021f9d802296241dc69431407551d824beeb814) --- lib/dns/ede.c | 63 ++++++++++++++++++++------------------- lib/dns/include/dns/ede.h | 4 ++- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/lib/dns/ede.c b/lib/dns/ede.c index 7dcb05f281..7fbbd83905 100644 --- a/lib/dns/ede.c +++ b/lib/dns/ede.c @@ -21,38 +21,39 @@ #define DNS_EDE_MAGIC ISC_MAGIC('E', 'D', 'E', '!') #define DNS_EDE_VALID(v) ISC_MAGIC_VALID(v, DNS_EDE_MAGIC) +static bool +dns__ede_checkandupdateedeused(dns_edectx_t *edectx, uint16_t code) { + if (edectx->edeused & (1 << code)) { + return true; + } + + edectx->edeused |= 1 << code; + return false; +} + void dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text) { REQUIRE(DNS_EDE_VALID(edectx)); REQUIRE(code <= DNS_EDE_MAX_CODE); - size_t pos = 0; uint16_t becode = htobe16(code); dns_ednsopt_t *edns = NULL; size_t textlen = 0; - for (pos = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { - edns = edectx->ede[pos]; - - if (edns == NULL) { - break; - } - - if (memcmp(&becode, edns->value, sizeof(becode)) == 0) { - isc_log_write(DNS_LOGCATEGORY_RESOLVER, - DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), - "ignoring duplicate ede %u %s", code, - text == NULL ? "(null)" : text); - return; - } + if (dns__ede_checkandupdateedeused(edectx, code)) { + isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, + ISC_LOG_DEBUG(1), "ignoring duplicate ede %u %s", + code, text == NULL ? "(null)" : text); + return; } - if (pos >= DNS_EDE_MAX_ERRORS) { + if (edectx->nextede >= DNS_EDE_MAX_ERRORS) { isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), "too many ede, ignoring %u %s", code, text == NULL ? "(null)" : text); return; } + INSIST(edectx->ede[edectx->nextede] == NULL); isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), "set ede: info-code %u extra-text %s", @@ -83,7 +84,8 @@ dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text) { memmove(edns->value + sizeof(becode), text, textlen); } - edectx->ede[pos] = edns; + edectx->ede[edectx->nextede] = edns; + edectx->nextede++; } void @@ -113,6 +115,8 @@ dns_ede_reset(dns_edectx_t *edectx) { isc_mem_put(edectx->mctx, edns, sizeof(*edns) + edns->length); edectx->ede[i] = NULL; } + + dns_ede_init(edectx->mctx, edectx); } void @@ -126,31 +130,30 @@ dns_ede_invalidate(dns_edectx_t *edectx) { } void -dns_ede_copy(dns_edectx_t *edectx_to, dns_edectx_t *edectx_from) { +dns_ede_copy(dns_edectx_t *edectx_to, const dns_edectx_t *edectx_from) { REQUIRE(DNS_EDE_VALID(edectx_to)); REQUIRE(DNS_EDE_VALID(edectx_from)); - size_t nextede = 0; - - for (nextede = 0; nextede < DNS_EDE_MAX_ERRORS; nextede++) { - if (edectx_to->ede[nextede] == NULL) { - break; - } - } - for (size_t pos = 0; pos < DNS_EDE_MAX_ERRORS; pos++) { + uint16_t fromcode; + if (edectx_from->ede[pos] == NULL) { break; } - if (nextede >= DNS_EDE_MAX_ERRORS) { + fromcode = ISC_U8TO16_BE(edectx_from->ede[pos]->value); + if (dns__ede_checkandupdateedeused(edectx_to, fromcode)) { + continue; + } + + if (edectx_to->nextede >= DNS_EDE_MAX_ERRORS) { isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), "too many ede from subfetch"); break; } - INSIST(edectx_to->ede[nextede] == NULL); + INSIST(edectx_to->ede[edectx_to->nextede] == NULL); dns_ednsopt_t *edns = isc_mem_get( edectx_to->mctx, @@ -163,7 +166,7 @@ dns_ede_copy(dns_edectx_t *edectx_to, dns_edectx_t *edectx_from) { memmove(edns->value, edectx_from->ede[pos]->value, edectx_from->ede[pos]->length); - edectx_to->ede[nextede] = edns; - nextede++; + edectx_to->ede[edectx_to->nextede] = edns; + edectx_to->nextede++; } } diff --git a/lib/dns/include/dns/ede.h b/lib/dns/include/dns/ede.h index 8f6cf3bf9a..37194dd5ef 100644 --- a/lib/dns/include/dns/ede.h +++ b/lib/dns/include/dns/ede.h @@ -63,6 +63,8 @@ struct dns_edectx { int magic; isc_mem_t *mctx; dns_ednsopt_t *ede[DNS_EDE_MAX_ERRORS]; + uint32_t edeused; + size_t nextede; }; /*%< * Multiple extended DNS errors (EDE) (defined in RFC 8914) can be raised during @@ -132,7 +134,7 @@ dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text); */ void -dns_ede_copy(dns_edectx_t *edectx_to, dns_edectx_t *edectx_from); +dns_ede_copy(dns_edectx_t *edectx_to, const dns_edectx_t *edectx_from); /*%< * Copy all EDE from "edectx_from" into "edectx_to". If "edectx_to" reaches the * maximum number of EDE (see DNS_EDE_MAX_ERRORS), the copy stops and From 3bc6de926587804514cfdee06745a10a0a47f9d7 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Wed, 29 Jan 2025 23:30:07 +0100 Subject: [PATCH 5/7] update EDE copy and add dup tests Update EDE tests to exercise the bitmap and next ede index logic (cherry picked from commit c7b0fe5bec7a37ca69f359ab0de4d09d41c834b5) --- tests/dns/ede_test.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/tests/dns/ede_test.c b/tests/dns/ede_test.c index b509fc1c45..76761cfb8f 100644 --- a/tests/dns/ede_test.c +++ b/tests/dns/ede_test.c @@ -159,26 +159,54 @@ ISC_RUN_TEST_IMPL(dns_ede_test_infocode_range) { ISC_RUN_TEST_IMPL(dns_ede_test_copy) { dns_edectx_t edectx1; dns_edectx_t edectx2; + dns_edectx_t edectx3; dns_ede_init(mctx, &edectx1); dns_ede_init(mctx, &edectx2); dns_ede_add(&edectx1, 1, NULL); - dns_ede_add(&edectx1, 2, "two"); + dns_ede_add(&edectx1, 2, "two-the-first"); dns_ede_add(&edectx1, 3, "three"); const ede_test_expected_t expected[] = { { .code = 1, .txt = NULL }, - { .code = 2, .txt = "two" }, + { .code = 2, .txt = "two-the-first" }, { .code = 3, .txt = "three" }, }; dns_ede_test_equals(expected, 3, &edectx1); dns_ede_copy(&edectx2, &edectx1); dns_ede_test_equals(expected, 3, &edectx2); + dns_ede_test_equals(expected, 3, &edectx1); + + dns_ede_reset(&edectx2); + dns_ede_add(&edectx2, 1, "one-the-first-with-txt"); + dns_ede_add(&edectx2, 2, "two-the-second"); + + const ede_test_expected_t expected2[] = { + { .code = 1, .txt = "one-the-first-with-txt" }, + { .code = 2, .txt = "two-the-second" }, + { .code = 3, .txt = "three" } + }; + + dns_ede_copy(&edectx2, &edectx1); + dns_ede_test_equals(expected2, 3, &edectx2); + dns_ede_test_equals(expected, 3, &edectx1); + + dns_ede_init(mctx, &edectx3); + dns_ede_add(&edectx3, 2, "two-the-third"); + dns_ede_copy(&edectx3, &edectx2); + + const ede_test_expected_t expected3[] = { + { .code = 2, .txt = "two-the-third" }, + { .code = 1, .txt = "one-the-first-with-txt" }, + { .code = 3, .txt = "three" } + }; + dns_ede_test_equals(expected3, 3, &edectx3); dns_ede_reset(&edectx1); dns_ede_reset(&edectx2); + dns_ede_reset(&edectx3); } ISC_TEST_LIST_START From ccafa27b446b17e3ac44474d7283b1b78fbb0d28 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Thu, 30 Jan 2025 11:54:36 +0100 Subject: [PATCH 6/7] Use DNS_EDE_OTHER instead of its literal value (cherry picked from commit 7c5678bb03ac1fe8484ca72203b035cd16a0a635) --- lib/ns/query.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 0c426d3167..eff16b45a1 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -11722,7 +11722,7 @@ ns_query_done(query_ctx_t *qctx) { */ partial_result_with_servfail = true; - dns_ede_add(&qctx->client->edectx, 0, + dns_ede_add(&qctx->client->edectx, DNS_EDE_OTHER, "max. restarts reached"); ns_client_log(qctx->client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_QUERY, ISC_LOG_INFO, From 7b04c801837be18879b392de6269fc425270dc8e Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Thu, 30 Jan 2025 13:06:08 +0100 Subject: [PATCH 7/7] manually add dns_lctx to isc_log_write in ede.c Because the new introduced code in main doesn't use the log context anymore, manually add the log context for isc_log_write usages in the new ede.c file. --- lib/dns/ede.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/lib/dns/ede.c b/lib/dns/ede.c index 7fbbd83905..4b6e7faad7 100644 --- a/lib/dns/ede.c +++ b/lib/dns/ede.c @@ -41,29 +41,32 @@ dns_ede_add(dns_edectx_t *edectx, uint16_t code, const char *text) { size_t textlen = 0; if (dns__ede_checkandupdateedeused(edectx, code)) { - isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, - ISC_LOG_DEBUG(1), "ignoring duplicate ede %u %s", - code, text == NULL ? "(null)" : text); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), + "ignoring duplicate ede %u %s", code, + text == NULL ? "(null)" : text); return; } if (edectx->nextede >= DNS_EDE_MAX_ERRORS) { - isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, - ISC_LOG_DEBUG(1), "too many ede, ignoring %u %s", - code, text == NULL ? "(null)" : text); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), + "too many ede, ignoring %u %s", code, + text == NULL ? "(null)" : text); return; } INSIST(edectx->ede[edectx->nextede] == NULL); - isc_log_write(DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, - ISC_LOG_DEBUG(1), "set ede: info-code %u extra-text %s", - code, text == NULL ? "(null)" : text); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), + "set ede: info-code %u extra-text %s", code, + text == NULL ? "(null)" : text); if (text != NULL) { textlen = strlen(text); if (textlen > DNS_EDE_EXTRATEXT_LEN) { - isc_log_write(DNS_LOGCATEGORY_RESOLVER, + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), "truncate EDE code %hu text: %s", code, text); @@ -147,7 +150,7 @@ dns_ede_copy(dns_edectx_t *edectx_to, const dns_edectx_t *edectx_from) { } if (edectx_to->nextede >= DNS_EDE_MAX_ERRORS) { - isc_log_write(DNS_LOGCATEGORY_RESOLVER, + isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(1), "too many ede from subfetch"); break;