Mark LDAP user query timeout as incorrect login instead of disabling user immediately (#11220)

* Mark LDAP user query timeout as incorrect login instead of disabling user immediately

* code improvements
This commit is contained in:
Suresh Kumar Anaparti 2025-07-25 19:31:43 +05:30 committed by GitHub
parent 890386e949
commit ed6ee6b704
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 50 additions and 35 deletions

View File

@ -140,10 +140,11 @@ public class LdapListUsersCmd extends BaseListCmd {
try { try {
final List<LdapUser> users = _ldapManager.getUsers(domainId); final List<LdapUser> users = _ldapManager.getUsers(domainId);
ldapResponses = createLdapUserResponse(users); ldapResponses = createLdapUserResponse(users);
// now filter and annotate // now filter and annotate
ldapResponses = applyUserFilter(ldapResponses); ldapResponses = applyUserFilter(ldapResponses);
} catch (final NoLdapUserMatchingQueryException ex) { } catch (final NoLdapUserMatchingQueryException ex) {
// ok, we'll make do with the empty list ldapResponses = new ArrayList<LdapUserResponse>(); logger.debug(ex.getMessage());
// ok, we'll make do with the empty list
} finally { } finally {
response.setResponses(ldapResponses); response.setResponses(ldapResponses);
response.setResponseName(getCommandName()); response.setResponseName(getCommandName());

View File

@ -45,6 +45,8 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
@Inject @Inject
private AccountManager _accountManager; private AccountManager _accountManager;
private static final String LDAP_READ_TIMED_OUT_MESSAGE = "LDAP response read timed out";
public LdapAuthenticator() { public LdapAuthenticator() {
super(); super();
} }
@ -74,8 +76,8 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
return rc; return rc;
} }
List<LdapTrustMapVO> ldapTrustMapVOs = getLdapTrustMapVOS(domainId); List<LdapTrustMapVO> ldapTrustMapVOs = getLdapTrustMapVOS(domainId);
if(ldapTrustMapVOs != null && ldapTrustMapVOs.size() > 0) { if (ldapTrustMapVOs != null && ldapTrustMapVOs.size() > 0) {
if(ldapTrustMapVOs.size() == 1 && ldapTrustMapVOs.get(0).getAccountId() == 0) { if (ldapTrustMapVOs.size() == 1 && ldapTrustMapVOs.get(0).getAccountId() == 0) {
if (logger.isTraceEnabled()) { if (logger.isTraceEnabled()) {
logger.trace("We have a single mapping of a domain to an ldap group or ou"); logger.trace("We have a single mapping of a domain to an ldap group or ou");
} }
@ -125,11 +127,11 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
mappedGroups.retainAll(memberships); mappedGroups.retainAll(memberships);
tracelist("actual groups for " + username, mappedGroups); tracelist("actual groups for " + username, mappedGroups);
// check membership, there must be only one match in this domain // check membership, there must be only one match in this domain
if(ldapUser.isDisabled()) { if (ldapUser.isDisabled()) {
logAndDisable(userAccount, "attempt to log on using disabled ldap user " + userAccount.getUsername(), false); logAndDisable(userAccount, "attempt to log on using disabled ldap user " + userAccount.getUsername(), false);
} else if(mappedGroups.size() > 1) { } else if (mappedGroups.size() > 1) {
logAndDisable(userAccount, "user '" + username + "' is mapped to more then one account in domain and will be disabled.", false); logAndDisable(userAccount, "user '" + username + "' is mapped to more then one account in domain and will be disabled.", false);
} else if(mappedGroups.size() < 1) { } else if (mappedGroups.size() < 1) {
logAndDisable(userAccount, "user '" + username + "' is not mapped to an account in domain and will be removed.", true); logAndDisable(userAccount, "user '" + username + "' is not mapped to an account in domain and will be removed.", true);
} else { } else {
// a valid ldap configured user exists // a valid ldap configured user exists
@ -137,12 +139,12 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
// we could now assert that ldapTrustMapVOs.contains(mapping); // we could now assert that ldapTrustMapVOs.contains(mapping);
// createUser in Account can only be done by account name not by account id; // createUser in Account can only be done by account name not by account id;
Account account = _accountManager.getAccount(mapping.getAccountId()); Account account = _accountManager.getAccount(mapping.getAccountId());
if(null == account) { if (null == account) {
throw new CloudRuntimeException(String.format("account for user (%s) not found by id %d", username, mapping.getAccountId())); throw new CloudRuntimeException(String.format("account for user (%s) not found by id %d", username, mapping.getAccountId()));
} }
String accountName = account.getAccountName(); String accountName = account.getAccountName();
rc.first(_ldapManager.canAuthenticate(ldapUser.getPrincipal(), password, domainId)); rc.first(_ldapManager.canAuthenticate(ldapUser.getPrincipal(), password, domainId));
if (! rc.first()) { if (!rc.first()) {
rc.second(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT); rc.second(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT);
} }
// for security reasons we keep processing on faulty login attempt to not give a way information on userid existence // for security reasons we keep processing on faulty login attempt to not give a way information on userid existence
@ -162,7 +164,7 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
userAccount = _accountManager.getUserAccountById(user.getId()); userAccount = _accountManager.getUserAccountById(user.getId());
} else { } else {
// not a new user, check if mapped group has changed // not a new user, check if mapped group has changed
if(userAccount.getAccountId() != mapping.getAccountId()) { if (userAccount.getAccountId() != mapping.getAccountId()) {
final Account mappedAccount = _accountManager.getAccount(mapping.getAccountId()); final Account mappedAccount = _accountManager.getAccount(mapping.getAccountId());
if (mappedAccount == null || mappedAccount.getRemoved() != null) { if (mappedAccount == null || mappedAccount.getRemoved() != null) {
throw new CloudRuntimeException("Mapped account for users does not exist. Please contact your administrator."); throw new CloudRuntimeException("Mapped account for users does not exist. Please contact your administrator.");
@ -174,12 +176,21 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
} }
} catch (NoLdapUserMatchingQueryException e) { } catch (NoLdapUserMatchingQueryException e) {
logger.debug(e.getMessage()); logger.debug(e.getMessage());
disableUserInCloudStack(userAccount); processLdapUserErrorMessage(userAccount, e.getMessage(), rc);
} }
return rc; return rc;
} }
private void processLdapUserErrorMessage(UserAccount user, String errorMessage, Pair<Boolean, ActionOnFailedAuthentication> rc) {
if (StringUtils.isNotEmpty(errorMessage) && errorMessage.contains(LDAP_READ_TIMED_OUT_MESSAGE) && !rc.first()) {
rc.second(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT);
} else {
// no user in ldap ==>> disable user in cloudstack
disableUserInCloudStack(user);
}
}
private void tracelist(String msg, List<String> listToTrace) { private void tracelist(String msg, List<String> listToTrace) {
if (logger.isTraceEnabled()) { if (logger.isTraceEnabled()) {
StringBuilder logMsg = new StringBuilder(); StringBuilder logMsg = new StringBuilder();
@ -197,7 +208,7 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
if (logger.isInfoEnabled()) { if (logger.isInfoEnabled()) {
logger.info(msg); logger.info(msg);
} }
if(remove) { if (remove) {
removeUserInCloudStack(userAccount); removeUserInCloudStack(userAccount);
} else { } else {
disableUserInCloudStack(userAccount); disableUserInCloudStack(userAccount);
@ -229,23 +240,22 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
processLdapUser(password, domainId, user, rc, ldapUser, accountType); processLdapUser(password, domainId, user, rc, ldapUser, accountType);
} catch (NoLdapUserMatchingQueryException e) { } catch (NoLdapUserMatchingQueryException e) {
logger.debug(e.getMessage()); logger.debug(e.getMessage());
// no user in ldap ==>> disable user in cloudstack processLdapUserErrorMessage(user, e.getMessage(), rc);
disableUserInCloudStack(user);
} }
return rc; return rc;
} }
private void processLdapUser(String password, Long domainId, UserAccount user, Pair<Boolean, ActionOnFailedAuthentication> rc, LdapUser ldapUser, Account.Type accountType) { private void processLdapUser(String password, Long domainId, UserAccount user, Pair<Boolean, ActionOnFailedAuthentication> rc, LdapUser ldapUser, Account.Type accountType) {
if(!ldapUser.isDisabled()) { if (!ldapUser.isDisabled()) {
rc.first(_ldapManager.canAuthenticate(ldapUser.getPrincipal(), password, domainId)); rc.first(_ldapManager.canAuthenticate(ldapUser.getPrincipal(), password, domainId));
if(rc.first()) { if (rc.first()) {
if(user == null) { if (user == null) {
// import user to cloudstack // import user to cloudstack
createCloudStackUserAccount(ldapUser, domainId, accountType); createCloudStackUserAccount(ldapUser, domainId, accountType);
} else { } else {
enableUserInCloudStack(user); enableUserInCloudStack(user);
} }
} else if(user != null) { } else if (user != null) {
rc.second(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT); rc.second(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT);
} }
} else { } else {
@ -264,30 +274,34 @@ public class LdapAuthenticator extends AdapterBase implements UserAuthenticator
*/ */
Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username, String password, Long domainId, UserAccount user) { Pair<Boolean, ActionOnFailedAuthentication> authenticate(String username, String password, Long domainId, UserAccount user) {
boolean result = false; boolean result = false;
boolean timedOut = false;
if(user != null ) { if (user != null ) {
try { try {
LdapUser ldapUser = _ldapManager.getUser(username, domainId); LdapUser ldapUser = _ldapManager.getUser(username, domainId);
if(!ldapUser.isDisabled()) { if (!ldapUser.isDisabled()) {
result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password, domainId); result = _ldapManager.canAuthenticate(ldapUser.getPrincipal(), password, domainId);
} else { } else {
logger.debug("user with principal "+ ldapUser.getPrincipal() + " is disabled in ldap"); logger.debug("user with principal "+ ldapUser.getPrincipal() + " is disabled in ldap");
} }
} catch (NoLdapUserMatchingQueryException e) { } catch (NoLdapUserMatchingQueryException e) {
logger.debug(e.getMessage()); logger.debug(e.getMessage());
if (e.getMessage().contains(LDAP_READ_TIMED_OUT_MESSAGE)) {
timedOut = true;
} }
} }
return processResultAndAction(user, result); }
return processResultAndAction(user, result, timedOut);
} }
private Pair<Boolean, ActionOnFailedAuthentication> processResultAndAction(UserAccount user, boolean result) { private Pair<Boolean, ActionOnFailedAuthentication> processResultAndAction(UserAccount user, boolean result, boolean timedOut) {
return (!result && user != null) ? return (!result && (user != null || timedOut)) ?
new Pair<Boolean, ActionOnFailedAuthentication>(result, ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT): new Pair<Boolean, ActionOnFailedAuthentication>(result, ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT):
new Pair<Boolean, ActionOnFailedAuthentication>(result, null); new Pair<Boolean, ActionOnFailedAuthentication>(result, null);
} }
private void enableUserInCloudStack(UserAccount user) { private void enableUserInCloudStack(UserAccount user) {
if(user != null && (user.getState().equalsIgnoreCase(Account.State.DISABLED.toString()))) { if (user != null && (user.getState().equalsIgnoreCase(Account.State.DISABLED.toString()))) {
_accountManager.enableUser(user.getId()); _accountManager.enableUser(user.getId());
} }
} }

View File

@ -166,7 +166,7 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
private LdapConfigurationResponse addConfigurationInternal(final String hostname, int port, final Long domainId) throws InvalidParameterValueException { private LdapConfigurationResponse addConfigurationInternal(final String hostname, int port, final Long domainId) throws InvalidParameterValueException {
// TODO evaluate what the right default should be // TODO evaluate what the right default should be
if(port <= 0) { if (port <= 0) {
port = 389; port = 389;
} }
@ -210,7 +210,7 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
// TODO return the right account for this user // TODO return the right account for this user
final LdapContext context = _ldapContextFactory.createUserContext(principal, password, domainId); final LdapContext context = _ldapContextFactory.createUserContext(principal, password, domainId);
closeContext(context); closeContext(context);
if(logger.isTraceEnabled()) { if (logger.isTraceEnabled()) {
logger.trace(String.format("User(%s) authenticated for domain(%s)", principal, domainId)); logger.trace(String.format("User(%s) authenticated for domain(%s)", principal, domainId));
} }
return true; return true;
@ -234,7 +234,7 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
@Override @Override
public LdapConfigurationResponse createLdapConfigurationResponse(final LdapConfigurationVO configuration) { public LdapConfigurationResponse createLdapConfigurationResponse(final LdapConfigurationVO configuration) {
String domainUuid = null; String domainUuid = null;
if(configuration.getDomainId() != null) { if (configuration.getDomainId() != null) {
DomainVO domain = domainDao.findById(configuration.getDomainId()); DomainVO domain = domainDao.findById(configuration.getDomainId());
if (domain != null) { if (domain != null) {
domainUuid = domain.getUuid(); domainUuid = domain.getUuid();
@ -303,8 +303,8 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(null)).getUser(escapedUsername, context, domainId); return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(null)).getUser(escapedUsername, context, domainId);
} catch (NamingException | IOException e) { } catch (NamingException | IOException e) {
logger.debug("ldap Exception: ",e); logger.debug("LDAP Exception: ", e);
throw new NoLdapUserMatchingQueryException("No Ldap User found for username: "+username); throw new NoLdapUserMatchingQueryException("Unable to find LDAP User for username: " + username + ", due to " + e.getMessage());
} finally { } finally {
closeContext(context); closeContext(context);
} }
@ -324,8 +324,8 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
LdapUserManager userManagerFactory = _ldapUserManagerFactory.getInstance(ldapProvider); LdapUserManager userManagerFactory = _ldapUserManagerFactory.getInstance(ldapProvider);
return userManagerFactory.getUser(escapedUsername, type, name, context, domainId); return userManagerFactory.getUser(escapedUsername, type, name, context, domainId);
} catch (NamingException | IOException e) { } catch (NamingException | IOException e) {
logger.debug("ldap Exception: ",e); logger.debug("LDAP Exception: ", e);
throw new NoLdapUserMatchingQueryException("No Ldap User found for username: "+username + " in group: " + name + " of type: " + type); throw new NoLdapUserMatchingQueryException("Unable to find LDAP User for username: " + username + " in group: " + name + " of type: " + type + ", due to " + e.getMessage());
} finally { } finally {
closeContext(context); closeContext(context);
} }
@ -338,7 +338,7 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
context = _ldapContextFactory.createBindContext(domainId); context = _ldapContextFactory.createBindContext(domainId);
return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(domainId)).getUsers(context, domainId); return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(domainId)).getUsers(context, domainId);
} catch (NamingException | IOException e) { } catch (NamingException | IOException e) {
logger.debug("ldap Exception: ",e); logger.debug("LDAP Exception: ", e);
throw new NoLdapUserMatchingQueryException("*"); throw new NoLdapUserMatchingQueryException("*");
} finally { } finally {
closeContext(context); closeContext(context);
@ -352,7 +352,7 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
context = _ldapContextFactory.createBindContext(domainId); context = _ldapContextFactory.createBindContext(domainId);
return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(domainId)).getUsersInGroup(groupName, context, domainId); return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(domainId)).getUsersInGroup(groupName, context, domainId);
} catch (NamingException | IOException e) { } catch (NamingException | IOException e) {
logger.debug("ldap NamingException: ",e); logger.debug("LDAP Exception: ", e);
throw new NoLdapUserMatchingQueryException("groupName=" + groupName); throw new NoLdapUserMatchingQueryException("groupName=" + groupName);
} finally { } finally {
closeContext(context); closeContext(context);
@ -390,7 +390,7 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
final String escapedUsername = LdapUtils.escapeLDAPSearchFilter(username); final String escapedUsername = LdapUtils.escapeLDAPSearchFilter(username);
return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(null)).getUsers("*" + escapedUsername + "*", context, null); return _ldapUserManagerFactory.getInstance(_ldapConfiguration.getLdapProvider(null)).getUsers("*" + escapedUsername + "*", context, null);
} catch (NamingException | IOException e) { } catch (NamingException | IOException e) {
logger.debug("ldap Exception: ",e); logger.debug("LDAP Exception: ",e);
throw new NoLdapUserMatchingQueryException(username); throw new NoLdapUserMatchingQueryException(username);
} finally { } finally {
closeContext(context); closeContext(context);
@ -481,7 +481,7 @@ public class LdapManagerImpl extends ComponentLifecycleBase implements LdapManag
private void clearOldAccountMapping(LinkAccountToLdapCmd cmd) { private void clearOldAccountMapping(LinkAccountToLdapCmd cmd) {
// first find if exists log warning and update // first find if exists log warning and update
LdapTrustMapVO oldVo = _ldapTrustMapDao.findGroupInDomain(cmd.getDomainId(), cmd.getLdapDomain()); LdapTrustMapVO oldVo = _ldapTrustMapDao.findGroupInDomain(cmd.getDomainId(), cmd.getLdapDomain());
if(oldVo != null) { if (oldVo != null) {
// deal with edge cases, i.e. check if the old account is indeed deleted etc. // deal with edge cases, i.e. check if the old account is indeed deleted etc.
if (oldVo.getAccountId() != 0l) { if (oldVo.getAccountId() != 0l) {
AccountVO oldAcount = accountDao.findByIdIncludingRemoved(oldVo.getAccountId()); AccountVO oldAcount = accountDao.findByIdIncludingRemoved(oldVo.getAccountId());