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
keepassxreboot · Apr 3, 2022 · 48a3fd8 · 48a3fd8
1 parent 7d7c635
commit 48a3fd8
Show file tree

Hide file tree

Showing 15 changed files with 207 additions and 224 deletions.
diff --git a/share/translations/keepassxc_en.ts b/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>

diff --git 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<ChallengeResponseKey>(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
@@ -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
@@ -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
@@ -20,52 +20,36 @@
 #include "YubiKeyInterfacePCSC.h"
 #include "YubiKeyInterfaceUSB.h"
 
+#include <QtConcurrent>
+
 YubiKey::YubiKey()
     : m_interfaces_detect_mutex(QMutex::Recursive)
 {
     int num_interfaces = 0;
 
     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);
-
-    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_initialized = num_interfaces > 0;
+
+    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<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);
     }

diff --git a/src/keys/drivers/YubiKey.h b/src/keys/drivers/YubiKey.h
@@ -46,7 +46,8 @@ class YubiKey : public QObject
     static YubiKey* instance();
     bool isInitialized();
 
-    void findValidKeys();
+    bool findValidKeys();
+    void findValidKeysAsync();
 
     QList<YubiKeySlot> foundKeys();
     QString getDisplayName(YubiKeySlot slot);
@@ -84,8 +85,6 @@ class YubiKey : public QObject
     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
@@ -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;

diff --git a/src/keys/drivers/YubiKeyInterface.h b/src/keys/drivers/YubiKeyInterface.h
@@ -36,7 +36,7 @@ class YubiKeyInterface : public QObject
     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;