protocols/mdns: Use a random alphanumeric string for peer name #2311

jochasinga · 2021-10-25T21:26:13Z

mxinden

Very cool. Thanks a bunch for picking this up @jochasinga!

Would you mind adding a changelog entry as well?

//CC @peat as you have been working on this logic in the past.

protocols/mdns/src/dns.rs

jochasinga · 2021-10-30T16:53:54Z

Very cool. Thanks a bunch for picking this up @jochasinga!

Would you mind adding a changelog entry as well?

//CC @peat as you have been working on this logic in the past.

In the changelog it is currently 0.32.0rc-1. I didn't find a doc explaining the process. Do you mind advise how this change should be reflected in the versioning?

mxinden · 2021-11-02T19:44:47Z

Sorry for the confusion @jochasinga. Could you apply the diff below?

diff --git a/protocols/mdns/CHANGELOG.md b/protocols/mdns/CHANGELOG.md
index f3287379..a2376242 100644
--- a/protocols/mdns/CHANGELOG.md
+++ b/protocols/mdns/CHANGELOG.md
@@ -1,3 +1,9 @@
+# 0.32.1 [unreleased]
+
+- Use a random alphanumeric string for mDNS peer name (see [PR 2311]).
+
+[PR 2311]: https://github.com/libp2p/rust-libp2p/pull/2311/
+
 # 0.32.0 [2021-11-01]
 
 - Make default features of `libp2p-core` optional.
diff --git a/protocols/mdns/Cargo.toml b/protocols/mdns/Cargo.toml
index 43370a9b..0bf1605e 100644
--- a/protocols/mdns/Cargo.toml
+++ b/protocols/mdns/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "libp2p-mdns"
 edition = "2018"
-version = "0.32.0"
+version = "0.32.1"
 description = "Implementation of the libp2p mDNS discovery method"
 authors = ["Parity Technologies <[email protected]>"]
 license = "MIT"

mxinden

Looks good to me.

One small nit that just now caught my attention: Given that the peer name is no longer based on the peer ID, would you mind renaming the references from peer ID to peer name?

protocols/mdns/src/dns.rs

elenaf9 · 2021-11-03T12:19:08Z

Just that I understand it correctly: with this change, the PeerId that is reported in an MdnsEvent (Either in the DiscoveredAddrsIter or ExpiredAddrsIter) is not the actual id of the peer that is listening on that address?

If yes, shouldn't this then also be changed in the examples? E.g. /examples/distributed-key-value-store.rs adds the mdns peer-address pairs to the kademlia DHT, which then wouldn't really make sense.

Because the peer name is no longer based on peer ID. See also: libp2p#2311 (review)

mxinden · 2021-11-03T16:18:15Z

Just that I understand it correctly: with this change, the PeerId that is reported in an MdnsEvent (Either in the DiscoveredAddrsIter or ExpiredAddrsIter) is not the actual id of the peer that is listening on that address?

If yes, shouldn't this then also be changed in the examples? E.g. /examples/distributed-key-value-store.rs adds the mdns peer-address pairs to the kademlia DHT, which then wouldn't really make sense.

You are raising a very important point here @elenaf9 that I wish I didn't miss before writing #2285. Sorry for the trouble this will cause @jochasinga.

I will write up more details on #2285, just a short summary for now:

When receiving an mDNS response, rust-libp2p actually does parse the peer name as a PeerId and discards an address in case it contains a /p2p/QmXX PeerId which doesn't match the one retrieved from the peer name field.

rust-libp2p/protocols/mdns/src/query.rs

Lines 162 to 176 in 012287a

    
           let mut peer_name = match record_value.rsplitn(4, |c| c == '.').last() { 
        
               Some(n) => n.to_owned(), 
        
               None => return None, 
        
           }; 
        
           // if we have a segmented name, remove the '.' 
        
           peer_name.retain(|c| c != '.'); 
        
           let peer_id = match data_encoding::BASE32_DNSCURVE.decode(peer_name.as_bytes()) { 
        
               Ok(bytes) => match PeerId::from_bytes(&bytes) { 
        
                   Ok(id) => id, 
        
                   Err(_) => return None, 
        
               }, 
        
               Err(_) => return None, 
        
           };

The above implies that peers not running this patch will discard any responses send by peers that are running this patch.

To prevent a network to split, we will need to do a step in between namely to ignore the peer name of a mDNS response and not discard responses in case there is a mismatch. Once that is rolled out on a network, step two would be to roll out this patch setting the peer name to a random string.

jochasinga · 2021-11-05T16:20:45Z

@mxinden keep me posted on the update. Would love to get this fixed up.

mxinden · 2021-11-09T14:58:21Z

@mxinden keep me posted on the update. Would love to get this fixed up.

