openssh unix dev - Jan 2025 - [PATCH] ssh-add: support parser-friendly operation

From: Corey Hickey <chickey at tagged.com>

When ssh-add is used in a script like:

    if ! KEY_LISTING=$(ssh-add -l 2>&1) ; then
        echo "SSH agent error" >&2
        exit 2
    fi

...the operation fails when there is an agent but there are no keys in
the agent. This is because ssh-add exits with status of 1. If the
intent is to examine the keys in the agent, then this behavior is
undesired and not easily distinguishable from an error (e.g. no agent
running).

To address this, add a new option -p to make ssh-add behavior more
friendly to parsing.
---
 ssh-add.1 |  4 ++++
 ssh-add.c | 13 +++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/ssh-add.1 b/ssh-add.1
index c31de4dd9..80fdfcbcc 100644
--- a/ssh-add.1
+++ b/ssh-add.1
@@ -223,6 +223,10 @@ Lists public key parameters of all identities currently
represented
 by the agent.
 .It Fl l
 Lists fingerprints of all identities currently represented by the agent.
+.It Fl p
+Behave in a more parsing-friendly fashion: when listing public keys or
+fingerprints, and there are none in the agent, print nothing and exit with a
+status of 0.
 .It Fl q
 Be quiet after a successful operation.
 .It Fl S Ar provider
diff --git a/ssh-add.c b/ssh-add.c
index 0035cb84a..889decc0d 100644
--- a/ssh-add.c
+++ b/ssh-add.c
@@ -529,7 +529,7 @@ test_key(int agent_fd, const char *filename)
 }
 
 static int
-list_identities(int agent_fd, int do_fp)
+list_identities(int agent_fd, int do_fp, int parse_friendly)
 {
 	char *fp;
 	int r;
@@ -541,6 +541,8 @@ list_identities(int agent_fd, int do_fp)
 		if (r != SSH_ERR_AGENT_NO_IDENTITIES)
 			fprintf(stderr, "error fetching identities: %s\n",
 			    ssh_err(r));
+		else if (parse_friendly)
+			return 0; /* no identities; nothing to do */
 		else
 			printf("The agent has no identities.\n");
 		return -1;
@@ -814,7 +816,7 @@ main(int argc, char **argv)
 	char **dest_constraint_strings = NULL, **hostkey_files = NULL;
 	int r, i, ch, deleting = 0, ret = 0, key_only = 0, cert_only = 0;
 	int do_download = 0, xflag = 0, lflag = 0, Dflag = 0;
-	int qflag = 0, Tflag = 0;
+	int pflag = 0, qflag = 0, Tflag = 0;
 	SyslogFacility log_facility = SYSLOG_FACILITY_AUTH;
 	LogLevel log_level = SYSLOG_LEVEL_INFO;
 	struct sshkey *k, **certs = NULL;
@@ -846,7 +848,7 @@ main(int argc, char **argv)
 
 	skprovider = getenv("SSH_SK_PROVIDER");
 
-	while ((ch = getopt(argc, argv, "vkKlLCcdDTxXE:e:h:H:M:m:qs:S:t:"))
!= -1) {
+	while ((ch = getopt(argc, argv, "vkKlLCcdDTxXE:e:h:H:M:m:pqs:S:t:"))
!= -1) {
 		switch (ch) {
 		case 'v':
 			if (log_level == SYSLOG_LEVEL_INFO)
@@ -935,6 +937,9 @@ main(int argc, char **argv)
 		case 'T':
 			Tflag = 1;
 			break;
+		case 'p':
+			pflag = 1;
+			break;
 		default:
 			usage();
 			ret = 1;
@@ -950,7 +955,7 @@ main(int argc, char **argv)
 			ret = 1;
 		goto done;
 	} else if (lflag) {
-		if (list_identities(agent_fd, lflag == 'l' ? 1 : 0) == -1)
+		if (list_identities(agent_fd, lflag == 'l' ? 1 : 0, pflag) == -1)
 			ret = 1;
 		goto done;
 	} else if (Dflag) {
-- 
2.47.1

On 2025-01-09 15:27, Corey Hickey wrote:
> From: Corey Hickey <chickey at tagged.com>
> 
> When ssh-add is used in a script like:
> 
>      if ! KEY_LISTING=$(ssh-add -l 2>&1) ; then
>          echo "SSH agent error" >&2
>          exit 2
>      fi
> 
> ...the operation fails when there is an agent but there are no keys in
> the agent. This is because ssh-add exits with status of 1. If the
> intent is to examine the keys in the agent, then this behavior is
> undesired and not easily distinguishable from an error (e.g. no agent
> running).
> 
> To address this, add a new option -p to make ssh-add behavior more
> friendly to parsing.
I took the approach of preserving current behavior by default, but 
another approach would be to:

* print "The agent has no identities." to stderr instead of stdout
* exit with a status of 0 instead of 1

If that alternate approach would be better, please let me know and I
will send a new patch.

Thank you,
Corey

From: Corey Hickey <chickey at tagged.com>

When ssh-add is used in a script like:

    if ! KEY_LISTING=$(ssh-add -l 2>&1) ; then
        echo "SSH agent error" >&2
        exit 2
    fi

...the operation fails when there is an agent but there are no keys in
the agent. This is because ssh-add exits with status of 1. If the
intent is to examine the keys in the agent, then this behavior is
undesired and not easily distinguishable from an error (e.g. no agent
running).

To address this, modify ssh-add to:
* print "The agent has no identities." to stderr instead of stdout
* exit with a status of 0 instead of 1

Also modify tests accordingly, so `make tests` passes.
---
 regress/agent-restrict.sh |  4 ++--
 regress/agent-timeout.sh  |  5 +----
 regress/agent.sh          | 47 +++++++++++++++++----------------------
 regress/test-exec.sh      | 22 ++++++++++++++++++
 ssh-add.c                 | 10 +++++----
 5 files changed, 51 insertions(+), 37 deletions(-)

diff --git a/regress/agent-restrict.sh b/regress/agent-restrict.sh
index 62cea8522..9d3fd71ac 100644
--- a/regress/agent-restrict.sh
+++ b/regress/agent-restrict.sh
@@ -204,8 +204,8 @@ trap "kill $AGENT_PID" EXIT
 sleep 4 # Give it a chance to start
 # Check that it's running.
 ${SSHADD} -l > /dev/null 2>&1
-if [ $? -ne 1 ]; then
-	fail "ssh-add -l did not fail with exit code 1"
+if [ $? -ne 0 ]; then
+	fail "ssh-add -l did not fail with exit code 0"
 fi
 
 verbose "authentication with agent (no restrict)"
diff --git a/regress/agent-timeout.sh b/regress/agent-timeout.sh
index 6dec09285..1c11c5da6 100644
--- a/regress/agent-timeout.sh
+++ b/regress/agent-timeout.sh
@@ -28,10 +28,7 @@ else
 	trace "sleeping 2*${SSHAGENT_TIMEOUT} seconds"
 	sleep ${SSHAGENT_TIMEOUT}
 	sleep ${SSHAGENT_TIMEOUT}
-	${SSHADD} -l 2> /dev/null | grep 'The agent has no identities.'
>/dev/null
-	if [ $? -ne 0 ]; then
-		fail "ssh-add -l still returns keys after timeout"
-	fi
+	check_keys_absent "after timeout"
 
 	trace "kill agent"
 	${SSHAGENT} -k > /dev/null
diff --git a/regress/agent.sh b/regress/agent.sh
index f0022aca5..c33e8407e 100644
--- a/regress/agent.sh
+++ b/regress/agent.sh
@@ -22,8 +22,9 @@ if [ $r -ne 0 ]; then
 fi
 
 ${SSHADD} -l > /dev/null 2>&1
-if [ $? -ne 1 ]; then
-	fail "ssh-add -l did not fail with exit code 1"
+r=$?
+if [ $r -ne 0 ]; then
+	fail "with no keys, ssh-add -l failed: exit code $r"
 fi
 
 rm -f $OBJ/user_ca_key $OBJ/user_ca_key.pub
@@ -99,11 +100,15 @@ for t in ${SSH_KEYTYPES}; do
 done
 
 trace "agent forwarding"
-${SSH} -A -F $OBJ/ssh_proxy somehost ${SSHADD} -l > /dev/null 2>&1
+AGENT_LISTING=$(${SSH} -A -F $OBJ/ssh_proxy somehost ${SSHADD} -l 2>
/dev/null)
 r=$?
 if [ $r -ne 0 ]; then
 	fail "ssh-add -l via agent fwd failed (exit code $r)"
 fi
+NUM_LINES=$(printf "%s" "$AGENT_LISTING" | wc -l)
+if [ "$NUM_LINES" -eq 0 ]; then
+	fail "ssh-add -l via agent fwd has no keys"
+fi
 ${SSH} "-oForwardAgent=$SSH_AUTH_SOCK" -F $OBJ/ssh_proxy somehost
${SSHADD} -l > /dev/null 2>&1
 r=$?
 if [ $r -ne 0 ]; then
@@ -128,16 +133,20 @@ if [ $r -ne 0 ]; then
 	fail "ssh-add -l via agent path env fwd of different agent failed (exit
code $r)"
 fi
 
-# Remove keys from forwarded agent, ssh-add on remote machine should now fail.
+# Remove keys from forwarded agent, then test for keys on remote machine.
 SSH_AUTH_SOCK=$FW_SSH_AUTH_SOCK ${SSHADD} -D > /dev/null 2>&1
 r=$?
 if [ $r -ne 0 ]; then
 	fail "ssh-add -D failed: exit code $r"
 fi
-${SSH} '-oForwardAgent=$FW_SSH_AUTH_SOCK' -F $OBJ/ssh_proxy somehost
${SSHADD} -l > /dev/null 2>&1
+AGENT_LISTING=$(${SSH} '-oForwardAgent=$FW_SSH_AUTH_SOCK' -F
$OBJ/ssh_proxy somehost ${SSHADD} -l 2> /dev/null)
 r=$?
-if [ $r -ne 1 ]; then
-	fail "ssh-add -l with different agent did not fail with exit code 1 (exit
code $r)"
+if [ $r -ne 0 ]; then
+	fail "ssh-add -l via agent fwd failed (exit code $r)"
+fi
+NUM_LINES=$(printf "%s" "$AGENT_LISTING" | wc -l)
+if [ "$NUM_LINES" -ne 0 ]; then
+	fail "ssh-add -l via agent fwd still has keys"
 fi
 
 (printf 'cert-authority,principals="estragon" '; cat
$OBJ/user_ca_key.pub) \
@@ -164,11 +173,7 @@ 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
-r=$?
-if [ $r -ne 1 ]; then
-	fail "ssh-add -l returned unexpected exit code: $r"
-fi
+check_keys_absent "after delete"
 trace "readd keys"
 # re-add keys/certs to agent
 for t in ${SSH_KEYTYPES}; do
@@ -176,11 +181,7 @@ for t in ${SSH_KEYTYPES}; do
 		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
+check_keys_present "after readd"
 trace "delete all agent keys using SIGUSR1"
 kill -s USR1 $SSH_AGENT_PID
 r=$?
@@ -188,22 +189,14 @@ if [ $r -ne 0 ]; then
 	fail "kill -s USR1 failed: exit code $r"
 fi
 # 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
+check_keys_absent "after USR1"
 # 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
+check_keys_present "after readd again"
 
 check_key_absent() {
 	${SSHADD} -L | grep "^$1 " >/dev/null
diff --git a/regress/test-exec.sh b/regress/test-exec.sh
index 62c00fd8c..89170657e 100644
--- a/regress/test-exec.sh
+++ b/regress/test-exec.sh
@@ -495,6 +495,28 @@ stop_sshd ()
 	PIDFILE=""
 }
 
+_check_keys_present() {
+	AGENT_LISTING=$(${SSHADD} -l 2> /dev/null)
+	r=$?
+	if [ $r -ne 0 ]; then
+		fail "$1: ssh-add -l returned unexpected exit code: $r"
+	fi
+	NUM_LINES=$(printf "%s" "$AGENT_LISTING" | wc -l)
+	[ "$NUM_LINES" -ne 0 ]
+}
+
+check_keys_absent() {
+	if _check_keys_present "$1" ; then
+		fail "$1: keys unexpectedly present"
+	fi
+}
+
+check_keys_present() {
+	if ! _check_keys_present "$1" ; then
+		fail "$1: keys unexpectedly absent"
+	fi
+}
+
 # helper
 cleanup ()
 {
diff --git a/ssh-add.c b/ssh-add.c
index 0035cb84a..8194a1649 100644
--- a/ssh-add.c
+++ b/ssh-add.c
@@ -538,12 +538,14 @@ list_identities(int agent_fd, int do_fp)
 	size_t i;
 
 	if ((r = ssh_fetch_identitylist(agent_fd, &idlist)) != 0) {
-		if (r != SSH_ERR_AGENT_NO_IDENTITIES)
+		if (r == SSH_ERR_AGENT_NO_IDENTITIES) {
+			fprintf(stderr, "The agent has no identities.\n");
+			return 0;
+		} else {
 			fprintf(stderr, "error fetching identities: %s\n",
 			    ssh_err(r));
-		else
-			printf("The agent has no identities.\n");
-		return -1;
+			return -1;
+		}
 	}
 	for (i = 0; i < idlist->nkeys; i++) {
 		if (do_fp) {
-- 
2.47.1