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

USB Hotplug Detection and TouchID lid close fix #10092

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

phoerious
Copy link
Member

@phoerious phoerious commented Dec 10, 2023

KeePassXC now automatically detects hotplugged USB devices, so users don't have to manually refresh the YubiKey list anymore.

Also fixes TouchID not being available after lid close (fixes #8945 and #10315)

Type of change

  • ✅ New feature (change that adds functionality)
  • ✅ Bug fix

@phoerious phoerious added this to the v2.8.0 milestone Dec 10, 2023
@phoerious phoerious requested a review from a team December 10, 2023 18:52
@droidmonkey
Copy link
Member

This is great, we can also implement an option to auto lock the database when the yubikey is unplugged. The downside to that is if you have pending changes to save, you'll have to plug your key back in.

@phoerious phoerious force-pushed the feature/usb-hotplug branch 10 times, most recently from 37d333d to 07a35e1 Compare December 13, 2023 00:38
@phoerious phoerious marked this pull request as ready for review December 13, 2023 00:39
@phoerious
Copy link
Member Author

Windows implementation added. This should be ready for review.

@phoerious phoerious force-pushed the feature/usb-hotplug branch 8 times, most recently from de2c066 to a5d50f4 Compare December 13, 2023 09:58
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Attention: Patch coverage is 53.75375% with 154 lines in your changes are missing coverage. Please review.

Project coverage is 63.94%. Comparing base (79ca006) to head (42a9c3a).

Files Patch % Lines
src/keys/drivers/YubiKeyInterfacePCSC.cpp 1.67% 59 Missing ⚠️
src/gui/DatabaseOpenWidget.cpp 61.79% 47 Missing ⚠️
src/keys/drivers/YubiKey.cpp 44.00% 14 Missing ⚠️
src/gui/osutils/nixutils/DeviceListenerLibUsb.cpp 79.69% 13 Missing ⚠️
src/keys/drivers/YubiKeyInterfaceUSB.cpp 16.67% 10 Missing ⚠️
src/gui/osutils/DeviceListener.cpp 70.00% 6 Missing ⚠️
src/gui/databasekey/YubiKeyEditWidget.cpp 83.33% 4 Missing ⚠️
src/core/Translator.cpp 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #10092      +/-   ##
===========================================
+ Coverage    63.80%   63.94%   +0.14%     
===========================================
  Files          355      358       +3     
  Lines        43298    43375      +77     
===========================================
+ Hits         27625    27736     +111     
+ Misses       15673    15639      -34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phoerious
Copy link
Member Author

phoerious commented Dec 13, 2023

I took the chance to also improve the unlock widget.

All clutter except the password field is now hidden away by default. The hardware key selection has been reduced to a single checkbox that is only shown when a key is inserted and the key file selection is just a small link:

image

I show a hardware key selection combobox only if more than one key is inserted (when does that ever happen?):
image

Clicking the key file link opens the key file selection and then adds a new UI element for it:
image

@phoerious phoerious force-pushed the feature/usb-hotplug branch 8 times, most recently from df8a90d to 17b00b7 Compare March 8, 2024 02:31
@droidmonkey droidmonkey force-pushed the feature/usb-hotplug branch from 17b00b7 to 42a9c3a Compare March 8, 2024 13:44
@droidmonkey droidmonkey merged commit 0acb15d into develop Mar 8, 2024
10 of 11 checks passed
@droidmonkey droidmonkey deleted the feature/usb-hotplug branch March 8, 2024 15:55
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Mar 9, 2024
@liayn
Copy link

liayn commented Mar 11, 2024

I show a hardware key selection combobox only if more than one key is inserted (when does that ever happen?):

That happens all the time if you have any account that allows to register only 1 OTP key (Paypal... to name a few), but one owns multiple keys (which you should).

libf-de pushed a commit to libf-de/keepassxc-secretservice-dbus that referenced this pull request Mar 11, 2024
Release 2.7.7

- Support USB Hotplug for Hardware Key interface [keepassxreboot#10092]
- Support 1PUX and Bitwarden import [keepassxreboot#9815]
- Browser: Add support for PassKeys [keepassxreboot#8825, keepassxreboot#9987, keepassxreboot#10318]
- Build System: Move to vcpkg manifest mode [keepassxreboot#10088]

- Fix multiple TOTP issues [keepassxreboot#9874]
- Fix focus loss on save when the editor is not visible anymore [keepassxreboot#10075]
- Fix visual when removing entry from history [keepassxreboot#9947]
- Fix first entry is not selected when a search is performed [keepassxreboot#9868]
- Prevent scrollbars on entry drag/drop [keepassxreboot#9747]
- Prevent duplicate characters in "Also choose from" field of password generator  [keepassxreboot#9803]
- Security: Prevent byte-by-byte and attachment inference side channel attacks [keepassxreboot#10266]
- Browser: Fix raising Update Entry messagebox [keepassxreboot#9853]
- Browser: Fix bugs when returning credentials [keepassxreboot#9136]
- Browser: Fix crash on database open from browser [keepassxreboot#9939]
- Browser: Fix support for referenced URL fields [keepassxreboot#8788]
- MacOS: Fix crash when changing highlight/accent color [keepassxreboot#10348]
- MacOS: Fix TouchID appearing even though lid is closed [keepassxreboot#10092]
- Windows: Fix terminating KeePassXC processes with MSI installer [keepassxreboot#9822]
- FdoSecrets: Fix database merge crash when enabled [keepassxreboot#10136]

# -----BEGIN PGP SIGNATURE-----
#
# iQEzBAABCAAdFiEENIkEDB8MPuq41ValRA/GXy4MbgEFAmXs7VsACgkQRA/GXy4M
# bgHLpwf/brnyPPs3gJxZmD2pn8542D4CCsDh0fTceurOtqCe3J4Y+Fftc5euuoQu
# 6rP4vJdd586l7JX5FnYIPXvGiU9op3MudJh+y+RN/PWwKcXNIXfUItMhpZEka49n
# xnw+Wvbilg1QIHSSmZdIjBpohnEkA67qhWauc3bCacrRyEvIOzVMTxnqDTe4GUDy
# CyauaRMMKezRTpLxSsk63TDAZZgDwK4ci5lC6ysHekc1Za6IbI3fMFjz1BGj+kPU
# tMHMfDCWqK/5JZ27ZWcxy7m8tJY9m3rb+MoCyFRQz9ixaEe29yf5NqYdm9sn1Dlh
# O7aFi7/EJtsBlXdguw5BcTPbsL7XEQ==
# =Cots
# -----END PGP SIGNATURE-----
# gpg: directory '/home/runner/.gnupg' created
# gpg: keybox '/home/runner/.gnupg/pubring.kbx' created
# gpg: Signature made Sat Mar  9 23:14:35 2024 UTC
# gpg:                using RSA key 3489040C1F0C3EEAB8D556A5440FC65F2E0C6E01
# gpg: Can't check signature: No public key
@phoerious
Copy link
Member Author

You should have multiple keys for backup purposes, but there is no point in having them plugged in at the same time, unless perhaps you have one for work and another personal key. If you are just too paranoid to use the same key for different services, then you also shouldn't have them with you at the same time.

But anyway. You can have two plugged in if you need to.

@synthgab
Copy link

synthgab commented Mar 14, 2024

when does that ever happen?

When you have a Yubikey with two slots (e.g. a Neo) ...

By the way, I struggled a little bit with this new feature and it seems that the key is not detected correctly right upon insertion: my 2015 Yubikey Neo still works like a charm, but its getting old and now needs some time to wake up, so I'm not sure this is worth filing a bug report.

The trick is to check the serial number: a 'refresh' is needed when it's only 2 or 3 digits instead of the 7 digits I expect, not a big deal, just paying attention when unlocking the database...

@phoerious
Copy link
Member Author

Which YubiKey do you have?

@synthgab
Copy link

synthgab commented Mar 14, 2024

Hello, I have a Yubikey NEO, I bought it in something like 2015. Firmware version is 3.4.9, if that may help, but as I said, I'm used to have to wait a few seconds for it to initialize when plugged-in (TOTP over NFC on Android also takes a few seconds to load the codes).

I have kept slot 1 for Yubico OTP (with a custom keypair used for VPN connection), and slot 2 with challenge-response for KeepassXC, on top of the TOTP feature for several other accounts.

[Edit] Syntax fixing.

@droidmonkey
Copy link
Member

The NEO keys were garbage, they have to be handled specially because they don't comply with yubikeys spec.

@phoerious
Copy link
Member Author

I have a spare NEO as well and tested it with the change. For me, it worked totally fine. What OS are you on?

@synthgab
Copy link

Hello,

Thank you for following up on my comment, I'm on Windows and I reproduce this delay on both Windows 11 (23H2) and Windows 10 (21H2).

However, I agree with Neo 'randomness': each manufacturing year behaves differently, and this is why I don't believe it's worth spending time on this. I managed to adapt to my NEO specialties , my friends who are using some did the same, and when the replacement time will come, I'll perform a more thorough review...

Thanks !

@cdo256
Copy link

cdo256 commented Apr 19, 2024

This is a great feature for convenience, however it's not automatically detecting new keys. On Linux (Guix), on the unlock screen, I'm only seeing new hardware keys I insert after I exit to the main page and go back to the unlock screen. On the terminal I'm getting:

Hardware key USB error: Access denied (insufficient permissions)

I performed an strace, but I couldn't determine which access it was. Nothing related to udev, dev, sys, pam, or usb stood out as being the culpret. As expected running keepassxc as root, resolves the issue. I've attached both straces for running under sudo -E and straight as a user.

The operating system is Guix Linux at a8353e9d6b34fd8d42d2e8f14ce844849fe9c293.

Let me know if you need any more information.

sudo-strace.trace, user-strace.trace.

@droidmonkey
Copy link
Member

droidmonkey commented Apr 19, 2024

You need the yubikey udev rules installed. Never heard of guix though.

https://github.com/Yubico/yubikey-personalization/blob/master/70-yubikey.rules

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Hardware Keys pr: backported Pull request backported to previous release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[macOS] Prompt for a master password, despite having quick unlock via Touch ID after opening the lid
5 participants