Redfish secure boot management

Story: #2008270
Task: #41137
Change-Id: Ied53f8dc5b93522ac9ffc25ec93ad2347a7d1c7c

doc/source/admin/drivers/redfish.rst

@@ -117,6 +117,19 @@ bare metal node as well as set it to either Legacy BIOS or UEFI.
    it remains the responsibility of the operator to configure proper
    boot mode to their bare metal nodes.
 
+UEFI secure boot
+~~~~~~~~~~~~~~~~
+
+Secure boot mode can be automatically set and unset during deployment for nodes
+in UEFI boot mode, see :ref:`secure-boot` for an explanation how to use it.
+
+Two clean and deploy steps are provided for key management:
+
+``management.reset_secure_boot_keys_to_default``
+    resets secure boot keys to their manufacturing defaults.
+``management.clear_secure_boot_keys``
+    removes all secure boot keys from the node.
+
 Out-Of-Band inspection
 ======================
 

doc/source/admin/security.rst

@@ -136,8 +136,8 @@ UEFI secure boot mode
 =====================
 
 Some hardware types support turning `UEFI secure boot`_ dynamically when
-deploying an instance. Currently these are :doc:`/admin/drivers/ilo` and
+deploying an instance. Currently these are :doc:`/admin/drivers/ilo`,
-:doc:`/admin/drivers/irmc`.
+:doc:`/admin/drivers/irmc` and :doc:`/admin/drivers/redfish`.
 
 Support for the UEFI secure boot is declared by adding the ``secure_boot``
 capability in the ``capabilities`` parameter in the ``properties`` field of

driver-requirements.txt

@@ -11,7 +11,7 @@ python-dracclient>=5.1.0,<6.0.0
 python-xclarityclient>=0.1.6
 
 # The Redfish hardware type uses the Sushy library
-sushy>=3.4.0
+sushy>=3.6.0
 
 # Ansible-deploy interface
 ansible>=2.7

ironic/drivers/modules/redfish/boot.py

@@ -541,10 +541,12 @@ class RedfishVirtualMediaBoot(base.BootInterface):
         """
         node = task.node
 
+        self._eject_all(task)
+
         boot_mode_utils.sync_boot_mode(task)
+        boot_mode_utils.configure_secure_boot_if_needed(task)
 
         boot_option = deploy_utils.get_boot_option(node)
-        self.clean_up_instance(task)
         iwdi = node.driver_internal_info.get('is_whole_disk_image')
         if boot_option == "local" or iwdi:
             self._set_boot_device(task, boot_devices.DISK, persistent=True)
@@ -596,18 +598,7 @@ class RedfishVirtualMediaBoot(base.BootInterface):
                   "%(device)s", {'node': task.node.uuid,
                                  'device': boot_devices.CDROM})
 
-    def clean_up_instance(self, task):
+    def _eject_all(self, task):
-        """Cleans up the boot of instance.
-
-        This method cleans up the environment that was setup for booting
-        the instance.
-
-        :param task: A task from TaskManager.
-        :returns: None
-        """
-        LOG.debug("Cleaning up instance boot for "
-                  "%(node)s", {'node': task.node.uuid})
-
         managers = redfish_utils.get_system(task.node).managers
 
         _eject_vmedia(task, managers, sushy.VIRTUAL_MEDIA_CD)
@@ -624,6 +615,20 @@ class RedfishVirtualMediaBoot(base.BootInterface):
 
         image_utils.cleanup_iso_image(task)
 
+    def clean_up_instance(self, task):
+        """Cleans up the boot of instance.
+
+        This method cleans up the environment that was setup for booting
+        the instance.
+
+        :param task: A task from TaskManager.
+        :returns: None
+        """
+        LOG.debug("Cleaning up instance boot for "
+                  "%(node)s", {'node': task.node.uuid})
+        self._eject_all(task)
+        boot_mode_utils.deconfigure_secure_boot_if_needed(task)
+
     @classmethod
     def _set_boot_device(cls, task, device, persistent=False):
         """Set the boot device for a node.

ironic/drivers/modules/redfish/management.py

@@ -1004,3 +1004,116 @@ class RedfishManagement(base.ManagementInterface):
                       'firmware %(firmware_image)s.',
                       {'node': node.uuid,
                        'firmware_image': current_update['url']})
