Skip to content
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: Fix handling of encrypted RSA keys #1587

Merged

Conversation

hifi
Copy link
Member

@hifi hifi commented Mar 1, 2018

Fixes #1560

Description

Multiple UI issues caused by not having public key available have been fixed as well. All old RSA keys are always decrypted for display now, there's no manual decrypt option available for such keys.

The changes also handle the edge case that should not happen that such key is loaded but not decrypted.

How has this been tested?

Tested all kinds of keys in all kinds of situations by hand in the UI and automatic add/removal. Could not find any issues so far.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

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]

@louib louib added this to the 2.3.1 milestone Mar 1, 2018
@@ -94,6 +94,10 @@ int OpenSSHKey::keyLength() const

const QString OpenSSHKey::fingerprint() const
{
if (m_publicData.isEmpty()) {
return "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have to be an empty string or could you also return a null string {}?

@@ -115,6 +119,10 @@ const QString OpenSSHKey::comment() const

const QString OpenSSHKey::publicKey() const
{
if (m_publicData.isEmpty()) {
return "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

@hifi
Copy link
Member Author

hifi commented Mar 3, 2018

Changed.

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please rebase.

@hifi hifi force-pushed the hotfix/ssh-agent-enc-rsa branch from eabfacc to 5a28a40 Compare March 3, 2018 14:16
@hifi
Copy link
Member Author

hifi commented Mar 3, 2018

Done.

@phoerious
Copy link
Member

Still out of date.

Also fix multiple UI issues caused by said keys.

Fixes keepassxreboot#1560
@hifi hifi force-pushed the hotfix/ssh-agent-enc-rsa branch from 5a28a40 to 103ec9f Compare March 3, 2018 15:13
@phoerious phoerious merged commit b0a61f4 into keepassxreboot:release/2.3.1 Mar 3, 2018
phoerious added a commit that referenced this pull request Mar 6, 2018
- Fix unnecessary automatic upgrade to KDBX 4.0 and prevent challenge-response key being stripped [#1568]
- Abort saving and show an error message when challenge-response fails [#1659]
- Support inner stream protection on all string attributes [#1646]
- Fix favicon downloads not finishing on some websites [#1657]
- Fix freeze due to invalid STDIN data [#1628]
- Correct issue with encrypted RSA SSH keys [#1587]
- Fix crash on macOS due to QTBUG-54832 [#1607]
- Show error message if ssh-agent communication fails [#1614]
- Fix --pw-stdin and filename parameters being ignored [#1608]
- Fix Auto-Type syntax check not allowing spaces and special characters [#1626]
- Fix reference placeholders in combination with Auto-Type [#1649]
- Fix qtbase translations not being loaded [#1611]
- Fix startup crash on Windows due to missing SVG libraries [#1662]
- Correct database tab order regression [#1610]
- Fix GCC 8 compilation error [#1612]
- Fix copying of advanced attributes on KDE [#1640]
- Fix member initialization of CategoryListWidgetDelegate [#1613]
- Fix inconsistent toolbar icon sizes and provide higher-quality icons [#1616]
- Improve preview panel geometry [#1609]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants