Re: [PATCH 1/4] simplify image-account-extension

["Schaffner, Tobias" <tobias.schaffner@siemens.com>]

On 30.03.23 21:55, Schild, Henning (T CED SES-DE) wrote:
> Am Thu, 30 Mar 2023 13:08:01 +0200
> schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> 
>> 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
>> 1a1f704d..d1133bb4 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
> 
> 2019-2023
> 
> did not look at the rest, this feels like it should be tried out ...
> functional review, which i would try to do
> 
> please make sure that would pass flake8 ... might be hard since it is
> "embedded code"

Hi Henning,

the code already pep8 compliant and passes flake8 with two exceptions:
  * line length may be > 79 chars (E501) as everywhere else in the project
  * bb and os modules are not visible to flake8 (F821) as they are 
imported by bitbake

Actually it is not that hard. If you want to use flake8 from the command 
line you can write a wrapper for the handle function in the bb.parse 
module similar to the bitbake module that I created in 
testsuite/unittests. If you print the python functions to the stdout you 
can pipe it to the stdin of flake8.

If you use vscode with python extensions its even easier. Just configure 
your desired linter and set the Language Mode to Python for the open 
.bbclass.

Best,
Tobias

> a long time ago we had contributions from Harald Seiler, who managed to
> somehow lint python code inside bb code.
> 
> Henning >>   #
>>   # 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) 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').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').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") or "").split()
>> +    rootfsdir = d.getVar("ROOTFSDIR")
>> +    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") or ""
>> +        if gid:
>> +            args.append("--gid")
>> +            args.append(gid)
>> +
>> +        flags = (d.getVarFlag(group_entry, "flags") 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") or "").split()
>> +    rootfsdir = d.getVar("ROOTFSDIR")
>> +    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) 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") or ""
>> +        if groups:
>> +            args.append("--groups")
>> +            args.append(groups.replace(' ', ','))
>> +
>> +        flags = (d.getVarFlag(user_entry, "flags") or "").split()
>> +
>> +        if exists:
>> +            add_user_option("--home", "home")
>> +            if d.getVarFlag(user_entry, "home") 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") 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") 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)
>>   }
>