fix(iroh): Parse DNS answer with multiple records into a proper NodeAddr

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;
@@ -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<Self> {
-        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<Self> {
+        let attrs = TxtAttrs::from_hickory_lookup(lookup)?;
         Ok(attrs.into())
     }
 
@@ -258,7 +258,7 @@ impl<T: FromStr + Display + Hash + Ord> TxtAttrs<T> {
     async fn lookup(resolver: &TokioResolver, name: Name) -> Result<Self> {
         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,24 +311,19 @@ impl<T: FromStr + Display + Hash + Ord> TxtAttrs<T> {
         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<Self> {
-        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<Self> {
+        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")
-        })?;
-        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);
+
         Self::from_strings(node_id, strings)
     }
 
@@ -405,9 +400,18 @@ fn node_domain(node_id: &NodeId, origin: &str) -> Result<Name> {
 
 #[cfg(test)]
 mod tests {
-    use std::str::FromStr;
-
-    use iroh_base::SecretKey;
+    use std::{collections::BTreeSet, str::FromStr, sync::Arc};
+
+    use hickory_resolver::{
+        lookup::Lookup,
+        proto::{
+            op::Query,
+            rr::{rdata::TXT, RData, Record, RecordType},
+        },
+        Name,
+    };
+    use iroh_base::{NodeId, SecretKey};
+    use testresult::TestResult;
 
     use super::NodeInfo;
 
@@ -438,4 +442,55 @@ mod tests {
         let actual = NodeInfo::from_pkarr_signed_packet(&packet).unwrap();
         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 test_dns_answer_multiple_txt_records() -> TestResult {
+        let name = Name::from_utf8(
+            "_iroh.dgjpkxyn3zyrk3zfads5duwdgbqpkwbjxfj4yt7rezidr3fijccy.dns.iroh.link.",
+        )?;
+        let query = Query::query(name.clone(), RecordType::TXT);
+        let 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 lookup = Lookup::new_with_max_ttl(query, Arc::new(records));
+
+        let node_info = NodeInfo::from_hickory_lookup(lookup.into())?;
+        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(())
+    }
 }