fix: cc_user_groups incorrectly assumes "useradd" never locks password field

[dermotbradley]

Proposed Commit Message

fix: cc_user_groups incorrectly assumes "useradd" never locks password

Currently cc_user_groups assumes that "useradd" never locks the password
field of newly created users. This is an incorrect assumption.

From the useradd manpage:

'-p, --password PASSWORD
The encrypted password, as returned by crypt(3). The default is to
disable the password.'

That is, if cloud-init runs 'useradd' but does not pass it the "-p"
option (with an encrypted password) then the new user's password field
will be locked by "useradd".

cloud-init only passes the "-p" option when calling "useradd" when
user-data specifies the "passwd" option for a new user. For user-data
that specifies either the "hashed_passwd" or "plain_text_passwd"
options instead then cloud-init calls "useradd" without the "-p" option
and so the password field of such a user will be locked by "useradd".

For user-data that specifies "hash_passwd" for a new user then "useradd"
is called with no "-p" option, so causing "useradd" to lock the
password field, however then cloud-init calls "chpasswd -e" to set the
encrypted password which also results in the password field being
unlocked.

For user-data that specifies either "plain_text_passwd" for a new user
then "useradd" is called with no "-p" option, so causing "useradd" to
lock the password. cloud-init then calls "chpasswd" to set the password
which also results in the password field being unlocked.

For user-data that specifies no password at all for a new user then
"useradd" is called with no "-p" option, so causing "useradd" to lock
the password. The password field is left locked.

In all the above scenarios "passwd -l" may be called later by
cloud-init to enforce "lock_passwd: true").

Conversely where "lock_passwd: false" applies the above "usermod"
situation (for "hash_passwd", "plain_text_passwd" or no
password) means that newly created users may have password
fields locked when they should be unlocked.

For Alpine, "adduser" does not support any form of password being
passed and it always locks the password field (the same point
applies about password field being unlocked when/if "chpasswd" is
called). Therefore in some situations (i.e. no password specified
in user-data) the password needs to be unlocked if
"lock_passwd: false".

This PR changes add_user (in both __init__.py and alpine.py) to
explicitly call either lock_passwd or unlock_passwd at all times to
achieve the desired final result.

Additional Context

Test Steps

User-data:

users:
  - name: hash
    hash_passwd: $6$nonsense
    locked_passd: false
  - name: password
    passwd: $6$nonsense
    locked_passd: false
  - name: plain
    plain_text_passwd: test
    locked_passd: false
  - name: none
    locked_passd: false

Then check /etc/shadow to see if all the above users have locked or unlocked password fields.

Checklist

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

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>)

[dermotbradley]

@holmanb I'm guessing your "exception" changes (#5202) suggestion would also apply here to init.py

[dermotbradley]

@blackboxsw @holmanb

I'd like to get this into 24.2 if possible.

[dermotbradley]

Hmm, I just noticed that freebsd.py does not have an unlock_passwd function, whereas netbsd.py and opensdb.py do.

The BSDs are a little strange in that there is no distinction between password locking and account locking/disabling, unlike Linux.

@igalic any thoughts? Would "pw usermod -w none" be safe/sensible for a FreeBSD unlock_passwd function in response to "lock_passwd: false" user-data?

[blackboxsw]

@dermotbradley thank you for putting together this PR and continued refinement of what user setup means on cloud-init. I feel like this represents a behavior change broadly that now allows for setup of password-less user accounts which I didn't think cloud-init allowed previously because of security concerns.

I think we may want to avoid 24.2 for this particular behavior change because I think it warrants an extra set of eyes from our security team to think through the implications of a configured vm w/ user accounts that can be allowed to have empty passwords.

I believe if we can come to a strict requirement in cloud-config configuration that establishes a pattern where the admin can be explicit that they do want a password-less user account, I'd feel better about this approach instead of just keying on lock_passwd: false.

[blackboxsw]

Given that we want security review on this behavior let's push this to 24.3 milestone to make sure we can come up with an acceptable/explicit solution that allows password-less account and test this broadly

[blackboxsw]

Additionally I think we don't actually want to succeed on exit 3 here do we? That means empty password setup didn't work and we noop'd.

Suggested change

	_, err = subp.subp(cmd, rcs=[0, 3])
	subp.subp(cmd)

[dermotbradley]

