Initial commit for vaults #4312

svyatonik · 2017-01-26T10:07:51Z

Initial commit for vaults

This is first part of implementation of #3501. Here's outline (and some caveats) of implementation:

vault are implemented sub-directories of key directory with name of directory = name of vault. This leads to the fact that we can't use special symbols in vaults name. So I've added restriction that vault name can consist of alphanumeric chars, whitespace chars, dash && underscore.
Another approach is to unlink name of vault from name of vault-dir && have some metadata file in keys directory, which will be of form [{ "vault_name": "MyVault", "vault_dir": "abcdef-123141-125421-12" }].
For me, the former is better, as it is more self-descriptive (it maybe more convenient for users who want to copy vaults) + we avoid some errors (like adding vaults with duplicate names => unable to work with both vaults at all, etc).
Anyway, this is discussable.
all accounts in the same vaults are protected with the same password. Here comes two caveats:
2.1) vault can be empty, but it still has to be protected with password. So here's vault.json file (aka vault_file.rs) in each vault, which contains encrypted password. Password is encrypted with the same password, so that decrypt(what: encrypted, with: password) = password. I'm not sure this is cryptographically strong, but I don't have another ideas how to achieve the same result.
2.2) some operations on accounts require no password (for example: changing name of account, changing meta). But this data is password-protected for vault accounts => password is stored in memory for all the time, when vault is open. I don't see another option currently. Maybe we should change API to support vault accounts? Or use some OS-backed protected memory (haven't found any cross-platform solutions for now) in future.
vaults exist as an addition to main key directory (aka RootDiskDirectory) && can be loaded/unloaded at any time
copying/moving accounts between vaults && root directory is possible

JSONRPC && Parity Wallet integration is yet to be done.

svyatonik · 2017-01-26T10:08:21Z

ethcore/src/account_provider/mod.rs

@@ -24,7 +24,8 @@ use std::fmt;
 use std::collections::HashMap;
 use std::time::{Instant, Duration};
 use util::RwLock;
-use ethstore::{SimpleSecretStore, SecretStore, Error as SSError, EthStore, EthMultiStore, random_string};
+use ethstore::{SimpleSecretStore, SecretStore, Error as SSError, EthStore, EthMultiStore


Updated this file just to make it work with new ethstore API. Its own API will be updated in next PR[s] (about RPCs).

svyatonik · 2017-01-26T10:08:47Z

ethstore/src/dir/vault.rs

+			.expect("last component of path is checked in make_vault_dir_path; it contains only valid unicode characters; to_str fails when file_name is not valid unicode; qed")
+	}
+
+	fn set_key(&self, key: VaultKey, new_key: VaultKey) -> Result<(), SetKeyError> {


As there's no way to make atomic cross-platform swap of two directories, here's my attempt to minimize potential losses:

create temp vault

copy all accounts to the temp vault [with same file names]

move all files from temp vault to self [with replace] - if crash/shutdown occurs here, directory is partially moved => user can finish this move.
I also thought on some making some '.dirty' file when vault is in inconsistent state && then automatically continue moving on next opening.
But I think it is too much && it could corrupt vault totally.

svyatonik · 2017-01-26T10:38:47Z

build is broken with message:

ERROR: Build failed: execution took longer than 3600 seconds

but all tests I have written are in ethstore crate && it has passed the check

gavofyork · 2017-01-26T19:56:07Z

For me, the former is better.

agree.

2.1) vault can be empty, but it still has to be protected with password. So here's vault.json file (aka vault_file.rs) in each vault, which contains encrypted password. Password is encrypted with the same password, so that decrypt(what: encrypted, with: password) = password. I'm not sure this is cryptographically strong, but I don't have another ideas how to achieve the same result.

not sure what this is about. there should be no additional "vault file" and an empty vault is simply an empty directory. encrypted content is stored (as hex) in the JSON key file under the field metacrypto.

2.2) ...password is stored in memory for all the time, when vault is open...

that's fine.

svyatonik · 2017-01-27T01:31:41Z

@gavofyork
2.1) ok - imagine that next APIs are called:
createVault("myVault", "myPassword")
closeVault("myVault")
openVault("myVault", "incorrectPassword")
how should I check the password in 3rd call if password from 1st call was just lost (no accounts were created)?

svyatonik · 2017-01-27T01:56:49Z

Options that I've rejected:

open empty vault with any password, but this is confusing, as on previous call I've set up the password && I expect it will be the same on next calls, until I change it;
we could just say that empty vaults must not exist. But then vaults API is inconsistent - why should I call parity_newVault at all if actuall call for vault creation is parity_changeVault("myAddress", "myVault")? We can just add an vault arg to createAccount/removeAccount && remove parity_newVault then - it seems logical.

svyatonik · 2017-01-27T02:34:19Z

Option 3 - is to remove password argument from parity_newVault, as there's actually no difference between 'empty directory' and 'directory with wrong files'. It will contradict with 'all JSON files in the same path will have the same vault password' requirement, but...Next call sequence is possible:

parity_newVault("myVault"); // no open actually occurs
parity_openVault("myVault", "password1");
parity_changeVault("myAddress1", "myVault");
parity_closeVault("myVault");
parity_openVault("myVault", "password2"); // another password, but that's fine - "myAddress1" will be invisible
parity_changeVault("myAddress2", "myVault");
// here if we call list_accounts(), we will see 'myAddress2' only, as 'myAddress1' is protected with another password
parity_closeVault("myVault");

I.e. vault is still referenced by name, but it's unique key is name + password.

gavofyork · 2017-01-27T09:49:31Z

closeVault closes the vault, deleting it if empty;
openVault opens the vault, creating if none yet exists.

Alternative which persists empty vaults is to place a CSRNGed token encrypted with the vault key in the path, which I guess is similar to your original solution (e.g. a single file containing 2x 32 bytes one is the plain text and the other is encrypted). This is considered secure (it's known in crypto speak as a MAC). I'm fairly ambivalent.

svyatonik · 2017-01-27T10:01:22Z

I would prefer to leave it as is then, because otherwise (as I told above) this would be a total mess just because vault could then contain keys, encrypted with different passwords (part of these will be ignored when opened with pass1, another part - when opened with pass2). + vault currently is a filesystem-entity (dir) && I can't delete it when there are some other files, which I do not manage (like keys, encryptd with different password).

gavofyork · 2017-01-27T10:04:26Z

how could it get into a state where keys are encrypted with different vault passwords? (vault password could only "change" when the vault is empty/non-existent)

svyatonik · 2017-01-27T10:13:48Z

createVault("vault1", "password1");
createAccountInVault("vault1", "myAddress1");
createVault("vault2", "password2");
createAccountInVault("vault2", "myAddress2");
fs::rename("vault1/myAddress1", "vault2/myAddress1"); // why not :)
...
openVault("vault2", "password1"); // myAddress1 is visible
openVault("vault2", "password2"); // myAddress2 is visible

In my impl this is impossible, as I could only open vault with the password passed to createVault. && myAddress1 will always be unavailable.
Now imagine that I have removed file, which stands for myAddress2. In your impl, vault is empty && I can delete it, but the folder itself is not empty && I do not known - whether this information is important for user, or it is some garbage left => vault will not be removed

gavofyork · 2017-01-27T10:24:41Z

// why not :)

because it breaks things? :-)

if we're up against a user who is happy to messing up our file structure then they only have themselves to blame for any ensuing confusion.

that said, i'm fairly ambivalent; if you prefer to mac approach, go for it.

svyatonik · 2017-01-27T10:28:45Z

But it is allowed - and most of users do not know how it works internally :) I could want to restore keys from backup && just select wrong directory for restore. Or restore keys, which have been made using old password. There are too many options to make such a mistake.

gavofyork · 2017-01-27T10:33:57Z

if a user restores a database backup but places files in different paths (or mixes old files with new), the database will break and the user will have only them self to blame. this is a type of database, so i don't see all that much difference.

rphmeier · 2017-01-27T19:39:31Z

ethcore/src/account_provider/mod.rs

@@ -24,7 +24,8 @@ use std::fmt;
 use std::collections::HashMap;
 use std::time::{Instant, Duration};
 use util::RwLock;
-use ethstore::{SimpleSecretStore, SecretStore, Error as SSError, EthStore, EthMultiStore, random_string};
+use ethstore::{SimpleSecretStore, SecretStore, Error as SSError, EthStore, EthMultiStore
+	, random_string, SecretVaultRef, StoreAccountRef};


nit: comma on newline?

rphmeier · 2017-01-27T19:40:43Z

ethstore/src/account/crypto.rs

+use account::{Cipher, Kdf, Aes128Ctr, Pbkdf2, Prf};
+
+#[derive(Debug, PartialEq, Clone)]
+pub struct Crypto {


I just take this code && moved to separate file, as it has grown a bit :) Also the whole ethstore crate is undocumented - just thought it is not a good idea to document 1/100 of undocumented code :) Anyway - I've tried to add docs for this struct && others I've added. But I wonder - is there some policy on documentation? I mean - if mod makes something pub - it should be documented? Even if this is some technical mod, unavailable to all others, but its parent?

rphmeier · 2017-01-27T19:44:03Z

ethstore/src/dir/disk.rs

 }

