Bump `keyring-controller` and `browser-passworder` #21878

mikesposito · 2023-11-17T13:19:11Z

Description

This PR bumps these package versions:

@metamask/keyring-controller to ^9.0.0
@metamask/browser-passworder to ^4.3.0
@metamask/eth-keyring-controller to ^15.1.0

This set of updates brings a change in the way encryption keys are derived from the user password.

To allow encryption with different iteration numbers (e.g. for different use cases), and to easily change these iterations independently in the future, an encryptor factory has been added.

For performance reasons, @metamask/eth-keyring-controller has been fixed to prefer the cached encryption key over the password (it will not derive it again), when it is initialized with cacheEncryptionKey: true and the encryptor supports it (which is the case with @metamask/browser-passworder) - for this reason, this change only impacts the login UX, instead of every user interaction with the keychain.

The encryptor for Snaps has been set lower for now.

Related issues

n/a

Manual testing steps

Unlock an existing vault should work
Locking and unlocking again should still work

Note from DanM: probably a good idea to test this on a lower powered machine to see the performance impact. Test the before and after impact on performance of logging in, going from the unlock screen to the home screen.

These changes might have a UX impact on slower devices, for the following actions:

Wallet unlock
Actions on KeyringController (or direct keyrings) that end with a call to persistAllKeyrings, which derives the encryption key again and re-encrypt the vault. e.g.:
- Creating first seed phrase
- Importing an existing seed phrase
- ~~Add an account~~
- ~~Remove an account~~
- ~~Connecting a hardware wallet of any kind~~
- ~~Adding a keyring~~
- ~~Removing a keyring~~
- ~~Importing account~~
- (I might be forgetting something)
~~Snaps interactions: some of them call encrypt / decrypt, which also derive the key before/after all the interactions~~
There might be some UX oddities that we never noticed because of the fast derivation time, but that might be noticeable now (see this as an example)

Screenshots/Recordings

n/a

Before

n/a

After

n/a

Pre-merge author checklist

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2023-11-17T13:19:24Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

socket-security · 2023-11-17T13:24:52Z

New and updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/eth-keyring-controller	15.1.0	None	`+2`	256 kB	gudahtt
@metamask/keyring-controller	9.0.0	None	`+3`	402 kB	metamaskbot
@metamask/eth-trezor-keyring	2.0.0...3.0.0	None	`+0/-6`	82.3 kB	metamaskbot

mikesposito · 2023-11-20T14:41:27Z

.yarn/patches/@metamask-eth-keyring-controller-npm-13.0.1-06ff83faad.patch

The applied fix has been ported upstream

mcmire

Looks good!

mikesposito · 2023-11-21T09:43:38Z

Uhm, looks like some of the tests intermittently fail, with a timeout error. Most of them seem to be unrelated (or not directly related at least) to a change in the encryption or the vault - This makes me think that the increased time needed by the key derivation might take up too much of the test case time, which ultimately leads to a timeout in some cases: this would explain a similar test flakiness

mikesposito · 2023-11-21T13:14:07Z

After some local debugging, I think I have a clearer idea of why this happens.

Some of our test cases open external pages in the browser to test dapp features, and these tests take action on these pages right after the login. Some of them explicitly wait some time (with driver.delay(1000);), some others don't wait at all: this was ok-ish when the key derivation and vault decryption was fast, because the login operation used to end before the external pages were loaded, but now that the decryption is much slower this assumption is wrong because this happens:

The test case inputs the password, then clicks on login
The test case immediately opens an external page
The test case immediately executes an action on the test dapp
The pop-up (any pop-up screen, like the one to sign for example) will open, but instead of showing the expected screen, it will show the login screen to input the password, as the decryption is still happening on the main initial view.
The test fails with a timeout, as the screen expected by the test will never show up

Not all tests are affected, as some of them don't use external pages, and some of them wait ~1 sec before taking actions on dapps, but this issue makes the tests completely flaky and dependent on the resources available on the system that runs them.

mikesposito · 2023-11-21T13:35:28Z

The solution can be to use a shared helper login function to be used by all tests. This function would:

Input the password, press on login
Wait for the login to actually end
Resolve, giving the control back to the test

