From 5ebf0cb4139675dcef01b5eb2b207ed443d32360 Mon Sep 17 00:00:00 2001 From: Vishesh Date: Thu, 22 Aug 2024 13:00:38 +0530 Subject: [PATCH 1/4] Add validation for secstorage.allowed.internal.sites --- .../configuration/ConfigurationManagerImpl.java | 16 ++++++++++++++++ .../SecondaryStorageManagerImpl.java | 3 +++ ui/src/views/setting/ConfigurationValue.vue | 2 +- .../main/java/com/cloud/utils/net/NetUtils.java | 12 ++++++++++++ .../java/com/cloud/utils/net/NetUtilsTest.java | 11 +++++++++++ 5 files changed, 43 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 29579759c7f4..6764f8dba8a6 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -303,6 +303,8 @@ import com.google.common.collect.Sets; import com.googlecode.ipv6.IPv6Network; +import static com.cloud.configuration.Config.SecStorageAllowedInternalDownloadSites; + public class ConfigurationManagerImpl extends ManagerBase implements ConfigurationManager, ConfigurationService, Configurable { public static final Logger s_logger = Logger.getLogger(ConfigurationManagerImpl.class); public static final String PERACCOUNT = "peraccount"; @@ -1314,6 +1316,20 @@ protected String validateConfigurationValue(final String name, String value, fin } } + if (type.equals(String.class)) { + if (name.equals(SecStorageAllowedInternalDownloadSites.key())) { + if (StringUtils.isNotEmpty(value)) { + final String[] cidrs = value.split(","); + for (final String cidr : cidrs) { + if (!NetUtils.isValidIp4(cidr) && !NetUtils.isValidIp6(cidr) && !NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { + s_logger.error(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); + throw new InvalidParameterValueException(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); + } + } + } + } + } + if (configuration == null ) { //range validation has to be done per case basis, for now //return in case of Configkey parameters diff --git a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java index f37caa712bc7..d380df5783e1 100644 --- a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java +++ b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java @@ -401,6 +401,9 @@ private List getAllowedInternalSiteCidrs() { String[] cidrs = _allowedInternalSites.split(","); for (String cidr : cidrs) { if (NetUtils.isValidIp4Cidr(cidr) || NetUtils.isValidIp4(cidr) || !cidr.startsWith("0.0.0.0")) { + if (NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { + s_logger.warn(String.format("Invalid CIDR %s in secstorage.allowed.internal.sites", cidr)); + } allowedCidrs.add(cidr); } } diff --git a/ui/src/views/setting/ConfigurationValue.vue b/ui/src/views/setting/ConfigurationValue.vue index 0069896f7a56..836aed69dd3b 100644 --- a/ui/src/views/setting/ConfigurationValue.vue +++ b/ui/src/views/setting/ConfigurationValue.vue @@ -266,7 +266,7 @@ export default { this.$message.error(this.$t('message.error.save.setting')) this.$notification.error({ message: this.$t('label.error'), - description: this.$t('message.error.save.setting') + description: error?.response?.data?.updateconfigurationresponse?.errortext || this.$t('message.error.save.setting') }) }).finally(() => { this.valueLoading = false diff --git a/utils/src/main/java/com/cloud/utils/net/NetUtils.java b/utils/src/main/java/com/cloud/utils/net/NetUtils.java index 91a2f4eb7558..1b4ebcccf94d 100644 --- a/utils/src/main/java/com/cloud/utils/net/NetUtils.java +++ b/utils/src/main/java/com/cloud/utils/net/NetUtils.java @@ -626,6 +626,18 @@ public static String getCidrFromGatewayAndNetmask(final String gatewayStr, final return long2Ip(firstPart) + "/" + size; } + public static String getCleanIp4Cidr(final String cidr) { + if (!isValidIp4Cidr(cidr)) { + throw new CloudRuntimeException("Invalid CIDR: " + cidr); + } + String gateway = cidr.split("/")[0]; + Long netmaskSize = Long.parseLong(cidr.split("/")[1]); + final long ip = ip2Long(gateway); + final long startNetMask = ip2Long(getCidrNetmask(netmaskSize)); + final long start = (ip & startNetMask); + return String.format("%s/%s", long2Ip(start), netmaskSize); + } + public static String[] getIpRangeFromCidr(final String cidr, final long size) { assert size < MAX_CIDR : "You do know this is not for ipv6 right? Keep it smaller than 32 but you have " + size; final String[] result = new String[2]; diff --git a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java index defb440c2a51..0f19da389226 100644 --- a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java +++ b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java @@ -296,6 +296,17 @@ public void testIsValidCIDR() throws Exception { assertTrue(NetUtils.isValidIp4Cidr(cidrThird));; } + @Test + public void testGetCleanIp4Cidr() throws Exception { + final String cidrFirst = "10.0.144.0/20"; + final String cidrSecond = "10.0.151.5/20"; + final String cidrThird = "10.0.144.10/21"; + + assertEquals(cidrFirst, NetUtils.getCleanIp4Cidr(cidrFirst)); + assertEquals("10.0.144.0/20", NetUtils.getCleanIp4Cidr(cidrSecond)); + assertEquals("10.0.144.0/21", NetUtils.getCleanIp4Cidr(cidrThird));; + } + @Test public void testIsValidCidrList() throws Exception { final String cidrFirst = "10.0.144.0/20,1.2.3.4/32,5.6.7.8/24"; From c56e3ea3fc07b8dcf5c119937fabdabdbe3377a9 Mon Sep 17 00:00:00 2001 From: Vishesh Date: Thu, 22 Aug 2024 16:21:16 +0530 Subject: [PATCH 2/4] Address comments --- .../configuration/ConfigurationManagerImpl.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 6764f8dba8a6..114a4349dd04 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -1317,14 +1317,12 @@ protected String validateConfigurationValue(final String name, String value, fin } if (type.equals(String.class)) { - if (name.equals(SecStorageAllowedInternalDownloadSites.key())) { - if (StringUtils.isNotEmpty(value)) { - final String[] cidrs = value.split(","); - for (final String cidr : cidrs) { - if (!NetUtils.isValidIp4(cidr) && !NetUtils.isValidIp6(cidr) && !NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { - s_logger.error(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); - throw new InvalidParameterValueException(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); - } + if (name.equalsIgnoreCase(SecStorageAllowedInternalDownloadSites.key()) && StringUtils.isNotEmpty(value)) { + final String[] cidrs = value.split(","); + for (final String cidr : cidrs) { + if (!NetUtils.isValidIp4(cidr) && !NetUtils.isValidIp6(cidr) && !NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { + s_logger.error(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); + throw new InvalidParameterValueException(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); } } } From eb35f2d00e8f1e3f485beb729745bd2a9db50196 Mon Sep 17 00:00:00 2001 From: Vishesh Date: Fri, 23 Aug 2024 11:58:35 +0530 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Suresh Kumar Anaparti --- .../com/cloud/configuration/ConfigurationManagerImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 114a4349dd04..9df33b47257f 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -1321,8 +1321,8 @@ protected String validateConfigurationValue(final String name, String value, fin final String[] cidrs = value.split(","); for (final String cidr : cidrs) { if (!NetUtils.isValidIp4(cidr) && !NetUtils.isValidIp6(cidr) && !NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { - s_logger.error(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); - throw new InvalidParameterValueException(String.format("Invalid CIDR %s value specified for the config value %s", cidr, name)); + s_logger.error(String.format("Invalid CIDR %s value specified for the config %s", cidr, name)); + throw new InvalidParameterValueException(String.format("Invalid CIDR %s value specified for the config %s", cidr, name)); } } } From 4ba14eff36367c43766b75902d0deca1068e182c Mon Sep 17 00:00:00 2001 From: Vishesh Date: Fri, 23 Aug 2024 11:59:03 +0530 Subject: [PATCH 4/4] Address comments --- .../secondarystorage/SecondaryStorageManagerImpl.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java index d380df5783e1..cd6f23923e14 100644 --- a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java +++ b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java @@ -158,6 +158,8 @@ import com.cloud.vm.dao.UserVmDetailsDao; import com.cloud.vm.dao.VMInstanceDao; +import static com.cloud.configuration.Config.SecStorageAllowedInternalDownloadSites; + /** * Class to manage secondary storages.

* Possible secondary storage VM state transition cases:
@@ -402,7 +404,7 @@ private List getAllowedInternalSiteCidrs() { for (String cidr : cidrs) { if (NetUtils.isValidIp4Cidr(cidr) || NetUtils.isValidIp4(cidr) || !cidr.startsWith("0.0.0.0")) { if (NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) { - s_logger.warn(String.format("Invalid CIDR %s in secstorage.allowed.internal.sites", cidr)); + s_logger.warn(String.format("Invalid CIDR %s in %s", cidr, SecStorageAllowedInternalDownloadSites.key())); } allowedCidrs.add(cidr); }