fix(multiaddr): handle multiaddr mismatch with/without p2p proto

libp2p · Oct 30, 2023 · 7ca8502 · 7ca8502
1 parent 1ce9fe0
commit 7ca8502
Show file tree

Hide file tree

Showing 11 changed files with 77 additions and 54 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -112,7 +112,8 @@ libp2p-websocket = { version = "0.43.0", path = "transports/websocket" }
 libp2p-websocket-websys = { version = "0.3.0", path = "transports/websocket-websys" }
 libp2p-webtransport-websys = { version = "0.2.0", path = "transports/webtransport-websys" }
 libp2p-yamux = { version = "0.45.0", path = "muxers/yamux" }
-multiaddr = "0.18.0"
+# multiaddr = "0.18.0"
+multiaddr = { git = "https://github.com/multiformats/rust-multiaddr", rev = "d0e9ec219645ce8bd8ae7926e4b7316a67c35f84" }
 multihash = "0.19.1"
 multistream-select = { version = "0.13.0", path = "misc/multistream-select" }
 prometheus-client = "0.22.0"

diff --git a/protocols/identify/CHANGELOG.md b/protocols/identify/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 - Add `Info` to the `libp2p-identify::Event::Pushed` to report pushed info.
   See [PR 4527](https://github.com/libp2p/rust-libp2p/pull/4527)
+- Ensure `Multiaddr` handled and returned by `Behaviour` are `/p2p` terminated.
+  See [PR 4596](https://github.com/libp2p/rust-libp2p/pull/4596).
 
 ## 0.43.1
 

diff --git a/protocols/identify/src/behaviour.rs b/protocols/identify/src/behaviour.rs
@@ -471,6 +471,7 @@ impl PeerCache {
             Some(cache) => cache,
         };
 
+        let addresses = addresses.filter_map(|a| a.with_p2p(peer).ok());
         cache.put(peer, HashSet::from_iter(addresses));
     }
 

diff --git a/protocols/kad/CHANGELOG.md b/protocols/kad/CHANGELOG.md
@@ -11,6 +11,8 @@
 - Remove previously deprecated type-aliases.
   Users should follow the convention of importing the `libp2p::kad` module and referring to symbols as `kad::Behaviour` etc.
   See [PR 4733](https://github.com/libp2p/rust-libp2p/pull/4733). 
+- Ensure `Multiaddr` handled and returned by `Behaviour` are `/p2p` terminated.
+  See [PR 4596](https://github.com/libp2p/rust-libp2p/pull/4596).
 
 ## 0.44.6
 

diff --git a/protocols/kad/src/addresses.rs b/protocols/kad/src/addresses.rs
@@ -23,6 +23,7 @@ use smallvec::SmallVec;
 use std::fmt;
 
 /// A non-empty list of (unique) addresses of a peer in the routing table.
+/// Every address must be a fully-qualified /p2p address.
 #[derive(Clone)]
 pub struct Addresses {
     addrs: SmallVec<[Multiaddr; 6]>,

diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs
@@ -513,6 +513,10 @@ where
     /// If the routing table has been updated as a result of this operation,
     /// a [`Event::RoutingUpdated`] event is emitted.
     pub fn add_address(&mut self, peer: &PeerId, address: Multiaddr) -> RoutingUpdate {
+        // ensuring address is a fully-qualified /p2p multiaddr
+        let Ok(address) = address.with_p2p(*peer) else {
+            return RoutingUpdate::Failed;
+        };
         let key = kbucket::Key::from(*peer);
         match self.kbuckets.entry(&key) {
             kbucket::Entry::Present(mut entry, _) => {
@@ -593,6 +597,7 @@ where
         peer: &PeerId,
         address: &Multiaddr,
     ) -> Option<kbucket::EntryView<kbucket::Key<PeerId>, Addresses>> {
+        let address = &address.to_owned().with_p2p(*peer).ok()?;
         let key = kbucket::Key::from(*peer);
         match self.kbuckets.entry(&key) {
             kbucket::Entry::Present(mut entry, _) => {

diff --git a/protocols/kad/src/protocol.rs b/protocols/kad/src/protocol.rs
@@ -103,8 +103,11 @@ impl TryFrom<proto::Peer> for KadPeer {
 
         let mut addrs = Vec::with_capacity(peer.addrs.len());
         for addr in peer.addrs.into_iter() {
-            match Multiaddr::try_from(addr) {
-                Ok(a) => addrs.push(a),
+            match Multiaddr::try_from(addr).map(|addr| addr.with_p2p(node_id)) {
+                Ok(Ok(a)) => addrs.push(a),
+                Ok(Err(a)) => {
+                    log::debug!("Unable to parse multiaddr: {a} is not compatible with {node_id}");
+                }
                 Err(e) => {
                     log::debug!("Unable to parse multiaddr: {e}");
                 }
@@ -596,10 +599,34 @@ where
 mod tests {
     use super::*;
 
+    #[test]
+    fn append_p2p() {
+        let peer_id = PeerId::random();
+        let multiaddr = "/ip6/2001:db8::/tcp/1234".parse::<Multiaddr>().unwrap();
+
+        let payload = proto::Peer {
+            id: peer_id.to_bytes(),
+            addrs: vec![multiaddr.to_vec()],
+            connection: proto::ConnectionType::CAN_CONNECT,
+        };
+
+        let peer = KadPeer::try_from(payload).unwrap();
+
+        assert_eq!(peer.multiaddrs, vec![multiaddr.with_p2p(peer_id).unwrap()])
+    }
+
     #[test]
     fn skip_invalid_multiaddr() {
-        let valid_multiaddr: Multiaddr = "/ip6/2001:db8::/tcp/1234".parse().unwrap();
-        let valid_multiaddr_bytes = valid_multiaddr.to_vec();
+        let peer_id = PeerId::random();
+        let multiaddr = "/ip6/2001:db8::/tcp/1234".parse::<Multiaddr>().unwrap();
+
+        let valid_multiaddr = multiaddr.clone().with_p2p(peer_id).unwrap();
+
+        let multiaddr_with_incorrect_peer_id = {
+            let other_peer_id = PeerId::random();
+            assert_ne!(peer_id, other_peer_id);
+            multiaddr.with_p2p(other_peer_id).unwrap()
+        };
 
         let invalid_multiaddr = {
             let a = vec![255; 8];
@@ -608,12 +635,16 @@ mod tests {
         };
 
         let payload = proto::Peer {
-            id: PeerId::random().to_bytes(),
-            addrs: vec![valid_multiaddr_bytes, invalid_multiaddr],
+            id: peer_id.to_bytes(),
+            addrs: vec![
+                valid_multiaddr.to_vec(),
+                multiaddr_with_incorrect_peer_id.to_vec(),
+                invalid_multiaddr,
+            ],
             connection: proto::ConnectionType::CAN_CONNECT,
         };
 
-        let peer = KadPeer::try_from(payload).expect("not to fail");
+        let peer = KadPeer::try_from(payload).unwrap();
 
         assert_eq!(peer.multiaddrs, vec![valid_multiaddr])
     }

diff --git a/protocols/mdns/CHANGELOG.md b/protocols/mdns/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 - Don't perform IO in `Behaviour::poll`.
   See [PR 4623](https://github.com/libp2p/rust-libp2p/pull/4623).
+- Ensure `Multiaddr` handled and returned by `Behaviour` are `/p2p` terminated.
+  See [PR 4596](https://github.com/libp2p/rust-libp2p/pull/4596).
 
 ## 0.44.0 
 

diff --git a/protocols/mdns/src/behaviour/iface/query.rs b/protocols/mdns/src/behaviour/iface/query.rs
@@ -181,6 +181,7 @@ impl MdnsResponse {
 
                 peer.addresses().iter().filter_map(move |address| {
                     let new_addr = address_translation(address, &observed)?;
+                    let new_addr = new_addr.with_p2p(*peer.id()).ok()?;
 
                     Some((*peer.id(), new_addr, new_expiration))
                 })

diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs
@@ -135,7 +135,6 @@ use dial_opts::{DialOpts, PeerCondition};
 use futures::{prelude::*, stream::FusedStream};
 use libp2p_core::{
     connection::ConnectedPoint,
-    multiaddr,
     muxing::StreamMuxerBox,
     transport::{self, ListenerId, TransportError, TransportEvent},
     Endpoint, Multiaddr, Transport,
@@ -513,24 +512,30 @@ where
 
         let dials = addresses
             .into_iter()
-            .map(|a| match p2p_addr(peer_id, a) {
-                Ok(address) => {
-                    let dial = match dial_opts.role_override() {
-                        Endpoint::Dialer => self.transport.dial(address.clone()),
-                        Endpoint::Listener => self.transport.dial_as_listener(address.clone()),
-                    };
-                    match dial {
-                        Ok(fut) => fut
-                            .map(|r| (address, r.map_err(TransportError::Other)))
-                            .boxed(),
-                        Err(err) => futures::future::ready((address, Err(err))).boxed(),
+            .map(|address| {
+                let address_result = match peer_id {
+                    Some(peer_id) => address.with_p2p(peer_id),
+                    None => Ok(address),
+                };
+                match address_result {
+                    Ok(address) => {
+                        let dial = match dial_opts.role_override() {
+                            Endpoint::Dialer => self.transport.dial(address.clone()),
+                            Endpoint::Listener => self.transport.dial_as_listener(address.clone()),
+                        };
+                        match dial {
+                            Ok(fut) => fut
+                                .map(|r| (address, r.map_err(TransportError::Other)))
+                                .boxed(),
+                            Err(err) => futures::future::ready((address, Err(err))).boxed(),
+                        }
                     }
+                    Err(address) => futures::future::ready((
+                        address.clone(),
+                        Err(TransportError::MultiaddrNotSupported(address)),
+                    ))
+                    .boxed(),
                 }
-                Err(address) => futures::future::ready((
-                    address.clone(),
-                    Err(TransportError::MultiaddrNotSupported(address)),
-                ))
-                .boxed(),
             })
             .collect();
 
@@ -1703,33 +1708,6 @@ impl NetworkInfo {
     }
 }
 
-/// Ensures a given `Multiaddr` is a `/p2p/...` address for the given peer.
-///
-/// If the given address is already a `p2p` address for the given peer,
-/// i.e. the last encapsulated protocol is `/p2p/<peer-id>`, this is a no-op.
-///
-/// If the given address is already a `p2p` address for a different peer
-/// than the one given, the given `Multiaddr` is returned as an `Err`.
-///
-/// If the given address is not yet a `p2p` address for the given peer,
-/// the `/p2p/<peer-id>` protocol is appended to the returned address.
-fn p2p_addr(peer: Option<PeerId>, addr: Multiaddr) -> Result<Multiaddr, Multiaddr> {
-    let peer = match peer {
-        Some(p) => p,
-        None => return Ok(addr),
-    };
-
-    if let Some(multiaddr::Protocol::P2p(peer_id)) = addr.iter().last() {
-        if peer_id != peer {
-            return Err(addr);
-        }
-
-        return Ok(addr);
-    }
-
-    Ok(addr.with(multiaddr::Protocol::P2p(peer)))
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;