MetaMask / MetaMask/metamask-extension

Move Encryptor outside of KeyringController

Open
#4,088 1 comment 0 reactions 0 assignees View on GitHub
type-discussion type-enhancement type-security
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

Once #4034 is completed, the next function that could be moved outside of the KeyringController is the encryption functions.

Currently the KeyringController stores accounts and their schema, but these aren't actually the only portions of MetaMask state that might need to be encrypted.

Instead, we should move our encryption related functions into their own module at the `MetaMaskController` level, so that it can combine encryption-needing state as needed.

Two functions involving the password currently pass through the KeyringController:
- [ ] setLocked
- [ ] submitPassword

Also, a couple methods that assume keyringController manages the password encryption exist:
- [ ] createNewVaultAndKeychain
- [ ] createNewVaultAndRestore

Since we are using [obs-store](https://www.npmjs.com/package/obs-store) for most of our state objects now, it would probably make sense to take advantage of this architecture for the new encryption strategy.

## One implementation idea: SecureComposableObservableStore

We could maybe achieve this goal with a subclass of [ComposableObservableStore](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/ComposableObservableStore.js), [as the `metamaskController.store` instead](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/metamask-controller.js#L70).

### The `SecureComposableObservableStore` class

- Has a method named `subscribeToEncrypted`, which emits an encrypted blob instead of the clear text contents.
- Has a method named `submitPassword`, and does not emit any events until it has been unlocked.
- Has a `setLocked` method that deallocates any secret material and stops emitting updates.
- Emits events for `locked` and `unlocked`.
- `MetaMaskController` uses `locked` and `unlocked` events to setup the controllers managed by this store.
- `MetaMaskController` must also call [`updateStructure(opts)`](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/ComposableObservableStore.js#L24) on its `.memStore` on both of these events, to add/remove the added/removed controllers from the memStore.

Contributor guide

Open the contributing guide

Research direction

Review app/scripts/lib/ComposableObservableStore.js, app/scripts/metamask-controller.js around line 70, and the listed KeyringController methods after #4034 is complete. Determine how encrypted subscriptions, password submission, locking, and controller setup should work through the MetaMaskController store. Done means encryption no longer passes through KeyringController and locked/unlocked store events update memStore structure as described.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend, security
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.