Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 8, 2024
Merged

fix(cli/ssh): prevent reads/writes to stdin/stdout in stdio mode #12045

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd1f7c4
fix(cli/ssh): prevent stdin/stdout reads/writes in stdio mode
mafredri Feb 7, 2024
80f9ebb
improve naming and add pipe flowchart
mafredri Feb 7, 2024
6df02c1
missed some names
mafredri Feb 7, 2024
7def320
fix comment
mafredri Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
improve naming and add pipe flowchart
  • Loading branch information
@mafredri
mafredri committed Feb 7, 2024
commit 80f9ebb61dd2411cb0fb0ee46f3184f699cc6325
43 changes: 26 additions & 17 deletions cli/ssh_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,11 +362,20 @@ func TestSSH(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()

clientOutput, clientInput := io.Pipe()
serverOutput, serverInput := io.Pipe()
monitorServerOutput, monitorServerInput := io.Pipe()
clientStdinR, clientStdinW := io.Pipe()
// Here's a simple flowchart for how these pipes are used:
//
// flowchart LR
// A[ProxyCommand] --> B[captureProxyCommandStdoutW]
// B --> C[captureProxyCommandStdoutR]
// C --> VA[Validate output]
// C --> D[proxyCommandOutputW]
// D --> E[proxyCommandOutputR]
// E --> F[SSH Client]
proxyCommandOutputR, proxyCommandOutputW := io.Pipe()
captureProxyCommandStdoutR, captureProxyCommandStdoutW := io.Pipe()
closePipes := func() {
for _, c := range []io.Closer{clientOutput, clientInput, serverOutput, serverInput, monitorServerOutput, monitorServerInput} {
for _, c := range []io.Closer{clientStdinR, clientStdinW, proxyCommandOutputR, proxyCommandOutputW, captureProxyCommandStdoutR, captureProxyCommandStdoutW} {
_ = c.Close()
}
}
Expand All @@ -378,13 +387,13 @@ func TestSSH(t *testing.T) {

// Here we start a monitor for the input going to the server
// (i.e. client stdout) to ensure that the output is clean.
Copy link
Contributor

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.

Copy link
Member Author

@mafredri mafredri Feb 7, 2024 •
edited
Loading

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. 😄

serverInputBuf := make(chan byte, 4096)
proxyCommandOutputBuf := make(chan byte, 4096)
tGo(t, func() {
defer close(serverInputBuf)
defer close(proxyCommandOutputBuf)

gotHeader := false
buf := bytes.Buffer{}
r := bufio.NewReader(monitorServerOutput)
r := bufio.NewReader(captureProxyCommandStdoutR)
for {
b, err := r.ReadByte()
if err != nil {
Expand All @@ -402,7 +411,7 @@ func TestSSH(t *testing.T) {
// 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.
// being sent to the SSH client before trying to connect.
if !gotHeader {
gotHeader = true
assert.Equal(t, "SSH-2.0-Go", string(out), "invalid header")
Expand All @@ -411,18 +420,18 @@ func TestSSH(t *testing.T) {
_ = buf.WriteByte(b)
}
select {
case serverInputBuf <- b:
case proxyCommandOutputBuf <- b:
case <-ctx.Done():
return
}
}
})
tGo(t, func() {
defer serverInput.Close()
defer proxyCommandOutputW.Close()

// Range closed by above goroutine.
for b := range serverInputBuf {
_, err := serverInput.Write([]byte{b})
for b := range proxyCommandOutputBuf {
_, err := proxyCommandOutputW.Write([]byte{b})
if err != nil {
if errors.Is(err, io.ErrClosedPipe) {
return
Expand All @@ -436,8 +445,8 @@ func TestSSH(t *testing.T) {
// Start the SSH stdio command.
inv, root := clitest.New(t, "ssh", "--stdio", workspace.Name)
clitest.SetupConfig(t, client, root)
inv.Stdin = clientOutput
inv.Stdout = monitorServerInput
inv.Stdin = clientStdinR
inv.Stdout = captureProxyCommandStdoutW
inv.Stderr = io.Discard

cmdDone := tGo(t, func() {
Expand All @@ -453,8 +462,8 @@ func TestSSH(t *testing.T) {
})

conn, channels, requests, err := ssh.NewClientConn(&stdioConn{
Reader: serverOutput,
Writer: clientInput,
Reader: proxyCommandOutputR,
Writer: clientStdinW,
}, "", &ssh.ClientConfig{
// #nosec
HostKeyCallback: ssh.InsecureIgnoreHostKey(),
Expand All @@ -475,7 +484,7 @@ func TestSSH(t *testing.T) {
require.NoError(t, err)
err = sshClient.Close()
require.NoError(t, err)
_ = clientOutput.Close()
_ = clientStdinR.Close()

<-cmdDone
})
Expand Down