Vaults RPCs #4366

svyatonik · 2017-01-31T09:39:14Z

This is second part of work on vaults (https://github.com/ethcore/parity/issues/3501).
Outline: new RPCs are introduced + meta for 'vault accounts' is populated with vault field.
I have changed some RPCs + have couple of concerns:

parity_changeVaultPassword(name, new) changed to parity_changeVaultPassword(name, old, new). To me it is strange that, for example, copying account, signing message, etc. require account password && this action does not. So anyone with access to RPC could create vault && move all available accounts there, even if he had no access to these accounts => get full access to the account. Please insist, if you think that's ok - I'll change back to proposed prototype.
parity_changeVault(address, newVault) changed to parity_changeVault(address, newVault, oldPassword, newPassword) because of same reasons as above. Again - this is just my proposal && it can be reverted to original prototype.
the issue stated that there's some parity_getAddressMeta RPC method. I haven't found one on master - so most probably it is in development, or will be created in future. So I'm adding vault field to account' meta field at the lowest level possible (in VaultDiskDirectory)
the same account can exist in two vaults. The simplest way to reproduce is to cp vault1 vault2. In parity_changeVault there's no argument, which is responsible for 'origin vault' => currently I've selected approach similar to the one, selected in EthMultiStore - choose the first matching account (matching here means that address && password are the same). Maybe there should be another way to deal with this? Another argument for parity_changeVault or move all matching accounts? Please ping me, if you think I've selected wrong way.

gavofyork · 2017-01-31T11:57:26Z

1 & 2. so the attack case you're suggesting is Alice opens up her vault V, Bob gains access to the signer RPCs, creates a new vault W with a randomised password and moves all of Alice's keys from V into W?
this would cause Alice some annoyance as she would lose knowledge of the keys' metadata. however, her keys' secrets would still be as accessible as they were; Bob wouldn't gain "full access" to them.
furthermore, these RPCs are part of the restricted set and are known to be sensitive and able to cause grief if misused. as such they are only available to trusted components like the Signer/Parity Wallet and if Bob has access to these RPCs, he can cause substantial grief anyway.
the issue with forcing the RPC to require the vault password is that introduce a temptation for RPC clients (like authors of the UI) cache a plain-text password client-side to avoid unnecessary user interaction. plain-text caching in languages & REs not-designed for security would expose the password to a greater attack surface and could reasonably lead to an overall reduction in security.

this is a much greater argument for (2) than (1), however since (2) is ultimately reducible to multiple (1)s, it makes little sense to require the password for (2) and not (1).

in fact the RPC i was thinking of was parity_getAccountsInfo, which returns the metadata. there are corresponding parity_setAccountMeta in ParityAccounts RPC.
perhaps just alter any call which can change/create a new to changeVault, createAccount, ... to return an error if the name already exists. can be a separate PR though.

svyatonik · 2017-01-31T12:36:24Z

@gavofyork
1 & 2. Yes - that's exactly scenario, that I was thinking about. But I still do not understand - why

...she would lose knowledge of the keys' metadata. however, her keys' secrets would still be as accessible as they were...

Currently both secret and meta for vault accounts are encrypted with the same password (which is also vault password && it is the same for all accounts from this vault) => if Bob will change password for Alice' vault or move accounts from Alice' vault to Bob's vault, it will get "full access" to both the secret and meta. Maybe I'm wrong && missing something? Could you please explain?
Also - at the beginning I was thinking about that every account in the root directory is actually a small vault (which contains single account). And change_password() for the single account is basically the same that change_vault_password(). But the former requires old password && the latter does not. It seems strange.
Or do you mean that vault accounts remain 'unlocked' for all the time vault is opened? I.e. when sign is called for the vault account, UI won't ask the password (because we have already opened the vault with password) && pass password = empty string && server-side will ignore passed password && will use its own cached password to sign the message? If that's true, then I get it wrong. I'll update the PR then.
3. Cool - I'll add a test for it
4. But in my scenario it is not created, it is copied. Imagine another, a little bit more adequate:

importGethKeys(); // address1 is imported
createNewVault("vault1");
moveToVault("address1", "vault1");
closeVault("vault1"); // here we forget that address1 ever exists
importGethKeys(); // address1 is imported again, as it is not yet in keys directory
createNewVault("vault2");
moveToVault("address1", "vault2");
openVault("vault1"); // we have opened 2nd vault with address1 => there are 2 opened vaults, both contain address1-account

svyatonik · 2017-01-31T13:05:37Z

OK - looks like I have an issue here:

Currently both secret and meta for vault accounts are encrypted with the same password (which is also vault password && it is the same for all accounts from this vault)

Meta must be encrypted with vault password && every account still will have its own password (to encrypt secret). Then everything seems logical. I'll update PR

svyatonik · 2017-02-01T08:57:01Z

Changes outline:

vault password is used to encrypt vault accounts' meta. Every vault account still have its own password to protect secret (most of chnages)
added test for (3) above
postponed solution of (4) above by adding account_ref() methof
reverted RPC method prototypes to proposed in original issue

tomusdrw

Grubmles about RPC namespace.

tomusdrw · 2017-02-01T09:03:35Z

rpc/src/v1/traits/parity.rs

+
+		/// Create new vault.
+		#[rpc(name = "parity_newVault")]
+		fn create_vault(&self, String, String) -> Result<bool, Error>;


I think those methods should be part of parity_accounts namespace (so that they are not expoed to dapps by default)

done, thanks for clarification

tomusdrw · 2017-02-01T09:05:45Z

ethstore/src/ethstore.rs

+	fn account_ref(&self, address: &Address) -> Result<StoreAccountRef, Error> {
+		self.reload_accounts()?;
+		self.cache.read().keys()
+			.filter(|r| &r.address == address)


find() instead of filter.nth(0)?

gavofyork · 2017-02-04T09:04:19Z

we probably want an rpc to list vaults; either by logging vaults or deriving from the directory structure. can be a different PR.

gavofyork

looks good in general but i'd like to see more comprehensive test coverage to ensure all operations behave correctly with respect to all others.

gavofyork · 2017-02-04T09:01:55Z

rpc/src/v1/tests/mocked/parity_accounts.rs

+	let request = r#"{"jsonrpc": "2.0", "method": "parity_changeVaultPassword", "params":["vault1", "password2"], "id": 1}"#;
+	let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#;
+
+	assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned()));


