Merge "Handle migrating encryption key IDs in Backup table"

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()

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}')
+    @coordination.synchronized('{item.id}-{f_name}')
-    def _migrate_volume_key(self, volume):
+    def _migrate_encryption_key(self, item):
-        if volume.encryption_key_id == self.fixed_key_id:
+        if item.encryption_key_id == self.fixed_key_id:
-            self._update_encryption_key_id(volume)
+            self._update_encryption_key_id(item)
-
-    def _update_encryption_key_id(self, volume):
-        LOG.info("Migrating volume %s encryption key to Barbican", volume.id)
 
+    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
+        return encryption_key_id
-        volume.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,
-            volume.id)
+                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):
+def migrate_fixed_key(volumes=None, backups=None, conf=CONF):
-    KeyMigrator(conf).handle_key_migration(volumes)
+    volumes = volumes or []
+    backups = backups or []
+    KeyMigrator(conf).handle_key_migration(volumes, backups)

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.

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')

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
+        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.encryption_key_id = key_id
-                             vol_id,
+            vol.save()
-                             {'encryption_key_id': key_id})
         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'
+        vol.refresh()
-        # and not the DB object).
+        return vol
-        return next((v for v in self.my_vols if v.id == vol_id))
+
+    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)
+        migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf)
-        mock_migrate_keys.assert_called_once_with(self.my_vols)
+        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)
+        migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf)
-        mock_migrate_volume_key.assert_not_called()
+        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
+        mock_migrate_encryption_key.side_effect = Exception
-        migration.migrate_fixed_key(self.my_vols, conf=self.conf)
+        migration.migrate_fixed_key(self.my_vols, self.my_baks, conf=self.conf)
-        self.assertEqual(mock_migrate_volume_key.call_count,
+        self.assertEqual(mock_migrate_encryption_key.call_count,
                          (migration.MAX_KEY_MIGRATION_ERRORS + 1))
 
     @mock.patch('cinder.keymgr.migration.KeyMigrator._migrate_keys')
@@ -152,27 +168,29 @@ 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.
+        # Look for one warning (more volumes to migrate) and one info (no
-        mock_log.info.assert_not_called()
+        # 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,
+    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
@@ -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)
+        # Create a few backups
-        calls = [mock.call(vol_1), mock.call(vol_2)]
+        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,
+    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'])

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_add_threadpool.assert_called_once_with(
-                                                    mock_get_my_volumes())
+            mock_migrate_fixed_key,
+            volumes=mock_get_my_volumes())

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)

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 <https://bugs.launchpad.net/tripleo/+bug/1757235>`__.