-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SSH agent client support (KeeAgent compatible) #1098
Conversation
1caf560
to
d0ea74f
Compare
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. |
168757a
to
cce6d71
Compare
@yan12125 Rebased. |
ab208b4
to
5577ed6
Compare
Ok, so I'm going to apologise for this in advance, but: You sir, are a fucking legend. |
b5fd350
to
9fc45cf
Compare
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? |
@mfulz: Support for aes256-ctr is currently missing from this PR. That shouldn't be too long to come :) As references for developers:
|
One Bug: I'f auto adding key when db is unlocked the user confirmation is not working. |
@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. |
src/sshagent/DatabaseAgent.cpp
Outdated
|| mode == DatabaseWidget::EditMode) | ||
&& !m_sentKeys) { | ||
for (QSharedPointer<OpenSSHKey> e : keys) { | ||
AgentClient::instance()->addIdentity(*e.data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here lifetime
and confirm
should be passed, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to do a bit of refactoring there, it was written before constraints existed.
9fc45cf
to
d6ca747
Compare
@yan12125 AES-256-CTR should work with the latest version. |
@mfulz Constraints should now work if using automatic add on unlock. Thank you for testing. |
2a7b1d8
to
bdad109
Compare
@hifi Thank you very much, I can confirm that everything is working now absolutely perfect: aes256-ctr, confirmation on auto unlock. |
0752358
to
9581dba
Compare
@hifi you are reporting errors in the commentEdit field, what about using keepassxc/src/gui/EditWidget.h Line 57 in 139658b
|
@TheZ3ro Fixed missing translation, moved to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/gui/entry/EditEntryWidget.cpp
Outdated
@@ -28,6 +29,7 @@ | |||
#include <QMenu> | |||
#include <QSortFilterProxyModel> | |||
#include <QTemporaryFile> | |||
#include <QtWidgets> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this include line to prevent compile errors on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing it will make Travis fail with older Qt 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you are missing a more specific include then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't care. But if including QWidgets produces errors on Windows, this needs to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/gui/entry/EditEntryWidget.h
Outdated
@@ -122,14 +144,20 @@ private slots: | |||
|
|||
bool m_create; | |||
bool m_history; | |||
#ifdef WITH_XC_SSHAGENT | |||
bool m_sshAgent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest renaming this to m_sshAgentEnabled
as it is named now it implies holding an object not a boolean.
#include <QIODevice> | ||
#include <QBuffer> | ||
|
||
class BinaryStream : QObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend removing this implementation entirely in favor of QDataStream (you can set Endianness!) http://doc.qt.io/qt-5/qdatastream.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenSSH has its own buffer format, which is not compatible with Qt's. Maybe renaming BinaryStream to something like SSHBufferStream is better.
@@ -0,0 +1,57 @@ | |||
/* | |||
* Copyright (C) 2017 Toni Spets <toni.spets@iki.fi> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
|
||
void AgentSettingsWidget::saveSettings() | ||
{ | ||
config()->set("SSHAgent", m_ui->enableSSHAgentCheckBox->isChecked()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave that to a future enhancement after this merge. I'll create an issue to track.
<item> | ||
<widget class="QCheckBox" name="lifetimeCheckBox"> | ||
<property name="text"> | ||
<string>Remove key from agent after</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree on the ambiguity but if someone has a good rewording that works, sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the answer though... I didn't parse the code to find out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/sshagent/OpenSSHKey.cpp
Outdated
} | ||
|
||
if (beginEx.cap(1) != "OPENSSH PRIVATE KEY") { | ||
m_error = tr("This is not an OpenSSH key, only modern keys are supported"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what to call the new type keys. Suggestions welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means the new "OpenSSH file format", it supports all key types and conversions are possible as stated in OP with instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a link to the wiki? Is there any other reasonable way to document things like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, we miss a lot of documentation, it's an open issue #696
May I also suggest at some point we have some documentation on how to configure this to work? |
@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. |
src/sshagent/OpenSSHKey.cpp
Outdated
QString begin = rows.first(); | ||
QString end = rows.last(); | ||
|
||
QRegExp beginEx("-----BEGIN (.+)-----"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's using exactMatch
which implies ^...$
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still leaves the thing in the middle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
2a2e12d
to
8fe507b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/gui/entry/EditEntryWidget.cpp
Outdated
continue; | ||
} | ||
|
||
m_sshAgentUi->privateKeyComboBox->addItem("Attachment: " + fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a translatable string.
src/gui/entry/EditEntryWidget.cpp
Outdated
} | ||
|
||
if (settings.selectedType() == "attachment") { | ||
m_sshAgentUi->privateKeyComboBox->setCurrentText("Attachment: " + settings.attachmentName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dito
src/sshagent/SSHAgent.cpp
Outdated
m_instance = new SSHAgent(parent); | ||
} | ||
|
||
bool SSHAgent::agentRunning() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be named isAgentRunning()
src/gui/entry/EditEntryWidget.cpp
Outdated
settings.setUseLifetimeConstraintWhenAdding(m_sshAgentUi->lifetimeCheckBox->isChecked()); | ||
settings.setLifetimeConstraintDuration(m_sshAgentUi->lifetimeSpinBox->value()); | ||
|
||
QString prefix = "Attachment: "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the prefix format for KeeAgent so shoudn't be translatable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's my own convention unrelated to KeeAgent so no worries there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's an hardcoded prefix you should consider something like _ATTACHMENT:
, like this it really seems a translatable string
src/gui/entry/EditEntryWidget.cpp
Outdated
settings.setLifetimeConstraintDuration(m_sshAgentUi->lifetimeSpinBox->value()); | ||
|
||
QString prefix = "Attachment: "; | ||
if (privateKeyPath.startsWith(prefix)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startsWith() may not work any more, once the string has been translated.
src/sshagent/OpenSSHKey.cpp
Outdated
|
||
// load private if no encryption | ||
if (!encrypted()) { | ||
return decrypt(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something? Why do you decrypt if it is not encrypted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decryption also implies parsing the private portion regardless if there's actual encryption used. Function naming issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Should be renamed then.
src/sshagent/OpenSSHKey.cpp
Outdated
|
||
QByteArray phraseData = passphrase.toLatin1(); | ||
if (bcrypt_pbkdf(phraseData, salt, decryptKey, rounds) < 0) { | ||
m_error = tr("Decryption failed, wrong passphrase?"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message is confusing. You are deriving the decryption key here, not decrypting the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens because of an empty passphrase. Will catch that special case earlier and have a proper error message for both.
keyData.setRawData(decryptKey.data(), cipher->keySize()); | ||
ivData.setRawData(decryptKey.data() + cipher->keySize(), cipher->blockSize()); | ||
|
||
cipher->init(keyData, ivData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find it peculiar, that the OpenSSH format has no HMAC for the cipher text. PuTTY format has that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want me to file an issue with OpenSSH developers? 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that was just a comment on the sidelines.
src/sshagent/OpenSSHKey.cpp
Outdated
return false; | ||
} | ||
|
||
for (int i = 0; i < keyParts; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++i
src/sshagent/OpenSSHKey.cpp
Outdated
return false; | ||
} | ||
|
||
for (int i = 0; i < keyParts; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++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.
7ea79d2
to
4840c2c
Compare
Congratulations, you got merged with the Matrix! |
🎉 |
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... |
Great news! Waiting for release. |
@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. |
@hifi Does that include the new browser plugin I keep hearing about? |
Yes absolutely |
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]
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
KeeAgent.settings
reading/writing for compatibility*
and&
in arguments/variablestr()
Non-goals (updated)
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:
Old keys can be converted into the new format like this (make a copy of the key first just in case):
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
Checklist: