Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

signed peer records: make them actually usable #2304

Open
marten-seemann opened this issue May 20, 2023 · 3 comments
Open

signed peer records: make them actually usable #2304

marten-seemann opened this issue May 20, 2023 · 3 comments
Labels
kind/discussion Topical discussion; usually not changes to codebase

Comments

@marten-seemann
Copy link
Contributor

marten-seemann commented May 20, 2023

To be able to retrieve signed peer records, we need to store the serialized format, thanks to their non-deterministic serialization.
We also need to store the contents in the peer store, if we actually want to use the contents.

The current peer store API makes using signed peer records very cumbersome:

// CertifiedAddrBook manages "self-certified" addresses for remote peers.
// Self-certified addresses are contained in peer.PeerRecords
// which are wrapped in a record.Envelope and signed by the peer
// to whom they belong.
//
// Certified addresses (CA) are generally more secure than uncertified
// addresses (UA). Consequently, CAs beat and displace UAs. When the
// peerstore learns CAs for a peer, it will reject UAs for the same peer
// (as long as the former haven't expired).
// Furthermore, peer records act like sequenced snapshots of CAs. Therefore,
// processing a peer record that's newer than the last one seen overwrites
// all addresses with the incoming ones.
//
// This interface is most useful when combined with AddrBook.
// To test whether a given AddrBook / Peerstore implementation supports
// certified addresses, callers should use the GetCertifiedAddrBook helper or
// type-assert on the CertifiedAddrBook interface:
//
// if cab, ok := aPeerstore.(CertifiedAddrBook); ok {
// cab.ConsumePeerRecord(signedPeerRecord, aTTL)
// }
type CertifiedAddrBook interface {
// ConsumePeerRecord adds addresses from a signed peer.PeerRecord (contained in
// a record.Envelope), which will expire after the given TTL.
//
// The 'accepted' return value indicates that the record was successfully processed
// and integrated into the CertifiedAddrBook state. If 'accepted' is false but no
// error is returned, it means that the record was ignored, most likely because
// a newer record exists for the same peer.
//
// Signed records added via this method will be stored without
// alteration as long as the address TTLs remain valid. The Envelopes
// containing the PeerRecords can be retrieved by calling GetPeerRecord(peerID).
//
// If the signed PeerRecord belongs to a peer that already has certified
// addresses in the CertifiedAddrBook, the new addresses will replace the
// older ones, if the new record has a higher sequence number than the
// existing record. Attempting to add a peer record with a
// sequence number that's <= an existing record for the same peer will not
// result in an error, but the record will be ignored, and the 'accepted'
// bool return value will be false.
//
// If the CertifiedAddrBook is also an AddrBook (which is most likely the case),
// adding certified addresses for a peer will *replace* any
// existing non-certified addresses for that peer, and only the certified
// addresses will be returned from AddrBook.Addrs thereafter.
//
// Likewise, once certified addresses have been added for a given peer,
// any non-certified addresses added via AddrBook.AddAddrs or
// AddrBook.SetAddrs will be ignored. AddrBook.SetAddrs may still be used
// to update the TTL of certified addresses that have previously been
// added via ConsumePeerRecord.
ConsumePeerRecord(s *record.Envelope, ttl time.Duration) (accepted bool, err error)
// GetPeerRecord returns a Envelope containing a PeerRecord for the
// given peer id, if one exists.
// Returns nil if no signed PeerRecord exists for the peer.
GetPeerRecord(p peer.ID) *record.Envelope
}

Most importantly, it doesn't allow us to filter addresses, it's all or nothing. This is a problem for #2300, and will be for Kademlia, once it adds support for signed peer records (see libp2p/go-libp2p-kad-dht#839).
In identify, we currently accept any signed peer record the peer sends us, without even checking that it's the peer's signed peer record (any correctly signed record will be accepted).

What we should do instead is the following:

  1. Remove the CertifiedAddrBook interface.
  2. Use the regular AddAddrs API to add save the addresses contained in the signed peer record.
  3. Save the serialized bytes of the record in the metadata.
@marten-seemann
Copy link
Contributor Author

marten-seemann commented May 20, 2023

Metadata might not give us the properties we're looking for, we need to be able to atomically update the metadata iff we receive a new record with a higher sequence number.

This has be atomic since there will be multiple places from which such updates will happen (e.g. Identify, Kademlia, etc.).

Possible API:

type CertifiedAddrBook interface { 
     MaybeAddCertifiedRecord(p peer.ID, seq uint64, envelope []byte) (updated ok) // we only update when the sequence number is higher than what we're currently storing
     GetCertifiedRecord(p peer.ID) []byte // nil if we don't have one
}

@sukunrt
Copy link
Member

sukunrt commented May 20, 2023

Can we move the address handling logic out and keep the peer record api as

type CertifiedAddrBook interface { 
     MaybeAddCertifiedPeerRecord(envelope *record.Envelope) (updated ok) 
     GetCertifiedRecord(p peer.ID) *record.Envelope 
}

@p-shahi p-shahi added the kind/discussion Topical discussion; usually not changes to codebase label May 22, 2023
@p-shahi p-shahi closed this as completed May 30, 2023
@marten-seemann marten-seemann reopened this Jun 1, 2023
@marten-seemann
Copy link
Contributor Author

Related proposal to fully embrace signed peer records: libp2p/specs#552

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/discussion Topical discussion; usually not changes to codebase
Projects
None yet
Development

No branches or pull requests

3 participants