Mailing List Archive

Sending envvars via ssh agent protocol
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.
Re: Sending envvars via ssh agent protocol [ In reply to ]
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
Gnupg-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-devel
Re: Sending envvars via ssh agent protocol [ In reply to ]
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
Re: Sending envvars via ssh agent protocol [ In reply to ]
On Fri, 29 Jan 2021 00:06, Ángel said:

> This seems backwards. I would expect command line to have hidher
> priority than ssh_config, not ~/.ssh/config to be able to disable an

I also wondered about this but it is clearly stated at the top of
ssh_config(5)

> 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")

I agree, lowercase envvars and in particular "none" and "auto" should be
rearly be exportable.

>> + 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

Its longer but fir sure more correct.

>> 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 -

You need to look at the previous condition:

if ((*arg == '-' || *arg == '#') && arg[1]) {
ERROR-RETURN

> 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.

You like in SendEnv. I decded against doing this because the number of
envvars to send here should be pretty limited and does not need for more
complex code.

> + if (options->num_agent_env >= INT_MAX) {
>
> It is a bit strange to compare >= INT_MAX, since num_agent_env is an

Copied from SendEnv

> 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.

I guess I did not read the hacking guide completely. Frankly I didn't
expect that any software these days still uses tabs to compress the
source. I consider invisible characters a no-go unless POSIX says to
use them (Makefile ungliness). Thanks for the patch in your other mail.

> 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).

(See above.)


Any other technical opinions?



Salam-Shalom,

Werner



--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.
Re: Sending envvars via ssh agent protocol [ In reply to ]
On Sonntag, 7. M?rz 2021 10:20:30 CET Werner Koch via Gnupg-devel wrote:
> On Fri, 29 Jan 2021 00:06, ?ngel said:
> > This seems backwards. I would expect command line to have hidher
> > priority than ssh_config, not ~/.ssh/config to be able to disable an
>
> I also wondered about this but it is clearly stated at the top of
> ssh_config(5)

At least on my system, ssh_config(5) states that command-line options have the
highest priority:
=====
ssh(1) obtains configuration data from the following sources in the following
order:
1. command-line options
2. user's configuration file (~/.ssh/config)
3. system-wide configuration file (/etc/ssh/ssh_config)
For each parameter, the first obtained value will be used.
=====

Regards,
Ingo
Re: Sending envvars via ssh agent protocol [ In reply to ]
On Sun, 7 Mar 2021 21:39, Ingo Klöcker said:

> At least on my system, ssh_config(5) states that command-line options
> have the highest priority:

You are right. Sorry for the confusion.


Salam-Shalom,

Werner

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