[PATCH 0/5] allow creation of users/groups before rootfs creation

[PATCH 0/5] allow creation of users/groups before rootfs creation

[T. Schaffner]

From: Tobias Schaffner <tobias.schaffner@siemens.com>

This patch series will allow to specify a `pre` flag for the USER_ and
GROUP_ bitbake variables. If this flag is set to `true` the given user
or group will be created in the rootfs configuration step instead of on
rootfs postprocessing. This is helpful when a specific id should be used
which would otherwise be picked by a user or group created by one of the
installed packages.

A rewrite of the image-account-extension in python was done on the way.
This allows us to drop a lot of encoding and parsing code that was used
to transition to shell and therefore made it easier to read and maintain.

Using python functions for more complex tasks allows us the usage of
unittests. A very basic infrastructure for unittesting using the build
in python unittest and the bb.parse module was added. This was used to
test the re-implementation of the image-account-extension as a first
showcase.

Tobias Schaffner (5):
  simplify image-account-extension
  allow creation of users/groups before rootfs creation
  create a minimal python unittest infrastructure
  add unittests for the image-account-extension
  set minimal python version in user_manual to 3.5

 doc/user_manual.md                            |   4 +-
 meta/classes/image-account-extension.bbclass  | 391 +++++++-----------
 testsuite/unittests/README.md                 |  28 ++
 testsuite/unittests/bitbake.py                |  37 ++
 testsuite/unittests/rootfs.py                 |  45 ++
 .../unittests/test_image_account_extension.py | 175 ++++++++
 6 files changed, 434 insertions(+), 246 deletions(-)
 create mode 100644 testsuite/unittests/README.md
 create mode 100644 testsuite/unittests/bitbake.py
 create mode 100644 testsuite/unittests/rootfs.py
 create mode 100644 testsuite/unittests/test_image_account_extension.py

-- 
2.34.1

[PATCH 1/5] simplify image-account-extension

[T. Schaffner]

From: Tobias Schaffner <tobias.schaffner@siemens.com>

Do the complete user and group creation in python. This allows us to
drop the encoding and parsing code that was used to make the user and
group lists available in the shell function.

Signed-off-by: Tobias Schaffner <tobias.schaffner@siemens.com>
---
 meta/classes/image-account-extension.bbclass | 368 +++++++------------
 1 file changed, 124 insertions(+), 244 deletions(-)

diff --git a/meta/classes/image-account-extension.bbclass b/meta/classes/image-account-extension.bbclass
index cbc20a2..127732a 100644
--- a/meta/classes/image-account-extension.bbclass
+++ b/meta/classes/image-account-extension.bbclass
@@ -1,5 +1,5 @@
 # This software is a part of ISAR.
-# Copyright (C) Siemens AG, 2019
+# Copyright (C) Siemens AG, 2023
 #
 # SPDX-License-Identifier: MIT
 #
@@ -25,251 +25,131 @@ GROUPS ??= ""
 #GROUP_root[gid] = ""
 #GROUP_root[flags] = "system"
 
-def gen_accounts_array(d, listname, entryname, flags, verb_flags=None):
-    from itertools import chain
-
-    entries = (d.getVar(listname, True) or "").split()
-    return " ".join(
-        ":".join(
-            chain(
-                (entry,),
-                (
-                    (",".join(
-                        (
-                            d.getVarFlag(entryname + "_" + entry, flag, True) or ""
-                        ).split()
-                    ) if flag not in (verb_flags or []) else (
-                        d.getVarFlag(entryname + "_" + entry, flag, True) or ""
-                    )).replace(":","=")
-                    for flag in flags
-                ),
-            )
-        )
-        for entry in entries
-    )
-
-# List of space separated entries, where each entry has the format:
-# username:encryptedpassword:expiredate:inactivenumber:userid:groupid:comment:homedir:shell:group1,group2:flag1,flag2
-IMAGE_ACCOUNTS_USERS =+ "${@gen_accounts_array(d, 'USERS', 'USER', ['password',  'expire', 'inactive', 'uid', 'gid', 'comment', 'home', 'shell', 'groups', 'flags'], ['password', 'comment', 'home', 'shell'])}"
-
-# List of space separated entries, where each entry has the format:
-# groupname:groupid:flag1,flag2
-IMAGE_ACCOUNTS_GROUPS =+ "${@gen_accounts_array(d, 'GROUPS', 'GROUP', ['gid', 'flags'])}"
-
-do_rootfs_install[vardeps] += "${IMAGE_ACCOUNTS_GROUPS} ${IMAGE_ACCOUNTS_USERS}"
 
-ROOTFS_POSTPROCESS_COMMAND += "image_postprocess_accounts"
-image_postprocess_accounts() {
-    # Create groups
-    # Add space to the end of the list:
-    list='${@" ".join(d.getVar('IMAGE_ACCOUNTS_GROUPS', True).split())} '
-    while true; do
-        # Pop first group entry:
-        list_rest="${list#*:*:* }"
-        entry="${list%%${list_rest}}"
-        list="${list_rest}"
-
-        if [ -z "${entry}" ]; then
-            break
-        fi
-
-        # Add colon to the end of the entry and remove trailing space:
-        entry="${entry% }:"
-
-        # Decode entries:
-        name="${entry%%:*}"
-        entry="${entry#${name}:}"
-
-        gid="${entry%%:*}"
-        entry="${entry#${gid}:}"
-
-        flags="${entry%%:*}"
-        entry="${entry#${flags}:}"
-
-        flags=",${flags}," # Needed for searching for substrings
-
-        # Check if user already exists:
-        if grep -q "^${name}:" '${ROOTFSDIR}/etc/group'; then
-            exists="y"
-        else
-            exists="n"
-        fi
-
-        # Create arguments:
-        set -- # clear arguments
-
-        if [ -n "$gid" ]; then
-            set -- "$@" --gid "$gid"
-        fi
-
-        if [ "n" = "$exists" ]; then
-            if [ "${flags}" != "${flags%*,system,*}" ]; then
-                set -- "$@" --system
-            fi
-        fi
-
-        # Create or modify groups:
-        if [ "y" = "$exists" ]; then
-            if [ -z "$@" ]; then
-                echo "Do not execute groupmod (no changes)."
-            else
-                echo "Execute groupmod with \"$@\" for \"$name\""
-                sudo -E chroot '${ROOTFSDIR}' \
-                    /usr/sbin/groupmod "$@" "$name"
-            fi
-        else
-            echo "Execute groupadd with \"$@\" for \"$name\""
-            sudo -E chroot '${ROOTFSDIR}' \
-                /usr/sbin/groupadd "$@" "$name"
-        fi
-    done
-
-    # Create users
-    list='${@" ".join(d.getVar('IMAGE_ACCOUNTS_USERS', True).split())} '
-    while true; do
-        # Pop first user entry:
-        list_rest="${list#*:*:*:*:*:*:*:*:*:*:* }"
-        entry="${list%%${list_rest}}"
-        list="${list_rest}"
-
-        if [ -z "${entry}" ]; then
-            break
-        fi
-
-        # Add colon to the end of the entry and remove trailing space:
-        entry="${entry% }:"
-
-        # Decode entries:
-        name="${entry%%:*}"
-        entry="${entry#${name}:}"
-
-        password="${entry%%:*}"
-        entry="${entry#${password}:}"
-
-        expire="${entry%%:*}"
-        entry="${entry#${expire}:}"
-
-        inactive="${entry%%:*}"
-        entry="${entry#${inactive}:}"
-
-        uid="${entry%%:*}"
-        entry="${entry#${uid}:}"
-
-        gid="${entry%%:*}"
-        entry="${entry#${gid}:}"
-
-        comment="${entry%%:*}"
-        entry="${entry#${comment}:}"
-
-        home="${entry%%:*}"
-        entry="${entry#${home}:}"
-
-        shell="${entry%%:*}"
-        entry="${entry#${shell}:}"
-
-        groups="${entry%%:*}"
-        entry="${entry#${groups}:}"
-
-        flags="${entry%%:*}"
-        entry="${entry#${flags}:}"
-
-        flags=",${flags}," # Needed for searching for substrings
-
-        # Check if user already exists:
-        if grep -q "^${name}:" '${ROOTFSDIR}/etc/passwd'; then
-            exists="y"
-        else
-            exists="n"
-        fi
-
-        # Create arguments:
-        set -- # clear arguments
-
-        if [ -n "$expire" ]; then
-            set -- "$@" --expiredate "$expire"
-        fi
-
-        if [ -n "$inactive" ]; then
-            set -- "$@" --inactive "$inactive"
-        fi
-
-        if [ -n "$uid" ]; then
-            set -- "$@" --uid "$uid"
-        fi
-
-        if [ -n "$gid" ]; then
-            set -- "$@" --gid "$gid"
-        fi
-
-        if [ -n "$comment" ]; then
-            set -- "$@" --comment "$comment"
-        fi
-
-        if [ -n "$home" ]; then
-            if [ "y" = "$exists" ]; then
-                set -- "$@" --home "$home" --move-home
-            else
-                set -- "$@" --home-dir "$home"
-            fi
-        fi
-
-        if [ -n "$shell" ]; then
-            set -- "$@" --shell "$shell"
-        fi
-
-        if [ -n "$groups" ]; then
-            set -- "$@" --groups "$groups"
-        fi
-
-        if [ "n" = "$exists" ]; then
-            if [ "${flags}" != "${flags%*,system,*}" ]; then
-                set -- "$@" --system
-            fi
-            if [ "${flags}" != "${flags%*,no-create-home,*}" ]; then
-                set -- "$@" --no-create-home
-            else
-                if [ "${flags}" != "${flags%*,create-home,*}" ]; then
-                    set -- "$@" --create-home
-                fi
-            fi
-        fi
-
-        # Create or modify users:
-        if [ "y" = "$exists" ]; then
-            if [ -z "$@" ]; then
-                echo "Do not execute usermod (no changes)."
-            else
-                echo "Execute usermod with \"$@\" for \"$name\""
-                sudo -E chroot '${ROOTFSDIR}' \
-                    /usr/sbin/usermod "$@" "$name"
-            fi
-        else
-            echo "Execute useradd with \"$@\" for \"$name\""
-            sudo -E chroot '${ROOTFSDIR}' \
-                /usr/sbin/useradd "$@" "$name"
-        fi
-
-        # Set password:
-        if [ -n "$password" -o "${flags}" != "${flags%*,allow-empty-password,*}" ]; then
-            chpasswd_args="-e"
-            if [ "${flags}" != "${flags%*,clear-text-password,*}" ]; then
+def image_create_groups(d: "DataSmart") -> None:
+    """Creates the groups defined in the ``GROUPS`` bitbake variable.
+
+    Args:
+        d (DataSmart): The bitbake datastore.
+
+    Returns:
+        None
+    """
+    entries = (d.getVar("GROUPS", True) or "").split()
+    rootfsdir = d.getVar("ROOTFSDIR", True)
+    chroot = ["sudo", "-E", "chroot", rootfsdir]
+
+    for entry in entries:
+        args = []
+        group_entry = "GROUP_{}".format(entry)
+
+        with open("{}/etc/group".format(rootfsdir), "r") as group_file:
+            exists = any(line.startswith("{}:".format(entry)) for line in group_file)
+
+        gid = d.getVarFlag(group_entry, "gid", True) or ""
+        if gid:
+            args.append("--gid")
+            args.append(gid)
+
+        flags = (d.getVarFlag(group_entry, "flags", True) or "").split()
+        if "system" in flags:
+            args.append("--system")
+
+        if exists:
+            if args:
+                bb.process.run([*chroot, "/usr/sbin/groupmod", *args, entry])
+        else:
+            bb.process.run([*chroot, "/usr/sbin/groupadd", *args, entry])
+
+
+def image_create_users(d: "DataSmart") -> None:
+    """Creates the users defined in the ``USERS`` bitbake variable.
+
+    Args:
+        d (DataSmart): The bitbake datastore.
+
+    Returns:
+        None
+    """
+    import hashlib
+    import crypt
+
+    entries = (d.getVar("USERS", True) or "").split()
+    rootfsdir = d.getVar("ROOTFSDIR", True)
+    chroot = ["sudo", "-E", "chroot", rootfsdir]
+
+    for entry in entries:
+        args = []
+        user_entry = "USER_{}".format(entry)
+
+        with open("{}/etc/passwd".format(rootfsdir), "r") as passwd_file:
+            exists = any(line.startswith("{}:".format(entry)) for line in passwd_file)
+
+        def add_user_option(option_name, flag_name):
+            flag_value = d.getVarFlag(user_entry, flag_name, True) or ""
+            if flag_value:
+                args.append(option_name)
+                args.append(flag_value)
+
+        add_user_option("--expire", "expiredate")
+        add_user_option("--inactive", "inactive")
+        add_user_option("--uid", "uid")
+        add_user_option("--gid", "gid")
+        add_user_option("--comment", "comment")
+        add_user_option("--shell", "shell")
+
+        groups = d.getVarFlag(user_entry, "groups", True) or ""
+        if groups:
+            args.append("--groups")
+            args.append(groups.replace(' ', ','))
+
+        flags = (d.getVarFlag(user_entry, "flags", True) or "").split()
+
+        if exists:
+            add_user_option("--home", "home")
+            if d.getVarFlag(user_entry, "home", True) or "":
+                args.append("--move-home")
+        else:
+            add_user_option("--home-dir", "home")
+
+            if "system" in flags:
+                args.append("--system")
+            if "no-create-home" in flags:
+                args.append("--no-create-home")
+            if "create-home" in flags:
+                args.append("--create-home")
+
+        if exists:
+            if args:
+                bb.process.run([*chroot, "/usr/sbin/usermod", *args, entry])
+        else:
+            bb.process.run([*chroot, "/usr/sbin/useradd", *args, entry])
+
+        command = [*chroot, "/usr/sbin/chpasswd"]
+        password = d.getVarFlag(user_entry, "password", True) or ""
+        if password or "allow-empty-password" in flags:
+            if "clear-text-password" in flags:
+
                 # chpasswd adds a random salt when running against a clear-text password.
                 # For reproducible images, we manually generate the password and use the
                 # SOURCE_DATE_EPOCH to generate the salt in a deterministic way.
-                if [ -z "${SOURCE_DATE_EPOCH}" ]; then
-                    chpasswd_args=""
-                else
-                    salt="$(echo "${SOURCE_DATE_EPOCH}" | sha256sum -z | cut -c 1-15)"
-                    password="$(openssl passwd -6 -salt $salt "$password")"
-                fi
-            fi
-            printf '%s:%s' "$name" "$password" | sudo chroot '${ROOTFSDIR}' \
-                /usr/sbin/chpasswd $chpasswd_args
-        fi
-        if [ "${flags}" != "${flags%*,force-passwd-change,*}" ]; then
-            echo "Execute passwd to force password change on first boot for \"$name\""
-            sudo -E chroot '${ROOTFSDIR}' \
-                /usr/bin/passwd --expire "$name"
-        fi
-    done
+                source_date_epoch = d.getVar("SOURCE_DATE_EPOCH", True) or ""
+                if source_date_epoch:
+                    command.append("-e")
+                    salt = hashlib.sha256("{}\n".format(source_date_epoch).encode()).hexdigest()[0:15]
+                    password = crypt.crypt(password, "$6${}".format(salt))
+
+            else:
+                command.append("-e")
+
+            bb.process.run(command, "{}:{}".format(entry, password).encode())
+
+        if "force-passwd-change" in flags:
+            bb.process.run([*chroot, "/usr/sbin/passwd", "--expire", entry])
+
+
+ROOTFS_POSTPROCESS_COMMAND += "image_postprocess_accounts"
+python image_postprocess_accounts() {
+    image_create_groups(d)
+    image_create_users(d)
 }
-- 
2.34.1

[PATCH 2/5] allow creation of users/groups before rootfs creation

[T. Schaffner]

From: Tobias Schaffner <tobias.schaffner@siemens.com>

Allow the user to specify that a user or group should be created before
rootfs creation instead of in the postprocessing step.

If a user or group is tagged with `USER_x[pre] = "true"` it will be
created in the rootfs configuration step instead.

Signed-off-by: Tobias Schaffner <tobias.schaffner@siemens.com>
---
 doc/user_manual.md                           |  2 ++
 meta/classes/image-account-extension.bbclass | 25 ++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/doc/user_manual.md b/doc/user_manual.md
index ec639e7..1d209b3 100644
--- a/doc/user_manual.md
+++ b/doc/user_manual.md
@@ -661,6 +661,7 @@ The `GROUP_<groupname>` variable contains the settings of a group named `groupna
  - `gid` - The numeric group id.
  - `flags` - A list of additional flags of the group. Those are the currently recognized flags:
    - `system` - The group is created using the `--system` parameter.
+ - `pre` - Creates the group in the rootfs configuration instead of the postprocessing step if set to `true`.
 
 The `USERS` and `USER_<username>` variable works similar to the `GROUPS` and `GROUP_<groupname>` variable. The difference are the accepted flags of the `USER_<username>` variable. It accepts the following flags:
 
@@ -680,6 +681,7 @@ The `USERS` and `USER_<username>` variable works similar to the `GROUPS` and `GR
    - `allow-empty-password` - Even if the `password` flag is empty, it will still be set. This results in a login without password.
    - `clear-text-password` - The `password` flag of the given user contains a clear-text password and not an encrypted version of it.
    - `force-passwd-change` - Force the user to change to password on first login.
+ - `pre` - Creates the user in the rootfs configuration instead of the postprocessing step if set to `true`.
 
 #### Home directory contents prefilling
 
diff --git a/meta/classes/image-account-extension.bbclass b/meta/classes/image-account-extension.bbclass
index 127732a..c0f2269 100644
--- a/meta/classes/image-account-extension.bbclass
+++ b/meta/classes/image-account-extension.bbclass
@@ -18,19 +18,23 @@ USERS ??= ""
 #USER_root[shell] = "/bin/sh"
 #USER_root[groups] = "audio video"
 #USER_root[flags] = "no-create-home create-home system allow-empty-password clear-text-password force-passwd-change"
+#USER_root[pre] = ""
 
 GROUPS ??= ""
 
 #GROUPS += "root"
 #GROUP_root[gid] = ""
 #GROUP_root[flags] = "system"
+#GROUP_root[pre] = ""
 
 
-def image_create_groups(d: "DataSmart") -> None:
+def image_create_groups(d: "DataSmart", pre: bool = False) -> None:
     """Creates the groups defined in the ``GROUPS`` bitbake variable.
 
     Args:
         d (DataSmart): The bitbake datastore.
+        pre (bool): Creates only the entries tagged with GROUP_x[pre] = "true" if True or all others
+            if set to False (default).
 
     Returns:
         None
@@ -43,6 +47,10 @@ def image_create_groups(d: "DataSmart") -> None:
         args = []
         group_entry = "GROUP_{}".format(entry)
 
+        pre_flag = (d.getVarFlag(group_entry, "pre", True) or "") == "true"
+        if pre_flag != pre:
+            continue
+
         with open("{}/etc/group".format(rootfsdir), "r") as group_file:
             exists = any(line.startswith("{}:".format(entry)) for line in group_file)
 
@@ -62,11 +70,13 @@ def image_create_groups(d: "DataSmart") -> None:
             bb.process.run([*chroot, "/usr/sbin/groupadd", *args, entry])
 
 
-def image_create_users(d: "DataSmart") -> None:
+def image_create_users(d: "DataSmart", pre: bool = False) -> None:
     """Creates the users defined in the ``USERS`` bitbake variable.
 
     Args:
         d (DataSmart): The bitbake datastore.
+        pre (bool): Creates only the entries tagged with USER_x[pre] = "true" if True or all others
+            if set to False (default).
 
     Returns:
         None
@@ -82,6 +92,10 @@ def image_create_users(d: "DataSmart") -> None:
         args = []
         user_entry = "USER_{}".format(entry)
 
+        pre_flag = (d.getVarFlag(user_entry, "pre", True) or "") == "true"
+        if pre_flag != pre:
+            continue
+
         with open("{}/etc/passwd".format(rootfsdir), "r") as passwd_file:
             exists = any(line.startswith("{}:".format(entry)) for line in passwd_file)
 
@@ -148,6 +162,13 @@ def image_create_users(d: "DataSmart") -> None:
             bb.process.run([*chroot, "/usr/sbin/passwd", "--expire", entry])
 
 
+ROOTFS_CONFIGURE_COMMAND += "image_configure_accounts"
+image_configure_accounts[weight] = "3"
+python image_configure_accounts() {
+    image_create_groups(d, True)
+    image_create_users(d, True)
+}
+
 ROOTFS_POSTPROCESS_COMMAND += "image_postprocess_accounts"
 python image_postprocess_accounts() {
     image_create_groups(d)
-- 
2.34.1

[PATCH 3/5] create a minimal python unittest infrastructure

[T. Schaffner]

From: Tobias Schaffner <tobias.schaffner@siemens.com>

Add some some infrastructure for python unittesting. The unittest_isar
module exposes a function that uses the bb.parse module to import
python functions that are defined in bitbake files.

Signed-off-by: Tobias Schaffner <tobias.schaffner@siemens.com>
---
 testsuite/unittests/README.md  | 28 +++++++++++++++++++++
 testsuite/unittests/bitbake.py | 37 ++++++++++++++++++++++++++++
 testsuite/unittests/rootfs.py  | 45 ++++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+)
 create mode 100644 testsuite/unittests/README.md
 create mode 100644 testsuite/unittests/bitbake.py
 create mode 100644 testsuite/unittests/rootfs.py

diff --git a/testsuite/unittests/README.md b/testsuite/unittests/README.md
new file mode 100644
index 0000000..75a3bb0
--- /dev/null
+++ b/testsuite/unittests/README.md
@@ -0,0 +1,28 @@
+# Isar Unittests
+
+The unittest python module adds some simple infrastructure that allows to
+unittest python functions defined in bitbake files.
+
+## Running the tests
+
+You can run the tests using avocado with `avocado --show=app,test run testsuite/unittests/`
+or by using the buildin module with `python3 -m unittest discover testsuite/unittests/`
+
+## Creating new tests
+
+See the [unittest documentation](https://docs.python.org/3/library/unittest.html)
+on how to create a test module and name it test_*bitbake_module_name*.py
+
+Use the function `load_function(file_name: str, function_name: str) -> Callable`
+in the bitbake module to load the function.
+
+Example:
+```python
+from bitbake import load_function
+
+my_function = load_function("meta/classes/my_module.bbclass", "my_function")
+my_function(arg1, arg2)
+```
+
+Use the [unittest.mock](https://docs.python.org/3/library/unittest.mock.html)
+library to mock the bb modules as needed.
diff --git a/testsuite/unittests/bitbake.py b/testsuite/unittests/bitbake.py
new file mode 100644
index 0000000..1e2f685
--- /dev/null
+++ b/testsuite/unittests/bitbake.py
@@ -0,0 +1,37 @@
+# This software is a part of ISAR.
+# Copyright (C) Siemens AG, 2023
+#
+# SPDX-License-Identifier: MIT
+
+import sys
+import pathlib
+from typing import Callable
+
+location = pathlib.Path(__file__).parent.resolve()
+sys.path.insert(0, "{}/../../bitbake/lib".format(location))
+
+from bb.parse import handle
+from bb.data import init
+
+# Modules added for reimport from testfiles
+from bb.data_smart import DataSmart
+
+
+def load_function(file_name: str, function_name: str) -> Callable:
+    """Load a python function defined in a bitbake file.
+
+    Args:
+        file_name (str): The path to the file e.g. `meta/classes/my_special.bbclass`.
+        function_name (str): The name of the python function without braces e.g. `my_special_function`
+
+    Returns:
+        Callable: The loaded function.
+    """
+    d = init()
+    parse = handle("{}/../../{}".format(location, file_name), d)
+    if function_name not in parse:
+        raise KeyError("Function {} does not exist in {}".format(
+            function_name, file_name))
+    namespace = {}
+    exec(parse[function_name], namespace)
+    return namespace[function_name]
diff --git a/testsuite/unittests/rootfs.py b/testsuite/unittests/rootfs.py
new file mode 100644
index 0000000..6c51149
--- /dev/null
+++ b/testsuite/unittests/rootfs.py
@@ -0,0 +1,45 @@
+# This software is a part of ISAR.
+# Copyright (C) Siemens AG, 2023
+#
+# SPDX-License-Identifier: MIT
+
+import tempfile
+import pathlib
+import shutil
+import atexit
+
+temp_dirs = []
+
+
+class TemporaryRootfs:
+    """ A temporary rootfs folder that will be removed after the testrun. """
+
+    def __init__(self):
+        self._rootfs_path = tempfile.mkdtemp()
+        temp_dirs.append(self._rootfs_path)
+
+    def path(self) -> str:
+        return self._rootfs_path
+
+    def create_file(self, path: str, content: str) -> None:
+        """ Create a file with the given content.
+
+        Args:
+            path (str): The path to the file e.g. `/etc/hostname`.
+            content (str): The content of the file e.g. `my_special_host`
+
+        Returns:
+            None
+        """
+        pathlib.Path(self._rootfs_path +
+                     path).parent.mkdir(parents=True, exist_ok=True)
+        with open(self._rootfs_path + path, 'w') as file:
+            file.write(content)
+
+
+def cleanup():
+    for temp_dir in temp_dirs:
+        shutil.rmtree(temp_dir)
+
+
+atexit.register(cleanup)
-- 
2.34.1

[PATCH 4/5] add unittests for the image-account-extension

[T. Schaffner]

From: Tobias Schaffner <tobias.schaffner@siemens.com>

This is a first example on how to use the unittest_isar module to test
python functions defined in a bitbake file.

Signed-off-by: Tobias Schaffner <tobias.schaffner@siemens.com>
---
 .../unittests/test_image_account_extension.py | 175 ++++++++++++++++++
 1 file changed, 175 insertions(+)
 create mode 100644 testsuite/unittests/test_image_account_extension.py

diff --git a/testsuite/unittests/test_image_account_extension.py b/testsuite/unittests/test_image_account_extension.py
new file mode 100644
index 0000000..11a0aa1
--- /dev/null
+++ b/testsuite/unittests/test_image_account_extension.py
@@ -0,0 +1,175 @@
+# This software is a part of ISAR.
+# Copyright (C) Siemens AG, 2023
+#
+# SPDX-License-Identifier: MIT
+
+from bitbake import load_function, DataSmart
+from rootfs import TemporaryRootfs
+
+import unittest
+from unittest.mock import patch
+from typing import Tuple
+
+
+file_name = "meta/classes/image-account-extension.bbclass"
+image_create_users = load_function(file_name, "image_create_users")
+image_create_groups = load_function(file_name, "image_create_groups")
+
+
+class TestImageAccountExtensionCommon(unittest.TestCase):
+
+    def setup(self) -> Tuple[DataSmart, TemporaryRootfs]:
+        rootfs = TemporaryRootfs()
+
+        d = DataSmart()
+        d.setVar("ROOTFSDIR", rootfs.path())
+
+        return (d, rootfs)
+
+
+class TestImageAccountExtensionImageCreateUsers(TestImageAccountExtensionCommon):
+
+    def setup(self, user_name: str) -> Tuple[DataSmart, TemporaryRootfs]:
+        d, rootfs = super().setup()
+        rootfs.create_file(
+            "/etc/passwd", "test:x:1000:1000::/home/test:/bin/sh")
+        d.setVar("USERS", user_name)
+        return (d, rootfs)
+
+    def test_new_user(self):
+        test_user = "new"
+        d, rootfs = self.setup(test_user)
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_users(d)
+
+        run_mock.assert_called_once_with(
+            ["sudo", "-E", "chroot", rootfs.path(), "/usr/sbin/useradd", test_user])
+
+    def test_existing_user_no_change(self):
+        test_user = "test"
+        d, _ = self.setup(test_user)
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_users(d)
+
+        run_mock.assert_not_called()
+
+    def test_existing_user_home_change(self):
+        test_user = "test"
+        d, _ = self.setup(test_user)
+        d.setVarFlag("USER_{}".format(test_user), "home", "/home/new_home")
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_users(d)
+
+        assert run_mock.call_count == 1
+        assert run_mock.call_args[0][0][-5:] == ["/usr/sbin/usermod",
+                                                 '--home', '/home/new_home', '--move-home', 'test']
+
+    def test_pre_flag(self):
+        test_user = "new"
+        d, _ = self.setup(test_user)
+        d.setVarFlag("USER_{}".format(test_user), "pre", "true")
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_users(d)
+
+        run_mock.assert_not_called()
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_users(d, True)
+
+        assert run_mock.call_count == 1
+
+    def test_deterministic_password(self):
+        test_user = "new"
+        cleartext_password = "test"
+        d, _ = self.setup(test_user)
+
+        d.setVarFlag("USER_{}".format(test_user),
+                     "flags", "clear-text-password")
+        d.setVarFlag("USER_{}".format(test_user),
+                     "password", cleartext_password)
+
+        source_date_epoch = "1672427776"
+        d.setVar("SOURCE_DATE_EPOCH", source_date_epoch)
+
+        # openssl passwd -6 -salt $(echo "1672427776" | sha256sum -z | cut -c 1-15) test
+        encrypted_password = "$6$eb2e2a12cccc88a$IuhgisFe5AKM5.VREKg8wIAcPSkaJDWBM1cMUsEjNZh2Wa6BT2f5OFhqGTGpL4lFzHGN8oiwvAh0jFO1GhO3S."
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_users(d)
+
+        assert run_mock.call_count == 2
+        assert run_mock.call_args[0][1] == "{}:{}".format(
+            test_user, encrypted_password).encode()
+
+
+class TestImageAccountExtensionImageCreateGroups(TestImageAccountExtensionCommon):
+
+    def setup(self, group_name: str) -> Tuple[DataSmart, TemporaryRootfs]:
+        d, rootfs = super().setup()
+        rootfs.create_file("/etc/group", "test:x:1000:test")
+        d.setVar("GROUPS", group_name)
+        return (d, rootfs)
+
+    def test_new_group(self):
+        test_group = "new"
+        d, rootfs = self.setup(test_group)
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_groups(d)
+
+        run_mock.assert_called_once_with(
+            ["sudo", "-E", "chroot", rootfs.path(), "/usr/sbin/groupadd", test_group])
+
+    def test_existing_group_no_change(self):
+        test_group = "test"
+        d, _ = self.setup(test_group)
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_groups(d)
+
+        run_mock.assert_not_called()
+
+    def test_existing_group_id_change(self):
+        test_group = "test"
+        d, rootfs = self.setup(test_group)
+        d.setVarFlag("GROUP_{}".format(test_group), "gid", "1005")
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_groups(d)
+
+        run_mock.assert_called_once_with(
+            ["sudo", "-E", "chroot", rootfs.path(), "/usr/sbin/groupmod", "--gid", "1005", test_group])
+
+    def test_system_flag(self):
+        test_group = "test"
+        d, _ = self.setup(test_group)
+        d.setVarFlag("GROUP_{}".format(test_group), "flags", "system")
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_groups(d)
+
+        assert run_mock.call_count == 1
+        assert "--system" in run_mock.call_args[0][0]
+
+    def test_pre_flag(self):
+        test_user = "new"
+        d, _ = self.setup(test_user)
+        d.setVarFlag("GROUP_{}".format(test_user), "pre", "true")
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_groups(d)
+
+        run_mock.assert_not_called()
+
+        with patch.object(bb.process, "run") as run_mock:
+            image_create_groups(d, True)
+
+        assert run_mock.call_count == 1
+
+
+if __name__ == "__main__":
+    unittest.main()
-- 
2.34.1

[PATCH 5/5] set minimal python version in user_manual to 3.5

[T. Schaffner]

From: Tobias Schaffner <tobias.schaffner@siemens.com>

Set the minimal python version to 3.5 in the user_manual as defined in
bitbake/lib/bb/__init__.py

Signed-off-by: Tobias Schaffner <tobias.schaffner@siemens.com>
---
 doc/user_manual.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/user_manual.md b/doc/user_manual.md
index 1d209b3..af55c69 100644
--- a/doc/user_manual.md
+++ b/doc/user_manual.md
@@ -107,7 +107,7 @@ unstable/bullseye included in `/etc/apt/sources.list[.d]`).
 
 Notes:
 
-* BitBake requires Python 3.4+.
+* BitBake requires Python 3.5+.
 * The python3 package is required for the correct `alternatives` setting.
 * If you'd like to run bitbake in a container (chroot, docker, etc.), install
   the above in the container, and also perform `sudo apt-get install
-- 
2.34.1

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Henning Schild]

Am Wed, 25 Jan 2023 10:01:51 +0100
schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:

> From: Tobias Schaffner <tobias.schaffner@siemens.com>
> 
> This patch series will allow to specify a `pre` flag for the USER_ and
> GROUP_ bitbake variables. If this flag is set to `true` the given user
> or group will be created in the rootfs configuration step instead of
> on rootfs postprocessing. This is helpful when a specific id should
> be used which would otherwise be picked by a user or group created by
> one of the installed packages.

While i do understand the reason i am not sure how relevant that is.
Why would anything only function with a fixed ID? Whoever provided that
thing should maybe fix it.

So i am willing to say that this is super-niche! And it deserves a
niche-solution in its layer, not a feature in Isar.

You could hook in a task between bootstrap and image_install. Or you
could rebuild a bootstrap package to have reserved ids. You could run
"the thing" in namespaces ...

So is that really relevant? Please go into detail.

Whatever happens i think the python rewrite is cool. But the code may
have been coming/inspired from OE ... in which case it would not be
cool, because it would fork away further.

Henning

> A rewrite of the image-account-extension in python was done on the
> way. This allows us to drop a lot of encoding and parsing code that
> was used to transition to shell and therefore made it easier to read
> and maintain.
> 
> Using python functions for more complex tasks allows us the usage of
> unittests. A very basic infrastructure for unittesting using the build
> in python unittest and the bb.parse module was added. This was used to
> test the re-implementation of the image-account-extension as a first
> showcase.
> 
> Tobias Schaffner (5):
>   simplify image-account-extension
>   allow creation of users/groups before rootfs creation
>   create a minimal python unittest infrastructure
>   add unittests for the image-account-extension
>   set minimal python version in user_manual to 3.5
> 
>  doc/user_manual.md                            |   4 +-
>  meta/classes/image-account-extension.bbclass  | 391
> +++++++----------- testsuite/unittests/README.md                 |
> 28 ++ testsuite/unittests/bitbake.py                |  37 ++
>  testsuite/unittests/rootfs.py                 |  45 ++
>  .../unittests/test_image_account_extension.py | 175 ++++++++
>  6 files changed, 434 insertions(+), 246 deletions(-)
>  create mode 100644 testsuite/unittests/README.md
>  create mode 100644 testsuite/unittests/bitbake.py
>  create mode 100644 testsuite/unittests/rootfs.py
>  create mode 100644
> testsuite/unittests/test_image_account_extension.py
> 

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Gylstorff Quirin]

On 1/25/23 14:29, Henning Schild wrote:
> Am Wed, 25 Jan 2023 10:01:51 +0100
> schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> 
>> From: Tobias Schaffner <tobias.schaffner@siemens.com>
>>
>> This patch series will allow to specify a `pre` flag for the USER_ and
>> GROUP_ bitbake variables. If this flag is set to `true` the given user
>> or group will be created in the rootfs configuration step instead of
>> on rootfs postprocessing. This is helpful when a specific id should
>> be used which would otherwise be picked by a user or group created by
>> one of the installed packages.
> 
> While i do understand the reason i am not sure how relevant that is.
> Why would anything only function with a fixed ID? Whoever provided that
> thing should maybe fix it.

Specific IDs are necessary for Updating an A/B rootfs system with a 
persistent partition. In this case you do not want to change any IDs as
this can lead to wrong file permissions.
> 
> So i am willing to say that this is super-niche! And it deserves a
> niche-solution in its layer, not a feature in Isar.
> 
> You could hook in a task between bootstrap and image_install. Or you
> could rebuild a bootstrap package to have reserved ids. You could run
> "the thing" in namespaces ...
> 
> So is that really relevant? Please go into detail.
> 
> Whatever happens i think the python rewrite is cool. But the code may
> have been coming/inspired from OE ... in which case it would not be
> cool, because it would fork away further.


OE uses a complete different implementation than to original:
https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass

see also:

https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb 



Quirin
> 
> Henning
> 
>> A rewrite of the image-account-extension in python was done on the
>> way. This allows us to drop a lot of encoding and parsing code that
>> was used to transition to shell and therefore made it easier to read
>> and maintain.
>>
>> Using python functions for more complex tasks allows us the usage of
>> unittests. A very basic infrastructure for unittesting using the build
>> in python unittest and the bb.parse module was added. This was used to
>> test the re-implementation of the image-account-extension as a first
>> showcase.
>>
>> Tobias Schaffner (5):
>>    simplify image-account-extension
>>    allow creation of users/groups before rootfs creation
>>    create a minimal python unittest infrastructure
>>    add unittests for the image-account-extension
>>    set minimal python version in user_manual to 3.5
>>
>>   doc/user_manual.md                            |   4 +-
>>   meta/classes/image-account-extension.bbclass  | 391
>> +++++++----------- testsuite/unittests/README.md                 |
>> 28 ++ testsuite/unittests/bitbake.py                |  37 ++
>>   testsuite/unittests/rootfs.py                 |  45 ++
>>   .../unittests/test_image_account_extension.py | 175 ++++++++
>>   6 files changed, 434 insertions(+), 246 deletions(-)
>>   create mode 100644 testsuite/unittests/README.md
>>   create mode 100644 testsuite/unittests/bitbake.py
>>   create mode 100644 testsuite/unittests/rootfs.py
>>   create mode 100644
>> testsuite/unittests/test_image_account_extension.py
>>
> 

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Henning Schild]

Am Wed, 25 Jan 2023 14:44:40 +0100
schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:

> On 1/25/23 14:29, Henning Schild wrote:
> > Am Wed, 25 Jan 2023 10:01:51 +0100
> > schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> >   
> >> From: Tobias Schaffner <tobias.schaffner@siemens.com>
> >>
> >> This patch series will allow to specify a `pre` flag for the USER_
> >> and GROUP_ bitbake variables. If this flag is set to `true` the
> >> given user or group will be created in the rootfs configuration
> >> step instead of on rootfs postprocessing. This is helpful when a
> >> specific id should be used which would otherwise be picked by a
> >> user or group created by one of the installed packages.  
> > 
> > While i do understand the reason i am not sure how relevant that is.
> > Why would anything only function with a fixed ID? Whoever provided
> > that thing should maybe fix it.  
> 
> Specific IDs are necessary for Updating an A/B rootfs system with a 
> persistent partition. In this case you do not want to change any IDs
> as this can lead to wrong file permissions.

I see, that is much more understandable. It also goes into the
reproducible build direction.
But if that is the case the solution does not seem to be good enough
because it only considers users/groups created by isar. Not the ones
created by installing debian packages. Where the order could be
potentially random. Say you DEBIAN_DEPENDS or IMAGE_PREINSTALL "ftpd
wwwd" which will craete users "ftp www" where the two deamons do not
have any dep on each other and apt-get could install them in any order.
That order might in reality not change too often but it could i.e. when
you move from debian11 to debian12 or when you bring the third (or
10th) user-adding package into your new firmware.

So what you maybe really want is giving isar an /etc/passwd /etc/group
pair. Every new firmware is build with the given layer code and that
file-pair from the first release. Where you inject those files between
bootstrap and install ... hoping that bootstrap will always be the same.
Maybe one can inject before bootstrap ... or write a postproc function
that will find all different ids and all files and fix up. Or at least
start with an assertion in postproc that looks at the old database.

Is the problem of uid/gid depend on install order known in the debian
community and how do others solve it? Gentoo has moved from such
dynamic allocation to static some years ago, probably for similar
reasons.

Henning

> > 
> > So i am willing to say that this is super-niche! And it deserves a
> > niche-solution in its layer, not a feature in Isar.
> > 
> > You could hook in a task between bootstrap and image_install. Or you
> > could rebuild a bootstrap package to have reserved ids. You could
> > run "the thing" in namespaces ...
> > 
> > So is that really relevant? Please go into detail.
> > 
> > Whatever happens i think the python rewrite is cool. But the code
> > may have been coming/inspired from OE ... in which case it would
> > not be cool, because it would fork away further.  
> 
> 
> OE uses a complete different implementation than to original:
> https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
> 
> see also:
> 
> https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb 
> 
> 
> 
> Quirin
> > 
> > Henning
> >   
> >> A rewrite of the image-account-extension in python was done on the
> >> way. This allows us to drop a lot of encoding and parsing code that
> >> was used to transition to shell and therefore made it easier to
> >> read and maintain.
> >>
> >> Using python functions for more complex tasks allows us the usage
> >> of unittests. A very basic infrastructure for unittesting using
> >> the build in python unittest and the bb.parse module was added.
> >> This was used to test the re-implementation of the
> >> image-account-extension as a first showcase.
> >>
> >> Tobias Schaffner (5):
> >>    simplify image-account-extension
> >>    allow creation of users/groups before rootfs creation
> >>    create a minimal python unittest infrastructure
> >>    add unittests for the image-account-extension
> >>    set minimal python version in user_manual to 3.5
> >>
> >>   doc/user_manual.md                            |   4 +-
> >>   meta/classes/image-account-extension.bbclass  | 391
> >> +++++++----------- testsuite/unittests/README.md                 |
> >> 28 ++ testsuite/unittests/bitbake.py                |  37 ++
> >>   testsuite/unittests/rootfs.py                 |  45 ++
> >>   .../unittests/test_image_account_extension.py | 175 ++++++++
> >>   6 files changed, 434 insertions(+), 246 deletions(-)
> >>   create mode 100644 testsuite/unittests/README.md
> >>   create mode 100644 testsuite/unittests/bitbake.py
> >>   create mode 100644 testsuite/unittests/rootfs.py
> >>   create mode 100644
> >> testsuite/unittests/test_image_account_extension.py
> >>  
> >   
> 
> 
> 
> 

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Schaffner, Tobias]

On 25.01.23 17:29, Schild, Henning (T CED SES-DE) wrote:
> Am Wed, 25 Jan 2023 14:44:40 +0100
> schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:
> 
>> On 1/25/23 14:29, Henning Schild wrote:
>>> Am Wed, 25 Jan 2023 10:01:51 +0100
>>> schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
>>>    
>>>> From: Tobias Schaffner <tobias.schaffner@siemens.com>
>>>>
>>>> This patch series will allow to specify a `pre` flag for the USER_
>>>> and GROUP_ bitbake variables. If this flag is set to `true` the
>>>> given user or group will be created in the rootfs configuration
>>>> step instead of on rootfs postprocessing. This is helpful when a
>>>> specific id should be used which would otherwise be picked by a
>>>> user or group created by one of the installed packages.
>>>
>>> While i do understand the reason i am not sure how relevant that is.
>>> Why would anything only function with a fixed ID? Whoever provided
>>> that thing should maybe fix it.
>>
>> Specific IDs are necessary for Updating an A/B rootfs system with a
>> persistent partition. In this case you do not want to change any IDs
>> as this can lead to wrong file permissions.
> 
> I see, that is much more understandable. It also goes into the
> reproducible build direction.
> But if that is the case the solution does not seem to be good enough
> because it only considers users/groups created by isar. Not the ones
> created by installing debian packages. Where the order could be
> potentially random. Say you DEBIAN_DEPENDS or IMAGE_PREINSTALL "ftpd
> wwwd" which will craete users "ftp www" where the two deamons do not
> have any dep on each other and apt-get could install them in any order.
> That order might in reality not change too often but it could i.e. when
> you move from debian11 to debian12 or when you bring the third (or
> 10th) user-adding package into your new firmware.

Thanks for the review!

This series is not about reproducible builds. The IDs of the users and
groups that are created by debian packages are expected to change
between _image_ versions. So we expect these IDs to be different after
a swupdate.

For reproducible builds it is important that the ordering of installed
packages and their dependencies stay the same but that is a different
story. I expect the ordering algorithm for a specific set of packages
including dependencies to be deterministic but would have to look into
that in detail.

Still, if somebody had a requirement that a user or group of a package
(e.g. ftp or www) stays the same, one could use this change to create
the user beforehand and pin its id using this change.

> So what you maybe really want is giving isar an /etc/passwd /etc/group
> pair. Every new firmware is build with the given layer code and that
> file-pair from the first release. Where you inject those files between
> bootstrap and install ... hoping that bootstrap will always be the same.
> Maybe one can inject before bootstrap ... or write a postproc function
> that will find all different ids and all files and fix up. Or at least
> start with an assertion in postproc that looks at the old database.

Care that just adding /etc/passwd and /etc/group might not be enough but
you would have to handle the side effects of a useradd/groupadd call
properly (E.g. creating the home if set). And I expect more things to
come up if you have a detailed look.

Take into account that the specific patch that introduces the pre flag
is small, easy to maintain and configure.

> Is the problem of uid/gid depend on install order known in the debian
> community and how do others solve it? Gentoo has moved from such
> dynamic allocation to static some years ago, probably for similar
> reasons.

I am open to discuss this with the reproducible build guys but again
IMHO this discussion belongs into another thread.

> Henning
> 
>>>
>>> So i am willing to say that this is super-niche! And it deserves a
>>> niche-solution in its layer, not a feature in Isar.
>>>
>>> You could hook in a task between bootstrap and image_install. Or you
>>> could rebuild a bootstrap package to have reserved ids. You could
>>> run "the thing" in namespaces ...
>>>
>>> So is that really relevant? Please go into detail.
>>>
>>> Whatever happens i think the python rewrite is cool. But the code
>>> may have been coming/inspired from OE ... in which case it would
>>> not be cool, because it would fork away further.
>>
>>
>> OE uses a complete different implementation than to original:
>> https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
>>
>> see also:
>>
>> https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb
>>
>>
>>
>> Quirin
>>>
>>> Henning
>>>    
>>>> A rewrite of the image-account-extension in python was done on the
>>>> way. This allows us to drop a lot of encoding and parsing code that
>>>> was used to transition to shell and therefore made it easier to
>>>> read and maintain.
>>>>
>>>> Using python functions for more complex tasks allows us the usage
>>>> of unittests. A very basic infrastructure for unittesting using
>>>> the build in python unittest and the bb.parse module was added.
>>>> This was used to test the re-implementation of the
>>>> image-account-extension as a first showcase.
>>>>
>>>> Tobias Schaffner (5):
>>>>     simplify image-account-extension
>>>>     allow creation of users/groups before rootfs creation
>>>>     create a minimal python unittest infrastructure
>>>>     add unittests for the image-account-extension
>>>>     set minimal python version in user_manual to 3.5
>>>>
>>>>    doc/user_manual.md                            |   4 +-
>>>>    meta/classes/image-account-extension.bbclass  | 391
>>>> +++++++----------- testsuite/unittests/README.md                 |
>>>> 28 ++ testsuite/unittests/bitbake.py                |  37 ++
>>>>    testsuite/unittests/rootfs.py                 |  45 ++
>>>>    .../unittests/test_image_account_extension.py | 175 ++++++++
>>>>    6 files changed, 434 insertions(+), 246 deletions(-)
>>>>    create mode 100644 testsuite/unittests/README.md
>>>>    create mode 100644 testsuite/unittests/bitbake.py
>>>>    create mode 100644 testsuite/unittests/rootfs.py
>>>>    create mode 100644
>>>> testsuite/unittests/test_image_account_extension.py
>>>>   
>>>    
>>
>>
>>
>>
> 

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Henning Schild]

Am Wed, 25 Jan 2023 21:55:00 +0100
schrieb "Schaffner, Tobias (T CED SES-DE)"
<tobias.schaffner@siemens.com>:

> On 25.01.23 17:29, Schild, Henning (T CED SES-DE) wrote:
> > Am Wed, 25 Jan 2023 14:44:40 +0100
> > schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:
> >   
> >> On 1/25/23 14:29, Henning Schild wrote:  
> >>> Am Wed, 25 Jan 2023 10:01:51 +0100
> >>> schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> >>>      
> >>>> From: Tobias Schaffner <tobias.schaffner@siemens.com>
> >>>>
> >>>> This patch series will allow to specify a `pre` flag for the
> >>>> USER_ and GROUP_ bitbake variables. If this flag is set to
> >>>> `true` the given user or group will be created in the rootfs
> >>>> configuration step instead of on rootfs postprocessing. This is
> >>>> helpful when a specific id should be used which would otherwise
> >>>> be picked by a user or group created by one of the installed
> >>>> packages.  
> >>>
> >>> While i do understand the reason i am not sure how relevant that
> >>> is. Why would anything only function with a fixed ID? Whoever
> >>> provided that thing should maybe fix it.  
> >>
> >> Specific IDs are necessary for Updating an A/B rootfs system with a
> >> persistent partition. In this case you do not want to change any
> >> IDs as this can lead to wrong file permissions.  
> > 
> > I see, that is much more understandable. It also goes into the
> > reproducible build direction.
> > But if that is the case the solution does not seem to be good enough
> > because it only considers users/groups created by isar. Not the ones
> > created by installing debian packages. Where the order could be
> > potentially random. Say you DEBIAN_DEPENDS or IMAGE_PREINSTALL "ftpd
> > wwwd" which will craete users "ftp www" where the two deamons do not
> > have any dep on each other and apt-get could install them in any
> > order. That order might in reality not change too often but it
> > could i.e. when you move from debian11 to debian12 or when you
> > bring the third (or 10th) user-adding package into your new
> > firmware.  
> 
> Thanks for the review!
> 
> This series is not about reproducible builds. The IDs of the users and
> groups that are created by debian packages are expected to change
> between _image_ versions. So we expect these IDs to be different after
> a swupdate.

But your initial argument was that you have to create a user before any
debian package comes, or did i misunderstand? Maybe we need to
differentiate between upstream packages and isar-created ones?

> For reproducible builds it is important that the ordering of installed
> packages and their dependencies stay the same but that is a different
> story. I expect the ordering algorithm for a specific set of packages
> including dependencies to be deterministic but would have to look into
> that in detail.

I expect the algorithm to be non-deterministic ... potentially as you
progress using debian. I can just claim, you have to prove.

> Still, if somebody had a requirement that a user or group of a package
> (e.g. ftp or www) stays the same, one could use this change to create
> the user beforehand and pin its id using this change.

True. But we might still want to keep the old database to run
assertions so such things do not go unnoticed.

> > So what you maybe really want is giving isar an /etc/passwd
> > /etc/group pair. Every new firmware is build with the given layer
> > code and that file-pair from the first release. Where you inject
> > those files between bootstrap and install ... hoping that bootstrap
> > will always be the same. Maybe one can inject before bootstrap ...
> > or write a postproc function that will find all different ids and
> > all files and fix up. Or at least start with an assertion in
> > postproc that looks at the old database.  
> 
> Care that just adding /etc/passwd and /etc/group might not be enough
> but you would have to handle the side effects of a useradd/groupadd
> call properly (E.g. creating the home if set). And I expect more
> things to come up if you have a detailed look.

Yes. Not even want to think about LDAP or yp where the databases live
in different files.

> Take into account that the specific patch that introduces the pre flag
> is small, easy to maintain and configure.

It should still be motivated so that we later understand why we have
it. And i would not call it easy to configure if one does not
understand why ... people will "pre" randomly because they do not know
what they are doing. We moved that stuff from pre to post once, and i
can not help the feeling that people might want to want to mix pre and
post one day ... at which point it might become really un-easy.

> > Is the problem of uid/gid depend on install order known in the
> > debian community and how do others solve it? Gentoo has moved from
> > such dynamic allocation to static some years ago, probably for
> > similar reasons.  
> 
> I am open to discuss this with the reproducible build guys but again
> IMHO this discussion belongs into another thread.

IMHO not, the repro guys want full repro ... which we can happily leave
out when it comes to me. You want partial repro, which we should
discuss here.

I do not want to hold this back, just understand it and see if there
can be a better solution. Feel free to ignore me and merge if nobody
else asks questions.

Henning

> > Henning
> >   
> >>>
> >>> So i am willing to say that this is super-niche! And it deserves a
> >>> niche-solution in its layer, not a feature in Isar.
> >>>
> >>> You could hook in a task between bootstrap and image_install. Or
> >>> you could rebuild a bootstrap package to have reserved ids. You
> >>> could run "the thing" in namespaces ...
> >>>
> >>> So is that really relevant? Please go into detail.
> >>>
> >>> Whatever happens i think the python rewrite is cool. But the code
> >>> may have been coming/inspired from OE ... in which case it would
> >>> not be cool, because it would fork away further.  
> >>
> >>
> >> OE uses a complete different implementation than to original:
> >> https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
> >>
> >> see also:
> >>
> >> https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb
> >>
> >>
> >>
> >> Quirin  
> >>>
> >>> Henning
> >>>      
> >>>> A rewrite of the image-account-extension in python was done on
> >>>> the way. This allows us to drop a lot of encoding and parsing
> >>>> code that was used to transition to shell and therefore made it
> >>>> easier to read and maintain.
> >>>>
> >>>> Using python functions for more complex tasks allows us the usage
> >>>> of unittests. A very basic infrastructure for unittesting using
> >>>> the build in python unittest and the bb.parse module was added.
> >>>> This was used to test the re-implementation of the
> >>>> image-account-extension as a first showcase.
> >>>>
> >>>> Tobias Schaffner (5):
> >>>>     simplify image-account-extension
> >>>>     allow creation of users/groups before rootfs creation
> >>>>     create a minimal python unittest infrastructure
> >>>>     add unittests for the image-account-extension
> >>>>     set minimal python version in user_manual to 3.5
> >>>>
> >>>>    doc/user_manual.md                            |   4 +-
> >>>>    meta/classes/image-account-extension.bbclass  | 391
> >>>> +++++++----------- testsuite/unittests/README.md
> >>>> | 28 ++ testsuite/unittests/bitbake.py                |  37 ++
> >>>>    testsuite/unittests/rootfs.py                 |  45 ++
> >>>>    .../unittests/test_image_account_extension.py | 175 ++++++++
> >>>>    6 files changed, 434 insertions(+), 246 deletions(-)
> >>>>    create mode 100644 testsuite/unittests/README.md
> >>>>    create mode 100644 testsuite/unittests/bitbake.py
> >>>>    create mode 100644 testsuite/unittests/rootfs.py
> >>>>    create mode 100644
> >>>> testsuite/unittests/test_image_account_extension.py
> >>>>     
> >>>      
> >>
> >>
> >>
> >>  
> >   

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Schaffner, Tobias]

On 25.01.23 22:38, Schild, Henning (T CED SES-DE) wrote:
> Am Wed, 25 Jan 2023 21:55:00 +0100
> schrieb "Schaffner, Tobias (T CED SES-DE)"
> <tobias.schaffner@siemens.com>:
> 
>> On 25.01.23 17:29, Schild, Henning (T CED SES-DE) wrote:
>>> Am Wed, 25 Jan 2023 14:44:40 +0100
>>> schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:
>>>    
>>>> On 1/25/23 14:29, Henning Schild wrote:
>>>>> Am Wed, 25 Jan 2023 10:01:51 +0100
>>>>> schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
>>>>>       
>>>>>> From: Tobias Schaffner <tobias.schaffner@siemens.com>
>>>>>>
>>>>>> This patch series will allow to specify a `pre` flag for the
>>>>>> USER_ and GROUP_ bitbake variables. If this flag is set to
>>>>>> `true` the given user or group will be created in the rootfs
>>>>>> configuration step instead of on rootfs postprocessing. This is
>>>>>> helpful when a specific id should be used which would otherwise
>>>>>> be picked by a user or group created by one of the installed
>>>>>> packages.
>>>>>
>>>>> While i do understand the reason i am not sure how relevant that
>>>>> is. Why would anything only function with a fixed ID? Whoever
>>>>> provided that thing should maybe fix it.
>>>>
>>>> Specific IDs are necessary for Updating an A/B rootfs system with a
>>>> persistent partition. In this case you do not want to change any
>>>> IDs as this can lead to wrong file permissions.
>>>
>>> I see, that is much more understandable. It also goes into the
>>> reproducible build direction.
>>> But if that is the case the solution does not seem to be good enough
>>> because it only considers users/groups created by isar. Not the ones
>>> created by installing debian packages. Where the order could be
>>> potentially random. Say you DEBIAN_DEPENDS or IMAGE_PREINSTALL "ftpd
>>> wwwd" which will craete users "ftp www" where the two deamons do not
>>> have any dep on each other and apt-get could install them in any
>>> order. That order might in reality not change too often but it
>>> could i.e. when you move from debian11 to debian12 or when you
>>> bring the third (or 10th) user-adding package into your new
>>> firmware.
>>
>> Thanks for the review!
>>
>> This series is not about reproducible builds. The IDs of the users and
>> groups that are created by debian packages are expected to change
>> between _image_ versions. So we expect these IDs to be different after
>> a swupdate.
> 
> But your initial argument was that you have to create a user before any
> debian package comes, or did i misunderstand? Maybe we need to
> differentiate between upstream packages and isar-created ones?

Lets make this clearer with an example:
A downstream layer creates one user X that writes to a separate partition.
This user gets the ID 1000 as there are no packages that create any users.

Now they want to create a image update that should be deployed.
The updated image includes a new debian package that creates a user Y.
The user Y of this package gets the ID 1000 as it is now the first user that
is created.

User X will now get 1001 and it is not possible to change this. The data
created on the separate partition belongs to user Y.

The only resort at the moment is to revert the patch that moved the user
creation to the post processing.

>> For reproducible builds it is important that the ordering of installed
>> packages and their dependencies stay the same but that is a different
>> story. I expect the ordering algorithm for a specific set of packages
>> including dependencies to be deterministic but would have to look into
>> that in detail.
> 
> I expect the algorithm to be non-deterministic ... potentially as you
> progress using debian. I can just claim, you have to prove.

 From my point of view this problem derives from the fact that the set of
installed packages will change on image updates. If the user Y is created
in the first or in the last package does not matter. Its just the fact
that an additional user gets created.

>> Still, if somebody had a requirement that a user or group of a package
>> (e.g. ftp or www) stays the same, one could use this change to create
>> the user beforehand and pin its id using this change.
> 
> True. But we might still want to keep the old database to run
> assertions so such things do not go unnoticed.
> 
>>> So what you maybe really want is giving isar an /etc/passwd
>>> /etc/group pair. Every new firmware is build with the given layer
>>> code and that file-pair from the first release. Where you inject
>>> those files between bootstrap and install ... hoping that bootstrap
>>> will always be the same. Maybe one can inject before bootstrap ...
>>> or write a postproc function that will find all different ids and
>>> all files and fix up. Or at least start with an assertion in
>>> postproc that looks at the old database.
>>
>> Care that just adding /etc/passwd and /etc/group might not be enough
>> but you would have to handle the side effects of a useradd/groupadd
>> call properly (E.g. creating the home if set). And I expect more
>> things to come up if you have a detailed look.
> 
> Yes. Not even want to think about LDAP or yp where the databases live
> in different files.
> 
>> Take into account that the specific patch that introduces the pre flag
>> is small, easy to maintain and configure.
> 
> It should still be motivated so that we later understand why we have
> it. And i would not call it easy to configure if one does not
> understand why ... people will "pre" randomly because they do not know
> what they are doing. We moved that stuff from pre to post once, and i
> can not help the feeling that people might want to want to mix pre and
> post one day ... at which point it might become really un-easy.
> 
>>> Is the problem of uid/gid depend on install order known in the
>>> debian community and how do others solve it? Gentoo has moved from
>>> such dynamic allocation to static some years ago, probably for
>>> similar reasons.
>>
>> I am open to discuss this with the reproducible build guys but again
>> IMHO this discussion belongs into another thread.
> 
> IMHO not, the repro guys want full repro ... which we can happily leave
> out when it comes to me. You want partial repro, which we should
> discuss here.

I think you may have a valid point that a non deterministic ordering on
package install may be a problem for reproducable builds. Not only when
it comes to users but also e.g. every time two packages append to the same
file.

> I do not want to hold this back, just understand it and see if there
> can be a better solution. Feel free to ignore me and merge if nobody
> else asks questions.

I am happy to discuss this, I just think we are mixing things up. But
maybe I am missing something.

Best!

> Henning
> 
>>> Henning
>>>    
>>>>>
>>>>> So i am willing to say that this is super-niche! And it deserves a
>>>>> niche-solution in its layer, not a feature in Isar.
>>>>>
>>>>> You could hook in a task between bootstrap and image_install. Or
>>>>> you could rebuild a bootstrap package to have reserved ids. You
>>>>> could run "the thing" in namespaces ...
>>>>>
>>>>> So is that really relevant? Please go into detail.
>>>>>
>>>>> Whatever happens i think the python rewrite is cool. But the code
>>>>> may have been coming/inspired from OE ... in which case it would
>>>>> not be cool, because it would fork away further.
>>>>
>>>>
>>>> OE uses a complete different implementation than to original:
>>>> https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
>>>>
>>>> see also:
>>>>
>>>> https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb
>>>>
>>>>
>>>>
>>>> Quirin
>>>>>
>>>>> Henning
>>>>>       
>>>>>> A rewrite of the image-account-extension in python was done on
>>>>>> the way. This allows us to drop a lot of encoding and parsing
>>>>>> code that was used to transition to shell and therefore made it
>>>>>> easier to read and maintain.
>>>>>>
>>>>>> Using python functions for more complex tasks allows us the usage
>>>>>> of unittests. A very basic infrastructure for unittesting using
>>>>>> the build in python unittest and the bb.parse module was added.
>>>>>> This was used to test the re-implementation of the
>>>>>> image-account-extension as a first showcase.
>>>>>>
>>>>>> Tobias Schaffner (5):
>>>>>>      simplify image-account-extension
>>>>>>      allow creation of users/groups before rootfs creation
>>>>>>      create a minimal python unittest infrastructure
>>>>>>      add unittests for the image-account-extension
>>>>>>      set minimal python version in user_manual to 3.5
>>>>>>
>>>>>>     doc/user_manual.md                            |   4 +-
>>>>>>     meta/classes/image-account-extension.bbclass  | 391
>>>>>> +++++++----------- testsuite/unittests/README.md
>>>>>> | 28 ++ testsuite/unittests/bitbake.py                |  37 ++
>>>>>>     testsuite/unittests/rootfs.py                 |  45 ++
>>>>>>     .../unittests/test_image_account_extension.py | 175 ++++++++
>>>>>>     6 files changed, 434 insertions(+), 246 deletions(-)
>>>>>>     create mode 100644 testsuite/unittests/README.md
>>>>>>     create mode 100644 testsuite/unittests/bitbake.py
>>>>>>     create mode 100644 testsuite/unittests/rootfs.py
>>>>>>     create mode 100644
>>>>>> testsuite/unittests/test_image_account_extension.py
>>>>>>      
>>>>>       
>>>>
>>>>
>>>>
>>>>   
>>>    
> 

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Florian Bezdeka]

On Thu, 2023-01-26 at 08:21 +0000, Schaffner, Tobias wrote:
> On 25.01.23 22:38, Schild, Henning (T CED SES-DE) wrote:
> > Am Wed, 25 Jan 2023 21:55:00 +0100
> > schrieb "Schaffner, Tobias (T CED SES-DE)"
> > <tobias.schaffner@siemens.com>:
> > 
> > > On 25.01.23 17:29, Schild, Henning (T CED SES-DE) wrote:
> > > > Am Wed, 25 Jan 2023 14:44:40 +0100
> > > > schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:
> > > >    
> > > > > On 1/25/23 14:29, Henning Schild wrote:
> > > > > > Am Wed, 25 Jan 2023 10:01:51 +0100
> > > > > > schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> > > > > >       
> > > > > > > From: Tobias Schaffner <tobias.schaffner@siemens.com>
> > > > > > > 
> > > > > > > This patch series will allow to specify a `pre` flag for the
> > > > > > > USER_ and GROUP_ bitbake variables. If this flag is set to
> > > > > > > `true` the given user or group will be created in the rootfs
> > > > > > > configuration step instead of on rootfs postprocessing. This is
> > > > > > > helpful when a specific id should be used which would otherwise
> > > > > > > be picked by a user or group created by one of the installed
> > > > > > > packages.
> > > > > > 
> > > > > > While i do understand the reason i am not sure how relevant that
> > > > > > is. Why would anything only function with a fixed ID? Whoever
> > > > > > provided that thing should maybe fix it.
> > > > > 
> > > > > Specific IDs are necessary for Updating an A/B rootfs system with a
> > > > > persistent partition. In this case you do not want to change any
> > > > > IDs as this can lead to wrong file permissions.
> > > > 
> > > > I see, that is much more understandable. It also goes into the
> > > > reproducible build direction.
> > > > But if that is the case the solution does not seem to be good enough
> > > > because it only considers users/groups created by isar. Not the ones
> > > > created by installing debian packages. Where the order could be
> > > > potentially random. Say you DEBIAN_DEPENDS or IMAGE_PREINSTALL "ftpd
> > > > wwwd" which will craete users "ftp www" where the two deamons do not
> > > > have any dep on each other and apt-get could install them in any
> > > > order. That order might in reality not change too often but it
> > > > could i.e. when you move from debian11 to debian12 or when you
> > > > bring the third (or 10th) user-adding package into your new
> > > > firmware.
> > > 
> > > Thanks for the review!
> > > 
> > > This series is not about reproducible builds. The IDs of the users and
> > > groups that are created by debian packages are expected to change
> > > between _image_ versions. So we expect these IDs to be different after
> > > a swupdate.
> > 
> > But your initial argument was that you have to create a user before any
> > debian package comes, or did i misunderstand? Maybe we need to
> > differentiate between upstream packages and isar-created ones?
> 
> Lets make this clearer with an example:
> A downstream layer creates one user X that writes to a separate partition.
> This user gets the ID 1000 as there are no packages that create any users.
> 
> Now they want to create a image update that should be deployed.
> The updated image includes a new debian package that creates a user Y.
> The user Y of this package gets the ID 1000 as it is now the first user that
> is created.
> 
> User X will now get 1001 and it is not possible to change this. The data
> created on the separate partition belongs to user Y.
> 
> The only resort at the moment is to revert the patch that moved the user
> creation to the post processing.

Hm... I have the strong feeling that your approach is broken in the
same way...

I was questioning myself how Debian itself solves this problem. The
answer is: Debian has access to the "old" or "current" user database
while doing updates. So it can not happen that a user gets a different
UID. We would have to feed in the "current" user database during such
updates to do it the same way.

Why your approach is broken: Consider three image versions A, B and C.
There is no guarantee that someone will update from A to B and then to
C. A -> C is also possible.

Image A can add a user using your "pre" logic, B can remove that user,
C can introduce a new different user, but with same ID. That brings you
into the same situation.

As Isar can't tell if that is a dangerous scenario for all the possible
use cases it's a NACK security wise.


> 
> > > For reproducible builds it is important that the ordering of installed
> > > packages and their dependencies stay the same but that is a different
> > > story. I expect the ordering algorithm for a specific set of packages
> > > including dependencies to be deterministic but would have to look into
> > > that in detail.
> > 
> > I expect the algorithm to be non-deterministic ... potentially as you
> > progress using debian. I can just claim, you have to prove.
> 
>  From my point of view this problem derives from the fact that the set of
> installed packages will change on image updates. If the user Y is created
> in the first or in the last package does not matter. Its just the fact
> that an additional user gets created.
> 
> > > Still, if somebody had a requirement that a user or group of a package
> > > (e.g. ftp or www) stays the same, one could use this change to create
> > > the user beforehand and pin its id using this change.
> > 
> > True. But we might still want to keep the old database to run
> > assertions so such things do not go unnoticed.
> > 
> > > > So what you maybe really want is giving isar an /etc/passwd
> > > > /etc/group pair. Every new firmware is build with the given layer
> > > > code and that file-pair from the first release. Where you inject
> > > > those files between bootstrap and install ... hoping that bootstrap
> > > > will always be the same. Maybe one can inject before bootstrap ...
> > > > or write a postproc function that will find all different ids and
> > > > all files and fix up. Or at least start with an assertion in
> > > > postproc that looks at the old database.
> > > 
> > > Care that just adding /etc/passwd and /etc/group might not be enough
> > > but you would have to handle the side effects of a useradd/groupadd
> > > call properly (E.g. creating the home if set). And I expect more
> > > things to come up if you have a detailed look.
> > 
> > Yes. Not even want to think about LDAP or yp where the databases live
> > in different files.
> > 
> > > Take into account that the specific patch that introduces the pre flag
> > > is small, easy to maintain and configure.
> > 
> > It should still be motivated so that we later understand why we have
> > it. And i would not call it easy to configure if one does not
> > understand why ... people will "pre" randomly because they do not know
> > what they are doing. We moved that stuff from pre to post once, and i
> > can not help the feeling that people might want to want to mix pre and
> > post one day ... at which point it might become really un-easy.
> > 
> > > > Is the problem of uid/gid depend on install order known in the
> > > > debian community and how do others solve it? Gentoo has moved from
> > > > such dynamic allocation to static some years ago, probably for
> > > > similar reasons.
> > > 
> > > I am open to discuss this with the reproducible build guys but again
> > > IMHO this discussion belongs into another thread.
> > 
> > IMHO not, the repro guys want full repro ... which we can happily leave
> > out when it comes to me. You want partial repro, which we should
> > discuss here.
> 
> I think you may have a valid point that a non deterministic ordering on
> package install may be a problem for reproducable builds. Not only when
> it comes to users but also e.g. every time two packages append to the same
> file.
> 
> > I do not want to hold this back, just understand it and see if there
> > can be a better solution. Feel free to ignore me and merge if nobody
> > else asks questions.
> 
> I am happy to discuss this, I just think we are mixing things up. But
> maybe I am missing something.
> 
> Best!
> 
> > Henning
> > 
> > > > Henning
> > > >    
> > > > > > 
> > > > > > So i am willing to say that this is super-niche! And it deserves a
> > > > > > niche-solution in its layer, not a feature in Isar.
> > > > > > 
> > > > > > You could hook in a task between bootstrap and image_install. Or
> > > > > > you could rebuild a bootstrap package to have reserved ids. You
> > > > > > could run "the thing" in namespaces ...
> > > > > > 
> > > > > > So is that really relevant? Please go into detail.
> > > > > > 
> > > > > > Whatever happens i think the python rewrite is cool. But the code
> > > > > > may have been coming/inspired from OE ... in which case it would
> > > > > > not be cool, because it would fork away further.
> > > > > 
> > > > > 
> > > > > OE uses a complete different implementation than to original:
> > > > > https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
> > > > > 
> > > > > see also:
> > > > > 
> > > > > https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb
> > > > > 
> > > > > 
> > > > > 
> > > > > Quirin
> > > > > > 
> > > > > > Henning
> > > > > >       
> > > > > > > A rewrite of the image-account-extension in python was done on
> > > > > > > the way. This allows us to drop a lot of encoding and parsing
> > > > > > > code that was used to transition to shell and therefore made it
> > > > > > > easier to read and maintain.
> > > > > > > 
> > > > > > > Using python functions for more complex tasks allows us the usage
> > > > > > > of unittests. A very basic infrastructure for unittesting using
> > > > > > > the build in python unittest and the bb.parse module was added.
> > > > > > > This was used to test the re-implementation of the
> > > > > > > image-account-extension as a first showcase.
> > > > > > > 
> > > > > > > Tobias Schaffner (5):
> > > > > > >      simplify image-account-extension
> > > > > > >      allow creation of users/groups before rootfs creation
> > > > > > >      create a minimal python unittest infrastructure
> > > > > > >      add unittests for the image-account-extension
> > > > > > >      set minimal python version in user_manual to 3.5
> > > > > > > 
> > > > > > >     doc/user_manual.md                            |   4 +-
> > > > > > >     meta/classes/image-account-extension.bbclass  | 391
> > > > > > > +++++++----------- testsuite/unittests/README.md
> > > > > > > > 28 ++ testsuite/unittests/bitbake.py                |  37 ++
> > > > > > >     testsuite/unittests/rootfs.py                 |  45 ++
> > > > > > >     .../unittests/test_image_account_extension.py | 175 ++++++++
> > > > > > >     6 files changed, 434 insertions(+), 246 deletions(-)
> > > > > > >     create mode 100644 testsuite/unittests/README.md
> > > > > > >     create mode 100644 testsuite/unittests/bitbake.py
> > > > > > >     create mode 100644 testsuite/unittests/rootfs.py
> > > > > > >     create mode 100644
> > > > > > > testsuite/unittests/test_image_account_extension.py
> > > > > > >      
> > > > > >       
> > > > > 
> > > > > 
> > > > > 
> > > > >   
> > > >    
> > 
> 

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Henning Schild]

Am Thu, 26 Jan 2023 09:48:06 +0100
schrieb Florian Bezdeka <florian.bezdeka@siemens.com>:

> On Thu, 2023-01-26 at 08:21 +0000, Schaffner, Tobias wrote:
> > On 25.01.23 22:38, Schild, Henning (T CED SES-DE) wrote:  
> > > Am Wed, 25 Jan 2023 21:55:00 +0100
> > > schrieb "Schaffner, Tobias (T CED SES-DE)"
> > > <tobias.schaffner@siemens.com>:
> > >   
> > > > On 25.01.23 17:29, Schild, Henning (T CED SES-DE) wrote:  
> > > > > Am Wed, 25 Jan 2023 14:44:40 +0100
> > > > > schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:
> > > > >      
> > > > > > On 1/25/23 14:29, Henning Schild wrote:  
> > > > > > > Am Wed, 25 Jan 2023 10:01:51 +0100
> > > > > > > schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> > > > > > >         
> > > > > > > > From: Tobias Schaffner <tobias.schaffner@siemens.com>
> > > > > > > > 
> > > > > > > > This patch series will allow to specify a `pre` flag
> > > > > > > > for the USER_ and GROUP_ bitbake variables. If this
> > > > > > > > flag is set to `true` the given user or group will be
> > > > > > > > created in the rootfs configuration step instead of on
> > > > > > > > rootfs postprocessing. This is helpful when a specific
> > > > > > > > id should be used which would otherwise be picked by a
> > > > > > > > user or group created by one of the installed packages.
> > > > > > > >  
> > > > > > > 
> > > > > > > While i do understand the reason i am not sure how
> > > > > > > relevant that is. Why would anything only function with a
> > > > > > > fixed ID? Whoever provided that thing should maybe fix
> > > > > > > it.  
> > > > > > 
> > > > > > Specific IDs are necessary for Updating an A/B rootfs
> > > > > > system with a persistent partition. In this case you do not
> > > > > > want to change any IDs as this can lead to wrong file
> > > > > > permissions.  
> > > > > 
> > > > > I see, that is much more understandable. It also goes into the
> > > > > reproducible build direction.
> > > > > But if that is the case the solution does not seem to be good
> > > > > enough because it only considers users/groups created by
> > > > > isar. Not the ones created by installing debian packages.
> > > > > Where the order could be potentially random. Say you
> > > > > DEBIAN_DEPENDS or IMAGE_PREINSTALL "ftpd wwwd" which will
> > > > > craete users "ftp www" where the two deamons do not have any
> > > > > dep on each other and apt-get could install them in any
> > > > > order. That order might in reality not change too often but
> > > > > it could i.e. when you move from debian11 to debian12 or when
> > > > > you bring the third (or 10th) user-adding package into your
> > > > > new firmware.  
> > > > 
> > > > Thanks for the review!
> > > > 
> > > > This series is not about reproducible builds. The IDs of the
> > > > users and groups that are created by debian packages are
> > > > expected to change between _image_ versions. So we expect these
> > > > IDs to be different after a swupdate.  
> > > 
> > > But your initial argument was that you have to create a user
> > > before any debian package comes, or did i misunderstand? Maybe we
> > > need to differentiate between upstream packages and isar-created
> > > ones?  
> > 
> > Lets make this clearer with an example:
> > A downstream layer creates one user X that writes to a separate
> > partition. This user gets the ID 1000 as there are no packages that
> > create any users.
> > 
> > Now they want to create a image update that should be deployed.
> > The updated image includes a new debian package that creates a user
> > Y. The user Y of this package gets the ID 1000 as it is now the
> > first user that is created.
> > 
> > User X will now get 1001 and it is not possible to change this. The
> > data created on the separate partition belongs to user Y.
> > 
> > The only resort at the moment is to revert the patch that moved the
> > user creation to the post processing.  
> 
> Hm... I have the strong feeling that your approach is broken in the
> same way...
> 
> I was questioning myself how Debian itself solves this problem. The
> answer is: Debian has access to the "old" or "current" user database
> while doing updates. So it can not happen that a user gets a different
> UID. We would have to feed in the "current" user database during such
> updates to do it the same way.
> 
> Why your approach is broken: Consider three image versions A, B and C.
> There is no guarantee that someone will update from A to B and then to
> C. A -> C is also possible.
> 
> Image A can add a user using your "pre" logic, B can remove that user,
> C can introduce a new different user, but with same ID. That brings
> you into the same situation.

Whether a firmware update supports skipping intermediates is up to
whoever builds a product. Interface changing releases should always
support old and new interface. And at some point one can drop the old.

The most trivial/conservative implementation would be to make any
update a stop-release which has to be taken. That greatly reduces the
testing effort and you have to think less about interfaces. But it can
lead to a bad user experience if you have to update too often because
you can not just jump.

Henning

> As Isar can't tell if that is a dangerous scenario for all the
> possible use cases it's a NACK security wise.
> 
> 
> >   
> > > > For reproducible builds it is important that the ordering of
> > > > installed packages and their dependencies stay the same but
> > > > that is a different story. I expect the ordering algorithm for
> > > > a specific set of packages including dependencies to be
> > > > deterministic but would have to look into that in detail.  
> > > 
> > > I expect the algorithm to be non-deterministic ... potentially as
> > > you progress using debian. I can just claim, you have to prove.  
> > 
> >  From my point of view this problem derives from the fact that the
> > set of installed packages will change on image updates. If the user
> > Y is created in the first or in the last package does not matter.
> > Its just the fact that an additional user gets created.
> >   
> > > > Still, if somebody had a requirement that a user or group of a
> > > > package (e.g. ftp or www) stays the same, one could use this
> > > > change to create the user beforehand and pin its id using this
> > > > change.  
> > > 
> > > True. But we might still want to keep the old database to run
> > > assertions so such things do not go unnoticed.
> > >   
> > > > > So what you maybe really want is giving isar an /etc/passwd
> > > > > /etc/group pair. Every new firmware is build with the given
> > > > > layer code and that file-pair from the first release. Where
> > > > > you inject those files between bootstrap and install ...
> > > > > hoping that bootstrap will always be the same. Maybe one can
> > > > > inject before bootstrap ... or write a postproc function that
> > > > > will find all different ids and all files and fix up. Or at
> > > > > least start with an assertion in postproc that looks at the
> > > > > old database.  
> > > > 
> > > > Care that just adding /etc/passwd and /etc/group might not be
> > > > enough but you would have to handle the side effects of a
> > > > useradd/groupadd call properly (E.g. creating the home if set).
> > > > And I expect more things to come up if you have a detailed
> > > > look.  
> > > 
> > > Yes. Not even want to think about LDAP or yp where the databases
> > > live in different files.
> > >   
> > > > Take into account that the specific patch that introduces the
> > > > pre flag is small, easy to maintain and configure.  
> > > 
> > > It should still be motivated so that we later understand why we
> > > have it. And i would not call it easy to configure if one does not
> > > understand why ... people will "pre" randomly because they do not
> > > know what they are doing. We moved that stuff from pre to post
> > > once, and i can not help the feeling that people might want to
> > > want to mix pre and post one day ... at which point it might
> > > become really un-easy. 
> > > > > Is the problem of uid/gid depend on install order known in the
> > > > > debian community and how do others solve it? Gentoo has moved
> > > > > from such dynamic allocation to static some years ago,
> > > > > probably for similar reasons.  
> > > > 
> > > > I am open to discuss this with the reproducible build guys but
> > > > again IMHO this discussion belongs into another thread.  
> > > 
> > > IMHO not, the repro guys want full repro ... which we can happily
> > > leave out when it comes to me. You want partial repro, which we
> > > should discuss here.  
> > 
> > I think you may have a valid point that a non deterministic
> > ordering on package install may be a problem for reproducable
> > builds. Not only when it comes to users but also e.g. every time
> > two packages append to the same file.
> >   
> > > I do not want to hold this back, just understand it and see if
> > > there can be a better solution. Feel free to ignore me and merge
> > > if nobody else asks questions.  
> > 
> > I am happy to discuss this, I just think we are mixing things up.
> > But maybe I am missing something.
> > 
> > Best!
> >   
> > > Henning
> > >   
> > > > > Henning
> > > > >      
> > > > > > > 
> > > > > > > So i am willing to say that this is super-niche! And it
> > > > > > > deserves a niche-solution in its layer, not a feature in
> > > > > > > Isar.
> > > > > > > 
> > > > > > > You could hook in a task between bootstrap and
> > > > > > > image_install. Or you could rebuild a bootstrap package
> > > > > > > to have reserved ids. You could run "the thing" in
> > > > > > > namespaces ...
> > > > > > > 
> > > > > > > So is that really relevant? Please go into detail.
> > > > > > > 
> > > > > > > Whatever happens i think the python rewrite is cool. But
> > > > > > > the code may have been coming/inspired from OE ... in
> > > > > > > which case it would not be cool, because it would fork
> > > > > > > away further.  
> > > > > > 
> > > > > > 
> > > > > > OE uses a complete different implementation than to
> > > > > > original:
> > > > > > https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
> > > > > > 
> > > > > > see also:
> > > > > > 
> > > > > > https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Quirin  
> > > > > > > 
> > > > > > > Henning
> > > > > > >         
> > > > > > > > A rewrite of the image-account-extension in python was
> > > > > > > > done on the way. This allows us to drop a lot of
> > > > > > > > encoding and parsing code that was used to transition
> > > > > > > > to shell and therefore made it easier to read and
> > > > > > > > maintain.
> > > > > > > > 
> > > > > > > > Using python functions for more complex tasks allows us
> > > > > > > > the usage of unittests. A very basic infrastructure for
> > > > > > > > unittesting using the build in python unittest and the
> > > > > > > > bb.parse module was added. This was used to test the
> > > > > > > > re-implementation of the image-account-extension as a
> > > > > > > > first showcase.
> > > > > > > > 
> > > > > > > > Tobias Schaffner (5):
> > > > > > > >      simplify image-account-extension
> > > > > > > >      allow creation of users/groups before rootfs
> > > > > > > > creation create a minimal python unittest infrastructure
> > > > > > > >      add unittests for the image-account-extension
> > > > > > > >      set minimal python version in user_manual to 3.5
> > > > > > > > 
> > > > > > > >     doc/user_manual.md                            |   4
> > > > > > > > +- meta/classes/image-account-extension.bbclass  | 391
> > > > > > > > +++++++----------- testsuite/unittests/README.md  
> > > > > > > > > 28 ++ testsuite/unittests/bitbake.py                |
> > > > > > > > >  37 ++  
> > > > > > > >     testsuite/unittests/rootfs.py                 |  45
> > > > > > > > ++ .../unittests/test_image_account_extension.py | 175
> > > > > > > > ++++++++ 6 files changed, 434 insertions(+), 246
> > > > > > > > deletions(-) create mode 100644
> > > > > > > > testsuite/unittests/README.md create mode 100644
> > > > > > > > testsuite/unittests/bitbake.py create mode 100644
> > > > > > > > testsuite/unittests/rootfs.py create mode 100644
> > > > > > > > testsuite/unittests/test_image_account_extension.py
> > > > > > > >        
> > > > > > >         
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >     
> > > > >      
> > >   
> >   
> 

Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Henning Schild]

Am Thu, 26 Jan 2023 09:21:12 +0100
schrieb "Schaffner, Tobias (T CED SES-DE)"
<tobias.schaffner@siemens.com>:

> On 25.01.23 22:38, Schild, Henning (T CED SES-DE) wrote:
> > Am Wed, 25 Jan 2023 21:55:00 +0100
> > schrieb "Schaffner, Tobias (T CED SES-DE)"
> > <tobias.schaffner@siemens.com>:
> >   
> >> On 25.01.23 17:29, Schild, Henning (T CED SES-DE) wrote:  
> >>> Am Wed, 25 Jan 2023 14:44:40 +0100
> >>> schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:
> >>>      
> >>>> On 1/25/23 14:29, Henning Schild wrote:  
> >>>>> Am Wed, 25 Jan 2023 10:01:51 +0100
> >>>>> schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> >>>>>         
> >>>>>> From: Tobias Schaffner <tobias.schaffner@siemens.com>
> >>>>>>
> >>>>>> This patch series will allow to specify a `pre` flag for the
> >>>>>> USER_ and GROUP_ bitbake variables. If this flag is set to
> >>>>>> `true` the given user or group will be created in the rootfs
> >>>>>> configuration step instead of on rootfs postprocessing. This is
> >>>>>> helpful when a specific id should be used which would otherwise
> >>>>>> be picked by a user or group created by one of the installed
> >>>>>> packages.  
> >>>>>
> >>>>> While i do understand the reason i am not sure how relevant that
> >>>>> is. Why would anything only function with a fixed ID? Whoever
> >>>>> provided that thing should maybe fix it.  
> >>>>
> >>>> Specific IDs are necessary for Updating an A/B rootfs system
> >>>> with a persistent partition. In this case you do not want to
> >>>> change any IDs as this can lead to wrong file permissions.  
> >>>
> >>> I see, that is much more understandable. It also goes into the
> >>> reproducible build direction.
> >>> But if that is the case the solution does not seem to be good
> >>> enough because it only considers users/groups created by isar.
> >>> Not the ones created by installing debian packages. Where the
> >>> order could be potentially random. Say you DEBIAN_DEPENDS or
> >>> IMAGE_PREINSTALL "ftpd wwwd" which will craete users "ftp www"
> >>> where the two deamons do not have any dep on each other and
> >>> apt-get could install them in any order. That order might in
> >>> reality not change too often but it could i.e. when you move from
> >>> debian11 to debian12 or when you bring the third (or 10th)
> >>> user-adding package into your new firmware.  
> >>
> >> Thanks for the review!
> >>
> >> This series is not about reproducible builds. The IDs of the users
> >> and groups that are created by debian packages are expected to
> >> change between _image_ versions. So we expect these IDs to be
> >> different after a swupdate.  
> > 
> > But your initial argument was that you have to create a user before
> > any debian package comes, or did i misunderstand? Maybe we need to
> > differentiate between upstream packages and isar-created ones?  
> 
> Lets make this clearer with an example:
> A downstream layer creates one user X that writes to a separate
> partition. This user gets the ID 1000 as there are no packages that
> create any users.
> 
> Now they want to create a image update that should be deployed.
> The updated image includes a new debian package that creates a user Y.
> The user Y of this package gets the ID 1000 as it is now the first
> user that is created.

Not sure a real debian package would create a non-system user and get a
1000 allocated in the first place. Are we talking about an
isar-generated package creating a non-system user?
 
> User X will now get 1001 and it is not possible to change this. The
> data created on the separate partition belongs to user Y.
> 
> The only resort at the moment is to revert the patch that moved the
> user creation to the post processing.
> 
> >> For reproducible builds it is important that the ordering of
> >> installed packages and their dependencies stay the same but that
> >> is a different story. I expect the ordering algorithm for a
> >> specific set of packages including dependencies to be
> >> deterministic but would have to look into that in detail.  
> > 
> > I expect the algorithm to be non-deterministic ... potentially as
> > you progress using debian. I can just claim, you have to prove.  
> 
>  From my point of view this problem derives from the fact that the
> set of installed packages will change on image updates. If the user Y
> is created in the first or in the last package does not matter. Its
> just the fact that an additional user gets created.
> 
> >> Still, if somebody had a requirement that a user or group of a
> >> package (e.g. ftp or www) stays the same, one could use this
> >> change to create the user beforehand and pin its id using this
> >> change.  
> > 
> > True. But we might still want to keep the old database to run
> > assertions so such things do not go unnoticed.
> >   
> >>> So what you maybe really want is giving isar an /etc/passwd
> >>> /etc/group pair. Every new firmware is build with the given layer
> >>> code and that file-pair from the first release. Where you inject
> >>> those files between bootstrap and install ... hoping that
> >>> bootstrap will always be the same. Maybe one can inject before
> >>> bootstrap ... or write a postproc function that will find all
> >>> different ids and all files and fix up. Or at least start with an
> >>> assertion in postproc that looks at the old database.  
> >>
> >> Care that just adding /etc/passwd and /etc/group might not be
> >> enough but you would have to handle the side effects of a
> >> useradd/groupadd call properly (E.g. creating the home if set).
> >> And I expect more things to come up if you have a detailed look.  
> > 
> > Yes. Not even want to think about LDAP or yp where the databases
> > live in different files.
> >   
> >> Take into account that the specific patch that introduces the pre
> >> flag is small, easy to maintain and configure.  
> > 
> > It should still be motivated so that we later understand why we have
> > it. And i would not call it easy to configure if one does not
> > understand why ... people will "pre" randomly because they do not
> > know what they are doing. We moved that stuff from pre to post
> > once, and i can not help the feeling that people might want to want
> > to mix pre and post one day ... at which point it might become
> > really un-easy. 
> >>> Is the problem of uid/gid depend on install order known in the
> >>> debian community and how do others solve it? Gentoo has moved from
> >>> such dynamic allocation to static some years ago, probably for
> >>> similar reasons.  
> >>
> >> I am open to discuss this with the reproducible build guys but
> >> again IMHO this discussion belongs into another thread.  
> > 
> > IMHO not, the repro guys want full repro ... which we can happily
> > leave out when it comes to me. You want partial repro, which we
> > should discuss here.  
> 
> I think you may have a valid point that a non deterministic ordering
> on package install may be a problem for reproducable builds. Not only
> when it comes to users but also e.g. every time two packages append
> to the same file.
> 
> > I do not want to hold this back, just understand it and see if there
> > can be a better solution. Feel free to ignore me and merge if nobody
> > else asks questions.  
> 
> I am happy to discuss this, I just think we are mixing things up. But
> maybe I am missing something.

Yes it is a mix. The repro guys are currently focussing on the very
same code generating the very same output. Here we talk about the next
gen of the code needing to repro bits that have to stay the same.

We keep discussing a solution and abstracted away problems. Maybe it
would be better to put the real problem at hand on the table. Then we
can throw around ideas on how that affected layer could deal with it,
and how isar could change to help in such cases.

The move from pre to post we did some time ago, might indeed make that
a problem we caused in all such layers that had a release at pre-times
and now want the next at post-time.

And touching this again might have the next such effect which we only
start understanding once it is too late.

And we should add tests. Looking at Isar today, we have the user "isar"
added both in local.conf.sample and in example-raw. Maybe that should
be two different users where one would even be non-system.

Henning

> Best!
> 
> > Henning
> >   
> >>> Henning
> >>>      
> >>>>>
> >>>>> So i am willing to say that this is super-niche! And it
> >>>>> deserves a niche-solution in its layer, not a feature in Isar.
> >>>>>
> >>>>> You could hook in a task between bootstrap and image_install. Or
> >>>>> you could rebuild a bootstrap package to have reserved ids. You
> >>>>> could run "the thing" in namespaces ...
> >>>>>
> >>>>> So is that really relevant? Please go into detail.
> >>>>>
> >>>>> Whatever happens i think the python rewrite is cool. But the
> >>>>> code may have been coming/inspired from OE ... in which case it
> >>>>> would not be cool, because it would fork away further.  
> >>>>
> >>>>
> >>>> OE uses a complete different implementation than to original:
> >>>> https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
> >>>>
> >>>> see also:
> >>>>
> >>>> https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb
> >>>>
> >>>>
> >>>>
> >>>> Quirin  
> >>>>>
> >>>>> Henning
> >>>>>         
> >>>>>> A rewrite of the image-account-extension in python was done on
> >>>>>> the way. This allows us to drop a lot of encoding and parsing
> >>>>>> code that was used to transition to shell and therefore made it
> >>>>>> easier to read and maintain.
> >>>>>>
> >>>>>> Using python functions for more complex tasks allows us the
> >>>>>> usage of unittests. A very basic infrastructure for
> >>>>>> unittesting using the build in python unittest and the
> >>>>>> bb.parse module was added. This was used to test the
> >>>>>> re-implementation of the image-account-extension as a first
> >>>>>> showcase.
> >>>>>>
> >>>>>> Tobias Schaffner (5):
> >>>>>>      simplify image-account-extension
> >>>>>>      allow creation of users/groups before rootfs creation
> >>>>>>      create a minimal python unittest infrastructure
> >>>>>>      add unittests for the image-account-extension
> >>>>>>      set minimal python version in user_manual to 3.5
> >>>>>>
> >>>>>>     doc/user_manual.md                            |   4 +-
> >>>>>>     meta/classes/image-account-extension.bbclass  | 391
> >>>>>> +++++++----------- testsuite/unittests/README.md
> >>>>>> | 28 ++ testsuite/unittests/bitbake.py                |  37 ++
> >>>>>>     testsuite/unittests/rootfs.py                 |  45 ++
> >>>>>>     .../unittests/test_image_account_extension.py | 175
> >>>>>> ++++++++ 6 files changed, 434 insertions(+), 246 deletions(-)
> >>>>>>     create mode 100644 testsuite/unittests/README.md
> >>>>>>     create mode 100644 testsuite/unittests/bitbake.py
> >>>>>>     create mode 100644 testsuite/unittests/rootfs.py
> >>>>>>     create mode 100644
> >>>>>> testsuite/unittests/test_image_account_extension.py
> >>>>>>        
> >>>>>         
> >>>>
> >>>>
> >>>>
> >>>>     
> >>>      
> >