diff mbox series

[1/3] support/scripts/mkusers: allow option for system uid/gid

Message ID 20200113153516.486106-1-nolange79@gmail.com
State New
Headers show
Series [1/3] support/scripts/mkusers: allow option for system uid/gid | expand

Commit Message

Norbert Lange Jan. 13, 2020, 3:35 p.m. UTC
Extend the mkusers script to allow -2 for uid/gid.
This value will take an identifier from the system range.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 support/scripts/mkusers | 49 +++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 14 deletions(-)

Comments

Thomas Petazzoni Sept. 15, 2020, 8:47 p.m. UTC | #1
Hello Norbert,

On Mon, 13 Jan 2020 16:35:13 +0100
Norbert Lange <nolange79@gmail.com> wrote:

> Extend the mkusers script to allow -2 for uid/gid.
> This value will take an identifier from the system range.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>

Sorry for the long delay in getting back to you. We had an earlier
proposal from Stephan Henningsen doing pretty much the same:

  https://patchwork.ozlabs.org/project/buildroot/patch/20191023211313.6758-1-stephan+buildroot@asklandd.dk/

The argument of Stephan was pretty much beauty/consistency with what
"most systems do" without much other arguments.

However, based on your PATCH 2/3 and a reading of
https://systemd.io/UIDS-GIDS/ it seems like systemd somehow cares about
this system vs. normal user difference.

Could you give some details about the *why* you did this change?
Indeed, your commit log doesn't explain anything about the *why*.

Also, could you compare your changes to mkusers with the ones proposed
by Stephan? The ones proposed by Stephan looked quite a bit more
complicated.

Another (minor) question is: if we're going to go to this route of
separating system and normal users, wouldn't it make sense to have -1
identify system users, and -2 identify normal users? Indeed the vast
majority (all?) Buildroot packages probably want to create system
users, and they already use -1.

Best regards,

Thomas
Norbert Lange Sept. 15, 2020, 9:29 p.m. UTC | #2
Thomas Petazzoni <thomas.petazzoni@bootlin.com> schrieb am Di., 15. Sep.
2020, 22:47:

> Hello Norbert,
>
> On Mon, 13 Jan 2020 16:35:13 +0100
> Norbert Lange <nolange79@gmail.com> wrote:
>
> > Extend the mkusers script to allow -2 for uid/gid.
> > This value will take an identifier from the system range.
> >
> > Signed-off-by: Norbert Lange <nolange79@gmail.com>
>
> Sorry for the long delay in getting back to you. We had an earlier
> proposal from Stephan Henningsen doing pretty much the same:
>
>
> https://patchwork.ozlabs.org/project/buildroot/patch/20191023211313.6758-1-stephan+buildroot@asklandd.dk/
>
> The argument of Stephan was pretty much beauty/consistency with what
> "most systems do" without much other arguments.
>
> However, based on your PATCH 2/3 and a reading of
> https://systemd.io/UIDS-GIDS/ it seems like systemd somehow cares about
> this system vs. normal user difference.
>
> Could you give some details about the *why* you did this change?
> Indeed, your commit log doesn't explain anything about the *why*.
>
> Also, could you compare your changes to mkusers with the ones proposed
> by Stephan? The ones proposed by Stephan looked quite a bit more
> complicated.
>
> Another (minor) question is: if we're going to go to this route of
> separating system and normal users, wouldn't it make sense to have -1
> identify system users, and -2 identify normal users? Indeed the vast
> majority (all?) Buildroot packages probably want to create system
> users, and they already use -1.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com


Yeah, this was touched upon (both points), see [1].

System users exist as concept on all distros an observable effects are for
ex journald spawning a separate logging stream.

So no, it's not just cosmetics, and yes I'd make system user the default.

Norbert

[1] http://lists.busybox.net/pipermail/buildroot/2020-February/273558.html

>
>
Jérémy ROSEN Sept. 16, 2020, 6:41 a.m. UTC | #3
I'll give a bit more detail about the way journald handles system/user UID.
there might be other places where systemd treats them differently, but
that's the only
one I know from memory.

journald collects all logs on the system
* from the daemons running around
* from the kernel/audit system
* from containers
* from user sessions.

When a user logs in, a "per user" instance of systemd is spawned that
can starts daemons for that user (ssh-agent, pulseaudio, colord...
daemons that make sense at the user level but not at the system level)

To ease the handling of permissions and reading those files, journald does
not store all logs in a single file, but in one file per user. Journald uses
ACL to allow each user to have access to his logs through normal unix
permissions instead of relying on some sort of SUID mechanism.

However, it would be a bad idea to have a separate journal file for system
users, since system users are part of the system and the log they produce
are really only for the administrator to see. So those logs are stored with
the
system logs in the machine's main log file.

The separation between the two types of users uses the UID1000 split

A quick grep in systemd yields a couple of other usages
* systemd-coredumps will tweak access right to allow non-system users to
  read the core-dumps they generate
* When closing a session, logind may clean up IPC for non-system user.
  The explanation is a bit complex so i'll just copy/paste the comment in
the code

        /* Clean SysV + POSIX IPC objects, but only if this is not a system
user. Background: in many setups cronjobs
         * are run in full PAM and thus logind sessions, even if the code
run doesn't belong to actual users but to
         * system components. Since enable RemoveIPC= globally for all
users, we need to be a bit careful with such
         * cases, as we shouldn't accidentally remove a system service's
IPC objects while it is running, just because
         * a cronjob running as the same user just finished. Hence: exclude
system users generally from IPC clean-up,
         * and do it only for normal users. */

* there is a unit condition calle ConditionUser= (and AssertUser=) that
  allow to limit a unit file to only be allowed for a certain user. This
condition
  can take a user name, a UID or the magic value "@system" to be allowed
  to any system user


Regards
Jeremy
Thomas Petazzoni Sept. 16, 2020, 6:53 a.m. UTC | #4
Hello,

On Tue, 15 Sep 2020 23:29:24 +0200
Norbert Lange <nolange79@gmail.com> wrote:

> Yeah, this was touched upon (both points), see [1].
> 
> System users exist as concept on all distros an observable effects are for
> ex journald spawning a separate logging stream.

Right, thanks for the additional feedback.

> So no, it's not just cosmetics, and yes I'd make system user the default.

Agreed.

Did you compare your implementation with the one proposed by Stephan
earlier ?

Best regards,

Thomas
diff mbox series

Patch

diff --git a/support/scripts/mkusers b/support/scripts/mkusers
index d00ba33823..1bf1336e48 100755
--- a/support/scripts/mkusers
+++ b/support/scripts/mkusers
@@ -8,6 +8,12 @@  MIN_UID=1000
 MAX_UID=1999
 MIN_GID=1000
 MAX_GID=1999
+# use names from /etc/adduser.conf
+FIRST_SYSTEM_UID=100
+LAST_SYSTEM_UID=999
+FIRST_SYSTEM_GID=100
+LAST_SYSTEM_GID=999
+
 # No more is configurable below this point
 #----------------------------------------------------------------------------
 
@@ -136,9 +142,9 @@  check_user_validity() {
         fail "invalid username '%s\n'" "${username}"
     fi
 
-    if [ ${gid} -lt -1 -o ${gid} -eq 0 ]; then
+    if [ ${gid} -lt -2 -o ${gid} -eq 0 ]; then
         fail "invalid gid '%d' for '%s'\n" ${gid} "${username}"
-    elif [ ${gid} -ne -1 ]; then
+    elif [ ${gid} -gt -1 ]; then
         # check the gid is not already used for another group
         if [ -n "${_group}" -a "${_group}" != "${group}" ]; then
             fail "gid '%d' for '%s' is already used by group '%s'\n" \
@@ -162,9 +168,9 @@  check_user_validity() {
         fi
     fi
 
-    if [ ${uid} -lt -1 -o ${uid} -eq 0 ]; then
+    if [ ${uid} -lt -2 -o ${uid} -eq 0 ]; then
         fail "invalid uid '%d' for '%s'\n" ${uid} "${username}"
-    elif [ ${uid} -ne -1 ]; then
+    elif [ ${uid} -gt -1 ]; then
         # check the uid is not already used for another user
         if [ -n "${_username}" -a "${_username}" != "${username}" ]; then
             fail "uid '%d' for '%s' already used by user '%s'\n" \
@@ -198,16 +204,18 @@  check_user_validity() {
 #   - not already used by a group
 generate_gid() {
     local group="${1}"
+    local mingid="${2:-$MIN_UID}"
+    local maxgid="${3:-$MAX_UID}"
     local gid
 
     gid="$( get_gid "${group}" )"
     if [ -z "${gid}" ]; then
-        for(( gid=MIN_GID; gid<=MAX_GID; gid++ )); do
+        for(( gid=mingid; gid<=maxgid; gid++ )); do
             if [ -z "$( get_group "${gid}" )" ]; then
                 break
             fi
         done
-        if [ ${gid} -gt ${MAX_GID} ]; then
+        if [ ${gid} -gt ${maxgid} ]; then
             fail "can not allocate a GID for group '%s'\n" "${group}"
         fi
     fi
@@ -222,8 +230,13 @@  add_one_group() {
     local members
 
     # Generate a new GID if needed
-    if [ ${gid} -eq -1 ]; then
-        gid="$( generate_gid "${group}" )"
+    if [ ${gid} -le -1 ]; then
+        if [ ${gid} -eq -1 ]; then
+            gid="$( generate_gid "${group}" )"
+        else
+            gid="$( generate_gid "${group}" $FIRST_SYSTEM_GID $LAST_SYSTEM_GID )"
+
+        fi
     fi
 
     members=$(get_members "$group")
@@ -247,16 +260,19 @@  add_one_group() {
 #   - not already used by a user
 generate_uid() {
     local username="${1}"
+    local minuid="${2:-$MIN_UID}"
+    local maxuid="${3:-$MAX_UID}"
+
     local uid
 
     uid="$( get_uid "${username}" )"
     if [ -z "${uid}" ]; then
-        for(( uid=MIN_UID; uid<=MAX_UID; uid++ )); do
+        for(( uid=minuid; uid<=maxuid; uid++ )); do
             if [ -z "$( get_username "${uid}" )" ]; then
                 break
             fi
         done
-        if [ ${uid} -gt ${MAX_UID} ]; then
+        if [ ${uid} -gt ${maxuid} ]; then
             fail "can not allocate a UID for user '%s'\n" "${username}"
         fi
     fi
@@ -307,8 +323,13 @@  add_one_user() {
     check_user_validity "${username}" "${uid}" "${group}" "${gid}"
 
     # Generate a new UID if needed
-    if [ ${uid} -eq -1 ]; then
-        uid="$( generate_uid "${username}" )"
+    if [ ${uid} -le -1 ]; then
+        if [ ${uid} -eq -1 ]; then
+            uid="$( generate_uid "${username}" )"
+        else
+            uid="$( generate_uid "${username}" $FIRST_SYSTEM_UID $LAST_SYSTEM_UID )"
+
+        fi
     fi
 
     # Remove any previous instance of this user
@@ -399,7 +420,7 @@  main() {
     # Then, create all the main groups which gid *is* automatic
     for line in "${ENTRIES[@]}"; do
         read username uid group gid passwd home shell groups comment <<<"${line}"
-        [ ${gid} -eq -1 ] || continue    # Non-automatic gid
+        [ ${gid} -le -1 ] || continue    # Non-automatic gid
         add_one_group "${group}" "${gid}"
     done
 
@@ -433,7 +454,7 @@  main() {
     for line in "${ENTRIES[@]}"; do
         read username uid group gid passwd home shell groups comment <<<"${line}"
         [ "${username}" != "-" ] || continue # Magic string to skip user creation
-        [ ${uid} -eq -1        ] || continue # Non-automatic uid
+        [ ${uid} -le -1        ] || continue # Non-automatic uid
         add_one_user "${username}" "${uid}" "${group}" "${gid}" "${passwd}" \
                      "${home}" "${shell}" "${groups}" "${comment}"
     done