fix(dns): Dynamic DNS sorting (RFC 6724) to work in IPv6-only network (IDFGH-12553) #66

sgryphon · 2024-02-26T10:55:52Z

To address issue: espressif/esp-idf#13255

Also useful is setting up to configure the option via KConfig (a separate PR to the main esp-idf repository).

This fix adds an option to dynamically set the order of DNS results based on RFC 6724, to return the best destination address based on the available source addresses, allowing a device with both IPv4 and IPv6 enabled to work in any network configuration, including IPv4-only, IPv6-only, and dual-stack.

Without the option there is a static ordering preferring IPv4, which means on an IPv6-only network, a dual-stack destination returns the IPv4 address, which fails. Setting the static preference to IPv6 would have the opposite problem. A dynamic sort, based on available source addresses, is required to work on all networks.

The dynamic sorting only applies if you have both IPv6 and IPv4 enabled.

Summary of behaviour:

If the destination is single stack, it will use the available address.
If you are on a single stack network, connecting to a general address, it will use that network (IPv6 or IPv4).

Any IPv6 link-local address you get does not count for global scope

On a dual stack network, connecting to a dual-stack host:

If the destination is a general IPv6 (including DNS64/NAT64) then it will be used
Otherwise the IPv4 address will be used

Other IPv6 addresses (Toredo, ULA, etc) will still be used if there is no IPv4 address (see step 1), but if there is also an IPv4 address then the IPv4 address will be used in preference.

Replaces PR #65 that had a very basic implementation (not full RFC 6724).

This fix adds an option to dynamically set the order based on whether an the host has a global (or ULA) IPv6 address, allowing a device with both IPv4 and IPv6 enabled to work in any network configuration, including IPv4-only, IPv6-only, and dual-stack. Without the option there is a static ordering preferring IPv4, which means on an IPv6-only network, a dual-stack destination returns the IPv4 address, which fails. Setting the static preference to IPv6 would have the opposite problem. A dynamic sort, based on available source addresses, is required to work on all networks.

sgryphon · 2024-04-07T21:48:18Z

Testing with Arduino-ESP32.

I have built a version of the Arduino-ESP32 libraries with the fix included, coupled with some fixes in the Arduino-ESP32 project.

The results work with most networks and destinations, although there are still issues with TLS / HTTPS. (Most production systems should be using TLS):

Network	Dual-Stack	IPv6	IPv4	TLS Dual-Stack
IPv4 (Shadow)	IPv4	not possible	Yes	Yes
IPv6 disabled, Dual-stack+NAT64	IPv4	not possible	Yes	Yes
Dual-stack+NAT64 (Astral)	IPv6	Yes	NAT64-IPv4	fail
IPv6+NAT64 (Wildspace)	IPv6	Yes	NAT64-IPv4	fail

Tests were done using a custom sample app with an M5Stack Core2, https://github.com/sgryphon/iot-demo-build/tree/feature/arduino-esp32-v3-update/m5stack/m5unified_wifi_https

PlatformIO was used to build, with the following custom configuration:

platform = https://github.com/sgryphon/platform-espressif32.git#sgryphon/add-esp32-arduino-libs
platform_packages = 
  platformio/framework-arduinoespressif32-libs @ https://github.com/sgryphon/esp32-arduino-libs.git#sgryphon/test-fix-ipv6-lwip-and-config
  platformio/framework-arduinoespressif32 @ https://github.com/espressif/arduino-esp32.git

david-cermak

Thanks for implementing this useful feature and sorry for the slow response time.
We're currently in the process of making (other) changes to the DNS module, as well, supporting multiple addresses, so there might be some conflicts after merging.

We'd like to accept your changes, but it would still take some time I'm afraid. I performed a basic formal review for now. (would be good if we make the CI running as well).
Thank you again and the contribution, and for your patience.

david-cermak · 2024-04-15T04:14:56Z

src/api/netdb.c

-#include "lwip/ip_addr.h"
+#include "lwip/ip6_addr.h"


Could you revert this change? I think including ip_addr.h should be enough

david-cermak · 2024-04-15T04:16:09Z

src/api/netdb.c

+ip6_addr_t * dns_select_destination_address(ip6_addr_t *cand_list[], const int cand_list_length);
+u8_t dns_addr_get_scope(const ip6_addr_t *addr);
+u8_t dns_precedence_for_label(u8_t label);
+u8_t dns_get_precedence_label(const ip6_addr_t *addr);


Are these functions supposed to be internal only? If so, could you please make them static?

david-cermak · 2024-04-15T04:26:19Z

