fix: dont allow multiaddr dials without a peer id #558
Conversation
| try { | ||
| await libp2p.dial(remoteLibp2p.transportManager.getAddrs()[0]) | ||
| } catch (err) { | ||
| expect(err).to.have.property('code', ErrorCodes.ERR_INVALID_PEER) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
Since libp2p/js-libp2p#558 js-libp2p now requires peer ids to be part of dialled multiaddrs. Previously a user was able to copy/paste swarm addresses printed as part of the daemon starting up but now they can't because the peer id is not included in the console output. This PR prints the node's peer id as part of the swarm address to let people continue doing this and fixes the custom-libp2p example.
…up (#2775) Since libp2p/js-libp2p#558 js-libp2p now requires peer ids to be part of dialled multiaddrs. Previously a user was able to copy/paste swarm addresses printed as part of the daemon starting up but now they can't because the peer id is not included in the console output. This PR prints the node's peer id as part of the swarm address to let people continue doing this and fixes the custom-libp2p example.
…up (#2775) Since libp2p/js-libp2p#558 js-libp2p now requires peer ids to be part of dialled multiaddrs. Previously a user was able to copy/paste swarm addresses printed as part of the daemon starting up but now they can't because the peer id is not included in the console output. This PR prints the node's peer id as part of the swarm address to let people continue doing this and fixes the custom-libp2p example.
…up (#2775) Since libp2p/js-libp2p#558 js-libp2p now requires peer ids to be part of dialled multiaddrs. Previously a user was able to copy/paste swarm addresses printed as part of the daemon starting up but now they can't because the peer id is not included in the console output. This PR prints the node's peer id as part of the swarm address to let people continue doing this and fixes the custom-libp2p example.
…up (#2775) Since libp2p/js-libp2p#558 js-libp2p now requires peer ids to be part of dialled multiaddrs. Previously a user was able to copy/paste swarm addresses printed as part of the daemon starting up but now they can't because the peer id is not included in the console output. This PR prints the node's peer id as part of the swarm address to let people continue doing this and fixes the custom-libp2p example.
…up (#2775) Since libp2p/js-libp2p#558 js-libp2p now requires peer ids to be part of dialled multiaddrs. Previously a user was able to copy/paste swarm addresses printed as part of the daemon starting up but now they can't because the peer id is not included in the console output. This PR prints the node's peer id as part of the swarm address to let people continue doing this and fixes the custom-libp2p example.
…up (#2775) Since libp2p/js-libp2p#558 js-libp2p now requires peer ids to be part of dialled multiaddrs. Previously a user was able to copy/paste swarm addresses printed as part of the daemon starting up but now they can't because the peer id is not included in the console output. This PR prints the node's peer id as part of the swarm address to let people continue doing this and fixes the custom-libp2p example.
This enforces dials to a
multiaddrto include the Peer Id of the peer being dialed. This ensures that we're only connecting to peers we expect.Notes
The primary reason to specify a Peer Id when dialing is that we can verify the authenticity of the peer we are dialing during the connection crypto handshake. If the expected Peer Id does not match the Peer Id determined via the crypto handshake, we can immediately terminate the connection. Not including a Peer Id in the multiaddr prevents us from doing this.
There is the potential for a future use case where we may wish to dial an address without knowing the PeerId, for example, a js-ipfs browser node being able to dial
/ip4/127.0.0.1/tcp/4001to connect to a local running IPFS node regardless of the nodes PeerId. This would assume the address being dialed is "blessed", which should be the case with local nodes. While there is the potential for use cases like this, we don't yet have a concrete use case that warrants allowing this behavior. Additionally, if we were to allow this behavior, only specific addresses should be allowed to prevent the ability to blanket dial any address, which would require a configuration update. As there is no valid, known use case to warrant supporting this behavior, it is being removed.