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

fix(swarm): Showcase impossibility of dialing self by ip. #5124

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vnermolaev
Copy link
Contributor

@vnermolaev vnermolaev commented Jan 25, 2024

Description

This PR addresses issue #4460.

However, the issue described in #4460 is not actually an issue, and, as the update test showcases, errors were already raised when swarm tried to dial self.
The updated test adds ::1 and 127.0.0.1 version of self.

Change checklist

I do not believe there must be any changelog items as the code did not change at all.

  • I have performed a self-review of my own code
  • (Not required) I have made corresponding changes to the documentation
  • (Not required) I have added tests that prove my fix is effective or that my feature works
  • (Not required) A changelog entry has been made in the appropriate crates

@vnermolaev vnermolaev changed the title fix(swarm) Disallow dialing of 0.0.0.0 #4460 Extend the test showcasing impossibility of dialing self by ip. fix(swarm): Disallow dialing of 0.0.0.0 #4460 Extend the test showcasing impossibility of dialing self by ip. Jan 25, 2024
@vnermolaev vnermolaev changed the title fix(swarm): Disallow dialing of 0.0.0.0 #4460 Extend the test showcasing impossibility of dialing self by ip. fix(swarm): Extend the test showcasing impossibility of dialing self by ip. Jan 25, 2024
@vnermolaev vnermolaev changed the title fix(swarm): Extend the test showcasing impossibility of dialing self by ip. fix(swarm): Showcase impossibility of dialing self by ip. Jan 25, 2024
Copy link

mergify bot commented Jan 25, 2024

This pull request has merge conflicts. Could you please resolve them @vnermolaev? 🙏

@vnermolaev
Copy link
Contributor Author

I am getting a failure with Cargo.lock; however, my changes do not concern it.

@thomaseizinger
Copy link
Contributor

What is the actual error that is returned here? I think to correctly fix #4460, we should catch this case explicitly and not return on the kernel to fail dialing 0.0.0.0.

@vnermolaev
Copy link
Contributor Author

The error returned is

SwarmEvent::OutgoingConnectionError {
   peer_id,
   // multiaddr,
   error: DialError::Transport(errors),
    ..
}

It is already the case that libp2p fails on dialling 0.0.0.0 as shown by the original test. However, this test did not list ::1 as an erroneous case, which already is. I just added an ipv6 address to showcase that.

#4460 requests to disallow dialling self, which is already the case.
As I see it, there are two options:

  • keep things intact because libp2p won't dial self. This requires the small changes I introduced,
  • filter the localhost versions from the list of addresses to dial with a warning, then ofc no dial error will be returned.

I believe the former is better because it already works and produces expected errors. In the latter case, imagine the list of addresses consists of only the localhost versions, which will be filtered out, then one shall receive an error stating that no addresses were present. Such an error is confusing and requires a log investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants