From e14c2ec7241ec2333ea3ac3d56a0e938fe1bf131 Mon Sep 17 00:00:00 2001 From: Prachi Damle Date: Tue, 29 Apr 2014 18:32:55 -0700 Subject: [PATCH] CLOUDSTACK-6517: IAM - Admin is allowed to create PortFowarding rule for a regular user, when admin does not have " UseEntry" permission for IpAddress. Changes: - IAM was applying ordering on accessTypes. Thus if an account had Operate, he got USe access as well. So even if IAM schema did not have 'UseEntry" permission for IpAddress, some other 'OperateEntry' permission on IpAddress was letting this operation go through. - Fixed IAM to NOT do ordering of access types anymore. IAm will perform strict accessType check only. - This fix is needed so that admin does not get permission to USE resources from other account just becase he has OPERATE access on those resources due to some other APIs. - However due to this fix, we break backwards compatibilty with CS 4.3. - CS 4.3 allowed root admin to do the createPF operation for a user by passing in networkId of the user. - Same was the case for domain admins within their domains - Why this worked was due to CS 4.3 simply returning true for root admin/domain admin - So to maintain backwards compatibilty, we are adding the logic to return "true" for root admin and domain admin just like CS 4.3. - Exception is: For Network, AffinityGroup and Templates, we still call IAM even for root admin/domain admin, since thats what CS 4.3 did. Just for these 3 resource_types, it used to perform access checks even for root admin/domain admin. --- .../ConfigurationManagerImpl.java | 3 +- .../com/cloud/user/AccountManagerImpl.java | 90 +++++++++++++++++-- .../iam/RoleBasedAPIAccessChecker.java | 8 +- .../iam/RoleBasedEntityAccessChecker.java | 16 +--- .../iam/RoleBasedEntityQuerySelector.java | 35 +++----- .../apache/cloudstack/iam/api/IAMService.java | 4 +- .../cloudstack/iam/server/IAMServiceImpl.java | 12 +-- .../server/dao/IAMPolicyPermissionDao.java | 2 +- .../dao/IAMPolicyPermissionDaoImpl.java | 7 +- 9 files changed, 121 insertions(+), 56 deletions(-) diff --git a/server/src/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/com/cloud/configuration/ConfigurationManagerImpl.java index a3d31a6c093..6f3a0e27fa1 100755 --- a/server/src/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/com/cloud/configuration/ConfigurationManagerImpl.java @@ -39,6 +39,7 @@ import javax.naming.ConfigurationException; import org.apache.log4j.Logger; import org.apache.cloudstack.acl.SecurityChecker; +import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.affinity.AffinityGroup; import org.apache.cloudstack.affinity.AffinityGroupService; import org.apache.cloudstack.affinity.dao.AffinityGroupDao; @@ -4326,7 +4327,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati throw new InvalidParameterValueException("Can't update system networks"); } - _accountMgr.checkAccess(caller, null, network); + _accountMgr.checkAccess(caller, AccessType.ListEntry, network); List offeringIds = _networkModel.listNetworkOfferingsForUpgrade(networkId); diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index 3ab26ccd52a..b5fdc3a9163 100755 --- a/server/src/com/cloud/user/AccountManagerImpl.java +++ b/server/src/com/cloud/user/AccountManagerImpl.java @@ -46,6 +46,7 @@ import org.apache.cloudstack.acl.QuerySelector; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.acl.SecurityChecker.AccessType; +import org.apache.cloudstack.affinity.AffinityGroup; import org.apache.cloudstack.affinity.dao.AffinityGroupDao; import org.apache.cloudstack.api.InternalIdentity; import org.apache.cloudstack.api.command.admin.account.UpdateAccountCmd; @@ -89,6 +90,7 @@ import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceUnavailableException; import com.cloud.network.IpAddress; import com.cloud.network.IpAddressManager; +import com.cloud.network.Network; import com.cloud.network.VpnUserVO; import com.cloud.network.as.AutoScaleManager; import com.cloud.network.dao.AccountGuestVlanMapDao; @@ -123,6 +125,7 @@ import com.cloud.storage.dao.VMTemplateDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.snapshot.SnapshotManager; import com.cloud.template.TemplateManager; +import com.cloud.template.VirtualMachineTemplate; import com.cloud.user.Account.State; import com.cloud.user.dao.AccountDao; import com.cloud.user.dao.UserAccountDao; @@ -484,13 +487,67 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } entityBuf.append("}"); String entityStr = entityBuf.toString(); - for (SecurityChecker checker : _securityCheckers) { - if (checker.checkAccess(caller, accessType, apiName, entities)) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("Access to " + entityStr + " granted to " + caller + " by " + checker.getName()); + + boolean isRootAdmin = isRootAdmin(caller.getAccountId()); + boolean isDomainAdmin = isDomainAdmin(caller.getAccountId()); + boolean isResourceDomainAdmin = isResourceDomainAdmin(caller.getAccountId()); + + if ((isRootAdmin || isDomainAdmin || isResourceDomainAdmin || caller.getId() == Account.ACCOUNT_ID_SYSTEM) + && (accessType == null || accessType == AccessType.UseEntry)) { + + for (ControlledEntity entity : entities) { + if (entity instanceof VirtualMachineTemplate || entity instanceof Network + || entity instanceof AffinityGroup) { + for (SecurityChecker checker : _securityCheckers) { + if (checker.checkAccess(caller, accessType, apiName, entity)) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Access to " + entityStr + " granted to " + caller + " by " + + checker.getName()); + } + granted = true; + break; + } + } + } else { + if (isRootAdmin || caller.getId() == Account.ACCOUNT_ID_SYSTEM) { + // no need to make permission checks if the system/root + // admin makes the call + if (s_logger.isTraceEnabled()) { + s_logger.trace("No need to make permission check for System/RootAdmin account, returning true"); + } + granted = true; + } else if (isDomainAdmin || isResourceDomainAdmin) { + Domain entityDomain = getEntityDomain(entity); + if (entityDomain != null) { + try { + checkAccess(caller, entityDomain); + granted = true; + } catch (PermissionDeniedException e) { + List entityList = new ArrayList(); + entityList.add(entity); + e.addDetails(caller, entityList); + throw e; + } + } + } + } + + if (!granted) { + assert false : "How can all of the security checkers pass on checking this check: " + entityStr; + throw new PermissionDeniedException("There's no way to confirm " + caller + " has access to " + + entityStr); + } + + } + } else { + for (SecurityChecker checker : _securityCheckers) { + if (checker.checkAccess(caller, accessType, apiName, entities)) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("Access to " + entityStr + " granted to " + caller + " by " + checker.getName()); + } + granted = true; + break; } - granted = true; - break; } } @@ -500,6 +557,27 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + private Domain getEntityDomain(ControlledEntity entity) { + Domain entityDomain = null; + long domainId = entity.getDomainId(); + + if (domainId != -1) { + entityDomain = _domainMgr.getDomain(domainId); + } else { + if (entity.getAccountId() != -1) { + // If account exists domainId should too so + // calculate + // it. This condition might be hit for templates or + // entities which miss domainId in their tables + Account account = getAccount(entity.getAccountId()); + domainId = account != null ? account.getDomainId() : -1; + entityDomain = _domainMgr.getDomain(domainId); + } + } + + return entityDomain; + } + @Override public Long checkAccessAndSpecifyAuthority(Account caller, Long zoneId) { // We just care for resource domain admin for now. He should be permitted to see only his zone. diff --git a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedAPIAccessChecker.java b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedAPIAccessChecker.java index 3a3ba4dd3f2..91593f6f23e 100644 --- a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedAPIAccessChecker.java +++ b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedAPIAccessChecker.java @@ -247,12 +247,8 @@ public class RoleBasedAPIAccessChecker extends AdapterBase implements APIChecker try { cmdObj = (BaseCmd) cmdClass.newInstance(); if (cmdObj instanceof BaseListCmd) { - if (permissionScope == PermissionScope.ACCOUNT) { - accessType = AccessType.UseEntry; - } else { - accessType = AccessType.ListEntry; - addAccountScopedUseEntry = true; - } + accessType = AccessType.ListEntry; + addAccountScopedUseEntry = true; } else { accessType = AccessType.OperateEntry; } diff --git a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityAccessChecker.java b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityAccessChecker.java index cc29ab541bb..63aa82786f0 100644 --- a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityAccessChecker.java +++ b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityAccessChecker.java @@ -107,22 +107,14 @@ public class RoleBasedEntityAccessChecker extends DomainChecker implements Secur permissions = _iamSrv.listPolicyPermissionByActionAndEntity(policy.getId(), action, entityType); if (permissions.isEmpty()) { if (accessType != null) { - for (AccessType type : AccessType.values()) { - if (type.ordinal() >= accessType.ordinal()) { - permissions.addAll(_iamSrv.listPolicyPermissionByAccessAndEntity(policy.getId(), - type.toString(), entityType)); - } - } + permissions.addAll(_iamSrv.listPolicyPermissionByAccessAndEntity(policy.getId(), + accessType.toString(), entityType)); } } } else { if (accessType != null) { - for (AccessType type : AccessType.values()) { - if (type.ordinal() >= accessType.ordinal()) { - permissions.addAll(_iamSrv.listPolicyPermissionByAccessAndEntity(policy.getId(), - type.toString(), entityType)); - } - } + permissions.addAll(_iamSrv.listPolicyPermissionByAccessAndEntity(policy.getId(), + accessType.toString(), entityType)); } } for (IAMPolicyPermission permission : permissions) { diff --git a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java index b7c3d352773..5d31433803f 100644 --- a/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java +++ b/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java @@ -57,11 +57,9 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe List domainIds = new ArrayList(); for (IAMPolicy policy : policies) { List pp = new ArrayList(); - for (AccessType type : AccessType.values()) { - if (type.ordinal() >= accessType.ordinal()) { - pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.DOMAIN.toString(), type)); - } - } + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, + PermissionScope.DOMAIN.toString(), accessType.toString())); + if (pp != null) { for (IAMPolicyPermission p : pp) { if (p.getScopeId() != null) { @@ -101,11 +99,9 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe List accountIds = new ArrayList(); for (IAMPolicy policy : policies) { List pp = new ArrayList(); - for (AccessType type : AccessType.values()) { - if (type.ordinal() >= accessType.ordinal()) { - pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.ACCOUNT.toString(), type)); - } - } + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, + PermissionScope.ACCOUNT.toString(), accessType.toString())); + if (pp != null) { for (IAMPolicyPermission p : pp) { if (p.getScopeId() != null) { @@ -144,11 +140,9 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe List entityIds = new ArrayList(); for (IAMPolicy policy : policies) { List pp = new ArrayList(); - for (AccessType type : AccessType.values()) { - if (type.ordinal() >= accessType.ordinal()) { - pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.RESOURCE.toString(), type)); - } - } + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, + PermissionScope.RESOURCE.toString(), accessType.toString())); + if (pp != null) { for (IAMPolicyPermission p : pp) { if (p.getScopeId() != null) { @@ -171,13 +165,10 @@ public class RoleBasedEntityQuerySelector extends AdapterBase implements QuerySe // for each policy, find granted permission with ALL scope for (IAMPolicy policy : policies) { List pp = new ArrayList(); - for (AccessType type : AccessType.values()) { - if (type.ordinal() >= accessType.ordinal()) { - pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.ALL.toString(), type)); - } - if (pp != null && pp.size() > 0) { - return true; - } + pp.addAll(_iamService.listPolicyPermissionsByScope(policy.getId(), action, PermissionScope.ALL.toString(), + accessType.toString())); + if (pp != null && pp.size() > 0) { + return true; } } return false; diff --git a/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java b/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java index c396fa922fe..20326e974c0 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java @@ -18,7 +18,7 @@ package org.apache.cloudstack.iam.api; import java.util.List; -import org.apache.cloudstack.acl.SecurityChecker.AccessType; + import org.apache.cloudstack.iam.api.IAMPolicyPermission.Permission; import com.cloud.utils.Pair; @@ -73,7 +73,7 @@ public interface IAMService { List listPolicyPermissions(long policyId); - List listPolicyPermissionsByScope(long policyId, String action, String scope, AccessType accessType); + List listPolicyPermissionsByScope(long policyId, String action, String scope, String accessType); List listPolicyPermissionByActionAndEntity(long policyId, String action, String entityType); diff --git a/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java b/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java index d6a61a10457..c35ac1d2734 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java @@ -25,7 +25,6 @@ import javax.inject.Inject; import org.apache.log4j.Logger; import org.apache.cloudstack.acl.PermissionScope; -import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.iam.api.IAMGroup; import org.apache.cloudstack.iam.api.IAMPolicy; import org.apache.cloudstack.iam.api.IAMPolicyPermission; @@ -632,7 +631,8 @@ public class IAMServiceImpl extends ManagerBase implements IAMService, Manager { } // add entry in acl_policy_permission table - IAMPolicyPermissionVO permit = _policyPermissionDao.findByPolicyAndEntity(iamPolicyId, entityType, scope, scopeId, action, perm); + IAMPolicyPermissionVO permit = _policyPermissionDao.findByPolicyAndEntity(iamPolicyId, entityType, scope, + scopeId, action, perm, accessType); if (permit == null) { // not there already permit = new IAMPolicyPermissionVO(iamPolicyId, action, entityType, accessType, scope, scopeId, perm, @@ -654,7 +654,8 @@ public class IAMServiceImpl extends ManagerBase implements IAMService, Manager { + "; failed to revoke permission from policy."); } // remove entry from acl_entity_permission table - IAMPolicyPermissionVO permit = _policyPermissionDao.findByPolicyAndEntity(iamPolicyId, entityType, scope, scopeId, action, Permission.Allow); + IAMPolicyPermissionVO permit = _policyPermissionDao.findByPolicyAndEntity(iamPolicyId, entityType, scope, + scopeId, action, Permission.Allow, null); if (permit != null) { // not removed yet _policyPermissionDao.remove(permit.getId()); @@ -766,9 +767,10 @@ public class IAMServiceImpl extends ManagerBase implements IAMService, Manager { @SuppressWarnings("unchecked") @Override - public List listPolicyPermissionsByScope(long policyId, String action, String scope, AccessType accessType) { + public List listPolicyPermissionsByScope(long policyId, String action, String scope, + String accessType) { @SuppressWarnings("rawtypes") - List pp = _policyPermissionDao.listByPolicyActionAndScope(policyId, action, scope, accessType.toString()); + List pp = _policyPermissionDao.listByPolicyActionAndScope(policyId, action, scope, accessType); return pp; } diff --git a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java index 6e6099c6648..ebb4916470d 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java @@ -27,7 +27,7 @@ public interface IAMPolicyPermissionDao extends GenericDao listByPolicy(long policyId); IAMPolicyPermissionVO findByPolicyAndEntity(long policyId, String entityType, String scope, Long scopeId, - String action, Permission perm); + String action, Permission perm, String accessType); List listByPolicyActionAndScope(long policyId, String action, String scope, String accessType); diff --git a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java index 9cc1af07e8c..44b77d1163e 100644 --- a/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java +++ b/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java @@ -70,7 +70,7 @@ public class IAMPolicyPermissionDaoImpl extends GenericDaoBase sc = fullSearch.create(); sc.setParameters("policyId", policyId); sc.setParameters("entityType", entityType); @@ -78,6 +78,11 @@ public class IAMPolicyPermissionDaoImpl extends GenericDaoBase