Review requests

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>

lib/ansible/module_utils/basic.py

@@ -1459,6 +1459,10 @@ class AnsibleModule(object):
 
     def digest_from_file(self, filename, algorithm):
         """ Return hex digest of local file for a digest_method specified by name, or None if file is not present. """
+        self.deprecate(
+            msg="API digest_from_file is deprecated. Use generate_secure_file_checksum instead.",
+            version="2.22"
+        )
         b_filename = to_bytes(filename, errors='surrogate_or_strict')
 
         if not os.path.exists(b_filename):
@@ -1475,18 +1479,17 @@ class AnsibleModule(object):
                 self.module.fail_json(
                     msg=f"Could not find file {filename} to calculate hash: {e}"
                 )
-
+            except ValueError as e:
-        if algorithm not in AVAILABLE_HASH_ALGORITHMS:
+                self.module.fail_json(msg=f"{e}")
-            self.fail_json(
-                msg=f"Could not hash file '{filename}' with algorithm '{algorithm}'. Available algorithms: {', '.join(AVAILABLE_HASH_ALGORITHMS)}"
-            )
 
         try:
-            return generate_secure_file_checksum(filename, hash_func=AVAILABLE_HASH_ALGORITHMS[algorithm])
+            return generate_secure_file_checksum(filename, hash_func=algorithm)
         except IOError as e:
             self.module.fail_json(
                 msg=f"Could not find file {filename} to calculate hash: {e}"
             )
+        except ValueError as e:
+            self.module.fail_json(msg=f"{e}")
 
     def md5(self, filename):
         """ Return MD5 hex digest of local file using digest_from_file().
@@ -1499,16 +1502,28 @@ class AnsibleModule(object):
 
         Most uses of this function can use the module.sha1 function instead.
         """
+        self.deprecate(
+            msg="Using md5 is not recommended. Use generate_secure_file_checksum with SHA256 instead.",
+            version="2.22"
+        )
         if 'md5' not in AVAILABLE_HASH_ALGORITHMS:
             raise ValueError('MD5 not available.  Possibly running in FIPS mode')
         return self.digest_from_file(filename, 'md5')
 
     def sha1(self, filename):
         """ Return SHA1 hex digest of local file using digest_from_file(). """
+        self.deprecate(
+            msg="Using SHA1 is not recommended. Use generate_secure_file_checksum with SHA256 instead.",
+            version="2.22"
+        )
         return self.digest_from_file(filename, 'sha1')
 
     def sha256(self, filename):
         """ Return SHA-256 hex digest of local file using digest_from_file(). """
+        self.deprecate(
+            msg="Use generate_secure_file_checksum with SHA256 instead.",
+            version="2.22"
+        )
         return self.digest_from_file(filename, 'sha256')
 
     def backup_local(self, fn):

lib/ansible/module_utils/common/hashing.py

@@ -15,6 +15,7 @@ except ImportError:
     _md5 = None  # type: ignore[assignment]
 
 from ansible.module_utils.common.text.converters import to_bytes
+from ansible.module_utils.six import string_types
 
 
 def _get_available_hash_algorithms():
@@ -39,16 +40,26 @@ def _get_available_hash_algorithms():
 AVAILABLE_HASH_ALGORITHMS = _get_available_hash_algorithms()
 
 
-def generate_secure_checksum(data, hash_func=hashlib.sha256):
+def generate_secure_checksum(data, hash_func='sha256'):
     """Generates a secure checksum for the given data using the specified hash function.
 
     Args:
         data: The data to be hashed.
-        hash_func: The hash function to use (default: hashlib.sha256).
+        hash_func: The hash function to use (default: sha256).
 
     Returns:
         A hexadecimal string representing the checksum.
     """
+    if hash_func is None:
+        raise ValueError("The parameter 'hash_func' can not be None")
+
+    if isinstance(hash_func, string_types):
+        if hash_func not in AVAILABLE_HASH_ALGORITHMS:
+            raise ValueError(f"{hash_func} is not available. Available algorithms: {', '.join(AVAILABLE_HASH_ALGORITHMS)}")
+        hash_func = getattr(hashlib, hash_func, None)
+
+    if not callable(hash_func):
+        raise ValueError("The parameter value of 'hash_func' is not callable. Please make sure hash_func is either string or method.")
 
     digest = hash_func()
     data = to_bytes(data, errors='surrogate_or_strict')
