Skip to content

Conversation

@iaorekhov-1980
Copy link

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

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  1. ldap.conf and LdapConfig.java - new configuration ldap_allow_empty_pass property with default value true to keep existing behavior as default
  2. ErrorCode.java - specific error message for case with empty password was added
  3. LdapAuthenticator.java and Auth.java - additional check was added to validate two conditions
    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.
  4. LdapAuthenticatorTest.java - introduced new test method to validate existing behavior (without specified ldap_allow_empty_pass property or true) and new one (with ldap_use_ssl property specified to false) to check that login is still successful in first case and failed in the second one.
  • Does this need documentation?
    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen
Copy link
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@iaorekhov-1980
Copy link
Author

run feut

@iaorekhov-1980
Copy link
Author

run feut

@iaorekhov-1980
Copy link
Author

run feut

@iaorekhov-1980
Copy link
Author

run buildall

@doris-robot
Copy link

TPC-H: Total hot run time: 31646 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 6ea7c8c998ec1172e13c20767d1f480cb66c8e83, data reload: false

------ Round 1 ----------------------------------
q1	17610	5254	5023	5023
q2	2075	308	184	184
q3	10196	1421	744	744
q4	10207	821	319	319
q5	7548	2194	1865	1865
q6	204	187	150	150
q7	912	723	627	627
q8	9284	1395	1053	1053
q9	5221	4880	4812	4812
q10	6824	1964	1565	1565
q11	543	302	272	272
q12	343	374	227	227
q13	17787	4028	3235	3235
q14	240	239	222	222
q15	898	824	815	815
q16	698	675	607	607
q17	628	757	552	552
q18	6702	6378	6408	6378
q19	1256	982	624	624
q20	405	349	223	223
q21	2648	1954	1880	1880
q22	356	313	269	269
Total cold run time: 102585 ms
Total hot run time: 31646 ms

----- Round 2, with runtime_filter_mode=off -----
q1	5300	5297	5308	5297
q2	258	328	259	259
q3	2163	2678	2255	2255
q4	1390	1743	1303	1303
q5	4410	4319	4390	4319
q6	211	184	141	141
q7	2284	2160	1974	1974
q8	2627	2390	2445	2390
q9	7513	7556	7659	7556
q10	2829	3101	2748	2748
q11	577	483	478	478
q12	690	746	601	601
q13	3828	4551	3606	3606
q14	299	330	304	304
q15	864	818	867	818
q16	674	722	700	700
q17	1111	1329	1349	1329
q18	8042	8009	7807	7807
q19	926	930	917	917
q20	2156	2169	1930	1930
q21	4513	4133	4028	4028
q22	590	537	520	520
Total cold run time: 53255 ms
Total hot run time: 51280 ms

@doris-robot
Copy link

ClickBench: Total hot run time: 28.63 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 6ea7c8c998ec1172e13c20767d1f480cb66c8e83, data reload: false

query1	0.06	0.05	0.05
query2	0.10	0.05	0.04
query3	0.26	0.09	0.08
query4	1.60	0.11	0.11
query5	0.26	0.24	0.25
query6	1.16	0.67	0.66
query7	0.03	0.03	0.03
query8	0.05	0.04	0.04
query9	0.56	0.48	0.49
query10	0.55	0.55	0.55
query11	0.14	0.09	0.10
query12	0.14	0.10	0.10
query13	0.63	0.61	0.61
query14	1.07	1.06	1.07
query15	0.89	0.86	0.87
query16	0.37	0.38	0.43
query17	1.12	1.15	1.14
query18	0.23	0.21	0.22
query19	2.10	2.04	2.06
query20	0.03	0.02	0.01
query21	15.42	0.28	0.15
query22	5.12	0.05	0.05
query23	15.89	0.27	0.10
query24	1.02	0.67	0.53
query25	0.09	0.10	0.05
query26	0.13	0.14	0.13
query27	0.07	0.06	0.05
query28	4.66	1.13	0.96
query29	12.61	3.95	3.16
query30	0.28	0.14	0.11
query31	2.81	0.64	0.39
query32	3.26	0.59	0.50
query33	3.27	3.24	3.32
query34	16.06	5.40	4.76
query35	4.78	4.84	4.81
query36	0.64	0.50	0.48
query37	0.12	0.07	0.07
query38	0.07	0.04	0.04
query39	0.05	0.03	0.03
query40	0.19	0.16	0.14
query41	0.09	0.04	0.03
query42	0.04	0.03	0.03
query43	0.05	0.04	0.04
Total cold run time: 98.07 s
Total hot run time: 28.63 s

@hello-stephen
Copy link
Contributor

FE UT Coverage Report

Increment line coverage 50.00% (6/12) 🎉
Increment coverage report
Complete coverage report

@hello-stephen
Copy link
Contributor

FE Regression Coverage Report

Increment line coverage 8.33% (1/12) 🎉
Increment coverage report
Complete coverage report

@iaorekhov-1980
Copy link
Author

run nonCurrent

2 similar comments
@iaorekhov-1980
Copy link
Author

run nonCurrent

@iaorekhov-1980
Copy link
Author

run nonCurrent

@iaorekhov-1980
Copy link
Author

run p0

@iaorekhov-1980
Copy link
Author

run nonConcurrent

@hello-stephen
Copy link
Contributor

FE Regression Coverage Report

Increment line coverage 8.33% (1/12) 🎉
Increment coverage report
Complete coverage report

1 similar comment
@hello-stephen
Copy link
Contributor

FE Regression Coverage Report

Increment line coverage 8.33% (1/12) 🎉
Increment coverage report
Complete coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] (auth) add configuration to support disable of login with empty LDAP password

3 participants