Copilot commented on code in PR #60188:
URL: https://github.com/apache/doris/pull/60188#discussion_r2731106987
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/MysqlPasswordTest.java:
##########
@@ -79,4 +109,311 @@ public void testCheckPasswdFail2() throws
AnalysisException {
Assert.fail("No exception throws");
}
+ // ==================== validatePlainPassword Tests ====================
+
+ @Test
+ public void testValidatePasswordDisabledPolicy() throws AnalysisException {
+ // When policy is DISABLED, any password should pass
+ GlobalVariable.validatePasswordDictionaryFile = "";
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"weak");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
null);
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"test123");
+ }
+
+ @Test
+ public void testValidatePasswordStrongPolicyValid() throws
AnalysisException {
+ // Valid password: 8+ chars, has digit, lowercase, uppercase, special
char, no dictionary word
+ GlobalVariable.validatePasswordDictionaryFile = "";
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Xk9$mN2@pL");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"MyP@ss1!");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Str0ng!Powd");
+ }
+
+ @Test
+ public void testValidatePasswordTooShort() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Aa1!abc");
+ Assert.fail("Expected AnalysisException for password too short");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordNullOrEmpty() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
null);
+ Assert.fail("Expected AnalysisException for null password");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"");
+ Assert.fail("Expected AnalysisException for empty password");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingDigit() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Abcdefgh!");
+ Assert.fail("Expected AnalysisException for missing digit");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: numeric"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingLowercase() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"ABCDEFG1!");
+ Assert.fail("Expected AnalysisException for missing lowercase");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: lowercase"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingUppercase() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"abcdefg1!");
+ Assert.fail("Expected AnalysisException for missing uppercase");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: uppercase"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingSpecialChar() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Abcdefg12");
+ Assert.fail("Expected AnalysisException for missing special
character");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: special
character"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingMultipleTypes() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+ // Missing digit, uppercase, special char
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"abcdefghij");
+ Assert.fail("Expected AnalysisException for missing multiple
types");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("numeric"));
+ Assert.assertTrue(e.getMessage().contains("uppercase"));
+ Assert.assertTrue(e.getMessage().contains("special character"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordBuiltinDictionaryWord() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ // Test various built-in dictionary words
+ String[] dictionaryPasswords = {
+ "Test@123X", // contains "test"
+ "Admin@123X", // contains "admin"
+ "Password1!", // contains "password"
+ "Root@1234X", // contains "root"
+ "User@1234X", // contains "user"
+ "Doris@123X", // contains "doris"
+ "Qwerty@12X", // contains "qwerty"
+ "Welcome1!X", // contains "welcome"
+ "Hello@123X", // contains "hello"
+ };
+
+ for (String password : dictionaryPasswords) {
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
password);
+ Assert.fail("Expected AnalysisException for dictionary word
in: " + password);
+ } catch (AnalysisException e) {
+ Assert.assertTrue("Expected dictionary word error for: " +
password,
+ e.getMessage().contains("dictionary word"));
+ }
+ }
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryWordCaseInsensitive() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ // Dictionary check should be case-insensitive
+ String[] caseVariants = {
+ "TEST@123Xy",
+ "TeSt@123Xy",
+ "tEsT@123Xy",
+ "ADMIN@12Xy",
+ "AdMiN@12Xy",
+ };
+
+ for (String password : caseVariants) {
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
password);
+ Assert.fail("Expected AnalysisException for case-insensitive
dictionary word in: " + password);
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("dictionary word"));
+ }
+ }
+ }
+
+ @Test
+ public void testValidatePasswordWithExternalDictionary() throws
IOException, AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Create a temporary dictionary file in the security_plugins_dir
+ File dictFile = tempFolder.newFile("test_dictionary.txt");
+ try (FileWriter writer = new FileWriter(dictFile)) {
+ writer.write("# This is a comment\n");
+ writer.write("customword\n");
+ writer.write(" secretkey \n"); // with spaces
+ writer.write("\n"); // empty line
+ writer.write("forbidden\n");
+ }
+
+ // Use just the filename (not full path)
+ GlobalVariable.validatePasswordDictionaryFile = "test_dictionary.txt";
+
+ // Password containing custom dictionary word should fail
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Customword1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("customword"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Secretkey1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("secretkey"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Forbidden1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("forbidden"));
+ }
+
+ // Password not containing custom dictionary words should pass
+ // Note: built-in words like "test" should NOT fail because we're
using external dictionary
Review Comment:
Design consideration: When an external dictionary file is successfully
loaded, it completely replaces the built-in dictionary rather than
supplementing it. This means passwords containing common weak words like
"password", "admin", "test" will be allowed if they're not in the custom
dictionary. This could weaken security if administrators provide incomplete
custom dictionaries.
Consider whether the external dictionary should supplement (merge with) the
built-in dictionary rather than replace it, or clearly document this behavior
so administrators understand they need to include common weak words in their
custom dictionaries for comprehensive protection.
```suggestion
// Password containing built-in dictionary word should still fail
even with external dictionary
try {
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Test@123Xy");
Assert.fail("Expected AnalysisException for built-in dictionary
word");
} catch (AnalysisException e) {
Assert.assertTrue(e.getMessage().contains("dictionary word"));
}
// Password not containing either built-in or custom dictionary
words should pass
```
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/MysqlPasswordTest.java:
##########
@@ -79,4 +109,311 @@ public void testCheckPasswdFail2() throws
AnalysisException {
Assert.fail("No exception throws");
}
+ // ==================== validatePlainPassword Tests ====================
+
+ @Test
+ public void testValidatePasswordDisabledPolicy() throws AnalysisException {
+ // When policy is DISABLED, any password should pass
+ GlobalVariable.validatePasswordDictionaryFile = "";
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"weak");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
null);
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"test123");
+ }
+
+ @Test
+ public void testValidatePasswordStrongPolicyValid() throws
AnalysisException {
+ // Valid password: 8+ chars, has digit, lowercase, uppercase, special
char, no dictionary word
+ GlobalVariable.validatePasswordDictionaryFile = "";
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Xk9$mN2@pL");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"MyP@ss1!");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Str0ng!Powd");
+ }
+
+ @Test
+ public void testValidatePasswordTooShort() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Aa1!abc");
+ Assert.fail("Expected AnalysisException for password too short");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordNullOrEmpty() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
null);
+ Assert.fail("Expected AnalysisException for null password");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"");
+ Assert.fail("Expected AnalysisException for empty password");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingDigit() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Abcdefgh!");
+ Assert.fail("Expected AnalysisException for missing digit");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: numeric"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingLowercase() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"ABCDEFG1!");
+ Assert.fail("Expected AnalysisException for missing lowercase");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: lowercase"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingUppercase() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"abcdefg1!");
+ Assert.fail("Expected AnalysisException for missing uppercase");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: uppercase"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingSpecialChar() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Abcdefg12");
+ Assert.fail("Expected AnalysisException for missing special
character");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: special
character"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingMultipleTypes() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+ // Missing digit, uppercase, special char
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"abcdefghij");
+ Assert.fail("Expected AnalysisException for missing multiple
types");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("numeric"));
+ Assert.assertTrue(e.getMessage().contains("uppercase"));
+ Assert.assertTrue(e.getMessage().contains("special character"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordBuiltinDictionaryWord() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ // Test various built-in dictionary words
+ String[] dictionaryPasswords = {
+ "Test@123X", // contains "test"
+ "Admin@123X", // contains "admin"
+ "Password1!", // contains "password"
+ "Root@1234X", // contains "root"
+ "User@1234X", // contains "user"
+ "Doris@123X", // contains "doris"
+ "Qwerty@12X", // contains "qwerty"
+ "Welcome1!X", // contains "welcome"
+ "Hello@123X", // contains "hello"
+ };
+
+ for (String password : dictionaryPasswords) {
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
password);
+ Assert.fail("Expected AnalysisException for dictionary word
in: " + password);
+ } catch (AnalysisException e) {
+ Assert.assertTrue("Expected dictionary word error for: " +
password,
+ e.getMessage().contains("dictionary word"));
+ }
+ }
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryWordCaseInsensitive() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ // Dictionary check should be case-insensitive
+ String[] caseVariants = {
+ "TEST@123Xy",
+ "TeSt@123Xy",
+ "tEsT@123Xy",
+ "ADMIN@12Xy",
+ "AdMiN@12Xy",
+ };
+
+ for (String password : caseVariants) {
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
password);
+ Assert.fail("Expected AnalysisException for case-insensitive
dictionary word in: " + password);
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("dictionary word"));
+ }
+ }
+ }
+
+ @Test
+ public void testValidatePasswordWithExternalDictionary() throws
IOException, AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Create a temporary dictionary file in the security_plugins_dir
+ File dictFile = tempFolder.newFile("test_dictionary.txt");
+ try (FileWriter writer = new FileWriter(dictFile)) {
+ writer.write("# This is a comment\n");
+ writer.write("customword\n");
+ writer.write(" secretkey \n"); // with spaces
+ writer.write("\n"); // empty line
+ writer.write("forbidden\n");
+ }
+
+ // Use just the filename (not full path)
+ GlobalVariable.validatePasswordDictionaryFile = "test_dictionary.txt";
+
+ // Password containing custom dictionary word should fail
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Customword1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("customword"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Secretkey1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("secretkey"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Forbidden1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("forbidden"));
+ }
+
+ // Password not containing custom dictionary words should pass
+ // Note: built-in words like "test" should NOT fail because we're
using external dictionary
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Xk9$mN2@pL");
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryFileNotFound() throws
AnalysisException {
+ // Set security_plugins_dir to a valid path
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // When dictionary file doesn't exist, should fall back to built-in
dictionary
+ GlobalVariable.validatePasswordDictionaryFile =
"non_existent_dictionary.txt";
+
+ // Built-in dictionary word should still fail
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Test@123Xy");
+ Assert.fail("Expected AnalysisException for built-in dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("dictionary word"));
+ }
+
+ // Valid password should pass
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Xk9$mN2@pL");
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryFileReload() throws IOException,
AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Create first dictionary file
+ File dictFile1 = tempFolder.newFile("dict1.txt");
+ try (FileWriter writer = new FileWriter(dictFile1)) {
+ writer.write("wordone\n");
+ }
+
+ // Use just the filename
+ GlobalVariable.validatePasswordDictionaryFile = "dict1.txt";
+
+ // Should fail for wordone
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Wordone12!");
+ Assert.fail("Expected AnalysisException for wordone");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("wordone"));
+ }
+
+ // Create second dictionary file with different content
+ File dictFile2 = tempFolder.newFile("dict2.txt");
+ try (FileWriter writer = new FileWriter(dictFile2)) {
+ writer.write("wordtwo\n");
+ }
+
+ // Change to second dictionary file (just filename)
+ GlobalVariable.validatePasswordDictionaryFile = "dict2.txt";
+
+ // Should now pass for wordone (not in new dictionary)
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Wordone12!");
+
+ // Should fail for wordtwo
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Wordtwo12!");
+ Assert.fail("Expected AnalysisException for wordtwo");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("wordtwo"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordEmptyDictionaryFile() throws IOException,
AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Use just the filename
+ GlobalVariable.validatePasswordDictionaryFile = "empty_dict.txt";
+
+ // With empty dictionary, only character requirements should be checked
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Test@123X");
+ Assert.fail("Expected AnalysisException for test");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("test"));
+ }
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Admin@12X");
+ Assert.fail("Expected AnalysisException for admin");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("admin"));
+ }
+ }
Review Comment:
The testValidatePasswordEmptyDictionaryFile test expects passwords
containing dictionary words ("test", "admin") to fail even when using an empty
external dictionary file. However, the test does not actually create the
empty_dict.txt file. When the file doesn't exist, loadDictionaryFromFile
returns null, and getDictionaryWords falls back to BUILTIN_DICTIONARY_WORDS,
which contains "test" and "admin". This makes the test pass for the wrong
reason - it's testing the fallback behavior rather than the empty dictionary
behavior.
The test should create the empty file to properly test the scenario where an
empty external dictionary is loaded successfully. Add a line to create the
empty file before setting the GlobalVariable, for example:
tempFolder.newFile("empty_dict.txt");
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/MysqlPassword.java:
##########
@@ -88,10 +95,97 @@ public class MysqlPassword {
private static final Set<Character> complexCharSet;
public static final int MIN_PASSWORD_LEN = 8;
+ /**
+ * Built-in dictionary of common weak password words.
+ * Used as fallback when no external dictionary file is configured.
+ * Password containing any of these words (case-insensitive) will be
rejected under STRONG policy.
+ */
+ private static final Set<String> BUILTIN_DICTIONARY_WORDS =
ImmutableSet.of(
+ // Common password words
+ "password", "passwd", "pass", "pwd", "secret",
+ // User/role related
+ "admin", "administrator", "root", "user", "guest", "login",
"master", "super",
+ // Test/demo related
+ "test", "testing", "demo", "sample", "example", "temp",
"temporary",
+ // System/database related
+ "system", "server", "database", "mysql", "doris", "oracle",
"postgres",
+ // Common weak patterns
+ "qwerty", "abc", "letmein", "welcome", "hello", "monkey",
"dragon", "iloveyou",
+ "trustno", "sunshine", "princess", "football", "baseball", "soccer"
+ );
+
+ // Lazy-loaded dictionary from external file
+ private static volatile Set<String> loadedDictionaryWords = null;
+ // The file path that was used to load the dictionary (for detecting
changes)
+ private static volatile String loadedDictionaryFilePath = null;
+ // Lock object for thread-safe lazy loading
+ private static final Object DICTIONARY_LOAD_LOCK = new Object();
+
static {
complexCharSet = "~!@#$%^&*()_+|<>,.?/:;'[]{}".chars().mapToObj(c ->
(char) c).collect(Collectors.toSet());
}
+ /**
+ * Get the dictionary words to use for password validation.
+ * If an external dictionary file is configured, load it lazily.
+ * Otherwise, use the built-in dictionary.
+ *
+ * @return the set of dictionary words (all in lowercase)
+ */
+ private static Set<String> getDictionaryWords() {
+ String configuredFileName =
GlobalVariable.validatePasswordDictionaryFile;
+
+ // If no file is configured, use built-in dictionary
+ if (Strings.isNullOrEmpty(configuredFileName)) {
+ return BUILTIN_DICTIONARY_WORDS;
+ }
+
+ // Construct full path: security_plugins_dir/<configured_file_name>
and normalize for safe comparison
+ String configuredFilePath = Paths.get(Config.security_plugins_dir,
configuredFileName)
+ .normalize().toString();
+
+ // Check if we need to (re)load the dictionary
+ // Double-checked locking for thread safety
+ if (loadedDictionaryWords == null ||
!configuredFilePath.equals(loadedDictionaryFilePath)) {
+ synchronized (DICTIONARY_LOAD_LOCK) {
+ if (loadedDictionaryWords == null ||
!configuredFilePath.equals(loadedDictionaryFilePath)) {
+ loadedDictionaryWords =
loadDictionaryFromFile(configuredFilePath);
+ loadedDictionaryFilePath = configuredFilePath;
+ }
Review Comment:
Race condition in dictionary loading: The double-checked locking pattern is
implemented, but there's a subtle issue. After loadedDictionaryWords is set to
a non-null value on line 152, another thread could read loadedDictionaryWords
(line 158) before loadedDictionaryFilePath is updated (line 153). This could
cause the wrong dictionary to be returned briefly during a reload.
The assignment order should be reversed: first update
loadedDictionaryFilePath, then loadedDictionaryWords. However, this would still
have issues. A better approach is to assign both inside the synchronized block
atomically, or use a holder object that contains both the path and words
together to ensure they're always consistent.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/MysqlPassword.java:
##########
@@ -88,10 +95,97 @@ public class MysqlPassword {
private static final Set<Character> complexCharSet;
public static final int MIN_PASSWORD_LEN = 8;
+ /**
+ * Built-in dictionary of common weak password words.
+ * Used as fallback when no external dictionary file is configured.
+ * Password containing any of these words (case-insensitive) will be
rejected under STRONG policy.
+ */
+ private static final Set<String> BUILTIN_DICTIONARY_WORDS =
ImmutableSet.of(
+ // Common password words
+ "password", "passwd", "pass", "pwd", "secret",
+ // User/role related
+ "admin", "administrator", "root", "user", "guest", "login",
"master", "super",
+ // Test/demo related
+ "test", "testing", "demo", "sample", "example", "temp",
"temporary",
+ // System/database related
+ "system", "server", "database", "mysql", "doris", "oracle",
"postgres",
+ // Common weak patterns
+ "qwerty", "abc", "letmein", "welcome", "hello", "monkey",
"dragon", "iloveyou",
+ "trustno", "sunshine", "princess", "football", "baseball", "soccer"
+ );
+
+ // Lazy-loaded dictionary from external file
+ private static volatile Set<String> loadedDictionaryWords = null;
+ // The file path that was used to load the dictionary (for detecting
changes)
+ private static volatile String loadedDictionaryFilePath = null;
+ // Lock object for thread-safe lazy loading
+ private static final Object DICTIONARY_LOAD_LOCK = new Object();
+
static {
complexCharSet = "~!@#$%^&*()_+|<>,.?/:;'[]{}".chars().mapToObj(c ->
(char) c).collect(Collectors.toSet());
}
+ /**
+ * Get the dictionary words to use for password validation.
+ * If an external dictionary file is configured, load it lazily.
+ * Otherwise, use the built-in dictionary.
+ *
+ * @return the set of dictionary words (all in lowercase)
+ */
+ private static Set<String> getDictionaryWords() {
+ String configuredFileName =
GlobalVariable.validatePasswordDictionaryFile;
+
+ // If no file is configured, use built-in dictionary
+ if (Strings.isNullOrEmpty(configuredFileName)) {
+ return BUILTIN_DICTIONARY_WORDS;
+ }
+
Review Comment:
Path traversal vulnerability: The code does not validate the
configuredFileName before constructing the file path. A malicious user with
permission to set the validate_password_dictionary_file global variable could
use path traversal sequences (e.g., "../../etc/passwd") to read arbitrary files
from the filesystem. While normalize() is called, it's only used for comparison
and doesn't prevent the initial file access.
Add validation to reject filenames containing path separators or traversal
sequences. For example, check that configuredFileName doesn't contain '/', '\',
or '..' before constructing the path. This is similar to the security check in
GetLogFileAction.java:129 which validates file names with a regex pattern.
```suggestion
// Validate that the configured file name is a simple file name
without path separators
// or traversal sequences to prevent path traversal attacks.
if (configuredFileName.contains("/") ||
configuredFileName.contains("\\") || configuredFileName.contains("..")) {
LOG.warn("Invalid validate_password_dictionary_file '{}': path
separators or traversal sequences are not allowed. "
+ "Falling back to built-in dictionary words.",
configuredFileName);
return BUILTIN_DICTIONARY_WORDS;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/MysqlPassword.java:
##########
@@ -88,10 +95,97 @@ public class MysqlPassword {
private static final Set<Character> complexCharSet;
public static final int MIN_PASSWORD_LEN = 8;
+ /**
+ * Built-in dictionary of common weak password words.
+ * Used as fallback when no external dictionary file is configured.
+ * Password containing any of these words (case-insensitive) will be
rejected under STRONG policy.
+ */
+ private static final Set<String> BUILTIN_DICTIONARY_WORDS =
ImmutableSet.of(
+ // Common password words
+ "password", "passwd", "pass", "pwd", "secret",
+ // User/role related
+ "admin", "administrator", "root", "user", "guest", "login",
"master", "super",
+ // Test/demo related
+ "test", "testing", "demo", "sample", "example", "temp",
"temporary",
+ // System/database related
+ "system", "server", "database", "mysql", "doris", "oracle",
"postgres",
+ // Common weak patterns
+ "qwerty", "abc", "letmein", "welcome", "hello", "monkey",
"dragon", "iloveyou",
+ "trustno", "sunshine", "princess", "football", "baseball", "soccer"
+ );
+
+ // Lazy-loaded dictionary from external file
+ private static volatile Set<String> loadedDictionaryWords = null;
+ // The file path that was used to load the dictionary (for detecting
changes)
+ private static volatile String loadedDictionaryFilePath = null;
+ // Lock object for thread-safe lazy loading
+ private static final Object DICTIONARY_LOAD_LOCK = new Object();
+
static {
complexCharSet = "~!@#$%^&*()_+|<>,.?/:;'[]{}".chars().mapToObj(c ->
(char) c).collect(Collectors.toSet());
}
+ /**
+ * Get the dictionary words to use for password validation.
+ * If an external dictionary file is configured, load it lazily.
+ * Otherwise, use the built-in dictionary.
+ *
+ * @return the set of dictionary words (all in lowercase)
+ */
+ private static Set<String> getDictionaryWords() {
+ String configuredFileName =
GlobalVariable.validatePasswordDictionaryFile;
+
+ // If no file is configured, use built-in dictionary
+ if (Strings.isNullOrEmpty(configuredFileName)) {
+ return BUILTIN_DICTIONARY_WORDS;
+ }
+
+ // Construct full path: security_plugins_dir/<configured_file_name>
and normalize for safe comparison
+ String configuredFilePath = Paths.get(Config.security_plugins_dir,
configuredFileName)
+ .normalize().toString();
+
+ // Check if we need to (re)load the dictionary
+ // Double-checked locking for thread safety
+ if (loadedDictionaryWords == null ||
!configuredFilePath.equals(loadedDictionaryFilePath)) {
+ synchronized (DICTIONARY_LOAD_LOCK) {
+ if (loadedDictionaryWords == null ||
!configuredFilePath.equals(loadedDictionaryFilePath)) {
+ loadedDictionaryWords =
loadDictionaryFromFile(configuredFilePath);
+ loadedDictionaryFilePath = configuredFilePath;
+ }
+ }
+ }
Review Comment:
Potential issue with file content change detection: The dictionary loading
mechanism only detects when the file path changes, not when the file content
changes. If an administrator updates the dictionary file content without
changing the filename, the stale cached dictionary will continue to be used
until the FE process is restarted or the filename is changed.
Consider adding file modification time tracking (using
Files.getLastModifiedTime()) to detect when the file has been updated, and
reload it automatically. Alternatively, document this limitation so
administrators know they need to change the filename or restart the FE to
reload the dictionary.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/MysqlPassword.java:
##########
@@ -289,31 +383,74 @@ public static byte[] checkPassword(String passwdString)
throws AnalysisException
return passwd;
}
+ /**
+ * Validate plain text password according to MySQL's validate_password
policy.
+ * For STRONG policy, the password must meet all of the following
requirements:
+ * 1. At least MIN_PASSWORD_LEN (8) characters long
+ * 2. Contains at least 1 digit
+ * 3. Contains at least 1 lowercase letter
+ * 4. Contains at least 1 uppercase letter
+ * 5. Contains at least 1 special character
+ * 6. Does not contain any dictionary words (case-insensitive)
+ */
public static void validatePlainPassword(long validaPolicy, String text)
throws AnalysisException {
if (validaPolicy == GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG) {
if (Strings.isNullOrEmpty(text) || text.length() <
MIN_PASSWORD_LEN) {
throw new AnalysisException(
- "Violate password validation policy: STRONG. The
password must be at least 8 characters");
+ "Violate password validation policy: STRONG. "
+ + "The password must be at least " +
MIN_PASSWORD_LEN + " characters.");
}
- int i = 0;
- if (text.chars().anyMatch(Character::isDigit)) {
- i++;
+ StringBuilder missingTypes = new StringBuilder();
+
+ if (text.chars().noneMatch(Character::isDigit)) {
+ missingTypes.append("numeric, ");
}
- if (text.chars().anyMatch(Character::isLowerCase)) {
- i++;
+ if (text.chars().noneMatch(Character::isLowerCase)) {
+ missingTypes.append("lowercase, ");
}
- if (text.chars().anyMatch(Character::isUpperCase)) {
- i++;
+ if (text.chars().noneMatch(Character::isUpperCase)) {
+ missingTypes.append("uppercase, ");
}
- if (text.chars().anyMatch(c -> complexCharSet.contains((char) c)))
{
- i++;
+ if (text.chars().noneMatch(c -> complexCharSet.contains((char)
c))) {
+ missingTypes.append("special character, ");
}
- if (i < 3) {
+
+ if (missingTypes.length() > 0) {
+ // Remove trailing ", "
+ missingTypes.setLength(missingTypes.length() - 2);
throw new AnalysisException(
- "Violate password validation policy: STRONG. The
password must contain at least 3 types of "
- + "numbers, uppercase letters, lowercase
letters and special characters.");
+ "Violate password validation policy: STRONG. "
+ + "The password must contain at least one
character from each of the following types: "
+ + "numeric, lowercase, uppercase, and special
characters. "
+ + "Missing: " + missingTypes + ".");
+ }
+
+ // Check for dictionary words (case-insensitive)
+ String foundWord = containsDictionaryWord(text);
+ if (foundWord != null) {
+ throw new AnalysisException(
+ "Violate password validation policy: STRONG. "
+ + "The password contains a common dictionary
word '" + foundWord + "', "
+ + "which makes it easy to guess. Please choose
a more secure password.");
+ }
+ }
+ }
+
+ /**
+ * Check if the password contains any dictionary word (case-insensitive).
+ * Uses either the external dictionary file (if configured) or the
built-in dictionary.
+ *
+ * @param password the password to check
+ * @return the found dictionary word, or null if none found
+ */
+ private static String containsDictionaryWord(String password) {
+ String lowerPassword = password.toLowerCase();
+ for (String word : getDictionaryWords()) {
+ if (lowerPassword.contains(word)) {
+ return word;
}
}
+ return null;
Review Comment:
Performance concern: The containsDictionaryWord method uses a simple
substring search (contains()) for each dictionary word. For large dictionaries
and frequent password validations, this could become a performance bottleneck
as it results in O(n*m) complexity where n is the number of dictionary words
and m is the password length.
Consider using a more efficient algorithm such as the Aho-Corasick algorithm
for multi-pattern string matching, or pre-processing the dictionary into a trie
structure. Alternatively, document the expected dictionary size limits to set
appropriate performance expectations.
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/MysqlPasswordTest.java:
##########
@@ -18,13 +18,43 @@
package org.apache.doris.mysql;
import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.Config;
+import org.apache.doris.qe.GlobalVariable;
+import org.junit.After;
import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
import java.io.UnsupportedEncodingException;
public class MysqlPasswordTest {
+
+ @Rule
+ public TemporaryFolder tempFolder = new TemporaryFolder();
+
+ private String originalDictionaryFile;
+ private String originalSecurityPluginsDir;
+
+ @Before
+ public void setUp() {
+ // Save original values
+ originalDictionaryFile = GlobalVariable.validatePasswordDictionaryFile;
+ originalSecurityPluginsDir = Config.security_plugins_dir;
+ }
+
+ @After
+ public void tearDown() {
+ // Restore original values
+ GlobalVariable.validatePasswordDictionaryFile = originalDictionaryFile;
+ Config.security_plugins_dir = originalSecurityPluginsDir;
+ }
Review Comment:
Test isolation issue: The tests modify global static state
(loadedDictionaryWords and loadedDictionaryFilePath in MysqlPassword.java) but
the tearDown method doesn't reset these cached values. This means the execution
order of tests can affect their results. For example, if
testValidatePasswordWithExternalDictionary runs before
testValidatePasswordBuiltinDictionaryWord, the cached dictionary from the first
test could interfere with the second test.
Consider adding a method to reset the dictionary cache (e.g., a
package-private resetDictionaryCache() method in MysqlPassword) and call it in
the setUp() or tearDown() methods to ensure test isolation.
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/MysqlPasswordTest.java:
##########
@@ -79,4 +109,311 @@ public void testCheckPasswdFail2() throws
AnalysisException {
Assert.fail("No exception throws");
}
+ // ==================== validatePlainPassword Tests ====================
+
+ @Test
+ public void testValidatePasswordDisabledPolicy() throws AnalysisException {
+ // When policy is DISABLED, any password should pass
+ GlobalVariable.validatePasswordDictionaryFile = "";
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"weak");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
null);
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_DISABLED,
"test123");
+ }
+
+ @Test
+ public void testValidatePasswordStrongPolicyValid() throws
AnalysisException {
+ // Valid password: 8+ chars, has digit, lowercase, uppercase, special
char, no dictionary word
+ GlobalVariable.validatePasswordDictionaryFile = "";
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Xk9$mN2@pL");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"MyP@ss1!");
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Str0ng!Powd");
+ }
+
+ @Test
+ public void testValidatePasswordTooShort() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Aa1!abc");
+ Assert.fail("Expected AnalysisException for password too short");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordNullOrEmpty() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
null);
+ Assert.fail("Expected AnalysisException for null password");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"");
+ Assert.fail("Expected AnalysisException for empty password");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("at least 8
characters"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingDigit() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Abcdefgh!");
+ Assert.fail("Expected AnalysisException for missing digit");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: numeric"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingLowercase() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"ABCDEFG1!");
+ Assert.fail("Expected AnalysisException for missing lowercase");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: lowercase"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingUppercase() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"abcdefg1!");
+ Assert.fail("Expected AnalysisException for missing uppercase");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: uppercase"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingSpecialChar() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Abcdefg12");
+ Assert.fail("Expected AnalysisException for missing special
character");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("Missing: special
character"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordMissingMultipleTypes() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ try {
+ // Missing digit, uppercase, special char
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"abcdefghij");
+ Assert.fail("Expected AnalysisException for missing multiple
types");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("numeric"));
+ Assert.assertTrue(e.getMessage().contains("uppercase"));
+ Assert.assertTrue(e.getMessage().contains("special character"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordBuiltinDictionaryWord() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ // Test various built-in dictionary words
+ String[] dictionaryPasswords = {
+ "Test@123X", // contains "test"
+ "Admin@123X", // contains "admin"
+ "Password1!", // contains "password"
+ "Root@1234X", // contains "root"
+ "User@1234X", // contains "user"
+ "Doris@123X", // contains "doris"
+ "Qwerty@12X", // contains "qwerty"
+ "Welcome1!X", // contains "welcome"
+ "Hello@123X", // contains "hello"
+ };
+
+ for (String password : dictionaryPasswords) {
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
password);
+ Assert.fail("Expected AnalysisException for dictionary word
in: " + password);
+ } catch (AnalysisException e) {
+ Assert.assertTrue("Expected dictionary word error for: " +
password,
+ e.getMessage().contains("dictionary word"));
+ }
+ }
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryWordCaseInsensitive() {
+ GlobalVariable.validatePasswordDictionaryFile = "";
+ // Dictionary check should be case-insensitive
+ String[] caseVariants = {
+ "TEST@123Xy",
+ "TeSt@123Xy",
+ "tEsT@123Xy",
+ "ADMIN@12Xy",
+ "AdMiN@12Xy",
+ };
+
+ for (String password : caseVariants) {
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
password);
+ Assert.fail("Expected AnalysisException for case-insensitive
dictionary word in: " + password);
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("dictionary word"));
+ }
+ }
+ }
+
+ @Test
+ public void testValidatePasswordWithExternalDictionary() throws
IOException, AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Create a temporary dictionary file in the security_plugins_dir
+ File dictFile = tempFolder.newFile("test_dictionary.txt");
+ try (FileWriter writer = new FileWriter(dictFile)) {
+ writer.write("# This is a comment\n");
+ writer.write("customword\n");
+ writer.write(" secretkey \n"); // with spaces
+ writer.write("\n"); // empty line
+ writer.write("forbidden\n");
+ }
+
+ // Use just the filename (not full path)
+ GlobalVariable.validatePasswordDictionaryFile = "test_dictionary.txt";
+
+ // Password containing custom dictionary word should fail
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Customword1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("customword"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Secretkey1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("secretkey"));
+ }
+
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Forbidden1!");
+ Assert.fail("Expected AnalysisException for custom dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("forbidden"));
+ }
+
+ // Password not containing custom dictionary words should pass
+ // Note: built-in words like "test" should NOT fail because we're
using external dictionary
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Xk9$mN2@pL");
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryFileNotFound() throws
AnalysisException {
+ // Set security_plugins_dir to a valid path
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // When dictionary file doesn't exist, should fall back to built-in
dictionary
+ GlobalVariable.validatePasswordDictionaryFile =
"non_existent_dictionary.txt";
+
+ // Built-in dictionary word should still fail
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Test@123Xy");
+ Assert.fail("Expected AnalysisException for built-in dictionary
word");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("dictionary word"));
+ }
+
+ // Valid password should pass
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Xk9$mN2@pL");
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryFileReload() throws IOException,
AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Create first dictionary file
+ File dictFile1 = tempFolder.newFile("dict1.txt");
+ try (FileWriter writer = new FileWriter(dictFile1)) {
+ writer.write("wordone\n");
+ }
+
+ // Use just the filename
+ GlobalVariable.validatePasswordDictionaryFile = "dict1.txt";
+
+ // Should fail for wordone
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Wordone12!");
+ Assert.fail("Expected AnalysisException for wordone");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("wordone"));
+ }
+
+ // Create second dictionary file with different content
+ File dictFile2 = tempFolder.newFile("dict2.txt");
+ try (FileWriter writer = new FileWriter(dictFile2)) {
+ writer.write("wordtwo\n");
+ }
+
+ // Change to second dictionary file (just filename)
+ GlobalVariable.validatePasswordDictionaryFile = "dict2.txt";
+
+ // Should now pass for wordone (not in new dictionary)
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Wordone12!");
+
+ // Should fail for wordtwo
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Wordtwo12!");
+ Assert.fail("Expected AnalysisException for wordtwo");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("wordtwo"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordEmptyDictionaryFile() throws IOException,
AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Use just the filename
+ GlobalVariable.validatePasswordDictionaryFile = "empty_dict.txt";
+
+ // With empty dictionary, only character requirements should be checked
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Test@123X");
+ Assert.fail("Expected AnalysisException for test");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("test"));
+ }
+ try {
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Admin@12X");
+ Assert.fail("Expected AnalysisException for admin");
+ } catch (AnalysisException e) {
+ Assert.assertTrue(e.getMessage().contains("admin"));
+ }
+ }
+
+ @Test
+ public void testValidatePasswordDictionaryWithCommentsOnly() throws
IOException, AnalysisException {
+ // Set security_plugins_dir to temp folder
+ Config.security_plugins_dir = tempFolder.getRoot().getAbsolutePath();
+
+ // Create a dictionary file with only comments
+ File dictFile = tempFolder.newFile("comments_dict.txt");
+ try (FileWriter writer = new FileWriter(dictFile)) {
+ writer.write("# comment 1\n");
+ writer.write("# comment 2\n");
+ writer.write(" # comment with leading spaces\n");
+ }
+
+ // Use just the filename
+ GlobalVariable.validatePasswordDictionaryFile = "comments_dict.txt";
+
+ // Should pass since dictionary effectively has no words
+
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Test@123X");
Review Comment:
The test testValidatePasswordDictionaryWithCommentsOnly passes a password
"Test@123X" that contains the built-in dictionary word "test", and expects it
to pass. However, looking at the implementation, when a dictionary file is
successfully loaded (even if empty after filtering comments), it replaces the
built-in dictionary. The issue is that if the loaded dictionary has no words
(empty Set), it's still treated as a valid dictionary and returned. This means
passwords like "Test@123X" would pass, which is inconsistent with the security
expectations.
While this behavior might be intentional to allow administrators to disable
dictionary checking by providing an empty file, it should be clearly
documented. Consider whether an empty external dictionary should fall back to
the built-in dictionary, or if it should truly disable dictionary checking.
```suggestion
// Even when the external dictionary file contains only comments,
// built-in dictionary words should still be enforced.
try {
MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG,
"Test@123X");
Assert.fail("Expected AnalysisException for test");
} catch (AnalysisException e) {
Assert.assertTrue(e.getMessage().contains("test"));
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/MysqlPassword.java:
##########
@@ -88,10 +95,97 @@ public class MysqlPassword {
private static final Set<Character> complexCharSet;
public static final int MIN_PASSWORD_LEN = 8;
+ /**
+ * Built-in dictionary of common weak password words.
+ * Used as fallback when no external dictionary file is configured.
+ * Password containing any of these words (case-insensitive) will be
rejected under STRONG policy.
+ */
+ private static final Set<String> BUILTIN_DICTIONARY_WORDS =
ImmutableSet.of(
+ // Common password words
+ "password", "passwd", "pass", "pwd", "secret",
+ // User/role related
+ "admin", "administrator", "root", "user", "guest", "login",
"master", "super",
+ // Test/demo related
+ "test", "testing", "demo", "sample", "example", "temp",
"temporary",
+ // System/database related
+ "system", "server", "database", "mysql", "doris", "oracle",
"postgres",
+ // Common weak patterns
+ "qwerty", "abc", "letmein", "welcome", "hello", "monkey",
"dragon", "iloveyou",
+ "trustno", "sunshine", "princess", "football", "baseball", "soccer"
+ );
+
+ // Lazy-loaded dictionary from external file
+ private static volatile Set<String> loadedDictionaryWords = null;
+ // The file path that was used to load the dictionary (for detecting
changes)
+ private static volatile String loadedDictionaryFilePath = null;
+ // Lock object for thread-safe lazy loading
+ private static final Object DICTIONARY_LOAD_LOCK = new Object();
+
static {
complexCharSet = "~!@#$%^&*()_+|<>,.?/:;'[]{}".chars().mapToObj(c ->
(char) c).collect(Collectors.toSet());
}
+ /**
+ * Get the dictionary words to use for password validation.
+ * If an external dictionary file is configured, load it lazily.
+ * Otherwise, use the built-in dictionary.
+ *
+ * @return the set of dictionary words (all in lowercase)
+ */
+ private static Set<String> getDictionaryWords() {
+ String configuredFileName =
GlobalVariable.validatePasswordDictionaryFile;
+
+ // If no file is configured, use built-in dictionary
+ if (Strings.isNullOrEmpty(configuredFileName)) {
Review Comment:
Missing cache invalidation: When
GlobalVariable.validatePasswordDictionaryFile is changed back to an empty
string (to use built-in dictionary), the cached loadedDictionaryWords and
loadedDictionaryFilePath are not cleared. The next call to getDictionaryWords()
will correctly return BUILTIN_DICTIONARY_WORDS due to the early return on line
140, but the stale cached values remain in memory. If the variable is later set
to the same filename again, the stale cache will be reused without checking if
the file content has changed.
Add logic to clear the cache (set loadedDictionaryWords and
loadedDictionaryFilePath to null) when configuredFileName is empty or when we
detect a change.
```suggestion
// If no file is configured, clear any cached external dictionary
and use built-in dictionary
if (Strings.isNullOrEmpty(configuredFileName)) {
synchronized (DICTIONARY_LOAD_LOCK) {
loadedDictionaryWords = null;
loadedDictionaryFilePath = null;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/GlobalVariable.java:
##########
@@ -139,6 +141,17 @@ public final class GlobalVariable {
@VariableMgr.VarAttr(name = VALIDATE_PASSWORD_POLICY, flag =
VariableMgr.GLOBAL)
public static long validatePasswordPolicy = 0;
+ @VariableMgr.VarAttr(name = VALIDATE_PASSWORD_DICTIONARY_FILE, flag =
VariableMgr.GLOBAL,
+ description = {"密码验证字典文件路径。文件为纯文本格式，每行一个词。"
+ + "当 validate_password_policy 为 STRONG(2)
时，密码中不能包含字典中的任何词（不区分大小写）。"
+ + "如果为空，则使用内置字典。",
+ "Path to the password validation dictionary file. "
+ + "The file should be plain text with one word per
line. "
+ + "When validate_password_policy is STRONG(2), "
+ + "the password cannot contain any word from the
dictionary "
+ + "(case-insensitive). If empty, a built-in
dictionary will be used."})
Review Comment:
The description states "Path to the password validation dictionary file" and
"If empty, a built-in dictionary will be used." However, based on the
implementation (MysqlPassword.java:144), the value should be just a filename,
not a full path. The full path is constructed by combining
Config.security_plugins_dir with the provided filename. The description is
misleading and should clearly state that only the filename should be specified,
and it will be looked up in the security_plugins_dir directory.
Update the description to: "Filename of the password validation dictionary
file in the security_plugins_dir. The file should be plain text with one word
per line. When validate_password_policy is STRONG(2), the password cannot
contain any word from the dictionary (case-insensitive). If empty, a built-in
dictionary will be used."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