should test that you can open with new password

gavofyork · 2017-02-04T09:02:32Z

rpc/src/v1/tests/mocked/parity_accounts.rs

+	let request = format!(r#"{{"jsonrpc": "2.0", "method": "parity_changeVault", "params":["0x{}", "vault1"], "id": 1}}"#, address.hex());
+	let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#;
+
+	assert_eq!(tester.io.handle_request_sync(&request), Some(response.to_owned()));


should test that it no longer appears when old vault open and does appear when new vault open.

svyatonik · 2017-02-04T15:47:07Z

@gavofyork

OK - I'll update PR with RPC for listing vaults
there are a lot of tests for vaults (and of course for changing passwords, moving accounts to/from vaults) in ethstore crate here:
https://github.com/ethcore/parity/blob/master/ethstore/src/ethstore.rs
https://github.com/ethcore/parity/blob/master/ethstore/src/dir/vault.rs
I thought that RPC tests are more about correctness of parameters, serialization/deserialization, etc. In vaults case RPCs level is just about passing parameters to ethstore.
=> I don't think that duplicating same tests here same tests here will help with anything. Or maybe it worth moving tests from ethstore to here?

svyatonik · 2017-02-05T07:32:45Z

done
this is tested on lower level, but if you insist - I'll duplicate or move ethstore tests here

gavofyork · 2017-02-05T15:17:49Z

ok - missed those additional tests. should be good.

vaults RPCs

f4aeaaf

svyatonik added the A0-pleasereview 🤓 Pull request needs code review. label Jan 31, 2017

gavofyork added M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. labels Jan 31, 2017

svyatonik closed this Jan 31, 2017

vault.password != vault_account.password

f00c3d8

svyatonik reopened this Feb 1, 2017

tomusdrw suggested changes Feb 1, 2017

View reviewed changes

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

moved vault RPCs to parityAccounts NS

fdae7b5

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 Feb 1, 2017

gavofyork suggested changes Feb 4, 2017

View reviewed changes

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

svyatonik added 2 commits February 5, 2017 10:08

parity_listVaults + parity_listOpenedVaults

0868750

Merge branch 'master' into vaults_rpcs

ef1539f

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 Feb 5, 2017

gavofyork merged commit 2f340a5 into master Feb 5, 2017

gavofyork deleted the vaults_rpcs branch February 5, 2017 15:17

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

This was referenced Feb 8, 2017

Vaults #3501

Closed

Deal with duplicated accounts in ethstore #4533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vaults RPCs #4366

Vaults RPCs #4366

svyatonik commented Jan 31, 2017

gavofyork commented Jan 31, 2017

svyatonik commented Jan 31, 2017

svyatonik commented Jan 31, 2017

svyatonik commented Feb 1, 2017 •

edited

Loading

tomusdrw left a comment

tomusdrw Feb 1, 2017

svyatonik Feb 1, 2017

tomusdrw Feb 1, 2017

svyatonik Feb 1, 2017

gavofyork commented Feb 4, 2017 •

edited

Loading

gavofyork left a comment

gavofyork Feb 4, 2017

gavofyork Feb 4, 2017

svyatonik commented Feb 4, 2017

svyatonik commented Feb 5, 2017

gavofyork commented Feb 5, 2017

Vaults RPCs #4366

Vaults RPCs #4366

Conversation

svyatonik commented Jan 31, 2017

gavofyork commented Jan 31, 2017

svyatonik commented Jan 31, 2017

svyatonik commented Jan 31, 2017

svyatonik commented Feb 1, 2017 • edited Loading

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Feb 1, 2017

Choose a reason for hiding this comment

svyatonik Feb 1, 2017

Choose a reason for hiding this comment

tomusdrw Feb 1, 2017

Choose a reason for hiding this comment

svyatonik Feb 1, 2017

Choose a reason for hiding this comment

gavofyork commented Feb 4, 2017 • edited Loading

gavofyork left a comment

Choose a reason for hiding this comment

gavofyork Feb 4, 2017

Choose a reason for hiding this comment

gavofyork Feb 4, 2017

Choose a reason for hiding this comment

svyatonik commented Feb 4, 2017

svyatonik commented Feb 5, 2017

gavofyork commented Feb 5, 2017

svyatonik commented Feb 1, 2017 •

edited

Loading

gavofyork commented Feb 4, 2017 •

edited

Loading