See #2285 (comment).

protocols/mdns/src/query.rs

@MarcoPolo

Per @MarcoPolo points that an error isn't useful for the caller and following MdnsPacket API. See also: libp2p#2311 (comment)

jochasinga · 2021-11-17T21:44:09Z

@mxinden Do we need to change the examples per @elenaf9 comment?

MarcoPolo · 2021-11-17T23:39:48Z

Just that I understand it correctly: with this change, the PeerId that is reported in an MdnsEvent (Either in the DiscoveredAddrsIter or ExpiredAddrsIter) is not the actual id of the peer that is listening on that address?

(from @elenaf9)

With the changes in here now, I don't think this is true anymore.

Now the peerid is correct in MdnsResponse, which is used to generate the MdnsEvent. So I don't think we need to change the examples. If this is wrong, then the correct fix is to have the correct PeerId be reported, not changing the examples.

MarcoPolo · 2021-11-17T23:40:31Z

@jochasinga can you please rebase this branch?

Fixes libp2p#2285

Rename `encode_peer_id` to `generate_peer_id` Because `encode_peer_id` now does not encode anything.

Update CHANGELOG and Cargo.toml

Because the peer name is no longer based on peer ID. See also: libp2p#2311 (review)

- Remove the peer ID parsing logic and ignore the peer name. - Remove `my_peer_id` from `MdnsPeer::new(...)` parameters. - Ignore addresses with bad or no peer IDs, use the peer ID of the first address with a peer ID as a reference, and ignore all addresses whose peer ID does not match the first peer ID. Note that `MdnsPeer::new(...)` now returns a Result type because there is no gaurantee that there will be a peer ID to create the peer.

@MarcoPolo

Per @MarcoPolo points that an error isn't useful for the caller and following MdnsPacket API. See also: libp2p#2311 (comment)

jochasinga · 2021-11-19T16:50:11Z

@mxinden can you please run checks on this?

protocols/mdns/src/query.rs

Refactor match statement to map since it's more idiomatic and concise. Use expect in tests for better cue in the test.

mxinden

Code looks good to me. Thank you for the many iterations!

Just two comments on the release process:

mxinden · 2021-11-23T17:09:16Z

protocols/mdns/CHANGELOG.md

@@ -1,3 +1,9 @@
+# 0.33.1 [unreleased]
+
+- Use a random alphanumeric string for mDNS peer name (see [PR 2311]).


Suggested change

- Use a random alphanumeric string for mDNS peer name (see [PR 2311]).

- Use a random alphanumeric string instead of the local peer ID for mDNS peer

name (see [PR 2311]).

Note that previous versions of `libp2p-mdns` expect the peer name to be a

valid peer ID. Thus they will be unable to discover nodes running this new

version of `libp2p-mdns`.

Do I need to correct the protocols/mdns/CHANGELOG to 0.34.0 as well here?

Yes please 🙏

protocols/mdns/Cargo.toml

Co-authored-by: Max Inden <[email protected]>

mxinden

I will merge later today unless anyone objects.

Thanks @jochasinga for all the work and thanks @MarcoPolo for the additional help!

…p#2311) With libp2p/specs#368 the definition of the _peer name_ changed in the mDNS specification. > peer-name is the case-insensitive unique identifier of the peer, and is less > than 64 characters. > > As the this field doesn't carry any meaning, it is sufficient to ensure the > uniqueness of this identifier. Peers SHOULD generate a random, lower-case > alphanumeric string of least 32 characters in length when booting up their > node. Peers SHOULD NOT use their Peer ID here because a future Peer ID could > exceed the DNS label limit of 63 characters. https://github.com/libp2p/specs/blob/master/discovery/mdns.md This commit adjusts `libp2p-mdns` accordingly. Also see libp2p/go-libp2p#1222 for the corresponding change on the Golang side. Co-authored-by: Max Inden <[email protected]>

mxinden reviewed Oct 27, 2021

View reviewed changes

protocols/mdns/src/dns.rs Outdated Show resolved Hide resolved

jochasinga marked this pull request as ready for review November 2, 2021 20:12

mxinden reviewed Nov 3, 2021

View reviewed changes

protocols/mdns/src/dns.rs Outdated Show resolved Hide resolved

protocols/mdns/src/dns.rs Outdated Show resolved Hide resolved

protocols/mdns/src/dns.rs Outdated Show resolved Hide resolved

jochasinga added a commit to jochasinga/rust-libp2p that referenced this pull request Nov 3, 2021

Rename references from peer ID to peer name

7150248

Because the peer name is no longer based on peer ID. See also: libp2p#2311 (review)

mxinden mentioned this pull request Nov 9, 2021

protocols/mdns: Use a random string as peer-name #2285

Closed

jochasinga commented Nov 17, 2021

View reviewed changes

protocols/mdns/src/query.rs Outdated Show resolved Hide resolved

jochasinga added a commit to jochasinga/rust-libp2p that referenced this pull request Nov 17, 2021

Returns an Option for MdnsPeer::new(...)

7dc6b1f

Per @MarcoPolo points that an error isn't useful for the caller and following MdnsPacket API. See also: libp2p#2311 (comment)

jochasinga added 6 commits November 18, 2021 10:06

Use a random alphanumeric string for mDNS peer name

e10ff38

Fixes libp2p#2285

Remove unused code and rename the function

5d563c6

Rename `encode_peer_id` to `generate_peer_id` Because `encode_peer_id` now does not encode anything.

Update CHANGELOG

99e1cc9

Update CHANGELOG and Cargo.toml

Rename references from peer ID to peer name

36aa172

Because the peer name is no longer based on peer ID. See also: libp2p#2311 (review)

Returns an Option for MdnsPeer::new(...)

f31ce25

Per @MarcoPolo points that an error isn't useful for the caller and following MdnsPacket API. See also: libp2p#2311 (comment)

jochasinga force-pushed the 2285-random-peername branch from 7dc6b1f to f31ce25 Compare November 18, 2021 18:15

Clean up with rustfmt

4a005e0

MarcoPolo reviewed Nov 19, 2021

View reviewed changes

protocols/mdns/src/query.rs Outdated Show resolved Hide resolved

protocols/mdns/src/query.rs Outdated Show resolved Hide resolved

Minor refactor

25f7176

Refactor match statement to map since it's more idiomatic and concise. Use expect in tests for better cue in the test.

jochasinga requested a review from mxinden November 21, 2021 20:41

mxinden reviewed Nov 23, 2021

View reviewed changes

Apply suggestions on release version

36d3c2f

Co-authored-by: Max Inden <[email protected]>

*: Fix libp2p-mdns version and changelog entries

d769fd6

mxinden approved these changes Nov 25, 2021

View reviewed changes

Merge branch 'libp2p/master' into 2285-random-peername

684eb16

mxinden changed the title ~~Use a random alphanumeric string for mDNS peer name~~ protocols/mdns: Use a random alphanumeric string for peer name Nov 25, 2021

mxinden merged commit f0000d5 into libp2p:master Nov 25, 2021

jochasinga deleted the 2285-random-peername branch November 30, 2021 01:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protocols/mdns: Use a random alphanumeric string for peer name #2311

protocols/mdns: Use a random alphanumeric string for peer name #2311

jochasinga commented Oct 25, 2021

mxinden left a comment

jochasinga commented Oct 30, 2021

mxinden commented Nov 2, 2021

mxinden left a comment

elenaf9 commented Nov 3, 2021

mxinden commented Nov 3, 2021

jochasinga commented Nov 5, 2021

mxinden commented Nov 9, 2021

jochasinga commented Nov 17, 2021

MarcoPolo commented Nov 17, 2021

MarcoPolo commented Nov 17, 2021

jochasinga commented Nov 19, 2021

mxinden left a comment

mxinden Nov 23, 2021

jochasinga Nov 23, 2021

mxinden Nov 24, 2021

mxinden left a comment

		@@ -1,3 +1,9 @@
		# 0.33.1 [unreleased]

		- Use a random alphanumeric string for mDNS peer name (see [PR 2311]).

-- Use a random alphanumeric string for mDNS peer name (see [PR 2311]).
+- Use a random alphanumeric string instead of the local peer ID for mDNS peer
+  name (see [PR 2311]).
+  Note that previous versions of `libp2p-mdns` expect the peer name to be a
+  valid peer ID. Thus they will be unable to discover nodes running this new
+  version of `libp2p-mdns`.

protocols/mdns: Use a random alphanumeric string for peer name #2311

protocols/mdns: Use a random alphanumeric string for peer name #2311

Conversation

jochasinga commented Oct 25, 2021

mxinden left a comment

Choose a reason for hiding this comment

jochasinga commented Oct 30, 2021

mxinden commented Nov 2, 2021

mxinden left a comment

Choose a reason for hiding this comment

elenaf9 commented Nov 3, 2021

mxinden commented Nov 3, 2021

jochasinga commented Nov 5, 2021

mxinden commented Nov 9, 2021

jochasinga commented Nov 17, 2021

MarcoPolo commented Nov 17, 2021

MarcoPolo commented Nov 17, 2021

jochasinga commented Nov 19, 2021

mxinden left a comment

Choose a reason for hiding this comment

mxinden Nov 23, 2021

Choose a reason for hiding this comment

jochasinga Nov 23, 2021

Choose a reason for hiding this comment

mxinden Nov 24, 2021

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment