[enhance](auth) introduction of configuration property to prohibit login with empty LDAP password#61440
[enhance](auth) introduction of configuration property to prohibit login with empty LDAP password#61440iaorekhov-1980 wants to merge 47 commits intoapache:masterfrom
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.
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
2 times, most recently
from
aaa284b to
267c2e3
Compare
|
run feut |
8 similar comments
|
run feut |
|
run feut |
|
run feut |
|
run feut |
|
run feut |
|
run feut |
|
run feut |
|
run feut |
iaorekhov-1980
force-pushed
the
feat/disable_ldap_empty_pass
branch
from
94da28d to
17d3d1b
Compare
|
run feut |
|
run feut |
|
/review |
There was a problem hiding this comment.
Found 1 blocking issue.\n\n1. fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java: marking as is not implemented end-to-end. FE startup only loads LDAP settings from ( calls only), while persisted mutable config updates are written through into . That means an operator can disable empty LDAP passwords at runtime, persist the change successfully, and still silently revert to the insecure default after an FE restart. For a security control, that is a correctness bug. Either keep this setting file-only (remove ) or add a real custom LDAP config load/persist path.\n\nCritical checkpoints:\n- Goal / test: Partially accomplished. The legacy empty-password checks were added in both legacy auth entry points and unit tests cover allow/deny behavior, but the new mutable-config contract is broken across restart. I could not run the FE tests in this runner because is missing.\n- Small / focused: Yes, aside from the mutable-config exposure.\n- Concurrency: No new lock-order or thread-safety issue found in the auth branches.\n- Lifecycle / config: Not OK for the new config; persisted runtime updates are not reloaded.\n- Compatibility: No FE/BE protocol or storage compatibility issue found.\n- Parallel paths: Legacy LDAP paths were updated consistently; the plugin-based LDAP path intentionally remains unchanged and is tested.\n- Special checks: The empty-password guard itself is straightforward.\n- Test coverage: Good functional unit coverage for allow/deny paths, but missing coverage for persist/restart behavior.\n- Test results changed: Not verified in this runner because FE UT prerequisites are missing.\n- Observability: Adequate for this small auth change.\n- Transaction / persistence: The new config persistence lifecycle is incorrect.\n- Data writes / atomicity: N/A.\n- FE/BE variable passing: N/A.\n- Performance: No meaningful regression found.\n- Other issues: None beyond the config persistence bug.
There was a problem hiding this comment.
Found 1 blocking issue.
fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java: markingldap_allow_empty_passasmutable = trueis not implemented end-to-end. FE startup only loads LDAP settings fromconf/ldap.conf(DorisFE.start()callsLdapConfig.init(...)only), while persisted mutable config updates are written throughConfigBase.persistConfig(...)intofe_custom.conf. That means an operator can disable empty LDAP passwords at runtime, persist the change successfully, and still silently revert to the insecure default after an FE restart. For a security control, that is a correctness bug. Either keep this setting file-only (removemutable = true) or add a real custom LDAP config load/persist path.
Critical checkpoints:
- Goal / test: Partially accomplished. The legacy empty-password checks were added in both legacy auth entry points and unit tests cover allow/deny behavior, but the new mutable-config contract is broken across restart. I could not run the FE tests in this runner because
thirdparty/installed/bin/protocis missing. - Small / focused: Yes, aside from the mutable-config exposure.
- Concurrency: No new lock-order or thread-safety issue found in the auth branches.
- Lifecycle / config: Not OK for the new config; persisted runtime updates are not reloaded.
- Compatibility: No FE/BE protocol or storage compatibility issue found.
- Parallel paths: Legacy LDAP paths were updated consistently; the plugin-based LDAP path intentionally remains unchanged and is tested.
- Special checks: The empty-password guard itself is straightforward.
- Test coverage: Good functional unit coverage for allow/deny paths, but missing coverage for persist/restart behavior.
- Test results changed: Not verified in this runner because FE UT prerequisites are missing.
- Observability: Adequate for this small auth change.
- Transaction / persistence: The new config persistence lifecycle is incorrect.
- Data writes / atomicity: N/A.
- FE/BE variable passing: N/A.
- Performance: No meaningful regression found.
- Other issues: None beyond the config persistence bug.
|
/review |
There was a problem hiding this comment.
No blocking findings.
Critical checkpoints:
- Goal and correctness: The PR goal is to let legacy LDAP auth reject empty passwords via a new
ldap_allow_empty_passconfig while preserving current behavior by default. The implementation does that in both legacy entry points:LdapAuthenticator.internalAuthenticate()andAuth.checkPlainPassword(). The plugin-based LDAP path remains intentionally unchanged. - Scope and focus: The change is small and focused: one new config, one new error code, two legacy auth guards, and targeted tests.
- Concurrency and locking: No new concurrent state or lock ordering changes were introduced. The added checks are simple reads of existing static config before the LDAP call.
- Lifecycle and initialization: No special lifecycle risk found. The new option follows the existing
LdapConfiginitialization path fromconf/ldap.conf. - Configuration: A new LDAP config item is added and wired correctly. Its startup-loaded behavior is consistent with the surrounding LDAP config model.
- Compatibility: No FE/BE protocol, storage-format, or rolling-upgrade compatibility issue is involved.
- Parallel code paths: The two legacy plain-password LDAP paths were both updated, and the newer plugin-based path is explicitly covered by tests to confirm it is unaffected.
- Conditional logic: The new condition is straightforward and placed before the LDAP bind, which avoids unnecessary LDAP work on rejected empty-password attempts.
- Test coverage: Coverage is reasonable for this scope: legacy
LdapAuthenticatortests, legacyAuth.checkPlainPassword()tests, and plugin integration tests showing the new config does not affect the plugin path. Residual gap: I did not see an end-to-end MySQL handshake test that asserts the exact client-visible error text forERR_EMPTY_PASSWORD. - Observability: Rejected attempts are logged and the legacy MySQL path sets a specific FE error code/message.
- Transactions / persistence / FE-BE variable passing / data-write correctness: Not applicable to this PR.
- Performance: The new guard is cheap and slightly improves performance for the rejected case by skipping the LDAP password check.
- Other issues: None blocking found in the reviewed paths.
Summary opinion: The PR is good to merge from a code-review perspective.
|
run feut |
|
run buildall |
iaorekhov-1980
left a comment
iaorekhov-1980
left a comment
There was a problem hiding this comment.
I've introduced all changes required in this and subsequent reviews.
iaorekhov-1980
left a comment
iaorekhov-1980
left a comment
There was a problem hiding this comment.
I've introduced all required changes to resolved issues, discovered during review
|
/approve |
FE UT Coverage ReportIncrement line coverage |
|
Hello, @morningman! |
FE Regression Coverage ReportIncrement line coverage |
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 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 new error is reported.
Check List (For Reviewer who merge this PR)