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

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

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,

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)
 

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"):

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

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

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
+

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(