Remove token-less agent support

Removes the deprecated support for token-less agents which
better secures the ironic-python-agent<->ironic interactions
to help ensure heartbeat operations are coming from the same
node which originally checked-in with the Ironic and that
commands coming to an agent are originating from the same
ironic deployment which the agent checked-in with to begin
with.

Story: 2007025
Task: 40814
Change-Id: Id7a3f402285c654bc4665dcd45bd0730128bf9b0

8 files changed, 46 insertions, 150 deletions

diff --git a/ironic/api/controllers/v1/ramdisk.py b/ironic/api/controllers/v1/ramdisk.py
index a79b070fa..787955eea 100644
--- a/ironic/api/controllers/v1/ramdisk.py
+++ b/ironic/api/controllers/v1/ramdisk.py
@@ -54,9 +54,10 @@ def config(token):
         },
         'heartbeat_timeout': CONF.api.ramdisk_heartbeat_timeout,
         'agent_token': token,
-        # Not an API version based indicator, passing as configuration
-        # as the signifigants indicates support should also be present.
-        'agent_token_required': CONF.require_agent_token,
+        # Since this is for the Victoria release, we send this as an
+        # explicit True statement for newer agents to lock the setting
+        # and behavior into place.
+        'agent_token_required': True,
     }
 
 
@@ -154,7 +155,7 @@ class LookupController(rest.RestController):
                 and node.provision_state not in self.lookup_allowed_states):
             raise exception.NotFound()
 
-        if api_utils.allow_agent_token() or CONF.require_agent_token:
+        if api_utils.allow_agent_token():
             try:
                 topic = api.request.rpcapi.get_topic_for(node)
             except exception.NoValidHost as e:
@@ -216,8 +217,7 @@ class HeartbeatController(rest.RestController):
                   '"callback_url"'))
         # NOTE(TheJulia): If tokens are required, lets go ahead and fail the
         # heartbeat very early on.
-        token_required = CONF.require_agent_token
-        if token_required and agent_token is None:
+        if agent_token is None:
             LOG.error('Agent heartbeat received for node %(node)s '
                       'without an agent token.', {'node': node_ident})
             raise exception.InvalidParameterValue(
diff --git a/ironic/cmd/conductor.py b/ironic/cmd/conductor.py
index 4164db18a..d2ee20f5f 100644
--- a/ironic/cmd/conductor.py
+++ b/ironic/cmd/conductor.py
@@ -50,20 +50,8 @@ def warn_about_unsafe_shred_parameters(conf):
                     'Secure Erase. This is a possible SECURITY ISSUE!')
 
 
-def warn_about_agent_token_deprecation(conf):
-    if not conf.require_agent_token:
-        LOG.warning('The ``[DEFAULT]require_agent_token`` option is not '
-                    'set and support for ironic-python-agents that do not '
-                    'utilize agent tokens, along with the configuration '
-                    'option will be removed in the W development cycle. '
-                    'Please upgrade your ironic-python-agent version, and '
-                    'consider adopting the require_agent_token setting '
-                    'during the Victoria development cycle.')
-
-
 def issue_startup_warnings(conf):
     warn_about_unsafe_shred_parameters(conf)
-    warn_about_agent_token_deprecation(conf)
 
 
 def main():
diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index cb20fcfd8..b4959abdf 100644
--- a/ironic/conductor/manager.py
+++ b/ironic/conductor/manager.py
@@ -3120,8 +3120,6 @@ class ConductorManager(base_manager.BaseConductorManager):
         if agent_version is None:
             agent_version = '3.0.0'
 
-        token_required = CONF.require_agent_token
-
         # NOTE(dtantsur): we acquire a shared lock to begin with, drivers are
         # free to promote it to an exclusive one.
         with task_manager.acquire(context, node_id, shared=True,
@@ -3131,32 +3129,11 @@ 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) 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})
-                    raise exception.InvalidParameterValue(
-                        'Invalid or missing agent token received.')
-            elif utils.is_agent_token_supported(agent_version):
-                LOG.error('Suspicious activity detected for node %(node)s '
-                          'when attempting to heartbeat. Heartbeat '
-                          'request has been rejected as the version of '
-                          'ironic-python-agent indicated in the heartbeat '
-                          'operation should support agent token '
-                          'functionality.',
-                          {'node': task.node.uuid})
+            if not utils.is_agent_token_valid(task.node, agent_token):
+                LOG.error('Invalid or missing agent_token receieved for '
+                          'node %(node)s', {'node': node_id})
                 raise exception.InvalidParameterValue(
                     'Invalid or missing agent token received.')
-            else:
-                LOG.warning('Out of date agent detected for node '
-                            '%(node)s. Agent version %(version)s '
-                            'reported. Support for this version is '
-                            'deprecated.',
-                            {'node': task.node.uuid,
-                             'version': agent_version})
-                # TODO(TheJulia): raise an exception as of the
-                # ?Victoria? development cycle.
 
             task.spawn_after(
                 self._spawn_worker, task.driver.deploy.heartbeat,
diff --git a/ironic/conf/default.py b/ironic/conf/default.py
index b799208f0..ffe2b2040 100644
--- a/ironic/conf/default.py
+++ b/ironic/conf/default.py
@@ -348,16 +348,6 @@ service_opts = [
                         ('json-rpc', _('use JSON RPC transport'))],
                help=_('Which RPC transport implementation to use between '
                       'conductor and API services')),
-    cfg.BoolOpt('require_agent_token',
-                default=False,
-                mutable=True,
-                help=_('Used to require the use of agent tokens. These '
-                       'tokens are used to guard the api lookup endpoint and '
-                       'conductor heartbeat processing logic to authenticate '
-                       'transactions with the ironic-python-agent. Tokens '
-                       'are provided only upon the first lookup of a node '
-                       'and may be provided via out of band means through '
-                       'the use of virtual media.')),
 ]
 
 utils_opts = [
diff --git a/ironic/drivers/modules/agent_client.py b/ironic/drivers/modules/agent_client.py
index 4cb349509..74f28581c 100644
--- a/ironic/drivers/modules/agent_client.py
+++ b/ironic/drivers/modules/agent_client.py
@@ -209,30 +209,19 @@ class AgentClient(object):
                    'code': response.status_code})
         if response.status_code >= http_client.BAD_REQUEST:
             faultstring = result.get('faultstring')
-            if 'agent_token' in faultstring and agent_token:
-                # NOTE(TheJulia) We have an agent that is out of date.
-                # which means I guess grenade updates the agent image
-                # for upgrades... :(
-                if not CONF.require_agent_token:
-                    LOG.warning('Agent command %(method)s for node %(node)s '
-                                'failed. Expected 2xx HTTP status code, got '
-                                '%(code)d. Error suggests an older ramdisk '
-                                'which does not support ``agent_token``. '
-                                'Removing the token for the next retry.',
-                                {'method': method, 'node': node.uuid,
-                                 'code': response.status_code})
-                    i_info = node.driver_internal_info
-                    i_info.pop('agent_secret_token')
-                    node.driver_internal_info = i_info
-                    node.save()
-                    msg = ('Node {} does not appear to support '
-                           'agent_token and it is not required. Next retry '
-                           'will be without the token.').format(node.uuid)
-                    raise exception.AgentConnectionFailed(reason=msg)
-            LOG.error('Agent command %(method)s for node %(node)s failed. '
-                      'Expected 2xx HTTP status code, got %(code)d.',
-                      {'method': method, 'node': node.uuid,
-                       'code': response.status_code})
+            if 'agent_token' in faultstring:
+                LOG.error('Agent command %(method)s for node %(node)s '
+                          'failed. Expected 2xx HTTP status code, got '
+                          '%(code)d. Error suggests an older ramdisk '
+                          'which does not support ``agent_token``. '
+                          'This is a fatal error.',
+                          {'method': method, 'node': node.uuid,
+                           'code': response.status_code})
+            else:
+                LOG.error('Agent command %(method)s for node %(node)s failed. '
+                          'Expected 2xx HTTP status code, got %(code)d.',
+                          {'method': method, 'node': node.uuid,
+                           'code': response.status_code})
             raise exception.AgentAPIError(node=node.uuid,
                                           status=response.status_code,
                                           error=faultstring)
diff --git a/ironic/tests/unit/api/controllers/v1/test_ramdisk.py b/ironic/tests/unit/api/controllers/v1/test_ramdisk.py
index 1b233fc70..d35a864ef 100644
--- a/ironic/tests/unit/api/controllers/v1/test_ramdisk.py
+++ b/ironic/tests/unit/api/controllers/v1/test_ramdisk.py
@@ -77,7 +77,7 @@ class TestLookup(test_api_base.BaseApiTest):
             },
             'heartbeat_timeout': CONF.api.ramdisk_heartbeat_timeout,
             'agent_token': mock.ANY,
-            'agent_token_required': False,
+            'agent_token_required': True,
         }
         self.assertEqual(expected_config, data['config'])
         self.assertIsNotNone(data['config']['agent_token'])
@@ -218,12 +218,13 @@ class TestHeartbeat(test_api_base.BaseApiTest):
         node = obj_utils.create_test_node(self.context)
         response = self.post_json(
             '/heartbeat/%s' % node.uuid,
-            {'callback_url': 'url'},
+            {'callback_url': 'url',
+             'agent_token': 'x'},
             headers={api_base.Version.string: str(api_v1.max_version())})
         self.assertEqual(http_client.ACCEPTED, response.status_int)
         self.assertEqual(b'', response.body)
         mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
-                                               node.uuid, 'url', None, None,
+                                               node.uuid, 'url', None, 'x',
                                                topic='test-topic')
 
     @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True)
@@ -231,12 +232,14 @@ class TestHeartbeat(test_api_base.BaseApiTest):
         node = obj_utils.create_test_node(self.context)
         response = self.post_json(
             '/heartbeat/%s.json' % node.uuid,
-            {'callback_url': 'url'},
+            {'callback_url': 'url',
+             'agent_token': 'maybe some magic'},
             headers={api_base.Version.string: str(api_v1.max_version())})
         self.assertEqual(http_client.ACCEPTED, response.status_int)
         self.assertEqual(b'', response.body)
         mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
-                                               node.uuid, 'url', None, None,
+                                               node.uuid, 'url', None,
+                                               'maybe some magic',
                                                topic='test-topic')
 
     @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True)
@@ -244,12 +247,14 @@ class TestHeartbeat(test_api_base.BaseApiTest):
         node = obj_utils.create_test_node(self.context, name='test.1')
         response = self.post_json(
             '/heartbeat/%s' % node.name,
-            {'callback_url': 'url'},
+            {'callback_url': 'url',
+             'agent_token': 'token'},
             headers={api_base.Version.string: str(api_v1.max_version())})
         self.assertEqual(http_client.ACCEPTED, response.status_int)
         self.assertEqual(b'', response.body)
         mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
-                                               node.uuid, 'url', None, None,
+                                               node.uuid, 'url', None,
+                                               'token',
                                                topic='test-topic')
 
     @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True)
@@ -258,12 +263,14 @@ class TestHeartbeat(test_api_base.BaseApiTest):
         response = self.post_json(
             '/heartbeat/%s' % node.uuid,
             {'callback_url': 'url',
-             'agent_version': '1.4.1'},
+             'agent_version': '1.4.1',
+             'agent_token': 'meow'},
             headers={api_base.Version.string: str(api_v1.max_version())})
         self.assertEqual(http_client.ACCEPTED, response.status_int)
         self.assertEqual(b'', response.body)
         mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
-                                               node.uuid, 'url', '1.4.1', None,
+                                               node.uuid, 'url', '1.4.1',
+                                               'meow',
                                                topic='test-topic')
 
     @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True)
diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py
index 7ab03e175..994ba78b9 100644
--- a/ironic/tests/unit/conductor/test_manager.py
+++ b/ironic/tests/unit/conductor/test_manager.py
@@ -7221,7 +7221,8 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
-            target_provision_state=states.ACTIVE)
+            target_provision_state=states.ACTIVE,
+            driver_internal_info={'agent_secret_token': 'magic'})
 
         self._start_service()
 
@@ -7229,7 +7230,8 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
 
         mock_spawn.side_effect = self._fake_spawn
 
-        self.service.heartbeat(self.context, node.uuid, 'http://callback')
+        self.service.heartbeat(self.context, node.uuid, 'http://callback',
+                               agent_token='magic')
         mock_heartbeat.assert_called_with(mock.ANY, mock.ANY,
                                           'http://callback', '3.0.0')
 
@@ -7242,7 +7244,8 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
-            target_provision_state=states.ACTIVE)
+            target_provision_state=states.ACTIVE,
+            driver_internal_info={'agent_secret_token': 'magic'})
 
         self._start_service()
 
@@ -7250,8 +7253,8 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
 
         mock_spawn.side_effect = self._fake_spawn
 
-        self.service.heartbeat(
-            self.context, node.uuid, 'http://callback', '1.4.1')
+        self.service.heartbeat(self.context, node.uuid, 'http://callback',
+                               '1.4.1', agent_token='magic')
         mock_heartbeat.assert_called_with(mock.ANY, mock.ANY,
                                           'http://callback', '1.4.1')
 
@@ -7259,34 +7262,9 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
                 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',
-                autospec=True)
     def test_heartbeat_with_no_required_agent_token(self, mock_spawn,
                                                     mock_heartbeat):
         """Tests that we kill the heartbeat attempt very early on."""
-        self.config(require_agent_token=True)
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
@@ -7311,7 +7289,6 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
     def test_heartbeat_with_required_agent_token(self, mock_spawn,
                                                  mock_heartbeat):
         """Test heartbeat works when token matches."""
-        self.config(require_agent_token=True)
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
@@ -7336,7 +7313,6 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
     def test_heartbeat_with_agent_token(self, mock_spawn,
                                         mock_heartbeat):
         """Test heartbeat works when token matches."""
-        self.config(require_agent_token=False)
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
@@ -7361,7 +7337,6 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
     def test_heartbeat_invalid_agent_token(self, mock_spawn,
                                            mock_heartbeat):
         """Heartbeat fails when it does not match."""
-        self.config(require_agent_token=False)
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
@@ -7388,7 +7363,6 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
     def test_heartbeat_invalid_agent_token_older_version(
             self, mock_spawn, mock_heartbeat):
         """Heartbeat is rejected if token is received that is invalid."""
-        self.config(require_agent_token=False)
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
@@ -7416,7 +7390,6 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
     def test_heartbeat_invalid_newer_version(
             self, mock_spawn, mock_heartbeat):
         """Heartbeat rejected if client should be sending a token."""
-        self.config(require_agent_token=False)
         node = obj_utils.create_test_node(
             self.context, driver='fake-hardware',
             provision_state=states.DEPLOYING,
diff --git a/ironic/tests/unit/drivers/modules/test_agent_client.py b/ironic/tests/unit/drivers/modules/test_agent_client.py
index cb794c94c..343010534 100644
--- a/ironic/tests/unit/drivers/modules/test_agent_client.py
+++ b/ironic/tests/unit/drivers/modules/test_agent_client.py
@@ -616,35 +616,7 @@ class TestAgentClientAttempts(base.TestCase):
             verify=True)
 
     @mock.patch.object(retrying.time, 'sleep', autospec=True)
-    def test__command_succeed_after_agent_token(self, mock_sleep):
-        self.config(require_agent_token=False)
-        mock_sleep.return_value = None
-        error = 'Unknown Argument: "agent_token"'
-        response_data = {'status': 'ok'}
-        method = 'standby.run_image'
-        image_info = {'image_id': 'test_image'}
-        params = {'image_info': image_info}
-        i_info = self.node.driver_internal_info
-        i_info['agent_secret_token'] = 'meowmeowmeow'
-        self.client.session.post.side_effect = [
-            MockFault(error),
-            MockResponse(response_data),
-        ]
-
-        response = self.client._command(self.node, method, params)
-        self.assertEqual(2, self.client.session.post.call_count)
-        self.assertEqual(response, response_data)
-        self.client.session.post.assert_called_with(
-            self.client._get_command_url(self.node),
-            data=self.client._get_command_body(method, params),
-            params={'wait': 'false'},
-            timeout=60,
-            verify=True)
-        self.assertNotIn('agent_secret_token', self.node.driver_internal_info)
-
-    @mock.patch.object(retrying.time, 'sleep', autospec=True)
     def test__command_fail_agent_token_required(self, mock_sleep):
-        self.config(require_agent_token=True)
         mock_sleep.return_value = None
         error = 'Unknown Argument: "agent_token"'
         method = 'standby.run_image'