coreos / fedora-coreos-pinger

Telemetry service to be used in Fedora CoreOS (see https://github.com/coreos/fedora-coreos-tracker/issues/86)
Apache License 2.0
15 stars 7 forks source link

Use DynamicUser=yes #29

Closed cgwalters closed 4 years ago

cgwalters commented 4 years ago

I was looking at /etc/passwd on FCOS and noticed a user allocated for this service.

With modern systemd, I think DynamicUser=yes is exactly what we want for this type of service. We don't need to allocate a uid/gid persistently on the system. It's just a lot cleaner this way.

Drop the tmpfiles.d snippet too; this means the admin would need to mkdir that directory. Which is probably better actually, because we don't want the directory to actually be owned by the service user since then it'd be mutable by the service. The admin just needs to make the directories world executable and the files world-readable.

zonggen commented 4 years ago

LGTM! Just a reminder for myself, this would also involve updating fedora package located here once merged.

bgilbert commented 4 years ago

I agree with dropping the tmpfiles.d snippet. But I don't think DynamicUser is going to work for us, since pinger will need to store persistent state in /var and that state will need to be owned by a persistent user.

zonggen commented 4 years ago

@bgilbert And that state is related to the timer mechanism mentioned in https://github.com/coreos/fedora-coreos-tracker/issues/86#issuecomment-456671991?

bgilbert commented 4 years ago

@zonggen Yup.

cgwalters commented 4 years ago

But I don't think DynamicUser is going to work for us, since pinger will need to store persistent state in /var and that state will need to be owned by a persistent user.

Hm, does it do that currently? Anyways persistent state is fully supported, see http://0pointer.net/blog/dynamic-users-with-systemd.html section "Persistent Data".

bgilbert commented 4 years ago

It doesn't do that currently, no. And I didn't know about persistent data support; that's really neat! Let's add a StateDirectory while we're here.

cgwalters commented 4 years ago

Let's add a StateDirectory while we're here.

Hmm; not opposed, but why not do it when we actually start writing state?

bgilbert commented 4 years ago

Sure, fine with me.

jlebon commented 4 years ago

(Offhand it seems like we should be able to do the same thing for Zincati too?)

cgwalters commented 4 years ago

(Offhand it seems like we should be able to do the same thing for Zincati too?)

Ah yes, see I came here because I saw Zincati first but I'm not 100% sure it will play nicely with the polkit rule.

zonggen commented 4 years ago

Just want to make a note that StateDirectory= does not play well with DynamicUser= currently. Systemd does not allow multiple dynamic user share the same state directory. For example,

# This will run without issue
$ sudo systemd-run --pty --property=DynamicUser=yes --property=StateDirectory=wuff /bin/sh
Running as unit: run-u32.service
Press ^] three times within 1s to disconnect TTY.
sh-4.4$
[CTRL+D]

# This will fail with permission error since /var/lib/wuff already exists
$ sudo systemd-run --pty --property=DynamicUser=yes --property=StateDirectory=wuff /bin/sh

However, LogsDirectory= will work nicely with DynamicUser=, and the only difference is that LogsDirectory will store persistent files under /var/log instead of var/lib. The same example runs without issue:

# This will run without issue
$ sudo systemd-run --pty --property=DynamicUser=yes --property=LogsDirectory=wuff /bin/sh
Running as unit: run-u32.service
Press ^] three times within 1s to disconnect TTY.
sh-4.4$
[CTRL+D]

# Runs this time
$ sudo systemd-run --pty --property=DynamicUser=yes --property=LogsDirectory=wuff /bin/sh
Running as unit: run-u36.service
Press ^] three times within 1s to disconnect TTY.
sh-4.4$
[CTRL+D]
cgwalters commented 4 years ago

Just want to make a note that StateDirectory= does not play well with DynamicUser= currently.

Isn't that more because systemd (by default) creates a separate unit for each systemd-run, which then can't share the same StateDirectory. But the below works fine for me - note we specify --unit to get the same unit each time:

[root@coreos ~]# systemd-run --unit=walters-test --pty --property=DynamicUser=yes --property=StateDirectory=canada echo hello world  
Running as unit: walters-test.service
Press ^] three times within 1s to disconnect TTY.
hello world
[root@coreos ~]# systemd-run --unit=walters-test --pty --property=DynamicUser=yes --property=StateDirectory=canada echo hello world 
Running as unit: walters-test.service
Press ^] three times within 1s to disconnect TTY.
[root@coreos ~]# 

IOW it should work to do this for "static" units like fedora-coreos-pinger.service.

zonggen commented 4 years ago

@cgwalters The second run didn't echo the 'hello world'.. Ran it on a fcos machine, second run still failed by looking at journalctl..

cgwalters commented 4 years ago

Hmm you're right but it should have worked...digging in slightly, this looks like a Fedora SELinux policy bug. From `dmesg | grep avc.*denied':

[  557.343265] audit: type=1400 audit(1574797761.390:159): avc:  denied  { read } for  pid=1341 comm="(echo)" name="canada" dev="vda4" ino=7288065 scontext=system_u:system_r:init_t:s0 tcontext=system_u:object_r:var_lib_t:s0 tclass=lnk_file permissive=0

And indeed setenforce 0 makes this work.

This is definitely just a pure policy bug.

cgwalters commented 4 years ago

https://bugzilla.redhat.com/show_bug.cgi?id=1777043