@@ -56,12 +67,12 @@ def generate_secure_checksum(data, hash_func=hashlib.sha256):
     return digest.hexdigest()
 
 
-def generate_secure_file_checksum(filename, hash_func=hashlib.sha256, write_to=None):
+def generate_secure_file_checksum(filename, hash_func='sha256', write_to=None):
     """Return a secure hash hex digest of local file, None if file is not present or a directory.
 
     Args:
         filename: The filename to be hashed.
-        hash_func: The hash function to use (default: hashlib.sha256).
+        hash_func: The hash function to use (default: sha256).
         write_to: The file handle to write to (default: None).
 
     Returns:
@@ -72,6 +83,17 @@ def generate_secure_file_checksum(filename, hash_func=hashlib.sha256, write_to=N
     if not os.path.exists(b_filename) or os.path.isdir(to_bytes(filename, errors='strict')):
         return None
 
+    if hash_func is None:
+        raise ValueError("The parameter 'hash_func' can not be None")
+
+    if isinstance(hash_func, string_types):
+        if hash_func not in AVAILABLE_HASH_ALGORITHMS:
+            raise ValueError(f"{hash_func} is not available. Available algorithms: {', '.join(AVAILABLE_HASH_ALGORITHMS)}")
+        hash_func = getattr(hashlib, hash_func, None)
+
+    if not callable(hash_func):
+        raise ValueError("The parameter value of 'hash_func' is not callable. Please make sure hash_func is either string or method.")
+
     digest = hash_func()
     blocksize = 64 * 1024
     try:

test/units/utils/test_hashing.py

@@ -14,21 +14,39 @@ import pytest
 
 
 secure_hash_testdata = [
-    pytest.param(hashlib.sha1, "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3", id="sha1"),
+    pytest.param("sha1", "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3", id="sha1_str"),
+    pytest.param(
+        "sha224",
+        "90a3ed9e32b2aaf4c61c410eb925426119e1a9dc53d4286ade99a809",
+        id="sha224_str",
+    ),
+    pytest.param(
+        "sha256",
+        "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08",
+        id="sha256_str",
+    ),
+    pytest.param(
+        "sha384",
+        "768412320f7b0aa5812fce428dc4706b3cae50e02a64caa16a782249bfe8efc4b7ef1ccb126255d196047dfedf17a0a9",
+        id="sha384_str",
+    ),
+    pytest.param(
+        hashlib.sha1, "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3", id="sha1_call"
+    ),
     pytest.param(
         hashlib.sha224,
         "90a3ed9e32b2aaf4c61c410eb925426119e1a9dc53d4286ade99a809",
-        id="sha224",
+        id="sha224_call",
     ),
     pytest.param(
         hashlib.sha256,
         "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08",
-        id="sha256",
+        id="sha256_call",
     ),
     pytest.param(
         hashlib.sha384,
         "768412320f7b0aa5812fce428dc4706b3cae50e02a64caa16a782249bfe8efc4b7ef1ccb126255d196047dfedf17a0a9",
-        id="sha384",
+        id="sha384_call",
     ),
 ]
 
@@ -52,3 +70,16 @@ def test_generate_secure_file_checksum(hash_func, expected):
         assert (
             hashing.generate_secure_file_checksum(text_file.name, hash_func) == expected
         )
+
+
+def test_generate_secure_checksum_fail_none():
+    with pytest.raises(ValueError, match=r"^The parameter 'hash_func'"):
+        hashing.generate_secure_checksum("test", hash_func=None)
+
+
+def test_generate_secure_file_checksum_fail_none():
+    with pytest.raises(ValueError, match=r"^The parameter 'hash_func'"):
+        with tempfile.NamedTemporaryFile() as text_file:
+            text_file.write(to_bytes("test"))
+            text_file.flush()
+            hashing.generate_secure_file_checksum(text_file.name, hash_func=None)