From 9d3de26fb17f247c3140aaf31839b7cc3589ede0 Mon Sep 17 00:00:00 2001 From: Arun S A G Date: Tue, 23 Feb 2021 22:28:01 -0800 Subject: [PATCH] Validate the kickstart template and file before use The kickstart template is supplied by the user and it needs to be validated to make sure it includes all the expected variables and nothing else. We validate the template by rendering it using expected variables. If any of the expected variables are not present in the template or unexpected variables are defined in the template we raise InvalidKickstartTemplate exception Once we render the template into kickstart file we pass the file to 'ksvalidator' tool if it is present on the system to validate the rendered kickstart file for correctness. 'ksvalidator' tool comes from pykickstart libarary and it is GPLv2 licensed. GPLv2 license is incompatible with Openstack. So we do not explicitly include the library in requirements.txt instead rely on it being pre-existing on the conductor. If the 'ksvalidator' binary is not present on the system, kickstart validation will be skipped Change-Id: I3e040bbdbcefb8764c93355d0ba7179e2110b9c6 --- ironic/common/exception.py | 8 +++ ironic/common/pxe_utils.py | 63 +++++++++++++++++++ ironic/common/utils.py | 13 ++-- ironic/drivers/modules/pxe_base.py | 5 ++ ironic/tests/unit/common/test_pxe_utils.py | 22 +++++++ ironic/tests/unit/drivers/ks_extra_vars.tmpl | 41 ++++++++++++ ironic/tests/unit/drivers/ks_missing_var.tmpl | 37 +++++++++++ ironic/tests/unit/drivers/modules/test_pxe.py | 9 ++- 8 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 ironic/tests/unit/drivers/ks_extra_vars.tmpl create mode 100644 ironic/tests/unit/drivers/ks_missing_var.tmpl diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 4d1eb582c2..b0666748e4 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -711,6 +711,14 @@ class InvalidDeployTemplate(Invalid): _msg_fmt = _("Deploy template invalid: %(err)s.") +class InvalidKickstartTemplate(Invalid): + _msg_fmt = _("The kickstart template is missing required variables") + + +class InvalidKickstartFile(Invalid): + _msg_fmt = _("The kickstart file is not valid.") + + class IBMCError(DriverOperationError): _msg_fmt = _("IBMC exception occurred on node %(node)s. Error: %(error)s") diff --git a/ironic/common/pxe_utils.py b/ironic/common/pxe_utils.py index b670d983de..e37a3fcbc4 100644 --- a/ironic/common/pxe_utils.py +++ b/ironic/common/pxe_utils.py @@ -16,8 +16,11 @@ import copy import os +import tempfile from ironic_lib import utils as ironic_utils +import jinja2 +from oslo_concurrency import processutils from oslo_log import log as logging from oslo_utils import excutils from oslo_utils import fileutils @@ -1060,6 +1063,66 @@ def validate_boot_parameters_for_trusted_boot(node): raise exception.InvalidParameterValue(msg) +def validate_kickstart_template(ks_template): + """Validate the kickstart template + + :param ks_template: Path to the kickstart template + :raises: InvalidKickstartTemplate + """ + ks_options = {'liveimg_url': 'fake_image_url', + 'agent_token': 'fake_token', + 'heartbeat_url': 'fake_heartbeat_url'} + params = {'ks_options': ks_options} + try: + rendered_tmpl = utils.render_template(ks_template, params, strict=True) + except jinja2.exceptions.UndefinedError as exc: + msg = (_("The kickstart template includes a variable that is not " + "a valid kickstart option. Rendering the template returned " + " %(msg)s. The valid options are %(valid_options)s.") % + {'msg': exc.message, + 'valid_options': ','.join(ks_options.keys())}) + raise exception.InvalidKickstartTemplate(msg) + + missing_required_options = [] + for var, value in ks_options.items(): + if rendered_tmpl.find(value) == -1: + missing_required_options.append(var) + if missing_required_options: + msg = (_("Following required kickstart option variables are missing " + "from the kickstart template: %(missing_opts)s.") % + {'missing_opts': ','.join(missing_required_options)}) + raise exception.InvalidKickstartTemplate(msg) + return rendered_tmpl + + +def validate_kickstart_file(ks_cfg): + """Check if the kickstart file is valid + + :param ks_cfg: Contents of kickstart file to validate + :raises: InvalidKickstartFile + """ + if not os.path.isfile('/usr/bin/ksvalidator'): + LOG.warning( + "Unable to validate the kickstart file as ksvalidator binary is " + "missing. Please install pykickstart package to enable " + "validation of kickstart file." + ) + return + + with tempfile.NamedTemporaryFile( + dir=CONF.tempdir, suffix='.cfg') as ks_file: + ks_file.writelines(ks_cfg) + try: + result = utils.execute( + 'ksvalidator', ks_file.name, check_on_exit=[0], attempts=1 + ) + except processutils.ProcessExecutionError: + msg = _(("The kickstart file generated does not pass validation. " + "The ksvalidator tool returned following error(s): %s") % + (result)) + raise exception.InvalidKickstartFile(msg) + + def prepare_instance_pxe_config(task, image_info, iscsi_boot=False, ramdisk_boot=False, diff --git a/ironic/common/utils.py b/ironic/common/utils.py index cd323af364..eee581da63 100644 --- a/ironic/common/utils.py +++ b/ironic/common/utils.py @@ -470,13 +470,15 @@ def validate_network_port(port, port_name="Port"): {'port_name': port_name, 'port': port}) -def render_template(template, params, is_file=True): +def render_template(template, params, is_file=True, strict=False): """Renders Jinja2 template file with given parameters. :param template: full path to the Jinja2 template file :param params: dictionary with parameters to use when rendering :param is_file: whether template is file or string with template itself - :returns: the rendered template as a string + :param strict: Enable strict template rendering. Default is False + :returns: Rendered template + :raises: jinja2.exceptions.UndefinedError """ if is_file: tmpl_path, tmpl_name = os.path.split(template) @@ -488,8 +490,11 @@ def render_template(template, params, is_file=True): # and still complains with B701 for that line # NOTE(pas-ha) not using default_for_string=False as we set the name # of the template above for strings too. - env = jinja2.Environment(loader=loader, # nosec B701 - autoescape=jinja2.select_autoescape()) + env = jinja2.Environment( + loader=loader, + autoescape=jinja2.select_autoescape(), # nosec B701 + undefined=jinja2.StrictUndefined if strict else jinja2.Undefined + ) tmpl = env.get_template(tmpl_name) return tmpl.render(params, enumerate=enumerate) diff --git a/ironic/drivers/modules/pxe_base.py b/ironic/drivers/modules/pxe_base.py index d0c3a5e4ac..eb30f4846e 100644 --- a/ironic/drivers/modules/pxe_base.py +++ b/ironic/drivers/modules/pxe_base.py @@ -239,6 +239,11 @@ class PXEBaseMixin(object): task, ipxe_enabled=self.ipxe_enabled) pxe_utils.cache_ramdisk_kernel(task, instance_image_info, ipxe_enabled=self.ipxe_enabled) + if 'ks_template' in instance_image_info: + ks_cfg = pxe_utils.validate_kickstart_template( + instance_image_info['ks_template'][1] + ) + pxe_utils.validate_kickstart_file(ks_cfg) if (deploy_utils.is_iscsi_boot(task) or boot_option == "ramdisk" or boot_option == "kickstart"): diff --git a/ironic/tests/unit/common/test_pxe_utils.py b/ironic/tests/unit/common/test_pxe_utils.py index c7e5d76304..7dd28d61f6 100644 --- a/ironic/tests/unit/common/test_pxe_utils.py +++ b/ironic/tests/unit/common/test_pxe_utils.py @@ -1380,6 +1380,28 @@ class PXEBuildKickstartConfigOptionsTestCase(db_base.DbTestCase): write_mock.assert_called_with(image_info['ks_cfg'][1], render_mock.return_value) + def test_validate_kickstart_template(self): + self.config_temp_dir('http_root', group='deploy') + ks_template_path = 'ironic/drivers/modules/ks.cfg.template' + pxe_utils.validate_kickstart_template(ks_template_path) + + def test_validate_kickstart_template_missing_variable(self): + self.config_temp_dir('http_root', group='deploy') + # required variable is missing from the template + ks_template_path = 'ironic/tests/unit/drivers/ks_missing_var.tmpl' + self.assertRaises( + exception.InvalidKickstartTemplate, + pxe_utils.validate_kickstart_template, + ks_template_path) + + def test_validate_kickstart_template_has_additional_variables(self): + self.config_temp_dir('http_root', group='deploy') + ks_template_path = 'ironic/tests/unit/drivers/ks_extra_vars.tmpl' + self.assertRaises( + exception.InvalidKickstartTemplate, + pxe_utils.validate_kickstart_template, + ks_template_path) + @mock.patch.object(pxe.PXEBoot, '__init__', lambda self: None) class PXEBuildConfigOptionsTestCase(db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/ks_extra_vars.tmpl b/ironic/tests/unit/drivers/ks_extra_vars.tmpl new file mode 100644 index 0000000000..37f2e2d194 --- /dev/null +++ b/ironic/tests/unit/drivers/ks_extra_vars.tmpl @@ -0,0 +1,41 @@ +lang en_US +keyboard us +timezone UTC --utc +#platform x86, AMD64, or Intel EM64T +text +cmdline +reboot +selinux --enforcing +firewall --enabled +firstboot --disabled + +bootloader --location=mbr --append="rhgb quiet crashkernel=auto" +zerombr +clearpart --all --initlabel +autopart + +# Downloading and installing OS image using liveimg section is mandatory +liveimg --url {{ ks_options.liveimg_url }} + +# Following %pre, %onerror and %trackback sections are mandatory +%pre +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "start"}' {{ ks_options.heartbeat_url }} +%end + +%onerror +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "Error: Deploying using anaconda. Check console for more information."}' {{ ks_options.heartbeat_url }} +%end + +%traceback +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "Error: Installer crashed unexpectedly."}' {{ ks_options.heartbeat_url }} +%end + +# Sending callback after the installation is mandatory +%post +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "success"}' {{ ks_options.heartbeat_url }} +%end + +# config_drive is an extra variable and should raise an exception +%post +{{ config_drive }} +%end diff --git a/ironic/tests/unit/drivers/ks_missing_var.tmpl b/ironic/tests/unit/drivers/ks_missing_var.tmpl new file mode 100644 index 0000000000..ad160fb8f9 --- /dev/null +++ b/ironic/tests/unit/drivers/ks_missing_var.tmpl @@ -0,0 +1,37 @@ +lang en_US +keyboard us +timezone UTC --utc +#platform x86, AMD64, or Intel EM64T +text +cmdline +reboot +selinux --enforcing +firewall --enabled +firstboot --disabled + +bootloader --location=mbr --append="rhgb quiet crashkernel=auto" +zerombr +clearpart --all --initlabel +autopart + +# Downloading and installing OS image using liveimg section is mandatory +liveimg --url http://this_should_raise_an_exception + +# Following %pre, %onerror and %trackback sections are mandatory +%pre +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "start"}' {{ ks_options.heartbeat_url }} +%end + +%onerror +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "Error: Deploying using anaconda. Check console for more information."}' {{ ks_options.heartbeat_url }} +%end + +%traceback +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "Error: Installer crashed unexpectedly."}' {{ ks_options.heartbeat_url }} +%end + +# Sending callback after the installation is mandatory +%post +/usr/bin/curl -X PUT -H 'Content-Type: application/json' -H 'Accept:application/json' -d '{"agent_token": {{ ks_options.agent_token }}, "agent_status": "success"}' {{ ks_options.heartbeat_url }} +%end + diff --git a/ironic/tests/unit/drivers/modules/test_pxe.py b/ironic/tests/unit/drivers/modules/test_pxe.py index a204f6954b..3f7d9e4b73 100644 --- a/ironic/tests/unit/drivers/modules/test_pxe.py +++ b/ironic/tests/unit/drivers/modules/test_pxe.py @@ -755,9 +755,10 @@ class PXEBootTestCase(db_base.DbTestCase): return_value='http://fakeserver/api', autospec=True) @mock.patch('ironic.common.utils.render_template', autospec=True) @mock.patch('ironic.common.utils.write_to_file', autospec=True) + @mock.patch('ironic.common.utils.execute', autospec=True) def test_prepare_instance_kickstart( - self, write_file_mock, render_mock, api_url_mock, boot_opt_mock, - get_image_info_mock, cache_mock, dhcp_factory_mock, + self, exec_mock, write_file_mock, render_mock, api_url_mock, + boot_opt_mock, get_image_info_mock, cache_mock, dhcp_factory_mock, create_pxe_config_mock, switch_pxe_config_mock, set_boot_device_mock): image_info = {'kernel': ['ins_kernel_id', '/path/to/kernel'], @@ -784,6 +785,10 @@ class PXEBootTestCase(db_base.DbTestCase): ipxe_enabled=False) cache_mock.assert_called_once_with( task, image_info, False) + if os.path.isfile('/usr/bin/ksvalidator'): + exec_mock.assert_called_once_with( + 'ksvalidator', mock.ANY, check_on_exit=[0], attempts=1 + ) provider_mock.update_dhcp.assert_called_once_with(task, dhcp_opts) render_mock.assert_called() write_file_mock.assert_called_with(