From 341dd44ba796e933920da6718a2891e35ed88506 Mon Sep 17 00:00:00 2001 From: Alan Bishop Date: Tue, 20 Mar 2018 15:10:28 -0400 Subject: [PATCH] Handle migrating encryption key IDs in Backup table Enhance the code that migrates the ConfKeyManager's fixed_key to Barbican to also consider the Backup table. When the original key migration feature was added, the encryption key ID was not stored in the Backup table. But now the Backup table contains that field, so the migration code needs to handle that table as well. Whereas the cinder-volume service is responsible for migrating keys in the Volume and Snapshot tables, the cinder-backup service handles migrating keys in the Backup table. Each instance of the service migrates its own entries by matching the "host" field in the corresponding tables. The Backup OVO now inherits from base.CinderComparableObject. This does not affect the object's hash signature, and so the version number does need to be incremented. Closes-Bug: #1757235 Change-Id: Id4581eec80f82925c20c424847bff1baceda2349 --- cinder/backup/manager.py | 7 + cinder/keymgr/migration.py | 84 +++++++---- cinder/objects/backup.py | 2 +- cinder/tests/unit/backup/test_backup.py | 16 +- cinder/tests/unit/keymgr/test_migration.py | 142 ++++++++++++------ cinder/tests/unit/volume/test_init_host.py | 5 +- cinder/volume/manager.py | 3 +- ...ion-keys-to-barbican-6f07fd48d4937b2a.yaml | 7 + 8 files changed, 189 insertions(+), 77 deletions(-) create mode 100644 releasenotes/notes/migrate-backup-encryption-keys-to-barbican-6f07fd48d4937b2a.yaml diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index 84bc3953983..29f80adce50 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 @@ -153,6 +154,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 ede7cd5e8ad..8ca0f1ac862 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 6b2686e7007..abb1d005d19 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -486,7 +486,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 `__.