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

Enforce 256 bit keys on raw import, support raw mobile imports #14502

Merged
merged 2 commits into from May 24, 2017

Conversation

Projects
None yet
5 participants
@karalabe
Member

karalabe commented May 23, 2017

Fixes #14481

The first commit changes our crypto API a bit so importing raw ECDSA keys are enforced to be 32 bytes in size not only for hex strings, but for raw byte slices too. The second commit surfaces importing raw keys into the mobile keystore (hence why it was important to enforce the correct bytes, to prevent users importing hex strings accidentally).

karalabe added some commits May 23, 2017

@karalabe karalabe requested review from fjl, holiman and obscuren May 23, 2017

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe
Member

karalabe commented May 23, 2017

@ligi PTAL

@karalabe karalabe changed the title from Enforce 26 bit keys on raw import, support raw mobile imports to Enforce 32 bit keys on raw import, support raw mobile imports May 23, 2017

@karalabe karalabe changed the title from Enforce 32 bit keys on raw import, support raw mobile imports to Enforce 256 bit keys on raw import, support raw mobile imports May 23, 2017

@karalabe karalabe added this to the 1.6.2 milestone May 23, 2017

@ligi

This comment has been minimized.

Show comment
Hide comment
@ligi

ligi May 23, 2017

Member

YAY - LGTM - Thanks!

Member

ligi commented May 23, 2017

YAY - LGTM - Thanks!

@ligi

This comment has been minimized.

Show comment
Hide comment
@ligi

ligi May 23, 2017

Member

After thinking about it a bit more - perhaps also exporting should be done to get symmetric API-wise - that said for exporting I think I will in the end use JSON for making paper-wallets with WALLETH - just opened #14481 as I saw raw keys used for paper-wallets often - so I need it for importing - but otherwise I like the context that the JSON gives ..

Member

ligi commented May 23, 2017

After thinking about it a bit more - perhaps also exporting should be done to get symmetric API-wise - that said for exporting I think I will in the end use JSON for making paper-wallets with WALLETH - just opened #14481 as I saw raw keys used for paper-wallets often - so I need it for importing - but otherwise I like the context that the JSON gives ..

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe May 24, 2017

Member

I don't think we want to support that use case tbh. Exporting unencrypted raw private keys has more dangers that benefits imho. The whole use case of exporting keys is to allow moving them to a different service/device, which is covered by the json format.

Member

karalabe commented May 24, 2017

I don't think we want to support that use case tbh. Exporting unencrypted raw private keys has more dangers that benefits imho. The whole use case of exporting keys is to allow moving them to a different service/device, which is covered by the json format.

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe May 24, 2017

Member

Regarding generating paper wallets, I can accept to put raw keys on paper wallets, but those should be done when generating the account and not later (which is a security risk if your mobile is stolen). Similarly to how hardware wallets do the recovery mnemonics.

Member

karalabe commented May 24, 2017

Regarding generating paper wallets, I can accept to put raw keys on paper wallets, but those should be done when generating the account and not later (which is a security risk if your mobile is stolen). Similarly to how hardware wallets do the recovery mnemonics.

@holiman

looks good to me

@fjl

fjl approved these changes May 24, 2017

Let's see what breaks

@fjl fjl merged commit ef25b82 into ethereum:master May 24, 2017

3 checks passed

commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast May 30, 2017

Contributor

No regression tests?

Contributor

chfast commented May 30, 2017

No regression tests?

@karalabe

This comment has been minimized.

Show comment
Hide comment
@karalabe

karalabe May 31, 2017

Member

@chfast Please provide a bit more detail :)

Member

karalabe commented May 31, 2017

@chfast Please provide a bit more detail :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment