From 5b272b0c46f5a10c50fc7325cc653fd577908ca0 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 1 Sep 2020 16:10:34 -0700 Subject: [PATCH] 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 --- devstack/lib/ironic | 5 --- doc/source/admin/agent-token.rst | 10 +++-- ironic/api/controllers/v1/ramdisk.py | 12 +++--- ironic/cmd/conductor.py | 12 ------ ironic/conductor/manager.py | 29 ++----------- ironic/conf/default.py | 10 ----- ironic/drivers/modules/agent_client.py | 37 ++++++---------- .../unit/api/controllers/v1/test_ramdisk.py | 25 +++++++---- ironic/tests/unit/conductor/test_manager.py | 43 ++++--------------- .../unit/drivers/modules/test_agent_client.py | 28 ------------ .../no-tokenless-agents-c6c16d79ccc0da7a.yaml | 20 +++++++++ zuul.d/ironic-jobs.yaml | 1 - 12 files changed, 72 insertions(+), 160 deletions(-) create mode 100644 releasenotes/notes/no-tokenless-agents-c6c16d79ccc0da7a.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index 475f14e741..f3ff1eb00a 100644 --- a/devstack/lib/ironic +++ b/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:-} @@ -1423,8 +1420,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 diff --git a/doc/source/admin/agent-token.rst b/doc/source/admin/agent-token.rst index 4c2fd0e348..85d6b8354a 100644 --- a/doc/source/admin/agent-token.rst +++ b/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 -``[DEFAULT]require_agent_token`` can be set ``True`` to explicitly require -token use. +``ironic-python-agent``. As of the Victoria release, use of Agent Token is +required for all agents and the previously available setting to force this +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 diff --git a/ironic/api/controllers/v1/ramdisk.py b/ironic/api/controllers/v1/ramdisk.py index a79b070fa7..787955eea6 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 4164db18a9..d2ee20f5f9 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 cb20fcfd82..b4959abdf4 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 b799208f03..ffe2b20401 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 4cb3495096..74f28581c9 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 1b233fc701..d35a864ef7 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 7ab03e1754..994ba78b9a 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,35 +7253,11 @@ 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') - @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', @@ -7286,7 +7265,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, @@ -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 cb794c94c7..3430105349 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_client.py +++ b/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' diff --git a/releasenotes/notes/no-tokenless-agents-c6c16d79ccc0da7a.yaml b/releasenotes/notes/no-tokenless-agents-c6c16d79ccc0da7a.yaml new file mode 100644 index 0000000000..e3715fbaa6 --- /dev/null +++ b/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 `_ + 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 `_ + documentation. diff --git a/zuul.d/ironic-jobs.yaml b/zuul.d/ironic-jobs.yaml index 91f45a2c76..c1d8fe00d6 100644 --- a/zuul.d/ironic-jobs.yaml +++ b/zuul.d/ironic-jobs.yaml @@ -821,7 +821,6 @@ IRONIC_VM_SPECS_RAM: 384 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