From f8ba33d5703035facab88106b60923d797aa852b Mon Sep 17 00:00:00 2001 From: lujiefsi Date: Mon, 26 Apr 2021 16:09:03 +0800 Subject: [PATCH] server: Some APIs should have access check (#4859) This PR fixes the CLOUDSTACK-10434. I think some APIs lack access check and list them in below table. I also give the pattch to add the access check for the api in this table. Anyone chould change this table, If you think the APIs do not need access check and change their lable as "no". API Lack? VolumeApiServiceImpl # updateVolume yes VolumeApiServiceImpl # detachVolumeViaDestroyVM yes VolumeApiServiceImpl # takeSnapshot yes VolumeApiServiceImpl # migrateVolume yes AccountManagerImpl#createApiKeyAndSecretKey yes LoadBalancingRulesManagerImpl#applyLBStickinessPolicy yes LoadBalancingRulesManagerImpl#applyLBHealthCheckPolicy yes TemplateManagerImpl#createPrivateTemplate yes SnapshotManagerImpl#updateSnapshotPolicy Co-authored-by: lujie --- .../lb/LoadBalancingRulesManagerImpl.java | 4 ++++ .../com/cloud/storage/VolumeApiServiceImpl.java | 16 +++++++++++++++- .../storage/snapshot/SnapshotManagerImpl.java | 8 ++++++++ .../com/cloud/template/TemplateManagerImpl.java | 6 ++++++ .../java/com/cloud/user/AccountManagerImpl.java | 3 +++ .../java/com/cloud/vm/UserVmManagerImpl.java | 4 ++++ 6 files changed, 40 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java index 0ac03742046..d612ad53966 100644 --- a/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java +++ b/server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java @@ -663,6 +663,9 @@ public class LoadBalancingRulesManagerImpl extends ManagerBase implements if (loadBalancer == null) { throw new InvalidParameterException("Invalid Load balancer Id:" + cmd.getLbRuleId()); } + + _accountMgr.checkAccess(CallContext.current().getCallingAccount(), null, true, loadBalancer); + List stickinessPolicies = _lb2stickinesspoliciesDao.listByLoadBalancerId(cmd.getLbRuleId(), false); for (LBStickinessPolicyVO stickinessPolicy : stickinessPolicies) { if (stickinessPolicy.getId() == cmd.getEntityId()) { @@ -717,6 +720,7 @@ public class LoadBalancingRulesManagerImpl extends ManagerBase implements if (loadBalancer == null) { throw new InvalidParameterException("Invalid Load balancer Id:" + cmd.getLbRuleId()); } + _accountMgr.checkAccess(CallContext.current().getCallingAccount(), null, true, loadBalancer); FirewallRule.State backupState = loadBalancer.getState(); loadBalancer.setState(FirewallRule.State.Add); _lbDao.persist(loadBalancer); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 74b65b8ccdc..c45cadea99d 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -1783,13 +1783,16 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic @Override @ActionEvent(eventType = EventTypes.EVENT_VOLUME_UPDATE, eventDescription = "updating volume", async = true) public Volume updateVolume(long volumeId, String path, String state, Long storageId, Boolean displayVolume, String customId, long entityOwnerId, String chainInfo) { - + Account caller = CallContext.current().getCallingAccount(); VolumeVO volume = _volsDao.findById(volumeId); if (volume == null) { throw new InvalidParameterValueException("The volume id doesn't exist"); } + /* Does the caller have authority to act on this volume? */ + _accountMgr.checkAccess(caller, null, true, volume); + if (path != null) { volume.setPath(path); } @@ -2013,6 +2016,10 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic @ActionEvent(eventType = EventTypes.EVENT_VOLUME_DETACH, eventDescription = "detaching volume") public Volume detachVolumeViaDestroyVM(long vmId, long volumeId) { + Account caller = CallContext.current().getCallingAccount(); + Volume volume = _volsDao.findById(volumeId); + // Permissions check + _accountMgr.checkAccess(caller, null, true, volume); return orchestrateDetachVolumeFromVM(vmId, volumeId); } @@ -2202,6 +2209,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic @Override @ActionEvent(eventType = EventTypes.EVENT_VOLUME_MIGRATE, eventDescription = "migrating volume", async = true) public Volume migrateVolume(MigrateVolumeCmd cmd) { + Account caller = CallContext.current().getCallingAccount(); Long volumeId = cmd.getVolumeId(); Long storagePoolId = cmd.getStoragePoolId(); @@ -2210,6 +2218,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic throw new InvalidParameterValueException("Failed to find the volume id: " + volumeId); } + _accountMgr.checkAccess(caller, null, true, vol); + if (vol.getState() != Volume.State.Ready) { throw new InvalidParameterValueException("Volume must be in ready state"); } @@ -2575,11 +2585,14 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic private Snapshot takeSnapshotInternal(Long volumeId, Long policyId, Long snapshotId, Account account, boolean quiescevm, Snapshot.LocationType locationType, boolean asyncBackup) throws ResourceAllocationException { + Account caller = CallContext.current().getCallingAccount(); VolumeInfo volume = volFactory.getVolume(volumeId); if (volume == null) { throw new InvalidParameterValueException("Creating snapshot failed due to volume:" + volumeId + " doesn't exist"); } + _accountMgr.checkAccess(caller, null, true, volume); + if (volume.getState() != Volume.State.Ready) { throw new InvalidParameterValueException("VolumeId: " + volumeId + " is not in " + Volume.State.Ready + " state but " + volume.getState() + ". Cannot take snapshot."); } @@ -2596,6 +2609,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic } if (vm != null) { + _accountMgr.checkAccess(caller, null, true, vm); // serialize VM operation AsyncJobExecutionContext jobContext = AsyncJobExecutionContext.getCurrentExecutionContext(); if (jobContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER)) { diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index f46367835ee..4b95eff7b7c 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -335,6 +335,14 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement Boolean display = cmd.getDisplay(); SnapshotPolicyVO policyVO = _snapshotPolicyDao.findById(id); + VolumeInfo volume = volFactory.getVolume(policyVO.getVolumeId()); + if (volume == null) { + throw new InvalidParameterValueException("No such volume exist"); + } + + // does the caller have the authority to act on this volume + _accountMgr.checkAccess(CallContext.current().getCallingAccount(), null, true, volume); + if (display != null) { boolean previousDisplay = policyVO.isDisplay(); policyVO.setDisplay(display); diff --git a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java index 57a8fdfe10c..deb5feb2e81 100755 --- a/server/src/main/java/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/main/java/com/cloud/template/TemplateManagerImpl.java @@ -1648,6 +1648,7 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, final Long accountId = CallContext.current().getCallingAccountId(); SnapshotVO snapshot = null; VolumeVO volume = null; + Account caller = CallContext.current().getCallingAccount(); try { TemplateInfo tmplInfo = _tmplFactory.getTemplate(templateId, DataStoreRole.Image); @@ -1686,6 +1687,7 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, throw new CloudRuntimeException("Cannot find snapshot " + snapshotId + " on secondary and could not create backup"); } } + _accountMgr.checkAccess(caller, null, true, snapInfo); DataStore snapStore = snapInfo.getDataStore(); if (snapStore != null) { @@ -1696,7 +1698,11 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, future = _tmpltSvr.createTemplateFromSnapshotAsync(snapInfo, tmplInfo, store); } else if (volumeId != null) { VolumeInfo volInfo = _volFactory.getVolume(volumeId); + if (volInfo == null) { + throw new InvalidParameterValueException("No such volume exist"); + } + _accountMgr.checkAccess(caller, null, true, volInfo); future = _tmpltSvr.createTemplateFromVolumeAsync(volInfo, tmplInfo, store); } else { throw new CloudRuntimeException("Creating private Template need to specify snapshotId or volumeId"); diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index a20090cf7c1..054c3342204 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -2487,10 +2487,13 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @DB @ActionEvent(eventType = EventTypes.EVENT_REGISTER_FOR_SECRET_API_KEY, eventDescription = "register for the developer API keys") public String[] createApiKeyAndSecretKey(final long userId) { + Account caller = getCurrentCallingAccount(); User user = getUserIncludingRemoved(userId); if (user == null) { throw new InvalidParameterValueException("Unable to find user by id"); } + Account account = _accountDao.findById(user.getAccountId()); + checkAccess(caller, null, true, account); final String[] keys = new String[2]; Transaction.execute(new TransactionCallbackNoReturn() { @Override diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 6f668516744..951f03de65c 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -56,6 +56,7 @@ import com.cloud.deployasis.dao.UserVmDeployAsIsDetailsDao; import com.cloud.exception.UnsupportedServiceException; import com.cloud.hypervisor.Hypervisor; import com.cloud.deployasis.dao.TemplateDeployAsIsDetailsDao; +import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.affinity.AffinityGroupService; @@ -2966,6 +2967,9 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir checkForUnattachedVolumes(vmId, volumesToBeDeleted); validateVolumes(volumesToBeDeleted); + final ControlledEntity[] volumesToDelete = volumesToBeDeleted.toArray(new ControlledEntity[0]); + _accountMgr.checkAccess(ctx.getCallingAccount(), null, true, volumesToDelete); + stopVirtualMachine(vmId, VmDestroyForcestop.value()); detachVolumesFromVm(volumesToBeDeleted);