User data content size validation, and related code improvements (apache#8418)

sureshanaparti · JoaoJandre · dhslove · commit 106989c9d652 · 2024-06-24T14:02:20.000+09:00
Co-authored-by: João Jandre &lt;48719461+JoaoJandre@users.noreply.github.com&gt;
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/autoscale/CreateAutoScaleVmProfileCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/autoscale/CreateAutoScaleVmProfileCmd.java
@@ -100,7 +100,7 @@ public class CreateAutoScaleVmProfileCmd extends BaseAsyncCreateCmd {
             description = "an optional binary data that can be sent to the virtual machine upon a successful deployment. " +
                     "This binary data must be base64 encoded before adding it to the request. " +
                     "Using HTTP GET (via querystring), you can send up to 4KB of data after base64 encoding. " +
-                    "Using HTTP POST(via POST body), you can send up to 1MB of data after base64 encoding." +
+                    "Using HTTP POST (via POST body), you can send up to 1MB of data after base64 encoding. " +
                     "You also need to change vm.userdata.max.length value",
             length = 1048576,
             since = "4.18.0")
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/autoscale/UpdateAutoScaleVmProfileCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/autoscale/UpdateAutoScaleVmProfileCmd.java
@@ -95,7 +95,7 @@ public class UpdateAutoScaleVmProfileCmd extends BaseAsyncCustomIdCmd {
             description = "an optional binary data that can be sent to the virtual machine upon a successful deployment. " +
                     "This binary data must be base64 encoded before adding it to the request. " +
                     "Using HTTP GET (via querystring), you can send up to 4KB of data after base64 encoding. " +
-                    "Using HTTP POST(via POST body), you can send up to 1MB of data after base64 encoding." +
+                    "Using HTTP POST (via POST body), you can send up to 1MB of data after base64 encoding. " +
                     "You also need to change vm.userdata.max.length value",
             length = 1048576,
             since = "4.18.0")
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/userdata/RegisterUserDataCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/userdata/RegisterUserDataCmd.java
@@ -72,7 +72,14 @@ public class RegisterUserDataCmd extends BaseCmd {
     @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "an optional project for the userdata")
     private Long projectId;
 
-    @Parameter(name = ApiConstants.USER_DATA, type = CommandType.STRING, required = true, description = "Userdata content", length = 1048576)
+    @Parameter(name = ApiConstants.USER_DATA,
+            type = CommandType.STRING,
+            required = true,
+            description = "Base64 encoded userdata content. " +
+                    "Using HTTP GET (via querystring), you can send up to 4KB of data after base64 encoding. " +
+                    "Using HTTP POST (via POST body), you can send up to 1MB of data after base64 encoding. " +
+                    "You also need to change vm.userdata.max.length value",
+            length = 1048576)
     private String userData;
 
     @Parameter(name = ApiConstants.PARAMS, type = CommandType.STRING, description = "comma separated list of variables declared in userdata content")
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/DeployVMCmd.java
@@ -156,7 +156,11 @@ public class DeployVMCmd extends BaseAsyncCreateCustomIdCmd implements SecurityG
     private String hypervisor;
 
     @Parameter(name = ApiConstants.USER_DATA, type = CommandType.STRING,
-            description = "an optional binary data that can be sent to the virtual machine upon a successful deployment. This binary data must be base64 encoded before adding it to the request. Using HTTP GET (via querystring), you can send up to 4KB of data after base64 encoding. Using HTTP POST(via POST body), you can send up to 1MB of data after base64 encoding.",
+            description = "an optional binary data that can be sent to the virtual machine upon a successful deployment. " +
+                    "This binary data must be base64 encoded before adding it to the request. " +
+                    "Using HTTP GET (via querystring), you can send up to 4KB of data after base64 encoding. " +
+                    "Using HTTP POST (via POST body), you can send up to 1MB of data after base64 encoding. " +
+                    "You also need to change vm.userdata.max.length value",
             length = 1048576)
     private String userData;
 
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ResetVMUserDataCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/ResetVMUserDataCmd.java
@@ -60,7 +60,7 @@ public class ResetVMUserDataCmd extends BaseCmd implements UserCmd {
             description = "an optional binary data that can be sent to the virtual machine upon a successful deployment. " +
                     "This binary data must be base64 encoded before adding it to the request. " +
                     "Using HTTP GET (via querystring), you can send up to 4KB of data after base64 encoding. " +
-                    "Using HTTP POST(via POST body), you can send up to 1MB of data after base64 encoding." +
+                    "Using HTTP POST (via POST body), you can send up to 1MB of data after base64 encoding. " +
                     "You also need to change vm.userdata.max.length value",
             length = 1048576)
     private String userData;
diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java
@@ -84,7 +84,7 @@ public class UpdateVMCmd extends BaseCustomIdCmd implements SecurityGroupAction,
                description = "an optional binary data that can be sent to the virtual machine upon a successful deployment. " +
                        "This binary data must be base64 encoded before adding it to the request. " +
                        "Using HTTP GET (via querystring), you can send up to 4KB of data after base64 encoding. " +
-                       "Using HTTP POST(via POST body), you can send up to 1MB of data after base64 encoding." +
+                       "Using HTTP POST (via POST body), you can send up to 1MB of data after base64 encoding. " +
                        "You also need to change vm.userdata.max.length value",
                length = 1048576,
                since = "4.16.0")
diff --git a/api/src/main/java/org/apache/cloudstack/api/response/AutoScaleVmProfileResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/AutoScaleVmProfileResponse.java
@@ -69,7 +69,7 @@ public class AutoScaleVmProfileResponse extends BaseResponse implements Controll
     private Map<String, String> counterParams;
 
     @SerializedName(ApiConstants.USER_DATA)
-    @Param(description = "Base 64 encoded VM user data")
+    @Param(description = "Base64 encoded VM user data")
     private String userData;
 
     @SerializedName(ApiConstants.USER_DATA_ID) @Param(description="the id of userdata used for the VM", since = "4.18.1")
diff --git a/api/src/main/java/org/apache/cloudstack/api/response/VMUserDataResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/VMUserDataResponse.java
@@ -30,7 +30,7 @@ public class VMUserDataResponse extends BaseResponse {
     private String vmId;
 
     @SerializedName(ApiConstants.USER_DATA)
-    @Param(description = "Base 64 encoded VM user data")
+    @Param(description = "Base64 encoded VM user data")
     private String userData;
 
     public void setUserData(String userData) {
diff --git a/api/src/main/java/org/apache/cloudstack/userdata/UserDataManager.java b/api/src/main/java/org/apache/cloudstack/userdata/UserDataManager.java
@@ -17,11 +17,16 @@
 package org.apache.cloudstack.userdata;
 
 import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.framework.config.ConfigKey;
 import org.apache.cloudstack.framework.config.Configurable;
 
 import com.cloud.utils.component.Manager;
 
 public interface UserDataManager extends Manager, Configurable {
+    String VM_USERDATA_MAX_LENGTH_STRING = "vm.userdata.max.length";
+    ConfigKey<Integer> VM_USERDATA_MAX_LENGTH = new ConfigKey<>("Advanced", Integer.class, VM_USERDATA_MAX_LENGTH_STRING, "32768",
+            "Max length of vm userdata after base64 encoding. Default is 32768 and maximum is 1048576", true);
+
     String concatenateUserData(String userdata1, String userdata2, String userdataProvider);
     String validateUserData(String userData, BaseCmd.HTTPMethod httpmethod);
 }
diff --git a/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java b/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java
@@ -61,9 +61,6 @@ public interface ConfigurationManager {
     public static final String MESSAGE_CREATE_VLAN_IP_RANGE_EVENT = "Message.CreateVlanIpRange.Event";
     public static final String MESSAGE_DELETE_VLAN_IP_RANGE_EVENT = "Message.DeleteVlanIpRange.Event";
 
-    static final String VM_USERDATA_MAX_LENGTH_STRING = "vm.userdata.max.length";
-    static final ConfigKey<Integer> VM_USERDATA_MAX_LENGTH = new ConfigKey<>("Advanced", Integer.class, VM_USERDATA_MAX_LENGTH_STRING, "32768",
-            "Max length of vm userdata after base64 decoding. Default is 32768 and maximum is 1048576", true);
     public static final ConfigKey<Boolean> AllowNonRFC1918CompliantIPs = new ConfigKey<>(Boolean.class,
             "allow.non.rfc1918.compliant.ips", "Advanced", "false",
             "Allows non-compliant RFC 1918 IPs for Shared, Isolated networks and VPCs", true, null);
diff --git a/engine/userdata/src/main/java/org/apache/cloudstack/userdata/UserDataManagerImpl.java b/engine/userdata/src/main/java/org/apache/cloudstack/userdata/UserDataManagerImpl.java
@@ -26,19 +26,18 @@
 import org.apache.cloudstack.framework.config.ConfigKey;
 import org.apache.commons.codec.binary.Base64;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
 
-import com.cloud.configuration.ConfigurationManager;
 import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.utils.component.ManagerBase;
 import com.cloud.utils.exception.CloudRuntimeException;
 
 public class UserDataManagerImpl extends ManagerBase implements UserDataManager {
-
-
+    private static final Logger s_logger = Logger.getLogger(UserDataManagerImpl.class);
     private static final int MAX_USER_DATA_LENGTH_BYTES = 2048;
-    private static final int MAX_HTTP_GET_LENGTH = 2 * MAX_USER_DATA_LENGTH_BYTES;
+    private static final int MAX_HTTP_GET_LENGTH = 2 * MAX_USER_DATA_LENGTH_BYTES; // 4KB
     private static final int NUM_OF_2K_BLOCKS = 512;
-    private static final int MAX_HTTP_POST_LENGTH = NUM_OF_2K_BLOCKS * MAX_USER_DATA_LENGTH_BYTES;
+    private static final int MAX_HTTP_POST_LENGTH = NUM_OF_2K_BLOCKS * MAX_USER_DATA_LENGTH_BYTES; // 1MB
     private List<UserDataProvider> userDataProviders;
     private static Map<String, UserDataProvider> userDataProvidersMap = new HashMap<>();
 
@@ -67,7 +66,7 @@ public String getConfigComponentName() {
 
     @Override
     public ConfigKey<?>[] getConfigKeys() {
-        return new ConfigKey[] {};
+        return new ConfigKey[] {VM_USERDATA_MAX_LENGTH};
     }
 
     protected UserDataProvider getUserdataProvider(String name) {
@@ -90,49 +89,57 @@ public String concatenateUserData(String userdata1, String userdata2, String use
 
     @Override
     public String validateUserData(String userData, BaseCmd.HTTPMethod httpmethod) {
-        byte[] decodedUserData = null;
-        if (userData != null) {
-
-            if (userData.contains("%")) {
-                try {
-                    userData = URLDecoder.decode(userData, "UTF-8");
-                } catch (UnsupportedEncodingException e) {
-                    throw new InvalidParameterValueException("Url decoding of userdata failed.");
-                }
-            }
+        s_logger.trace(String.format("Validating base64 encoded user data: [%s].", userData));
+        if (StringUtils.isBlank(userData)) {
+            s_logger.debug("Null/empty base64 encoded user data set");
+            return null;
+        }
 
-            if (!Base64.isBase64(userData)) {
-                throw new InvalidParameterValueException("User data is not base64 encoded");
-            }
-            // If GET, use 4K. If POST, support up to 1M.
-            if (httpmethod.equals(BaseCmd.HTTPMethod.GET)) {
-                decodedUserData = validateAndDecodeByHTTPMethod(userData, MAX_HTTP_GET_LENGTH, BaseCmd.HTTPMethod.GET);
-            } else if (httpmethod.equals(BaseCmd.HTTPMethod.POST)) {
-                decodedUserData = validateAndDecodeByHTTPMethod(userData, MAX_HTTP_POST_LENGTH, BaseCmd.HTTPMethod.POST);
+        if (userData.contains("%")) {
+            try {
+                userData = URLDecoder.decode(userData, "UTF-8");
+            } catch (UnsupportedEncodingException e) {
+                throw new InvalidParameterValueException("Url decoding of user data failed.");
             }
+        }
 
-            if (decodedUserData == null || decodedUserData.length < 1) {
-                throw new InvalidParameterValueException("User data is too short");
-            }
-            // Re-encode so that the '=' paddings are added if necessary since 'isBase64' does not require it, but python does on the VR.
-            return Base64.encodeBase64String(decodedUserData);
+        if (!Base64.isBase64(userData)) {
+            throw new InvalidParameterValueException("User data is not base64 encoded.");
         }
-        return null;
-    }
 
-    private byte[] validateAndDecodeByHTTPMethod(String userData, int maxHTTPLength, BaseCmd.HTTPMethod httpMethod) {
         byte[] decodedUserData = null;
 
-        if (userData.length() >= maxHTTPLength) {
-            throw new InvalidParameterValueException(String.format("User data is too long for an http %s request", httpMethod.toString()));
+        // If GET, use 4K. If POST, support up to 1M.
+        if (httpmethod.equals(BaseCmd.HTTPMethod.GET)) {
+            decodedUserData = validateAndDecodeByHTTPMethod(userData, MAX_HTTP_GET_LENGTH, BaseCmd.HTTPMethod.GET);
+        } else if (httpmethod.equals(BaseCmd.HTTPMethod.POST)) {
+            decodedUserData = validateAndDecodeByHTTPMethod(userData, MAX_HTTP_POST_LENGTH, BaseCmd.HTTPMethod.POST);
         }
-        if (userData.length() > ConfigurationManager.VM_USERDATA_MAX_LENGTH.value()) {
-            throw new InvalidParameterValueException("User data has exceeded configurable max length : " + ConfigurationManager.VM_USERDATA_MAX_LENGTH.value());
+
+        // Re-encode so that the '=' paddings are added if necessary since 'isBase64' does not require it, but python does on the VR.
+        return Base64.encodeBase64String(decodedUserData);
+    }
+
+    private byte[] validateAndDecodeByHTTPMethod(String userData, int maxHTTPLength, BaseCmd.HTTPMethod httpMethod) {
+        byte[] decodedUserData = Base64.decodeBase64(userData.getBytes());
+        if (decodedUserData == null || decodedUserData.length < 1) {
+            throw new InvalidParameterValueException("User data is too short.");
         }
-        decodedUserData = Base64.decodeBase64(userData.getBytes());
-        if (decodedUserData.length > maxHTTPLength) {
+
+        s_logger.trace(String.format("Decoded user data: [%s].", decodedUserData));
+        int userDataLength = userData.length();
+        int decodedUserDataLength = decodedUserData.length;
+        s_logger.info(String.format("Configured base64 encoded user data size: %d bytes, actual user data size: %d bytes", userDataLength, decodedUserDataLength));
+
+        if (userDataLength > maxHTTPLength) {
+            s_logger.warn(String.format("Base64 encoded user data (size: %d bytes) too long for http %s request (accepted size: %d bytes)", userDataLength, httpMethod.toString(), maxHTTPLength));
             throw new InvalidParameterValueException(String.format("User data is too long for http %s request", httpMethod.toString()));
         }
+        if (userDataLength > VM_USERDATA_MAX_LENGTH.value()) {
+            s_logger.warn(String.format("Base64 encoded user data (size: %d bytes) has exceeded configurable max length of %d bytes", userDataLength, VM_USERDATA_MAX_LENGTH.value()));
+            throw new InvalidParameterValueException("User data has exceeded configurable max length: " + VM_USERDATA_MAX_LENGTH.value());
+        }
+
         return decodedUserData;
     }
 }
diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
@@ -16,6 +16,7 @@
 // under the License.
 package com.cloud.configuration;
 
+import java.io.ObjectInputFilter.Config;
 import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -136,6 +137,7 @@
 import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao;
 import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
+import org.apache.cloudstack.userdata.UserDataManager;
 import org.apache.cloudstack.utils.jsinterpreter.TagAsRuleHelper;
 import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
 import org.apache.commons.collections.CollectionUtils;
@@ -564,7 +566,7 @@ private void populateConfigValuesForValidationSet() {
         configValuesForValidation.add(StorageManager.STORAGE_POOL_DISK_WAIT.key());
         configValuesForValidation.add(StorageManager.STORAGE_POOL_CLIENT_TIMEOUT.key());
         configValuesForValidation.add(StorageManager.STORAGE_POOL_CLIENT_MAX_CONNECTIONS.key());
-        configValuesForValidation.add(VM_USERDATA_MAX_LENGTH_STRING);
+        configValuesForValidation.add(UserDataManager.VM_USERDATA_MAX_LENGTH_STRING);
     }
 
     private void weightBasedParametersForValidation() {
@@ -1295,7 +1297,7 @@ private String validateConfigurationValue(final String name, String value, final
                         throw new InvalidParameterValueException("Please enter a value less than 257 for the configuration parameter:" + name);
                     }
                 }
-                if (VM_USERDATA_MAX_LENGTH_STRING.equalsIgnoreCase(name)) {
+                if (UserDataManager.VM_USERDATA_MAX_LENGTH_STRING.equalsIgnoreCase(name)) {
                     if (val > 1048576) {
                         throw new InvalidParameterValueException("Please enter a value less than 1048576 for the configuration parameter:" + name);
                     }
@@ -7852,7 +7854,7 @@ public String getConfigComponentName() {
     public ConfigKey<?>[] getConfigKeys() {
         return new ConfigKey<?>[] {SystemVMUseLocalStorage, IOPS_MAX_READ_LENGTH, IOPS_MAX_WRITE_LENGTH,
                 BYTES_MAX_READ_LENGTH, BYTES_MAX_WRITE_LENGTH, ADD_HOST_ON_SERVICE_RESTART_KVM, SET_HOST_DOWN_TO_MAINTENANCE, VM_SERVICE_OFFERING_MAX_CPU_CORES,
-                VM_SERVICE_OFFERING_MAX_RAM_SIZE, VM_USERDATA_MAX_LENGTH, MIGRATE_VM_ACROSS_CLUSTERS, ENABLE_ACCOUNT_SETTINGS_FOR_DOMAIN,
+                VM_SERVICE_OFFERING_MAX_RAM_SIZE, MIGRATE_VM_ACROSS_CLUSTERS, ENABLE_ACCOUNT_SETTINGS_FOR_DOMAIN,
                 ENABLE_DOMAIN_SETTINGS_FOR_CHILD_DOMAIN, ALLOW_DOMAIN_ADMINS_TO_CREATE_TAGGED_OFFERINGS, AllowNonRFC1918CompliantIPs
         };
     }
diff --git a/server/src/main/java/com/cloud/network/as/AutoScaleManagerImpl.java b/server/src/main/java/com/cloud/network/as/AutoScaleManagerImpl.java
@@ -70,6 +70,7 @@
 import org.apache.cloudstack.framework.config.Configurable;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.cloudstack.userdata.UserDataManager;
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.collections.MapUtils;
 import org.apache.commons.lang3.RandomStringUtils;
@@ -252,6 +253,8 @@ public class AutoScaleManagerImpl extends ManagerBase implements AutoScaleManage
     @Inject
     private UserVmManager userVmMgr;
     @Inject
+    private UserDataManager userDataMgr;
+    @Inject
     private UserVmDao userVmDao;
     @Inject
     private HostDao hostDao;
@@ -571,7 +574,7 @@ public AutoScaleVmProfile createAutoScaleVmProfile(CreateAutoScaleVmProfileCmd c
             userDataDetails = cmd.getUserDataDetails().toString();
         }
         userData = userVmMgr.finalizeUserData(userData, userDataId, template);
-        userData = userVmMgr.validateUserData(userData, cmd.getHttpMethod());
+        userData = userDataMgr.validateUserData(userData, cmd.getHttpMethod());
         if (userData != null) {
             profileVO.setUserData(userData);
         }
@@ -650,7 +653,7 @@ public AutoScaleVmProfile updateAutoScaleVmProfile(UpdateAutoScaleVmProfileCmd c
             }
             VirtualMachineTemplate template = entityMgr.findByIdIncludingRemoved(VirtualMachineTemplate.class, templateId);
             userData = userVmMgr.finalizeUserData(userData, userDataId, template);
-            userData = userVmMgr.validateUserData(userData, cmd.getHttpMethod());
+            userData = userDataMgr.validateUserData(userData, cmd.getHttpMethod());
             vmProfile.setUserDataId(userDataId);
             vmProfile.setUserData(userData);
             vmProfile.setUserDataDetails(userDataDetails);
diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java
@@ -95,8 +95,6 @@ public interface UserVmManager extends UserVmService {
 
     String finalizeUserData(String userData, Long userDataId, VirtualMachineTemplate template);
 
-    String validateUserData(String userData, HTTPMethod httpmethod);
-
     void validateExtraConfig(long accountId, HypervisorType hypervisorType, String extraConfig);
 
     boolean isVMUsingLocalStorage(VMInstanceVO vm);
diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
diff --git a/server/src/test/java/com/cloud/network/as/AutoScaleManagerImplTest.java b/server/src/test/java/com/cloud/network/as/AutoScaleManagerImplTest.java
