Add a SSH agent option to use both Pageant and OpenSSH agent simultaneously in Windows #6288
Conversation
|
Why not just use both at all times and only error if we find neither? |
|
This design gives users more choices, perhaps more secure? Maybe some windows users never use openssh agent, they may not realize that their keys have been added to the openssh agent. Although I'm not sure whether this can cause security issues. |
|
There are already bridge solutions for Windows that allow you to forward requests to another agent. There's also implementations that provide most if not all of the common ones in the same software. See https://github.com/buptczq/WinCryptSSHAgent for example for complete replacement. |
I have spent hours on WinCryptSSHAgent, but I am not possible to use it to connect remote Linux via ssh, only WSL2 working. However, both WinCryptSSHAgent and wsl-ssh-pageant are identified as malware by Windows Security application, although it is maybe a problem for Windows itself. If OpenSSH agent for Windows isn't implemented by KeepassXC, it is reasonable to use other software. Since both agent methods are already available, why not just simply provide a decision interface to users, and additional software is unneeded. |
|
I am not a fan of the UI/UX of this change, we don't use radio lists (with exception to the TOTP setup dialog). Also the option for "both" is not necessary. Two checkboxes, one for Pageant and one for OpenSSH agent would be sufficient. |
It is also a good idea, it just needs a little bit more logic control once both agent checkbox are unchecked, but "Enable SSH Agent integration" is checked. It's not complicated, i have created a new PR #6302 (comment) for such design. |
|
You should just push the changes to this PR... |
|
Thank you |
|
add a screenshot for changes
|
There was a problem hiding this comment.
In addition to the changes requested you need to change isAgentRunning to check both agents separately and return false if either of them seems unreachable. We really don't want silent failures to happen more than they already do as it makes understanding problems even harder than it currently is.
We can merge this after fixing up the raised issues and if nothing comes up in testing as the changes are quite small and there have been multiple requests for something like this so it would be a workable middle ground.
Thanks.
| @@ -120,30 +130,37 @@ bool SSHAgent::isAgentRunning() const | |||
| bool SSHAgent::sendMessage(const QByteArray& in, QByteArray& out) | |||
| { | |||
| #ifdef Q_OS_WIN | |||
| if (!useOpenSSH()) { | |||
There was a problem hiding this comment.
This section needs to be rewritten.
It would probably clean this logic up a lot if the OpenSSH stuff is moved under a new function sendMessageOpenSSH which always exists and sendMessage only has the selection logic in. You also need to change it so that if either of them return false you return false immediately without calling the other so the error is handled properly by the caller.
You can also set an error and return false here if both agents are disabled as it's a configuration error and would otherwise be a silent fail.
Additionally run the format target of CMake to make sure the code adheres to the formatting standards we have.
src/sshagent/SSHAgent.cpp
Outdated
| @@ -449,7 +466,7 @@ void SSHAgent::databaseLocked(QSharedPointer<Database> db) | |||
|
|
|||
| void SSHAgent::databaseUnlocked(QSharedPointer<Database> db) | |||
| { | |||
| if (!db || !isEnabled()) { | |||
| if (!db || !isEnabled() || (!useOpenSSH() && !usePageant())) { | |||
There was a problem hiding this comment.
This wouldn't compile on any other platform than Windows. Though you could just remove this logic completely as we can return an error later, see previous comment.
@hifi If return true for this case, the subsequent adding-key functions will be called. Though in the function |
|
If either of them return false you need to return false so the error is shown. We must not add any keys if either of them is unreachable and both enabled. |
|
I don't know why the last commit cannot build on Teamcity win10, I cannot see details, but it is fine on my win10 computer. |
|
Just ignore we are having issues with that CI machine |
|
@hifi does this need more work or is it able to be merged? |
|
|
OpenSSH is already supported in Windows 10. In some situations, I really need to use both Pageant and OpenSSH agent at the same time. Therefore, I have modified the SSH Agent setting which is able to choice to use both agent simultaneously. As the result, the SSH key can be added/ removed from both agent by KeePassXC.
Screenshots
Testing strategy
ssh-add -Loutputs nothing.ssh-add -Loutputs SSH key list.ssh-add -Loutputs SSH key list.Type of change
Possible issues
Currently, the stable OpenSSH version for Win10 is 7.7.2.1. However, I got error message
agent returned different signature type ssh-rsa (expected rsa-sha2-512).when I use ssh.exe command to connect my server. The problem can be solved by updating OpenSSH to 8.1.0.0 Beta.