-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Desktop: Use Electron safeStorage
for keychain support
#10535
Conversation
@personalizedrefrigerator - thx for this pull! Glad to see this change. But, |
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.
…d .electron files" This reverts commit 44b2644.
Running Electron with the NodeJS version installed through the Snap Store seems to break keychain storage.
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.
} | ||
|
||
public async setPassword(name: string, password: string): Promise<boolean> { | ||
if (canUseSafeStorage()) { |
There was a problem hiding this comment.
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.
The goal of this change is to prevent secrets from becoming inaccessible if, for example, a user upgrades to the beta desktop release, then reverts to the stable release.
Summary
This PR uses Electron
safeStorage
when available and falls back tokeytar
.On desktop, fixes #8829.
May also fix #10526.
This pull request:
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
KvStore
.safeStorage
doesn't seem to provide key-value storage. Instead, it provides methods that encrypt or decrypt data.settings
table, using theencryptedValue.
key prefix. For example, a setting with keysync.6.password
would have its encrypted value stored asencryptedValue.sync.6.password
.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 existingsettings
table.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).safeStorage
.Manual testing
Notes:
keytar
tosafeStorage
(with mockedkeytar
andsafeStorage
).Ubuntu 24.04:
.AppImage
version of Joplin..AppImage
version of Joplin.test1
(or some other short string)..AppImage
.Windows 11:
C:\Users\User\.config\joplin-desktop\
.a
).personalizedrefrigerator:pr/desktop/use-electron-safestorage
.MacOS Sonoma:
cd packages/app-desktop && yarn dist --publish=never && open dist/Joplin-3.*.dmg
).Past manual testing with earlier commits (before a refactor)
So far, limited manual testing has been done on Ubuntu 24.04:
Setting.resetKey('keychain.supported')
from Joplin's development tools, then restart Joplin.key
.KeychainService: check was already done - skipping. Supported: 1
appears in the console.Additional manual testing will be necessary on Windows and MacOS.
Additional testing done on Windows:
personalizedrefrigerator:pr/desktop/use-electron-safestorage
.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.