Clean addresses with peer id before adding to addrbook #2007

sukunrt · 2023-01-23T09:01:03Z

Fixes: #2001

Fixes: libp2p#2001

sukunrt · 2023-01-23T09:01:50Z

I'm cleaning the address before adding it to addressbook. The other option is to change the check in all transports. This seemed cleaner to me.

MarcoPolo

Thanks for picking this up! This is really helpful 🎉. There's a couple of things we should do before merging this:

The changes mentioned in the review.
We should update the implementations of Host.Connect so that they check that the multiaddrs with a p2p component in the Addrs field match the ID field. Filter out any mismatching multiaddrs (maybe log a warning). And return an error if no addresses are dialable because of mismatched peer ids. As well as removing the p2p component of the multiaddr before passing it down the call stack.

MarcoPolo · 2023-01-23T19:23:07Z

p2p/host/peerstore/test/addr_book_suite.go

+			p2pAddr := addr[0].Encapsulate(Multiaddr("/p2p/" + peerId.String()))
+			ab.AddAddr(peerId, p2pAddr, time.Hour)
+			AssertAddressesEqual(t, addr, ab.Addrs(peerId))
+		})


We should have a case that fails if the peer id in the multiaddr is != to the peer id passed in.

The annoying thing is that AddAddr doesn't return an error. So the best we can do here is to fail silently and log an error in the implementations.

Added another test case with filtering on invalid peerId

MarcoPolo · 2023-01-23T19:32:52Z

p2p/host/peerstore/pstoreds/addr_book.go

@@ -601,10 +601,12 @@ func (ab *dsAddrBook) deleteAddrs(p peer.ID, addrs []ma.Multiaddr) (err error) {
 func cleanAddrs(addrs []ma.Multiaddr) []ma.Multiaddr {


This should now take a peer id as well, and filter out multiaddrs that have an id component but don't match (and log an error when this happens)

I'm logging a warning which was previously logged for nil addresses, should I make it an error?

MarcoPolo · 2023-01-23T19:33:45Z

p2p/host/peerstore/pstoremem/addr_book.go

@@ -238,19 +238,21 @@ func (mab *memoryAddrBook) addAddrsUnlocked(s *addrSegment, p peer.ID, addrs []m

 	exp := mab.clock.Now().Add(ttl)
 	for _, addr := range addrs {
-		if addr == nil {
+		// Remove suffix of /p2p/peer-id from address
+		taddr, _ := peer.SplitAddr(addr)


same comment as above about the peer id check. Also I think it's okay to shadow the addr variable with the new multiaddr.

This should be fixed

MarcoPolo · 2023-01-23T19:34:07Z

p2p/host/peerstore/pstoremem/addr_book.go

@@ -283,17 +285,18 @@ func (mab *memoryAddrBook) SetAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Du

 	exp := mab.clock.Now().Add(ttl)
 	for _, addr := range addrs {
-		if addr == nil {
+		taddr, _ := peer.SplitAddr(addr)


same comment as above.

sukunrt · 2023-01-24T04:18:49Z

We should update the implementations of Host.Connect so that they check that the multiaddrs with a p2p component in the Addrs field match the ID field. Filter out any mismatching multiaddrs (maybe log a warning). And return an error if no addresses are dialable because of mismatched peer ids. As well as removing the p2p component of the multiaddr before passing it down the call stack.

All Host.Connect implementations use PeerStore.AddAddrs which will filter out for invalid ID and log the warning, should I do it in the connect implementation as well?
I was thinking of doing this:

func (h *BasicHost) Connect(ctx context.Context, pi peer.AddrInfo) error {
	// absorb addresses into peerstore
	h.Peerstore().AddAddrs(pi.ID, pi.Addrs, peerstore.TempAddrTTL)
    addrs := h.Peerstore().Addrs(pi.ID)
	if len(addrs) == 0 {
		return fmt.Errorf("no valid address found for peer %s", pi.ID)
	}

Add error reporting from Host.Connect

sukunrt · 2023-01-24T04:47:32Z

@MarcoPolo Addressed review comments. Do the changes in Host.Connect look okay?

MarcoPolo · 2023-01-25T21:36:50Z

All Host.Connect implementations use PeerStore.AddAddrs which will filter out for invalid ID and log the warning,

Good point. If there are no available addresses we'll still get that error from Connect. I think that's okay.

MarcoPolo

Some small nits. I pushed a change. Thanks a ton for fixing this :)

MarcoPolo · 2023-01-25T21:41:01Z

p2p/host/basic/basic_host.go

@@ -709,6 +709,11 @@ func (h *BasicHost) Connect(ctx context.Context, pi peer.AddrInfo) error {
 		}
 	}

+	addrs := h.Peerstore().Addrs(pi.ID)
+	if len(addrs) == 0 {


On second thought we already return a similar error here https://github.com/libp2p/go-libp2p/blob/master/p2p/net/swarm/swarm_dial.go#L340. So this change is not required. Sorry I added confusion here.

that's fair. Thanks for fixing this!

MarcoPolo · 2023-01-25T21:47:02Z

Will merge after CI passes.

masih · 2023-01-26T10:23:37Z

Thanking you folks for getting this issue resolved so quickly 🙇

Clean addresses with peer id before adding to addrbook

1929d4e

Fixes: libp2p#2001

p-shahi requested a review from MarcoPolo January 23, 2023 18:47

MarcoPolo requested changes Jan 23, 2023

View reviewed changes

Filter p2p addresses with invalid peerid

1e5c9d4

Add error reporting from Host.Connect

sukunrt requested a review from MarcoPolo January 25, 2023 03:09

Nits

fb7d0a9

MarcoPolo approved these changes Jan 25, 2023

View reviewed changes

MarcoPolo merged commit 084de06 into libp2p:master Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean addresses with peer id before adding to addrbook #2007

Clean addresses with peer id before adding to addrbook #2007

sukunrt commented Jan 23, 2023

sukunrt commented Jan 23, 2023

MarcoPolo left a comment

MarcoPolo Jan 23, 2023

MarcoPolo Jan 23, 2023

sukunrt Jan 24, 2023

MarcoPolo Jan 23, 2023

sukunrt Jan 24, 2023

sukunrt Jan 24, 2023

MarcoPolo Jan 23, 2023

sukunrt Jan 24, 2023

MarcoPolo Jan 23, 2023

sukunrt Jan 24, 2023

sukunrt commented Jan 24, 2023 •

edited

Loading

sukunrt commented Jan 24, 2023

MarcoPolo commented Jan 25, 2023

MarcoPolo left a comment

MarcoPolo Jan 25, 2023

sukunrt Jan 26, 2023

MarcoPolo commented Jan 25, 2023

masih commented Jan 26, 2023

		@@ -601,10 +601,12 @@ func (ab *dsAddrBook) deleteAddrs(p peer.ID, addrs []ma.Multiaddr) (err error) {
		func cleanAddrs(addrs []ma.Multiaddr) []ma.Multiaddr {

Clean addresses with peer id before adding to addrbook #2007

Clean addresses with peer id before adding to addrbook #2007

Conversation

sukunrt commented Jan 23, 2023

sukunrt commented Jan 23, 2023

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukunrt commented Jan 24, 2023 • edited Loading

sukunrt commented Jan 24, 2023

MarcoPolo commented Jan 25, 2023

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoPolo commented Jan 25, 2023

masih commented Jan 26, 2023

sukunrt commented Jan 24, 2023 •

edited

Loading