From ce3bbbde3f4d45072016bf58bcfe4f481905931d Mon Sep 17 00:00:00 2001 From: Ev Kontsevoy Date: Thu, 26 May 2016 17:22:34 -0700 Subject: [PATCH] SCP error messages are proper now `tsh scp` now reports the same error messages as OpenSSH scp: - when talking to Teleport servers - when talking to OpenSSH servers --- lib/client/client.go | 26 +++++++++++++++----------- lib/client/keyagent.go | 2 +- lib/srv/exec.go | 4 ---- lib/sshutils/scp/scp.go | 35 +++++++++++++++++++++++------------ 4 files changed, 39 insertions(+), 28 deletions(-) diff --git a/lib/client/client.go b/lib/client/client.go index 795bbfce8ba..30996a8c952 100644 --- a/lib/client/client.go +++ b/lib/client/client.go @@ -213,7 +213,10 @@ func (proxy *ProxyClient) ConnectToNode(nodeAddress string, user string) (*NodeC err = proxySession.RequestSubsystem("proxy:" + nodeAddress) if err != nil { defer printErrors() - return nil, trace.Wrap(err) + + parts := strings.Split(nodeAddress, "@") + siteName := parts[len(parts)-1] + return nil, trace.Errorf("Failed connecting to cluster %v: %v", siteName, err) } pipeNetConn := utils.NewPipeNetConn( proxyReader, @@ -490,15 +493,6 @@ func (client *NodeClient) scp(scpCommand scp.Command, shellCmd string, errWriter return trace.Wrap(err) } - stderr, err := session.StderrPipe() - if err != nil { - return trace.Wrap(err) - } - - go func() { - io.Copy(errWriter, stderr) - }() - ch := utils.NewPipeNetConn( stdout, stdin, @@ -507,14 +501,24 @@ func (client *NodeClient) scp(scpCommand scp.Command, shellCmd string, errWriter &net.IPAddr{}, ) + closeC := make(chan interface{}, 1) go func() { if err = scpCommand.Execute(ch); err != nil { log.Error(err) } stdin.Close() + close(closeC) }() - return trace.Wrap(session.Run(shellCmd)) + runErr := session.Run(shellCmd) + if runErr != nil && err == nil { + err = runErr + } + <-closeC + if trace.IsEOF(err) { + err = nil + } + return trace.Wrap(err) } // listenAndForward listens on a given socket and forwards all incoming connections diff --git a/lib/client/keyagent.go b/lib/client/keyagent.go index ec017d758cb..067a7f32e1b 100644 --- a/lib/client/keyagent.go +++ b/lib/client/keyagent.go @@ -101,7 +101,7 @@ func (a *LocalKeyAgent) AddHostSignersToCache(hostSigners []services.CertAuthori func (a *LocalKeyAgent) CheckHostSignature(hostId string, remote net.Addr, key ssh.PublicKey) error { // TODO (ev) remove this! // we're temporarily turning off host validation to experiment with OpenSSH servers - return nil + //return nil cert, ok := key.(*ssh.Certificate) if !ok { diff --git a/lib/srv/exec.go b/lib/srv/exec.go index 0452c646b41..104f197b5e8 100644 --- a/lib/srv/exec.go +++ b/lib/srv/exec.go @@ -58,7 +58,6 @@ type execResponse struct { cmdName string cmd *exec.Cmd ctx *ctx - isSCP bool } // parseExecRequest parses SSH exec request @@ -68,12 +67,10 @@ func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) { return nil, fmt.Errorf("failed to parse exec request, error: %v", err) } // is this scp request? - isSCP := false args := strings.Split(e.Command, " ") if len(args) > 0 { _, f := filepath.Split(args[0]) if f == "scp" { - isSCP = true // for 'scp' requests, we'll fork ourselves with scp parameters: teleportBin, err := osext.Executable() if err != nil { @@ -89,7 +86,6 @@ func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) { return &execResponse{ ctx: ctx, cmdName: e.Command, - isSCP: isSCP, }, nil } diff --git a/lib/sshutils/scp/scp.go b/lib/sshutils/scp/scp.go index 41559541eeb..5090deac49e 100644 --- a/lib/sshutils/scp/scp.go +++ b/lib/sshutils/scp/scp.go @@ -158,17 +158,18 @@ func (cmd *Command) sendFile(r *reader, ch io.ReadWriter, fi os.FileInfo, path s events.SCPAction: "read", }) } - out := fmt.Sprintf("C%04o %d %s\n", fi.Mode()&os.ModePerm, fi.Size(), fi.Name()) log.Infof("sendFile: %v", out) + _, err := io.WriteString(ch, out) if err != nil { return trace.Wrap(err) } + if err := r.read(); err != nil { return trace.Wrap(err) } - log.Infof("sendFile got OK") + f, err := os.Open(path) if err != nil { return trace.Wrap(err) @@ -185,7 +186,7 @@ func (cmd *Command) sendFile(r *reader, ch io.ReadWriter, fi os.FileInfo, path s if err := sendOK(ch); err != nil { return trace.Wrap(err) } - return r.read() + return trace.Wrap(r.read()) } // serveSink executes file uploading, when a remote server sends file(s) @@ -235,8 +236,7 @@ func (cmd *Command) processCommand(ch io.ReadWriter, st *state, b byte, line str log.Infof("<- %v %v", string(b), line) switch b { case WarnByte: - log.Warningf("got warning: %v", line) - return nil + return trace.Errorf(line) case ErrByte: return trace.Errorf(line) case 'C': @@ -244,10 +244,11 @@ func (cmd *Command) processCommand(ch io.ReadWriter, st *state, b byte, line str if err != nil { return trace.Wrap(err) } - if err := sendOK(ch); err != nil { + err = cmd.receiveFile(st, *f, ch) + if err != nil { return trace.Wrap(err) } - return cmd.receiveFile(st, *f, ch) + return nil case 'D': d, err := ParseNewFile(line) if err != nil { @@ -269,6 +270,8 @@ func (cmd *Command) processCommand(ch io.ReadWriter, st *state, b byte, line str } func (cmd *Command) receiveFile(st *state, fc NewFileCmd, ch io.ReadWriter) error { + log.Infof("scp.receiveFile(%v)", cmd.Target) + // if the dest path is a folder, we should save the file to that folder, but // only if is 'recursive' is set path := cmd.Target @@ -293,10 +296,17 @@ func (cmd *Command) receiveFile(st *state, fc NewFileCmd, ch io.ReadWriter) erro } defer f.Close() - n, err := io.CopyN(f, ch, int64(fc.Length)) - if err != nil { + + if err = sendOK(ch); err != nil { return trace.Wrap(err) } + + n, err := io.CopyN(f, ch, int64(fc.Length)) + if err != nil { + log.Error(err) + return trace.Wrap(err) + } + if n != int64(fc.Length) { return trace.Errorf("unexpected file copy length: %v", n) } @@ -437,6 +447,7 @@ func sendOK(ch io.ReadWriter) error { // sendError gets called during all errors during SCP transmission. It does // logs the error into Teleport log and also writes it back to the SCP client func sendError(ch io.ReadWriter, err error) error { + log.Error(err) if err == nil { return nil } @@ -492,6 +503,9 @@ type reader struct { r io.Reader } +// read is used to "ask" for response messages after each SCP transmission +// it only reads text data until a newline and returns 'nil' for "OK" responses +// and errors for everything else func (r *reader) read() error { n, err := r.r.Read(r.b) if err != nil { @@ -510,9 +524,6 @@ func (r *reader) read() error { if err := r.s.Err(); err != nil { return trace.Wrap(err) } - if r.b[0] == ErrByte { - return trace.Wrap(err) - } return trace.Errorf(r.s.Text()) } return trace.Errorf("unrecognized command: %#v", r.b)