There was some question as to whether /dev/urandom was being used for generating user password salts, this makes it explicit and reuses the same code for the bootloader password generation.
Also adds a test for the new function, iutil.encrypt_password
Resolves: rhbz#1229474
From: "Brian C. Lane" bcl@redhat.com
There was some question as to whether /dev/urandom was being used for generating user password salts, this makes it explicit and reuses the same code for the bootloader password generation.
Also adds a test for the new function, iutil.encrypt_password
Resolves: rhbz#1229474 --- pyanaconda/bootloader.py | 13 ++----------- pyanaconda/constants.py | 1 + pyanaconda/iutil.py | 18 +++++++++++++++++- pyanaconda/users.py | 18 ++---------------- tests/pyanaconda_tests/iutil_test.py | 10 ++++++++++ 5 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/pyanaconda/bootloader.py b/pyanaconda/bootloader.py index 264f8a6..1e1a95e 100644 --- a/pyanaconda/bootloader.py +++ b/pyanaconda/bootloader.py @@ -1104,17 +1104,8 @@ def _encrypt_password(self): if not self.password: raise BootLoaderError("cannot encrypt empty password")
- # Used for ascii_letters and digits constants - import string # pylint: disable=deprecated-module - import crypt - import random - salt = "$6$" - salt_len = 16 - salt_chars = string.ascii_letters + string.digits + './' - - rand_gen = random.SystemRandom() - salt += "".join(rand_gen.choice(salt_chars) for i in range(salt_len)) - self.encrypted_password = crypt.crypt(self.password, salt) + # Encrypt using sha512 and 16 character salt + self.encrypted_password = iutil.encrypt_password(self.password, "$6$", 16)
def write_config_password(self, config): """ Write password-related configuration. """ diff --git a/pyanaconda/constants.py b/pyanaconda/constants.py index df1a171..0141aad 100644 --- a/pyanaconda/constants.py +++ b/pyanaconda/constants.py @@ -152,6 +152,7 @@
# all ASCII characters PW_ASCII_CHARS = string.digits + string.ascii_letters + string.punctuation + " " +SALT_CHARS = string.ascii_letters + string.digits + './'
# Recognizing a tarfile TAR_SUFFIX = (".tar", ".tbz", ".tgz", ".txz", ".tar.bz2", "tar.gz", "tar.xz") diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py index 5a327ba..46506c5 100644 --- a/pyanaconda/iutil.py +++ b/pyanaconda/iutil.py @@ -35,6 +35,8 @@ import gettext import signal import sys +import crypt +import random
import requests from requests_file import FileAdapter @@ -47,7 +49,7 @@
from pyanaconda.flags import flags from pyanaconda.constants import DRACUT_SHUTDOWN_EJECT, TRANSLATIONS_UPDATE_DIR, UNSUPPORTED_HW -from pyanaconda.constants import SCREENSHOTS_DIRECTORY, SCREENSHOTS_TARGET_DIRECTORY +from pyanaconda.constants import SCREENSHOTS_DIRECTORY, SCREENSHOTS_TARGET_DIRECTORY, SALT_CHARS from pyanaconda.regexes import URL_PARSE
from pyanaconda.i18n import _ @@ -1373,3 +1375,17 @@ def save_screenshots():
except OSError: log.exception("saving screenshots to installed system failed") + +def encrypt_password(password, algo, salt_len): + """ Encrypt a password using the selected algorithm and salt length. + + :param str password: password to encrypt + :param str algo: Algorithm to use, from crypt(3) manpage + :param int salt_len: Lenth of salt to generate + + This uses the system urandom as the random number generator + """ + salt = algo or "" + rand_gen = random.SystemRandom() + salt += "".join(rand_gen.choice(SALT_CHARS) for i in range(salt_len)) + return crypt.crypt(password, salt) diff --git a/pyanaconda/users.py b/pyanaconda/users.py index 4fc38d1..9f6035a 100644 --- a/pyanaconda/users.py +++ b/pyanaconda/users.py @@ -20,9 +20,6 @@ #
# Used for ascii_letters and digits constants -import string # pylint: disable=deprecated-module -import crypt -import random import os import os.path import subprocess @@ -58,21 +55,10 @@ def getPassAlgo(authconfigStr): # $6$ SHA512 def cryptPassword(password, algo=None): salts = {'md5': '$1$', 'sha256': '$5$', 'sha512': '$6$'} - saltlen = 2 - - if algo is None: + if algo not in salts: algo = 'sha512'
- if algo == 'md5' or algo == 'sha256' or algo == 'sha512': - saltlen = 16 - - saltstr = salts[algo] - - for _i in range(saltlen): - saltstr = saltstr + random.choice(string.ascii_letters + - string.digits + './') - - cryptpw = crypt.crypt(password, saltstr) + cryptpw = iutil.encrypt_password(password, salts[algo], 16) if cryptpw is None: exn = PasswordCryptError(algo=algo) if errorHandler.cb(exn) == ERROR_RAISE: diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py index 55d5df2..a286ea6 100644 --- a/tests/pyanaconda_tests/iutil_test.py +++ b/tests/pyanaconda_tests/iutil_test.py @@ -28,6 +28,7 @@ import tempfile import signal import shutil +import crypt from .test_constants import ANACONDA_TEST_DIR
from timer import timer @@ -807,3 +808,12 @@ def open_with_perm_test(self): os.umask(old_umask) finally: shutil.rmtree(test_dir) + +class EncryptPasswordTests(unittest.TestCase): + def encrypt_password_test(self): + """ Test the encrypt_password function""" + for algo in ["$1$", "$5$", "$6$"]: + enc_pw = iutil.encrypt_password("DocBrown", algo, 16) + self.assertEqual(algo, enc_pw[:3]) + self.assertEqual(crypt.crypt("DocBrown", enc_pw), enc_pw) + self.assertNotEqual(crypt.crypt("Einstein", enc_pw), enc_pw)
@@ -807,3 +808,12 @@ def open_with_perm_test(self): os.umask(old_umask) finally: shutil.rmtree(test_dir)
+class EncryptPasswordTests(unittest.TestCase):
- def encrypt_password_test(self):
""" Test the encrypt_password function"""for algo in ["$1$", "$5$", "$6$"]:enc_pw = iutil.encrypt_password("DocBrown", algo, 16)self.assertEqual(algo, enc_pw[:3])self.assertEqual(crypt.crypt("DocBrown", enc_pw), enc_pw)self.assertNotEqual(crypt.crypt("Einstein", enc_pw), enc_pw)
Looking at this makes me wonder why we are not using ``crypt.crypt()`` instead of our custom code? Looks like it even got some improvements with Python 3 which were backported to 2.7.
Looks good to me except for the above question.
Added label: ACK.
@@ -807,3 +808,12 @@ def open_with_perm_test(self): os.umask(old_umask) finally: shutil.rmtree(test_dir)
+class EncryptPasswordTests(unittest.TestCase):
- def encrypt_password_test(self):
""" Test the encrypt_password function"""for algo in ["$1$", "$5$", "$6$"]:enc_pw = iutil.encrypt_password("DocBrown", algo, 16)self.assertEqual(algo, enc_pw[:3])self.assertEqual(crypt.crypt("DocBrown", enc_pw), enc_pw)self.assertNotEqual(crypt.crypt("Einstein", enc_pw), enc_pw)
Looking at this makes me wonder why we are not using crypt.crypt()
We do use crypt.crypt (see iutil.encrypt_password), this is just testing that the created password works.
@@ -807,3 +808,12 @@ def open_with_perm_test(self): os.umask(old_umask) finally: shutil.rmtree(test_dir)
+class EncryptPasswordTests(unittest.TestCase):
- def encrypt_password_test(self):
""" Test the encrypt_password function"""for algo in ["$1$", "$5$", "$6$"]:enc_pw = iutil.encrypt_password("DocBrown", algo, 16)self.assertEqual(algo, enc_pw[:3])self.assertEqual(crypt.crypt("DocBrown", enc_pw), enc_pw)self.assertNotEqual(crypt.crypt("Einstein", enc_pw), enc_pw)
I saw that. What I meant above is that AFAICT we duplicate things that ``crypt.crypt()`` now can do for us. There's ``crypt.mksalt()`` and ``crypt.crypt()`` can generate the salt on it's own if we give it the hashing algorithm. The only missing thing seems to be the length of the salt, but I'm not sure if we really use that with any non-standard lengths.
@@ -807,3 +808,12 @@ def open_with_perm_test(self): os.umask(old_umask) finally: shutil.rmtree(test_dir)
+class EncryptPasswordTests(unittest.TestCase):
- def encrypt_password_test(self):
""" Test the encrypt_password function"""for algo in ["$1$", "$5$", "$6$"]:enc_pw = iutil.encrypt_password("DocBrown", algo, 16)self.assertEqual(algo, enc_pw[:3])self.assertEqual(crypt.crypt("DocBrown", enc_pw), enc_pw)self.assertNotEqual(crypt.crypt("Einstein", enc_pw), enc_pw)
For 3.4 and later crypt.mksalt is ok, for 2.7 it isn't. Salt length is really always 16 since we don't allow DES. I guess I could simplify everything for master and leave this for rhel7-branch.
@@ -807,3 +808,12 @@ def open_with_perm_test(self): os.umask(old_umask) finally: shutil.rmtree(test_dir)
+class EncryptPasswordTests(unittest.TestCase):
- def encrypt_password_test(self):
""" Test the encrypt_password function"""for algo in ["$1$", "$5$", "$6$"]:enc_pw = iutil.encrypt_password("DocBrown", algo, 16)self.assertEqual(algo, enc_pw[:3])self.assertEqual(crypt.crypt("DocBrown", enc_pw), enc_pw)self.assertNotEqual(crypt.crypt("Einstein", enc_pw), enc_pw)
Sounds great to me.
Added label: rhel7-branch.
Pushed.
Closed.
anaconda-patches@lists.fedorahosted.org
-
dashea