Repository: cloudstack Updated Branches: refs/heads/4.4 376078e90 -> c32b7ab7c
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. Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/c32b7ab7 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/c32b7ab7 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/c32b7ab7 Branch: refs/heads/4.4 Commit: c32b7ab7c8e73a3422ff31d754c28c8997a9a84c Parents: 376078e Author: Prachi Damle <pra...@cloud.com> Authored: Tue Apr 29 18:32:55 2014 -0700 Committer: Daan Hoogland <d...@onecht.net> Committed: Thu May 1 08:36:39 2014 +0200 ---------------------------------------------------------------------- .../configuration/ConfigurationManagerImpl.java | 3 +- .../src/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 +-- .../iam/server/dao/IAMPolicyPermissionDao.java | 2 +- .../server/dao/IAMPolicyPermissionDaoImpl.java | 7 +- 9 files changed, 121 insertions(+), 56 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/server/src/com/cloud/configuration/ConfigurationManagerImpl.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/com/cloud/configuration/ConfigurationManagerImpl.java index a3d31a6..6f3a0e2 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<Long> offeringIds = _networkModel.listNetworkOfferingsForUpgrade(networkId); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/server/src/com/cloud/user/AccountManagerImpl.java ---------------------------------------------------------------------- diff --git a/server/src/com/cloud/user/AccountManagerImpl.java b/server/src/com/cloud/user/AccountManagerImpl.java index 3ab26cc..b5fdc3a 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<ControlledEntity> entityList = new ArrayList<ControlledEntity>(); + 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. http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedAPIAccessChecker.java ---------------------------------------------------------------------- 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 3a3ba4d..91593f6 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; } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityAccessChecker.java ---------------------------------------------------------------------- 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 cc29ab5..63aa827 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) { http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/services/iam/plugin/src/org/apache/cloudstack/iam/RoleBasedEntityQuerySelector.java ---------------------------------------------------------------------- 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 b7c3d35..5d31433 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<Long> domainIds = new ArrayList<Long>(); for (IAMPolicy policy : policies) { List<IAMPolicyPermission> pp = new ArrayList<IAMPolicyPermission>(); - 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<Long> accountIds = new ArrayList<Long>(); for (IAMPolicy policy : policies) { List<IAMPolicyPermission> pp = new ArrayList<IAMPolicyPermission>(); - 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<Long> entityIds = new ArrayList<Long>(); for (IAMPolicy policy : policies) { List<IAMPolicyPermission> pp = new ArrayList<IAMPolicyPermission>(); - 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<IAMPolicyPermission> pp = new ArrayList<IAMPolicyPermission>(); - 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; http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/services/iam/server/src/org/apache/cloudstack/iam/api/IAMService.java ---------------------------------------------------------------------- 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 c396fa9..20326e97 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<IAMPolicyPermission> listPolicyPermissions(long policyId); - List<IAMPolicyPermission> listPolicyPermissionsByScope(long policyId, String action, String scope, AccessType accessType); + List<IAMPolicyPermission> listPolicyPermissionsByScope(long policyId, String action, String scope, String accessType); List<IAMPolicyPermission> listPolicyPermissionByActionAndEntity(long policyId, String action, String entityType); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/services/iam/server/src/org/apache/cloudstack/iam/server/IAMServiceImpl.java ---------------------------------------------------------------------- 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 d6a61a1..c35ac1d 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<IAMPolicyPermission> listPolicyPermissionsByScope(long policyId, String action, String scope, AccessType accessType) { + public List<IAMPolicyPermission> 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; } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDao.java ---------------------------------------------------------------------- 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 6e6099c..ebb4916 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<IAMPolicyPermissionVO List<IAMPolicyPermissionVO> listByPolicy(long policyId); IAMPolicyPermissionVO findByPolicyAndEntity(long policyId, String entityType, String scope, Long scopeId, - String action, Permission perm); + String action, Permission perm, String accessType); List<IAMPolicyPermissionVO> listByPolicyActionAndScope(long policyId, String action, String scope, String accessType); http://git-wip-us.apache.org/repos/asf/cloudstack/blob/c32b7ab7/services/iam/server/src/org/apache/cloudstack/iam/server/dao/IAMPolicyPermissionDaoImpl.java ---------------------------------------------------------------------- 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 9cc1af0..44b77d1 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<IAMPolicyPermissi @Override public IAMPolicyPermissionVO findByPolicyAndEntity(long policyId, String entityType, String scope, Long scopeId, - String action, Permission perm) { + String action, Permission perm, String accessType) { SearchCriteria<IAMPolicyPermissionVO> sc = fullSearch.create(); sc.setParameters("policyId", policyId); sc.setParameters("entityType", entityType); @@ -78,6 +78,11 @@ public class IAMPolicyPermissionDaoImpl extends GenericDaoBase<IAMPolicyPermissi sc.setParameters("scopeId", scopeId); sc.setParameters("action", action); sc.setParameters("permission", perm); + if (accessType != null) { + // accessType can be optional, used mainly in list apis with + // ListEntry and UseEntry distinction + sc.setParameters("accessType", accessType); + } return findOneBy(sc); }
