add openssl support #61
Conversation
ghost
assigned 
| ) | ||
|
|
||
| // define these as separate types so we can add more key types later and reuse | ||
| // code. |
There was a problem hiding this comment.
Unfortunately, RSA is the only place where we use the PKCS1v15 signature format, as far as I can tell and I can't find an easy way to get something else from this openssl library.
1. openssl doesn't support pk encryption. 2. marshaling the private key can return an error in openssl (and possibly other libraries).
These keys are _shared_ and shouldn't be modified.
We can take a pointer to the struct field later.
|
The OpenSSL library uses cgo, which afaik will cause the whole binary to be built on cgo mode, which prevents a lot of optimizations usually performed by the Go compiler. Do we really need to fix this issue, considering that we’re in the procreas of phasing our secio right at this moment? |
|
This is only enabled when
I know performance from go to C will suck but I've never heard of it preventing other optimizations. |
.travis.yml
Outdated
|
|
||
| matrix: | ||
| - GOTFLAGS="-race -tags=openssl" | ||
| - GOTFLAGS="-tags=openssl" |
There was a problem hiding this comment.
shouldn't we have a build in the matrix to test the go implementation as well?
|
Are there any other hosts than mars that are experiencing that problem and that are expected to compile their own IPFS? If not, this might be better left in a separate branch, instead of adding it to the master branch just to become technical debt as soon as the transition to TLS is completed.
I think I’ve read an article about that some time ago, but I could remember it incorrectly. |
|
@marten-seemann the relays are pretty hammered with connections and spend a lot of time in RSA handshakes. |
|
@vyzo My main point is that the benefits of this workaround will be pretty short-lives, as we’re already deploying TLS: The last IPFS release added TLS support (but defaults to secio), and the plan is to switch to TLS by default (with secio as a fallback) with the next release. |
|
We'll have secio with us for a while, not everyone in the network updates promptly. |
There was a problem hiding this comment.
Note that all of this will be moved to go-libp2p-core.
Besides https://github.com/libp2p/go-libp2p-crypto/pull/61/files#r280682324, this LGTM.
Benchmarks, consistent with my observations in libp2p/go-libp2p-secio#42 (comment) 🚀
❯ openssl version
LibreSSL 2.6.5
❯ go test -test.bench=.*Benchmark.*RSA.* -test.benchmem | tee 1.txt
goos: darwin
goarch: amd64
pkg: github.com/libp2p/go-libp2p-crypto
BenchmarkSignRSA1B-8 5000 341062 ns/op 16137 B/op 103 allocs/op
BenchmarkSignRSA10B-8 5000 337009 ns/op 16135 B/op 103 allocs/op
BenchmarkSignRSA100B-8 5000 342037 ns/op 16142 B/op 103 allocs/op
BenchmarkSignRSA1000B-8 5000 347712 ns/op 16139 B/op 103 allocs/op
BenchmarkSignRSA10000B-8 5000 362775 ns/op 16140 B/op 103 allocs/op
BenchmarkSignRSA100000B-8 3000 556653 ns/op 16137 B/op 103 allocs/op
BenchmarkVerifyRSA1B-8 5000 340684 ns/op 16140 B/op 103 allocs/op
BenchmarkVerifyRSA10B-8 5000 337609 ns/op 16137 B/op 103 allocs/op
BenchmarkVerifyRSA100B-8 5000 340003 ns/op 16138 B/op 103 allocs/op
BenchmarkVerifyRSA1000B-8 5000 340086 ns/op 16138 B/op 103 allocs/op
BenchmarkVerifyRSA10000B-8 5000 366710 ns/op 16142 B/op 103 allocs/op
BenchmarkVerifyRSA100000B-8 3000 562177 ns/op 16140 B/op 103 allocs/op
PASS
ok github.com/libp2p/go-libp2p-crypto 21.866s
❯ go test -tags=openssl -test.bench=.*Benchmark.*RSA.* -test.benchmem | tee 2.txt
goos: darwin
goarch: amd64
pkg: github.com/libp2p/go-libp2p-crypto
BenchmarkSignRSA1B-8 20000 97005 ns/op 196 B/op 5 allocs/op
BenchmarkSignRSA10B-8 20000 95826 ns/op 196 B/op 5 allocs/op
BenchmarkSignRSA100B-8 20000 98084 ns/op 196 B/op 5 allocs/op
BenchmarkSignRSA1000B-8 10000 100483 ns/op 196 B/op 5 allocs/op
BenchmarkSignRSA10000B-8 10000 118962 ns/op 196 B/op 5 allocs/op
BenchmarkSignRSA100000B-8 5000 315869 ns/op 196 B/op 5 allocs/op
BenchmarkVerifyRSA1B-8 20000 95454 ns/op 196 B/op 5 allocs/op
BenchmarkVerifyRSA10B-8 20000 102122 ns/op 196 B/op 5 allocs/op
BenchmarkVerifyRSA100B-8 20000 103856 ns/op 196 B/op 5 allocs/op
BenchmarkVerifyRSA1000B-8 10000 103769 ns/op 196 B/op 5 allocs/op
BenchmarkVerifyRSA10000B-8 10000 124242 ns/op 196 B/op 5 allocs/op
BenchmarkVerifyRSA100000B-8 5000 335315 ns/op 196 B/op 5 allocs/op
PASS
ok github.com/libp2p/go-libp2p-crypto 26.499s
❯ benchcmp {1,2}.txt
benchmark old ns/op new ns/op delta
BenchmarkSignRSA1B-8 341062 97005 -71.56%
BenchmarkSignRSA10B-8 337009 95826 -71.57%
BenchmarkSignRSA100B-8 342037 98084 -71.32%
BenchmarkSignRSA1000B-8 347712 100483 -71.10%
BenchmarkSignRSA10000B-8 362775 118962 -67.21%
BenchmarkSignRSA100000B-8 556653 315869 -43.26%
BenchmarkVerifyRSA1B-8 340684 95454 -71.98%
BenchmarkVerifyRSA10B-8 337609 102122 -69.75%
BenchmarkVerifyRSA100B-8 340003 103856 -69.45%
BenchmarkVerifyRSA1000B-8 340086 103769 -69.49%
BenchmarkVerifyRSA10000B-8 366710 124242 -66.12%
BenchmarkVerifyRSA100000B-8 562177 335315 -40.35%
benchmark old allocs new allocs delta
BenchmarkSignRSA1B-8 103 5 -95.15%
BenchmarkSignRSA10B-8 103 5 -95.15%
BenchmarkSignRSA100B-8 103 5 -95.15%
BenchmarkSignRSA1000B-8 103 5 -95.15%
BenchmarkSignRSA10000B-8 103 5 -95.15%
BenchmarkSignRSA100000B-8 103 5 -95.15%
BenchmarkVerifyRSA1B-8 103 5 -95.15%
BenchmarkVerifyRSA10B-8 103 5 -95.15%
BenchmarkVerifyRSA100B-8 103 5 -95.15%
BenchmarkVerifyRSA1000B-8 103 5 -95.15%
BenchmarkVerifyRSA10000B-8 103 5 -95.15%
BenchmarkVerifyRSA100000B-8 103 5 -95.15%
benchmark old bytes new bytes delta
BenchmarkSignRSA1B-8 16137 196 -98.79%
BenchmarkSignRSA10B-8 16135 196 -98.79%
BenchmarkSignRSA100B-8 16142 196 -98.79%
BenchmarkSignRSA1000B-8 16139 196 -98.79%
BenchmarkSignRSA10000B-8 16140 196 -98.79%
BenchmarkSignRSA100000B-8 16137 196 -98.79%
BenchmarkVerifyRSA1B-8 16140 196 -98.79%
BenchmarkVerifyRSA10B-8 16137 196 -98.79%
BenchmarkVerifyRSA100B-8 16138 196 -98.79%
BenchmarkVerifyRSA1000B-8 16138 196 -98.79%
BenchmarkVerifyRSA10000B-8 16142 196 -98.79%
BenchmarkVerifyRSA100000B-8 16140 196 -98.79%
|
@marten-seemann yeah, we'll need to support SecIO with RSA keys for a while... |
|
Sidenote: so happy we don't have to gx new imports any longer. |
|
I am dying to get this bubbled up and deployed to the relays! |
|
@vyzo while this is definitely worth a release, in general we're abusing releases by making one per PR merged (almost). Just do this for now, and you're set: |
|
What's wrong with one release per PR when merging in major new functionality? These are patch releases, not minor increments. |
|
In the meantime I'll deploy relays from a branch, using this branch, to get some early testing. |
Non-comprehensive list of reasons: too much noise, unpredictable for downstream users, haphazard, difficult to keep tabs on, high risk of making bad/incomplete releases, lack of planning... Basically: we are abusing patch releases as a way to compensate for lack of continuous integration and nightly builds. You should have an easy way to run the latest daemon commit pulling in all transitive masters -- that's where https://github.com/libp2p/workspace-go-libp2p is headed, but incomplete. |
|
I am not interested in running the daemon locally, but rather deploying it to the relay fleet.
That's heavy. I don't think we are abusing anything by making a patch release for each new major PR merged in. And it makes fleet maintenance much easier. |
|
@vyzo you are only thinking about your needs, we should think about users too ;-) Offloading the conversation to libp2p/go-libp2p#621. The lack of CI creates many deficiencies as pointed out in that issue, and patch releases ain't gonna solve those (it's still a manual process based on developer judgement). |
|
I would argue that our users do care about having the latest patch release be up-to-date with master :) |
(and tests)
WRT technical debt, this package isn't changed all that much so I don't expect too much maintenance burden. If that changes, this change is also mostly invisible so we should be able to just remove it without downstream noticing (we're not extending any interfaces). |
Also adds conformance tests to make sure we're generating the same thing each
time.
Also removes:
MarshalRsa{Private,Public}Key. These didn't return errors but openssl may.fixes libp2p/go-libp2p-secio#42