Sending envvars via ssh agent protocol

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Sending envvars via ssh agent protocol

GnuPG - Dev mailing list
Hi!

[ I sent this mail to the OpenSSH list openssh-unix-dev at mindrot.org
  but given that this is GnuPG related it should go here as well. ]

There are quite some folks out there who use GnuPG's implementation of
the ssh-agent which we implemented about 15 years ago.  It nicely fits
into the OpenPGP framework and we even have support for several
smartcards and tokens.  In fact the standard OpenPGP card is be default
created with an authentication key to be used with ssh.

So far, so good.  There is one annoying thing which we can only properly
solve by adding code to ssh.  The problem is that if you switch between
different X-servers or ttys, gpg-agent does not know where to popup the
passphrase or PIN entry dialog.  For example I am either working on
laptop directly or using an X server to work on that laptop.  So when
switching between these devices I am meanwhile very accustomed to run
the command "gpg-connect-agent updatestartuptty /bye" to tell gpg-agent
the default tty or display it shall use by default.  With gpg etc the
default is not used because gpg tells gpg-agent via its own IPC a number
of envvar values.

It would be very cool to get rid of this and so I hacked gpg-agent and
openssh to convet the required envvars via the ssh agent protocols
(according to draft-miller-ssh-agent-04 which is expired, but who
cares).

The new extension mechanism from this protocol is used; the details
should be easyl available from the attached patch.  However, I can
describe them in another post.

The visisble change in ssh is a new option:

  AgentEnv
     
    Specifies what variables from the local environ(7) should be sent to
    a running ssh-agent(1).  The agent may use these environment
    variables at its own discretion.  Note that patterns for the
    variable names are not supported.  To empty the list of previously
    set AgentEnv variable names the special name "-" may be used.  To
    ignore all further set names use the special name "#".  To ask the
    agent for a list of names to send use "auto" as the first and only
    item.

    The default is not to send any environment variables to the agent.

The rationale for the "-" thingy is to allow a config file to override
what for example the command line has already set.  The "#" can be used
to disable a globally set option from the commandline or ~/.ssh/config.
On a GnuPG system you would usually have

  AgentEnv auto

in ssh_config.  "auto" reads the envvars known by GnuPG and sends their
values back.  This is easier than to list them as arguments to AgentEnv.
GnuPG from Git is required but if things go smoothly we may even
backport this to the stable GnuPG 2.2 version.

I have not implemented that feature yet for ssh-add and ssh-keygen
because both don't parse ssh_config and thus this needs more thinking.
Anyway for everydays use it is enough to have this in ssh.

Please let me know whether this patch (against yesterday's Git) might be
acceptable to be included into the portable or upstream OpenSSH version.
Comments on the code are also appreciated.  I merely followed the
existing style.  I noticed that there are some ways to improve it but
that might me more intrusive as this change.


Salam-Shalom,

   Werner


--
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.

From 3a9a092530396d66d91059efd3737f9dae5051be Mon Sep 17 00:00:00 2001
From: Werner Koch <[hidden email]>
Date: Mon, 25 Jan 2021 10:30:04 +0100
Subject: [PATCH] Allow sending envrionment variables to the agent.

* authfd.c (put_one_env, ssh_send_agent_env): New.
* authfd.h (SSH_AGENTC_EXTENSION): New.
(SSH_AGENT_EXTENSION_FAILURE): New.
* readconf.h (Options): Add fields no_more_agent_env, num_agent_env,
agent_env.
* readconf.c (OpCodes, keywords): Add oAgentEnv and "agentenv".
(free_options): Move macro FREE_ARRAY to outer scope.  Free agent_env.
(process_config_line_depth): Parse oAgentEnv.
(initialize_options): Reset AgentEnv stuff.
(dump_client_config): Dump AgentEnv.
* sshconnect.c (maybe_add_key_to_agent): Call ssh_send_agent_env.
* sshconnect2.c (authentication_socket): New.
(pubkey_prepare): Call ssh_send_agent_env via new func.
--

This features comes handy with recent GnuPG versions and avoids the
long standing use of

  gpg-connect-agent updatestartuptty /bye

to tell gpg-agent that ssh is now used on the current X-server or tty.

The ssh-agent mechanism is described in
https://tools.ietf.org/html/draft-miller-ssh-agent-04
---
 authfd.c      | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 authfd.h      |  3 ++
 readconf.c    | 71 +++++++++++++++++++++++++++++++++++------
 readconf.h    |  4 +++
 ssh-add.c     |  4 +++
 ssh-keygen.c  |  2 ++
 ssh_config    |  3 ++
 ssh_config.5  | 17 ++++++++++
 sshconnect.c  |  6 ++++
 sshconnect2.c | 25 ++++++++++++++-
 10 files changed, 213 insertions(+), 10 deletions(-)

diff --git a/authfd.c b/authfd.c
index 189ebb39..b425f86f 100644
--- a/authfd.c
+++ b/authfd.c
@@ -189,6 +189,94 @@ ssh_close_authentication_socket(int sock)
  close(sock);
 }
 
