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

OpenSSL vs. BoringSSL vs. Go standard library crypto #1952

Closed
marten-seemann opened this issue Dec 13, 2022 · 7 comments · Fixed by #1953
Closed

OpenSSL vs. BoringSSL vs. Go standard library crypto #1952

marten-seemann opened this issue Dec 13, 2022 · 7 comments · Fixed by #1953

Comments

@marten-seemann
Copy link
Contributor

Context

We're currently maintaining a fork of go-openssl which has created a lot of maintenance burden in the past, and I'm not optimistic that the situation will improve in the future. Also, I don't want to be maintain this library.

RSA used to be the standard key type in the early days of IPFS / libp2p, and we still have a minority of nodes in the network that use RSA keys. I just instrumented a kubo node, and 15 - 20% of nodes it connects to use RSA keys (the rest is almost exclusively Ed25519).

PL is running some nodes as a service to the network. Of those, bootstrap nodes handle a lot of short-lived connections, i.e. perform a lot of handshakes, during which we need to perform a signature verification.

Benchmarks

@Jorropo ran some benchmarks of signature verification using different key types a while back in #1686 (comment). However, there's currently a bug in our key generation code, rendering those results invalid. Benchmarks need to be run with https://github.com/libp2p/go-libp2p/tree/fix-crypto-benchmark due to #1951.

It looks like RSA signature verification is about 4x faster than the respective Go standard library code path.

  Go Standard Library OpenSSL BoringSSL
BenchmarkVerifyRSA1B-2 85.905 26.792 20.336
BenchmarkVerifyRSA10B-2 106.754 27.453 19.681
BenchmarkVerifyRSA100B-2 94.862 26.88 20.728
BenchmarkVerifyRSA1000B-2 101.62 28.35 19.747
BenchmarkVerifyRSA10000B-2 110.342 32.806 25.194
BenchmarkVerifyRSA100000B-2 170.742 100.674 80.228
BenchmarkVerifyEd255191B-2 76.216 82.31 80.681
BenchmarkVerifyEd2551910B-2 79.42 78.811 82.704
BenchmarkVerifyEd25519100B-2 78.767 81.161 89.527
BenchmarkVerifyEd255191000B-2 79.369 84.34 81.65
BenchmarkVerifyEd2551910000B-2 94.968 93.985 101.424
BenchmarkVerifyEd25519100000B-2 256.963 247.958 282.375

(all values in microseconds)

Conclusion

Go standard library signature verification is slightly (10%?) slower for RSA than for Ed25519. A significant speedup of the RSA code path can be achieved by using OpenSSL or BoringSSL, but it's questionable if that will have a big impact on the node's CPU load, given that RSA peers are just a small (and decreasing) fraction of the network. Furthermore, verifying host key signatures is not the only cost during the handshake (there's a regular TLS or Noise handshake as well). It also doesn't seem appropriate to introduce significant complexity to optimize the RSA code path, even though it's not significantly slower than the Ed25519 code path.

Am I missing something here? @vyzo @Stebalien @guseggert @lidel @p-shahi @Jorropo

@vyzo
Copy link
Contributor

vyzo commented Dec 13, 2022

As long as there is a sizeable number of nodes using RSA, we should probably keep it around.

We can probably drop it when that number drops to (say) less than 5%, maybe in a couple of kubo release cycle.

Regardless, we should also check if there are actual users that explicitly want RSA keys (and why).

@marten-seemann
Copy link
Contributor Author

marten-seemann commented Dec 14, 2022

Unfortunately, OpenSSL is and has created quite a maintenance burden over the last couple of month: maintaining go-openssl (doing the OpenSSL v3 update) and debugging OpenSSL-induced flakiness (#1892) are examples of this. I don't expect this to get better in the future. If we want to keep OpenSSL support, this would also mean we'd have to fix #1951 really soon.

Unless there's a clear, tangible benefit (1000s of $ saved in infrastructure bill per months), I'd much rather spend this effort an actually improving the stack.


I activated the Accelerated DHT client and I'm getting a very different key distribution now, RSA is down to 3.3%:

image

And even though the node is doing handling tons of connections and handshakes, certificate signing / verification doesn't even show up in the CPU profile: cpu.pprof.zip. This suggests that other parts of our stack are way more expensive to run than signature verifications, which would also mean that it's not worth putting in extra effort to optimize it.

@BigLep
Copy link
Contributor

BigLep commented Dec 14, 2022

This is great analysis @marten-seemann . Maintainer resources are precious. If we don't have compelling reasons to maintain something that's old, we should drop. I'm not seeing anything compelling here.

@p-shahi
Copy link
Member

p-shahi commented Dec 14, 2022

👍 in favor of moving away from go-openssl. Regarding maintenance overhead, one more datapoint: there's an issue @hsanjuan created libp2p/go-openssl#38 which suggests master is broken to some degree...investing in that seems like a sunken cost.

Great news that the RSA distribution is so tiny now. It is a good idea to check up on why users would still want to use RSA keys. I can start a discussion in the IPFS and libp2p forums for this (at the very least to notify people of our attention ahead of time.)
I also think we should have the Infra team resolve: https://github.com/protocol/bifrost-infra/issues/2050 this experiment and do some additional profiling on those bootstrap nodes you mentioned.

Overall, if people are in agreement here, we should move forward and timebox this migration. Mainly to ensure this doesn't get bikeshedded or stay halted on user feedback/Infra findings.
So, should we aim to include this in #1916 or v0.26.0 - imo 0.26.0 might give us the right balance of urgency (for us) and time (for user feedback and Infra investigation.)

@marten-seemann
Copy link
Contributor Author

I also think we should have the Infra team resolve: https://github.com/protocol/bifrost-infra/issues/2050 this experiment and do some additional profiling on those bootstrap nodes you mentioned.

I'm not sure BoringSSL experiment is worth running any more, since from all of the pprofs I've seen. that signature verification doesn't matter at all any more. Grabbing a pprof from a bootstrapper should allow us to double-check this.

This would also allow us to ship this in v0.25.0.

@marten-seemann
Copy link
Contributor Author

After discussing in ipfs/kubo#9506, the problem is signing, not verifying This makes sense, looking at the benchmarks: #1686 (comment).

This means that fixing this problem is somewhat easier: It doesn't matter what type of keys your peer uses, it only matters which keys you use on your node.

Unfortunately, (some of) the bootstrappers still use RSA keys, and their peer IDs are hardcoded, so we can't just turn them off immediately. At the speed the IPFS network upgrades, it probably won't be feasible to get rid of (some) RSA bootstrappers for years to come.
We should discuss a strategy to replace RSA bootstrappers with Ed25519 bootstrappers with the IPFS team. It might be reasonable to keep the RSA bootstrappers running the current Kubo version. That would allow us to drop OpenSSL support in libp2p.

@hsanjuan
Copy link
Contributor

If you remove openssl support, everyone could still run things using Go implementation, even if slower. So the fact that bootstrappers have not migrated is not a blocker is it? They will just be slower.

Jorropo added a commit to Jorropo/nixpkgs that referenced this issue Aug 11, 2023
We removed openssl support from go-libp2p and thus Kubo:
- libp2p/go-libp2p#1952

Now we exclusively on the options provided by the golang std librairy.
The openssl tag is now a noop, having it does not cause any harm except
making nixos install openssl for no reason while using kubo but I guess
many systems already build openssl but might as well not have it.
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 a pull request may close this issue.

5 participants