SSH Agent: Support old unencrypted DSA and RSA keys by hifi · Pull Request #1450 · keepassxreboot/keepassxc

hifi · 2018-02-02T18:25:45Z

Adds support for plain DSA and RSA keys in the "regular" format people actually use.

Candidate for 2.3.0 as it will prevent some support noise.

Description

Not too fond of the partial ASN.1 parser but it works well enough for these key types. Supporting encrypted keys would require importing even more code just for KDF so at this time I'm leaving it out. The integration of these keys into OpenSSHKey class is done in a way decryption is straightforward to add later.

The empty comment thing is so that the username field of an entry is used as it for these keys as the private key file does not contain a comment.

Motivation and context

SSH Agent was first introduced supporting only the "new" private key file format which does support all existing key types but the file format is not used by default except for ED25519 keys. This is very confusing for users who are moving their existing keys into KeePassXC.

How has this been tested?

New tests cover both new key types and existing tests for OpenSSHKey pass.

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.

phoerious · 2018-02-03T10:27:46Z

src/sshagent/ASN1Key.cpp

+
+    u = gcry_mpi_snew(bap.length() * 8);
+    gcry_mpi_scan(&p, GCRYMPI_FMT_HEX, bap.toHex().data(), 0, NULL);
+    gcry_mpi_scan(&q, GCRYMPI_FMT_HEX, baq.toHex().data(), 0, NULL);


Use nullptr instead of NULL

phoerious · 2018-02-03T10:29:04Z

src/sshagent/ASN1Key.h

+private:
+    static const quint8 TAG_INT        = 0x02;
+    static const quint8 TAG_SEQUENCE   = 0x30;
+    static const quint8 KEY_ZERO       = 0x0;


constexpr instead of const.

phoerious · 2018-02-03T10:29:31Z

src/sshagent/ASN1Key.h

+    static const quint8 TAG_SEQUENCE   = 0x30;
+    static const quint8 KEY_ZERO       = 0x0;
+
+    ASN1Key() { }


= default or remove entirely as the default constructor is implied.

Wouldn't an implied constructor be public? Went with = default for now.

phoerious · 2018-02-03T10:30:46Z

src/sshagent/ASN1Key.h

+#include "OpenSSHKey.h"
+#include <QtCore>
+
+class ASN1Key


I wonder if this should be a namespace, since this class has no members. Private functions can be wrapped into an anonymous namespace.

What benefits a namespace has over a class in this case? I understand it would work given the circumstances of the class.

It's more expressive. A stateless class is not a class, therefore use a namespace.
namespaces also use ADL, but that's not really important here.

Converted public facing functions to namespace and made all private members file static.

There is no reason to make those functions static if they are free functions already.

phoerious · 2018-02-03T10:36:19Z

src/sshagent/OpenSSHKey.cpp

+            return false;
+        }
+    } else {
+        m_error = tr("Unsupported key type: ") + m_privateType;


Don't concatenate strings, this must be a format expression.

phoerious · 2018-02-03T10:38:40Z

src/sshagent/OpenSSHKey.cpp

-    quint32 checkInt1;
-    quint32 checkInt2;
+        return true;
+    } else if (m_privateType == "OPENSSH PRIVATE KEY") {


These strings should be defined as static constexpr somewhere. You use them too often as literals.

phoerious · 2018-02-03T10:39:05Z

src/sshagent/OpenSSHKey.cpp

    }

-    return readPrivate(keyStream);
+    m_error = tr("Unsupported key type: ") + m_privateType;


Format expression.

phoerious · 2018-02-03T10:42:49Z

src/sshagent/ASN1Key.cpp

+    mpi_invm(u, q, p);
+
+    iqmp_hex.resize((bap.length() + 1) * 2);
+    gcry_mpi_print(GCRYMPI_FMT_HEX, reinterpret_cast<unsigned char *>(iqmp_hex.data()), iqmp_hex.length(), NULL, u);


No space between char and *, nullptr instead of NULL

yan12125

Another point: how about renaming OpenSSHKey to a more general name (e.g., SSHKey)? This class now handles more than the new OpenSSH format.

yan12125 · 2018-02-04T09:01:33Z

src/sshagent/ASN1Key.h

+#define ASN1KEY_H
+
+#include "OpenSSHKey.h"
+#include <QtCore>


As only reference types are used, how about forward declaration? For example: (not tested yet)

class OpenSSHKey; class QByteArray;

hifi · 2018-02-04T09:14:38Z

I was thinking of merging them all together later, yeah. I didn't want the code changes to be very intrusive for this PR so that it doesn't add much possibility of breaking something that has already been tested to be working (OpenSSH keys).

Decryption support is also needed for full conformance and I left it out because it would require importing code from OpenSSL for the KDF used for old key format or writing an original implementation of it and we are way too late in the "merge window" for that.

- 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]

phoerious requested changes Feb 3, 2018

View reviewed changes

phoerious added this to the v2.3.0 milestone Feb 3, 2018

phoerious added the feature: SSH agent label Feb 3, 2018

hifi force-pushed the feature/sshagent-old-keys branch 2 times, most recently from ecd2f8a to 91fe651 Compare February 4, 2018 04:56

yan12125 reviewed Feb 4, 2018

View reviewed changes

hifi force-pushed the feature/sshagent-old-keys branch from 91fe651 to 1308015 Compare February 4, 2018 13:54

phoerious approved these changes Feb 4, 2018

View reviewed changes

SSH Agent: Support old unencrypted DSA and RSA keys

cbb70cd

hifi force-pushed the feature/sshagent-old-keys branch from 1308015 to cbb70cd Compare February 4, 2018 13:59

phoerious merged commit a5993af into keepassxreboot:release/2.3.0 Feb 4, 2018

hifi mentioned this pull request Feb 13, 2018

SSH Agent: Error about unsupported (old) key format is ambiguous #1198

Closed

droidmonkey mentioned this pull request Jan 21, 2019

SSH Agent droidmonkey/keepassxc-docs#2

Closed

Uh oh!

Conversation

hifi commented Feb 2, 2018

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

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!

phoerious Feb 3, 2018 • 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!

yan12125 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hifi commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phoerious Feb 3, 2018 •

edited

Loading

hifi commented Feb 4, 2018 •

edited

Loading