Async Crypto Endeavour #12

dignifiedquire · 2016-09-23T12:59:30Z

No more node-forge
No more elliptic.js

~~Needs https://github.com/multiformats/js-multihashing/pull/12~~ Now multihashing-async

Stats can be found here: https://github.com/libp2p/js-libp2p-crypto/blob/webcrypto/stats.md

dignifiedquire · 2016-09-26T11:24:40Z

Added PKCS#1 handling to ensure interop with go, which made rsa key generation a lot slower. Need to find a faster way to do this.

dignifiedquire · 2016-09-26T12:19:08Z

Added PKCS#1 handling to ensure interop with go, which made rsa key generation a lot slower. Need to find a faster way to do this.

Turns out it is not slower, I just misread my stats :) Just a small size increase due to the need to bundle some more JavaScript libraries.

dignifiedquire · 2016-10-04T17:11:27Z

Investigate random failure on P-521 ephemeral key gen https://travis-ci.org/libp2p/js-libp2p-crypto/jobs/165005410#L1626

daviddias · 2016-10-05T12:55:18Z

package.json

-    "protocol-buffers": "^3.1.6"
+    "asn1.js": "^4.8.1",
+    "async": "^2.0.1",
+    "multihashing-async": "^0.1.0",


the "Needs" on #12 (comment) is no longer valid, right?

daviddias · 2016-10-05T12:55:54Z

API.md

+- `buf: Buffer`
+- `callback: Function`
+
+Converts a protobuf serialized private key into its representative object.


API should be on the README.md

It's too long and was moved out, as we did before with very large APIs

I don't believe that is true, which other modules have an API.md?

daviddias · 2016-10-05T12:56:11Z

README.md

