From 952df50a98128a5649b311325818fabb7cef7620 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 21 Sep 2025 13:00:40 +0200 Subject: [PATCH] remove ftpserverlib fork the correct flow is to add features to the upstream library first Signed-off-by: Nicola Murino --- go.mod | 1 - go.sum | 4 +- internal/config/config.go | 12 ------ internal/config/config_test.go | 6 --- internal/ftpd/ftpd.go | 11 ----- internal/ftpd/ftpd_test.go | 74 ++-------------------------------- internal/ftpd/handler.go | 34 ++++++++++++++-- internal/ftpd/server.go | 12 +----- 8 files changed, 38 insertions(+), 116 deletions(-) diff --git a/go.mod b/go.mod index a807d01e..8e3db2df 100644 --- a/go.mod +++ b/go.mod @@ -192,7 +192,6 @@ require ( ) replace ( - github.com/fclairamb/ftpserverlib => github.com/drakkan/ftpserverlib v0.0.0-20250204143431-e069fad14727 github.com/jlaffaye/ftp => github.com/drakkan/ftp v0.0.0-20240430173938-7ba8270c8e7f github.com/robfig/cron/v3 => github.com/drakkan/cron/v3 v3.0.0-20230222140221-217a1e4d96c0 ) diff --git a/go.sum b/go.sum index dd4da8d4..d1dbe163 100644 --- a/go.sum +++ b/go.sum @@ -130,8 +130,6 @@ github.com/drakkan/cron/v3 v3.0.0-20230222140221-217a1e4d96c0 h1:EW9gIJRmt9lzk66 github.com/drakkan/cron/v3 v3.0.0-20230222140221-217a1e4d96c0/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro= github.com/drakkan/ftp v0.0.0-20240430173938-7ba8270c8e7f h1:S9JUlrOzjK58UKoLqqb40YLyVlt0bcIFtYrvnanV3zc= github.com/drakkan/ftp v0.0.0-20240430173938-7ba8270c8e7f/go.mod h1:4p8lUl4vQ80L598CygL+3IFtm+3nggvvW/palOlViwE= -github.com/drakkan/ftpserverlib v0.0.0-20250204143431-e069fad14727 h1:OwxAvQejxuEYFtuXcOxuepEjt6VPLEQ3zK+5k9p4M60= -github.com/drakkan/ftpserverlib v0.0.0-20250204143431-e069fad14727/go.mod h1:TRdVBbJEt+KihZKGl6jgSp6H/yPc0NxMUAlMZuNHcmY= github.com/drakkan/webdav v0.0.0-20241026165615-b8b8f74ae71b h1:Y1tLiQ8fnxM5f3wiBjAXsHzHNwiY9BR+mXZA75nZwrs= github.com/drakkan/webdav v0.0.0-20241026165615-b8b8f74ae71b/go.mod h1:zOVb1QDhwwqWn2L2qZ0U3swMSO4GTSNyIwXCGO/UGWE= github.com/eikenb/pipeat v0.0.0-20210730190139-06b3e6902001 h1:/ZshrfQzayqRSBDodmp3rhNCHJCff+utvgBuWRbiqu4= @@ -147,6 +145,8 @@ github.com/envoyproxy/protoc-gen-validate v1.2.1/go.mod h1:d/C80l/jxXLdfEIhX1W2T github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk= github.com/fatih/color v1.18.0 h1:S8gINlzdQ840/4pfAwic/ZE0djQEH3wM94VfqLTZcOM= github.com/fatih/color v1.18.0/go.mod h1:4FelSpRwEGDpQ12mAdzqdOukCy4u8WUtOY6lkT/6HfU= +github.com/fclairamb/ftpserverlib v0.26.0 h1:86K7qms8rrguRYQ4hxVMVl5HBSymUlqu+enjdk9Ug5A= +github.com/fclairamb/ftpserverlib v0.26.0/go.mod h1:XMm3NdvCvmBtoAVK86oERDVmoYo0GTNS5gdds4f9lpM= github.com/fclairamb/go-log v0.6.0 h1:1V7BJ75P2PvanLHRyGBBFjncB6d4AgEmu+BPWKbMkaU= github.com/fclairamb/go-log v0.6.0/go.mod h1:cyXxOw4aJwO6lrZb8GRELSw+sxO6wwkLJdsjY5xYCWA= github.com/felixge/httpsnoop v1.0.4 h1:NFTV2Zj1bL4mc9sqWACXbQFVBBg2W3GPvqp8/ESS2Wg= diff --git a/internal/config/config.go b/internal/config/config.go index 9ddcdfde..e34aabb6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1201,12 +1201,6 @@ func getFTPDBindingSecurityFromEnv(idx int, binding *ftpd.Binding) bool { isSet = true } - tlsSessionReuse, ok := lookupIntFromEnv(fmt.Sprintf("SFTPGO_FTPD__BINDINGS__%v__TLS_SESSION_REUSE", idx), 32) - if ok { - binding.TLSSessionReuse = int(tlsSessionReuse) - isSet = true - } - tlsVer, ok := lookupIntFromEnv(fmt.Sprintf("SFTPGO_FTPD__BINDINGS__%v__MIN_TLS_VERSION", idx), 32) if ok { binding.MinTLSVersion = int(tlsVer) @@ -1237,12 +1231,6 @@ func getFTPDBindingSecurityFromEnv(idx int, binding *ftpd.Binding) bool { isSet = true } - ignoreASCIITransferType, ok := lookupIntFromEnv(fmt.Sprintf("SFTPGO_FTPD__BINDINGS__%d__IGNORE_ASCII_TRANSFER_TYPE", idx), 32) - if ok { - binding.IgnoreASCIITransferType = int(ignoreASCIITransferType) - isSet = true - } - return isSet } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 094a467a..5840ca9b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -942,7 +942,6 @@ func TestFTPDBindingsFromEnv(t *testing.T) { os.Setenv("SFTPGO_FTPD__BINDINGS__0__PORT", "2200") os.Setenv("SFTPGO_FTPD__BINDINGS__0__APPLY_PROXY_CONFIG", "f") os.Setenv("SFTPGO_FTPD__BINDINGS__0__TLS_MODE", "2") - os.Setenv("SFTPGO_FTPD__BINDINGS__0__TLS_SESSION_REUSE", "1") os.Setenv("SFTPGO_FTPD__BINDINGS__0__FORCE_PASSIVE_IP", "127.0.1.2") os.Setenv("SFTPGO_FTPD__BINDINGS__0__PASSIVE_IP_OVERRIDES__0__IP", "172.16.1.1") os.Setenv("SFTPGO_FTPD__BINDINGS__0__PASSIVE_HOST", "127.0.1.3") @@ -967,7 +966,6 @@ func TestFTPDBindingsFromEnv(t *testing.T) { os.Unsetenv("SFTPGO_FTPD__BINDINGS__0__PORT") os.Unsetenv("SFTPGO_FTPD__BINDINGS__0__APPLY_PROXY_CONFIG") os.Unsetenv("SFTPGO_FTPD__BINDINGS__0__TLS_MODE") - os.Unsetenv("SFTPGO_FTPD__BINDINGS__0__TLS_SESSION_REUSE") os.Unsetenv("SFTPGO_FTPD__BINDINGS__0__FORCE_PASSIVE_IP") os.Unsetenv("SFTPGO_FTPD__BINDINGS__0__PASSIVE_IP_OVERRIDES__0__IP") os.Unsetenv("SFTPGO_FTPD__BINDINGS__0__PASSIVE_HOST") @@ -996,7 +994,6 @@ func TestFTPDBindingsFromEnv(t *testing.T) { require.Equal(t, "127.0.0.1", bindings[0].Address) require.False(t, bindings[0].ApplyProxyConfig) require.Equal(t, 2, bindings[0].TLSMode) - require.Equal(t, 1, bindings[0].TLSSessionReuse) require.Equal(t, 12, bindings[0].MinTLSVersion) require.Equal(t, "127.0.1.2", bindings[0].ForcePassiveIP) require.Len(t, bindings[0].PassiveIPOverrides, 0) @@ -1008,12 +1005,10 @@ func TestFTPDBindingsFromEnv(t *testing.T) { require.False(t, bindings[0].Debug) require.Equal(t, 1, bindings[0].PassiveConnectionsSecurity) require.Equal(t, 0, bindings[0].ActiveConnectionsSecurity) - require.Equal(t, 0, bindings[0].IgnoreASCIITransferType) require.Equal(t, 2203, bindings[1].Port) require.Equal(t, "127.0.1.1", bindings[1].Address) require.True(t, bindings[1].ApplyProxyConfig) // default value require.Equal(t, 1, bindings[1].TLSMode) - require.Equal(t, 0, bindings[1].TLSSessionReuse) require.Equal(t, 13, bindings[1].MinTLSVersion) require.Equal(t, "127.0.1.1", bindings[1].ForcePassiveIP) require.Empty(t, bindings[1].PassiveHost) @@ -1026,7 +1021,6 @@ func TestFTPDBindingsFromEnv(t *testing.T) { require.Nil(t, bindings[1].TLSCipherSuites) require.Equal(t, 0, bindings[1].PassiveConnectionsSecurity) require.Equal(t, 1, bindings[1].ActiveConnectionsSecurity) - require.Equal(t, 1, bindings[1].IgnoreASCIITransferType) require.True(t, bindings[1].Debug) require.Equal(t, "cert.crt", bindings[1].CertificateFile) require.Equal(t, "cert.key", bindings[1].CertificateKeyFile) diff --git a/internal/ftpd/ftpd.go b/internal/ftpd/ftpd.go index 80dd1531..6a3c1714 100644 --- a/internal/ftpd/ftpd.go +++ b/internal/ftpd/ftpd.go @@ -66,8 +66,6 @@ type Binding struct { // Set to 1 to require TLS for both data and control connection. // Set to 2 to enable implicit TLS TLSMode int `json:"tls_mode" mapstructure:"tls_mode"` - // 0 disabled, 1 required - TLSSessionReuse int `json:"tls_session_reuse" mapstructure:"tls_session_reuse"` // Certificate and matching private key for this specific binding, if empty the global // ones will be used, if any CertificateFile string `json:"certificate_file" mapstructure:"certificate_file"` @@ -109,11 +107,6 @@ type Binding struct { // Please note that disabling the security checks you will make the FTP service vulnerable to bounce attacks // on active data connections, so change the default value only if you are on a trusted/internal network ActiveConnectionsSecurity int `json:"active_connections_security" mapstructure:"active_connections_security"` - // Set to 1 to silently ignore any client requests to perform ASCII translations via the TYPE command. - // That is, FTP clients can request ASCII translations, and SFTPGo will respond as the client expects, - // but will not actually perform the translation for either uploads or downloads. This behavior can be - // useful in circumstances involving older/mainframe clients and EBCDIC files. - IgnoreASCIITransferType int `json:"ignore_ascii_transfer_type" mapstructure:"ignore_ascii_transfer_type"` // Debug enables the FTP debug mode. In debug mode, every FTP command will be logged Debug bool `json:"debug" mapstructure:"debug"` ciphers []uint16 @@ -141,10 +134,6 @@ func (b *Binding) isTLSModeValid() bool { return b.TLSMode >= 0 && b.TLSMode <= 2 } -func (b *Binding) isTLSSessionReuseValid() bool { - return b.TLSSessionReuse >= 0 && b.TLSSessionReuse <= 1 -} - func (b *Binding) checkSecuritySettings() error { if b.PassiveConnectionsSecurity < 0 || b.PassiveConnectionsSecurity > 1 { return fmt.Errorf("invalid passive_connections_security: %v", b.PassiveConnectionsSecurity) diff --git a/internal/ftpd/ftpd_test.go b/internal/ftpd/ftpd_test.go index 76e77de2..aebf5f37 100644 --- a/internal/ftpd/ftpd_test.go +++ b/internal/ftpd/ftpd_test.go @@ -443,7 +443,6 @@ func TestMain(m *testing.M) { //nolint:gocyclo CertificateFile: certPath, CertificateKeyFile: keyPath, TLSMode: 1, - TLSSessionReuse: 1, ClientAuthType: 2, }, } @@ -530,16 +529,6 @@ func TestInitializationFailure(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "the provided passive IP \"127001\" is not valid") ftpdConf.Bindings[1].ForcePassiveIP = "" - ftpdConf.Bindings[1].TLSMode = 2 - ftpdConf.Bindings[1].TLSSessionReuse = 1 - err = ftpdConf.Initialize(configDir) - require.Error(t, err, "TLS session resumption should not be supported with implicit FTPS") - ftpdConf.Bindings[1].TLSMode = 0 - ftpdConf.Bindings[1].TLSSessionReuse = 100 - err = ftpdConf.Initialize(configDir) - require.Error(t, err) - assert.Contains(t, err.Error(), "unsupported TLS reuse mode") - ftpdConf.Bindings[1].TLSSessionReuse = 0 err = ftpdConf.Initialize(configDir) require.Error(t, err) @@ -3165,12 +3154,12 @@ func TestMODEType(t *testing.T) { if assert.NoError(t, err) { code, response, err := client.SendCommand("MODE s") assert.NoError(t, err) - assert.Equal(t, ftp.StatusCommandOK, code) - assert.Equal(t, "Transfer mode set to 'S'", response) + assert.Equal(t, ftp.StatusNotImplementedParameter, code) + assert.Equal(t, "Unsupported mode", response) code, response, err = client.SendCommand("MODE S") assert.NoError(t, err) assert.Equal(t, ftp.StatusCommandOK, code) - assert.Equal(t, "Transfer mode set to 'S'", response) + assert.Equal(t, "Using stream mode", response) code, _, err = client.SendCommand("MODE Z") assert.NoError(t, err) @@ -3178,7 +3167,7 @@ func TestMODEType(t *testing.T) { code, _, err = client.SendCommand("MODE SS") assert.NoError(t, err) - assert.Equal(t, ftp.StatusBadArguments, code) + assert.Equal(t, ftp.StatusNotImplementedParameter, code) err = client.Quit() assert.NoError(t, err) @@ -3538,61 +3527,6 @@ func TestCombine(t *testing.T) { assert.NoError(t, err) } -func TestTLSSessionReuse(t *testing.T) { - u := getTestUser() - user, _, err := httpdtest.AddUser(u, http.StatusCreated) - assert.NoError(t, err) - - client, err := getFTPClientWithSessionReuse(user, nil) - if assert.NoError(t, err) { - err = checkBasicFTP(client) - assert.NoError(t, err) - - testFilePath := filepath.Join(homeBasePath, testFileName) - testFileSize := int64(65535) - err = createTestFile(testFilePath, testFileSize) - assert.NoError(t, err) - - err = ftpUploadFile(testFilePath, testFileName, testFileSize, client, 0) - assert.NoError(t, err) - - localDownloadPath := filepath.Join(homeBasePath, testDLFileName) - err = ftpDownloadFile(testFileName, localDownloadPath, testFileSize, client, 0) - assert.NoError(t, err) - - entries, err := client.List("/") - assert.NoError(t, err) - assert.Len(t, entries, 1) - - err = client.Quit() - assert.NoError(t, err) - err = os.Remove(testFilePath) - assert.NoError(t, err) - err = os.Remove(localDownloadPath) - assert.NoError(t, err) - } - - // this TLS config does not support session resumption - tlsConfig := &tls.Config{ - ServerName: "localhost", - InsecureSkipVerify: true, // use this for tests only - MinVersion: tls.VersionTLS12, - } - client, err = getFTPClientWithSessionReuse(user, tlsConfig) - if assert.NoError(t, err) { - err = checkBasicFTP(client) - assert.Error(t, err) - - err = client.Quit() - assert.NoError(t, err) - } - - _, err = httpdtest.RemoveUser(user, http.StatusOK) - assert.NoError(t, err) - err = os.RemoveAll(user.GetHomeDir()) - assert.NoError(t, err) -} - func TestClientCertificateAuthRevokedCert(t *testing.T) { u := getTestUser() u.Username = tlsClient2Username diff --git a/internal/ftpd/handler.go b/internal/ftpd/handler.go index 9ba113b6..628e42bf 100644 --- a/internal/ftpd/handler.go +++ b/internal/ftpd/handler.go @@ -291,7 +291,7 @@ func (c *Connection) Symlink(oldname, newname string) error { } // ReadDir implements ClientDriverExtensionFilelist -func (c *Connection) ReadDir(name string) (ftpserver.DirLister, error) { +func (c *Connection) ReadDir(name string) ([]os.FileInfo, error) { c.UpdateLastActivity() if c.doWildcardListDir { @@ -306,16 +306,21 @@ func (c *Connection) ReadDir(name string) (ftpserver.DirLister, error) { if err != nil { return nil, err } - return &patternDirLister{ + patternLister := &patternDirLister{ DirLister: lister, pattern: baseName, lastCommand: c.clientContext.GetLastCommand(), dirName: name, connectionPath: c.clientContext.Path(), - }, nil + } + return consumeDirLister(patternLister) } - return c.ListDir(name) + lister, err := c.ListDir(name) + if err != nil { + return nil, err + } + return consumeDirLister(lister) } // GetHandle implements ClientDriverExtentionFileTransfer @@ -583,3 +588,24 @@ func (l *patternDirLister) Next(limit int) ([]os.FileInfo, error) { } } } + +func consumeDirLister(lister vfs.DirLister) ([]os.FileInfo, error) { + defer lister.Close() + + var results []os.FileInfo + + for { + files, err := lister.Next(vfs.ListerBatchSize) + finished := errors.Is(err, io.EOF) + results = append(results, files...) + if err != nil && !finished { + return results, err + } + if finished { + lister.Close() + break + } + } + + return results, nil +} diff --git a/internal/ftpd/server.go b/internal/ftpd/server.go index 3593a932..7dd2e7e1 100644 --- a/internal/ftpd/server.go +++ b/internal/ftpd/server.go @@ -110,11 +110,7 @@ func (s *Server) GetSettings() (*ftpserver.Settings, error) { return nil, fmt.Errorf("unsupported TLS mode: %d", s.binding.TLSMode) } - if !s.binding.isTLSSessionReuseValid() { - return nil, fmt.Errorf("unsupported TLS reuse mode %d", s.binding.TLSSessionReuse) - } - - if (s.binding.TLSMode > 0 || s.binding.TLSSessionReuse > 0) && certMgr == nil { + if s.binding.TLSMode > 0 && certMgr == nil { return nil, errors.New("to enable TLS you need to provide a certificate") } @@ -128,13 +124,11 @@ func (s *Server) GetSettings() (*ftpserver.Settings, error) { ConnectionTimeout: 20, Banner: s.statusBanner, TLSRequired: ftpserver.TLSRequirement(s.binding.TLSMode), - TLSSessionReuse: ftpserver.TLSSessionReuse(s.binding.TLSSessionReuse), DisableSite: !s.config.EnableSite, DisableActiveMode: s.config.DisableActiveMode, EnableHASH: s.config.HASHSupport > 0, EnableCOMB: s.config.CombineSupport > 0, DefaultTransferType: ftpserver.TransferTypeBinary, - IgnoreASCIITranferType: s.binding.IgnoreASCIITransferType == 1, ActiveConnectionsCheck: ftpserver.DataConnectionRequirement(s.binding.ActiveConnectionsSecurity), PasvConnectionsCheck: ftpserver.DataConnectionRequirement(s.binding.PassiveConnectionsSecurity), }, nil @@ -292,9 +286,7 @@ func (s *Server) buildTLSConfig() { s.binding.GetAddress(), s.binding.ciphers, certID) if s.binding.isMutualTLSEnabled() { s.tlsConfig.ClientCAs = certMgr.GetRootCAs() - if s.binding.TLSSessionReuse != int(ftpserver.TLSSessionReuseRequired) { - s.tlsConfig.VerifyConnection = s.verifyTLSConnection - } + s.tlsConfig.VerifyConnection = s.verifyTLSConnection switch s.binding.ClientAuthType { case 1: s.tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert