From 872e914520c4b075163ea8af470793ac5b18372a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konrad=20Vit=C3=A9?= Date: Thu, 6 Feb 2025 22:09:24 +0100 Subject: [PATCH] ssh-agent: Implement destination constraints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change implements loading ssh-agent destination constraints from KeeAgent.settings into the ssh-agent. For now there is no UI so configuration must be done in KeePass2/KeeAgent or manually. The ssh-agent constrain extension is described at [1]. However, I found it partly misleading: - in the constaint array each constraint is enveloped where in the keyspec arrays the keyspec are just appended to the constraint. - each constraint and host has an additional string field reserved for future use. The actual structure has been obtained from openssh ssh-add source code [2]. [1]: https://www.openssh.com/agent-restrict.html [2]: https://github.com/openssh/openssh-portable/blob/3ad669f81aabbd2ba9fbd472903f680f598e1e99/authfd.c#L538 Signed-off-by: Konrad Vité --- share/translations/keepassxc_en.ts | 4 + src/sshagent/KeeAgentSettings.cpp | 178 +++++++++++++++++++++++++++++ src/sshagent/KeeAgentSettings.h | 27 +++++ src/sshagent/SSHAgent.cpp | 58 ++++++++++ src/sshagent/SSHAgent.h | 10 +- tests/TestSSHAgent.cpp | 118 +++++++++++++++++++ tests/TestSSHAgent.h | 1 + 7 files changed, 395 insertions(+), 1 deletion(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 7d4dee1ee..3463122c4 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -9839,6 +9839,10 @@ This option is deprecated, use --set-key-file instead. All SSH identities removed from agent. + + Destination constraints are invalid or not supported by the agent (check options). + + SearchHelpWidget diff --git a/src/sshagent/KeeAgentSettings.cpp b/src/sshagent/KeeAgentSettings.cpp index a794eac93..0d3084742 100644 --- a/src/sshagent/KeeAgentSettings.cpp +++ b/src/sshagent/KeeAgentSettings.cpp @@ -35,6 +35,29 @@ KeeAgentSettings::KeeAgentSettings() reset(); } +bool KeeAgentSettings::KeySpec::operator==(const KeeAgentSettings::KeySpec& other) const +{ + return (key == other.key && isCertificateAuthority == other.isCertificateAuthority); +} + +QByteArray KeeAgentSettings::KeySpec::getKeyBlob() const +{ + // In KeeAgent the key data is the second word in the string. First is the + // key type. Third is the key comment which is optional. + auto words = key.split(" "); + if (words.length() >= 2) { + return QByteArray::fromBase64(words[1].toLatin1(), QByteArray::Base64Encoding); + } else { + return QByteArray(); + } +} + +bool KeeAgentSettings::DestinationConstraint::operator==(const KeeAgentSettings::DestinationConstraint& other) const +{ + return (fromHost == other.fromHost && fromHostKeys == other.fromHostKeys && toUser == other.toUser + && toHost == other.toHost && toHostKeys == other.toHostKeys); +} + bool KeeAgentSettings::operator==(const KeeAgentSettings& other) const { // clang-format off @@ -43,6 +66,8 @@ bool KeeAgentSettings::operator==(const KeeAgentSettings& other) const && m_useConfirmConstraintWhenAdding == other.m_useConfirmConstraintWhenAdding && m_useLifetimeConstraintWhenAdding == other.m_useLifetimeConstraintWhenAdding && m_lifetimeConstraintDuration == other.m_lifetimeConstraintDuration + && m_useDestinationConstraintsWhenAdding == other.m_useDestinationConstraintsWhenAdding + && m_destinationConstraints == other.m_destinationConstraints && m_selectedType == other.m_selectedType && m_attachmentName == other.m_attachmentName && m_saveAttachmentToTempFile == other.m_saveAttachmentToTempFile @@ -77,6 +102,8 @@ void KeeAgentSettings::reset() m_useConfirmConstraintWhenAdding = false; m_useLifetimeConstraintWhenAdding = false; m_lifetimeConstraintDuration = 600; + m_useDestinationConstraintsWhenAdding = false; + m_destinationConstraints.clear(); m_selectedType = QStringLiteral("file"); m_attachmentName.clear(); @@ -125,6 +152,16 @@ int KeeAgentSettings::lifetimeConstraintDuration() const return m_lifetimeConstraintDuration; } +bool KeeAgentSettings::useDestinationConstraintsWhenAdding() const +{ + return m_useDestinationConstraintsWhenAdding; +} + +QList KeeAgentSettings::destinationConstraints() const +{ + return m_destinationConstraints; +} + const QString KeeAgentSettings::selectedType() const { return m_selectedType; @@ -180,6 +217,16 @@ void KeeAgentSettings::setLifetimeConstraintDuration(int lifetimeConstraintDurat m_lifetimeConstraintDuration = lifetimeConstraintDuration; } +void KeeAgentSettings::setUseDestinationConstraintsWhenAdding(bool useDestinationConstraintsWhenAdding) +{ + m_useDestinationConstraintsWhenAdding = useDestinationConstraintsWhenAdding; +} + +void KeeAgentSettings::setDestinationConstraints(const QList& destinationConstraints) +{ + m_destinationConstraints = destinationConstraints; +} + void KeeAgentSettings::setSelectedType(const QString& selectedType) { m_selectedType = selectedType; @@ -229,6 +276,8 @@ bool KeeAgentSettings::fromXml(const QByteArray& ba) QXmlStreamReader reader; reader.addData(ba); + reset(); + if (reader.error() || !reader.readNextStartElement()) { m_error = reader.errorString(); return false; @@ -273,6 +322,88 @@ bool KeeAgentSettings::fromXml(const QByteArray& ba) reader.skipCurrentElement(); } } + if (!reader.error()) + reader.readNext(); + } else if (reader.name() == "UseDestinationConstraintWhenAdding") { + m_useDestinationConstraintsWhenAdding = readBool(reader); + } else if (reader.name() == "DestinationConstraints") { + while (!reader.error() && reader.readNextStartElement()) { + if (reader.name() == "Constraint") { + KeeAgentSettings::DestinationConstraint constraint; + while (!reader.error() && reader.readNextStartElement()) { + if (reader.name() == "FromHostKeys" || reader.name() == "ToHostKeys") { + QString section = reader.name().toString(); + while (!reader.error() && reader.readNextStartElement()) { + if (reader.name() == "KeySpec") { + KeeAgentSettings::KeySpec keyspec; + while (!reader.error() && reader.readNextStartElement()) { + if (reader.name() == "HostKey") { + reader.readNext(); + keyspec.key = reader.text().toString(); + reader.readNext(); + } else if (reader.name() == "IsCA") { + keyspec.isCertificateAuthority = readBool(reader); + } else { + qWarning() << "Skipping KeySpec element" << reader.name(); + reader.skipCurrentElement(); + } + } + + if (keyspec.getKeyBlob().isEmpty()) { + return false; + } + + if (section == "FromHostKeys") { + constraint.fromHostKeys.append(std::move(keyspec)); + } else { + constraint.toHostKeys.append(std::move(keyspec)); + } + if (!reader.error()) + reader.readNext(); + } else { + qWarning() << "Skipping " << section << " element" << reader.name(); + reader.skipCurrentElement(); + } + } + if (!reader.error()) + reader.readNext(); + } else if (reader.name() == "FromHost") { + reader.readNext(); + constraint.fromHost = reader.text().toString(); + reader.readNext(); + } else if (reader.name() == "ToUser") { + reader.readNext(); + constraint.toUser = reader.text().toString(); + reader.readNext(); + } else if (reader.name() == "ToHost") { + reader.readNext(); + constraint.toHost = reader.text().toString(); + reader.readNext(); + } else { + qWarning() << "Skipping Constraint element" << reader.name(); + reader.skipCurrentElement(); + } + } + + if ((constraint.fromHost.isEmpty() && !constraint.fromHostKeys.isEmpty()) + || (!constraint.fromHost.isEmpty() && constraint.fromHostKeys.isEmpty())) { + return false; + } + if (constraint.toHost.isEmpty() || constraint.toHostKeys.isEmpty()) { + return false; + } + + m_destinationConstraints.append(std::move(constraint)); + + if (!reader.error()) + reader.readNext(); + } else { + qWarning() << "Skipping DestinationConstraints element" << reader.name(); + reader.skipCurrentElement(); + } + } + if (!reader.error()) + reader.readNext(); } else { qWarning() << "Skipping element" << reader.name(); reader.skipCurrentElement(); @@ -309,6 +440,53 @@ QByteArray KeeAgentSettings::toXml() const writer.writeTextElement("UseConfirmConstraintWhenAdding", m_useConfirmConstraintWhenAdding ? "true" : "false"); writer.writeTextElement("UseLifetimeConstraintWhenAdding", m_useLifetimeConstraintWhenAdding ? "true" : "false"); writer.writeTextElement("LifetimeConstraintDuration", QString::number(m_lifetimeConstraintDuration)); + writer.writeTextElement("UseDestinationConstraintWhenAdding", + m_useDestinationConstraintsWhenAdding ? "true" : "false"); + + writer.writeStartElement("DestinationConstraints"); + + foreach (const auto& constraint, m_destinationConstraints) { + writer.writeStartElement("Constraint"); + + if (constraint.fromHost.isEmpty()) { + writer.writeEmptyElement("FromHost"); + } else { + writer.writeTextElement("FromHost", constraint.fromHost); + } + + writer.writeStartElement("FromHostKeys"); + foreach (const auto& keyspec, constraint.fromHostKeys) { + writer.writeStartElement("KeySpec"); + writer.writeTextElement("HostKey", keyspec.key); + writer.writeTextElement("IsCA", keyspec.isCertificateAuthority ? "true" : "false"); + writer.writeEndElement(); // KeySpec + } + writer.writeEndElement(); // FromHostKeys + + if (constraint.toUser.isEmpty()) { + writer.writeEmptyElement("ToUser"); + } else { + writer.writeTextElement("ToUser", constraint.toUser); + } + if (constraint.toHost.isEmpty()) { + writer.writeEmptyElement("ToHost"); + } else { + writer.writeTextElement("ToHost", constraint.toHost); + } + + writer.writeStartElement("ToHostKeys"); + foreach (const auto& keyspec, constraint.toHostKeys) { + writer.writeStartElement("KeySpec"); + writer.writeTextElement("HostKey", keyspec.key); + writer.writeTextElement("IsCA", keyspec.isCertificateAuthority ? "true" : "false"); + writer.writeEndElement(); // KeySpec + } + writer.writeEndElement(); // ToHostKeys + + writer.writeEndElement(); // Constraint + } + + writer.writeEndElement(); // DestinationConstraints writer.writeStartElement("Location"); writer.writeTextElement("SelectedType", m_selectedType); diff --git a/src/sshagent/KeeAgentSettings.h b/src/sshagent/KeeAgentSettings.h index ffc14044e..d3a6da16e 100644 --- a/src/sshagent/KeeAgentSettings.h +++ b/src/sshagent/KeeAgentSettings.h @@ -29,6 +29,27 @@ class QXmlStreamReader; class KeeAgentSettings { public: + struct KeySpec + { + QString key; + bool isCertificateAuthority; + + bool operator==(const KeySpec& other) const; + + QByteArray getKeyBlob() const; + }; + + struct DestinationConstraint + { + QString fromHost; + QList fromHostKeys; + QString toUser; + QString toHost; + QList toHostKeys; + + bool operator==(const DestinationConstraint& other) const; + }; + KeeAgentSettings(); bool operator==(const KeeAgentSettings& other) const; bool operator!=(const KeeAgentSettings& other) const; @@ -58,6 +79,8 @@ public: bool useConfirmConstraintWhenAdding() const; bool useLifetimeConstraintWhenAdding() const; int lifetimeConstraintDuration() const; + bool useDestinationConstraintsWhenAdding() const; + QList destinationConstraints() const; const QString selectedType() const; const QString attachmentName() const; @@ -71,6 +94,8 @@ public: void setUseConfirmConstraintWhenAdding(bool useConfirmConstraintWhenAdding); void setUseLifetimeConstraintWhenAdding(bool useLifetimeConstraintWhenAdding); void setLifetimeConstraintDuration(int lifetimeConstraintDuration); + void setUseDestinationConstraintsWhenAdding(bool useDestinationConstraintsWhenAdding); + void setDestinationConstraints(const QList& destinationConstraints); void setSelectedType(const QString& type); void setAttachmentName(const QString& attachmentName); @@ -87,6 +112,8 @@ private: bool m_useConfirmConstraintWhenAdding; bool m_useLifetimeConstraintWhenAdding; int m_lifetimeConstraintDuration; + bool m_useDestinationConstraintsWhenAdding; + QList m_destinationConstraints; // location QString m_selectedType; diff --git a/src/sshagent/SSHAgent.cpp b/src/sshagent/SSHAgent.cpp index a8aa695ce..ead599c77 100644 --- a/src/sshagent/SSHAgent.cpp +++ b/src/sshagent/SSHAgent.cpp @@ -305,6 +305,12 @@ bool SSHAgent::addIdentity(OpenSSHKey& key, const KeeAgentSettings& settings, co request.writeString(securityKeyProvider()); } + if (settings.useDestinationConstraintsWhenAdding()) { + request.write(SSH_AGENT_CONSTRAIN_EXTENSION); + request.writeString(QString("restrict-destination-v00@openssh.com")); + encodeDestinationConstraints(settings.destinationConstraints(), request); + } + QByteArray responseData; if (!sendMessage(requestData, responseData)) { return false; @@ -322,6 +328,10 @@ bool SSHAgent::addIdentity(OpenSSHKey& key, const KeeAgentSettings& settings, co m_error += "\n" + tr("A confirmation request is not supported by the agent (check options)."); } + if (settings.useDestinationConstraintsWhenAdding()) { + m_error += "\n" + tr("Destination constraints are invalid or not supported by the agent (check options)."); + } + if (isSecurityKey) { m_error += "\n" + tr("Security keys are not supported by the agent or the security key provider is unavailable."); @@ -336,6 +346,54 @@ bool SSHAgent::addIdentity(OpenSSHKey& key, const KeeAgentSettings& settings, co return true; } +bool SSHAgent::encodeDestinationConstraints(const QList& constraints, + BinaryStream& out) +{ + QByteArray data; + BinaryStream stream(&data); + + foreach (const auto& constraint, constraints) { + encodeDestinationConstraint(constraint, stream); + } + + out.writeString(data); + return true; +} + +bool SSHAgent::encodeDestinationConstraint(const KeeAgentSettings::DestinationConstraint& constraint, BinaryStream& out) +{ + QByteArray data; + BinaryStream stream(&data); + + encodeDestinationConstraintHost("", constraint.fromHost, constraint.fromHostKeys, stream); + encodeDestinationConstraintHost(constraint.toUser, constraint.toHost, constraint.toHostKeys, stream); + stream.writeString(QString("")); // reserved + + out.writeString(data); + return true; +} + +bool SSHAgent::encodeDestinationConstraintHost(const QString user, + const QString hostname, + const QList& keys, + BinaryStream& out) +{ + QByteArray data; + BinaryStream stream(&data); + + stream.writeString(user); + stream.writeString(hostname); + stream.writeString(QString("")); // reserved + + foreach (const auto& key, keys) { + stream.writeString(key.getKeyBlob()); + stream.write(static_cast(key.isCertificateAuthority)); + } + + out.writeString(data); + return true; +} + /** * Remove an identity from the SSH agent. * diff --git a/src/sshagent/SSHAgent.h b/src/sshagent/SSHAgent.h index d3eeb4ebc..0001b8167 100644 --- a/src/sshagent/SSHAgent.h +++ b/src/sshagent/SSHAgent.h @@ -21,9 +21,9 @@ #include +#include "KeeAgentSettings.h" #include "OpenSSHKey.h" -class KeeAgentSettings; class Database; class SSHAgent : public QObject @@ -91,6 +91,14 @@ private: const quint32 AGENT_COPYDATA_ID = 0x804e50ba; #endif + bool encodeDestinationConstraints(const QList& constraints, + BinaryStream& out); + bool encodeDestinationConstraint(const KeeAgentSettings::DestinationConstraint& constraint, BinaryStream& out); + bool encodeDestinationConstraintHost(const QString user, + const QString hostname, + const QList& keys, + BinaryStream& out); + QHash> m_addedKeys; QString m_error; }; diff --git a/tests/TestSSHAgent.cpp b/tests/TestSSHAgent.cpp index 5becddd5d..152cb3fd6 100644 --- a/tests/TestSSHAgent.cpp +++ b/tests/TestSSHAgent.cpp @@ -27,6 +27,52 @@ QTEST_GUILESS_MAIN(TestSSHAgent) +static const QList githubKeys = { + { + .key = "ssh-rsa " + "AAAAB3NzaC1yc2EAAAADAQABAAABgQCj7ndNxQowgcQnjshcLrqPEiiphnt+" + "VTTvDP6mHBL9j1aNUkY4Ue1gvwnGLVlOhGeYrnZaMgRK6+PKCUXaDbC7qtbW8gIkhL7aGCsOr/C56SJMy/" + "BCZfxd1nWzAOxSDPgVsmerOBYfNqltV9/" + "hWCqBywINIR+5dIg6JTJ72pcEpEjcYgXkE2YEFXV1JHnsKgbLWNlhScqb2UmyRkQyytRLtL+38TGxkxCflmO+" + "5Z8CSSNY7GidjMIZ7Q4zMjA2n1nGrlTDkzwDCsw+" + "wqFPGQA179cnfGWOWRVruj16z6XyvxvjJwbz0wQZ75XK5tKSb7FNyeIEs4TT4jk+S4dhPeAUC5y+" + "bDYirYgM4GC7uEnztnZyaVWQ7B381AK4Qdrwt51ZqExKbQpTUNn+EjqoTwvqNj4kqx5QUCI0ThS/" + "YkOxJCXmPUWZbhjpCg56i+2aB6CmK2JGhn57K5mj0MNdBXA4/WnwH6XoPWJzK5Nyu2zB3nAZp+S5hpQs+p1vN1/wsjk=", + .isCertificateAuthority = false, + }, + { + .key = "ecdsa-sha2-nistp256 " + "AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEmKSENjQEezOmxkZMy7opKgwFB9nkt5YRrYMjNuG5N" + "87uRgg6CLrbo5wAdT/y6v0mKV0U2w0WZ2YB/++Tpockg=", + .isCertificateAuthority = false, + }, + { + .key = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl", + .isCertificateAuthority = false, + }}; + +static const QList gitlabKeys = { + { + .key = "ssh-rsa " + "AAAAB3NzaC1yc2EAAAADAQABAAABAQCsj2bNKTBSpIYDEGk9KxsGh3mySTRgMtXL583qmBpzeQ+jqCMRgBqB98u3z++" + "J1sKlXHWfM9dyhSevkMwSbhoR8XIq/U0tCNyokEi/" + "ueaBMCvbcTHhO7FcwzY92WK4Yt0aGROY5qX2UKSeOvuP4D6TPqKF1onrSzH9bx9XUf2lEdWT/ia1NEKjunUqu1xOB/" + "StKDHMoX4/OKyIzuS0q/" + "T1zOATthvasJFoPrAjkohTyaDUz2LN5JoH839hViyEG82yB+MjcFV5MU3N1l1QL3cVUCh93xSaua1N85qivl+" + "siMkPGbO5xR/En4iEY6K2XPASUEMaieWVNTRCtJ4S8H+9", + .isCertificateAuthority = false, + }, + { + .key = "ecdsa-sha2-nistp256 " + "AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBFSMqzJeV9rUzU4kWitGjeR4PWSa29SPqJ1fVkhtj3H" + "w9xjLVXVYrU9QlYWrOLXBpQ6KWjbjTDTdDkoohFzgbEY=", + .isCertificateAuthority = false, + }, + { + .key = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAfuCHKVTjquxvt6CM6tdG4SLp1Btn/nOeHHE5UOzRdf", + .isCertificateAuthority = false, + }}; + void TestSSHAgent::initTestCase() { QVERIFY(Crypto::init()); @@ -177,6 +223,41 @@ void TestSSHAgent::testKeeAgentSettings() QVERIFY(settings2.fromXml(settings.toXml())); QVERIFY(settings2.saveAttachmentToTempFile()); QVERIFY(settings == settings2); + + QVERIFY(!settings.useDestinationConstraintsWhenAdding()); + settings.setUseDestinationConstraintsWhenAdding(true); + QVERIFY(settings2.fromXml(settings.toXml())); + QVERIFY(settings2.useDestinationConstraintsWhenAdding()); + QVERIFY(settings == settings2); + + QList destinationConstraints; + + // ssh-add -h github.com + destinationConstraints.append({ + .fromHost = "", + .fromHostKeys = {}, + .toUser = "", + .toHost = "github.com", + .toHostKeys = githubKeys, + }); + QVERIFY(settings.destinationConstraints().isEmpty()); + settings.setDestinationConstraints(destinationConstraints); + QVERIFY(settings2.fromXml(settings.toXml())); + QVERIFY(settings2.destinationConstraints() == destinationConstraints); + QVERIFY(settings == settings2); + + // ssh-add -h github.com -h "github.com>git@gitlab.com" + destinationConstraints.append({ + .fromHost = "github.com", + .fromHostKeys = githubKeys, + .toUser = "git", + .toHost = "gitlab.com", + .toHostKeys = gitlabKeys, + }); + settings.setDestinationConstraints(destinationConstraints); + QVERIFY(settings2.fromXml(settings.toXml())); + QVERIFY(settings2.destinationConstraints() == destinationConstraints); + QVERIFY(settings == settings2); } void TestSSHAgent::testIdentity() @@ -281,6 +362,43 @@ void TestSSHAgent::testConfirmConstraint() QVERIFY(agent.checkIdentity(m_key, keyInAgent) && !keyInAgent); } +void TestSSHAgent::testDestinationConstraints() +{ + SSHAgent agent; + agent.setEnabled(true); + agent.setAuthSockOverride(m_agentSocketFileName); + + QVERIFY(agent.isAgentRunning()); + + KeeAgentSettings settings; + bool keyInAgent; + + // ssh-add -h github.com -h "github.com>git@gitlab.com" + settings.setUseDestinationConstraintsWhenAdding(true); + settings.setDestinationConstraints({{ + .fromHost = "", + .fromHostKeys = {}, + .toUser = "", + .toHost = "github.com", + .toHostKeys = githubKeys, + }, + { + .fromHost = "github.com", + .fromHostKeys = githubKeys, + .toUser = "git", + .toHost = "gitlab.com", + .toHostKeys = gitlabKeys, + }}); + + QVERIFY(agent.addIdentity(m_key, settings, m_uuid)); + + // we can't test destination constraints itself is working but we can test the agent accepts the key + QVERIFY(agent.checkIdentity(m_key, keyInAgent) && keyInAgent); + + QVERIFY(agent.removeIdentity(m_key)); + QVERIFY(agent.checkIdentity(m_key, keyInAgent) && !keyInAgent); +} + void TestSSHAgent::testToOpenSSHKey() { KeeAgentSettings settings; diff --git a/tests/TestSSHAgent.h b/tests/TestSSHAgent.h index a51f4a1be..bdf697998 100644 --- a/tests/TestSSHAgent.h +++ b/tests/TestSSHAgent.h @@ -35,6 +35,7 @@ private slots: void testRemoveOnClose(); void testLifetimeConstraint(); void testConfirmConstraint(); + void testDestinationConstraints(); void testToOpenSSHKey(); void testKeyGenRSA(); void testKeyGenECDSA();