Bug#961311: ssh-agent.service: cannot work without x11-common installed

[Paul Wise]

Package: openssh-client
Version: 1:8.2p1-4
Severity: normal
File: /usr/lib/systemd/user/ssh-agent.service
Usertags: wayland

On pure-Wayland systems, the ssh-agent.service is not going to be able
to start because x11-common won't nessecarily be installed. I think
either the ConditionPathExists should get dropped and the script should
check if the Xsession.options file exists before using it, when it
doesn't exist people using Wayland without x11-common installed can just use `systemctl --user mask/enable/disable` to manage the service.

PS: shellcheck reports some issues for the agent-launch script.

$ systemctl --user cat ssh-agent.service
# /usr/lib/systemd/user/ssh-agent.service
[Unit]
Description=OpenSSH Agent
Documentation=man:ssh-agent(1)
Before=graphical-session-pre.target
ConditionPathExists=/etc/X11/Xsession.options
Wants=dbus.socket
After=dbus.socket

[Service]
ExecStart=/usr/lib/openssh/agent-launch start
ExecStopPost=/usr/lib/openssh/agent-launch stop

$ dpkg -S /etc/X11/Xsession.options
x11-common: /etc/X11/Xsession.options

$ grep -A4 /etc/X11/Xsession.options /usr/lib/openssh/agent-launch
if [ -z "$SSH_AUTH_SOCK" ] && grep -s -q '^use-ssh-agent$' /etc/X11/Xsession.options; then
S="$XDG_RUNTIME_DIR/openssh_agent"
dbus-update-activation-environment --verbose --systemd SSH_AUTH_SOCK=$S SSH_AGENT_LAUNCHER=openssh
exec ssh-agent -D -a $S
fi

$ shellcheck /usr/lib/openssh/agent-launch

In /usr/lib/openssh/agent-launch line 6:
echo 'This needs $XDG_RUNTIME_DIR to be set' >&2
^-- SC2016: Expressions don't expand in single quotes, use double quotes for that.


In /usr/lib/openssh/agent-launch line 13:
dbus-update-activation-environment --verbose --systemd SSH_AUTH_SOCK=$S SSH_AGENT_LAUNCHER=openssh
^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
dbus-update-activation-environment --verbose --systemd SSH_AUTH_SOCK="$S" SSH_AGENT_LAUNCHER=openssh


In /usr/lib/openssh/agent-launch line 14:
exec ssh-agent -D -a $S
^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
exec ssh-agent -D -a "$S"

For more information:
https://www.shellcheck.net/wiki/SC2016 -- Expressions don't expand in singl...
https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...