src/api/netdb.c

+ */
+u8_t
+dns_get_precedence_label(const ip6_addr_t *addr) {
+  /* IDEA: Allow this function to be overriden by a customisation hook */


👍 good idea, you can look at other places in lwip, for example:

esp-lwip/src/core/ipv6/nd6.c

Lines 1998 to 2003 in a45be9e

#ifdef LWIP_HOOK_ND6_GET_GW

} else if ((next_hop_addr = LWIP_HOOK_ND6_GET_GW(netif, ip6addr)) != NULL) {

/* Next hop for destination provided by hook function. */

destination_cache[nd6_cached_destination_index].pmtu = netif->mtu;

ip6_addr_set(&destination_cache[nd6_cached_destination_index].next_hop_addr, next_hop_addr);

#endif /* LWIP_HOOK_ND6_GET_GW */

david-cermak · 2024-04-15T04:32:42Z

src/api/netdb.c

+  u8_t cand_0_precedence = dns_precedence_for_label(cand_0_label);
+  u8_t cand_1_precedence = dns_precedence_for_label(cand_1_label);
+
+  LWIP_DEBUGF(DNS_DEBUG, ("dns_select: rule 6, cand_0 precedence %d, cand_1 precedence %d\n",


these parameters are u8_t, could you please use lwip format macros (S16_U, X8_U, etc), or simply declare them as int ?

david-cermak · 2024-04-15T04:35:11Z

src/api/netdb.c

+  u32_t has_ipv4_source_scope_flags = 0x0;
+  u32_t has_source_precedence_label_flags = 0x0;
+  struct netif *netif;
+  for(netif = netif_list; netif != NULL; netif = netif->next) {


You can use:

Suggested change

for(netif = netif_list; netif != NULL; netif = netif->next) {

NETIF_FOREACH(netif) {

david-cermak · 2024-04-15T04:41:30Z

src/api/netdb.c

+          addr_list[index] = ip_2_ip6(&addr6);
+          index++;
+        }
+        err_t err4 = netconn_gethostbyname_addrtype(nodename, &addr4, NETCONN_DNS_IPV4);


Not sure if this would compile. lwip still uses C90 (for using tests and some basic checks), so you need to define/declare all local vars before statements.

But saw that CI didn't run on this PR, strange, I'll try to fix that... you can try to update the triggers on your branch. It's good to have these basic tests running and passing:

esp-lwip/.github/workflows/ci-linux.yml

Line 3 in f792214

on: [push, pull_request]

espressif-abhikroy · 2024-04-22T13:48:58Z

src/api/netdb.c

+        ip_addr_t addr4;
+        ip_addr_t addr6;
+        ip6_addr_t *addr_list[2];
+        int index = 0;
+        err_t err6 = netconn_gethostbyname_addrtype(nodename, &addr6, NETCONN_DNS_IPV6);
+        if (err6 == ERR_OK) {
+          addr_list[index] = ip_2_ip6(&addr6);
+          index++;
+        }
+        err_t err4 = netconn_gethostbyname_addrtype(nodename, &addr4, NETCONN_DNS_IPV4);
+        if (err4 == ERR_OK) {
+          /* Convert native V4 address to a V4-mapped IPV6 address */
+          ip4_2_ipv4_mapped_ipv6(ip_2_ip6(&addr4), ip_2_ip4(&addr4));
+          IP_SET_TYPE_VAL(addr4, IPADDR_TYPE_V6);
+          addr_list[index] = ip_2_ip6(&addr4);
+          index++;
+        }
+        err = err6 == ERR_OK || err4 == ERR_OK ? ERR_OK : EAI_FAIL;
+        if (err != ERR_OK) {
+          return EAI_FAIL;
+        }
+        ip6_addr_t *best_addr = dns_select_destination_address(addr_list, index);
+        if (ip6_addr_isipv4mappedipv6(best_addr)) {
+          unmap_ipv4_mapped_ipv6(ip_2_ip4(&addr), best_addr);
+        } else {
+          ip_addr_copy_from_ip6(addr, *best_addr);


Lwip implementation of getaddrinfo() has certain limitations.
AI_V4MAPPED and AI_ADDRCONFIG are pending, as mentioned.
I was unable to find the details of this DNS_DYNAMIC_SORT algorithm in the manual pages of getaddrinfo().
Moreover, this algorithm can also be implemented in the application layer by modifying the hints->ai_family and hints->ai_flags.
I would recommend contributing an example in the https://github.com/lwip-tcpip/lwip/tree/master/contrib with DNS_DYNAMIC_SORT.

I am unsure which manual pages of getaddrinfo() you are referring to. LWIP does not have this sorting algorithm (yet), but it is common in Linux implementations, e.g. https://man7.org/linux/man-pages/man3/getaddrinfo.3.html

"The sorting function used within getaddrinfo() is defined in RFC 3484" (RFC 6724 is the new version that obsoletes RFC 3484). The actual algorithm is defined in the RFC.

The V4-mapped conversion is done because the RFC 6724 is defined in terms of mapped addresss. The result is unmapped before returning. When/if AI_V4MAPPED is implemented, this would need to be updated.

AI_ADDRCONFIG may help, but only when there are no addresses of the wrong type. Because link-local IPv4 addresses are not usually enabled, this may help the IPv6-only network to dual-stack bug (no v4 unless you turn on link-local RFC 3927), but doesn't provide a lot of benefit over the standard sorting algorithm.

I am looking to also contribute this upstream.

This was referenced Feb 26, 2024

DNS dynamic sort (IDFGH-12207) #65

Closed

DNS lookup should take into account available source IPv6 and IPv4 addresses (IDFGH-12201) espressif/esp-idf#13255

Open

sgryphon force-pushed the sgryphon/dns-dynamic-sort-rfc6724 branch from fdcd480 to efccf13 Compare February 26, 2024 12:13

sgryphon changed the base branch from 2.1.2-esp to 2.1.3-esp February 28, 2024 00:59

sgryphon force-pushed the sgryphon/dns-dynamic-sort-rfc6724 branch 2 times, most recently from 6287920 to 009dc00 Compare February 28, 2024 01:10

sgryphon force-pushed the sgryphon/dns-dynamic-sort-rfc6724 branch from 009dc00 to fd387d8 Compare February 28, 2024 01:45

sgryphon mentioned this pull request Mar 13, 2024

Fix: Change DNS to dynamic sort for IPv6-only network (option) (IDFGH-12204) espressif/esp-idf#13258

Open

espressif-bot added the Status: Opened label Apr 7, 2024

github-actions bot changed the title ~~fix(dns): Dynamic DNS sorting (RFC 6724) to work in IPv6-only network~~ fix(dns): Dynamic DNS sorting (RFC 6724) to work in IPv6-only network (IDFGH-12553) Apr 7, 2024

espressif-bot assigned espressif-abhikroy Apr 8, 2024

david-cermak reviewed Apr 15, 2024

View reviewed changes

espressif-bot added Status: In Progress and removed Status: Opened labels Apr 19, 2024

espressif-abhikroy reviewed Apr 22, 2024

View reviewed changes

espressif-bot added Status: Reviewing and removed Status: In Progress labels Apr 24, 2024

tomassebestik removed the Status: Reviewing label Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dns): Dynamic DNS sorting (RFC 6724) to work in IPv6-only network (IDFGH-12553) #66

fix(dns): Dynamic DNS sorting (RFC 6724) to work in IPv6-only network (IDFGH-12553) #66

sgryphon commented Feb 26, 2024

sgryphon commented Apr 7, 2024 •

edited

Loading

david-cermak left a comment

david-cermak Apr 15, 2024

david-cermak Apr 15, 2024

david-cermak Apr 15, 2024

david-cermak Apr 15, 2024

david-cermak Apr 15, 2024

david-cermak Apr 15, 2024

espressif-abhikroy Apr 22, 2024

sgryphon Apr 22, 2024

	#ifdef LWIP_HOOK_ND6_GET_GW
	} else if ((next_hop_addr = LWIP_HOOK_ND6_GET_GW(netif, ip6addr)) != NULL) {
	/* Next hop for destination provided by hook function. */
	destination_cache[nd6_cached_destination_index].pmtu = netif->mtu;
	ip6_addr_set(&destination_cache[nd6_cached_destination_index].next_hop_addr, next_hop_addr);
	#endif /* LWIP_HOOK_ND6_GET_GW */

	for(netif = netif_list; netif != NULL; netif = netif->next) {
	NETIF_FOREACH(netif) {

		#include "lwip/ip_addr.h"
		#include "lwip/ip6_addr.h"

fix(dns): Dynamic DNS sorting (RFC 6724) to work in IPv6-only network (IDFGH-12553) #66

Are you sure you want to change the base?

fix(dns): Dynamic DNS sorting (RFC 6724) to work in IPv6-only network (IDFGH-12553) #66

Conversation

sgryphon commented Feb 26, 2024

sgryphon commented Apr 7, 2024 • edited Loading

david-cermak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgryphon commented Apr 7, 2024 •

edited

Loading