From 0e2e8d91075b9b68fa7c3fd3557e654c1f8917a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Pierret=20=28fepitre=29?= Date: Sat, 23 Nov 2024 14:54:46 +0100 Subject: [PATCH] Introduce LocalVM to extract BaseVM being smaller class It prepares the work for RemoteVM. --- doc/qubes-features.rst | 2 +- doc/qubes-vm/index.rst | 6 +- qubes/app.py | 10 +- qubes/ext/__init__.py | 2 +- qubes/features.py | 2 +- qubes/tests/__init__.py | 2 +- qubes/tests/init.py | 2 +- qubes/tests/integ/backupcompatibility.py | 2 +- qubes/tests/storage_callback.py | 2 +- qubes/tests/storage_lvm.py | 4 +- qubes/tests/vm/init.py | 4 +- qubes/vm/__init__.py | 210 +++++++++++++---------- qubes/vm/adminvm.py | 6 +- qubes/vm/qubesvm.py | 6 +- run-tests | 2 +- 15 files changed, 151 insertions(+), 111 deletions(-) diff --git a/doc/qubes-features.rst b/doc/qubes-features.rst index 6f257e7f6..fd62f7ce0 100644 --- a/doc/qubes-features.rst +++ b/doc/qubes-features.rst @@ -72,7 +72,7 @@ request in ``features-request`` event handler. If no extension handles given feature request, it will be ignored. The extension should carefuly validate requested features (ignoring those not recognized - may be for another extension) and only then set appropriate value on VM object -(:py:attr:`qubes.vm.BaseVM.features`). It is recommended to make the +(:py:attr:`qubes.vm.LocalVM.features`). It is recommended to make the verification code as bulletproof as possible (for example allow only specific simple values, instead of complex structures), because feature requests come from untrusted sources. The features actually set on the VM in some cases may diff --git a/doc/qubes-vm/index.rst b/doc/qubes-vm/index.rst index 7023be321..755d6c0a7 100644 --- a/doc/qubes-vm/index.rst +++ b/doc/qubes-vm/index.rst @@ -19,13 +19,13 @@ two, the :py:class:`qubes.vm.qubesvm.QubesVM` cares about Qubes-specific actions, that are more or less directly related to security model. It is intended to be easily auditable by non-expert programmers (ie. we don't use Python's magic there). The second class is its parent, -:py:class:`qubes.vm.BaseVM`, which is concerned about technicalities like XML +:py:class:`qubes.vm.LocalVM`, which is concerned about technicalities like XML serialising/deserialising. It is of less concern to threat model auditors, but still relevant to overall security of the Qubes OS. It is written for programmers by programmers. The second object is the XML node that refers to the domain. It can be accessed -as :py:attr:`Qubes.vm.BaseVM.xml` attribute of the domain object. The third one +as :py:attr:`Qubes.vm.LocalVM.xml` attribute of the domain object. The third one is :py:attr:`Qubes.vm.qubesvm.QubesVM.libvirt_domain` object for directly interacting with libvirt. Those objects are intended to be used from core and/or plugins, but not directly by user or from qvm-tools. They are however public, so @@ -48,7 +48,7 @@ Package contents Main public classes ^^^^^^^^^^^^^^^^^^^ -.. autoclass:: qubes.vm.BaseVM +.. autoclass:: qubes.vm.LocalVM :members: :show-inheritance: diff --git a/qubes/app.py b/qubes/app.py index 1017d171a..8296dbde2 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -503,7 +503,7 @@ def vms(self): def add(self, value, _enable_events=True): """Add VM to collection - :param qubes.vm.BaseVM value: VM to add + :param qubes.vm.LocalVM value: VM to add :param _enable_events: :raises TypeError: when value is of wrong type :raises ValueError: when there is already VM which has equal ``qid`` @@ -511,9 +511,11 @@ def add(self, value, _enable_events=True): # this violates duck typing, but is needed # for VMProperty to function correctly - if not isinstance(value, qubes.vm.BaseVM): + if not isinstance(value, qubes.vm.LocalVM): raise TypeError( - "{} holds only BaseVM instances".format(self.__class__.__name__) + "{} holds only LocalVM instances".format( + self.__class__.__name__ + ) ) if value.qid in self: @@ -543,7 +545,7 @@ def __getitem__(self, key): return vm raise KeyError(key) - if isinstance(key, qubes.vm.BaseVM): + if isinstance(key, qubes.vm.LocalVM): key = key.uuid if isinstance(key, uuid.UUID): diff --git a/qubes/ext/__init__.py b/qubes/ext/__init__.py index bc8129838..517ed0ddc 100644 --- a/qubes/ext/__init__.py +++ b/qubes/ext/__init__.py @@ -94,7 +94,7 @@ def decorator(func): elif "vm" in kwargs: func.ha_vm = kwargs["vm"] else: - func.ha_vm = qubes.vm.BaseVM + func.ha_vm = qubes.vm.LocalVM return func diff --git a/qubes/features.py b/qubes/features.py index 38e0eaf4e..1c0499de0 100644 --- a/qubes/features.py +++ b/qubes/features.py @@ -161,7 +161,7 @@ def _recursive_check( raise NotImplementedError("app does not have features yet") assert isinstance( - self.subject, _vm.BaseVM + self.subject, _vm.LocalVM ), "recursive checks do not work for {}".format( type(self.subject).__name__ ) diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py index 4189ed06c..4e365c37e 100644 --- a/qubes/tests/__init__.py +++ b/qubes/tests/__init__.py @@ -507,7 +507,7 @@ def cleanup_gc(self): obj, ( qubes.Qubes, - qubes.vm.BaseVM, + qubes.vm.LocalVM, libvirt.virConnect, libvirt.virDomain, ), diff --git a/qubes/tests/init.py b/qubes/tests/init.py index fae1aedfc..a846f8324 100644 --- a/qubes/tests/init.py +++ b/qubes/tests/init.py @@ -402,7 +402,7 @@ def test_010_property_require(self): pass -class TestVM(qubes.vm.BaseVM): +class TestVM(qubes.vm.LocalVM): qid = qubes.property("qid", type=int) name = qubes.property("name") uuid = uuid.uuid5(uuid.NAMESPACE_DNS, "testvm") diff --git a/qubes/tests/integ/backupcompatibility.py b/qubes/tests/integ/backupcompatibility.py index e5163c882..e45404b9a 100644 --- a/qubes/tests/integ/backupcompatibility.py +++ b/qubes/tests/integ/backupcompatibility.py @@ -494,7 +494,7 @@ def assertRestored(self, name, **kwargs): ) else: actual_value = getattr(vm, prop) - if isinstance(actual_value, qubes.vm.BaseVM): + if isinstance(actual_value, qubes.vm.LocalVM): self.assertEqual( value, actual_value.name, diff --git a/qubes/tests/storage_callback.py b/qubes/tests/storage_callback.py index 07fa93b0e..bbbe26a68 100644 --- a/qubes/tests/storage_callback.py +++ b/qubes/tests/storage_callback.py @@ -240,7 +240,7 @@ def tearDown(self): self.app.close() del self.app for attr in dir(self): - if isinstance(getattr(self, attr), qubes.vm.BaseVM): + if isinstance(getattr(self, attr), qubes.vm.LocalVM): delattr(self, attr) if os.path.exists(self.test_log): diff --git a/qubes/tests/storage_lvm.py b/qubes/tests/storage_lvm.py index 80060557a..62bfd3971 100644 --- a/qubes/tests/storage_lvm.py +++ b/qubes/tests/storage_lvm.py @@ -176,7 +176,7 @@ def tearDown(self): self.app.close() del self.app for attr in dir(self): - if isinstance(getattr(self, attr), qubes.vm.BaseVM): + if isinstance(getattr(self, attr), qubes.vm.LocalVM): delattr(self, attr) def test_000_default_thin_pool(self): @@ -1488,7 +1488,7 @@ def tearDown(self): self.app.close() del self.app for attr in dir(self): - if isinstance(getattr(self, attr), qubes.vm.BaseVM): + if isinstance(getattr(self, attr), qubes.vm.LocalVM): delattr(self, attr) def test_000_search_thin_pool(self): diff --git a/qubes/tests/vm/init.py b/qubes/tests/vm/init.py index 3ad3fb4ff..04c6f1423 100644 --- a/qubes/tests/vm/init.py +++ b/qubes/tests/vm/init.py @@ -44,7 +44,7 @@ def __init__(self): self.vmm = TestVMM() -class TestVM(qubes.vm.BaseVM): +class TestVM(qubes.vm.LocalVM): qid = qubes.property("qid", type=int) name = qubes.property("name") testprop = qubes.property("testprop") @@ -55,7 +55,7 @@ def is_running(self): return False -class TC_10_BaseVM(qubes.tests.QubesTestCase): +class TC_10_LocalVM(qubes.tests.QubesTestCase): def setUp(self): super().setUp() self.xml = lxml.etree.XML( diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py index 840944ea8..e5ee67026 100644 --- a/qubes/vm/__init__.py +++ b/qubes/vm/__init__.py @@ -206,10 +206,10 @@ class BaseVM(qubes.PropertyHolder): :param app: Qubes application context :type app: :py:class:`qubes.Qubes` - :param xml: xml node from which to deserialise + :param xml: xml node from which to deserialize :type xml: :py:class:`lxml.etree._Element` or :py:obj:`None` - This class is responsible for serializing and deserialising machines and + This class is responsible for serializing and deserializing machines and provides basic framework. It contains no management logic. For that, see :py:class:`qubes.vm.qubesvm.QubesVM`. """ @@ -249,6 +249,113 @@ class BaseVM(qubes.PropertyHolder): padlock is set.""", ) + def __init__(self, app, xml, features=None, tags=None, **kwargs): + # self.app must be set before super().__init__, because some property + # setters need working .app attribute + #: mother :py:class:`qubes.Qubes` object + self.app = app + + super().__init__(xml, **kwargs) + + #: dictionary of features of this qube + self.features = qubes.features.Features(self, features) + + #: user-specified tags + self.tags = Tags(self, tags or ()) + + #: logger instance for logging messages related to this VM + self.log = None + + if hasattr(self, "name"): + self.init_log() + + @qubes.stateless_property + def klass(self): + """Domain class name""" + return type(self).__name__ + + def close(self): + super().close() + del self.app + del self.features + del self.tags + + def load_extras(self): + if self.xml is None: + return + + # features + for node in self.xml.xpath("./features/feature"): + self.features[node.get("name")] = node.text + + # tags + for node in self.xml.xpath("./tags/tag"): + self.tags.add(node.get("name")) + + # SEE:1815 firewall, policy. + + def init_log(self): + """Initialise logger for this domain.""" + self.log = qubes.log.get_vm_logger(self.name) + + def __xml__(self): + element = lxml.etree.Element("domain") + element.set("id", "domain-" + str(self.qid)) + element.set("class", self.__class__.__name__) + + element.append(self.xml_properties()) + + features = lxml.etree.Element("features") + for feature in self.features: + node = lxml.etree.Element("feature", name=feature) + node.text = self.features[feature] + features.append(node) + element.append(features) + + tags = lxml.etree.Element("tags") + for tag in self.tags: + node = lxml.etree.Element("tag", name=tag) + tags.append(node) + element.append(tags) + + return element + + def __repr__(self): + proprepr = [] + for prop in self.property_list(): + if prop.__name__ in ("name", "qid"): + continue + try: + proprepr.append( + "{}={!s}".format( + prop.__name__, getattr(self, prop.__name__) + ) + ) + except AttributeError: + continue + + return "<{} at {:#x} name={!r} qid={!r} {}>".format( + type(self).__name__, + id(self), + self.name, + self.qid, + " ".join(proprepr), + ) + + +class LocalVM(BaseVM): + """Base class for all local VMs + + :param app: Qubes application context + :type app: :py:class:`qubes.Qubes` + :param xml: xml node from which to deserialize + :type xml: :py:class:`lxml.etree._Element` or :py:obj:`None` + + This class is responsible for serializing and deserializing machines and + provides basic framework. It contains no management logic. For that, see + :py:class:`qubes.vm.qubesvm.QubesVM`. + """ + def __init__( self, app, xml, features=None, devices=None, tags=None, **kwargs ): @@ -262,33 +369,20 @@ def __init__( #: mother :py:class:`qubes.Qubes` object self.app = app - super().__init__(xml, **kwargs) - - #: dictionary of features of this qube - self.features = qubes.features.Features(self, features) + super().__init__(app, xml, features, tags, **kwargs) #: :py:class:`DeviceManager` object keeping devices that are attached to #: this domain self.devices = devices or qubes.devices.DeviceManager(self) - #: user-specified tags - self.tags = Tags(self, tags or ()) - - #: logger instance for logging messages related to this VM - self.log = None - #: storage volumes self.volumes = {} #: storage manager self.storage = None - if hasattr(self, "name"): - self.init_log() - def close(self): - super().close() - + # pylint: disable=no-member if self._qdb_connection_watch is not None: asyncio.get_event_loop().remove_reader( self._qdb_connection_watch.watch_fd() @@ -296,21 +390,18 @@ def close(self): self._qdb_connection_watch.close() del self._qdb_connection_watch - del self.app - del self.features del self.storage # TODO storage may have circ references, but it doesn't leak fds del self.devices - del self.tags + + super().close() def load_extras(self): + super().load_extras() + if self.xml is None: return - # features - for node in self.xml.xpath("./features/feature"): - self.features[node.get("name")] = node.text - # devices (pci, usb, ...) for parent in self.xml.xpath("./devices"): devclass = parent.get("class") @@ -376,12 +467,6 @@ def load_extras(self): self.log.info(msg) continue - # tags - for node in self.xml.xpath("./tags/tag"): - self.tags.add(node.get("name")) - - # SEE:1815 firewall, policy. - def get_provided_assignments( self, required_only: bool = False ) -> List["qubes.device_protocol.DeviceAssignment"]: @@ -401,23 +486,8 @@ def get_provided_assignments( assignments.append(assignment) return assignments - def init_log(self): - """Initialise logger for this domain.""" - self.log = qubes.log.get_vm_logger(self.name) - def __xml__(self): - element = lxml.etree.Element("domain") - element.set("id", "domain-" + str(self.qid)) - element.set("class", self.__class__.__name__) - - element.append(self.xml_properties()) - - features = lxml.etree.Element("features") - for feature in self.features: - node = lxml.etree.Element("feature", name=feature) - node.text = self.features[feature] - features.append(node) - element.append(features) + element = super().__xml__() for devclass in self.devices: devices = lxml.etree.Element("devices") @@ -437,36 +507,8 @@ def __xml__(self): devices.append(node) element.append(devices) - tags = lxml.etree.Element("tags") - for tag in self.tags: - node = lxml.etree.Element("tag", name=tag) - tags.append(node) - element.append(tags) - return element - def __repr__(self): - proprepr = [] - for prop in self.property_list(): - if prop.__name__ in ("name", "qid"): - continue - try: - proprepr.append( - "{}={!s}".format( - prop.__name__, getattr(self, prop.__name__) - ) - ) - except AttributeError: - continue - - return "<{} at {:#x} name={!r} qid={!r} {}>".format( - type(self).__name__, - id(self), - self.name, - self.qid, - " ".join(proprepr), - ) - # # xml serialising methods # @@ -495,7 +537,7 @@ def watch_qdb_path(self, path): You can call this method for example in response to `domain-init` and `domain-load` events. """ - + # pylint: disable=no-member if path not in self._qdb_watch_paths: self._qdb_watch_paths.add(path) if self._qdb_connection_watch: @@ -510,6 +552,7 @@ def _qdb_watch_reader(self, loop): import qubesdb # pylint: disable=import-error try: + # pylint: disable=no-member path = self._qdb_connection_watch.read_watch() for watched_path in self._qdb_watch_paths: if watched_path == path or ( @@ -530,6 +573,7 @@ def start_qdb_watch(self, loop=None): class. """ # cleanup old watch connection first, if any + # pylint: disable=no-member if self._qdb_connection_watch is not None: asyncio.get_event_loop().remove_reader( self._qdb_connection_watch.watch_fd() @@ -547,33 +591,27 @@ def start_qdb_watch(self, loop=None): for path in self._qdb_watch_paths: self._qdb_connection_watch.watch(path) - @qubes.stateless_property - def klass(self): - """Domain class name""" - return type(self).__name__ - class VMProperty(qubes.property): """Property that is referring to a VM - :param type vmclass: class that returned VM is supposed to be instance of + :param type vmclass: class that returned VM is supposed to be an instance of - and all supported by :py:class:`property` with the exception of ``type`` \ - and ``setter`` + and all supported by :py:class:`property` except ``type`` and ``setter`` """ _none_value = "" - def __init__(self, name, vmclass=BaseVM, allow_none=False, **kwargs): + def __init__(self, name, vmclass=LocalVM, allow_none=False, **kwargs): if "type" in kwargs: raise TypeError( "'type' keyword parameter is unsupported in {}".format( self.__class__.__name__ ) ) - if not issubclass(vmclass, BaseVM): + if not issubclass(vmclass, LocalVM): raise TypeError( - "'vmclass' should specify a subclass of qubes.vm.BaseVM" + "'vmclass' should specify a subclass of qubes.vm.LocalVM" ) super().__init__( diff --git a/qubes/vm/adminvm.py b/qubes/vm/adminvm.py index 11323415f..b84e4e1bf 100644 --- a/qubes/vm/adminvm.py +++ b/qubes/vm/adminvm.py @@ -31,10 +31,10 @@ import qubes.exc import qubes.vm from qubes.vm.qubesvm import _setter_kbd_layout -from qubes.vm import BaseVM +from qubes.vm import LocalVM -class AdminVM(BaseVM): +class AdminVM(LocalVM): """Dom0""" dir_path = None @@ -98,7 +98,7 @@ def __str__(self): return self.name def __lt__(self, other: object): - if not isinstance(other, BaseVM): + if not isinstance(other, LocalVM): return NotImplemented # order dom0 before anything if not isinstance(other, AdminVM): diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 195b28510..353225843 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -33,7 +33,7 @@ import uuid import libvirt # pylint: disable=import-error -import lxml +import lxml.etree import qubes import qubes.config @@ -271,7 +271,7 @@ def _default_kernelopts(self): ) + extra_opts -class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): +class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.LocalVM): """Base functionality of Qubes VM shared between all VMs. The following events are raised on this class or its subclasses: @@ -1069,7 +1069,7 @@ def __hash__(self): return self.qid def __lt__(self, other): - if not isinstance(other, qubes.vm.BaseVM): + if not isinstance(other, qubes.vm.LocalVM): return NotImplemented if isinstance(other, qubes.vm.adminvm.AdminVM): return False diff --git a/run-tests b/run-tests index 6c1b22dd1..21b2e3421 100755 --- a/run-tests +++ b/run-tests @@ -8,7 +8,7 @@ install_rpm_deps () { applications=(lvm2 python3-docutils python3-pyyaml python3-jinja2 python3-lxml btrfs-progs vim-common python3-coverage python3-inotify cryptsetup) rpm -q --quiet -- "${applications[@]}" || - sudo dnf -- install "${applications[@]}" || + sudo dnf install "${applications[@]}" || : # we don’t actually care if this succeeds } if { command -pv rpm && command -pv dnf; }>/dev/null; then install_rpm_deps; fi