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.

Sign up for GitHub

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

Jump to bottom

fix: dont allow multiaddr dials without a peer id #558

Merged
merged 3 commits into from
Feb 11, 2020
Merged

fix: dont allow multiaddr dials without a peer id #558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54516ed
fix: require peer ids when dialing multiaddrs
jacobheun Feb 11, 2020
ae8ee10
chore: fix lint
jacobheun Feb 11, 2020
58265e3
docs: add more info about multiaddr peer ids
jacobheun Feb 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions doc/API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,13 @@ for (const [peerId, connections] of libp2p.connections) {

### dial

Dials to another peer in the network and establishes the connection.

`dial(peer, options)`

#### Parameters

| Name | Type | Description |
|------|------|-------------|
| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | peer to dial |
| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | The peer to dial. If a [`Multiaddr`][multiaddr] or its string is provided, it **must** include the peer id |
| [options] | `Object` | dial options |
| [options.signal] | [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal) | An `AbortSignal` instance obtained from an [`AbortController`](https://developer.mozilla.org/en-US/docs/Web/API/AbortController) that can be used to abort the connection before it completes |

Expand Down Expand Up @@ -217,7 +215,7 @@ Dials to another peer in the network and selects a protocol to communicate with

| Name | Type | Description |
|------|------|-------------|
| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | peer to dial |
| peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | The peer to dial. If a [`Multiaddr`][multiaddr] or its string is provided, it **must** include the peer id |
| protocols | `String|Array<String>` | A list of protocols (or single protocol) to negotiate with. Protocols are attempted in order until a match is made. (e.g '/ipfs/bitswap/1.1.0') |
| [options] | `Object` | dial options |
| [options.signal] | [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal) | An `AbortSignal` instance obtained from an [`AbortController`](https://developer.mozilla.org/en-US/docs/Web/API/AbortController) that can be used to abort the connection before it completes |
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@
"cids": "^0.7.1",
"delay": "^4.3.0",
"dirty-chai": "^2.0.1",
"interop-libp2p": "~0.0.1",
"it-concat": "^1.0.0",
"it-pair": "^1.0.0",
"it-pushable": "^1.4.0",
"interop-libp2p": "~0.0.1",
"libp2p-bootstrap": "^0.10.3",
"libp2p-delegated-content-routing": "^0.4.1",
"libp2p-delegated-peer-routing": "^0.4.0",
Expand Down
7 changes: 3 additions & 4 deletions src/dialer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class Dialer {
async connectToPeer (peer, options = {}) {
const dialTarget = this._createDialTarget(peer)
if (dialTarget.addrs.length === 0) {
throw errCode(new Error('The dial request has no addresses'), 'ERR_NO_DIAL_MULTIADDRS')
throw errCode(new Error('The dial request has no addresses'), codes.ERR_NO_VALID_ADDRESSES)
}
const pendingDial = this._pendingDials.get(dialTarget.id) || this._createPendingDial(dialTarget, options)

Expand Down Expand Up @@ -136,7 +136,7 @@ class Dialer {
*/
_createPendingDial (dialTarget, options) {
const dialAction = (addr, options) => {
if (options.signal.aborted) throw errCode(new Error('already aborted'), 'ERR_ALREADY_ABORTED')
if (options.signal.aborted) throw errCode(new Error('already aborted'), codes.ERR_ALREADY_ABORTED)
return this.transportManager.dial(addr, options)
}

Expand Down Expand Up @@ -197,8 +197,7 @@ class Dialer {
try {
peer = PeerId.createFromCID(peer.getPeerId())
} catch (err) {
// Couldn't get the PeerId, just use the address
return peer
throw errCode(new Error('The multiaddr did not contain a valid peer id'), codes.ERR_INVALID_PEER)
}
}

Expand Down
1 change: 1 addition & 0 deletions src/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ exports.codes = {
ERR_CONNECTION_ENDED: 'ERR_CONNECTION_ENDED',
ERR_CONNECTION_FAILED: 'ERR_CONNECTION_FAILED',
ERR_NODE_NOT_STARTED: 'ERR_NODE_NOT_STARTED',
ERR_ALREADY_ABORTED: 'ERR_ALREADY_ABORTED',
ERR_NO_VALID_ADDRESSES: 'ERR_NO_VALID_ADDRESSES',
ERR_DISCOVERED_SELF: 'ERR_DISCOVERED_SELF',
ERR_DUPLICATE_TRANSPORT: 'ERR_DUPLICATE_TRANSPORT',
Expand Down
8 changes: 6 additions & 2 deletions src/peer-store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,12 +224,16 @@ class PeerStore extends EventEmitter {
}

/**
* Returns the known multiaddrs for a given `PeerInfo`
* Returns the known multiaddrs for a given `PeerInfo`. All returned multiaddrs
* will include the encapsulated `PeerId` of the peer.
* @param {PeerInfo} peer
* @returns {Array<Multiaddr>}
*/
multiaddrsForPeer (peer) {
return this.put(peer, true).multiaddrs.toArray()
return this.put(peer, true).multiaddrs.toArray().map(addr => {
if (addr.getPeerId()) return addr
return addr.encapsulate(`/p2p/${peer.id.toB58String()}`)
})
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/content-routing/dht/operation.node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('DHT subsystem operates correctly', () => {
remoteLibp2p.start()
])

remAddr = remoteLibp2p.transportManager.getAddrs()[0]
remAddr = libp2p.peerStore.multiaddrsForPeer(remotePeerInfo)[0]
})

afterEach(() => Promise.all([
Expand Down Expand Up @@ -98,7 +98,7 @@ describe('DHT subsystem operates correctly', () => {
await libp2p.start()
await remoteLibp2p.start()

remAddr = remoteLibp2p.transportManager.getAddrs()[0]
remAddr = libp2p.peerStore.multiaddrsForPeer(remotePeerInfo)[0]
})

afterEach(() => Promise.all([
Expand Down
76 changes: 37 additions & 39 deletions test/dialing/direct.node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,25 @@ const Peers = require('../fixtures/peers')
const { createPeerInfo } = require('../utils/creators/peer')

const listenAddr = multiaddr('/ip4/127.0.0.1/tcp/0')
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws')
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws/p2p/QmckxVrJw1Yo8LqvmDJNUmdAsKtSbiKWmrXJFyKmUraBoN')

describe('Dialing (direct, TCP)', () => {
let remoteTM
let localTM
let peerStore
let remoteAddr

before(async () => {
const [remotePeerId] = await Promise.all([
PeerId.createFromJSON(Peers[0])
])
remoteTM = new TransportManager({
libp2p: {},
upgrader: mockUpgrader
})
remoteTM.add(Transport.prototype[Symbol.toStringTag], Transport)

peerStore = new PeerStore()
localTM = new TransportManager({
libp2p: {},
upgrader: mockUpgrader
Expand All @@ -56,7 +61,7 @@ describe('Dialing (direct, TCP)', () => {

await remoteTM.listen([listenAddr])

remoteAddr = remoteTM.getAddrs()[0]
remoteAddr = remoteTM.getAddrs()[0].encapsulate(`/p2p/${remotePeerId.toB58String()}`)
})

after(() => remoteTM.close())
Expand All @@ -66,15 +71,15 @@ describe('Dialing (direct, TCP)', () => {
})

it('should be able to connect to a remote node via its multiaddr', async () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })

const connection = await dialer.connectToPeer(remoteAddr)
expect(connection).to.exist()
await connection.close()
})

it('should be able to connect to a remote node via its stringified multiaddr', async () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })

const dialable = Dialer.getDialable(remoteAddr.toString())
const connection = await dialer.connectToPeer(dialable)
Expand All @@ -83,7 +88,7 @@ describe('Dialing (direct, TCP)', () => {
})

it('should fail to connect to an unsupported multiaddr', async () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })

await expect(dialer.connectToPeer(unsupportedAddr))
.to.eventually.be.rejectedWith(AggregateError)
Expand Down Expand Up @@ -140,6 +145,7 @@ describe('Dialing (direct, TCP)', () => {
it('should abort dials on queue task timeout', async () => {
const dialer = new Dialer({
transportManager: localTM,
peerStore,
timeout: 50
})
sinon.stub(localTM, 'dial').callsFake(async (addr, options) => {
Expand Down Expand Up @@ -224,7 +230,7 @@ describe('Dialing (direct, TCP)', () => {
remoteLibp2p.handle('/echo/1.0.0', ({ stream }) => pipe(stream, stream))

await remoteLibp2p.start()
remoteAddr = remoteLibp2p.transportManager.getAddrs()[0]
remoteAddr = remoteLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${remotePeerId.toB58String()}`)
})

afterEach(async () => {
Expand All @@ -235,6 +241,28 @@ describe('Dialing (direct, TCP)', () => {

after(() => remoteLibp2p.stop())

it('should fail if no peer id is provided', async () => {
libp2p = new Libp2p({
peerInfo,
modules: {
transport: [Transport],
streamMuxer: [Muxer],
connEncryption: [Crypto]
}
})

sinon.spy(libp2p.dialer, 'connectToPeer')

try {
await libp2p.dial(remoteLibp2p.transportManager.getAddrs()[0])
} catch (err) {
expect(err).to.have.property('code', ErrorCodes.ERR_INVALID_PEER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we divide this error in 2 different ones?

Throwing to the user an invalid peer error, when the peer id was not provided might lead to confusion. I would separate between an invalid peerID and a not provided peerID error. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message that gets returned with the error is The multiaddr did not contain a valid peer id. It doesn't necessarily mean that no peer id was provided, it could also mean that the peer id provided isn't supported. Are you suggesting that if no peer id is provided we return one error, and if a peer id is provided but it's a bad peer id we return a different error? If so, I think it might be excessive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting that if no peer id is provided we return one error, and if a peer id is provided but it's a bad peer id we return a different error? If so, I think it might be excessive.

Yes, that was my suggestion. I am just concerned that a new user (who is also does not have knowledge on multiaddrs) might not understand immediately that the error originated from a multiaddr with no peer-id. However, from the error msg the user can understand that, so yeah I agree that this might be excessive.

Other suggestion. Should we add this requirements In the api docs? We just say that libp2p.dial can receive the following types of parameter: PeerInfo|PeerId|Multiaddr|string. Being a multiaddr valid without a peer id, but then throwing invalid peer id when no peer id is provided seems strange. Meanwhile, I will approve this PR, as this is a small detail

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the description in the api for dial and dialProtocol to read:
"The peer to dial. If a [Multiaddr][multiaddr] or its string is provided, it must include the peer id"

return
}

expect.fail('dial should have failed')
})

it('should use the dialer for connecting to a multiaddr', async () => {
libp2p = new Libp2p({
peerInfo,
Expand Down Expand Up @@ -267,10 +295,8 @@ describe('Dialing (direct, TCP)', () => {
})

sinon.spy(libp2p.dialer, 'connectToPeer')
const remotePeer = new PeerInfo(remoteLibp2p.peerInfo.id)
remotePeer.multiaddrs.add(remoteAddr)

const connection = await libp2p.dial(remotePeer)
const connection = await libp2p.dial(remotePeerInfo)
expect(connection).to.exist()
const { stream, protocol } = await connection.newStream('/echo/1.0.0')
expect(stream).to.exist()
Expand Down Expand Up @@ -306,7 +332,7 @@ describe('Dialing (direct, TCP)', () => {
}
})

const connection = await libp2p.dial(`${remoteAddr.toString()}/p2p/${remotePeerInfo.id.toB58String()}`)
const connection = await libp2p.dial(`${remoteAddr.toString()}`)
expect(connection).to.exist()
expect(connection.stat.timeline.close).to.not.exist()
await libp2p.hangUp(connection.remotePeer)
Expand Down Expand Up @@ -337,33 +363,6 @@ describe('Dialing (direct, TCP)', () => {
expect(libp2p.upgrader.protector.protect.callCount).to.equal(1)
})

it('should coalesce parallel dials to the same peer (no id in multiaddr)', async () => {
libp2p = new Libp2p({
peerInfo,
modules: {
transport: [Transport],
streamMuxer: [Muxer],
connEncryption: [Crypto]
}
})
const dials = 10

const dialResults = await Promise.all([...new Array(dials)].map((_, index) => {
if (index % 2 === 0) return libp2p.dial(remoteLibp2p.peerInfo)
return libp2p.dial(remoteLibp2p.peerInfo.multiaddrs.toArray()[0])
}))

// All should succeed and we should have ten results
expect(dialResults).to.have.length(10)
for (const connection of dialResults) {
expect(Connection.isConnection(connection)).to.equal(true)
}

// We will have two connections, since the multiaddr dial doesn't have a peer id
expect(libp2p.connectionManager._connections.size).to.equal(2)
expect(remoteLibp2p.connectionManager._connections.size).to.equal(2)
})

it('should coalesce parallel dials to the same peer (id in multiaddr)', async () => {
libp2p = new Libp2p({
peerInfo,
Expand Down Expand Up @@ -405,10 +404,9 @@ describe('Dialing (direct, TCP)', () => {
const error = new Error('Boom')
sinon.stub(libp2p.transportManager, 'dial').callsFake(() => Promise.reject(error))

const fullAddress = remoteAddr.encapsulate(`/p2p/${remoteLibp2p.peerInfo.id.toB58String()}`)
const dialResults = await pSettle([...new Array(dials)].map((_, index) => {
if (index % 2 === 0) return libp2p.dial(remoteLibp2p.peerInfo)
return libp2p.dial(fullAddress)
return libp2p.dial(remoteAddr)
}))

// All should succeed and we should have ten results
Expand Down
15 changes: 9 additions & 6 deletions test/dialing/direct.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const { AbortError } = require('libp2p-interfaces/src/transport/errors')
const { codes: ErrorCodes } = require('../../src/errors')
const Constants = require('../../src/constants')
const Dialer = require('../../src/dialer')
const PeerStore = require('../../src/peer-store')
const TransportManager = require('../../src/transport-manager')
const Libp2p = require('../../src')

Expand All @@ -29,13 +30,15 @@ const { MULTIADDRS_WEBSOCKETS } = require('../fixtures/browser')
const mockUpgrader = require('../utils/mockUpgrader')
const createMockConnection = require('../utils/mockConnection')
const { createPeerId } = require('../utils/creators/peer')
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws')
const unsupportedAddr = multiaddr('/ip4/127.0.0.1/tcp/9999/ws/p2p/QmckxVrJw1Yo8LqvmDJNUmdAsKtSbiKWmrXJFyKmUraBoN')
const remoteAddr = MULTIADDRS_WEBSOCKETS[0]

describe('Dialing (direct, WebSockets)', () => {
let localTM
let peerStore

before(() => {
peerStore = new PeerStore()
localTM = new TransportManager({
libp2p: {},
upgrader: mockUpgrader,
Expand All @@ -49,13 +52,13 @@ describe('Dialing (direct, WebSockets)', () => {
})

it('should have appropriate defaults', () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })
expect(dialer.concurrency).to.equal(Constants.MAX_PARALLEL_DIALS)
expect(dialer.timeout).to.equal(Constants.DIAL_TIMEOUT)
})

it('should limit the number of tokens it provides', () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })
const maxPerPeer = Constants.MAX_PER_PEER_DIALS
expect(dialer.tokens).to.have.length(Constants.MAX_PARALLEL_DIALS)
const tokens = dialer.getTokens(maxPerPeer + 1)
Expand All @@ -64,14 +67,14 @@ describe('Dialing (direct, WebSockets)', () => {
})

it('should not return tokens if non are left', () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })
sinon.stub(dialer, 'tokens').value([])
const tokens = dialer.getTokens(1)
expect(tokens.length).to.equal(0)
})

it('should NOT be able to return a token twice', () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })
const tokens = dialer.getTokens(1)
expect(tokens).to.have.length(1)
expect(dialer.tokens).to.have.length(Constants.MAX_PARALLEL_DIALS - 1)
Expand Down Expand Up @@ -107,7 +110,7 @@ describe('Dialing (direct, WebSockets)', () => {
})

it('should fail to connect to an unsupported multiaddr', async () => {
const dialer = new Dialer({ transportManager: localTM })
const dialer = new Dialer({ transportManager: localTM, peerStore })

await expect(dialer.connectToPeer(unsupportedAddr))
.to.eventually.be.rejectedWith(AggregateError)
Expand Down
8 changes: 3 additions & 5 deletions test/dialing/relay.node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,10 @@ describe('Dialing (via relay, TCP)', () => {
})

it('dialer should stay connected to an already connected relay on hop failure', async () => {
const relayAddr = relayLibp2p.transportManager.getAddrs()[0]
const relayIdString = relayLibp2p.peerInfo.id.toB58String()
const relayAddr = relayLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${relayIdString}`)

const dialAddr = relayAddr
.encapsulate(`/p2p/${relayIdString}`)
.encapsulate(`/p2p-circuit/p2p/${dstLibp2p.peerInfo.id.toB58String()}`)

await srcLibp2p.dial(relayAddr)
Expand All @@ -142,16 +141,15 @@ describe('Dialing (via relay, TCP)', () => {
})

it('destination peer should stay connected to an already connected relay on hop failure', async () => {
const relayAddr = relayLibp2p.transportManager.getAddrs()[0]
const relayIdString = relayLibp2p.peerInfo.id.toB58String()
const relayAddr = relayLibp2p.transportManager.getAddrs()[0].encapsulate(`/p2p/${relayIdString}`)

const dialAddr = relayAddr
.encapsulate(`/p2p/${relayIdString}`)
.encapsulate(`/p2p-circuit/p2p/${dstLibp2p.peerInfo.id.toB58String()}`)

// Connect the destination peer and the relay
const tcpAddrs = dstLibp2p.transportManager.getAddrs()
await dstLibp2p.transportManager.listen([multiaddr(`/p2p-circuit${relayAddr}/p2p/${relayIdString}`)])
await dstLibp2p.transportManager.listen([multiaddr(`/p2p-circuit${relayAddr}`)])
expect(dstLibp2p.transportManager.getAddrs()).to.have.deep.members([...tcpAddrs, dialAddr.decapsulate('p2p')])

// Tamper with the our multiaddrs for the circuit message
Expand Down
Loading