[enhance](auth) introduction of configuration property to prohibit login with empty LDAP password#61440
Open
iaorekhov-1980 wants to merge 6 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
iaorekhov-1980
changed the title
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 27089 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 169028 ms |
Contributor
Author
|
run external |
Contributor
Author
|
run nonConcurrent |
iaorekhov-1980
marked this pull request as ready for review
March 18, 2026 09:15
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
March 23, 2026 07:38
8fa5633 to
629cab0
Compare
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 26705 ms |
TPC-DS: Total hot run time: 167913 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run external |
1 similar comment
Contributor
Author
|
run external |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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.
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
2 times, most recently
from
March 26, 2026 11:17
aaa284b to
267c2e3
Compare
Contributor
Author
|
run feut |
8 similar comments
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
July 14, 2026 13:02
9c31d8e to
7e04059
Compare
Contributor
Author
|
run feut |
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
July 14, 2026 14:37
7e04059 to
ba374c8
Compare
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29149 ms |
Contributor
TPC-DS: Total hot run time: 178006 ms |
Contributor
ClickBench: Total hot run time: 24.93 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
Hello, @CalvinKirs Could you please check my PR as I've fixed and answered comments from your review. Thanks in advance! |
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
July 16, 2026 07:40
ba374c8 to
a4a8562
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29103 ms |
Contributor
TPC-DS: Total hot run time: 178313 ms |
Contributor
ClickBench: Total hot run time: 24.97 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run feut |
Member
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for three correctness/security issues in the legacy LDAP authentication update. The review is complete for head a4a85622acc8c6983ed57ef6536368e15fb453a6: three review rounds converged, including separate whole-diff and authentication-risk passes, and the final live-thread sweep found no duplicate of these comments. No additional user review focus was provided.
Critical-checkpoint conclusions:
- Goal and proof: the central
LdapManager.checkUserPasswdcheck does block an empty string before cached-password acceptance and LDAP bind when the startup flag is false, and no supported legacy caller bypass was found. The added tests do not exercise that production chokepoint or either changed caller, so they do not prove the shipped behavior. - Scope and clarity: the nine-file patch is focused on a restart-scoped legacy-LDAP policy plus plugin compatibility tests. The plugin remains behaviorally independent and rejects zero-length credentials itself; its
fe-commondependency is test-scoped. - Concurrency and lifecycle:
ldap_allow_empty_passis loaded fromldap.confbefore FE services start, is non-mutable in production, and is read concurrently without writes. No lock, deadlock, static-initialization, resource-release, or ownership issue was found. - Configuration and compatibility: the setting follows the existing per-FE startup lifecycle; mixed-version/restart behavior is local to each FE. There is no storage format, serialized state, function-symbol, EditLog, transaction, or FE-BE protocol change. The insecure default is already covered by existing review discussion and is not duplicated here.
- Parallel paths and conditions: all legacy MySQL and plain-password entry paths reach the gated overload chain. Native fallback, optional authentication-chain success, certificate-authenticated paths, and the newer LDAP plugin are separate mechanisms rather than gate bypasses.
- Error handling and security: the boolean manager result loses the rejection cause, so both callers can issue the new policy error for ordinary failures. The new LDAP-specific failure text also creates a client-visible pre-auth LDAP-membership distinction, which is in scope under the repository threat model.
- Tests and results: production gate ordering, flag-true versus flag-false failure semantics, both changed callers, and response neutrality are not covered. Live compile, CheckStyle, FE unit tests, regressions, and performance checks pass; FE coverage fails and the automated review check remains pending. Per the review contract, no local build was run;
.worktree_initializedandthirdparty/installed/bin/protocare absent, so validation was static plus live CI. - Observability and performance: the rejection log is consistent with existing authentication logging, and the constant-time check avoids cache/LDAP work when disabled. No additional metric or material CPU/memory issue was found.
- Transactions, persistence, and data writes: not applicable; the patch changes no persisted metadata, master-failover path, data mutation, or result file.
Please address the three inline findings before approval.
| throw new AuthenticationException(ErrorCode.ERR_EMPTY_PASSWORD, remoteUser + "@" + remoteHost); | ||
| } else { | ||
| throw new AuthenticationException(ErrorCode.ERR_ACCESS_DENIED_ERROR, | ||
| remoteUser + "@" + remoteHost + " via LDAP", |
Contributor
There was a problem hiding this comment.
[P1] Keep failed authentication responses backend-neutral
This branch is reached only after doesUserExist(remoteUser) resolved the name in LDAP. Adding via LDAP therefore makes every nonempty bad password return a different client-visible message from a nonexistent or native user. BaseController returns formatErrMsg() in the HTTP unauthorized response body, and Arrow Flight includes e.getMessage() in its UNAUTHENTICATED description, so an unauthenticated caller can enumerate LDAP-backed accounts. Please keep the client response identical to the generic access-denied form and put the selected backend only in server-side debug/audit logs; the LDAP-only empty-password response needs the same neutrality.
| LOG.debug("internalAuthenticate: user={}, success=false", userName); | ||
| } | ||
| ErrorReport.report(ErrorCode.ERR_ACCESS_DENIED_ERROR, qualifiedUser, remoteIp, usePasswd); | ||
| if (Strings.isNullOrEmpty(password)) { |
Contributor
There was a problem hiding this comment.
[P2] Report the policy error only when the policy rejected the login
checkUserPasswd returns false for the null precondition, missing users, LDAP bind rejection, and operational LDAP failures as well as for the new policy gate. This branch looks only at the supplied password, so with the default ldap_allow_empty_pass=true an LDAP server that rejects an empty bind still produces error 6001 saying empty passwords are prohibited and advising the operator to enable an already-enabled flag. Auth.checkPlainPassword has the same branch (and Arrow Flight explicitly converts its empty-password sentinel to null). Please preserve a typed failure reason, or at minimum require !LdapConfig.ldap_allow_empty_pass before emitting ERR_EMPTY_PASSWORD; ordinary failures should retain their existing error.
| @Test | ||
| public void testUserLoginWithEmptyLDAPPasswordDefault() { | ||
| LdapManager ldapManager = new LdapManager(); | ||
| Assert.assertTrue(ldapManager.checkLoginWithEmptyPasswordForLdapIsAllowed("username", null)); |
Contributor
There was a problem hiding this comment.
[P1] Exercise the production empty-password chokepoint
These new tests call the helper directly, so they still pass if its invocation is deleted from LdapManager.checkUserPasswd or moved after the cached-password/LDAP-bind success paths. The existing manager test uses only nonempty passwords, LdapAuthenticatorTest mocks checkUserPasswd, and no Auth.checkPlainPassword test is changed despite the PR description claiming both caller suites. Please test an empty credential through checkUserPasswd with cached/client success under both flag values, assert the disabled case returns before those success paths, and cover both callers. Those caller assertions should distinguish a flag-disabled policy rejection from a flag-enabled empty-bind failure and keep LDAP-present/nonempty failures message-neutral with LDAP-absent/native failures.
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
July 17, 2026 15:13
2631282 to
29a1361
Compare
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
July 17, 2026 15:18
29a1361 to
00b5785
Compare
Contributor
Author
|
run feut |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 for legacy approach.
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 true - user can login with empty pass (existing behavior).
If ldap_allow_empty_pass specified as false - login attempt with empty password will be rejected with corresponding error message.
Could you please include this PR into 4.x and 3.1.x branches, 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 default behavior still allows to 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)