From 83ceec25e66ca967847401c910a61b7ff3737881 Mon Sep 17 00:00:00 2001 From: Henry Heino Date: Thu, 17 Oct 2024 10:13:47 -0700 Subject: [PATCH 1/2] Desktop: Linux: Move keychain support behind an off-by-default feature flag This commit is in response to a failure to decrypt saved keychain secrets on Fedora 40. It makes the following changes: - Makes the keychain read-only just after loading settings UNLESS the user has opted in to keychain support through a feature flag. - Updates versionInfo to not display Yes for "Keychain supported" if the keychain is in read-only mode. With the keychain in read-only mode, new secrets will be saved to the database (as they were in v3.0) but old secrets can still be read from the KeychainDriver. --- packages/lib/models/settings/builtInMetadata.ts | 14 ++++++++++++++ packages/lib/services/SettingUtils.ts | 7 +++++++ packages/lib/versionInfo.ts | 13 ++++++++++++- 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/lib/models/settings/builtInMetadata.ts b/packages/lib/models/settings/builtInMetadata.ts index e822d3689ce..0482f4fb483 100644 --- a/packages/lib/models/settings/builtInMetadata.ts +++ b/packages/lib/models/settings/builtInMetadata.ts @@ -1576,6 +1576,20 @@ const builtInMetadata = (Setting: typeof SettingType) => { isGlobal: true, }, + 'featureFlag.linuxKeychain': { + value: false, + type: SettingItemType.Bool, + public: true, + storage: SettingStorage.File, + appTypes: [AppType.Desktop], + label: () => 'Enable keychain support', + description: () => 'This is an experimental setting to enable keychain support on Linux', + show: () => shim.isLinux(), + section: 'general', + isGlobal: true, + advanced: true, + }, + // 'featureFlag.syncAccurateTimestamps': { // value: false, diff --git a/packages/lib/services/SettingUtils.ts b/packages/lib/services/SettingUtils.ts index 131b555d211..4686a27a80d 100644 --- a/packages/lib/services/SettingUtils.ts +++ b/packages/lib/services/SettingUtils.ts @@ -34,6 +34,13 @@ export async function loadKeychainServiceAndSettings(keychainServiceDrivers: Key Setting.setKeychainService(KeychainService.instance()); await Setting.load(); + // Using Linux with the keychain has been observed to cause all secure settings to be lost + // on Fedora 40 + GNOME. (This may have been related to running multiple Joplin instances). + // For now, make saving to the keychain opt-in until more feedback is received. + if (shim.isLinux() && !Setting.value('featureFlag.linuxKeychain')) { + KeychainService.instance().readOnly = true; + } + // This is part of the migration to the new sync target info. It needs to be // set as early as possible since it's used to tell if E2EE is enabled, it // contains the master keys, etc. Once it has been set, it becomes a noop diff --git a/packages/lib/versionInfo.ts b/packages/lib/versionInfo.ts index dd9d061bc74..dd72e12ef3c 100644 --- a/packages/lib/versionInfo.ts +++ b/packages/lib/versionInfo.ts @@ -1,9 +1,13 @@ +import Logger from '@joplin/utils/Logger'; import { _ } from './locale'; import Setting from './models/Setting'; import { reg } from './registry'; +import KeychainService from './services/keychain/KeychainService'; import { Plugins } from './services/plugins/PluginService'; import shim from './shim'; +const logger = Logger.create('versionInfo'); + export interface PackageInfo { name: string; version: string; @@ -70,6 +74,13 @@ export default function versionInfo(packageInfo: PackageInfo, plugins: Plugins) copyrightText.replace('YYYY', `${now.getFullYear()}`), ]; + let keychainSupported = false; + try { + keychainSupported = Setting.value('keychain.supported') >= 1 && !KeychainService.instance().readOnly; + } catch (error) { + logger.error('Failed to determine if keychain is supported', error); + } + const body = [ _('%s %s (%s, %s)', p.name, p.version, Setting.value('env'), shim.platformName()), '', @@ -78,7 +89,7 @@ export default function versionInfo(packageInfo: PackageInfo, plugins: Plugins) _('Profile Version: %s', reg.db().version()), // The portable app temporarily supports read-only keychain access (but disallows // write). - _('Keychain Supported: %s', (Setting.value('keychain.supported') >= 1 && !shim.isPortable()) ? _('Yes') : _('No')), + _('Keychain Supported: %s', keychainSupported ? _('Yes') : _('No')), ]; if (gitInfo) { From 15bc507e8d4461a1983772b763e1dcadbf5c73d1 Mon Sep 17 00:00:00 2001 From: Henry Heino Date: Thu, 17 Oct 2024 11:30:09 -0700 Subject: [PATCH 2/2] Update comment --- packages/lib/versionInfo.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/lib/versionInfo.ts b/packages/lib/versionInfo.ts index dd72e12ef3c..037e3408534 100644 --- a/packages/lib/versionInfo.ts +++ b/packages/lib/versionInfo.ts @@ -76,6 +76,7 @@ export default function versionInfo(packageInfo: PackageInfo, plugins: Plugins) let keychainSupported = false; try { + // To allow old keys to be read, certain apps allow read-only keychain access: keychainSupported = Setting.value('keychain.supported') >= 1 && !KeychainService.instance().readOnly; } catch (error) { logger.error('Failed to determine if keychain is supported', error); @@ -87,8 +88,6 @@ export default function versionInfo(packageInfo: PackageInfo, plugins: Plugins) _('Client ID: %s', Setting.value('clientId')), _('Sync Version: %s', Setting.value('syncVersion')), _('Profile Version: %s', reg.db().version()), - // The portable app temporarily supports read-only keychain access (but disallows - // write). _('Keychain Supported: %s', keychainSupported ? _('Yes') : _('No')), ];