Addressed.

[blackboxsw]

Additionally I think we need:

• the second subp which call which attempts to delete the user password should not pass on exit 3.
• we should get some unittest coverage of the fallback to delete user passwords when empty string passwd, plain_text_passwd or hashed_passwd.

Here's a combined diff of my suggestions including some additional tests:

diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index 60466eba5..f26420afd 100644
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -833,7 +833,17 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
         if kwargs.get("lock_passwd", True):
             self.lock_passwd(name)
         else:
-            self.unlock_passwd(name)
+            allow_empty_passwd = False
+            for passwd_key in ("passwd", "hashed_passwd", "plain_text_passwd"):
+                if "" == kwargs.get(passwd_key):
+                    allow_empty_passwd = True
+                    LOG.debug(
+                        "Allowing empty password for %s based on empty %s in"
+                        " cloud-config",
+                        name,
+                        passwd_key,
+                    )
+            self.unlock_passwd(name, allow_empty_passwd)
 
         # Configure doas access
         if "doas" in kwargs:
@@ -910,7 +920,7 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
             util.logexc(LOG, "Failed to disable password for user %s", name)
             raise e
 
-    def unlock_passwd(self, name):
+    def unlock_passwd(self, name: str, allow_empty_passwd: bool = False):
         """
         Unlock the password of a user, i.e., enable password logins
         """
@@ -929,6 +939,14 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
             util.logexc(LOG, "Failed to enable password for user %s", name)
             raise e
         if err:
+            if not allow_empty_passwd:
+                LOG.warning(
+                    "Leaving password locked of user %s. lock_passwd: false in"
+                    " user-data but no passwd, plain_text_passwd or"
+                    " hashed_passwd provided",
+                    name,
+                )
+                return
             # if "passwd" or "usermod" are unable to unlock an account with
             # an empty password then they display a message on stdout. In
             # that case then instead set a blank password.
@@ -947,7 +965,7 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
                     "  Tried: %s." % (name, [c[0] for c in unlock_tools])
                 ) from e
             try:
-                _, err = subp.subp(cmd, rcs=[0, 3])
+                subp.subp(cmd)
             except Exception as e:
                 util.logexc(
                     LOG, "Failed to set blank password for user %s", name
diff --git a/tests/unittests/distros/test_create_users.py b/tests/unittests/distros/test_create_users.py
index 3e13316c9..459820433 100644
--- a/tests/unittests/distros/test_create_users.py
+++ b/tests/unittests/distros/test_create_users.py
@@ -36,10 +36,11 @@ class TestCreateUser:
         )
 
     @pytest.mark.parametrize(
-        "create_kwargs,expected",
+        "create_kwargs,subp_side_effect, expected",
         [
             pytest.param(
                 {},
+                None,
                 [
                     _useradd2call([USER, "-m"]),
                     mock.call(["passwd", "-l", USER]),
@@ -48,6 +49,7 @@ class TestCreateUser:
             ),
             pytest.param(
                 {"no_create_home": True},
+                None,
                 [
                     _useradd2call([USER, "-M"]),
                     mock.call(["passwd", "-l", USER]),
@@ -56,6 +58,7 @@ class TestCreateUser:
             ),
             pytest.param(
                 {"system": True},
+                None,
                 [
                     _useradd2call([USER, "--system", "-M"]),
                     mock.call(["passwd", "-l", USER]),
@@ -64,6 +67,7 @@ class TestCreateUser:
             ),
             pytest.param(
                 {"create_no_home": False},
+                None,
                 [
                     _useradd2call([USER, "-m"]),
                     mock.call(["passwd", "-l", USER]),
@@ -72,14 +76,82 @@ class TestCreateUser:
             ),
             pytest.param(
                 {"lock_passwd": False},
+                None,
                 [
                     _useradd2call([USER, "-m"]),
                     mock.call(["passwd", "-u", USER], rcs=[0, 3]),
                 ],
                 id="unlocked",
             ),
+            pytest.param(
+                {"lock_passwd": False},
+                [
+                    ("", ""),
+                    (
+                        "",
+                        "usermod: unlocking the user's password would result in a passwordless account.",
+                    ),
+                ],
+                [
+                    _useradd2call([USER, "-m"]),
+                    mock.call(["passwd", "-u", USER], rcs=[0, 3]),
+                ],
+                id="unlocked_without_empty_string_passwd_does_not_unlock",
+            ),
+            pytest.param(
+                {"lock_passwd": False, "passwd": ""},
+                [
+                    ("", ""),
+                    (
+                        "",
+                        "usermod: unlocking the user's password would result in a passwordless account.",
+                    ),
+                    ("", ""),
+                ],
+                [
+                    _useradd2call([USER, "-m"]),
+                    mock.call(["passwd", "-u", USER], rcs=[0, 3]),
+                    mock.call(["passwd", "-d", USER]),
+                ],
+                id="unlocked_with_empty_string_passwd_does_unlock",
+            ),
+            pytest.param(
+                {"lock_passwd": False, "plain_text_passwd": ""},
+                [
+                    ("", ""),
+                    (
+                        "",
+                        "usermod: unlocking the user's password would result in a passwordless account.",
+                    ),
+                    ("", ""),
+                ],
+                [
+                    _useradd2call([USER, "-m"]),
+                    mock.call(["passwd", "-u", USER], rcs=[0, 3]),
+                    mock.call(["passwd", "-d", USER]),
+                ],
+                id="unlocked_with_empty_string_plain_text_passwd_does_unlock",
+            ),
+            pytest.param(
+                {"lock_passwd": False, "hashed_passwd": ""},
+                [
+                    ("", ""),
+                    (
+                        "",
+                        "usermod: unlocking the user's password would result in a passwordless account.",
+                    ),
+                    ("", ""),
+                ],
+                [
+                    _useradd2call([USER, "-m"]),
+                    mock.call(["passwd", "-u", USER], rcs=[0, 3]),
+                    mock.call(["passwd", "-d", USER]),
+                ],
+                id="unlocked_with_empty_string_hashed_passwd_does_unlock",
+            ),
             pytest.param(
                 {"passwd": "passfoo"},
+                None,
                 [
                     _useradd2call([USER, "--password", "passfoo", "-m"]),
                     mock.call(["passwd", "-l", USER]),
@@ -88,8 +160,13 @@ class TestCreateUser:
             ),
         ],
     )
-    def test_create_options(self, m_subp, dist, create_kwargs, expected):
-        m_subp.return_value = ("", "")
+    def test_create_options(
+        self, m_subp, dist, create_kwargs, subp_side_effect, expected
+    ):
+        if subp_side_effect:
+            m_subp.side_effect = subp_side_effect
+        else:
+            m_subp.return_value = ("", "")
         dist.create_user(name=USER, **create_kwargs)
         assert m_subp.call_args_list == expected
 

[dermotbradley]

I feel like this represents a behavior change broadly that now allows for setup of password-less user accounts which I didn't think cloud-init allowed previously because of security concerns.

@blackboxsw

Well cloud-init has always allowed the setup of password-less user accounts when neither "passwd", "hashed_passwd", nor "plain_text_passwd" are specified for a user.

I assume you meant 'password-less and not-locked user accounts'. cloud-init has always only enforced the locking of passwords whenever "lock_passwd: true" (whether as defined by default in /etc/cloud/cloud.cfg or explicitly specified in user-data).

If "lock_passwd: true" is not specified, indeed if "lock_passwd" is not specified at all, then cloud-init makes no attempt to lock passwords.

I think we may want to avoid 24.2 for this particular behavior change because I think it warrants an extra set of eyes from our security team to think through the implications of a configured vm w/ user accounts that can be allowed to have empty passwords.

Ok.

I believe if we can come to a strict requirement in cloud-config configuration that establishes a pattern where the admin can be explicit that they do want a password-less user account, I'd feel better about this approach instead of just keying on lock_passwd: false.

There are 2 distinct issues here: the issue of empty/blank passwords, and the issue of password-accessible accounts (i.e. via SSH) whether they have passwords or not. Yes the 2 issues are inter-related but they are also distinct issues.

One suggestion would be to deprecate "lock_passwd" and as a replacement to define "unlock_passwd" instead (which is "false" by default) and perhaps to also define "unlock_when_passwd_blank" (again "false" by default) as an additional level of safety.

[dermotbradley]

@blackboxsw

I looked at your comments out of order. I'll look into the combination of lock_passwd: false and passwd: "" as you suggested.

[dermotbradley]

@blackboxsw

How would you suggest treating "lock_passwd" for existing users? i.e. this config:

users:
  - name: existinguser
    lock_passwd: false

Should the code grep /etc/shadow to check whether the existing user's password is blank and only unlock when it is not blank?

[blackboxsw]

@blackboxsw

How would you suggest treating "lock_passwd" for existing users? i.e. this config:

users:
  - name: existinguser
    lock_passwd: false

Should the code grep /etc/shadow to check whether the existing user's password is blank and only unlock when it is not blank?

@dermotbradley That adds a bit of complexity certainly. But, I'm understanding that the use-case here would be to support custom images w/ precreated user account which allows image creators to define an opaque password that we don't want to represent in sensitive user-data. I think we actually may have to treat that case as I believe Azure has cases where they may set lock_passwd = False during preprovisioning and not provide a hashed_password when the password was redacted due to lack of complexity. In those cases where user-exists from an earlier provisioning stage, but we don't represent that in current boot's user-data, we'd probably want to allow that account to login, but only if it has some representation of complexity (like non-empty).

CC: @cjp256 FYI here as this PR in progress touches represents a potential change in behavior as it will explicitly perform an unlock of a account's password via passwd -u <default_username> when lock_passwd: false is provided for the user in configuration. This differs from previous behavior on lock_passwd: false which only would only avoid calling passwd -l <username> in distros.lock_passwd

[dermotbradley]

@dermotbradley That adds a bit of complexity certainly. But, I'm understanding that the use-case here would be to support custom images w/ precreated user account which allows image creators to define an opaque password that we don't want to represent in sensitive user-data.

@blackboxsw I already went ahead and implemented /etc/shadow password value checking for pre-existing user accounts.

I have reworked my changes based on your feedback and manually tested it locally. I haven't yet pushed my changes as I'm trying to determine which tests to modify or add for this.

I could go ahead now and push my code changes (without testcase changes) anyway if you want to have a look at it...

[blackboxsw]

@dermotbradley please feel free to push and I'll understand more may come or you may squash merge testing as added. I could assist if needed on test generation front for this.

[dermotbradley]

@dermotbradley please feel free to push and I'll understand more may come or you may squash merge testing as added. I could assist if needed on test generation front for this.

Ok, I've pushed my current version.

Addressed:

• FreeBSD/OpenBSD changes

Still to be addressed:

• testcases

[dermotbradley]

@blackboxsw could you have a look at this?

[dermotbradley]

Ok, added some DragonflyBSD/FreeBSD/OpenBSD tests.

[blackboxsw]

Thank you @dermotbradley. In your latest squash commit you added these two methods but overrode _check_if_existing_pasword on all BSDs.

Instead of hard-coding minor differences this method in each BSD subclass, can we hang a class attribute shadow_empty_locked_passwd_patterns on the distro class that we override when needed?

It'd look like this:

--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -134,6 +134,8 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
     ci_sudoers_fn = "/etc/sudoers.d/90-cloud-init-users"
     hostname_conf_fn = "/etc/hostname"
     shadow_fn = "/etc/shadow"
+    # /etc/shadow match patterns indicating empty passwords
+    shadow_empty_locked_passwd_patterns = ["^{username}::", "^{username}:!:"]
     tz_zone_dir = "/usr/share/zoneinfo"
     default_owner = "root:root"
     init_cmd = ["service"]  # systemctl, service etc
@@ -799,7 +801,7 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
         return username
 
     def _check_if_password_field_matches(
-        self, username, pattern1, pattern2, pattern3=None, check_file=None
+        self, username, patterns=[], check_file=None
     ) -> bool:
         """
         Check whether ``username`` user has a hashed password matching
@@ -817,13 +819,9 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
         cmd = [
             "grep",
             "-q",
-            "-e",
-            "^%s%s" % (username, pattern1),
-            "-e",
-            "^%s%s" % (username, pattern2),
         ]
-        if pattern3 is not None:
-            cmd.extend(["-e", "^%s%s" % (username, pattern3)])
+        for pattern in patterns:
+            cmd.extend(["-e", pattern.format(username=username)])
         cmd.append(check_file)
         try:
             subp.subp(cmd)
@@ -850,7 +848,9 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
         """
 
         status = not self._check_if_password_field_matches(
-            username, "::", ":!:", check_file=shadow_file
+            username,
+            self.shadow_empty_locked_passwd_patterns,
+            check_file=shadow_file,
         )
         return status
 
diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
index ccd961159..022a5b4ea 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -41,6 +41,17 @@ class Distro(cloudinit.distros.bsd.BSD):
     dhclient_lease_directory = "/var/db"
     dhclient_lease_file_regex = r"dhclient.leases.\w+"
 
+    # /etc/shadow match patterns indicating empty passwords
+    # For FreeBSD (from https://man.freebsd.org/cgi/man.cgi?passwd(5)) a
+    # password field of "" indicates no password, and a password
+    # field value of either "*" or "*LOCKED*" indicate differing forms of
+    # "locked" but with no password defined.
+    shadow_empty_locked_passwd_patterns = [
+        "^{username}::",
+        "^{username}:*:",
+        "^{username}:*LOCKED*:",
+    ]
+
     @classmethod
     def reload_init(cls, rcs=None):
         """
@@ -148,25 +159,6 @@ class Distro(cloudinit.distros.bsd.BSD):
         # Indicate that a new user was created
         return True
 
-    def _check_if_existing_password(self, username, shadow_file=None) -> bool:
-        """
-        Check whether ``username`` user has an existing password (regardless
-        of whether locked or not).
-
-        For FreeBSD (from https://man.freebsd.org/cgi/man.cgi?passwd(5)) a
-        password field of "" indicates no password, and a password
-        field value of either "*" or "*LOCKED*" indicate differing forms of
-        "locked" but with no password defined.
-
-        Returns either 'True' to indicate a password present, or 'False'
-        for no password set.
-        """
-
-        status = not self._check_if_password_field_matches(
-            username, "::", ":*:", ":*LOCKED*:", check_file=shadow_file
-        )
-        return status
-
     def expire_passwd(self, user):
         try:
             subp.subp(["pw", "usermod", user, "-p", "01-Jan-1970"])
diff --git a/cloudinit/distros/netbsd.py b/cloudinit/distros/netbsd.py
index b9e038e89..ce65e5283 100644
--- a/cloudinit/distros/netbsd.py
+++ b/cloudinit/distros/netbsd.py
@@ -49,6 +49,17 @@ class NetBSD(cloudinit.distros.bsd.BSD):
     ci_sudoers_fn = "/usr/pkg/etc/sudoers.d/90-cloud-init-users"
     group_add_cmd_prefix = ["groupadd"]
 
+    # For NetBSD (from https://man.netbsd.org/passwd.5) a password field
+    # value of either "" or "*************" (13 "*") indicates no password,
+    # a password field prefixed with "*LOCKED*" indicates a locked
+    # password, and a password field of "*LOCKED*" followed by 13 "*"
+    # indicates a locked and blank password.
+    shadow_empty_locked_passwd_patterns = [
+        "^{username}::",
+        "^{username}:*************:",
+        "^{username}:*LOCKED**************:",
+    ]
+
     def __init__(self, name, cfg, paths):
         super().__init__(name, cfg, paths)
         if os.path.exists("/usr/pkg/bin/pkgin"):
@@ -120,30 +131,6 @@ class NetBSD(cloudinit.distros.bsd.BSD):
         # Indicate that a new user was created
         return True
 
-    def _check_if_existing_password(self, username, shadow_file=None) -> bool:
-        """
-        Check whether ``username`` user has an existing password (regardless
-        of whether locked or not).
-
-        For NetBSD (from https://man.netbsd.org/passwd.5) a password field
-        value of either "" or "*************" (13 "*") indicates no password,
-        a password field prefixed with "*LOCKED*" indicates a locked
-        password, and a password field of "*LOCKED*" followed by 13 "*"
-        indicates a locked and blank password.
-
-        Returns either 'True' to indicate a password present, or 'False'
-        for no password set.
-        """
-
-        status = not self._check_if_password_field_matches(
-            username,
-            "::",
-            ":*************:",
-            ":*LOCKED**************:",
-            check_file=shadow_file,
-        )
-        return status
-
     def set_passwd(self, user, passwd, hashed=False):
         if hashed:
             hashed_pw = passwd
diff --git a/cloudinit/distros/openbsd.py b/cloudinit/distros/openbsd.py
index 418e99bc4..cc72f9c8b 100644
--- a/cloudinit/distros/openbsd.py
+++ b/cloudinit/distros/openbsd.py
@@ -14,6 +14,16 @@ class Distro(cloudinit.distros.netbsd.NetBSD):
     hostname_conf_fn = "/etc/myname"
     init_cmd = ["rcctl"]
 
+    # For OpenBSD (from https://man.openbsd.org/passwd.5) a password field
+    # of "" indicates no password, and password field values of either
+    # "*" or "*************" (13 "*") indicate differing forms of "locked"
+    # but with no password defined.
+    shadow_empty_locked_passwd_patterns = [
+        "^{username}::",
+        "^{username}:*:",
+        "^{username}:*************:",
+    ]
+
     def _read_hostname(self, filename, default=None):
         return util.load_text_file(self.hostname_conf_fn)
 
@@ -45,25 +55,6 @@ class Distro(cloudinit.distros.netbsd.NetBSD):
         cmd = list(init_cmd) + list(cmds[action])
         return subp.subp(cmd, capture=True, rcs=rcs)
 
-    def _check_if_existing_password(self, username, shadow_file=None) -> bool:
-        """
-        Check whether ``username`` user has an existing password (regardless
-        of whether locked or not).
-
-        For OpenBSD (from https://man.openbsd.org/passwd.5) a password field
-        of "" indicates no password, and password field values of either
-        "*" or "*************" (13 "*") indicate differing forms of "locked"
-        but with no password defined.
-
-        Returns either 'True' to indicate a password present, or 'False'
-        for no password set.
-        """
-
-        status = not self._check_if_password_field_matches(
-            username, "::", ":*:", ":*************:", check_file=shadow_file
-        )
-        return status
-
     def lock_passwd(self, name):
         try:
             subp.subp(["usermod", "-p", "*", name])

[blackboxsw]

@dermotbradley thanks for the changes here. I pushed a separate commit to capture a number of inline comments I had to seed review and potentially refactor a bit of this proposal.

I'll capture the general details of the proposed fixes here:

1. Let's avoid subprocess(grep -e) commands and instead use python to process shadow files because subprocess calls to grep for small files take longer than re processing in python, and we should also be checking if our known username is in a given shadow file before processing it, which would require a separate shell out to grep adding to that cost.
2. Rename _check_if_existing_password -> _shadow_file_has_empty_user_password
  to clearly declare intent of the method

3. drop wrapper method _check_if_existing_password as it is misleading
   since we just not the return_value from _shadow_file_has_empty_user_password
4. define class attributes shadow_extrausers_fn and shadow_empty_locked_passwd_patterns
   allowing BSD subclasses to specialize those match patterns for finding empty
   passwords
5. Move all BSD specific password unlock tests up into test_create_users.py since the only differentiator in distro-specific behavior is defined in shadow_empty_locked_passwd_patterns

Unrelated to this PR:
- Avoid significant using subprocess calls in
init methods of classes as costly side-effects during class init force testing to mock out this logic. Move subp(ifconfig -a) call out of
cloudinit/distros/networking/NetworkingBSD.init and into a cached
property. Only perform significant side-effects when the data is needed.

[blackboxsw]

@dermotbradley unittest coverage looks good here and the functional changes to support password unlock of empty passwords cover clear use-cases. I think what we may need additionally is an integration test extension to cover pre-existing user with empty or locked password in either tests/integration_tests/modules/test_set_password.py or tests/integration_tests/modules/test_users_groups.py to assert warning condition and not-unlocked user when password is empty.

[blackboxsw]

@dermotbradley this patchset and behavior policy for allowing empty passwords looks good to me. I think we have fairly good unittest coverage and integration test coverage now of the unlock conditions to ensure we raise warnings about unexpected/undesired behavior and disallowing unlocks on empty passwords.

I'm +1 on this branch and will await your feedback/confirmation that this looks good on your end.

You mentioned side-channel about potential unittest failures you were seeing during alpine package build for unmocked ifconfig -a due to NetworkingBSD behavior. I have been unable to reproduce these unittest assert failures during ubuntu package builds or in Alpline LXC containers using this branch directly. I also see our Alpine CI workflow is also happy with this changeset.

If this remains a problem let's see if we can understand the details of those specific test failures and what may be going on

[dermotbradley]

Yupe, I think we're good to go...