identify: fix normalization of interface listen addresses (#2250)

* Fix listenAddrs comparison in maybeRecordObservation * Add test * identify: use network.ConnMultiaddrs interface --------- Co-authored-by: Marten Seemann <[email protected]>
libp2p · Apr 12, 2023 · ba328ae · ba328ae
1 parent 952f7cb
commit ba328ae
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 23 deletions.
diff --git a/p2p/protocol/identify/obsaddr.go b/p2p/protocol/identify/obsaddr.go
@@ -360,26 +360,35 @@ type normalizeMultiaddrer interface {
 	NormalizeMultiaddr(addr ma.Multiaddr) ma.Multiaddr
 }
 
-func (oas *ObservedAddrManager) maybeRecordObservation(conn network.Conn, observed ma.Multiaddr) {
+type addrsProvider interface {
+	Addrs() []ma.Multiaddr
+}
+
+type listenAddrsProvider interface {
+	ListenAddresses() []ma.Multiaddr
+	InterfaceListenAddresses() ([]ma.Multiaddr, error)
+}
+
+func shouldRecordObservation(host addrsProvider, network listenAddrsProvider, conn network.ConnMultiaddrs, observed ma.Multiaddr) bool {
 	// First, determine if this observation is even worth keeping...
 
 	// Ignore observations from loopback nodes. We already know our loopback
 	// addresses.
 	if manet.IsIPLoopback(observed) {
-		return
+		return false
 	}
 
 	// we should only use ObservedAddr when our connection's LocalAddr is one
 	// of our ListenAddrs. If we Dial out using an ephemeral addr, knowing that
 	// address's external mapping is not very useful because the port will not be
 	// the same as the listen addr.
-	ifaceaddrs, err := oas.host.Network().InterfaceListenAddresses()
+	ifaceaddrs, err := network.InterfaceListenAddresses()
 	if err != nil {
 		log.Infof("failed to get interface listen addrs", err)
-		return
+		return false
 	}
 
-	normalizer, canNormalize := oas.host.(normalizeMultiaddrer)
+	normalizer, canNormalize := host.(normalizeMultiaddrer)
 
 	if canNormalize {
 		for i, a := range ifaceaddrs {
@@ -391,23 +400,25 @@ func (oas *ObservedAddrManager) maybeRecordObservation(conn network.Conn, observ
 	if canNormalize {
 		local = normalizer.NormalizeMultiaddr(local)
 	}
-	if !ma.Contains(ifaceaddrs, local) && !ma.Contains(oas.host.Network().ListenAddresses(), local) {
+
+	listenAddrs := network.ListenAddresses()
+	if canNormalize {
+		for i, a := range listenAddrs {
+			listenAddrs[i] = normalizer.NormalizeMultiaddr(a)
+		}
+	}
+
+	if !ma.Contains(ifaceaddrs, local) && !ma.Contains(listenAddrs, local) {
 		// not in our list
-		return
+		return false
 	}
 
-	hostAddrs := oas.host.Addrs()
+	hostAddrs := host.Addrs()
 	if canNormalize {
 		for i, a := range hostAddrs {
 			hostAddrs[i] = normalizer.NormalizeMultiaddr(a)
 		}
 	}
-	listenAddrs := oas.host.Network().ListenAddresses()
-	if canNormalize {
-		for i, a := range listenAddrs {
-			listenAddrs[i] = normalizer.NormalizeMultiaddr(a)
-		}
-	}
 
 	// We should reject the connection if the observation doesn't match the
 	// transports of one of our advertised addresses.
@@ -418,20 +429,26 @@ func (oas *ObservedAddrManager) maybeRecordObservation(conn network.Conn, observ
 			"from", conn.RemoteMultiaddr(),
 			"observed", observed,
 		)
-		return
+		return false
 	}
 
-	// Ok, the observation is good, record it.
-	log.Debugw("added own observed listen addr", "observed", observed)
+	return true
+}
 
-	defer oas.addConn(conn, observed)
+func (oas *ObservedAddrManager) maybeRecordObservation(conn network.Conn, observed ma.Multiaddr) {
+	shouldRecord := shouldRecordObservation(oas.host, oas.host.Network(), conn, observed)
+	if shouldRecord {
+		// Ok, the observation is good, record it.
+		log.Debugw("added own observed listen addr", "observed", observed)
+		defer oas.addConn(conn, observed)
 
-	oas.mu.Lock()
-	defer oas.mu.Unlock()
-	oas.recordObservationUnlocked(conn, observed)
+		oas.mu.Lock()
+		defer oas.mu.Unlock()
+		oas.recordObservationUnlocked(conn, observed)
 
-	if oas.reachability == network.ReachabilityPrivate {
-		oas.emitAllNATTypes()
+		if oas.reachability == network.ReachabilityPrivate {
+			oas.emitAllNATTypes()
+		}
 	}
 }
 

diff --git a/p2p/protocol/identify/obsaddr_glass_test.go b/p2p/protocol/identify/obsaddr_glass_test.go
@@ -36,3 +36,70 @@ func TestObservedAddrGroupKey(t *testing.T) {
 	require.Equal(t, oa6.groupKey(), oa7.groupKey())
 	require.NotEqual(t, oa7.groupKey(), oa8.groupKey())
 }
+
+type mockHost struct {
+	addrs            []ma.Multiaddr
+	listenAddrs      []ma.Multiaddr
+	ifaceListenAddrs []ma.Multiaddr
+}
+
+// InterfaceListenAddresses implements listenAddrsProvider
+func (h *mockHost) InterfaceListenAddresses() ([]ma.Multiaddr, error) {
+	return h.ifaceListenAddrs, nil
+}
+
+// ListenAddresses implements listenAddrsProvider
+func (h *mockHost) ListenAddresses() []ma.Multiaddr {
+	return h.listenAddrs
+}
+
+// Addrs implements addrsProvider
+func (h *mockHost) Addrs() []ma.Multiaddr {
+	return h.addrs
+}
+
+// NormalizeMultiaddr implements normalizeMultiaddrer
+func (h *mockHost) NormalizeMultiaddr(m ma.Multiaddr) ma.Multiaddr {
+	original := m
+	for {
+		rest, tail := ma.SplitLast(m)
+		if rest == nil {
+			return original
+		}
+		if tail.Protocol().Code == ma.P_WEBTRANSPORT {
+			return m
+		}
+		m = rest
+	}
+}
+
+type mockConn struct {
+	local, remote ma.Multiaddr
+}
+
+// LocalMultiaddr implements connMultiaddrProvider
+func (c *mockConn) LocalMultiaddr() ma.Multiaddr {
+	return c.local
+}
+
+// RemoteMultiaddr implements connMultiaddrProvider
+func (c *mockConn) RemoteMultiaddr() ma.Multiaddr {
+	return c.remote
+}
+
+func TestShouldRecordObservationWithWebTransport(t *testing.T) {
+	listenAddr := ma.StringCast("/ip4/0.0.0.0/udp/0/quic-v1/webtransport/certhash/uEgNmb28")
+	ifaceAddr := ma.StringCast("/ip4/10.0.0.2/udp/9999/quic-v1/webtransport/certhash/uEgNmb28")
+	h := &mockHost{
+		listenAddrs:      []ma.Multiaddr{listenAddr},
+		ifaceListenAddrs: []ma.Multiaddr{ifaceAddr},
+		addrs:            []ma.Multiaddr{listenAddr},
+	}
+	c := &mockConn{
+		local:  listenAddr,
+		remote: ma.StringCast("/ip4/1.2.3.6/udp/1236/quic-v1/webtransport"),
+	}
+	observedAddr := ma.StringCast("/ip4/1.2.3.4/udp/1231/quic-v1/webtransport")
+
+	require.True(t, shouldRecordObservation(h, h, c, observedAddr))
+}