From 89be857a41c4cfb5e701ba89b9c7a644d678894b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Sep 2018 09:56:36 +0200 Subject: [PATCH 1/6] Revert "unit: drop After=systemd-sysusers.service from timesyncd" This reverts commit be80154827100b19b6cc79a59323791b4f1a409f. --- units/systemd-timesyncd.service.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/units/systemd-timesyncd.service.in b/units/systemd-timesyncd.service.in index 4a490b6e16..7478906ae5 100644 --- a/units/systemd-timesyncd.service.in +++ b/units/systemd-timesyncd.service.in @@ -13,7 +13,7 @@ Documentation=man:systemd-timesyncd.service(8) ConditionCapability=CAP_SYS_TIME ConditionVirtualization=!container DefaultDependencies=no -After=systemd-remount-fs.service +After=systemd-remount-fs.service systemd-sysusers.service Before=time-sync.target sysinit.target shutdown.target Conflicts=shutdown.target Wants=time-sync.target From 162e0b75f9c9f698f94c228c2f9148120f03e9a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Sep 2018 10:00:09 +0200 Subject: [PATCH 2/6] Revert "timesyncd: enable DynamicUser=" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 48d3e88c18258d423c3953372ec4a2e638ab0422. I kept the follow-symlink=false → follow-symlink=true change instact, since we're likely to have existing installations with a symlink now. --- units/systemd-timesyncd.service.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/units/systemd-timesyncd.service.in b/units/systemd-timesyncd.service.in index 7478906ae5..12f918dd11 100644 --- a/units/systemd-timesyncd.service.in +++ b/units/systemd-timesyncd.service.in @@ -25,10 +25,11 @@ RestartSec=0 ExecStart=!!@rootlibexecdir@/systemd-timesyncd WatchdogSec=3min User=systemd-timesync -DynamicUser=yes CapabilityBoundingSet=CAP_SYS_TIME AmbientCapabilities=CAP_SYS_TIME +PrivateTmp=yes PrivateDevices=yes +ProtectSystem=strict ProtectHome=yes ProtectControlGroups=yes ProtectKernelTunables=yes From 62fb7e80fcc45a1530ed58a84980be8cfafa9b3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Sep 2018 10:04:33 +0200 Subject: [PATCH 3/6] Revert "resolve: enable DynamicUser= for systemd-resolved.service" This reverts commit 0187368cadea183e18c6d575a9d6b7f491a402af. (systemd.conf.m4 part was already reverted in 5b5d82615011b9827466b7cd5756da35627a1608.) --- src/resolve/resolved-bus.c | 2 +- test/networkd-test.py | 1 + units/systemd-resolved.service.in | 5 +++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 3859d41029..75702d593f 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -1920,7 +1920,7 @@ int manager_connect_bus(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to register dnssd enumerator: %m"); - r = bus_request_name_async_may_reload_dbus(m->bus, NULL, "org.freedesktop.resolve1", 0, NULL); + r = sd_bus_request_name_async(m->bus, NULL, "org.freedesktop.resolve1", 0, NULL, NULL); if (r < 0) return log_error_errno(r, "Failed to request name: %m"); diff --git a/test/networkd-test.py b/test/networkd-test.py index 131b48f611..79d6250cdc 100755 --- a/test/networkd-test.py +++ b/test/networkd-test.py @@ -67,6 +67,7 @@ def setUpModule(): tmpmounts.append(d) if os.path.isdir('/run/systemd/resolve'): os.chmod('/run/systemd/resolve', 0o755) + shutil.chown('/run/systemd/resolve', 'systemd-resolve', 'systemd-resolve') # Avoid "Failed to open /dev/tty" errors in containers. os.environ['SYSTEMD_LOG_TARGET'] = 'journal' diff --git a/units/systemd-resolved.service.in b/units/systemd-resolved.service.in index 9982ecebff..ef5398cbf0 100644 --- a/units/systemd-resolved.service.in +++ b/units/systemd-resolved.service.in @@ -14,7 +14,7 @@ Documentation=https://www.freedesktop.org/wiki/Software/systemd/resolved Documentation=https://www.freedesktop.org/wiki/Software/systemd/writing-network-configuration-managers Documentation=https://www.freedesktop.org/wiki/Software/systemd/writing-resolver-clients DefaultDependencies=no -After=systemd-networkd.service +After=systemd-sysusers.service systemd-networkd.service Before=network.target nss-lookup.target shutdown.target Conflicts=shutdown.target Wants=nss-lookup.target @@ -26,10 +26,11 @@ RestartSec=0 ExecStart=!!@rootlibexecdir@/systemd-resolved WatchdogSec=3min User=systemd-resolve -DynamicUser=yes CapabilityBoundingSet=CAP_SETPCAP CAP_NET_RAW CAP_NET_BIND_SERVICE AmbientCapabilities=CAP_SETPCAP CAP_NET_RAW CAP_NET_BIND_SERVICE +PrivateTmp=yes PrivateDevices=yes +ProtectSystem=strict ProtectHome=yes ProtectControlGroups=yes ProtectKernelTunables=yes From 696fc8360df7c3acfa1ad4cebd66bcdfe5a9d575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Sep 2018 10:06:09 +0200 Subject: [PATCH 4/6] Revert "network: set DynamicUser= to systemd-networkd.service" This reverts commit d4e9e574ea0b5d23598a317e68399584d229568b. (systemd.conf.m4 part was already reverted in 5b5d82615011b9827466b7cd5756da35627a1608.) Together those reverts should "fix" #10025 and #10011. ("fix" is in quotes because this doesn't really fix the underlying issue, which is combining DynamicUser= with strict container sandbox, but it avoids the problem by not using that feature in our default installation.) Dynamic users don't work well if the service requires matching configuration in other places, for example dbus policy. This is true for those three services. In effect, distros create the user statically [1, 2]. Dynamic users make more sense for "add-on" services where not creating the user, or more precisely, creating the user lazily, can save resources. For "basic" services, if we are going to create the user on package installation anyway, setting DynamicUser= just creates unneeded confusion. The only case where it is actually used is when somebody forgets to do system configuration. But it's better to have the service fail cleanly in this case too. If we want to turn on some side-effect of DynamicUser=yes for those services, we should just do that directly through fine-grained options. By not using DynamicUser= we also avoid the need to restart dbus. [1] https://salsa.debian.org/systemd-team/systemd/commit/bd9bf307274faca24699c0c2d67cb86f18c0b2cb [2] https://src.fedoraproject.org/rpms/systemd/blob/48ac1cebdedb055d9daf3dfe28c7bde80103f7a1/f/systemd.spec#_473 (Fedora does not create systemd-timesync user.) --- src/network/networkd-manager.c | 2 +- units/systemd-networkd.service.in | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 27a4f6db61..e17ee2a133 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -151,7 +151,7 @@ int manager_connect_bus(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to add network enumerator: %m"); - r = bus_request_name_async_may_reload_dbus(m->bus, NULL, "org.freedesktop.network1", 0, NULL); + r = sd_bus_request_name_async(m->bus, NULL, "org.freedesktop.network1", 0, NULL, NULL); if (r < 0) return log_error_errno(r, "Failed to request name: %m"); diff --git a/units/systemd-networkd.service.in b/units/systemd-networkd.service.in index 371ab3a9cf..65d3e2a660 100644 --- a/units/systemd-networkd.service.in +++ b/units/systemd-networkd.service.in @@ -13,7 +13,7 @@ Documentation=man:systemd-networkd.service(8) ConditionCapability=CAP_NET_ADMIN DefaultDependencies=no # systemd-udevd.service can be dropped once tuntap is moved to netlink -After=systemd-udevd.service network-pre.target systemd-sysctl.service +After=systemd-udevd.service network-pre.target systemd-sysusers.service systemd-sysctl.service Before=network.target multi-user.target shutdown.target Conflicts=shutdown.target Wants=network.target @@ -25,9 +25,9 @@ RestartSec=0 ExecStart=!!@rootlibexecdir@/systemd-networkd WatchdogSec=3min User=systemd-network -DynamicUser=yes CapabilityBoundingSet=CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_BROADCAST CAP_NET_RAW AmbientCapabilities=CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_BROADCAST CAP_NET_RAW +ProtectSystem=strict ProtectHome=yes ProtectControlGroups=yes ProtectKernelModules=yes From b8b20f5a6a71a7796377d52d05812cc251d30c7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Sep 2018 10:12:02 +0200 Subject: [PATCH 5/6] timesyncd: no need to reload dbus now --- src/timesync/timesyncd-bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/timesync/timesyncd-bus.c b/src/timesync/timesyncd-bus.c index f2527f52e4..c945c3bf7b 100644 --- a/src/timesync/timesyncd-bus.c +++ b/src/timesync/timesyncd-bus.c @@ -185,7 +185,7 @@ int manager_connect_bus(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to add manager object vtable: %m"); - r = bus_request_name_async_may_reload_dbus(m->bus, NULL, "org.freedesktop.timesync1", 0, NULL); + r = sd_bus_request_name_async(m->bus, NULL, "org.freedesktop.timesync1", 0, NULL, NULL); if (r < 0) return log_error_errno(r, "Failed to request name: %m"); From 2e7e76af99548aa52ea378a4475c0be44763435a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 19 Sep 2018 10:15:26 +0200 Subject: [PATCH 6/6] bus-util: drop now-unused functions --- src/shared/bus-util.c | 162 --------------------------------------- src/shared/bus-util.h | 2 - src/test/test-bus-util.c | 34 -------- 3 files changed, 198 deletions(-) diff --git a/src/shared/bus-util.c b/src/shared/bus-util.c index ddc8364511..808df9b154 100644 --- a/src/shared/bus-util.c +++ b/src/shared/bus-util.c @@ -1761,168 +1761,6 @@ int bus_open_system_watch_bind_with_description(sd_bus **ret, const char *descri return 0; } -struct request_name_data { - unsigned n_ref; - - const char *name; - uint64_t flags; - void *userdata; -}; - -static void request_name_destroy_callback(void *userdata) { - struct request_name_data *data = userdata; - - assert(data); - assert(data->n_ref > 0); - - log_debug("%s n_ref=%u", __func__, data->n_ref); - - data->n_ref--; - if (data->n_ref == 0) - free(data); -} - -static int reload_dbus_handler(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) { - struct request_name_data *data = userdata; - const sd_bus_error *e; - int r; - - assert(data); - assert(data->name); - assert(data->n_ref > 0); - - e = sd_bus_message_get_error(m); - if (e) { - log_error_errno(sd_bus_error_get_errno(e), "Failed to reload DBus configuration: %s", e->message); - return 1; - } - - /* Here, use the default request name handler to avoid an infinite loop of reloading and requesting. */ - r = sd_bus_request_name_async(sd_bus_message_get_bus(m), NULL, data->name, data->flags, NULL, data->userdata); - if (r < 0) - log_error_errno(r, "Failed to request name: %m"); - - return 1; -} - -static int request_name_handler_may_reload_dbus(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) { - struct request_name_data *data = userdata; - uint32_t ret; - int r; - - assert(m); - assert(data); - - if (sd_bus_message_is_method_error(m, NULL)) { - const sd_bus_error *e = sd_bus_message_get_error(m); - _cleanup_(sd_bus_slot_unrefp) sd_bus_slot *slot = NULL; - - if (!sd_bus_error_has_name(e, SD_BUS_ERROR_ACCESS_DENIED)) { - log_debug_errno(sd_bus_error_get_errno(e), - "Unable to request name, failing connection: %s", - e->message); - - bus_enter_closing(sd_bus_message_get_bus(m)); - return 1; - } - - log_debug_errno(sd_bus_error_get_errno(e), - "Unable to request name, will retry after reloading DBus configuration: %s", - e->message); - - /* If a service enables DynamicUser= and dbus.service started before the dynamic user is realized, - * then the DBus policy about the service has not been enabled yet. So, let's try to reload DBus - * configuration, and after that request the name again. Note that it seems that no privileges are - * necessary to call the following method. */ - - r = sd_bus_call_method_async( - sd_bus_message_get_bus(m), - &slot, - "org.freedesktop.DBus", - "/org/freedesktop/DBus", - "org.freedesktop.DBus", - "ReloadConfig", - reload_dbus_handler, - data, NULL); - if (r < 0) { - log_error_errno(r, "Failed to reload DBus configuration: %m"); - bus_enter_closing(sd_bus_message_get_bus(m)); - return 1; - } - - data->n_ref ++; - assert_se(sd_bus_slot_set_destroy_callback(slot, request_name_destroy_callback) >= 0); - - r = sd_bus_slot_set_floating(slot, true); - if (r < 0) - return r; - - return 1; - } - - r = sd_bus_message_read(m, "u", &ret); - if (r < 0) - return r; - - switch (ret) { - - case BUS_NAME_ALREADY_OWNER: - log_debug("Already owner of requested service name, ignoring."); - return 1; - - case BUS_NAME_IN_QUEUE: - log_debug("In queue for requested service name."); - return 1; - - case BUS_NAME_PRIMARY_OWNER: - log_debug("Successfully acquired requested service name."); - return 1; - - case BUS_NAME_EXISTS: - log_debug("Requested service name already owned, failing connection."); - bus_enter_closing(sd_bus_message_get_bus(m)); - return 1; - } - - log_debug("Unexpected response from RequestName(), failing connection."); - bus_enter_closing(sd_bus_message_get_bus(m)); - return 1; -} - -int bus_request_name_async_may_reload_dbus(sd_bus *bus, sd_bus_slot **ret_slot, const char *name, uint64_t flags, void *userdata) { - _cleanup_free_ struct request_name_data *data = NULL; - _cleanup_(sd_bus_slot_unrefp) sd_bus_slot *slot = NULL; - int r; - - data = new(struct request_name_data, 1); - if (!data) - return -ENOMEM; - - *data = (struct request_name_data) { - .n_ref = 1, - .name = name, - .flags = flags, - .userdata = userdata, - }; - - r = sd_bus_request_name_async(bus, &slot, name, flags, request_name_handler_may_reload_dbus, data); - if (r < 0) - return r; - - assert_se(sd_bus_slot_set_destroy_callback(slot, request_name_destroy_callback) >= 0); - TAKE_PTR(data); - - if (ret_slot) - *ret_slot = TAKE_PTR(slot); - else { - r = sd_bus_slot_set_floating(slot, true); - if (r < 0) - return r; - } - - return 0; -} - int bus_reply_pair_array(sd_bus_message *m, char **l) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; char **k, **v; diff --git a/src/shared/bus-util.h b/src/shared/bus-util.h index 80f4606ba4..3d3cdecb43 100644 --- a/src/shared/bus-util.h +++ b/src/shared/bus-util.h @@ -177,6 +177,4 @@ static inline int bus_open_system_watch_bind(sd_bus **ret) { return bus_open_system_watch_bind_with_description(ret, NULL); } -int bus_request_name_async_may_reload_dbus(sd_bus *bus, sd_bus_slot **ret_slot, const char *name, uint64_t flags, void *userdata); - int bus_reply_pair_array(sd_bus_message *m, char **l); diff --git a/src/test/test-bus-util.c b/src/test/test-bus-util.c index 789d19cf7f..a536608ce0 100644 --- a/src/test/test-bus-util.c +++ b/src/test/test-bus-util.c @@ -4,38 +4,6 @@ #include "log.h" #include "tests.h" -static void test_name_async(unsigned n_messages) { - _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; - int r; - unsigned i; - - log_info("/* %s (%u) */", __func__, n_messages); - - r = bus_open_system_watch_bind_with_description(&bus, "test-bus"); - if (r < 0) { - log_error_errno(r, "Failed to connect to bus: %m"); - return; - } - - r = bus_request_name_async_may_reload_dbus(bus, NULL, "org.freedesktop.systemd.test-bus-util", 0, NULL); - if (r < 0) { - log_error_errno(r, "Failed to request name: %m"); - return; - } - - for (i = 0; i < n_messages; i++) { - r = sd_bus_process(bus, NULL); - log_debug("stage %u: sd_bus_process returned %d", i, r); - if (r < 0) { - log_notice_errno(r, "Processing failed: %m"); - return; - } - - if (r > 0 && i + 1 < n_messages) - (void) sd_bus_wait(bus, USEC_PER_SEC / 3); - } -} - static int callback(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) { return 1; } @@ -81,8 +49,6 @@ static void test_destroy_callback(void) { int main(int argc, char **argv) { test_setup_logging(LOG_DEBUG); - test_name_async(0); - test_name_async(20); test_destroy_callback(); return 0;