[Enhancement](auth) Improve password validation to align with MySQL STRONG policy #60188
Conversation
…TRONG policy
Enhance the validatePlainPassword function in MysqlPassword.java to fully comply
with MySQL's STRONG password validation policy.
Changes:
1. Require all 4 character types (digit, lowercase, uppercase, special character)
instead of the previous "3 out of 4" requirement.
2. Add dictionary word check to reject passwords containing common weak words.
- Built-in dictionary includes common words like: password, admin, test, root, etc.
- Support loading custom dictionary from external file via the new global variable
`validate_password_dictionary_file`.
3. Implement lazy loading for external dictionary file:
- Dictionary is loaded on first password validation call.
- Automatically reloads when the file path is changed.
- Falls back to built-in dictionary if file loading fails.
4. Improve error messages to clearly indicate which requirements are missing.
5. Add comprehensive unit tests for all validation scenarios.
New global variable:
- `validate_password_dictionary_file`: Path to custom dictionary file (one word per line).
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 31426 ms |
TPC-DS: Total hot run time: 172454 ms |
ClickBench: Total hot run time: 26.77 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
TPC-H: Total hot run time: 31278 ms |
TPC-DS: Total hot run time: 172511 ms |
ClickBench: Total hot run time: 26.76 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
run buildall |
TPC-H: Total hot run time: 30967 ms |
TPC-DS: Total hot run time: 173576 ms |
ClickBench: Total hot run time: 26.89 s |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
TPC-H: Total hot run time: 30675 ms |
TPC-DS: Total hot run time: 172314 ms |
ClickBench: Total hot run time: 27.62 s |
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
LGTM |
There was a problem hiding this comment.
Pull request overview
This PR enhances password validation in Apache Doris to align with MySQL's STRONG password policy. It introduces stricter character requirements (all 4 types required instead of 3 out of 4), adds dictionary-based weak password detection with both built-in and external dictionary support, and improves error messages for better user guidance.
Changes:
- Enhanced password validation to require all 4 character types (digit, lowercase, uppercase, special character) and reject passwords containing common dictionary words
- Added configurable external dictionary file support with lazy loading and automatic reload when the file path changes
- Introduced new global variable
validate_password_dictionary_fileand Config propertysecurity_plugins_dirfor dictionary file management
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| fe/fe-core/src/main/java/org/apache/doris/mysql/MysqlPassword.java | Core implementation of enhanced password validation with dictionary checking, lazy loading, and improved error messages |
| fe/fe-core/src/main/java/org/apache/doris/qe/GlobalVariable.java | Added new global variable for dictionary file configuration and spacing improvements in Chinese descriptions |
| fe/fe-common/src/main/java/org/apache/doris/common/Config.java | Added security_plugins_dir configuration property and spacing improvements in Chinese descriptions |
| fe/fe-core/src/test/java/org/apache/doris/mysql/MysqlPasswordTest.java | Comprehensive unit tests covering all validation scenarios including dictionary loading, reloading, and edge cases |
| regression-test/suites/account_p0/test_alter_user.groovy | Updated test assertions to accommodate improved error messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| loadedDictionaryWords = loadDictionaryFromFile(configuredFilePath); | ||
| loadedDictionaryFilePath = configuredFilePath; | ||
| } |
There was a problem hiding this 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.
| // Should pass since dictionary effectively has no words | ||
| MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG, "Test@123X"); |
There was a problem hiding this 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.
| // Should pass since dictionary effectively has no words | |
| MysqlPassword.validatePlainPassword(GlobalVariable.VALIDATE_PASSWORD_POLICY_STRONG, "Test@123X"); | |
| // 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")); | |
| } |
| private static String containsDictionaryWord(String password) { | ||
| String lowerPassword = password.toLowerCase(); | ||
| for (String word : getDictionaryWords()) { | ||
| if (lowerPassword.contains(word)) { | ||
| return word; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this 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.
| // Password not containing custom dictionary words should pass | ||
| // Note: built-in words like "test" should NOT fail because we're using external dictionary |
There was a problem hiding this 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.
| // Password not containing custom dictionary words should pass | |
| // Note: built-in words like "test" should NOT fail because we're using external dictionary | |
| // 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 |
| @After | ||
| public void tearDown() { | ||
| // Restore original values | ||
| GlobalVariable.validatePasswordDictionaryFile = originalDictionaryFile; | ||
| Config.security_plugins_dir = originalSecurityPluginsDir; | ||
| } |
There was a problem hiding this 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.
| 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")); | ||
| } | ||
| } |
There was a problem hiding this 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");
| if (Strings.isNullOrEmpty(configuredFileName)) { | ||
| return BUILTIN_DICTIONARY_WORDS; | ||
| } | ||
|
|
There was a problem hiding this 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.
| // 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; | |
| } |
| @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."}) |
There was a problem hiding this 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."
| // If no file is configured, use built-in dictionary | ||
| if (Strings.isNullOrEmpty(configuredFileName)) { |
There was a problem hiding this 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.
| // If no file is configured, use built-in dictionary | |
| if (Strings.isNullOrEmpty(configuredFileName)) { | |
| // 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; | |
| } |
| // 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this 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.
…TRONG policy (#60188) ### What problem does this PR solve? Enhance the validatePlainPassword function in MysqlPassword.java to fully comply with MySQL's STRONG password validation policy. Changes: 1. Require all 4 character types (digit, lowercase, uppercase, special character) instead of the previous "3 out of 4" requirement. 2. Add dictionary word check to reject passwords containing common weak words. - Built-in dictionary includes common words like: password, admin, test, root, etc. - Support loading custom dictionary from external file via the new global variable `validate_password_dictionary_file`. 3. Implement lazy loading for external dictionary file: - Dictionary is loaded on first password validation call. - Automatically reloads when the file path is changed. - Falls back to built-in dictionary if file loading fails. 4. Improve error messages to clearly indicate which requirements are missing. 5. Add comprehensive unit tests for all validation scenarios. Change the password dictionary file path resolution to use `Config.security_plugins_dir` as the base directory prefix. New `GlobalVariable.validatePasswordDictionaryFile` only needs to specify the filename, and the full path will be constructed as: `${security_plugins_dir}/<filename>`
6 participants
What problem does this PR solve?
Enhance the validatePlainPassword function in MysqlPassword.java to fully comply with MySQL's STRONG password validation policy.
Changes:
Require all 4 character types (digit, lowercase, uppercase, special character) instead of the previous "3 out of 4" requirement.
Add dictionary word check to reject passwords containing common weak words.
validate_password_dictionary_file.Implement lazy loading for external dictionary file:
Improve error messages to clearly indicate which requirements are missing.
Add comprehensive unit tests for all validation scenarios.
Change the password dictionary file path resolution to use
Config.security_plugins_diras the base directory prefix. New
GlobalVariable.validatePasswordDictionaryFileonlyneeds to specify the filename, and the full path will be constructed as:
${security_plugins_dir}/<filename>Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)