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.

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 support for quick unlock with TouchID on Macbook Pro #1851

Merged

Conversation

Projects
None yet
10 participants
@mxk6n
Copy link
Contributor

mxk6n commented Apr 16, 2018

Description

Unlock your database using TouchID on supported Macbook Pro models.

Motivation and context

With quick unlock one can simply unlock the database using TouchID without having to enter your password again while the application is still running. #209

Features

  • TouchID is a option that can be activated on the unlock screen
  • TouchID is only used for temporary quick unlock
    • Database has to be unlocked once after restart of the application in order to activate quick unlock using TouchID
  • TouchID quick unlock only replaces the password part of the unlock process
    • With quick unlock activated the password will be encrypted (AES 256) and then kept in memory
    • The random encryption key (and IV) will be protected by the KeyChain using TouchID
  • Next time you don't need to enter your password, just press "OK" to unlock your database with TouchID (however you will still need your additional database key, Yubikey, ...)

Compilation

  • Use -DWITH_XC_TOUCHID=ON
  • Needs code-signature and entitlements in order to work (see issue reference)

Possible future enhancements:

  • Provide abstraction layer for other OS/fingerprint reader.
  • Unlock with just putting on fingerprint w/o actively having to click unlock (KeyChain API may not allow this).
  • Indicate if TouchID is currently active (i.e. no password is needed)
    • e.g. show special button on TouchBar

How has this been tested?

  • So far only manually on MBP with TouchID.

Screenshots:

image

image

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
@@ -0,0 +1,212 @@
#define SECURITY_ATTRIBUTE_ACCOUNT CFSTR("KeepassXC TouchID Keys")
#define TOUCH_ID_PROMPT_MESSAGE CFSTR("authenticate to access KeepassXC database!")

This comment has been minimized.

Copy link
@RealOrangeOne

RealOrangeOne Apr 17, 2018

Contributor

From the screenshot, looks like the touchid UI automatically adds a . to the end of this message, making the exclamation mark seem out of place.

This comment has been minimized.

Copy link
@mxk6n

mxk6n Apr 17, 2018

Author Contributor

You're right, I'll remove it and replace the whole thing with QCoreApplication::translate(...).

@weslly

This comment has been minimized.

Copy link
Member

weslly commented Apr 20, 2018

Looks great! Unfortunately I don't have touch ID on my mac so this I'll be pretty hard to test :(

@mxk6n

This comment has been minimized.

Copy link
Contributor Author

mxk6n commented Apr 22, 2018

@weslly Does anyone of the development team have one to test with? Also, can you tell me why the build fails so I can make adjustments?

@weslly

This comment has been minimized.

Copy link
Member

weslly commented Apr 22, 2018

@kolhagen You can see the build log if you click in details and login as a guest at TeamCity

@TheZ3ro

This comment has been minimized.

Copy link
Member

TheZ3ro commented Apr 24, 2018

Please fix the conflicts and rebase (on top of develop)

@mxk6n mxk6n force-pushed the mxk6n:feature/macos-touchid-support branch from b45c8b7 to 298d39f Apr 24, 2018

@mxk6n mxk6n force-pushed the mxk6n:feature/macos-touchid-support branch 2 times, most recently from b3e9d4c to 8ca9f46 May 15, 2018

@mxk6n

This comment has been minimized.

Copy link
Contributor Author

mxk6n commented May 15, 2018

Did another rebase. Any updates on a possible integration?

@weslly

This comment has been minimized.

Copy link
Member

weslly commented May 16, 2018

I haven't found any easy way to simulate TouchID on macOS like you can do on iOS simulator, but it seems there's a workaround:

https://stackoverflow.com/questions/40414077/how-can-you-test-touch-id-for-macos

@AyaTech
Copy link

AyaTech left a comment

works fine, excellent feature.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 15, 2018

I think we can merge this

@weslly
Copy link
Member

weslly left a comment

I tried using LAPolicyDeviceOwnerAuthentication instead of LAPolicyDeviceOwnerAuthenticationWithBiometrics as suggested by that stackoverflow answer and it didn't work, but it also didn't seem to break anything either so I guess we can merge this and see if anyone else with TouchID has problems with this feature. But before that we need to have some kind of test to hide the TouchID checkbox(es) for devices that don't have the sensor available.

@weslly

weslly approved these changes Jun 16, 2018

@weslly

This comment has been minimized.

Copy link
Member

weslly commented Jun 16, 2018

Actually it already seems to hide the checkbox if I change the policy back to DeviceOwnerAuthenticationWithBiometrics

@droidmonkey
Copy link
Member

droidmonkey left a comment

Minor changes and a discussion point

m_ui->checkTouchID->setVisible(false);
#else
if (!TouchID::getInstance().isAvailable())
m_ui->checkTouchID->setVisible(false);

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Jun 16, 2018

Member

Wrap with { }

useTouchID.insert(m_filename, true);
}
} else {
// when TouchID not available or unchecked, reset for all databases

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Jun 16, 2018

Member

Technically this code only resets for the current database

public:
static TouchID& getInstance()
{
static TouchID instance; // Guaranteed to be destroyed.

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Jun 16, 2018

Member

I much prefer having this function body and global variable declared in TouchID.mm. Declaring it in the header may cause issues if we move this to an external library in the future.

@mxk6n mxk6n force-pushed the mxk6n:feature/macos-touchid-support branch from e738558 to c77286d Jun 17, 2018

@id-kemo

This comment has been minimized.

Copy link

id-kemo commented Jun 18, 2018

In case you would need any help with testing the feature, I can provide some sort of feedback or help if needed as I have a Macbook Pro 15'' with TouchID (2017 model).

@mxk6n

This comment has been minimized.

Copy link
Contributor Author

mxk6n commented Jun 18, 2018

There's still an open issue with the complexity of the method DatabaseOpenWidget::databaseKey().

Does it help splitting the method up (e.g. like this)?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jun 18, 2018

You can ignore that failure, I might remove codefactor as a check. You can't turn complexity to a warning....

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Jul 29, 2018

Hey, due to previous time constraints, I'm a little late to the review party. So first of all, thanks for your effort. I haven't really had a chance to look at the code yet, but I think at least the UI needs some tweaks. The most important issue is probably the alignment of the checkbox on the unlock screen. Please fix that before we merge. After this PR we should also have some discussion about a general redesign of that screen, since it starts becoming really crowded with all sorts of options.

@mxk6n mxk6n force-pushed the mxk6n:feature/macos-touchid-support branch from c77286d to ab3d9d7 Jul 30, 2018

@mxk6n

This comment has been minimized.

Copy link
Contributor Author

mxk6n commented Jul 30, 2018

Thanks for the feedback! Fixed the UI issue with checkboxes not being aligned and did another rebase.

@bhavers

This comment has been minimized.

Copy link

bhavers commented Aug 10, 2018

Great addition, thx! Happy to help testing, i have a MBP 15" 2017. Let me know when and what to do when you are ready.

@bhavers

This comment has been minimized.

Copy link

bhavers commented Aug 30, 2018

Hi, i just tried version 2.3.4 but couldn't find the touchid support. I thought there was nothing blocking the merger into a final version. Don't want to push anybody, but would be great if it could be in the next release.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Aug 31, 2018

This is destined for 2.4, it will be merged very soon

@droidmonkey droidmonkey added this to the v2.4.0 milestone Sep 5, 2018

@droidmonkey droidmonkey force-pushed the mxk6n:feature/macos-touchid-support branch from ab3d9d7 to 2e85456 Sep 18, 2018

@droidmonkey droidmonkey force-pushed the mxk6n:feature/macos-touchid-support branch from 2e85456 to a069377 Sep 18, 2018

@droidmonkey droidmonkey merged commit d9fcdd2 into keepassxreboot:develop Sep 19, 2018

2 checks passed

Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@bhavers

This comment has been minimized.

Copy link

bhavers commented Jan 19, 2019

This is destined for 2.4, it will be merged very soon

Do you have an outlook when the new 2.4 version will be released as binary?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Jan 19, 2019

We will be doing a beta launch very soon. The last major feature was just integrated (group sharing). Snapshots of the develop branch are already available at https://snapshot.keepassxc.org, however TouchID does not work unless the binary is signed.

@bhavers

This comment has been minimized.

Copy link

bhavers commented Feb 3, 2019

Hi, i tried 2.4 Beta 1 on my MBP 2017 (Mojave 10.14.3) but can't get TouchID to work, not sure if i am doing the right thing. Should i open a separate issue or can we use this thread?

Problem:
When i open KeepassXC i do see the TouchID option (see screenshot below).
I enter my password and open my database file. Than, when the session expires, i can't reopen it with TouchID. I have to provide the password again.
If i just check TouchID and select OK it is unable to open the database.

image

image

image

@tijme

This comment has been minimized.

Copy link

tijme commented Feb 3, 2019

Hi, i tried 2.4 Beta 1 on my MBP 2017 (Mojave 10.14.3) but can't get TouchID to work, not sure if i am doing the right thing. Should i open a separate issue or can we use this thread?

Problem:
When i open KeepassXC i do see the TouchID option (see screenshot below).
I enter my password and open my database file. Than, when the session expires, i can't reopen it with TouchID. I have to provide the password again.
If i just check TouchID and select OK it is unable to open the database.

image image image

If you're using the beta version (which is probably not signed), TouchID will not work? I think @droidmonkey stated that TouchID does not work unless the binary is signed.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Feb 3, 2019

I signed the beta version. Unfortunately I cannot test touch id since I only have a Mac mini. Please open a new issue, we may have broken it during one of our refactors.

@mxk6n

This comment has been minimized.

Copy link
Contributor Author

mxk6n commented Feb 3, 2019

Are the entitlements for code-signing added like described in #209?

@bhavers

This comment has been minimized.

Copy link

bhavers commented Feb 3, 2019

I opened a new bug report for this.
Let me know how i can help you test if you don't have a TouchID device.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Feb 3, 2019

@mxk6n probably not. That entitlement definition should have been included in the repository.... I will add and re-sign.

@bhavers

This comment has been minimized.

Copy link

bhavers commented Feb 20, 2019

Hi, just downloaded en tried beta 2. But still not able to log in with TouchID. Should i open a new bug report?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Feb 20, 2019

@bhavers yes please, include screenshots of the unlock dialog

@bhavers

This comment has been minimized.

Copy link

bhavers commented Feb 20, 2019

Done, see this bug report.

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
@mxk6n

This comment has been minimized.

Copy link
Contributor Author

mxk6n commented Mar 24, 2019

To further improve the utilization of TouchID on Macbook Pro and make the unlock feature more intuitive to use (see comments in #2720), I would propose implementing the following changes:

  • Make enabling TouchID a global setting (not per database)
  • Add an indication that TouchID is available to unlock (colored icon) on the unlock screen / greyed out when it is not. When it is disabled in the settings no icon is shown.
  • Unlock with TouchID is triggered when it is activated/available and ...
    • ... User clicks on the colored icon
    • ... User tries to unlock database without entering a password
    • ... Window is being brought to the foreground by the user
    • ... Tab is being changed / brought to foreground (multi database usage)
    • I would suggest making the last two scenarios an optional setting
  • TouchID is automatically deactivated on 3 consecutively failed tries (until user enters the password correctly again)
  • Raising the maximum time-out of TouchID (currently the maximum value is 999 minutes)

How do you guys feel about this?

Edit: Moved the discussion to #2865

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Mar 24, 2019

Please make a new issue with your proposed changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.