net: disconnect peers by address without using a subnet

vasild commented at 4:25 pm on January 4, 2021: member

Previously, when disconnecting a peer with a given address we would create a dummy subnet that contains just 1 address (/32 for IPv4 and /128 for IPv6) and would disconnect all peers that belong to this subnet (there may be more than one connection, to a different port).

The problem is that for any non-IPv4 and non-IPv6 address we would create an invalid subnet which would later not match any addresses.

Thus, don’t use a one-host-subnet, but compare the addresses directly. This works for any type of addresses.

Previously, when disconnecting a peer with a given address we would
create a dummy subnet that contains just 1 address (/32 for IPv4 and
/128 for IPv6) and would disconnect all peers that belong to this
subnet (there may be more than one connection, to a different port).

The problem is that for any non-IPv4 and non-IPv6 address we would
create an invalid subnet which would later not match any addresses.

Thus, don't use a one-host-subnet, but compare the addresses directly.
This works for any type of addresses.

5fd2cee7fb

vasild commented at 4:27 pm on January 4, 2021: member

The problem was discovered by @MarcoFalke.

MarcoFalke added this to the milestone 0.21.0 on Jan 4, 2021

MarcoFalke added the label Needs backport (0.21) on Jan 4, 2021

MarcoFalke added the label P2P on Jan 4, 2021

MarcoFalke commented at 4:35 pm on January 4, 2021: member

tested-only ACK 5fd2cee7fbc38c677d08a1545770156cc061833e did not review the code 🔮

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3tested-only ACK 5fd2cee7fbc38c677d08a1545770156cc061833e did not review the code 🔮
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh/Sgv/W9d+TPMjSm+JqcwJBW7n3wmto0PDu+CvGkHSwHJaoLU7toj8W1y6Ohfz
 8CDKRGq2ZG+VnxSbKRhItSeaWqleHfAJ0Ns1xZNbDudAw7/IrLMsvNd7+/apFgpUT
 9iM1ytrZMRRLqoPdRVgpzkamKMUyPTPzzXzGQoH6xdxSjMuKbJ69DVzg4d0DuAmH+
10y+ZP85yi7KDq5YvS25JUaOpZg4m9UnwTHctL1+hMeIWBtxlx3pFohExswbUC28xz
11ytN4dWFxyQXC/t3i3fY3AiLiHZZ0frxHTd26sL/vqj935MWnH3SsIpIwm57kl1v2
12vHR7yakgWEcDF5qEQHa1+wYi1p26BgYQYjlPqYCM4fp+0MBg6lPpggthZkYVKTXI
13Jg2mxdUl/kuFDA8Xy9u00uQj6BOWJfshBuClQkxf5184qTcMdQ6SwxagpfDGHEjn
14JDHbX+djj8+CfR6CVc3P7MWP1UxnuFI8NWiyMI6KrcEH3ESw16LmcrS3KOeLLI4Q
155IVN+71Z
16=kXbR
17-----END PGP SIGNATURE-----

Timestamp of file with hash ffb08779a664f7e34db6952de6f0948d1fb8eeb7089b81525f9ffde67a8ac202 -

MarcoFalke referenced this in commit 6f256c267b on Jan 4, 2021

jonatack commented at 4:40 pm on January 4, 2021: member

Code review ACK 5fd2cee7fbc38c677d08a1545770156cc061833e

It would be good to have a failing test case that this patch fixes, either unit tests or in the functional rpc_setban or p2p_disconnect_ban ones.

vasild commented at 5:57 pm on January 4, 2021: member

#20852 contains a fix for this issue + it also fixes a similar issue in BanMan. If it is merged, then this can be closed without merge. They do not conflict in any way. Both can go in.

jonatack commented at 12:20 pm on January 5, 2021: member

Any tips how to test this manually? Disconnecting outbound onion peers with disconnectnode works for me on both master and this PR and in #20852.

vasild commented at 4:21 pm on January 5, 2021: member

@jonatack yes, the disconnectnode RPC uses either DisconnectNode(std::string) or DisconnectNode(NodeId) and the problem is in DisconnectNode(CNetAddr).

bitcoin-cli setban foo.onion add will do the job.

taurus228 commented at 4:26 pm on January 5, 2021: none

вт, 5 янв. 2021 г. в 19:24, Vasil Dimov notifications@github.com:

