feat(security): RN-1303: Update password storage to use argon2 #5872
Conversation
| `); | ||
| const users = await db.runSql('SELECT id, password_hash_old, password_salt FROM user_account'); | ||
|
|
||
| await Promise.all( |
There was a problem hiding this comment.
There are about 4,400 user_account records in the database so this set of updates take 5 - 6 minutes (on my local) . I'm not sure if there is a precedent for long migrations like this or if it is a problem?!
There was a problem hiding this comment.
I had a chat about this with @rohan-bes and we agreed that it's fine. Our rule of thumb is
- Sub 10 mins is totally fine
- 10+ worth having a discussion with the team but generally safe
- 1 hour + probably prefer to do OTS during scheduled downtime
tcaiger
changed the title
| @@ -66,7 +66,7 @@ const upsertApiClient = async ({ | |||
| password: string; | |||
| salt: string; | |||
| }) => { | |||
| const secretKeyHash = encryptPassword(password, salt); | |||
| const secretKeyHash = await encryptPassword(password, salt); | |||
There was a problem hiding this comment.
When the api clients start up, the api client hashes in the database are re-created so we don't need to manually migrate them. meditrak_app client is an exception to this since it does't have an orchestration server but since we know the password for it I am going to manually migrate its hash as a release step.
| @@ -53,8 +53,9 @@ export async function changePassword(req, res, next) { | |||
| throw new FormValidationError(error.message, ['password', 'passwordConfirm']); | |||
| } | |||
|
|
|||
| const passwordAndSalt = await hashAndSaltPassword(passwordParam); | |||
There was a problem hiding this comment.
I like this, it's nice to break this out as it was a bit opaque before.
Could even do something like:
| const passwordAndSalt = await hashAndSaltPassword(passwordParam); | |
| const { password, salt } = await hashAndSaltPassword(passwordParam); | |
| await models.user.updateById(userId, { | |
| password, | |
| salt, | |
| }); |
| const verified = await verifyPassword( | ||
| secretKey, | ||
| process.env.API_CLIENT_SALT, | ||
| apiClient.secret_key_hash, | ||
| ); | ||
| return verified ? apiClient?.getUser() : undefined; |
There was a problem hiding this comment.
This could throw an error if no apiClient is found?
| const verified = await verifyPassword( | |
| secretKey, | |
| process.env.API_CLIENT_SALT, | |
| apiClient.secret_key_hash, | |
| ); | |
| return verified ? apiClient?.getUser() : undefined; | |
| if (!apiClient) { | |
| return undefined; | |
| } | |
| const verified = await verifyPassword( | |
| secretKey, | |
| process.env.API_CLIENT_SALT, | |
| apiClient.secret_key_hash, | |
| ); | |
| return verified ? apiClient.getUser() : undefined; |
| * Attempts to verify the password using argon2, if that fails, it tries to verify the password | ||
| * using sha256 plus argon2. If the password is verified using sha256, the password is moved to | ||
| * argon2. |
There was a problem hiding this comment.
Thanks for the comments 🙏
Issue #: feat(security): RN-1303: Update password storage
Changes: