feat(encrypt): add functions to allow encryption and decryption of arbitrary data #241

trueinsider · 2018-06-17T06:41:32Z

Description

There is three new API functions:
getPublicKey -- provides the public key of the currently authenticated account
encrypt -- allows to encrypt arbitrary data
decrypt -- allows to decrypt previously encrypted data

Motivation and Context

To securely exchange messages in the decentralized applications context some kind of encryption is necessary.

How Has This Been Tested?

Tested in client connected to nos-local with the help of slightly modified template dApp. Also there is new tests added.

Types of changes

Chore (tests, refactors, and fixes)
New feature (adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
I have read the CONTRIBUTING document.

mhuggins

Overall, this looks like a great addition. I also appreciate the thoroughness of the tests added, thank you for that!

I added a few comments that I'd like to have addressed before merging, I'm always open to discussion around any comments I left as well.

I'd like to verify some of the questions I left regarding passing a Buffer via IPC too. I'll circle back on that unless you beat me to it.

mhuggins · 2018-06-17T21:41:30Z

docs/api.md

+
+#### Parameters
+* `recipientPublicKey` **string** - The public key of the recipient account.
+* `data` **string** | **Buffer** - The data to encrypt.


This documentation should be restructured a bit to clarify that it's a single object argument, and that recipientPublicKey and data are keys on the object. i.e.:

* `config` **object** - The config options to perform this operation. * `config.recipientPublicKey` **string** - The public key of the recipient account. * `config.data` **string | Buffer** - The data to encrypt.

Another thing worth mentioning is that I'm not confident passing a Buffer data type will work due to the way IPC is implemented. Per the electron documentation:

Arguments will be serialized in JSON internally and hence no functions or prototype chain will be included.

There is a JSON representation of a Buffer, but I doubt that electron elegantly parses and converts it back to a Buffer on the other end.

const buffer = Buffer.from("foo"); // <Buffer 66 6f 6f> const serializedBuffer = JSON.stringify(buffer); // '{"type":"Buffer","data":[102,111,111]}' const deserializedBuffer = JSON.parse(serializedBuffer); // { type: 'Buffer', data: [ 102, 111, 111 ] }

I will try to verify if this is accurate, but something that will need to be verified before accepting as is.

fixed

I believe this is not a concern anymore as electron seems to support Buffers

mhuggins · 2018-06-17T21:47:01Z

docs/api.md

+The `encrypt` function allows you to encrypt arbitrary data for another user (you will need his public key, please see `getPublicKey`). It does not require the user to grant permission.
+
+#### Parameters
+* `recipientPublicKey` **string** - The public key of the recipient account.


Can we just call this publicKey? I think you're making an assumption that the data being serialized will be sent to another address, when it could just be for storing something pertaining to the current account's address.

I called it recipientPublicKey to clarify that it's not your public key that you need to pass to the function.
The way ECIES/ECDH work is that when encrypting I take my private key and your public key
and when decrypting you take your private key and my public key.
That way we'll get same shared secret without exchanging our private keys.

If you want to just encrypt some data for yourself you don't need shared secret or public key, you can just use hash of your private key as encryption key. Of course, then you will need private key for decryption too.

mhuggins · 2018-06-17T21:47:22Z

docs/api.md

+The `decrypt` function allows you to decrypt previously encrypted data for this user (you will need public key of the sender account, please see `getPublicKey`). It does not require the user to grant permission.
+
+#### Parameters
+* `senderPublicKey` **string** - The public key of the sender account.


Same here with calling this publicKey. The description for this field could be clarify which key by stating something like "the public key used to encrypt the data".

Answered above

mhuggins · 2018-06-17T21:48:33Z

docs/api.md

+* `senderPublicKey` **string** - The public key of the sender account.
+* `iv` **string** - The IV received during encryption.
+* `mac` **string** - The MAC received during encryption.
+* `data` **string** | **Buffer** - The data to decrypt.


Same concern here regarding Buffer.

I believe this is not a concern anymore as electron seems to support Buffers

mhuggins · 2018-06-17T21:48:54Z

docs/api.md

+* `data` **string** | **Buffer** - The data to decrypt.
+
+#### Returns
+**Buffer** - decrypted data


I don't believe we'll be able to return a Buffer for the same reason, but will verify. If not, then we should just make this a string.

I believe this is not a concern anymore as electron seems to support Buffers

mhuggins · 2018-06-17T21:52:14Z

src/browser/actions/makeDecryptActions.js

+}
+
+const decrypt = ({ senderPublicKey, wif, iv, mac, data }) => {
+  const ecdh = ECDH('prime256v1');


What factored into the decision to use AES-256-CBC above and prime256v1 here?

AES-256-CBC: no particular reason, it's good enough

prime256v1 = secp256r1: this is what used by NEO itself according to this: https://github.com/neo-project/neo/blob/master/neo/Wallets/KeyPair.cs#L26

mhuggins · 2018-06-17T21:52:54Z

src/browser/actions/makeDecryptActions.js

+  return Buffer.concat([firstChunk, secondChunk]);
+}
+
+const decrypt = ({ senderPublicKey, wif, iv, mac, data }) => {


I haven't taken a thorough look through the encryption/decryption logic yet. Adding a note here to ensure that happens before this PR is merged.

mhuggins · 2018-06-17T21:56:22Z

src/browser/actions/makeEncryptActions.js

+  return {
+    iv: iv.toString('hex'),
+    mac: mac.toString('hex'),
+    data: encrypted.toString('hex')


It looks like data is actually a string, not a Buffer. Is the documentation just incorrect?

documentation for result doesn't mention Buffer anywhere

mhuggins · 2018-06-17T22:02:14Z

__tests__/browser/components/RequestsProcessor/Decrypt/index.test.js

+    });
+  });
+
+  describe('when decrypted data has loaded', () => {


All these tests show the happy path, which is great. Can you add a test for when decryption fails to ensure that onReject is called and onResolve is not called?

mhuggins · 2018-06-17T22:02:44Z

__tests__/browser/components/RequestsProcessor/Encrypt/index.test.js

+    });
+  });
+
+  describe('when encrypted data has loaded', () => {


Same here, can a test be added for the unhappy path?

mhuggins · 2018-06-17T22:14:36Z

Did a little digging on the electron side, and buffers may actually be supported. I haven't tested yet. Also, I think the documentation you added for the return value is still wrong because it's returning a string, not a buffer.

trueinsider · 2018-06-17T23:49:48Z

I believe I've fixed all points of concern.
Also there is no Buffers in documentation for return values. But input parameters can take string or Buffer.

mhuggins · 2018-06-18T02:16:33Z

docs/api.md

+* `config.data` **string** | **Buffer** - The data to decrypt.
+
+#### Returns
+**Buffer** - decrypted data


@trueinsider This is the return value of type Buffer I was referring to.

oh, you right. but it actually returns Buffer.

inside decrypt():

return aes256CbcDecrypt(ivBuffer, encryptionKey, dataBuffer);

and inside aes256CbcDecrypt():

return Buffer.concat([firstChunk, secondChunk]);

it's encrypt() that returns hex-encoded string and documentation of encrypt result doesn't mention Buffers. maybe I should return Buffers here too.

trueinsider · 2018-06-18T20:49:32Z

I think maybe I should rename these functions to encryptFor/decryptFrom and then make another pair to just encrypt/decrypt stuff for myself using my private key
Or change API in a way that if no public key is provided in the args then encryption will be using only private key
What do you think?

…bitrary data

…on example

trueinsider · 2018-06-23T09:36:36Z

I was thinking, maybe I should do only getSharedSecret(publicKey) which will generate shared secret from provided public key and user's private key. That way we won't compromise private key and at the same time dApp developer could implement encryption/decryption that he wants/needs, not only hardcoded aes-256-cbc. And I can add encrypt/decrypt example in documentation on how to implement it using getSharedSecret() and aes-256-cbc.

And for encryption of user's own data I could implement getPrivateSecret() which will return sha512 hash of user's private key.

codecov · 2018-07-01T19:32:56Z

Codecov Report

Merging #241 into develop will increase coverage by 1.87%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #241      +/-   ##
===========================================
+ Coverage    53.44%   55.32%   +1.87%     
===========================================
  Files          207      220      +13     
  Lines         1783     1858      +75     
  Branches       249      250       +1     
===========================================
+ Hits           953     1028      +75     
  Misses         691      691              
  Partials       139      139

mhuggins · 2018-07-01T20:03:41Z

@trueinsider I'm communicating with the team regarding your last question here. I'll circle back with you and let you know what we settle on.

DalderupMaurice · 2018-07-05T07:46:11Z

Had a small discussing concluding that for the moment being - the additional feature is a too specific use-case. It's also no high risk of implementing it on a later stage, so we're happy to finalize this PR and potentially add in the additional feature later and keeping it simple to begin with. :)
cc FYI @mhuggins

…tions

mhuggins

I added a few commits at the end here as well as moving the documentation to nos/documentation via nos/documentation#10. I think this is good, but I want to check with @deanpress how he feels about merging this before/after having a security expert review it.

mhuggins · 2018-07-30T01:13:43Z

This change looks good to me, but I'm labeling with "don't merge" until we get a more in depth security review. At this point, no code changes are deemed needed prior to merging.

DalderupMaurice · 2018-09-15T09:55:16Z

no more 0.3.0? 🙈

mhuggins · 2018-09-15T13:08:49Z

@DalderupMaurice @deanpress since we're waiting on a review from @knaps, I can't guarantee it will happen before we're ready to release 0.3.0.

knaps · 2018-09-15T14:40:35Z

@mhuggins - I think you have the wrong person! I'm not a contributor to this project

mhuggins · 2018-09-15T14:43:40Z

Sorry about that!

DalderupMaurice · 2018-12-28T20:38:26Z

src/renderer/browser/components/RequestsProcessor/Decrypt/index.js

+
+    withData(authActions, mapAuthDataToProps),
+
+    withCall(decryptActions, ({ senderPublicKey, wif, iv, mac, data }) => ({


We currently use withInitialCall everywhere for the API, is this something we should address before merging? @mhuggins

That would potentially be safer here to ensure we don't inadvertently call the action twice (in case of a re-render for any reason).

src/renderer/browser/util/getRandomIV.js

DalderupMaurice · 2018-12-28T20:42:32Z

src/renderer/browser/util/getRandomIV.js

+import { randomBytes } from 'crypto';
+
+export default function getRandomIV() {
+  return randomBytes(16);


Would we maybe want the ability to choose the strength?
Having an optional parameter with 128 as default, and having return randomBytes(strength /8);

DalderupMaurice · 2018-12-29T09:17:49Z

src/renderer/browser/components/RequestsProcessor/Encrypt/index.js

+
+    withData(authActions, mapAuthDataToProps),
+
+    withCall(encryptActions, ({ recipientPublicKey, wif, data }) => ({


Suggested change

withCall(encryptActions, ({ recipientPublicKey, wif, data }) => ({

withInitialCall(encryptActions, ({ recipientPublicKey, wif, data }) => ({

DalderupMaurice · 2018-12-29T09:18:04Z

src/renderer/browser/components/RequestsProcessor/Encrypt/index.js

@@ -0,0 +1,34 @@
+import { withCall, withData } from 'spunky';


Suggested change

import { withCall, withData } from 'spunky';

import { withData } from 'spunky';

DalderupMaurice · 2018-12-29T09:19:01Z

src/renderer/browser/components/RequestsProcessor/Encrypt/index.js

+import { pick } from 'lodash';
+
+import authActions from 'login/actions/authActions';
+


Suggested change

import withInitialCall from 'shared/hocs/withInitialCall';

DalderupMaurice · 2018-12-29T09:19:33Z

src/renderer/browser/components/RequestsProcessor/Decrypt/index.js

+import { pick } from 'lodash';
+
+import authActions from 'login/actions/authActions';
+


Suggested change

import withInitialCall from 'shared/hocs/withInitialCall';

DalderupMaurice · 2018-12-29T09:19:56Z

src/renderer/browser/components/RequestsProcessor/GetPublicKey/index.js

+import { withCall, withData } from 'spunky';
+import { compose } from 'recompose';
+
+import authActions from 'login/actions/authActions';


Suggested change

import authActions from 'login/actions/authActions';

import authActions from 'login/actions/authActions';

import withInitialCall from 'shared/hocs/withInitialCall';

DalderupMaurice · 2018-12-29T09:20:33Z

src/renderer/browser/components/RequestsProcessor/GetPublicKey/index.js

+
+    withData(authActions, mapAuthDataToProps),
+
+    withCall(publicKeyActions, ({ wif }) => ({ wif })),


Suggested change

withCall(publicKeyActions, ({ wif }) => ({ wif })),

withInitialCall(publicKeyActions, ({ wif }) => ({ wif })),

DalderupMaurice · 2018-12-29T09:20:46Z

src/renderer/browser/components/RequestsProcessor/GetPublicKey/index.js

@@ -0,0 +1,23 @@
+import { withCall, withData } from 'spunky';


Suggested change

import { withCall, withData } from 'spunky';

import { withData } from 'spunky';

DalderupMaurice · 2018-12-29T09:21:31Z

src/renderer/browser/components/RequestsProcessor/Decrypt/index.js

+
+    withData(authActions, mapAuthDataToProps),
+
+    withCall(decryptActions, ({ senderPublicKey, wif, iv, mac, data }) => ({


Suggested change

withCall(decryptActions, ({ senderPublicKey, wif, iv, mac, data }) => ({

withInitialCall(decryptActions, ({ senderPublicKey, wif, iv, mac, data }) => ({

DalderupMaurice · 2018-12-29T09:21:45Z

src/renderer/browser/components/RequestsProcessor/Decrypt/index.js

@@ -0,0 +1,36 @@
+import { withCall, withData } from 'spunky';


Suggested change

import { withCall, withData } from 'spunky';

import { withData } from 'spunky';

DalderupMaurice · 2018-12-30T11:36:00Z

I'll take these changes in a followup PR

mhuggins self-assigned this Jun 17, 2018

mhuggins added the PR: needs review Pull request label Jun 17, 2018

mhuggins reviewed Jun 17, 2018

View reviewed changes

mhuggins added PR: reviewed w/ comments Reviewed but needs to be looked at and removed PR: needs review Pull request labels Jun 17, 2018

mhuggins reviewed Jun 18, 2018

View reviewed changes

jeroenptrs added this to the v0.3.0 milestone Jun 18, 2018

mhuggins added PR: needs review Pull request and removed PR: reviewed w/ comments Reviewed but needs to be looked at labels Jun 18, 2018

trueinsider and others added 11 commits June 21, 2018 02:19

feat(encrypt): add functions to allow encryption and decryption of ar…

8dea419

…bitrary data

fix typo

c269134

chore(tests): add tests for encryption functions

2f6c039

chore(docs): fix documentation structure and update full implementati…

6ea1157

…on example

chore(docs): fix getPublicKey link

e73d104

chore(docs): fix input parameters structure

9549597

chore(tests): added unhappy path for encryption tests

c13717d

remove trailing space

fa8dadd

chore(docs): fix input parameters names

c1a43ca

move files after rebase

cdd2276

Merge branch 'develop' into feat/encrypt

781fb94

trueinsider and others added 2 commits June 30, 2018 20:52

Merge branch 'develop' into feat/encrypt

41b8c89

Merge branch 'develop' into feat/encrypt

ca34786

mhuggins added 4 commits July 29, 2018 12:16

chore(encrypt): update syntax to match codebase

b502f0c

chore(encrypt): extract getPublicKey to testable util function

873e911

chore(encrypt): extract encrypt and getRandomIV to testable util func…

52d386b

…tions

chore(encrypt): extract decrypt to testable util function

b207a13

mhuggins mentioned this pull request Jul 29, 2018

feat(api): added encrypt, decrypt, and getPublicKey functions nos/documentation#10

Merged

6 tasks

mhuggins approved these changes Jul 29, 2018

View reviewed changes

mhuggins added PR: good to merge Reviewed and approved PR: don't merge This doesn't seem right and removed PR: needs review Pull request PR: good to merge Reviewed and approved labels Jul 29, 2018

mhuggins removed this from the v0.3.0 milestone Sep 8, 2018

Merge branch 'develop' into feat/encrypt

de290d4

DalderupMaurice reviewed Dec 28, 2018

View reviewed changes

src/renderer/browser/util/getRandomIV.js Show resolved Hide resolved

DalderupMaurice reviewed Dec 28, 2018

View reviewed changes

DalderupMaurice added PR: reviewed w/ comments Reviewed but needs to be looked at and removed PR: don't merge This doesn't seem right labels Dec 28, 2018

DalderupMaurice reviewed Dec 29, 2018

View reviewed changes

Maurice Dalderup added 2 commits December 29, 2018 10:23

Merge branch 'develop' into feat/encrypt

1f92efd

Merge branch 'develop' into feat/encrypt

e7ef136

DalderupMaurice merged commit 4939abf into nos:develop Dec 30, 2018

DalderupMaurice added PR: good to merge Reviewed and approved and removed PR: reviewed w/ comments Reviewed but needs to be looked at labels Dec 30, 2018

hal0x2328 mentioned this pull request Sep 15, 2019

Support sender decryption of messages encrypted to counterparty #1147

Open


		withData(authActions, mapAuthDataToProps),

		withCall(decryptActions, ({ senderPublicKey, wif, iv, mac, data }) => ({


		withData(authActions, mapAuthDataToProps),

		withCall(encryptActions, ({ recipientPublicKey, wif, data }) => ({

	withCall(encryptActions, ({ recipientPublicKey, wif, data }) => ({
	withInitialCall(encryptActions, ({ recipientPublicKey, wif, data }) => ({

		@@ -0,0 +1,34 @@
		import { withCall, withData } from 'spunky';

	import { withCall, withData } from 'spunky';
	import { withData } from 'spunky';

		import { pick } from 'lodash';

		import authActions from 'login/actions/authActions';

	import authActions from 'login/actions/authActions';
	import authActions from 'login/actions/authActions';
	import withInitialCall from 'shared/hocs/withInitialCall';


		withData(authActions, mapAuthDataToProps),

		withCall(publicKeyActions, ({ wif }) => ({ wif })),

	withCall(publicKeyActions, ({ wif }) => ({ wif })),
	withInitialCall(publicKeyActions, ({ wif }) => ({ wif })),

		@@ -0,0 +1,23 @@
		import { withCall, withData } from 'spunky';

	withCall(decryptActions, ({ senderPublicKey, wif, iv, mac, data }) => ({
	withInitialCall(decryptActions, ({ senderPublicKey, wif, iv, mac, data }) => ({

		@@ -0,0 +1,36 @@
		import { withCall, withData } from 'spunky';

feat(encrypt): add functions to allow encryption and decryption of arbitrary data #241

feat(encrypt): add functions to allow encryption and decryption of arbitrary data #241

Conversation

trueinsider commented Jun 17, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mhuggins left a 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

trueinsider Jun 17, 2018 • 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

mhuggins commented Jun 17, 2018

trueinsider commented Jun 17, 2018

Choose a reason for hiding this comment

trueinsider Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

trueinsider commented Jun 18, 2018 • edited Loading

trueinsider commented Jun 23, 2018 • edited Loading

codecov bot commented Jul 1, 2018 • edited Loading

Codecov Report

mhuggins commented Jul 1, 2018

DalderupMaurice commented Jul 5, 2018 • edited Loading

mhuggins left a comment

Choose a reason for hiding this comment

mhuggins commented Jul 30, 2018

DalderupMaurice commented Sep 15, 2018 • edited Loading

mhuggins commented Sep 15, 2018

knaps commented Sep 15, 2018

mhuggins commented Sep 15, 2018

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

DalderupMaurice commented Dec 30, 2018

trueinsider commented Jun 17, 2018 •

edited

Loading

trueinsider Jun 17, 2018 •

edited

Loading

trueinsider Jun 18, 2018 •

edited

Loading

trueinsider commented Jun 18, 2018 •

edited

Loading

trueinsider commented Jun 23, 2018 •

edited

Loading

codecov bot commented Jul 1, 2018 •

edited

Loading

DalderupMaurice commented Jul 5, 2018 •

edited

Loading

DalderupMaurice commented Sep 15, 2018 •

edited

Loading