breaking: refactor to use mocha, tweetnacl #77

garbados · 2021-05-03T17:09:29Z

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

> [email protected] 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 · 2021-05-03T17:15:51Z

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 · 2021-05-03T17:26:27Z

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 · 2021-05-03T17:44:45Z

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 · 2021-05-03T18:36:27Z

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 · 2021-05-03T19:27:51Z

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 · 2021-05-05T02:48:13Z

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 · 2021-05-07T14:03:05Z

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 · 2021-05-07T14:55:26Z

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 · 2021-05-07T16:08:10Z

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 · 2021-05-07T17:03:10Z

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 · 2021-05-07T17:12:05Z

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 · 2021-05-07T17:32:27Z

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 · 2021-06-01T15:42:01Z

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

.crypto() should throw an error if using an HTTP adapter.
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 · 2021-07-21T15:31:02Z

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 · 2021-08-03T17:06:17Z

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 · 2021-08-03T17:57:53Z

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 <#77 (comment)>, 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 · 2021-08-04T15:02:03Z

@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 · 2021-08-04T15:22:26Z

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

I sure am :)

garbados · 2021-08-04T15:27:44Z

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 · 2021-08-04T15:54:46Z

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 · 2021-08-04T15:57:17Z

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

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

... is that 04c5e0b 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 · 2021-08-04T16:19:01Z

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

garbados · 2021-08-04T16:20:14Z

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 · 2021-08-05T09:00:18Z

@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 · 2021-08-06T15:09:02Z

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 <#77 (comment)>, 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 · 2021-08-06T22:39:32Z

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

garbados added 2 commits May 3, 2021 09:53

breaking: refactor to use mocha, tweetnacl

f40917b

fix: ignore bundles, update travis

3d53f69

garbados added 2 commits May 4, 2021 19:50

chore: use garbados-crypt for crypto

a9bb455

chore: bump garbados-crypt

7859da1

garbados added 5 commits July 21, 2021 07:29

fix: throw an error with http adapters

402de45

chore: verify explicitly that running .crypto repeatedly works ok

0bbda8a

chore: update readme

122de86

chore: update ci, travis -> github-actions

cfe4bf3

chore: version bump

b89a6a8

garbados added 2 commits August 3, 2021 09:17

chore: update deps, coverage

352d214

fix: save encrypted crypto creds in a local doc

04c5e0b

garbados added 2 commits August 4, 2021 09:06

fix: handle concurrent setup ok

c0802d5

chore: be very clear that crypto is now async

6af9409

chore: update garbados-crypt

7d7bb24

garbados merged commit f6edf9d into calvinmetcalf:master Aug 6, 2021

garbados mentioned this pull request Aug 6, 2021

Move repository to live under the pouchdb-community organization #78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking: refactor to use mocha, tweetnacl #77

breaking: refactor to use mocha, tweetnacl #77

garbados commented May 3, 2021 •

edited

Loading

calvinmetcalf commented May 3, 2021

garbados commented May 3, 2021

calvinmetcalf commented May 3, 2021

garbados commented May 3, 2021

calvinmetcalf commented May 3, 2021

garbados commented May 5, 2021

calvinmetcalf commented May 7, 2021

garbados commented May 7, 2021

calvinmetcalf commented May 7, 2021

garbados commented May 7, 2021

calvinmetcalf commented May 7, 2021

garbados commented May 7, 2021

garbados commented Jun 1, 2021 •

edited

Loading

garbados commented Jul 21, 2021

garbados commented Aug 3, 2021

calvinmetcalf commented Aug 3, 2021 via email

jcoglan commented Aug 4, 2021

garbados commented Aug 4, 2021

garbados commented Aug 4, 2021

jcoglan commented Aug 4, 2021

jcoglan commented Aug 4, 2021

garbados commented Aug 4, 2021

garbados commented Aug 4, 2021

jcoglan commented Aug 5, 2021

calvinmetcalf commented Aug 6, 2021 via email

garbados commented Aug 6, 2021

breaking: refactor to use mocha, tweetnacl #77

breaking: refactor to use mocha, tweetnacl #77

Conversation

garbados commented May 3, 2021 • edited Loading

calvinmetcalf commented May 3, 2021

garbados commented May 3, 2021

calvinmetcalf commented May 3, 2021

garbados commented May 3, 2021

calvinmetcalf commented May 3, 2021

garbados commented May 5, 2021

calvinmetcalf commented May 7, 2021

garbados commented May 7, 2021

calvinmetcalf commented May 7, 2021

garbados commented May 7, 2021

calvinmetcalf commented May 7, 2021

garbados commented May 7, 2021

garbados commented Jun 1, 2021 • edited Loading

garbados commented Jul 21, 2021

garbados commented Aug 3, 2021

calvinmetcalf commented Aug 3, 2021 via email

jcoglan commented Aug 4, 2021

garbados commented Aug 4, 2021

garbados commented Aug 4, 2021

jcoglan commented Aug 4, 2021

jcoglan commented Aug 4, 2021

garbados commented Aug 4, 2021

garbados commented Aug 4, 2021

jcoglan commented Aug 5, 2021

calvinmetcalf commented Aug 6, 2021 via email

garbados commented Aug 6, 2021

garbados commented May 3, 2021 •

edited

Loading

garbados commented Jun 1, 2021 •

edited

Loading