From 0f7a85e1ecd7b3e7bdbc297f47a498139ed40107 Mon Sep 17 00:00:00 2001 From: Madhuri Kumari Date: Tue, 31 Jan 2017 09:16:25 +0000 Subject: [PATCH] Fix directories permission for tftpboot Currently method "_ensure_config_dirs_exist" creates tftpboot/ dir with wrong permission. This is due to the system umask setting which overrides the default permission of 0777 to 0755 or 0750. When the permission is 0750, BM can't get deploy_kernel and ramdisk from tftpserver. This may happen only when tftp process is launched from other user than root and as result can't read files created by Ironic. So this patch tries to fix the issue by explicitly changing the permissions defined in the config option ``[pxe]/dir_permission``. Change-Id: I3119ec7ae31bf82f716bf082fa4c3296d6aa3587 Closes-bug: #1655568 --- etc/ironic/ironic.conf.sample | 11 +++ ironic/common/pxe_utils.py | 13 ++- ironic/conf/pxe.py | 11 +++ ironic/tests/unit/common/test_pxe_utils.py | 87 +++++++++++++++++-- .../fix-dir-permissions-bc56e83a651bbdb0.yaml | 7 ++ 5 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/fix-dir-permissions-bc56e83a651bbdb0.yaml diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 405a66003f..32be0cb3a5 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -3309,6 +3309,17 @@ # caching. (string value) #tftp_master_path = /tftpboot/master_images +# The permission that will be applied to the TFTP folders upon +# creation. This should be set to the permission such that the +# tftpserver has access to read the contents of the configured +# TFTP folder. This setting is only required when the +# operating system's umask is restrictive such that ironic- +# conductor is creating files that cannot be read by the TFTP +# server. Setting to will result in the operating +# system's umask to be utilized for the creation of new tftp +# folders. (integer value) +#dir_permission = + # Bootfile DHCP parameter. (string value) #pxe_bootfile_name = pxelinux.0 diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py index 5779d21990..bd57f6ef47 100644 --- a/ironic/common/pxe_utils.py +++ b/ironic/common/pxe_utils.py @@ -49,8 +49,17 @@ def _ensure_config_dirs_exist(node_uuid): """ root_dir = get_root_dir() - fileutils.ensure_tree(os.path.join(root_dir, node_uuid)) - fileutils.ensure_tree(os.path.join(root_dir, PXE_CFG_DIR_NAME)) + node_dir = os.path.join(root_dir, node_uuid) + pxe_dir = os.path.join(root_dir, PXE_CFG_DIR_NAME) + # NOTE: We should only change the permissions if the folder + # does not exist. i.e. if defined, an operator could have + # already created it and placed specific ACLs upon the folder + # which may not recurse downward. + for directory in (node_dir, pxe_dir): + if not os.path.isdir(directory): + fileutils.ensure_tree(directory) + if CONF.pxe.dir_permission: + os.chmod(directory, CONF.pxe.dir_permission) def _link_mac_pxe_configs(task): diff --git a/ironic/conf/pxe.py b/ironic/conf/pxe.py index 355c45f104..c8c02a8f9d 100644 --- a/ironic/conf/pxe.py +++ b/ironic/conf/pxe.py @@ -77,6 +77,17 @@ opts = [ help=_('On ironic-conductor node, directory where master TFTP ' 'images are stored on disk. ' 'Setting to disables image caching.')), + cfg.IntOpt('dir_permission', + help=_("The permission that will be applied to the TFTP " + "folders upon creation. This should be set to the " + "permission such that the tftpserver has access to " + "read the contents of the configured TFTP folder. This " + "setting is only required when the operating system's " + "umask is restrictive such that ironic-conductor is " + "creating files that cannot be read by the TFTP server. " + "Setting to will result in the operating " + "system's umask to be utilized for the creation of new " + "tftp folders.")), cfg.StrOpt('pxe_bootfile_name', default='pxelinux.0', help=_('Bootfile DHCP parameter.')), diff --git a/ironic/tests/unit/common/test_pxe_utils.py b/ironic/tests/unit/common/test_pxe_utils.py index 668cac7ad0..830ce2f37c 100644 --- a/ironic/tests/unit/common/test_pxe_utils.py +++ b/ironic/tests/unit/common/test_pxe_utils.py @@ -277,11 +277,12 @@ class TestPXEUtils(db_base.DbTestCase): unlink_mock.assert_called_once_with('/tftpboot/10.10.0.1.conf') create_link_mock.assert_has_calls(create_link_calls) + @mock.patch.object(os, 'chmod', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config(self, ensure_tree_mock, render_mock, - write_mock): + write_mock, chmod_mock): with task_manager.acquire(self.context, self.node.uuid) as task: pxe_utils.create_pxe_config(task, self.pxe_options, CONF.pxe.pxe_config_template) @@ -291,23 +292,84 @@ class TestPXEUtils(db_base.DbTestCase): 'ROOT': '{{ ROOT }}', 'DISK_IDENTIFIER': '{{ DISK_IDENTIFIER }}'} ) + node_dir = os.path.join(CONF.pxe.tftp_root, self.node.uuid) + pxe_dir = os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg') ensure_calls = [ - mock.call(os.path.join(CONF.pxe.tftp_root, self.node.uuid)), - mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')) + mock.call(node_dir), mock.call(pxe_dir), ] ensure_tree_mock.assert_has_calls(ensure_calls) + chmod_mock.assert_not_called() pxe_cfg_file_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) write_mock.assert_called_with(pxe_cfg_file_path, render_mock.return_value) + @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch('ironic.common.utils.write_to_file', autospec=True) + @mock.patch('ironic.common.utils.render_template', autospec=True) + @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) + def test_create_pxe_config_set_dir_permission(self, ensure_tree_mock, + render_mock, + write_mock, chmod_mock): + self.config(dir_permission=0o755, group='pxe') + with task_manager.acquire(self.context, self.node.uuid) as task: + pxe_utils.create_pxe_config(task, self.pxe_options, + CONF.pxe.pxe_config_template) + render_mock.assert_called_with( + CONF.pxe.pxe_config_template, + {'pxe_options': self.pxe_options, + 'ROOT': '{{ ROOT }}', + 'DISK_IDENTIFIER': '{{ DISK_IDENTIFIER }}'} + ) + node_dir = os.path.join(CONF.pxe.tftp_root, self.node.uuid) + pxe_dir = os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg') + ensure_calls = [ + mock.call(node_dir), mock.call(pxe_dir), + ] + ensure_tree_mock.assert_has_calls(ensure_calls) + chmod_calls = [mock.call(node_dir, 0o755), mock.call(pxe_dir, 0o755)] + chmod_mock.assert_has_calls(chmod_calls) + + pxe_cfg_file_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) + write_mock.assert_called_with(pxe_cfg_file_path, + render_mock.return_value) + + @mock.patch.object(os.path, 'isdir', autospec=True) + @mock.patch.object(os, 'chmod', autospec=True) + @mock.patch('ironic.common.utils.write_to_file', autospec=True) + @mock.patch('ironic.common.utils.render_template', autospec=True) + @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) + def test_create_pxe_config_existing_dirs(self, ensure_tree_mock, + render_mock, + write_mock, chmod_mock, + isdir_mock): + self.config(dir_permission=0o755, group='pxe') + with task_manager.acquire(self.context, self.node.uuid) as task: + isdir_mock.return_value = True + pxe_utils.create_pxe_config(task, self.pxe_options, + CONF.pxe.pxe_config_template) + render_mock.assert_called_with( + CONF.pxe.pxe_config_template, + {'pxe_options': self.pxe_options, + 'ROOT': '{{ ROOT }}', + 'DISK_IDENTIFIER': '{{ DISK_IDENTIFIER }}'} + ) + ensure_tree_mock.assert_has_calls([]) + chmod_mock.assert_not_called() + isdir_mock.assert_has_calls([]) + pxe_cfg_file_path = pxe_utils.get_pxe_config_file_path(self.node.uuid) + write_mock.assert_called_with(pxe_cfg_file_path, + render_mock.return_value) + + @mock.patch.object(os, 'chmod', autospec=True) @mock.patch('ironic.common.pxe_utils._link_ip_address_pxe_configs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config_uefi_elilo(self, ensure_tree_mock, render_mock, - write_mock, link_ip_configs_mock): + write_mock, link_ip_configs_mock, + chmod_mock): self.config( uefi_pxe_config_template=('ironic/drivers/modules/' 'elilo_efi_pxe_config.template'), @@ -320,9 +382,10 @@ class TestPXEUtils(db_base.DbTestCase): ensure_calls = [ mock.call(os.path.join(CONF.pxe.tftp_root, self.node.uuid)), - mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')) + mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')), ] ensure_tree_mock.assert_has_calls(ensure_calls) + chmod_mock.assert_not_called() render_mock.assert_called_with( CONF.pxe.uefi_pxe_config_template, {'pxe_options': self.pxe_options, @@ -334,13 +397,15 @@ class TestPXEUtils(db_base.DbTestCase): write_mock.assert_called_with(pxe_cfg_file_path, render_mock.return_value) + @mock.patch.object(os, 'chmod', autospec=True) @mock.patch('ironic.common.pxe_utils._link_ip_address_pxe_configs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config_uefi_grub(self, ensure_tree_mock, render_mock, - write_mock, link_ip_configs_mock): + write_mock, link_ip_configs_mock, + chmod_mock): grub_tmplte = "ironic/drivers/modules/pxe_grub_config.template" with task_manager.acquire(self.context, self.node.uuid) as task: task.node.properties['capabilities'] = 'boot_mode:uefi' @@ -349,9 +414,10 @@ class TestPXEUtils(db_base.DbTestCase): ensure_calls = [ mock.call(os.path.join(CONF.pxe.tftp_root, self.node.uuid)), - mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')) + mock.call(os.path.join(CONF.pxe.tftp_root, 'pxelinux.cfg')), ] ensure_tree_mock.assert_has_calls(ensure_calls) + chmod_mock.assert_not_called() render_mock.assert_called_with( grub_tmplte, {'pxe_options': self.pxe_options, @@ -363,13 +429,15 @@ class TestPXEUtils(db_base.DbTestCase): write_mock.assert_called_with(pxe_cfg_file_path, render_mock.return_value) + @mock.patch.object(os, 'chmod', autospec=True) @mock.patch('ironic.common.pxe_utils._link_mac_pxe_configs', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('oslo_utils.fileutils.ensure_tree', autospec=True) def test_create_pxe_config_uefi_ipxe(self, ensure_tree_mock, render_mock, - write_mock, link_mac_pxe_mock): + write_mock, link_mac_pxe_mock, + chmod_mock): self.config(ipxe_enabled=True, group='pxe') ipxe_template = "ironic/drivers/modules/ipxe_config.template" with task_manager.acquire(self.context, self.node.uuid) as task: @@ -379,9 +447,10 @@ class TestPXEUtils(db_base.DbTestCase): ensure_calls = [ mock.call(os.path.join(CONF.deploy.http_root, self.node.uuid)), - mock.call(os.path.join(CONF.deploy.http_root, 'pxelinux.cfg')) + mock.call(os.path.join(CONF.deploy.http_root, 'pxelinux.cfg')), ] ensure_tree_mock.assert_has_calls(ensure_calls) + chmod_mock.assert_not_called() render_mock.assert_called_with( ipxe_template, {'pxe_options': self.ipxe_options, diff --git a/releasenotes/notes/fix-dir-permissions-bc56e83a651bbdb0.yaml b/releasenotes/notes/fix-dir-permissions-bc56e83a651bbdb0.yaml new file mode 100644 index 0000000000..e36b4bb219 --- /dev/null +++ b/releasenotes/notes/fix-dir-permissions-bc56e83a651bbdb0.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - Adds the capability for an operator to explicitly define the permission + for created tftpboot folders. This provides the ability for ironic to be + utilized with a restrictive umask, where the tftp server may not be able + to read the file. Introduced a new config option ``[pxe]/dir_permission`` + to specify the permission for the tftpboot directories to be created with.