Merge "Remove bespoke logic for handling redirects while validating URLs"

This commit is contained in:
Zuul
2025-11-04 11:28:35 +00:00
committed by Gerrit Code Review
8 changed files with 102 additions and 231 deletions

View File

@@ -953,21 +953,6 @@ class NodeVerifyFailure(IronicException):
_msg_fmt = _("Failed to verify node %(node)s: %(reason)s")
class ImageRefIsARedirect(IronicException):
_msg_fmt = _("Received a URL redirect when attempting to evaluate "
"image reference %(image_ref)s pointing to "
"%(redirect_url)s. This may, or may not be recoverable.")
redirect_url = None
def __init__(self, image_ref=None, redirect_url=None, msg=None):
self.redirect_url = redirect_url
# Kwargs are expected by IronicException to convert the message.
super(ImageRefIsARedirect, self).__init__(
message=msg,
image_ref=image_ref,
redirect_url=redirect_url)
class ConcurrentActionLimit(TemporaryFailure):
# NOTE(TheJulia): We explicitly don't report the concurrent
# action limit configuration value as a security guard since

View File

@@ -153,9 +153,6 @@ class HttpImageService(BaseImageService):
shown in exception message.
:raises: exception.ImageRefValidationFailed if HEAD request failed or
returned response code not equal to 200.
:raises: exception.ImageRefIsARedirect if the supplied URL is a
redirect to a different URL. The caller may be able to handle
this.
:returns: Response to HEAD request.
"""
output_url = 'secreturl' if secret else image_href
@@ -170,32 +167,9 @@ class HttpImageService(BaseImageService):
auth = HttpImageService.gen_auth_from_conf_user_pass(image_href)
# NOTE(TheJulia): Head requests do not work on things that are not
# files, but they can be responded with redirects or a 200 OK....
# We don't want to permit endless redirects either, thus not
# request an override to the requests default to try and resolve
# redirects as otherwise we might end up with something like
# HTTPForbidden or a list of files. Both should be okay to at
# least know things are okay in a limited fashion.
response = requests.head(image_href, verify=verify,
timeout=CONF.webserver_connection_timeout,
auth=auth)
if (response.status_code == http_client.MOVED_PERMANENTLY
or response.status_code == http_client.FOUND
or response.status_code == http_client.TEMPORARY_REDIRECT
or response.status_code == http_client.PERMANENT_REDIRECT):
# NOTE(TheJulia): In the event we receive a redirect, we need
# to notify the caller. Before this we would just fail,
# but a url which is missing a trailing slash results in a
# redirect to a target path, and the caller *may* actually
# care about that.
redirect = requests.Session().get_redirect_target(response)
# Extra guard because this is pointless if there is no
# location in the field. Requests also properly formats
# our string for us, or gives us None.
if redirect:
raise exception.ImageRefIsARedirect(
image_ref=image_href,
redirect_url=redirect)
auth=auth, allow_redirects=True)
if (response.status_code == http_client.FORBIDDEN
and str(image_href).endswith('/')):

View File

@@ -790,24 +790,12 @@ def is_source_a_path(ctx, image_source):
context=ctx)
try:
res = image_service.validate_href(image_source)
if 'headers' in dir(res):
# response/result is from the HTTP check path.
headers = res.headers
else:
# We have no headers.
headers = {}
except exception.ImageRefIsARedirect as e:
# Our exception handling formats this for us in this
# case. \o/
LOG.debug(str(e))
# Servers redirect to a proper folder ending in a slash if
# not supplied originally.
if e.redirect_url and e.redirect_url.endswith('/'):
return True
headers = getattr(res, 'headers', {})
except Exception:
# NOTE(TheJulia): I don't really like this pattern, *but*
# the wholedisk image support is similar.
return
# NOTE(TheJulia): Files should have been caught almost exclusively
# before with the Content-Length check.
# When the ISO is mounted and the webserver mount point url is
@@ -833,7 +821,8 @@ def is_source_a_path(ctx, image_source):
# NOTE(TheJulia): Files on a webserver have a length which is returned
# when headres are queried.
return False
if image_source.endswith('/'):
# In case of redirects, URL may be different from image_source.
if res.url.endswith('/'):
# If all else fails, looks like a URL, and the server didn't give
# us any hints.
return True

View File

@@ -1210,7 +1210,10 @@ def _validate_image_url(node, url, secret=False, inspect_image=None,
oci.set_image_auth(url, image_auth)
oci.validate_href(url, secret)
else:
image_service.HttpImageService().validate_href(url, secret)
resp = image_service.HttpImageService().validate_href(url, secret)
if resp and resp.url != url:
LOG.debug('URL %s redirected to %s', url, resp.url)
image_info['image_source'] = resp.url
except exception.ImageRefValidationFailed as e:
with excutils.save_and_reraise_exception():
LOG.error("The specified URL is not a valid HTTP(S) URL or is "
@@ -1540,73 +1543,48 @@ def build_instance_info_for_deploy(task):
# and not a file, or where a user may be supplying a full URL to
# a remotely hosted image, we at a minimum need to check if the
# url is valid, and address any redirects upfront.
try:
# NOTE(TheJulia): In the case we're here, we not doing an
# integrated image based deploy, but we may also be doing
# a path based anaconda base deploy, in which case we have
# no backing image, but we need to check for a URL
# redirection. So, if the source is a path (i.e. isap),
# we don't need to inspect the image as there is no image
# in the case for the deployment to drive.
validated_results = {}
if isap:
# This is if the source is a path url, such as one used by
# anaconda templates to to rely upon bootstrapping
# defaults.
_validate_image_url(node, image_source,
inspect_image=False)
else:
# When not isap, we can just let _validate_image_url make
# the required decision on if contents need to be sampled,
# or not. We try to pass the image_disk_format which may
# be declared by the user, and if not we set
# expected_format to None.
validate_results = _validate_image_url(
node,
image_source,
expected_format=instance_info.get(
'image_disk_format',
None))
# NOTE(TheJulia): In the case we're here, we not doing an
# integrated image based deploy, but we may also be doing
# a path based anaconda base deploy, in which case we have
# no backing image, but we need to check for a URL
# redirection. So, if the source is a path (i.e. isap),
# we don't need to inspect the image as there is no image
# in the case for the deployment to drive.
if isap:
# This is if the source is a path url, such as one used by
# anaconda templates to to rely upon bootstrapping
# defaults.
validate_results = _validate_image_url(node, image_source,
inspect_image=False)
else:
# When not isap, we can just let _validate_image_url make
# the required decision on if contents need to be sampled,
# or not. We try to pass the image_disk_format which may
# be declared by the user, and if not we set
# expected_format to None.
validate_results = _validate_image_url(
node,
image_source,
expected_format=instance_info.get(
'image_disk_format',
None))
# image_url is internal, and used by IPA and some boot
# templates. In most cases, it needs to come from image_source
# explicitly.
if 'disk_format' in validated_results:
if 'disk_format' in validate_results:
# Ensure IPA has the value available, so write what we
# detect, if anything. This is also an item which might be
# needful with ansible deploy interface, when used in
# standalone mode.
instance_info['image_disk_format'] = \
validate_results.get('disk_format')
if not instance_info.get('image_url'):
instance_info['image_url'] = image_source
except exception.ImageRefIsARedirect as e:
# At this point, we've got a redirect response from the
# webserver, and we're going to try to handle it as a single
# redirect action, as requests, by default, only lets a single
# redirect to occur. This is likely a URL pathing fix, like a
# trailing / on a path,
# or move to HTTPS from a user supplied HTTP url.
if e.redirect_url:
# Since we've got a redirect, we need to carry the rest of
# the request logic as well, which includes recording a
# disk format, if applicable.
instance_info['image_url'] = e.redirect_url
# We need to save the image_source back out so it caches
instance_info['image_source'] = e.redirect_url
task.node.instance_info = instance_info
if not isap:
# The redirect doesn't relate to a path being used, so
# the target is a filename, likely cause is webserver
# telling the client to use HTTPS.
validated_results = _validate_image_url(
node, e.redirect_url,
expected_format=instance_info.get(
'image_disk_format', None))
if 'disk_format' in validated_results:
instance_info['image_disk_format'] = \
validated_results.get('disk_format')
else:
raise
redirect_target = validate_results.get('image_source')
if redirect_target:
instance_info['image_source'] = redirect_target
instance_info['image_url'] = redirect_target
if not instance_info.get('image_url'):
instance_info['image_url'] = image_source
if not isap:
if not iwdi:

View File

@@ -44,7 +44,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.service.validate_href(self.href)
path_mock.assert_not_called()
head_mock.assert_called_once_with(self.href, verify=True,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
response.status_code = http_client.NO_CONTENT
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href,
@@ -62,7 +63,8 @@ class HttpImageServiceTestCase(base.TestCase):
response.status_code = http_client.OK
self.service.validate_href(self.href)
head_mock.assert_called_once_with(self.href, verify=False,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
response.status_code = http_client.NO_CONTENT
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href,
@@ -79,7 +81,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href, self.href)
head_mock.assert_called_once_with(self.href, verify=False,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
head_mock.side_effect = requests.RequestException()
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href, self.href)
@@ -92,7 +95,8 @@ class HttpImageServiceTestCase(base.TestCase):
response.status_code = http_client.OK
self.service.validate_href(self.href)
head_mock.assert_called_once_with(self.href, verify=True,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
response.status_code = http_client.NO_CONTENT
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href,
@@ -110,7 +114,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href, self.href)
head_mock.assert_called_once_with(self.href, verify=True,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
head_mock.side_effect = requests.RequestException()
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href, self.href)
@@ -124,7 +129,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.service.validate_href(self.href)
head_mock.assert_called_once_with(self.href, verify='/some/path',
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
response.status_code = http_client.NO_CONTENT
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href,
@@ -150,7 +156,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.service.validate_href(self.href)
head_mock.assert_called_once_with(self.href, verify='/some/path',
timeout=60, auth=auth_creds)
timeout=60, auth=auth_creds,
allow_redirects=True)
response.status_code = http_client.NO_CONTENT
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href,
@@ -179,7 +186,8 @@ class HttpImageServiceTestCase(base.TestCase):
response.status_code = http_client.OK
self.service.validate_href(self.href)
head_mock.assert_called_once_with(self.href, verify=True,
timeout=15, auth=None)
timeout=15, auth=None,
allow_redirects=True)
response.status_code = http_client.NO_CONTENT
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href,
@@ -199,7 +207,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href, self.href)
head_mock.assert_called_once_with(self.href, verify='/some/path',
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_verify_error(self, head_mock):
@@ -208,7 +217,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href, self.href)
head_mock.assert_called_once_with(self.href, verify='/some/path',
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_verify_os_error(self, head_mock):
@@ -217,7 +227,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.assertRaises(exception.ImageRefValidationFailed,
self.service.validate_href, self.href)
head_mock.assert_called_once_with(self.href, verify='/some/path',
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_error_with_secret_parameter(self, head_mock):
@@ -230,7 +241,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.assertIn('secreturl', str(e))
self.assertNotIn(self.href, str(e))
head_mock.assert_called_once_with(self.href, verify=False,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_path_forbidden(self, head_mock):
@@ -241,73 +253,10 @@ class HttpImageServiceTestCase(base.TestCase):
url = self.href + '/'
resp = self.service.validate_href(url)
head_mock.assert_called_once_with(url, verify=True,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
self.assertEqual(http_client.FORBIDDEN, resp.status_code)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_path_moved_permanently(self, head_mock):
cfg.CONF.set_override('webserver_verify_ca', 'True')
response = head_mock.return_value
response.status_code = http_client.MOVED_PERMANENTLY
url = self.href + '/'
new_url = 'http://new-url'
response.headers = {'location': new_url}
exc = self.assertRaises(exception.ImageRefIsARedirect,
self.service.validate_href,
url)
self.assertEqual(new_url, exc.redirect_url)
head_mock.assert_called_once_with(url, verify=True,
timeout=60, auth=None)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_path_found(self, head_mock):
cfg.CONF.set_override('webserver_verify_ca', 'True')
response = head_mock.return_value
response.status_code = http_client.FOUND
url = self.href + '/'
new_url = 'http://new-url'
response.headers = {'location': new_url}
exc = self.assertRaises(exception.ImageRefIsARedirect,
self.service.validate_href,
url)
self.assertEqual(new_url, exc.redirect_url)
head_mock.assert_called_once_with(url, verify=True,
timeout=60, auth=None)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_path_temporary_redirect(self, head_mock):
cfg.CONF.set_override('webserver_verify_ca', 'True')
response = head_mock.return_value
response.status_code = http_client.TEMPORARY_REDIRECT
url = self.href + '/'
new_url = 'http://new-url'
response.headers = {'location': new_url}
exc = self.assertRaises(exception.ImageRefIsARedirect,
self.service.validate_href,
url)
self.assertEqual(new_url, exc.redirect_url)
head_mock.assert_called_once_with(url, verify=True,
timeout=60, auth=None)
@mock.patch.object(requests, 'head', autospec=True)
def test_validate_href_path_permanent_redirect(self, head_mock):
cfg.CONF.set_override('webserver_verify_ca', 'True')
response = head_mock.return_value
response.status_code = http_client.PERMANENT_REDIRECT
url = self.href + '/'
new_url = 'http://new-url'
response.headers = {'location': new_url}
exc = self.assertRaises(exception.ImageRefIsARedirect,
self.service.validate_href,
url)
self.assertEqual(new_url, exc.redirect_url)
head_mock.assert_called_once_with(url, verify=True,
timeout=60, auth=None)
def test_verify_basic_auth_cred_format(self):
self.assertIsNone(self
.service
@@ -373,7 +322,8 @@ class HttpImageServiceTestCase(base.TestCase):
}
result = self.service.show(self.href)
head_mock.assert_called_once_with(self.href, verify=True,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
self.assertEqual({'size': 100, 'updated_at': mtime_date,
'properties': {}, 'no_cache': False}, result)
@@ -399,7 +349,8 @@ class HttpImageServiceTestCase(base.TestCase):
}
result = self.service.show(self.href)
head_mock.assert_called_once_with(self.href, verify=True,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
self.assertEqual({
'size': 100,
'updated_at': datetime.datetime(2014, 11, 15, 8, 12, 31),
@@ -423,7 +374,8 @@ class HttpImageServiceTestCase(base.TestCase):
self.assertRaises(exception.ImageRefValidationFailed,
self.service.show, self.href)
head_mock.assert_called_with(self.href, verify=True,
timeout=60, auth=None)
timeout=60, auth=None,
allow_redirects=True)
@mock.patch.object(shutil, 'copyfileobj', autospec=True)
@mock.patch.object(requests, 'get', autospec=True)

View File

@@ -624,6 +624,7 @@ class IronicImagesTestCase(base.TestCase):
mock_gip,
mock_validate):
mock_igi.return_value = False
mock_validate.return_value.url = 'http://whole-disk-image'
instance_info = {
'image_source': 'http://whole-disk-image'}
is_whole_disk_image = images.is_whole_disk_image('context',
@@ -665,22 +666,6 @@ class IronicImagesTestCase(base.TestCase):
self.assertTrue(images.is_source_a_path('context', 'http://foo/bar'))
validate_mock.assert_called_once_with(mock.ANY, 'http://foo/bar')
@mock.patch.object(images, 'LOG', autospec=True)
@mock.patch.object(image_service.HttpImageService, 'validate_href',
autospec=True)
def test_is_source_a_path_redirect(self, validate_mock, log_mock):
url = 'http://foo/bar'
redirect_url = url + '/'
validate_mock.side_effect = exception.ImageRefIsARedirect(
url, redirect_url)
self.assertTrue(images.is_source_a_path('context', url))
log_mock.debug.assert_called_once_with('Received a URL redirect when '
'attempting to evaluate image '
'reference http://foo/bar '
'pointing to http://foo/bar/. '
'This may, or may not be '
'recoverable.')
@mock.patch.object(image_service.HttpImageService, 'validate_href',
autospec=True)
def test_is_source_a_path_other_error(self, validate_mock):

View File

@@ -2361,6 +2361,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
autospec=True)
def test_build_instance_info_for_deploy_nonglance_image(
self, validate_href_mock, mock_cache_image):
validate_href_mock.return_value.url = 'http://image-ref'
i_info = self.node.instance_info
driver_internal_info = self.node.driver_internal_info
i_info['image_source'] = 'http://image-ref'
@@ -2392,6 +2393,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
self, validate_href_mock, mock_cache_image):
cfg.CONF.set_override('conductor_always_validates_images', True,
group='conductor')
validate_href_mock.return_value.url = 'http://image-ref'
i_info = self.node.instance_info
driver_internal_info = self.node.driver_internal_info
i_info['image_source'] = 'http://image-ref'
@@ -2426,6 +2428,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
group='conductor')
cfg.CONF.set_override('disable_deep_image_inspection', True,
group='conductor')
validate_href_mock.return_value.url = 'http://image-ref'
i_info = self.node.instance_info
driver_internal_info = self.node.driver_internal_info
i_info['image_source'] = 'http://image-ref'
@@ -2473,11 +2476,14 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
self.node.driver_internal_info = driver_internal_info
self.node.save()
validate_href_mock.side_effect = ['http://image-ref',
'http://kernel-ref',
'http://ramdisk-ref']
validate_href_mock.side_effect = [
mock.Mock(url='http://image-ref'),
mock.Mock(url='http://kernel-ref'),
mock.Mock(url='http://ramdisk-ref'),
]
parse_instance_info_mock.return_value = {'swap_mb': 5}
expected_i_info = {'image_source': 'http://image-ref',
expected_i_info = {'image_disk_format': 'qcow2',
'image_source': 'http://image-ref',
'image_url': 'http://image-ref',
'image_type': 'partition',
'kernel': 'http://kernel-ref',
@@ -2524,9 +2530,11 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
self.node.driver_internal_info = driver_internal_info
self.node.save()
validate_href_mock.side_effect = ['http://image-ref',
'http://kernel-ref',
'http://ramdisk-ref']
validate_href_mock.side_effect = [
mock.Mock(url='http://image-ref'),
mock.Mock(url='http://kernel-ref'),
mock.Mock(url='http://ramdisk-ref'),
]
parse_instance_info_mock.return_value = {'swap_mb': 5}
expected_i_info = {'image_source': 'http://image-ref',
'image_url': 'http://image-ref',
@@ -2576,6 +2584,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
autospec=True)
def test_build_instance_info_for_deploy_source_is_a_path(
self, validate_href_mock):
validate_href_mock.return_value.url = 'http://image-url/folder/'
i_info = self.node.instance_info
driver_internal_info = self.node.driver_internal_info
i_info['image_source'] = 'http://image-url/folder/'
@@ -2611,9 +2620,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
self.node.instance_info = i_info
self.node.driver_internal_info = driver_internal_info
self.node.save()
validate_href_mock.side_effect = exception.ImageRefIsARedirect(
image_ref=url,
redirect_url=r_url)
validate_href_mock.return_value = mock.Mock(url=r_url)
with task_manager.acquire(
self.context, self.node.uuid, shared=False) as task:
@@ -2643,11 +2650,7 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
self.node.instance_info = i_info
self.node.driver_internal_info = driver_internal_info
self.node.save()
validate_href_mock.side_effect = iter([
exception.ImageRefIsARedirect(
image_ref=url,
redirect_url=r_url),
None])
validate_href_mock.return_value = mock.Mock(url=r_url)
with task_manager.acquire(
self.context, self.node.uuid, shared=False) as task:
@@ -2657,10 +2660,8 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
self.assertNotEqual(self.node.instance_info['image_source'],
info['image_url'])
self.assertEqual(r_url, info['image_url'])
validate_href_mock.assert_has_calls([
mock.call(mock.ANY, 'http://image-url/file', False),
mock.call(mock.ANY, 'https://image-url/file', False)
])
validate_href_mock.assert_called_once_with(
mock.ANY, 'http://image-url/file', False)
mock_cache_image.assert_called_once_with(mock.ANY,
task.node,
force_raw=False,
@@ -2849,6 +2850,7 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
autospec=True)
def test_build_instance_info_local_image(self, validate_href_mock):
cfg.CONF.set_override('image_download_source', 'local', group='agent')
validate_href_mock.return_value.url = 'http://image-ref'
i_info = self.node.instance_info
driver_internal_info = self.node.driver_internal_info
i_info['image_source'] = 'http://image-ref'
@@ -2926,6 +2928,7 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
cfg.CONF.set_override('image_download_source', 'http', group='agent')
cfg.CONF.set_override('conductor_always_validates_images', True,
group='conductor')
validate_href_mock.return_value.url = 'http://image-ref'
i_info = self.node.instance_info
driver_internal_info = self.node.driver_internal_info
i_info['image_source'] = 'http://image-ref'

View File

@@ -0,0 +1,5 @@
---
fixes:
- |
Fixes validation of image source URLs that are redirects to another URL.
Previously, it would raise UnboundLocalError.