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(mdns): remove unnecessary question about loopback addresses #571

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

Conversation

maschad
Copy link
Member

@maschad maschad commented Sep 1, 2023

The issues section which includes a question about ignoring loopback addresses doesn't provide any clarity for implementations to follow

Related libp2p/js-libp2p#2014

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change. Sending localhost addresses helps two libp2p nodes running on the same host to discover each other, which seems like a valuable thing, considering how cheap mDNS is.

If you want to propose a normative change here, you'll need to move that sentence out of the Issues section.

@maschad
Copy link
Member Author

maschad commented Sep 4, 2023

If you want to propose a normative change here, you'll need to move that sentence out of the Issues section.

Thanks for that suggestion, I've moved to the Responses section. The impetus behind this PR was to remove the ambiguity created by the question

Loopback and "NAT busting" addresses should not sent and must be ignored on receipt?

which doesn't really give any direction on how implementations should handle loopback addresses.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong, and I don't understand the motivation for this change. Please include more details when proposing changes.

discovery/mdns.md Outdated Show resolved Hide resolved
@maschad
Copy link
Member Author

maschad commented Sep 22, 2023

I decided to just remove the statement all together as a normative change. I don't think the open question provides any clarity to implementers.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR title and description now needs fixing but removing this LGTM

@achingbrain
Copy link
Member

achingbrain commented Sep 26, 2023

Sending localhost addresses helps two libp2p nodes running on the same host to discover each other, which seems like a valuable thing, considering how cheap mDNS is.

The problem is the mDNS spec says you should only send link-local addresses (section 3 rfc-6762), which means things like "alexs-computer.local." instead of "localhost" or "127.0.0.1" so that packets can be sent with the expectation that they will arrive (section 1.2 rfc-3927).

go-libp2p sends localhost/loopback addresses so js-libp2p does too but really they shouldn't.

It's worth removing the ambiguity from our spec because the RFCs are clear, and go/js should be updated to send valid mDNS responses.

@maschad maschad changed the title fix(mdns): require loopback addresses to be ignored fix(mdns): remove unnecessary statement about link-local addresses Sep 26, 2023
@maschad maschad changed the title fix(mdns): remove unnecessary statement about link-local addresses fix(mdns): remove unnecessary question about link-local addresses Sep 26, 2023
@maschad maschad changed the title fix(mdns): remove unnecessary question about link-local addresses fix(mdns): remove unnecessary question about loopback addresses Sep 26, 2023
@thomaseizinger
Copy link
Contributor

Sending localhost addresses helps two libp2p nodes running on the same host to discover each other, which seems like a valuable thing, considering how cheap mDNS is.

The problem is the mDNS spec says you should only send link-local addresses (section 3 rfc-6762), which means things like "alexs-computer.local." instead of "localhost" or "127.0.0.1" so that packets can be sent with the expectation that they will arrive (section 1.2 rfc-3927).

go-libp2p sends localhost/loopback addresses so js-libp2p does too but really they shouldn't.

It's worth removing the ambiguity from our spec because the RFCs are clear, and go/js should be updated to send valid mDNS responses.

So effectively, this means that we should only use mDNS to discover nodes on the same link but provide a different discovery mechanism for hosts running the same machine.

Whilst that is technically correct, it seems like quite the burden if we are only doing this to "just" comply with the mDNS spec. I am a big proponent of following specs clearly, that is what they are there for. The question is, is developing an entire new "host-local discovery" mechanism worth the effort? The way we currently use mDNS is convenient for testing on a local machine, running examples and the like.

Do we know of any problems or incompatibilities that arise from us not following the mDNS spec here? If there aren't any, I'd suggest we simply document this in the spec with an explanation similar to above.

@achingbrain
Copy link
Member

So effectively, this means that we should only use mDNS to discover nodes on the same link but provide a different discovery mechanism for hosts running the same machine.

Not exactly - as I read it the idea is I publish "alexs-computer.local." which the DNS resolver on my computer would resolve to 127.0.0.1 but on your computer it might resolve to 192.168.1.123, so discovery on the local host is handled.

Do we know of any problems or incompatibilities that arise from us not following the mDNS spec here?

There is the potential to waste dial resources as when "localhost" is published you're not sure if the "localhost" in a received mDNS message is your local host or not so you have to dial it to find out.

@thomaseizinger
Copy link
Contributor

So effectively, this means that we should only use mDNS to discover nodes on the same link but provide a different discovery mechanism for hosts running the same machine.

Not exactly - as I read it the idea is I publish "alexs-computer.local." which the DNS resolver on my computer would resolve to 127.0.0.1 but on your computer it might resolve to 192.168.1.123, so discovery on the local host is handled.

With publishing you mean what we include in the TXT records right? Maybe I am misunderstanding something but I thought the TXT record doesn't matter here. The way I read the RFC is that what is not allowed is sending mDNS packets on the loopback interface, irrespective of what the content is.

I had a look at what we do in rust-libp2p. We continuously monitor all interfaces of the machine and set up a socket for each interface:

https://github.com/libp2p/rust-libp2p/blob/c8b5f49ec2478338df2f7c323d572751f7b66b90/protocols/mdns/src/behaviour.rs#L258-L287

Note that we ignore the loopback interface (line 262 - 264). So if your node is only listening on 127.0.0.1, mDNS is not going to work. But if you are listening on 0.0.0.0 and you have a network interface with 192.168.1.0/24, we will set up a socket for that and send and receive mDNS packets that advertise this IP address to other nodes.

Thus, we can discover other nodes on the same machine as long as they listen at least on one non-loopback interface. That is compliant with the mDNS spec, right?

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

Successfully merging this pull request may close these issues.

4 participants