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

[RFC] peer discovery with mDNS #80

Merged
merged 14 commits into from
May 6, 2019
Merged

[RFC] peer discovery with mDNS #80

merged 14 commits into from
May 6, 2019

Conversation

richardschneider
Copy link
Contributor

Background discussion at Future of mDNS discovery

discovery/mdns.md Outdated Show resolved Hide resolved
<host-name> AAAA <ipv6 address>


Multiple A and AAAA records are expected. The `TXT` is not needed for IPFS peer discovery, but is required by DNS-SD.
Copy link
Member

Choose a reason for hiding this comment

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

So, we'll need it for multiaddrs. We're able to get away with this for now but not once we enable QUIC by default. Ideally, we'd only use TXT records. However, we have to supply an A or AAAA record and also have to supply a port. Personally, I'm all for using our TCP port and real IP addresses when announcing but mostly ignoring them when accepting (unless we get no TXT records in which case we can try the IP/PORT from the other records).

Note: We'd probably want to use dnsaddr=/ip4/.../tcp/... as the format for TXT records.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TXT record dnsaddr=/full/address is a problem. Strings must be less than 128 bytes.

dnsaddr=/ip6/2406:e001:1467:1:ecec:9ca6:XXXX:XXXX/tcp/4009/ipfs/QmTp1fwYnJxc5rUEVM2xowcDiyR61J7yvmBRUQzT4g8MHs almost reaches this limit. If the peer-id hashing algorithm changes to sha2-512 then the byte limit is exceeded.

I strongly suggest that TXT dnsaddr is not used.

Copy link
Contributor Author

@richardschneider richardschneider Jul 24, 2018

Choose a reason for hiding this comment

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

Also, we are just trying to discover peers on the local link. So to my thinking is that an IP address, port and peer ID is all that is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we send multiple records?

Copy link
Member

Choose a reason for hiding this comment

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

Well, we also have protocols like QUIC and may add other random protocols in the future. We also do need to put the peer ID somewhere.

It looks like the standard way to do this is to break it into multiple records. You can see how DKIM does it here: https://serverfault.com/questions/255580/how-do-i-enter-a-strong-long-dkim-key-into-dns

We could also use a base64 encoded binary multiaddr but I'd rather not.

@richardschneider
Copy link
Contributor Author

@Stebalien

We also do need to put the peer ID somewhere.

The peer id is the first label of the SRV record's name.

@richardschneider
Copy link
Contributor Author

I'm not sure that the spec on TXT record allows multiple strings with the same property name. I know for certain that most "mdns browsers" only show the first/last value,

I'll dig into the specs and report back.

@richardschneider
Copy link
Contributor Author

richardschneider commented Jul 25, 2018

I was wrong about the maximum string length it is 255 bytes, not 128.

But this does not help with the network MTU. To make life easier, I want all required resource records (in Additional Records) to fit into one message response.

@Stebalien
Copy link
Member

Yeah, but MTUs are usually at least 1280 bytes. I think we'll be well within that.

discovery/mdns.md Outdated Show resolved Hide resolved
@richardschneider
Copy link
Contributor Author

@Stebalien I hope all you concerns have been answered, dnsaddr=... is now part of the spec.

If you're happy, I start on the worked examples.

@richardschneider
Copy link
Contributor Author

Should we specifically exclude loopback addresses (127.0.0.1 and ::1) from being discovered?

@Stebalien
Copy link
Member

Should we specifically exclude loopback addresses (127.0.0.1 and ::1) from being discovered?

No, they're actually quite useful for finding multiple nodes on the same machine. However, we do want to avoid advertising addresses on the wrong interfaces. We should also filter incoming advertisements but that gets a bit tricker. At a minimum, we could filter incoming localhost advertisements to localhost.


`peer-id` is the ID of the peer. It normally is the base-58 enconding of the hash of the peer's public key.
`peer-name` is the case-insenstive ID of the peer and less than 64 characters. It is normally is the base-32 encoding of peer's ID.
Copy link
Member

@lidel lidel Sep 17, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess until ipfs/kubo#5287 is solved this would require apps to do a manual step of converting back to Base58, yes?

peer-name is just a unique name, we should not assume it's the peer id. The dnsaddr ends with the peer ID.

What if PeerID is longer than 64 characters? Should we split after 63 characters like noted in ipfs/in-web-browsers#89 ?

I think Workaround A: Split at 63rd character makes sense.

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

tomaka commented Oct 10, 2018

Ideally a link to theses specs should be added in 4-architecture.md.

@richardschneider richardschneider changed the title feat: initial description of peer discovery with MDNS [RFC] peer discovery with MDNS Oct 11, 2018
@richardschneider
Copy link
Contributor Author

@Stebalien @diasdavid What is needed to get some approval and then merge it?

@ghost ghost assigned daviddias Oct 12, 2018
@daviddias
Copy link
Member

@richardschneider it would be great to give it a proper review (I can do it probably later next week). One strong case to merge would be if the implementations were both done (probably they are already) and that interoperability has been tested.

@tomaka
Copy link
Member

tomaka commented Oct 22, 2018

@richardschneider it would be great to give it a proper review (I can do it probably later next week). One strong case to merge would be if the implementations were both done (probably they are already) and that interoperability has been tested.

I just tried the latest ipfs/go-ipfs:master Docker image.
Sniffing traffic with Wireshark, it seems to always generate two identical queries in a row, with the service name _ipfs-discovery._udp.local, and never seems to generate any answer, neither from its own queries (as the specs say it should) nor from other queries.

@richardschneider
Copy link
Contributor Author

@tomaka Yes, go-ipfs mdns has been broken for a long time. Background discussion at Future of mDNS discovery.

The "novel" idea here, is that we first write the spec and then implement it!

@jacobheun
Copy link
Contributor

The state of this spec looks reasonable to me for us to get this merged in and marked as Draft. rust-libp2p looks to already have implemented this. It would be ideal to get our MDNS implementations in go and js to an interoperable and better state.

@richardschneider
Copy link
Contributor Author

How should a private network interact with MDNS? I think the service-name should change from _p2p._udp.local to _p2p-x.udp.local , where x is the fingerprint of the private network in base16.

@ghost ghost assigned raulk Apr 24, 2019
@raulk raulk changed the title [RFC] peer discovery with MDNS [RFC] peer discovery with mDNS Apr 24, 2019
@raulk
Copy link
Member

raulk commented Apr 24, 2019

In the interest of advancing this, I'm listing TODOs based on above comments:

  • support loopback addresses to enable local process discovery.
  • context-sensitive address advertisement based on network interface.
  • private network discovery.

@richardschneider – if you agree on points 1 & 2, do you have enough bandwidth to enhance the spec?

Re: private networks, SGTM unless we want to hide private network membership somehow. But arguably, if that were the case, you shouldn't be using mDNS at all.

Should we enforce that nodes belonging to a pnet only respond to queries for that pnet?

@richardschneider
Copy link
Contributor Author

@raulk

  • The spec does not prevent loopback, it was just a question.
  • I think we should postpone "context-sensitive address advertisement based on network interface". My feeling is that a node can filter on TXT dnsaddr already (like swarm filters)
  • By using a private network specific service-name we get for free that nodes belonging to a pnet only respond to queries for that pnet.

It's midnight my time, I'll update the doc for a private network tomorrow.

@raulk
Copy link
Member

raulk commented Apr 25, 2019

@richardschneider that's great, thanks!

@richardschneider
Copy link
Contributor Author

@raulk IMHO this is ready to go.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

There may still be some open questions but we can fill those in as we discover them in implementation.

LGTM!

(and thanks @richardschneider for sticking this out)

@Stebalien Stebalien merged commit 743b6fd into master May 6, 2019
@ghost ghost removed the in progress label May 6, 2019
@Stebalien
Copy link
Member

(i.e., let's just iterate in new PRs so we have something we can work with now)

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