use DynamicUser=yes for all cockpit components #16811

allisonkarlitskaya · 2022-01-11T08:00:34Z

Switch over to systemd allocating user IDs for us dynamically, dropping our static users.

We create these users dynamically:

cockpit-session-socket is the group owner of /run/cockpit/session
cockpit-wsinstance-socket is the group owner of all sockets in /run/cockpit/wsinstance/
cockpit-tls gets its own user
cockpit-wsinstance-http{,s@}.service each get a user. In this case, I think the separate https instances even each get a separate user.

This is a bit complicated due to systemd/systemd#23067

Launch cockpit-session via socket activation on /run/cockpit/session #16808
Clean up obsolete cockpit-wsinstance user in spec and debian packaging on upgrade

martinpitt · 2022-04-13T11:21:42Z

I filed systemd/systemd#23067 to start the discussion.

martinpitt · 2024-05-10T10:03:07Z

Rebased after the rebasing of #16808 and the recent heavy changes to sysuser management. Locally this at least works for a simple local session, let's see if there's any fallout beyond what's already in #16808.

cockpit-wsinstance-http{,s@}.service each get a user.

I didn't port this bit yet, let's do that step separately. We have much more freedom to change this once everything is a DynamicUser.

martinpitt · 2024-05-10T11:52:21Z

Cool -- the test failures are by and large the same as in #16808. Just the unit lifecycle test fails in addition, but that'll be straightforward to fix -- and indeed we should adjust this some more to check proper cleanup after stopping.

cgwalters · 2024-11-05T15:26:00Z

xref containers/bootc#870 too here

martinpitt

