cosmos-sdk icon indicating copy to clipboard operation
cosmos-sdk copied to clipboard

`keys list` with `keyring-backend=os` requires you to enter your password once per key :o

Open ebuchman opened this issue 2 years ago • 10 comments

Summary of Bug

It appears that trying to run keys list with keyring-backend=os requires you to type in your computers master password once for every key you have.

I always felt the OS backend was a bit under developed. I get that it's a nice idea to use the built in keyring system but having to enter your master password to use the tooling by default seems like an anti-pattern, and having to enter it once per key just to list the keys is clearly a problem.

For what it's worth, I never use it, but I'm helping someone debug and was pretty appalled to learn they've just been putting up with entering their password 20 times to run keys list :(

Version

v0.44.3 (v6 of gaia)

Steps to Reproduce

Run gaia keys list --keyring-backend=os on a system with a bunch of keys


For Admin Use

  • [ ] Not duplicate issue
  • [ ] Appropriate labels applied
  • [ ] Appropriate contributors tagged
  • [ ] Contributor assigned/self-assigned

ebuchman avatar Jan 04 '22 18:01 ebuchman

I've experienced this issue as well. It's super annoying

fedekunze avatar Jan 04 '22 20:01 fedekunze

Yes, this is a big issue for testing purposes especially

sunnya97 avatar Jan 11 '22 21:01 sunnya97

This has come up a few times since this feature landed in the sdk. It has been a problem with the amount of times you enter the password.

tac0turtle avatar Jan 12 '22 09:01 tac0turtle

Is anyone working on this?

sunnya97 avatar Mar 23 '22 01:03 sunnya97

We are working on bringing on zondax to help with the keyring and help in a possible rewrite of keyring to something like keystone. At this time we don't have anyone working on this.

tac0turtle avatar Mar 23 '22 01:03 tac0turtle

ok this is extremely annoying, can someone fix this lol

fedekunze avatar May 05 '22 11:05 fedekunze

cc: @julienrbrt @jleni

fedekunze avatar May 05 '22 11:05 fedekunze

cc: @julienrbrt @jleni

If @jleni does not have time, I could have a look 🤔

julienrbrt avatar May 05 '22 11:05 julienrbrt

lets hold off on working on this for now. We are working with Zondax to decide what the future of keyring is.

tac0turtle avatar May 05 '22 14:05 tac0turtle

I am not able to reproduce this. Is it specific to gaia ?

I tried with gaia too and I can't reproduce it. I am running Ubuntu 20.04.

rllola avatar Jul 29 '22 16:07 rllola

Hi, I was able to reproduce the issue on macos 12.5. The fix for me was to simply click on Always allow when keychain prompt asks for my password.

raynaudoe avatar Aug 15 '22 17:08 raynaudoe

we should avoid recommended always allow. It could lead to unforeseen issues.

tac0turtle avatar Aug 15 '22 20:08 tac0turtle

@raynaudoe using Always Allow fixes it for the current binary, but for developers (who are probably the main users of CLI), everytime we update the binary, we have to reallow every key

sunnya97 avatar Aug 16 '22 05:08 sunnya97

Is there really something that can be done in cosmos side ? IMO since this is a backend issue, you either change your OS' config or type the passwords each time its prompt. Maybe I'm missing some other way ?

raynaudoe avatar Aug 22 '22 12:08 raynaudoe

I'm not really sure what the solution here is. It seems like requiring a passphrase per keyring item (i.e. keypair) is the default and correct safe behavior, but people are finding it annoying.

So is the idea that we want a passphrase on the entire keyring, not per item? If so, is that a config or something we can set? If not, perhaps we have to refactor how items are stored in the OS keyring so that they're treated as one "blob".

alexanderbez avatar Aug 22 '22 14:08 alexanderbez

One idea, is to just avoid calling this two functions together: https://github.com/cosmos/cosmos-sdk/blob/184235e1a8a0bf922d729c7b3dbca70c92637187/crypto/keyring/keyring.go#L524 and https://github.com/cosmos/cosmos-sdk/blob/184235e1a8a0bf922d729c7b3dbca70c92637187/crypto/keyring/keyring.go#L539

I think that if we create a function GetAll() that uses MatchLimitAll like here

and since would be only one call, the user would only have to type its password once to get all the items, instead of typing it once for getting the item list and then one more time per item on the list. Thoughts ?

raynaudoe avatar Sep 07 '22 20:09 raynaudoe

@raynaudoe let's give it a shot!

alexanderbez avatar Sep 07 '22 20:09 alexanderbez

Seems that is not possible, at least on macos: https://github.com/mogol/flutter_secure_storage/issues/70#issuecomment-518850439 I'll keep looking for another alternative that doesn't involve too much of a refactor

raynaudoe avatar Sep 08 '22 13:09 raynaudoe

ok, I found an easy way to reduce to a half the times the password gets prompted. Since the MigrateAll function does fairly the same process regarding keychain queries as the List function: If we make MigrateAll to return the records it finds: MigrateAll() ([]*Record, error) we have all the information there and List() would simply be:

func (ks keystore) List() ([]*Record, error) {
  return ks.MigrateAll()
}

thus avoiding doing again calls to Keys() and Get() for each key

raynaudoe avatar Sep 08 '22 15:09 raynaudoe

Yeah sounds good! Recall, we don't mind breaking APIs here if it means a better UX! Let's go for it 🚀

alexanderbez avatar Sep 08 '22 15:09 alexanderbez

Ok! I'll prepare a PR soon

raynaudoe avatar Sep 08 '22 15:09 raynaudoe