api/admin: split vm.volume.Clone to CloneFrom and CloneTo

The first operation returns a token, which can be passed to the second
one to actually perform clone operation. This way the caller needs have
power over both source and destination VMs (or at least appropriate
volumes), so it's easier to enforce appropriate qrexec policy.

The pending tokens are stored on Qubes() instance (as QubesAdminAPI is
not persistent). It is design choice to keep them in RAM only - those
are one time use and this way restarting qubesd is a simple way to
invalidate all of them. Otherwise we'd need some additional calls like
CloneCancel or such.

QubesOS/qubes-issues#2622

qubes/api/admin.py

@@ -293,22 +293,57 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
         self.dest.storage.get_pool(volume).revert(revision)
         self.app.save()
 
-    @qubes.api.method('admin.vm.volume.Clone')
+    @qubes.api.method('admin.vm.volume.CloneFrom', no_payload=True)
     @asyncio.coroutine
-    def vm_volume_clone(self, untrusted_payload):
+    def vm_volume_clone_from(self):
         assert self.arg in self.dest.volumes.keys()
-        untrusted_target = untrusted_payload.decode('ascii').strip()
-        del untrusted_payload
-        qubes.vm.validate_name(None, None, untrusted_target)
-        target_vm = self.app.domains[untrusted_target]
-        del untrusted_target
-        assert self.arg in target_vm.volumes.keys()
 
         volume = self.dest.volumes[self.arg]
 
-        self.fire_event_for_permission(target_vm=target_vm, volume=volume)
+        self.fire_event_for_permission(volume=volume)
 
-        yield from target_vm.storage.clone_volume(self.dest, self.arg)
+        token = qubes.utils.random_string(32)
+        # save token on self.app, as self is not persistent
+        if not hasattr(self.app, 'api_admin_pending_clone'):
+            self.app.api_admin_pending_clone = {}
+        # don't handle collisions any better - if someone is so much out of
+        # luck, can try again anyway
+        assert token not in self.app.api_admin_pending_clone
+
+        self.app.api_admin_pending_clone[token] = volume
+        return token
+
+    @qubes.api.method('admin.vm.volume.CloneTo')
+    @asyncio.coroutine
+    def vm_volume_clone_to(self, untrusted_payload):
+        assert self.arg in self.dest.volumes.keys()
+        untrusted_token = untrusted_payload.decode('ascii').strip()
+        del untrusted_payload
+        assert untrusted_token in getattr(self.app,
+            'api_admin_pending_clone', {})
+        token = untrusted_token
+        del untrusted_token
+
+        src_volume = self.app.api_admin_pending_clone[token]
+        del self.app.api_admin_pending_clone[token]
+
+        # make sure the volume still exists, but invalidate token anyway
+        assert str(src_volume.pool) in self.app.pools
+        assert src_volume in self.app.pools[str(src_volume.pool)].volumes
+
+        dst_volume = self.dest.volumes[self.arg]
+
+        self.fire_event_for_permission(src_volume=src_volume,
+            dst_volume=dst_volume)
+
+        op_retval = dst_volume.import_volume(src_volume)
+
+        # clone/import functions may be either synchronous or asynchronous
+        # in the later case, we need to wait for them to finish
+        if asyncio.iscoroutine(op_retval):
+            op_retval = yield from op_retval
+
+        self.dest.volumes[self.arg] = op_retval
         self.app.save()
 
     @qubes.api.method('admin.vm.volume.Resize')

qubes/tests/api_admin.py

@@ -21,6 +21,7 @@
 ''' Tests for management calls endpoints '''
 
 import asyncio
+import operator
 import os
 import shutil
 import unittest.mock
@@ -31,6 +32,7 @@ import qubes
 import qubes.devices
 import qubes.api.admin
 import qubes.tests
