From 9387b5a8cc99595af60eee8191dc05dcec2fd435 Mon Sep 17 00:00:00 2001 From: whyrusleeping Date: Tue, 25 Jun 2019 17:34:25 +0200 Subject: [PATCH 1/6] don't add peers with only private addresses to your routing table --- dht.go | 45 ++++++++++++++++++++++++++++++++++++++++++++- ext_test.go | 5 +++++ go.mod | 1 + 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/dht.go b/dht.go index da3c5cc33..77eb65011 100644 --- a/dht.go +++ b/dht.go @@ -32,6 +32,7 @@ import ( kb "github.com/libp2p/go-libp2p-kbucket" record "github.com/libp2p/go-libp2p-record" recpb "github.com/libp2p/go-libp2p-record/pb" + manet "github.com/multiformats/go-multiaddr-net" base32 "github.com/whyrusleeping/base32" ) @@ -275,7 +276,49 @@ func (dht *IpfsDHT) putLocal(key string, rec *recpb.Record) error { // on the given peer. func (dht *IpfsDHT) Update(ctx context.Context, p peer.ID) { logger.Event(ctx, "updatePeer", p) - dht.routingTable.Update(p) + if dht.shouldAddPeerToRoutingTable(p) { + dht.routingTable.Update(p) + } +} + +func (dht *IpfsDHT) shouldAddPeerToRoutingTable(p peer.ID) bool { + return dht.hasOutboundConnToPeer(p) || + dht.hasSensibleAddressesForPeer(p) +} + +func (dht *IpfsDHT) hasOutboundConnToPeer(p peer.ID) bool { + cons := dht.host.Network().ConnsToPeer(p) + for _, c := range cons { + if c.Stat().Direction == network.DirOutbound { + return true + } + } + return false +} + +func (dht *IpfsDHT) hasSensibleAddressesForPeer(p peer.ID) bool { + if dht.peerIsOnSameSubnet(p) { + // TODO: for now, we can't easily tell if the peer on our subnet + // is dialable or not, so don't discriminate. + return true + } + + for _, a := range dht.host.Peerstore().Addrs(p) { + if manet.IsPublicAddr(a) { + return true + } + } + return false +} + +func (dht *IpfsDHT) peerIsOnSameSubnet(p peer.ID) bool { + cons := dht.host.Network().ConnsToPeer(p) + for _, c := range cons { + if manet.IsPrivateAddr(c.RemoteMultiaddr()) { + return true + } + } + return false } // FindLocal looks for a peer with a given ID connected to this dht and returns the peer and the table it was found in. diff --git a/ext_test.go b/ext_test.go index cb0ac7634..d7754bb98 100644 --- a/ext_test.go +++ b/ext_test.go @@ -33,6 +33,9 @@ func TestGetFailures(t *testing.T) { if err != nil { t.Fatal(err) } + + // TODO: replace with identify event bus event + time.Sleep(time.Millisecond * 100) d.Update(ctx, hosts[1].ID()) // Reply with failures to every message @@ -302,6 +305,8 @@ func TestMultipleQueries(t *testing.T) { t.Fatal(err) } + // TODO: need to wait for Identify Event from event bus + time.Sleep(time.Millisecond * 100) d.Update(ctx, hosts[1].ID()) // It would be nice to be able to just get a value and succeed but then diff --git a/go.mod b/go.mod index 1cbc5a07e..4a100eba7 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/mr-tron/base58 v1.1.2 github.com/multiformats/go-multiaddr v0.0.4 github.com/multiformats/go-multiaddr-dns v0.0.2 + github.com/multiformats/go-multiaddr-net v0.0.1 github.com/multiformats/go-multistream v0.1.0 github.com/stretchr/testify v1.3.0 github.com/whyrusleeping/base32 v0.0.0-20170828182744-c30ac30633cc From 03293e8d81a04ba1279cd05c97271cfb3f2fec9a Mon Sep 17 00:00:00 2001 From: whyrusleeping Date: Tue, 25 Jun 2019 23:46:41 +0200 Subject: [PATCH 2/6] add more filters --- dht.go | 84 +++++++++++++++++++++++++++++++++++++++++----------------- go.mod | 1 + go.sum | 1 + 3 files changed, 61 insertions(+), 25 deletions(-) diff --git a/dht.go b/dht.go index 77eb65011..13202c195 100644 --- a/dht.go +++ b/dht.go @@ -29,9 +29,11 @@ import ( logging "github.com/ipfs/go-log" goprocess "github.com/jbenet/goprocess" goprocessctx "github.com/jbenet/goprocess/context" + circuit "github.com/libp2p/go-libp2p-circuit" kb "github.com/libp2p/go-libp2p-kbucket" record "github.com/libp2p/go-libp2p-record" recpb "github.com/libp2p/go-libp2p-record/pb" + ma "github.com/multiformats/go-multiaddr" manet "github.com/multiformats/go-multiaddr-net" base32 "github.com/whyrusleeping/base32" ) @@ -275,50 +277,82 @@ func (dht *IpfsDHT) putLocal(key string, rec *recpb.Record) error { // Update signals the routingTable to Update its last-seen status // on the given peer. func (dht *IpfsDHT) Update(ctx context.Context, p peer.ID) { - logger.Event(ctx, "updatePeer", p) - if dht.shouldAddPeerToRoutingTable(p) { - dht.routingTable.Update(p) + conns := dht.host.Network().ConnsToPeer(p) + switch len(conns) { + default: + logger.Warning("unclear if we should add peer with multiple open connections to us to our routing table") + case 0: + logger.Error("called update on a peer with no connection") + case 1: + dht.UpdateConn(ctx, conns[0]) } } -func (dht *IpfsDHT) shouldAddPeerToRoutingTable(p peer.ID) bool { - return dht.hasOutboundConnToPeer(p) || - dht.hasSensibleAddressesForPeer(p) +func (dht *IpfsDHT) UpdateConn(ctx context.Context, c network.Conn) { + logger.Event(ctx, "updatePeer", c.RemotePeer()) + if dht.shouldAddPeerToRoutingTable(c) { + dht.routingTable.Update(c.RemotePeer()) + } } -func (dht *IpfsDHT) hasOutboundConnToPeer(p peer.ID) bool { - cons := dht.host.Network().ConnsToPeer(p) - for _, c := range cons { - if c.Stat().Direction == network.DirOutbound { - return true - } +func (dht *IpfsDHT) shouldAddPeerToRoutingTable(c network.Conn) bool { + if isRelayAddr(c.RemoteMultiaddr()) { + return false } - return false + return c.Stat().Direction == network.DirOutbound || + dht.hasSensibleAddressesForPeer(c) } -func (dht *IpfsDHT) hasSensibleAddressesForPeer(p peer.ID) bool { - if dht.peerIsOnSameSubnet(p) { +func (dht *IpfsDHT) hasSensibleAddressesForPeer(c network.Conn) bool { + addrs := dht.host.Peerstore().Addrs(c.RemotePeer()) + if len(addrs) == 0 { + return false + } + + var hasPublicAddr bool + for _, a := range dht.host.Peerstore().Addrs(c.RemotePeer()) { + if isRelayAddr(a) { + return false + } + + if manet.IsPublicAddr(a) { + hasPublicAddr = true + } + } + + if dht.peerIsOnSameSubnet(c) && !hasPublicAddr { // TODO: for now, we can't easily tell if the peer on our subnet // is dialable or not, so don't discriminate. + // ! It may be okay to not add these to our routing table... return true } - for _, a := range dht.host.Peerstore().Addrs(p) { - if manet.IsPublicAddr(a) { - return true - } + if !hasPublicAddr { + return false } + return false } -func (dht *IpfsDHT) peerIsOnSameSubnet(p peer.ID) bool { - cons := dht.host.Network().ConnsToPeer(p) - for _, c := range cons { - if manet.IsPrivateAddr(c.RemoteMultiaddr()) { +// taken from go-libp2p/p2p/host/relay +func isRelayAddr(a ma.Multiaddr) bool { + isRelay := false + + ma.ForEach(a, func(c ma.Component) bool { + switch c.Protocol().Code { + case circuit.P_CIRCUIT: + isRelay = true + return false + default: return true } - } - return false + }) + + return isRelay +} + +func (dht *IpfsDHT) peerIsOnSameSubnet(c network.Conn) bool { + return manet.IsPrivateAddr(c.RemoteMultiaddr()) } // FindLocal looks for a peer with a given ID connected to this dht and returns the peer and the table it was found in. diff --git a/go.mod b/go.mod index 4a100eba7..dec761f87 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/ipfs/go-todocounter v0.0.1 github.com/jbenet/goprocess v0.1.3 github.com/libp2p/go-libp2p v0.1.0 + github.com/libp2p/go-libp2p-circuit v0.1.0 github.com/libp2p/go-libp2p-core v0.0.1 github.com/libp2p/go-libp2p-kbucket v0.2.0 github.com/libp2p/go-libp2p-peerstore v0.1.0 diff --git a/go.sum b/go.sum index bd8387422..f1d2fc11f 100644 --- a/go.sum +++ b/go.sum @@ -111,6 +111,7 @@ github.com/libp2p/go-libp2p v0.1.0/go.mod h1:6D/2OBauqLUoqcADOJpn9WbKqvaM07tDw68 github.com/libp2p/go-libp2p-autonat v0.1.0/go.mod h1:1tLf2yXxiE/oKGtDwPYWTSYG3PtvYlJmg7NeVtPRqH8= github.com/libp2p/go-libp2p-blankhost v0.1.1 h1:X919sCh+KLqJcNRApj43xCSiQRYqOSI88Fdf55ngf78= github.com/libp2p/go-libp2p-blankhost v0.1.1/go.mod h1:pf2fvdLJPsC1FsVrNP3DUUvMzUts2dsLLBEpo1vW1ro= +github.com/libp2p/go-libp2p-circuit v0.1.0 h1:eniLL3Y9aq/sryfyV1IAHj5rlvuyj3b7iz8tSiZpdhY= github.com/libp2p/go-libp2p-circuit v0.1.0/go.mod h1:Ahq4cY3V9VJcHcn1SBXjr78AbFkZeIRmfunbA7pmFh8= github.com/libp2p/go-libp2p-core v0.0.1 h1:HSTZtFIq/W5Ue43Zw+uWZyy2Vl5WtF0zDjKN8/DT/1I= github.com/libp2p/go-libp2p-core v0.0.1/go.mod h1:g/VxnTZ/1ygHxH3dKok7Vno1VfpvGcGip57wjTU4fco= From c6cd6d5b3e2684d1303b9a74e2fe4bbf0408ca3f Mon Sep 17 00:00:00 2001 From: whyrusleeping Date: Tue, 25 Jun 2019 23:49:28 +0200 Subject: [PATCH 3/6] use less wasteful update method --- dht_net.go | 10 +++++----- notif.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/dht_net.go b/dht_net.go index 685b155be..ae9a23e0c 100644 --- a/dht_net.go +++ b/dht_net.go @@ -139,7 +139,7 @@ func (dht *IpfsDHT) handleNewMessage(s network.Stream) bool { return false } - dht.updateFromMessage(ctx, mPeer, &req) + dht.updateFromMessage(ctx, s.Conn(), &req) if resp == nil { continue @@ -179,7 +179,7 @@ func (dht *IpfsDHT) sendRequest(ctx context.Context, p peer.ID, pmes *pb.Message } // update the peer (on valid msgs only) - dht.updateFromMessage(ctx, p, rpmes) + dht.updateFromMessage(ctx, ms.s.Conn(), rpmes) stats.Record( ctx, @@ -218,11 +218,11 @@ func (dht *IpfsDHT) sendMessage(ctx context.Context, p peer.ID, pmes *pb.Message return nil } -func (dht *IpfsDHT) updateFromMessage(ctx context.Context, p peer.ID, mes *pb.Message) error { +func (dht *IpfsDHT) updateFromMessage(ctx context.Context, c network.Conn, mes *pb.Message) error { // Make sure that this node is actually a DHT server, not just a client. - protos, err := dht.peerstore.SupportsProtocols(p, dht.protocolStrs()...) + protos, err := dht.peerstore.SupportsProtocols(c.RemotePeer(), dht.protocolStrs()...) if err == nil && len(protos) > 0 { - dht.Update(ctx, p) + dht.UpdateConn(ctx, c) } return nil } diff --git a/notif.go b/notif.go index 3af758492..9d1d15c93 100644 --- a/notif.go +++ b/notif.go @@ -32,7 +32,7 @@ func (nn *netNotifiee) Connected(n network.Network, v network.Conn) { dht.plk.Lock() defer dht.plk.Unlock() if dht.host.Network().Connectedness(p) == network.Connected { - dht.Update(dht.Context(), p) + dht.UpdateConn(dht.Context(), v) } return } @@ -71,7 +71,7 @@ func (nn *netNotifiee) testConnection(v network.Conn) { dht.plk.Lock() defer dht.plk.Unlock() if dht.host.Network().Connectedness(p) == network.Connected { - dht.Update(dht.Context(), p) + dht.UpdateConn(dht.Context(), v) } } From 8bf0863a79aec7471fb518ab4978285f2468792c Mon Sep 17 00:00:00 2001 From: whyrusleeping Date: Wed, 26 Jun 2019 10:24:24 +0200 Subject: [PATCH 4/6] address PR review --- dht.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dht.go b/dht.go index 13202c195..4e879f02e 100644 --- a/dht.go +++ b/dht.go @@ -289,8 +289,8 @@ func (dht *IpfsDHT) Update(ctx context.Context, p peer.ID) { } func (dht *IpfsDHT) UpdateConn(ctx context.Context, c network.Conn) { - logger.Event(ctx, "updatePeer", c.RemotePeer()) if dht.shouldAddPeerToRoutingTable(c) { + logger.Event(ctx, "updatePeer", c.RemotePeer()) dht.routingTable.Update(c.RemotePeer()) } } @@ -310,7 +310,7 @@ func (dht *IpfsDHT) hasSensibleAddressesForPeer(c network.Conn) bool { } var hasPublicAddr bool - for _, a := range dht.host.Peerstore().Addrs(c.RemotePeer()) { + for _, a := range addrs { if isRelayAddr(a) { return false } @@ -331,7 +331,7 @@ func (dht *IpfsDHT) hasSensibleAddressesForPeer(c network.Conn) bool { return false } - return false + return true } // taken from go-libp2p/p2p/host/relay From f34a5b1104c40fb37ef024031fd49eb1e429192a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Kripalani?= Date: Wed, 26 Jun 2019 11:44:58 +0200 Subject: [PATCH 5/6] rejig for query filtering. --- dht.go | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/dht.go b/dht.go index 4e879f02e..cc99c4792 100644 --- a/dht.go +++ b/dht.go @@ -299,18 +299,29 @@ func (dht *IpfsDHT) shouldAddPeerToRoutingTable(c network.Conn) bool { if isRelayAddr(c.RemoteMultiaddr()) { return false } - return c.Stat().Direction == network.DirOutbound || - dht.hasSensibleAddressesForPeer(c) + if c.Stat().Direction == network.DirOutbound { + // we established this connection, so they're definitely diallable. + return true + } + if dht.peerIsOnSameSubnet(c) { + // TODO: for now, we can't easily tell if the peer on our subnet + // is dialable or not, so don't discriminate. + + // We won't return these peers in queries unless the requester's + // remote addr is also private. + return true + } + ai := dht.host.Peerstore().PeerInfo(c.RemotePeer()) + return dht.hasSensibleAddressesForPeer(ai) } -func (dht *IpfsDHT) hasSensibleAddressesForPeer(c network.Conn) bool { - addrs := dht.host.Peerstore().Addrs(c.RemotePeer()) - if len(addrs) == 0 { +func (dht *IpfsDHT) hasSensibleAddressesForPeer(ai peer.AddrInfo) bool { + if len(ai.Addrs) == 0 { return false } var hasPublicAddr bool - for _, a := range addrs { + for _, a := range ai.Addrs { if isRelayAddr(a) { return false } @@ -319,19 +330,7 @@ func (dht *IpfsDHT) hasSensibleAddressesForPeer(c network.Conn) bool { hasPublicAddr = true } } - - if dht.peerIsOnSameSubnet(c) && !hasPublicAddr { - // TODO: for now, we can't easily tell if the peer on our subnet - // is dialable or not, so don't discriminate. - // ! It may be okay to not add these to our routing table... - return true - } - - if !hasPublicAddr { - return false - } - - return true + return hasPublicAddr } // taken from go-libp2p/p2p/host/relay From 9bc949dca36e71a2b3458c4b79311fe6c754063a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Kripalani?= Date: Wed, 26 Jun 2019 12:19:54 +0200 Subject: [PATCH 6/6] rejig v2. --- dht.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/dht.go b/dht.go index cc99c4792..316ecb401 100644 --- a/dht.go +++ b/dht.go @@ -303,19 +303,21 @@ func (dht *IpfsDHT) shouldAddPeerToRoutingTable(c network.Conn) bool { // we established this connection, so they're definitely diallable. return true } + + ai := dht.host.Peerstore().PeerInfo(c.RemotePeer()) if dht.peerIsOnSameSubnet(c) { // TODO: for now, we can't easily tell if the peer on our subnet // is dialable or not, so don't discriminate. // We won't return these peers in queries unless the requester's // remote addr is also private. - return true + return len(ai.Addrs) > 0 } - ai := dht.host.Peerstore().PeerInfo(c.RemotePeer()) - return dht.hasSensibleAddressesForPeer(ai) + + return dht.isPubliclyRoutable(ai) } -func (dht *IpfsDHT) hasSensibleAddressesForPeer(ai peer.AddrInfo) bool { +func (dht *IpfsDHT) isPubliclyRoutable(ai peer.AddrInfo) bool { if len(ai.Addrs) == 0 { return false } @@ -325,7 +327,6 @@ func (dht *IpfsDHT) hasSensibleAddressesForPeer(ai peer.AddrInfo) bool { if isRelayAddr(a) { return false } - if manet.IsPublicAddr(a) { hasPublicAddr = true }