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

Remove/split out account management #9997

Closed
5chdn opened this issue Nov 30, 2018 · 29 comments
Closed

Remove/split out account management #9997

5chdn opened this issue Nov 30, 2018 · 29 comments
Assignees
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. F8-enhancement 🎊 An additional feature request. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. P7-nicetohave 🐕 Issue is worth doing eventually. Q5-substantial 📓 Can be fixed by a developer with decent experience.
Milestone

Comments

@5chdn
Copy link
Contributor

5chdn commented Nov 30, 2018

Ideally, an Ethereum client should not contain account management and keystore.

Keys and transactions should be managed by external applications. Now that we removed the wallet from the client, this is a breaking, but easy to implement change. Maybe we double down our efforts on ethstore and ethkey.

@5chdn 5chdn added F8-enhancement 🎊 An additional feature request. F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. P7-nicetohave 🐕 Issue is worth doing eventually. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. labels Nov 30, 2018
@5chdn 5chdn added this to the 2.4 milestone Nov 30, 2018
@5chdn 5chdn added the Q5-substantial 📓 Can be fixed by a developer with decent experience. label Nov 30, 2018
@amaury1093
Copy link
Contributor

To be coordinated with Fether, who will take care of account management: openethereum/fether#148

@folsen
Copy link
Contributor

folsen commented Nov 30, 2018

See also https://github.com/ethereum/go-ethereum/tree/master/cmd/clef

@5chdn
Copy link
Contributor Author

5chdn commented Dec 3, 2018

also: hardware-wallet support is unused: #9622

@gituser
Copy link

gituser commented Dec 9, 2018

What?
Do you want to remove whole account (e.g. personal RPC api endpoint) management from parity?
It seems to me you love to break things for users.

Why remove what is working for million services without proper alternative first and proper migration guide?

If you really want to remove it please make 3.x version of parity as it's super breaking change and I'm sure not many will want to update.

@folsen
Copy link
Contributor

folsen commented Dec 11, 2018

@gituser @lazaridiscom please follow the general discussion that is being had around this topic within the community for more information.

A quick recap is; a node should not manage private keys and it shouldn't be within the responsibility of a node to do so. There are many reasons for why this is bad, first is security, many peoples private keys have been stolen and funds have been stolen because of stupid mistakes like opening a port when they shouldn't have. This is something that has plagued blockchain nodes since before Parity existed and since before even Ethereum existed.

Account management should be handled by a separate program, that you can run in a controlled environment, maybe even in a secure enclave itself, but regardless it should probably not be connected to the internet and not be remotely accessible.

Designing the system this way is just better. Geth has already gone down this path.

Do you want to remove whole account (e.g. personal RPC api endpoint) management from parity?

Obviously the RPC API endpoints are still needed somewhere, just not in the client. If we went down this path we would provide an account management program such as described above that would also provide these endpoints (but perhaps changed so that they're not exposable to the internet).

If you have any specific requests that would make your life easier, here and now would be the place to make those requests. Simply saying "No client should ever change" isn't helpful.

Why remove what is working for million services without proper alternative first and proper migration guide?

Why would you assume that this is the case?

@gituser
Copy link

gituser commented Dec 11, 2018

@folsen, regarding RPC API endpoints you can always disable those before starting parity, I don't see who will benefit if you remove/split those from the client.

For example: the UI split resulted in negative impact for parity many users moved out of it because of the complications in the installation and maintenance.

Regarding, security - you can always enforce best security default settings for node operators and let them decide what they want to open via settings.

I'm not saying I'm against changes - but they should be rolled out slowly and imo there are many other more important bugs out there which needs fixing.

Geth still have signing and account management inside the client I believe, at least v1.8.20 does:

NAME:
   geth - the go-ethereum command line interface

   Copyright 2013-2018 The go-ethereum Authors

USAGE:
   geth [options] command [command options] [arguments...]
   
VERSION:
   1.8.20-stable
   
COMMANDS:
   account           Manage accounts

@tomusdrw tomusdrw self-assigned this Dec 31, 2018
@5chdn 5chdn modified the milestones: 2.4, 2.5 Jan 10, 2019
@5chdn 5chdn modified the milestones: 2.5, 2.6 Feb 21, 2019
@soc1c soc1c modified the milestones: 2.6, 2.7 Apr 2, 2019
@cemkod
Copy link

cemkod commented Apr 11, 2019

What is the alternative to personal RPC api endpoint? What should we use? The depreciation message says to look at this issue for alternatives, yet I couldn't see any alternatives here.

@tomusdrw
Copy link
Collaborator

@cemkod The recommended way is to manage the accounts on your own (preferably via secure hardware wallet) and submit signed transactions via eth_sendRawTransaction.

That said before removing the methods completely we will most likely provide an alternative via JSON-RPC proxy that would handle signing (for instance using https://github.com/ethereum/go-ethereum/tree/master/cmd/clef)

@cemkod
Copy link

cemkod commented Apr 11, 2019

As out hotwallet is automated and in a datacenter somewhere, using a hardware wallet is not an option for us. A clef style account management software tool would work though. Please make sure everything is working flawlessly with the new tool before taking this out of parity though as I know many institutions like us depend on it.

@gituser
Copy link

gituser commented May 13, 2019

Can you remove the annoying line from logs:

jsonrpc-eventloop-0 WARN rpc eth_accounts is deprecated and will be removed in future versions: Account management is being phased out see #9997 for alternatives.

as there is no real alternative at the moment?

@stone212
Copy link

@5chdn @tomusdrw @folsen I am seeing this thread now for the first time because only now do I see the error message in Parity Account management is being phased out see #9997 for alternatives.

Questions and thoughts:

  1. When is this planned for deprecation? We have a live/production app that relies on this functionality. How much time do we have?

  2. There must be many other uses like this who have not seen the warning. Why would they see it? I saw it on an un-related project.

  3. For automated systems (like ours) a hardware wallet is not possible because that requires human intervention.

  4. Yes of course this is potentially un-safe, but it is very safe if handled correctly with SSH tunnel, good server security, etc.

  5. The functionality already exists.

  6. 4+5 = Maybe a good idea is to keep this function and require the use of a --not-recommended flag, at least until:

  7. There are no current alternatives. Are there? If there are please tell me.

  8. Fether is not an alternative because

i. Fether is not stable enough for production

ii. Fether is a graphical UI, and that also requires human involvement so is not a solution for automated systems.

@folsen
Copy link
Contributor

folsen commented Jul 2, 2019

@stone212

When is this planned for deprecation? We have a live/production app that relies on this functionality. How much time do we have?

There is no real plan around when we want to deprecate this honestly, it's been shelved a little bit while we work on other stuff.

For automated systems (like ours) a hardware wallet is not possible because that requires human intervention.

There are HSM solutions provided on most cloud providers but you can also buy HSMs if you have dedicated machines deployed.

There are no current alternatives. Are there? If there are please tell me.

clef would be an example of an existing alternative.

@stone212
Copy link

stone212 commented Jul 2, 2019

@folsen

There are HSM solutions provided on most cloud providers but you can also buy HSMs if you have dedicated machines deployed.

I don't understand how this is related to the topic?

clef would be an example of an existing alternative.

I will advise the developers. Thank you. But from reading the Readme it looks like clef requres user intervention. That won't work for an automated system. Maybe this is why you mentioned an HSM but at this moment I am not seeing a connection. EDIT: I should be clear. I know what an HSM is but the provider definitely does not offer this (and the app doesn't use a dedicated machine). But also I don't know how I would "tie into" the HSM with a Parity/clef key. Do you know of a doc or API reference? That's more of a clef question I think. Anyway thank you for the information and now it's up to the devs.

@slavserver

This comment has been minimized.

@tomusdrw
Copy link
Collaborator

tomusdrw commented Jul 3, 2019

Guys, calm down. We are planning to deprecate account management in the core client, but as you can see, no solid dates are mentioned, so it's only going to be removed if and when we make everybody happy with proposed alternatives.
No one plans to disrupt your business and break compatibility without proper notice. The deprecation was added to get your attention and discuss possible solutions.

The reasons we would rather avoid having account management in the client itself is:

  1. There are more secure alternatives to JSON key files (like hardware wallets, HSM, secured clef instance, etc)
  2. Managing accounts in the client itself requires a lot of code to maintain and causes some subtle issues (nonce conflicts)
  3. In general the sentiment among tool developers is that private keys should be kept as local and as close to the user as possible. Since many people now run hosted nodes, we want them to keep the keys locally.

So alternatives we propose:

  1. Managing JSON keys on your own (in the application that connects over RPC) - that requires changes in the client code, but keeps the keys local to the client app.
  2. Using hardware wallet
  3. A proxy that will delegate to clef or handle JSON keys (the proxy can be kept local and just connect to remote node).

With (3) the transition would be almost seamless, it just requires running two separate processes. However the development of this solution has stalled a little bit, but as I mentioned earlier no one is really planning to leave you without a proper alternative that does not require substantial changes in your infrastructure.

@slavserver
Copy link

Ok

@stone212
Copy link

stone212 commented Jul 3, 2019

@tomusdrw I appreciate your comment but no one (well... one person only) is getting excited.

The deprecation was added to get your attention and discuss possible solutions.

Yes, that's why I'm glad to be in the thread. About your suggestions:

Managing JSON keys on your own (in the application that connects over RPC) - that requires changes in the client code, but keeps the keys local to the client app.

That is a good idea of course but I do not understand the specifics well enough. It is better of course to keep keys and key management away from the node, agreed. But what functionality would the client software use to send the transaction that is different from now? Basically the packaging of the data into a bona fide encrypted, well-formatted transaction has to take place away from Parity right? So where is the ruleset that client-side devs should follow? And to what RPC function should the data be sent?

Using hardware wallet

Yes. This is appropriate for single transactions, only not batched.

A proxy that will delegate to clef or handle JSON keys (the proxy can be kept local and just connect to remote node).

Yes this is something I am asking some devs to research and I hope they (and maybe I) can contribute to that development as it continues.

I also think my idea is not terrible as a temporary fix (say, 6 months after you do deprecate the function) which is:

  1. Allow people to continue the current potentially unsafe practice with a --i-know-what-im-doing or something flag. For users who are happy with the security of an SSH tunnel etc. (Most RPC attacks come from bad server security).

@slavserver
Copy link

--i-know-what-im-doing- use current rpc, ok)

