Bug 8159 - crypt module is deprecated
Summary: crypt module is deprecated
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Server Installer (show other bugs)
Version: trunk
Hardware: PC Linux
: P2 Normal
Target Milestone: 4.16.0
Assignee: Alexander Zeijlon
Keywords: linma_tester, relnotes
Depends on:
Reported: 2023-05-24 16:09 CEST by Alexander Zeijlon
Modified: 2023-12-13 14:56 CET (History)
2 users (show)

See Also:
Acceptance Criteria:
MUST: * Lib/crypt.py must not be used, since it is deprecated. * After fix, TL-admins should be able to authenticate.


Description Alexander Zeijlon cendio 2023-05-24 16:09:43 CEST
When running the installer, python gives the following deprecation warning:
>/opt/thinlinc/modules/thinlinc/crypt.py:11: DeprecationWarning: 'crypt' is deprecated and slated for removal in Python 3.13
>  import crypt as orig_crypt
Installer running on Fedora 38, Python 3.11.3.
Comment 1 Alexander Zeijlon cendio 2023-08-28 14:40:25 CEST
The crypt-module is only directly used in one file in our code. And there its crypt-function is used to encrypt passwords if the supplied salt-string is not using one of the methods SHA1 [1], SHA256 or SHA512.

[1] We do not allow passwords to be encrypted with SHA1.
Comment 2 Alexander Zeijlon cendio 2023-08-28 14:59:27 CEST
This means that it could be used (successfully) for the encryption methods:
* crypt.METHOD_MD5

Hence, we need to make sure to support any of these [1] if they have been used by us, as the method of choice for encrypting passwords in the past. Otherwise, we would risk disabling logins for admins, migrating from older versions of TL.

[1] Without using Lib/crypt.py.
Comment 3 Alexander Zeijlon cendio 2023-08-29 07:29:57 CEST
Since we already have bug 7740, which discusses replacing SHA512 with something more modern at some point, we can set the scope here to only getting rid of the dependency on the crypt-module.
Comment 4 Alexander Zeijlon cendio 2023-08-29 10:11:39 CEST
In r28216 (commit year 2013) as a part of bug 4918, we switched over from our own key stretching algorithm to SHA512.

Additionally, in bug 4918 comment 5, it is mentioned that actions were taken at that point to handle the issue of using a newer method for logins that previously used the old one.

This means that we do not need to support any of the methods mentioned in comment 2.
Comment 6 Alexander Zeijlon cendio 2023-08-29 15:45:29 CEST
The test cases in test_crypt.py has been updated to reflect that we are now:
* Not using the Lib/crypt.py
* Currently only allowing our own crypt.py to encrypt with SHA-2 [1].

[1] Again, the scope of this bug is not to implement the usage of a better method/algorithm

Additionally, I tested that:
* The installer still manages to set a new password for the admin account without warnings or errors.
* tl-gen-auth is still able to create valid encrypted passwords with SHA512.
* It is possible to log in to tlwebadm as admin with the set password.

This bug is now fixed.
Comment 7 Samuel Mannehed cendio 2023-08-29 19:37:59 CEST
See bug 8162 comment 12 for estimates as to which future distros this removal is likely to affect. That bug also regards a Python module scheduled to be removed in version 3.13.
Comment 8 Linn cendio 2023-08-31 16:24:00 CEST
Tested acceptance criteria for server build 3367 on SLES 12 where I had removed crypt from the Python standard lib.


 ✅ Lib/crypt.py must not be used, since it is deprecated.
Tested the scenarios below when crypt was removed from the standard lib, and things worked well.

 ✅ After fix, TL-admins should be able to authenticate.
Authentication in webadmin works well. Tested the following:
 - tl-setup sets the expected password
 - The password appears in the right format in tlwebadm.hconf
 - Manually created a password hash with tl-gen-auth and set it in tlwebadm.hconf
 - Could log in to webadmin with the expected password

Also checked that the deprecation warning (seen on Python 3.11 or later) had disappeared when running build 3367 on Fedora 37 with Python 3.11. The warning was previously printed at the start of tl-setup.

Also checked the commits, looks good. Closing!

Note You need to log in before you can comment on or make changes to this bug.