From 458074df82be32bdd993517ad48275306d15fe3a Mon Sep 17 00:00:00 2001 From: catborise Date: Thu, 5 Nov 2026 15:49:31 +0300 Subject: [PATCH] fix(console): the console daemons hold to what the console page allowed The token now names the console type it was made for: novncd accepts only a VNC page's token and socketiod only a serial page's, so a VNC page's token no longer opens the serial console without the VNC password. A VNC session re-checks every 10 s, as the serial one did, that its user may still open the console; a deactivated user or a removed owner loses it. The check (may_still_open) moves to console.tokens for both daemons. --- conf/daemon/consolecallback | 15 +---- console/novncd | 25 ++++++-- console/socketiod | 2 +- console/tests/test_consolecallback.py | 3 +- console/tests/test_live_serial.py | 6 +- console/tests/test_socketiod.py | 9 +-- console/tests/test_socketiod_child.py | 2 +- console/tests/test_views.py | 88 +++++++++++++++------------ console/tokens.py | 32 +++++++--- console/views.py | 2 +- 10 files changed, 110 insertions(+), 74 deletions(-) diff --git a/conf/daemon/consolecallback b/conf/daemon/consolecallback index e70b6241..d7351ae5 100755 --- a/conf/daemon/consolecallback +++ b/conf/daemon/consolecallback @@ -20,16 +20,13 @@ os.environ.setdefault("DJANGO_SETTINGS_MODULE", "webvirtcloud.settings") django.setup() import libvirt # noqa: E402 -from django import db # noqa: E402 -from django.contrib.auth import get_user_model # noqa: E402 from instances.models import Instance # noqa: E402 -from instances.utils import can_open_console # noqa: E402 +from console.tokens import ACCESS_CHECK, may_still_open # noqa: E402 from operations.utils import public_error_text # noqa: E402 from vrtManager.connection import connection_manager # noqa: E402 SEND_WAIT = 5 # seconds a paste may wait for the VM to take it -ACCESS_CHECK = 10 # seconds between checks that the user may still use the console log = logging.getLogger("consolecallback") @@ -112,16 +109,6 @@ def from_vm(stream, events, console): os.write(1, data) -def may_still_open(user_id, instance): - """The user still may open the console: an owner may have been removed or - the user deactivated since the session started.""" - try: - user = get_user_model().objects.filter(pk=user_id).first() - return user is not None and can_open_console(user, instance) - finally: - db.connection.close() # a session may last for days: no idle connection meanwhile - - def main(): # what the operator reads goes to socketiod's log (the journal, docker # logs), on the descriptor it names; the user's terminal gets a short line diff --git a/console/novncd b/console/novncd index 83fd94c0..516b21f4 100755 --- a/console/novncd +++ b/console/novncd @@ -8,6 +8,7 @@ import logging import os import socket import sys +import threading from urllib.parse import parse_qs, urlparse import django @@ -21,6 +22,7 @@ django.setup() from websockify import ProxyRequestHandler, WebSocketProxy # noqa: E402 from console.sshtunnels import open_tunnel, tunnel_target # noqa: E402 +from console.tokens import ACCESS_CHECK, authorize, may_still_open # noqa: E402 from vrtManager.connection import CONN_SOCKET, CONN_SSH # noqa: E402 from vrtManager.util import is_loopback # noqa: E402 from webvirtcloud.settings import WS_HOST, WS_PORT # noqa: E402 @@ -54,11 +56,9 @@ def get_parser(default_host=WS_HOST, default_port=WS_PORT or 6080): return parser -def get_connection_infos(token): - from console.tokens import authorize +def get_connection_infos(instance): from vrtManager.instance import wvmInstance - instance, _user = authorize(token) conn = wvmInstance( instance.compute.hostname, instance.compute.login, @@ -77,6 +77,19 @@ def get_connection_infos(token): WILDCARD = (None, "", "0.0.0.0", "::") +def end_when_refused(instance, user_id, tsock, ended): + """Close the session's VNC socket, which ends it, once its user may no + longer open the console (as the serial console does).""" + while not ended.wait(ACCESS_CHECK): + if not may_still_open(user_id, instance): + logging.info("VNC console of VM %s closed: its user may no longer open it", instance.name) + try: + tsock.shutdown(socket.SHUT_RDWR) + except OSError: + pass # the session ended meanwhile + return + + class ConsoleClient: """What a console websocket is joined to; ConsoleProxyHandler is the websockify handler that runs it.""" @@ -90,7 +103,8 @@ class ConsoleClient: return try: - connhost, connuser, conntype, console_host, console_port, console_socket = get_connection_infos(token) + instance, user = authorize(token, "vnc") + connhost, connuser, conntype, console_host, console_port, console_socket = get_connection_infos(instance) except Exception as e: logging.warning("Console connection denied: %s", e) return @@ -126,10 +140,13 @@ class ConsoleClient: self.msg("connecting to: %s:%s" % (address, console_port)) tsock = socket_factory(address, console_port, connect=True) + ended = threading.Event() + threading.Thread(target=end_when_refused, args=(instance, user.pk, tsock, ended), daemon=True).start() try: self.msg("Start proxying") self.do_proxy(tsock) finally: + ended.set() tsock.close() if tunnel: error = tunnel.close() diff --git a/console/socketiod b/console/socketiod index bf37e009..2215ac5b 100755 --- a/console/socketiod +++ b/console/socketiod @@ -61,7 +61,7 @@ def connect(sid, environ, auth): if not settings.SERIAL_CONSOLE_ENABLED: raise refused("The serial console is turned off on this server") try: - instance, user = authorize((auth or {}).get("token")) + instance, user = authorize((auth or {}).get("token"), "pty") except PermissionError: raise refused("The console link has expired, or you may no longer open this console: reload the page") finally: diff --git a/console/tests/test_consolecallback.py b/console/tests/test_consolecallback.py index bea07fa2..623e9695 100644 --- a/console/tests/test_consolecallback.py +++ b/console/tests/test_consolecallback.py @@ -13,6 +13,7 @@ from django.test import TestCase as DjangoTestCase from accounts.models import UserInstance from computes.models import Compute +from console import tokens from instances.models import Instance CONSOLECALLBACK = Path(__file__).resolve().parent.parent.parent / "conf" / "daemon" / "consolecallback" @@ -100,7 +101,7 @@ class AccessTests(DjangoTestCase): vm = Instance.objects.create(compute=compute, name="vm-a", uuid="11111111-0000-0000-0000-000000000009") owner = get_user_model().objects.create_user("owner", password="pw") ownership = UserInstance.objects.create(user=owner, instance=vm) - with patch.object(cb.db.connection, "close"): # the test's transaction keeps its connection + with patch.object(tokens.db.connection, "close"): # the test's transaction keeps its connection self.assertTrue(cb.may_still_open(owner.pk, vm)) ownership.delete() self.assertFalse(cb.may_still_open(owner.pk, vm)) diff --git a/console/tests/test_live_serial.py b/console/tests/test_live_serial.py index f9a5cd32..a6875716 100644 --- a/console/tests/test_live_serial.py +++ b/console/tests/test_live_serial.py @@ -61,12 +61,12 @@ class LiveSerialConsoleTestCase(TransactionTestCase): dom = serial_vm(conn, f"{P}serial-{conn_type}") vm = Instance.objects.create(compute=compute, name=dom.name(), uuid=dom.UUIDString()) UserInstance.objects.create(user=self.user, instance=vm) - return tokens.SIGNER.sign(f"{compute.id}:{vm.uuid}:{self.user.id}"), dom + return tokens.sign(compute.id, vm.uuid, self.user.id, "pty"), dom def admin_token(self, token): - host, uuid, _user = tokens.SIGNER.unsign(token).split(":") + host, uuid, _user, _type = tokens.SIGNER.unsign(token).split(":") admin, _ = get_user_model().objects.get_or_create(username="serial-live-admin", is_superuser=True) - return tokens.SIGNER.sign(f"{host}:{uuid}:{admin.id}") + return tokens.sign(host, uuid, admin.id, "pty") def test_the_console_of_a_running_vm_opens_once(self): for args in self.hosts: diff --git a/console/tests/test_socketiod.py b/console/tests/test_socketiod.py index 7e593b36..183426f8 100644 --- a/console/tests/test_socketiod.py +++ b/console/tests/test_socketiod.py @@ -36,12 +36,12 @@ class Base(TestCase): def token(self, user=None, vm=None): vm = vm or self.vm - return tokens.SIGNER.sign(f"{vm.compute_id}:{vm.uuid}:{(user or self.user).id}") + return tokens.sign(vm.compute_id, vm.uuid, (user or self.user).id, "pty") class TokenTestCase(Base): def test_a_token_opens_its_vm_for_a_user_who_may_open_it(self): - self.assertEqual(tokens.authorize(self.token()), (self.vm, self.user)) + self.assertEqual(tokens.authorize(self.token(), "pty"), (self.vm, self.user)) def test_refused_tokens(self): stranger = get_user_model().objects.create_user("stranger", password="pw") @@ -53,14 +53,15 @@ class TokenTestCase(Base): "tampered": self.token()[:-2] + "xx", "no access": self.token(stranger), "inactive user": self.token(inactive), + "a VNC page's token": tokens.sign(self.vm.compute_id, self.vm.uuid, self.user.id, "vnc"), } for name, token in cases.items(): with self.subTest(name), self.assertRaises(PermissionError): - tokens.authorize(token) + tokens.authorize(token, "pty") token = self.token() with patch("django.core.signing.time.time", return_value=time.time() + tokens.MAX_AGE + 1): with self.assertRaises(PermissionError): - tokens.authorize(token) + tokens.authorize(token, "pty") class SocketiodTestCase(Base): diff --git a/console/tests/test_socketiod_child.py b/console/tests/test_socketiod_child.py index 752caf3c..d2308b50 100644 --- a/console/tests/test_socketiod_child.py +++ b/console/tests/test_socketiod_child.py @@ -107,7 +107,7 @@ class SocketiodProcessTestCase(TransactionTestCase): for n, name in enumerate(("vm-a", "vm-b"), 1): vm = Instance.objects.create(compute=compute, name=name, uuid=f"22222222-0000-0000-0000-00000000000{n}") UserInstance.objects.create(user=user, instance=vm) - self.tokens[name] = tokens.SIGNER.sign(f"{compute.id}:{vm.uuid}:{user.id}") + self.tokens[name] = tokens.sign(compute.id, vm.uuid, user.id, "pty") self.server, self.port = start_socketiod(self) def console(self, token): diff --git a/console/tests/test_views.py b/console/tests/test_views.py index dada7f44..c53d2113 100644 --- a/console/tests/test_views.py +++ b/console/tests/test_views.py @@ -8,6 +8,7 @@ from unittest.mock import MagicMock, patch from accounts.models import UserInstance from appsettings.models import AppSettings from computes.models import Compute +from console import tokens from console.views import LAYOUTS_DIR from django.contrib.auth import get_user_model from django.contrib.auth.models import Permission @@ -96,7 +97,7 @@ class ConsoleViewsTestCase(TestCase): signer = TimestampSigner(salt="console.novnc") unsigned = signer.unsign(config["token"], max_age=120) self.assertEqual( - unsigned, f"{self.compute.id}:{self.instance.uuid}:{self.admin_user.id}" + unsigned, f"{self.compute.id}:{self.instance.uuid}:{self.admin_user.id}:vnc" ) self.assertEqual(config["password"], "vncsecret") @@ -130,7 +131,7 @@ class ConsoleViewsTestCase(TestCase): # the page's credential for socketiod, as data signed = json.loads(html.fromstring(response.content).get_element_by_id("serial-token").text) self.assertEqual(TimestampSigner(salt="console.novnc").unsign(signed), - f"{self.compute.id}:{self.instance.uuid}:{self.admin_user.id}") + f"{self.compute.id}:{self.instance.uuid}:{self.admin_user.id}:pty") @override_settings(SERIAL_CONSOLE_ENABLED=False) @patch("console.views.wvmInstance") @@ -227,7 +228,7 @@ class ConsoleViewsTestCase(TestCase): signed = vnc_config(response)["token"] self.assertEqual(TimestampSigner(salt="console.novnc").unsign(signed), - f"{self.compute.id}:{self.instance.uuid}:{self.admin_user.id}") + f"{self.compute.id}:{self.instance.uuid}:{self.admin_user.id}:vnc") @patch("console.views.wvmInstance") def test_a_shut_off_vm_says_so(self, mock_wvm): @@ -405,6 +406,9 @@ class NovncdDaemonLogicTestCase(TestCase): def setUp(self): self.get_parser = novncd_mod.get_parser self.ConsoleClient = novncd_mod.ConsoleClient + patcher = patch.object(novncd_mod, "authorize", return_value=(MagicMock(), MagicMock())) + self.authorize = patcher.start() + self.addCleanup(patcher.stop) def test_novncd_parser_defaults(self): parser = self.get_parser() @@ -431,7 +435,7 @@ class NovncdDaemonLogicTestCase(TestCase): handler.msg = MagicMock() handler.do_proxy = MagicMock() handler._new_client(socket_factory=MagicMock()) - mock_get_info.assert_called_once_with("1-uuid-of-this-tab") + self.authorize.assert_called_once_with("1-uuid-of-this-tab", "vnc") @patch.object(novncd_mod, "get_connection_infos") def test_compatibility_mixin_token_from_query_params(self, mock_get_info): @@ -453,7 +457,7 @@ class NovncdDaemonLogicTestCase(TestCase): socket_factory = MagicMock() handler._new_client(socket_factory=socket_factory) - mock_get_info.assert_called_once_with("1-uuid-from-query") + self.authorize.assert_called_once_with("1-uuid-from-query", "vnc") # the address the console listens on socket_factory.assert_called_once_with("192.168.1.50", 5901, connect=True) @@ -539,18 +543,14 @@ class NovncdDaemonLogicTestCase(TestCase): socket_factory.assert_not_called() self.assertIn("without a token", log.output[0]) - @patch("console.tokens.authorize") @patch("vrtManager.instance.wvmInstance") - def test_get_connection_infos_valid_signed_token(self, mock_wvm, mock_inst_get): - User = get_user_model() - user = User.objects.filter(is_superuser=True).first() + def test_get_connection_infos_reads_the_vm_it_was_given(self, mock_wvm): mock_instance = MagicMock() mock_instance.compute.hostname = "127.0.0.1" mock_instance.compute.login = "root" mock_instance.compute.password = "" mock_instance.compute.type = 1 mock_instance.name = "test-vm" - mock_inst_get.return_value = (mock_instance, user) mock_conn = MagicMock() mock_conn.get_console_listener_addr.return_value = "127.0.0.1" @@ -558,22 +558,38 @@ class NovncdDaemonLogicTestCase(TestCase): mock_conn.get_console_socket.return_value = None mock_wvm.return_value = mock_conn - signer = TimestampSigner(salt="console.novnc") - token = signer.sign(f"1:test-uuid:{user.id}") - - infos = novncd_mod.get_connection_infos(token) + infos = novncd_mod.get_connection_infos(mock_instance) self.assertEqual(infos[0], "127.0.0.1") 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) + def test_a_vnc_session_ends_when_its_user_may_no_longer_open_it(self): + tsock, ended = MagicMock(), MagicMock() + ended.wait.return_value = False # ACCESS_CHECK passed, the session still open + vm = MagicMock() + with patch.object(novncd_mod, "may_still_open", side_effect=[True, False]) as may_still_open: + novncd_mod.end_when_refused(vm, 7, tsock, ended) + self.assertEqual(may_still_open.call_count, 2) + may_still_open.assert_called_with(7, vm) + tsock.shutdown.assert_called_once_with(novncd_mod.socket.SHUT_RDWR) + + def test_a_token_of_a_serial_console_does_not_open_a_vnc_console(self): + user = get_user_model().objects.create_user("vnc-owner", password="pw") + compute = Compute.objects.create(name="novnc-type-compute", hostname="127.0.0.1", login="root", password="", type=1) + instance = Instance.objects.create(compute=compute, name="novnc-type-vm", uuid="77777777-7777-7777-7777-777777777777") + UserInstance.objects.create(instance=instance, user=user) + self.assertEqual(tokens.authorize(tokens.sign(compute.id, instance.uuid, user.id, "vnc"), "vnc"), (instance, user)) + with self.assertRaises(PermissionError): + tokens.authorize(tokens.sign(compute.id, instance.uuid, user.id, "pty"), "vnc") + @patch("sys.argv", ["novncd"]) @override_settings(DEBUG=False) - def test_get_connection_infos_invalid_token_in_production(self): + def test_vnc_token_invalid_token_in_production(self): with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos("invalid-raw-token") + tokens.authorize("invalid-raw-token", "vnc") - def test_get_connection_infos_unauthorized_user(self): + def test_vnc_token_unauthorized_user(self): User = get_user_model() unauthorized_user = User.objects.create_user( username="unauthorized_console_user", @@ -592,13 +608,12 @@ class NovncdDaemonLogicTestCase(TestCase): uuid="33333333-3333-3333-3333-333333333333", ) - signer = TimestampSigner(salt="console.novnc") - token = signer.sign(f"{compute.id}:{instance.uuid}:{unauthorized_user.id}") + token = tokens.sign(compute.id, instance.uuid, unauthorized_user.id, "vnc") with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos(token) + tokens.authorize(token, "vnc") - def test_get_connection_infos_rejects_signed_token_without_user(self): + def test_vnc_token_rejects_signed_token_without_user(self): # The console view always signs host:uuid:user_id; a token without the # user part would skip the per-user access check entirely. compute = Compute.objects.create( @@ -613,9 +628,9 @@ class NovncdDaemonLogicTestCase(TestCase): token = signer.sign(f"{compute.id}:{instance.uuid}") with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos(token) + tokens.authorize(token, "vnc") - def test_get_connection_infos_rejects_unsigned_token_even_under_test_runner(self): + def test_vnc_token_rejects_unsigned_token_even_under_test_runner(self): # sys.argv contains "test" while this suite runs; production code must # not change behaviour based on that. compute = Compute.objects.create( @@ -628,9 +643,9 @@ class NovncdDaemonLogicTestCase(TestCase): ) with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos(f"{compute.id}-{instance.uuid}") + tokens.authorize(f"{compute.id}-{instance.uuid}", "vnc") - def test_get_connection_infos_view_instances_user_is_not_enough(self): + def test_vnc_token_view_instances_user_is_not_enough(self): User = get_user_model() viewer = User.objects.create_user(username="novnc_viewer", password="password") viewer.user_permissions.add(Permission.objects.get(codename="view_instances")) @@ -643,13 +658,12 @@ class NovncdDaemonLogicTestCase(TestCase): uuid="55555555-5555-5555-5555-555555555555", ) - signer = TimestampSigner(salt="console.novnc") - token = signer.sign(f"{compute.id}:{instance.uuid}:{viewer.id}") + token = tokens.sign(compute.id, instance.uuid, viewer.id, "vnc") with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos(token) + tokens.authorize(token, "vnc") - def test_get_connection_infos_inactive_user(self): + def test_vnc_token_inactive_user(self): User = get_user_model() inactive_user = User.objects.create_user( username="inactive_console_user", @@ -670,20 +684,19 @@ class NovncdDaemonLogicTestCase(TestCase): ) UserInstance.objects.create(instance=instance, user=inactive_user) - signer = TimestampSigner(salt="console.novnc") - token = signer.sign(f"{compute.id}:{instance.uuid}:{inactive_user.id}") + token = tokens.sign(compute.id, instance.uuid, inactive_user.id, "vnc") with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos(token) + tokens.authorize(token, "vnc") @patch("sys.argv", ["novncd"]) @override_settings(DEBUG=True) - def test_get_connection_infos_debug_true_does_not_bypass(self): + def test_vnc_token_debug_true_does_not_bypass(self): with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos("invalid-token-in-debug") + tokens.authorize("invalid-token-in-debug", "vnc") @patch("sys.argv", ["novncd"]) - def test_get_connection_infos_expired_token(self): + def test_vnc_token_expired_token(self): User = get_user_model() user = User.objects.filter(is_superuser=True).first() compute = Compute.objects.create( @@ -703,8 +716,7 @@ class NovncdDaemonLogicTestCase(TestCase): from unittest.mock import patch # Sign token 300 seconds in the past (> 120s TTL) with patch("time.time", return_value=time.time() - 300): - signer = TimestampSigner(salt="console.novnc") - expired_token = signer.sign(f"{compute.id}:{instance.uuid}:{user.id}") + expired_token = tokens.sign(compute.id, instance.uuid, user.id, "vnc") with self.assertRaises(PermissionError): - novncd_mod.get_connection_infos(expired_token) + tokens.authorize(expired_token, "vnc") diff --git a/console/tokens.py b/console/tokens.py index b9f84a71..503c7c2b 100644 --- a/console/tokens.py +++ b/console/tokens.py @@ -1,8 +1,9 @@ -"""The console token: the console view signs "host:uuid:user_id" for the VM -whose console it shows; the console daemons (novncd, socketiod) accept it for -120 s if that user may still open that VM's console. A bearer token: whoever -holds it within that time gets the console.""" +"""The console token: the console view signs "host:uuid:user_id:console type" +for the VM whose console it shows; the console daemons (novncd for vnc, +socketiod for pty) accept it for 120 s if that user may still open that VM's +console. A bearer token: whoever holds it within that time gets the console.""" +from django import db from django.contrib.auth import get_user_model from django.core.signing import BadSignature, SignatureExpired, TimestampSigner @@ -11,18 +12,35 @@ from instances.utils import can_open_console SIGNER = TimestampSigner(salt="console.novnc") MAX_AGE = 120 +ACCESS_CHECK = 10 # seconds between checks that an open console's user may still use it -def authorize(token): +def sign(compute_id, uuid, user_id, console_type): + return SIGNER.sign(f"{compute_id}:{uuid}:{user_id}:{console_type}") + + +def authorize(token, console_type): """The Instance the token opens and the user it was given to; - PermissionError otherwise.""" + PermissionError otherwise, also for a token of another console type.""" try: - host, uuid, user_id = SIGNER.unsign(token, max_age=MAX_AGE).split(":") + host, uuid, user_id, signed_type = SIGNER.unsign(token, max_age=MAX_AGE).split(":") host, user_id = int(host), int(user_id) except (BadSignature, SignatureExpired, ValueError, TypeError): raise PermissionError("Invalid or expired console token") + if signed_type != console_type: + raise PermissionError(f"The token is for a {signed_type} console") instance = Instance.objects.filter(compute_id=host, uuid=uuid).select_related("compute").first() user = get_user_model().objects.filter(id=user_id, is_active=True).first() if instance is None or user is None or not can_open_console(user, instance): raise PermissionError("No access to this console") return instance, user + + +def may_still_open(user_id, instance): + """The user still may open the console: an owner may have been removed or + the user deactivated since the session started.""" + try: + user = get_user_model().objects.filter(pk=user_id).first() + return user is not None and can_open_console(user, instance) + finally: + db.connection.close() # a session may last for days: no idle connection meanwhile diff --git a/console/views.py b/console/views.py index 59b02b94..c584550d 100644 --- a/console/views.py +++ b/console/views.py @@ -90,7 +90,7 @@ def console(request, compute_id, uuid): return render(request, "console-error.html", {"instance": instance, "console_error": console_error}) # the console daemons' credential for this page - token = tokens.SIGNER.sign(f"{compute_id}:{uuid}:{request.user.id}") + token = tokens.sign(compute_id, uuid, request.user.id, console_type) if console_type == "pty": return render(request, "console-xterm.html", {"instance": instance, "token": token})