Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protocols/identify: Validate addresses before caching #2302

Closed
mxinden opened this issue Oct 19, 2021 · 5 comments · Fixed by #2338
Closed

protocols/identify: Validate addresses before caching #2302

mxinden opened this issue Oct 19, 2021 · 5 comments · Fixed by #2338
Assignees
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted

Comments

@mxinden
Copy link
Member

mxinden commented Oct 19, 2021

With #2232 libp2p-identify caches discovered listen addresses to provide additional addresses when dialing a peer via NetworkBehaviour::addresses_of_peer.

Before caching received listen addresses, libp2p-identify should validate these addresses. Validations:

  • If the address contains a /p2p/QmXXX make sure QmXX matches the peer's PeerId.
  • ...

//CC @AgeManning: This might be the reason for the dial failures you are seeing.

@mxinden mxinden added difficulty:easy help wanted getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Oct 19, 2021
@thomaseizinger
Copy link
Contributor

Do we want to treat validation errors as bad behaviour and disconnect from that peer?

@mxinden
Copy link
Member Author

mxinden commented Oct 22, 2021

For now I would suggest we just ignore those addresses with a peer ID mismatch in libp2p-identify.

Reasoning: Maybe there is a benign way (in old versions) how this could happen. Disconnecting on mismatch might lead to large disconnects in live networks. Without measuring this beforehand, I would deem this change too risky.

Do you have some time to file a patch? I am sorry, I am a bit low on time / energy (sick). If not, I can just set the default value of cache_size to 0 for v0.40.0 and re-enable it by default with the patch in v0.41.0.

@thomaseizinger
Copy link
Contributor

Do you have some time to file a patch?

Not at the moment but I am okay with disabling the current behaviour if it causes problems.

mxinden added a commit to mxinden/rust-libp2p that referenced this issue Oct 27, 2021
mxinden added a commit that referenced this issue Oct 27, 2021
@MarcoPolo
Copy link
Contributor

@thomaseizinger I'll pick this up unless you're working on it, thanks!

@thomaseizinger
Copy link
Contributor

@thomaseizinger I'll pick this up unless you're working on it, thanks!

Go for it! I haven't started on this yet :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted
Projects
None yet
3 participants