Derive encryption key in dashboard using Argon2 #424
Conversation
| * @param salt the salt that is used for key derivation. | ||
| * @returns a hex encoded string of the derived key. | ||
| */ | ||
| export function deriveEncryptionKey(password: string, salt: string): string { |
There was a problem hiding this comment.
Is this really deriving or more precisely calculating?
There was a problem hiding this comment.
Well... this function is just a wrapper around PBKDF2 (Password-Based Key Derivation Function 2). So I assume this is deriving?
| return crypto | ||
| .PBKDF2(password, saltWordArray, { | ||
| keySize: 256 / 32, | ||
| iterations: 5000, |
There was a problem hiding this comment.
Is 5000 iterations like normal, or your approach, where does this originate, what does it mean? a comment might be a good fit for these settings
| @@ -292,12 +381,36 @@ export class PersistenceManager { | |||
| if (bundles.length > 0) { | |||
| try { | |||
| this.nodecg.log.info("Attempting to automatically login..."); | |||
| const loadResult = await this.load(password); | |||
|
|
|||
There was a problem hiding this comment.
This code duplication with crypto.ts is required because there is no way to serve the same functionality in the extension and the dashboard? Then I would propose to at least add a note to minimize breaking this in evolution (which could probably mess up configs...)
|
Because of the problems with this PBKDF2 implementation (see b504cc5) I'm thinking about exploring argon2 for this. It has a WebAssembly implementation available and thus has not the performance problems that running PBKDF2 in JS has. Additionally argon2 seems to be more secure. If we're changing the config layout we might as well do it future-proof. However this is currently only a idea and I will need to explore that possibility and decide whether or not we should do it. I still should add at least one unit test for migration. It has always worked for my various configs but I don't want to have the migration suddenly break in future nodecg-io versions. Additionally I should write the security assumptions that nodecg-io makes into the docs before merging this PR. I hopefully should have some spare time in about three weeks to do all this, handle some maintenance tasks like removing the broken curseforge service and release 0.3.0. |
hlxid
force-pushed
the
feat/encrypt-using-password-hash
branch
from
fb47e57 to
f93a6fe
Compare
hlxid
changed the title
The latest official release argon2-browser doesn't support node.js v18+ and also has some other caveats. Because of this I've used my fork of it in 74099d4 with some patches. Instead of using my patched version this commit uses hash-wasm which also includes a implementation of argon2, works correctly with node.js v18 and works fine in the browser as well.
hlxid
force-pushed
the
feat/encrypt-using-password-hash
branch
from
4f70456 to
ef05ddd
Compare
hlxid
force-pushed
the
feat/encrypt-using-password-hash
branch
from
ef05ddd to
40ac791
Compare
|
This PR is finally ready to be merged. @sebinside could you please re-review this? |
See #408 (comment).
After doing some research I found out that a key derivation function like PBKDF2 is probably better suited for this than a plain hashing algorithm.
CryptoJS actually already does this when a password is provided to the
AES.encryptfunction.We now just do the key derivation ourselves in the dashboard, only transmit the key, and pass the key to CryptoJS.
Because we're now ourselves managing the encryption key we also need to manage the salt and initialization vector.
All old configurations will be automatically migrated on the first login.
Todo list:
Any feedback is highly appreciated.