-
Notifications
You must be signed in to change notification settings - Fork 282
Conversation
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.
ACK.
Tested login, 2nd pwd, wallet creation and BIP 38.
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.
❤️
src/wallet-crypto.js
Outdated
|
||
salt = sjcl.codec.hex.toBits(salt.toString('hex')); | ||
var stretched = sjcl.misc.pbkdf2(password, salt, iterations, keylen || 256, hmacSHA1); | ||
assert(typeof password === 'string', 'password string required'); |
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.
probably not important, but the previous assert failed on empty string while the new one doesn't
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.
Nice, will check for that. Thanks!
src/wallet-crypto.js
Outdated
var stretched = sjcl.misc.pbkdf2(password, salt, iterations, keylen || 256, hmacSHA1); | ||
assert(typeof password === 'string', 'password string required'); | ||
assert(typeof iterations === 'number' && !isNaN(iterations), 'iterations number required'); | ||
assert(keyLenBits == null || keyLenBits % 8 === 0, 'key length must be evenly divisible into bytes'); |
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.
successfully fails on keyLenBits === Infinity and -Infinity :)
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.
untested but looks good to me. nice improvement on stricter asserts and better var naming. would be nice one day to add a unit test or two more for stretchPassword using more typical values for the wallet to ensure consistency between sjcl and crypto-browserify, but I bet all sorts of obvious stuff would break in the wallet if they weren't the same.
This removes SJCL by replacing it with the latest version of crypto-browserify's pbkdf2 implementation. It's ~1.4x slower, but that's acceptable.
I'd like to add the option to encrypt / decrypt asynchronously as well, since that will be ~10x faster than SJCL was and not block the UI. But with the async approach there's more that could potentially go wrong, so will leave that out for now.