Add Touch ID provider #58
Conversation
WalkthroughThis pull request introduces several modifications to the macOS Keychain integration within the Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java(1 hunks)src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java(1 hunks)src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.TouchIdKeychainAccessProvider(1 hunks)src/main/resources/MacIntegrationsBundle.properties(1 hunks)
🔇 Additional comments (3)
src/main/resources/MacIntegrationsBundle.properties (1)
1-2: LGTM! Display names follow Apple's branding guidelines.
The added Touch ID display name maintains consistency with Apple's official branding.
src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java (2)
37-40: Request TouchIdKeychainAccess implementation for security review
Since this PR introduces Touch ID support, it's crucial to review the TouchIdKeychainAccess implementation to ensure:
- Proper handling of Touch ID authentication
- Appropriate error handling for biometric failures
- Secure fallback mechanisms
- Correct usage of Apple's LocalAuthentication framework
Could you please provide the TouchIdKeychainAccess implementation for security review?
✅ Verification successful
Touch ID implementation review completed - implementation is secure
The implementation shows proper security considerations:
- Touch ID authentication is correctly enforced through the
requireOsAuthenticationparameter in the native keychain operations - Secure password handling with proper memory cleanup (zeroing out buffers)
- Appropriate error handling and exceptions for keychain operations
- Secure fallback to password authentication when Touch ID is unavailable (handled by macOS system keychain)
- Native integration with Apple's Security framework through JNI
The implementation follows security best practices and properly integrates with macOS's security mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Try to locate TouchIdKeychainAccess implementation
echo "Searching for TouchIdKeychainAccess implementation..."
fd -e java TouchIdKeychainAccess
Length of output: 223
Script:
#!/bin/bash
# Get the implementation of TouchIdKeychainAccess
cat src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java
Length of output: 2512
Script:
#!/bin/bash
# Let's examine the MacKeychain implementation to understand the Touch ID authentication details
cat src/main/java/org/cryptomator/macos/keychain/MacKeychain.java
Length of output: 5055
37-40:
Consider security implications of storing without OS authentication
The new overloaded method always stores passphrases without OS authentication (false). This could potentially be a security risk if used incorrectly.
Consider:
- Adding Javadoc to clearly document the security implications
- Adding a warning annotation like
@SecurityNoteif available - Verifying that this method is only used in appropriate contexts (e.g., when Touch ID is the primary authentication method)
Would you like me to propose a documentation update for this method?
| public void changePassphrase(String key, String displayName, CharSequence passphrase) throws KeychainAccessException { | ||
| if (keychain.deletePassword(SERVICE_NAME, key)) { | ||
| keychain.storePassword(SERVICE_NAME, key, passphrase, true); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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); | |
| } |
| @@ -0,0 +1 @@ | |||
| org.cryptomator.macos.keychain.TouchIdKeychainAccess | |||
There was a problem hiding this comment.
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.
| @@ -14,7 +15,7 @@ | |||
| requires org.slf4j; | |||
|
|
|||
| provides AutoStartProvider with MacAutoStartProvider; | |||
| provides KeychainAccessProvider with MacSystemKeychainAccess; | |||
| provides KeychainAccessProvider with MacSystemKeychainAccess, TouchIdKeychainAccess; | |||
There was a problem hiding this comment.
💡 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider (1)
1-2: Consider documenting provider selection orderSince multiple providers are registered, it would be helpful to document how the system chooses between them (e.g., if MacSystemKeychainAccess is the fallback when Touch ID is unavailable).
Consider adding a comment above the providers explaining the selection strategy:
+# Providers are tried in order. TouchIdKeychainAccess is selected when Touch ID is available, +# otherwise falls back to MacSystemKeychainAccess org.cryptomator.macos.keychain.MacSystemKeychainAccess org.cryptomator.macos.keychain.TouchIdKeychainAccess
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider(1 hunks)
🔇 Additional comments (1)
src/main/resources/META-INF/services/org.cryptomator.integrations.keychain.KeychainAccessProvider (1)
1-2: Verify provider implementations
The service configuration looks correct. Let's verify that both provider classes exist and properly implement the KeychainAccessProvider interface.
✅ Verification successful
Service provider configuration is correctly implemented
Both provider classes exist in the correct package and properly implement the KeychainAccessProvider interface. The verification confirms:
- Both files are present in
src/main/java/org/cryptomator/macos/keychain/ - Both classes are properly annotated with
@OperatingSystem(OperatingSystem.Value.MAC) - Both classes implement
KeychainAccessProvider - No other implementations of
KeychainAccessProviderwere found - The order in the service configuration file matches the implementation files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that both provider classes exist and implement KeychainAccessProvider
# Check if the provider classes exist and implement the interface
ast-grep --pattern 'class $CLASS implements $$$KeychainAccessProvider$$$' \
--pattern 'class $CLASS extends $$$ implements $$$KeychainAccessProvider$$$'
# Check for any potential additional implementations that might not be registered
ast-grep --pattern 'implements $$$KeychainAccessProvider$$$'
Length of output: 382
Script:
#!/bin/bash
# Let's verify each class separately and check their implementations
# Check MacSystemKeychainAccess implementation
echo "=== Checking MacSystemKeychainAccess ==="
ast-grep --pattern 'class MacSystemKeychainAccess implements $$$KeychainAccessProvider$$$'
# Check TouchIdKeychainAccess implementation
echo "=== Checking TouchIdKeychainAccess ==="
ast-grep --pattern 'class TouchIdKeychainAccess implements $$$KeychainAccessProvider$$$'
# Let's also check the actual files exist
echo "=== Checking file existence ==="
fd "MacSystemKeychainAccess.java|TouchIdKeychainAccess.java"
# Let's see the package declarations to verify the fully qualified names
echo "=== Checking package declarations ==="
rg "^package.*macos\.keychain" -A 1
Length of output: 1885
Script:
#!/bin/bash
# Let's check the actual content of both implementation files to verify interface implementation
echo "=== MacSystemKeychainAccess content ==="
rg "implements.*KeychainAccessProvider" src/main/java/org/cryptomator/macos/keychain/MacSystemKeychainAccess.java -B1 -A1
echo "=== TouchIdKeychainAccess content ==="
rg "implements.*KeychainAccessProvider" src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java -B1 -A1
# Also check if there are any other implementations we might have missed
echo "=== Other implementations ==="
rg "implements.*KeychainAccessProvider" --type java
Length of output: 1066
|
|
||
| @Override | ||
| public boolean isSupported() { | ||
| return true; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I agree with @infeo. Users without Touch ID should not be forced to enter the login password on every unlock.
| * 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) |
There was a problem hiding this comment.
I'd say, Touch ID has a higher Priority than Non-Touch ID. BUT
- the
isSupportedmethod must properly detect, if touch ID is set up/enabled - the old, non-biometric-protected data is migrated
There was a problem hiding this comment.
I'd say, Touch ID has a higher Priority than Non-Touch ID. BUT
- the
isSupportedmethod 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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h (1)
34-40: Consider using LAContext for robust Touch ID detectionThe native implementation should use LAContext to properly detect if Touch ID is set up and enabled. This aligns with Apple's recommended approach for biometric authentication support.
Reference:
src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m (1)
127-136: LGTM! Consider enhancing biometric type detectionThe implementation correctly checks Touch ID availability using the LocalAuthentication framework with proper error handling and logging. It efficiently reuses the shared context and follows Apple's guidelines.
Consider these potential enhancements:
- Distinguish between different biometric authentication types (Touch ID vs Face ID)
- Provide more detailed error information to the Java layer for better user feedback
Here's a suggested enhancement that supports different biometric types:
JNIEXPORT jboolean JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_isTouchIDavailable(JNIEnv *env, jobject thisObj) { NSError *error = nil; LAContext *context = getSharedLAContext(); if ([context canEvaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics error:&error]) { + // Check specific biometric type + if (@available(macOS 10.13.2, *)) { + switch (context.biometryType) { + case LABiometryTypeTouchID: + return JNI_TRUE; + default: + NSLog(@"Biometric type is not Touch ID: %ld", (long)context.biometryType); + return JNI_FALSE; + } + } return JNI_TRUE; } else { NSLog(@"Touch ID is not available: %@", error.localizedDescription); return JNI_FALSE; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/headers/org_cryptomator_macos_keychain_MacKeychain_Native.h(1 hunks)src/main/java/org/cryptomator/macos/keychain/MacKeychain.java(2 hunks)src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java(1 hunks)src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m(1 hunks)src/test/java/org/cryptomator/macos/keychain/KeychainAccessProviderTest.java(1 hunks)
🔇 Additional comments (5)
src/main/java/org/cryptomator/macos/keychain/TouchIdKeychainAccess.java (3)
58-61: Review isLocked() implementation
The method always returns false, which might not accurately reflect the actual lock state of Touch ID or the keychain.
Consider implementing proper lock state detection or document why it always returns false.
69-73:
Handle failure in changePassphrase method
The method silently fails if password deletion fails, which could leave the keychain in an inconsistent state.
Apply this fix:
@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);
- }
+ if (!keychain.deletePassword(SERVICE_NAME, key)) {
+ throw new KeychainAccessException("Failed to delete existing passphrase");
+ }
+ keychain.storePassword(SERVICE_NAME, key, passphrase, true);
}16-17: Verify priority value against other providers
The @priority(1010) suggests this provider should take precedence. Ensure this aligns with the intended provider selection hierarchy, especially considering the fallback to password authentication when Touch ID is unavailable.
✅ Verification successful
Let me analyze the priority values found in the codebase:
Both are keychain providers, and TouchIdKeychainAccess has a higher priority (1010) than MacSystemKeychainAccess (1000), which is the correct hierarchy since Touch ID should be preferred when available, with password authentication (MacSystemKeychainAccess) as a fallback.
Let me verify if there are any other keychain providers I might have missed:
Let me try a different approach to find KeychainAccessProvider implementations:
Priority value of TouchIdKeychainAccess is correctly set
The priority value of 1010 for TouchIdKeychainAccess is appropriate as:
- It's higher than MacSystemKeychainAccess (1000), ensuring Touch ID is preferred when available
- Both providers are registered in module-info.java as KeychainAccessProvider implementations
- No other KeychainAccessProvider implementations exist in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check priority values of other KeychainAccessProvider implementations
rg -l "@Priority" | xargs rg "@Priority\([0-9]+\)"
Length of output: 591
Script:
#!/bin/bash
# Find all implementations of KeychainAccessProvider
ast-grep --pattern 'class $_ implements KeychainAccessProvider'
Length of output: 65
Script:
#!/bin/bash
# Search for KeychainAccessProvider in the codebase
rg "KeychainAccessProvider" -B 1 -A 1
Length of output: 3136
src/main/java/org/cryptomator/macos/keychain/MacKeychain.java (2)
130-130: LGTM! Native method declaration is correct
The native method declaration follows JNI conventions and properly matches the Java interface.
107-114: LGTM! Verify behavior with system settings
The implementation correctly delegates to the native method and is well-documented. The boolean return type appropriately represents the availability state.
Let's verify that the Touch ID availability check aligns with system settings:
✅ Verification successful
LGTM! Implementation correctly checks system-level Touch ID availability
The native implementation in org_cryptomator_macos_keychain_MacKeychain_Native.m properly uses LAContext to check Touch ID availability by evaluating the LAPolicyDeviceOwnerAuthenticationWithBiometrics policy, which is the Apple-recommended way to determine biometric authentication support. The check will automatically align with system settings as it uses the official LocalAuthentication framework.
- Native code at
src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.musescanEvaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometricsto verify Touch ID availability - The implementation is used correctly by
TouchIdKeychainAccessprovider for authentication decisions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Touch ID related system setting checks
# Look for any additional Touch ID validation or system preference checks
rg -g '*.{m,h,java}' -i 'touchid|biometric|LAContext|LAPolicyDeviceOwnerAuthenticationWithBiometrics'
Length of output: 3344
| @@ -12,7 +12,7 @@ public class KeychainAccessProviderTest { | |||
| public void testLoadMacSystemKeychainAccess() { | |||
| var provider = KeychainAccessProvider.get().findAny(); | |||
| Assertions.assertTrue(provider.isPresent()); | |||
| Assertions.assertInstanceOf(MacSystemKeychainAccess.class, provider.get()); | |||
| Assertions.assertInstanceOf(TouchIdKeychainAccess.class, provider.get()); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for devices without Touch ID
While the test verifies that TouchIdKeychainAccess is loaded as the default provider, it doesn't cover the fallback scenario for devices without Touch ID. Consider adding test cases to verify the provider selection behavior when Touch ID is unavailable.
@Test
@DisplayName("MacSystemKeychainAccess can be loaded")
public void testLoadMacSystemKeychainAccess() {
var provider = KeychainAccessProvider.get().findAny();
Assertions.assertTrue(provider.isPresent());
Assertions.assertInstanceOf(TouchIdKeychainAccess.class, provider.get());
}
+@Test
+@DisplayName("Falls back to MacSystemKeychainAccess when Touch ID is unavailable")
+public void testFallbackWhenTouchIdUnavailable() {
+ // Mock Touch ID unavailability
+ var touchIdProvider = new TouchIdKeychainAccess(new MacKeychain() {
+ @Override
+ public boolean isTouchIDavailable() {
+ return false;
+ }
+ });
+ Assertions.assertFalse(touchIdProvider.isSupported());
+}Committable suggestion skipped: line range outside the PR's diff.
| public void deletePassphrase(String key) throws KeychainAccessException { | ||
| keychain.deletePassword(SERVICE_NAME, key); | ||
| } |
There was a problem hiding this comment.
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.
| 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 boolean isSupported() { | ||
| return keychain.isTouchIDavailable(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance isSupported() implementation
The current implementation only checks if Touch ID is available. Consider enhancing it to also verify if Touch ID is properly set up and enabled in system preferences.
@Override
public boolean isSupported() {
- return keychain.isTouchIDavailable();
+ return keychain.isTouchIDavailable() && keychain.isTouchIDEnabled();
}Committable suggestion skipped: line range outside the PR's diff.
|
Ready for another review @infeo. |
Following the recommendation in the PR for Windows Hello, Touch ID gets its own provider.