openstack/cinder
Code Issues Proposed changes

Handle migrating encryption key IDs in Backup table

Browse Source 
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
This commit is contained in:
Alan Bishop 2018-03-20 15:10:28 -04:00
parent c61daa766b
commit 341dd44ba7
8 changed files with 189 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
cinder/backup/manager.py
View File

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

74
cinder/keymgr/migration.py
View File

@ -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
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):
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)

2
cinder/objects/backup.py
View File

@ -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.

16
cinder/tests/unit/backup/test_backup.py
View File

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

130
cinder/tests/unit/keymgr/test_migration.py
View File

@ -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_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,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.
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,
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)
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,
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'])

5
cinder/tests/unit/volume/test_init_host.py
View File

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

3
cinder/volume/manager.py
View File

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

7
releasenotes/notes/migrate-backup-encryption-keys-to-barbican-6f07fd48d4937b2a.yaml Normal file
View File

@ -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>`__.