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

feat: allow unlocking private keys directly through seed ids #121

Conversation

marcoskichel
Copy link
Contributor

@marcoskichel marcoskichel commented Jul 17, 2024

Description

Allow clients to feed the ids of the seeds directly instead of the seed when unlocking the private keys.

Important

I am not sure if this is completely safe, so please let me know if you think it is not.

Even though it seems to simply set a boolean indicating whether the keys are locked or not, it might have other security impacts that Im not aware. 🤔

The worst scenario I can think of is someone being able to intercept the seed ids and then being able to call unlockPrivateKeys effectively unlocking the private keys, which AFAIK will simply prevent re-authentication per transaction...

But as I said, I might be missing some important context here, so please let me know.

Towards: https://app.asana.com/0/1201782092493631/1207828730720173/f

Context

In ME, with transaction verification enabled, it is impossible, to the extent of my knowledge, to unlock the private keys if the user has at least one extra seed.

This happens because:

  • Keychain requires clients to provide the seeds when unlocking, it then derives the seedIds from them.
  • In the current implementation of Hydra wallet module, when retrieving the extra seeds it requires the extra seeds passphrase
  • The passphrases however, are stored in the keychain (it seems to use the private keys as passphrase)
  • With private keys locked, it is not possible to retrieve the extra seed passphrase, therefore making it impossible to provide the keys to the keychain in the first bullet point 🤣

We do already have the seedIds stored as keys of seedMetadataAtom, so we can simply forward it keychain instead of the seeds.

This is how I intend to use it: https://github.com/ExodusMovement/exodus-hydra/pull/7917

@marcoskichel
Copy link
Contributor Author

marcoskichel commented Jul 17, 2024

Perhaps a better alternative would be to use the same user password for all seeds instead of the private keys? That would probably require a larger refactor though.

Thoughts? @sparten11740

@marcoskichel marcoskichel changed the title feat: allow unlocking private keys by providing seed ids feat: allow unlocking private keys directly through seed ids Jul 18, 2024
@mvayngrib
Copy link
Collaborator

mvayngrib commented Jul 19, 2024

i think this breaks the security implications of the original use case #78

We want an operational mode, that would allow the wallet using the keychain to keep operating, e.g. keep generating addresses & adding portfolios/accounts, but not allow any signing unless the user reenters their authentication. E.g. block sending until password is entered on the send screen.

@sparten11740
Copy link
Contributor

sparten11740 commented Jul 19, 2024

Perhaps a better alternative would be to use the same user password for all seeds instead of the private keys?

not sure I fully understand the issue. The extra seeds passphrase is a private key derived from the primary seed. Is it not enough to use the user password to unlock the primary seed, which in turn can give us access to the other seeds?

The seed ids are not treated confidentially and shouldn't be sufficient to unlock the keychain

@marcoskichel
Copy link
Contributor Author

marcoskichel commented Jul 19, 2024

not sure I fully understand the issue. The extra seeds passphrase is a private key derived from the primary seed. Is it not enough to use the user password to unlock the primary seed, which in turn can give us access to the other seeds?

The seed ids are not treated confidentially and shouldn't be sufficient to unlock the keychain

How would we unlock only the primary seed though? keychain#unlockPrivateKeys requires all seeds to be provided as parameter, so if you have multiple seeds but try to unlock just one (the primary seed) it fails in this check. E.g.:

keychain.unlockPrivateKeys([primarySeed]) // Won't work with multiple seeds

@marcoskichel
Copy link
Contributor Author

As extra context, in ME we have this feature which locks the private keys every X seconds based on a user configuration: https://github.com/ExodusMovement/magic-eden/blob/268e0b6ed37a9d4727ab2aa52f35b7805d80bf38/packages/features/verify-transactions/module.js#L57

@mvayngrib
Copy link
Collaborator

How would we unlock only the primary seed though

why do you need to unlock the primary seed without unlocking the others?

@marcoskichel
Copy link
Contributor Author

marcoskichel commented Jul 20, 2024

How would we unlock only the primary seed though

why do you need to unlock the primary seed without unlocking the others?

I need to unlock all of them, but we currently only have access to the primary one during runtime.

My previous comment was to address what Jan said:

Is it not enough to use the user password to unlock the primary seed, which in turn can give us access to the other seeds?

It's not currently possible to do that if the user has more than one seed and the private keys are locked.

Anyway, I am going to close this and explore other alternatives.

@mvayngrib
Copy link
Collaborator

mvayngrib commented Jul 20, 2024

unlocking the primary seed does unlock all of them: https://github.com/ExodusMovement/exodus-hydra/blob/2cb482378d7f95f4764a5ff5e634971762f1cc8f/features/wallet/module/wallet.js#L257-L264

maybe we just need to call unlockPrivateKeys there after that block

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants