protocols/identify Check multiaddr has valid peer id component prior to caching #2338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarcoPolo
changed the title
|
tagging @mxinden here on GH as well |
mxinden
reviewed
Nov 13, 2021
protocols/identify/src/identify.rs
Outdated
| fn check_multiaddr_matches_peer_id() { | ||
| let peer_id = PeerId::random(); | ||
| let other_peer_id = PeerId::random(); | ||
| let mut addr: Multiaddr = ("/ip4/147.75.69.143/tcp/4001".to_string()) |
There was a problem hiding this comment.
Suggested change
| let mut addr: Multiaddr = ("/ip4/147.75.69.143/tcp/4001".to_string()) | |
| let mut addr: Multiaddr = "/ip4/147.75.69.143/tcp/4001".to_string() |
Are these needed?
Yes, a node could advertise a dnsaddr as their listen address. |
|
Thanks for the follow-up. Just missing a changelog entry (see release docs). |
|
Done, should I also enable this by default or do that in another PR? (essentially undoing this PR: #2312) |
Merged
... or none at all. We don't want to cache a multiaddr with a purposely wrong multiaddr. e.g. something that ends with .../p2p/some-other-peer. While this should fail to dial because we [check this before dialing](https://github.com/libp2p/rust-libp2p/blob/master/core/src/connection/pool/concurrent_dial.rs#L144), it's probably better to not cache this in the first place.
MarcoPolo
force-pushed
the
check-multiaddr-has-correct-peer-or-none-at-all
branch
from
27340f8 to
314c0f0
Compare
mxinden
approved these changes
Nov 16, 2021
There was a problem hiding this comment.
Thanks @MarcoPolo.
Done, should I also enable this by default or do that in another PR? (essentially undoing this PR: #2312)
I think it is fine for now. I would suggest enabling it by default once people had the chance to test it out on a larger deployment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Resolves #2302.
#2232 introduced a change to the Identify protocol implementation that caches the discovered
listenAddrsfrom the Identify message. The cache is then used in the addresses_of_peer method that is called by swarm code to find possible addresses to dial for a given peer.This lets the Identify assist in peer discovery of a given network behavior automatically. See #2216 for more information.
This cache was disabled in #2312 because we weren't doing any validation. This PR adds the peer id validation.
Proposed Solution
This adds a simple validation before we cache the multiaddr. We don't cache a multiaddr with a wrong final peer-id component. e.g. something that ends with
.../p2p/Qmsome-other-peer. While this should fail to dial because we check this before dialing, it's probably better to not cache this in the first place.If the multiaddr doesn't have a final
.../p2p/Qmfoocomponent, we consider it valid.Testing this locally, it seems like all
listenAddrsin Identify messages generally don't include the.../p2p/component (except for dnsaddr, but do these even show up in identify messages?)Rollback strategy
This is safe to revert. Without this, multiaddrs with a wrong peerid will fail to dial.
Next steps
This doesn't enable the caching, I can do that in this PR or another one if we want to keep things smaller.