diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index aca403313b..bfc86e8254 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 2d97d655c3..170218c613 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 8b37b54679..da98678a6c 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 32427c6e71..cd3a91ef18 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 285317f31b..59cc627690 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 6a437debe2..c600341e24 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 81790a28ae..9a337e58a5 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 a39477f02b..560a30e33b 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) diff --git a/releasenotes/notes/support_to_hash_rescue_password-0915927e41e6d845.yaml b/releasenotes/notes/support_to_hash_rescue_password-0915927e41e6d845.yaml new file mode 100644 index 0000000000..178e0dc22c --- /dev/null +++ b/releasenotes/notes/support_to_hash_rescue_password-0915927e41e6d845.yaml @@ -0,0 +1,23 @@ +--- +features: + - | + Passwords for ``rescue`` operation are now hashed for + transmission to the ``ironic-python-agent``. This functionality + requires ``ironic-python-agent`` version ``6.0.0``. + + The setting ``[conductor]rescue_password_hash_algorithm`` + now defaults to ``sha256``, and may be set to + ``sha256``, or ``sha512``. +upgrades: + - | + The version of ``ironic-python-agent`` should be upgraded to + at least version ``6.0.0`` for rescue passwords to be hashed + for transmission. +security: + - | + Operators wishing to enforce all rescue passwords to be hashed + should use the ``[conductor]require_rescue_password_hashed`` + setting and set it to a value of ``True``. + + This setting will be changed to a default of ``True`` in the + Victoria development cycle.