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

metadata: encryption version migration #8825

Closed

mroz22 wants to merge 6 commits into metadata-new-structure from metadata-migrations

Contributor

mroz22 commented Jun 28, 2023 •

edited by dahaca

Loading

Based on #8646 which is a PR that introduces changes to metadata data model in reducers. That PR is basically ready to be merged but as it does not bring any new features I am hesitant to do it. Also I dont want to bloat it with more changes so I decided to build on it instead. I am also stealing some ideas from #8567

Commits

1st commit - is basically fixup for chore(metadata): new data structure in suite #8646 I realized that device.metadata.key should also be versioned as it determines account.metadata.aesKey and account.metadata.fileName. This fixup is incomplete, there are also some changes in the second commit that should go here.
2nd commit -implements metadata migration and resolves Don't ask Trezor for enable labelling every session #6304
3rd commit does not exist yet. I would definitely add at least e2e test from here feat(suite): labeling improvements #8567 and whatevere else might be useful

Todos:

unit tests
e2e tests
couple of todos and areas for improvements

How does it work

user tries to add a label, initation process kicks in as usual
once data provider is connected and metadata keys (for the latest encryption version) are derived we fetch list of all files from data provider
if all labelable entities (currently discovered) have associated file in what we fetchted in 2 no further action is needed
else we separate labelable entities that don't have their counterpart in dataprovider and derive legacy keys for them
then we fetch legacy files from data provider
we decrypt legacy files using legacy keys, load them into application memory and encrypt them using new keys and upload them to data provider.
if there was no legacy file match in 5, we create dummy files and upload them as well. this ensures that next time metadata is initiated, we are likely to be able to stop at point 3 (unless user created a new account in the meantime somewhere else)

Testing tips

create an empty account on dropbox
go to https://suite.trezor.io/web
create some labels
open build of this branch
enable labeling
labels should be there
forget device, enable labeling again. you should not be prompted on device. labels should still be there

Drawbacks

we can not migrate all labels at once because we simply can't be sure we know them. Labels are bound to 'labelable entities' which are now accounts and wallets. So whenever user adds a labelable entity, we must run "metadata discovery" including migration if needed. So it is very well possible that user might migrate his data on day, they might be using suite without "enable labeling prompt" for years, and then, after adding some long unused passphrase or account, user will need to confirm labeling on device again.
possibility of mismatch between versions of suite. after this is release user opens updated version of suite, migrates data, then opens outdated version of suite and works with labeling. Under current implementation labels added in the outdated version will be lost.

mroz22 force-pushed the metadata-new-structure branch from 4f244e0 to b5ed29f Compare

June 29, 2023 15:12

mroz22 force-pushed the metadata-migrations branch 2 times, most recently from bf8966e to ea07c7a Compare

June 30, 2023 15:40

mroz22 changed the title ~~[wip] metadata: next steps~~ [wip] metadata: encryption version migration

mroz22 force-pushed the metadata-migrations branch 2 times, most recently from 37254af to 8bbad83 Compare

July 7, 2023 09:15

dahaca reviewed

View reviewed changes

packages/suite/src/actions/suite/metadataActions.ts Outdated Show resolved Hide resolved

packages/suite/src/actions/suite/metadataActions.ts Outdated Show resolved Hide resolved

packages/suite/src/actions/suite/metadataActions.ts Outdated Show resolved Hide resolved

packages/suite/src/actions/suite/metadataActions.ts Outdated Show resolved Hide resolved

packages/suite/src/actions/suite/metadataActions.ts Show resolved Hide resolved

packages/suite/src/actions/suite/metadataActions.ts Outdated Show resolved Hide resolved

packages/suite/src/actions/suite/metadataActions.ts Show resolved Hide resolved

mroz22 force-pushed the metadata-new-structure branch from b5ed29f to 26f0684 Compare

July 11, 2023 11:56

dahaca force-pushed the metadata-migrations branch from 8bbad83 to 905904b Compare

July 12, 2023 17:04

mroz22 commented

View reviewed changes

packages/suite/src/actions/suite/metadataActions.ts

Comment on lines +354 to +360

+                      if (entity.type === 'device' && entity.status !== 'enabled') {
+                          return;
+                      }

Contributor Author

mroz22 Jul 17, 2023

why this? sometimes not fetch?

Contributor

dahaca Jul 17, 2023

this is for TS. but basically, what would you fetch if you don't have metadata enabled? 🤔

mroz22 commented

View reviewed changes

packages/suite/src/actions/suite/metadataActions.ts

Comment on lines +358 to +365

+                      const entityData = entity[encryptionVersion];
+                      if (!entityData) {
+                          return;
+                      }

Contributor Author

mroz22 Jul 17, 2023

this makes me think "what does entityData mean?"
what about just?

if (!entity[encryptionVersion]) return

Contributor

dahaca Jul 20, 2023

but then there is const { fileName, aesKey } = entityData; on the next line anyways

mroz22 commented

View reviewed changes

packages/suite/src/actions/suite/metadataActions.ts Outdated

Comment on lines 957 to 964

+                          const defaultEntityData =
+                              entity.type === 'account'
+                                  ? ({
+                                        accountLabel: '',
+                                        outputLabels: {},
+                                        addressLabels: {},
+                                    } as AccountLabels)
+                                  : ({ walletLabel: '' } as WalletLabels);

Contributor Author

mroz22 Jul 17, 2023

just a nitpick to think about. I think we are already defining this on a couple of places, maybe it could go to some single point of definition

Contributor

dahaca Jul 20, 2023

mroz22 commented

View reviewed changes

packages/suite/src/actions/suite/metadataActions.ts Outdated

Comment on lines 993 to 977

+                  if (!filesToMigrate || !filesToMigrate.length) {
+                      return;
+                  }

Contributor Author

mroz22 Jul 17, 2023

nitpick according to m personal taste

!filesToMigrate?.length

Contributor

dahaca Jul 20, 2023

dahaca force-pushed the metadata-migrations branch from c920d4e to 70414f3 Compare

July 21, 2023 14:02

dahaca force-pushed the metadata-new-structure branch 2 times, most recently from c6a3cb9 to 6736de5 Compare

July 21, 2023 15:08

dahaca force-pushed the metadata-migrations branch 2 times, most recently from ab6f1f0 to 913bf8a Compare

July 24, 2023 12:39

dahaca marked this pull request as ready for review

July 24, 2023 12:39

dahaca requested review from marekrjpolak, tomasklim and matejkriz as code owners

July 24, 2023 12:39

dahaca changed the title ~~[wip] metadata: encryption version migration~~ metadata: encryption version migration

mroz22 and others added 5 commits

July 24, 2023 17:50


          fixup: change to data structure

992274a


          feat: metadata encryption migration

4d701ce


          feat(suite-web): add e2e tests for metadata migration

07c738f


          feat(suite): remove the cancelled metadata state from wallets

41e989a


          fix(suite): fix metadata actions test after state structure changes

0dbbe39

dahaca force-pushed the metadata-migrations branch from 913bf8a to 0dbbe39 Compare

July 24, 2023 15:50

matejkriz self-assigned this

dahaca added the labelling label


          fixup! feat: metadata encryption migration

389722d

mroz22 mentioned this pull request

metadata encryption v2 (confirm-less labeling) #9130

Open

7 tasks

mroz22 closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dahaca dahaca left review comments

marekrjpolak Awaiting requested review from marekrjpolak marekrjpolak is a code owner

tomasklim Awaiting requested review from tomasklim tomasklim is a code owner

matejkriz Awaiting requested review from matejkriz matejkriz is a code owner