Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

systemd: Use sysusers.d to create ws users #18112

Closed
wants to merge 3 commits into from

Conversation

martinpitt
Copy link
Member

@martinpitt martinpitt commented Jan 3, 2023

Move to using systemd's sysusers declarative files [1] for creating our
system users/groups. Arch already does that, Fedora moved to it since
Fedora 32 [2], and Debian supports it as well [3].

In debian/cockpit-ws.postinst, move the #DEBHELPER# block above the
statoverride, as the former now generates the user, and the latter needs
it.

Unfortunately Fedora/rpm's %attr does not really work with sysusers
files shipped upstream yet. The conf files are not installed yet during
%pre, but creating the users in %post is too late for the file
unpack phase, so cockpit-session would get the wrong permissions. Thus
duplicate the two sysusers config lines verbatim in %pre, which is at
least marginally better than calling useradd etc. programmatically.

Extend TestConnection.testWsPackage to remove the system users, reboot,
and validate that cockpit still works. This ensures correct sysusers.d
packaging across all distributions, as our normal CI images already have
the system users.

Fixes #15027

[1] https://www.freedesktop.org/software/systemd/man/sysusers.d.html
[2] https://fedoraproject.org/wiki/Changes/Adopting_sysusers.d_format
[3] https://manpages.debian.org/dh_installsysusers


We originally tried DynamicUser= in #16811, but that has been blocked for too long. IMHO this is a nice improvement already, and does not block moving to DynamicUser= in the future.

@martinpitt
Copy link
Member Author

The arch failure is straightforward, already fixed/verified locally.

The two coreos failures are more serious: "error: While applying overrides for pkg cockpit-ws: Could not find group 'cockpit-wsinstance' in group file". It seems rpm-ostree install does not properly process the sysusers.d rpm script.

I moved it from %post to %pre, and that works. It's the right thing to do anyway, %post was an oversight. Yay tests!

This should also fix all the TF failures which use a fresh install instead of an upgrade. Nice to cover that case cleanly as well!

@martinpitt martinpitt temporarily deployed to cockpit-dist January 3, 2023 10:23 — with GitHub Actions Inactive
@martinpitt martinpitt marked this pull request as ready for review January 3, 2023 11:00
@martinpitt martinpitt marked this pull request as draft January 3, 2023 11:59
@martinpitt
Copy link
Member Author

martinpitt commented Jan 3, 2023

TF failures are still significant:

  Running scriptlet: cockpit-ws-282.1-1.20230103102400455339.pr18112.15   87/95 
Failed to open 'cockpit.conf', ignoring: No such file or directory
  Installing       : cockpit-ws-282.1-1.20230103102400455339.pr18112.15   87/95 
warning: group cockpit-wsinstance does not exist - using root

That's because %pre is actually too early -- of course the file does not exist yet. But later on it does get created:

Creating group 'cockpit-ws' with GID 990.
Creating user 'cockpit-ws' (User for cockpit web service) with UID 990 and GID 990.
Creating group 'cockpit-wsinstance' with GID 989.
Creating user 'cockpit-wsinstance' (User for cockpit-ws instances) with UID 989 and GID 989.

Investigating in a prepared fedora-37 VM:

rpm -e cockpit-ws cockpit; userdel cockpit-ws; userdel cockpit-wsinstance
rpm -i /var/tmp/build/cockpit-ws-*.rpm
# Failed to open 'cockpit.conf', ignoring: No such file or directory
# warning: group cockpit-wsinstance does not exist - using root
# Creating group 'cockpit-ws' with GID 982.
# Creating user 'cockpit-ws' (User for cockpit web service) with UID 982 and GID 982.
# Creating group 'cockpit-wsinstance' with GID 981.
# Creating user 'cockpit-wsinstance' (User for cockpit-ws instances) with UID 981 and GID 981.

But:

ls -l /usr/libexec/cockpit-session
# -rwsr-x---. 1 root root 58168 Jan  3 12:06 /usr/libexec/cockpit-session

This failure is an independent flake, I'll add a piggy-back commit here.

@martinpitt martinpitt temporarily deployed to cockpit-dist January 3, 2023 12:42 — with GitHub Actions Inactive
@martinpitt martinpitt marked this pull request as ready for review January 3, 2023 13:45
@martinpitt martinpitt requested review from allisonkarlitskaya and jelly and removed request for allisonkarlitskaya January 3, 2023 13:45
@allisonkarlitskaya
Copy link
Member

See also #16811

@martinpitt
Copy link
Member Author

@allisonkarlitskaya : Yes, I'm aware of this, I mentioned it in the description already. But I don't think this will make progress anytime soon.

Copy link
Member

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good assuming everything passes. One comment for your consideration.

@@ -48,6 +51,7 @@ src/systemd/%: src/systemd/%.in
systemdgenerated = \
$(nodist_systemdunit_DATA) \
$(nodist_tempconf_DATA) \
$(nodist_sysusers_DATA) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd lean towards doing that in configure.ac. The only reason we need to do these ones here is to get the recursive path expansion (see the comment just above), but that's not an issue for these files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually vaguely aware of this, but I found it nice to keep all the systemd bits together in one file. I don't have a strong opinion about it, though.

@allisonkarlitskaya
Copy link
Member

