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

@@ -7233,10 +7233,6 @@ Please consider generating a new key file.</source>
         <source>Invalid YubiKey serial %1</source>
         <translation type="unfinished"></translation>
     </message>
-    <message>
-        <source>Please present or touch your YubiKey to continue…</source>
-        <translation type="unfinished"></translation>
-    </message>
     <message>
         <source>Enter password to encrypt database (optional): </source>
         <translation type="unfinished"></translation>
@@ -7760,6 +7756,10 @@ Kernel: %3 %4</source>
         <source>Failed to sign challenge using Windows Hello.</source>
         <translation type="unfinished"></translation>
     </message>
+    <message>
+        <source>Please present or touch your YubiKey to continue.</source>
+        <translation type="unfinished"></translation>
+    </message>
 </context>
 <context>
     <name>QtIOCompressor</name>

src/cli/Utils.cpp

@@ -167,14 +167,14 @@ namespace Utils
                 }
             }
 
-            auto conn = QObject::connect(YubiKey::instance(), &YubiKey::userInteractionRequest, [&] {
+            QObject::connect(YubiKey::instance(), &YubiKey::userInteractionRequest, [&] {
-                err << QObject::tr("Please present or touch your YubiKey to continue…") << "\n\n" << flush;
+                err << QObject::tr("Please present or touch your YubiKey to continue.") << "\n\n" << flush;
             });
 
             auto key = QSharedPointer<ChallengeResponseKey>(new ChallengeResponseKey({serial, slot}));
             compositeKey->addChallengeResponseKey(key);
 
-            QObject::disconnect(conn);
+            YubiKey::instance()->findValidKeys();
         }
 #else
         Q_UNUSED(yubiKeySlot);

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)

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
 }
 

src/keys/drivers/YubiKey.cpp

@@ -20,6 +20,8 @@
 #include "YubiKeyInterfacePCSC.h"
 #include "YubiKeyInterfaceUSB.h"
 
+#include <QtConcurrent>
+
 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
+    m_initialized = num_interfaces > 0;
-    // 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);
 
-    if (num_interfaces != 0) {
+    m_interactionTimer.setSingleShot(true);
-        m_initialized = true;
+    m_interactionTimer.setInterval(200);
-        // 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
-    }
 }
 
 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;
+    bool found = false;
-    m_interfaces_detect_found = false;
+    if (m_interfaces_detect_mutex.tryLock(1000)) {
-    YubiKeyInterfaceUSB::instance()->findValidKeys();
+        found |= YubiKeyInterfaceUSB::instance()->findValidKeys();
-    YubiKeyInterfacePCSC::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<YubiKeySlot> 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);
     }

src/keys/drivers/YubiKey.h

@@ -46,7 +46,8 @@ public:
     static YubiKey* instance();
     bool isInitialized();
 
-    void findValidKeys();
+    bool findValidKeys();
+    void findValidKeysAsync();
 
     QList<YubiKeySlot> 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)

src/keys/drivers/YubiKeyInterface.cpp

@@ -37,6 +37,11 @@ QMultiMap<unsigned int, QPair<int, QString>> 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;

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<char>& response) = 0;
     virtual bool testChallenge(YubiKeySlot slot, bool* wouldBlock) = 0;

src/keys/drivers/YubiKeyInterfacePCSC.cpp

@@ -17,10 +17,9 @@
 
 #include "YubiKeyInterfacePCSC.h"
 
+#include "core/Tools.h"
 #include "crypto/Random.h"
 
-#include <QtConcurrent>
-
 // MSYS2 does not define these macros
 // So set them to the value used by pcsc-lite
 #ifndef MAX_ATR_SIZE
@@ -530,20 +529,12 @@ YubiKeyInterfacePCSC* YubiKeyInterfacePCSC::instance()
     return m_instance;
 }
 
-void YubiKeyInterfacePCSC::findValidKeys()
+bool YubiKeyInterfacePCSC::findValidKeys()
 {
     m_error.clear();
     if (!isInitialized()) {
-        return;
+        return false;
     }
-
-    QtConcurrent::run([this] {
-        // This mutex protects the smartcard against concurrent transmissions
-        if (!m_mutex.tryLock(1000)) {
-            emit detectComplete(false);
-            return;
-        }
-
     // Remove all known keys
     m_foundKeys.clear();
 
@@ -594,10 +585,9 @@ void YubiKeyInterfacePCSC::findValidKeys()
                     // Add the firmware version and the serial number
                     uint8_t version[3] = {0};
                     getStatus(satr, version);
-                        name += QString(" v%1.%2.%3")
+                    name +=
-                                    .arg(QString::number(version[0]),
+                        QString(" v%1.%2.%3")
-                                         QString::number(version[1]),
+                            .arg(QString::number(version[0]), QString::number(version[1]), QString::number(version[2]));
-                                         QString::number(version[2]));
 
                     unsigned int serial = 0;
                     getSerial(satr, serial);
@@ -630,9 +620,7 @@ void YubiKeyInterfacePCSC::findValidKeys()
         }
     }
 
-        m_mutex.unlock();
+    return !m_foundKeys.isEmpty();
-        emit detectComplete(!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);
         }
     }
 

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<char>& response) override;

src/keys/drivers/YubiKeyInterfaceUSB.cpp

@@ -24,8 +24,6 @@
 #include "thirdparty/ykcore/ykdef.h"
 #include "thirdparty/ykcore/ykstatus.h"
 
-#include <QtConcurrent>
-
 namespace
 {
     constexpr int MAX_KEYS = 4;
@@ -109,17 +107,11 @@ 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
@@ -188,9 +180,7 @@ void YubiKeyInterfaceUSB::findValidKeys()
         }
     }
 
-        m_mutex.unlock();
+    return !m_foundKeys.isEmpty();
-        emit detectComplete(!m_foundKeys.isEmpty());
-    });
 }
 
 /**

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<char>& response) override;

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()
 {
 }
 

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(),

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);