github-actions[bot] commented on code in PR #61362:
URL: https://github.com/apache/doris/pull/61362#discussion_r2944104604
##########
regression-test/suites/auth_p0/test_authentication_integration_auth.groovy:
##########
@@ -30,48 +30,18 @@ suite("test_authentication_integration_auth", "p0,auth") {
exception "Property 'type' is required"
}
- sql """
- CREATE AUTHENTICATION INTEGRATION ${integrationName}
- PROPERTIES (
- 'type'='ldap',
- 'ldap.server'='ldap://127.0.0.1:389',
- 'ldap.admin_password'='123456'
- )
- COMMENT 'for regression test'
- """
-
test {
sql """
CREATE AUTHENTICATION INTEGRATION ${integrationName}
- PROPERTIES ('type'='ldap',
'ldap.server'='ldap://127.0.0.1:1389')
- """
- exception "already exists"
- }
-
- test {
- sql """
- ALTER AUTHENTICATION INTEGRATION ${integrationName}
- SET PROPERTIES ('type'='oidc')
+ PROPERTIES (
+ 'type'='ldap',
+ 'ldap.server'='ldap://127.0.0.1:389',
+ 'ldap.admin_password'='123456'
+ )
+ COMMENT 'for regression test'
"""
- exception "does not allow modifying property 'type'"
+ exception "No authentication plugin factory found for type: ldap"
}
-
- sql """
Review Comment:
**[MEDIUM] Significantly reduced regression test coverage**: The original
test covered the full CRUD lifecycle:
- CREATE (positive)
- Duplicate CREATE (error path)
- ALTER with disallowed type change (error path)
- ALTER properties (positive)
- ALTER COMMENT (positive)
- DROP nonexistent (error path)
- DROP IF EXISTS nonexistent (positive)
The new test only covers 2 negative paths (missing type property, plugin not
found).
I understand the positive-flow tests can't work without a deployed LDAP
plugin, but several error-path tests are plugin-independent and should be
preserved:
- Duplicate CREATE ("already exists") — call CREATE twice with the same bad
config; the duplicate-name check happens before plugin loading
- ALTER nonexistent integration
- DROP nonexistent integration (without IF EXISTS)
- DROP IF EXISTS nonexistent integration
These tests exercise the DDL error handling in
`AuthenticationIntegrationMgr` and don't require a working plugin.
##########
fe/fe-core/src/main/java/org/apache/doris/authentication/AuthenticationIntegrationMgr.java:
##########
@@ -73,40 +75,66 @@ public void createAuthenticationIntegration(
}
throw new DdlException("Authentication integration " +
integrationName + " already exists");
}
+ try {
+ prepared =
Env.getCurrentEnv().getAuthenticationIntegrationRuntime()
+ .prepareAuthenticationIntegration(meta);
Review Comment:
**[HIGH] Heavy I/O under write lock**:
`prepareAuthenticationIntegration(meta)` is called inside the `writeLock()`
section. This method can perform:
- Disk I/O: loading plugin JARs via
`AuthenticationPluginManager.createPlugin()`
- Network I/O: LDAP connection initialization via
`LdapContextSource.afterPropertiesSet()`
This blocks **all** readers (including `getAuthenticationIntegration()` used
in the authentication chain hot path) for the duration of potentially slow
operations.
The code already has a prepare/activate two-phase design, which is the right
approach — but the prepare should happen **outside** the lock:
```java
// Prepare OUTSIDE the lock (may do I/O)
PreparedAuthenticationIntegration prepared;
try {
prepared = runtime.prepareAuthenticationIntegration(meta);
} catch (AuthenticationException e) {
throw new DdlException(e.getMessage(), e);
}
writeLock();
try {
if (nameToIntegration.containsKey(integrationName)) { ... }
nameToIntegration.put(integrationName, meta);
runtime.activatePreparedAuthenticationIntegration(prepared);
} finally {
writeUnlock();
}
```
The same pattern applies to `alterAuthenticationIntegrationProperties()` and
`alterAuthenticationIntegrationUnsetProperties()` below.
Also: if `activatePreparedAuthenticationIntegration()` or
`nameToIntegration.put()` fails after prepare succeeds, the prepared resources
(initialized plugins with open connections) leak. Consider adding
`discardPreparedAuthenticationIntegration(prepared)` in a catch/finally block.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/AuthenticatorManager.java:
##########
@@ -96,26 +151,107 @@ public boolean authenticate(ConnectContext context,
MysqlSerializer serializer,
MysqlAuthPacket authPacket,
MysqlHandshakePacket handshakePacket) throws
IOException {
- Authenticator authenticator = chooseAuthenticator(userName);
- Optional<Password> password = authenticator.getPasswordResolver()
+ String remoteIp = context.getMysqlChannel().getRemoteIp();
+ Authenticator primaryAuthenticator = chooseAuthenticator(userName,
remoteIp);
+ Optional<Password> password =
primaryAuthenticator.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()) {
+ AuthenticateResponse primaryResponse =
+ primaryAuthenticator.authenticate(new
AuthenticateRequest(userName, password.get(), remoteIp));
+ if (primaryResponse.isSuccess()) {
+ applyAuthenticateResponse(context, remoteIp, primaryResponse);
+ return true;
+ }
+
+ AuthenticateResponse chainResponse =
tryAuthenticationChainFallback(context, userName, remoteIp,
+ channel, serializer, authPacket, handshakePacket,
password.get());
+ if (chainResponse != null && chainResponse.isSuccess()) {
+ context.getState().setOk();
+ applyAuthenticateResponse(context, remoteIp, chainResponse);
+ return true;
+ }
+
+ ensureAuthenticationErrorReported(context, userName, remoteIp,
password.get());
+ if (context.getState().getStateType() ==
QueryState.MysqlStateType.ERR) {
MysqlProto.sendResponsePacket(context);
- return false;
}
+ return false;
+ }
+
+ Authenticator chooseAuthenticator(String userName, String remoteIp) {
+ return authTypeAuthenticator.canDeal(userName) ? authTypeAuthenticator
: defaultAuthenticator;
+ }
Review Comment:
**[MEDIUM] TOCTOU race on volatile field**: `authTypeAuthenticator` is
`private static volatile` and is read **twice** in this expression — once for
`canDeal()` and once for the return value. Between the two reads, a concurrent
call to the constructor could swap the field to a different authenticator.
This means the method could return an authenticator that was never checked
with `canDeal()`.
Fix by reading the volatile into a local variable:
```java
Authenticator chooseAuthenticator(String userName, String remoteIp) {
Authenticator primary = authTypeAuthenticator;
return primary.canDeal(userName) ? primary : defaultAuthenticator;
}
```
Note: the old code (line being deleted below) had the same issue, so this is
a pre-existing bug — but since you're rewriting the method, now would be a good
time to fix it.
--
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]
