[enhance](auth) introduction of ldaps support via configuration property#60275
[enhance](auth) introduction of ldaps support via configuration property#60275iaorekhov-1980 wants to merge 12 commits intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
e6954e3 to
59a60c2
Compare
|
run buildall |
|
run feut |
1 similar comment
|
run feut |
2dc20f8 to
2cc92da
Compare
|
run feut |
1 similar comment
|
run feut |
4452270 to
c73cc70
Compare
|
run feut |
1 similar comment
|
run feut |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
TPC-H: Total hot run time: 33048 ms |
ClickBench: Total hot run time: 28.47 s |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
run p0 |
TPC-H: Total hot run time: 32785 ms |
ClickBench: Total hot run time: 28.41 s |
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
|
run cloud_p0 |
FE Regression Coverage ReportIncrement line coverage |
|
run p0 |
FE Regression Coverage ReportIncrement line coverage |
|
run nonConcurrent |
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
|
run p0 |
FE Regression Coverage ReportIncrement line coverage |
|
run p0 |
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
|
Hi, this PR is ready for review. All CI checks have passed. Looking forward to your feedback. |
There was a problem hiding this comment.
Pull request overview
This PR introduces support for LDAPS (LDAP over SSL) by adding a new configuration property ldap_use_ssl to enable secure connections to LDAP servers. When enabled, the connection URL uses the "ldaps://" protocol instead of "ldap://", maintaining backward compatibility with the default value of false.
Changes:
- Added
ldap_use_sslconfiguration property with default valuefalseto maintain backward compatibility - Extracted URL connection string construction logic into a new static method
getConnectionURL()inLdapConfigto support both LDAP and LDAPS protocols - Updated both LDAP connection creation points in
LdapClientto use the new method instead of hardcoded "ldap://" protocol
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| conf/ldap.conf | Added documentation for the new ldap_use_ssl configuration property |
| fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java | Added ldap_use_ssl property and getConnectionURL() method to dynamically construct connection URLs |
| fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapClient.java | Replaced hardcoded "ldap://" URLs with calls to LdapConfig.getConnectionURL() in both pooled and non-pooled connection methods |
| fe/fe-core/src/test/java/org/apache/doris/mysql/authenticate/ldap/LdapClientTest.java | Added unit test to verify correct URL generation for both LDAP and LDAPS protocols, with proper cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java
Outdated
Show resolved
Hide resolved
fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/test/java/org/apache/doris/mysql/authenticate/ldap/LdapClientTest.java
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
run feut |
|
run buildall |
TPC-H: Total hot run time: 31275 ms |
ClickBench: Total hot run time: 28.41 s |
FE UT Coverage ReportIncrement line coverage |
|
run p0 |
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
|
Hello, @morningman |
| * @param hostPortInAccessibleFormat the host and port in accessible format (for example, "host:port") | ||
| * @return the LDAP or LDAPS connection URL string | ||
| */ | ||
| public static String getConnectionURL(String hostPortInAccessibleFormat) { |
There was a problem hiding this comment.
where to load/find certificate and CA when ldaps is enabled?
There was a problem hiding this comment.
pls also update the corresponding docs
There was a problem hiding this comment.
Hi. @gavinchou
The information about certificates location is provided within JVM arguments in fe.conf in standard parameter, like below
# For jdk 17, this JAVA_OPTS will be used as default JVM options
JAVA_OPTS_FOR_JDK_17="-Djavax.net.ssl.trustStore=/opt/apache-doris/certs/cacerts -Dfile.encoding=UTF-8 -Djavax.security.auth.useSubjectCredsOnly=false -Xmx8192m -Xms2048m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=$LOG_DIR -Xlog:gc
*,classhisto*=trace:$LOG_DIR/fe.gc.log.$CUR_DATE:time,uptime:filecount=10,filesize=50M --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens java.base/jdk.internal.ref=ALL-UNNAMED"
Also could you please confirm the documentation to be changed.
https://github.com/apache/doris-website/blob/master/docs/admin-manual/auth/authentication/ldap.md
I assume this file should be updated with new information about support of LDAPS?
There was a problem hiding this comment.
Hi, @gavinchou
I've update corresponding docs in apache/doris-website#3351
Could you please take a look?
There was a problem hiding this comment.
Hi, @gavinchou
Could you please continue review of this PR?
There was a problem hiding this comment.
Hello, @gavinchou
Could you please return to my PR as all comments have been solved
What problem does this PR solve?
This PR adds new configuration property ldap_use_ssl to enable usage of LDAPS to establish connection to LDAP instance.
If ldap_use_ssl in ldap.conf is specified as true - LDAPS is used to create connection string.
If ldap_use_ssl in ldap.conf is not specified or specified as false - LDAP is used to create connection string as now.
Could you please include this PR into 4.x and 3.1.x branches, please!
Issue Number: close #60236
Related PR: #xxx
Problem Summary:
Currently it is not possible to use LDAPS to create connection to secured LDAP instances.
New configuration property allows to connect to such instances, but default behavior still relies on LDAP as is.
Release note
None
Check List (For Author)
Test
Behavior changed:
Check List (For Reviewer who merge this PR)