RedHatInsights / yggdrasil

GNU General Public License v3.0
21 stars 37 forks source link

Support running yggd as non-root #224

Closed subpop closed 3 months ago

subpop commented 6 months ago

Add support for running yggd as a non-root system user. Two new build options, "user" (defaulting to "yggdrasil") and "group" (defaulting to "yggdrasil"), are used to configure the various systemd units, systemd sysuser templates, D-Bus policy files and a pkgconfig module. This allows distributions to decide how to configure the yggdrasil service to suit their exact requirements.

Additionally, this PR includes some extra commits:

  1. Separate the system and user units; prior to this, the system unit was installed twice, once to the system services directory and again into the user services directory. This proved to be inadequate. The user slice does not have a "network-online" target, resulting in the user service failing to start. Separating the two units fixes the user service so it works properly.
  2. A bug was found in the way values were computed in the constants package when running as root compared to non-root. The commit reworks the way constants are determined; they're refactored to prefer systemd provided values, falling back to XDG paths (if non-root) and custom paths if root.
  3. Use systemd macros in the spec file to properly reload services during installation.

All three of these commits are not technically part of this feature implementation, but were so closely related I felt it acceptable to keep them in this same PR.

The final commit not mentioned above implements the meson build options, systemd sysuser template and the necessary configuration changes to the systemd services and D-Bus policy files.

Fixes: #152

jirihnidek commented 6 months ago

Original PR #157 does more things. It also introduces group ygg_worker to be able run worker as non-root user. #157 also modified https://github.com/RedHatInsights/yggdrasil/pull/157/files#diff-7eb531046c44fae543db356a1c18ce0ad5c9217049353d04fe87eac8da7f80a4 to be able to work with yggd.service running as non-root user.

The yggd_worker should be defined in the system user configuration file like this: https://github.com/RedHatInsights/yggdrasil/pull/157/files#diff-59036393d7e836a1742b10c66b44912b081764e45354aebee0b2c608ac03f391

I will add more comments to the review of code.

subpop commented 6 months ago

157 also modified com.redhat.Yggdrasil1.Worker1.echo.conf to be able to work with yggd.service running as non-root user.

This raises an interesting question. The echo worker exists as a demonstration of how to write a worker. If we incorporate the non-root sysuser option into that worker, it means that other, out-of-tree workers would need to somehow know the sysuser running the main yggd process at compile time in order to write the appropriate busconfig policy. Perhaps we need to add a pkgconfig ".pc" file again so that out-of-tree workers can configure their busconfig policy files.

Original PR https://github.com/RedHatInsights/yggdrasil/pull/157 does more things. It also introduces group ygg_worker to be able run worker as non-root user. The yggd_worker should be defined in the system user configuration file like this: https://github.com/RedHatInsights/yggdrasil/pull/157/files#diff-59036393d7e836a1742b10c66b44912b081764e45354aebee0b2c608ac03f391

This is a good point too, but is tangential to #152. Running yggd as a non-root user is not the same as running all workers as a non-root user. There are workers that expect to have full root access (yggdrasil-worker-package-manager, rhc-worker-playbook, rhc-script-worker), and there are others (yggdrasil-worker-desktop-notification) that expect to run as non-root. I don't think the upstream yggdrasil project should dictate which user a worker runs as.

We can make echo run as a non-root user, again, for demonstrative purposes, but I don't think we need to add a new user for that; we can reuse the same sysuser that yggd uses.

jirihnidek commented 6 months ago

We are limited here, because non-root user could not read consumer cert and key. We need to find some solution for this issue.

subpop commented 6 months ago

OK. I've experimented with this some more, and I'm of the opinion that running as a non-root user without the use of setuid/setgid syscalls is a cleaner approach. Initializing the process as root creates a window of time when the process is privileged. While it's true now that (1) we control that window; and (2) the window is very small, over time, those two statements may not remain true. Having this window of privileged execution means present creates the possibility of a privileged execution attack. Considering we can make this work as non-root from process origination, we can avoid the additional risk this theoretical attack surface brings.

subpop commented 5 months ago

Some of my individual comments seem to have been lost between Visual Studio Code and GitHub. They all revolve around the same subject though. You suggested using a separate group for worker processes. What is the reason for having two groups: yggdrasil and yggdrasil_workers? The reason a group exists at all is to permit processes to send to the com.redhat.Yggdrasil1.Dispatcher1 destination. I don't see why that group needs to be separate from the primary group of the main yggd process.

Both user and group are configurable build options, so if a distribution wanted to use a separate group, they could configure it like so: meson setup -Duser=yggdrasil -Dgroup=yggdrasil_workers

The sysusers.d manpage implies that in this case, a separate primary group and secondary group will get created. If the user and group value match, a single user & group entry will be created. If they differ, then a primary group matching the user name will be created, and a secondary group will be created and the user will be added to the group.

When configured with a matching user and group the following user & group configuration is created:

Creating group 'yggdrasil' with GID 991. Creating user 'yggdrasil' (yggdrasil system user) with UID 991 and GID 991.

$ id yggdrasil
uid=991(yggdrasil) gid=991(yggdrasil) groups=991(yggdrasil)

When configured with a separate user & group, the following user & group configuration is created:

Creating group 'yggdrasil_workers' with GID 991. Creating group 'yggdrasil' with GID 987. Creating user 'yggdrasil' (yggdrasil system user) with UID 987 and GID 987.

$ id yggdrasil
uid=987(yggdrasil) gid=987(yggdrasil) groups=987(yggdrasil),991(yggdrasil_workers)

So should a distribution wish to run workers with a separate group, they could configure it to do so. I don't see the need to add an explicit, second "worker group" in this case, since the sysusers configuration file we ship can already handle both cases.

jirihnidek commented 5 months ago

@subpop if the only one intention of yggdrasil group is to allow workers to call Transmit() method from interface com.redhat.Yggdrasil1.Dispatcher1, then it is little bit risky. Why? The same group is also used for creating all files. There is danger that we will give write access to some file that should not be writable by workers. Maybe it is not so risky now, but you never know what file will be created by yggdrasil service in the future.

For this reason I still believe that it is good to have separate group allowing to worker to call method(s) from interface com.redhat.Yggdrasil1.Dispatcher1

subpop commented 5 months ago

@subpop if the only one intention of yggdrasil group is to allow workers to call Transmit() method from interface com.redhat.Yggdrasil1.Dispatcher1, then it is little bit risky. Why? The same group is also used for creating all files. There is danger that we will give write access to some file that should not be writable by workers. Maybe it is not so risky now, but you never know what file will be created by yggdrasil service in the future.

For this reason I still believe that it is good to have separate group allowing to worker to call method(s) from interface com.redhat.Yggdrasil1.Dispatcher1

That's a very good point. I'll add a second group for workers to run as.