github-actions[bot] commented on code in PR #61440:
URL: https://github.com/apache/doris/pull/61440#discussion_r2990722610
##########
fe/fe-common/src/main/java/org/apache/doris/common/ErrorCode.java:
##########
@@ -1233,7 +1233,10 @@ public enum ErrorCode {
ERR_NO_CLUSTER_ERROR(5099, new byte[]{'4', '2', '0', '0', '0'}, "No
compute group (cloud cluster) selected"),
ERR_NOT_CLOUD_MODE(6000, new byte[]{'4', '2', '0', '0', '0'},
- "Command only support in cloud mode.");
+ "Command only support in cloud mode."),
+
+ ERR_EMPTY_PASSWORD(6001, new byte[]{'4', '2', '0', '0', '0'},
+ "Access with empty password is prohibited for user %s because of
current mode");
Review 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:
```java
"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.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapAuthenticator.java:
##########
@@ -21,6 +21,7 @@
import org.apache.doris.catalog.Env;
import org.apache.doris.common.ErrorCode;
import org.apache.doris.common.ErrorReport;
+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;
Review 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:
```java
LOG.info("user:{} login rejected: empty LDAP password is prohibited
(ldap_allow_empty_pass=false)", userName);
```
##########
fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java:
##########
@@ -175,4 +175,10 @@ public class LdapConfig extends ConfigBase {
public static String getConnectionURL(String hostPortInAccessibleFormat) {
return ((LdapConfig.ldap_use_ssl ? "ldaps" : "ldap") + "://" +
hostPortInAccessibleFormat);
}
+
+ /**
+ * Flag to enable login with empty pass.
+ */
+ @ConfigBase.ConfField
+ public static boolean ldap_allow_empty_pass = true;
Review Comment:
**[Suggestion]** This config should be `mutable = true` so it can be toggled
at runtime without restarting FE:
```java
@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.
--
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]
