[enhance](auth) introduction of configuration property to prohibit login with empty LDAP password#61440
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
iaorekhov-1980
changed the title
|
run buildall |
TPC-H: Total hot run time: 27089 ms |
FE UT Coverage ReportIncrement line coverage |
TPC-DS: Total hot run time: 169028 ms |
|
run external |
|
run nonConcurrent |
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
8fa5633 to
629cab0
Compare
|
run buildall |
TPC-H: Total hot run time: 26705 ms |
TPC-DS: Total hot run time: 167913 ms |
FE UT Coverage ReportIncrement line coverage |
|
run external |
1 similar comment
|
run external |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
| * Flag to enable login with empty pass. | ||
| */ | ||
| @ConfigBase.ConfField | ||
| public static boolean ldap_allow_empty_pass = true; |
There was a problem hiding this comment.
[Suggestion] This config should be mutable = true so it can be toggled at runtime without restarting FE:
@ConfigBase.ConfField(mutable = true)Rationale: This is a security policy toggle checked at authentication time with no cached state or initialization dependency. An admin discovering empty-password logins as a security risk would want to disable this immediately without restarting FE (which disconnects all users). The precedent in this file (ldap_user_cache_timeout_s, ldap_cache_timeout_day) uses mutable = true for runtime-tunable configs, and this config is an even stronger candidate since it's a pure runtime check.
|
|
||
| ERR_EMPTY_PASSWORD(6001, new byte[]{'4', '2', '0', '0', '0'}, | ||
| "Access with empty password is prohibited for user %s because of current mode"); | ||
|
|
There was a problem hiding this comment.
[Suggestion] The phrase "because of current mode" is vague and doesn't help the user understand what to do. Consider a more specific message:
"Access with empty password is prohibited for LDAP user '%s'. Set ldap_allow_empty_pass=true to allow."This tells the user both what happened and how to resolve it.
| import org.apache.doris.common.LdapConfig; | ||
| import org.apache.doris.mysql.authenticate.AuthenticateRequest; | ||
| import org.apache.doris.mysql.authenticate.AuthenticateResponse; | ||
| import org.apache.doris.mysql.authenticate.Authenticator; |
There was a problem hiding this comment.
[Nit] The log message includes ldap_allow_empty_pass:{} but since this branch is only entered when ldap_allow_empty_pass is false, the logged value is always false — it adds no information. Consider simplifying:
LOG.info("user:{} login rejected: empty LDAP password is prohibited (ldap_allow_empty_pass=false)", userName);5 participants
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
None
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)