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.

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

metadata encryption v2 (confirm-less labeling) #9130

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Aug 8, 2023

This PR implements "confirm-less labeling" feature. Previously, you needed to confirm "Enable labeling" dialogue on device screen. This is not needed anymore, or, more specifically, after this is merged the aforementioned dialogue will appear only once (in optimistic scenario).

Changes:

  • introduced concept of encryption versions
  • new filenames have encryption version suffixes
  • introduced "dummy data" when a new file is created.
  • introduced "rename" method to metadata providers
  • metadata migration procedure.

image

image

Migration procedure

  1. select lower encryption version
  2. in general, metadata related actions are per device as everything is encrypted by device.metadata[version].key
  3. fetch list of all files saved within currently selected provider for labeling. based on file suffix we are able to determine which files are associated with which encryption version.
  4. if there are no old files, there is nothing to migrate
  5. there are old files, but also all labelable entities currently known to suite have some record in files containing current extension suffix fetched in point 3. this means that they have already been migrated or a dummy file was created. also note that when we migrate a file, we rename the old file from <filename.mtdt> to <filename_v1.mtdt> which increases probability of stopping on point 4.
  6. now we know that migration is needed.
  7. sync metadata keys for prev encryption version. NOTE: result of this operation is saved for device and account encryption keys are computed from it. This means that we can add new accounts at any point of time later without calling this again
  8. get labelable entitites again (there was async operation in between)
  9. split labelable entities into 2 groups: entitiesToMigrate : don't have new file && have old file entititiesToCreateDummies: don't have new file && don't have old file
  10. now all data is ready. we know what operations should be carried out. dummy files will be created, old files will be migrated and renamed and their content will be filled into local state

Notes

  • in case user aborts migration (cancels enable labeling on device). or simply migration fails for other user unerelated reasons. should we ignore migration (which could cause user losing data) or should we disable labeling in that case? At this moment we disable labeling. We should be aware also about cases when user enabled labeling, went thourgh migration and discovered labelable entity later on and disabled labeling on device.
  • what about some UI, migration might take some time (under current logic design). should we communicate this with users somehow?

To test

  • not only happy path but also cases when: user does not connect to cloud provider, user does not confirm migration on device, etc
  • all other, not obvious places where labeling is used: send form, export, import csv in send form.
  • all providers, also look how files are changing inside them.
  • also might be worth to think about application version glitches - what if user does migration in one suite and opens an old suite later and does some changes there? not saying we support every edgecase scenario here, it just must not end up with some unwanted result.

Related

Followups:

  • racecondition alert ⚠️ - we poll metadata from provider && we create dummy files - in some cases it may happen that I am editing label and at the same time I receive update from provider which overwrites me. This is in general not a new issue.
  • ratelimiting on metadata providers layer. now this is solved in metadata actions which is not right.

@mroz22 mroz22 force-pushed the metadata-migrations-meows branch 3 times, most recently from ab1b36a to 25c4479 Compare August 9, 2023 14:52
@mroz22 mroz22 force-pushed the metadata-migrations-meows branch 3 times, most recently from ca52c1b to 97759e2 Compare August 22, 2023 08:03
@mroz22 mroz22 mentioned this pull request Aug 22, 2023
@mroz22 mroz22 force-pushed the metadata-migrations-meows branch 2 times, most recently from ed90995 to e016e27 Compare August 22, 2023 09:48
@mroz22 mroz22 force-pushed the metadata-migrations-meows branch 4 times, most recently from 4379e53 to 1b962bf Compare August 30, 2023 16:17
@mroz22 mroz22 changed the title [WIP]: metadata migrations 2 metadata encryption v2 (confirm-less labeling) Aug 30, 2023
@mroz22 mroz22 marked this pull request as ready for review August 30, 2023 16:18
Comment on lines +194 to +196
migratedFrom?: string;
/**
* we might create some dummy data to make it hard to guess number of labels by file size
*/
dummy?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we also bump version field saved in file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

might make sense to keep track of this dac3416

@mroz22 mroz22 force-pushed the metadata-migrations-meows branch 6 times, most recently from e4441ba to 53ec390 Compare October 24, 2023 07:48
@matejkriz matejkriz added the labelling Naming of transactions and accounts label Oct 24, 2023
@matejkriz
Copy link
Member

deduplication of labelable entities is needed, because i.e. ADA, XRP and TSEP TGOR reuse the same xpub with testnet so renaming files currently throw an error
image
image

@matejkriz
Copy link
Member

check how it behaves if user have also some legacy files in apps/trezor/apps/trezor, see https://github.com/trezor/trezor-suite/blob/metadata-migrations-meows/packages/suite/src/services/suite/metadata/DropboxProvider.ts/#L129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
labelling Naming of transactions and accounts
Projects
No open projects
Status: 🧊 Freezer
Development

Successfully merging this pull request may close these issues.

None yet

3 participants