From d8d32d93bd9fd0a9e759f7babe5db6fa804626de Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 22 Oct 2024 11:36:21 +0200 Subject: [PATCH] Warn when the provided checksum algorithm does not match the detected I have a case where a user provided the checksum URL with SHA256 checksums, while Metal3 defaulted os_hash_algo to "md5". We're going to change the Metal3 defaults in the next API version, but for now let us issue a clear warning in such case. Closes-Bug: #2085331 Change-Id: Ie4e62a378dc4a2089944f4302df3a8671b7c960f --- ironic_python_agent/extensions/standby.py | 19 ++++++++ .../tests/unit/extensions/test_standby.py | 47 ++++++++++++++++++- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/ironic_python_agent/extensions/standby.py b/ironic_python_agent/extensions/standby.py index d3bf04af2..8cbb4f83e 100644 --- a/ironic_python_agent/extensions/standby.py +++ b/ironic_python_agent/extensions/standby.py @@ -581,6 +581,25 @@ class ImageDownload(object): self._expected_hash_value, image_info) + # NOTE(dtantsur): verify that the user's input does not obviously + # contradict the actual value. It is especially easy to make such + # a mistake when providing a checksum URL. + if algo: + try: + detected_algo = _get_algorithm_by_length( + self._expected_hash_value) + except ValueError: + pass # an exotic algorithm? + else: + if detected_algo.name != algo: + LOG.warning("Provided checksum algorithm %(provided)s " + "does not match the detected algorithm " + "%(detected)s. It may be a sign of a user " + "error when providing the algorithm or the " + "checksum URL.", + {'provided': algo, + 'detected': detected_algo.name}) + details = [] for url in image_info['urls']: try: diff --git a/ironic_python_agent/tests/unit/extensions/test_standby.py b/ironic_python_agent/tests/unit/extensions/test_standby.py index c5d467fc1..ab514853b 100644 --- a/ironic_python_agent/tests/unit/extensions/test_standby.py +++ b/ironic_python_agent/tests/unit/extensions/test_standby.py @@ -795,11 +795,13 @@ class TestStandbyExtension(base.IronicAgentTest): image_download.verify_image(image_location) hashlib_mock.assert_called_with('sha512') + @mock.patch.object(standby.LOG, 'warning', autospec=True) @mock.patch('hashlib.new', autospec=True) @mock.patch('builtins.open', autospec=True) @mock.patch('requests.get', autospec=True) def test_verify_image_success_with_md5_fallback(self, requests_mock, - open_mock, hash_mock): + open_mock, hash_mock, + warn_mock): CONF.set_override('md5_enabled', True) image_info = _build_fake_image_info() image_info['os_hash_algo'] = 'algo-beyond-milky-way' @@ -807,6 +809,7 @@ class TestStandbyExtension(base.IronicAgentTest): image_info['checksum'] = 'd41d8cd98f00b204e9800998ecf8427e' response = requests_mock.return_value response.status_code = 200 + hash_mock.return_value.name = 'md5' hexdigest_mock = hash_mock.return_value.hexdigest hexdigest_mock.return_value = image_info['checksum'] image_location = '/foo/bar' @@ -818,7 +821,11 @@ class TestStandbyExtension(base.IronicAgentTest): hash_mock.assert_has_calls([ mock.call('md5'), mock.call().__bool__(), + mock.call('md5'), mock.call().hexdigest()]) + warn_mock.assert_called_once_with( + mock.ANY, + {'provided': 'algo-beyond-milky-way', 'detected': 'md5'}) @mock.patch('hashlib.new', autospec=True) @mock.patch('builtins.open', autospec=True) @@ -1760,7 +1767,9 @@ class TestImageDownload(base.IronicAgentTest): sleep_mock.assert_called_with(10) self.assertEqual(2, sleep_mock.call_count) - def test_download_image_and_checksum(self, requests_mock, hash_mock): + @mock.patch.object(standby.LOG, 'warning', autospec=True) + def test_download_image_and_checksum(self, warn_mock, requests_mock, + hash_mock): content = ['SpongeBob', 'SquarePants'] fake_cs = "019fe036425da1c562f2e9f5299820bf" cs_response = mock.Mock() @@ -1775,6 +1784,7 @@ class TestImageDownload(base.IronicAgentTest): image_info['os_hash_algo'] = 'sha512' image_info['os_hash_value'] = 'http://example.com/checksum' hash_mock.return_value.hexdigest.return_value = fake_cs + hash_mock.return_value.name = 'sha512' image_download = standby.ImageDownload(image_info) self.assertEqual(content, list(image_download)) @@ -1785,6 +1795,39 @@ class TestImageDownload(base.IronicAgentTest): stream=True, proxies={}, timeout=60), ]) self.assertEqual(fake_cs, image_download._hash_algo.hexdigest()) + warn_mock.assert_not_called() + + @mock.patch.object(standby.LOG, 'warning', autospec=True) + def test_download_image_and_checksum_warning_on_mismatch( + self, warn_mock, requests_mock, hash_mock): + content = ['SpongeBob', 'SquarePants'] + fake_cs = "019fe036425da1c562f2e9f5299820bf" + cs_response = mock.Mock() + cs_response.status_code = 200 + cs_response.text = fake_cs + '\n' + response = mock.Mock() + response.status_code = 200 + response.iter_content.return_value = content + requests_mock.side_effect = [cs_response, response] + + image_info = _build_fake_image_info() + image_info['os_hash_algo'] = 'sha512' + image_info['os_hash_value'] = 'http://example.com/checksum' + hash_mock.return_value.hexdigest.return_value = fake_cs + hash_mock.return_value.name = 'md5' + image_download = standby.ImageDownload(image_info) + + self.assertEqual(content, list(image_download)) + requests_mock.assert_has_calls([ + mock.call('http://example.com/checksum', cert=None, verify=True, + stream=True, proxies={}, timeout=60), + mock.call(image_info['urls'][0], cert=None, verify=True, + stream=True, proxies={}, timeout=60), + ]) + self.assertEqual(fake_cs, image_download._hash_algo.hexdigest()) + warn_mock.assert_called_once_with( + mock.ANY, + {'provided': 'sha512', 'detected': 'md5'}) def test_download_image_and_checksum_md5(self, requests_mock, hash_mock):