Throw error when reusing keyPair for listens (#208)

* Throw error when reusing keyPair for listens

* Do not delete server from listening if it throws KEYPAIR_ALREADY_USED

* Do not remove server from listening set if announcer.start() throws

* Always clean up if _listen throws before starting the announcer

Author: HDegroote

lib/errors.js

@@ -64,6 +64,10 @@ module.exports = class DHTError extends Error {
     return new DHTError(msg, 'ALREADY_LISTENING', DHTError.ALREADY_LISTENING)
   }
 
+  static KEYPAIR_ALREADY_USED (msg = 'Keypair already used') {
+    return new DHTError(msg, 'KEYPAIR_ALREADY_USED', DHTError.KEYPAIR_ALREADY_USED)
+  }
+
   static NODE_DESTROYED (msg = 'Node destroyed') {
     return new DHTError(msg, 'NODE_DESTROYED', DHTError.NODE_DESTROYED)
   }

lib/server.js

@@ -10,7 +10,7 @@ const { unslabbedHash } = require('./crypto')
 const SecurePayload = require('./secure-payload')
 const Holepuncher = require('./holepuncher')
 const { isPrivate } = require('bogon')
-const { ALREADY_LISTENING, NODE_DESTROYED } = require('./errors')
+const { ALREADY_LISTENING, NODE_DESTROYED, KEYPAIR_ALREADY_USED } = require('./errors')
 
 const HANDSHAKE_CLEAR_WAIT = 10000
 const HANDSHAKE_INITIAL_TIMEOUT = 10000
@@ -150,25 +150,30 @@ module.exports = class Server extends EventEmitter {
     // From now on, the DHT object which created me is responsible for closing me
     this.dht.listening.add(this)
 
-    await this.dht.bind()
-    if (this._closing) return
+    try {
+      await this.dht.bind()
+      if (this._closing) return
 
-    this.target = unslabbedHash(keyPair.publicKey)
+      for (const s of this.dht.listening) {
+        if (s._keyPair && b4a.equals(s._keyPair.publicKey, keyPair.publicKey)) {
+          throw KEYPAIR_ALREADY_USED()
+        }
+      }
 
-    this._keyPair = keyPair
-    this._announcer = new Announcer(this.dht, keyPair, this.target, opts)
+      this.target = unslabbedHash(keyPair.publicKey)
+      this._keyPair = keyPair
+      this._announcer = new Announcer(this.dht, keyPair, this.target, opts)
 
-    this.dht._router.set(this.target, {
-      relay: null,
-      record: this._announcer.record,
-      onpeerhandshake: this._onpeerhandshake.bind(this),
-      onpeerholepunch: this._onpeerholepunch.bind(this)
-    })
+      this.dht._router.set(this.target, {
+        relay: null,
+        record: this._announcer.record,
+        onpeerhandshake: this._onpeerhandshake.bind(this),
+        onpeerholepunch: this._onpeerholepunch.bind(this)
+      })
 
-    // warm it up for now
-    this._localAddresses().catch(safetyCatch)
+      // warm it up for now
+      this._localAddresses().catch(safetyCatch)
 
-    try {
       await this._announcer.start()
     } catch (err) {
       await this._stopListening()

test/lifecycle.js

@@ -1,6 +1,7 @@
 const test = require('brittle')
-const { swarm } = require('./helpers')
 const safetyCatch = require('safety-catch')
+const hypCrypto = require('hypercore-crypto')
+const { swarm } = require('./helpers')
 
 test('Can destroy a DHT node while server.listen() is called', async function (t) {
   const [a] = await swarm(t)
@@ -16,3 +17,29 @@ test('Can destroy a DHT node while server.listen() is called', async function (t
   await listenProm
   t.pass('The listen function does not error when the DHT closes while it is running')
 })
+
+test('Cannot listen on multiple servers with the same keypair', async function (t) {
+  const [a] = await swarm(t)
+
+  const s1 = a.createServer()
+  const s2 = a.createServer()
+
+  const s3 = a.createServer()
+  const s4 = a.createServer()
+  const s5 = a.createServer()
+
+  await s1.listen()
+  await t.exception(
+    async () => await s2.listen(),
+    /KEYPAIR_ALREADY_USED/
+  )
+
+  const keyPair = hypCrypto.keyPair()
+
+  await s3.listen(keyPair)
+  await t.exception(
+    async () => await s4.listen(keyPair),
+    /KEYPAIR_ALREADY_USED/
+  )
+  await s5.listen(hypCrypto.keyPair())
+})