-
-
Notifications
You must be signed in to change notification settings - Fork 951
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.
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 support #3311
base: develop
Are you sure you want to change the base?
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 :) |
This comment was marked as outdated.
This comment was marked as outdated.
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? 😄 EDIT: configuring this for a library is the wrong approach, see #3311 (comment) below. |
This comment was marked as outdated.
This comment was marked as outdated.
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. 🤷🏻♂️ |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
While still working on Touch ID support, I'd like to offer the tested and mature code so far to replace the deprecated |
I ruled out the Error code The correct entitlement for accessing data protection keychain items is the following: <key>keychain-access-groups</key>
<array/> In addition to that, we need a provisioning profile: In contrast, restricted entitlements must be authorized by a provisioning profile. This is an important security feature on macOS. For example, the fact that the keychain-access-groups entitlement must be authorized by a profile means that other developers can’t impersonate your app in order to steal its keychain items. macOS expects to find the profile at Finally, the Cryptomator entitlements need to match the entitlements in the provisioning profile, namely the keys index 22d0f6954d..178c0fc974 100644
--- a/dist/mac/Cryptomator.entitlements
+++ b/dist/mac/Cryptomator.entitlements
@@ -2,6 +2,10 @@
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
+ <key>com.apple.application-identifier</key>
+ <string>XXXXXXXXXX.net.plawetzki.cryptomator</string>
+ <key>com.apple.developer.team-identifier</key>
+ <string>XXXXXXXXXX</string>
<key>com.apple.security.cs.allow-jit</key>
<true/>
<key>com.apple.security.cs.allow-unsigned-executable-memory</key> The former needs to match the App ID in the provisioning profile: |
Depends on cryptomator/integrations-mac#36.Obsolete.