From 88c45151fa0beea9cb9e218af632ab640074f65a Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 4 Jun 2024 07:13:36 -0700 Subject: [PATCH] Assert URL consistency for agent_url While agent_url is software generated, it is still a public endpoint and at least needs some upfront filtering applied. To do this, we can leverage urllib in the standard library to disassemble the url, and reconstruct it based upon the standards. The plus of this approach is that it will remove some invalid formatting for us, and if things are too out of line, an exception is raised as ValueError. An important note, this is *not* explicitly urlparsing security[0] as denoted in the Python urllib documentation, but that the application should operate defensively. [0]: https://docs.python.org/3/library/urllib.parse.html#url-parsing-security Change-Id: I45ee1c8a73ed13511bc47a69130105f16d34be1e --- ironic/api/controllers/v1/ramdisk.py | 15 +++++ .../unit/api/controllers/v1/test_ramdisk.py | 60 +++++++++++-------- ...agent-url-validation-97271ce72b0b1a9d.yaml | 10 ++++ 3 files changed, 61 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/additional-agent-url-validation-97271ce72b0b1a9d.yaml diff --git a/ironic/api/controllers/v1/ramdisk.py b/ironic/api/controllers/v1/ramdisk.py index 47b320b24c..9ce0608f21 100644 --- a/ironic/api/controllers/v1/ramdisk.py +++ b/ironic/api/controllers/v1/ramdisk.py @@ -13,6 +13,7 @@ # under the License. from http import client as http_client +from urllib import parse as urlparse from oslo_config import cfg from oslo_log import log @@ -225,6 +226,20 @@ class HeartbeatController(rest.RestController): rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) dii = rpc_node['driver_internal_info'] agent_url = dii.get('agent_url') + try: + # NOTE(TheJulia): Use of urllib.urlparse is not a security + # guard, but detecting oddities and incorrect formatting + # https://docs.python.org/3/library/urllib.parse.html#url-parsing-security # noqa + parsed_url = urlparse.urlparse(callback_url) + # Check if http (compatibility), or https (much newer agents) + if 'http' not in parsed_url.scheme: + raise ValueError + callback_url = parsed_url.geturl() + except ValueError: + raise exception.InvalidParameterValue( + _('An issue with the supplied "callback_url" has been ' + 'detected.')) + # If we have an agent_url on file, and we get something different # we should fail because this is unexpected behavior of the agent. if agent_url is not None and agent_url != callback_url: diff --git a/ironic/tests/unit/api/controllers/v1/test_ramdisk.py b/ironic/tests/unit/api/controllers/v1/test_ramdisk.py index 70a918fde6..0eb4fe426f 100644 --- a/ironic/tests/unit/api/controllers/v1/test_ramdisk.py +++ b/ironic/tests/unit/api/controllers/v1/test_ramdisk.py @@ -204,7 +204,7 @@ class TestHeartbeat(test_api_base.BaseApiTest): def test_old_api_version(self): response = self.post_json( '/heartbeat/%s' % uuidutils.generate_uuid(), - {'callback_url': 'url'}, + {'callback_url': 'https://url'}, headers={api_base.Version.string: str(api_v1.min_version())}, expect_errors=True) self.assertEqual(http_client.NOT_FOUND, response.status_int) @@ -212,7 +212,7 @@ class TestHeartbeat(test_api_base.BaseApiTest): def test_node_not_found(self): response = self.post_json( '/heartbeat/%s' % uuidutils.generate_uuid(), - {'callback_url': 'url'}, + {'callback_url': 'https://url'}, headers={api_base.Version.string: str(api_v1.max_version())}, expect_errors=True) self.assertEqual(http_client.NOT_FOUND, response.status_int) @@ -222,14 +222,14 @@ 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': 'https://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, 'x', - None, None, None, + node.uuid, 'https://url', None, + 'x', None, None, None, topic='test-topic') @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True) @@ -238,14 +238,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': 'https://url', 'agent_token': 'maybe some magic'}, headers=headers) 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, - 'maybe some magic', + node.uuid, 'https://url', + None, 'maybe some magic', None, None, None, topic='test-topic') @@ -254,13 +254,13 @@ 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': 'https://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, + node.uuid, 'https://url', None, 'token', None, None, None, topic='test-topic') @@ -269,14 +269,15 @@ 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': 'https://url', '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', + node.uuid, 'https://url', + '1.4.1', 'meow', None, None, None, topic='test-topic') @@ -286,20 +287,31 @@ 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': 'https://url', 'agent_version': '1.4.1'}, headers={api_base.Version.string: '1.35'}, expect_errors=True) self.assertEqual(http_client.BAD_REQUEST, response.status_int) + @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True) + def test_heartbeat_rejects_file_url(self, mock_heartbeat): + node = obj_utils.create_test_node( + self.context) + response = self.post_json( + '/heartbeat/%s' % node.uuid, + {'callback_url': 'file:///path/to/the/wizzard'}, + headers={api_base.Version.string: str(api_v1.max_version())}, + expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True) def test_heartbeat_rejects_different_callback_url(self, mock_heartbeat): node = obj_utils.create_test_node( self.context, - driver_internal_info={'agent_url': 'url'}) + driver_internal_info={'agent_url': 'https://url'}) response = self.post_json( '/heartbeat/%s' % node.uuid, - {'callback_url': 'url2'}, + {'callback_url': 'https://url2'}, headers={api_base.Version.string: str(api_v1.max_version())}, expect_errors=True) self.assertEqual(http_client.BAD_REQUEST, response.status_int) @@ -309,13 +321,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': 'http://url', 'agent_token': 'abcdef1'}, 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, + node.uuid, 'http://url', None, 'abcdef1', None, None, None, topic='test-topic') @@ -324,14 +336,14 @@ 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': 'https://url', 'agent_token': 'meow', 'agent_verify_ca': 'abcdef1'}, 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, + node.uuid, 'https://url', None, 'meow', 'abcdef1', None, None, topic='test-topic') @@ -340,7 +352,7 @@ 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': 'https://url', 'agent_token': 'meow', 'agent_status': 'start', 'agent_status_message': 'woof', @@ -349,7 +361,7 @@ class TestHeartbeat(test_api_base.BaseApiTest): 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, + node.uuid, 'https://url', None, 'meow', 'abcdef1', 'start', 'woof', topic='test-topic') @@ -358,7 +370,7 @@ 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': 'https://url', 'agent_token': 'meow', 'agent_status': 'invalid_state', 'agent_status_message': 'woof', @@ -372,7 +384,7 @@ 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': 'https://url', 'agent_token': 'meow', 'agent_verify_ca': 'abcd'}, headers={api_base.Version.string: '1.67'}, @@ -384,7 +396,7 @@ 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': 'https://url', 'agent_token': 'meow', 'agent_verify_ca': 'abcd', 'agent_status': 'wow', diff --git a/releasenotes/notes/additional-agent-url-validation-97271ce72b0b1a9d.yaml b/releasenotes/notes/additional-agent-url-validation-97271ce72b0b1a9d.yaml new file mode 100644 index 0000000000..2dbcb21cdf --- /dev/null +++ b/releasenotes/notes/additional-agent-url-validation-97271ce72b0b1a9d.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + Adds additional validation to the agent ``callback_url``. +security: + - | + Additional validation of the ``callback_url`` which is supplied to Ironic + by the agent has been added. In addition to any standardized formatting + checks included in Python urllib, we will also reject requests which + have an invalid URL schema formatting.