From fd06c18cf0fbf19ff697bc53b29afe9003eacaf9 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Mon, 2 Sep 2024 04:26:43 -0700 Subject: [PATCH] Desktop: Windows portable: Fix keychain-backed storage incorrectly enabled (#10942) --- packages/lib/services/SettingUtils.ts | 9 ++ .../services/keychain/KeychainService.test.ts | 89 ++++++++++++++++--- .../lib/services/keychain/KeychainService.ts | 39 ++++++-- .../keychain/KeychainServiceDriver.node.ts | 2 +- packages/lib/versionInfo.ts | 4 +- 5 files changed, 119 insertions(+), 24 deletions(-) diff --git a/packages/lib/services/SettingUtils.ts b/packages/lib/services/SettingUtils.ts index 682b8882d..131b555d2 100644 --- a/packages/lib/services/SettingUtils.ts +++ b/packages/lib/services/SettingUtils.ts @@ -5,6 +5,7 @@ import Setting from '../models/Setting'; import uuid from '../uuid'; import { migrateLocalSyncInfo } from './synchronizer/syncInfoUtils'; import KeychainServiceDriverBase from './keychain/KeychainServiceDriverBase'; +import shim from '../shim'; type KeychainServiceDriverConstructor = new (appId: string, clientId: string)=> KeychainServiceDriverBase; @@ -19,6 +20,14 @@ type KeychainServiceDriverConstructor = new (appId: string, clientId: string)=> export async function loadKeychainServiceAndSettings(keychainServiceDrivers: KeychainServiceDriverConstructor[]) { const clientIdSetting = await Setting.loadOne('clientId'); const clientId = clientIdSetting ? clientIdSetting.value : uuid.create(); + + // Temporary workaround: For a short time, pre-release versions of Joplin Portable encrypted + // saved keys using the keychain. This can break sync when transferring Joplin between devices. + // To prevent secure keys from being lost, we enable read-only keychain access in portable mode. + if (shim.isPortable()) { + KeychainService.instance().readOnly = true; + } + await KeychainService.instance().initialize( keychainServiceDrivers.map(Driver => new Driver(Setting.value('appId'), clientId)), ); diff --git a/packages/lib/services/keychain/KeychainService.test.ts b/packages/lib/services/keychain/KeychainService.test.ts index 4f7914bb2..65499173d 100644 --- a/packages/lib/services/keychain/KeychainService.test.ts +++ b/packages/lib/services/keychain/KeychainService.test.ts @@ -13,18 +13,21 @@ interface SafeStorageMockOptions { } const mockSafeStorage = ({ // Safe storage - isEncryptionAvailable = jest.fn(() => true), - encryptString = jest.fn(async s => (`e:${s}`)), - decryptString = jest.fn(async s => s.substring(2)), + isEncryptionAvailable = () => true, + encryptString = async s => (`e:${s}`), + decryptString = async (s) => s.substring(2), }: SafeStorageMockOptions) => { + const mock = { + isEncryptionAvailable: jest.fn(isEncryptionAvailable), + encryptString: jest.fn(encryptString), + decryptString: jest.fn(decryptString), + getSelectedStorageBackend: jest.fn(() => 'mock'), + }; shim.electronBridge = () => ({ - safeStorage: { - isEncryptionAvailable, - encryptString, - decryptString, - getSelectedStorageBackend: () => 'mock', - }, + safeStorage: mock, }); + + return mock; }; const mockKeytar = () => { @@ -51,10 +54,19 @@ const makeDrivers = () => [ new KeychainServiceDriverNode(Setting.value('appId'), Setting.value('clientId')), ]; +const testSaveLoadSecureSetting = async (expectedPassword: string) => { + Setting.setValue('encryption.masterPassword', expectedPassword); + await Setting.saveAll(); + + await Setting.load(); + expect(Setting.value('encryption.masterPassword')).toBe(expectedPassword); +}; + describe('KeychainService', () => { beforeEach(async () => { await setupDatabaseAndSynchronizer(0); await switchClient(0); + KeychainService.instance().readOnly = false; Setting.setValue('keychain.supported', 1); shim.electronBridge = null; shim.keytar = null; @@ -91,16 +103,12 @@ describe('KeychainService', () => { const keytarMock = mockKeytar(); await KeychainService.instance().initialize(makeDrivers()); - Setting.setValue('encryption.masterPassword', 'test-password'); - await Setting.saveAll(); + await testSaveLoadSecureSetting('test-password'); expect(keytarMock.setPassword).toHaveBeenCalledWith( `${Setting.value('appId')}.setting.encryption.masterPassword`, `${Setting.value('clientId')}@joplin`, 'test-password', ); - - await Setting.load(); - expect(Setting.value('encryption.masterPassword')).toBe('test-password'); }); test('should re-check for keychain support when a new driver is added', async () => { @@ -125,4 +133,57 @@ describe('KeychainService', () => { await KeychainService.instance().detectIfKeychainSupported(); expect(Setting.value('keychain.supported')).toBe(0); }); + + test('should load settings from a read-only KeychainService if not present in the database', async () => { + mockSafeStorage({}); + + const service = KeychainService.instance(); + await service.initialize(makeDrivers()); + expect(await service.setPassword('setting.encryption.masterPassword', 'keychain password')).toBe(true); + + service.readOnly = true; + await service.initialize(makeDrivers()); + await Setting.load(); + + expect(Setting.value('encryption.masterPassword')).toBe('keychain password'); + }); + + test('settings should be saved to database with a read-only keychain', async () => { + const safeStorage = mockSafeStorage({}); + + const service = KeychainService.instance(); + service.readOnly = true; + + await service.initialize(makeDrivers()); + await service.detectIfKeychainSupported(); + + expect(Setting.value('keychain.supported')).toBe(1); + await testSaveLoadSecureSetting('testing...'); + + expect(safeStorage.encryptString).not.toHaveBeenCalledWith('testing...'); + }); + + test('loading settings with a read-only keychain should prefer the database', async () => { + const safeStorage = mockSafeStorage({}); + + const service = KeychainService.instance(); + await service.initialize(makeDrivers()); + // Set an initial value + expect(await service.setPassword('setting.encryption.masterPassword', 'test keychain password')).toBe(true); + + service.readOnly = true; + await service.initialize(makeDrivers()); + + safeStorage.encryptString.mockClear(); + + Setting.setValue('encryption.masterPassword', 'test database password'); + await Setting.saveAll(); + + await Setting.load(); + expect(Setting.value('encryption.masterPassword')).toBe('test database password'); + expect(await service.password('setting.encryption.masterPassword')).toBe('test keychain password'); + + // Should not have attempted to encrypt settings in read-only mode. + expect(safeStorage.encryptString).not.toHaveBeenCalled(); + }); }); diff --git a/packages/lib/services/keychain/KeychainService.ts b/packages/lib/services/keychain/KeychainService.ts index b0d975f8b..84c90e01a 100644 --- a/packages/lib/services/keychain/KeychainService.ts +++ b/packages/lib/services/keychain/KeychainService.ts @@ -11,6 +11,7 @@ export default class KeychainService extends BaseService { private keysNeedingMigration_: Set; private static instance_: KeychainService; private enabled_ = true; + private readOnly_ = false; public static instance(): KeychainService { if (!this.instance_) this.instance_ = new KeychainService(); @@ -53,8 +54,17 @@ export default class KeychainService extends BaseService { this.enabled_ = v; } + public get readOnly() { + return this.readOnly_; + } + + public set readOnly(v: boolean) { + this.readOnly_ = v; + } + public async setPassword(name: string, password: string): Promise { if (!this.enabled) return false; + if (this.readOnly_) return false; // Optimization: Handles the case where the password doesn't need to change. // TODO: Re-evaluate whether this optimization is necessary after refactoring the driver @@ -137,16 +147,29 @@ export default class KeychainService extends BaseService { Setting.setValue('keychain.supported', -1); } - const passwordIsSet = await this.setPassword('zz_testingkeychain', 'mytest'); + if (!this.readOnly) { + const passwordIsSet = await this.setPassword('zz_testingkeychain', 'mytest'); - if (!passwordIsSet) { - this.logger().info('KeychainService: could not set test password - keychain support will be disabled'); - Setting.setValue('keychain.supported', 0); + if (!passwordIsSet) { + this.logger().info('KeychainService: could not set test password - keychain support will be disabled'); + Setting.setValue('keychain.supported', 0); + } else { + const result = await this.password('zz_testingkeychain'); + await this.deletePassword('zz_testingkeychain'); + this.logger().info('KeychainService: tried to set and get password. Result was:', result); + Setting.setValue('keychain.supported', result === 'mytest' ? 1 : 0); + } } else { - const result = await this.password('zz_testingkeychain'); - await this.deletePassword('zz_testingkeychain'); - this.logger().info('KeychainService: tried to set and get password. Result was:', result); - Setting.setValue('keychain.supported', result === 'mytest' ? 1 : 0); + // The supported check requires write access to the keychain -- rely on the more + // limited support checks done by each driver. + const supported = this.drivers_.length > 0; + Setting.setValue('keychain.supported', supported ? 1 : 0); + + if (supported) { + logger.info('Starting KeychainService in read-only mode. Keys will be read, but not written.'); + } else { + logger.info('Failed to start in read-only mode -- no supported drivers found.'); + } } Setting.setValue('keychain.lastAvailableDrivers', this.drivers_.map(driver => driver.driverId)); } diff --git a/packages/lib/services/keychain/KeychainServiceDriver.node.ts b/packages/lib/services/keychain/KeychainServiceDriver.node.ts index 23dcca593..3f5600d4d 100644 --- a/packages/lib/services/keychain/KeychainServiceDriver.node.ts +++ b/packages/lib/services/keychain/KeychainServiceDriver.node.ts @@ -5,7 +5,7 @@ export default class KeychainServiceDriver extends KeychainServiceDriverBase { public override readonly driverId: string = 'node-keytar'; public async supported(): Promise { - return !!shim.keytar(); + return !!shim.keytar?.(); } public async setPassword(name: string, password: string): Promise { diff --git a/packages/lib/versionInfo.ts b/packages/lib/versionInfo.ts index eae0e6b35..dd9d061bc 100644 --- a/packages/lib/versionInfo.ts +++ b/packages/lib/versionInfo.ts @@ -76,7 +76,9 @@ 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()), - _('Keychain Supported: %s', Setting.value('keychain.supported') >= 1 ? _('Yes') : _('No')), + // The portable app temporarily supports read-only keychain access (but disallows + // write). + _('Keychain Supported: %s', (Setting.value('keychain.supported') >= 1 && !shim.isPortable()) ? _('Yes') : _('No')), ]; if (gitInfo) {