- Notifications
You must be signed in to change notification settings - Fork928
refactor: PTY & SSH#7100
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.
refactor: PTY & SSH#7100
Changes fromall commits
0075d7da491d4f2cf357a28c0646872e3573f21e30e83ff6eb61057990bfe94d6e131c2c9c6efe39e8858ec3d1fdf424e650e3fecc09083e439107dc6a3229aa945460f07cb9eaabb3a50060d8File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -253,102 +253,12 @@ func (s *Server) sessionStart(session ssh.Session, extraEnv []string) (retErr er | ||
| sshPty, windowSize, isPty := session.Pty() | ||
| if isPty { | ||
| return s.startPTYSession(session, cmd, sshPty, windowSize) | ||
| } | ||
| return startNonPTYSession(session, cmd) | ||
| } | ||
| func startNonPTYSession(session ssh.Session, cmd *exec.Cmd) error { | ||
| cmd.Stdout = session | ||
| cmd.Stderr = session.Stderr() | ||
| // This blocks forever until stdin is received if we don't | ||
| @@ -368,10 +278,94 @@ func (s *Server) sessionStart(session ssh.Session, extraEnv []string) (retErr er | ||
| return cmd.Wait() | ||
| } | ||
| // ptySession is the interface to the ssh.Session that startPTYSession uses | ||
| // we use an interface here so that we can fake it in tests. | ||
| type ptySession interface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Is there a reason you chose a stripped down interface vs using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more.
I couldn't see any way to directly instantiate any of the concrete implementations in like My first stab at the test I wanted to do created the whole server like the other ssh tests do, but I found the even after calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. It's a bit annoying that I have to mock out the context methods, but unfortunately, Unfortunately the go type-checker isn't smart enough to understand that | ||
| io.ReadWriter | ||
| Context() ssh.Context | ||
| DisablePTYEmulation() | ||
| RawCommand() string | ||
| } | ||
| func (s *Server) startPTYSession(session ptySession, cmd *exec.Cmd, sshPty ssh.Pty, windowSize <-chan ssh.Window) (retErr error) { | ||
| ctx := session.Context() | ||
| // Disable minimal PTY emulation set by gliderlabs/ssh (NL-to-CRNL). | ||
| // See https://github.com/coder/coder/issues/3371. | ||
| session.DisablePTYEmulation() | ||
| if !isQuietLogin(session.RawCommand()) { | ||
| manifest := s.Manifest.Load() | ||
| if manifest != nil { | ||
| err := showMOTD(session, manifest.MOTDFile) | ||
| if err != nil { | ||
| s.logger.Error(ctx, "show MOTD", slog.Error(err)) | ||
| } | ||
| } else { | ||
| s.logger.Warn(ctx, "metadata lookup failed, unable to show MOTD") | ||
| } | ||
| } | ||
| cmd.Env = append(cmd.Env, fmt.Sprintf("TERM=%s", sshPty.Term)) | ||
| // The pty package sets `SSH_TTY` on supported platforms. | ||
| ptty, process, err := pty.Start(cmd, pty.WithPTYOption( | ||
| pty.WithSSHRequest(sshPty), | ||
| pty.WithLogger(slog.Stdlib(ctx, s.logger, slog.LevelInfo)), | ||
| )) | ||
| if err != nil { | ||
| return xerrors.Errorf("start command: %w", err) | ||
| } | ||
| defer func() { | ||
| closeErr := ptty.Close() | ||
| if closeErr != nil { | ||
| s.logger.Warn(ctx, "failed to close tty", slog.Error(closeErr)) | ||
| if retErr == nil { | ||
| retErr = closeErr | ||
| } | ||
| } | ||
| }() | ||
| go func() { | ||
| for win := range windowSize { | ||
| resizeErr := ptty.Resize(uint16(win.Height), uint16(win.Width)) | ||
| // If the pty is closed, then command has exited, no need to log. | ||
| if resizeErr != nil && !errors.Is(resizeErr, pty.ErrClosed) { | ||
| s.logger.Warn(ctx, "failed to resize tty", slog.Error(resizeErr)) | ||
| } | ||
| } | ||
| }() | ||
| go func() { | ||
| _, _ = io.Copy(ptty.InputWriter(), session) | ||
| }() | ||
| // We need to wait for the command output to finish copying. It's safe to | ||
| // just do this copy on the main handler goroutine because one of two things | ||
| // will happen: | ||
| // | ||
| // 1. The command completes & closes the TTY, which then triggers an error | ||
| // after we've Read() all the buffered data from the PTY. | ||
| // 2. The client hangs up, which cancels the command's Context, and go will | ||
| // kill the command's process. This then has the same effect as (1). | ||
| n, err := io.Copy(session, ptty.OutputReader()) | ||
| s.logger.Debug(ctx, "copy output done", slog.F("bytes", n), slog.Error(err)) | ||
| if err != nil { | ||
| return xerrors.Errorf("copy error: %w", err) | ||
| } | ||
| // We've gotten all the output, but we need to wait for the process to | ||
| // complete so that we can get the exit code. This returns | ||
| // immediately if the TTY was closed as part of the command exiting. | ||
| err = process.Wait() | ||
| var exitErr *exec.ExitError | ||
| // ExitErrors just mean the command we run returned a non-zero exit code, which is normal | ||
| // and not something to be concerned about. But, if it's something else, we should log it. | ||
| if err != nil && !xerrors.As(err, &exitErr) { | ||
| s.logger.Warn(ctx, "wait error", slog.Error(err)) | ||
| } | ||
| if err != nil { | ||
| return xerrors.Errorf("process wait: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
| func (s *Server) sftpHandler(session ssh.Session) { | ||
| ctx := session.Context() | ||
Uh oh!
There was an error while loading.Please reload this page.