Steffen Nurpmeso
2021-Sep-08 11:46 UTC
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
2021-Sep-08 22:05 UTC
ssh-agent: perform AGENTC_REMOVE_ALL_IDENTITIES on SIGUSR1
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)
Steffen Nurpmeso
2022-Apr-25 15:12 UTC
ssh-agent: perform AGENTC_REMOVE_ALL_IDENTITIES on SIGUSR1
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] 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)