+
+/* Helper for ssh_send_agent_env.  */
+static int
+put_one_env (struct sshbuf *msg, const char *name)
+{
+        int r;
+        const char *val;
+
+        val = getenv (name);
+        if ((r = sshbuf_put_cstring(msg, name)) == 0) {
+                if (val)
+                        r = sshbuf_put_cstring(msg, val);
+                else
+                        r = sshbuf_put_string(msg, "", 1);
+        }
+        return r;
+}
+
+/* Send configured envvars to the agent.  Returns an error code. */
+int
+ssh_send_agent_env (int sock, char **env, int num_env)
+{
+ struct sshbuf *msg;
+ int i, r;
+ u_char type;
+        char *p, *ptr;
+        char *namelist = NULL;
+
+        if (num_env < 1)
+                return 0;  /* None configured.  */
+
+ if ((msg = sshbuf_new()) == NULL)
+ return SSH_ERR_ALLOC_FAIL;
+
+        if (num_env == 1 && !strcmp (env[0], "auto")) {
+                /* If only one variable named "auto" is defined, query
+                 * the supported variables from the agent.  */
+                if ((r = sshbuf_put_u8(msg, SSH_AGENTC_EXTENSION)) != 0 ||
+                    (r = sshbuf_put_cstring(msg,"[hidden email]"))!= 0)
+                        goto out;
+                if ((r = ssh_request_reply(sock, msg, msg)) != 0)
+                        goto out;
+                if ((r = sshbuf_get_u8(msg, &type)) != 0)
+                        goto out;
+                if (type == SSH_AGENT_EXTENSION_FAILURE)
+                        r = SSH_ERR_AGENT_FAILURE;
+                else
+                        r = decode_reply(type);
+                if (r)
+                        goto out;
+                if ((r = sshbuf_get_cstring(msg, &namelist, NULL)) != 0)
+                        goto out;
+                sshbuf_reset (msg);
+        }
+
+ if ((r = sshbuf_put_u8(msg, SSH_AGENTC_EXTENSION)) != 0 ||
+    (r = sshbuf_put_cstring(msg, "[hidden email]")) != 0)
+ goto out;
+
+        if (namelist != NULL) {
+                ptr = namelist;
+                while ((p = strsep (&ptr, ","))) {
+                        if ((r = put_one_env (msg, p)))
+                                goto out;
+                }
+        }
+        else {
+                for (i = 0; i < num_env; i++) {
+                        if ((r = put_one_env (msg, env[i])))
+                                goto out;
+                }
+        }
+
+ if ((r = ssh_request_reply(sock, msg, msg)) != 0)
+ goto out;
+ if ((r = sshbuf_get_u8(msg, &type)) != 0)
+ goto out;
+ if (type == SSH_AGENT_EXTENSION_FAILURE)
+ r = SSH_ERR_AGENT_FAILURE;
+        else
+                r = decode_reply(type);
+ out:
+ sshbuf_free(msg);
+        free (namelist);
+ return r;
+
+}
+
 /* Lock/unlock agent */
 int
 ssh_lock_agent(int sock, int lock, const char *password)
diff --git a/authfd.h b/authfd.h
index 4fbf82f8..20cc108b 100644
--- a/authfd.h
+++ b/authfd.h
@@ -27,6 +27,7 @@ int ssh_get_authentication_socket(int *fdp);
 int ssh_get_authentication_socket_path(const char *authsocket, int *fdp);
 void ssh_close_authentication_socket(int sock);
 
+int     ssh_send_agent_env (int sock, char **env, int num_env);
 int ssh_lock_agent(int sock, int lock, const char *password);
 int ssh_fetch_identitylist(int sock, struct ssh_identitylist **idlp);
 void ssh_free_identitylist(struct ssh_identitylist *idl);
@@ -75,6 +76,8 @@ int ssh_agent_sign(int sock, const struct sshkey *key,
 #define SSH_AGENTC_ADD_RSA_ID_CONSTRAINED 24
 #define SSH2_AGENTC_ADD_ID_CONSTRAINED 25
 #define SSH_AGENTC_ADD_SMARTCARD_KEY_CONSTRAINED 26
+#define SSH_AGENTC_EXTENSION                    27
+#define SSH_AGENT_EXTENSION_FAILURE 28
 
 #define SSH_AGENT_CONSTRAIN_LIFETIME 1
 #define SSH_AGENT_CONSTRAIN_CONFIRM 2
diff --git a/readconf.c b/readconf.c
index c7df93de..50a79e17 100644
--- a/readconf.c
+++ b/readconf.c
@@ -172,7 +172,7 @@ typedef enum {
  oStreamLocalBindMask, oStreamLocalBindUnlink, oRevokedHostKeys,
  oFingerprintHash, oUpdateHostkeys, oHostbasedKeyTypes,
  oPubkeyAcceptedAlgorithms, oCASignatureAlgorithms, oProxyJump,
- oSecurityKeyProvider, oKnownHostsCommand,
+ oSecurityKeyProvider, oKnownHostsCommand, oAgentEnv,
  oIgnore, oIgnoredUnknownOption, oDeprecated, oUnsupported
 } OpCodes;
 
@@ -313,11 +313,22 @@ static struct {
  { "proxyjump", oProxyJump },
  { "securitykeyprovider", oSecurityKeyProvider },
  { "knownhostscommand", oKnownHostsCommand },
+        { "agentenv", oAgentEnv },
 
  { NULL, oBadOption }
 };
 
 
+/* Free the array A which holds N items and is indexed with a
+ * variable of TYPE.  */
+#define FREE_ARRAY(type, n, a) \
+ do { \
+ type _i; \
+ for (_i = 0; _i < (n); _i++) \
+ free((a)[_i]); \
+ } while (0)
+
+
 const char *
 kex_default_pk_alg(void)
 {
@@ -766,6 +777,7 @@ rm_env(Options *options, const char *arg, const char *filename, int linenum)
  }
 }
 
+
 /*
  * Returns the number of the token pointed to by cp or oBadOption.
  */
@@ -2001,6 +2013,49 @@ parse_pubkey_algos:
  *charptr = xstrdup(arg);
  break;
 
+ case oAgentEnv:
+ while ((arg = strdelim(&s)) != NULL && *arg != '\0') {
+ if (strchr(arg, '=') != NULL) {
+ error("%s line %d: Invalid environment name.",
+    filename, linenum);
+ return -1;
+ }
+ if (!*activep || options->no_more_agent_env)
+ continue;
+ if ((*arg == '-' || *arg == '#') && arg[1]) {
+                                error("%s line %d: Invalid environment name.",
+                                      filename, linenum);
+ return -1;
+                        }
+ if (*arg == '-') {
+ /* Remove all names */
+                                if (options->num_agent_env) {
+                                        FREE_ARRAY(int, options->num_agent_env,
+                                                   options->agent_env);
+                                        free(options->agent_env);
+                                        options->agent_env = NULL;
+                                        options->num_agent_env = 0;
+                                }
+ continue;
+                        }
+                        if (*arg == '#') {
+                                options->no_more_agent_env = 1;
+                                continue;
+                        }
+                        if (options->num_agent_env >= INT_MAX) {
+                          error("%s line %d: too many agent env.",
+                                filename, linenum);
+                          return -1;
+                        }
+                        options->agent_env = xrecallocarray(
+    options->agent_env, options->num_agent_env,
+    options->num_agent_env + 1,
+    sizeof(*options->agent_env));
+                        options->agent_env[options->num_agent_env++] =
+                                    xstrdup(arg);
+ }
+ break;
+
  case oDeprecated:
  debug("%s line %d: Deprecated option \"%s\"",
     filename, linenum, keyword);
@@ -2193,6 +2248,9 @@ initialize_options(Options * options)
  options->num_send_env = 0;
  options->setenv = NULL;
  options->num_setenv = 0;
+ options->agent_env = NULL;
+ options->num_agent_env = 0;
+ options->no_more_agent_env = 0;
  options->control_path = NULL;
  options->control_master = -1;
  options->control_persist = -1;
@@ -2496,13 +2554,6 @@ free_options(Options *o)
  if (o == NULL)
  return;
 
-#define FREE_ARRAY(type, n, a) \
- do { \
- type _i; \
- for (_i = 0; _i < (n); _i++) \
- free((a)[_i]); \
- } while (0)
-
  free(o->forward_agent_sock_path);
  free(o->xauth_location);
  FREE_ARRAY(u_int, o->num_log_verbose, o->log_verbose);
@@ -2551,6 +2602,8 @@ free_options(Options *o)
  free(o->send_env);
  FREE_ARRAY(int, o->num_setenv, o->setenv);
  free(o->setenv);
+ FREE_ARRAY(int, o->num_agent_env, o->agent_env);
+ free(o->agent_env);
  free(o->control_path);
  free(o->local_command);
  free(o->remote_command);
@@ -2567,7 +2620,6 @@ free_options(Options *o)
  free(o->jump_extra);
  free(o->ignored_unknown);
  explicit_bzero(o, sizeof(*o));
-#undef FREE_ARRAY
 }
 
 struct fwdarg {
@@ -3120,6 +3172,7 @@ dump_client_config(Options *o, const char *host)
  dump_cfg_strarray_oneline(oUserKnownHostsFile, o->num_user_hostfiles, o->user_hostfiles);
  dump_cfg_strarray(oSendEnv, o->num_send_env, o->send_env);
  dump_cfg_strarray(oSetEnv, o->num_setenv, o->setenv);
+ dump_cfg_strarray(oAgentEnv, o->num_agent_env, o->agent_env);
  dump_cfg_strarray_oneline(oLogVerbose,
     o->num_log_verbose, o->log_verbose);
 
diff --git a/readconf.h b/readconf.h
index 4ee730b9..e0b5f370 100644
--- a/readconf.h
+++ b/readconf.h
@@ -126,6 +126,10 @@ typedef struct {
  char   **send_env;
  int     num_setenv;
  char   **setenv;
+        int     no_more_agent_env;
+   int     num_agent_env;
+ char   **agent_env;
+
 
  char *control_path;
  int control_master;
diff --git a/ssh-add.c b/ssh-add.c
index 7edb9f9a..c6a9b2ce 100644
--- a/ssh-add.c
+++ b/ssh-add.c
@@ -780,6 +780,10 @@ main(int argc, char **argv)
  }
  log_init(__progname, log_level, log_facility, 1);
 
+        /* FIXME: Read the AgentEnv option from ssh_config and call
+         * ssh_send_agent_env (int sock, char **env, int num_env)
+         */
+
  if ((xflag != 0) + (lflag != 0) + (Dflag != 0) > 1)
  fatal("Invalid combination of actions");
  else if (xflag) {
diff --git a/ssh-keygen.c b/ssh-keygen.c
index cfb5f115..331ad7f6 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -2622,6 +2622,8 @@ sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
  goto done;
  }
 
+        /* FIXME: Handle AgentEnv */
+
  if ((r = ssh_get_authentication_socket(&agent_fd)) != 0)
  debug_r(r, "Couldn't get agent socket");
  else {
diff --git a/ssh_config b/ssh_config
index 842ea866..05c5539b 100644
--- a/ssh_config
+++ b/ssh_config
@@ -44,3 +44,6 @@
 #   ProxyCommand ssh -q -W %h:%p gateway.example.com
 #   RekeyLimit 1G 1h
 #   UserKnownHostsFile ~/.ssh/known_hosts.d/%k
+
+# For use with GnuPG's ssh-agent implementation use
+#   AgentEnv auto
diff --git a/ssh_config.5 b/ssh_config.5
index 96d6f658..fd33e773 100644
--- a/ssh_config.5
+++ b/ssh_config.5
@@ -270,6 +270,23 @@ Valid arguments are
 (use IPv4 only), or
 .Cm inet6
 (use IPv6 only).
+.It Cm AgentEnv
+Specifies what variables from the local
+.Xr environ 7
+should be sent to a running ssh-agent(1).
+The agent may use these environment variables at its own discretion.
+Note that patterns for the variable names are not supported.  To empty
+the list of previously set
+.Cm AgentEnv
+variable names the special name
+.Pa -
+may be used.  To ignore all further set names use the special name
+.Pa # .
+To ask the agent for a list of names to send use
+.Pa auto
+as the first and only item.
+.Pp
+The default is not to send any environment variables to the agent.
 .It Cm BatchMode
 If set to
 .Cm yes ,
diff --git a/sshconnect.c b/sshconnect.c
index 616ee37e..adce5d80 100644
--- a/sshconnect.c
+++ b/sshconnect.c
@@ -1718,6 +1718,12 @@ maybe_add_key_to_agent(const char *authfile, struct sshkey *private,
  }
  if (sshkey_is_sk(private))
  skprovider = options.sk_provider;
+        if ((r = ssh_send_agent_env (auth_sock, options.agent_env,
+                                     options.num_agent_env)) != 0) {
+                debug("agent does not support AgentEnv: (%d)", r);
+                close(auth_sock);
+                return;
+        }
  if ((r = ssh_add_identity_constrained(auth_sock, private,
     comment == NULL ? authfile : comment,
     options.add_keys_to_agent_lifespan,
diff --git a/sshconnect2.c b/sshconnect2.c
index de89b761..39302829 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -1623,6 +1623,29 @@ key_type_allowed_by_config(struct sshkey *key)
 }
 
 
+/* Helper for pubkey_prepare.  */
+static int
+authentication_socket(int *fdp)
+{
+        int r;
+        int sock;
+
+ if ((r = ssh_get_authentication_socket(&sock)) == 0) {
+                if ((r = ssh_send_agent_env (sock, options.agent_env,
+                                             options.num_agent_env)) != 0) {
+                        debug("agent does not support AgentEnv: (%d)", r);
+                        close(sock);
+                        *fdp = -1;
+                }
+        }
+
+        if (!r) {
+ *fdp = sock;
+        }
+
+        return r;
+}
+
 /*
  * try keys in the following order:
  * 1. certificates listed in the config file
@@ -1694,7 +1717,7 @@ pubkey_prepare(Authctxt *authctxt)
  TAILQ_INSERT_TAIL(preferred, id, next);
  }
  /* list of keys supported by the agent */
- if ((r = ssh_get_authentication_socket(&agent_fd)) != 0) {
+ if ((r = authentication_socket(&agent_fd)) != 0) {
  if (r != SSH_ERR_AGENT_NOT_PRESENT)
  debug_fr(r, "ssh_get_authentication_socket");
  } else if ((r = ssh_fetch_identitylist(agent_fd, &idlist)) != 0) {
--
2.20.1


_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

signature.asc (233 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Sending envvars via ssh agent protocol

Ángel
On 2021-01-28 at 09:24 +0100, Werner Koch via Gnupg-devel wrote:
> The rationale for the "-" thingy is to allow a config file to
> override what for example the command line has already set.  The "#"
> can be used to disable a globally set option from the commandline or
> ~/.ssh/config.

This seems backwards. I would expect command line to have hidher
priority than ssh_config, not ~/.ssh/config to be able to disable an
explicit command line option.
However, this may be useful for ~/.ssh/configto override
/etc/ssh/ssh_config (or as a command line parameter -oAcceptEnv=-)

Also, I would suggest using none instead of -, as that's what other ssh
options use. (This might cause issues if you wanted to pass an
environment variable named "none", but the same problem already exists
for "auto")


On a quick review of the patch:


> @@ -313,11 +313,22 @@ static struct {
>         { "proxyjump", oProxyJump },
>         { "securitykeyprovider", oSecurityKeyProvider },
>         { "knownhostscommand", oKnownHostsCommand },
> +        { "agentenv", oAgentEnv },

There is an extra space identation here.


> +                               error("%s line %d: Invalid environment name.",
Maybe nitpicking, but on this error (appears twice) I would say
"Invalid name of environment variable". The environment would be the
whole block of variables and values, not just one variable.


> if (*arg == '-') {

> if (*arg == '#') {

You are comparing against the first character of the argument.
Per your description I would expect that you compared that the whole
was that, not just that it began with # or -


And particularly, I can easily see how one might want to prefix an
environment variable with a minus to *substract* it from the set of
accepted vars.


+                        if (options->num_agent_env >= INT_MAX) {

It is a bit strange to compare >= INT_MAX, since num_agent_env is an
int, but after reviewing, you only need the == comparison, so probably
ok.

There are more indentation sheningans around this block.


> +++ b/readconf.h
> @@ -126,6 +126,10 @@ typedef struct {
>   char   **send_env;
>   int     num_setenv;
>   char   **setenv;
> +        int     no_more_agent_env;
> +   int     num_agent_env;
> + char   **agent_env;
> +

Bad indentation. send_env, num_setenv and setenv are indented with a
tab, no_more_agent_env with 8 spaces, num_agent_env with 3 spaces and
agent_env with a tab.


The fixme comments of ssh-add.c and ssh-keygen.c also use 8 spaces
instead of a tab (but these should probably end up implemented).

> +++ b/sshconnect.c
> @@ -1718,6 +1718,12 @@ maybe_add_key_to_agent(const char *authfile,
> struct sshkey *private,
>   }
>   if (sshkey_is_sk(private))
>   skprovider = options.sk_provider;
> +        if ((r = ssh_send_agent_env (auth_sock, options.agent_env,
> +                                     options.num_agent_env)) != 0) {
> +                debug("agent does not support AgentEnv: (%d)", r);
> +                close(auth_sock);
> +                return;
> +        }

Indentation with 8 spaces, not with tabs

> +++ b/sshconnect2.c
>
> +/* Helper for pubkey_prepare.  */
> +static int
> +authentication_socket(int *fdp)

More indentation errors (there is a mixture in the function itself)


Best regards



_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel
Reply | Threaded
Open this post in threaded view
|

Re: Sending envvars via ssh agent protocol

Ángel
On 2021-01-29 at 00:06 +0100, Ángel wrote:
> (lots of notes about indentation on previos patch)
>

I attach a revised version of the patch addressing the indentation
issues. It has only spacing changes from Werner's original patch.

Best regards



_______________________________________________
Gnupg-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnupg-devel

attachment0 (13K) Download Attachment