ssh-agent: perform AGENTC_REMOVE_ALL_IDENTITIES on SIGUSR1

Steffen Nurpmeso steffen at sdaoden.eu
Tue Apr 26 01:12:29 AEST 2022


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)


More information about the openssh-unix-dev mailing list