From 82af555d45326f7377f04ba8529bff100790a167 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 26 Oct 2026 19:27:31 +0300 Subject: [PATCH] fix(console): a serial session follows its user's access; a superuser takes over - The console worker learns whose session it is and checks every ten seconds that the user may still open the console: an owner removed or a user deactivated meanwhile loses it, with a line in the terminal. Access was checked only when the session started, which could last for days. - One console per VM: a console left open in a forgotten tab locked everyone out, superusers included. A superuser's connection now takes the console over, and the other window is told so. --- conf/daemon/consolecallback | 23 ++++++++++++++++++++++- console/novncd | 2 +- console/socketiod | 25 +++++++++++++++++++------ console/tests/test_consolecallback.py | 23 +++++++++++++++++++++++ console/tests/test_live_serial.py | 26 +++++++++++++++++++++----- console/tests/test_socketiod.py | 16 ++++++++++++++-- console/tests/test_views.py | 2 +- console/tokens.py | 5 +++-- 8 files changed, 104 insertions(+), 18 deletions(-) diff --git a/conf/daemon/consolecallback b/conf/daemon/consolecallback index 414e8c24..60585846 100755 --- a/conf/daemon/consolecallback +++ b/conf/daemon/consolecallback @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -# consolecallback - the serial console of one VM on stdin/stdout +# consolecallback - the serial console of one VM on stdin/stdout # (socketiod runs it in a pty, one per client). It opens the console through # the compute's connection (its stored credentials); the connection's event # loop thread runs the stream callbacks. The console survives guest reboots. @@ -18,8 +18,11 @@ 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 operations.utils import public_error_text # noqa: E402 from vrtManager.connection import connection_manager # noqa: E402 @@ -27,6 +30,7 @@ from vrtManager.connection import connection_manager # noqa: E402 # raised and told in one line instead libvirt.registerErrorHandler(lambda context, error: None, None) 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 def say(text): @@ -97,8 +101,19 @@ 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(): instance = Instance.objects.select_related("compute").get(pk=int(sys.argv[1])) + user_id = int(sys.argv[2]) c = instance.compute try: console = Console(connection_manager.get_connection(c.hostname, c.login, c.password, c.type), instance.uuid) @@ -110,10 +125,16 @@ def main(): if console.stream is None: say("The VM is not running: its console opens when it starts") libvirt.virEventAddHandle(0, libvirt.VIR_EVENT_HANDLE_READABLE, from_client, console) + checked = time.monotonic() try: while console.running: time.sleep(0.2) console.check() + if time.monotonic() - checked > ACCESS_CHECK: + if not may_still_open(user_id, instance): + say("You may no longer open this console") + sys.exit(1) + checked = time.monotonic() except libvirt.libvirtError as err: # the host went away, the VM was undefined or migrated say("The console of %s closed: %s" % (instance.name, public_error_text(err))) sys.exit(1) diff --git a/console/novncd b/console/novncd index 636d56ba..67f4c41e 100755 --- a/console/novncd +++ b/console/novncd @@ -86,7 +86,7 @@ def get_connection_infos(token): from vrtManager.instance import wvmInstance try: - instance = authorize(token) + instance, _user = authorize(token) conn = wvmInstance( instance.compute.hostname, instance.compute.login, diff --git a/console/socketiod b/console/socketiod index 4305023f..83cf014e 100755 --- a/console/socketiod +++ b/console/socketiod @@ -49,26 +49,35 @@ def connect(sid, environ, auth): if not settings.SERIAL_CONSOLE_ENABLED: raise socketio.exceptions.ConnectionRefusedError("The serial console is turned off on this server") try: - instance = authorize((auth or {}).get("token")) + instance, user = authorize((auth or {}).get("token")) except PermissionError: raise socketio.exceptions.ConnectionRefusedError("The console link has expired: reload the page") finally: db.close_old_connections() + taken = None with sessions_lock: - if any(s["vm"] == instance.pk for s in sessions.values()): + open_sid = next((s for s, session in sessions.items() if session["vm"] == instance.pk), None) + if open_sid is not None and not user.is_superuser: raise socketio.exceptions.ConnectionRefusedError("The console of this VM is open in another window") - if len(sessions) >= MAX_SESSIONS: + if open_sid is None and len(sessions) >= MAX_SESSIONS: raise socketio.exceptions.ConnectionRefusedError("Too many consoles are open on this server: try again later") + if open_sid is not None: # a superuser takes over a console left open + taken = open_sid, sessions.pop(open_sid) pid, fd = pty.fork() if pid == 0: try: # pty.fork's master is inheritable: without this each worker would # hold the other sessions' consoles open os.closerange(3, os.sysconf("SC_OPEN_MAX")) - os.execv(sys.executable, [sys.executable, CONSOLECALLBACK, str(instance.pk)]) + os.execv(sys.executable, [sys.executable, CONSOLECALLBACK, str(instance.pk), str(user.pk)]) finally: os._exit(1) sessions[sid] = session = {"pid": pid, "fd": fd, "vm": instance.pk, "lock": threading.Lock(), "open": True} + if taken: + old_sid, old_session = taken + sio.emit("pty_output", {"output": "\r\n[An administrator opened this console]\r\n"}, to=old_sid) + end(old_session) + sio.disconnect(old_sid) sio.start_background_task(forward, sid, session) @@ -102,8 +111,12 @@ def stop(sid): """End the sid's worker; once, whoever calls it first.""" with sessions_lock: session = sessions.pop(sid, None) - if session is None: - return + if session is not None: + end(session) + + +def end(session): + """End a session taken out of sessions.""" try: os.kill(session["pid"], signal.SIGKILL) # first: a write blocked on its terminal ends except ProcessLookupError: diff --git a/console/tests/test_consolecallback.py b/console/tests/test_consolecallback.py index b1260017..0b391891 100644 --- a/console/tests/test_consolecallback.py +++ b/console/tests/test_consolecallback.py @@ -7,6 +7,13 @@ from pathlib import Path from unittest import TestCase from unittest.mock import MagicMock, patch +from django.contrib.auth import get_user_model +from django.test import TestCase as DjangoTestCase + +from accounts.models import UserInstance +from computes.models import Compute +from instances.models import Instance + CONSOLECALLBACK = Path(__file__).resolve().parent.parent.parent / "conf" / "daemon" / "consolecallback" @@ -57,3 +64,19 @@ class ConsoleWorkerTests(TestCase): self.cb.from_client(None, 0, None, self.console) self.assertFalse(self.console.running) self.console.stream.send.assert_not_called() + + +class AccessTests(DjangoTestCase): + """The worker ends a session whose user may no longer open the console.""" + + def test_an_owner_removed_meanwhile_loses_the_console(self): + cb = load() + compute = Compute.objects.create(name="c", hostname="192.0.2.1", login="u", password="p", type=1) + 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 + self.assertTrue(cb.may_still_open(owner.pk, vm)) + ownership.delete() + self.assertFalse(cb.may_still_open(owner.pk, vm)) + self.assertFalse(cb.may_still_open(999999, vm)) diff --git a/console/tests/test_live_serial.py b/console/tests/test_live_serial.py index aafd6adf..926da97b 100644 --- a/console/tests/test_live_serial.py +++ b/console/tests/test_live_serial.py @@ -23,6 +23,12 @@ from instances.tests import livetest P = livetest.PREFIX +def wait_for(condition, seconds=30): + deadline = time.monotonic() + seconds + while not condition() and time.monotonic() < deadline: + time.sleep(0.2) + + def serial_vm(conn, name): """A paused VM without graphics whose BIOS writes to the serial console once resumed.""" dom = conn.defineXML( @@ -57,6 +63,11 @@ class LiveSerialConsoleTestCase(TransactionTestCase): UserInstance.objects.create(user=self.user, instance=vm) return tokens.SIGNER.sign(f"{compute.id}:{vm.uuid}:{self.user.id}"), dom + def admin_token(self, token): + host, uuid, _user = 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}") + def test_the_console_of_a_running_vm_opens_once(self): for args in self.hosts: with self.subTest(host=args[0], type=args[3]): @@ -68,12 +79,17 @@ class LiveSerialConsoleTestCase(TransactionTestCase): while "SeaBIOS" not in "".join(received["output"]) and time.monotonic() < deadline: time.sleep(0.2) self.assertIn("SeaBIOS", "".join(received["output"])) - # one client per VM + # one client per VM; a superuser takes a console left open over with self.assertRaises(socketio.exceptions.ConnectionError): console(self.port, token) + admin_client, admin_received = console(self.port, self.admin_token(token)) + wait_for(lambda: "An administrator opened this console" in "".join(received["output"])) + self.assertIn("An administrator opened this console", "".join(received["output"])) client.disconnect() - deadline = time.monotonic() + 5 - while consoles(self.server.pid) and time.monotonic() < deadline: - time.sleep(0.05) - self.assertEqual(consoles(self.server.pid), []) + # the VM stops: the terminal says so dom.destroy() + wait_for(lambda: "not running" in "".join(admin_received["output"])) + self.assertIn("The VM is not running", "".join(admin_received["output"])) + admin_client.disconnect() + wait_for(lambda: not consoles(self.server.pid)) + self.assertEqual(consoles(self.server.pid), []) diff --git a/console/tests/test_socketiod.py b/console/tests/test_socketiod.py index 1b4e203f..274c09f9 100644 --- a/console/tests/test_socketiod.py +++ b/console/tests/test_socketiod.py @@ -40,7 +40,7 @@ class Base(TestCase): 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.assertEqual(tokens.authorize(self.token()), (self.vm, self.user)) def test_refused_tokens(self): stranger = get_user_model().objects.create_user("stranger", password="pw") @@ -142,6 +142,18 @@ class SocketiodTestCase(Base): write.assert_not_called() self.assertIsNone(session_input) + @override_settings(SERIAL_CONSOLE_ENABLED=True) + def test_a_superuser_takes_over_a_console_left_open(self): + admin = get_user_model().objects.create_superuser("root", "root@example.com", "pw") + self.d.connect("owner-tab", {}, {"token": self.token()}) + old = self.d.sessions["owner-tab"] + with patch.object(self.d.sio, "emit") as emit, patch.object(self.d.sio, "disconnect") as disconnect: + self.assertIsNone(self.d.connect("admin-tab", {}, {"token": self.token(admin)})) + self.assertEqual(list(self.d.sessions), ["admin-tab"]) + self.assertFalse(old["open"]) + self.assertIn("An administrator opened this console", emit.call_args.args[1]["output"]) + disconnect.assert_called_once_with("owner-tab") + @override_settings(SERIAL_CONSOLE_ENABLED=True) def test_two_connections_at_once_start_one_console(self): import threading @@ -157,7 +169,7 @@ class SocketiodTestCase(Base): threads = [threading.Thread(target=connect, args=(n,)) for n in range(8)] # the threads have their own database connections, outside this test's transaction - with patch.object(self.d, "authorize", return_value=self.vm): + with patch.object(self.d, "authorize", return_value=(self.vm, self.user)): for t in threads: t.start() for t in threads: diff --git a/console/tests/test_views.py b/console/tests/test_views.py index 832c000a..ef3bf7d7 100644 --- a/console/tests/test_views.py +++ b/console/tests/test_views.py @@ -606,7 +606,7 @@ class NovncdDaemonLogicTestCase(TestCase): mock_instance.compute.password = "" mock_instance.compute.type = 1 mock_instance.name = "test-vm" - mock_inst_get.return_value = mock_instance + mock_inst_get.return_value = (mock_instance, user) mock_conn = MagicMock() mock_conn.get_console_listener_addr.return_value = "127.0.0.1" diff --git a/console/tokens.py b/console/tokens.py index b624981f..b9f84a71 100644 --- a/console/tokens.py +++ b/console/tokens.py @@ -14,7 +14,8 @@ MAX_AGE = 120 def authorize(token): - """The Instance the token opens; PermissionError otherwise.""" + """The Instance the token opens and the user it was given to; + PermissionError otherwise.""" try: host, uuid, user_id = SIGNER.unsign(token, max_age=MAX_AGE).split(":") host, user_id = int(host), int(user_id) @@ -24,4 +25,4 @@ def authorize(token): 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 + return instance, user