[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #6577: [WIP] New API: createConsoleURL

[GitBox <gi...@apache.org>]

JoaoJandre commented on code in PR #6577:
URL: https://github.com/apache/cloudstack/pull/6577#discussion_r937007741


##########
server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java:
##########
@@ -0,0 +1,467 @@
+// 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.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.exception.AgentUnavailableException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.host.HostVO;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceState;
+import com.cloud.server.ManagementServer;
+import com.cloud.servlet.ConsoleProxyClientParam;
+import com.cloud.servlet.ConsoleProxyPasswordBasedEncryptor;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.EntityManager;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.UserVmDetailVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.VirtualMachineManager;
+import com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+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;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.net.InetAddress;
+import java.util.Date;
+
+import java.util.Map;
+
+public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAccessManager {
+
+    @Inject
+    private AccountManager _accountMgr;
+    @Inject
+    private VirtualMachineManager _vmMgr;
+    @Inject
+    private ManagementServer _ms;
+    @Inject
+    private EntityManager _entityMgr;
+    @Inject
+    private UserVmDetailsDao _userVmDetailsDao;
+    @Inject
+    private KeysManager _keysMgr;
+    @Inject
+    private AgentManager agentManager;
+
+    private static KeysManager s_keysMgr;
+    private final Gson _gson = new GsonBuilder().create();
+
+    public static final Logger s_logger = Logger.getLogger(ConsoleAccessManagerImpl.class.getName());
+
+    @Override
+    public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+        s_keysMgr = _keysMgr;
+        return super.configure(name, params);
+    }
+
+    @Override
+    public Pair<Boolean, String> generateConsoleUrl(Long vmId) {
+        try {
+            if (_accountMgr == null || _vmMgr == null || _ms == null) {
+                return new Pair<>(false, "Console service is not ready");
+            }
+
+            if (_keysMgr.getHashKey() == null) {
+                String msg = "Console access denied. Ticket service is not ready yet";
+                s_logger.debug(msg);
+                return new Pair<>(false, msg);
+            }
+
+            Account account = CallContext.current().getCallingAccount();
+
+            // Do a sanity check here to make sure the user hasn't already been deleted
+            if (account == null) {
+                s_logger.debug("Invalid user/account, reject console access");
+                return new Pair<>(false, "Access denied. Invalid or inconsistent account is found");
+            }
+
+            VirtualMachine vm = _entityMgr.findById(VirtualMachine.class, vmId);
+            if (vm == null) {
+                s_logger.info("Invalid console servlet command parameter: " + vmId);
+                return new Pair<>(false, "Cannot find VM with ID " + vmId);
+            }
+
+            if (!checkSessionPermision(vm, account)) {
+                return new Pair<>(false, "Permission denied");
+            }
+
+            return new Pair<>(true, generateAccessUrl(vmId));
+        } catch (Throwable e) {
+            s_logger.error("Unexepected exception in ConsoleProxyServlet", e);

Review Comment:
   ```suggestion
               s_logger.error("Unexpected exception in ConsoleProxyServlet", e);
   ```



##########
server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java:
##########
@@ -0,0 +1,467 @@
+// 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.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.exception.AgentUnavailableException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.host.HostVO;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceState;
+import com.cloud.server.ManagementServer;
+import com.cloud.servlet.ConsoleProxyClientParam;
+import com.cloud.servlet.ConsoleProxyPasswordBasedEncryptor;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.EntityManager;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.UserVmDetailVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.VirtualMachineManager;
+import com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+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;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.net.InetAddress;
+import java.util.Date;
+
+import java.util.Map;
+
+public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAccessManager {
+
+    @Inject
+    private AccountManager _accountMgr;
+    @Inject
+    private VirtualMachineManager _vmMgr;
+    @Inject
+    private ManagementServer _ms;
+    @Inject
+    private EntityManager _entityMgr;
+    @Inject
+    private UserVmDetailsDao _userVmDetailsDao;
+    @Inject
+    private KeysManager _keysMgr;
+    @Inject
+    private AgentManager agentManager;
+
+    private static KeysManager s_keysMgr;
+    private final Gson _gson = new GsonBuilder().create();
+
+    public static final Logger s_logger = Logger.getLogger(ConsoleAccessManagerImpl.class.getName());
+
+    @Override
+    public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+        s_keysMgr = _keysMgr;
+        return super.configure(name, params);
+    }
+
+    @Override
+    public Pair<Boolean, String> generateConsoleUrl(Long vmId) {
+        try {
+            if (_accountMgr == null || _vmMgr == null || _ms == null) {
+                return new Pair<>(false, "Console service is not ready");
+            }
+
+            if (_keysMgr.getHashKey() == null) {
+                String msg = "Console access denied. Ticket service is not ready yet";
+                s_logger.debug(msg);
+                return new Pair<>(false, msg);
+            }
+
+            Account account = CallContext.current().getCallingAccount();
+
+            // Do a sanity check here to make sure the user hasn't already been deleted
+            if (account == null) {
+                s_logger.debug("Invalid user/account, reject console access");
+                return new Pair<>(false, "Access denied. Invalid or inconsistent account is found");
+            }
+
+            VirtualMachine vm = _entityMgr.findById(VirtualMachine.class, vmId);
+            if (vm == null) {
+                s_logger.info("Invalid console servlet command parameter: " + vmId);
+                return new Pair<>(false, "Cannot find VM with ID " + vmId);
+            }
+
+            if (!checkSessionPermision(vm, account)) {
+                return new Pair<>(false, "Permission denied");
+            }
+
+            return new Pair<>(true, generateAccessUrl(vmId));
+        } catch (Throwable e) {
+            s_logger.error("Unexepected exception in ConsoleProxyServlet", e);
+            return new Pair<>(false, "Server Internal Error: " + e.getMessage());
+        }
+    }
+
+    private boolean checkSessionPermision(VirtualMachine vm, Account account) {
+        if (_accountMgr.isRootAdmin(account.getId())) {
+            return true;
+        }
+
+        switch (vm.getType()) {
+            case User:
+                try {
+                    _accountMgr.checkAccess(account, null, true, vm);
+                } catch (PermissionDeniedException ex) {
+                    if (_accountMgr.isNormalUser(account.getId())) {
+                        if (s_logger.isDebugEnabled()) {
+                            s_logger.debug("VM access is denied. VM owner account " + vm.getAccountId() + " does not match the account id in session " +
+                                    account.getId() + " and caller is a normal user");
+                        }
+                    } else if (_accountMgr.isDomainAdmin(account.getId())
+                            || account.getType() == Account.Type.READ_ONLY_ADMIN) {
+                        if(s_logger.isDebugEnabled()) {
+                            s_logger.debug("VM access is denied. VM owner account " + vm.getAccountId()
+                                    + " does not match the account id in session " + account.getId() + " and the domain-admin caller does not manage the target domain");
+                        }
+                    }
+                    return false;
+                }
+                break;
+
+            case DomainRouter:
+            case ConsoleProxy:
+            case SecondaryStorageVm:
+                return false;
+
+            default:
+                s_logger.warn("Unrecoginized virtual machine type, deny access by default. type: " + vm.getType());

Review Comment:
   ```suggestion
                   s_logger.warn("Unrecognized virtual machine type, deny access by default. type: " + vm.getType());
   ```



##########
server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java:
##########
@@ -0,0 +1,467 @@
+// 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.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.exception.AgentUnavailableException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.host.HostVO;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceState;
+import com.cloud.server.ManagementServer;
+import com.cloud.servlet.ConsoleProxyClientParam;
+import com.cloud.servlet.ConsoleProxyPasswordBasedEncryptor;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.EntityManager;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.UserVmDetailVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.VirtualMachineManager;
+import com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+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;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.net.InetAddress;
+import java.util.Date;
+
+import java.util.Map;
+
+public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAccessManager {
+
+    @Inject
+    private AccountManager _accountMgr;
+    @Inject
+    private VirtualMachineManager _vmMgr;
+    @Inject
+    private ManagementServer _ms;
+    @Inject
+    private EntityManager _entityMgr;
+    @Inject
+    private UserVmDetailsDao _userVmDetailsDao;
+    @Inject
+    private KeysManager _keysMgr;
+    @Inject
+    private AgentManager agentManager;
+
+    private static KeysManager s_keysMgr;
+    private final Gson _gson = new GsonBuilder().create();
+
+    public static final Logger s_logger = Logger.getLogger(ConsoleAccessManagerImpl.class.getName());
+
+    @Override
+    public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+        s_keysMgr = _keysMgr;
+        return super.configure(name, params);
+    }
+
+    @Override
+    public Pair<Boolean, String> generateConsoleUrl(Long vmId) {
+        try {
+            if (_accountMgr == null || _vmMgr == null || _ms == null) {
+                return new Pair<>(false, "Console service is not ready");
+            }
+
+            if (_keysMgr.getHashKey() == null) {
+                String msg = "Console access denied. Ticket service is not ready yet";
+                s_logger.debug(msg);
+                return new Pair<>(false, msg);
+            }
+
+            Account account = CallContext.current().getCallingAccount();
+
+            // Do a sanity check here to make sure the user hasn't already been deleted
+            if (account == null) {
+                s_logger.debug("Invalid user/account, reject console access");
+                return new Pair<>(false, "Access denied. Invalid or inconsistent account is found");
+            }
+
+            VirtualMachine vm = _entityMgr.findById(VirtualMachine.class, vmId);
+            if (vm == null) {
+                s_logger.info("Invalid console servlet command parameter: " + vmId);
+                return new Pair<>(false, "Cannot find VM with ID " + vmId);
+            }
+
+            if (!checkSessionPermision(vm, account)) {
+                return new Pair<>(false, "Permission denied");
+            }
+
+            return new Pair<>(true, generateAccessUrl(vmId));
+        } catch (Throwable e) {
+            s_logger.error("Unexepected exception in ConsoleProxyServlet", e);
+            return new Pair<>(false, "Server Internal Error: " + e.getMessage());
+        }
+    }
+
+    private boolean checkSessionPermision(VirtualMachine vm, Account account) {
+        if (_accountMgr.isRootAdmin(account.getId())) {
+            return true;
+        }
+
+        switch (vm.getType()) {
+            case User:
+                try {
+                    _accountMgr.checkAccess(account, null, true, vm);
+                } catch (PermissionDeniedException ex) {
+                    if (_accountMgr.isNormalUser(account.getId())) {
+                        if (s_logger.isDebugEnabled()) {
+                            s_logger.debug("VM access is denied. VM owner account " + vm.getAccountId() + " does not match the account id in session " +
+                                    account.getId() + " and caller is a normal user");
+                        }
+                    } else if (_accountMgr.isDomainAdmin(account.getId())
+                            || account.getType() == Account.Type.READ_ONLY_ADMIN) {
+                        if(s_logger.isDebugEnabled()) {
+                            s_logger.debug("VM access is denied. VM owner account " + vm.getAccountId()
+                                    + " does not match the account id in session " + account.getId() + " and the domain-admin caller does not manage the target domain");
+                        }
+                    }
+                    return false;
+                }
+                break;
+
+            case DomainRouter:
+            case ConsoleProxy:
+            case SecondaryStorageVm:
+                return false;
+
+            default:
+                s_logger.warn("Unrecoginized virtual machine type, deny access by default. type: " + vm.getType());
+                return false;
+        }
+
+        return true;
+    }
+
+    private String generateAccessUrl(Long vmId) {
+        VirtualMachine vm = _vmMgr.findById(vmId);
+        String msg;
+        if (vm == null) {
+            msg = "VM " + vmId + " does not exist, sending blank response for console access request";
+            s_logger.warn(msg);
+            throw new CloudRuntimeException(msg);
+        }
+
+        if (vm.getHostId() == null) {
+            msg = "VM " + vmId + " lost host info, sending blank response for console access request";
+            s_logger.warn(msg);
+            throw new CloudRuntimeException(msg);
+        }
+
+        HostVO host = _ms.getHostBy(vm.getHostId());
+        if (host == null) {
+            msg = "VM " + vmId + "'s host does not exist, sending blank response for console access request";
+            s_logger.warn(msg);
+            throw new CloudRuntimeException(msg);
+        }
+
+        if (Hypervisor.HypervisorType.LXC.equals(vm.getHypervisorType())) {
+            throw new CloudRuntimeException("Console access is not supported for LXC");
+        }
+
+        String rootUrl = _ms.getConsoleAccessUrlRoot(vmId);
+        if (rootUrl == null) {
+            throw new CloudRuntimeException("Console access will be ready in a few minutes. Please try it again later.");
+        }
+
+        String vmName = vm.getHostName();
+        if (vm.getType() == VirtualMachine.Type.User) {
+            UserVm userVm = _entityMgr.findById(UserVm.class, vmId);
+            String displayName = userVm.getDisplayName();
+            if (displayName != null && !displayName.isEmpty() && !displayName.equals(vmName)) {
+                vmName += "(" + displayName + ")";
+            }
+        }
+
+//        InetAddress remoteAddress = null;
+//        try {
+//            remoteAddress = ApiServlet.getClientAddress();
+//        } catch (UnknownHostException e) {
+//            s_logger.warn("UnknownHostException when trying to lookup remote IP-Address. This should never happen. Blocking request.", e);
+//        }
+
+        String url = composeConsoleAccessUrl(rootUrl, vm, host, null);
+        s_logger.debug("The console URL is: " + url);
+        return url;
+    }
+
+    public static final String escapeHTML(String content) {
+        if (content == null || content.isEmpty())
+            return content;
+
+        StringBuilder sb = new StringBuilder();
+        for (int i = 0; i < content.length(); i++) {
+            char c = content.charAt(i);
+            switch (c) {
+                case '<':
+                    sb.append("&lt;");
+                    break;
+                case '>':
+                    sb.append("&gt;");
+                    break;
+                case '&':
+                    sb.append("&amp;");
+                    break;
+                case '"':
+                    sb.append("&quot;");
+                    break;
+                case ' ':
+                    sb.append("&nbsp;");
+                    break;
+                default:
+                    sb.append(c);
+                    break;
+            }
+        }
+        return sb.toString();
+    }
+
+    private String composeConsoleAccessUrl(String rootUrl, VirtualMachine vm, HostVO hostVo, InetAddress addr) {
+        StringBuffer sb = new StringBuffer(rootUrl);
+        String host = hostVo.getPrivateIpAddress();
+
+        Pair<String, Integer> portInfo = null;
+        if (hostVo.getHypervisorType() == Hypervisor.HypervisorType.KVM &&
+                (hostVo.getResourceState().equals(ResourceState.ErrorInMaintenance) ||
+                        hostVo.getResourceState().equals(ResourceState.ErrorInPrepareForMaintenance))) {
+            UserVmDetailVO detailAddress = _userVmDetailsDao.findDetail(vm.getId(), VmDetailConstants.KVM_VNC_ADDRESS);
+            UserVmDetailVO detailPort = _userVmDetailsDao.findDetail(vm.getId(), VmDetailConstants.KVM_VNC_PORT);
+            if (detailAddress != null && detailPort != null) {
+                portInfo = new Pair<>(detailAddress.getValue(), Integer.valueOf(detailPort.getValue()));
+            } else {
+                s_logger.warn("KVM Host in ErrorInMaintenance/ErrorInPrepareForMaintenance but " +
+                        "no VNC Address/Port was available. Falling back to default one from MS.");
+            }
+        }
+
+        if (portInfo == null) {
+            portInfo = _ms.getVncPort(vm);
+        }
+
+        if (s_logger.isDebugEnabled())
+            s_logger.debug("Port info " + portInfo.first());
+
+        Ternary<String, String, String> parsedHostInfo = parseHostInfo(portInfo.first());
+
+        int port = -1;
+        if (portInfo.second() == -9) {
+            //for hyperv
+            port = Integer.parseInt(_ms.findDetail(hostVo.getId(), "rdp.server.port").getValue());
+        } else {
+            port = portInfo.second();
+        }
+
+        String sid = vm.getVncPassword();
+        UserVmDetailVO details = _userVmDetailsDao.findDetail(vm.getId(), VmDetailConstants.KEYBOARD);
+
+        String tag = vm.getUuid();
+
+        String ticket = genAccessTicket(parsedHostInfo.first(), String.valueOf(port), sid, tag);
+        ConsoleProxyPasswordBasedEncryptor encryptor = new ConsoleProxyPasswordBasedEncryptor(getEncryptorPassword());
+        ConsoleProxyClientParam param = new ConsoleProxyClientParam();
+        param.setClientHostAddress(parsedHostInfo.first());
+        param.setClientHostPort(port);
+        param.setClientHostPassword(sid);
+        param.setClientTag(tag);
+        param.setTicket(ticket);
+        param.setSourceIP(addr != null ? addr.getHostAddress(): null);
+
+        if (requiresVncOverWebSocketConnection(vm, hostVo)) {
+            setWebsocketUrl(vm, param);
+        }
+
+        if (details != null) {
+            param.setLocale(details.getValue());
+        }
+
+        if (portInfo.second() == -9) {
+            //For Hyperv Clinet Host Address will send Instance id
+            param.setHypervHost(host);
+            param.setUsername(_ms.findDetail(hostVo.getId(), "username").getValue());
+            param.setPassword(_ms.findDetail(hostVo.getId(), "password").getValue());
+        }
+        if (parsedHostInfo.second() != null  && parsedHostInfo.third() != null) {
+            param.setClientTunnelUrl(parsedHostInfo.second());
+            param.setClientTunnelSession(parsedHostInfo.third());
+        }
+
+        if (param.getHypervHost() != null || !ConsoleProxyManager.NoVncConsoleDefault.value()) {
+            sb.append("/ajax?token=" + encryptor.encryptObject(ConsoleProxyClientParam.class, param));
+        } else {
+            sb.append("/resource/noVNC/vnc.html")
+                    .append("?autoconnect=true")
+                    .append("&port=" + ConsoleProxyManager.DEFAULT_NOVNC_PORT)
+                    .append("&token=" + encryptor.encryptObject(ConsoleProxyClientParam.class, param));
+        }
+
+        // for console access, we need guest OS type to help implement keyboard
+        long guestOs = vm.getGuestOSId();
+        GuestOSVO guestOsVo = _ms.getGuestOs(guestOs);
+        if (guestOsVo.getCategoryId() == 6)
+            sb.append("&guest=windows");
+
+        if (s_logger.isDebugEnabled()) {
+            s_logger.debug("Compose console url: " + sb);
+        }
+        return sb.toString();
+    }
+
+    static public Ternary<String, String, String> parseHostInfo(String hostInfo) {
+        String host = null;
+        String tunnelUrl = null;
+        String tunnelSession = null;
+
+        s_logger.info("Parse host info returned from executing GetVNCPortCommand. host info: " + hostInfo);
+
+        if (hostInfo != null) {
+            if (hostInfo.startsWith("consoleurl")) {
+                String tokens[] = hostInfo.split("&");
+
+                if (hostInfo.length() > 19 && hostInfo.indexOf('/', 19) > 19) {
+                    host = hostInfo.substring(19, hostInfo.indexOf('/', 19)).trim();

Review Comment:
   This 19 should probably be extracted to a variable and documented. We should avoid using ''magical numbers''.



##########
server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java:
##########
@@ -0,0 +1,467 @@
+// 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.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.exception.AgentUnavailableException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.host.HostVO;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceState;
+import com.cloud.server.ManagementServer;
+import com.cloud.servlet.ConsoleProxyClientParam;
+import com.cloud.servlet.ConsoleProxyPasswordBasedEncryptor;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.EntityManager;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.UserVmDetailVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.VirtualMachineManager;
+import com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+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;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.net.InetAddress;
+import java.util.Date;
+
+import java.util.Map;
+
+public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAccessManager {
+
+    @Inject
+    private AccountManager _accountMgr;
+    @Inject
+    private VirtualMachineManager _vmMgr;
+    @Inject
+    private ManagementServer _ms;
+    @Inject
+    private EntityManager _entityMgr;
+    @Inject
+    private UserVmDetailsDao _userVmDetailsDao;
+    @Inject
+    private KeysManager _keysMgr;
+    @Inject
+    private AgentManager agentManager;
+
+    private static KeysManager s_keysMgr;
+    private final Gson _gson = new GsonBuilder().create();
+
+    public static final Logger s_logger = Logger.getLogger(ConsoleAccessManagerImpl.class.getName());
+
+    @Override
+    public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+        s_keysMgr = _keysMgr;
+        return super.configure(name, params);
+    }
+
+    @Override
+    public Pair<Boolean, String> generateConsoleUrl(Long vmId) {
+        try {
+            if (_accountMgr == null || _vmMgr == null || _ms == null) {
+                return new Pair<>(false, "Console service is not ready");
+            }
+
+            if (_keysMgr.getHashKey() == null) {
+                String msg = "Console access denied. Ticket service is not ready yet";
+                s_logger.debug(msg);
+                return new Pair<>(false, msg);
+            }
+
+            Account account = CallContext.current().getCallingAccount();
+
+            // Do a sanity check here to make sure the user hasn't already been deleted
+            if (account == null) {
+                s_logger.debug("Invalid user/account, reject console access");
+                return new Pair<>(false, "Access denied. Invalid or inconsistent account is found");
+            }
+
+            VirtualMachine vm = _entityMgr.findById(VirtualMachine.class, vmId);
+            if (vm == null) {
+                s_logger.info("Invalid console servlet command parameter: " + vmId);
+                return new Pair<>(false, "Cannot find VM with ID " + vmId);
+            }
+
+            if (!checkSessionPermision(vm, account)) {
+                return new Pair<>(false, "Permission denied");
+            }
+
+            return new Pair<>(true, generateAccessUrl(vmId));
+        } catch (Throwable e) {
+            s_logger.error("Unexepected exception in ConsoleProxyServlet", e);
+            return new Pair<>(false, "Server Internal Error: " + e.getMessage());
+        }
+    }
+
+    private boolean checkSessionPermision(VirtualMachine vm, Account account) {
+        if (_accountMgr.isRootAdmin(account.getId())) {
+            return true;
+        }
+
+        switch (vm.getType()) {
+            case User:
+                try {
+                    _accountMgr.checkAccess(account, null, true, vm);
+                } catch (PermissionDeniedException ex) {
+                    if (_accountMgr.isNormalUser(account.getId())) {
+                        if (s_logger.isDebugEnabled()) {
+                            s_logger.debug("VM access is denied. VM owner account " + vm.getAccountId() + " does not match the account id in session " +

Review Comment:
   ```suggestion
                               s_logger.debug("VM access denied. VM owner account " + vm.getAccountId() + " does not match the account id in session " +
   ```



##########
server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java:
##########
@@ -0,0 +1,467 @@
+// 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.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.exception.AgentUnavailableException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.host.HostVO;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceState;
+import com.cloud.server.ManagementServer;
+import com.cloud.servlet.ConsoleProxyClientParam;
+import com.cloud.servlet.ConsoleProxyPasswordBasedEncryptor;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.EntityManager;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.UserVmDetailVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.VirtualMachineManager;
+import com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+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;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.net.InetAddress;
+import java.util.Date;
+
+import java.util.Map;
+
+public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAccessManager {
+
+    @Inject
+    private AccountManager _accountMgr;
+    @Inject
+    private VirtualMachineManager _vmMgr;
+    @Inject
+    private ManagementServer _ms;
+    @Inject
+    private EntityManager _entityMgr;
+    @Inject
+    private UserVmDetailsDao _userVmDetailsDao;
+    @Inject
+    private KeysManager _keysMgr;
+    @Inject
+    private AgentManager agentManager;
+
+    private static KeysManager s_keysMgr;
+    private final Gson _gson = new GsonBuilder().create();
+
+    public static final Logger s_logger = Logger.getLogger(ConsoleAccessManagerImpl.class.getName());
+
+    @Override
+    public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+        s_keysMgr = _keysMgr;
+        return super.configure(name, params);
+    }
+
+    @Override
+    public Pair<Boolean, String> generateConsoleUrl(Long vmId) {
+        try {
+            if (_accountMgr == null || _vmMgr == null || _ms == null) {
+                return new Pair<>(false, "Console service is not ready");
+            }
+
+            if (_keysMgr.getHashKey() == null) {
+                String msg = "Console access denied. Ticket service is not ready yet";
+                s_logger.debug(msg);
+                return new Pair<>(false, msg);
+            }
+
+            Account account = CallContext.current().getCallingAccount();
+
+            // Do a sanity check here to make sure the user hasn't already been deleted
+            if (account == null) {
+                s_logger.debug("Invalid user/account, reject console access");
+                return new Pair<>(false, "Access denied. Invalid or inconsistent account is found");
+            }
+
+            VirtualMachine vm = _entityMgr.findById(VirtualMachine.class, vmId);
+            if (vm == null) {
+                s_logger.info("Invalid console servlet command parameter: " + vmId);
+                return new Pair<>(false, "Cannot find VM with ID " + vmId);
+            }
+
+            if (!checkSessionPermision(vm, account)) {
+                return new Pair<>(false, "Permission denied");
+            }
+
+            return new Pair<>(true, generateAccessUrl(vmId));
+        } catch (Throwable e) {
+            s_logger.error("Unexepected exception in ConsoleProxyServlet", e);
+            return new Pair<>(false, "Server Internal Error: " + e.getMessage());
+        }
+    }
+
+    private boolean checkSessionPermision(VirtualMachine vm, Account account) {
+        if (_accountMgr.isRootAdmin(account.getId())) {
+            return true;
+        }
+
+        switch (vm.getType()) {
+            case User:
+                try {
+                    _accountMgr.checkAccess(account, null, true, vm);
+                } catch (PermissionDeniedException ex) {
+                    if (_accountMgr.isNormalUser(account.getId())) {
+                        if (s_logger.isDebugEnabled()) {
+                            s_logger.debug("VM access is denied. VM owner account " + vm.getAccountId() + " does not match the account id in session " +
+                                    account.getId() + " and caller is a normal user");
+                        }
+                    } else if (_accountMgr.isDomainAdmin(account.getId())
+                            || account.getType() == Account.Type.READ_ONLY_ADMIN) {
+                        if(s_logger.isDebugEnabled()) {
+                            s_logger.debug("VM access is denied. VM owner account " + vm.getAccountId()

Review Comment:
   ```suggestion
                               s_logger.debug("VM access denied. VM owner account " + vm.getAccountId()
   ```



##########
server/src/main/java/com/cloud/consoleproxy/ConsoleAccessManagerImpl.java:
##########
@@ -0,0 +1,467 @@
+// 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.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.exception.AgentUnavailableException;
+import com.cloud.exception.OperationTimedoutException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.host.HostVO;
+import com.cloud.hypervisor.Hypervisor;
+import com.cloud.resource.ResourceState;
+import com.cloud.server.ManagementServer;
+import com.cloud.servlet.ConsoleProxyClientParam;
+import com.cloud.servlet.ConsoleProxyPasswordBasedEncryptor;
+import com.cloud.storage.GuestOSVO;
+import com.cloud.user.Account;
+import com.cloud.user.AccountManager;
+import com.cloud.uservm.UserVm;
+import com.cloud.utils.Pair;
+import com.cloud.utils.Ternary;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.db.EntityManager;
+import com.cloud.utils.exception.CloudRuntimeException;
+import com.cloud.vm.UserVmDetailVO;
+import com.cloud.vm.VirtualMachine;
+import com.cloud.vm.VirtualMachineManager;
+import com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+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;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.net.InetAddress;
+import java.util.Date;
+
+import java.util.Map;
+
+public class ConsoleAccessManagerImpl extends ManagerBase implements ConsoleAccessManager {
+
+    @Inject
+    private AccountManager _accountMgr;
+    @Inject
+    private VirtualMachineManager _vmMgr;
+    @Inject
+    private ManagementServer _ms;
+    @Inject
+    private EntityManager _entityMgr;
+    @Inject
+    private UserVmDetailsDao _userVmDetailsDao;
+    @Inject
+    private KeysManager _keysMgr;
+    @Inject
+    private AgentManager agentManager;
+
+    private static KeysManager s_keysMgr;
+    private final Gson _gson = new GsonBuilder().create();
+
+    public static final Logger s_logger = Logger.getLogger(ConsoleAccessManagerImpl.class.getName());
+
+    @Override
+    public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
+        s_keysMgr = _keysMgr;
+        return super.configure(name, params);
+    }
+
+    @Override
+    public Pair<Boolean, String> generateConsoleUrl(Long vmId) {
+        try {
+            if (_accountMgr == null || _vmMgr == null || _ms == null) {
+                return new Pair<>(false, "Console service is not ready");
+            }
+
+            if (_keysMgr.getHashKey() == null) {
+                String msg = "Console access denied. Ticket service is not ready yet";
+                s_logger.debug(msg);
+                return new Pair<>(false, msg);
+            }
+
+            Account account = CallContext.current().getCallingAccount();
+
+            // Do a sanity check here to make sure the user hasn't already been deleted
+            if (account == null) {
+                s_logger.debug("Invalid user/account, reject console access");
+                return new Pair<>(false, "Access denied. Invalid or inconsistent account is found");
+            }
+
+            VirtualMachine vm = _entityMgr.findById(VirtualMachine.class, vmId);
+            if (vm == null) {
+                s_logger.info("Invalid console servlet command parameter: " + vmId);
+                return new Pair<>(false, "Cannot find VM with ID " + vmId);
+            }
+
+            if (!checkSessionPermision(vm, account)) {
+                return new Pair<>(false, "Permission denied");
+            }
+
+            return new Pair<>(true, generateAccessUrl(vmId));
+        } catch (Throwable e) {

Review Comment:
   Catching `Throwable` is a bad practice, what methods do you worry may throw exceptions? We should catch those specific exceptions instead. `generateAccessUrl()` only throws `CloudRuntimeException`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org