From fc47bea69b63addb3250e3b2865488cf0d1a7835 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sun, 17 Sep 2017 14:14:33 +0100 Subject: [PATCH 01/10] man/sd_bus_creds_get_pid.xml: fix typo Replace references to non-existent functions sd_bus_get_pid() etc (missing _cred). --- man/sd_bus_creds_get_pid.xml | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/man/sd_bus_creds_get_pid.xml b/man/sd_bus_creds_get_pid.xml index 9e68d5e8c7..6ea95e0665 100644 --- a/man/sd_bus_creds_get_pid.xml +++ b/man/sd_bus_creds_get_pid.xml @@ -500,12 +500,11 @@ The given field is not specified for the described process or peer. This will be returned by - sd_bus_get_unit(), - sd_bus_get_slice(), - sd_bus_get_user_unit(), - sd_bus_get_user_slice(), - sd_bus_get_session(), and - sd_bus_get_owner_uid() if the process is + sd_bus_creds_get_unit(), + sd_bus_creds_get_slice(), + sd_bus_creds_get_user_unit(), + sd_bus_creds_get_user_slice(), and + sd_bus_creds_get_session() if the process is not part of a systemd system unit, systemd user unit, systemd slice, or logind session. It will also be returned by sd_bus_creds_get_exe() and From 1c97e2ebf4981ada50ce8fd167d6521ba4663c7c Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sun, 17 Sep 2017 14:11:20 +0100 Subject: [PATCH 02/10] man: de-emphasize *_get_session() Explanation: "Please note the login session may be limited to a stub process or two. User processes may instead be started from their systemd user manager, e.g. GUI applications started using DBus activation, as well as service processes which are shared between multiple logins of the same user." The most glaring example being when you run commands from gnome-terminal, or as you see nowadays, "gnome-terminal-server". *_get_session() is still currently used (directly or indirectly) by Xorg, Weston etc. running within the session scope. That setup is perfectly functional, although code will be more generally useful if it is able to run outside the session scope.[1] [1] https://wiki.archlinux.org/index.php/Systemd/User#Xorg_as_a_systemd_user_service Re-order the man pages a bit at the same time. This is to avoid having the first and titular entry introduce the session concept, and then immediately try and persuade you not to use it :). --- man/rules/meson.build | 4 +- man/sd_bus_creds_get_pid.xml | 21 ++-- ...t_session.xml => sd_pid_get_owner_uid.xml} | 114 +++++++++--------- 3 files changed, 73 insertions(+), 66 deletions(-) rename man/{sd_pid_get_session.xml => sd_pid_get_owner_uid.xml} (85%) diff --git a/man/rules/meson.build b/man/rules/meson.build index ae0556058e..f74dbf9c77 100644 --- a/man/rules/meson.build +++ b/man/rules/meson.build @@ -441,7 +441,7 @@ manpages = [ '3', ['sd_notifyf', 'sd_pid_notify', 'sd_pid_notify_with_fds', 'sd_pid_notifyf'], ''], - ['sd_pid_get_session', + ['sd_pid_get_owner_uid', '3', ['sd_peer_get_cgroup', 'sd_peer_get_machine_name', @@ -453,7 +453,7 @@ manpages = [ 'sd_peer_get_user_unit', 'sd_pid_get_cgroup', 'sd_pid_get_machine_name', - 'sd_pid_get_owner_uid', + 'sd_pid_get_session', 'sd_pid_get_slice', 'sd_pid_get_unit', 'sd_pid_get_user_slice', diff --git a/man/sd_bus_creds_get_pid.xml b/man/sd_bus_creds_get_pid.xml index 6ea95e0665..56e62863f2 100644 --- a/man/sd_bus_creds_get_pid.xml +++ b/man/sd_bus_creds_get_pid.xml @@ -394,16 +394,19 @@ sd_bus_creds_get_session() will retrieve the identifier of the login session that the process is - a part of. See - systemd-logind.service8. For - processes that are not part of a session, returns -ENXIO. - + a part of. Please note the login session may be limited to a stub + process or two. User processes may instead be started from their + systemd user manager, e.g. GUI applications started using DBus + activation, as well as service processes which are shared between + multiple logins of the same user. For processes that are not part + of a session, returns -ENXIO. sd_bus_creds_get_owner_uid() will retrieve the numeric UID (user identifier) of the user who owns - the login session that the process is a part of. See + the user unit or login session that the process is a part of. See systemd-logind.service8. - For processes that are not part of a session, returns -ENXIO. + For processes that are not part of a user unit or session, returns + -ENXIO. sd_bus_creds_has_effective_cap() will check whether the capability specified by @@ -506,8 +509,10 @@ sd_bus_creds_get_user_slice(), and sd_bus_creds_get_session() if the process is not part of a systemd system unit, systemd user unit, systemd - slice, or logind session. It will also be returned by - sd_bus_creds_get_exe() and + slice, or logind session. It will be returned by + sd_bus_creds_get_owner_uid() if the process is + not part of a systemd user unit or logind session. It will also be + returned by sd_bus_creds_get_exe() and sd_bus_creds_get_cmdline() for kernel threads (since these are not started from an executable binary, nor have a command line), and by diff --git a/man/sd_pid_get_session.xml b/man/sd_pid_get_owner_uid.xml similarity index 85% rename from man/sd_pid_get_session.xml rename to man/sd_pid_get_owner_uid.xml index 14ebd53e36..5a2a07d096 100644 --- a/man/sd_pid_get_session.xml +++ b/man/sd_pid_get_owner_uid.xml @@ -21,10 +21,10 @@ along with systemd; If not, see . --> - + - sd_pid_get_session + sd_pid_get_owner_uid systemd @@ -38,30 +38,30 @@ - sd_pid_get_session + sd_pid_get_owner_uid 3 - sd_pid_get_session - sd_pid_get_unit - sd_pid_get_user_unit sd_pid_get_owner_uid + sd_pid_get_session + sd_pid_get_user_unit + sd_pid_get_unit sd_pid_get_machine_name sd_pid_get_slice sd_pid_get_user_slice sd_pid_get_cgroup - sd_peer_get_session - sd_peer_get_unit - sd_peer_get_user_unit sd_peer_get_owner_uid + sd_peer_get_session + sd_peer_get_user_unit + sd_peer_get_unit sd_peer_get_machine_name sd_peer_get_slice sd_peer_get_user_slice sd_peer_get_cgroup - Determine session, unit, owner of a session, - container/VM or slice of a specific PID or socket - peer + Determine the owner uid of the user unit or session, + or the session, user unit, system unit, container/VM or slice that + a specific PID or socket peer belongs to. @@ -69,15 +69,15 @@ #include <systemd/sd-login.h> - int sd_pid_get_session + int sd_pid_get_owner_uid pid_t pid - char **session + uid_t *uid - int sd_pid_get_unit + int sd_pid_get_session pid_t pid - char **unit + char **session @@ -87,9 +87,9 @@ - int sd_pid_get_owner_uid + int sd_pid_get_unit pid_t pid - uid_t *uid + char **unit @@ -117,15 +117,15 @@ - int sd_peer_get_session + int sd_peer_get_owner_uid int fd - char **session + uid_t *uid - int sd_peer_get_unit + int sd_peer_get_session int fd - char **unit + char **session @@ -135,9 +135,9 @@ - int sd_peer_get_owner_uid + int sd_peer_get_unit int fd - uid_t *uid + char **unit @@ -169,16 +169,34 @@ Description + sd_pid_get_owner_uid() may be used to + determine the Unix UID (user identifier) which owns the login + session or systemd user unit of a process identified by the + specified PID. For processes which are not part of a login session + and not managed by a user manager, this function will fail with + -ENODATA. + sd_pid_get_session() may be used to determine the login session identifier of a process identified by the specified process identifier. The session identifier is a - short string, suitable for usage in file system paths. Note that - not all processes are part of a login session (e.g. system service - processes, user processes that are shared between multiple - sessions of the same user, or kernel threads). For processes not - being part of a login session, this function will fail with - -ENODATA. The returned string needs to be freed with the libc - -ENODATA. + The returned string needs to be freed with the libc free3 + call after use. + + sd_pid_get_user_unit() may be used to + determine the systemd user unit (i.e. user service or scope unit) + identifier of a process identified by the specified PID. The + unit name is a short string, suitable for usage in file system + paths. For processes which are not managed by a user manager, this + function will fail with -ENODATA. The + returned string needs to be freed with the libc free3 call after use. @@ -186,37 +204,21 @@ determine the systemd system unit (i.e. system service or scope unit) identifier of a process identified by the specified PID. The unit name is a short string, suitable for usage in file system - paths. Note that not all processes are part of a system - unit/service (e.g. user processes, or kernel threads). For - processes not being part of a systemd system unit, this function - will fail with -ENODATA. (More specifically, this call will not - work for kernel threads.) The returned string needs to be freed - with the libc -ENODATA. + (More specifically, this call will not work for kernel threads.) + The returned string needs to be freed with the libc free3 call after use. - sd_pid_get_user_unit() may be used to - determine the systemd user unit (i.e. user service or scope unit) - identifier of a process identified by the specified PID. This is - similar to sd_pid_get_unit(), but applies to - user units instead of system units. - - sd_pid_get_owner_uid() may be used to - determine the Unix UID (user identifier) of the owner of the - session of a process identified the specified PID. Note that this - function will succeed for user processes which are shared between - multiple login sessions of the same user, whereas - sd_pid_get_session() will fail. For processes - not being part of a login session and not being a shared process - of a user, this function will fail with -ENODATA. - sd_pid_get_machine_name() may be used to determine the name of the VM or container is a member of. The machine name is a short string, suitable for usage in file system paths. The returned string needs to be freed with the libc free3 - call after use. For processes not part of a VM or containers, this + call after use. For processes not part of a VM or container, this function fails with -ENODATA. sd_pid_get_slice() may be used to @@ -246,10 +248,10 @@ functions is passed as 0, the operation is executed for the calling process. - The sd_peer_get_session(), - sd_peer_get_unit(), + The sd_peer_get_owner_uid(), + sd_peer_get_session(), sd_peer_get_user_unit(), - sd_peer_get_owner_uid(), + sd_peer_get_unit(), sd_peer_get_machine_name(), sd_peer_get_slice(), sd_peer_get_user_slice() and From fa93fe9c1530656222c9b9a216e1e75ad5c6b469 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sun, 17 Sep 2017 14:51:49 +0100 Subject: [PATCH 03/10] man: de-emphasize journal field _SYSTEMD_SESSION See previous commit message. --- man/systemd.journal-fields.xml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/man/systemd.journal-fields.xml b/man/systemd.journal-fields.xml index e488affe3e..cab356c5e7 100644 --- a/man/systemd.journal-fields.xml +++ b/man/systemd.journal-fields.xml @@ -193,18 +193,19 @@ _SYSTEMD_CGROUP= - _SYSTEMD_SESSION= + _SYSTEMD_SLICE= _SYSTEMD_UNIT= _SYSTEMD_USER_UNIT= + _SYSTEMD_SESSION= _SYSTEMD_OWNER_UID= - _SYSTEMD_SLICE= The control group path in the systemd hierarchy, the - systemd session ID (if any), the systemd unit name (if any), - the systemd user session unit name (if any), the owner UID - of the systemd session (if any) and the systemd slice unit - of the process the journal entry originates from. + the systemd slice unit name, the systemd unit name, the + unit name in the systemd user manager (if any), the systemd + session ID (if any), and the owner UID of the systemd user + unit or systemd session (if any) of the process the journal + entry originates from. From 8d9c2bca41b03fd113edd2fe66dd88c3894debd3 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sun, 17 Sep 2017 14:53:14 +0100 Subject: [PATCH 04/10] nspawn: comment to acknowledge lying about "user session" --- src/nspawn/nspawn.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 4e3803be82..991bf5e9c7 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1120,6 +1120,8 @@ static int parse_argv(int argc, char *argv[]) { arg_userns_chown = true; if (arg_keep_unit && arg_register && cg_pid_get_owner_uid(0, NULL) >= 0) { + /* Save the user from accidentally registering either user-$SESSION.scope or user@.service. + * The latter is not technically a user session, but we don't need to labour the point. */ log_error("--keep-unit --register=yes may not be used when invoked from a user session."); return -EINVAL; } From aeb075704b5e2ae4757da69ca8d6eda0235d327d Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sat, 7 Oct 2017 12:24:02 +0100 Subject: [PATCH 05/10] logind: remove an obscure dbus error from GetSessionByPID(0) and friends GetSessionByPID(0) can fail with NO_SESSION_FOR_PID. More obscurely, if the session is abandoned, it can return NO_SUCH_SESSION. It is not clear that the latter was intended. The message associated with the former, hints that this was overlooked. We don't have a document enumerating the errors. Any specific error-handling in client code, e.g. translated messages, would also be liable to overlook the more obscure error code. I can't see any equivalent condition for GetUserByPID(0). On the other hand, the code did not return NO_USER_FOR_PID where it probably should. The relevant code is right next to that for GetSessionByPID(0), so it will be simpler to understand if both follow the same pattern. --- src/login/logind-dbus.c | 87 +++++++++++++++++++++++++++++------------ 1 file changed, 61 insertions(+), 26 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index f62a02c4d1..1350452d7e 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -50,27 +50,44 @@ #include "user-util.h" #include "utmp-wtmp.h" -int manager_get_session_from_creds(Manager *m, sd_bus_message *message, const char *name, sd_bus_error *error, Session **ret) { +static int get_sender_session(Manager *m, sd_bus_message *message, sd_bus_error *error, Session **ret) { + _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; + const char *name; Session *session; int r; + r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_SESSION|SD_BUS_CREDS_AUGMENT, &creds); + if (r < 0) + return r; + + r = sd_bus_creds_get_session(creds, &name); + if (r == -ENXIO) + goto err_no_session; + if (r < 0) + return r; + + session = hashmap_get(m->sessions, name); + if (!session) + goto err_no_session; + + *ret = session; + return 0; + +err_no_session: + return sd_bus_error_setf(error, BUS_ERROR_NO_SESSION_FOR_PID, + "Caller does not belong to any known session"); +} + +int manager_get_session_from_creds(Manager *m, sd_bus_message *message, const char *name, sd_bus_error *error, Session **ret) { + Session *session; + assert(m); assert(message); assert(ret); - if (isempty(name)) { - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_SESSION|SD_BUS_CREDS_AUGMENT, &creds); - if (r < 0) - return r; - - r = sd_bus_creds_get_session(creds, &name); - if (r == -ENXIO) - return sd_bus_error_setf(error, BUS_ERROR_NO_SESSION_FOR_PID, - "Caller does not belong to any known session"); - if (r < 0) - return r; - } + if (isempty(name)) + return get_sender_session(m, message, error, ret); session = hashmap_get(m->sessions, name); if (!session) @@ -80,26 +97,44 @@ int manager_get_session_from_creds(Manager *m, sd_bus_message *message, const ch return 0; } -int manager_get_user_from_creds(Manager *m, sd_bus_message *message, uid_t uid, sd_bus_error *error, User **ret) { +static int get_sender_user(Manager *m, sd_bus_message *message, sd_bus_error *error, User **ret) { + + _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; + uid_t uid; User *user; int r; + /* Note that we get the owner UID of the session, not the actual client UID here! */ + r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_OWNER_UID|SD_BUS_CREDS_AUGMENT, &creds); + if (r < 0) + return r; + + r = sd_bus_creds_get_owner_uid(creds, &uid); + if (r == -ENXIO) + goto err_no_user; + if (r < 0) + return r; + + user = hashmap_get(m->users, UID_TO_PTR(uid)); + if (!user) + goto err_no_user; + + *ret = user; + return 0; + +err_no_user: + return sd_bus_error_setf(error, BUS_ERROR_NO_USER_FOR_PID, "Caller does not belong to any known or logged in user"); +} + +int manager_get_user_from_creds(Manager *m, sd_bus_message *message, uid_t uid, sd_bus_error *error, User **ret) { + User *user; + assert(m); assert(message); assert(ret); - if (!uid_is_valid(uid)) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - - /* Note that we get the owner UID of the session, not the actual client UID here! */ - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_OWNER_UID|SD_BUS_CREDS_AUGMENT, &creds); - if (r < 0) - return r; - - r = sd_bus_creds_get_owner_uid(creds, &uid); - if (r < 0) - return r; - } + if (!uid_is_valid(uid)) + return get_sender_user(m, message, error, ret); user = hashmap_get(m->users, UID_TO_PTR(uid)); if (!user) From e4d2984bf8514ab576a66d5ac1f1cde746bb32a3 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sat, 14 Oct 2017 09:25:56 +0100 Subject: [PATCH 06/10] logind: "self" objects which do not apply - return specific error messages It's confusing that the bus API has aliases like "session/self" that return an error based on ENXIO, when it also has methods that return e.g. NO_SESSION_FOR_PID for the same problem. The latter kind of error includes more specifically helpful messages. "user/self" is the odd one out; it returns a generic UnknownObject error when it is not applicable to the caller. It's not clear whether this was intentional, but at first I thought it was more correct. More specifically, user_object_find() was returning 0 for "user/self", in the same situations (more or less) where user_node_enumerator() was omitting "user/self". I thought that was a good idea, because returning e.g. -ENXIO instead suggested that there _is_ something specific on that path. And it could be confused with errors of the method being called. Therefore I suggested changing the enumerator, always admitting that there is a handler for the path "foo/self", but returning a specific error when queried. However this interacts poorly with tools like D-Feet or `busctl`. In either tool, looking at logind would show an error message, and then go on to omit "user/self" in the normal listing. These tools are very useful, so we don't want to interfere with them. I think we can change the error codes without causing problems. The self objects were not listed in the documentation. They have been suggested to other projects - but without reference to error reporting. "seat/self" is used by various Wayland compositors for VT switching, but they don't appear to reference specific errors. We _could_ insist on the link between enumeration and UnknownObject, and standardize on that as the error for the aliases. But I'm not aware of any practical complaints, that we returned an error from an object that didn't exist. Instead, let's unify the codepaths for "user/self" vs GetUserByPid(0) etc. We will return the most helpful error message we can think of, if the object does not exist. E.g. for "session/self", we might return an error that the caller does not belong to a session. If one of the compositors is ever simplified to use "session/self" in initialization, users would be able to trigger such errors (e.g. run `gnome-shell` inside gnome-terminal). The message text will most likely be logged. The user might not know what the "session" is, but at least we'll be pointing towards the right questions. I think it should also be clearer for development / debugging. Unifying the code paths is also slightly helpful for auditing / marking calls to sd_bus_creds_get_session() in subsequent commits. --- src/login/logind-seat-dbus.c | 20 +++----------------- src/login/logind-session-dbus.c | 15 +++------------ src/login/logind-user-dbus.c | 5 +---- 3 files changed, 7 insertions(+), 33 deletions(-) diff --git a/src/login/logind-seat-dbus.c b/src/login/logind-seat-dbus.c index f934a5326a..2154b878dd 100644 --- a/src/login/logind-seat-dbus.c +++ b/src/login/logind-seat-dbus.c @@ -332,28 +332,15 @@ int seat_object_find(sd_bus *bus, const char *path, const char *interface, void assert(m); if (streq(path, "/org/freedesktop/login1/seat/self")) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; sd_bus_message *message; - Session *session; - const char *name; message = sd_bus_get_current_message(bus); if (!message) return 0; - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_SESSION|SD_BUS_CREDS_AUGMENT, &creds); + r = manager_get_seat_from_creds(m, message, NULL, error, &seat); if (r < 0) return r; - - r = sd_bus_creds_get_session(creds, &name); - if (r < 0) - return r; - - session = hashmap_get(m->sessions, name); - if (!session) - return 0; - - seat = session->seat; } else { _cleanup_free_ char *e = NULL; const char *p; @@ -367,11 +354,10 @@ int seat_object_find(sd_bus *bus, const char *path, const char *interface, void return -ENOMEM; seat = hashmap_get(m->seats, e); + if (!seat) + return 0; } - if (!seat) - return 0; - *found = seat; return 1; } diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c index 649f3c155e..09ecb49d33 100644 --- a/src/login/logind-session-dbus.c +++ b/src/login/logind-session-dbus.c @@ -580,23 +580,15 @@ int session_object_find(sd_bus *bus, const char *path, const char *interface, vo assert(m); if (streq(path, "/org/freedesktop/login1/session/self")) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; sd_bus_message *message; - const char *name; message = sd_bus_get_current_message(bus); if (!message) return 0; - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_SESSION|SD_BUS_CREDS_AUGMENT, &creds); + r = manager_get_session_from_creds(m, message, NULL, error, &session); if (r < 0) return r; - - r = sd_bus_creds_get_session(creds, &name); - if (r < 0) - return r; - - session = hashmap_get(m->sessions, name); } else { _cleanup_free_ char *e = NULL; const char *p; @@ -610,11 +602,10 @@ int session_object_find(sd_bus *bus, const char *path, const char *interface, vo return -ENOMEM; session = hashmap_get(m->sessions, e); + if (!session) + return 0; } - if (!session) - return 0; - *found = session; return 1; } diff --git a/src/login/logind-user-dbus.c b/src/login/logind-user-dbus.c index 987c63014f..921f1e0f91 100644 --- a/src/login/logind-user-dbus.c +++ b/src/login/logind-user-dbus.c @@ -270,18 +270,15 @@ int user_object_find(sd_bus *bus, const char *path, const char *interface, void assert(m); if (streq(path, "/org/freedesktop/login1/user/self")) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; sd_bus_message *message; message = sd_bus_get_current_message(bus); if (!message) return 0; - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_OWNER_UID|SD_BUS_CREDS_AUGMENT, &creds); + r = manager_get_user_from_creds(m, message, UID_INVALID, error, &user); if (r < 0) return r; - - r = sd_bus_creds_get_owner_uid(creds, &uid); } else { const char *p; From 095b8833d65bcda44b02ab72cd7ae9a1bf2a2572 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 17 Oct 2017 16:03:58 +0100 Subject: [PATCH 07/10] logind: more specific error message for unknown users If you try to run `loginctl user-status` on a non-logged in user to see whether "Linger" is enabled, it doesn't work. If you're already an expert in logind, the fact that the user is considered unknown actually tells you the user is not lingering. So, probably they they do not have lingering enabled. I think we can point towards this without being misleading. I also reword it because I thought it was slightly confusing to run `loginctl user-status root` and get an error back about "User 0". Try to be more specific, that it is "User ID 0". --- src/login/logind-dbus.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 1350452d7e..5b5fdf443b 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -123,7 +123,7 @@ static int get_sender_user(Manager *m, sd_bus_message *message, sd_bus_error *er return 0; err_no_user: - return sd_bus_error_setf(error, BUS_ERROR_NO_USER_FOR_PID, "Caller does not belong to any known or logged in user"); + return sd_bus_error_setf(error, BUS_ERROR_NO_USER_FOR_PID, "Caller does not belong to any logged in user or lingering user"); } int manager_get_user_from_creds(Manager *m, sd_bus_message *message, uid_t uid, sd_bus_error *error, User **ret) { @@ -138,7 +138,7 @@ int manager_get_user_from_creds(Manager *m, sd_bus_message *message, uid_t uid, user = hashmap_get(m->users, UID_TO_PTR(uid)); if (!user) - return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_USER, "No user "UID_FMT" known or logged in", uid); + return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_USER, "User ID "UID_FMT" is not logged in or lingering", uid); *ret = user; return 0; @@ -454,7 +454,9 @@ static int method_get_user_by_pid(sd_bus_message *message, void *userdata, sd_bu if (r < 0) return r; if (!user) - return sd_bus_error_setf(error, BUS_ERROR_NO_USER_FOR_PID, "PID "PID_FMT" does not belong to any known or logged in user", pid); + return sd_bus_error_setf(error, BUS_ERROR_NO_USER_FOR_PID, + "PID "PID_FMT" does not belong to any logged in user or lingering user", + pid); } p = user_bus_path(user); From 7b33c6228ff26b0bdb8ca5acf50b758c50cc9c6d Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Sun, 17 Sep 2017 15:49:12 +0100 Subject: [PATCH 08/10] logind: comment use of *_get_session() The manpages tell that such calls have quite limited meaning. logind has a few in the implementation of what remains of the session concept. At the same time, logind basically exposes sd_pid_get_session() as public API. This is absolutely required, to retain compatability e.g. with Xorg. But client code will work in more situations if it avoids assuming that it runs in a session itself. Its use inside the login session could be replaced with $XDG_SESSION_ID (which pam_systemd sets). I don't know whether it would be useful to change Xorg at this point or not. But if you were building something new, you would think about whether you want to support running it in a systemd service. Comment these logind API features, acknowledging the reason they exist is based in history. I.e. help readers avoid drawing implications from their existence which apply to history, but not the current general case. Finally, searching these revealed a call to sd_pid_get_session() in implementing some types of logind inhibitors. So these inhibitors don't work as intended when taken from inside a systemd user service :(. Comment this as well, deferring it as ticket #6852. --- src/login/logind-dbus.c | 6 ++++++ src/login/logind-inhibit.c | 2 ++ 2 files changed, 8 insertions(+) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 5b5fdf443b..a023546b4e 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -57,6 +57,9 @@ static int get_sender_session(Manager *m, sd_bus_message *message, sd_bus_error Session *session; int r; + /* Get client login session. This is not what you are looking for these days, + * as apps may instead belong to a user service unit. This includes terminal + * emulators and hence command-line apps. */ r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_SESSION|SD_BUS_CREDS_AUGMENT, &creds); if (r < 0) return r; @@ -364,6 +367,9 @@ static int method_get_session(sd_bus_message *message, void *userdata, sd_bus_er return sd_bus_reply_method_return(message, "o", p); } +/* Get login session of a process. This is not what you are looking for these days, + * as apps may instead belong to a user service unit. This includes terminal + * emulators and hence command-line apps. */ static int method_get_session_by_pid(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_free_ char *p = NULL; Session *session = NULL; diff --git a/src/login/logind-inhibit.c b/src/login/logind-inhibit.c index c1a319b87d..f97e237019 100644 --- a/src/login/logind-inhibit.c +++ b/src/login/logind-inhibit.c @@ -357,6 +357,8 @@ static int pid_is_active(Manager *m, pid_t pid) { Session *s; int r; + /* Get client session. This is not what you are looking for these days. + * FIXME #6852 */ r = manager_get_session_by_pid(m, pid, &s); if (r < 0) return r; From 545f779f5c5cfac557442910f4182708750facbb Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Mon, 18 Sep 2017 18:04:59 +0100 Subject: [PATCH 09/10] loginctl: enable-linger does not need fallback to XDG_SESSION_ID To maintain consistency with `loginctl user-status`, drop the fallback to XDG_SESSION_ID for `loginctl enable-linger`. The fallback was unnecessary and also incorrect: it passed the numeric value of the session identifier as a UID value. --- src/login/loginctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/login/loginctl.c b/src/login/loginctl.c index a63174c093..e8d5492787 100644 --- a/src/login/loginctl.c +++ b/src/login/loginctl.c @@ -1164,11 +1164,11 @@ static int enable_linger(int argc, char *argv[], void *userdata) { b = streq(argv[0], "enable-linger"); if (argc < 2) { - /* No argument? Let's either use $XDG_SESSION_ID (if specified), or an empty - * session name, in which case logind will try to guess our session. */ + /* No argument? Let's use an empty user name, + * then logind will use our user. */ short_argv[0] = argv[0]; - short_argv[1] = getenv("XDG_SESSION_ID") ?: (char*) ""; + short_argv[1] = (char*) ""; short_argv[2] = NULL; argv = short_argv; argc = 2; From 34160d91953da7074e40341b6a29d216f36b6045 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Fri, 15 Sep 2017 17:35:02 +0100 Subject: [PATCH 10/10] logind: fix SetLinger to authorize by client's effective User ID SetLinger is authorized by the PolicyKit action "set-self-linger", if it is not passed an explicit UID. According to comments we were determining the default UID from the client's session. However, user processes e.g. which are run from a terminal emulator do not necessarily belong to a session scope unit. They may equally be started from the systemd user manager [1][2]. Actually the comment was wrong, and it would also have worked for processes started from the systemd user manager. Nevertheless it seems to involve fetching "augmented credentials" i.e. it's using a racy method, so we shouldn't have been authenticating based on it. We could change the default UID, but that raises issues especially for consistency between the methods. Instead we can just use the clients effective UID for authorization. This commit also fixes `loginctl enable-linger $USER` to match the docs that say it was equivalent to `loginctl enable-linger` (given that $USER matches the callers user and owner_uid). Previously, the former would not have suceeded for unpriviliged users in the default configuration. [1] It seems the main meaning of per-session scopes is tracking the PAM login process. Killing that provokes logind to revoke device access. Less circularly, killing it provokes getty to hangup the TTY. [2] User units may be started with an environment which includes XDG_SESSION_ID (presuambly GNOME does this?). Or not. --- man/loginctl.xml | 9 ++++----- src/login/logind-dbus.c | 30 ++++++++++++++++-------------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/man/loginctl.xml b/man/loginctl.xml index 534a0d961e..617852dfa7 100644 --- a/man/loginctl.xml +++ b/man/loginctl.xml @@ -280,11 +280,10 @@ one or more logged in users, followed by the most recent log data from the journal. Takes one or more user names or numeric user IDs as parameters. If no parameters are passed, the status - of the caller's user is shown. This function is intended to - generate human-readable output. If you are looking for - computer-parsable output, use show-user - instead. Users may be specified by their usernames or numeric - user IDs. + is shown for the user of the session of the caller. This + function is intended to generate human-readable output. If you + are looking for computer-parsable output, use + show-user instead. diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index a023546b4e..016274fb7a 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -1160,13 +1160,13 @@ static int method_terminate_seat(sd_bus_message *message, void *userdata, sd_bus } static int method_set_user_linger(sd_bus_message *message, void *userdata, sd_bus_error *error) { + _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; _cleanup_free_ char *cc = NULL; Manager *m = userdata; int r, b, interactive; struct passwd *pw; const char *path; - uint32_t uid; - bool self = false; + uint32_t uid, auth_uid; assert(message); assert(m); @@ -1175,22 +1175,23 @@ static int method_set_user_linger(sd_bus_message *message, void *userdata, sd_bu if (r < 0) return r; + r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID | + SD_BUS_CREDS_OWNER_UID|SD_BUS_CREDS_AUGMENT, &creds); + if (r < 0) + return r; + if (!uid_is_valid(uid)) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - - /* Note that we get the owner UID of the session, not the actual client UID here! */ - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_OWNER_UID|SD_BUS_CREDS_AUGMENT, &creds); - if (r < 0) - return r; - + /* Note that we get the owner UID of the session or user unit, + * not the actual client UID here! */ r = sd_bus_creds_get_owner_uid(creds, &uid); if (r < 0) return r; + } - self = true; - - } else if (!uid_is_valid(uid)) - return -EINVAL; + /* owner_uid is racy, so for authorization we must use euid */ + r = sd_bus_creds_get_euid(creds, &auth_uid); + if (r < 0) + return r; errno = 0; pw = getpwuid(uid); @@ -1200,7 +1201,8 @@ static int method_set_user_linger(sd_bus_message *message, void *userdata, sd_bu r = bus_verify_polkit_async( message, CAP_SYS_ADMIN, - self ? "org.freedesktop.login1.set-self-linger" : "org.freedesktop.login1.set-user-linger", + uid == auth_uid ? "org.freedesktop.login1.set-self-linger" : + "org.freedesktop.login1.set-user-linger", NULL, interactive, UID_INVALID,