Skip to content

Commit

Permalink
fix: obsaddr: do not record observations over relayed conn
Browse files Browse the repository at this point in the history
These observations are telling us what the relay's IP address is. This
isn't useful for us at all.

This also solves an issue during hole punching where we may report the
relay's address as our own.
  • Loading branch information
MarcoPolo committed Nov 15, 2024
1 parent 7268c98 commit 7b95317
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
13 changes: 12 additions & 1 deletion p2p/protocol/identify/obsaddr.go
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,11 @@ func (o *ObservedAddrManager) worker() {
}
}

func isRelayedAddress(a ma.Multiaddr) bool {
_, err := a.ValueForProtocol(ma.P_CIRCUIT)
return err == nil
}

func (o *ObservedAddrManager) shouldRecordObservation(conn connMultiaddrs, observed ma.Multiaddr) (shouldRecord bool, localTW thinWaist, observedTW thinWaist) {
if conn == nil || observed == nil {
return false, thinWaist{}, thinWaist{}
Expand All @@ -350,6 +355,12 @@ func (o *ObservedAddrManager) shouldRecordObservation(conn connMultiaddrs, obser
return false, thinWaist{}, thinWaist{}
}

// Ignore p2p-circuit addresses. These are the observed address of the relay.
// Not useful for us.
if isRelayedAddress(observed) {
return false, thinWaist{}, thinWaist{}
}

// 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
Expand Down Expand Up @@ -410,7 +421,7 @@ func (o *ObservedAddrManager) maybeRecordObservation(conn connMultiaddrs, observ
if !shouldRecord {
return
}
log.Debugw("added own observed listen addr", "observed", observed)
log.Debugw("added own observed listen addr", "conn", conn, "observed", observed)

o.mu.Lock()
defer o.mu.Unlock()
Expand Down
18 changes: 18 additions & 0 deletions p2p/protocol/identify/obsaddr_glass_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,24 @@ func TestShouldRecordObservationWithWebTransport(t *testing.T) {
require.True(t, shouldRecord)
}

func TestShouldNotRecordObservationWithRelayedAddr(t *testing.T) {
listenAddr := ma.StringCast("/ip4/1.2.3.4/udp/8888/quic-v1/p2p-circuit")
ifaceAddr := ma.StringCast("/ip4/10.0.0.2/udp/9999/quic-v1")
listenAddrs := func() []ma.Multiaddr { return []ma.Multiaddr{listenAddr} }
ifaceListenAddrs := func() ([]ma.Multiaddr, error) { return []ma.Multiaddr{ifaceAddr}, nil }
addrs := func() []ma.Multiaddr { return []ma.Multiaddr{listenAddr} }

c := &mockConn{
local: listenAddr,
remote: ma.StringCast("/ip4/1.2.3.6/udp/1236/quic-v1/p2p-circuit"),
}
observedAddr := ma.StringCast("/ip4/1.2.3.4/udp/1231/quic-v1/p2p-circuit")
o, err := NewObservedAddrManager(listenAddrs, addrs, ifaceListenAddrs, normalize)
require.NoError(t, err)
shouldRecord, _, _ := o.shouldRecordObservation(c, observedAddr)
require.False(t, shouldRecord)
}

func TestShouldRecordObservationWithNAT64Addr(t *testing.T) {
listenAddr1 := ma.StringCast("/ip4/0.0.0.0/tcp/1234")
ifaceAddr1 := ma.StringCast("/ip4/10.0.0.2/tcp/4321")
Expand Down

0 comments on commit 7b95317

Please sign in to comment.