search

dlech / KeeAgent

ssh agent plugin for KeePass 2.x
http://lechnology.com/software/keeagent
Other
522 stars 35 forks source link

Fix #387 - getting stuck on key usage confirmation #394

Closed ExtraClock closed 5 months ago

ExtraClock commented 9 months ago

Abstract

PR is supposed to fix #387 - getting stuck on key usage confirmation. The idea is to marshal the MessageBox.Show call to the MainWindow's thread. The idea is to marshal the MessageBox.Show and KeyPicker.ShowDialog calls to a separate UI thread dedicated to the KeeAgent plugin.

Problem research

My research didn't show any confirmation on the point that we should marshal MessageBox calls to the UI thread, nor that we shouldn't. A random comment on SO says that MessageBox will spin up its own message pump, but I haven't found a confirmation in the documentation.

Impact analysis

Changes are contained in the KeeAgent library and don't leak to the SshAgentLib.

Possible issue: getting stuck in other situations due to the changes introduced. I haven't found a situation where the issue will be plausible. Here is my reasoning:

Reproducibility

I'm not sure this PR fixes the root issue instead of adding one more trick around UI-synchronization problems. The main reason why I'm not sure is because I can't really reproduce the issue.

The most "valuable" thing I brought from those unsuccessful attempts to reproduce the issue is a screenshot of stuck callstacks. Here it goes: Screenshot_32

It reproduces on my machine if I put the plugin in C:\Program Files\KeePass Password Safe 2\Plugins and launch KeePass from program files. Usual behavior: I have a confirmation window on the first SSH attempt, and on the second attempt, I get stuck.

It doesn't reproduce if I launch KeePass from KeeAgent bin\Debug. I tried to bump KeePass's version to match the version in program files - but it still doesn't reproduce.

I had some luck reproducing some delays of the MessageBox's appearance when launched from bin\Debug. I have two displays. I initiated a git push over ssh on one display, and there were no confirmation windows. When I clicked on the debugger window on the second display, the confirmation window popped up immediately. But that behavior didn't last long. I moved some windows and rearranged the setup to be able to pause the debugger without switching windows, and the behavior had gone.

Prebuilt plugin to give it a shot

If anyone wants to give it a shot and doesn't have time to build the plugin from sources, here is a prebuilt version:

ExtraClock commented 9 months ago

@dlech, there was a problem with my account (they flagged my innocent account as spammy, ha-ha). You might have missed this PR because all my activity turned out to be invisible to others for a while.

dlech commented 6 months ago
  • If the MainWindow's thread is busy, then it is either getting released any time soon or it is stuck already

I'm not sure this is a valid premise. For example, if there is a modal dialog open, the main window will be blocked indefinitely.

  • The MainWindow's thread shouldn't be doing any calls to the Pageant's worker thread, so we are on the safe side of possible dead-locks

In the past we had issues with the IOProtocolExt extension blocking the main window waiting for an SSH agent which, if it is KeeAgent, would cause a deadlock.

ExtraClock commented 5 months ago

In the past we had issues with the IOProtocolExt extension blocking the main window waiting for an SSH agent which, if it is KeeAgent, would cause a deadlock.

@dlech , I introduced a separate UI thread and marshaled the KeyPicker and key-usage-confirmation MessageBox to that thread. Now it shouldn't interlock with the IOProtocolExt plugin's activity.

dlech commented 5 months ago

Merged, thanks again!

dlech commented 5 months ago

I was going through old issue related to these dialogs and found https://github.com/dlech/KeeAgent/issues/198. So it would be a good idea to test what happens when there are multiple concurrent requests for confirmation dialog or key selection dialog.

ExtraClock commented 5 months ago

So it would be a good idea to test what happens when there are multiple concurrent requests for confirmation dialog or key selection dialog.

@dlech , LGTM. Basically, simultaneous confirmations chain one after another and there is no change in observable behavior.

  1. When both KeyPicker and UsageConfirmation options are enabled:
    • Two simultaneous KeyPicker dialogs appear.
    • The second KeyPicker dialog blocks access to the first one - you can't click on the first dialog until you respond to the second one.
    • Once the key has been chosen, a usage confirmation dialog appears. It blocks the first KeyPicker as well.
    • Once confirmation is received, the first KeyPicker gets unblocked and the workflow is as usual from this point forward.
      1. When only one of the options is enabled:
    • Two simultaneous dialogs appear.
    • The second dialog blocks access to the first one - you can't click on the first dialog until you respond to the second one.
    • Once confirmation is received, the first dialog gets unblocked and the workflow is as usual from this point forward.

I tested it on the clean 0.13.6 release and it behaves the same with an exception that sometimes it gets stuck on usage confirmation due to the bug we were trying to solve in this PR.

dlech commented 5 months ago

Thanks for testing. I guess if there is a way to get stuck, someone will let us know. But this sounds like it is good enough for now.