SSH agent client support (KeeAgent compatible) by hifi · Pull Request #1098 · keepassxreboot/keepassxc

hifi · 2017-10-21T11:30:22Z

This implements a ssh-agent client that sends keys from your KeePassXC database to a running agent when a database is unlocked and removes them when it's locked.

Description

New OpenSSH key types are supported (DSA, RSA, ECDSA and ED25519) including decryption. Old keys can be converted to new format with ssh-keygen or PuTTYgen without needing to rotate your public key on remote hosts.

POSIX and Mac agent access is through the environment SSH_AUTH_SOCK UNIX socket and the socket is opened on-demand when keys are added or removed from the agent. Communication is blocking through BinaryStream class that wraps around QIODevice and thus QLocalSocket.

Windows (Pageant) agent access is using shared memory and it shares the same protocol. This has been inlined and #ifdef separated from the socket version as it is quite small.

TODO

Non-goals (updated)

Old OpenSSH key format support (manual conversion possible)
PuTTY key (ppk) support (manual conversion possible)
Implementing an agent server (use ssh-agent on Linux/Mac or Pageant on Windows)

Motivation and context

I didn't know I need KeeAgent. Now I definitely need KeeAgent. Issue #278 is tracking the wishlist item.

How has this been tested?

In daily use by author. If you want to test this please build with -DWITH_XC_SSHAGENT=ON and enable SSH Agent from Settings.

Not working for you?

If the entry agent control buttons are greyed out on Linux/Mac when a key is loaded the environment does not have SSH_AUTH_SOCK variable set. You need to make sure your session has an agent running so that the environment is populated before you start KeePassXC.

On Windows, you can start Pageant at any time. The buttons will light up after you re-open entry edit.

Key conversion tips

New compatible keys can be generated via the following command:

ssh-keygen -o -f id_rsa

Old keys can be converted into the new format like this (make a copy of the key first just in case):

ssh-keygen -o -p -f id_rsa

PuTTYgen can import any key (OpenSSH or PuTTY key) and export them in the new format via the Conversions menu and selecting Export OpenSSH key (force new file format).

Screenshots (if appropriate):

Types of changes

✅ New feature (non-breaking change which adds functionality)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have added tests to cover my changes.

yan12125 · 2017-10-27T06:36:03Z

Hi hifi, there's a conflict after #1124 (https://github.com/keepassxreboot/keepassxc/pull/1124/files#diff-7b98a79aea3db9f4e15f2f31fe24c087R103), could you fix that? I always apply useful patches on top of latest git.

hifi · 2017-10-27T13:41:52Z

@yan12125 Rebased.

CRCinAU · 2017-10-28T10:26:40Z

Ok, so I'm going to apologise for this in advance, but:

You sir, are a fucking legend.

mfulz · 2017-10-28T21:27:15Z

Trying to use that but I've just always the comment "unknown cipher aes256-ctr" and can't add keys to agent. Can you help me?
More Info: Sorry now I understood this: Adding the privkey as attachment and then referencing it is the intentional way, isn't it?
I was thinking I can store the pw and just decrypt the privkey.

yan12125 · 2017-10-28T21:56:41Z

@mfulz: Support for aes256-ctr is currently missing from this PR. That shouldn't be too long to come :)

As references for developers:

ssh-keygen of OpenSSH switches from aes256-cbc to aes256-ctr since version 7.6 openssh/openssh-portable@0f34553. Before that the default had always been aes256-cbc.
-Z can force another cipher name (e.g., ssh-keygen -f test-ed25519 -t ed25519 -Z aes256-cbc) It is an undocumented option, so it should be safe to assume there are only aes256-cbc and aes256-ctr keys in the wild.
Seems only a few more lines are necessary for aes256-ctr as gcrypt already supports CTR mode.

mfulz · 2017-10-28T22:03:41Z

One Bug: I'f auto adding key when db is unlocked the user confirmation is not working.

mfulz · 2017-10-28T22:10:18Z

@yan12125 Great thanks for the help, this undocumented option was the missing link. Perfect feature. Just the bug with user confirmation on db unlock is not working.

