TouchID unlock Feature for MacPass. #1089
Conversation
|
Cool so is this fully working? I had been working on it but just have had limited time to finish. There was one other person who was working on GUI implementation which was the last piece. Did you fork out work or is this from scratch? |
|
Ok So I read further. I will take a look. I am not married to the implementation I was working on. If this is a better and the preferred direction. |
|
@georgesnow Its written from scratch and with the |
|
@juliuszint i tried it. I am having code signing issues with enabling Keychain sharing. I believe I have been able to use this entitlement before without a paid developer cert. I could be wrong though. I can't seem to get the correct configuration as it crashes on launch because of the code signing. This would be my only hesitation with this implementation. If you have to have paid dev cert to use this entitlement. Then for the people who build MacPass from code (like me). They won't be able use this feature without the paid developer cert. Or you have to add flags to compile with the feature turned off, which adds complexity. I like that this implementation as it seems to be more secure. It will require more investigation to figure out if and how you can build it without paid developer cert. |
|
@georgesnow thanks for trying it out. My own Apple account is not a paid either, but it is part of a Team that pays the 100$ annually (not sure if this makes it a paid account). With the latest changes that i have pushed, i was able to build an run it locally and have the TouchID Feature working as expected for a single database. I would appreciate if you could try it as well by doing the following steps.
|
MacPass/MPPasswordInputController.m
Outdated
| } | ||
| else { | ||
| NSError *err = CFBridgingRelease(error); | ||
| NSLog(@"Error while trying decrypt password for TouchID unlock: %@", [err description]); |
There was a problem hiding this comment.
Missing to between trying and decrypt.
There was a problem hiding this comment.
Fixed. Thanks for the review.
|
From what I gathered on apple’s website that keychain sharing doesn’t require a paid developer certificate. I will give it go. |
|
I tried to give it a fair shot to try to build MacPass:
I'm inclined to wait for official releases, at this point. Unless people are prepared to do some more hand holding. |
|
Not a mac coder, but here is what I tried:
|
I think the only thing you need to do is to add the |
|
Thanks for that tip @juliuszint . For an existing cloned directory, I issued: and that brought in submodule |
|
I have a Perhaps I missed something. Thanks in advance. |
|
Works fine with a |
That is indeed something i have not implemented yet. I have to take a closer look at how big these keyfiles are and whether it is possible to pass a memory stream to the underlying layers instead of a filename. |
|
The latest commit enables support for multiple Databases. Any feedback is appreciated. |
|
-edit- never mind. I was using the wrong |
It was indendet for everyone who wants to give it a shot, but keyfiles are sadly still not supported. But you can now for example unlock |
Currently the completion handler uses the password and the key file url as parameters. You store the encrypted password and restore it when unlocking. Refactoring this to use the Another way might be to simply store the url as well and then retrieve it. This would add a bit of security since the key-file still needs to be present when unlocking (e.g. when it's stored on a thumb drive) |
This seems to me like the best option after thinking about it. While TouchID unlock can eliminate attack vectors like shoulder surfing, when you unlock your database in a public place. Ultimately it lowers the security, by introducing another way (that might contain security vulnerabilities) to unlock your database. The TouchID feature should be optional anyway, so if the user decides to trade in security for ease of use, then we might as well cache the keyfile in memory. @mstarke Would you agree with that? |
|
Databases with key files are now supported. If you want to try it out, you have to apply the following patch first. After this you need to run |
|
Note to self: Can't get the |
Some more information would be good. First: The Xcode Application output while Debugging would be good. Second: The output of |
Not sure if this is what you're looking for, but in Xcode, under git diff: Hope that helps. |
|
I think i have spotted a mistake on my side. I updated the diff that needs to be applied to the Cartfile. Would you be willing to give it one more shot? Revert the Cartfile with |
|
Output of |
|
Output of |
|
Thats odd. I myself am no expert with carthage. You could try to just update the resolved file itself and replace the KeePassKit line. At that point it should work in my opinion. But you do need to run |
|
Everything works (including unlock with touch ID while a database uses a key file):
Fantastic work and appreciate your help. Looking forward to the pull request being merged. |
|
My daily driver is the "other" program. Hoping to use MacPass as a daily driver. |
|
@juliuszint Just a heads-up. I've merged your code. I've started refactoring it to slim down the view controller a bit. The UI has not changed but I have an idea to use a more verbose system than the check-box. The three-state checkbox is an elegant solution but I did find it a bit too undiscoverable. MacPass tends to have those things but I have to try to minimize that so people actually find stuff and how it works :) |
|
Another status update: Merge is complete. Everything works. A couple of memory mananagement issues where also fixed in the process. I did change the touchID unlock button but this isn't final. I do want to change the ui a bit more. TouchID settings with a popup button. Hide the password and keyfile input when touchID is available. |
|
Can you push the merged branch if you get a chance? I’ve been working off of this branch for basically years now (rebuilding every month to get around the licensing issue) and would love to test it immediately. |
|
@hasnolen sure will do. |
|
The checkbox on the password input screen bothers me. All other password managers set those things inside the preferences since it's an global application setting and has nothing to do with an individual database. Would it be so bad to move this into an own settings tab "Biometrics" and let the user decide what to do there? This would also allow for some explanations to get the user to understand better what's going on? This then could be coupled with the regeneration of the encryptions keys and might even go as far as show a list of all known databases and the ability to purge them selectively? |
|
I should apologise to @juliuszint for taking so long for merging your awesome feature! |
|
🎉 🎉 👯 🥂 Thank you all! |
|
When can we expect a new release with this feature? |
|
@slashrsm When it's done. But I do intend to release it soon since there are also a couple of bug-fixes included @juliuszint I did encounter a bit of a problem and wanted to ask what your intentions where: When the touchID settings (checkbox) is changed, those changes are stored as a user default and then nothing happens. Only after unlocking via typing in the correct password/using the correct key-file does this setting make any effect. I would change this to the following:
|
There were to reasons for this approach.
All of the changes that you proposed will either improve the user experience or the security (hashed filenames) and are therefore a good thing. Sorry for taking so long to respond and I am looking forward to seeing the improvements that you are implementing. |
|
@juliuszint No worries about the delay. I'm the last one that is allowed to complain about having to wait. Your goal to keep it simple makes sense and explains your approach. About point 2: That's something I haven't – shockingly – thought about. There are a couple of ways to deal with this:
I tend to lean to 2 which makes this a mix of the current solution and the one I had in mind. |
|
I also think that 2 is the best approach. This also enables the use case of having a system with multiple KeePass files where the user wants to unlock one with TouchID and not the other. |
|
Hi, |
|
Any updates on releasing this change? 😊 |
|
It's been a while, any news on that? |
|
Also waiting for TouchID feature in MacPass, any ETA for release ? :) |
|
Oh man, what an ass I am. I'm currently drowning in work since we have a tight project that consumes a lot of my energy and spending time with my family is also a nice thing to do so development time for MacPass is sparse right now. I am fully aware of the dreadful timeline and will try my best to push this out ASAP. |
|
Thank you! Will wait |
|
@mstarke No worries! Personal life and health should always come first. Thank you for the update 😁 |
|
Hi @mstarke, any chance for an update on this feature? 😃 |
|
It's merged; it just hasn't been tagged and released; all* you have to do is clone and build from source. *took me about an hour of intense debugging to actually accomplish this. The issue(s) seem to be that Carthage fails to build the dependencies due to changes in dependent files packaged with XCode itself. Basically, it consisted of hunting down |
I tried doing this but failed. A quick guide would be very helpful. Thanks! |
|
I'll try to get a new release out in August to fix up some long standing issues and get development rolling again. |
|
@mstarke when will the new release be available? would love to have this feature |
|
Any chance we can get this feature in 2024? |
|
I built an unofficial release of MacPass that includes the fingerprint feature. You can find my build here: https://github.com/cathyjf/MacPass/releases/tag/0.8.1.1 |
|
@cathyjf awesome! I hope I can make the official version be compatible with your built so users will have a seamless transition. If this fails the only real issue then would be that all your TouchId unlocks won’t work anymore. Regarding the inner workings the keys have to be associated with a specific database file since the only way to truly determine uniqueness is via the root node id but this is only available for unlocked files. Currently the file name is used wich might require some changes to allow for multiple different files with the same name using different keys. My idea there was so store all known keys for a file name and then iterate through then to unlock the file. This then requires some housekeeping since we need to prevent those list to get too long. All that aside, I’m thankful for @cathyjf for providing the built until I get “my shit together” and release an official version with this feature! |
|
@mstarke, I think that because your release's app bundle will be signed with a different Team ID than mine, your release won't be able to access any items stored in keychain by the app bundle signed by my key. However, as you say, it really won't be a big deal -- it just means that, after upgrading to your build, users will need to authenticate once to their password database in the same way they did before the touch ID feature was added to the app. After that, users can go back to using touch ID normally. |
|
@cathyjf I totally forgot about the team id for signing since Touch ID forces the need to sign the app. Before this feature was present MacPass was able to work with all features without any signing so this is still a bit stuck in my head. Thanks for the clarification! |
Feature
This patch enables MacPass users to unlock their KeePass containers with TouchID. In contrast to #638, the implementation does not store the master password directly into the keychain of macOS. Instead it creates a RSA 2048 key pair and uses the public key to encrypt the password. The encrypted password is then kept in memory as long as the MacPass process is alive.
The private key is then used on a subsequent unlock to decrypt the master password. macOS will only return the private key if the device is unlocked and a user, who`s fingerprints were enrolled at the time the key pair was created, authenticates via TouchID.
On every launch of MacPass the user has to supply the master password once for TouchID unlock to work. This is a trade of between security and convenience. But on macOS system restarts are fewer than on windows and the MacPass process stays alive, even if all windows are closed.
Todo
SecKeyGeneratePairreturns with the error:Required entitlement isn't presentif theKeychain Sharingcapability is not enabled. For this reason i tested most of this code in a small test project with this capability enabled. For testing in MacPass i commented out this line. With this change the key is created successfully even if the entitlement is not set. This is only for testing purposes since the user is not prompted for authentication via TouchID.In short: it would be great if someone could enable this entitlement and build a signed version to actually verify it.
@DatabaseIDis used and needs to be replaced in order to support multiple databases.staticvariables are often viewed as bad practice. i am open to changing this to something that gets injected. For the first pull request this was the easiest way.