openssh unix dev - Sep 2015 - [RFC][PATCH v2] Support a list of sockets on SSH_AUTH_SOCK

The idea behind this change is to add support for different
"ssh-agents"
being able to run at the same time. It does not change the current
behaviour of the ssh-agent (which will set SSH_AUTH_SOCK just for
itself). Neither does it change the behaviour of SSH_AGENT_PID (which
still supports only one pid).
The new implementation will go through the list of sockets (which are
separated by a colon (:)), and will return the very first functional
one. An example of the new supported syntax is:
SSH_AUTH_SOCK=/run/user/1000/spice/ssh:/tmp/ssh-hHomdONwQus6/agent.6907

The idea has been discussed a little in this e-mail thread:
http://lists.mindrot.org/pipermail/openssh-unix-dev/2015-September/034381.html

Signed-off-by: Fabiano Fid?ncio <fidencio at redhat.com>
---
Changes since v1:
- Fix a typo in the commit (SSH_AUTH_SOCKET -> SSH_AUTH_SOCK)
---
 authfd.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/authfd.c b/authfd.c
index 12bf125..20fcba2 100644
--- a/authfd.c
+++ b/authfd.c
@@ -83,21 +83,12 @@ decode_reply(u_char type)
 		return SSH_ERR_INVALID_FORMAT;
 }
 
-/* Returns the number of the authentication fd, or -1 if there is none. */
-int
-ssh_get_authentication_socket(int *fdp)
+static int
+get_authentication_socket(const char *authsocket, int *fdp)
 {
-	const char *authsocket;
 	int sock, oerrno;
 	struct sockaddr_un sunaddr;
 
-	if (fdp != NULL)
-		*fdp = -1;
-
-	authsocket = getenv(SSH_AUTHSOCKET_ENV_NAME);
-	if (!authsocket)
-		return SSH_ERR_AGENT_NOT_PRESENT;
-
 	memset(&sunaddr, 0, sizeof(sunaddr));
 	sunaddr.sun_family = AF_UNIX;
 	strlcpy(sunaddr.sun_path, authsocket, sizeof(sunaddr.sun_path));
@@ -117,7 +108,32 @@ ssh_get_authentication_socket(int *fdp)
 		*fdp = sock;
 	else
 		close(sock);
-	return 0;
+	return SSH_ERR_SUCCESS;
+}
+
+/* Returns the number of the authentication fd, or -1 if there is none. */
+int
+ssh_get_authentication_socket(int *fdp)
+{
+	const char *authsocketlist;
+	const char *authsocket;
+	int rc;
+
+	if (fdp != NULL)
+		*fdp = -1;
+
+	authsocketlist = getenv(SSH_AUTHSOCKET_ENV_NAME);
+	if (!authsocketlist)
+		return SSH_ERR_AGENT_NOT_PRESENT;
+
+	authsocket = strtok((char *)authsocketlist, ":");
+
+	do {
+		rc = get_authentication_socket(authsocket, fdp);
+		authsocket = strtok(NULL, ":");
+	} while (rc != SSH_ERR_SUCCESS && authsocket != NULL);
+
+	return rc;
 }
 
 /* Communicate with agent: send request and read reply */
-- 
2.4.3

On Fri, Sep 25, 2015 at 9:41 PM, Fabiano Fid?ncio <fidencio at redhat.com>
wrote:
> The idea behind this change is to add support for different
"ssh-agents"
> being able to run at the same time. It does not change the current
> behaviour of the ssh-agent (which will set SSH_AUTH_SOCK just for
> itself). Neither does it change the behaviour of SSH_AGENT_PID (which
> still supports only one pid).
Conceptually, it seems reasonable. But I'd recommend being very, very
careful with environment parsing between multiple old and new versions
of client, agent, and server..

As a purely practical and local approach, I personally tend to use
multiple perl "keychain" tool commands.

             # keycain       # Leaves sourceable ssh-agent config in
$HOME/.keychain/$HOSTNAME.sh
             # HOSTNAME=github keychain     # Leaves sourceable
ssh-agent config in $HOME/.keychin/github.sh
             # HOSTNAME=work keychain     # Leaves sourceable
ssh-agent config for work keys in $HOME/.keychain/work.sh

Then I can source and enable keys for the keychain as desired, and
switch among them. It's not perfect, but it lets me switch from one
keychain to the other for work related github keys, personal github
keys, root keys, personal keys, etc. and only have the relevant ones
in a particular shell session.

> The new implementation will go through the list of sockets (which are
> separated by a colon (:)), and will return the very first functional
> one. An example of the new supported syntax is:
> SSH_AUTH_SOCK=/run/user/1000/spice/ssh:/tmp/ssh-hHomdONwQus6/agent.6907
>
> The idea has been discussed a little in this e-mail thread:
>
http://lists.mindrot.org/pipermail/openssh-unix-dev/2015-September/034381.html
>
> Signed-off-by: Fabiano Fid?ncio <fidencio at redhat.com>
> ---
> Changes since v1:
> - Fix a typo in the commit (SSH_AUTH_SOCKET -> SSH_AUTH_SOCK)
> ---
>  authfd.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/authfd.c b/authfd.c
> index 12bf125..20fcba2 100644
> --- a/authfd.c
> +++ b/authfd.c
> @@ -83,21 +83,12 @@ decode_reply(u_char type)
>                 return SSH_ERR_INVALID_FORMAT;
>  }
>
> -/* Returns the number of the authentication fd, or -1 if there is none. */
> -int
> -ssh_get_authentication_socket(int *fdp)
> +static int
> +get_authentication_socket(const char *authsocket, int *fdp)
>  {
> -       const char *authsocket;
>         int sock, oerrno;
>         struct sockaddr_un sunaddr;
>
> -       if (fdp != NULL)
> -               *fdp = -1;
> -
> -       authsocket = getenv(SSH_AUTHSOCKET_ENV_NAME);
> -       if (!authsocket)
> -               return SSH_ERR_AGENT_NOT_PRESENT;
> -
>         memset(&sunaddr, 0, sizeof(sunaddr));
>         sunaddr.sun_family = AF_UNIX;
>         strlcpy(sunaddr.sun_path, authsocket, sizeof(sunaddr.sun_path));
> @@ -117,7 +108,32 @@ ssh_get_authentication_socket(int *fdp)
>                 *fdp = sock;
>         else
>                 close(sock);
> -       return 0;
> +       return SSH_ERR_SUCCESS;
> +}
> +
> +/* Returns the number of the authentication fd, or -1 if there is none. */
> +int
> +ssh_get_authentication_socket(int *fdp)
> +{
> +       const char *authsocketlist;
> +       const char *authsocket;
> +       int rc;
> +
> +       if (fdp != NULL)
> +               *fdp = -1;
> +
> +       authsocketlist = getenv(SSH_AUTHSOCKET_ENV_NAME);
> +       if (!authsocketlist)
> +               return SSH_ERR_AGENT_NOT_PRESENT;
> +
> +       authsocket = strtok((char *)authsocketlist, ":");
> +
> +       do {
> +               rc = get_authentication_socket(authsocket, fdp);
> +               authsocket = strtok(NULL, ":");
> +       } while (rc != SSH_ERR_SUCCESS && authsocket != NULL);
> +
> +       return rc;
>  }
>
>  /* Communicate with agent: send request and read reply */
> --
> 2.4.3
>
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev at mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

On 2015-09-26T03:47, Fabiano Fid?ncio <fidencio at redhat.com>
wrote:
> The idea behind this change is to add support for different
"ssh-agents"
> being able to run at the same time. It does not change the current
> behaviour of the ssh-agent (which will set SSH_AUTH_SOCK just for
> itself). Neither does it change the behaviour of SSH_AGENT_PID (which
> still supports only one pid).
> The new implementation will go through the list of sockets (which are
> separated by a colon (:)), and will return the very first functional
> one. An example of the new supported syntax is:
> SSH_AUTH_SOCK=/run/user/1000/spice/ssh:/tmp/ssh-hHomdONwQus6/agent.6907
I think changing the semantics of SSH_AUTH_SOCK may be problematic. I'm
currently using a few scripts that create a socket per X display, named
like '/path/somewhere/:17.agent'. The choice of ':' as a
separator would
of course break those scripts.

While my personal problem described above is easily fixable, I think the
bigger picture is: No choice[0] of separator character is possible that
won't break existing usage. Therefore I'd rather suggest introducing a
separate SSH_AUTH_SOCK_FALLBACKS environment in addition to
SSH_AUTH_SOCK. SSH_AUTH_SOCK_FALLBACKS would then be used as the list of
fallbacks if SSH_AUTH_SOCK is not working currently.

Another advantage of a separate environment variable is that existing
scripts and programs that replace or alter SSH_AUTH_SOCK won't interfere
with it and won't need to be changed. 


Ciao,

Alexander Wuerstlein.


[0] all whitespace like \n and \t would break some shellscript
    somewhere, simple spaces are sometimes used for directory names
    (think 'Program Files' or 'Application Data') and
nonprintable ASCII
    characters would be even more of a pain to work with

Alexander,

On Sun, Sep 27, 2015 at 4:45 AM, Alexander Wuerstlein <arw at cs.fau.de>
wrote:
> On 2015-09-26T03:47, Fabiano Fid?ncio <fidencio at redhat.com> wrote:
>> The idea behind this change is to add support for different
"ssh-agents"
>> being able to run at the same time. It does not change the current
>> behaviour of the ssh-agent (which will set SSH_AUTH_SOCK just for
>> itself). Neither does it change the behaviour of SSH_AGENT_PID (which
>> still supports only one pid).
>> The new implementation will go through the list of sockets (which are
>> separated by a colon (:)), and will return the very first functional
>> one. An example of the new supported syntax is:
>> SSH_AUTH_SOCK=/run/user/1000/spice/ssh:/tmp/ssh-hHomdONwQus6/agent.6907
>
> I think changing the semantics of SSH_AUTH_SOCK may be problematic. I'm
> currently using a few scripts that create a socket per X display, named
> like '/path/somewhere/:17.agent'. The choice of ':' as a
separator would
> of course break those scripts.
Your point really make sense.
This is the first approach that came to my mind and could be
acceptable by the community (according to the discussions I linked in
the email).
But seems that now we have a better option ...
>
> While my personal problem described above is easily fixable, I think the
> bigger picture is: No choice[0] of separator character is possible that
> won't break existing usage. Therefore I'd rather suggest
introducing a
> separate SSH_AUTH_SOCK_FALLBACKS environment in addition to
> SSH_AUTH_SOCK. SSH_AUTH_SOCK_FALLBACKS would then be used as the list of
> fallbacks if SSH_AUTH_SOCK is not working currently.
... because I this idea sounds better than the initial approach.
OTOH, we still have the problem about the separator as using a colon
would break your fallbacks as well.
Do you have some suggestion about this? Or as it is a new env var we
can just warn the users and then they will have enough time for
changing their scripts (like in your case)?

Best Regards,
---
Fabiano Fid?ncio

> > The idea behind this change is to add support for different
"ssh-agents"
> > being able to run at the same time. It does not change the current
> > behaviour of the ssh-agent (which will set SSH_AUTH_SOCK just for
> > itself). Neither does it change the behaviour of SSH_AGENT_PID (which
> > still supports only one pid).
> 
> Conceptually, it seems reasonable. But I'd recommend being very, very
> careful with environment parsing between multiple old and new versions
> of client, agent, and server..
IMO having another environment variable with similar meaning is not a good 
design. In shell scripts it will be left alone, so having another ssh-agent 
active by error, and similar things.


Well, I can offer a few ideas.

One is to use the ":" separator, like in $PATH. Yes, it got discarded
for
various reasons in the other thread; yes, X11 uses that for display names, 
but observe:

    $ echo $DISPLAY
    :0
    $ ls -la /tmp/.X11-unix/X0
    srwxrwxrwx 1 root root 0 Sep 26 22:36 /tmp/.X11-unix/X0

Although the display has a ":" in it, the socket in the filesystem
doesn't;
so I guess that scripts wanting to store a SSH agent per-display (instead 
of per-user) can get that working, too.

Whitespace (with a fixed set, eg. space and tab - not any 'whitespace' 
unicode points) would be another idea, but see IFS, quoting, etc.


The second idea is to have $SSH_AUTH_SOCK point to a *directory*, and to 
use the sockets in there in ASCII alphanumeric order - so the default agent 
would register itself with as
"/tmp/ssh-<random>/500-agent.8903", and other
agents could move themselved earlier or later in the list.

The third idea is similar: keep pointing to a file, but look at all 
glob("$SSH_AUTH_SOCK*") sockets in there, in ASCII alphanumeric order 
again.


Or, the other idea from the original question - have an agent push queries 
to the "previous" agent as a fallback.


I'd prefer the last one - because it transparently works with all programs 
that know how to access *one* agent socket (like some java implementations, 
etc.), followed by 3,2, and 1, I guess - although it doesn't matter as much
with these any more.

People,

I've filed a bug[0] about this topic and hope we can keep the discussion
there.

[0]: https://bugzilla.mindrot.org/show_bug.cgi?id=2480

Best Regards,
--
Fabiano Fid?nncio