diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index 853f112187c..a7a5c705f15 100644 --- a/cinder/backup/manager.py +++ b/cinder/backup/manager.py @@ -50,6 +50,7 @@ from cinder.backup import rpcapi as backup_rpcapi from cinder import context from cinder import exception from cinder.i18n import _ +from cinder.keymgr import migration as key_migration from cinder import manager from cinder import objects from cinder.objects import fields @@ -164,6 +165,12 @@ class BackupManager(manager.ThreadPoolManager): # Don't block startup of the backup service. LOG.exception("Problem cleaning incomplete backup operations.") + # Migrate any ConfKeyManager keys based on fixed_key to the currently + # configured key manager. + backups = objects.BackupList.get_all_by_host(ctxt, self.host) + self._add_to_threadpool(key_migration.migrate_fixed_key, + backups=backups) + def _setup_backup_driver(self, ctxt): backup_service = self.get_backup_driver(ctxt) backup_service.check_for_setup_error() diff --git a/cinder/keymgr/migration.py b/cinder/keymgr/migration.py index fddce081a5c..e0290dc87bc 100644 --- a/cinder/keymgr/migration.py +++ b/cinder/keymgr/migration.py @@ -14,6 +14,7 @@ # under the License. import binascii +import itertools from oslo_config import cfg from oslo_log import log as logging @@ -42,7 +43,7 @@ class KeyMigrator(object): self.fixed_key_bytes = None self.fixed_key_length = None - def handle_key_migration(self, volumes): + def handle_key_migration(self, volumes, backups): castellan_options.set_defaults(self.conf) try: self.conf.import_opt(name='fixed_key', @@ -70,17 +71,17 @@ class KeyMigrator(object): "the '%s' key_manager backend is not supported.", backend) self._log_migration_status() - elif not volumes: + elif not volumes and not backups: LOG.info("Not migrating encryption keys because there are no " - "volumes associated with this host.") + "volumes or backups associated with this host.") self._log_migration_status() else: self.fixed_key_bytes = bytes(binascii.unhexlify(fixed_key)) self.fixed_key_length = len(self.fixed_key_bytes) * 8 - self._migrate_keys(volumes) + self._migrate_keys(volumes, backups) self._log_migration_status() - def _migrate_keys(self, volumes): + def _migrate_keys(self, volumes, backups): LOG.info("Starting migration of ConfKeyManager keys.") # Establish a Barbican client session that will be used for the entire @@ -98,9 +99,9 @@ class KeyMigrator(object): return errors = 0 - for volume in volumes: + for item in itertools.chain(volumes, backups): try: - self._migrate_volume_key(volume) + self._migrate_encryption_key(item) except Exception as e: LOG.error("Error migrating encryption key: %s", e) # NOTE(abishop): There really shouldn't be any soft errors, so @@ -113,14 +114,12 @@ class KeyMigrator(object): "(too many errors).") break - @coordination.synchronized('{volume.id}-{f_name}') - def _migrate_volume_key(self, volume): - if volume.encryption_key_id == self.fixed_key_id: - self._update_encryption_key_id(volume) - - def _update_encryption_key_id(self, volume): - LOG.info("Migrating volume %s encryption key to Barbican", volume.id) + @coordination.synchronized('{item.id}-{f_name}') + def _migrate_encryption_key(self, item): + if item.encryption_key_id == self.fixed_key_id: + self._update_encryption_key_id(item) + def _get_barbican_key_id(self, user_id): # Create a Barbican secret using the same fixed_key algorithm. secret = self.barbican.secrets.create(algorithm='AES', bit_length=self.fixed_key_length, @@ -129,34 +128,61 @@ class KeyMigrator(object): payload=self.fixed_key_bytes) secret_ref = secret.store() - # Create a Barbican ACL so the volume's user can access the secret. + # Create a Barbican ACL so the user can access the secret. acl = self.barbican.acls.create(entity_ref=secret_ref, - users=[volume.user_id]) + users=[user_id]) acl.submit() _, _, encryption_key_id = secret_ref.rpartition('/') - volume.encryption_key_id = encryption_key_id - volume.save() + return encryption_key_id - snapshots = objects.snapshot.SnapshotList.get_all_for_volume( - self.admin_context, - volume.id) - for snapshot in snapshots: - snapshot.encryption_key_id = encryption_key_id - snapshot.save() + def _update_encryption_key_id(self, item): + LOG.info("Migrating %(item_type)s %(item_id)s encryption key " + "to Barbican", + {'item_type': type(item).__name__, 'item_id': item.id}) + + encryption_key_id = self._get_barbican_key_id(item.user_id) + item.encryption_key_id = encryption_key_id + item.save() + + if isinstance(item, objects.volume.Volume): + snapshots = objects.snapshot.SnapshotList.get_all_for_volume( + self.admin_context, + item.id) + for snapshot in snapshots: + snapshot.encryption_key_id = encryption_key_id + snapshot.save() def _log_migration_status(self): - num_to_migrate = len(objects.volume.VolumeList.get_all( + volumes_to_migrate = len(objects.volume.VolumeList.get_all( context=self.admin_context, filters={'encryption_key_id': self.fixed_key_id})) - if num_to_migrate == 0: + if volumes_to_migrate == 0: LOG.info("No volumes are using the ConfKeyManager's " "encryption_key_id.") else: LOG.warning("There are still %d volume(s) using the " "ConfKeyManager's all-zeros encryption key ID.", - num_to_migrate) + volumes_to_migrate) + + backups_to_migrate = len(objects.backup.BackupList.get_all( + context=self.admin_context, + filters={'encryption_key_id': self.fixed_key_id})) + if backups_to_migrate == 0: + # Old backups may exist that were created prior to when the + # encryption_key_id is stored in the backup table. It's not + # easy to tell whether the backup was of an encrypted volume, + # in which case an all-zeros encryption key ID might be present + # in the backup's metadata. + LOG.info("No backups are known to be using the ConfKeyManager's " + "encryption_key_id.") + else: + LOG.warning("There are still %d backups(s) using the " + "ConfKeyManager's all-zeros encryption key ID.", + backups_to_migrate) -def migrate_fixed_key(volumes, conf=CONF): - KeyMigrator(conf).handle_key_migration(volumes) +def migrate_fixed_key(volumes=None, backups=None, conf=CONF): + volumes = volumes or [] + backups = backups or [] + KeyMigrator(conf).handle_key_migration(volumes, backups) diff --git a/cinder/objects/backup.py b/cinder/objects/backup.py index add4b8c49ee..e1fa7b48b40 100644 --- a/cinder/objects/backup.py +++ b/cinder/objects/backup.py @@ -31,7 +31,7 @@ CONF = cfg.CONF @base.CinderObjectRegistry.register class Backup(base.CinderPersistentObject, base.CinderObject, - base.CinderObjectDictCompat): + base.CinderObjectDictCompat, base.CinderComparableObject): # Version 1.0: Initial version # Version 1.1: Add new field num_dependent_backups and extra fields # is_incremental and has_dependent_backups. diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index 36cfc7cc650..5a1ea311288 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -313,7 +313,21 @@ class BackupTestCase(BaseBackupTest): calls = [mock.call(self.backup_mgr.delete_backup, mock.ANY, backup1), mock.call(self.backup_mgr.delete_backup, mock.ANY, backup2)] mock_add_threadpool.assert_has_calls(calls, any_order=True) - self.assertEqual(2, mock_add_threadpool.call_count) + # 3 calls because 1 is always made to handle encryption key migration. + self.assertEqual(3, mock_add_threadpool.call_count) + + @mock.patch('cinder.keymgr.migration.migrate_fixed_key') + @mock.patch('cinder.objects.BackupList.get_all_by_host') + @mock.patch('cinder.manager.ThreadPoolManager._add_to_threadpool') + def test_init_host_key_migration(self, + mock_add_threadpool, + mock_get_all_by_host, + mock_migrate_fixed_key): + + self.backup_mgr.init_host() + mock_add_threadpool.assert_called_once_with( + mock_migrate_fixed_key, + backups=mock_get_all_by_host()) @mock.patch('cinder.objects.service.Service.get_minimum_rpc_version') @mock.patch('cinder.objects.service.Service.get_minimum_obj_version') diff --git a/cinder/tests/unit/keymgr/test_migration.py b/cinder/tests/unit/keymgr/test_migration.py index 28e52a983e4..d668161e4a8 100644 --- a/cinder/tests/unit/keymgr/test_migration.py +++ b/cinder/tests/unit/keymgr/test_migration.py @@ -34,9 +34,12 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): super(KeyMigrationTestCase, self).setUp() self.conf = CONF self.fixed_key = '1' * 64 - self.conf.import_opt(name='fixed_key', - module_str='cinder.keymgr.conf_key_mgr', - group='key_manager') + try: + self.conf.import_opt(name='fixed_key', + module_str='cinder.keymgr.conf_key_mgr', + group='key_manager') + except cfg.DuplicateOptError: + pass self.conf.set_override('fixed_key', self.fixed_key, group='key_manager') @@ -44,24 +47,37 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): 'barbican', group='key_manager') self.my_vols = [] + self.my_baks = [] def tearDown(self): for vol in objects.VolumeList.get_all(self.context): self.volume.delete_volume(self.context, vol) + for bak in objects.BackupList.get_all(self.context): + bak.destroy() super(KeyMigrationTestCase, self).tearDown() def create_volume(self, key_id=FIXED_KEY_ID): vol = tests_utils.create_volume(self.context, host=self.conf.host) - vol_id = self.volume.create_volume(self.context, vol) + self.volume.create_volume(self.context, vol) if key_id: - db.volume_update(self.context, - vol_id, - {'encryption_key_id': key_id}) + vol.encryption_key_id = key_id + vol.save() self.my_vols = objects.VolumeList.get_all_by_host(self.context, self.conf.host) - # Return a fully baked Volume object (not the partially baked 'vol' - # and not the DB object). - return next((v for v in self.my_vols if v.id == vol_id)) + vol.refresh() + return vol + + def create_backup(self, volume_id=fake.VOLUME_ID, key_id=FIXED_KEY_ID): + bak = tests_utils.create_backup(self.context, + volume_id=volume_id, + host=self.conf.host) + if key_id: + bak.encryption_key_id = key_id + bak.save() + self.my_baks = objects.BackupList.get_all_by_host(self.context, + self.conf.host) + bak.refresh() + return bak @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_keys') @mock.patch('cinder.keymgr.migration.KeyMigrator._log_migration_status') @@ -70,7 +86,7 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): mock_migrate_keys): self.create_volume() self.conf.set_override('fixed_key', None, group='key_manager') - migration.migrate_fixed_key(self.my_vols, conf=self.conf) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) mock_migrate_keys.assert_not_called() mock_log_migration_status.assert_not_called() @@ -83,7 +99,7 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): self.conf.set_override('backend', 'some.ConfKeyManager', group='key_manager') - migration.migrate_fixed_key(self.my_vols, conf=self.conf) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) mock_migrate_keys.assert_not_called() mock_log_migration_status.assert_not_called() @@ -99,8 +115,8 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): 'backend', 'castellan.key_manager.barbican_key_manager.BarbicanKeyManager', group='key_manager') - migration.migrate_fixed_key(self.my_vols, conf=self.conf) - mock_migrate_keys.assert_called_once_with(self.my_vols) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) + mock_migrate_keys.assert_called_once_with(self.my_vols, self.my_baks) mock_log_migration_status.assert_called_once_with() @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_keys') @@ -112,7 +128,7 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): self.conf.set_override('backend', 'some.OtherKeyManager', group='key_manager') - migration.migrate_fixed_key(self.my_vols, conf=self.conf) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) mock_migrate_keys.assert_not_called() mock_log_migration_status.assert_called_once_with() @@ -121,30 +137,30 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): def test_no_volumes(self, mock_log_migration_status, mock_migrate_keys): - migration.migrate_fixed_key(self.my_vols, conf=self.conf) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) mock_migrate_keys.assert_not_called() mock_log_migration_status.assert_called_once_with() - @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_volume_key') + @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_encryption_key') @mock.patch('barbicanclient.client.Client') def test_fail_no_barbican_client(self, mock_barbican_client, - mock_migrate_volume_key): + mock_migrate_encryption_key): self.create_volume() mock_barbican_client.side_effect = Exception - migration.migrate_fixed_key(self.my_vols, conf=self.conf) - mock_migrate_volume_key.assert_not_called() + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) + mock_migrate_encryption_key.assert_not_called() - @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_volume_key') + @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_encryption_key') @mock.patch('barbicanclient.client.Client') def test_fail_too_many_errors(self, mock_barbican_client, - mock_migrate_volume_key): + mock_migrate_encryption_key): for n in range(0, (migration.MAX_KEY_MIGRATION_ERRORS + 3)): self.create_volume() - mock_migrate_volume_key.side_effect = Exception - migration.migrate_fixed_key(self.my_vols, conf=self.conf) - self.assertEqual(mock_migrate_volume_key.call_count, + mock_migrate_encryption_key.side_effect = Exception + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) + self.assertEqual(mock_migrate_encryption_key.call_count, (migration.MAX_KEY_MIGRATION_ERRORS + 1)) @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_keys') @@ -152,29 +168,31 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): mock_migrate_keys): mock_log = self.mock_object(migration, 'LOG') self.create_volume() - migration.migrate_fixed_key(self.my_vols, conf=self.conf) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) - # Look for one warning (more to migrate) and no info log messages. - mock_log.info.assert_not_called() + # Look for one warning (more volumes to migrate) and one info (no + # backups to migrate) log messages. self.assertEqual(mock_log.warning.call_count, 1) + self.assertEqual(mock_log.info.call_count, 1) @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_keys') def test_migration_status_all_done(self, mock_migrate_keys): mock_log = self.mock_object(migration, 'LOG') self.create_volume(key_id=fake.ENCRYPTION_KEY_ID) - migration.migrate_fixed_key(self.my_vols, conf=self.conf) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) - # Look for one info (all done) and no warning log messages. + # Look for two info (no volumes to migrate, no backups to migrate) + # and no warning log messages. mock_log.warning.assert_not_called() - self.assertEqual(mock_log.info.call_count, 1) + self.assertEqual(mock_log.info.call_count, 2) @mock.patch( 'cinder.keymgr.migration.KeyMigrator._update_encryption_key_id') @mock.patch('barbicanclient.client.Client') - def test_migrate_fixed_key_volumes(self, - mock_barbican_client, - mock_update_encryption_key_id): + def test_fixed_key_migration(self, + mock_barbican_client, + mock_update_encryption_key_id): # Create two volumes with fixed key ID that needs to be migrated, and # a couple of volumes with key IDs that don't need to be migrated, # or no key ID. @@ -184,20 +202,25 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): vol_2 = self.create_volume() self.create_volume(key_id=fake.UUID2) - migration.migrate_fixed_key(self.my_vols, conf=self.conf) - calls = [mock.call(vol_1), mock.call(vol_2)] + # Create a few backups + self.create_backup(key_id=None) + self.create_backup(key_id=fake.UUID3) + bak_1 = self.create_backup() + self.create_backup(key_id=fake.UUID4) + bak_2 = self.create_backup() + + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) + + calls = [mock.call(vol_1), mock.call(vol_2), + mock.call(bak_1), mock.call(bak_2)] mock_update_encryption_key_id.assert_has_calls(calls, any_order=True) self.assertEqual(mock_update_encryption_key_id.call_count, len(calls)) @mock.patch('barbicanclient.client.Client') - def test_update_encryption_key_id(self, - mock_barbican_client): + def test_get_barbican_key_id(self, + mock_barbican_client): vol = self.create_volume() - snap_ids = [fake.SNAPSHOT_ID, fake.SNAPSHOT2_ID, fake.SNAPSHOT3_ID] - for snap_id in snap_ids: - tests_utils.create_snapshot(self.context, vol.id, id=snap_id) - # Barbican's secret.store() returns a URI that contains the # secret's key ID at the end. secret_ref = 'http://some/path/' + fake.ENCRYPTION_KEY_ID @@ -207,7 +230,29 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): mock_barbican_client.return_value.secrets.create.return_value \ = mock_secret - migration.migrate_fixed_key(self.my_vols, conf=self.conf) + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) + + mock_acls_create = mock_barbican_client.return_value.acls.create + mock_acls_create.assert_called_once_with(entity_ref=secret_ref, + users=[fake.USER_ID]) + mock_acls_create.return_value.submit.assert_called_once_with() + + vol_db = db.volume_get(self.context, vol.id) + self.assertEqual(fake.ENCRYPTION_KEY_ID, vol_db['encryption_key_id']) + + @mock.patch('cinder.keymgr.migration.KeyMigrator._get_barbican_key_id') + @mock.patch('barbicanclient.client.Client') + def test_update_volume_encryption_key_id(self, + mock_barbican_client, + mock_get_barbican_key_id): + vol = self.create_volume() + + snap_ids = [fake.SNAPSHOT_ID, fake.SNAPSHOT2_ID, fake.SNAPSHOT3_ID] + for snap_id in snap_ids: + tests_utils.create_snapshot(self.context, vol.id, id=snap_id) + + mock_get_barbican_key_id.return_value = fake.ENCRYPTION_KEY_ID + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) vol_db = db.volume_get(self.context, vol.id) self.assertEqual(fake.ENCRYPTION_KEY_ID, vol_db['encryption_key_id']) @@ -215,3 +260,14 @@ class KeyMigrationTestCase(base.BaseVolumeTestCase): snap_db = db.snapshot_get(self.context, snap_id) self.assertEqual(fake.ENCRYPTION_KEY_ID, snap_db['encryption_key_id']) + + @mock.patch('cinder.keymgr.migration.KeyMigrator._get_barbican_key_id') + @mock.patch('barbicanclient.client.Client') + def test_update_backup_encryption_key_id(self, + mock_barbican_client, + mock_get_barbican_key_id): + bak = self.create_backup() + mock_get_barbican_key_id.return_value = fake.ENCRYPTION_KEY_ID + migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf) + bak_db = db.backup_get(self.context, bak.id) + self.assertEqual(fake.ENCRYPTION_KEY_ID, bak_db['encryption_key_id']) diff --git a/cinder/tests/unit/volume/test_init_host.py b/cinder/tests/unit/volume/test_init_host.py index d6c9c242040..d0b806573b3 100644 --- a/cinder/tests/unit/volume/test_init_host.py +++ b/cinder/tests/unit/volume/test_init_host.py @@ -278,5 +278,6 @@ class VolumeInitHostTestCase(base.BaseVolumeTestCase): mock_migrate_fixed_key): self.volume.init_host(service_id=self.service_id) - mock_add_threadpool.assert_called_once_with(mock_migrate_fixed_key, - mock_get_my_volumes()) + mock_add_threadpool.assert_called_once_with( + mock_migrate_fixed_key, + volumes=mock_get_my_volumes()) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index c808351af32..2d186cae45b 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -510,7 +510,8 @@ class VolumeManager(manager.CleanableManager, # Migrate any ConfKeyManager keys based on fixed_key to the currently # configured key manager. - self._add_to_threadpool(key_migration.migrate_fixed_key, volumes) + self._add_to_threadpool(key_migration.migrate_fixed_key, + volumes=volumes) # collect and publish service capabilities self.publish_service_capabilities(ctxt) diff --git a/releasenotes/notes/migrate-backup-encryption-keys-to-barbican-6f07fd48d4937b2a.yaml b/releasenotes/notes/migrate-backup-encryption-keys-to-barbican-6f07fd48d4937b2a.yaml new file mode 100644 index 00000000000..4c66e4164c1 --- /dev/null +++ b/releasenotes/notes/migrate-backup-encryption-keys-to-barbican-6f07fd48d4937b2a.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + When encryption keys based on the ConfKeyManager's fixed_key are migrated + to Barbican, ConfKeyManager keys stored in the Backup table are included + in the migration process. + Fixes `bug 1757235 `__.