Cloudstack 10170: Fix resource tags security bugs and add account tags support (#2350)

This PR introduces several features and fixes some bugs:
- account tags feature
- fixed resource tags bugs which happened during tags search (found wrong entries because of mysql string to number translation - see #905, but this PR does more and fixes also resource access - vulnerability during list resource tags)
- some marvin improvements (speed, sanity)

Improved resource tags code:
1. Enhanced listTags security
2. Added support for account tags (account tags are required to support tags common for all users of an account)
3. Improved the tag management code (refactoring and cleanup)

Marvin:
1. Fixed Marvin wait timeout between async pools. To decrease polling interval and improve CI speed.
2. Fixed /tmp/ to /tmp in zone configuration files.
3. Fixed + to os.path.join in log class.
4. Fixed + to os.path.join in deployDataCenter class.
5. Fixed typos in tag tests.
6. Modified Tags base class delete method.

Deploy Datacenter script:
1. Improved deployDatacenter. Added option logdir to specify where script places results of evaluation.

ConfigurationManagerImpl:
1. Added logging to ConfigurationManagerImpl to log when vlan is not found. Added test stubs for tags. Found accidental exception during simulator running after CI.

tests_tags.py:
1. Fixed stale undeleted tags.
2. Changed region:India to scope:TestName.

Author: bwsw

api/src/com/cloud/server/ResourceTag.java

@@ -38,6 +38,7 @@ public enum ResourceObjectType {
         SecurityGroupRule(true, false),
         PublicIpAddress(true, true),
         Project(true, false),
+        Account(true, false),
         Vpc(true, true),
         NetworkACL(true, true),
         StaticRoute(true, false),

server/src/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -3804,6 +3804,9 @@ public boolean releasePublicIpRange(final ReleasePublicIpRangeCmd cmd) {
     @DB
     public boolean releasePublicIpRange(final long vlanDbId, final long userId, final Account caller) {
         VlanVO vlan = _vlanDao.findById(vlanDbId);
+        if(vlan == null) {
+            s_logger.warn("VLAN information for Account '" + caller + "', User '" + userId + "' VLAN '" + vlanDbId + "' is null. This is NPE situation.");
+        }
 
         // Verify range is dedicated
         boolean isAccountSpecific = false;

server/src/com/cloud/tags/TaggedResourceManagerImpl.java

@@ -16,28 +16,13 @@
 // under the License.
 package com.cloud.tags;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
-import javax.inject.Inject;
-import javax.naming.ConfigurationException;
-
-import org.apache.cloudstack.api.Identity;
-import org.apache.cloudstack.api.InternalIdentity;
-import org.apache.cloudstack.context.CallContext;
-import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
-import org.apache.commons.lang.StringUtils;
-import org.apache.log4j.Logger;
-
-import com.cloud.api.query.dao.ResourceTagJoinDao;
 import com.cloud.dc.DataCenterVO;
 import com.cloud.domain.PartOf;
 import com.cloud.event.ActionEvent;
 import com.cloud.event.EventTypes;
 import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.exception.PermissionDeniedException;
+import com.cloud.offerings.NetworkOfferingVO;
 import com.cloud.network.LBHealthCheckPolicyVO;
 import com.cloud.network.as.AutoScaleVmGroupVO;
 import com.cloud.network.as.AutoScaleVmProfileVO;
@@ -58,7 +43,6 @@
 import com.cloud.network.vpc.StaticRouteVO;
 import com.cloud.network.vpc.VpcOfferingVO;
 import com.cloud.network.vpc.VpcVO;
-import com.cloud.offerings.NetworkOfferingVO;
 import com.cloud.projects.ProjectVO;
 import com.cloud.server.ResourceTag;
 import com.cloud.server.ResourceTag.ResourceObjectType;
@@ -72,6 +56,7 @@
 import com.cloud.tags.dao.ResourceTagDao;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
+import com.cloud.user.AccountVO;
 import com.cloud.user.DomainManager;
 import com.cloud.user.OwnedBy;
 import com.cloud.user.UserVO;
@@ -89,11 +74,28 @@
 import com.cloud.vm.NicVO;
 import com.cloud.vm.UserVmVO;
 import com.cloud.vm.snapshot.VMSnapshotVO;
+import org.apache.cloudstack.api.Identity;
+import org.apache.cloudstack.api.InternalIdentity;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.commons.lang.StringUtils;
+import org.apache.log4j.Logger;
+
+import org.apache.commons.collections.MapUtils;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
 
 public class TaggedResourceManagerImpl extends ManagerBase implements TaggedResourceService {
     public static final Logger s_logger = Logger.getLogger(TaggedResourceManagerImpl.class);
 
-    private static final Map<ResourceObjectType, Class<?>> s_typeMap = new HashMap<ResourceObjectType, Class<?>>();
+    private static final Map<ResourceObjectType, Class<?>> s_typeMap = new HashMap<>();
     static {
         s_typeMap.put(ResourceObjectType.UserVm, UserVmVO.class);
         s_typeMap.put(ResourceObjectType.Volume, VolumeVO.class);
@@ -108,6 +110,7 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
         s_typeMap.put(ResourceObjectType.SecurityGroupRule, SecurityGroupRuleVO.class);
         s_typeMap.put(ResourceObjectType.PublicIpAddress, IPAddressVO.class);
         s_typeMap.put(ResourceObjectType.Project, ProjectVO.class);
+        s_typeMap.put(ResourceObjectType.Account, AccountVO.class);
         s_typeMap.put(ResourceObjectType.Vpc, VpcVO.class);
         s_typeMap.put(ResourceObjectType.Nic, NicVO.class);
         s_typeMap.put(ResourceObjectType.NetworkACL, NetworkACLItemVO.class);
@@ -140,8 +143,6 @@ public class TaggedResourceManagerImpl extends ManagerBase implements TaggedReso
     @Inject
     ResourceTagDao _resourceTagDao;
     @Inject
-    ResourceTagJoinDao _resourceTagJoinDao;
-    @Inject
     DomainManager _domainMgr;
     @Inject
     AccountDao _accountDao;
@@ -194,6 +195,12 @@ private Pair<Long, Long> getAccountDomain(long resourceId, ResourceObjectType re
             domainId = ((SecurityGroupVO)SecurityGroup).getDomainId();
         }
 
+        if (resourceType == ResourceObjectType.Account) {
+            AccountVO account = (AccountVO)entity;
+            accountId = account.getId();
+            domainId = account.getDomainId();
+        }
+
         // if the resource type is network acl, get the accountId and domainId from VPC following: NetworkACLItem -> NetworkACL -> VPC
         if (resourceType == ResourceObjectType.NetworkACL) {
             NetworkACLItemVO aclItem = (NetworkACLItemVO)entity;
@@ -223,7 +230,23 @@ private Pair<Long, Long> getAccountDomain(long resourceId, ResourceObjectType re
         if ((domainId == null) || ((accountId != null) && (domainId.longValue() == -1))) {
             domainId = _accountDao.getDomainIdForGivenAccountId(accountId);
         }
-        return new Pair<Long, Long>(accountId, domainId);
+        return new Pair<>(accountId, domainId);
+    }
+
+    private void checkResourceAccessible(Long accountId, Long domainId, String exceptionMessage) {
+        Account caller = CallContext.current().getCallingAccount();
+        if (Objects.equals(domainId, -1))
+        {
+            throw new CloudRuntimeException("Invalid DomainId: -1");
+        }
+        if (accountId != null) {
+            _accountMgr.checkAccess(caller, null, false, _accountMgr.getAccount(accountId));
+        } else if (domainId != null && !_accountMgr.isNormalUser(caller.getId())) {
+            //check permissions;
+            _accountMgr.checkAccess(caller, _domainMgr.getDomain(domainId));
+        } else {
+            throw new PermissionDeniedException(exceptionMessage);
+        }
     }
 
     @Override
@@ -237,13 +260,29 @@ public ResourceObjectType getResourceType(String resourceTypeStr) {
         throw new InvalidParameterValueException("Invalid resource type " + resourceTypeStr);
     }
 
+    @Override
+    public String getUuid(String resourceId, ResourceObjectType resourceType) {
+        if (!StringUtils.isNumeric(resourceId)) {
+            return resourceId;
+        }
+
+        Class<?> clazz = s_typeMap.get(resourceType);
+
+        Object entity = _entityMgr.findById(clazz, resourceId);
+        if (entity != null && entity instanceof Identity) {
+            return ((Identity)entity).getUuid();
+        }
+
+        return resourceId;
+    }
+
     @Override
     @DB
     @ActionEvent(eventType = EventTypes.EVENT_TAGS_CREATE, eventDescription = "creating resource tags")
     public List<ResourceTag> createTags(final List<String> resourceIds, final ResourceObjectType resourceType, final Map<String, String> tags, final String customer) {
         final Account caller = CallContext.current().getCallingAccount();
 
-        final List<ResourceTag> resourceTags = new ArrayList<ResourceTag>(tags.size());
+        final List<ResourceTag> resourceTags = new ArrayList<>(tags.size());
 
         Transaction.execute(new TransactionCallbackNoReturn() {
             @Override
@@ -261,17 +300,8 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
                         Long domainId = accountDomainPair.second();
                         Long accountId = accountDomainPair.first();
 
-                        if ((domainId != null) && (domainId == -1)) {
-                            throw new CloudRuntimeException("Invalid DomainId : -1");
-                        }
-                        if (accountId != null) {
-                            _accountMgr.checkAccess(caller, null, false, _accountMgr.getAccount(accountId));
-                        } else if (domainId != null && !_accountMgr.isNormalUser(caller.getId())) {
-                            //check permissions;
-                            _accountMgr.checkAccess(caller, _domainMgr.getDomain(domainId));
-                        } else {
-                            throw new PermissionDeniedException("Account " + caller + " doesn't have permissions to create tags" + " for resource " + key);
-                        }
+                        checkResourceAccessible(accountId, domainId, "Account '" + caller +
+                                "' doesn't have permissions to create tags" + " for resource '" + id + "(" + key + ")'.");
 
                         String value = tags.get(key);
 
@@ -290,83 +320,77 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
         return resourceTags;
     }
 
-    @Override
-    public String getUuid(String resourceId, ResourceObjectType resourceType) {
-        if (!StringUtils.isNumeric(resourceId)) {
-            return resourceId;
-        }
-
-        Class<?> clazz = s_typeMap.get(resourceType);
-
-        Object entity = _entityMgr.findById(clazz, resourceId);
-        if (entity != null && entity instanceof Identity) {
-            return ((Identity)entity).getUuid();
-        }
+    private List<? extends ResourceTag> searchResourceTags(List<String> resourceIds, ResourceObjectType resourceType) {
+        List<String> resourceUuids = resourceIds.stream().map(resourceId -> getUuid(resourceId, resourceType)).collect(Collectors.toList());
+        SearchBuilder<ResourceTagVO> sb = _resourceTagDao.createSearchBuilder();
+        sb.and("resourceUuid", sb.entity().getResourceUuid(), SearchCriteria.Op.IN);
+        sb.and("resourceType", sb.entity().getResourceType(), SearchCriteria.Op.EQ);
 
-        return resourceId;
+        SearchCriteria<ResourceTagVO> sc = sb.create();
+        sc.setParameters("resourceUuid", resourceUuids.toArray());
+        sc.setParameters("resourceType", resourceType);
+        return _resourceTagDao.search(sc, null);
     }
 
     @Override
     @DB
     @ActionEvent(eventType = EventTypes.EVENT_TAGS_DELETE, eventDescription = "deleting resource tags")
     public boolean deleteTags(List<String> resourceIds, ResourceObjectType resourceType, Map<String, String> tags) {
         Account caller = CallContext.current().getCallingAccount();
-
-        SearchBuilder<ResourceTagVO> sb = _resourceTagDao.createSearchBuilder();
-        sb.and().op("resourceId", sb.entity().getResourceId(), SearchCriteria.Op.IN);
-        sb.or("resourceUuid", sb.entity().getResourceUuid(), SearchCriteria.Op.IN);
-        sb.cp();
-        sb.and("resourceType", sb.entity().getResourceType(), SearchCriteria.Op.EQ);
-
-        SearchCriteria<ResourceTagVO> sc = sb.create();
-        sc.setParameters("resourceId", resourceIds.toArray());
-        sc.setParameters("resourceUuid", resourceIds.toArray());
-        sc.setParameters("resourceType", resourceType);
-
-        List<? extends ResourceTag> resourceTags = _resourceTagDao.search(sc, null);
-        ;
-        final List<ResourceTag> tagsToRemove = new ArrayList<ResourceTag>();
+        if(s_logger.isDebugEnabled()) {
+            s_logger.debug("ResourceIds to Find " + String.join(", ", resourceIds));
+        }
+        List<? extends ResourceTag> resourceTags = searchResourceTags(resourceIds, resourceType);
+        final List<ResourceTag> tagsToDelete = new ArrayList<>();
 
         // Finalize which tags should be removed
         for (ResourceTag resourceTag : resourceTags) {
             //1) validate the permissions
+            if(s_logger.isDebugEnabled()) {
+                s_logger.debug("Resource Tag Id: " + resourceTag.getResourceId());
+                s_logger.debug("Resource Tag AccountId: " + resourceTag.getAccountId());
+            }
             Account owner = _accountMgr.getAccount(resourceTag.getAccountId());
+            if(s_logger.isDebugEnabled()) {
+                s_logger.debug("Resource Owner: " + owner);
+            }
             _accountMgr.checkAccess(caller, null, false, owner);
             //2) Only remove tag if it matches key value pairs
-            if (tags != null && !tags.isEmpty()) {
+            if (MapUtils.isEmpty(tags)) {
+                tagsToDelete.add(resourceTag);
+            } else {
                 for (String key : tags.keySet()) {
-                    boolean canBeRemoved = false;
+                    boolean deleteTag = false;
                     if (resourceTag.getKey().equalsIgnoreCase(key)) {
                         String value = tags.get(key);
                         if (value != null) {
                             if (resourceTag.getValue().equalsIgnoreCase(value)) {
-                                canBeRemoved = true;
+                                deleteTag = true;
                             }
                         } else {
-                            canBeRemoved = true;
+                            deleteTag = true;
                         }
-                        if (canBeRemoved) {
-                            tagsToRemove.add(resourceTag);
+                        if (deleteTag) {
+                            tagsToDelete.add(resourceTag);
                             break;
                         }
                     }
                 }
-            } else {
-                tagsToRemove.add(resourceTag);
             }
         }
 
-        if (tagsToRemove.isEmpty()) {
-            throw new InvalidParameterValueException("Unable to find tags by parameters specified");
+        if (tagsToDelete.isEmpty()) {
+            throw new InvalidParameterValueException("Unable to find any tags which conform to specified delete parameters.");
         }
 
         //Remove the tags
         Transaction.execute(new TransactionCallbackNoReturn() {
             @Override
             public void doInTransactionWithoutResult(TransactionStatus status) {
-                for (ResourceTag tagToRemove : tagsToRemove) {
+                for (ResourceTag tagToRemove : tagsToDelete) {
                     _resourceTagDao.remove(tagToRemove.getId());
-                    s_logger.debug("Removed the tag " + tagToRemove);
+                    s_logger.debug("Removed the tag '" + tagToRemove + "' for resources (" +
+                            String.join(", ", resourceIds) + ")");
                 }
             }
         });
@@ -375,7 +399,7 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
     }
 
     @Override
-    public List<? extends ResourceTag> listByResourceTypeAndId(ResourceObjectType type, long resourceId) {
-        return _resourceTagDao.listBy(resourceId, type);
+    public List<? extends ResourceTag> listByResourceTypeAndId(ResourceObjectType resourceType, long resourceId) {
+        return _resourceTagDao.listBy(resourceId, resourceType);
     }
 }

setup/dev/advanced.cfg

@@ -154,7 +154,7 @@
     },
     "logger":
         {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
         },
     "globalConfig": [
         {

setup/dev/advancedsg.cfg

@@ -109,7 +109,7 @@
     },
     "logger": 
         {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
         },
     "globalConfig": [
         {

setup/dev/basic.cfg

@@ -110,7 +110,7 @@
     },
     "logger": 
         {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
         },
     "globalConfig": [
         {

setup/dev/local.cfg

@@ -25,7 +25,7 @@
     },
     "logger":
     {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
     },
     "mgtSvr": [
         {

setup/dev/s3.cfg

@@ -115,7 +115,7 @@
     ],
     "logger":
     {
-            "LogFolderPath": "/tmp/"
+            "LogFolderPath": "/tmp"
     },
     "mgtSvr": [
         {