Periodically update DNS caches for better privacy of non-reachable nodes

naumenkogs commented at 3:58 pm on March 24, 2020: member

Every time a fresh Bitcoin Core node starts, it makes a DNS query to learn about nodes in the network. This process leaks the privacy of those new nodes: every required DNS server and the corresponding infrastructure would be aware that a new node was spinned up in a particular internet segment. The goal of this proposal is to reduce the number of those actors learning about a new node. The way to achieve it is to keep DNS server caches updated, so that new nodes rarely hit anything past the early servers.

To keep them updated, every reachable node would periodically make all widely used DNS queries, thus, triggering DNS cache updates on the resolvers appearing on their path to the end DNS servers. This, obviously, would leak more information about the existence of these reachable nodes. We think this is no big deal, because those nodes are already easy to find, since they are reachable.

Note that this helps only to those private nodes, which share a subnet with some reachable node running this code.

In future, it would be great to analyze the results of those queries against a local AddrMan to check for anomalies.

The idea was originally proposed by Greg Maxwell.

naumenkogs renamed this:
~~Periodically update DNS caches for better privacy of non reachable-nodes~~
Periodically update DNS caches for better privacy of non-reachable nodes
on Mar 24, 2020

laanwj added the label P2P on Mar 24, 2020

DrahtBot commented at 4:18 pm on March 24, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19316 ([net] Cleanup logic around connection types by amitiuttarwar)
#19064 (refactor: Cleanup thread ctor calls by hebasto)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa commented at 4:19 pm on March 24, 2020: member

Can this use the scheduler instead of using a separate thread?

practicalswift commented at 4:42 pm on March 24, 2020: contributor

Concept ACK

naumenkogs force-pushed on Mar 24, 2020

naumenkogs commented at 4:42 pm on March 24, 2020: member

@sipa yes! I simply forgot that this can be done with scheduler. Just force-pushed an update.

in doc/reduce-traffic.md:33 in 5e87cba47a outdated

27@@ -28,7 +28,9 @@ calculating the target.
28 
29 Disabling listening will result in fewer nodes connected (remember the maximum of 8
30 outbound peers). Fewer nodes will result in less traffic usage as you are relaying
31-blocks and transactions to fewer nodes.
32+blocks and transactions to fewer nodes. This would also prevent reachable nodes from
33+periodically querying DNS servers when it is not needed (feature, which gives more
34+privacy to the non-reachable nodes located in the same subnet).

MarcoFalke commented at 4:44 pm on March 24, 2020:

I think this in an implementation detail which people that want to reduce their traffic are not interested in learning about.

A release notes snippet would be a better place to put this, no?

naumenkogs commented at 4:47 pm on March 24, 2020:

Yeah, I was hesitant about this part. Committed this for now simply to not ignore this. Unless someone expresses support for keeping this, I would remove this commit later.

in src/net.cpp:51 in 5e87cba47a outdated

