Merge "Validate the kickstart template and file before use"

8 files changed, 192 insertions, 6 deletions

diff --git a/ironic/common/exception.py b/ironic/common/exception.py
index 4d1eb582c..b0666748e 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 98346d650..5c5d771bb 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
@@ -1064,6 +1067,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 ce482f2cc..390fe0305 100644
--- a/ironic/common/utils.py
+++ b/ironic/common/utils.py
@@ -468,13 +468,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)
@@ -486,8 +488,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 d0c3a5e4a..eb30f4846 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 36c23c70d..1bd47192a 100644
--- a/ironic/tests/unit/common/test_pxe_utils.py
+++ b/ironic/tests/unit/common/test_pxe_utils.py
@@ -1411,6 +1411,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 000000000..37f2e2d19
--- /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 000000000..ad160fb8f
--- /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 a204f6954..3f7d9e4b7 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(