Skip to content
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

Discv5 Routing Table: Add support for banning nodes #768

Merged
merged 7 commits into from
Jan 24, 2025
Merged

Discv5 Routing Table: Add support for banning nodes #768

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c0c3ba0
Add banned nodes to routing table.
bhartnett Jan 20, 2025
59b17d1
Add tests.
bhartnett Jan 21, 2025
6cdd9cd
Fix test.
bhartnett Jan 21, 2025
e6a542c
Reduce sleep time in test.
bhartnett Jan 22, 2025
52b72df
Cleanup expired bans from banned nodes table when adding a new ban.
bhartnett Jan 22, 2025
f672d72
Make cleanupExpiredBans public so it can be called independently of b…
bhartnett Jan 22, 2025
1e8110c
Init bannedNodes field.
bhartnett Jan 23, 2025
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
57 changes: 56 additions & 1 deletion eth/p2p/discoveryv5/routing_table.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import
../../net/utils,
"."/[node, random2, enr]

export results
export results, chronos.timer

declareGauge routing_table_nodes,
"Discovery routing table nodes", labels = ["state"]
Expand Down Expand Up @@ -47,6 +47,10 @@ type
## replacement caches.
distanceCalculator: DistanceCalculator
rng: ref HmacDrbgContext
bannedNodes: Table[NodeId, chronos.Moment] ## Nodes can be banned from the
## routing table for a period until the timeout is reached. Banned nodes
## are removed from the routing table and not allowed to be included again
## until the timeout expires.

KBucket = ref object
istart, iend: NodeId ## Range of NodeIds this KBucket covers. This is not a
Expand Down Expand Up @@ -95,6 +99,7 @@ type
ReplacementAdded
ReplacementExisting
NoAddress
Banned

# xor distance functions
func distance*(a, b: NodeId): UInt256 =
Expand Down Expand Up @@ -189,6 +194,53 @@ func ipLimitDec(r: var RoutingTable, b: KBucket, n: Node) =
b.ipLimits.dec(ip)
r.ipLimits.dec(ip)

func getNode*(r: RoutingTable, id: NodeId): Opt[Node]
proc replaceNode*(r: var RoutingTable, n: Node)

proc cleanupExpiredBans(r: var RoutingTable) =
let currentTime = now(chronos.Moment)

var expiredIds = newSeq[NodeId]()
for id, timeout in r.bannedNodes:
if currentTime >= timeout:
expiredIds.add(id)

for id in expiredIds:
r.bannedNodes.del(id)

proc banNode*(r: var RoutingTable, nodeId: NodeId, period: chronos.Duration) =
## Ban a node from the routing table for the given period. The node is removed
## from the routing table and replaced using a node from the replacement cache.
let banTimeout = now(chronos.Moment) + period

if r.bannedNodes.contains(nodeId):
let existingTimeout = r.bannedNodes.getOrDefault(nodeId)
if existingTimeout < banTimeout:
r.bannedNodes[nodeId] = banTimeout
return # node is already banned so we don't need to try replacing it because
# it should have already been replaced when it was initially banned

# NodeId doesn't yet exist in the banned nodes table
r.bannedNodes[nodeId] = banTimeout

# Remove the node from the routing table
let node = r.getNode(nodeId)
if node.isSome():
r.replaceNode(node.get())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we replace the node as I've done here or simply remove it without using the replacement cache?

Copy link
Contributor

Choose a reason for hiding this comment

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

Replace is the way to go 👍

In general we never really use remove AFAIK (maybe in testing?), always use the replacement cache as that cache is not actively used to search for nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok great, I'll leave it as it is then.


# Remove all expired bans from the banned nodes table
r.cleanupExpiredBans()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having the cleanup here is potentially worse than the actual mem leak.

Allowing this to run in the banNode call effectively puts it in an attacker's power to run this cleanup. While at the same time the attacker can make it more expensive by "adding bans".

I think that this type of clean-up call should always be run independently of any peer node actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see so the concern in this case is the computational overhead of looping over the table rather than the list growing?

Did you have a suggestion for where you think the cleanup should happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now removed the call to cleanupExpiredBans from the banPeer function and made it public so it can be called at a high layer.


