- Notifications
You must be signed in to change notification settings - Fork928
fix(cli/ssh): prevent reads/writes to stdin/stdout in stdio mode#12045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3793256 to3be345dCompare3be345d todd1f7c4CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the commentary and naming could be improved with some inline suggestions, but LGTM.
cli/ssh_test.go Outdated
| clientOutput, clientInput := io.Pipe() | ||
| serverOutput, serverInput := io.Pipe() | ||
| monitorServerOutput, monitorServerInput := io.Pipe() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I neither know mermaid or how to turn it into ascii, so I pasted it in there 😅
flowchart LRA[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| // being sent to theserver before trying to connect. | |
| // being sent to theclient before trying to connect. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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: