fix: signature compliant with spec

package.json

@@ -78,6 +78,7 @@
     "sanitize-filename": "^1.6.3",
     "streaming-iterables": "^4.1.0",
     "timeout-abort-controller": "^1.0.0",
+    "varint": "^5.0.0",
     "xsalsa20": "^1.0.2"
   },
   "devDependencies": {

src/errors.js

@@ -29,5 +29,6 @@ exports.codes = {
   ERR_TRANSPORT_UNAVAILABLE: 'ERR_TRANSPORT_UNAVAILABLE',
   ERR_TRANSPORT_DIAL_FAILED: 'ERR_TRANSPORT_DIAL_FAILED',
   ERR_UNSUPPORTED_PROTOCOL: 'ERR_UNSUPPORTED_PROTOCOL',
-  ERR_INVALID_MULTIADDR: 'ERR_INVALID_MULTIADDR'
+  ERR_INVALID_MULTIADDR: 'ERR_INVALID_MULTIADDR',
+  ERR_SIGNATURE_NOT_VALID: 'ERR_SIGNATURE_NOT_VALID'
 }

src/record/README.md

@@ -47,7 +47,7 @@ All libp2p nodes keep a `PeerStore`, that among other information stores a set o
 
 Libp2p peer records were created to enable the distribution of verifiable address records, which we can prove originated from the addressed peer itself. With such guarantees, libp2p can prioritize addresses based on their authenticity, with the most strict strategy being to only dial certified addresses.
 
-A peer record contains the peers' publicly reachable listen addresses, and may be extended in the future to contain additional metadata relevant to routing. It also contains a `seq` field, so that we can order peer records by time and identify if a received record is more recent than the stored one.
+A peer record contains the peers' publicly reachable listen addresses, and may be extended in the future to contain additional metadata relevant to routing. It also contains a `seqNumber` field, so that we can order peer records by time and identify if a received record is more recent than the stored one.
 
 You can read further about the Peer Record in [libp2p/specs#217](https://github.com/libp2p/specs/pull/217).
 
@@ -92,7 +92,7 @@ When a libp2p node changes its listen addresses, the identify service should be
 
 Considering that a node can discover other peers' addresses from a variety of sources, Libp2p Peerstore should be able to differentiate the addresses that were obtained through a signed peer record.
 
-Once a record is received and its signature properly validated, its envelope should be stored in the AddressBook on its byte representations. However, the `seq` number of the record must be compared with potentially stored records, so that we do not override correct data.
+Once a record is received and its signature properly validated, its envelope should be stored in the AddressBook on its byte representations. However, the `seqNumber` number of the record must be compared with potentially stored records, so that we do not override correct data.
 
 The AddressBook Addresses must be updated with the content of the envelope with a certified property that allows other subsystems to identify that the known certified addresses of a peer.
 
@@ -112,17 +112,3 @@ When a subsystem wants to provide a record, it should get it from the AddressBoo
   - Modular dialer? (taken from go PR notes)
     - With the modular dialer, users should easily be able to configure precedence. With dialer v1, anything we do to prioritise dials is gonna be spaghetti and adhoc. With the modular dialer, you’d be able to specify the order of dials when instantiating the pipeline.
     - Multiple parallel dials. We already have the issue where new addresses aren't added to existing dials.
-
-### Notes:
-
-- Possible design for AddressBook
-
-```
-addr_book_record
-  \_ peer_id: bytes
-  \_ signed_addrs: []AddrEntry
-  \_ unsigned_addrs: []AddrEntry
-  \_ certified_record
-      \_ seq: bytes
-      \_ raw: bytes
-```

src/record/envelope/index.js

@@ -5,14 +5,18 @@ const log = debug('libp2p:envelope')
 log.error = debug('libp2p:envelope:error')
 const errCode = require('err-code')
 
+const { Buffer } = require('buffer')
+
 const crypto = require('libp2p-crypto')
-const multicodec = require('multicodec')
 const PeerId = require('peer-id')
+const varint = require('varint')
 
+const { codes } = require('../../errors')
 const Protobuf = require('./envelope.proto')
 
 /**
- * The Envelope is responsible for keeping arbitrary signed by a libp2p peer.
+ * The Envelope is responsible for keeping an arbitrary signed record
+ * by a libp2p peer.
  */
 class Envelope {
   /**
@@ -41,7 +45,7 @@ class Envelope {
     if (this._marshal) {
       return this._marshal
     }
-    // TODO: type for marshal (default: RSA)
+
     const publicKey = crypto.keys.marshalPublicKey(this.peerId.pubKey)
 
     this._marshal = Protobuf.encode({
@@ -69,34 +73,43 @@ class Envelope {
   /**
    * Validate envelope data signature for the given domain.
    * @param {string} domain
-   * @return {Promise}
+   * @return {Promise<boolean>}
    */
-  async validate (domain) {
+  validate (domain) {
     const signData = createSignData(domain, this.payloadType, this.payload)
 
-    try {
-      await this.peerId.pubKey.verify(signData, this.signature)
-    } catch (_) {
-      log.error('record signature verification failed')
-      // TODO
-      throw errCode(new Error('record signature verification failed'), 'ERRORS.ERR_SIGNATURE_VERIFICATION')
-    }
+    return this.peerId.pubKey.verify(signData, this.signature)
   }
 }
 
 /**
  * Helper function that prepares a buffer to sign or verify a signature.
  * @param {string} domain
- * @param {number} payloadType
+ * @param {Buffer} payloadType
  * @param {Buffer} payload
  * @return {Buffer}
  */
 const createSignData = (domain, payloadType, payload) => {
-  // TODO: this should be compliant with the spec!
-  const domainBuffer = Buffer.from(domain)
-  const payloadTypeBuffer = Buffer.from(payloadType.toString())
-
-  return Buffer.concat([domainBuffer, payloadTypeBuffer, payload])
+  // When signing, a peer will prepare a buffer by concatenating the following:
+  // - The length of the domain separation string string in bytes
+  // - The domain separation string, encoded as UTF-8
+  // - The length of the payload_type field in bytes
+  // - The value of the payload_type field
+  // - The length of the payload field in bytes
+  // - The value of the payload field
+
+  const domainLength = varint.encode(Buffer.byteLength(domain))
+  const payloadTypeLength = varint.encode(payloadType.length)
+  const payloadLength = varint.encode(payload.length)
+
+  return Buffer.concat([
+    Buffer.from(domainLength),
+    Buffer.from(domain),
+    Buffer.from(payloadTypeLength),
+    payloadType,
+    Buffer.from(payloadLength),
+    payload
+  ])
 }
 
 /**
@@ -118,15 +131,15 @@ const unmarshalEnvelope = async (data) => {
 
 /**
 * Seal marshals the given Record, places the marshaled bytes inside an Envelope
-* and signs with the given private key.
+* and signs it with the given peerId's private key.
 * @async
 * @param {Record} record
 * @param {PeerId} peerId
 * @return {Envelope}
 */
 Envelope.seal = async (record, peerId) => {
   const domain = record.domain
-  const payloadType = Buffer.from(`${multicodec.print[record.codec]}${domain}`)
+  const payloadType = Buffer.from(record.codec)
   const payload = record.marshal()
 
   const signData = createSignData(domain, payloadType, payload)
@@ -149,7 +162,11 @@ Envelope.seal = async (record, peerId) => {
  */
 Envelope.openAndCertify = async (data, domain) => {
   const envelope = await unmarshalEnvelope(data)
-  await envelope.validate(domain)
+  const valid = await envelope.validate(domain)
+
+  if (!valid) {
+    throw errCode(new Error('envelope signature is not valid for the given domain'), codes.ERR_SIGNATURE_NOT_VALID)
+  }
 
   return envelope
 }

src/record/peer-record/consts.js

@@ -1,6 +1,5 @@
 'use strict'
 
-// const { Buffer } = require('buffer')
 const multicodec = require('multicodec')
 
 // The domain string used for peer records contained in a Envelope.
@@ -9,10 +8,4 @@ module.exports.ENVELOPE_DOMAIN_PEER_RECORD = 'libp2p-peer-record'
 // The type hint used to identify peer records in a Envelope.
 // Defined in https://github.com/multiformats/multicodec/blob/master/table.csv
 // with name "libp2p-peer-record"
-// TODO
-// const b = Buffer.aloc(2)
-// b.writeInt16BE(multicodec.LIBP2P_PEER_RECORD)
-// module.exports.ENVELOPE_PAYLOAD_TYPE_PEER_RECORD = b
-
-// const ENVELOPE_PAYLOAD_TYPE_PEER_RECORD = Buffer.aloc(2)
-module.exports.ENVELOPE_PAYLOAD_TYPE_PEER_RECORD = multicodec.LIBP2P_PEER_RECORD
+module.exports.ENVELOPE_PAYLOAD_TYPE_PEER_RECORD = multicodec.print[multicodec.LIBP2P_PEER_RECORD]

src/record/peer-record/index.js

@@ -24,7 +24,6 @@ class PeerRecord extends Record {
    * @param {number} [params.seqNumber] monotonically-increasing sequence counter that's used to order PeerRecords in time.
    */
   constructor ({ peerId, multiaddrs = [], seqNumber = Date.now() }) {
-    // TODO: verify domain/payload type
     super(ENVELOPE_DOMAIN_PEER_RECORD, ENVELOPE_PAYLOAD_TYPE_PEER_RECORD)
 
     this.peerId = peerId

test/record/envelope.spec.js

@@ -6,18 +6,18 @@ chai.use(require('dirty-chai'))
 chai.use(require('chai-bytes'))
 const { expect } = chai
 
-const multicodec = require('multicodec')
-
 const Envelope = require('../../src/record/envelope')
 const Record = require('libp2p-interfaces/src/record')
+const { codes: ErrorCodes } = require('../../src/errors')
 
 const peerUtils = require('../utils/creators/peer')
 
-const domain = '/test-domain'
+const domain = 'libp2p-testing'
+const codec = '/libp2p/testdata'
 
 class TestRecord extends Record {
   constructor (data) {
-    super(domain, multicodec.LIBP2P_PEER_RECORD)
+    super(domain, codec)
     this.data = data
   }
 
@@ -31,7 +31,7 @@ class TestRecord extends Record {
 }
 
 describe('Envelope', () => {
-  const payloadType = Buffer.from(`${multicodec.print[multicodec.LIBP2P_PEER_RECORD]}${domain}`)
+  const payloadType = Buffer.from(codec)
   let peerId
   let testRecord
 
@@ -78,11 +78,12 @@ describe('Envelope', () => {
     expect(isEqual).to.eql(true)
   })
 
-  it.skip('throw on open and verify when a different domain is used', async () => {
+  it('throw on open and verify when a different domain is used', async () => {
     const envelope = await Envelope.seal(testRecord, peerId)
     const rawEnvelope = envelope.marshal()
 
-    await expect(Envelope.openAndCertify(rawEnvelope, '/fake-domain'))
-      .to.eventually.rejected()
+    await expect(Envelope.openAndCertify(rawEnvelope, '/bad-domain'))
+      .to.eventually.be.rejected()
+      .and.to.have.property('code', ErrorCodes.ERR_SIGNATURE_NOT_VALID)
   })
 })

test/record/peer-record.spec.js

@@ -5,9 +5,10 @@ const chai = require('chai')
 chai.use(require('dirty-chai'))
 const { expect } = chai
 
+const tests = require('libp2p-interfaces/src/record/tests')
 const multiaddr = require('multiaddr')
 
-const tests = require('libp2p-interfaces/src/record/tests')
+const Envelope = require('../../src/record/envelope')
 const PeerRecord = require('../../src/record/peer-record')
 
 const peerUtils = require('../utils/creators/peer')
@@ -113,5 +114,28 @@ describe('PeerRecord', () => {
 })
 
 describe('PeerRecord inside Envelope', () => {
-  // TODO
+  let peerId
+  let peerRecord
+
+  before(async () => {
+    [peerId] = await peerUtils.createPeerId()
+    const multiaddrs = [
+      multiaddr('/ip4/127.0.0.1/tcp/2000')
+    ]
+    const seqNumber = Date.now()
+    peerRecord = new PeerRecord({ peerId, multiaddrs, seqNumber })
+  })
+
+  it('creates an envelope with the PeerRecord and can unmarshal it', async () => {
+    const e = await Envelope.seal(peerRecord, peerId)
+    const byteE = e.marshal()
+
+    const decodedE = await Envelope.openAndCertify(byteE, peerRecord.domain)
+    expect(decodedE).to.exist()
+
+    const decodedPeerRecord = PeerRecord.createFromProtobuf(decodedE.payload)
+
+    const isEqual = peerRecord.isEqual(decodedPeerRecord)
+    expect(isEqual).to.eql(true)
+  })
 })