@jonatack https://github.com/jonatack yes, the disconnectnode RPC uses either DisconnectNode(std::string) or DisconnectNode(NodeId) and the problem is in DisconnectNode(CNetAddr).

bitcoin-cli setban foo.onion add will do the job.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/20849#issuecomment-754740638, or unsubscribe https://github.com/notifications/unsubscribe-auth/AK4ZHQKMTWFQ66BD3TC7WTLSYM4J7ANCNFSM4VTHTMFA .

– Nik

sipa commented at 6:06 pm on January 5, 2021: member

utACK 5fd2cee7fbc38c677d08a1545770156cc061833e

Let’s get this merged so the path forward for 0.21 is clear (we can still do #20852, but it’s no longer critical then).

jonatack commented at 12:06 pm on January 6, 2021: member

Adding logging to the code path and verified that setban <onion> add disconnects peers with this patch and fails to do so on master @ 68196a891056f, with both tor v2 and v3 peers.

Logging added to master and to this patch:

0@@ -2805,6 +2805,7 @@ bool CConnman::DisconnectNode(const CSubNet& subnet)
1     LOCK(cs_vNodes);
2     for (CNode* pnode : vNodes) {
3         if (subnet.Match(pnode->addr)) {
4+            LogPrintf("Disconnecting %s via subnet peer=%d address=%s network=%s\n",
5+                      pnode->ConnectionTypeAsString(), pnode->GetId(),
6+                      pnode->addr.ToString(), GetNetworkName(pnode->ConnectedThroughNetwork()));
7             pnode->fDisconnect = true;
8             disconnected = true;
9         }

Logging also added to this patch:

0@@ -2818,6 +2819,7 @@ bool CConnman::DisconnectNode(const CNetAddr& addr)
1     LOCK(cs_vNodes);
2     for (CNode* pnode : vNodes) {
3         if (addr == pnode->addr) {
4+            LogPrintf("Disconnecting %s via addr peer=%d address=%s network=%s\n",
5+                      pnode->ConnectionTypeAsString(), pnode->GetId(),
6+                      pnode->addr.ToString(), GetNetworkName(pnode->ConnectedThroughNetwork()));
7             pnode->fDisconnect = true;
8             disconnected = true;
9         }

On master, disconnection fails and nothing is logged. With this patch, the peer is immediately removed from the -netinfo 4 peers dashboard and the log shows:

02021-01-06T11:47:36Z Disconnecting manual via addr peer=5 address=TOR_V3_PEER network=onion
12021-01-06T11:54:30Z Disconnecting manual via addr peer=25 address=TOR_V2_PEER network=onion
22021-01-06T11:54:49Z Disconnecting outbound-full-relay via addr peer=16 address=TOR_V2_PEER:8333 network=onion

Upgrading to tested ACK 5fd2cee7fbc38c677d08a1545770156cc061833e

test: add test to ensure non-IP peers are properly misbehaved

Change the `peer_discouragement` test to use `CNode` pointers so that
the nodes it uses can be added to `CConnman::vNodes` and cleaned up
properly. Make it use `CConnmanTest` instead of `CConnman`. This is
needed because we want to check `CNode::fDisconnect` and for this flag
to be flipped by `CConnman::DisconnectNode()` the node must be in
`CConnman::vNodes`.

Extend the test with one torv3 peer and check that it is discouraged and
disconnected as expected.

98140bc855

vasild commented at 2:55 pm on January 6, 2021: member

Appended an extra commit with unit test that fails on master and passes on this PR.

If it does not get re-ACKs or if it turns out to be problematic I will remove it, restoring the already ACK’ed commit id.

jonatack commented at 3:33 pm on January 6, 2021: member

Nice, will review asap.

DrahtBot commented at 7:03 pm on January 6, 2021: 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:

#20811 (refactor: move net_processing implementation details out of header by ajtowns)
#20228 (addrman: Make addrman a top-level component by jnewbery)

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.

in src/test/denialofservice_tests.cpp:296 in 98140bc855

309+    }
310+    BOOST_CHECK(banman->IsDiscouraged(addr1));
311+    BOOST_CHECK(banman->IsDiscouraged(addr2));
312+    BOOST_CHECK(banman->IsDiscouraged(addr3));
313+    for (CNode* node : nodes) {
314+        BOOST_CHECK(node->fDisconnect);

jonatack commented at 7:58 pm on January 6, 2021:

The new test code fails at the right place on master

0test/denialofservice_tests.cpp(292): info: check banman->IsDiscouraged(addr1) has passed
1test/denialofservice_tests.cpp(293): info: check banman->IsDiscouraged(addr2) has passed
2test/denialofservice_tests.cpp(294): info: check banman->IsDiscouraged(addr3) has passed
3test/denialofservice_tests.cpp(296): info: check node->fDisconnect has passed
4test/denialofservice_tests.cpp(296): info: check node->fDisconnect has passed
5test/denialofservice_tests.cpp(296): error: in "denialofservice_tests/peer_discouragement": check node->fDisconnect has failed
6test/denialofservice_tests.cpp(222): Leaving test case "peer_discouragement"; testing time: 32905us

nit, maybe not worth it for tests, could use nodes.at() notation for bounds checking instead of nodes[]

jonatack commented at 8:36 pm on January 6, 2021: member

ACK 98140bc855b037d2c552153496e8e4e2c2efdad9 code review, manual testing with setban OUTBOUND_ONION add, and verified the new test fails on master on the non-IP Tor v3 peer disconnection assertion.

in src/net.cpp:2817 in 98140bc855

2813@@ -2814,7 +2814,15 @@ bool CConnman::DisconnectNode(const CSubNet& subnet)
2814 
2815 bool CConnman::DisconnectNode(const CNetAddr& addr)
2816 {
2817-    return DisconnectNode(CSubNet(addr));
2818+    bool disconnected = false;

laanwj commented at 2:49 pm on January 7, 2021:

Is this change still needed with #20852? It seems to me that makes the old, much more compact, construction work?

vasild commented at 3:13 pm on January 7, 2021:

No, if #20852 is merged, then this PR is not needed, see #20849 (comment).

I first opened this PR to fix DisconnectNode() and then realized that the same problem exists in BanMan, thus opened #20852 which fixes both by modifying CSubNet.

Maybe this PR could be preferred as less risky compared to #20852. It is ok if this is merged first and later #20852 is also merged - they do not conflict and both go in master.

vasild commented at 10:05 am on January 8, 2021: member

Just to elaborate on the relationship between #20849 and #20852:

The problem in master is that CSubNet(foo.onion) creates an invalid subnet which later does not match any address, not even foo.onion. In two places in the code we expect that once we create such subnet later it will match foo.onion: in CConnman::DisconnectNode() and in BanMan.

#20849 fixes DisconnectNode() by changing it to not use subnets at all (small isolated change, easy to verify it is correct).

#20852 changes CSubNet to follow the above expected behavior, thus fixing both DisconnectNode() and BanMan (bigger change, harder to verify it is correct, caused unexpected CI failure (fixed)).

About the release, maybe the following makes sense:

Include #20849 in the next rc and in the 0.21.0 release.

Later, if #20852 is merged (ever) there is no need to revert #20849.

MarcoFalke commented at 10:15 am on January 11, 2021: member

It might be better to wait for #20852 (0.21.1), after which this is not needed.

fanquake closed this on Jan 11, 2021

in src/test/denialofservice_tests.cpp:226 in 98140bc855

222@@ -223,47 +223,84 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
223 {
224     const CChainParams& chainparams = Params();
225     auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
226-    auto connman = MakeUnique<CConnman>(0x1337, 0x1337);
227+    auto connman = MakeUnique<CConnmanTest>(0x1337, 0x1337);

MarcoFalke commented at 12:40 pm on January 11, 2021:

std::make_unique for new code?

Also, could split the refactoring changes from the new-test changes?

MarcoFalke commented at 12:44 pm on January 11, 2021:

Nvm, forget the std::make_unique suggestion. We want to backport the test changes.

MarcoFalke commented at 12:43 pm on January 11, 2021: member

Concept ACK on adding the test. Mind to open a new pull for this?

vasild deleted the branch on Jan 11, 2021

vasild commented at 4:39 pm on January 11, 2021: member

The test is included in #20904.

MarcoFalke removed the label Needs backport (0.21) on Mar 13, 2021

fanquake deleted a comment on Aug 15, 2021

fanquake locked this on Aug 15, 2021

net: disconnect peers by address without using a subnet #20849

Conflicts