+
+    def get_secure_boot_state(self, task):
+        """Get the current secure boot state for the node.
+
+        :param task: A task from TaskManager.
+        :raises: MissingParameterValue if a required parameter is missing
+        :raises: RedfishError or its derivative in case of a driver
+            runtime error.
+        :raises: UnsupportedDriverExtension if secure boot is
+                 not supported by the hardware.
+        :returns: Boolean
+        """
+        system = redfish_utils.get_system(task.node)
+        try:
+            return system.secure_boot.enabled
+        except sushy.exceptions.MissingAttributeError:
+            raise exception.UnsupportedDriverExtension(
+                driver=task.node.driver, extension='get_secure_boot_state')
+
+    def set_secure_boot_state(self, task, state):
+        """Set the current secure boot state for the node.
+
+        :param task: A task from TaskManager.
+        :param state: A new state as a boolean.
+        :raises: MissingParameterValue if a required parameter is missing
+        :raises: RedfishError or its derivative in case of a driver
+            runtime error.
+        :raises: UnsupportedDriverExtension if secure boot is
+                 not supported by the hardware.
+        """
+        system = redfish_utils.get_system(task.node)
+        try:
+            sb = system.secure_boot
+        except sushy.exceptions.MissingAttributeError:
+            LOG.error('Secure boot has been requested for node %s but its '
+                      'Redfish BMC does not have a SecureBoot object',
+                      task.node.uuid)
+            raise exception.UnsupportedDriverExtension(
+                driver=task.node.driver, extension='set_secure_boot_state')
+
+        if sb.enabled == state:
+            LOG.info('Secure boot state for node %(node)s is already '
+                     '%(value)s', {'node': task.node.uuid, 'value': state})
+            return
+
+        boot_mode = system.boot.get('mode')
+        if boot_mode == sushy.BOOT_SOURCE_MODE_BIOS:
+            # NOTE(dtantsur): the case of disabling secure boot when boot mode
+            # is legacy should be covered by the check above.
+            msg = (_("Configuring secure boot requires UEFI for node %s")
+                   % task.node.uuid)
+            LOG.error(msg)
+            raise exception.RedfishError(error=msg)
+
+        try:
+            sb.set_enabled(state)
+        except sushy.exceptions.SushyError as exc:
+            msg = (_('Failed to set secure boot state on node %(node)s to '
+                     '%(value)s: %(exc)s')
+                   % {'node': task.node.uuid, 'value': state, 'exc': exc})
+            LOG.error(msg)
+            raise exception.RedfishError(error=msg)
+        else:
+            LOG.info('Secure boot state for node %(node)s has been set to '
+                     '%(value)s', {'node': task.node.uuid, 'value': state})
+
+    def _reset_keys(self, task, reset_type):
+        system = redfish_utils.get_system(task.node)
+        try:
+            sb = system.secure_boot
+        except sushy.exceptions.MissingAttributeError:
+            LOG.error('Resetting secure boot keys has been requested for node '
+                      '%s but its Redfish BMC does not have a SecureBoot '
+                      'object', task.node.uuid)
+            raise exception.UnsupportedDriverExtension(
+                driver=task.node.driver, extension='reset_keys')
+
+        try:
+            sb.reset_keys(reset_type)
+        except sushy.exceptions.SushyError as exc:
+            msg = (_('Failed to reset secure boot keys on node %(node)s: '
+                     '%(exc)s')
+                   % {'node': task.node.uuid, 'exc': exc})
+            LOG.error(msg)
+            raise exception.RedfishError(error=msg)
+
+    @METRICS.timer('RedfishManagement.reset_secure_boot_keys_to_default')
+    @base.deploy_step(priority=0)
+    @base.clean_step(priority=0)
+    def reset_secure_boot_keys_to_default(self, task):
+        """Reset secure boot keys to manufacturing defaults.
+
+        :param task: a task from TaskManager.
+        :raises: UnsupportedDriverExtension if secure boot is now supported.
+        :raises: RedfishError on runtime driver error.
+        """
+        self._reset_keys(task, sushy.SECURE_BOOT_RESET_KEYS_TO_DEFAULT)
+        LOG.info('Secure boot keys have been reset to their defaults on '
+                 'node %s', task.node.uuid)
+
+    @METRICS.timer('RedfishManagement.clear_secure_boot_keys')
+    @base.deploy_step(priority=0)
+    @base.clean_step(priority=0)
+    def clear_secure_boot_keys(self, task):
+        """Clear all secure boot keys.
+
+        :param task: a task from TaskManager.
+        :raises: UnsupportedDriverExtension if secure boot is now supported.
+        :raises: RedfishError on runtime driver error.
+        """
+        self._reset_keys(task, sushy.SECURE_BOOT_RESET_KEYS_DELETE_ALL)
+        LOG.info('Secure boot keys have been removed from node %s',
+                 task.node.uuid)

