Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

Use DynamicUser=yes #29

Merged
merged 1 commit into from
Sep 19, 2019
Merged

Use DynamicUser=yes #29

merged 1 commit into from
Sep 19, 2019

Conversation

cgwalters
Copy link
Member

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.

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
Copy link
Member

zonggen commented Sep 16, 2019

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

@bgilbert
Copy link
Contributor

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
Copy link
Member

zonggen commented Sep 16, 2019

@bgilbert And that state is related to the timer mechanism mentioned in coreos/fedora-coreos-tracker#86 (comment)?

@bgilbert
Copy link
Contributor

@zonggen Yup.

@cgwalters
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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

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

@bgilbert
Copy link
Contributor

Sure, fine with me.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jlebon jlebon merged commit edc9570 into coreos:master Sep 19, 2019
@jlebon
Copy link
Member

jlebon commented Sep 19, 2019

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

@cgwalters
Copy link
Member Author

(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
Copy link
Member

zonggen commented Nov 26, 2019

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
Copy link
Member Author

cgwalters commented Nov 26, 2019

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
Copy link
Member

zonggen commented Nov 26, 2019

@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
Copy link
Member Author

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
Copy link
Member Author

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

Successfully merging this pull request may close these issues.

None yet

4 participants