feat: signed peer records data types #681

vasco-santos · 2020-06-21T19:05:16Z

This PR adds the Libp2p Envelope and Peer Record data type for #653 .

Needs:

Solution design

A Record interface was created libp2p/js-libp2p-interfaces#52 was the base for any Record compatible with a Libp2p Envelope. A record implementation should run the interface tests to guarantee its compatibility with the Envelope.

A Libp2p Envelope will be a container for a record implementing the Record interface. It will include the record marshalled data as a payload property, as well as a signature and a payloadType for easing the decoding in the future, when we have multiple records differently serialised.

A PeerRecord was created implementing the Record interface. This record will be using for many libp2p protocols that exchange peers' listen multiaddrs, such as Identify, Gossipsub1.1, DHT, Rendezvous, etc. The protocols should support the legacy protocol for the time being.

What is missing?

The envelope signature generate still is being used as a placeholder and not compliant with the spec. This should be solved for the creation of the module. There is no js implementation for the uvarint spec and I will work on it (there is one that I need to cross check with the spec).

What's next?

I expect to create the follow up PRs

A libp2p node should exchange a signed peer record in the Identify protocol (identify + identify-push), so that we find needed changes for the RecordManager/Envelope implementations.
Create certified AddressBook / Adapt AddressBook to store signed peer records data. Update API.md
Interop tests for the identify protocol using the new protocol.
Integrate signed Peer Records in Pubsub
Integrate signed Peer Records in the DHT
Integrate signed Peer Records in the Rendezvous

References

vasco-santos · 2020-06-21T19:07:40Z

@jacobheun can I have an initial feedback on this implementation design?

Once we are aligned, I plan to:

Create peer-record module and move the code there
Create libp2p-envelope module, move the code there and make the signature compliant with the spec
Get this PR using the modules and ready to be merged

Follow up draft PRs for refactoring the Identify Protocol and CertifiedAddressBook for initial feedback should land during the next couple of days

EDIT: #682 was created so that you can see how the records / envelopes will be created and consumed in identify. Go to the second commit of the PR, as the first one is this PR

jacobheun

This PR is for the support of Peer Routing Records. With that in mind I'd like to clarify the flow, the API specifically, of how these records are created and fetched, as this isn't very clear.

Right now it looks like the Record Manager is intended to be run like other services, but I don't think that's necessary. While addresses are not currently dynamic, I think we should make it so this system allows for easily invalidating the cached envelope, ideally we should update the cache on change, instead of doing lazy loading as this can adversely affect response times. It would be helpful to document the API for some of these known use cases to provide clarity to the design here.

When we get a new address, how do we create a new routing record for self? (Assume we have AutoRelay/AutoNAT in place and we are able to determine addresses dynamically, how does this get updated from those subystems?)
How do subsystems (identify, dht, pubsub) get our current routing record?
How do subsystems store routing records for other peers?
How do subsystems get routing records for other peers?

src/record-manager/README.md

jacobheun · 2020-06-22T14:45:33Z

src/record-manager/envelope/envelope.proto.js

+  // signature is the signature produced by the private key corresponding to
+  // the enclosed public key, over the payload, prefixing a domain string for
+  // additional security.
+  bytes signature = 5;


This matches Go but not the spec, should be 4 https://github.com/libp2p/specs/pull/217/files#diff-1d3e44bb1e34e0581019f35b96151a0dR70.

Created an issue in Go libp2p/go-libp2p-core#160

Should we already change it, or wait for go?

jacobheun · 2020-06-22T14:46:52Z

src/record-manager/envelope/envelope.proto.js

