[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#1068416: ssh-agent: improve systemd user session integration



Package: openssh-client
Version: 1:9.7p1-4
Severity: wishlist
X-Debbugs-Cc: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
Tags: patch

Hi Debian OpenSSH maintainers!

ssh-agent is a critical piece of infrastructure for my workflow, and i
want it better integrated with my user session, which is managed by
systemd's per-user login manager (`systemd --user`).

It seems to me that the ideal scenario is to use a socket-activated
ssh-agent at a fixed location (in $XDG_RUNTIME_DIR).

The attached series of patches do this, by:

- patching ssh-agent.c to enable socket-activation when run in the
  foreground and the environment matches that described in
  sd_listen_fds(3)

- Adding an ssh-agent.socket file to debian/systemd/, so that the user
  session manager creates the socket and updates the dbus and systemd
  activation environments to point to the socket.  This systemd unit
  should be automatically activated at user login.

- Updating debian's ssh-agent.service unit to simply run ssh-agent in
  the foreground, accepting the distributed socket as its listening
  port.  I think this makes /usr/lib/openssh/agent-launch superfluous;
  maybe it can be removed.

- making `ssh-agent -k` Do What I Mean™ when $SSH_AGENT_PID is unset --
  it just tries to run `systemctl --user stop ssh-agent`, which has the
  same semantics.

Happy to get any feedback on this proposed change.

      --dkg

-- System Information:
Debian Release: trixie/sid
  APT prefers testing-debug
  APT policy: (500, 'testing-debug'), (500, 'testing'), (500, 'stable'), (500, 'oldstable'), (200, 'unstable-debug'), (200, 'unstable'), (1, 'experimental-debug'), (1, 'experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 6.6.15-amd64 (SMP w/4 CPU threads; PREEMPT)
Kernel taint flags: TAINT_FIRMWARE_WORKAROUND
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)

Versions of packages openssh-client depends on:
ii  adduser           3.137
ii  libc6             2.37-15
ii  libedit2          3.1-20230828-1
ii  libfido2-1        1.14.0-1
ii  libgssapi-krb5-2  1.20.1-5+b1
ii  libselinux1       3.5-2
ii  libssl3t64        3.1.5-1.1
ii  passwd            1:4.13+dfsg1-4
ii  zlib1g            1:1.3.dfsg-3+b1

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

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

-- no debconf information
From c5fc76aab1b956c74b2a46b45532f5802f7a1c1a Mon Sep 17 00:00:00 2001
From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
Date: Thu, 4 Apr 2024 13:09:10 -0400
Subject: [PATCH 1/3] ssh-agent: automatically detect when socket-activated via
 systemd

Socket activation for the ssh-agent is useful for several reasons:

- the systemd user session can choose and reserve the socket before
  the session has fully started.

- systemd can inject SSH_AUTH_SOCK directly into the dbus and systemd
  environment, even before the agent is started.

- the agent will be started lazily, on-demand.  This consumes no
  system resources (not even a pid) if the user never tries to talk to
  the agent, and the agent will inherit the systemd service activation
  environment when it is first accessed (for example, if it is first
  accessed from a Wayland session, $WAYLAND_DISPLAY will be set; if it
  is first accessed from an X11 session, $DISPLAY will be set).

We only enter this mode if the following conditions are true:

 - One of the -D or -d flags are given, and no command arguments are
   present (ssh-agent when supervised by systemd must be run in the
   foreground and not as a subprocess), and

 - The environment variable conditions described in
   sd_listen_fds(3) are met, and only a single socket is provided.
---
 ssh-agent.1 |  8 ++++++++
 ssh-agent.c | 55 +++++++++++++++++++++++++++++++++++------------------
 2 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/ssh-agent.1 b/ssh-agent.1
index 1ab7d729d..0ce0cd781 100644
--- a/ssh-agent.1
+++ b/ssh-agent.1
@@ -247,6 +247,14 @@ starts, it creates a
 socket and stores its pathname in this variable.
 It is accessible only to the current user,
 but is easily abused by root or another instance of the same user.
+.It Ev SD_LISTEN_FDS
+When
+.Nm
+starts, if no socket path has been specified, but this variable and the
+accompanying SD_LISTEN_PID are set as described in
+.Xr sd_listen_fds 3 ,
+.Nm
+uses the socket passed in by the systemd supervisor.
 .El
 .Pp
 In Debian,
diff --git a/ssh-agent.c b/ssh-agent.c
index d35741a86..47472e069 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -2194,7 +2194,7 @@ main(int ac, char **av)
 {
 	int c_flag = 0, d_flag = 0, D_flag = 0, k_flag = 0, s_flag = 0;
 	int sock, ch, result, saved_errno;
-	char *shell, *format, *pidstr, *agentsocket = NULL;
+	char *shell, *format, *pidstr, *listen, *agentsocket = NULL;
 #ifdef HAVE_SETRLIMIT
 	struct rlimit rlim;
 #endif
@@ -2339,14 +2339,27 @@ main(int ac, char **av)
 	parent_pid = getpid();
 
 	if (agentsocket == NULL) {
-		/* Create private directory for agent socket */
-		mktemp_proto(socket_dir, sizeof(socket_dir));
-		if (mkdtemp(socket_dir) == NULL) {
-			perror("mkdtemp: private socket dir");
-			exit(1);
+		/* Check if we are socket-activated with a single socket, as
+		 * described in sd_listen_fds(3) */
+		if ((D_flag || d_flag) &&
+			(listen = getenv("LISTEN_PID")) &&
+			strtol(listen, NULL, 10) == (long)parent_pid &&
+			(listen = getenv("LISTEN_FDS")) &&
+			strcmp(listen, "1") == 0) {
+			socket_name[0] = '\0';
+			socket_dir[0] = '\0';
+			/* this is SD_LISTEN_FDS_START */
+			sock = 3;
+		} else {
+			/* Create private directory for agent socket */
+			mktemp_proto(socket_dir, sizeof(socket_dir));
+			if (mkdtemp(socket_dir) == NULL) {
+				perror("mkdtemp: private socket dir");
+				exit(1);
+			}
+			snprintf(socket_name, sizeof socket_name, "%s/agent.%ld", socket_dir,
+					 (long)parent_pid);
 		}
-		snprintf(socket_name, sizeof socket_name, "%s/agent.%ld", socket_dir,
-		    (long)parent_pid);
 	} else {
 		/* Try to use specified agent socket */
 		socket_dir[0] = '\0';
@@ -2357,14 +2370,16 @@ main(int ac, char **av)
 	 * Create socket early so it will exist before command gets run from
 	 * the parent.
 	 */
-	prev_mask = umask(0177);
-	sock = unix_listener(socket_name, SSH_LISTEN_BACKLOG, 0);
 	if (sock < 0) {
-		/* XXX - unix_listener() calls error() not perror() */
-		*socket_name = '\0'; /* Don't unlink any existing file */
-		cleanup_exit(1);
+		prev_mask = umask(0177);
+		sock = unix_listener(socket_name, SSH_LISTEN_BACKLOG, 0);
+		if (sock < 0) {
+			/* XXX - unix_listener() calls error() not perror() */
+			*socket_name = '\0'; /* Don't unlink any existing file */
+			cleanup_exit(1);
+		}
+		umask(prev_mask);
 	}
-	umask(prev_mask);
 
 	/*
 	 * Fork, and have the parent execute the command, if any, or present
@@ -2374,11 +2389,13 @@ main(int ac, char **av)
 		log_init(__progname,
 		    d_flag ? SYSLOG_LEVEL_DEBUG3 : SYSLOG_LEVEL_INFO,
 		    SYSLOG_FACILITY_AUTH, 1);
-		format = c_flag ? "setenv %s %s;\n" : "%s=%s; export %s;\n";
-		printf(format, SSH_AUTHSOCKET_ENV_NAME, socket_name,
-		    SSH_AUTHSOCKET_ENV_NAME);
-		printf("echo Agent pid %ld;\n", (long)parent_pid);
-		fflush(stdout);
+		if (socket_name[0]) {
+			format = c_flag ? "setenv %s %s;\n" : "%s=%s; export %s;\n";
+			printf(format, SSH_AUTHSOCKET_ENV_NAME, socket_name,
+				   SSH_AUTHSOCKET_ENV_NAME);
+			printf("echo Agent pid %ld;\n", (long)parent_pid);
+			fflush(stdout);
+		}
 		goto skip;
 	}
 	pid = fork();
-- 
2.43.0

From 2af234a4cc89fc1f0643fc3c059a45d84e2f5876 Mon Sep 17 00:00:00 2001
From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
Date: Thu, 4 Apr 2024 13:08:55 -0400
Subject: [PATCH 2/3] update systemd unit files for socket-activated ssh-agent

---
 debian/systemd/ssh-agent.service | 10 +++-------
 debian/systemd/ssh-agent.socket  | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 7 deletions(-)
 create mode 100644 debian/systemd/ssh-agent.socket

diff --git a/debian/systemd/ssh-agent.service b/debian/systemd/ssh-agent.service
index 68273bd75..72e0a3e46 100644
--- a/debian/systemd/ssh-agent.service
+++ b/debian/systemd/ssh-agent.service
@@ -1,17 +1,13 @@
 [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]
+Environment=SSH_ASKPASS_REQUIRE=force
 # If you need to pass extra arguments to ssh-agent, you can use "systemctl
 # --user edit ssh-agent.service" to add a drop-in unit with contents along
 # these lines:
 #   [Service]
 #   ExecStart=
-#   ExecStart=/usr/lib/openssh/agent-launch start -- -t 1200
-ExecStart=/usr/lib/openssh/agent-launch start
-ExecStopPost=/usr/lib/openssh/agent-launch stop
+#   ExecStart=/usr/bin/ssh-agent -D -t 1200
+ExecStart=/usr/bin/ssh-agent -D
diff --git a/debian/systemd/ssh-agent.socket b/debian/systemd/ssh-agent.socket
new file mode 100644
index 000000000..6bdc6f3f0
--- /dev/null
+++ b/debian/systemd/ssh-agent.socket
@@ -0,0 +1,14 @@
+[Unit]
+Description=OpenSSH Agent socket
+Documentation=man:ssh-agent(1)
+Before=graphical-session-pre.target
+Wants=dbus.socket
+After=dbus.socket
+
+[Socket]
+ListenStream=%t/openssh_agent
+ExecStartPost=/usr/bin/dbus-update-activation-environment --systemd SSH_AUTH_SOCK=%t/openssh_agent
+ExecStopPre=/usr/bin/dbus-update-activation-environment --systemd SSH_AUTH_SOCK=
+
+[Install]
+WantedBy=sockets.target
-- 
2.43.0

From 34a148930a9fc4eb7f99265b26829d9ff0c6c6e6 Mon Sep 17 00:00:00 2001
From: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
Date: Thu, 4 Apr 2024 14:17:36 -0400
Subject: [PATCH 3/3] ssh-agent -k: kill via systemctl --user if SSH_AGENT_PID
 is unset

When ssh-agent is socket-activated and managed for the session by
systemd, $SSH_AGENT_PID will not be explicitly set.

In this case, ssh-agent -k should try to talk to the local systemd
user session manager before giving up.
---
 ssh-agent.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/ssh-agent.c b/ssh-agent.c
index 47472e069..c1fd2cc20 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -2303,8 +2303,11 @@ main(int ac, char **av)
 
 		pidstr = getenv(SSH_AGENTPID_ENV_NAME);
 		if (pidstr == NULL) {
-			fprintf(stderr, "%s not set, cannot kill agent\n",
-			    SSH_AGENTPID_ENV_NAME);
+			execlp("systemctl", "systemctl", "--user", "stop", "ssh-agent.service", (char*)NULL);
+			fprintf(stderr, "%s not set, cannot kill agent directly.\n"
+					"'systemctl --user stop ssh-agent.service' did not run: %d (%s).\n",
+					SSH_AGENTPID_ENV_NAME,
+					errno, strerror(errno));
 			exit(1);
 		}
 		pid = (int)strtonum(pidstr, 2, INT_MAX, &errstr);
-- 
2.43.0

Attachment: signature.asc
Description: PGP signature


Reply to: