fix: ensure Multiaddrs are /p2p terminated where possible

stormshield-frb · web-flow · commit d649070da9fd · 2023-11-14T01:36:18.000Z
Fixes #4573. Pull-Request: #4596.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -80,10 +80,10 @@ libp2p-dcutr = { version = "0.11.0", path = "protocols/dcutr" }
 libp2p-dns = { version = "0.41.1", path = "transports/dns" }
 libp2p-floodsub = { version = "0.44.0", path = "protocols/floodsub" }
 libp2p-gossipsub = { version = "0.46.1", path = "protocols/gossipsub" }
-libp2p-identify = { version = "0.44.0", path = "protocols/identify" }
+libp2p-identify = { version = "0.44.1", path = "protocols/identify" }
 libp2p-identity = { version = "0.2.8" }
-libp2p-kad = { version = "0.45.1", path = "protocols/kad" }
-libp2p-mdns = { version = "0.45.0", path = "protocols/mdns" }
+libp2p-kad = { version = "0.45.2", path = "protocols/kad" }
+libp2p-mdns = { version = "0.45.1", path = "protocols/mdns" }
 libp2p-memory-connection-limits = { version = "0.2.0", path = "misc/memory-connection-limits" }
 libp2p-metrics = { version = "0.14.1", path = "misc/metrics" }
 libp2p-mplex = { version = "0.41.0", path = "muxers/mplex" }
@@ -122,7 +122,7 @@ rw-stream-sink = { version = "0.4.0", path = "misc/rw-stream-sink" }
 
 [patch.crates-io]
 
-# Patch away `libp2p-idnentity` in our dependency tree with the workspace version.
+# Patch away `libp2p-identity` in our dependency tree with the workspace version.
 # `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
 # As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
 # we import via `rust-multiaddr`.
diff --git a/protocols/identify/CHANGELOG.md b/protocols/identify/CHANGELOG.md
@@ -1,3 +1,8 @@
+## 0.44.1 - unreleased
+
+- Ensure `Multiaddr` handled and returned by `Behaviour` are `/p2p` terminated.
+  See [PR 4596](https://github.com/libp2p/rust-libp2p/pull/4596).
+
 ## 0.44.0
 
 - Add `Info` to the `libp2p-identify::Event::Pushed` to report pushed info.
diff --git a/protocols/identify/Cargo.toml b/protocols/identify/Cargo.toml
@@ -3,7 +3,7 @@ name = "libp2p-identify"
 edition = "2021"
 rust-version = { workspace = true }
 description = "Nodes identifcation protocol for libp2p"
-version = "0.44.0"
+version = "0.44.1"
 authors = ["Parity Technologies <admin@parity.io>"]
 license = "MIT"
 repository = "https://github.com/libp2p/rust-libp2p"
diff --git a/protocols/identify/src/behaviour.rs b/protocols/identify/src/behaviour.rs
@@ -466,6 +466,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
@@ -1,3 +1,8 @@
+## 0.45.2 - unreleased
+
+- Ensure `Multiaddr` handled and returned by `Behaviour` are `/p2p` terminated.
+  See [PR 4596](https://github.com/libp2p/rust-libp2p/pull/4596).
+
 ## 0.45.1
 
 - Fix a bug where calling `Behaviour::remove_address` with an address not in the peer's bucket would remove the peer from the routing table if the bucket has only one address left.
diff --git a/protocols/kad/Cargo.toml b/protocols/kad/Cargo.toml
@@ -3,7 +3,7 @@ name = "libp2p-kad"
 edition = "2021"
 rust-version = { workspace = true }
 description = "Kademlia protocol for libp2p"
-version = "0.45.1"
+version = "0.45.2"
 authors = ["Parity Technologies <admin@parity.io>"]
 license = "MIT"
 repository = "https://github.com/libp2p/rust-libp2p"
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
@@ -39,6 +39,7 @@ use libp2p_swarm::StreamProtocol;
 use std::marker::PhantomData;
 use std::{convert::TryFrom, time::Duration};
 use std::{io, iter};
+use tracing::debug;
 
 /// The protocol name used for negotiating with multistream-select.
 pub(crate) const DEFAULT_PROTO_NAME: StreamProtocol = StreamProtocol::new("/ipfs/kad/1.0.0");
@@ -103,11 +104,12 @@ 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),
-                Err(e) => {
-                    tracing::debug!("Unable to parse multiaddr: {e}");
+            match Multiaddr::try_from(addr).map(|addr| addr.with_p2p(node_id)) {
+                Ok(Ok(a)) => addrs.push(a),
+                Ok(Err(a)) => {
+                    debug!("Unable to parse multiaddr: {a} is not compatible with {node_id}")
                 }
+                Err(e) => debug!("Unable to parse multiaddr: {e}"),
             };
         }
 
@@ -596,10 +598,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 +634,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
@@ -1,3 +1,8 @@
+## 0.45.1 - unreleased
+
+- Ensure `Multiaddr` handled and returned by `Behaviour` are `/p2p` terminated.
+  See [PR 4596](https://github.com/libp2p/rust-libp2p/pull/4596).
+
 ## 0.45.0
 
 - Don't perform IO in `Behaviour::poll`.
diff --git a/protocols/mdns/Cargo.toml b/protocols/mdns/Cargo.toml
@@ -2,7 +2,7 @@
 name = "libp2p-mdns"
 edition = "2021"
 rust-version = { workspace = true }
-version = "0.45.0"
+version = "0.45.1"
 description = "Implementation of the libp2p mDNS discovery method"
 authors = ["Parity Technologies <admin@parity.io>"]
 license = "MIT"
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,
@@ -525,7 +524,7 @@ where
 
         let dials = addresses
             .into_iter()
-            .map(|a| match p2p_addr(peer_id, a) {
+            .map(|a| match peer_id.map_or(Ok(a.clone()), |p| a.with_p2p(p)) {
                 Ok(address) => {
                     let (dial, span) = match dial_opts.role_override() {
                         Endpoint::Dialer => (
@@ -1746,33 +1745,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::*;

Original file line number	Diff line number	Diff line change
`@@ -466,6 +466,7 @@ impl PeerCache {`
`466`	`466`	`Some(cache) => cache,`
`467`	`467`	`};`
`468`	`468`
	`469`	`+ let addresses = addresses.filter_map(\|a\| a.with_p2p(peer).ok());`
`469`	`470`	`cache.put(peer, HashSet::from_iter(addresses));`
`470`	`471`	`}`
`471`	`472`