-
Notifications
You must be signed in to change notification settings - Fork 276
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
addressing: Specify security protocols in multiaddr #353
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Max Inden <[email protected]>
Co-authored-by: Max Inden <[email protected]>
This proposal is ready for review and we welcome feedback by everyone across the whole libp2p community. Note that this proposal touches two fundamental building block of libp2p, namely addressing and protocol negotiation. For additional context: Protocol Select builds on top of this proposal (#349). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so nice to see this happening :)
Do we need to plan for a "phased roll out" for this, where nodes advertise both styles of multiaddr for a while?
We'll need a phased rollout, as the current code fails to properly parse / handle a security-enabled multiaddr (at least on the Go side). This is good. There are two ways to do a phased rollout here:
Option 1 allows for a faster rollout, at the expense of more addresses being advertised. I think we can live with that overhead, so this would be my preference. |
relay/circuit-v2.md
Outdated
- `<relay-R1-multiaddr>/p2p-circuit/p2p/QmTarget/p2p-circuit-inner/tls` | ||
- `<relay-R1-multiaddr>/p2p-circuit/p2p/QmTarget/p2p-circuit-inner/noise` | ||
- `<relay-R2-multiaddr>/p2p-circuit/p2p/QmTarget/p2p-circuit-inner/tls` | ||
- `<relay-R2-multiaddr>/p2p-circuit/p2p/QmTarget/p2p-circuit-inner/noise` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, when we advertise our relay addr we typically omit the p2p
part; how is this now going to work, should we duplicate the p2p
part?
Also, how does this parse as an AddrInfo?
It seems to me that we are doing this backwards with the relay addrs. |
If we must introduce So the circuit multiaddr for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are handling relay addresses wrong.
The unambiguous form would be /relay-addr/p2p-circuit/p2p-circuit-sec/tls/p2p/QmX
, which parses as an AddrInfo
and allows us to advertise just /relay-addr/p2p-circuit/p2p-circuit-sec/tls
as the address of QmX
.
@vyzo thank you for taking a look. I am having difficulties following your five comments above. Would you mind summarizing them in a single one? Do I understand correctly that you would like to:
|
yes. the second part is important for composability and parsing of
multiaddrs; it allows us to omit the p2p part for our own addresses and
parse complete multiaddrs as AddrInfos.
…On Wed, Sep 29, 2021, 14:55 Max Inden ***@***.***> wrote:
@vyzo <https://github.com/vyzo> thank you for taking a look. I am having
difficulties following your five comments above. Would you mind summarizing
them in a single one?
Do I understand correctly that you would like to:
- Rename /p2p-circuit-inner to /p2p-circuit-sec?
- Move p2p-circuit-sec before the /p2p/QmX of the destination node?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#353 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAI4STOFS6UYQXL7I4GFUDUEL5DPANCNFSM5A6TGU5A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
When using passive relaying, there is no need for the initiator to include multiple addresses in the `peer` field of the `HopMessage`.
Friendly ping @vyzo. Do the latest commits address your "request for change"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks much better now.
However, i think the changes to circuit v2 spec need a bit more thought; and it also should upgrade the document version!
@@ -294,6 +294,42 @@ Common failure status codes are: | |||
|
|||
***Note: implementations _should not_ accept connection initiations over already relayed connections*** | |||
|
|||
##### Security protocol selection for the relayed connection | |||
|
|||
Instead of negotiating the security protocol in-band, security protocols should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a may
instead of a should
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on this @vyzo? In the long run I would like to see libp2p move away from negotiating security protocols entirely and instead always advertise them in the multiaddr. Main motivator is the prevention of downgrade attacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but this really is up to the client and it also reduces the noise in addr advertisement.
I think we should allow both.
Also, mandating the sec part makes current implementations automatically non compliant, which is bad karma!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, mandating the sec part makes current implementations automatically non compliant, which is bad karma!
Implementations would still be spec compliant.
- SHOULD This word, or the adjective "RECOMMENDED", mean that there
may exist valid reasons in particular circumstances to ignore a
particular item, but the full implications must be understood and
carefully weighed before choosing a different course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a MAY
, at least for the time being. We can change it to SHOULD
later, when all is implemented and the spec is ready for candidate recommendation status.
If I understand correctly, we won't be able to support multiple security protocol on a single port anymore? That seems bad to me, it would become very difficult to, for instance, update noise to a new non-compatible version. (if that seem unlikely to you, #355 requires a new non-compatible noise version to be fixed, for instance) I'm also not 100% convinced by the "downgrade" argument. Sure, it applies to mobile phones because you can't update the 3310s to safer protocols, but in our case, if a protocol is less secure, just disable it (like it was done for secio), push an update, and problem solved. Finally, I'm having a hard time to imagine how active relays work in this scenario. How does B advertise that it handles noise? Is it supposed to advertise something like
Weird, but ok. Except that now we're back to my first remark, when it will receive a connection coming from a relay, it will not know if it's a noise or a tls one. So again, need to listen on 2 different ports. Now, let's be crazy and say that B also handles Webrtc, it will need to advertise:
To sum up, it seems to me that we are putting "capabilities" into the MA, and that doesn't seem right. Each MA is already a capability. My understanding is that this grew out of the goal of 0/low-RTT setups, but low RTTs can also be achieved with a negotiation. A: supports Noise, TLS, Secio A -> B: open connection using Noise, TLS or Secio You can improve on this in multiple ways (include handshake for both noise & tls in the first message, or in case of incompatibility send a TLS handshake from B to A to save half a RTT) |
@Menduist Thank for you for your review. There are multiple reasons for including the security in the multiaddr:
Note that is not required to run different handshake algorithms on different ports. You could also try to demux on the first (couple of) bytes of the first handshake message. Using different ports is the implementation strategy we will use in go-libp2p though.
Sure, this is clear-cut example. If a protocol is broken, you disable it. But the more realistic option is that you (strongly) prefer one protocol over the other, even if it's not considered broken yet. This was actually the case with TLS / Noise vs. secio for the longest time.
Another angle to look is that we're using port number to run multiple incompatible protocols on the same host, which is exactly what port numbers were invented for.
|
Thanks for the details, let me address the benefits you talked about: It prevents downgrade attacks.I would argue that since different security protocols runs on different ports, it becomes even easier to pull up a downgrade attack.
Since libp2p will try every address, it will eventually try a port/security protocol combo that is unsafe, or the connection won't go through. It's a bit slower than the previous attack, but can be sped up by knowing the "safe" ports before hand, and blocking them Note that even if you manage to put every security protocol on the same port, I presented in my first review a solution to downgrade attacks, I'll go in more detail here: It's slow and stupid, but the fact that this security exist should deter anyone from trying to downgrade the connections. It saves one roundtripSaving a connection after an invalid message has been sent has proved quite difficultSee my proposal here: #349 (comment) You're assuming that Noise is supported by all / most implementationsI used noise as an example, but it could be any protocol. Here are a few possibilities to improve that:
Also, I'm may be biased because of how the nim-libp2p is used, but sometimes, the security protocols are defined by an upper spec, eg eth2 It makes it possible to disguise libp2p traffic as regular web traffic (by running /tls on port 443).I agree. However, if we take a step back, that is, as far as I know, just a "theoretical" problem for now, ie, no one blocked libp2p using traffic pattern detection so far. If it happens one day*, we already have some transports that can't be detected (WSS, Quic, Webrtc), which would leave us enough time to apply proper countermeasures.
Other comments
Yes, though you don't see a new port being used everytime TLS is updated. I mean, in the 30 years HTTP existed, they only used 2 ports (80, 443), but in my last relay example I already need 4 ports.
Let's just take as an example "noise migration from
You could argue that the port should be transparent to the user, and in some cases that apply. But if you run a node worth >70k$, you probably want drastic security on it, with every unknown port blocked. Closing thoughtsWith this proposal, the downgrade is not addressed completely, the round trip could be saved in other ways. The complexity added to relays is imo, unnecessary (if a relay is blocking libp2p.. just use another one) imo, the only valid improvement is the ability to do So it should not become a default, but just something we could do if at some point we start to see traffic-based censorship |
Proposal to advertise security protocols in multiaddr, e.g.
/ip4/6.6.6.6/tcp/1234/tls
.This is split out of the Protocol Select specification proposal (#349) and instead specified as a independent change.