From be02e0f3467b8cac20eaf73c52164f6bed88f7de Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Fri, 26 Jun 2020 14:42:09 +0200 Subject: [PATCH] RBD: cinderlib support for rbd_keyring_conf option In the last cycle we deprecated the RBD configuration option as per OSSN-0085, and it was removed for victoria by Change I3cc58b2d74d82ab6b2186e9ea7cfafeb4c3de822 This patch modifies the RBD driver to support cinderlib use cases that are not affected by the security vulnerability. Even if we have the configuration option in cinder.conf it will not be seen by the Cinder RBD driver, it will only see it if we skip the Oslo Config mechanism and set it directly on the instance as an attribute, like cinderlib does. Related-Bug: #1849624 Implements: blueprint rbd-remove-option-causing-ossn-0085 Change-Id: Iae63aee973932b2eef26d832a7f413a04bad0df1 --- cinder/tests/unit/volume/drivers/test_rbd.py | 78 ++++++++++++++++++++ cinder/volume/drivers/rbd.py | 34 +++++++++ 2 files changed, 112 insertions(+) diff --git a/cinder/tests/unit/volume/drivers/test_rbd.py b/cinder/tests/unit/volume/drivers/test_rbd.py index 6e8e65fe30d..b8e5cd06cc7 100644 --- a/cinder/tests/unit/volume/drivers/test_rbd.py +++ b/cinder/tests/unit/volume/drivers/test_rbd.py @@ -258,6 +258,16 @@ class RBDTestCase(test.TestCase): self.assertRaises(exception.InvalidConfigurationValue, self.driver.check_for_setup_error) + @mock.patch.object(driver, 'rados', mock.Mock()) + @mock.patch.object(driver, 'RADOSClient') + def test_check_for_setup_error_missing_keyring_data(self, mock_client): + self.driver.keyring_file = '/etc/ceph/ceph.client.admin.keyring' + self.driver.keyring_data = None + + self.assertRaises(exception.InvalidConfigurationValue, + self.driver.check_for_setup_error) + mock_client.assert_called_once_with(self.driver) + def test_parse_replication_config_empty(self): self.driver._parse_replication_configs([]) self.assertEqual([], self.driver._replication_targets) @@ -1593,6 +1603,12 @@ class RBDTestCase(test.TestCase): } self._initialize_connection_helper(expected, hosts, ports) + # Check how it will work with keyring data (for cinderlib) + keyring_data = "[client.cinder]\n key = test\n" + self.driver.keyring_data = keyring_data + expected['data']['keyring'] = keyring_data + self._initialize_connection_helper(expected, hosts, ports) + self.driver._active_config = {'name': 'secondary_id', 'user': 'foo', 'conf': 'bar', @@ -1600,6 +1616,68 @@ class RBDTestCase(test.TestCase): expected['data']['secret_uuid'] = 'secondary_secret_uuid' self._initialize_connection_helper(expected, hosts, ports) + def test__set_keyring_attributes_openstack(self): + # OpenStack usage doesn't have the rbd_keyring_conf Oslo Config option + self.assertFalse(hasattr(self.driver.configuration, + 'rbd_keyring_conf')) + # Set initial values so we can confirm that we set them to None + self.driver.keyring_file = mock.sentinel.keyring_file + self.driver.keyring_data = mock.sentinel.keyring_data + + self.driver._set_keyring_attributes() + + self.assertIsNone(self.driver.keyring_file) + self.assertIsNone(self.driver.keyring_data) + + def test__set_keyring_attributes_cinderlib(self): + # OpenStack usage doesn't have the rbd_keyring_conf Oslo Config option + cfg_file = '/etc/ceph/ceph.client.admin.keyring' + self.driver.configuration.rbd_keyring_conf = cfg_file + self.driver._set_keyring_attributes() + self.assertEqual(cfg_file, self.driver.keyring_file) + self.assertIsNone(self.driver.keyring_data) + + @mock.patch('os.path.isfile') + @mock.patch.object(driver, 'open') + def test__set_keyring_attributes_cinderlib_read_file(self, mock_open, + mock_isfile): + cfg_file = '/etc/ceph/ceph.client.admin.keyring' + # This is how cinderlib sets the config option + setattr(self.driver.configuration, 'rbd_keyring_conf', cfg_file) + + keyring_data = "[client.cinder]\n key = test\n" + mock_read = mock_open.return_value.__enter__.return_value.read + mock_read.return_value = keyring_data + + self.assertIsNone(self.driver.keyring_file) + self.assertIsNone(self.driver.keyring_data) + + self.driver._set_keyring_attributes() + + mock_isfile.assert_called_once_with(cfg_file) + mock_open.assert_called_once_with(cfg_file, 'r') + mock_read.assert_called_once_with() + self.assertEqual(cfg_file, self.driver.keyring_file) + self.assertEqual(keyring_data, self.driver.keyring_data) + + @mock.patch('os.path.isfile') + @mock.patch.object(driver, 'open', side_effect=IOError) + def test__set_keyring_attributes_cinderlib_error(self, mock_open, + mock_isfile): + cfg_file = '/etc/ceph/ceph.client.admin.keyring' + # This is how cinderlib sets the config option + setattr(self.driver.configuration, 'rbd_keyring_conf', cfg_file) + + self.assertIsNone(self.driver.keyring_file) + self.driver.keyring_data = mock.sentinel.keyring_data + + self.driver._set_keyring_attributes() + + mock_isfile.assert_called_once_with(cfg_file) + mock_open.assert_called_once_with(cfg_file, 'r') + self.assertEqual(cfg_file, self.driver.keyring_file) + self.assertIsNone(self.driver.keyring_data) + @ddt.data({'rbd_chunk_size': 1, 'order': 20}, {'rbd_chunk_size': 8, 'order': 23}, {'rbd_chunk_size': 32, 'order': 25}) diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index e5c3a89522f..6496bf8c588 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -261,6 +261,30 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, self.RBD_FEATURE_OBJECT_MAP | self.RBD_FEATURE_EXCLUSIVE_LOCK) + self._set_keyring_attributes() + + def _set_keyring_attributes(self): + # The rbd_keyring_conf option is not available for OpenStack usage + # for security reasons (OSSN-0085) and in OpenStack we use + # rbd_secret_uuid or make sure that the keyring files are present on + # the hosts (where os-brick will look for them). + # For cinderlib usage this option is necessary (no security issue, as + # in those cases the contents of the connection are not available to + # users). By using getattr Oslo-conf won't read the option from the + # file even if it's there (because we have removed the conf option + # definition), but cinderlib will find it because it sets the option + # directly as an attribute. + self.keyring_file = getattr(self.configuration, 'rbd_keyring_conf', + None) + + self.keyring_data = None + try: + if self.keyring_file and os.path.isfile(self.keyring_file): + with open(self.keyring_file, 'r') as k_file: + self.keyring_data = k_file.read() + except IOError: + LOG.debug('Cannot read RBD keyring file: %s.', self.keyring_file) + @classmethod def get_driver_options(cls): additional_opts = cls._get_oslo_driver_opts( @@ -388,6 +412,14 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, LOG.error(msg) raise exception.VolumeBackendAPIException(data=msg) + # If the keyring is defined (cinderlib usage), then the contents are + # necessary. + if self.keyring_file and not self.keyring_data: + msg = _('No keyring data found') + LOG.error(msg) + raise exception.InvalidConfigurationValue( + option='rbd_keyring_conf', value=self.keyring_file) + self._start_periodic_tasks() def RBDProxy(self): @@ -1423,6 +1455,8 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD, "discard": True, } } + if self.keyring_data: + data['data']['keyring'] = self.keyring_data LOG.debug('connection data: %s', data) return data