openssh unix dev - Sep 2021 - ssh-agent: perform AGENTC_REMOVE_ALL_IDENTITIES on SIGUSR1

Hello.

A long time ago i asked for the possibility to remove all
identities from a running agent by sending a signal.

The use case i have in mind is that upon LID close or another
thinkable event all identities can be removed from all running
agents simply by calling "pkill -USR1 ssh-agent".

Another option would be automatic locking as via "ssh-add -x", but
extending ssh-agent to simply require the user's password for
unlocking seemed very complicated or even impossible the way that
the according code is organized and usable in OpenSSH.

For example, what i am currently doing upon LID close is

   if command -v ssh-add >/dev/null 2>&1; then
      for a in /tmp/ssh-*/agent.*; do
         [ -e "$a" ] || continue
         act 'SSH_AUTH_SOCK="$a" ssh-add -D </dev/null
>/dev/null 2>&1 &'
         : > /run/.zzz_act
      done
   fi

which works nicely here, but would not work in environments with
private home directories, or any other complication.
"pkill -USR1 ssh-agent", on the other hand, would work just fine.

The necessary code addition to ssh-agent is minimal, and builds
upon existing code.  The work is done in a signal handler, because
ssh_signal() sets SA_RESTART and thus the loop would not wake up
to tick.  On the other hand ssh_signal() fills the signal mask, so
signal recursion of any form cannot happen, therefore processing
the list in the signal handler seems safe.

I have compiled the diff below, but have not tried it out yet.
(Oh the grief if it could not be upstreamed, then.)

It would be nice if that would be considered.
Thank you.

Ciao from Germany already here,

diff --git a/ssh-agent.1 b/ssh-agent.1
index ed8c870969..dd2b0e8af1 100644
--- a/ssh-agent.1
+++ b/ssh-agent.1
@@ -178,6 +178,9 @@ will automatically use them if present.
 is also used to remove keys from
 .Nm
 and to query the keys that are held in one.
+.Nm
+will remove all keys when it receives the user signal
+.Dv SIGUSR1 .
 .Pp
 Connections to
 .Nm
diff --git a/ssh-agent.c b/ssh-agent.c
index 48a47d45a9..96c3ed77c2 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -529,11 +529,10 @@ process_remove_identity(SocketEntry *e)
 }
 
 static void
