Pyethereum keystore support #9710

tworec · 2018-10-05T22:54:06Z

Continuation of #9444
(sorry for duplicate, but I can not reopen #9444)

parity-cla-bot · 2018-10-05T22:54:08Z

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

debris

Thanks for contribution! There are several blockers for this pull request. Most importantly badly designed KeyFileManager trait

ethkey/cli/src/main.rs

ethstore/cli/src/main.rs

ethstore/src/account/safe_account.rs

ethstore/src/accounts_dir/disk.rs

ethstore/src/json/key_file.rs

debris · 2018-10-10T10:14:05Z

ethstore/src/account/safe_account.rs

+		let crypto = Crypto::from(json.crypto);
+		let address = match json.address {
+			Some(address) => address.into(),
+			None => match password {


what if the json file contains address and we provided a password? imo, this should result in an Error

Again, I would silently ignore this. As for now all errors reading kestores are silently ignored, which is confusing me too, but to be consistent I propose to ignore it.

I sustain to ignore this case. There can be many key files in src dir, and just some of them need passwd.. rest should be ignored, even if rest is all.

ethstore/src/account/kdf.rs

ethstore/src/json/bytes.rs

ethstore/src/json/kdf.rs

5chdn · 2018-10-26T11:36:54Z

Are you still working on this @tworec

tworec · 2018-11-26T13:03:52Z

I want to finish this. I was looking to see @debris answers...

answered

Co-Authored-By: tworec <[email protected]>

5chdn · 2018-12-04T10:32:23Z

👍 needs a 2nd review

niklasad1

Hey,

Can you please revert the changes eprintln! to ẁarn! but inorder to get the logger working for the cli you must enable it which it is not.

The easiest is to use env_logger

You need the following:

# ethstore/cli/Cargo.toml
env_logger = "0.5"

# ethstore/cli/src/main.rs
extern crate env_logger;
env_logger::try_init().expect("Logger initialized only once.");

tworec · 2018-12-19T11:12:23Z

@niklasad1 done, but I've initialized log level to warn also. It will enable logs for invalid password.

This is sample CLI output with invalid password

parity-ethereum/ethstore/cli pyethereum-keystore-support (0282536a4)$ cargo run import pwd.txx --src ~/ethstore/
   Compiling ethstore-cli v0.1.1 (/Users/tworec/git/parity-ethereum/ethstore/cli)                                                                                                    
    Finished dev [unoptimized + debuginfo] target(s) in 3.30s                                                                                                                        
     Running `/Users/tworec/git/parity-ethereum/target/debug/ethstore import pwd.txx --src /Users/tworec/ethstore/`
 WARN 2018-12-19T11:05:15Z: ethstore::accounts_dir::disk: Invalid key file: "/Users/tworec/ethstore/keystore-mainnet-backup.json" (Invalid password)
 WARN 2018-12-19T11:05:15Z: ethstore::accounts_dir::disk: Invalid key file: "/Users/tworec/ethstore/mainnet_keystore.json" (Invalid password)

5chdn · 2018-12-28T13:10:33Z

@tworec could you rebase this?

@niklasad1 could you review this again?

tworec · 2018-12-28T14:50:29Z

@5chdn I think rebase is overkill here. I've merged instead.

5chdn · 2018-12-28T14:54:45Z

yes, whatever you prefer :)

tworec · 2018-12-28T16:03:46Z

So why do we need a review? Merge introduced no changes in this PR

5chdn · 2019-01-02T09:50:07Z

yes. one more review. please wait till this holiday craze is over.

tworec · 2019-01-02T09:56:34Z

ok

…

On Wed, 2 Jan 2019 at 10:50, Afri Schoedon ***@***.***> wrote: yes. one more review. please wait till this holiday craze is over. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9710 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMIYUW4O4D5r9oH2PF_YNrYD8mJPNaHgks5u_IDagaJpZM4XLCJZ> .

tomusdrw

Looks decent, I have couple of grumbles regarding code pollution.

Since the password is required to import I think we should treat python directory as a vault if that's possible.

tomusdrw · 2019-01-02T09:57:52Z

accounts/ethstore/cli/src/main.rs

@@ -146,30 +148,34 @@ impl fmt::Display for Error {

 fn main() {
 	panic_hook::set_abort();
+	if env::var("RUST_LOG").is_err() {
+		env::set_var("RUST_LOG", "warn")


Why is this needed? Wouldn't it default to info anyway (which imho seems better than warn)

nope, see https://docs.rs/env_logger/0.5.0/env_logger/#enabling-logging

by default all logging is disabled except for error!

And I want to see this warning:
warn!("Invalid key file: {:?} ({})", path, err);

see discussion above

tomusdrw · 2019-01-02T09:58:09Z

accounts/ethstore/cli/src/main.rs


 	match execute(env::args()) {
 		Ok(result) => println!("{}", result),
 		Err(Error::Docopt(ref e)) => e.exit(),
 		Err(err) => {
-			println!("{}", err);
+			eprintln!("{}", err);


maybe use error! since the logger is already initialized.

maybe, but see this #9710 (comment) by @debris

guys pls, can we just proceed and not nitpicking about error messages?

tomusdrw · 2019-01-02T10:01:15Z

accounts/ethstore/src/accounts_dir/disk.rs


 impl RootDiskDirectory {
 	pub fn create<P>(path: P) -> Result<Self, Error> where P: AsRef<Path> {
 		fs::create_dir_all(&path)?;
 		Ok(Self::at(path))
 	}

+	/// allows to read keyfiles with given password (needed for keyfiles w/o address)
+	pub fn with_password(&self, password: Option<Password>) -> Self {


I don't think it's a good idea to implement that here. Reasons:

The assumption is that all accounts inside the directory share the same password which is most likely incorrect.

It's used only for python keyfiles import, but will polute (already convoluted) codebase

Can we not use DiskDirectory for python keyfiles import and just allow importing particular account files?
Alternatively can't we treat python keyfiles as a vault (they share the same password actually) and use structs from this code part?

ad1. I agree, it is not very generic, but as a user I just create special dir for my keyfiles (even if its single one) before import.

ad 2. it is used for all keyfiles w/o address. Not only pyethereum ones. As any new feature it introduces some noice.

Can we not use DiskDirectory for python keyfiles import and just allow importing particular account files?

I've tried to be consistent with current approach which is importing whole directories. Introduction of new import-single in CLI seems overkill to me.

Alternatively can't we treat python keyfiles as a vault (they share the same password actually) and use structs from this code part?

I've analyzed vault's, but it does not fit here. You need to repack your keyfiles into valuts, and in order to repack you need to import it first, right?

ad1. if your keyfiles have different pwds you will see warnings, but import will succeed with at least this file with pwd you've entered. You can reexecute import with another pwd to import others.

see example session here #9710 (comment)

We have import-wallet, but it seems it only works for presale wallets currently. I'd be more inclined to handle it there than to hack around DiskKeyFileManager (since the former command allows importing directly to a vault, etc).

That said I'm fine with this if you address the SafeAccount semantics in a way I proposed in the other comment.

tomusdrw · 2019-01-02T10:02:20Z

accounts/ethstore/src/account/safe_account.rs

+	pub fn from_file_with_password(json: json::KeyFile, filename: Option<String>, password: &Option<Password>) -> Result<Self, Error> {
+		let crypto = Crypto::from(json.crypto);
+		let address = match json.address {
+			Some(address) => address.into(),


imho if the password is provided we should validate that the adress is correct. Maybe switch the order here: first check if we have password and after deriving the address optionally compare it with the one in json.

nice feature. It will need to throw an error when addresses do not match, but in next comment you're proposing to revert Result.

tomusdrw · 2019-01-02T10:05:40Z

accounts/ethstore/src/account/safe_account.rs

@@ -77,16 +77,32 @@ impl SafeAccount {
 	/// Create a new `SafeAccount` from the given `json`; if it was read from a
 	/// file, the `filename` should be `Some` name. If it is as yet anonymous, then it
 	/// can be left `None`.
-	pub fn from_file(json: json::KeyFile, filename: Option<String>) -> Self {
-		SafeAccount {
+	pub fn from_file(json: json::KeyFile, filename: Option<String>) -> Result<Self, Error> {


This now errors for no good reason, afaict it will never fail. Can we please get the proper return type back?

it will fail for example when the password is incorrect.

Ok, I went through the code and propose a following change:

/// Create a new `SafeAccount` from the given `json`; if it was read from a /// file, the `filename` should be `Some` name. If it is as yet anonymous, then it /// can be left `None`. /// In case `password` is provided, we will attempt to read the secret from the keyfile and derive the address from it instead of reading it directly. /// Providing password is required for `json::KeyFile`s with no address. pub fn from_file(json: json::KeyFile, filename: Option<String>, password: Option<String>) -> Resutl<Self, Error> { let crypto = Crypto::from(json.crypto); let address = match (password, json.address) { (None, None) => Err("This keystore does not contain address. You need to provide password to import it".into())? (None, Some(address)) => address, (Some(password), address) => { let derived_address = KeyPair::from_secret(crypto.secret(&password).map_err(|_| Error::InvalidPassword)?)?.address(); match address { Some(address) if address != derived_address => { warn!("Detected address mismatch when opening an account. Derived: {}, got: {}", derived_address, address); }, _ => {}, } derived_address } }; Ok(SafeAccount { id: json.id.into(), version: json.version.into(), address, crypto, filename, name: json.name.unwrap_or(String::new()), meta: json.meta.unwrap_or("{}".to_owned()), }) }

So that all callers are required to explicitly set password to None and be aware of the failure (in case json::KeyFile does not contain an address).
(We have 3 occurences of that code)

What do you think?

tomusdrw · 2019-01-02T10:06:40Z

accounts/ethstore/src/account/safe_account.rs

+	}
+
+	/// Same as [`from_file`](#method.from_file), but with `password` which enables reading keystore files w/o address.
+	pub fn from_file_with_password(json: json::KeyFile, filename: Option<String>, password: &Option<Password>) -> Result<Self, Error> {


I would make password mandatory here (see above comment). The decision which function to use and what errors to handle can be left to the caller.

sure, it can be done, but for price of code repetition, because I use it to implement from_file

tomusdrw

Happy with the responses, proposed just one change to SafeAccount so that the caller knows under what circumstances he can expect failures.

tomusdrw · 2019-01-02T12:58:24Z

accounts/ethstore/src/account/safe_account.rs

@@ -77,16 +77,32 @@ impl SafeAccount {
 	/// Create a new `SafeAccount` from the given `json`; if it was read from a
 	/// file, the `filename` should be `Some` name. If it is as yet anonymous, then it
 	/// can be left `None`.
-	pub fn from_file(json: json::KeyFile, filename: Option<String>) -> Self {
-		SafeAccount {
+	pub fn from_file(json: json::KeyFile, filename: Option<String>) -> Result<Self, Error> {


Ok, I went through the code and propose a following change:

/// Create a new `SafeAccount` from the given `json`; if it was read from a /// file, the `filename` should be `Some` name. If it is as yet anonymous, then it /// can be left `None`. /// In case `password` is provided, we will attempt to read the secret from the keyfile and derive the address from it instead of reading it directly. /// Providing password is required for `json::KeyFile`s with no address. pub fn from_file(json: json::KeyFile, filename: Option<String>, password: Option<String>) -> Resutl<Self, Error> { let crypto = Crypto::from(json.crypto); let address = match (password, json.address) { (None, None) => Err("This keystore does not contain address. You need to provide password to import it".into())? (None, Some(address)) => address, (Some(password), address) => { let derived_address = KeyPair::from_secret(crypto.secret(&password).map_err(|_| Error::InvalidPassword)?)?.address(); match address { Some(address) if address != derived_address => { warn!("Detected address mismatch when opening an account. Derived: {}, got: {}", derived_address, address); }, _ => {}, } derived_address } }; Ok(SafeAccount { id: json.id.into(), version: json.version.into(), address, crypto, filename, name: json.name.unwrap_or(String::new()), meta: json.meta.unwrap_or("{}".to_owned()), }) }

So that all callers are required to explicitly set password to None and be aware of the failure (in case json::KeyFile does not contain an address).
(We have 3 occurences of that code)

What do you think?

tomusdrw · 2019-01-02T13:15:08Z

accounts/ethstore/src/accounts_dir/disk.rs


 impl RootDiskDirectory {
 	pub fn create<P>(path: P) -> Result<Self, Error> where P: AsRef<Path> {
 		fs::create_dir_all(&path)?;
 		Ok(Self::at(path))
 	}

+	/// allows to read keyfiles with given password (needed for keyfiles w/o address)
+	pub fn with_password(&self, password: Option<Password>) -> Self {


We have import-wallet, but it seems it only works for presale wallets currently. I'd be more inclined to handle it there than to hack around DiskKeyFileManager (since the former command allows importing directly to a vault, etc).

That said I'm fine with this if you address the SafeAccount semantics in a way I proposed in the other comment.

tworec · 2019-01-02T14:57:52Z

@tomusdrw you're suggestion is incorporated.
@niklasad1 can you pls approve?

tworec · 2019-01-03T11:23:44Z

pushing merge to retrigger builds (tests are passing for me)

niklasad1 · 2019-01-03T11:24:14Z

@tworec Thank you very much sorry for delayed response/review.

I don't know really understand why the tests fail seems unrelated to this PR (stratum)

Perhaps merge/rebase will fix it?

/cc @tomusdrw any idea?

tworec added 9 commits October 5, 2018 21:17

support for keystore format produced by pyethereum lib + some debug msgs

9c860d6

made salt unbound also for Scrypt

5e7e9ab

made address optional and skip if its none

cff6f23

ignore values of any type, not just i32

012dd5b

WIP: local deps

09d5a66

cleanup

b86fcb0

enable optional address + more cleanup

e9f2d11

yet another cleanup

b74936b

enable keystore wo address import (using passwd)

216fcd3

tworec added 2 commits October 6, 2018 00:57

cleanup

1ba0de3

doc enchancement

0f31116

5chdn added this to the 2.2 milestone Oct 8, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 8, 2018

parity/account fix

93f782b

debris previously requested changes Oct 10, 2018

View reviewed changes

debris added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 10, 2018

5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 26, 2018

5chdn modified the milestones: 2.2, 2.3 Oct 29, 2018

5chdn closed this Nov 25, 2018

5chdn reopened this Nov 27, 2018

niklasad1 and others added 4 commits December 3, 2018 22:32

remove unnecessary borrowing within match str

1919bd5

Co-Authored-By: tworec <[email protected]>

remove another unnecessary borrowing

1c6cdbe

Co-Authored-By: tworec <[email protected]>

default derived instead of implementing

69b4958

log dependency removed

0282536

niklasad1 suggested changes Dec 15, 2018

View reviewed changes

[ethstore] warn restored + env_logger initialized in CLI

b42f277

Merge branch 'master' into pyethereum-keystore-support

8736654

tomusdrw suggested changes Jan 2, 2019

View reviewed changes

tomusdrw approved these changes Jan 2, 2019

View reviewed changes

applied suggestion from @tomusdrw

53d4d84

niklasad1 approved these changes Jan 3, 2019

View reviewed changes

niklasad1 added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jan 3, 2019

Merge branch 'master' into pyethereum-keystore-support

97e0405

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Jan 3, 2019

niklasad1 merged commit 469f9c2 into openethereum:master Jan 3, 2019

tworec deleted the pyethereum-keystore-support branch January 3, 2019 15:21

tworec mentioned this pull request Jan 31, 2019

parity dependencies refresh golemfactory/golem-unlimited#122

Merged

Pyethereum keystore support #9710

Pyethereum keystore support #9710

Conversation

tworec commented Oct 5, 2018 • edited Loading

parity-cla-bot commented Oct 5, 2018

debris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5chdn commented Oct 26, 2018

tworec commented Nov 26, 2018

5chdn commented Dec 4, 2018

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

tworec commented Dec 19, 2018

5chdn commented Dec 28, 2018

tworec commented Dec 28, 2018

5chdn commented Dec 28, 2018

tworec commented Dec 28, 2018

5chdn commented Jan 2, 2019

tworec commented Jan 2, 2019 via email

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tworec Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tworec Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tworec commented Jan 2, 2019

tworec commented Jan 3, 2019

niklasad1 commented Jan 3, 2019

tworec commented Oct 5, 2018 •

edited

Loading

niklasad1 left a comment •

edited

Loading

tworec Jan 2, 2019 •

edited

Loading

tworec Jan 2, 2019 •

edited

Loading