-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
use TLS by default, secio as a fallback #710
Conversation
b64e0d7
to
f831944
Compare
.travis.yml
Outdated
@@ -4,7 +4,7 @@ os: | |||
language: go | |||
|
|||
go: | |||
- 1.11.x | |||
- 1.12.x |
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.
We need TLS 1.3, which was added in Go 1.12.
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.
Only 80% of the DHT currently supports TLS. We may want to hold off on this for a bit.
Makes sense. |
I'd like to wait for ~90%, but I don't feel strongly about this. No latter than October and probably in go-ipfs 0.5.0 (the next feature release). |
Besides the RTT tradeoffs, we need to characterise handshake cost (CPU, memory, allocs, etc.) and ongoing AEAD costs once the secure channel is established. A good test case for our fancy new testbed. |
f831944
to
891ad2e
Compare
@Stebalien I rebased and updated the PR. Do you think we can merge this now? |
Let's set it as the default in testground first. I'm worried that libp2p/go-libp2p-tls#37 will be problematic for switching to TLS by default. |
Isn’t that one fixed? |
Yes, but that fix hasn't been released. If we dial one of these nodes with the TCP transport, we could connect then have the connection closed (by the remote peer) out from under us. |
The fix has been released but a large portion of the DHT is still running a very old version of go-ipfs. At this point, enough of the network has updated that we can probably merge this. |
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 would not like to automatically penalise connection establishment against all other libp2p implementations with an extra round trip, on every connection. The TLS handshake is not supported on all other implementations -- they will actually lean towards Noise.
Can IPFS do this in their host constructor?
We will do this. At a minimum, we need to support TLS by default, even if it's not the default. |
But don't make it the default per #710 (review).
We won't be defaulting to TLS, as that would introduce an interoperability tax at this point. We will be migrating to Noise as a default handshake though, as 6 out of 7 implementations of libp2p support it. |
No description provided.