RedHatInsights / yggdrasil

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

feat: Run yggd as non-root user by default #157

Closed jirihnidek closed 3 months ago

jirihnidek commented 1 year ago
jirihnidek commented 1 year ago

If you want to test this PR, then it is necessary to build yggdrasil and then install it to your system using sudo meson install -C builddir

subpop commented 1 year ago

Did you look into using systemd's DynamicUser= directive before settling on this approach? I don't quite see the reason why we must include a static username in the configuration file. Shouldn't the username of the service be irrelevant to the user, and more of a configuration detail?

jirihnidek commented 1 year ago

Correct me if I'm wrong, but directive DynamicUser can be used only with directive User and Group. We cannot use this approach (unit file with directive User=yggd & Group=yggd), because the service would be started with such user from the beginning. We need to start service as a root to be able to set correct ownership of client-id file (possibly change ownership of other files/directories in the future) and later we change uid and gid using setgid() and setuid().

In theory we could also use something like ExecStartPre=chown yggd:yggd /path/to/file for fixing ownership, but it gives us less flexibility. Maybe we will need to do other things as root in the future.

I'm also not sure if dynamically created user would play nicely with D-Bus policy file.

I think that it is still important to give users option to change user to root in some configuration file (config.toml looks like a logical file for that configuration).

It will be probably necessary to modify SELinux rules related to yggd, and it will be possible to do it only with static user.

subpop commented 1 year ago

According to the documentation, DynamicUser will derive a username based on the unit name if User is unset. yggd already can run as a non-root user. You can run it and connect it to your session bus. It's also possible to run as a non-root user and connect to the system bus, but that non-root user needs to be given explicit permissions to send messages on the bus. What is preventing us from running yggd as non-root from the beginning of the process? Just reading random files like cert-file and key-file?

jirihnidek commented 1 year ago

What is preventing us from running yggd as non-root from the beginning of the process? Just reading random files like cert-file and key-file?

Yeah, reading/writing some files and making sure that the client-id file has right owner and permission.

I remember that we had some discussion about running of yggdrasil as non-root user and my impression was that it will be OK to use setgid() & setuid() eventually setegid() & seteuid(). So, it was reason, why I decided to implement it in this way.

If it was possible to run yggdrasil as non-root user using only some trivial changes in unit file, then I would do that, but I still believe that it is not possible and it would add us some unnecessary constrains.

jirihnidek commented 7 months ago

@subpop proposed solution just does not work: https://github.com/RedHatInsights/yggdrasil/pull/218 and I consider it too brittle.

jirihnidek commented 7 months ago

BTW: I did some experiments with seteuid() and setegid() and I would not recommend to use it, because, when you set temporally euid back to 0 to do something root specific (e.g. write some file as a root), then all go routines run ATM using 0, which is not what you want.

subpop commented 7 months ago

Interesting. That makes sense when I think about it. So are you suggesting we close this PR in favor of #218?

jirihnidek commented 7 months ago

Interesting. That makes sense when I think about it. So are you suggesting we close this PR in favor of #218?

No, I would just close the #218 ... I created this new PR to demonstrate that benefits of systemd could not help us in this case.

jirihnidek commented 7 months ago

Hmm, it breaks packit build... I will have to fix it.

jirihnidek commented 3 months ago

Closing this PR, because #224 won the competition.