Fix IPv6-only DNS by checking IPv6 first if have a public scope address #9443
Conversation
sgryphon
changed the title
👋 Hello sgryphon, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
from
8287cf8
to
d5591b0
Compare
me-no-dev
added
Area: LIB Builder
|
I need a bit more testing on this branch after the other changes. |
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
2 times, most recently
from
c4f5437
to
3ac49e9
Compare
|
your lib-builder changes are not here yet :) when something is updated in IDF a new set of libs will be pushed to the idf-release/v5.1 branch here (5.1 libs PR) |
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
from
3ac49e9
to
2a8ff99
Compare
|
I have re-tested everything with the latest changes. I also tested a dual-stack network but with IPv6 not enabled, i.e. don't call WiFi.enableIPv6(), and it behaves as if it is on an IPv4 only network, so fully backwards compatible (if code can't handle IPv6 at all, e.g. tries to output to a string buffer that is too small, etc). I also built the libs with the config changes and pushed to my own repo, so I could try a PlatformIO build with everything set up. The check for if you have a global IPv6 also now uses Arduino functions like getNetifByID() and hasGlobalIPv6(), rather than low level functions, as it reads nicer. This is ready to merge, but really needs the libs config update to be of much use. There are still some problems with TLS / HTTPS, which most production systems should be using, but they need the fix in LWIP, and TLS uses the DNS directly at the lower level. |
|
The contents of this PR do exactly what |
Kind of, except preferV6 was a fixed value, whereas checking IPv6 first is dynamic based on whether you have a public (i.e. global, not link scope) IPv6 address. If the target is dual stack, then it has both IPv4 and IPv6 If you have IPv4 only, you want the IPv4 address; if you only have IPv6 then you want the IPv6 address. So you can't have a fixed preference decided by the programmer when they wrote the code, but have to look at the actual addresses you have been given (depending on what is available on the network and run time). |
|
@sgryphon you must have not read the code before. It was checking exactly if we have a global address and if so, it allows to set if you prefer V6 or V4 address. You just did not see that code after the refactoring (placeholder left with ToDo) |
|
Please rebase the PR so it fixes the conflicts. |
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
from
2a8ff99
to
01d7efb
Compare
|
Rebase done.
Yes, I implemented the ToDo. i.e. the first part of the code that checks if you have a global IPv6. I also added back in the IPv6 check because the code is now using getaddrinfo(), which has different parameters. The old code to host passed in either V4_V6 or V6_V4 as a preference order to hostbyname (i.e. manual preference order). getaddrinfo() doesn't do that -- you pass in AF_UNSPEC for any (which should match your address but the LWIP bug hard codes to V4_V6), or you can limit to one address family only, i.e. AF_INET or AF_INET6. To implement the preference order, based on actual addresses, the code checks if we have a global IPv6 (no longer a TODO), and if so, looks for AF_INET6 first (if no IPv6, then it doesn't). If AF_INET6 fails, then it falls back to AF_UNSPEC (which currently prefers IPv4). |
|
... But, I finally worked out how to the Libs building with my LWIP fix, espressif/esp-lwip#66. With the LWIP fix, then AF_UNSPEC returns the correct destination address selection according to RFC 6724, i.e. if you have an IPv6 it prefers IPv6, so the work around is not needed. i.e. Current Arduino-ESP32 + LWIP fix works, and this PR is not needed. If possible, I would prefer the LWIP fix get merged instead, as that is where the issue really needs to be fixed, for all of ESP-IDF (not just Arduino). The PlatformIO config, with reference to my build of Libs with the LWIP fix is: 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 |
|
i've marked it as blocked, but this PR is not really blocked by anything, is it? I do not find it nice to ask DNS twice. Can we make it optional through |
|
BTW. let's split this into two PRs. One will detect if public V6 address exists (and reset DNS cache), the other will do the |
Well, it doesn't have a lot of value until the Libs config change is available. By itself, this will only change the dual stack preference order from V4/V6 to V6/V4, but it still won't work in IPv6 only. When the Libs change is merged, and IPv6-only is working, this change will then be required so that dual-stack destinations don't try and use the IPv4 address and fail. You are welcome to merge this one now, as is (although I would much prefer the LWIP fix).
I agree, if we can get the fix applied to LWIP that would be a much better solution. LWIP may have to do multiple DNS queries anyway, e.g. if V6 fails then it will do a second query for V4 (because LWIP only supports one result at a time). But this is only relevant if we have both addresses. A lot of the time only one query will be made, e.g. if in an IPv4-only network (or IPv6 is disabled) then the first DNS will be skipped. Or with IPv6 if the first succeeds, the second is not made.
Not entirely sure what you want here. Preference then only applies if we have both addresses, so for a single type of address we need to call the relevant DNS (4 OR 6). We could add a check for if we have an IPv4, and then allow an alternate preference to check IPv4 first. (RFC 6724 default is to preference IPv6) We would then call LWIP DNS with only AF_UNSPEC, although internally it will still do two DNS checks if needed. |
|
I also looked into the TLS (HTTPS / SSL) issues, which I thought were the same as ESP-IDF, because even with the LWIP fix the TLS connections were still failing. I have found the problem and have a fix up in another branch; I have tested a few scenarios and it is working, but need to finish testing all scenarios before I put up the PR. As I would encourage systems to always use HTTPS, getting it working is important. |
|
Will you please split the PR so I can merge the change of IP detection? |
|
Related TLS fix is here: #9470 |
I'm not entirely sure what the IP detection would do by itself, and trying to understand what you want. |
|
Okay, I think I found what you want -- although I don't think it will achieve much.
I can split it like that, although I don't think it will do much. i.e. the first change won't fix anything (I'm not sure what resetting DNS cache will achieve). The rest of the code is nothing about preferences or preferring, but about solving the problem that if you are IPv6-only then you need to get an IPv6 address to connect to, whereas if you are IPv4 only you need to get an IPv4 address. That is not a preference, but a bug fix to be able to connect IPv6 to a dual-stack destination. (The only time a preference would apply would be dual-stack to dual-stack where either will work; I don't suggest allowing a preference, but just following RFC6724 that prioritises IPv6.) So 3 things:
|
very useful if you first got your IPv4 address. Please split the PR |
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
from
01d7efb
to
16a7b9b
Compare
|
I have split the PRs. The new PR has the IP address type check and DNS cache refresh. #9476 This one only adds the IPv6 fix as a second commit (i.e. consider it targetting the other PR). The full diff for this will contain both changes, but you can look at the individual commits for details. |
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
from
16a7b9b
to
07dc0a6
Compare
|
Rebased on latest main, with just the IPv6 DNS fix commit |
| @@ -86,12 +86,36 @@ int NetworkManager::hostByName(const char* aHostname, IPAddress& aResult) | |||
| log_d("Clearing DNS cache"); | |||
| } | |||
|
|
|||
| struct addrinfo hints; | |||
There was a problem hiding this comment.
this is a better way to init the structure. especially in CPP. Please keep it that way and only change the relevant fields before each call to lwip_getaddrinfo
me-no-dev
removed
Area: LIB Builder
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
from
07dc0a6
to
f0d14f8
Compare
…ublic address Work around because AF_UNSPEC does not check available addresses when determining result. If you have a global scope IPv6 address, then first check for IPv6 DNS result; if you don't have an IPv6, or there is no IPv6 result, then check IPv4. This allows IPv6-only networks to connect to dual-stack destinations, as they will get the IPv6 address (rather than the unusable IPv4). It also means a dual-stack host to a dual-stack destination will preference IPv6. There is no effect if you are on an IPv4-only network, or it is an IPv4-only destination.
sgryphon
force-pushed
the
sgryphon/dns-ipv6-check-first-if-have-addr
branch
from
f0d14f8
to
2d0bab7
Compare
…ublic address (espressif#9443) Work around because AF_UNSPEC does not check available addresses when determining result. If you have a global scope IPv6 address, then first check for IPv6 DNS result; if you don't have an IPv6, or there is no IPv6 result, then check IPv4. This allows IPv6-only networks to connect to dual-stack destinations, as they will get the IPv6 address (rather than the unusable IPv4). It also means a dual-stack host to a dual-stack destination will preference IPv6. There is no effect if you are on an IPv4-only network, or it is an IPv4-only destination.
Description of Change
On top of the IPv6 change to use
getaddrinfo(), this implements a fix for IPv6-only networks by checking if we have an IPv6 public scope address, and if we do then check IPv6 first. If there is no IPv6-address, or we are on an IPv4-only network, then the change has no effect.By checking IPv6 first when available, it allows devices to work across all network types -- IPv4-only, IPv6-only, and dual stack, connecting to all destination types -- IPv4-only, IPv6-only, and dual-stack servers.
Previously, while IPv6-only to IPv6-only worked, trying to connect from IPv6-only to dual stack (including DNS64) did not work, as the IPv4 address was always returned (and unreachable).
Note that this change does not do much unless IPv6 is enabled (Libs change): it will simply change the dual-stack preference order to IPv6-first, but IPv6-only will still fail if there is no DNS to query.
Tests scenarios
I have tested on a M5Stack Core2, with an ESP32 chip.
I have tested across different network types, and different destination types. Now that the TLS IPv6 support bug is merged, this now works across both HTTP and HTTPS connections.
Note: If IPv6 is never enabled, then the code is backwards compatible and behaves as if it is on an IPv4 only network.
PlatformIO configuration for testing. I have build and uploaded a version of the libs with the required configuration change, so that I can test using PlatformIO:
Some example outputs. (These are from my own test app, at https://github.com/sgryphon/iot-demo-build/tree/feature/arduino-esp32-v3-update/m5stack/m5unified_wifi_https)
IPv6-only to dual-stack:
IPv6-only to IPv4-only, also works via DNS64+NAT64, by using a synthentic IPv6 DNS64 address -- effectively making the destination dual-stack. From the point of view of the destination the source address appears to be the IPv4 NAT address (same as for NAT44).
IPv6-only via HTTPS/TLS to dual-stack.
Related links
Now that the TLS change is merged, once the Libs are also rebuilt (the config is already merged), this addresses the issues in #9143, and in the IPv6 discussion.
An alternative fix, that implements RFC6724 destination address selection, has been proposed for LWIP; this would make AF_UNSPEC work and remove the need for the work around checking for IPv6 separately: espressif/esp-lwip#66