core: add '-' prefix to AmbientCapabilities= #6564

yuwata · 2017-08-08T07:46:38Z

If AmbientCapabilities= is prefixed by '-' and the ambient capability is not supported by the kernel, then the listed ambient capabilities are ignored and the commands are executed by root.
On top of the options, networkd, resolved, and timesyncd drop the capabilities in the following commits.

If AmbientCapabilities= is prefixed by '-' and the ambient capability is not supported by the kernel, then the listed ambient capabilities are ignored and the commands are executed by root.

Siosm · 2017-08-08T09:43:53Z

As far as I'm concerned, this looks good to me as it appears to fix all issues raised in previous discussions. I've closed my previous PR.

I think there are three minor points:

you should create the /run/systemd/resolved folder using tmpfiles (in commit "resolved: support to run by non-root user"):

diff --git a/tmpfiles.d/systemd.conf.m4 b/tmpfiles.d/systemd.conf.m4
@@ -21,6 +21,9 @@ d /run/systemd/netif 0755 systemd-network systemd-network -
 d /run/systemd/netif/links 0755 systemd-network systemd-network -
 d /run/systemd/netif/leases 0755 systemd-network systemd-network -
 )m4_dnl
+m4_ifdef(`ENABLE_RESOLVED',
+d /run/systemd/resolve 0755 systemd-resolve systemd-resolve -
+)m4_dnl
 
 d /run/log 0755 root root -

commit "timesyncd: support to run by non-root user" might require packaging glue to move the previous 'clock' file to the new location during upgrades.
Should there be a warning during unit spawning that would remind administrators that the user/group has not been changed for the process if ambient capabilities are not available? This warning would then be dismissable by explicitly overriding AmbientCapabilities/User/Group options in the units.

yuwata · 2017-08-09T06:40:20Z

@Siosm Thank you for the comments.

you should create the /run/systemd/resolved folder using tmpfiles (in commit "resolved: support to run by non-root user"):

Is it necessary? The service runs with RuntimeDirectory=systemd/resolve... Unlike /run/systemd/netif, the directory is not inotified, I think. Is it true?

commit "timesyncd: support to run by non-root user" might require packaging glue to move the previous 'clock' file to the new location during upgrades.

OK. I will document it.

Should there be a warning during unit spawning that would remind administrators that the user/group has not been changed for the process if ambient capabilities are not available? This warning would then be dismissable by explicitly overriding AmbientCapabilities/User/Group options in the units.

Hmm, the user/group setting is ignored only if AmbientCapabilities= is prefixed. So, I do not think such warning is necessary.

Also this moves the location of clock file to /var/lib/systemd/timesync.

yuwata · 2017-08-09T07:01:07Z

Updated.

Siosm · 2017-08-09T08:04:13Z

units/systemd-timesyncd.service.in

 SystemCallArchitectures=native
-ReadWritePaths=/var/lib/systemd
+StateDirectory=systemd/timesync
+User=systemd-timesync


Minor personal preference: Could you move the User option near the top of the Service section and add Group option even if it is implied by the User option?

Siosm · 2017-08-09T08:42:26Z

NEWS


+        * clock file is now moved to /var/lib/systemd/timesync/clock.
+          Distributors should change the location of the file if it is packaged.
+


I don't think this file is packaged with systemd. The minor issue here would happen during a restart after an upgrade as the old timesyncd would use the old path and the new timsyncd the new path thus loosing the recorded timestamp. This will also leave an used file in place.

I would suggest:
The file /var/lib/systemd/clock used by systemd-timesyncd to remember the last known good clock value has been moved to /var/lib/systemd/timesync/clock. Distributors should either remove the previous file after the service has been restarted or move the current file to the new directory once the old daemon has been stopped and before the new daemon startup.

But this may be to much of a hassle for such a minor issue / minor gain.

Thank you for the suggestion. I will update the NEWS entry based on that.

I don't think this file is packaged with systemd.

For example, http://pkgs.fedoraproject.org/rpms/systemd/blob/master/f/systemd.spec#_688

The %ghost directive means that the file will be removed when the package is removed/updated. But this file will be written to by systemd-timesyncd when it is stopped. Thus this is still relevant I think.

Siosm · 2017-08-09T08:48:56Z

Is it necessary? The service runs with RuntimeDirectory=systemd/resolve... Unlike /run/systemd/netif, the directory is not inotified, I think. Is it true?

This "double" setup was also suggested to ease debugging (#6393 (comment)), but is not strictly necessary I agree.

This patch adds two new special character prefixes to ExecStart= and friends, in addition to the existing "-", "@" and "+": "!" → much like "+", except with a much reduced effect as it only disables the actual setresuid()/setresgid()/setgroups() calls, but leaves all other security features on, including namespace options. This is very useful in combination with RuntimeDirectory= or DynamicUser= and similar option, as a user is still allocated and used for the runtime directory, but the actual UID/GID dropping is left to the daemon process itself. This should make RuntimeDirectory= a lot more useful for daemons which insist on doing their own privilege dropping. "!!" → Similar to "!", but on systems supporting ambient caps this becomes a NOP. This makes it relatively straightforward to write unit files that make use of ambient capabilities to let systemd drop all privs while retaining compatibility with systems that lack ambient caps, where priv dropping is the left to the daemon codes themselves. This is an alternative approach to systemd#6564 and related PRs.

poettering · 2017-08-09T14:42:27Z

Heya, sorry for not being more responsive on the whole ambient caps/user dropping stuff. I go sidetracked by other stuff for a while. Anyway, a few weeks ago I started working on #6577 which I now filed, which tries to solve the same issue in a different way, and I think in a slightly nicer way...

Any chance you can have a look at that, so that we can find some common ground we can all agree on?

This patch adds two new special character prefixes to ExecStart= and friends, in addition to the existing "-", "@" and "+": "!" → much like "+", except with a much reduced effect as it only disables the actual setresuid()/setresgid()/setgroups() calls, but leaves all other security features on, including namespace options. This is very useful in combination with RuntimeDirectory= or DynamicUser= and similar option, as a user is still allocated and used for the runtime directory, but the actual UID/GID dropping is left to the daemon process itself. This should make RuntimeDirectory= a lot more useful for daemons which insist on doing their own privilege dropping. "!!" → Similar to "!", but on systems supporting ambient caps this becomes a NOP. This makes it relatively straightforward to write unit files that make use of ambient capabilities to let systemd drop all privs while retaining compatibility with systems that lack ambient caps, where priv dropping is the left to the daemon codes themselves. This is an alternative approach to systemd#6564 and related PRs.

yuwata · 2017-08-12T02:48:40Z

Closing this in favor of #6577.

This patch adds two new special character prefixes to ExecStart= and friends, in addition to the existing "-", "@" and "+": "!" → much like "+", except with a much reduced effect as it only disables the actual setresuid()/setresgid()/setgroups() calls, but leaves all other security features on, including namespace options. This is very useful in combination with RuntimeDirectory= or DynamicUser= and similar option, as a user is still allocated and used for the runtime directory, but the actual UID/GID dropping is left to the daemon process itself. This should make RuntimeDirectory= a lot more useful for daemons which insist on doing their own privilege dropping. "!!" → Similar to "!", but on systems supporting ambient caps this becomes a NOP. This makes it relatively straightforward to write unit files that make use of ambient capabilities to let systemd drop all privs while retaining compatibility with systems that lack ambient caps, where priv dropping is the left to the daemon codes themselves. This is an alternative approach to systemd#6564 and related PRs.

yuwata added 6 commits August 8, 2017 12:29

capability-util: add ambient_capability_is_supported()

43465b5

core: add '-' prefix to AmbientCapabilities=

7995b52

If AmbientCapabilities= is prefixed by '-' and the ambient capability is not supported by the kernel, then the listed ambient capabilities are ignored and the commands are executed by root.

capability-util: add get_effective_caps()

a0d4d48

shared: add check_privileges()

11f0965

networkd: support to run by non-root user

5b7189b

resolved: support to run by non-root user

f568a74

Siosm mentioned this pull request Aug 8, 2017

Launch resolved & networkd unprivileged using RuntimeDirectory & Ambient capabilities #6393

Closed

timesyncd: support to run by non-root user

0815410

Also this moves the location of clock file to /var/lib/systemd/timesync.

yuwata force-pushed the ambient-prefix branch from 3c37d50 to 0815410 Compare August 9, 2017 06:52

Siosm reviewed Aug 9, 2017

View reviewed changes

poettering mentioned this pull request Aug 9, 2017

add ! and !! ExecStart= flags to make ambient caps useful #6577

Merged

yuwata closed this Aug 12, 2017

yuwata deleted the ambient-prefix branch November 12, 2017 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

core: add '-' prefix to AmbientCapabilities= #6564

core: add '-' prefix to AmbientCapabilities= #6564

Uh oh!

yuwata commented Aug 8, 2017 •

edited

Loading

Uh oh!

Siosm commented Aug 8, 2017

Uh oh!

yuwata commented Aug 9, 2017

Uh oh!

yuwata commented Aug 9, 2017

Uh oh!

Siosm Aug 9, 2017

Uh oh!

Siosm Aug 9, 2017

Uh oh!

yuwata Aug 9, 2017

Uh oh!

Siosm Aug 9, 2017

Uh oh!

Siosm commented Aug 9, 2017

Uh oh!

poettering commented Aug 9, 2017

Uh oh!

yuwata commented Aug 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants


		* clock file is now moved to /var/lib/systemd/timesync/clock.
		Distributors should change the location of the file if it is packaged.

Uh oh!

core: add '-' prefix to AmbientCapabilities= #6564

core: add '-' prefix to AmbientCapabilities= #6564

Uh oh!

Conversation

yuwata commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siosm commented Aug 8, 2017

Uh oh!

yuwata commented Aug 9, 2017

Uh oh!

yuwata commented Aug 9, 2017

Uh oh!

Siosm Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

Siosm Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

yuwata Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

Siosm Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

Siosm commented Aug 9, 2017

Uh oh!

poettering commented Aug 9, 2017

Uh oh!

yuwata commented Aug 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

yuwata commented Aug 8, 2017 •

edited

Loading