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

On 2025-01-10 01:35, Jochen Bern wrote:
> On 10.01.25 00:33, Corey Hickey wrote:
>> 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
> 
> Please don't. If you want to ever get people to load their privkeys
into
> the agent *with a limited lifetime*, having a trivial, *universal* way
> to check whether they have expired by now is an asset.
> 
>> workplace$ egrep ' ssh(|add)=' .bashrc
>> alias sshadd='( echo -n "`tput dim`" ; ssh-add -c -t 1800
; echo -n "`tput sgr0`" )'
>> alias ssh='ssh-add -l >/dev/null || sshadd ; ssh'
With my patch v2, that would need to be:

 > alias ssh='ssh-add -l | grep -q . || sshadd ; ssh'

...though the message "The agent has no identities." would be printed
to
stderr, for better or for worse. Perhaps that should require a higher 
log_level (via -v).


I can definitely see that there can be potential harm in changing 
default behavior, if people are relying on the current behavior. That's 
why my first patch did not change the default.

That said, I do think the current behavior is not optimal. In a general 
sense, when listing something, an empty list is a valid outcome. If the 
listing tool returns an error status after _successfully_ determining 
that the list is empty, then the caller cannot easily know whether the 
tool succeeded or was unable to determine the list.

For some precedence:

$ mkdir x ; ls x ; echo "ls: $?" ; find x -mindepth 1 ; echo
"find: $?"
ls: 0
find: 0
$ awk '/foo/' /etc/passwd ; echo "awk: $?"
awk: 0
$ sed -n '/foo/p' /etc/passwd ; echo "sed: $?"
sed: 0


Of course, I can't say that all tools work this way. Here are a couple 
that do not:

$ ps -u games ; echo "ps: $?" ; grep foo /etc/passwd ; echo
"grep: $?"
     PID TTY          TIME CMD
ps: 1
grep: 1

...but I do find it easier to work with listing-tools that consider an 
empty list to not be an error.


I can adjust and refine whichever approach the maintainers think is best.

Thanks,
Corey

On 10.01.25 18:27, Corey Hickey wrote:
> On 2025-01-10 01:35, Jochen Bern wrote:
>> Please don't. If you want to ever get people to load their privkeys
into
>> the agent *with a limited lifetime*, having a trivial, *universal* way
>> to check whether they have expired by now is an asset.
> 
> With my patch v2, that would need to be:
>  > alias ssh='ssh-add -l | grep -q . || sshadd ; ssh'
Which admittedly would still be "trivial", but there's a reason
why I
stressed "universal". (I'm sort of the SSH/tunneling/crypto-parms
guru
here and my SSH configs/templates tend to percolate throughout the IT 
and support dpt.s, whatever their local OpenSSH version happens to be.)

Of course, if OpenSSH executables could output further 
(version-dependent) Best Current Practices on their own usage in a shell 
(script), like "ssh-agent -s" does ... ;-)
> ...though the message "The agent has no identities." would be
printed to
> stderr, for better or for worse. Perhaps that should require a higher 
> log_level (via -v).
(Nothing a "2>&1" or "2>/dev/null" on top
couldn't fix, FWIW.)
> That said, I do think the current behavior is not optimal. In a general 
> sense, when listing something, an empty list is a valid outcome. If the 
> listing tool returns an error status after _successfully_ determining 
> that the list is empty, then the caller cannot easily know whether the 
> tool succeeded or was unable to determine the list.
I can see that, but I wouldn't consider it good enough a reason on its 
own to break backwards compatibility ...

(FWIW, "grep" is technically not outputting a list, but a list of
lists
- one list of matches for every file named in the params - and offers 
condensing this into a plain list again, think "grep -l $USER /etc/*".
I'd guess that further condensing that into a single exit status made 
sense in a "someone *will* find this useful" way. Which raises the 
question whether the currently *plain* list output by "ssh-add -l"
might
see subcategories in the future - say, per providers or constraints ...)

On 10.01.25 18:57, Jim Knoble wrote:
> When Damien wrote:
> 
>> Adding a new exit status for the
>> no-keys-in-agent case would be
>> acceptable too I think.
> 
> I interpreted that as "make ssh-add exit with status 2 or 3 or 99, for
example, as opposed to 1".
> 
> That is differentiate between:
> 
> - There is an agent, and it has keys, and ssh-add listed them (exit status
0).
> - There is no agent, or there is a problem communicating with the agent
(exit status 1).
> - There is an agent, but it has no keys (exit status 2, for example).
Agreed - with the minor caveat that IIUC higher values tend to get 
assigned to the *more* severe failure.

Kind regards,
-- 
Jochen Bern
Systemingenieur

Binect GmbH
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4336 bytes
Desc: S/MIME Cryptographic Signature
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20250110/f737edfd/attachment-0001.p7s>

On Fri, 10 Jan 2025, Corey Hickey wrote:
> On 2025-01-10 01:35, Jochen Bern wrote:
> > On 10.01.25 00:33, Corey Hickey wrote:
> > > 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
> > 
> > Please don't. If you want to ever get people to load their
privkeys into
> > the agent *with a limited lifetime*, having a trivial, *universal* way
> > to check whether they have expired by now is an asset.
> > 
> > > workplace$ egrep ' ssh(|add)=' .bashrc
> > > alias sshadd='( echo -n "`tput dim`" ; ssh-add -c
-t 1800 ; echo -n "`tput
> > > sgr0`" )'
> > > alias ssh='ssh-add -l >/dev/null || sshadd ; ssh'
> 
> With my patch v2, that would need to be:
> 
> > alias ssh='ssh-add -l | grep -q . || sshadd ; ssh'
> 
> ...though the message "The agent has no identities." would be
printed to
> stderr, for better or for worse. Perhaps that should require a higher
> log_level (via -v).
Are you aware of ssh's AddKeysToAgent option? It seems to already do
what you're trying to implement here.

-d