breaking: refactor to use mocha, tweetnacl

[garbados]

This PR:

• Refactors the codebase to use async/await syntax.
• Uses garbados-crypt for encryption.
• Uses mocha instead of tape and tspec.
• Applies linting via standard, checks dependencies with dependency-check.
• Updates dependencies to latest versions.
• Allows running tests against a CouchDB instance using the USE_COUCH and COUCH_URL environment variables.
• Tells Travis to current use versions of NodeJS.

Here is example test output:

  crypto-pouch
    ✓ should encrypt documents
    ✓ should fail when using a bad password
    ✓ should preserve primary index sorting
    ✓ should preserve secondary index sorting
    ✓ should error on attachments
    ✓ should ignore attachments when so instructed
    ✓ should handle _deleted:true ok
    ✓ should accept crypto params as an object

  8 passing (54ms)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |   93.48 |    80.95 |     100 |   97.67 |                   
 index.js |   93.48 |    80.95 |     100 |   97.67 | 82                
----------|---------|----------|---------|---------|-------------------

However, when running against CouchDB, the test suite fails because the incoming transform is never applied. This seems to be an issue with transform-pouch that I have not yet debugged. As an example:

$ USE_COUCH=true npm test

> crypto-pouch@3.1.3 test /home/deus/code/crypto-pouch
> standard && dependency-check --unused --no-dev . && mocha

  crypto-pouch
    1) should encrypt documents
    2) should fail when using a bad password
    ✓ should preserve primary index sorting (67ms)
    ✓ should preserve secondary index sorting (113ms)
    3) should error on attachments
    ✓ should ignore attachments when so instructed (66ms)
    ✓ should handle _deleted:true ok (46ms)
    ✓ should accept crypto params as an object (52ms)

  5 passing (662ms)
  3 failing

  1) crypto-pouch
       should encrypt documents:
     TypeError: invalid encoding
      at validateBase64 (node_modules/tweetnacl-util/nacl-util.js:18:13)
      at util.decodeBase64 (node_modules/tweetnacl-util/nacl-util.js:45:9)
      at Crypt.decrypt (index.js:25:42)
      at Object.outgoing (index.js:70:51)
      at outgoing (node_modules/transform-pouch/index.js:38:21)
      at /home/deus/code/crypto-pouch/node_modules/transform-pouch/index.js:87:16
      at processTicksAndRejections (internal/process/task_queues.js:93:5)
      at async Context.<anonymous> (test.js:44:23)

Until this issue with using remote databases is resolved, this PR should be considered a work in progress submitted for review.

[calvinmetcalf]

This package is pretty much abandoned by me so I gave you access feel free to go nuts, though if you change the crypto please make it a major release (I can give you npm access).

Now that browser crypto is available in node, if you were to change the crypto primitives I'd suggest using stuff from there as it would be faster and probably safer

[garbados]

Thanks, Calvin. I'll see what I can do. I'm not prepared to merge this yet but I appreciate the vote of confidence :)

Regarding browser crypto, I've found that tweetnacl has a smaller footprint than using built-in crypto, especially as crypto-browserify is itself three years unmaintained. Additionally tweetnacl uses the xsalsa20-poly1305 algorithm for symmetric encryption (a recommended algorithm) which built-in crypto does not.

[calvinmetcalf]

right xsalsa20-poly1305 is probably better all things being equal but AES-GCM done natively is probably better then salsa20-poly1305 done in pure JS. Same with curves, x25519 is probably better then P-521 in a vacuum but the native stuff is going to be much faster and possibly more secure then the pure js stuff.

Hashing is going to be an order of magnitude faster especially if you use a KDF to generate the key from the password (which you probably should to make brute forcing the DB harder).

I also wasn't suggesting you use crypto-browserify I was suggesting you use https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto / https://nodejs.org/api/webcrypto.html

[garbados]

Valid. I'll change up the primitives... And probably release it as its own package, since I want to use the same approach for ComDB.

[calvinmetcalf]

https://github.com/calvinmetcalf/native-crypto might be helpful, it doesn't do exactly what you want but it might be a helpful starting point

[garbados]

I've isolated the transform-pouch bug detailed above in an issue here.

