Closed kdrag0n closed 9 months ago
Might be a dupe of #392 but with an explanation.
That would indeed explain #392 – thanks for the detailed explanation @kdrag0n and @espadolini for making that connection.
I've been mulling rewriting those bits in more modern async-await paradigms for a while but didn't have a great reason to do so, that might be a good enough one.
So this might be a little simpler than I thought – basically the issue is https://github.com/maxgoedjen/secretive/blob/main/Sources/SecretAgent/AppDelegate.swift#L34 blocks the main queue – this can actually (for the most part) be shunted off to a concurrent queue and handle two requests in parallel (tested by putting some synthetic hangs in there and running a few requests at once) – ie, the most of the existing socket code is capable of dealing with multiple connections at once. I'm still gonna play with cleaning this up a little and seeing if it's more manageable with modern concurrency.
Upon receiving a notification that a connection socket is readable, the agent reads
availableData
and assumes that the data constitutes a full request message:https://github.com/maxgoedjen/secretive/blob/0944d65ccb0d0b71a686995f7beeef4a8e9ad516/Sources/Packages/Sources/SecretAgentKit/Agent.swift#L41
However, since the OpenSSH client writes the 4-byte length and the message payload separately, there's a race condition where the agent could read only the 4-byte length header, then reject the message for being incomplete. In practice this almost never happens with a local Unix socket client, but it can happen when forwarding to a VM over TCP due to increased latency and TCP buffering semantics.
I think this is a bit complicated to fix with the current design of the SocketController, since it's stateless, single-threaded, and async. Possible solutions that I can think of:
It's not safe to use the synchronous
readDataUpToLength
with the current design because a client could block the agent thread by not sending anything after the length header. An ideal solution would read the length into a 4-byte buffer, then wait for the payload asynchronously and continue reading until the full buffer has been filled or the connection has been closed.This can be reproduced with OrbStack v0.17.1: https://github.com/orbstack/orbstack/issues/625