review comments: update shadow file processing

- Avoid signficant functional side-effects like subprocess calls in
  __init__ methods of classes. 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.
- avoid subprocess(grep -e) commands and instead use python to process shadow
  files, removing need for _check_if_password_field_matches.
- rename _check_if_existing_password -> _shadow_file_has_empty_user_password
  to clearly declare intent of the method
- drop wrapper method _check_if_existing_password as it is misleading
  since we just not the retvalue value from _shadow_file_has_empty_user_password
- define class attributes shadow_extrausers_fn and shadow_empty_locked_passwd_patterns
  allowing BSD subclasses to specialize those match patterns for finding empty
  passwords

WIP checkpoint for review, will rebase force push this commit today

cloudinit/distros/__init__.py

@@ -137,6 +137,9 @@ 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"
+    shadow_extrausers_fn = "/var/lib/extrausers/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
@@ -804,62 +807,40 @@ def add_snap_user(self, name, **kwargs):
 
         return username
 
-    def _check_if_password_field_matches(
-        self, username, pattern1, pattern2, pattern3=None, check_file=None
-    ) -> bool:
+    def _shadow_file_has_empty_user_password(self, username) -> bool:
         """
-        Check whether ``username`` user has a hashed password matching
-        either pattern.
+        Check whether username exists in shadow files with empty password.
 
-        FreeBSD, NetBSD, and OpenBSD use 3 patterns, others only use
-        2 patterns.
-
-        Returns either 'True' to indicate a match, otherwise 'False'.
+        Support reading /var/lib/extrausers/shadow on snappy systems.
         """
-
-        if not check_file:
-            check_file = self.shadow_fn
-
-        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)])
-        cmd.append(check_file)
-        try:
-            subp.subp(cmd)
-        except subp.ProcessExecutionError as e:
-            if e.exit_code == 1:
-                # Exit code 1 means 'grep' didn't find empty password
+        if util.system_is_snappy():
+            shadow_files = [self.shadow_extrausers_fn, self.shadow_fn]
+        else:
+            shadow_files = [self.shadow_fn]
+        shadow_empty_passwd_re = "|".join(
+            [
+                pattern.format(username=username)
+                for pattern in self.shadow_empty_locked_passwd_patterns
+            ]
+        )
+        for shadow_file in shadow_files:
+            if not os.path.exists(shadow_file):
+                continue
+            shadow_content = util.load_text_file(shadow_file)
+            if not re.findall(rf"^{username}:", shadow_content, re.MULTILINE):
+                LOG.debug("User %s not found in %s", username, shadow_file)
+                continue
+            LOG.debug(
+                "User %s found in %s. Checking for empty password",
+                username,
+                shadow_file,
+            )
+            if re.findall(
+                shadow_empty_passwd_re, shadow_content, re.MULTILINE
+            ):
                 return True
-            else:
-                util.logexc(
-                    LOG,
-                    "Failed to check the status of password for user %s",
-                    username,
-                )
-                raise e
         return False
 
-    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).
-
-        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 create_user(self, name, **kwargs):
         """
         Creates or partially updates the ``name`` user in the system.
@@ -925,20 +906,10 @@ def create_user(self, name, **kwargs):
 
                 # As no password specified for the existing user in user-data
                 # then check if the existing user's hashed password value is
-                # blank (whether locked or not).
-                if util.system_is_snappy():
-                    has_existing_password = self._check_if_existing_password(
-                        name, "/var/lib/extrausers/shadow"
-                    )
-                    if not has_existing_password:
-                        # Check /etc/shadow also
-                        has_existing_password = (
-                            self._check_if_existing_password(name)
-                        )
-                else:
-                    has_existing_password = self._check_if_existing_password(
-                        name
-                    )
+                # empty (whether locked or not).
+                has_existing_password = (
+                    not self._shadow_file_has_empty_user_password(name)
+                )
         else:
             if "passwd" in kwargs:
                 ud_password_specified = True

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 @@ def add_user(self, name, **kwargs) -> bool:
         # 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"])

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 @@ def add_user(self, name, **kwargs) -> bool:
         # 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

cloudinit/distros/networking.py

@@ -179,16 +179,21 @@ class BSDNetworking(Networking):
 
     def __init__(self):
         self.ifc = ifconfig.Ifconfig()
-        self.ifs = {}
-        self._update_ifs()
+        self._ifs = {}
         super().__init__()
 
+    @property
+    def ifs(self) -> dict:
+        if not self._ifs:
+            self._update_ifs()
+        return self._ifs
+
     def _update_ifs(self):
         ifconf = subp.subp(["ifconfig", "-a"])
         # ``ifconfig -a`` always returns at least ``lo0``.
         # So this ``if`` is really just to make testing/mocking easier
         if ifconf[0]:
-            self.ifs = self.ifc.parse(ifconf[0])
+            self._ifs = self.ifc.parse(ifconf[0])
 
     def apply_network_config_names(self, netcfg: NetworkConfig) -> None:
         LOG.debug("Cannot rename network interface.")

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 @@ def manage_service(cls, action: str, service: str, *extra_args, rcs=None):
         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])