Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accounts, cmd, internal: disable unlock account as default #17037

Merged

Conversation

Projects
None yet
6 participants
@rjl493456442
Copy link
Member

commented Jun 20, 2018

Implements #17023

@rjl493456442 rjl493456442 force-pushed the rjl493456442:disable_unlock_account_asdefault branch from d417e87 to 93ed6c9 Jun 20, 2018

@rjl493456442 rjl493456442 changed the title Disable unlock account asdefault Disable unlock account as default Jun 20, 2018

@ligi
Copy link
Member

left a comment

nice - thanks for the fast work on this - users will appreciate this - just some small nits from my side.

Also perhaps put a [WIP] in the tittle - saw a todo in the code - and thought before it is ready to review.

Show resolved Hide resolved cmd/geth/main.go Outdated
Show resolved Hide resolved cmd/geth/main.go Outdated
Show resolved Hide resolved internal/ethapi/api.go Outdated

@rjl493456442 rjl493456442 force-pushed the rjl493456442:disable_unlock_account_asdefault branch from 93ed6c9 to 3305fce Jun 20, 2018

@rjl493456442

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

@ligi Thanks for your comments, now updated! For the todo, i think we need to decouple the config stuff to fix it. Maybe in another PR will be better.

@ligi

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

Great - thanks!
looks good - would just really DRY the error message - currently it's in there twice - slightly different

Not safe to unlock accounts. Please enable allow-insecure-unlock flag if still need to unlock

and

not safe to unlock account, please enable allow-insecure-unlock flag if necessary

I would really define this message once somewhere - also has the advantage that it connects both code-parts that care for a similar thing. Also it streamlines things and makes things DRY ;-)

@rjl493456442 rjl493456442 changed the title Disable unlock account as default WIP: Disable unlock account as default Jun 21, 2018

@rjl493456442 rjl493456442 force-pushed the rjl493456442:disable_unlock_account_asdefault branch from 3305fce to 17c9c1f Jun 21, 2018

@rjl493456442 rjl493456442 requested a review from holiman as a code owner Jun 21, 2018

@rjl493456442 rjl493456442 force-pushed the rjl493456442:disable_unlock_account_asdefault branch from 17c9c1f to a6c9231 Jun 21, 2018

@rjl493456442 rjl493456442 changed the title WIP: Disable unlock account as default accounts, cmd, internal: disable unlock account as default Jun 21, 2018

Show resolved Hide resolved cmd/geth/main.go Outdated
Show resolved Hide resolved cmd/geth/main.go Outdated
Show resolved Hide resolved cmd/geth/main.go Outdated
Show resolved Hide resolved cmd/geth/main.go Outdated
@holiman

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Sorry, I totally missed this PR. Would you care to fix the conflicts again?

@rjl493456442

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@holiman Sure! But this PR is super old, I need some time to recall the implementation detail.

@rjl493456442 rjl493456442 force-pushed the rjl493456442:disable_unlock_account_asdefault branch from 5fda690 to 0885e5f Feb 19, 2019

@rjl493456442

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

@holiman Updated, please take another look.

@holiman
Copy link
Contributor

left a comment

Codewise, this looks good to me, will do some testing aswell

@holiman
Copy link
Contributor

left a comment

LGTM

Show resolved Hide resolved accounts/manager.go Outdated
func unlockAccounts(ctx *cli.Context, stack *node.Node) {
// personalExposed checks whether personal API is exposed by http.
personalExposed := func() bool {
for _, module := range stack.Config().HTTPModules {

This comment has been minimized.

Copy link
@karalabe

karalabe Mar 7, 2019

Member

We should not tie it to personal rather to the entire RPC (both HTTP and WS). It's safer as eth is also susceptible.

Show resolved Hide resolved internal/ethapi/api.go Outdated

@karalabe karalabe removed the status:triage label Mar 7, 2019

@rjl493456442 rjl493456442 force-pushed the rjl493456442:disable_unlock_account_asdefault branch 2 times, most recently from 4056460 to 74ebd88 Mar 13, 2019

@holiman
Copy link
Contributor

left a comment

LGTM! I'll do some testing aswell, and report back

@holiman

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

As it now works, I think it's fine.

  • If you call unlock via CLI, it fails the unlock if ws or rpc is enabled,
  • If you call personal.unlock via rpc, it fails it if ws or http-rpc is enanbled

It is still possible to shoot yourself in the foot if you really try hard, via

> build/bin/geth --unlock 0 --password password.txt --nodiscover --maxpeers 0 console
[...]
> admin.startRPC()
INFO [03-29|11:32:57.269] HTTP endpoint opened                     url=http://localhost:8545         cors= vhosts=localhost
true

@holiman holiman added this to the 1.9.0 milestone Mar 29, 2019

Show resolved Hide resolved cmd/geth/main.go
@rjl493456442

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@holiman updated

rjl493456442 and others added some commits Feb 18, 2019

@karalabe karalabe force-pushed the rjl493456442:disable_unlock_account_asdefault branch from d5f332e to 41a3c11 Apr 4, 2019

@karalabe
Copy link
Member

left a comment

LGTM

@karalabe karalabe merged commit d5cae48 into ethereum:master Apr 4, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.