From 4096f2713075362b204a88eee1b6bc30e609a328 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Tue, 14 Jan 2025 17:13:42 +0100 Subject: [PATCH 1/2] add support for multiple EDE Extended DNS error mechanism (EDE) enables to have several EDE raised during a DNS resolution (typically, a DNSSEC query will do multiple fetches which each of them can have an error). Add support to up to 3 EDE errors in an DNS response. If duplicates occur (two EDEs with the same code, the extra text is not compared), only the first one will be part of the DNS answer. Because the maximum number of EDE is statically fixed, `ns_client_t` object own a static vector of `DNS_DE_MAX_ERRORS` (instead of a linked list, for instance). The array can be fully filled (all slots point to an allocated `dns_ednsopt_t` object) or partially filled (or empty). In such case, the first NULL slot means there is no more EDE objects. --- lib/dns/include/dns/message.h | 8 +-- lib/dns/message.c | 16 ++---- lib/ns/client.c | 95 ++++++++++++++++++++++++----------- lib/ns/include/ns/client.h | 2 +- lib/ns/query.c | 1 + 5 files changed, 77 insertions(+), 45 deletions(-) diff --git a/lib/dns/include/dns/message.h b/lib/dns/include/dns/message.h index c7ebf49591..e8ea327ba0 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -125,10 +125,10 @@ /*%< * The maximum number of EDNS options we allow to set. Reserve space for the - * options we know about. Extended DNS Errors may occur multiple times, but we - * will set only one per message (for now). + * options we know about. Extended DNS Errors may occur multiple times, see + * DNS_EDE_MAX_ERRORS. */ -#define DNS_EDNSOPTIONS 9 +#define DNS_EDNSOPTIONS 8 + DNS_EDE_MAX_ERRORS /*%< EDNS0 extended DNS errors */ #define DNS_EDE_OTHER 0 /*%< Other Error */ @@ -176,6 +176,8 @@ typedef ISC_LIST(dns_ede_t) dns_edelist_t; */ #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) diff --git a/lib/dns/message.c b/lib/dns/message.c index c7441071a8..28b3de2387 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -5088,19 +5088,9 @@ dns_ede_append(isc_mem_t *mctx, dns_edelist_t *list, uint16_t info_code, }; if (extra_text) { - size_t len = strlen(extra_text); - - if (len >= DNS_EDE_EXTRATEXT_LEN) { - isc_log_write(DNS_LOGCATEGORY_GENERAL, - DNS_LOGMODULE_MESSAGE, ISC_LOG_PRINTTIME, - "truncate EDE code %hu text: %s", - info_code, extra_text); - len = DNS_EDE_EXTRATEXT_LEN - 1; - } - - ede->extra_text = isc_mem_allocate(mctx, len + 1); - strncpy(ede->extra_text, extra_text, len); - ede->extra_text[len] = '\0'; + ede->extra_text = isc_mem_allocate(mctx, + strlen(extra_text) + 1); + strcpy(ede->extra_text, extra_text); } ISC_LIST_APPEND(*list, ede, link); diff --git a/lib/ns/client.c b/lib/ns/client.c index b1502dee48..a330f663cb 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -220,27 +220,54 @@ ns_client_settimeout(ns_client_t *client, unsigned int seconds) { static void client_extendederror_reset(ns_client_t *client) { - if (client->ede == NULL) { - return; + 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; + } } - isc_mem_put(client->manager->mctx, client->ede->value, - client->ede->length); - isc_mem_put(client->manager->mctx, client->ede, sizeof(dns_ednsopt_t)); - client->ede = NULL; } void ns_client_extendederror(ns_client_t *client, uint16_t code, const char *text) { - unsigned char ede[DNS_EDE_EXTRATEXT_LEN + 2]; - isc_buffer_t buf; - uint16_t len = sizeof(uint16_t); + 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)); - if (client->ede != NULL) { + /* + * 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), - "already have ede, ignoring %u %s", code, + "too many ede, ignoring %u %s", code, text == NULL ? "(null)" : text); return; } @@ -249,24 +276,31 @@ ns_client_extendederror(ns_client_t *client, uint16_t code, const char *text) { ISC_LOG_DEBUG(1), "set ede: info-code %u extra-text %s", code, text == NULL ? "(null)" : text); - isc_buffer_init(&buf, ede, sizeof(ede)); - isc_buffer_putuint16(&buf, code); if (text != NULL && strlen(text) > 0) { - if (strlen(text) < DNS_EDE_EXTRATEXT_LEN) { - isc_buffer_putstr(&buf, text); - len += (uint16_t)(strlen(text)); - } else { + textlen = strlen(text); + + if (textlen > DNS_EDE_EXTRATEXT_LEN) { ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_CLIENT, ISC_LOG_WARNING, - "ede extra-text too long, ignoring"); + NS_LOGMODULE_CLIENT, ISC_LOG_DEBUG(1), + "truncate EDE code %hu text: %s", code, + text); + textlen = DNS_EDE_EXTRATEXT_LEN; } } - client->ede = isc_mem_get(client->manager->mctx, sizeof(dns_ednsopt_t)); - client->ede->code = DNS_OPT_EDE; - client->ede->length = len; - client->ede->value = isc_mem_get(client->manager->mctx, len); - memmove(client->ede->value, ede, 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 @@ -1231,11 +1265,17 @@ no_nsid: count++; } - if (client->ede != NULL) { + for (size_t i = 0; i < DNS_EDE_MAX_ERRORS; i++) { + dns_ednsopt_t *ede = client->ede[i]; + + if (ede == NULL) { + break; + } + INSIST(count < DNS_EDNSOPTIONS); ednsopts[count].code = DNS_OPT_EDE; - ednsopts[count].length = client->ede->length; - ednsopts[count].value = client->ede->value; + ednsopts[count].length = ede->length; + ednsopts[count].value = ede->value; count++; } @@ -2034,7 +2074,6 @@ ns_client_request(isc_nmhandle_t *handle, isc_result_t eresult, } client->message->rcode = dns_rcode_noerror; - client->ede = NULL; /* * Deal with EDNS. diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 2c516de3b8..96f10665e1 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_ednsopt_t *ede[DNS_EDE_MAX_ERRORS]; uint16_t udpsize; uint16_t extflags; int16_t ednsversion; /* -1 noedns */ diff --git a/lib/ns/query.c b/lib/ns/query.c index 8464e782d9..ff75978b13 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -899,6 +899,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(). From 950a0cffb376d19085330e25aee7a93ef4124cdb Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Tue, 14 Jan 2025 17:15:42 +0100 Subject: [PATCH 2/2] add unit tests covering multiple EDE support --- tests/dns/ede_test.c | 8 +- tests/ns/Makefile.am | 3 +- tests/ns/client_test.c | 171 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 3 deletions(-) create mode 100644 tests/ns/client_test.c diff --git a/tests/dns/ede_test.c b/tests/dns/ede_test.c index f7a8c19002..74194a4ee2 100644 --- a/tests/dns/ede_test.c +++ b/tests/dns/ede_test.c @@ -56,12 +56,16 @@ ISC_RUN_TEST_IMPL(ede_enqueue_unlink) { assert_string_equal(ede->extra_text, msg1); assert_ptr_not_equal(ede->extra_text, msg1); + /* + * 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_not_equal(ede->extra_text, msg2); + assert_string_equal(ede->extra_text, msg2); assert_ptr_not_equal(ede->extra_text, msg2); - assert_int_equal(strlen(ede->extra_text), 63); dns_ede_unlinkall(mctx, &list); assert_true(ISC_LIST_EMPTY(list)); diff --git a/tests/ns/Makefile.am b/tests/ns/Makefile.am index 655bdc835c..68adc36f83 100644 --- a/tests/ns/Makefile.am +++ b/tests/ns/Makefile.am @@ -17,7 +17,8 @@ LDADD += \ check_PROGRAMS = \ notify_test \ plugin_test \ - query_test + query_test \ + client_test notify_test_SOURCES = \ notify_test.c \ diff --git a/tests/ns/client_test.c b/tests/ns/client_test.c new file mode 100644 index 0000000000..83a174c1d4 --- /dev/null +++ b/tests/ns/client_test.c @@ -0,0 +1,171 @@ +/* + * 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; + + 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; + } + 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->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); + assert_int_equal(edns->code, DNS_OPT_EDE); + + code = ISC_U8TO16_BE(edns->value); + assert_int_equal(code, expected[count].code); + + if (edns->length > codelen) { + assert_non_null(expected[count].txt); + txt = edns->value + codelen; + assert_memory_equal(expected[count].txt, txt, + edns->length - codelen); + } 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"; + + ns_client_extendederror(client, 2, txt1); + ns_client_extendederror(client, 22, NULL); + ns_client_extendederror(client, 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(); + + 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"); + + 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(); + + ns_client_extendederror(client, 1, NULL); + ns_client_extendederror(client, 1, "two"); + ns_client_extendederror(client, 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