From 8f079955bbfa59ca92c19a6ae70a11794c60d50f Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Wed, 12 Aug 2026 20:59:48 +0200 Subject: [PATCH] ftpd: refactor TLS requirement Signed-off-by: Nicola Murino --- internal/dataprovider/dataprovider.go | 1 + internal/ftpd/ftpd_test.go | 6 ++-- internal/ftpd/server.go | 50 ++++++++++++++++++--------- openapi/openapi.yaml | 2 +- 4 files changed, 39 insertions(+), 20 deletions(-) diff --git a/internal/dataprovider/dataprovider.go b/internal/dataprovider/dataprovider.go index 80c62702..0c13614d 100644 --- a/internal/dataprovider/dataprovider.go +++ b/internal/dataprovider/dataprovider.go @@ -1415,6 +1415,7 @@ func CheckKeyboardInteractiveAuth(username, authHook string, client ssh.Keyboard func GetFTPPreAuthUser(username, ip string) (User, error) { var user User var err error + username = config.convertName(username) if config.PreLoginHook != "" { user, err = executePreLoginHook(username, "", ip, protocolFTP, nil) } else { diff --git a/internal/ftpd/ftpd_test.go b/internal/ftpd/ftpd_test.go index b97ecbe0..ef19c2c6 100644 --- a/internal/ftpd/ftpd_test.go +++ b/internal/ftpd/ftpd_test.go @@ -3720,7 +3720,7 @@ func TestClientCertificateAuth(t *testing.T) { assert.NoError(t, err) _, err = getFTPClient(user2, true, tlsConfig) if assert.Error(t, err) { - assert.Contains(t, err.Error(), "does not match username") + assert.Contains(t, err.Error(), "invalid credentials") } // add the certs to the user user2.Filters.TLSUsername = sdk.TLSUsernameNone @@ -3739,7 +3739,7 @@ func TestClientCertificateAuth(t *testing.T) { assert.NoError(t, err) _, err = getFTPClient(user2, true, tlsConfig) if assert.Error(t, err) { - assert.Contains(t, err.Error(), "TLS certificate is not valid") + assert.Contains(t, err.Error(), "invalid credentials") } // now disable certificate authentication @@ -3943,7 +3943,7 @@ func TestPreLoginHookWithClientCert(t *testing.T) { assert.NoError(t, err) _, err = getFTPClient(u, true, tlsConfig) if assert.Error(t, err) { - assert.Contains(t, err.Error(), "does not match username") + assert.Contains(t, err.Error(), "invalid credentials") } user2, _, err := httpdtest.GetUserByUsername(tlsClient2Username, http.StatusOK) diff --git a/internal/ftpd/server.go b/internal/ftpd/server.go index 6055d49f..3728f803 100644 --- a/internal/ftpd/server.go +++ b/internal/ftpd/server.go @@ -211,23 +211,28 @@ func (s *Server) AuthUser(cc ftpserver.ClientContext, username, password string) return connection, nil } -// PreAuthUser implements the MainDriverExtensionUserVerifier interface +// PreAuthUser implements the MainDriverExtensionUserVerifier interface. +// It ends the sessions of the accounts that require TLS before the password is sent. +// The TLS requirement is applied after the authentication too, see validateUser. func (s *Server) PreAuthUser(cc ftpserver.ClientContext, username string) error { - if s.binding.TLSMode == 0 && s.tlsConfig != nil { - user, err := dataprovider.GetFTPPreAuthUser(username, util.GetIPFromRemoteAddress(cc.RemoteAddr().String())) - if err == nil { - if user.Filters.FTPSecurity == 1 { - return cc.SetTLSRequirement(ftpserver.MandatoryEncryption) - } - return nil - } - if !errors.Is(err, util.ErrNotFound) { - logger.Error(logSender, fmt.Sprintf("%v_%v_%v", common.ProtocolFTP, s.ID, cc.ID()), - "unable to get user on pre auth: %v", err) - return common.ErrInternalFailure - } + if s.binding.TLSMode != 0 || cc.HasTLSForControl() { + return nil } - return nil + ipAddr := util.GetIPFromRemoteAddress(cc.RemoteAddr().String()) + user, err := dataprovider.GetFTPPreAuthUser(username, ipAddr) + if err == nil { + if user.Filters.FTPSecurity == 1 { + return cc.SetTLSRequirement(ftpserver.MandatoryEncryption) + } + return nil + } + if errors.Is(err, util.ErrNotFound) { + common.AddDefenderEvent(ipAddr, common.ProtocolFTP, common.HostEventUserNotFound) + return nil + } + logger.Error(logSender, fmt.Sprintf("%v_%v_%v", common.ProtocolFTP, s.ID, cc.ID()), + "unable to get user on pre auth: %v", err) + return common.ErrInternalFailure } // WrapPassiveListener implements the MainDriverExtensionPassiveWrapper interface @@ -265,7 +270,9 @@ func (s *Server) VerifyConnection(cc ftpserver.ClientContext, user string, tlsCo if dbUser.IsTLSVerificationEnabled() { dbUser, err = dataprovider.CheckUserAndTLSCert(user, ipAddr, common.ProtocolFTP, state.PeerCertificates[0]) if err != nil { - return nil, err + dbUser.Username = user + updateLoginMetrics(&dbUser, ipAddr, dataprovider.LoginMethodTLSCertificate, err, nil) + return nil, dataprovider.ErrInvalidCredentials } cc.SetExtra(&tlsState{ @@ -384,6 +391,17 @@ func (s *Server) validateUser(user dataprovider.User, cc ftpserver.ClientContext user.Username, loginMethod) return nil, fmt.Errorf("login method %v is not allowed for user %q", loginMethod, user.Username) } + if user.Filters.FTPSecurity == 1 { + if !cc.HasTLSForControl() { + logger.Info(logSender, connectionID, "cannot login user %q, TLS is required", user.Username) + return nil, fmt.Errorf("TLS is required for user %q", user.Username) + } + if s.binding.TLSMode != 2 { + if err := cc.SetTLSRequirement(ftpserver.MandatoryEncryption); err != nil { + return nil, err + } + } + } if user.MustSetSecondFactorForProtocol(common.ProtocolFTP) { logger.Info(logSender, connectionID, "cannot login user %q, second factor authentication is not set", user.Username) diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index c359301f..b5899c03 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -5413,7 +5413,7 @@ components: enum: - 0 - 1 - description: 'Set to `1` to require TLS for both data and control connection. his setting is useful if you want to allow both encrypted and plain text FTP sessions globally and then you want to require encrypted sessions on a per-user basis. It has no effect if TLS is already required for all users in the configuration file.' + description: 'Set to `1` to require TLS for both data and control connection. This setting is useful to allow both encrypted and plain text FTP sessions globally and require encrypted sessions on a per-user basis. The requirement is enforced after the authentication, so it holds for accounts resolved during the login as well; cleartext sessions are closed before the password is sent when the account can be resolved beforehand. It has no effect if TLS is already required for all users in the configuration file.' is_anonymous: type: boolean description: 'If enabled the user can login with any password or no password at all. Anonymous users are supported for FTP and WebDAV protocols and permissions will be automatically set to "list" and "download" (read only)'