github-actions[bot] commented on PR #60275: URL: https://github.com/apache/doris/pull/60275#issuecomment-4048724648
## Code Review Summary **PR:** [enhance](auth) introduction of ldaps support via configuration property ### Overall Assessment This is a clean, well-structured PR that adds LDAPS support through a new `ldap_use_ssl` configuration property. No blocking issues found. ### Review Checklist **Correctness:** Pass. The URL construction logic correctly switches between `ldap://` and `ldaps://` based on the flag. Both LDAP connection creation paths in `LdapClient.java` (`setLdapTemplateNoPool` and `setLdapTemplatePool`) are updated to use the centralized `LdapConfig.getConnectionURL()` method. No hardcoded `ldap://` strings remain in production code. **Backward Compatibility:** Pass. The `ldap_use_ssl` flag defaults to `false`, preserving existing behavior. No behavioral changes occur unless the user explicitly sets `ldap_use_ssl = true`. **Test Coverage:** Pass. The new `testSecuredProtocolIsUsed` test verifies both the default (`ldap://`) and SSL (`ldaps://`) URL generation. Proper cleanup in `@After`/`tearDown()` ensures test isolation. Existing tests are not impacted. **Design:** Pass. The centralized `getConnectionURL()` method in `LdapConfig` is the single source of truth for URL construction - clean and maintainable. **Configuration:** Pass. The config property is properly commented in `ldap.conf` with a helpful note about port 636 for LDAPS. ### Observations (Non-blocking) 1. **SSL trust store configuration:** When `ldap_use_ssl=true`, the LDAPS connection will rely entirely on the JVM's default trust store (`cacerts`). Users connecting to LDAP servers with self-signed or non-publicly-trusted certificates would need to manually import the certificate via JVM arguments (`-Djavax.net.ssl.trustStore`). This is standard behavior for Spring LDAP and acceptable for an initial implementation, but it may be worth documenting this requirement (e.g., in the conf comment or user-facing docs) so users understand how to configure trust for non-public CAs. 2. **Minor style note:** The outer parentheses in `return ((LdapConfig.ldap_use_ssl ? "ldaps" : "ldap") + "://" + hostPortInAccessibleFormat);` at `LdapConfig.java:176` are redundant - `return (LdapConfig.ldap_use_ssl ? "ldaps" : "ldap") + "://" + hostPortInAccessibleFormat;` would be sufficient. Very minor. **No issues requiring changes. The PR is ready for merge from a code quality perspective.** -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
