Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

marten-seemann
Copy link
Contributor

No description provided.

.travis.yml Outdated
@@ -4,7 +4,7 @@ os:
language: go

go:
- 1.11.x
- 1.12.x
Copy link
Contributor Author

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.

Copy link
Member

@Stebalien Stebalien left a 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.

@marten-seemann
Copy link
Contributor Author

Makes sense.
What's the threshold for rolling this out, considering that selecting TLS will save peers one network round trip if both of them support it, whereas falling back to secio will cost one network round trip?

@Stebalien
Copy link
Member

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).

@raulk
Copy link
Member

raulk commented Aug 28, 2019

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.

@marten-seemann
Copy link
Contributor Author

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).

@Stebalien I rebased and updated the PR. Do you think we can merge this now?

@Stebalien
Copy link
Member

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.

@marten-seemann
Copy link
Contributor Author

Isn’t that one fixed?

@Stebalien
Copy link
Member

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.

@Stebalien
Copy link
Member

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.

@raulk?

Copy link
Member

@raulk raulk left a 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?

@Stebalien
Copy link
Member

We will do this. At a minimum, we need to support TLS by default, even if it's not the default.

Stebalien added a commit that referenced this pull request Apr 13, 2020
But don't make it the default per #710 (review).
@raulk
Copy link
Member

raulk commented May 12, 2020

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.

@raulk raulk closed this May 12, 2020
@raulk raulk deleted the tls-default-security branch May 12, 2020 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants