User prompted by Gnome Keyring / KDE Wallet when clicking on "Unlock..."

[overheadhunter]

Description

This is a follow-up on #1525. Essentially, the problem is that Cryptomator asks whether a password is stored, as soon as a user triggers the unlock workflow. While necessary, this has unwanted side effects on Linux, if no password is stored and the user doesn't expect any interaction with any 3rd party keychain.

System Setup

• Operating system and version: Any Linux Distro providing KDE Wallet or Gnome Keyring
• Cryptomator version: 1.5.12
• Volume type: N/A

Steps to Reproduce

1. Either
   • don't use KDE Wallet or Gnome Keyring
   • or make sure, your keychain is currently locked
2. Inside Cryptomator, click on "Unlock..." for any vault (regardless if a password is stored or not)

Expected Behavior

No interaction with Gnome Keyring / KDE Wallet

Actual Behavior

System keychain prompts the user.

Reproducibility

Always

Additional Information

@purejava has tracked this down to the eager access to stored passwords here:

cryptomator/main/commons/src/main/java/org/cryptomator/common/keychain/KeychainManager.java

Line 83 in e4709ed

storedPw = getKeychainOrFail().loadPassphrase(key);

[overheadhunter]

Maybe, inside integrations-linux, we need one more abstraction layer: Since we can't know if the keychain is currently unlocked (or can we?), we might want to store whether there should be a saved password in some config file, so we know when not to invoke the keychain API?

[purejava]

At least the secret-service has a Collection#isLocked() method and the kdewallet has a KWallet#isOpen() method.
These sound, as if they fit. There is some memory in my mind that especially this method in KWallet did not behave as expected.

I'll check that out for both backends.

[purejava]

While necessary, this has unwanted side effects on Linux, if no password is stored and the user doesn't expect any interaction with any 3rd party keychain.

According to the commits that introduced the Cryptomator inside password cache, this was done due to the fact, that calls to the backends are too expensive.
Is it an option to fill the cache later, e.g. when the user decided to use a password backend?

[overheadhunter]

Is it an option to fill the cache later, e.g. when the user decided to use a password backend?

Not really. We need to know if a stored password exists, when the user starts the unlock process. This is where we attempt to load it (if existing):

cryptomator/main/ui/src/main/java/org/cryptomator/ui/unlock/UnlockModule.java

Lines 49 to 63 in e4709ed

	@Provides
	@Named("savedPassword")
	@UnlockScoped
	static Optional<char[]> provideStoredPassword(KeychainManager keychain, @UnlockWindow Vault vault) {
	if (!keychain.isSupported()) {
	return Optional.empty();
	} else {
	try {
	return Optional.ofNullable(keychain.loadPassphrase(vault.getId()));
	} catch (KeychainAccessException e) {
	LOG.error("Failed to load entry from system keychain.", e);
	return Optional.empty();
	}
	}
	}

Maybe we can change line 53 to something like

