From bd64329ed618fdbdf7130b3ca2e3a4618b184012 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 13 Jun 2024 22:33:48 +0200 Subject: [PATCH 01/20] Re-enable the cookies Stelline test. --- ...dns_downstream_cookies.rpl.not => edns_downstream_cookies.rpl} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test-data/server/{edns_downstream_cookies.rpl.not => edns_downstream_cookies.rpl} (100%) diff --git a/test-data/server/edns_downstream_cookies.rpl.not b/test-data/server/edns_downstream_cookies.rpl similarity index 100% rename from test-data/server/edns_downstream_cookies.rpl.not rename to test-data/server/edns_downstream_cookies.rpl From d6265eee016211832f269b637643d0908fe6844a Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 13 Jun 2024 22:46:35 +0200 Subject: [PATCH 02/20] Fixes to the cookies middleware processor to make the Stelline cookies test pass: - Allow requests with invalid cookies to proceed if they are authenticated or not required to authenticate. Improvements to the Stelline server test support needed by the Stelline cookies test: - Advance mock system time in the Stelline server tests. - Move Stelline server tests under src/ to permit #cfg(test) based swap out of real system time for mock system time. - Use the thread_local version of mock_instant to ensure parallel mock time dependent tests don't interfere with each other (such tests also use tokio::time which only works if they run in a single thread). - Set mock system time to start at zero for each Stelline server test (as expected by the cookies .rpl test script). - Pass the IP address of the test client to the server so that the cookies middleware can match it against its deny list. Other: - Add support for net blocks in the IP deny list of the cookie middleware processor, ala Unbound, otherwise the deny list is difficult to use beyond a few simple speciifc IP addresses. - Enable tracing log output in the server Stelline tests. - Move unit server tests and Stelline server tests together under src/net/server/tests/. - Additional logging. --- Cargo.toml | 22 +-- examples/server-transports.rs | 10 +- src/base/serial.rs | 2 +- src/net/client/validator_test.rs | 2 +- .../server/middleware/processors/cookies.rs | 145 +++++++++++++----- src/net/server/middleware/processors/mod.rs | 1 - .../net/server/tests/integration.rs | 99 ++++++------ src/net/server/tests/mod.rs | 3 + src/net/server/{tests.rs => tests/unit.rs} | 11 +- src/stelline/channel.rs | 55 +++++-- src/stelline/client.rs | 24 ++- 11 files changed, 245 insertions(+), 129 deletions(-) rename tests/net-server.rs => src/net/server/tests/integration.rs (84%) create mode 100644 src/net/server/tests/mod.rs rename src/net/server/{tests.rs => tests/unit.rs} (98%) diff --git a/Cargo.toml b/Cargo.toml index 9b343aa69..0946bd5eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,9 +17,9 @@ name = "domain" path = "src/lib.rs" [dependencies] -octseq = { version = "0.5.2-dev", git = "https://github.com/NLnetLabs/octseq.git", rev ="3f7797f4274af0a52e66105250ee1186ff2ab6ac", default-features = false } -time = { version = "0.3.1", default-features = false } - +octseq = { version = "0.5.2-dev", git = "https://github.com/NLnetLabs/octseq.git", rev ="3f7797f4274af0a52e66105250ee1186ff2ab6ac", default-features = false } +time = { version = "0.3.1", default-features = false } +ipnetwork = { version = "0.20.0", optional = true } rand = { version = "0.8", optional = true } arc-swap = { version = "1.7.0", optional = true } bytes = { version = "1.0", optional = true, default-features = false } @@ -41,7 +41,7 @@ tracing = { version = "0.1.40", optional = true } tracing-subscriber = { version = "0.3.18", optional = true, features = ["env-filter"] } # For testing in integration tests: -mock_instant = { version = "0.4.0", optional = true } +mock_instant = { version = "0.5.1", optional = true } [features] default = ["std", "rand"] @@ -59,20 +59,12 @@ validate = ["bytes", "std", "ring"] zonefile = ["bytes", "serde", "std"] # Unstable features -unstable-client-transport = [ "moka", "net", "tracing" ] -unstable-server-transport = ["arc-swap", "chrono/clock", "libc", "net", "tracing"] -unstable-stelline = ["tokio/test-util", "tracing", "tracing-subscriber", "unstable-server-transport", "zonefile"] +unstable-client-transport = ["moka", "net", "tracing"] +unstable-server-transport = ["arc-swap", "chrono/clock", "libc", "net", "siphasher", "tracing"] +unstable-stelline = ["tokio/test-util", "tracing", "tracing-subscriber", "unstable-server-transport", "zonefile", "mock_instant"] unstable-validator = ["validate", "zonefile", "unstable-client-transport"] unstable-zonetree = ["futures", "parking_lot", "serde", "tokio", "tracing"] -# Test features -# Commented out as using --all-features to build would cause mock time to also -# be used. We plan to move the test code under src/ and then use #[cfg(test)] -# and this will no longer be needed. The cookies test that depends on this is -# currently also disabled by being renamed to .rpl.not so it is okay to comment -# this out. -#mock-time = ["mock_instant"] - [dev-dependencies] lazy_static = { version = "1.4.0" } rstest = "0.19.0" diff --git a/examples/server-transports.rs b/examples/server-transports.rs index 3adc6d416..091bdf46e 100644 --- a/examples/server-transports.rs +++ b/examples/server-transports.rs @@ -33,7 +33,6 @@ use domain::net::server::dgram::DgramServer; use domain::net::server::message::Request; use domain::net::server::middleware::builder::MiddlewareBuilder; use domain::net::server::middleware::processor::MiddlewareProcessor; -#[cfg(feature = "siphasher")] use domain::net::server::middleware::processors::cookies::CookiesMiddlewareProcessor; use domain::net::server::middleware::processors::mandatory::MandatoryMiddlewareProcessor; use domain::net::server::service::{ @@ -688,12 +687,9 @@ async fn main() { let mut fn_svc_middleware = MiddlewareBuilder::new(); fn_svc_middleware.push(MandatoryMiddlewareProcessor::new().into()); - #[cfg(feature = "siphasher")] - { - let server_secret = "server12secret34".as_bytes().try_into().unwrap(); - fn_svc_middleware - .push(CookiesMiddlewareProcessor::new(server_secret).into()); - } + let server_secret = "server12secret34".as_bytes().try_into().unwrap(); + fn_svc_middleware + .push(CookiesMiddlewareProcessor::new(server_secret).into()); let fn_svc_middleware = fn_svc_middleware.build(); diff --git a/src/base/serial.rs b/src/base/serial.rs index b325057db..9bb91b46e 100644 --- a/src/base/serial.rs +++ b/src/base/serial.rs @@ -13,7 +13,7 @@ use chrono::{DateTime, TimeZone}; use core::cmp::Ordering; use core::{cmp, fmt, str}; #[cfg(all(feature = "std", test))] -use mock_instant::{SystemTime, UNIX_EPOCH}; +use mock_instant::thread_local::{SystemTime, UNIX_EPOCH}; use octseq::parse::Parser; #[cfg(all(feature = "std", not(test)))] use std::time::{SystemTime, UNIX_EPOCH}; diff --git a/src/net/client/validator_test.rs b/src/net/client/validator_test.rs index 4c000dba9..6b8a587e9 100644 --- a/src/net/client/validator_test.rs +++ b/src/net/client/validator_test.rs @@ -14,7 +14,7 @@ use crate::stelline::connect::Connect; use crate::stelline::parse_stelline::parse_file; use crate::stelline::parse_stelline::Config; -use mock_instant::MockClock; +use mock_instant::thread_local::MockClock; use rstest::rstest; use tracing::instrument; diff --git a/src/net/server/middleware/processors/cookies.rs b/src/net/server/middleware/processors/cookies.rs index f02630c45..6643ce091 100644 --- a/src/net/server/middleware/processors/cookies.rs +++ b/src/net/server/middleware/processors/cookies.rs @@ -1,9 +1,12 @@ //! DNS Cookies related message processing. use core::ops::ControlFlow; +use core::str::FromStr; use std::net::IpAddr; +use std::string::{String, ToString}; use std::vec::Vec; +use ipnetwork::IpNetwork; use octseq::Octets; use rand::RngCore; use tracing::{debug, trace, warn}; @@ -18,6 +21,8 @@ use crate::net::server::middleware::processor::MiddlewareProcessor; use crate::net::server::util::add_edns_options; use crate::net::server::util::{mk_builder_for_target, start_reply}; +//----------- Constants ------------------------------------------------------- + /// The five minute period referred to by /// https://www.rfc-editor.org/rfc/rfc9018.html#section-4.3. const FIVE_MINUTES_AS_SECS: u32 = 5 * 60; @@ -26,6 +31,38 @@ const FIVE_MINUTES_AS_SECS: u32 = 5 * 60; /// https://www.rfc-editor.org/rfc/rfc9018.html#section-4.3. const ONE_HOUR_AS_SECS: u32 = 60 * 60; +//----------- NetBlock -------------------------------------------------------- + +/// An IPv4 or IPv6 network range. +/// +// Note: Using a wrapper type avoids exposing the 3rd party IpNetwork type in +// our public API so that we can swap it out later for an alternative if +// needed without impacting the public API. +#[derive(Clone, Debug)] +pub struct NetBlock(IpNetwork); + +impl NetBlock { + /// Is the given IP address part of this network range? + fn contains(&self, ip: IpAddr) -> bool { + self.0.contains(ip) + } +} + +//--- FromStr + +impl FromStr for NetBlock { + type Err = String; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + Ok(NetBlock( + IpNetwork::from_str(s) + .map_err(|err| ToString::to_string(&err))?, + )) + } +} + +//----------- CookiesMiddlewareProcessor -------------------------------------- + /// A DNS Cookies [`MiddlewareProcessor`]. /// /// Standards covered by ths implementation: @@ -46,7 +83,7 @@ pub struct CookiesMiddlewareProcessor { /// Clients connecting from these IP addresses will be required to provide /// a cookie otherwise they will receive REFUSED with TC=1 prompting them /// to reconnect with TCP in order to "authenticate" themselves. - ip_deny_list: Vec<IpAddr>, + deny_list: Vec<NetBlock>, } impl CookiesMiddlewareProcessor { @@ -55,35 +92,36 @@ impl CookiesMiddlewareProcessor { pub fn new(server_secret: [u8; 16]) -> Self { Self { server_secret, - ip_deny_list: vec![], + deny_list: vec![], } } /// Define IP addresses required to supply DNS cookies if using UDP. #[must_use] - pub fn with_denied_ips<T: Into<Vec<IpAddr>>>( + pub fn with_denied_addresses<T: Into<Vec<NetBlock>>>( mut self, - ip_deny_list: T, + deny_list: T, ) -> Self { - self.ip_deny_list = ip_deny_list.into(); + self.deny_list = deny_list.into(); self } } impl CookiesMiddlewareProcessor { - /// Get the DNS COOKIE, if any, for the given message. + /// Get the DNS cookie, if any, for the given message. /// - /// https://datatracker.ietf.org/doc/html/rfc7873#section-5.2: Responding - /// to a Request: "In all cases of multiple COOKIE options in a request, - /// only the first (the one closest to the DNS header) is considered. - /// All others are ignored." + /// https://datatracker.ietf.org/doc/html/rfc7873#section-5.2 + /// 5.2 Responding to a Request + /// "In all cases of multiple COOKIE options in a request, only the + /// first (the one closest to the DNS header) is considered. All others + /// are ignored." /// /// Returns: - /// - `None` if the request has no cookie, - /// - Some(Ok(cookie)) if the request has a cookie in the correct - /// format, - /// - Some(Err(err)) if the request has a cookie that we could not - /// parse. + /// - None if the request has no cookie, + /// - Some(Ok(cookie)) if the first cookie in the request could be + /// parsed. + /// - Some(Err(err)) if the first cookie in the request could not be + /// parsed. #[must_use] fn cookie<RequestOctets: Octets>( request: &Request<RequestOctets>, @@ -117,7 +155,15 @@ impl CookiesMiddlewareProcessor { let now = Serial::now(); let too_new_at = now.add(FIVE_MINUTES_AS_SECS); let expires_at = serial.add(ONE_HOUR_AS_SECS); - now <= expires_at && serial <= too_new_at + if now > expires_at { + trace!("Invalid server cookie: cookie has expired ({now} > {expires_at})"); + false + } else if serial > too_new_at { + trace!("Invalid server cookie: cookie is too new ({serial} > {too_new_at})"); + false + } else { + true + } } /// Create a DNS response message for the given request, including cookie. @@ -203,6 +249,14 @@ impl CookiesMiddlewareProcessor { // Cookie, the response SHALL have the RCODE NOERROR." self.response_with_cookie(request, Rcode::NOERROR.into()) } + + /// Is the given IP address required to authenticate itself? + /// + /// If the given IP address is on our deny list it is required to + /// authenticate itself. + fn must_authenticate(&self, ip: IpAddr) -> bool { + self.deny_list.iter().any(|netblock| netblock.contains(ip)) + } } //--- Default @@ -217,7 +271,7 @@ impl Default for CookiesMiddlewareProcessor { Self { server_secret, - ip_deny_list: Default::default(), + deny_list: Default::default(), } } } @@ -230,6 +284,7 @@ where RequestOctets: Octets, Target: Composer + Default, { + #[tracing::instrument(skip_all, fields(request_ip = %request.client_addr().ip()))] fn preprocess( &self, request: &Request<RequestOctets>, @@ -245,24 +300,31 @@ where // the request as if the server doesn't implement the // COOKIE option." - // For clients on the IP deny list they MUST authenticate - // themselves to the server, either with a cookie or by - // re-connecting over TCP, so we REFUSE them and reply with - // TC=1 to prompt them to reconnect via TCP. + // https://datatracker.ietf.org/doc/html/rfc7873#section-1 + // 1. Introduction + // "The protection provided by DNS Cookies is similar to + // that provided by using TCP for DNS transactions. + // ... + // Where DNS Cookies are not available but TCP is, falling + // back to using TCP is reasonable." + + // While not required by RFC 7873, like Unbound the caller can + // configure this middleware processor to require clients + // contacting it from certain IP addresses or ranges to + // authenticate themselves or be refused with TC=1 to signal + // that they should resubmit their request via TCP. if request.transport_ctx().is_udp() - && self.ip_deny_list.contains(&request.client_addr().ip()) + && self.must_authenticate(request.client_addr().ip()) { - debug!( - "Rejecting cookie-less non-TCP request due to matching IP deny list entry" - ); + debug!("Rejecting cookie-less non-TCP request due to matching deny list entry"); let builder = mk_builder_for_target(); let mut additional = builder.additional(); additional.header_mut().set_rcode(Rcode::REFUSED); additional.header_mut().set_tc(true); return ControlFlow::Break(additional); - } else { - trace!("Permitting cookie-less request to flow due to use of TCP transport"); } + + // Continue as if we we don't implement the COOKIE option. } Some(Err(err)) => { @@ -305,6 +367,8 @@ where ); if !server_cookie_is_valid { + trace!("Request has an invalid DNS server cookie"); + // https://datatracker.ietf.org/doc/html/rfc7873#section-5.2.3 // Only a Client Cookie: // "Based on server policy, including rate limiting, the @@ -379,10 +443,11 @@ where self.bad_cookie_response(request) }; return ControlFlow::Break(additional); - } else if request.transport_ctx().is_udp() { + } else if request.transport_ctx().is_udp() + && self.must_authenticate(request.client_addr().ip()) + { let additional = self.bad_cookie_response(request); - debug!( - "Rejecting non-TCP request due to invalid server cookie"); + debug!("Rejecting non-TCP request with invalid server cookie due to matching deny list entry"); return ControlFlow::Break(additional); } } else if request.message().header_counts().qdcount() == 0 { @@ -460,9 +525,17 @@ mod tests { use crate::net::server::middleware::processor::MiddlewareProcessor; use super::CookiesMiddlewareProcessor; + use tracing::Level; #[test] fn dont_add_cookie_twice() { + tracing_subscriber::fmt() + .with_max_level(Level::TRACE) + .with_thread_ids(true) + .without_time() + .try_init() + .ok(); + // Build a dummy DNS query containing a client cookie. let query = MessageBuilder::new_vec(); let mut query = query.question(); @@ -476,17 +549,19 @@ mod tests { // Package the query into a context aware request to make it look // as if it came from a UDP server. let ctx = UdpTransportContext::default(); - let client_addr = "127.0.0.1:12345".parse().unwrap(); + let client_addr = "127.0.0.18:12345".parse().unwrap(); let request = Request::new(client_addr, Instant::now(), message, ctx.into()); - // And pass the query through the middleware processor - let server_secret: [u8; 16] = - [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]; - let processor = CookiesMiddlewareProcessor::new(server_secret); + // Setup the cookie middleware processor such that it requires + // the mock client to provide a valid cookie. + let server_secret: [u8; 16] = [1u8; 16]; + let processor = CookiesMiddlewareProcessor::new(server_secret) + .with_denied_addresses(["127.0.0.1/24".parse().unwrap()]); let processor: &dyn MiddlewareProcessor<Vec<u8>, Vec<u8>> = &processor; + // And pass the query through the middleware processor let ControlFlow::Break(mut response) = processor.preprocess(&request) else { unreachable!() diff --git a/src/net/server/middleware/processors/mod.rs b/src/net/server/middleware/processors/mod.rs index 18635c239..a2df774f6 100644 --- a/src/net/server/middleware/processors/mod.rs +++ b/src/net/server/middleware/processors/mod.rs @@ -1,7 +1,6 @@ //! Pre-supplied [`MiddlewareProcessor`] implementations. //! //! [`MiddlewareProcessor`]: super::processor::MiddlewareProcessor -#[cfg(feature = "siphasher")] pub mod cookies; pub mod edns; pub mod mandatory; diff --git a/tests/net-server.rs b/src/net/server/tests/integration.rs similarity index 84% rename from tests/net-server.rs rename to src/net/server/tests/integration.rs index d9959ed80..b5c277946 100644 --- a/tests/net-server.rs +++ b/src/net/server/tests/integration.rs @@ -1,47 +1,48 @@ -#![cfg(feature = "net")] +use core::net::SocketAddr; +use std::boxed::Box; use std::collections::VecDeque; use std::fs::File; use std::future::Future; -use std::net::IpAddr; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; +use std::vec::Vec; use octseq::Octets; use rstest::rstest; use tracing::instrument; use tracing::{trace, warn}; -use domain::base::iana::Rcode; -use domain::base::name::{Name, ToName}; -use domain::base::wire::Composer; -use domain::net::client::{dgram, stream}; -use domain::net::server::buf::VecBufSource; -use domain::net::server::dgram::DgramServer; -use domain::net::server::message::Request; -use domain::net::server::middleware::builder::MiddlewareBuilder; -#[cfg(feature = "siphasher")] -use domain::net::server::middleware::processors::cookies::CookiesMiddlewareProcessor; -use domain::net::server::middleware::processors::edns::EdnsMiddlewareProcessor; -use domain::net::server::service::{ +use crate::base::iana::Rcode; +use crate::base::name::{Name, ToName}; +use crate::base::wire::Composer; +use crate::net::client::{dgram, stream}; +use crate::net::server::buf::VecBufSource; +use crate::net::server::dgram::DgramServer; +use crate::net::server::message::Request; +use crate::net::server::middleware::builder::MiddlewareBuilder; +use crate::net::server::middleware::processors::cookies::{ + CookiesMiddlewareProcessor, NetBlock, +}; +use crate::net::server::middleware::processors::edns::EdnsMiddlewareProcessor; +use crate::net::server::service::{ CallResult, Service, ServiceError, Transaction, }; -use domain::net::server::stream::StreamServer; -use domain::net::server::util::{mk_builder_for_target, service_fn}; -use domain::utils::base16; -use domain::zonefile::inplace::{Entry, ScannedRecord, Zonefile}; - -use domain::stelline::channel::ClientServerChannel; -use domain::stelline::client::do_client; -use domain::stelline::client::ClientFactory; -use domain::stelline::client::{ +use crate::net::server::stream::StreamServer; +use crate::net::server::util::{mk_builder_for_target, service_fn}; +use crate::stelline::channel::ClientServerChannel; +use crate::stelline::client::do_client; +use crate::stelline::client::ClientFactory; +use crate::stelline::client::{ CurrStepValue, PerClientAddressClientFactory, QueryTailoredClientFactory, }; -use domain::stelline::parse_stelline; -use domain::stelline::parse_stelline::parse_file; -use domain::stelline::parse_stelline::Config; -use domain::stelline::parse_stelline::Matches; +use crate::stelline::parse_stelline; +use crate::stelline::parse_stelline::parse_file; +use crate::stelline::parse_stelline::Config; +use crate::stelline::parse_stelline::Matches; +use crate::utils::base16; +use crate::zonefile::inplace::{Entry, ScannedRecord, Zonefile}; //----------- Tests ---------------------------------------------------------- @@ -59,6 +60,16 @@ async fn server_tests(#[files("test-data/server/*.rpl")] rpl_file: PathBuf) { // and which responses will be expected, and how the server that // answers them should be configured. + // Initialize tracing based logging. Override with env var RUST_LOG, e.g. + // RUST_LOG=trace. DEBUG level will show the .rpl file name, Stelline step + // numbers and types as they are being executed. + tracing_subscriber::fmt() + .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) + .with_thread_ids(true) + .without_time() + .try_init() + .ok(); + let file = File::open(&rpl_file).unwrap(); let stelline = parse_file(&file, rpl_file.to_str().unwrap()); let server_config = parse_server_config(&stelline.config); @@ -155,8 +166,9 @@ fn mk_client_factory( }; let tcp_client_factory = PerClientAddressClientFactory::new( - move |_source_addr| { - let stream = stream_server_conn.connect(); + move |source_addr| { + let stream = stream_server_conn + .connect(Some(SocketAddr::new(*source_addr, 0))); let (conn, transport) = stream::Connection::new(stream); tokio::spawn(transport.run()); Box::new(conn) @@ -169,7 +181,12 @@ fn mk_client_factory( let for_all_other_queries = |_: &_| true; let udp_client_factory = PerClientAddressClientFactory::new( - move |_| Box::new(dgram::Connection::new(dgram_server_conn.clone())), + move |source_addr| { + Box::new(dgram::Connection::new( + dgram_server_conn + .new_client(Some(SocketAddr::new(*source_addr, 0))), + )) + }, for_all_other_queries, ); @@ -185,8 +202,8 @@ fn mk_client_factory( fn mk_server_configs<RequestOctets, Target>( config: &ServerConfig, ) -> ( - domain::net::server::dgram::Config<RequestOctets, Target>, - domain::net::server::stream::Config<RequestOctets, Target>, + crate::net::server::dgram::Config<RequestOctets, Target>, + crate::net::server::stream::Config<RequestOctets, Target>, ) where RequestOctets: Octets, @@ -195,18 +212,14 @@ where let mut middleware = MiddlewareBuilder::minimal(); if config.cookies.enabled { - #[cfg(feature = "siphasher")] if let Some(secret) = config.cookies.secret { let secret = base16::decode_vec(secret).unwrap(); let secret = <[u8; 16]>::try_from(secret).unwrap(); let processor = CookiesMiddlewareProcessor::new(secret); let processor = processor - .with_denied_ips(config.cookies.ip_deny_list.clone()); + .with_denied_addresses(config.cookies.deny_list.clone()); middleware.push(processor.into()); } - - #[cfg(not(feature = "siphasher"))] - panic!("The test uses cookies but the required 'siphasher' feature is not enabled."); } if config.edns_tcp_keepalive { @@ -216,13 +229,13 @@ where let middleware = middleware.build(); - let mut dgram_config = domain::net::server::dgram::Config::default(); + let mut dgram_config = crate::net::server::dgram::Config::default(); dgram_config.set_middleware_chain(middleware.clone()); - let mut stream_config = domain::net::server::stream::Config::default(); + let mut stream_config = crate::net::server::stream::Config::default(); if let Some(idle_timeout) = config.idle_timeout { let mut connection_config = - domain::net::server::ConnectionConfig::default(); + crate::net::server::ConnectionConfig::default(); connection_config.set_idle_timeout(idle_timeout); connection_config.set_middleware_chain(middleware); stream_config.set_connection_config(connection_config); @@ -263,7 +276,7 @@ fn test_service( } fn as_records( - e: Result<Entry, domain::zonefile::inplace::Error>, + e: Result<Entry, crate::zonefile::inplace::Error>, ) -> Option<ScannedRecord> { match e { Ok(Entry::Record(r)) => Some(r), @@ -328,7 +341,7 @@ struct ServerConfig<'a> { struct CookieConfig<'a> { enabled: bool, secret: Option<&'a str>, - ip_deny_list: Vec<IpAddr>, + deny_list: Vec<NetBlock>, } fn parse_server_config(config: &Config) -> ServerConfig { @@ -373,7 +386,7 @@ fn parse_server_config(config: &Config) -> ServerConfig { if let Ok(ip) = ip.parse() { parsed_config .cookies - .ip_deny_list + .deny_list .push(ip); } else { eprintln!("Ignoring malformed IP address '{ip}' in 'access-control' setting"); diff --git a/src/net/server/tests/mod.rs b/src/net/server/tests/mod.rs new file mode 100644 index 000000000..f4e60adc5 --- /dev/null +++ b/src/net/server/tests/mod.rs @@ -0,0 +1,3 @@ +#![cfg(all(feature = "net", test))] +mod integration; +mod unit; diff --git a/src/net/server/tests.rs b/src/net/server/tests/unit.rs similarity index 98% rename from src/net/server/tests.rs rename to src/net/server/tests/unit.rs index 6ac63da98..e728abcf1 100644 --- a/src/net/server/tests.rs +++ b/src/net/server/tests/unit.rs @@ -20,14 +20,13 @@ use crate::base::Name; use crate::base::Rtype; use crate::base::StaticCompressor; use crate::base::StreamTarget; - -use super::buf::BufSource; -use super::message::Request; -use super::service::{ +use crate::net::server::buf::BufSource; +use crate::net::server::message::Request; +use crate::net::server::service::{ CallResult, Service, ServiceError, ServiceFeedback, Transaction, }; -use super::sock::AsyncAccept; -use super::stream::StreamServer; +use crate::net::server::sock::AsyncAccept; +use crate::net::server::stream::StreamServer; /// Mock I/O which supplies a sequence of mock messages to the server at a /// defined rate. diff --git a/src/stelline/channel.rs b/src/stelline/channel.rs index 5076c3843..9498b264b 100644 --- a/src/stelline/channel.rs +++ b/src/stelline/channel.rs @@ -28,7 +28,7 @@ pub const DEF_CLIENT_ADDR: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST); pub const DEF_CLIENT_PORT: u16 = 0; enum Data { - DgramRequest(Vec<u8>), + DgramRequest(SocketAddr, Vec<u8>), StreamAccept(ClientServerChannel), StreamRequest(Vec<u8>), } @@ -152,7 +152,6 @@ impl ServerSocket { } } -#[derive(Default)] pub struct ClientServerChannel { /// Details of the server end of the connection. server: Arc<Mutex<ServerSocket>>, @@ -160,10 +159,26 @@ pub struct ClientServerChannel { /// Details of the client end of the connection, if connected. client: Option<ClientSocket>, + /// Simulated client address. + client_addr: SocketAddr, + /// Type of connection. is_stream: bool, } +impl Default for ClientServerChannel { + fn default() -> Self { + let client_addr = SocketAddr::new("::".parse().unwrap(), 0); + + Self { + server: Default::default(), + client: Default::default(), + client_addr, + is_stream: Default::default(), + } + } +} + impl Clone for ClientServerChannel { /// Clones only the server half, the client half cannot be cloned. The /// result can be used to connect a new client to an existing server. @@ -171,6 +186,7 @@ impl Clone for ClientServerChannel { Self { server: self.server.clone(), client: None, + client_addr: self.client_addr, is_stream: self.is_stream, } } @@ -191,7 +207,18 @@ impl ClientServerChannel { } } - pub fn connect(&self) -> Self { + pub fn new_client(&self, client_addr: Option<SocketAddr>) -> Self { + let client_addr = client_addr + .unwrap_or_else(|| SocketAddr::new("::".parse().unwrap(), 0)); + Self { + server: self.server.clone(), + client: None, + is_stream: self.is_stream, + client_addr, + } + } + + pub fn connect(&self, client_addr: Option<SocketAddr>) -> Self { fn setup_client(server_socket: &mut ServerSocket) -> ClientSocket { // Create a client socket for sending requests to the server. let (client, response_tx) = @@ -204,6 +231,9 @@ impl ClientServerChannel { client } + let client_addr = client_addr + .unwrap_or_else(|| SocketAddr::new("::".parse().unwrap(), 0)); + match self.is_stream { false => { // For dgram connections all clients communicate with the same @@ -215,6 +245,7 @@ impl ClientServerChannel { Self { server: self.server.clone(), client: Some(client), + client_addr: self.client_addr, is_stream: false, } } @@ -229,6 +260,7 @@ impl ClientServerChannel { let channel = Self { server: Arc::new(Mutex::new(server_socket)), client: Some(client), + client_addr: self.client_addr, is_stream: true, }; @@ -236,9 +268,10 @@ impl ClientServerChannel { // by unblocking AsyncAccept::poll_accept() which is being polled // by the server. let sender = self.server.lock().unwrap().tx.clone(); - let cloned_channel = channel.clone(); + let channel_for_client = + channel.new_client(Some(client_addr)); tokio::spawn(async move { - sender.send(Data::StreamAccept(cloned_channel)).await + sender.send(Data::StreamAccept(channel_for_client)).await }); channel @@ -263,8 +296,7 @@ impl AsyncConnect for ClientServerChannel { >; fn connect(&self) -> Self::Fut { - let conn = self.connect(); - + let conn = self.connect(None); Box::pin(async move { Ok(conn) }) } } @@ -309,7 +341,7 @@ impl AsyncDgramSend for ClientServerChannel { ) -> Poll<Result<usize, io::Error>> { match &self.client { Some(client) => { - let msg = Data::DgramRequest(data.into()); + let msg = Data::DgramRequest(self.client_addr, data.into()); // TODO: Can Stelline scripts mix and match fake responses with // responses from a real server? Do we need to first try @@ -407,12 +439,11 @@ impl AsyncDgramSock for ClientServerChannel { let mut server_socket = self.server.lock().unwrap(); let rx = &mut server_socket.rx; match rx.try_recv() { - Ok(Data::DgramRequest(data)) => { + Ok(Data::DgramRequest(addr, data)) => { // TODO: use unread buf here to prevent overflow of given buf. - trace!("Reading {} bytes into buffer of len {} in dgram server channel", data.len(), buf.remaining()); + trace!("Reading {} bytes from {addr} into buffer of len {} in dgram server channel", data.len(), buf.remaining()); buf.put_slice(&data); - let socket_addr = SocketAddr::new("::".parse().unwrap(), 0); - Ok((data.len(), socket_addr)) + Ok((data.len(), addr)) } Ok(Data::StreamAccept(..)) => unreachable!(), Ok(Data::StreamRequest(..)) => unreachable!(), diff --git a/src/stelline/client.rs b/src/stelline/client.rs index d32386dab..c1e5e3577 100644 --- a/src/stelline/client.rs +++ b/src/stelline/client.rs @@ -10,10 +10,8 @@ use std::time::Duration; use std::vec::Vec; use bytes::Bytes; -/* -#[cfg(feature = "mock-time")] -use mock_instant::MockClock; -*/ +#[cfg(all(feature = "std", test))] +use mock_instant::thread_local::MockClock; use tracing::{debug, info_span, trace}; use tracing_subscriber::EnvFilter; @@ -377,6 +375,12 @@ pub async fn do_client<'a, T: ClientFactory>( ) -> Result<(), StellineErrorCause> { let mut resp: Option<Message<Bytes>> = None; + #[cfg(all(feature = "std", test))] + { + trace!("Setting mock system time to zero."); + MockClock::set_system_time(Duration::ZERO); + } + // Assume steps are in order. Maybe we need to define that. for step in &stelline.scenario.steps { let span = @@ -430,10 +434,14 @@ pub async fn do_client<'a, T: ClientFactory>( let duration = Duration::from_secs(step.time_passes.unwrap()); tokio::time::advance(duration).await; - /* - #[cfg(feature = "mock-time")] - MockClock::advance_system_time(duration); - */ + #[cfg(all(feature = "std", test))] + { + trace!( + "Advancing mock system time by {} seconds...", + duration.as_secs() + ); + MockClock::advance_system_time(duration); + } } StepType::Traffic | StepType::CheckTempfile From 35ca1c9e5cd23fa9ac95e23ee41b784010301809 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Fri, 14 Jun 2024 01:13:03 +0200 Subject: [PATCH 03/20] Increase log level to investigate MacOS failure. --- src/net/server/tests/integration.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/net/server/tests/integration.rs b/src/net/server/tests/integration.rs index b5c277946..f2084a4fc 100644 --- a/src/net/server/tests/integration.rs +++ b/src/net/server/tests/integration.rs @@ -63,8 +63,10 @@ async fn server_tests(#[files("test-data/server/*.rpl")] rpl_file: PathBuf) { // Initialize tracing based logging. Override with env var RUST_LOG, e.g. // RUST_LOG=trace. DEBUG level will show the .rpl file name, Stelline step // numbers and types as they are being executed. + + use tracing::Level; tracing_subscriber::fmt() - .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) + .with_max_level(Level::TRACE) .with_thread_ids(true) .without_time() .try_init() From 5f522f6c533744c766607df83c5857141b7a8f21 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Fri, 14 Jun 2024 08:55:26 +0200 Subject: [PATCH 04/20] Increase log level to investigate MacOS failure. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 393ddc800..fb1aa1c05 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,7 +24,7 @@ jobs: - if: matrix.rust == 'stable' && matrix.os == 'ubuntu-latest' run: cargo fmt --all -- --check - run: cargo check --no-default-features --all-targets - - run: cargo test --all-features + - run: RUST_LOG=trace cargo test --all-features -- --show-output --nocapture - if: matrix.rust == 'nightly' run: | cargo +nightly update -Z minimal-versions From 2d5dc32192d354ac661264077eb56a2e623a9479 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Fri, 14 Jun 2024 09:00:09 +0200 Subject: [PATCH 05/20] Temporary workaround for new nightly rust error: this function depends on never type fallback being `()` --- src/rdata/svcb/value.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rdata/svcb/value.rs b/src/rdata/svcb/value.rs index 9a175233d..9130f3339 100644 --- a/src/rdata/svcb/value.rs +++ b/src/rdata/svcb/value.rs @@ -709,6 +709,7 @@ impl<Target> AlpnBuilder<Target> { /// /// Returns an error if the name is too long or the ALPN value would /// become too long or the underlying octets builder runs out of space. + // #[allow(dependency_on_unit_never_type_fallback)] // Temporary work around pub fn push( &mut self, protocol: impl AsRef<[u8]> ) -> Result<(), BuildAlpnError> From 7d71767ce70e1f44a833c365ebfa2f1ca340a4ce Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Fri, 14 Jun 2024 09:01:21 +0200 Subject: [PATCH 06/20] Temporary workaround for new nightly rust error: this function depends on never type fallback being `()` --- src/rdata/svcb/value.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rdata/svcb/value.rs b/src/rdata/svcb/value.rs index 9130f3339..89c74a4fd 100644 --- a/src/rdata/svcb/value.rs +++ b/src/rdata/svcb/value.rs @@ -709,7 +709,7 @@ impl<Target> AlpnBuilder<Target> { /// /// Returns an error if the name is too long or the ALPN value would /// become too long or the underlying octets builder runs out of space. - // #[allow(dependency_on_unit_never_type_fallback)] // Temporary work around + #[allow(dependency_on_unit_never_type_fallback)] // Temporary work around pub fn push( &mut self, protocol: impl AsRef<[u8]> ) -> Result<(), BuildAlpnError> From 47abac577c8d398d3ffcc6d074bee71e4baa8154 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Sat, 15 Jun 2024 14:28:45 +0200 Subject: [PATCH 07/20] Revert temporay workaround. --- src/rdata/svcb/value.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rdata/svcb/value.rs b/src/rdata/svcb/value.rs index 89c74a4fd..9a175233d 100644 --- a/src/rdata/svcb/value.rs +++ b/src/rdata/svcb/value.rs @@ -709,7 +709,6 @@ impl<Target> AlpnBuilder<Target> { /// /// Returns an error if the name is too long or the ALPN value would /// become too long or the underlying octets builder runs out of space. - #[allow(dependency_on_unit_never_type_fallback)] // Temporary work around pub fn push( &mut self, protocol: impl AsRef<[u8]> ) -> Result<(), BuildAlpnError> From 0570d2f7d225aea03f9b072cc98f1612788d6037 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Sat, 15 Jun 2024 17:00:41 +0200 Subject: [PATCH 08/20] Fix a new nightly warning involving never and unit types. --- src/rdata/svcb/value.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rdata/svcb/value.rs b/src/rdata/svcb/value.rs index 9a175233d..371c631c3 100644 --- a/src/rdata/svcb/value.rs +++ b/src/rdata/svcb/value.rs @@ -725,7 +725,7 @@ impl<Target> AlpnBuilder<Target> { protocol.len() + 1 ).expect("long Alpn value") ).map_err(|_| BuildAlpnError::LongSvcParam)?; - len.compose(&mut self.target).map(Into::into)?; + len.compose(&mut self.target)?; self.target.append_slice( protocol ).map_err(|_| BuildAlpnError::ShortBuf) From dd226dcb515146f04edb6ab4554c8329d9ea7d13 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Sat, 15 Jun 2024 20:37:34 +0200 Subject: [PATCH 09/20] Update ci.yml --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb1aa1c05..5bc1e21b4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,7 @@ jobs: rust: [1.78.0, stable, beta, nightly] env: RUSTFLAGS: "-D warnings" + RUST_LOG: "trace" steps: - name: Checkout repository uses: actions/checkout@v1 @@ -24,7 +25,7 @@ jobs: - if: matrix.rust == 'stable' && matrix.os == 'ubuntu-latest' run: cargo fmt --all -- --check - run: cargo check --no-default-features --all-targets - - run: RUST_LOG=trace cargo test --all-features -- --show-output --nocapture + - run: cargo test --all-features -- --show-output --nocapture - if: matrix.rust == 'nightly' run: | cargo +nightly update -Z minimal-versions From 31223dcd66ac3fc4532202e3547d788d7c90d88d Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 18 Jun 2024 23:26:33 +0200 Subject: [PATCH 10/20] Fixes and improvements to Stelline mock channel code. In particular, use a task based waker instead of a thread based waker for scenarios where CPU resource is low such as in GH Actions (or when simualted with taskset -c 0). --- src/net/server/dgram.rs | 8 +++-- src/stelline/channel.rs | 74 ++++++++++++++++++++++++++--------------- src/stelline/client.rs | 2 ++ 3 files changed, 56 insertions(+), 28 deletions(-) diff --git a/src/net/server/dgram.rs b/src/net/server/dgram.rs index 59356c36d..f2c8f5422 100644 --- a/src/net/server/dgram.rs +++ b/src/net/server/dgram.rs @@ -28,6 +28,7 @@ use tokio::time::interval; use tokio::time::timeout; use tokio::time::Instant; use tokio::time::MissedTickBehavior; +use tracing::warn; use tracing::Level; use tracing::{enabled, error, trace}; @@ -720,13 +721,16 @@ where // Actually write the DNS response message bytes to the UDP // socket. - let _ = Self::send_to( + if let Err(err) = Self::send_to( &state.sock, bytes, &client_addr, state.write_timeout, ) - .await; + .await + { + warn!(%client_addr, "Failed to send response: {err}"); + } metrics.dec_num_pending_writes(); metrics.inc_num_sent_responses(); diff --git a/src/stelline/channel.rs b/src/stelline/channel.rs index 9498b264b..66f79c085 100644 --- a/src/stelline/channel.rs +++ b/src/stelline/channel.rs @@ -22,6 +22,7 @@ use crate::net::client::protocol::{ AsyncConnect, AsyncDgramRecv, AsyncDgramSend, }; use crate::net::server::sock::{AsyncAccept, AsyncDgramSock}; +use core::sync::atomic::{AtomicU16, Ordering}; // If MSRV gets bumped to 1.69.0 we can replace these with a const SocketAddr. pub const DEF_CLIENT_ADDR: IpAddr = IpAddr::V4(Ipv4Addr::LOCALHOST); @@ -124,7 +125,7 @@ struct ServerSocket { /// Senders for the server to send responses to clients. /// /// One per client to which responses must be sent. - response_txs: HashMap<(), mpsc::Sender<Vec<u8>>>, + response_txs: HashMap<SocketAddr, mpsc::Sender<Vec<u8>>>, /// Buffer for received bytes that overflowed the server read buffer. unread_buf: ReadBufBuffer, @@ -162,18 +163,22 @@ pub struct ClientServerChannel { /// Simulated client address. client_addr: SocketAddr, + /// Next mock client port number to use. + next_client_port: Arc<AtomicU16>, + /// Type of connection. is_stream: bool, } impl Default for ClientServerChannel { fn default() -> Self { - let client_addr = SocketAddr::new("::".parse().unwrap(), 0); + let client_addr = SocketAddr::new("127.0.0.1".parse().unwrap(), 0); Self { server: Default::default(), client: Default::default(), client_addr, + next_client_port: Arc::new(AtomicU16::new(1)), is_stream: Default::default(), } } @@ -187,6 +192,7 @@ impl Clone for ClientServerChannel { server: self.server.clone(), client: None, client_addr: self.client_addr, + next_client_port: self.next_client_port.clone(), is_stream: self.is_stream, } } @@ -208,44 +214,60 @@ impl ClientServerChannel { } pub fn new_client(&self, client_addr: Option<SocketAddr>) -> Self { - let client_addr = client_addr - .unwrap_or_else(|| SocketAddr::new("::".parse().unwrap(), 0)); + let mut client_addr = client_addr.unwrap_or_else(|| { + SocketAddr::new("127.0.0.1".parse().unwrap(), 0) + }); + + if client_addr.port() == 0 { + let client_port = + self.next_client_port.fetch_add(1, Ordering::SeqCst); + client_addr.set_port(client_port); + } + Self { server: self.server.clone(), client: None, - is_stream: self.is_stream, client_addr, + next_client_port: self.next_client_port.clone(), + is_stream: self.is_stream, } } pub fn connect(&self, client_addr: Option<SocketAddr>) -> Self { - fn setup_client(server_socket: &mut ServerSocket) -> ClientSocket { + fn setup_client( + server_socket: &mut ServerSocket, + client_addr: SocketAddr, + ) -> ClientSocket { // Create a client socket for sending requests to the server. let (client, response_tx) = ClientSocket::new(server_socket.sender()); // Tell the server how to respond to the client. - server_socket.response_txs.insert((), response_tx); + server_socket.response_txs.insert(client_addr, response_tx); // Return the created client socket client } - let client_addr = client_addr - .unwrap_or_else(|| SocketAddr::new("::".parse().unwrap(), 0)); + let client_addr = client_addr.unwrap_or_else(|| { + let client_port = + self.next_client_port.fetch_add(1, Ordering::SeqCst); + SocketAddr::new("127.0.0.1".parse().unwrap(), client_port) + }); match self.is_stream { false => { // For dgram connections all clients communicate with the same // single server socket. let server_socket = &mut self.server.lock().unwrap(); - let client = setup_client(server_socket); + let client = setup_client(server_socket, client_addr); // Tell the client how to contact the server. Self { server: self.server.clone(), client: Some(client), client_addr: self.client_addr, + next_client_port: self.next_client_port.clone(), is_stream: false, } } @@ -254,13 +276,14 @@ impl ClientServerChannel { // But for stream connections each new client communicates // with a new server-side connection handler socket. let mut server_socket = ServerSocket::default(); - let client = setup_client(&mut server_socket); + let client = setup_client(&mut server_socket, client_addr); // Tell the client how to contact the new server connection handler. let channel = Self { server: Arc::new(Mutex::new(server_socket)), client: Some(client), client_addr: self.client_addr, + next_client_port: self.next_client_port.clone(), is_stream: true, }; @@ -296,7 +319,7 @@ impl AsyncConnect for ClientServerChannel { >; fn connect(&self) -> Self::Fut { - let conn = self.connect(None); + let conn = self.connect(Some(self.client_addr)); Box::pin(async move { Ok(conn) }) } } @@ -322,7 +345,7 @@ impl AsyncDgramRecv for ClientServerChannel { Poll::Ready(Ok(())) } Poll::Ready(None) => { - trace!("Broken pipe while reading in dgram client channel"); + trace!("Broken pipe while reading in dgram client channel (is_closed={})", rx.is_closed()); Poll::Ready(Err(io::Error::from(io::ErrorKind::BrokenPipe))) } Poll::Pending => { @@ -390,10 +413,10 @@ impl AsyncDgramSock for ClientServerChannel { &self, cx: &mut Context, data: &[u8], - dest: &std::net::SocketAddr, + dest: &SocketAddr, ) -> Poll<io::Result<usize>> { let server_socket = self.server.lock().unwrap(); - let tx = server_socket.response_txs.get(&()); + let tx = server_socket.response_txs.get(dest); if let Some(server_tx) = tx { let mut fut = Box::pin(server_tx.send(data.to_vec())); match fut.poll_unpin(cx) { @@ -470,17 +493,16 @@ impl Future for ClientServerChannelReadableFut { ) -> Poll<Self::Output> { let server_socket = self.0.lock().unwrap(); let rx = &server_socket.rx; - trace!("ReadableFut {} in dgram server channel", !rx.is_empty()); - match !rx.is_empty() { - true => Poll::Ready(Ok(())), - false => { - let waker = cx.waker().clone(); - std::thread::spawn(move || { - std::thread::yield_now(); - waker.wake(); - }); - Poll::Pending - } + if !rx.is_empty() { + trace!("Server socket is now readable"); + Poll::Ready(Ok(())) + } else { + trace!("Server socket is not yet readable"); + let waker = cx.waker().clone(); + tokio::task::spawn(async move { + waker.wake(); + }); + Poll::Pending } } } diff --git a/src/stelline/client.rs b/src/stelline/client.rs index c1e5e3577..6d8f85ba2 100644 --- a/src/stelline/client.rs +++ b/src/stelline/client.rs @@ -414,6 +414,8 @@ pub async fn do_client<'a, T: ClientFactory>( .await; } + trace!("Receive result: {res:?}"); + resp = res?; trace!(?resp); From ce29b37bdfb56749ae4b8a236262f0be2aaf41df Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 18 Jun 2024 23:38:27 +0200 Subject: [PATCH 11/20] Revert "Update ci.yml" This reverts commit dd226dcb515146f04edb6ab4554c8329d9ea7d13. --- .github/workflows/ci.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5bc1e21b4..fb1aa1c05 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,7 +10,6 @@ jobs: rust: [1.78.0, stable, beta, nightly] env: RUSTFLAGS: "-D warnings" - RUST_LOG: "trace" steps: - name: Checkout repository uses: actions/checkout@v1 @@ -25,7 +24,7 @@ jobs: - if: matrix.rust == 'stable' && matrix.os == 'ubuntu-latest' run: cargo fmt --all -- --check - run: cargo check --no-default-features --all-targets - - run: cargo test --all-features -- --show-output --nocapture + - run: RUST_LOG=trace cargo test --all-features -- --show-output --nocapture - if: matrix.rust == 'nightly' run: | cargo +nightly update -Z minimal-versions From 4eda7cad3edce01c86fe283e61c40bd32463af27 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 18 Jun 2024 23:39:15 +0200 Subject: [PATCH 12/20] Revert "Increase log level to investigate MacOS failure." This reverts commit 5f522f6c533744c766607df83c5857141b7a8f21. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb1aa1c05..393ddc800 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,7 +24,7 @@ jobs: - if: matrix.rust == 'stable' && matrix.os == 'ubuntu-latest' run: cargo fmt --all -- --check - run: cargo check --no-default-features --all-targets - - run: RUST_LOG=trace cargo test --all-features -- --show-output --nocapture + - run: cargo test --all-features - if: matrix.rust == 'nightly' run: | cargo +nightly update -Z minimal-versions From ffd715a91085165c376a84c5561552e54c0a7d60 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 18 Jun 2024 23:39:25 +0200 Subject: [PATCH 13/20] Revert "Increase log level to investigate MacOS failure." This reverts commit 35ca1c9e5cd23fa9ac95e23ee41b784010301809. --- src/net/server/tests/integration.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/net/server/tests/integration.rs b/src/net/server/tests/integration.rs index f2084a4fc..b5c277946 100644 --- a/src/net/server/tests/integration.rs +++ b/src/net/server/tests/integration.rs @@ -63,10 +63,8 @@ async fn server_tests(#[files("test-data/server/*.rpl")] rpl_file: PathBuf) { // Initialize tracing based logging. Override with env var RUST_LOG, e.g. // RUST_LOG=trace. DEBUG level will show the .rpl file name, Stelline step // numbers and types as they are being executed. - - use tracing::Level; tracing_subscriber::fmt() - .with_max_level(Level::TRACE) + .with_env_filter(tracing_subscriber::EnvFilter::from_default_env()) .with_thread_ids(true) .without_time() .try_init() From 6207d2368eb9e9a44522d0af2852797815e7bf09 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Tue, 18 Jun 2024 23:42:45 +0200 Subject: [PATCH 14/20] Remove temporarily added logging. --- src/net/server/middleware/processors/cookies.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/net/server/middleware/processors/cookies.rs b/src/net/server/middleware/processors/cookies.rs index 6643ce091..8c1328751 100644 --- a/src/net/server/middleware/processors/cookies.rs +++ b/src/net/server/middleware/processors/cookies.rs @@ -525,17 +525,9 @@ mod tests { use crate::net::server::middleware::processor::MiddlewareProcessor; use super::CookiesMiddlewareProcessor; - use tracing::Level; #[test] fn dont_add_cookie_twice() { - tracing_subscriber::fmt() - .with_max_level(Level::TRACE) - .with_thread_ids(true) - .without_time() - .try_init() - .ok(); - // Build a dummy DNS query containing a client cookie. let query = MessageBuilder::new_vec(); let mut query = query.question(); From ca0f08ac82ccd323352e517cd7c5b72dd865814f Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Wed, 19 Jun 2024 00:36:36 +0200 Subject: [PATCH 15/20] Revert the addition of netblock support - add that in a separate PR. --- Cargo.toml | 6 +- .../server/middleware/processors/cookies.rs | 69 ++++--------------- src/net/server/tests/integration.rs | 12 ++-- 3 files changed, 23 insertions(+), 64 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0946bd5eb..b69970713 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,9 +17,9 @@ name = "domain" path = "src/lib.rs" [dependencies] -octseq = { version = "0.5.2-dev", git = "https://github.com/NLnetLabs/octseq.git", rev ="3f7797f4274af0a52e66105250ee1186ff2ab6ac", default-features = false } -time = { version = "0.3.1", default-features = false } -ipnetwork = { version = "0.20.0", optional = true } +octseq = { version = "0.5.2-dev", git = "https://github.com/NLnetLabs/octseq.git", rev ="3f7797f4274af0a52e66105250ee1186ff2ab6ac", default-features = false } +time = { version = "0.3.1", default-features = false } + rand = { version = "0.8", optional = true } arc-swap = { version = "1.7.0", optional = true } bytes = { version = "1.0", optional = true, default-features = false } diff --git a/src/net/server/middleware/processors/cookies.rs b/src/net/server/middleware/processors/cookies.rs index 8c1328751..2b2c7fa31 100644 --- a/src/net/server/middleware/processors/cookies.rs +++ b/src/net/server/middleware/processors/cookies.rs @@ -1,12 +1,9 @@ //! DNS Cookies related message processing. use core::ops::ControlFlow; -use core::str::FromStr; use std::net::IpAddr; -use std::string::{String, ToString}; use std::vec::Vec; -use ipnetwork::IpNetwork; use octseq::Octets; use rand::RngCore; use tracing::{debug, trace, warn}; @@ -31,36 +28,6 @@ const FIVE_MINUTES_AS_SECS: u32 = 5 * 60; /// https://www.rfc-editor.org/rfc/rfc9018.html#section-4.3. const ONE_HOUR_AS_SECS: u32 = 60 * 60; -//----------- NetBlock -------------------------------------------------------- - -/// An IPv4 or IPv6 network range. -/// -// Note: Using a wrapper type avoids exposing the 3rd party IpNetwork type in -// our public API so that we can swap it out later for an alternative if -// needed without impacting the public API. -#[derive(Clone, Debug)] -pub struct NetBlock(IpNetwork); - -impl NetBlock { - /// Is the given IP address part of this network range? - fn contains(&self, ip: IpAddr) -> bool { - self.0.contains(ip) - } -} - -//--- FromStr - -impl FromStr for NetBlock { - type Err = String; - - fn from_str(s: &str) -> Result<Self, Self::Err> { - Ok(NetBlock( - IpNetwork::from_str(s) - .map_err(|err| ToString::to_string(&err))?, - )) - } -} - //----------- CookiesMiddlewareProcessor -------------------------------------- /// A DNS Cookies [`MiddlewareProcessor`]. @@ -83,7 +50,7 @@ pub struct CookiesMiddlewareProcessor { /// Clients connecting from these IP addresses will be required to provide /// a cookie otherwise they will receive REFUSED with TC=1 prompting them /// to reconnect with TCP in order to "authenticate" themselves. - deny_list: Vec<NetBlock>, + ip_deny_list: Vec<IpAddr>, } impl CookiesMiddlewareProcessor { @@ -92,17 +59,17 @@ impl CookiesMiddlewareProcessor { pub fn new(server_secret: [u8; 16]) -> Self { Self { server_secret, - deny_list: vec![], + ip_deny_list: vec![], } } /// Define IP addresses required to supply DNS cookies if using UDP. #[must_use] - pub fn with_denied_addresses<T: Into<Vec<NetBlock>>>( + pub fn with_denied_ips<T: Into<Vec<IpAddr>>>( mut self, - deny_list: T, + ip_deny_list: T, ) -> Self { - self.deny_list = deny_list.into(); + self.ip_deny_list = ip_deny_list.into(); self } } @@ -249,14 +216,6 @@ impl CookiesMiddlewareProcessor { // Cookie, the response SHALL have the RCODE NOERROR." self.response_with_cookie(request, Rcode::NOERROR.into()) } - - /// Is the given IP address required to authenticate itself? - /// - /// If the given IP address is on our deny list it is required to - /// authenticate itself. - fn must_authenticate(&self, ip: IpAddr) -> bool { - self.deny_list.iter().any(|netblock| netblock.contains(ip)) - } } //--- Default @@ -271,7 +230,7 @@ impl Default for CookiesMiddlewareProcessor { Self { server_secret, - deny_list: Default::default(), + ip_deny_list: Default::default(), } } } @@ -310,11 +269,11 @@ where // While not required by RFC 7873, like Unbound the caller can // configure this middleware processor to require clients - // contacting it from certain IP addresses or ranges to - // authenticate themselves or be refused with TC=1 to signal - // that they should resubmit their request via TCP. + // contacting it from certain IP addresses to authenticate + // themselves or be refused with TC=1 to signal that they + // should resubmit their request via TCP. if request.transport_ctx().is_udp() - && self.must_authenticate(request.client_addr().ip()) + && self.ip_deny_list.contains(&request.client_addr().ip()) { debug!("Rejecting cookie-less non-TCP request due to matching deny list entry"); let builder = mk_builder_for_target(); @@ -444,7 +403,9 @@ where }; return ControlFlow::Break(additional); } else if request.transport_ctx().is_udp() - && self.must_authenticate(request.client_addr().ip()) + && self + .ip_deny_list + .contains(&request.client_addr().ip()) { let additional = self.bad_cookie_response(request); debug!("Rejecting non-TCP request with invalid server cookie due to matching deny list entry"); @@ -541,7 +502,7 @@ mod tests { // Package the query into a context aware request to make it look // as if it came from a UDP server. let ctx = UdpTransportContext::default(); - let client_addr = "127.0.0.18:12345".parse().unwrap(); + let client_addr = "127.0.0.1:12345".parse().unwrap(); let request = Request::new(client_addr, Instant::now(), message, ctx.into()); @@ -549,7 +510,7 @@ mod tests { // the mock client to provide a valid cookie. let server_secret: [u8; 16] = [1u8; 16]; let processor = CookiesMiddlewareProcessor::new(server_secret) - .with_denied_addresses(["127.0.0.1/24".parse().unwrap()]); + .with_denied_ips(["127.0.0.1".parse().unwrap()]); let processor: &dyn MiddlewareProcessor<Vec<u8>, Vec<u8>> = &processor; diff --git a/src/net/server/tests/integration.rs b/src/net/server/tests/integration.rs index b5c277946..a42d74e4b 100644 --- a/src/net/server/tests/integration.rs +++ b/src/net/server/tests/integration.rs @@ -1,4 +1,4 @@ -use core::net::SocketAddr; +use core::net::{IpAddr, SocketAddr}; use std::boxed::Box; use std::collections::VecDeque; @@ -22,9 +22,7 @@ use crate::net::server::buf::VecBufSource; use crate::net::server::dgram::DgramServer; use crate::net::server::message::Request; use crate::net::server::middleware::builder::MiddlewareBuilder; -use crate::net::server::middleware::processors::cookies::{ - CookiesMiddlewareProcessor, NetBlock, -}; +use crate::net::server::middleware::processors::cookies::CookiesMiddlewareProcessor; use crate::net::server::middleware::processors::edns::EdnsMiddlewareProcessor; use crate::net::server::service::{ CallResult, Service, ServiceError, Transaction, @@ -217,7 +215,7 @@ where let secret = <[u8; 16]>::try_from(secret).unwrap(); let processor = CookiesMiddlewareProcessor::new(secret); let processor = processor - .with_denied_addresses(config.cookies.deny_list.clone()); + .with_denied_ips(config.cookies.ip_deny_list.clone()); middleware.push(processor.into()); } } @@ -341,7 +339,7 @@ struct ServerConfig<'a> { struct CookieConfig<'a> { enabled: bool, secret: Option<&'a str>, - deny_list: Vec<NetBlock>, + ip_deny_list: Vec<IpAddr>, } fn parse_server_config(config: &Config) -> ServerConfig { @@ -386,7 +384,7 @@ fn parse_server_config(config: &Config) -> ServerConfig { if let Ok(ip) = ip.parse() { parsed_config .cookies - .deny_list + .ip_deny_list .push(ip); } else { eprintln!("Ignoring malformed IP address '{ip}' in 'access-control' setting"); From a72ded1341327d29ae4fe76028ac3f8b5297d161 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Wed, 19 Jun 2024 00:38:31 +0200 Subject: [PATCH 16/20] Revert "Revert the addition of netblock support - add that in a separate PR." This reverts commit ca0f08ac82ccd323352e517cd7c5b72dd865814f. --- Cargo.toml | 6 +- .../server/middleware/processors/cookies.rs | 69 +++++++++++++++---- src/net/server/tests/integration.rs | 12 ++-- 3 files changed, 64 insertions(+), 23 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b69970713..0946bd5eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,9 +17,9 @@ name = "domain" path = "src/lib.rs" [dependencies] -octseq = { version = "0.5.2-dev", git = "https://github.com/NLnetLabs/octseq.git", rev ="3f7797f4274af0a52e66105250ee1186ff2ab6ac", default-features = false } -time = { version = "0.3.1", default-features = false } - +octseq = { version = "0.5.2-dev", git = "https://github.com/NLnetLabs/octseq.git", rev ="3f7797f4274af0a52e66105250ee1186ff2ab6ac", default-features = false } +time = { version = "0.3.1", default-features = false } +ipnetwork = { version = "0.20.0", optional = true } rand = { version = "0.8", optional = true } arc-swap = { version = "1.7.0", optional = true } bytes = { version = "1.0", optional = true, default-features = false } diff --git a/src/net/server/middleware/processors/cookies.rs b/src/net/server/middleware/processors/cookies.rs index 2b2c7fa31..8c1328751 100644 --- a/src/net/server/middleware/processors/cookies.rs +++ b/src/net/server/middleware/processors/cookies.rs @@ -1,9 +1,12 @@ //! DNS Cookies related message processing. use core::ops::ControlFlow; +use core::str::FromStr; use std::net::IpAddr; +use std::string::{String, ToString}; use std::vec::Vec; +use ipnetwork::IpNetwork; use octseq::Octets; use rand::RngCore; use tracing::{debug, trace, warn}; @@ -28,6 +31,36 @@ const FIVE_MINUTES_AS_SECS: u32 = 5 * 60; /// https://www.rfc-editor.org/rfc/rfc9018.html#section-4.3. const ONE_HOUR_AS_SECS: u32 = 60 * 60; +//----------- NetBlock -------------------------------------------------------- + +/// An IPv4 or IPv6 network range. +/// +// Note: Using a wrapper type avoids exposing the 3rd party IpNetwork type in +// our public API so that we can swap it out later for an alternative if +// needed without impacting the public API. +#[derive(Clone, Debug)] +pub struct NetBlock(IpNetwork); + +impl NetBlock { + /// Is the given IP address part of this network range? + fn contains(&self, ip: IpAddr) -> bool { + self.0.contains(ip) + } +} + +//--- FromStr + +impl FromStr for NetBlock { + type Err = String; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + Ok(NetBlock( + IpNetwork::from_str(s) + .map_err(|err| ToString::to_string(&err))?, + )) + } +} + //----------- CookiesMiddlewareProcessor -------------------------------------- /// A DNS Cookies [`MiddlewareProcessor`]. @@ -50,7 +83,7 @@ pub struct CookiesMiddlewareProcessor { /// Clients connecting from these IP addresses will be required to provide /// a cookie otherwise they will receive REFUSED with TC=1 prompting them /// to reconnect with TCP in order to "authenticate" themselves. - ip_deny_list: Vec<IpAddr>, + deny_list: Vec<NetBlock>, } impl CookiesMiddlewareProcessor { @@ -59,17 +92,17 @@ impl CookiesMiddlewareProcessor { pub fn new(server_secret: [u8; 16]) -> Self { Self { server_secret, - ip_deny_list: vec![], + deny_list: vec![], } } /// Define IP addresses required to supply DNS cookies if using UDP. #[must_use] - pub fn with_denied_ips<T: Into<Vec<IpAddr>>>( + pub fn with_denied_addresses<T: Into<Vec<NetBlock>>>( mut self, - ip_deny_list: T, + deny_list: T, ) -> Self { - self.ip_deny_list = ip_deny_list.into(); + self.deny_list = deny_list.into(); self } } @@ -216,6 +249,14 @@ impl CookiesMiddlewareProcessor { // Cookie, the response SHALL have the RCODE NOERROR." self.response_with_cookie(request, Rcode::NOERROR.into()) } + + /// Is the given IP address required to authenticate itself? + /// + /// If the given IP address is on our deny list it is required to + /// authenticate itself. + fn must_authenticate(&self, ip: IpAddr) -> bool { + self.deny_list.iter().any(|netblock| netblock.contains(ip)) + } } //--- Default @@ -230,7 +271,7 @@ impl Default for CookiesMiddlewareProcessor { Self { server_secret, - ip_deny_list: Default::default(), + deny_list: Default::default(), } } } @@ -269,11 +310,11 @@ where // While not required by RFC 7873, like Unbound the caller can // configure this middleware processor to require clients - // contacting it from certain IP addresses to authenticate - // themselves or be refused with TC=1 to signal that they - // should resubmit their request via TCP. + // contacting it from certain IP addresses or ranges to + // authenticate themselves or be refused with TC=1 to signal + // that they should resubmit their request via TCP. if request.transport_ctx().is_udp() - && self.ip_deny_list.contains(&request.client_addr().ip()) + && self.must_authenticate(request.client_addr().ip()) { debug!("Rejecting cookie-less non-TCP request due to matching deny list entry"); let builder = mk_builder_for_target(); @@ -403,9 +444,7 @@ where }; return ControlFlow::Break(additional); } else if request.transport_ctx().is_udp() - && self - .ip_deny_list - .contains(&request.client_addr().ip()) + && self.must_authenticate(request.client_addr().ip()) { let additional = self.bad_cookie_response(request); debug!("Rejecting non-TCP request with invalid server cookie due to matching deny list entry"); @@ -502,7 +541,7 @@ mod tests { // Package the query into a context aware request to make it look // as if it came from a UDP server. let ctx = UdpTransportContext::default(); - let client_addr = "127.0.0.1:12345".parse().unwrap(); + let client_addr = "127.0.0.18:12345".parse().unwrap(); let request = Request::new(client_addr, Instant::now(), message, ctx.into()); @@ -510,7 +549,7 @@ mod tests { // the mock client to provide a valid cookie. let server_secret: [u8; 16] = [1u8; 16]; let processor = CookiesMiddlewareProcessor::new(server_secret) - .with_denied_ips(["127.0.0.1".parse().unwrap()]); + .with_denied_addresses(["127.0.0.1/24".parse().unwrap()]); let processor: &dyn MiddlewareProcessor<Vec<u8>, Vec<u8>> = &processor; diff --git a/src/net/server/tests/integration.rs b/src/net/server/tests/integration.rs index a42d74e4b..b5c277946 100644 --- a/src/net/server/tests/integration.rs +++ b/src/net/server/tests/integration.rs @@ -1,4 +1,4 @@ -use core::net::{IpAddr, SocketAddr}; +use core::net::SocketAddr; use std::boxed::Box; use std::collections::VecDeque; @@ -22,7 +22,9 @@ use crate::net::server::buf::VecBufSource; use crate::net::server::dgram::DgramServer; use crate::net::server::message::Request; use crate::net::server::middleware::builder::MiddlewareBuilder; -use crate::net::server::middleware::processors::cookies::CookiesMiddlewareProcessor; +use crate::net::server::middleware::processors::cookies::{ + CookiesMiddlewareProcessor, NetBlock, +}; use crate::net::server::middleware::processors::edns::EdnsMiddlewareProcessor; use crate::net::server::service::{ CallResult, Service, ServiceError, Transaction, @@ -215,7 +217,7 @@ where let secret = <[u8; 16]>::try_from(secret).unwrap(); let processor = CookiesMiddlewareProcessor::new(secret); let processor = processor - .with_denied_ips(config.cookies.ip_deny_list.clone()); + .with_denied_addresses(config.cookies.deny_list.clone()); middleware.push(processor.into()); } } @@ -339,7 +341,7 @@ struct ServerConfig<'a> { struct CookieConfig<'a> { enabled: bool, secret: Option<&'a str>, - ip_deny_list: Vec<IpAddr>, + deny_list: Vec<NetBlock>, } fn parse_server_config(config: &Config) -> ServerConfig { @@ -384,7 +386,7 @@ fn parse_server_config(config: &Config) -> ServerConfig { if let Ok(ip) = ip.parse() { parsed_config .cookies - .ip_deny_list + .deny_list .push(ip); } else { eprintln!("Ignoring malformed IP address '{ip}' in 'access-control' setting"); From a64744f1c1b0a336c87e1de19293a2407872aa92 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 20 Jun 2024 00:12:13 +0200 Subject: [PATCH 17/20] Review feedback: Use inetnum instead of ipnetwork. --- Cargo.toml | 2 +- src/net/server/middleware/processors/cookies.rs | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0946bd5eb..a60611408 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ path = "src/lib.rs" [dependencies] octseq = { version = "0.5.2-dev", git = "https://github.com/NLnetLabs/octseq.git", rev ="3f7797f4274af0a52e66105250ee1186ff2ab6ac", default-features = false } time = { version = "0.3.1", default-features = false } -ipnetwork = { version = "0.20.0", optional = true } +inetnum = { version = "0.1.0", optional = true } rand = { version = "0.8", optional = true } arc-swap = { version = "1.7.0", optional = true } bytes = { version = "1.0", optional = true, default-features = false } diff --git a/src/net/server/middleware/processors/cookies.rs b/src/net/server/middleware/processors/cookies.rs index 8c1328751..fee85566d 100644 --- a/src/net/server/middleware/processors/cookies.rs +++ b/src/net/server/middleware/processors/cookies.rs @@ -6,7 +6,7 @@ use std::net::IpAddr; use std::string::{String, ToString}; use std::vec::Vec; -use ipnetwork::IpNetwork; +use inetnum::addr::Prefix; use octseq::Octets; use rand::RngCore; use tracing::{debug, trace, warn}; @@ -39,7 +39,7 @@ const ONE_HOUR_AS_SECS: u32 = 60 * 60; // our public API so that we can swap it out later for an alternative if // needed without impacting the public API. #[derive(Clone, Debug)] -pub struct NetBlock(IpNetwork); +pub struct NetBlock(Prefix); impl NetBlock { /// Is the given IP address part of this network range? @@ -55,8 +55,7 @@ impl FromStr for NetBlock { fn from_str(s: &str) -> Result<Self, Self::Err> { Ok(NetBlock( - IpNetwork::from_str(s) - .map_err(|err| ToString::to_string(&err))?, + Prefix::from_str(s).map_err(|err| ToString::to_string(&err))?, )) } } @@ -549,7 +548,7 @@ mod tests { // the mock client to provide a valid cookie. let server_secret: [u8; 16] = [1u8; 16]; let processor = CookiesMiddlewareProcessor::new(server_secret) - .with_denied_addresses(["127.0.0.1/24".parse().unwrap()]); + .with_denied_addresses(["127.0.0.0/24".parse().unwrap()]); let processor: &dyn MiddlewareProcessor<Vec<u8>, Vec<u8>> = &processor; From 15b18d6ad1b83a24ef4ffa838f6fda62de6192e4 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 20 Jun 2024 10:02:38 +0200 Subject: [PATCH 18/20] Don't use inetnum FromStr for Prefix as it doesn't support IP addresses without a prefix length. --- .../server/middleware/processors/cookies.rs | 49 +++++++++++++++++-- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/src/net/server/middleware/processors/cookies.rs b/src/net/server/middleware/processors/cookies.rs index fee85566d..664358a5b 100644 --- a/src/net/server/middleware/processors/cookies.rs +++ b/src/net/server/middleware/processors/cookies.rs @@ -6,7 +6,7 @@ use std::net::IpAddr; use std::string::{String, ToString}; use std::vec::Vec; -use inetnum::addr::Prefix; +use inetnum::addr::{ParsePrefixError, Prefix}; use octseq::Octets; use rand::RngCore; use tracing::{debug, trace, warn}; @@ -54,9 +54,28 @@ impl FromStr for NetBlock { type Err = String; fn from_str(s: &str) -> Result<Self, Self::Err> { - Ok(NetBlock( - Prefix::from_str(s).map_err(|err| ToString::to_string(&err))?, - )) + let prefix = match Prefix::from_str(s) { + Ok(prefix) => Ok(prefix), + Err(ParsePrefixError::MissingLen) => prefix_from_addr_str(s), + other_err => other_err, + } + .map_err(|err| ToString::to_string(&err))?; + + Ok(Self(prefix)) + } +} + +/// Construct a Prefix from an IP address string. +fn prefix_from_addr_str(s: &str) -> Result<Prefix, ParsePrefixError> { + match IpAddr::from_str(s) { + // TODO: Use IpvNAddr::BITS rather than 32/128 if our MSRV rises to + // Rust >= 1.80.0. + Ok(addr) => match addr { + IpAddr::V4(addr) => Prefix::new_v4(addr, 32), + IpAddr::V6(addr) => Prefix::new_v6(addr, 128), + } + .map_err(ParsePrefixError::InvalidPrefix), + Err(err) => Err(ParsePrefixError::InvalidAddr(err)), } } @@ -523,7 +542,27 @@ mod tests { use crate::net::server::message::{Request, UdpTransportContext}; use crate::net::server::middleware::processor::MiddlewareProcessor; - use super::CookiesMiddlewareProcessor; + use super::{CookiesMiddlewareProcessor, NetBlock}; + use core::str::FromStr; + + #[test] + fn netblock_from_str() { + assert!(NetBlock::from_str("").is_err()); + assert!(NetBlock::from_str("not-an-ip-address").is_err()); + assert!(NetBlock::from_str("1-2-3-4").is_err()); + assert!(NetBlock::from_str("1.2.3.4/").is_err()); + assert!(NetBlock::from_str("::1/").is_err()); + assert!(NetBlock::from_str("1-2-3-4/8").is_err()); + assert!(NetBlock::from_str("::/").is_err()); + assert!(NetBlock::from_str("1.2.3.4/not-a-prefix-length").is_err()); + assert!(NetBlock::from_str("1.2.3.4-4.5.6.7").is_err()); + + assert!(NetBlock::from_str("1.2.3.4").is_ok()); + assert!(NetBlock::from_str("::1").is_ok()); + assert!(NetBlock::from_str("1.2.3.4/32").is_ok()); + assert!(NetBlock::from_str("127.0.0.0/24").is_ok()); + assert!(NetBlock::from_str("::1/128").is_ok()); + } #[test] fn dont_add_cookie_twice() { From 78d7de5b74ebfd1945607b3b05438f472898a2b6 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 20 Jun 2024 10:10:08 +0200 Subject: [PATCH 19/20] Fix misleading Rust docs and comments. --- src/net/server/middleware/processors/cookies.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/net/server/middleware/processors/cookies.rs b/src/net/server/middleware/processors/cookies.rs index 664358a5b..8b88f3641 100644 --- a/src/net/server/middleware/processors/cookies.rs +++ b/src/net/server/middleware/processors/cookies.rs @@ -33,7 +33,7 @@ const ONE_HOUR_AS_SECS: u32 = 60 * 60; //----------- NetBlock -------------------------------------------------------- -/// An IPv4 or IPv6 network range. +/// An IPv4 or IPv6 network block. /// // Note: Using a wrapper type avoids exposing the 3rd party IpNetwork type in // our public API so that we can swap it out later for an alternative if @@ -42,7 +42,7 @@ const ONE_HOUR_AS_SECS: u32 = 60 * 60; pub struct NetBlock(Prefix); impl NetBlock { - /// Is the given IP address part of this network range? + /// Is the given IP address part of this network block? fn contains(&self, ip: IpAddr) -> bool { self.0.contains(ip) } @@ -328,7 +328,7 @@ where // While not required by RFC 7873, like Unbound the caller can // configure this middleware processor to require clients - // contacting it from certain IP addresses or ranges to + // contacting it from certain IP addresses or blocks to // authenticate themselves or be refused with TC=1 to signal // that they should resubmit their request via TCP. if request.transport_ctx().is_udp() From 929fb56e5694021b55db97ddb6ad83c9657db9ea Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 20 Jun 2024 13:18:42 +0200 Subject: [PATCH 20/20] Replace test references to ip with netblock. --- src/net/server/tests/integration.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/net/server/tests/integration.rs b/src/net/server/tests/integration.rs index b5c277946..fc5e030a6 100644 --- a/src/net/server/tests/integration.rs +++ b/src/net/server/tests/integration.rs @@ -378,18 +378,18 @@ fn parse_server_config(config: &Config) -> ServerConfig { // for a classless network block", but we only handle // an IP address here for now. // See: https://unbound.docs.nlnetlabs.nl/en/latest/manpages/unbound.conf.html?highlight=edns-tcp-keepalive#unbound-conf-access-control - if let Some((ip, action)) = + if let Some((netblock, action)) = v.split_once(|c: char| c.is_whitespace()) { match action { "allow_cookie" => { - if let Ok(ip) = ip.parse() { + if let Ok(netblock) = netblock.parse() { parsed_config .cookies .deny_list - .push(ip); + .push(netblock); } else { - eprintln!("Ignoring malformed IP address '{ip}' in 'access-control' setting"); + eprintln!("Ignoring malformed netblock '{netblock}' in 'access-control' setting"); } }