xbps-triggers:system-accounts: use grep to check for user/group existent

[sgn]

In system-accounts triggers, we're using getent(1) to check whether
the username or group in question is existed before doing the heavy
lifting.

However, getent(1) will check the database in host system instead of our
rootfs, and by PATH manipulation logic, we prefer to use usr/bin/getent
inside our rootfs instead of host getent(1).

This is usually not a problem since we mostly run xbps-triggers in
a real system instead of running from foreign system.

Except for base-files packages, which used to not have group kvm
pre-allocated. Thus, requires running this trigger, and lead to all sort
of problems:

• If host system is a musl-based linux system, with gcompat installed,
  and we're bootstrapping a glibc one, getent(1) will be executable,
  however, when getent(1) attempt to dlopen(3) other libraries,
  it'll run into failure.
• If host system doesn't have kvm group pre-allocated (bootstrapping
  from foreign distro), we attempt to run groupadd(1) on such system,
  thus failing with EPERM.

If we run into one of those cases, xbps-reconfigure(1) will stop
configuring base-files, not running base-files' INSTALL and leave
the system in half-baked state, without some requires files and
directories.

Switch to grep(1) to check for username and group existence,
since passwd(5) and group(5) is well-documented.

[ericonr]

Remember to change the version / revbump the package.

[sgn]

On 2020-09-07 18:29:26-0700, Érico Nogueira Rolim ***@***.***> wrote: Remember to change the version / revbump the package.

Yup, I forgot to revbump (since no behavior changes). Also, would wait for other's opinions.

[sgn]

@void-linux/pkg-committers

[ahesford]

This is probably a fine replacement, depending on how much we care about sanity checks on values on group and user definitions.

As a totally contrived example, suppose somebody puts an invalid value in $system_groups, say wheel:x:123. This will be split into a group wheel:x and a gid 123. Currently, when getent group wheel:x is run, the output will be empty and the return value nonzero because the group does not (and can never) exist. The trigger will try to create the group, but groupadd will fail because wheel:x is not a valid name, causing the trigger to complain during install.

The grep replacement will instead test a match on wheel:x: in etc/group, which (in this case) should match the existing group definition for wheel, meaning the trigger will not report an error. (It will also not report that a group was created, but that's much harder to notice than a failure message.)

In the end, it seems like the system state would be the same either way, because either a valid group is created or already exists; an invalid group fails to match and the trigger tries unsuccessfully to create it; or an invalid group falsely matches and the trigger doesn't try to run a groupadd command that would have failed anyway.

If we care about the failure always appearing when a group should be created but isn't, you might have to cut the fields of the files or pull awk into the picture.

[sgn]

As a totally contrived example, suppose somebody puts an invalid value in $system_groups, say wheel:x:123. This will be split into a group wheel:x and a gid 123.

Minor correction:
The current code will split into:

_grname=wheel:x
_gid=x:123

Currently, when getent group wheel:x is run, the output will be empty and the return value nonzero because the group does not (and can never) exist. The trigger will try to create the group, but groupadd will fail because wheel:x is not a valid name, causing the trigger to complain during install.

The grep replacement will instead test a match on wheel:x: in etc/group, which (in this case) should match the existing group definition for wheel, meaning the trigger will not report an error. (It will also not report that a group was created, but that's much harder to notice than a failure message.)

I believe we can do the sanity check for argument by:

case "$1" in
*:*:*)  echo "Invalid group specification" >&2; exit 1;;
esac

In the end, it seems like the system state would be the same either way, because either a valid group is created or already exists; an invalid group fails to match and the trigger tries unsuccessfully to create it; or an invalid group falsely matches and the trigger doesn't try to run a groupadd command that would have failed anyway.

If we care about the failure always appearing when a group should be created but isn't, you might have to cut the fields of the files or pull awk into the picture.

[ahesford]

Looks good to me

[the-maldridge]

Just to point out that this can break in really hard to diagnose ways on systems that are configured with NSS.

[ericonr]

Could we instead make this trigger run only inside chroot (for the masterdir case, as well as installing new systems) and in a real system? Add some sort of early failure if ROOTDIR != /

[sgn]

On 2020-09-09 23:54:26-0700, Michael Aldridge ***@***.***> wrote: Just to point out that this can break in really hard to diagnose ways on systems that are configured with NSS.

Hm, I've never used such system. I'm in a fence, now. I'm not sure what to do now. - Use both grep and getent to check - ignore xbps-trigger's system-accounts outside of chroot I'm really to prefer the second approach since it's the sanner approach.

[ahesford]

The more I look at this, the more I see breakage when populating an alternate root. Won't useradd, usermod and groupadd always act on the system root? These commands provide a --root option that seems to redirect their actions in the way we want, but we aren't using that option now. It's also not clear from their man pages how well these commands get along with directory services.

Ignoring system-accounts unless $ROOTDIR = / like @ericonr suggests is a reasonable way to avoid the worst of these issues. If you go that route, the hook should at least print all of the users and groups that should exist (along with ID numbers, if provided) for the package to function as expected.

If we really want to do this the "right" way, it's not obvious to me that the existence tests add value. For groups, just groupadd --root ... and catch the return code to know the difference between a real failure and an attempt to create a group that already exists. For users, just useradd --root ... and, if the return code indicates that the username already exists, try usermod --root ... instead.

[sgn]

On 2020-09-10 06:48:52-0700, "Andrew J. Hesford" ***@***.***> wrote: The more I look at this, the more I see breakage when populating an alternate root. Won't `useradd`, `usermod` and `groupadd` always act on the system root? These commands provide a `--root` option that seems to redirect their actions in the way we want, but we aren't using that option now. It's also not clear from their man pages how well these commands get along with directory services.

Yes, it's broken.

Ignoring `system-accounts` unless `$ROOTDIR = /` like @ericonr suggests is a reasonable way to avoid the worst of these issues. If you go that route, the hook should at least print all of the users and groups that should exist (along with ID numbers, if provided) for the package to function as expected.

Agree.

If we really want to do this the "right" way, it's not obvious to me that the existence tests add value. For groups, just `groupadd --root ...` and catch the return code to know the difference between a real failure and an attempt to create a group that already exists. For users, just `useradd --root ...` and, if the return code indicates that the username already exists, try `usermod --root ...` instead.

Hm, I think we need `groupadd --prefix` and `useradd --prefix` instead.

[ericonr]

Wouldn't all of this still be broken if messing with a device that uses NSS? afaik that would require a proper service and all

[sgn]

Yeah, this PR is fundamentally broken, closing.