[desktop] use electron safeStorage to store device keys

src/api/common/error/DeviceStorageUnavailableError.js

@@ -4,7 +4,7 @@
 import {TutanotaError} from "./TutanotaError"
 
 export class DeviceStorageUnavailableError extends TutanotaError {
-	constructor(msg: string, error: Error) {
+	constructor(msg: string, error: ?Error) {
 		super("DeviceStorageUnavailableError", error ? (msg + "> " + (error.stack ? error.stack : error.message)) : msg)
 	}
 }

src/desktop/DesktopMain.js

@@ -23,8 +23,9 @@ import {DesktopTray} from "./tray/DesktopTray"
 import {log} from "./DesktopLog";
 import {UpdaterWrapperImpl} from "./UpdaterWrapper"
 import {ElectronNotificationFactory} from "./NotificatonFactory"
-import {KeytarSecretStorage} from "./sse/SecretStorage"
+import {SafeStorageSecretStorage} from "./sse/SecretStorage"
 import fs from "fs"
+import path from "path"
 import {DesktopIntegrator, getDesktopIntegratorForPlatform} from "./integration/DesktopIntegrator"
 import net from "net"
 import child_process from "child_process"
@@ -96,7 +97,7 @@ if (opts.registerAsMailHandler && opts.unregisterAsMailHandler) {
 
 async function createComponents(): Promise<Components> {
 	lang.init(en)
-	const secretStorage = new KeytarSecretStorage()
+	const secretStorage = new SafeStorageSecretStorage(electron, fs, path)
 	const deviceKeyProvider = new DeviceKeyProviderImpl(secretStorage, desktopCrypto)
 	const configMigrator = new DesktopConfigMigrator(desktopCrypto, deviceKeyProvider, electron)
 	const conf = new DesktopConfig(app, configMigrator, deviceKeyProvider, desktopCrypto)

src/desktop/DesktopUtils.js

@@ -7,7 +7,7 @@ import {app} from 'electron'
 import {defer, delay, noOp, uint8ArrayToHex} from '@tutao/tutanota-utils'
 import {log} from "./DesktopLog"
 import {DesktopCryptoFacade} from "./DesktopCryptoFacade"
-import {fileExists, swapFilename} from "./PathUtils"
+import {fileExists, replaceLastComponent} from "./PathUtils"
 import url from "url"
 import {registerKeys, unregisterKeys} from "./reg-templater"
 
@@ -29,7 +29,7 @@ export class DesktopUtils {
 	}
 
 	checkIsPerUserInstall(): Promise<boolean> {
-		const markerPath = swapFilename(process.execPath, "per_user")
+		const markerPath = replaceLastComponent(process.execPath, "per_user")
 		return fileExists(markerPath)
 	}
 
@@ -217,7 +217,7 @@ export class DesktopUtils {
 	 */
 	_writeToDisk(contents: string): string {
 		const filename = uint8ArrayToHex(this._desktopCrypto.randomBytes(12))
-		const filePath = swapFilename(process.execPath, filename)
+		const filePath = replaceLastComponent(process.execPath, filename)
 		this._fs.writeFileSync(filePath, contents, {encoding: 'utf-8', mode: 0o400})
 		return filePath
 	}
@@ -234,7 +234,7 @@ export class DesktopUtils {
 		// * where to put tmp files (also user-dependent)
 		// all these must be set in the registry
 		const execPath = process.execPath
-		const dllPath = swapFilename(execPath, 'mapirs.dll')
+		const dllPath = replaceLastComponent(execPath, 'mapirs.dll')
 		// we may be a per-machine installation that's used by multiple users, so the dll will replace %USERPROFILE%
 		// with the value of the USERPROFILE env var.
 		const appData = path.join("%USERPROFILE%", 'AppData')

src/desktop/PathUtils.js

@@ -88,12 +88,12 @@ export function urlIsPrefix(prefix: URL, url: URL): boolean {
 }
 
 /**
- * replace the last component in a file path with another
+ * replace the last component in a path with another
  * @param p path to a file/folder
- * @param file the file name to put in the last path component
+ * @param newLast the name to put in the last path component
  * @param pathModule path module to use for cross platform testing
  */
-export function swapFilename(p: string, file: string, pathModule: $Exports<"path"> = path): string {
+export function replaceLastComponent(p: string, newLast: string, pathModule: $Exports<"path"> = path): string {
 	const dir = pathModule.dirname(p)
-	return pathModule.join(dir, file)
+	return pathModule.join(dir, newLast)
 }

src/desktop/sse/SecretStorage.js

@@ -1,19 +1,126 @@
 //@flow
 
-import {getPassword, setPassword} from "keytar"
+import {deletePassword, getPassword, setPassword} from "keytar"
+import type {App, SafeStorage} from "electron"
+import {log} from "../DesktopLog"
+import {DeviceStorageUnavailableError} from "../../api/common/error/DeviceStorageUnavailableError"
 
 export interface SecretStorage {
 	getPassword(service: string, account: string): Promise<?string>;
 
 	setPassword(service: string, account: string, password: string): Promise<void>;
 }
 
-export class KeytarSecretStorage implements SecretStorage {
+/**
+ * @deprecated: will be replaced by SafeStorageSecretStorage
+ */
+class KeytarSecretStorage implements SecretStorage {
+
 	getPassword(service: string, account: string): Promise<?string> {
 		return getPassword(service, account)
 	}
 
 	setPassword(service: string, account: string, password: string): Promise<void> {
-		return setPassword(service, account, password);
+		return setPassword(service, account, password)
+	}
+
+	deletePassword(service: string, account: string): Promise<boolean> {
+		return deletePassword(service, account)
+	}
+}
+
+/**
+ * Secret Storage impl using the electron 15+ SafeStorage API
+ *
+ * Note: the main thread will be blocked while the keychain is being unlocked,
+ * potentially for as long as the user takes to enter a password.
+ * We're asking for access before any windows are created, which should prevent
+ * any weirdness arising from that.
+ */
+export class SafeStorageSecretStorage implements SecretStorage {
+	_safeStorage: SafeStorage
+	_app: App
+	_fs: $Exports<"fs">
+	_path: $Exports<"path">
+	_keytarSecretStorage: KeytarSecretStorage
+
+	constructor({safeStorage, app}: $Exports<"electron">, fs: $Exports<"fs">, path: $Exports<"path">) {
+		this._safeStorage = safeStorage
+		this._app = app
+		this._fs = fs
+		this._path = path
+		this._keytarSecretStorage = new KeytarSecretStorage()
+	}
+
+	async getPassword(service: string, account: string): Promise<?string> {
+		await this._assertAvailable()
+		await this._migrateKeytarPassword(service, account)
+		const keyPath = this._getKeyPath(service, account)
+		try {
+			const encPwBuffer = await this._fs.promises.readFile(keyPath)
+			const plainPw = this._safeStorage.decryptString(encPwBuffer)
+			return Promise.resolve(plainPw)
+		} catch (e) {
+			if (e.code === "ENOENT") return null
+			throw e
+		}
+	}
+
+	async setPassword(service: string, account: string, password: string): Promise<void> {
+		await this._assertAvailable()
+		const keyPath = this._getKeyPath(service, account)
+		const cypherBuffer = this._safeStorage.encryptString(password)
+		return this._fs.promises.writeFile(keyPath, cypherBuffer)
+	}
+
+	_getKeyPath(service: string, account: string): string {
+		const fname = service.concat("-", account)
+		const safeStoragePath = this._getSafeStoragePath()
+		return this._path.join(safeStoragePath, fname)
+	}
+
+	/**
+	 * this should always be a path inside the user's home directory (or equivalent)
+	 * @private
+	 */
+	_getSafeStoragePath(): string {
+		return this._path.join(this._app.getPath('userData'), "safe_storage")
+	}
+
+	/**
+	 * ensures that the safe_storage directory exists and that we can use the
+	 * safeStorage API
+	 * @private
+	 */
+	async _assertAvailable(): Promise<void> {
+		await this._fs.promises.mkdir(this._getSafeStoragePath(), {recursive: true})
+		// see https://github.com/electron/electron/issues/32206
+		// the rest of the safeStorage API should be throwing errors
+		// we can catch until this works.
+		if (process.platform === 'linux') return
+		if (this._safeStorage.isEncryptionAvailable()) return
+		throw new DeviceStorageUnavailableError("safeStorage API is not available", null)
+	}
+
+	/**
+	 * most devices will have stored a deviceKey with keytar, which we can move
+	 * to the safeStorage impl.
+	 *
+	 * @private
+	 */
+	async _migrateKeytarPassword(service: string, account: string): Promise<void> {
+		let keytarPw = null
+		try {
+			keytarPw = await this._keytarSecretStorage.getPassword(service, account)
+		} catch (e) {
+			log.debug("keytar failed, assuming there's no pw stored")
+		}
+		if (keytarPw) {
+			await this.setPassword(service, account, keytarPw)
+			// do not do this until later. there may be multiple installs using
+			// the deviceKey if for some reason keytar used a system keychain
+			// to store it.
+			// await this._keytarSecretStorage.deletePassword(service, account)
+		}
 	}
 }

test/client/desktop/PathUtilsTest.js

@@ -1,6 +1,6 @@
 //@flow
 import o from "ospec"
-import {nonClobberingFilename, swapFilename} from "../../../src/desktop/PathUtils"
+import {nonClobberingFilename, replaceLastComponent} from "../../../src/desktop/PathUtils"
 import path from "path"
 
 o.spec("PathUtils", function () {
@@ -206,21 +206,21 @@ o.spec("PathUtils", function () {
 
 	o.spec("swapFileName Test", function () {
 		o("replace file with file, posix", function () {
-			o(swapFilename("/a/b/c.txt", "d.txt")).equals("/a/b/d.txt")
-			o(swapFilename("a/b/c.txt", "d.txt")).equals("a/b/d.txt")
-			o(swapFilename("/a/b/c.txt", "d")).equals("/a/b/d")
-			o(swapFilename("/a/b/c", "d.txt")).equals("/a/b/d.txt")
-			o(swapFilename("/a/b/c", "d.txt")).equals("/a/b/d.txt")
-			o(swapFilename("/", "bla.txt")).equals("/bla.txt")
+			o(replaceLastComponent("/a/b/c.txt", "d.txt")).equals("/a/b/d.txt")
+			o(replaceLastComponent("a/b/c.txt", "d.txt")).equals("a/b/d.txt")
+			o(replaceLastComponent("/a/b/c.txt", "d")).equals("/a/b/d")
+			o(replaceLastComponent("/a/b/c", "d.txt")).equals("/a/b/d.txt")
+			o(replaceLastComponent("/a/b/c", "d.txt")).equals("/a/b/d.txt")
+			o(replaceLastComponent("/", "bla.txt")).equals("/bla.txt")
 		})
 
 		o("replace file with file, windows", function () {
-			o(swapFilename("C:\\tmp\\file.html", "text.txt", path.win32)).equals("C:\\tmp\\text.txt")
-			o(swapFilename("C:\\tmp\\file.html", "text", path.win32)).equals("C:\\tmp\\text")
-			o(swapFilename("C:\\tmp\\folder\\", "text", path.win32)).equals("C:\\tmp\\text")
-			o(swapFilename("tmp\\file.html", "text.txt", path.win32)).equals("tmp\\text.txt")
-			o(swapFilename("tmp", "text.txt", path.win32)).equals("text.txt")
-			o(swapFilename("C:\\", "text.txt", path.win32)).equals("C:\\text.txt")
+			o(replaceLastComponent("C:\\tmp\\file.html", "text.txt", path.win32)).equals("C:\\tmp\\text.txt")
+			o(replaceLastComponent("C:\\tmp\\file.html", "text", path.win32)).equals("C:\\tmp\\text")
+			o(replaceLastComponent("C:\\tmp\\folder\\", "text", path.win32)).equals("C:\\tmp\\text")
+			o(replaceLastComponent("tmp\\file.html", "text.txt", path.win32)).equals("tmp\\text.txt")
+			o(replaceLastComponent("tmp", "text.txt", path.win32)).equals("text.txt")
+			o(replaceLastComponent("C:\\", "text.txt", path.win32)).equals("C:\\text.txt")
 		})
 	})
 })