+const protons = require('protons')
+
+const message = `
+message Envelope {


Go calls this an Envelope, but in the spec it's SignedEnvelope.

Maybe also report in the spec? I think the SignedEnvelope naming is redundant, as the Envelope purpose is to have the signature

jacobheun · 2020-06-22T14:50:02Z

src/record-manager/envelope/index.js

+* @param {PeerId} peerId
+* @return {Envelope}
+*/
+exports.seal = async (record, peerId) => {


Extend Envelope instead of exports. I recall issues in the past with exporting more than 1 thing.

We are currently doing the same in the peer-id. Anyway, I like that we move this like you suggest and also in the Record, as in the record we could also add it to the interface validations (createFromProtobuf)

vasco-santos · 2020-06-23T08:20:24Z

This PR is for the support of Peer Routing Records. With that in mind I'd like to clarify the flow, the API specifically, of how these records are created and fetched, as this isn't very clear.

Yes, this is an important thing to discuss. I created #682 so that you could see it in practise.
I started by creating the Envelope and the PeerRecord inside js-libp2p to avoid the mountain of extra modules we have. But then, I gave more thought to it and I did not like how other subsystems would need to use this we keep this in here.

While the identify protocol it is straightforward, as it also lives in this repo, other modules like gossipsub/dht would need to require the modules as follows:

const Envelope = require('libp2p/src/...')
const PeerRecord = require('libp2p/src/...')

or libp2p can provide some API methods to access them without going this way.
The other solution would be to have them published as their own modules.

For now, the open PRs are going on the first solution, but I personally don't like it.

Right now it looks like the Record Manager is intended to be run like other services, but I don't think that's necessary. While addresses are not currently dynamic, I think we should make it so this system allows for easily invalidating the cached envelope, ideally we should update the cache on change, instead of doing lazy loading as this can adversely affect response times.

There is lazy loading for the records and envelope so that when users do in different occasions marshal on the same instace, it is not computed again. But in the context of self, the idea it to be cached from libp2p.start, after the transportManager finishes. Then, we just need to update it when the listen addresses change

Regarding its usability, I expect the following:

Create self peer record

Once the listen addresses are known, it should create a PeerRecord instance, and use Envelope.seal() to create an envelope wrapping the record. This enveloped should be cached to be sent anytime, and should be updated when necessary. My idea is to have Record Manager coordinating it by listen Address Manager events.

Send own peer record

Get a cached signed peer record though libp2p.x() and send it through the wire according to the protocol being used.

Receive peer record

A protocol receives the envelope from the wire protocol and it should use Envelope.openAndCertify() to open the envelope and verify the signatures. If the envelope is valid, the envelope should be added to a Certified AddressBook (still looking on this part).

Send other party peer record

Get it from the AddressBook

vasco-santos · 2020-06-24T08:53:39Z

Me and @jacobheun synced yesterday and we will do as follows:

Envelope and Libp2p Record will live inside the js-libp2p repo
Identify will be responsible for creating the self record
- Self record stored in the PeerStore AddressBook
- Identify gets it from the PeerStore (if it exists, otherwise identify creates it and stores it in the PeerStore)
- Identify verifies if it is updated (do we really need this? let's discuss further in the identify PR)
- Identify sends the self peer record
- FUTURE: Identify listens for address changes and creates the new PeerRecord on identify.push / delta

--

Since we do not need the RecordManager anymore, at least until having more records in libp2p, the scope of this PR will change to have the implementations of the Envelope and Peer Record. #682 will create self and exchange it in the Identify Protocol. Follow up PR will handle the peer records storage in the PeerStore.

vasco-santos · 2020-06-26T15:42:52Z

src/record/README.md

+
+### Usage
+
+- create an envelope with an instance of an `interface-record` implementation and prepare it for being exchanged:


TODO: link interface-record once merged

vasco-santos · 2020-06-26T15:54:45Z

src/record-manager/envelope/envelope.proto.js

+  // signature is the signature produced by the private key corresponding to
+  // the enclosed public key, over the payload, prefixing a domain string for
+  // additional security.
+  bytes signature = 5;


Should we already change it, or wait for go?

jacobheun

Overall this looks good, just some minor changes

jacobheun · 2020-07-14T20:06:47Z

doc/API.md

 | [options.peerId] | [`PeerId`][peer-id] | peerId instance (it will be created if not provided) |
-| [options.peerStore] | [`object`](./CONFIGURATION.md#configuring-peerstore) | libp2p PeerStore configuration |
+| [options.peerStore] | [`object`](./CONFIGURATION.md#configuring-peerstore) | libp2p PeerStore [configuration]((./CONFIGURATION.md#configuring-peerstore)) |


Several of these added links are broken due to double ()'s.

jacobheun · 2020-07-14T20:07:38Z

doc/CONFIGURATION.md

@@ -52,7 +52,7 @@ The libp2p ecosystem contains at least one module for each of these subsystems.

 After selecting the modules to use, it is also possible to configure each one according to your needs.

-Bear in mind that only a **transport** and **connection encryption** are required, while all the other subsystems are optional.
+Bear in mind that a **transport** and **connection encryption** are **required**, while all the other subsystems are optional.


Suggested change

Bear in mind that a **transport** and **connection encryption** are **required**, while all the other subsystems are optional.

Bear in mind that a **transport** and **connection encryption** module are **required**, while all the other subsystems are optional.

jacobheun · 2020-07-14T20:17:26Z

src/record/README.md

+```js
+const Envelope = require('libp2p/src/record/envelop')
+
+// ... create a record named rec with domain X


This description isn't clear to me. I don't see domain X listed in this snippet.

jacobheun · 2020-07-14T20:18:11Z

src/record/README.md

+
+```js
+const Envelope = require('libp2p/src/record/envelop')
+// const Record = ...


jacobheun · 2020-07-14T20:19:38Z

src/record/README.md

+const Envelope = require('libp2p/src/record/envelop')
+// const Record = ...
+
+// ... receive envelope data


Clarify this. Maybe create a utility function that takes an envelope and a domain and returns a record?

jacobheun · 2020-07-14T21:02:32Z