proc isBanned*(r: RoutingTable, nodeId: NodeId): bool =
if not r.bannedNodes.contains(nodeId):
return false

let
currentTime = now(chronos.Moment)
banTimeout = r.bannedNodes.getOrDefault(nodeId)

currentTime < banTimeout

proc add(k: KBucket, n: Node) =
k.nodes.add(n)
routing_table_nodes.inc()
Expand Down Expand Up @@ -343,6 +395,9 @@ proc addNode*(r: var RoutingTable, n: Node): NodeStatus =
if n == r.localNode:
return LocalNode

if r.isBanned(n.id):
return Banned

let bucket = r.bucketForNode(n.id)

## Check if the node is already present. If so, check if the record requires
Expand Down
99 changes: 99 additions & 0 deletions tests/p2p/test_routing_table.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{.used.}

import
std/os,
unittest2,
../../eth/common/keys, ../../eth/p2p/discoveryv5/[routing_table, node, enr],
./discv5_test_helper
Expand Down Expand Up @@ -561,3 +562,101 @@ suite "Routing Table Tests":
# there may be more than one node at provided distance
check len(neighboursAtLogDist) >= 1
check neighboursAtLogDist.contains(n)

test "Banned nodes: banned node cannot be added":
let
localNode = generateNode(PrivateKey.random(rng[]))
node1 = generateNode(PrivateKey.random(rng[]))
node2 = generateNode(PrivateKey.random(rng[]))

var table = RoutingTable.init(localNode, 1, DefaultTableIpLimits, rng = rng)

# Can add a node that is not banned
check:
table.contains(node1) == false
table.isBanned(node1.id) == false
table.addNode(node1) == Added
table.contains(node1) == true
table.isBanned(node1.id) == false

# Can ban a node that exists in the routing table
table.banNode(node1.id, 1.minutes)
check:
table.contains(node1) == false # the node is removed when banned
table.isBanned(node1.id) == true
table.addNode(node1) == Banned # the node cannot be added while banned
table.contains(node1) == false
table.getNode(node1.id).isNone()
table.isBanned(node1.id) == true

# Can ban a node that doesn't yet exist in the routing table
check:
table.contains(node2) == false
table.isBanned(node2.id) == false

table.banNode(node2.id, 1.minutes)
check:
table.contains(node2) == false
table.isBanned(node2.id) == true
table.addNode(node2) == Banned # the node cannot be added while banned
table.contains(node2) == false
table.getNode(node2.id).isNone()
table.isBanned(node2.id) == true

test "Banned nodes: nodes with expired bans can be added":
let
localNode = generateNode(PrivateKey.random(rng[]))
node1 = generateNode(PrivateKey.random(rng[]))
node2 = generateNode(PrivateKey.random(rng[]))

var table = RoutingTable.init(localNode, 1, DefaultTableIpLimits, rng = rng)

check table.addNode(node1) == Added
table.banNode(node1.id, 1.nanoseconds)
table.banNode(node2.id, 1.nanoseconds)

sleep(1)

# Can add nodes for which the ban has expired
check:
table.contains(node1) == false
table.isBanned(node1.id) == false
table.addNode(node1) == Added
table.contains(node1) == true
table.isBanned(node1.id) == false

table.contains(node2) == false
table.isBanned(node2.id) == false
table.addNode(node2) == Added
table.contains(node2) == true
table.isBanned(node2.id) == false

test "Banned nodes: ban nodes with existing bans":
let
localNode = generateNode(PrivateKey.random(rng[]))
node1 = generateNode(PrivateKey.random(rng[]))
node2 = generateNode(PrivateKey.random(rng[]))

var table = RoutingTable.init(localNode, 1, DefaultTableIpLimits, rng = rng)

check:
table.addNode(node1) == Added
table.addNode(node2) == Added

table.banNode(node1.id, 1.nanoseconds)
sleep(1) # node1's ban is expired
table.banNode(node2.id, 1.minutes)

check:
table.isBanned(node1.id) == false
table.isBanned(node2.id) == true

# Can ban nodes which were previously banned
table.banNode(node1.id, 1.minutes)
table.banNode(node2.id, 1.minutes)

check:
table.contains(node1) == false
table.isBanned(node1.id) == true
table.contains(node2) == false
table.isBanned(node2.id) == true
Loading