[RFC] incorrect parsing of sysusers.d in rootfs generation

[RFC] incorrect parsing of sysusers.d in rootfs generation

[Louis Rannou]

Hello,

I have found an issue in the rootfs routine. The 
rootfs-postcommands.bbclass has a funtion systemd_create_users that 
reads /etc/sysusers.d/*.conf files and parses lines as 'type name id 
comment'.

However, the sysusers.d manual says, those lines can be 'type name id 
comment home_dir shell'. If a home directory of shell is defined, they 
are considered as part of the comment, and we run incorrect commands 
such as the one below :

useradd --shell /sbin/nologin --uid 0 --comment "Super User" /root 
--system root

To fix that, we require a stronger parsing. Several options look 
possible to me, but I am not sure which one is preferred.

1. sed with a regular expression that returns something that still needs 
parsing
2. awk with a step by step script that returns something that still 
needs to be parsed
3. use python and regexp module

Also I don't know if the parsing should completely check the sysusers 
syntax as said in the manual (first field is [urgm], second is 
alphanum_-, etc.). In my opinion it should not as this will be made by 
the useradd command.

Do you think it worth to add some testing about that ? I am not sure how 
to do that.

Regards,
Louis Rannou

Re: [RFC] incorrect parsing of sysusers.d in rootfs generation

[Louis Rannou]

Hello again,

a python solution could be one below.

Also, I found that most of users/groups defined there are redundant as 
they already exist (such as root). I guess they are defined from 
base-passwd. I am not sure which recipe (base-passwd or systemd) should 
have the precedence on this. If it's base-passwd, perhaps this 
postcommand should check first if the user does already exist.

Regards,
Louis

---

  meta/classes/rootfs-postcommands.bbclass | 69 ++++++++++++++++--------
  1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/meta/classes/rootfs-postcommands.bbclass 
b/meta/classes/rootfs-postcommands.bbclass
index 5c0b3ec37c..1741919918 100644
--- a/meta/classes/rootfs-postcommands.bbclass
+++ b/meta/classes/rootfs-postcommands.bbclass
@@ -61,29 +61,52 @@ python () {
      d.appendVar('ROOTFS_POSTPROCESS_COMMAND', 'rootfs_reproducible;')
  }

-systemd_create_users () {
-	for conffile in ${IMAGE_ROOTFS}/usr/lib/sysusers.d/*.conf; do
-		[ -e $conffile ] || continue
-		grep -v "^#" $conffile | sed -e '/^$/d' | while read type name id 
comment; do
-		if [ "$type" = "u" ]; then
-			useradd_params="--shell /sbin/nologin"
-			[ "$id" != "-" ] && useradd_params="$useradd_params --uid $id"
-			[ "$comment" != "-" ] && useradd_params="$useradd_params --comment 
$comment"
-			useradd_params="$useradd_params --system $name"
-			eval useradd --root ${IMAGE_ROOTFS} $useradd_params || true
-		elif [ "$type" = "g" ]; then
-			groupadd_params=""
-			[ "$id" != "-" ] && groupadd_params="$groupadd_params --gid $id"
-			groupadd_params="$groupadd_params --system $name"
-			eval groupadd --root ${IMAGE_ROOTFS} $groupadd_params || true
-		elif [ "$type" = "m" ]; then
-			group=$id
-			eval groupadd --root ${IMAGE_ROOTFS} --system $group || true
-			eval useradd --root ${IMAGE_ROOTFS} --shell /sbin/nologin --system 
$name --no-user-group || true
-			eval usermod --root ${IMAGE_ROOTFS} -a -G $group $name
-		fi
-		done
-	done
+python systemd_create_users() {
+    import glob
+    import re
+    import subprocess
+
+    pattern_comment = r'(-|\"[^:\"]+\")'
+    pattern_word    = r'[^\s]+'
+    pattern_line   = r'(' + pattern_word + r')\s+(' + pattern_word + 
r')\s+(' + pattern_word + r')(\s+' \
+        + pattern_comment + r')?' + r'(\s+(' + pattern_word + r'))?' + 
r'(\s+(' + pattern_word + r'))?'
+
+    IMAGE_ROOTFS = d.getVar('IMAGE_ROOTFS')
+
+    for conffile in glob.glob(os.path.join(IMAGE_ROOTFS, 
'usr/lib/sysusers.d/*.conf')):
+        with open(conffile, 'r') as f:
+            for line in f:
+                line = line.strip()
+                if not len(line) or line[0] == '#': continue
+                ret = re.fullmatch(pattern_line, line.strip())
+                if not ret: continue
+                (stype, sname, sid, _, scomment, _, shomedir, _, 
sshell) = ret.groups()
+                if stype == 'u':
+                    useradd_command = ['useradd']
+                    if sid != '-':
+                        useradd_command.extend(['--uid', sid])
+                    if scomment and scomment != '-':
+                        useradd_command.extend(['--comment', scomment])
+                    if shomedir and shomedir != '-':
+                        useradd_command.extend(['--root', IMAGE_ROOTFS 
+ shomedir])
+                    else:
+                        useradd_command.extend(['--root', IMAGE_ROOTFS])
+                    if sshell and sshell != '-':
+                        useradd_command.extend(['--shell', sshell])
+                    else:
+                        useradd_command.extend(['--shell', 
'/sbin/nologin'])
+                    useradd_command.extend(['--system', sname])
+                    subprocess.run(useradd_command)
+                elif stype == 'g':
+                    groupadd_command = ['groupadd']
+                    if sid != '-':
+                        groupadd_command.extend(['--gid', sid])
+                    groupadd_command.extend(['--system', sname])
+                    subprocess.run(groupadd_command)
+                elif stype == 'm':
+                    subprocess.run(['groupadd', '--root', IMAGE_ROOTFS, 
'--system', sid])
+                    subprocess.run(['useradd', '--root', IMAGE_ROOTFS, 
'--shell', '/sbin/nologin', '--system', name, 'no-user-group'])
+                    subprocess.run(['usermod', '-a', '-G', sid, sname])
  }

  #



On 05/06/2023 17:55, Louis Rannou wrote:> Hello,
 >
 > I have found an issue in the rootfs routine. The
 > rootfs-postcommands.bbclass has a funtion systemd_create_users that
 > reads /etc/sysusers.d/*.conf files and parses lines as 'type name id
 > comment'.
 >
 > However, the sysusers.d manual says, those lines can be 'type name id
 > comment home_dir shell'. If a home directory of shell is defined, they
 > are considered as part of the comment, and we run incorrect commands
 > such as the one below :
 >
 > useradd --shell /sbin/nologin --uid 0 --comment "Super User" /root
 > --system root
 >
 > To fix that, we require a stronger parsing. Several options look
 > possible to me, but I am not sure which one is preferred.
 >
 > 1. sed with a regular expression that returns something that still needs
 > parsing
 > 2. awk with a step by step script that returns something that still
 > needs to be parsed
 > 3. use python and regexp module
 >
 > Also I don't know if the parsing should completely check the sysusers
 > syntax as said in the manual (first field is [urgm], second is
 > alphanum_-, etc.). In my opinion it should not as this will be made by
 > the useradd command.
 >
 > Do you think it worth to add some testing about that ? I am not sure how
 > to do that.
 >
 > Regards,
 > Louis Rannou

Re: [OE-core] [RFC] incorrect parsing of sysusers.d in rootfs generation

[Richard Purdie]

On Wed, 2023-06-07 at 17:16 +0200, Louis Rannou wrote:
> Hello again,
> 
> a python solution could be one below.
> 
> Also, I found that most of users/groups defined there are redundant as 
> they already exist (such as root). I guess they are defined from 
> base-passwd. I am not sure which recipe (base-passwd or systemd) should 
> have the precedence on this. If it's base-passwd, perhaps this 
> postcommand should check first if the user does already exist.

I'd say base-passwd should likely be the winner but we should probably
error if there is a conflict between what sysusers.d and base-passwd
are defining.

Cheers,

Richard

Re: [OE-core] [RFC] incorrect parsing of sysusers.d in rootfs generation

[Louis Rannou]

Hello,

On 08/06/2023 11:19, Richard Purdie wrote:
> On Wed, 2023-06-07 at 17:16 +0200, Louis Rannou wrote:
>> Hello again,
>>
>> a python solution could be one below.
>>
>> Also, I found that most of users/groups defined there are redundant as
>> they already exist (such as root). I guess they are defined from
>> base-passwd. I am not sure which recipe (base-passwd or systemd) should
>> have the precedence on this. If it's base-passwd, perhaps this
>> postcommand should check first if the user does already exist.
> 
> I'd say base-passwd should likely be the winner but we should probably
> error if there is a conflict between what sysusers.d and base-passwd
> are defining.
There will be some conflict. As an example the root home directory in 
sysusers.d is /root.

Some others users/groups defined in sysusers.d files are already created 
in recipes with the useradd class (such as systemd-resolved, 
systemd-network who are defined in the systemd recipe).

In the end, almost all users/groups in sysusers.d/* already exist. 
Perhaps it's a wrong way to parse this file to add users. In my opinion, 
we should only parse this file to check users/groups are already created 
and raise an error if one is missing.

Louis

Re: [OE-core] [RFC] incorrect parsing of sysusers.d in rootfs generation

[Richard Purdie]

On Thu, 2023-06-08 at 12:28 +0200, Louis Rannou wrote:
> Hello,
> 
> On 08/06/2023 11:19, Richard Purdie wrote:
> > On Wed, 2023-06-07 at 17:16 +0200, Louis Rannou wrote:
> > > Hello again,
> > > 
> > > a python solution could be one below.
> > > 
> > > Also, I found that most of users/groups defined there are redundant as
> > > they already exist (such as root). I guess they are defined from
> > > base-passwd. I am not sure which recipe (base-passwd or systemd) should
> > > have the precedence on this. If it's base-passwd, perhaps this
> > > postcommand should check first if the user does already exist.
> > 
> > I'd say base-passwd should likely be the winner but we should probably
> > error if there is a conflict between what sysusers.d and base-passwd
> > are defining.
> There will be some conflict. As an example the root home directory in 
> sysusers.d is /root.
> 
> Some others users/groups defined in sysusers.d files are already created 
> in recipes with the useradd class (such as systemd-resolved, 
> systemd-network who are defined in the systemd recipe).
> 
> In the end, almost all users/groups in sysusers.d/* already exist. 
> Perhaps it's a wrong way to parse this file to add users. In my opinion, 
> we should only parse this file to check users/groups are already created 
> and raise an error if one is missing.

Perhaps lets start there. The fact different bits of systemd are
configured with different home directories is a problem we should
really fix though and ultimately that probably should be an error too.

Cheers,

Richard

Re: [OE-core] [RFC] incorrect parsing of sysusers.d in rootfs generation

[Louis Rannou]

On 08/06/2023 12:36, Richard Purdie wrote:
> On Thu, 2023-06-08 at 12:28 +0200, Louis Rannou wrote:
>> Hello,
>>
>> On 08/06/2023 11:19, Richard Purdie wrote:
>>> On Wed, 2023-06-07 at 17:16 +0200, Louis Rannou wrote:
>>>> Hello again,
>>>>
>>>> a python solution could be one below.
>>>>
>>>> Also, I found that most of users/groups defined there are redundant as
>>>> they already exist (such as root). I guess they are defined from
>>>> base-passwd. I am not sure which recipe (base-passwd or systemd) should
>>>> have the precedence on this. If it's base-passwd, perhaps this
>>>> postcommand should check first if the user does already exist.
>>>
>>> I'd say base-passwd should likely be the winner but we should probably
>>> error if there is a conflict between what sysusers.d and base-passwd
>>> are defining.
>> There will be some conflict. As an example the root home directory in
>> sysusers.d is /root.
>>
>> Some others users/groups defined in sysusers.d files are already created
>> in recipes with the useradd class (such as systemd-resolved,
>> systemd-network who are defined in the systemd recipe).
>>
>> In the end, almost all users/groups in sysusers.d/* already exist.
>> Perhaps it's a wrong way to parse this file to add users. In my opinion,
>> we should only parse this file to check users/groups are already created
>> and raise an error if one is missing.
> 
> Perhaps lets start there. The fact different bits of systemd are
> configured with different home directories is a problem we should
> really fix though and ultimately that probably should be an error too.

Concerning the root home directory, I asked a question to systemd 
https://github.com/systemd/systemd/issues/27959 and LP answered he 
doesn't want to support a customizable root home directory.

Should we patch systemd to match our configuration, or should we change 
our configuration to match systemd's will...

Louis

Re: [OE-core] [RFC] incorrect parsing of sysusers.d in rootfs generation

[Louis Rannou]

Hello, below is a counter proposition including some work in the systemd 
recipe,



On 08/06/2023 13:56, Louis Rannou wrote:
> 
> 
> On 08/06/2023 12:36, Richard Purdie wrote:
>> On Thu, 2023-06-08 at 12:28 +0200, Louis Rannou wrote:
>>> Hello,
>>>
>>> On 08/06/2023 11:19, Richard Purdie wrote:
>>>> On Wed, 2023-06-07 at 17:16 +0200, Louis Rannou wrote:
>>>>> Hello again,
>>>>>
>>>>> a python solution could be one below.
>>>>>
>>>>> Also, I found that most of users/groups defined there are redundant as
>>>>> they already exist (such as root). I guess they are defined from
>>>>> base-passwd. I am not sure which recipe (base-passwd or systemd) 
>>>>> should
>>>>> have the precedence on this. If it's base-passwd, perhaps this
>>>>> postcommand should check first if the user does already exist.
>>>>
>>>> I'd say base-passwd should likely be the winner but we should probably
>>>> error if there is a conflict between what sysusers.d and base-passwd
>>>> are defining.
>>> There will be some conflict. As an example the root home directory in
>>> sysusers.d is /root.
>>>
>>> Some others users/groups defined in sysusers.d files are already created
>>> in recipes with the useradd class (such as systemd-resolved,
>>> systemd-network who are defined in the systemd recipe).
>>>
>>> In the end, almost all users/groups in sysusers.d/* already exist.
>>> Perhaps it's a wrong way to parse this file to add users. In my opinion,
>>> we should only parse this file to check users/groups are already created
>>> and raise an error if one is missing.
>>
>> Perhaps lets start there. The fact different bits of systemd are
>> configured with different home directories is a problem we should
>> really fix though and ultimately that probably should be an error too.
> 
> Concerning the root home directory, I asked a question to systemd 
> https://github.com/systemd/systemd/issues/27959 and LP answered he 
> doesn't want to support a customizable root home directory.
> 
> Should we patch systemd to match our configuration, or should we change 
> our configuration to match systemd's will...
> 
> Louis

The more I look at this and at #9497 (which gives the reason of this 
command), the more I think the problem was not correctly handled.

sysusers.d is made to create missing users at runtime with the 
systemd-sysusers service.

First of all, that means there is a wrong runtime configuration as long 
as /etc/{passwd, gpasswd, shadow, gshadow} do not match sysusers.d/*.conf

I think it is fine to check during the build that sysusers.d/*.conf do 
match configuration for users/groups created during build, but we should 
not create users and groups at this stage because missing users will be 
created at runtime.

I think sysusers/*.conf should be provided with the systemd recipe so 
they match our configuration (such as ROOT_HOME for the root directory).

If you are ok with this, I may suggest a patch.

Regards,
Louis