From 40e5ea5dd1338d390fe57fec20d05799347d82c6 Mon Sep 17 00:00:00 2001 From: "Roman S. Borschel" Date: Wed, 10 Jun 2020 11:10:23 +0200 Subject: [PATCH 1/3] Ensure authority discovery avoids self-lookups. Thereby additionally guard the `NetworkService` against adding the local peer to the PSM or registering a "known address" for the local peer. --- client/authority-discovery/src/lib.rs | 23 ++++++++++++++++++----- client/network/src/service.rs | 19 ++++++++++++++++--- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/client/authority-discovery/src/lib.rs b/client/authority-discovery/src/lib.rs index bc76c14331403..de98e6a4a38ae 100644 --- a/client/authority-discovery/src/lib.rs +++ b/client/authority-discovery/src/lib.rs @@ -294,13 +294,26 @@ where .authorities(&id) .map_err(Error::CallingRuntime)?; + let local_keys = match &self.role { + Role::Authority(key_store) => { + key_store.read() + .sr25519_public_keys(key_types::AUTHORITY_DISCOVERY) + .into_iter() + .collect::>() + }, + Role::Sentry => HashSet::new(), + }; + for authority_id in authorities.iter() { - if let Some(metrics) = &self.metrics { - metrics.request.inc(); - } + // Make sure we don't look up our own keys. + if !local_keys.contains(authority_id.as_ref()) { + if let Some(metrics) = &self.metrics { + metrics.request.inc(); + } - self.network - .get_value(&hash_authority_id(authority_id.as_ref())); + self.network + .get_value(&hash_authority_id(authority_id.as_ref())); + } } Ok(()) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index fd58aa631d6b2..5ef427ac39d48 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -674,6 +674,10 @@ impl NetworkService { /// and peer ID of the remote node. pub fn add_reserved_peer(&self, peer: String) -> Result<(), String> { let (peer_id, addr) = parse_str_addr(&peer).map_err(|e| format!("{:?}", e))?; + // Make sure the local peer ID is never added to the PSM. + if peer == &self.local_peer_id { + return Ok(()) // no-op + } self.peerset.add_reserved_peer(peer_id.clone()); let _ = self .to_worker @@ -695,11 +699,20 @@ impl NetworkService { /// Modify a peerset priority group. pub fn set_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { - let peers = peers.into_iter().map(|p| { - parse_addr(p).map_err(|e| format!("{:?}", e)) - }).collect::, String>>()?; + let peers = peers.into_iter() + .map(|p| parse_addr(p).map_err(|e| format!("{:?}", e))) + .filter(|r| + // Make sure the local peer ID is never added to the PSM + // or added as a "known address", even if given. + if let Ok((peer, _)) = r { + peer != &self.local_peer_id + } else { + true // keep all errors + }) + .collect::, String>>()?; let peer_ids = peers.iter().map(|(peer_id, _addr)| peer_id.clone()).collect(); + self.peerset.set_priority_group(group_id, peer_ids); for (peer_id, addr) in peers.into_iter() { From 216cf7ba3adcbe24d3f0c952bbeedb93383a1f9a Mon Sep 17 00:00:00 2001 From: "Roman S. Borschel" Date: Wed, 10 Jun 2020 11:26:35 +0200 Subject: [PATCH 2/3] Clarify comments. --- client/network/src/service.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 5ef427ac39d48..78197e68e7793 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -675,8 +675,10 @@ impl NetworkService { pub fn add_reserved_peer(&self, peer: String) -> Result<(), String> { let (peer_id, addr) = parse_str_addr(&peer).map_err(|e| format!("{:?}", e))?; // Make sure the local peer ID is never added to the PSM. + // We don't treat this as an error, because the caller may + // not know the peer ID beforehand. if peer == &self.local_peer_id { - return Ok(()) // no-op + return Ok(()) } self.peerset.add_reserved_peer(peer_id.clone()); let _ = self @@ -704,6 +706,8 @@ impl NetworkService { .filter(|r| // Make sure the local peer ID is never added to the PSM // or added as a "known address", even if given. + // We don't treat this as an error, because the caller may + // not know the peer ID beforehand. if let Ok((peer, _)) = r { peer != &self.local_peer_id } else { From dfa12b660e304f1e9a788b0258c82765d2ec2fc3 Mon Sep 17 00:00:00 2001 From: "Roman S. Borschel" Date: Wed, 10 Jun 2020 12:49:53 +0200 Subject: [PATCH 3/3] See if returning errors is ok. --- client/network/src/service.rs | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 78197e68e7793..2297fe6a52f72 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -672,13 +672,14 @@ impl NetworkService { /// Adds a `PeerId` and its address as reserved. The string should encode the address /// and peer ID of the remote node. + /// + /// Returns an `Err` if the given string is not a valid multiaddress + /// or contains an invalid peer ID (which includes the local peer ID). pub fn add_reserved_peer(&self, peer: String) -> Result<(), String> { let (peer_id, addr) = parse_str_addr(&peer).map_err(|e| format!("{:?}", e))?; // Make sure the local peer ID is never added to the PSM. - // We don't treat this as an error, because the caller may - // not know the peer ID beforehand. - if peer == &self.local_peer_id { - return Ok(()) + if peer_id == self.local_peer_id { + return Err("Local peer ID cannot be added as a reserved peer.".to_string()) } self.peerset.add_reserved_peer(peer_id.clone()); let _ = self @@ -700,18 +701,21 @@ impl NetworkService { } /// Modify a peerset priority group. + /// + /// Returns an `Err` if one of the given addresses contains an invalid + /// peer ID (which includes the local peer ID). pub fn set_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { let peers = peers.into_iter() - .map(|p| parse_addr(p).map_err(|e| format!("{:?}", e))) - .filter(|r| - // Make sure the local peer ID is never added to the PSM - // or added as a "known address", even if given. - // We don't treat this as an error, because the caller may - // not know the peer ID beforehand. - if let Ok((peer, _)) = r { - peer != &self.local_peer_id - } else { - true // keep all errors + .map(|p| match parse_addr(p) { + Err(e) => Err(format!("{:?}", e)), + Ok((peer, addr)) => + // Make sure the local peer ID is never added to the PSM + // or added as a "known address", even if given. + if peer == self.local_peer_id { + Err("Local peer ID in priority group.".to_string()) + } else { + Ok((peer, addr)) + } }) .collect::, String>>()?;