Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace deprecated crypto.createDecipher() and crypto.createCipher() Node.js methods #76

Merged
merged 9 commits into from Jun 20, 2019

Conversation

popod
Copy link
Contributor

@popod popod commented Jun 16, 2019

Fix sindresorhus/electron-store#67

It is better than #72 in this way:

  • It doesn't break existing encrypted data
  • The data encryption is secure and could be used for security purposes

@sindresorhus
Copy link
Owner

It doesn't break existing encrypted data

Can you explain how it manages that?

The data encryption is secure and could be used for security purposes

That's meaningless as the decryption key can be easily found in the JS code of the app.

@popod
Copy link
Contributor Author

popod commented Jun 16, 2019

  1. It doesn't break existing encrypted data

In the other PR, already existed data (encrypted with crypto.createCipher()) cannot be decrypted and will be lost after the sindresorhus/conf update: this is a breaking change!
With this PR, encrypted data contain the IV vector: data are stored like this: <the IV vector>:<the config data>. By this way, we can know (in the deciphering part) if the data had been encrypted with crypto.createCipher() (no IV vector is stored with the data) or crypto.createCipherIV().

  1. The data encryption is secure and could be used for security purposes

Yes, but this is no more because of sindresorhus/conf. Now the security is assured by this package (random IV, strong data encryption). This is now the developer of the application which shoud take care of the password security if it want to use this package for security purposes. If the key used to decrypt the data is stored with keytar or is a password entered by the user (stored in a variable by example), this is secure.

To resume, this PR resolve the depreciation by a way that is transparent for the user and without break existing implementations.

Is it more understandable? ;)

@popod
Copy link
Contributor Author

popod commented Jun 18, 2019

Hi @sindresorhus,

Is my proposal okey for you ? Do you require some changes or additional clarification ?

Thanks

@sindresorhus
Copy link
Owner

Yes, makes sense. I think indeed this is the best solution. Can you add a test that ensures a config encrypted with the existing version will be correctly decrypted with this version? Just commit a small config fixture encrypted with the current version (in master).

@popod
Copy link
Contributor Author

popod commented Jun 19, 2019

@sindresorhus done.

Do you think we should update this in readme.md according with my previous message ?

Note that this is not intended for security purposes, since the encryption key would be easily found inside a plain-text Node.js app.

@sindresorhus
Copy link
Owner

Sure

@popod
Copy link
Contributor Author

popod commented Jun 19, 2019

@sindresorhus is all okey for you now ?

test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Filename test/encrypted_config.json => test/config-encrypted-with-conf-4-1-0.json

@sindresorhus sindresorhus changed the title Replace deprecated crypto.createDecipher() and crypto.createCipher() Replace deprecated crypto.createDecipher() and crypto.createCipher() Jun 20, 2019
index.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Replace deprecated crypto.createDecipher() and crypto.createCipher() Replace deprecated crypto.createDecipher() and crypto.createCipher() Node.js methods Jun 20, 2019
@sindresorhus sindresorhus merged commit 177fe65 into sindresorhus:master Jun 20, 2019
@sindresorhus
Copy link
Owner

A new release is out. Can you add this to electron-store too in form of a dependency bump and readme updates?

https://github.com/sindresorhus/conf/releases/tag/v5.0.0

@popod
Copy link
Contributor Author

popod commented Jun 20, 2019

Yes, I will do it. Thanks for reviews and merge ;)

Edit: done in sindresorhus/electron-store#72.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace deprecated crypto.createDecipher usage with crypto.createDecipheriv
2 participants