net: Allow -proxy=[::1] on nodes with IPV6 lo only #30245

pull m3dwards wants to merge 1 commits into bitcoin:master from m3dwards:allow-dns-ipv6-lo-only changing 1 files +41 −22
  1. m3dwards commented at 12:53 pm on June 7, 2024: contributor

    This is similar to (but does not fix) #13155 which I believe is the same issue but in libevent.

    The issue is on a host that has IPV6 enabled but only a loopback IP address -proxy=[::1] will fail as [::1] is not considered valid by getaddrinfo with AI_ADDRCONFIG flag. I think the loopback interface should be considered valid and we have a functional test that will try to test this: feature_proxy.py.

    To replicate the issue, run feature_proxy.py inside a docker container that has IPV6 loopback ::1 address without specifically giving that container an external IPV6 address. This should be the default with recent versions of docker. IPV6 on loopback interface was enabled in docker engine 26 and later (https://docs.docker.com/engine/release-notes/26.0/#bug-fixes-and-enhancements-2).

    AI_ADDRCONFIG was introduced to prevent slow DNS lookups on systems that were IPV4 only. I think most systems will be dual stack now and I’m not sure this is really a concern for us but this is something to consider as a potential downside of this PR.

    References:

    Man section on AI_ADDRCONFIG:`

    0If hints.ai_flags includes the AI_ADDRCONFIG flag, then IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured, and  IPv6  addresses
1       are  returned only if the local system has at least one IPv6 address configured.  The loopback address is not considered for this case as valid as a configured address.  This flag is useful on, for ex‐
2       ample, IPv4-only systems, to ensure that getaddrinfo() does not return IPv6 socket addresses that would always fail in connect(2) or bind(2).

    AI_ADDRCONFIG considered harmful Wiki entry by Fedora

    Mozilla discussing slow DNS without AI_ADDRCONFIG and also localhost issues with it

  2. DrahtBot commented at 12:53 pm on June 7, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK vasild

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label P2P on Jun 7, 2024
  4. in src/netbase.cpp:57 in 84e63f34af outdated 
    53@@ -54,7 +54,7 @@ std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_loo
54     // If we don't allow lookups, then use the AI_NUMERICHOST flag for
55     // getaddrinfo to only decode numerical network addresses and suppress
56     // hostname lookups.
57-    ai_hint.ai_flags = allow_lookup ? AI_ADDRCONFIG : AI_NUMERICHOST;
    vasild commented at 9:38 am on June 14, 2024:

    There is a comment above which has to be removed if this patch makes it as it is:

    0// If we allow lookups of hostnames, use the AI_ADDRCONFIG flag to only
1// return addresses whose family we have an address configured for.
  5. vasild commented at 11:35 am on June 14, 2024: contributor

    Concept ACK on allowing usage of [::1] on nodes with IPv6 lo only.

    A workaround is to use -dns=0 (which will avoid AI_ADDRCONFIG but will make it impossible to use hostnames).

    I can reproduce the problem on Linux. FreeBSD treats the loopback address as valid, so this problem is non-existent on FreeBSD.

    I think there is a merit in this for our use cases of getaddrinfo() / Lookup*():

    IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured … (and same for IPv6)

    But not this:

    The loopback address is not considered for this case as valid as a configured address. This flag is useful on, for example, IPv4-only systems, to ensure that getaddrinfo() does not return IPv6 socket addresses that would always fail in connect(2) or bind(2).

    This is even bogus to some extent - if the system has only [::1] configured and we want to connect(2) or bind(2) to [::1] that will work. So the “always” is too strong / untrue.

    Now, can we have the AI_ADDRCONFIG behavior but get it to consider loopback addresses as valid? For example, after running getaddrinfo() with AI_ADDRCONFIG run it again without AI_ADDRCONFIG and append any loopback addresses from the second run to the results?

  6. m3dwards commented at 4:39 pm on June 14, 2024: contributor

    Now, can we have the AI_ADDRCONFIG behavior but get it to consider loopback addresses as valid? For example, after running getaddrinfo() with AI_ADDRCONFIG run it again without AI_ADDRCONFIG and append any loopback addresses from the second run to the results?

    I like this idea, I’ll have a go at implementing it.

  7. m3dwards force-pushed on Jun 18, 2024
  8. in src/netbase.cpp:63 in d1337d575c outdated 
    61+
62     addrinfo* ai_res{nullptr};
63-    const int n_err{getaddrinfo(name.c_str(), nullptr, &ai_hint, &ai_res)};
64-    if (n_err != 0) {
65-        return {};
66-    }
    m3dwards commented at 10:00 am on June 18, 2024:

    I removed the error handling here for two reasons:

    Firstly it just returns an empty vector which is what I believe this function will do anyway if it fails to get address info.

    Secondly, if getaddrinfo doesn’t allow loopback only IPV6 then that is returned as an error code 1 “address family for hostname not supported” so this specific case would have to be allowed.

    vasild commented at 10:57 am on June 21, 2024:

    Are you sure that getaddrinfo() would return error (ie n_err != 0) and still set something useful in ai_res? That sounds strange.

    I am worried that it may return an error (for whatever reason) and set ai_res to a bogus pointer which we later try to dereference.

    Just returning from the lambda and not from WrappedGetAddrInfo() if getaddrinfo() returns an error seems reasonable to me.

    m3dwards commented at 4:08 pm on June 21, 2024:

    It was working but I agree handling the error is safer.

    Also I was wrong that it would need to handle the specific “address family for hostname not supported” error.

    Implemented your suggestion of just returning from the lambda.

  9. m3dwards commented at 10:28 am on June 18, 2024: contributor
    Pushed new approach of calling getaddrinfo twice. First pass is unchanged from original behaviour, second pass adds local IP addresses should they have not been found on the first pass.
  10. m3dwards force-pushed on Jun 18, 2024
  11. DrahtBot added the label CI failed on Jun 18, 2024
  12. DrahtBot removed the label CI failed on Jun 19, 2024
  13. in src/netbase.cpp:64 in a7cbc27101 outdated 
    60+
61+    // getaddrinfo is called twice and to prevent duplicates a set is used
62+    // std::set used over a std::unordered_set as CNetAddr does not have a hash function
63+    std::set<CNetAddr> resolved_addresses_set;
64+
65+    auto callgetaddrinfo = [&](const int flags, const bool only_add_local_addr) {
    vasild commented at 10:26 am on June 21, 2024:

    nit: function arguments that are passed by value need not be const

    0    auto callgetaddrinfo = [&](int flags, bool only_add_local_addr) {

    http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in

  14. in src/netbase.cpp:82 in a7cbc27101 outdated 
    78+            if (ai_trav->ai_family == AF_INET6) {
79+                assert(ai_trav->ai_addrlen >= sizeof(sockaddr_in6));
80+                const sockaddr_in6* s6{reinterpret_cast<sockaddr_in6*>(ai_trav->ai_addr)};
81+                addr = CNetAddr{s6->sin6_addr, s6->sin6_scope_id};
82+            }
83+            if (!only_add_local_addr || (only_add_local_addr && addr.IsLocal())) {
    vasild commented at 10:40 am on June 21, 2024:

    nit, feel free to ignore if you find the current one more readable, but this is equivalent to:

    0            if (!only_add_local_addr || addr.IsLocal()) {
  15. in src/netbase.cpp:105 in a7cbc27101 outdated 
    119+    for (auto it = resolved_addresses_set.begin(); it != resolved_addresses_set.end(); ) {
120+        resolved_addresses.push_back(std::move(resolved_addresses_set.extract(it++).value()));
121     }
122-    freeaddrinfo(ai_res);
123 
124     return resolved_addresses;
    vasild commented at 10:51 am on June 21, 2024:

    Simpler:

    0-    // Convert the set to a vector
1-    std::vector<CNetAddr> resolved_addresses;
2-    resolved_addresses.reserve(resolved_addresses_set.size());
3-    for (auto it = resolved_addresses_set.begin(); it != resolved_addresses_set.end(); ) {
4-        resolved_addresses.push_back(std::move(resolved_addresses_set.extract(it++).value()));
5-    }
6-
7-    return resolved_addresses;
8+    return {resolved_addresses_set.begin(), resolved_addresses_set.end()};
  16. vasild commented at 10:52 am on June 21, 2024: contributor
    Approach ACK a7cbc27101677ee5ff357da9595f386b03b4756d
  17. m3dwards force-pushed on Jun 21, 2024
  18. net: Allow DNS lookups on nodes with IPV6 lo only 
    AI_ADDRCONFIG prevents ::1 from being considered a valid address on hosts that have a IPV6 loopback IP address but no other IPV6 interfaces.
    60a753b7b3
  19. m3dwards force-pushed on Jun 21, 2024
  20. m3dwards commented at 4:10 pm on June 21, 2024: contributor
    Thanks for your review @vasild, should have addressed all of your comments.
  21. vasild approved
  22. vasild commented at 3:31 pm on June 25, 2024: contributor

    ACK 60a753b7b38fbe89494f8df66f13a84f28af244b

    Thank you!


m3dwards DrahtBot vasild

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me