You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ro...@apache.org on 2024/04/04 05:31:02 UTC
(cloudstack) branch 4.18 updated (8c62365dbb1 -> 00f687db1be)
This is an automated email from the ASF dual-hosted git repository.
rohit pushed a change to branch 4.18
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
from 8c62365dbb1 VPC VR: fix empty iptables if there is no vpc tier (#8787)
new 72b2eb0087f server: fix security issues caused by extraconfig on KVM
new 00f687db1be api: client verification in servlet
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../java/org/apache/cloudstack/ServerDaemon.java | 3 +-
.../cloud/configuration/ConfigurationManager.java | 2 +
.../cloudstack/framework/config/ConfigKey.java | 1 +
server/src/main/java/com/cloud/api/ApiServer.java | 40 ++++++++++++++++----
server/src/main/java/com/cloud/api/ApiServlet.java | 41 +++++++++++++++------
.../configuration/ConfigurationManagerImpl.java | 10 +++++
.../com/cloud/hypervisor/HypervisorGuruBase.java | 15 ++++++--
.../src/main/java/com/cloud/vm/UserVmManager.java | 3 ++
.../main/java/com/cloud/vm/UserVmManagerImpl.java | 43 +++++++++++++++++-----
.../test/java/com/cloud/api/ApiServletTest.java | 29 +++++++++++----
.../cloud/vpc/MockConfigurationManagerImpl.java | 5 +++
ui/public/locales/en.json | 1 +
ui/src/components/view/DetailSettings.vue | 13 ++++++-
.../src/main/java/com/cloud/utils/StringUtils.java | 2 +-
14 files changed, 163 insertions(+), 45 deletions(-)
(cloudstack) 01/02: server: fix security issues caused by extraconfig on KVM
Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch 4.18
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 72b2eb0087f60453fa68a3749c3d8cc6025794dc
Author: Wei Zhou <we...@apache.org>
AuthorDate: Thu Mar 21 18:51:49 2024 +0100
server: fix security issues caused by extraconfig on KVM
- Move allow.additional.vm.configuration.list.kvm from Global to Account setting
- Disallow VM details start with "extraconfig" when deploy VMs
- Skip changes on VM details start with "extraconfig" when update VM settings
- Allow only extraconfig for DPDK in service offering details
- Check if extraconfig values in vm details are supported when start VMs
- Check if extraconfig values in service offering details are supported when start VMs
- Disallow add/edit/update VM setting for extraconfig on UI
(cherry picked from commit e6e4fe16fb1ee428c3664b6b57384514e5a9252e)
Signed-off-by: Rohit Yadav <ro...@shapeblue.com>
---
.../cloud/configuration/ConfigurationManager.java | 2 +
.../configuration/ConfigurationManagerImpl.java | 10 +++++
.../com/cloud/hypervisor/HypervisorGuruBase.java | 15 ++++++--
.../src/main/java/com/cloud/vm/UserVmManager.java | 3 ++
.../main/java/com/cloud/vm/UserVmManagerImpl.java | 43 +++++++++++++++++-----
.../cloud/vpc/MockConfigurationManagerImpl.java | 5 +++
ui/public/locales/en.json | 1 +
ui/src/components/view/DetailSettings.vue | 13 ++++++-
8 files changed, 78 insertions(+), 14 deletions(-)
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
index c5caa312b58..2ce7631862b 100644
--- a/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java
+++ b/engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java
@@ -276,4 +276,6 @@ public interface ConfigurationManager {
Pair<String, String> getConfigurationGroupAndSubGroup(String configName);
List<ConfigurationSubGroupVO> getConfigurationSubGroups(Long groupId);
+
+ void validateExtraConfigInServiceOfferingDetail(String detailName);
}
diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
index 455a964b8d7..080bb83253c 100644
--- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
+++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
@@ -193,6 +193,7 @@ import com.cloud.host.HostVO;
import com.cloud.host.dao.HostDao;
import com.cloud.host.dao.HostTagsDao;
import com.cloud.hypervisor.Hypervisor.HypervisorType;
+import com.cloud.hypervisor.kvm.dpdk.DpdkHelper;
import com.cloud.network.IpAddress;
import com.cloud.network.IpAddressManager;
import com.cloud.network.Ipv6GuestPrefixSubnetNetworkMapVO;
@@ -3201,6 +3202,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
}
}
if (detailEntry.getKey().startsWith(ApiConstants.EXTRA_CONFIG)) {
+ validateExtraConfigInServiceOfferingDetail(detailEntry.getKey());
try {
detailEntryValue = URLDecoder.decode(detailEntry.getValue(), "UTF-8");
} catch (UnsupportedEncodingException | IllegalArgumentException e) {
@@ -3266,6 +3268,14 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
}
}
+ @Override
+ public void validateExtraConfigInServiceOfferingDetail(String detailName) {
+ if (!detailName.equals(DpdkHelper.DPDK_NUMA) && !detailName.equals(DpdkHelper.DPDK_HUGE_PAGES)
+ && !detailName.startsWith(DpdkHelper.DPDK_INTERFACE_PREFIX)) {
+ throw new InvalidParameterValueException("Only extraconfig for DPDK are supported in service offering details");
+ }
+ }
+
private DiskOfferingVO createDiskOfferingInternal(final long userId, final boolean isSystem, final VirtualMachine.Type vmType,
final String name, final Integer cpu, final Integer ramSize, final Integer speed, final String displayText, final ProvisioningType typedProvisioningType, final boolean localStorageRequired,
final boolean offerHA, final boolean limitResourceUse, final boolean volatileVm, String tags, final List<Long> domainIds, List<Long> zoneIds, final String hostTag,
diff --git a/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java b/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
index 37b430a4cfc..4803e271a5f 100644
--- a/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
+++ b/server/src/main/java/com/cloud/hypervisor/HypervisorGuruBase.java
@@ -37,6 +37,7 @@ import com.cloud.agent.api.Command;
import com.cloud.agent.api.to.DiskTO;
import com.cloud.agent.api.to.NicTO;
import com.cloud.agent.api.to.VirtualMachineTO;
+import com.cloud.configuration.ConfigurationManager;
import com.cloud.gpu.GPU;
import com.cloud.host.HostVO;
import com.cloud.host.dao.HostDao;
@@ -59,6 +60,7 @@ import com.cloud.utils.Pair;
import com.cloud.utils.component.AdapterBase;
import com.cloud.vm.NicProfile;
import com.cloud.vm.NicVO;
+import com.cloud.vm.UserVmManager;
import com.cloud.vm.VMInstanceVO;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachineProfile;
@@ -96,6 +98,10 @@ public abstract class HypervisorGuruBase extends AdapterBase implements Hypervis
@Inject
protected
HostDao hostDao;
+ @Inject
+ private UserVmManager userVmManager;
+ @Inject
+ private ConfigurationManager configurationManager;
public static ConfigKey<Boolean> VmMinMemoryEqualsMemoryDividedByMemOverprovisioningFactor = new ConfigKey<Boolean>("Advanced", Boolean.class, "vm.min.memory.equals.memory.divided.by.mem.overprovisioning.factor", "true",
"If we set this to 'true', a minimum memory (memory/ mem.overprovisioning.factor) will be set to the VM, independent of using a scalable service offering or not.", true, ConfigKey.Scope.Cluster);
@@ -180,10 +186,12 @@ public abstract class HypervisorGuruBase extends AdapterBase implements Hypervis
/**
* Add extra configuration from VM details. Extra configuration is stored as details starting with 'extraconfig'
*/
- private void addExtraConfig(Map<String, String> details, VirtualMachineTO to) {
+ private void addExtraConfig(Map<String, String> details, VirtualMachineTO to, long accountId, Hypervisor.HypervisorType hypervisorType) {
for (String key : details.keySet()) {
if (key.startsWith(ApiConstants.EXTRA_CONFIG)) {
- to.addExtraConfig(key, details.get(key));
+ String extraConfig = details.get(key);
+ userVmManager.validateExtraConfig(accountId, hypervisorType, extraConfig);
+ to.addExtraConfig(key, extraConfig);
}
}
}
@@ -199,6 +207,7 @@ public abstract class HypervisorGuruBase extends AdapterBase implements Hypervis
if (CollectionUtils.isNotEmpty(details)) {
for (ServiceOfferingDetailsVO detail : details) {
if (detail.getName().startsWith(ApiConstants.EXTRA_CONFIG)) {
+ configurationManager.validateExtraConfigInServiceOfferingDetail(detail.getName());
to.addExtraConfig(detail.getName(), detail.getValue());
}
}
@@ -262,7 +271,7 @@ public abstract class HypervisorGuruBase extends AdapterBase implements Hypervis
Map<String, String> detailsInVm = _userVmDetailsDao.listDetailsKeyPairs(vm.getId());
if (detailsInVm != null) {
to.setDetails(detailsInVm);
- addExtraConfig(detailsInVm, to);
+ addExtraConfig(detailsInVm, to, vm.getAccountId(), vm.getHypervisorType());
}
addServiceOfferingExtraConfiguration(offering, to);
diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java
index 4f1396913cc..b107a520205 100644
--- a/server/src/main/java/com/cloud/vm/UserVmManager.java
+++ b/server/src/main/java/com/cloud/vm/UserVmManager.java
@@ -30,6 +30,7 @@ import com.cloud.exception.ManagementServerException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.exception.VirtualMachineMigrationException;
+import com.cloud.hypervisor.Hypervisor.HypervisorType;
import com.cloud.offering.ServiceOffering;
import com.cloud.service.ServiceOfferingVO;
import com.cloud.storage.Storage.StoragePoolType;
@@ -96,6 +97,8 @@ public interface UserVmManager extends UserVmService {
String validateUserData(String userData, HTTPMethod httpmethod);
+ void validateExtraConfig(long accountId, HypervisorType hypervisorType, String extraConfig);
+
boolean isVMUsingLocalStorage(VMInstanceVO vm);
boolean expunge(UserVmVO vm);
diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
index ffa427677dd..9a5e7685c70 100644
--- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
+++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
@@ -630,7 +630,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
"enable.additional.vm.configuration", "false", "allow additional arbitrary configuration to vm", true, ConfigKey.Scope.Account);
private static final ConfigKey<String> KvmAdditionalConfigAllowList = new ConfigKey<>(String.class,
- "allow.additional.vm.configuration.list.kvm", "Advanced", "", "Comma separated list of allowed additional configuration options.", true, ConfigKey.Scope.Global, null, null, EnableAdditionalVmConfig.key(), null, null, ConfigKey.Kind.CSV, null);
+ "allow.additional.vm.configuration.list.kvm", "Advanced", "", "Comma separated list of allowed additional configuration options.", true, ConfigKey.Scope.Account, null, null, EnableAdditionalVmConfig.key(), null, null, ConfigKey.Kind.CSV, null);
private static final ConfigKey<String> XenServerAdditionalConfigAllowList = new ConfigKey<>(String.class,
"allow.additional.vm.configuration.list.xenserver", "Advanced", "", "Comma separated list of allowed additional configuration options", true, ConfigKey.Scope.Global, null, null, EnableAdditionalVmConfig.key(), null, null, ConfigKey.Kind.CSV, null);
@@ -2774,14 +2774,15 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
if (cleanupDetails){
if (caller != null && caller.getType() == Account.Type.ADMIN) {
for (final UserVmDetailVO detail : existingDetails) {
- if (detail != null && detail.isDisplay()) {
+ if (detail != null && detail.isDisplay() && !isExtraConfig(detail.getName())) {
userVmDetailsDao.removeDetail(id, detail.getName());
}
}
} else {
for (final UserVmDetailVO detail : existingDetails) {
if (detail != null && !userDenyListedSettings.contains(detail.getName())
- && !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()) {
+ && !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()
+ && !isExtraConfig(detail.getName())) {
userVmDetailsDao.removeDetail(id, detail.getName());
}
}
@@ -2792,6 +2793,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
throw new InvalidParameterValueException("'extraconfig' should not be included in details as key");
}
+ details.entrySet().removeIf(detail -> isExtraConfig(detail.getKey()));
+
if (caller != null && caller.getType() != Account.Type.ADMIN) {
// Ensure denied or read-only detail is not passed by non-root-admin user
for (final String detailName : details.keySet()) {
@@ -2815,7 +2818,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
// ensure details marked as non-displayable are maintained, regardless of admin or not
for (final UserVmDetailVO existingDetail : existingDetails) {
- if (!existingDetail.isDisplay()) {
+ if (!existingDetail.isDisplay() || isExtraConfig(existingDetail.getName())) {
details.put(existingDetail.getName(), existingDetail.getValue());
}
}
@@ -2837,6 +2840,10 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
cmd.getHttpMethod(), cmd.getCustomId(), hostName, cmd.getInstanceName(), securityGroupIdList, cmd.getDhcpOptionsMap());
}
+ private boolean isExtraConfig(String detailName) {
+ return detailName != null && detailName.startsWith(ApiConstants.EXTRA_CONFIG);
+ }
+
protected void updateDisplayVmFlag(Boolean isDisplayVm, Long id, UserVmVO vmInstance) {
vmInstance.setDisplayVm(isDisplayVm);
@@ -6101,7 +6108,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
*/
protected void persistExtraConfigKvm(String decodedUrl, UserVm vm) {
// validate config against denied cfg commands
- validateKvmExtraConfig(decodedUrl);
+ validateKvmExtraConfig(decodedUrl, vm.getAccountId());
String[] extraConfigs = decodedUrl.split("\n\n");
for (String cfg : extraConfigs) {
int i = 1;
@@ -6119,6 +6126,18 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
i++;
}
}
+ /**
+ * This method is used to validate if extra config is valid
+ */
+ @Override
+ public void validateExtraConfig(long accountId, HypervisorType hypervisorType, String extraConfig) {
+ if (!EnableAdditionalVmConfig.valueIn(accountId)) {
+ throw new CloudRuntimeException("Additional VM configuration is not enabled for this account");
+ }
+ if (HypervisorType.KVM.equals(hypervisorType)) {
+ validateKvmExtraConfig(extraConfig, accountId);
+ }
+ }
/**
* This method is called by the persistExtraConfigKvm
@@ -6126,9 +6145,9 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
* controlled by Root admin
* @param decodedUrl string containing xml configuration to be validated
*/
- protected void validateKvmExtraConfig(String decodedUrl) {
- String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.value().split(",");
- // Skip allowed keys validation validation for DPDK
+ protected void validateKvmExtraConfig(String decodedUrl, long accountId) {
+ String[] allowedConfigOptionList = KvmAdditionalConfigAllowList.valueIn(accountId).split(",");
+ // Skip allowed keys validation for DPDK
if (!decodedUrl.contains(":")) {
try {
DocumentBuilder builder = ParserUtils.getSaferDocumentBuilderFactory().newDocumentBuilder();
@@ -6147,7 +6166,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
}
}
if (!isValidConfig) {
- throw new CloudRuntimeException(String.format("Extra config %s is not on the list of allowed keys for KVM hypervisor hosts", currentConfig));
+ throw new CloudRuntimeException(String.format("Extra config '%s' is not on the list of allowed keys for KVM hypervisor hosts", currentConfig));
}
}
} catch (ParserConfigurationException | IOException | SAXException e) {
@@ -6235,6 +6254,12 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
if (details.containsKey("extraconfig")) {
throw new InvalidParameterValueException("'extraconfig' should not be included in details as key");
}
+
+ for (String detailName : details.keySet()) {
+ if (isExtraConfig(detailName)) {
+ throw new InvalidParameterValueException("detail name should not start with extraconfig");
+ }
+ }
}
}
diff --git a/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java b/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java
index 96dc8277a91..b9b8ca8677a 100644
--- a/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java
+++ b/server/src/test/java/com/cloud/vpc/MockConfigurationManagerImpl.java
@@ -679,4 +679,9 @@ public class MockConfigurationManagerImpl extends ManagerBase implements Configu
// TODO Auto-generated method stub
return null;
}
+
+ @Override
+ public void validateExtraConfigInServiceOfferingDetail(String detailName) {
+ // TODO Auto-generated method stub
+ }
}
diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json
index d94ca7cce81..b11a8f864fb 100644
--- a/ui/public/locales/en.json
+++ b/ui/public/locales/en.json
@@ -13,6 +13,7 @@
"error.release.dedicate.host": "Failed to release dedicated host.",
"error.release.dedicate.pod": "Failed to release dedicated pod.",
"error.release.dedicate.zone": "Failed to release dedicated zone.",
+"error.unable.to.add.setting.extraconfig": "It is not allowed to add setting for extraconfig. Please update VirtualMachine with extraconfig parameter.",
"error.unable.to.proceed": "Unable to proceed. Please contact your administrator.",
"firewall.close": "Firewall",
"icmp.code.desc": "Please specify -1 if you want to allow all ICMP codes.",
diff --git a/ui/src/components/view/DetailSettings.vue b/ui/src/components/view/DetailSettings.vue
index ba96ad6ee8b..ba9f61860e4 100644
--- a/ui/src/components/view/DetailSettings.vue
+++ b/ui/src/components/view/DetailSettings.vue
@@ -101,7 +101,7 @@
<tooltip-button
:tooltip="$t('label.edit')"
icon="edit-outlined"
- :disabled="deployasistemplate === true"
+ :disabled="deployasistemplate === true || item.name.startsWith('extraconfig')"
v-if="!item.edit"
@onClick="showEditDetail(index)" />
</div>
@@ -115,7 +115,12 @@
:cancelText="$t('label.no')"
placement="left"
>
- <tooltip-button :tooltip="$t('label.delete')" :disabled="deployasistemplate === true" type="primary" :danger="true" icon="delete-outlined" />
+ <tooltip-button
+ :tooltip="$t('label.delete')"
+ :disabled="deployasistemplate === true || item.name.startsWith('extraconfig')"
+ type="primary"
+ :danger="true"
+ icon="delete-outlined" />
</a-popconfirm>
</div>
</template>
@@ -307,6 +312,10 @@ export default {
this.error = this.$t('message.error.provide.setting')
return
}
+ if (this.newKey.startsWith('extraconfig')) {
+ this.error = this.$t('error.unable.to.add.setting.extraconfig')
+ return
+ }
if (!this.allowEditOfDetail(this.newKey)) {
this.error = this.$t('error.unable.to.proceed')
return
(cloudstack) 02/02: api: client verification in servlet
Posted by ro...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch 4.18
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 00f687db1bee019adc1363b236caea3d7d4d7cc8
Author: dahn <da...@onecht.net>
AuthorDate: Tue Mar 26 07:13:23 2024 +0100
api: client verification in servlet
This introduces new global settings to handle how client address checks
are handled by the API layer:
proxy.header.verify: enables/disables checking of ipaddresses from a
proxy set header
proxy.header.names: a list of names to check for allowed ipaddresses
from a proxy set header.
proxy.cidr: a list of cidrs for which \"proxy.header.names\" are
honoured if the \"Remote_Addr\" is in this list.
(cherry picked from commit b65546636d84a5790e0297b1b0ca8e5a67a48dbc)
Signed-off-by: Rohit Yadav <ro...@shapeblue.com>
---
.../java/org/apache/cloudstack/ServerDaemon.java | 3 +-
.../cloudstack/framework/config/ConfigKey.java | 1 +
server/src/main/java/com/cloud/api/ApiServer.java | 40 ++++++++++++++++-----
server/src/main/java/com/cloud/api/ApiServlet.java | 41 +++++++++++++++-------
.../test/java/com/cloud/api/ApiServletTest.java | 29 ++++++++++-----
.../src/main/java/com/cloud/utils/StringUtils.java | 2 +-
6 files changed, 85 insertions(+), 31 deletions(-)
diff --git a/client/src/main/java/org/apache/cloudstack/ServerDaemon.java b/client/src/main/java/org/apache/cloudstack/ServerDaemon.java
index 08f856655dc..ce927601653 100644
--- a/client/src/main/java/org/apache/cloudstack/ServerDaemon.java
+++ b/client/src/main/java/org/apache/cloudstack/ServerDaemon.java
@@ -31,7 +31,6 @@ import com.cloud.utils.server.ServerProperties;
import org.apache.commons.daemon.Daemon;
import org.apache.commons.daemon.DaemonContext;
import org.eclipse.jetty.jmx.MBeanContainer;
-import org.eclipse.jetty.server.ForwardedRequestCustomizer;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.HttpConnectionFactory;
import org.eclipse.jetty.server.NCSARequestLog;
@@ -167,7 +166,7 @@ public class ServerDaemon implements Daemon {
// HTTP config
final HttpConfiguration httpConfig = new HttpConfiguration();
- httpConfig.addCustomizer( new ForwardedRequestCustomizer() );
+// it would be nice to make this dynamic but we take care of this ourselves for now: httpConfig.addCustomizer( new ForwardedRequestCustomizer() );
httpConfig.setSecureScheme("https");
httpConfig.setSecurePort(httpsPort);
httpConfig.setOutputBufferSize(32768);
diff --git a/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java b/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java
index 13c594f9367..bd38ba92fd5 100644
--- a/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java
+++ b/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java
@@ -34,6 +34,7 @@ public class ConfigKey<T> {
public static final String CATEGORY_ADVANCED = "Advanced";
public static final String CATEGORY_ALERT = "Alert";
+ public static final String CATEGORY_NETWORK = "Network";
public enum Scope {
Global, Zone, Cluster, StoragePool, Account, ManagementServer, ImageStore, Domain
diff --git a/server/src/main/java/com/cloud/api/ApiServer.java b/server/src/main/java/com/cloud/api/ApiServer.java
index e88d7cf8b53..4a7259c6d33 100644
--- a/server/src/main/java/com/cloud/api/ApiServer.java
+++ b/server/src/main/java/com/cloud/api/ApiServer.java
@@ -234,42 +234,42 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer
@Inject
private MessageBus messageBus;
- private static final ConfigKey<Integer> IntegrationAPIPort = new ConfigKey<Integer>("Advanced"
+ private static final ConfigKey<Integer> IntegrationAPIPort = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
, Integer.class
, "integration.api.port"
, "0"
, "Integration (unauthenticated) API port. To disable set it to 0 or negative."
, false
, ConfigKey.Scope.Global);
- private static final ConfigKey<Long> ConcurrentSnapshotsThresholdPerHost = new ConfigKey<Long>("Advanced"
+ private static final ConfigKey<Long> ConcurrentSnapshotsThresholdPerHost = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
, Long.class
, "concurrent.snapshots.threshold.perhost"
, null
, "Limits number of snapshots that can be handled by the host concurrently; default is NULL - unlimited"
, true // not sure if this is to be dynamic
, ConfigKey.Scope.Global);
- private static final ConfigKey<Boolean> EncodeApiResponse = new ConfigKey<Boolean>("Advanced"
+ private static final ConfigKey<Boolean> EncodeApiResponse = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
, Boolean.class
, "encode.api.response"
, "false"
, "Do URL encoding for the api response, false by default"
, false
, ConfigKey.Scope.Global);
- static final ConfigKey<String> JSONcontentType = new ConfigKey<String>( "Advanced"
+ static final ConfigKey<String> JSONcontentType = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
, String.class
, "json.content.type"
, "application/json; charset=UTF-8"
, "Http response content type for .js files (default is text/javascript)"
, false
, ConfigKey.Scope.Global);
- static final ConfigKey<Boolean> EnableSecureSessionCookie = new ConfigKey<Boolean>("Advanced"
+ static final ConfigKey<Boolean> EnableSecureSessionCookie = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
, Boolean.class
, "enable.secure.session.cookie"
, "false"
, "Session cookie is marked as secure if this is enabled. Secure cookies only work when HTTPS is used."
, false
, ConfigKey.Scope.Global);
- private static final ConfigKey<String> JSONDefaultContentType = new ConfigKey<String> ("Advanced"
+ private static final ConfigKey<String> JSONDefaultContentType = new ConfigKey<> (ConfigKey.CATEGORY_ADVANCED
, String.class
, "json.content.type"
, "application/json; charset=UTF-8"
@@ -277,13 +277,34 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer
, false
, ConfigKey.Scope.Global);
- private static final ConfigKey<Boolean> UseEventAccountInfo = new ConfigKey<Boolean>( "advanced"
+ private static final ConfigKey<Boolean> UseEventAccountInfo = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED
, Boolean.class
, "event.accountinfo"
, "false"
, "use account info in event logging"
, true
, ConfigKey.Scope.Global);
+ static final ConfigKey<Boolean> useForwardHeader = new ConfigKey<>(ConfigKey.CATEGORY_NETWORK
+ , Boolean.class
+ , "proxy.header.verify"
+ , "false"
+ , "enables/disables checking of ipaddresses from a proxy set header. See \"proxy.header.names\" for the headers to allow."
+ , true
+ , ConfigKey.Scope.Global);
+ static final ConfigKey<String> listOfForwardHeaders = new ConfigKey<>(ConfigKey.CATEGORY_NETWORK
+ , String.class
+ , "proxy.header.names"
+ , "X-Forwarded-For,HTTP_CLIENT_IP,HTTP_X_FORWARDED_FOR"
+ , "a list of names to check for allowed ipaddresses from a proxy set header. See \"proxy.cidr\" for the proxies allowed to set these headers."
+ , true
+ , ConfigKey.Scope.Global);
+ static final ConfigKey<String> proxyForwardList = new ConfigKey<>(ConfigKey.CATEGORY_NETWORK
+ , String.class
+ , "proxy.cidr"
+ , ""
+ , "a list of cidrs for which \"proxy.header.names\" are honoured if the \"Remote_Addr\" is in this list."
+ , true
+ , ConfigKey.Scope.Global);
@Override
public boolean configure(final String name, final Map<String, Object> params) throws ConfigurationException {
@@ -1499,7 +1520,10 @@ public class ApiServer extends ManagerBase implements HttpRequestHandler, ApiSer
ConcurrentSnapshotsThresholdPerHost,
EncodeApiResponse,
EnableSecureSessionCookie,
- JSONDefaultContentType
+ JSONDefaultContentType,
+ proxyForwardList,
+ useForwardHeader,
+ listOfForwardHeaders
};
}
}
diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java
index 626678649d7..f6f46419c04 100644
--- a/server/src/main/java/com/cloud/api/ApiServlet.java
+++ b/server/src/main/java/com/cloud/api/ApiServlet.java
@@ -21,7 +21,6 @@ import java.net.InetAddress;
import java.net.URLDecoder;
import java.net.UnknownHostException;
import java.util.Arrays;
-import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -69,13 +68,9 @@ import com.cloud.utils.db.EntityManager;
import com.cloud.utils.net.NetUtils;
@Component("apiServlet")
-@SuppressWarnings("serial")
public class ApiServlet extends HttpServlet {
public static final Logger s_logger = Logger.getLogger(ApiServlet.class.getName());
private static final Logger s_accessLogger = Logger.getLogger("apiserver." + ApiServlet.class.getName());
- private final static List<String> s_clientAddressHeaders = Collections
- .unmodifiableList(Arrays.asList("X-Forwarded-For",
- "HTTP_CLIENT_IP", "HTTP_X_FORWARDED_FOR", "Remote_Addr"));
private static final String REPLACEMENT = "_";
private static final String LOG_REPLACEMENTS = "[\n\r\t]";
@@ -570,17 +565,39 @@ public class ApiServlet extends HttpServlet {
}
return false;
}
+ boolean doUseForwardHeaders() {
+ return Boolean.TRUE.equals(ApiServer.useForwardHeader.value());
+ }
+ String[] proxyNets() {
+ return ApiServer.proxyForwardList.value().split(",");
+ }
//This method will try to get login IP of user even if servlet is behind reverseProxy or loadBalancer
- public static InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
- for(final String header : s_clientAddressHeaders) {
- final String ip = getCorrectIPAddress(request.getHeader(header));
- if (ip != null) {
- return InetAddress.getByName(ip);
- }
+ public InetAddress getClientAddress(final HttpServletRequest request) throws UnknownHostException {
+ String ip = null;
+ InetAddress pretender = InetAddress.getByName(request.getRemoteAddr());
+ if(doUseForwardHeaders()) {
+ if (NetUtils.isIpInCidrList(pretender, proxyNets())) {
+ for (String header : getClientAddressHeaders()) {
+ 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));
+ 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()));
+ return pretender;
+ }
+
+ return InetAddress.getByName(ip);
+ }
- return InetAddress.getByName(request.getRemoteAddr());
+ private String[] getClientAddressHeaders() {
+ return ApiServer.listOfForwardHeaders.value().split(",");
}
private static String getCorrectIPAddress(String ip) {
diff --git a/server/src/test/java/com/cloud/api/ApiServletTest.java b/server/src/test/java/com/cloud/api/ApiServletTest.java
index ce1f009d3e8..250d2b06d97 100644
--- a/server/src/test/java/com/cloud/api/ApiServletTest.java
+++ b/server/src/test/java/com/cloud/api/ApiServletTest.java
@@ -99,15 +99,17 @@ public class ApiServletTest {
@Mock
AccountService accountMgr;
+ @Mock ConfigKey<Boolean> useForwardHeader;
StringWriter responseWriter;
ApiServlet servlet;
-
+ ApiServlet spyServlet;
@SuppressWarnings("unchecked")
@Before
public void setup() throws SecurityException, NoSuchFieldException,
IllegalArgumentException, IllegalAccessException, IOException, UnknownHostException {
servlet = new ApiServlet();
+ spyServlet = Mockito.spy(servlet);
responseWriter = new StringWriter();
Mockito.when(response.getWriter()).thenReturn(
new PrintWriter(responseWriter));
@@ -261,32 +263,43 @@ public class ApiServletTest {
@Test
public void getClientAddressWithXForwardedFor() throws UnknownHostException {
+ String[] proxynet = {"127.0.0.0/8"};
+ Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+ Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
Mockito.when(request.getHeader(Mockito.eq("X-Forwarded-For"))).thenReturn("192.168.1.1");
- Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+ Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
}
@Test
public void getClientAddressWithHttpXForwardedFor() throws UnknownHostException {
+ String[] proxynet = {"127.0.0.0/8"};
+ Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+ Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
Mockito.when(request.getHeader(Mockito.eq("HTTP_X_FORWARDED_FOR"))).thenReturn("192.168.1.1");
- Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+ Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
}
@Test
- public void getClientAddressWithXRemoteAddr() throws UnknownHostException {
- Mockito.when(request.getHeader(Mockito.eq("Remote_Addr"))).thenReturn("192.168.1.1");
- Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+ public void getClientAddressWithRemoteAddr() throws UnknownHostException {
+ String[] proxynet = {"127.0.0.0/8"};
+ Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+ Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
+ Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
}
@Test
public void getClientAddressWithHttpClientIp() throws UnknownHostException {
+ String[] proxynet = {"127.0.0.0/8"};
+ Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
+ Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
Mockito.when(request.getHeader(Mockito.eq("HTTP_CLIENT_IP"))).thenReturn("192.168.1.1");
- Assert.assertEquals(InetAddress.getByName("192.168.1.1"), ApiServlet.getClientAddress(request));
+ Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
}
@Test
public void getClientAddressDefault() throws UnknownHostException {
Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
- Assert.assertEquals(InetAddress.getByName("127.0.0.1"), ApiServlet.getClientAddress(request));
+ Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
}
@Test
diff --git a/utils/src/main/java/com/cloud/utils/StringUtils.java b/utils/src/main/java/com/cloud/utils/StringUtils.java
index 9e197a8a94b..93e66ecfc02 100644
--- a/utils/src/main/java/com/cloud/utils/StringUtils.java
+++ b/utils/src/main/java/com/cloud/utils/StringUtils.java
@@ -27,7 +27,7 @@ import java.util.Map;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
-public class StringUtils {
+public class StringUtils extends org.apache.commons.lang3.StringUtils {
private static final char[] hexChar = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
private static Charset preferredACSCharset;