feat: lockPrivateKeys and unlockPrivateKeys

[andrejborstnik]

Motivation

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.

What this PR adds

Adds a semi-locked state, which blocks external usage of private keys, but still allows internal usage to generate new public keys. To start using private keys again the keychain needs to be "unlocked". To achieve that securely we force the user to re-add the seeds previously inserted into the keychain, which inherently proves that the user has the permission to view the seeds.

For a use-case where we would also want to remove the seeds from memory for improved security we can use the existing removeAllSeeds instead.

[mvayngrib]

For a use-case where we would also want to remove the seeds from memory for improved security we can use the existing removeAllSeeds instead.

given we require seeds to be passed in to unlockPrivateKeys, why don't we always use removeAllSeeds instead? (and keep the public master keys). remove privateKeys from memory and only keep the master public keys

[andrejborstnik]

For a use-case where we would also want to remove the seeds from memory for improved security we can use the existing removeAllSeeds instead.

given we require seeds to be passed in to unlockPrivateKeys, why don't we always use removeAllSeeds instead? (and keep the public master keys). remove privateKeys from memory and only keep the master public keys

We could keep only the public keys if we wanted - I assume you meant we would derive the master/toplevel public key and derive addresses from that? Should work, but might need a bigger refactor, since right now we derive everything from private keys

[mvayngrib]

@exodus/slip10 and @exodus/hd-key-slip-10 would need to support derivation without the private key present and we'd need this in lockPrivateKeys:

    for (const seedId in this.#masters) {
      for (const derivationAlgorithm in this.#masters[seedId]) {
        const key = this.#masters[seedId][derivationAlgorithm]
        key.wipePrivateData()
      }
    }

seems safer though 🤔

[andrejborstnik]

@exodus/slip10 and @exodus/hd-key-slip-10 would need to support derivation without the private key present and we'd need this in lockPrivateKeys:

    for (const seedId in this.#masters) {
      for (const derivationAlgorithm in this.#masters[seedId]) {
        const key = this.#masters[seedId][derivationAlgorithm]
        key.wipePrivateData()
      }
    }

seems safer though 🤔

@mvayngrib reading the SLIP10 spec it seems that slip10 doesn't support continued derivation of child public keys since each step is hardened https://github.com/satoshilabs/slips/blob/master/slip-0010.md. My understanding is that means that we can continue deriving new addresses on existing xpubs, but could not derive new xpubs for new accounts of new coins.

Similar for BIP32 when we do hardening, child public keys cannot be derived.

Lmk if I am misunderstanding & if you still think it is worth moving forward with the pubkey approach.

[mvayngrib]

utACK

prob need to export api hasLockedPrivateKeys() (or maybe arePrivateKeysLocked), but separate PR is ok

[andrejborstnik]

utACK

prob need to export api hasLockedPrivateKeys() (or maybe arePrivateKeysLocked), but separate PR is ok

Done in 1de6179

[kewde]

My preference goes to moving this locking mechanism into the transaction & message signer modules. "Locking the private keys" will impact every feature and we're not going to change every feature to accommodate this, instead we'll be sticking some callbacks to open up the UI in the transactionSigner and messagSigner to cover all the cases. At which point we can just do it at that level.

[andrejborstnik]

My preference goes to moving this locking mechanism into the transaction & message signer modules. "Locking the private keys" will impact every feature and we're not going to change every feature to accommodate this,

If a feature can use private keys while the keychain is locked for signing then that defeats the purpose of the check. Why would we implement a less secure solution?

instead we'll be sticking some callbacks to open up the UI in the transactionSigner and messagSigner to cover all the cases. At which point we can just do it at that level.

Even if we only added UI support for unlocking in transactionSigner and messageSigner having signing throw for other cases is preferred to it working while the app is supposed to be locked imo. Don't you agree?

[andrejborstnik]

hard lock and wipe private keys
cache public keys at the publicKeyProvider level

is that an option for the desired use cases @andrejborstnik ?

No, will clarify on slack

[andrejborstnik]

re-pushed so that commits are signed (likely pressed the rebase button in UI in the past, which broke that). Can you re-review @mvayngrib (should be no changes)

[mvayngrib]

utACK