ironic/tests/unit/drivers/modules/redfish/test_boot.py

@@ -524,7 +524,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
             mock__eject_vmedia.assert_has_calls(eject_calls)
 
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
-                       'clean_up_instance', autospec=True)
+                       '_eject_all', autospec=True)
     @mock.patch.object(image_utils, 'prepare_boot_iso', autospec=True)
     @mock.patch.object(redfish_boot, '_eject_vmedia', autospec=True)
     @mock.patch.object(redfish_boot, '_insert_vmedia', autospec=True)
@@ -576,9 +576,11 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                 task, boot_devices.CDROM, persistent=True)
 
             mock_boot_mode_utils.sync_boot_mode.assert_called_once_with(task)
+            csb = mock_boot_mode_utils.configure_secure_boot_if_needed
+            csb.assert_called_once_with(task)
 
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
-                       'clean_up_instance', autospec=True)
+                       '_eject_all', autospec=True)
     @mock.patch.object(image_utils, 'prepare_configdrive_image', autospec=True)
     @mock.patch.object(image_utils, 'prepare_boot_iso', autospec=True)
     @mock.patch.object(redfish_boot, '_eject_vmedia', autospec=True)
@@ -640,7 +642,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
             mock_boot_mode_utils.sync_boot_mode.assert_called_once_with(task)
 
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
-                       'clean_up_instance', autospec=True)
+                       '_eject_all', autospec=True)
     @mock.patch.object(image_utils, 'prepare_boot_iso', autospec=True)
     @mock.patch.object(redfish_boot, '_eject_vmedia', autospec=True)
     @mock.patch.object(redfish_boot, '_insert_vmedia', autospec=True)
@@ -689,7 +691,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
             mock_boot_mode_utils.sync_boot_mode.assert_called_once_with(task)
 
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
-                       'clean_up_instance', autospec=True)
+                       '_eject_all', autospec=True)
     @mock.patch.object(image_utils, 'prepare_boot_iso', autospec=True)
     @mock.patch.object(redfish_boot, '_eject_vmedia', autospec=True)
     @mock.patch.object(redfish_boot, '_insert_vmedia', autospec=True)
@@ -731,6 +733,8 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
 
             mock_boot_mode_utils.sync_boot_mode.assert_called_once_with(task)
 
+    @mock.patch.object(boot_mode_utils, 'configure_secure_boot_if_needed',
+                       autospec=True)
     @mock.patch.object(boot_mode_utils, 'sync_boot_mode', autospec=True)
     @mock.patch.object(redfish_boot, '_eject_vmedia', autospec=True)
     @mock.patch.object(image_utils, 'cleanup_iso_image', autospec=True)
@@ -738,7 +742,8 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def _test_prepare_instance_local_boot(
             self, mock_system, mock_manager_utils,
-            mock_cleanup_iso_image, mock__eject_vmedia, mock_sync_boot_mode):
+            mock_cleanup_iso_image, mock__eject_vmedia, mock_sync_boot_mode,
+            mock_secure_boot):
 
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
@@ -755,6 +760,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                 task, mock_system.return_value.managers,
                 sushy.VIRTUAL_MEDIA_CD)
             mock_sync_boot_mode.assert_called_once_with(task)
+            mock_secure_boot.assert_called_once_with(task)
 
     def test_prepare_instance_local_whole_disk_image(self):
         self.node.driver_internal_info = {'is_whole_disk_image': True}
@@ -768,11 +774,13 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
         self.node.save()
         self._test_prepare_instance_local_boot()
 
+    @mock.patch.object(boot_mode_utils, 'deconfigure_secure_boot_if_needed',
+                       autospec=True)
     @mock.patch.object(redfish_boot, '_eject_vmedia', autospec=True)
     @mock.patch.object(image_utils, 'cleanup_iso_image', autospec=True)
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
     def _test_clean_up_instance(self, mock_system, mock_cleanup_iso_image,
-                                mock__eject_vmedia):
+                                mock__eject_vmedia, mock_secure_boot):
         managers = mock_system.return_value.managers
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
@@ -786,6 +794,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                                              sushy.VIRTUAL_MEDIA_FLOPPY))
 
             mock__eject_vmedia.assert_has_calls(eject_calls)
+            mock_secure_boot.assert_called_once_with(task)
 
     def test_clean_up_instance_only_cdrom(self):
         self._test_clean_up_instance()
@@ -797,6 +806,8 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
         self.node.save()
         self._test_clean_up_instance()
 
+    @mock.patch.object(boot_mode_utils, 'deconfigure_secure_boot_if_needed',
+                       autospec=True)
     @mock.patch.object(deploy_utils, 'get_boot_option', autospec=True)
     @mock.patch.object(redfish_boot, '_eject_vmedia', autospec=True)
     @mock.patch.object(image_utils, 'cleanup_disk_image', autospec=True)
@@ -806,7 +817,8 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                                        mock_cleanup_iso_image,
                                        mock_cleanup_disk_image,
                                        mock__eject_vmedia,
-                                       mock_get_boot_option):
+                                       mock_get_boot_option,
+                                       mock_secure_boot):
         managers = mock_system.return_value.managers
 
         with task_manager.acquire(self.context, self.node.uuid,
@@ -824,6 +836,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
             ]
 
             mock__eject_vmedia.assert_has_calls(eject_calls)
+            mock_secure_boot.assert_called_once_with(task)
 
     def test__insert_vmedia_anew(self):
 

ironic/tests/unit/drivers/modules/redfish/test_management.py

@@ -1258,3 +1258,145 @@ class RedfishManagementTestCase(db_base.DbTestCase):
                 task.node.driver_internal_info['firmware_updates'])
             task.node.save.assert_called_once_with()
             mock_node_power_action.assert_called_once_with(task, states.REBOOT)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_get_secure_boot_state(self, mock_get_system):
