openstack/ironic
Code Issues Proposed changes

Hash the rescue_password

Browse Source 
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
This commit is contained in:
Julia Kreger 2019-10-25 11:31:53 -07:00
parent 3eb0c16401
commit fcaefdbe74
9 changed files with 167 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
ironic/conductor/manager.py
View File

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

35
ironic/conductor/utils.py
View File

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

12
ironic/conf/conductor.py
View File

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

30
ironic/drivers/modules/agent_client.py
View File

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

47
ironic/tests/unit/conductor/test_manager.py
View File

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

7
ironic/tests/unit/conductor/test_utils.py
View File

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

3
ironic/tests/unit/drivers/modules/test_agent.py
View File

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

34
ironic/tests/unit/drivers/modules/test_agent_client.py
View File

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

23
releasenotes/notes/support_to_hash_rescue_password-0915927e41e6d845.yaml Normal file
View File

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