Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add challenge-response support for Nitrokey 3 #9631

Merged
merged 4 commits into from
Jul 16, 2023

Conversation

droidmonkey
Copy link
Member

Add challenge-response support for Nitrokey 3.

In detail:

  • Add Get Response call when More Available / 0x61 SW1 is received
  • Increase buffer for answer to select call (required for Nitrokey 3)
  • Small refactorization for reading the SW

Example log for selecting app with More Available / Get Response used below:

00009450 APDU: 00 A4 04 00 07 A0 00 00 05 27 20 01
00001675 SW: 6A 82
00000071 APDU: 00 A4 04 00 07 A0 00 00 05 27 21 01
00057807 SW: 61 0F
00000037 APDU: 00 C0 00 00 FF
00000893 SW: 79 03 04 0B 00 71 08 3C 73 5F 60 F2 03 EB 0D 90 00

To test:

  • Yubikey behavior for that change. The Nitrokey 3's application responsible for challenge-response is QByteArrayLiteral("\xA0\x00\x00\x05\x27\x21\x01"), which is Yubikey's OATH AID. Check if that could make any conflict.
    • Yubikey behaves normally - tests are passing.

Screenshots

image

Testing strategy

  1. Test creating database (manual)
  2. Test opening database (manual)`

Automatic tests: testykchallengeresponsekey (built with ASAN)

~/w/3/k/c/tests (support-challenge-response-in-nitrokey3|✚2) $ ./testykchallengeresponsekey
********* Start testing of TestYubiKeyChallengeResponse *********
Config: Using QtTest library 5.15.9, Qt 5.15.9 (x86_64-little_endian-lp64 shared (dynamic) release build;
by GCC 13.0.1 20230401 (Red Hat 13.0.1-0)), fedora 38
PASS   : TestYubiKeyChallengeResponse::initTestCase()
PASS   : TestYubiKeyChallengeResponse::testDetectDevices()
PASS   : TestYubiKeyChallengeResponse::testKeyChallenge()
PASS   : TestYubiKeyChallengeResponse::cleanupTestCase()
Totals: 4 passed, 0 failed, 0 skipped, 0 blacklisted, 1336ms
********* Finished testing of TestYubiKeyChallengeResponse *********

This PR was tested against:

  • Nitrokey 3 (unreleased firmware, based on v1.4), and
  • YubiKey 4 (4.3.5) [OTP+FIDO+CCID] Serial: 5668784

Type of change

  • ✅ New feature (change that adds functionality)

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.07 ⚠️

Comparison is base (9214ab2) 64.82% compared to head (f23ee13) 64.75%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #9631      +/-   ##
===========================================
- Coverage    64.82%   64.75%   -0.07%     
===========================================
  Files          337      337              
  Lines        44564    44606      +42     
===========================================
- Hits         28885    28882       -3     
- Misses       15679    15724      +45     
Impacted Files Coverage Δ
src/keys/drivers/YubiKeyInterfacePCSC.cpp 4.68% <0.00%> (-0.40%) ⬇️
src/keys/drivers/YubiKeyInterfacePCSC.h 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@droidmonkey droidmonkey merged commit bb37cf3 into develop Jul 16, 2023
@droidmonkey droidmonkey deleted the support-challenge-response-in-nitrokey3 branch July 16, 2023 02:47
@rugk
Copy link

rugk commented Jul 31, 2023

Is this also compatible with SoloKeys – which AFAIK share quite much code with Nitrokey's? Or FIDO2 keys in general?

(keywords: U2F, WebAuthn although likely these protocols are not used here, it makes this PR searchable hehe)

@szszszsz
Copy link
Contributor

szszszsz commented Aug 1, 2023

Is this also compatible with SoloKeys – which AFAIK share quite much code with Nitrokey's? Or FIDO2 keys in general?
(keywords: U2F, WebAuthn although likely these protocols are not used here, it makes this PR searchable hehe)

Hey @rugk !
This PR is sadly using the proprietary Yubikey challenge-response protocol. The PKCS#11- and FIDO-based would be much better, but we wanted to add support to NK3 for this one quickly for the interim/temporary solution until they will become ready.

Regarding SoloKeys, I do not know the answer. I can tell, that we have forked their oath-authenticator app to secrets-app, and heavily extended it. Technically it should be possible for them to switch to it at some point, but I do not know their plans.

@droidmonkey droidmonkey modified the milestones: v2.8.0, v2.7.6 Aug 5, 2023
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Aug 5, 2023
droidmonkey added a commit that referenced this pull request Aug 5, 2023
Co-authored-by: Szczepan Zalega <szczepan@nitrokey.com>
libf-de pushed a commit to libf-de/keepassxc-secretservice-dbus that referenced this pull request Aug 16, 2023
Release 2.7.6

Changes
- Significant improvement to visual when drag/drop entries [keepassxreboot#9698]
- Automatically prompt for Quick Unlock when showing unlock dialog [keepassxreboot#9697]
- Improve colorful lock icon and fix file MIME icon on KDE [keepassxreboot#9632]
- Ability to search by entry UUID [keepassxreboot#9571]
- Add challenge-response support for NitroKey 3 [keepassxreboot#9631]
- Auto-Type: Disable entry level Auto-Type when disabled at group/entry [keepassxreboot#9672]
- Browser: Show warning when adding duplicate URL's to entry [keepassxreboot#9588][keepassxreboot#9635]
- Browser: Improve error message when proxy cannot be found [keepassxreboot#9385]

Fixes
- Fix crash on exit on macOS [keepassxreboot#9620]
- Fix crash on search if entry doesn't have a group [keepassxreboot#9633]
- Fix several issues with Quick Unlock [keepassxreboot#9697]
- Enable save button when not auto-saving non-data changes [keepassxreboot#9634]
- Several UI/UX fixes [keepassxreboot#9647]
- Move toolbar back to top of window when disabling movement [keepassxreboot#9699]
- Browser: Fix closing password generator dialog with X button [keepassxreboot#9636]
- Browser: Fix handling of expired credentials [keepassxreboot#9595]
- Windows: Prevent white flicker when launching application [keepassxreboot#9637]
- Linux: Fix warning message about allow screencapture [keepassxreboot#9638]
- FdoSecrets: Fix access confirmation dialog showing even when disabled [keepassxreboot#9690]

# -----BEGIN PGP SIGNATURE-----
#
# iQJIBAABCAAyFiEE6A9nU7OzJX8m9B8ILPQdKqhDj5kFAmTb/usUHGphbmVrQGtl
# ZXBhc3N4Yy5vcmcACgkQLPQdKqhDj5l9vBAAmiCQR+L3ZLVq7CfXK+yOrCr1pV1J
# H6znrRe4SC5MR/dyrx+EVbkaPI0aWtW/NWa4REB9BUxkbXKIPy/9M6smj3xkjAqX
# YuYThNneRBFns9Rb5RyAIonwEXXmYHAWG2wdRXXFOnsb/Dxy9DYZK6+Ysbj55CQJ
# RBJ1y0IKCuajLvENW9zQQ/vTX0oxCQ2F9Fz7aTqGIoxW6NMhjTso7IPvKYWPzbNj
# FBOiI4kusL32pT5u+XwSUjmBvXrIEBjETYFTVgqesItAr0dFAgEh8f0jvuy8on8K
# ukVzD02JqavkMfwtDsvUVLdVdr1PJMOu4/qDodR1xC39VOjS9LQ6dK8rb/1Q4/MR
# cAXjBhNBZ0A5yq9XtdNvl8xYqkvYa/KcFuHUFwBoinLXtKLnh4aswDqk4caNeI4O
# O40Nk5J4J6Qgs89XIsQHkXkGTaPxuISHVeFWWqcpX9kRJhtlt5eIS6nDv8nGx8iq
# q65NfCldPckgmuIxeCX2lYtxieq09jAhD1/92eXsH1aNkZce4W1UcjGE58cduODd
# oXV7VCo0JUzkMky9I9/G+hAqWwLp94D5ewYG8yX2Oz2jwcoWvZSIZ6MtR+2NiYpL
# pFSFB/yoqWQOIVc9eHqCQl7rMMK66pJWwu7boxS22/xoNTAfzMwNtp8CmbLpqIhF
# 7lPQiiC2DnqfR0E=
# =l8kk
# -----END PGP SIGNATURE-----
# gpg: directory '/home/runner/.gnupg' created
# gpg: keybox '/home/runner/.gnupg/pubring.kbx' created
# gpg: Signature made Tue Aug 15 22:40:43 2023 UTC
# gpg:                using RSA key E80F6753B3B3257F26F41F082CF41D2AA8438F99
# gpg:                issuer "janek@keepassxc.org"
# gpg: Can't check signature: No public key
# for free to join this conversation on GitHub. Already have an account? # 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.

3 participants