[enhance](auth) introduction of configuration property to prohibit login with empty LDAP password#61440
Open
iaorekhov-1980 wants to merge 20 commits intoapache:masterfrom
Open
[enhance](auth) introduction of configuration property to prohibit login with empty LDAP password#61440iaorekhov-1980 wants to merge 20 commits intoapache:masterfrom
iaorekhov-1980 wants to merge 20 commits intoapache:masterfrom
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
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
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.
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapAuthenticator.java
Show resolved
Hide resolved
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
629cab0 to
aaa284b
Compare
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
aaa284b to
267c2e3
Compare
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
Contributor
Author
|
run feut |
1 similar comment
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 |
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
5 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.
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 was introduced into ldap.conf to prohibit login with empty or null 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 new error is returned.
Check List (For Reviewer who merge this PR)