openstack/ironic
Code Issues Proposed changes

Assert URL consistency for agent_url

Browse Source 
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
This commit is contained in:
Julia Kreger 2024-06-04 07:13:36 -07:00
parent 8bdf1fca91
commit 88c45151fa
3 changed files with 61 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
ironic/api/controllers/v1/ramdisk.py
View File

@ -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:

60
ironic/tests/unit/api/controllers/v1/test_ramdisk.py
View File

@ -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',

10
releasenotes/notes/additional-agent-url-validation-97271ce72b0b1a9d.yaml Normal file
View File

@ -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.