Enable agent_token for virtual media boot

This provides an embedded agent_token through out of band
means which ensures greater security for such deployments.

Also some changes to the redfish virtual media boot unit
tests as the entirety of conductor utilties was mocked.

Downside with the pregenerated token, the logic had to be
slightly revised as the existence of a token with an older
client is a case that virtual media deployments will hit.

This was addressed, and tests updated as a result.

Story: 2007025
Task: 37819
Change-Id: Iaa2a52c2e53534cbf6998ad24f1f1184c0f222d8

ironic/conductor/manager.py

@@ -2959,7 +2959,8 @@ class ConductorManager(base_manager.BaseConductorManager):
             # either tokens are required and they are present,
             # or a token is present in general and needs to be
             # validated.
-            if token_required or utils.is_agent_token_present(task.node):
+            if (token_required
+                or (utils.is_agent_token_present(task.node) and agent_token)):
                 if not utils.is_agent_token_valid(task.node, agent_token):
                     LOG.error('Invalid agent_token receieved for node '
                               '%(node)s', {'node': node_id})

ironic/conductor/utils.py

@@ -1010,13 +1010,22 @@ def get_node_next_deploy_steps(task, skip_current_step=True):
                                 skip_current_step=skip_current_step)
 
 
-def add_secret_token(node):
+def add_secret_token(node, pregenerated=False):
-    """Adds a secret token to driver_internal_info for IPA verification."""
+    """Adds a secret token to driver_internal_info for IPA verification.
+
+    :param node: Node object
+    :param pregenerated: Boolean value, default False, which indicates if
+                         the token should be marked as "pregenerated" in
+                         order to facilitate virtual media booting where
+                         the token is embedded into the configuration.
+    """
     characters = string.ascii_letters + string.digits
     token = ''.join(
         random.SystemRandom().choice(characters) for i in range(128))
     i_info = node.driver_internal_info
     i_info['agent_secret_token'] = token
+    if pregenerated:
+        i_info['agent_secret_token_pregenerated'] = True
     node.driver_internal_info = i_info
 
 

ironic/drivers/modules/ilo/boot.py

@@ -509,6 +509,13 @@ class IloVirtualMediaBoot(base.BootInterface):
         # during boot.
         ilo_common.eject_vmedia_devices(task)
 
+        # NOTE(TheJulia): Since we're deploying, cleaning, or rescuing,
+        # with virtual media boot, we should generate a token!
+        manager_utils.add_secret_token(task.node, pregenerated=True)
+        ramdisk_params['ipa-agent-token'] = \
+            task.node.driver_internal_info['agent_secret_token']
+        task.node.save()
+
         deploy_nic_mac = deploy_utils.get_single_nic_with_vif_port_id(task)
         ramdisk_params['BOOTIF'] = deploy_nic_mac
         if node.provision_state == states.RESCUING:

ironic/drivers/modules/irmc/boot.py

@@ -990,6 +990,13 @@ class IRMCVirtualMediaBoot(base.BootInterface, IRMCVolumeBootMixIn):
                           {'node': task.node.uuid})
                 return
 
+        # NOTE(TheJulia): Since we're deploying, cleaning, or rescuing,
+        # with virtual media boot, we should generate a token!
+        manager_utils.add_secret_token(task.node, pregenerated=True)
+        ramdisk_params['ipa-agent-token'] = \
+            task.node.driver_internal_info['agent_secret_token']
+        task.node.save()
+
         deploy_nic_mac = deploy_utils.get_single_nic_with_vif_port_id(task)
         ramdisk_params['BOOTIF'] = deploy_nic_mac
 

ironic/drivers/modules/redfish/boot.py

@@ -682,6 +682,13 @@ class RedfishVirtualMediaBoot(base.BootInterface):
                                         states.INSPECTING):
             return
 
+        # NOTE(TheJulia): Since we're deploying, cleaning, or rescuing,
+        # with virtual media boot, we should generate a token!
+        manager_utils.add_secret_token(node, pregenerated=True)
+        node.save()
+        ramdisk_params['ipa-agent-token'] = \
+            node.driver_internal_info['agent_secret_token']
+
         manager_utils.node_power_action(task, states.POWER_OFF)
 
         d_info = self._parse_driver_info(node)

ironic/tests/unit/conductor/test_manager.py

@@ -7082,6 +7082,30 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
         mock_heartbeat.assert_called_with(mock.ANY, mock.ANY,
                                           'http://callback', '1.4.1')
 
