diff --git a/client_handler.go b/client_handler.go index 2a622300..351fee80 100644 --- a/client_handler.go +++ b/client_handler.go @@ -3,6 +3,7 @@ package ftpserver import ( "bufio" + "errors" "fmt" "io" "net" @@ -24,6 +25,11 @@ const ( HASHAlgoSHA512 ) +var ( + errNoTrasferConnection = errors.New("unable to open transfer: no transfer connection") + errTLSRequired = errors.New("unable to open transfer: TLS is required") +) + func getHashMapping() map[string]HASHAlgo { mapping := make(map[string]HASHAlgo) mapping["CRC32"] = HASHAlgoCRC32 @@ -35,14 +41,6 @@ func getHashMapping() map[string]HASHAlgo { return mapping } -type openTransferError struct { - err string -} - -func (e *openTransferError) Error() string { - return fmt.Sprintf("Unable to open transfer: %s", e.err) -} - // nolint: maligned type clientHandler struct { id uint32 // ID of the client @@ -341,22 +339,17 @@ func (c *clientHandler) writeMessage(code int, message string) { } } -// ErrNoPassiveConnectionDeclared is defined when a transfer is openeed without any passive connection declared -// var ErrNoPassiveConnectionDeclared = errors.New("no passive connection declared") - func (c *clientHandler) TransferOpen() (net.Conn, error) { if c.transfer == nil { - err := &openTransferError{err: "No passive connection declared"} - c.writeMessage(StatusActionNotTaken, err.Error()) + c.writeMessage(StatusActionNotTaken, errNoTrasferConnection.Error()) - return nil, err + return nil, errNoTrasferConnection } if c.server.settings.TLSRequired == MandatoryEncryption && !c.transferTLS { - err := &openTransferError{err: "TLS is required"} - c.writeMessage(StatusServiceNotAvailable, err.Error()) + c.writeMessage(StatusServiceNotAvailable, errTLSRequired.Error()) - return nil, err + return nil, errTLSRequired } conn, err := c.transfer.Open() @@ -365,10 +358,9 @@ func (c *clientHandler) TransferOpen() (net.Conn, error) { "Unable to open transfer", "error", err) - err = &openTransferError{err: err.Error()} c.writeMessage(StatusCannotOpenDataConnection, err.Error()) - return conn, err + return nil, err } c.writeMessage(StatusFileStatusOK, "Using transfer connection") @@ -384,13 +376,10 @@ func (c *clientHandler) TransferOpen() (net.Conn, error) { } func (c *clientHandler) TransferClose(err error) { - if c.transfer != nil { - if err == nil { - // only send the OK status if there is no error - c.writeMessage(StatusClosingDataConn, "Closing transfer connection") - } + var errClose error - if err := c.transfer.Close(); err != nil { + if c.transfer != nil { + if errClose = c.transfer.Close(); errClose != nil { c.logger.Warn( "Problem closing transfer connection", "err", err, @@ -403,6 +392,15 @@ func (c *clientHandler) TransferClose(err error) { c.logger.Debug("Transfer connection closed") } } + + switch { + case err == nil && errClose == nil: + c.writeMessage(StatusClosingDataConn, "Closing transfer connection") + case errClose != nil: + c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Issue during transfer close: %v", errClose)) + case err != nil: + c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Issue during transfer: %v", err)) + } } func parseLine(line string) (string, string) { diff --git a/client_handler_test.go b/client_handler_test.go index 1e52e28f..5112345b 100644 --- a/client_handler_test.go +++ b/client_handler_test.go @@ -50,6 +50,28 @@ func TestLastCommand(t *testing.T) { assert.Empty(t, cc.GetLastCommand()) } +func TestTransferOpenError(t *testing.T) { + s := NewTestServer(t, true) + conf := goftp.Config{ + User: authUser, + Password: authPass, + } + + c, err := goftp.DialConfig(conf, s.Addr()) + require.NoError(t, err, "Couldn't connect") + + defer func() { panicOnError(c.Close()) }() + + raw, err := c.OpenRawConn() + require.NoError(t, err, "Couldn't open raw connection") + + // we send STOR without opening a transfer connection + rc, response, err := raw.SendCommand("STOR file") + require.NoError(t, err) + require.Equal(t, StatusActionNotTaken, rc) + require.Equal(t, "unable to open transfer: no transfer connection", response) +} + func TestTLSMethods(t *testing.T) { t.Run("without-tls", func(t *testing.T) { cc := clientHandler{ diff --git a/driver_test.go b/driver_test.go index 7be541fa..d624cc30 100644 --- a/driver_test.go +++ b/driver_test.go @@ -6,6 +6,7 @@ import ( "io" "io/ioutil" "os" + "strings" "testing" gklog "github.com/go-kit/kit/log" @@ -78,17 +79,49 @@ type TestServerDriver struct { Debug bool // To display connection logs information TLS bool - Settings *Settings // Settings - FileOverride afero.File - fs afero.Fs + Settings *Settings // Settings + fs afero.Fs } // TestClientDriver defines a minimal serverftp client driver type TestClientDriver struct { - FileOverride afero.File afero.Fs } +type testFile struct { + afero.File +} + +var errFailClose = errors.New("couldn't close") + +var errFailWrite = errors.New("couldn't write") + +var errFailSeek = errors.New("couldn't seek") + +func (f *testFile) Write(b []byte) (int, error) { + if strings.Contains(f.File.Name(), "fail-to-write") { + return 0, errFailWrite + } + + return f.File.Write(b) +} + +func (f *testFile) Close() error { + if strings.Contains(f.File.Name(), "fail-to-close") { + return errFailClose + } + + return f.File.Close() +} + +func (f *testFile) Seek(offset int64, whence int) (int64, error) { + if strings.Contains(f.File.Name(), "fail-to-seek") { + return 0, errFailSeek + } + + return f.File.Seek(offset, whence) +} + // NewTestClientDriver creates a client driver func NewTestClientDriver(server *TestServerDriver) *TestClientDriver { return &TestClientDriver{ @@ -117,10 +150,6 @@ func (driver *TestServerDriver) AuthUser(_ ClientContext, user, pass string) (Cl if user == authUser && pass == authPass { clientdriver := NewTestClientDriver(driver) - if driver.FileOverride != nil { - clientdriver.FileOverride = driver.FileOverride - } - return clientdriver, nil } @@ -156,11 +185,13 @@ func (driver *TestServerDriver) GetTLSConfig() (*tls.Config, error) { // OpenFile opens a file in 3 possible modes: read, write, appending write (use appropriate flags) func (driver *TestClientDriver) OpenFile(path string, flag int, perm os.FileMode) (afero.File, error) { - if driver.FileOverride != nil { - return driver.FileOverride, nil + file, err := driver.Fs.OpenFile(path, flag, perm) + + if err == nil { + file = &testFile{File: file} } - return driver.Fs.OpenFile(path, flag, perm) + return file, err } var errTooMuchSpaceRequested = errors.New("you're requesting too much space") diff --git a/go.mod b/go.mod index 3c9836e6..f8cf713d 100644 --- a/go.mod +++ b/go.mod @@ -10,4 +10,4 @@ require ( golang.org/x/text v0.3.4 // indirect ) -replace github.com/secsy/goftp => github.com/drakkan/goftp v0.0.0-20200916091733-843d4cca4bb2 +replace github.com/secsy/goftp => github.com/drakkan/goftp v0.0.0-20201217084041-93793d4ac54d diff --git a/go.sum b/go.sum index c75db567..a8efa5a9 100644 --- a/go.sum +++ b/go.sum @@ -40,8 +40,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= -github.com/drakkan/goftp v0.0.0-20200916091733-843d4cca4bb2 h1:aQCoXDfkaeU2/KqIvTZAcVnVmaiGdfrGF9PUnjNVjac= -github.com/drakkan/goftp v0.0.0-20200916091733-843d4cca4bb2/go.mod h1:K3yqfa64LwJzUpdUWC6b524HO7U7DmBnrJuBjxKSZOQ= +github.com/drakkan/goftp v0.0.0-20201217084041-93793d4ac54d h1:hIhohhJRpUzEuZj2qExBNzEUAxWk8+ahitt7BEFP1KU= +github.com/drakkan/goftp v0.0.0-20201217084041-93793d4ac54d/go.mod h1:K3yqfa64LwJzUpdUWC6b524HO7U7DmBnrJuBjxKSZOQ= github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk= github.com/eapache/go-resiliency v1.1.0/go.mod h1:kFI+JgMyC7bLPUVY133qvEBtVayf5mFgVsvEsIPBvNs= github.com/eapache/go-xerial-snappy v0.0.0-20180814174437-776d5712da21/go.mod h1:+020luEh2TKB4/GOp8oxxtq0Daoen/Cii55CzbTV6DU= diff --git a/handle_dirs_test.go b/handle_dirs_test.go index 6ae18e37..0b3769de 100644 --- a/handle_dirs_test.go +++ b/handle_dirs_test.go @@ -235,7 +235,7 @@ func TestTLSTransfer(t *testing.T) { rc, response, err = raw.SendCommand("MLSD /") require.NoError(t, err) require.Equal(t, StatusServiceNotAvailable, rc) - require.Equal(t, "Unable to open transfer: TLS is required", response) + require.Equal(t, "unable to open transfer: TLS is required", response) } func TestDirListingBeforeLogin(t *testing.T) { diff --git a/handle_files.go b/handle_files.go index 4b205b4e..a4ee943f 100644 --- a/handle_files.go +++ b/handle_files.go @@ -38,100 +38,101 @@ func (c *clientHandler) handleRETR() error { func (c *clientHandler) transferFile(write bool, append bool) { var file FileTransfer var err error + var fileFlag int + var filePerm os.FileMode = 0777 path := c.absPath(c.param) // We try to open the file - { - var fileFlag int - var filePerm os.FileMode = 0777 - if write { - fileFlag = os.O_WRONLY - if append { - fileFlag |= os.O_APPEND - } else { - fileFlag |= os.O_CREATE - // if this isn't a resume we add the truncate flag - // to be sure to overwrite an existing file - if c.ctxRest == 0 { - fileFlag |= os.O_TRUNC - } - } - } else { - fileFlag = os.O_RDONLY - } - if fileTransfer, ok := c.driver.(ClientDriverExtentionFileTransfer); ok { - file, err = fileTransfer.GetHandle(path, fileFlag, c.ctxRest) + if write { + fileFlag = os.O_WRONLY + if append { + fileFlag |= os.O_APPEND } else { - file, err = c.driver.OpenFile(path, fileFlag, filePerm) + fileFlag |= os.O_CREATE + // if this isn't a resume we add the truncate flag + // to be sure to overwrite an existing file + if c.ctxRest == 0 { + fileFlag |= os.O_TRUNC + } } + } else { + fileFlag = os.O_RDONLY + } - // If this fail, can stop right here and reset the seek position - if err != nil { - c.writeMessage(550, "Could not access file: "+err.Error()) - c.ctxRest = 0 - return - } + if fileTransfer, ok := c.driver.(ClientDriverExtentionFileTransfer); ok { + file, err = fileTransfer.GetHandle(path, fileFlag, c.ctxRest) + } else { + file, err = c.driver.OpenFile(path, fileFlag, filePerm) + } + + // If this fail, can stop right here and reset the seek position + if err != nil { + c.writeMessage(StatusActionNotTaken, "Could not access file: "+err.Error()) + c.ctxRest = 0 + + return } // Try to seek on it if c.ctxRest != 0 { - if _, errSeek := file.Seek(c.ctxRest, 0); errSeek != nil { - err = errSeek - } + _, err = file.Seek(c.ctxRest, 0) // Whatever happens we should reset the seek position c.ctxRest = 0 + + if err != nil { + // if we are unable to seek we can stop right here and close the file + c.writeMessage(StatusActionNotTaken, "Could not seek file: "+err.Error()) + // we can ignore the close error here + file.Close() //nolint:errcheck,gosec + + return + } } - // Start the transfer - if err == nil { - err = c.doTransfer(file, write) + tr, err := c.TransferOpen() + if err != nil { + // an error is already returned to the FTP client + // we can stop right here and close the file ignoring close error if any + file.Close() //nolint:errcheck,gosec + + return } - // *ALWAYS* close the file but only save the error if there wasn't one before - if errClose := file.Close(); errClose != nil && err == nil { + err = c.doFileTransfer(tr, file, write) + // we ignore close error for reads + if errClose := file.Close(); errClose != nil && err == nil && write { err = errClose } - if err != nil { - // if the transfer could not be open we already sent an error - if _, isOpenTransferErr := err.(*openTransferError); !isOpenTransferErr { - c.writeMessage(StatusActionNotTaken, "Could not transfer file: "+err.Error()) - return - } - } + // closing the transfer we also send the response message to the FTP client + c.TransferClose(err) } -func (c *clientHandler) doTransfer(file io.ReadWriter, write bool) error { - var tr net.Conn +func (c *clientHandler) doFileTransfer(tr net.Conn, file io.ReadWriter, write bool) error { var err error - - if tr, err = c.TransferOpen(); err == nil { - // Copy the data - var in io.Reader - var out io.Writer - - if write { // ... from the connection to the file - in = tr - out = file - } else { // ... from the file to the connection - in = file - out = tr - } - // for reads io.EOF isn't an error, for writes it must be considered an error - if written, errCopy := io.Copy(out, in); errCopy != nil && (errCopy != io.EOF || write) { - err = errCopy - } else { - c.logger.Debug( - "Stream copy finished", - "writtenBytes", written, - ) - if written == 0 { - _, err = out.Write([]byte("")) - } + var in io.Reader + var out io.Writer + + // Copy the data + if write { // ... from the connection to the file + in = tr + out = file + } else { // ... from the file to the connection + in = file + out = tr + } + // for reads io.EOF isn't an error, for writes it must be considered an error + if written, errCopy := io.Copy(out, in); errCopy != nil && (errCopy != io.EOF || write) { + err = errCopy + } else { + c.logger.Debug( + "Stream copy finished", + "writtenBytes", written, + ) + if written == 0 { + _, err = out.Write([]byte{}) } - - c.TransferClose(err) } if err != nil { @@ -164,9 +165,9 @@ func (c *clientHandler) handleCHMOD(params string) { // https://www.raidenftpd.com/en/raiden-ftpd-doc/help-sitecmd.html (wildcard isn't supported) func (c *clientHandler) handleCHOWN(params string) { - spl := strings.SplitN(params, " ", 2) + spl := strings.SplitN(params, " ", 3) - if len(spl) < 2 { + if len(spl) != 2 { c.writeMessage(StatusSyntaxErrorParameters, "bad command") return } @@ -205,9 +206,9 @@ func (c *clientHandler) handleCHOWN(params string) { // https://learn.akamai.com/en-us/webhelp/netstorage/netstorage-user-guide/ // GUID-AB301948-C6FF-4957-9291-FE3F02457FD0.html func (c *clientHandler) handleSYMLINK(params string) { - spl := strings.SplitN(params, " ", 2) + spl := strings.SplitN(params, " ", 3) - if len(spl) < 2 { + if len(spl) != 2 { c.writeMessage(StatusSyntaxErrorParameters, "bad command") return } diff --git a/handle_files_test.go b/handle_files_test.go index a1392cd3..7b7bb255 100644 --- a/handle_files_test.go +++ b/handle_files_test.go @@ -248,8 +248,25 @@ func TestSYMLINK(t *testing.T) { raw, err := c.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") - // Creating a bad clunky is authorized - rc, _, err := raw.SendCommand("SITE SYMLINK file3 file4") + // Bad syntaxes + rc, _, err := raw.SendCommand("SITE SYMLINK") + require.NoError(t, err) + require.Equal(t, StatusSyntaxErrorNotRecognised, rc, "Should have been refused") + + rc, _, err = raw.SendCommand("SITE SYMLINK ") + require.NoError(t, err) + require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + + rc, _, err = raw.SendCommand("SITE SYMLINK file1") + require.NoError(t, err) + require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + + rc, _, err = raw.SendCommand("SITE SYMLINK file1 file2 file3") + require.NoError(t, err) + require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + + // Creating a bad symlink is authorized + rc, _, err = raw.SendCommand("SITE SYMLINK file3 file4") require.NoError(t, err) require.Equal(t, StatusOK, rc, "Should have been accepted") diff --git a/transfer_test.go b/transfer_test.go index 896bd75c..e0f931ac 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -1,10 +1,10 @@ package ftpserver import ( + "bytes" "crypto/sha256" "crypto/tls" "encoding/hex" - "errors" "fmt" "io" "io/ioutil" @@ -293,7 +293,7 @@ func TestBogusTransferStart(t *testing.T) { } } -func TestFailedFileClose(t *testing.T) { +func TestFailingFileTransfer(t *testing.T) { driver := &TestServerDriver{ Debug: true, } @@ -302,22 +302,95 @@ func TestFailedFileClose(t *testing.T) { User: authUser, Password: authPass, } + + file := createTemporaryFile(t, 1*1024) + c, err := goftp.DialConfig(conf, s.Addr()) - require.NoError(t, err, "Couldn't connect") + require.NoError(t, err) - defer func() { panicOnError(c.Close()) }() + defer func() { require.NoError(t, c.Close()) }() - file := createTemporaryFile(t, 1*1024) - driver.FileOverride = &failingCloser{File: *file} - err = c.Store("file.bin", file) - require.Error(t, err, "this upload should not succeed") - require.True(t, strings.Contains(err.Error(), errFailingCloser.Error())) -} + t.Run("on write", func(t *testing.T) { + err = c.Store("fail-to-write.bin", file) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), errFailWrite.Error()), err) + }) -type failingCloser struct { - os.File + t.Run("on close", func(t *testing.T) { + err = c.Store("fail-to-close.bin", file) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), errFailClose.Error()), err) + }) + + t.Run("on seek", func(t *testing.T) { + appendFile := createTemporaryFile(t, 1*1024) + err = c.Store("fail-to-seek.bin", appendFile) + require.NoError(t, err) + err = appendFile.Close() + require.NoError(t, err) + appendFile, err = os.OpenFile(appendFile.Name(), os.O_APPEND|os.O_WRONLY, os.ModePerm) + require.NoError(t, err) + data := []byte("some more data") + _, err = io.Copy(appendFile, bytes.NewReader(data)) + require.NoError(t, err) + info, err := appendFile.Stat() + require.NoError(t, err) + require.Equal(t, int64(1024+len(data)), info.Size()) + _, err = c.TransferFromOffset("fail-to-seek.bin", nil, appendFile, 1024) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), errFailSeek.Error()), err) + }) + + t.Run("check for sync", func(t *testing.T) { + require.NoError(t, c.Store("ok", file)) + }) } -var errFailingCloser = errors.New("the hard disk crashed") +func TestTransfersFromOffset(t *testing.T) { + driver := &TestServerDriver{ + Debug: true, + } + s := NewTestServerWithDriver(t, driver) + conf := goftp.Config{ + User: authUser, + Password: authPass, + } + file := createTemporaryFile(t, 1*1024) + c, err := goftp.DialConfig(conf, s.Addr()) + require.NoError(t, err) + + defer func() { require.NoError(t, c.Close()) }() + + _, err = file.Seek(0, io.SeekStart) + require.NoError(t, err) + + err = c.Store("file", file) + require.NoError(t, err) + + _, err = file.Seek(0, io.SeekEnd) + require.NoError(t, err) + + data := []byte("some more data") + _, err = file.Write(data) + require.NoError(t, err) + + _, err = file.Seek(1024, io.SeekStart) + require.NoError(t, err) + + _, err = c.TransferFromOffset("file", nil, file, 1024) + require.NoError(t, err) + + info, err := c.Stat("file") + require.NoError(t, err) + require.Equal(t, int64(1024+len(data)), info.Size()) -func (f *failingCloser) Close() error { return errFailingCloser } + localHash := hashFile(t, file) + remoteHash := ftpDownloadAndHash(t, c, "file") + require.Equal(t, localHash, remoteHash) + + // finally test a partial RETR + buf := bytes.NewBuffer(nil) + _, err = c.TransferFromOffset("file", buf, nil, 1024) + require.NoError(t, err) + require.Equal(t, string(data), buf.String()) +}