From 3cf41d921b3c72a48b3ebbc6a80e9fd6273c2fc6 Mon Sep 17 00:00:00 2001 From: Yuriy Zveryanskyy Date: Fri, 27 Feb 2015 14:12:52 +0200 Subject: [PATCH] Do not save auth token on TFTP server in PXE driver Reasons: 1) Store admin auth token on the disk is big security risk. 2) Mechanism is not generic, only PXE driver does this. 3) IPA uses vendor API via 'acl_public_routes'. 4) Internal 'deploy_key' check used for iSCSI deploy. Changes in this patch: 1) Token saving code removed from PXE driver. 2) PXE vendor methods added to 'acl_public_routes'. 3) Unit tests of PXE driver updated. Change-Id: I12cfdc0a8a7740cadd768068b91c258e028ef385 --- ironic/api/config.py | 8 +++- ironic/drivers/modules/pxe.py | 43 ++--------------- ironic/tests/drivers/test_pxe.py | 79 +++----------------------------- 3 files changed, 16 insertions(+), 114 deletions(-) diff --git a/ironic/api/config.py b/ironic/api/config.py index 38938c159a..0321306feb 100644 --- a/ironic/api/config.py +++ b/ironic/api/config.py @@ -30,8 +30,14 @@ app = { 'acl_public_routes': [ '/', '/v1', + # IPA ramdisk methods '/v1/drivers/[a-z_]*/vendor_passthru/lookup', - '/v1/nodes/[a-z0-9\-]+/vendor_passthru/heartbeat' + '/v1/nodes/[a-z0-9\-]+/vendor_passthru/heartbeat', + # DIB ramdisk methods + # NOTE(yuriyz): support URL without 'v1' for backward compatibility + # with old DIB ramdisks. + '(?:/v1)?/nodes/[a-z0-9\-]+/vendor_passthru/pass_(?:deploy|' + 'bootloader_install)_info', ], } diff --git a/ironic/drivers/modules/pxe.py b/ironic/drivers/modules/pxe.py index 07ccf2fe67..d4d28faa92 100644 --- a/ironic/drivers/modules/pxe.py +++ b/ironic/drivers/modules/pxe.py @@ -31,7 +31,6 @@ from ironic.common.i18n import _ from ironic.common.i18n import _LE from ironic.common.i18n import _LW from ironic.common import image_service as service -from ironic.common import keystone from ironic.common import paths from ironic.common import pxe_utils from ironic.common import states @@ -205,11 +204,6 @@ def _build_pxe_config_options(node, pxe_info, ctx): return pxe_options -def _get_token_file_path(node_uuid): - """Generate the path for PKI token file.""" - return os.path.join(CONF.pxe.tftp_root, 'token-' + node_uuid) - - def validate_boot_option_for_uefi(node): """In uefi boot mode, validate if the boot option is compatible. @@ -293,25 +287,6 @@ def _get_image_info(node, ctx): return image_info -def _create_token_file(task): - """Save PKI token to file.""" - token_file_path = _get_token_file_path(task.node.uuid) - token = task.context.auth_token - if token: - timeout = CONF.conductor.deploy_callback_timeout - if timeout and keystone.token_expires_soon(token, timeout): - token = keystone.get_admin_auth_token() - utils.write_to_file(token_file_path, token) - else: - utils.unlink_without_raise(token_file_path) - - -def _destroy_token_file(node): - """Delete PKI token file.""" - token_file_path = _get_token_file_path(node['uuid']) - utils.unlink_without_raise(token_file_path) - - class PXEDeploy(base.DeployInterface): """PXE Deploy Interface for deploy-related actions.""" @@ -368,9 +343,8 @@ class PXEDeploy(base.DeployInterface): def deploy(self, task): """Start deployment of the task's node'. - Fetches instance image, creates a temporary keystone token file, - updates the DHCP port options for next boot, and issues a reboot - request to the power driver. + Fetches instance image, updates the DHCP port options for next boot, + and issues a reboot request to the power driver. This causes the node to boot into the deployment ramdisk and triggers the next phase of PXE-based deployment via VendorPassthru.pass_deploy_info(). @@ -381,9 +355,6 @@ class PXEDeploy(base.DeployInterface): iscsi_deploy.cache_instance_image(task.context, task.node) iscsi_deploy.check_image_size(task) - # TODO(yuriyz): more secure way needed for pass auth token - # to deploy ramdisk - _create_token_file(task) dhcp_opts = pxe_utils.dhcp_options_for_instance(task) provider = dhcp_factory.DHCPFactory() provider.update_dhcp(task, dhcp_opts) @@ -471,8 +442,7 @@ class PXEDeploy(base.DeployInterface): """Clean up the deployment environment for the task's node. Unlinks TFTP and instance images and triggers image cache cleanup. - Removes the TFTP configuration files for this node. As a precaution, - this method also ensures the keystone auth token file was removed. + Removes the TFTP configuration files for this node. :param task: a TaskManager instance containing the node to act on. """ @@ -493,7 +463,6 @@ class PXEDeploy(base.DeployInterface): pxe_utils.clean_up_pxe_config(task) iscsi_deploy.destroy_images(node.uuid) - _destroy_token_file(node) def take_over(self, task): if not iscsi_deploy.get_boot_option(task.node) == "local": @@ -577,7 +546,6 @@ class VendorPassthru(agent_base_vendor.BaseAgentVendor): task.process_event('resume') LOG.debug('Continuing the deployment on node %s', node.uuid) - _destroy_token_file(node) is_whole_disk_image = node.driver_internal_info['is_whole_disk_image'] uuid_dict = iscsi_deploy.continue_deploy(task, **kwargs) root_uuid_or_disk_id = uuid_dict.get( @@ -644,11 +612,6 @@ class VendorPassthru(agent_base_vendor.BaseAgentVendor): node = task.node LOG.debug('Continuing the deployment on node %s', node.uuid) - # NOTE(lucasagomes): We don't use the token file with the agent, - # but as it's created as part of deploy() we are going to remove - # it here. - _destroy_token_file(node) - uuid_dict = iscsi_deploy.do_agent_iscsi_deploy(task, self._client) is_whole_disk_image = node.driver_internal_info['is_whole_disk_image'] diff --git a/ironic/tests/drivers/test_pxe.py b/ironic/tests/drivers/test_pxe.py index 8be559c999..fe39797d9d 100644 --- a/ironic/tests/drivers/test_pxe.py +++ b/ironic/tests/drivers/test_pxe.py @@ -359,11 +359,6 @@ class PXEPrivateMethodsTestCase(db_base.DbTestCase): self.context) self.assertEqual(expected_options, options) - def test_get_token_file_path(self): - node_uuid = self.node.uuid - self.assertEqual('/tftpboot/token-' + node_uuid, - pxe._get_token_file_path(node_uuid)) - @mock.patch.object(deploy_utils, 'fetch_images', autospec=True) def test__cache_tftp_images_master_path(self, mock_fetch_image): temp_dir = tempfile.mkdtemp() @@ -459,7 +454,7 @@ class PXEDriverTestCase(db_base.DbTestCase): def setUp(self): super(PXEDriverTestCase, self).setUp() - self.context.auth_token = '4562138218392831' + self.context.auth_token = 'fake' self.temp_dir = tempfile.mkdtemp() self.config(tftp_root=self.temp_dir, group='pxe') self.temp_dir = tempfile.mkdtemp() @@ -477,11 +472,6 @@ class PXEDriverTestCase(db_base.DbTestCase): node_id=self.node.id) self.config(group='conductor', api_url='http://127.0.0.1:1234/') - def _create_token_file(self): - token_path = pxe._get_token_file_path(self.node.uuid) - open(token_path, 'w').close() - return token_path - def test_get_properties(self): expected = pxe.COMMON_PROPERTIES with task_manager.acquire(self.context, self.node.uuid, @@ -809,7 +799,6 @@ class PXEDriverTestCase(db_base.DbTestCase): self.node.save() self._test_prepare_node_active() - @mock.patch.object(keystone, 'token_expires_soon', autospec=True) @mock.patch.object(deploy_utils, 'get_image_mb', autospec=True) @mock.patch.object(iscsi_deploy, '_get_image_file_path', autospec=True) @mock.patch.object(iscsi_deploy, 'cache_instance_image', autospec=True) @@ -818,11 +807,10 @@ class PXEDriverTestCase(db_base.DbTestCase): @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True) def test_deploy(self, mock_node_set_boot, mock_node_power_action, mock_update_dhcp, mock_cache_instance_image, - mock_get_image_file_path, mock_get_image_mb, mock_expire): + mock_get_image_file_path, mock_get_image_mb): fake_img_path = '/test/path/test.img' mock_get_image_file_path.return_value = fake_img_path mock_get_image_mb.return_value = 1 - mock_expire.return_value = False self.config(deploy_callback_timeout=600, group='conductor') with task_manager.acquire(self.context, @@ -835,45 +823,10 @@ class PXEDriverTestCase(db_base.DbTestCase): mock_get_image_file_path.assert_called_once_with(task.node.uuid) mock_get_image_mb.assert_called_once_with(fake_img_path) mock_update_dhcp.assert_called_once_with(mock.ANY, task, dhcp_opts) - mock_expire.assert_called_once_with(self.context.auth_token, 600) mock_node_set_boot.assert_called_once_with(task, 'pxe', persistent=True) mock_node_power_action.assert_called_once_with(task, states.REBOOT) - # ensure token file created - t_path = pxe._get_token_file_path(self.node.uuid) - token = open(t_path, 'r').read() - self.assertEqual(self.context.auth_token, token) - - @mock.patch.object(keystone, 'get_admin_auth_token', autospec=True) - @mock.patch.object(keystone, 'token_expires_soon', autospec=True) - @mock.patch.object(deploy_utils, 'get_image_mb', autospec=True) - @mock.patch.object(iscsi_deploy, '_get_image_file_path', autospec=True) - @mock.patch.object(iscsi_deploy, 'cache_instance_image', autospec=True) - @mock.patch.object(dhcp_factory.DHCPFactory, 'update_dhcp', autospec=True) - @mock.patch.object(manager_utils, 'node_power_action', autospec=True) - @mock.patch.object(manager_utils, 'node_set_boot_device', autospec=True) - def test_deploy_token_near_expiration(self, mock_node_set_boot, - mock_node_power_action, mock_update_dhcp, - mock_cache_instance_image, mock_get_image_file_path, - mock_get_image_mb, mock_expire, mock_admin_token): - mock_get_image_mb.return_value = 1 - mock_expire.return_value = True - new_token = 'new_admin_token' - mock_admin_token.return_value = new_token - self.config(deploy_callback_timeout=600, group='conductor') - - with task_manager.acquire(self.context, - self.node.uuid, shared=False) as task: - task.driver.deploy.deploy(task) - - mock_expire.assert_called_once_with(self.context.auth_token, 600) - mock_admin_token.assert_called_once_with() - # ensure token file created with new token - t_path = pxe._get_token_file_path(self.node.uuid) - token = open(t_path, 'r').read() - self.assertEqual(new_token, token) - @mock.patch.object(deploy_utils, 'get_image_mb', autospec=True) @mock.patch.object(iscsi_deploy, '_get_image_file_path', autospec=True) @mock.patch.object(iscsi_deploy, 'cache_instance_image', autospec=True) @@ -942,8 +895,6 @@ class PXEDriverTestCase(db_base.DbTestCase): mock_image_cache, mock_switch_config, notify_mock, mock_node_boot_dev, mock_clean_pxe): - token_path = self._create_token_file() - # set local boot if is_localboot: i_info = self.node.instance_info @@ -968,7 +919,6 @@ class PXEDriverTestCase(db_base.DbTestCase): self.assertEqual(states.POWER_ON, self.node.power_state) self.assertIn('root_uuid_or_disk_id', self.node.driver_internal_info) self.assertIsNone(self.node.last_error) - self.assertFalse(os.path.exists(token_path)) mock_image_cache.assert_called_once_with() mock_image_cache.return_value.clean_up.assert_called_once_with() pxe_config_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) @@ -1000,8 +950,6 @@ class PXEDriverTestCase(db_base.DbTestCase): notify_mock, mock_node_boot_dev, mock_clean_pxe): - token_path = self._create_token_file() - # set local boot if is_localboot: i_info = self.node.instance_info @@ -1027,7 +975,6 @@ class PXEDriverTestCase(db_base.DbTestCase): self.node.refresh() self.assertEqual(states.POWER_ON, self.node.power_state) self.assertIsNone(self.node.last_error) - self.assertFalse(os.path.exists(token_path)) mock_image_cache.assert_called_once_with() mock_image_cache.return_value.clean_up.assert_called_once_with() pxe_config_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) @@ -1139,8 +1086,6 @@ class CleanUpTestCase(db_base.DbTestCase): task.context) mock_pxe_clean.assert_called_once_with(task) mock_unlink.assert_any_call('deploy_kernel') - mock_unlink.assert_any_call(pxe._get_token_file_path( - task.node.uuid)) mock_iscsi_clean.assert_called_once_with(task.node.uuid) mock_cache.return_value.clean_up.assert_called_once_with() @@ -1154,8 +1099,6 @@ class CleanUpTestCase(db_base.DbTestCase): mock_image_info.assert_called_once_with(task.node, task.context) mock_pxe_clean.assert_called_once_with(task) - mock_unlink.assert_called_once_with(pxe._get_token_file_path( - task.node.uuid)) mock_iscsi_clean.assert_called_once_with(task.node.uuid) mock_cache.return_value.clean_up.assert_called_once_with() @@ -1212,11 +1155,10 @@ class CleanUpFullFlowTestCase(db_base.DbTestCase): self.image_path = iscsi_deploy._get_image_file_path(self.node.uuid) self.config_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) self.mac_path = pxe_utils._get_pxe_mac_path(self.port.address) - self.token_path = pxe._get_token_file_path(self.node.uuid) # Create files self.files = [self.config_path, self.master_kernel_path, - self.master_instance_path, self.token_path] + self.master_instance_path] for fname in self.files: # NOTE(dtantsur): files with 0 size won't be cleaned up with open(fname, 'w') as fp: @@ -1267,16 +1209,13 @@ class TestAgentVendorPassthru(db_base.DbTestCase): 'reboot_and_finish_deploy', autospec=True) @mock.patch.object(deploy_utils, 'switch_pxe_config', autospec=True) @mock.patch.object(iscsi_deploy, 'do_agent_iscsi_deploy', autospec=True) - @mock.patch.object(pxe, '_destroy_token_file', autospec=True) - def test_continue_deploy_netboot(self, destroy_token_file_mock, - do_agent_iscsi_deploy_mock, + def test_continue_deploy_netboot(self, do_agent_iscsi_deploy_mock, switch_pxe_config_mock, reboot_and_finish_deploy_mock): uuid_dict_returned = {'root uuid': 'some-root-uuid'} do_agent_iscsi_deploy_mock.return_value = uuid_dict_returned self.driver.vendor.continue_deploy(self.task) - destroy_token_file_mock.assert_called_once_with(self.node) do_agent_iscsi_deploy_mock.assert_called_once_with( self.task, self.driver.vendor._client) tftp_config = '/tftpboot/%s/config' % self.node.uuid @@ -1292,9 +1231,7 @@ class TestAgentVendorPassthru(db_base.DbTestCase): @mock.patch.object(agent_base_vendor.BaseAgentVendor, 'configure_local_boot', autospec=True) @mock.patch.object(iscsi_deploy, 'do_agent_iscsi_deploy', autospec=True) - @mock.patch.object(pxe, '_destroy_token_file', autospec=True) - def test_continue_deploy_localboot(self, destroy_token_file_mock, - do_agent_iscsi_deploy_mock, + def test_continue_deploy_localboot(self, do_agent_iscsi_deploy_mock, configure_local_boot_mock, clean_up_pxe_config_mock, reboot_and_finish_deploy_mock): @@ -1306,7 +1243,6 @@ class TestAgentVendorPassthru(db_base.DbTestCase): do_agent_iscsi_deploy_mock.return_value = uuid_dict_returned self.driver.vendor.continue_deploy(self.task) - destroy_token_file_mock.assert_called_once_with(self.node) do_agent_iscsi_deploy_mock.assert_called_once_with( self.task, self.driver.vendor._client) configure_local_boot_mock.assert_called_once_with( @@ -1322,9 +1258,7 @@ class TestAgentVendorPassthru(db_base.DbTestCase): @mock.patch.object(agent_base_vendor.BaseAgentVendor, 'configure_local_boot', autospec=True) @mock.patch.object(iscsi_deploy, 'do_agent_iscsi_deploy', autospec=True) - @mock.patch.object(pxe, '_destroy_token_file', autospec=True) - def test_continue_deploy_localboot_uefi(self, destroy_token_file_mock, - do_agent_iscsi_deploy_mock, + def test_continue_deploy_localboot_uefi(self, do_agent_iscsi_deploy_mock, configure_local_boot_mock, clean_up_pxe_config_mock, reboot_and_finish_deploy_mock): @@ -1337,7 +1271,6 @@ class TestAgentVendorPassthru(db_base.DbTestCase): do_agent_iscsi_deploy_mock.return_value = uuid_dict_returned self.driver.vendor.continue_deploy(self.task) - destroy_token_file_mock.assert_called_once_with(self.node) do_agent_iscsi_deploy_mock.assert_called_once_with( self.task, self.driver.vendor._client) configure_local_boot_mock.assert_called_once_with(