systemd: Use systemd-sysusers to create cockpit-wsinstance user

[travier]

Use systemd-sysusers to create users & groups

Add a templated sysusers config file and use it in the RPM spec to
create users.

Replace the current Arch Linux specific config files.

Note that the (already resolved) cockpit-sysusers.conf file is needed
for the RPM specfile as a source as the macro can not be expanded using
the content from the source archive as it is not extracted during the
stage where RPM macro expansion happens. We thus need to keep a copy in
the dist-git repo.

See: https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_dynamic_allocation

---

Todo:

• broken out commit with fix: build: Remove cockpit(-ws-instance) user/group config option #20402
• broken out commit with fix: Rename cockpit-tempfiles.conf to cockpit-ws.conf #20403
• Built on top of tools: Use DynamicUser for cockpit.service #20425, rebase after landing
• get rid of dist-git fie duplication

@travier 's original proposal: travier@c585677 (doesn't build, dist-git hack)

[travier]

Ideally those files would be both named cockpit.conf, each in their own respective folders, but that was too much autoconf work for this initial PR. Open to suggestions.

[martinpitt]

@travier I didn't review yet, but we already made such an attempt two years ago in #18112. It unfortunately got stuck on its precondition #16808. But personally I'd be happy to see some progress here even if it's not perfect.

[travier]

Note that this still uses "static" users with "dynamic at install time" allocation. So not the systemd DynamicUsers support.

I don't have the full context so I don't understand why it's related to socket activation.

[travier]

I don't understand the packit build failures :/

2024-04-24 11:19:08.887 logging.py        INFO   make[1]: *** No rule to make target 'src/systemd/cockpit-tmpfiles.conf.in', needed by 'distdir'.  Stop.
2024-04-24 11:19:08.888 logging.py        INFO   make[1]: *** Waiting for unfinished jobs....
2024-04-24 11:19:08.892 logging.py        INFO   make: *** [Makefile:8081: dist] Error 2
2024-04-24 11:19:08.893 commands.py       ERROR  Command 'tools/make-dist' failed.
2024-04-24 11:19:08.893 commands.py       DEBUG  Command stderr:   AUTOGEN  configure, Makefile.in

[martinpitt]

Thanks! Reviewed, and now I remember again why our initial approach got stuck, and why we didn't move forward with sysusers. Let's discuss this a bit.

@allisonkarlitskaya this probably interests you as well.

[martinpitt]

Having to duplicate and keep this in sync with upstream is really ugly, let's avoid that. The whole point of moving to sysusers is to drop downstream packaging stuff IMHO, otherwise we could just leave it as-is.

[martinpitt]

I wonder, how would that even work with this approach? Even if the sysusers file is taken from dist-git instead of upstream, it wuold still be shipped by the rpm and thus isn't yet available in %pre?

I suppose this will do that evil thing where it translates a sysusers file into useradd commands? I just found this:

❱❱❱ rpm -q --scripts  pipewire
preinstall scriptlet (using /bin/sh):

# generated from pipewire.sysusers
getent group 'pipewire' >/dev/null || groupadd -r 'pipewire' || :
getent passwd 'pipewire' >/dev/null || \
    useradd -r -g 'pipewire' -d '/run/pipewire' -s '/usr/sbin/nologin' -c 'PipeWire System Daemon' 'pipewire' || :

This is all new to me, so sorry for rambling.. But this looks hackish. I suppose it allows rpms to ship files which are owned by a sysusers owner. The only file in cockpit-ws which needs that is

%attr(4750, root, cockpit-wsinstance) %{_libexecdir}/cockpit-session

I think all this was the reason why in our initial approach we wanted to first get rid of that suid root binary (see PR #16808), and then there wouldn't be any need for static sysusers any more.

[travier]

Unfortunately I don't think it's possible to drop this copy due to how macro expansions work and until we fully have Fedora switch to using systemd sysusers (it's only using the format right now, not the command).

See also https://src.fedoraproject.org/rpms/systemd/pull-request/121 that might simplify things a bit.

[martinpitt]

TBH I consider this is a blocker. This makes the packaging structure more complicated and brittle. So for me, it's either "chmod cockpit-session in %post as we do in Debian/Arch" (see below), or block this on getting rid of the suid root binary.

[allisonkarlitskaya]

Thanks! Reviewed, and now I remember again why our initial approach got stuck, and why we didn't move forward with sysusers. Let's discuss this a bit.

@allisonkarlitskaya this probably interests you as well.

I think we didn't do this in the past because we hoped to remove the users entirely.

Since that didn't (can't) happen, we're stuck requiring these users to exist. I have nothing against moving to using systemd-sysusers for it (and even prefer it).

[martinpitt]

@travier Thanks! I like the new commits, and as the main "use sysusers" thing needs more investigation, we can land them in a separate PR now. "Remove cockpit(-ws-instance) user/group config option" should come first, and then the renaming of src/systemd/cockpit-tempfiles.conf.in . However, not to -tmpfiles.conf please, as it ends up in /usr/lib/tmpfiles.d/ and there it should be named cockpit-ws.conf. I propose to move them to src/systemd/tmpfiles.d/ in the source tree, which avoids having to rename them.

[martinpitt]

@travier Sorry, forgot: do you want to send a new PR for this, or shall I take this over?

[martinpitt]

@travier I read https://rpm-software-management.github.io/rpm/manual/users_and_groups.html and rpm 4.19 makes this much easier -- in essence, just shipping a sysusers.d/ file should be enough (plus the %attr()).

Unfortunately RHEL 9 still has rpm 4.16, but for that I'd be ok with duplicating the sysusers.d line with %add_sysuser in the spec if we conditionalize it with %if 0%{?rhel} == 9. That should avoid the dist-git complication and the hack has an end-of-life. What do you think?

[travier]

@travier Sorry, forgot: do you want to send a new PR for this, or shall I take this over?

I'll happily let you take over if you don't mind as it's autoconf changes that I'm not familiar with and just hack around.

I propose to move them to src/systemd/tmpfiles.d/ in the source tree, which avoids having to rename them.

Sound like a good idea.

Unfortunately RHEL 9 still has rpm 4.16, but for that I'd be ok with duplicating the sysusers.d line with %add_sysuser in the spec if we conditionalize it with %if 0%{?rhel} == 9. That should avoid the dist-git complication and the hack has an end-of-life. What do you think?

Not sure I understand that. I would keep the existing useradd calls for RHEL 9.

[travier]

I've not addressed all comments yet, but this is rebased on top of #20402 & #20403

[martinpitt]

Thanks! Still fails to build:

make[1]: *** No rule to make target 'src/systemd/sysusers.d/cockpit-ws.conf', needed by 'all-am'.  Stop.

I was going to take a look at this soon. Let's utilize rpm 4.19 where available, and add a hack for RHEL 9 only, as discussed above. Of course I'll appreciate if you want to work on this further 😄

[martinpitt]

I locally fixed the autoconfery and package/image build.

I tried https://rpm-software-management.github.io/rpm/manual/users_and_groups.html, i.e. dropped all the %pre and spec hacks . It does create the "magic" provides:

# rpm -q --provides cockpit-ws
cockpit-ws = 316.dev27+g1dc9d3b8f-1.fc40
cockpit-ws(x86-64) = 316.dev27+g1dc9d3b8f-1.fc40
config(cockpit-ws) = 316.dev27+g1dc9d3b8f-1.fc40
group(cockpit-ws)
group(cockpit-wsinstance)
user(cockpit-ws) = dSBjb2NrcGl0LXdzIC0gIlVzZXIgZm9yIGNvY2twaXQgd2ViIHNlcnZpY2UiIC0A
user(cockpit-wsinstance) = dSBjb2NrcGl0LXdzaW5zdGFuY2UgLSAiVXNlciBmb3IgY29ja3BpdC13cyBpbnN0YW5jZXMiIC0A

# echo dSBjb2NrcGl0LXdzIC0gIlVzZXIgZm9yIGNvY2twaXQgd2ViIHNlcnZpY2UiIC0A | base64 -d
u cockpit-ws - "User for cockpit web service" -

but on install it does the wrong order, i.e. unpacks the files before creating the users:

# rpm -i --verbose /var/tmp/build/cockpit-ws-316.dev27+g1dc9d3b8f-1.fc40.x86_64.rpm 
Verifying packages...
Preparing packages...
cockpit-ws-316.dev27+g1dc9d3b8f-1.fc40.x86_64
warning: group cockpit-wsinstance does not exist - using root
Creating group 'cockpit-ws' with GID 979.
Creating user 'cockpit-ws' (User for cockpit web service) with UID 979 and GID 979.
Creating group 'cockpit-wsinstance' with GID 978.
Creating user 'cockpit-wsinstance' (User for cockpit-ws instances) with UID 978 and GID 978.

and after installation, /usr/libexec/cockpit-session has the wrong ownership (group root). So we require some extra hack for this. Why is this so complicated!?

[travier]

Of course I'll appreciate if you want to work on this further 😄

Up to you :). I have some spare time from time to time to push this but feel free to push any changes here directly if you get to it faster than me :).

