[BugFix] Fix valid Password Bug and refactor authentication info buil…

…d logic (#49250) Signed-off-by: HangyuanLiu <[email protected]>
StarRocks · Aug 1, 2024 · 8953fe6 · 8953fe6
1 parent 4c122d6
commit 8953fe6
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 128 deletions.
diff --git a/fe/fe-core/src/main/java/com/starrocks/authentication/AuthenticationMgr.java b/fe/fe-core/src/main/java/com/starrocks/authentication/AuthenticationMgr.java
@@ -53,7 +53,6 @@
 
 public class AuthenticationMgr {
     private static final Logger LOG = LogManager.getLogger(AuthenticationMgr.class);
-    private static final String DEFAULT_PLUGIN = PlainPasswordAuthenticationProvider.PLUGIN_NAME;
     public static final String ROOT_USER = "root";
     public static final long DEFAULT_MAX_CONNECTION_FOR_EXTERNAL_USER = 100;
 
@@ -191,10 +190,6 @@ public long getMaxConn(String userName) {
         }
     }
 
-    public String getDefaultPlugin() {
-        return DEFAULT_PLUGIN;
-    }
-
     private boolean match(String remoteUser, String remoteHost, boolean isDomain, UserAuthenticationInfo info) {
         // quickly filter unmatched entries by username
         if (!info.matchUser(remoteUser)) {

diff --git a/fe/fe-core/src/main/java/com/starrocks/authentication/AuthenticationProvider.java b/fe/fe-core/src/main/java/com/starrocks/authentication/AuthenticationProvider.java
@@ -15,7 +15,7 @@
 
 package com.starrocks.authentication;
 
-import com.starrocks.mysql.privilege.Password;
+import com.starrocks.sql.ast.UserAuthOption;
 import com.starrocks.sql.ast.UserIdentity;
 
 public interface AuthenticationProvider {
@@ -24,10 +24,8 @@ public interface AuthenticationProvider {
      * valid authentication info, and initialize the UserAuthenticationInfo structure
      * used when creating a user or modifying user's authentication information
      */
-    UserAuthenticationInfo validAuthenticationInfo(
-            UserIdentity userIdentity,
-            String password,
-            String textForAuthPlugin) throws AuthenticationException;
+    UserAuthenticationInfo analyzeAuthOption(
+            UserIdentity userIdentity, UserAuthOption userAuthOption) throws AuthenticationException;
 
     /**
      * login authentication
@@ -38,10 +36,4 @@ void authenticate(
             byte[] password,
             byte[] randomString,
             UserAuthenticationInfo authenticationInfo) throws AuthenticationException;
-
-    /**
-     * upgraded from 2.x
-     **/
-    UserAuthenticationInfo upgradedFromPassword(UserIdentity userIdentity, Password password)
-            throws AuthenticationException;
 }
diff --git a/fe/fe-core/src/main/java/com/starrocks/authentication/KerberosAuthenticationProvider.java b/fe/fe-core/src/main/java/com/starrocks/authentication/KerberosAuthenticationProvider.java
@@ -16,8 +16,8 @@
 import com.starrocks.common.Config;
 import com.starrocks.mysql.MysqlPassword;
 import com.starrocks.mysql.privilege.AuthPlugin;
-import com.starrocks.mysql.privilege.Password;
 import com.starrocks.server.GlobalStateMgr;
+import com.starrocks.sql.ast.UserAuthOption;
 import com.starrocks.sql.ast.UserIdentity;
 
 import java.lang.reflect.Method;
@@ -26,18 +26,20 @@ public class KerberosAuthenticationProvider implements AuthenticationProvider {
     public static final String PLUGIN_NAME = AuthPlugin.AUTHENTICATION_KERBEROS.name();
 
     @Override
-    public UserAuthenticationInfo validAuthenticationInfo(UserIdentity userIdentity, String password,
-                                                          String textForAuthPlugin) throws AuthenticationException {
+    public UserAuthenticationInfo analyzeAuthOption(UserIdentity userIdentity, UserAuthOption userAuthOption)
+            throws AuthenticationException {
         if (!GlobalStateMgr.getCurrentState().getAuthenticationMgr().isSupportKerberosAuth()) {
             throw new AuthenticationException("Not support kerberos authentication");
         }
 
         UserAuthenticationInfo info = new UserAuthenticationInfo();
+        info.setAuthPlugin(PLUGIN_NAME);
         info.setPassword(MysqlPassword.EMPTY_PASSWORD);
-        if (textForAuthPlugin == null) {
+        info.setOrigUserHost(userIdentity.getUser(), userIdentity.getHost());
+        if (userAuthOption == null || userAuthOption.getAuthString() == null) {
             info.setTextForAuthPlugin(Config.authentication_kerberos_service_principal.split("@")[1]);
         } else {
-            info.setTextForAuthPlugin(textForAuthPlugin);
+            info.setTextForAuthPlugin(userAuthOption.getAuthString());
         }
         return info;
     }
@@ -63,15 +65,4 @@ public void authenticate(String user, String host, byte[] password, byte[] rando
                     "by kerberos, msg: " + e.getMessage());
         }
     }
-
-    @Override
-    public UserAuthenticationInfo upgradedFromPassword(UserIdentity userIdentity, Password password)
-            throws AuthenticationException {
-        UserAuthenticationInfo ret = new UserAuthenticationInfo();
-        ret.setPassword(password.getPassword());
-        ret.setAuthPlugin(PLUGIN_NAME);
-        ret.setOrigUserHost(userIdentity.getUser(), userIdentity.getHost());
-        ret.setTextForAuthPlugin(password.getUserForAuthPlugin());
-        return ret;
-    }
 }
diff --git a/fe/fe-core/src/main/java/com/starrocks/authentication/LDAPAuthProviderForNative.java b/fe/fe-core/src/main/java/com/starrocks/authentication/LDAPAuthProviderForNative.java
@@ -16,8 +16,8 @@
 import com.google.common.base.Strings;
 import com.starrocks.mysql.MysqlPassword;
 import com.starrocks.mysql.privilege.AuthPlugin;
-import com.starrocks.mysql.privilege.Password;
 import com.starrocks.mysql.security.LdapSecurity;
+import com.starrocks.sql.ast.UserAuthOption;
 import com.starrocks.sql.ast.UserIdentity;
 
 import java.nio.charset.StandardCharsets;
@@ -27,11 +27,13 @@ public class LDAPAuthProviderForNative implements AuthenticationProvider {
     public static final String PLUGIN_NAME = AuthPlugin.AUTHENTICATION_LDAP_SIMPLE.name();
 
     @Override
-    public UserAuthenticationInfo validAuthenticationInfo(UserIdentity userIdentity,
-                                                          String password, String textForAuthPlugin) {
+    public UserAuthenticationInfo analyzeAuthOption(UserIdentity userIdentity, UserAuthOption userAuthOption)
+            throws AuthenticationException {
         UserAuthenticationInfo info = new UserAuthenticationInfo();
+        info.setAuthPlugin(PLUGIN_NAME);
         info.setPassword(MysqlPassword.EMPTY_PASSWORD);
-        info.setTextForAuthPlugin(textForAuthPlugin);
+        info.setOrigUserHost(userIdentity.getUser(), userIdentity.getHost());
+        info.setTextForAuthPlugin(userAuthOption == null ? null : userAuthOption.getAuthString());
         return info;
     }
 
@@ -54,15 +56,4 @@ public void authenticate(String user, String host, byte[] remotePassword, byte[]
             }
         }
     }
-
-    @Override
-    public UserAuthenticationInfo upgradedFromPassword(UserIdentity userIdentity, Password password)
-            throws AuthenticationException {
-        UserAuthenticationInfo ret = new UserAuthenticationInfo();
-        ret.setPassword(password.getPassword());
-        ret.setAuthPlugin(PLUGIN_NAME);
-        ret.setOrigUserHost(userIdentity.getUser(), userIdentity.getHost());
-        ret.setTextForAuthPlugin(password.getUserForAuthPlugin());
-        return ret;
-    }
 }
diff --git a/...-core/src/main/java/com/starrocks/authentication/PlainPasswordAuthenticationProvider.java b/...-core/src/main/java/com/starrocks/authentication/PlainPasswordAuthenticationProvider.java
@@ -15,9 +15,11 @@
 
 package com.starrocks.authentication;
 
+import com.google.common.base.Strings;
 import com.starrocks.common.Config;
 import com.starrocks.mysql.MysqlPassword;
-import com.starrocks.mysql.privilege.Password;
+import com.starrocks.server.GlobalStateMgr;
+import com.starrocks.sql.ast.UserAuthOption;
 import com.starrocks.sql.ast.UserIdentity;
 
 import java.nio.charset.StandardCharsets;
@@ -30,7 +32,7 @@ public class PlainPasswordAuthenticationProvider implements AuthenticationProvid
      * <p>
      * The rules are hard-coded for temporary, will change to a plugin config later
      **/
-    protected void validatePassword(String password) throws AuthenticationException {
+    protected void validatePassword(UserIdentity userIdentity, String password) throws AuthenticationException {
         if (!Config.enable_validate_password) {
             return;
         }
@@ -58,17 +60,32 @@ protected void validatePassword(String password) throws AuthenticationException
             throw new AuthenticationException(
                     "password should contains at least one digit, one lowercase letter and one uppercase letter!");
         }
+
+        if (!Config.enable_password_reuse) {
+            GlobalStateMgr.getCurrentState().getAuthenticationMgr().checkPlainPassword(
+                    userIdentity.getUser(), userIdentity.getHost(), password);
+        }
     }
 
     @Override
-    public UserAuthenticationInfo validAuthenticationInfo(
-            UserIdentity userIdentity,
-            String password,
-            String textForAuthPlugin) throws AuthenticationException {
-        validatePassword(password);
+    public UserAuthenticationInfo analyzeAuthOption(UserIdentity userIdentity, UserAuthOption userAuthOption)
+            throws AuthenticationException {
+        byte[] passwordScrambled = MysqlPassword.EMPTY_PASSWORD;
+        if (userAuthOption != null) {
+            boolean isPasswordPlain = userAuthOption.isPasswordPlain();
+            String password = userAuthOption.getAuthPlugin() == null ?
+                    userAuthOption.getPassword() : userAuthOption.getAuthString();
+            if (isPasswordPlain) {
+                validatePassword(userIdentity, password);
+            }
+            passwordScrambled = scramblePassword(password, isPasswordPlain);
+        }
+
         UserAuthenticationInfo info = new UserAuthenticationInfo();
-        info.setPassword(password.getBytes(StandardCharsets.UTF_8));
-        info.setTextForAuthPlugin(textForAuthPlugin);
+        info.setAuthPlugin(PLUGIN_NAME);
+        info.setPassword(passwordScrambled);
+        info.setOrigUserHost(userIdentity.getUser(), userIdentity.getHost());
+        info.setTextForAuthPlugin(userAuthOption == null ? null : userAuthOption.getAuthString());
         return info;
     }
 
@@ -101,14 +118,17 @@ public void authenticate(
         }
     }
 
-    @Override
-    public UserAuthenticationInfo upgradedFromPassword(UserIdentity userIdentity, Password password)
-            throws AuthenticationException {
-        UserAuthenticationInfo ret = new UserAuthenticationInfo();
-        ret.setPassword(password.getPassword() == null ? MysqlPassword.EMPTY_PASSWORD : password.getPassword());
-        ret.setAuthPlugin(PLUGIN_NAME);
-        ret.setOrigUserHost(userIdentity.getUser(), userIdentity.getHost());
-        ret.setTextForAuthPlugin(password.getUserForAuthPlugin());
-        return ret;
+    /**
+     * Get scrambled password from plain password
+     */
+    private byte[] scramblePassword(String originalPassword, boolean isPasswordPlain) {
+        if (Strings.isNullOrEmpty(originalPassword)) {
+            return MysqlPassword.EMPTY_PASSWORD;
+        }
+        if (isPasswordPlain) {
+            return MysqlPassword.makeScrambledPassword(originalPassword);
+        } else {
+            return MysqlPassword.checkPassword(originalPassword);
+        }
     }
 }
diff --git a/fe/fe-core/src/main/java/com/starrocks/sql/analyzer/AuthenticationAnalyzer.java b/fe/fe-core/src/main/java/com/starrocks/sql/analyzer/AuthenticationAnalyzer.java
@@ -13,15 +13,13 @@
 // limitations under the License.
 package com.starrocks.sql.analyzer;
 
-import com.google.common.base.Strings;
 import com.starrocks.authentication.AuthenticationException;
 import com.starrocks.authentication.AuthenticationMgr;
 import com.starrocks.authentication.AuthenticationProvider;
 import com.starrocks.authentication.AuthenticationProviderFactory;
 import com.starrocks.authentication.PlainPasswordAuthenticationProvider;
 import com.starrocks.authentication.UserAuthenticationInfo;
 import com.starrocks.common.Config;
-import com.starrocks.mysql.MysqlPassword;
 import com.starrocks.privilege.AuthorizationMgr;
 import com.starrocks.qe.ConnectContext;
 import com.starrocks.server.GlobalStateMgr;
@@ -35,8 +33,6 @@
 import com.starrocks.sql.ast.UserAuthOption;
 import com.starrocks.sql.ast.UserIdentity;
 
-import java.nio.charset.StandardCharsets;
-
 public class AuthenticationAnalyzer {
     public static void analyze(StatementBase statement, ConnectContext session) {
         new AuthenticationAnalyzerVisitor().analyze(statement, session);
@@ -76,20 +72,6 @@ private void validRoleName(String roleName, String errMsg, boolean checkExist) {
             }
         }
 
-        /**
-         * Get scrambled password from plain password
-         */
-        private byte[] analysePassword(String originalPassword, boolean isPasswordPlain) {
-            if (Strings.isNullOrEmpty(originalPassword)) {
-                return MysqlPassword.EMPTY_PASSWORD;
-            }
-            if (isPasswordPlain) {
-                return MysqlPassword.makeScrambledPassword(originalPassword);
-            } else {
-                return MysqlPassword.checkPassword(originalPassword);
-            }
-        }
-
         @Override
         public Void visitCreateUserStatement(CreateUserStmt stmt, ConnectContext context) {
             stmt.getUserIdentity().analyze();
@@ -116,42 +98,20 @@ public Void visitAlterUserStatement(AlterUserStmt stmt, ConnectContext context)
 
             UserAuthenticationInfo userAuthenticationInfo = analyzeAuthOption(stmt.getUserIdentity(), stmt.getAuthOption());
             stmt.setAuthenticationInfo(userAuthenticationInfo);
-
-            if (!Config.enable_password_reuse) {
-                UserIdentity user = stmt.getUserIdentity();
-                GlobalStateMgr.getCurrentState().getAuthenticationMgr().checkPlainPassword(
-                        user.getUser(), user.getHost(), stmt.getAuthOption().getPassword());
-            }
             return null;
         }
 
         private UserAuthenticationInfo analyzeAuthOption(UserIdentity userIdentity, UserAuthOption userAuthOption) {
-            byte[] password = MysqlPassword.EMPTY_PASSWORD;
             String authPluginUsing;
-            if (userAuthOption == null) {
-                authPluginUsing = authenticationManager.getDefaultPlugin();
+            if (userAuthOption == null || userAuthOption.getAuthPlugin() == null) {
+                authPluginUsing = PlainPasswordAuthenticationProvider.PLUGIN_NAME;
             } else {
                 authPluginUsing = userAuthOption.getAuthPlugin();
-                if (authPluginUsing == null) {
-                    authPluginUsing = authenticationManager.getDefaultPlugin();
-                    password = analysePassword(userAuthOption.getPassword(), userAuthOption.isPasswordPlain());
-                } else {
-                    authPluginUsing = userAuthOption.getAuthPlugin();
-                    if (authPluginUsing.equals(PlainPasswordAuthenticationProvider.PLUGIN_NAME)) {
-                        // In this case, authString is the password
-                        password = analysePassword(userAuthOption.getAuthString(), userAuthOption.isPasswordPlain());
-                    }
-                }
             }
 
             try {
                 AuthenticationProvider provider = AuthenticationProviderFactory.create(authPluginUsing);
-                UserAuthenticationInfo info = provider.validAuthenticationInfo(
-                        userIdentity, new String(password, StandardCharsets.UTF_8),
-                        userAuthOption == null ? null : userAuthOption.getAuthString());
-                info.setAuthPlugin(authPluginUsing);
-                info.setOrigUserHost(userIdentity.getUser(), userIdentity.getHost());
-                return info;
+                return provider.analyzeAuthOption(userIdentity, userAuthOption);
             } catch (AuthenticationException e) {
                 throw new SemanticException("invalidate authentication: " + e.getMessage(), e);
             }

diff --git a/fe/fe-core/src/test/java/com/starrocks/authentication/AuthenticationProviderFactoryTest.java b/fe/fe-core/src/test/java/com/starrocks/authentication/AuthenticationProviderFactoryTest.java
@@ -15,7 +15,7 @@
 
 package com.starrocks.authentication;
 
-import com.starrocks.mysql.privilege.Password;
+import com.starrocks.sql.ast.UserAuthOption;
 import com.starrocks.sql.ast.UserIdentity;
 import org.junit.Assert;
 import org.junit.Test;
@@ -25,8 +25,7 @@ public class AuthenticationProviderFactoryTest {
     public void testNormal() throws Exception {
         AuthenticationProvider fakeProvider = new AuthenticationProvider() {
             @Override
-            public UserAuthenticationInfo validAuthenticationInfo(UserIdentity userIdentity, String password,
-                                                                  String textForAuthPlugin)
+            public UserAuthenticationInfo analyzeAuthOption(UserIdentity userIdentity, UserAuthOption userAuthOption)
                     throws AuthenticationException {
                 return null;
             }
@@ -36,12 +35,6 @@ public void authenticate(String user, String host, byte[] password, byte[] rando
                                      UserAuthenticationInfo authenticationInfo) throws AuthenticationException {
 
             }
-
-            @Override
-            public UserAuthenticationInfo upgradedFromPassword(UserIdentity userIdentity, Password password)
-                    throws AuthenticationException {
-                return null;
-            }
         };
         String fakeName = "fake";