Check and ban peer when accepting incoming connection

[kaedei]

Description

The current implementation only checks if a peer should be banned during TryConnect. This PR adds a check using ShouldBanPeer() within the IncomingConnectionAcceptedAsync() method to ensure that peers are also evaluated for banning when an incoming connection is accepted.

Changes

• Added ShouldBanPeer() check in IncomingConnectionAcceptedAsync().

Motivation

Enhancing the robustness of the peer banning mechanism by ensuring that peers are evaluated for banning not only during connection attempts but also when incoming connections are accepted.

[alanmcgovern]

https://github.com/alanmcgovern/monotorrent/blob/779388bbca0e55c0ee9885220f51a4d601f5a153/src/MonoTorrent.Client/MonoTorrent.Client/Managers/ListenManager.cs#L96

The connection should be ban-checked as soon as it's received, before handshakes have been exchanged.

Are you seeing examples of where peers are establishing incoming connections even when they should have been banned? If so, could you analyse the earlier phases to see where the check is missing?

If it's not missing, maybe it's incorrectly implemented and isn't doing what it should be doing in your case?

[kaedei]

Current in the ListenManager.ConnectionReceived() event, we only have the PeerInfo.ConnectionUri property (IP address) to determine whether this peer should be banned.

var peerInfo = new PeerInfo (e.Connection.Uri);
try {
    if (Engine.ConnectionManager.ShouldBanPeer (peerInfo)) {

In my application, I want to implement a feature to filter clients based on PeerID (due to some malicious leeching BT clients), so I need to wait until the connection is truly established and data is exchanged to make the PeerID judgment (when PeerInfo.PeerId is not empty).

That's why I need another ban check in IncomingConnectionAcceptedAsync(). Hope my understanding is correct and my explanation is clear.

[alanmcgovern]

Ah that makes sense! I'd suggest two changes if you have the time:

Can this be moved 1 level further up the stack? I think there's a method inside ListenManager which receives the incoming handshake, then determines whether or not to send a response. If the Ban call was added there the connection could be dropped before it's fully initialized, but after the handshakes have been received.

Secondly, could you augment the BanPeerEventArgs (or whatever it's called) to have an 'enum' property which tells people which stage they're in so they know which properties should be available? That way you could deterministically know whether to expect just an IP address, or the full peer data.

If there are other options you think could work, I'm all ears! Perhaps passing just a Uri and Peerinfo could be better. The first time round you get a populated Uri and a null Peerinfo. The second time round you get a null Uri and a non-null PeerInfo as the handshake will have been received.

[alanmcgovern]

That should do the trick! It's probably easier to add a 'stage' enum than it is to change the nullability of the 'PeerInfo' object, so i took that approach!

[kaedei]

Thank you for your modification! I have been quite busy these past few days and haven't had the chance to modify the code. As you mentioned earlier, I am considering whether it would be better to move the ShouldBanPeer check from ConnectionManager.IncomingConnectionAcceptedAsync() to ListenManager.HandleHandshake().

For example:

var infoHash = message.SupportsUpgradeToV2 && man.InfoHashes.IsHybrid ? man.InfoHashes.V2! : man.InfoHashes.Expand (message.InfoHash);
var peer = new Peer (peerInfo);
peer.UpdatePeerId (message.PeerId);

logger.InfoFormatted (connection, "[incoming] Received handshake with peer_id '{0}'", message.PeerId);

// This is the new ban check before HandleMessage, PeerId is updated above:
if (!Engine.ConnectionManager.ShouldBanPeer (peer.Info, AttemptConnectionStage.BeforeConnectionEstablished)) {
    return false;
}

var id = new PeerId (peer, connection, new BitField (man.Bitfield.Length).SetAll (false), infoHash) {
    Decryptor = decryptor,
    Encryptor = encryptor,
    ClientApp = new Software (message.PeerId),
};

man.Mode.HandleMessage (id, message, default);
logger.Info (id.Connection, "Handshake successfully handled");

return await Engine.ConnectionManager.IncomingConnectionAcceptedAsync (man, id);

This way, a ban check can be performed before handle the message (man.Mode.HandleMessage()), is this necessary?

[alanmcgovern]

:nod:

The thing I'm currently trying to accomplish is that the PeerId object is only instantiated when we're certain the connection will be fully established, and therefore the peer can only be added to the TorrentManager.Peers.ConnectedPeers list etc after we're guaranteed the connection is operational.

This means no PeerId object should be created before the banchecks have been executed too. As i went through the code, the refactor was a little more in-depth so I decided to move ahead with your PR as-is, and then a few cleanup PRs. This refactor may also resolve https://github.com/alanmcgovern/monotorrent/issues/695 as there's an unexpected NullReferenceException which i'm unsure of the cause of. Perhaps under an older monotorrent library the fact it added peers to the 'Connected' list a little early contributed to that particular issue? We'll see ;)