diff --git a/nova/compute/manager.py b/nova/compute/manager.py index a1a203339dfa..540d1d5c1db6 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -5110,8 +5110,8 @@ class ComputeManager(manager.Manager): "old: %(old_cinfo)s", {'new_cinfo': new_cinfo, 'old_cinfo': old_cinfo}, instance=instance) - self.driver.swap_volume(old_cinfo, new_cinfo, instance, mountpoint, - resize_to) + self.driver.swap_volume(context, old_cinfo, new_cinfo, instance, + mountpoint, resize_to) LOG.debug("swap_volume: Driver volume swap returned, new " "connection_info is now : %(new_cinfo)s", {'new_cinfo': new_cinfo}) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 5bce8a02b491..689c46f3f21c 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -1955,8 +1955,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): self.assertTrue(uuidutils.is_uuid_like(volume)) return {} - def _assert_swap_volume(self, old_connection_info, new_connection_info, - instance, mountpoint, resize_to): + def _assert_swap_volume(self, context, old_connection_info, + new_connection_info, instance, mountpoint, + resize_to): self.assertEqual(2, resize_to) @mock.patch.object(cinder.API, 'initialize_connection') @@ -2273,7 +2274,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): instance, uuids.new_attachment_id) # Assert the expected calls. # The new connection_info has the new_volume_id as the serial. - new_cinfo = mock_driver_swap.call_args[0][1] + new_cinfo = mock_driver_swap.call_args[0][2] self.assertIn('serial', new_cinfo) self.assertEqual(uuids.new_volume_id, new_cinfo['serial']) get_bdm.assert_called_once_with( diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index e4ae0a0c2161..67fb9c03e915 100755 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -15275,6 +15275,26 @@ class LibvirtConnTestCase(test.NoDBTestCase, def test_cleanup_encryption_volume_detach_failed(self): self._test_cleanup_encryption_process_execution_error(not_found=False) + @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption') + def test_swap_volume_native_luks_blocked(self, mock_get_encryption): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) + + # dest volume is encrypted + mock_get_encryption.side_effect = [{}, {'provider': 'luks'}] + self.assertRaises(NotImplementedError, drvr.swap_volume, self.context, + {}, {}, None, None, None) + + # src volume is encrypted + mock_get_encryption.side_effect = [{'provider': 'luks'}, {}] + self.assertRaises(NotImplementedError, drvr.swap_volume, self.context, + {}, {}, None, None, None) + + # both volumes are encrypted + mock_get_encryption.side_effect = [{'provider': 'luks'}, + {'provider': 'luks'}] + self.assertRaises(NotImplementedError, drvr.swap_volume, self.context, + {}, {}, None, None, None) + @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete', return_value=True) def _test_swap_volume(self, mock_is_job_complete, source_type, @@ -15404,8 +15424,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, conf = mock.MagicMock(source_path='/fake-new-volume') get_volume_config.return_value = conf - conn.swap_volume(old_connection_info, new_connection_info, instance, - '/dev/vdb', 1) + conn.swap_volume(self.context, old_connection_info, + new_connection_info, instance, '/dev/vdb', 1) get_guest.assert_called_once_with(instance) connect_volume.assert_called_once_with(new_connection_info, disk_info, @@ -15424,6 +15444,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, def test_swap_volume_driver_source_is_snapshot(self): self._test_swap_volume_driver(source_type='snapshot') + @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption') @mock.patch('nova.virt.libvirt.guest.BlockDevice.rebase') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._connect_volume') @@ -15433,7 +15454,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch('nova.virt.libvirt.host.Host.write_instance_config') def test_swap_volume_disconnect_new_volume_on_rebase_error(self, write_config, get_guest, get_disk, get_volume_config, - connect_volume, disconnect_volume, rebase): + connect_volume, disconnect_volume, rebase, + get_volume_encryption): """Assert that disconnect_volume is called for the new volume if an error is encountered while rebasing """ @@ -15441,12 +15463,13 @@ class LibvirtConnTestCase(test.NoDBTestCase, instance = objects.Instance(**self.test_instance) guest = libvirt_guest.Guest(mock.MagicMock()) get_guest.return_value = guest + get_volume_encryption.return_value = {} exc = fakelibvirt.make_libvirtError(fakelibvirt.libvirtError, 'internal error', error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR) rebase.side_effect = exc self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume, - mock.sentinel.old_connection_info, + self.context, mock.sentinel.old_connection_info, mock.sentinel.new_connection_info, instance, '/dev/vdb', 0) connect_volume.assert_called_once_with( @@ -15455,6 +15478,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, disconnect_volume.assert_called_once_with( mock.sentinel.new_connection_info, 'vdb', instance) + @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption') @mock.patch('nova.virt.libvirt.guest.BlockDevice.is_job_complete') @mock.patch('nova.virt.libvirt.guest.BlockDevice.abort_job') @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') @@ -15465,7 +15489,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, @mock.patch('nova.virt.libvirt.host.Host.write_instance_config') def test_swap_volume_disconnect_new_volume_on_pivot_error(self, write_config, get_guest, get_disk, get_volume_config, - connect_volume, disconnect_volume, abort_job, is_job_complete): + connect_volume, disconnect_volume, abort_job, is_job_complete, + get_volume_encryption): """Assert that disconnect_volume is called for the new volume if an error is encountered while pivoting to the new volume """ @@ -15473,13 +15498,14 @@ class LibvirtConnTestCase(test.NoDBTestCase, instance = objects.Instance(**self.test_instance) guest = libvirt_guest.Guest(mock.MagicMock()) get_guest.return_value = guest + get_volume_encryption.return_value = {} exc = fakelibvirt.make_libvirtError(fakelibvirt.libvirtError, 'internal error', error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR) is_job_complete.return_value = True abort_job.side_effect = [None, exc] self.assertRaises(exception.VolumeRebaseFailed, conn.swap_volume, - mock.sentinel.old_connection_info, + self.context, mock.sentinel.old_connection_info, mock.sentinel.new_connection_info, instance, '/dev/vdb', 0) connect_volume.assert_called_once_with( diff --git a/nova/tests/unit/virt/test_block_device.py b/nova/tests/unit/virt/test_block_device.py index 609adf18605f..1e7f2ee7aa34 100644 --- a/nova/tests/unit/virt/test_block_device.py +++ b/nova/tests/unit/virt/test_block_device.py @@ -1125,3 +1125,28 @@ class TestDriverBlockDevice(test.NoDBTestCase): # can't assert_not_called if the method isn't in the spec. self.assertFalse(hasattr(test_eph, 'refresh_connection_info')) self.assertFalse(hasattr(test_swap, 'refresh_connection_info')) + + +class TestGetVolumeId(test.NoDBTestCase): + + def test_get_volume_id_none_found(self): + self.assertIsNone(driver_block_device.get_volume_id(None)) + self.assertIsNone(driver_block_device.get_volume_id({})) + self.assertIsNone(driver_block_device.get_volume_id({'data': {}})) + + def test_get_volume_id_found_volume_id_no_serial(self): + self.assertEqual(uuids.volume_id, + driver_block_device.get_volume_id( + {'data': {'volume_id': uuids.volume_id}})) + + def test_get_volume_id_found_no_volume_id_serial(self): + self.assertEqual(uuids.serial, + driver_block_device.get_volume_id( + {'serial': uuids.serial})) + + def test_get_volume_id_found_both(self): + # volume_id is taken over serial + self.assertEqual(uuids.volume_id, + driver_block_device.get_volume_id( + {'serial': uuids.serial, + 'data': {'volume_id': uuids.volume_id}})) diff --git a/nova/tests/unit/virt/test_virt_drivers.py b/nova/tests/unit/virt/test_virt_drivers.py index a996950dad18..0c32b1f2fd9e 100644 --- a/nova/tests/unit/virt/test_virt_drivers.py +++ b/nova/tests/unit/virt/test_virt_drivers.py @@ -492,7 +492,7 @@ class _VirtDriverTestCase(_FakeDriverBackendTestCase): instance_ref, '/dev/sda')) self.assertIsNone( - self.connection.swap_volume({'driver_volume_type': 'fake', + self.connection.swap_volume(None, {'driver_volume_type': 'fake', 'data': {}}, {'driver_volume_type': 'fake', 'data': {}}, diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py index aa1a3ee0499f..aecb342f0fea 100644 --- a/nova/virt/block_device.py +++ b/nova/virt/block_device.py @@ -687,3 +687,13 @@ def is_block_device_mapping(bdm): return (bdm.source_type in ('image', 'volume', 'snapshot', 'blank') and bdm.destination_type == 'volume' and is_implemented(bdm)) + + +def get_volume_id(connection_info): + if connection_info: + # Check for volume_id in 'data' and if not there, fallback to + # the 'serial' that the DriverVolumeBlockDevice adds during attach. + volume_id = connection_info.get('data', {}).get('volume_id') + if not volume_id: + volume_id = connection_info.get('serial') + return volume_id diff --git a/nova/virt/driver.py b/nova/virt/driver.py index d4c4cdf329fd..3e52c875a5a3 100644 --- a/nova/virt/driver.py +++ b/nova/virt/driver.py @@ -463,10 +463,11 @@ class ComputeDriver(object): """Detach the disk attached to the instance.""" raise NotImplementedError() - def swap_volume(self, old_connection_info, new_connection_info, + def swap_volume(self, context, old_connection_info, new_connection_info, instance, mountpoint, resize_to): """Replace the volume attached to the given `instance`. + :param context: The request context. :param dict old_connection_info: The volume for this connection gets detached from the given `instance`. diff --git a/nova/virt/fake.py b/nova/virt/fake.py index e08f32205031..25b22de8864f 100644 --- a/nova/virt/fake.py +++ b/nova/virt/fake.py @@ -308,7 +308,7 @@ class FakeDriver(driver.ComputeDriver): except KeyError: pass - def swap_volume(self, old_connection_info, new_connection_info, + def swap_volume(self, context, old_connection_info, new_connection_info, instance, mountpoint, resize_to): """Replace the disk attached to the instance.""" instance_name = instance.name diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index e9c956ee0a41..f353d51590b8 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1218,6 +1218,16 @@ class LibvirtDriver(driver.ComputeDriver): connection_info=connection_info, **encryption) + def _get_volume_encryption(self, context, connection_info): + """Get the encryption metadata dict if it is not provided + """ + encryption = {} + volume_id = driver_block_device.get_volume_id(connection_info) + if volume_id: + encryption = encryptors.get_encryption_metadata(context, + self._volume_api, volume_id, connection_info) + return encryption + def _check_discard_for_attach_volume(self, conf, instance): """Perform some checks for volumes configured for discard support. @@ -1353,9 +1363,19 @@ class LibvirtDriver(driver.ComputeDriver): finally: self._host.write_instance_config(xml) - def swap_volume(self, old_connection_info, + def swap_volume(self, context, old_connection_info, new_connection_info, instance, mountpoint, resize_to): + # NOTE(lyarwood): Bug #1739593 uncovered a nasty data corruption + # issue that was fixed in Queens by Ica323b87fa85a454fca9d46ada3677f18. + # Given the size of the bugfix it was agreed not to backport the change + # to earlier stable branches and to instead block swap volume attempts. + if (self._get_volume_encryption(context, old_connection_info) or + self._get_volume_encryption(context, new_connection_info)): + raise NotImplementedError(_("Swap volume is not supported when " + "using encrypted volumes. For more details see " + "https://bugs.launchpad.net/nova/+bug/1739593.")) + guest = self._host.get_guest(instance) disk_dev = mountpoint.rpartition("/")[2] diff --git a/releasenotes/notes/bug-1739593-cve-2017-18191-25fe48d336d8cf13.yaml b/releasenotes/notes/bug-1739593-cve-2017-18191-25fe48d336d8cf13.yaml new file mode 100644 index 000000000000..915b34053eed --- /dev/null +++ b/releasenotes/notes/bug-1739593-cve-2017-18191-25fe48d336d8cf13.yaml @@ -0,0 +1,9 @@ +--- +prelude: > + This release includes fixes for security vulnerabilities. +security: + - | + [CVE-2017-18191] Swapping encrypted volumes can lead to data loss and a + possible compute host DOS attack. + + * `Bug 1739593 `_