From b4bc767a7c4de12c3b4e1cb2555ceb5afae18e75 Mon Sep 17 00:00:00 2001 From: Roman Tkachenko Date: Thu, 3 Jun 2021 11:37:10 -0700 Subject: [PATCH] Buddy: https://github.com/gravitational/teleport/pull/6250 (#7165) --- lib/srv/forward/sshserver.go | 6 ++- lib/srv/regular/sshserver.go | 8 +++- lib/utils/cli.go | 93 +++++++++++++++++++++++++----------- 3 files changed, 76 insertions(+), 31 deletions(-) diff --git a/lib/srv/forward/sshserver.go b/lib/srv/forward/sshserver.go index b1faa2e1cc2..3f27d3b81a5 100644 --- a/lib/srv/forward/sshserver.go +++ b/lib/srv/forward/sshserver.go @@ -1164,7 +1164,11 @@ func (s *Server) handleEnv(ch ssh.Channel, req *ssh.Request, ctx *srv.ServerCont func (s *Server) replyError(ch ssh.Channel, req *ssh.Request, err error) { s.log.Error(err) - message := utils.UserMessageFromError(err) + // Terminate the error with a newline when writing to remote channel's + // stderr so the output does not mix with the rest of the output if the remote + // side is not doing additional formatting for extended data. + // See github.com/gravitational/teleport/issues/4542 + message := utils.FormatErrorWithNewline(err) s.stderrWrite(ch, message) if req.WantReply { if err := req.Reply(false, []byte(message)); err != nil { diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go index 7e24f092f41..18483b54ea5 100644 --- a/lib/srv/regular/sshserver.go +++ b/lib/srv/regular/sshserver.go @@ -1593,7 +1593,11 @@ func (s *Server) handleProxyJump(ctx context.Context, ccx *sshutils.ConnectionCo func (s *Server) replyError(ch ssh.Channel, req *ssh.Request, err error) { log.Error(err) - message := trace.UserMessage(err) + // Terminate the error with a newline when writing to remote channel's + // stderr so the output does not mix with the rest of the output if the remote + // side is not doing additional formatting for extended data. + // See github.com/gravitational/teleport/issues/4542 + message := utils.FormatErrorWithNewline(err) writeStderr(ch, message) if req.WantReply { if err := req.Reply(false, []byte(message)); err != nil { @@ -1617,7 +1621,7 @@ func (s *Server) parseSubsystemRequest(req *ssh.Request, ctx *srv.ServerContext) } func writeStderr(ch ssh.Channel, msg string) { - if _, err := fmt.Fprint(ch.Stderr(), msg); err != nil { + if _, err := io.WriteString(ch.Stderr(), msg); err != nil { log.Warnf("Failed writing to ssh.Channel.Stderr(): %v", err) } } diff --git a/lib/utils/cli.go b/lib/utils/cli.go index c5f753ee57f..1d5fa2ba268 100644 --- a/lib/utils/cli.go +++ b/lib/utils/cli.go @@ -148,9 +148,70 @@ func GetIterations() int { return iter } -// UserMessageFromError returns user friendly error message from error +// UserMessageFromError returns user-friendly error message from error. +// The error message will be formatted for output depending on the debug +// flag func UserMessageFromError(err error) string { - // untrusted cert? + if err == nil { + return "" + } + if log.GetLevel() == log.DebugLevel { + return trace.DebugReport(err) + } + var buf bytes.Buffer + fmt.Fprint(&buf, Color(Red, "ERROR: ")) + formatErrorWriter(err, &buf) + return buf.String() +} + +// FormatErrorWithNewline returns user friendly error message from error. +// The error message is escaped if necessary. A newline is added if the error text +// does not end with a newline. +func FormatErrorWithNewline(err error) string { + message := formatError(err) + if !strings.HasSuffix(message, "\n") { + message = message + "\n" + } + return message +} + +// formatError returns user friendly error message from error. +// The error message is escaped if necessary +func formatError(err error) string { + var buf bytes.Buffer + formatErrorWriter(err, &buf) + return buf.String() +} + +// formatErrorWriter formats the specified error into the provided writer. +// The error message is escaped if necessary +func formatErrorWriter(err error, w io.Writer) { + if err == nil { + return + } + if certErr := formatCertError(err); certErr != "" { + fmt.Fprintln(w, certErr) + return + } + // If the error is a trace error, check if it has a user message embedded in + // it, if it does, print it, otherwise escape and print the original error. + if traceErr, ok := err.(*trace.TraceErr); ok { + for _, message := range traceErr.Messages { + fmt.Fprintln(w, AllowNewlines(message)) + } + fmt.Fprintln(w, AllowNewlines(trace.Unwrap(traceErr).Error())) + return + } + strErr := err.Error() + // Error can be of type trace.proxyError where error message didn't get captured. + if strErr == "" { + fmt.Fprintln(w, "please check Teleport's log for more details") + } else { + fmt.Fprintln(w, AllowNewlines(err.Error())) + } +} + +func formatCertError(err error) string { switch innerError := trace.Unwrap(err).(type) { case x509.HostnameError: return fmt.Sprintf("Cannot establish https connection to %s:\n%s\n%s\n", @@ -182,34 +243,10 @@ func UserMessageFromError(err error) string { The certificate presented by the proxy is invalid: %v. Contact your Teleport system administrator to resolve this issue.`, innerError) + default: + return "" } - if log.GetLevel() == log.DebugLevel { - return trace.DebugReport(err) - } - if err != nil { - var buf bytes.Buffer - fmt.Fprint(&buf, Color(Red, "ERROR: ")) - // If the error is a trace error, check if it has a user message embedded in - // it, if it does, print it, otherwise escape and print the original error. - if er, ok := err.(*trace.TraceErr); ok { - for _, message := range er.Messages { - fmt.Fprintln(&buf, AllowNewlines(message)) - } - fmt.Fprintln(&buf, AllowNewlines(trace.Unwrap(er).Error())) - } else { - strErr := err.Error() - // Error can be of type trace.proxyError where error message didn't get captured. - if strErr == "" { - fmt.Fprintln(&buf, "please check Teleport's log for more details") - } else { - fmt.Fprintln(&buf, AllowNewlines(err.Error())) - } - } - - return buf.String() - } - return "" } const (