Merge pull request #9667 from poettering/pam_systemd-fixes

pam_systemd fixes

README

@@ -169,7 +169,7 @@ REQUIREMENTS:
         dependencies:
 
         util-linux >= v2.27.1 required
-        dbus >= 1.4.0 (strictly speaking optional, but recommended)
+        dbus >= 1.9.14 (strictly speaking optional, but recommended)
                 NOTE: If using dbus < 1.9.18, you should override the default
                 policy directory (--with-dbuspolicydir=/etc/dbus-1/system.d).
         dracut (optional)

man/pam_systemd.xml

@@ -84,40 +84,43 @@
       <varlistentry>
         <term><option>class=</option></term>
 
-        <listitem><para>Takes a string argument which sets the session
+        <listitem><para>Takes a string argument which sets the session class. The <varname>XDG_SESSION_CLASS</varname>
-        class. The XDG_SESSION_CLASS environmental variable takes
+        environment variable (see below) takes precedence. One of <literal>user</literal>, <literal>greeter</literal>,
-        precedence. One of
+        <literal>lock-screen</literal> or <literal>background</literal>. See
-        <literal>user</literal>,
+        <citerefentry><refentrytitle>sd_session_get_class</refentrytitle><manvolnum>3</manvolnum></citerefentry> for
-        <literal>greeter</literal>,
+        details about the session class.</para></listitem>
-        <literal>lock-screen</literal> or
-        <literal>background</literal>. See
-        <citerefentry><refentrytitle>sd_session_get_class</refentrytitle><manvolnum>3</manvolnum></citerefentry>
-        for details about the session class.</para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><option>type=</option></term>
 
-        <listitem><para>Takes a string argument which sets the session
+        <listitem><para>Takes a string argument which sets the session type. The <varname>XDG_SESSION_TYPE</varname>
-        type. The XDG_SESSION_TYPE environmental variable takes
+        environment variable (see below) takes precedence. One of <literal>unspecified</literal>,
-        precedence. One of
+        <literal>tty</literal>, <literal>x11</literal>, <literal>wayland</literal> or <literal>mir</literal>. See
-        <literal>unspecified</literal>,
+        <citerefentry><refentrytitle>sd_session_get_type</refentrytitle><manvolnum>3</manvolnum></citerefentry> for
-        <literal>tty</literal>,
+        details about the session type.</para></listitem>
-        <literal>x11</literal>,
+      </varlistentry>
-        <literal>wayland</literal> or
+
-        <literal>mir</literal>. See
+      <varlistentry>
-        <citerefentry><refentrytitle>sd_session_get_type</refentrytitle><manvolnum>3</manvolnum></citerefentry>
+        <term><option>desktop=</option></term>
-        for details about the session type.</para></listitem>
+
+        <listitem><para>Takes a single, short identifier string for the desktop environment. The
+        <varname>XDG_SESSION_DESKTOP</varname> environment variable (see below) takes precedence. This may be used to
+        indicate the session desktop used, where this applies and if this information is available. For example:
+        <literal>GNOME</literal>, or <literal>KDE</literal>. It is recommended to use the same identifiers and
+        capitalization as for <varname>$XDG_CURRENT_DESKTOP</varname>, as defined by the <ulink
+        url="http://standards.freedesktop.org/desktop-entry-spec/latest/">Desktop Entry
+        Specification</ulink>. (However, note that the option only takes a single item, and not a colon-separated list
+        like <varname>$XDG_CURRENT_DESKTOP</varname>.) See
+        <citerefentry><refentrytitle>sd_session_get_desktop</refentrytitle><manvolnum>3</manvolnum></citerefentry> for
+        further details.</para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><option>debug<optional>=</optional></option></term>
 
-        <listitem><para>Takes an optional
+        <listitem><para>Takes an optional boolean argument. If yes or without the argument, the module will log
-        boolean argument. If yes or without
+        debugging information as it operates.</para></listitem>
-        the argument, the module will log
-        debugging information as it
-        operates.</para></listitem>
       </varlistentry>
     </variablelist>
   </refsect1>
@@ -131,20 +134,20 @@
   <refsect1>
     <title>Environment</title>
 
-    <para>The following environment variables are set for the
+    <para>The following environment variables are initialized by the module and available to the processes of the
-    processes of the user's session:</para>
+    user's session:</para>
 
     <variablelist class='environment-variables'>
       <varlistentry>
         <term><varname>$XDG_SESSION_ID</varname></term>
 
-        <listitem><para>A session identifier, suitable to be used in
+        <listitem><para>A short session identifier, suitable to be used in filenames. The string itself should be
-        filenames. The string itself should be considered opaque,
+        considered opaque, although often it is just the audit session ID as reported by
-        although often it is just the audit session ID as reported by
+        <filename>/proc/self/sessionid</filename>. Each ID will be assigned only once during machine uptime. It may
-        <filename>/proc/self/sessionid</filename>. Each ID will be
+        hence be used to uniquely label files or other resources of this session. Combine this ID with the boot
-        assigned only once during machine uptime. It may hence be used
+        identifier, as returned by
-        to uniquely label files or other resources of this
+        <citerefentry><refentrytitle>sd_id128_get_boot</refentrytitle><manvolnum>3</manvolnum></citerefentry>, for a
-        session.</para></listitem>
+        globally unique identifier for the current session.</para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -174,45 +177,31 @@
 
     </variablelist>
 
-    <para>The following environment variables are read by the module
+    <para>The following environment variables are read by the module and may be used by the PAM service to pass
-    and may be used by the PAM service to pass metadata to the
+    metadata to the module. If these variables are not set when the PAM module is invoked but can be determined
-    module:</para>
+    otherwise they are set by the module, so that these variables are initialized for the session and applications if
+    known at all.</para>
 
     <variablelist class='environment-variables'>
       <varlistentry>
         <term><varname>$XDG_SESSION_TYPE</varname></term>
 
-        <listitem><para>The session type. This may be used instead of
+        <listitem><para>The session type. This may be used instead of <option>session=</option> on the module parameter
-        <option>session=</option> on the module parameter line, and is
+        line, and is usually preferred.</para></listitem>
-        usually preferred.</para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><varname>$XDG_SESSION_CLASS</varname></term>
 
-        <listitem><para>The session class. This may be used instead of
+        <listitem><para>The session class. This may be used instead of <option>class=</option> on the module parameter
-        <option>class=</option> on the module parameter line, and is
+        line, and is usually preferred.</para></listitem>
-        usually preferred.</para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><varname>$XDG_SESSION_DESKTOP</varname></term>
 
-        <listitem><para>A single, short identifier string for the
+        <listitem><para>The desktop identifier. This may be used instead of <option>desktop=</option> on the module
-        desktop environment. This may be used to indicate the session
+        parameter line, and is usually preferred.</para></listitem>
-        desktop used, where this applies and if this information is
-        available. For example: <literal>GNOME</literal>, or
-        <literal>KDE</literal>. It is recommended to use the same
-        identifiers and capitalization as for
-        <varname>$XDG_CURRENT_DESKTOP</varname>, as defined by the
-        <ulink
-        url="http://standards.freedesktop.org/desktop-entry-spec/latest/">Desktop
-        Entry Specification</ulink>. (However, note that
-        <varname>$XDG_SESSION_DESKTOP</varname> only takes a single
-        item, and not a colon-separated list like
-        <varname>$XDG_CURRENT_DESKTOP</varname>.) See
-        <citerefentry><refentrytitle>sd_session_get_desktop</refentrytitle><manvolnum>3</manvolnum></citerefentry>
-        for more details.</para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -231,9 +220,9 @@
       </varlistentry>
     </variablelist>
 
-    <para>If not set, <command>pam_systemd</command> will determine the
+    <para>If not set, <command>pam_systemd</command> will initialize
-    values for <varname>$XDG_SEAT</varname> and <varname>$XDG_VTNR</varname>
+    <varname>$XDG_SEAT</varname> and <varname>$XDG_VTNR</varname>
-    based on the <varname>$DISPLAY</varname> variable.</para>
+    based on the <varname>$DISPLAY</varname> variable (if the latter is set).</para>
   </refsect1>
 
   <refsect1>

src/basic/util.c

@@ -77,31 +77,6 @@ bool display_is_local(const char *display) {
                 display[1] <= '9';
 }
 
-int socket_from_display(const char *display, char **path) {
-        size_t k;
-        char *f, *c;
-
-        assert(display);
-        assert(path);
-
-        if (!display_is_local(display))
-                return -EINVAL;
-
-        k = strspn(display+1, "0123456789");
-
-        f = new(char, STRLEN("/tmp/.X11-unix/X") + k + 1);
-        if (!f)
-                return -ENOMEM;
-
-        c = stpcpy(f, "/tmp/.X11-unix/X");
-        memcpy(c, display+1, k);
-        c[k] = 0;
-
-        *path = f;
-
-        return 0;
-}
-
 bool kexec_loaded(void) {
        _cleanup_free_ char *s = NULL;
 

src/basic/util.h

@@ -50,7 +50,6 @@ static inline const char* enable_disable(bool b) {
 bool plymouth_running(void);
 
 bool display_is_local(const char *display) _pure_;
-int socket_from_display(const char *display, char **path);
 
 #define NULSTR_FOREACH(i, l)                                    \
         for ((i) = (l); (i) && *(i); (i) = strchr((i), 0)+1)

src/login/pam_systemd.c

@@ -16,6 +16,7 @@
 #include "bus-common-errors.h"
 #include "bus-error.h"
 #include "bus-util.h"
+#include "cgroup-util.h"
 #include "def.h"
 #include "fd-util.h"
 #include "fileio.h"
@@ -24,19 +25,19 @@
 #include "login-util.h"
 #include "macro.h"
 #include "parse-util.h"
+#include "path-util.h"
 #include "process-util.h"
 #include "socket-util.h"
 #include "strv.h"
 #include "terminal-util.h"
 #include "util.h"
-#include "path-util.h"
-#include "cgroup-util.h"
 
 static int parse_argv(
                 pam_handle_t *handle,
                 int argc, const char **argv,
                 const char **class,
                 const char **type,
+                const char **desktop,
                 bool *debug) {
 
         unsigned i;
@@ -53,6 +54,10 @@ static int parse_argv(
                         if (type)
                                 *type = argv[i] + 5;
 
+                } else if (startswith(argv[i], "desktop=")) {
+                        if (desktop)
+                                *desktop = argv[i] + 8;
+
                 } else if (streq(argv[i], "debug")) {
                         if (debug)
                                 *debug = true;
@@ -109,6 +114,31 @@ static int get_user_data(
         return PAM_SUCCESS;
 }
 
+static int socket_from_display(const char *display, char **path) {
+        size_t k;
+        char *f, *c;
+
+        assert(display);
+        assert(path);
+
+        if (!display_is_local(display))
+                return -EINVAL;
+
+        k = strspn(display+1, "0123456789");
+
+        f = new(char, STRLEN("/tmp/.X11-unix/X") + k + 1);
+        if (!f)
+                return -ENOMEM;
+
+        c = stpcpy(f, "/tmp/.X11-unix/X");
+        memcpy(c, display+1, k);
+        c[k] = 0;
+
+        *path = f;
+
+        return 0;
+}
+
 static int get_seat_from_display(const char *display, const char **seat, uint32_t *vtnr) {
         union sockaddr_union sa = {
                 .un.sun_family = AF_UNIX,
@@ -160,40 +190,6 @@ static int get_seat_from_display(const char *display, const char **seat, uint32_
         return 0;
 }
 
-static int export_legacy_dbus_address(
-                pam_handle_t *handle,
-                uid_t uid,
-                const char *runtime) {
-
-        _cleanup_free_ char *s = NULL;
-        int r = PAM_BUF_ERR;
-
-        /* FIXME: We *really* should move the access() check into the
-         * daemons that spawn dbus-daemon, instead of forcing
-         * DBUS_SESSION_BUS_ADDRESS= here. */
-
-        s = strjoin(runtime, "/bus");
-        if (!s)
-                goto error;
-
-        if (access(s, F_OK) < 0)
-                return PAM_SUCCESS;
-
-        s = mfree(s);
-        if (asprintf(&s, DEFAULT_USER_BUS_ADDRESS_FMT, runtime) < 0)
-                goto error;
-
-        r = pam_misc_setenv(handle, "DBUS_SESSION_BUS_ADDRESS", s, 0);
-        if (r != PAM_SUCCESS)
-                goto error;
-
-        return PAM_SUCCESS;
-
-error:
-        pam_syslog(handle, LOG_ERR, "Failed to set bus variable.");
-        return r;
-}
-
 static int append_session_memory_max(pam_handle_t *handle, sd_bus_message *m, const char *limit) {
         uint64_t val;
         int r;
@@ -231,8 +227,7 @@ static int append_session_memory_max(pam_handle_t *handle, sd_bus_message *m, co
         return 0;
 }
 
-static int append_session_tasks_max(pam_handle_t *handle, sd_bus_message *m, const char *limit)
+static int append_session_tasks_max(pam_handle_t *handle, sd_bus_message *m, const char *limit) {
-{
         uint64_t val;
         int r;
 
@@ -257,23 +252,62 @@ static int append_session_cg_weight(pam_handle_t *handle, sd_bus_message *m, con
         uint64_t val;
         int r;
 
-        if (!isempty(limit)) {
+        if (isempty(limit))
-                r = cg_weight_parse(limit, &val);
+                return 0;
-                if (r >= 0) {
+
-                        r = sd_bus_message_append(m, "(sv)", field, "t", val);
+        r = cg_weight_parse(limit, &val);
-                        if (r < 0) {
+        if (r >= 0) {
-                                pam_syslog(handle, LOG_ERR, "Failed to append to bus message: %s", strerror(-r));
+                r = sd_bus_message_append(m, "(sv)", field, "t", val);
-                                return r;
+                if (r < 0) {
-                        }
+                        pam_syslog(handle, LOG_ERR, "Failed to append to bus message: %s", strerror(-r));
-                } else if (streq(field, "CPUWeight"))
+                        return r;
-                        pam_syslog(handle, LOG_WARNING, "Failed to parse systemd.cpu_weight: %s, ignoring.", limit);
+                }
-                else
+        } else if (streq(field, "CPUWeight"))
-                        pam_syslog(handle, LOG_WARNING, "Failed to parse systemd.io_weight: %s, ignoring.", limit);
+                pam_syslog(handle, LOG_WARNING, "Failed to parse systemd.cpu_weight: %s, ignoring.", limit);
-        }
+        else
+                pam_syslog(handle, LOG_WARNING, "Failed to parse systemd.io_weight: %s, ignoring.", limit);
 
         return 0;
 }
 
+static const char* getenv_harder(pam_handle_t *handle, const char *key, const char *fallback) {
+        const char *v;
+
+        assert(handle);
+        assert(key);
+
+        /* Looks for an environment variable, preferrably in the environment block associated with the specified PAM
+         * handle, falling back to the process' block instead. */
+
+        v = pam_getenv(handle, key);
+        if (!isempty(v))
+                return v;
+
+        v = getenv(key);
+        if (!isempty(v))
+                return v;
+
+        return fallback;
+}
+
+static int update_environment(pam_handle_t *handle, const char *key, const char *value) {
+        int r;
+
+        assert(handle);
+        assert(key);
+
+        /* Updates the environment, but only if there's actually a value set. Also, log about errors */
+
+        if (isempty(value))
+                return PAM_SUCCESS;
+
+        r = pam_misc_setenv(handle, key, value, 0);
+        if (r != PAM_SUCCESS)
+                pam_syslog(handle, LOG_ERR, "Failed to set environment variable %s.", key);
+
+        return r;
+}
+
 _public_ PAM_EXTERN int pam_sm_open_session(
                 pam_handle_t *handle,
                 int flags,
@@ -288,7 +322,7 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 *remote_user = NULL, *remote_host = NULL,
                 *seat = NULL,
                 *type = NULL, *class = NULL,
-                *class_pam = NULL, *type_pam = NULL, *cvtnr = NULL, *desktop = NULL,
+                *class_pam = NULL, *type_pam = NULL, *cvtnr = NULL, *desktop = NULL, *desktop_pam = NULL,
                 *memory_max = NULL, *tasks_max = NULL, *cpu_weight = NULL, *io_weight = NULL;
         _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL;
         int session_fd = -1, existing, r;
@@ -307,6 +341,7 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                        argc, argv,
                        &class_pam,
                        &type_pam,
+                       &desktop_pam,
                        &debug) < 0)
                 return PAM_SESSION_ERR;
 
@@ -338,10 +373,6 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                         return r;
                 }
 
-                r = export_legacy_dbus_address(handle, pw->pw_uid, rt);
-                if (r != PAM_SUCCESS)
-                        return r;
-
                 return PAM_SUCCESS;
         }
 
@@ -352,29 +383,11 @@ _public_ PAM_EXTERN int pam_sm_open_session(
         pam_get_item(handle, PAM_RUSER, (const void**) &remote_user);
         pam_get_item(handle, PAM_RHOST, (const void**) &remote_host);
 
-        seat = pam_getenv(handle, "XDG_SEAT");
+        seat = getenv_harder(handle, "XDG_SEAT", NULL);
-        if (isempty(seat))
+        cvtnr = getenv_harder(handle, "XDG_VTNR", NULL);
-                seat = getenv("XDG_SEAT");
+        type = getenv_harder(handle, "XDG_SESSION_TYPE", type_pam);
-
+        class = getenv_harder(handle, "XDG_SESSION_CLASS", class_pam);
-        cvtnr = pam_getenv(handle, "XDG_VTNR");
+        desktop = getenv_harder(handle, "XDG_SESSION_DESKTOP", desktop_pam);
-        if (isempty(cvtnr))
-                cvtnr = getenv("XDG_VTNR");
-
-        type = pam_getenv(handle, "XDG_SESSION_TYPE");
-        if (isempty(type))
-                type = getenv("XDG_SESSION_TYPE");
-        if (isempty(type))
-                type = type_pam;
-
-        class = pam_getenv(handle, "XDG_SESSION_CLASS");
-        if (isempty(class))
-                class = getenv("XDG_SESSION_CLASS");
-        if (isempty(class))
-                class = class_pam;
-
-        desktop = pam_getenv(handle, "XDG_SESSION_DESKTOP");
-        if (isempty(desktop))
-                desktop = getenv("XDG_SESSION_DESKTOP");
 
         tty = strempty(tty);
 
@@ -415,9 +428,9 @@ _public_ PAM_EXTERN int pam_sm_open_session(
 
         if (!isempty(display) && !vtnr) {
                 if (isempty(seat))
-                        get_seat_from_display(display, &seat, &vtnr);
+                        (void) get_seat_from_display(display, &seat, &vtnr);
                 else if (streq(seat, "seat0"))
-                        get_seat_from_display(display, NULL, &vtnr);
+                        (void) get_seat_from_display(display, NULL, &vtnr);
         }
 
         if (seat && !streq(seat, "seat0") && vtnr != 0) {
@@ -550,11 +563,9 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                            "id=%s object_path=%s runtime_path=%s session_fd=%d seat=%s vtnr=%u original_uid=%u",
                            id, object_path, runtime_path, session_fd, seat, vtnr, original_uid);
 
-        r = pam_misc_setenv(handle, "XDG_SESSION_ID", id, 0);
+        r = update_environment(handle, "XDG_SESSION_ID", id);
-        if (r != PAM_SUCCESS) {
+        if (r != PAM_SUCCESS)
-                pam_syslog(handle, LOG_ERR, "Failed to set session id.");
                 return r;
-        }
 
         if (original_uid == pw->pw_uid) {
                 /* Don't set $XDG_RUNTIME_DIR if the user we now
@@ -563,34 +574,38 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                  * in privileged apps clobbering the runtime directory
                  * unnecessarily. */
 
-                r = pam_misc_setenv(handle, "XDG_RUNTIME_DIR", runtime_path, 0);
+                r = update_environment(handle, "XDG_RUNTIME_DIR", runtime_path);
-                if (r != PAM_SUCCESS) {
-                        pam_syslog(handle, LOG_ERR, "Failed to set runtime dir.");
-                        return r;
-                }
-
-                r = export_legacy_dbus_address(handle, pw->pw_uid, runtime_path);
                 if (r != PAM_SUCCESS)
                         return r;
         }
 
-        if (!isempty(seat)) {
+        /* Most likely we got the session/type/class from environment variables, but might have gotten the data
-                r = pam_misc_setenv(handle, "XDG_SEAT", seat, 0);
+         * somewhere else (for example PAM module parameters). Let's now update the environment variables, so that this
-                if (r != PAM_SUCCESS) {
+         * data is inherited into the session processes, and programs can rely on them to be initialized. */
-                        pam_syslog(handle, LOG_ERR, "Failed to set seat.");
+
-                        return r;
+        r = update_environment(handle, "XDG_SESSION_TYPE", type);
-                }
+        if (r != PAM_SUCCESS)
-        }
+                return r;
+
+        r = update_environment(handle, "XDG_SESSION_CLASS", class);
+        if (r != PAM_SUCCESS)
+                return r;
+
+        r = update_environment(handle, "XDG_SESSION_DESKTOP", desktop);
+        if (r != PAM_SUCCESS)
+                return r;
+
+        r = update_environment(handle, "XDG_SEAT", seat);
+        if (r != PAM_SUCCESS)
+                return r;
 
         if (vtnr > 0) {
                 char buf[DECIMAL_STR_MAX(vtnr)];
                 sprintf(buf, "%u", vtnr);
 
-                r = pam_misc_setenv(handle, "XDG_VTNR", buf, 0);
+                r = update_environment(handle, "XDG_VTNR", buf);
-                if (r != PAM_SUCCESS) {
+                if (r != PAM_SUCCESS)
-                        pam_syslog(handle, LOG_ERR, "Failed to set virtual terminal number.");
                         return r;
-                }
         }
 
         r = pam_set_data(handle, "systemd.existing", INT_TO_PTR(!!existing), NULL);
@@ -632,7 +647,7 @@ _public_ PAM_EXTERN int pam_sm_close_session(
 
         /* Only release session if it wasn't pre-existing when we
          * tried to create it */
-        pam_get_data(handle, "systemd.existing", &existing);
+        (void) pam_get_data(handle, "systemd.existing", &existing);
 
         id = pam_getenv(handle, "XDG_SESSION_ID");
         if (id && !existing) {