From fd265a9ae46573433ff748940aaae7a29990e551 Mon Sep 17 00:00:00 2001 From: Colin Date: Thu, 9 May 2024 08:57:59 +0000 Subject: [PATCH] recursor: fix to resolve most CNAMEs --- crates/recursor/src/recursor.rs | 84 +++++++++++++++++-- crates/resolver/src/lookup.rs | 3 +- .../integration-tests/tests/recursor_tests.rs | 58 +++++++++++++ 3 files changed, 138 insertions(+), 7 deletions(-) diff --git a/crates/recursor/src/recursor.rs b/crates/recursor/src/recursor.rs index 76145ba7..2be53f3b 100644 --- a/crates/recursor/src/recursor.rs +++ b/crates/recursor/src/recursor.rs @@ -12,6 +12,7 @@ use futures_util::{future::select_all, FutureExt}; use hickory_resolver::name_server::TokioConnectionProvider; use lru_cache::LruCache; use parking_lot::Mutex; +use std::sync::Arc; use tracing::{debug, info, warn}; #[cfg(test)] @@ -252,8 +253,45 @@ impl Recursor

{ /// contiguous zone at ISI.EDU. /// ``` pub async fn resolve(&self, query: Query, request_time: Instant) -> Result { - if let Some(lookup) = self.record_cache.get(&query, request_time) { - return lookup.map_err(Into::into); + let mut all_found = Lookup::new_with_max_ttl(query.clone(), Arc::new([])); + let mut active_query = query.clone(); + // max number of CNAMEs to traverse + const MAX_FOLLOWS: usize = 20; + for iter in 0..MAX_FOLLOWS { + let lookup = self.resolve_once(active_query.clone(), request_time).await?; + + if lookup.query() == &query { + // Resolved as expected: done! + all_found = all_found.append(lookup); + debug!("resolve complete in iteration {}", iter); + return Ok(all_found); + } + // Extract whichever CNAME the server told us of + let mut cnames = lookup.records().iter().filter_map(|r| match r.data() { + Some(RData::CNAME(target)) if r.name() == active_query.name() => Some(target), + _ => None, + }); + if let Some(cname_data) = cnames.next() { + debug!("resolve to CNAME target: {}", *cname_data); + active_query.set_name((**cname_data).clone()); + all_found = all_found.append(lookup); + } else { + // The server gave us neither the expected record, nor any CNAMEs to follow. + // This is probably an error. + all_found = all_found.append(lookup); + debug!("resolve complete in iteration {}: no more CNAMEs to follow", iter); + return Ok(all_found); + } + } + debug!("giving up after following {} CNAMEs", MAX_FOLLOWS); + Err(ErrorKind::Timeout.into()) + } + + /// Recursively resolve a query until we find the record, or reach a CNAME that must be + /// manually dereferenced. + async fn resolve_once(&self, query: Query, request_time: Instant) -> Result { + if let Some(lookup) = self.get_cached_query_or_cname(&query, request_time) { + return lookup; } // not in cache, let's look for an ns record for lookup @@ -297,15 +335,17 @@ impl Recursor

{ Ok(response) } + /// Perform a single DNS query, checking/updating the cache. + /// In case that the nameserver answers to a non-CNAME query with a CNAME, this function will + /// return the CNAME. It's up to the caller to expand the answer further. async fn lookup( &self, query: Query, ns: RecursorPool

, now: Instant, ) -> Result { - if let Some(lookup) = self.record_cache.get(&query, now) { - debug!("cached data {lookup:?}"); - return lookup.map_err(Into::into); + if let Some(lookup) = self.get_cached_query_or_cname(&query, now) { + return lookup; } let response = ns.lookup(query.clone()); @@ -334,7 +374,17 @@ impl Recursor

{ } }); - let lookup = self.record_cache.insert_records(query, records, now); + let lookup = self.record_cache.insert_records(query.clone(), records, now); + let lookup = lookup.or_else(|| { + if query.query_type().is_cname() || query.query_type().is_any() { + None + } else { + debug!("no records for {}: checking for cached CNAME", query.name()); + let mut cname_query = query; + cname_query.set_query_type(RecordType::CNAME); + self.record_cache.get(&cname_query, now).and_then(Result::ok) + } + }); lookup.ok_or_else(|| Error::from("no records found")) } @@ -490,6 +540,28 @@ impl Recursor

