Improve checking for netvm loop

There were many cases were the check was missing:
 - changing default_netvm
 - resetting netvm to default value
 - loading already broken qubes.xml

Since it was possible to create broken qubes.xml using legal calls, do
not reject loading such file, instead break the loop(s) by setting netvm
to None when loop is detected. This will be also useful if still not all
places are covered...

Place the check in default_netvm setter. Skip it during qubes.xml loading
(when events_enabled=False), but still keep it in setter, to _validate_ the
value before any property-* event got fired.

qubes/app.py

@@ -606,6 +606,27 @@ def _setter_pool(app, prop, value):
         raise qubes.exc.QubesPropertyValueError(app, prop, value,
             'No such storage pool')
 
+def _setter_default_netvm(app, prop, value):
+    # skip netvm loop check while loading qubes.xml, to avoid tricky loading
+    # order
+    if not app.events_enabled:
+        return value
+
+    if value is None:
+        return value
+    # forbid setting to a value that would result in netvm loop
+    for vm in app.domains:
+        if not hasattr(vm, 'netvm'):
+            continue
+        if not vm.property_is_default('netvm'):
+            continue
+        if value == vm \
+                or value in app.domains.get_vms_connected_to(vm):
+            raise qubes.exc.QubesPropertyValueError(app, prop, value,
+                'Network loop on \'{!s}\''.format(vm))
+    return value
+
+
 class Qubes(qubes.PropertyHolder):
     '''Main Qubes application
 
@@ -664,6 +685,7 @@ class Qubes(qubes.PropertyHolder):
 
     default_netvm = qubes.VMProperty('default_netvm', load_stage=3,
         default=None, allow_none=True,
+        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,

qubes/tests/app.py

@@ -327,6 +327,30 @@ class TC_90_Qubes(qubes.tests.QubesTestCase):
         self.assertIn('service.clocksync', self.template.features)
         self.assertTrue(self.template.features['service.clocksync'])
 
+    def test_110_netvm_loop(self):
+        '''Netvm loop through default_netvm'''
+        netvm = self.app.add_new_vm('AppVM', name='test-net',
+            template=self.template, label='red')
+        try:
+            self.app.default_netvm = None
+            netvm.netvm = qubes.property.DEFAULT
+            with self.assertRaises(ValueError):
+                self.app.default_netvm = netvm
+        finally:
+            del netvm
+
+    def test_111_netvm_loop(self):
+        '''Netvm loop through default_netvm'''
+        netvm = self.app.add_new_vm('AppVM', name='test-net',
+            template=self.template, label='red')
+        try:
+            netvm.netvm = None
+            self.app.default_netvm = netvm
+            with self.assertRaises(ValueError):
+                netvm.netvm = qubes.property.DEFAULT
+        finally:
+            del netvm
+
     def test_200_remove_template(self):
         appvm = self.app.add_new_vm('AppVM', name='test-vm',
             template=self.template,
@@ -351,6 +375,7 @@ class TC_90_Qubes(qubes.tests.QubesTestCase):
         netvm = self.app.add_new_vm('AppVM', name='test-netvm',
             template=self.template, provides_network=True,
             label='red')
+        netvm.netvm = None
         self.app.default_netvm = netvm
         with mock.patch.object(self.app, 'vmm'):
             with self.assertRaises(qubes.exc.QubesVMInUseError):

qubes/tests/vm/mix/net.py

@@ -40,10 +40,10 @@ class TC_00_NetVMMixin(
         # testing properties used here
         self.netvm1 = qubes.vm.qubesvm.QubesVM(self.app, None, qid=2,
             name=qubes.tests.VMPREFIX + 'netvm1',
-            provides_network=True)
+            provides_network=True, netvm=None)
         self.netvm2 = qubes.vm.qubesvm.QubesVM(self.app, None, qid=3,
             name=qubes.tests.VMPREFIX + 'netvm2',
-            provides_network=True)
+            provides_network=True, netvm=None)
         self.nonetvm = qubes.vm.qubesvm.QubesVM(self.app, None, qid=4,
             name=qubes.tests.VMPREFIX + 'nonet')
         self.app.domains = qubes.app.VMCollection(self.app)

qubes/vm/mix/net.py

@@ -70,10 +70,13 @@ def _setter_netvm(self, prop, value):
         raise qubes.exc.QubesValueError(
             'The {!s} qube does not provide network'.format(value))
 
-    if value is self \
-            or value in self.app.domains.get_vms_connected_to(self):
-        raise qubes.exc.QubesValueError(
-            'Loops in network are unsupported')
+    # skip check for netvm loops during qubes.xml loading, to avoid tricky
+    # loading order
+    if self.events_enabled:
+        if value is self \
+                or value in self.app.domains.get_vms_connected_to(self):
+            raise qubes.exc.QubesValueError(
+                'Loops in network are unsupported')
     return value
 
 
@@ -187,6 +190,22 @@ class NetVMMixin(qubes.events.Emitter):
         self._firewall = None
         super(NetVMMixin, self).__init__(*args, **kwargs)
 
+    @qubes.events.handler('domain-load')
+    def on_domain_load_netvm_loop_check(self, event):
+        # pylint: disable=unused-argument
+        # make sure there are no netvm loops - which could cause qubesd
+        # looping infinitely
+        if self is self.netvm:
+            self.log.error(
+                'vm \'%s\' network-connected to itself, breaking the '
+                'connection', self.name)
+            self.netvm = None
+        elif self.netvm in self.app.domains.get_vms_connected_to(self):
+            self.log.error(
+                'netvm loop detected on \'%s\', breaking the connection',
+                self.name)
+            self.netvm = None
+
     @qubes.events.handler('domain-start')
     def on_domain_started(self, event, **kwargs):
         '''Connect this domain to its downstream domains. Also reload firewall
@@ -323,6 +342,8 @@ class NetVMMixin(qubes.events.Emitter):
         # pylint: disable=unused-argument
         # we are changing to default netvm
         newvalue = type(self).netvm.get_default(self)
+        # check for netvm loop
+        _setter_netvm(self, type(self).netvm, newvalue)
         if newvalue == oldvalue:
             return
         self.fire_event('property-pre-set:netvm', pre_event=True,