+import qubes.storage
 
 # properties defined in API
 volume_properties = [
@@ -1532,93 +1534,100 @@ class TC_00_VMs(AdminAPITestCase):
                 self.call_mgmt_func(b'admin.vm.volume.Import', b'test-vm1',
                     b'private')
 
+    def setup_for_clone(self):
+        self.pool = unittest.mock.MagicMock()
+        self.app.pools['test'] = self.pool
+        self.vm2 = self.app.add_new_vm('AppVM', label='red',
+            name='test-vm2',
+            template='test-template', kernel='')
+        self.pool.configure_mock(**{
+            'volumes': qubes.storage.VolumesCollection(self.pool),
+            'init_volume.return_value.pool': self.pool,
+            '__str__.return_value': 'test',
+            'get_volume.side_effect': (lambda vid:
+                self.vm.volumes['private']
+                    if vid is self.vm.volumes['private'].vid
+                    else self.vm2.volumes['private']
+            ),
+        })
+        self.loop.run_until_complete(
+            self.vm.create_on_disk(pool='test'))
+        self.loop.run_until_complete(
+            self.vm2.create_on_disk(pool='test'))
+
+        # the call replaces self.vm.volumes[...] with result of import
+        # operation - make sure it stays as the same object
+        self.vm.volumes['private'].import_volume.return_value = \
+            self.vm.volumes['private']
+        self.vm2.volumes['private'].import_volume.return_value = \
+            self.vm2.volumes['private']
+
     def test_520_vm_volume_clone(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
+        self.setup_for_clone()
-            template='test-template')
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
-        self.vm.volumes = unittest.mock.MagicMock()
+                b'test-vm1', b'private', b'')
-        self.vm2.volumes = unittest.mock.MagicMock()
+        # token
-        volumes_conf = {
+        self.assertEqual(len(token), 32)
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
+        self.assertFalse(self.app.save.called)
-        }
+        value = self.call_mgmt_func(b'admin.vm.volume.CloneTo',
-        self.vm.volumes.configure_mock(**volumes_conf)
+                b'test-vm2', b'private', token.encode())
-        self.vm2.volumes.configure_mock(**volumes_conf)
+        self.assertIsNone(value)
-        self.vm2.storage = unittest.mock.Mock()
+        self.vm2.volumes['private'].import_volume.assert_called_once_with(
-        func_mock = unittest.mock.Mock()
+            self.vm.volumes['private']
-
+        )
-        @asyncio.coroutine
+        self.vm2.volumes['private'].import_volume.assert_called_once_with(
-        def coroutine_mock(*args, **kwargs):
+            self.vm2.volumes['private']
-            return func_mock(*args, **kwargs)
+        )
-        self.vm2.storage.clone_volume = coroutine_mock
-
-        self.call_mgmt_func(b'admin.vm.volume.Clone',
-                b'test-vm1', b'private', b'test-vm2')
-        self.assertEqual(self.vm.volumes.mock_calls,
-            [('keys', (), {}),
-             ('__getitem__', ('private', ), {}),
-             ('__getitem__().__hash__', (), {}),
-            ])
-        self.assertEqual(self.vm2.volumes.mock_calls,
-            [unittest.mock.call.keys()])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [
-            unittest.mock.call(self.vm, 'private')
-        ])
         self.app.save.assert_called_once_with()
 
     def test_521_vm_volume_clone_invalid_volume(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
+        self.setup_for_clone()
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
-
-        @asyncio.coroutine
-        def coroutine_mock(*args, **kwargs):
-            return func_mock(*args, **kwargs)
-        self.vm2.storage.clone_volume = coroutine_mock
 
         with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'admin.vm.volume.Clone',
+            self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
-                    b'test-vm1', b'private123', b'test-vm2')
+                    b'test-vm1', b'private123', None)
-        self.assertEqual(self.vm.volumes.mock_calls,
+        self.assertNotIn('init_volume().import_volume',
-            [('keys', (), {})])
+            map(operator.itemgetter(0), self.pool.mock_calls))
-        self.assertEqual(self.vm2.volumes.mock_calls, [])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
-    def test_522_vm_volume_clone_invalid_vm(self):
+    def test_522_vm_volume_clone_invalid_volume2(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
+        self.setup_for_clone()
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
 
-        @asyncio.coroutine
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
-        def coroutine_mock(*args, **kwargs):
+                b'test-vm1', b'private', b'')
-            return func_mock(*args, **kwargs)
+        with self.assertRaises(AssertionError):
-        self.vm2.storage.clone_volume = coroutine_mock
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private123', token.encode())
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
+        self.assertFalse(self.app.save.called)
+
+    def test_523_vm_volume_clone_removed_volume(self):
+        self.setup_for_clone()
+
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                b'test-vm1', b'private', b'')
+        def get_volume(vid):
+            if vid == self.vm.volumes['private']:
+                raise KeyError(vid)
+            else:
+                return unittest.mock.DEFAULT
+        self.pool.get_volume.side_effect = get_volume
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private', token.encode())
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
+        self.assertFalse(self.app.save.called)
+
+    def test_524_vm_volume_clone_invlid_token(self):
+        self.setup_for_clone()
 
         with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'admin.vm.volume.Clone',
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
-                    b'test-vm1', b'private123', b'no-such-vm')
+                    b'test-vm1', b'private', b'no-such-token')
-        self.assertEqual(self.vm.volumes.mock_calls,
+        self.assertNotIn('init_volume().import_volume',
-            [('keys', (), {})])
+            map(operator.itemgetter(0), self.pool.mock_calls))
-        self.assertEqual(self.vm2.volumes.mock_calls, [])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [])
         self.assertFalse(self.app.save.called)
 
     def test_530_tag_list(self):