From 48a3fd8e3ceb2a90361a54f0d6eef015b29c53fc Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Tue, 22 Mar 2022 15:12:52 -0400 Subject: [PATCH] Fix detection of hardware keys in keepassxc-cli * Split calls to finding hardware keys into sync and async methods. This has the side effect of simplifying the code. * Check for keys before performing challenge/response if no keys have been found previously. * Correct timeout of user interaction message to interact with the hardware key. * Correct error in TestCli::testYubiKeyOption --- share/translations/keepassxc_en.ts | 8 +- src/cli/Utils.cpp | 6 +- src/gui/DatabaseOpenWidget.cpp | 2 +- src/gui/databasekey/YubiKeyEditWidget.cpp | 2 +- src/keys/drivers/YubiKey.cpp | 66 ++++----- src/keys/drivers/YubiKey.h | 5 +- src/keys/drivers/YubiKeyInterface.cpp | 5 + src/keys/drivers/YubiKeyInterface.h | 2 +- src/keys/drivers/YubiKeyInterfacePCSC.cpp | 166 ++++++++++------------ src/keys/drivers/YubiKeyInterfacePCSC.h | 2 +- src/keys/drivers/YubiKeyInterfaceUSB.cpp | 134 ++++++++--------- src/keys/drivers/YubiKeyInterfaceUSB.h | 2 +- src/keys/drivers/YubiKeyStub.cpp | 7 +- tests/TestCli.cpp | 12 +- tests/TestYkChallengeResponseKey.cpp | 4 - 15 files changed, 203 insertions(+), 220 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 85c3307ca..fca641d13 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -7233,10 +7233,6 @@ Please consider generating a new key file. Invalid YubiKey serial %1 - - Please present or touch your YubiKey to continue… - - Enter password to encrypt database (optional): @@ -7760,6 +7756,10 @@ Kernel: %3 %4 Failed to sign challenge using Windows Hello. + + Please present or touch your YubiKey to continue. + + QtIOCompressor diff --git a/src/cli/Utils.cpp b/src/cli/Utils.cpp index 265c6c26b..ec4fc81f7 100644 --- a/src/cli/Utils.cpp +++ b/src/cli/Utils.cpp @@ -167,14 +167,14 @@ namespace Utils } } - auto conn = QObject::connect(YubiKey::instance(), &YubiKey::userInteractionRequest, [&] { - err << QObject::tr("Please present or touch your YubiKey to continue…") << "\n\n" << flush; + QObject::connect(YubiKey::instance(), &YubiKey::userInteractionRequest, [&] { + err << QObject::tr("Please present or touch your YubiKey to continue.") << "\n\n" << flush; }); auto key = QSharedPointer(new ChallengeResponseKey({serial, slot})); compositeKey->addChallengeResponseKey(key); - QObject::disconnect(conn); + YubiKey::instance()->findValidKeys(); } #else Q_UNUSED(yubiKeySlot); diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index 26ac61d79..d5c9707f8 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -456,7 +456,7 @@ void DatabaseOpenWidget::pollHardwareKey() m_ui->hardwareKeyProgress->setVisible(true); m_pollingHardwareKey = true; - YubiKey::instance()->findValidKeys(); + YubiKey::instance()->findValidKeysAsync(); } void DatabaseOpenWidget::hardwareKeyResponse(bool found) diff --git a/src/gui/databasekey/YubiKeyEditWidget.cpp b/src/gui/databasekey/YubiKeyEditWidget.cpp index 569a78c9d..2e6cf4ff3 100644 --- a/src/gui/databasekey/YubiKeyEditWidget.cpp +++ b/src/gui/databasekey/YubiKeyEditWidget.cpp @@ -122,7 +122,7 @@ void YubiKeyEditWidget::pollYubikey() m_compUi->comboChallengeResponse->setEnabled(false); m_compUi->yubikeyProgress->setVisible(true); - YubiKey::instance()->findValidKeys(); + YubiKey::instance()->findValidKeysAsync(); #endif } diff --git a/src/keys/drivers/YubiKey.cpp b/src/keys/drivers/YubiKey.cpp index be90b461d..f735c5dba 100644 --- a/src/keys/drivers/YubiKey.cpp +++ b/src/keys/drivers/YubiKey.cpp @@ -20,6 +20,8 @@ #include "YubiKeyInterfacePCSC.h" #include "YubiKeyInterfaceUSB.h" +#include + YubiKey::YubiKey() : m_interfaces_detect_mutex(QMutex::Recursive) { @@ -27,45 +29,27 @@ YubiKey::YubiKey() if (YubiKeyInterfaceUSB::instance()->isInitialized()) { ++num_interfaces; + connect(YubiKeyInterfaceUSB::instance(), SIGNAL(challengeStarted()), this, SIGNAL(challengeStarted())); + connect(YubiKeyInterfaceUSB::instance(), SIGNAL(challengeCompleted()), this, SIGNAL(challengeCompleted())); } else { qDebug("YubiKey: USB interface is not initialized."); } - connect(YubiKeyInterfaceUSB::instance(), SIGNAL(challengeStarted()), this, SIGNAL(challengeStarted())); - connect(YubiKeyInterfaceUSB::instance(), SIGNAL(challengeCompleted()), this, SIGNAL(challengeCompleted())); if (YubiKeyInterfacePCSC::instance()->isInitialized()) { ++num_interfaces; + connect(YubiKeyInterfacePCSC::instance(), SIGNAL(challengeStarted()), this, SIGNAL(challengeStarted())); + connect(YubiKeyInterfacePCSC::instance(), SIGNAL(challengeCompleted()), this, SIGNAL(challengeCompleted())); } else { qDebug("YubiKey: PCSC interface is disabled or not initialized."); } - connect(YubiKeyInterfacePCSC::instance(), SIGNAL(challengeStarted()), this, SIGNAL(challengeStarted())); - connect(YubiKeyInterfacePCSC::instance(), SIGNAL(challengeCompleted()), this, SIGNAL(challengeCompleted())); - // Collapse the detectComplete signals from all interfaces into one signal - // If multiple interfaces are used, wait for them all to finish - auto detect_handler = [this, num_interfaces](bool found) { - if (!m_interfaces_detect_mutex.tryLock(1000)) { - return; - } - m_interfaces_detect_found |= found; - m_interfaces_detect_completed++; - if (m_interfaces_detect_completed != -1 && m_interfaces_detect_completed == num_interfaces) { - m_interfaces_detect_completed = -1; - emit detectComplete(m_interfaces_detect_found); - } - m_interfaces_detect_mutex.unlock(); - }; - connect(YubiKeyInterfaceUSB::instance(), &YubiKeyInterfaceUSB::detectComplete, this, detect_handler); - connect(YubiKeyInterfacePCSC::instance(), &YubiKeyInterfacePCSC::detectComplete, this, detect_handler); + m_initialized = num_interfaces > 0; - if (num_interfaces != 0) { - m_initialized = true; - // clang-format off - connect(&m_interactionTimer, SIGNAL(timeout()), this, SIGNAL(userInteractionRequest())); - connect(this, &YubiKey::challengeStarted, this, [this] { m_interactionTimer.start(); }); - connect(this, &YubiKey::challengeCompleted, this, [this] { m_interactionTimer.stop(); }); - // clang-format on - } + m_interactionTimer.setSingleShot(true); + m_interactionTimer.setInterval(200); + connect(&m_interactionTimer, SIGNAL(timeout()), this, SIGNAL(userInteractionRequest())); + connect(this, &YubiKey::challengeStarted, this, [this] { m_interactionTimer.start(); }); + connect(this, &YubiKey::challengeCompleted, this, [this] { m_interactionTimer.stop(); }); } YubiKey* YubiKey::m_instance(nullptr); @@ -84,12 +68,23 @@ bool YubiKey::isInitialized() return m_initialized; } -void YubiKey::findValidKeys() +bool YubiKey::findValidKeys() { - m_interfaces_detect_completed = 0; - m_interfaces_detect_found = false; - YubiKeyInterfaceUSB::instance()->findValidKeys(); - YubiKeyInterfacePCSC::instance()->findValidKeys(); + bool found = false; + if (m_interfaces_detect_mutex.tryLock(1000)) { + found |= YubiKeyInterfaceUSB::instance()->findValidKeys(); + found |= YubiKeyInterfacePCSC::instance()->findValidKeys(); + m_interfaces_detect_mutex.unlock(); + } + return found; +} + +void YubiKey::findValidKeysAsync() +{ + QtConcurrent::run([this] { + bool found = findValidKeys(); + emit detectComplete(found); + }); } QList YubiKey::foundKeys() @@ -207,6 +202,11 @@ YubiKey::challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_ { m_error.clear(); + // Make sure we tried to find available keys + if (foundKeys().isEmpty()) { + findValidKeys(); + } + if (YubiKeyInterfaceUSB::instance()->hasFoundKey(slot)) { return YubiKeyInterfaceUSB::instance()->challenge(slot, challenge, response); } diff --git a/src/keys/drivers/YubiKey.h b/src/keys/drivers/YubiKey.h index 5d4ce518e..312dea897 100644 --- a/src/keys/drivers/YubiKey.h +++ b/src/keys/drivers/YubiKey.h @@ -46,7 +46,8 @@ public: static YubiKey* instance(); bool isInitialized(); - void findValidKeys(); + bool findValidKeys(); + void findValidKeysAsync(); QList foundKeys(); QString getDisplayName(YubiKeySlot slot); @@ -84,8 +85,6 @@ private: QTimer m_interactionTimer; bool m_initialized = false; QString m_error; - int m_interfaces_detect_completed = -1; - bool m_interfaces_detect_found = false; QMutex m_interfaces_detect_mutex; Q_DISABLE_COPY(YubiKey) diff --git a/src/keys/drivers/YubiKeyInterface.cpp b/src/keys/drivers/YubiKeyInterface.cpp index 1164c1264..fe7f984b7 100644 --- a/src/keys/drivers/YubiKeyInterface.cpp +++ b/src/keys/drivers/YubiKeyInterface.cpp @@ -37,6 +37,11 @@ QMultiMap> YubiKeyInterface::foundKeys() bool YubiKeyInterface::hasFoundKey(YubiKeySlot slot) { + // A serial number of 0 implies use the first key + if (slot.first == 0 && !m_foundKeys.isEmpty()) { + return true; + } + for (const auto& key : m_foundKeys.values(slot.first)) { if (slot.second == key.first) { return true; diff --git a/src/keys/drivers/YubiKeyInterface.h b/src/keys/drivers/YubiKeyInterface.h index e87c3e9f1..6a7294616 100644 --- a/src/keys/drivers/YubiKeyInterface.h +++ b/src/keys/drivers/YubiKeyInterface.h @@ -36,7 +36,7 @@ public: bool hasFoundKey(YubiKeySlot slot); QString getDisplayName(YubiKeySlot slot); - virtual void findValidKeys() = 0; + virtual bool findValidKeys() = 0; virtual YubiKey::ChallengeResult challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_vector& response) = 0; virtual bool testChallenge(YubiKeySlot slot, bool* wouldBlock) = 0; diff --git a/src/keys/drivers/YubiKeyInterfacePCSC.cpp b/src/keys/drivers/YubiKeyInterfacePCSC.cpp index cb3c4f3ba..e871d63d4 100644 --- a/src/keys/drivers/YubiKeyInterfacePCSC.cpp +++ b/src/keys/drivers/YubiKeyInterfacePCSC.cpp @@ -17,10 +17,9 @@ #include "YubiKeyInterfacePCSC.h" +#include "core/Tools.h" #include "crypto/Random.h" -#include - // MSYS2 does not define these macros // So set them to the value used by pcsc-lite #ifndef MAX_ATR_SIZE @@ -530,109 +529,98 @@ YubiKeyInterfacePCSC* YubiKeyInterfacePCSC::instance() return m_instance; } -void YubiKeyInterfacePCSC::findValidKeys() +bool YubiKeyInterfacePCSC::findValidKeys() { m_error.clear(); if (!isInitialized()) { - return; + return false; } + // Remove all known keys + m_foundKeys.clear(); - QtConcurrent::run([this] { - // This mutex protects the smartcard against concurrent transmissions - if (!m_mutex.tryLock(1000)) { - emit detectComplete(false); - return; + // Connect to each reader and look for cards + auto readers_list = getReaders(m_sc_context); + foreach (const QString& reader_name, readers_list) { + + /* Some Yubikeys present their PCSC interface via USB as well + Although this would not be a problem in itself, + we filter these connections because in USB mode, + the PCSC challenge-response interface is usually locked + Instead, the other USB (HID) interface should pick up and + interface the key. + For more info see the comment block further below. */ + if (reader_name.contains("yubikey", Qt::CaseInsensitive)) { + continue; } - // Remove all known keys - m_foundKeys.clear(); + SCARDHANDLE hCard; + SCUINT dwActiveProtocol = SCARD_PROTOCOL_UNDEFINED; + auto rv = SCardConnect(m_sc_context, + reader_name.toStdString().c_str(), + SCARD_SHARE_SHARED, + SCARD_PROTOCOL_T0 | SCARD_PROTOCOL_T1, + &hCard, + &dwActiveProtocol); - // Connect to each reader and look for cards - auto readers_list = getReaders(m_sc_context); - foreach (const QString& reader_name, readers_list) { + if (rv == SCARD_S_SUCCESS) { + // Read the potocol and the ATR record + char pbReader[MAX_READERNAME] = {0}; + SCUINT dwReaderLen = sizeof(pbReader); + SCUINT dwState = 0; + SCUINT dwProt = SCARD_PROTOCOL_UNDEFINED; + uint8_t pbAtr[MAX_ATR_SIZE] = {0}; + SCUINT dwAtrLen = sizeof(pbAtr); - /* Some Yubikeys present their PCSC interface via USB as well - Although this would not be a problem in itself, - we filter these connections because in USB mode, - the PCSC challenge-response interface is usually locked - Instead, the other USB (HID) interface should pick up and - interface the key. - For more info see the comment block further below. */ - if (reader_name.contains("yubikey", Qt::CaseInsensitive)) { - continue; - } + rv = SCardStatus(hCard, pbReader, &dwReaderLen, &dwState, &dwProt, pbAtr, &dwAtrLen); + if (rv == SCARD_S_SUCCESS && (dwProt == SCARD_PROTOCOL_T0 || dwProt == SCARD_PROTOCOL_T1)) { + // Find which AID to use + SCardAID satr; + if (findAID(hCard, m_aid_codes, satr)) { + // Build the UI name using the display name found in the ATR map + QByteArray atr(reinterpret_cast(pbAtr), dwAtrLen); + QString name("Unknown Key"); + if (m_atr_names.contains(atr)) { + name = m_atr_names.value(atr); + } + // Add the firmware version and the serial number + uint8_t version[3] = {0}; + getStatus(satr, version); + name += + QString(" v%1.%2.%3") + .arg(QString::number(version[0]), QString::number(version[1]), QString::number(version[2])); - SCARDHANDLE hCard; - SCUINT dwActiveProtocol = SCARD_PROTOCOL_UNDEFINED; - auto rv = SCardConnect(m_sc_context, - reader_name.toStdString().c_str(), - SCARD_SHARE_SHARED, - SCARD_PROTOCOL_T0 | SCARD_PROTOCOL_T1, - &hCard, - &dwActiveProtocol); + unsigned int serial = 0; + getSerial(satr, serial); - if (rv == SCARD_S_SUCCESS) { - // Read the potocol and the ATR record - char pbReader[MAX_READERNAME] = {0}; - SCUINT dwReaderLen = sizeof(pbReader); - SCUINT dwState = 0; - SCUINT dwProt = SCARD_PROTOCOL_UNDEFINED; - uint8_t pbAtr[MAX_ATR_SIZE] = {0}; - SCUINT dwAtrLen = sizeof(pbAtr); - - rv = SCardStatus(hCard, pbReader, &dwReaderLen, &dwState, &dwProt, pbAtr, &dwAtrLen); - if (rv == SCARD_S_SUCCESS && (dwProt == SCARD_PROTOCOL_T0 || dwProt == SCARD_PROTOCOL_T1)) { - // Find which AID to use - SCardAID satr; - if (findAID(hCard, m_aid_codes, satr)) { - // Build the UI name using the display name found in the ATR map - QByteArray atr(reinterpret_cast(pbAtr), dwAtrLen); - QString name("Unknown Key"); - if (m_atr_names.contains(atr)) { - name = m_atr_names.value(atr); - } - // Add the firmware version and the serial number - uint8_t version[3] = {0}; - getStatus(satr, version); - name += QString(" v%1.%2.%3") - .arg(QString::number(version[0]), - QString::number(version[1]), - QString::number(version[2])); - - unsigned int serial = 0; - getSerial(satr, serial); - - /* This variable indicates that the key is locked / timed out. - When using the key via NFC, the user has to re-present the key to clear the timeout. - Also, the key can be programmatically reset (see below). - When using the key via USB (where the Yubikey presents as a PCSC reader in itself), - the non-HMAC-SHA1 slots (eg. OTP) are incorrectly recognized as locked HMAC-SHA1 slots. - Due to this conundrum, we exclude "locked" keys from the key enumeration, - but only if the reader is the "virtual yubikey reader device". - This also has the nice side effect of de-duplicating interfaces when a key - Is connected via USB and also accessible via PCSC */ - bool wouldBlock = false; - /* When the key is used via NFC, the lock state / time-out is cleared when - the smartcard connection is re-established / the applet is selected - so the next call to performTestChallenge actually clears the lock. - Due to this the key is unlocked, and we display it as such. - When the key times out in the time between the key listing and - the database unlock /save, an interaction request will be displayed. */ - for (int slot = 1; slot <= 2; ++slot) { - if (performTestChallenge(&satr, slot, &wouldBlock)) { - auto display = tr("(PCSC) %1 [%2] Challenge-Response - Slot %3") - .arg(name, QString::number(serial), QString::number(slot)); - m_foundKeys.insert(serial, {slot, display}); - } + /* This variable indicates that the key is locked / timed out. + When using the key via NFC, the user has to re-present the key to clear the timeout. + Also, the key can be programmatically reset (see below). + When using the key via USB (where the Yubikey presents as a PCSC reader in itself), + the non-HMAC-SHA1 slots (eg. OTP) are incorrectly recognized as locked HMAC-SHA1 slots. + Due to this conundrum, we exclude "locked" keys from the key enumeration, + but only if the reader is the "virtual yubikey reader device". + This also has the nice side effect of de-duplicating interfaces when a key + Is connected via USB and also accessible via PCSC */ + bool wouldBlock = false; + /* When the key is used via NFC, the lock state / time-out is cleared when + the smartcard connection is re-established / the applet is selected + so the next call to performTestChallenge actually clears the lock. + Due to this the key is unlocked, and we display it as such. + When the key times out in the time between the key listing and + the database unlock /save, an interaction request will be displayed. */ + for (int slot = 1; slot <= 2; ++slot) { + if (performTestChallenge(&satr, slot, &wouldBlock)) { + auto display = tr("(PCSC) %1 [%2] Challenge-Response - Slot %3") + .arg(name, QString::number(serial), QString::number(slot)); + m_foundKeys.insert(serial, {slot, display}); } } } } } + } - m_mutex.unlock(); - emit detectComplete(!m_foundKeys.isEmpty()); - }); + return !m_foundKeys.isEmpty(); } bool YubiKeyInterfacePCSC::testChallenge(YubiKeySlot slot, bool* wouldBlock) @@ -704,7 +692,7 @@ YubiKeyInterfacePCSC::challenge(YubiKeySlot slot, const QByteArray& challenge, B } if (--tries > 0) { - QThread::msleep(250); + Tools::sleep(250); } } diff --git a/src/keys/drivers/YubiKeyInterfacePCSC.h b/src/keys/drivers/YubiKeyInterfacePCSC.h index 56f7c3e5e..17e282111 100644 --- a/src/keys/drivers/YubiKeyInterfacePCSC.h +++ b/src/keys/drivers/YubiKeyInterfacePCSC.h @@ -50,7 +50,7 @@ class YubiKeyInterfacePCSC : public YubiKeyInterface public: static YubiKeyInterfacePCSC* instance(); - void findValidKeys() override; + bool findValidKeys() override; YubiKey::ChallengeResult challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_vector& response) override; diff --git a/src/keys/drivers/YubiKeyInterfaceUSB.cpp b/src/keys/drivers/YubiKeyInterfaceUSB.cpp index 264caf2bc..ffbceeebb 100644 --- a/src/keys/drivers/YubiKeyInterfaceUSB.cpp +++ b/src/keys/drivers/YubiKeyInterfaceUSB.cpp @@ -24,8 +24,6 @@ #include "thirdparty/ykcore/ykdef.h" #include "thirdparty/ykcore/ykstatus.h" -#include - namespace { constexpr int MAX_KEYS = 4; @@ -109,88 +107,80 @@ YubiKeyInterfaceUSB* YubiKeyInterfaceUSB::instance() return m_instance; } -void YubiKeyInterfaceUSB::findValidKeys() +bool YubiKeyInterfaceUSB::findValidKeys() { m_error.clear(); if (!isInitialized()) { - return; + return false; } - QtConcurrent::run([this] { - if (!m_mutex.tryLock(1000)) { - emit detectComplete(false); - return; - } + // Remove all known keys + m_foundKeys.clear(); - // Remove all known keys - m_foundKeys.clear(); + // Try to detect up to 4 connected hardware keys + for (int i = 0; i < MAX_KEYS; ++i) { + auto yk_key = openKey(i); + if (yk_key) { + auto serial = getSerial(yk_key); + if (serial == 0) { + closeKey(yk_key); + continue; + } - // Try to detect up to 4 connected hardware keys - for (int i = 0; i < MAX_KEYS; ++i) { - auto yk_key = openKey(i); - if (yk_key) { - auto serial = getSerial(yk_key); - if (serial == 0) { - closeKey(yk_key); + auto st = ykds_alloc(); + yk_get_status(yk_key, st); + int vid, pid; + yk_get_key_vid_pid(yk_key, &vid, &pid); + + QString name = m_pid_names.value(pid, tr("Unknown")); + if (vid == 0x1d50) { + name = QStringLiteral("OnlyKey"); + } + name += QString(" v%1.%2.%3") + .arg(QString::number(ykds_version_major(st)), + QString::number(ykds_version_minor(st)), + QString::number(ykds_version_build(st))); + + bool wouldBlock; + for (int slot = 1; slot <= 2; ++slot) { + auto config = (slot == 1 ? CONFIG1_VALID : CONFIG2_VALID); + if (!(ykds_touch_level(st) & config)) { + // Slot is not configured continue; } - - auto st = ykds_alloc(); - yk_get_status(yk_key, st); - int vid, pid; - yk_get_key_vid_pid(yk_key, &vid, &pid); - - QString name = m_pid_names.value(pid, tr("Unknown")); - if (vid == 0x1d50) { - name = QStringLiteral("OnlyKey"); + // Don't actually challenge a YubiKey Neo or below, they always require button press + // if it is enabled for the slot resulting in failed detection + if (pid <= NEO_OTP_U2F_CCID_PID) { + auto display = tr("(USB) %1 [%2] Configured Slot - %3") + .arg(name, QString::number(serial), QString::number(slot)); + m_foundKeys.insert(serial, {slot, display}); + } else if (performTestChallenge(yk_key, slot, &wouldBlock)) { + auto display = + tr("(USB) %1 [%2] Challenge-Response - Slot %3 - %4") + .arg(name, + QString::number(serial), + QString::number(slot), + wouldBlock ? tr("Press", "USB Challenge-Response Key interaction request") + : tr("Passive", "USB Challenge-Response Key no interaction required")); + m_foundKeys.insert(serial, {slot, display}); } - name += QString(" v%1.%2.%3") - .arg(QString::number(ykds_version_major(st)), - QString::number(ykds_version_minor(st)), - QString::number(ykds_version_build(st))); - - bool wouldBlock; - for (int slot = 1; slot <= 2; ++slot) { - auto config = (slot == 1 ? CONFIG1_VALID : CONFIG2_VALID); - if (!(ykds_touch_level(st) & config)) { - // Slot is not configured - continue; - } - // Don't actually challenge a YubiKey Neo or below, they always require button press - // if it is enabled for the slot resulting in failed detection - if (pid <= NEO_OTP_U2F_CCID_PID) { - auto display = tr("(USB) %1 [%2] Configured Slot - %3") - .arg(name, QString::number(serial), QString::number(slot)); - m_foundKeys.insert(serial, {slot, display}); - } else if (performTestChallenge(yk_key, slot, &wouldBlock)) { - auto display = - tr("(USB) %1 [%2] Challenge-Response - Slot %3 - %4") - .arg(name, - QString::number(serial), - QString::number(slot), - wouldBlock ? tr("Press", "USB Challenge-Response Key interaction request") - : tr("Passive", "USB Challenge-Response Key no interaction required")); - m_foundKeys.insert(serial, {slot, display}); - } - } - - ykds_free(st); - closeKey(yk_key); - - Tools::wait(100); - } else if (yk_errno == YK_ENOKEY) { - // No more keys are connected - break; - } else if (yk_errno == YK_EUSBERR) { - qWarning("Hardware key USB error: %s", yk_usb_strerror()); - } else { - qWarning("Hardware key error: %s", yk_strerror(yk_errno)); } - } - m_mutex.unlock(); - emit detectComplete(!m_foundKeys.isEmpty()); - }); + ykds_free(st); + closeKey(yk_key); + + Tools::wait(100); + } else if (yk_errno == YK_ENOKEY) { + // No more keys are connected + break; + } else if (yk_errno == YK_EUSBERR) { + qWarning("Hardware key USB error: %s", yk_usb_strerror()); + } else { + qWarning("Hardware key error: %s", yk_strerror(yk_errno)); + } + } + + return !m_foundKeys.isEmpty(); } /** diff --git a/src/keys/drivers/YubiKeyInterfaceUSB.h b/src/keys/drivers/YubiKeyInterfaceUSB.h index 6d0381c03..885188615 100644 --- a/src/keys/drivers/YubiKeyInterfaceUSB.h +++ b/src/keys/drivers/YubiKeyInterfaceUSB.h @@ -33,7 +33,7 @@ class YubiKeyInterfaceUSB : public YubiKeyInterface public: static YubiKeyInterfaceUSB* instance(); - void findValidKeys() override; + bool findValidKeys() override; YubiKey::ChallengeResult challenge(YubiKeySlot slot, const QByteArray& challenge, Botan::secure_vector& response) override; diff --git a/src/keys/drivers/YubiKeyStub.cpp b/src/keys/drivers/YubiKeyStub.cpp index 15b5ed02e..5609c4b4a 100644 --- a/src/keys/drivers/YubiKeyStub.cpp +++ b/src/keys/drivers/YubiKeyStub.cpp @@ -38,7 +38,12 @@ bool YubiKey::isInitialized() return false; } -void YubiKey::findValidKeys() +bool YubiKey::findValidKeys() +{ + return false; +} + +void YubiKey::findValidKeysAsync() { } diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp index 003d8fdbf..b79b5357c 100644 --- a/tests/TestCli.cpp +++ b/tests/TestCli.cpp @@ -2075,10 +2075,6 @@ void TestCli::testYubiKeyOption() YubiKey::instance()->findValidKeys(); - // Wait for the hardware to respond - QSignalSpy detected(YubiKey::instance(), SIGNAL(detectComplete(bool))); - QTRY_VERIFY_WITH_TIMEOUT(detected.count() > 0, 2000); - auto keys = YubiKey::instance()->foundKeys(); if (keys.isEmpty()) { QSKIP("No YubiKey devices were detected."); @@ -2094,7 +2090,7 @@ void TestCli::testYubiKeyOption() for (auto key : keys) { if (YubiKey::instance()->testChallenge(key, &wouldBlock) && !wouldBlock) { YubiKey::instance()->challenge(key, challenge, response); - if (std::memcmp(response.data(), expected.data(), expected.size())) { + if (std::memcmp(response.data(), expected.data(), expected.size()) == 0) { pKey = key; break; } @@ -2110,7 +2106,11 @@ void TestCli::testYubiKeyOption() Add addCmd; setInput("a"); - execCmd(listCmd, {"ls", "-y", "2", m_yubiKeyProtectedDbFile->fileName()}); + execCmd(listCmd, + {"ls", + "-y", + QString("%1:%2").arg(QString::number(pKey.second), QString::number(pKey.first)), + m_yubiKeyProtectedDbFile->fileName()}); m_stderr->readLine(); // skip password prompt QCOMPARE(m_stderr->readAll(), QByteArray()); QCOMPARE(m_stdout->readAll(), diff --git a/tests/TestYkChallengeResponseKey.cpp b/tests/TestYkChallengeResponseKey.cpp index aab5e6fe5..341c90715 100644 --- a/tests/TestYkChallengeResponseKey.cpp +++ b/tests/TestYkChallengeResponseKey.cpp @@ -43,10 +43,6 @@ void TestYubiKeyChallengeResponse::testDetectDevices() { YubiKey::instance()->findValidKeys(); - // Wait for the hardware to respond - QSignalSpy detected(YubiKey::instance(), SIGNAL(detectComplete(bool))); - QTRY_VERIFY_WITH_TIMEOUT(detected.count() > 0, 2000); - // Look at the information retrieved from the key(s) for (auto key : YubiKey::instance()->foundKeys()) { auto displayName = YubiKey::instance()->getDisplayName(key);