Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: revamp user storage encryption process #4981

Merged
merged 15 commits into from
Dec 3, 2024
Merged

feat: revamp user storage encryption process #4981

merged 15 commits into from
Dec 3, 2024

Conversation

mathieuartu
Copy link
Contributor

@mathieuartu mathieuartu commented Nov 26, 2024 •
edited
Loading

Explanation

This PR adds new steps when both UserStorageController and SDK performs:

  • getUserStorageAllFeatureEntries
  • getUserStorage

This steps looks for fetched entries' salts and, if random salts are found, re-encrypts these entries with a constant salt and uploads them back to user storage.

This PR also removes the salt randomness when generating the keys, by adding a new shared salt.

This is done to prevent performance issues when decrypting multiple entries that have different salts in applications using UserStorageController / SDK.

References

Changelog

@metamask/profile-sync-controller

  • CHANGED: Stop using a random salt when generating scrypt keys and use a shared one
  • ADDED: Re-encrypt data fetched by getUserStorageAllFeatureEntries and getUserStorage with the shared salt if fetched entries were encrypted with random salts

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 highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@mathieuartu mathieuartu added the team-identity Identity Team changes. https://github.com/orgs/MetaMask/teams/identity label Nov 26, 2024
@mathieuartu mathieuartu self-assigned this Nov 26, 2024
@mathieuartu mathieuartu marked this pull request as ready for review November 26, 2024 12:32
@mathieuartu mathieuartu requested review from a team as code owners November 26, 2024 12:32
@mathieuartu mathieuartu changed the title feat: re-encrypt batchGet results when they have different salts feat: revamp user storage encryption process Nov 29, 2024
Prithpal-Sooriya
Prithpal-Sooriya reviewed Nov 29, 2024
View reviewed changes
packages/profile-sync-controller/src/shared/encryption/encryption.ts Outdated
@@ -210,7 +236,7 @@ class EncryptorDecryptor {
};
}

const newSalt = salt ?? randomBytes(SCRYPT_SALT_SIZE);
const newSalt = salt ?? new Uint8Array(SCRYPT_SALT_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you validate both Scrypt KDFs work with an empty salt size?

Copy link
Contributor

Choose a reason for hiding this comment

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

FUTURE - This is a missing test on mobile, but I would like us to add a test to assert that both scrypt functions generate the exact same encrypted data (to avoid the issue we had early on)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you validate both Scrypt KDFs work with an empty salt size?

I CAN! 😄

Copy link

Choose a reason for hiding this comment

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

I'd argue that if we want to avoid running into multiple salts we can hardcode a salt here instead of fiddling with the size.
I'm also concerned about the behavior of different libraries when given empty salts

Prithpal-Sooriya
Prithpal-Sooriya reviewed Nov 29, 2024
View reviewed changes
packages/profile-sync-controller/src/controllers/user-storage/services.test.ts

mockGetUserStorageAllFeatureEntries.done();
mockBatchUpsertUserStorage.done();
expect(result).toStrictEqual(['data1', 'data2', MOCK_STORAGE_DATA]);
Copy link
Contributor

Choose a reason for hiding this comment

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

FUTURE - this tests have a lot of repetition, it would be nice to clean up these up so the tests clearly show us what we are testing instead of needing to read all this setup/acting.

@mathieuartu mathieuartu marked this pull request as draft November 29, 2024 16:20
Prithpal-Sooriya
Prithpal-Sooriya reviewed Dec 2, 2024
View reviewed changes
packages/profile-sync-controller/src/sdk/user-storage.ts
@@ -200,6 +201,47 @@ export class UserStorage {
}
}

async #batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries(
Copy link
Contributor

Choose a reason for hiding this comment

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

Future - I don't like how we've shoved all the user storage logic in a class, this bloats the SDK class and makes it harder to test things individually.

Similar to the authentication SDK, we should split out services from the user storage SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Prithpal-Sooriya Agreed! Added that to my to-do list!

Prithpal-Sooriya
Prithpal-Sooriya previously approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya left a comment

Choose a reason for hiding this comment

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

LGTM.

Test this on mobile before merging. I want to ensure that both JS and Native Scrypt will generate same key / same encrypted payload.

mirceanis
mirceanis requested changes Dec 2, 2024
View reviewed changes
Copy link

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Overall looks good.

There's a spot where the comparison is still done by salt.length instead of comparing to the shared salt.

Once that's fixed it should be good to go.

packages/profile-sync-controller/src/sdk/user-storage.ts Outdated

// Re-encrypt the entry if it was encrypted with a random salt
const salt = encryption.getSalt(encryptedData);
if (salt.length) {
Copy link

Choose a reason for hiding this comment

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

This is missing a comparison to the SHARED_SALT

Suggested change
if (salt.length) {
if (salt.toString() !== SHARED_SALT.toString())

Copy link

Choose a reason for hiding this comment

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

I wonder if it's difficult to cover this in a test too

mirceanis
mirceanis previously approved these changes Dec 2, 2024
View reviewed changes
Copy link

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

looks good

mirceanis
mirceanis requested changes Dec 3, 2024
View reviewed changes
Copy link

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

I disagree with the approach to delete the data that fails to decrypt.
Such failures can happen for multiple unforeseen reasons so any such destructive operation should be guarded much more thoroughly.

If we intend to avoid spending time in KDF on seemingly corrupted entries perhaps an alternative approach can be considered: marking the corrupted entries locally so that decryption attempts are stopped before KDF is triggered.

packages/profile-sync-controller/src/controllers/user-storage/services.ts Outdated
return decryptedData;
} catch {
// If the data cannot be decrypted, delete it from user storage
await deleteUserStorage(opts);
Copy link

Choose a reason for hiding this comment

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

To me this is a foot gun.
Exceptions during decryption can happen in situations we can't predict now.

Also, this delete operation can be invoked if there's any other problem obtaining the salt, or deriving the decryption key, or there's any error during upsert, including any network error.

packages/profile-sync-controller/src/controllers/user-storage/services.ts Outdated
@@ -173,7 +181,8 @@ export async function getUserStorageAllFeatureEntries(
]);
}
} catch {
// do nothing
// If the data cannot be decrypted, delete it from user storage
entriesToDelete.push(entry.HashedKey);
Copy link

Choose a reason for hiding this comment

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

same unintentional failure scenarios as with getUserStorage().
The try block is too broad

mirceanis
mirceanis approved these changes Dec 3, 2024
View reviewed changes
Copy link

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks good again :)

mirceanis
mirceanis reviewed Dec 3, 2024
View reviewed changes
packages/profile-sync-controller/src/controllers/user-storage/services.ts
nativeScryptCrypto,
),
]);
}
} catch {
// do nothing
Copy link

Choose a reason for hiding this comment

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

Let's address this in a separate issue.

@mathieuartu mathieuartu marked this pull request as ready for review December 3, 2024 18:42
@mathieuartu mathieuartu merged commit a6937e3 into main Dec 3, 2024
120 checks passed
@mathieuartu mathieuartu deleted the feat/re_encrypt_data_if_salts_different branch December 3, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirceanis mirceanis mirceanis approved these changes

@Prithpal-Sooriya Prithpal-Sooriya Prithpal-Sooriya left review comments

Assignees

@mathieuartu mathieuartu

Labels
team-identity Identity Team changes. https://github.com/orgs/MetaMask/teams/identity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mathieuartu @mirceanis @Prithpal-Sooriya