Skip to content

Commit

Permalink
Fixed #1048 (scp error with OpenSSH client)
Browse files Browse the repository at this point in the history
When this command is executed:

```
$ scp host:path/with/wildcards/* .
```

Teleport would launch "SSH exec" request on the sever side, which in
turn would execute the following:

```
/bin/bash -c /usr/bin/teleport scp --remote-addr=127.0.0.1:44226 --local-addr=127.0.0.1:3022 -r -f path/with/wildcards/*
```

The problem is that bash will attempt to "expand" the wildcard, sending
a bunch of files as an input into -f, but `teleport scp` needs to see
the _exact_ string as passed via scp client.

The proposed solution is to detect shell wildcard characters and wrap
them in single quotes preventing them from being expanded.

Another potential solution is to NOT use shell to execute SCP commands.
  • Loading branch information
kontsevoy committed Jun 9, 2017
1 parent 9ba5f9c commit e0360ac
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 6 deletions.
5 changes: 4 additions & 1 deletion lib/auth/clt.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ func NewClient(addr string, dialer Dialer, params ...roundtrip.ClientParam) (*Cl
roundtrip.HTTPClient(&http.Client{
Transport: transport,
}),
roundtrip.Tracer(NewTracer),
// TODO (ekontsevoy) this tracer pollutes the logs making it harder to work
// on issues that have nothing to do with the auth API, consider activating it
// via special environment variable?
// roundtrip.Tracer(NewTracer),
)

c, err := roundtrip.NewClient(addr, CurrentVersion, params...)
Expand Down
1 change: 1 addition & 0 deletions lib/config/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ func ApplyFileConfig(fc *FileConfig, cfg *service.Config) error {
cfg.CachePolicy = *cachePolicy

// TODO(klizhentas): Removed on sasha/ha?
// TODO(ekontsevoy): I would advise against it. syslog is the only logger which works with scp
if strings.ToLower(fc.Logger.Output) == "syslog" {
utils.SwitchLoggingtoSyslog()
}
Expand Down
25 changes: 22 additions & 3 deletions lib/srv/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,25 @@ type execResponse struct {
ctx *ctx
}

// quoteShellWildcards takes a slice of strings and wraps some of them
// into single quotes if they contain bash wildcard expansion characters like
// '*' or '?'
func quoteShellWildcards(args []string) []string {
retval := make([]string, len(args))
for i := range args {
a := args[i]
// check each arg for 2 things:
// - contains wildcards?
// - already wrapped in "'"?
if strings.ContainsAny(a, "*?") && !(strings.HasPrefix(a, "'") && strings.HasSuffix(a, "'")) {
retval[i] = fmt.Sprintf("'%s'", a)
} else {
retval[i] = a
}
}
return retval
}

// parseExecRequest parses SSH exec request
func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) {
var e execReq
Expand All @@ -86,7 +105,7 @@ func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) {

// is this scp request?
if f == "scp" {
// for 'scp' requests, we'll fork ourselves with scp parameters:
// for 'scp' requests, we'll launch ourselves with scp parameters:
teleportBin, err := osext.Executable()
if err != nil {
return nil, trace.Wrap(err)
Expand All @@ -95,7 +114,7 @@ func parseExecRequest(req *ssh.Request, ctx *ctx) (*execResponse, error) {
teleportBin,
ctx.conn.RemoteAddr().String(),
ctx.conn.LocalAddr().String(),
strings.Join(args[1:], " "))
strings.Join(quoteShellWildcards(args[1:]), " "))
}
}
ctx.exec = &execResponse{
Expand Down Expand Up @@ -194,7 +213,7 @@ func prepareCommand(ctx *ctx) (*exec.Cmd, error) {
}
}

// execute command using user's shell like openssh does:
// by default, execute command using user's shell like openssh does:
// https://github.com/openssh/openssh-portable/blob/master/session.c
c := exec.Command(shell, "-c", ctx.exec.cmdName)

Expand Down
13 changes: 13 additions & 0 deletions lib/srv/exec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"os/user"
"path"
"path/filepath"
"strings"

"gopkg.in/check.v1"

Expand Down Expand Up @@ -119,6 +120,18 @@ func (s *ExecSuite) TestLoginDefsParser(c *check.C) {
c.Assert(getDefaultEnvPath("bad/file"), check.Equals, "PATH="+defaultPath)
}

func (s *ExecSuite) TestShellEscaping(c *check.C) {
c.Assert(strings.Join(quoteShellWildcards([]string{"one", "two"}), " "),
check.Equals,
"one two")
c.Assert(strings.Join(quoteShellWildcards([]string{"o*ne", "two?"}), " "),
check.Equals,
"'o*ne' 'two?'")
c.Assert(strings.Join(quoteShellWildcards([]string{"'o*ne'", "'two?"}), " "),
check.Equals,
"'o*ne' ''two?'")
}

// implementation of ssh.Conn interface
func (s *ExecSuite) User() string { return s.usr.Username }
func (s *ExecSuite) SessionID() []byte { return []byte{1, 2, 3} }
Expand Down
1 change: 0 additions & 1 deletion lib/utils/websocketwriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ func (w *WebSockWrapper) Write(data []byte) (n int, err error) {
err = websocket.Message.Send(w.ws, utf8)
}
if err != nil {
log.Error(err)
n = 0
}
return n, err
Expand Down
2 changes: 1 addition & 1 deletion tool/teleport/common/teleport.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func Run(cmdlineArgs []string, testRun bool) (executedCommand string, conf *serv
// configure logger for a typical CLI scenario until configuration file is
// parsed
utils.InitLogger(utils.LoggingForDaemon, log.WarnLevel)
app := utils.InitCLIParser("teleport", "Clustered SSH service. Learn more at http://teleport.gravitational.com")
app := utils.InitCLIParser("teleport", "Clustered SSH service. Learn more at https://gravitational.com/teleport")

// define global flags:
var ccf config.CommandLineFlags
Expand Down

0 comments on commit e0360ac

Please sign in to comment.