Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Touch ID provider #58

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Add Touch ID provider #58

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eabcbc9
Add Touch ID provider
purejava Dec 1, 2024
f54f550
One provider configuration file is enough
purejava Dec 2, 2024
c909265
Higher Touch ID priority
purejava Dec 12, 2024
b1cdd16
Add check, whether Touch ID is available on the device
purejava Dec 12, 2024
f88d9d5
Both KeychainAccessProviders need to be possible
purejava Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import org.cryptomator.integrations.uiappearance.UiAppearanceProvider;
import org.cryptomator.macos.autostart.MacAutoStartProvider;
import org.cryptomator.macos.keychain.MacSystemKeychainAccess;
import org.cryptomator.macos.keychain.TouchIdKeychainAccess;
import org.cryptomator.macos.revealpath.OpenCmdRevealPathService;
import org.cryptomator.macos.tray.MacTrayIntegrationProvider;
import org.cryptomator.macos.uiappearance.MacUiAppearanceProvider;
Expand All @@ -14,7 +15,7 @@
requires org.slf4j;

provides AutoStartProvider with MacAutoStartProvider;
provides KeychainAccessProvider with MacSystemKeychainAccess;
provides KeychainAccessProvider with MacSystemKeychainAccess, TouchIdKeychainAccess;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Both providers have the same priority level

Both TouchIdKeychainAccess and MacSystemKeychainAccess are annotated with @Priority(1000). Since they have equal priority, the service loader's selection between them might be non-deterministic. Consider assigning different priority values to establish a clear preference order between these authentication methods.

  • src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java: Currently @Priority(1000)
  • src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java: Currently @Priority(1000)
🔗 Analysis chain

Verify provider priority handling

The module now provides two KeychainAccessProvider implementations. We should ensure proper priority handling between TouchIdKeychainAccess and MacSystemKeychainAccess for correct fallback behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check Priority annotations on both providers
echo "Checking provider priority annotations..."
ast-grep --pattern 'class $CLASS implements KeychainAccessProvider {
  $$$
}' | grep -B 2 'implements KeychainAccessProvider'

Length of output: 220

Script:

#!/bin/bash
# Search for provider class files and their content
echo "Searching for KeychainAccessProvider implementations..."
fd -e java | xargs rg -l "implements KeychainAccessProvider" | while read -r file; do
    echo "=== $file ==="
    rg -B3 "implements KeychainAccessProvider" "$file"
done

# Look for any priority-related annotations
echo -e "\nChecking for priority annotations..."
rg -i "priority|@Order" $(fd -e java)

Length of output: 4072

provides RevealPathService with OpenCmdRevealPathService;
provides TrayIntegrationProvider with MacTrayIntegrationProvider;
provides UiAppearanceProvider with MacUiAppearanceProvider;
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ public String displayName() {
return Localization.get().getString("org.cryptomator.macos.keychain.displayName");
}

@Override
public void storePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
keychain.storePassword(SERVICE_NAME, key, passphrase, false);
}

