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

Mobile,Desktop: Resolves #10856: Allow disabling master keys to improve startup performance #10943

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Aug 29, 2024

Summary

This pull request:

  1. Allows disabling master keys on mobile.
  2. Skips decrypting disabled master keys on startup.
  3. Reloads master keys on mobile in more cases to match the CLI and desktop app's behavior.
    • This was necessary for clicking "enable" on a master key to cause it to load.

Note

The encryption config screen with many master keys may still be very slow (usePasswordChecker seems to check passwords for both enabled and disabled keys). A possible fix can be found here.

Screenshot

screenshot: Disable/enable buttons are included after master keys

Testing plan

Web:

  1. Setup (partially done before fixing the issue): Open the web client on two different browsers (Firefox and Chromium). Enable encryption on both (use different master passwords). Connect both to the same Joplin Server account.
  2. Sync. For each client, go to settings and verify that two master keys are visible.
  3. On Chrome:
    • Disable both.
    • Create a note.
    • Click "sync".
    • Verify that sync fails with an error: "Could not encrypt item [...] master key is disabled":
      screenshot: Encryption error message below the sync icon
    • Enable one of the master keys.
    • Click "sync".
    • Verify that sync completes successfully.
  4. On Firefox, if not already done, enter the password for the remote master key and click save.
    • Note: While testing locally, I then refreshed the page to apply the latest changes made locally.
  5. Still on Firefox,
    • Sync.
    • Verify that sync completes successfully.

The above steps were last done on this commit which is not the latest in this pull request.

Desktop:

  1. Create a new profile in Joplin desktop.
  2. Start a sync with the Joplin Server account used for the web tests above.
  3. Verify that, during sync, a "One or more master keys need a password" banner is visible.
  4. Click "Set the password".
  5. Enter the passwords for the keys.
    • Existing bug: Even after entering the correct password, the red borders around password inputs are still visible: screenshot: Red borders around some password inputs
  6. Disable one of the master keys (one that does not have "password" set to "master password").
  7. Restart Joplin.
  8. Go back to encryption settings. Verify that the just-disabled key is still listed as disabled.
  9. Allow sync to finish.

Android 13:

  1. Connect to the same Joplin Server account used for the web and desktop tests above.
  2. Start sync.
  3. Verify that a "Press to set the decryption password" banner is visible.
  4. Press "Press to set the decryption password".
    • For me, this was done while still syncing.
  5. Verify that 3 master keys are listed, each without a password.
  6. Enter a password for one of the master keys.
  7. Enter a password for one of the other master keys.
  8. Verify that the passwords are accepted/listed as saved.
    • I encountered the issue described in this comment at this point.
  9. Disable one of the master keys.
  10. Verify that its button's content changes from "DISABLE" to "ENABLE".
  11. Close the encryption config screen.
  12. Verify that items are decrypting.

…isabled master keys, allow disabling master keys on mobile
@personalizedrefrigerator personalizedrefrigerator changed the title Mobile,Desktop: Allow disabling master keys to more significantly improve startup performance Mobile,Desktop: Allow disabling master keys to improve startup performance Aug 29, 2024
// Like the desktop and CLI apps, we run this whenever the sync target properties change.
// Previously, this only ran when encryption was enabled/disabled. However, after fetching
// a new key, this needs to run.
if ((action.type === 'SETTING_UPDATE_ONE' && action.key === 'syncInfoCache') || (action.type === 'SETTING_UPDATE_ALL')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: Similar logic in BaseApplication.ts had a similar change applied in 2021.

@personalizedrefrigerator
Copy link
Collaborator Author

Converting to a draft — there seem to be issues with "invalid password" errors in some cases.

@personalizedrefrigerator personalizedrefrigerator marked this pull request as draft August 29, 2024 22:36
@personalizedrefrigerator personalizedrefrigerator marked this pull request as ready for review August 29, 2024 23:39
@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Aug 29, 2024

The issue seems unrelated to this pull request. Instead, it seems to be caused by setting the same password for both the active master key (key A, correct password) and a secondary master key (key B, incorrect password). As such, I'm marking this pull request as ready for review. A fix for this issue, which I plan to include in a separate pull request, can be found here.

@personalizedrefrigerator personalizedrefrigerator changed the title Mobile,Desktop: Allow disabling master keys to improve startup performance Mobile,Desktop: Resolves #10856: Allow disabling master keys to improve startup performance Aug 30, 2024
@laurent22
Copy link
Owner

The logic for disabled master keys is that they can't be used for encryption, and they are hidden in the list of keys. However it should still be possible to use them for decrypting data. For example if the user disable a key, then sync, and some of the notes were encrypted with this now disabled key, it should still be used for decryption.

I'm wondering if with this change this would still happen?

@personalizedrefrigerator
Copy link
Collaborator Author

For example if the user disable a key, then sync, and some of the notes were encrypted with this now disabled key, it should still be used for decryption.

I'm wondering if with this change this would still happen?

Thank you for noticing this! No, it wouldn't happen with this change.

A possible alternative could be to store whether a key has an incorrect password and, if so, skip trying to decrypt it on startup. This has the advantage of not changing how disabled keys affect sync. If users have a large number of master keys with invalid passwords, this would improve startup performance (for me, 5 of my 6 master keys are unused and have incorrect passwords).

@personalizedrefrigerator
Copy link
Collaborator Author

See #10990

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.

Too many master keys slowing down iOS/iPadOS app startup tremendously
2 participants