{ self.name_server_cache.lock().insert(zone, ns.clone()); Ok(ns) } + + /// Return cached data for the precise query, if present, + /// falling back to aliased data if an answer compatible with the given query exists. + /// + /// For example, a query for `www.example.com. A` could yield `www.example.com. A 1.2.3.4` + /// OR it could yield `www.example.com. CNAME other.example.com.`. + fn get_cached_query_or_cname(&self, query: &Query, request_time: Instant) -> Option> { + if let Some(lookup) = self.record_cache.get(query, request_time) { + debug!("cached data {lookup:?}"); + return Some(lookup.map_err(Into::into)); + } + if !query.query_type().is_cname() && !query.query_type().is_any() { + // If trying to lookup e.g. an A record, check if it's actually a record we've looked + // up before, behind a CNAME. + let mut cname_query = query.clone(); + cname_query.set_query_type(RecordType::CNAME); + if let Some(lookup) = self.record_cache.get(&cname_query, request_time) { + return Some(lookup.map_err(Into::into)); + } + } + None + } } fn recursor_opts() -> ResolverOpts { diff --git a/crates/resolver/src/lookup.rs b/crates/resolver/src/lookup.rs index 120c220b..a7b241b5 100644 --- a/crates/resolver/src/lookup.rs +++ b/crates/resolver/src/lookup.rs @@ -129,7 +129,8 @@ impl Lookup { } /// Clones the inner vec, appends the other vec - pub(crate) fn append(&self, other: Self) -> Self { + #[doc(hidden)] + pub fn append(&self, other: Self) -> Self { let mut records = Vec::with_capacity(self.len() + other.len()); records.extend_from_slice(&self.records); records.extend_from_slice(&other.records); diff --git a/tests/integration-tests/tests/recursor_tests.rs b/tests/integration-tests/tests/recursor_tests.rs index 1523620c..9291d2b0 100644 --- a/tests/integration-tests/tests/recursor_tests.rs +++ b/tests/integration-tests/tests/recursor_tests.rs @@ -95,6 +95,7 @@ const ZONE_EXAMPLE_COM: &str = r#" example.com. A 10.0.100.1 www.example.com. A 10.0.100.1 cname.sub.example.com. CNAME www.example.com. +cname.example.com. CNAME www.example.com. "#; type HardcodedNameServer = NameServer; @@ -398,3 +399,60 @@ fn test_cname_below_nonexistent_parent() { assert_eq!(&*lookup.records().to_vec(), &[expected_record]); } + +/// `.` (NS) -> `com.` (NS) -> `example.com.` (NS) -> `cname.example.com.`. +/// This results in a lookup for `cname.example.com. NS`, even though the record is actually CNAME. +#[test] +fn test_cname_under_ns() { + logger("DEBUG"); + + let query = Query::query(Name::from_str("cname.example.com.").unwrap(), RecordType::CNAME); + let expected_record = cname_record( + Name::from_str("cname.example.com.").unwrap(), + Name::from_str("www.example.com.").unwrap(), + ); + + let roots = NameServerPool::from_nameservers( + Default::default(), + vec![mock_nameserver(NS_ROOT)], + vec![], + ); + let recursor = Recursor::new_with_pool(roots, 1024, 1048576).unwrap(); + + let now = Instant::now(); + let lookup = block_on(recursor.resolve(query, now)).unwrap(); + + assert_eq!(&*lookup.records().to_vec(), &[expected_record]); +} + +/// Query for an A record which is actually a CNAME record. +/// The server should answer with CNAME, and (since the target is in the same zone) an additional +/// A record. +#[test] +fn test_cname_queried_as_v4() { + logger("DEBUG"); + + let query = Query::query(Name::from_str("cname.example.com.").unwrap(), RecordType::A); + let expected_records = [ + cname_record( + Name::from_str("cname.example.com.").unwrap(), + Name::from_str("www.example.com.").unwrap(), + ), + v4_record( + Name::from_str("www.example.com.").unwrap(), + Ipv4Addr::new(10, 0, 100, 1), + ), + ]; + + let roots = NameServerPool::from_nameservers( + Default::default(), + vec![mock_nameserver(NS_ROOT)], + vec![], + ); + let recursor = Recursor::new_with_pool(roots, 1024, 1048576).unwrap(); + + let now = Instant::now(); + let lookup = block_on(recursor.resolve(query, now)).unwrap(); + + assert_eq!(&*lookup.records().to_vec(), &expected_records); +}