[travier]

The Debian side also still needs cleaning up -- there's dh_installsysusers which is supposed to take care of it, and should replace the two adduser calls in tools/debian/cockpit-ws.postinst.

I'm definitely not knowledgeable regarding Debian packaging however so I'll let that to you.

[travier]

but on install it does the wrong order, i.e. unpacks the files before creating the users:

Yeah, that's the whole reason we have this duplication right now :/

[martinpitt]

I pushed what I have so far, as I'm EOD. This is the "clean" approach that ideally should work, but doesn't. rpm build works, but the tests will/should fail as cockpit-session has the wrong permissions. I captured the commit SHA of your original proposal in the description, so that we can always go back and compare. I'll continue this tomorrow.

[martinpitt]

I filed rpm-software-management/rpm#3073 about this. Now pondering a workaround -- we could just leave the %pre script with useradd, also for RHEL 9 (which only has rpm 4.16). This %sysusers_create_compat is too ugly really..

But I'll try to eliminate at least half of the problem -- we only need a persistent user for cockpit-wsinstance, we should be able to replace cockpit-ws with DynamicUser=. See PR #20425

[martinpitt]

@travier I couldn't resist, and I have something that works now. This now builds on top of #20425. The second-to-last commit is the "conservative" approach which uses sysusers on Debian/Arch plus the original manual useradd on rpm. The last commit ("FIXUP") drops that and replaces it with a tmpfiles stanza to set up the permissions. This has been used on Arch for a long time, so this upstreams the concept. That is actually quite nice, sudo make install from upstream finally does the right thing.

I tested this locally on Arch, Debian, and Fedora 40, both an upgrade and from a squeaky clean install, and it works nicely. What do you think?

[travier]

I filed rpm-software-management/rpm#3073 about this. Now pondering a workaround -- we could just leave the %pre script with useradd, also for RHEL 9 (which only has rpm 4.16). This %sysusers_create_compat is too ugly really..

That's the option the libvirt maintainers took. Keep the useradd calls in the %pre section and install the sysusers config so that the user definition is clearly tracked that way. This is the "fix" for the issue that lead me here in the first place: in some case on Fedora CoreOS, we need sysusers files to have users/groups created properly.

We can drop the %pre code once Fedora completes the move to sysusers configs.

[martinpitt]

@travier Yeah, I'm not sure if the tmpfiles approach would violate some Fedora packaging standards. At this point this PR is all just draft/RFC. Thanks for your insights!

[martinpitt]

I wanted to rebase this, but all of a sudden I'm not allowed to push into your branch any more:

To github.com:travier/cockpit.git
 ! [remote rejected]     HEAD -> main-sysuserconfig (permission denied)
error: failed to push some refs to 'github.com:travier/cockpit.git'

The ":heavy_check_mark: Maintainers are allowed to edit this pull request. " flag is still set, so this is some GitHub bug.

So I cloned this as #20447 . @travier if you are, feel free to sync your branch with mine, otherwise we'll close this one and continue there.

[martinpitt]

#20447 landed (although without the tmpfiles.d fix attempt), so this is obsolete. Thanks @travier !