Add libp2p-mdns #590

tomaka · 2018-10-27T11:04:47Z

Allows automatically discovering other nodes on the same local network.

tomaka · 2018-10-28T17:21:16Z

Disabled libp2p-mdns on emscripten, as it's not available in the browser.

dvdplm · 2018-10-29T08:30:37Z

misc/mdns/src/dns.rs

+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+// DEALINGS IN THE SOFTWARE.
+


Module docs?

misc/mdns/src/dns.rs

dvdplm · 2018-10-29T08:41:20Z

misc/mdns/src/dns.rs

+        append_txt_record(&mut out, &peer_id_bytes, ttl, Some(&txt_to_send_bytes[..]))?;
+    }
+
+    if out.len() > 9000 {


Is 9000 from the spec or simply an empirically found reasonable upper limit?

From the specs.

Then I'd make it a const. Or a comment.

dvdplm · 2018-10-29T08:47:39Z

misc/mdns/src/dns.rs

+    cmp::min(secs, From::from(u32::max_value())) as u32
+}
+
+/// Appends a big-endian u32 to `out`.


Why do you prefer to not use the byteorder crate for the append_*methods?

The methods of byteorder return a Result, which is not great.

How about:

fn append_u32(out: &mut Vec<u8>, value: u32) { let n = out.len(); out.resize(n + 4, 0); BE::write_u32(&mut out[n ..], value) }

BE is an alias for byteorder::BigEndian.

Well, to me the point of using byteorder is to make the code more straight-forward to read, and that doesn't.

dvdplm · 2018-10-29T08:56:26Z

misc/mdns/src/lib.rs

+            let builder = net2::UdpBuilder::new_v4()?;
+            builder.reuse_address(true)?;
+            platform_specific(&builder)?;
+            builder.bind(("0.0.0.0", 5353))?


Consider using a const MDNS_PORT?

mDNS is an old and well-established standard, and there's 0 chance that the port changes in the future.

It was more of a readability concern – easier to read than just the number imo.

RFC 6762 states that "[...] using UDP source port 5353 signals the presence of a fully compliant Multicast DNS querier [...]" which this crate is not. Queriers issuing one-shot queries must not use source port 5353.

dvdplm · 2018-10-29T09:02:23Z

misc/mdns/src/lib.rs

+        let socket = UdpSocket::from_std(socket, &Handle::default())?;
+        socket.set_multicast_loop_v4(true)?;
+        socket.set_multicast_ttl_v4(255)?;
+        // TODO: correct interfaces?


Yes, 224.0.0.251 should be right.

It's not about the IP here but about 0.0.0.0.
Some mDNS examples enumerate the interfaces available on the machine and create a socket for each one of them. However there is no cross-platform way to do that at the moment.

The docs here indicate that it should be fine to use 0.0.0.0, but maybe not ideal for IPv4:

https://docs.rs/net2/0.2.33/net2/trait.UdpSocketExt.html#tymethod.join_multicast_v4

https://docs.rs/net2/0.2.33/net2/trait.UdpSocketExt.html#tymethod.join_multicast_v6

I think it should be fine. Going to remove that TODO.

dvdplm · 2018-10-29T09:10:45Z

misc/mdns/src/lib.rs

+pub enum MdnsPacket<'a> {
+    /// A query made by a remote.
+    Query(MdnsQuery<'a>),
+    /// A response received by a remote to one of our queries.


Is it A response **sent** by a remote in answer to one of our queries.?

dvdplm · 2018-10-29T09:12:25Z

misc/mdns/src/lib.rs

+    ///
+    /// Pass the ID of the local peer, and the list o addresses we're listening on.
+    ///
+    /// If there are more than 2^16-1 addresses, ignores the other.


s/other/others/

misc/mdns/src/lib.rs

twittner · 2018-10-30T15:07:37Z

misc/mdns/src/dns.rs

+
+/// Decodes a `<character-string>` (as defined by RFC1035) into a `Vec` of ASCII characters.
+// TODO: better error type?
+pub fn decode_character_string(mut from: &[u8]) -> Result<Vec<u8>, ()> {


Why allocating a vector? As currently implemented, this could just return the sub-slice. The current use site parses the result as a str and turns it into a Multiaddr which will allocate another vector.

That's not implemented, but we need to turn \\ into \ and \" into ".
Since it's quite unlikely to happen in practice, I'm going to use a Cow though.

twittner · 2018-10-30T17:05:27Z

misc/mdns/src/dns.rs

+    cmp::min(secs, From::from(u32::max_value())) as u32
+}
+
+/// Appends a big-endian u32 to `out`.


How about:

fn append_u32(out: &mut Vec<u8>, value: u32) { let n = out.len(); out.resize(n + 4, 0); BE::write_u32(&mut out[n ..], value) }

BE is an alias for byteorder::BigEndian.

twittner · 2018-10-31T11:06:30Z

misc/mdns/src/lib.rs

+            let builder = net2::UdpBuilder::new_v4()?;
+            builder.reuse_address(true)?;
+            platform_specific(&builder)?;
+            builder.bind(("0.0.0.0", 5353))?


RFC 6762 states that "[...] using UDP source port 5353 signals the presence of a fully compliant Multicast DNS querier [...]" which this crate is not. Queriers issuing one-shot queries must not use source port 5353.

misc/mdns/src/dns.rs

twittner · 2018-10-31T12:00:55Z

misc/mdns/src/dns.rs

+
+    append_u16(&mut out, id);
+    // Flags ; 0x80 for an answer.
+    append_u16(&mut out, 0x8000);


Should AA not be set to 1?

twittner · 2018-10-31T12:08:35Z

misc/mdns/src/dns.rs

+
+    // Flags.
+    out.push(0x00);
+    out.push(0x0c);     // PTR record.


Since type is 16bit, why not append_u16(&mut out, 0xc) as you did elsewhere?

twittner · 2018-10-31T12:16:59Z

misc/mdns/src/dns.rs

+    out.push(0x00);
+    out.push(0x0c);     // PTR record.
+    out.push(0x80);
+    out.push(0x01);


What class value is this?

twittner · 2018-10-31T12:30:40Z

misc/mdns/src/lib.rs

+
+    /// Polls the service for packets.
+    pub fn poll(&mut self) -> Async<MdnsPacket> {
+        // Send a query every time `query_interval` fires.


According to RFC 6762, exponential delays should be used for continuous querying and queries should include known answers to not cause unnecessary re-transmissions and elevated network traffic.

twittner · 2018-10-31T12:39:05Z

misc/mdns/src/lib.rs

+        peer_id: PeerId,
+        addresses: TAddresses,
+        ttl: Duration,
+    ) -> Result<(), MdnsResponseError>


Should responders not probe and announce on startup (cf. RFC 6762, section 8)?

It would be really great to avoid having to figure out the IP address of our network interfaces, as there's no easy cross-platform way to do that.
If the specs allow avoiding that, it would be great.

twittner · 2018-10-31T12:46:39Z

misc/mdns/src/lib.rs

+                                return Async::NotReady;
+                            }
+                        } else {
+                            return Async::Ready(MdnsPacket::Response(MdnsResponse {


IIUC, it is trivial for an adversary to inject itself into to the set of listen addresses of another peer just by answering queries.

Yes, by design it's not secure.

twittner · 2018-11-21T14:37:21Z

misc/mdns/src/lib.rs

+        socket.set_multicast_loop_v4(true)?;
+        socket.set_multicast_ttl_v4(255)?;
+        // TODO: correct interfaces?
+        socket.join_multicast_v4(&From::from([224, 0, 0, 251]), &From::from([0, 0, 0, 0]))?;


You could use std::net::Ipv4Addr::UNSPECIFIED here.

twittner · 2018-11-21T16:23:56Z

misc/mdns/src/dns.rs

+
+    // Flags.
+    append_u16(&mut out, 0x000c);
+    append_u16(&mut out, 0x8001);


IIUC this sets the flush-cache bit saying that this record is the sole truth. I guess this implies that this responder owns the name and no other responder is expected to answer that query.

If I set this to 0x8000, then the dnsparser crate is incapable of parsing the response w generate. Not sure why.

Without the flush-cache-bit you would use 1 (internet class).

Add libp2p-mdns

4e9fef7

tomaka force-pushed the mdns branch from 100f638 to 4e9fef7 Compare October 27, 2018 11:04

tomaka added 3 commits October 27, 2018 13:08

Fix win32

6c4ad95

Make compatible with Rust 1.29

f1e0c49

Remove mDNS on esmcripten

795f7e2

dvdplm reviewed Oct 29, 2018

View reviewed changes

tomaka added 2 commits October 29, 2018 13:49

Merge remote-tracking branch 'upstream/master' into mdns

185645f

Fix concerns

183e4eb

tomaka force-pushed the mdns branch from 5bb0591 to 183e4eb Compare October 29, 2018 12:52

tomaka mentioned this pull request Oct 29, 2018

Future of mDNS discovery libp2p/libp2p#28

Open

More concern

4b435ee

soyuka mentioned this pull request Oct 30, 2018

Implement peers / network datrs/hypercore#11

Open

17 tasks

twittner reviewed Oct 31, 2018

View reviewed changes

tomaka added 4 commits November 2, 2018 13:31

Use append_u16

ff9b1c3

Make decode_character_string return a Cow

8843055

Add TODO

323e3b0

Merge remote-tracking branch 'upstream/master' into mdns

0183bd5

ghost assigned tomaka Nov 21, 2018

ghost added the in progress label Nov 21, 2018

tomaka requested a review from twittner November 21, 2018 14:06

tomaka added 3 commits November 21, 2018 16:14

Don't send queries from 5353

7397397

Fix flags

d748cf5

More flags fix

0bb3335

twittner approved these changes Nov 21, 2018

View reviewed changes

tomaka added 2 commits November 22, 2018 18:14

More concerns

e68a788

Merge branch 'master' into mdns

a67f550

tomaka added pr-queued-to-merge and removed in progress labels Nov 22, 2018

Fix flags

e1e9745

ghost added the in progress label Nov 23, 2018

tomaka added 3 commits November 24, 2018 11:50

Merge branch 'master' into mdns

cff276e

Fix removed env_logger

65b5ede

Merge branch 'master' into mdns

f812b27

tomaka merged commit 1b05132 into libp2p:master Nov 24, 2018

ghost removed the in progress label Nov 24, 2018

tomaka deleted the mdns branch November 24, 2018 12:55

tomaka mentioned this pull request Dec 5, 2018

Implement mDNS discovery #350

Closed

Add libp2p-mdns #590

Add libp2p-mdns #590

Conversation

tomaka commented Oct 27, 2018

tomaka commented Oct 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka Oct 29, 2018 •

edited

Loading