SSH agent plugin does not add ssh keys consistently after each database unlock

[coalwater]

Just to be clear the keys are added successfully after the initial db unlock after the app starts. but if the database gets locked automatically or manually, the subsequent database unlocks don't add the ssh keys.

Expected Behavior

Keepassxc should add the ssh private keys after each unlock, and if the keys already exist in the ssh-agent they still get re-added ( and the life time gets refreshed, that's the behavior I'm used to )

Current Behavior

The keys are added to the agent in the initial unlock, but not after subsequent unlocks.

Steps to Reproduce

• Have a database with private keys
• Exit the keepassxc session then upgrade to the new version ( 2.4.0 )
• Delete all keys from your agent ssh-add -D
• Start keepassxc and Unlock your database
• Check the loaded keys by running ssh-add -l ( keys should exist this time )
• Lock the database ( using ctrl+l ) or ( tools > lock databases )
• Delete all keys again using ssh-add -D
• Unlock the database again
• Check if the keys exist again using ssh-add -l

The keys should exist at this point but they don't

If you close the whole app and restart it and unlock the database, the keys will be added as expected.

Debug Info

KeePassXC - Version 2.4.0
Revision: c51752d

Libraries:

• Qt 5.12.2
• libgcrypt 1.8.4

Operating system: Arch Linux
CPU architecture: x86_64
Kernel: linux 5.0.2-arch1-1-ARCH

Enabled extensions:

• Auto-Type
• Browser Integration
• SSH Agent
• KeeShare (only unsigned sharing)
• YubiKey

[hifi]

Just to be clear, this is a 2.4.0 regression from 2.3.x? Thanks.

[coalwater]

@hifi Yes, my old version was 2.3.4

[hifi]

Using the 2.4.0 snap in --devmode (how I have it installed) I can't reproduce this issue in the most simple use case.

Here's how I tested with my key that is set to automatically add on unlock and remove on lock:

1. $ watch -n 1 ssh-add -l in a terminal to monitor.
2. Open KeePassXC 2.4.0, unlock my database -> key added to agent.
3. Ctrl-L to lock -> key removed from agent.
4. Unlock by typing the passphrase -> key added to agent.
5. Ctrl-L to lock -> key removed from agent.

I tried this loop for 5 iterations and it kept consistently adding and removing my key.

Since I've been using 2.4.0 since release day like this I haven't noticed my key would have not been re-added on unlock.

[coalwater]

I have my keys set to be removed after a certain time, not when the database is locked, I don't know if that makes a difference

[DevOliv]

If i open manually my database, my keys are loaded.

If i open automatically at session openning by a launcher (bash -c "/usr/bin/secret-tool lookup keepass KeePassBdd | /usr/bin/keepassxc --pw-stdin <database_path>") they aren't. They are if i lock/unlock the database.

This config was ok in previous version.

KeePassXC - Version 2.4.0 (from official PPA)

Libraries:

• Qt 5.11.1
• libgcrypt 1.8.3

Operating system: Ubuntu 18.10
CPU architecture: x86_64
Kernel: linux 4.18.0-16-generic

Enabled extensions:

• Auto-Type
• Browser Integration
• SSH Agent
• KeeShare (signed and unsigned sharing)
• YubiKey

[hifi]

I have now reproduced both of the issues and split the --pw-stdin into #2912.

Thank you for the reports, will fix these for 2.4.1.

[hifi]

785a64cc3b3123541657831e615b0e97d3341ca5 is the first bad commit
commit 785a64cc3b3123541657831e615b0e97d3341ca5
Author: Janek Bevendorff <janek@jbev.net>
Date:   Fri Nov 23 13:49:55 2018 +0100

    Fix bugs introduced by database refactor #2491 (#2503)
    
    * Fix SSHAgent identity removal on database lock
    * Refactor storage and manipulation of SSHAgent keys to streamline process with multiple db's
    * Clear password field when widget is hidden, resolves #2502

:040000 040000 07f86848fa8d6727859128d10342ab840f6a01e4 52fb425869d62b5672acc841ac8d6e211778067d M      src

This wasn't the refactor itself but an attempted fix just after.

[hifi]

So the issue lies here:

keepassxc/src/sshagent/SSHAgent.cpp

Lines 232 to 235 in d7660da

	OpenSSHKey keyCopy = key;
	keyCopy.clearPrivate();
	m_addedKeys[keyCopy] = removeOnLock;
	return true;

Ping @phoerious, just adding if (removeOnLock) around those lines would sort of fix it but I'm afraid the way the hashmap is used throughout the agent code it would not fix it consistently.

I don't have more time to dig into this right now but this definitely is a 2.4.1 target as it's an easy to fix regression.

[xenithorb]

Also would like to note that 2.4.0 completely bypasses the routine to insert keys into the ssh-agent when invoked with --pw-stdin for some reason. Not sure if this is a separate issue

[hifi]

@xenithorb --pw-stdin was referenced in comment #2902 (comment) and it has a merged fix for 2.4.1.

[droidmonkey]

The root cause of this issue is that the structure that stores which keys have been loaded is not cleared (right or wrong) after the database is locked. The reason for this is because the setting "remove keys on database lock" is not enabled. Coupled with the setting for the agent to remove the key after a timeout, you can be left with a deadlock scenario where the key is never re-added unless KeePassXC is restarted or you do it manually. There are two options to fix this:

1. Always re-add keys marked for "add on unlock". Perhaps ignore any errors during this phase if we already added the key in the past (silent drop).

2. Disallow the combination of "don't remove key on lock" and "remove key from agent after XX time"