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

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

Merged
merged 11 commits into from
Oct 25, 2019

Conversation

mkg20001
Copy link
Member

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

@mkg20001 support needs to be shimmed for the browser as it was before

@mkg20001
Copy link
Member Author

Would need some help with finishing this pr. Anyone mind helping here?

@mkg20001
Copy link
Member Author

Currently trying to implement this, anyone mind helping?

@daviddias
Copy link
Member

What’s the main blockers you are hitting, @mkg20001?

@mkg20001
Copy link
Member Author

@daviddias The problem is that webcrypto requires RSA-OAEP keys for signatures/verifications and RSASSA-PKCS1-v1_5 for encryption/decryption. I tried multiple methods to convert between those formats and got stuck because I only tried messing with the JWK object so far because messing with all the different types of buffers and base64-de/encoding methods that exist nowadays is... a mess.

And that's when I decided to just push this incomplete thing that's mostly just an attempt at modifying the jwk to fool webcrypto into thinking it's the right format (I'm guessing contents are identical for both formats because nodeJS allows re-using the same key for encryption and signing, I guess I'm wrong in terms of the JWK contents not needing conversion) and work on another solution based on node-forge. Turns out it's a mess, because encodings are for some reason always messed up (binary <-> utf8).

Afterwards I found a stackoverflow answer, which doesn't include any code and advice to not do that.

@dignifiedquire
Copy link
Member

Could you explain why RSA encryption suddenly is used/required, and why that is?

@mkg20001
Copy link
Member Author

mkg20001 commented Feb 20, 2019

@dignifiedquire libp2p/js-libp2p-stardust#7 is the reason this is being required.

It really would make a lot of sense to have those APIs too, because I don't really see any reason against it. It allows for stuff like not having to use nonce-signatures to confirm the identity of a peer, which would allow an attacker to make the client sign arbitrary data.

@mkg20001
Copy link
Member Author

Any updates on this? Is anyone going to help me with this one? Still stuck...

@mkg20001
Copy link
Member Author

mkg20001 commented Apr 7, 2019

@jacobheun This PR is blocking libp2p/js-libp2p-webrtc-star#148 and js-libp2p-stardust from being ready-to-use

@dignifiedquire
Copy link
Member

If we want to match go-libp2p-crypto, RSA encryption should be PKCS1v15, not OAEP.

ref https://github.com/libp2p/go-libp2p-crypto/blob/master/rsa.go#L70

@dignifiedquire
Copy link
Member

Okay, ignore my last comment, you already know that we need PKCS1v15, sorry.

The state is pretty bad looking at things, both asmcrypto.js and webcrypto.subtle only implement OAEP for encryption/decryption, and these padding algorithms are different enough that transforming one into the other is either very tricky or not possible.

@dignifiedquire
Copy link
Member

It looks like the only viable option in the browser is node-forge: https://github.com/digitalbazaar/forge#rsa

@jacobheun
Copy link
Contributor

So without using separate keys for signing and encrypting, which is probably not an option for interop at the moment, we'd need to undo some of the work from #12 to go back to node-forge?

It looks like the major impact there is bundle size, or are there other drawbacks there? If size is the major drawback I wonder if it would be worth it to move to a plugin based approach. Although as more keys are supported across the network it will create more issues if we have nodes that only come with support for specific keys.

Related issue #122 (asymmetric de/encryption)

@mkg20001
Copy link
Member Author

mkg20001 commented Apr 10, 2019 via email

@mkg20001
Copy link
Member Author

So, I've made a test to test node<->browser compatiblity:

'use strict'

const crypto = require('..')
const { deepEqual } = require('assert').strict

async function main () {
  // const id = await crypto.keys.generateKeyPair('rsa', 2048)
  const id = await crypto.keys.unmarshalPrivateKey(Buffer.from('CAASqAkwggSkAgEAAoIBAQCk0O+6oNRxhcdZe2GxEDrFBkDV4TZFZnp2ly/dL1cGMBql/8oXPZgei6h7+P5zzfDq2YCfwbjbf0IVY1AshRl6B5VGE1WS+9p1y1OZxJf5os6V1ENnTi6FTcyuBl4BN8dmIKOif0hqgqflaT5OhfYZDXfbJyVQj4vb2+Stu2Xpph3nwqAnTw/7GC/7jrt2Cq6Tu1PoZi36wSwEPYW3eQ1HAYxZjTYYDXl2iyHygnTcbkGRwAQ7vjk+mW7u60zyoolCm9f6Y7c/orJ33DDUocbaGJLlHcfd8bioBwaZy/2m7q43X8pQs0Q1/iwUt0HHZj1YARmHKbh0zR31ciFiV37dAgMBAAECggEADtJBNKnA4QKURj47r0YT2uLwkqtBi6UnDyISalQXAdXyl4n0nPlrhBewC5H9I+HZr+zmTbeIjaiYgz7el1pSy7AB4v7bG7AtWZlyx6mvtwHGjR+8/f3AXjl8Vgv5iSeAdXUq8fJ7SyS7v3wi38HZOzCEXj9bci6ud5ODMYJgLE4gZD0+i1+/V9cpuYfGpS/gLTLEMQLiw/9o8NSZ7sAnxg0UlYhotqaQY23hvXPBOe+0oa95zl2n6XTxCafa3dQl/B6CD1tUq9dhbQew4bxqMq/mhRO9pREEqZ083Uh+u4PTc1BeHgIQaS864pHPb+AY1F7KDvPtHhdojnghp8d70QKBgQDeRYFxo6sd04ohY86Z/i9icVYIyCvfXAKnaMKeGUjK7ou6sDJwFX8W97+CzXpZ/vffsk/l5GGhC50KqrITxHAy/h5IjyDODfps7NMIp0Dm9sO4PWibbw3OOVBRc8w3b3i7I8MrUUA1nLHE1T1HA1rKOTz5jYhE0fi9XKiT1ciKOQKBgQC903w+n9y7M7eaMW7Z5/13kZ7PS3HlM681eaPrk8J4J+c6miFF40/8HOsmarS38v0fgTeKkriPz5A7aLzRHhSiOnp350JNM6c3sLwPEs2qx/CRuWWx1rMERatfDdUH6mvlK6QHu0QgSfQR27EO6a6XvVSJXbvFmimjmtIaz/IpxQKBgQDWJ9HYVAGC81abZTaiWK3/A4QJYhQjWNuVwPICsgnYvI4Uib+PDqcs0ffLZ38DRw48kek5bxpBuJbOuDhro1EXUJCNCJpq7jzixituovd9kTRyR3iKii2bDM2+LPwOTXDdnk9lZRugjCEbrPkleq33Ob7uEtfAty4aBTTHe6uEwQKBgQCB+2q8RyMSXNuADhFlzOFXGrOwJm0bEUUMTPrduRQUyt4e1qOqA3klnXe3mqGcxBpnlEe/76/JacvNom6Ikxx16a0qpYRU8OWz0KU1fR6vrrEgV98241k5t6sdL4+MGA1Bo5xyXtzLb1hdUh3vpDwVU2OrnC+To3iXus/b5EBiMQKBgEI1OaBcFiyjgLGEyFKoZbtzH1mdatTExfrAQqCjOVjQByoMpGhHTXwEaosvyYu63Pa8AJPT7juSGaiKYEJFcXO9BiNyVfmQiqSHJcYeuh+fmO9IlHRHgy5xaIIC00AHS2vC/gXwmXAdPis6BZqDJeiCuOLWJ94QXn8JBT8IgGAI', 'base64'))

  const msg = Buffer.from('hello')

  const enc = await id.public.encrypt(msg)
  const dec = await id.decrypt(enc)
  deepEqual(dec, msg)

  // seems like there's some padding errors

  // browser
  const dec1 = await id.decrypt(Buffer.from('fEpJkyAhuM19EQioSsJnBXrQ9oiST77PHmVstho/L5iLIHHIK27uv8IQA7AmDMFutzFyg/q+sGmJ93iWo4FINAoj3iFpdHpAGBtXIpqTg+s0rjUP4shfO+mLje2JjzEdyV8BsdcdGP1MHn4+pMITdx4NxCXqA7HgrotT3vgWo4b22NoiPWqxencGQCGUIvahY3NZ/jrohvbJNkidEjyoX54ziVNYWp54kt2dYvAwzFrtXwgogxCKEeNeGkUkY4kDn+O/8HhnLLQjGeVsWXTi8/lpEjdUB3WLvHuY5HutfirQzbB9QMkOy5gJKxxCLjDU8KZiJQE122rcv6wHu/NX1A==', 'base64'))
  deepEqual(dec1, msg)
  // node
  const dec2 = await id.decrypt(Buffer.from('BBuW8erio4kXA9djU0nDo5KxLwcw8/jDqMy9IyXoDsshwfAI5/bztM5fTIoWjEEaK0spdpqJuyB4tYRqe4uf6jRS8Uyq3zdrPq+XubJHXyFY1v8zH+tk8AIx7TnOg61lTw+G4H53m2QCdvDPQfETkmbLQOLtZLPh2GtG2IeRHov7MfGfR4NEJxX0zu5iHtWBiqJ1XVc0KQO+8MHQurC3Cx9pZJ8PFc58gGqdZhsCUi/ScrR1o8UU9IwdII5zbm7cEMwI+VKmUDAL4vhWkYf5r2MId9+5KtpxUxKvp2iIf4eyUFVxN17HZUBWwbnI6GocksGVZRJUboldJJJGUmeNeg==', 'base64'))
  deepEqual(dec2, msg)
}

main()

Seems like node-forge does padding differently

@mkg20001
Copy link
Member Author

Finally, fixed it (for real this time)! 🎉

@jacobheun
Copy link
Contributor

Nice! Overall this looks good but it more than doubles the bundlesize :/. The majority of that is coming from pem-jwk so I am trying to see what options we have to minimize that, especially if people don't need the support in their build.

@mkg20001
Copy link
Member Author