47@@ -48,6 +48,9 @@ static_assert(MINIUPNPC_API_VERSION >= 10, "miniUPnPc API version >= 10 assumed"
48 // How often to dump addresses to peers.dat
49 static constexpr std::chrono::minutes DUMP_PEERS_INTERVAL{15};
50 
51+// How often to make a DNS caches updating query

MarcoFalke commented at 4:57 pm on March 24, 2020:

nit: Could use doxygen comment?

0/** How often to make a DNS caches updating query */

in src/net.cpp:1593 in 5e87cba47a outdated

1588+    // due to external factors (e.g. local network settings update).
1589+    if (CountInboundConnections() == 0) return;
1590+
1591+    // Every time a new widely-requests service bit is deployed, this list should be
1592+    // updated to invalide records for that service bit.
1593+    std::vector<uint64_t> service_bits_combinations{

MarcoFalke commented at 5:02 pm on March 24, 2020:

nit: I think this list reads easier if each entry was on its own line.

nit: compile time constants should be all upper case and static

0    static const std::vector<uint64_t> SERVICE_BITS_COMBINATIONS{

in src/net.h:482 in 5e87cba47a outdated

478@@ -468,6 +479,7 @@ class CConnman
479 
480     CThreadInterrupt interruptNet;
481 
482+    std::thread thread_dns_caches_update;

MarcoFalke commented at 5:05 pm on March 24, 2020:

nit: Should be removed now?

naumenkogs commented at 5:28 pm on March 24, 2020:

oops. yes!

in src/net.h:373 in 5e87cba47a outdated

368+    // they are likely to hit the cache instead.
369+    // For non-reachable nodes this means more privacy,
370+    // because fewer actors would know about their query.
371+    // For reachable nodes there is no privacy loss due to this,
372+    // because being reachable already makes them public.
373+    void DNSCachesUpdate();

MarcoFalke commented at 5:06 pm on March 24, 2020:

nit: Both of the newly added methods can be const because they are read-only, no?

0    void DNSCachesUpdate() const;

MarcoFalke commented at 5:24 pm on March 24, 2020: member

Just some style-nits

MarcoFalke commented at 5:31 pm on March 24, 2020: member

Can this use the scheduler instead of using a separate thread?

What is the maximum time this can take? If it is unbounded, it seems that this would be a trivial DOS attack against a node, no?

naumenkogs force-pushed on Mar 24, 2020

naumenkogs commented at 11:30 pm on March 24, 2020: member

@sipa I was not aware that scheduled event will block other execution threads, and now that Marco pointed out that attack, i’m thinking separate thread makes more sense? We don’t want to make any assumptions about these requests executing very fast.

MarcoFalke commented at 11:40 pm on March 24, 2020: member

LimitValidationInterfaceQueue is called in ActivateBestChain. So even validation itself can be blocked, if an attacker can make those requests go slow. Not sure if that is possible, but I wanted to raise the point.

sipa commented at 0:09 am on March 26, 2020: member

@naumenkogs It won’t block other threads, but it will delay other events in the same scheduler. I agree that it’s probably too slow for that (I don’t know what latency getaddrinfo can have, but I expect it may be substantial). An alternative is doing it from PeerValidationLogic::SendMessages, once a timer expires. I’m hammering on avoiding a separate thread for this, because threads have a significant memory footprint (several MB, at least), so a thread that’s idle almost all the time is a waste. @MarcoFalke Vaguely related, do we have any policies around what maximum execution duration things in the main scheduler are allowed to have?

naumenkogs commented at 1:41 am on March 26, 2020: member

Agreed that that RAM overhead of a new thread for just this is no good.

Yeah, I would say normal execution of this function takes 2s, abnormal may take…a minute even if everyone is honest, due to random internet/os glitches. Do we have anything in our toolset to make sure Lookup() function doesn’t take more than couple seconds? I don’t see anything within scheduler. Maybe it’s doable to add this feature?

An alternative crazy idea is to have a special thread for a number of rare not-so-critical functions, which might take long time (for example, I can imagine DumpAddresses takes long due to I/O issues).

Separate a function for counting inbound connections 3c8908347c

Periodically query DNS for better privacy of non-reachable nodes

Reachable nodes would query DNS servers every hour
for all widely used combinations of service bits.
This would give non-reachable nodes from the same subnets
more privacy, because they will be hitting early DNS caches.

de74888e9d

naumenkogs force-pushed on Apr 16, 2020

in src/net.cpp:1589 in de74888e9d

1584+    // Only reachable nodes do this, because those are already easy to find.
1585+    // Unless the node is explicitly set to be non-reachable (-listen=0),
1586+    // the only way to check if it is reachable is to observe inbound connections.
1587+    // This check is made every time, because a node may become non-reachable
1588+    // due to external factors (e.g. local network settings update).
1589+    if (CountInboundConnections() == 0) return;

luke-jr commented at 11:39 pm on April 22, 2020:

How about a -dnscloaking option to force it on/off?

naumenkogs commented at 10:47 pm on April 23, 2020:

Good idea. Will add this config and add the documentation once we get more concept acks.

in src/net.cpp:1595 in de74888e9d

1590+
1591+    // Every time a new widely-requests service bit is deployed, this list should be
1592+    // updated to invalide records for that service bit.
1593+    std::vector<uint64_t> SERVICE_BITS_COMBINATIONS{
1594+        // Legacy nodes
1595+        NODE_NETWORK,

luke-jr commented at 11:40 pm on April 22, 2020:

Wouldn’t these be requesting the bare name without any service bit specification?

naumenkogs commented at 10:50 pm on April 23, 2020:

I think this should be x1.dns.whatever.com?

in src/net.cpp:1596 in de74888e9d

1591+    // Every time a new widely-requests service bit is deployed, this list should be
1592+    // updated to invalide records for that service bit.
1593+    std::vector<uint64_t> SERVICE_BITS_COMBINATIONS{
1594+        // Legacy nodes
1595+        NODE_NETWORK,
1596+        NODE_NETWORK_LIMITED,

luke-jr commented at 11:40 pm on April 22, 2020:

What nodes would ever request this?

in src/net.cpp:1619 in de74888e9d

1614+        LookupHost(seed, ips, 1, true);
1615+        // TODO: Here and below, make sure downloaded IPs are not very different
1616+        // from what stored locally, to identify AddrMan or DNS poisoning.
1617 
1618+        // query for all widely used combinations of service bits
1619+        for (uint64_t service_bits_combination : SERVICE_BITS_COMBINATIONS) {

luke-jr commented at 11:42 pm on April 22, 2020:

Should these really be done all at the same time? No new node would batch them like this…?

naumenkogs commented at 10:52 pm on April 23, 2020:

Well, one private node in the subnet will request one combination, another private node will request another one…

Why would you want the already public node separate cache updates in time?

in src/net.cpp:2382 in de74888e9d

2374@@ -2322,8 +2375,13 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2375 
2376     if (!gArgs.GetBoolArg("-dnsseed", true))
2377         LogPrintf("DNS seeding disabled\n");
2378-    else
2379+    else {
2380         threadDNSAddressSeed = std::thread(&TraceThread<std::function<void()> >, "dnsseed", std::function<void()>(std::bind(&CConnman::ThreadDNSAddressSeed, this)));
2381+        if (fListen && fNameLookup && !HaveNameProxy()) {
2382+            LogPrintf("Proactive querying DNS servers to update caches is enabled\n");
2383+            scheduler.scheduleEvery([this] { DNSCachesUpdate(); }, DNS_CACHES_UPDATE_INTERVAL);

luke-jr commented at 11:45 pm on April 22, 2020:

Probably there should be some randomness added to the interval for each iteration?

luke-jr changes_requested

TheBlueMatt commented at 9:40 pm on May 8, 2020: member

I thought we had two scheduler threads? If not we should just do that and then its not a problem.

in src/net.cpp:947 in de74888e9d

939@@ -937,6 +940,16 @@ bool CConnman::AttemptToEvictConnection()
940     return false;
941 }
942 
943+int CConnman::CountInboundConnections() const
944+{
945+    int inbounds = 0;
946+    LOCK(cs_vNodes);
947+    for (const CNode* node : vNodes) {

TheBlueMatt commented at 9:42 pm on May 8, 2020:

Can we count that such connections are from non-local addresses? Should be an extra two LoC

DrahtBot added the label Needs rebase on Aug 12, 2020

DrahtBot commented at 2:43 am on August 12, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

in src/net.cpp:1627 in de74888e9d

1622+            }
1623+            std::string host = strprintf("x%x.%s", ServiceFlags(service_bits_combination), seed);
1624+            LookupHost(host, ips, 1, true);
1625+        }
1626+    }
1627+    LogPrintf("DNS requests were made to update caches.\n");

practicalswift commented at 9:57 am on August 12, 2020:

This could be logged to a debug category to avoid reducing the signal to noise in the default bitcoind output? We already print too much along the lines of “everything is working as expected” which makes it easy to miss the more relevant “oh, something seems broken” log messages :)

in src/net.cpp:2381 in de74888e9d

2374@@ -2322,8 +2375,13 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2375 
2376     if (!gArgs.GetBoolArg("-dnsseed", true))
2377         LogPrintf("DNS seeding disabled\n");
2378-    else
2379+    else {
2380         threadDNSAddressSeed = std::thread(&TraceThread<std::function<void()> >, "dnsseed", std::function<void()>(std::bind(&CConnman::ThreadDNSAddressSeed, this)));
2381+        if (fListen && fNameLookup && !HaveNameProxy()) {
2382+            LogPrintf("Proactive querying DNS servers to update caches is enabled\n");

practicalswift commented at 9:58 am on August 12, 2020:

This could be logged to a debug category to avoid reducing the signal to noise in the default bitcoind output?

naumenkogs commented at 10:35 am on September 29, 2021: member

Not sure what to do with this PR. The show-stopper here was the discussion around the blocking behavior of DNS queries: the options were either add a new thread or employing existing threads for such non-critical task (see discussion).

It kind of went nowhere, so I’m gonna close this PR for now. If you have an opinion on the way to move forward with one of those paths, please go to the mentioned discussion.

If you have an alternative way to move forward, feel free to comment here and we can resurrect it.

naumenkogs closed this on Sep 29, 2021

DrahtBot locked this on Oct 30, 2022

Periodically update DNS caches for better privacy of non-reachable nodes #18421

Conflicts