From c5a0d5bfe6941866145e2159957575e3bc556827 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Wed, 29 Jan 2025 21:18:16 +0100 Subject: [PATCH 1/5] ZoneUpdater currently only supports ParsedRecord which is quite hard to construct, as that is what is output by the XfrResponseInterpreter. This commit generalizes the support to any Record type, not just ParsedRecord. --- src/zonetree/update.rs | 85 ++++++++++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 29 deletions(-) diff --git a/src/zonetree/update.rs b/src/zonetree/update.rs index 6a20140b3..bc77deca1 100644 --- a/src/zonetree/update.rs +++ b/src/zonetree/update.rs @@ -4,17 +4,17 @@ //! content of zones without requiring knowledge of the low-level details of //! how the [`WritableZone`] trait implemented by [`Zone`] works. use core::future::Future; +use core::marker::PhantomData; use core::pin::Pin; -use std::borrow::ToOwned; use std::boxed::Box; use bytes::Bytes; use tracing::trace; use crate::base::name::{FlattenInto, Label}; -use crate::base::{ParsedName, Record, Rtype}; -use crate::net::xfr::protocol::ParsedRecord; +use crate::base::scan::ScannerError; +use crate::base::{Name, Record, Rtype, ToName}; use crate::rdata::ZoneRecordData; use crate::zonetree::{Rrset, SharedRrset}; @@ -214,7 +214,7 @@ use super::{InMemoryZoneDiff, WritableZone, WritableZoneNode, Zone}; /// ``` /// /// [`apply()`]: ZoneUpdater::apply() -pub struct ZoneUpdater { +pub struct ZoneUpdater { /// The zone to be updated. zone: Zone, @@ -226,9 +226,15 @@ pub struct ZoneUpdater { /// The current state of the updater. state: ZoneUpdaterState, + + _phantom: PhantomData, } -impl ZoneUpdater { +impl ZoneUpdater +where + N: ToName + Clone, + ZoneRecordData: FlattenInto>>, +{ /// Creates a new [`ZoneUpdater`] that will update the given [`Zone`] /// content. /// @@ -246,12 +252,17 @@ impl ZoneUpdater { zone, write, state: Default::default(), + _phantom: PhantomData, }) }) } } -impl ZoneUpdater { +impl ZoneUpdater +where + N: ToName + Clone, + ZoneRecordData: FlattenInto>>, +{ /// Apply the given [`ZoneUpdate`] to the [`Zone`] being updated. /// /// Returns `Ok` on success, `Err` otherwise. On success, if changes were @@ -266,7 +277,7 @@ impl ZoneUpdater { /// progress and re-open the zone for editing again. pub async fn apply( &mut self, - update: ZoneUpdate, + update: ZoneUpdate>>, ) -> Result, Error> { trace!("Update: {update}"); @@ -344,7 +355,11 @@ impl ZoneUpdater { } } -impl ZoneUpdater { +impl ZoneUpdater +where + N: ToName + Clone, + ZoneRecordData: FlattenInto>>, +{ /// Given a zone record, obtain a [`WritableZoneNode`] for the owner. /// /// A [`Zone`] is a tree structure which can be modified by descending the @@ -364,7 +379,7 @@ impl ZoneUpdater { /// the record owner name. async fn get_writable_child_node_for_owner( &mut self, - rec: &ParsedRecord, + rec: &Record>, ) -> Result>, Error> { let mut it = rel_name_rev_iter(self.zone.apex_name(), rec.owner())?; @@ -386,17 +401,19 @@ impl ZoneUpdater { /// Create or update the SOA RRset using the given SOA record. async fn update_soa( &mut self, - new_soa: Record< - ParsedName, - ZoneRecordData>, - >, + new_soa: Record>, ) -> Result<(), Error> { if new_soa.rtype() != Rtype::SOA { return Err(Error::NotSoaRecord); } let mut rrset = Rrset::new(Rtype::SOA, new_soa.ttl()); - rrset.push_data(new_soa.data().to_owned().flatten_into()); + let Ok(flattened) = new_soa.data().clone().try_flatten_into() else { + return Err(Error::IoError(std::io::Error::custom( + "Unable to flatten bytes", + ))); + }; + rrset.push_data(flattened); self.write .update_root_rrset(SharedRrset::new(rrset)) .await?; @@ -407,10 +424,7 @@ impl ZoneUpdater { /// Find and delete a resource record in the zone by exact match. async fn delete_record_from_rrset( &mut self, - rec: Record< - ParsedName, - ZoneRecordData>, - >, + rec: Record>, ) -> Result<(), Error> { // Find or create the point to edit in the node tree. let tree_node = self.get_writable_child_node_for_owner(&rec).await?; @@ -443,11 +457,12 @@ impl ZoneUpdater { /// Add a resource record to a new or existing RRset. async fn add_record_to_rrset( &mut self, - rec: Record< - ParsedName, - ZoneRecordData>, - >, - ) -> Result<(), Error> { + rec: Record>, + ) -> Result<(), Error> + where + ZoneRecordData: + FlattenInto>>, + { // Find or create the point to edit in the node tree. let tree_node = self.get_writable_child_node_for_owner(&rec).await?; let tree_node = tree_node.as_ref().unwrap_or(self.write.root()); @@ -456,7 +471,11 @@ impl ZoneUpdater { // RRset in the tree plus the one to add. let mut rrset = Rrset::new(rec.rtype(), rec.ttl()); let rtype = rec.rtype(); - let data = rec.into_data().flatten_into(); + let Ok(data) = rec.into_data().try_flatten_into() else { + return Err(Error::IoError(std::io::Error::custom( + "Unable to flatten bytes", + ))); + }; rrset.push_data(data); @@ -904,7 +923,7 @@ mod tests { // IN NS NS.JAIN.AD.JP. let ns_1 = Record::new( - ParsedName::from(Name::from_str("JAIN.AD.JP.").unwrap()), + ParsedName::from(Name::::from_str("JAIN.AD.JP.").unwrap()), Class::IN, Ttl::from_secs(0), Ns::new(ParsedName::from( @@ -919,7 +938,9 @@ mod tests { // NS.JAIN.AD.JP. IN A 133.69.136.1 let a_1 = Record::new( - ParsedName::from(Name::from_str("NS.JAIN.AD.JP.").unwrap()), + ParsedName::from( + Name::::from_str("NS.JAIN.AD.JP.").unwrap(), + ), Class::IN, Ttl::from_secs(0), A::new(Ipv4Addr::new(133, 69, 136, 1)).into(), @@ -931,7 +952,9 @@ mod tests { // NEZU.JAIN.AD.JP. IN A 133.69.136.5 let nezu = Record::new( - ParsedName::from(Name::from_str("NEZU.JAIN.AD.JP.").unwrap()), + ParsedName::from( + Name::::from_str("NEZU.JAIN.AD.JP.").unwrap(), + ), Class::IN, Ttl::from_secs(0), A::new(Ipv4Addr::new(133, 69, 136, 5)).into(), @@ -956,7 +979,9 @@ mod tests { .await .unwrap(); let a_2 = Record::new( - ParsedName::from(Name::from_str("JAIN-BB.JAIN.AD.JP.").unwrap()), + ParsedName::from( + Name::::from_str("JAIN-BB.JAIN.AD.JP.").unwrap(), + ), Class::IN, Ttl::from_secs(0), A::new(Ipv4Addr::new(133, 69, 136, 4)).into(), @@ -991,7 +1016,9 @@ mod tests { .await .unwrap(); let a_4 = Record::new( - ParsedName::from(Name::from_str("JAIN-BB.JAIN.AD.JP.").unwrap()), + ParsedName::from( + Name::::from_str("JAIN-BB.JAIN.AD.JP.").unwrap(), + ), Class::IN, Ttl::from_secs(0), A::new(Ipv4Addr::new(133, 69, 136, 3)).into(), From f785c51d37b1af1242199875bc99457b50eabbef Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Thu, 30 Jan 2025 23:59:54 +0100 Subject: [PATCH 2/5] Fix failing doc tests. --- src/net/server/service.rs | 16 ---------------- src/zonetree/update.rs | 12 ++++++------ 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/src/net/server/service.rs b/src/net/server/service.rs index f7577b171..ba5720956 100644 --- a/src/net/server/service.rs +++ b/src/net/server/service.rs @@ -98,22 +98,6 @@ pub type ServiceResult = Result, ServiceError>; /// } /// } /// -/// //------------ An anonymous async block service example ------------------- -/// struct MyAsyncBlockService; -/// -/// impl Service, ()> for MyAsyncBlockService { -/// type Target = Vec; -/// type Stream = Once>>; -/// type Future = Pin>>; -/// -/// fn call( -/// &self, -/// msg: Request, ()>, -/// ) -> Self::Future { -/// Box::pin(async move { mk_response_stream(&msg) }) -/// } -/// } -/// /// //------------ A named Future service example ----------------------------- /// struct MyFut(Request, ()>); /// diff --git a/src/zonetree/update.rs b/src/zonetree/update.rs index bc77deca1..2e066b467 100644 --- a/src/zonetree/update.rs +++ b/src/zonetree/update.rs @@ -51,7 +51,7 @@ use super::{InMemoryZoneDiff, WritableZone, WritableZoneNode, Zone}; /// /// ``` /// # use std::str::FromStr; -/// # +/// # use bytes::Bytes; /// # use domain::base::iana::Class; /// # use domain::base::MessageBuilder; /// # use domain::base::Name; @@ -76,8 +76,8 @@ use super::{InMemoryZoneDiff, WritableZone, WritableZoneNode, Zone}; /// # /// # // Prepare some records to pass to ZoneUpdater /// # let serial = Serial::now(); -/// # let mname = ParsedName::from(Name::from_str("mname").unwrap()); -/// # let rname = ParsedName::from(Name::from_str("rname").unwrap()); +/// # let mname = ParsedName::from(Name::::from_str("mname").unwrap()); +/// # let rname = ParsedName::from(Name::::from_str("rname").unwrap()); /// # let ttl = Ttl::from_secs(0); /// # let new_soa_rec = Record::new( /// # ParsedName::from(Name::from_str("example.com").unwrap()), @@ -106,7 +106,7 @@ use super::{InMemoryZoneDiff, WritableZone, WritableZoneNode, Zone}; /// /// ```rust /// # use std::str::FromStr; -/// # +/// # use bytes::Bytes; /// # use domain::base::iana::Class; /// # use domain::base::MessageBuilder; /// # use domain::base::Name; @@ -133,8 +133,8 @@ use super::{InMemoryZoneDiff, WritableZone, WritableZoneNode, Zone}; /// # /// # // Prepare some records to pass to ZoneUpdater /// # let serial = Serial::now(); -/// # let mname = ParsedName::from(Name::from_str("mname").unwrap()); -/// # let rname = ParsedName::from(Name::from_str("rname").unwrap()); +/// # let mname = ParsedName::from(Name::::from_str("mname").unwrap()); +/// # let rname = ParsedName::from(Name::::from_str("rname").unwrap()); /// # let ttl = Ttl::from_secs(0); /// # let new_soa_rec = Record::new( /// # ParsedName::from(Name::from_str("example.com").unwrap()), From aab0d9cc3040bb22b36fe25032d7da85f82a61d4 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Fri, 31 Jan 2025 00:01:23 +0100 Subject: [PATCH 3/5] Cargo fmt. --- src/zonetree/update.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zonetree/update.rs b/src/zonetree/update.rs index 2e066b467..460f64095 100644 --- a/src/zonetree/update.rs +++ b/src/zonetree/update.rs @@ -51,7 +51,7 @@ use super::{InMemoryZoneDiff, WritableZone, WritableZoneNode, Zone}; /// /// ``` /// # use std::str::FromStr; -/// # use bytes::Bytes; +/// # use bytes::Bytes; /// # use domain::base::iana::Class; /// # use domain::base::MessageBuilder; /// # use domain::base::Name; From 13d94a16acf76f9aa1b98146527a890e613ec618 Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Sun, 2 Feb 2025 15:39:12 +0100 Subject: [PATCH 4/5] Simplify NSEC unit test code. --- src/sign/denial/nsec.rs | 49 ++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/sign/denial/nsec.rs b/src/sign/denial/nsec.rs index f7479ffa8..02894dce4 100644 --- a/src/sign/denial/nsec.rs +++ b/src/sign/denial/nsec.rs @@ -215,13 +215,13 @@ mod tests { use super::*; + type StoredSortedRecords = SortedRecords; + #[test] fn soa_is_required() { let cfg = GenerateNsecConfig::default() .without_assuming_dnskeys_will_be_added(); - let mut records = - SortedRecords::::default(); - records.insert(mk_a_rr("some_a.a.")).unwrap(); + let records = StoredSortedRecords::from_iter([mk_a_rr("some_a.a.")]); let res = generate_nsecs(records.owner_rrs(), &cfg); assert!(matches!( res, @@ -233,10 +233,10 @@ mod tests { fn multiple_soa_rrs_in_the_same_rrset_are_not_permitted() { let cfg = GenerateNsecConfig::default() .without_assuming_dnskeys_will_be_added(); - let mut records = - SortedRecords::::default(); - records.insert(mk_soa_rr("a.", "b.", "c.")).unwrap(); - records.insert(mk_soa_rr("a.", "d.", "e.")).unwrap(); + let records = StoredSortedRecords::from_iter([ + mk_soa_rr("a.", "b.", "c."), + mk_soa_rr("a.", "d.", "e."), + ]); let res = generate_nsecs(records.owner_rrs(), &cfg); assert!(matches!( res, @@ -248,13 +248,12 @@ mod tests { fn records_outside_zone_are_ignored() { let cfg = GenerateNsecConfig::default() .without_assuming_dnskeys_will_be_added(); - let mut records = - SortedRecords::::default(); - - records.insert(mk_soa_rr("b.", "d.", "e.")).unwrap(); - records.insert(mk_a_rr("some_a.b.")).unwrap(); - records.insert(mk_soa_rr("a.", "b.", "c.")).unwrap(); - records.insert(mk_a_rr("some_a.a.")).unwrap(); + let records = StoredSortedRecords::from_iter([ + mk_soa_rr("b.", "d.", "e."), + mk_a_rr("some_a.b."), + mk_soa_rr("a.", "b.", "c."), + mk_a_rr("some_a.a."), + ]); // First generate NSECs for the total record collection. As the // collection is sorted in canonical order the a zone preceeds the b @@ -290,14 +289,11 @@ mod tests { fn occluded_records_are_ignored() { let cfg = GenerateNsecConfig::default() .without_assuming_dnskeys_will_be_added(); - let mut records = - SortedRecords::::default(); - - records.insert(mk_soa_rr("a.", "b.", "c.")).unwrap(); - records - .insert(mk_ns_rr("some_ns.a.", "some_a.other.b.")) - .unwrap(); - records.insert(mk_a_rr("some_a.some_ns.a.")).unwrap(); + let records = StoredSortedRecords::from_iter([ + mk_soa_rr("a.", "b.", "c."), + mk_ns_rr("some_ns.a.", "some_a.other.b."), + mk_a_rr("some_a.some_ns.a."), + ]); let nsecs = generate_nsecs(records.owner_rrs(), &cfg).unwrap(); @@ -318,11 +314,10 @@ mod tests { fn expect_dnskeys_at_the_apex() { let cfg = GenerateNsecConfig::default(); - let mut records = - SortedRecords::::default(); - - records.insert(mk_soa_rr("a.", "b.", "c.")).unwrap(); - records.insert(mk_a_rr("some_a.a.")).unwrap(); + let records = StoredSortedRecords::from_iter([ + mk_soa_rr("a.", "b.", "c."), + mk_a_rr("some_a.a."), + ]); let nsecs = generate_nsecs(records.owner_rrs(), &cfg).unwrap(); From 37f71b27e5c7eef7d4ffc43d9c5193fdcd46ee5c Mon Sep 17 00:00:00 2001 From: Ximon Eighteen <3304436+ximon18@users.noreply.github.com> Date: Sun, 2 Feb 2025 15:40:45 +0100 Subject: [PATCH 5/5] Simplify NSEC3 unit test code and fix up the occluded test copied from the NSEC unit test suite with the changes required for the NSEC3 case. --- src/sign/denial/nsec3.rs | 117 ++++++++++++++++++--------------------- 1 file changed, 55 insertions(+), 62 deletions(-) diff --git a/src/sign/denial/nsec3.rs b/src/sign/denial/nsec3.rs index 24bb39bc3..8faeb9703 100644 --- a/src/sign/denial/nsec3.rs +++ b/src/sign/denial/nsec3.rs @@ -804,8 +804,6 @@ mod tests { use crate::sign::records::SortedRecords; use crate::sign::test_util::*; - use crate::zonetree::types::StoredRecordData; - use crate::zonetree::StoredName; use super::*; @@ -813,9 +811,8 @@ mod tests { fn soa_is_required() { let mut cfg = GenerateNsec3Config::default() .without_assuming_dnskeys_will_be_added(); - let mut records = - SortedRecords::::default(); - records.insert(mk_a_rr("some_a.a.")).unwrap(); + let records = + SortedRecords::<_, _>::from_iter([mk_a_rr("some_a.a.")]); let res = generate_nsec3s(records.owner_rrs(), &mut cfg); assert!(matches!( res, @@ -827,10 +824,10 @@ mod tests { fn multiple_soa_rrs_in_the_same_rrset_are_not_permitted() { let mut cfg = GenerateNsec3Config::default() .without_assuming_dnskeys_will_be_added(); - let mut records = - SortedRecords::::default(); - records.insert(mk_soa_rr("a.", "b.", "c.")).unwrap(); - records.insert(mk_soa_rr("a.", "d.", "e.")).unwrap(); + let records = SortedRecords::<_, _>::from_iter([ + mk_soa_rr("a.", "b.", "c."), + mk_soa_rr("a.", "d.", "e."), + ]); let res = generate_nsec3s(records.owner_rrs(), &mut cfg); assert!(matches!( res, @@ -842,25 +839,23 @@ mod tests { fn records_outside_zone_are_ignored() { let mut cfg = GenerateNsec3Config::default() .without_assuming_dnskeys_will_be_added(); - let mut records = - SortedRecords::::default(); - - records.insert(mk_soa_rr("b.", "d.", "e.")).unwrap(); - records.insert(mk_a_rr("some_a.b.")).unwrap(); - records.insert(mk_soa_rr("a.", "b.", "c.")).unwrap(); - records.insert(mk_a_rr("some_a.a.")).unwrap(); + let records = SortedRecords::<_, _>::from_iter([ + mk_soa_rr("b.", "d.", "e."), + mk_soa_rr("a.", "b.", "c."), + mk_a_rr("some_a.a."), + mk_a_rr("some_a.b."), + ]); - // First generate NSECs for the total record collection. As the + // First generate NSEC3s for the total record collection. As the // collection is sorted in canonical order the a zone preceeds the b - // zone and NSECs should only be generated for the first zone in the + // zone and NSEC3s should only be generated for the first zone in the // collection. let a_and_b_records = records.owner_rrs(); let generated_records = generate_nsec3s(a_and_b_records, &mut cfg).unwrap(); - let mut expected_records = SortedRecords::default(); - expected_records.extend([ + let expected_records = SortedRecords::<_, _>::from_iter([ mk_nsec3_rr( "a.", "a.", @@ -873,16 +868,15 @@ mod tests { assert_eq!(generated_records.nsec3s, expected_records.into_inner()); - // Now skip the a zone in the collection and generate NSECs for the - // remaining records which should only generate NSECs for the b zone. + // Now skip the a zone in the collection and generate NSEC3s for the + // remaining records which should only generate NSEC3s for the b zone. let mut b_records_only = records.owner_rrs(); b_records_only.skip_before(&mk_name("b.")); let generated_records = generate_nsec3s(b_records_only, &mut cfg).unwrap(); - let mut expected_records = SortedRecords::default(); - expected_records.extend([ + let expected_records = SortedRecords::<_, _>::from_iter([ mk_nsec3_rr( "b.", "b.", @@ -896,48 +890,47 @@ mod tests { assert_eq!(generated_records.nsec3s, expected_records.into_inner()); } - // #[test] - // fn occluded_records_are_ignored() { - // let mut cfg = GenerateNsec3Config::default() - // .without_assuming_dnskeys_will_be_added(); - // let mut records = SortedRecords::default(); + #[test] + fn occluded_records_are_ignored() { + let mut cfg = GenerateNsec3Config::default() + .without_assuming_dnskeys_will_be_added(); + let records = SortedRecords::<_, _>::from_iter([ + mk_soa_rr("a.", "b.", "c."), + mk_ns_rr("some_ns.a.", "some_a.other.b."), + mk_a_rr("some_a.some_ns.a."), + ]); - // records.insert(mk_soa_rr("a.", "b.", "c.")).unwrap(); - // records - // .insert(mk_ns_rr("some_ns.a.", "some_a.other.b.")) - // .unwrap(); - // records.insert(mk_a_rr("some_a.some_ns.a.")).unwrap(); + let generated_records = + generate_nsec3s(records.owner_rrs(), &mut cfg).unwrap(); - // let generated_records = - // generate_nsec3s(records.owner_rrs(), &mut cfg).unwrap(); + let expected_records = SortedRecords::<_, _>::from_iter([ + mk_nsec3_rr( + "a.", + "a.", + "some_ns.a.", + "SOA RRSIG NSEC3PARAM", + &cfg, + ), + // Unlike with NSEC the type bitmap for the NSEC3 for some_ns.a + // does NOT include RRSIG. This is because with NSEC "Each owner + // name in the zone that has authoritative data or a delegation + // point NS RRset MUST have an NSEC resource record" (RFC 4035 + // section 2.3), and while the zone is not authoritative for the + // NS record, "NSEC RRsets are authoritative data and are + // therefore signed" (RFC 4035 section 2.3). With NSEC3 however + // as the NSEC3 record for the unsigned delegation is generated + // (because we are not using opt out) but not stored at some_ns.a + // (but instead at .a.) then the only record at some_ns.a + // is the NS record itself which is not authoritative and so + // doesn't get an RRSIG. + mk_nsec3_rr("a.", "some_ns.a.", "a.", "NS", &cfg), + ]); - // // Implicit negative test. - // assert_eq!( - // generated_records.nsec3s, - // [ - // mk_nsec3_rr( - // "a.", - // "a.", - // "some_ns.a.", - // "SOA RRSIG NSEC", - // &mut cfg - // ), - // mk_nsec3_rr( - // "a.", - // "some_ns.a.", - // "a.", - // "NS RRSIG NSEC", - // &mut cfg - // ), - // ] - // ); + assert_eq!(generated_records.nsec3s, expected_records.into_inner()); + } - // // Explicit negative test. - // assert!(!contains_owner( - // &generated_records.nsec3s, - // "some_a.some_ns.a.example." - // )); - // } + // TODO: Repeat above test but with opt out enabled. Or add a separate opt + // test. // #[test] // fn expect_dnskeys_at_the_apex() {