Add Touch ID support #3311
Add Touch ID support #3311
Conversation
|
Warning Rate Limit Exceeded@purejava has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 21 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Adresses #2180. |
|
I wonder whether this implementation does suffice from the security point of view. You can enable Touch ID on capable devices, which unlocks vaults only after the fingerprint was provided, but this feature can be disabled in the prefs. Maybe only allow to disable Touch ID via a successful Touch ID authentication? Discussion welcome! Edit: added some more thoughts. |
|
I'm wondering if we should "just" refactor org_cryptomator_macos_keychain_MacKeychain_Native.m from the deprecated SecKeychain API to the "new" SecItem API. With With that, I don't think we even need a new "two-factor" API because these APIs will take care of the Touch ID prompts. And also make sure that the password is only accessible via Touch ID. I believe this is the "most secure" way instead of splitting up saving the password and prompting for Touch ID. What do you think? |
I would support this, however I am wondering if items stores with the old API are accessible from the new one. It is certainly more secure to let the OS combine authentication and secret storage instead of triggering biometric authentication ourselves without it being really required in order to access stored secrets. |
It could be possible. If I'm understanding TN3137 correctly, all these different keychain APIs target the same file-based keychain. The new API can also target the data protection keychain (which is recommended but not the default yet). If it doesn't work, we could migrate the keychain entries. All in all, this would require some experimentation. |
@tobihagemann @overheadhunter I raise my hand and take care of it. I already started implementing the Objective-C code. |
|
Thank you! 🙇♂️ Just a quick tip: I'm willing to raise the minimum version to 10.13.4 (currently, it's 10.13), so that you don't have to do if/else logic for this feature. |
|
Thank you @tobihagemann, that's a good advice! |
|
Despite the impl being in ObjC, if the API is C compatible, we could use Panama :) |
I used to create the Java bindings with But I think, that does not work here, as the header files on macOS are organized in a way that can not be processed by JNA has no problems with this and requires only an additional Java interface to access the lib. Edit: rephrased for better readability |
|
Writing the code was no problem, but I got stuck with configuring Xcode the right way. Googling for hours did not help. What's the problem? The code seems to work. Reading "old" keychain items that were created before without 10:16:55.905 [App Background Thread 002] ERROR o.c.ui.keyloading.KeyLoadingStrategy - Failed to store passphrase in system keychain.
org.cryptomator.integrations.keychain.KeychainAccessException: Failed to store password. Error code -34018
at org.cryptomator.integrations.mac@1.3.0-SNAPSHOT/org.cryptomator.macos.keychain.MacSystemKeychainAccess.storePassphrase(MacSystemKeychainAccess.java:43)
at org.cryptomator.desktop/org.cryptomator.common.keychain.KeychainManager.storePassphrase(KeychainManager.java:48)
at org.cryptomator.desktop/org.cryptomator.ui.keyloading.masterkeyfile.MasterkeyFileLoadingStrategy.savePasswordToSystemkeychain(MasterkeyFileLoadingStrategy.java:117)
at org.cryptomator.desktop/org.cryptomator.ui.keyloading.masterkeyfile.MasterkeyFileLoadingStrategy.cleanup(MasterkeyFileLoadingStrategy.java:107)
at org.cryptomator.desktop/org.cryptomator.ui.keyloading.KeyLoadingStrategy.use(KeyLoadingStrategy.java:89)
at org.cryptomator.desktop/org.cryptomator.ui.unlock.UnlockWorkflow.call(UnlockWorkflow.java:70)
at org.cryptomator.desktop/org.cryptomator.ui.unlock.UnlockWorkflow.call(UnlockWorkflow.java:31)
at javafx.graphics/javafx.concurrent.Task$TaskCallable.call(Task.java:1399)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
at java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:718)
at java.base/java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:482)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
at java.base/java.lang.Thread.run(Thread.java:1583)
10:16:55.906 [JavaFX Application Thread] INFO o.c.ui.unlock.UnlockWorkflow - Unlock of 'Test' succeeded.Error code But: where to configure this for a library? The Xcode library code project does not have the "Signing & Capabilities" tab (picture). Are there Xcode cracks around, @tobihagemann maybe? 😄 |
|
I tried something and manually edited the Setting signing and entitlements up and running does lead to another error: Caused by: java.lang.UnsatisfiedLinkError: dlopen(/Users/ralph/Library/Caches/JNA/temp/jna5535787194538925389.tmp, 0x0009): tried: '/Users/ralph/Library/Caches/JNA/temp/jna5535787194538925389.tmp' (code signature invalid in <9734A791-82DB-377F-BD23-FE563CBD2556> '/Users/ralph/Library/Caches/JNA/temp/jna5535787194538925389.tmp' (errno=1) sliceOffset=0x00014000, codeBlobOffset=0x0000CA40, codeBlobSize=0x00004B20), '/System/Volumes/Preboot/Cryptexes/OS/Users/ralph/Library/Caches/JNA/temp/jna5535787194538925389.tmp' (no such file), '/Users/ralph/Library/Caches/JNA/temp/jna5535787194538925389.tmp' (code signature invalid in <9734A791-82DB-377F-BD23-FE563CBD2556> '/Users/ralph/Library/Caches/JNA/temp/jna5535787194538925389.tmp' (errno=1) sliceOffset=0x00014000, codeBlobOffset=0x0000CA40, codeBlobSize=0x00004B20)
at com.sun.jna@5.12.1/com.sun.jna.Native.open(Native Method)
at com.sun.jna@5.12.1/com.sun.jna.NativeLibrary.loadLibrary(NativeLibrary.java:283)
at com.sun.jna@5.12.1/com.sun.jna.NativeLibrary.getInstance(NativeLibrary.java:467)
at com.sun.jna@5.12.1/com.sun.jna.Library$Handler.<init>(Library.java:192)
at com.sun.jna@5.12.1/com.sun.jna.Native.load(Native.java:622)
at com.sun.jna@5.12.1/com.sun.jna.Native.load(Native.java:596)
at org.cryptomator.integrations.mac@1.3.0-SNAPSHOT/org.cryptomator.macos.keychain.MacSystemKeychainAccess.<clinit>(MacSystemKeychainAccess.java:31)
at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized0(Native Method)
at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized(Unsafe.java:1160)
at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.ensureClassInitialized(MethodHandleAccessorFactory.java:300)
at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.newConstructorAccessor(MethodHandleAccessorFactory.java:103)
at java.base/jdk.internal.reflect.ReflectionFactory.newConstructorAccessor(ReflectionFactory.java:200)
at java.base/java.lang.reflect.Constructor.acquireConstructorAccessor(Constructor.java:549)
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:789)
... 56 common frames omittedI do not have a purchased code signing certificate or enrolled in the Apple developer program. Could this cause the error? |
|
Yeah, code signing is no fun. 😅 We are enrolled in the Apple Developer program, but we can't share the certificate publicly. To be honest, I don't even know what we should do exactly. Cryptomator, the main app, doesn't have any code signing set up for development. It will only be code signed during distribution. I have the feeling, even with the certificate installed, I have to build the app and can't test this by running it in the IDE. Apart from that: Total Java noob here, but I've looked through your commits and the native integration seems totally different from the rest. Did we use JNI in the past and you're now using JNA? I'm just wondering if this is necessary. It seems like that we would then distribute two |
Yeah, nobody should share her certificate, that's for sure and that wasn't my intention, of course. I know, that Cryptomator uses the certificate for signing for distribution, not before. Unfortunately, if we want to use the new What I asked for is to take the ready to compile code from this branch and use it as a dependency for Cryptomator run in an IDE on Mac. As the code contains me as the "Development team" (
Yes, it's two libs. For the question on why JNA and not Panama, please see my #3311 (comment) before. But you mentioned JNI. This requires C code and someone would need to write it. I am not capable of doing that. 🤷🏻♂️ |
|
Due to the adjustments on the |
|
After learning how this Java - Objective-C - C type conversions work, I reworked the The changes needed for the Cryptomator part aren't finished yet, as it needs to be designed on how to make the Cryptomator Touch ID setting available for The entitlement issues raised in #3311 (comment) and #3311 (comment) are still there, but should go away, once the app is compiled by the Cryptomator team, because you have a valid certificate for signing available. |
Depends on cryptomator/integrations-mac#36.