Implement support for Yubikeys and potential other tokens via wireless NFC using smartcard readers (Rebase) #6895

StarGate01 · 2021-09-05T16:04:04Z

This PR is just #6766 but rebased on develop. Unfortunately GitHub does not allow re-opening closed PRs where the target branch has been removed.

Closes #4090

droidmonkey · 2021-09-05T16:10:54Z

I would just make this required for building with yubikey support. No need for the additional compile time feature. Need to add pcsc to our docker

StarGate01 · 2021-09-05T16:15:42Z

I agree, I initially added the compile flag to make migration for maintainers easier. I'll update the PR today or in the next few days.

droidmonkey · 2021-09-05T16:21:45Z

I ordered an nfc reader to test this. Pretty excited actually.

StarGate01 · 2021-09-05T16:35:29Z

Cool! Let me know if you encounter any issues, also note that some readers can only do basic NFC and have no proper smartcard (PCSC) interface, and some don't work that well in Linux (see https://ccid.apdu.fr/ccid/section.html).

droidmonkey · 2021-09-05T16:49:35Z

I got one with reviews saying they were using it with yubikey 5, hopefully it works!

https://smile.amazon.com/dp/B01KIKBYAG/ref=cm_sw_r_cp_apa_glt_fabc_3VN0H0BBCDJKM5G5DJWX

StarGate01 · 2021-09-05T21:44:18Z

I removed the WITH_XC_YUBIKEY_PCSC flag, and also fixed a compilation error when YubiKey support was disabled.

You probably want to update the package dependencies for eg. Snap and other systems to include something like pcsclite (-dev)/opensc/pcscd (depending on the distribution) and libusb-1.0 (-dev), and remove the old ykpers.

droidmonkey · 2021-09-20T01:15:58Z

I tested this with my new NFC reader and it worked flawlessly, great work!

droidmonkey · 2021-09-20T03:06:10Z

@phoerious would appreciate a second set of eyes on this code. I did a little cleanup but the PCSC code is rather dense. It functionally works.

StarGate01 · 2021-09-20T06:08:46Z

Great to hear you got it working. If you have any questions or comments on the PCSC code feel free to ask, unfortunately the WinAPI-type model requires some workarounds.

Also, it appears as if the Windows 10 CI test step sometimes fails?

droidmonkey · 2021-09-20T10:01:44Z

That is just a random failure

StarGate01 · 2021-09-21T08:56:15Z

Before you merge this, I would like to change the detection procedure to a more robust and flexible one, that does not rely on a hard-coded ATR whitelist but instead queries the AID. I'll probably implement this in the next 1-2 Weeks.

Do you have a release window planned for 2.7?

droidmonkey · 2021-09-21T10:17:56Z

You got plenty of time, please do make it more flexible

StarGate01 · 2021-09-29T01:03:30Z

Alright, I implemented AID scanning instead of the ATR whitelist. Give it a try and let me know if there are any issues.

This should enable unknown or unreleased hardware keys to work with the code, if they implement the HMAC-SHA1 protocol and use one of the AIDs listed.

This requires a new library dependency: PCSC. The PCSC library provides methods to access smartcards. On Linux, the third-party pcsc-lite package is used. On Windows, the native Windows API (Winscard.dll) is used. On Mac OSX, the native OSX API (framework-PCSC) is used. * Split hardware key access into multiple classes to handle different methods of communicating with the keys. * Since the Yubikey can now be a wireless token as well, the verb "plug in" was replaced with a more generic "interface with". This shall indicate that the user has to present their token to the reader, or plug it in via USB. * Add PC/SC interface for YubiKey challenge-response This new interface uses the PC/SC protocol and API instead of the USB protocol via ykpers. Many YubiKeys expose their functionality as a CCID device, which can be interfaced with using PC/SC. This is especially useful for NFC-only or NFC-capable Yubikeys, when they are used together with a PC/SC compliant NFC reader device. Although many (not all) Yubikeys expose their CCID functionality over their own USB connection as well, the HMAC-SHA1 functionality is often locked in this mode, as it requires eg. a touch on the gold button. When accessing the CCID functionality wirelessly via NFC (like this code can do using a reader), then the user interaction is to present the key to the reader. This implementation has been tested on Linux using pcsc-lite, Windows using the native Winscard.dll library, and Mac OSX using the native PCSC-framework library.

Before, a whitelist of ATR codes (answer to reset, hardware-specific) was used to scan for compatible (Yubi)Keys. Now, every connected smartcard is scanned for AIDs (applet identifier), which are known to implement the HMAC-SHA1 protocol. This enables the support of currently unknown or unreleased hardware.

droidmonkey · 2021-09-29T02:28:33Z

I cleaned up the code a little and fixed use of a Qt 5.15 only function

The error code datatype differs across OSX, Linux and Windows, so we align it to int32_t, because we dont care for the actual constant values at all.

codecov-commenter · 2021-09-30T15:39:44Z

Codecov Report

Merging #6895 (589bf1b) into develop (e8a32cc) will decrease coverage by 0.83%.
The diff coverage is 13.93%.

@@             Coverage Diff             @@
##           develop    #6895      +/-   ##
===========================================
- Coverage    64.58%   63.75%   -0.83%     
===========================================
  Files          326      330       +4     
  Lines        40727    41364     +637     
===========================================
+ Hits         26302    26369      +67     
- Misses       14425    14995     +570

Impacted Files	Coverage Δ
src/cli/Utils.cpp	`75.11% <0.00%> (ø)`
src/gui/DatabaseOpenWidget.cpp	`54.65% <0.00%> (ø)`
src/gui/MainWindow.cpp	`72.04% <0.00%> (ø)`
src/keys/ChallengeResponseKey.cpp	`6.25% <0.00%> (ø)`
src/keys/drivers/YubiKeyInterfacePCSC.h	`0.00% <0.00%> (ø)`
src/keys/drivers/YubiKeyInterfacePCSC.cpp	`4.85% <4.85%> (ø)`
src/keys/drivers/YubiKeyInterfaceUSB.cpp	`22.12% <22.12%> (ø)`
src/keys/drivers/YubiKeyInterface.cpp	`36.84% <36.84%> (ø)`
src/keys/drivers/YubiKey.cpp	`45.74% <40.82%> (+19.54%)`	⬆️
src/gui/databasekey/YubiKeyEditWidget.cpp	`59.34% <0.00%> (-9.89%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a32cc...589bf1b. Read the comment docs.

Without this check, keys that have more than one slot configured show up multiple times instead of listing multiple slots.

droidmonkey · 2021-10-01T02:16:05Z

OK final cleanup round, I changed the message when the YubiKey is missing or needs to be touched to "Please present or touch your YubiKey to continue…" this wording works even when the key is a USB key and not currently plugged in. I did not like the word interface, too mechanical.

I also changed the QMultiHash to a QMultiMap so we can use uniqueKeys() function in all Qt versions and get free sorting by serial number.

droidmonkey · 2021-10-01T14:40:01Z

Excellent work on this one, and yes this does make the PKCS#11 integration easier! I plan to add another interface for PKCS#11 and further abstract the differences away. I also plan to eliminate the use of "YubiKey" in favor of "Hardware Key".

StarGate01 · 2021-10-01T16:10:27Z

Thank you for fixing the wording and translation entries, since English is not my first language. Also thanks for your support on this PR, working with KeePassXC is fun.

Eveything seems to work like intended on the current version of develop 👍. If you have any questions regarding PC/SC development in the future, feel free to contact me.

Moving to a generic "Hardware Key" interface sounds great, especially since many keys implement PC/SC (and even PKCS#11).

droidmonkey · 2021-10-01T16:13:35Z

I must say using the NFC reader is awesome, except mine makes this horrific beeping noise when establishing connection to the key.

StarGate01 · 2021-10-01T16:15:07Z

You could just desolder the buzzer :D but I agree - I even modified my laptop and integrated a reader under the palm rest.

droidmonkey · 2021-10-01T16:16:00Z

Strongly considering it

* libusb & pcsclite: needed since keepassxreboot/keepassxc#6895 * yubikey-personalization: vendored since keepassxreboot/keepassxc#6654

p0rt9 · 2021-10-09T19:09:57Z

When does this get added to the main release? Is there anything I can do to support/promote this feature?

StarGate01 · 2021-10-09T19:11:48Z

This PR has already been accepted and merged into the main codebase, and is on track to release with the next version afaik.

droidmonkey · 2021-10-09T21:19:55Z

When we release 2.7.0

StarGate01 · 2021-10-10T15:21:04Z

@p0rt9 If you want to help, you can build the development version and test the feature. Any bugs we catch before release are great.

tarioch · 2021-10-13T12:15:51Z

It's somewhere mentioned that this might also fix the issue with using Yubikey over a Remote Desktop connection.

I tried with the snappshot release from here https://snapshot.keepassxc.org/latest/ with a date of 2021-10-11 06:52 (Windows Portable Version).

And it doesn't seem to work (other yubikey things are passed through, but Keepassxc still doesn't detect any hardware).

droidmonkey · 2021-10-13T14:18:45Z

Are you passing smart cards?

tarioch · 2021-10-13T14:41:54Z

Yes, I think so. It's enabled on the rdp settings and I can also run

certutil -scinfo

and it gives


Current reader/card status:
Readers: 1
  0: Yubico Yubikey NEO OTP+U2F+CCID 0
--- Reader: Yubico Yubikey NEO OTP+U2F+CCID 0
--- Status: SCARD_STATE_PRESENT | SCARD_STATE_UNPOWERED
--- Status: The card is available for use.
---   Card: Identity Device (NIST SP 800-73 [PIV])
---    ATR:
        ......


=======================================================
Analyzing card in reader: Yubico Yubikey NEO OTP+U2F+CCID 0

--------------===========================--------------
================ Certificate 0 ================
--- Reader: Yubico Yubikey NEO OTP+U2F+CCID 0
---   Card: Identity Device (NIST SP 800-73 [PIV])
Provider = Microsoft Base Smart Card Crypto Provider
Key Container = ..... [Default Container]

and then asks for my pin and it gives me the certificate I have on the yubikey.

droidmonkey · 2021-10-13T15:50:23Z

Interesting

StarGate01 · 2021-10-13T16:59:25Z

This module does not use the CCID interface that the Yubikey provides via USB, instead it searches for a third-party reader that reads the Yubikey via NFC.

StarGate01 changed the title ~~Feature/yubikey pcsc~~ Implement support for Yubikeys and potential other tokens via wireless NFC using smartcard readers (Rebase) Sep 5, 2021

droidmonkey added feature: Hardware Keys new feature labels Sep 20, 2021

droidmonkey added this to the v2.7.0 milestone Sep 20, 2021

droidmonkey force-pushed the feature/yubikey-pcsc branch from c510bdf to 250a737 Compare September 20, 2021 03:05

droidmonkey approved these changes Sep 20, 2021

View reviewed changes

StarGate01 force-pushed the feature/yubikey-pcsc branch from fe49dd1 to a48c6fe Compare September 29, 2021 00:50

StarGate01 and others added 3 commits September 28, 2021 21:29

Cleanup code

20cc87d

droidmonkey force-pushed the feature/yubikey-pcsc branch from a48c6fe to 20cc87d Compare September 29, 2021 02:27

Use explicit downcast for PC/SC error codes

2482b48

The error code datatype differs across OSX, Linux and Windows, so we align it to int32_t, because we dont care for the actual constant values at all.

StarGate01 force-pushed the feature/yubikey-pcsc branch from cc9dcf0 to 2482b48 Compare September 30, 2021 15:25

Fix YubiKey duplicate detection regression

a760f9f

Without this check, keys that have more than one slot configured show up multiple times instead of listing multiple slots.

droidmonkey force-pushed the feature/yubikey-pcsc branch from f7fd94a to 589bf1b Compare October 1, 2021 02:17

Some more cleanup

912c51e

droidmonkey force-pushed the feature/yubikey-pcsc branch from 589bf1b to 912c51e Compare October 1, 2021 12:30

droidmonkey merged commit 6d1fc31 into keepassxreboot:develop Oct 1, 2021

yan12125 pushed a commit to archlinuxcn/repo that referenced this pull request Oct 2, 2021

keepassxc-git: update dependencies

98eef22

* libusb & pcsclite: needed since keepassxreboot/keepassxc#6895 * yubikey-personalization: vendored since keepassxreboot/keepassxc#6654

StarGate01 mentioned this pull request Aug 4, 2022

Hardware keys: Add VivoKey and DT token ATRs #8332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for Yubikeys and potential other tokens via wireless NFC using smartcard readers (Rebase) #6895

Implement support for Yubikeys and potential other tokens via wireless NFC using smartcard readers (Rebase) #6895

StarGate01 commented Sep 5, 2021 •

edited by droidmonkey

Loading

droidmonkey commented Sep 5, 2021 •

edited

Loading

StarGate01 commented Sep 5, 2021

droidmonkey commented Sep 5, 2021

StarGate01 commented Sep 5, 2021 •

edited

Loading

droidmonkey commented Sep 5, 2021 •

edited

Loading

StarGate01 commented Sep 5, 2021

droidmonkey commented Sep 20, 2021

droidmonkey commented Sep 20, 2021

StarGate01 commented Sep 20, 2021

droidmonkey commented Sep 20, 2021

StarGate01 commented Sep 21, 2021

droidmonkey commented Sep 21, 2021

StarGate01 commented Sep 29, 2021

droidmonkey commented Sep 29, 2021

codecov-commenter commented Sep 30, 2021 •

edited

Loading

droidmonkey commented Oct 1, 2021

droidmonkey commented Oct 1, 2021 •

edited

Loading

StarGate01 commented Oct 1, 2021 •

edited

Loading

droidmonkey commented Oct 1, 2021

StarGate01 commented Oct 1, 2021

droidmonkey commented Oct 1, 2021

p0rt9 commented Oct 9, 2021

StarGate01 commented Oct 9, 2021

droidmonkey commented Oct 9, 2021

StarGate01 commented Oct 10, 2021

tarioch commented Oct 13, 2021

droidmonkey commented Oct 13, 2021

tarioch commented Oct 13, 2021

droidmonkey commented Oct 13, 2021

StarGate01 commented Oct 13, 2021

Implement support for Yubikeys and potential other tokens via wireless NFC using smartcard readers (Rebase) #6895

Implement support for Yubikeys and potential other tokens via wireless NFC using smartcard readers (Rebase) #6895

Conversation

StarGate01 commented Sep 5, 2021 • edited by droidmonkey Loading

droidmonkey commented Sep 5, 2021 • edited Loading

StarGate01 commented Sep 5, 2021

droidmonkey commented Sep 5, 2021

StarGate01 commented Sep 5, 2021 • edited Loading

droidmonkey commented Sep 5, 2021 • edited Loading

StarGate01 commented Sep 5, 2021

droidmonkey commented Sep 20, 2021

droidmonkey commented Sep 20, 2021

StarGate01 commented Sep 20, 2021

droidmonkey commented Sep 20, 2021

StarGate01 commented Sep 21, 2021

droidmonkey commented Sep 21, 2021

StarGate01 commented Sep 29, 2021

droidmonkey commented Sep 29, 2021

codecov-commenter commented Sep 30, 2021 • edited Loading

Codecov Report

droidmonkey commented Oct 1, 2021

droidmonkey commented Oct 1, 2021 • edited Loading

StarGate01 commented Oct 1, 2021 • edited Loading

droidmonkey commented Oct 1, 2021

StarGate01 commented Oct 1, 2021

droidmonkey commented Oct 1, 2021

p0rt9 commented Oct 9, 2021

StarGate01 commented Oct 9, 2021

droidmonkey commented Oct 9, 2021

StarGate01 commented Oct 10, 2021

tarioch commented Oct 13, 2021

droidmonkey commented Oct 13, 2021

tarioch commented Oct 13, 2021

droidmonkey commented Oct 13, 2021

StarGate01 commented Oct 13, 2021

StarGate01 commented Sep 5, 2021 •

edited by droidmonkey

Loading

droidmonkey commented Sep 5, 2021 •

edited

Loading

StarGate01 commented Sep 5, 2021 •

edited

Loading

droidmonkey commented Sep 5, 2021 •

edited

Loading

codecov-commenter commented Sep 30, 2021 •

edited

Loading

droidmonkey commented Oct 1, 2021 •

edited

Loading

StarGate01 commented Oct 1, 2021 •

edited

Loading