- if (!keychain.isSupported()) { 
+ if (!keychain.isSupported() || keychain.isLocked()) { 

However, if there is a password inside a locked keychain, it would not prompt the user, even if he*she expected it.

[purejava]

Maybe we can change line 53 to something like

- if (!keychain.isSupported()) { 
+ if (!keychain.isSupported() || keychain.isLocked()) { 

However, if there is a password inside a locked keychain, it would not prompt the user, even if he*she expected it.

Yes, I think this is the right place for the change.

KWallet#isOpen() can be used, Collection#isLocked() would also fit, but the initialization of Collection requires a Service that is initialized in SimpleCollection with quite some effort.

I tried to tug things together for a first draft, but could not manage to compile integrations-mac. I am missing something:

[INFO] --- exec-maven-plugin:1.6.0:exec (default) @ integrations-mac ---
In file included from /Users/ralph/IdeaProjects/integrations-mac/src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m:9:
../headers/org_cryptomator_macos_keychain_MacKeychain_Native.h:2:10: fatal error: 'jni.h' file not found
#include <jni.h>
         ^~~~~~~
1 error generated.
In file included from /Users/ralph/IdeaProjects/integrations-mac/src/main/native/SKYAppearanceNotifier.m:10:
/Users/ralph/IdeaProjects/integrations-mac/src/main/native/SKYAppearanceObserver.h:9:10: fatal error: 'jni.h' file not found
#include <jni.h>
         ^~~~~~~
1 error generated.
In file included from /Users/ralph/IdeaProjects/integrations-mac/src/main/native/org_cryptomator_macos_autostart_MacLaunchServices_Native.m:9:
../headers/org_cryptomator_macos_autostart_MacLaunchServices_Native.h:2:10: fatal error: 'jni.h' file not found
#include <jni.h>
         ^~~~~~~
1 error generated.
note: Using new build system
note: Building targets in parallel
note: Planning build
note: Constructing build description
** ARCHIVE FAILED **

[ERROR] Command execution failed.
org.apache.commons.exec.ExecuteException: Process exited with an error: 65 (Exit value: 65)

The single APIs (integrations-api, -linux, -mac and -win) do need to level up together. As I cannot compile integrations-mac, unfortunately I couldn't come any further than this. My code so far is here:
https://github.com/purejava/integrations-api/tree/keychain-isLocked
https://github.com/purejava/integrations-linux/tree/keychain-isLocked
https://github.com/purejava/integrations-mac/tree/keychain-isLocked

What would be the best way to proceed from here?

[purejava]

I fixed the issue for the secret-service and the kdewallet backend.

It was indeed necessary to add another abstraction layer to the integrations-api: isLocked(). This reduces the required code to a minimum and has another advantage: it clears the way to implement KeePassXC as a new backend, which, like the latter two backends, can be and is locked for security reasons on startup.

This fix prevents the prompts to unlock the backends from coming up:

• on creating a vault
• on selecting a vault
• on clicking Unlock

Two points need to be mentioned: after a review of the changes and before merging, SecretServiceKeychainAccess in integrations-linux needs to get updated. As isLocked() is not yet available in SimpleCollection, the class only contains dummy code so far:

@Override
public boolean isLocked() {
  return true;
}

Second for the implementation in the Cryptomator-Code:

Maybe we can change line 53 to something like

- if (!keychain.isSupported()) { 
+ if (!keychain.isSupported() || keychain.isLocked()) { 

However, if there is a password inside a locked keychain, it would not prompt the user, even if he*she expected it.

it had no effect when I implemented the above statement in UnlockModule. Therefore it's implemented in KeychainManager.

These changes are required to fix the issue:
https://github.com/purejava/integrations-api/tree/keychain-isLocked
https://github.com/purejava/integrations-linux/tree/keychain-isLocked - needs changes in secret-service before merging
https://github.com/purejava/integrations-mac/tree/keychain-isLocked
https://github.com/purejava/integrations-win/tree/keychain-isLocked
https://github.com/purejava/cryptomator/tree/keychain-isLocked

The changes were tested intensively and should be fine.

[overheadhunter]

it had no effect when I implemented the above statement in UnlockModule. Therefore it's implemented in KeychainManager.

It should, though. Accessing the keychain when it is locked, should cause an IllegalStateException, not return null. Let's rather find the cause why the abovementioned didn't work as expected.

Changes to the integrations-* are fine. Feel free to open PRs.

[swiesend]

I though isLocked() would already be provided since secret-service 1.3.0:
https://github.com/swiesend/secret-service/blob/afe9b97ec8dcbd1244b3d1e7d3beaea133b80239/src/main/java/org/freedesktop/secret/simple/SimpleCollection.java#L609-L612

@purejava was it optimized as true by the compiler? Or did you refer an not yet fulfilled interface change of the org.cryptomator.integrations.keychain.KeychainAccessProvider? And I can get away with a non static method of isLocked(), right? And do I have to care about cryptomator/integrations-linux@b84073f ?

And I really like the KeePassXC access idea! @purejava did you come up with that idea?

@overheadhunter the IllegalStateException should be thrown by whom?

I try to find some time tomorrow to address all the issues.

[overheadhunter]

@overheadhunter the IllegalStateException should be thrown by whom?

My comment referred to a proposed change in purejava@d947ffa, which simply returns "null" when it is impossible to access a locked keychain. In my opinion the caller is responsible for not attempting to load a password from a locked keychain. I.e. the callee should throw an ISE if its state doesn't allow to use this method. Rather fail early than emitting null values that cause hard-to-debug unexpected behaviour down the road.

[purejava]

I though isLocked() would already be provided since secret-service 1.3.0:
https://github.com/swiesend/secret-service/blob/afe9b97ec8dcbd1244b3d1e7d3beaea133b80239/src/main/java/org/freedesktop/secret/simple/SimpleCollection.java#L609-L612

You are right. I must have looked into my not-up-to-date fork of your repo. Sorry for that!

@purejava was it optimized as true by the compiler? Or did you refer an not yet fulfilled interface change of the org.cryptomator.integrations.keychain.KeychainAccessProvider? And I can get away with a non static method of isLocked(), right?

isLocked just returns true as some dummy code as I thought there is no isLocked in SimpleCollection yet.

Something like the following might work (untested):

@Override
public boolean isLocked() {
	try (@SuppressWarnings("unused") SimpleCollection keyring = new SimpleCollection()) {
		// seems like we're able to access the keyring.
		return keyring.isLocked();
	} catch (IOException | ExceptionInInitializerError | RuntimeException e) {
		return true;
	}
}

And do I have to care about cryptomator/integrations-linux@b84073f ?

No. This has no relevance for the question at hand.

And I really like the KeePassXC access idea! @purejava did you come up with that idea?

Thank you. Yes, I noticed that KeePassXC is communicating with its browser-extensions via a proxy that KeePassXC provides and that this proxy can be accessed from other applications too.

[swiesend]

Regarding to the

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.2.0:jar (attach-javadocs) on project integrations-linux: MavenReportException: Error while generating Javadoc: Unable to find javadoc command: The environment variable JAVA_HOME is not correctly set. -> [Help 1]

for me it was sufficient to set javadoc explicitly.:

$ ll /opt/jdk/jdk-14.0.2+12/bin/javadoc
-rwxr-xr-x 1 sebastian sebastian 13176 Jul 15  2020 /opt/jdk/jdk-14.0.2+12/bin/javadoc*

<properties>
  <javadocExecutable>/opt/jdk/jdk-14.0.2+12/bin/javadoc</javadocExecutable>
</properties>

$JAVA_HOME alone did not help, but I did not need to set the USER_HEADER_SEARCH_PATHS:

$ echo $JAVA_HOME 
/opt/jdk/jdk-14.0.2+12

[purejava]

it had no effect when I implemented the above statement in UnlockModule. Therefore it's implemented in KeychainManager.

I need to correct this statement: it had no effect on creating a vault and on selecting a vault, but it does work for clicking Unlock.

It should, though. Accessing the keychain when it is locked, should cause an IllegalStateException, not return null. Let's rather find the cause why the abovementioned didn't work as expected.

Looking through the code I've found the right places for the changes.
The unwanted prompt to unlock the backends comes up on these occasions:

• on creating a vault
• on selecting a vault
• on clicking Unlock
• on clicking Vault Options after a vault has been selected on the left

I've tested the changes on both Linux backends and will create a PR.

Please note, that on a locked backend, with this PR, a user that changes the password for a vault does not change the one that is saved in the backend!

I don't know, if this is wanted. The alternative would be to always prompt to unlock the backend in case a password gets changed and the backend is locked.

Both have pros and cons. For the user who brought up this issue (does not want to interact with a backend at all) the PR is fine. On the other hand, I can imagine a user that boots his machine to just quickly change the password of a vault and has her backend locked at that time. For her, the password in the backend would stay unchanged.

You decide.

[infeo]

An idea would be to add an ui element which reminds the user that the selected keychain is locked and changes in Cryptomator won't find its way into the used keychain.

An alternative would be implementing a checkbox in the general preferences which switches between ignore keychain if it is locked and the previous behaviour.

[overheadhunter]

We could add a new (nullable) value to settings.json which tells Cryptomator what keychain has been used to remember the passphrase.

• If it is null, then don't bother asking the user to unlock the keychain.
• If it is non-null and the keychain is unlocked: good.
• If it is non-null and the keychain is locked: Attempt to access the password, triggering a keychain-unlock

Special cases:

• False positive: If it is non-null but the keychain didn't find the requested item: Adjust setting to null
• False negative: If it is null but the keychain does have an entry: Shit happens 🤷‍♂️