Skip to content

Support Multi-srp backups in SeedlessOnboardingController #5685

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

Closed

Conversation

lwin-kyaw
Copy link
Contributor

@lwin-kyaw lwin-kyaw commented Apr 22, 2025

Explanation

Support multiple SeedPhrases backup under one user account (social).

  • Added addNewSeedPhraseBackup in controller to create backup for new SeedPhrase.
  • Added EncryptionKey (which will be derived from the password) in the controller as non-persisted state, to unlock the Seedless Vault to retrieve the Encryption Key and Auth Key without entering the password. The reason for storing this is that when user imports new SeedPhrase in the wallet, the newly imported SeedPhrase can be backed up automatically without needing to type the password.
  • Added Controller Lock and sync with KeyringController:lock and KeyringController:unlock events

References

Changelog

@metamask/seedless-onboarding-controller

  • ADDED: added addNewSeedPhraseBackup method in the controller to use for import new SeedPhrase
  • UPDATED: updated Encryptor interface with the ExportableKeyEncryptor from KeyringController
  • ADDED: encryptionKey and encryptionSalt state as non-persisted state
  • ADDED: lock to sync with KeyringController's lock and unlock events
  • UPDATED: Order of restored SeedPhrases from the fetchAllSeedPhrases method to be the same order as KeyringsMetadata from KeyringController
  • ADDED: helper methods to check/assert Controller Lock state
  • UPDATED: mark encryptor as required property in SeedlessOnboardingControllerOptions

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

* @param password - The password to verify.
* @throws {Error} If the password is invalid or the vault is not initialized.
*/
async #verifyPassword(password: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async #verifyPassword(password: string) {
async #verifyVaultPassword(password: string) {

@@ -94,6 +130,8 @@ export class SeedlessOnboardingController extends BaseController<

readonly toprfClient: ToprfSecureBackup;

#password?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we storing password here? Does keyring controller also does this?

}
await this.#vaultEncryptor.decrypt(password, this.state.vault);
}

/**
* Unlocks the encrypted vault using the provided password and returns the decrypted vault data.
* This method ensures thread-safety by using a mutex lock when accessing the vault.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function description needs to be updated since we don't provide password anymore as arg

@@ -423,25 +492,58 @@ export class SeedlessOnboardingController extends BaseController<
* - The password is incorrect (from encryptor.decrypt)
* - The decrypted vault data is malformed
*/
async #unlockVaultWithPassword(password: string): Promise<{
async #unlockVaultAndGetBackupEncKey(): Promise<{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be rather better to have password as optional param here, rather thn loading it from state inside the function

});

this.#password = password;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we are setting vaultEncryptionKey above, do we really need to set password as well in class variable, wht the case when vaultEncryptionKey might be missing but password would be required instead?

encryptedString: EncryptionResult,
) => Promise<unknown>;
/**
* Generates an encryption key from exported key string.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Generates an encryption key from exported key string.
* Generates an encryption key from imported key string.

/**
* Generates an encryption key from exported key string.
*
* @param key - The exported key string.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param key - The exported key string.
* @param key - The imported key string.

mikesposito and others added 3 commits May 13, 2025 11:09
## Explanation

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->
When the user vault is decrypted and there is an attempt to restore an
unsupported/deprecated/faulty keyring there's no mechanism to remove
related metadata, which leads to a situation where no further action can
be made on the controller, because checks for keyrings and metadata
length will fail.

We could remove the related metadata object when the keyring restore
fails, but then we would lose the original ID generated for the keyring.
We can, instead, change the place where the metadata is stored from a
state property to the encrypted vault: by placing the metadata along
with its serialised keyring in the vault we can guarantee a 1:1 link
between them while being able to keep metadata for unsupported keyrings.

Given that we don't need to use the KeyringController state to persist
metadata anymore (as it is persisted along with the vault), we can also
remove `keyringsMetadata` completely, and add a `metadata` attribute to
each keyring in `state.keyrings` instead - which won't be persisted, as
it will be recreated at runtime every time the vault is decrypted and
the keyrings are deserialised.

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->
* Fixes #5701

## Changelog

<!--
THIS SECTION IS NO LONGER NEEDED.

The process for updating changelogs has changed. Please consult the
"Updating changelogs" section of the Contributing doc for more.
-->

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

---------

Co-authored-by: Mark Stacey <[email protected]>
#5788)

## Explanation

This PR moves a changelog entry from **13.0.0** to **Unreleased** for
`@metamask/profile-sync-controller`.
This entry was mistakenly placed in an already released version's
changelog.

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->

## Changelog

<!--
THIS SECTION IS NO LONGER NEEDED.

The process for updating changelogs has changed. Please consult the
"Updating changelogs" section of the Contributing doc for more.
-->

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
## Explanation

This is a RC for v393.0.0. See changelog for more details

- `@metamask/[email protected]`

## References

Instructions for client migration are in these test drive PRs:

- ✅ Extension test drive PR:
MetaMask/metamask-extension#32572
- ✅ Mobile test drive PR:
MetaMask/metamask-mobile#15211

## Changelog

```ms
### Changed

- Bump `@metamask/profile-sync-controller` from `^13.0.0` to `^14.0.0` ([#5789](#5789))
```

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
@lwin-kyaw lwin-kyaw force-pushed the feat/seedless-multi-srp branch from 8578a48 to 2136c32 Compare May 13, 2025 12:55
Copy link

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updated@​metamask/​profile-sync-controller@​13.0.0 ⏵ 14.0.091 +110086100 +1100

View full report

Copy link

Caution

Review the following alerts detected in dependencies.

According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.

Action Severity Alert (click for details)
Block Medium
@metamask/[email protected] has Network access.

Module: globalThis["fetch"]

Location: Package overview

From: packages/notification-services-controller/package.jsonnpm/@metamask/[email protected]

ℹ Read more on: This package | This alert | What is network access?

Next steps: 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 [email protected].

Suggestion: Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/@metamask/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@lwin-kyaw lwin-kyaw closed this May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants