From f223594f92eca167973d4c9cdd262c48251c73d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 1 Dec 2017 03:05:57 +0100 Subject: [PATCH] app: kill default_fw_netvm property Having both default_netvm and default_fw_netvm cause a lot of confusion, because it isn't clear for the user which one is used when. Additionally changing provides_network property may also change netvm property, which may be unintended effect. This as a whole make it hard to: - cover all netvm-changing actions with policy for Admin API - cover all netvm-changing events (for example to apply the change to the running VM, or to check for netvm loops) As suggested by @qubesuser, kill the default_fw_netvm property and simplify the logic around it. Since we're past rc1, implement also migration logic. And add tests for said migration. Fixes QubesOS/qubes-issues#3247 --- qubes/app.py | 56 +++++++++++++++--- qubes/core2migration.py | 5 -- qubes/tests/app.py | 122 ++++++++++++++++++++++++++++++++++++++++ qubes/vm/mix/net.py | 3 +- 4 files changed, 170 insertions(+), 16 deletions(-) diff --git a/qubes/app.py b/qubes/app.py index afebdcf8..f81bdf29 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -724,11 +724,6 @@ class Qubes(qubes.PropertyHolder): setter=_setter_default_netvm, doc='''Default NetVM for AppVMs. Initial state is `None`, which means that AppVMs are not connected to the Internet.''') - default_fw_netvm = qubes.VMProperty('default_fw_netvm', load_stage=3, - default=None, allow_none=True, - doc='''Default NetVM for ProxyVMs. Initial state is `None`, which means - that ProxyVMs (including FirewallVM) are not connected to the - Internet.''') default_template = qubes.VMProperty('default_template', load_stage=3, vmclass=qubes.vm.templatevm.TemplateVM, doc='Default template for new AppVMs') @@ -838,6 +833,50 @@ class Qubes(qubes.PropertyHolder): def store(self): return self._store + def _migrate_global_properties(self): + '''Migrate renamed/dropped properties''' + if self.xml is None: + return + + # drop default_fw_netvm + node_default_fw_netvm = self.xml.find( + './properties/property[@name=\'default_fw_netvm\']') + if node_default_fw_netvm is not None: + node_default_netvm = self.xml.find( + './properties/property[@name=\'default_netvm\']') + try: + default_fw_netvm = self.domains[node_default_fw_netvm.text] + if node_default_netvm is None: + default_netvm = None + else: + default_netvm = self.domains[node_default_netvm.text] + if default_netvm != default_fw_netvm: + for vm in self.domains: + if not hasattr(vm, 'netvm'): + continue + if not getattr(vm, 'provides_network', False): + continue + node_netvm = vm.xml.find( + './properties/property[@name=\'netvm\']') + if node_netvm is not None: + # non-default netvm + continue + # this will unfortunately break "being default" + # property state, but the alternative (changing + # value behind user's back) is worse + properties = vm.xml.find('./properties') + element = lxml.etree.Element('property', + name='netvm') + element.text = default_fw_netvm.name + # manipulate xml directly, before loading netvm + # property, to avoid hitting netvm loop detection + properties.append(element) + except KeyError: + # if default_fw_netvm was set to invalid value, simply + # drop it + pass + node_default_fw_netvm.getparent().remove(node_default_fw_netvm) + def load(self, lock=False): '''Open qubes.xml @@ -876,6 +915,8 @@ class Qubes(qubes.PropertyHolder): qubes.vm.adminvm.AdminVM(self, None), _enable_events=False) + self._migrate_global_properties() + # stage 3: load global properties self.load_properties(load_stage=3) @@ -886,7 +927,6 @@ class Qubes(qubes.PropertyHolder): # stage 5: misc fixups - self.property_require('default_fw_netvm', allow_none=True) self.property_require('default_netvm', allow_none=True) self.property_require('default_template') self.property_require('clockvm', allow_none=True) @@ -1305,9 +1345,7 @@ class Qubes(qubes.PropertyHolder): if oldvalue and oldvalue.features.get('service.clocksync', False): del oldvalue.features['service.clocksync'] - @qubes.events.handler( - 'property-pre-set:default_netvm', - 'property-pre-set:default_fw_netvm') + @qubes.events.handler('property-pre-set:default_netvm') def on_property_pre_set_default_netvm(self, event, name, newvalue, oldvalue=None): # pylint: disable=unused-argument,invalid-name diff --git a/qubes/core2migration.py b/qubes/core2migration.py index a090d1fe..80fb59b7 100644 --- a/qubes/core2migration.py +++ b/qubes/core2migration.py @@ -75,11 +75,6 @@ class Core2Qubes(qubes.Qubes): self.default_netvm = int(default_netvm) \ if default_netvm != "None" else None - default_fw_netvm = element.get("default_fw_netvm") - if default_fw_netvm is not None: - self.default_fw_netvm = int(default_fw_netvm) \ - if default_fw_netvm != "None" else None - updatevm = element.get("updatevm") if updatevm is not None: self.updatevm = int(updatevm) \ diff --git a/qubes/tests/app.py b/qubes/tests/app.py index c6f6f31e..04ad2171 100644 --- a/qubes/tests/app.py +++ b/qubes/tests/app.py @@ -270,6 +270,11 @@ class TC_89_QubesEmpty(qubes.tests.QubesTestCase): os.unlink('/tmp/qubestest.xml') except: pass + try: + self.app.close() + del self.app + except AttributeError: + pass super().tearDown() @qubes.tests.skipUnlessDom0 @@ -281,6 +286,123 @@ class TC_89_QubesEmpty(qubes.tests.QubesTestCase): pass qubes.Qubes.create_empty_store('/tmp/qubestest.xml').close() + def test_100_property_migrate_default_fw_netvm(self): + xml_template = ''' + + + {default_netvm} + {default_fw_netvm} + + + + + + + + + + + 1 + sys-net + True + + + 2fcfc1f4-b2fe-4361-931a-c5294b35edfa + + + + + + + + 2 + sys-firewall + True + + 9a6d9689-25f7-48c9-a15f-8205d6c5b7c6 + + + + + + 3 + appvm + + 1d6aab41-3262-400a-b3d3-21aae8fdbec8 + + + + + ''' + with self.subTest('default_setup'): + with open('/tmp/qubestest.xml', 'w') as xml_file: + xml_file.write(xml_template.format( + default_netvm='sys-firewall', + default_fw_netvm='sys-net')) + self.app = qubes.Qubes('/tmp/qubestest.xml', offline_mode=True) + self.assertEqual( + self.app.domains['sys-net'].netvm, None) + self.assertEqual( + self.app.domains['sys-firewall'].netvm, self.app.domains['sys-net']) + # property is no longer "default" + self.assertFalse( + self.app.domains['sys-firewall'].property_is_default('netvm')) + # verify that appvm.netvm is unaffected + self.assertTrue( + self.app.domains['appvm'].property_is_default('netvm')) + self.assertEqual( + self.app.domains['appvm'].netvm, + self.app.domains['sys-firewall']) + with self.assertRaises(AttributeError): + self.app.default_fw_netvm + + self.app.close() + del self.app + + with self.subTest('same'): + with open('/tmp/qubestest.xml', 'w') as xml_file: + xml_file.write(xml_template.format( + default_netvm='sys-net', + default_fw_netvm='sys-net')) + self.app = qubes.Qubes('/tmp/qubestest.xml', offline_mode=True) + self.assertEqual( + self.app.domains['sys-net'].netvm, None) + self.assertEqual( + self.app.domains['sys-firewall'].netvm, + self.app.domains['sys-net']) + self.assertTrue( + self.app.domains['sys-firewall'].property_is_default('netvm')) + # verify that appvm.netvm is unaffected + self.assertTrue( + self.app.domains['appvm'].property_is_default('netvm')) + self.assertEqual( + self.app.domains['appvm'].netvm, + self.app.domains['sys-net']) + with self.assertRaises(AttributeError): + self.app.default_fw_netvm + + with self.subTest('loop'): + with open('/tmp/qubestest.xml', 'w') as xml_file: + xml_file.write(xml_template.format( + default_netvm='sys-firewall', + default_fw_netvm='sys-firewall')) + self.app = qubes.Qubes('/tmp/qubestest.xml', offline_mode=True) + self.assertEqual( + self.app.domains['sys-net'].netvm, None) + # this was netvm loop, better set to none, to not crash qubesd + self.assertEqual( + self.app.domains['sys-firewall'].netvm, None) + self.assertFalse( + self.app.domains['sys-firewall'].property_is_default('netvm')) + # verify that appvm.netvm is unaffected + self.assertTrue( + self.app.domains['appvm'].property_is_default('netvm')) + self.assertEqual( + self.app.domains['appvm'].netvm, + self.app.domains['sys-firewall']) + with self.assertRaises(AttributeError): + self.app.default_fw_netvm + class TC_90_Qubes(qubes.tests.QubesTestCase): def tearDown(self): diff --git a/qubes/vm/mix/net.py b/qubes/vm/mix/net.py index 139825d2..22b68146 100644 --- a/qubes/vm/mix/net.py +++ b/qubes/vm/mix/net.py @@ -94,8 +94,7 @@ class NetVMMixin(qubes.events.Emitter): # CORE2: swallowed uses_default_netvm netvm = qubes.VMProperty('netvm', load_stage=4, allow_none=True, - default=(lambda self: self.app.default_fw_netvm if self.provides_network - else self.app.default_netvm), + default=(lambda self: self.app.default_netvm), setter=_setter_netvm, doc='''VM that provides network connection to this domain. When `None`, machine is disconnected. When absent, domain uses default