Re-enable and fix the Stelline cookies test (#336)

- Re-enable the cookies Stelline test and fix `CookiesMiddlewareProcessor` to allow requests with invalid cookies to proceed if they are authenticated or not required to authenticate.
- Add support for net blocks in the deny list of the cookie middleware processor, ala Unbound, otherwise the deny list is difficult to use beyond a few simple specific IP addresses.
- 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.

Cargo.toml

@@ -60,8 +60,8 @@ 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-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"]
 unstable-validator = ["validate", "zonefile", "unstable-client-transport"]
 unstable-zonetree = ["futures", "parking_lot", "serde", "tokio", "tracing"]
@@ -84,7 +84,7 @@ webpki-roots       = { version = "0.26" }
 #sqlx = { version = "0.6", features = [ "runtime-tokio-native-tls", "mysql" ] }
 
 # For testing in integration tests:
-mock_instant = { version = "0.4.0" }
+mock_instant = { version = "0.5.1" }
 
 [package.metadata.docs.rs]
 all-features = true

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();
 

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};

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;
 

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();

src/net/server/middleware/processors/cookies.rs

@@ -1,7 +1,6 @@
 //! DNS Cookies related message processing.
 use core::ops::ControlFlow;
 
-use std::net::IpAddr;
 use std::vec::Vec;
 
 use octseq::Octets;
@@ -10,6 +9,7 @@ use tracing::{debug, trace, warn};
 
 use crate::base::iana::{OptRcode, Rcode};
 use crate::base::message_builder::AdditionalBuilder;
+use crate::base::net::IpAddr;
 use crate::base::opt;
 use crate::base::wire::{Composer, ParseError};
 use crate::base::{Serial, StreamTarget};
@@ -18,6 +18,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 +28,8 @@ 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;
 
+//----------- CookiesMiddlewareProcessor --------------------------------------
+
 /// A DNS Cookies [`MiddlewareProcessor`].
 ///
 /// Standards covered by ths implementation:
@@ -71,19 +75,20 @@ impl CookiesMiddlewareProcessor {
 }
 
 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 +122,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.
@@ -230,6 +243,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 +259,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 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())
                 {
-                    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 +326,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 +402,13 @@ 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
+                            .ip_deny_list
+                            .contains(&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 {
@@ -480,13 +506,15 @@ mod tests {
         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_ips(["127.0.0.1".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!()

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;