+    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.heartbeat',
+                autospec=True)
+    @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker',
+                autospec=True)
+    def test_heartbeat_with_agent_pregenerated_token(
+            self, mock_spawn, mock_heartbeat):
+        """Test heartbeating."""
+        node = obj_utils.create_test_node(
+            self.context, driver='fake-hardware',
+            provision_state=states.DEPLOYING,
+            target_provision_state=states.ACTIVE,
+            driver_internal_info={'agent_secret_token': 'a secret'})
+
+        self._start_service()
+
+        mock_spawn.reset_mock()
+
+        mock_spawn.side_effect = self._fake_spawn
+        self.service.heartbeat(
+            self.context, node.uuid, 'http://callback', '6.0.1',
+            agent_token=None)
+        mock_heartbeat.assert_called_with(mock.ANY, mock.ANY,
+                                          'http://callback', '6.0.1')
+
     @mock.patch('ironic.drivers.modules.fake.FakeDeploy.heartbeat',
                 autospec=True)
     @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker',
@@ -7209,32 +7233,6 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
                           agent_token='evil', agent_version='4.0.0')
         self.assertFalse(mock_heartbeat.called)
 
-    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.heartbeat',
-                autospec=True)
-    @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker',
-                autospec=True)
-    def test_heartbeat_invalid_when_token_on_file_older_agent(
-            self, mock_spawn, mock_heartbeat):
-        """Heartbeat rejected if a token is on file."""
-        self.config(require_agent_token=False)
-        node = obj_utils.create_test_node(
-            self.context, driver='fake-hardware',
-            provision_state=states.DEPLOYING,
-            target_provision_state=states.ACTIVE,
-            driver_internal_info={'agent_secret_token': 'a secret'})
-
-        self._start_service()
-
-        mock_spawn.reset_mock()
-
-        mock_spawn.side_effect = self._fake_spawn
-
-        self.assertRaises(exception.InvalidParameterValue,
-                          self.service.heartbeat, self.context,
-                          node.uuid, 'http://callback',
-                          agent_token=None, agent_version='4.0.0')
-        self.assertFalse(mock_heartbeat.called)
-
     @mock.patch('ironic.drivers.modules.fake.FakeDeploy.heartbeat',
                 autospec=True)
     @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker',

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

@@ -821,7 +821,8 @@ class IloVirtualMediaBootTestCase(test_common.BaseIloTest):
 
             prepare_node_for_deploy_mock.assert_called_once_with(task)
             eject_mock.assert_called_once_with(task)
-            expected_ramdisk_opts = {'a': 'b', 'BOOTIF': '12:34:56:78:90:ab'}
+            expected_ramdisk_opts = {'a': 'b', 'BOOTIF': '12:34:56:78:90:ab',
+                                     'ipa-agent-token': mock.ANY}
             get_nic_mock.assert_called_once_with(task)
             setup_vmedia_mock.assert_called_once_with(task, iso,
                                                       expected_ramdisk_opts)

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

@@ -990,7 +990,9 @@ class IRMCVirtualMediaBootTestCase(test_common.BaseIRMCTest):
                                   shared=False) as task:
             task.driver.boot.prepare_ramdisk(task, ramdisk_params)
 
-            expected_ramdisk_opts = {'a': 'b', 'BOOTIF': '12:34:56:78:90:ab'}
+            expected_ramdisk_opts = {'a': 'b', 'BOOTIF': '12:34:56:78:90:ab',
+                                     'ipa-agent-token': mock.ANY}
+
             get_single_nic_with_vif_port_id_mock.assert_called_once_with(
                 task)
             _setup_vmedia_mock.assert_called_once_with(

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

@@ -552,6 +552,8 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
             self.assertRaises(exception.UnsupportedDriverExtension,
                               task.driver.boot.validate_inspection, task)
 
+    @mock.patch.object(redfish_boot.manager_utils, 'node_set_boot_device',
+                       autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
                        '_prepare_deploy_iso', autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
@@ -560,15 +562,16 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                        '_insert_vmedia', autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
                        '_parse_driver_info', autospec=True)
-    @mock.patch.object(redfish_boot, 'manager_utils', autospec=True)
+    @mock.patch.object(redfish_boot.manager_utils, 'node_power_action',
+                       autospec=True)
     @mock.patch.object(redfish_boot, 'boot_mode_utils', autospec=True)
     def test_prepare_ramdisk_with_params(
-            self, mock_boot_mode_utils, mock_manager_utils,
+            self, mock_boot_mode_utils, mock_node_power_action,
             mock__parse_driver_info, mock__insert_vmedia, mock__eject_vmedia,
-            mock__prepare_deploy_iso):
+            mock__prepare_deploy_iso, mock_node_set_boot_device):
 
         with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=True) as task:
+                                  shared=False) as task:
             task.node.provision_state = states.DEPLOYING
 
             mock__parse_driver_info.return_value = {}
@@ -576,7 +579,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
 
             task.driver.boot.prepare_ramdisk(task, {})
 
-            mock_manager_utils.node_power_action.assert_called_once_with(
+            mock_node_power_action.assert_called_once_with(
                 task, states.POWER_OFF)
 
             mock__eject_vmedia.assert_called_once_with(
@@ -587,17 +590,20 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
 
             expected_params = {
                 'BOOTIF': None,
+                'ipa-agent-token': mock.ANY,
                 'ipa-debug': '1',
             }
 
             mock__prepare_deploy_iso.assert_called_once_with(
                 task, expected_params, 'deploy')
 
-            mock_manager_utils.node_set_boot_device.assert_called_once_with(
+            mock_node_set_boot_device.assert_called_once_with(
                 task, boot_devices.CDROM, False)
 
             mock_boot_mode_utils.sync_boot_mode.assert_called_once_with(task)
 
+    @mock.patch.object(redfish_boot.manager_utils, 'node_set_boot_device',
+                       autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
                        '_prepare_deploy_iso', autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
@@ -606,12 +612,13 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                        '_insert_vmedia', autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
                        '_parse_driver_info', autospec=True)
-    @mock.patch.object(redfish_boot, 'manager_utils', autospec=True)
+    @mock.patch.object(redfish_boot.manager_utils, 'node_power_action',
+                       autospec=True)
     @mock.patch.object(redfish_boot, 'boot_mode_utils', autospec=True)
     def test_prepare_ramdisk_no_debug(
-            self, mock_boot_mode_utils, mock_manager_utils,
+            self, mock_boot_mode_utils, mock_node_power_action,
             mock__parse_driver_info, mock__insert_vmedia, mock__eject_vmedia,
-            mock__prepare_deploy_iso):
+            mock__prepare_deploy_iso, mock_node_set_boot_device):
         self.config(debug=False)
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
@@ -622,7 +629,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
 
             task.driver.boot.prepare_ramdisk(task, {})
 
-            mock_manager_utils.node_power_action.assert_called_once_with(
+            mock_node_power_action.assert_called_once_with(
                 task, states.POWER_OFF)
 
             mock__eject_vmedia.assert_called_once_with(
@@ -633,16 +640,19 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
 
             expected_params = {
                 'BOOTIF': None,
+                'ipa-agent-token': mock.ANY,
             }
 
             mock__prepare_deploy_iso.assert_called_once_with(
                 task, expected_params, 'deploy')
 
-            mock_manager_utils.node_set_boot_device.assert_called_once_with(
+            mock_node_set_boot_device.assert_called_once_with(
                 task, boot_devices.CDROM, False)
 
             mock_boot_mode_utils.sync_boot_mode.assert_called_once_with(task)
 
+    @mock.patch.object(redfish_boot.manager_utils, 'node_set_boot_device',
+                       autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
                        '_prepare_floppy_image', autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
@@ -655,16 +665,17 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                        '_insert_vmedia', autospec=True)
     @mock.patch.object(redfish_boot.RedfishVirtualMediaBoot,
                        '_parse_driver_info', autospec=True)
-    @mock.patch.object(redfish_boot, 'manager_utils', autospec=True)
+    @mock.patch.object(redfish_boot.manager_utils, 'node_power_action',
+                       autospec=True)
     @mock.patch.object(redfish_boot, 'boot_mode_utils', autospec=True)
     def test_prepare_ramdisk_with_floppy(
-            self, mock_boot_mode_utils, mock_manager_utils,
+            self, mock_boot_mode_utils, mock_node_power_action,
             mock__parse_driver_info, mock__insert_vmedia, mock__eject_vmedia,
             mock__has_vmedia_device, mock__prepare_deploy_iso,
-            mock__prepare_floppy_image):
+            mock__prepare_floppy_image, mock_node_set_boot_device):
 
         with task_manager.acquire(self.context, self.node.uuid,
-                                  shared=True) as task:
+                                  shared=False) as task:
             task.node.provision_state = states.DEPLOYING
 
             mock__parse_driver_info.return_value = {
@@ -677,7 +688,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
 
             task.driver.boot.prepare_ramdisk(task, {})
 
-            mock_manager_utils.node_power_action.assert_called_once_with(
+            mock_node_power_action.assert_called_once_with(
                 task, states.POWER_OFF)
 
             mock__has_vmedia_device.assert_called_once_with(
@@ -703,12 +714,13 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                 'BOOTIF': None,
                 'boot_method': 'vmedia',
                 'ipa-debug': '1',
+                'ipa-agent-token': mock.ANY,
             }
 
             mock__prepare_deploy_iso.assert_called_once_with(
                 task, expected_params, 'deploy')
 
-            mock_manager_utils.node_set_boot_device.assert_called_once_with(
+            mock_node_set_boot_device.assert_called_once_with(
                 task, boot_devices.CDROM, False)
 
             mock_boot_mode_utils.sync_boot_mode.assert_called_once_with(task)