From fcaefdbe74c63d6ad42fd23cdb5cb98373d83443 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 25 Oct 2019 11:31:53 -0700 Subject: Hash the rescue_password In order to provide increased security, it is necessary to hash the rescue password in advance of it being stored into the database and to provide some sort of control for hash strength. This change IS incompatible with prior IPA versions with regard to use of the rescue feature, but I fully expect we will backport the change to IPA on to stable branches and perform a release as it is a security improvement. Change-Id: I1e118467a536229de6f7c245c1c48f0af38dcef2 Story: 2006777 Task: 27301 --- ironic/conductor/manager.py | 8 ++-- ironic/conductor/utils.py | 35 ++++++++++++++-- ironic/conf/conductor.py | 12 ++++++ ironic/drivers/modules/agent_client.py | 30 +++++++++++--- ironic/tests/unit/conductor/test_manager.py | 47 ++++++++++++++-------- ironic/tests/unit/conductor/test_utils.py | 7 +++- ironic/tests/unit/drivers/modules/test_agent.py | 3 +- .../unit/drivers/modules/test_agent_client.py | 34 +++++++++++++++- 8 files changed, 144 insertions(+), 32 deletions(-) (limited to 'ironic') diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index aca403313..bfc86e825 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -610,9 +610,11 @@ class ConductorManager(base_manager.BaseConductorManager): # driver validation may check rescue_password, so save it on the # node early - instance_info = node.instance_info - instance_info['rescue_password'] = rescue_password - node.instance_info = instance_info + i_info = node.instance_info + i_info['rescue_password'] = rescue_password + i_info['hashed_rescue_password'] = utils.hash_password( + rescue_password) + node.instance_info = i_info node.save() try: diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 2d97d655c..170218c61 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -13,6 +13,7 @@ # under the License. import contextlib +import crypt import datetime from distutils.version import StrictVersion import random @@ -42,6 +43,12 @@ LOG = log.getLogger(__name__) CONF = cfg.CONF +PASSWORD_HASH_FORMAT = { + 'sha256': crypt.METHOD_SHA256, + 'sha512': crypt.METHOD_SHA512, +} + + @task_manager.require_exclusive_lock def node_set_boot_device(task, device, persistent=False): """Set the boot device for a node. @@ -707,9 +714,13 @@ def remove_node_rescue_password(node, save=True): instance_info = node.instance_info if 'rescue_password' in instance_info: del instance_info['rescue_password'] - node.instance_info = instance_info - if save: - node.save() + + if 'hashed_rescue_password' in instance_info: + del instance_info['hashed_rescue_password'] + + node.instance_info = instance_info + if save: + node.save() def validate_instance_info_traits(node): @@ -1108,3 +1119,21 @@ def is_agent_token_pregenerated(node): """ return node.driver_internal_info.get( 'agent_secret_token_pregenerated', False) + + +def make_salt(): + """Generate a random salt with the indicator tag for password type. + + :returns: a valid salt for use with crypt.crypt + """ + return crypt.mksalt( + method=PASSWORD_HASH_FORMAT[ + CONF.conductor.rescue_password_hash_algorithm]) + + +def hash_password(password=''): + """Hashes a supplied password. + + :param value: Value to be hashed + """ + return crypt.crypt(password, make_salt()) diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index 8b37b5467..da98678a6 100644 --- a/ironic/conf/conductor.py +++ b/ironic/conf/conductor.py @@ -252,6 +252,18 @@ opts = [ mutable=True, help=_('Glance ID, http:// or file:// URL of the initramfs of ' 'the default rescue image.')), + cfg.StrOpt('rescue_password_hash_algorithm', + default='sha256', + choices=['sha256', 'sha512'], + help=_('Password hash algorithm to be used for the rescue ' + 'password.')), + cfg.BoolOpt('require_rescue_password_hashed', + # TODO(TheJulia): Change this to True in Victoria. + default=False, + help=_('Option to cause the conductor to not fallback to ' + 'an un-hashed version of the rescue password, ' + 'permitting rescue with older ironic-python-agent ' + 'ramdisks.')), cfg.StrOpt('bootloader', mutable=True, help=_('Glance ID, http:// or file:// URL of the EFI system ' diff --git a/ironic/drivers/modules/agent_client.py b/ironic/drivers/modules/agent_client.py index 32427c6e7..cd3a91ef1 100644 --- a/ironic/drivers/modules/agent_client.py +++ b/ironic/drivers/modules/agent_client.py @@ -379,15 +379,35 @@ class AgentClient(object): to issue the request, or there was a malformed response from the agent. :raises: AgentAPIError when agent failed to execute specified command. + :raises: InstanceRescueFailure when the agent ramdisk is too old + to support transmission of the rescue password. :returns: A dict containing command response from agent. See :func:`get_commands_status` for a command result sample. """ - rescue_pass = node.instance_info.get('rescue_password') + rescue_pass = node.instance_info.get('hashed_rescue_password') + # TODO(TheJulia): Remove fallback to use the fallback_rescue_password + # in the Victoria cycle. + fallback_rescue_pass = node.instance_info.get( + 'rescue_password') if not rescue_pass: raise exception.IronicException(_('Agent rescue requires ' 'rescue_password in ' 'instance_info')) - params = {'rescue_password': rescue_pass} - return self._command(node=node, - method='rescue.finalize_rescue', - params=params) + params = {'rescue_password': rescue_pass, + 'hashed': True} + try: + return self._command(node=node, + method='rescue.finalize_rescue', + params=params) + except exception.AgentAPIError: + if CONF.conductor.require_rescue_password_hashed: + raise exception.InstanceRescueFailure( + _('Unable to rescue node due to an out of date agent ' + 'ramdisk. Please contact the administrator to update ' + 'the rescue ramdisk to contain an ironic-python-agent ' + 'version of at least 6.0.0.')) + else: + params = {'rescue_password': fallback_rescue_pass} + return self._command(node=node, + method='rescue.finalize_rescue', + params=params) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 285317f31..59cc62769 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2650,6 +2650,7 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, call_args=(self.service._do_node_rescue, task), err_handler=conductor_utils.spawn_rescue_error_handler) self.assertIn('rescue_password', task.node.instance_info) + self.assertIn('hashed_rescue_password', task.node.instance_info) self.assertNotIn('agent_url', task.node.driver_internal_info) def test_do_node_rescue_invalid_state(self): @@ -2663,6 +2664,7 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, self.context, node.uuid, "password") node.refresh() self.assertNotIn('rescue_password', node.instance_info) + self.assertNotIn('hashed_rescue_password', node.instance_info) self.assertEqual(exception.InvalidStateRequested, exc.exc_info[0]) def _test_do_node_rescue_when_validate_fail(self, mock_validate): @@ -2677,7 +2679,7 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, self.service.do_node_rescue, self.context, node.uuid, "password") node.refresh() - self.assertNotIn('rescue_password', node.instance_info) + self.assertNotIn('hashed_rescue_password', node.instance_info) # Compare true exception hidden by @messaging.expected_exceptions self.assertEqual(exception.InstanceRescueFailure, exc.exc_info[0]) @@ -2712,10 +2714,11 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, @mock.patch('ironic.drivers.modules.fake.FakeRescue.rescue') def test__do_node_rescue_returns_rescuewait(self, mock_rescue): self._start_service() - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.RESCUING, - instance_info={'rescue_password': - 'password'}) + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.RESCUING, + instance_info={'rescue_password': 'password', + 'hashed_rescue_password': '1234'}) with task_manager.TaskManager(self.context, node.uuid) as task: mock_rescue.return_value = states.RESCUEWAIT self.service._do_node_rescue(task) @@ -2723,14 +2726,17 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, self.assertEqual(states.RESCUEWAIT, node.provision_state) self.assertEqual(states.RESCUE, node.target_provision_state) self.assertIn('rescue_password', node.instance_info) + self.assertIn('hashed_rescue_password', node.instance_info) @mock.patch('ironic.drivers.modules.fake.FakeRescue.rescue') def test__do_node_rescue_returns_rescue(self, mock_rescue): self._start_service() - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.RESCUING, - instance_info={'rescue_password': - 'password'}) + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.RESCUING, + instance_info={ + 'rescue_password': 'password', + 'hashed_rescue_password': '1234'}) with task_manager.TaskManager(self.context, node.uuid) as task: mock_rescue.return_value = states.RESCUE self.service._do_node_rescue(task) @@ -2738,15 +2744,18 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, self.assertEqual(states.RESCUE, node.provision_state) self.assertEqual(states.NOSTATE, node.target_provision_state) self.assertIn('rescue_password', node.instance_info) + self.assertIn('hashed_rescue_password', node.instance_info) @mock.patch.object(manager, 'LOG') @mock.patch('ironic.drivers.modules.fake.FakeRescue.rescue') def test__do_node_rescue_errors(self, mock_rescue, mock_log): self._start_service() - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.RESCUING, - instance_info={'rescue_password': - 'password'}) + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.RESCUING, + instance_info={ + 'rescue_password': 'password', + 'hashed_rescue_password': '1234'}) mock_rescue.side_effect = exception.InstanceRescueFailure( 'failed to rescue') with task_manager.TaskManager(self.context, node.uuid) as task: @@ -2756,6 +2765,7 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, self.assertEqual(states.RESCUEFAIL, node.provision_state) self.assertEqual(states.RESCUE, node.target_provision_state) self.assertNotIn('rescue_password', node.instance_info) + self.assertNotIn('hashed_rescue_password', node.instance_info) self.assertTrue(node.last_error.startswith('Failed to rescue')) self.assertTrue(mock_log.error.called) @@ -2763,10 +2773,12 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, @mock.patch('ironic.drivers.modules.fake.FakeRescue.rescue') def test__do_node_rescue_bad_state(self, mock_rescue, mock_log): self._start_service() - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.RESCUING, - instance_info={'rescue_password': - 'password'}) + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.RESCUING, + instance_info={ + 'rescue_password': 'password', + 'hashed_rescue_password': '1234'}) mock_rescue.return_value = states.ACTIVE with task_manager.TaskManager(self.context, node.uuid) as task: self.service._do_node_rescue(task) @@ -2774,6 +2786,7 @@ class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, self.assertEqual(states.RESCUEFAIL, node.provision_state) self.assertEqual(states.RESCUE, node.target_provision_state) self.assertNotIn('rescue_password', node.instance_info) + self.assertNotIn('hashed_rescue_password', node.instance_info) self.assertTrue(node.last_error.startswith('Failed to rescue')) self.assertTrue(mock_log.error.called) diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 6a437debe..c600341e2 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -1184,17 +1184,20 @@ class ErrorHandlersTestCase(tests_base.TestCase): @mock.patch.object(conductor_utils, 'LOG') def test_spawn_rescue_error_handler_no_worker(self, log_mock): exc = exception.NoFreeConductorWorker() - self.node.instance_info = {'rescue_password': 'pass'} + self.node.instance_info = {'rescue_password': 'pass', + 'hashed_rescue_password': '12'} conductor_utils.spawn_rescue_error_handler(exc, self.node) self.node.save.assert_called_once_with() self.assertIn('No free conductor workers', self.node.last_error) self.assertTrue(log_mock.warning.called) self.assertNotIn('rescue_password', self.node.instance_info) + self.assertNotIn('hashed_rescue_password', self.node.instance_info) @mock.patch.object(conductor_utils, 'LOG') def test_spawn_rescue_error_handler_other_error(self, log_mock): exc = Exception('foo') - self.node.instance_info = {'rescue_password': 'pass'} + self.node.instance_info = {'rescue_password': 'pass', + 'hashed_rescue_password': '12'} conductor_utils.spawn_rescue_error_handler(exc, self.node) self.assertFalse(self.node.save.called) self.assertFalse(log_mock.warning.called) diff --git a/ironic/tests/unit/drivers/modules/test_agent.py b/ironic/tests/unit/drivers/modules/test_agent.py index 81790a28a..9a337e58a 100644 --- a/ironic/tests/unit/drivers/modules/test_agent.py +++ b/ironic/tests/unit/drivers/modules/test_agent.py @@ -1954,7 +1954,8 @@ class AgentRescueTestCase(db_base.DbTestCase): self.config(**config_kwarg) self.config(enabled_hardware_types=['fake-hardware']) instance_info = INSTANCE_INFO - instance_info.update({'rescue_password': 'password'}) + instance_info.update({'rescue_password': 'password', + 'hashed_rescue_password': '1234'}) driver_info = DRIVER_INFO driver_info.update({'rescue_ramdisk': 'my_ramdisk', 'rescue_kernel': 'my_kernel'}) diff --git a/ironic/tests/unit/drivers/modules/test_agent_client.py b/ironic/tests/unit/drivers/modules/test_agent_client.py index a39477f02..560a30e33 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_client.py +++ b/ironic/tests/unit/drivers/modules/test_agent_client.py @@ -330,8 +330,10 @@ class TestAgentClient(base.TestCase): def test_finalize_rescue(self): self.client._command = mock.MagicMock(spec_set=[]) self.node.instance_info['rescue_password'] = 'password' + self.node.instance_info['hashed_rescue_password'] = '1234' expected_params = { - 'rescue_password': 'password', + 'rescue_password': '1234', + 'hashed': True, } self.client.finalize_rescue(self.node) self.client._command.assert_called_once_with( @@ -346,6 +348,36 @@ class TestAgentClient(base.TestCase): self.node) self.assertFalse(self.client._command.called) + def test_finalize_rescue_fallback(self): + self.config(require_rescue_password_hashed=False, group="conductor") + self.client._command = mock.MagicMock(spec_set=[]) + self.node.instance_info['rescue_password'] = 'password' + self.node.instance_info['hashed_rescue_password'] = '1234' + self.client._command.side_effect = [ + exception.AgentAPIError('blah'), + ('', '')] + self.client.finalize_rescue(self.node) + self.client._command.assert_has_calls([ + mock.call(node=mock.ANY, method='rescue.finalize_rescue', + params={'rescue_password': '1234', + 'hashed': True}), + mock.call(node=mock.ANY, method='rescue.finalize_rescue', + params={'rescue_password': 'password'})]) + + def test_finalize_rescue_fallback_restricted(self): + self.config(require_rescue_password_hashed=True, group="conductor") + self.client._command = mock.MagicMock(spec_set=[]) + self.node.instance_info['rescue_password'] = 'password' + self.node.instance_info['hashed_rescue_password'] = '1234' + self.client._command.side_effect = exception.AgentAPIError('blah') + self.assertRaises(exception.InstanceRescueFailure, + self.client.finalize_rescue, + self.node) + self.client._command.assert_has_calls([ + mock.call(node=mock.ANY, method='rescue.finalize_rescue', + params={'rescue_password': '1234', + 'hashed': True})]) + def test__command_agent_client(self): response_data = {'status': 'ok'} response_text = json.dumps(response_data) -- cgit v1.2.1