From e685443c74f1faae6f83f0c3c68d67dc903bb4aa 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. (cherry picked from commit 4096f2713075362b204a88eee1b6bc30e609a328) --- 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 8e7b6dd29f..b461992334 100644 --- a/lib/dns/include/dns/message.h +++ b/lib/dns/include/dns/message.h @@ -119,10 +119,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 8 +#define DNS_EDNSOPTIONS 7 + DNS_EDE_MAX_ERRORS /*%< EDNS0 extended DNS errors */ #define DNS_EDE_OTHER 0 /*%< Other Error */ @@ -170,6 +170,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 f095c2decf..94f2d92ab1 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -5029,19 +5029,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_lctx, 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 a51b82b889..63b806d522 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -225,27 +225,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; } @@ -254,24 +281,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 @@ -1233,11 +1267,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++; } @@ -2022,7 +2062,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 0711add8e5..e62fd8db07 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 29000be3ee..714c8d6c93 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -902,6 +902,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 01bbc357c797b91685415177809a70b0610ba0d5 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 (cherry picked from commit 950a0cffb376d19085330e25aee7a93ef4124cdb) --- 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 c26bec85e6..23b3ccbf7b 100644 --- a/tests/ns/Makefile.am +++ b/tests/ns/Makefile.am @@ -18,7 +18,8 @@ check_PROGRAMS = \ listenlist_test \ 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