diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapper.java index cbc1e3daeb4..32d731de35c 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapper.java @@ -46,7 +46,7 @@ import com.cloud.vm.VirtualMachine; import com.cloud.utils.ssh.SshHelper; @ResourceWrapper(handles = StartCommand.class) -public final class LibvirtStartCommandWrapper extends CommandWrapper { +public class LibvirtStartCommandWrapper extends CommandWrapper { private static final Logger s_logger = Logger.getLogger(LibvirtStartCommandWrapper.class); private static final String VNC_CONF_FILE_LOCATION = "/root/vncport"; @@ -119,8 +119,10 @@ public final class LibvirtStartCommandWrapper extends CommandWrapper " + VNC_CONF_FILE_LOCATION; for (int retries = 1; retries <= 3; retries++) { diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index b951f997909..aa3fa6adb98 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -27,6 +27,7 @@ import static org.mockito.ArgumentMatchers.nullable; import static org.mockito.Matchers.any; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -5330,6 +5331,9 @@ public class LibvirtComputingResourceTest { when(libvirtComputingResource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); try { when(libvirtUtilitiesHelper.getConnectionByType(vmDef.getHvsType())).thenReturn(conn); + when(libvirtUtilitiesHelper.retrieveSshPrvKeyPath()).thenReturn(LibvirtComputingResource.SSHPRVKEYPATH); + File pemFile = mock(File.class); + PowerMockito.whenNew(File.class).withAnyArguments().thenReturn(pemFile); when(conn.listDomains()).thenReturn(vms); doNothing().when(libvirtComputingResource).createVbd(conn, vmSpec, vmName, vmDef); } catch (final LibvirtException e) { @@ -5338,6 +5342,8 @@ public class LibvirtComputingResourceTest { fail(e.getMessage()); } catch (final URISyntaxException e) { fail(e.getMessage()); + } catch (Exception e) { + fail(e.getMessage()); } when(storagePoolMgr.connectPhysicalDisksViaVmSpec(vmSpec)).thenReturn(true); @@ -5404,6 +5410,9 @@ public class LibvirtComputingResourceTest { when(libvirtComputingResource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); try { when(libvirtUtilitiesHelper.getConnectionByType(vmDef.getHvsType())).thenReturn(conn); + when(libvirtUtilitiesHelper.retrieveSshPrvKeyPath()).thenReturn(LibvirtComputingResource.SSHPRVKEYPATH); + File pemFile = mock(File.class); + PowerMockito.whenNew(File.class).withAnyArguments().thenReturn(pemFile); when(conn.listDomains()).thenReturn(vms); doNothing().when(libvirtComputingResource).createVbd(conn, vmSpec, vmName, vmDef); } catch (final LibvirtException e) { @@ -5412,6 +5421,8 @@ public class LibvirtComputingResourceTest { fail(e.getMessage()); } catch (final URISyntaxException e) { fail(e.getMessage()); + } catch (Exception e) { + fail(e.getMessage()); } when(storagePoolMgr.connectPhysicalDisksViaVmSpec(vmSpec)).thenReturn(true); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapperTest.java new file mode 100644 index 00000000000..02780e0f5ea --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartCommandWrapperTest.java @@ -0,0 +1,59 @@ +// +// 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 com.cloud.hypervisor.kvm.resource.wrapper; + +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.utils.Pair; +import com.cloud.utils.ssh.SshHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import java.io.File; + +@RunWith(PowerMockRunner.class) +@PrepareForTest(SshHelper.class) +public class LibvirtStartCommandWrapperTest { + + @Mock + private File pemFile; + + @Spy + private LibvirtStartCommandWrapper wrapper = new LibvirtStartCommandWrapper(); + + private static final String CONTROL_IP = "169.254.106.231"; + private static final int SSH_PORT = Integer.parseInt(LibvirtComputingResource.DEFAULTDOMRSSHPORT); + + @Test + @PrepareForTest(SshHelper.class) + public void testConfigureVncPortOnCpvm() throws Exception { + PowerMockito.mockStatic(SshHelper.class); + PowerMockito.when(SshHelper.sshExecute(Mockito.eq(CONTROL_IP), Mockito.eq(SSH_PORT), Mockito.eq("root"), + Mockito.eq(pemFile), Mockito.nullable(String.class), Mockito.anyString())) + .thenReturn(new Pair<>(true, "")); + String port = "8081"; + PowerMockito.mockStatic(SshHelper.class); + wrapper.configureVncPortOnCpvm(port, CONTROL_IP, pemFile); + } +} diff --git a/server/src/main/java/com/cloud/api/ApiServlet.java b/server/src/main/java/com/cloud/api/ApiServlet.java index a7a0b622468..1e6117984f4 100644 --- a/server/src/main/java/com/cloud/api/ApiServlet.java +++ b/server/src/main/java/com/cloud/api/ApiServlet.java @@ -321,16 +321,7 @@ public class ApiServlet extends HttpServlet { // Add the HTTP method (GET/POST/PUT/DELETE) as well into the params map. params.put("httpmethod", new String[]{req.getMethod()}); setProjectContext(params); - if (org.apache.commons.lang3.StringUtils.isNotBlank(command) && - command.equalsIgnoreCase(CreateConsoleEndpointCmd.APINAME)) { - InetAddress addr = getClientAddress(req); - String clientAddress = addr != null ? addr.getHostAddress() : null; - params.put(ConsoleAccessUtils.CLIENT_INET_ADDRESS_KEY, new String[]{clientAddress}); - if (BooleanUtils.isTrue(ConsoleAccessManager.ConsoleProxyExtraSecurityHeaderEnabled.value())) { - String clientSecurityToken = req.getHeader(ConsoleAccessManager.ConsoleProxyExtraSecurityHeaderName.value()); - params.put(ConsoleAccessUtils.CLIENT_SECURITY_HEADER_PARAM_KEY, new String[]{clientSecurityToken}); - } - } + setExtraHeaderSecurityToConsoleEndpointIfEnabled(command, params, req); final String response = apiServer.handleRequest(params, responseType, auditTrailSb); HttpUtils.writeHttpResponse(resp, response != null ? response : "", HttpServletResponse.SC_OK, responseType, ApiServer.JSONcontentType.value()); } else { @@ -366,6 +357,19 @@ public class ApiServlet extends HttpServlet { } } + protected void setExtraHeaderSecurityToConsoleEndpointIfEnabled(String command, Map params, HttpServletRequest req) throws UnknownHostException { + if (org.apache.commons.lang3.StringUtils.isNotBlank(command) && + command.equalsIgnoreCase(CreateConsoleEndpointCmd.APINAME)) { + InetAddress addr = getClientAddress(req); + String clientAddress = addr != null ? addr.getHostAddress() : null; + params.put(ConsoleAccessUtils.CLIENT_INET_ADDRESS_KEY, new String[]{clientAddress}); + if (BooleanUtils.isTrue(ConsoleAccessManager.ConsoleProxyExtraSecurityHeaderEnabled.value())) { + String clientSecurityToken = req.getHeader(ConsoleAccessManager.ConsoleProxyExtraSecurityHeaderName.value()); + params.put(ConsoleAccessUtils.CLIENT_SECURITY_HEADER_PARAM_KEY, new String[]{clientSecurityToken}); + } + } + } + private void setProjectContext(Map requestParameters) { final String[] command = (String[])requestParameters.get(ApiConstants.COMMAND); if (command == null) { diff --git a/server/src/main/java/com/cloud/consoleproxy/AgentHookBase.java b/server/src/main/java/com/cloud/consoleproxy/AgentHookBase.java index 8064b8b7114..619825ecf43 100644 --- a/server/src/main/java/com/cloud/consoleproxy/AgentHookBase.java +++ b/server/src/main/java/com/cloud/consoleproxy/AgentHookBase.java @@ -22,6 +22,7 @@ import java.security.SecureRandom; import java.util.Date; import org.apache.cloudstack.consoleproxy.ConsoleAccessManager; +import org.apache.cloudstack.consoleproxy.ConsoleAccessManagerImpl; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.framework.security.keys.KeysManager; import org.apache.cloudstack.framework.security.keystore.KeystoreManager; diff --git a/server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java b/server/src/main/java/org/apache/cloudstack/consoleproxy/ConsoleAccessManagerImpl.java similarity index 98% rename from server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java rename to server/src/main/java/org/apache/cloudstack/consoleproxy/ConsoleAccessManagerImpl.java index 3cf0cbfc92d..14bc20139f2 100644 --- a/server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/consoleproxy/ConsoleAccessManagerImpl.java @@ -14,12 +14,13 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -package com.cloud.consoleproxy; +package org.apache.cloudstack.consoleproxy; import com.cloud.agent.AgentManager; import com.cloud.agent.api.Answer; import com.cloud.agent.api.GetVmVncTicketAnswer; import com.cloud.agent.api.GetVmVncTicketCommand; +import com.cloud.consoleproxy.ConsoleProxyManager; import com.cloud.exception.AgentUnavailableException; import com.cloud.exception.OperationTimedoutException; import com.cloud.exception.PermissionDeniedException; @@ -45,7 +46,6 @@ import com.cloud.vm.dao.UserVmDetailsDao; import com.google.gson.Gson; import com.google.gson.GsonBuilder; import org.apache.cloudstack.api.command.user.consoleproxy.ConsoleEndpoint; -import org.apache.cloudstack.consoleproxy.ConsoleAccessManager; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.security.keys.KeysManager; @@ -122,7 +122,7 @@ public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAcce return new ConsoleEndpoint(false, null, "Cannot find VM with ID " + vmId); } - if (!checkSessionPermision(vm, account)) { + if (!checkSessionPermission(vm, account)) { return new ConsoleEndpoint(false, null, "Permission denied"); } @@ -146,7 +146,7 @@ public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAcce } } - private boolean checkSessionPermision(VirtualMachine vm, Account account) { + protected boolean checkSessionPermission(VirtualMachine vm, Account account) { if (accountManager.isRootAdmin(account.getId())) { return true; } diff --git a/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml b/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml index e34ea66da44..2604f8ed127 100644 --- a/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml +++ b/server/src/main/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml @@ -107,7 +107,7 @@ value="#{consoleProxyAllocatorsRegistry.registered}" /> - + diff --git a/server/src/test/java/com/cloud/api/ApiServletTest.java b/server/src/test/java/com/cloud/api/ApiServletTest.java index fa582991e6b..804340c0728 100644 --- a/server/src/test/java/com/cloud/api/ApiServletTest.java +++ b/server/src/test/java/com/cloud/api/ApiServletTest.java @@ -16,6 +16,8 @@ // under the License. package com.cloud.api; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.nullable; import java.io.IOException; @@ -23,6 +25,7 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.io.UnsupportedEncodingException; import java.lang.reflect.Field; +import java.lang.reflect.Modifier; import java.net.InetAddress; import java.net.URLEncoder; import java.net.UnknownHostException; @@ -37,6 +40,10 @@ import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.auth.APIAuthenticationManager; import org.apache.cloudstack.api.auth.APIAuthenticationType; import org.apache.cloudstack.api.auth.APIAuthenticator; +import org.apache.cloudstack.api.command.user.consoleproxy.CreateConsoleEndpointCmd; +import org.apache.cloudstack.consoleproxy.ConsoleAccessManager; +import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.utils.consoleproxy.ConsoleAccessUtils; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -44,14 +51,17 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.Mockito; -import org.mockito.junit.MockitoJUnitRunner; import com.cloud.server.ManagementServer; import com.cloud.user.Account; import com.cloud.user.AccountService; import com.cloud.user.User; +import org.mockito.junit.MockitoJUnitRunner; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; @RunWith(MockitoJUnitRunner.class) +@PrepareForTest(ConsoleAccessManager.class) public class ApiServletTest { @Mock @@ -84,6 +94,15 @@ public class ApiServletTest { @Mock ManagementServer managementServer; + @Mock + private ConfigKey enableSecurityHeaderSetting; + + @Mock + private ConfigKey securityHeaderNameSetting; + + @Mock + private HashMap params; + StringWriter responseWriter; ApiServlet servlet; @@ -273,4 +292,50 @@ public class ApiServletTest { Assert.assertEquals(InetAddress.getByName("127.0.0.1"), ApiServlet.getClientAddress(request)); } + @Test + public void testSetExtraHeaderSecurityToConsoleEndpointIfEnabledNotMatchingCommand() throws UnknownHostException { + HashMap params = Mockito.mock(HashMap.class); + PowerMockito.mockStatic(InetAddress.class); + servlet.setExtraHeaderSecurityToConsoleEndpointIfEnabled("login", params, request); + Mockito.verify(params, Mockito.never()).put(anyString(), any()); + } + + private void replaceConsoleAccessSetting(String settingName, ConfigKey replaceKey) throws Exception { + Field field = ConsoleAccessManager.class.getDeclaredField(settingName); + field.setAccessible(true); + // remove final modifier from field + Field modifiersField = Field.class.getDeclaredField("modifiers"); + modifiersField.setAccessible(true); + modifiersField.setInt(field, field.getModifiers() & ~Modifier.FINAL); + field.set(null, replaceKey); + } + + @Test + public void testSetExtraHeaderSecurityToConsoleEndpointIfEnabledDisabledSetting() throws Exception { + String ip = "127.0.0.1"; + Mockito.when(request.getHeader("Remote_Addr")).thenReturn(ip); + PowerMockito.mockStatic(InetAddress.class); + Mockito.when(enableSecurityHeaderSetting.value()).thenReturn(false); + replaceConsoleAccessSetting("ConsoleProxyExtraSecurityHeaderEnabled", enableSecurityHeaderSetting); + servlet.setExtraHeaderSecurityToConsoleEndpointIfEnabled(CreateConsoleEndpointCmd.APINAME, params, request); + Mockito.verify(params).put(ConsoleAccessUtils.CLIENT_INET_ADDRESS_KEY, new String[] { ip }); + } + + @Test + public void testSetExtraHeaderSecurityToConsoleEndpointIfEnabledEnabledSetting() throws Exception { + String ip = "127.0.0.1"; + String headerName = "HEADER_NAME"; + String headerValue = "HEADER_VALUE"; + Mockito.when(request.getHeader("Remote_Addr")).thenReturn(ip); + Mockito.when(request.getHeader(headerName)).thenReturn(headerValue); + PowerMockito.mockStatic(InetAddress.class); + Mockito.when(enableSecurityHeaderSetting.value()).thenReturn(true); + replaceConsoleAccessSetting("ConsoleProxyExtraSecurityHeaderEnabled", enableSecurityHeaderSetting); + Mockito.when(securityHeaderNameSetting.value()).thenReturn(headerName); + replaceConsoleAccessSetting("ConsoleProxyExtraSecurityHeaderName", securityHeaderNameSetting); + servlet.setExtraHeaderSecurityToConsoleEndpointIfEnabled(CreateConsoleEndpointCmd.APINAME, params, request); + Mockito.verify(params).put(ConsoleAccessUtils.CLIENT_INET_ADDRESS_KEY, new String[] { ip }); + Mockito.verify(params).put(ConsoleAccessUtils.CLIENT_SECURITY_HEADER_PARAM_KEY, new String[] { headerValue }); + } + } diff --git a/server/src/test/java/org/apache/cloudstack/consoleproxy/ConsoleAccessManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/consoleproxy/ConsoleAccessManagerImplTest.java new file mode 100644 index 00000000000..df218f49bfd --- /dev/null +++ b/server/src/test/java/org/apache/cloudstack/consoleproxy/ConsoleAccessManagerImplTest.java @@ -0,0 +1,109 @@ +// 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.consoleproxy; + +import com.cloud.agent.AgentManager; +import com.cloud.exception.PermissionDeniedException; +import com.cloud.server.ManagementServer; +import com.cloud.user.Account; +import com.cloud.user.AccountManager; +import com.cloud.utils.db.EntityManager; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.VirtualMachineManager; +import com.cloud.vm.dao.UserVmDetailsDao; +import org.apache.cloudstack.acl.SecurityChecker; +import org.apache.cloudstack.framework.security.keys.KeysManager; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import java.util.Arrays; +import java.util.List; + +@RunWith(MockitoJUnitRunner.class) +public class ConsoleAccessManagerImplTest { + + @Mock + private AccountManager accountManager; + @Mock + private VirtualMachineManager virtualMachineManager; + @Mock + private ManagementServer managementServer; + @Mock + private EntityManager entityManager; + @Mock + private UserVmDetailsDao userVmDetailsDao; + @Mock + private KeysManager keysManager; + @Mock + private AgentManager agentManager; + + @Spy + @InjectMocks + ConsoleAccessManagerImpl consoleAccessManager = new ConsoleAccessManagerImpl(); + + @Mock + VirtualMachine virtualMachine; + @Mock + Account account; + + @Test + public void testCheckSessionPermissionAdminAccount() { + Mockito.when(account.getId()).thenReturn(1L); + Mockito.when(accountManager.isRootAdmin(1L)).thenReturn(true); + Assert.assertTrue(consoleAccessManager.checkSessionPermission(virtualMachine, account)); + } + + @Test + public void testCheckSessionPermissionUserOwnedVm() { + Mockito.when(account.getId()).thenReturn(1L); + Mockito.when(accountManager.isRootAdmin(1L)).thenReturn(false); + Mockito.when(virtualMachine.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.doNothing().when(accountManager).checkAccess( + Mockito.eq(account), Mockito.nullable(SecurityChecker.AccessType.class), + Mockito.eq(true), Mockito.eq(virtualMachine)); + Assert.assertTrue(consoleAccessManager.checkSessionPermission(virtualMachine, account)); + } + + @Test + public void testCheckSessionPermissionDifferentUserOwnedVm() { + Mockito.when(account.getId()).thenReturn(1L); + Mockito.when(accountManager.isRootAdmin(1L)).thenReturn(false); + Mockito.when(virtualMachine.getType()).thenReturn(VirtualMachine.Type.User); + Mockito.doThrow(PermissionDeniedException.class).when(accountManager).checkAccess( + Mockito.eq(account), Mockito.nullable(SecurityChecker.AccessType.class), + Mockito.eq(true), Mockito.eq(virtualMachine)); + Assert.assertFalse(consoleAccessManager.checkSessionPermission(virtualMachine, account)); + } + + @Test + public void testCheckSessionPermissionForUsersOnSystemVms() { + Mockito.when(account.getId()).thenReturn(1L); + Mockito.when(accountManager.isRootAdmin(1L)).thenReturn(false); + List systemVmTypes = Arrays.asList(VirtualMachine.Type.DomainRouter, + VirtualMachine.Type.ConsoleProxy, VirtualMachine.Type.SecondaryStorageVm); + for (VirtualMachine.Type type : systemVmTypes) { + Mockito.when(virtualMachine.getType()).thenReturn(type); + Assert.assertFalse(consoleAccessManager.checkSessionPermission(virtualMachine, account)); + } + } +}