Configurable Peer Filtering #471

aschmahmann · 2020-03-03T08:01:36Z

Required for libp2p/go-libp2p#803. Allows peers to be filtered out of queries and routing tables, comes with a couple suggested functions for public and private network peer filtering.

Bumps [github.com/ipfs/go-datastore](https://github.com/ipfs/go-datastore) from 0.4.2 to 0.4.4. - [Release notes](https://github.com/ipfs/go-datastore/releases) - [Commits](ipfs/go-datastore@v0.4.2...v0.4.4) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

aschmahmann

This is a start and I'd really appreciate feedback. The two main things are:

Are the filters sufficiently aggressive, overly aggressive, or not aggressive enough?
Does the API seem reasonable?

dht.go

query.go

opts/filters.go

aschmahmann · 2020-03-03T08:21:41Z

opts/filters.go

+	conns := h.Network().ConnsToPeer(ai.ID)
+	if len(conns) > 0 {
+		for _, c := range conns {
+			if manet.IsPrivateAddr(c.RemoteMultiaddr()) {
+				return true
+			}
+		}
+		return false
+	}


This may be overkill in the sense that if we're already connected to the peer and we discovered them via a private DHT query then we may as well query them if we're already connected.

The approach here was to be highly defensive of our queries being polluted with non-private peers in order to keep the private DHT small and therefore make the cost of running 2 DHTs small.

Maybe we should remove this check?

I'd remove this check. We may learn about a few undailable peers, but we won't waste too much time dialing them.

see https://github.com/libp2p/go-libp2p-kad-dht/pull/471/files#r387477337

Note: I would short-circuit and, if we have a private connection, we should assume that the peer is local. I just wouldn't do the inverse "we're connected over a public IP" check.

aschmahmann · 2020-03-03T08:26:57Z

opts/filters.go

+	for _, a := range ai.Addrs {
+		if manet.IsPublicAddr(a) {
+			if !isRelayAddr(a) {
+				return false


This check is perhaps overly aggressive because it's possible that a peer has both a private address and a public one. Perhaps for queries it's not worth doing this check and only being critical of routing table additions.

What's the idea here? Why would we want to filter out peers that have public addresses? IIRC, peers on the public network should also be in the private DHT.

The concern here isn't so much about undialable nodes, but about accidentally bridging the networks (e.g. starting a local DHT request and ending up with a global DHT request). However, I suspect that it's not really a big deal since if you get the data then you have it and this bridging shouldn't happen with behaving nodes.

We shouldn't filter out peers with public addresses. All peers on the local network, dialable or not, should be part of the local DHT.

Notes:

We should have some way to pass an address filter function down into the dialer in the future. That's probably the right way to handle this.

We should apply all the checks from https://github.com/libp2p/go-libp2p-kad-dht/pull/471/files#r387453724. E.g., we should filter out peers when (a) we know our public IPv4 address and (b) we see that it's not the same as theirs.

aschmahmann · 2020-03-03T08:29:42Z

opts/options.go

+// QueryFilter sets a function that approves which peers may be dialed in a query
+func QueryFilter(filter func(h host.Host, ai peer.AddrInfo) bool) Option {
+	return func(o *Options) error {
+		o.QueryPeerFilter = filter
+		return nil
+	}
+}
+
+// RoutingTableFilter sets a function that approves which peers may be added to the routing table. The host should
+// already have at least one connection to the peer under consideration.
+func RoutingTableFilter(filter func(conns []core.Conn) bool) Option {


Would happily take better names for these. Is it obvious from the name the when true is returned that we keep that peer?

aschmahmann · 2020-03-03T08:30:09Z

opts/options.go

+// QueryFilter sets a function that approves which peers may be dialed in a query
+func QueryFilter(filter func(h host.Host, ai peer.AddrInfo) bool) Option {
+	return func(o *Options) error {
+		o.QueryPeerFilter = filter
+		return nil
+	}
+}
+
+// RoutingTableFilter sets a function that approves which peers may be added to the routing table. The host should
+// already have at least one connection to the peer under consideration.
+func RoutingTableFilter(filter func(conns []core.Conn) bool) Option {


Part of me wants to include a host.Host parameter in this function just in case we need it in the future. However, given that most users will probably just use our pre-built function breaking the API in the future may not be that big a deal.

aschmahmann · 2020-03-03T08:35:28Z

query.go

-	for _, np := range newPeers {
-		closer := q.localPeers.Add(np.ID)
-		foundCloserPeer = foundCloserPeer || closer
+		if q.dht.queryPeerFilter(q.dht.host, *next) {


How necessary is the query filter really, it seems like it's kind of "plan B" since hopefully the routing tables should be clean and only contain relevant peers. Is it ok for functions to overfilter peers here?

Do we also want to filter which addresses we add into the peerstore and/or which ones we eventually dial? If so is it important to do this now?

How necessary is the query filter really, it seems like it's kind of "plan B" since hopefully the routing tables should be clean and only contain relevant peers. Is it ok for functions to overfilter peers here?

For the public DHT, I'd introduce two checks:

I can dial the peer.

They aren't advertising relay addresses. That's a bit over-zealous, but we don't want to accidentally dial it.

For the private DHT, I'm not sure if we want to filter at all.

Do we also want to filter which addresses we add into the peerstore and/or which ones we eventually dial? If so is it important to do this now?

I'd actually keep all addresses. If the peer is on the local network, we might as well use those addresses.

I can dial the peer

What does this mean? If we don't check at all we will end up eventually dialing the peer (if they are useful). To me this sounds like the only query filter we want is for the public DHT to check for relay addresses, is that correct?

I can dial the peer

Sorry, I meant to say "the peer has a public address". Really, just checking for "no relay address" should be sufficient.

opts/filters.go

aschmahmann · 2020-03-03T08:48:35Z

opts/filters.go

+		addr := c.RemoteMultiaddr()
+		if !isRelayAddr(addr) && manet.IsPublicAddr(addr) {
+			return true


This may be a bit more aggressive than necessary since AFAIK if two peers are publicly dialable via port forwarding and are also on the same local network they likely will be connected to each other over private multiaddrs instead of public ones. This means we are potentially missing peers from our routing tables that could be there.

In the future we could add more expensive tests like pinging any non-relay public addresses we have for the peer in the peerstore (requires that the function have access to the peerstore), to see if the peer we're locally connected to is also publicly dialable.

If desired there could also be a simple check to see if (as far as we know) the peer and us share the same public network address (e.g. are behind the same NAT).

Can we just check "do we have a public addr for this peer in the peerstore"?

We can, but there's no guarantee that it means anything right? This is the backup for what happens if an undialable peer doesn't remove themselves from being a DHT server.

@aschmahmann

Would I be correct in saying that the problem we want to solve is :

"Even for a public DHT, we should add a peer to the RT if that peer is on the same network as ours, even though we might not see a public address for it" ?

Would such a peer get added to our private DHT then ?

@aarshkshah1992 The problem we're trying to solve in this function (PublicRoutingTableFilter) is:

How can we help ourselves ensure (at minimal cost) that even if peers are not properly removing themselves from the public DHT (i.e. switching to client mode if they are undialable) that we do not add non-publicly dialable peers to our routing table?

opts/filters.go

dht.go

opts/filters.go

Stebalien · 2020-03-04T05:20:16Z

opts/filters.go

+			return true
+		}
+	}
+	return false


We should check a few more things:

If the remote address is in the carrier grade NAT range, abort.

IPv4: If the remote address is our public IPv4 address, accept them (NAT hairpin).

IPv6: If the peer is advertising the a public address that matches ours, accept them (could be abused?).

IPv6: If the peer is advertising an address that looks like an EUI-64 Bit address, check to see if the first 64 bits are the same as ours.

Note: on linux, we can check the route if the remote address is IPv6:

import "github.com/vishvananda/netlink" ... routes, err := netlink.RouteGet(myIP) for _, route := range routes { if route.Gw == nil { // We have a route to this host that doesn't go through a gateway. They must be on the same lan. } }

But we can file this away in the "to be improved" bucket.

But we can file this away in the "to be improved" bucket.

SGTM

@willscott is looking into this.

Stebalien · 2020-03-04T06:48:55Z

opts/filters.go

+		addr := c.RemoteMultiaddr()
+		if !isRelayAddr(addr) && manet.IsPublicAddr(addr) {
+			return true


Can we just check "do we have a public addr for this peer in the peerstore"?

Stebalien · 2020-03-04T06:50:43Z

opts/filters.go

+	conns := h.Network().ConnsToPeer(ai.ID)
+	if len(conns) > 0 {
+		for _, c := range conns {
+			if manet.IsPrivateAddr(c.RemoteMultiaddr()) {
+				return true
+			}
+		}
+		return false
+	}


I'd remove this check. We may learn about a few undailable peers, but we won't waste too much time dialing them.

Stebalien · 2020-03-04T06:53:24Z

opts/filters.go

+	for _, a := range ai.Addrs {
+		if manet.IsPublicAddr(a) {
+			if !isRelayAddr(a) {
+				return false


What's the idea here? Why would we want to filter out peers that have public addresses? IIRC, peers on the public network should also be in the private DHT.

opts/filters.go

Stebalien · 2020-03-04T06:59:03Z

query.go

-	for _, np := range newPeers {
-		closer := q.localPeers.Add(np.ID)
-		foundCloserPeer = foundCloserPeer || closer
+		if q.dht.queryPeerFilter(q.dht.host, *next) {


How necessary is the query filter really, it seems like it's kind of "plan B" since hopefully the routing tables should be clean and only contain relevant peers. Is it ok for functions to overfilter peers here?

For the public DHT, I'd introduce two checks:

I can dial the peer.

They aren't advertising relay addresses. That's a bit over-zealous, but we don't want to accidentally dial it.

For the private DHT, I'm not sure if we want to filter at all.

Do we also want to filter which addresses we add into the peerstore and/or which ones we eventually dial? If so is it important to do this now?

I'd actually keep all addresses. If the peer is on the local network, we might as well use those addresses.

query.go

aschmahmann

Thanks, it sounds like the requested changes are:

Remove almost all of the query filtering logic from PublicQueryFilter and actually all of the logic from PrivateQueryFilter
Cover more edge cases in IPv4 and IPv6 PrivateRoutingTableFilter
Docs + Cleanup

I left a few clarifying questions

aschmahmann · 2020-03-04T17:43:20Z

opts/filters.go

+		addr := c.RemoteMultiaddr()
+		if !isRelayAddr(addr) && manet.IsPublicAddr(addr) {
+			return true


We can, but there's no guarantee that it means anything right? This is the backup for what happens if an undialable peer doesn't remove themselves from being a DHT server.

aschmahmann · 2020-03-04T17:47:11Z

opts/filters.go

+	conns := h.Network().ConnsToPeer(ai.ID)
+	if len(conns) > 0 {
+		for _, c := range conns {
+			if manet.IsPrivateAddr(c.RemoteMultiaddr()) {
+				return true
+			}
+		}
+		return false
+	}


see https://github.com/libp2p/go-libp2p-kad-dht/pull/471/files#r387477337

aschmahmann · 2020-03-04T17:47:16Z

opts/filters.go

+	for _, a := range ai.Addrs {
+		if manet.IsPublicAddr(a) {
+			if !isRelayAddr(a) {
+				return false


The concern here isn't so much about undialable nodes, but about accidentally bridging the networks (e.g. starting a local DHT request and ending up with a global DHT request). However, I suspect that it's not really a big deal since if you get the data then you have it and this bridging shouldn't happen with behaving nodes.

aschmahmann · 2020-03-04T17:49:26Z

opts/filters.go

+			return true
+		}
+	}
+	return false


But we can file this away in the "to be improved" bucket.

SGTM

aschmahmann · 2020-03-04T17:59:34Z

query.go

-	for _, np := range newPeers {
-		closer := q.localPeers.Add(np.ID)
-		foundCloserPeer = foundCloserPeer || closer
+		if q.dht.queryPeerFilter(q.dht.host, *next) {


I can dial the peer

What does this mean? If we don't check at all we will end up eventually dialing the peer (if they are useful). To me this sounds like the only query filter we want is for the public DHT to check for relay addresses, is that correct?

aarshkshah1992

Some comments to improve my own understanding of what we are trying to do.

aarshkshah1992 · 2020-03-04T18:39:06Z

opts/filters.go

+)
+
+// PublicQueryFilter returns true if the peer is suspected of being publicly accessible
+func PublicQueryFilter(h host.Host, ai peer.AddrInfo) bool {


@aschmahmann

Why not read the addresses from the peer store ?

opts/filters.go

…ipfs/go-datastore-0.4.4 build(deps): bump github.com/ipfs/go-datastore from 0.4.2 to 0.4.4

…igin/feat/peer-filtering

willscott · 2020-03-10T22:33:40Z

dht.go

@@ -259,6 +264,9 @@ func makeRoutingTable(h host.Host, cfg config) (*kb.RoutingTable, error) {
 			rtPvLogger.Errorf("failed to connect to peer %s for validation, err=%s", p, err)
 			return false
 		}
+		if !cfg.routingTable.peerFilter(h, h.Network().ConnsToPeer(p)) {


@aschmahmann : is using the RT peerFilter within the peerValidationFunction of the routing table the right place to put it after the kbucket refactor?

Stebalien · 2020-03-19T06:43:23Z

dht_filters.go

+}
+
+// PrivateQueryFilter returns true if the peer is suspected of being accessible over a shared private network
+func PrivateQueryFilter(h host.Host, ai peer.AddrInfo) bool {


See #471 (comment)

Stebalien · 2020-03-19T06:44:37Z

dht_filters.go

+				return true
+			}
+		}
+		return false


This is a bit too aggressive. If they share a public address with us, we're using NAT hairpin.

Stebalien · 2020-03-19T06:45:16Z

dht_filters.go

+		} else {
+			hasPrivateAddr = true
+		}
+	}


https://github.com/libp2p/go-libp2p-kad-dht/pull/471/files/f4e42b882ca52932cc9783d1f3c0e9519735d332#diff-a38407e48af581fd58c4683c33e6dd42

Stebalien · 2020-03-19T06:47:01Z

dht_filters.go

+
+// PrivateRoutingTableFilter allows a peer to be added to the routing table if the connections to that peer indicate
+// that it is on a private network
+func PrivateRoutingTableFilter(h host.Host, conns []network.Conn) bool {


Ah, wait, why aren't we using all this logic in the PrivateQueryFilter? The query filter should generally be less restrictive than the routing table filter.

Stebalien · 2020-03-19T06:51:00Z

@willscott take a look at some of the unresolved discussions including:

(you'll probably have to go through the unresolved discussions one by one)

…iltering

dht_filters.go

Stebalien

This looks pretty much ready.

dht_filters.go

go.mod

Stebalien

Let's work through the filtering logic explaining what we're doing and why (in comments). That will help us figure out which choices appear to be arbitrary and/or inconsistent.

dht.go

Stebalien · 2020-03-31T21:42:31Z

dht_filters.go

+
+	for _, c := range conns {
+		ra := c.RemoteMultiaddr()
+		if manet.IsPrivateAddr(ra) || isRelayAddr(ra) {


This will add all relayed peers to the private routing table.

dht_filters.go

Stebalien · 2020-03-31T21:52:12Z

dht_filters.go

+		if !isRelayAddr(addr) && manet.IsPublicAddr(addr) {
+			return true
+		}
+	}


On further consideration, drop this entire check. We only want peers that announce public addresses. Direct connections form undialable peers will have public addresses.

The check below should (I think?) look at all the addresses advertised by the peer (assuming identify has finished).

dht_filters.go

…at/peer-filtering

Allows specifying peer filter functions on query and on adding peers to the routing table. This patch also includes some reasonable default functions for a public-only and private-only DHT. Co-authored-by: Will Scott <will@cypherpunk.email> Co-authored-by: Steven Allen <steven@stebalien.com>

aschmahmann commented Mar 3, 2020

View reviewed changes

aschmahmann force-pushed the feat/peer-filtering branch from f4e42b8 to c276983 Compare March 3, 2020 08:55

aschmahmann requested a review from Stebalien March 3, 2020 09:03

aschmahmann self-assigned this Mar 3, 2020

willscott reviewed Mar 3, 2020

View reviewed changes

opts/filters.go Outdated Show resolved Hide resolved

dht.go Outdated Show resolved Hide resolved

aschmahmann mentioned this pull request Mar 3, 2020

Proposed DHTv2 Changes #473

Merged

Stebalien force-pushed the cypress branch from 3e405f7 to bf5f87a Compare March 4, 2020 03:50

Stebalien force-pushed the feat/peer-filtering branch from c276983 to 537e8f9 Compare March 4, 2020 03:51

Stebalien reviewed Mar 4, 2020

View reviewed changes

aschmahmann commented Mar 4, 2020

View reviewed changes

aarshkshah1992 reviewed Mar 4, 2020

View reviewed changes

Stebalien force-pushed the cypress branch 2 times, most recently from 22b3963 to 5d313b1 Compare March 5, 2020 06:26

Stebalien force-pushed the feat/peer-filtering branch from 537e8f9 to eeaa1d7 Compare March 5, 2020 06:29

aschmahmann force-pushed the feat/peer-filtering branch from 5cc259c to afc98b9 Compare March 6, 2020 05:15

aschmahmann assigned willscott and unassigned aschmahmann Mar 6, 2020

aschmahmann added 2 commits March 6, 2020 15:11

add configurable peer filtering

c6155be

simplify relay address check

67b9cef

aschmahmann force-pushed the feat/peer-filtering branch from afc98b9 to 67b9cef Compare March 6, 2020 20:16

add types for FilterFunc's

a1ad06b

Stebalien added this to the Working Kademlia milestone Mar 10, 2020

Stebalien and others added 4 commits March 9, 2020 22:13

Merge pull request #456 from libp2p/dependabot/go_modules/github.com/…

6e748c2

…ipfs/go-datastore-0.4.4 build(deps): bump github.com/ipfs/go-datastore from 0.4.2 to 0.4.4

Merge branch 'master' into origin/feat/peer-filtering

c775021

reflect the anticipated PrivateRoutingTableFilter checks

1cf80dd

Merge branch 'cypress' of github.com:libp2p/go-libp2p-kad-dht into or…

50815a2

…igin/feat/peer-filtering

willscott reviewed Mar 10, 2020

View reviewed changes

willscott added 2 commits March 10, 2020 16:13

v6 eui filter

a9421e1

peerFilter applied at PeerFound

922495b

willscott marked this pull request as ready for review March 16, 2020 17:03

Stebalien reviewed Mar 19, 2020

View reviewed changes

willscott added 4 commits March 19, 2020 15:22

address outstanding discussion

c032ad8

Merge remote-tracking branch 'origin/cypress' into origin/feat/peer-f…

899af0f

…iltering

unrestricted private queries

6a5bf3b

Merge remote-tracking branch 'origin/cypress' into feat/peer-filtering

f01285e