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

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

Closed
martinpitt opened this issue Nov 5, 2024 · 10 comments
Closed

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

martinpitt opened this issue Nov 5, 2024 · 10 comments

Comments

@martinpitt
Copy link

In cockpit-project/cockpit#21201 we got a report about failing to deploy a bootc container with the cockpit-ws package:

FROM quay.io/centos-bootc/centos-bootc:stream9
RUN dnf -y install cockpit-ws cockpit-bridge
RUN systemctl enable cockpit.socket
EOF

You can build yourself, but for convenience I built that on github:

bootc switch ghcr.io/martinpitt/workstation-bootc@sha256:8f6f12fa630e49541281d32825ca16e534da7cfe0014074fc3d3fcc2550d648e

This command works, but after boot, cockpit-ws' cockpit-session program has a broken owner:

# id cockpit-wsinstance
uid=981(cockpit-wsinstance) gid=981(cockpit-wsinstance) groups=981(cockpit-wsinstance)

# ls -l /usr/libexec/cockpit-session 
-rwsr-x---. 1 root 995 57120 Jan  1  1970 /usr/libexec/cockpit-session

It needs to have group "cockpit-wsinstance".

A temporary workaround is

bootc usr-overlay
chgrp cockpit-wsinstance /usr/libexec/cockpit-session

but of course that doesn't stick, and enabling the overlay is ugly.

@martinpitt
Copy link
Author

@sigulete just did a response in the original bug, which I'm forwarding here:

I dug deeper into this issue and it doesn’t seem to be a bootc bug. It is just the nature of how the workflow works.

Bootc relies on OCI containers to build and transport an image to be deployed in what we will call the server for the sake of this example. Each time the container image gets re-built, it will create a brand new deployment that will replace everything in /usr and everything that hasn't been locally changed in /etc.

Some applications including cockpit get an UID and GID assigned within a range. Sometimes, it is the same for various iterations until it is not, and the allocated UID/GID in the container for the new image will eventually differ from the previous one. As a consequence the file /etc/passwd in the container will be adjusted based on this UID/GID allocation.

If /etc/passwd was not changed in the server, then it will be replaced by bootc during the deployment of an upgrade, and all works fine. But if this file was locally modified, then the local version will prevail and it won't be replaced by the one from the container. If the administrator creates, modifies or removes users in the server, then /etc/passwd will be modified and it will never get updated.

I looked at my deployment and I found that it is not common to have files in /usr with non-root USERID or GROUPID. In my case, I found 10 files with non-root GROUPID and with the exception of cockpit all the rest are using system users, which are not defined in /etc/passwd. Consequently cockpit was the only application prone to this issue.

I resolved the problem creating my user within the container build, so /etc/passwd won't get modified locally after deployment and it will always be managed and updated by bootc. But this is something that won't be possible for a multiuser environment or similar use case.

You mentioned before that cockpit will eventually move to Dynamic Users, I suppose it will resolve this issue.

@sigulete
Copy link

sigulete commented Nov 5, 2024

I want to report that there are a few nuances with this approach, for some reason the /home/$USER directory is not being copied from the container to the server during installation when using ISO image (created by bootc-image-builder).
I used the following script to fix it for now:

USER_NAME=<username>
USER_DIR=/var/home/$USER_NAME

if [ ! -d $USER_DIR ]
then
  mkdir $USER_DIR
  cp -r /etc/skel/.* $USER_DIR
  chown -R $USER_NAME:$USER_NAME $USER_DIR
fi

@cgwalters
Copy link
Collaborator

There's a lot more in https://containers.github.io/bootc/building/users-and-groups.html but yes basically stop using useradd for system services.

@cgwalters
Copy link
Collaborator

Also setuid binaries are bad

@martinpitt
Copy link
Author

Well, cockpit-ws primarily wants to use /usr/lib/sysusers.d/cockpit-wsinstance.conf to create the sysuser, but that doesn't work yet in rpm-land, see rpm-software-management/rpm#3073 (it was fixed upstream, but isn't available widely yet in Fedora and RHEL land). But also, the system user actually exists, it's just not applied correctly to the binary ownership.

Also setuid binaries are bad

Yup, I hear you. Getting rid of it is a ginormous pile of work (see linked PR above), but also, it's the file ownership here, not the suid-ness (that's fine).

@cgwalters
Copy link
Collaborator

But also, the system user actually exists, it's just not applied correctly to the binary ownership.

Yeah there's a conceptual conflict between sysusers and files owned by images (I suspect for the same reason this gets trickier with RPM).

I think for this case ideally we fatally error out at build time and require a static allocation at build time; the most practical is nss-altfiles; we just don't document today how to extend what we have in the base image for it.


That said, there is an entirely different approach which is basically have /usr/libexec/cockpit-session be owned by root and not setuid, and have a systemd-tmpfiles snippet (or maybe needs to be a unit) which copies it to /run and makes it setuid there and chowns it.

For everyone else's reference the PR Martin is talking about for cockpit that would drop the suid binary looks to be cockpit-project/cockpit#16811

@cgwalters
Copy link
Collaborator

but also, it's the file ownership here, not the suid-ness (that's fine).

I think they are related, no? If the binary wasn't setuid it could just be owned by root:root like any other...

@martinpitt
Copy link
Author

That said, there is an entirely different approach which is basically have /usr/libexec/cockpit-session be owned by root and not setuid, and have a systemd-tmpfiles snippet (or maybe needs to be a unit)

That's a hilariously clever and evil 😈 idea, thanks 🤓 ! That would make packaging dramatically easier, and also avoid all this ownership mess, and even unblock delivering this as a sysext. Need to check how this plays out with SELinux, and of course it does feel rather root-kit-y to have a writable suid root binary in /run... But it's worth discussing at least.

(Or finally carve out two weeks to fix all the fallout from making cockpit-session a systemd .path unit...)

@cgwalters
Copy link
Collaborator

cgwalters commented Nov 5, 2024

Need to check how this plays out with SELinux, and of course it does feel rather root-kit-y to have a writable suid root binary in /run...

Dunno, on package systems the binary is writable in /usr/bin too...

@cgwalters
Copy link
Collaborator

Closing this as there's not much we can technically do on the bootc side today unfortunately, other than the existing documentation we have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants