diff --git a/ironic_python_agent/extensions/standby.py b/ironic_python_agent/extensions/standby.py index 2f224fe6a..a89ba7548 100644 --- a/ironic_python_agent/extensions/standby.py +++ b/ironic_python_agent/extensions/standby.py @@ -198,9 +198,21 @@ class ImageDownload(object): :raises: ImageDownloadError if starting the image download fails for any reason. """ - self._md5checksum = hashlib.md5() self._time = time_obj or time.time() + self._image_info = image_info self._request = None + + # Determine the hash algorithm and value will be used for calculation + # and verification, fallback to md5 if algorithm is not set or not + # supported. + algo = image_info.get('os_hash_algo') + if algo and algo in hashlib.algorithms_available: + self._hash_algo = hashlib.new(algo) + self._expected_hash_value = image_info.get('os_hash_value') + else: + self._hash_algo = hashlib.md5() + self._expected_hash_value = image_info['checksum'] + details = [] for url in image_info['urls']: try: @@ -249,41 +261,31 @@ class ImageDownload(object): which is a constant in this module. """ for chunk in self._request.iter_content(IMAGE_CHUNK_SIZE): - self._md5checksum.update(chunk) + self._hash_algo.update(chunk) yield chunk - def md5sum(self): - """Computes and returns the md5 checksum of the downloaded image. + def verify_image(self, image_location): + """Verifies the checksum of the local images matches expectations. - Note that md5sum will not return the true checksum of the image until - the download has been fully completed through this object's - iterator inferface. + If this function does not raise ImageChecksumError then it is very + likely that the local copy of the image was transmitted and stored + correctly. - :returns: The md5 checksum of the image as a string in hexadecimal. + :param image_location: The location of the local image. + :raises: ImageChecksumError if the checksum of the local image does + not match the checksum as reported by glance in image_info. """ - return self._md5checksum.hexdigest() - - -def _verify_image(image_info, image_location, checksum): - """Verifies the checksum of the local images matches expectations. - - If this function does not raise ImageChecksumError then it is very likely - that the local copy of the image was transmitted and stored correctly. - - :param image_info: Image information dictionary. - :param image_location: The location of the local image. - :param checksum: The computed checksum of the local image. - :raises: ImageChecksumError if the checksum of the local image does not - match the checksum as reported by glance in image_info. - """ - LOG.debug('Verifying image at {} against MD5 checksum ' - '{}'.format(image_location, checksum)) - if checksum != image_info['checksum']: - LOG.error(errors.ImageChecksumError.details_str.format( - image_location, image_info['id'], - image_info['checksum'], checksum)) - raise errors.ImageChecksumError(image_location, image_info['id'], - image_info['checksum'], checksum) + checksum = self._hash_algo.hexdigest() + LOG.debug('Verifying image at {} against {} checksum ' + '{}'.format(image_location, self._hash_algo.name, checksum)) + if checksum != self._expected_hash_value: + LOG.error(errors.ImageChecksumError.details_str.format( + image_location, self._image_info['id'], + self._expected_hash_value, checksum)) + raise errors.ImageChecksumError(image_location, + self._image_info['id'], + self._expected_hash_value, + checksum) def _download_image(image_info): @@ -310,7 +312,7 @@ def _download_image(image_info): totaltime = time.time() - starttime LOG.info("Image downloaded from {} in {} seconds".format(image_location, totaltime)) - _verify_image(image_info, image_location, image_download.md5sum()) + image_download.verify_image(image_location) def _validate_image_info(ext, image_info=None, **kwargs): @@ -341,6 +343,18 @@ def _validate_image_info(ext, image_info=None, **kwargs): raise errors.InvalidCommandParamsError( 'Image \'checksum\' must be a non-empty string.') + os_hash_algo = image_info.get('os_hash_algo') + os_hash_value = image_info.get('os_hash_value') + if os_hash_algo or os_hash_value: + if (not isinstance(os_hash_algo, six.string_types) or + not os_hash_algo): + raise errors.InvalidCommandParamsError( + 'Image \'os_hash_algo\' must be a non-empty string.') + if (not isinstance(os_hash_value, six.string_types) or + not os_hash_value): + raise errors.InvalidCommandParamsError( + 'Image \'os_hash_value\' must be a non-empty string.') + class StandbyExtension(base.BaseAgentExtension): """Extension which adds stand-by related functionality to agent.""" @@ -397,7 +411,7 @@ class StandbyExtension(base.BaseAgentExtension): LOG.info("Image streamed onto device {} in {} " "seconds".format(device, totaltime)) # Verify if the checksum of the streamed image is correct - _verify_image(image_info, device, image_download.md5sum()) + image_download.verify_image(device) @base.async_command('cache_image', _validate_image_info) def cache_image(self, image_info=None, force=False): diff --git a/ironic_python_agent/tests/unit/extensions/test_standby.py b/ironic_python_agent/tests/unit/extensions/test_standby.py index 8b61301f6..7a942006d 100644 --- a/ironic_python_agent/tests/unit/extensions/test_standby.py +++ b/ironic_python_agent/tests/unit/extensions/test_standby.py @@ -68,6 +68,12 @@ class TestStandbyExtension(base.IronicAgentTest): def test_validate_image_info_success(self): standby._validate_image_info(None, _build_fake_image_info()) + def test_validate_image_info_success_with_new_hash_fields(self): + image_info = _build_fake_image_info() + image_info['os_hash_algo'] = 'md5' + image_info['os_hash_value'] = 'fake-checksum' + standby._validate_image_info(None, image_info) + def test_validate_image_info_missing_field(self): for field in ['id', 'urls', 'checksum']: invalid_info = _build_fake_image_info() @@ -109,6 +115,22 @@ class TestStandbyExtension(base.IronicAgentTest): standby._validate_image_info, invalid_info) + def test_validate_image_info_no_hash_value(self): + invalid_info = _build_fake_image_info() + invalid_info['os_hash_algo'] = 'sha512' + + self.assertRaises(errors.InvalidCommandParamsError, + standby._validate_image_info, + invalid_info) + + def test_validate_image_info_no_hash_algo(self): + invalid_info = _build_fake_image_info() + invalid_info['os_hash_value'] = 'fake-checksum' + + self.assertRaises(errors.InvalidCommandParamsError, + standby._validate_image_info, + invalid_info) + def test_cache_image_invalid_image_list(self): self.assertRaises(errors.InvalidCommandParamsError, self.agent_extension.cache_image, @@ -385,19 +407,87 @@ class TestStandbyExtension(base.IronicAgentTest): standby._download_image, image_info) - def test_verify_image_success(self): + @mock.patch('hashlib.md5', autospec=True) + @mock.patch('six.moves.builtins.open', autospec=True) + @mock.patch('requests.get', autospec=True) + def test_verify_image_success(self, requests_mock, open_mock, md5_mock): image_info = _build_fake_image_info() + response = requests_mock.return_value + response.status_code = 200 + hexdigest_mock = md5_mock.return_value.hexdigest + hexdigest_mock.return_value = image_info['checksum'] image_location = '/foo/bar' - checksum = image_info['checksum'] - standby._verify_image(image_info, image_location, checksum) + image_download = standby.ImageDownload(image_info) + image_download.verify_image(image_location) - def test_verify_image_failure(self): + @mock.patch('hashlib.new', autospec=True) + @mock.patch('six.moves.builtins.open', autospec=True) + @mock.patch('requests.get', autospec=True) + def test_verify_image_success_with_new_hash_fields(self, requests_mock, + open_mock, + hashlib_mock): image_info = _build_fake_image_info() + image_info['os_hash_algo'] = 'sha512' + image_info['os_hash_value'] = 'fake-sha512-value' + response = requests_mock.return_value + response.status_code = 200 + hexdigest_mock = hashlib_mock.return_value.hexdigest + hexdigest_mock.return_value = image_info['os_hash_value'] image_location = '/foo/bar' - checksum = 'invalid-checksum' + image_download = standby.ImageDownload(image_info) + image_download.verify_image(image_location) + hashlib_mock.assert_called_with('sha512') + + @mock.patch('hashlib.md5', autospec=True) + @mock.patch('six.moves.builtins.open', autospec=True) + @mock.patch('requests.get', autospec=True) + def test_verify_image_success_with_md5_fallback(self, requests_mock, + open_mock, md5_mock): + image_info = _build_fake_image_info() + image_info['os_hash_algo'] = 'algo-beyond-milky-way' + image_info['os_hash_value'] = 'mysterious-alien-codes' + response = requests_mock.return_value + response.status_code = 200 + hexdigest_mock = md5_mock.return_value.hexdigest + hexdigest_mock.return_value = image_info['checksum'] + image_location = '/foo/bar' + image_download = standby.ImageDownload(image_info) + image_download.verify_image(image_location) + + @mock.patch('hashlib.new', autospec=True) + @mock.patch('six.moves.builtins.open', autospec=True) + @mock.patch('requests.get', autospec=True) + def test_verify_image_failure_with_new_hash_fields(self, requests_mock, + open_mock, + hashlib_mock): + image_info = _build_fake_image_info() + image_info['os_hash_algo'] = 'sha512' + image_info['os_hash_value'] = 'fake-sha512-value' + response = requests_mock.return_value + response.status_code = 200 + image_download = standby.ImageDownload(image_info) + image_location = '/foo/bar' + hexdigest_mock = hashlib_mock.return_value.hexdigest + hexdigest_mock.return_value = 'invalid-checksum' self.assertRaises(errors.ImageChecksumError, - standby._verify_image, - image_info, image_location, checksum) + image_download.verify_image, + image_location) + hashlib_mock.assert_called_with('sha512') + + @mock.patch('hashlib.md5', autospec=True) + @mock.patch('six.moves.builtins.open', autospec=True) + @mock.patch('requests.get', autospec=True) + def test_verify_image_failure(self, requests_mock, open_mock, md5_mock): + image_info = _build_fake_image_info() + response = requests_mock.return_value + response.status_code = 200 + image_download = standby.ImageDownload(image_info) + image_location = '/foo/bar' + hexdigest_mock = md5_mock.return_value.hexdigest + hexdigest_mock.return_value = 'invalid-checksum' + self.assertRaises(errors.ImageChecksumError, + image_download.verify_image, + image_location) @mock.patch('ironic_lib.disk_utils.get_disk_identifier', lambda dev: 'ROOT') @@ -945,7 +1035,8 @@ class TestImageDownload(base.IronicAgentTest): requests_mock.assert_called_once_with(image_info['urls'][0], cert=None, verify=True, stream=True, proxies={}) - self.assertEqual(image_info['checksum'], image_download.md5sum()) + self.assertEqual(image_info['checksum'], + image_download._hash_algo.hexdigest()) @mock.patch('time.time', autospec=True) @mock.patch('requests.get', autospec=True) diff --git a/releasenotes/notes/enhance-checksum-2256ffdcce13836e.yaml b/releasenotes/notes/enhance-checksum-2256ffdcce13836e.yaml new file mode 100644 index 000000000..1d630f294 --- /dev/null +++ b/releasenotes/notes/enhance-checksum-2256ffdcce13836e.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Adds enhanced checksum support to IPA, when ``os_hash_algo`` and + ``os_hash_value`` are passed in via ``image_info``, it will be used + for image checksum calculation and verification. The md5 checksum + is supported if these information are absent. \ No newline at end of file