Enhanced checksum support

Adds enhanced checksum support to IPA, when os_hash_algo and os_hash_value
are passed in via image_info, it will be used to calculate image checksum
and verification.

In other cases, the old md5 checksum is used.

Change-Id: I1d2f33e7059910326b4ac3f7786543b333a93a5a
Story: 2003938
Task: 26846

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):
+    def verify_image(self, image_location):
-        """Computes and returns the md5 checksum of the downloaded image.
-
-        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.
-
-        :returns: The md5 checksum of the image as a string in hexadecimal.
-        """
-        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
+        If this function does not raise ImageChecksumError then it is very
-    that the local copy of the image was transmitted and stored correctly.
+        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
-    :raises: ImageChecksumError if the checksum of the local image does not
+                 not match the checksum as reported by glance in image_info.
-             match the checksum as reported by glance in image_info.
         """
-    LOG.debug('Verifying image at {} against MD5 checksum '
+        checksum = self._hash_algo.hexdigest()
-              '{}'.format(image_location, checksum))
+        LOG.debug('Verifying image at {} against {} checksum '
-    if checksum != image_info['checksum']:
+                  '{}'.format(image_location, self._hash_algo.name, checksum))
+        if checksum != self._expected_hash_value:
             LOG.error(errors.ImageChecksumError.details_str.format(
-            image_location, image_info['id'],
+                image_location, self._image_info['id'],
-            image_info['checksum'], checksum))
+                self._expected_hash_value, checksum))
-        raise errors.ImageChecksumError(image_location, image_info['id'],
+            raise errors.ImageChecksumError(image_location,
-                                        image_info['checksum'], checksum)
+                                            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):

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']
+        image_download = standby.ImageDownload(image_info)
-        standby._verify_image(image_info, image_location, checksum)
+        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_download.verify_image,
-                          image_info, image_location, checksum)
+                          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)

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.