From 3b3f330380aa383ef81f98d8e1af534fd3a4e043 Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Wed, 24 Apr 2024 19:27:49 +0200 Subject: [PATCH 1/5] recursor: set DO in outgoing queries when the recursor is "security-aware" -- that is the "dnssec" feature is enabled -- as per RFC 4035 section 3.2.1 --- crates/proto/src/xfer/dns_handle.rs | 3 ++- crates/proto/src/xfer/dns_request.rs | 3 +++ crates/recursor/src/lib.rs | 4 ++++ crates/recursor/src/recursor_pool.rs | 4 ++-- crates/server/Cargo.toml | 2 +- 5 files changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/proto/src/xfer/dns_handle.rs b/crates/proto/src/xfer/dns_handle.rs index d41385e349..874ecc857a 100644 --- a/crates/proto/src/xfer/dns_handle.rs +++ b/crates/proto/src/xfer/dns_handle.rs @@ -85,7 +85,8 @@ fn build_message(query: Query, options: DnsRequestOptions) -> Message { .extensions_mut() .get_or_insert_with(Edns::new) .set_max_payload(MAX_PAYLOAD_LEN) - .set_version(0); + .set_version(0) + .set_dnssec_ok(options.edns_set_dnssec_ok); } message } diff --git a/crates/proto/src/xfer/dns_request.rs b/crates/proto/src/xfer/dns_request.rs index 2d93c9b981..a20a5b60dd 100644 --- a/crates/proto/src/xfer/dns_request.rs +++ b/crates/proto/src/xfer/dns_request.rs @@ -25,6 +25,8 @@ pub struct DnsRequestOptions { // TODO: add EDNS options here? /// When true, will add EDNS options to the request. pub use_edns: bool, + /// When true, sets the DO bit in the EDNS options + pub edns_set_dnssec_ok: bool, /// Specifies maximum request depth for DNSSEC validation. pub max_request_depth: usize, /// set recursion desired (or not) for any requests @@ -38,6 +40,7 @@ impl Default for DnsRequestOptions { max_request_depth: 26, expects_multiple_responses: false, use_edns: false, + edns_set_dnssec_ok: false, recursion_desired: true, } } diff --git a/crates/recursor/src/lib.rs b/crates/recursor/src/lib.rs index 9488b472bd..c9a4b94f49 100644 --- a/crates/recursor/src/lib.rs +++ b/crates/recursor/src/lib.rs @@ -35,3 +35,7 @@ pub use hickory_proto as proto; pub use hickory_resolver as resolver; pub use hickory_resolver::config::NameServerConfig; pub use recursor::Recursor; + +fn is_security_aware() -> bool { + cfg!(feature = "dnssec") +} diff --git a/crates/recursor/src/recursor_pool.rs b/crates/recursor/src/recursor_pool.rs index 2611324c81..06abd2806b 100644 --- a/crates/recursor/src/recursor_pool.rs +++ b/crates/recursor/src/recursor_pool.rs @@ -90,8 +90,8 @@ where info!("querying {} for {}", self.zone, query_cpy); let mut options = DnsRequestOptions::default(); - options.use_edns = false; // TODO: this should be configurable - options.recursion_desired = false; + options.use_edns = crate::is_security_aware(); + options.edns_set_dnssec_ok = crate::is_security_aware(); // convert the lookup into a shared future let lookup = ns diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index e4da6f82f6..79c701ff45 100644 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -48,7 +48,7 @@ dnssec-ring = [ "hickory-proto/dnssec-ring", "hickory-resolver/dnssec-ring", ] -dnssec = [] +dnssec = ["hickory-recursor?/dnssec"] # Recursive Resolution is Experimental! recursor = ["hickory-recursor"] resolver = ["hickory-resolver"] From 7f24fcb163720087cdf9ab645e00961f6d1a4a4b Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Mon, 29 Apr 2024 16:14:19 +0200 Subject: [PATCH 2/5] recursor: preserve DNSSEC records --- crates/recursor/src/recursor.rs | 69 ++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/crates/recursor/src/recursor.rs b/crates/recursor/src/recursor.rs index ff91e16650..c662590f1a 100644 --- a/crates/recursor/src/recursor.rs +++ b/crates/recursor/src/recursor.rs @@ -5,7 +5,7 @@ // https://opensource.org/licenses/MIT>, at your option. This file may not be // copied, modified, or distributed except according to those terms. -use std::{net::SocketAddr, time::Instant}; +use std::{net::SocketAddr, sync::Arc, time::Instant}; use async_recursion::async_recursion; use futures_util::{future::select_all, FutureExt}; @@ -239,7 +239,11 @@ 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) { + if crate::is_security_aware() { + // TODO RFC4035 section 4.5 recommends caching "each response as a single atomic entry + // containing the entire answer, including the named RRset and any associated DNSSEC + // RRs" + } else if let Some(lookup) = self.record_cache.get(&query, request_time) { return lookup.map_err(Into::into); } @@ -280,7 +284,9 @@ impl Recursor { let ns = ns.ok_or_else(|| Error::from(format!("no nameserver found for {zone}")))?; debug!("found zone {} for {}", ns.zone(), query); - let response = self.lookup(query, ns, request_time).await?; + let response = self + .lookup(query, ns, request_time, crate::is_security_aware()) + .await?; Ok(response) } @@ -289,10 +295,13 @@ impl Recursor { query: Query, ns: RecursorPool, now: Instant, + security_aware: bool, ) -> Result { - if let Some(lookup) = self.record_cache.get(&query, now) { - debug!("cached data {lookup:?}"); - return lookup.map_err(Into::into); + if !security_aware { + if let Some(lookup) = self.record_cache.get(&query, now) { + debug!("cached data {lookup:?}"); + return lookup.map_err(Into::into); + } } let response = ns.lookup(query.clone()); @@ -304,26 +313,34 @@ impl Recursor { Ok(r) => { let mut r = r.into_message(); info!("response: {}", r.header()); - let records = r - .take_answers() - .into_iter() - .chain(r.take_name_servers()) - .chain(r.take_additionals()) - .filter(|x| { - if !is_subzone(ns.zone().clone(), x.name().clone()) { - warn!( - "Dropping out of bailiwick record {x} for zone {}", - ns.zone().clone() - ); - false - } else { - true - } - }); - let lookup = self.record_cache.insert_records(query, records, now); - - lookup.ok_or_else(|| Error::from("no records found")) + if security_aware { + // TODO: validation must be performed if the CD (Checking Disabled) is not set + // TODO: if DO bit is not set then DNSSEC records must be stripped unless + // explicitly requested in the query + Ok(Lookup::new_with_max_ttl(query, Arc::from(r.take_answers()))) + } else { + let records = r + .take_answers() + .into_iter() + .chain(r.take_name_servers()) + .chain(r.take_additionals()) + .filter(|x| { + if !is_subzone(ns.zone().clone(), x.name().clone()) { + warn!( + "Dropping out of bailiwick record {x} for zone {}", + ns.zone().clone() + ); + false + } else { + true + } + }); + + let lookup = self.record_cache.insert_records(query, records, now); + + lookup.ok_or_else(|| Error::from("no records found")) + } } Err(e) => { warn!("lookup error: {e}"); @@ -356,7 +373,7 @@ impl Recursor { let lookup = Query::query(zone.clone(), RecordType::NS); let response = self - .lookup(lookup.clone(), nameserver_pool.clone(), request_time) + .lookup(lookup.clone(), nameserver_pool.clone(), request_time, false) .await?; // let zone_nameservers = response.name_servers(); From 48a2531781358bb33475cf4942edc0f4bba24ada Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Mon, 29 Apr 2024 16:43:51 +0200 Subject: [PATCH 3/5] recursor: honor DO bit in client's query --- crates/recursor/src/recursor.rs | 64 +++++++++++++++---- crates/server/src/store/recursor/authority.rs | 6 +- util/src/bin/recurse.rs | 2 +- 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/crates/recursor/src/recursor.rs b/crates/recursor/src/recursor.rs index c662590f1a..1c4777340d 100644 --- a/crates/recursor/src/recursor.rs +++ b/crates/recursor/src/recursor.rs @@ -238,7 +238,12 @@ impl Recursor { /// has contiguous zones at the root and MIL domains, but also has a non- /// contiguous zone at ISI.EDU. /// ``` - pub async fn resolve(&self, query: Query, request_time: Instant) -> Result { + pub async fn resolve( + &self, + query: Query, + request_time: Instant, + query_has_dnssec_ok: bool, + ) -> Result { if crate::is_security_aware() { // TODO RFC4035 section 4.5 recommends caching "each response as a single atomic entry // containing the entire answer, including the named RRset and any associated DNSSEC @@ -284,9 +289,14 @@ impl Recursor { let ns = ns.ok_or_else(|| Error::from(format!("no nameserver found for {zone}")))?; debug!("found zone {} for {}", ns.zone(), query); - let response = self - .lookup(query, ns, request_time, crate::is_security_aware()) - .await?; + let dnssec = if crate::is_security_aware() { + Dnssec::Aware { + query_has_dnssec_ok, + } + } else { + Dnssec::Unaware + }; + let response = self.lookup(query, ns, request_time, dnssec).await?; Ok(response) } @@ -295,9 +305,9 @@ impl Recursor { query: Query, ns: RecursorPool, now: Instant, - security_aware: bool, + dnssec: Dnssec, ) -> Result { - if !security_aware { + if !dnssec.is_security_aware() { if let Some(lookup) = self.record_cache.get(&query, now) { debug!("cached data {lookup:?}"); return lookup.map_err(Into::into); @@ -314,11 +324,22 @@ impl Recursor { let mut r = r.into_message(); info!("response: {}", r.header()); - if security_aware { + if let Dnssec::Aware { + query_has_dnssec_ok, + } = dnssec + { // TODO: validation must be performed if the CD (Checking Disabled) is not set - // TODO: if DO bit is not set then DNSSEC records must be stripped unless - // explicitly requested in the query - Ok(Lookup::new_with_max_ttl(query, Arc::from(r.take_answers()))) + let mut answers = r.take_answers(); + if !query_has_dnssec_ok { + answers.retain(|rrset| { + // RFC 4035 section 3.2.1 if DO bit not set, strip DNSSEC records + // unless explicitly requested + let record_type = rrset.record_type(); + record_type == query.query_type() || !record_type.is_dnssec() + }); + } + + Ok(Lookup::new_with_max_ttl(query, Arc::from(answers))) } else { let records = r .take_answers() @@ -373,7 +394,12 @@ impl Recursor { let lookup = Query::query(zone.clone(), RecordType::NS); let response = self - .lookup(lookup.clone(), nameserver_pool.clone(), request_time, false) + .lookup( + lookup.clone(), + nameserver_pool.clone(), + request_time, + Dnssec::Unaware, + ) .await?; // let zone_nameservers = response.name_servers(); @@ -446,12 +472,12 @@ impl Recursor { debug!("need glue for {}", zone); let a_resolves = need_ips_for_names.iter().take(1).map(|name| { let a_query = Query::query(name.0.clone(), RecordType::A); - self.resolve(a_query, request_time).boxed() + self.resolve(a_query, request_time, false).boxed() }); let aaaa_resolves = need_ips_for_names.iter().take(1).map(|name| { let aaaa_query = Query::query(name.0.clone(), RecordType::AAAA); - self.resolve(aaaa_query, request_time).boxed() + self.resolve(aaaa_query, request_time, false).boxed() }); let mut a_resolves: Vec<_> = a_resolves.chain(aaaa_resolves).collect(); @@ -496,6 +522,18 @@ impl Recursor { } } +enum Dnssec { + Unaware, + Aware { query_has_dnssec_ok: bool }, +} + +impl Dnssec { + #[must_use] + fn is_security_aware(&self) -> bool { + matches!(self, Self::Aware { .. }) + } +} + fn recursor_opts() -> ResolverOpts { let mut options = ResolverOpts::default(); options.ndots = 0; diff --git a/crates/server/src/store/recursor/authority.rs b/crates/server/src/store/recursor/authority.rs index e52f33520b..a6b948257e 100644 --- a/crates/server/src/store/recursor/authority.rs +++ b/crates/server/src/store/recursor/authority.rs @@ -115,15 +115,15 @@ impl Authority for RecursiveAuthority { &self, name: &LowerName, rtype: RecordType, - _lookup_options: LookupOptions, + lookup_options: LookupOptions, ) -> Result { - debug!("recursive lookup: {} {}", name, rtype); + debug!("recursive lookup: {} {} {:?}", name, rtype, lookup_options); let query = Query::query(name.into(), rtype); let now = Instant::now(); self.recursor - .resolve(query, now) + .resolve(query, now, lookup_options.is_dnssec()) .await .map(RecursiveLookup) .map_err(Into::into) diff --git a/util/src/bin/recurse.rs b/util/src/bin/recurse.rs index 5bf8946098..3690d88259 100644 --- a/util/src/bin/recurse.rs +++ b/util/src/bin/recurse.rs @@ -168,7 +168,7 @@ pub async fn main() -> Result<(), Box> { let now = Instant::now(); let query = Query::query(name, ty); - let lookup = recursor.resolve(query, now).await?; + let lookup = recursor.resolve(query, now, false).await?; // report response, TODO: better display of errors println!( From 93bb8b6a85d6087e0e505c2dc9d874d41cca58cb Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Wed, 8 May 2024 18:31:00 +0200 Subject: [PATCH 4/5] make Recursor configurable via a "builder" and make security-awareness opt-in --- crates/recursor/src/lib.rs | 2 +- crates/recursor/src/recursor.rs | 80 +++++++++++++++++-- crates/server/src/store/recursor/authority.rs | 5 +- util/src/bin/recurse.rs | 2 +- 4 files changed, 78 insertions(+), 11 deletions(-) diff --git a/crates/recursor/src/lib.rs b/crates/recursor/src/lib.rs index c9a4b94f49..d22b7b6449 100644 --- a/crates/recursor/src/lib.rs +++ b/crates/recursor/src/lib.rs @@ -34,7 +34,7 @@ pub use error::{Error, ErrorKind}; pub use hickory_proto as proto; pub use hickory_resolver as resolver; pub use hickory_resolver::config::NameServerConfig; -pub use recursor::Recursor; +pub use recursor::{Recursor, RecursorBuilder}; fn is_security_aware() -> bool { cfg!(feature = "dnssec") diff --git a/crates/recursor/src/recursor.rs b/crates/recursor/src/recursor.rs index 1c4777340d..56e2eb7f9f 100644 --- a/crates/recursor/src/recursor.rs +++ b/crates/recursor/src/recursor.rs @@ -37,6 +37,66 @@ use crate::{ /// Set of nameservers by the zone name type NameServerCache

= LruCache>; +/// A `Recursor` builder +#[derive(Clone, Copy)] +pub struct RecursorBuilder { + ns_cache_size: usize, + record_cache_size: usize, + #[cfg(feature = "dnssec")] + security_aware: bool, +} + +impl Default for RecursorBuilder { + fn default() -> Self { + Self { + ns_cache_size: 1024, + record_cache_size: 1048576, + #[cfg(feature = "dnssec")] + security_aware: false, + } + } +} + +impl RecursorBuilder { + /// Sets the size of the list of cached name servers + pub fn ns_cache_size(&mut self, size: usize) -> &mut Self { + self.ns_cache_size = size; + self + } + + /// Sets the size of the list of cached records + pub fn record_cache_size(&mut self, size: usize) -> &mut Self { + self.record_cache_size = size; + self + } + + /// Enables or disables (DNSSEC) security awareness + #[cfg(feature = "dnssec")] + pub fn security_aware(&mut self, security_aware: bool) -> &mut Self { + self.security_aware = security_aware; + self + } + + /// Construct a new recursor using the list of NameServerConfigs for the root node list + /// + /// # Panics + /// + /// This will panic if the roots are empty. + pub fn build(&self, roots: impl Into) -> Result { + #[cfg(not(feature = "dnssec"))] + let security_aware = false; + #[cfg(feature = "dnssec")] + let security_aware = self.security_aware; + + Recursor::build( + roots, + self.ns_cache_size, + self.record_cache_size, + security_aware, + ) + } +} + /// A top down recursive resolver which operates off a list of roots for initial recursive requests. /// /// This is the well known root nodes, referred to as hints in RFCs. See the IANA [Root Servers](https://www.iana.org/domains/root/servers) list. @@ -44,18 +104,21 @@ pub struct Recursor { roots: RecursorPool, name_server_cache: Mutex>, record_cache: DnsLru, + security_aware: bool, } impl Recursor { - /// Construct a new recursor using the list of NameServerConfigs for the root node list - /// - /// # Panics - /// - /// This will panic if the roots are empty. - pub fn new( + /// Short-hand for `RecursorBuilder::default` + #[allow(clippy::new_ret_no_self)] + pub fn new() -> RecursorBuilder { + RecursorBuilder::default() + } + + fn build( roots: impl Into, ns_cache_size: usize, record_cache_size: usize, + security_aware: bool, ) -> Result { // configure the hickory-resolver let roots: NameServerConfigGroup = roots.into(); @@ -74,6 +137,7 @@ impl Recursor { roots, name_server_cache, record_cache, + security_aware, }) } @@ -244,7 +308,7 @@ impl Recursor { request_time: Instant, query_has_dnssec_ok: bool, ) -> Result { - if crate::is_security_aware() { + if self.security_aware { // TODO RFC4035 section 4.5 recommends caching "each response as a single atomic entry // containing the entire answer, including the named RRset and any associated DNSSEC // RRs" @@ -289,7 +353,7 @@ impl Recursor { let ns = ns.ok_or_else(|| Error::from(format!("no nameserver found for {zone}")))?; debug!("found zone {} for {}", ns.zone(), query); - let dnssec = if crate::is_security_aware() { + let dnssec = if self.security_aware { Dnssec::Aware { query_has_dnssec_ok, } diff --git a/crates/server/src/store/recursor/authority.rs b/crates/server/src/store/recursor/authority.rs index a6b948257e..56667689f7 100644 --- a/crates/server/src/store/recursor/authority.rs +++ b/crates/server/src/store/recursor/authority.rs @@ -73,7 +73,10 @@ impl RecursiveAuthority { }); } - let recursor = Recursor::new(roots, config.ns_cache_size, config.record_cache_size) + let recursor = Recursor::new() + .ns_cache_size(config.ns_cache_size) + .record_cache_size(config.record_cache_size) + .build(roots) .map_err(|e| format!("failed to initialize recursor: {e}"))?; Ok(Self { diff --git a/util/src/bin/recurse.rs b/util/src/bin/recurse.rs index 3690d88259..1bb10eb2b4 100644 --- a/util/src/bin/recurse.rs +++ b/util/src/bin/recurse.rs @@ -157,7 +157,7 @@ pub async fn main() -> Result<(), Box> { let name = opts.domainname; let ty = opts.ty; - let recursor = Recursor::new(roots, 1024, 1048576)?; + let recursor = Recursor::new().build(roots)?; // execute query println!( From 0ede6a567e7a501c91c05a26d1ec698affb2e5e6 Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Wed, 8 May 2024 18:37:36 +0200 Subject: [PATCH 5/5] expose security-aware setting in named.toml --- crates/server/src/store/recursor/authority.rs | 8 ++++++-- crates/server/src/store/recursor/config.rs | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/server/src/store/recursor/authority.rs b/crates/server/src/store/recursor/authority.rs index 56667689f7..3211306f80 100644 --- a/crates/server/src/store/recursor/authority.rs +++ b/crates/server/src/store/recursor/authority.rs @@ -73,9 +73,13 @@ impl RecursiveAuthority { }); } - let recursor = Recursor::new() + let mut recursor = Recursor::new(); + recursor .ns_cache_size(config.ns_cache_size) - .record_cache_size(config.record_cache_size) + .record_cache_size(config.record_cache_size); + #[cfg(feature = "dnssec")] + recursor.security_aware(config.security_aware); + let recursor = recursor .build(roots) .map_err(|e| format!("failed to initialize recursor: {e}"))?; diff --git a/crates/server/src/store/recursor/config.rs b/crates/server/src/store/recursor/config.rs index 1697000388..41843ef04c 100644 --- a/crates/server/src/store/recursor/config.rs +++ b/crates/server/src/store/recursor/config.rs @@ -24,6 +24,7 @@ use crate::resolver::Name; /// Configuration for file based zones #[derive(Clone, Deserialize, Eq, PartialEq, Debug)] +#[serde(deny_unknown_fields)] pub struct RecursiveConfig { /// File with roots, aka hints pub roots: PathBuf, @@ -35,6 +36,11 @@ pub struct RecursiveConfig { /// Maximum DNS record cache size #[serde(default = "record_cache_size_default")] pub record_cache_size: usize, + + /// Whether the recursor is security-aware (RFC4035 section 3.2) + #[cfg(feature = "dnssec")] + #[serde(default)] + pub security_aware: bool, } impl RecursiveConfig {