Xud: Prevent a node from being connected to itself

[moshababo]

Make sure a node won’t get connected to itself, nor saving itself in the host list. It’s a likely scenario when querying other peers for their list of peers (#147), but it can also happen by mistake, when initiating a connection manually.
We might just want to check if before attempting to add a new outbound connection, and not in a new inbound connection, because the same node will control these two sides.

181 is a prerequisite.

[sangaman]

To be clear, do we only want to make sure we are not connecting to our own external IP address and our listen port? Alternatively, we could prevent any connections to ourselves via external IP, and still allow connections to our own machine via 127.0.0.1. Also, should we put in logic to prevent connections to ourselves via 127.0.0.1 and our listen port?

[michael1011]

Another approach would be not allowing connections to nodes with the same public key as your own node. But this should be used as last resort and for edge cases because it requires the handshake to happen.

I propose blocking connections to your external IP and P2P port and blocking the same public key for everything else.

[kilrau]

• Don't try to connect: own external IP address with own listen port.
• Try to connect and check if nodePubKey is different from mine: all other IP address and port combinations, which includes own external IP address with a different port.

Reason for this is simple: two xud's might just be sitting behind the same IP of the same VPN provider.

[sangaman]

We can put in a check for public key, but as @michael1011 says it's an edge case and can only happen after handshake. I think it's best to catch this before the handshake begins. But it sounds like there's agreement on only checking to stop connections to our own IP and listen port, otherwise allow the connection. Like @kilrau says it's possible for two nodes to have the same external IP even when running on separate machines.

[moshababo]

Agree, we need to apply this to both internal/external IP address + listenPort.

We can add nodePubKey check as well, in case we missed it. Although last time I checked, trying to connect to yourself via external IP leads to immediate disconnection.

[kilrau]

Summary:

• filter on connection attempt if internal/external IP address + listenPort is myIP+Port combination (as per https://github.com/ExchangeUnion/xud/issues/181), if yes don't connect.
• in any case, after handshake always check additionally that nodePubKey =/ ownNodePubKey.
• EDIT: we don't allow adding any localhost/127.0.0.1/::1 to the hosts table. During testing this got messy real fast as every host is sharing it's localhost with me... . We'll allow ownExternalIP into the host table, but filter on connection attempt. Reason: my ownExternalIP+Port can change

[kilrau]

And just FYI: we already have the case where 2 xuds are behind the same domain (0.tcp.ngrok.io) but get identified by different ports ;)

[offerm]

Guys, the PubKey is available now as part of the handshake. Why don't we kill this by checking it?

Can easily be done as part of handleHello

What do you think? @sangaman @michael1011 @moshababo @kilrau

[michael1011]

@offerm What I am thinking right now is:

1. check if the IP and port you want to connect to are your own ones: #181. If that is the case don't even try to connect
2. if the public key of the peer is the same as your own disconnect immediately because you are trying to connect to yourself

[offerm]

point 2 is a must.
point 1 is nice to have.

The value that we gain from 2 is huge regardless of 1.

IMHO, it should be done asap.

Offer

[kilrau]

Yes, let's do both, 1+2 https://github.com/ExchangeUnion/xud/issues/177#issuecomment-411369865 @michael1011 @offerm

[kilrau]

Who did put the pubKey into the handshake now btw? Kudos for that, we should have done this way earlier.

[sangaman]

That was me in #243.