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

Desktop: Use Electron safeStorage for keychain support #10535

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Jun 3, 2024

Summary

This PR uses Electron safeStorage when available and falls back to keytar.

On desktop, fixes #8829.
May also fix #10526.

This pull request:

  1. Adds keychain support on Linux.
  2. This may help address a keychain-related bug (from my very limited testing).

The CLI app doesn't have access to Electron APIs, and so will still need to use keytar after this pull request.

Important

This pull request does not increase Joplin's database/profile version. As a result, users that upgrade, then downgrade Joplin and will lose access to secure settings until Joplin is upgraded again. If desired, this can be fixed by adding an empty migration file.

To-do

  • Decide where to store encrypted settings. Using KvStore.
    • Electron's safeStorage doesn't seem to provide key-value storage. Instead, it provides methods that encrypt or decrypt data.
    • Currently, encrypted secure settings are stored in the settings table, using the encryptedValue. key prefix. For example, a setting with key sync.6.password would have its encrypted value stored as encryptedValue.sync.6.password.
    • It could make sense to store these settings separately from the database (e.g. in an encrypted-settings.json file). This could make it clear that these settings need special care when creating a backup of Joplin (if backing up by copying the database). It would also avoid reusing the existing settings table.
  • Decide whether this should be enabled for all platforms initially, or just Linux.
  • Decide what to do if keychain support is temporarily unavailable.
  • Migrations
    • On Linux, keychain.supported needs to be reset to -1 on existing installs for the keychain check to be re-run (allowing the keychain to be used).
    • Migrate existing settings away from keytar?
    • Migrate existing unencrypted settings to safeStorage.
  • Test on more platforms.
    • Ubuntu 24.04
    • Windows (testing done with a recent, but not the latest, commit)
    • MacOS

Manual testing

Notes:

  • This pull request includes an automated test that tests migrating secure settings from keytar to safeStorage (with mocked keytar and safeStorage).
  • The below manual testing was last done prior to 6646a6a. 6646a6a removes the code that deletes keys from the old storage driver. See the commit for details.

Ubuntu 24.04:

  1. Build a release .AppImage version of Joplin.
  2. Run an older .AppImage version of Joplin.
  3. Enable password protected backups.
    • Set the password and password confirmation to test1 (or some other short string).
  4. Enable encryption
  5. Quit Joplin
  6. Run the new .AppImage.
  7. Go to settings > encryption > manage master password
  8. Verify that the master password setting can still be acessed.
  9. Verify that password protected backups are enabled.
  10. Create a backup using tools > "create backup".
  11. Verify that keychain support is enabled using "Help" > "About Joplin".
  12. Restart Joplin.
  13. Repeat steps 9-12.

Windows 11:

  1. Install Joplin v3.0.11.
  2. Delete C:\Users\User\.config\joplin-desktop\.
  3. Delete all Joplin settings from Windows Credential Manager > Windows credentials.
  4. Enable encryption.
  5. Enable password protected backups (important -- the password will be reset without this).
  6. Change the backup password to something very short (e.g. a).
  7. Change the backup password repeat to the same password entered in step 6.
  8. Save settings & quit Joplin.
  9. Open Windows Credential Manager > Windows credentials (refresh if already open).
  10. Verify that 3 Joplin settings are present.
  11. Build and install Joplin from personalizedrefrigerator:pr/desktop/use-electron-safestorage.
  12. Start Joplin.
  13. Refresh Windows Credential Manager.
  14. Verify that only no Joplin settings are present.
  15. Open Joplin again.
  16. Verify that the Joplin backup password is still present and has the same length as it did in step 6.
  17. Verify that the master key is still loaded (no "master key password is missing" banner visible).

