diff --git a/console/novncd b/console/novncd index 995e46fa..636d56ba 100755 --- a/console/novncd +++ b/console/novncd @@ -20,7 +20,7 @@ import socket from urllib.parse import parse_qs, urlparse import argparse -from console.sshtunnels import SSHTunnels +from console.sshtunnels import close_tunnel, open_tunnel, tunnel_target from vrtManager.connection import CONN_SOCKET, CONN_SSH from webvirtcloud.settings import WS_CERT, WS_HOST, WS_PORT @@ -95,29 +95,14 @@ def get_connection_infos(token): instance.name, uuid=instance.uuid, # the VM access was checked for, whatever its name now ) - if instance.compute.hostname.count(":"): - connhost = instance.compute.hostname.split(":")[0] - connport = instance.compute.hostname.split(":")[1] - else: - connhost = instance.compute.hostname - connport = 22 - connuser = instance.compute.login - conntype = instance.compute.type console_host = conn.get_console_listener_addr() console_port = conn.get_console_port() console_socket = conn.get_console_socket() except Exception as e: logging.error("Fail to retrieve console connection infos: %s", e) raise - return ( - connhost, - connport, - connuser, - conntype, - console_host, - console_port, - console_socket, - ) + compute = instance.compute + return compute.hostname, compute.login, compute.type, console_host, console_port, console_socket class CompatibilityMixIn(object): @@ -130,77 +115,36 @@ class CompatibilityMixIn(object): return try: - ( - connhost, - connport, - connuser, - conntype, - console_host, - console_port, - console_socket, - ) = get_connection_infos(token) + connhost, connuser, conntype, console_host, console_port, console_socket = get_connection_infos(token) except Exception as e: self.msg("Console connection denied: %s" % e) return - - cnx_debug_msg = "Connection infos :\n" - cnx_debug_msg += "- connhost : '%s'\n" % connhost - cnx_debug_msg += "- connport : '%s'\n" % connport - cnx_debug_msg += "- connuser : '%s'\n" % connuser - cnx_debug_msg += "- conntype : '%s'\n" % conntype - cnx_debug_msg += "- console_host : '%s'\n" % console_host - cnx_debug_msg += "- console_port : '%s'\n" % console_port - cnx_debug_msg += "- console_socket : '%s'\n" % console_socket - logging.debug(cnx_debug_msg) + logging.debug( + "Console of %s@%s (type %s): %s:%s, socket %s", + connuser, connhost, conntype, console_host, console_port, console_socket, + ) tunnel = None - if console_socket and conntype == CONN_SOCKET: + loopback = re.match(r"^127\.", console_host or "") + if conntype == CONN_SSH: + # the console travels inside the SSH connection, encrypted, + # whatever address it listens on + target = tunnel_target(console_host, console_port, console_socket) + self.msg("Opening an SSH tunnel to %s@%s, console %s" % (connuser, connhost, target)) + tsock, tunnel = open_tunnel(connhost, connuser, target) + elif console_socket and conntype == CONN_SOCKET: # Local socket on local host self.msg("Try to open local socket %s" % console_socket) tsock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) tsock.connect(console_socket) - elif conntype == CONN_SOCKET and re.match(r"^127\.", console_host): + elif conntype == CONN_SOCKET and loopback: # libvirt runs on this host: its loopback is ours self.msg("connecting to: %s:%s" % (console_host, console_port)) tsock = socket_factory(console_host, console_port, connect=True) - elif console_socket or re.match(r"^127\.", console_host): - # Need tunnel to physical host - if conntype != CONN_SSH: - message = "Need a tunnel to access console but can't mount one because it's not a SSH host" - self.msg(message) - raise Exception(message) - try: - # generate a string with all placeholders to avoid TypeErrors - # in sprintf - # https://github.com/retspen/webvirtmgr/pull/497 - error_msg = "Try to open tunnel on %s@%s:%s on console %s:%s " - error_msg += "(or socket %s)" - self.msg( - error_msg - % ( - connuser, - connhost, - connport, - console_host, - console_port, - console_socket, - ) - ) - tunnel = SSHTunnels( - connhost, - connuser, - connport, - console_host, - console_port, - console_socket, - ) - fd = tunnel.open_new() - tunnel.unlock() - tsock = socket.fromfd(fd, socket.AF_INET, socket.SOCK_STREAM) - except Exception as e: - self.msg("Fail to open tunnel : %s" % e) - raise - self.msg("Tunnel opened") + elif console_socket or loopback: + message = "The console listens only on its compute; it is reached through SSH computes only" + self.msg(message) + raise Exception(message) else: # Direct access self.msg("connecting to: %s:%s" % (connhost, console_port)) @@ -209,21 +153,15 @@ class CompatibilityMixIn(object): if self.verbose and not daemon: print(self.traffic_legend) - # Start proxying try: self.msg("Start proxying") self.do_proxy(tsock) - except Exception: + finally: + tsock.close() if tunnel: - self.vmsg( - "%s:%s (via %s@%s:%s) : Websocket client or Target closed" - % (console_host, console_port, connuser, connhost, connport) - ) - if tsock: - tsock.shutdown(socket.SHUT_RDWR) - tsock.close() - tunnel.close_all() - raise + error = close_tunnel(tunnel) + if error: + logging.warning("SSH tunnel to %s@%s failed: %s", connuser, connhost, error) if USE_HANDLER: diff --git a/console/sshtunnels.py b/console/sshtunnels.py index 44f7a53d..81e225b0 100644 --- a/console/sshtunnels.py +++ b/console/sshtunnels.py @@ -1,217 +1,41 @@ -# Copyright (C) 2014, 2015 Red Hat, Inc. -# -# This work is licensed under the GNU GPLv2 or later. -# See the COPYING file in the top-level directory. +"""The VNC console of an SSH compute, carried by the SSH connection: `ssh -W` +forwards its standard input and output to the console's address or Unix +socket on the compute, so the compute needs no nc and runs no shell.""" -import functools -import logging as log -import os -import queue -import signal +import ipaddress import socket -import threading +import subprocess -class _TunnelScheduler(object): - """ - If the user is using an SSH URI without SSH keys, we need to - serialize connection opening otherwise ssh-askpass gets all angry. - This handles the locking and scheduling. - It's only instantiated once for the whole app, because we serialize - independent of connection, vm, etc. - """ - - def __init__(self): - self._thread = None - self._queue = queue.Queue() - self._lock = threading.Lock() - - def _handle_queue(self): - while True: - ( - lock_cb, - cb, - args, - ) = self._queue.get() - lock_cb() - cb(*args) - - def schedule(self, lock_cb, cb, *args): - if not self._thread: - self._thread = threading.Thread( - name="Tunnel thread", target=self._handle_queue, args=() - ) - self._thread.daemon = True - if not self._thread.is_alive(): - self._thread.start() - self._queue.put((lock_cb, cb, args)) - - def lock(self): - self._lock.acquire() - - def unlock(self): - self._lock.release() +def tunnel_target(address, port, unix_socket): + """Where on the compute ssh forwards to: the Unix socket, or the address + and port, a wildcard or missing address meaning the compute's loopback.""" + if unix_socket: + return unix_socket + address = {None: "127.0.0.1", "": "127.0.0.1", "0.0.0.0": "127.0.0.1", "::": "::1"}.get(address, address) + try: + if ipaddress.ip_address(address).version == 6: + address = f"[{address}]" + except ValueError: # a host name + pass + return f"{address}:{port}" -_tunnel_scheduler = _TunnelScheduler() +def open_tunnel(host, user, target): + """A socket connected to target on host, and the ssh process carrying it. + ssh never asks for anything: a missing key or an unknown host key fails.""" + ours, theirs = socket.socketpair() + argv = ["ssh", "-o", "BatchMode=yes", "-o", "ConnectTimeout=10", "-W", target] + if user: + argv += ["-l", user] + process = subprocess.Popen(argv + ["--", host], stdin=theirs, stdout=theirs, stderr=subprocess.PIPE) + theirs.close() + return ours, process -class _Tunnel(object): - def __init__(self): - self._pid = None - self._closed = False - self._errfd = None - - def close(self): - if self._closed: - return - self._closed = True - - log.debug( - "Close tunnel PID=%s ERRFD=%s", - self._pid, - self._errfd and self._errfd.fileno() or None, - ) - - # Since this is a socket object, the file descriptor is closed - # when it's garbage collected. - self._errfd = None - - if self._pid: - os.kill(self._pid, signal.SIGKILL) - os.waitpid(self._pid, 0) - self._pid = None - - def get_err_output(self): - errout = "" - while True: - try: - new = self._errfd.recv(1024) - except Exception: - break - - if not new: - break - - errout += new.decode() - - return errout - - def open(self, argv, sshfd): - if self._closed: - return - - errfds = socket.socketpair() - pid = os.fork() - if pid == 0: - errfds[0].close() - - os.dup2(sshfd.fileno(), 0) - os.dup2(sshfd.fileno(), 1) - os.dup2(errfds[1].fileno(), 2) - os.execlp(*argv) - os._exit(1) # pylint: disable=protected-access - - sshfd.close() - errfds[1].close() - - self._errfd = errfds[0] - self._errfd.setblocking(0) - log.debug("Opened tunnel PID=%d ERRFD=%d", pid, self._errfd.fileno()) - - self._pid = pid - - -def _make_ssh_command(connhost, connuser, connport, gaddr, gport, gsocket): - - # Build SSH cmd - argv = ["ssh", "ssh"] - if connport: - argv += ["-p", str(connport)] - - if connuser: - argv += ["-l", connuser] - - argv += [connhost] - - # Build 'nc' command run on the remote host - # - # This ugly thing is a shell script to detect availability of - # the -q option for 'nc': debian and suse based distros need this - # flag to ensure the remote nc will exit on EOF, so it will go away - # when we close the VNC tunnel. If it doesn't go away, subsequent - # VNC connection attempts will hang. - # - # Fedora's 'nc' doesn't have this option, and apparently defaults - # to the desired behavior. - # - if gsocket: - nc_params = "-U %s" % gsocket - else: - nc_params = "%s %s" % (gaddr, gport) - - nc_cmd = ( - """nc -q 2>&1 | grep "requires an argument" >/dev/null;""" - """if [ $? -eq 0 ] ; then""" - """ CMD="nc -q 0 %(nc_params)s";""" - """else""" - """ CMD="nc %(nc_params)s";""" - """fi;""" - """eval "$CMD";""" % {"nc_params": nc_params} - ) - - argv.append("sh -c") - argv.append("'%s'" % nc_cmd) - - argv_str = functools.reduce(lambda x, y: x + " " + y, argv[1:]) - log.debug("Pre-generated ssh command for info: %s", argv_str) - return argv - - -class SSHTunnels(object): - def __init__(self, connhost, connuser, connport, gaddr, gport, gsocket): - self._tunnels = [] - self._sshcommand = _make_ssh_command( - connhost, connuser, connport, gaddr, gport, gsocket - ) - self._locked = False - - def open_new(self): - t = _Tunnel() - self._tunnels.append(t) - - # socket FDs are closed when the object is garbage collected. This - # can close an FD behind the VNC client's back which causes crashes. - # - # Dup a bare FD for the viewer side of things, but keep the high - # level socket object for the SSH side, since it simplifies things - # in that area. - viewerfd, sshfd = socket.socketpair() - _tunnel_scheduler.schedule(self._lock, t.open, self._sshcommand, sshfd) - - retfd = os.dup(viewerfd.fileno()) - log.debug("Generated tunnel fd=%s for viewer", retfd) - return retfd - - def close_all(self): - for l in self._tunnels: - l.close() - self._tunnels = [] - self.unlock() - - def get_err_output(self): - errstrings = [] - for l in self._tunnels: - e = l.get_err_output().strip() - if e and e not in errstrings: - errstrings.append(e) - return "\n".join(errstrings) - - def _lock(self): - _tunnel_scheduler.lock() - self._locked = True - - def unlock(self, *args, **kwargs): - if self._locked: - _tunnel_scheduler.unlock(*args, **kwargs) - self._locked = False +def close_tunnel(process): + """End the ssh process; its error output, if it failed.""" + if process.poll() is None: + process.terminate() + _, error = process.communicate() + return error.decode(errors="replace").strip() if process.returncode not in (0, -15) else "" diff --git a/console/tests/test_sshtunnels.py b/console/tests/test_sshtunnels.py new file mode 100644 index 00000000..4e2fc7f6 --- /dev/null +++ b/console/tests/test_sshtunnels.py @@ -0,0 +1,42 @@ +"""The SSH console tunnel: ssh -W to the console's address or socket, never a +prompt, no remote shell, and its error output when it fails.""" + +import sys +from unittest import TestCase +from unittest.mock import patch + +from console import sshtunnels + + +class SshTunnelTests(TestCase): + def test_ssh_forwards_to_the_console_without_a_shell_or_a_prompt(self): + with patch.object(sshtunnels.subprocess, "Popen") as popen: + sock, process = sshtunnels.open_tunnel("kvm1.example.com", "root", "127.0.0.1:5900") + sock.close() + self.assertEqual(popen.call_args.args[0], [ + "ssh", "-o", "BatchMode=yes", "-o", "ConnectTimeout=10", "-W", "127.0.0.1:5900", + "-l", "root", "--", "kvm1.example.com"]) + self.assertIs(process, popen.return_value) + + def test_the_target_on_the_compute(self): + for address, unix_socket, target in ( + ("192.0.2.40", None, "192.0.2.40:5900"), + ("2001:db8::40", None, "[2001:db8::40]:5900"), + (None, None, "127.0.0.1:5900"), + ("0.0.0.0", None, "127.0.0.1:5900"), + ("::", None, "[::1]:5900"), + ("localhost", None, "localhost:5900"), + ("127.0.0.1", "/run/vnc.sock", "/run/vnc.sock"), + ): + with self.subTest(address=address, unix_socket=unix_socket): + self.assertEqual(sshtunnels.tunnel_target(address, 5900, unix_socket), target) + + def test_a_failed_tunnel_reports_what_ssh_said(self): + failing = sshtunnels.subprocess.Popen( + [sys.executable, "-c", "import sys; sys.stderr.write('Host key verification failed.'); sys.exit(255)"], + stderr=sshtunnels.subprocess.PIPE) + failing.wait() # ssh fails at once, before the session + self.assertEqual(sshtunnels.close_tunnel(failing), "Host key verification failed.") + ended = sshtunnels.subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"], + stderr=sshtunnels.subprocess.PIPE) + self.assertEqual(sshtunnels.close_tunnel(ended), "") # ended with the session: no error diff --git a/console/tests/test_views.py b/console/tests/test_views.py index c0d6e5f7..832c000a 100644 --- a/console/tests/test_views.py +++ b/console/tests/test_views.py @@ -506,7 +506,7 @@ class NovncdDaemonLogicTestCase(TestCase): @patch.object(novncd_mod, "get_connection_infos") def test_a_cookie_does_not_choose_the_console(self, mock_get_info): # another tab's console in a cookie: the URL's own token is used - mock_get_info.return_value = ("host1", 22, "root", 1, "192.168.1.50", 5900, None) + mock_get_info.return_value = ("host1", "root", 1, "192.168.1.50", 5900, None) handler = self.CompatibilityMixIn() handler.headers = {"cookie": "token=1-uuid-of-another-tab"} handler.path = "/novncd/?token=1-uuid-of-this-tab" @@ -520,7 +520,6 @@ class NovncdDaemonLogicTestCase(TestCase): def test_compatibility_mixin_token_from_query_params(self, mock_get_info): mock_get_info.return_value = ( "host2", - 22, "root", 1, "192.168.1.50", @@ -544,7 +543,7 @@ class NovncdDaemonLogicTestCase(TestCase): @patch.object(novncd_mod, "get_connection_infos") def test_a_local_socket_host_reaches_a_console_on_its_loopback(self, mock_get_info): # libvirt on the panel's host, the VM's console listening on 127.0.0.1 - mock_get_info.return_value = ("localhost", None, "", novncd_mod.CONN_SOCKET, "127.0.0.1", 5902, None) + mock_get_info.return_value = ("localhost", "", novncd_mod.CONN_SOCKET, "127.0.0.1", 5902, None) handler = self.CompatibilityMixIn() handler.headers = {} handler.path = "/novncd/?token=1-uuid" @@ -555,6 +554,32 @@ class NovncdDaemonLogicTestCase(TestCase): handler._new_client(daemon=False, socket_factory=socket_factory) socket_factory.assert_called_once_with("127.0.0.1", 5902, connect=True) + def test_an_ssh_hosts_console_always_goes_through_the_tunnel(self): + # whatever address the console listens on, the VNC stream is not sent unencrypted + for console_host, console_socket, target in ( + ("192.0.2.40", None, "192.0.2.40:5900"), + ("0.0.0.0", None, "127.0.0.1:5900"), + ("::", None, "[::1]:5900"), + ("127.0.0.1", None, "127.0.0.1:5900"), + (None, "/run/libvirt/qemu/vnc.sock", "/run/libvirt/qemu/vnc.sock"), + ): + with self.subTest(console_host=console_host), patch.object( + novncd_mod, "get_connection_infos", + return_value=("kvm1.example.com", "root", novncd_mod.CONN_SSH, console_host, 5900, console_socket), + ), patch.object(novncd_mod, "open_tunnel", return_value=(MagicMock(), MagicMock())) as open_tunnel, \ + patch.object(novncd_mod, "close_tunnel", return_value="") as close_tunnel: + handler = self.CompatibilityMixIn() + handler.path = "/novncd/?token=1-uuid" + handler.msg = MagicMock() + handler.do_proxy = MagicMock() + handler.verbose = False + socket_factory = MagicMock() + handler._new_client(daemon=False, socket_factory=socket_factory) + open_tunnel.assert_called_once_with("kvm1.example.com", "root", target) + socket_factory.assert_not_called() + # the tunnel ends with the session + close_tunnel.assert_called_once_with(open_tunnel.return_value[1]) + @patch.object(novncd_mod, "get_connection_infos") def test_compatibility_mixin_missing_token_safe_exit(self, mock_get_info): handler = self.CompatibilityMixIn() @@ -594,7 +619,7 @@ class NovncdDaemonLogicTestCase(TestCase): infos = novncd_mod.get_connection_infos(token) self.assertEqual(infos[0], "127.0.0.1") - self.assertEqual(infos[5], 5900) + self.assertEqual(infos[4], 5900) # the VM access was checked for, not whichever VM has its name now self.assertEqual(mock_wvm.call_args.kwargs["uuid"], mock_instance.uuid)