-- System Information:
Debian Release: bullseye/sid
APT prefers testing-debug
APT policy: (900, 'testing-debug'), (900, 'testing'), (800, 'unstable-debug'), (800, 'unstable'), (790, 'buildd-unstable'), (700, 'experimental-debug'), (700, 'experimental'), (690, 'buildd-experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 5.6.0-1-amd64 (SMP w/4 CPU cores)
Kernel taint flags: TAINT_FIRMWARE_WORKAROUND
Locale: LANG=en_AU.utf8, LC_CTYPE=en_AU.utf8 (charmap=UTF-8), LANGUAGE=en_AU:en (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages openssh-client depends on:
ii adduser 3.118
ii dpkg 1.19.7
ii libc6 2.30-8
ii libedit2 3.1-20191231-1
ii libfido2-1 1.4.0-1
ii libgssapi-krb5-2 1.17-7
ii libselinux1 3.0-1+b3
ii libssl1.1 1.1.1g-1
ii passwd 1:4.8.1-1
ii zlib1g 1:1.2.11.dfsg-2

Versions of packages openssh-client recommends:
ii xauth 1:1.0.10-1

Versions of packages openssh-client suggests:
pn keychain <none>
pn libpam-ssh <none>
ii monkeysphere 0.43-3
ii ssh-askpass-gnome [ssh-askpass] 1:8.2p1-4

-- no debconf information

--
bye,
pabs

https://wiki.debian.org/PaulWise

[Colin Watson]

On Sat, May 23, 2020 at 09:45:55AM +0800, Paul Wise wrote:
> On pure-Wayland systems, the ssh-agent.service is not going to be able
> to start because x11-common won't nessecarily be installed. I think
> either the ConditionPathExists should get dropped and the script should
> check if the Xsession.options file exists before using it, when it
> doesn't exist people using Wayland without x11-common installed can just use `systemctl --user mask/enable/disable` to manage the service.

Hm. I'm somewhat inclined to do something like this. What do you
think?

(I'm aware using loginctl to figure out the session type might be more
technically correct. On the other hand, I seem to be suffering from
something similar to
https://askubuntu.com/questions/1238984/ubuntu-20-04-xdg-seat-xdg-vtnr-xdg-session-id-env-variables-are-not-set
here, so can't easily test it. Besides, checking the environment
variables is likely faster, so as long as it isn't hopelessly wrong ...)

diff --git a/debian/agent-launch b/debian/agent-launch
index 3b2abdbf7..75f185b72 100755
--- a/debian/agent-launch
+++ b/debian/agent-launch
@@ -8,8 +8,23 @@ if [ ! -d "$XDG_RUNTIME_DIR" ]; then
exit 1
fi

+session_is_x11 () {
+ [ "$DISPLAY" ] && [ -z "$WAYLAND_DISPLAY" ]
+}
+
+session_uses_ssh_agent () {
+ if session_is_x11; then
+ grep -s -q '^use-ssh-agent$' /etc/X11/Xsession.options
+ else
+ # Users of non-X11 sessions can use "systemctl --user mask
+ # ssh-agent.service" or similar to control whether this service
+ # should run.
+ return 0
+ fi
+}
+
if [ "$1" = start ]; then
- if [ -z "$SSH_AUTH_SOCK" ] && grep -s -q '^use-ssh-agent$' /etc/X11/Xsession.options; then
+ if [ -z "$SSH_AUTH_SOCK" ] && session_uses_ssh_agent; then
S="$XDG_RUNTIME_DIR/openssh_agent"
dbus-update-activation-environment --verbose --systemd SSH_AUTH_SOCK="$S" SSH_AGENT_LAUNCHER=openssh

exec ssh-agent -D -a "$S"

diff --git a/debian/systemd/ssh-agent.service b/debian/systemd/ssh-agent.service
index cdc10bb47..4d5d3faf8 100644
--- a/debian/systemd/ssh-agent.service
+++ b/debian/systemd/ssh-agent.service
@@ -2,7 +2,6 @@

Description=OpenSSH Agent
Documentation=man:ssh-agent(1)
Before=graphical-session-pre.target

-ConditionPathExists=/etc/X11/Xsession.options
Wants=dbus.socket
After=dbus.socket

> PS: shellcheck reports some issues for the agent-launch script.

Fixed in git master, thanks.

--
Colin Watson (he/him) [cjwa...@debian.org]

[Paul Wise]

On Sat, 2020-05-23 at 13:07 +0100, Colin Watson wrote:

> What do you think?

I don't think that will work because neither of the DISPLAY nor
WAYLAND_DISPLAY environment variables are set for user services since
they are user services *not* session services and systemd does not
support session services. You could of course check the session type,
but I'm not sure what sets that environment variable and if it is
present for all Wayland compositors. It is also a per-session variable
rather than per-user variable. So I think my idea of checking if
Xsession.options exists is best after all.

$ sudo sh -c "tr '\0' '\n' < /proc/$(pgrep ssh-agent)/environ" | grep DISPLAY
$ sudo sh -c "tr '\0' '\n' < /proc/$(pgrep ssh-agent)/environ" | grep -i wayland
XDG_SESSION_TYPE=wayland

[Colin Watson]

On Sat, May 23, 2020 at 08:18:55PM +0800, Paul Wise wrote:
> On Sat, 2020-05-23 at 13:07 +0100, Colin Watson wrote:
> > What do you think?
>
> I don't think that will work because neither of the DISPLAY nor
> WAYLAND_DISPLAY environment variables are set for user services since
> they are user services *not* session services and systemd does not
> support session services.

OK, fair point.

> You could of course check the session type, but I'm not sure what sets
> that environment variable and if it is present for all Wayland
> compositors. It is also a per-session variable rather than per-user
> variable. So I think my idea of checking if Xsession.options exists is
> best after all.

My concern about that is that it's about what's installed, not what's
active/running for that particular user. It may not matter in your case
but I'm sure cutting that corner is going to be wrong for somebody.

[Paul Wise]

Control: severity -1 minor

I consider this bug pretty minor since X11 stuff is going to be
installed for quite some time on Debian's Wayland based desktops, so
don't spend too much effort on this right now.

On Sat, 2020-05-23 at 15:57 +0100, Colin Watson wrote:

> My concern about that is that it's about what's installed, not what's
> active/running for that particular user. It may not matter in your case
> but I'm sure cutting that corner is going to be wrong for somebody.

Hmm, that is a good point.

I think the main problem here is that the service itself (ssh-agent) is
checking settings (Xsession.options) for the service manager (Xsession)
to see if the service should run instead of the service manager doing
the checking. Ideally it should be the other way around instead.

I noticed that Xsession.d 99x11-common_start exists too and also starts
the ssh-agent when appropriate but I assume Xsession isn't used for the
GNOME X11 session run from gdm on systemd though.

In addition various desktops have their own session service starting
mechanisms, although some are moving to systemd user services.

In addition there is the xdg autostart mechanism and I guess there are
user services started by dbus too.

I'm not sure how to untangle this mess, but I guess the first thing to
be done would be to test on both systemd/sysvinit what happens when
launching different X11/Wayland desktops and window managers from
startx and from different login managers to see exactly when ssh-agent
is launched by Xsession and when by systemd and when there are any race
conditions between the two options. Then you can go through the matrix
of possibilities and find plausible fixes. Since the results will
probably be useful to other Debian folks, perhaps a wiki page of the
results and a thread about them would be useful to Debian too.

[Chris Mitchell]

Hi there,

Maybe I'm being hopelessly naive, but is there any value in tying
ssh-agent to X11/Wayland or trying to make it behave like a session
service (as opposed to a user service), or having this "agent-launch"
init script at all?

Personally, I've been using a systemd "global" user unit to manage my
ssh-agent for a couple of years, with a consistent socket location and
no checks for Xsession.options or session type or anything, and I love
it.

$ cat /etc/systemd/user/ssh-agent.service

[Unit]
Description=SSH key agent

[Service]
Type=exec
# %t resolves to XDG_RUNTIME_DIR; see SPECIFIERS section in
systemd.unit(5)
ExecStart=/usr/bin/ssh-agent -D -a "%t/ssh-agent.socket"

[Install]
WantedBy=default.target

To my mind, one of the big advantages of simply letting systemd manage
it is the fact that it's *not* doing any GUI-specific or per-session
cleverness. If I'm logged into XFCE4 and have keys loaded into
ssh-agent, then swap out to VT1 and log in there: systemd no-ops
ssh-agent.service because it's already running. Then I restart lightdm,
which kills the session where I started the agent initially: because I
still have a non-zero number of logged-in sessions, systemd keeps my
ssh-agent.service up, and I can access it right there in VT1. Swap back
to the lightdm greeter and log in to whatever DE, and my agent is still
running, the keys are still loaded, the socket is still where it's
expected to be, and everything Just Works™. On a server where I don't
have X or Wayland or any graphical anything installed, my ssh-agent
still starts when I log in, and everything works exactly the same way.
Why in the world would I want my ssh-agent to not work just because I
don't have X installed?

Any user who doesn't want it can use "systemctl --user disable (or mask)
ssh-agent", or root can do the same for all users with "systemctl
--global", so the blunt implement of .Xsession.options "use-ssh-agent"
option is obsolete.

By far the hardest part of this has been tracking down all the legacy
session scripts and DE hacks and things that kept launching ssh-agent
even though it was already running, (or gnome-keyring trying to
emulate ssh-agent, poorly) and clobbering my SSH_AUTH_SOCK env-var.

As I say, maybe I'm being naive, but I'm tempted to suggest dropping
the ConditionPathExists, doing away with /usr/lib/openssh/agent-launch
completely, dropping the check for "use-ssh-agent" (preferably with a
comment in that file pointing out that the option is deprecated because
enabling/disabling the service with systemd is more granular, more
flexible, more consistent, and more reliable), and starting the agent
directly with ExecStart.

Since the socket location doesn't change, and the user being logged in
*at all* will trigger systemd to start their ssh-agent.service, is
there any need to set and unset SSH_AUTH_SOCK dynamically instead of
just setting it once at login like most env-vars? I guess setting it
dynamically would have the advantage that if *yet another* legacy
workaround launches its own ssh-agent and clobbers the env-var,
restarting ssh-agent.service would clobber it right back.

If we *do* want to dynamically set and unset SSH_AUTH_SOCK, why not do
it directly in the .service file with ExecStartPre and ExecStartPost
directives? I'd prefix the ExecStartPre one with a "-" though, so if
dbus isn't available the service can still start.

Does that make any sense?

Cheers!
-Chris