-
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
OpenSSL vs. BoringSSL vs. Go standard library crypto #1952
Comments
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). |
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%: 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. |
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. |
👍 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 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.) 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. |
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. |
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. |
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. |
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.
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.
(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
The text was updated successfully, but these errors were encountered: