From f2a6a2ff1319b9a1a4aefcb19368140f59232962 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 20 May 2025 15:26:54 +0200 Subject: [PATCH 01/14] .github: fix sonar checks (#10894) * .github: fix sonar check * .github: fix main sonar check * add more pom.xml files --- .github/workflows/main-sonar-check.yml | 2 +- .github/workflows/sonar-check.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main-sonar-check.yml b/.github/workflows/main-sonar-check.yml index 8248e48022a..dccd7174e54 100644 --- a/.github/workflows/main-sonar-check.yml +++ b/.github/workflows/main-sonar-check.yml @@ -54,7 +54,7 @@ jobs: uses: actions/cache@v4 with: path: ~/.m2/repository - key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }} + key: ${{ runner.os }}-m2-${{ hashFiles('pom.xml', '*/pom.xml', '*/*/pom.xml', '*/*/*/pom.xml') }} restore-keys: | ${{ runner.os }}-m2 diff --git a/.github/workflows/sonar-check.yml b/.github/workflows/sonar-check.yml index c36bceb2b90..d31f55980a8 100644 --- a/.github/workflows/sonar-check.yml +++ b/.github/workflows/sonar-check.yml @@ -56,7 +56,7 @@ jobs: uses: actions/cache@v4 with: path: ~/.m2/repository - key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }} + key: ${{ runner.os }}-m2-${{ hashFiles('pom.xml', '*/pom.xml', '*/*/pom.xml', '*/*/*/pom.xml') }} restore-keys: | ${{ runner.os }}-m2 From bb79f0b727eaaff324e52b76063989341f808012 Mon Sep 17 00:00:00 2001 From: dahn Date: Tue, 27 May 2025 08:17:49 +0200 Subject: [PATCH 02/14] engine/schema: create default network offering for vpc tier with conserve_mode=1 for fresh installation (#10744) (#10843) Co-authored-by: Wei Zhou --- .../cloudstack/engine/orchestration/NetworkOrchestrator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 2ec79bc80f1..e2919c5180d 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -577,7 +577,7 @@ public class NetworkOrchestrator extends ManagerBase implements NetworkOrchestra if (_networkOfferingDao.findByUniqueName(NetworkOffering.DefaultIsolatedNetworkOfferingForVpcNetworks) == null) { offering = _configMgr.createNetworkOffering(NetworkOffering.DefaultIsolatedNetworkOfferingForVpcNetworks, "Offering for Isolated VPC networks with Source Nat service enabled", TrafficType.Guest, null, false, Availability.Optional, null, - defaultVPCOffProviders, true, Network.GuestType.Isolated, false, null, false, null, false, false, null, false, null, true, true, false, null, null, true, null); + defaultVPCOffProviders, true, Network.GuestType.Isolated, false, null, true, null, false, false, null, false, null, true, true, false, null, null, true, null); } //#6 - default vpc offering with no LB service From b760b0262f2b8577eefce349832a1d563b378eb8 Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Tue, 27 May 2025 13:44:04 +0530 Subject: [PATCH 03/14] Fix issue with configdrive on XenServer (#10912) --- .../resource/wrapper/xenbase/CitrixStartCommandWrapper.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixStartCommandWrapper.java b/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixStartCommandWrapper.java index 5867a151c85..f6a60d65d6f 100644 --- a/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixStartCommandWrapper.java +++ b/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixStartCommandWrapper.java @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import com.cloud.agent.resource.virtualnetwork.VRScripts; @@ -241,7 +242,7 @@ public final class CitrixStartCommandWrapper extends CommandWrapper disks = new ArrayList(vmSpec.getDisks().length); int index = 0; for (final DiskTO disk : vmSpec.getDisks()) { - if (Volume.Type.ISO.equals(disk.getType())) { + if (Volume.Type.ISO.equals(disk.getType()) && Objects.nonNull(disk.getPath())) { disks.add(0, disk); } else { disks.add(index, disk); From 7e565d2524f355e06fe3ec218d57e55b03c337d2 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 27 May 2025 10:43:37 +0200 Subject: [PATCH 04/14] Routed: support vxlan networks (#10861) --- .../network/guru/VxlanGuestNetworkGuru.java | 3 +++ .../guru/ExternalGuestNetworkGuru.java | 25 +++---------------- .../cloud/network/guru/GuestNetworkGuru.java | 24 ++++++++++++++++++ 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/plugins/network-elements/vxlan/src/main/java/com/cloud/network/guru/VxlanGuestNetworkGuru.java b/plugins/network-elements/vxlan/src/main/java/com/cloud/network/guru/VxlanGuestNetworkGuru.java index ec654af2b32..e2c313d0821 100644 --- a/plugins/network-elements/vxlan/src/main/java/com/cloud/network/guru/VxlanGuestNetworkGuru.java +++ b/plugins/network-elements/vxlan/src/main/java/com/cloud/network/guru/VxlanGuestNetworkGuru.java @@ -76,6 +76,9 @@ public class VxlanGuestNetworkGuru extends GuestNetworkGuru { network.setBroadcastUri(BroadcastDomainType.Vxlan.toUri(vxlan)); } network.setBroadcastDomainType(BroadcastDomainType.Vxlan); + + getOrCreateIpv4SubnetForGuestNetwork(offering, network, userSpecified, owner); + return updateNetworkDesignForIPv6IfNeeded(network, userSpecified); } diff --git a/server/src/main/java/com/cloud/network/guru/ExternalGuestNetworkGuru.java b/server/src/main/java/com/cloud/network/guru/ExternalGuestNetworkGuru.java index 4f76488337d..954c70f610e 100644 --- a/server/src/main/java/com/cloud/network/guru/ExternalGuestNetworkGuru.java +++ b/server/src/main/java/com/cloud/network/guru/ExternalGuestNetworkGuru.java @@ -24,8 +24,6 @@ import javax.inject.Inject; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService; -import org.apache.cloudstack.network.Ipv4GuestSubnetNetworkMap; -import org.apache.cloudstack.network.RoutedIpv4Manager; import com.cloud.dc.DataCenter; import com.cloud.dc.DataCenter.NetworkType; @@ -37,7 +35,6 @@ import com.cloud.event.EventTypes; import com.cloud.event.EventVO; import com.cloud.exception.InsufficientAddressCapacityException; import com.cloud.exception.InsufficientVirtualNetworkCapacityException; -import com.cloud.exception.InvalidParameterValueException; import com.cloud.network.IpAddressManager; import com.cloud.network.Network; import com.cloud.network.Network.GuestType; @@ -90,8 +87,6 @@ public class ExternalGuestNetworkGuru extends GuestNetworkGuru { FirewallRulesDao _fwRulesDao; @Inject FirewallRulesCidrsDao _fwRulesCidrDao; - @Inject - RoutedIpv4Manager routedIpv4Manager; public ExternalGuestNetworkGuru() { super(); @@ -126,23 +121,9 @@ public class ExternalGuestNetworkGuru extends GuestNetworkGuru { /* In order to revert userSpecified network setup */ config.setState(State.Allocated); } - if (NetworkOffering.NetworkMode.ROUTED.equals(offering.getNetworkMode()) && !offering.isForVpc()) { - if (userSpecified.getCidr() != null) { - routedIpv4Manager.getOrCreateIpv4SubnetForGuestNetwork(config, userSpecified.getCidr()); - } else { - if (userSpecified.getNetworkCidrSize() == null) { - throw new InvalidParameterValueException("The network CIDR or CIDR size must be specified."); - } - Ipv4GuestSubnetNetworkMap subnet = routedIpv4Manager.getOrCreateIpv4SubnetForGuestNetwork(owner.getDomainId(), owner.getAccountId(), config.getDataCenterId(), userSpecified.getNetworkCidrSize()); - if (subnet != null) { - final String[] cidrTuple = subnet.getSubnet().split("\\/"); - config.setGateway(NetUtils.getIpRangeStartIpFromCidr(cidrTuple[0], Long.parseLong(cidrTuple[1]))); - config.setCidr(subnet.getSubnet()); - } else { - throw new InvalidParameterValueException("Failed to allocate a CIDR with requested size."); - } - } - } + + getOrCreateIpv4SubnetForGuestNetwork(offering, config, userSpecified, owner); + return updateNetworkDesignForIPv6IfNeeded(config, userSpecified); } diff --git a/server/src/main/java/com/cloud/network/guru/GuestNetworkGuru.java b/server/src/main/java/com/cloud/network/guru/GuestNetworkGuru.java index 96c3da66c09..2f668b59d62 100644 --- a/server/src/main/java/com/cloud/network/guru/GuestNetworkGuru.java +++ b/server/src/main/java/com/cloud/network/guru/GuestNetworkGuru.java @@ -28,6 +28,8 @@ import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationSe import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; +import org.apache.cloudstack.network.Ipv4GuestSubnetNetworkMap; +import org.apache.cloudstack.network.RoutedIpv4Manager; import org.apache.commons.lang3.StringUtils; import com.cloud.configuration.Config; @@ -121,6 +123,8 @@ public abstract class GuestNetworkGuru extends AdapterBase implements NetworkGur Ipv6AddressManager ipv6AddressManager; @Inject DomainRouterDao domainRouterDao; + @Inject + RoutedIpv4Manager routedIpv4Manager; Random _rand = new Random(System.currentTimeMillis()); @@ -571,4 +575,24 @@ public abstract class GuestNetworkGuru extends AdapterBase implements NetworkGur } return network; } + + public void getOrCreateIpv4SubnetForGuestNetwork(NetworkOffering offering, NetworkVO config, Network userSpecified, Account owner) { + if (NetworkOffering.NetworkMode.ROUTED.equals(offering.getNetworkMode()) && !offering.isForVpc()) { + if (userSpecified.getCidr() != null) { + routedIpv4Manager.getOrCreateIpv4SubnetForGuestNetwork(config, userSpecified.getCidr()); + } else { + if (userSpecified.getNetworkCidrSize() == null) { + throw new InvalidParameterValueException("The network CIDR or CIDR size must be specified."); + } + Ipv4GuestSubnetNetworkMap subnet = routedIpv4Manager.getOrCreateIpv4SubnetForGuestNetwork(owner.getDomainId(), owner.getAccountId(), config.getDataCenterId(), userSpecified.getNetworkCidrSize()); + if (subnet != null) { + final String[] cidrTuple = subnet.getSubnet().split("\\/"); + config.setGateway(NetUtils.getIpRangeStartIpFromCidr(cidrTuple[0], Long.parseLong(cidrTuple[1]))); + config.setCidr(subnet.getSubnet()); + } else { + throw new InvalidParameterValueException("Failed to allocate a CIDR with requested size."); + } + } + } + } } From 857ccb0a3b8084d07d440aa61749f61557372912 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 27 May 2025 10:56:35 +0200 Subject: [PATCH 05/14] server: fix list diskoffering by domainid returns Inactive offerings (#10916) --- .../src/main/java/com/cloud/api/query/QueryManagerImpl.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 0e99af41338..1a03ea93dcb 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -3485,7 +3485,9 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q SearchCriteria sc = diskOfferingSearch.create(); sc.setParameters("computeOnly", false); - sc.setParameters("activeState", DiskOffering.State.Active); + if (state != null) { + sc.setParameters("state", state); + } sc.setJoinParameters("domainDetailsSearch", "domainId", domainId); From 38f3107211bc767096984024c7b7dd5721ebb387 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Wed, 9 Apr 2025 07:50:32 -0300 Subject: [PATCH 06/14] Fix aaccess to template/ISO list for domain/resource admins In Apache CloudStack, while using the listTemplates and listIsos APIs, Domain Admins and Resource Admins can retrieve templates and ISOs outside their intended scope. Co-authored-by: bernardodemarco Co-authored-by: nvazquez --- server/src/main/java/com/cloud/api/query/QueryManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 1a03ea93dcb..d0f6fc0b16d 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -4572,7 +4572,7 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q if (!permittedAccounts.isEmpty()) { domain = _domainDao.findById(permittedAccounts.get(0).getDomainId()); } else { - domain = _domainDao.findById(Domain.ROOT_DOMAIN); + domain = _domainDao.findById(caller.getDomainId()); } setIdsListToSearchCriteria(sc, ids); From ad9d9cd3f6820db911e0920fed89e8a84cde1fd4 Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Wed, 16 Apr 2025 15:25:26 +0530 Subject: [PATCH 07/14] Keep same/consistent auth time for valid & invalid users --- .../com/cloud/user/AccountManagerImpl.java | 222 ++++++++++-------- 1 file changed, 124 insertions(+), 98 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 4e3a2e98564..5ecdb40b714 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -343,6 +343,9 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M private List userTwoFactorAuthenticationProviders; + private long validUserLastAuthTimeDurationInMs = 0L; + private static final long DEFAULT_USER_AUTH_TIME_DURATION_MS = 350L; + public static ConfigKey enableUserTwoFactorAuthentication = new ConfigKey<>("Advanced", Boolean.class, "enable.user.2fa", @@ -1557,7 +1560,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M for (UserAuthenticator userAuthenticator : _userPasswordEncoders) { Pair authenticationResult = userAuthenticator.authenticate(user.getUsername(), currentPassword, userAccount.getDomainId(), null); if (authenticationResult == null) { - s_logger.trace(String.format("Authenticator [%s] is returning null for the authenticate mehtod.", userAuthenticator.getClass())); + s_logger.trace(String.format("Authenticator [%s] is returning null for the authenticate method.", userAuthenticator.getClass())); continue; } if (BooleanUtils.toBoolean(authenticationResult.first())) { @@ -2561,107 +2564,17 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @Override public UserAccount authenticateUser(final String username, final String password, final Long domainId, final InetAddress loginIpAddress, final Map requestParameters) { + long authStartTimeInMs = System.currentTimeMillis(); UserAccount user = null; final String[] oAuthProviderArray = (String[])requestParameters.get(ApiConstants.PROVIDER); final String[] secretCodeArray = (String[])requestParameters.get(ApiConstants.SECRET_CODE); String oauthProvider = ((oAuthProviderArray == null) ? null : oAuthProviderArray[0]); String secretCode = ((secretCodeArray == null) ? null : secretCodeArray[0]); - if ((password != null && !password.isEmpty()) || (oauthProvider != null && secretCode != null)) { user = getUserAccount(username, password, domainId, requestParameters); } else { - String key = _configDao.getValue("security.singlesignon.key"); - if (key == null) { - // the SSO key is gone, don't authenticate - return null; - } - - String singleSignOnTolerance = _configDao.getValue("security.singlesignon.tolerance.millis"); - if (singleSignOnTolerance == null) { - // the SSO tolerance is gone (how much time before/after system time we'll allow the login request to be - // valid), - // don't authenticate - return null; - } - - long tolerance = Long.parseLong(singleSignOnTolerance); - String signature = null; - long timestamp = 0L; - String unsignedRequest = null; - StringBuffer unsignedRequestBuffer = new StringBuffer(); - - // - build a request string with sorted params, make sure it's all lowercase - // - sign the request, verify the signature is the same - List parameterNames = new ArrayList(); - - for (Object paramNameObj : requestParameters.keySet()) { - parameterNames.add((String)paramNameObj); // put the name in a list that we'll sort later - } - - Collections.sort(parameterNames); - - try { - for (String paramName : parameterNames) { - // parameters come as name/value pairs in the form String/String[] - String paramValue = ((String[])requestParameters.get(paramName))[0]; - - if ("signature".equalsIgnoreCase(paramName)) { - signature = paramValue; - } else { - if ("timestamp".equalsIgnoreCase(paramName)) { - String timestampStr = paramValue; - try { - // If the timestamp is in a valid range according to our tolerance, verify the request - // signature, otherwise return null to indicate authentication failure - timestamp = Long.parseLong(timestampStr); - long currentTime = System.currentTimeMillis(); - if (Math.abs(currentTime - timestamp) > tolerance) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("Expired timestamp passed in to login, current time = " + currentTime + ", timestamp = " + timestamp); - } - return null; - } - } catch (NumberFormatException nfe) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("Invalid timestamp passed in to login: " + timestampStr); - } - return null; - } - } - - if (unsignedRequestBuffer.length() != 0) { - unsignedRequestBuffer.append("&"); - } - unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, "UTF-8")); - } - } - - if ((signature == null) || (timestamp == 0L)) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("Missing parameters in login request, signature = " + signature + ", timestamp = " + timestamp); - } - return null; - } - - unsignedRequest = unsignedRequestBuffer.toString().toLowerCase().replaceAll("\\+", "%20"); - - Mac mac = Mac.getInstance("HmacSHA1"); - SecretKeySpec keySpec = new SecretKeySpec(key.getBytes(), "HmacSHA1"); - mac.init(keySpec); - mac.update(unsignedRequest.getBytes()); - byte[] encryptedBytes = mac.doFinal(); - String computedSignature = new String(Base64.encodeBase64(encryptedBytes)); - boolean equalSig = ConstantTimeComparator.compareStrings(signature, computedSignature); - if (!equalSig) { - s_logger.info("User signature: " + signature + " is not equaled to computed signature: " + computedSignature); - } else { - user = _userAccountDao.getUserAccount(username, domainId); - } - } catch (Exception ex) { - s_logger.error("Exception authenticating user", ex); - return null; - } + user = getUserAccountForSSO(username, domainId, requestParameters); } if (user != null) { @@ -2695,18 +2608,35 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + ActionEventUtils.onActionEvent(user.getId(), user.getAccountId(), user.getDomainId(), EventTypes.EVENT_USER_LOGIN, "user has logged in from IP Address " + loginIpAddress, user.getId(), ApiCommandResourceType.User.toString()); + + validUserLastAuthTimeDurationInMs = System.currentTimeMillis() - authStartTimeInMs; // Here all is fine! if (s_logger.isDebugEnabled()) { - s_logger.debug("User: " + username + " in domain " + domainId + " has successfully logged in"); + s_logger.debug(String.format("User: %s in domain %d has successfully logged in, auth time duration - %d ms", username, domainId, validUserLastAuthTimeDurationInMs)); } - ActionEventUtils.onActionEvent(user.getId(), user.getAccountId(), user.getDomainId(), EventTypes.EVENT_USER_LOGIN, "user has logged in from IP Address " + loginIpAddress, user.getId(), ApiCommandResourceType.User.toString()); - return user; } else { if (s_logger.isDebugEnabled()) { s_logger.debug("User: " + username + " in domain " + domainId + " has failed to log in"); } + + long waitTimeDurationInMs; + long invalidUserAuthTimeDurationInMs = System.currentTimeMillis() - authStartTimeInMs; + if (validUserLastAuthTimeDurationInMs > 0) { + waitTimeDurationInMs = validUserLastAuthTimeDurationInMs - invalidUserAuthTimeDurationInMs; + } else { + waitTimeDurationInMs = DEFAULT_USER_AUTH_TIME_DURATION_MS - invalidUserAuthTimeDurationInMs; + } + + if (waitTimeDurationInMs > 0) { + try { + Thread.sleep(waitTimeDurationInMs); + } catch (final InterruptedException e) { + } + } + return null; } } @@ -2718,7 +2648,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M UserAccount userAccount = _userAccountDao.getUserAccount(username, domainId); boolean authenticated = false; - HashSet actionsOnFailedAuthenticaion = new HashSet(); + HashSet actionsOnFailedAuthenticaion = new HashSet<>(); User.Source userSource = userAccount != null ? userAccount.getSource() : User.Source.UNKNOWN; for (UserAuthenticator authenticator : _userAuthenticators) { final String[] secretCodeArray = (String[])requestParameters.get(ApiConstants.SECRET_CODE); @@ -2744,7 +2674,6 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M boolean updateIncorrectLoginCount = actionsOnFailedAuthenticaion.contains(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT); if (authenticated) { - Domain domain = _domainMgr.getDomain(domainId); String domainName = null; if (domain != null) { @@ -2786,6 +2715,103 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + private UserAccount getUserAccountForSSO(String username, Long domainId, Map requestParameters) { + String key = _configDao.getValue("security.singlesignon.key"); + if (key == null) { + // the SSO key is gone, don't authenticate + return null; + } + + String singleSignOnTolerance = _configDao.getValue("security.singlesignon.tolerance.millis"); + if (singleSignOnTolerance == null) { + // the SSO tolerance is gone (how much time before/after system time we'll allow the login request to be + // valid), + // don't authenticate + return null; + } + + UserAccount user = null; + long tolerance = Long.parseLong(singleSignOnTolerance); + String signature = null; + long timestamp = 0L; + String unsignedRequest; + StringBuffer unsignedRequestBuffer = new StringBuffer(); + + // - build a request string with sorted params, make sure it's all lowercase + // - sign the request, verify the signature is the same + List parameterNames = new ArrayList<>(); + + for (Object paramNameObj : requestParameters.keySet()) { + parameterNames.add((String)paramNameObj); // put the name in a list that we'll sort later + } + + Collections.sort(parameterNames); + + try { + for (String paramName : parameterNames) { + // parameters come as name/value pairs in the form String/String[] + String paramValue = ((String[])requestParameters.get(paramName))[0]; + + if ("signature".equalsIgnoreCase(paramName)) { + signature = paramValue; + } else { + if ("timestamp".equalsIgnoreCase(paramName)) { + String timestampStr = paramValue; + try { + // If the timestamp is in a valid range according to our tolerance, verify the request + // signature, otherwise return null to indicate authentication failure + timestamp = Long.parseLong(timestampStr); + long currentTime = System.currentTimeMillis(); + if (Math.abs(currentTime - timestamp) > tolerance) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Expired timestamp passed in to login, current time = " + currentTime + ", timestamp = " + timestamp); + } + return null; + } + } catch (NumberFormatException nfe) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Invalid timestamp passed in to login: " + timestampStr); + } + return null; + } + } + + if (unsignedRequestBuffer.length() != 0) { + unsignedRequestBuffer.append("&"); + } + unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, "UTF-8")); + } + } + + if ((signature == null) || (timestamp == 0L)) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Missing parameters in login request, signature = " + signature + ", timestamp = " + timestamp); + } + return null; + } + + unsignedRequest = unsignedRequestBuffer.toString().toLowerCase().replaceAll("\\+", "%20"); + + Mac mac = Mac.getInstance("HmacSHA1"); + SecretKeySpec keySpec = new SecretKeySpec(key.getBytes(), "HmacSHA1"); + mac.init(keySpec); + mac.update(unsignedRequest.getBytes()); + byte[] encryptedBytes = mac.doFinal(); + String computedSignature = new String(Base64.encodeBase64(encryptedBytes)); + boolean equalSig = ConstantTimeComparator.compareStrings(signature, computedSignature); + if (!equalSig) { + s_logger.info("User signature: " + signature + " is not equaled to computed signature: " + computedSignature); + } else { + user = _userAccountDao.getUserAccount(username, domainId); + } + } catch (Exception ex) { + s_logger.error("Exception authenticating user", ex); + return null; + } + + return user; + } + protected void updateLoginAttemptsWhenIncorrectLoginAttemptsEnabled(UserAccount account, boolean updateIncorrectLoginCount, int allowedLoginAttempts) { int attemptsMade = account.getLoginAttempts() + 1; From e2f187912cd5cf97d4ffe7fcdcdc003721be495d Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 21 Feb 2025 11:16:22 +0530 Subject: [PATCH 08/14] cks: create separate service account in project A separate service account will be created and added in the project, if not exist already, when a Kubernetes cluster is deployed in a project. This account will have a role with limited API access. Cleanup clusters on owner account cleanup, delete service account if needed When the owner account of k8s clusters is deleted, while its node VMs get expunged, the cluster entry in DB remain present. This fixes the issue by cleaning up all clusters for the account deleted. Project k8s service account will be deleted on account cleanup or when there is no active k8s cluster remaining Signed-off-by: Abhishek Kumar --- .../cluster/KubernetesServiceHelper.java | 2 + .../java/com/cloud/user/dao/AccountDao.java | 8 +- .../com/cloud/user/dao/AccountDaoImpl.java | 27 +- .../cluster/KubernetesClusterManagerImpl.java | 251 ++++++++++++++++-- .../cluster/KubernetesClusterService.java | 8 +- .../cluster/KubernetesServiceHelperImpl.java | 8 + .../cluster/dao/KubernetesClusterDao.java | 4 +- .../cluster/dao/KubernetesClusterDaoImpl.java | 33 ++- .../com/cloud/user/AccountManagerImpl.java | 15 ++ 9 files changed, 307 insertions(+), 49 deletions(-) diff --git a/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java b/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java index a39c26680fd..924dbf37f67 100644 --- a/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java +++ b/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java @@ -18,6 +18,7 @@ package com.cloud.kubernetes.cluster; import org.apache.cloudstack.acl.ControlledEntity; +import com.cloud.user.Account; import com.cloud.uservm.UserVm; import com.cloud.utils.component.Adapter; @@ -25,4 +26,5 @@ public interface KubernetesServiceHelper extends Adapter { ControlledEntity findByUuid(String uuid); void checkVmCanBeDestroyed(UserVm userVm); + void cleanupForAccount(Account account); } diff --git a/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java b/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java index 17b07496731..dae5f3a3467 100644 --- a/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java +++ b/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java @@ -16,6 +16,9 @@ // under the License. package com.cloud.user.dao; +import java.util.Date; +import java.util.List; + import com.cloud.user.Account; import com.cloud.user.AccountVO; import com.cloud.user.User; @@ -23,9 +26,6 @@ import com.cloud.utils.Pair; import com.cloud.utils.db.Filter; import com.cloud.utils.db.GenericDao; -import java.util.Date; -import java.util.List; - public interface AccountDao extends GenericDao { Pair findUserAccountByApiKey(String apiKey); @@ -33,6 +33,8 @@ public interface AccountDao extends GenericDao { Pair, Integer> findAccountsLike(String accountName, Filter filter); + List findAccountsByName(String accountName); + List findActiveAccounts(Long maxAccountId, Filter filter); List findRecentlyDeletedAccounts(Long maxAccountId, Date earliestRemovedDate, Filter filter); diff --git a/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java b/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java index 3dacbb70f39..be5ac2e8204 100644 --- a/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java @@ -16,6 +16,15 @@ // under the License. package com.cloud.user.dao; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.util.Date; +import java.util.List; + +import org.apache.commons.lang3.StringUtils; +import org.apache.log4j.Logger; +import org.springframework.stereotype.Component; + import com.cloud.user.Account; import com.cloud.user.Account.State; import com.cloud.user.AccountVO; @@ -30,15 +39,7 @@ import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; import com.cloud.utils.db.SearchCriteria.Func; import com.cloud.utils.db.SearchCriteria.Op; -import org.apache.commons.lang3.StringUtils; import com.cloud.utils.db.TransactionLegacy; -import org.apache.log4j.Logger; -import org.springframework.stereotype.Component; - -import java.sql.PreparedStatement; -import java.sql.ResultSet; -import java.util.Date; -import java.util.List; @Component public class AccountDaoImpl extends GenericDaoBase implements AccountDao { @@ -180,6 +181,16 @@ public class AccountDaoImpl extends GenericDaoBase implements A return searchAndCount(sc, filter); } + @Override + public List findAccountsByName(String accountName) { + SearchBuilder sb = createSearchBuilder(); + sb.and("accountName", sb.entity().getAccountName(), SearchCriteria.Op.EQ); + sb.done(); + SearchCriteria sc = sb.create(); + sc.setParameters("accountName", accountName); + return search(sc, null); + } + @Override public Account findEnabledAccount(String accountName, Long domainId) { SearchCriteria sc = AllFieldsSearch.create("accountName", accountName); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java index 9c402f83b03..3fea585cbfe 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java @@ -41,6 +41,11 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; import org.apache.cloudstack.acl.ControlledEntity; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RolePermissionEntity; +import org.apache.cloudstack.acl.RoleService; +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.acl.Rule; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; @@ -49,6 +54,14 @@ import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiConstants.VMDetails; import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.ResponseObject.ResponseView; +import org.apache.cloudstack.api.command.user.address.AssociateIPAddrCmd; +import org.apache.cloudstack.api.command.user.address.DisassociateIPAddrCmd; +import org.apache.cloudstack.api.command.user.address.ListPublicIpAddressesCmd; +import org.apache.cloudstack.api.command.user.firewall.CreateFirewallRuleCmd; +import org.apache.cloudstack.api.command.user.firewall.DeleteFirewallRuleCmd; +import org.apache.cloudstack.api.command.user.firewall.ListFirewallRulesCmd; +import org.apache.cloudstack.api.command.user.firewall.UpdateFirewallRuleCmd; +import org.apache.cloudstack.api.command.user.job.QueryAsyncJobResultCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.AddVirtualMachinesToKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.CreateKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.DeleteKubernetesClusterCmd; @@ -59,6 +72,18 @@ import org.apache.cloudstack.api.command.user.kubernetes.cluster.ScaleKubernetes import org.apache.cloudstack.api.command.user.kubernetes.cluster.StartKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.StopKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.UpgradeKubernetesClusterCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.AssignToLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.CreateLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.DeleteLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.ListLoadBalancerRuleInstancesCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.ListLoadBalancerRulesCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.RemoveFromLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.UpdateLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd; +import org.apache.cloudstack.api.command.user.network.DeleteNetworkACLCmd; +import org.apache.cloudstack.api.command.user.network.ListNetworkACLsCmd; +import org.apache.cloudstack.api.command.user.network.ListNetworksCmd; +import org.apache.cloudstack.api.command.user.vm.ListVMsCmd; import org.apache.cloudstack.api.response.KubernetesClusterConfigResponse; import org.apache.cloudstack.api.response.KubernetesClusterResponse; import org.apache.cloudstack.api.response.ListResponse; @@ -145,6 +170,8 @@ import com.cloud.offerings.dao.NetworkOfferingServiceMapDao; import com.cloud.org.Cluster; import com.cloud.org.Grouping; import com.cloud.projects.Project; +import com.cloud.projects.ProjectAccount; +import com.cloud.projects.ProjectManager; import com.cloud.resource.ResourceManager; import com.cloud.service.ServiceOfferingVO; import com.cloud.service.dao.ServiceOfferingDao; @@ -153,15 +180,18 @@ import com.cloud.storage.dao.VMTemplateDao; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.AccountService; +import com.cloud.user.AccountVO; import com.cloud.user.SSHKeyPairVO; import com.cloud.user.User; import com.cloud.user.UserAccount; import com.cloud.user.UserVO; +import com.cloud.user.dao.AccountDao; import com.cloud.user.dao.SSHKeyPairDao; import com.cloud.user.dao.UserDao; import com.cloud.uservm.UserVm; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; +import com.cloud.utils.UuidUtils; import com.cloud.utils.component.ComponentContext; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.concurrency.NamedThreadFactory; @@ -186,6 +216,33 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne private static final Logger LOGGER = Logger.getLogger(KubernetesClusterManagerImpl.class); private static final String DEFAULT_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_NAME = "DefaultNetworkOfferingforKubernetesService"; + private static final List> PROJECT_KUBERNETES_ACCOUNT_ROLE_ALLOWED_APIS = Arrays.asList( + QueryAsyncJobResultCmd.class, + ListVMsCmd.class, + ListNetworksCmd.class, + ListPublicIpAddressesCmd.class, + AssociateIPAddrCmd.class, + DisassociateIPAddrCmd.class, + ListLoadBalancerRulesCmd.class, + CreateLoadBalancerRuleCmd.class, + UpdateLoadBalancerRuleCmd.class, + DeleteLoadBalancerRuleCmd.class, + AssignToLoadBalancerRuleCmd.class, + RemoveFromLoadBalancerRuleCmd.class, + ListLoadBalancerRuleInstancesCmd.class, + ListFirewallRulesCmd.class, + CreateFirewallRuleCmd.class, + UpdateFirewallRuleCmd.class, + DeleteFirewallRuleCmd.class, + ListNetworkACLsCmd.class, + CreateNetworkACLCmd.class, + DeleteNetworkACLCmd.class, + ListKubernetesClustersCmd.class, + ScaleKubernetesClusterCmd.class + ); + private static final String PROJECT_KUBERNETES_ACCOUNT_FIRST_NAME = "Kubernetes"; + private static final String PROJECT_KUBERNETES_ACCOUNT_LAST_NAME = "Service User"; + protected StateMachine2 _stateMachine = KubernetesCluster.State.getStateMachine(); @@ -258,9 +315,14 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne public SecurityGroupService securityGroupService; @Inject public NetworkHelper networkHelper; - @Inject private UserVmService userVmService; + @Inject + public AccountDao accountDao; + @Inject + public ProjectManager projectManager; + @Inject + RoleService roleService; private void logMessage(final Level logLevel, final String message, final Exception e) { if (logLevel == Level.WARN) { @@ -1371,23 +1433,31 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne } } - private String[] getServiceUserKeys(KubernetesClusterVO kubernetesCluster) { - Account owner = accountService.getActiveAccountById(kubernetesCluster.getAccountId()); - if (owner == null || owner.getType() == Account.Type.PROJECT) { - owner = CallContext.current().getCallingAccount(); + protected String[] createUserApiKeyAndSecretKey(long userId) { + CallContext.register(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM); + try { + return accountService.createApiKeyAndSecretKey(userId); + } finally { + CallContext.unregister(); + } + } + + protected String[] getServiceUserKeys(Account owner) { + String username = owner.getAccountName(); + if (!username.startsWith(KUBEADMIN_ACCOUNT_NAME + "-")) { + username += "-" + KUBEADMIN_ACCOUNT_NAME; } - String username = owner.getAccountName() + "-" + KUBEADMIN_ACCOUNT_NAME; UserAccount kubeadmin = accountService.getActiveUserAccount(username, owner.getDomainId()); - String[] keys = null; + String[] keys; if (kubeadmin == null) { User kube = userDao.persist(new UserVO(owner.getAccountId(), username, UUID.randomUUID().toString(), owner.getAccountName(), - KUBEADMIN_ACCOUNT_NAME, "kubeadmin", null, UUID.randomUUID().toString(), User.Source.UNKNOWN)); - keys = accountService.createApiKeyAndSecretKey(kube.getId()); + KUBEADMIN_ACCOUNT_NAME, "kubeadmin", null, UUID.randomUUID().toString(), User.Source.UNKNOWN)); + keys = createUserApiKeyAndSecretKey(kube.getId()); } else { String apiKey = kubeadmin.getApiKey(); String secretKey = kubeadmin.getSecretKey(); if (StringUtils.isAnyEmpty(apiKey, secretKey)) { - keys = accountService.createApiKeyAndSecretKey(kubeadmin.getId()); + keys = createUserApiKeyAndSecretKey(kubeadmin.getId()); } else { keys = new String[]{apiKey, secretKey}; } @@ -1395,6 +1465,76 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne return keys; } + protected Role createProjectKubernetesAccountRole() { + Role role = roleService.createRole(PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME, RoleType.User, + PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME, false); + for (Class allowedApi : PROJECT_KUBERNETES_ACCOUNT_ROLE_ALLOWED_APIS) { + final String apiName = BaseCmd.getCommandNameByClass(allowedApi); + roleService.createRolePermission(role, new Rule(apiName), RolePermissionEntity.Permission.ALLOW, + String.format("Allow %s", apiName)); + } + roleService.createRolePermission(role, new Rule("*"), RolePermissionEntity.Permission.DENY, + "Deny all"); + LOGGER.debug(String.format("Created default role for Kubernetes service account in projects: %s", role)); + return role; + } + + public Role getProjectKubernetesAccountRole() { + List roles = roleService.findRolesByName(PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME); + if (CollectionUtils.isNotEmpty(roles)) { + Role role = roles.get(0); + LOGGER.debug(String.format("Found default role for Kubernetes service account in projects: %s", role)); + return role; + } + return createProjectKubernetesAccountRole(); + } + + protected Account createProjectKubernetesAccount(final Project project, final String accountName) { + CallContext.register(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM); + try { + Role role = getProjectKubernetesAccountRole(); + UserAccount userAccount = accountService.createUserAccount(accountName, + UuidUtils.first(UUID.randomUUID().toString()), PROJECT_KUBERNETES_ACCOUNT_FIRST_NAME, + PROJECT_KUBERNETES_ACCOUNT_LAST_NAME, null, null, accountName, Account.Type.NORMAL, role.getId(), + project.getDomainId(), null, null, null, null, User.Source.NATIVE); + projectManager.assignAccountToProject(project, userAccount.getAccountId(), ProjectAccount.Role.Regular, + userAccount.getId(), null); + Account account = accountService.getAccount(userAccount.getAccountId()); + LOGGER.debug(String.format("Created Kubernetes service account in project %s: %s", project, account)); + return account; + } finally { + CallContext.unregister(); + } + } + + protected Account getProjectKubernetesAccount(final Account callerAccount, final boolean create) { + Project project = ApiDBUtils.findProjectByProjectAccountId(callerAccount.getId()); + final String accountName = String.format("%s-%s", KUBEADMIN_ACCOUNT_NAME, UuidUtils.first(project.getUuid())); + List accounts = accountDao.findAccountsByName(accountName); + for (AccountVO account : accounts) { + if (projectManager.canAccessProjectAccount(account, project.getProjectAccountId())) { + LOGGER.debug(String.format("Created Kubernetes service account in project %s: %s", project, account)); + return account; + } + } + return create ? createProjectKubernetesAccount(project, accountName) : null; + } + + protected Account getProjectKubernetesAccount(final Account callerAccount) { + return getProjectKubernetesAccount(callerAccount, true); + } + + private String[] getServiceUserKeys(KubernetesClusterVO kubernetesCluster) { + Account owner = accountService.getActiveAccountById(kubernetesCluster.getAccountId()); + if (owner == null) { + owner = CallContext.current().getCallingAccount(); + } + if (owner.getType() == Account.Type.PROJECT) { + owner = getProjectKubernetesAccount(owner); + } + return getServiceUserKeys(owner); + } + @Override @ActionEvent(eventType = KubernetesClusterEventTypes.EVENT_KUBERNETES_CLUSTER_STOP, eventDescription = "stopping Kubernetes cluster", async = true) @@ -1439,15 +1579,13 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne logAndThrow(Level.ERROR, "Kubernetes Service plugin is disabled"); } Long kubernetesClusterId = cmd.getId(); - KubernetesClusterVO cluster = kubernetesClusterDao.findById(kubernetesClusterId); + final KubernetesClusterVO cluster = kubernetesClusterDao.findById(kubernetesClusterId); if (cluster == null) { throw new InvalidParameterValueException("Invalid cluster id specified"); } accountManager.checkAccess(CallContext.current().getCallingAccount(), SecurityChecker.AccessType.OperateEntry, false, cluster); if (cluster.getClusterType() == KubernetesCluster.ClusterType.CloudManaged) { - KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(cluster, this); - destroyWorker = ComponentContext.inject(destroyWorker); - return destroyWorker.destroy(); + return destroyKubernetesCluster(cluster); } else { boolean cleanup = cmd.getCleanup(); boolean expunge = cmd.getExpunge(); @@ -1470,13 +1608,14 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne } } } - return Transaction.execute(new TransactionCallback() { - @Override - public Boolean doInTransaction(TransactionStatus status) { - kubernetesClusterDetailsDao.removeDetails(kubernetesClusterId); - kubernetesClusterVmMapDao.removeByClusterId(kubernetesClusterId); - return kubernetesClusterDao.remove(kubernetesClusterId); + return Transaction.execute((TransactionCallback) status -> { + kubernetesClusterDetailsDao.removeDetails(kubernetesClusterId); + kubernetesClusterVmMapDao.removeByClusterId(kubernetesClusterId); + if (kubernetesClusterDao.remove(kubernetesClusterId)) { + deleteProjectKubernetesAccountIfNeeded(cluster); + return true; } + return false; }); } } @@ -1730,6 +1869,66 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne return responseList; } + protected void deleteProjectKubernetesAccount(Account projectAccount) { + CallContext.register(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM); + try { + Account serviceAccount = getProjectKubernetesAccount(projectAccount, false); + if (serviceAccount != null) { + accountManager.deleteAccount(accountDao.findById(serviceAccount.getId()), User.UID_SYSTEM, + accountService.getSystemAccount()); + } + } finally { + CallContext.unregister(); + } + } + + protected void deleteProjectKubernetesAccountIfNeeded(final KubernetesCluster kubernetesCluster) { + Account owner = accountService.getAccount(kubernetesCluster.getAccountId()); + if (owner == null) { + return; + } + if (Account.Type.PROJECT.equals(owner.getType()) && + kubernetesClusterDao.countNotForGCByAccount(owner.getAccountId()) == 0) { + deleteProjectKubernetesAccount(owner); + } + } + + protected boolean destroyKubernetesCluster(KubernetesCluster kubernetesCluster, boolean deleteProjectAccount) { + KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(kubernetesCluster, + KubernetesClusterManagerImpl.this); + destroyWorker = ComponentContext.inject(destroyWorker); + boolean result = destroyWorker.destroy(); + if (deleteProjectAccount) { + deleteProjectKubernetesAccountIfNeeded(kubernetesCluster); + } + return result; + } + + protected boolean destroyKubernetesCluster(KubernetesCluster kubernetesCluster) { + return destroyKubernetesCluster(kubernetesCluster, true); + } + + @Override + public void cleanupForAccount(Account account) { + List clusters = kubernetesClusterDao.listForCleanupByAccount(account.getId()); + if (CollectionUtils.isEmpty(clusters)) { + return; + } + LOGGER.debug(String.format("Cleaning up %d Kubernetes cluster for %s", clusters.size(), account)); + for (KubernetesClusterVO cluster : clusters) { + try { + destroyKubernetesCluster(cluster, false); + } catch (CloudRuntimeException e) { + LOGGER.warn(String.format("Failed to destroy Kubernetes cluster: %s during cleanup for %s", + cluster.getName(), account), e); + } + } + if (!Account.Type.PROJECT.equals(account.getType())) { + return; + } + deleteProjectKubernetesAccount(account); + } + @Override public List> getCommands() { List> cmdList = new ArrayList>(); @@ -1781,9 +1980,7 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne LOGGER.info(String.format("Running Kubernetes cluster garbage collector on Kubernetes cluster : %s", kubernetesCluster.getName())); } try { - KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(kubernetesCluster, KubernetesClusterManagerImpl.this); - destroyWorker = ComponentContext.inject(destroyWorker); - if (destroyWorker.destroy()) { + if (destroyKubernetesCluster(kubernetesCluster)) { if (LOGGER.isInfoEnabled()) { LOGGER.info(String.format("Garbage collection complete for Kubernetes cluster : %s", kubernetesCluster.getName())); } @@ -1909,9 +2106,7 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne LOGGER.info(String.format("Running Kubernetes cluster state scanner on Kubernetes cluster : %s for state: %s", kubernetesCluster.getName(), KubernetesCluster.State.Destroying.toString())); } try { - KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(kubernetesCluster, KubernetesClusterManagerImpl.this); - destroyWorker = ComponentContext.inject(destroyWorker); - destroyWorker.destroy(); + destroyKubernetesCluster(kubernetesCluster); } catch (Exception e) { LOGGER.warn(String.format("Failed to run Kubernetes cluster Destroying state scanner on Kubernetes cluster : %s status scanner", kubernetesCluster.getName()), e); } @@ -1925,7 +2120,7 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne } // checks if Kubernetes cluster is in desired state - boolean isClusterVMsInDesiredState(KubernetesCluster kubernetesCluster, VirtualMachine.State state) { + private boolean isClusterVMsInDesiredState(KubernetesCluster kubernetesCluster, VirtualMachine.State state) { List clusterVMs = kubernetesClusterVmMapDao.listByClusterId(kubernetesCluster.getId()); // check cluster is running at desired capacity include control nodes as well @@ -1985,6 +2180,8 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne LOGGER.trace("Added service for the network offering: " + offService); } + getProjectKubernetesAccountRole(); + _gcExecutor.scheduleWithFixedDelay(new KubernetesClusterGarbageCollector(), 300, 300, TimeUnit.SECONDS); _stateScanner.scheduleWithFixedDelay(new KubernetesClusterStatusScanner(), 300, 30, TimeUnit.SECONDS); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java index 6acc876493e..20550496a6d 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java @@ -16,6 +16,8 @@ // under the License. package com.cloud.kubernetes.cluster; +import java.util.List; + import org.apache.cloudstack.api.command.user.kubernetes.cluster.AddVirtualMachinesToKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.CreateKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.DeleteKubernetesClusterCmd; @@ -33,16 +35,16 @@ import org.apache.cloudstack.api.response.RemoveVirtualMachinesFromKubernetesClu import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; +import com.cloud.user.Account; import com.cloud.utils.component.PluggableService; import com.cloud.utils.exception.CloudRuntimeException; -import java.util.List; - public interface KubernetesClusterService extends PluggableService, Configurable { static final String MIN_KUBERNETES_VERSION_HA_SUPPORT = "1.16.0"; static final int MIN_KUBERNETES_CLUSTER_NODE_CPU = 2; static final int MIN_KUBERNETES_CLUSTER_NODE_RAM_SIZE = 2048; static final String KUBEADMIN_ACCOUNT_NAME = "kubeadmin"; + String PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME = "Project Kubernetes Service Role"; static final ConfigKey KubernetesServiceEnabled = new ConfigKey("Advanced", Boolean.class, "cloud.kubernetes.service.enabled", @@ -122,4 +124,6 @@ public interface KubernetesClusterService extends PluggableService, Configurable boolean addVmsToCluster(AddVirtualMachinesToKubernetesClusterCmd cmd); List removeVmsFromCluster(RemoveVirtualMachinesFromKubernetesClusterCmd cmd); + + void cleanupForAccount(Account account); } diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java index 9551142d24e..57c0ede3654 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java @@ -34,6 +34,7 @@ import com.cloud.kubernetes.cluster.dao.KubernetesClusterDao; import com.cloud.kubernetes.cluster.dao.KubernetesClusterVmMapDao; import com.cloud.kubernetes.version.KubernetesSupportedVersion; import com.cloud.kubernetes.version.KubernetesVersionEventTypes; +import com.cloud.user.Account; import com.cloud.uservm.UserVm; import com.cloud.utils.component.AdapterBase; import com.cloud.utils.exception.CloudRuntimeException; @@ -47,6 +48,8 @@ public class KubernetesServiceHelperImpl extends AdapterBase implements Kubernet private KubernetesClusterDao kubernetesClusterDao; @Inject private KubernetesClusterVmMapDao kubernetesClusterVmMapDao; + @Inject + KubernetesClusterService kubernetesClusterService; protected void setEventTypeEntityDetails(Class eventTypeDefinedClass, Class entityClass) { Field[] declaredFields = eventTypeDefinedClass.getDeclaredFields(); @@ -94,6 +97,11 @@ public class KubernetesServiceHelperImpl extends AdapterBase implements Kubernet throw new CloudRuntimeException(msg); } + @Override + public void cleanupForAccount(Account account) { + kubernetesClusterService.cleanupForAccount(account); + } + @Override public String getConfigComponentName() { return KubernetesServiceHelper.class.getSimpleName(); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java index 9341912012f..7df6a6b1dce 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java @@ -25,8 +25,8 @@ import com.cloud.utils.fsm.StateDao; public interface KubernetesClusterDao extends GenericDao, StateDao { - - List listByAccount(long accountId); + List listForCleanupByAccount(long accountId); + int countNotForGCByAccount(long accountId); List findKubernetesClustersToGarbageCollect(); List findManagedKubernetesClustersInState(KubernetesCluster.State state); List listByNetworkId(long networkId); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java index 63cca3563f7..7bec98d5d25 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java @@ -30,16 +30,25 @@ import com.cloud.utils.db.TransactionLegacy; @Component public class KubernetesClusterDaoImpl extends GenericDaoBase implements KubernetesClusterDao { - private final SearchBuilder AccountIdSearch; + private final SearchBuilder CleanupAccountIdSearch; + private final SearchBuilder NotForGCByAccountIDCount; private final SearchBuilder GarbageCollectedSearch; private final SearchBuilder ManagedStateSearch; private final SearchBuilder SameNetworkSearch; private final SearchBuilder KubernetesVersionSearch; public KubernetesClusterDaoImpl() { - AccountIdSearch = createSearchBuilder(); - AccountIdSearch.and("account", AccountIdSearch.entity().getAccountId(), SearchCriteria.Op.EQ); - AccountIdSearch.done(); + CleanupAccountIdSearch = createSearchBuilder(); + CleanupAccountIdSearch.and("account", CleanupAccountIdSearch.entity().getAccountId(), SearchCriteria.Op.EQ); + CleanupAccountIdSearch.and("cluster_type", CleanupAccountIdSearch.entity().getClusterType(), SearchCriteria.Op.EQ); + CleanupAccountIdSearch.done(); + + NotForGCByAccountIDCount = createSearchBuilder(); + NotForGCByAccountIDCount.and("gc", NotForGCByAccountIDCount.entity().isCheckForGc(), SearchCriteria.Op.EQ); + NotForGCByAccountIDCount.and("account", NotForGCByAccountIDCount.entity().getAccountId(), SearchCriteria.Op.EQ); + NotForGCByAccountIDCount.and("cluster_type", NotForGCByAccountIDCount.entity().getClusterType(), SearchCriteria.Op.EQ); + NotForGCByAccountIDCount.select(null, SearchCriteria.Func.COUNT, null); + NotForGCByAccountIDCount.done(); GarbageCollectedSearch = createSearchBuilder(); GarbageCollectedSearch.and("gc", GarbageCollectedSearch.entity().isCheckForGc(), SearchCriteria.Op.EQ); @@ -62,10 +71,20 @@ public class KubernetesClusterDaoImpl extends GenericDaoBase listByAccount(long accountId) { - SearchCriteria sc = AccountIdSearch.create(); + public List listForCleanupByAccount(long accountId) { + SearchCriteria sc = CleanupAccountIdSearch.create(); + sc.setParameters("cluster_type", KubernetesCluster.ClusterType.CloudManaged); sc.setParameters("account", accountId); - return listBy(sc, null); + return listBy(sc); + } + + @Override + public int countNotForGCByAccount(long accountId) { + SearchCriteria sc = NotForGCByAccountIDCount.create(); + sc.setParameters("cluster_type", KubernetesCluster.ClusterType.CloudManaged); + sc.setParameters("account", accountId); + sc.setParameters("gc", false); + return getCount(sc); } @Override diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 5ecdb40b714..180f4e49521 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -85,6 +85,7 @@ import org.apache.commons.lang3.BooleanUtils; import org.apache.commons.lang3.StringUtils; import org.apache.log4j.Logger; import org.jetbrains.annotations.NotNull; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; import com.cloud.api.ApiDBUtils; import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd; @@ -117,6 +118,7 @@ import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.OperationTimedoutException; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceUnavailableException; +import com.cloud.kubernetes.cluster.KubernetesServiceHelper; import com.cloud.network.IpAddress; import com.cloud.network.IpAddressManager; import com.cloud.network.Network; @@ -171,6 +173,7 @@ import com.cloud.utils.ConstantTimeComparator; import com.cloud.utils.NumbersUtil; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; +import com.cloud.utils.component.ComponentContext; import com.cloud.utils.component.Manager; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.component.PluggableService; @@ -859,6 +862,16 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return cleanupAccount(account, callerUserId, caller); } + protected void cleanupPluginsResourcesIfNeeded(Account account) { + try { + KubernetesServiceHelper kubernetesServiceHelper = + ComponentContext.getDelegateComponentOfType(KubernetesServiceHelper.class); + kubernetesServiceHelper.cleanupForAccount(account); + } catch (NoSuchBeanDefinitionException ignored) { + s_logger.debug("No KubernetesServiceHelper bean found"); + } + } + protected boolean cleanupAccount(AccountVO account, long callerUserId, Account caller) { long accountId = account.getId(); boolean accountCleanupNeeded = false; @@ -940,6 +953,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + cleanupPluginsResourcesIfNeeded(account); + // Destroy the account's VMs List vms = _userVmDao.listByAccountId(accountId); if (s_logger.isDebugEnabled()) { From a0080a04fe66b1b1cf1f67bb674a079d7cab857a Mon Sep 17 00:00:00 2001 From: nvazquez Date: Sun, 11 May 2025 22:48:41 -0300 Subject: [PATCH 09/14] Adding privilege checks on user and account operations Co-authored-by: Harikrishna --- api/src/main/java/com/cloud/user/Account.java | 1 + .../management/MockAccountManager.java | 4 + .../ConfigurationManagerImpl.java | 37 +++++ .../ResourceLimitManagerImpl.java | 6 + .../java/com/cloud/user/AccountManager.java | 1 + .../com/cloud/user/AccountManagerImpl.java | 128 +++++++++++++++++- .../ConfigurationManagerImplTest.java | 47 +++++++ .../cloud/user/AccountManagerImplTest.java | 101 +++++++++++++- ...countManagerImplVolumeDeleteEventTest.java | 2 + .../cloud/user/MockAccountManagerImpl.java | 4 + 10 files changed, 321 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/com/cloud/user/Account.java b/api/src/main/java/com/cloud/user/Account.java index bb9838f137a..2b815531116 100644 --- a/api/src/main/java/com/cloud/user/Account.java +++ b/api/src/main/java/com/cloud/user/Account.java @@ -71,6 +71,7 @@ public interface Account extends ControlledEntity, InternalIdentity, Identity { } public static final long ACCOUNT_ID_SYSTEM = 1; + public static final long ACCOUNT_ID_ADMIN = 2; public String getAccountName(); diff --git a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index a63c5b68e57..353b062b90b 100644 --- a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java @@ -521,4 +521,8 @@ public class MockAccountManager extends ManagerBase implements AccountManager { public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) { return null; } + + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + } } diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 11d478ccf62..4c03122e463 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -47,11 +47,13 @@ import javax.naming.ConfigurationException; import com.cloud.hypervisor.HypervisorGuru; +import com.cloud.user.AccountManagerImpl; import com.cloud.utils.crypt.DBEncryptionUtil; import com.cloud.host.HostTagVO; import com.cloud.storage.StoragePoolTagVO; import com.cloud.storage.VolumeApiServiceImpl; import com.googlecode.ipv6.IPv6Address; +import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.affinity.AffinityGroup; import org.apache.cloudstack.affinity.AffinityGroupService; @@ -470,6 +472,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati private long _defaultPageSize = Long.parseLong(Config.DefaultPageSize.getDefaultValue()); private static final String DOMAIN_NAME_PATTERN = "^((?!-)[A-Za-z0-9-]{1,63}(? configValuesForValidation = new HashSet(); + private Set configKeysAllowedOnlyForDefaultAdmin = new HashSet(); private Set weightBasedParametersForValidation = new HashSet(); private Set overprovisioningFactorsForValidation = new HashSet(); @@ -533,6 +536,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati populateConfigValuesForValidationSet(); weightBasedParametersForValidation(); overProvisioningFactorsForValidation(); + populateConfigKeysAllowedOnlyForDefaultAdmin(); initMessageBusListener(); return true; } @@ -596,6 +600,11 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati overprovisioningFactorsForValidation.add(CapacityManager.StorageOverprovisioningFactor.key()); } + protected void populateConfigKeysAllowedOnlyForDefaultAdmin() { + configKeysAllowedOnlyForDefaultAdmin.add(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key()); + configKeysAllowedOnlyForDefaultAdmin.add(AccountManagerImpl.allowOperationsOnUsersInSameAccount.key()); + } + private void initMessageBusListener() { messageBus.subscribe(EventTypes.EVENT_CONFIGURATION_VALUE_EDIT, new MessageSubscriber() { @Override @@ -1183,6 +1192,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati s_logger.error("Missing configuration variable " + name + " in configuration table"); return "Invalid configuration variable."; } + validateConfigurationAllowedOnlyForDefaultAdmin(name, value); final String configScope = cfg.getScope(); if (scope != null) { @@ -1347,6 +1357,33 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati return String.format("Invalid value for configuration [%s].", name); } + protected void validateConfigurationAllowedOnlyForDefaultAdmin(String configName, String value) { + if (configKeysAllowedOnlyForDefaultAdmin.contains(configName)) { + final Long userId = CallContext.current().getCallingUserId(); + if (userId != User.UID_ADMIN) { + throw new CloudRuntimeException("Only default admin is allowed to change this setting"); + } + + if (AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key().equals(configName)) { + if (value != null && !value.isBlank()) { + List validRoleTypes = Arrays.stream(RoleType.values()) + .map(Enum::name) + .collect(Collectors.toList()); + + boolean allValid = Arrays.stream(value.split(",")) + .map(String::trim) + .allMatch(validRoleTypes::contains); + + if (!allValid) { + throw new CloudRuntimeException("Invalid role types provided in value"); + } + } else { + throw new CloudRuntimeException("Value for role types must not be empty"); + } + } + } + } + /** * A valid value should be an integer between min and max (the values from the range). */ diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index 959a0dc3bb2..0e9fc0f80ef 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -784,6 +784,7 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim } else { _accountMgr.checkAccess(caller, null, true, account); } + _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account); ownerType = ResourceOwnerType.Account; ownerId = accountId; @@ -853,6 +854,11 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim throw new InvalidParameterValueException("Please specify a valid domain ID."); } _accountMgr.checkAccess(callerAccount, domain); + Account account = _entityMgr.findById(Account.class, accountId); + if (account == null) { + throw new InvalidParameterValueException("Unable to find account " + accountId); + } + _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account); if (resourceType != null) { resourceTypes.add(resourceType); diff --git a/server/src/main/java/com/cloud/user/AccountManager.java b/server/src/main/java/com/cloud/user/AccountManager.java index 1d7611d5b54..98939cb7deb 100644 --- a/server/src/main/java/com/cloud/user/AccountManager.java +++ b/server/src/main/java/com/cloud/user/AccountManager.java @@ -200,4 +200,5 @@ public interface AccountManager extends AccountService, Configurable { UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user); + void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount); } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 180f4e49521..cd07303a82d 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -20,6 +20,7 @@ import java.net.InetAddress; import java.net.URLEncoder; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -386,6 +387,22 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M true, ConfigKey.Scope.Domain); + public static ConfigKey listOfRoleTypesAllowedForOperationsOfSameRoleType = new ConfigKey<>("Hidden", + String.class, + "role.types.allowed.for.operations.on.accounts.of.same.role.type", + "Admin, ResourceAdmin, DomainAdmin", + "Comma separated list of role types that are allowed to do operations on accounts or users of the same role type within a domain.", + true, + ConfigKey.Scope.Domain); + + public static ConfigKey allowOperationsOnUsersInSameAccount = new ConfigKey<>("Hidden", + Boolean.class, + "allow.operations.on.users.in.same.account", + "true", + "Allow operations on users among them in the same account", + true, + ConfigKey.Scope.Domain); + protected AccountManagerImpl() { super(); } @@ -1369,7 +1386,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M /** * if there is any permission under the requested role that is not permitted for the caller, refuse */ - private void checkRoleEscalation(Account caller, Account requested) { + protected void checkRoleEscalation(Account caller, Account requested) { if (s_logger.isDebugEnabled()) { s_logger.debug(String.format("Checking if user of account %s [%s] with role-id [%d] can create an account of type %s [%s] with role-id [%d]", caller.getAccountName(), @@ -1471,7 +1488,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // users can't exist in same account assertUserNotAlreadyInAccount(duplicatedUser, account); } - + verifyCallerPrivilegeForUserOrAccountOperations(account); UserVO user = null; user = createUser(account.getId(), userName, password, firstName, lastName, email, timeZone, userUUID, source); return user; @@ -1488,10 +1505,14 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @ActionEvent(eventType = EventTypes.EVENT_USER_UPDATE, eventDescription = "Updating User") public UserAccount updateUser(UpdateUserCmd updateUserCmd) { UserVO user = retrieveAndValidateUser(updateUserCmd); + Account account = retrieveAndValidateAccount(user); + User caller = CallContext.current().getCallingUser(); + checkAccess(caller, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); + s_logger.debug("Updating user with Id: " + user.getUuid()); validateAndUpdateApiAndSecretKeyIfNeeded(updateUserCmd, user); - Account account = retrieveAndValidateAccount(user); validateAndUpdateFirstNameIfNeeded(updateUserCmd, user); validateAndUpdateLastNameIfNeeded(updateUserCmd, user); @@ -1514,6 +1535,85 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return _userAccountDao.findById(user.getId()); } + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + s_logger.debug(String.format("Verifying whether the caller has the correct privileges based on the user's role type and API permissions: %s", userAccount)); + + checkCallerRoleTypeAllowedForUserOrAccountOperations(userAccount, null); + checkCallerApiPermissionsForUserOrAccountOperations(userAccount); + } + + protected void verifyCallerPrivilegeForUserOrAccountOperations(User user) { + s_logger.debug(String.format("Verifying whether the caller has the correct privileges based on the user's role type and API permissions: %s", user)); + + Account userAccount = getAccount(user.getAccountId()); + checkCallerRoleTypeAllowedForUserOrAccountOperations(userAccount, user); + checkCallerApiPermissionsForUserOrAccountOperations(userAccount); + } + + protected void checkCallerRoleTypeAllowedForUserOrAccountOperations(Account userAccount, User user) { + Account callingAccount = getCurrentCallingAccount(); + RoleType callerRoleType = getRoleType(callingAccount); + RoleType userAccountRoleType = getRoleType(userAccount); + + if (RoleType.Unknown == callerRoleType || RoleType.Unknown == userAccountRoleType) { + String errMsg = String.format("The role type of account [%s, %s] or [%s, %s] is unknown", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + throw new PermissionDeniedException(errMsg); + } + + boolean isCallerSystemOrDefaultAdmin = callingAccount.getId() == Account.ACCOUNT_ID_SYSTEM || callingAccount.getId() == Account.ACCOUNT_ID_ADMIN; + if (isCallerSystemOrDefaultAdmin) { + s_logger.trace(String.format("Admin account [%s, %s] performing this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (callerRoleType.getId() < userAccountRoleType.getId()) { + s_logger.trace(String.format("The calling account [%s] has a higher role type than the user account [%s]", + callingAccount, userAccount)); + } else if (callerRoleType.getId() == userAccountRoleType.getId()) { + if (callingAccount.getId() != userAccount.getId()) { + String allowedRoleTypes = listOfRoleTypesAllowedForOperationsOfSameRoleType.valueInDomain(callingAccount.getDomainId()); + boolean updateAllowed = allowedRoleTypes != null && + Arrays.stream(allowedRoleTypes.split(",")) + .map(String::trim) + .anyMatch(role -> role.equals(callerRoleType.toString())); + if (BooleanUtils.isFalse(updateAllowed)) { + String errMsg = String.format("The calling account [%s, %s] is not allowed to perform this operation on users from other accounts " + + "of the same role type within the domain", callingAccount.getName(), callingAccount.getUuid()); + s_logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } + } else if ((callingAccount.getId() == userAccount.getId()) && user != null) { + Boolean allowOperationOnUsersinSameAccount = allowOperationsOnUsersInSameAccount.valueInDomain(callingAccount.getDomainId()); + User callingUser = CallContext.current().getCallingUser(); + if (callingUser.getId() != user.getId() && BooleanUtils.isFalse(allowOperationOnUsersinSameAccount)) { + String errMsg = "The user operations are not allowed by the users in the same account"; + s_logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } + } + } else { + String errMsg = String.format("The calling account [%s, %s] has a lower role type than the user account [%s, %s]", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + throw new PermissionDeniedException(errMsg); + } + } + + protected void checkCallerApiPermissionsForUserOrAccountOperations(Account userAccount) { + Account callingAccount = getCurrentCallingAccount(); + boolean isCallerRootAdmin = callingAccount.getId() == Account.ACCOUNT_ID_SYSTEM || isRootAdmin(callingAccount.getId()); + + if (isCallerRootAdmin) { + s_logger.trace(String.format("Admin account [%s, %s] performing this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (isRootAdmin(userAccount.getAccountId())) { + String errMsg = String.format("Account [%s, %s] cannot perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + s_logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } else { + s_logger.debug(String.format("Checking calling account [%s, %s] permission to perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + checkRoleEscalation(callingAccount, userAccount); + s_logger.debug(String.format("Calling account [%s, %s] is allowed to perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } + } + /** * Updates the password in the user POJO if needed. If no password is provided, then the password is not updated. * The following validations are executed if 'password' is not null. Admins (root admins or domain admins) can execute password updates without entering the current password. @@ -1760,6 +1860,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); boolean success = doSetUserStatus(userId, State.DISABLED); if (success) { @@ -1801,6 +1902,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); boolean success = Transaction.execute(new TransactionCallback() { @Override @@ -1853,6 +1955,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); // make sure the account is enabled too // if the user is either locked already or disabled already, don't change state...only lock currently enabled @@ -1909,6 +2012,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkIfAccountManagesProjects(accountId); + verifyCallerPrivilegeForUserOrAccountOperations(account); CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -1971,6 +2075,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check if user performing the action is allowed to modify this account Account caller = getCurrentCallingAccount(); checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); boolean success = enableAccount(account.getId()); if (success) { @@ -2004,6 +2109,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); if (lockAccount(account.getId())) { CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2034,6 +2140,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); if (disableAccount(account.getId())) { CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2079,6 +2186,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check if user performing the action is allowed to modify this account Account caller = getCurrentCallingAccount(); checkAccess(caller, _domainMgr.getDomain(account.getDomainId())); + verifyCallerPrivilegeForUserOrAccountOperations(account); if(newAccountName != null) { @@ -2160,6 +2268,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // don't allow to delete the user from the account of type Project checkAccountAndAccess(user, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); + return Transaction.execute((TransactionCallback) status -> deleteAndCleanupUser(user)); } @@ -2178,6 +2288,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M UserVO user = getValidUserVO(id); Account oldAccount = _accountDao.findById(user.getAccountId()); checkAccountAndAccess(user, oldAccount); + verifyCallerPrivilegeForUserOrAccountOperations(user); long domainId = oldAccount.getDomainId(); long newAccountId = getNewAccountId(domainId, cmd.getAccountName(), cmd.getAccountId()); @@ -2515,6 +2626,11 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + if (!Account.Type.PROJECT.equals(accountType)) { + AccountVO newAccount = new AccountVO(accountName, domainId, networkDomain, accountType, roleId, uuid); + verifyCallerPrivilegeForUserOrAccountOperations(newAccount); + } + // Create the account return Transaction.execute(new TransactionCallback() { @Override @@ -2863,8 +2979,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } final ControlledEntity account = getAccount(getUserAccountById(userId).getAccountId()); //Extracting the Account from the userID of the requested user. User caller = CallContext.current().getCallingUser(); - preventRootDomainAdminAccessToRootAdminKeys(caller, account); checkAccess(caller, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); Map keys = new HashMap(); keys.put("apikey", user.getApiKey()); @@ -2920,8 +3036,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } Account account = _accountDao.findById(user.getAccountId()); - preventRootDomainAdminAccessToRootAdminKeys(user, account); checkAccess(caller, null, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); // don't allow updating system user if (user.getId() == User.UID_SYSTEM) { @@ -3377,7 +3493,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M public ConfigKey[] getConfigKeys() { return new ConfigKey[] {UseSecretKeyInResponse, enableUserTwoFactorAuthentication, userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication, userTwoFactorAuthenticationIssuer, - userAllowMultipleAccounts}; + userAllowMultipleAccounts, listOfRoleTypesAllowedForOperationsOfSameRoleType, allowOperationsOnUsersInSameAccount}; } public List getUserTwoFactorAuthenticationProviders() { diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java index 8f4dbbe675d..f4541cd49f3 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java +++ b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java @@ -16,12 +16,16 @@ // under the License. package com.cloud.configuration; +import static org.junit.Assert.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.cloud.exception.InvalidParameterValueException; import com.cloud.storage.StorageManager; +import com.cloud.user.AccountManagerImpl; +import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; +import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.framework.config.ConfigDepot; import org.apache.cloudstack.framework.config.ConfigKey; import com.cloud.dc.dao.DataCenterDao; @@ -97,6 +101,7 @@ public class ConfigurationManagerImplTest { @Before public void setUp() throws Exception { configurationManagerImplSpy._configDepot = configDepot; + configurationManagerImplSpy.populateConfigKeysAllowedOnlyForDefaultAdmin(); } @Test @@ -474,4 +479,46 @@ public class ConfigurationManagerImplTest { Assert.assertThrows(InvalidParameterValueException.class, () -> configurationManagerImplSpy.checkIfDomainIsChildDomain(diskOfferingMock, accountMock, userMock, filteredDomainIds)); } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withAdminUser_shouldNotThrowException() { + CallContext callContext = mock(CallContext.class); + when(callContext.getCallingUserId()).thenReturn(User.UID_ADMIN); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key(), "Admin"); + } + } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withNonAdminUser_shouldThrowException() { + CallContext callContext = mock(CallContext.class); + when(callContext.getCallingUserId()).thenReturn(123L); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + assertThrows(CloudRuntimeException.class, () -> + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.allowOperationsOnUsersInSameAccount.key(), "Admin") + ); + } + } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withNonRestrictedKey_shouldNotThrowException() { + CallContext callContext = mock(CallContext.class); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin("some.other.config.key", "Admin"); + } + } + + @Test(expected = CloudRuntimeException.class) + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withValidConfigNameAndInvalidValue_shouldThrowException() { + CallContext callContext = mock(CallContext.class); + try (MockedStatic mockedCallContext = Mockito.mockStatic(CallContext.class)) { + mockedCallContext.when(CallContext::current).thenReturn(callContext); + when(callContext.getCallingUserId()).thenReturn(User.UID_ADMIN); + String invalidValue = "Admin, SuperUser"; + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key(), invalidValue); + } + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index f1cf0008676..8472a85cc30 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -108,6 +108,9 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Mock ConfigKey enableUserTwoFactorAuthenticationMock; + + @Mock + ConfigKey allowOperationsOnUsersInSameAccountMock; @Mock RoleService roleService; @@ -115,6 +118,9 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { public void setUp() throws Exception { enableUserTwoFactorAuthenticationMock = Mockito.mock(ConfigKey.class); accountManagerImpl.enableUserTwoFactorAuthentication = enableUserTwoFactorAuthenticationMock; + + allowOperationsOnUsersInSameAccountMock = Mockito.mock(ConfigKey.class); + accountManagerImpl.allowOperationsOnUsersInSameAccount = allowOperationsOnUsersInSameAccountMock; } @Before @@ -125,6 +131,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(accountMockId).when(userVoMock).getAccountId(); Mockito.doReturn(userVoIdMock).when(userVoMock).getId(); + + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(accountMock, accountMock); } @Test @@ -166,6 +174,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), Mockito.any(Domain.class))).thenReturn(true); Mockito.when(_vmSnapshotDao.listByAccountId(Mockito.anyLong())).thenReturn(new ArrayList()); Mockito.when(_autoscaleMgr.deleteAutoScaleVmGroupsByAccount(42l)).thenReturn(true); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations(Mockito.any(Account.class)); List sshkeyList = new ArrayList(); SSHKeyPairVO sshkey = new SSHKeyPairVO(); @@ -193,6 +202,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.when(_vmMgr.expunge(Mockito.any(UserVmVO.class))).thenReturn(false); Mockito.lenient().when(_domainMgr.getDomain(Mockito.anyLong())).thenReturn(domain); Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), Mockito.any(Domain.class))).thenReturn(true); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations(Mockito.any(Account.class)); Assert.assertTrue(accountManagerImpl.deleteUserAccount(42l)); // assert that this was NOT a clean delete @@ -261,7 +271,6 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { // Queried account - admin account AccountVO adminAccountMock = Mockito.mock(AccountVO.class); - Mockito.when(adminAccountMock.getAccountId()).thenReturn(2L); Mockito.when(_accountDao.findByIdIncludingRemoved(2L)).thenReturn(adminAccountMock); Mockito.lenient().when(accountService.isRootAdmin(2L)).thenReturn(true); Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), @@ -309,6 +318,12 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Test public void updateUserTestTimeZoneAndEmailNull() { + Mockito.when(userVoMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(accountMock).when(accountManagerImpl).getAccount(10L); + Mockito.when(accountMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(10L); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.User); + prepareMockAndExecuteUpdateUserTest(0); } @@ -316,6 +331,11 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { public void updateUserTestTimeZoneAndEmailNotNull() { Mockito.when(UpdateUserCmdMock.getEmail()).thenReturn("email"); Mockito.when(UpdateUserCmdMock.getTimezone()).thenReturn("timezone"); + Mockito.when(userVoMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(accountMock).when(accountManagerImpl).getAccount(10L); + Mockito.when(accountMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(10L); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.User); prepareMockAndExecuteUpdateUserTest(1); } @@ -333,14 +353,17 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(true).when(userDaoMock).update(Mockito.anyLong(), Mockito.eq(userVoMock)); Mockito.doReturn(Mockito.mock(UserAccountVO.class)).when(userAccountDaoMock).findById(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).checkAccess(nullable(User.class), nullable(Account.class)); accountManagerImpl.updateUser(UpdateUserCmdMock); + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(accountMock, accountMock); + InOrder inOrder = Mockito.inOrder(userVoMock, accountManagerImpl, userDaoMock, userAccountDaoMock); inOrder.verify(accountManagerImpl).retrieveAndValidateUser(UpdateUserCmdMock); - inOrder.verify(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).retrieveAndValidateAccount(userVoMock); + inOrder.verify(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).validateAndUpdateFirstNameIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).validateAndUpdateLastNameIfNeeded(UpdateUserCmdMock, userVoMock); @@ -1203,8 +1226,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { accountManagerImpl.validateRoleChange(account, newRole, caller); } - @Test - public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNotAProjectAdministrator() { + @Test + public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNotAProjectAdministrator() { long accountId = 1L; List managedProjectIds = new ArrayList<>(); @@ -1341,4 +1364,74 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount); } + + @Test + public void testCheckCallerRoleTypeAllowedToUpdateUserSameAccount() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(accountMock); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.DomainAdmin); + + accountManagerImpl.checkCallerRoleTypeAllowedForUserOrAccountOperations(accountMock, userVoMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckCallerRoleTypeAllowedToUpdateUserLowerAccountRoleType() { + Account callingAccount = Mockito.mock(Account.class); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(2L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(2L); + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(callingAccount))).thenReturn(RoleType.DomainAdmin); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.Admin); + accountManagerImpl.checkCallerRoleTypeAllowedForUserOrAccountOperations(accountMock, userVoMock); + } + + @Test + public void testcheckCallerApiPermissionsForUserOperationsRootAdminSameCaller() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(accountMock); + Mockito.when(accountMock.getId()).thenReturn(2L); + Mockito.doReturn(true).when(accountManagerImpl).isRootAdmin(2L); + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testcheckCallerApiPermissionsForUserOperationsRootAdminDifferentAccount() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(true).when(accountManagerImpl).isRootAdmin(2L); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test + public void testcheckCallerApiPermissionsForUserOperationsAllowedApis() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(2L); + + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(callingAccount, accountMock); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testcheckCallerApiPermissionsForUserOperationsNotAllowedApis() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(2L); + + Mockito.lenient().doThrow(PermissionDeniedException.class).when(accountManagerImpl).checkRoleEscalation(callingAccount, accountMock); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java index 21474a53ee7..31a12c0432c 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java @@ -193,6 +193,7 @@ public class AccountManagerImplVolumeDeleteEventTest extends AccountManagetImplT public void destroyedVMRootVolumeUsageEvent() throws SecurityException, IllegalArgumentException, ReflectiveOperationException, AgentUnavailableException, ConcurrentOperationException, CloudException { Mockito.lenient().doReturn(vm).when(_vmMgr).destroyVm(nullable(Long.class), nullable(Boolean.class)); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations(Mockito.any(Account.class)); List emittedEvents = deleteUserAccountRootVolumeUsageEvents(true); Assert.assertEquals(0, emittedEvents.size()); } @@ -204,6 +205,7 @@ public class AccountManagerImplVolumeDeleteEventTest extends AccountManagetImplT throws SecurityException, IllegalArgumentException, ReflectiveOperationException, AgentUnavailableException, ConcurrentOperationException, CloudException { Mockito.doNothing().when(vmStatsDaoMock).removeAllByVmId(Mockito.anyLong()); Mockito.lenient().when(_vmMgr.destroyVm(nullable(Long.class), nullable(Boolean.class))).thenReturn(vm); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations(Mockito.any(Account.class)); List emittedEvents = deleteUserAccountRootVolumeUsageEvents(false); UsageEventVO event = emittedEvents.get(0); Assert.assertEquals(EventTypes.EVENT_VOLUME_DELETE, event.getType()); diff --git a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java index 96b73cc63dd..414b98aacfd 100644 --- a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java +++ b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java @@ -488,4 +488,8 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) { return null; } + + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + } } From 64d83ce9d127e40bd40cd1af4784b26fabf923af Mon Sep 17 00:00:00 2001 From: nvazquez Date: Wed, 9 Apr 2025 07:50:32 -0300 Subject: [PATCH 10/14] Fix access to template/ISO list for domain/resource admins In Apache CloudStack, while using the listTemplates and listIsos APIs, Domain Admins and Resource Admins can retrieve templates and ISOs outside their intended scope. Co-authored-by: bernardodemarco Co-authored-by: nvazquez --- server/src/main/java/com/cloud/api/query/QueryManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java index 7c3e9391989..a2edc05a492 100644 --- a/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java +++ b/server/src/main/java/com/cloud/api/query/QueryManagerImpl.java @@ -4660,7 +4660,7 @@ public class QueryManagerImpl extends MutualExclusiveIdsManagerBase implements Q if (!permittedAccounts.isEmpty()) { domain = _domainDao.findById(permittedAccounts.get(0).getDomainId()); } else { - domain = _domainDao.findById(Domain.ROOT_DOMAIN); + domain = _domainDao.findById(caller.getDomainId()); } setIdsListToSearchCriteria(sc, ids); From c3c6d3458930319fc50c0f2f0a91602f373a59d0 Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Thu, 16 Jan 2025 11:10:40 -0300 Subject: [PATCH 11/14] Add access validation to Quota email APIs --- .../cloudstack/api/command/QuotaConfigureEmailCmd.java | 2 ++ .../api/command/QuotaListEmailConfigurationCmd.java | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaConfigureEmailCmd.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaConfigureEmailCmd.java index 01d9ffc1529..f658783179f 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaConfigureEmailCmd.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaConfigureEmailCmd.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.api.command; import com.cloud.utils.Pair; +import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseCmd; @@ -32,6 +33,7 @@ import javax.inject.Inject; requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class QuotaConfigureEmailCmd extends BaseCmd { + @ACL @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, required = true, description = "Account ID for which to configure quota template email or min balance") private long accountId; diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaListEmailConfigurationCmd.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaListEmailConfigurationCmd.java index 8915158461f..294738a7b99 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaListEmailConfigurationCmd.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaListEmailConfigurationCmd.java @@ -16,7 +16,7 @@ //under the License. package org.apache.cloudstack.api.command; -import com.cloud.user.Account; +import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseCmd; @@ -32,6 +32,7 @@ import javax.inject.Inject; requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class QuotaListEmailConfigurationCmd extends BaseCmd { + @ACL @Parameter(name = ApiConstants.ACCOUNT_ID, type = BaseCmd.CommandType.UUID, entityType = AccountResponse.class, required = true, description = "Account ID for which to list quota template email configurations") private long accountId; @@ -49,6 +50,6 @@ public class QuotaListEmailConfigurationCmd extends BaseCmd { @Override public long getEntityOwnerId() { - return Account.ACCOUNT_ID_SYSTEM; + return accountId; } } From 9cf9966dfea8c1fc51a152d7fb3ff62ac2385c7a Mon Sep 17 00:00:00 2001 From: Suresh Kumar Anaparti Date: Wed, 16 Apr 2025 15:25:26 +0530 Subject: [PATCH 12/14] Keep same/consistent auth time for valid & invalid users --- .../com/cloud/user/AccountManagerImpl.java | 216 ++++++++++-------- 1 file changed, 119 insertions(+), 97 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 4913b9a56b5..db2b5f32a7f 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -350,6 +350,9 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M private List userTwoFactorAuthenticationProviders; + private long validUserLastAuthTimeDurationInMs = 0L; + private static final long DEFAULT_USER_AUTH_TIME_DURATION_MS = 350L; + public static ConfigKey enableUserTwoFactorAuthentication = new ConfigKey<>("Advanced", Boolean.class, "enable.user.2fa", @@ -1599,7 +1602,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M for (UserAuthenticator userAuthenticator : _userPasswordEncoders) { Pair authenticationResult = userAuthenticator.authenticate(user.getUsername(), currentPassword, userAccount.getDomainId(), null); if (authenticationResult == null) { - logger.trace(String.format("Authenticator [%s] is returning null for the authenticate mehtod.", userAuthenticator.getClass())); + logger.trace(String.format("Authenticator [%s] is returning null for the authenticate method.", userAuthenticator.getClass())); continue; } if (BooleanUtils.toBoolean(authenticationResult.first())) { @@ -2644,107 +2647,17 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @Override public UserAccount authenticateUser(final String username, final String password, final Long domainId, final InetAddress loginIpAddress, final Map requestParameters) { + long authStartTimeInMs = System.currentTimeMillis(); UserAccount user = null; final String[] oAuthProviderArray = (String[])requestParameters.get(ApiConstants.PROVIDER); final String[] secretCodeArray = (String[])requestParameters.get(ApiConstants.SECRET_CODE); String oauthProvider = ((oAuthProviderArray == null) ? null : oAuthProviderArray[0]); String secretCode = ((secretCodeArray == null) ? null : secretCodeArray[0]); - if ((password != null && !password.isEmpty()) || (oauthProvider != null && secretCode != null)) { user = getUserAccount(username, password, domainId, requestParameters); } else { - String key = _configDao.getValue("security.singlesignon.key"); - if (key == null) { - // the SSO key is gone, don't authenticate - return null; - } - - String singleSignOnTolerance = _configDao.getValue("security.singlesignon.tolerance.millis"); - if (singleSignOnTolerance == null) { - // the SSO tolerance is gone (how much time before/after system time we'll allow the login request to be - // valid), - // don't authenticate - return null; - } - - long tolerance = Long.parseLong(singleSignOnTolerance); - String signature = null; - long timestamp = 0L; - String unsignedRequest; - StringBuffer unsignedRequestBuffer = new StringBuffer(); - - // - build a request string with sorted params, make sure it's all lowercase - // - sign the request, verify the signature is the same - List parameterNames = new ArrayList<>(); - - for (Object paramNameObj : requestParameters.keySet()) { - parameterNames.add((String)paramNameObj); // put the name in a list that we'll sort later - } - - Collections.sort(parameterNames); - - try { - for (String paramName : parameterNames) { - // parameters come as name/value pairs in the form String/String[] - String paramValue = ((String[])requestParameters.get(paramName))[0]; - - if ("signature".equalsIgnoreCase(paramName)) { - signature = paramValue; - } else { - if ("timestamp".equalsIgnoreCase(paramName)) { - String timestampStr = paramValue; - try { - // If the timestamp is in a valid range according to our tolerance, verify the request - // signature, otherwise return null to indicate authentication failure - timestamp = Long.parseLong(timestampStr); - long currentTime = System.currentTimeMillis(); - if (Math.abs(currentTime - timestamp) > tolerance) { - if (logger.isDebugEnabled()) { - logger.debug("Expired timestamp passed in to login, current time = " + currentTime + ", timestamp = " + timestamp); - } - return null; - } - } catch (NumberFormatException nfe) { - if (logger.isDebugEnabled()) { - logger.debug("Invalid timestamp passed in to login: " + timestampStr); - } - return null; - } - } - - if (unsignedRequestBuffer.length() != 0) { - unsignedRequestBuffer.append("&"); - } - unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, "UTF-8")); - } - } - - if ((signature == null) || (timestamp == 0L)) { - if (logger.isDebugEnabled()) { - logger.debug("Missing parameters in login request, signature = " + signature + ", timestamp = " + timestamp); - } - return null; - } - - unsignedRequest = unsignedRequestBuffer.toString().toLowerCase().replaceAll("\\+", "%20"); - - Mac mac = Mac.getInstance("HmacSHA1"); - SecretKeySpec keySpec = new SecretKeySpec(key.getBytes(), "HmacSHA1"); - mac.init(keySpec); - mac.update(unsignedRequest.getBytes()); - byte[] encryptedBytes = mac.doFinal(); - String computedSignature = new String(Base64.encodeBase64(encryptedBytes)); - boolean equalSig = ConstantTimeComparator.compareStrings(signature, computedSignature); - if (!equalSig) { - logger.info("User signature: " + signature + " is not equaled to computed signature: " + computedSignature); - } else { - user = _userAccountDao.getUserAccount(username, domainId); - } - } catch (Exception ex) { - logger.error("Exception authenticating user", ex); - return null; - } + user = getUserAccountForSSO(username, domainId, requestParameters); } if (user != null) { @@ -2778,18 +2691,35 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + ActionEventUtils.onActionEvent(user.getId(), user.getAccountId(), user.getDomainId(), EventTypes.EVENT_USER_LOGIN, "user has logged in from IP Address " + loginIpAddress, user.getId(), ApiCommandResourceType.User.toString()); + + validUserLastAuthTimeDurationInMs = System.currentTimeMillis() - authStartTimeInMs; // Here all is fine! if (logger.isDebugEnabled()) { - logger.debug("User: " + username + " in domain " + domainId + " has successfully logged in"); + logger.debug(String.format("User: %s in domain %d has successfully logged in, auth time duration - %d ms", username, domainId, validUserLastAuthTimeDurationInMs)); } - ActionEventUtils.onActionEvent(user.getId(), user.getAccountId(), user.getDomainId(), EventTypes.EVENT_USER_LOGIN, "user has logged in from IP Address " + loginIpAddress, user.getId(), ApiCommandResourceType.User.toString()); - return user; } else { if (logger.isDebugEnabled()) { logger.debug("User: " + username + " in domain " + domainId + " has failed to log in"); } + + long waitTimeDurationInMs; + long invalidUserAuthTimeDurationInMs = System.currentTimeMillis() - authStartTimeInMs; + if (validUserLastAuthTimeDurationInMs > 0) { + waitTimeDurationInMs = validUserLastAuthTimeDurationInMs - invalidUserAuthTimeDurationInMs; + } else { + waitTimeDurationInMs = DEFAULT_USER_AUTH_TIME_DURATION_MS - invalidUserAuthTimeDurationInMs; + } + + if (waitTimeDurationInMs > 0) { + try { + Thread.sleep(waitTimeDurationInMs); + } catch (final InterruptedException e) { + } + } + return null; } } @@ -2827,7 +2757,6 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M boolean updateIncorrectLoginCount = actionsOnFailedAuthenticaion.contains(ActionOnFailedAuthentication.INCREMENT_INCORRECT_LOGIN_ATTEMPT_COUNT); if (authenticated) { - Domain domain = _domainMgr.getDomain(domainId); String domainName = null; if (domain != null) { @@ -2869,6 +2798,99 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + private UserAccount getUserAccountForSSO(String username, Long domainId, Map requestParameters) { + String key = _configDao.getValue("security.singlesignon.key"); + if (key == null) { + // the SSO key is gone, don't authenticate + return null; + } + + String singleSignOnTolerance = _configDao.getValue("security.singlesignon.tolerance.millis"); + if (singleSignOnTolerance == null) { + // the SSO tolerance is gone (how much time before/after system time we'll allow the login request to be + // valid), + // don't authenticate + return null; + } + + UserAccount user = null; + long tolerance = Long.parseLong(singleSignOnTolerance); + String signature = null; + long timestamp = 0L; + String unsignedRequest; + StringBuffer unsignedRequestBuffer = new StringBuffer(); + + // - build a request string with sorted params, make sure it's all lowercase + // - sign the request, verify the signature is the same + List parameterNames = new ArrayList<>(); + + for (Object paramNameObj : requestParameters.keySet()) { + parameterNames.add((String)paramNameObj); // put the name in a list that we'll sort later + } + + Collections.sort(parameterNames); + + try { + for (String paramName : parameterNames) { + // parameters come as name/value pairs in the form String/String[] + String paramValue = ((String[])requestParameters.get(paramName))[0]; + + if ("signature".equalsIgnoreCase(paramName)) { + signature = paramValue; + } else { + if ("timestamp".equalsIgnoreCase(paramName)) { + String timestampStr = paramValue; + try { + // If the timestamp is in a valid range according to our tolerance, verify the request + // signature, otherwise return null to indicate authentication failure + timestamp = Long.parseLong(timestampStr); + long currentTime = System.currentTimeMillis(); + if (Math.abs(currentTime - timestamp) > tolerance) { + logger.debug("Expired timestamp passed in to login, current time = {}, timestamp = {}", currentTime, timestamp); + return null; + } + } catch (NumberFormatException nfe) { + logger.debug("Invalid timestamp passed in to login: {}", timestampStr); + return null; + } + } + + if (unsignedRequestBuffer.length() != 0) { + unsignedRequestBuffer.append("&"); + } + unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, "UTF-8")); + } + } + + if ((signature == null) || (timestamp == 0L)) { + if (logger.isDebugEnabled()) { + logger.debug("Missing parameters in login request, signature = " + signature + ", timestamp = " + timestamp); + } + return null; + } + + unsignedRequest = unsignedRequestBuffer.toString().toLowerCase().replaceAll("\\+", "%20"); + + Mac mac = Mac.getInstance("HmacSHA1"); + SecretKeySpec keySpec = new SecretKeySpec(key.getBytes(), "HmacSHA1"); + mac.init(keySpec); + mac.update(unsignedRequest.getBytes()); + byte[] encryptedBytes = mac.doFinal(); + String computedSignature = new String(Base64.encodeBase64(encryptedBytes)); + boolean equalSig = ConstantTimeComparator.compareStrings(signature, computedSignature); + if (!equalSig) { + logger.info("User signature: " + signature + " is not equaled to computed signature: " + computedSignature); + } else { + user = _userAccountDao.getUserAccount(username, domainId); + } + } catch (Exception ex) { + logger.error("Exception authenticating user", ex); + return null; + } + + return user; + } + protected void updateLoginAttemptsWhenIncorrectLoginAttemptsEnabled(UserAccount account, boolean updateIncorrectLoginCount, int allowedLoginAttempts) { int attemptsMade = account.getLoginAttempts() + 1; From 19d6b979af64555955f20ad027876f5a139cea0a Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 21 Feb 2025 11:16:22 +0530 Subject: [PATCH 13/14] cks: create separate service account in project A separate service account will be created and added in the project, if not exist already, when a Kubernetes cluster is deployed in a project. This account will have a role with limited API access. Cleanup clusters on owner account cleanup, delete service account if needed When the owner account of k8s clusters is deleted, while its node VMs get expunged, the cluster entry in DB remain present. This fixes the issue by cleaning up all clusters for the account deleted. Project k8s service account will be deleted on account cleanup or when there is no active k8s cluster remaining Signed-off-by: Abhishek Kumar --- .../cluster/KubernetesServiceHelper.java | 2 + .../java/com/cloud/user/dao/AccountDao.java | 8 +- .../com/cloud/user/dao/AccountDaoImpl.java | 25 +- .../cluster/KubernetesClusterManagerImpl.java | 256 ++++++++++++++++-- .../cluster/KubernetesClusterService.java | 8 +- .../cluster/KubernetesServiceHelperImpl.java | 8 + .../cluster/dao/KubernetesClusterDao.java | 4 +- .../cluster/dao/KubernetesClusterDaoImpl.java | 33 ++- .../com/cloud/user/AccountManagerImpl.java | 13 + 9 files changed, 306 insertions(+), 51 deletions(-) diff --git a/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java b/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java index a13c1b3a6a8..84e898528b5 100644 --- a/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java +++ b/api/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelper.java @@ -18,6 +18,7 @@ package com.cloud.kubernetes.cluster; import org.apache.cloudstack.acl.ControlledEntity; +import com.cloud.user.Account; import com.cloud.uservm.UserVm; import com.cloud.utils.component.Adapter; @@ -26,4 +27,5 @@ public interface KubernetesServiceHelper extends Adapter { ControlledEntity findByUuid(String uuid); ControlledEntity findByVmId(long vmId); void checkVmCanBeDestroyed(UserVm userVm); + void cleanupForAccount(Account account); } diff --git a/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java b/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java index 17b07496731..dae5f3a3467 100644 --- a/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java +++ b/engine/schema/src/main/java/com/cloud/user/dao/AccountDao.java @@ -16,6 +16,9 @@ // under the License. package com.cloud.user.dao; +import java.util.Date; +import java.util.List; + import com.cloud.user.Account; import com.cloud.user.AccountVO; import com.cloud.user.User; @@ -23,9 +26,6 @@ import com.cloud.utils.Pair; import com.cloud.utils.db.Filter; import com.cloud.utils.db.GenericDao; -import java.util.Date; -import java.util.List; - public interface AccountDao extends GenericDao { Pair findUserAccountByApiKey(String apiKey); @@ -33,6 +33,8 @@ public interface AccountDao extends GenericDao { Pair, Integer> findAccountsLike(String accountName, Filter filter); + List findAccountsByName(String accountName); + List findActiveAccounts(Long maxAccountId, Filter filter); List findRecentlyDeletedAccounts(Long maxAccountId, Date earliestRemovedDate, Filter filter); diff --git a/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java b/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java index 2654b22374f..f5f95d5da1f 100644 --- a/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java @@ -16,6 +16,14 @@ // under the License. package com.cloud.user.dao; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.util.Date; +import java.util.List; + +import org.apache.commons.lang3.StringUtils; +import org.springframework.stereotype.Component; + import com.cloud.user.Account; import com.cloud.user.Account.State; import com.cloud.user.AccountVO; @@ -30,14 +38,7 @@ import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; import com.cloud.utils.db.SearchCriteria.Func; import com.cloud.utils.db.SearchCriteria.Op; -import org.apache.commons.lang3.StringUtils; import com.cloud.utils.db.TransactionLegacy; -import org.springframework.stereotype.Component; - -import java.sql.PreparedStatement; -import java.sql.ResultSet; -import java.util.Date; -import java.util.List; @Component public class AccountDaoImpl extends GenericDaoBase implements AccountDao { @@ -190,6 +191,16 @@ public class AccountDaoImpl extends GenericDaoBase implements A return searchAndCount(sc, filter); } + @Override + public List findAccountsByName(String accountName) { + SearchBuilder sb = createSearchBuilder(); + sb.and("accountName", sb.entity().getAccountName(), SearchCriteria.Op.EQ); + sb.done(); + SearchCriteria sc = sb.create(); + sc.setParameters("accountName", accountName); + return search(sc, null); + } + @Override public Account findEnabledAccount(String accountName, Long domainId) { SearchCriteria sc = AllFieldsSearch.create("accountName", accountName); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java index 131d7b22606..5e86bb6daf0 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java @@ -44,6 +44,11 @@ import javax.naming.ConfigurationException; import com.cloud.uservm.UserVm; import com.cloud.vm.UserVmService; import org.apache.cloudstack.acl.ControlledEntity; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RolePermissionEntity; +import org.apache.cloudstack.acl.RoleService; +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.acl.Rule; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; @@ -52,6 +57,14 @@ import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiConstants.VMDetails; import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.ResponseObject.ResponseView; +import org.apache.cloudstack.api.command.user.address.AssociateIPAddrCmd; +import org.apache.cloudstack.api.command.user.address.DisassociateIPAddrCmd; +import org.apache.cloudstack.api.command.user.address.ListPublicIpAddressesCmd; +import org.apache.cloudstack.api.command.user.firewall.CreateFirewallRuleCmd; +import org.apache.cloudstack.api.command.user.firewall.DeleteFirewallRuleCmd; +import org.apache.cloudstack.api.command.user.firewall.ListFirewallRulesCmd; +import org.apache.cloudstack.api.command.user.firewall.UpdateFirewallRuleCmd; +import org.apache.cloudstack.api.command.user.job.QueryAsyncJobResultCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.AddVirtualMachinesToKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.CreateKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.DeleteKubernetesClusterCmd; @@ -62,6 +75,18 @@ import org.apache.cloudstack.api.command.user.kubernetes.cluster.ScaleKubernetes import org.apache.cloudstack.api.command.user.kubernetes.cluster.StartKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.StopKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.UpgradeKubernetesClusterCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.AssignToLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.CreateLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.DeleteLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.ListLoadBalancerRuleInstancesCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.ListLoadBalancerRulesCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.RemoveFromLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.loadbalancer.UpdateLoadBalancerRuleCmd; +import org.apache.cloudstack.api.command.user.network.CreateNetworkACLCmd; +import org.apache.cloudstack.api.command.user.network.DeleteNetworkACLCmd; +import org.apache.cloudstack.api.command.user.network.ListNetworkACLsCmd; +import org.apache.cloudstack.api.command.user.network.ListNetworksCmd; +import org.apache.cloudstack.api.command.user.vm.ListVMsCmd; import org.apache.cloudstack.api.response.KubernetesClusterConfigResponse; import org.apache.cloudstack.api.response.KubernetesClusterResponse; import org.apache.cloudstack.api.response.ListResponse; @@ -147,6 +172,8 @@ import com.cloud.offerings.dao.NetworkOfferingServiceMapDao; import com.cloud.org.Cluster; import com.cloud.org.Grouping; import com.cloud.projects.Project; +import com.cloud.projects.ProjectAccount; +import com.cloud.projects.ProjectManager; import com.cloud.resource.ResourceManager; import com.cloud.service.ServiceOfferingVO; import com.cloud.service.dao.ServiceOfferingDao; @@ -155,14 +182,17 @@ import com.cloud.storage.dao.VMTemplateDao; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.AccountService; +import com.cloud.user.AccountVO; import com.cloud.user.SSHKeyPairVO; import com.cloud.user.User; import com.cloud.user.UserAccount; import com.cloud.user.UserVO; +import com.cloud.user.dao.AccountDao; import com.cloud.user.dao.SSHKeyPairDao; import com.cloud.user.dao.UserDao; import com.cloud.utils.Pair; import com.cloud.utils.Ternary; +import com.cloud.utils.UuidUtils; import com.cloud.utils.component.ComponentContext; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.concurrency.NamedThreadFactory; @@ -186,12 +216,40 @@ import org.apache.logging.log4j.Level; public class KubernetesClusterManagerImpl extends ManagerBase implements KubernetesClusterService { private static final String DEFAULT_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_NAME = "DefaultNetworkOfferingforKubernetesService"; + private static final String DEFAULT_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_DISPLAY_TEXT = "Network Offering used for CloudStack Kubernetes service"; private static final String DEFAULT_NSX_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_NAME = "DefaultNSXNetworkOfferingforKubernetesService"; private static final String DEFAULT_NSX_VPC_TIER_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_NAME = "DefaultNSXVPCNetworkOfferingforKubernetesService"; private static final String DEFAULT_NSX_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_DISPLAY_TEXT = "Network Offering for NSX CloudStack Kubernetes Service"; private static final String DEFAULT_NSX_VPC_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_DISPLAY_TEXT = "Network Offering for NSX CloudStack Kubernetes service on VPC"; + private static final List> PROJECT_KUBERNETES_ACCOUNT_ROLE_ALLOWED_APIS = Arrays.asList( + QueryAsyncJobResultCmd.class, + ListVMsCmd.class, + ListNetworksCmd.class, + ListPublicIpAddressesCmd.class, + AssociateIPAddrCmd.class, + DisassociateIPAddrCmd.class, + ListLoadBalancerRulesCmd.class, + CreateLoadBalancerRuleCmd.class, + UpdateLoadBalancerRuleCmd.class, + DeleteLoadBalancerRuleCmd.class, + AssignToLoadBalancerRuleCmd.class, + RemoveFromLoadBalancerRuleCmd.class, + ListLoadBalancerRuleInstancesCmd.class, + ListFirewallRulesCmd.class, + CreateFirewallRuleCmd.class, + UpdateFirewallRuleCmd.class, + DeleteFirewallRuleCmd.class, + ListNetworkACLsCmd.class, + CreateNetworkACLCmd.class, + DeleteNetworkACLCmd.class, + ListKubernetesClustersCmd.class, + ScaleKubernetesClusterCmd.class + ); + private static final String PROJECT_KUBERNETES_ACCOUNT_FIRST_NAME = "Kubernetes"; + private static final String PROJECT_KUBERNETES_ACCOUNT_LAST_NAME = "Service User"; + protected StateMachine2 _stateMachine = KubernetesCluster.State.getStateMachine(); ScheduledExecutorService _gcExecutor; @@ -263,11 +321,16 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne public SecurityGroupService securityGroupService; @Inject public NetworkHelper networkHelper; - @Inject private UserVmService userVmService; @Inject RoutedIpv4Manager routedIpv4Manager; + @Inject + public AccountDao accountDao; + @Inject + public ProjectManager projectManager; + @Inject + RoleService roleService; private void logMessage(final Level logLevel, final String message, final Exception e) { if (logLevel == Level.WARN) { @@ -1380,23 +1443,31 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne } } - private String[] getServiceUserKeys(KubernetesClusterVO kubernetesCluster) { - Account owner = accountService.getActiveAccountById(kubernetesCluster.getAccountId()); - if (owner == null || owner.getType() == Account.Type.PROJECT) { - owner = CallContext.current().getCallingAccount(); + protected String[] createUserApiKeyAndSecretKey(long userId) { + CallContext.register(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM); + try { + return accountService.createApiKeyAndSecretKey(userId); + } finally { + CallContext.unregister(); + } + } + + protected String[] getServiceUserKeys(Account owner) { + String username = owner.getAccountName(); + if (!username.startsWith(KUBEADMIN_ACCOUNT_NAME + "-")) { + username += "-" + KUBEADMIN_ACCOUNT_NAME; } - String username = owner.getAccountName() + "-" + KUBEADMIN_ACCOUNT_NAME; UserAccount kubeadmin = accountService.getActiveUserAccount(username, owner.getDomainId()); - String[] keys = null; + String[] keys; if (kubeadmin == null) { User kube = userDao.persist(new UserVO(owner.getAccountId(), username, UUID.randomUUID().toString(), owner.getAccountName(), - KUBEADMIN_ACCOUNT_NAME, "kubeadmin", null, UUID.randomUUID().toString(), User.Source.UNKNOWN)); - keys = accountService.createApiKeyAndSecretKey(kube.getId()); + KUBEADMIN_ACCOUNT_NAME, "kubeadmin", null, UUID.randomUUID().toString(), User.Source.UNKNOWN)); + keys = createUserApiKeyAndSecretKey(kube.getId()); } else { String apiKey = kubeadmin.getApiKey(); String secretKey = kubeadmin.getSecretKey(); if (StringUtils.isAnyEmpty(apiKey, secretKey)) { - keys = accountService.createApiKeyAndSecretKey(kubeadmin.getId()); + keys = createUserApiKeyAndSecretKey(kubeadmin.getId()); } else { keys = new String[]{apiKey, secretKey}; } @@ -1404,6 +1475,76 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne return keys; } + protected Role createProjectKubernetesAccountRole() { + Role role = roleService.createRole(PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME, RoleType.User, + PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME, false); + for (Class allowedApi : PROJECT_KUBERNETES_ACCOUNT_ROLE_ALLOWED_APIS) { + final String apiName = BaseCmd.getCommandNameByClass(allowedApi); + roleService.createRolePermission(role, new Rule(apiName), RolePermissionEntity.Permission.ALLOW, + String.format("Allow %s", apiName)); + } + roleService.createRolePermission(role, new Rule("*"), RolePermissionEntity.Permission.DENY, + "Deny all"); + logger.debug(String.format("Created default role for Kubernetes service account in projects: %s", role)); + return role; + } + + public Role getProjectKubernetesAccountRole() { + List roles = roleService.findRolesByName(PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME); + if (CollectionUtils.isNotEmpty(roles)) { + Role role = roles.get(0); + logger.debug(String.format("Found default role for Kubernetes service account in projects: %s", role)); + return role; + } + return createProjectKubernetesAccountRole(); + } + + protected Account createProjectKubernetesAccount(final Project project, final String accountName) { + CallContext.register(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM); + try { + Role role = getProjectKubernetesAccountRole(); + UserAccount userAccount = accountService.createUserAccount(accountName, + UuidUtils.first(UUID.randomUUID().toString()), PROJECT_KUBERNETES_ACCOUNT_FIRST_NAME, + PROJECT_KUBERNETES_ACCOUNT_LAST_NAME, null, null, accountName, Account.Type.NORMAL, role.getId(), + project.getDomainId(), null, null, null, null, User.Source.NATIVE); + projectManager.assignAccountToProject(project, userAccount.getAccountId(), ProjectAccount.Role.Regular, + userAccount.getId(), null); + Account account = accountService.getAccount(userAccount.getAccountId()); + logger.debug(String.format("Created Kubernetes service account in project %s: %s", project, account)); + return account; + } finally { + CallContext.unregister(); + } + } + + protected Account getProjectKubernetesAccount(final Account callerAccount, final boolean create) { + Project project = ApiDBUtils.findProjectByProjectAccountId(callerAccount.getId()); + final String accountName = String.format("%s-%s", KUBEADMIN_ACCOUNT_NAME, UuidUtils.first(project.getUuid())); + List accounts = accountDao.findAccountsByName(accountName); + for (AccountVO account : accounts) { + if (projectManager.canAccessProjectAccount(account, project.getProjectAccountId())) { + logger.debug(String.format("Created Kubernetes service account in project %s: %s", project, account)); + return account; + } + } + return create ? createProjectKubernetesAccount(project, accountName) : null; + } + + protected Account getProjectKubernetesAccount(final Account callerAccount) { + return getProjectKubernetesAccount(callerAccount, true); + } + + private String[] getServiceUserKeys(KubernetesClusterVO kubernetesCluster) { + Account owner = accountService.getActiveAccountById(kubernetesCluster.getAccountId()); + if (owner == null) { + owner = CallContext.current().getCallingAccount(); + } + if (owner.getType() == Account.Type.PROJECT) { + owner = getProjectKubernetesAccount(owner); + } + return getServiceUserKeys(owner); + } + @Override @ActionEvent(eventType = KubernetesClusterEventTypes.EVENT_KUBERNETES_CLUSTER_STOP, eventDescription = "stopping Kubernetes cluster", async = true) @@ -1448,15 +1589,13 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne logAndThrow(Level.ERROR, "Kubernetes Service plugin is disabled"); } Long kubernetesClusterId = cmd.getId(); - KubernetesClusterVO cluster = kubernetesClusterDao.findById(kubernetesClusterId); + final KubernetesClusterVO cluster = kubernetesClusterDao.findById(kubernetesClusterId); if (cluster == null) { throw new InvalidParameterValueException("Invalid cluster id specified"); } accountManager.checkAccess(CallContext.current().getCallingAccount(), SecurityChecker.AccessType.OperateEntry, false, cluster); if (cluster.getClusterType() == KubernetesCluster.ClusterType.CloudManaged) { - KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(cluster, this); - destroyWorker = ComponentContext.inject(destroyWorker); - return destroyWorker.destroy(); + return destroyKubernetesCluster(cluster); } else { boolean cleanup = cmd.getCleanup(); boolean expunge = cmd.getExpunge(); @@ -1483,13 +1622,14 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne } } } - return Transaction.execute(new TransactionCallback() { - @Override - public Boolean doInTransaction(TransactionStatus status) { - kubernetesClusterDetailsDao.removeDetails(kubernetesClusterId); - kubernetesClusterVmMapDao.removeByClusterId(kubernetesClusterId); - return kubernetesClusterDao.remove(kubernetesClusterId); + return Transaction.execute((TransactionCallback) status -> { + kubernetesClusterDetailsDao.removeDetails(kubernetesClusterId); + kubernetesClusterVmMapDao.removeByClusterId(kubernetesClusterId); + if (kubernetesClusterDao.remove(kubernetesClusterId)) { + deleteProjectKubernetesAccountIfNeeded(cluster); + return true; } + return false; }); } } @@ -1752,6 +1892,66 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne return responseList; } + protected void deleteProjectKubernetesAccount(Account projectAccount) { + CallContext.register(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM); + try { + Account serviceAccount = getProjectKubernetesAccount(projectAccount, false); + if (serviceAccount != null) { + accountManager.deleteAccount(accountDao.findById(serviceAccount.getId()), User.UID_SYSTEM, + accountService.getSystemAccount()); + } + } finally { + CallContext.unregister(); + } + } + + protected void deleteProjectKubernetesAccountIfNeeded(final KubernetesCluster kubernetesCluster) { + Account owner = accountService.getAccount(kubernetesCluster.getAccountId()); + if (owner == null) { + return; + } + if (Account.Type.PROJECT.equals(owner.getType()) && + kubernetesClusterDao.countNotForGCByAccount(owner.getAccountId()) == 0) { + deleteProjectKubernetesAccount(owner); + } + } + + protected boolean destroyKubernetesCluster(KubernetesCluster kubernetesCluster, boolean deleteProjectAccount) { + KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(kubernetesCluster, + KubernetesClusterManagerImpl.this); + destroyWorker = ComponentContext.inject(destroyWorker); + boolean result = destroyWorker.destroy(); + if (deleteProjectAccount) { + deleteProjectKubernetesAccountIfNeeded(kubernetesCluster); + } + return result; + } + + protected boolean destroyKubernetesCluster(KubernetesCluster kubernetesCluster) { + return destroyKubernetesCluster(kubernetesCluster, true); + } + + @Override + public void cleanupForAccount(Account account) { + List clusters = kubernetesClusterDao.listForCleanupByAccount(account.getId()); + if (CollectionUtils.isEmpty(clusters)) { + return; + } + logger.debug(String.format("Cleaning up %d Kubernetes cluster for %s", clusters.size(), account)); + for (KubernetesClusterVO cluster : clusters) { + try { + destroyKubernetesCluster(cluster, false); + } catch (CloudRuntimeException e) { + logger.warn(String.format("Failed to destroy Kubernetes cluster: %s during cleanup for %s", + cluster.getName(), account), e); + } + } + if (!Account.Type.PROJECT.equals(account.getType())) { + return; + } + deleteProjectKubernetesAccount(account); + } + @Override public List> getCommands() { List> cmdList = new ArrayList>(); @@ -1803,12 +2003,8 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne logger.info("Running Kubernetes cluster garbage collector on Kubernetes cluster: {}", kubernetesCluster); } try { - KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(kubernetesCluster, KubernetesClusterManagerImpl.this); - destroyWorker = ComponentContext.inject(destroyWorker); - if (destroyWorker.destroy()) { - if (logger.isInfoEnabled()) { - logger.info("Garbage collection complete for Kubernetes cluster: {}", kubernetesCluster); - } + if (destroyKubernetesCluster(kubernetesCluster)) { + logger.info("Garbage collection complete for Kubernetes cluster: {}", kubernetesCluster); } else { logger.warn("Garbage collection failed for Kubernetes cluster : {}, it will be attempted to garbage collected in next run", kubernetesCluster); } @@ -1931,9 +2127,7 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne logger.info("Running Kubernetes cluster state scanner on Kubernetes cluster: {} for state: {}", kubernetesCluster, KubernetesCluster.State.Destroying.toString()); } try { - KubernetesClusterDestroyWorker destroyWorker = new KubernetesClusterDestroyWorker(kubernetesCluster, KubernetesClusterManagerImpl.this); - destroyWorker = ComponentContext.inject(destroyWorker); - destroyWorker.destroy(); + destroyKubernetesCluster(kubernetesCluster); } catch (Exception e) { logger.warn("Failed to run Kubernetes cluster Destroying state scanner on Kubernetes cluster : {} status scanner", kubernetesCluster, e); } @@ -1947,7 +2141,7 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne } // checks if Kubernetes cluster is in desired state - boolean isClusterVMsInDesiredState(KubernetesCluster kubernetesCluster, VirtualMachine.State state) { + private boolean isClusterVMsInDesiredState(KubernetesCluster kubernetesCluster, VirtualMachine.State state) { List clusterVMs = kubernetesClusterVmMapDao.listByClusterId(kubernetesCluster.getId()); // check cluster is running at desired capacity include control nodes as well @@ -1985,6 +2179,8 @@ public class KubernetesClusterManagerImpl extends ManagerBase implements Kuberne createNetworkOfferingForKubernetes(DEFAULT_NSX_VPC_TIER_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_NAME, DEFAULT_NSX_VPC_NETWORK_OFFERING_FOR_KUBERNETES_SERVICE_DISPLAY_TEXT , true, true); + getProjectKubernetesAccountRole(); + _gcExecutor.scheduleWithFixedDelay(new KubernetesClusterGarbageCollector(), 300, 300, TimeUnit.SECONDS); _stateScanner.scheduleWithFixedDelay(new KubernetesClusterStatusScanner(), 300, 30, TimeUnit.SECONDS); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java index 9d86c564de4..0c2338465de 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterService.java @@ -16,6 +16,8 @@ // under the License. package com.cloud.kubernetes.cluster; +import java.util.List; + import org.apache.cloudstack.api.command.user.kubernetes.cluster.AddVirtualMachinesToKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.CreateKubernetesClusterCmd; import org.apache.cloudstack.api.command.user.kubernetes.cluster.DeleteKubernetesClusterCmd; @@ -34,16 +36,16 @@ import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; import com.cloud.network.Network; +import com.cloud.user.Account; import com.cloud.utils.component.PluggableService; import com.cloud.utils.exception.CloudRuntimeException; -import java.util.List; - public interface KubernetesClusterService extends PluggableService, Configurable { static final String MIN_KUBERNETES_VERSION_HA_SUPPORT = "1.16.0"; static final int MIN_KUBERNETES_CLUSTER_NODE_CPU = 2; static final int MIN_KUBERNETES_CLUSTER_NODE_RAM_SIZE = 2048; static final String KUBEADMIN_ACCOUNT_NAME = "kubeadmin"; + String PROJECT_KUBEADMIN_ACCOUNT_ROLE_NAME = "Project Kubernetes Service Role"; static final ConfigKey KubernetesServiceEnabled = new ConfigKey("Advanced", Boolean.class, "cloud.kubernetes.service.enabled", @@ -125,4 +127,6 @@ public interface KubernetesClusterService extends PluggableService, Configurable List removeVmsFromCluster(RemoveVirtualMachinesFromKubernetesClusterCmd cmd); boolean isDirectAccess(Network network); + + void cleanupForAccount(Account account); } diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java index bf49c2abb8d..d7e6f65ca05 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesServiceHelperImpl.java @@ -32,6 +32,7 @@ import com.cloud.kubernetes.cluster.dao.KubernetesClusterDao; import com.cloud.kubernetes.cluster.dao.KubernetesClusterVmMapDao; import com.cloud.kubernetes.version.KubernetesSupportedVersion; import com.cloud.kubernetes.version.KubernetesVersionEventTypes; +import com.cloud.user.Account; import com.cloud.uservm.UserVm; import com.cloud.utils.component.AdapterBase; import com.cloud.utils.exception.CloudRuntimeException; @@ -50,6 +51,8 @@ public class KubernetesServiceHelperImpl extends AdapterBase implements Kubernet private KubernetesClusterDao kubernetesClusterDao; @Inject private KubernetesClusterVmMapDao kubernetesClusterVmMapDao; + @Inject + KubernetesClusterService kubernetesClusterService; protected void setEventTypeEntityDetails(Class eventTypeDefinedClass, Class entityClass) { Field[] declaredFields = eventTypeDefinedClass.getDeclaredFields(); @@ -106,6 +109,11 @@ public class KubernetesServiceHelperImpl extends AdapterBase implements Kubernet throw new CloudRuntimeException(msg); } + @Override + public void cleanupForAccount(Account account) { + kubernetesClusterService.cleanupForAccount(account); + } + @Override public String getConfigComponentName() { return KubernetesServiceHelper.class.getSimpleName(); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java index 9341912012f..7df6a6b1dce 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDao.java @@ -25,8 +25,8 @@ import com.cloud.utils.fsm.StateDao; public interface KubernetesClusterDao extends GenericDao, StateDao { - - List listByAccount(long accountId); + List listForCleanupByAccount(long accountId); + int countNotForGCByAccount(long accountId); List findKubernetesClustersToGarbageCollect(); List findManagedKubernetesClustersInState(KubernetesCluster.State state); List listByNetworkId(long networkId); diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java index 63cca3563f7..7bec98d5d25 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/dao/KubernetesClusterDaoImpl.java @@ -30,16 +30,25 @@ import com.cloud.utils.db.TransactionLegacy; @Component public class KubernetesClusterDaoImpl extends GenericDaoBase implements KubernetesClusterDao { - private final SearchBuilder AccountIdSearch; + private final SearchBuilder CleanupAccountIdSearch; + private final SearchBuilder NotForGCByAccountIDCount; private final SearchBuilder GarbageCollectedSearch; private final SearchBuilder ManagedStateSearch; private final SearchBuilder SameNetworkSearch; private final SearchBuilder KubernetesVersionSearch; public KubernetesClusterDaoImpl() { - AccountIdSearch = createSearchBuilder(); - AccountIdSearch.and("account", AccountIdSearch.entity().getAccountId(), SearchCriteria.Op.EQ); - AccountIdSearch.done(); + CleanupAccountIdSearch = createSearchBuilder(); + CleanupAccountIdSearch.and("account", CleanupAccountIdSearch.entity().getAccountId(), SearchCriteria.Op.EQ); + CleanupAccountIdSearch.and("cluster_type", CleanupAccountIdSearch.entity().getClusterType(), SearchCriteria.Op.EQ); + CleanupAccountIdSearch.done(); + + NotForGCByAccountIDCount = createSearchBuilder(); + NotForGCByAccountIDCount.and("gc", NotForGCByAccountIDCount.entity().isCheckForGc(), SearchCriteria.Op.EQ); + NotForGCByAccountIDCount.and("account", NotForGCByAccountIDCount.entity().getAccountId(), SearchCriteria.Op.EQ); + NotForGCByAccountIDCount.and("cluster_type", NotForGCByAccountIDCount.entity().getClusterType(), SearchCriteria.Op.EQ); + NotForGCByAccountIDCount.select(null, SearchCriteria.Func.COUNT, null); + NotForGCByAccountIDCount.done(); GarbageCollectedSearch = createSearchBuilder(); GarbageCollectedSearch.and("gc", GarbageCollectedSearch.entity().isCheckForGc(), SearchCriteria.Op.EQ); @@ -62,10 +71,20 @@ public class KubernetesClusterDaoImpl extends GenericDaoBase listByAccount(long accountId) { - SearchCriteria sc = AccountIdSearch.create(); + public List listForCleanupByAccount(long accountId) { + SearchCriteria sc = CleanupAccountIdSearch.create(); + sc.setParameters("cluster_type", KubernetesCluster.ClusterType.CloudManaged); sc.setParameters("account", accountId); - return listBy(sc, null); + return listBy(sc); + } + + @Override + public int countNotForGCByAccount(long accountId) { + SearchCriteria sc = NotForGCByAccountIDCount.create(); + sc.setParameters("cluster_type", KubernetesCluster.ClusterType.CloudManaged); + sc.setParameters("account", accountId); + sc.setParameters("gc", false); + return getCount(sc); } @Override diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index db2b5f32a7f..177f821d178 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -120,6 +120,7 @@ import com.cloud.exception.OperationTimedoutException; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceUnavailableException; import com.cloud.host.dao.HostDao; +import com.cloud.kubernetes.cluster.KubernetesServiceHelper; import com.cloud.network.IpAddress; import com.cloud.network.IpAddressManager; import com.cloud.network.Network; @@ -887,6 +888,16 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return cleanupAccount(account, callerUserId, caller); } + protected void cleanupPluginsResourcesIfNeeded(Account account) { + try { + KubernetesServiceHelper kubernetesServiceHelper = + ComponentContext.getDelegateComponentOfType(KubernetesServiceHelper.class); + kubernetesServiceHelper.cleanupForAccount(account); + } catch (NoSuchBeanDefinitionException ignored) { + logger.debug("No KubernetesServiceHelper bean found"); + } + } + protected boolean cleanupAccount(AccountVO account, long callerUserId, Account caller) { long accountId = account.getId(); boolean accountCleanupNeeded = false; @@ -968,6 +979,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + cleanupPluginsResourcesIfNeeded(account); + // Destroy the account's VMs List vms = _userVmDao.listByAccountId(accountId); if (logger.isDebugEnabled()) { From d5fbd07b9fb69aaea4b6d6c6068fde1e7eff0fb7 Mon Sep 17 00:00:00 2001 From: Nicolas Vazquez Date: Mon, 19 May 2025 12:44:40 +0530 Subject: [PATCH 14/14] Adding privilege checks on user and account operations Co-authored-by: Harikrishna --- api/src/main/java/com/cloud/user/Account.java | 1 + .../management/MockAccountManager.java | 4 + .../ConfigurationManagerImpl.java | 37 +++++ .../ResourceLimitManagerImpl.java | 6 + .../java/com/cloud/user/AccountManager.java | 1 + .../com/cloud/user/AccountManagerImpl.java | 126 +++++++++++++++++- .../ConfigurationManagerImplTest.java | 46 +++++++ .../cloud/user/AccountManagerImplTest.java | 104 ++++++++++++++- ...countManagerImplVolumeDeleteEventTest.java | 1 + .../user/AccountManagetImplTestBase.java | 3 + .../cloud/user/MockAccountManagerImpl.java | 4 + 11 files changed, 324 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/com/cloud/user/Account.java b/api/src/main/java/com/cloud/user/Account.java index 6be4d0a48f6..1cc5eae64a6 100644 --- a/api/src/main/java/com/cloud/user/Account.java +++ b/api/src/main/java/com/cloud/user/Account.java @@ -71,6 +71,7 @@ public interface Account extends ControlledEntity, InternalIdentity, Identity { } public static final long ACCOUNT_ID_SYSTEM = 1; + public static final long ACCOUNT_ID_ADMIN = 2; public String getAccountName(); diff --git a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index 12741e0996a..d30d7b2f74d 100644 --- a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java @@ -533,4 +533,8 @@ public class MockAccountManager extends ManagerBase implements AccountManager { public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) { return null; } + + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + } } diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 85225fca6d7..c8ab6b4b069 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -50,6 +50,8 @@ import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; +import com.cloud.user.AccountManagerImpl; +import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.affinity.AffinityGroup; import org.apache.cloudstack.affinity.AffinityGroupService; @@ -481,6 +483,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati private long _defaultPageSize = Long.parseLong(Config.DefaultPageSize.getDefaultValue()); private static final String DOMAIN_NAME_PATTERN = "^((?!-)[A-Za-z0-9-]{1,63}(? configValuesForValidation = new HashSet<>(); + private Set configKeysAllowedOnlyForDefaultAdmin = new HashSet<>(); private Set weightBasedParametersForValidation = new HashSet<>(); private Set overprovisioningFactorsForValidation = new HashSet<>(); @@ -545,6 +548,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati populateConfigValuesForValidationSet(); weightBasedParametersForValidation(); overProvisioningFactorsForValidation(); + populateConfigKeysAllowedOnlyForDefaultAdmin(); initMessageBusListener(); return true; } @@ -609,6 +613,11 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati overprovisioningFactorsForValidation.add(CapacityManager.StorageOverprovisioningFactor.key()); } + protected void populateConfigKeysAllowedOnlyForDefaultAdmin() { + configKeysAllowedOnlyForDefaultAdmin.add(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key()); + configKeysAllowedOnlyForDefaultAdmin.add(AccountManagerImpl.allowOperationsOnUsersInSameAccount.key()); + } + private void initMessageBusListener() { messageBus.subscribe(EventTypes.EVENT_CONFIGURATION_VALUE_EDIT, new MessageSubscriber() { @Override @@ -1221,6 +1230,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati logger.error("Missing configuration variable " + name + " in configuration table"); return "Invalid configuration variable."; } + validateConfigurationAllowedOnlyForDefaultAdmin(name, value); String configScope = cfg.getScope(); if (scope != null) { @@ -1255,6 +1265,33 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati return validateValueRange(name, value, type, configuration); } + protected void validateConfigurationAllowedOnlyForDefaultAdmin(String configName, String value) { + if (configKeysAllowedOnlyForDefaultAdmin.contains(configName)) { + final Long userId = CallContext.current().getCallingUserId(); + if (userId != User.UID_ADMIN) { + throw new CloudRuntimeException("Only default admin is allowed to change this setting"); + } + + if (AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key().equals(configName)) { + if (value != null && !value.isBlank()) { + List validRoleTypes = Arrays.stream(RoleType.values()) + .map(Enum::name) + .collect(Collectors.toList()); + + boolean allValid = Arrays.stream(value.split(",")) + .map(String::trim) + .allMatch(validRoleTypes::contains); + + if (!allValid) { + throw new CloudRuntimeException("Invalid role types provided in value"); + } + } else { + throw new CloudRuntimeException("Value for role types must not be empty"); + } + } + } + } + /** * Returns whether a value is valid for a configuration of the provided type. * Valid configuration values are: diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index b8c6e29c278..4a2702eb76d 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -939,6 +939,7 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim } else { _accountMgr.checkAccess(caller, null, true, account); } + _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account); ownerType = ResourceOwnerType.Account; ownerId = accountId; @@ -1090,6 +1091,11 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim throw new InvalidParameterValueException("Please specify a valid domain ID."); } _accountMgr.checkAccess(callerAccount, domain); + Account account = _entityMgr.findById(Account.class, accountId); + if (account == null) { + throw new InvalidParameterValueException("Unable to find account " + accountId); + } + _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account); if (resourceType != null) { resourceTypes.add(resourceType); diff --git a/server/src/main/java/com/cloud/user/AccountManager.java b/server/src/main/java/com/cloud/user/AccountManager.java index 640eaa00076..c46ac78526b 100644 --- a/server/src/main/java/com/cloud/user/AccountManager.java +++ b/server/src/main/java/com/cloud/user/AccountManager.java @@ -206,4 +206,5 @@ public interface AccountManager extends AccountService, Configurable { UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user); + void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount); } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 177f821d178..a0cd5113812 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -20,6 +20,7 @@ import java.net.InetAddress; import java.net.URLEncoder; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -398,6 +399,22 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M true, ConfigKey.Scope.Domain); + public static ConfigKey listOfRoleTypesAllowedForOperationsOfSameRoleType = new ConfigKey<>("Hidden", + String.class, + "role.types.allowed.for.operations.on.accounts.of.same.role.type", + "Admin, ResourceAdmin, DomainAdmin", + "Comma separated list of role types that are allowed to do operations on accounts or users of the same role type within a domain.", + true, + ConfigKey.Scope.Domain); + + public static ConfigKey allowOperationsOnUsersInSameAccount = new ConfigKey<>("Hidden", + Boolean.class, + "allow.operations.on.users.in.same.account", + "true", + "Allow operations on users among them in the same account", + true, + ConfigKey.Scope.Domain); + protected AccountManagerImpl() { super(); } @@ -1404,7 +1421,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M /** * if there is any permission under the requested role that is not permitted for the caller, refuse */ - private void checkRoleEscalation(Account caller, Account requested) { + protected void checkRoleEscalation(Account caller, Account requested) { if (logger.isDebugEnabled()) { logger.debug(String.format("Checking if user of account %s [%s] with role-id [%d] can create an account of type %s [%s] with role-id [%d]", caller.getAccountName(), @@ -1510,6 +1527,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M assertUserNotAlreadyInAccount(duplicatedUser, account); } + verifyCallerPrivilegeForUserOrAccountOperations(account); UserVO user; user = createUser(account.getId(), userName, password, firstName, lastName, email, timeZone, userUUID, source); return user; @@ -1526,11 +1544,15 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @ActionEvent(eventType = EventTypes.EVENT_USER_UPDATE, eventDescription = "Updating User") public UserAccount updateUser(UpdateUserCmd updateUserCmd) { UserVO user = retrieveAndValidateUser(updateUserCmd); + Account account = retrieveAndValidateAccount(user); + User caller = CallContext.current().getCallingUser(); + checkAccess(caller, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); + logger.debug("Updating user {}", user); validateAndUpdateApiAndSecretKeyIfNeeded(updateUserCmd, user); validateAndUpdateUserApiKeyAccess(updateUserCmd, user); - Account account = retrieveAndValidateAccount(user); validateAndUpdateFirstNameIfNeeded(updateUserCmd, user); validateAndUpdateLastNameIfNeeded(updateUserCmd, user); @@ -1553,6 +1575,85 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return _userAccountDao.findById(user.getId()); } + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + logger.debug(String.format("Verifying whether the caller has the correct privileges based on the user's role type and API permissions: %s", userAccount)); + + checkCallerRoleTypeAllowedForUserOrAccountOperations(userAccount, null); + checkCallerApiPermissionsForUserOrAccountOperations(userAccount); + } + + protected void verifyCallerPrivilegeForUserOrAccountOperations(User user) { + logger.debug(String.format("Verifying whether the caller has the correct privileges based on the user's role type and API permissions: %s", user)); + + Account userAccount = getAccount(user.getAccountId()); + checkCallerRoleTypeAllowedForUserOrAccountOperations(userAccount, user); + checkCallerApiPermissionsForUserOrAccountOperations(userAccount); + } + + protected void checkCallerRoleTypeAllowedForUserOrAccountOperations(Account userAccount, User user) { + Account callingAccount = getCurrentCallingAccount(); + RoleType callerRoleType = getRoleType(callingAccount); + RoleType userAccountRoleType = getRoleType(userAccount); + + if (RoleType.Unknown == callerRoleType || RoleType.Unknown == userAccountRoleType) { + String errMsg = String.format("The role type of account [%s, %s] or [%s, %s] is unknown", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + throw new PermissionDeniedException(errMsg); + } + + boolean isCallerSystemOrDefaultAdmin = callingAccount.getId() == Account.ACCOUNT_ID_SYSTEM || callingAccount.getId() == Account.ACCOUNT_ID_ADMIN; + if (isCallerSystemOrDefaultAdmin) { + logger.trace(String.format("Admin account [%s, %s] performing this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (callerRoleType.getId() < userAccountRoleType.getId()) { + logger.trace(String.format("The calling account [%s, %s] has a higher role type than the user account [%s, %s]", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (callerRoleType.getId() == userAccountRoleType.getId()) { + if (callingAccount.getId() != userAccount.getId()) { + String allowedRoleTypes = listOfRoleTypesAllowedForOperationsOfSameRoleType.valueInDomain(callingAccount.getDomainId()); + boolean updateAllowed = allowedRoleTypes != null && + Arrays.stream(allowedRoleTypes.split(",")) + .map(String::trim) + .anyMatch(role -> role.equals(callerRoleType.toString())); + if (BooleanUtils.isFalse(updateAllowed)) { + String errMsg = String.format("The calling account [%s, %s] is not allowed to perform this operation on users from other accounts " + + "of the same role type within the domain", callingAccount.getName(), callingAccount.getUuid()); + logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } + } else if ((callingAccount.getId() == userAccount.getId()) && user != null) { + Boolean allowOperationOnUsersinSameAccount = allowOperationsOnUsersInSameAccount.valueInDomain(callingAccount.getDomainId()); + User callingUser = CallContext.current().getCallingUser(); + if (callingUser.getId() != user.getId() && BooleanUtils.isFalse(allowOperationOnUsersinSameAccount)) { + String errMsg = "The user operations are not allowed by the users in the same account"; + logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } + } + } else { + String errMsg = String.format("The calling account [%s, %s] has a lower role type than the user account [%s, %s]", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + throw new PermissionDeniedException(errMsg); + } + } + + protected void checkCallerApiPermissionsForUserOrAccountOperations(Account userAccount) { + Account callingAccount = getCurrentCallingAccount(); + boolean isCallerRootAdmin = callingAccount.getId() == Account.ACCOUNT_ID_SYSTEM || isRootAdmin(callingAccount.getId()); + + if (isCallerRootAdmin) { + logger.trace(String.format("Admin account [%s, %s] performing this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (isRootAdmin(userAccount.getAccountId())) { + String errMsg = String.format("Account [%s, %s] cannot perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } else { + logger.debug(String.format("Checking calling account [%s, %s] permission to perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + checkRoleEscalation(callingAccount, userAccount); + logger.debug(String.format("Calling account [%s, %s] is allowed to perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } + } + /** * Updates the password in the user POJO if needed. If no password is provided, then the password is not updated. * The following validations are executed if 'password' is not null. Admins (root admins or domain admins) can execute password updates without entering the current password. @@ -1832,6 +1933,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); boolean success = doSetUserStatus(userId, State.DISABLED); if (success) { @@ -1873,6 +1975,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); boolean success = Transaction.execute(new TransactionCallback<>() { @Override @@ -1925,6 +2028,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); // make sure the account is enabled too // if the user is either locked already or disabled already, don't change state...only lock currently enabled @@ -1988,6 +2092,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkIfAccountManagesProjects(accountId); + verifyCallerPrivilegeForUserOrAccountOperations(account); CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2050,6 +2155,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check if user performing the action is allowed to modify this account Account caller = getCurrentCallingAccount(); checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); boolean success = enableAccount(account.getId()); if (success) { @@ -2083,6 +2189,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); if (lockAccount(account.getId())) { CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2113,6 +2220,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); if (disableAccount(account.getId())) { CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2158,6 +2266,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check if user performing the action is allowed to modify this account Account caller = getCurrentCallingAccount(); checkAccess(caller, _domainMgr.getDomain(account.getDomainId())); + verifyCallerPrivilegeForUserOrAccountOperations(account); validateAndUpdateAccountApiKeyAccess(cmd, acctForUpdate); @@ -2249,6 +2358,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // don't allow to delete the user from the account of type Project checkAccountAndAccess(user, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); return _userDao.remove(id); } @@ -2259,6 +2369,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M UserVO user = getValidUserVO(id); Account oldAccount = _accountDao.findById(user.getAccountId()); checkAccountAndAccess(user, oldAccount); + verifyCallerPrivilegeForUserOrAccountOperations(user); long domainId = oldAccount.getDomainId(); long newAccountId = getNewAccountId(domainId, cmd.getAccountName(), cmd.getAccountId()); @@ -2596,6 +2707,11 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + if (!Account.Type.PROJECT.equals(accountType)) { + AccountVO newAccount = new AccountVO(accountName, domainId, networkDomain, accountType, roleId, uuid); + verifyCallerPrivilegeForUserOrAccountOperations(newAccount); + } + // Create the account return Transaction.execute(new TransactionCallback<>() { @Override @@ -2939,8 +3055,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } final Account account = getAccount(getUserAccountById(userId).getAccountId()); //Extracting the Account from the userID of the requested user. User caller = CallContext.current().getCallingUser(); - preventRootDomainAdminAccessToRootAdminKeys(caller, account); checkAccess(caller, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); Map keys = new HashMap<>(); keys.put("apikey", user.getApiKey()); @@ -3004,8 +3120,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } Account account = _accountDao.findById(user.getAccountId()); - preventRootDomainAdminAccessToRootAdminKeys(user, account); checkAccess(caller, null, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); // don't allow updating system user if (user.getId() == User.UID_SYSTEM) { @@ -3461,7 +3577,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M public ConfigKey[] getConfigKeys() { return new ConfigKey[] {UseSecretKeyInResponse, enableUserTwoFactorAuthentication, userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication, userTwoFactorAuthenticationIssuer, apiKeyAccess, - userAllowMultipleAccounts}; + userAllowMultipleAccounts, listOfRoleTypesAllowedForOperationsOfSameRoleType, allowOperationsOnUsersInSameAccount}; } public List getUserTwoFactorAuthenticationProviders() { diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java index 0a045296821..1309842b706 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java +++ b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java @@ -45,15 +45,18 @@ import com.cloud.storage.StorageManager; import com.cloud.storage.dao.VMTemplateZoneDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.user.Account; +import com.cloud.user.AccountManagerImpl; import com.cloud.user.User; import com.cloud.utils.db.EntityManager; import com.cloud.utils.db.SearchCriteria; +import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.command.admin.network.CreateNetworkOfferingCmd; import org.apache.cloudstack.api.command.admin.offering.UpdateDiskOfferingCmd; import org.apache.cloudstack.api.command.admin.zone.DeleteZoneCmd; +import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.subsystem.api.storage.ZoneScope; import org.apache.cloudstack.framework.config.ConfigDepot; import org.apache.cloudstack.framework.config.ConfigKey; @@ -182,6 +185,7 @@ public class ConfigurationManagerImplTest { configurationManagerImplSpy.populateConfigValuesForValidationSet(); configurationManagerImplSpy.weightBasedParametersForValidation(); configurationManagerImplSpy.overProvisioningFactorsForValidation(); + configurationManagerImplSpy.populateConfigKeysAllowedOnlyForDefaultAdmin(); ReflectionTestUtils.setField(configurationManagerImplSpy, "templateZoneDao", vmTemplateZoneDao); ReflectionTestUtils.setField(configurationManagerImplSpy, "annotationDao", annotationDao); @@ -851,4 +855,46 @@ public class ConfigurationManagerImplTest { boolean result = configurationManagerImplSpy.shouldValidateConfigRange(Config.ConsoleProxySessionMax.name(), "test", Config.ConsoleProxyUrlDomain); Assert.assertTrue(result); } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withAdminUser_shouldNotThrowException() { + CallContext callContext = mock(CallContext.class); + when(callContext.getCallingUserId()).thenReturn(User.UID_ADMIN); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key(), "Admin"); + } + } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withNonAdminUser_shouldThrowException() { + CallContext callContext = mock(CallContext.class); + when(callContext.getCallingUserId()).thenReturn(123L); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + Assert.assertThrows(CloudRuntimeException.class, () -> + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.allowOperationsOnUsersInSameAccount.key(), "Admin") + ); + } + } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withNonRestrictedKey_shouldNotThrowException() { + CallContext callContext = mock(CallContext.class); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin("some.other.config.key", "Admin"); + } + } + + @Test(expected = CloudRuntimeException.class) + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withValidConfigNameAndInvalidValue_shouldThrowException() { + CallContext callContext = mock(CallContext.class); + try (MockedStatic mockedCallContext = Mockito.mockStatic(CallContext.class)) { + mockedCallContext.when(CallContext::current).thenReturn(callContext); + when(callContext.getCallingUserId()).thenReturn(User.UID_ADMIN); + String invalidValue = "Admin, SuperUser"; + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key(), invalidValue); + } + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index eb401efa341..09a2d73080d 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -16,6 +16,7 @@ // under the License. package com.cloud.user; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.nullable; import java.net.InetAddress; @@ -124,6 +125,9 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Mock ConfigKey enableUserTwoFactorAuthenticationMock; + + @Mock + ConfigKey allowOperationsOnUsersInSameAccountMock; @Mock RoleService roleService; @@ -131,6 +135,9 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { public void setUp() throws Exception { enableUserTwoFactorAuthenticationMock = Mockito.mock(ConfigKey.class); accountManagerImpl.enableUserTwoFactorAuthentication = enableUserTwoFactorAuthenticationMock; + + allowOperationsOnUsersInSameAccountMock = Mockito.mock(ConfigKey.class); + accountManagerImpl.allowOperationsOnUsersInSameAccount = allowOperationsOnUsersInSameAccountMock; } @Before @@ -141,6 +148,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(accountMockId).when(userVoMock).getAccountId(); Mockito.doReturn(userVoIdMock).when(userVoMock).getId(); + + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(accountMock, accountMock); } @Test @@ -191,6 +200,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.when(_sshKeyPairDao.remove(Mockito.anyLong())).thenReturn(true); Mockito.when(userDataDao.removeByAccountId(Mockito.anyLong())).thenReturn(222); Mockito.doNothing().when(accountManagerImpl).deleteWebhooksForAccount(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); Assert.assertTrue(accountManagerImpl.deleteUserAccount(42l)); // assert that this was a clean delete @@ -211,6 +221,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.lenient().when(_domainMgr.getDomain(Mockito.anyLong())).thenReturn(domain); Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), Mockito.any(Domain.class))).thenReturn(true); Mockito.doNothing().when(accountManagerImpl).deleteWebhooksForAccount(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); Assert.assertTrue(accountManagerImpl.deleteUserAccount(42l)); // assert that this was NOT a clean delete @@ -245,6 +256,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(2L).when(accountVoMock).getId(); Mockito.doReturn(true).when(accountManagerImpl).isDeleteNeeded(Mockito.any(), Mockito.anyLong(), Mockito.any()); Mockito.doReturn(new ArrayList()).when(_projectAccountDao).listAdministratedProjectIds(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); accountManagerImpl.deleteUserAccount(accountId); } @@ -282,6 +294,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(2L).when(userVoMock).getId(); Mockito.doNothing().when(accountManagerImpl).checkAccountAndAccess(Mockito.any(), Mockito.any()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations(userVoMock); accountManagerImpl.deleteUser(cmd); } } @@ -348,7 +361,6 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { // Queried account - admin account AccountVO adminAccountMock = Mockito.mock(AccountVO.class); - Mockito.when(adminAccountMock.getAccountId()).thenReturn(2L); Mockito.when(_accountDao.findByIdIncludingRemoved(2L)).thenReturn(adminAccountMock); Mockito.lenient().when(accountService.isRootAdmin(2L)).thenReturn(true); Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), @@ -396,6 +408,12 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Test public void updateUserTestTimeZoneAndEmailNull() { + Mockito.when(userVoMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(accountMock).when(accountManagerImpl).getAccount(10L); + Mockito.when(accountMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(10L); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.User); + prepareMockAndExecuteUpdateUserTest(0); } @@ -403,6 +421,11 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { public void updateUserTestTimeZoneAndEmailNotNull() { Mockito.when(UpdateUserCmdMock.getEmail()).thenReturn("email"); Mockito.when(UpdateUserCmdMock.getTimezone()).thenReturn("timezone"); + Mockito.when(userVoMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(accountMock).when(accountManagerImpl).getAccount(10L); + Mockito.when(accountMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(10L); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.User); prepareMockAndExecuteUpdateUserTest(1); } @@ -420,14 +443,17 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(true).when(userDaoMock).update(Mockito.anyLong(), Mockito.eq(userVoMock)); Mockito.doReturn(Mockito.mock(UserAccountVO.class)).when(userAccountDaoMock).findById(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).checkAccess(nullable(User.class), nullable(Account.class)); accountManagerImpl.updateUser(UpdateUserCmdMock); + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(accountMock, accountMock); + InOrder inOrder = Mockito.inOrder(userVoMock, accountManagerImpl, userDaoMock, userAccountDaoMock); inOrder.verify(accountManagerImpl).retrieveAndValidateUser(UpdateUserCmdMock); - inOrder.verify(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).retrieveAndValidateAccount(userVoMock); + inOrder.verify(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).validateAndUpdateFirstNameIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).validateAndUpdateLastNameIfNeeded(UpdateUserCmdMock, userVoMock); @@ -1339,8 +1365,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { accountManagerImpl.validateRoleChange(account, newRole, caller); } - @Test - public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNotAProjectAdministrator() { + @Test + public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNotAProjectAdministrator() { long accountId = 1L; List managedProjectIds = new ArrayList<>(); @@ -1477,4 +1503,74 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount); } + + @Test + public void testCheckCallerRoleTypeAllowedToUpdateUserSameAccount() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(accountMock); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.DomainAdmin); + + accountManagerImpl.checkCallerRoleTypeAllowedForUserOrAccountOperations(accountMock, userVoMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckCallerRoleTypeAllowedToUpdateUserLowerAccountRoleType() { + Account callingAccount = Mockito.mock(Account.class); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(2L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(2L); + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(callingAccount))).thenReturn(RoleType.DomainAdmin); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.Admin); + accountManagerImpl.checkCallerRoleTypeAllowedForUserOrAccountOperations(accountMock, userVoMock); + } + + @Test + public void testcheckCallerApiPermissionsForUserOperationsRootAdminSameCaller() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(accountMock); + Mockito.when(accountMock.getId()).thenReturn(2L); + Mockito.doReturn(true).when(accountManagerImpl).isRootAdmin(2L); + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testcheckCallerApiPermissionsForUserOperationsRootAdminDifferentAccount() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(true).when(accountManagerImpl).isRootAdmin(2L); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test + public void testcheckCallerApiPermissionsForUserOperationsAllowedApis() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(2L); + + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(callingAccount, accountMock); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testcheckCallerApiPermissionsForUserOperationsNotAllowedApis() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(2L); + + Mockito.lenient().doThrow(PermissionDeniedException.class).when(accountManagerImpl).checkRoleEscalation(callingAccount, accountMock); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java index 4004321b8e9..6d69890c9a5 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java @@ -192,6 +192,7 @@ public class AccountManagerImplVolumeDeleteEventTest extends AccountManagetImplT public void destroyedVMRootVolumeUsageEvent() throws SecurityException, IllegalArgumentException, ReflectiveOperationException, AgentUnavailableException, ConcurrentOperationException, CloudException { lenient().doReturn(vm).when(_vmMgr).destroyVm(nullable(Long.class), nullable(Boolean.class)); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); List emittedEvents = deleteUserAccountRootVolumeUsageEvents(true); Assert.assertEquals(0, emittedEvents.size()); } diff --git a/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java b/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java index 1b75638b125..98f152088ed 100644 --- a/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java +++ b/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java @@ -210,6 +210,9 @@ public class AccountManagetImplTestBase { @Mock RoutedIpv4Manager routedIpv4Manager; + @Mock + Account accountMock; + @Before public void setup() { accountManagerImpl.setUserAuthenticators(Arrays.asList(userAuthenticator)); diff --git a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java index 62a95380a15..8c569fb3ec8 100644 --- a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java +++ b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java @@ -503,4 +503,8 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) { return null; } + + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + } }