From b6068b90a4965744329036315355d436b2ea6a12 Mon Sep 17 00:00:00 2001 From: underclockeddev <71031886+underclockeddev@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:11:10 -0400 Subject: [PATCH] Track SSH agent key ownership by the root group uuid SSHAgent recorded which database added each key by Database::uuid(). That uuid is generated for every Database object and is not stored in the file, so a reload from disk (which builds a new Database object) orphaned the keys: locking no longer removed them, and unlocking was refused as an ownership conflict. Use the root group uuid instead, which is stored in the file and survives a reload. Two open copies of the same file now share ownership of their keys instead of refusing each other: either one can add them, and locking either removes them. Fixes #13704 Co-Authored-By: Claude Opus 5.5 --- src/gui/DatabaseWidget.cpp | 2 +- src/gui/entry/EditEntryWidget.cpp | 2 +- src/sshagent/SSHAgent.cpp | 7 +- tests/TestSSHAgent.cpp | 136 +++++++++++++++++++++++++++++- tests/TestSSHAgent.h | 8 ++ 5 files changed, 148 insertions(+), 7 deletions(-) diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 8a29d1822a..10c8b1acbf 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -842,7 +842,7 @@ void DatabaseWidget::addToAgent() SSHAgent* agent = SSHAgent::instance(); OpenSSHKey key; if (settings.toOpenSSHKey(currentEntry, key, true)) { - if (!agent->addIdentity(key, settings, database()->uuid())) { + if (!agent->addIdentity(key, settings, database()->rootGroup()->uuid())) { m_messageWidget->showMessage(agent->errorString(), MessageWidget::Error); } } else { diff --git a/src/gui/entry/EditEntryWidget.cpp b/src/gui/entry/EditEntryWidget.cpp index e98c05fce3..c8c16e6ebb 100644 --- a/src/gui/entry/EditEntryWidget.cpp +++ b/src/gui/entry/EditEntryWidget.cpp @@ -886,7 +886,7 @@ void EditEntryWidget::addKeyToAgent() KeeAgentSettings settings; toKeeAgentSettings(settings); - if (!sshAgent()->addIdentity(key, settings, m_db->uuid())) { + if (!sshAgent()->addIdentity(key, settings, m_db->rootGroup()->uuid())) { showMessage(sshAgent()->errorString(), MessageWidget::Error); return; } diff --git a/src/sshagent/SSHAgent.cpp b/src/sshagent/SSHAgent.cpp index 37881fdf5e..fc811e2f5c 100644 --- a/src/sshagent/SSHAgent.cpp +++ b/src/sshagent/SSHAgent.cpp @@ -264,7 +264,8 @@ bool SSHAgent::sendMessagePageant(const QByteArray& in, QByteArray& out) * * @param key identity / key to add * @param settings constraints (lifetime, confirm), remove-on-lock - * @param databaseUuid database that owns the key for remove-on-lock + * @param databaseUuid database that owns the key for remove-on-lock: its root group uuid, + * which is stored in the file and survives a reload (Database::uuid() does not) * @return true on success */ bool SSHAgent::addIdentity(OpenSSHKey& key, const KeeAgentSettings& settings, const QUuid& databaseUuid) @@ -535,7 +536,7 @@ void SSHAgent::databaseLocked(const QSharedPointer& db) auto it = m_addedKeys.begin(); while (it != m_addedKeys.end()) { - if (it.value().first != db->uuid()) { + if (it.value().first != db->rootGroup()->uuid()) { ++it; continue; } @@ -578,7 +579,7 @@ void SSHAgent::databaseUnlocked(const QSharedPointer& db) // Add key to agent; ignore errors if we have previously added the key bool known_key = m_addedKeys.contains(key); - if (!addIdentity(key, settings, db->uuid()) && !known_key) { + if (!addIdentity(key, settings, db->rootGroup()->uuid()) && !known_key) { emit error(m_error); } } diff --git a/tests/TestSSHAgent.cpp b/tests/TestSSHAgent.cpp index 6ff0f18a7a..2d18529fad 100644 --- a/tests/TestSSHAgent.cpp +++ b/tests/TestSSHAgent.cpp @@ -18,12 +18,15 @@ #include "TestSSHAgent.h" #include "config-keepassx-tests.h" #include "core/Config.h" +#include "core/Group.h" #include "crypto/Crypto.h" +#include "keys/PasswordKey.h" #include "sshagent/KeeAgentSettings.h" #include "sshagent/OpenSSHKeyGen.h" #include "sshagent/SSHAgent.h" #include +#include #include QTEST_GUILESS_MAIN(TestSSHAgent) @@ -84,9 +87,9 @@ void TestSSHAgent::initTestCase() "MEBQY=\n" "-----END OPENSSH PRIVATE KEY-----\n"); - const QByteArray keyData = keyString.toLatin1(); + m_keyData = keyString.toLatin1(); - QVERIFY(m_key.parsePKCS1PEM(keyData)); + QVERIFY(m_key.parsePKCS1PEM(m_keyData)); } void TestSSHAgent::init() @@ -164,6 +167,135 @@ void TestSSHAgent::testRemoveOnClose() QVERIFY(agent.checkIdentity(m_key, keyInAgent) && !keyInAgent); } +// A database file holding m_key as an attachment, set to be added to the agent +// on unlock and removed on lock. +void TestSSHAgent::writeKeyDatabase(const QString& filePath) +{ + auto db = QSharedPointer::create(); + auto key = QSharedPointer::create(); + key->addKey(QSharedPointer::create("a")); + db->setKey(key); + + auto* entry = new Entry(); + entry->setUuid(QUuid::createUuid()); + entry->setGroup(db->rootGroup()); + entry->attachments()->set("id_ed25519", m_keyData); + + KeeAgentSettings settings; + settings.setAllowUseOfSshKey(true); + settings.setAddAtDatabaseOpen(true); + settings.setRemoveAtDatabaseClose(true); + settings.setSelectedType("attachment"); + settings.setAttachmentName("id_ed25519"); + settings.toEntry(entry); + + QString error; + QVERIFY2(db->saveAs(filePath, Database::Atomic, QString(), &error), qPrintable(error)); +} + +// Every load of the file builds a new Database object, as a reload from disk +// (DatabaseWidget::reloadDatabaseFile) or opening a second copy of the file does. +QSharedPointer TestSSHAgent::loadKeyDatabase(const QString& filePath) +{ + auto db = QSharedPointer::create(); + auto key = QSharedPointer::create(); + key->addKey(QSharedPointer::create("a")); + QString error; + if (!db->open(filePath, key, &error)) { + qWarning() << error; + return {}; + } + return db; +} + +void TestSSHAgent::testRemoveOnLockAfterReload() +{ + SSHAgent agent; + agent.setEnabled(true); + agent.setAuthSockOverride(m_agentSocketFileName); + QVERIFY(agent.isAgentRunning()); + auto cleanup = qScopeGuard([&] { agent.removeIdentity(m_key); }); + + TemporaryFile file; + QVERIFY(file.open()); + writeKeyDatabase(file.fileName()); + bool keyInAgent; + + auto db = loadKeyDatabase(file.fileName()); + QVERIFY(db); + agent.databaseUnlocked(db); + QVERIFY(agent.checkIdentity(m_key, keyInAgent) && keyInAgent); + + auto reloaded = loadKeyDatabase(file.fileName()); + QVERIFY(reloaded); + db.reset(); + + agent.databaseLocked(reloaded); + QVERIFY(agent.checkIdentity(m_key, keyInAgent)); + QVERIFY2(!keyInAgent, "key stayed in the agent after locking a reloaded database"); +} + +void TestSSHAgent::testReaddOnUnlockAfterReload() +{ + SSHAgent agent; + agent.setEnabled(true); + agent.setAuthSockOverride(m_agentSocketFileName); + QVERIFY(agent.isAgentRunning()); + auto cleanup = qScopeGuard([&] { agent.removeIdentity(m_key); }); + + TemporaryFile file; + QVERIFY(file.open()); + writeKeyDatabase(file.fileName()); + bool keyInAgent; + + auto db = loadKeyDatabase(file.fileName()); + QVERIFY(db); + agent.databaseUnlocked(db); + + auto reloaded = loadKeyDatabase(file.fileName()); + QVERIFY(reloaded); + db.reset(); + agent.databaseLocked(reloaded); + + // The user clears the agent (ssh-add -D) and unlocks again. + agent.removeIdentity(m_key); + agent.databaseUnlocked(reloaded); + QCOMPARE(agent.errorString(), QString()); + QVERIFY(agent.checkIdentity(m_key, keyInAgent)); + QVERIFY2(keyInAgent, "key was not added back when unlocking a reloaded database"); +} + +// Two copies of one file open at once hold the same keys, so the agent treats +// them as one owner: neither is refused, and locking either removes the keys. +void TestSSHAgent::testTwoOpenCopiesShareKeys() +{ + SSHAgent agent; + agent.setEnabled(true); + agent.setAuthSockOverride(m_agentSocketFileName); + QVERIFY(agent.isAgentRunning()); + auto cleanup = qScopeGuard([&] { agent.removeIdentity(m_key); }); + + TemporaryFile original; + QVERIFY(original.open()); + writeKeyDatabase(original.fileName()); + TemporaryFile copy; + QVERIFY(copy.copyFromFile(original.fileName())); + bool keyInAgent; + + auto first = loadKeyDatabase(original.fileName()); + auto second = loadKeyDatabase(copy.fileName()); + QVERIFY(first && second); + + agent.databaseUnlocked(first); + agent.databaseUnlocked(second); + QCOMPARE(agent.errorString(), QString()); + QVERIFY(agent.checkIdentity(m_key, keyInAgent) && keyInAgent); + + agent.databaseLocked(second); + QVERIFY(agent.checkIdentity(m_key, keyInAgent)); + QVERIFY2(!keyInAgent, "key stayed in the agent after locking the second copy"); +} + void TestSSHAgent::testLifetimeConstraint() { SSHAgent agent; diff --git a/tests/TestSSHAgent.h b/tests/TestSSHAgent.h index db06fd8069..80f3f81f7f 100644 --- a/tests/TestSSHAgent.h +++ b/tests/TestSSHAgent.h @@ -18,6 +18,7 @@ #ifndef TESTSSHAGENT_H #define TESTSSHAGENT_H +#include "core/Database.h" #include "sshagent/OpenSSHKey.h" #include "util/TemporaryFile.h" #include @@ -33,6 +34,9 @@ private slots: void testConfiguration(); void testIdentity(); void testRemoveOnClose(); + void testRemoveOnLockAfterReload(); + void testReaddOnUnlockAfterReload(); + void testTwoOpenCopiesShareKeys(); void testLifetimeConstraint(); void testConfirmConstraint(); void testToOpenSSHKey(); @@ -45,6 +49,10 @@ private slots: QScopedPointer m_agentSocketFile; QString m_agentSocketFileName; QProcess m_agentProcess; + void writeKeyDatabase(const QString& filePath); + QSharedPointer loadKeyDatabase(const QString& filePath); + + QByteArray m_keyData; OpenSSHKey m_key; QUuid m_uuid; };