Fall back to saving credentials without OS keystore encryption

Some Linux systems have no Secret Service running, and if users can't
change that (e.g., a managed system), storing an API key fails and login
never completes. Offer to store credentials unencrypted instead, and try
to encrypt them on a later read if the keystore becomes usable.

https://forums.zotero.org/discussion/133418/

chrome/content/zotero/xpcom/osKeyStore.js

@@ -63,17 +63,22 @@ Zotero.OSKeyStore = {
 			|| Services.wm.getMostRecentWindow('navigator:browser');
 	},
 
-	// Show an alert when an active write of new credentials fails (e.g., keychain unavailable)
-	alertSaveFailed: function () {
-		let win = this._getParentWindow();
-		if (!win) {
-			return;
-		}
-		Zotero.alert(
-			win,
-			Zotero.getString('general-error'),
-			Zotero.getString('os-keystore-save-failed')
-		);
+	// Offer to store credentials unencrypted after a write to the keystore fails. The keystore
+	// can be unusable in ways the user can't fix -- most often on Linux, where the platform
+	// requires a Secret Service that some managed systems don't run.
+	//
+	// Returns true if the user agrees.
+	confirmUnencryptedFallback: function () {
+		let index = Zotero.Prompt.confirm({
+			window: this._getParentWindow(),
+			title: Zotero.getString('general-error'),
+			text: Zotero.getString('os-keystore-save-failed') + "\n\n"
+				+ Zotero.getString('os-keystore-save-unencrypted'),
+			button0: Zotero.getString('os-keystore-save-unencrypted-button'),
+			button1: Zotero.Prompt.BUTTON_TITLE_CANCEL,
+			defaultButton: 1
+		});
+		return index == 0;
 	},
 
 	// Show a one-shot alert when migration of an existing legacy plaintext entry

chrome/content/zotero/xpcom/storage/webdav.js

@@ -273,7 +273,23 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = {
 		});
 		for (var i = 0; i < logins.length; i++) {
 			if (logins[i].username == username) {
-				return Zotero.OSKeyStore.decrypt(logins[i].password);
+				let password = logins[i].password;
+				if (Zotero.OSKeyStore.isEncrypted(password)) {
+					return Zotero.OSKeyStore.decrypt(password);
+				}
+				// Password stored without encryption after a failed write -- encrypt it now,
+				// in case the keystore has since become usable
+				if (!this._reencryptedPassword) {
+					this._reencryptedPassword = true;
+					try {
+						Zotero.debug("Encrypting unencrypted WebDAV password");
+						await this._writeEncryptedPassword(username, password);
+					}
+					catch (e) {
+						Zotero.logError(e);
+					}
+				}
+				return password;
 			}
 		}
 		
