Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fd2cee
net: disconnect peers by address without using a subnet
vasild Jan 4, 2021
98140bc
test: add test to ensure non-IP peers are properly misbehaved
vasild Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2814,7 +2814,15 @@ bool CConnman::DisconnectNode(const CSubNet& subnet)

bool CConnman::DisconnectNode(const CNetAddr& addr)
{
return DisconnectNode(CSubNet(addr));
bool disconnected = false;
Copy link
Member

@laanwj laanwj Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

LOCK(cs_vNodes);
for (CNode* pnode : vNodes) {
if (addr == pnode->addr) {
pnode->fDisconnect = true;
disconnected = true;
}
}
return disconnected;
}

bool CConnman::DisconnectNode(NodeId id)
Expand Down
81 changes: 59 additions & 22 deletions src/test/denialofservice_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,47 +223,84 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
{
const CChainParams& chainparams = Params();
auto banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME);
auto connman = MakeUnique<CConnman>(0x1337, 0x1337);
auto connman = MakeUnique<CConnmanTest>(0x1337, 0x1337);
auto peerLogic = std::make_unique<PeerManager>(chainparams, *connman, banman.get(), *m_node.scheduler,
*m_node.chainman, *m_node.mempool, false);

std::vector<CNode*> nodes(3);

banman->ClearBanned();
CAddress addr1(ip(0xa0b0c001), NODE_NONE);
CNode dummyNode1(id++, NODE_NETWORK, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::INBOUND);
dummyNode1.SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(&dummyNode1);
dummyNode1.fSuccessfullyConnected = true;
peerLogic->Misbehaving(dummyNode1.GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ ""); // Should be discouraged
nodes[0] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr1, 0, 0, CAddress(), "", ConnectionType::INBOUND);
nodes[0]->SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(nodes[0]);
nodes[0]->fSuccessfullyConnected = true;
connman->AddNode(*nodes[0]);
peerLogic->Misbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ ""); // Should be discouraged
{
LOCK(dummyNode1.cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(&dummyNode1));
LOCK(nodes[0]->cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
}
BOOST_CHECK(banman->IsDiscouraged(addr1));
BOOST_CHECK(nodes[0]->fDisconnect);
BOOST_CHECK(!banman->IsDiscouraged(ip(0xa0b0c001|0x0000ff00))); // Different IP, not discouraged

CAddress addr2(ip(0xa0b0c002), NODE_NONE);
CNode dummyNode2(id++, NODE_NETWORK, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND);
dummyNode2.SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(&dummyNode2);
dummyNode2.fSuccessfullyConnected = true;
peerLogic->Misbehaving(dummyNode2.GetId(), DISCOURAGEMENT_THRESHOLD - 1, /* message */ "");
nodes[1] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr2, 1, 1, CAddress(), "", ConnectionType::INBOUND);
nodes[1]->SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(nodes[1]);
nodes[1]->fSuccessfullyConnected = true;
connman->AddNode(*nodes[1]);
peerLogic->Misbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1, /* message */ "");
{
LOCK(dummyNode2.cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
LOCK(nodes[1]->cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
}
BOOST_CHECK(!banman->IsDiscouraged(addr2)); // 2 not discouraged yet...
BOOST_CHECK(!nodes[1]->fDisconnect);
BOOST_CHECK(banman->IsDiscouraged(addr1)); // ... but 1 still should be
peerLogic->Misbehaving(dummyNode2.GetId(), 1, /* message */ ""); // 2 reaches discouragement threshold
BOOST_CHECK(nodes[0]->fDisconnect);
peerLogic->Misbehaving(nodes[1]->GetId(), 1, /* message */ ""); // 2 reaches discouragement threshold
{
LOCK(nodes[1]->cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
}
// Expect both 1 and 2 to be discouraged now.
BOOST_CHECK(banman->IsDiscouraged(addr1));
BOOST_CHECK(nodes[0]->fDisconnect);
BOOST_CHECK(banman->IsDiscouraged(addr2));
BOOST_CHECK(nodes[1]->fDisconnect);

// Make sure non-IP peers get discouraged and disconnected properly.

CNetAddr tor_netaddr;
BOOST_REQUIRE(
tor_netaddr.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"));

const CAddress addr3(CService(tor_netaddr, Params().GetDefaultPort()), NODE_NONE);
nodes[2] = new CNode(id++, NODE_NETWORK, INVALID_SOCKET, addr3, 1, 1, CAddress(), "",
ConnectionType::OUTBOUND_FULL_RELAY);
nodes[2]->SetCommonVersion(PROTOCOL_VERSION);
peerLogic->InitializeNode(nodes[2]);
connman->AddNode(*nodes[2]);
nodes[2]->fSuccessfullyConnected = true;
peerLogic->Misbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD, /* message */ "");
{
LOCK(dummyNode2.cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(&dummyNode2));
LOCK(nodes[2]->cs_sendProcessing);
BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
}
BOOST_CHECK(banman->IsDiscouraged(addr1));
BOOST_CHECK(banman->IsDiscouraged(addr2));
BOOST_CHECK(banman->IsDiscouraged(addr3));
for (CNode* node : nodes) {
BOOST_CHECK(node->fDisconnect);
Copy link
Member

@jonatack jonatack Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test code fails at the right place on master

test/denialofservice_tests.cpp(292): info: check banman->IsDiscouraged(addr1) has passed
test/denialofservice_tests.cpp(293): info: check banman->IsDiscouraged(addr2) has passed
test/denialofservice_tests.cpp(294): info: check banman->IsDiscouraged(addr3) has passed
test/denialofservice_tests.cpp(296): info: check node->fDisconnect has passed
test/denialofservice_tests.cpp(296): info: check node->fDisconnect has passed
test/denialofservice_tests.cpp(296): error: in "denialofservice_tests/peer_discouragement": check node->fDisconnect has failed
test/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[]

}
BOOST_CHECK(banman->IsDiscouraged(addr1)); // Expect both 1 and 2
BOOST_CHECK(banman->IsDiscouraged(addr2)); // to be discouraged now

bool dummy;
peerLogic->FinalizeNode(dummyNode1, dummy);
peerLogic->FinalizeNode(dummyNode2, dummy);
for (CNode* node : nodes) {
peerLogic->FinalizeNode(*node, dummy);
}
connman->ClearNodes();
}

BOOST_AUTO_TEST_CASE(DoS_bantime)
Expand Down