-  - [`marshalPublicKey(key[, type])`](#marshalpublickeykey-type)
-  - [`unmarshalPublicKey(buf)`](#unmarshalpublickeybuf)
-  - [`marshalPrivateKey(key[, type])`](#marshalprivatekeykey-type)
-  - [`unmarshalPrivateKey(buf)`](#unmarshalprivatekeybuf)


why stop linking to things?

Let's continue linking the methods.

daviddias · 2016-10-05T12:56:55Z

README.md

- `buf: Buffer`
-
-Converts a protobuf serialized private key into its  representative object.
+See [API.md](API.md)


The API should be on the README, it is how we do it in every other module.

daviddias · 2016-10-05T12:57:24Z

benchmarks/ephemeral-keys.js

+curves.forEach((curve) => {
+  suite.add(`ephemeral key with secrect ${curve}`, (d) => {
+    crypto.generateEphemeralKeyPair('P-256', (err, res) => {
+      if (err) throw err


Is this using latest aegir? Missing the curly

daviddias · 2016-10-05T13:05:13Z

src/crypto/webcrypto.js

+'use strict'
+
+module.exports = function getWebCrypto () {
+  const WebCrypto = require('node-webcrypto-ossl')


WARNING At this time this solution should be considered suitable for research and experimentation, further code and security review is needed before utilization in a production application.

This makes the statement that 'using webcrypto is safer' invalid because we know have to trust this shim

It's a pretty thin wrapper around openssl, so I wouldn't say that this makes it less safe than the current things we have

daviddias · 2016-10-05T13:06:23Z

src/crypto/webcrypto-browser.js

+    if (window.crypto) {
+      return window.crypto
+    }
+  }


wait, so, if in browser, we have to use a "webcrypto-shim"?

webcrypto-shim only helps to fix some differences in api in some browsers, it doesn't actually provide a shim for the apis if they are not available

I see, can you add that as a comment?

daviddias · 2016-10-05T13:07:24Z

src/crypto/webcrypto-browser.js

+    }
+  }
+
+  throw new Error('Please use an environment with crypto support')


If the shim is there, when would this happen?

see above, in all browsers that don't have webcrypto this will happen

daviddias · 2016-10-05T13:07:48Z

src/crypto/webcrypto-browser.js

+'use strict'
+
+module.exports = function getWebCrypto () {
+  if (typeof window !== 'undefined') {


use detect-node

No, we don't want to know if we are in node, we explicitly want to know if there is a window object available.

daviddias · 2016-10-05T13:08:54Z

src/keys/rsa.js

-  decrypt (bytes) {
-    return this._privateKey.decrypt(bytes, 'RSAES-PKCS1-V1_5')
+  decrypt (msg, callback) {
+    crypto.decrypt(this._key, msg, callback)


what is the difference between bytes and msg? Probably node, why change the name?

I found msg to better describe what is actually being passed through.

daviddias · 2016-10-05T13:10:00Z

This is a giant PR. I didn't see any direct go-ipfs interop tests with regards to crypto.

dignifiedquire · 2016-10-05T13:42:38Z

I didn't see any direct go-ipfs interop tests with regards to crypto.

They were there before, you can grep for go interop in the tests folder

daviddias · 2016-10-12T13:44:11Z

Needs:

Squashing of commits (plus use of commit convention)
Move API.md to README.md
Keep the table of contents with links to API calls
Do we need: Async Crypto Endeavour #12 (comment) ? When is it used?

daviddias · 2016-10-12T13:46:51Z

test/ephemeral-keys.spec.js

+          expect(err).to.not.exist
+          if (err) {
+            return done(err)
+          }


No need for this check

daviddias · 2016-10-12T13:49:55Z

src/crypto/ecdh.js

+const curveLengths = {
+  'P-256': 32,
+  'P-384': 48,
+  'P-521': 66


These lengths, are not equal to there lenghts:

daviddias · 2016-10-12T13:50:19Z

test/ephemeral-keys.spec.js

+const lengths = {
+  'P-256': 65,
+  'P-384': 97,
+  'P-521': 133


these lengths are not the same as : https://github.com/libp2p/js-libp2p-crypto/pull/12/files#r83008550

daviddias · 2016-10-12T13:50:45Z

test/index.spec.js

-    key = crypto.generateKeyPair('RSA', 2048)
+  before((done) => {
+    crypto.generateKeyPair('RSA', 2048, (err, _key) => {
+      if (err) return done(err)


Weird that aegir didn't fail to lint this one. It is missing the curly

kumavis · 2016-10-20T17:37:44Z

☕️ I am happily awaiting this PR being merged
let me know if I can assist in any way 🙆

daviddias · 2016-11-01T11:40:03Z

README.md

@@ -44,43 +58,91 @@ npm install --save libp2p-crypto
 ```js
 const crypto = require('libp2p-crypto')

-var keyPair = crypto.generateKeyPair('RSA', 2048)
+crypto.generateKeyPair('RSA', 2048, (err, key) => {
+})


Is it key or keyPair? Wanna show an example of logging out each?

BREAKING CHANGE: generateKeyPair is now async

JustinDrake · 2016-11-23T11:41:22Z

Stats return a 404

https://github.com/libp2p/js-libp2p-crypto/blob/webcrypto/stats.md

daviddias · 2016-11-23T13:13:54Z

@JustinDrake check https://github.com/libp2p/js-libp2p-crypto/blob/master/stats.md

dignifiedquire added the status/in-progress In progress label Sep 23, 2016

daviddias added libp2p-backlog and removed status/in-progress In progress labels Sep 26, 2016

dignifiedquire force-pushed the webcrypto branch from 5bf081f to eb5de10 Compare September 26, 2016 11:24

dignifiedquire mentioned this pull request Sep 26, 2016

feat(rsa): always use the fastest method available for rsa keygeneration #10

Closed

dignifiedquire changed the title ~~[WIP] Move to webcrypto as much as possible~~ Replace elliptic.js and node-forge with webcrypto Sep 26, 2016

nikuda mentioned this pull request Oct 3, 2016

Getting js-ipfs to load in browser ipfs/js-ipfs#509

Closed

dignifiedquire mentioned this pull request Oct 3, 2016

Awesome Async Crypto + Less magic to 'run in the browser' ipfs/js-ipfs#485

Merged

5 tasks

dignifiedquire added the needs review label Oct 3, 2016

dignifiedquire assigned daviddias Oct 3, 2016

daviddias suggested changes Oct 5, 2016

View reviewed changes

daviddias removed the needs review label Oct 5, 2016

dignifiedquire added the needs review label Oct 6, 2016

dignifiedquire mentioned this pull request Oct 7, 2016

Sprint: Oct 10 → Oct 17 ipfs/team-mgmt#217

Closed

daviddias removed the needs review label Oct 12, 2016

daviddias reviewed Oct 12, 2016

View reviewed changes

src/crypto/ecdh.js

const curveLengths = {

'P-256': 32,

'P-384': 48,

'P-521': 66

Copy link

Member

daviddias Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lengths, are not equal to there lenghts:

daviddias reviewed Oct 12, 2016

View reviewed changes

dignifiedquire mentioned this pull request Oct 20, 2016

browser fix - pre-load files with brfs #17

Closed

kumavis mentioned this pull request Oct 20, 2016

libp2p mesh testing MetaMask/metamask-extension#719

Closed

36 tasks

This was referenced Oct 24, 2016

Enable WebWorkers to make it go faster #9

Closed

Cross-Browser compatibility #18

Closed

daviddias changed the title ~~Replace elliptic.js and node-forge with webcrypto~~ Async Crypto Endeavour Oct 30, 2016

daviddias added libp2p-ready and removed libp2p-backlog labels Nov 1, 2016

daviddias reviewed Nov 1, 2016

View reviewed changes

daviddias approved these changes Nov 1, 2016

View reviewed changes

dignifiedquire and others added 4 commits November 1, 2016 12:47

feat: use webcrypto in favor of node-forge

08c5df5

BREAKING CHANGE: generateKeyPair is now async

docs: update the docs

6ebc408

chore: remove unused .aegir config

cecadba

feat: replace lib multihashing with multihashing-async

b088bab

dignifiedquire force-pushed the webcrypto branch from 5fd0a5c to b088bab Compare November 1, 2016 11:51

daviddias merged commit 12630dc into master Nov 3, 2016

daviddias deleted the webcrypto branch November 3, 2016 07:42

daviddias removed the libp2p-ready label Nov 3, 2016

richardschneider mentioned this pull request Dec 8, 2017

Key exchange #114

Closed

jacobheun mentioned this pull request Apr 10, 2019

feat: Add (rsa)pubKey.encrypt and (rsa)privKey.decrypt #125

Merged

Async Crypto Endeavour #12

Async Crypto Endeavour #12

Conversation

dignifiedquire commented Sep 23, 2016 • edited Loading

dignifiedquire commented Sep 26, 2016

dignifiedquire commented Sep 26, 2016

dignifiedquire commented Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Oct 5, 2016

dignifiedquire commented Oct 5, 2016

daviddias commented Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis commented Oct 20, 2016

Choose a reason for hiding this comment

JustinDrake commented Nov 23, 2016

daviddias commented Nov 23, 2016

dignifiedquire commented Sep 23, 2016 •

edited

Loading

dignifiedquire commented Oct 4, 2016 •

edited

Loading

daviddias Oct 28, 2016 •

edited

Loading

daviddias commented Oct 12, 2016 •

edited

Loading