From 56ebb42d5fe8163a5cf0ff8c43b538b7a4d31c4a Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 5 Jun 2020 17:50:12 -0700 Subject: [PATCH] fix: always update address TTLs in a signed peer record Previously, we'd only set the TTL if the addresses were new. --- pstoreds/addr_book.go | 5 +++-- pstoremem/addr_book.go | 4 ++-- test/addr_book_suite.go | 49 ++++++++++++++++++++++++++--------------- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/pstoreds/addr_book.go b/pstoreds/addr_book.go index 7368718..51a1cf0 100644 --- a/pstoreds/addr_book.go +++ b/pstoreds/addr_book.go @@ -278,8 +278,9 @@ func (ab *dsAddrBook) ConsumePeerRecord(recordEnvelope *record.Envelope, ttl tim return false, fmt.Errorf("signing key does not match PeerID in PeerRecord") } - // ensure that the seq number from envelope is > any previously received seq no - if ab.latestPeerRecordSeq(rec.PeerID) >= rec.Seq { + // ensure that the seq number from envelope is >= any previously received seq no + // update when equal to extend the ttls + if ab.latestPeerRecordSeq(rec.PeerID) > rec.Seq { return false, nil } diff --git a/pstoremem/addr_book.go b/pstoremem/addr_book.go index 48f7f44..72f4e2f 100644 --- a/pstoremem/addr_book.go +++ b/pstoremem/addr_book.go @@ -178,11 +178,11 @@ func (mab *memoryAddrBook) ConsumePeerRecord(recordEnvelope *record.Envelope, tt return false, fmt.Errorf("signing key does not match PeerID in PeerRecord") } - // ensure seq is greater than last received + // ensure seq is greater than, or equal to, the last received s := mab.segments.get(rec.PeerID) s.Lock() lastState, found := s.signedPeerRecords[rec.PeerID] - if found && lastState.Seq >= rec.Seq { + if found && lastState.Seq > rec.Seq { s.Unlock() return false, nil } diff --git a/test/addr_book_suite.go b/test/addr_book_suite.go index f05a418..6bc9dd8 100644 --- a/test/addr_book_suite.go +++ b/test/addr_book_suite.go @@ -375,10 +375,18 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) { allAddrs := GenerateAddrs(10) certifiedAddrs := allAddrs[:5] uncertifiedAddrs := allAddrs[5:] - rec := peer.NewPeerRecord() - rec.PeerID = id - rec.Addrs = certifiedAddrs - signedRec, err := record.Seal(rec, priv) + rec1 := peer.NewPeerRecord() + rec1.PeerID = id + rec1.Addrs = certifiedAddrs + signedRec1, err := record.Seal(rec1, priv) + if err != nil { + t.Errorf("error creating signed routing record: %v", err) + } + + rec2 := peer.NewPeerRecord() + rec2.PeerID = id + rec2.Addrs = certifiedAddrs + signedRec2, err := record.Seal(rec2, priv) if err != nil { t.Errorf("error creating signed routing record: %v", err) } @@ -390,10 +398,13 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) { AssertAddressesEqual(t, uncertifiedAddrs, m.Addrs(id)) // add the signed record to addr book - _, err = cab.ConsumePeerRecord(signedRec, time.Hour) + accepted, err := cab.ConsumePeerRecord(signedRec2, time.Hour) if err != nil { t.Errorf("error adding signed routing record to addrbook: %v", err) } + if !accepted { + t.Errorf("should have accepted signed peer record") + } // the non-certified addrs should be gone & we should get only certified addrs back from Addrs // AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id)) @@ -404,36 +415,37 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) { t.Errorf("expected PeersWithAddrs to return 1, got %d", len(m.PeersWithAddrs())) } - // adding the same peer record again should result in the record being ignored - accepted, err := cab.ConsumePeerRecord(signedRec, time.Hour) + // Adding an old record should fail + accepted, err = cab.ConsumePeerRecord(signedRec1, time.Hour) if accepted { - t.Error("Expected record with duplicate sequence number to be ignored") + t.Error("We should have failed to accept a record with an old sequence number") } if err != nil { - t.Errorf("Expected record with duplicate sequence number to be ignored without error, got err: %s", err) + t.Errorf("expected no error, got: %s", err) } // once certified addrs exist, trying to add non-certified addrs should have no effect // m.AddAddrs(id, uncertifiedAddrs, time.Hour) // AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id)) + // XXX: Disabled until signed records are required m.AddAddrs(id, uncertifiedAddrs, time.Hour) AssertAddressesEqual(t, allAddrs, m.Addrs(id)) // we should be able to retrieve the signed peer record - rec2 := cab.GetPeerRecord(id) - if rec2 == nil || !signedRec.Equal(rec2) { + rec3 := cab.GetPeerRecord(id) + if rec3 == nil || !signedRec2.Equal(rec3) { t.Error("unable to retrieve signed routing record from addrbook") } // Adding a new envelope should clear existing certified addresses. // Only the newly-added ones should remain certifiedAddrs = certifiedAddrs[:3] - rec = peer.NewPeerRecord() - rec.PeerID = id - rec.Addrs = certifiedAddrs - signedRec, err = record.Seal(rec, priv) + rec4 := peer.NewPeerRecord() + rec4.PeerID = id + rec4.Addrs = certifiedAddrs + signedRec4, err := record.Seal(rec4, priv) test.AssertNilError(t, err) - _, err = cab.ConsumePeerRecord(signedRec, time.Hour) + _, err = cab.ConsumePeerRecord(signedRec4, time.Hour) test.AssertNilError(t, err) // AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id)) AssertAddressesEqual(t, allAddrs, m.Addrs(id)) @@ -441,6 +453,7 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) { // update TTL on signed addrs to -1 to remove them. // the signed routing record should be deleted // m.SetAddrs(id, certifiedAddrs, -1) + // XXX: Disabled until signed records are required m.SetAddrs(id, allAddrs, -1) if len(m.Addrs(id)) != 0 { t.Error("expected zero certified addrs after setting TTL to -1") @@ -450,7 +463,7 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) { } // Test that natural TTL expiration clears signed peer records - _, err = cab.ConsumePeerRecord(signedRec, time.Second) + _, err = cab.ConsumePeerRecord(signedRec4, time.Second) test.AssertNilError(t, err) AssertAddressesEqual(t, certifiedAddrs, m.Addrs(id)) @@ -462,7 +475,7 @@ func testCertifiedAddresses(m pstore.AddrBook) func(*testing.T) { // adding a peer record that's signed with the wrong key should fail priv2, _, err := test.RandTestKeyPair(crypto.Ed25519, 256) test.AssertNilError(t, err) - env, err := record.Seal(rec, priv2) + env, err := record.Seal(rec4, priv2) test.AssertNilError(t, err) accepted, err = cab.ConsumePeerRecord(env, time.Second)