From 00fe5f14719d22c429d609657887198d93c1b98f Mon Sep 17 00:00:00 2001 From: dahn Date: Tue, 1 Oct 2024 10:25:31 +0200 Subject: [PATCH 1/9] cleanup validations for VPN connection creation (#9195) --- .../network/vpn/Site2SiteVpnManagerImpl.java | 114 ++++++++++-------- ui/src/views/network/VpcTab.vue | 8 +- 2 files changed, 67 insertions(+), 55 deletions(-) diff --git a/server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java b/server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java index 51d5f9cf97b..3efe9a8788f 100644 --- a/server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java @@ -23,11 +23,12 @@ import java.util.Map; import javax.inject.Inject; import javax.naming.ConfigurationException; -import org.apache.cloudstack.annotation.AnnotationService; -import org.apache.cloudstack.annotation.dao.AnnotationDao; +import org.apache.commons.collections.CollectionUtils; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; +import org.apache.cloudstack.annotation.AnnotationService; +import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.command.user.vpn.CreateVpnConnectionCmd; import org.apache.cloudstack.api.command.user.vpn.CreateVpnCustomerGatewayCmd; import org.apache.cloudstack.api.command.user.vpn.CreateVpnGatewayCmd; @@ -46,7 +47,6 @@ import com.cloud.configuration.Config; import com.cloud.event.ActionEvent; import com.cloud.event.EventTypes; import com.cloud.exception.InvalidParameterValueException; -import com.cloud.exception.NetworkRuleConflictException; import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceUnavailableException; import com.cloud.network.Site2SiteCustomerGateway; @@ -108,7 +108,6 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn @Inject private AnnotationDao annotationDao; - String _name; int _connLimit; int _subnetsLimit; @@ -255,7 +254,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn @Override @ActionEvent(eventType = EventTypes.EVENT_S2S_VPN_CONNECTION_CREATE, eventDescription = "creating s2s vpn connection", create = true) - public Site2SiteVpnConnection createVpnConnection(CreateVpnConnectionCmd cmd) throws NetworkRuleConflictException { + public Site2SiteVpnConnection createVpnConnection(CreateVpnConnectionCmd cmd) { Account caller = CallContext.current().getCallingAccount(); Account owner = _accountMgr.getAccount(cmd.getEntityOwnerId()); @@ -263,27 +262,15 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn _accountMgr.checkAccess(caller, null, false, owner); Long customerGatewayId = cmd.getCustomerGatewayId(); - Site2SiteCustomerGateway customerGateway = _customerGatewayDao.findById(customerGatewayId); - if (customerGateway == null) { - throw new InvalidParameterValueException("Unable to found specified Site to Site VPN customer gateway " + customerGatewayId + " !"); - } - _accountMgr.checkAccess(caller, null, false, customerGateway); + Site2SiteCustomerGateway customerGateway = getAndValidateSite2SiteCustomerGateway(customerGatewayId, caller); Long vpnGatewayId = cmd.getVpnGatewayId(); - Site2SiteVpnGateway vpnGateway = _vpnGatewayDao.findById(vpnGatewayId); - if (vpnGateway == null) { - throw new InvalidParameterValueException("Unable to found specified Site to Site VPN gateway " + vpnGatewayId + " !"); - } - _accountMgr.checkAccess(caller, null, false, vpnGateway); + Site2SiteVpnGateway vpnGateway = getAndValidateSite2SiteVpnGateway(vpnGatewayId, caller); - if (customerGateway.getAccountId() != vpnGateway.getAccountId() || customerGateway.getDomainId() != vpnGateway.getDomainId()) { - throw new InvalidParameterValueException("VPN connection can only be esitablished between same account's VPN gateway and customer gateway!"); - } + validateVpnConnectionOfTheRightAccount(customerGateway, vpnGateway); + validateVpnConnectionDoesntExist(vpnGatewayId, customerGatewayId); + validatePrerequisiteVpnGateway(vpnGateway); - if (_vpnConnectionDao.findByVpnGatewayIdAndCustomerGatewayId(vpnGatewayId, customerGatewayId) != null) { - throw new InvalidParameterValueException("The vpn connection with customer gateway id " + customerGatewayId + " and vpn gateway id " + vpnGatewayId + - " already existed!"); - } String[] cidrList = customerGateway.getGuestCidrList().split(","); // Remote sub nets cannot overlap VPC's sub net @@ -326,13 +313,51 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn return conn; } + private Site2SiteCustomerGateway getAndValidateSite2SiteCustomerGateway(Long customerGatewayId, Account caller) { + Site2SiteCustomerGateway customerGateway = _customerGatewayDao.findById(customerGatewayId); + if (customerGateway == null) { + throw new InvalidParameterValueException(String.format("Unable to find specified Site to Site VPN customer gateway %s !", customerGatewayId)); + } + _accountMgr.checkAccess(caller, null, false, customerGateway); + return customerGateway; + } + + private Site2SiteVpnGateway getAndValidateSite2SiteVpnGateway(Long vpnGatewayId, Account caller) { + Site2SiteVpnGateway vpnGateway = _vpnGatewayDao.findById(vpnGatewayId); + if (vpnGateway == null) { + throw new InvalidParameterValueException(String.format("Unable to find specified Site to Site VPN gateway %s !", vpnGatewayId)); + } + _accountMgr.checkAccess(caller, null, false, vpnGateway); + return vpnGateway; + } + + private void validateVpnConnectionOfTheRightAccount(Site2SiteCustomerGateway customerGateway, Site2SiteVpnGateway vpnGateway) { + if (customerGateway.getAccountId() != vpnGateway.getAccountId() || customerGateway.getDomainId() != vpnGateway.getDomainId()) { + throw new InvalidParameterValueException("VPN connection can only be established between same account's VPN gateway and customer gateway!"); + } + } + + private void validateVpnConnectionDoesntExist(Long vpnGatewayId, Long customerGatewayId) { + if (_vpnConnectionDao.findByVpnGatewayIdAndCustomerGatewayId(vpnGatewayId, customerGatewayId) != null) { + throw new InvalidParameterValueException("The vpn connection with customer gateway id " + customerGatewayId + " and vpn gateway id " + vpnGatewayId + + " already existed!"); + } + } + + private void validatePrerequisiteVpnGateway(Site2SiteVpnGateway vpnGateway) { + // check if gateway has been defined on the VPC + if (_vpnGatewayDao.findByVpcId(vpnGateway.getVpcId()) == null) { + throw new InvalidParameterValueException("we can not create a VPN connection for a VPC that does not have a VPN gateway defined"); + } + } + @Override @DB @ActionEvent(eventType = EventTypes.EVENT_S2S_VPN_CONNECTION_CREATE, eventDescription = "starting s2s vpn connection", async = true) public Site2SiteVpnConnection startVpnConnection(long id) throws ResourceUnavailableException { Site2SiteVpnConnectionVO conn = _vpnConnectionDao.acquireInLockTable(id); if (conn == null) { - throw new CloudRuntimeException("Unable to acquire lock on " + conn); + throw new CloudRuntimeException("Unable to acquire lock for starting of VPN connection with ID " + id); } try { if (conn.getState() != State.Pending && conn.getState() != State.Disconnected) { @@ -382,11 +407,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn Account caller = CallContext.current().getCallingAccount(); Long id = cmd.getId(); - Site2SiteCustomerGateway customerGateway = _customerGatewayDao.findById(id); - if (customerGateway == null) { - throw new InvalidParameterValueException("Fail to find customer gateway with " + id + " !"); - } - _accountMgr.checkAccess(caller, null, false, customerGateway); + Site2SiteCustomerGateway customerGateway = getAndValidateSite2SiteCustomerGateway(id, caller); return doDeleteCustomerGateway(customerGateway); } @@ -394,7 +415,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn protected boolean doDeleteCustomerGateway(Site2SiteCustomerGateway gw) { long id = gw.getId(); List vpnConnections = _vpnConnectionDao.listByCustomerGatewayId(id); - if (vpnConnections != null && vpnConnections.size() != 0) { + if (!CollectionUtils.isEmpty(vpnConnections)) { throw new InvalidParameterValueException("Unable to delete VPN customer gateway with id " + id + " because there is still related VPN connections!"); } annotationDao.removeByEntityType(AnnotationService.EntityType.VPN_CUSTOMER_GATEWAY.name(), gw.getUuid()); @@ -404,7 +425,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn protected void doDeleteVpnGateway(Site2SiteVpnGateway gw) { List conns = _vpnConnectionDao.listByVpnGatewayId(gw.getId()); - if (conns != null && conns.size() != 0) { + if (!CollectionUtils.isEmpty(conns)) { throw new InvalidParameterValueException("Unable to delete VPN gateway " + gw.getId() + " because there is still related VPN connections!"); } _vpnGatewayDao.remove(gw.getId()); @@ -417,12 +438,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn Account caller = CallContext.current().getCallingAccount(); Long id = cmd.getId(); - Site2SiteVpnGateway vpnGateway = _vpnGatewayDao.findById(id); - if (vpnGateway == null) { - throw new InvalidParameterValueException("Fail to find vpn gateway with " + id + " !"); - } - - _accountMgr.checkAccess(caller, null, false, vpnGateway); + Site2SiteVpnGateway vpnGateway = getAndValidateSite2SiteVpnGateway(id, caller); doDeleteVpnGateway(vpnGateway); return true; @@ -578,7 +594,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn private void stopVpnConnection(Long id) throws ResourceUnavailableException { Site2SiteVpnConnectionVO conn = _vpnConnectionDao.acquireInLockTable(id); if (conn == null) { - throw new CloudRuntimeException("Unable to acquire lock on " + conn); + throw new CloudRuntimeException("Unable to acquire lock for stopping of VPN connection with ID " + id); } try { if (conn.getState() == State.Pending) { @@ -639,10 +655,9 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn String keyword = cmd.getKeyword(); Account caller = CallContext.current().getCallingAccount(); - List permittedAccounts = new ArrayList(); + List permittedAccounts = new ArrayList<>(); - Ternary domainIdRecursiveListProject = new Ternary(domainId, isRecursive, null); + Ternary domainIdRecursiveListProject = new Ternary<>(domainId, isRecursive, null); _accountMgr.buildACLSearchParameters(caller, id, accountName, cmd.getProjectId(), permittedAccounts, domainIdRecursiveListProject, listAll, false); domainId = domainIdRecursiveListProject.first(); isRecursive = domainIdRecursiveListProject.second(); @@ -667,7 +682,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn } Pair, Integer> result = _customerGatewayDao.searchAndCount(sc, searchFilter); - return new Pair, Integer>(result.first(), result.second()); + return new Pair<>(result.first(), result.second()); } @Override @@ -684,10 +699,9 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn long pageSizeVal = cmd.getPageSizeVal(); Account caller = CallContext.current().getCallingAccount(); - List permittedAccounts = new ArrayList(); + List permittedAccounts = new ArrayList<>(); - Ternary domainIdRecursiveListProject = new Ternary(domainId, isRecursive, null); + Ternary domainIdRecursiveListProject = new Ternary<>(domainId, isRecursive, null); _accountMgr.buildACLSearchParameters(caller, id, accountName, cmd.getProjectId(), permittedAccounts, domainIdRecursiveListProject, listAll, false); domainId = domainIdRecursiveListProject.first(); isRecursive = domainIdRecursiveListProject.second(); @@ -717,7 +731,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn } Pair, Integer> result = _vpnGatewayDao.searchAndCount(sc, searchFilter); - return new Pair, Integer>(result.first(), result.second()); + return new Pair<>(result.first(), result.second()); } @Override @@ -734,10 +748,9 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn long pageSizeVal = cmd.getPageSizeVal(); Account caller = CallContext.current().getCallingAccount(); - List permittedAccounts = new ArrayList(); + List permittedAccounts = new ArrayList<>(); - Ternary domainIdRecursiveListProject = new Ternary(domainId, isRecursive, null); + Ternary domainIdRecursiveListProject = new Ternary<>(domainId, isRecursive, null); _accountMgr.buildACLSearchParameters(caller, id, accountName, cmd.getProjectId(), permittedAccounts, domainIdRecursiveListProject, listAll, false); domainId = domainIdRecursiveListProject.first(); isRecursive = domainIdRecursiveListProject.second(); @@ -771,7 +784,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn } Pair, Integer> result = _vpnConnectionDao.searchAndCount(sc, searchFilter); - return new Pair, Integer>(result.first(), result.second()); + return new Pair<>(result.first(), result.second()); } @Override @@ -818,7 +831,7 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn @Override public List getConnectionsForRouter(DomainRouterVO router) { - List conns = new ArrayList(); + List conns = new ArrayList<>(); // One router for one VPC Long vpcId = router.getVpcId(); if (router.getVpcId() == null) { @@ -831,7 +844,6 @@ public class Site2SiteVpnManagerImpl extends ManagerBase implements Site2SiteVpn @Override public boolean deleteCustomerGatewayByAccount(long accountId) { boolean result = true; - ; List gws = _customerGatewayDao.listByAccountId(accountId); for (Site2SiteCustomerGatewayVO gw : gws) { result = result & doDeleteCustomerGateway(gw); diff --git a/ui/src/views/network/VpcTab.vue b/ui/src/views/network/VpcTab.vue index 4603e0abf69..657d3c133ac 100644 --- a/ui/src/views/network/VpcTab.vue +++ b/ui/src/views/network/VpcTab.vue @@ -795,12 +795,12 @@ export default { this.formRef.value.validate().then(() => { const values = toRaw(this.form) - - api('createVpnConnection', { - s2svpngatewayid: this.vpnGateways[0].id, + const params = { + s2svpngatewayid: this.vpnGateways[0] ? this.vpnGateways[0].id : null, s2scustomergatewayid: values.vpncustomergateway, passive: values.passive ? values.passive : false - }).then(response => { + } + api('createVpnConnection', params).then(response => { this.$pollJob({ jobId: response.createvpnconnectionresponse.jobid, title: this.$t('label.vpn.connection'), From 28f425a9f97bee9ed505017c119e865cd4ff0a73 Mon Sep 17 00:00:00 2001 From: Felipe <124818914+FelipeM525@users.noreply.github.com> Date: Tue, 1 Oct 2024 17:37:47 -0300 Subject: [PATCH 2/9] Hide UserData field from the EditVM view for VMs that do not offer it (#9731) * added a v-if directive within the EditVm view to not show the field userdata if the vm's network does not offer the feature * added the parameter listall:true to the requests made to listNetworks and listVirtualMachines --- ui/src/views/compute/EditVM.vue | 35 +++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/ui/src/views/compute/EditVM.vue b/ui/src/views/compute/EditVM.vue index 550c4645ed6..876bc7819b4 100644 --- a/ui/src/views/compute/EditVM.vue +++ b/ui/src/views/compute/EditVM.vue @@ -84,7 +84,7 @@ }" :options="groups.opts" /> - + @@ -143,6 +143,7 @@ export default { return { serviceOffering: {}, template: {}, + userDataEnabled: false, securityGroupsEnabled: false, dynamicScalingVmConfig: false, loading: false, @@ -289,15 +290,37 @@ export default { return decodedData.toString('utf-8') }, fetchUserData () { - const params = { - id: this.resource.id, - userdata: true + let networkId + this.resource.nic.forEach(nic => { + if (nic.isdefault) { + networkId = nic.networkid + } + }) + if (!networkId) { + return } + const listNetworkParams = { + id: networkId, + listall: true + } + api(`listNetworks`, listNetworkParams).then(json => { + json.listnetworksresponse.network[0].service.forEach(service => { + if (service.name === 'UserData') { + this.userDataEnabled = true - api('listVirtualMachines', params).then(json => { - this.form.userdata = this.decodeUserData(json.listvirtualmachinesresponse.virtualmachine[0].userdata || '') + const listVmParams = { + id: this.resource.id, + userdata: true, + listall: true + } + api('listVirtualMachines', listVmParams).then(json => { + this.form.userdata = atob(json.listvirtualmachinesresponse.virtualmachine[0].userdata || '') + }) + } + }) }) }, + handleSubmit () { this.formRef.value.validate().then(() => { const values = toRaw(this.form) From 2e4dd69fa179c58c6d785c681d4bafd5000e2d42 Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Wed, 2 Oct 2024 09:25:29 -0400 Subject: [PATCH 3/9] API: Fix listing Userdata by keyword or name (#9751) --- .../src/main/java/com/cloud/server/ManagementServerImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 5ac34fba1d5..c032019510f 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -4753,7 +4753,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe sb.and("id", sb.entity().getId(), SearchCriteria.Op.EQ); sb.and("name", sb.entity().getName(), SearchCriteria.Op.EQ); - sb.and("name", sb.entity().getName(), SearchCriteria.Op.EQ); + sb.and("keyword", sb.entity().getName(), SearchCriteria.Op.LIKE); final SearchCriteria sc = sb.create(); _accountMgr.buildACLSearchCriteria(sc, domainId, isRecursive, permittedAccounts, listProjectResourcesCriteria); @@ -4766,7 +4766,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe } if (keyword != null) { - sc.setParameters("name", "%" + keyword + "%"); + sc.setParameters("keyword", "%" + keyword + "%"); } final Pair, Integer> result = userDataDao.searchAndCount(sc, searchFilter); From e666dca403d48765aac3692bf59566eae4285200 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Jandre?= <48719461+JoaoJandre@users.noreply.github.com> Date: Wed, 2 Oct 2024 13:15:29 -0300 Subject: [PATCH 4/9] linked clone for file based storage (#8911) --- .../StorageSystemDataMotionStrategy.java | 73 +++++++++++-------- .../kvm/resource/MigrateKVMAsync.java | 2 + 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java index c83d52c3d2b..82f9a84cdb5 100644 --- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java +++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java @@ -1957,25 +1957,26 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { * - Full clones (no backing file): Take snapshot of the VM prior disk creation * Return this information */ - protected void setVolumeMigrationOptions(VolumeInfo srcVolumeInfo, VolumeInfo destVolumeInfo, - VirtualMachineTO vmTO, Host srcHost, StoragePoolVO destStoragePool) { - if (!destStoragePool.isManaged()) { - String srcVolumeBackingFile = getVolumeBackingFile(srcVolumeInfo); - - String srcPoolUuid = srcVolumeInfo.getDataStore().getUuid(); - StoragePoolVO srcPool = _storagePoolDao.findById(srcVolumeInfo.getPoolId()); - Storage.StoragePoolType srcPoolType = srcPool.getPoolType(); - - MigrationOptions migrationOptions; - if (StringUtils.isNotBlank(srcVolumeBackingFile)) { - migrationOptions = createLinkedCloneMigrationOptions(srcVolumeInfo, destVolumeInfo, - srcVolumeBackingFile, srcPoolUuid, srcPoolType); - } else { - migrationOptions = createFullCloneMigrationOptions(srcVolumeInfo, vmTO, srcHost, srcPoolUuid, srcPoolType); - } - migrationOptions.setTimeout(StorageManager.KvmStorageOnlineMigrationWait.value()); - destVolumeInfo.setMigrationOptions(migrationOptions); + protected void setVolumeMigrationOptions(VolumeInfo srcVolumeInfo, VolumeInfo destVolumeInfo, VirtualMachineTO vmTO, Host srcHost, StoragePoolVO destStoragePool, + MigrationOptions.Type migrationType) { + if (destStoragePool.isManaged()) { + return; } + + String srcVolumeBackingFile = getVolumeBackingFile(srcVolumeInfo); + + String srcPoolUuid = srcVolumeInfo.getDataStore().getUuid(); + StoragePoolVO srcPool = _storagePoolDao.findById(srcVolumeInfo.getPoolId()); + Storage.StoragePoolType srcPoolType = srcPool.getPoolType(); + + MigrationOptions migrationOptions; + if (MigrationOptions.Type.LinkedClone.equals(migrationType)) { + migrationOptions = createLinkedCloneMigrationOptions(srcVolumeInfo, destVolumeInfo, srcVolumeBackingFile, srcPoolUuid, srcPoolType); + } else { + migrationOptions = createFullCloneMigrationOptions(srcVolumeInfo, vmTO, srcHost, srcPoolUuid, srcPoolType); + } + migrationOptions.setTimeout(StorageManager.KvmStorageOnlineMigrationWait.value()); + destVolumeInfo.setMigrationOptions(migrationOptions); } /** @@ -2006,6 +2007,7 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { Map srcVolumeInfoToDestVolumeInfo = new HashMap<>(); boolean managedStorageDestination = false; + boolean migrateNonSharedInc = false; for (Map.Entry entry : volumeDataStoreMap.entrySet()) { VolumeInfo srcVolumeInfo = entry.getKey(); DataStore destDataStore = entry.getValue(); @@ -2023,15 +2025,8 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { continue; } - VMTemplateVO vmTemplate = _vmTemplateDao.findById(vmInstance.getTemplateId()); - if (srcVolumeInfo.getTemplateId() != null && - Objects.nonNull(vmTemplate) && - !Arrays.asList(KVM_VM_IMPORT_DEFAULT_TEMPLATE_NAME, VM_IMPORT_DEFAULT_TEMPLATE_NAME).contains(vmTemplate.getName())) { - LOGGER.debug(String.format("Copying template [%s] of volume [%s] from source storage pool [%s] to target storage pool [%s].", srcVolumeInfo.getTemplateId(), srcVolumeInfo.getId(), sourceStoragePool.getId(), destStoragePool.getId())); - copyTemplateToTargetFilesystemStorageIfNeeded(srcVolumeInfo, sourceStoragePool, destDataStore, destStoragePool, destHost); - } else { - LOGGER.debug(String.format("Skipping copy template from source storage pool [%s] to target storage pool [%s] before migration due to volume [%s] does not have a template.", sourceStoragePool.getId(), destStoragePool.getId(), srcVolumeInfo.getId())); - } + MigrationOptions.Type migrationType = decideMigrationTypeAndCopyTemplateIfNeeded(destHost, vmInstance, srcVolumeInfo, sourceStoragePool, destStoragePool, destDataStore); + migrateNonSharedInc = migrateNonSharedInc || MigrationOptions.Type.LinkedClone.equals(migrationType); VolumeVO destVolume = duplicateVolumeOnAnotherStorage(srcVolume, destStoragePool); VolumeInfo destVolumeInfo = _volumeDataFactory.getVolume(destVolume.getId(), destDataStore); @@ -2043,7 +2038,7 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { // move the volume from Ready to Migrating destVolumeInfo.processEvent(Event.MigrationRequested); - setVolumeMigrationOptions(srcVolumeInfo, destVolumeInfo, vmTO, srcHost, destStoragePool); + setVolumeMigrationOptions(srcVolumeInfo, destVolumeInfo, vmTO, srcHost, destStoragePool, migrationType); // create a volume on the destination storage destDataStore.getDriver().createAsync(destDataStore, destVolumeInfo, null); @@ -2058,7 +2053,7 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { _volumeDao.update(destVolume.getId(), destVolume); - postVolumeCreationActions(srcVolumeInfo, destVolumeInfo, vmTO, srcHost); + postVolumeCreationActions(srcVolumeInfo, destVolumeInfo); destVolumeInfo = _volumeDataFactory.getVolume(destVolume.getId(), destDataStore); @@ -2109,8 +2104,6 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { VMInstanceVO vm = _vmDao.findById(vmTO.getId()); boolean isWindows = _guestOsCategoryDao.findById(_guestOsDao.findById(vm.getGuestOSId()).getCategoryId()).getName().equalsIgnoreCase("Windows"); - boolean migrateNonSharedInc = isSourceAndDestinationPoolTypeOfNfs(volumeDataStoreMap); - MigrateCommand migrateCommand = new MigrateCommand(vmTO.getName(), destHost.getPrivateIpAddress(), isWindows, vmTO, true); migrateCommand.setWait(StorageManager.KvmStorageOnlineMigrationWait.value()); migrateCommand.setMigrateStorage(migrateStorage); @@ -2160,6 +2153,22 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { } } + private MigrationOptions.Type decideMigrationTypeAndCopyTemplateIfNeeded(Host destHost, VMInstanceVO vmInstance, VolumeInfo srcVolumeInfo, StoragePoolVO sourceStoragePool, StoragePoolVO destStoragePool, DataStore destDataStore) { + VMTemplateVO vmTemplate = _vmTemplateDao.findById(vmInstance.getTemplateId()); + String srcVolumeBackingFile = getVolumeBackingFile(srcVolumeInfo); + if (StringUtils.isNotBlank(srcVolumeBackingFile) && supportStoragePoolType(destStoragePool.getPoolType(), StoragePoolType.Filesystem) && + srcVolumeInfo.getTemplateId() != null && + Objects.nonNull(vmTemplate) && + !Arrays.asList(KVM_VM_IMPORT_DEFAULT_TEMPLATE_NAME, VM_IMPORT_DEFAULT_TEMPLATE_NAME).contains(vmTemplate.getName())) { + LOGGER.debug(String.format("Copying template [%s] of volume [%s] from source storage pool [%s] to target storage pool [%s].", srcVolumeInfo.getTemplateId(), srcVolumeInfo.getId(), sourceStoragePool.getId(), destStoragePool.getId())); + copyTemplateToTargetFilesystemStorageIfNeeded(srcVolumeInfo, sourceStoragePool, destDataStore, destStoragePool, destHost); + return MigrationOptions.Type.LinkedClone; + } + LOGGER.debug(String.format("Skipping copy template from source storage pool [%s] to target storage pool [%s] before migration due to volume [%s] does not have a " + + "template or we are doing full clone migration.", sourceStoragePool.getId(), destStoragePool.getId(), srcVolumeInfo.getId())); + return MigrationOptions.Type.FullClone; + } + protected String formatMigrationElementsAsJsonToDisplayOnLog(String objectName, Object object, Object from, Object to){ return String.format("{%s: \"%s\", from: \"%s\", to:\"%s\"}", objectName, object, from, to); } @@ -2421,7 +2430,7 @@ public class StorageSystemDataMotionStrategy implements DataMotionStrategy { /** * Handle post destination volume creation actions depending on the migrating volume type: full clone or linked clone */ - protected void postVolumeCreationActions(VolumeInfo srcVolumeInfo, VolumeInfo destVolumeInfo, VirtualMachineTO vmTO, Host srcHost) { + protected void postVolumeCreationActions(VolumeInfo srcVolumeInfo, VolumeInfo destVolumeInfo) { MigrationOptions migrationOptions = destVolumeInfo.getMigrationOptions(); if (migrationOptions != null) { if (migrationOptions.getType() == MigrationOptions.Type.LinkedClone && migrationOptions.isCopySrcTemplate()) { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java index bc94bb47ed8..09ae293ceb7 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/MigrateKVMAsync.java @@ -122,8 +122,10 @@ public class MigrateKVMAsync implements Callable { if (migrateNonSharedInc) { flags |= VIR_MIGRATE_PERSIST_DEST; flags |= VIR_MIGRATE_NON_SHARED_INC; + logger.debug("Setting VIR_MIGRATE_NON_SHARED_INC for linked clone migration."); } else { flags |= VIR_MIGRATE_NON_SHARED_DISK; + logger.debug("Setting VIR_MIGRATE_NON_SHARED_DISK for full clone migration."); } } From ee0ab2ac9e441638225a774365c956baf451d6ed Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 28 Aug 2024 15:22:29 +0200 Subject: [PATCH 5/9] Session Token Invalidation on Logout --- .../main/java/com/cloud/api/ApiServlet.java | 27 ++++++++++--------- ui/src/api/index.js | 1 - ui/src/store/modules/user.js | 11 ++++---- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index f6f46419c04..e719238afef 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -260,19 +260,22 @@ public class ApiServlet extends HttpServlet { } if (apiAuthenticator.getAPIType() == APIAuthenticationType.LOGOUT_API) { - if (session != null) { - final Long userId = (Long) session.getAttribute("userid"); - final Account account = (Account) session.getAttribute("accountobj"); - Long accountId = null; - if (account != null) { - accountId = account.getId(); - } - auditTrailSb.insert(0, "(userId=" + userId + " accountId=" + accountId + " sessionId=" + session.getId() + ")"); - if (userId != null) { - apiServer.logoutUser(userId); - } - invalidateHttpSession(session, "invalidating session after logout call"); + if (session == null) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Session not found for the logout process."); } + + final Long userId = (Long) session.getAttribute("userid"); + final Account account = (Account) session.getAttribute("accountobj"); + Long accountId = null; + if (account != null) { + accountId = account.getId(); + } + auditTrailSb.insert(0, "(userId=" + userId + " accountId=" + accountId + " sessionId=" + session.getId() + ")"); + if (userId != null) { + apiServer.logoutUser(userId); + } + invalidateHttpSession(session, "invalidating session after logout call"); + final Cookie[] cookies = req.getCookies(); if (cookies != null) { for (final Cookie cookie : cookies) { diff --git a/ui/src/api/index.js b/ui/src/api/index.js index 1db41661276..14432010738 100644 --- a/ui/src/api/index.js +++ b/ui/src/api/index.js @@ -65,7 +65,6 @@ export function login (arg) { } export function logout () { - sourceToken.cancel() message.destroy() notification.destroy() return api('logout') diff --git a/ui/src/store/modules/user.js b/ui/src/store/modules/user.js index fb5b6ff5e0b..08a0c340c64 100644 --- a/ui/src/store/modules/user.js +++ b/ui/src/store/modules/user.js @@ -24,6 +24,7 @@ import router from '@/router' import store from '@/store' import { oauthlogin, login, logout, api } from '@/api' import { i18n } from '@/locales' +import { sourceToken } from '@/utils/request' import { ACCESS_TOKEN, @@ -374,11 +375,6 @@ const user = { cloudianUrl = state.cloudian.url + 'logout.htm?redirect=' + encodeURIComponent(window.location.href) } - Object.keys(Cookies.get()).forEach(cookieName => { - Cookies.remove(cookieName) - Cookies.remove(cookieName, { path: '/client' }) - }) - commit('SET_TOKEN', '') commit('SET_APIS', {}) commit('SET_PROJECT', {}) @@ -406,6 +402,11 @@ const user = { } }).catch(() => { resolve() + }).finally(() => { + Object.keys(Cookies.get()).forEach(cookieName => { + Cookies.remove(cookieName) + Cookies.remove(cookieName, { path: '/client' }) + }) }) }) }, From 7d70e32378f0d5141d54fa3c95df855f147e66a8 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 20 Aug 2024 15:12:54 +0200 Subject: [PATCH 6/9] fix quota resource access validation --- .../java/com/cloud/user/AccountService.java | 2 + .../api/command/QuotaBalanceCmd.java | 13 +- .../api/command/QuotaCreditsCmd.java | 6 + .../api/command/QuotaStatementCmd.java | 6 + .../management/MockAccountManager.java | 5 + .../api/dispatch/ParamProcessWorker.java | 59 ++++---- .../com/cloud/user/AccountManagerImpl.java | 14 ++ .../api/dispatch/ParamProcessWorkerTest.java | 131 +++++++++++++++--- .../cloud/user/MockAccountManagerImpl.java | 5 + 9 files changed, 189 insertions(+), 52 deletions(-) diff --git a/api/src/main/java/com/cloud/user/AccountService.java b/api/src/main/java/com/cloud/user/AccountService.java index 63f5455cfd0..60db7abb734 100644 --- a/api/src/main/java/com/cloud/user/AccountService.java +++ b/api/src/main/java/com/cloud/user/AccountService.java @@ -116,6 +116,8 @@ public interface AccountService { void checkAccess(Account account, AccessType accessType, boolean sameOwner, String apiName, ControlledEntity... entities) throws PermissionDeniedException; + void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource); + Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly); /** diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java index f4e248855fd..ecb6531d0cf 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaBalanceCmd.java @@ -21,6 +21,8 @@ import java.util.List; import javax.inject.Inject; +import com.cloud.user.Account; +import org.apache.cloudstack.api.ACL; import org.apache.log4j.Logger; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; @@ -42,6 +44,7 @@ public class QuotaBalanceCmd extends BaseCmd { @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Account Id for which statement needs to be generated") private String accountName; + @ACL @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "If domain Id is given and the caller is domain admin then the statement is generated for domain.") private Long domainId; @@ -51,6 +54,7 @@ public class QuotaBalanceCmd extends BaseCmd { @Parameter(name = ApiConstants.START_DATE, type = CommandType.DATE, description = "Start date range quota query. Use yyyy-MM-dd as the date format, e.g. startDate=2009-06-01.") private Date startDate; + @ACL @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "List usage records for the specified account") private Long accountId; @@ -104,7 +108,14 @@ public class QuotaBalanceCmd extends BaseCmd { @Override public long getEntityOwnerId() { - return _accountService.getActiveAccountByName(accountName, domainId).getAccountId(); + if (accountId != null) { + return accountId; + } + Account account = _accountService.getActiveAccountByName(accountName, domainId); + if (account != null) { + return account.getAccountId(); + } + return Account.ACCOUNT_ID_SYSTEM; } @Override diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java index c47c0ad2d76..e83d52f94f4 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaCreditsCmd.java @@ -18,6 +18,7 @@ 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.ApiErrorCode; @@ -48,6 +49,7 @@ public class QuotaCreditsCmd extends BaseCmd { @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Account Id for which quota credits need to be added") private String accountName; + @ACL @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "Domain for which quota credits need to be added") private Long domainId; @@ -132,6 +134,10 @@ public class QuotaCreditsCmd extends BaseCmd { @Override public long getEntityOwnerId() { + Account account = _accountService.getActiveAccountByName(accountName, domainId); + if (account != null) { + return account.getAccountId(); + } return Account.ACCOUNT_ID_SYSTEM; } diff --git a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java index 4d1c233c37a..3219ba1448f 100644 --- a/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java +++ b/plugins/database/quota/src/main/java/org/apache/cloudstack/api/command/QuotaStatementCmd.java @@ -21,6 +21,7 @@ import java.util.List; import javax.inject.Inject; +import org.apache.cloudstack.api.ACL; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseCmd; @@ -44,6 +45,7 @@ public class QuotaStatementCmd extends BaseCmd { @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, required = true, description = "Optional, Account Id for which statement needs to be generated") private String accountName; + @ACL @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, required = true, entityType = DomainResponse.class, description = "Optional, If domain Id is given and the caller is domain admin then the statement is generated for domain.") private Long domainId; @@ -56,6 +58,7 @@ public class QuotaStatementCmd extends BaseCmd { @Parameter(name = ApiConstants.TYPE, type = CommandType.INTEGER, description = "List quota usage records for the specified usage type") private Integer usageType; + @ACL @Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, description = "List usage records for the specified account") private Long accountId; @@ -112,6 +115,9 @@ public class QuotaStatementCmd extends BaseCmd { @Override public long getEntityOwnerId() { + if (accountId != null) { + return accountId; + } Account activeAccountByName = _accountService.getActiveAccountByName(accountName, domainId); if (activeAccountByName != null) { return activeAccountByName.getAccountId(); 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 67cfe1df3e1..a3fac2c8be9 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 @@ -451,6 +451,11 @@ public class MockAccountManager extends ManagerBase implements AccountManager { // TODO Auto-generated method stub } + @Override + public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) { + // TODO Auto-generated method stub + } + @Override public Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly) { // TODO Auto-generated method stub diff --git a/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java b/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java index 9f07db4b033..b11a5282e62 100644 --- a/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java +++ b/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java @@ -32,8 +32,6 @@ import java.util.regex.Matcher; import javax.inject.Inject; -import org.apache.cloudstack.acl.ControlledEntity; -import org.apache.cloudstack.acl.InfrastructureEntity; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.ACL; @@ -288,41 +286,44 @@ public class ParamProcessWorker implements DispatchWorker { doAccessChecks(cmd, entitiesToAccess); } - private void doAccessChecks(BaseCmd cmd, Map entitiesToAccess) { - Account caller = CallContext.current().getCallingAccount(); - List entityOwners = cmd.getEntityOwnerIds(); - Account[] owners = null; - if (entityOwners != null) { - owners = entityOwners.stream().map(id -> _accountMgr.getAccount(id)).toArray(Account[]::new); - } else { - if (cmd.getEntityOwnerId() == Account.ACCOUNT_ID_SYSTEM && cmd instanceof BaseAsyncCmd && ((BaseAsyncCmd)cmd).getApiResourceType() == ApiCommandResourceType.Network) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("Skipping access check on the network owner if the owner is ROOT/system."); - } - owners = new Account[]{}; - } else { - owners = new Account[]{_accountMgr.getAccount(cmd.getEntityOwnerId())}; - } - } + protected void doAccessChecks(BaseCmd cmd, Map entitiesToAccess) { + Account[] owners = getEntityOwners(cmd); + Account caller = CallContext.current().getCallingAccount(); if (cmd instanceof BaseAsyncCreateCmd) { // check that caller can access the owner account. _accountMgr.checkAccess(caller, null, false, owners); } - if (!entitiesToAccess.isEmpty()) { - // check that caller can access the owner account. - _accountMgr.checkAccess(caller, null, false, owners); - for (Map.Entryentry : entitiesToAccess.entrySet()) { - Object entity = entry.getKey(); - if (entity instanceof ControlledEntity) { - _accountMgr.checkAccess(caller, entry.getValue(), true, (ControlledEntity) entity); - } else if (entity instanceof InfrastructureEntity) { - // FIXME: Move this code in adapter, remove code from - // Account manager - } + checkCallerAccessToEntities(caller, owners, entitiesToAccess); + } + + protected Account[] getEntityOwners(BaseCmd cmd) { + List entityOwners = cmd.getEntityOwnerIds(); + if (entityOwners != null) { + return entityOwners.stream().map(id -> _accountMgr.getAccount(id)).toArray(Account[]::new); + } + + if (cmd.getEntityOwnerId() == Account.ACCOUNT_ID_SYSTEM && cmd instanceof BaseAsyncCmd && cmd.getApiResourceType() == ApiCommandResourceType.Network) { + s_logger.debug("Skipping access check on the network owner if the owner is ROOT/system."); + } else { + Account owner = _accountMgr.getAccount(cmd.getEntityOwnerId()); + if (owner != null) { + return new Account[]{owner}; } } + return new Account[]{}; + } + + protected void checkCallerAccessToEntities(Account caller, Account[] owners, Map entitiesToAccess) { + if (entitiesToAccess.isEmpty()) { + return; + } + _accountMgr.checkAccess(caller, null, false, owners); + for (Map.Entry entry : entitiesToAccess.entrySet()) { + Object entity = entry.getKey(); + _accountMgr.validateAccountHasAccessToResource(caller, entry.getValue(), entity); + } } @SuppressWarnings({"unchecked", "rawtypes"}) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 0a0d9265ac4..c7ceb00cb57 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -43,6 +43,7 @@ import javax.naming.ConfigurationException; import org.apache.cloudstack.acl.APIChecker; import org.apache.cloudstack.acl.ControlledEntity; +import org.apache.cloudstack.acl.InfrastructureEntity; import org.apache.cloudstack.acl.QuerySelector; import org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RoleService; @@ -722,6 +723,19 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } + @Override + public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) { + Class resourceClass = resource.getClass(); + if (ControlledEntity.class.isAssignableFrom(resourceClass)) { + checkAccess(account, accessType, true, (ControlledEntity) resource); + } else if (Domain.class.isAssignableFrom(resourceClass)) { + checkAccess(account, (Domain) resource); + } else if (InfrastructureEntity.class.isAssignableFrom(resourceClass)) { + s_logger.trace("Validation of access to infrastructure entity has been disabled in CloudStack version 4.4."); + } + s_logger.debug(String.format("Account [%s] has access to resource.", account.getUuid())); + } + @Override public Long checkAccessAndSpecifyAuthority(Account caller, Long zoneId) { // We just care for resource domain admins for now, and they should be permitted to see only their zone. diff --git a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java index cf2a43eceda..0604405f4a5 100644 --- a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java +++ b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java @@ -18,6 +18,29 @@ */ package com.cloud.api.dispatch; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.InjectMocks; +import org.mockito.Spy; + +import org.mockito.junit.MockitoJUnitRunner; + +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.user.address.AssociateIPAddrCmd; +import org.apache.cloudstack.acl.SecurityChecker; +import org.apache.cloudstack.context.CallContext; + import com.cloud.exception.ConcurrentOperationException; import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.NetworkRuleConflictException; @@ -26,28 +49,33 @@ import com.cloud.exception.ResourceUnavailableException; import com.cloud.user.Account; import com.cloud.user.AccountManager; import com.cloud.user.User; -import org.apache.cloudstack.api.BaseCmd; -import org.apache.cloudstack.api.Parameter; -import org.apache.cloudstack.api.ServerApiException; -import org.apache.cloudstack.context.CallContext; -import org.junit.After; -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.mockito.runners.MockitoJUnitRunner; - -import java.util.HashMap; +import com.cloud.vm.VMInstanceVO; @RunWith(MockitoJUnitRunner.class) public class ParamProcessWorkerTest { - @Mock - protected AccountManager accountManager; + @Spy + @InjectMocks + private ParamProcessWorker paramProcessWorkerSpy; - protected ParamProcessWorker paramProcessWorker; + @Mock + private AccountManager accountManagerMock; + + @Mock + private Account callingAccountMock; + + @Mock + private User callingUserMock; + + @Mock + private Account ownerAccountMock; + + @Mock + BaseCmd baseCmdMock; + + private Account[] owners = new Account[]{ownerAccountMock}; + + private Map entities = new HashMap<>(); public static class TestCmd extends BaseCmd { @@ -65,7 +93,7 @@ public class ParamProcessWorkerTest { @Override public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, - ResourceAllocationException, NetworkRuleConflictException { + ResourceAllocationException, NetworkRuleConflictException { // well documented nothing } @@ -83,9 +111,7 @@ public class ParamProcessWorkerTest { @Before public void setup() { - CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class)); - paramProcessWorker = new ParamProcessWorker(); - paramProcessWorker._accountMgr = accountManager; + CallContext.register(callingUserMock, callingAccountMock); } @After @@ -101,10 +127,71 @@ public class ParamProcessWorkerTest { params.put("boolparam1", "true"); params.put("doubleparam1", "11.89"); final TestCmd cmd = new TestCmd(); - paramProcessWorker.processParameters(cmd, params); + paramProcessWorkerSpy.processParameters(cmd, params); Assert.assertEquals("foo", cmd.strparam1); Assert.assertEquals(100, cmd.intparam1); Assert.assertTrue(Double.compare(cmd.doubleparam1, 11.89) == 0); + Mockito.verify(paramProcessWorkerSpy).doAccessChecks(Mockito.any(), Mockito.any()); } + @Test + public void doAccessChecksTestChecksCallerAccessToOwnerWhenCmdExtendsBaseAsyncCreateCmd() { + Mockito.doReturn(owners).when(paramProcessWorkerSpy).getEntityOwners(Mockito.any()); + Mockito.doNothing().when(paramProcessWorkerSpy).checkCallerAccessToEntities(Mockito.any(), Mockito.any(), Mockito.any()); + + paramProcessWorkerSpy.doAccessChecks(new AssociateIPAddrCmd(), entities); + + Mockito.verify(accountManagerMock).checkAccess(callingAccountMock, null, false, owners); + } + + @Test + public void doAccessChecksTestChecksCallerAccessToEntities() { + Mockito.doReturn(owners).when(paramProcessWorkerSpy).getEntityOwners(Mockito.any()); + Mockito.doNothing().when(paramProcessWorkerSpy).checkCallerAccessToEntities(Mockito.any(), Mockito.any(), Mockito.any()); + + paramProcessWorkerSpy.doAccessChecks(new AssociateIPAddrCmd(), entities); + + Mockito.verify(paramProcessWorkerSpy).checkCallerAccessToEntities(callingAccountMock, owners, entities); + } + + @Test + public void getEntityOwnersTestReturnsAccountsWhenCmdHasMultipleEntityOwners() { + Mockito.when(baseCmdMock.getEntityOwnerIds()).thenReturn(List.of(1L, 2L)); + Mockito.doReturn(callingAccountMock).when(accountManagerMock).getAccount(1L); + Mockito.doReturn(ownerAccountMock).when(accountManagerMock).getAccount(2L); + + List result = List.of(paramProcessWorkerSpy.getEntityOwners(baseCmdMock)); + + Assert.assertEquals(List.of(callingAccountMock, ownerAccountMock), result); + } + + @Test + public void getEntityOwnersTestReturnsAccountWhenCmdHasOneEntityOwner() { + Mockito.when(baseCmdMock.getEntityOwnerId()).thenReturn(1L); + Mockito.when(baseCmdMock.getEntityOwnerIds()).thenReturn(null); + Mockito.doReturn(ownerAccountMock).when(accountManagerMock).getAccount(1L); + + List result = List.of(paramProcessWorkerSpy.getEntityOwners(baseCmdMock)); + + Assert.assertEquals(List.of(ownerAccountMock), result); + } + + @Test + public void checkCallerAccessToEntitiesTestChecksCallerAccessToOwners() { + entities.put(ownerAccountMock, SecurityChecker.AccessType.UseEntry); + + paramProcessWorkerSpy.checkCallerAccessToEntities(callingAccountMock, owners, entities); + + Mockito.verify(accountManagerMock).checkAccess(callingAccountMock, null, false, owners); + } + + @Test + public void checkCallerAccessToEntitiesTestChecksCallerAccessToResource() { + VMInstanceVO vmInstanceVo = new VMInstanceVO(); + entities.put(vmInstanceVo, SecurityChecker.AccessType.UseEntry); + + paramProcessWorkerSpy.checkCallerAccessToEntities(callingAccountMock, owners, entities); + + Mockito.verify(accountManagerMock).validateAccountHasAccessToResource(callingAccountMock, SecurityChecker.AccessType.UseEntry, vmInstanceVo); + } } diff --git a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java index fe7748b8581..f8558992440 100644 --- a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java +++ b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java @@ -438,6 +438,11 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco // TODO Auto-generated method stub } + @Override + public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) { + // TODO Auto-generated method stub + } + @Override public Long finalyzeAccountId(String accountName, Long domainId, Long projectId, boolean enabledOnly) { // TODO Auto-generated method stub From 066d5bcbff1b6b967fa21cf1013225437249bb97 Mon Sep 17 00:00:00 2001 From: Daniel Augusto Veronezi Salvador Date: Fri, 30 Aug 2024 19:03:28 -0300 Subject: [PATCH 7/9] Validate QCOW2 on upload and register --- .../formatinspector/Qcow2HeaderField.java | 51 ++++ .../formatinspector/Qcow2Inspector.java | 267 ++++++++++++++++++ .../resource/NfsSecondaryStorageResource.java | 14 +- .../storage/template/DownloadManagerImpl.java | 39 ++- 4 files changed, 360 insertions(+), 11 deletions(-) create mode 100644 services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2HeaderField.java create mode 100644 services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2Inspector.java diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2HeaderField.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2HeaderField.java new file mode 100644 index 00000000000..4a8e8b51a47 --- /dev/null +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2HeaderField.java @@ -0,0 +1,51 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.storage.formatinspector; + +public enum Qcow2HeaderField { + MAGIC(0, 4), + VERSION(4, 4), + BACKING_FILE_OFFSET(8, 8), + BACKING_FILE_NAME_LENGTH(16, 4), + CLUSTER_BITS(20, 4), + SIZE(24, 8), + CRYPT_METHOD(32, 4), + L1_SIZE(36, 4), + LI_TABLE_OFFSET(40, 8), + REFCOUNT_TABLE_OFFSET(48, 8), + REFCOUNT_TABLE_CLUSTERS(56, 4), + NB_SNAPSHOTS(60, 4), + SNAPSHOTS_OFFSET(64, 8), + INCOMPATIBLE_FEATURES(72, 8); + + private final int offset; + private final int length; + + Qcow2HeaderField(int offset, int length) { + this.offset = offset; + this.length = length; + } + + public int getLength() { + return length; + } + + public int getOffset() { + return offset; + } +} diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2Inspector.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2Inspector.java new file mode 100644 index 00000000000..1ad2076a12d --- /dev/null +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/formatinspector/Qcow2Inspector.java @@ -0,0 +1,267 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.cloudstack.storage.formatinspector; + +import com.cloud.utils.NumbersUtil; +import org.apache.commons.lang3.ArrayUtils; +import org.apache.log4j.Logger; + +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +/** + * Class to inspect QCOW2 files/objects. In our context, a QCOW2 might be a threat to the environment if it meets one of the following criteria when coming from external sources + * (like registering or uploading volumes and templates): + *
    + *
  • has a backing file reference;
    • + *
  • has an external data file reference;
    • + *
  • has unknown incompatible features.
    • + *
+ * + * The implementation was done based on the QEMU's official interoperability documentation + * and on the OpenStack's Cinder implementation for Python. + */ +public class Qcow2Inspector { + protected static Logger LOGGER = Logger.getLogger(Qcow2Inspector.class); + + private static final byte[] QCOW_MAGIC_STRING = ArrayUtils.add("QFI".getBytes(), (byte) 0xfb); + private static final int INCOMPATIBLE_FEATURES_MAX_KNOWN_BIT = 4; + private static final int INCOMPATIBLE_FEATURES_MAX_KNOWN_BYTE = 0; + private static final int EXTERNAL_DATA_FILE_BYTE_POSITION = 7; + private static final int EXTERNAL_DATA_FILE_BIT = 2; + private static final byte EXTERNAL_DATA_FILE_BITMASK = (byte) (1 << EXTERNAL_DATA_FILE_BIT); + + private static final Set SET_OF_HEADER_FIELDS_TO_READ = Set.of(Qcow2HeaderField.MAGIC, + Qcow2HeaderField.VERSION, + Qcow2HeaderField.SIZE, + Qcow2HeaderField.BACKING_FILE_OFFSET, + Qcow2HeaderField.INCOMPATIBLE_FEATURES); + + /** + * Validates if the file is a valid and allowed QCOW2 (i.e.: does not contain external references). + * @param filePath Path of the file to be validated. + * @throws RuntimeException If the QCOW2 file meets one of the following criteria: + *
    + *
  • has a backing file reference;
    • + *
  • has an external data file reference;
    • + *
  • has unknown incompatible features.
    • + *
+ */ + public static void validateQcow2File(String filePath) throws RuntimeException { + LOGGER.info(String.format("Verifying if [%s] is a valid and allowed QCOW2 file .", filePath)); + + Map headerFieldsAndValues; + try (InputStream inputStream = new FileInputStream(filePath)) { + headerFieldsAndValues = unravelQcow2Header(inputStream, filePath); + } catch (IOException ex) { + throw new RuntimeException(String.format("Unable to validate file [%s] due to: ", filePath), ex); + } + + validateQcow2HeaderFields(headerFieldsAndValues, filePath); + + LOGGER.info(String.format("[%s] is a valid and allowed QCOW2 file.", filePath)); + } + + /** + * Unravels the QCOW2 header in a serial fashion, iterating through the {@link Qcow2HeaderField}, reading the fields specified in + * {@link Qcow2Inspector#SET_OF_HEADER_FIELDS_TO_READ} and skipping the others. + * @param qcow2InputStream InputStream of the QCOW2 being unraveled. + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @return A map of the header fields and their values according to the {@link Qcow2Inspector#SET_OF_HEADER_FIELDS_TO_READ}. + * @throws IOException If the field cannot be read or skipped. + */ + public static Map unravelQcow2Header(InputStream qcow2InputStream, String qcow2LogReference) throws IOException { + Map result = new HashMap<>(); + + LOGGER.debug(String.format("Unraveling QCOW2 [%s] headers.", qcow2LogReference)); + for (Qcow2HeaderField qcow2Header : Qcow2HeaderField.values()) { + if (!SET_OF_HEADER_FIELDS_TO_READ.contains(qcow2Header)) { + skipHeader(qcow2InputStream, qcow2Header, qcow2LogReference); + continue; + } + + byte[] headerValue = readHeader(qcow2InputStream, qcow2Header, qcow2LogReference); + result.put(qcow2Header.name(), headerValue); + } + + return result; + } + + /** + * Skips the field's length in the InputStream. + * @param qcow2InputStream InputStream of the QCOW2 being unraveled. + * @param field Field being skipped (name and length). + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws IOException If the bytes skipped do not match the field length. + */ + protected static void skipHeader(InputStream qcow2InputStream, Qcow2HeaderField field, String qcow2LogReference) throws IOException { + LOGGER.trace(String.format("Skipping field [%s] of QCOW2 [%s].", field, qcow2LogReference)); + + if (qcow2InputStream.skip(field.getLength()) != field.getLength()) { + throw new IOException(String.format("Unable to skip field [%s] of QCOW2 [%s].", field, qcow2LogReference)); + } + } + + /** + * Reads the field's length in the InputStream. + * @param qcow2InputStream InputStream of the QCOW2 being unraveled. + * @param field Field being read (name and length). + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws IOException If the bytes read do not match the field length. + */ + protected static byte[] readHeader(InputStream qcow2InputStream, Qcow2HeaderField field, String qcow2LogReference) throws IOException { + byte[] readBytes = new byte[field.getLength()]; + + LOGGER.trace(String.format("Reading field [%s] of QCOW2 [%s].", field, qcow2LogReference)); + if (qcow2InputStream.read(readBytes) != field.getLength()) { + throw new IOException(String.format("Unable to read field [%s] of QCOW2 [%s].", field, qcow2LogReference)); + } + + LOGGER.trace(String.format("Read %s as field [%s] of QCOW2 [%s].", ArrayUtils.toString(readBytes), field, qcow2LogReference)); + return readBytes; + } + + /** + * Validates the values of the header fields {@link Qcow2HeaderField#MAGIC}, {@link Qcow2HeaderField#BACKING_FILE_OFFSET}, and {@link Qcow2HeaderField#INCOMPATIBLE_FEATURES}. + * @param headerFieldsAndValues A map of the header fields and their values. + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws SecurityException If the QCOW2 does not contain the QCOW magic string or contains a backing file reference or incompatible features. + */ + public static void validateQcow2HeaderFields(Map headerFieldsAndValues, String qcow2LogReference) throws SecurityException{ + byte[] fieldValue = headerFieldsAndValues.get(Qcow2HeaderField.MAGIC.name()); + validateQcowMagicString(fieldValue, qcow2LogReference); + + fieldValue = headerFieldsAndValues.get(Qcow2HeaderField.BACKING_FILE_OFFSET.name()); + validateAbsenceOfBackingFileReference(NumbersUtil.bytesToLong(fieldValue), qcow2LogReference); + + fieldValue = headerFieldsAndValues.get(Qcow2HeaderField.INCOMPATIBLE_FEATURES.name()); + validateAbsenceOfIncompatibleFeatures(fieldValue, qcow2LogReference); + } + + /** + * Verifies if the first 4 bytes of the header are the QCOW magic string. Throws an exception if not. + * @param headerMagicString The first 4 bytes of the header. + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws SecurityException If the header's magic string is not the QCOW magic string. + */ + private static void validateQcowMagicString(byte[] headerMagicString, String qcow2LogReference) throws SecurityException { + LOGGER.debug(String.format("Verifying if [%s] has a valid QCOW magic string.", qcow2LogReference)); + + if (!Arrays.equals(QCOW_MAGIC_STRING, headerMagicString)) { + throw new SecurityException(String.format("[%s] is not a valid QCOW2 because its first 4 bytes are not the QCOW magic string.", qcow2LogReference)); + } + + LOGGER.debug(String.format("[%s] has a valid QCOW magic string.", qcow2LogReference)); + } + + /** + * Verifies if the QCOW2 has a backing file and throws an exception if so. + * @param backingFileOffset The backing file offset value of the QCOW2 header. + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws SecurityException If the QCOW2 has a backing file reference. + */ + private static void validateAbsenceOfBackingFileReference(long backingFileOffset, String qcow2LogReference) throws SecurityException { + LOGGER.debug(String.format("Verifying if [%s] has a backing file reference.", qcow2LogReference)); + + if (backingFileOffset != 0) { + throw new SecurityException(String.format("[%s] has a backing file reference. This can be an attack to the infrastructure; therefore, we will not accept" + + " this QCOW2.", qcow2LogReference)); + } + + LOGGER.debug(String.format("[%s] does not have a backing file reference.", qcow2LogReference)); + } + + /** + * Verifies if the QCOW2 has incompatible features and throw an exception if it has an external data file reference or unknown incompatible features. + * @param incompatibleFeatures The incompatible features bytes of the QCOW2 header. + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws SecurityException If the QCOW2 has an external data file reference or unknown incompatible features. + */ + private static void validateAbsenceOfIncompatibleFeatures(byte[] incompatibleFeatures, String qcow2LogReference) throws SecurityException { + LOGGER.debug(String.format("Verifying if [%s] has incompatible features.", qcow2LogReference)); + + if (NumbersUtil.bytesToLong(incompatibleFeatures) == 0) { + LOGGER.debug(String.format("[%s] does not have incompatible features.", qcow2LogReference)); + return; + } + + LOGGER.debug(String.format("[%s] has incompatible features.", qcow2LogReference)); + + validateAbsenceOfExternalDataFileReference(incompatibleFeatures, qcow2LogReference); + validateAbsenceOfUnknownIncompatibleFeatures(incompatibleFeatures, qcow2LogReference); + } + + /** + * Verifies if the QCOW2 has an external data file reference and throw an exception if so. + * @param incompatibleFeatures The incompatible features bytes of the QCOW2 header. + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws SecurityException If the QCOW2 has an external data file reference. + */ + private static void validateAbsenceOfExternalDataFileReference(byte[] incompatibleFeatures, String qcow2LogReference) throws SecurityException { + LOGGER.debug(String.format("Verifying if [%s] has an external data file reference.", qcow2LogReference)); + + if ((incompatibleFeatures[EXTERNAL_DATA_FILE_BYTE_POSITION] & EXTERNAL_DATA_FILE_BITMASK) != 0) { + throw new SecurityException(String.format("[%s] has an external data file reference. This can be an attack to the infrastructure; therefore, we will discard" + + " this file.", qcow2LogReference)); + } + + LOGGER.info(String.format("[%s] does not have an external data file reference.", qcow2LogReference)); + } + + /** + * Verifies if the QCOW2 has unknown incompatible features and throw an exception if so. + *

+ * Unknown incompatible features are those with bit greater than + * {@link Qcow2Inspector#INCOMPATIBLE_FEATURES_MAX_KNOWN_BIT}, which will be the represented by bytes in positions greater than + * {@link Qcow2Inspector#INCOMPATIBLE_FEATURES_MAX_KNOWN_BYTE} (in Big Endian order). Therefore, we expect that those bytes are always zero. If not, an exception is thrown. + * @param incompatibleFeatures The incompatible features bytes of the QCOW2 header. + * @param qcow2LogReference A reference (like the filename) of the QCOW2 being unraveled to print in the logs and exceptions. + * @throws SecurityException If the QCOW2 has unknown incompatible features. + */ + private static void validateAbsenceOfUnknownIncompatibleFeatures(byte[] incompatibleFeatures, String qcow2LogReference) throws SecurityException { + LOGGER.debug(String.format("Verifying if [%s] has unknown incompatible features [%s].", qcow2LogReference, ArrayUtils.toString(incompatibleFeatures))); + + for (int byteNum = incompatibleFeatures.length - 1; byteNum >= 0; byteNum--) { + int bytePosition = incompatibleFeatures.length - 1 - byteNum; + LOGGER.trace(String.format("Looking for unknown incompatible feature bit in position [%s].", bytePosition)); + + byte bitmask = 0; + if (byteNum == INCOMPATIBLE_FEATURES_MAX_KNOWN_BYTE) { + bitmask = ((1 << INCOMPATIBLE_FEATURES_MAX_KNOWN_BIT) - 1); + } + + LOGGER.trace(String.format("Bitmask for byte in position [%s] is [%s].", bytePosition, Integer.toBinaryString(bitmask))); + + int featureBit = incompatibleFeatures[bytePosition] & ~bitmask; + if (featureBit != 0) { + throw new SecurityException(String.format("Found unknown incompatible feature bit [%s] in byte [%s] of [%s]. This can be an attack to the infrastructure; " + + "therefore, we will discard this QCOW2.", featureBit, bytePosition + Qcow2HeaderField.INCOMPATIBLE_FEATURES.getOffset(), qcow2LogReference)); + } + + LOGGER.trace(String.format("Did not find unknown incompatible feature in position [%s].", bytePosition)); + } + + LOGGER.info(String.format("[%s] does not have unknown incompatible features.", qcow2LogReference)); + } + +} diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java index 41cf11025a1..ceaa37926e9 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/resource/NfsSecondaryStorageResource.java @@ -71,6 +71,7 @@ import org.apache.cloudstack.storage.command.UploadStatusCommand; import org.apache.cloudstack.storage.command.browser.ListDataStoreObjectsCommand; import org.apache.cloudstack.storage.configdrive.ConfigDrive; import org.apache.cloudstack.storage.configdrive.ConfigDriveBuilder; +import org.apache.cloudstack.storage.formatinspector.Qcow2Inspector; import org.apache.cloudstack.storage.template.DownloadManager; import org.apache.cloudstack.storage.template.DownloadManagerImpl; import org.apache.cloudstack.storage.template.UploadEntity; @@ -3482,8 +3483,19 @@ public class NfsSecondaryStorageResource extends ServerResourceBase implements S return result; } + String finalFilename = resourcePath + "/" + templateFilename; + + if (ImageStoreUtil.isCorrectExtension(finalFilename, "qcow2")) { + try { + Qcow2Inspector.validateQcow2File(finalFilename); + } catch (RuntimeException e) { + s_logger.error(String.format("Uploaded file [%s] is not a valid QCOW2.", finalFilename), e); + return "The uploaded file is not a valid QCOW2. Ask the administrator to check the logs for more details."; + } + } + // Set permissions for the downloaded template - File downloadedTemplate = new File(resourcePath + "/" + templateFilename); + File downloadedTemplate = new File(finalFilename); _storage.setWorldReadableAndWriteable(downloadedTemplate); // Set permissions for template/volume.properties diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java index 11878ae6400..c946614934b 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java @@ -16,8 +16,6 @@ // under the License. package org.apache.cloudstack.storage.template; -import static com.cloud.utils.NumbersUtil.toHumanReadableSize; - import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -51,9 +49,11 @@ import org.apache.cloudstack.storage.command.DownloadProgressCommand.RequestType import org.apache.cloudstack.storage.resource.IpTablesHelper; import org.apache.cloudstack.storage.resource.NfsSecondaryStorageResource; import org.apache.cloudstack.storage.resource.SecondaryStorageResource; +import org.apache.cloudstack.storage.formatinspector.Qcow2HeaderField; +import org.apache.cloudstack.storage.formatinspector.Qcow2Inspector; import org.apache.cloudstack.utils.security.ChecksumValue; import org.apache.cloudstack.utils.security.DigestHelper; -import org.apache.commons.lang3.StringUtils; + import org.apache.log4j.Logger; import com.cloud.agent.api.storage.DownloadAnswer; @@ -91,7 +91,9 @@ import com.cloud.utils.component.ManagerBase; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.Proxy; import com.cloud.utils.script.Script; -import com.cloud.utils.storage.QCOW2Utils; +import com.cloud.utils.StringUtils; + +import static com.cloud.utils.NumbersUtil.toHumanReadableSize; public class DownloadManagerImpl extends ManagerBase implements DownloadManager { private String _name; @@ -365,11 +367,17 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager // The QCOW2 is the only format with a header, // and as such can be easily read. - try (InputStream inputStream = td.getS3ObjectInputStream();) { - dnld.setTemplatesize(QCOW2Utils.getVirtualSize(inputStream, false)); - } - catch (IOException e) { - result = "Couldn't read QCOW2 virtual size. Error: " + e.getMessage(); + try (InputStream inputStream = td.getS3ObjectInputStream()) { + Map qcow2HeaderFieldsAndValues = Qcow2Inspector.unravelQcow2Header(inputStream, td.getDownloadUrl()); + Qcow2Inspector.validateQcow2HeaderFields(qcow2HeaderFieldsAndValues, td.getDownloadUrl()); + + dnld.setTemplatesize(NumbersUtil.bytesToLong(qcow2HeaderFieldsAndValues.get(Qcow2HeaderField.SIZE.name()))); + } catch (IOException ex) { + result = String.format("Unable to read QCOW2 metadata. Error: %s", ex.getMessage()); + LOGGER.error(result, ex); + } catch (SecurityException ex) { + result = String.format("[%s] is not a valid QCOW2:", td.getDownloadUrl()); + LOGGER.error(result, ex); } } @@ -515,8 +523,19 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager return result; } + String finalFilename = resourcePath + "/" + templateFilename; + + if (ImageFormat.QCOW2.equals(dnld.getFormat())) { + try { + Qcow2Inspector.validateQcow2File(finalFilename); + } catch (RuntimeException e) { + LOGGER.error(String.format("The downloaded file [%s] is not a valid QCOW2.", finalFilename), e); + return "The downloaded file is not a valid QCOW2. Ask the administrator to check the logs for more details."; + } + } + // Set permissions for the downloaded template - File downloadedTemplate = new File(resourcePath + "/" + templateFilename); + File downloadedTemplate = new File(finalFilename); _storage.setWorldReadableAndWriteable(downloadedTemplate); setPermissionsForTheDownloadedTemplate(resourcePath, resourceType); From 5ab0a52d66235a0a825d44f74cf7505d0044799c Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Tue, 10 Sep 2024 18:25:36 +0200 Subject: [PATCH 8/9] util: check JSESSIONID in cookies if user is passed --- .../command/ListAndSwitchSAMLAccountCmd.java | 7 +- .../cloudstack/saml/SAML2AuthManager.java | 3 + .../cloudstack/saml/SAML2AuthManagerImpl.java | 3 +- .../org/apache/cloudstack/saml/SAMLUtils.java | 25 ++++- .../ListAndSwitchSAMLAccountCmdTest.java | 25 ++++- .../main/java/com/cloud/api/ApiServer.java | 26 ++++- .../main/java/com/cloud/api/ApiServlet.java | 102 +++++++++++------- test/integration/smoke/test_login.py | 1 + ui/src/api/index.js | 10 ++ ui/src/store/modules/user.js | 1 - .../main/java/com/cloud/utils/HttpUtils.java | 42 ++++++-- .../java/com/cloud/utils/HttpUtilsTest.java | 86 ++++++++++++--- 12 files changed, 266 insertions(+), 65 deletions(-) diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java index 25f056adf68..c2f81cd3356 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmd.java @@ -48,6 +48,7 @@ import org.apache.cloudstack.saml.SAML2AuthManager; import org.apache.cloudstack.saml.SAMLUtils; import org.apache.log4j.Logger; +import com.cloud.api.ApiServer; import com.cloud.api.response.ApiResponseSerializer; import com.cloud.domain.Domain; import com.cloud.domain.dao.DomainDao; @@ -60,6 +61,8 @@ import com.cloud.user.dao.UserAccountDao; import com.cloud.user.dao.UserDao; import com.cloud.utils.HttpUtils; +import org.apache.commons.lang3.EnumUtils; + @APICommand(name = "listAndSwitchSamlAccount", description = "Lists and switches to other SAML accounts owned by the SAML user", responseObject = SuccessResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthenticator { public static final Logger s_logger = Logger.getLogger(ListAndSwitchSAMLAccountCmd.class.getName()); @@ -104,7 +107,9 @@ public class ListAndSwitchSAMLAccountCmd extends BaseCmd implements APIAuthentic params, responseType)); } - if (!HttpUtils.validateSessionKey(session, params, req.getCookies(), ApiConstants.SESSIONKEY)) { + HttpUtils.ApiSessionKeyCheckOption sessionKeyCheckOption = EnumUtils.getEnumIgnoreCase(HttpUtils.ApiSessionKeyCheckOption.class, + ApiServer.ApiSessionKeyCheckLocations.value(), HttpUtils.ApiSessionKeyCheckOption.CookieAndParameter); + if (!HttpUtils.validateSessionKey(session, params, req.getCookies(), ApiConstants.SESSIONKEY, sessionKeyCheckOption)) { throw new ServerApiException(ApiErrorCode.UNAUTHORIZED, _apiServer.getSerializedApiError(ApiErrorCode.UNAUTHORIZED.getHttpCode(), "Unauthorized session, please re-login", params, responseType)); diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java index 3a4030f9c0d..e10ea08012f 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java @@ -73,6 +73,9 @@ public interface SAML2AuthManager extends PluggableAPIAuthenticator, PluggableSe ConfigKey SAMLCheckSignature = new ConfigKey("Advanced", Boolean.class, "saml2.check.signature", "true", "When enabled (default and recommended), SAML2 signature checks are enforced and lack of signature in the SAML SSO response will cause login exception. Disabling this is not advisable but provided for backward compatibility for users who are able to accept the risks.", false); + ConfigKey SAMLUserSessionKeyPathAttribute = new ConfigKey("Advanced", String.class, "saml2.user.sessionkey.path", "", + "The Path attribute of sessionkey cookie when SAML users have logged in. If not set, it will be set to the path of SAML redirection URL (saml2.redirect.url).", true); + SAMLProviderMetadata getSPMetadata(); SAMLProviderMetadata getIdPMetadata(String entityId); Collection getAllIdPMetadata(); diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java index dfa76414fb7..aa1e0be91c7 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java @@ -542,6 +542,7 @@ public class SAML2AuthManagerImpl extends AdapterBase implements SAML2AuthManage SAMLServiceProviderSingleSignOnURL, SAMLServiceProviderSingleLogOutURL, SAMLCloudStackRedirectionUrl, SAMLUserAttributeName, SAMLIdentityProviderMetadataURL, SAMLDefaultIdentityProviderId, - SAMLSignatureAlgorithm, SAMLAppendDomainSuffix, SAMLTimeout, SAMLCheckSignature}; + SAMLSignatureAlgorithm, SAMLAppendDomainSuffix, SAMLTimeout, SAMLCheckSignature, + SAMLUserSessionKeyPathAttribute}; } } diff --git a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java index f10bc891368..bb94c8af4c2 100644 --- a/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java +++ b/plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java @@ -25,6 +25,8 @@ import java.io.IOException; import java.io.StringWriter; import java.io.UnsupportedEncodingException; import java.math.BigInteger; +import java.net.URI; +import java.net.URISyntaxException; import java.net.URLEncoder; import java.nio.charset.Charset; import java.security.InvalidKeyException; @@ -101,7 +103,9 @@ import org.w3c.dom.Document; import org.w3c.dom.Element; import org.xml.sax.SAXException; +import com.cloud.api.ApiServlet; import com.cloud.utils.HttpUtils; +import com.cloud.utils.exception.CloudRuntimeException; public class SAMLUtils { public static final Logger s_logger = Logger.getLogger(SAMLUtils.class); @@ -296,7 +300,26 @@ public class SAMLUtils { resp.addCookie(new Cookie("timezone", URLEncoder.encode(timezone, HttpUtils.UTF_8))); } resp.addCookie(new Cookie("userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20"))); - resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;Path=/client/api", ApiConstants.SESSIONKEY, loginResponse.getSessionKey())); + + String redirectUrl = SAML2AuthManager.SAMLCloudStackRedirectionUrl.value(); + String path = SAML2AuthManager.SAMLUserSessionKeyPathAttribute.value(); + String domain = null; + try { + URI redirectUri = new URI(redirectUrl); + domain = redirectUri.getHost(); + if (StringUtils.isBlank(path)) { + path = redirectUri.getPath(); + } + if (StringUtils.isBlank(path)) { + path = "/"; + } + } catch (URISyntaxException ex) { + throw new CloudRuntimeException("Invalid URI: " + redirectUrl); + } + String sameSite = ApiServlet.getApiSessionKeySameSite(); + String sessionKeyCookie = String.format("%s=%s;Domain=%s;Path=%s;%s", ApiConstants.SESSIONKEY, loginResponse.getSessionKey(), domain, path, sameSite); + s_logger.debug("Adding sessionkey cookie to response: " + sessionKeyCookie); + resp.addHeader("SET-COOKIE", sessionKeyCookie); } /** diff --git a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java index b4d230e3cd6..729334d22ce 100644 --- a/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java +++ b/plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/ListAndSwitchSAMLAccountCmdTest.java @@ -27,6 +27,7 @@ import java.net.InetAddress; import java.util.HashMap; import java.util.Map; +import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; @@ -88,6 +89,9 @@ public class ListAndSwitchSAMLAccountCmdTest extends TestCase { @Mock HttpServletRequest req; + final String sessionId = "node0xxxxxxxxxxxxx"; + Cookie[] cookies; + @Test public void testListAndSwitchSAMLAccountCmd() throws Exception { // Setup @@ -95,6 +99,7 @@ public class ListAndSwitchSAMLAccountCmdTest extends TestCase { final String sessionKeyValue = "someSessionIDValue"; Mockito.when(session.getAttribute(ApiConstants.SESSIONKEY)).thenReturn(sessionKeyValue); Mockito.when(session.getAttribute("userid")).thenReturn(2L); + Mockito.when(session.getId()).thenReturn(sessionId); params.put(ApiConstants.USER_ID, new String[]{"2"}); params.put(ApiConstants.DOMAIN_ID, new String[]{"1"}); Mockito.when(userDao.findByUuid(anyString())).thenReturn(new UserVO(2L)); @@ -146,7 +151,25 @@ public class ListAndSwitchSAMLAccountCmdTest extends TestCase { Mockito.verify(accountService, Mockito.times(0)).getUserAccountById(Mockito.anyLong()); } - // valid sessionkey value test + // valid sessionkey value and invalid JSESSIONID test + cookies = new Cookie[2]; + cookies[0] = new Cookie(ApiConstants.SESSIONKEY, sessionKeyValue); + cookies[1] = new Cookie("JSESSIONID", "invalid-JSESSIONID"); + Mockito.when(req.getCookies()).thenReturn(cookies); + params.put(ApiConstants.SESSIONKEY, new String[]{sessionKeyValue}); + try { + cmd.authenticate("command", params, session, null, HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); + } catch (ServerApiException exception) { + assertEquals(exception.getErrorCode(), ApiErrorCode.UNAUTHORIZED); + } finally { + Mockito.verify(accountService, Mockito.times(0)).getUserAccountById(Mockito.anyLong()); + } + + // valid sessionkey value and valid JSESSIONID test + cookies = new Cookie[2]; + cookies[0] = new Cookie(ApiConstants.SESSIONKEY, sessionKeyValue); + cookies[1] = new Cookie("JSESSIONID", sessionId + ".node0"); + Mockito.when(req.getCookies()).thenReturn(cookies); params.put(ApiConstants.SESSIONKEY, new String[]{sessionKeyValue}); try { cmd.authenticate("command", params, session, null, HttpUtils.RESPONSE_TYPE_JSON, new StringBuilder(), req, resp); diff --git a/server/src/main/java/com/cloud/api/ApiServer.java b/server/src/main/java/com/cloud/api/ApiServer.java index b1a04932cf9..d68f42470d5 100644 --- a/server/src/main/java/com/cloud/api/ApiServer.java +++ b/server/src/main/java/com/cloud/api/ApiServer.java @@ -33,6 +33,7 @@ import java.text.ParseException; import java.util.ArrayList; import java.util.Collections; import java.util.Date; +import java.util.EnumSet; import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; @@ -47,6 +48,7 @@ import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; @@ -168,6 +170,8 @@ import com.cloud.user.UserVO; import com.cloud.utils.ConstantTimeComparator; import com.cloud.utils.DateUtil; import com.cloud.utils.HttpUtils; +import com.cloud.utils.HttpUtils.ApiSessionKeySameSite; +import com.cloud.utils.HttpUtils.ApiSessionKeyCheckOption; import com.cloud.utils.Pair; import com.cloud.utils.ReflectUtil; import com.cloud.utils.StringUtils; @@ -306,6 +310,24 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer , true , ConfigKey.Scope.Global); + static final ConfigKey ApiSessionKeyCookieSameSiteSetting = new ConfigKey<>(String.class + , "api.sessionkey.cookie.samesite" + , ConfigKey.CATEGORY_ADVANCED + , ApiSessionKeySameSite.Lax.name() + , "The SameSite attribute of cookie 'sessionkey'. Valid options are: Lax (default), Strict, NoneAndSecure and Null." + , true + , ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.Select, + EnumSet.allOf(ApiSessionKeySameSite.class).stream().map(Enum::toString).collect(Collectors.joining(", "))); + + public static final ConfigKey ApiSessionKeyCheckLocations = new ConfigKey<>(String.class + , "api.sessionkey.check.locations" + , ConfigKey.CATEGORY_ADVANCED + , ApiSessionKeyCheckOption.CookieAndParameter.name() + , "The locations of 'sessionkey' during the validation of the API requests. Valid options are: CookieOrParameter, ParameterOnly, CookieAndParameter (default)." + , true + , ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.Select, + EnumSet.allOf(ApiSessionKeyCheckOption.class).stream().map(Enum::toString).collect(Collectors.joining(", "))); + @Override public boolean configure(final String name, final Map params) throws ConfigurationException { messageBus.subscribe(AsyncJob.Topics.JOB_EVENT_PUBLISH, MessageDispatcher.getDispatcher(this)); @@ -1531,7 +1553,9 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer JSONDefaultContentType, proxyForwardList, useForwardHeader, - listOfForwardHeaders + listOfForwardHeaders, + ApiSessionKeyCookieSameSiteSetting, + ApiSessionKeyCheckLocations }; } } diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index e719238afef..d9654f03916 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -47,6 +47,7 @@ import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.managed.context.ManagedContext; import org.apache.cloudstack.utils.consoleproxy.ConsoleAccessUtils; import org.apache.log4j.Logger; +import org.apache.commons.lang3.EnumUtils; import org.jetbrains.annotations.Nullable; import org.springframework.stereotype.Component; import org.springframework.web.context.support.SpringBeanAutowiringSupport; @@ -63,13 +64,15 @@ import com.cloud.user.User; import com.cloud.user.UserAccount; import com.cloud.utils.HttpUtils; +import com.cloud.utils.HttpUtils.ApiSessionKeySameSite; +import com.cloud.utils.HttpUtils.ApiSessionKeyCheckOption; import com.cloud.utils.StringUtils; import com.cloud.utils.db.EntityManager; import com.cloud.utils.net.NetUtils; @Component("apiServlet") public class ApiServlet extends HttpServlet { - public static final Logger s_logger = Logger.getLogger(ApiServlet.class.getName()); + public static final Logger LOGGER = Logger.getLogger(ApiServlet.class.getName()); private static final Logger s_accessLogger = Logger.getLogger("apiserver." + ApiServlet.class.getName()); private static final String REPLACEMENT = "_"; private static final String LOG_REPLACEMENTS = "[\n\r\t]"; @@ -127,7 +130,7 @@ public class ApiServlet extends HttpServlet { String value = decodeUtf8(paramTokens[1]); params.put(name, new String[] {value}); } else { - s_logger.debug("Invalid parameter in URL found. param: " + param); + LOGGER.debug("Invalid parameter in URL found. param: " + param); } } } @@ -156,7 +159,7 @@ public class ApiServlet extends HttpServlet { if (v.length > 1) { String message = String.format("Query parameter '%s' has multiple values %s. Only the last value will be respected." + "It is advised to pass only a single parameter", k, Arrays.toString(v)); - s_logger.warn(message); + LOGGER.warn(message); } }); @@ -167,7 +170,7 @@ public class ApiServlet extends HttpServlet { try { remoteAddress = getClientAddress(req); } catch (UnknownHostException e) { - s_logger.warn("UnknownHostException when trying to lookup remote IP-Address. This should never happen. Blocking request.", e); + LOGGER.warn("UnknownHostException when trying to lookup remote IP-Address. This should never happen. Blocking request.", e); final String response = apiServer.getSerializedApiError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "UnknownHostException when trying to lookup remote IP-Address", null, HttpUtils.RESPONSE_TYPE_XML); @@ -191,17 +194,17 @@ public class ApiServlet extends HttpServlet { // logging the request start and end in management log for easy debugging String reqStr = ""; String cleanQueryString = StringUtils.cleanString(req.getQueryString()); - if (s_logger.isDebugEnabled()) { + if (LOGGER.isDebugEnabled()) { reqStr = auditTrailSb.toString() + " " + cleanQueryString; - s_logger.debug("===START=== " + reqStr); + LOGGER.debug("===START=== " + reqStr); } try { resp.setContentType(HttpUtils.XML_CONTENT_TYPE); HttpSession session = req.getSession(false); - if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("session found: %s", session)); + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("session found: %s", session)); } final Object[] responseTypeParam = params.get(ApiConstants.RESPONSE); if (responseTypeParam != null) { @@ -212,10 +215,10 @@ public class ApiServlet extends HttpServlet { final String command = commandObj == null ? null : (String) commandObj[0]; final Object[] userObj = params.get(ApiConstants.USERNAME); String username = userObj == null ? null : (String)userObj[0]; - if (s_logger.isTraceEnabled()) { + if (LOGGER.isTraceEnabled()) { String logCommand = saveLogString(command); String logName = saveLogString(username); - s_logger.trace(String.format("command %s processing for user \"%s\"", + LOGGER.trace(String.format("command %s processing for user \"%s\"", logCommand, logName)); } @@ -238,25 +241,26 @@ public class ApiServlet extends HttpServlet { if (ApiServer.EnableSecureSessionCookie.value()) { resp.setHeader("SET-COOKIE", String.format("JSESSIONID=%s;Secure;HttpOnly;Path=/client", session.getId())); - if (s_logger.isDebugEnabled()) { - s_logger.debug("Session cookie is marked secure!"); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("Session cookie is marked secure!"); } } } try { - if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("apiAuthenticator.authenticate(%s, params[%d], %s, %s, %s, %s, %s,%s)", + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("apiAuthenticator.authenticate(%s, params[%d], %s, %s, %s, %s, %s,%s)", saveLogString(command), params.size(), session.getId(), remoteAddress.getHostAddress(), saveLogString(responseType), "auditTrailSb", "req", "resp")); } responseString = apiAuthenticator.authenticate(command, params, session, remoteAddress, responseType, auditTrailSb, req, resp); if (session != null && session.getAttribute(ApiConstants.SESSIONKEY) != null) { - resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, session.getAttribute(ApiConstants.SESSIONKEY))); + String sameSite = getApiSessionKeySameSite(); + resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;%s", ApiConstants.SESSIONKEY, session.getAttribute(ApiConstants.SESSIONKEY), sameSite)); } } catch (ServerApiException e) { httpResponseCode = e.getErrorCode().getHttpCode(); responseString = e.getMessage(); - s_logger.debug("Authentication failure: " + e.getMessage()); + LOGGER.debug("Authentication failure: " + e.getMessage()); } if (apiAuthenticator.getAPIType() == APIAuthenticationType.LOGOUT_API) { @@ -289,7 +293,7 @@ public class ApiServlet extends HttpServlet { return; } } else { - s_logger.trace("no command available"); + LOGGER.trace("no command available"); } auditTrailSb.append(cleanQueryString); final boolean isNew = ((session == null) ? true : session.isNew()); @@ -298,15 +302,15 @@ public class ApiServlet extends HttpServlet { // we no longer rely on web-session here, verifyRequest will populate user/account information // if a API key exists - if (isNew && s_logger.isTraceEnabled()) { - s_logger.trace(String.format("new session: %s", session)); + if (isNew && LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("new session: %s", session)); } if (!isNew && (command.equalsIgnoreCase(ValidateUserTwoFactorAuthenticationCodeCmd.APINAME) || (!skip2FAcheckForAPIs(command) && !skip2FAcheckForUser(session)))) { - s_logger.debug("Verifying two factor authentication"); + LOGGER.debug("Verifying two factor authentication"); boolean success = verify2FA(session, command, auditTrailSb, params, remoteAddress, responseType, req, resp); if (!success) { - s_logger.debug("Verification of two factor authentication failed"); + LOGGER.debug("Verification of two factor authentication failed"); return; } } @@ -319,8 +323,8 @@ public class ApiServlet extends HttpServlet { if (account != null) { if (invalidateHttpSessionIfNeeded(req, resp, auditTrailSb, responseType, params, session, account)) return; } else { - if (s_logger.isDebugEnabled()) { - s_logger.debug("no account, this request will be validated through apikey(%s)/signature"); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("no account, this request will be validated through apikey(%s)/signature"); } } @@ -330,8 +334,8 @@ public class ApiServlet extends HttpServlet { CallContext.register(accountMgr.getSystemUser(), accountMgr.getSystemAccount()); } setProjectContext(params); - if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("verifying request for user %s from %s with %d parameters", + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("verifying request for user %s from %s with %d parameters", userId, remoteAddress.getHostAddress(), params.size())); } if (apiServer.verifyRequest(params, userId, remoteAddress)) { @@ -362,18 +366,34 @@ public class ApiServlet extends HttpServlet { HttpUtils.writeHttpResponse(resp, serializedResponseText, se.getErrorCode().getHttpCode(), responseType, ApiServer.JSONcontentType.value()); auditTrailSb.append(" " + se.getErrorCode() + " " + se.getDescription()); } catch (final Exception ex) { - s_logger.error("unknown exception writing api response", ex); + LOGGER.error("unknown exception writing api response", ex); auditTrailSb.append(" unknown exception writing api response"); } finally { s_accessLogger.info(auditTrailSb.toString()); - if (s_logger.isDebugEnabled()) { - s_logger.debug("===END=== " + reqStr); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug("===END=== " + reqStr); } // cleanup user context to prevent from being peeked in other request context CallContext.unregister(); } } + public static String getApiSessionKeySameSite() { + ApiSessionKeySameSite sameSite = EnumUtils.getEnumIgnoreCase(ApiSessionKeySameSite.class, + ApiServer.ApiSessionKeyCookieSameSiteSetting.value(), ApiSessionKeySameSite.Lax); + switch (sameSite) { + case Strict: + return "SameSite=Strict"; + case NoneAndSecure: + return "SameSite=None;Secure"; + case Null: + return ""; + case Lax: + default: + return "SameSite=Lax"; + } + } + private boolean checkIfAuthenticatorIsOf2FA(String command) { boolean verify2FA = false; APIAuthenticator apiAuthenticator = authManager.getAPIAuthenticator(command); @@ -402,7 +422,7 @@ public class ApiServlet extends HttpServlet { Long userId = (Long) session.getAttribute("userid"); boolean is2FAverified = (boolean) session.getAttribute(ApiConstants.IS_2FA_VERIFIED); if (is2FAverified) { - s_logger.debug(String.format("Two factor authentication is already verified for the user %d, so skipping", userId)); + LOGGER.debug(String.format("Two factor authentication is already verified for the user %d, so skipping", userId)); skip2FAcheck = true; } else { UserAccount userAccount = accountMgr.getUserAccountById(userId); @@ -433,7 +453,7 @@ public class ApiServlet extends HttpServlet { HttpUtils.writeHttpResponse(resp, responseString, HttpServletResponse.SC_OK, responseType, ApiServer.JSONcontentType.value()); verify2FA = true; } else { - s_logger.error("Cannot find API authenticator while verifying 2FA"); + LOGGER.error("Cannot find API authenticator while verifying 2FA"); auditTrailSb.append(" Cannot find API authenticator while verifying 2FA"); verify2FA = false; } @@ -457,7 +477,7 @@ public class ApiServlet extends HttpServlet { errorMsg = "Two factor authentication is mandated by admin, user needs to setup 2FA using setupUserTwoFactorAuthentication API and" + " then verify 2FA using validateUserTwoFactorAuthenticationCode API before calling other APIs. Existing session is invalidated."; } - s_logger.error(errorMsg); + LOGGER.error(errorMsg); invalidateHttpSession(session, String.format("Unable to process the API request for %s from %s due to %s", userId, remoteAddress.getHostAddress(), errorMsg)); auditTrailSb.append(" " + ApiErrorCode.UNAUTHORIZED2FA + " " + errorMsg); @@ -488,7 +508,7 @@ public class ApiServlet extends HttpServlet { private boolean requestChecksoutAsSane(HttpServletResponse resp, StringBuilder auditTrailSb, String responseType, Map params, HttpSession session, String command, Long userId, String account, Object accountObj) { if ((userId != null) && (account != null) && (accountObj != null) && apiServer.verifyUser(userId)) { if (command == null) { - s_logger.info("missing command, ignoring request..."); + LOGGER.info("missing command, ignoring request..."); auditTrailSb.append(" " + HttpServletResponse.SC_BAD_REQUEST + " " + "no command specified"); final String serializedResponse = apiServer.getSerializedApiError(HttpServletResponse.SC_BAD_REQUEST, "no command specified", params, responseType); HttpUtils.writeHttpResponse(resp, serializedResponse, HttpServletResponse.SC_BAD_REQUEST, responseType, ApiServer.JSONcontentType.value()); @@ -509,7 +529,9 @@ public class ApiServlet extends HttpServlet { } private boolean invalidateHttpSessionIfNeeded(HttpServletRequest req, HttpServletResponse resp, StringBuilder auditTrailSb, String responseType, Map params, HttpSession session, String account) { - if (!HttpUtils.validateSessionKey(session, params, req.getCookies(), ApiConstants.SESSIONKEY)) { + ApiSessionKeyCheckOption sessionKeyCheckOption = EnumUtils.getEnumIgnoreCase(ApiSessionKeyCheckOption.class, + ApiServer.ApiSessionKeyCheckLocations.value(), ApiSessionKeyCheckOption.CookieAndParameter); + if (!HttpUtils.validateSessionKey(session, params, req.getCookies(), ApiConstants.SESSIONKEY, sessionKeyCheckOption)) { String msg = String.format("invalidating session %s for account %s", session.getId(), account); invalidateHttpSession(session, msg); auditTrailSb.append(" " + HttpServletResponse.SC_UNAUTHORIZED + " " + "unable to verify user credentials"); @@ -523,13 +545,13 @@ public class ApiServlet extends HttpServlet { public static void invalidateHttpSession(HttpSession session, String msg) { try { - if (s_logger.isTraceEnabled()) { - s_logger.trace(msg); + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(msg); } session.invalidate(); } catch (final IllegalStateException ise) { - if (s_logger.isTraceEnabled()) { - s_logger.trace(String.format("failed to invalidate session %s", session.getId())); + if (LOGGER.isTraceEnabled()) { + LOGGER.trace(String.format("failed to invalidate session %s", session.getId())); } } } @@ -537,7 +559,7 @@ public class ApiServlet extends HttpServlet { private void setProjectContext(Map requestParameters) { final String[] command = (String[])requestParameters.get(ApiConstants.COMMAND); if (command == null) { - s_logger.info("missing command, ignoring request..."); + LOGGER.info("missing command, ignoring request..."); return; } @@ -585,14 +607,14 @@ public class ApiServlet extends HttpServlet { header = header.trim(); ip = getCorrectIPAddress(request.getHeader(header)); if (StringUtils.isNotBlank(ip)) { - s_logger.debug(String.format("found ip %s in header %s ", ip, header)); + LOGGER.debug(String.format("found ip %s in header %s ", ip, header)); break; } } // no address found in header so ip is blank and use remote addr } // else not an allowed proxy address, ip is blank and use remote addr } if (StringUtils.isBlank(ip)) { - s_logger.trace(String.format("no ip found in headers, returning remote address %s.", pretender.getHostAddress())); + LOGGER.trace(String.format("no ip found in headers, returning remote address %s.", pretender.getHostAddress())); return pretender; } diff --git a/test/integration/smoke/test_login.py b/test/integration/smoke/test_login.py index 40d8349a13d..acd6c4334ac 100644 --- a/test/integration/smoke/test_login.py +++ b/test/integration/smoke/test_login.py @@ -133,6 +133,7 @@ class TestLogin(cloudstackTestCase): args["command"] = 'listUsers' args["listall"] = 'true' args["response"] = "json" + args["sessionkey"] = response.json()['loginresponse']['sessionkey'] response = session.get(self.server_url, params=args) self.assertEqual( response.status_code, diff --git a/ui/src/api/index.js b/ui/src/api/index.js index 14432010738..7ab87780a9d 100644 --- a/ui/src/api/index.js +++ b/ui/src/api/index.js @@ -15,8 +15,13 @@ // specific language governing permissions and limitations // under the License. +import Cookies from 'js-cookie' import { axios, sourceToken } from '@/utils/request' import { message, notification } from 'ant-design-vue' +import { vueProps } from '@/vue-app' +import { + ACCESS_TOKEN +} from '@/store/mutation-types' export function api (command, args = {}, method = 'GET', data = {}) { let params = {} @@ -30,6 +35,11 @@ export function api (command, args = {}, method = 'GET', data = {}) { }) } + const sessionkey = vueProps.$localStorage.get(ACCESS_TOKEN) || Cookies.get('sessionkey') + if (sessionkey) { + args.sessionkey = sessionkey + } + return axios({ params: { ...args diff --git a/ui/src/store/modules/user.js b/ui/src/store/modules/user.js index 08a0c340c64..46a1905619f 100644 --- a/ui/src/store/modules/user.js +++ b/ui/src/store/modules/user.js @@ -24,7 +24,6 @@ import router from '@/router' import store from '@/store' import { oauthlogin, login, logout, api } from '@/api' import { i18n } from '@/locales' -import { sourceToken } from '@/utils/request' import { ACCESS_TOKEN, diff --git a/utils/src/main/java/com/cloud/utils/HttpUtils.java b/utils/src/main/java/com/cloud/utils/HttpUtils.java index a5d9f6a16b6..cc97bf4ba15 100644 --- a/utils/src/main/java/com/cloud/utils/HttpUtils.java +++ b/utils/src/main/java/com/cloud/utils/HttpUtils.java @@ -37,6 +37,14 @@ public class HttpUtils { public static final String JSON_CONTENT_TYPE = "application/json; charset=UTF-8"; public static final String XML_CONTENT_TYPE = "text/xml; charset=UTF-8"; + public enum ApiSessionKeySameSite { + Lax, Strict, NoneAndSecure, Null + } + + public enum ApiSessionKeyCheckOption { + CookieOrParameter, ParameterOnly, CookieAndParameter + } + public static void addSecurityHeaders(final HttpServletResponse resp) { if (resp.containsHeader("X-Content-Type-Options")) { resp.setHeader("X-Content-Type-Options", "nosniff"); @@ -103,23 +111,43 @@ public class HttpUtils { return null; } - public static boolean validateSessionKey(final HttpSession session, final Map params, final Cookie[] cookies, final String sessionKeyString) { + public static boolean validateSessionKey(final HttpSession session, final Map params, final Cookie[] cookies, final String sessionKeyString, final ApiSessionKeyCheckOption apiSessionKeyCheckLocations) { if (session == null || sessionKeyString == null) { return false; } + final String jsessionidFromCookie = HttpUtils.findCookie(cookies, "JSESSIONID"); + if (jsessionidFromCookie == null + || !(jsessionidFromCookie.startsWith(session.getId() + '.'))) { + s_logger.error("JSESSIONID from cookie is invalid."); + return false; + } final String sessionKey = (String) session.getAttribute(sessionKeyString); + if (sessionKey == null) { + s_logger.error("sessionkey attribute of the session is null."); + return false; + } final String sessionKeyFromCookie = HttpUtils.findCookie(cookies, sessionKeyString); + boolean isSessionKeyFromCookieValid = sessionKeyFromCookie != null && sessionKey.equals(sessionKeyFromCookie); + String[] sessionKeyFromParams = null; if (params != null) { sessionKeyFromParams = (String[]) params.get(sessionKeyString); } - if ((sessionKey == null) - || (sessionKeyFromParams == null && sessionKeyFromCookie == null) - || (sessionKeyFromParams != null && !sessionKey.equals(sessionKeyFromParams[0])) - || (sessionKeyFromCookie != null && !sessionKey.equals(sessionKeyFromCookie))) { - return false; + boolean isSessionKeyFromParamsValid = sessionKeyFromParams != null && sessionKey.equals(sessionKeyFromParams[0]); + + switch (apiSessionKeyCheckLocations) { + case CookieOrParameter: + return (sessionKeyFromCookie != null || sessionKeyFromParams != null) + && (sessionKeyFromCookie == null || isSessionKeyFromCookieValid) + && (sessionKeyFromParams == null || isSessionKeyFromParamsValid); + case ParameterOnly: + return sessionKeyFromParams != null && isSessionKeyFromParamsValid + && (sessionKeyFromCookie == null || isSessionKeyFromCookieValid); + case CookieAndParameter: + default: + return sessionKeyFromCookie != null && isSessionKeyFromCookieValid + && sessionKeyFromParams != null && isSessionKeyFromParamsValid; } - return true; } } diff --git a/utils/src/test/java/com/cloud/utils/HttpUtilsTest.java b/utils/src/test/java/com/cloud/utils/HttpUtilsTest.java index e10a5a36b27..e94724ce3d6 100644 --- a/utils/src/test/java/com/cloud/utils/HttpUtilsTest.java +++ b/utils/src/test/java/com/cloud/utils/HttpUtilsTest.java @@ -60,35 +60,97 @@ public class HttpUtilsTest { final String sessionKeyValue = "randomUniqueSessionID"; // session and sessionKeyString null test - assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); sessionKeyString = "sessionkey"; - assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); // param and cookie null test session = new MockHttpSession(); + final String sessionId = session.getId(); session.setAttribute(sessionKeyString, sessionKeyValue); - assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); - // param null, cookies not null test + // param null, cookies not null test (JSESSIONID is null) params = null; cookies = new Cookie[]{new Cookie(sessionKeyString, sessionKeyValue)}; - assertFalse(HttpUtils.validateSessionKey(session, params, cookies, "randomString")); - assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, "randomString", HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + + // param null, cookies not null test (JSESSIONID is not null and matches) + cookies = new Cookie[2]; + cookies[0] = new Cookie(sessionKeyString, sessionKeyValue); + cookies[1] = new Cookie("JSESSIONID", sessionId + ".node0"); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, "randomString", HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + + // param null, cookies not null test (JSESSIONID is not null but mismatches) + cookies = new Cookie[2]; + cookies[0] = new Cookie(sessionKeyString, sessionKeyValue); + cookies[1] = new Cookie("JSESSIONID", "node0xxxxxxxxxxxxx.node0"); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, "randomString", HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); // param not null, cookies null test params = new HashMap(); params.put(sessionKeyString, new String[]{"randomString"}); cookies = null; - assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); params.put(sessionKeyString, new String[]{sessionKeyValue}); - assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); - // both param and cookies not null test + // both param and cookies not null test (JSESSIONID is null) params = new HashMap(); - cookies = new Cookie[]{new Cookie(sessionKeyString, sessionKeyValue)}; + cookies = new Cookie[2]; + cookies[0] = new Cookie(sessionKeyString, sessionKeyValue); params.put(sessionKeyString, new String[]{"incorrectValue"}); - assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); params.put(sessionKeyString, new String[]{sessionKeyValue}); - assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + + // both param and cookies not null test (JSESSIONID is not null but mismatches) + params = new HashMap(); + cookies = new Cookie[2]; + cookies[0] = new Cookie(sessionKeyString, sessionKeyValue); + cookies[1] = new Cookie("JSESSIONID", "node0xxxxxxxxxxxxx.node0"); + params.put(sessionKeyString, new String[]{"incorrectValue"}); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + params.put(sessionKeyString, new String[]{sessionKeyValue}); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + + // both param and cookies not null test (JSESSIONID is not null amd matches) + params = new HashMap(); + cookies = new Cookie[2]; + cookies[0] = new Cookie(sessionKeyString, sessionKeyValue); + cookies[1] = new Cookie("JSESSIONID", sessionId + ".node0"); + params.put(sessionKeyString, new String[]{"incorrectValue"}); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + params.put(sessionKeyString, new String[]{sessionKeyValue}); + assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + + // param not null, cookies null test (JSESSIONID is not null amd matches) + params = new HashMap(); + cookies = new Cookie[1]; + cookies[0] = new Cookie("JSESSIONID", sessionId + ".node0"); + params.put(sessionKeyString, new String[]{"incorrectValue"}); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.ParameterOnly)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieAndParameter)); + params.put(sessionKeyString, new String[]{sessionKeyValue}); + assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.ParameterOnly)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieAndParameter)); + + // param not null (correct), cookies not null test (correct) + cookies = new Cookie[2]; + cookies[0] = new Cookie(sessionKeyString, sessionKeyValue); + cookies[1] = new Cookie("JSESSIONID", sessionId + ".node0"); + assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.ParameterOnly)); + assertTrue(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieAndParameter)); + + // param not null (correct), cookies not null test (wrong) + cookies = new Cookie[2]; + cookies[0] = new Cookie(sessionKeyString, "incorrectValue"); + cookies[1] = new Cookie("JSESSIONID", sessionId + ".node0"); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieOrParameter)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.ParameterOnly)); + assertFalse(HttpUtils.validateSessionKey(session, params, cookies, sessionKeyString, HttpUtils.ApiSessionKeyCheckOption.CookieAndParameter)); } } From 0602f46d82b1ffd1732dda5d0d39b1d8e95a32fb Mon Sep 17 00:00:00 2001 From: Daniel Augusto Veronezi Salvador Date: Tue, 17 Sep 2024 16:08:23 -0300 Subject: [PATCH 9/9] Fix Vue devServer after CSRF fix --- ui/vue.config.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ui/vue.config.js b/ui/vue.config.js index c74218b4bcf..9cae2ff66fb 100644 --- a/ui/vue.config.js +++ b/ui/vue.config.js @@ -142,7 +142,11 @@ const vueConfig = { secure: false, ws: false, changeOrigin: true, - proxyTimeout: 10 * 60 * 1000 // 10 minutes + proxyTimeout: 10 * 60 * 1000, // 10 minutes + cookieDomainRewrite: '*', + cookiePathRewrite: { + '/client': '/' + } } }, https: process.env.HTTPS_KEY ? {