From 2f0beffa8dacadd8983c6f5e1c856ba29d825aa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jan 2025 10:49:02 +0100 Subject: [PATCH 1/7] test: Write regression tests for parsing DNS records into `NodeInfo`s --- iroh/src/dns/node_info.rs | 87 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/iroh/src/dns/node_info.rs b/iroh/src/dns/node_info.rs index 4f5134323e..3e388341ac 100644 --- a/iroh/src/dns/node_info.rs +++ b/iroh/src/dns/node_info.rs @@ -405,9 +405,14 @@ fn node_domain(node_id: &NodeId, origin: &str) -> Result { #[cfg(test)] mod tests { - use std::str::FromStr; + use std::{collections::BTreeSet, str::FromStr}; - use iroh_base::SecretKey; + use hickory_resolver::{ + proto::rr::{rdata::TXT, RData, Record}, + Name, + }; + use iroh_base::{NodeId, SecretKey}; + use testresult::TestResult; use super::NodeInfo; @@ -438,4 +443,82 @@ mod tests { let actual = NodeInfo::from_pkarr_signed_packet(&packet).unwrap(); assert_eq!(expected, actual); } + + #[test] + fn regression_record_parse_multiple_txt_entries() -> TestResult { + let name = Name::from_utf8( + "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.", + )?; + let test_records = [ + Record::from_rdata( + name.clone(), + 30, + RData::TXT(TXT::new(vec!["addr=192.168.96.145:60165".to_string()])), + ), + Record::from_rdata( + name.clone(), + 30, + RData::TXT(TXT::new(vec!["addr=213.208.157.87:60165".to_string()])), + ), + Record::from_rdata( + name.clone(), + 30, + RData::TXT(TXT::new(vec![ + "relay=https://euw1-1.relay.iroh.network./".to_string() + ])), + ), + ]; + + let node_info = NodeInfo::from_hickory_records(&test_records)?; + + assert_eq!( + node_info, + NodeInfo { + node_id: NodeId::from_str( + "1992d53c02cdc04566e5c0edb1ce83305cd550297953a047a445ea3264b54b18" + )?, + relay_url: Some("https://euw1-1.relay.iroh.network./".parse()?), + direct_addresses: BTreeSet::from([ + "192.168.96.145:60165".parse()?, + "213.208.157.87:60165".parse()?, + ]) + } + ); + + Ok(()) + } + + #[test] + fn regression_record_parse_bigger_txt_record() -> TestResult { + let name = Name::from_utf8( + "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.", + )?; + let test_records = [Record::from_rdata( + name, + 30, + RData::TXT(TXT::new(vec![ + "addr=192.168.96.145:60165".to_string(), + "addr=213.208.157.87:60165".to_string(), + "relay=https://euw1-1.relay.iroh.network./".to_string(), + ])), + )]; + + let node_info = NodeInfo::from_hickory_records(&test_records)?; + + assert_eq!( + node_info, + NodeInfo { + node_id: NodeId::from_str( + "1992d53c02cdc04566e5c0edb1ce83305cd550297953a047a445ea3264b54b18" + )?, + relay_url: Some("https://euw1-1.relay.iroh.network./".parse()?), + direct_addresses: BTreeSet::from([ + "192.168.96.145:60165".parse()?, + "213.208.157.87:60165".parse()?, + ]) + } + ); + + Ok(()) + } } From 43f46f721273eeeea18ad0ef45422d2ffd36cd57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jan 2025 11:01:55 +0100 Subject: [PATCH 2/7] Use slightly better names --- iroh/src/dns/node_info.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/iroh/src/dns/node_info.rs b/iroh/src/dns/node_info.rs index 3e388341ac..7010571678 100644 --- a/iroh/src/dns/node_info.rs +++ b/iroh/src/dns/node_info.rs @@ -444,8 +444,14 @@ mod tests { assert_eq!(expected, actual); } + /// There used to be a bug where uploading a NodeAddr with more than only exactly + /// one relay URL or one publicly reachable IP addr would prevent connection + /// establishment. + /// + /// The reason was that only the first address was parsed (e.g. 192.168.96.145 in + /// this example), which could be a local, unreachable address. #[test] - fn regression_record_parse_multiple_txt_entries() -> TestResult { + fn test_dns_answer_multiple_txt_records() -> TestResult { let name = Name::from_utf8( "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.", )?; @@ -489,7 +495,7 @@ mod tests { } #[test] - fn regression_record_parse_bigger_txt_record() -> TestResult { + fn test_record_multiple_values() -> TestResult { let name = Name::from_utf8( "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.", )?; From 7a678c197fa92767ec14ce31b7cc850b2742d54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jan 2025 11:14:20 +0100 Subject: [PATCH 3/7] Remove test that was never intended to pass --- iroh/src/dns/node_info.rs | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/iroh/src/dns/node_info.rs b/iroh/src/dns/node_info.rs index 7010571678..5064771822 100644 --- a/iroh/src/dns/node_info.rs +++ b/iroh/src/dns/node_info.rs @@ -493,38 +493,4 @@ mod tests { Ok(()) } - - #[test] - fn test_record_multiple_values() -> TestResult { - let name = Name::from_utf8( - "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.", - )?; - let test_records = [Record::from_rdata( - name, - 30, - RData::TXT(TXT::new(vec![ - "addr=192.168.96.145:60165".to_string(), - "addr=213.208.157.87:60165".to_string(), - "relay=https://euw1-1.relay.iroh.network./".to_string(), - ])), - )]; - - let node_info = NodeInfo::from_hickory_records(&test_records)?; - - assert_eq!( - node_info, - NodeInfo { - node_id: NodeId::from_str( - "1992d53c02cdc04566e5c0edb1ce83305cd550297953a047a445ea3264b54b18" - )?, - relay_url: Some("https://euw1-1.relay.iroh.network./".parse()?), - direct_addresses: BTreeSet::from([ - "192.168.96.145:60165".parse()?, - "213.208.157.87:60165".parse()?, - ]) - } - ); - - Ok(()) - } } From babec3aa301a536160efb9ff8b85beb2d332ab43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jan 2025 11:15:13 +0100 Subject: [PATCH 4/7] fix: Parse multiple TXT answers into correct `NodeAddr` --- iroh/src/dns/node_info.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/iroh/src/dns/node_info.rs b/iroh/src/dns/node_info.rs index 5064771822..1eba9837a0 100644 --- a/iroh/src/dns/node_info.rs +++ b/iroh/src/dns/node_info.rs @@ -40,7 +40,7 @@ use std::{ str::FromStr, }; -use anyhow::{anyhow, ensure, Result}; +use anyhow::{anyhow, Result}; use hickory_resolver::{proto::ProtoError, Name, TokioResolver}; use iroh_base::{NodeAddr, NodeId, SecretKey}; use url::Url; @@ -240,9 +240,13 @@ impl TxtAttrs { } /// Creates [`TxtAttrs`] from a node id and an iterator of "{key}={value}" strings. - pub fn from_strings(node_id: NodeId, strings: impl Iterator) -> Result { + pub fn from_strings( + node_id: NodeId, + strings: impl Iterator>, + ) -> Result { let mut attrs: BTreeMap> = BTreeMap::new(); for s in strings { + let s = s?; let mut parts = s.split('='); let (Some(key), Some(value)) = (parts.next(), parts.next()) else { continue; @@ -307,7 +311,7 @@ impl TxtAttrs { _ => None, }); - let txt_strs = txt_data.filter_map(|s| String::try_from(s.clone()).ok()); + let txt_strs = txt_data.filter_map(|s| String::try_from(s.clone()).ok().map(Ok)); Self::from_strings(node_id, txt_strs) } @@ -323,12 +327,13 @@ impl TxtAttrs { let (node_id, first) = records.next().ok_or_else(|| { anyhow!("invalid DNS answer: no TXT record with name _iroh.z32encodedpubkey found") })?; - ensure!( - &records.all(|(n, _)| n == node_id), - "invalid DNS answer: all _iroh txt records must belong to the same node domain" - ); - let records = records.map(|(_, txt)| txt).chain(Some(first)); - let strings = records.map(ToString::to_string); + let strings = records.map(|(n, txt)| { + if n == node_id { + Ok(txt.to_string()) + } else { + Err(anyhow!("invalid DNS answer: all _iroh txt records must belong to the same node domain")) + } + }).chain(Some(Ok(first.to_string()))); Self::from_strings(node_id, strings) } From c5399fb7df61c457cde46dc49134fe0c5b4bb844 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jan 2025 12:03:51 +0100 Subject: [PATCH 5/7] refactor: Make Use `TxtLookup` type in favor of `&[Record]` --- iroh/src/dns/node_info.rs | 59 ++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/iroh/src/dns/node_info.rs b/iroh/src/dns/node_info.rs index 1eba9837a0..2cdd4be4ae 100644 --- a/iroh/src/dns/node_info.rs +++ b/iroh/src/dns/node_info.rs @@ -162,9 +162,9 @@ impl NodeInfo { self.into() } - /// Parses a [`NodeInfo`] from a set of DNS records. - pub fn from_hickory_records(records: &[hickory_resolver::proto::rr::Record]) -> Result { - let attrs = TxtAttrs::from_hickory_records(records)?; + /// Parses a [`NodeInfo`] from a TXT records lookup. + pub fn from_hickory_lookup(lookup: hickory_resolver::lookup::TxtLookup) -> Result { + let attrs = TxtAttrs::from_hickory_lookup(lookup)?; Ok(attrs.into()) } @@ -240,13 +240,9 @@ impl TxtAttrs { } /// Creates [`TxtAttrs`] from a node id and an iterator of "{key}={value}" strings. - pub fn from_strings( - node_id: NodeId, - strings: impl Iterator>, - ) -> Result { + pub fn from_strings(node_id: NodeId, strings: impl Iterator) -> Result { let mut attrs: BTreeMap> = BTreeMap::new(); for s in strings { - let s = s?; let mut parts = s.split('='); let (Some(key), Some(value)) = (parts.next(), parts.next()) else { continue; @@ -262,7 +258,7 @@ impl TxtAttrs { async fn lookup(resolver: &TokioResolver, name: Name) -> Result { let name = ensure_iroh_txt_label(name)?; let lookup = resolver.txt_lookup(name).await?; - let attrs = Self::from_hickory_records(lookup.as_lookup().records())?; + let attrs = Self::from_hickory_lookup(lookup)?; Ok(attrs) } @@ -311,29 +307,23 @@ impl TxtAttrs { _ => None, }); - let txt_strs = txt_data.filter_map(|s| String::try_from(s.clone()).ok().map(Ok)); + let txt_strs = txt_data.filter_map(|s| String::try_from(s.clone()).ok()); Self::from_strings(node_id, txt_strs) } - /// Parses a set of DNS resource records. - pub fn from_hickory_records(records: &[hickory_resolver::proto::rr::Record]) -> Result { - use hickory_resolver::proto::rr; - let mut records = records.iter().filter_map(|rr| match rr.data() { - rr::RData::TXT(txt) => { - node_id_from_hickory_name(rr.name()).map(|node_id| (node_id, txt)) + /// Parses a TXT records lookup. + pub fn from_hickory_lookup(lookup: hickory_resolver::lookup::TxtLookup) -> Result { + let node_id = node_id_from_hickory_name(lookup.query().name()) + .ok_or_else(|| anyhow!("invalid DNS answer: not a query for _iroh.z32encodedpubkey"))?; + + let strings = lookup.as_lookup().record_iter().filter_map(|record| { + match node_id_from_hickory_name(record.name()) { + // Filter out only TXT record answers that match the node_id we searched for. + Some(n) if n == node_id => record.data().as_txt().map(|txt| txt.to_string()), + _ => None, } - _ => None, }); - let (node_id, first) = records.next().ok_or_else(|| { - anyhow!("invalid DNS answer: no TXT record with name _iroh.z32encodedpubkey found") - })?; - let strings = records.map(|(n, txt)| { - if n == node_id { - Ok(txt.to_string()) - } else { - Err(anyhow!("invalid DNS answer: all _iroh txt records must belong to the same node domain")) - } - }).chain(Some(Ok(first.to_string()))); + Self::from_strings(node_id, strings) } @@ -410,10 +400,14 @@ fn node_domain(node_id: &NodeId, origin: &str) -> Result { #[cfg(test)] mod tests { - use std::{collections::BTreeSet, str::FromStr}; + use std::{collections::BTreeSet, str::FromStr, sync::Arc}; use hickory_resolver::{ - proto::rr::{rdata::TXT, RData, Record}, + lookup::Lookup, + proto::{ + op::Query, + rr::{rdata::TXT, RData, Record, RecordType}, + }, Name, }; use iroh_base::{NodeId, SecretKey}; @@ -460,7 +454,8 @@ mod tests { let name = Name::from_utf8( "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.", )?; - let test_records = [ + let query = Query::query(name.clone(), RecordType::TXT); + let records = [ Record::from_rdata( name.clone(), 30, @@ -479,9 +474,9 @@ mod tests { ])), ), ]; + let lookup = Lookup::new_with_max_ttl(query, Arc::new(records)); - let node_info = NodeInfo::from_hickory_records(&test_records)?; - + let node_info = NodeInfo::from_hickory_lookup(lookup.into())?; assert_eq!( node_info, NodeInfo { From 8e5c89fcdac0c1873f9fd44d5ce1948fe0ccbe24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jan 2025 12:20:59 +0100 Subject: [PATCH 6/7] Test some extra cases --- iroh/src/dns/node_info.rs | 67 +++++++++++++++++++++++++++++++++++---- 1 file changed, 61 insertions(+), 6 deletions(-) diff --git a/iroh/src/dns/node_info.rs b/iroh/src/dns/node_info.rs index 2cdd4be4ae..fea258bbb8 100644 --- a/iroh/src/dns/node_info.rs +++ b/iroh/src/dns/node_info.rs @@ -43,6 +43,7 @@ use std::{ use anyhow::{anyhow, Result}; use hickory_resolver::{proto::ProtoError, Name, TokioResolver}; use iroh_base::{NodeAddr, NodeId, SecretKey}; +use tracing::warn; use url::Url; /// The DNS name for the iroh TXT record. @@ -313,18 +314,43 @@ impl TxtAttrs { /// Parses a TXT records lookup. pub fn from_hickory_lookup(lookup: hickory_resolver::lookup::TxtLookup) -> Result { - let node_id = node_id_from_hickory_name(lookup.query().name()) + let queried_node_id = node_id_from_hickory_name(lookup.query().name()) .ok_or_else(|| anyhow!("invalid DNS answer: not a query for _iroh.z32encodedpubkey"))?; let strings = lookup.as_lookup().record_iter().filter_map(|record| { match node_id_from_hickory_name(record.name()) { // Filter out only TXT record answers that match the node_id we searched for. - Some(n) if n == node_id => record.data().as_txt().map(|txt| txt.to_string()), - _ => None, + Some(n) if n == queried_node_id => match record.data().as_txt() { + Some(txt) => Some(txt.to_string()), + None => { + warn!( + ?queried_node_id, + data = ?record.data(), + "unexpected record type for DNS discovery query" + ); + None + } + }, + Some(answered_node_id) => { + warn!( + ?queried_node_id, + ?answered_node_id, + "unexpected node ID answered for DNS query" + ); + None + } + None => { + warn!( + ?queried_node_id, + name = ?record.name(), + "unexpected answer record name for DNS query" + ); + None + } } }); - Self::from_strings(node_id, strings) + Self::from_strings(queried_node_id, strings) } fn to_txt_strings(&self) -> impl Iterator + '_ { @@ -406,13 +432,18 @@ mod tests { lookup::Lookup, proto::{ op::Query, - rr::{rdata::TXT, RData, Record, RecordType}, + rr::{ + rdata::{A, TXT}, + RData, Record, RecordType, + }, }, Name, }; use iroh_base::{NodeId, SecretKey}; use testresult::TestResult; + use crate::dns::node_info::to_z32; + use super::NodeInfo; #[test] @@ -450,7 +481,7 @@ mod tests { /// The reason was that only the first address was parsed (e.g. 192.168.96.145 in /// this example), which could be a local, unreachable address. #[test] - fn test_dns_answer_multiple_txt_records() -> TestResult { + fn test_from_hickory_lookup() -> TestResult { let name = Name::from_utf8( "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.", )?; @@ -466,6 +497,30 @@ mod tests { 30, RData::TXT(TXT::new(vec!["addr=213.208.157.87:60165".to_string()])), ), + // Test a record with mismatching record type (A instead of TXT). It should be filtered out. + Record::from_rdata(name.clone(), 30, RData::A(A::new(127, 0, 0, 1))), + // Test a record with a mismatching name + Record::from_rdata( + Name::from_utf8(&format!( + "_iroh.{}.dns.iroh.link.", + to_z32(&NodeId::from_str( + // Another NodeId + "a55f26132e5e43de834d534332f66a20d480c3e50a13a312a071adea6569981e" + )?) + ))?, + 30, + RData::TXT(TXT::new(vec![ + "relay=https://euw1-1.relay.iroh.network./".to_string() + ])), + ), + // Test a record with a completely different name + Record::from_rdata( + Name::from_utf8("dns.iroh.link.")?, + 30, + RData::TXT(TXT::new(vec![ + "relay=https://euw1-1.relay.iroh.network./".to_string() + ])), + ), Record::from_rdata( name.clone(), 30, From 76d27f64933adc2e596c9474ab19cfb9f9cf9ad2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Wed, 8 Jan 2025 16:56:28 +0100 Subject: [PATCH 7/7] `cargo make format` and clippy fix --- iroh/src/dns/node_info.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/iroh/src/dns/node_info.rs b/iroh/src/dns/node_info.rs index fea258bbb8..1d7c1f471f 100644 --- a/iroh/src/dns/node_info.rs +++ b/iroh/src/dns/node_info.rs @@ -442,9 +442,8 @@ mod tests { use iroh_base::{NodeId, SecretKey}; use testresult::TestResult; - use crate::dns::node_info::to_z32; - use super::NodeInfo; + use crate::dns::node_info::to_z32; #[test] fn txt_attr_roundtrip() { @@ -501,7 +500,7 @@ mod tests { Record::from_rdata(name.clone(), 30, RData::A(A::new(127, 0, 0, 1))), // Test a record with a mismatching name Record::from_rdata( - Name::from_utf8(&format!( + Name::from_utf8(format!( "_iroh.{}.dns.iroh.link.", to_z32(&NodeId::from_str( // Another NodeId