refactor: migrate Encryptor to TypeScript and increase PBKDF2 iterations number #9093

gantunesr · 2024-03-28T19:37:32Z

Description

📣 The code in this PR has already been reviewed and QA'd in #8828

This PR introduces the following modifications

@metamask/keyring-controller bump from v8.1.0 to v9.0.0
Refactor the Encryptor class from JS to TS
Adds unit tests to the Encryptor class
Increases the number of iterations for PBKDF2 from 5.000 to 900.000

CHANGELOG from `@metamask/keyring-controller`

Added,

Add KeyringController:persistAllKeyrings messenger action (#1965)

Changed,

BREAKING Change encryptor constructor option property type to GenericEncryptor | ExportableKeyEncryptor | > undefined (#2041)

When the controller is instantiated with cacheEncryptionKey: true, encryptor may no longer be of type GenericEncryptor.

Bump dependency on @metamask/scure-bip39 2.1.1 (#1868)

Bump dependency on @metamask/utils to 8.2.0 (#1957)

Bump @metamask/eth-keyring-controller to 14.0.0 (#1771)

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/100

Manual testing steps

Test case 1: Upgrade the client from the current version to feature branch

Install the current version of MM mobile or the last commit in the main branch
Create or import a wallet and verify it is correctly created
Lock the wallet
Upgrade the app to this feature branch
Unlock the wallet, it should unlock the wallet

Test case 2: Lock and unlock the wallet

Create or import a wallet and verify it is correctly created
Lock the wallet
Unlock the wallet, it should unlock the wallet

Test case 3: Reveal SRP and private key

Create or import a wallet and verify it is correctly created
Go to Settings > Security & Privacy
Reveal the SRP, it should show the correct SRP
Go back to Security & Privacy
Reveal the private key of the current account

Test case 4: Connect QR wallet

Create or import a wallet and verify it is correctly created
Connect QR wallet and import 2 or more accounts

Test case 5: Connect Ledger wallet

Create or import a wallet and verify it is correctly created
Connect Ledger wallet and import the account

Test case 6: Sign a transaction with each account type

Create or import a wallet and verify it is correctly created
Connect Ledger wallet and import the account
Connect QR wallet and import 2 or more accounts
Send a transaction in a test network using each account type (HD, QR, and Ledger)

Test case 7: Sign a different messages with each account type

Create or import a wallet and verify it is correctly created
Connect Ledger wallet and import the account
Connect QR wallet and import 2 or more accounts
Go to MM E2E Test Dapp and sign different messages (Eth Sign, Personal Sign, Sign Typed Data, Sign Typed Data V3, Sign Typed Data V4, Sign Permit, Sign In With Ethereum) with each account (HD, QR, and Ledger). Note: Some methods are not supported in Ledger

Test case 8: Change password

Document with test case

Test case 9: Unlock with biometrics

Document with test case

Test case 10: Remember me

Document with test case

Test case 11: Onboarding with biometrics

Document with test case

Unit Tests and Coverage

yarn jest -u app/core/Encryptor/Encryptor.test.ts --collectCoverage --collectCoverageFrom=app/core/Encryptor/Encryptor.ts

=============================== Coverage summary ===============================
Statements   : 100% ( 46/46 )
Branches     : 94.73% ( 18/19 )
Functions    : 100% ( 14/14 )
Lines        : 100% ( 45/45 )
================================================================================

Screenshots/Recordings

Not applicable. No changes to the UI/UX.

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
I've clearly explained what problem this PR is solving and how it is solved.
I've linked related issues
I've included manual testing steps
I've included screenshots/recordings if applicable
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
I’ve properly set the pull request status:
- In case it's not yet "ready for review", I've set it to "draft".
- In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

…ons number

socket-security · 2024-03-28T19:41:37Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@metamask/eth-keyring-controller@15.1.0

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

github-actions · 2024-03-28T20:10:04Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 8777bac
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a9634414-a34d-44f2-8d16-cfe6db679f24

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

gantunesr · 2024-03-28T20:15:31Z

@SocketSecurity ignore npm/@metamask/eth-keyring-controller@15.1.0

codecov-commenter · 2024-03-28T20:17:11Z

Codecov Report

Attention: Patch coverage is 96.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 45.66%. Comparing base (543d671) to head (8777bac).

Files	Patch %	Lines
app/core/Encryptor/Encryptor.ts	97.95%	1 Missing ⚠️
app/util/validators/index.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9093      +/-   ##
==========================================
+ Coverage   45.55%   45.66%   +0.10%     
==========================================
  Files        1272     1273       +1     
  Lines       31238    31263      +25     
  Branches     3189     3198       +9     
==========================================
+ Hits        14232    14275      +43     
+ Misses      16167    16149      -18     
  Partials      839      839

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tommasini

LGTM! Awesome refactor on the Encryptor class

github-actions · 2024-04-08T10:37:50Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 8efbf16
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/403a3269-ffa5-432a-8c73-8f0abafa2f06

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

socket-security · 2024-04-08T18:58:12Z

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

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/keyring-controller@9.0.0	None	`+20`	1.72 MB	metamaskbot

🚮 Removed packages: npm/@metamask/keyring-controller@8.1.0

View full report↗︎

app/core/Encryptor/Encryptor.ts

app/core/Encryptor/Encryptor.test.ts

app/core/Encryptor/Encryptor.ts

app/core/Encryptor/types.ts

github-actions · 2024-04-16T22:22:09Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: b3ff44f
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/31ce1a39-e153-4aa7-a535-eef87eebfac0

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

github-actions · 2024-04-18T00:07:52Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 429d8cd
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/67b0405e-982c-42a4-a692-6dd3fa6ce9f6

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

…to refactor/encryptor-class

github-actions · 2024-04-18T21:27:23Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 3a70ab2
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ee59d7fe-460f-4782-b8cb-5b8d78bfb1f7

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

This reverts commit e3a4e66.

…k/metamask-mobile into refactor/encryptor-class

github-actions · 2024-04-19T12:03:31Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: ccf8052
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/17bcf00e-9d63-4e04-bc97-59061679f44d

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

sonarqubecloud · 2024-04-19T12:13:18Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
1 Security Hotspot
90.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

ccharly

LGTM. Just left minor remarks that we can handle later on! :)

app/core/Encryptor/Encryptor.test.ts

gantunesr added 2 commits March 26, 2024 11:57

refactor: encryptor to typescript

5b43a7b

refactor: migrate Encryptor to TypeScript and increase PBKDF2 iterati…

5516e4a

…ons number

gantunesr added the team-accounts-framework Accounts Framework team label Mar 28, 2024

gantunesr force-pushed the refactor/encryptor-class branch from 3b72a9e to 5516e4a Compare March 28, 2024 20:04

gantunesr marked this pull request as ready for review March 28, 2024 20:06

gantunesr requested a review from a team March 28, 2024 20:06

gantunesr requested a review from a team as a code owner March 28, 2024 20:06

Merge branch 'main' into refactor/encryptor-class

8777bac

gantunesr added the Run Smoke E2E label Mar 28, 2024

gantunesr mentioned this pull request Mar 28, 2024

refactor: migrate Encryptor to TypeScript and increase PBKDF2 iterations number #8828

Closed

11 tasks

Merge branch 'main' into refactor/encryptor-class

2e6efc1

gantunesr self-assigned this Apr 7, 2024

test: add return value to crypto.getRandomValues mock

8efbf16

gauthierpetetin added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 8, 2024

tommasini previously approved these changes Apr 8, 2024

View reviewed changes

tommasini added Run Smoke E2E and removed Run Smoke E2E labels Apr 8, 2024

feat: update iteration numbers

7e40de3

gantunesr dismissed tommasini’s stale review via 7e40de3 April 8, 2024 18:19

Merge branch 'main' into refactor/encryptor-class

a9366e1

FrederikBolding reviewed Apr 9, 2024

View reviewed changes

app/core/Encryptor/Encryptor.ts Show resolved Hide resolved

ccharly reviewed Apr 12, 2024

View reviewed changes

app/core/Encryptor/Encryptor.test.ts Outdated Show resolved Hide resolved

ccharly previously approved these changes Apr 12, 2024

View reviewed changes

app/core/Encryptor/Encryptor.ts Show resolved Hide resolved

app/core/Encryptor/types.ts Outdated Show resolved Hide resolved

gantunesr removed the Run Smoke E2E label Apr 15, 2024

gantunesr added 2 commits April 16, 2024 14:50

Merge branch 'main' into refactor/encryptor-class

ae71f72

Merge branch 'main' into refactor/encryptor-class

b3ff44f

gantunesr added Run Smoke E2E and removed Run Smoke E2E labels Apr 16, 2024

gantunesr added Run Smoke E2E and removed Run Smoke E2E labels Apr 17, 2024

Merge branch 'main' into refactor/encryptor-class

429d8cd

gantunesr added Run Smoke E2E and removed Run Smoke E2E labels Apr 18, 2024

gantunesr and others added 3 commits April 18, 2024 08:20

Merge branch 'main' of https://github.com/MetaMask/metamask-mobile in…

c861bee

…to refactor/encryptor-class

chore: use legacy number of iterations

e3a4e66

Merge branch 'main' into refactor/encryptor-class

3a70ab2

gantunesr added Run Smoke E2E and removed Run Smoke E2E labels Apr 18, 2024

gantunesr and others added 3 commits April 18, 2024 18:15

Revert "chore: use legacy number of iterations"

2b41822

This reverts commit e3a4e66.

Merge branch 'refactor/encryptor-class' of https://github.com/MetaMas…

72bbd42

…k/metamask-mobile into refactor/encryptor-class

Merge branch 'main' into refactor/encryptor-class

ccf8052

gantunesr added Run Smoke E2E and removed Run Smoke E2E labels Apr 19, 2024

tommasini approved these changes Apr 19, 2024

View reviewed changes

ccharly approved these changes Apr 19, 2024

View reviewed changes

app/core/Encryptor/Encryptor.test.ts Show resolved Hide resolved

app/core/Encryptor/Encryptor.test.ts Show resolved Hide resolved

gantunesr merged commit 13afba8 into main Apr 19, 2024

gantunesr deleted the refactor/encryptor-class branch April 19, 2024 15:03

github-actions bot locked and limited conversation to collaborators Apr 19, 2024

metamaskbot added the release-7.22.0 Issue or pull request that will be included in release 7.22.0 label Apr 19, 2024

Uh oh!

refactor: migrate Encryptor to TypeScript and increase PBKDF2 iterations number #9093

refactor: migrate Encryptor to TypeScript and increase PBKDF2 iterations number #9093

Uh oh!

Conversation

gantunesr commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

CHANGELOG from @metamask/keyring-controller

Related issues

Manual testing steps

Unit Tests and Coverage

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

socket-security bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Next steps

Uh oh!

github-actions bot commented Mar 28, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

gantunesr commented Mar 28, 2024

Uh oh!

codecov-commenter commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tommasini left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 8, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

socket-security bot commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 16, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

github-actions bot commented Apr 18, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

github-actions bot commented Apr 18, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

github-actions bot commented Apr 19, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

sonarqubecloud bot commented Apr 19, 2024

Quality Gate passed

Uh oh!

ccharly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

gantunesr commented Mar 28, 2024 •

edited

Loading

CHANGELOG from `@metamask/keyring-controller`

socket-security bot commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited by metamaskbot

Loading

codecov-commenter commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024 •

edited by metamaskbot

Loading

socket-security bot commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 16, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Apr 18, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Apr 18, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Apr 19, 2024 •

edited by metamaskbot

Loading