identify: Update addr advertise logic to exclude localhost addrs selectively

[bigs]

No longer send localhost addrs in Identify requests unless the connection is via a loopback interface.

[Stebalien]

#436? This would break that proposal. May be worth doing it anyways, but we should discuss this a bit.

[raulk]

Hm. With the current single-layer DHT, we do kind of have to advertise private addresses to allow peers in the local network to dial us locally even if they found us through a public peer. We definitely need to contextualise advertisements better, but such a change would need to percolate to the DHT first as we need the capability to form multiple tiers like Coral proposes (e.g. via the unused cluster level param). Until then, I don’t think it’s safe to discard private addresses, even I hate divulging them because they leak details.

I believe the optimisation we wanted to get in was filtering out loopback addresses unless the peer dialled us via a loopback interface. For this I have a higher confidence it won’t break anything. Loopback is mostly used in special setups and testing.

[raulk]

See my comment above.

[raulk]

Realistically though, the multi-tier DHT is not an immediate priority, so an alternative is to just erase private addresses and let hairpinning in NATs do their job. @Stebalien?

[Stebalien]

Realistically though, the multi-tier DHT is not an immediate priority, so an alternative is to just erase private addresses and let hairpinning in NATs do their job.

Could be an issue if a node doesn't have a dialable public address. But really, mdns should solve this (mostly).

---

Have we measured how big of an issue this is in practice? Dialing localhost should generally fail fast.

[raulk]

It does fail fast, but it’s something I don’t like us doing. It’s also a security hole to allow a random peer to command you to dial a localhost address.

[bigs]

this has been updated to use the existing IsIPLoopback

[whyrusleeping]

I wonder if a milder version of this would help resolve libp2p/go-libp2p-kad-dht#357

[markg85]

Hi,

I ended up in here thanks to a pointer of @aschmahmann in ipfs/kubo#6742

Upon looking at the code i'm curious if IPv6 is being handled in there as well?
As i don't see it in the test cases and i definitely do see a fdxx:xxxx:xxxx:yyyy address, which is a local range for IPv6. For more information see https://en.wikipedia.org/wiki/Private_network. That wiki page doesn't say it, but addresses starting with fe80 are link local ones only too.

Also, as this fix landed in June, which go-ipfs version carries it? I'm on 0.4.22 that definitely still does show those local non-callable addresses.

[aschmahmann]

@markg85 I could be wrong, but it looks like this does work for Loopback (but not Link Local) ipv6 addresses. You're right, we should probably have an ipv6 test case though (want to file an issue?).

This PR is in go-libp2p and is about the identify protocol (where peers tell each other about themselves). We definitely still have reasons to tell other peers about our link local addresses. What if we're on the same LAN? What you're probably more interested in is the DHT related PRs that I linked to in that issue.

As you can see this change (and the other ones I linked) are not in master yet and therefore also not in a go-ipfs release. There's currently a lot of work going into setting up test infrastructure prior to the next release (happening at https://github.com/ipfs/testground) so we can be more confident that these changes will have a positive effect on the network. Hopefully these changes will show good results on the tests and make it into an ipfs release soon.

[markg85]

Hi @aschmahmann,

I'd like to be capable of making a PR for that! It would be my first IPFS contribution in actual code too! But... Both IPv6 and Go really hurt my brain. IPv6 because of it's extremely non-intuitive logic that i just don't get. I can do educated guesses there and actually get somewhere but that's about all. As for Go, it feels a bit alien to me, it "somewhat" resembles the C++ syntax i know and like but still is totally different that it hurts my head ;)

I'd happily leave this to someone capable in Go and IPv6.

Also, thank you for pointing out that this change isn't in master yet. I somehow blindly assumed that merged meant it was in master till i looked at where it was merged into... oops.

[Stebalien]

@markg85 fdda:... is a private address but not a loopback address. This issue is specifically about not announcing loopback addresses to the global DHT.