-process_remove_all_identities(SocketEntry *e)
+remove_all_identities(void)
 {
 	Identity *id;
 
-	debug2_f("entering");
 	/* Loop over all identities and clear the keys. */
 	for (id = TAILQ_FIRST(&idtab->idlist); id;
 	    id = TAILQ_FIRST(&idtab->idlist)) {
@@ -543,6 +542,13 @@ process_remove_all_identities(SocketEntry *e)
 
 	/* Mark that there are no identities. */
 	idtab->nentries = 0;
+}
+
+static void
+process_remove_all_identities(SocketEntry *e)
+{
+	debug2_f("entering");
+	remove_all_identities();
 
 	/* Send success. */
 	send_status(e, 1);
@@ -1342,6 +1348,13 @@ cleanup_handler(int sig)
 	_exit(2);
 }
 
+/*ARGSUSED*/
+static void
+remove_all_handler(int sig)
+{
+	remove_all_identities();
+}
+
 static void
 check_parent_exists(void)
 {
@@ -1619,6 +1632,7 @@ skip:
 	ssh_signal(SIGINT, (d_flag | D_flag) ? cleanup_handler : SIG_IGN);
 	ssh_signal(SIGHUP, cleanup_handler);
 	ssh_signal(SIGTERM, cleanup_handler);
+	ssh_signal(SIGUSR1, remove_all_handler);
 
 	if (pledge("stdio rpath cpath unix id proc exec", NULL) == -1)
 		fatal("%s: pledge: %s", __progname, strerror(errno));

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)

Steffen Nurpmeso wrote in
 <20210908114659.EuomV%steffen at sdaoden.eu>:
 |A long time ago i asked for the possibility to remove all
 |identities from a running agent by sending a signal.
 ...
 |The necessary code addition to ssh-agent is minimal, and builds
 |upon existing code.  The work is done in a signal handler, because
 |ssh_signal() sets SA_RESTART and thus the loop would not wake up
 |to tick.  On the other hand ssh_signal() fills the signal mask, so
 |signal recursion of any form cannot happen, therefore processing
 |the list in the signal handler seems safe.

Ha!  Not true, i have been kindly enlightened that the signal
can happen at anytime and thus my approach may corrupt state, so
"it cannot be coded this way".
Instead the signal handler shall set a volatile sig_atomic_t that
the main loop later picks up.

diff --git a/ssh-agent.1 b/ssh-agent.1
index ed8c870969..dd2b0e8af1 100644
--- a/ssh-agent.1
+++ b/ssh-agent.1
@@ -178,6 +178,9 @@ will automatically use them if present.
 is also used to remove keys from
 .Nm
 and to query the keys that are held in one.
+.Nm
+will remove all keys when it receives the user signal
+.Dv SIGUSR1 .
 .Pp
 Connections to
 .Nm
diff --git a/ssh-agent.c b/ssh-agent.c
index 48a47d45a9..bc24dadaa8 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -171,6 +171,9 @@ static int fingerprint_hash = SSH_FP_HASH_DEFAULT;
 /* Refuse signing of non-SSH messages for web-origin FIDO keys */
 static int restrict_websafe = 1;
 
+/* Request to remove_all_identities() pending */
+static volatile sig_atomic_t remove_all_next_tick = 0;
+
 static void
 close_socket(SocketEntry *e)
 {
@@ -529,11 +532,10 @@ process_remove_identity(SocketEntry *e)
 }
 
 static void
-process_remove_all_identities(SocketEntry *e)
+remove_all_identities(void)
 {
 	Identity *id;
 
-	debug2_f("entering");
 	/* Loop over all identities and clear the keys. */
 	for (id = TAILQ_FIRST(&idtab->idlist); id;
 	    id = TAILQ_FIRST(&idtab->idlist)) {
@@ -543,6 +545,13 @@ process_remove_all_identities(SocketEntry *e)
 
 	/* Mark that there are no identities. */
 	idtab->nentries = 0;
+}
+
+static void
+process_remove_all_identities(SocketEntry *e)
+{
+	debug2_f("entering");
+	remove_all_identities();
 
 	/* Send success. */
 	send_status(e, 1);
@@ -1342,6 +1351,13 @@ cleanup_handler(int sig)
 	_exit(2);
 }
 
+/*ARGSUSED*/
+static void
+remove_all_handler(int sig)
+{
+	remove_all_next_tick = 1;
+}
+
 static void
 check_parent_exists(void)
 {
@@ -1619,6 +1635,7 @@ skip:
 	ssh_signal(SIGINT, (d_flag | D_flag) ? cleanup_handler : SIG_IGN);
 	ssh_signal(SIGHUP, cleanup_handler);
 	ssh_signal(SIGTERM, cleanup_handler);
+	ssh_signal(SIGUSR1, remove_all_handler);
 
 	if (pledge("stdio rpath cpath unix id proc exec", NULL) == -1)
 		fatal("%s: pledge: %s", __progname, strerror(errno));
@@ -1630,7 +1647,11 @@ skip:
 		saved_errno = errno;
 		if (parent_alive_interval != 0)
 			check_parent_exists();
-		(void) reaper();	/* remove expired keys */
+		if (remove_all_next_tick) {
+			remove_all_next_tick = 0;
+			remove_all_identities();
+		} else
+			(void) reaper();	/* remove expired keys */
 		if (result == -1) {
 			if (saved_errno == EINTR)
 				continue;

--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)

Hello.

So please let me ping this once, i thought it is a good time, now
that OpenBSD 7.1 is out.

It is a small and pretty local change, and it incorporates the
comments of Darren Tucker (i finally understood what you were
thinking) in another now closed pull on github (being there again
for such things, but i did not read the docs..), the still open
pull request is [1].  Tests run.  (I will not mail again.)

Thanks for your consideration, and
Ciao from Germany already here.

  [1] https://github.com/openssh/openssh-portable/pull/297

From: Steffen Nurpmeso <steffen at sdaoden.eu>
Date: Fri, 21 Jan 2022 00:11:18 +0100
Subject: [PATCH] ssh-agent: remove all keys upon SIGUSR1..

With the advent of per-user temporary directories it became
hard for an administrator to remove all keys from all running
ssh-agent instances; what formerly could be done like so

   if command -v ssh-add >/dev/null 2>&1; then
      for a in /tmp/ssh-*/agent.*; do
         [ -e "$a" ] || continue
         act "SSH_AUTH_SOCK=\"$a\" ssh-add -D </dev/null
>/dev/null 2>&1 &"
         inc
      done
   fi

has become a major undertaking, especially with even more
containerization.  Being able to remove all keys from all agents
with a single command seems so desirable that it is available in
other agents in the software world.
---
 regress/agent.sh | 45 +++++++++++++++++++++++----------
 ssh-agent.1      |  3 +++
 ssh-agent.c      | 66 ++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 87 insertions(+), 27 deletions(-)

diff --git a/regress/agent.sh b/regress/agent.sh
index f187b67572..43d4bd0724 100644
--- a/regress/agent.sh
+++ b/regress/agent.sh
@@ -157,30 +157,49 @@ done
 
 ## Deletion tests.
 
+delete_cycle() {
+	# make sure they're gone
+	${SSHADD} -l > /dev/null 2>&1
+	r=$?
+	if [ $r -ne 1 ]; then
+		fail "ssh-add -l returned unexpected exit code: $r"
+	fi
+	trace "readd keys"
+	# re-add keys/certs to agent
+	for t in ${SSH_KEYTYPES}; do
+		${SSHADD} $OBJ/$t-agent-private >/dev/null 2>&1 || \
+			fail "ssh-add failed exit code $?"
+	done
+	# make sure they are there
+	${SSHADD} -l > /dev/null 2>&1
+	r=$?
+	if [ $r -ne 0 ]; then
+		fail "ssh-add -l failed: exit code $r"
+	fi
+}
+
 trace "delete all agent keys"
 ${SSHADD} -D > /dev/null 2>&1
 r=$?
 if [ $r -ne 0 ]; then
 	fail "ssh-add -D failed: exit code $r"
 fi
-# make sure they're gone
-${SSHADD} -l > /dev/null 2>&1
+delete_cycle
+
+trace "delete all agent keys via SIGUSR1"
+kill -USR1 $SSH_AGENT_PID >/dev/null 2>&1
 r=$?
-if [ $r -ne 1 ]; then
-	fail "ssh-add -l returned unexpected exit code: $r"
+if [ $r -ne 0 ]; then
+	fail "kill -USR1: exit code $r"
 fi
-trace "readd keys"
-# re-add keys/certs to agent
-for t in ${SSH_KEYTYPES}; do
-	${SSHADD} $OBJ/$t-agent-private >/dev/null 2>&1 || \
-		fail "ssh-add failed exit code $?"
-done
-# make sure they are there
-${SSHADD} -l > /dev/null 2>&1
+delete_cycle
+trace ".. and again"
+kill -USR1 $SSH_AGENT_PID >/dev/null 2>&1
 r=$?
 if [ $r -ne 0 ]; then
-	fail "ssh-add -l failed: exit code $r"
+	fail "kill -USR1: exit code $r"
 fi
+delete_cycle
 
 check_key_absent() {
 	${SSHADD} -L | grep "^$1 " >/dev/null
diff --git a/ssh-agent.1 b/ssh-agent.1
index ea43cd156b..7748293791 100644
--- a/ssh-agent.1
+++ b/ssh-agent.1
@@ -178,6 +178,9 @@ will automatically use them if present.
 is also used to remove keys from
 .Nm
 and to query the keys that are held in one.
+.Nm
+will remove all keys when it receives the user signal
+.Dv SIGUSR1 .
 .Pp
 Connections to
 .Nm
diff --git a/ssh-agent.c b/ssh-agent.c
index 03ae2b022e..ff091c3f8a 100644
--- a/ssh-agent.c
+++ b/ssh-agent.c
@@ -189,6 +189,9 @@ static int fingerprint_hash = SSH_FP_HASH_DEFAULT;
 /* Refuse signing of non-SSH messages for web-origin FIDO keys */
 static int restrict_websafe = 1;
 
+/* Request to remove_all_identities() pending */
+static volatile sig_atomic_t received_sigusr1 = 0;
+
 static void
 close_socket(SocketEntry *e)
 {
@@ -915,11 +918,10 @@ process_remove_identity(SocketEntry *e)
 }
 
 static void
-process_remove_all_identities(SocketEntry *e)
+remove_all_identities(void)
 {
 	Identity *id;
 
-	debug2_f("entering");
 	/* Loop over all identities and clear the keys. */
 	for (id = TAILQ_FIRST(&idtab->idlist); id;
 	    id = TAILQ_FIRST(&idtab->idlist)) {
@@ -929,6 +931,13 @@ process_remove_all_identities(SocketEntry *e)
 
 	/* Mark that there are no identities. */
 	idtab->nentries = 0;
+}
+
+static void
+process_remove_all_identities(SocketEntry *e)
+{
+	debug2_f("entering");
+	remove_all_identities();
 
 	/* Send success. */
 	send_status(e, 1);
@@ -1874,7 +1883,8 @@ after_poll(struct pollfd *pfd, size_t npfd, u_int maxfds)
 }
 
 static int
-prepare_poll(struct pollfd **pfdp, size_t *npfdp, int *timeoutp, u_int maxfds)
+prepare_poll(struct pollfd **pfdp, size_t *npfdp, struct timespec **ptimeoutpp,
+    u_int maxfds)
 {
 	struct pollfd *pfd = *pfdp;
 	size_t i, j, npfd = 0;
@@ -1936,17 +1946,17 @@ prepare_poll(struct pollfd **pfdp, size_t *npfdp, int
*timeoutp, u_int maxfds)
 			break;
 		}
 	}
+
+	/* This also removes expired keys */
 	deadline = reaper();
 	if (parent_alive_interval != 0)
 		deadline = (deadline == 0) ? parent_alive_interval :
 		    MINIMUM(deadline, parent_alive_interval);
-	if (deadline == 0) {
-		*timeoutp = -1; /* INFTIM */
-	} else {
-		if (deadline > INT_MAX / 1000)
-			*timeoutp = INT_MAX / 1000;
-		else
-			*timeoutp = deadline * 1000;
+	if (deadline == 0)
+		*ptimeoutpp = NULL; /* INFTIM */
+	else {
+		(*ptimeoutpp)->tv_nsec = 0;
+		(*ptimeoutpp)->tv_sec = deadline;
 	}
 	return (1);
 }
@@ -1981,6 +1991,13 @@ cleanup_handler(int sig)
 	_exit(2);
 }
 
+/*ARGSUSED*/
+static void
+onsigusr1(int sig)
+{
+	received_sigusr1 = 1;
+}
+
 static void
 check_parent_exists(void)
 {
@@ -2022,7 +2039,8 @@ main(int ac, char **av)
 	char pidstrbuf[1 + 3 * sizeof pid];
 	size_t len;
 	mode_t prev_mask;
-	int timeout = -1; /* INFTIM */
+	sigset_t psigset, psigseto;
+	struct timespec ptimeout, *ptimeoutp;
 	struct pollfd *pfd = NULL;
 	size_t npfd = 0;
 	u_int maxfds;
@@ -2258,18 +2276,38 @@ skip:
 	ssh_signal(SIGINT, (d_flag | D_flag) ? cleanup_handler : SIG_IGN);
 	ssh_signal(SIGHUP, cleanup_handler);
 	ssh_signal(SIGTERM, cleanup_handler);
+	ssh_signal(SIGUSR1, onsigusr1);
 
 	if (pledge("stdio rpath cpath unix id proc exec", NULL) == -1)
 		fatal("%s: pledge: %s", __progname, strerror(errno));
 	platform_pledge_agent();
 
+	/*
+	 * Prepare signal mask that we use to block signals that might set
+	 * received_sigusr1, so that we are guaranteed to immediately wake up
+	 * the ppoll if a signal is received after the flag is checked.
+	 */
+	sigemptyset(&psigset);
+	if (d_flag | D_flag)
+		sigaddset(&psigset, SIGINT);
+	sigaddset(&psigset, SIGHUP);
+	sigaddset(&psigset, SIGTERM);
+	sigaddset(&psigset, SIGUSR1);
+
 	while (1) {
-		prepare_poll(&pfd, &npfd, &timeout, maxfds);
-		result = poll(pfd, npfd, timeout);
+		sigprocmask(SIG_BLOCK, &psigset, &psigseto);
+		if (received_sigusr1) {
+			received_sigusr1 = 0;
+			remove_all_identities();
+		}
+		/* This also removes expired keys */
+		ptimeoutp = &ptimeout;
+		prepare_poll(&pfd, &npfd, &ptimeoutp, maxfds);
+		result = ppoll(pfd, npfd, ptimeoutp, &psigseto);
 		saved_errno = errno;
+		sigprocmask(SIG_SETMASK, &psigseto, NULL);
 		if (parent_alive_interval != 0)
 			check_parent_exists();
-		(void) reaper();	/* remove expired keys */
 		if (result == -1) {
 			if (saved_errno == EINTR)
 				continue;
-- 
2.35.3


--steffen
|
|Der Kragenbaer,                The moon bear,
|der holt sich munter           he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)