Merge "Remove token-less agent support"

devstack/lib/ironic

@@ -550,9 +550,6 @@ IRONIC_DEPLOY_LOGS_LOCAL_PATH=${IRONIC_DEPLOY_LOGS_LOCAL_PATH:-$IRONIC_VM_LOG_DI
 # Fast track option
 IRONIC_DEPLOY_FAST_TRACK=${IRONIC_DEPLOY_FAST_TRACK:-False}
 
-# Agent Token requirement
-IRONIC_REQUIRE_AGENT_TOKEN=${IRONIC_REQUIRE_AGENT_TOKEN:-True}
-
 # Define baremetal min_microversion in tempest config. Default value None is picked from tempest.
 TEMPEST_BAREMETAL_MIN_MICROVERSION=${TEMPEST_BAREMETAL_MIN_MICROVERSION:-}
 
@@ -1426,8 +1423,6 @@ function configure_ironic {
     # Set fast track options
     iniset $IRONIC_CONF_FILE deploy fast_track $IRONIC_DEPLOY_FAST_TRACK
 
-    # Set requirement for agent tokens
-    iniset $IRONIC_CONF_FILE DEFAULT require_agent_token $IRONIC_REQUIRE_AGENT_TOKEN
     # No need to check if RabbitMQ is enabled, this call does it in a smart way
     if [[ "$IRONIC_RPC_TRANSPORT" == "oslo" ]]; then
         iniset_rpc_backend ironic $IRONIC_CONF_FILE

doc/source/admin/agent-token.rst

@@ -29,7 +29,8 @@ These tokens are provided in one of two ways to the running agent.
 2. A one-time generated token that are provided upon the first "lookup"
    of the node.
 
-In both cases, the tokens are a randomly generated length of 128 characters.
+In both cases, the tokens are a randomly generated using the Python
+``secrets`` library. As of mid-2020, the default length is 43 characters.
 
 Once the token has been provided, the token cannot be retrieved or accessed.
 It remains available to the conductors, and is stored in memory of the
@@ -43,9 +44,10 @@ It remains available to the conductors, and is stored in memory of the
 With the token is available in memory in the agent, the token is embedded with
 ``heartbeat`` operations to the ironic API endpoint. This enables the API to
 authenticate the heartbeat request, and refuse "heartbeat" requests from the
-``ironic-python-agent``. With the ``Ussuri`` release, the configuration option
+``ironic-python-agent``. As of the Victoria release, use of Agent Token is
-``[DEFAULT]require_agent_token`` can be set ``True`` to explicitly require
+required for all agents and the previously available setting to force this
-token use.
+functionality to be manditory, ``[DEFAULT]require_agent_token`` no longer has
+any effect.
 
 .. warning::
    If the Bare Metal Service is updated, and the version of

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
+        # Since this is for the Victoria release, we send this as an
-        # as the signifigants indicates support should also be present.
+        # explicit True statement for newer agents to lock the setting
-        'agent_token_required': CONF.require_agent_token,
+        # 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 agent_token is None:
-        if token_required and agent_token is None:
             LOG.error('Agent heartbeat received for node %(node)s '
                       'without an agent token.', {'node': node_ident})
             raise exception.InvalidParameterValue(

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

ironic/conductor/manager.py

@@ -3121,8 +3121,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,
@@ -3132,32 +3130,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
+            if not utils.is_agent_token_valid(task.node, agent_token):
-                or (utils.is_agent_token_present(task.node) and agent_token)):
+                LOG.error('Invalid or missing agent_token receieved for '
-                if not utils.is_agent_token_valid(task.node, agent_token):
+                          'node %(node)s', {'node': node_id})
-                    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})
                 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,

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 = [

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:
+            if 'agent_token' in faultstring:
-                # NOTE(TheJulia) We have an agent that is out of date.
+                LOG.error('Agent command %(method)s for node %(node)s '
-                # which means I guess grenade updates the agent image
+                          'failed. Expected 2xx HTTP status code, got '
-                # for upgrades... :(
+                          '%(code)d. Error suggests an older ramdisk '
-                if not CONF.require_agent_token:
+                          'which does not support ``agent_token``. '
-                    LOG.warning('Agent command %(method)s for node %(node)s '
+                          'This is a fatal error.',
-                                'failed. Expected 2xx HTTP status code, got '
+                          {'method': method, 'node': node.uuid,
-                                '%(code)d. Error suggests an older ramdisk '
+                           'code': response.status_code})
-                                'which does not support ``agent_token``. '
+            else:
-                                'Removing the token for the next retry.',
+                LOG.error('Agent command %(method)s for node %(node)s failed. '
-                                {'method': method, 'node': node.uuid,
+                          'Expected 2xx HTTP status code, got %(code)d.',
-                                 'code': response.status_code})
+                          {'method': method, 'node': node.uuid,
-                    i_info = node.driver_internal_info
+                           'code': response.status_code})
-                    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})
             raise exception.AgentAPIError(node=node.uuid,
                                           status=response.status_code,
                                           error=faultstring)

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)

ironic/tests/unit/conductor/test_manager.py

@@ -7244,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()
 
@@ -7252,7 +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')
+        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')
 
@@ -7265,7 +7267,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()
 
@@ -7273,35 +7276,11 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
 
         mock_spawn.side_effect = self._fake_spawn
 
-        self.service.heartbeat(
+        self.service.heartbeat(self.context, node.uuid, 'http://callback',
-            self.context, node.uuid, 'http://callback', '1.4.1')
+                               '1.4.1', agent_token='magic')
         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',
@@ -7309,7 +7288,6 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
     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,
@@ -7334,7 +7312,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,
@@ -7359,7 +7336,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,
@@ -7384,7 +7360,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,
@@ -7411,7 +7386,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,
@@ -7439,7 +7413,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,

ironic/tests/unit/drivers/modules/test_agent_client.py

@@ -615,36 +615,8 @@ class TestAgentClientAttempts(base.TestCase):
             timeout=60,
             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'

releasenotes/notes/no-tokenless-agents-c6c16d79ccc0da7a.yaml

@@ -0,0 +1,20 @@
+---
+upgrade:
+  - |
+    Support for token-less agents has been removed as the token-less agent
+    support was deprecated in the Ussuri development cycle. The ironic-python-agent
+    must be updated to 6.1.0 or higher to support communicating with the
+    Ironic deployment after upgrade. This will generally require deployment,
+    cleaning, and rescue kernels and ramdisks to be updated. If this is not
+    done, actions such as cleaning and deployment will time out as the agent
+    will be unable to record heartbeats with Ironic. For more information,
+    please see the `agent token <https://docs.openstack.org/ironic/latest/admin/agent-token.html>`_
+    documentation.
+security:
+  - |
+    Ramdisks supporting agent token are now globally required by Ironic.
+    As this is a core security mechanism, it cannot be disabled and support
+    for the ``[DEFAULT]require_agent_token`` configuration parameter has been
+    removed as tokens are now always required by Ironic. For more information,
+    please see the `agent token <https://docs.openstack.org/ironic/latest/admin/agent-token.html>`_
+    documentation.

zuul.d/ironic-jobs.yaml

@@ -822,7 +822,6 @@
           IRONIC_VM_SPECS_RAM: 512
           IRONIC_DEFAULT_BOOT_OPTION: netboot
           IRONIC_AUTOMATED_CLEAN_ENABLED: False
-          IRONIC_REQUIRE_AGENT_TOKEN: False
           Q_AGENT: openvswitch
           Q_ML2_TENANT_NETWORK_TYPE: vxlan
           EBTABLES_RACE_FIX: True