As for swapping out crypto primitives, I've tried implementing password-based encryption using webcrypto here but the result is... very problematic. As you have some experience in this area, perhaps you could provide some pointers?

[calvinmetcalf]

ok remember what i said on the other pull about it being ok to derive the password for each message? That probably doesn't apply here, you'd probably want to save the salt and the parameters used to derive the password in the database, and then do the expenseive derive function once and save the resulting thing to use for all the objects.

[garbados]

you'd probably want to save the salt and the parameters used to derive the password in the database

The problem I encountered when deriving the whole key during startup is that you're left with this difficult-to-transport salt. If I use the same password to instrument encryption on another device, and then I replicate encrypted documents to it, I won't be able to decrypt them because I haven't got the salt that was used to derive the initial key. Unless I misunderstand, I ultimately think using doc-specific salts is better despite the performance hit because it supports this portability. What do you think?

[calvinmetcalf]

the way crypto-pouch works currently is that it's only local encryption, so when you replicate, it decrypts replicates the decrypted documents to the other database, where that person can use crypto-pouch too if they want or not. So my comments are based on that model of crypto pouch so it may not apply.

[garbados]

Ah, yeah. I'm coming from the usage model of ComDB where replicating encrypted documents is totally something you sometimes want to do.

I'm also concerned about using crypto-pouch with pouchdb's http adapter, which will place encrypted documents into CouchDB in a way that can't be intercepted at index or replication time. This creates the possibility of replicating encrypted documents, but crypto-pouch has no way of dealing with this scenario -- of loading documents from another, encrypted database, like ComDB's .loadEncrypted(). I'd like to add that feature to support this usage model... OR have crypto-pouch throw an error when trying to use the http adapter, so users don't experience unexpected behavior.

[calvinmetcalf]

yeah the current crypto-pouch model is very much encryption at rest only, and I'm not totally sure it's the right model, it's caused some confusion in the pst

[garbados]

I mean, it's certainly not a bad model! ComDB is just a different model. It might even make sense to use them together in some cases:

const db1 = new PouchDB('a')
db1.crypto('password-1')
const db2 = new PouchDB('b', { adapter: 'memory' })
db2.setPassword('password-2', { name: `${COUCH_URL}/b` })
await db1.replicate.to(db2, { comdb: false }) // replicate unencrypted only
// now the remote couchdb is full of encrypted documents encrypted with password-2

I think I'm inclined to have .crypto() throw an error if you're using the http adapter directly, just to avoid unexpected behavior.

[garbados]

Just checking in on the status of this. Here's what's left to do before I feel OK hitting merge:

• [x] .crypto() should throw an error if using an HTTP adapter.
• [x] Update README.md

Additionally, transform-pouch has a critical dependency bug that I am waiting on. (transform-pouch has another critical bug which is why my tests with crypto-pouch and CouchDB failed)

Once transform-pouch has been updated with the appropriate patches, and the other items above are addressed, I'll be prepared to merge this.

[garbados]

I've updated the README, disabled use of the plugin with the HTTP adapter, and switched from TravisCI to GitHub Actions. Once this is merged, @calvinmetcalf you will need to enable GitHub Actions in the Settings panel under Actions.

As this plugin no longer interacts with the HTTP adapter, we do not have to wait on this transform-pouch fix. So: this PR is ready for final review and merge.

[garbados]

Due to a breaking update to garbados-crypt, I've updated this refactor so that it saves an encrypted export string in a local document which is then used to re-initialize subsequent instantiations with the same random salt. This string is encrypted using the user's password but with a separate random salt; Crypt.import and crypt.export are used to instrument this.

As a result of this change, db.crypto has become an async method. The readme has been updated accordingly.

@jcoglan Could I request a final review from you?

@calvinmetcalf We are nearing the hour when I will need NPM permissions to update the package. Can you provide those to me?

[calvinmetcalf]

are you garbados on npm too ?

On Tue, Aug 3, 2021 at 1:06 PM Diana Thayer @.***> wrote:

Due to a breaking update to garbados-crypt https://github.com/garbados/crypt#new-cryptpassword-salt-opts, I've updated this refactor so that it saves an encrypted export string in a local document which is then used to re-initialize subsequent instantiations with the same random salt. This string is encrypted using the user's password but with a separate random salt; Crypt.import and crypt.export are used to instrument this.

As a result of this change, db.crypto has become an async method. The readme has been updated accordingly.

@jcoglan https://github.com/jcoglan Could I request a final review from you?

@calvinmetcalf https://github.com/calvinmetcalf We are nearing the hour when I will need NPM permissions to update the package. Can you provide those to me?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/calvinmetcalf/crypto-pouch/pull/77#issuecomment-892013679, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAITRHZWMD4DGZD3VPQYTY3T3AOZHANCNFSM44BFMKTQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

-- -Calvin W. Metcalf

[jcoglan]

@garbados The approach here looks sound, and I've verified that crypt works as required, particularly that the export() and import() functions let you reliably transport the state so that a ciphertext can be decrypted by a different Crypt instance than the one that created it.

There's a possible race condition in the section below but I don't know if it's something we should care about:

    try {
      const { exportString } = await this.get('_local/crypto')
      this._crypt = await Crypt.import(password, exportString)
    } catch (err) {
      if (err.status === 404) {
        this._crypt = new Crypt(password)
        const exportString = await this._crypt.export()
        await this.put({ _id: '_local/crypto', exportString })
      } else {
        throw err
      }
    }

If _local/crypto does not yet exist, then it is possible for concurrent instances of this code to both run await this.get('_local/crypto'), get an error, and so both try to create this document with different random content, leading to a conflict.

It might be better to do this operation by first trying to create _local/crypto, and then reading it if you get a conflict. However this might have the downside of running pbkdf2 over the password twice because of the way the crypt export()/import() workflow works.

That aside, it would be worth documenting that the setup function db.crypto() is async and must be awaited before any document writes are attempted, because the transform functions are not installed until this function completes. Writing before it completes could cause unencrypted data to be stored. Documenting that it must be awaited would also make it less likely that applications trigger the above race.

[garbados]

are you garbados on npm too ? -Calvin W. Metcalf

I sure am :)

[garbados]

it would be worth documenting that the setup function db.crypto() is async and must be awaited

Indeed, the readme has already been updated accordingly.

[...] and so both try to create this document with different random content, leading to a conflict.

I am looking at the code now to see about resolving this issue.

[jcoglan]

Indeed, the readme has already been updated accordingly.

I meant that I couldn't see an explicit mention that you must not write documents until the promise returned by db.crypto() has resolved. We do have this:

// init; after this, docs will be transparently en/decrypted
db.crypto(password).then(() => { ... })

But I worry this could be read as meaning that this is safe, when it isn't:

db.crypto(password).then(() => { ... })
db.put({ ... })

You need to do one of these to get safety:

db.crypto(password).then(() => { db.put({ ... }) })

// or
await db.crypto(password)
db.put({ ... })

Some other steps that need doing to make the package releasable:

• Add a "files" field to package.json
• Get https://github.com/pouchdb-community/crypto-pouch created, and update references in the package to point to that repo

[jcoglan]

Just to clarify the reason I mention this specifically...

db.crypto(password).then(() => { ... })
db.put({ ... })

... is that 04c5e0b81815499024b8996aba783fa8150c3b12 makes the crypto() function async in a way that means transform() is not called immediately, which means the above code used to be safe but isn't any more.

[garbados]

OK. I have made it very clear that setup is now async.

[garbados]

I tried to make setup synchronous but it is actually a huge pain that simply defers possible issues during setup. I prefer setup being async so that we can fully await the necessary prerequisite steps to encryption.

[jcoglan]

@garbados These changes look really solid :) I agree with your call that any problems encountered during setup should be surfaced by making crypto() fail, not by deferring failure until you try to read/write documents.

[calvinmetcalf]

fyi Diana you should have access on NPM.

On Thu, Aug 5, 2021 at 5:00 AM James Coglan @.***> wrote:

@garbados https://github.com/garbados These changes look really solid :) I agree with your call that any problems encountered during setup should be surfaced by making crypto() fail, not by deferring failure until you try to read/write documents.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/calvinmetcalf/crypto-pouch/pull/77#issuecomment-893290009, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAITRH5TXEN5PSL3F5W5WFDT3JHKZANCNFSM44BFMKTQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

-- -Calvin W. Metcalf

[garbados]

Thanks @calvinmetcalf ! I think we should be good to go then.