+        fake_system = mock_get_system.return_value
+        fake_system.secure_boot.enabled = False
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            response = task.driver.management.get_secure_boot_state(task)
+            self.assertIs(False, response)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_get_secure_boot_state_not_implemented(self, mock_get_system):
+        # Yes, seriously, that's the only way to do it.
+        class NoSecureBoot(mock.Mock):
+            @property
+            def secure_boot(self):
+                raise sushy.exceptions.MissingAttributeError("boom")
+
+        mock_get_system.return_value = NoSecureBoot()
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            self.assertRaises(exception.UnsupportedDriverExtension,
+                              task.driver.management.get_secure_boot_state,
+                              task)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_secure_boot_state(self, mock_get_system):
+        fake_system = mock_get_system.return_value
+        fake_system.secure_boot.enabled = False
+        fake_system.boot = {'mode': sushy.BOOT_SOURCE_MODE_UEFI}
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            task.driver.management.set_secure_boot_state(task, True)
+            fake_system.secure_boot.set_enabled.assert_called_once_with(True)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_secure_boot_state_boot_mode_unknown(self, mock_get_system):
+        fake_system = mock_get_system.return_value
+        fake_system.secure_boot.enabled = False
+        fake_system.boot = {}
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            task.driver.management.set_secure_boot_state(task, True)
+            fake_system.secure_boot.set_enabled.assert_called_once_with(True)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_secure_boot_state_boot_mode_no_change(self, mock_get_system):
+        fake_system = mock_get_system.return_value
+        fake_system.secure_boot.enabled = False
+        fake_system.boot = {'mode': sushy.BOOT_SOURCE_MODE_BIOS}
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            task.driver.management.set_secure_boot_state(task, False)
+            self.assertFalse(fake_system.secure_boot.set_enabled.called)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_secure_boot_state_boot_mode_incorrect(self, mock_get_system):
+        fake_system = mock_get_system.return_value
+        fake_system.secure_boot.enabled = False
+        fake_system.boot = {'mode': sushy.BOOT_SOURCE_MODE_BIOS}
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            self.assertRaisesRegex(
+                exception.RedfishError, 'requires UEFI',
+                task.driver.management.set_secure_boot_state, task, True)
+            self.assertFalse(fake_system.secure_boot.set_enabled.called)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_secure_boot_state_boot_mode_fails(self, mock_get_system):
+        fake_system = mock_get_system.return_value
+        fake_system.secure_boot.enabled = False
+        fake_system.secure_boot.set_enabled.side_effect = \
+            sushy.exceptions.SushyError
+        fake_system.boot = {'mode': sushy.BOOT_SOURCE_MODE_UEFI}
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            self.assertRaisesRegex(
+                exception.RedfishError, 'Failed to set secure boot',
+                task.driver.management.set_secure_boot_state, task, True)
+            fake_system.secure_boot.set_enabled.assert_called_once_with(True)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_set_secure_boot_state_not_implemented(self, mock_get_system):
+        # Yes, seriously, that's the only way to do it.
+        class NoSecureBoot(mock.Mock):
+            @property
+            def secure_boot(self):
+                raise sushy.exceptions.MissingAttributeError("boom")
+
+        mock_get_system.return_value = NoSecureBoot()
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            self.assertRaises(exception.UnsupportedDriverExtension,
+                              task.driver.management.set_secure_boot_state,
+                              task, True)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_reset_secure_boot_to_default(self, mock_get_system):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            task.driver.management.reset_secure_boot_keys_to_default(task)
+            sb = mock_get_system.return_value.secure_boot
+            sb.reset_keys.assert_called_once_with(
+                sushy.SECURE_BOOT_RESET_KEYS_TO_DEFAULT)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_reset_secure_boot_to_default_not_implemented(self,
+                                                          mock_get_system):
+        class NoSecureBoot(mock.Mock):
+            @property
+            def secure_boot(self):
+                raise sushy.exceptions.MissingAttributeError("boom")
+
+        mock_get_system.return_value = NoSecureBoot()
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            self.assertRaises(
+                exception.UnsupportedDriverExtension,
+                task.driver.management.reset_secure_boot_keys_to_default, task)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_clear_secure_boot(self, mock_get_system):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            task.driver.management.clear_secure_boot_keys(task)
+            sb = mock_get_system.return_value.secure_boot
+            sb.reset_keys.assert_called_once_with(
+                sushy.SECURE_BOOT_RESET_KEYS_DELETE_ALL)
+
+    @mock.patch.object(redfish_utils, 'get_system', autospec=True)
+    def test_clear_secure_boot_not_implemented(self, mock_get_system):
+        class NoSecureBoot(mock.Mock):
+            @property
+            def secure_boot(self):
+                raise sushy.exceptions.MissingAttributeError("boom")
+
+        mock_get_system.return_value = NoSecureBoot()
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            self.assertRaises(
+                exception.UnsupportedDriverExtension,
+                task.driver.management.clear_secure_boot_keys, task)

ironic/tests/unit/drivers/third_party_driver_mock_specs.py

@@ -159,7 +159,9 @@ SUSHY_SPEC = (
     'APPLY_TIME_ON_RESET',
     'TASK_STATE_COMPLETED',
     'HEALTH_OK',
-    'HEALTH_WARNING'
+    'HEALTH_WARNING',
+    'SECURE_BOOT_RESET_KEYS_TO_DEFAULT',
+    'SECURE_BOOT_RESET_KEYS_DELETE_ALL',
 )
 
 SUSHY_AUTH_SPEC = (

ironic/tests/unit/drivers/third_party_driver_mocks.py

@@ -221,7 +221,9 @@ if not sushy:
         APPLY_TIME_ON_RESET='on reset',
         TASK_STATE_COMPLETED='completed',
         HEALTH_OK='ok',
-        HEALTH_WARNING='warning'
+        HEALTH_WARNING='warning',
+        SECURE_BOOT_RESET_KEYS_TO_DEFAULT="ResetAllKeysToDefault",
+        SECURE_BOOT_RESET_KEYS_DELETE_ALL="DeleteAllKeys",
     )
 
     sys.modules['sushy'] = sushy

releasenotes/notes/redfish-secure-boot-8e3b2fcad137e31e.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - |
+    Adds support for automatically configuring secure boot for nodes using
+    the ``redfish`` management interface.