Uh oh!
There was an error while loading. Please reload this page.
[enhance](auth) introduction of configuration property to prohibit login with empty LDAP password - #61440
Conversation
Thearas
commented
Mar 17, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
iaorekhov-1980
commented
Mar 17, 2026
run buildall |
doris-robot
commented
Mar 17, 2026
TPC-H: Total hot run time: 27089 ms |
hello-stephen
commented
Mar 17, 2026
FE UT Coverage ReportIncrement line coverage |
doris-robot
commented
Mar 17, 2026
TPC-DS: Total hot run time: 169028 ms |
iaorekhov-1980
commented
Mar 18, 2026
run external |
iaorekhov-1980
commented
Mar 18, 2026
run nonConcurrent |
8fa5633 to
629cab0Compareiaorekhov-1980
commented
Mar 23, 2026
run buildall |
doris-robot
commented
Mar 23, 2026
TPC-H: Total hot run time: 26705 ms |
doris-robot
commented
Mar 23, 2026
TPC-DS: Total hot run time: 167913 ms |
hello-stephen
commented
Mar 23, 2026
FE UT Coverage ReportIncrement line coverage |
iaorekhov-1980
commented
Mar 23, 2026
run external |
1 similar comment
iaorekhov-1980
commented
Mar 23, 2026
run external |
morningman
commented
Mar 25, 2026
/review |
There was a problem hiding this comment.
Code Review Summary for PR #61440
PR Goal: Add ldap_allow_empty_pass config to prohibit LDAP login with empty passwords.
Critical Checkpoint Conclusions
1. Does the code accomplish its goal? Is there a test that proves it?
Yes. The check is correctly placed in both independent LDAP authentication paths (LdapAuthenticator.internalAuthenticate for MySQL wire protocol and Auth.checkPlainPassword for Thrift/HTTP/Arrow Flight). Tests cover the LdapAuthenticator path. However, there is no test for the Auth.checkPlainPassword path.
2. Is the modification as small, clear, and focused as possible?
Yes. The change is minimal and well-scoped.
3. Concurrency concerns?
The config ldap_allow_empty_pass is a static boolean read without synchronization. This is acceptable for a config flag — worst case is a brief window of stale reads during config reload, which is tolerable for this use case.
4. Configuration items added — should it allow dynamic changes?
Yes — see inline comment. The config should be mutable = true so it can be toggled at runtime without FE restart. This is a pure runtime policy check with no initialization dependency, unlike connection/pool configs.
5. Functionally parallel code paths?
Both independent LDAP auth paths are covered. No parallel paths are missed.
6. Error message quality?
The error message "Access with empty password is prohibited for user %s because of current mode" is vague — "current mode" doesn't explain what mode. See inline comment for suggested improvement.
7. Test coverage?
- Unit test covers the
LdapAuthenticatorpath with empty password allowed/denied scenarios. Good. - Missing: test for
Auth.checkPlainPasswordpath (the Thrift/HTTP entry point). - Missing: test with
nullpassword (not just empty string"").
8. Incompatible changes / rolling upgrade?
No incompatible changes. Default value true preserves backward compatibility.
9. Observability?
LOG.info is adequate for rejected login attempts.
10. Transaction/persistence?
Not applicable.
11. Performance?
No concerns — the check is a simple boolean comparison on a non-hot path.
12. Other issues?
- The
LdapManager.checkUserPasswdat line 106 already rejectsnullpasswords (Objects.isNull(passwd)returns false) but does NOT reject empty strings — it will proceed toldapClient.checkPassword()with an empty string, which typically results in an LDAP "unauthenticated bind" (silently succeeds). This confirms the PR addresses a real security issue. - The PR description mentions
ldap_use_sslin section 3.2 but meansldap_allow_empty_pass— this is a typo in the PR description only (not in code). - The
Release notesection says "None" but this is a user-visible behavior change (new config property). It should have a release note.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
aaa284b to
267c2e3Compareiaorekhov-1980
commented
Mar 26, 2026
run feut |
8 similar comments
iaorekhov-1980
commented
Mar 26, 2026
run feut |
iaorekhov-1980
commented
Mar 26, 2026
run feut |
iaorekhov-1980
commented
Mar 26, 2026
run feut |
iaorekhov-1980
commented
Mar 26, 2026
run feut |
iaorekhov-1980
commented
Mar 26, 2026
run feut |
iaorekhov-1980
commented
Mar 26, 2026
run feut |
iaorekhov-1980
commented
Mar 27, 2026
run feut |
iaorekhov-1980
commented
Mar 27, 2026
run feut |
iaorekhov-1980
commented
Aug 14, 2026
run feut |
47d78e1 to
aea784fCompareiaorekhov-1980
commented
Aug 14, 2026
run feut |
hello-stephen
commented
Aug 14, 2026
FE UT Coverage ReportIncrement line coverage |
aea784f to
74b527dCompareiaorekhov-1980
commented
Aug 14, 2026
run buildall |
hello-stephen
commented
Aug 14, 2026
TPC-H: Total hot run time: 17494 ms |
hello-stephen
commented
Aug 14, 2026
TPC-DS: Total hot run time: 85501 ms |
hello-stephen
commented
Aug 14, 2026
ClickBench: Total hot run time: 14.86 s |
iaorekhov-1980
commented
Aug 16, 2026
Hello, @morningman |
74b527d to
804326aCompareiaorekhov-1980
commented
Aug 17, 2026
run buildall |
iaorekhov-1980
commented
Aug 17, 2026
run compile |
Address the review findings on the empty-password LDAP guard: conf/ldap.conf: the commented-out line declared `ldap_allow_empty_pass = true` while the shipped default is `false`, and the surrounding text still described the abandoned "default true" design. In a Doris conf file the commented `# key = value` line *is* the declaration of the default, so an operator locked out after the upgrade would read the one document that explains this switch and be told the hole is already open. Rewrite it to match the code, state that the config is not runtime-mutable, and name the mechanism instead of the "4.1" version tag, which cannot be verified in-tree and would be wrong once this change is picked to branch-4.0 / branch-3.1. LdapConfig: expand the javadoc so it says the same thing as conf/ldap.conf. LdapManager: the comment above the guard had the condition backwards - it said login is disabled "in case when specific property is true", while the rejection actually happens when the property is false. Replace it with an explanation of why the check has to run before the cached-password comparison and the LDAP bind. Inline isEmptyPasswordLoginAllowed(): it returned true for any non-empty password, so the predicate did not describe its own contract, and its Strings.isNullOrEmpty() re-tested a null that checkUserPasswd() has already rejected a few lines above. LdapManagerTest: testCachedEmptyPasswordIsRejectedAfterFlagDisabled neither disabled the flag nor asserted any rejection - it re-ran the "allowed" scenario under a name that claimed otherwise, and the comment belonging to its second half was left stranded outside any method body. Complete it. Add testEmptyPasswordIsRejectedBeforeCacheLookup to pin the ordering invariant the whole fix rests on: the cached-password comparison returns true on plain string equality without consulting the flag, so the guard is the only barrier between a cached empty password and a successful login, and nothing was asserting that getUserInfo() is not reached first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
00a6a24 to
3ee24dfComparemorningman
commented
Aug 17, 2026
Local pipeline review — ✅ PASSschema: doris-repo-review/v1status: PASSpr: apache/doris#61440commit: 3ee24dfcc664baf0ff8ee470ad2e2d93c8098241base: 168d07778338761de5da5f0e97c36eee1e265b4breviewed_at: 2026-08-17T23:19+08:00reviewer: morningmanmodel: claude-opus-5[1m]effort: maxfindings: {blocker: 0, major: 0, minor: 2, nit: 4}rounds: 2converged: falseNotes for maintainers
Reviewed locally with the |
morningman
commented
Aug 18, 2026
run buildall |
hello-stephen
commented
Aug 18, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Aug 18, 2026
TPC-H: Total hot run time: 17025 ms |
hello-stephen
commented
Aug 18, 2026
TPC-DS: Total hot run time: 80940 ms |
What problem does this PR solve?
This PR adds new configuration property ldap_allow_empty_pass to prohibit option for existing user to login into LDAP with empty password.
It doesn't impact new approach from #60407 , because since 4.1.x new LDAP plugin explicitly prohibits login with empty pass.
But in legacy version - 3.1.x and 4.0.x such option is still available.
If ldap_allow_empty_pass in ldap.conf is not specified or specified as false - user can't login with empty pass (new behavior, aligned with newly introduced plugin authentication).
If ldap_allow_empty_pass specified as true - login attempt with empty password will be possible as legacy behavior.
Could you please include this PR into 4.x branch, please!
Issue Number: close#60353
Related PR: #xxx
Problem Summary:
Currently for existing user it is possible to login into LDAP with empty password.
New configuration property disables such option, but user is still able to activate legacy behavior and allow login without specified password.
Release note
New ldap_allow_empty_pass property for legacy authentication approach was introduced into ldap.conf to prohibit login with empty LDAP password as it is allowed by LDAP protocol by default.
Check List (For Author)
Test
Behavior changed:
3.1 user has specified empty password
3.2 property ldap_allow_empty_pass is false and doesn't allow to login with empty password
If both conditions met - authentication is failed and false is returning, as by other check in checkUserPassword
Check List (For Reviewer who merge this PR)