saltstack / saltstack/salt

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

Open
#67,118 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Python
Stars
15.7k
Forks
5.6k
Avg merge
2d 44m
Merged PRs (30d)
80

Description

$ 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: 58e415df0204bd2516f06848378237ecfb27b96d)

This always returns HAS_CRYPT=True because that code imports salt/utils/crypt.py (introduced: 8c936666bf919a53433e652d97ef62b9c5f3a4c5 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in salt/utils/pycrypto.py, reading the crypt import, gen_hash, and secure_password paths, then compare the Python 3.13 failure with the existing passlib and hashlib options described here. Done means Salt no longer raises errors because of the removed crypt module and password hashing retains the supported algorithm behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.