MacOS Sonoma:

  1. On an existing Joplin installation.
  2. Go to "Joplin > About" and verify that "Keychain Supported" is listed as "Yes".
  3. Go to settings > backup and enable password protected backups (enter and confirm password, too).
  4. Create a backup & verify that this completes.
  5. Verify that sync is enabled with a password.
    • For me, I had previously set up sync with Joplin Cloud.
  6. Build and install a version of Joplin from this pull request (using a command similar to cd packages/app-desktop && yarn dist --publish=never && open dist/Joplin-3.*.dmg).
  7. Start Joplin and verify that "Keychain Supported" is still listed as "Yes".
    • Note: I had previously tested this with a different version of Joplin. Perhaps because of this, Joplin requested access to the "Joplin Safe Storage" item in the keychain on startup.
  8. Go to settings and verify that password protected backups are still enabled.
  9. Sync -- verify that sync completes successfully.
  10. Change the backup password to something with a visibly different length and save.
    • It should be the same in both fields (password & confirm).
  11. Restart Joplin.
  12. Verify that the backup password is still very long.
  13. Verify that "Keychain supported" is still listed as "yes".
Past manual testing with earlier commits (before a refactor)

So far, limited manual testing has been done on Ubuntu 24.04:

  1. Run Setting.resetKey('keychain.supported') from Joplin's development tools, then restart Joplin.
  2. Restart Joplin
  3. Open Joplin's devtools (if not already open -- I tested in development mode) and search for key.
  4. Verify that KeychainService: check was already done - skipping. Supported: 1 appears in the console.
  5. Change the password for the backup plugin and save.
  6. Verify that no new errors are logged to the console.

Additional manual testing will be necessary on Windows and MacOS.


Additional testing done on Windows:

  1. Install Joplin v3.0.11.
  2. Enable encryption.
  3. Change the backup password.
  4. Save settings & quit Joplin.
  5. Open Windows Credential Manager.
  6. Verify that 3 Joplin settings are present.
  7. Build and install Joplin from personalizedrefrigerator:pr/desktop/use-electron-safestorage.
  8. Start Joplin.
  9. Refresh Windows Credential Manager.
  10. Verify that only two Joplin settings are present.
  11. Verify that the Joplin backup password is still present.
  12. Verify that the master key is still loaded (no "master key password is missing" banner visible).

Note: At present, the migration step does not migrate secure plugin settings from keytar to safeStorage. This is because the migration currently runs before plugins (and thus their settings) are loaded. At present, these secure plugin settings remain stored in keytar until manually changed by the user.

@personalizedrefrigerator personalizedrefrigerator marked this pull request as draft June 3, 2024 16:50
@djsudduth
Copy link

djsudduth commented Jun 3, 2024

@personalizedrefrigerator - thx for this pull! Glad to see this change. But, keytar needs to eventually be replaced in the CLI somehow since it's repo has been archived.

Before this pull request, Linux systems lacked keychain support. This
commit adds a migration that resets `keychain.supported` to -1, allowing
the keychain support check to be re-run.

At present, this migration just re-runs the keychain support check. It
does not remove or modify existing secure settings previously stored
in the database.
@laurent22 laurent22 added the v3.1 label Jun 10, 2024
@personalizedrefrigerator personalizedrefrigerator marked this pull request as ready for review July 1, 2024 22:34
@personalizedrefrigerator personalizedrefrigerator marked this pull request as draft July 1, 2024 23:36
This commit:
- Removes the need for a new database migration.
- Simplifies adding and removing new KeychainService drivers.
- Should (untested) fix migrating secure settings for plugins.
@personalizedrefrigerator personalizedrefrigerator marked this pull request as ready for review July 2, 2024 01:25
}

public async setPassword(name: string, password: string): Promise<boolean> {
if (canUseSafeStorage()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, canUseSafeStorage is checked just before each attempt to use safeStorage. The goal of this is to handle the case where keychain support becomes unavailable, when previously available. This might not be necessary: safeStorage uses the keychain only to store an encryption key. I'm unsure if canUseSafeStorage can become false when previously true without restarting the app.

@laurent22 laurent22 merged commit 08eab7a into laurent22:dev Aug 8, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugins: settings fields that are secure are unable to be set on keychain Find alternative to keytar package
3 participants