Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Salt relies on crypt module, deprecated in 3.11 and removed in 3.13 #67118

Open
Vaelatern opened this issue Dec 23, 2024 · 1 comment
Open
Labels
Bug broken, incorrect, or confusing behavior needs-triage

Comments

@Vaelatern
Copy link
Contributor

$ python3
Python 3.13.1 (main, Dec 14 2024, 12:44:03) [GCC 13.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import crypt
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    import crypt
ModuleNotFoundError: No module named 'crypt'
$ python3
Python 3.12.6 (main, Sep  7 2024, 15:48:22) [GCC 13.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import crypt
<stdin>:1: DeprecationWarning: 'crypt' is deprecated and slated for removal in Python 3.13

We use, in salt/utils/pycrypto.py, the following stanzas:

try:
    import crypt

    HAS_CRYPT = True
except ImportError:
    HAS_CRYPT = False

(introduced: 58e415d)

This always returns HAS_CRYPT=True because that code imports salt/utils/crypt.py (introduced: 8c93666 which is 3 years later).

This HAS_CRYPT variable guards a few pieces of code.

if HAS_CRYPT:
    methods = {m.name.lower(): m for m in crypt.methods}
else:
    methods = {}
known_methods = ["sha512", "sha256", "blowfish", "md5", "crypt"]
# ...
    if algorithm is None:
        # prefer the most secure natively supported method
        algorithm = crypt.methods[0].name.lower() if HAS_CRYPT else known_methods[0]
# ...
    if HAS_CRYPT and algorithm in methods:
        return _gen_hash_crypt(
            crypt_salt=crypt_salt, password=password, algorithm=algorithm
        )
    elif HAS_PASSLIB and algorithm in known_methods:

This means that we throw repeatedly during executions:

[ERROR   ] Failed to import utils pycrypto, this is due most likely to a syntax error:
Traceback (most recent call last):
  File "/usr/lib/python3.13/site-packages/salt/loader/lazy.py", line 806, in _load_module
    spec.loader.exec_module(mod)
    ~~~~~~~~~~~~~~~~~~~~~~~^^^^^
  File "<frozen importlib._bootstrap_external>", line 1026, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "/usr/lib/python3.13/site-packages/salt/utils/pycrypto.py", line 105, in <module>
    methods = {m.name.lower(): m for m in crypt.methods}
                                          ^^^^^^^^^^^^^
AttributeError: module 'crypt' has no attribute 'methods'

In fact, now that crypt is obsoleted, crypt.methods and in fact all of the possible options are removed.

There are a few options. One is the stated replacement for some usecases, hashlib.

>>> crypt.methods
[<crypt.METHOD_SHA512>, <crypt.METHOD_SHA256>, <crypt.METHOD_BLOWFISH>, <crypt.METHOD_MD5>, <crypt.METHOD_CRYPT>]
>>> import hashlib
>>> hashlib.algorithms_available
{'sha256', 'sha3_384', 'sha3_512', 'sha384', 'sha3_256', 'sha224', 'sm3', 'shake_256', 'sha512', 'blake2s', 'ripemd160', 'md5', 'sha512_256', 'sha512_224', 'sha1', 'shake_128', 'md5-sha1', 'sha3_224', 'blake2b'}

One important quality: crypt.methods is sorted so crypt.methods[0] is the most secure. hashlib.algorithms_available has no such promise at all, and in fact says due to OpenSSL, some algorithms will appear multiple times under similar names.

I've looked at how to write this code and it seems possible but not as straightforward as with crypt.

The other option is to not bother with a standard library solution and require passlib.

diff --git a/salt/utils/pycrypto.py b/salt/utils/pycrypto.py
index e50ac323eb..f13158a112 100644
--- a/salt/utils/pycrypto.py
+++ b/salt/utils/pycrypto.py
@@ -23,13 +23,6 @@ try:
 except ImportError:
     HAS_RANDOM = False
 
-try:
-    import crypt
-
-    HAS_CRYPT = True
-except (ImportError, PermissionError):
-    HAS_CRYPT = False
-
 try:
     import passlib.context
 
@@ -101,10 +94,6 @@ def secure_password(
         raise CommandExecutionError(str(exc))
 
 
-if HAS_CRYPT:
-    methods = {m.name.lower(): m for m in crypt.methods}
-else:
-    methods = {}
 known_methods = ["sha512", "sha256", "blowfish", "md5", "crypt"]
 
 
@@ -130,26 +119,6 @@ def _gen_hash_passlib(crypt_salt=None, password=None, algorithm=None):
     return ctx.hash(**kwargs)
 
 
-def _gen_hash_crypt(crypt_salt=None, password=None, algorithm=None):
-    """
-    Generate /etc/shadow hash using the native crypt module
-    """
-    if crypt_salt is None:
-        # setting crypt_salt to the algorithm makes crypt generate
-        #  a salt compatible with the specified algorithm.
-        crypt_salt = methods[algorithm]
-    else:
-        if algorithm != "crypt":
-            # all non-crypt algorithms are specified as part of the salt
-            crypt_salt = f"${methods[algorithm].ident}${crypt_salt}"
-
-    try:
-        ret = crypt.crypt(password, crypt_salt)
-    except OSError:
-        ret = None
-    return ret
-
-
 def gen_hash(crypt_salt=None, password=None, algorithm=None):
     """
     Generate /etc/shadow hash
@@ -159,16 +128,12 @@ def gen_hash(crypt_salt=None, password=None, algorithm=None):
 
     if algorithm is None:
         # prefer the most secure natively supported method
-        algorithm = crypt.methods[0].name.lower() if HAS_CRYPT else known_methods[0]
+        algorithm = known_methods[0]
 
     if algorithm == "crypt" and crypt_salt and len(crypt_salt) != 2:
         log.warning("Hash salt is too long for 'crypt' hash.")
 
-    if HAS_CRYPT and algorithm in methods:
-        return _gen_hash_crypt(
-            crypt_salt=crypt_salt, password=password, algorithm=algorithm
-        )
-    elif HAS_PASSLIB and algorithm in known_methods:
+    if HAS_PASSLIB and algorithm in known_methods:
         return _gen_hash_passlib(
             crypt_salt=crypt_salt, password=password, algorithm=algorithm
         )
@@ -177,6 +142,6 @@ def gen_hash(crypt_salt=None, password=None, algorithm=None):
             "Cannot hash using '{}' hash algorithm. Natively supported "
             "algorithms are: {}. If passlib is installed ({}), the supported "
             "algorithms are: {}.".format(
-                algorithm, list(methods), HAS_PASSLIB, known_methods
+                algorithm, [], HAS_PASSLIB, known_methods
             )
         )

I'm happy to formalize this in a PR if anybody finds this useful.

@frebib
Copy link
Contributor

frebib commented Dec 23, 2024

I'm happy to formalize this in a PR if anybody finds this useful.

This would be useful! We are preparing to get Salt 3007/(upcoming) 3008 working on Debian Testing/Trixie which will have python 3.13 soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior needs-triage
Projects
None yet
Development

No branches or pull requests

2 participants