fix(iroh-net): prevent adding addressing info that points back to us (#2333)

## Description

Failed disco boxes for which the sender is our own node is are because
we tried to contact another node for which the addressing information we
have points back to us. We are the sender (this explains the source of
the disco box being ourselves) but we are not the meant recipient (thus
the failed decryption - we should always be able to decrypt a message
meant to ourselves - ).

This is of course undesirable. This Pr should mostly mitigate this
issue, but a failed disco box to ourselves can happen in other less
likely ways.

## Breaking Changes

n/a

## Notes & open questions

n/a

## Change checklist

- [x] Self-review.
- [x] Documentation updates if relevant.
- [ ] ~Tests if relevant.~
- [ ] ~All breaking changes documented.~

Author: divagant-martian

iroh-docs/src/engine/live.rs

@@ -31,6 +31,9 @@ use crate::{
 use super::gossip::{GossipActor, ToGossipActor};
 use super::state::{NamespaceStates, Origin, SyncReason};
 
+/// Name used for logging when new node addresses are added from the docs engine.
+const SOURCE_NAME: &str = "docs_engine";
+
 /// An iroh-docs operation
 ///
 /// This is the message that is broadcast over iroh-gossip.
@@ -437,7 +440,7 @@ impl<B: iroh_blobs::store::Store> LiveActor<B> {
         // add addresses of peers to our endpoint address book
         for peer in peers.into_iter() {
             let peer_id = peer.node_id;
-            if let Err(err) = self.endpoint.add_node_addr(peer) {
+            if let Err(err) = self.endpoint.add_node_addr_with_source(peer, SOURCE_NAME) {
                 warn!(peer = %peer_id.fmt_short(), "failed to add known addrs: {err:?}");
             }
         }

iroh-gossip/src/net.rs

@@ -38,6 +38,8 @@ const TO_ACTOR_CAP: usize = 64;
 const IN_EVENT_CAP: usize = 1024;
 /// Channel capacity for endpoint change message queue (single)
 const ON_ENDPOINTS_CAP: usize = 64;
+/// Name used for logging when new node addresses are added from gossip.
+const SOURCE_NAME: &str = "gossip";
 
 /// Events emitted from the gossip protocol
 pub type Event = proto::Event<PublicKey>;
@@ -578,7 +580,10 @@ impl Actor {
                     Ok(info) => {
                         debug!(peer = ?node_id, "add known addrs: {info:?}");
                         let node_addr = NodeAddr { node_id, info };
-                        if let Err(err) = self.endpoint.add_node_addr(node_addr) {
+                        if let Err(err) = self
+                            .endpoint
+                            .add_node_addr_with_source(node_addr, SOURCE_NAME)
+                        {
                             debug!(peer = ?node_id, "add known failed: {err:?}");
                         }
                     }

iroh-net/src/discovery.rs

@@ -13,6 +13,9 @@ use crate::{AddrInfo, Endpoint, NodeId};
 pub mod dns;
 pub mod pkarr_publish;
 
+/// Name used for logging when new node addresses are added from discovery.
+const SOURCE_NAME: &str = "discovery";
+
 /// Node discovery for [`super::Endpoint`].
 ///
 /// The purpose of this trait is to hook up a node discovery mechanism that
@@ -252,7 +255,7 @@ impl DiscoveryTask {
                         info: r.addr_info,
                         node_id,
                     };
-                    ep.add_node_addr(addr).ok();
+                    ep.add_node_addr_with_source(addr, SOURCE_NAME).ok();
                     if let Some(tx) = on_first_tx.take() {
                         tx.send(Ok(())).ok();
                     }
@@ -669,11 +672,6 @@ mod test_dns_pkarr {
         // wait until our shared state received the update from pkarr publishing
         dns_pkarr_server.on_node(&ep1.node_id(), timeout).await?;
 
-        let node_addr = NodeAddr::new(ep1.node_id());
-
-        // add empty node address. We *should* launch discovery before attempting to dial.
-        ep2.add_node_addr(node_addr)?;
-
         // we connect only by node id!
         let res = ep2.connect(ep1.node_id().into(), TEST_ALPN).await;
         assert!(res.is_ok(), "connection established");

iroh-net/src/endpoint.rs

@@ -531,21 +531,48 @@ impl Endpoint {
     /// This updates the local state for the remote node.  If the provided [`NodeAddr`]
     /// contains a [`RelayUrl`] this will be used as the new relay server for this node.  If
     /// it contains any new IP endpoints they will also be stored and tried when next
-    /// connecting to this node.
+    /// connecting to this node. Any address that matches this node's direct addresses will be
+    /// silently ignored.
+    ///
+    /// See also [`Endpoint::add_node_addr_with_source`].
     ///
     /// # Errors
     ///
-    /// Will return an error if we attempt to add our own [`PublicKey`] to the node map.
+    /// Will return an error if we attempt to add our own [`PublicKey`] to the node map or if the
+    /// direct addresses are a subset of ours.
     pub fn add_node_addr(&self, node_addr: NodeAddr) -> Result<()> {
+        self.add_node_addr_inner(node_addr, magicsock::Source::App)
+    }
+
+    /// Informs this [`Endpoint`] about addresses of the iroh-net node, noting the source.
+    ///
+    /// This updates the local state for the remote node.  If the provided [`NodeAddr`] contains a
+    /// [`RelayUrl`] this will be used as the new relay server for this node.  If it contains any
+    /// new IP endpoints they will also be stored and tried when next connecting to this node. Any
+    /// address that matches this node's direct addresses will be silently ignored. The *source* is
+    /// used for logging exclusively and will not be stored.
+    ///
+    /// # Errors
+    ///
+    /// Will return an error if we attempt to add our own [`PublicKey`] to the node map or if the
+    /// direct addresses are a subset of ours.
+    pub fn add_node_addr_with_source(
+        &self,
+        node_addr: NodeAddr,
+        source: &'static str,
+    ) -> Result<()> {
+        self.add_node_addr_inner(node_addr, magicsock::Source::NamedApp { name: source })
+    }
+
+    fn add_node_addr_inner(&self, node_addr: NodeAddr, source: magicsock::Source) -> Result<()> {
         // Connecting to ourselves is not supported.
         if node_addr.node_id == self.node_id() {
             bail!(
                 "Adding our own address is not supported ({} is the node id of this node)",
                 node_addr.node_id.fmt_short()
             );
         }
-        self.msock.add_node_addr(node_addr);
-        Ok(())
+        self.msock.add_node_addr(node_addr, source)
     }
 
     // # Getter methods for properties of this Endpoint itself.
@@ -1386,8 +1413,9 @@ mod tests {
 
     #[tokio::test]
     async fn endpoint_conn_type_stream() {
+        const TIMEOUT: Duration = std::time::Duration::from_secs(15);
         let _logging_guard = iroh_test::logging::setup();
-        let (relay_map, relay_url, _relay_guard) = run_relay_server().await.unwrap();
+        let (relay_map, _relay_url, _relay_guard) = run_relay_server().await.unwrap();
         let mut rng = rand_chacha::ChaCha8Rng::seed_from_u64(42);
         let ep1_secret_key = SecretKey::generate_with_rng(&mut rng);
         let ep2_secret_key = SecretKey::generate_with_rng(&mut rng);
@@ -1408,31 +1436,25 @@ mod tests {
             .await
             .unwrap();
 
-        async fn handle_direct_conn(ep: Endpoint, node_id: PublicKey) -> Result<()> {
-            let node_addr = NodeAddr::new(node_id);
-            ep.add_node_addr(node_addr)?;
-            let stream = ep.conn_type_stream(node_id)?;
-            async fn get_direct_event(
-                src: &PublicKey,
-                dst: &PublicKey,
-                mut stream: ConnectionTypeStream,
-            ) -> Result<()> {
-                let src = src.fmt_short();
-                let dst = dst.fmt_short();
-                while let Some(conn_type) = stream.next().await {
-                    tracing::info!(me = %src, dst = %dst, conn_type = ?conn_type);
-                    if matches!(conn_type, ConnectionType::Direct(_)) {
-                        return Ok(());
-                    }
+        async fn handle_direct_conn(ep: &Endpoint, node_id: PublicKey) -> Result<()> {
+            let mut stream = ep.conn_type_stream(node_id)?;
+            let src = ep.node_id().fmt_short();
+            let dst = node_id.fmt_short();
+            while let Some(conn_type) = stream.next().await {
+                tracing::info!(me = %src, dst = %dst, conn_type = ?conn_type);
+                if matches!(conn_type, ConnectionType::Direct(_)) {
+                    return Ok(());
                 }
-                anyhow::bail!("conn_type stream ended before `ConnectionType::Direct`");
             }
-            tokio::time::timeout(
-                Duration::from_secs(15),
-                get_direct_event(&ep.node_id(), &node_id, stream),
-            )
-            .await??;
-            Ok(())
+            anyhow::bail!("conn_type stream ended before `ConnectionType::Direct`");
+        }
+
+        async fn accept(ep: &Endpoint) -> NodeId {
+            let incoming = ep.accept().await.unwrap();
+            let conn = incoming.await.unwrap();
+            let node_id = get_remote_node_id(&conn).unwrap();
+            tracing::info!(node_id=%node_id.fmt_short(), "accepted connection");
+            node_id
         }
 
         let ep1_nodeid = ep1.node_id();
@@ -1445,39 +1467,31 @@ mod tests {
         );
         tracing::info!("node id 2 {ep2_nodeid}");
 
-        let res_ep1 = tokio::spawn(handle_direct_conn(ep1.clone(), ep2_nodeid));
+        let ep1_side = async move {
+            accept(&ep1).await;
+            handle_direct_conn(&ep1, ep2_nodeid).await
+        };
+
+        let ep2_side = async move {
+            ep2.connect(ep1_nodeaddr, TEST_ALPN).await.unwrap();
+            handle_direct_conn(&ep2, ep1_nodeid).await
+        };
+
+        let res_ep1 = tokio::spawn(tokio::time::timeout(TIMEOUT, ep1_side));
 
         let ep1_abort_handle = res_ep1.abort_handle();
         let _ep1_guard = CallOnDrop::new(move || {
             ep1_abort_handle.abort();
         });
 
-        let res_ep2 = tokio::spawn(handle_direct_conn(ep2.clone(), ep1_nodeid));
+        let res_ep2 = tokio::spawn(tokio::time::timeout(TIMEOUT, ep2_side));
         let ep2_abort_handle = res_ep2.abort_handle();
         let _ep2_guard = CallOnDrop::new(move || {
             ep2_abort_handle.abort();
         });
-        async fn accept(ep: Endpoint) -> NodeId {
-            let incoming = ep.accept().await.unwrap();
-            let conn = incoming.await.unwrap();
-            get_remote_node_id(&conn).unwrap()
-        }
-
-        // create a node addr with no direct connections
-        let ep1_nodeaddr = NodeAddr::from_parts(ep1_nodeid, Some(relay_url), vec![]);
-
-        let accept_res = tokio::spawn(accept(ep1.clone()));
-        let accept_abort_handle = accept_res.abort_handle();
-        let _accept_guard = CallOnDrop::new(move || {
-            accept_abort_handle.abort();
-        });
-
-        let _conn_2 = ep2.connect(ep1_nodeaddr, TEST_ALPN).await.unwrap();
-
-        let got_id = accept_res.await.unwrap();
-        assert_eq!(ep2_nodeid, got_id);
 
-        res_ep1.await.unwrap().unwrap();
-        res_ep2.await.unwrap().unwrap();
+        let (r1, r2) = tokio::try_join!(res_ep1, res_ep2).unwrap();
+        r1.expect("ep1 timeout").unwrap();
+        r2.expect("ep2 timeout").unwrap();
     }
 }

iroh-net/src/magicsock.rs

@@ -80,6 +80,7 @@ pub use self::node_map::{
     ConnectionType, ConnectionTypeStream, ControlMsg, DirectAddrInfo, NodeInfo as ConnectionInfo,
 };
 pub(super) use self::timer::Timer;
+pub(crate) use node_map::Source;
 
 /// How long we consider a STUN-derived endpoint valid for. UDP NAT mappings typically
 /// expire at 30 seconds, so this is a few seconds shy of that.
@@ -367,8 +368,39 @@ impl MagicSock {
 
     /// Add addresses for a node to the magic socket's addresbook.
     #[instrument(skip_all, fields(me = %self.me))]
-    pub(crate) fn add_node_addr(&self, addr: NodeAddr) {
-        self.node_map.add_node_addr(addr);
+    pub fn add_node_addr(&self, mut addr: NodeAddr, source: node_map::Source) -> Result<()> {
+        let my_addresses = self.endpoints.get().last_endpoints;
+        let mut pruned = 0;
+        for my_addr in my_addresses.into_iter().map(|ep| ep.addr) {
+            if addr.info.direct_addresses.remove(&my_addr) {
+                warn!(node_id=addr.node_id.fmt_short(), %my_addr, %source, "not adding our addr for node");
+                pruned += 1;
+            }
+        }
+        if !addr.info.is_empty() {
+            self.node_map.add_node_addr(addr, source);
+            Ok(())
+        } else if pruned != 0 {
+            Err(anyhow::anyhow!(
+                "empty addressing info, {pruned} direct addresses have been pruned"
+            ))
+        } else {
+            Err(anyhow::anyhow!("empty addressing info"))
+        }
+    }
+
+    /// Updates our direct addresses.
+    ///
+    /// On a successful update, our address is published to discovery.
+    pub(super) fn update_direct_addresses(&self, eps: Vec<DirectAddr>) {
+        let updated = self.endpoints.update(DiscoveredEndpoints::new(eps)).is_ok();
+        if updated {
+            let eps = self.endpoints.read();
+            eps.log_endpoint_change();
+            self.node_map
+                .on_direct_addr_discovered(eps.iter().map(|ep| ep.addr));
+            self.publish_my_addr();
+        }
     }
 
     /// Get a reference to the DNS resolver used in this [`MagicSock`].
@@ -2095,15 +2127,7 @@ impl Actor {
             // The STUN address(es) are always first.
             // Despite this sorting, clients are not relying on this sorting for decisions;
 
-            let updated = msock
-                .endpoints
-                .update(DiscoveredEndpoints::new(eps))
-                .is_ok();
-            if updated {
-                let eps = msock.endpoints.read();
-                eps.log_endpoint_change();
-                msock.publish_my_addr();
-            }
+            msock.update_direct_addresses(eps);
 
             // Regardless of whether our local endpoints changed, we now want to send any queued
             // call-me-maybe messages.
@@ -2722,6 +2746,14 @@ mod tests {
 
     use super::*;
 
+    impl MagicSock {
+        #[track_caller]
+        pub fn add_test_addr(&self, node_addr: NodeAddr) {
+            self.add_node_addr(node_addr, Source::NamedApp { name: "test" })
+                .unwrap()
+        }
+    }
+
     /// Magicsock plus wrappers for sending packets
     #[derive(Clone)]
     struct MagicStack {
@@ -2791,7 +2823,7 @@ mod tests {
                         direct_addresses: new_addrs.iter().map(|ep| ep.addr).collect(),
                     },
                 };
-                m.endpoint.magic_sock().add_node_addr(addr);
+                m.endpoint.magic_sock().add_test_addr(addr);
             }
         }