Overhaul WindowsOpenSshPipe to use asynchronous connections

[ralish]

The existing implementation uses synchronous blocking APIs for handling client connections to the named pipe. This PR overhauls the implementation to use asynchronous non-blocking named pipes, while also fixing several issues with the implementation.

If Windows OpenSSH agent support is enabled and subsequently disabled, the agent named pipe will not be closed. This is a (minor) security vulnerability as KeeAgent will continue to serve SSH agent responses when the functionality is meant to be disabled.

There's two reasons why this happens:

• While Dispose() is called on the class, a new server is started by listenerThread(). Also, thread objects aren't garbage collected, so this won't eventually fix itself when the garbage collector runs.
• The call to WaitForConnection() is a blocking call. That extends to the Dispose() call, which will be blocked until a connection to the pipe is made. It'll then be immediately closed, so the client won't likely get any response, but per above a new named pipe is created.

A side-effect of the latter is that re-enabling the feature in the same KeePass process will never work, as the named pipe path will always be still in use. An error message to that effect is shown to the user.

Switching to asynchronous connections helps solve both of these issues. Instead of spawning threads we leverage the .NET thread pool, and the Dispose() logic has been improved to ensure the listening server will be reliably closed if the feature is disabled. Some more explicit exception handling and minimal logging for debugging purposes has also been added.

This PR fixes dlech/KeeAgent#226 and at least the error in the initial report in dlech/KeeAgent#246.

[dlech]

Thanks for the pull request. Can we add a test that tests that sockets and threads are properly shut down when the WindowsOpenSshPipe is disposed?

[dlech]

I'm a bit confused by the name AwaitConnection This method looks like it just starts listening but doesn't actually wait for anything.

[ralish]

In AwaitConnection() there's a call to BeginWaitForConnection(...) which starts the wait. The AcceptConnection(...) method is provided as a callback for when a connection is received, while the listeningServer we created immediately prior is what we're awaiting a connection to.

[dlech]

Let's drop 3d6ff37 for now. I would like to eventually apply some automatic code formatting here, but I would like to get through the backlog of pull requests first.

[ralish]

Thanks for the pull request. Can we add a test that tests that sockets and threads are properly shut down when the WindowsOpenSshPipe is disposed?

Sounds like a good idea to me, though I'm not sure when I'll have time to do so. I've been running these changes locally without issue since I created the branch.

Let's drop 3d6ff37 for now.

I've dropped all the code formatting changes in the commit except for whitespace fixes. It turns out that pretty much all the other changes were replaced in subsequent commits anyway, so the net change is minimal even were it to remain unchanged.

I would like to eventually apply some automatic code formatting here, but I would like to get through the backlog of pull requests first.

Agree this would be worthwhile, and also a good time to consider standardising on some code style guidelines. For what it's worth, I found aspects of the current style made reading the code more difficult than the average C# project for a few reasons:

• I found the 2 spaces indentation made following indentation levels hard.
• Lack of standard naming scheme for variables makes it more difficult to determine variable scoping and attributes, requiring closer inspection of the variable via its definition.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (master@e18e571). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #15   +/-   ##
=========================================
  Coverage          ?   51.01%           
=========================================
  Files             ?       40           
  Lines             ?     4765           
  Branches          ?      457           
=========================================
  Hits              ?     2431           
  Misses            ?     2175           
  Partials          ?      159           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e18e571...0dc59e5. Read the comment docs.

[dlech]

I rewrote this using async/await. Can you try https://github.com/dlech/KeeAgent/suites/5104845555/artifacts/152784220 to see if it fixes the issues you were seeing?

[ralish]

@dlech Tested the referenced artifact and all seems to work as expected.

[dlech]

Thanks for testing.

[ralish]

@dlech I'm going to have to retract the above. It definitely has problems, with the agent seemingly "dying" after a relatively short period of time (minutes). Disabling then re-enabling the OpenSSH agent functionality in KeePass temporarily resolves the problem. I'll try to find some time to look into it soon.

By the way, what was the reason for rewriting to use async/await? The original PR already used a separate thread alongside the named pipes asynchronous API, so I'm not sure what the benefit is?

[dlech]

By the way, what was the reason for rewriting to use async/await? The original PR already used a separate thread alongside the named pipes asynchronous API, so I'm not sure what the benefit is?

I have a much easier time understanding async/away code. And I wanted to fix the other socket types (e.g WslSocket) using the same pattern so I did something I already knew so I didn't have to spend so much time on it.