@Override
public void storePassphrase(String key, String displayName, CharSequence passphrase, boolean requireOsAuthentication) throws KeychainAccessException {
keychain.storePassword(SERVICE_NAME, key, passphrase, requireOsAuthentication);
Expand Down
75 changes: 75 additions & 0 deletions src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package org.cryptomator.macos.keychain;

import org.cryptomator.integrations.common.OperatingSystem;
import org.cryptomator.integrations.common.Priority;
import org.cryptomator.integrations.keychain.KeychainAccessException;
import org.cryptomator.integrations.keychain.KeychainAccessProvider;
import org.cryptomator.macos.common.Localization;

/**
* Stores passwords in the macOS system keychain. Requires an authenticated user to do so.
* Authentication is done via TouchID or password as a fallback, when TouchID is not available.
* <p>
* Items are stored in the default keychain with the service name <code>Cryptomator</code>, unless configured otherwise
* using the system property <code>cryptomator.integrationsMac.keychainServiceName</code>.
*/
@Priority(1000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say, Touch ID has a higher Priority than Non-Touch ID. BUT

  • the isSupported method must properly detect, if touch ID is set up/enabled
  • the old, non-biometric-protected data is migrated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say, Touch ID has a higher Priority than Non-Touch ID. BUT

  • the isSupported method must properly detect, if touch ID is set up/enabled
  • the old, non-biometric-protected data is migrated

Both is implemented. Please not, due to the higher priority, on new installations, Touch ID will be set up as the KeychainAccessProvider.

@OperatingSystem(OperatingSystem.Value.MAC)
public class TouchIdKeychainAccess implements KeychainAccessProvider {

private static final String SERVICE_NAME = System.getProperty("cryptomator.integrationsMac.keychainServiceName", "Cryptomator");

private final MacKeychain keychain;

public TouchIdKeychainAccess() {
this(new MacKeychain());
}

// visible for testing
TouchIdKeychainAccess(MacKeychain keychain) {
this.keychain = keychain;
}

@Override
public String displayName() {
return Localization.get().getString("org.cryptomator.macos.keychain.touchIdDisplayName");
}

@Override
public void storePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
keychain.storePassword(SERVICE_NAME, key, passphrase, true);
}

@Override
public void storePassphrase(String key, String displayName, CharSequence passphrase, boolean requireOsAuthentication) throws KeychainAccessException {
keychain.storePassword(SERVICE_NAME, key, passphrase, requireOsAuthentication);
}

@Override
public char[] loadPassphrase(String key) {
return keychain.loadPassword(SERVICE_NAME, key);
}

@Override
public boolean isSupported() {
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be improved by calling native functions. See https://stackoverflow.com/questions/46887547/how-to-programmatically-check-support-of-face-id-and-touch-id and https://developer.apple.com/documentation/localauthentication/lacontext/biometrytype?language=objc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but is this needed as we use kSecAccessControlUserPresence and fall back to password, when Touch ID is not available?

Restricting this to devices with Touch ID would making access to keychain items more secure for owners of non Touch ID devices impossible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the provider is called "Touch ID".^^ Also, i'm asking myself: As a user, do i expect to always enter my login pasword on every unlock, although i clicked the "save password" checkbox?

@tobihagemann What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @infeo. Users without Touch ID should not be forced to enter the login password on every unlock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

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

@Override
public void deletePassphrase(String key) throws KeychainAccessException {
keychain.deletePassword(SERVICE_NAME, key);
}
Comment on lines +64 to +66
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance error handling in deletePassphrase

Similar to the changePassphrase method, deletePassphrase should throw an exception if the deletion fails.

@Override
public void deletePassphrase(String key) throws KeychainAccessException {
-    keychain.deletePassword(SERVICE_NAME, key);
+    if (!keychain.deletePassword(SERVICE_NAME, key)) {
+        throw new KeychainAccessException("Failed to delete passphrase");
+    }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void deletePassphrase(String key) throws KeychainAccessException {
keychain.deletePassword(SERVICE_NAME, key);
}
public void deletePassphrase(String key) throws KeychainAccessException {
if (!keychain.deletePassword(SERVICE_NAME, key)) {
throw new KeychainAccessException("Failed to delete passphrase");
}
}


@Override
public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
if (keychain.deletePassword(SERVICE_NAME, key)) {
keychain.storePassword(SERVICE_NAME, key, passphrase, true);
}
}
Comment on lines +69 to +73
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle failure in deletePassword within changePassphrase method

In the changePassphrase method, if deletePassword returns false, the new passphrase is not stored, and no exception is thrown. This could leave the keychain in an inconsistent state.

Apply this diff to handle the failure appropriately:

@Override
public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
-    if (keychain.deletePassword(SERVICE_NAME, key)) {
+    if (!keychain.deletePassword(SERVICE_NAME, key)) {
+        throw new KeychainAccessException("Failed to delete existing passphrase.");
+    }
    keychain.storePassword(SERVICE_NAME, key, passphrase, true);
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
if (keychain.deletePassword(SERVICE_NAME, key)) {
keychain.storePassword(SERVICE_NAME, key, passphrase, true);
}
}
public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException {
if (!keychain.deletePassword(SERVICE_NAME, key)) {
throw new KeychainAccessException("Failed to delete existing passphrase.");
}
keychain.storePassword(SERVICE_NAME, key, passphrase, true);
}


}
1 change: 1 addition & 0 deletions ...ces/META-INF/services/org.cryptomator.integrations.keychain.TouchIdKeychainAccessProvider
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.cryptomator.macos.keychain.TouchIdKeychainAccess
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Incorrect service provider filename

The service provider configuration file should be named org.cryptomator.integrations.keychain.KeychainAccessProvider, not TouchIdKeychainAccessProvider. According to Java's Service Loader mechanism, the filename in META-INF/services must match the fully qualified name of the service interface.

To fix this issue, rename the file and ensure it contains the correct implementation class:

  • Rename the file:

    src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider

  • Ensure the file content lists the implementation class:

    -org.cryptomator.macos.keychain.TouchIdKeychainAccess
+org.cryptomator.macos.keychain.TouchIdKeychainAccess

Committable suggestion skipped: line range outside the PR's diff.

3 changes: 2 additions & 1 deletion src/main/resources/MacIntegrationsBundle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
org.cryptomator.macos.keychain.displayName=macOS Keychain
org.cryptomator.macos.keychain.displayName=macOS Keychain
org.cryptomator.macos.keychain.touchIdDisplayName=Touch ID
infeo marked this conversation as resolved.
Show resolved Hide resolved