github-actions[bot] commented on code in PR #61362:
URL: https://github.com/apache/doris/pull/61362#discussion_r2972836599
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/AuthenticatorManager.java:
##########
@@ -96,26 +152,144 @@ public boolean authenticate(ConnectContext context,
MysqlSerializer serializer,
MysqlAuthPacket authPacket,
MysqlHandshakePacket handshakePacket) throws
IOException {
- Authenticator authenticator = chooseAuthenticator(userName);
- Optional<Password> password = authenticator.getPasswordResolver()
- .resolvePassword(context, channel, serializer, authPacket,
handshakePacket);
- if (!password.isPresent()) {
- return false;
- }
String remoteIp = context.getMysqlChannel().getRemoteIp();
- AuthenticateRequest request = new AuthenticateRequest(userName,
password.get(), remoteIp);
- AuthenticateResponse response = authenticator.authenticate(request);
- if (!response.isSuccess()) {
- MysqlProto.sendResponsePacket(context);
+ Authenticator primaryAuthenticator = chooseAuthenticator(userName,
remoteIp);
+ Optional<AuthenticateRequest> primaryRequest =
resolveAuthenticateRequest(primaryAuthenticator, userName,
+ context, channel, serializer, authPacket, handshakePacket);
+ if (!primaryRequest.isPresent()) {
return false;
}
+
+ AuthenticateRequest request = primaryRequest.get();
+ remoteIp = request.getRemoteIp();
+ AuthenticateResponse primaryResponse =
authenticateWith(primaryAuthenticator, request);
+ if (primaryResponse.isSuccess()) {
+ return finishSuccessfulAuthentication(context, remoteIp,
primaryResponse, false);
+ }
Review Comment:
**[Low] New instance created on every failed auth attempt**:
`getAuthenticationChainAuthenticator()` creates a new
`AuthenticationIntegrationAuthenticator` on every call. While the constructor
is lightweight (just string parsing + `ClearPasswordResolver` allocation), this
runs on every failed primary authentication when `authentication_chain` is
configured. Consider caching the authenticator instance (invalidated when
`Config.authentication_chain` changes) for efficiency, especially under
authentication storms or brute-force attempts.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -2772,10 +2772,17 @@ public class Config extends ConfigBase {
@ConfField
public static boolean ignore_bdbje_log_checksum_read = false;
- @ConfField(description = {"Specifies the authentication type"},
- options = {"default", "ldap"})
+ @ConfField(description = {"指定 mysql 登录主认证器名称，可为内置认证器或认证插件名称",
+ "Specifies the primary MySQL authenticator name, either a built-in
authenticator or an "
+ + "authentication plugin name"},
+ options = {"default", "password", "ldap", "<plugin_name>"})
public static String authentication_type = "default";
+ @ConfField(description = {"指定主认证失败后使用的认证链，多个 integration 名称用逗号分隔",
+ "Specifies the authentication chain used after primary
authentication failure, multiple integration "
+ + "names are comma-separated"})
+ public static String authentication_chain = "";
+
Review Comment:
**[Low] Consider `mutable = true`**: This config is read on every
authentication attempt (in `AuthenticatorManager.hasAuthenticationChain()` and
`getAuthenticationChainAuthenticator()`), not just at startup. If the intent is
to allow operators to reconfigure the auth chain without restarting FE, this
should be marked `mutable = true`. If restart-required is intentional (for
safety), please add a comment explaining that design choice.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/plugin/AuthenticationPluginAuthenticator.java:
##########
@@ -0,0 +1,198 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.mysql.authenticate.plugin;
+
+import org.apache.doris.analysis.UserIdentity;
+import org.apache.doris.authentication.AuthenticationException;
+import org.apache.doris.authentication.AuthenticationFailureType;
+import org.apache.doris.authentication.AuthenticationIntegration;
+import org.apache.doris.authentication.AuthenticationRequest;
+import org.apache.doris.authentication.AuthenticationResult;
+import org.apache.doris.authentication.CredentialType;
+import org.apache.doris.authentication.handler.AuthenticationPluginManager;
+import org.apache.doris.authentication.spi.AuthenticationPlugin;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.Config;
+import org.apache.doris.mysql.authenticate.AuthenticateRequest;
+import org.apache.doris.mysql.authenticate.AuthenticateResponse;
+import org.apache.doris.mysql.authenticate.Authenticator;
+import org.apache.doris.mysql.authenticate.password.ClearPassword;
+import org.apache.doris.mysql.authenticate.password.ClearPasswordResolver;
+import org.apache.doris.mysql.authenticate.password.NativePassword;
+import org.apache.doris.mysql.authenticate.password.NativePasswordResolver;
+import org.apache.doris.mysql.authenticate.password.Password;
+import org.apache.doris.mysql.authenticate.password.PasswordResolver;
+import org.apache.doris.mysql.privilege.Auth;
+import org.apache.doris.plugin.PropertiesUtils;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Properties;
+
+/**
+ * Bridge authenticator that adapts an {@link
org.apache.doris.authentication.spi.AuthenticationPluginFactory}
+ * to the legacy MySQL {@link Authenticator} contract.
+ */
+public class AuthenticationPluginAuthenticator implements Authenticator {
+ private static final Logger LOG =
LogManager.getLogger(AuthenticationPluginAuthenticator.class);
+ private static final String CONFIG_INTEGRATION_NAME_PREFIX =
"__config_auth_type__:";
+
+ private final AuthenticationIntegration integration;
+ private final AuthenticationPlugin plugin;
+ private final PasswordResolver passwordResolver;
+
+ public AuthenticationPluginAuthenticator(String pluginType, Properties
initProps) throws AuthenticationException {
+ this(pluginType, PropertiesUtils.propertiesToMap(initProps), new
AuthenticationPluginManager());
+ }
+
+ AuthenticationPluginAuthenticator(String pluginType, Map<String, String>
initProps,
+ AuthenticationPluginManager pluginManager) throws
AuthenticationException {
+ Objects.requireNonNull(pluginType, "pluginType");
+ AuthenticationPluginManager resolvedPluginManager =
Objects.requireNonNull(pluginManager, "pluginManager");
+ ensurePluginFactoryLoaded(resolvedPluginManager, pluginType);
+ integration = AuthenticationIntegration.builder()
+ .name(CONFIG_INTEGRATION_NAME_PREFIX + pluginType)
+ .type(pluginType)
+ .properties(initProps == null ? Collections.emptyMap() :
initProps)
+ .build();
+ plugin = resolvedPluginManager.createPlugin(integration);
+ passwordResolver = plugin.requiresClearPassword() ? new
ClearPasswordResolver() : new NativePasswordResolver();
+ }
+
+ @Override
+ public AuthenticateResponse authenticate(AuthenticateRequest request)
throws IOException {
+ AuthenticationRequest pluginRequest = toPluginRequest(request);
+ if (!plugin.supports(pluginRequest)) {
+ return AuthenticateResponse.failedResponse;
+ }
+
+ AuthenticationResult result;
+ try {
+ result = plugin.authenticate(pluginRequest, integration);
+ } catch (AuthenticationException e) {
+ LOG.warn("Authentication plugin '{}' failed for user '{}': {}",
integration.getType(),
+ request.getUserName(), e.getMessage(), e);
+ return AuthenticateResponse.failedResponse;
+ }
+
+ if (result.isContinue()) {
+ LOG.warn("Authentication plugin '{}' returned CONTINUE for user
'{}', which is not supported",
+ integration.getType(), request.getUserName());
+ return AuthenticateResponse.failedResponse;
+ }
+ if (!result.isSuccess()) {
+ if (result.getException() != null) {
+ LOG.info("Authentication plugin '{}' rejected user '{}': {}",
integration.getType(),
+ request.getUserName(),
result.getException().getMessage());
+ }
+ return AuthenticateResponse.failedResponse;
+ }
+
+ return mapSuccessfulAuthentication(request.getUserName(),
request.getRemoteIp());
+ }
+
+ @Override
+ public boolean canDeal(String qualifiedUser) {
+ return !Auth.ROOT_USER.equals(qualifiedUser) &&
!Auth.ADMIN_USER.equals(qualifiedUser);
+ }
+
+ @Override
+ public PasswordResolver getPasswordResolver() {
+ return passwordResolver;
+ }
+
+ private AuthenticateResponse mapSuccessfulAuthentication(String
qualifiedUser, String remoteIp) {
+ List<UserIdentity> userIdentities =
+
Env.getCurrentEnv().getAuth().getUserIdentityForExternalAuth(qualifiedUser,
remoteIp);
+ if (!userIdentities.isEmpty()) {
+ return new AuthenticateResponse(true, userIdentities.get(0),
false);
Review Comment:
**[Medium] Code duplication**: This `mapSuccessfulAuthentication` method is
nearly identical to the one in
`AuthenticationIntegrationAuthenticator.mapSuccessfulAuthentication()`. Both
follow the same pattern:
1. Look up existing Doris user via `getUserIdentityForExternalAuth()`
2. If found, return success with that identity
3. If not found, check `enable_jit_user` property
4. If JIT enabled, create temp user; otherwise return failure
Per the review principles (Reuse First), this should be extracted into a
shared utility method (e.g., in a common helper class) to avoid maintaining the
same logic in two places.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Auth.java:
##########
@@ -272,6 +272,14 @@ public List<UserIdentity> getUserIdentityForLdap(String
remoteUser, String remot
return userManager.getUserIdentityUncheckPasswd(remoteUser,
remoteHost);
}
+ public List<UserIdentity> getUserIdentityForExternalAuth(String
remoteUser, String remoteHost) {
+ return userManager.getUserIdentityUncheckPasswd(remoteUser,
remoteHost);
+ }
+
Review Comment:
**[Low] Redundant method**: `getUserIdentityForExternalAuth()` has an
identical implementation to `getUserIdentityForLdap()` (both delegate to
`userManager.getUserIdentityUncheckPasswd(remoteUser, remoteHost)`). Consider
either:
1. Having the new code call `getUserIdentityForLdap()` directly (if the
semantics are truly the same), or
2. Adding a comment explaining why a separate method exists (e.g., for
future divergence where external auth may need different user resolution logic)
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/plugin/AuthenticationPluginAuthenticator.java:
##########
@@ -0,0 +1,198 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.mysql.authenticate.plugin;
+
+import org.apache.doris.analysis.UserIdentity;
+import org.apache.doris.authentication.AuthenticationException;
+import org.apache.doris.authentication.AuthenticationFailureType;
+import org.apache.doris.authentication.AuthenticationIntegration;
+import org.apache.doris.authentication.AuthenticationRequest;
+import org.apache.doris.authentication.AuthenticationResult;
+import org.apache.doris.authentication.CredentialType;
+import org.apache.doris.authentication.handler.AuthenticationPluginManager;
+import org.apache.doris.authentication.spi.AuthenticationPlugin;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.Config;
+import org.apache.doris.mysql.authenticate.AuthenticateRequest;
+import org.apache.doris.mysql.authenticate.AuthenticateResponse;
+import org.apache.doris.mysql.authenticate.Authenticator;
+import org.apache.doris.mysql.authenticate.password.ClearPassword;
+import org.apache.doris.mysql.authenticate.password.ClearPasswordResolver;
+import org.apache.doris.mysql.authenticate.password.NativePassword;
+import org.apache.doris.mysql.authenticate.password.NativePasswordResolver;
+import org.apache.doris.mysql.authenticate.password.Password;
+import org.apache.doris.mysql.authenticate.password.PasswordResolver;
+import org.apache.doris.mysql.privilege.Auth;
+import org.apache.doris.plugin.PropertiesUtils;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Properties;
+
+/**
+ * Bridge authenticator that adapts an {@link
org.apache.doris.authentication.spi.AuthenticationPluginFactory}
+ * to the legacy MySQL {@link Authenticator} contract.
+ */
+public class AuthenticationPluginAuthenticator implements Authenticator {
+ private static final Logger LOG =
LogManager.getLogger(AuthenticationPluginAuthenticator.class);
+ private static final String CONFIG_INTEGRATION_NAME_PREFIX =
"__config_auth_type__:";
+
+ private final AuthenticationIntegration integration;
+ private final AuthenticationPlugin plugin;
+ private final PasswordResolver passwordResolver;
+
+ public AuthenticationPluginAuthenticator(String pluginType, Properties
initProps) throws AuthenticationException {
+ this(pluginType, PropertiesUtils.propertiesToMap(initProps), new
AuthenticationPluginManager());
+ }
+
+ AuthenticationPluginAuthenticator(String pluginType, Map<String, String>
initProps,
+ AuthenticationPluginManager pluginManager) throws
AuthenticationException {
+ Objects.requireNonNull(pluginType, "pluginType");
+ AuthenticationPluginManager resolvedPluginManager =
Objects.requireNonNull(pluginManager, "pluginManager");
+ ensurePluginFactoryLoaded(resolvedPluginManager, pluginType);
+ integration = AuthenticationIntegration.builder()
+ .name(CONFIG_INTEGRATION_NAME_PREFIX + pluginType)
+ .type(pluginType)
+ .properties(initProps == null ? Collections.emptyMap() :
initProps)
+ .build();
+ plugin = resolvedPluginManager.createPlugin(integration);
+ passwordResolver = plugin.requiresClearPassword() ? new
ClearPasswordResolver() : new NativePasswordResolver();
+ }
+
+ @Override
+ public AuthenticateResponse authenticate(AuthenticateRequest request)
throws IOException {
+ AuthenticationRequest pluginRequest = toPluginRequest(request);
+ if (!plugin.supports(pluginRequest)) {
+ return AuthenticateResponse.failedResponse;
+ }
+
+ AuthenticationResult result;
+ try {
+ result = plugin.authenticate(pluginRequest, integration);
+ } catch (AuthenticationException e) {
+ LOG.warn("Authentication plugin '{}' failed for user '{}': {}",
integration.getType(),
+ request.getUserName(), e.getMessage(), e);
+ return AuthenticateResponse.failedResponse;
+ }
+
+ if (result.isContinue()) {
+ LOG.warn("Authentication plugin '{}' returned CONTINUE for user
'{}', which is not supported",
+ integration.getType(), request.getUserName());
+ return AuthenticateResponse.failedResponse;
+ }
+ if (!result.isSuccess()) {
+ if (result.getException() != null) {
+ LOG.info("Authentication plugin '{}' rejected user '{}': {}",
integration.getType(),
+ request.getUserName(),
result.getException().getMessage());
+ }
+ return AuthenticateResponse.failedResponse;
+ }
+
+ return mapSuccessfulAuthentication(request.getUserName(),
request.getRemoteIp());
+ }
+
+ @Override
+ public boolean canDeal(String qualifiedUser) {
+ return !Auth.ROOT_USER.equals(qualifiedUser) &&
!Auth.ADMIN_USER.equals(qualifiedUser);
+ }
+
+ @Override
+ public PasswordResolver getPasswordResolver() {
+ return passwordResolver;
+ }
+
+ private AuthenticateResponse mapSuccessfulAuthentication(String
qualifiedUser, String remoteIp) {
+ List<UserIdentity> userIdentities =
+
Env.getCurrentEnv().getAuth().getUserIdentityForExternalAuth(qualifiedUser,
remoteIp);
+ if (!userIdentities.isEmpty()) {
+ return new AuthenticateResponse(true, userIdentities.get(0),
false);
+ }
+ if (!Boolean.parseBoolean(integration.getProperty("enable_jit_user",
"false"))) {
+ LOG.info("Authentication plugin '{}' authenticated user '{}' but
JIT is disabled",
+ integration.getType(), qualifiedUser);
+ return AuthenticateResponse.failedResponse;
+ }
+ UserIdentity tempUserIdentity =
UserIdentity.createAnalyzedUserIdentWithIp(qualifiedUser, remoteIp);
+ return new AuthenticateResponse(true, tempUserIdentity, true);
+ }
+
+ private AuthenticationRequest toPluginRequest(AuthenticateRequest request)
{
+ AuthenticationRequest.Builder builder = AuthenticationRequest.builder()
+ .username(request.getUserName())
+ .remoteHost(request.getRemoteHost())
+ .remotePort(request.getRemotePort())
+ .clientType(request.getClientType() == null ? "mysql" :
request.getClientType());
+
+ if (!request.getProperties().isEmpty()) {
+ builder.properties(request.getProperties());
+ }
+ if (request.getCredentialType() != null) {
+ return builder.credentialType(request.getCredentialType())
+ .credential(request.getCredential())
+ .build();
+ }
+
+ // TODO(authentication): drop password fallback once protocol adapters
emit
+ // generic credentials for all plugin-based authenticators.
+ Password password = request.getPassword();
+ if (password instanceof ClearPassword) {
+ builder.credentialType(CredentialType.CLEAR_TEXT_PASSWORD)
+ .credential(((ClearPassword)
password).getPassword().getBytes(StandardCharsets.UTF_8));
+ } else if (password instanceof NativePassword) {
+ NativePassword nativePassword = (NativePassword) password;
+ builder.credentialType(CredentialType.MYSQL_NATIVE_PASSWORD)
+ .credential(nativePassword.getRemotePasswd())
+
.property(NativePasswordResolver.MYSQL_RANDOM_STRING_PROPERTY,
nativePassword.getRandomString());
+ } else {
+ throw new IllegalArgumentException("Unsupported password type: "
+ + (password == null ? "null" :
password.getClass().getName()));
+ }
+
+ return builder.build();
Review Comment:
**[Medium] Uncaught `IllegalArgumentException` can crash the connection
handler**: If `request.getCredentialType()` is null AND `request.getPassword()`
is null (or an unexpected `Password` subclass), this throws
`IllegalArgumentException`. This unchecked exception will propagate through
`authenticate()` -> `AuthenticatorManager.authenticateWith()` ->
`AuthenticatorManager.authenticate()`, where it is NOT caught (only
`IOException` is declared). This would crash the MySQL connection handler
thread rather than returning a clean authentication failure to the client.
Suggestion: Either catch this in `authenticate()` and convert to
`failedResponse`, or use a check-and-return-failure pattern instead of
throwing. Per Doris coding standards, assertion-style crashes are for invariant
violations, but receiving an unsupported password type from the protocol layer
is a plausible runtime condition, not a programming invariant.
--
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]