yan12125 · 2017-10-29T06:59:29Z

src/sshagent/DatabaseAgent.cpp

+            || mode == DatabaseWidget::EditMode)
+            && !m_sentKeys) {
+        for (QSharedPointer<OpenSSHKey> e : keys) {
+            AgentClient::instance()->addIdentity(*e.data());


Here lifetime and confirm should be passed, too.

Need to do a bit of refactoring there, it was written before constraints existed.

hifi · 2017-10-29T07:30:11Z

@yan12125 AES-256-CTR should work with the latest version.

hifi · 2017-10-29T08:57:40Z

@mfulz You can also use external keys with the Browse button. Like @yan12125 said it was missing decryption support for OpenSSH 7.6+ keys but that has been now sorted.

Will fix constraints not being applied on unlock later today.

hifi · 2017-10-29T12:29:19Z

@mfulz Constraints should now work if using automatic add on unlock. Thank you for testing.

mfulz · 2017-10-29T21:25:33Z

@hifi Thank you very much, I can confirm that everything is working now absolutely perfect: aes256-ctr, confirmation on auto unlock.
Great job man !!!

droidmonkey

Needs a little bit of work. Overall great job! Instead of rolling your own BinaryStream you need to use the QDataStream which will be inherently platform agnostic.

Visually, on the SSH Agent settings page, align the checkboxes with the second column (ie not the label text) to give a cleaner appearance.

droidmonkey · 2017-11-12T18:32:09Z

src/gui/entry/EditEntryWidget.cpp

 #include <QMenu>
 #include <QSortFilterProxyModel>
 #include <QTemporaryFile>
+#include <QtWidgets>


Remove this include line to prevent compile errors on Windows.

I think removing it will make Travis fail with older Qt 5.

Perhaps you are missing a more specific include then?

One of you told me on IRC to include QtWidgets instead of the specific ones when I had them in the early incarnations. Please decide.

Personally, I don't care. But if including QWidgets produces errors on Windows, this needs to be addressed.

That was throw away advice to get you going. You always want to scope down if possible, and especially if it causes issues. The ui_XXXX.h files bring in all the widget headers you need. It was likely a false positive that Travis was barfing on a widget file.

droidmonkey · 2017-11-12T18:39:28Z

src/gui/entry/EditEntryWidget.h

    bool m_create;
    bool m_history;
+#ifdef WITH_XC_SSHAGENT
+    bool m_sshAgent;


Suggest renaming this to m_sshAgentEnabled as it is named now it implies holding an object not a boolean.

droidmonkey · 2017-11-12T18:42:20Z

src/sshagent/BinaryStream.h

+#include <QIODevice>
+#include <QBuffer>
+
+class BinaryStream : QObject


Recommend removing this implementation entirely in favor of QDataStream (you can set Endianness!) http://doc.qt.io/qt-5/qdatastream.html

OpenSSH has its own buffer format, which is not compatible with Qt's. Maybe renaming BinaryStream to something like SSHBufferStream is better.

droidmonkey · 2017-11-12T18:44:47Z

src/sshagent/AgentSettingsPage.cpp

@@ -0,0 +1,57 @@
+/*
+ *  Copyright (C) 2017 Toni Spets <toni.spets@iki.fi>


All new files need to include the standard KeePassXC team copyright (in can be in addition to your own):

Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>

droidmonkey · 2017-11-12T18:48:15Z

src/sshagent/AgentSettingsWidget.cpp

+
+void AgentSettingsWidget::saveSettings()
+{
+    config()->set("SSHAgent", m_ui->enableSSHAgentCheckBox->isChecked());


At this point you should emit a signal that the SSH Agent settings have changed which would alert the system to either start or stop the ssh agent. We should never have to restart the application to enact a setting.

This will require some refactoring as yeah, it needs to send the keys out to an agent if databases are unlocked when you enable it and emulate a lock if you disable while keys have been sent that are supposed to be removed on lock.

We can leave that to a future enhancement after this merge. I'll create an issue to track.

droidmonkey · 2017-11-12T18:55:34Z

src/gui/entry/EditEntryWidgetSSHAgent.ui

+     <item>
+      <widget class="QCheckBox" name="lifetimeCheckBox">
+       <property name="text">
+        <string>Remove key from agent after</string>


It's not entirely clear when this timeout kicks in. Is it on database open? Is it on last use? Is it a timeout between uses? I think it needs to be reworded to be clear.

I disagree on the ambiguity but if someone has a good rewording that works, sure.

What's the answer though... I didn't parse the code to find out

Regardless how the key is added to the agent, the agent will auto-remove the key after said timeout. It's an agent feature and we just tell it how long it should keep the key.

droidmonkey · 2017-11-12T19:00:08Z

src/sshagent/OpenSSHKey.cpp

+    }
+
+    if (beginEx.cap(1) != "OPENSSH PRIVATE KEY") {
+        m_error = tr("This is not an OpenSSH key, only modern keys are supported");


This error is obtuse. What is meant by "modern keys"?? None of my keys loaded up and I have absolutely no idea how to fix the problem. Why isn't RSA PRIVATE KEY accepted?

I don't know what to call the new type keys. Suggestions welcome.

After researching it they are elliptic curve generated keys (ie, ed25519). Why don't you allow loading RSA keys? RSA 4096-bits is perfectly secure...

http://blog.siphos.be/2015/08/switching-openssh-to-ed25519-keys/

Do note that not all SSH servers support the new ed25519. Dropbear is a noted hold out.

It means the new "OpenSSH file format", it supports all key types and conversions are possible as stated in OP with instructions.

Gotcha, I guess I should read your description first. Either way the link to the documentation (could even be within the GUI layout, not the error string) needs to be settled for the lay user. We will get SO MANY bug reports as it is implemented now.

Would it be a link to the wiki? Is there any other reasonable way to document things like this?

Sadly, we miss a lot of documentation, it's an open issue #696

CRCinAU · 2017-11-13T05:28:58Z

May I also suggest at some point we have some documentation on how to configure this to work?

hifi · 2017-11-13T05:35:04Z

@CRCinAU It most likely is needed because it's not obvious what the new key format means and what would be the troubleshooting steps to get your agent visible on Linux/BSD.

The key format error really needs to point to some documentation how you convert your key the very least. As @droidmonkey said for the live enable/disable configuration option it could also be tracked as a separate issue to improve the UX before 2.3.0 lands.

phoerious · 2017-11-13T08:08:19Z

src/sshagent/OpenSSHKey.cpp

+    QString begin = rows.first();
+    QString end = rows.last();
+
+    QRegExp beginEx("-----BEGIN (.+)-----");


More thorough review pending, but I just saw this while reading my notifications. This should be

"^-----BEGIN ([^\\-]+)-----$"

Also use QRegularExpression instead of QRegExp. Or maybe even split after -----BEGIN and before ----- using QString::indexOf().

It's using exactMatch which implies ^...$.

Still leaves the thing in the middle.

I had it like that first but opted to use the shorter version as it doesn't matter much with a single line exact match. Sure, it bleeds extra dashes into the capture. I'll change it back.

phoerious

I found a few things that need to be changed. After that I think this is ready for merging. Most of the changes are minor nitpicks, but I also found a few potential gotchas.

I would also prefer some doc comments for the new methods, so we can generate an API documentation in the future, but that can be done in a separate PR.

phoerious · 2017-11-17T16:30:51Z

src/gui/entry/EditEntryWidget.cpp

+            continue;
+        }
+
+        m_sshAgentUi->privateKeyComboBox->addItem("Attachment: " + fileName);


Should be a translatable string.

phoerious · 2017-11-17T16:31:08Z

src/gui/entry/EditEntryWidget.cpp

+    }
+
+    if (settings.selectedType() == "attachment") {
+        m_sshAgentUi->privateKeyComboBox->setCurrentText("Attachment: " + settings.attachmentName());


phoerious · 2017-11-17T16:39:18Z

src/sshagent/SSHAgent.cpp

+    m_instance = new SSHAgent(parent);
+}
+
+bool SSHAgent::agentRunning() const


Should be named isAgentRunning()

phoerious · 2017-11-17T16:40:20Z

src/gui/entry/EditEntryWidget.cpp

+    settings.setUseLifetimeConstraintWhenAdding(m_sshAgentUi->lifetimeCheckBox->isChecked());
+    settings.setLifetimeConstraintDuration(m_sshAgentUi->lifetimeSpinBox->value());
+
+    QString prefix = "Attachment: ";


Translation

I think this is the prefix format for KeeAgent so shoudn't be translatable

It's my own convention unrelated to KeeAgent so no worries there.

If it's an hardcoded prefix you should consider something like _ATTACHMENT: , like this it really seems a translatable string

phoerious · 2017-11-17T16:40:54Z

src/gui/entry/EditEntryWidget.cpp

+    settings.setLifetimeConstraintDuration(m_sshAgentUi->lifetimeSpinBox->value());
+
+    QString prefix = "Attachment: ";
+    if (privateKeyPath.startsWith(prefix)) {


startsWith() may not work any more, once the string has been translated.

phoerious · 2017-11-17T17:06:39Z

src/sshagent/OpenSSHKey.cpp

+
+    // load private if no encryption
+    if (!encrypted()) {
+        return decrypt();


Am I missing something? Why do you decrypt if it is not encrypted?

Decryption also implies parsing the private portion regardless if there's actual encryption used. Function naming issue.

OK. Should be renamed then.

phoerious · 2017-11-17T17:25:32Z

src/sshagent/OpenSSHKey.cpp

+
+        QByteArray phraseData = passphrase.toLatin1();
+        if (bcrypt_pbkdf(phraseData, salt, decryptKey, rounds) < 0) {
+            m_error = tr("Decryption failed, wrong passphrase?");


This error message is confusing. You are deriving the decryption key here, not decrypting the data.

It happens because of an empty passphrase. Will catch that special case earlier and have a proper error message for both.

phoerious · 2017-11-17T17:27:01Z

src/sshagent/OpenSSHKey.cpp

+        keyData.setRawData(decryptKey.data(), cipher->keySize());
+        ivData.setRawData(decryptKey.data() + cipher->keySize(), cipher->blockSize());
+
+        cipher->init(keyData, ivData);


I still find it peculiar, that the OpenSSH format has no HMAC for the cipher text. PuTTY format has that.

You want me to file an issue with OpenSSH developers? 😕

No, that was just a comment on the sidelines.

phoerious · 2017-11-17T17:28:08Z

src/sshagent/OpenSSHKey.cpp

+        return false;
+    }
+
+    for (int i = 0; i < keyParts; i++) {


phoerious · 2017-11-17T17:28:36Z

src/sshagent/OpenSSHKey.cpp

+        return false;
+    }
+
+    for (int i = 0; i < keyParts; i++) {


Includes AES-256-CTR non-stream tests

This additionally makes keySize() and blockSize() work before setting the key and IV. Required for SSH agent decryption.

phoerious · 2017-11-19T12:51:59Z

Congratulations, you got merged with the Matrix!

hifi · 2017-11-19T12:52:48Z

🎉

CRCinAU · 2017-11-19T13:07:10Z

So urrrm... When are we looking at doing a new release? I'm super keen to look at this plus the yubikey fixes of a while ago...

Mogost · 2017-11-19T13:10:24Z

Great news! Waiting for release.

hifi · 2017-11-19T14:57:24Z

@CRCinAU Official daily builds are being discussed as the 2.3.0 cycle will bring in many new features that will require heavy testing before release.

CRCinAU · 2017-11-19T14:59:58Z

@hifi Does that include the new browser plugin I keep hearing about?

droidmonkey · 2017-11-19T15:24:40Z

Yes absolutely

JelteF · 2018-01-02T17:25:12Z

Oh man, thanks a lot for this! Keeagent was the only reason keeping me on Keepass (even though that crashed for me when pressing the windows key on linux). I've tried this out and it is works great so far (had to convert my key though)!

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

hifi force-pushed the feature/sshagent branch 6 times, most recently from 1caf560 to d0ea74f Compare October 25, 2017 17:10

hifi force-pushed the feature/sshagent branch from 168757a to cce6d71 Compare October 27, 2017 13:40

hifi force-pushed the feature/sshagent branch 4 times, most recently from ab208b4 to 5577ed6 Compare October 28, 2017 08:12

hifi mentioned this pull request Oct 28, 2017

SSH keys storage #278

Closed

hifi force-pushed the feature/sshagent branch from b5fd350 to 9fc45cf Compare October 28, 2017 12:10

yan12125 reviewed Oct 29, 2017

View reviewed changes

hifi force-pushed the feature/sshagent branch from 9fc45cf to d6ca747 Compare October 29, 2017 07:28

hifi force-pushed the feature/sshagent branch 2 times, most recently from 2a7b1d8 to bdad109 Compare October 29, 2017 15:21

hifi force-pushed the feature/sshagent branch 2 times, most recently from 0752358 to 9581dba Compare October 30, 2017 17:24

TheZ3ro approved these changes Nov 12, 2017

View reviewed changes

droidmonkey requested changes Nov 12, 2017

View reviewed changes

phoerious requested changes Nov 13, 2017

View reviewed changes

droidmonkey approved these changes Nov 16, 2017

View reviewed changes

hifi force-pushed the feature/sshagent branch from 2a2e12d to 8fe507b Compare November 16, 2017 16:06

phoerious requested changes Nov 17, 2017

View reviewed changes

droidmonkey mentioned this pull request Nov 19, 2017

PR code review showing all comments in same file multiple times k0shk0sh/FastHub#1269

Closed

hifi added 3 commits November 19, 2017 14:38

SymmetricCipher: Support CTR mode

a81a5fa

Includes AES-256-CTR non-stream tests

SymmetricCipher: Add keySize(), don't rely on state for sizes

248ae9d

This additionally makes keySize() and blockSize() work before setting the key and IV. Required for SSH agent decryption.

SSH agent support

4840c2c

hifi force-pushed the feature/sshagent branch from 7ea79d2 to 4840c2c Compare November 19, 2017 12:40

phoerious approved these changes Nov 19, 2017

View reviewed changes

phoerious merged commit ac73e25 into keepassxreboot:develop Nov 19, 2017

hifi mentioned this pull request Nov 19, 2017

SSH Agent: Global enable/disable requires a restart #1196

Closed

iceman73 mentioned this pull request Dec 28, 2017

Merge the Following SSH-Agent Extension varjolintu/keepassxc#18

Closed

mannp mentioned this pull request Mar 5, 2018

ssh-agent: segfaults with gnome-keyring on removing a ssh key. #1632

Closed

droidmonkey mentioned this pull request Jan 21, 2019

SSH Agent droidmonkey/keepassxc-docs#2

Closed

phoerious added pr: new feature Pull request adds a new feature and removed new feature labels Nov 22, 2024

		@@ -0,0 +1,57 @@
		/*
		* Copyright (C) 2017 Toni Spets <toni.spets@iki.fi>

Uh oh!

Conversation

hifi commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Non-goals (updated)

Motivation and context

How has this been tested?

Not working for you?

Key conversion tips

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

yan12125 commented Oct 27, 2017

Uh oh!

hifi commented Oct 27, 2017

Uh oh!

CRCinAU commented Oct 28, 2017

Uh oh!

mfulz commented Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yan12125 commented Oct 28, 2017

Uh oh!

mfulz commented Oct 28, 2017

Uh oh!

mfulz commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hifi commented Oct 29, 2017

Uh oh!

hifi commented Oct 29, 2017

Uh oh!

hifi commented Oct 29, 2017

Uh oh!

mfulz commented Oct 29, 2017

Uh oh!

droidmonkey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hifi commented Oct 21, 2017 •

edited

Loading

mfulz commented Oct 28, 2017 •

edited

Loading

droidmonkey left a comment •

edited

Loading

droidmonkey Nov 13, 2017 •

edited

Loading