@allisonkarlitskaya : Yes, I'm aware of this, I mentioned it in the description already. But I don't think this will make progress anytime soon.

I totally agree. We're basically waiting on support in systemd which nobody seems to be interested in working on. I have no intention to block this PR in the meantime.

@martinpitt
Copy link
Member Author

martinpitt commented Jan 3, 2023

Hmm, the two IPA tests fail a little too often for my taste on R8.8 and C8S. A lot of it is noise due to the "3x affected" retries, but apparently not these. They reproduce well locally, I'll investigate them tomorrow.

Curiously these tests aren't even affected by the sysusers changes -- the packages are upgraded, and it's still exactly the same useradd users as before:

# getent passwd cockpit-wsinstance
cockpit-wsinstance:x:993:990:User for cockpit-ws instances:/nonexisting:/sbin/nologin

There should be a /etc/cockpit/ws-certs.d/10-ipa.cert, but isn't. And lo and behold:

audit: type=1400 audit(1672770758.901:4): avc:  denied  { create } for  pid=5969 comm="certmonger" name="10-ipa.key" scontext=system_u:system_r:certmonger_t:s0 tcontext=system_u:object_r:etc_t:s0 tclass=file permissive=0

With setenforce 0 both tests work fine, except for the unexpected SELinux message (with permissive=1).

Tests pass again when I drop the %sysusers_create_inline from the RPM, even though it ought to be a no-op if the users already exist.

Seems the RPM macro is broken:

/var/tmp/rpm-tmp.Ayc35I: line 27: warning: here-document at line 6 delimited by end-of-file (wanted `SYSTEMD_INLINE_EOF')

@martinpitt martinpitt marked this pull request as draft January 3, 2023 18:21
@allisonkarlitskaya
Copy link
Member

could this be caused by a conflict with the existing users that we get from installing the package in the base image? in a certain sense we're not really properly testing the creation of new users here...

@martinpitt
Copy link
Member Author

@allisonkarlitskaya : We test three cases now:

  • Upgrades from the old useradd generated user, where the sysusers.d config should be a no-op (most tests in our CI). That's causing the subtle breakage of IPA certificate generation, which must be some weird side effect. I'm going to investigate it this morning.
  • Fresh package installs with sysusers generated users from RPM %pre script (Testing Farm)
  • sysusers generated users during boot after resetting /etc (TestConnection.testWsPackage)

On a busy CI machine this can easily take more than a minute, which
causes test flakes. Increase the timeout to 3 minutes.
`dh_install --fail-missing` has been deprecated for a while and got
removed in dh compat level 12 (which we use). Use `dh_missing` instead.
We can drop the override once we move to dh compat level 13. This fixes
the warning during build:

    dh_install: warning: Please use dh_missing --list-missing/--fail-missing instead
    dh_install: warning: This feature will be removed in compat 12.
Move to using systemd's sysusers declarative files [1] for creating our
system users/groups. Arch already does that, Fedora moved to it since
Fedora 32 [2], and Debian supports it as well [3].

In debian/cockpit-ws.postinst, move the `#DEBHELPER#` block above the
statoverride, as the former now generates the user, and the latter needs
it.

Unfortunately Fedora/rpm's `%attr` does not really work with sysusers
files shipped upstream yet. The conf files are not installed yet during
`%pre`, but creating the users in `%post` is too late for the file
unpack phase, so cockpit-session would get the wrong permissions. Thus
duplicate the two sysusers config lines verbatim in `%pre`, which is at
least marginally better than calling `useradd` etc. programmatically.

Extend TestConnection.testWsPackage to remove the system users, reboot,
and validate that cockpit still works. This ensures correct sysusers.d
packaging across all distributions, as our normal CI images already have
the system users.

Fixes cockpit-project#15027

[1] https://www.freedesktop.org/software/systemd/man/sysusers.d.html
[2] https://fedoraproject.org/wiki/Changes/Adopting_sysusers.d_format
[3] https://manpages.debian.org/dh_installsysusers
@martinpitt
Copy link
Member Author

LOL 🤣 The reason was that the macro is broken in RHEL 8 (see edited comment above), and thus the remainder of %pre was skipped. As a result, it was using the selinux-policy cockpit policy instead of our own. Fixed!

@martinpitt martinpitt temporarily deployed to cockpit-dist January 4, 2023 06:21 — with GitHub Actions Inactive
@martinpitt martinpitt marked this pull request as ready for review January 4, 2023 07:29
@allisonkarlitskaya
Copy link
Member

I think the trouble we're seeing here is maybe a fundamental incompatibility between sysusers and having non-root-owned files installed by the package.

If the user gets deleted and later recreated, the uid on the filesystem might not be the same as the uid of the new user...

@martinpitt martinpitt marked this pull request as draft January 4, 2023 07:57
@martinpitt martinpitt added the blocked Don't land until something else happens first (see task list) label Jan 4, 2023
@martinpitt
Copy link
Member Author

Ack, indeed. So let's shelve this and reconsider after #16808 -- if that ever lands, that would unblock the sysusers approach. Thanks for catching!

@martinpitt
Copy link
Member Author

This is still very far out, and I don't want to stare at this on my /pulls page for that long. I sent the Debian packaging fix to #18339, and will close this one for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Don't land until something else happens first (see task list)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ws: please provide sysusers.d entries for system users/groups
2 participants