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

@@ -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

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) \

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 = '''<?xml version="1.0" encoding="utf-8" ?>
+        <qubes version="3.0">
+            <properties>
+                <property name="default_netvm">{default_netvm}</property>
+                <property name="default_fw_netvm">{default_fw_netvm}</property>
+            </properties>
+            <labels>
+                <label id="label-1" color="#cc0000">red</label>
+            </labels>
+            <pools>
+              <pool driver="file" dir_path="/tmp/qubes-test" name="default"/>
+            </pools>
+            <domains>
+                <domain class="StandaloneVM" id="domain-1">
+                    <properties>
+                        <property name="qid">1</property>
+                        <property name="name">sys-net</property>
+                        <property name="provides_network">True</property>
+                        <property name="label" ref="label-1" />
+                        <property name="netvm"></property>
+                        <property name="uuid">2fcfc1f4-b2fe-4361-931a-c5294b35edfa</property>
+                    </properties>
+                    <features/>
+                    <devices class="pci"/>
+                </domain>
+
+                <domain class="StandaloneVM" id="domain-2">
+                    <properties>
+                        <property name="qid">2</property>
+                        <property name="name">sys-firewall</property>
+                        <property name="provides_network">True</property>
+                        <property name="label" ref="label-1" />
+                        <property name="uuid">9a6d9689-25f7-48c9-a15f-8205d6c5b7c6</property>
+                    </properties>
+                </domain>
+
+                <domain class="StandaloneVM" id="domain-3">
+                    <properties>
+                        <property name="qid">3</property>
+                        <property name="name">appvm</property>
+                        <property name="label" ref="label-1" />
+                        <property name="uuid">1d6aab41-3262-400a-b3d3-21aae8fdbec8</property>
+                    </properties>
+                </domain>
+            </domains>
+        </qubes>
+        '''
+        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):

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