ftpd: refactor TLS requirement

Signed-off-by: Nicola Murino <nicola.murino@gmail.com>

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 {

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)

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)

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)'