-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize salt, iv, uuid params of .toV3() before encrypting #95
Normalize salt, iv, uuid params of .toV3() before encrypting #95
Conversation
@michaelsbradleyjr thanks for submitting this PR! It looks good. Just a few questions:
Thanks! |
@alcuadrado sorry, I didn't see your reply earlier. The closest thing to a spec I could find is this: https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition.
See: https://github.com/ethereum/go-ethereum/blob/master/accounts/keystore/testdata/v3_test_vector.json Now, the While I don't think it should hold up this PR, I do think it would be a good idea to do a bit more options checking for |
I've reviewed this PR and think it's correct. We should implement these changes after #93. Thanks @michaelsbradleyjr for taking the time to investigate and fixing this! |
@michaelsbradleyjr this fall into this heavy typescript transition work, sorry for the inconveniences, can you update the PR here on top? |
@holgerd77 I'll give it a shot. TS isn't my expertise, but I'll see what I can do. |
@michaelsbradleyjr Thanks, should be relatively straight-forward and nothing really deeply TS-specific. |
@michaelsbradleyjr We would like to do a follow-up release after this has been merged, please let us know if you won't make it to update here in the next 3-5 days, then we might want to find another solution. Thanks! |
@michaelsbradleyjr @alcuadrado Just had a longer look at this since I was planning to manually do the changes within a new PR and then ask for permission from @michaelsbradleyjr to merge. TypeScript is actually now enforcing this to be a buffer (see here), so this should be implicitly fixed. Generally this is a good thing and I think this library will really hugely profit from the TypeScript transition. On the same time this makes me feel uncomfortable since this is some kind of implicit API change (strings now being disallowed), and I wonder how many other such implicit changes/corrections are in with the TypeScript merge, eventually @the-jackalope can also give some additional comment here. When I looked at the docs in README I realized that this library is very seriously under-documented, you can't even explicitly find out about the types accepted for parameters without looking at the code. I think we shouldn't do this release without also having the docs automated. This is generally super important, that people can easily look up the current API and accepted types and especially have some guidance for people doing this specific version update to the TypeScript release. @alcuadrado do you eventually have some time/capacity to do the TSDoc integration here? Cheers |
I think you are right. @michaelsbradleyjr can you confirm this? I'm not that familiar with this lib.
Sure, I'll do it later this week. |
@alcuadrado Great, thanks! 😄 |
I'll get it done today, the bug is still there but it's subtle. It's still possible for folks using JS instead of TS to pass a string for the salt, iv, etc. The inputs need to be normalized into buffers before being passed to scrypt. I started work on the changes to the sources and the tests last week but didn't have time to finish. |
Progress is here: master...michaelsbradleyjr:fix/TS-REWRITE-tov3-salt-buffer See L239 in Bonus round: implement guards for The reason for pulling in the Some of these improvements need to be adapted into PRs for the |
👍 |
09786cc
to
fb7bde9
Compare
fb7bde9
to
ea99042
Compare
@alcuadrado @holgerd77 my refactor is finished. As I mentioned previously, TypeScript isn't one of my strengths so I'm very open to suggestions re: improving what I changed and authored. Note: the tests take longer to run now because they're doing more encryption and decryption work. |
Looks like Node v6.x would need a polyfill for |
@michaelsbradleyjr that's great! Yes, we've dropped Node 6 support some time ago, you can just remove it from Travis. |
@s1na since this is falling very much into this area of type conversion, hex checking and the like you've been working on along the util library updates, can you do a review here? |
ea99042
to
7904ac8
Compare
@holgerd77 okay, I removed Node v6.x from Travis — all checks now passing. I also made a few small tweaks I thought of later last night. |
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.
Thanks for the PR and the extensive tests. Made some minor comments here and there. I didn't go through the tests in detail.
As I've mentioned in other issues/PRs, I think using strings to represent binary data has its challenges and I'd rather we restrict the API to Buffer. So what I'd to would be to throw if e.g. salt is not a buffer. But I understand that's not very user-friendly...So if everyone else is aboard with these changes, I'm okay too.
I also noticed you're testing this library's outputs against ethers
outputs. While I have lots of respect for ethers
, it might hide a bug in both implementations.
Some note for future: The types have gotten quite complex and there are a lot of duplication. If something changes in one type, it's easy to forget modifying another one... I think it's in need of some refactoring/simplification.
return { | ||
cipher: params.cipher || 'aes-128-ctr', |
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 can be simplified as: return { ...params, ...v3Defaults }
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.
Done, but it needed to be: return { ...v3Defaults, ...(params as V3ParamsStrict) }
in order for caller supplied params to replace the defaults.
@@ -361,6 +443,7 @@ export default class Wallet { | |||
|
|||
// public instance methods | |||
|
|||
// tslint:disable-next-line |
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.
Why is it necessary to ignore tslint errors here and in getPublicKey()
?
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.
Without disabling those lines for tslint I get the following when doing npm run tslint
:
ERROR: /Users/michael/repos/ethereumjs-wallet/src/index.ts:439:10 - Refactor this getter so that it actually refers to the property 'privateKey'
ERROR: /Users/michael/repos/ethereumjs-wallet/src/index.ts:448:10 - Refactor this getter so that it actually refers to the property 'publicKey'
I felt that refactoring that code from the TS rewrite was beyond the scope of this PR, especially since the README documents those get...
methods as part of the API. So I chose to stop the linter from failing on those lines.
|
||
let kdfParams: PBKDFParams | ScryptKDFParams | ||
let kdfParams: KDFParams | ||
let derivedKey: Buffer | ||
switch (v3Params.kdf) { | ||
case KDFFunctions.PBKDF: |
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.
I couldn't comment on the code below, but now that KDFParams.salt
is a buffer and not converted to string, kdfparams.salt
could be passed to pbkdf2Sync
and scryptsy
instead of v3Params
.
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 catch, I've made the change.
@@ -38,17 +83,38 @@ function mergeToV3ParamsWithDefaults(params?: Partial<V3Params>): V3Params { | |||
if (!params) { | |||
return v3Defaults | |||
} | |||
|
|||
if (typeof params.salt === 'string') { | |||
params.salt = Buffer.from(validateHexString('salt', params.salt), 'hex') |
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.
I just noticed that in ethereumjs-util
we pad the string to even length, so that if implicitly e.g. a 0x01
byte was converted to 0x1
, it would be covered:
Not sure whether that applies here.
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.
It's a possibility, for sure, though I wonder if it would end up masking legit user errors more than providing shorthand/convenience.
src/index.ts
Outdated
if (str.toLowerCase().startsWith('0x')) { | ||
str = str.slice(2) | ||
} | ||
if (!/^[0-9a-f]{2}([0-9a-f]{2})*$/i.test(str)) { |
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 is implicitly testing for length >= 2.
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.
Right: ff
or 0xff
would work but not f
or 0xf
. But I think I'm missing the point you have in mind, can you explain further?
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.
Just that if validateHexString
is called without a length argument (e.g. for salt
), inputs with length < 2 will still be rejected. I don't expect anyone to provide a salt
with length < 2, so it should be fine.
uuid: Buffer.from(uuid, 'hex'), | ||
} | ||
|
||
const makePermutations = (...objs: Array<object>): Array<object> => { |
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.
Can you add some comments to clarify this part?
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.
Comments added, let me know if additional clarity is needed.
@s1na I've added a commit that can be squashed prior to merge, so it's easier to see the changes made. One additional change I made is slicing away leading Regarding use of ethers. You're right, it's not a bullet-proof solution; but combined with testing random wallets in addition to fixtures it offers some reasonable assurance that encrypted wallets are portable across different implementations. It's already borne some fruit: ethers-io/ethers.js#582 (comment). Also, making use of ethers in the test suite of web3 was what originally tipped me off to the problem with the string/buffer parameters and I realized ethereumjs-wallet had the same problem. |
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
b575950
to
693ee8b
Compare
Previously, if `salt`, `iv` and/or `uuid` options were supplied as strings to `.toV3()` they would be passed to `pbkdf2Sync`/`scrypt` as strings. That could result in errors during encryption. Also, during decryption these options were always converted to Buffer instances such that supplying strings during encryption could result in output that could not be decrypted. This commit fixes the inconsistencies, guards against bad inputs, and also makes encrypted output match up with the output of other wallet libraries, e.g. `ethers`, whenever the equivalent encryption options are used consistently across libraries.
693ee8b
to
debbd4f
Compare
The PR review commit has been squashed, so I think this one's ready to go. @holgerd77 @alcuadrado |
Build was failing here, have restarted to see if it was something temporary. |
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.
Tests are passing now, reapprove here after the commit squashing.
Ok, would follow up with a |
For the record: forgot that we wanted to do the docs automation (see thread above), will postpone the release for a few days until we are through with that. |
Previously, if
salt
,iv
and/oruuid
options were supplied as strings to.toV3()
they would be passed topbkdf2Sync
/scrypt
as strings. That could result in errors during encryption. Also, during decryption these options were always converted to Buffer instances such that supplying strings during encryption could result in output that could not be decrypted.This commit fixes the inconsistencies, guards against bad inputs, and also makes encrypted output match up with the output of other wallet libraries, e.g.
ethers
, whenever the equivalent encryption options are used consistently across libraries.The problem was discovered when working on #2938 of web3. The original idea was to remove the
scrypt
package as a strict dependency: for Node 12 compatibility (owing to ABI changes) and as part of an effort to remove modules that require compilation, i.e. require dev tools to be installed. That involved swapping inethers
forethereumjs-wallet
as a dev dependency.ethers
is a distinct implementation when it comes to the to/from v3 code, unlikeweb3
andethereumjs-wallet
which share much of the same logic. I couldn't get the json outputs to match unless no custom scrypt options were provided when encrypting on the web3 side, and finally I realized what was going wrong with the options when they're strings.