Merge pull request #23569 from msekletar/pam-systemd-no-user-systemd

Don't spawn systemd --user instance for background (i.e. cron) sessions

NEWS

@@ -1,5 +1,13 @@
 systemd System and Service Manager
 
+CHANGES WITH 252:
+        Backwards-incompatible changes:
+
+        * systemd-logind's background session class, which is by default used
+          for sessions started via cron, is no longer going to start systemd
+          --user instance. Note that because of that the user dbus instance
+          will not start either. Other session types remain unaffected.
+
 CHANGES WITH 251:
 
         Backwards-incompatible changes:

src/core/execute.c

@@ -1213,13 +1213,15 @@ static int setup_pam(
 
                 if (getttyname_malloc(STDIN_FILENO, &q) >= 0)
                         tty = strjoina("/dev/", q);
+                else
+                        /* If everything else failed then let's just use value "systemd". This will cause that session
+                         * isn't going to be marked as "background" and user manager will be started. */
+                        tty = "systemd";
         }
 
-        if (tty) {
-                pam_code = pam_set_item(handle, PAM_TTY, tty);
-                if (pam_code != PAM_SUCCESS)
-                        goto fail;
-        }
+        pam_code = pam_set_item(handle, PAM_TTY, tty);
+        if (pam_code != PAM_SUCCESS)
+               goto fail;
 
         STRV_FOREACH(nv, *env) {
                 pam_code = pam_putenv(handle, *nv);

src/login/logind-dbus.c

@@ -1305,7 +1305,7 @@ static int method_set_user_linger(sd_bus_message *message, void *userdata, sd_bu
                         return r;
 
                 if (manager_add_user_by_uid(m, uid, &u) >= 0)
-                        user_start(u);
+                        user_start(u, /* want_user_instance= */ true);
 
         } else {
                 User *u;

src/login/logind-session.c

@@ -664,12 +664,12 @@ static int session_start_scope(Session *s, sd_bus_message *properties, sd_bus_er
                                 description,
                                 /* These two have StopWhenUnneeded= set, hence add a dep towards them */
                                 STRV_MAKE(s->user->runtime_dir_service,
-                                          s->user->service),
+                                          s->class != SESSION_BACKGROUND ? s->user->service : NULL),
                                 /* And order us after some more */
                                 STRV_MAKE("systemd-logind.service",
                                           "systemd-user-sessions.service",
                                           s->user->runtime_dir_service,
-                                          s->user->service),
+                                          s->class != SESSION_BACKGROUND ? s->user->service : NULL),
                                 user_record_home_directory(s->user->user_record),
                                 properties,
                                 error,
@@ -700,7 +700,7 @@ int session_start(Session *s, sd_bus_message *properties, sd_bus_error *error) {
         if (s->started)
                 return 0;
 
-        r = user_start(s->user);
+        r = user_start(s->user, /* want_user_instance= */ s->class != SESSION_BACKGROUND);
         if (r < 0)
                 return r;
 

src/login/logind-user.c

@@ -442,7 +442,7 @@ static int user_update_slice(User *u) {
         return 0;
 }
 
-int user_start(User *u) {
+int user_start(User *u, bool want_user_instance) {
         assert(u);
 
         if (u->started && !u->stopping)
@@ -465,7 +465,8 @@ int user_start(User *u) {
         (void) user_update_slice(u);
 
         /* Start user@UID.service */
-        user_start_service(u);
+        if (want_user_instance)
+                user_start_service(u);
 
         if (!u->started) {
                 if (!dual_timestamp_is_set(&u->timestamp))

src/login/logind-user.h

@@ -57,7 +57,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(User *, user_free);
 
 bool user_may_gc(User *u, bool drop_not_started);
 void user_add_to_gc_queue(User *u);
-int user_start(User *u);
+int user_start(User *u, bool need_user_instance);
 int user_stop(User *u, bool force);
 int user_finalize(User *u);
 UserState user_get_state(User *u);

src/login/logind.c

@@ -1041,7 +1041,6 @@ static int manager_startup(Manager *m) {
         int r;
         Seat *seat;
         Session *session;
-        User *user;
         Button *button;
         Inhibitor *inhibitor;
 
@@ -1118,9 +1117,7 @@ static int manager_startup(Manager *m) {
         HASHMAP_FOREACH(seat, m->seats)
                 (void) seat_start(seat);
 
-        HASHMAP_FOREACH(user, m->users)
-                (void) user_start(user);
-
+        /* Users are started by respective sessions */
         HASHMAP_FOREACH(session, m->sessions)
                 (void) session_start(session, NULL, NULL);
 

src/login/pam_systemd.c

@@ -790,7 +790,10 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                              * does the PAM session registration early for new connections, and registers a pty only
                              * much later (this is because it doesn't know yet if it needs one at all, as whether to
                              * register a pty or not is negotiated much later in the protocol). */
-
+        } else if (streq(tty, "systemd")) {
+                if (isempty(class))
+                        class = "user";
+                tty = NULL;
         } else
                 /* Chop off leading /dev prefix that some clients specify, but others do not. */
                 tty = skip_dev_prefix(tty);

test/test-functions

@@ -1736,6 +1736,7 @@ install_basic_tools() {
     # in Debian ldconfig is just a shell script wrapper around ldconfig.real
     image_install -o ldconfig.real
     # for TEST-35-LOGIN
+    image_install -o evemu-device evemu-event crond crontab
     image_install -o evemu-device evemu-event
     if command -v expect >/dev/null && command -v tclsh >/dev/null ; then
         # shellcheck disable=SC2016

test/units/testsuite-35.sh

@@ -443,6 +443,69 @@ EOF
     rm -f dbus.log logind.log
 }
 
+setup_cron() {
+    # Setup test user and cron
+    useradd test || :
+    crond -s -n &
+    # Install crontab for the test user that runs sleep every minute. But let's sleep for
+    # 65 seconds to make sure there is overlap between two consecutive runs, i.e. we have
+    # always a cron session running.
+    crontab -u test - <<EOF
+RANDOM_DELAY=0
+* * * * * /bin/sleep 65
+EOF
+    # Let's wait (at most one interval plus 10s to accomodate for slow machines) for at least one session of test user
+    timeout 70s bash -c "while true; do loginctl --no-legend list-sessions | awk '{ print \$3 }' | grep -q test && break || sleep 1 ; done"
+}
+
+teardown_cron() (
+    set +ex
+    pkill -9 -u "$(id -u test)"
+    pkill crond
+    crontab -r -u test
+    userdel -r test
+)
+
+test_no_user_instance_for_cron() {
+    if ! command -v crond || ! command -v crontab ; then
+        echo >&2 "Skipping test for background cron sessions because cron is missing."
+        return
+    fi
+
+    trap teardown_cron EXIT
+    setup_cron
+
+    if [[ $(loginctl --no-legend list-sessions | grep -c test) -lt 1 ]]; then
+        echo >&2 '"test" user should have at least one session'
+        loginctl list-sessions
+        return 1
+    fi
+
+    # Check that all sessions of test user have class=background and no user instance was started
+    # for the test user.
+    while read -r s _; do
+        local class
+
+        class=$(loginctl --property Class --value show-session "$s")
+        if [[ "$class" != "background" ]]; then
+            echo >&2 "Session has incorrect class, expected \"background\", got \"$class\"."
+            return 1
+        fi
+    done < <(loginctl --no-legend list-sessions | grep test)
+
+    state=$(systemctl --property ActiveState --value show user@"$(id -u test)".service)
+    if [[ "$state" != "inactive" ]]; then
+        echo >&2 "User instance state is unexpected, expected \"inactive\", got \"$state\""
+        return 1
+    fi
+
+    state=$(systemctl --property SubState --value show user@"$(id -u test)".service)
+    if [[ "$state" != "dead" ]]; then
+        echo >&2 "User instance state is unexpected, expected \"dead\", got \"$state\""
+        return 1
+    fi
+}
+
 : >/failed
 
 test_enable_debug
@@ -452,6 +515,7 @@ test_suspend_on_lid
 test_shutdown
 test_session
 test_lock_idle_action
+test_no_user_instance_for_cron
 
 touch /testok
 rm /failed