From f09013e893874997918a1be2c271dd492ccae876 Mon Sep 17 00:00:00 2001 From: jstuczyn Date: Tue, 19 Jan 2021 16:11:55 +0000 Subject: [PATCH 1/3] Initial split into domains and ipnets --- .../network-requester/Cargo.toml | 3 +- .../network-requester/src/allowed_hosts.rs | 120 +++++++++++++++--- 2 files changed, 102 insertions(+), 21 deletions(-) diff --git a/service-providers/network-requester/Cargo.toml b/service-providers/network-requester/Cargo.toml index 9acf292c7e4..a21d6b037be 100644 --- a/service-providers/network-requester/Cargo.toml +++ b/service-providers/network-requester/Cargo.toml @@ -14,7 +14,8 @@ log = "0.4" pretty_env_logger = "0.3" tokio = { version = "0.2", features = ["stream", "tcp", "rt-threaded", "macros"] } tokio-tungstenite = "0.11.0" -publicsuffix = "1.3.1" +publicsuffix = "1.5" +ipnet = "2.3.0" # internal nymsphinx = { path = "../../common/nymsphinx" } diff --git a/service-providers/network-requester/src/allowed_hosts.rs b/service-providers/network-requester/src/allowed_hosts.rs index 350746da1ff..b7a929b0efa 100644 --- a/service-providers/network-requester/src/allowed_hosts.rs +++ b/service-providers/network-requester/src/allowed_hosts.rs @@ -1,6 +1,8 @@ use fs::OpenOptions; use io::BufReader; +use ipnet::IpNet; use publicsuffix::{errors, List}; +use std::collections::HashSet; use std::fs; use std::fs::File; use std::io; @@ -50,10 +52,12 @@ impl OutboundRequestFilter { /// /// If it's not in the list, return `false` and write it to the `unknown_hosts` storefile. pub(crate) fn check(&mut self, host: &str) -> bool { + // check if it's an ip address first + let trimmed = Self::trim_port(host); match self.get_domain_root(&trimmed) { Some(domain_root) => { - if self.allowed_hosts.contains(&domain_root) { + if self.allowed_hosts.contains_domain(&domain_root) { true } else { // not in allowed list but it's a domain @@ -96,11 +100,48 @@ impl OutboundRequestFilter { } } +enum Host { + Domain(String), + Ip(IpNet), +} + +impl From for Host { + fn from(raw: String) -> Self { + if let Ok(ipnet) = raw.parse() { + Host::Ip(ipnet) + } else { + Host::Domain(raw) + } + } +} + +impl Host { + fn is_domain(&self) -> bool { + matches!(self, Host::Domain(..)) + } + + fn extract_domain(self) -> String { + match self { + Host::Domain(domain) => domain, + _ => panic!("called extract domain on an ipnet!"), + } + } + + fn extract_ipnet(self) -> IpNet { + match self { + Host::Ip(ipnet) => ipnet, + _ => panic!("called extract ipnet on a domain!"), + } + } +} + /// A simple file-based store for information about allowed / unknown hosts. #[derive(Debug)] pub(crate) struct HostsStore { storefile: PathBuf, - hosts: Vec, + + domains: HashSet, + ip_nets: Vec, } impl HostsStore { @@ -109,7 +150,17 @@ impl HostsStore { let storefile = HostsStore::setup_storefile(base_dir, filename); let hosts = HostsStore::load_from_storefile(&storefile) .unwrap_or_else(|_| panic!("Could not load hosts from storefile at {:?}", storefile)); - HostsStore { storefile, hosts } + + let (domains, ip_nets): (Vec<_>, Vec<_>) = + hosts.into_iter().partition(|host| host.is_domain()); + + HostsStore { + storefile, + domains: domains.into_iter().map(Host::extract_domain).collect(), + ip_nets: ip_nets.into_iter().map(Host::extract_ipnet).collect(), + } + + // HostsStore { storefile, hosts } } fn append(path: &Path, text: &str) { @@ -129,8 +180,8 @@ impl HostsStore { HostsStore::append(&self.storefile, host); } - fn contains(&self, host: &str) -> bool { - self.hosts.contains(&host.to_string()) + fn contains_domain(&self, host: &str) -> bool { + self.domains.contains(&host.to_string()) } /// Returns the default base directory for the storefile. @@ -143,8 +194,8 @@ impl HostsStore { } fn maybe_add(&mut self, host: &str) { - if !self.contains(host) { - self.hosts.push(host.to_string()); + if !self.contains_domain(host) { + self.domains.insert(host.to_string()); self.append_to_file(host); } } @@ -162,14 +213,16 @@ impl HostsStore { } /// Loads the storefile contents into memory. - fn load_from_storefile

(filename: P) -> io::Result> + fn load_from_storefile

(filename: P) -> io::Result> where P: AsRef, { let file = File::open(filename)?; let reader = BufReader::new(&file); - let lines: Vec = reader.lines().collect::>().unwrap(); - Ok(lines) + Ok(reader + .lines() + .map(|line| Host::from(line.expect("failed to read input file line!"))) + .collect()) } } @@ -281,30 +334,36 @@ mod tests { let host = "unknown.com"; let mut filter = setup(); filter.check(host); - assert_eq!(1, filter.unknown_hosts.hosts.len()); - assert_eq!("unknown.com", filter.unknown_hosts.hosts.first().unwrap()); + assert_eq!(1, filter.unknown_hosts.domains.len()); + assert!(filter.unknown_hosts.domains.contains("unknown.com")); filter.check(host); - assert_eq!(1, filter.unknown_hosts.hosts.len()); - assert_eq!("unknown.com", filter.unknown_hosts.hosts.first().unwrap()); + assert_eq!(1, filter.unknown_hosts.domains.len()); + assert!(filter.unknown_hosts.domains.contains("unknown.com")); } } + #[cfg(test)] mod requests_to_allowed_hosts { use super::*; - fn setup() -> OutboundRequestFilter { + + fn setup(allowed: &[&str]) -> OutboundRequestFilter { let (allowed_storefile, base_dir1, allowed_filename) = create_test_storefile(); let (_, base_dir2, unknown_filename) = create_test_storefile(); - HostsStore::append(&allowed_storefile, "nymtech.net"); + + for allowed_host in allowed { + HostsStore::append(&allowed_storefile, &*allowed_host) + } let allowed = HostsStore::new(base_dir1, allowed_filename); let unknown = HostsStore::new(base_dir2, unknown_filename); OutboundRequestFilter::new(allowed, unknown) } + #[test] fn are_allowed() { let host = "nymtech.net"; - let mut filter = setup(); + let mut filter = setup(&["nymtech.net"]); assert_eq!(true, filter.check(host)); } @@ -312,13 +371,13 @@ mod tests { fn are_allowed_for_subdomains() { let host = "foomp.nymtech.net"; - let mut filter = setup(); + let mut filter = setup(&["nymtech.net"]); assert_eq!(true, filter.check(host)); } #[test] fn are_not_appended_to_file() { - let mut filter = setup(); + let mut filter = setup(&["nymtech.net"]); // test initial state let lines = HostsStore::load_from_storefile(&filter.allowed_hosts.storefile).unwrap(); @@ -330,6 +389,25 @@ mod tests { let lines = HostsStore::load_from_storefile(&filter.allowed_hosts.storefile).unwrap(); assert_eq!(1, lines.len()); } + + // #[test] + // fn are_allowed_for_ipv4_addresses() { + // let address1 = "1.1.1.1"; + // let address2 = "1.1.1.1:1234"; + // + // let mut filter = setup(&["1.1.1.1"]); + // assert_eq!(true, filter.check(address1)); + // assert_eq!(true, filter.check(address2)); + // } + // + // #[test] + // fn are_allowed_for_ipv6_addresses() {} + // + // #[test] + // fn are_allowed_for_ipv4_address_ranges() {} + // + // #[test] + // fn are_allowed_for_ipv6_address_ranges() {} } fn random_string() -> String { @@ -354,6 +432,7 @@ mod tests { #[cfg(test)] mod creating_a_new_host_store { use super::*; + #[test] fn loads_its_host_list_from_storefile() { let (storefile, base_dir, filename) = create_test_storefile(); @@ -361,7 +440,8 @@ mod tests { HostsStore::append(&storefile, "edwardsnowden.com"); let host_store = HostsStore::new(base_dir, filename); - assert_eq!(vec!["nymtech.net", "edwardsnowden.com"], host_store.hosts); + assert!(host_store.domains.contains("nymtech.net")); + assert!(host_store.domains.contains("edwardsnowden.com")); } } } From 409a9abbfe727d3257f71431f6cb76ace321ecc0 Mon Sep 17 00:00:00 2001 From: jstuczyn Date: Wed, 20 Jan 2021 10:30:18 +0000 Subject: [PATCH 2/3] Changed the library used for ip networks for more convenience --- .../network-requester/Cargo.toml | 3 +- .../network-requester/src/allowed_hosts.rs | 195 +++++++++++++----- 2 files changed, 148 insertions(+), 50 deletions(-) diff --git a/service-providers/network-requester/Cargo.toml b/service-providers/network-requester/Cargo.toml index a21d6b037be..652734587a4 100644 --- a/service-providers/network-requester/Cargo.toml +++ b/service-providers/network-requester/Cargo.toml @@ -15,7 +15,8 @@ pretty_env_logger = "0.3" tokio = { version = "0.2", features = ["stream", "tcp", "rt-threaded", "macros"] } tokio-tungstenite = "0.11.0" publicsuffix = "1.5" -ipnet = "2.3.0" +ipnetwork = "0.17" + # internal nymsphinx = { path = "../../common/nymsphinx" } diff --git a/service-providers/network-requester/src/allowed_hosts.rs b/service-providers/network-requester/src/allowed_hosts.rs index b7a929b0efa..99f9a46d025 100644 --- a/service-providers/network-requester/src/allowed_hosts.rs +++ b/service-providers/network-requester/src/allowed_hosts.rs @@ -1,12 +1,13 @@ use fs::OpenOptions; use io::BufReader; -use ipnet::IpNet; +use ipnetwork::IpNetwork; use publicsuffix::{errors, List}; use std::collections::HashSet; use std::fs; use std::fs::File; use std::io; use std::io::BufRead; +use std::net::{IpAddr, SocketAddr}; use std::path::Path; use std::path::PathBuf; @@ -52,31 +53,39 @@ impl OutboundRequestFilter { /// /// If it's not in the list, return `false` and write it to the `unknown_hosts` storefile. pub(crate) fn check(&mut self, host: &str) -> bool { - // check if it's an ip address first - - let trimmed = Self::trim_port(host); - match self.get_domain_root(&trimmed) { - Some(domain_root) => { - if self.allowed_hosts.contains_domain(&domain_root) { - true - } else { - // not in allowed list but it's a domain - log::warn!( - "Blocked outbound connection to {:?}, add it to allowed.list if needed", - &domain_root - ); - self.unknown_hosts.maybe_add(&domain_root); - false // domain is unknown - } - } - None => { - false // the host was either an IP or nonsense. For this release, we'll ignore it. + // first check if it's a socket address (ip:port) + // (this check is performed to not incorrectly strip what we think might be a port + // from ipv6 address, as for example ::1 contains colons but has no port + let check_res = if let Ok(socketaddr) = host.parse::() { + self.allowed_hosts.contains_ip_address(socketaddr.ip()) + } else if let Ok(ipaddr) = host.parse::() { + // then check if it was an ip address + self.allowed_hosts.contains_ip_address(ipaddr) + } else { + // finally, then assume it might be a domain + let trimmed = Self::trim_port(host); + if let Some(domain_root) = self.get_domain_root(&trimmed) { + // it's a domain + self.allowed_hosts.contains_domain(&domain_root) + } else { + // it's something else, no idea what, probably some nonsense + false } + }; + + if !check_res { + log::warn!( + "Blocked outbound connection to {:?}, add it to allowed.list if needed", + &host + ); + self.unknown_hosts.maybe_add(&host); } + + check_res } fn trim_port(host: &str) -> String { - let mut tmp: Vec<&str> = host.split(':').collect(); + let mut tmp: Vec<_> = host.split(':').collect(); if tmp.len() > 1 { tmp.pop(); // get rid of last element (port) tmp.join(":") //rejoin @@ -102,13 +111,15 @@ impl OutboundRequestFilter { enum Host { Domain(String), - Ip(IpNet), + IpNetwork(IpNetwork), + // IP + // but what if you wanted filtering on cidr + ip? } impl From for Host { fn from(raw: String) -> Self { if let Ok(ipnet) = raw.parse() { - Host::Ip(ipnet) + Host::IpNetwork(ipnet) } else { Host::Domain(raw) } @@ -127,21 +138,24 @@ impl Host { } } - fn extract_ipnet(self) -> IpNet { + fn extract_ipnetwork(self) -> IpNetwork { match self { - Host::Ip(ipnet) => ipnet, + Host::IpNetwork(ipnet) => ipnet, _ => panic!("called extract ipnet on a domain!"), } } } /// A simple file-based store for information about allowed / unknown hosts. +/// Currently it completely ignores any port information. +// TODO: in the future allow filtering by port, so for example 1.1.1.1:80 would be a valid filter, +// which would allow connections to the port :80 while any requests to say 1.1.1.1:1234 would be denied. #[derive(Debug)] pub(crate) struct HostsStore { storefile: PathBuf, domains: HashSet, - ip_nets: Vec, + ip_nets: Vec, } impl HostsStore { @@ -157,7 +171,7 @@ impl HostsStore { HostsStore { storefile, domains: domains.into_iter().map(Host::extract_domain).collect(), - ip_nets: ip_nets.into_iter().map(Host::extract_ipnet).collect(), + ip_nets: ip_nets.into_iter().map(Host::extract_ipnetwork).collect(), } // HostsStore { storefile, hosts } @@ -184,6 +198,19 @@ impl HostsStore { self.domains.contains(&host.to_string()) } + fn contains_ip_address(&self, address: IpAddr) -> bool { + // I'm not sure it's possible to achieve the same functionality without iterating through + // the whole thing. Maybe by some clever usage of tries? But I doubt we're going to have + // so many filtering rules that it's going to matter at this point. + for ip_net in &self.ip_nets { + if ip_net.contains(address) { + return true; + } + } + + false + } + /// Returns the default base directory for the storefile. /// /// This is split out so we can easily inject our own base_dir for unit tests. @@ -194,10 +221,14 @@ impl HostsStore { } fn maybe_add(&mut self, host: &str) { - if !self.contains_domain(host) { - self.domains.insert(host.to_string()); - self.append_to_file(host); - } + // TODO + + log::error!("need to finish 'maybe add'") + + // if !self.contains_domain(host) { + // self.domains.insert(host.to_string()); + // self.append_to_file(host); + // } } fn setup_storefile(base_dir: PathBuf, filename: PathBuf) -> PathBuf { @@ -390,24 +421,81 @@ mod tests { assert_eq!(1, lines.len()); } - // #[test] - // fn are_allowed_for_ipv4_addresses() { - // let address1 = "1.1.1.1"; - // let address2 = "1.1.1.1:1234"; - // - // let mut filter = setup(&["1.1.1.1"]); - // assert_eq!(true, filter.check(address1)); - // assert_eq!(true, filter.check(address2)); - // } - // - // #[test] - // fn are_allowed_for_ipv6_addresses() {} - // - // #[test] - // fn are_allowed_for_ipv4_address_ranges() {} - // - // #[test] - // fn are_allowed_for_ipv6_address_ranges() {} + #[test] + fn are_allowed_for_ipv4_addresses() { + let address_good = "1.1.1.1"; + let address_good_port = "1.1.1.1:1234"; + let address_bad = "1.1.1.2"; + + let mut filter = setup(&["1.1.1.1"]); + assert!(filter.check(address_good)); + assert!(filter.check(address_good_port)); + assert!(!filter.check(address_bad)); + } + + #[test] + fn are_allowed_for_ipv6_addresses() { + let ip_v6_full = "2001:0db8:85a3:0000:0000:8a2e:0370:7334"; + let ip_v6_full_rendered = "2001:0db8:85a3::8a2e:0370:7334"; + let ip_v6_full_port = "[2001:0db8:85a3::8a2e:0370:7334]:1234"; + + let ip_v6_semi = "2001:0db8::0001:0000"; + let ip_v6_semi_rendered = "2001:db8::1:0"; + + let ip_v6_loopback_port = "[::1]:1234"; + + let mut filter1 = setup(&[ip_v6_full, ip_v6_semi, "::1"]); + let mut filter2 = setup(&[ip_v6_full_rendered, ip_v6_semi_rendered, "::1"]); + + assert!(filter1.check(ip_v6_full)); + assert!(filter1.check(ip_v6_full_rendered)); + assert!(filter1.check(ip_v6_full_port)); + assert!(filter1.check(ip_v6_semi)); + assert!(filter1.check(ip_v6_semi_rendered)); + assert!(filter1.check(ip_v6_loopback_port)); + + assert!(filter2.check(ip_v6_full)); + assert!(filter2.check(ip_v6_full_rendered)); + assert!(filter2.check(ip_v6_full_port)); + assert!(filter2.check(ip_v6_semi)); + assert!(filter2.check(ip_v6_semi_rendered)); + assert!(filter2.check(ip_v6_loopback_port)); + } + + #[test] + fn are_allowed_for_ipv4_address_ranges() { + let range1 = "127.0.0.1/32"; + let range2 = "1.2.3.4/24"; + + let bottom_range2 = "1.2.3.0"; + let top_range2 = "1.2.3.255"; + + let outside_range2 = "1.2.2.4"; + + let mut filter = setup(&[range1, range2]); + assert!(filter.check("127.0.0.1")); + assert!(filter.check("127.0.0.1:1234")); + assert!(filter.check(bottom_range2)); + assert!(filter.check(top_range2)); + assert!(!filter.check(outside_range2)); + } + + #[test] + fn are_allowed_for_ipv6_address_ranges() { + let range = "2620:0:2d0:200::7/32"; + + let bottom1 = "2620:0:0:0:0:0:0:0"; + let bottom2 = "2620::"; + + let top = "2620:0:ffff:ffff:ffff:ffff:ffff:ffff"; + let mid = "2620:0:42::42"; + + let mut filter = setup(&[range]); + assert!(filter.check(bottom1)); + assert!(filter.check(bottom2)); + assert!(filter.check(top)); + assert!(filter.check(mid)); + } } fn random_string() -> String { @@ -438,10 +526,19 @@ mod tests { let (storefile, base_dir, filename) = create_test_storefile(); HostsStore::append(&storefile, "nymtech.net"); HostsStore::append(&storefile, "edwardsnowden.com"); + HostsStore::append(&storefile, "1.2.3.4"); + HostsStore::append(&storefile, "5.6.7.8/16"); + HostsStore::append(&storefile, "1:2:3::"); + HostsStore::append(&storefile, "5:6:7::/48"); let host_store = HostsStore::new(base_dir, filename); assert!(host_store.domains.contains("nymtech.net")); assert!(host_store.domains.contains("edwardsnowden.com")); + + assert!(host_store.ip_nets.contains(&"1.2.3.4".parse().unwrap())); + assert!(host_store.ip_nets.contains(&"5.6.7.8/16".parse().unwrap())); + assert!(host_store.ip_nets.contains(&"1:2:3::".parse().unwrap())); + assert!(host_store.ip_nets.contains(&"5:6:7::/48".parse().unwrap())); } } } From 9f4646b9634d1f8283b5f5dfc84c59d325d36889 Mon Sep 17 00:00:00 2001 From: jstuczyn Date: Wed, 20 Jan 2021 10:46:13 +0000 Subject: [PATCH 3/3] Adding disallowed hosts to the unknown file --- .../network-requester/src/allowed_hosts.rs | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/service-providers/network-requester/src/allowed_hosts.rs b/service-providers/network-requester/src/allowed_hosts.rs index 99f9a46d025..e8c04793bd9 100644 --- a/service-providers/network-requester/src/allowed_hosts.rs +++ b/service-providers/network-requester/src/allowed_hosts.rs @@ -56,32 +56,43 @@ impl OutboundRequestFilter { // first check if it's a socket address (ip:port) // (this check is performed to not incorrectly strip what we think might be a port // from ipv6 address, as for example ::1 contains colons but has no port - let check_res = if let Ok(socketaddr) = host.parse::() { - self.allowed_hosts.contains_ip_address(socketaddr.ip()) + let allowed = if let Ok(socketaddr) = host.parse::() { + if !self.allowed_hosts.contains_ip_address(socketaddr.ip()) { + self.unknown_hosts.maybe_add_ip(socketaddr.ip()); + return false; + } + true } else if let Ok(ipaddr) = host.parse::() { // then check if it was an ip address - self.allowed_hosts.contains_ip_address(ipaddr) + if !self.allowed_hosts.contains_ip_address(ipaddr) { + self.unknown_hosts.maybe_add_ip(ipaddr); + return false; + } + true } else { // finally, then assume it might be a domain let trimmed = Self::trim_port(host); if let Some(domain_root) = self.get_domain_root(&trimmed) { // it's a domain - self.allowed_hosts.contains_domain(&domain_root) + if !self.allowed_hosts.contains_domain(&domain_root) { + self.unknown_hosts.maybe_add_domain(&trimmed); + return false; + } + true } else { // it's something else, no idea what, probably some nonsense false } }; - if !check_res { + if !allowed { log::warn!( "Blocked outbound connection to {:?}, add it to allowed.list if needed", &host ); - self.unknown_hosts.maybe_add(&host); } - check_res + allowed } fn trim_port(host: &str) -> String { @@ -109,13 +120,15 @@ impl OutboundRequestFilter { } } +// used for parsing file content enum Host { Domain(String), IpNetwork(IpNetwork), - // IP - // but what if you wanted filtering on cidr + ip? } +// TODO: perphaps in the future it should do some domain validation? +// so for example if somebody put some nonsense in the whitelist file like "foomp", it would get +// rejected? impl From for Host { fn from(raw: String) -> Self { if let Ok(ipnet) = raw.parse() { @@ -173,8 +186,6 @@ impl HostsStore { domains: domains.into_iter().map(Host::extract_domain).collect(), ip_nets: ip_nets.into_iter().map(Host::extract_ipnetwork).collect(), } - - // HostsStore { storefile, hosts } } fn append(path: &Path, text: &str) { @@ -220,15 +231,18 @@ impl HostsStore { .join(".nym") } - fn maybe_add(&mut self, host: &str) { - // TODO - - log::error!("need to finish 'maybe add'") + fn maybe_add_ip(&mut self, ip: IpAddr) { + if !self.contains_ip_address(ip) { + self.ip_nets.push(ip.into()); + self.append_to_file(&ip.to_string()); + } + } - // if !self.contains_domain(host) { - // self.domains.insert(host.to_string()); - // self.append_to_file(host); - // } + fn maybe_add_domain(&mut self, domain: &str) { + if !self.contains_domain(domain) { + self.domains.insert(domain.to_string()); + self.append_to_file(domain); + } } fn setup_storefile(base_dir: PathBuf, filename: PathBuf) -> PathBuf {