+impl DiskKeyFileManager {
+	pub fn new() -> Self {
+		DiskKeyFileManager {}


braces unnecessary

actually, new is unnecessary, it's less work to write let _ = DiskKeyFileManager than let _ = DiskKeyFileManager::new()

rphmeier · 2017-01-27T19:45:48Z

ethstore/src/dir/mod.rs

+}
+
+impl SetKeyError {
+	pub fn fatal(err: Error) -> Self {


why have these functions? the variants are public. just seems like more code to maintain.

rphmeier · 2017-01-27T19:51:34Z

lack of documentation on public items is a problem.

gavofyork · 2017-01-28T10:24:38Z

could do with merging master so the tests don't fail

gavofyork · 2017-01-28T10:31:59Z

@svyatonik assuming you're going for the mac approach, the most cryptographically secure means i can think of is to encrypt the hash of the password with a KDF of the salted password, so file contains two 32-byte arrays:

salt, enc(input: sha3(password), key: kdf(password ++ salt))

for the wallet files within this path, the secret for their metadata should be similarly derived (though the format of crypto JSON field should already include all needed info).

svyatonik · 2017-01-28T11:01:20Z

@gavofyork ok - thanks for the advice. I'll update implementation => in progress

svyatonik · 2017-01-30T06:59:29Z

done - now vault.json will contain original password hash, encrypted with derived key + salt

gavofyork · 2017-01-30T10:00:17Z

conflicts...

svyatonik added 2 commits January 26, 2017 12:13

initial commit for vaults

4259abb

fixed TODO

b2472c6

svyatonik commented Jan 26, 2017

View reviewed changes

svyatonik added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 26, 2017

rphmeier reviewed Jan 27, 2017

View reviewed changes

rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 27, 2017

public docs

b527db8

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 28, 2017

svyatonik added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 28, 2017

svyatonik added 2 commits January 28, 2017 14:03

Merge branch 'master' into vaults

13b35cb

vault_file.json now contains enc(pwd hash)

f51b180

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 30, 2017

removed doc

ae37a59

gavofyork added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 30, 2017

Merge branch 'master' into vaults

d900b76

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Jan 30, 2017

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 30, 2017

NikVolf merged commit 9ac4d83 into master Jan 30, 2017

NikVolf deleted the vaults branch January 30, 2017 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit for vaults #4312

Initial commit for vaults #4312

svyatonik commented Jan 26, 2017

svyatonik Jan 26, 2017

svyatonik Jan 26, 2017

svyatonik commented Jan 26, 2017

gavofyork commented Jan 26, 2017 •

edited

svyatonik commented Jan 27, 2017

svyatonik commented Jan 27, 2017

svyatonik commented Jan 27, 2017

gavofyork commented Jan 27, 2017 •

edited

svyatonik commented Jan 27, 2017 •

edited

gavofyork commented Jan 27, 2017 •

edited

svyatonik commented Jan 27, 2017

gavofyork commented Jan 27, 2017 •

edited

svyatonik commented Jan 27, 2017

gavofyork commented Jan 27, 2017 •

edited

rphmeier Jan 27, 2017

svyatonik Jan 28, 2017

rphmeier Jan 27, 2017

svyatonik Jan 28, 2017

rphmeier Jan 27, 2017

rphmeier Jan 27, 2017

svyatonik Jan 28, 2017

rphmeier Jan 27, 2017

svyatonik Jan 28, 2017

rphmeier commented Jan 27, 2017

gavofyork commented Jan 28, 2017

gavofyork commented Jan 28, 2017

svyatonik commented Jan 28, 2017

svyatonik commented Jan 30, 2017

gavofyork commented Jan 30, 2017

Initial commit for vaults #4312

Initial commit for vaults #4312

Conversation

svyatonik commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svyatonik commented Jan 26, 2017

gavofyork commented Jan 26, 2017 • edited

svyatonik commented Jan 27, 2017

svyatonik commented Jan 27, 2017

svyatonik commented Jan 27, 2017

gavofyork commented Jan 27, 2017 • edited

svyatonik commented Jan 27, 2017 • edited

gavofyork commented Jan 27, 2017 • edited

svyatonik commented Jan 27, 2017

gavofyork commented Jan 27, 2017 • edited

svyatonik commented Jan 27, 2017

gavofyork commented Jan 27, 2017 • edited

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

rphmeier commented Jan 27, 2017

gavofyork commented Jan 28, 2017

gavofyork commented Jan 28, 2017

svyatonik commented Jan 28, 2017

svyatonik commented Jan 30, 2017

gavofyork commented Jan 30, 2017

gavofyork commented Jan 26, 2017 •

edited

gavofyork commented Jan 27, 2017 •

edited

svyatonik commented Jan 27, 2017 •

edited

gavofyork commented Jan 27, 2017 •

edited

gavofyork commented Jan 27, 2017 •

edited

gavofyork commented Jan 27, 2017 •

edited