From 605f8fcde3b02de1fbf0c52f28d88476d0a989ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C4=99drzej=20Stuczy=C5=84ski?= Date: Fri, 31 Mar 2023 15:06:12 +0100 Subject: [PATCH] updated and cleaned up tests --- Cargo.lock | 65 +++++- .../network-requester/Cargo.toml | 3 + .../src/allowed_hosts/filter.rs | 218 ++++++++++-------- .../src/allowed_hosts/group.rs | 16 +- .../src/allowed_hosts/hosts.rs | 27 ++- .../src/allowed_hosts/stored_allowed_hosts.rs | 8 + 6 files changed, 225 insertions(+), 112 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 944b28701e..e2d5a90838 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1610,6 +1610,17 @@ dependencies = [ "winapi", ] +[[package]] +name = "errno" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50d6a0976c999d473fe89ad888d5a284e55366d9dc9038b1ba2aa15128c4afa0" +dependencies = [ + "errno-dragonfly", + "libc", + "windows-sys 0.45.0", +] + [[package]] name = "errno-dragonfly" version = "0.1.2" @@ -1721,7 +1732,7 @@ checksum = "8a3de6e8d11b22ff9edc6d916f890800597d60f8b2da1caf2955c274638d6412" dependencies = [ "cfg-if", "libc", - "redox_syscall", + "redox_syscall 0.2.16", "windows-sys 0.45.0", ] @@ -2636,7 +2647,7 @@ checksum = "8687c819457e979cc940d09cb16e42a1bf70aa6b60a549de6d3a62a0ee90c69e" dependencies = [ "hermit-abi 0.3.1", "io-lifetimes", - "rustix", + "rustix 0.36.11", "windows-sys 0.45.0", ] @@ -2848,6 +2859,12 @@ version = "0.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f051f77a7c8e6957c0696eac88f26b0117e54f52d3fc682ab19397a8812846a4" +[[package]] +name = "linux-raw-sys" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d59d8c75012853d2e872fb56bc8a2e53718e2cafe1a4c823143141c6d90c322f" + [[package]] name = "lioness" version = "0.1.2" @@ -3687,6 +3704,7 @@ dependencies = [ "serde", "sqlx 0.6.2", "tap", + "tempfile", "thiserror", "tokio", "tokio-tungstenite 0.17.2", @@ -4290,7 +4308,7 @@ dependencies = [ "cfg-if", "instant", "libc", - "redox_syscall", + "redox_syscall 0.2.16", "smallvec", "winapi", ] @@ -4303,7 +4321,7 @@ checksum = "9069cbb9f99e3a5083476ccb29ceb1de18b9118cafa53e90c9551235de2b9521" dependencies = [ "cfg-if", "libc", - "redox_syscall", + "redox_syscall 0.2.16", "smallvec", "windows-sys 0.45.0", ] @@ -4944,6 +4962,15 @@ dependencies = [ "bitflags 1.3.2", ] +[[package]] +name = "redox_syscall" +version = "0.3.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "567664f262709473930a4bf9e51bf2ebf3348f2e748ccc50dea20646858f8f29" +dependencies = [ + "bitflags 1.3.2", +] + [[package]] name = "redox_users" version = "0.4.3" @@ -4951,7 +4978,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b033d837a7cf162d7993aded9304e30a83213c648b6e389db233191f891e5c2b" dependencies = [ "getrandom 0.2.8", - "redox_syscall", + "redox_syscall 0.2.16", "thiserror", ] @@ -5227,10 +5254,24 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "db4165c9963ab29e422d6c26fbc1d37f15bace6b2810221f9d925023480fcf0e" dependencies = [ "bitflags 1.3.2", - "errno", + "errno 0.2.8", "io-lifetimes", "libc", - "linux-raw-sys", + "linux-raw-sys 0.1.4", + "windows-sys 0.45.0", +] + +[[package]] +name = "rustix" +version = "0.37.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0e78cc525325c06b4a7ff02db283472f3c042b7ff0c391f96c6d5ac6f4f91b75" +dependencies = [ + "bitflags 1.3.2", + "errno 0.3.0", + "io-lifetimes", + "libc", + "linux-raw-sys 0.3.1", "windows-sys 0.45.0", ] @@ -6107,15 +6148,15 @@ checksum = "55937e1799185b12863d447f42597ed69d9928686b8d88a1df17376a097d8369" [[package]] name = "tempfile" -version = "3.4.0" +version = "3.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af18f7ae1acd354b992402e9ec5864359d693cd8a79dcbef59f76891701c1e95" +checksum = "b9fbec84f381d5795b08656e4912bec604d162bff9291d6189a78f4c8ab87998" dependencies = [ "cfg-if", "fastrand", - "redox_syscall", - "rustix", - "windows-sys 0.42.0", + "redox_syscall 0.3.5", + "rustix 0.37.5", + "windows-sys 0.45.0", ] [[package]] diff --git a/service-providers/network-requester/Cargo.toml b/service-providers/network-requester/Cargo.toml index 95cbba6c01..bcac96ffbe 100644 --- a/service-providers/network-requester/Cargo.toml +++ b/service-providers/network-requester/Cargo.toml @@ -47,3 +47,6 @@ nym-socks5-requests = { path = "../../common/socks5/requests" } nym-statistics-common = { path = "../../common/statistics" } nym-task = { path = "../../common/task" } websocket-requests = { path = "../../clients/native/websocket-requests" } + +[dev-dependencies] +tempfile = "3.5.0" \ No newline at end of file diff --git a/service-providers/network-requester/src/allowed_hosts/filter.rs b/service-providers/network-requester/src/allowed_hosts/filter.rs index 7950bfdbca..8404bd5fb3 100644 --- a/service-providers/network-requester/src/allowed_hosts/filter.rs +++ b/service-providers/network-requester/src/allowed_hosts/filter.rs @@ -7,6 +7,7 @@ use crate::allowed_hosts::standard_list::StandardList; use crate::allowed_hosts::stored_allowed_hosts::StoredAllowedHosts; use std::net::{IpAddr, SocketAddr}; +#[derive(Debug)] enum RequestHost { IpAddr(IpAddr), SocketAddr(SocketAddr), @@ -168,12 +169,100 @@ impl OutboundRequestFilter { #[cfg(test)] mod tests { - use std::{ - fs::{self, File}, - path::PathBuf, - }; - use super::*; + use std::ops::{Deref, DerefMut}; + + struct HostsStoreFixture { + inner: HostsStore, + // take ownership of temp file that will get cleaned up on drop (i.e. when test finishes) + _tmp_file: tempfile::NamedTempFile, + } + + impl Deref for HostsStoreFixture { + type Target = HostsStore; + + fn deref(&self) -> &Self::Target { + &self.inner + } + } + + impl DerefMut for HostsStoreFixture { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner + } + } + + impl HostsStoreFixture { + fn new() -> HostsStoreFixture { + let tmp_file = tempfile::NamedTempFile::new().unwrap(); + let inner = HostsStore::new(&tmp_file); + HostsStoreFixture { + inner, + _tmp_file: tmp_file, + } + } + } + + struct OutboundRequestFilterFixture { + inner: OutboundRequestFilter, + // take ownership of temp files that will get cleaned up on drop (i.e. when test finishes) + _allow_tmp_file: tempfile::NamedTempFile, + _unknown_tmp_file: tempfile::NamedTempFile, + } + + impl Deref for OutboundRequestFilterFixture { + type Target = OutboundRequestFilter; + + fn deref(&self) -> &Self::Target { + &self.inner + } + } + + impl DerefMut for OutboundRequestFilterFixture { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner + } + } + + impl OutboundRequestFilterFixture { + fn new() -> OutboundRequestFilterFixture { + let allow_tmp_file = tempfile::NamedTempFile::new().unwrap(); + let unknown_tmp_file = tempfile::NamedTempFile::new().unwrap(); + let allowed = HostsStore::new(&allow_tmp_file); + let unknown = HostsStore::new(&unknown_tmp_file); + let standard = StandardList::new(); + + let inner = OutboundRequestFilter::new(allowed.into(), standard, unknown); + OutboundRequestFilterFixture { + inner, + _allow_tmp_file: allow_tmp_file, + _unknown_tmp_file: unknown_tmp_file, + } + } + } + + fn setup_empty() -> OutboundRequestFilterFixture { + OutboundRequestFilterFixture::new() + } + + fn setup_with_allowed(allowed: &[&str]) -> OutboundRequestFilterFixture { + let allow_tmp_file = tempfile::NamedTempFile::new().unwrap(); + let unknown_tmp_file = tempfile::NamedTempFile::new().unwrap(); + let mut allowed_store = HostsStore::new(&allow_tmp_file); + let unknown = HostsStore::new(&unknown_tmp_file); + let standard = StandardList::new(); + + for allow in allowed { + allowed_store.add_host(allow) + } + + let inner = OutboundRequestFilter::new(allowed_store.into(), standard, unknown); + OutboundRequestFilterFixture { + inner, + _allow_tmp_file: allow_tmp_file, + _unknown_tmp_file: unknown_tmp_file, + } + } #[cfg(test)] mod trimming_port_information { @@ -194,24 +283,11 @@ mod tests { #[cfg(test)] mod getting_the_domain_root { - use std::path::PathBuf; - use super::*; - fn setup() -> OutboundRequestFilter { - let base_dir = test_base_dir(); - let allowed_filename = PathBuf::from(format!("allowed-{}.list", random_string())); - let unknown_filename = PathBuf::from(&format!("unknown-{}.list", random_string())); - let allowed = HostsStore::new(base_dir.clone(), allowed_filename); - let standard = StandardList::new(); - let unknown = HostsStore::new(base_dir, unknown_filename); - - OutboundRequestFilter::new(allowed, standard, unknown) - } - #[test] fn leaves_a_com_alone() { - let filter = setup(); + let filter = setup_empty(); assert_eq!( Some("domain.com".to_string()), filter.get_domain_root("domain.com") @@ -220,7 +296,7 @@ mod tests { #[test] fn trims_subdomains_from_com() { - let filter = setup(); + let filter = setup_empty(); assert_eq!( Some("domain.com".to_string()), filter.get_domain_root("foomp.domain.com") @@ -229,7 +305,7 @@ mod tests { #[test] fn works_for_non_com_roots() { - let filter = setup(); + let filter = setup_empty(); assert_eq!( Some("domain.co.uk".to_string()), filter.get_domain_root("domain.co.uk") @@ -238,7 +314,7 @@ mod tests { #[test] fn works_for_non_com_roots_with_subdomains() { - let filter = setup(); + let filter = setup_empty(); assert_eq!( Some("domain.co.uk".to_string()), filter.get_domain_root("foomp.domain.co.uk") @@ -247,13 +323,13 @@ mod tests { #[test] fn returns_none_on_garbage() { - let filter = setup(); + let filter = setup_empty(); assert_eq!(None, filter.get_domain_root("::/&&%@")); } #[test] fn returns_full_on_suffix_domains() { - let filter = setup(); + let filter = setup_empty(); assert_eq!( Some("s3.amazonaws.com".to_string()), filter.get_domain_root("s3.amazonaws.com") @@ -263,32 +339,19 @@ mod tests { #[cfg(test)] mod requests_to_unknown_hosts { - use std::path::PathBuf; - use super::*; - fn setup() -> OutboundRequestFilter { - let base_dir = test_base_dir(); - let allowed_filename = PathBuf::from(format!("allowed-{}.list", random_string())); - let unknown_filename = PathBuf::from(&format!("unknown-{}.list", random_string())); - let allowed = HostsStore::new(base_dir.clone(), allowed_filename); - let standard = StandardList::new(); - let unknown = HostsStore::new(base_dir, unknown_filename); - - OutboundRequestFilter::new(allowed, standard, unknown) - } - #[tokio::test] async fn are_not_allowed() { let host = "unknown.com"; - let mut filter = setup(); + let mut filter = setup_empty(); assert!(!filter.check(host).await); } #[tokio::test] async fn get_appended_once_to_the_unknown_hosts_list() { let host = "unknown.com"; - let mut filter = setup(); + let mut filter = setup_empty(); filter.check(host).await; assert_eq!(1, filter.unknown_hosts.data.domains.len()); assert!(filter.unknown_hosts.data.domains.contains("unknown.com")); @@ -302,26 +365,11 @@ mod tests { mod requests_to_allowed_hosts { use super::*; - fn setup(allowed: &[&str]) -> OutboundRequestFilter { - let (allowed_storefile, base_dir1, allowed_filename) = create_test_storefile(); - let (_, base_dir2, unknown_filename) = create_test_storefile(); - - for allowed_host in allowed { - HostsStore::append(&allowed_storefile, allowed_host) - } - - let allowed = HostsStore::new(base_dir1, allowed_filename); - let standard = StandardList::new(); - let unknown = HostsStore::new(base_dir2, unknown_filename); - - OutboundRequestFilter::new(allowed, standard, unknown) - } - #[tokio::test] async fn are_allowed() { let host = "nymtech.net"; - let mut filter = setup(&["nymtech.net"]); + let mut filter = setup_with_allowed(&["nymtech.net"]); assert!(filter.check(host).await); } @@ -329,22 +377,26 @@ mod tests { async fn are_allowed_for_subdomains() { let host = "foomp.nymtech.net"; - let mut filter = setup(&["nymtech.net"]); + let mut filter = setup_with_allowed(&["nymtech.net"]); assert!(filter.check(host).await); } #[tokio::test] async fn are_not_appended_to_file() { - let mut filter = setup(&["nymtech.net"]); + let mut filter = setup_with_allowed(&["nymtech.net"]); // test initial state - let lines = HostsStore::load_from_storefile(&filter.allowed_hosts.storefile).unwrap(); + let lines = + HostsStore::load_from_storefile(&filter.allowed_hosts.get().await.storefile) + .unwrap(); assert_eq!(1, lines.len()); - filter.check("nymtech.net"); + assert!(filter.check("nymtech.net").await); // test state after we've checked to make sure no unexpected changes - let lines = HostsStore::load_from_storefile(&filter.allowed_hosts.storefile).unwrap(); + let lines = + HostsStore::load_from_storefile(&filter.allowed_hosts.get().await.storefile) + .unwrap(); assert_eq!(1, lines.len()); } @@ -354,7 +406,7 @@ mod tests { let address_good_port = "1.1.1.1:1234"; let address_bad = "1.1.1.2"; - let mut filter = setup(&["1.1.1.1"]); + let mut filter = setup_with_allowed(&["1.1.1.1"]); assert!(filter.check(address_good).await); assert!(filter.check(address_good_port).await); assert!(!filter.check(address_bad).await); @@ -371,8 +423,9 @@ mod tests { 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"]); + let mut filter1 = setup_with_allowed(&[ip_v6_full, ip_v6_semi, "::1"]); + let mut filter2 = + setup_with_allowed(&[ip_v6_full_rendered, ip_v6_semi_rendered, "::1"]); assert!(filter1.check(ip_v6_full).await); assert!(filter1.check(ip_v6_full_rendered).await); @@ -399,7 +452,7 @@ mod tests { let outside_range2 = "1.2.2.4"; - let mut filter = setup(&[range1, range2]); + let mut filter = setup_with_allowed(&[range1, range2]); assert!(filter.check("127.0.0.1").await); assert!(filter.check("127.0.0.1:1234").await); assert!(filter.check(bottom_range2).await); @@ -417,7 +470,7 @@ mod tests { let top = "2620:0:ffff:ffff:ffff:ffff:ffff:ffff"; let mid = "2620:0:42::42"; - let mut filter = setup(&[range]); + let mut filter = setup_with_allowed(&[range]); assert!(filter.check(bottom1).await); assert!(filter.check(bottom2).await); assert!(filter.check(top).await); @@ -425,40 +478,21 @@ mod tests { } } - fn random_string() -> String { - format!("{:?}", rand::random::()) - } - - fn test_base_dir() -> PathBuf { - ["/tmp/nym-tests"].iter().collect() - } - - fn create_test_storefile() -> (PathBuf, PathBuf, PathBuf) { - let base_dir = test_base_dir(); - let filename = PathBuf::from(format!("hosts-store-{}.list", random_string())); - let dirpath = base_dir.join("service-providers").join("network-requester"); - fs::create_dir_all(&dirpath) - .unwrap_or_else(|_| panic!("could not create storage directory at {dirpath:?}")); - let storefile = dirpath.join(&filename); - File::create(&storefile).unwrap(); - (storefile, base_dir, filename) - } - #[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(); - 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 mut host_store = HostsStoreFixture::new(); + + host_store.add_host("nymtech.net"); + host_store.add_host("edwardsnowden.com"); + host_store.add_host("1.2.3.4"); + host_store.add_host("5.6.7.8/16"); + host_store.add_host("1:2:3::"); + host_store.add_host("5:6:7::/48"); - let host_store = HostsStore::new(base_dir, filename); assert!(host_store.data.domains.contains("nymtech.net")); assert!(host_store.data.domains.contains("edwardsnowden.com")); diff --git a/service-providers/network-requester/src/allowed_hosts/group.rs b/service-providers/network-requester/src/allowed_hosts/group.rs index a5cdeba7d6..5b398e1ff0 100644 --- a/service-providers/network-requester/src/allowed_hosts/group.rs +++ b/service-providers/network-requester/src/allowed_hosts/group.rs @@ -11,20 +11,22 @@ use std::net::IpAddr; #[derive(Debug)] pub(crate) struct HostsGroup { pub(super) domains: HashSet, - pub(super) ip_nets: Vec, + pub(super) ip_nets: HashSet, } impl HostsGroup { pub(crate) fn new(raw_hosts: Vec) -> HostsGroup { let mut domains = HashSet::new(); - let mut ip_nets = Vec::new(); + let mut ip_nets = HashSet::new(); for host in raw_hosts { match host { Host::Domain(domain) => { domains.insert(domain); } - Host::IpNetwork(ipnet) => ip_nets.push(ipnet), + Host::IpNetwork(ipnet) => { + ip_nets.insert(ipnet); + } } } @@ -45,8 +47,12 @@ impl HostsGroup { false } - pub(super) fn add_ip(&mut self, ip: IpAddr) { - self.ip_nets.push(ip.into()); + pub(super) fn contains_ip_network(&self, network: IpNetwork) -> bool { + self.ip_nets.contains(&network) + } + + pub(super) fn add_ipnet>(&mut self, network: N) { + self.ip_nets.insert(network.into()); } pub(super) fn add_domain(&mut self, domain: &str) { diff --git a/service-providers/network-requester/src/allowed_hosts/hosts.rs b/service-providers/network-requester/src/allowed_hosts/hosts.rs index 896c14c8d6..cef9893a1e 100644 --- a/service-providers/network-requester/src/allowed_hosts/hosts.rs +++ b/service-providers/network-requester/src/allowed_hosts/hosts.rs @@ -1,5 +1,6 @@ use super::host::Host; use crate::allowed_hosts::group::HostsGroup; +use ipnetwork::IpNetwork; use std::{ fs::{self, File, OpenOptions}, io::{self, BufRead, BufReader}, @@ -23,7 +24,7 @@ impl HostsStore { pub(crate) fn new>(storefile: P) -> HostsStore { let storefile = storefile.as_ref().to_path_buf(); if !storefile.is_file() { - // there's no error handling in here and I'm not going to be changing it now. + // there's no error handling in here and I'm not going to be changing it right now. panic!( "the provided storefile {:?} is not a valid file!", storefile @@ -54,9 +55,28 @@ impl HostsStore { self.data.contains_ip_address(address) } + #[allow(unused)] + pub(super) fn contains_ipnetwork(&self, network: IpNetwork) -> bool { + self.data.contains_ip_network(network) + } + + pub(super) fn add_host>(&mut self, host: H) { + match host.into() { + Host::Domain(domain) => self.add_domain(&domain), + Host::IpNetwork(ipnet) => self.add_ipnet(ipnet), + } + } + + pub(super) fn add_ipnet(&mut self, network: IpNetwork) { + if !self.contains_ipnetwork(network) { + self.data.add_ipnet(network); + self.append_to_file(&network.to_string()); + } + } + pub(super) fn add_ip(&mut self, ip: IpAddr) { if !self.contains_ip_address(ip) { - self.data.add_ip(ip); + self.data.add_ipnet(ip); self.append_to_file(&ip.to_string()); } } @@ -117,7 +137,8 @@ mod constructor_tests { #[test] fn works_with_no_standard_hosts() { - let store = HostsStore::new(PathBuf::from("/tmp"), PathBuf::from("foomp.db")); + let temp_file = tempfile::NamedTempFile::new().unwrap(); + let store = HostsStore::new(temp_file); assert_eq!(store.data.domains.len(), 0); } } diff --git a/service-providers/network-requester/src/allowed_hosts/stored_allowed_hosts.rs b/service-providers/network-requester/src/allowed_hosts/stored_allowed_hosts.rs index d195fab8ec..20dc0499b9 100644 --- a/service-providers/network-requester/src/allowed_hosts/stored_allowed_hosts.rs +++ b/service-providers/network-requester/src/allowed_hosts/stored_allowed_hosts.rs @@ -35,6 +35,14 @@ impl StoredAllowedHosts { } } +impl From for StoredAllowedHosts { + fn from(value: HostsStore) -> Self { + StoredAllowedHosts { + inner: Arc::new(RwLock::new(value)), + } + } +} + pub(crate) struct StoredAllowedHostsReloader { stored_hosts: StoredAllowedHosts, events_receiver: FileWatcherEventReceiver,