Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

Make encrypting and decrypting progressive/async with streams #5

Merged
merged 5 commits into from
Oct 24, 2016

Conversation

stovmascript
Copy link
Contributor

@stovmascript stovmascript commented Oct 21, 2016

Hey, this addition would make the ciphering process progressive.

We are investigating a performance issue (blocked UI thread) in our react-native app and thought that redux-persist-transform-encrypt was the culprit. In the end we think it turns out not to be the case. In the process, I've made this. I think it should be an improvement over the synchronous encrypt/decrypt CryptoJS.AES methods. Especially if you have a large state object, like we have in our app. Although at the moment, I have no way of actually testing the performance...

I don't know what your code style preference is, there is a class and some arrow functions, which only work with node >=4.

I would be happy to work on this some more and especially find out if there actually is a performance increase. The key thing would probably be knowing how redux-persist handles individual transforms. Can they be async/streams?

Also, I'm no SSL expert, so don't know the strength of the cipher used the derive the initialization vector for the encryptor. But I would suspect the strength of the secretKey is more important?

Note: Works in react-native with rn-nodeify.

@stovmascript
Copy link
Contributor Author

Whoops, gonna have a look at that.

@maxdeviant
Copy link
Owner

@stovmascript Thanks for putting this together.

However, I think I would want to make this an option as opposed to the default behavior.

So if you could move ProgressiveCryptor to its own file, and then add an option for progressive/async then I think I will merge this.

@rt2zz
Copy link

rt2zz commented Oct 23, 2016

👍 this is an awesome feature! definitely agree it should live behind an option, or possibly as a separate export. Separate export might be nice because it will make dead code elimination easier for packagers.

@stovmascript
Copy link
Contributor Author

I've added an export, so that if you want progressive ciphering, you can import createProgressiveEncryptor instead of the default one.

@maxdeviant Would you prefer an option into the createEncryptor config instead?

Also, as you might have noticed, the test for the async way was failing. I've wrapped the stream in a promise that will resolve with the completed string when it's ready. But still the encryption test passes and decryption fails. I think the encryption is passing only due to the test state object being really small and it manages to resolve before expect() starts working.

@rt2zz The createTransform in() and out() methods of redux-persist might not be ready for this async execution. The test calls them one right after the other, so when it starts decrypting, the encrypted state isn't ready yet. I'm wondering if some sort of callback in these methods might do the trick?

@maxdeviant
Copy link
Owner

@stovmascript If we have a separate export I don't think we need a config option 😄

Also, you can mark the test as pending to get Travis to go green and then I will merge. Then just open a separate issue to fix the progressive test.

@maxdeviant maxdeviant merged commit 3e56f66 into maxdeviant:master Oct 24, 2016
@maxdeviant maxdeviant mentioned this pull request Oct 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants