generate random passwords using minclass

[xiaoge1001]

Proposed Commit Message

Fix the complexity of random passwords generated by the rand_user_password() method.

The complexity of the random password generated by the rand_user_password() method may not meet the security configuration requirements of the system authentication module. chpasswd will fail.

Fixes GH-5814

Additional Context

Test Steps

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[TheRealFalcon]

Thanks for the enhancement here. I left an inline comment. Also, it'd be good to get a unit test for the new behavior ensuring that one of each type of character class is included in a generated password strong.

[TheRealFalcon]

I'd prefer a solution that looks something this this rather than looping an indeterminate number of times.

[xiaoge1001]

Thanks for the enhancement here. I left an inline comment. Also, it'd be good to get a unit test for the new behavior ensuring that one of each type of character class is included in a generated password strong.

I have added a test case named test_get_str_class_num, please review it.

[TheRealFalcon]

Thanks for the updates here.

I don't think there is really an easy way to restrict the generated characters to the original PW_SET while also incorporating the minimum character set. It appears the previous restriction was related to confusion when typing characters manually, which is a fairly arbitrary concern we don't need to concern ourselves with. I think it is time to do away with it.

Beyond that, I don't see a reason to allow specifying an arbitrary amount of character classes. That may be useful for a user-generated password, but for a system generated password, let's just include every class we know about.

Finally, I don't see a good reason to have a separate utility function with a single caller. Let's just keep the implementation in cc_set_passwords.py. It can always be made more general later if needed.

I made these changes and included them in the following patch. They should be able to be git applyed to your current branch. If you're ok with the changes, please apply them and push the result. Also let me know if you have any concerns.

diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
index 058cc22fa..8cb6a1ec5 100644
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -8,8 +8,9 @@
 """Set Passwords: Set user passwords and enable/disable SSH password auth"""
 
 import logging
+import random
 import re
-from string import ascii_letters, digits
+import string
 from typing import List
 
 from cloudinit import features, lifecycle, subp, util
@@ -30,9 +31,6 @@ meta: MetaSchema = {
 
 LOG = logging.getLogger(__name__)
 
-# We are removing certain 'painful' letters/numbers
-PW_SET = "".join([x for x in ascii_letters + digits if x not in "loLOI01"])
-
 
 def get_users_by_type(users_list: list, pw_type: str) -> list:
     """either password or type: RANDOM is required, user is always required"""
@@ -248,4 +246,29 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
 
 
 def rand_user_password(pwlen=20):
-    return util.rand_str_minclass(pwlen, minclass=3, select_from=PW_SET)
+    if pwlen < 4:
+        raise ValueError("Password length must be at least 4 characters.")
+
+    # There are often restrictions on the minimum number of character
+    # classes required in a password, so ensure we at least one character
+    # from each class.
+    res_rand_list = [
+        random.choice(string.digits),
+        random.choice(string.ascii_lowercase),
+        random.choice(string.ascii_uppercase),
+        random.choice(string.punctuation),
+    ]
+
+    res_rand_list.extend(
+        list(
+            util.rand_str(
+                pwlen - len(res_rand_list),
+                select_from=string.digits
+                + string.ascii_lowercase
+                + string.ascii_uppercase
+                + string.punctuation,
+            )
+        )
+    )
+    random.shuffle(res_rand_list)
+    return "".join(res_rand_list)
diff --git a/cloudinit/util.py b/cloudinit/util.py
index f5de6e43a..8025f4d51 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -299,31 +299,6 @@ def rand_str(strlen=32, select_from=None):
     return "".join([r.choice(select_from) for _x in range(strlen)])
 
 
-def rand_str_minclass(strlen=32, minclass=3, select_from=None):
-    if strlen <= 0:
-        strlen = 32
-        LOG.warning("strlen <= 0, use default value(32)")
-
-    if minclass <= 0:
-        minclass = 3
-        LOG.warning("minclass <= 0, use default value(3)")
-
-    res_rand_list = [
-        random.choice(string.digits),
-        random.choice(string.ascii_lowercase),
-        random.choice(string.ascii_uppercase),
-    ]
-    if minclass > 3:
-        res_rand_list.append(random.choice(string.punctuation))
-        if select_from:
-            select_from += string.punctuation
-
-    res_rand_list = random.sample(res_rand_list, min(minclass, strlen, 4))
-    res_rand_list.extend(list(rand_str(strlen - minclass, select_from)))
-    random.shuffle(res_rand_list)
-    return "".join(res_rand_list)
-
-
 def rand_dict_key(dictionary, postfix=None):
     if not postfix:
         postfix = ""
diff --git a/tests/unittests/config/test_cc_set_passwords.py b/tests/unittests/config/test_cc_set_passwords.py
index bc6f4cbda..671229de1 100644
--- a/tests/unittests/config/test_cc_set_passwords.py
+++ b/tests/unittests/config/test_cc_set_passwords.py
@@ -2,6 +2,7 @@
 
 import copy
 import logging
+import string
 from unittest import mock
 
 import pytest
@@ -559,6 +560,40 @@ class TestExpire:
             assert "Expired passwords" not in caplog.text
 
 
+class TestRandUserPassword:
+    def _get_str_class_num(self, str):
+        return sum(
+            [
+                any(c.islower() for c in str),
+                any(c.isupper() for c in str),
+                any(c.isupper() for c in str),
+                any(c in string.punctuation for c in str),
+            ]
+        )
+
+    @pytest.mark.parametrize(
+        "strlen, expected_result",
+        [
+            (1, ValueError),
+            (2, ValueError),
+            (3, ValueError),
+            (4, 4),
+            (5, 4),
+            (5, 4),
+            (6, 4),
+            (20, 4),
+        ],
+    )
+    def test_get_str_class_num(self, strlen, expected_result):
+        if expected_result is ValueError:
+            with pytest.raises(expected_result):
+                setpass.rand_user_password(strlen)
+        else:
+            rand_password = setpass.rand_user_password(strlen)
+            assert len(rand_password) == strlen
+            assert self._get_str_class_num(rand_password) == expected_result
+
+
 class TestSetPasswordsSchema:
     @pytest.mark.parametrize(
         "config, expectation",
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 176b4b3e4..221e21de5 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -3369,64 +3369,3 @@ class TestLogExc:
             ),
             ("tests.unittests.test_util", logging.DEBUG, "an error occurred"),
         ]
-
-
-class TestRandStr:
-    def _get_str_class_num(self, str):
-        import string
-
-        str_class_num = 0
-        if any(c.islower() for c in str):
-            str_class_num += 1
-        if any(c.isupper() for c in str):
-            str_class_num += 1
-        if any(c.isdigit() for c in str):
-            str_class_num += 1
-        if any(c in string.punctuation for c in str):
-            str_class_num += 1
-        return str_class_num
-
-    @pytest.mark.parametrize(
-        "strlen, param_minclass, expected_minclass, expected_maxclass",
-        [
-            (1, 1, 1, 1),
-            (1, 2, 1, 1),
-            (1, 3, 1, 1),
-            (1, 4, 1, 1),
-            (2, 1, 1, 2),
-            (2, 2, 2, 2),
-            (2, 3, 2, 2),
-            (2, 4, 2, 2),
-            (3, 1, 1, 3),
-            (3, 2, 2, 3),
-            (3, 3, 3, 3),
-            (3, 4, 3, 3),
-            (4, 1, 1, 3),
-            (4, 2, 2, 3),
-            (4, 3, 3, 3),
-            (4, 4, 4, 4),
-            (5, 1, 1, 3),
-            (5, 2, 2, 3),
-            (5, 3, 3, 3),
-            (5, 4, 4, 4),
-            (5, 5, 4, 4),
-            (6, 1, 1, 3),
-            (6, 2, 2, 3),
-            (6, 3, 3, 3),
-            (6, 4, 4, 4),
-            (6, 5, 4, 4),
-            (6, 6, 4, 4),
-            (20, 1, 1, 3),
-            (20, 2, 2, 3),
-            (20, 3, 3, 3),
-            (20, 4, 4, 4),
-            (20, 5, 4, 4),
-        ],
-    )
-    def test_get_str_class_num(
-        self, strlen, param_minclass, expected_minclass, expected_maxclass
-    ):
-        res_rand_str = util.rand_str_minclass(strlen, param_minclass)
-        actual_class = self._get_str_class_num(res_rand_str)
-        assert actual_class >= expected_minclass
-        assert actual_class <= expected_maxclass

[TheRealFalcon]

Select from specifies the exact characters we can possibly use, so this doesn't really work the same.

[xiaoge1001]

In this case, a random password defaults to containing four classes random characters.

[xiaoge1001]

There are currently 9 commit, I will merge into 1 commit.

[TheRealFalcon]

Thanks!