-
Notifications
You must be signed in to change notification settings - Fork 971
eth-wallet: password flow based on the hash stored on the fs #15027
Conversation
app/ethWallet-geth.js
Outdated
|
||
ipcMain.on('eth-wallet-store-password-hash', (e, str) => { | ||
bcrypt.genSalt(10, (err, salt) => { | ||
if (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case we are checking for if (err)
, let's go ahead and output it via console.error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
535e858
to
e16edc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine by me, let's have the sec team take a look
app/ethWallet-geth.js
Outdated
}) | ||
|
||
ipcMain.on('eth-wallet-store-password-hash', (e, str) => { | ||
bcrypt.genSalt(10, (err, salt) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 is kind of a small factor. did you benchmark this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping it to 14, on my macbook it take 1.320s
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good except the bcrypt rounds seems kind of small
e16edc8
to
919d900
Compare
eth-wallet: password flow based on the hash stored on the fs
eth-wallet: password flow based on the hash stored on the fs
Versioning of the eth-wallet app is difficult so this update includes: