Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

refactor: the whole thing #102

Merged
merged 10 commits into from
Jul 22, 2017
Merged

refactor: the whole thing #102

merged 10 commits into from
Jul 22, 2017

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Jul 21, 2017

I'm currently going over this module to make it more maintainable and with an API that is easier to understand. The end goal is to make a tutorial for https://github.com/libp2p/js-libp2p and invite the community to start using it.

  • refactor to reduce dep complexity
  • update the README
  • use only one copy of webcrypto
  • explore if we really need node-webcrypto-ossl

@daviddias daviddias added the status/in-progress In progress label Jul 21, 2017
@dignifiedquire
Copy link
Member

refactor the madness

that is a bit harsh, isn't it :/

@daviddias
Copy link
Member Author

daviddias commented Jul 21, 2017

My apologies @dignifiedquire. I didn't mean that in a personal way at all, I know you put a lot of work into this module and made it work ❤️. I just had a very hard time myself wrapping my brain around how the code was organized and decided to shuffle things around so that others don't have to spend as much time. Updated my description there.

@daviddias
Copy link
Member Author

Was exposing webcrypto the whole reason why we have to have node-webcrypto-ossl?

@dignifiedquire
Copy link
Member

Was exposing webcrypto the whole reason why we have to have node-webcrypto-ossl?

The point was to expose a 1:1 mapping of all modules in nodejs which are backed by native implementations where possible, not just JavaScript.

@dignifiedquire
Copy link
Member

also some other reasons in terms of making sure formats are matching I think, try looking at past issues we talked about this multiple times as far as I remember

@daviddias
Copy link
Member Author

daviddias commented Jul 21, 2017

#102 (comment)

I recall something, but just did the diligence of trickling up the changes and npm linked everything till js-ipfs and run all the tests + interop tests with success. My conclusion is that what it was isn't an issue anymore.

@daviddias daviddias requested a review from pgte July 21, 2017 17:52
@daviddias daviddias mentioned this pull request Jul 21, 2017
@dignifiedquire
Copy link
Member

@diasdavid please run the benchmarks to check if there are regressions in node (references are here: https://github.com/libp2p/js-libp2p-crypto/blob/master/stats.md)

@daviddias
Copy link
Member Author

daviddias commented Jul 21, 2017

» node -v
v7.10.0
# before
» node ephemeral-keys.js
ephemeral key with secrect P-256 x 606 ops/sec ±5.02% (74 runs sampled)
ephemeral key with secrect P-384 x 650 ops/sec ±1.84% (80 runs sampled)
ephemeral key with secrect P-521 x 616 ops/sec ±3.41% (77 runs sampled)

# now
» node ephemeral-keys.js
ephemeral key with secrect P-256 x 612 ops/sec ±2.47% (79 runs sampled)
ephemeral key with secrect P-384 x 646 ops/sec ±1.69% (79 runs sampled)
ephemeral key with secrect P-521 x 665 ops/sec ±1.41% (81 runs sampled)

# conclusion: pretty much the same, new has slight perf wins
# before
» node key-stretcher.js
keyStretcher AES-128 SHA1 x 7,208 ops/sec ±7.02% (68 runs sampled)
keyStretcher AES-128 SHA256 x 9,894 ops/sec ±6.26% (68 runs sampled)
keyStretcher AES-128 SHA512 x 15,890 ops/sec ±5.76% (69 runs sampled)
keyStretcher AES-256 SHA1 x 6,798 ops/sec ±5.82% (73 runs sampled)
keyStretcher AES-256 SHA256 x 8,233 ops/sec ±8.23% (72 runs sampled)
keyStretcher AES-256 SHA512 x 11,676 ops/sec ±6.94% (69 runs sampled)
keyStretcher Blowfish SHA1 x 51,223 ops/sec ±12.21% (65 runs sampled)
keyStretcher Blowfish SHA256 x 50,531 ops/sec ±4.03% (64 runs sampled)
keyStretcher Blowfish SHA512 x 44,539 ops/sec ±21.92% (58 runs sampled)

# now
» node key-stretcher.js
keyStretcher AES-128 SHA1 x 7,825 ops/sec ±4.17% (69 runs sampled)
keyStretcher AES-128 SHA256 x 10,855 ops/sec ±5.73% (69 runs sampled)
keyStretcher AES-128 SHA512 x 17,042 ops/sec ±5.68% (70 runs sampled)
keyStretcher AES-256 SHA1 x 7,528 ops/sec ±5.55% (73 runs sampled)
keyStretcher AES-256 SHA256 x 9,136 ops/sec ±8.58% (71 runs sampled)
keyStretcher AES-256 SHA512 x 12,495 ops/sec ±6.97% (70 runs sampled)
keyStretcher Blowfish SHA1 x 52,725 ops/sec ±12.59% (66 runs sampled)
keyStretcher Blowfish SHA256 x 55,248 ops/sec ±4.40% (66 runs sampled)
keyStretcher Blowfish SHA512 x 44,169 ops/sec ±22.45% (57 runs sampled)

# conclusion: pretty much the same, new has interesting perf wins
# before
» node rsa.js
generateKeyPair 1024bits x 46.68 ops/sec ±8.73% (43 runs sampled)
generateKeyPair 2048bits x 7.94 ops/sec ±21.98% (24 runs sampled)
generateKeyPair 4096bits x 0.97 ops/sec ±62.45% (9 runs sampled)
sign and verify x 1,191 ops/sec ±3.24% (70 runs sampled)

# now
» node rsa.js
generateKeyPair 1024bits x 4.17 ops/sec ±34.07% (25 runs sampled)
generateKeyPair 2048bits x 0.31 ops/sec ±74.07% (6 runs sampled)
generateKeyPair 4096bits x 0.03 ops/sec ±50.20% (5 runs sampled)
sign and verify x 1,495 ops/sec ±1.14% (78 runs sampled)

# conclusion: new version show in fact some performance losses. This is due keypair generation from https://www.npmjs.com/package/keypair. This can be optimized separately.

@daviddias
Copy link
Member Author

daviddias commented Jul 21, 2017

Done. My take is that we should upgrade libp2p crypto to be able to accept other key implementations, the same way we support secp251k but using the pattern we do in js-ipfs for wrtc/webrtc.

This way, we get a module that is not painful to install and doesn't cause issues for users in Electron (for example) and still enables users that have a heavy use of RSA key sign to plug node-webcrypto (or something else) to improve the perf there.

Important to note: What is slow is generating the RSA keys, not the signing. Generating the keys only happens once.

@daviddias
Copy link
Member Author

I'm going on a limb here and merge this all the way and will monitor and run more tests. From IRC:

10:36 <@daviddias> On the libp2p-crypto, I've linked everything and run all the tests + interop tests and all of them have been successful. I recall the issue with the different encoding of keys, but can't seem to replicate it today. Truth is that the way that rsa was set up, if node-webcrypto-ossl failed, it would use the Node.js crypto module, so if there was still an issue, all the Linux people would already have called out.
10:37 <@daviddias> Any other test that comes to mind to increase the confidence level? Otherwise, I'm going on a limb here and merge it all the way. We are not in a pressure to ship a release. If we find that problem again we have time to come with a more elegant solution, or simply add node-webcrypto-ossl
10:38 <@daviddias> After this merge and release party. I want to add support BYOKM (Bring Your Own Keys Module)

@daviddias daviddias merged commit 2f8e234 into master Jul 22, 2017
@daviddias daviddias deleted the feat/refactor+polish branch July 22, 2017 17:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants