Significantly enhance hardware key robustness

* Significantly improve user experience when using hardware keys on databases in both GUI and CLI modes. Prevent locking up the YubiKey USB interface for prolonged periods of time. Allows for other apps to use the key concurrently with KeePassXC.

* Improve messages displayed to user when finding keys and when user interaction is required. Output specific error messages when handling hardware keys during database read/write.

* Only poll for keys when previously used or upon user request. Prevent continuously polling keys when accessing the UI such as switching tabs and minimize/maximize.

* Add support for using multiple hardware keys simultaneously. Keys are identified by their serial number which prevents using the wrong key during open and save operations.

* Fixes #4400
* Fixes #4065
* Fixes #1050
* Fixes #1215
* Fixes #3087
* Fixes #1088
* Fixes #1869

4 files changed, 88 insertions, 91 deletions

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 3a74622cb..e1bfaac13 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -207,9 +207,11 @@ add_unit_test(NAME testentrysearcher SOURCES TestEntrySearcher.cpp
 add_unit_test(NAME testcsvexporter SOURCES TestCsvExporter.cpp
         LIBS ${TEST_LIBRARIES})
 
-add_unit_test(NAME testykchallengeresponsekey
+if(WITH_XC_YUBIKEY)
+    add_unit_test(NAME testykchallengeresponsekey
         SOURCES TestYkChallengeResponseKey.cpp
         LIBS ${TEST_LIBRARIES})
+endif()
 
 if(WITH_XC_KEESHARE)
     add_unit_test(NAME testsharing SOURCES TestSharing.cpp
diff --git a/tests/TestCli.cpp b/tests/TestCli.cpp
index 10bde7a08..348afb670 100644
--- a/tests/TestCli.cpp
+++ b/tests/TestCli.cpp
@@ -51,6 +51,9 @@
 
 #include <QClipboard>
 #include <QFuture>
+#include <QSet>
+#include <QSignalSpy>
+#include <QTextStream>
 #include <QtConcurrent>
 
 QTEST_MAIN(TestCli)
@@ -1711,26 +1714,46 @@ void TestCli::testInvalidDbFiles()
 /**
  * Secret key for the YubiKey slot used by the unit test is
  * 1c e3 0f d7 8d 20 dc fa 40 b5 0c 18 77 9a fb 0f 02 28 8d b7
- * This secret should be configured at slot 2, and the slot
- * should be configured as passive.
+ * This secret can be on either slot but must be passive.
  */
 void TestCli::testYubiKeyOption()
 {
-    if (!YubiKey::instance()->init()) {
-        QSKIP("Unable to connect to YubiKey");
+    if (!YubiKey::instance()->isInitialized()) {
+        QSKIP("Unable to initialize YubiKey interface.");
     }
 
-    QString errorMessage;
-    bool isBlocking = YubiKey::instance()->checkSlotIsBlocking(2, errorMessage);
-    if (isBlocking && errorMessage.isEmpty()) {
-        QSKIP("Skipping YubiKey in press mode.");
+    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.");
     }
 
+    bool wouldBlock = false;
     QByteArray challenge("CLITest");
     QByteArray response;
-    YubiKey::instance()->challenge(2, false, challenge, response);
     QByteArray expected("\xA2\x3B\x94\x00\xBE\x47\x9A\x30\xA9\xEB\x50\x9B\x85\x56\x5B\x6B\x30\x25\xB4\x8E", 20);
-    QVERIFY2(response == expected, "YubiKey Slot 2 is not configured with correct secret key.");
+
+    // Find a key that as configured for this test
+    YubiKeySlot pKey(0, 0);
+    for (auto key : keys) {
+        if (YubiKey::instance()->testChallenge(key, &wouldBlock) && !wouldBlock) {
+            YubiKey::instance()->challenge(key, challenge, response);
+            if (response == expected) {
+                pKey = key;
+                break;
+            }
+            Tools::wait(100);
+        }
+    }
+
+    if (pKey.first == 0 && pKey.second == 0) {
+        QSKIP("No YubiKey is properly configured to perform this test.");
+    }
 
     List listCmd;
     Add addCmd;
diff --git a/tests/TestYkChallengeResponseKey.cpp b/tests/TestYkChallengeResponseKey.cpp
index a4dd76270..05161544b 100644
--- a/tests/TestYkChallengeResponseKey.cpp
+++ b/tests/TestYkChallengeResponseKey.cpp
@@ -19,82 +19,74 @@
 
 #include "TestYkChallengeResponseKey.h"
 #include "TestGlobal.h"
+
+#include "core/Tools.h"
 #include "crypto/Crypto.h"
+#include "keys/YkChallengeResponseKey.h"
 
-#include <QtConcurrentRun>
+#include <QScopedPointer>
+#include <QSignalSpy>
 
-QTEST_GUILESS_MAIN(TestYubiKeyChalResp)
+QTEST_GUILESS_MAIN(TestYubiKeyChallengeResponse)
 
-void TestYubiKeyChalResp::initTestCase()
+void TestYubiKeyChallengeResponse::initTestCase()
 {
     // crypto subsystem needs to be initialized for YubiKey testing
     QVERIFY(Crypto::init());
-}
 
-void TestYubiKeyChalResp::init()
-{
-    if (!YubiKey::instance()->init()) {
-        QSKIP("Unable to connect to YubiKey");
+    if (!YubiKey::instance()->isInitialized()) {
+        QSKIP("Unable to initialize YubiKey interface.");
     }
 }
 
-void TestYubiKeyChalResp::detectDevices()
-{
-    connect(YubiKey::instance(), SIGNAL(detected(int, bool)), SLOT(ykDetected(int, bool)), Qt::QueuedConnection);
-    QtConcurrent::run(YubiKey::instance(), &YubiKey::detect);
-
-    // need to wait for the hardware (that's hopefully plugged in)...
-    QTest::qWait(2000);
-    QVERIFY2(m_detected > 0, "Is a YubiKey attached?");
-}
-
-void TestYubiKeyChalResp::getSerial()
-{
-    unsigned int serial;
-    QVERIFY(YubiKey::instance()->getSerial(serial));
-}
-
-void TestYubiKeyChalResp::keyGetName()
-{
-    QVERIFY(m_key);
-    QVERIFY(m_key->getName().length() > 0);
-}
-
-void TestYubiKeyChalResp::keyIssueChallenge()
+void TestYubiKeyChallengeResponse::testDetectDevices()
 {
-    QVERIFY(m_key);
-    if (m_key->isBlocking()) {
-        /* Testing active mode in unit tests is unreasonable */
-        QSKIP("YubiKey not in passive mode", SkipSingle);
+    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);
+        QVERIFY(displayName.contains("Challenge Response - Slot") || displayName.contains("Configured Slot -"));
+        QVERIFY(displayName.contains(QString::number(key.first)));
+        QVERIFY(displayName.contains(QString::number(key.second)));
     }
-
-    QByteArray ba("UnitTest");
-    QVERIFY(m_key->challenge(ba));
-
-    /* TODO Determine if it's reasonable to provide a fixed secret key for
-     *      verification testing.  Obviously simple technically, but annoying
-     *      if devs need to re-program their yubikeys or have a spare test key
-     *      for unit tests to pass.
-     *
-     *      Might be worth it for integrity verification though.
-     */
 }
 
-void TestYubiKeyChalResp::ykDetected(int slot, bool blocking)
+/**
+ * Secret key for the YubiKey slot used by the unit test is
+ * 1c e3 0f d7 8d 20 dc fa 40 b5 0c 18 77 9a fb 0f 02 28 8d b7
+ * This secret can be on either slot but must be passive.
+ */
+void TestYubiKeyChallengeResponse::testKeyChallenge()
 {
-    Q_UNUSED(blocking);
+    auto keys = YubiKey::instance()->foundKeys();
+    if (keys.isEmpty()) {
+        QSKIP("No YubiKey devices were detected.");
+    }
 
-    if (slot > 0) {
-        m_detected++;
+    // Find a key that is configured in passive mode
+    bool wouldBlock = false;
+    YubiKeySlot pKey(0, 0);
+    for (auto key : keys) {
+        if (YubiKey::instance()->testChallenge(key, &wouldBlock) && !wouldBlock) {
+            pKey = key;
+            break;
+        }
+        Tools::wait(100);
     }
 
-    /* Key used for later testing */
-    if (!m_key) {
-        m_key.reset(new YkChallengeResponseKey(slot, blocking));
+    if (pKey.first == 0) {
+        /* Testing active mode in unit tests is unreasonable */
+        QSKIP("No YubiKey contains a slot in passive mode.");
     }
-}
 
-void TestYubiKeyChalResp::deinit()
-{
-    QVERIFY(YubiKey::instance()->deinit());
+    QScopedPointer<YkChallengeResponseKey> key(new YkChallengeResponseKey(pKey));
+
+    QByteArray ba("UnitTest");
+    QVERIFY(key->challenge(ba));
+    QCOMPARE(key->rawKey().size(), 20);
 }
diff --git a/tests/TestYkChallengeResponseKey.h b/tests/TestYkChallengeResponseKey.h
index 81253cc90..63fcaf6ee 100644
--- a/tests/TestYkChallengeResponseKey.h
+++ b/tests/TestYkChallengeResponseKey.h
@@ -20,36 +20,16 @@
 #define KEEPASSX_TESTYUBIKEYCHALRESP_H
 
 #include <QObject>
-#include <QScopedPointer>
 
-#include "keys/YkChallengeResponseKey.h"
-
-class TestYubiKeyChalResp : public QObject
+class TestYubiKeyChallengeResponse : public QObject
 {
     Q_OBJECT
 
 private slots:
     void initTestCase();
 
-    void init();
-
-    /* Order is important!
-     * Need to init and detectDevices() before proceeding
-     */
-    void detectDevices();
-
-    void getSerial();
-    void keyGetName();
-    void keyIssueChallenge();
-
-    void deinit();
-
-    /* Callback for detectDevices() */
-    void ykDetected(int slot, bool blocking);
-
-private:
-    int m_detected = 0;
-    QScopedPointer<YkChallengeResponseKey> m_key;
+    void testDetectDevices();
+    void testKeyChallenge();
 };
 
 #endif // KEEPASSX_TESTYUBIKEYCHALRESP_H