@@ -314,24 +330,7 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = {
 		this._basicAuthHeader = false;
 		this._digestParams = null;
 
-		try {
-			await this._writeEncryptedPassword(username, password);
-		}
-		catch (e) {
-			// If the write failed because the key database was unusable, reset the login
-			// manager and retry
-			if (!(await Zotero.Sync.Data.Local.repairLoginManager())) {
-				Zotero.OSKeyStore.alertSaveFailed();
-				throw e;
-			}
-			try {
-				await this._writeEncryptedPassword(username, password);
-			}
-			catch (e) {
-				Zotero.OSKeyStore.alertSaveFailed();
-				throw e;
-			}
-		}
+		await this._savePassword(username, password);
 		
 		// Drop any leftover plaintext entry from the legacy realm
 		var logins = await Services.logins.searchLoginsAsync({
@@ -368,7 +367,48 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = {
 		}
 	},
 	
+	/**
+	 * Store the password, retrying after a login manager repair and, if the keystore still
+	 * can't be used, offering to store the password without it
+	 */
+	async _savePassword(username, password) {
+		var error;
+		try {
+			await this._writeEncryptedPassword(username, password);
+			return;
+		}
+		catch (e) {
+			Zotero.logError(e);
+			error = e;
+		}
+		// If the write failed because the key database was unusable, reset the login manager
+		// and retry
+		if (await Zotero.Sync.Data.Local.repairLoginManager()) {
+			try {
+				await this._writeEncryptedPassword(username, password);
+				return;
+			}
+			catch (e) {
+				Zotero.logError(e);
+			}
+		}
+		// A password that would read back as ciphertext can't be stored unencrypted
+		if (Zotero.OSKeyStore.isEncrypted(password)) {
+			throw error;
+		}
+		if (!Zotero.OSKeyStore.confirmUnencryptedFallback()) {
+			throw error;
+		}
+		await this._writePassword(username, password);
+		// The keystore is unusable, so don't try to encrypt again on the next read
+		this._reencryptedPassword = true;
+	},
+	
 	async _writeEncryptedPassword(username, password) {
+		await this._writePassword(username, password && await Zotero.OSKeyStore.encrypt(password));
+	},
+	
+	async _writePassword(username, storedValue) {
 		// Remove any existing entries in the encrypted realm for this user
 		var logins = await Services.logins.searchLoginsAsync({
 			origin: this._loginManagerHost,
@@ -380,8 +420,7 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = {
 			}
 		}
 		
-		if (password) {
-			let storedValue = await Zotero.OSKeyStore.encrypt(password);
+		if (storedValue) {
 			let nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1",
 				Components.interfaces.nsILoginInfo, "init");
 			let loginInfo = new nsLoginInfo(this._loginManagerHost, null,

chrome/content/zotero/xpcom/sync/syncLocal.js

@@ -88,7 +88,22 @@ Zotero.Sync.Data.Local = {
 		}
 		var login = await this._getAPIKeyLoginInfo();
 		if (login) {
-			return Zotero.OSKeyStore.decrypt(login.password);
+			if (Zotero.OSKeyStore.isEncrypted(login.password)) {
+				return Zotero.OSKeyStore.decrypt(login.password);
+			}
+			// Key stored without encryption after a failed write -- encrypt it now, in case
+			// the keystore has since become usable
+			if (!this._reencryptedAPIKey) {
+				this._reencryptedAPIKey = true;
+				try {
+					Zotero.debug("Encrypting unencrypted API key");
+					await this._writeEncryptedAPIKey(login.password);
+				}
+				catch (e) {
+					Zotero.logError(e);
+				}
+			}
+			return login.password;
 		}
 		// If the login manager had to be reset, the stored API key is gone, so tell the user
 		// to log in again
@@ -154,24 +169,7 @@ Zotero.Sync.Data.Local = {
 			return;
 		}
 		
-		try {
-			await this._writeEncryptedAPIKey(apiKey);
-		}
-		catch (e) {
-			// If the write failed because the key database was unusable, reset the login
-			// manager and retry, so that logging in works without a manual fix
-			if (!(await this.repairLoginManager())) {
-				Zotero.OSKeyStore.alertSaveFailed();
-				throw e;
-			}
-			try {
-				await this._writeEncryptedAPIKey(apiKey);
-			}
-			catch (e) {
-				Zotero.OSKeyStore.alertSaveFailed();
-				throw e;
-			}
-		}
+		await this._saveAPIKey(apiKey);
 		// Drop any leftover plaintext entry from the legacy realm
 		if (legacyLoginInfo) {
 			await Services.logins.removeLoginAsync(legacyLoginInfo);
@@ -181,9 +179,47 @@ Zotero.Sync.Data.Local = {
 	},
 	
 	
+	/**
+	 * Store the API key, retrying after a login manager repair and, if the keystore still
+	 * can't be used, offering to store the key without it
+	 */
+	_saveAPIKey: async function (apiKey) {
+		var error;
+		try {
+			await this._writeEncryptedAPIKey(apiKey);
+			return;
+		}
+		catch (e) {
+			Zotero.logError(e);
+			error = e;
+		}
+		// If the write failed because the key database was unusable, reset the login manager
+		// and retry, so that logging in works without a manual fix
+		if (await this.repairLoginManager()) {
+			try {
+				await this._writeEncryptedAPIKey(apiKey);
+				return;
+			}
+			catch (e) {
+				Zotero.logError(e);
+			}
+		}
+		if (!Zotero.OSKeyStore.confirmUnencryptedFallback()) {
+			throw error;
+		}
+		await this._writeAPIKey(apiKey);
+		// The keystore is unusable, so don't try to encrypt again on the next read
+		this._reencryptedAPIKey = true;
+	},
+	
+	
 	_writeEncryptedAPIKey: async function (apiKey) {
+		await this._writeAPIKey(await Zotero.OSKeyStore.encrypt(apiKey));
+	},
+	
+	
+	_writeAPIKey: async function (storedValue) {
 		var oldLoginInfo = await this._getAPIKeyLoginInfo();
-		var storedValue = await Zotero.OSKeyStore.encrypt(apiKey);
 		var nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1",
 				Components.interfaces.nsILoginInfo, "init");
 		var loginInfo = new nsLoginInfo(

chrome/locale/en-US/zotero/zotero.ftl

@@ -1189,15 +1189,19 @@ login-manager-reset = { -app-name } was unable to read your saved login informat
 os-keystore-save-failed =
     { PLATFORM() ->
         [macos] { -app-name } couldn’t access the { -os-name } Keychain to securely save your credentials. Make sure your Keychain is accessible and try again.
-        [windows] { -app-name } couldn’t securely save your credentials. Try again or restart { -app-name }.
-       *[other] { -app-name } couldn’t access your { -os-name } keyring to securely save your credentials. Make sure a keyring service is running and try again.
+        [windows] { -app-name } couldn’t use { -os-name } Credential Manager to securely save your credentials. Try again or restart { -app-name }.
+       *[other] { -app-name } couldn’t access your { -os-name } keyring to securely save your credentials. Make sure a keyring service such as GNOME Keyring or KWallet is running and try again.
     }
 
+os-keystore-save-unencrypted = { -app-name } can save your credentials unencrypted instead. Anyone with access to your { -app-name } profile folder would then be able to read them.
+
+os-keystore-save-unencrypted-button = Save Anyway
+
 os-keystore-migrate-failed =
     { PLATFORM() ->
         [macos] { -app-name } couldn’t access the { -os-name } Keychain to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure your Keychain is accessible and restart { -app-name }.
         [windows] { -app-name } couldn’t encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Restart { -app-name } and try again.
-       *[other] { -app-name } couldn’t access your { -os-name } keyring to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure a keyring service is running and restart { -app-name }.
+       *[other] { -app-name } couldn’t access your { -os-name } keyring to encrypt your stored credentials. Your credentials remain stored unencrypted on disk. Make sure a keyring service such as GNOME Keyring or KWallet is running and restart { -app-name }.
     }
 
 search-button =

test/tests/syncLocalTest.js

@@ -18,6 +18,95 @@ describe("Zotero.Sync.Data.Local", function () {
 			await Zotero.Sync.Data.Local.setAPIKey("");
 			assert.strictEqual(await Zotero.Sync.Data.Local.getAPIKey(apiKey), "");
 		})
+		
+		
+		it("should store the key without encryption if the keystore is unusable and the user agrees", async function () {
+			var apiKey = Zotero.Utilities.randomString(24);
+			var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt")
+				.rejects(new Error("User canceled OS unlock entry"));
+			var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback")
+				.returns(true);
+			try {
+				await Zotero.Sync.Data.Local.setAPIKey(apiKey);
+				assert.ok(confirmStub.called);
+				assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey);
+				// The read shouldn't have retried encryption against the same broken keystore
+				assert.equal(encryptStub.callCount, 1);
+			}
+			finally {
+				encryptStub.restore();
+				confirmStub.restore();
+				await Zotero.Sync.Data.Local.setAPIKey("");
+			}
+		})
+		
+		
+		it("shouldn't store the key if the keystore is unusable and the user declines", async function () {
+			var apiKey = Zotero.Utilities.randomString(24);
+			var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt")
+				.rejects(new Error("User canceled OS unlock entry"));
+			var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback")
+				.returns(false);
+			try {
+				var e = await getPromiseError(Zotero.Sync.Data.Local.setAPIKey(apiKey));
+				assert.ok(e);
+				assert.strictEqual(await Zotero.Sync.Data.Local.getAPIKey(), "");
+			}
+			finally {
+				encryptStub.restore();
+				confirmStub.restore();
+				await Zotero.Sync.Data.Local.setAPIKey("");
+			}
+		})
+		
+		
+		it("should prompt before storing the key without encryption", async function () {
+			var apiKey = Zotero.Utilities.randomString(24);
+			var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt")
+				.rejects(new Error("User canceled OS unlock entry"));
+			var promptStub = sinon.stub(Zotero.Prompt, "confirm").returns(0);
+			try {
+				await Zotero.Sync.Data.Local.setAPIKey(apiKey);
+				assert.ok(promptStub.calledOnce);
+				assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey);
+			}
+			finally {
+				encryptStub.restore();
+				promptStub.restore();
+				await Zotero.Sync.Data.Local.setAPIKey("");
+			}
+		})
+		
+		
+		it("should encrypt a key stored without encryption in a later session", async function () {
+			if (!Zotero.OSKeyStore.available) {
+				this.skip();
+			}
+			var apiKey = Zotero.Utilities.randomString(24);
+			// Store it with an unusable keystore
+			var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt")
+				.rejects(new Error("User canceled OS unlock entry"));
+			var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback")
+				.returns(true);
+			try {
+				await Zotero.Sync.Data.Local.setAPIKey(apiKey);
+			}
+			finally {
+				encryptStub.restore();
+				confirmStub.restore();
+			}
+			try {
+				// Encryption is retried once per session, so stand in for a restart
+				Zotero.Sync.Data.Local._reencryptedAPIKey = false;
+				assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey);
+				let login = await Zotero.Sync.Data.Local._getAPIKeyLoginInfo();
+				assert.ok(Zotero.OSKeyStore.isEncrypted(login.password));
+				assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey);
+			}
+			finally {
+				await Zotero.Sync.Data.Local.setAPIKey("");
+			}
+		})
 	})
 
 

test/tests/webdavTest.js

@@ -259,6 +259,40 @@ describe("Zotero.Sync.Storage.Mode.WebDAV", function () {
 			await controller.setPassword(password);
 			assert.equal(await controller.getPassword(), password);
 		})
+		
+		
+		it("shouldn't store a password that would read back as encrypted", async function () {
+			var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt")
+				.rejects(new Error("User canceled OS unlock entry"));
+			var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback")
+				.returns(true);
+			try {
+				var e = await getPromiseError(controller.setPassword("oskv1:example"));
+				assert.ok(e);
+			}
+			finally {
+				encryptStub.restore();
+				confirmStub.restore();
+			}
+		})
+		
+		
+		it("should return a password stored without encryption after a keystore failure", async function () {
+			var password = "p\u00e4ssw\u20acrd";
+			var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt")
+				.rejects(new Error("User canceled OS unlock entry"));
+			var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback")
+				.returns(true);
+			try {
+				await controller.setPassword(password);
+				assert.ok(confirmStub.called);
+				assert.equal(await controller.getPassword(), password);
+			}
+			finally {
+				encryptStub.restore();
+				confirmStub.restore();
+			}
+		})
 	})
 	
 	describe("Syncing", function () {