allow connecting to addresses with p2p id

[arnetheduck]

When passing an address like /ip4/192.168.1.106/tcp/10634/p2p/16Uiu2HAmTaEoEpEFkrCKfU5srxYrgkgBz4e2LJDhdod4VwgH2DkC to connect, it should succeed to connect and verify that the peer id matches the given on.

Instead, the address gets dropped here because of the full match requirement:

https://github.com/status-im/nim-libp2p/blob/1de7508b64e29c2bcad7a393c36cf8390a8c50fe/libp2p/transports/tcptransport.nim#L261

[Menduist]

Not sure about this one, since we just introduce a connect without PeerId which has a very different meaning. However, we could do

# need better name
let (address, peerId) = parseFullMa("/ip4/192.168.1.106/tcp/10634/p2p/16Uiu2HAmTaEoEpEFkrCKfU5srxYrgkgBz4e2LJDhdod4VwgH2DkC")
await switch.connect(address, peerId)

[arnetheduck]

seems unnecessarily complicated: switch.connect(ma) could simply take the peerid into account and do a check if it's present, and do peerid discovery if it's not

[Menduist]

Knowing the PeerId beforehand or not gives you very different security properties (like using HTTPS with or without certificate) So connecting to someone without knowing his PeerId should be opt-in and explicit, not something determined at runtime depending on a parameter

[arnetheduck]

I'm not sure I see the difference - we have two connect API - one that requires peerid and one that doesn't - we also have two potential multiaddresses: with and without peerid:

• connect with peerid:
  • ma with peerid: allow if connect-peerid matches ma-peerid and negotiation matches
  • ma without peerid: allow if negotiation matches
• connect without peerid (that returns peerid)
  • ma with peerid: allow if negotiation matches, return the given peerid
  • ma without peerid: allow always and return negotiatied peerid

Under none of those cases do we "weaken" security - in two cases, we arguably strengthen it because we use the peerid dynamically given by the user - what am I missing?

[Menduist]

The connect without peerid (that returns peerid) should be an advanced feature that should not be promoted for general use, because it's easy to shoot yourself in the foot with it

Let's say someone do something like:

const bootstrap = "/ip4/27.44.1.106/tcp/10634/p2p/16Uiu2HAmTaEoEpEFkrCKfU5srxYrgkgBz4e2LJDhdod4VwgH2DkC"
discard await switch.connect(bootstrap)

for their bootstrap strategy. That's completely valid & safe, so no issue there. But then, at some later time, someone updates the bootstrap for whatever reason:

const bootstrap = "/ip4/53.16.2.75/tcp/1003/"
discard await switch.connect(bootstrap)

Well, they've just shoot themselves in the foot: you're now prone to very easy MitM attack. All of this coming from a simple string change, and that string could even come from a config file / program flag / whatever, making this even more likely.

So while adding /p2p support to the connect without PeerId make senses per-se, it would also make it easier to use than the regular connect, while it should be the opposite. We could also rename the connect without PeerId to connectDontUseIfYouDontKnowWhatYouAreDoing, but that seems less elegant

[arnetheduck]

In your example, I'd say the user of libp2p has already demonstrated a willingness to ignore MITM by discard:ing the result - I'm not convinced making them split the MA makes much of a difference - if they are discarding, they can still call the connect without peerid after splitting (which will be less secure) - the MITM problem is the discard, not the string you pass in.

One thing that could be done is to add allowUnverified: bool to the connect without peerid - it would then require a peerid in the MA unless allowUnverified is true.

[Menduist]

In your example, I'd say the user of libp2p has already demonstrated a willingness to ignore MITM by discard:ing the result

Not really, same exact issue here:

const bootstrap = "/ip4/27.44.1.106/tcp/10634/p2p/16Uiu2HAmTaEoEpEFkrCKfU5srxYrgkgBz4e2LJDhdod4VwgH2DkC"
let peerId = await switch.connect(bootstrap)
let stream = switch.dial(peerId, "SomeProtocol")

One thing that could be done is to add allowUnverified: bool to the connect without peerid

Yes, that seems good :+1: