nodecg-io icon indicating copy to clipboard operation
nodecg-io copied to clipboard

Derive encryption key in dashboard using Argon2

Open hlxid opened this issue 4 years ago • 2 comments

See https://github.com/codeoverflow-org/nodecg-io/pull/408#issuecomment-1006896276.

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.encrypt function. 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:

  • [x] First implementation
  • [x] Code cleanup and documentation
  • [x] Fix and update tests

Any feedback is highly appreciated.

hlxid avatar Jan 09 '22 21:01 hlxid

Because of the problems with this PBKDF2 implementation (see https://github.com/codeoverflow-org/nodecg-io/commit/b504cc5a9439c6f6e052976664dc64738f842562) 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 avatar Jul 24 '22 20:07 hlxid

This PR is finally ready to be merged. @sebinside could you please re-review this?

hlxid avatar Oct 12 '22 09:10 hlxid