From fd3d78276017e47d00e2997acf368e76d29458e1 Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 12 Mar 2020 12:42:22 +0100 Subject: [PATCH 1/8] [network discovery]: refactor assemble_packet --- util/network-devp2p/src/discovery.rs | 82 ++++++++++++++++++---------- 1 file changed, 52 insertions(+), 30 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 2a9820dda6c..b444b47badf 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -27,7 +27,7 @@ use lru_cache::LruCache; use parity_bytes::Bytes; use rlp::{Rlp, RlpStream}; -use parity_crypto::publickey::{KeyPair, recover, Secret, sign}; +use parity_crypto::publickey::{KeyPair, recover, Secret, Signature, sign}; use network::Error; use network::IpFilter; @@ -45,6 +45,7 @@ const PACKET_PING: u8 = 1; const PACKET_PONG: u8 = 2; const PACKET_FIND_NODE: u8 = 3; const PACKET_NEIGHBOURS: u8 = 4; +const PACKET_KIND_LEN: usize = 1; const PING_TIMEOUT: Duration = Duration::from_millis(500); const FIND_NODE_TIMEOUT: Duration = Duration::from_secs(2); @@ -57,8 +58,9 @@ const REQUEST_BACKOFF: [Duration; 4] = [ Duration::from_secs(64) ]; -const NODE_LAST_SEEN_TIMEOUT: Duration = Duration::from_secs(24*60*60); - +const HASH_LEN: usize = 32; +const SIGNATURE_LEN: usize = 65; +const NODE_LAST_SEEN_TIMEOUT: Duration = Duration::from_secs(24 * 60 * 60); const OBSERVED_NODES_MAX_SIZE: usize = 10_000; #[derive(Clone, Debug)] @@ -413,8 +415,7 @@ impl Discovery { } fn send_packet(&mut self, packet_id: u8, address: SocketAddr, payload: Bytes) -> Result { - let packet = assemble_packet(packet_id, payload, &self.secret)?; - let hash = H256::from_slice(&packet[0..32]); + let (packet, hash) = assemble_packet(packet_id, payload, &self.secret)?; self.send_to(packet, address); Ok(hash) } @@ -475,23 +476,23 @@ impl Discovery { } pub fn on_packet(&mut self, packet: &[u8], from: SocketAddr) -> Result, Error> { - // validate packet - if packet.len() < 32 + 65 + 4 + 1 { + if !packet_has_valid_length(packet.len()) { + warn!(target: "discovery", "Invalid packet length: {}", packet.len()); return Err(Error::BadProtocol); } - let hash_signed = keccak(&packet[32..]); + let hash_signed = keccak(&packet[HASH_LEN..]); if hash_signed[..] != packet[0..32] { return Err(Error::BadProtocol); } - let signed = &packet[(32 + 65)..]; - let signature = H520::from_slice(&packet[32..(32 + 65)]); - let node_id = recover(&signature.into(), &keccak(signed))?; - let packet_id = signed[0]; - let rlp = Rlp::new(&signed[1..]); + let payload_with_packet_id = &packet[HASH_LEN + SIGNATURE_LEN..]; + let signature = H520::from_slice(&packet[HASH_LEN..HASH_LEN + SIGNATURE_LEN]); + let node_id = recover(&signature.into(), &keccak(payload_with_packet_id))?; + let packet_id = payload_with_packet_id[0]; + let rlp = Rlp::new(&payload_with_packet_id[1..]); match packet_id { - PACKET_PING => self.on_ping(&rlp, node_id, from, hash_signed.as_bytes()), + PACKET_PING => self.on_ping(&rlp, node_id, from, hash_signed), PACKET_PONG => self.on_pong(&rlp, node_id, from), PACKET_FIND_NODE => self.on_find_node(&rlp, node_id, from), PACKET_NEIGHBOURS => self.on_neighbours(&rlp, node_id, from), @@ -516,7 +517,7 @@ impl Discovery { entry.endpoint.is_allowed(&self.ip_filter) && entry.id != self.id } - fn on_ping(&mut self, rlp: &Rlp, node_id: NodeId, from: SocketAddr, echo_hash: &[u8]) -> Result, Error> { + fn on_ping(&mut self, rlp: &Rlp, node_id: NodeId, from: SocketAddr, echo_hash: H256) -> Result, Error> { trace!(target: "discovery", "Got Ping from {:?}", &from); let ping_from = if let Ok(node_endpoint) = NodeEndpoint::from_rlp(&rlp.at(1)?) { node_endpoint @@ -855,24 +856,45 @@ fn append_expiration(rlp: &mut RlpStream) { rlp.append(×tamp); } -fn assemble_packet(packet_id: u8, payload: Bytes, secret: &Secret) -> Result { - let mut packet = Bytes::with_capacity(payload.len() + 32 + 65 + 1); - packet.resize(32 + 65, 0); // Filled in below + +/// Helper function to assemble node discovery messages +/// +/// The packet format is: `hash | signature | payload | packet_type`, where the maximum packet length is 1280 bytes +fn assemble_packet(packet_id: u8, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { + let packet_len = payload.len() + HASH_LEN + SIGNATURE_LEN + PACKET_KIND_LEN; + + if !packet_has_valid_length(packet_len) { + warn!(target: "discovery", "Invalid packet length: {}", packet_len); + return Err(Error::BadProtocol); + } + + let mut packet = Bytes::with_capacity(packet_len); + packet.resize(HASH_LEN + SIGNATURE_LEN, 0); // Filled in below packet.push(packet_id); packet.extend(payload); - let hash = keccak(&packet[(32 + 65)..]); - let signature = match sign(secret, &hash) { - Ok(s) => s, - Err(e) => { - warn!(target: "discovery", "Error signing UDP packet"); - return Err(Error::from(e)); - } - }; - packet[32..(32 + 65)].copy_from_slice(&signature[..]); - let signed_hash = keccak(&packet[32..]); - packet[0..32].copy_from_slice(signed_hash.as_bytes()); - Ok(packet) + let signature = sign_payload_with_packet_id(secret, &packet[HASH_LEN + SIGNATURE_LEN ..])?; + packet[HASH_LEN..(HASH_LEN + SIGNATURE_LEN)].copy_from_slice(&signature[..]); + let signed_hash = keccak(&packet[HASH_LEN..]); + packet[..HASH_LEN].copy_from_slice(signed_hash.as_bytes()); + Ok((packet, signed_hash)) +} + +fn sign_payload_with_packet_id(secret: &Secret, payload_with_packet_id: &[u8]) -> Result { + let hash = keccak(payload_with_packet_id); + sign(secret, &hash).map_err(|e| { + warn!(target: "discovery", "Error signing UDP packet"); + e.into() + }) +} + +fn packet_has_valid_length(packet_len: usize) -> bool { + // Q(niklasad1): why `4`? + if packet_len > MAX_DATAGRAM_SIZE || packet_len < HASH_LEN + SIGNATURE_LEN + PACKET_KIND_LEN + 4 { + false + } else { + true + } } // Selects the next node in a bucket to ping. Chooses the eligible node least recently seen. From 8533a05552f7af3144a12c23fb00d85c143515fd Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 12 Mar 2020 15:35:16 +0100 Subject: [PATCH 2/8] fix test build --- util/network-devp2p/src/discovery.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index b444b47badf..77f9a830c64 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -1058,8 +1058,8 @@ mod tests { let key = Random.generate(); discovery.send_find_node(&node_entries[100], key.public()).unwrap(); for payload in Discovery::prepare_neighbours_packets(&node_entries[101..116]) { - let packet = assemble_packet(PACKET_NEIGHBOURS, payload, &key.secret()).unwrap(); - discovery.on_packet(&packet, from.clone()).unwrap(); + let (packet, _hash) = assemble_packet(PACKET_NEIGHBOURS, payload, &key.secret()).unwrap(); + discovery.on_packet(&packet, from).unwrap(); } let num_nodes = total_bucket_nodes(&discovery.node_buckets); @@ -1070,8 +1070,8 @@ mod tests { // FIND_NODE does not time out because it receives k results. discovery.send_find_node(&node_entries[100], key.public()).unwrap(); for payload in Discovery::prepare_neighbours_packets(&node_entries[101..117]) { - let packet = assemble_packet(PACKET_NEIGHBOURS, payload, &key.secret()).unwrap(); - discovery.on_packet(&packet, from.clone()).unwrap(); + let (packet, _hash) = assemble_packet(PACKET_NEIGHBOURS, payload, &key.secret()).unwrap(); + discovery.on_packet(&packet, from).unwrap(); } let num_nodes = total_bucket_nodes(&discovery.node_buckets); @@ -1303,15 +1303,15 @@ mod tests { ep1.to_rlp_list(&mut incorrect_pong_rlp); incorrect_pong_rlp.append(&H256::zero()); append_expiration(&mut incorrect_pong_rlp); - let incorrect_pong_data = assemble_packet( + let (incorrect_pong_data, _hash) = assemble_packet( PACKET_PONG, incorrect_pong_rlp.drain(), &discovery2.secret ).unwrap(); - if let Some(_) = discovery1.on_packet(&incorrect_pong_data, ep2.address.clone()).unwrap() { + if let Some(_) = discovery1.on_packet(&incorrect_pong_data, ep2.address).unwrap() { panic!("Expected no changes to discovery1's table because pong hash is incorrect"); } // Delivery of valid pong response should add to routing table. - if let Some(table_updates) = discovery1.on_packet(&pong_data.payload, ep2.address.clone()).unwrap() { + if let Some(table_updates) = discovery1.on_packet(&pong_data.payload, ep2.address).unwrap() { assert_eq!(table_updates.added.len(), 1); assert_eq!(table_updates.removed.len(), 0); assert!(table_updates.added.contains_key(&discovery2.id)); @@ -1332,10 +1332,10 @@ mod tests { ep3.to_rlp_list(&mut unexpected_pong_rlp); unexpected_pong_rlp.append(&H256::zero()); append_expiration(&mut unexpected_pong_rlp); - let unexpected_pong = assemble_packet( + let (unexpected_pong, _hash) = assemble_packet( PACKET_PONG, unexpected_pong_rlp.drain(), key3.secret() ).unwrap(); - if let Some(_) = discovery1.on_packet(&unexpected_pong, ep3.address.clone()).unwrap() { + if let Some(_) = discovery1.on_packet(&unexpected_pong, ep3.address).unwrap() { panic!("Expected no changes to discovery1's table for unexpected pong"); } } From 0ccdbbfabe3ab65e43e2e6c502efd6aba40e0808 Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 12 Mar 2020 16:26:10 +0100 Subject: [PATCH 3/8] fix minor nits cleanup --- util/network-devp2p/src/discovery.rs | 36 ++++++++++++++++------------ 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 77f9a830c64..1f61c5db72d 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -16,7 +16,6 @@ use std::collections::{HashMap, HashSet, VecDeque}; use std::collections::hash_map::Entry; -use std::default::Default; use std::net::SocketAddr; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; @@ -24,33 +23,40 @@ use ethereum_types::{H256, H520}; use keccak_hash::keccak; use log::{debug, trace, warn}; use lru_cache::LruCache; +use network::{Error, IpFilter}; use parity_bytes::Bytes; -use rlp::{Rlp, RlpStream}; - use parity_crypto::publickey::{KeyPair, recover, Secret, Signature, sign}; -use network::Error; -use network::IpFilter; +use rlp::{Rlp, RlpStream}; -use crate::node_table::*; +use crate::node_table::{NodeEndpoint, NodeId}; use crate::PROTOCOL_VERSION; -const ADDRESS_BYTES_SIZE: usize = 32; // Size of address type in bytes. -const ADDRESS_BITS: usize = 8 * ADDRESS_BYTES_SIZE; // Denoted by n in [Kademlia]. -const DISCOVERY_MAX_STEPS: u16 = 8; // Max iterations of discovery. (discover) -const BUCKET_SIZE: usize = 16; // Denoted by k in [Kademlia]. Number of nodes stored in each bucket. -const ALPHA: usize = 3; // Denoted by \alpha in [Kademlia]. Number of concurrent FindNode requests. +/// Maximum Node discovery packet size pub const MAX_DATAGRAM_SIZE: usize = 1280; +/// Size of address type in bytes. +const ADDRESS_BYTES_SIZE: usize = 32; +/// Denoted by n in [Kademlia]. +const ADDRESS_BITS: usize = 8 * ADDRESS_BYTES_SIZE; +/// Max iterations of discovery. (discover) +const DISCOVERY_MAX_STEPS: u16 = 8; +/// Denoted by k in [Kademlia]. Number of nodes stored in each bucket. +const BUCKET_SIZE: usize = 16; +/// Denoted by \alpha in [Kademlia]. Number of concurrent FindNode requests. +const ALPHA: usize = 3; + const PACKET_PING: u8 = 1; const PACKET_PONG: u8 = 2; const PACKET_FIND_NODE: u8 = 3; const PACKET_NEIGHBOURS: u8 = 4; -const PACKET_KIND_LEN: usize = 1; +/// Packet type +const PACKET_TYPE_LEN: usize = 1; const PING_TIMEOUT: Duration = Duration::from_millis(500); const FIND_NODE_TIMEOUT: Duration = Duration::from_secs(2); const EXPIRY_TIME: Duration = Duration::from_secs(20); -const MAX_NODES_PING: usize = 32; // Max nodes to add/ping at once +/// Max nodes to add/ping at once +const MAX_NODES_PING: usize = 32; const REQUEST_BACKOFF: [Duration; 4] = [ Duration::from_secs(1), Duration::from_secs(4), @@ -861,7 +867,7 @@ fn append_expiration(rlp: &mut RlpStream) { /// /// The packet format is: `hash | signature | payload | packet_type`, where the maximum packet length is 1280 bytes fn assemble_packet(packet_id: u8, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { - let packet_len = payload.len() + HASH_LEN + SIGNATURE_LEN + PACKET_KIND_LEN; + let packet_len = payload.len() + HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN; if !packet_has_valid_length(packet_len) { warn!(target: "discovery", "Invalid packet length: {}", packet_len); @@ -890,7 +896,7 @@ fn sign_payload_with_packet_id(secret: &Secret, payload_with_packet_id: &[u8]) - fn packet_has_valid_length(packet_len: usize) -> bool { // Q(niklasad1): why `4`? - if packet_len > MAX_DATAGRAM_SIZE || packet_len < HASH_LEN + SIGNATURE_LEN + PACKET_KIND_LEN + 4 { + if packet_len > MAX_DATAGRAM_SIZE || packet_len < HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN + 4 { false } else { true From d1f20a3cdc7b923ea52ade636b1cdb42aae94e10 Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 12 Mar 2020 18:57:47 +0100 Subject: [PATCH 4/8] [devp2p discovery]: introduce `PacketKind` Introduced to achieve more type-safety than u8. Part of this the verify `packet_kind` before doing more expensive operations such as `hashing`, `signing`, `ecc recover` and similar --- util/network-devp2p/src/discovery.rs | 94 +++++++++++++++++++--------- 1 file changed, 63 insertions(+), 31 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 1f61c5db72d..1dd4a56e3b5 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -16,6 +16,7 @@ use std::collections::{HashMap, HashSet, VecDeque}; use std::collections::hash_map::Entry; +use std::convert::{TryFrom, TryInto}; use std::net::SocketAddr; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; @@ -52,6 +53,29 @@ const PACKET_NEIGHBOURS: u8 = 4; /// Packet type const PACKET_TYPE_LEN: usize = 1; +/// Node discovery packet kinds +#[derive(Debug)] +enum PacketKind { + Ping = 1, + Pong = 2, + FindNode = 3, + Neighbours = 4, +} + +impl TryFrom for PacketKind { + type Error = Error; + + fn try_from(kind: u8) -> Result { + match kind { + PACKET_PING => Ok(Self::Ping), + PACKET_PONG => Ok(Self::Pong), + PACKET_FIND_NODE => Ok(Self::FindNode), + PACKET_NEIGHBOURS => Ok(Self::Neighbours), + _ => Err(Error::BadProtocol), + } + } +} + const PING_TIMEOUT: Duration = Duration::from_millis(500); const FIND_NODE_TIMEOUT: Duration = Duration::from_secs(2); const EXPIRY_TIME: Duration = Duration::from_secs(20); @@ -390,7 +414,7 @@ impl Discovery { node.endpoint.to_rlp_list(&mut rlp); append_expiration(&mut rlp); let old_parity_hash = keccak(rlp.as_raw()); - let hash = self.send_packet(PACKET_PING, node.endpoint.udp_address(), rlp.drain())?; + let hash = self.send_packet(PacketKind::Ping, node.endpoint.udp_address(), rlp.drain())?; self.in_flight_pings.insert(node.id, PingRequest { sent_at: Instant::now(), @@ -408,7 +432,7 @@ impl Discovery { let mut rlp = RlpStream::new_list(2); rlp.append(target); append_expiration(&mut rlp); - self.send_packet(PACKET_FIND_NODE, node.endpoint.udp_address(), rlp.drain())?; + self.send_packet(PacketKind::FindNode, node.endpoint.udp_address(), rlp.drain())?; self.in_flight_find_nodes.insert(node.id, FindNodeRequest { sent_at: Instant::now(), @@ -420,7 +444,7 @@ impl Discovery { Ok(()) } - fn send_packet(&mut self, packet_id: u8, address: SocketAddr, payload: Bytes) -> Result { + fn send_packet(&mut self, packet_id: PacketKind, address: SocketAddr, payload: Bytes) -> Result { let (packet, hash) = assemble_packet(packet_id, payload, &self.secret)?; self.send_to(packet, address); Ok(hash) @@ -482,30 +506,13 @@ impl Discovery { } pub fn on_packet(&mut self, packet: &[u8], from: SocketAddr) -> Result, Error> { - if !packet_has_valid_length(packet.len()) { - warn!(target: "discovery", "Invalid packet length: {}", packet.len()); - return Err(Error::BadProtocol); - } - - let hash_signed = keccak(&packet[HASH_LEN..]); - if hash_signed[..] != packet[0..32] { - return Err(Error::BadProtocol); - } - - let payload_with_packet_id = &packet[HASH_LEN + SIGNATURE_LEN..]; - let signature = H520::from_slice(&packet[HASH_LEN..HASH_LEN + SIGNATURE_LEN]); - let node_id = recover(&signature.into(), &keccak(payload_with_packet_id))?; - let packet_id = payload_with_packet_id[0]; - let rlp = Rlp::new(&payload_with_packet_id[1..]); + let (node_id, payload, packet_id, signed_hash) = disassemble_packet(packet)?; + let rlp = Rlp::new(payload); match packet_id { - PACKET_PING => self.on_ping(&rlp, node_id, from, hash_signed), - PACKET_PONG => self.on_pong(&rlp, node_id, from), - PACKET_FIND_NODE => self.on_find_node(&rlp, node_id, from), - PACKET_NEIGHBOURS => self.on_neighbours(&rlp, node_id, from), - _ => { - debug!(target: "discovery", "Unknown UDP packet: {}", packet_id); - Ok(None) - } + PacketKind::Ping => self.on_ping(&rlp, node_id, from, signed_hash), + PacketKind::Pong => self.on_pong(&rlp, node_id, from), + PacketKind::FindNode => self.on_find_node(&rlp, node_id, from), + PacketKind::Neighbours => self.on_neighbours(&rlp, node_id, from), } } @@ -555,7 +562,7 @@ impl Discovery { response.append(&echo_hash); append_expiration(&mut response); - self.send_packet(PACKET_PONG, from, response.drain())?; + self.send_packet(PacketKind::Pong, from, response.drain())?; let entry = NodeEntry { id: node_id, endpoint: pong_to }; if !entry.endpoint.is_valid_discovery_node() { @@ -688,7 +695,7 @@ impl Discovery { } let mut packets = Discovery::prepare_neighbours_packets(&nearest); for p in packets.drain(..) { - self.send_packet(PACKET_NEIGHBOURS, node.endpoint.address, p)?; + self.send_packet(PacketKind::Neighbours, node.endpoint.address, p)?; } trace!(target: "discovery", "Sent {} Neighbours to {:?}", nearest.len(), &node.endpoint); Ok(()) @@ -863,10 +870,10 @@ fn append_expiration(rlp: &mut RlpStream) { } -/// Helper function to assemble node discovery messages +/// Helper function to assemble node discovery packets /// /// The packet format is: `hash | signature | payload | packet_type`, where the maximum packet length is 1280 bytes -fn assemble_packet(packet_id: u8, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { +fn assemble_packet(packet_id: PacketKind, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { let packet_len = payload.len() + HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN; if !packet_has_valid_length(packet_len) { @@ -876,7 +883,7 @@ fn assemble_packet(packet_id: u8, payload: Bytes, secret: &Secret) -> Result<(By let mut packet = Bytes::with_capacity(packet_len); packet.resize(HASH_LEN + SIGNATURE_LEN, 0); // Filled in below - packet.push(packet_id); + packet.push(packet_id as u8); packet.extend(payload); let signature = sign_payload_with_packet_id(secret, &packet[HASH_LEN + SIGNATURE_LEN ..])?; @@ -886,6 +893,31 @@ fn assemble_packet(packet_id: u8, payload: Bytes, secret: &Secret) -> Result<(By Ok((packet, signed_hash)) } +/// Helper to disassemble node discovery packets +fn disassemble_packet(packet: &[u8]) -> Result<(NodeId, &[u8], PacketKind, H256), Error> { + if !packet_has_valid_length(packet.len()) { + warn!(target: "discovery", "Invalid packet length: {}", packet.len()); + return Err(Error::BadProtocol); + } + + let payload_with_packet_id = &packet[HASH_LEN + SIGNATURE_LEN..]; + let packet_kind = payload_with_packet_id[0]; + let packet_kind: PacketKind = packet_kind.try_into().map_err(|e| { + debug!(target: "discovery", "Unknown UDP packet: {:?}", packet_kind); + e + })?; + + let signed_hash = keccak(&packet[HASH_LEN..]); + if signed_hash[..] != packet[0..HASH_LEN] { + return Err(Error::BadProtocol); + } + + let signature = H520::from_slice(&packet[HASH_LEN..HASH_LEN + SIGNATURE_LEN]); + let node_id = recover(&signature.into(), &keccak(payload_with_packet_id))?; + + Ok((node_id, &payload_with_packet_id[1..], packet_kind, signed_hash)) +} + fn sign_payload_with_packet_id(secret: &Secret, payload_with_packet_id: &[u8]) -> Result { let hash = keccak(payload_with_packet_id); sign(secret, &hash).map_err(|e| { From 851fe0fd055bca18d3235c738eb5387959588b13 Mon Sep 17 00:00:00 2001 From: Niklas Date: Thu, 12 Mar 2020 22:01:52 +0100 Subject: [PATCH 5/8] fix test build --- util/network-devp2p/src/discovery.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 1dd4a56e3b5..7fdb8b95a23 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -1096,7 +1096,7 @@ mod tests { let key = Random.generate(); discovery.send_find_node(&node_entries[100], key.public()).unwrap(); for payload in Discovery::prepare_neighbours_packets(&node_entries[101..116]) { - let (packet, _hash) = assemble_packet(PACKET_NEIGHBOURS, payload, &key.secret()).unwrap(); + let (packet, _hash) = assemble_packet(PacketKind::Neighbours, payload, &key.secret()).unwrap(); discovery.on_packet(&packet, from).unwrap(); } @@ -1108,7 +1108,7 @@ mod tests { // FIND_NODE does not time out because it receives k results. discovery.send_find_node(&node_entries[100], key.public()).unwrap(); for payload in Discovery::prepare_neighbours_packets(&node_entries[101..117]) { - let (packet, _hash) = assemble_packet(PACKET_NEIGHBOURS, payload, &key.secret()).unwrap(); + let (packet, _hash) = assemble_packet(PacketKind::Neighbours, payload, &key.secret()).unwrap(); discovery.on_packet(&packet, from).unwrap(); } @@ -1342,7 +1342,7 @@ mod tests { incorrect_pong_rlp.append(&H256::zero()); append_expiration(&mut incorrect_pong_rlp); let (incorrect_pong_data, _hash) = assemble_packet( - PACKET_PONG, incorrect_pong_rlp.drain(), &discovery2.secret + PacketKind::Pong, incorrect_pong_rlp.drain(), &discovery2.secret ).unwrap(); if let Some(_) = discovery1.on_packet(&incorrect_pong_data, ep2.address).unwrap() { panic!("Expected no changes to discovery1's table because pong hash is incorrect"); @@ -1371,7 +1371,7 @@ mod tests { unexpected_pong_rlp.append(&H256::zero()); append_expiration(&mut unexpected_pong_rlp); let (unexpected_pong, _hash) = assemble_packet( - PACKET_PONG, unexpected_pong_rlp.drain(), key3.secret() + PacketKind::Pong, unexpected_pong_rlp.drain(), key3.secret() ).unwrap(); if let Some(_) = discovery1.on_packet(&unexpected_pong, ep3.address).unwrap() { panic!("Expected no changes to discovery1's table for unexpected pong"); From a869f4509324e49a84cb9779cfa4c0689baaae7a Mon Sep 17 00:00:00 2001 From: Niklas Date: Fri, 13 Mar 2020 11:16:47 +0100 Subject: [PATCH 6/8] fix nits --- util/network-devp2p/src/discovery.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 7fdb8b95a23..4c111d3b2ca 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -14,6 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Open Ethereum. If not, see . +//! Ethereum Node Discovery Protocol V4 + use std::collections::{HashMap, HashSet, VecDeque}; use std::collections::hash_map::Entry; use std::convert::{TryFrom, TryInto}; @@ -34,6 +36,9 @@ use crate::PROTOCOL_VERSION; /// Maximum Node discovery packet size pub const MAX_DATAGRAM_SIZE: usize = 1280; +/// Minimum node discovery packet size +// TODO(niklasad1): why 4? +const MIN_DATAGRAM_SIZE: usize = HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN + 4; /// Size of address type in bytes. const ADDRESS_BYTES_SIZE: usize = 32; @@ -869,15 +874,14 @@ fn append_expiration(rlp: &mut RlpStream) { rlp.append(×tamp); } - /// Helper function to assemble node discovery packets /// -/// The packet format is: `hash | signature | payload | packet_type`, where the maximum packet length is 1280 bytes +/// The packet format is: `hash || signature || packet_type || payload`, where the maximum packet length is 1280 bytes fn assemble_packet(packet_id: PacketKind, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { let packet_len = payload.len() + HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN; if !packet_has_valid_length(packet_len) { - warn!(target: "discovery", "Invalid packet length: {}", packet_len); + warn!(target: "discovery", "Ignored to write discovery packet with invalid packet length: {}, expected to be in range {} - {}", packet_len, MIN_DATAGRAM_SIZE, MAX_DATAGRAM_SIZE); return Err(Error::BadProtocol); } @@ -894,9 +898,11 @@ fn assemble_packet(packet_id: PacketKind, payload: Bytes, secret: &Secret) -> Re } /// Helper to disassemble node discovery packets +/// +/// The packet format is: `hash || signature || packet_type || payload`, where the maximum packet length is 1280 bytes fn disassemble_packet(packet: &[u8]) -> Result<(NodeId, &[u8], PacketKind, H256), Error> { if !packet_has_valid_length(packet.len()) { - warn!(target: "discovery", "Invalid packet length: {}", packet.len()); + warn!(target: "discovery", "Ignored to read discovery packet with invalid packet length: {}, expected to be in range {} - {}", packet.len(), MIN_DATAGRAM_SIZE, MAX_DATAGRAM_SIZE); return Err(Error::BadProtocol); } @@ -927,8 +933,7 @@ fn sign_payload_with_packet_id(secret: &Secret, payload_with_packet_id: &[u8]) - } fn packet_has_valid_length(packet_len: usize) -> bool { - // Q(niklasad1): why `4`? - if packet_len > MAX_DATAGRAM_SIZE || packet_len < HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN + 4 { + if packet_len > MAX_DATAGRAM_SIZE || packet_len < MIN_DATAGRAM_SIZE { false } else { true From 2a483251cc33d7bcbc91ce12c5025d0efb1e223f Mon Sep 17 00:00:00 2001 From: Niklas Date: Tue, 17 Mar 2020 11:38:50 +0100 Subject: [PATCH 7/8] PacketKind -> PacketType --- util/network-devp2p/src/discovery.rs | 41 ++++++++++++++-------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 4c111d3b2ca..6ba00811ac1 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -55,19 +55,20 @@ const PACKET_PING: u8 = 1; const PACKET_PONG: u8 = 2; const PACKET_FIND_NODE: u8 = 3; const PACKET_NEIGHBOURS: u8 = 4; -/// Packet type +/// The length of the packet type const PACKET_TYPE_LEN: usize = 1; /// Node discovery packet kinds +// TODO: add `ENR packets #[derive(Debug)] -enum PacketKind { +enum PacketType { Ping = 1, Pong = 2, FindNode = 3, Neighbours = 4, } -impl TryFrom for PacketKind { +impl TryFrom for PacketType { type Error = Error; fn try_from(kind: u8) -> Result { @@ -419,7 +420,7 @@ impl Discovery { node.endpoint.to_rlp_list(&mut rlp); append_expiration(&mut rlp); let old_parity_hash = keccak(rlp.as_raw()); - let hash = self.send_packet(PacketKind::Ping, node.endpoint.udp_address(), rlp.drain())?; + let hash = self.send_packet(PacketType::Ping, node.endpoint.udp_address(), rlp.drain())?; self.in_flight_pings.insert(node.id, PingRequest { sent_at: Instant::now(), @@ -437,7 +438,7 @@ impl Discovery { let mut rlp = RlpStream::new_list(2); rlp.append(target); append_expiration(&mut rlp); - self.send_packet(PacketKind::FindNode, node.endpoint.udp_address(), rlp.drain())?; + self.send_packet(PacketType::FindNode, node.endpoint.udp_address(), rlp.drain())?; self.in_flight_find_nodes.insert(node.id, FindNodeRequest { sent_at: Instant::now(), @@ -449,7 +450,7 @@ impl Discovery { Ok(()) } - fn send_packet(&mut self, packet_id: PacketKind, address: SocketAddr, payload: Bytes) -> Result { + fn send_packet(&mut self, packet_id: PacketType, address: SocketAddr, payload: Bytes) -> Result { let (packet, hash) = assemble_packet(packet_id, payload, &self.secret)?; self.send_to(packet, address); Ok(hash) @@ -514,10 +515,10 @@ impl Discovery { let (node_id, payload, packet_id, signed_hash) = disassemble_packet(packet)?; let rlp = Rlp::new(payload); match packet_id { - PacketKind::Ping => self.on_ping(&rlp, node_id, from, signed_hash), - PacketKind::Pong => self.on_pong(&rlp, node_id, from), - PacketKind::FindNode => self.on_find_node(&rlp, node_id, from), - PacketKind::Neighbours => self.on_neighbours(&rlp, node_id, from), + PacketType::Ping => self.on_ping(&rlp, node_id, from, signed_hash), + PacketType::Pong => self.on_pong(&rlp, node_id, from), + PacketType::FindNode => self.on_find_node(&rlp, node_id, from), + PacketType::Neighbours => self.on_neighbours(&rlp, node_id, from), } } @@ -567,7 +568,7 @@ impl Discovery { response.append(&echo_hash); append_expiration(&mut response); - self.send_packet(PacketKind::Pong, from, response.drain())?; + self.send_packet(PacketType::Pong, from, response.drain())?; let entry = NodeEntry { id: node_id, endpoint: pong_to }; if !entry.endpoint.is_valid_discovery_node() { @@ -700,7 +701,7 @@ impl Discovery { } let mut packets = Discovery::prepare_neighbours_packets(&nearest); for p in packets.drain(..) { - self.send_packet(PacketKind::Neighbours, node.endpoint.address, p)?; + self.send_packet(PacketType::Neighbours, node.endpoint.address, p)?; } trace!(target: "discovery", "Sent {} Neighbours to {:?}", nearest.len(), &node.endpoint); Ok(()) @@ -877,7 +878,7 @@ fn append_expiration(rlp: &mut RlpStream) { /// Helper function to assemble node discovery packets /// /// The packet format is: `hash || signature || packet_type || payload`, where the maximum packet length is 1280 bytes -fn assemble_packet(packet_id: PacketKind, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { +fn assemble_packet(packet_id: PacketType, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { let packet_len = payload.len() + HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN; if !packet_has_valid_length(packet_len) { @@ -900,7 +901,7 @@ fn assemble_packet(packet_id: PacketKind, payload: Bytes, secret: &Secret) -> Re /// Helper to disassemble node discovery packets /// /// The packet format is: `hash || signature || packet_type || payload`, where the maximum packet length is 1280 bytes -fn disassemble_packet(packet: &[u8]) -> Result<(NodeId, &[u8], PacketKind, H256), Error> { +fn disassemble_packet(packet: &[u8]) -> Result<(NodeId, &[u8], PacketType, H256), Error> { if !packet_has_valid_length(packet.len()) { warn!(target: "discovery", "Ignored to read discovery packet with invalid packet length: {}, expected to be in range {} - {}", packet.len(), MIN_DATAGRAM_SIZE, MAX_DATAGRAM_SIZE); return Err(Error::BadProtocol); @@ -908,8 +909,8 @@ fn disassemble_packet(packet: &[u8]) -> Result<(NodeId, &[u8], PacketKind, H256) let payload_with_packet_id = &packet[HASH_LEN + SIGNATURE_LEN..]; let packet_kind = payload_with_packet_id[0]; - let packet_kind: PacketKind = packet_kind.try_into().map_err(|e| { - debug!(target: "discovery", "Unknown UDP packet: {:?}", packet_kind); + let packet_kind: PacketType = packet_kind.try_into().map_err(|e| { + debug!(target: "discovery", "Unknown discovery packet id: {:?}", packet_kind); e })?; @@ -1101,7 +1102,7 @@ mod tests { let key = Random.generate(); discovery.send_find_node(&node_entries[100], key.public()).unwrap(); for payload in Discovery::prepare_neighbours_packets(&node_entries[101..116]) { - let (packet, _hash) = assemble_packet(PacketKind::Neighbours, payload, &key.secret()).unwrap(); + let (packet, _hash) = assemble_packet(PacketType::Neighbours, payload, &key.secret()).unwrap(); discovery.on_packet(&packet, from).unwrap(); } @@ -1113,7 +1114,7 @@ mod tests { // FIND_NODE does not time out because it receives k results. discovery.send_find_node(&node_entries[100], key.public()).unwrap(); for payload in Discovery::prepare_neighbours_packets(&node_entries[101..117]) { - let (packet, _hash) = assemble_packet(PacketKind::Neighbours, payload, &key.secret()).unwrap(); + let (packet, _hash) = assemble_packet(PacketType::Neighbours, payload, &key.secret()).unwrap(); discovery.on_packet(&packet, from).unwrap(); } @@ -1347,7 +1348,7 @@ mod tests { incorrect_pong_rlp.append(&H256::zero()); append_expiration(&mut incorrect_pong_rlp); let (incorrect_pong_data, _hash) = assemble_packet( - PacketKind::Pong, incorrect_pong_rlp.drain(), &discovery2.secret + PacketType::Pong, incorrect_pong_rlp.drain(), &discovery2.secret ).unwrap(); if let Some(_) = discovery1.on_packet(&incorrect_pong_data, ep2.address).unwrap() { panic!("Expected no changes to discovery1's table because pong hash is incorrect"); @@ -1376,7 +1377,7 @@ mod tests { unexpected_pong_rlp.append(&H256::zero()); append_expiration(&mut unexpected_pong_rlp); let (unexpected_pong, _hash) = assemble_packet( - PacketKind::Pong, unexpected_pong_rlp.drain(), key3.secret() + PacketType::Pong, unexpected_pong_rlp.drain(), key3.secret() ).unwrap(); if let Some(_) = discovery1.on_packet(&unexpected_pong, ep3.address).unwrap() { panic!("Expected no changes to discovery1's table for unexpected pong"); From 71e4e9d511c3317aa41f3439b6bc1f2b5a06a950 Mon Sep 17 00:00:00 2001 From: Niklas Date: Wed, 18 Mar 2020 14:56:41 +0100 Subject: [PATCH 8/8] fix nits --- util/network-devp2p/src/discovery.rs | 83 ++++++++++++++-------------- 1 file changed, 42 insertions(+), 41 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 71bac485a93..19a7f0b76b6 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -38,9 +38,13 @@ use crate::PROTOCOL_VERSION; pub const MAX_DATAGRAM_SIZE: usize = 1280; /// Minimum node discovery packet size // TODO(niklasad1): why 4? -const MIN_DATAGRAM_SIZE: usize = HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN + 4; +const MIN_DATAGRAM_SIZE: usize = HEADER_SIZE + 4; -/// Size of address type in bytes. +/// Size of the `hash` and `signature` in bytes (denoted MAC) +const HEADER_MAC_SIZE: usize = ADDRESS_BYTES_SIZE + SIGNATURE_BYTES_LEN; +/// Size of the Node discovery wire protocol header +const HEADER_SIZE: usize = HEADER_MAC_SIZE + PACKET_TYPE_BYTES_LEN; +/// Size of address in bytes. const ADDRESS_BYTES_SIZE: usize = 32; /// Denoted by n in [Kademlia]. const ADDRESS_BITS: usize = 8 * ADDRESS_BYTES_SIZE; @@ -55,11 +59,29 @@ const PACKET_PING: u8 = 1; const PACKET_PONG: u8 = 2; const PACKET_FIND_NODE: u8 = 3; const PACKET_NEIGHBOURS: u8 = 4; -/// The length of the packet type -const PACKET_TYPE_LEN: usize = 1; +/// The length of the packet type in bytes +const PACKET_TYPE_BYTES_LEN: usize = 1; + +/// Length of `Ethereum signature` in number of bytes +const SIGNATURE_BYTES_LEN: usize = 65; +const PING_TIMEOUT: Duration = Duration::from_millis(500); +const FIND_NODE_TIMEOUT: Duration = Duration::from_secs(2); +const EXPIRY_TIME: Duration = Duration::from_secs(20); +/// Max nodes to add/ping at once +const MAX_NODES_PING: usize = 32; +const REQUEST_BACKOFF: [Duration; 4] = [ + Duration::from_secs(1), + Duration::from_secs(4), + Duration::from_secs(16), + Duration::from_secs(64) +]; + +const NODE_LAST_SEEN_TIMEOUT: Duration = Duration::from_secs(24 * 60 * 60); +const OBSERVED_NODES_MAX_SIZE: usize = 10_000; + /// Node discovery packet kinds -// TODO: add `ENR packets +// TODO: Add support for `Node Discovery v4 ENR Extension` #[derive(Debug)] enum PacketType { Ping = 1, @@ -82,23 +104,6 @@ impl TryFrom for PacketType { } } -const PING_TIMEOUT: Duration = Duration::from_millis(500); -const FIND_NODE_TIMEOUT: Duration = Duration::from_secs(2); -const EXPIRY_TIME: Duration = Duration::from_secs(20); -/// Max nodes to add/ping at once -const MAX_NODES_PING: usize = 32; -const REQUEST_BACKOFF: [Duration; 4] = [ - Duration::from_secs(1), - Duration::from_secs(4), - Duration::from_secs(16), - Duration::from_secs(64) -]; - -const HASH_LEN: usize = 32; -const SIGNATURE_LEN: usize = 65; -const NODE_LAST_SEEN_TIMEOUT: Duration = Duration::from_secs(24 * 60 * 60); -const OBSERVED_NODES_MAX_SIZE: usize = 10_000; - #[derive(Clone, Debug)] pub struct NodeEntry { pub id: NodeId, @@ -855,7 +860,7 @@ fn append_expiration(rlp: &mut RlpStream) { /// /// The packet format is: `hash || signature || packet_type || payload`, where the maximum packet length is 1280 bytes fn assemble_packet(packet_id: PacketType, payload: Bytes, secret: &Secret) -> Result<(Bytes, H256), Error> { - let packet_len = payload.len() + HASH_LEN + SIGNATURE_LEN + PACKET_TYPE_LEN; + let packet_len = payload.len() + HEADER_SIZE; if !packet_has_valid_length(packet_len) { warn!(target: "discovery", "Ignored to write discovery packet with invalid packet length: {}, expected to be in range {} - {}", packet_len, MIN_DATAGRAM_SIZE, MAX_DATAGRAM_SIZE); @@ -863,14 +868,14 @@ fn assemble_packet(packet_id: PacketType, payload: Bytes, secret: &Secret) -> Re } let mut packet = Bytes::with_capacity(packet_len); - packet.resize(HASH_LEN + SIGNATURE_LEN, 0); // Filled in below + packet.resize(HEADER_MAC_SIZE, 0); // Filled in below packet.push(packet_id as u8); packet.extend(payload); - let signature = sign_payload_with_packet_id(secret, &packet[HASH_LEN + SIGNATURE_LEN ..])?; - packet[HASH_LEN..(HASH_LEN + SIGNATURE_LEN)].copy_from_slice(&signature[..]); - let signed_hash = keccak(&packet[HASH_LEN..]); - packet[..HASH_LEN].copy_from_slice(signed_hash.as_bytes()); + let signature = sign_payload_with_packet_id(secret, &packet[HEADER_MAC_SIZE..])?; + packet[ADDRESS_BYTES_SIZE..HEADER_MAC_SIZE].copy_from_slice(&signature[..]); + let signed_hash = keccak(&packet[ADDRESS_BYTES_SIZE..]); + packet[..ADDRESS_BYTES_SIZE].copy_from_slice(signed_hash.as_bytes()); Ok((packet, signed_hash)) } @@ -883,22 +888,22 @@ fn disassemble_packet(packet: &[u8]) -> Result<(NodeId, &[u8], PacketType, H256) return Err(Error::BadProtocol); } - let payload_with_packet_id = &packet[HASH_LEN + SIGNATURE_LEN..]; - let packet_kind = payload_with_packet_id[0]; - let packet_kind: PacketType = packet_kind.try_into().map_err(|e| { - debug!(target: "discovery", "Unknown discovery packet id: {:?}", packet_kind); + let payload_with_packet_id = &packet[HEADER_MAC_SIZE..]; + let packet_id = payload_with_packet_id[0]; + let packet_id: PacketType = packet_id.try_into().map_err(|e| { + warn!(target: "discovery", "Unknown discovery packet id: {:?}", packet_id); e })?; - let signed_hash = keccak(&packet[HASH_LEN..]); - if signed_hash[..] != packet[0..HASH_LEN] { + let signed_hash = keccak(&packet[ADDRESS_BYTES_SIZE..]); + if signed_hash[..] != packet[0..ADDRESS_BYTES_SIZE] { return Err(Error::BadProtocol); } - let signature = H520::from_slice(&packet[HASH_LEN..HASH_LEN + SIGNATURE_LEN]); + let signature = H520::from_slice(&packet[ADDRESS_BYTES_SIZE..HEADER_MAC_SIZE]); let node_id = recover(&signature.into(), &keccak(payload_with_packet_id))?; - Ok((node_id, &payload_with_packet_id[1..], packet_kind, signed_hash)) + Ok((node_id, &payload_with_packet_id[1..], packet_id, signed_hash)) } fn sign_payload_with_packet_id(secret: &Secret, payload_with_packet_id: &[u8]) -> Result { @@ -910,11 +915,7 @@ fn sign_payload_with_packet_id(secret: &Secret, payload_with_packet_id: &[u8]) - } fn packet_has_valid_length(packet_len: usize) -> bool { - if packet_len > MAX_DATAGRAM_SIZE || packet_len < MIN_DATAGRAM_SIZE { - false - } else { - true - } + packet_len >= MIN_DATAGRAM_SIZE && packet_len <= MAX_DATAGRAM_SIZE } // Selects the next node in a bucket to ping. Chooses the eligible node least recently seen.