From 2bc858cde351cb299c8986fd1045da6d832acbdb Mon Sep 17 00:00:00 2001 From: Mark Sinclair Date: Fri, 27 Aug 2021 09:32:26 +0100 Subject: [PATCH 1/3] explorer-api: port test: split out address resolution and add units tests --- explorer-api/src/ping/http.rs | 103 +++++++++++++++++++++++++--------- 1 file changed, 76 insertions(+), 27 deletions(-) diff --git a/explorer-api/src/ping/http.rs b/explorer-api/src/ping/http.rs index 248db7874d..30cac9702e 100644 --- a/explorer-api/src/ping/http.rs +++ b/explorer-api/src/ping/http.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::net::ToSocketAddrs; +use std::net::{SocketAddr, ToSocketAddrs}; use std::time::Duration; use rocket::serde::json::Json; @@ -80,32 +80,81 @@ async fn port_check(bond: &MixNodeBond) -> HashMap { ports } -async fn do_port_check(host: &str, port: u16) -> bool { - let addr = format!("{}:{}", host, port) - .to_socket_addrs() - .unwrap() - .next() - .unwrap(); - match tokio::time::timeout( - CONNECTION_TIMEOUT_SECONDS, - tokio::net::TcpStream::connect(addr), - ) - .await - { - Ok(Ok(_stream)) => { - // didn't timeout and tcp stream is open - trace!("Successfully pinged {}", addr); - true - } - Ok(Err(_stream_err)) => { - error!("{} ping failed {:}", addr, _stream_err); - // didn't timeout but couldn't open tcp stream - false - } - Err(_timeout) => { - // timed out - error!("{} timed out {:}", addr, _timeout); - false +fn resolve_host(host: &str, port: u16) -> Option { + match format!("{}:{}", host, port).to_socket_addrs() { + Ok(mut addrs) => addrs.next(), + Err(e) => { + error!("Failed to resolve {}:{} - {}", host, port, e); + None } } } + +async fn do_port_check(host: &str, port: u16) -> bool { + match resolve_host(host, port) { + Some(addr) => match tokio::time::timeout( + CONNECTION_TIMEOUT_SECONDS, + tokio::net::TcpStream::connect(addr), + ) + .await + { + Ok(Ok(_stream)) => { + // didn't timeout and tcp stream is open + trace!("Successfully pinged {}", addr); + true + } + Ok(Err(_stream_err)) => { + error!("{} ping failed {:}", addr, _stream_err); + // didn't timeout but couldn't open tcp stream + false + } + Err(_timeout) => { + // timed out + error!("{} timed out {:}", addr, _timeout); + false + } + }, + None => false, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolve_host_with_valid_ip_address_returns_some() { + assert!(resolve_host("8.8.8.8", 1234).is_some()); + assert!(resolve_host("2001:4860:4860::8888", 1234).is_some()); + } + + #[test] + fn resolve_host_with_valid_hostname_returns_some() { + assert!(resolve_host("nymtech.net", 1234).is_some()); + } + + #[test] + fn resolve_host_with_malformed_ip_address_returns_some() { + // these have missing values filled in + assert!(resolve_host("1.2.3", 1234).is_some()); // 1.2.0.3 + assert!(resolve_host("1.2", 1234).is_some()); // 1.0.0.2 + assert!(resolve_host("1", 1234).is_some()); // 0.0.0.1 + } + + #[test] + fn resolve_host_with_unknown_hostname_returns_none() { + assert!(resolve_host( + "some-unknown-hostname-that-will-never-resolve.nymtech.net", + 1234 + ) + .is_none()); + } + + #[test] + fn resolve_host_with_bad_strings_return_none() { + assert!(resolve_host("", 1234).is_none()); + assert!(resolve_host("🤘", 1234).is_none()); + assert!(resolve_host("@", 1234).is_none()); + assert!(resolve_host("*", 1234).is_none()); + } +} From 92f976a45d2d05c1250d6351392a3acc4eb1c01f Mon Sep 17 00:00:00 2001 From: Mark Sinclair Date: Fri, 27 Aug 2021 11:34:21 +0100 Subject: [PATCH 2/3] explorer-api: sanitize hostname before running checks to avoid leading or trailing spaces that are known to exist in the current test net --- explorer-api/src/ping/http.rs | 51 ++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/explorer-api/src/ping/http.rs b/explorer-api/src/ping/http.rs index 30cac9702e..6728c7ebac 100644 --- a/explorer-api/src/ping/http.rs +++ b/explorer-api/src/ping/http.rs @@ -5,9 +5,10 @@ use std::time::Duration; use rocket::serde::json::Json; use rocket::{Route, State}; +use mixnet_contract::MixNodeBond; + use crate::ping::models::PingResponse; use crate::state::ExplorerApiStateContext; -use mixnet_contract::MixNodeBond; const CONNECTION_TIMEOUT_SECONDS: Duration = Duration::from_secs(10); @@ -80,18 +81,31 @@ async fn port_check(bond: &MixNodeBond) -> HashMap { ports } -fn resolve_host(host: &str, port: u16) -> Option { - match format!("{}:{}", host, port).to_socket_addrs() { +fn sanitize_and_resolve_host(host: &str, port: u16) -> Option { + // trim the host + let trimmed_host = host.trim(); + + // host must be at least one non-whitespace character + if trimmed_host.is_empty() { + return None; + } + + // the host string should hopefully parse and resolve into a valid socket address + let parsed_host = format!("{}:{}", trimmed_host, port); + match parsed_host.to_socket_addrs() { Ok(mut addrs) => addrs.next(), Err(e) => { - error!("Failed to resolve {}:{} - {}", host, port, e); + error!( + "Failed to resolve {}:{} -> {}. Error: {}", + host, port, parsed_host, e + ); None } } } async fn do_port_check(host: &str, port: u16) -> bool { - match resolve_host(host, port) { + match sanitize_and_resolve_host(host, port) { Some(addr) => match tokio::time::timeout( CONNECTION_TIMEOUT_SECONDS, tokio::net::TcpStream::connect(addr), @@ -124,26 +138,25 @@ mod tests { #[test] fn resolve_host_with_valid_ip_address_returns_some() { - assert!(resolve_host("8.8.8.8", 1234).is_some()); - assert!(resolve_host("2001:4860:4860::8888", 1234).is_some()); + assert!(sanitize_and_resolve_host("8.8.8.8", 1234).is_some()); + assert!(sanitize_and_resolve_host("2001:4860:4860::8888", 1234).is_some()); } #[test] fn resolve_host_with_valid_hostname_returns_some() { - assert!(resolve_host("nymtech.net", 1234).is_some()); + assert!(sanitize_and_resolve_host("nymtech.net", 1234).is_some()); } #[test] - fn resolve_host_with_malformed_ip_address_returns_some() { - // these have missing values filled in - assert!(resolve_host("1.2.3", 1234).is_some()); // 1.2.0.3 - assert!(resolve_host("1.2", 1234).is_some()); // 1.0.0.2 - assert!(resolve_host("1", 1234).is_some()); // 0.0.0.1 + fn resolve_host_with_malformed_ip_address_returns_none() { + // these are invalid ip addresses + assert!(sanitize_and_resolve_host("192.168.1.999", 1234).is_none()); + assert!(sanitize_and_resolve_host("10.999.999.999", 1234).is_none()); } #[test] fn resolve_host_with_unknown_hostname_returns_none() { - assert!(resolve_host( + assert!(sanitize_and_resolve_host( "some-unknown-hostname-that-will-never-resolve.nymtech.net", 1234 ) @@ -152,9 +165,11 @@ mod tests { #[test] fn resolve_host_with_bad_strings_return_none() { - assert!(resolve_host("", 1234).is_none()); - assert!(resolve_host("🤘", 1234).is_none()); - assert!(resolve_host("@", 1234).is_none()); - assert!(resolve_host("*", 1234).is_none()); + assert!(sanitize_and_resolve_host("", 1234).is_none()); + assert!(sanitize_and_resolve_host(" ", 1234).is_none()); + assert!(sanitize_and_resolve_host(" 🤘 ", 1234).is_none()); + assert!(sanitize_and_resolve_host("🤘", 1234).is_none()); + assert!(sanitize_and_resolve_host("@", 1234).is_none()); + assert!(sanitize_and_resolve_host("*", 1234).is_none()); } } From cdf0d443411618e0952c35bfb5972fc82023cdfa Mon Sep 17 00:00:00 2001 From: Mark Sinclair Date: Tue, 31 Aug 2021 14:37:28 +0100 Subject: [PATCH 3/3] explorer-api: turned down logging from `error` to `warn` --- explorer-api/src/ping/http.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/explorer-api/src/ping/http.rs b/explorer-api/src/ping/http.rs index 6728c7ebac..8853fbac61 100644 --- a/explorer-api/src/ping/http.rs +++ b/explorer-api/src/ping/http.rs @@ -95,7 +95,7 @@ fn sanitize_and_resolve_host(host: &str, port: u16) -> Option { match parsed_host.to_socket_addrs() { Ok(mut addrs) => addrs.next(), Err(e) => { - error!( + warn!( "Failed to resolve {}:{} -> {}. Error: {}", host, port, parsed_host, e ); @@ -118,13 +118,13 @@ async fn do_port_check(host: &str, port: u16) -> bool { true } Ok(Err(_stream_err)) => { - error!("{} ping failed {:}", addr, _stream_err); + warn!("{} ping failed {:}", addr, _stream_err); // didn't timeout but couldn't open tcp stream false } Err(_timeout) => { // timed out - error!("{} timed out {:}", addr, _timeout); + warn!("{} timed out {:}", addr, _timeout); false } },