An idea would be to use either a smaller module or make it so it's an optional argument where the user needs to pass in the module, e.g. crypto.withEncDec(require('pem-jwk')) (or in a similar fashion as is done with secp256k1)

Comment on lines 137 to 138
const forge = require('node-forge')
const pki = forge.pki
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing in the module as an extension sounds like a good approach, at least for now, so we can get this released.

Also, changing this to const pki = require('node-forge/lib/pki') will also shave off some kb, but the majority comes from pem-jwk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just found this: https://github.com/libp2p/js-libp2p-crypto/blob/master/src/keys/rsa.js#L28-L29

Also, applied that change and removed the PEM intermediary step for converting JWK to Forge

Copy link
Member Author

Choose a reason for hiding this comment

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

@mkg20001
Copy link
Member Author

So, for the bundle size:
I did a parcel build

✨  Built in 897ms.

dist/index.js.map                                                  ⚠️  1.94 MB    218ms

dist/index.js                                                       555.44 KB    463ms
├── node_modules/asn1.js/node_modules/bn.js/lib/bn.js                42.13 KB    351ms
├── node_modules/node-forge/lib/x509.js                              38.14 KB    235ms
├── node_modules/tweetnacl/nacl-fast.js                              31.34 KB    317ms
├── node_modules/node-forge/lib/util.js                              24.56 KB    260ms
├── node_modules/node-libs-browser/node_modules/buffer/index.js      19.54 KB    237ms
├── node_modules/node-forge/lib/jsbn.js                              18.35 KB    253ms
├── node_modules/node-forge/lib/rsa.js                               17.06 KB    181ms
├── node_modules/multihashes/src/constants.js                         16.6 KB    154ms
├── node_modules/node-forge/lib/pbe.js                               13.38 KB    129ms
├── node_modules/node-forge/lib/cipherModes.js                       12.29 KB     98ms
├── node_modules/node-forge/lib/pkcs12.js                            12.06 KB    173ms
├── node_modules/readable-stream/lib/_stream_readable.js             10.94 KB    116ms
├── node_modules/node-forge/lib/asn1.js                              10.76 KB    131ms
├── node_modules/secp256k1/lib/js/bn/index.js                         9.62 KB    121ms
├── node_modules/js-sha3/src/sha3.js                                  9.59 KB    167ms
├── node_modules/protocol-buffers-schema/parse.js                     9.31 KB    128ms
├── node_modules/secp256k1/lib/js/bn/optimized.js                     9.24 KB     38ms
├── node_modules/asn1.js/lib/asn1/base/node.js                        9.02 KB    138ms
├── node_modules/node-forge/lib/des.js                                8.71 KB     93ms
├── node_modules/readable-stream/lib/_stream_writable.js              7.38 KB     91ms
└── + 164 more assets                                          

The biggest dependency is forge's x509 and asn.1's bignumber, in terms of file size

We don't need x509, so once it's clear where that comes from we could possible remove it (unless it's needed)

@mkg20001
Copy link
Member Author

pki -> pkcs12 -> x509
And we need pki
So we can't remove that
Gzip increase is 15kb, is that negligible?

@mkg20001
Copy link
Member Author

mkg20001 commented Oct 25, 2019

OT, but noticed that parcel & aegir build exactly the same module in terms of size after minification, but the aegir one is bigger unminified 🤔

Edit: We'd save 400kb by using parcel, because dist/ folder is just 5,1mb compared to 5,5mb

$ npx parcel build src/index.js && du -hs dist/* && npm run build && du -hs dist/*
✨  Built in 888ms.

dist/index.js.map    ⚠️  1.94 MB    208ms
dist/index.js         555.44 KB    468ms
556K	dist/index.js
2,0M	dist/index.js.map
532K	dist/index.min.js
2,1M	dist/index.min.js.map

> [email protected] build /home/maciej/Projekte/arweave/js-libp2p-crypto
> aegir build

Child
       193 modules
Child
       193 modules
1,3M	dist/index.js
1,6M	dist/index.js.map
532K	dist/index.min.js
2,1M	dist/index.min.js.map

chore: update deps

chore: add bundlesize to ci
@jacobheun
Copy link
Contributor

I pushed up a commit to update some other dependencies and add the bundlesize to CI (and also more commitlint from ci). I am only seeing an 11kb increase after your changes, nice work! I think this is reasonable.

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

This looks good! 🚀

@jacobheun jacobheun dismissed daviddias’s stale review October 25, 2019 11:50

Review is outdated.

@jacobheun jacobheun merged commit d6d06a8 into master Oct 25, 2019
@jacobheun jacobheun deleted the feat/rsa-enc-dec branch October 25, 2019 11:51
@mkg20001
Copy link
Member Author

Can this also get backported to the non-promise version?

@jacobheun
Copy link
Contributor

@mkg20001 yes we can do that, would you be able to open up a PR for that against the v0.16.x branch?

@mkg20001
Copy link
Member Author

@jacobheun Sure, will do

@mkg20001
Copy link
Member Author

I made a pr #160
Still need to see if tests pass

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Seeking public contribution on this issue status/in-progress In progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'TypeError: this._key.encrypt is not a function' when encrypting
4 participants