swarm/network: Use actual remote peer ip in underlay

[nolash]

Resolves #1219 (see description below) by extracting the remote address of the p2p.Peer and overwriting the BzzAddr.UAddr if they differ.

This is a temporary fix awaiting ENR refactor.

---

Example; Nodes A, B and C are started without --nat flag. Their enode strings used in the swarm handshake will all have localhost.

You find a subnet ip for A that's reachable from B and C. You replace the localhost in the enode string with this ip. Then you execute RPC call admin.addPeer with that enode string to B and C.

Result: B and C will have connected to A, but not have connected to each other.

It seems that in the discovery protocol, B and C still "convince" A that they exist on localhost, and when A suggests B to C, it will be with localhost as the host part.

Instead, it would be more natural that A stores the ip from the interface the request can in on, and propagates this as the host part.

Note; the same problem applies if you do the admin.addPeer on A to B and C.

[skylenet]

Currently we can use the --nat flag to announce any kind of IP:port... what happens if I want to attack a specific service on the internet, could I in theory just announce the IP:port of that service and have swarm nodes trying to connect to it?

It that's the case, would this fix also solve that?

I see this would bring the advantage of:

• Nobody else can announce some wrong IP address because we're always using the IP of the incoming connection.

But would also bring the problem:

• What if we do indeed want to announce another IP address? I'm talking about setups that have different IP addresses for incoming and outgoing traffic. Could be someone running a reverse-proxy in front of their node and wanting that the traffic comes through that. Dunno if we want to support such cases, but currently it seems possible. With this change, this would probably get broken. Which me might not want to support at all.. since we're talking about P2P networks.

[nolash]

@skylenet

It that's the case, would this fix also solve that?

I don't think it would be much of an attack. Since that service wouldn't speak devp2p I assume the node would be just marked as useless. It might perhaps confuse the node when it tries to reconnect after the initial incoming connection, I don't know. Anyway, that belongs to the p2p layer.

That said: yes, this fix indeed improve the previous situation, as it won't allow arbitrary host information in the swarm handshake.

But would also bring the problem:

For the second part of your question, I would say that this is out of scope for this PR. What you are discussing seems to concern the p2p layer of the code. At the earliest, I wouldn't make any such considerations until we start using the new ENR infrastructure.

[nolash]

And actually, the description in the PR is a bit misleading. Their enode strings will not be localhost on the p2p level, only in the swarm handshake. (I've edited it now)

[zelig]

why return pointer then?

[zelig]

https://travis-ci.org/ethersphere/go-ethereum/jobs/494428028#L512

[acud]

LGTM, just please check len(hsSubmatch)

[nolash]

Submitted upstream ethereum/go-ethereum#19137

[nonsense]

I think we should revert this PR. We are already overwriting good remote IPs, for example:

DEBUG[02-25|14:07:21.894|swarm/network/protocol.go:235]        rewrote peer uaddr host/port             addr="fe3a0995fdc725a5a198220d44b51d8fa82961cf97ac1c43f63ee6ae73b2896a u003cenode://24abebe1c0e6d75d6052ce3219a87be8573fd6397b4cb51f0773b83abba9b3d872bfb273cdc07389715b87adfac02f5235f5241442c5089802cbd8d42e310fce@52.232.7.187:30406?discport=0u003e"

If a node announces itself with a given IP:PORT, by protocol definition this is what we should try to connect to, and not infer its remote IP:PORT.

If a node announces itself with localhost, then this is a misconfiguration on its part, and not a problem of its peers.

[nonsense]

cc @nolash @zelig @justelad