authority-discovery: Ignore multi-addresses without IP from DHT records

prdoc/pr_6545.prdoc

@@ -0,0 +1,12 @@
+title: Ignore multi-addresses without IP from DHT records
+
+doc:
+  - audience: [ Node Dev, Node Operator ]
+    description: |
+      This PR ensures that multiaddresses without an IP protocol are not published or utilized from DHT records.
+
+crates: 
+  - name: sc-network
+    bump: patch
+  - name: sc-authority-discovery
+    bump: patch

substrate/client/authority-discovery/src/worker.rs

@@ -280,7 +280,9 @@ where
 			config
 				.public_addresses
 				.into_iter()
-				.map(|address| AddressType::PublicAddress(address).without_p2p(local_peer_id))
+				.filter_map(|address| {
+					AddressType::PublicAddress(address).without_p2p(local_peer_id)
+				})
 				.collect()
 		};
 
@@ -399,6 +401,7 @@ where
 			.filter_map(|address| {
 				address_is_global(&address)
 					.then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id))
+					.flatten()
 			})
 			.take(MAX_GLOBAL_LISTEN_ADDRESSES)
 			.peekable();
@@ -411,6 +414,7 @@ where
 			.filter_map(|address| {
 				(publish_non_global_ips || address_is_global(&address))
 					.then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id))
+					.flatten()
 			})
 			.peekable();
 
@@ -846,7 +850,9 @@ where
 		// Ignore [`Multiaddr`]s without [`PeerId`] or with own addresses.
 		let addresses: Vec<Multiaddr> = addresses
 			.into_iter()
-			.filter(|a| get_peer_id(&a).filter(|p| *p != local_peer_id).is_some())
+			.filter(|a| {
+				!address_is_empty(a) && get_peer_id(&a).filter(|p| *p != local_peer_id).is_some()
+			})
 			.collect();
 
 		let remote_peer_id = single(addresses.iter().map(|a| get_peer_id(&a)))
@@ -998,7 +1004,9 @@ impl AddressType {
 	///
 	/// In case the peer id in the address does not match the local peer id, an error is logged for
 	/// `ExternalAddress` and `GlobalListenAddress`.
-	fn without_p2p(self, local_peer_id: PeerId) -> Multiaddr {
+	///
+	/// Returns `None` if the address is empty after removing the `/p2p/..` part.
+	fn without_p2p(self, local_peer_id: PeerId) -> Option<Multiaddr> {
 		// Get the address and the source str for logging.
 		let (mut address, source) = match self {
 			AddressType::PublicAddress(address) => (address, "public address"),
@@ -1016,10 +1024,26 @@ impl AddressType {
 			}
 			address.pop();
 		}
-		address
+
+		// Empty addresses cannot be published.
+		if address.is_empty() {
+			None
+		} else {
+			Some(address)
+		}
 	}
 }
 
+/// Checks if an authority-discovery address is empty.
+///
+/// An address is considered empty if:
+///  - The multiaddr is empty.
+///  - The multiaddr contains only the `/p2p/..` protocol.
+fn address_is_empty(address: &Multiaddr) -> bool {
+	address.is_empty() ||
+		address.iter().all(|protocol| matches!(protocol, multiaddr::Protocol::P2p(_)))
+}
+
 /// NetworkProvider provides [`Worker`] with all necessary hooks into the
 /// underlying Substrate networking. Using this trait abstraction instead of
 /// `sc_network::NetworkService` directly is necessary to unit test [`Worker`].
@@ -1199,3 +1223,23 @@ impl<Block: BlockT, Client, DhtEventStream> Worker<Client, Block, DhtEventStream
 		self.addr_cache.insert(authority, addresses);
 	}
 }
+
+#[cfg(test)]
+mod test {
+	use super::*;
+
+	#[test]
+	fn check_empty_address() {
+		// Plain empty address.
+		let empty_address = Multiaddr::empty();
+		assert!(address_is_empty(&empty_address));
+
+		// Address is still unusable.
+		let address_with_p2p = Multiaddr::from(multiaddr::Protocol::P2p(PeerId::random().into()));
+		assert!(address_is_empty(&address_with_p2p));
+
+		// Address is not empty.
+		let valid_address: Multiaddr = "/dns/domain1.com/tcp/30333".parse().unwrap();
+		assert!(!address_is_empty(&valid_address));
+	}
+}

substrate/client/network/src/litep2p/discovery.rs

@@ -488,6 +488,59 @@ impl Discovery {
 
 		(false, None)
 	}
+
+	/// Verify the external address discovered by the identify protocol.
+	///
+	/// This ensures that:
+	/// - the address is not empty
+	/// - the address contains a valid IP address
+	/// - the address is for the local peer ID
+	/// - the address always contains `/p2p/<local_peer_id>` protocol.
+	///
+	/// The provided peer ID is only used for logging purposes.
+	fn verify_external_address(&mut self, address: Multiaddr, peer: PeerId) -> Option<Multiaddr> {
+		if address.is_empty() {
+			log::warn!(
+				target: LOG_TARGET,
+				"🔍 Discovered empty address from {peer:?}",
+			);
+			return None;
+		};
+
+		// Expect the address to contain a valid IP address.
+		if std::matches!(
+			address.iter().next(),
+			Some(
+				Protocol::Dns(_) |
+					Protocol::Dns4(_) |
+					Protocol::Dns6(_) |
+					Protocol::Ip6(_) |
+					Protocol::Ip4(_),
+			)
+		) {
+			log::warn!(
+				target: LOG_TARGET,
+				"🔍 Discovered external address from {peer:?} does not contain a valid IP address: {address}",
+			);
+			return None;
+		};
+
+		if let Some(Protocol::P2p(peer_id)) = address.iter().last() {
+			// Invalid address if the reported peer ID is not the local peer ID.
+			if peer_id != *self.local_peer_id.as_ref() {
+				log::warn!(
+					target: LOG_TARGET,
+					"🔍 Discovered external address from {peer:?} that is not us: {address}",
+				);
+				return None
+			}
+
+			return Some(address)
+		}
+
+		// Ensure the address contains the local peer ID.
+		Some(address.with(Protocol::P2p(self.local_peer_id.into())))
+	}
 }
 
 impl Stream for Discovery {
@@ -596,20 +649,7 @@ impl Stream for Discovery {
 				observed_address,
 				..
 			})) => {
-				let observed_address =
-					if let Some(Protocol::P2p(peer_id)) = observed_address.iter().last() {
-						if peer_id != *this.local_peer_id.as_ref() {
-							log::warn!(
-								target: LOG_TARGET,
-								"Discovered external address for a peer that is not us: {observed_address}",
-							);
-							None
-						} else {
-							Some(observed_address)
-						}
-					} else {
-						Some(observed_address.with(Protocol::P2p(this.local_peer_id.into())))
-					};
+				let observed_address = this.verify_external_address(observed_address, peer);
 
 				// Ensure that an external address with a different peer ID does not have
 				// side effects of evicting other external addresses via `ExternalAddressExpired`.

substrate/client/network/src/litep2p/mod.rs

@@ -748,6 +748,14 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkBackend<B, H> for Litep2pNetworkBac
 						NetworkServiceCommand::AddKnownAddress { peer, address } => {
 							let mut address: Multiaddr = address.into();
 
+							if address.is_empty() {
+								log::debug!(
+									target: LOG_TARGET,
+									"Ignoring empty address for {peer:?}",
+								);
+								continue
+							}
+
 							if !address.iter().any(|protocol| std::matches!(protocol, Protocol::P2p(_))) {
 								address.push(Protocol::P2p(litep2p::PeerId::from(peer).into()));
 							}

substrate/client/network/src/peer_info.rs

@@ -31,6 +31,7 @@ use libp2p::{
 		Info as IdentifyInfo,
 	},
 	identity::PublicKey,
+	multiaddr::Protocol,
 	ping::{Behaviour as Ping, Config as PingConfig, Event as PingEvent},
 	swarm::{
 		behaviour::{
@@ -66,6 +67,9 @@ pub struct PeerInfoBehaviour {
 	ping: Ping,
 	/// Periodically identifies the remote and responds to incoming requests.
 	identify: Identify,
+	/// Identity of our local node.
+	local_peer_id: PeerId,
+
 	/// Information that we know about all nodes.
 	nodes_info: FnvHashMap<PeerId, NodeInfo>,
 	/// Interval at which we perform garbage collection in `nodes_info`.
@@ -123,6 +127,7 @@ impl PeerInfoBehaviour {
 		local_public_key: PublicKey,
 		external_addresses: Arc<Mutex<HashSet<Multiaddr>>>,
 	) -> Self {
+		let local_peer_id = local_public_key.to_peer_id();
 		let identify = {
 			let cfg = IdentifyConfig::new("/substrate/1.0".to_string(), local_public_key)
 				.with_agent_version(user_agent)
@@ -134,6 +139,7 @@ impl PeerInfoBehaviour {
 		Self {
 			ping: Ping::new(PingConfig::new()),
 			identify,
+			local_peer_id,
 			nodes_info: FnvHashMap::default(),
 			garbage_collect: Box::pin(interval(GARBAGE_COLLECT_INTERVAL)),
 			external_addresses: ExternalAddresses { addresses: external_addresses },
@@ -178,6 +184,53 @@ impl PeerInfoBehaviour {
 				"Received pong from node we're not connected to {:?}", peer_id);
 		}
 	}
+
+	/// Verify the external address discovered by the identify protocol.
+	///
+	/// This ensures that:
+	/// - the address is not empty
+	/// - the address contains a valid IP address
+	/// - the address is for the local peer ID
+	fn verify_external_address(&mut self, address: &Multiaddr) -> bool {
+		if address.is_empty() {
+			log::warn!(
+				target: "sub-libp2p",
+				"🔍 Discovered empty address",
+			);
+			return false;
+		};
+
+		// Expect the address to contain a valid IP address.
+		if std::matches!(
+			address.iter().next(),
+			Some(
+				Protocol::Dns(_) |
+					Protocol::Dns4(_) |
+					Protocol::Dns6(_) |
+					Protocol::Ip6(_) |
+					Protocol::Ip4(_),
+			)
+		) {
+			log::warn!(
+				target: "sub-libp2p",
+				"🔍 Discovered external address does not contain a valid IP address: {address}",
+			);
+			return false;
+		};
+
+		if let Some(Protocol::P2p(peer_id)) = address.iter().last() {
+			// Invalid address if the reported peer ID is not the local peer ID.
+			if peer_id != self.local_peer_id {
+				log::warn!(
+					target: "sub-libp2p",
+					"🔍 Discovered external address that is not us: {address}",
+				);
+				return false
+			}
+		}
+
+		true
+	}
 }
 
 /// Gives access to the information about a node.
@@ -401,17 +454,20 @@ impl NetworkBehaviour for PeerInfoBehaviour {
 				self.external_addresses.remove(e.addr);
 			},
 			FromSwarm::NewExternalAddrCandidate(e @ NewExternalAddrCandidate { addr }) => {
-				self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e));
-				self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e));
-
-				// Manually confirm all external address candidates.
-				// TODO: consider adding [AutoNAT protocol](https://docs.rs/libp2p/0.52.3/libp2p/autonat/index.html)
-				// (must go through the polkadot protocol spec) or implemeting heuristics for
-				// approving external address candidates. This can be done, for example, by
-				// approving only addresses reported by multiple peers.
-				// See also https://github.com/libp2p/rust-libp2p/pull/4721 introduced
-				// in libp2p v0.53 for heuristics approach.
-				self.pending_actions.push_back(ToSwarm::ExternalAddrConfirmed(addr.clone()));
+				// Verify the external address is valid.
+				if self.verify_external_address(addr) {
+					self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e));
+					self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e));
+
+					// Manually confirm all external address candidates.
+					// TODO: consider adding [AutoNAT protocol](https://docs.rs/libp2p/0.52.3/libp2p/autonat/index.html)
+					// (must go through the polkadot protocol spec) or implemeting heuristics for
+					// approving external address candidates. This can be done, for example, by
+					// approving only addresses reported by multiple peers.
+					// See also https://github.com/libp2p/rust-libp2p/pull/4721 introduced
+					// in libp2p v0.53 for heuristics approach.
+					self.pending_actions.push_back(ToSwarm::ExternalAddrConfirmed(addr.clone()));
+				}
 			},
 			FromSwarm::ExternalAddrConfirmed(e @ ExternalAddrConfirmed { addr }) => {
 				self.ping.on_swarm_event(FromSwarm::ExternalAddrConfirmed(e));

substrate/client/network/types/src/multiaddr.rs

@@ -42,6 +42,11 @@ pub struct Multiaddr {
 }
 
 impl Multiaddr {
+	/// Returns `true` if this multiaddress is empty.
+	pub fn is_empty(&self) -> bool {
+		self.multiaddr.is_empty()
+	}
+
 	/// Create a new, empty multiaddress.
 	pub fn empty() -> Self {
 		Self { multiaddr: LiteP2pMultiaddr::empty() }