fix(cli/ssh): prevent reads/writes to stdin/stdout in stdio mode #12045
Conversation
3793256 to
3be345d
Compare
3be345d to
dd1f7c4
Compare
cli/ssh_test.go
Outdated
|
|
||
| clientOutput, clientInput := io.Pipe() | ||
| serverOutput, serverInput := io.Pipe() | ||
| monitorServerOutput, monitorServerInput := io.Pipe() |
There was a problem hiding this comment.
it's pretty confusing what's happening with these 3 pipes, so I think an ASCII diagram would go a long way.
There was a problem hiding this comment.
I neither know mermaid or how to turn it into ascii, so I pasted it in there 😅
flowchart LR
A[ProxyCommand] --> B[captureProxyCommandStdoutW]
B --> C[captureProxyCommandStdoutR]
C --> VA[Validate output]
C --> D[proxyCommandStdoutW]
D --> E[proxyCommandStdoutR]
E --> F[SSH Client]
cli/ssh_test.go
Outdated
| }) | ||
|
|
||
| // Here we start a monitor for the input going to the server | ||
| // (i.e. client stdout) to ensure that the output is clean. |
There was a problem hiding this comment.
I think this comment is incorrect. This doesn't monitor data being sent to the server (to monitor that we'd need to be in the tailnet network path and we are not), but rather data being sent to the stdio client.
Underscores a need for a diagram about the pipes.
There was a problem hiding this comment.
The stdio client is a representation of a raw network connection, so I don't consider the statement incorrect. Lossy, perhaps.
Addendum: By "client stdout" I'm referring to the SSH command. Using "client" terminology for "stdio client" would cause more confusion IMO. Hence I prefer referring to the "stdio client" as "the connection" instead.
Yeah so after I rewrote the comment I realized what you meant, and you're right, it was incorrect, mb. 😄
cli/ssh_test.go
Outdated
| // Ideally we would do further verification, but that would | ||
| // involve parsing the SSH protocol to look for output that | ||
| // doesn't belong. This at least ensures that no garbage is | ||
| // being sent to the server before trying to connect. |
There was a problem hiding this comment.
| // being sent to the server before trying to connect. | |
| // being sent to the client before trying to connect. |
This PR prevents writes to stdout in stdio mode (assuming no developer error that uses
os.Stdout).For absolute safety, we also swap stdin with an EOF reader that logs errors on attempts to read. We can't make reads panic (or similar) since there's a high chance that such an issue could slip unnoticed into a release and trigger at runtime.
Fixes #11530
The added test, without the fix, would catch the issue at two stages: