feat(alpine): add support for Busybox adduser/addgroup

[dermotbradley]

feat(alpine): add support for Busybox adduser/addgroup

By default Alpine Linux provides Busybox utilities such as adduser
and addgroup for managing users and groups. Optionally the Alpine
"shadow" package provides the traditional useradd/groupadd utilities.

Add fallback support for the Busybox user/group management utilities
for Alpine Linux.

Additional Context

Test Steps

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

[TheRealFalcon]

@dermotbradley , thanks for this! It'll be good to have extra busybox support.

First pass, I mentioned this inline, but I'm wondering if we can re-use the adduser implementation that already exists rather than re-adding most of the existing code in the Alpine context. If there's a reason we can't call super() directly, is there a way we could modify the existing code so that it could be used in both places?

[dermotbradley]

@dermotbradley , thanks for this! It'll be good to have extra busybox support.

First pass, I mentioned this inline, but I'm wondering if we can re-use the adduser implementation that already exists rather than re-adding most of the existing code in the Alpine context. If there's a reason we can't call super() directly, is there a way we could modify the existing code so that it could be used in both places?

Done.

[dermotbradley]

Am unsure how to resolve the pylint failure:

cloudinit/distros/alpine.py:330: [E1133(not-an-iterable), Distro.add_user] Non-iterable value addn_groups is used in an iterating context

I added a type annotation to addn_groups but that had no effect.

[dermotbradley]

Am unsure how to resolve the pylint failure:

cloudinit/distros/alpine.py:330: [E1133(not-an-iterable), Distro.add_user] Non-iterable value addn_groups is used in an iterating context

I added a type annotation to addn_groups but that had no effect.

I ended up adding a pylint ignore - it seems there are known issues with this warning

[TheRealFalcon]

I left some more inline comments. They're hopefully simpler asks this time around.

One more general is that this Alpine add_user is becoming quite long. I won't block the PR on it since the parent code it's modeled after is also somewhat long and complicated, but I do think there are some sections like group handling or shadow handling that could easily be moved into helper functions.

That would also make the code easier to test. For how much was added here, it would be nice if we could get some additional test coverage. There are quite a few lines/functions with no coverage at all. Since us upstream devs don't have the Alpine expertise, I think it'd really benefit you long term so that we don't accidentally break things if we ever need to modify things here.

[dermotbradley]

One more general is that this Alpine add_user is becoming quite long. I won't block the PR on it since the parent code it's modeled after is also somewhat long and complicated, but I do think there are some sections like group handling or shadow handling that could easily be moved into helper functions.

Ok, I'll look into this as a future improvement.

For how much was added here, it would be nice if we could get some additional test coverage. There are quite a few lines/functions with no coverage at all. Since us upstream devs don't have the Alpine expertise, I think it'd really benefit you long term so that we don't accidentally break things if we ever need to modify things here.

Agreed, I do intend to extend the testing for this and yes this would become easier once things are split up more.

[TheRealFalcon]

LGTM! Thanks for the changes here @dermotbradley

I think @blackboxsw was also reviewing, so I'll wait to see if there's anything else from him.

[blackboxsw]

Thanks so much for your patience here @dermotbradley.

• let's avoid calling set_passwd when password value is an empty string (drop the is not None)

I've added a number of minor inline comments for minor performance issues:

• early exit from for loops
• avoid double split operations

It may be better to also look at the symlink of /usr/bin/passwd rather than string parsing the --help output when deciding to use busybox logic for password handling.

The full diff of suggestions to take as at your discretion:

diff --git a/cloudinit/distros/alpine.py b/cloudinit/distros/alpine.py
index 1a0fa488f..8dc4314f5 100644
--- a/cloudinit/distros/alpine.py
+++ b/cloudinit/distros/alpine.py
@@ -225,10 +225,7 @@ class Distro(distros.Distro):
         if subp.which("useradd"):
             return super().add_user(name, **kwargs)
 
-        if "create_groups" in kwargs:
-            create_groups = kwargs.pop("create_groups")
-        else:
-            create_groups = True
+        create_groups = kwargs.pop("create_groups", True)
 
         adduser_cmd = ["adduser", "-D"]
 
@@ -250,8 +247,7 @@ class Distro(distros.Distro):
         if groups:
             if isinstance(groups, str):
                 groups = groups.split(",")
-
-            if isinstance(groups, dict):
+            elif isinstance(groups, dict):
                 util.deprecate(
                     deprecated=f"The user {name} has a 'groups' config value "
                     "of type dict",
@@ -264,13 +260,12 @@ class Distro(distros.Distro):
             # that came in as a string like: groups: group1, group2
             groups = [g.strip() for g in groups]
 
-            # kwargs.items loop below wants a comma delimeted string
+            # kwargs.items loop below wants a comma delimited string
             # that can go right through to the command.
             kwargs["groups"] = ",".join(groups)
 
-            primary_group = kwargs.get("primary_group")
-            if primary_group:
-                groups.append(primary_group)
+            if kwargs.get("primary_group"):
+                groups.append(kwargs["primary_group"])
 
         if create_groups and groups:
             for group in groups:
@@ -281,7 +276,7 @@ class Distro(distros.Distro):
             kwargs["uid"] = str(kwargs["uid"])
 
         unsupported_busybox_values: Dict[str, Any] = {
-            "groups": None,
+            "groups": [],
             "expiredate": None,
             "inactive": None,
             "passwd": None,
@@ -317,34 +312,35 @@ class Distro(distros.Distro):
         LOG.debug("Adding user %s", name)
         try:
             subp.subp(adduser_cmd)
-        except Exception as e:
+        except subp.CalledProcessError as e:
             LOG.warning("Failed to create user %s", name)
             raise e
 
         # Process remaining options that Busybox's 'adduser' does not support
 
-        addn_groups = unsupported_busybox_values["groups"]
-        if addn_groups is not None:
-            # Separately add user to each additional group as Busybox's
-            # 'adduser' does not support specifying additional groups.
-            for addn_group in addn_groups:  # pylint: disable=E1133
-                LOG.debug("Adding user to group %s", addn_group)
-                try:
-                    subp.subp(["addgroup", name, addn_group])
-                except subp.ProcessExecutionError as e:
-                    util.logexc(
-                        LOG,
-                        "Failed to add user %s to group %s",
-                        name,
-                        addn_group,
-                    )
-                    raise e
+        # Separately add user to each additional group as Busybox's
+        # 'adduser' does not support specifying additional groups.
+        for addn_group in unsupported_busybox_values[
+            "groups"
+        ]:  # pylint: disable=E1133
+            LOG.debug("Adding user to group %s", addn_group)
+            try:
+                subp.subp(["addgroup", name, addn_group])
+            except subp.ProcessExecutionError as e:
+                util.logexc(
+                    LOG,
+                    "Failed to add user %s to group %s",
+                    name,
+                    addn_group,
+                )
+                raise e
 
-        passwd = unsupported_busybox_values["passwd"]
-        if passwd is not None:
+        if unsupported_busybox_values["passwd"]:
             # Separately set password as Busybox's 'adduser' does
             # not support passing password as CLI option.
-            super().set_passwd(name, passwd, hashed=True)
+            super().set_passwd(
+                name, unsupported_busybox_values["passwd"], hashed=True
+            )
 
         # Busybox's 'adduser' is hardcoded to always set the following field
         # values (numbered from "0") in /etc/shadow unlike 'useradd':
@@ -375,21 +371,21 @@ class Distro(distros.Distro):
         # Find the line in /etc/shadow for the user
         original_line = None
         for line in shadow_contents.splitlines():
-            current_user = line.split(":")[0]
-            if current_user == name:
+            new_line_parts = line.split(":")
+            if new_line_parts[0] == name:
                 original_line = line
+                break
 
-        if original_line is not None:
+        if original_line:
             # Modify field(s) in copy of user's shadow file entry
-            new_list = original_line.split(":")
             update_type = ""
 
             # Minimum password age
-            new_list[3] = ""
+            new_line_parts[3] = ""
             # Maximum password age
-            new_list[4] = ""
+            new_line_parts[4] = ""
             # Password warning period
-            new_list[5] = ""
+            new_line_parts[5] = ""
             update_type = "password aging"
 
             if expiredate is not None:
@@ -398,20 +394,20 @@ class Distro(distros.Distro):
                     datetime.fromisoformat(expiredate)
                     - datetime.fromisoformat("1970-01-01")
                 ).days
-                new_list[7] = str(days)
+                new_line_parts[7] = str(days)
                 if update_type != "":
                     update_type = update_type + " & "
                 update_type = update_type + "acct expiration date"
             if inactive is not None:
-                new_list[6] = inactive
+                new_line_parts[6] = inactive
                 if update_type != "":
                     update_type = update_type + " & "
                 update_type = update_type + "inactivity period"
 
-            new_line = ":".join(new_list)
-
             # Replace existing line for user with modified line
-            shadow_contents = shadow_contents.replace(original_line, new_line)
+            shadow_contents = shadow_contents.replace(
+                original_line, ":".join(new_line_parts)
+            )
             LOG.debug("Updating %s for user %s", update_type, name)
             try:
                 util.write_file(
@@ -433,8 +429,9 @@ class Distro(distros.Distro):
         # Check whether Shadow's or Busybox's version of 'passwd'.
         # If Shadow's 'passwd' is available then use the generic
         # lock_passwd function from __init__.py instead.
-        (_out, err) = subp.subp(["passwd", "--help"])
-        if not err.startswith("BusyBox"):
+        if os.path.islink("/usr/bin/passwd") and "busybox" not in os.readlink(
+            "/usr/bin/passwd"
+        ):
             return super().lock_passwd(name)
 
         cmd = ["passwd", "-l", name]
@@ -448,7 +445,7 @@ class Distro(distros.Distro):
             (_out, err) = subp.subp(cmd, rcs=[0, 1])
             if re.search(r"is already locked", err):
                 return True
-        except Exception as e:
+        except subp.CalledProcessError as e:
             util.logexc(LOG, "Failed to disable password for user %s", name)
             raise e
 
@@ -456,8 +453,9 @@ class Distro(distros.Distro):
         # Check whether Shadow's or Busybox's version of 'passwd'.
         # If Shadow's 'passwd' is available then use the generic
         # expire_passwd function from __init__.py instead.
-        (_out, err) = subp.subp(["passwd", "--help"])
-        if not err.startswith("BusyBox"):
+        if os.path.islink("/usr/bin/passwd") and "busybox" not in os.readlink(
+            "/usr/bin/passwd"
+        ):
             return super().expire_passwd(user)
 
         # Busybox's 'passwd' does not provide an expire option
@@ -473,23 +471,22 @@ class Distro(distros.Distro):
         # Find the line in /etc/shadow for the user
         original_line = None
         for line in shadow_contents.splitlines():
-            current_user = line.split(":")[0]
-            if current_user == user:
+            new_line_parts = line.split(":")
+            if new_line_parts[0] == user:
                 LOG.debug("Found /etc/shadow line matching user %s", user)
                 original_line = line
+                break
 
-        if original_line is not None:
+        if original_line:
             # Replace existing line for user with modified line
-            new_list = original_line.split(":")
             # Field '2' (numbered from '0') in /etc/shadow
             # is the "date of last password change".
-            if new_list[2] != "0":
+            if new_line_parts[2] != "0":
                 # Busybox's 'adduser' always expires password so only
                 # need to expire it now if this is not a new user.
-                new_list[2] = "0"
-                new_line = ":".join(new_list)
+                new_line_parts[2] = "0"
                 shadow_contents = shadow_contents.replace(
-                    original_line, new_line, 1
+                    original_line, ":".join(new_line_parts), 1
                 )
 
                 LOG.debug("Expiring password for user %s", user)

[blackboxsw]

Pending at least avoidance of an attempting to set_passwd on an empty string passwd value on line 344 of alpine.py. it'd be nice to check os.readlink on /usr/bin/passwd instead of stderr from --help. But not necessarily blocking.

[dermotbradley]

Ready to merge now I guess...

[blackboxsw]

Thank you for this correction @dermotbradley. I was basing my suggestion off of adduser behavior as seen in incus/lxd alpine cloud images and assumed it'd be comparable for /usr/bin/passwd ~ # cat /etc/os-release NAME="Alpine Linux" ID=alpine VERSION_ID=3.19.1 PRETTY_NAME="Alpine Linux v3.19" HOME_URL="https://alpinelinux.org/" BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues" ~ # ls -l /usr/sbin/adduser lrwxrwxrwx 1 root root 12 Jan 26 17:53 /usr/sbin/adduser -> /bin/busybox

…

On Thu, May 30, 2024 at 8:10 PM dermotbradley ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cloudinit/distros/alpine.py <#5176 (comment)> : > + (_out, err) = subp.subp(["passwd", "--help"]) + if not err.startswith("BusyBox"): Done. I had to change the logic to check if /usr/bin/passwd *not* being a softlink *or* the softlink not pointing to "*bbsuid*" (Alpine uses "bbsuid" binary rather than "busybox" for commands that require suid) — Reply to this email directly, view it on GitHub <#5176 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLZJRRZJ3LNZUEZKQJH7GLZE7L2BAVCNFSM6AAAAABGGNVJQ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBZG43TSNZZG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>