snarkOS icon indicating copy to clipboard operation
snarkOS copied to clipboard

[Feature] Introduce IP-level bans for connection spam targeting validators

Open ljedrz opened this issue 1 year ago • 4 comments

This PR primarily targets https://github.com/AleoNet/snarkOS/issues/3311, but my intention is for the new setup to be extensible based on potential future needs (e.g. detecting and banning for message spam).

The 1st commit alters the (currently mostly unused) low-level KnownPeers collection to work on IPs only (instead of IP+port pairs) and extends the related Stats object with a timestamp field which is updated whenever a (low-level) connection is established.

The new IP-level bans are introduced only at the (higher-level) handshake level for the following reasons:

  • the BFT setup can use and configure them separately from the Router setup
  • the Heartbeat can be used to clean up expired bans
  • the overall setup is simpler that way (e.g. the Tcp doesn't need to do any housekeeping)

The downside is that several concurrent low-level connection attempts may still be accepted even post-IP-ban. However, this would also (though to a lesser extent) be the case if the ban check was moved "to the lower level", as we would still have to accept an inbound connection before being able to check its IP. That being said, these are relatively lightweight and still subject to peer limits.

The ban-related consts were picked arbitrarily and we may choose any other values instead. Also, this new feature is disabled for tests and --dev runs (since they almost exclusively involve a single localhost IP).

CI run is here

ljedrz avatar Jul 18 '24 14:07 ljedrz

CI run link

ljedrz avatar Jul 18 '24 14:07 ljedrz

Thx for making this! @kpandl would you like to take the first pass review in the coming week?

vicsn avatar Jul 21 '24 11:07 vicsn

Generally looks good to me. Can you explain the setting of the const variables (MIN_CONNECTION_INTERVAL_IN_SECS and IP_BAN_TIME_IN_SECS) or do you know recommended values from other protocols? I understand they can be set arbitrarily, 30 could seem a bit low for IP_BAN_TIME_IN_SECS to me.

I've seen 24 hour bans in the Bitcoin protocol, but I've read other sources that suggest it could primarily limit DOS attacks over Tor.

Either way, setting the values in a config could also be an interesting feature in the long term.

kpandl avatar Aug 02 '24 19:08 kpandl

@kpandl the values were chosen semi-arbitrarily; basically any limit imposed on the number of connection attempts will suffice to avoid a "flood", but we may decide to be as harsh as we like.

Technically, MIN_CONNECTION_INTERVAL_IN_SECS should be equal to or (IMO preferably) smaller than the heartbeat interval (so we don't consider legitimate heartbeat re-connection attempts malicious), and IP_BAN_TIME_IN_SECS should be larger than the heartbeat interval, and it can be arbitrarily large (in order for it to be discouraging in addition to just limiting).

ljedrz avatar Aug 05 '24 08:08 ljedrz

Can be closed with - https://github.com/AleoNet/snarkOS/pull/3422.

raychu86 avatar Nov 15 '24 18:11 raychu86