-
Notifications
You must be signed in to change notification settings - Fork 281
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.
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
Specify the insecure
security channel
#184
Comments
We could also just send the keys only. But yeah, that sounds like the right approach. |
It looks like the c++ implementation that Soramitsu are working on sends just the keys. @Warchant, in the PR above, does the responder derive a peer id for the initiator from their key? |
So, personally, I would actually expect the plaintext protocol to not use keys at all, but still exchange peer ids. In which case you should be able to use arbitrary strings as peer ids, e.g. "insecure node 123" should be a valid plaintext peer id. The lack of keys is a pretty strong signal that this channel isn't secure 😄Whereas if a node were unknowingly misconfigured to allow plaintext connections, they could be presented with an arbitrary key and possibly trust it (e.g. by adding to the peerstore) without ever verifying. I don't know if that's actually problematic or not; just thought I'd mention. It's probably worth requiring keys just so that we can always assume that we have them, and avoid nil panics and so on. |
@yusefnapora in C++ the protocol is:
There is an invariant, that peer id is always sha256 hash of public key, so what is the reason to break it? Also, IMO
I propose to stick to |
Totally agree on it. IMO in this context of |
I thought about it more and it looks that libp2p requirement "all dials must be authenticated" is too restricting and not flexible. I, as a user of libp2p, would want to configure libp2p to have no authentication, responder authentication or mutual authentication (or even all of http://noiseprotocol.org/noise.html#payload-security-properties) - of course ability to perform one depends on the security adaptor (TLS/Noise/....) and its configuration. |
If we do that we'll have to be very careful. There are some use-cases for dropping authentication, but they're few and far between (e.g., connecting to all local peers by "guessing"). On the other hand, "helpful" parties will MITM connections to as a "feature" (e.g., captive portals, proxies, etc.). |
Great point about authentication vs encryption @Warchant. I also agree that things are much simpler if we have a consistent peer id definition. @mhchia I like having a signature to prove control of the private key, but it seems like it would be trivial to replay unless the signed data includes some kind of nonce or challenge. |
This is a reasonable ask. Would you mind opening an issue in libp2p/specs for this potential RFC? You don't need to draft the spec; I just want to make sure this request doesn't drop from the radar. By default we'd use responder authentication, but you could drop to no authentication if you're doing speculative dials, or upgrade to mutual authentication if you are certain the other peer has your key, maybe because you've exchanged them via a side channel, or because you're roaming or switching transports on an existing connection. |
#186 was merged. We're now going to adapt go-libp2p to adhere to the new spec and fix the downstream issues, and to facilitate interoperability test with py-libp2p and jvm-libp2p. |
Several implementers (e.g. cpp-libp2p and py-libp2p) are using this security channel for interoperability tests against go-libp2p.
While this is discouraged for production use, it should at least be functional. It is not.
The reason is that downstream components fail because we perform no handshake that covers identity exchange. Thus, the dialer knows the peer ID of the responder, but the latter never learns the peer ID of the dialer, causing panics and errors in places that require the peer ID:
addrSegments.get
when PeerID is empty go-libp2p-peerstore#87Let's iterate on our implementation of
insecure
, and let's do that spec-first. @yusefnapora has stepped up to lead the charge.In my view, the spec would be pretty brief.
/plaintext/1.0.0.
.The text was updated successfully, but these errors were encountered: