Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Signing transactions with rotating token #3691

Merged
merged 15 commits into from
Dec 15, 2016
Merged

Signing transactions with rotating token #3691

merged 15 commits into from
Dec 15, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Nov 30, 2016 •
edited
Loading

  1. Introduce new methods to AccountsProvider (sign_with_token)
  2. First token = Account password
  3. The account is copied to temporary store and encrypted with new randomly-generated token.
  4. Next request can use returned token to sign transaction instead of using original password.

TODO:

  • Finish implementing RPC method
  • Nicer handling of set_name/set_meta updates in EthStore
  • Tests for EthStore
  • Timeouts for items in transient_sstore (token is valid only for some specified time; logged here: Rotating tokens timeouts #3758 )

@tomusdrw tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. labels Nov 30, 2016
Tomasz Drwięga added 5 commits November 30, 2016 16:41
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 9, 2016
rphmeier
rphmeier reviewed Dec 9, 2016
View reviewed changes
ethcore/src/account_provider/mod.rs
let new_token = random_string(16);
let signature = if is_std_password {
// Insert to transient store
try!(self.sstore.copy_account(&self.transient_sstore, &account, &token, &new_token));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what will happen here when the account is already present in the transient store?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transient store supports many entries with the same address, so it will be fine. Actually this is exactly what we want.

@rphmeier
Copy link
Contributor

rphmeier commented Dec 9, 2016

What's the main use case for this? Will it be exposed over RPC to enable sending TXs in rapid succession more securely? It seems that once an account is in the transient store there are actually two valid "passwords" which could be used to unlock it, doubling the attack surface. Shouldn't it be impossible to unlock an account just using its password until any valid tokens have expired?

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Dec 9, 2016

This PR already contains RPC methods that could be used for signing with token.
The idea is to unlock account for particular "session" current unlocking stores user's password in memory, other option would be to store password in JS or store unencrypted key in memory. All those feel quite unsafe.
With token we can have many sessions (all start with user password) that use one-time tokens (with good enough entropy) to prove that the request is part of the session. Additionally we use the token to encrypt the private key. So Parity never stores neither password or unencrypted key longer than needed. JS will store the token which cannot be used twice.

@tomusdrw tomusdrw removed the M4-core ⛓ Core client code / Rust. label Dec 10, 2016
@gavofyork 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 Dec 10, 2016
@gavofyork
Copy link
Contributor

lots of try!(X)s rather than just X?s...

@tomusdrw tomusdrw added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Dec 10, 2016
Merge branch 'master' into rotating-key
27503e8 
Conflicts:
	ethstore/src/ethstore.rs
	ethstore/src/secret_store.rs
@tomusdrw tomusdrw 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 Dec 10, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 15, 2016
@gavofyork gavofyork merged commit c4406c9 into master Dec 15, 2016
@gavofyork gavofyork deleted the rotating-key branch December 15, 2016 12:08
@tomusdrw tomusdrw mentioned this pull request Dec 28, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M6-rpcapi 📣 RPC API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomusdrw @rphmeier @gavofyork