Skip to content

Commit

Permalink
identify: don't save signed peer records (#2325)
Browse files Browse the repository at this point in the history 
For multiple reasons:
1. They were not consumed anywhere.
2. We need to filter the addresses to avoid polluting the address book with local addresses.
  • Loading branch information
@marten-seemann
marten-seemann authored Jun 3, 2023
1 parent 8864d1c commit 581dd29
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 67 deletions.
48 changes: 37 additions & 11 deletions p2p/protocol/identify/id.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package identify
import (
"bytes"
"context"
"errors"
"fmt"
"io"
"sort"
Expand Down Expand Up @@ -757,21 +758,18 @@ func (ids *idService) consumeMessage(mes *pb.Identify, c network.Conn, isPush bo
ids.Host.Peerstore().UpdateAddrs(context.TODO(), p, ttl, peerstore.TempAddrTTL)
}

// add signed addrs if we have them and the peerstore supports them
cab, ok := peerstore.GetCertifiedAddrBook(ids.Host.Peerstore())

if ok && signedPeerRecord != nil && signedPeerRecord.PublicKey != nil {
id, err := peer.IDFromPublicKey(signedPeerRecord.PublicKey)
var addrs []ma.Multiaddr
if signedPeerRecord != nil {
signedAddrs, err := ids.consumeSignedPeerRecord(c.RemotePeer(), signedPeerRecord)
if err != nil {
log.Debugf("failed to derive peer ID from peer record: %s", err)
} else if id != c.RemotePeer() {
log.Debugf("received signed peer record for unexpected peer ID. expected %s, got %s", c.RemotePeer(), id)
} else if _, err := cab.ConsumePeerRecord(context.TODO(), signedPeerRecord, ttl); err != nil {
log.Debugf("error adding signed addrs to peerstore: %v", err)
log.Debugf("failed to consume signed peer record: %s", err)
} else {
addrs = signedAddrs
}
} else {
ids.Host.Peerstore().AddAddrs(context.TODO(), p, filterAddrs(lmaddrs, c.RemoteMultiaddr()), ttl)
addrs = lmaddrs
}
ids.Host.Peerstore().AddAddrs(context.TODO(), p, filterAddrs(addrs, c.RemoteMultiaddr()), ttl)

// Finally, expire all temporary addrs.
ids.Host.Peerstore().UpdateAddrs(context.TODO(), p, peerstore.TempAddrTTL, 0)
Expand All @@ -790,6 +788,34 @@ func (ids *idService) consumeMessage(mes *pb.Identify, c network.Conn, isPush bo
ids.consumeReceivedPubKey(c, mes.PublicKey)
}

func (ids *idService) consumeSignedPeerRecord(p peer.ID, signedPeerRecord *record.Envelope) ([]ma.Multiaddr, error) {
if signedPeerRecord.PublicKey == nil {
return nil, errors.New("missing pubkey")
}
id, err := peer.IDFromPublicKey(signedPeerRecord.PublicKey)
if err != nil {
return nil, fmt.Errorf("failed to derive peer ID: %s", err)
}
if id != p {
return nil, fmt.Errorf("received signed peer record envelope for unexpected peer ID. expected %s, got %s", p, id)
}
r, err := signedPeerRecord.Record()
if err != nil {
return nil, fmt.Errorf("failed to obtain record: %w", err)
}
rec, ok := r.(*peer.PeerRecord)
if !ok {
return nil, errors.New("not a peer record")
}
if rec.PeerID != p {
return nil, fmt.Errorf("received signed peer record for unexpected peer ID. expected %s, got %s", p, rec.PeerID)
}
// Don't put the signed peer record into the peer store.
// They're not used anywhere.
// All we care about are the addresses.
return rec.Addrs, nil
}

func (ids *idService) consumeReceivedPubKey(c network.Conn, kb []byte) {
lp := c.LocalPeer()
rp := c.RemotePeer()
Expand Down
58 changes: 2 additions & 56 deletions p2p/protocol/identify/id_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,46 +29,17 @@ import (
"github.com/libp2p/go-libp2p/p2p/protocol/identify/pb"

mockClock "github.com/benbjohnson/clock"
logging "github.com/ipfs/go-log/v2"
"github.com/libp2p/go-msgio/pbio"
ma "github.com/multiformats/go-multiaddr"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func init() {
logging.SetLogLevel("net/identify", "debug")
}

func testKnowsAddrs(t *testing.T, h host.Host, p peer.ID, expected []ma.Multiaddr) {
t.Helper()
require.True(t, assert.ElementsMatchf(t, expected, h.Peerstore().Addrs(context.Background(), p), fmt.Sprintf("%s did not have addr for %s", h.ID(), p)))
}

func testHasCertifiedAddrs(t *testing.T, h host.Host, p peer.ID, expected []ma.Multiaddr) {
t.Helper()
cab, ok := peerstore.GetCertifiedAddrBook(h.Peerstore())
if !ok {
t.Error("expected peerstore to implement CertifiedAddrBook")
}
recordEnvelope := cab.GetPeerRecord(context.Background(), p)
if recordEnvelope == nil {
if len(expected) == 0 {
return
}
t.Fatalf("peerstore has no signed record for peer %s", p)
}
r, err := recordEnvelope.Record()
if err != nil {
t.Error("Error unwrapping signed PeerRecord from envelope", err)
}
rec, ok := r.(*peer.PeerRecord)
if !ok {
t.Error("unexpected record type")
}
require.True(t, assert.ElementsMatchf(t, expected, rec.Addrs, fmt.Sprintf("%s did not have certified addr for %s", h.ID(), p)))
}

func testHasAgentVersion(t *testing.T, h host.Host, p peer.ID) {
v, err := h.Peerstore().Get(context.Background(), p, "AgentVersion")
if v.(string) != "github.com/libp2p/go-libp2p" { // this is the default user agent
Expand All @@ -95,13 +66,6 @@ func testHasPublicKey(t *testing.T, h host.Host, p peer.ID, shouldBe ic.PubKey)
}
}

func getSignedRecord(t *testing.T, h host.Host, p peer.ID) *record.Envelope {
cab, ok := peerstore.GetCertifiedAddrBook(h.Peerstore())
require.True(t, ok)
rec := cab.GetPeerRecord(context.Background(), p)
return rec
}

// we're using BlankHost in our tests, which doesn't automatically generate peer records
// and emit address change events on the bus like BasicHost.
// This generates a record, puts it in the peerstore and emits an addr change event
Expand Down Expand Up @@ -191,8 +155,7 @@ func TestIDService(t *testing.T) {
// the idService should be opened automatically, by the network.
// what we should see now is that both peers know about each others listen addresses.
t.Log("test peer1 has peer2 addrs correctly")
testKnowsAddrs(t, h1, h2p, h2.Addrs()) // has them
testHasCertifiedAddrs(t, h1, h2p, h2.Peerstore().Addrs(context.Background(), h2p)) // should have signed addrs also
testKnowsAddrs(t, h1, h2p, h2.Addrs()) // has them
testHasAgentVersion(t, h1, h2p)
testHasPublicKey(t, h1, h2p, h2.Peerstore().PubKey(context.Background(), h2p)) // h1 should have h2's public key

Expand All @@ -205,7 +168,6 @@ func TestIDService(t *testing.T) {
// and the protocol versions.
t.Log("test peer2 has peer1 addrs correctly")
testKnowsAddrs(t, h2, h1p, h1.Addrs()) // has them
testHasCertifiedAddrs(t, h2, h1p, h1.Peerstore().Addrs(context.Background(), h1p))
testHasAgentVersion(t, h2, h1p)
testHasPublicKey(t, h2, h1p, h1.Peerstore().PubKey(context.Background(), h1p)) // h1 should have h2's public key

Expand All @@ -222,8 +184,6 @@ func TestIDService(t *testing.T) {
// addresses don't immediately expire on disconnect, so we should still have them
testKnowsAddrs(t, h2, h1p, h1.Addrs())
testKnowsAddrs(t, h1, h2p, h2.Addrs())
testHasCertifiedAddrs(t, h1, h2p, h2.Peerstore().Addrs(context.Background(), h2p))
testHasCertifiedAddrs(t, h2, h1p, h1.Peerstore().Addrs(context.Background(), h1p))

<-sentDisconnect1
<-sentDisconnect2
Expand All @@ -234,8 +194,6 @@ func TestIDService(t *testing.T) {
clk.Add(time.Second)
testKnowsAddrs(t, h1, h2p, []ma.Multiaddr{})
testKnowsAddrs(t, h2, h1p, []ma.Multiaddr{})
testHasCertifiedAddrs(t, h1, h2p, []ma.Multiaddr{})
testHasCertifiedAddrs(t, h2, h1p, []ma.Multiaddr{})

// test that we received the "identify completed" event.
select {
Expand Down Expand Up @@ -481,7 +439,6 @@ func TestIdentifyPushOnAddrChange(t *testing.T) {
waitForAddrInStream(t, h2AddrStream, lad, 10*time.Second, "h2 did not receive addr change")

require.True(t, ma.Contains(h2.Peerstore().Addrs(context.Background(), h1p), lad))
require.NotNil(t, getSignedRecord(t, h2, h1p))

// change addr on host2 and ensure host 1 gets a pus
lad = ma.StringCast("/ip4/127.0.0.1/tcp/1235")
Expand All @@ -494,7 +451,6 @@ func TestIdentifyPushOnAddrChange(t *testing.T) {
waitForAddrInStream(t, h1AddrStream, lad, 10*time.Second, "h1 did not receive addr change")

require.True(t, ma.Contains(h1.Peerstore().Addrs(context.Background(), h2p), lad))
require.NotNil(t, getSignedRecord(t, h1, h2p))

// change addr on host2 again
lad2 := ma.StringCast("/ip4/127.0.0.1/tcp/1236")
Expand All @@ -506,7 +462,6 @@ func TestIdentifyPushOnAddrChange(t *testing.T) {
waitForAddrInStream(t, h1AddrStream, lad2, 10*time.Second, "h1 did not receive addr change")

require.True(t, ma.Contains(h1.Peerstore().Addrs(context.Background(), h2p), lad2))
require.NotNil(t, getSignedRecord(t, h1, h2p))
}

func TestUserAgent(t *testing.T) {
Expand Down Expand Up @@ -660,8 +615,7 @@ func TestLargeIdentifyMessage(t *testing.T) {
// the idService should be opened automatically, by the network.
// what we should see now is that both peers know about each others listen addresses.
t.Log("test peer1 has peer2 addrs correctly")
testKnowsAddrs(t, h1, h2p, h2.Addrs()) // has them
testHasCertifiedAddrs(t, h1, h2p, h2.Peerstore().Addrs(context.Background(), h2p)) // should have signed addrs also
testKnowsAddrs(t, h1, h2p, h2.Addrs()) // has them
testHasAgentVersion(t, h1, h2p)
testHasPublicKey(t, h1, h2p, h2.Peerstore().PubKey(context.Background(), h2p)) // h1 should have h2's public key

Expand All @@ -676,7 +630,6 @@ func TestLargeIdentifyMessage(t *testing.T) {
// and the protocol versions.
t.Log("test peer2 has peer1 addrs correctly")
testKnowsAddrs(t, h2, h1p, h1.Addrs()) // has them
testHasCertifiedAddrs(t, h2, h1p, h1.Peerstore().Addrs(context.Background(), h1p))
testHasAgentVersion(t, h2, h1p)
testHasPublicKey(t, h2, h1p, h1.Peerstore().PubKey(context.Background(), h1p)) // h1 should have h2's public key

Expand All @@ -693,8 +646,6 @@ func TestLargeIdentifyMessage(t *testing.T) {
// addresses don't immediately expire on disconnect, so we should still have them
testKnowsAddrs(t, h2, h1p, h1.Addrs())
testKnowsAddrs(t, h1, h2p, h2.Addrs())
testHasCertifiedAddrs(t, h1, h2p, h2.Peerstore().Addrs(context.Background(), h2p))
testHasCertifiedAddrs(t, h2, h1p, h1.Peerstore().Addrs(context.Background(), h1p))

<-sentDisconnect1
<-sentDisconnect2
Expand All @@ -705,8 +656,6 @@ func TestLargeIdentifyMessage(t *testing.T) {
clk.Add(time.Second)
testKnowsAddrs(t, h1, h2p, []ma.Multiaddr{})
testKnowsAddrs(t, h2, h1p, []ma.Multiaddr{})
testHasCertifiedAddrs(t, h1, h2p, []ma.Multiaddr{})
testHasCertifiedAddrs(t, h2, h1p, []ma.Multiaddr{})

// test that we received the "identify completed" event.
select {
Expand Down Expand Up @@ -770,7 +719,6 @@ func TestLargePushMessage(t *testing.T) {
require.Eventually(t, func() bool {
return ma.Contains(h2.Peerstore().Addrs(context.Background(), h1p), lad)
}, time.Second, 10*time.Millisecond)
require.NotNil(t, getSignedRecord(t, h2, h1p))

// change addr on host2 and ensure host 1 gets a pus
lad = ma.StringCast("/ip4/127.0.0.1/tcp/1235")
Expand All @@ -781,7 +729,6 @@ func TestLargePushMessage(t *testing.T) {
require.Eventually(t, func() bool {
return ma.Contains(h1.Peerstore().Addrs(context.Background(), h2p), lad)
}, time.Second, 10*time.Millisecond)
testHasCertifiedAddrs(t, h1, h2p, h2.Addrs())

// change addr on host2 again
lad2 := ma.StringCast("/ip4/127.0.0.1/tcp/1236")
Expand All @@ -792,7 +739,6 @@ func TestLargePushMessage(t *testing.T) {
require.Eventually(t, func() bool {
return ma.Contains(h1.Peerstore().Addrs(context.Background(), h2p), lad2)
}, time.Second, 10*time.Millisecond)
testHasCertifiedAddrs(t, h2, h1p, h1.Addrs())
}

func TestIdentifyResponseReadTimeout(t *testing.T) {
Expand Down

0 comments on commit 581dd29

Please sign in to comment.