openstack/ironic
Code Issues Proposed changes

Merge "Assert URL consistency for agent_url"

Browse Source
This commit is contained in:
Zuul 2024-06-06 13:14:36 +00:00 committed by Gerrit Code Review
commit 8086167054
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. # under the License.
from http import client as http_client from http import client as http_client
from urllib import parse as urlparse
from oslo_config import cfg from oslo_config import cfg
from oslo_log import log from oslo_log import log
@ -225,6 +226,20 @@ class HeartbeatController(rest.RestController):
rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) rpc_node = api_utils.get_rpc_node_with_suffix(node_ident)
dii = rpc_node['driver_internal_info'] dii = rpc_node['driver_internal_info']
agent_url = dii.get('agent_url') 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 # If we have an agent_url on file, and we get something different
# we should fail because this is unexpected behavior of the agent. # we should fail because this is unexpected behavior of the agent.
if agent_url is not None and agent_url != callback_url: 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): def test_old_api_version(self):
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % uuidutils.generate_uuid(), '/heartbeat/%s' % uuidutils.generate_uuid(),
{'callback_url': 'url'}, {'callback_url': 'https://url'},
headers={api_base.Version.string: str(api_v1.min_version())}, headers={api_base.Version.string: str(api_v1.min_version())},
expect_errors=True) expect_errors=True)
self.assertEqual(http_client.NOT_FOUND, response.status_int) 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): def test_node_not_found(self):
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % uuidutils.generate_uuid(), '/heartbeat/%s' % uuidutils.generate_uuid(),
{'callback_url': 'url'}, {'callback_url': 'https://url'},
headers={api_base.Version.string: str(api_v1.max_version())}, headers={api_base.Version.string: str(api_v1.max_version())},
expect_errors=True) expect_errors=True)
self.assertEqual(http_client.NOT_FOUND, response.status_int) 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) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'x'}, 'agent_token': 'x'},
headers={api_base.Version.string: str(api_v1.max_version())}) headers={api_base.Version.string: str(api_v1.max_version())})
self.assertEqual(http_client.ACCEPTED, response.status_int) self.assertEqual(http_client.ACCEPTED, response.status_int)
self.assertEqual(b'', response.body) self.assertEqual(b'', response.body)
mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
node.uuid, 'url', None, 'x', node.uuid, 'https://url', None,
None, None, None, 'x', None, None, None,
topic='test-topic') topic='test-topic')
@mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True) @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) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s.json' % node.uuid, '/heartbeat/%s.json' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'maybe some magic'}, 'agent_token': 'maybe some magic'},
headers=headers) headers=headers)
self.assertEqual(http_client.ACCEPTED, response.status_int) self.assertEqual(http_client.ACCEPTED, response.status_int)
self.assertEqual(b'', response.body) self.assertEqual(b'', response.body)
mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
node.uuid, 'url', None, node.uuid, 'https://url',
'maybe some magic', None, 'maybe some magic',
None, None, None, None, None, None,
topic='test-topic') topic='test-topic')
@ -254,13 +254,13 @@ class TestHeartbeat(test_api_base.BaseApiTest):
node = obj_utils.create_test_node(self.context, name='test.1') node = obj_utils.create_test_node(self.context, name='test.1')
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.name, '/heartbeat/%s' % node.name,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'token'}, 'agent_token': 'token'},
headers={api_base.Version.string: str(api_v1.max_version())}) headers={api_base.Version.string: str(api_v1.max_version())})
self.assertEqual(http_client.ACCEPTED, response.status_int) self.assertEqual(http_client.ACCEPTED, response.status_int)
self.assertEqual(b'', response.body) self.assertEqual(b'', response.body)
mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
node.uuid, 'url', None, node.uuid, 'https://url', None,
'token', None, None, None, 'token', None, None, None,
topic='test-topic') topic='test-topic')
@ -269,14 +269,15 @@ class TestHeartbeat(test_api_base.BaseApiTest):
node = obj_utils.create_test_node(self.context) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_version': '1.4.1', 'agent_version': '1.4.1',
'agent_token': 'meow'}, 'agent_token': 'meow'},
headers={api_base.Version.string: str(api_v1.max_version())}) headers={api_base.Version.string: str(api_v1.max_version())})
self.assertEqual(http_client.ACCEPTED, response.status_int) self.assertEqual(http_client.ACCEPTED, response.status_int)
self.assertEqual(b'', response.body) self.assertEqual(b'', response.body)
mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
node.uuid, 'url', '1.4.1', node.uuid, 'https://url',
'1.4.1',
'meow', 'meow',
None, None, None, None, None, None,
topic='test-topic') topic='test-topic')
@ -286,20 +287,31 @@ class TestHeartbeat(test_api_base.BaseApiTest):
node = obj_utils.create_test_node(self.context) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_version': '1.4.1'}, 'agent_version': '1.4.1'},
headers={api_base.Version.string: '1.35'}, headers={api_base.Version.string: '1.35'},
expect_errors=True) expect_errors=True)
self.assertEqual(http_client.BAD_REQUEST, response.status_int) 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) @mock.patch.object(rpcapi.ConductorAPI, 'heartbeat', autospec=True)
def test_heartbeat_rejects_different_callback_url(self, mock_heartbeat): def test_heartbeat_rejects_different_callback_url(self, mock_heartbeat):
node = obj_utils.create_test_node( node = obj_utils.create_test_node(
self.context, self.context,
driver_internal_info={'agent_url': 'url'}) driver_internal_info={'agent_url': 'https://url'})
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url2'}, {'callback_url': 'https://url2'},
headers={api_base.Version.string: str(api_v1.max_version())}, headers={api_base.Version.string: str(api_v1.max_version())},
expect_errors=True) expect_errors=True)
self.assertEqual(http_client.BAD_REQUEST, response.status_int) 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) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'http://url',
'agent_token': 'abcdef1'}, 'agent_token': 'abcdef1'},
headers={api_base.Version.string: str(api_v1.max_version())}) headers={api_base.Version.string: str(api_v1.max_version())})
self.assertEqual(http_client.ACCEPTED, response.status_int) self.assertEqual(http_client.ACCEPTED, response.status_int)
self.assertEqual(b'', response.body) self.assertEqual(b'', response.body)
mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
node.uuid, 'url', None, node.uuid, 'http://url', None,
'abcdef1', None, None, None, 'abcdef1', None, None, None,
topic='test-topic') topic='test-topic')
@ -324,14 +336,14 @@ class TestHeartbeat(test_api_base.BaseApiTest):
node = obj_utils.create_test_node(self.context) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'meow', 'agent_token': 'meow',
'agent_verify_ca': 'abcdef1'}, 'agent_verify_ca': 'abcdef1'},
headers={api_base.Version.string: str(api_v1.max_version())}) headers={api_base.Version.string: str(api_v1.max_version())})
self.assertEqual(http_client.ACCEPTED, response.status_int) self.assertEqual(http_client.ACCEPTED, response.status_int)
self.assertEqual(b'', response.body) self.assertEqual(b'', response.body)
mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
node.uuid, 'url', None, node.uuid, 'https://url', None,
'meow', 'abcdef1', None, None, 'meow', 'abcdef1', None, None,
topic='test-topic') topic='test-topic')
@ -340,7 +352,7 @@ class TestHeartbeat(test_api_base.BaseApiTest):
node = obj_utils.create_test_node(self.context) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'meow', 'agent_token': 'meow',
'agent_status': 'start', 'agent_status': 'start',
'agent_status_message': 'woof', 'agent_status_message': 'woof',
@ -349,7 +361,7 @@ class TestHeartbeat(test_api_base.BaseApiTest):
self.assertEqual(http_client.ACCEPTED, response.status_int) self.assertEqual(http_client.ACCEPTED, response.status_int)
self.assertEqual(b'', response.body) self.assertEqual(b'', response.body)
mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY, mock_heartbeat.assert_called_once_with(mock.ANY, mock.ANY,
node.uuid, 'url', None, node.uuid, 'https://url', None,
'meow', 'abcdef1', 'start', 'meow', 'abcdef1', 'start',
'woof', topic='test-topic') 'woof', topic='test-topic')
@ -358,7 +370,7 @@ class TestHeartbeat(test_api_base.BaseApiTest):
node = obj_utils.create_test_node(self.context) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'meow', 'agent_token': 'meow',
'agent_status': 'invalid_state', 'agent_status': 'invalid_state',
'agent_status_message': 'woof', 'agent_status_message': 'woof',
@ -372,7 +384,7 @@ class TestHeartbeat(test_api_base.BaseApiTest):
node = obj_utils.create_test_node(self.context) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'meow', 'agent_token': 'meow',
'agent_verify_ca': 'abcd'}, 'agent_verify_ca': 'abcd'},
headers={api_base.Version.string: '1.67'}, 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) node = obj_utils.create_test_node(self.context)
response = self.post_json( response = self.post_json(
'/heartbeat/%s' % node.uuid, '/heartbeat/%s' % node.uuid,
{'callback_url': 'url', {'callback_url': 'https://url',
'agent_token': 'meow', 'agent_token': 'meow',
'agent_verify_ca': 'abcd', 'agent_verify_ca': 'abcd',
'agent_status': 'wow', '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.