Use 127.0.0.1 no one has canceled. and if the user does not understand the essence, these are his problems. Imho

@stone212
Copy link

stone212 commented Jul 3, 2019

@slavserver

if the user does not understand the essence, these are his problems

But the news report will be "ETH stolen from Parity nodes" and we can understand the devs do not want this.

Use 127.0.0.1

Yes this is a good point I think I did not specify. If the node only allows RPC on 127.0.0.1 then the security risk of maintaining the wallet is small.

@slavserver
Copy link

127.0.0.1 the most important thing is not to proxy through www and other tools. and everything is perfectly protected.)

@slavserver
Copy link

if you still change, do not forget to add the option (--i-know-what-im-doing) to use the current rpc standard. I really do not want to rewrite the mining pool code. Thank you for your answers and have a nice day))

@stone212
Copy link

stone212 commented Jul 3, 2019

@slavserver This is maybe a little un-related but which pool are you using?

@slavserver
Copy link

slavserver commented Jul 3, 2019

By the way, you may not have noticed, but parity no works with musicoin, callisto, expanse. parity uses the old branch of the blockchain, on these coins was recently hardfork

@slavserver
Copy link

By the way, you may not have noticed, but parity no works with musicoin, callisto, expanse. parity uses the old branch of the blockchain, on these coins was recently hardfork

Please, fix this error

@jam10o-new
Copy link
Contributor

@slavserver please keep conversation on topic, make new issues for musicoin and callisto. PRs are also welcome - here's the expanse hard fork tracking issue: #10570

@tomusdrw
Copy link
Collaborator

tomusdrw commented Oct 1, 2019

Simple eth_sendTransaction interception is now available in proxy, see: https://github.com/tomusdrw/jsonrpc-proxy/pull/57/files#diff-26c33be24b76b7fc990476bca1da971bR28

Requires some docs though.

@mpirrongelli
Copy link

In my case, I'm happy that you guys are looking for new alternatives however for some reason the nodes I have are stopping sync because the use of this 2. when i check the log the only thing keeps repeating is the following:

  • Dec 30 12:46:13 ubuntu-c-4-8giXXXXXX parity[31042]: 2019-12-30 12:46:13 UTC eth_sendTransaction is deprecated and will be removed in future versions: Account management is being phased out see Remove/split out account management #9997 for alternatives.

  • Dec 30 12:46:21 ubuntu-c-4-8giXXXXXX parity[31042]: 2019-12-30 12:46:21 UTC eth_accounts is deprecated and will be removed in future versions: Account management is being phased out see Remove/split out account management #9997 for alternatives.

I have to stop the node and start again to continue sync.

Hope you can provide some feedback about this.
Thank you in advance.

@stone212
Copy link

stone212 commented Jan 1, 2020

@mpirrongelli There is a new beta that might fix this.

@Hornobster
Copy link

How should we keep the signer account for PoA (aura) authority nodes?
The tutorial uses the eth_accounts API.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. F1-security 🛡 The client fails to follow expected, security-sensitive, behaviour. F8-enhancement 🎊 An additional feature request. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. P7-nicetohave 🐕 Issue is worth doing eventually. Q5-substantial 📓 Can be fixed by a developer with decent experience.
Projects
None yet
Development

No branches or pull requests