chore: use webcrypto over tweetnacl

[garbados]

This PR swaps out TweetNaCl for webcrypto, a browser standard and experimental NodeJS API.

Unfortunately the result is... very slow. And it doesn't appear to work in the browser:

Uncaught TypeError: (intermediate value).webcrypto is undefined
    <anonymous> http://localhost:5000/bundle.js:1
    <anonymous> http://localhost:5000/bundle.js:1

As for the performance...

$ npm test

> garbados-crypt@1.0.0-alpha test
> standard && dependency-check --unused --no-dev . && mocha



  crypt
    ✓ should do the crypto dance
    ✓ should fail to decrypt ok
    ✓ should do the crypto dance 1000 times (10478ms)


  3 passing (11s)

Each encryption and decryption operation takes about 5ms. This is astonishingly slow relative to TweetNaCl, making me reticent to accept it as an alternative.

Comments welcome.

[calvinmetcalf]

first issue: require('crypto').webcrypto in browserify doesn't work so you probably need to have a file that switches between node and the brower that either has module.exports = require('crypto').webcrypto or module.exports = globalThis.crypto

[calvinmetcalf]

ok as for slowness two things:

1. the nacl version wasn't using a key strengthening algorithm at all (which is intentionally slow) so that could be slowing things down. edit: see bellow It looks like what you are doing is saving the password as itself and then using the key derivation algorithm on each encryption to generate a new key, what you probably want to do is have getKeyFromPassword run the key derivation algorithm just once instead of doing it on every encryption.
2. the webcrypto one is async while the nacl is not, I don't know how you are doing you're tests but the latency inherent to async operations can often make async ones look slower then they'd be in an app. Plus the webcrypto ones are going to be totally non blocking while the nacl ones could block a bit especially if doing a lot in a row so even it turns out to actually be slower it's a tradeoff that may be acceptable.

[calvinmetcalf]

ok on 2nd thought yes you do need to probably do need to derive the password on each message since it's for one off communications that have to include the salt with them, so the real speed difference is just that with nacl you could brute force the password to a message very easily while with one you couldn't because it takes longer

[garbados]

@calvinmetcalf Hey, thanks for this feedback! It really helps. That confirmation that I'm doing things right-ish here is good motive to keep at it.

[coveralls]

Pull Request Test Coverage Report for Build 820605883

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 85 of 87 (97.7%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-3.0%) to 93.137%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
index.js	85	87	97.7%

Totals
Change from base Build 818894132:	-3.0%
Covered Lines:	86
Relevant Lines:	88

---

💛 - Coveralls

[garbados]

I've just added some rudimentary cross-platform support along with mochify for testing. Everything seems to be working properly now.

Basically, we use webcrypto if we can find it, or we fallback to using NodeJS crypto. The differences get disambiguated during import into an intermediate object crypto that exposes .getKeyFromPassword, .encrypt, and .decrypt methods, which Crypt then uses.

[garbados]

The major drawback here is that bundling NodeJS crypto adds about 500KB to a browserify bundle. For comparison, using tweetnacl, the bundle size is 50KB. (Both of these values are on a minified bundle.) That's considerable!! I sure hope we can shrink it down.

@calvinmetcalf Any ideas here?

[garbados]

OK, so I was able to get as low as 375KB:

$ npm run build
$ stat bundle.min.js 
  File: bundle.min.js
  Size: 375035

Hm.

[garbados]

Looks like others have taken issue with the size of crypto-browserify as well. It looks like it's getting a nice refresh but I don't know when to expect bundling optimizations from it.

For now I'm content to leave this PR open for further discussion while the main branch retains the use of tweetnacl. Given the status of its audit, I am not inclined to consider it a vulnerability.

[garbados]

Had a thought and my hand slipped and now the bundle is only 26kb. Essentially you tell Browserify to explicitly not bundle require('crypto') calls, which chops off a little more than 350k in shims. That's smaller than requiring tweetnacl.

Downstream users will still face this bundling problem until crypto-browserify sees the relevant improvements.

[calvinmetcalf]

ok so you have a few choices, one is that almost all the crypto packages are available separately so you can do

const { createCipheriv, createDecipheriv } = require('browserify-cipher');
const {pbkdf2} = require('pbkdf2');
const randomBytes = require('randombytes');

that way you will only get the bits you need in node

the other option is that you have a file

crypto.node.js

module.exports = require('crypto').webcrypto;

and another one called crypto.browser.js

module.exports = global.crypto; // don't use window, you might be in a web worker

and then in your package.json file you put in

"browser": {
    "./crypto.node.js": "./crypto.browser.js"
  }

that'll swap them out for you at bundle time

[garbados]

Thanks Calvin, that's great. I'll see what I can do.

[garbados]

Bundle clocks in now at 3.7kb but flops on NodeJS <16 and any browser that hasn't got subtle crypto.

[calvinmetcalf]

there may be a node only polyfill that you could prevent from being bundled, the real question is how bad is browser support and do you want to include a polyfill

[garbados]

I was able to add support for node 12 and 14 and the bundle only grew to 4.5kb, which is fine. The trick was not compiling Buffer, which costs about 20kb. Unfortunately this, inexplicably, broke the build.

[garbados]

Build fixed. I think my only remaining worry is that only rather recent browsers are supported by this model -- I had to update my local firefox to get it to work outside of the testing environment -- but I'm otherwise ready to hit merge.

[garbados]

OK. I've added a note to the readme about not excluding crypto from bundling in order to support older browsers. That's enough compatibility for me!

[garbados]

OK! Last check. So far we've got:

• Native crypto with fallback to crypto-browserify
• 4.6kb bundle (-29kb!)
• Ops average 5ms, which is generally speaking just fine
• 97% test coverage (+1%! only one line uncovered)

I'm going to let this sit for comment for about 24hr and then do a squash merge.

[garbados]

After some time and reflection, I don't believe this is the best way forward right now. Rather than rely on a complex web of unaudited dependencies to facilitate native-ish cryptography, I'm going to stick with the thing that has been audited.