From f11884ccdb1846a68fa0e85f4107eaec1b2bf505 Mon Sep 17 00:00:00 2001 From: Divma Date: Tue, 3 Oct 2023 23:59:34 +0000 Subject: [PATCH] enforce non zero enr ports (#4776) ## Issue Addressed Right now lighthouse accepts zero as enr ports. Since enr ports should be reachable, zero ports should be rejected here ## Proposed Changes - update the config to use `NonZerou16` as an ENR port for all enr-related fields. - the enr builder from config now sets the enr to the listening port only if the enr port is not already set (prev behaviour) and the listening port is not zero (new behaviour) - reject zero listening ports when used with `enr-match`. - boot node now rejects listening port as zero, since those are advertised. - generate-bootnode-enr also rejected zero listening ports for the same reason. - update local network scripts ## Additional Info Unrelated, but why do we overwrite `enr-x-port` values with listening ports if `enr-match` is present? we prob should only do this for enr values that are not already set. --- beacon_node/lighthouse_network/src/config.rs | 13 ++-- .../lighthouse_network/src/discovery/enr.rs | 50 +++++++----- beacon_node/src/config.rs | 40 ++++++---- boot_node/src/config.rs | 26 ++++--- lcli/src/generate_bootnode_enr.rs | 6 +- lighthouse/tests/beacon_node.rs | 77 +++++++++++++++---- testing/simulator/src/local_network.rs | 14 ++-- 7 files changed, 155 insertions(+), 71 deletions(-) diff --git a/beacon_node/lighthouse_network/src/config.rs b/beacon_node/lighthouse_network/src/config.rs index c507744882..4226046396 100644 --- a/beacon_node/lighthouse_network/src/config.rs +++ b/beacon_node/lighthouse_network/src/config.rs @@ -11,6 +11,7 @@ use libp2p::Multiaddr; use serde_derive::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::net::{Ipv4Addr, Ipv6Addr}; +use std::num::NonZeroU16; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; @@ -64,22 +65,22 @@ pub struct Config { pub enr_address: (Option, Option), /// The udp ipv4 port to broadcast to peers in order to reach back for discovery. - pub enr_udp4_port: Option, + pub enr_udp4_port: Option, /// The quic ipv4 port to broadcast to peers in order to reach back for libp2p services. - pub enr_quic4_port: Option, + pub enr_quic4_port: Option, /// The tcp ipv4 port to broadcast to peers in order to reach back for libp2p services. - pub enr_tcp4_port: Option, + pub enr_tcp4_port: Option, /// The udp ipv6 port to broadcast to peers in order to reach back for discovery. - pub enr_udp6_port: Option, + pub enr_udp6_port: Option, /// The tcp ipv6 port to broadcast to peers in order to reach back for libp2p services. - pub enr_tcp6_port: Option, + pub enr_tcp6_port: Option, /// The quic ipv6 port to broadcast to peers in order to reach back for libp2p services. - pub enr_quic6_port: Option, + pub enr_quic6_port: Option, /// Target number of connected peers. pub target_peers: usize, diff --git a/beacon_node/lighthouse_network/src/discovery/enr.rs b/beacon_node/lighthouse_network/src/discovery/enr.rs index 3f46285a80..8eacabb4d0 100644 --- a/beacon_node/lighthouse_network/src/discovery/enr.rs +++ b/beacon_node/lighthouse_network/src/discovery/enr.rs @@ -158,11 +158,11 @@ pub fn create_enr_builder_from_config( } if let Some(udp4_port) = config.enr_udp4_port { - builder.udp4(udp4_port); + builder.udp4(udp4_port.get()); } if let Some(udp6_port) = config.enr_udp6_port { - builder.udp6(udp6_port); + builder.udp6(udp6_port.get()); } if enable_libp2p { @@ -171,35 +171,45 @@ pub fn create_enr_builder_from_config( // the related fields should only be added when both QUIC and libp2p are enabled if !config.disable_quic_support { // If we are listening on ipv4, add the quic ipv4 port. - if let Some(quic4_port) = config - .enr_quic4_port - .or_else(|| config.listen_addrs().v4().map(|v4_addr| v4_addr.quic_port)) - { - builder.add_value(QUIC_ENR_KEY, &quic4_port); + if let Some(quic4_port) = config.enr_quic4_port.or_else(|| { + config + .listen_addrs() + .v4() + .and_then(|v4_addr| v4_addr.quic_port.try_into().ok()) + }) { + builder.add_value(QUIC_ENR_KEY, &quic4_port.get()); } // If we are listening on ipv6, add the quic ipv6 port. - if let Some(quic6_port) = config - .enr_quic6_port - .or_else(|| config.listen_addrs().v6().map(|v6_addr| v6_addr.quic_port)) - { - builder.add_value(QUIC6_ENR_KEY, &quic6_port); + if let Some(quic6_port) = config.enr_quic6_port.or_else(|| { + config + .listen_addrs() + .v6() + .and_then(|v6_addr| v6_addr.quic_port.try_into().ok()) + }) { + builder.add_value(QUIC6_ENR_KEY, &quic6_port.get()); } } // If the ENR port is not set, and we are listening over that ip version, use the listening port instead. - let tcp4_port = config - .enr_tcp4_port - .or_else(|| config.listen_addrs().v4().map(|v4_addr| v4_addr.tcp_port)); + let tcp4_port = config.enr_tcp4_port.or_else(|| { + config + .listen_addrs() + .v4() + .and_then(|v4_addr| v4_addr.tcp_port.try_into().ok()) + }); if let Some(tcp4_port) = tcp4_port { - builder.tcp4(tcp4_port); + builder.tcp4(tcp4_port.get()); } - let tcp6_port = config - .enr_tcp6_port - .or_else(|| config.listen_addrs().v6().map(|v6_addr| v6_addr.tcp_port)); + let tcp6_port = config.enr_tcp6_port.or_else(|| { + config + .listen_addrs() + .v6() + .and_then(|v6_addr| v6_addr.tcp_port.try_into().ok()) + }); if let Some(tcp6_port) = tcp6_port { - builder.tcp6(tcp6_port); + builder.tcp6(tcp6_port.get()); } } builder diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 4ab92a7fd4..48b4a8f0d8 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -21,6 +21,7 @@ use std::fmt::Debug; use std::fs; use std::net::Ipv6Addr; use std::net::{IpAddr, Ipv4Addr, ToSocketAddrs}; +use std::num::NonZeroU16; use std::path::{Path, PathBuf}; use std::str::FromStr; use std::time::Duration; @@ -1178,23 +1179,23 @@ pub fn set_network_config( if let Some(enr_udp_port_str) = cli_args.value_of("enr-udp-port") { config.enr_udp4_port = Some( enr_udp_port_str - .parse::() - .map_err(|_| format!("Invalid discovery port: {}", enr_udp_port_str))?, + .parse::() + .map_err(|_| format!("Invalid ENR discovery port: {}", enr_udp_port_str))?, ); } if let Some(enr_quic_port_str) = cli_args.value_of("enr-quic-port") { config.enr_quic4_port = Some( enr_quic_port_str - .parse::() - .map_err(|_| format!("Invalid quic port: {}", enr_quic_port_str))?, + .parse::() + .map_err(|_| format!("Invalid ENR quic port: {}", enr_quic_port_str))?, ); } if let Some(enr_tcp_port_str) = cli_args.value_of("enr-tcp-port") { config.enr_tcp4_port = Some( enr_tcp_port_str - .parse::() + .parse::() .map_err(|_| format!("Invalid ENR TCP port: {}", enr_tcp_port_str))?, ); } @@ -1202,23 +1203,23 @@ pub fn set_network_config( if let Some(enr_udp_port_str) = cli_args.value_of("enr-udp6-port") { config.enr_udp6_port = Some( enr_udp_port_str - .parse::() - .map_err(|_| format!("Invalid discovery port: {}", enr_udp_port_str))?, + .parse::() + .map_err(|_| format!("Invalid ENR discovery port: {}", enr_udp_port_str))?, ); } if let Some(enr_quic_port_str) = cli_args.value_of("enr-quic6-port") { config.enr_quic6_port = Some( enr_quic_port_str - .parse::() - .map_err(|_| format!("Invalid quic port: {}", enr_quic_port_str))?, + .parse::() + .map_err(|_| format!("Invalid ENR quic port: {}", enr_quic_port_str))?, ); } if let Some(enr_tcp_port_str) = cli_args.value_of("enr-tcp6-port") { config.enr_tcp6_port = Some( enr_tcp_port_str - .parse::() + .parse::() .map_err(|_| format!("Invalid ENR TCP port: {}", enr_tcp_port_str))?, ); } @@ -1226,25 +1227,38 @@ pub fn set_network_config( if cli_args.is_present("enr-match") { // Match the IP and UDP port in the ENR. - // Set the ENR address to localhost if the address is unspecified. if let Some(ipv4_addr) = config.listen_addrs().v4().cloned() { + // ensure the port is valid to be advertised + let disc_port = ipv4_addr + .disc_port + .try_into() + .map_err(|_| "enr-match can only be used with non-zero listening ports")?; + + // Set the ENR address to localhost if the address is unspecified. let ipv4_enr_addr = if ipv4_addr.addr == Ipv4Addr::UNSPECIFIED { Ipv4Addr::LOCALHOST } else { ipv4_addr.addr }; config.enr_address.0 = Some(ipv4_enr_addr); - config.enr_udp4_port = Some(ipv4_addr.disc_port); + config.enr_udp4_port = Some(disc_port); } if let Some(ipv6_addr) = config.listen_addrs().v6().cloned() { + // ensure the port is valid to be advertised + let disc_port = ipv6_addr + .disc_port + .try_into() + .map_err(|_| "enr-match can only be used with non-zero listening ports")?; + + // Set the ENR address to localhost if the address is unspecified. let ipv6_enr_addr = if ipv6_addr.addr == Ipv6Addr::UNSPECIFIED { Ipv6Addr::LOCALHOST } else { ipv6_addr.addr }; config.enr_address.1 = Some(ipv6_enr_addr); - config.enr_udp6_port = Some(ipv6_addr.disc_port); + config.enr_udp6_port = Some(disc_port); } } diff --git a/boot_node/src/config.rs b/boot_node/src/config.rs index 5d7853bd24..d435efc6f5 100644 --- a/boot_node/src/config.rs +++ b/boot_node/src/config.rs @@ -60,19 +60,25 @@ impl BootNodeConfig { // Set the Enr Discovery ports to the listening ports if not present. if let Some(listening_addr_v4) = network_config.listen_addrs().v4() { - network_config.enr_udp4_port = Some( - network_config - .enr_udp4_port - .unwrap_or(listening_addr_v4.disc_port), - ) + if network_config.enr_udp4_port.is_none() { + network_config.enr_udp4_port = + Some(network_config.enr_udp4_port.unwrap_or( + listening_addr_v4.disc_port.try_into().map_err(|_| { + "boot node enr-udp-port not set and listening port is zero" + })?, + )) + } }; if let Some(listening_addr_v6) = network_config.listen_addrs().v6() { - network_config.enr_udp6_port = Some( - network_config - .enr_udp6_port - .unwrap_or(listening_addr_v6.disc_port), - ) + if network_config.enr_udp6_port.is_none() { + network_config.enr_udp6_port = + Some(network_config.enr_udp6_port.unwrap_or( + listening_addr_v6.disc_port.try_into().map_err(|_| { + "boot node enr-udp-port not set and listening port is zero" + })?, + )) + } }; // By default this is enabled. If it is not set, revert to false. diff --git a/lcli/src/generate_bootnode_enr.rs b/lcli/src/generate_bootnode_enr.rs index 0584cd6549..1d41bedc88 100644 --- a/lcli/src/generate_bootnode_enr.rs +++ b/lcli/src/generate_bootnode_enr.rs @@ -4,16 +4,16 @@ use lighthouse_network::{ libp2p::identity::secp256k1, NetworkConfig, NETWORK_KEY_FILENAME, }; -use std::fs::File; use std::io::Write; use std::path::PathBuf; use std::{fs, net::Ipv4Addr}; +use std::{fs::File, num::NonZeroU16}; use types::{ChainSpec, EnrForkId, Epoch, EthSpec, Hash256}; pub fn run(matches: &ArgMatches) -> Result<(), String> { let ip: Ipv4Addr = clap_utils::parse_required(matches, "ip")?; - let udp_port: u16 = clap_utils::parse_required(matches, "udp-port")?; - let tcp_port: u16 = clap_utils::parse_required(matches, "tcp-port")?; + let udp_port: NonZeroU16 = clap_utils::parse_required(matches, "udp-port")?; + let tcp_port: NonZeroU16 = clap_utils::parse_required(matches, "tcp-port")?; let output_dir: PathBuf = clap_utils::parse_required(matches, "output-dir")?; let genesis_fork_version: [u8; 4] = clap_utils::parse_ssz_required(matches, "genesis-fork-version")?; diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 4140a3f6b4..4ed2e9c2c8 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -1274,7 +1274,12 @@ fn enr_udp_port_flag() { CommandLineTest::new() .flag("enr-udp-port", Some(port.to_string().as_str())) .run_with_zero_port() - .with_config(|config| assert_eq!(config.network.enr_udp4_port, Some(port))); + .with_config(|config| { + assert_eq!( + config.network.enr_udp4_port.map(|port| port.get()), + Some(port) + ) + }); } #[test] fn enr_quic_port_flag() { @@ -1282,7 +1287,12 @@ fn enr_quic_port_flag() { CommandLineTest::new() .flag("enr-quic-port", Some(port.to_string().as_str())) .run_with_zero_port() - .with_config(|config| assert_eq!(config.network.enr_quic4_port, Some(port))); + .with_config(|config| { + assert_eq!( + config.network.enr_quic4_port.map(|port| port.get()), + Some(port) + ) + }); } #[test] fn enr_tcp_port_flag() { @@ -1290,7 +1300,12 @@ fn enr_tcp_port_flag() { CommandLineTest::new() .flag("enr-tcp-port", Some(port.to_string().as_str())) .run_with_zero_port() - .with_config(|config| assert_eq!(config.network.enr_tcp4_port, Some(port))); + .with_config(|config| { + assert_eq!( + config.network.enr_tcp4_port.map(|port| port.get()), + Some(port) + ) + }); } #[test] fn enr_udp6_port_flag() { @@ -1298,7 +1313,12 @@ fn enr_udp6_port_flag() { CommandLineTest::new() .flag("enr-udp6-port", Some(port.to_string().as_str())) .run_with_zero_port() - .with_config(|config| assert_eq!(config.network.enr_udp6_port, Some(port))); + .with_config(|config| { + assert_eq!( + config.network.enr_udp6_port.map(|port| port.get()), + Some(port) + ) + }); } #[test] fn enr_quic6_port_flag() { @@ -1306,7 +1326,12 @@ fn enr_quic6_port_flag() { CommandLineTest::new() .flag("enr-quic6-port", Some(port.to_string().as_str())) .run_with_zero_port() - .with_config(|config| assert_eq!(config.network.enr_quic6_port, Some(port))); + .with_config(|config| { + assert_eq!( + config.network.enr_quic6_port.map(|port| port.get()), + Some(port) + ) + }); } #[test] fn enr_tcp6_port_flag() { @@ -1314,7 +1339,12 @@ fn enr_tcp6_port_flag() { CommandLineTest::new() .flag("enr-tcp6-port", Some(port.to_string().as_str())) .run_with_zero_port() - .with_config(|config| assert_eq!(config.network.enr_tcp6_port, Some(port))); + .with_config(|config| { + assert_eq!( + config.network.enr_tcp6_port.map(|port| port.get()), + Some(port) + ) + }); } #[test] fn enr_match_flag_over_ipv4() { @@ -1340,7 +1370,10 @@ fn enr_match_flag_over_ipv4() { Some((addr, udp4_port, tcp4_port)) ); assert_eq!(config.network.enr_address, (Some(addr), None)); - assert_eq!(config.network.enr_udp4_port, Some(udp4_port)); + assert_eq!( + config.network.enr_udp4_port.map(|port| port.get()), + Some(udp4_port) + ); }); } #[test] @@ -1368,7 +1401,10 @@ fn enr_match_flag_over_ipv6() { Some((addr, udp6_port, tcp6_port)) ); assert_eq!(config.network.enr_address, (None, Some(addr))); - assert_eq!(config.network.enr_udp6_port, Some(udp6_port)); + assert_eq!( + config.network.enr_udp6_port.map(|port| port.get()), + Some(udp6_port) + ); }); } #[test] @@ -1416,8 +1452,14 @@ fn enr_match_flag_over_ipv4_and_ipv6() { config.network.enr_address, (Some(ipv4_addr), Some(ipv6_addr)) ); - assert_eq!(config.network.enr_udp6_port, Some(udp6_port)); - assert_eq!(config.network.enr_udp4_port, Some(udp4_port)); + assert_eq!( + config.network.enr_udp6_port.map(|port| port.get()), + Some(udp6_port) + ); + assert_eq!( + config.network.enr_udp4_port.map(|port| port.get()), + Some(udp4_port) + ); }); } #[test] @@ -1430,7 +1472,10 @@ fn enr_address_flag_with_ipv4() { .run_with_zero_port() .with_config(|config| { assert_eq!(config.network.enr_address, (Some(addr), None)); - assert_eq!(config.network.enr_udp4_port, Some(port)); + assert_eq!( + config.network.enr_udp4_port.map(|port| port.get()), + Some(port) + ); }); } #[test] @@ -1443,7 +1488,10 @@ fn enr_address_flag_with_ipv6() { .run_with_zero_port() .with_config(|config| { assert_eq!(config.network.enr_address, (Some(addr), None)); - assert_eq!(config.network.enr_udp4_port, Some(port)); + assert_eq!( + config.network.enr_udp4_port.map(|port| port.get()), + Some(port) + ); }); } #[test] @@ -1460,7 +1508,10 @@ fn enr_address_dns_flag() { config.network.enr_address.0 == Some(addr) || config.network.enr_address.1 == Some(ipv6addr) ); - assert_eq!(config.network.enr_udp4_port, Some(port)); + assert_eq!( + config.network.enr_udp4_port.map(|port| port.get()), + Some(port) + ); }); } #[test] diff --git a/testing/simulator/src/local_network.rs b/testing/simulator/src/local_network.rs index 69fa8ded02..1024c46e49 100644 --- a/testing/simulator/src/local_network.rs +++ b/testing/simulator/src/local_network.rs @@ -66,8 +66,8 @@ impl LocalNetwork { BOOTNODE_PORT, QUIC_PORT, ); - beacon_config.network.enr_udp4_port = Some(BOOTNODE_PORT); - beacon_config.network.enr_tcp4_port = Some(BOOTNODE_PORT); + beacon_config.network.enr_udp4_port = Some(BOOTNODE_PORT.try_into().expect("non zero")); + beacon_config.network.enr_tcp4_port = Some(BOOTNODE_PORT.try_into().expect("non zero")); beacon_config.network.discv5_config.table_filter = |_| true; let execution_node = if let Some(el_config) = &mut beacon_config.execution_layer { @@ -152,14 +152,16 @@ impl LocalNetwork { .expect("bootnode must have a network"), ); let count = (self.beacon_node_count() + self.proposer_node_count()) as u16; + let libp2p_tcp_port = BOOTNODE_PORT + count; + let discv5_port = BOOTNODE_PORT + count; beacon_config.network.set_ipv4_listening_address( std::net::Ipv4Addr::UNSPECIFIED, - BOOTNODE_PORT + count, - BOOTNODE_PORT + count, + libp2p_tcp_port, + discv5_port, QUIC_PORT + count, ); - beacon_config.network.enr_udp4_port = Some(BOOTNODE_PORT + count); - beacon_config.network.enr_tcp4_port = Some(BOOTNODE_PORT + count); + beacon_config.network.enr_udp4_port = Some(discv5_port.try_into().unwrap()); + beacon_config.network.enr_tcp4_port = Some(libp2p_tcp_port.try_into().unwrap()); beacon_config.network.discv5_config.table_filter = |_| true; beacon_config.network.proposer_only = is_proposer; }