This will ensure that by the time the tests use dapps (or any other operation that needs the login), the wallet will be already unlocked. I will address this issue in a separate PR for ease of review.

EDIT: Issue tracked by #21914 and fixed by #21915

mikesposito · 2023-12-06T13:58:26Z

@metamaskbot update-policies

metamaskbot · 2023-12-06T14:05:48Z

Policy update failed. You can review the logs or retry the policy update here

metamaskbot · 2023-12-06T15:29:11Z

Builds ready [0780ec6]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1397 ± 138 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	87	377	161	80	38
		domContentLoaded	10	195	33	48	23
		load	838	1833	1397	287	138
		domInteractive	10	195	33	48	23

Bundle size diffs [🚀 Bundle size reduced!]

background: -129.58 KiB (-3.48%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

yarn.lock

metamaskbot · 2023-12-07T03:30:30Z

Builds ready [96a1a43]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1335 ± 102 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	89	150	116	21	10
		domContentLoaded	9	32	15	6	3
		load	911	1638	1335	212	102
		domInteractive	9	32	15	6	3

Bundle size diffs [🚀 Bundle size reduced!]

background: -129.58 KiB (-3.48%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

legobeat

Have not performed manual testing but changes LGTM

metamaskbot · 2023-12-07T11:43:11Z

Builds ready [c33de4f]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1386 ± 135 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	86	272	151	48	23
		domContentLoaded	9	179	37	46	22
		load	799	1812	1386	280	135
		domInteractive	9	179	37	46	22

Bundle size diffs [🚀 Bundle size reduced!]

background: -294.98 KiB (-7.93%)
ui: 0 Bytes (0.00%)
common: -2 Bytes (-0.00%)

mikesposito · 2023-12-07T11:53:17Z

Bundle size diffs [🚀 Bundle size reduced!]
background: -294.98 KiB (-7.93%)

‼️

mcmire

Looks great!

mcmire · 2023-12-07T16:14:09Z

test/e2e/helpers.js

@@ -315,6 +315,7 @@ const importSRPOnboardingFlow = async (driver, seedPhrase, password) => {
  await driver.fill('[data-testid="create-password-confirm"]', password);
  await driver.clickElement('[data-testid="create-password-terms"]');
  await driver.clickElement('[data-testid="create-password-import"]');
+  await driver.waitForElementNotPresent('.loading-overlay');


Perhaps we could extract this to a function whose name reflects this need in the future if we forget this, but this makes sense for now!

legobeat · 2023-12-07T21:03:29Z

Bundle size diffs [🚀 Bundle size reduced!]
background: -294.98 KiB (-7.93%)

‼️

Catching up on and deduping dependencies starting to pay off !

metamaskbot · 2024-01-08T20:07:54Z

Missing release label release-11.7.4 on PR. Adding release label release-11.7.4 on PR and removing other release labels(release-11.8.0), as PR was cherry-picked in branch 11.7.4.

mikesposito requested a review from a team as a code owner November 17, 2023 13:19

mikesposito marked this pull request as draft November 17, 2023 13:19

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 17, 2023

mikesposito force-pushed the chore/bump-browser-passworder branch 2 times, most recently from f17aac4 to 07faa5f Compare November 20, 2023 11:36

This comment was marked as outdated.

Sign in to view

metamaskbot removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 20, 2023

This comment was marked as outdated.

Sign in to view

mikesposito added the team-wallet-framework label Nov 20, 2023

mikesposito marked this pull request as ready for review November 20, 2023 12:08

mikesposito requested review from a team as code owners November 20, 2023 12:08

mikesposito requested review from Gudahtt, danjm and NicholasEllul November 20, 2023 12:08

mikesposito force-pushed the chore/bump-browser-passworder branch 2 times, most recently from 51a9958 to 8c166cf Compare November 20, 2023 14:24

This comment was marked as outdated.

Sign in to view

mikesposito commented Nov 20, 2023

View reviewed changes

mcmire previously approved these changes Nov 20, 2023

View reviewed changes

mikesposito dismissed mcmire’s stale review via 583e6ea November 23, 2023 08:44

mikesposito force-pushed the chore/bump-browser-passworder branch from d5c97e4 to 583e6ea Compare November 23, 2023 08:44

This comment was marked as outdated.

Sign in to view

mikesposito added 4 commits December 6, 2023 14:36

refactor: lower iterations to 600_000

251007d

feat: enable encryption key caching

1481d64

test: change mock encryptor to support keys

a36712c

test: fix dapp test by waiting for login success

9a5edec

mikesposito force-pushed the chore/bump-browser-passworder branch from aad500d to 9a5edec Compare December 6, 2023 13:38

fix: feature flag changed by mistake on rebase

0780ec6

mikesposito force-pushed the chore/bump-browser-passworder branch from 668957e to 0780ec6 Compare December 6, 2023 14:19

legobeat previously approved these changes Dec 7, 2023

View reviewed changes

legobeat reviewed Dec 7, 2023

View reviewed changes

yarn.lock Outdated Show resolved Hide resolved

Merge branch 'develop' into chore/bump-browser-passworder

96a1a43

legobeat mentioned this pull request Dec 7, 2023

deps: @metamask/eth-trezor-keyring@^2.0.0->^3.0.0 #21894

Merged

13 tasks

deps: @metamask/eth-trezor-keyring@^2.0.0->^3.0.0 (#21894)

9a363a3

legobeat dismissed their stale review via 9a363a3 December 7, 2023 10:26

Merge branch 'develop' into chore/bump-browser-passworder

c33de4f

legobeat approved these changes Dec 7, 2023

View reviewed changes

mcmire approved these changes Dec 7, 2023

View reviewed changes

mikesposito merged commit a6b592d into develop Dec 7, 2023
66 of 67 checks passed

mikesposito deleted the chore/bump-browser-passworder branch December 7, 2023 16:40

github-actions bot locked and limited conversation to collaborators Dec 7, 2023

metamaskbot added the release-11.8.0 Issue or pull request that will be included in release 11.8.0 label Dec 7, 2023

metamaskbot added release-11.7.4 Issue or pull request that will be included in release 11.7.4 and removed release-11.8.0 Issue or pull request that will be included in release 11.8.0 labels Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump `keyring-controller` and `browser-passworder` #21878

Bump `keyring-controller` and `browser-passworder` #21878

mikesposito commented Nov 17, 2023 •

edited

Loading

github-actions bot commented Nov 17, 2023

socket-security bot commented Nov 17, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mikesposito Nov 20, 2023

mcmire left a comment

mikesposito commented Nov 21, 2023 •

edited

Loading

mikesposito commented Nov 21, 2023

mikesposito commented Nov 21, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

mikesposito commented Dec 6, 2023

metamaskbot commented Dec 6, 2023

metamaskbot commented Dec 6, 2023

metamaskbot commented Dec 7, 2023

legobeat left a comment

metamaskbot commented Dec 7, 2023

mikesposito commented Dec 7, 2023

mcmire left a comment

mcmire Dec 7, 2023

legobeat commented Dec 7, 2023

metamaskbot commented Jan 8, 2024

Bump keyring-controller and browser-passworder #21878

Bump keyring-controller and browser-passworder #21878

Conversation

mikesposito commented Nov 17, 2023 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Nov 17, 2023

socket-security bot commented Nov 17, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mikesposito Nov 20, 2023

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

mikesposito commented Nov 21, 2023 • edited Loading

mikesposito commented Nov 21, 2023

mikesposito commented Nov 21, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

mikesposito commented Dec 6, 2023

metamaskbot commented Dec 6, 2023

metamaskbot commented Dec 6, 2023

metamaskbot commented Dec 7, 2023

legobeat left a comment

Choose a reason for hiding this comment

metamaskbot commented Dec 7, 2023

mikesposito commented Dec 7, 2023

mcmire left a comment

Choose a reason for hiding this comment

mcmire Dec 7, 2023

Choose a reason for hiding this comment

legobeat commented Dec 7, 2023

metamaskbot commented Jan 8, 2024

Bump `keyring-controller` and `browser-passworder` #21878

Bump `keyring-controller` and `browser-passworder` #21878

mikesposito commented Nov 17, 2023 •

edited

Loading

socket-security bot commented Nov 17, 2023 •

edited

Loading

mikesposito commented Nov 21, 2023 •

edited

Loading

mikesposito commented Nov 21, 2023 •

edited

Loading