@allisonkarlitskaya This looks good in general (albeit annoyingly complicated, but that systemd request didn't go anywhere). There are two failures still which perfectly reproduce locally.

Now, do you want me to work on this (like #16808 reverse ownership/review 😁 ) or want to do this yourself?

martinpitt · 2024-11-21T13:57:30Z

src/systemd/cockpit-session-socket-user.service

@@ -0,0 +1,11 @@
+[Unit]
+Description=Dynamic user for /run/cockpit/session socket
+BindsTo=cockpit.service


Conceptually it may make more sense to bind to cockpit-session.socket here?

That actually explains this failure -- The test only wants to start cockpit-session.socket and run its own cockpit-ws process. Like this, starting the session socket also starts cockpit.socket, which then fails due to port 9090 being busy already.

postinst scripts can also be called in "abort-upgrade" and "abort-remove" mode. Don't clean up the statoverride in these cases. Fixes commit 98f3d6f.

Unfortunately, socket units can't set DynamicUser=yes [1], so add a dependency on a separate .service created just for this purpose. /run/cockpit/session is now owned by group cockpit-session-socket which also gets added as a supplementary group to the cockpit-ws units. Adjust the proxy tests accordingly. [1] systemd/systemd#23067

Similar to the last commit, we create a dynamic group for the sockets in /run/cockpit/wsinstance and add a supplementary group to cockpit-tls.

We only dynamically generate a single .socket file now. The rest of them are in version control and installed verbatim.

Clean it up on package upgrades. Unlike the old `cockpit-ws` user, this user never owned any files on disk (other than cockpit-system), so this is safe. Co-Authored-By: Martin Pitt <[email protected]>

martinpitt · 2024-11-21T19:24:06Z

Rebased, fixed the above systemd unit issue, adjusted the proxy tests, and added the cleanup of the obsolete static system user. That was pleasantly straightforward!

martinpitt

While this is quite a complex zoo of systemd units, it is rather straightforward and adequately easy to understand (at least for me). The benefits are definitively worth it, so let's do this!

@allisonkarlitskaya I dabbled with this (minor unit changes and test adjustments, and the package postinst cleanups), so please cross-check.

allisonkarlitskaya

Looking at this again, I think this is quite fine.

I see some cleanup potential here (comments below), but since there's no longer any static state on the system, we can easily change those in the future. And arguably, they're unrelated changes.

allisonkarlitskaya · 2024-11-22T08:48:51Z

src/systemd/cockpit.service.in

@@ -4,8 +4,8 @@ Documentation=man:cockpit-ws(8)
 Requires=cockpit.socket
 Requires=cockpit-wsinstance-http.socket cockpit-wsinstance-https-factory.socket
 # ensure our DynamicUser exists
-Requires=cockpit-ws-user.service
-After=cockpit-ws-user.service
+Requires=cockpit-ws-user.service cockpit-wsinstance-socket-user.service


Reading this, even as someone who understands how this pile of software works, is confusing.

We've used cockpit-ws for cockpit-tls and cockpit-wsinstance for cockpit-ws for a while now for reasons that are no longer relevant. I think we should take the time to change these usernames: cockpit-tls for cockpit-tls and cockpit-ws for cockpit-ws.

We need to be careful about "cockpit-ws", as we used to have that static user, and /etc/cockpit/ws-certs.d/* may still be owned by it on old systems. In general, +1 for cleaning up the user names, though!

For that reason I added postinst scripts which remove the cockpit-wsinstance user on upgrade, but not for cockpit-ws. I don't want to get into the business of randomly chowning files in /etc. You can just too easily screw up shared certificates, backups, break Ansible scripts, etc.

allisonkarlitskaya · 2024-11-22T08:49:34Z

src/systemd/cockpit.service.in

@@ -17,6 +17,7 @@ ExecStartPre=+@libexecdir@/cockpit-certificate-ensure --for-cockpit-tls
 ExecStart=@libexecdir@/cockpit-tls
 User=cockpit-ws
 Group=cockpit-ws
+SupplementaryGroups=cockpit-wsinstance-socket


And ditto. We should have cockpit-ws-socket and cockpit-session-socket.

allisonkarlitskaya · 2024-11-22T08:52:27Z

test/verify/check-connection

@@ -332,7 +332,7 @@ class TestConnection(testlib.MachineCase):
        # number of https instances is bounded (DoS prevention)
        # with MaxTasks=200 und 2 threads per ws instance we should have a
        # rough limit of 100 instances, so at some point curl should start failing
-        m.execute("runuser -u cockpit-ws -- sh -ec 'RC=1; for i in `seq 120`; do "
+        m.execute("runuser -u cockpit-wsinstance-socket -- sh -ec 'RC=1; for i in `seq 120`; do "


It occurs to me that if we ran this as cockpit-ws (ie: the account running cockpit-tls) it wouldn't work because the supplementary group isn't on that account, but rather comes in via an additional option in the unit. Can we move the supplementary group to the unit that dynamically creates the user? That might make things a bit easier to understand, and would allow us to drop this patch fragment...

allisonkarlitskaya added the blocked Don't land until something else happens first (see task list) label Jan 11, 2022

allisonkarlitskaya requested a review from martinpitt January 11, 2022 08:00

allisonkarlitskaya temporarily deployed to cockpit-dist January 11, 2022 08:06 Inactive

martinpitt removed their request for review April 7, 2022 09:02

martinpitt mentioned this pull request Apr 13, 2022

DynamicUser= support for AF_UNIX socket units for ownership of the inode systemd/systemd#23067

Open

KKoukiou added the review-2022-12 label Dec 14, 2022

This was referenced Jan 3, 2023

ws: please provide sysusers.d entries for system users/groups #15027

Open

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

Closed

martinpitt removed the review-2022-12 label Feb 15, 2023

martinpitt mentioned this pull request May 3, 2024

tools: Use DynamicUser for cockpit.service #20425

Merged

martinpitt force-pushed the dynamic-users branch from c01589f to 8460b13 Compare May 10, 2024 10:02

martinpitt added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label May 10, 2024

martinpitt force-pushed the dynamic-users branch from 8460b13 to 4752cca Compare May 10, 2024 10:03

martinpitt mentioned this pull request Nov 1, 2024

/usr/libexec/cockpit-session has wrong owner in deployment #21201

Closed

cgwalters mentioned this pull request Nov 5, 2024

/usr/libexec/cockpit-session has wrong owner in deployment containers/bootc#870

Closed

martinpitt mentioned this pull request Nov 15, 2024

Launch cockpit-session via socket activation on /run/cockpit/session #16808

Merged

14 tasks

martinpitt force-pushed the dynamic-users branch from 4752cca to e0fcb87 Compare November 15, 2024 12:26

This comment was marked as outdated.

Sign in to view

martinpitt force-pushed the dynamic-users branch 2 times, most recently from cd54812 to 2ad8273 Compare November 17, 2024 08:54

This comment was marked as outdated.

Sign in to view

martinpitt force-pushed the dynamic-users branch 2 times, most recently from 70f0d85 to 165d0b6 Compare November 21, 2024 13:35

martinpitt requested changes Nov 21, 2024

View reviewed changes

martinpitt removed the blocked Don't land until something else happens first (see task list) label Nov 21, 2024

martinpitt force-pushed the dynamic-users branch from 165d0b6 to ec1102e Compare November 21, 2024 16:50

martinpitt removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 21, 2024

martinpitt and others added 5 commits November 21, 2024 20:13

tools: Fix dpkg-statoverride cleanup

26f5aa8

postinst scripts can also be called in "abort-upgrade" and "abort-remove" mode. Don't clean up the statoverride in these cases. Fixes commit 98f3d6f.

systemd: dynamic group for wsinstance sockets

8ae0d32

Similar to the last commit, we create a dynamic group for the sockets in /run/cockpit/wsinstance and add a supplementary group to cockpit-tls.

.gitignore: only ignore cockpit.socket

e438ef2

We only dynamically generate a single .socket file now. The rest of them are in version control and installed verbatim.

systemd, tools: stop creating static cockpit-wsinstance user

73875c2

Clean it up on package upgrades. Unlike the old `cockpit-ws` user, this user never owned any files on disk (other than cockpit-system), so this is safe. Co-Authored-By: Martin Pitt <[email protected]>

martinpitt force-pushed the dynamic-users branch from ec1102e to 73875c2 Compare November 21, 2024 19:24

martinpitt marked this pull request as ready for review November 21, 2024 19:24

martinpitt approved these changes Nov 21, 2024

View reviewed changes

allisonkarlitskaya added the release-blocker Targetted for next release label Nov 22, 2024

allisonkarlitskaya commented Nov 22, 2024

View reviewed changes

allisonkarlitskaya merged commit 2ff19fa into cockpit-project:main Nov 22, 2024
87 checks passed

allisonkarlitskaya deleted the dynamic-users branch November 22, 2024 08:54

cgwalters mentioned this pull request Nov 22, 2024

uid/gid drift possible when switching between different host builds containers/bootc#673

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use DynamicUser=yes for all cockpit components #16811

use DynamicUser=yes for all cockpit components #16811

allisonkarlitskaya commented Jan 11, 2022 •

edited by martinpitt

Loading

martinpitt commented Apr 13, 2022

martinpitt commented May 10, 2024 •

edited

Loading

martinpitt commented May 10, 2024

cgwalters commented Nov 5, 2024

This comment was marked as outdated.

This comment was marked as outdated.

martinpitt left a comment

martinpitt Nov 21, 2024

martinpitt Nov 21, 2024

martinpitt commented Nov 21, 2024

martinpitt left a comment

allisonkarlitskaya left a comment

allisonkarlitskaya Nov 22, 2024

martinpitt Nov 22, 2024

martinpitt Nov 22, 2024

allisonkarlitskaya Nov 22, 2024

allisonkarlitskaya Nov 22, 2024

use DynamicUser=yes for all cockpit components #16811

use DynamicUser=yes for all cockpit components #16811

Conversation

allisonkarlitskaya commented Jan 11, 2022 • edited by martinpitt Loading

martinpitt commented Apr 13, 2022

martinpitt commented May 10, 2024 • edited Loading

martinpitt commented May 10, 2024

cgwalters commented Nov 5, 2024

This comment was marked as outdated.

This comment was marked as outdated.

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Nov 21, 2024

Choose a reason for hiding this comment

martinpitt Nov 21, 2024

Choose a reason for hiding this comment

martinpitt commented Nov 21, 2024

martinpitt left a comment

Choose a reason for hiding this comment

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

allisonkarlitskaya Nov 22, 2024

Choose a reason for hiding this comment

martinpitt Nov 22, 2024

Choose a reason for hiding this comment

martinpitt Nov 22, 2024

Choose a reason for hiding this comment

allisonkarlitskaya Nov 22, 2024

Choose a reason for hiding this comment

allisonkarlitskaya Nov 22, 2024

Choose a reason for hiding this comment

allisonkarlitskaya commented Jan 11, 2022 •

edited by martinpitt

Loading

martinpitt commented May 10, 2024 •

edited

Loading